LinuxLists.cc - [PATCHv1 0/8] qcom: Add cpuidle to some platforms

[permalink] [raw]

Subject: [PATCHv1 1/8] arm64: dts: Fix various entry-method properties to reflect documentation

The idle-states binding documentation[1] mentions that the
'entry-method' property is required on 64-bit platforms and must be set
to "psci".

We fixed up all uses of the entry-method property in
commit e9880240e4f4 ("arm64: dts: Fix various entry-method properties to
reflect documentation"). But a new one has appeared. Fix it up.

Cc: Sudeep Holla <[email protected]>
Signed-off-by: Amit Kucheria <[email protected]>
---
arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
index 2896bbcfa3bb..42e7822a0227 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
@@ -51,7 +51,7 @@
* PSCI node is not added default, U-boot will add missing
* parts if it determines to use PSCI.
*/
- entry-method = "arm,psci";
+ entry-method = "psci";

CPU_PH20: cpu-ph20 {
compatible = "arm,idle-state";
--
2.17.1

2019-05-10 11:31:20

[permalink] [raw]

Subject: [PATCHv1 2/8] Documentation: arm: Link idle-states binding to code

The enable-method needs to be psci for the psci_cpuidle_ops to be
correctly registered.

Add a note to the binding documentation on where to find the declaration
of the enable-method since it is a macro and escapes any attempts to
grep for it.

Cc: Sudeep Holla <[email protected]>
Signed-off-by: Amit Kucheria <[email protected]>
---
Documentation/devicetree/bindings/arm/idle-states.txt | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/idle-states.txt b/Documentation/devicetree/bindings/arm/idle-states.txt
index 45730ba60af5..3a42335a6f3d 100644
--- a/Documentation/devicetree/bindings/arm/idle-states.txt
+++ b/Documentation/devicetree/bindings/arm/idle-states.txt
@@ -239,6 +239,10 @@ processor idle states, defined as device tree nodes, are listed.
# On ARM v8 64-bit this property is required and must
be:
- "psci"
+ (This assumes that the enable-method is "psci"
+ in the cpu node[6] that then uses the
+ CPUIDLE_METHOD_OF_DECLARE macro to setup the
+ psci_cpuidle_ops callbacks)
# On ARM 32-bit systems this property is optional

The nodes describing the idle states (state) can only be defined within the
@@ -697,3 +701,6 @@ cpus {

[5] Devicetree Specification
https://www.devicetree.org/specifications/
+
+[6] ARM Linux Kernel documentation - Booting AArch64 Linux
+ Documentation/arm64/booting.txt
--
2.17.1

2019-05-10 11:31:30

[permalink] [raw]

Subject: [PATCHv1 4/8] arm64: dts: qcom: msm8916: Use more generic idle state names

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 <[email protected]>
---
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>;
--
2.17.1

2019-05-10 11:32:04

[permalink] [raw]

Subject: [PATCHv1 5/8] arm64: dts: qcom: qcs404: Add PSCI cpuidle low power states

From: Niklas Cassel <[email protected]>

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 <[email protected]>
Reviewed-by: Vinod Koul <[email protected]>
Signed-off-by: Amit Kucheria <[email protected]>
---
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";
+
+ 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

2019-05-10 11:32:15

[permalink] [raw]

Subject: [PATCHv1 7/8] arm64: dts: qcom: msm8998: Add PSCI cpuidle low power states

Add device bindings for cpuidle states for cpu devices.

Cc: Marc Gonzalez <[email protected]>
Signed-off-by: Amit Kucheria <[email protected]>
---
arch/arm64/boot/dts/qcom/msm8998.dtsi | 32 +++++++++++++++++++++++++++
1 file changed, 32 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
index 3fd0769fe648..208281f318e2 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_PD>;
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_PD>;
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_PD>;
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_PD>;
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_PD>;
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_PD>;
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_PD>;
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_PD>;
efficiency = <1536>;
next-level-cache = <&L2_1>;
L1_I_103: l1-icache {
@@ -238,6 +246,30 @@
};
};
};
+
+ idle-states {
+ entry-method="psci";
+
+ LITTLE_CPU_PD: little-power-down {
+ compatible = "arm,idle-state";
+ idle-state-name = "little-power-down";
+ arm,psci-suspend-param = <0x00000002>;
+ entry-latency-us = <43>;
+ exit-latency-us = <43>;
+ min-residency-us = <200>;
+ local-timer-stop;
+ };
+
+ BIG_CPU_PD: big-power-down {
+ compatible = "arm,idle-state";
+ idle-state-name = "big-power-down";
+ arm,psci-suspend-param = <0x00000002>;
+ entry-latency-us = <41>;
+ exit-latency-us = <41>;
+ min-residency-us = <200>;
+ local-timer-stop;
+ };
+ };
};

