[PATCH 0/1] Use PSCI OS initiated mode for sc7280

[PATCH 0/1] Use PSCI OS initiated mode for sc7280

[Maulik Shah]

This change adds power-domains for cpuidle states to use PSCI OS
initiated mode for sc7280.

This change depends on external project changes [1] & [2] which are under
review/discussion to add PSCI os-initiated support in Arm Trusted Firmware.

I can update here once the dependency are in and change is ready to merge.

[1] https://review.trustedfirmware.org/q/topic:psci-osi
[2] https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/19487

Maulik Shah (1):
  arm64: dts: qcom: sc7280: Add power-domains for cpuidle states

 arch/arm64/boot/dts/qcom/sc7280.dtsi | 96 +++++++++++++++++++++-------
 1 file changed, 72 insertions(+), 24 deletions(-)

-- 
2.17.1

[PATCH 1/1] arm64: dts: qcom: sc7280: Add power-domains for cpuidle states

[Maulik Shah]

Add power-domains for cpuidle states to use psci os-initiated idle states.

Signed-off-by: Maulik Shah <quic_mkshah@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc7280.dtsi | 96 +++++++++++++++++++++-------
 1 file changed, 72 insertions(+), 24 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index bdcb74925313..08ec96fc8eee 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -169,9 +169,8 @@
 			compatible = "qcom,kryo";
 			reg = <0x0 0x0>;
 			enable-method = "psci";
-			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
-					   &LITTLE_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD0>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_0>;
 			operating-points-v2 = <&cpu0_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
@@ -194,9 +193,8 @@
 			compatible = "qcom,kryo";
 			reg = <0x0 0x100>;
 			enable-method = "psci";
-			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
-					   &LITTLE_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD1>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_100>;
 			operating-points-v2 = <&cpu0_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
@@ -215,9 +213,8 @@
 			compatible = "qcom,kryo";
 			reg = <0x0 0x200>;
 			enable-method = "psci";
-			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
-					   &LITTLE_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD2>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_200>;
 			operating-points-v2 = <&cpu0_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
@@ -236,9 +233,8 @@
 			compatible = "qcom,kryo";
 			reg = <0x0 0x300>;
 			enable-method = "psci";
-			cpu-idle-states = <&LITTLE_CPU_SLEEP_0
-					   &LITTLE_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD3>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_300>;
 			operating-points-v2 = <&cpu0_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
@@ -257,9 +253,8 @@
 			compatible = "qcom,kryo";
 			reg = <0x0 0x400>;
 			enable-method = "psci";
-			cpu-idle-states = <&BIG_CPU_SLEEP_0
-					   &BIG_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD4>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_400>;
 			operating-points-v2 = <&cpu4_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
@@ -278,9 +273,8 @@
 			compatible = "qcom,kryo";
 			reg = <0x0 0x500>;
 			enable-method = "psci";
-			cpu-idle-states = <&BIG_CPU_SLEEP_0
-					   &BIG_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD5>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_500>;
 			operating-points-v2 = <&cpu4_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
@@ -299,9 +293,8 @@
 			compatible = "qcom,kryo";
 			reg = <0x0 0x600>;
 			enable-method = "psci";
-			cpu-idle-states = <&BIG_CPU_SLEEP_0
-					   &BIG_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD6>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_600>;
 			operating-points-v2 = <&cpu4_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
@@ -320,9 +313,8 @@
 			compatible = "qcom,kryo";
 			reg = <0x0 0x700>;
 			enable-method = "psci";
-			cpu-idle-states = <&BIG_CPU_SLEEP_0
-					   &BIG_CPU_SLEEP_1
-					   &CLUSTER_SLEEP_0>;
+			power-domains = <&CPU_PD7>;
+			power-domain-names = "psci";
 			next-level-cache = <&L2_700>;
 			operating-points-v2 = <&cpu7_opp_table>;
 			interconnects = <&gem_noc MASTER_APPSS_PROC 3 &mc_virt SLAVE_EBI1 3>,
@@ -414,7 +406,9 @@
 				min-residency-us = <5555>;
 				local-timer-stop;
 			};
+		};
 
