mbox series

[v2,0/9] qcom: Add cpuidle to some platforms

Message ID cover.1558430617.git.amit.kucheria@linaro.org
Headers show
Series qcom: Add cpuidle to some platforms | expand

Message

Amit Kucheria May 21, 2019, 9:35 a.m. UTC
Changes since v1:
 - Reworded changes to the idle-state documentation on Sudeep's feedback.
 - Renamed several idle-state node names to be homogeneous across qcom
   platforms. We now use cpu_sleep_0_0 format for the node name while using
   LITTLE_CPU_SLEEP_0 format for labels to help differentiate the different
   states for different CPU types.
 - Add a new patch to add capacity-dmips-mhz property for msm8996 to allow
   topology code to find its true capacity.
 - Add power-collapse state to msm8998 in additon to the retention state.
 - Added acks

MSM8998 changes are untested for v2 because I couldn't access the mtp I
usually have access to. Hopefully Marc and Sibi can help with testing.

Description
-----------
Fix up a few entry-method="psci" issues and then add cpuidle low power
states for msm8996, msm8998, qcs404, sdm845. All these have been tested
to only make sure that the C-states are entered from Linux point-of-view.

We will continue to add more states and make power measurements to tweak
some of these numbers, but getting these merged will allow other people to
use these platforms to work on cpuidle, eas and related topics.


Amit Kucheria (7):
  arm64: dts: fsl: ls1028a: Fix entry-method property to reflect
    documentation
  Documentation: arm: Link idle-states binding to "enable-method"
    property
  arm64: dts: qcom: msm8916: Add entry-method property for the
    idle-states node
  arm64: dts: qcom: msm8916: Use more generic idle state names
  arm64: dts: qcom: msm8996: Add PSCI cpuidle low power states
  arm64: dts: qcom: msm8998: Add PSCI cpuidle low power states
  arm64: dts: msm8996: Add proper capacity scaling for the cpus

Niklas Cassel (1):
  arm64: dts: qcom: qcs404: Add PSCI cpuidle low power states

Raju P.L.S.S.S.N (1):
  arm64: dts: qcom: sdm845: Add PSCI cpuidle low power states

 .../devicetree/bindings/arm/idle-states.txt   | 13 +++-
 .../arm64/boot/dts/freescale/fsl-ls1028a.dtsi |  2 +-
 arch/arm64/boot/dts/qcom/msm8916.dtsi         | 13 ++--
 arch/arm64/boot/dts/qcom/msm8996.dtsi         | 21 ++++++
 arch/arm64/boot/dts/qcom/msm8998.dtsi         | 50 ++++++++++++++
 arch/arm64/boot/dts/qcom/qcs404.dtsi          | 18 +++++
 arch/arm64/boot/dts/qcom/sdm845.dtsi          | 69 +++++++++++++++++++
 7 files changed, 177 insertions(+), 9 deletions(-)

-- 
2.17.1

Comments

Bjorn Andersson May 22, 2019, 3:31 a.m. UTC | #1
On Tue 21 May 02:35 PDT 2019, Amit Kucheria wrote:

> From: Niklas Cassel <niklas.cassel@linaro.org>

> 

> Add device bindings for cpuidle states for cpu devices.

> 

> Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>

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

> [rename the idle-states to more generic names and fixups]

> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>

> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> ---


Applied

Regards,
Bjorn

>  arch/arm64/boot/dts/qcom/qcs404.dtsi | 18 ++++++++++++++++++

>  1 file changed, 18 insertions(+)

> 

> diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi

> index e8fd26633d57..0a9b29af64c2 100644

> --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi

> +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi

> @@ -30,6 +30,7 @@

>  			compatible = "arm,cortex-a53";

>  			reg = <0x100>;

>  			enable-method = "psci";

> +			cpu-idle-states = <&CPU_SLEEP_0>;

>  			next-level-cache = <&L2_0>;

>  		};

>  

> @@ -38,6 +39,7 @@

>  			compatible = "arm,cortex-a53";

>  			reg = <0x101>;

>  			enable-method = "psci";

> +			cpu-idle-states = <&CPU_SLEEP_0>;

>  			next-level-cache = <&L2_0>;

>  		};

>  

> @@ -46,6 +48,7 @@

>  			compatible = "arm,cortex-a53";

>  			reg = <0x102>;

>  			enable-method = "psci";

> +			cpu-idle-states = <&CPU_SLEEP_0>;

>  			next-level-cache = <&L2_0>;

>  		};

>  

> @@ -54,6 +57,7 @@

>  			compatible = "arm,cortex-a53";

>  			reg = <0x103>;