firmware {
--
2.17.1

2019-05-10 11:32:25

[permalink] [raw]

Subject: [PATCHv1 8/8] arm64: dts: qcom: sdm845: Add PSCI cpuidle low power states

From: "Raju P.L.S.S.S.N" <[email protected]>

Add device bindings for cpuidle states for cpu devices.

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

Cc: <[email protected]>
Cc: <[email protected]>
Signed-off-by: Raju P.L.S.S.S.N <[email protected]>
Reviewed-by: Evan Green <[email protected]>
Signed-off-by: Amit Kucheria <[email protected]>
---
arch/arm64/boot/dts/qcom/sdm845.dtsi | 62 ++++++++++++++++++++++++++++
1 file changed, 62 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 5308f1671824..2c8c54e4bd77 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -119,6 +119,7 @@
compatible = "qcom,kryo385";
reg = <0x0 0x0>;
enable-method = "psci";
+ cpu-idle-states = <&LITTLE_CPU_PD &LITTLE_CPU_RPD &CLUSTER_PD>;
qcom,freq-domain = <&cpufreq_hw 0>;
#cooling-cells = <2>;
next-level-cache = <&L2_0>;
@@ -136,6 +137,7 @@
compatible = "qcom,kryo385";
reg = <0x0 0x100>;
enable-method = "psci";
+ cpu-idle-states = <&LITTLE_CPU_PD &LITTLE_CPU_RPD &CLUSTER_PD>;
qcom,freq-domain = <&cpufreq_hw 0>;
#cooling-cells = <2>;
next-level-cache = <&L2_100>;
@@ -150,6 +152,7 @@
compatible = "qcom,kryo385";
reg = <0x0 0x200>;
enable-method = "psci";
+ cpu-idle-states = <&LITTLE_CPU_PD &LITTLE_CPU_RPD &CLUSTER_PD>;
qcom,freq-domain = <&cpufreq_hw 0>;
#cooling-cells = <2>;
next-level-cache = <&L2_200>;
@@ -164,6 +167,7 @@
compatible = "qcom,kryo385";
reg = <0x0 0x300>;
enable-method = "psci";
+ cpu-idle-states = <&LITTLE_CPU_PD &LITTLE_CPU_RPD &CLUSTER_PD>;
qcom,freq-domain = <&cpufreq_hw 0>;
#cooling-cells = <2>;
next-level-cache = <&L2_300>;
@@ -178,6 +182,7 @@
compatible = "qcom,kryo385";
reg = <0x0 0x400>;
enable-method = "psci";
+ cpu-idle-states = <&BIG_CPU_PD &BIG_CPU_RPD &CLUSTER_PD>;
qcom,freq-domain = <&cpufreq_hw 1>;
#cooling-cells = <2>;
next-level-cache = <&L2_400>;
@@ -192,6 +197,7 @@
compatible = "qcom,kryo385";
reg = <0x0 0x500>;
enable-method = "psci";
+ cpu-idle-states = <&BIG_CPU_PD &BIG_CPU_RPD &CLUSTER_PD>;
qcom,freq-domain = <&cpufreq_hw 1>;
#cooling-cells = <2>;
next-level-cache = <&L2_500>;
@@ -206,6 +212,7 @@
compatible = "qcom,kryo385";
reg = <0x0 0x600>;
enable-method = "psci";
+ cpu-idle-states = <&BIG_CPU_PD &BIG_CPU_RPD &CLUSTER_PD>;
qcom,freq-domain = <&cpufreq_hw 1>;
#cooling-cells = <2>;
next-level-cache = <&L2_600>;
@@ -220,6 +227,7 @@
compatible = "qcom,kryo385";
reg = <0x0 0x700>;
enable-method = "psci";
+ cpu-idle-states = <&BIG_CPU_PD &BIG_CPU_RPD &CLUSTER_PD>;
qcom,freq-domain = <&cpufreq_hw 1>;
#cooling-cells = <2>;
next-level-cache = <&L2_700>;
@@ -228,6 +236,60 @@
next-level-cache = <&L3_0>;
};
};
+
+ idle-states {
+ entry-method = "psci";
+
+ LITTLE_CPU_PD: little-power-down {
+ compatible = "arm,idle-state";
+ idle-state-name = "little-power-down";
+ arm,psci-suspend-param = <0x40000003>;
+ entry-latency-us = <350>;
+ exit-latency-us = <461>;
+ min-residency-us = <1890>;
+ local-timer-stop;
+ };
+
+ LITTLE_CPU_RPD: little-rail-power-down {
+ compatible = "arm,idle-state";
+ idle-state-name = "little-rail-power-down";
+ arm,psci-suspend-param = <0x40000004>;
+ entry-latency-us = <360>;
+ exit-latency-us = <531>;
+ min-residency-us = <3934>;
+ local-timer-stop;
+ };
+
+ BIG_CPU_PD: big-power-down {
+ compatible = "arm,idle-state";
+ idle-state-name = "big-power-down";
+ arm,psci-suspend-param = <0x40000003>;
+ entry-latency-us = <264>;
+ exit-latency-us = <621>;
+ min-residency-us = <952>;
+ local-timer-stop;
+ };
+
+ BIG_CPU_RPD: big-rail-power-down {
+ compatible = "arm,idle-state";
+ idle-state-name = "big-rail-power-down";
+ arm,psci-suspend-param = <0x40000004>;
+ entry-latency-us = <702>;
+ exit-latency-us = <1061>;
+ min-residency-us = <4488>;
+ local-timer-stop;
+ };
+
+ CLUSTER_PD: cluster-power-down {
+ compatible = "arm,idle-state";
+ idle-state-name = "cluster-power-down";
+ arm,psci-suspend-param = <0x400000F4>;
+ entry-latency-us = <3263>;
+ exit-latency-us = <6562>;
+ min-residency-us = <9987>;
+ local-timer-stop;
+ };
+ };
};