+		domain-idle-states {
 			CLUSTER_SLEEP_0: cluster-sleep-0 {
 				compatible = "arm,idle-state";
 				idle-state-name = "cluster-power-down";
@@ -782,6 +776,59 @@
 	psci {
 		compatible = "arm,psci-1.0";
 		method = "smc";
+
+		CPU_PD0: cpu0 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;
+		};
+
+		CPU_PD1: cpu1 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;
+		};
+
+		CPU_PD2: cpu2 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;
+		};
+
+		CPU_PD3: cpu3 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;
+		};
+
+		CPU_PD4: cpu4 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;
+		};
+
+		CPU_PD5: cpu5 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;
+		};
+
+		CPU_PD6: cpu6 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;
+		};
+
+		CPU_PD7: cpu7 {
+			#power-domain-cells = <0>;
+			power-domains = <&CLUSTER_PD>;
+			domain-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;
+		};
+
+		CLUSTER_PD: cpu-cluster0 {
+			#power-domain-cells = <0>;
+			domain-idle-states = <&CLUSTER_SLEEP_0>;
+		};
 	};
 
 	qspi_opp_table: opp-table-qspi {
@@ -5262,6 +5309,7 @@
 					  <SLEEP_TCS   3>,
 					  <WAKE_TCS    3>,
 					  <CONTROL_TCS 1>;
+			power-domains = <&CLUSTER_PD>;
 
 			apps_bcm_voter: bcm-voter {
 				compatible = "qcom,bcm-voter";
-- 
2.17.1

Re: [PATCH 0/1] Use PSCI OS initiated mode for sc7280

[Bjorn Andersson]

On Wed, Feb 15, 2023 at 12:46:48PM +0530, Maulik Shah wrote:
> This change adds power-domains for cpuidle states to use PSCI OS
> initiated mode for sc7280.
> 
> This change depends on external project changes [1] & [2] which are under
> review/discussion to add PSCI os-initiated support in Arm Trusted Firmware.
> 
> I can update here once the dependency are in and change is ready to merge.
> 

Please do, I will drop this from the queue for now.

Thanks,
Bjorn

> [1] https://review.trustedfirmware.org/q/topic:psci-osi
> [2] https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/19487
> 
> Maulik Shah (1):
>   arm64: dts: qcom: sc7280: Add power-domains for cpuidle states
> 
>  arch/arm64/boot/dts/qcom/sc7280.dtsi | 96 +++++++++++++++++++++-------
>  1 file changed, 72 insertions(+), 24 deletions(-)
> 
> -- 
> 2.17.1
> 

Re: [PATCH 0/1] Use PSCI OS initiated mode for sc7280

[Doug Anderson]

Hi,

On Mon, Feb 27, 2023 at 7:35 AM Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Wed, Feb 15, 2023 at 12:46:48PM +0530, Maulik Shah wrote:
> > This change adds power-domains for cpuidle states to use PSCI OS
> > initiated mode for sc7280.
> >
> > This change depends on external project changes [1] & [2] which are under
> > review/discussion to add PSCI os-initiated support in Arm Trusted Firmware.
> >
> > I can update here once the dependency are in and change is ready to merge.
> >
>
> Please do, I will drop this from the queue for now.

I'm a bit confused about why we're doing this. There's always been a
question about exactly why we need OSI mode. As far as I can tell it
can't be for "correctness" reasons because we managed to ship sc7180
without OSI mode. ...so I guess somehow the argument is that OSI mode
is more performant in some cases? Are there actual numbers backing
this up, or is it all theoretical? Before making such a big change, it
would be good to actually understand what the motivation is and see
real data. This should be easy to collect since we currently have
things working without OSI and (presumably) you have OSI working. It
would also be good to document this motivation in the commit message
and/or cover letter.

-Doug

Re: [PATCH 0/1] Use PSCI OS initiated mode for sc7280

[Ulf Hansson]

On Mon, 27 Feb 2023 at 17:10, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Feb 27, 2023 at 7:35 AM Bjorn Andersson <andersson@kernel.org> wrote:
> >
> > On Wed, Feb 15, 2023 at 12:46:48PM +0530, Maulik Shah wrote:
> > > This change adds power-domains for cpuidle states to use PSCI OS
> > > initiated mode for sc7280.
> > >
> > > This change depends on external project changes [1] & [2] which are under
> > > review/discussion to add PSCI os-initiated support in Arm Trusted Firmware.
> > >
> > > I can update here once the dependency are in and change is ready to merge.
> > >
> >
> > Please do, I will drop this from the queue for now.
>
> I'm a bit confused about why we're doing this. There's always been a
> question about exactly why we need OSI mode. As far as I can tell it
> can't be for "correctness" reasons because we managed to ship sc7180
> without OSI mode. ...so I guess somehow the argument is that OSI mode
> is more performant in some cases? Are there actual numbers backing
> this up, or is it all theoretical? Before making such a big change, it
> would be good to actually understand what the motivation is and see
> real data. This should be easy to collect since we currently have
> things working without OSI and (presumably) you have OSI working. It
> would also be good to document this motivation in the commit message
> and/or cover letter.

I certainly don't object to what you say here. Although, let me also
share some more background to these suggested changes.

As you know, for mobile platforms, Qcom have been using OS-initiated
mode for years, but on Chromium platforms that has been limited to the
default platform-coordinated mode. Whether that is a deliberate
decision for the Chromium platforms or rather because the PSCI
implementation in TF-A has been lacking OSI support, I don't know.
Maybe you have some more insight to share around this?

Note that, Wing has been working on adding support for PSCI OSI mode
to TF-A [1], which hopefully should land soon. In this regard, it
seems like we are getting closer to finally being able to run some
more in-depth tests, that should allow us to better compare the
behaviour of the PSCI CPU-suspend modes - at least on some platforms.
In fact, Maulik/Wing also presented their work around this topic,
including some results around performance/energy tests at the last
TF-A call [2]. I think some of that data could be shared in the commit
message too.

Kind regards
Uffe

[1]
https://review.trustedfirmware.org/q/topic:psci-osi

[2]
https://www.trustedfirmware.org/meetings/tf-a-technical-forum

Re: [PATCH 0/1] Use PSCI OS initiated mode for sc7280

[Doug Anderson]

Hi,

On Tue, Feb 28, 2023 at 4:17 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Mon, 27 Feb 2023 at 17:10, Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Mon, Feb 27, 2023 at 7:35 AM Bjorn Andersson <andersson@kernel.org> wrote:
> > >
> > > On Wed, Feb 15, 2023 at 12:46:48PM +0530, Maulik Shah wrote:
> > > > This change adds power-domains for cpuidle states to use PSCI OS
> > > > initiated mode for sc7280.
> > > >
> > > > This change depends on external project changes [1] & [2] which are under
> > > > review/discussion to add PSCI os-initiated support in Arm Trusted Firmware.
> > > >
> > > > I can update here once the dependency are in and change is ready to merge.
> > > >
> > >
> > > Please do, I will drop this from the queue for now.
> >
> > I'm a bit confused about why we're doing this. There's always been a
> > question about exactly why we need OSI mode. As far as I can tell it
> > can't be for "correctness" reasons because we managed to ship sc7180
> > without OSI mode. ...so I guess somehow the argument is that OSI mode
> > is more performant in some cases? Are there actual numbers backing
> > this up, or is it all theoretical? Before making such a big change, it
> > would be good to actually understand what the motivation is and see
> > real data. This should be easy to collect since we currently have
> > things working without OSI and (presumably) you have OSI working. It
> > would also be good to document this motivation in the commit message
> > and/or cover letter.
>
> I certainly don't object to what you say here. Although, let me also
> share some more background to these suggested changes.
>
> As you know, for mobile platforms, Qcom have been using OS-initiated
> mode for years, but on Chromium platforms that has been limited to the
> default platform-coordinated mode. Whether that is a deliberate
> decision for the Chromium platforms or rather because the PSCI
> implementation in TF-A has been lacking OSI support, I don't know.
> Maybe you have some more insight to share around this?

You hit the reason exactly. Nobody on the ChromeOS team objected to
OSI, per say, but it was never supported in ARM Trusted Firmware. I
still don't have anything against OSI mode, but I just want to make
sure that the data is there and that we're not just arbitrarily
churning things. ;-)

I think sc7180's ship has sailed at this point. While we could update
the firmware for testing, I don't think we'd switch production sc7180
devices over to OSI. That means that we'll always need to support PC
mode for sc7180. Switching sc7280 over to OSI needs to be justified
given that we'll have to continue to support sc7180 with PC mode
anyway.


> Note that, Wing has been working on adding support for PSCI OSI mode
> to TF-A [1], which hopefully should land soon. In this regard, it
> seems like we are getting closer to finally being able to run some
> more in-depth tests, that should allow us to better compare the
> behaviour of the PSCI CPU-suspend modes - at least on some platforms.
> In fact, Maulik/Wing also presented their work around this topic,
> including some results around performance/energy tests at the last
> TF-A call [2]. I think some of that data could be shared in the commit
> message too.

Yup, I was mostly just asking for data like you provided to be in the
commit message.


> Kind regards
> Uffe
>
> [1]
> https://review.trustedfirmware.org/q/topic:psci-osi
>
> [2]
> https://www.trustedfirmware.org/meetings/tf-a-technical-forum

Re: [PATCH 0/1] Use PSCI OS initiated mode for sc7280

[Ulf Hansson]

On Tue, 28 Feb 2023 at 16:25, Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Feb 28, 2023 at 4:17 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Mon, 27 Feb 2023 at 17:10, Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Mon, Feb 27, 2023 at 7:35 AM Bjorn Andersson <andersson@kernel.org> wrote:
> > > >
> > > > On Wed, Feb 15, 2023 at 12:46:48PM +0530, Maulik Shah wrote:
> > > > > This change adds power-domains for cpuidle states to use PSCI OS
> > > > > initiated mode for sc7280.
> > > > >
> > > > > This change depends on external project changes [1] & [2] which are under
> > > > > review/discussion to add PSCI os-initiated support in Arm Trusted Firmware.
> > > > >
> > > > > I can update here once the dependency are in and change is ready to merge.
> > > > >
> > > >
> > > > Please do, I will drop this from the queue for now.
> > >
> > > I'm a bit confused about why we're doing this. There's always been a
> > > question about exactly why we need OSI mode. As far as I can tell it
> > > can't be for "correctness" reasons because we managed to ship sc7180
> > > without OSI mode. ...so I guess somehow the argument is that OSI mode
> > > is more performant in some cases? Are there actual numbers backing
> > > this up, or is it all theoretical? Before making such a big change, it
> > > would be good to actually understand what the motivation is and see
> > > real data. This should be easy to collect since we currently have
> > > things working without OSI and (presumably) you have OSI working. It
> > > would also be good to document this motivation in the commit message
> > > and/or cover letter.
> >
> > I certainly don't object to what you say here. Although, let me also
> > share some more background to these suggested changes.
> >
> > As you know, for mobile platforms, Qcom have been using OS-initiated
> > mode for years, but on Chromium platforms that has been limited to the
> > default platform-coordinated mode. Whether that is a deliberate
> > decision for the Chromium platforms or rather because the PSCI
> > implementation in TF-A has been lacking OSI support, I don't know.
> > Maybe you have some more insight to share around this?
>
> You hit the reason exactly. Nobody on the ChromeOS team objected to
> OSI, per say, but it was never supported in ARM Trusted Firmware. I
> still don't have anything against OSI mode, but I just want to make
> sure that the data is there and that we're not just arbitrarily
> churning things. ;-)

