| Message ID | cover.1558430617.git.amit.kucheria@linaro.org |
|---|---|
| Headers | show |
| Series | qcom: Add cpuidle to some platforms | expand |
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 >
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 >
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 >
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
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 > >
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
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.
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.
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