pmu {
--
2.17.1

2019-05-10 11:33:43

[permalink] [raw]

Subject: [PATCHv1 6/8] arm64: dts: qcom: msm8996: Add PSCI cpuidle low power states

Add device bindings for cpuidle states for cpu devices.

Signed-off-by: Amit Kucheria <[email protected]>
---
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";
+
+ 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 {
--
2.17.1

2019-05-10 11:48:06

[permalink] [raw]

Subject: [PATCHv1 3/8] arm64: dts: qcom: msm8916: Add entry-method property for the idle-states node

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 <[email protected]>
---
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>;
--
2.17.1

2019-05-10 13:04:12

by Sudeep Holla

[permalink] [raw]

Subject: Re: [PATCHv1 2/8] Documentation: arm: Link idle-states binding to code

On Fri, May 10, 2019 at 04:59:40PM +0530, Amit Kucheria wrote:
> The enable-method needs to be psci for the psci_cpuidle_ops to be
> correctly registered.
>
> Add a note to the binding documentation on where to find the declaration
> of the enable-method since it is a macro and escapes any attempts to
> grep for it.
>
> Cc: Sudeep Holla <[email protected]>
> Signed-off-by: Amit Kucheria <[email protected]>
> ---
> Documentation/devicetree/bindings/arm/idle-states.txt | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/idle-states.txt b/Documentation/devicetree/bindings/arm/idle-states.txt
> index 45730ba60af5..3a42335a6f3d 100644
> --- a/Documentation/devicetree/bindings/arm/idle-states.txt
> +++ b/Documentation/devicetree/bindings/arm/idle-states.txt
> @@ -239,6 +239,10 @@ processor idle states, defined as device tree nodes, are listed.
> # On ARM v8 64-bit this property is required and must
> be:
> - "psci"
> + (This assumes that the enable-method is "psci"
> + in the cpu node[6] that then uses the
> + CPUIDLE_METHOD_OF_DECLARE macro to setup the
> + psci_cpuidle_ops callbacks)

I don't prefer to refer some Linux implementation macros in DT bindings
as they may disappear any day. Further, the use of CPUIDLE_METHOD_OF_DECLARE
is restricted to ARM32 platforms only. So better to move it down without
the reference to the above macro or any kernel implementation details if
possible.

> # On ARM 32-bit systems this property is optional
>

Something like:
"This assumes that the "enable-method" property is set to "psci" in
in the cpu node[6] and use this property to set up the CPU idle
management in OS PM implementations"

With something on these line, you can add:

Acked-by: Sudeep Holla <[email protected]

--
Regards,
Sudeep

2019-05-10 13:17:15

by Marc Gonzalez

[permalink] [raw]

Subject: Re: [PATCHv1 7/8] arm64: dts: qcom: msm8998: Add PSCI cpuidle low power states

On 10/05/2019 13:29, Amit Kucheria wrote:

> Add device bindings for cpuidle states for cpu devices.
>
> Cc: Marc Gonzalez <[email protected]>
> Signed-off-by: Amit Kucheria <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/msm8998.dtsi | 32 +++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> index 3fd0769fe648..208281f318e2 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_PD>;

For some reason, I was expecting the big cores to come first, but according
to /proc/cpuinfo, cores 0-3 are part 0x801, while cores 4-7 are part 0x800.

According to https://github.com/pytorch/cpuinfo/blob/master/src/arm/uarch.c

0x801 = Low-power Kryo 260 / 280 "Silver" -> Cortex-A53
0x800 = High-performance Kryo 260 (r10p2) / Kryo 280 (r10p1) "Gold" -> Cortex-A73

> 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_PD>;
> 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_PD>;
> 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_PD>;
> 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_PD>;
> 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_PD>;
> 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_PD>;
> 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_PD>;
> efficiency = <1536>;
> next-level-cache = <&L2_1>;
> L1_I_103: l1-icache {
> @@ -238,6 +246,30 @@
> };
> };
> };
> +
> + idle-states {
> + entry-method="psci";
> +
> + LITTLE_CPU_PD: little-power-down {
> + compatible = "arm,idle-state";
> + idle-state-name = "little-power-down";
> + arm,psci-suspend-param = <0x00000002>;
> + entry-latency-us = <43>;
> + exit-latency-us = <43>;

Little cores have higher latency (+5%) than big cores?

> + min-residency-us = <200>;
> + local-timer-stop;
> + };
> +
> + BIG_CPU_PD: big-power-down {
> + compatible = "arm,idle-state";
> + idle-state-name = "big-power-down";
> + arm,psci-suspend-param = <0x00000002>;
> + entry-latency-us = <41>;
> + exit-latency-us = <41>;
> + min-residency-us = <200>;
> + local-timer-stop;
> + };
> + };

What is the simplest way to test this patch?

Regards.

2019-05-10 13:50:54

by Sudeep Holla

[permalink] [raw]

Subject: Re: [PATCHv1 1/8] arm64: dts: Fix various entry-method properties to reflect documentation