Thanks for sharing this information! It certainly helps to better
understand the background for all of us.

>
> I think sc7180's ship has sailed at this point. While we could update
> the firmware for testing, I don't think we'd switch production sc7180
> devices over to OSI. That means that we'll always need to support PC
> mode for sc7180. Switching sc7280 over to OSI needs to be justified
> given that we'll have to continue to support sc7180 with PC mode
> anyway.

Right. Supporting both OSI and PC mode, dynamically based upon what
the PSCI firmware (TF-A) supports should work fine from Linux point of
view.

I may be overlooking something for this particular case - and in that
case, I am happy to help to fix it!

>
>
> > Note that, Wing has been working on adding support for PSCI OSI mode
> > to TF-A [1], which hopefully should land soon. In this regard, it
> > seems like we are getting closer to finally being able to run some
> > more in-depth tests, that should allow us to better compare the
> > behaviour of the PSCI CPU-suspend modes - at least on some platforms.
> > In fact, Maulik/Wing also presented their work around this topic,
> > including some results around performance/energy tests at the last
> > TF-A call [2]. I think some of that data could be shared in the commit
> > message too.
>
> Yup, I was mostly just asking for data like you provided to be in the
> commit message.

Great, thanks for confirming!

>
>
> > Kind regards
> > Uffe
> >
> > [1]
> > https://review.trustedfirmware.org/q/topic:psci-osi
> >
> > [2]
> > https://www.trustedfirmware.org/meetings/tf-a-technical-forum

Kind regards
Uffe