| 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:38PM +0530, Amit Kucheria wrote: > 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. Hello Amit, Your subject looks a bit weird: [PATCHv1 0/8] qcom: Add cpuidle to some platforms No need to specify reroll count if it is the first version, and there is usually a space between PATCH and reroll count. git format-patch [(--reroll-count|-v) <n>] it should take care of this for you. Please also add all that is on either to: or cc: in any patch in the series as cc: for the cover letter. Kind regards, Niklas > > > 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 >
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?
On Tue, May 21, 2019 at 11:08:09AM +0530, Amit Kucheria wrote: > 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? Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
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(-)