On Fri, May 10, 2019 at 04:59:39PM +0530, 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".
>
> We fixed up all uses of the entry-method property in
> commit e9880240e4f4 ("arm64: dts: Fix various entry-method properties to
> reflect documentation"). But a new one has appeared. Fix it up.
>
> Cc: Sudeep Holla <[email protected]>

Ah right, new ones always appear for short period.
Anyways,

Acked-by: Sudeep Holla <[email protected]>

> Signed-off-by: Amit Kucheria <[email protected]>
> ---
> arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> index 2896bbcfa3bb..42e7822a0227 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> @@ -51,7 +51,7 @@
> * PSCI node is not added default, U-boot will add missing
> * parts if it determines to use PSCI.
> */
> - entry-method = "arm,psci";
> + entry-method = "psci";
>
> CPU_PH20: cpu-ph20 {
> compatible = "arm,idle-state";
> --
> 2.17.1
>

2019-05-10 14:14:34

[permalink] [raw]

Subject: Re: [PATCHv1 7/8] arm64: dts: qcom: msm8998: Add PSCI cpuidle low power states

On Fri, May 10, 2019 at 6:45 PM Marc Gonzalez <[email protected]> wrote:
>
> On 10/05/2019 13:29, Amit Kucheria wrote:
>
> > Add device bindings for cpuidle states for cpu devices.
> >
> > Cc: Marc Gonzalez <[email protected]>
> > Signed-off-by: Amit Kucheria <[email protected]>
> > ---
> > arch/arm64/boot/dts/qcom/msm8998.dtsi | 32 +++++++++++++++++++++++++++
> > 1 file changed, 32 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> > index 3fd0769fe648..208281f318e2 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_PD>;
>
> For some reason, I was expecting the big cores to come first, but according
> to /proc/cpuinfo, cores 0-3 are part 0x801, while cores 4-7 are part 0x800.
>
> According to https://github.com/pytorch/cpuinfo/blob/master/src/arm/uarch.c
>
> 0x801 = Low-power Kryo 260 / 280 "Silver" -> Cortex-A53
> 0x800 = High-performance Kryo 260 (r10p2) / Kryo 280 (r10p1) "Gold" -> Cortex-A73

Hmm, did I mess up the order of the big and LITTLE cores? I'll take a
look again.

> > 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_PD>;
> > 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_PD>;
> > 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_PD>;
> > 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_PD>;
> > 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_PD>;
> > 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_PD>;
> > 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_PD>;
> > efficiency = <1536>;
> > next-level-cache = <&L2_1>;
> > L1_I_103: l1-icache {
> > @@ -238,6 +246,30 @@
> > };
> > };
> > };
> > +
> > + idle-states {
> > + entry-method="psci";
> > +
> > + LITTLE_CPU_PD: little-power-down {
> > + compatible = "arm,idle-state";
> > + idle-state-name = "little-power-down";
> > + arm,psci-suspend-param = <0x00000002>;
> > + entry-latency-us = <43>;
> > + exit-latency-us = <43>;
>
> Little cores have higher latency (+5%) than big cores?
>
> > + min-residency-us = <200>;
> > + local-timer-stop;
> > + };
> > +
> > + BIG_CPU_PD: big-power-down {
> > + compatible = "arm,idle-state";
> > + idle-state-name = "big-power-down";
> > + arm,psci-suspend-param = <0x00000002>;
> > + entry-latency-us = <41>;
> > + exit-latency-us = <41>;
> > + min-residency-us = <200>;
> > + local-timer-stop;
> > + };
> > + };
>
> What is the simplest way to test this patch?

You should be able to see state transitions in /sys/devices/cpu/cpu?/cpuidle/*/*

$ grep "" /sys/devices/cpu/cpu?/cpuidle/*/*

And if you have an instrumented board with power rails exposed, you
could measure the cpu rails with and without some load on the CPUs.
That'd help us tune the values too, in the future.

Regards,
Amit

2019-05-10 15:14:35

by Marc Gonzalez

[permalink] [raw]

Subject: Re: [PATCHv1 7/8] arm64: dts: qcom: msm8998: Add PSCI cpuidle low power states

On 10/05/2019 16:12, Amit Kucheria wrote:

> On Fri, May 10, 2019 at 6:45 PM Marc Gonzalez wrote:
>>
>> On 10/05/2019 13:29, Amit Kucheria wrote:
>>
>>> Add device bindings for cpuidle states for cpu devices.
>>>
>>> Cc: Marc Gonzalez <[email protected]>
>>> Signed-off-by: Amit Kucheria <[email protected]>
>>> ---
>>> arch/arm64/boot/dts/qcom/msm8998.dtsi | 32 +++++++++++++++++++++++++++
>>> 1 file changed, 32 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
>>> index 3fd0769fe648..208281f318e2 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_PD>;
>>
>> For some reason, I was expecting the big cores to come first, but according
>> to /proc/cpuinfo, cores 0-3 are part 0x801, while cores 4-7 are part 0x800.
>>
>> According to https://github.com/pytorch/cpuinfo/blob/master/src/arm/uarch.c
>>
>> 0x801 = Low-power Kryo 260 / 280 "Silver" -> Cortex-A53
>> 0x800 = High-performance Kryo 260 (r10p2) / Kryo 280 (r10p1) "Gold" -> Cortex-A73
>
> Hmm, did I mess up the order of the big and LITTLE cores?
> I'll take a look again.

Sorry for being unclear. I was saying I expected the opposite,
but it appears the order in your patch is correct ;-)

Little cores have higher latency (+5%) than big cores?

Regards.

2019-05-13 15:16:31

[permalink] [raw]

Subject: Re: [PATCHv1 7/8] arm64: dts: qcom: msm8998: Add PSCI cpuidle low power states

