| Message ID | 8648ba97d49a9f731001e4b36611be9650e37f37.1557486950.git.amit.kucheria@linaro.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Andy Gross |
| Headers | show |
| Series | qcom: Add cpuidle to some platforms | expand |
On Fri, May 10, 2019 at 04:59:44PM +0530, Amit Kucheria wrote: > Add device bindings for cpuidle states for cpu devices. > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > --- > arch/arm64/boot/dts/qcom/msm8996.dtsi | 28 +++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi > index c761269caf80..b615bcb9e351 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 = <&LITTLE_CPU_PD>; > 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 = <&LITTLE_CPU_PD>; > next-level-cache = <&L2_0>; > }; > > @@ -115,6 +117,7 @@ > compatible = "qcom,kryo"; > reg = <0x0 0x100>; > enable-method = "psci"; > + cpu-idle-states = <&BIG_CPU_PD>; > 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 = <&BIG_CPU_PD>; > next-level-cache = <&L2_1>; > }; > > @@ -151,6 +155,30 @@ > }; > }; > }; > + > + idle-states { > + entry-method="psci"; Please add a space before and after "=". > + > + LITTLE_CPU_PD: little-power-down { In Documentation/devicetree/bindings/arm/idle-states.txt they seem to use labels such as CPU_SLEEP_0_0 for the first cluster and CPU_SLEEP_1_0 for the second cluster. Please also consider my comment in patch 4/8. > + compatible = "arm,idle-state"; > + idle-state-name = "standalone-power-collapse"; > + arm,psci-suspend-param = <0x00000004>; > + entry-latency-us = <40>; > + exit-latency-us = <40>; Where did you get the latency values from? Downstream seems to use qcom,latency-us = <80> for "fpc". (Sure downstream also defines "fpc-def", but that seems to require additional psci code/calls that doesn't exist upstream.) > + min-residency-us = <300>; > + local-timer-stop; Are you sure that the local timer is stopped? the equivalent DT property to "local-timer-stop" in downstream is "qcom,use-broadcast-timer", and this property seems to be missing from this node: https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8996-pm.dtsi?h=msm-4.4#n158 You could try to remove "local-timer-stop", if it is really needed, then the system should hang without this property. > + }; > + > + BIG_CPU_PD: big-power-down { > + compatible = "arm,idle-state"; > + idle-state-name = "standalone-power-collapse"; > + arm,psci-suspend-param = <0x00000004>; > + entry-latency-us = <40>; > + exit-latency-us = <40>; Where did you get the latency values from? Downstream seems to use qcom,latency-us = <80> for "fpc". (Sure downstream also defines "fpc-def", but that seems to require additional psci code/calls that doesn't exist upstream.) > + min-residency-us = <300>; > + local-timer-stop; Are you sure that the local timer is stopped? the equivalent DT property to "local-timer-stop" in downstream is "qcom,use-broadcast-timer", and this property seems to be missing from this node: https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8996-pm.dtsi?h=msm-4.4#n247 You could try to remove "local-timer-stop", if it is really needed, then the system should hang without this property. > + }; > + }; > }; > > thermal-zones { > -- > 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:44PM +0530, Amit Kucheria wrote: > > Add device bindings for cpuidle states for cpu devices. > > > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > > --- > > arch/arm64/boot/dts/qcom/msm8996.dtsi | 28 +++++++++++++++++++++++++++ > > 1 file changed, 28 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi > > index c761269caf80..b615bcb9e351 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 = <&LITTLE_CPU_PD>; > > 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 = <&LITTLE_CPU_PD>; > > next-level-cache = <&L2_0>; > > }; > > > > @@ -115,6 +117,7 @@ > > compatible = "qcom,kryo"; > > reg = <0x0 0x100>; > > enable-method = "psci"; > > + cpu-idle-states = <&BIG_CPU_PD>; > > 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 = <&BIG_CPU_PD>; > > next-level-cache = <&L2_1>; > > }; > > > > @@ -151,6 +155,30 @@ > > }; > > }; > > }; > > + > > + idle-states { > > + entry-method="psci"; > > Please add a space before and after "=". > > > + > > + LITTLE_CPU_PD: little-power-down { > > In Documentation/devicetree/bindings/arm/idle-states.txt > they seem to use labels such as CPU_SLEEP_0_0 for the first > cluster and CPU_SLEEP_1_0 for the second cluster. Will change this to LITTLE_CPU_SLEEP_0. I feel there is value in keeping BIG and LITTLE in the name explicitly to improve readability when correlating the idle state parameters to each CPU. > > Please also consider my comment in patch 4/8. > > > + compatible = "arm,idle-state"; > > + idle-state-name = "standalone-power-collapse"; > > + arm,psci-suspend-param = <0x00000004>; > > + entry-latency-us = <40>; > > + exit-latency-us = <40>; > > Where did you get the latency values from? > Downstream seems to use qcom,latency-us = <80> for "fpc". > Will fix. > (Sure downstream also defines "fpc-def", but that seems to require > additional psci code/calls that doesn't exist upstream.) > > > + min-residency-us = <300>; > > + local-timer-stop; > > Are you sure that the local timer is stopped? > the equivalent DT property to "local-timer-stop" in downstream is > "qcom,use-broadcast-timer", and this property seems to be missing > from this node: > https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8996-pm.dtsi?h=msm-4.4#n158 > > You could try to remove "local-timer-stop", if it is really needed, > then the system should hang without this property. Will review and test again. > > > + }; > > + > > + BIG_CPU_PD: big-power-down { > > + compatible = "arm,idle-state"; > > + idle-state-name = "standalone-power-collapse"; > > + arm,psci-suspend-param = <0x00000004>; > > + entry-latency-us = <40>; > > + exit-latency-us = <40>; > > Where did you get the latency values from? > Downstream seems to use qcom,latency-us = <80> for "fpc". > > (Sure downstream also defines "fpc-def", but that seems to require > additional psci code/calls that doesn't exist upstream.) > > > + min-residency-us = <300>; > > + local-timer-stop; > > Are you sure that the local timer is stopped? > the equivalent DT property to "local-timer-stop" in downstream is > "qcom,use-broadcast-timer", and this property seems to be missing > from this node: > https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8996-pm.dtsi?h=msm-4.4#n247 > > You could try to remove "local-timer-stop", if it is really needed, > then the system should hang without this property. > > > > + }; > > + }; > > }; > > > > thermal-zones { > > -- > > 2.17.1 > >
On 10/05/2019 13:29, Amit Kucheria wrote: > Add device bindings for cpuidle states for cpu devices. > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> > --- > arch/arm64/boot/dts/qcom/msm8996.dtsi | 28 +++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi > index c761269caf80..b615bcb9e351 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 = <&LITTLE_CPU_PD>; It is the same micro architecture, the CPUS differ by their max OPP. Shall we call it really little? I take the opportunity to report the capacity-dmips-mhz attribute is missing. The max capacity computation is not triggered, thus the scheduler see the same capacity for both cluster even if one has less OPP. Adding capacity-dmips-mhz = <1024>; to all CPUs will fix it. > 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 = <&LITTLE_CPU_PD>; > next-level-cache = <&L2_0>; > }; > > @@ -115,6 +117,7 @@ > compatible = "qcom,kryo"; > reg = <0x0 0x100>; > enable-method = "psci"; > + cpu-idle-states = <&BIG_CPU_PD>; > 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 = <&BIG_CPU_PD>; > next-level-cache = <&L2_1>; > }; > > @@ -151,6 +155,30 @@ > }; > }; > }; > + > + idle-states { > + entry-method="psci"; > + > + LITTLE_CPU_PD: little-power-down { > + compatible = "arm,idle-state"; > + idle-state-name = "standalone-power-collapse"; > + arm,psci-suspend-param = <0x00000004>; > + entry-latency-us = <40>; > + exit-latency-us = <40>; > + min-residency-us = <300>; > + local-timer-stop; > + }; > + > + BIG_CPU_PD: big-power-down { > + compatible = "arm,idle-state"; > + idle-state-name = "standalone-power-collapse"; > + arm,psci-suspend-param = <0x00000004>; > + entry-latency-us = <40>; > + exit-latency-us = <40>; > + min-residency-us = <300>; > + local-timer-stop; > + }; > + }; > }; > > thermal-zones { >
diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi index c761269caf80..b615bcb9e351 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 = <&LITTLE_CPU_PD>; 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 = <&LITTLE_CPU_PD>; next-level-cache = <&L2_0>; }; @@ -115,6 +117,7 @@ compatible = "qcom,kryo"; reg = <0x0 0x100>; enable-method = "psci"; + cpu-idle-states = <&BIG_CPU_PD>; 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 = <&BIG_CPU_PD>; next-level-cache = <&L2_1>; }; @@ -151,6 +155,30 @@ }; }; }; + + idle-states { + entry-method="psci"; + + LITTLE_CPU_PD: little-power-down { + compatible = "arm,idle-state"; + idle-state-name = "standalone-power-collapse"; + arm,psci-suspend-param = <0x00000004>; + entry-latency-us = <40>; + exit-latency-us = <40>; + min-residency-us = <300>; + local-timer-stop; + }; + + BIG_CPU_PD: big-power-down { + compatible = "arm,idle-state"; + idle-state-name = "standalone-power-collapse"; + arm,psci-suspend-param = <0x00000004>; + entry-latency-us = <40>; + exit-latency-us = <40>; + min-residency-us = <300>; + local-timer-stop; + }; + }; }; thermal-zones {
Add device bindings for cpuidle states for cpu devices. Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org> --- arch/arm64/boot/dts/qcom/msm8996.dtsi | 28 +++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)