LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support
@ 2019-05-06 19:31 Niklas Cassel
  2019-05-07  5:35 ` Vinod Koul
  2019-05-07 21:18 ` Amit Kucheria
  0 siblings, 2 replies; 13+ messages in thread
From: Niklas Cassel @ 2019-05-06 19:31 UTC (permalink / raw)
  To: Andy Gross, David Brown, Rob Herring, Mark Rutland
  Cc: amit.kucheria, jorge.ramirez-ortiz, lina.iyer, ulf.hansson,
	bjorn.andersson, Niklas Cassel, linux-arm-msm, devicetree,
	linux-kernel

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support
  2019-05-06 19:31 [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support Niklas Cassel
@ 2019-05-07  5:35 ` Vinod Koul
  2019-05-07  6:55   ` Bjorn Andersson
  2019-05-07 21:18 ` Amit Kucheria
  1 sibling, 1 reply; 13+ messages in thread
From: Vinod Koul @ 2019-05-07  5:35 UTC (permalink / raw)
  To: Niklas Cassel
  Cc: Andy Gross, David Brown, Rob Herring, Mark Rutland,
	amit.kucheria, jorge.ramirez-ortiz, lina.iyer, ulf.hansson,
	bjorn.andersson, linux-arm-msm, devicetree, linux-kernel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support
  2019-05-07  5:35 ` Vinod Koul
@ 2019-05-07  6:55   ` Bjorn Andersson
  2019-05-07  7:06     ` Vinod Koul
  2019-05-10 11:31     ` Amit Kucheria
  0 siblings, 2 replies; 13+ messages in thread
From: Bjorn Andersson @ 2019-05-07  6:55 UTC (permalink / raw)
  To: Vinod Koul
  Cc: Niklas Cassel, Andy Gross, David Brown, Rob Herring,
	Mark Rutland, amit.kucheria, jorge.ramirez-ortiz, lina.iyer,
	ulf.hansson, linux-arm-msm, devicetree, linux-kernel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support
  2019-05-07  6:55   ` Bjorn Andersson
@ 2019-05-07  7:06     ` Vinod Koul
  2019-05-10 11:31     ` Amit Kucheria
  1 sibling, 0 replies; 13+ messages in thread
From: Vinod Koul @ 2019-05-07  7:06 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Niklas Cassel, Andy Gross, David Brown, Rob Herring,
	Mark Rutland, amit.kucheria, jorge.ramirez-ortiz, lina.iyer,
	ulf.hansson, linux-arm-msm, devicetree, linux-kernel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support
  2019-05-06 19:31 [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support Niklas Cassel
  2019-05-07  5:35 ` Vinod Koul
@ 2019-05-07 21:18 ` Amit Kucheria
  2019-05-08 14:56   ` Niklas Cassel
  1 sibling, 1 reply; 13+ messages in thread
From: Amit Kucheria @ 2019-05-07 21:18 UTC (permalink / raw)
  To: Niklas Cassel
  Cc: Andy Gross, David Brown, Rob Herring, Mark Rutland,
	Jorge Ramirez-Ortiz, Lina Iyer, Ulf Hansson, Bjorn Andersson,
	linux-arm-msm, DTML, Linux Kernel Mailing List

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
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support
  2019-05-07 21:18 ` Amit Kucheria
@ 2019-05-08 14:56   ` Niklas Cassel
  2019-05-09 17:49     ` Amit Kucheria
  0 siblings, 1 reply; 13+ messages in thread
From: Niklas Cassel @ 2019-05-08 14:56 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: Andy Gross, David Brown, Rob Herring, Mark Rutland,
	Jorge Ramirez-Ortiz, Lina Iyer, Ulf Hansson, Bjorn Andersson,
	linux-arm-msm, DTML, Linux Kernel Mailing List

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
> >

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support
  2019-05-08 14:56   ` Niklas Cassel
@ 2019-05-09 17:49     ` Amit Kucheria
  2019-05-10  9:24       ` Sudeep Holla
  0 siblings, 1 reply; 13+ messages in thread
From: Amit Kucheria @ 2019-05-09 17:49 UTC (permalink / raw)
  To: Niklas Cassel, Lorenzo Pieralisi, Sudeep Holla
  Cc: Andy Gross, David Brown, Rob Herring, Mark Rutland,
	Jorge Ramirez-Ortiz, Lina Iyer, Ulf Hansson, Bjorn Andersson,
	linux-arm-msm, DTML, Linux Kernel Mailing List

(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
> > >

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support
  2019-05-09 17:49     ` Amit Kucheria
@ 2019-05-10  9:24       ` Sudeep Holla
  2019-05-10 18:28         ` Amit Kucheria
  0 siblings, 1 reply; 13+ messages in thread
From: Sudeep Holla @ 2019-05-10  9:24 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: Niklas Cassel, Lorenzo Pieralisi, Andy Gross, David Brown,
	Rob Herring, Mark Rutland, Jorge Ramirez-Ortiz, Lina Iyer,
	Ulf Hansson, Bjorn Andersson, linux-arm-msm, DTML, Sudeep Holla,
	Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support
  2019-05-07  6:55   ` Bjorn Andersson
  2019-05-07  7:06     ` Vinod Koul
@ 2019-05-10 11:31     ` Amit Kucheria
  1 sibling, 0 replies; 13+ messages in thread
From: Amit Kucheria @ 2019-05-10 11:31 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Vinod Koul, Niklas Cassel, Andy Gross, David Brown, Rob Herring,
	Mark Rutland, Jorge Ramirez-Ortiz, Lina Iyer, Ulf Hansson,
	linux-arm-msm,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support
  2019-05-10  9:24       ` Sudeep Holla
@ 2019-05-10 18:28         ` Amit Kucheria
  2019-05-13  9:49           ` Sudeep Holla
  0 siblings, 1 reply; 13+ messages in thread
From: Amit Kucheria @ 2019-05-10 18:28 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: Niklas Cassel, Lorenzo Pieralisi, Andy Gross, David Brown,
	Rob Herring, Mark Rutland, Jorge Ramirez-Ortiz, Lina Iyer,
	Ulf Hansson, Bjorn Andersson, linux-arm-msm, DTML,
	Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support
  2019-05-10 18:28         ` Amit Kucheria
@ 2019-05-13  9:49           ` Sudeep Holla
  2019-05-15  9:56             ` Amit Kucheria
  0 siblings, 1 reply; 13+ messages in thread
From: Sudeep Holla @ 2019-05-13  9:49 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: Niklas Cassel, Lorenzo Pieralisi, Andy Gross, David Brown,
	Rob Herring, Mark Rutland, Jorge Ramirez-Ortiz, Lina Iyer,
	Ulf Hansson, Bjorn Andersson, linux-arm-msm, DTML, Sudeep Holla,
	Linux Kernel Mailing List

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support
  2019-05-13  9:49           ` Sudeep Holla
@ 2019-05-15  9:56             ` Amit Kucheria
  2019-05-15 10:30               ` Sudeep Holla
  0 siblings, 1 reply; 13+ messages in thread
From: Amit Kucheria @ 2019-05-15  9:56 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: Niklas Cassel, Lorenzo Pieralisi, Andy Gross, David Brown,
	Rob Herring, Mark Rutland, Jorge Ramirez-Ortiz, Lina Iyer,
	Ulf Hansson, Bjorn Andersson, linux-arm-msm, DTML,
	Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support
  2019-05-15  9:56             ` Amit Kucheria
@ 2019-05-15 10:30               ` Sudeep Holla
  0 siblings, 0 replies; 13+ messages in thread
From: Sudeep Holla @ 2019-05-15 10:30 UTC (permalink / raw)
  To: Amit Kucheria
  Cc: Niklas Cassel, Lorenzo Pieralisi, Andy Gross, David Brown,
	Rob Herring, Mark Rutland, Jorge Ramirez-Ortiz, Lina Iyer,
	Ulf Hansson, Bjorn Andersson, linux-arm-msm, DTML,
	Linux Kernel Mailing List, 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-05-15 10:30 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-06 19:31 [PATCH] arm64: dts: qcom: qcs404: Add PSCI cpuidle support Niklas Cassel
2019-05-07  5:35 ` Vinod Koul
2019-05-07  6:55   ` Bjorn Andersson
2019-05-07  7:06     ` Vinod Koul
2019-05-10 11:31     ` Amit Kucheria
2019-05-07 21:18 ` Amit Kucheria
2019-05-08 14:56   ` Niklas Cassel
2019-05-09 17:49     ` Amit Kucheria
2019-05-10  9:24       ` Sudeep Holla
2019-05-10 18:28         ` Amit Kucheria
2019-05-13  9:49           ` Sudeep Holla
2019-05-15  9:56             ` Amit Kucheria
2019-05-15 10:30               ` Sudeep Holla

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).