On Fri, May 10, 2019 at 8:41 PM Marc Gonzalez <[email protected]> wrote:
>
> On 10/05/2019 16:12, Amit Kucheria wrote:
>
> > On Fri, May 10, 2019 at 6:45 PM Marc Gonzalez wrote:
> >>
> >> On 10/05/2019 13:29, Amit Kucheria wrote:
> >>
> >>> Add device bindings for cpuidle states for cpu devices.
> >>>
> >>> Cc: Marc Gonzalez <[email protected]>
> >>> Signed-off-by: Amit Kucheria <[email protected]>
> >>> ---
> >>> arch/arm64/boot/dts/qcom/msm8998.dtsi | 32 +++++++++++++++++++++++++++
> >>> 1 file changed, 32 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> >>> index 3fd0769fe648..208281f318e2 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_PD>;
> >>
> >> For some reason, I was expecting the big cores to come first, but according
> >> to /proc/cpuinfo, cores 0-3 are part 0x801, while cores 4-7 are part 0x800.
> >>
> >> According to https://github.com/pytorch/cpuinfo/blob/master/src/arm/uarch.c
> >>
> >> 0x801 = Low-power Kryo 260 / 280 "Silver" -> Cortex-A53
> >> 0x800 = High-performance Kryo 260 (r10p2) / Kryo 280 (r10p1) "Gold" -> Cortex-A73
> >
> > Hmm, did I mess up the order of the big and LITTLE cores?
> > I'll take a look again.
>
> Sorry for being unclear. I was saying I expected the opposite,
> but it appears the order in your patch is correct ;-)

OK :-)

> Little cores have higher latency (+5%) than big cores?

No, that is a result of me naively converting the downstream numbers
into cpuidle parameters for upstream. There is scope for tuning those
numbers with more instrumentation. My hope is that we will attract
more contributions once the basic idle states have landed upstream
i.e. change the story from "cpuidle isn't supported in upstream QC
platforms" to "cpuidle needs some tuning"

Regards,
Amit

2019-05-13 19:51:26

by Bjorn Andersson

[permalink] [raw]

Subject: Re: [PATCHv1 1/8] arm64: dts: Fix various entry-method properties to reflect documentation

On Fri 10 May 04:29 PDT 2019, Amit Kucheria wrote:

Subject indicates pluralism, but this fixes a specific platform
(board?). I think you should update that.