>  			enable-method = "psci";

> +			cpu-idle-states = <&CPU_SLEEP_0>;

>  			next-level-cache = <&L2_0>;

>  		};

>  

> @@ -61,6 +65,20 @@

>  			compatible = "cache";

>  			cache-level = <2>;

>  		};

> +

> +		idle-states {

> +			entry-method = "psci";

> +

> +			CPU_SLEEP_0: cpu-sleep-0 {

> +				compatible = "arm,idle-state";

> +				idle-state-name = "standalone-power-collapse";

> +				arm,psci-suspend-param = <0x40000003>;

> +				entry-latency-us = <125>;

> +				exit-latency-us = <180>;

> +				min-residency-us = <595>;

> +				local-timer-stop;

> +			};

> +		};

>  	};

>  

>  	firmware {

> -- 

> 2.17.1

>
Bjorn Andersson May 22, 2019, 3:49 a.m. UTC | #2
On Tue 21 May 02:35 PDT 2019, Amit Kucheria wrote:

> Add device bindings for cpuidle states for cpu devices.

> 

> msm8996 features 4 cpus - 2 in each cluster. However, all cpus implement

> the same microarchitecture and the two clusters only differ in the

> maximum frequency attainable by the CPUs.

> 

> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>


Applied

Regards,
Bjorn

> ---

>  arch/arm64/boot/dts/qcom/msm8996.dtsi | 17 +++++++++++++++++

>  1 file changed, 17 insertions(+)

> 

> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi

> index c761269caf80..4f2fb7885f39 100644

> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi

> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi

> @@ -95,6 +95,7 @@

>  			compatible = "qcom,kryo";

>  			reg = <0x0 0x0>;

>  			enable-method = "psci";

> +			cpu-idle-states = <&CPU_SLEEP_0>;

>  			next-level-cache = <&L2_0>;

>  			L2_0: l2-cache {

>  			      compatible = "cache";

> @@ -107,6 +108,7 @@

>  			compatible = "qcom,kryo";

>  			reg = <0x0 0x1>;

>  			enable-method = "psci";

> +			cpu-idle-states = <&CPU_SLEEP_0>;

>  			next-level-cache = <&L2_0>;

>  		};

>  

> @@ -115,6 +117,7 @@

>  			compatible = "qcom,kryo";

>  			reg = <0x0 0x100>;

>  			enable-method = "psci";

> +			cpu-idle-states = <&CPU_SLEEP_0>;

>  			next-level-cache = <&L2_1>;

>  			L2_1: l2-cache {

>  			      compatible = "cache";

> @@ -127,6 +130,7 @@

>  			compatible = "qcom,kryo";

>  			reg = <0x0 0x101>;

>  			enable-method = "psci";

> +			cpu-idle-states = <&CPU_SLEEP_0>;

>  			next-level-cache = <&L2_1>;

>  		};

>  

> @@ -151,6 +155,19 @@

>  				};

>  			};

>  		};

> +

> +		idle-states {

> +			entry-method = "psci";

> +

> +			CPU_SLEEP_0: cpu-sleep-0 {

> +				compatible = "arm,idle-state";

> +				idle-state-name = "standalone-power-collapse";

> +				arm,psci-suspend-param = <0x00000004>;

> +				entry-latency-us = <40>;

> +				exit-latency-us = <80>;

> +				min-residency-us = <300>;

> +			};

> +		};

>  	};

>  

