| Message ID | cover.1557486950.git.amit.kucheria@linaro.org |
|---|---|
| Headers | show |
| Series | qcom: Add cpuidle to some platforms | expand |
On Fri, May 10, 2019 at 04:59:42PM +0530, Amit Kucheria wrote: > Instead of using Qualcomm-specific terminology, use generic node names > for the idle states that are easier to understand. Move the description > into the "idle-state-name" property. > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > --- > arch/arm64/boot/dts/qcom/msm8916.dtsi | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi > index ded1052e5693..400b609bb3fd 100644 > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi > @@ -110,7 +110,7 @@ > reg = <0x0>; > next-level-cache = <&L2_0>; > enable-method = "psci"; > - cpu-idle-states = <&CPU_SPC>; > + cpu-idle-states = <&CPU_SLEEP_0>; > clocks = <&apcs>; > operating-points-v2 = <&cpu_opp_table>; > #cooling-cells = <2>; > @@ -122,7 +122,7 @@ > reg = <0x1>; > next-level-cache = <&L2_0>; > enable-method = "psci"; > - cpu-idle-states = <&CPU_SPC>; > + cpu-idle-states = <&CPU_SLEEP_0>; > clocks = <&apcs>; > operating-points-v2 = <&cpu_opp_table>; > #cooling-cells = <2>; > @@ -134,7 +134,7 @@ > reg = <0x2>; > next-level-cache = <&L2_0>; > enable-method = "psci"; > - cpu-idle-states = <&CPU_SPC>; > + cpu-idle-states = <&CPU_SLEEP_0>; > clocks = <&apcs>; > operating-points-v2 = <&cpu_opp_table>; > #cooling-cells = <2>; > @@ -146,7 +146,7 @@ > reg = <0x3>; > next-level-cache = <&L2_0>; > enable-method = "psci"; > - cpu-idle-states = <&CPU_SPC>; > + cpu-idle-states = <&CPU_SLEEP_0>; > clocks = <&apcs>; > operating-points-v2 = <&cpu_opp_table>; > #cooling-cells = <2>; > @@ -160,8 +160,9 @@ > idle-states { > entry-method="psci"; Please add a space before and after "=". > > - CPU_SPC: spc { > + CPU_SLEEP_0: cpu-sleep-0 { While I like your idea of using power state names from Server Base System Architecture document (SBSA) where applicable, does each qcom power state have a matching state in SBSA? These are the qcom power states: https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/Documentation/devicetree/bindings/arm/msm/lpm-levels.txt?h=msm-4.4#n53 Note that qcom defines: "wfi", "retention", "gdhs", "pc", "fpc" while SBSA simply defines "idle_standby" (aka wfi), "idle_retention", "sleep". Unless you know the equivalent name for each qcom power state (perhaps several qcom power states are really the same SBSA state?), I think that you should omit the renaming from this patch series. > compatible = "arm,idle-state"; > + idle-state-name = "standalone-power-collapse"; > arm,psci-suspend-param = <0x40000002>; > entry-latency-us = <130>; > exit-latency-us = <150>; > -- > 2.17.1 >
On Fri, May 10, 2019 at 04:59:43PM +0530, Amit Kucheria wrote: > From: Niklas Cassel <niklas.cassel@linaro.org> > > Add device bindings for cpuidle states for cpu devices. > > [amit: rename the idle-states to more generic names and fixups] > > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org> > Reviewed-by: Vinod Koul <vkoul@kernel.org> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > --- > 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..369c59c35bc7 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"; Please add a space before and after "=". > + > + CPU_SLEEP_0: cpu-sleep-0 { Regarding CPU_SLEEP_0 vs CPU_PC, see my comment on patch 4/8. > + 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, May 14, 2019 at 9:42 PM Niklas Cassel <niklas.cassel@linaro.org> wrote: > > On Fri, May 10, 2019 at 04:59:42PM +0530, Amit Kucheria wrote: > > Instead of using Qualcomm-specific terminology, use generic node names > > for the idle states that are easier to understand. Move the description > > into the "idle-state-name" property. > > > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > > --- > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi > > index ded1052e5693..400b609bb3fd 100644 > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi > > @@ -110,7 +110,7 @@ > > reg = <0x0>; > > next-level-cache = <&L2_0>; > > enable-method = "psci"; > > - cpu-idle-states = <&CPU_SPC>; > > + cpu-idle-states = <&CPU_SLEEP_0>; > > clocks = <&apcs>; > > operating-points-v2 = <&cpu_opp_table>; > > #cooling-cells = <2>; > > @@ -122,7 +122,7 @@ > > reg = <0x1>; > > next-level-cache = <&L2_0>; > > enable-method = "psci"; > > - cpu-idle-states = <&CPU_SPC>; > > + cpu-idle-states = <&CPU_SLEEP_0>; > > clocks = <&apcs>; > > operating-points-v2 = <&cpu_opp_table>; > > #cooling-cells = <2>; > > @@ -134,7 +134,7 @@ > > reg = <0x2>; > > next-level-cache = <&L2_0>; > > enable-method = "psci"; > > - cpu-idle-states = <&CPU_SPC>; > > + cpu-idle-states = <&CPU_SLEEP_0>; > > clocks = <&apcs>; > > operating-points-v2 = <&cpu_opp_table>; > > #cooling-cells = <2>; > > @@ -146,7 +146,7 @@ > > reg = <0x3>; > > next-level-cache = <&L2_0>; > > enable-method = "psci"; > > - cpu-idle-states = <&CPU_SPC>; > > + cpu-idle-states = <&CPU_SLEEP_0>; > > clocks = <&apcs>; > > operating-points-v2 = <&cpu_opp_table>; > > #cooling-cells = <2>; > > @@ -160,8 +160,9 @@ > > idle-states { > > entry-method="psci"; > > Please add a space before and after "=". > > > > > - CPU_SPC: spc { > > + CPU_SLEEP_0: cpu-sleep-0 { > > While I like your idea of using power state names from > Server Base System Architecture document (SBSA) where applicable, > does each qcom power state have a matching state in SBSA? > > These are the qcom power states: > https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/Documentation/devicetree/bindings/arm/msm/lpm-levels.txt?h=msm-4.4#n53 > > Note that qcom defines: > "wfi", "retention", "gdhs", "pc", "fpc" > while SBSA simply defines "idle_standby" (aka wfi), "idle_retention", "sleep". > > Unless you know the equivalent name for each qcom power state > (perhaps several qcom power states are really the same SBSA state?), > I think that you should omit the renaming from this patch series. That is what SLEEP_0, SLEEP_1, SLEEP_2 could be used for. IOW, all these qcom definitions are nicely represented in the state-name and we could simply stick to SLEEP_0, SLEEP_1 for the node names. There is wide variability in the the names of the qcom idle states across SoC families downstream, so I'd argue against using those for the node names. Just for cpu states (non-wfi) I see the use of the following names downstream across families. The C<num> seems to come from x86 world[1]: - C4, standalone power collapse (spc) - C4, power collapse (fpc) - C2D, retention - C3, power collapse (pc) - C4, rail power collapse (rail-pc) [1] https://www.hardwaresecrets.com/everything-you-need-to-know-about-the-cpu-c-states-power-saving-modes/ > > compatible = "arm,idle-state"; > > + idle-state-name = "standalone-power-collapse"; > > arm,psci-suspend-param = <0x40000002>; > > entry-latency-us = <130>; > > exit-latency-us = <150>; > > -- > > 2.17.1 > >
On 10/05/2019 13:29, Amit Kucheria wrote: > The idle-states binding documentation[1] mentions that the > 'entry-method' property is required on 64-bit platforms and must be set > to "psci". > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi > index 0803ca8c02da..ded1052e5693 100644 > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi > @@ -158,6 +158,8 @@ > }; > > idle-states { > + entry-method="psci"; > + > CPU_SPC: spc { > compatible = "arm,idle-state"; > arm,psci-suspend-param = <0x40000002>; > -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
On 10/05/2019 13:29, Amit Kucheria wrote: > Instead of using Qualcomm-specific terminology, use generic node names > for the idle states that are easier to understand. Move the description > into the "idle-state-name" property. > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org> > --- > arch/arm64/boot/dts/qcom/msm8916.dtsi | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi > index ded1052e5693..400b609bb3fd 100644 > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi > @@ -110,7 +110,7 @@ > reg = <0x0>; > next-level-cache = <&L2_0>; > enable-method = "psci"; > - cpu-idle-states = <&CPU_SPC>; > + cpu-idle-states = <&CPU_SLEEP_0>; > clocks = <&apcs>; > operating-points-v2 = <&cpu_opp_table>; > #cooling-cells = <2>; > @@ -122,7 +122,7 @@ > reg = <0x1>; > next-level-cache = <&L2_0>; > enable-method = "psci"; > - cpu-idle-states = <&CPU_SPC>; > + cpu-idle-states = <&CPU_SLEEP_0>; > clocks = <&apcs>; > operating-points-v2 = <&cpu_opp_table>; > #cooling-cells = <2>; > @@ -134,7 +134,7 @@ > reg = <0x2>; > next-level-cache = <&L2_0>; > enable-method = "psci"; > - cpu-idle-states = <&CPU_SPC>; > + cpu-idle-states = <&CPU_SLEEP_0>; > clocks = <&apcs>; > operating-points-v2 = <&cpu_opp_table>; > #cooling-cells = <2>; > @@ -146,7 +146,7 @@ > reg = <0x3>; > next-level-cache = <&L2_0>; > enable-method = "psci"; > - cpu-idle-states = <&CPU_SPC>; > + cpu-idle-states = <&CPU_SLEEP_0>; > clocks = <&apcs>; > operating-points-v2 = <&cpu_opp_table>; > #cooling-cells = <2>; > @@ -160,8 +160,9 @@ > idle-states { > entry-method="psci"; > > - CPU_SPC: spc { > + CPU_SLEEP_0: cpu-sleep-0 { > compatible = "arm,idle-state"; > + idle-state-name = "standalone-power-collapse"; > arm,psci-suspend-param = <0x40000002>; > entry-latency-us = <130>; > exit-latency-us = <150>; > -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
On Wed, May 15, 2019 at 6:33 PM Niklas Cassel <niklas.cassel@linaro.org> wrote: > > On Wed, May 15, 2019 at 03:43:19PM +0530, Amit Kucheria wrote: > > On Tue, May 14, 2019 at 9:42 PM Niklas Cassel <niklas.cassel@linaro.org> wrote: > > > > > > On Fri, May 10, 2019 at 04:59:42PM +0530, Amit Kucheria wrote: > > > > Instead of using Qualcomm-specific terminology, use generic node names > > > > for the idle states that are easier to understand. Move the description > > > > into the "idle-state-name" property. > > > > > > > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > > > > --- > > > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 11 ++++++----- > > > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi > > > > index ded1052e5693..400b609bb3fd 100644 > > > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi > > > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi > > > > @@ -110,7 +110,7 @@ > > > > reg = <0x0>; > > > > next-level-cache = <&L2_0>; > > > > enable-method = "psci"; > > > > - cpu-idle-states = <&CPU_SPC>; > > > > + cpu-idle-states = <&CPU_SLEEP_0>; > > > > clocks = <&apcs>; > > > > operating-points-v2 = <&cpu_opp_table>; > > > > #cooling-cells = <2>; > > > > @@ -122,7 +122,7 @@ > > > > reg = <0x1>; > > > > next-level-cache = <&L2_0>; > > > > enable-method = "psci"; > > > > - cpu-idle-states = <&CPU_SPC>; > > > > + cpu-idle-states = <&CPU_SLEEP_0>; > > > > clocks = <&apcs>; > > > > operating-points-v2 = <&cpu_opp_table>; > > > > #cooling-cells = <2>; > > > > @@ -134,7 +134,7 @@ > > > > reg = <0x2>; > > > > next-level-cache = <&L2_0>; > > > > enable-method = "psci"; > > > > - cpu-idle-states = <&CPU_SPC>; > > > > + cpu-idle-states = <&CPU_SLEEP_0>; > > > > clocks = <&apcs>; > > > > operating-points-v2 = <&cpu_opp_table>; > > > > #cooling-cells = <2>; > > > > @@ -146,7 +146,7 @@ > > > > reg = <0x3>; > > > > next-level-cache = <&L2_0>; > > > > enable-method = "psci"; > > > > - cpu-idle-states = <&CPU_SPC>; > > > > + cpu-idle-states = <&CPU_SLEEP_0>; > > > > clocks = <&apcs>; > > > > operating-points-v2 = <&cpu_opp_table>; > > > > #cooling-cells = <2>; > > > > @@ -160,8 +160,9 @@ > > > > idle-states { > > > > entry-method="psci"; > > > > > > Please add a space before and after "=". > > > > > > > > > > > - CPU_SPC: spc { > > > > + CPU_SLEEP_0: cpu-sleep-0 { > > > > > > While I like your idea of using power state names from > > > Server Base System Architecture document (SBSA) where applicable, > > > does each qcom power state have a matching state in SBSA? > > > > > > These are the qcom power states: > > > https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/Documentation/devicetree/bindings/arm/msm/lpm-levels.txt?h=msm-4.4#n53 > > > > > > Note that qcom defines: > > > "wfi", "retention", "gdhs", "pc", "fpc" > > > while SBSA simply defines "idle_standby" (aka wfi), "idle_retention", "sleep". > > > > > > Unless you know the equivalent name for each qcom power state > > > (perhaps several qcom power states are really the same SBSA state?), > > > I think that you should omit the renaming from this patch series. > > > > That is what SLEEP_0, SLEEP_1, SLEEP_2 could be used for. > > Ok, sounds good to me. > > > > > IOW, all these qcom definitions are nicely represented in the > > state-name and we could simply stick to SLEEP_0, SLEEP_1 for the node > > names. There is wide variability in the the names of the qcom idle > > states across SoC families downstream, so I'd argue against using > > those for the node names. > > > > Just for cpu states (non-wfi) I see the use of the following names > > downstream across families. The C<num> seems to come from x86 > > world[1]: > > > > - C4, standalone power collapse (spc) > > - C4, power collapse (fpc) > > - C2D, retention > > - C3, power collapse (pc) > > - C4, rail power collapse (rail-pc) > > > > [1] https://www.hardwaresecrets.com/everything-you-need-to-know-about-the-cpu-c-states-power-saving-modes/ > > Indeed, there seems to be mixed names used, I've also seen "fpc-def". > > So, you have convinced me. > > > Kind regards, > Niklas Can I take that as a Reviewed-by?
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 (6): arm64: dts: Fix various entry-method properties to reflect documentation Documentation: arm: Link idle-states binding to code 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 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 | 7 +++ .../arm64/boot/dts/freescale/fsl-ls1028a.dtsi | 2 +- arch/arm64/boot/dts/qcom/msm8916.dtsi | 13 ++-- arch/arm64/boot/dts/qcom/msm8996.dtsi | 28 +++++++++ arch/arm64/boot/dts/qcom/msm8998.dtsi | 32 ++++++++++ arch/arm64/boot/dts/qcom/qcs404.dtsi | 18 ++++++ arch/arm64/boot/dts/qcom/sdm845.dtsi | 62 +++++++++++++++++++ 7 files changed, 156 insertions(+), 6 deletions(-) -- 2.17.1