[PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support

[PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support

[Niklas Cassel]

Add device bindings for CPUs to suspend using PSCI as the enable-method.

Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
---
 arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
index ffedf9640af7..f9db9f3ee10c 100644
--- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
@@ -31,6 +31,7 @@
 			reg = <0x100>;
 			enable-method = "psci";
 			next-level-cache = <&L2_0>;
+			cpu-idle-states = <&CPU_PC>;
 		};
 
 		CPU1: cpu@101 {
@@ -39,6 +40,7 @@
 			reg = <0x101>;
 			enable-method = "psci";
 			next-level-cache = <&L2_0>;
+			cpu-idle-states = <&CPU_PC>;
 		};
 
 		CPU2: cpu@102 {
@@ -47,6 +49,7 @@
 			reg = <0x102>;
 			enable-method = "psci";
 			next-level-cache = <&L2_0>;
+			cpu-idle-states = <&CPU_PC>;
 		};
 
 		CPU3: cpu@103 {
@@ -55,12 +58,24 @@
 			reg = <0x103>;
 			enable-method = "psci";
 			next-level-cache = <&L2_0>;
+			cpu-idle-states = <&CPU_PC>;
 		};
 
 		L2_0: l2-cache {
 			compatible = "cache";
 			cache-level = <2>;
 		};
+
+		idle-states {
+			CPU_PC: pc {
+				compatible = "arm,idle-state";
+				arm,psci-suspend-param = <0x40000003>;
+				entry-latency-us = <125>;
+				exit-latency-us = <180>;
+				min-residency-us = <595>;
+				local-timer-stop;
+			};
+		};
 	};
 
 	firmware {
-- 
2.21.0

Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support

[Vinod Koul]

On 06-05-19, 21:31, Niklas Cassel wrote:
> Add device bindings for CPUs to suspend using PSCI as the enable-method.
> 
> Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> index ffedf9640af7..f9db9f3ee10c 100644
> --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
> +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> @@ -31,6 +31,7 @@
>  			reg = <0x100>;
>  			enable-method = "psci";
>  			next-level-cache = <&L2_0>;
> +			cpu-idle-states = <&CPU_PC>;
>  		};
>  
>  		CPU1: cpu@101 {
> @@ -39,6 +40,7 @@
>  			reg = <0x101>;
>  			enable-method = "psci";
>  			next-level-cache = <&L2_0>;
> +			cpu-idle-states = <&CPU_PC>;
>  		};
>  
>  		CPU2: cpu@102 {
> @@ -47,6 +49,7 @@
>  			reg = <0x102>;
>  			enable-method = "psci";
>  			next-level-cache = <&L2_0>;
> +			cpu-idle-states = <&CPU_PC>;
>  		};
>  
>  		CPU3: cpu@103 {
> @@ -55,12 +58,24 @@
>  			reg = <0x103>;
>  			enable-method = "psci";
>  			next-level-cache = <&L2_0>;
> +			cpu-idle-states = <&CPU_PC>;
>  		};
>  
>  		L2_0: l2-cache {
>  			compatible = "cache";
>  			cache-level = <2>;
>  		};
> +
> +		idle-states {

Since we are trying to sort the file per address and
alphabetically, it would be great if this can be moved before l2-cache
:)

Other than that this lgtm
 
> +			CPU_PC: pc {
> +				compatible = "arm,idle-state";
> +				arm,psci-suspend-param = <0x40000003>;
> +				entry-latency-us = <125>;
> +				exit-latency-us = <180>;
> +				min-residency-us = <595>;
> +				local-timer-stop;
> +			};
> +		};
>  	};
>  
>  	firmware {
> -- 
> 2.21.0

-- 
~Vinod

Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support

[Bjorn Andersson]

On Mon 06 May 22:35 PDT 2019, Vinod Koul wrote:

> On 06-05-19, 21:31, Niklas Cassel wrote:
> > Add device bindings for CPUs to suspend using PSCI as the enable-method.
> > 
> > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > index ffedf9640af7..f9db9f3ee10c 100644
> > --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > @@ -31,6 +31,7 @@
> >  			reg = <0x100>;
> >  			enable-method = "psci";
> >  			next-level-cache = <&L2_0>;
> > +			cpu-idle-states = <&CPU_PC>;
> >  		};
> >  
> >  		CPU1: cpu@101 {
> > @@ -39,6 +40,7 @@
> >  			reg = <0x101>;
> >  			enable-method = "psci";
> >  			next-level-cache = <&L2_0>;
> > +			cpu-idle-states = <&CPU_PC>;
> >  		};
> >  
> >  		CPU2: cpu@102 {
> > @@ -47,6 +49,7 @@
> >  			reg = <0x102>;
> >  			enable-method = "psci";
> >  			next-level-cache = <&L2_0>;
> > +			cpu-idle-states = <&CPU_PC>;
> >  		};
> >  
> >  		CPU3: cpu@103 {
> > @@ -55,12 +58,24 @@
> >  			reg = <0x103>;
> >  			enable-method = "psci";
> >  			next-level-cache = <&L2_0>;
> > +			cpu-idle-states = <&CPU_PC>;
> >  		};
> >  
> >  		L2_0: l2-cache {
> >  			compatible = "cache";
> >  			cache-level = <2>;
> >  		};
> > +
> > +		idle-states {
> 
> Since we are trying to sort the file per address and
> alphabetically, it would be great if this can be moved before l2-cache
> :)
> 