>  	thermal-zones {

> -- 

> 2.17.1

>
Bjorn Andersson May 22, 2019, 3:50 a.m. UTC | #3
On Tue 21 May 02:35 PDT 2019, Amit Kucheria wrote:

> msm8996 features 4 cpus - 2 in each cluster. However, all cpus implement

> the same microarchitecture and the two clusters only differ in the

> maximum frequency attainable by the CPUs.

> 

> Add capacity-dmips-mhz property to allow the topology code to determine

> the actual capacity by taking into account the highest frequency for

> each CPU.

> 

> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>

> Suggested-by: Daniel Lezcano <daniel.lezcano@linaro.org>


Applied

Regards,
Bjorn

> ---

>  arch/arm64/boot/dts/qcom/msm8996.dtsi | 4 ++++

>  1 file changed, 4 insertions(+)

> 

> diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi

> index 4f2fb7885f39..e0e8f30ce11a 100644

> --- a/arch/arm64/boot/dts/qcom/msm8996.dtsi

> +++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi

> @@ -96,6 +96,7 @@

>  			reg = <0x0 0x0>;

>  			enable-method = "psci";

>  			cpu-idle-states = <&CPU_SLEEP_0>;

> +			capacity-dmips-mhz = <1024>;

>  			next-level-cache = <&L2_0>;

>  			L2_0: l2-cache {

>  			      compatible = "cache";

> @@ -109,6 +110,7 @@

>  			reg = <0x0 0x1>;

>  			enable-method = "psci";

>  			cpu-idle-states = <&CPU_SLEEP_0>;

> +			capacity-dmips-mhz = <1024>;

>  			next-level-cache = <&L2_0>;

>  		};

>  

> @@ -118,6 +120,7 @@

>  			reg = <0x0 0x100>;

>  			enable-method = "psci";

>  			cpu-idle-states = <&CPU_SLEEP_0>;

> +			capacity-dmips-mhz = <1024>;

>  			next-level-cache = <&L2_1>;

>  			L2_1: l2-cache {

>  			      compatible = "cache";

> @@ -131,6 +134,7 @@

>  			reg = <0x0 0x101>;

>  			enable-method = "psci";

>  			cpu-idle-states = <&CPU_SLEEP_0>;

> +			capacity-dmips-mhz = <1024>;

>  			next-level-cache = <&L2_1>;

>  		};

>  

> -- 

> 2.17.1

>
Rob Herring June 13, 2019, 11:13 p.m. UTC | #4
On Tue, 21 May 2019 15:05:12 +0530, Amit Kucheria wrote:
> The "enable-method" property for cpu nodes needs to be "psci" for CPU

> idle management to be setup correctly.

> 

> Add a note to the binding documentation to this effect to make it

> obvious.

> 

> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>

> Acked-by: Sudeep Holla <sudeep.holla@arm.com>

> ---

>  .../devicetree/bindings/arm/idle-states.txt         | 13 ++++++++++---

>  1 file changed, 10 insertions(+), 3 deletions(-)

> 


Applied, thanks.

Rob
Amit Kucheria Sept. 30, 2019, 10:44 p.m. UTC | #5
Can you try removing just the *SLEEP_1 states from the cpu-idle-states
property? I want to understand if this is triggered just by the deeper
C-state.

On Tue, Oct 1, 2019 at 3:50 AM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote:
>

> Amit, the merged version of the below change causes a boot failure

> (nasty hang, sometimes with RCU stalls) on the msm8998 laptops.  Oddly

> enough, it seems to be resolved if I remove the cpu-idle-states

> property from one of the cpu nodes.

>

> I see no issues with the msm8998 MTP.

>

> Do you have any suggestions on how we might debug this?

>

> On Tue, May 21, 2019 at 3:38 AM Amit Kucheria <amit.kucheria@linaro.org> wrote:

> >

> > Add device bindings for cpuidle states for cpu devices.

> >

> > Cc: Marc Gonzalez <marc.w.gonzalez@free.fr>

> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>

> > Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

> > ---

> >  arch/arm64/boot/dts/qcom/msm8998.dtsi | 50 +++++++++++++++++++++++++++

> >  1 file changed, 50 insertions(+)

> >

> > diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi

> > index 3fd0769fe648..54810980fcf9 100644

> > --- a/arch/arm64/boot/dts/qcom/msm8998.dtsi

> > +++ b/arch/arm64/boot/dts/qcom/msm8998.dtsi

> > @@ -78,6 +78,7 @@

> >                         compatible = "arm,armv8";

> >                         reg = <0x0 0x0>;

> >                         enable-method = "psci";

> > +                       cpu-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;

> >                         efficiency = <1024>;

> >                         next-level-cache = <&L2_0>;

> >                         L2_0: l2-cache {

> > @@ -97,6 +98,7 @@

> >                         compatible = "arm,armv8";

> >                         reg = <0x0 0x1>;

> >                         enable-method = "psci";

> > +                       cpu-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;

> >                         efficiency = <1024>;

> >                         next-level-cache = <&L2_0>;

> >                         L1_I_1: l1-icache {

> > @@ -112,6 +114,7 @@

> >                         compatible = "arm,armv8";

> >                         reg = <0x0 0x2>;

> >                         enable-method = "psci";

> > +                       cpu-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;

> >                         efficiency = <1024>;

> >                         next-level-cache = <&L2_0>;

> >                         L1_I_2: l1-icache {

> > @@ -127,6 +130,7 @@

> >                         compatible = "arm,armv8";

> >                         reg = <0x0 0x3>;

> >                         enable-method = "psci";

> > +                       cpu-idle-states = <&LITTLE_CPU_SLEEP_0 &LITTLE_CPU_SLEEP_1>;

> >                         efficiency = <1024>;

> >                         next-level-cache = <&L2_0>;

> >                         L1_I_3: l1-icache {

> > @@ -142,6 +146,7 @@

> >                         compatible = "arm,armv8";

> >                         reg = <0x0 0x100>;

> >                         enable-method = "psci";

> > +                       cpu-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;

> >                         efficiency = <1536>;

> >                         next-level-cache = <&L2_1>;

> >                         L2_1: l2-cache {

> > @@ -161,6 +166,7 @@

> >                         compatible = "arm,armv8";

> >                         reg = <0x0 0x101>;

> >                         enable-method = "psci";

> > +                       cpu-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;

> >                         efficiency = <1536>;

> >                         next-level-cache = <&L2_1>;

> >                         L1_I_101: l1-icache {

> > @@ -176,6 +182,7 @@

> >                         compatible = "arm,armv8";

> >                         reg = <0x0 0x102>;

> >                         enable-method = "psci";

> > +                       cpu-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;

> >                         efficiency = <1536>;

> >                         next-level-cache = <&L2_1>;

> >                         L1_I_102: l1-icache {

> > @@ -191,6 +198,7 @@

> >                         compatible = "arm,armv8";

> >                         reg = <0x0 0x103>;

> >                         enable-method = "psci";

> > +                       cpu-idle-states = <&BIG_CPU_SLEEP_0 &BIG_CPU_SLEEP_1>;

> >                         efficiency = <1536>;

> >                         next-level-cache = <&L2_1>;

> >                         L1_I_103: l1-icache {

> > @@ -238,6 +246,48 @@

> >                                 };

> >                         };

> >                 };

> > +

> > +               idle-states {

> > +                       entry-method = "psci";

> > +

> > +                       LITTLE_CPU_SLEEP_0: cpu-sleep-0-0 {

> > +                               compatible = "arm,idle-state";

> > +                               idle-state-name = "little-retention";

> > +                               arm,psci-suspend-param = <0x00000002>;

> > +                               entry-latency-us = <43>;

> > +                               exit-latency-us = <86>;

> > +                               min-residency-us = <200>;

> > +                       };

> > +

> > +                       LITTLE_CPU_SLEEP_1: cpu-sleep-0-1 {

> > +                               compatible = "arm,idle-state";

> > +                               idle-state-name = "little-power-collapse";

> > +                               arm,psci-suspend-param = <0x00000003>;

> > +                               entry-latency-us = <100>;

> > +                               exit-latency-us = <612>;

> > +                               min-residency-us = <1000>;

> > +                               local-timer-stop;

> > +                       };

> > +

> > +                       BIG_CPU_SLEEP_0: cpu-sleep-1-0 {

> > +                               compatible = "arm,idle-state";

> > +                               idle-state-name = "big-retention";

> > +                               arm,psci-suspend-param = <0x00000002>;

> > +                               entry-latency-us = <41>;

> > +                               exit-latency-us = <82>;

> > +                               min-residency-us = <200>;

> > +                       };

> > +

> > +                       BIG_CPU_SLEEP_1: cpu-sleep-1-1 {

> > +                               compatible = "arm,idle-state";

> > +                               idle-state-name = "big-power-collapse";

> > +                               arm,psci-suspend-param = <0x00000003>;

> > +                               entry-latency-us = <100>;

> > +                               exit-latency-us = <525>;

> > +                               min-residency-us = <1000>;

> > +                               local-timer-stop;

> > +                       };

> > +               };

> >         };

> >

> >         firmware {

> > --

> > 2.17.1

> >
Niklas Cassel Oct. 2, 2019, 9:19 a.m. UTC | #6
On Mon, Sep 30, 2019 at 04:20:15PM -0600, Jeffrey Hugo wrote:
> Amit, the merged version of the below change causes a boot failure

> (nasty hang, sometimes with RCU stalls) on the msm8998 laptops.  Oddly

> enough, it seems to be resolved if I remove the cpu-idle-states

> property from one of the cpu nodes.

> 

> I see no issues with the msm8998 MTP.


Hello Jeffrey, Amit,

If the PSCI idle states work properly on the msm8998 devboard (MTP),
but causes crashes on msm8998 laptops, the only logical change is
that the PSCI firmware is different between the two devices.


Kind regards,
Niklas
Jeffrey Hugo Oct. 2, 2019, 6:18 p.m. UTC | #7
On Wed, Oct 2, 2019 at 3:27 AM Niklas Cassel <niklas.cassel@linaro.org> wrote:
>

> On Wed, Oct 02, 2019 at 11:19:50AM +0200, Niklas Cassel wrote:

> > On Mon, Sep 30, 2019 at 04:20:15PM -0600, Jeffrey Hugo wrote:

> > > Amit, the merged version of the below change causes a boot failure

> > > (nasty hang, sometimes with RCU stalls) on the msm8998 laptops.  Oddly

> > > enough, it seems to be resolved if I remove the cpu-idle-states

> > > property from one of the cpu nodes.

> > >

> > > I see no issues with the msm8998 MTP.

> >

> > Hello Jeffrey, Amit,

> >

> > If the PSCI idle states work properly on the msm8998 devboard (MTP),

> > but causes crashes on msm8998 laptops, the only logical change is

> > that the PSCI firmware is different between the two devices.

>

> Since the msm8998 laptops boot using ACPI, perhaps these laptops

> doesn't support PSCI/have any PSCI firmware at all.


They have PSCI.  If there was no PSCI, I would expect the PSCI
get_version request from Linux to fail, and all PSCI functionality to
be disabled.

However, your mention about ACPI sparked a thought.  ACPI describes
the idle states, along with the PSCI info, in the ACPI0007 devices.
Those exist on the laptops, and the info mostly correlates with Amit's
patch (ACPI seems to be a bit more conservative about the latencies,
and describes one additional deeper state).  However, upon a detailed
analysis of the ACPI description, I did find something relevant - the
retention state is not enabled.

So, I hacked out the retention state from Amit's patch, and I did not
observe a hang.  I used sysfs, and appeared able to validate that the
power collapse state was being used successfully.

I'm guessing that something is weird with the laptops, where the CPUs
can go into retention, but not come out, thus causing issues.

I'll post a patch to fix up the laptops.  Thanks for all the help.
Jeffrey Hugo Oct. 4, 2019, 3:14 a.m. UTC | #8
On Thu, Oct 3, 2019 at 7:36 PM Amit Kucheria <amit.kucheria@linaro.org> wrote:
>

> On Wed, Oct 2, 2019 at 11:48 PM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote:

> >

> > On Wed, Oct 2, 2019 at 3:27 AM Niklas Cassel <niklas.cassel@linaro.org> wrote:

> > >

> > > On Wed, Oct 02, 2019 at 11:19:50AM +0200, Niklas Cassel wrote:

> > > > On Mon, Sep 30, 2019 at 04:20:15PM -0600, Jeffrey Hugo wrote:

> > > > > Amit, the merged version of the below change causes a boot failure

> > > > > (nasty hang, sometimes with RCU stalls) on the msm8998 laptops.  Oddly

> > > > > enough, it seems to be resolved if I remove the cpu-idle-states

> > > > > property from one of the cpu nodes.

> > > > >

> > > > > I see no issues with the msm8998 MTP.

> > > >

> > > > Hello Jeffrey, Amit,

> > > >

> > > > If the PSCI idle states work properly on the msm8998 devboard (MTP),

> > > > but causes crashes on msm8998 laptops, the only logical change is

> > > > that the PSCI firmware is different between the two devices.

> > >

> > > Since the msm8998 laptops boot using ACPI, perhaps these laptops

> > > doesn't support PSCI/have any PSCI firmware at all.

> >

> > They have PSCI.  If there was no PSCI, I would expect the PSCI

> > get_version request from Linux to fail, and all PSCI functionality to

> > be disabled.

> >

> > However, your mention about ACPI sparked a thought.  ACPI describes

> > the idle states, along with the PSCI info, in the ACPI0007 devices.

> > Those exist on the laptops, and the info mostly correlates with Amit's

> > patch (ACPI seems to be a bit more conservative about the latencies,

> > and describes one additional deeper state).  However, upon a detailed

> > analysis of the ACPI description, I did find something relevant - the

> > retention state is not enabled.

> >

> > So, I hacked out the retention state from Amit's patch, and I did not

> > observe a hang.  I used sysfs, and appeared able to validate that the

> > power collapse state was being used successfully.

>

> Interesting that the shallower sleep state was causing problems.

> Usually, it is the deeper states that cause problems. So you plan to

> override the idle states table in the board-specific DT?


Yes.  Already posted.

>

> Why does the platform even rely on DT? Shouldn't we use the ACPI tables instead?


In theory, yes.  However the ACPI seems to be incomplete (assumes
things are just hardcoded in the driver maybe?) and has tons of
non-standard things in it.  DT seems to be the easy path to
enablement.
>

> > I'm guessing that something is weird with the laptops, where the CPUs

> > can go into retention, but not come out, thus causing issues.

> >

> > I'll post a patch to fix up the laptops.  Thanks for all the help.