> The idle-states binding documentation[1] mentions that the
> 'entry-method' property is required on 64-bit platforms and must be set
> to "psci".
>
> We fixed up all uses of the entry-method property in
> commit e9880240e4f4 ("arm64: dts: Fix various entry-method properties to
> reflect documentation"). But a new one has appeared. Fix it up.
>
> Cc: Sudeep Holla <[email protected]>

The message looks good though, so with a new subject you have my:

Reviewed-by: Bjorn Andersson <[email protected]>

Regards,
Bjorn

> Signed-off-by: Amit Kucheria <[email protected]>
> ---
> arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> index 2896bbcfa3bb..42e7822a0227 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> @@ -51,7 +51,7 @@
> * PSCI node is not added default, U-boot will add missing
> * parts if it determines to use PSCI.
> */
> - entry-method = "arm,psci";
> + entry-method = "psci";
>
> CPU_PH20: cpu-ph20 {
> compatible = "arm,idle-state";
> --
> 2.17.1
>

2019-05-14 05:59:23

[permalink] [raw]

Subject: Re: [PATCHv1 1/8] arm64: dts: Fix various entry-method properties to reflect documentation

On Tue, May 14, 2019 at 12:09 AM Bjorn Andersson
<[email protected]> wrote:
>
> On Fri 10 May 04:29 PDT 2019, Amit Kucheria wrote:
>
> Subject indicates pluralism, but this fixes a specific platform
> (board?). I think you should update that.

Copy paste from the previous cleanup commit :-) Will fix.

> > The idle-states binding documentation[1] mentions that the
> > 'entry-method' property is required on 64-bit platforms and must be set
> > to "psci".
> >
> > We fixed up all uses of the entry-method property in
> > commit e9880240e4f4 ("arm64: dts: Fix various entry-method properties to
> > reflect documentation"). But a new one has appeared. Fix it up.
> >
> > Cc: Sudeep Holla <[email protected]>
>
> The message looks good though, so with a new subject you have my:
>
> Reviewed-by: Bjorn Andersson <[email protected]>

Thanks

>
> > Signed-off-by: Amit Kucheria <[email protected]>
> > ---
> > arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> > index 2896bbcfa3bb..42e7822a0227 100644
> > --- a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> > @@ -51,7 +51,7 @@
> > * PSCI node is not added default, U-boot will add missing
> > * parts if it determines to use PSCI.
> > */
> > - entry-method = "arm,psci";
> > + entry-method = "psci";
> >
> > CPU_PH20: cpu-ph20 {
> > compatible = "arm,idle-state";
> > --
> > 2.17.1
> >

2019-05-14 16:13:55

[permalink] [raw]

Subject: Re: [PATCHv1 0/8] qcom: Add cpuidle to some platforms

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
>

2019-05-14 16:14:12

[permalink] [raw]

Subject: Re: [PATCHv1 1/8] arm64: dts: Fix various entry-method properties to reflect documentation

On Fri, May 10, 2019 at 04:59:39PM +0530, Amit Kucheria wrote:
> The idle-states binding documentation[1] mentions that the

This [1] reference is a null pointer ;)

Other than that:
Reviewed-by: Niklas Cassel <[email protected]>

> 'entry-method' property is required on 64-bit platforms and must be set
> to "psci".
>
> We fixed up all uses of the entry-method property in
> commit e9880240e4f4 ("arm64: dts: Fix various entry-method properties to
> reflect documentation"). But a new one has appeared. Fix it up.
>
> Cc: Sudeep Holla <[email protected]>
> Signed-off-by: Amit Kucheria <[email protected]>
> ---
> arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> index 2896bbcfa3bb..42e7822a0227 100644
> --- a/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> +++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> @@ -51,7 +51,7 @@
> * PSCI node is not added default, U-boot will add missing
> * parts if it determines to use PSCI.
> */
> - entry-method = "arm,psci";
> + entry-method = "psci";
>
> CPU_PH20: cpu-ph20 {
> compatible = "arm,idle-state";
> --
> 2.17.1
>

2019-05-14 16:14:13

[permalink] [raw]

Subject: Re: [PATCHv1 3/8] arm64: dts: qcom: msm8916: Add entry-method property for the idle-states node

On Fri, May 10, 2019 at 04:59:41PM +0530, 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 <[email protected]>
> ---
> 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";
> +

Please add a space before and after "=".

With that:
Reviewed-by: Niklas Cassel <[email protected]>

> CPU_SPC: spc {
> compatible = "arm,idle-state";
> arm,psci-suspend-param = <0x40000002>;
> --
> 2.17.1
>

2019-05-14 16:14:27

[permalink] [raw]

Subject: Re: [PATCHv1 5/8] arm64: dts: qcom: qcs404: Add PSCI cpuidle low power states

On Fri, May 10, 2019 at 04:59:43PM +0530, Amit Kucheria wrote:
> From: Niklas Cassel <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Vinod Koul <[email protected]>
> Signed-off-by: Amit Kucheria <[email protected]>
> ---
> 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
>

2019-05-14 16:14:41

[permalink] [raw]

Subject: Re: [PATCHv1 6/8] arm64: dts: qcom: msm8996: Add PSCI cpuidle low power states

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 <[email protected]>
> ---
> 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
>

2019-05-14 16:15:34

[permalink] [raw]

Subject: Re: [PATCHv1 4/8] arm64: dts: qcom: msm8916: Use more generic idle state names

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 <[email protected]>
> ---
> 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
>

2019-05-14 16:16:14

[permalink] [raw]

Subject: Re: [PATCHv1 7/8] arm64: dts: qcom: msm8998: Add PSCI cpuidle low power states

On Fri, May 10, 2019 at 04:59:45PM +0530, Amit Kucheria wrote:
> Add device bindings for cpuidle states for cpu devices.
>
> Cc: Marc Gonzalez <[email protected]>
> Signed-off-by: Amit Kucheria <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/msm8998.dtsi | 32 +++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> index 3fd0769fe648..208281f318e2 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_PD>;
> 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_PD>;
> 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_PD>;
> 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_PD>;
> 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_PD>;
> 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_PD>;
> 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_PD>;
> 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_PD>;
> efficiency = <1536>;
> next-level-cache = <&L2_1>;
> L1_I_103: l1-icache {
> @@ -238,6 +246,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 = "little-power-down";

Since all other idle-state-name in this series uses the qualcomm
terminology for idle states, I think this should as well.

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

PSCI suspend param 0x2 is actually "retention":
https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8998-pm.dtsi?h=msm-4.4#n155

So it actually feels incorrect to call this "power-down".

All other patches in this series has added support for standalone power
collapse, so why not add support for SPC rather than retention?

(For SPC arm,psci-suspend-param should be <0x40000003> .)

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

Shouldn't the latency be <86> ?
https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8998-pm.dtsi?h=msm-4.4#n157
AFAICT downstream assigns the exit_latency to what is parses from "qcom,latency-us":
https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/drivers/cpuidle/lpm-levels.c?h=msm-4.4#n1712

> + min-residency-us = <200>;
> + 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/msm8998-pm.dtsi?h=msm-4.4#n153

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 = "big-power-down";
> + arm,psci-suspend-param = <0x00000002>;
> + entry-latency-us = <41>;
> + exit-latency-us = <41>;
> + min-residency-us = <200>;
> + local-timer-stop;
> + };
> + };
> };
>
> firmware {
> --
> 2.17.1
>

2019-05-14 16:16:28

[permalink] [raw]

Subject: Re: [PATCHv1 8/8] arm64: dts: qcom: sdm845: Add PSCI cpuidle low power states

On Fri, May 10, 2019 at 04:59:46PM +0530, Amit Kucheria wrote:
> From: "Raju P.L.S.S.S.N" <[email protected]>
>
> Add device bindings for cpuidle states for cpu devices.
>
> [amit: rename the idle-states to more generic names and fixups]
>
> Cc: <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Raju P.L.S.S.S.N <[email protected]>
> Reviewed-by: Evan Green <[email protected]>
> Signed-off-by: Amit Kucheria <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/sdm845.dtsi | 62 ++++++++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 5308f1671824..2c8c54e4bd77 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -119,6 +119,7 @@
> compatible = "qcom,kryo385";
> reg = <0x0 0x0>;
> enable-method = "psci";
> + cpu-idle-states = <&LITTLE_CPU_PD &LITTLE_CPU_RPD &CLUSTER_PD>;
> qcom,freq-domain = <&cpufreq_hw 0>;
> #cooling-cells = <2>;
> next-level-cache = <&L2_0>;
> @@ -136,6 +137,7 @@
> compatible = "qcom,kryo385";
> reg = <0x0 0x100>;
> enable-method = "psci";
> + cpu-idle-states = <&LITTLE_CPU_PD &LITTLE_CPU_RPD &CLUSTER_PD>;
> qcom,freq-domain = <&cpufreq_hw 0>;
> #cooling-cells = <2>;
> next-level-cache = <&L2_100>;
> @@ -150,6 +152,7 @@
> compatible = "qcom,kryo385";
> reg = <0x0 0x200>;
> enable-method = "psci";
> + cpu-idle-states = <&LITTLE_CPU_PD &LITTLE_CPU_RPD &CLUSTER_PD>;
> qcom,freq-domain = <&cpufreq_hw 0>;
> #cooling-cells = <2>;
> next-level-cache = <&L2_200>;
> @@ -164,6 +167,7 @@
> compatible = "qcom,kryo385";
> reg = <0x0 0x300>;
> enable-method = "psci";
> + cpu-idle-states = <&LITTLE_CPU_PD &LITTLE_CPU_RPD &CLUSTER_PD>;
> qcom,freq-domain = <&cpufreq_hw 0>;
> #cooling-cells = <2>;
> next-level-cache = <&L2_300>;
> @@ -178,6 +182,7 @@
> compatible = "qcom,kryo385";
> reg = <0x0 0x400>;
> enable-method = "psci";
> + cpu-idle-states = <&BIG_CPU_PD &BIG_CPU_RPD &CLUSTER_PD>;
> qcom,freq-domain = <&cpufreq_hw 1>;
> #cooling-cells = <2>;
> next-level-cache = <&L2_400>;
> @@ -192,6 +197,7 @@
> compatible = "qcom,kryo385";
> reg = <0x0 0x500>;
> enable-method = "psci";
> + cpu-idle-states = <&BIG_CPU_PD &BIG_CPU_RPD &CLUSTER_PD>;
> qcom,freq-domain = <&cpufreq_hw 1>;
> #cooling-cells = <2>;
> next-level-cache = <&L2_500>;
> @@ -206,6 +212,7 @@
> compatible = "qcom,kryo385";
> reg = <0x0 0x600>;
> enable-method = "psci";
> + cpu-idle-states = <&BIG_CPU_PD &BIG_CPU_RPD &CLUSTER_PD>;
> qcom,freq-domain = <&cpufreq_hw 1>;
> #cooling-cells = <2>;
> next-level-cache = <&L2_600>;
> @@ -220,6 +227,7 @@
> compatible = "qcom,kryo385";
> reg = <0x0 0x700>;
> enable-method = "psci";
> + cpu-idle-states = <&BIG_CPU_PD &BIG_CPU_RPD &CLUSTER_PD>;
> qcom,freq-domain = <&cpufreq_hw 1>;
> #cooling-cells = <2>;
> next-level-cache = <&L2_700>;
> @@ -228,6 +236,60 @@
> next-level-cache = <&L3_0>;
> };
> };
> +
> + idle-states {
> + entry-method = "psci";
> +
> + LITTLE_CPU_PD: little-power-down {
> + compatible = "arm,idle-state";
> + idle-state-name = "little-power-down";
> + arm,psci-suspend-param = <0x40000003>;
> + entry-latency-us = <350>;
> + exit-latency-us = <461>;
> + min-residency-us = <1890>;
> + local-timer-stop;
> + };
> +
> + LITTLE_CPU_RPD: little-rail-power-down {
> + compatible = "arm,idle-state";
> + idle-state-name = "little-rail-power-down";
> + arm,psci-suspend-param = <0x40000004>;
> + entry-latency-us = <360>;
> + exit-latency-us = <531>;
> + min-residency-us = <3934>;
> + local-timer-stop;
> + };
> +
> + BIG_CPU_PD: big-power-down {
> + compatible = "arm,idle-state";
> + idle-state-name = "big-power-down";
> + arm,psci-suspend-param = <0x40000003>;
> + entry-latency-us = <264>;
> + exit-latency-us = <621>;
> + min-residency-us = <952>;
> + local-timer-stop;
> + };
> +
> + BIG_CPU_RPD: big-rail-power-down {
> + compatible = "arm,idle-state";
> + idle-state-name = "big-rail-power-down";
> + arm,psci-suspend-param = <0x40000004>;
> + entry-latency-us = <702>;
> + exit-latency-us = <1061>;
> + min-residency-us = <4488>;
> + local-timer-stop;
> + };
> +
> + CLUSTER_PD: cluster-power-down {
> + compatible = "arm,idle-state";
> + idle-state-name = "cluster-power-down";
> + arm,psci-suspend-param = <0x400000F4>;
> + entry-latency-us = <3263>;
> + exit-latency-us = <6562>;
> + min-residency-us = <9987>;
> + local-timer-stop;

I'm surprised that this power state is not defined in downstream node qcom,pm-cluster@0
https://source.codeaurora.org/quic/la/kernel/msm-4.9/tree/arch/arm64/boot/dts/qcom/sdm845-pm.dtsi?h=msm-4.9

Also note that Ulf and Lina's cluster idling patch series:
https://patchwork.kernel.org/project/linux-arm-msm/list/?series=117055
hasn't been merged yet.

Is it really safe to add a cluster power state without this series?

If the firmware fails to enter this cluster power state, won't the
cpuidle governor continue to try to enter this power state?
Thus basically disabling cpuidle, since the governor will never try
to enter the per cpu power states?

It would be interesting with statistics of how many times we've tried
to enter this power state, together with how many times entering this
power state has failed. If this percentage is very high, then we probably
need the cluster idling patch series before enabling this power state.

> + };
> + };
> };
>
> pmu {
> --
> 2.17.1
>

2019-05-15 10:15:51

[permalink] [raw]

Subject: Re: [PATCHv1 4/8] arm64: dts: qcom: msm8916: Use more generic idle state names

On Tue, May 14, 2019 at 9:42 PM Niklas Cassel <[email protected]> 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 <[email protected]>
> > ---
> > 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
> >

2019-05-15 14:13:12

[permalink] [raw]

Subject: Re: [PATCHv1 4/8] arm64: dts: qcom: msm8916: Use more generic idle state names

On Wed, May 15, 2019 at 03:43:19PM +0530, Amit Kucheria wrote:
> On Tue, May 14, 2019 at 9:42 PM Niklas Cassel <[email protected]> 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 <[email protected]>
> > > ---
> > > 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

>
> > > 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
> > >

2019-05-17 10:22:17

[permalink] [raw]

Subject: Re: [PATCHv1 6/8] arm64: dts: qcom: msm8996: Add PSCI cpuidle low power states

On Tue, May 14, 2019 at 9:42 PM Niklas Cassel <[email protected]> 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 <[email protected]>
> > ---
> > 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
> >

2019-05-17 16:18:56

[permalink] [raw]

Subject: Re: [PATCHv1 6/8] arm64: dts: qcom: msm8996: Add PSCI cpuidle low power states

On 10/05/2019 13:29, Amit Kucheria wrote:
> Add device bindings for cpuidle states for cpu devices.
>
> Signed-off-by: Amit Kucheria <[email protected]>
> ---
> 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 {
>

--
<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

2019-05-17 16:27:45

[permalink] [raw]

Subject: Re: [PATCHv1 8/8] arm64: dts: qcom: sdm845: Add PSCI cpuidle low power states

On 10/05/2019 13:29, Amit Kucheria wrote:
> From: "Raju P.L.S.S.S.N" <[email protected]>
>
> Add device bindings for cpuidle states for cpu devices.
>
> [amit: rename the idle-states to more generic names and fixups]
>
> Cc: <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Raju P.L.S.S.S.N <[email protected]>
> Reviewed-by: Evan Green <[email protected]>
> Signed-off-by: Amit Kucheria <[email protected]>

Acked-by: Daniel Lezcano <[email protected]>

--
<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

2019-05-17 16:33:53

[permalink] [raw]

Subject: Re: [PATCHv1 5/8] arm64: dts: qcom: qcs404: Add PSCI cpuidle low power states

On 10/05/2019 13:29, Amit Kucheria wrote:
> From: Niklas Cassel <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Vinod Koul <[email protected]>
> Signed-off-by: Amit Kucheria <[email protected]>

Acked-by: Daniel Lezcano <[email protected]>

> ---
> 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";
> +
> + 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 {
>

--
<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

2019-05-17 16:53:58

[permalink] [raw]

Subject: Re: [PATCHv1 4/8] arm64: dts: qcom: msm8916: Use more generic idle state names

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 <[email protected]>

Acked-by: Daniel Lezcano <[email protected]>

> ---
> 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

2019-05-17 16:54:15

[permalink] [raw]

Subject: Re: [PATCHv1 3/8] arm64: dts: qcom: msm8916: Add entry-method property for the idle-states node

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 <[email protected]>

Acked-by: Daniel Lezcano <[email protected]>

> ---
> 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

2019-05-17 16:57:21

[permalink] [raw]

Subject: Re: [PATCHv1 7/8] arm64: dts: qcom: msm8998: Add PSCI cpuidle low power states

On 10/05/2019 13:29, Amit Kucheria wrote:
> Add device bindings for cpuidle states for cpu devices.
>
> Cc: Marc Gonzalez <[email protected]>
> Signed-off-by: Amit Kucheria <[email protected]>

Acked-by: Daniel Lezcano <[email protected]>

> ---
> arch/arm64/boot/dts/qcom/msm8998.dtsi | 32 +++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/msm8998.dtsi b/arch/arm64/boot/dts/qcom/msm8998.dtsi
> index 3fd0769fe648..208281f318e2 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_PD>;
> 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_PD>;
> 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_PD>;
> 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_PD>;
> 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_PD>;
> 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_PD>;
> 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_PD>;
> 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_PD>;
> efficiency = <1536>;
> next-level-cache = <&L2_1>;
> L1_I_103: l1-icache {
> @@ -238,6 +246,30 @@
> };
> };
> };
> +
> + idle-states {
> + entry-method="psci";
> +
> + LITTLE_CPU_PD: little-power-down {
> + compatible = "arm,idle-state";
> + idle-state-name = "little-power-down";
> + arm,psci-suspend-param = <0x00000002>;
> + entry-latency-us = <43>;
> + exit-latency-us = <43>;
> + min-residency-us = <200>;
> + local-timer-stop;
> + };
> +
> + BIG_CPU_PD: big-power-down {
> + compatible = "arm,idle-state";
> + idle-state-name = "big-power-down";
> + arm,psci-suspend-param = <0x00000002>;
> + entry-latency-us = <41>;
> + exit-latency-us = <41>;
> + min-residency-us = <200>;
> + local-timer-stop;
> + };
> + };
> };
>
> firmware {
>

--
<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

2019-05-21 05:40:40

[permalink] [raw]

Subject: Re: [PATCHv1 4/8] arm64: dts: qcom: msm8916: Use more generic idle state names

On Wed, May 15, 2019 at 6:33 PM Niklas Cassel <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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?

2019-05-21 08:52:22