Picked up, with the order adjusted.

> Other than that this lgtm
>  

I presume that lgtm == Reviewed-by...

Thanks,
Bjorn

> > +			CPU_PC: pc {
> > +				compatible = "arm,idle-state";
> > +				arm,psci-suspend-param = <0x40000003>;
> > +				entry-latency-us = <125>;
> > +				exit-latency-us = <180>;
> > +				min-residency-us = <595>;
> > +				local-timer-stop;
> > +			};
> > +		};
> >  	};
> >  
> >  	firmware {
> > -- 
> > 2.21.0
> 
> -- 
> ~Vinod

Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support

[Vinod Koul]

On 06-05-19, 23:55, Bjorn Andersson wrote:
> On Mon 06 May 22:35 PDT 2019, Vinod Koul wrote:
> 
> > On 06-05-19, 21:31, Niklas Cassel wrote:
> > > Add device bindings for CPUs to suspend using PSCI as the enable-method.
> > > 
> > > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
> > > ---
> > >  arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > > index ffedf9640af7..f9db9f3ee10c 100644
> > > --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > > @@ -31,6 +31,7 @@
> > >  			reg = <0x100>;
> > >  			enable-method = "psci";
> > >  			next-level-cache = <&L2_0>;
> > > +			cpu-idle-states = <&CPU_PC>;
> > >  		};
> > >  
> > >  		CPU1: cpu@101 {
> > > @@ -39,6 +40,7 @@
> > >  			reg = <0x101>;
> > >  			enable-method = "psci";
> > >  			next-level-cache = <&L2_0>;
> > > +			cpu-idle-states = <&CPU_PC>;
> > >  		};
> > >  
> > >  		CPU2: cpu@102 {
> > > @@ -47,6 +49,7 @@
> > >  			reg = <0x102>;
> > >  			enable-method = "psci";
> > >  			next-level-cache = <&L2_0>;
> > > +			cpu-idle-states = <&CPU_PC>;
> > >  		};
> > >  
> > >  		CPU3: cpu@103 {
> > > @@ -55,12 +58,24 @@
> > >  			reg = <0x103>;
> > >  			enable-method = "psci";
> > >  			next-level-cache = <&L2_0>;
> > > +			cpu-idle-states = <&CPU_PC>;
> > >  		};
> > >  
> > >  		L2_0: l2-cache {
> > >  			compatible = "cache";
> > >  			cache-level = <2>;
> > >  		};
> > > +
> > > +		idle-states {
> > 
> > Since we are trying to sort the file per address and
> > alphabetically, it would be great if this can be moved before l2-cache
> > :)
> > 
> 
> Picked up, with the order adjusted.
> 
> > Other than that this lgtm
> >  
> 
> I presume that lgtm == Reviewed-by...

yes :-) and formally here as well..

Reviewed-by: Vinod Koul <vkoul@kernel.org>

> 
> Thanks,
> Bjorn
> 
> > > +			CPU_PC: pc {
> > > +				compatible = "arm,idle-state";
> > > +				arm,psci-suspend-param = <0x40000003>;
> > > +				entry-latency-us = <125>;
> > > +				exit-latency-us = <180>;
> > > +				min-residency-us = <595>;
> > > +				local-timer-stop;
> > > +			};
> > > +		};
> > >  	};
> > >  
> > >  	firmware {
> > > -- 
> > > 2.21.0
> > 
> > -- 
> > ~Vinod

-- 
~Vinod

Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support

[Amit Kucheria]

On Tue, May 7, 2019 at 1:01 AM Niklas Cassel <niklas.cassel@linaro.org> wrote:
>
> Add device bindings for CPUs to suspend using PSCI as the enable-method.
>
> Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> index ffedf9640af7..f9db9f3ee10c 100644
> --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
> +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> @@ -31,6 +31,7 @@
>                         reg = <0x100>;
>                         enable-method = "psci";
>                         next-level-cache = <&L2_0>;
> +                       cpu-idle-states = <&CPU_PC>;
>                 };
>
>                 CPU1: cpu@101 {
> @@ -39,6 +40,7 @@
>                         reg = <0x101>;
>                         enable-method = "psci";
>                         next-level-cache = <&L2_0>;
> +                       cpu-idle-states = <&CPU_PC>;
>                 };
>
>                 CPU2: cpu@102 {
> @@ -47,6 +49,7 @@
>                         reg = <0x102>;
>                         enable-method = "psci";
>                         next-level-cache = <&L2_0>;
> +                       cpu-idle-states = <&CPU_PC>;
>                 };
>
>                 CPU3: cpu@103 {
> @@ -55,12 +58,24 @@
>                         reg = <0x103>;
>                         enable-method = "psci";
>                         next-level-cache = <&L2_0>;
> +                       cpu-idle-states = <&CPU_PC>;
>                 };
>
>                 L2_0: l2-cache {
>                         compatible = "cache";
>                         cache-level = <2>;
>                 };
> +
> +               idle-states {

entry-method="psci" property goes here. I have a patch fixing it for 410c ;-)

I don't think the psci_cpuidle_ops will even get called without this.
Did you see any changes in consumption with this patch? I was trying
to measure that before sending this out.

> +                       CPU_PC: pc {
> +                               compatible = "arm,idle-state";
> +                               arm,psci-suspend-param = <0x40000003>;
> +                               entry-latency-us = <125>;
> +                               exit-latency-us = <180>;
> +                               min-residency-us = <595>;
> +                               local-timer-stop;
> +                       };
> +               };
>         };
>
>         firmware {
> --
> 2.21.0
>

Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support

[Niklas Cassel]

On Wed, May 08, 2019 at 02:48:19AM +0530, Amit Kucheria wrote:
> On Tue, May 7, 2019 at 1:01 AM Niklas Cassel <niklas.cassel@linaro.org> wrote:
> >
> > Add device bindings for CPUs to suspend using PSCI as the enable-method.
> >
> > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > index ffedf9640af7..f9db9f3ee10c 100644
> > --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > @@ -31,6 +31,7 @@
> >                         reg = <0x100>;
> >                         enable-method = "psci";
> >                         next-level-cache = <&L2_0>;
> > +                       cpu-idle-states = <&CPU_PC>;
> >                 };
> >
> >                 CPU1: cpu@101 {
> > @@ -39,6 +40,7 @@
> >                         reg = <0x101>;
> >                         enable-method = "psci";
> >                         next-level-cache = <&L2_0>;
> > +                       cpu-idle-states = <&CPU_PC>;
> >                 };
> >
> >                 CPU2: cpu@102 {
> > @@ -47,6 +49,7 @@
> >                         reg = <0x102>;
> >                         enable-method = "psci";
> >                         next-level-cache = <&L2_0>;
> > +                       cpu-idle-states = <&CPU_PC>;
> >                 };
> >
> >                 CPU3: cpu@103 {
> > @@ -55,12 +58,24 @@
> >                         reg = <0x103>;
> >                         enable-method = "psci";
> >                         next-level-cache = <&L2_0>;
> > +                       cpu-idle-states = <&CPU_PC>;
> >                 };
> >
> >                 L2_0: l2-cache {
> >                         compatible = "cache";
> >                         cache-level = <2>;
> >                 };
> > +
> > +               idle-states {
> 
> entry-method="psci" property goes here. I have a patch fixing it for 410c ;-)
> 
> I don't think the psci_cpuidle_ops will even get called without this.

Hello Amit,

I added debug prints in psci_cpu_suspend_enter() and arm_cpuidle_suspend()
when verifying this patch, and psci_cpu_suspend_enter() is indeed called,
with the correct psci suspend parameter.

The output from:
grep "" /sys/bus/cpu/devices/cpu0/cpuidle/state?/*
also looks sane.

However, if 'entry-method="psci"' is required according to the DT binding,
perhaps you can send a 2/2 series that fixes both this patch and msm8916 ?

> Did you see any changes in consumption with this patch? I was trying
> to measure that before sending this out.

I don't know of any way to measure the power consumption on this board,
so no, I haven't been able to verify that the firmware actually does
the right thing here.


Kind regards,
Niklas

> 
> > +                       CPU_PC: pc {
> > +                               compatible = "arm,idle-state";
> > +                               arm,psci-suspend-param = <0x40000003>;
> > +                               entry-latency-us = <125>;
> > +                               exit-latency-us = <180>;
> > +                               min-residency-us = <595>;
> > +                               local-timer-stop;
> > +                       };
> > +               };
> >         };
> >
> >         firmware {
> > --
> > 2.21.0
> >

Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support

[Amit Kucheria]

(Adding Lorenzo and Sudeep)

On Wed, May 8, 2019 at 8:26 PM Niklas Cassel <niklas.cassel@linaro.org> wrote:
>
> On Wed, May 08, 2019 at 02:48:19AM +0530, Amit Kucheria wrote:
> > On Tue, May 7, 2019 at 1:01 AM Niklas Cassel <niklas.cassel@linaro.org> wrote:
> > >
> > > Add device bindings for CPUs to suspend using PSCI as the enable-method.
> > >
> > > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
> > > ---
> > >  arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > > index ffedf9640af7..f9db9f3ee10c 100644
> > > --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > > @@ -31,6 +31,7 @@
> > >                         reg = <0x100>;
> > >                         enable-method = "psci";
> > >                         next-level-cache = <&L2_0>;
> > > +                       cpu-idle-states = <&CPU_PC>;
> > >                 };
> > >
> > >                 CPU1: cpu@101 {
> > > @@ -39,6 +40,7 @@
> > >                         reg = <0x101>;
> > >                         enable-method = "psci";
> > >                         next-level-cache = <&L2_0>;
> > > +                       cpu-idle-states = <&CPU_PC>;
> > >                 };
> > >
> > >                 CPU2: cpu@102 {
> > > @@ -47,6 +49,7 @@
> > >                         reg = <0x102>;
> > >                         enable-method = "psci";
> > >                         next-level-cache = <&L2_0>;
> > > +                       cpu-idle-states = <&CPU_PC>;
> > >                 };
> > >
> > >                 CPU3: cpu@103 {
> > > @@ -55,12 +58,24 @@
> > >                         reg = <0x103>;
> > >                         enable-method = "psci";
> > >                         next-level-cache = <&L2_0>;
> > > +                       cpu-idle-states = <&CPU_PC>;
> > >                 };
> > >
> > >                 L2_0: l2-cache {
> > >                         compatible = "cache";
> > >                         cache-level = <2>;
> > >                 };
> > > +
> > > +               idle-states {
> >
> > entry-method="psci" property goes here. I have a patch fixing it for 410c ;-)
> >
> > I don't think the psci_cpuidle_ops will even get called without this.
>
> Hello Amit,
>
> I added debug prints in psci_cpu_suspend_enter() and arm_cpuidle_suspend()
> when verifying this patch, and psci_cpu_suspend_enter() is indeed called,
> with the correct psci suspend parameter.
>
> The output from:
> grep "" /sys/bus/cpu/devices/cpu0/cpuidle/state?/*
> also looks sane.
>
> However, if 'entry-method="psci"' is required according to the DT binding,
> perhaps you can send a 2/2 series that fixes both this patch and msm8916 ?

Last time I discussed this with Lorenzo and Sudeep (on IRC), I pointed
out that entry-method="psci" isn't checked for in code anywhere. Let's
get their view on this for posterity.

What does entry-method="psci" in the idle-states node achieve that
enable-method="psci" in the cpu node doesn't achieve? (Note: enable-
vs. entry-).

The enable-method property is the one that sets up the
psci_cpuidle_ops callbacks through the CPUIDLE_METHOD_OF_DECLARE
macro.

IOW, if we deprecated the entry-method property, everything would
still work, wouldn't it?
Do we expect to support PSCI platforms that might have a different
entry-method for idle states?
Should I whip up a patch removing entry-method? Since we don't check
for it today, it won't break the old DTs either.

Regards,
Amit


> > Did you see any changes in consumption with this patch? I was trying
> > to measure that before sending this out.
>
> I don't know of any way to measure the power consumption on this board,
> so no, I haven't been able to verify that the firmware actually does
> the right thing here.
>
>
> Kind regards,
> Niklas
>
> >
> > > +                       CPU_PC: pc {
> > > +                               compatible = "arm,idle-state";
> > > +                               arm,psci-suspend-param = <0x40000003>;
> > > +                               entry-latency-us = <125>;
> > > +                               exit-latency-us = <180>;
> > > +                               min-residency-us = <595>;
> > > +                               local-timer-stop;
> > > +                       };
> > > +               };
> > >         };
> > >
> > >         firmware {
> > > --
> > > 2.21.0
> > >

Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support

[Sudeep Holla]

On Thu, May 09, 2019 at 11:19:23PM +0530, Amit Kucheria wrote:
> (Adding Lorenzo and Sudeep)
>
> On Wed, May 8, 2019 at 8:26 PM Niklas Cassel <niklas.cassel@linaro.org> wrote:
> >
> > On Wed, May 08, 2019 at 02:48:19AM +0530, Amit Kucheria wrote:
> > > On Tue, May 7, 2019 at 1:01 AM Niklas Cassel <niklas.cassel@linaro.org> wrote:
> > > >
> > > > Add device bindings for CPUs to suspend using PSCI as the enable-method.
> > > >
> > > > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
> > > > ---
> > > >  arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++
> > > >  1 file changed, 15 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > > > index ffedf9640af7..f9db9f3ee10c 100644
> > > > --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > > > @@ -31,6 +31,7 @@
> > > >                         reg = <0x100>;
> > > >                         enable-method = "psci";
> > > >                         next-level-cache = <&L2_0>;
> > > > +                       cpu-idle-states = <&CPU_PC>;
> > > >                 };
> > > >
> > > >                 CPU1: cpu@101 {
> > > > @@ -39,6 +40,7 @@
> > > >                         reg = <0x101>;
> > > >                         enable-method = "psci";
> > > >                         next-level-cache = <&L2_0>;
> > > > +                       cpu-idle-states = <&CPU_PC>;
> > > >                 };
> > > >
> > > >                 CPU2: cpu@102 {
> > > > @@ -47,6 +49,7 @@
> > > >                         reg = <0x102>;
> > > >                         enable-method = "psci";
> > > >                         next-level-cache = <&L2_0>;
> > > > +                       cpu-idle-states = <&CPU_PC>;
> > > >                 };
> > > >
> > > >                 CPU3: cpu@103 {
> > > > @@ -55,12 +58,24 @@
> > > >                         reg = <0x103>;
> > > >                         enable-method = "psci";
> > > >                         next-level-cache = <&L2_0>;
> > > > +                       cpu-idle-states = <&CPU_PC>;
> > > >                 };
> > > >
> > > >                 L2_0: l2-cache {
> > > >                         compatible = "cache";
> > > >                         cache-level = <2>;
> > > >                 };
> > > > +
> > > > +               idle-states {
> > >
> > > entry-method="psci" property goes here. I have a patch fixing it for 410c ;-)
> > >
> > > I don't think the psci_cpuidle_ops will even get called without this.
> >
> > Hello Amit,
> >
> > I added debug prints in psci_cpu_suspend_enter() and arm_cpuidle_suspend()
> > when verifying this patch, and psci_cpu_suspend_enter() is indeed called,
> > with the correct psci suspend parameter.
> >
> > The output from:
> > grep "" /sys/bus/cpu/devices/cpu0/cpuidle/state?/*
> > also looks sane.
> >
> > However, if 'entry-method="psci"' is required according to the DT binding,
> > perhaps you can send a 2/2 series that fixes both this patch and msm8916 ?
>
> Last time I discussed this with Lorenzo and Sudeep (on IRC), I pointed
> out that entry-method="psci" isn't checked for in code anywhere. Let's
> get their view on this for posterity.
>

Yes entry-method="psci" is required as per DT binding but not checked
in code on arm64. We have CPU ops with idle enabled only for "psci", so
there's not need to check.

Once we have DT schema validation, this will be caught, so it's better
to fix it.

> What does entry-method="psci" in the idle-states node achieve that
> enable-method="psci" in the cpu node doesn't achieve? (Note: enable-
> vs. entry-).
>

From DT binding perspective, we can have different CPU enable-method
and CPU idle entry-method. However on arm64, it's restricted to PSCI
only. I need to check what happens on arm32 though, as the driver
invocation happens via CPUIDLE_METHOD_OF_DECLARE.

> The enable-method property is the one that sets up the
> psci_cpuidle_ops callbacks through the CPUIDLE_METHOD_OF_DECLARE
> macro.
>

Indeed.

> IOW, if we deprecated the entry-method property, everything would
> still work, wouldn't it?

Why do you want to deprecated just because Linux kernel doesn't want to
use it. That's not a valid reason IMO.

> Do we expect to support PSCI platforms that might have a different
> entry-method for idle states?

Not on ARM64, but same DT bindings can be used for idle-states on
say RISC-V and have some value other than "psci".

> Should I whip up a patch removing entry-method? Since we don't check
> for it today, it won't break the old DTs either.
>

Nope, I don't think so. But if it's causing issues, we can look into it.
I don't want to restrict the use of the bindings for ARM/ARM64 or psci only.

--
Regards,
Sudeep

Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support

[Amit Kucheria]

On Tue, May 7, 2019 at 12:25 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Mon 06 May 22:35 PDT 2019, Vinod Koul wrote:
>
> > On 06-05-19, 21:31, Niklas Cassel wrote:
> > > Add device bindings for CPUs to suspend using PSCI as the enable-method.
> > >
> > > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
> > > ---
> > >  arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > > index ffedf9640af7..f9db9f3ee10c 100644
> > > --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > > @@ -31,6 +31,7 @@
> > >                     reg = <0x100>;
> > >                     enable-method = "psci";
> > >                     next-level-cache = <&L2_0>;
> > > +                   cpu-idle-states = <&CPU_PC>;
> > >             };
> > >
> > >             CPU1: cpu@101 {
> > > @@ -39,6 +40,7 @@
> > >                     reg = <0x101>;
> > >                     enable-method = "psci";
> > >                     next-level-cache = <&L2_0>;
> > > +                   cpu-idle-states = <&CPU_PC>;
> > >             };
> > >
> > >             CPU2: cpu@102 {
> > > @@ -47,6 +49,7 @@
> > >                     reg = <0x102>;
> > >                     enable-method = "psci";
> > >                     next-level-cache = <&L2_0>;
> > > +                   cpu-idle-states = <&CPU_PC>;
> > >             };
> > >
> > >             CPU3: cpu@103 {
> > > @@ -55,12 +58,24 @@
> > >                     reg = <0x103>;
> > >                     enable-method = "psci";
> > >                     next-level-cache = <&L2_0>;
> > > +                   cpu-idle-states = <&CPU_PC>;
> > >             };
> > >
> > >             L2_0: l2-cache {
> > >                     compatible = "cache";
> > >                     cache-level = <2>;
> > >             };
> > > +
> > > +           idle-states {
> >
> > Since we are trying to sort the file per address and
> > alphabetically, it would be great if this can be moved before l2-cache
> > :)
> >
>
> Picked up, with the order adjusted.
>
> > Other than that this lgtm
> >
>
> I presume that lgtm == Reviewed-by...
>
> Thanks,
> Bjorn

Hi Bjorn,

Please drop this patch and check the one I've sent as part of the qcom
cpuidle series?

Regards,
Amit

> > > +                   CPU_PC: pc {
> > > +                           compatible = "arm,idle-state";
> > > +                           arm,psci-suspend-param = <0x40000003>;
> > > +                           entry-latency-us = <125>;
> > > +                           exit-latency-us = <180>;
> > > +                           min-residency-us = <595>;
> > > +                           local-timer-stop;
> > > +                   };
> > > +           };
> > >     };
> > >
> > >     firmware {
> > > --
> > > 2.21.0
> >
> > --
> > ~Vinod

Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support

[Amit Kucheria]

On Fri, May 10, 2019 at 2:54 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, May 09, 2019 at 11:19:23PM +0530, Amit Kucheria wrote:
> > (Adding Lorenzo and Sudeep)
> >
> > On Wed, May 8, 2019 at 8:26 PM Niklas Cassel <niklas.cassel@linaro.org> wrote:
> > >
> > > On Wed, May 08, 2019 at 02:48:19AM +0530, Amit Kucheria wrote:
> > > > On Tue, May 7, 2019 at 1:01 AM Niklas Cassel <niklas.cassel@linaro.org> wrote:
> > > > >
> > > > > Add device bindings for CPUs to suspend using PSCI as the enable-method.
> > > > >
> > > > > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
> > > > > ---
> > > > >  arch/arm64/boot/dts/qcom/qcs404.dtsi | 15 +++++++++++++++
> > > > >  1 file changed, 15 insertions(+)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > > > > index ffedf9640af7..f9db9f3ee10c 100644
> > > > > --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > > > > +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> > > > > @@ -31,6 +31,7 @@
> > > > >                         reg = <0x100>;
> > > > >                         enable-method = "psci";
> > > > >                         next-level-cache = <&L2_0>;
> > > > > +                       cpu-idle-states = <&CPU_PC>;
> > > > >                 };
> > > > >
> > > > >                 CPU1: cpu@101 {
> > > > > @@ -39,6 +40,7 @@
> > > > >                         reg = <0x101>;
> > > > >                         enable-method = "psci";
> > > > >                         next-level-cache = <&L2_0>;
> > > > > +                       cpu-idle-states = <&CPU_PC>;
> > > > >                 };
> > > > >
> > > > >                 CPU2: cpu@102 {
> > > > > @@ -47,6 +49,7 @@
> > > > >                         reg = <0x102>;
> > > > >                         enable-method = "psci";
> > > > >                         next-level-cache = <&L2_0>;
> > > > > +                       cpu-idle-states = <&CPU_PC>;
> > > > >                 };
> > > > >
> > > > >                 CPU3: cpu@103 {
> > > > > @@ -55,12 +58,24 @@
> > > > >                         reg = <0x103>;
> > > > >                         enable-method = "psci";
> > > > >                         next-level-cache = <&L2_0>;
> > > > > +                       cpu-idle-states = <&CPU_PC>;
> > > > >                 };
> > > > >
> > > > >                 L2_0: l2-cache {
> > > > >                         compatible = "cache";
> > > > >                         cache-level = <2>;
> > > > >                 };
> > > > > +
> > > > > +               idle-states {
> > > >
> > > > entry-method="psci" property goes here. I have a patch fixing it for 410c ;-)
> > > >
> > > > I don't think the psci_cpuidle_ops will even get called without this.
> > >
> > > Hello Amit,
> > >
> > > I added debug prints in psci_cpu_suspend_enter() and arm_cpuidle_suspend()
> > > when verifying this patch, and psci_cpu_suspend_enter() is indeed called,
> > > with the correct psci suspend parameter.
> > >
> > > The output from:
> > > grep "" /sys/bus/cpu/devices/cpu0/cpuidle/state?/*
> > > also looks sane.
> > >
> > > However, if 'entry-method="psci"' is required according to the DT binding,
> > > perhaps you can send a 2/2 series that fixes both this patch and msm8916 ?
> >
> > Last time I discussed this with Lorenzo and Sudeep (on IRC), I pointed
> > out that entry-method="psci" isn't checked for in code anywhere. Let's
> > get their view on this for posterity.
> >
>
> Yes entry-method="psci" is required as per DT binding but not checked
> in code on arm64. We have CPU ops with idle enabled only for "psci", so
> there's not need to check.

I don't see it being checked on arm32 either.

> Once we have DT schema validation, this will be caught, so it's better
> to fix it.
>
> > What does entry-method="psci" in the idle-states node achieve that
> > enable-method="psci" in the cpu node doesn't achieve? (Note: enable-
> > vs. entry-).
> >
>
> From DT binding perspective, we can have different CPU enable-method
> and CPU idle entry-method. However on arm64, it's restricted to PSCI
> only. I need to check what happens on arm32 though, as the driver
> invocation happens via CPUIDLE_METHOD_OF_DECLARE.
>
> > The enable-method property is the one that sets up the
> > psci_cpuidle_ops callbacks through the CPUIDLE_METHOD_OF_DECLARE
> > macro.
> >
>
> Indeed.
>
> > IOW, if we deprecated the entry-method property, everything would
> > still work, wouldn't it?
>
> Why do you want to deprecated just because Linux kernel doesn't want to
> use it. That's not a valid reason IMO.

Fair enough. Just want to make sure that it isn't some vestigial
property that was never used. Do you know if another OS is actually
using it?

> > Do we expect to support PSCI platforms that might have a different
> > entry-method for idle states?
>
> Not on ARM64, but same DT bindings can be used for idle-states on
> say RISC-V and have some value other than "psci".

Both enable-method and entry-method properties are currently only used
(and documented) for ARM platforms. Hence this discussion about
deprecation of one of them.

> > Should I whip up a patch removing entry-method? Since we don't check
> > for it today, it won't break the old DTs either.
> >
>
> Nope, I don't think so. But if it's causing issues, we can look into it.
> I don't want to restrict the use of the bindings for ARM/ARM64 or psci only.

Only a couple of minor issues:
1. There is a trickle of DTs that need fixing up every now and then
because they don't use entry-method in their idle-states node. Schema
validation ought to fix that.
2. A property that isn't ready by any code is a bit confusing. Perhaps
we can mention something to the effect in the documentation?

Regards,
Amit

Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support

[Sudeep Holla]

On Fri, May 10, 2019 at 11:58:40PM +0530, Amit Kucheria wrote:
> On Fri, May 10, 2019 at 2:54 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >

[...]

> >
> > Yes entry-method="psci" is required as per DT binding but not checked
> > in code on arm64. We have CPU ops with idle enabled only for "psci", so
> > there's not need to check.
>
> I don't see it being checked on arm32 either.
>

arm_cpuidle_get_ops in arch/arm/kernel/cpuidle.c checks the method, has
to match "psci" for drivers/firmware/psci.c to work on arm32

[...]

> >
> > Why do you want to deprecated just because Linux kernel doesn't want to
> > use it. That's not a valid reason IMO.
>
> Fair enough. Just want to make sure that it isn't some vestigial
> property that was never used. Do you know if another OS is actually
> using it?
>

Not that I am aware of. But Linux uses it on arm32, so it's not entirely
unused.

> > > Do we expect to support PSCI platforms that might have a different
> > > entry-method for idle states?
> >
> > Not on ARM64, but same DT bindings can be used for idle-states on
> > say RISC-V and have some value other than "psci".
>
> Both enable-method and entry-method properties are currently only used
> (and documented) for ARM platforms. Hence this discussion about
> deprecation of one of them.
>

Yes, it's used on arm32 as mentioned above.

> > > Should I whip up a patch removing entry-method? Since we don't check
> > > for it today, it won't break the old DTs either.
> > >
> >
> > Nope, I don't think so. But if it's causing issues, we can look into it.
> > I don't want to restrict the use of the bindings for ARM/ARM64 or psci only.
>
> Only a couple of minor issues:
> 1. There is a trickle of DTs that need fixing up every now and then
> because they don't use entry-method in their idle-states node. Schema
> validation ought to fix that.

I understand, scheme should fix it. This is not just restricted to this,
it's generic DT problem. So let's hope we get schema based validation soon.

> 2. A property that isn't ready by any code is a bit confusing. Perhaps
> we can mention something to the effect in the documentation?
>

Not entirely true. We have quite a lot of bindings that are added just
because downstream drivers use e.g. GPU and even standard ePAPR or DT
specification has lots of bindings which OS like Linux may choose
not to use at all. Same applies to ACPI, so I am not for removing bindings
just because there are no users in Linux.

--
Regards,
Sudeep

Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support

[Amit Kucheria]

On Mon, May 13, 2019 at 3:19 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, May 10, 2019 at 11:58:40PM +0530, Amit Kucheria wrote:
> > On Fri, May 10, 2019 at 2:54 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
>
> [...]
>
> > >
> > > Yes entry-method="psci" is required as per DT binding but not checked
> > > in code on arm64. We have CPU ops with idle enabled only for "psci", so
> > > there's not need to check.
> >
> > I don't see it being checked on arm32 either.
> >
>
> arm_cpuidle_get_ops in arch/arm/kernel/cpuidle.c checks the method, has
> to match "psci" for drivers/firmware/psci.c to work on arm32

That is a check for the enable-method, not entry-method.

We don't check for entry-method anywhere, AFAICT.

> [...]
>
> > >
> > > Why do you want to deprecated just because Linux kernel doesn't want to
> > > use it. That's not a valid reason IMO.
> >
> > Fair enough. Just want to make sure that it isn't some vestigial
> > property that was never used. Do you know if another OS is actually
> > using it?
> >
>
> Not that I am aware of. But Linux uses it on arm32, so it's not entirely
> unused.

entry-method is not read in Linux code (see above).

> > > > Do we expect to support PSCI platforms that might have a different
> > > > entry-method for idle states?
> > >
> > > Not on ARM64, but same DT bindings can be used for idle-states on
> > > say RISC-V and have some value other than "psci".
> >
> > Both enable-method and entry-method properties are currently only used
> > (and documented) for ARM platforms. Hence this discussion about
> > deprecation of one of them.
> >
>
> Yes, it's used on arm32 as mentioned above.

Only enable-method is checked.

> > > > Should I whip up a patch removing entry-method? Since we don't check
> > > > for it today, it won't break the old DTs either.
> > > >
> > >
> > > Nope, I don't think so. But if it's causing issues, we can look into it.
> > > I don't want to restrict the use of the bindings for ARM/ARM64 or psci only.
> >
> > Only a couple of minor issues:
> > 1. There is a trickle of DTs that need fixing up every now and then
> > because they don't use entry-method in their idle-states node. Schema
> > validation ought to fix that.
>
> I understand, scheme should fix it. This is not just restricted to this,
> it's generic DT problem. So let's hope we get schema based validation soon.
>
> > 2. A property that isn't ready by any code is a bit confusing. Perhaps
> > we can mention something to the effect in the documentation?
> >
>
> Not entirely true. We have quite a lot of bindings that are added just
> because downstream drivers use e.g. GPU and even standard ePAPR or DT
> specification has lots of bindings which OS like Linux may choose
> not to use at all. Same applies to ACPI, so I am not for removing bindings
> just because there are no users in Linux.

That is a fair point. But in those cases, the binding is probably used
by another OS. entry-method seems to an example of one that isn't used
by Linux or other OSes.

Regards,
Amit

Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support

[Sudeep Holla]

On Wed, May 15, 2019 at 03:26:14PM +0530, Amit Kucheria wrote:
> On Mon, May 13, 2019 at 3:19 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Fri, May 10, 2019 at 11:58:40PM +0530, Amit Kucheria wrote:
> > > On Fri, May 10, 2019 at 2:54 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> >
> > [...]
> >
> > > >
> > > > Yes entry-method="psci" is required as per DT binding but not checked
> > > > in code on arm64. We have CPU ops with idle enabled only for "psci", so
> > > > there's not need to check.
> > >
> > > I don't see it being checked on arm32 either.
> > >
> >
> > arm_cpuidle_get_ops in arch/arm/kernel/cpuidle.c checks the method, has
> > to match "psci" for drivers/firmware/psci.c to work on arm32
>
> That is a check for the enable-method, not entry-method.
>
> We don't check for entry-method anywhere, AFAICT.
>

Ah, you right. My eyes can't distinguish between entry-method and enable-method
anymore :(

--
Regards,
Sudeep