* [PATCHv1 0/8] qcom: Add cpuidle to some platforms
@ 2019-05-10 11:29 Amit Kucheria
2019-05-10 11:29 ` [PATCHv1 1/8] arm64: dts: Fix various entry-method properties to reflect documentation Amit Kucheria
` (8 more replies)
0 siblings, 9 replies; 36+ messages in thread
From: Amit Kucheria @ 2019-05-10 11:29 UTC (permalink / raw)
To: linux-kernel, linux-arm-msm, bjorn.andersson, andy.gross,
David Brown, Li Yang, Shawn Guo
Cc: devicetree, linux-arm-kernel
Fix up a few entry-method="psci" issues and then add cpuidle low power
states for msm8996, msm8998, qcs404, sdm845. All these have been tested
to only make sure that the C-states are entered from Linux point-of-view.
We will continue to add more states and make power measurements to tweak
some of these numbers, but getting these merged will allow other people to
use these platforms to work on cpuidle, eas and related topics.
Amit Kucheria (6):
arm64: dts: Fix various entry-method properties to reflect
documentation
Documentation: arm: Link idle-states binding to code
arm64: dts: qcom: msm8916: Add entry-method property for the
idle-states node
arm64: dts: qcom: msm8916: Use more generic idle state names
arm64: dts: qcom: msm8996: Add PSCI cpuidle low power states
arm64: dts: qcom: msm8998: Add PSCI cpuidle low power states
Niklas Cassel (1):
arm64: dts: qcom: qcs404: Add PSCI cpuidle low power states
Raju P.L.S.S.S.N (1):
arm64: dts: qcom: sdm845: Add PSCI cpuidle low power states
.../devicetree/bindings/arm/idle-states.txt | 7 +++
.../arm64/boot/dts/freescale/fsl-ls1028a.dtsi | 2 +-
arch/arm64/boot/dts/qcom/msm8916.dtsi | 13 ++--
arch/arm64/boot/dts/qcom/msm8996.dtsi | 28 +++++++++
arch/arm64/boot/dts/qcom/msm8998.dtsi | 32 ++++++++++
arch/arm64/boot/dts/qcom/qcs404.dtsi | 18 ++++++
arch/arm64/boot/dts/qcom/sdm845.dtsi | 62 +++++++++++++++++++
7 files changed, 156 insertions(+), 6 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCHv1 1/8] arm64: dts: Fix various entry-method properties to reflect documentation
2019-05-10 11:29 [PATCHv1 0/8] qcom: Add cpuidle to some platforms Amit Kucheria
@ 2019-05-10 11:29 ` Amit Kucheria
2019-05-10 12:54 ` Sudeep Holla
` (2 more replies)
2019-05-10 11:29 ` [PATCHv1 2/8] Documentation: arm: Link idle-states binding to code Amit Kucheria
` (7 subsequent siblings)
8 siblings, 3 replies; 36+ messages in thread
From: Amit Kucheria @ 2019-05-10 11:29 UTC (permalink / raw)
To: linux-kernel, linux-arm-msm, bjorn.andersson, andy.gross,
David Brown, Li Yang, Shawn Guo
Cc: Sudeep Holla, linux-arm-kernel, devicetree
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 <sudeep.holla@arm.com>
Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCHv1 2/8] Documentation: arm: Link idle-states binding to code
2019-05-10 11:29 [PATCHv1 0/8] qcom: Add cpuidle to some platforms Amit Kucheria
2019-05-10 11:29 ` [PATCHv1 1/8] arm64: dts: Fix various entry-method properties to reflect documentation Amit Kucheria
@ 2019-05-10 11:29 ` Amit Kucheria
2019-05-10 13:02 ` Sudeep Holla
2019-05-10 11:29 ` [PATCHv1 3/8] arm64: dts: qcom: msm8916: Add entry-method property for the idle-states node Amit Kucheria
` (6 subsequent siblings)
8 siblings, 1 reply; 36+ messages in thread
From: Amit Kucheria @ 2019-05-10 11:29 UTC (permalink / raw)
To: linux-kernel, linux-arm-msm, bjorn.andersson, andy.gross,
David Brown, Li Yang, Shawn Guo
Cc: Sudeep Holla, devicetree
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 <sudeep.holla@arm.com>
Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCHv1 3/8] arm64: dts: qcom: msm8916: Add entry-method property for the idle-states node
2019-05-10 11:29 [PATCHv1 0/8] qcom: Add cpuidle to some platforms Amit Kucheria
2019-05-10 11:29 ` [PATCHv1 1/8] arm64: dts: Fix various entry-method properties to reflect documentation Amit Kucheria
2019-05-10 11:29 ` [PATCHv1 2/8] Documentation: arm: Link idle-states binding to code Amit Kucheria
@ 2019-05-10 11:29 ` Amit Kucheria
2019-05-14 16:12 ` Niklas Cassel
2019-05-17 15:40 ` Daniel Lezcano
2019-05-10 11:29 ` [PATCHv1 4/8] arm64: dts: qcom: msm8916: Use more generic idle state names Amit Kucheria
` (5 subsequent siblings)
8 siblings, 2 replies; 36+ messages in thread
From: Amit Kucheria @ 2019-05-10 11:29 UTC (permalink / raw)
To: linux-kernel, linux-arm-msm, bjorn.andersson, andy.gross,
David Brown, Li Yang, Shawn Guo
Cc: devicetree
The idle-states binding documentation[1] mentions that the
'entry-method' property is required on 64-bit platforms and must be set
to "psci".
Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCHv1 4/8] arm64: dts: qcom: msm8916: Use more generic idle state names
2019-05-10 11:29 [PATCHv1 0/8] qcom: Add cpuidle to some platforms Amit Kucheria
` (2 preceding siblings ...)
2019-05-10 11:29 ` [PATCHv1 3/8] arm64: dts: qcom: msm8916: Add entry-method property for the idle-states node Amit Kucheria
@ 2019-05-10 11:29 ` Amit Kucheria
2019-05-14 16:12 ` Niklas Cassel
2019-05-17 15:41 ` Daniel Lezcano
2019-05-10 11:29 ` [PATCHv1 5/8] arm64: dts: qcom: qcs404: Add PSCI cpuidle low power states Amit Kucheria
` (4 subsequent siblings)
8 siblings, 2 replies; 36+ messages in thread
From: Amit Kucheria @ 2019-05-10 11:29 UTC (permalink / raw)
To: linux-kernel, linux-arm-msm, bjorn.andersson, andy.gross,
David Brown, Li Yang, Shawn Guo
Cc: devicetree
Instead of using Qualcomm-specific terminology, use generic node names
for the idle states that are easier to understand. Move the description
into the "idle-state-name" property.
Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
arch/arm64/boot/dts/qcom/msm8916.dtsi | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index ded1052e5693..400b609bb3fd 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -110,7 +110,7 @@
reg = <0x0>;
next-level-cache = <&L2_0>;
enable-method = "psci";
- cpu-idle-states = <&CPU_SPC>;
+ cpu-idle-states = <&CPU_SLEEP_0>;
clocks = <&apcs>;
operating-points-v2 = <&cpu_opp_table>;
#cooling-cells = <2>;
@@ -122,7 +122,7 @@
reg = <0x1>;
next-level-cache = <&L2_0>;
enable-method = "psci";
- cpu-idle-states = <&CPU_SPC>;
+ cpu-idle-states = <&CPU_SLEEP_0>;
clocks = <&apcs>;
operating-points-v2 = <&cpu_opp_table>;
#cooling-cells = <2>;
@@ -134,7 +134,7 @@
reg = <0x2>;
next-level-cache = <&L2_0>;
enable-method = "psci";
- cpu-idle-states = <&CPU_SPC>;
+ cpu-idle-states = <&CPU_SLEEP_0>;
clocks = <&apcs>;
operating-points-v2 = <&cpu_opp_table>;
#cooling-cells = <2>;
@@ -146,7 +146,7 @@
reg = <0x3>;
next-level-cache = <&L2_0>;
enable-method = "psci";
- cpu-idle-states = <&CPU_SPC>;
+ cpu-idle-states = <&CPU_SLEEP_0>;
clocks = <&apcs>;
operating-points-v2 = <&cpu_opp_table>;
#cooling-cells = <2>;
@@ -160,8 +160,9 @@
idle-states {
entry-method="psci";
- 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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCHv1 5/8] arm64: dts: qcom: qcs404: Add PSCI cpuidle low power states
2019-05-10 11:29 [PATCHv1 0/8] qcom: Add cpuidle to some platforms Amit Kucheria
` (3 preceding siblings ...)
2019-05-10 11:29 ` [PATCHv1 4/8] arm64: dts: qcom: msm8916: Use more generic idle state names Amit Kucheria
@ 2019-05-10 11:29 ` Amit Kucheria
2019-05-14 16:12 ` Niklas Cassel
2019-05-17 15:42 ` Daniel Lezcano
2019-05-10 11:29 ` [PATCHv1 6/8] arm64: dts: qcom: msm8996: " Amit Kucheria
` (3 subsequent siblings)
8 siblings, 2 replies; 36+ messages in thread
From: Amit Kucheria @ 2019-05-10 11:29 UTC (permalink / raw)
To: linux-kernel, linux-arm-msm, bjorn.andersson, andy.gross,
David Brown, Li Yang, Shawn Guo
Cc: Niklas Cassel, devicetree
From: Niklas Cassel <niklas.cassel@linaro.org>
Add device bindings for cpuidle states for cpu devices.
[amit: rename the idle-states to more generic names and fixups]
Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
Reviewed-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
arch/arm64/boot/dts/qcom/qcs404.dtsi | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
index e8fd26633d57..369c59c35bc7 100644
--- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
+++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
@@ -30,6 +30,7 @@
compatible = "arm,cortex-a53";
reg = <0x100>;
enable-method = "psci";
+ cpu-idle-states = <&CPU_SLEEP_0>;
next-level-cache = <&L2_0>;
};
@@ -38,6 +39,7 @@
compatible = "arm,cortex-a53";
reg = <0x101>;
enable-method = "psci";
+ cpu-idle-states = <&CPU_SLEEP_0>;
next-level-cache = <&L2_0>;
};
@@ -46,6 +48,7 @@
compatible = "arm,cortex-a53";
reg = <0x102>;
enable-method = "psci";
+ cpu-idle-states = <&CPU_SLEEP_0>;
next-level-cache = <&L2_0>;
};
@@ -54,6 +57,7 @@
compatible = "arm,cortex-a53";
reg = <0x103>;
enable-method = "psci";
+ cpu-idle-states = <&CPU_SLEEP_0>;
next-level-cache = <&L2_0>;
};
@@ -61,6 +65,20 @@
compatible = "cache";
cache-level = <2>;
};
+
+ idle-states {
+ entry-method="psci";
+
+ 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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCHv1 6/8] arm64: dts: qcom: msm8996: Add PSCI cpuidle low power states
2019-05-10 11:29 [PATCHv1 0/8] qcom: Add cpuidle to some platforms Amit Kucheria
` (4 preceding siblings ...)
2019-05-10 11:29 ` [PATCHv1 5/8] arm64: dts: qcom: qcs404: Add PSCI cpuidle low power states Amit Kucheria
@ 2019-05-10 11:29 ` Amit Kucheria
2019-05-14 16:12 ` Niklas Cassel
2019-05-17 15:55 ` Daniel Lezcano
2019-05-10 11:29 ` [PATCHv1 7/8] arm64: dts: qcom: msm8998: " Amit Kucheria
` (2 subsequent siblings)
8 siblings, 2 replies; 36+ messages in thread
From: Amit Kucheria @ 2019-05-10 11:29 UTC (permalink / raw)
To: linux-kernel, linux-arm-msm, bjorn.andersson, andy.gross,
David Brown, Li Yang, Shawn Guo
Cc: devicetree
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";
+
+ 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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCHv1 7/8] arm64: dts: qcom: msm8998: Add PSCI cpuidle low power states
2019-05-10 11:29 [PATCHv1 0/8] qcom: Add cpuidle to some platforms Amit Kucheria
` (5 preceding siblings ...)
2019-05-10 11:29 ` [PATCHv1 6/8] arm64: dts: qcom: msm8996: " Amit Kucheria
@ 2019-05-10 11:29 ` Amit Kucheria
2019-05-10 13:15 ` Marc Gonzalez
` (2 more replies)
2019-05-10 11:29 ` [PATCHv1 8/8] arm64: dts: qcom: sdm845: " Amit Kucheria
2019-05-14 16:11 ` [PATCHv1 0/8] qcom: Add cpuidle to some platforms Niklas Cassel
8 siblings, 3 replies; 36+ messages in thread
From: Amit Kucheria @ 2019-05-10 11:29 UTC (permalink / raw)
To: linux-kernel, linux-arm-msm, bjorn.andersson, andy.gross,
David Brown, Li Yang, Shawn Guo
Cc: Marc Gonzalez, devicetree
Add device bindings for cpuidle states for cpu devices.
Cc: Marc Gonzalez <marc.w.gonzalez@free.fr>
Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCHv1 8/8] arm64: dts: qcom: sdm845: Add PSCI cpuidle low power states
2019-05-10 11:29 [PATCHv1 0/8] qcom: Add cpuidle to some platforms Amit Kucheria
` (6 preceding siblings ...)
2019-05-10 11:29 ` [PATCHv1 7/8] arm64: dts: qcom: msm8998: " Amit Kucheria
@ 2019-05-10 11:29 ` Amit Kucheria
2019-05-14 16:13 ` Niklas Cassel
2019-05-17 16:25 ` Daniel Lezcano
2019-05-14 16:11 ` [PATCHv1 0/8] qcom: Add cpuidle to some platforms Niklas Cassel
8 siblings, 2 replies; 36+ messages in thread
From: Amit Kucheria @ 2019-05-10 11:29 UTC (permalink / raw)
To: linux-kernel, linux-arm-msm, bjorn.andersson, andy.gross,
David Brown, Li Yang, Shawn Guo
Cc: Raju P.L.S.S.S.N, devicetree, mkshah
From: "Raju P.L.S.S.S.N" <rplsssn@codeaurora.org>
Add device bindings for cpuidle states for cpu devices.
[amit: rename the idle-states to more generic names and fixups]
Cc: <devicetree@vger.kernel.org>
Cc: <mkshah@codeaurora.org>
Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
Reviewed-by: Evan Green <evgreen@chromium.org>
Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
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
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCHv1 1/8] arm64: dts: Fix various entry-method properties to reflect documentation
2019-05-10 11:29 ` [PATCHv1 1/8] arm64: dts: Fix various entry-method properties to reflect documentation Amit Kucheria
@ 2019-05-10 12:54 ` Sudeep Holla
2019-05-13 18:39 ` Bjorn Andersson
2019-05-14 16:11 ` Niklas Cassel
2 siblings, 0 replies; 36+ messages in thread
From: Sudeep Holla @ 2019-05-10 12:54 UTC (permalink / raw)
To: Amit Kucheria
Cc: linux-kernel, linux-arm-msm, bjorn.andersson, andy.gross,
David Brown, Li Yang, Shawn Guo, linux-arm-kernel, devicetree
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 <sudeep.holla@arm.com>
Ah right, new ones always appear for short period.
Anyways,
Acked-by: Sudeep Holla <sudeep.holla@arm.com>
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
> 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
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 2/8] Documentation: arm: Link idle-states binding to code
2019-05-10 11:29 ` [PATCHv1 2/8] Documentation: arm: Link idle-states binding to code Amit Kucheria
@ 2019-05-10 13:02 ` Sudeep Holla
0 siblings, 0 replies; 36+ messages in thread
From: Sudeep Holla @ 2019-05-10 13:02 UTC (permalink / raw)
To: Amit Kucheria
Cc: linux-kernel, linux-arm-msm, bjorn.andersson, andy.gross,
David Brown, Li Yang, Shawn Guo, devicetree, Sudeep Holla
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 <sudeep.holla@arm.com>
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
> 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 <sudeep.holla@arm.com
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 7/8] arm64: dts: qcom: msm8998: Add PSCI cpuidle low power states
2019-05-10 11:29 ` [PATCHv1 7/8] arm64: dts: qcom: msm8998: " Amit Kucheria
@ 2019-05-10 13:15 ` Marc Gonzalez
2019-05-10 14:12 ` Amit Kucheria
2019-05-14 16:13 ` Niklas Cassel
2019-05-17 16:11 ` Daniel Lezcano
2 siblings, 1 reply; 36+ messages in thread
From: Marc Gonzalez @ 2019-05-10 13:15 UTC (permalink / raw)
To: Amit Kucheria, Bjorn Andersson; +Cc: MSM, LKML
On 10/05/2019 13:29, Amit Kucheria wrote:
> Add device bindings for cpuidle states for cpu devices.
>
> Cc: Marc Gonzalez <marc.w.gonzalez@free.fr>
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
> 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.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 7/8] arm64: dts: qcom: msm8998: Add PSCI cpuidle low power states
2019-05-10 13:15 ` Marc Gonzalez
@ 2019-05-10 14:12 ` Amit Kucheria
2019-05-10 15:11 ` Marc Gonzalez
0 siblings, 1 reply; 36+ messages in thread
From: Amit Kucheria @ 2019-05-10 14:12 UTC (permalink / raw)
To: Marc Gonzalez; +Cc: Bjorn Andersson, MSM, LKML
On Fri, May 10, 2019 at 6:45 PM Marc Gonzalez <marc.w.gonzalez@free.fr> wrote:
>
> On 10/05/2019 13:29, Amit Kucheria wrote:
>
> > Add device bindings for cpuidle states for cpu devices.
> >
> > Cc: Marc Gonzalez <marc.w.gonzalez@free.fr>
> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> > ---
> > 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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 7/8] arm64: dts: qcom: msm8998: Add PSCI cpuidle low power states
2019-05-10 14:12 ` Amit Kucheria
@ 2019-05-10 15:11 ` Marc Gonzalez
2019-05-13 12:38 ` Amit Kucheria
0 siblings, 1 reply; 36+ messages in thread
From: Marc Gonzalez @ 2019-05-10 15:11 UTC (permalink / raw)
To: Amit Kucheria, Bjorn Andersson; +Cc: MSM, LKML
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 <marc.w.gonzalez@free.fr>
>>> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
>>> ---
>>> 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.
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 7/8] arm64: dts: qcom: msm8998: Add PSCI cpuidle low power states
2019-05-10 15:11 ` Marc Gonzalez
@ 2019-05-13 12:38 ` Amit Kucheria
0 siblings, 0 replies; 36+ messages in thread
From: Amit Kucheria @ 2019-05-13 12:38 UTC (permalink / raw)
To: Marc Gonzalez; +Cc: Bjorn Andersson, MSM, LKML
On Fri, May 10, 2019 at 8:41 PM Marc Gonzalez <marc.w.gonzalez@free.fr> 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 <marc.w.gonzalez@free.fr>
> >>> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> >>> ---
> >>> 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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 1/8] arm64: dts: Fix various entry-method properties to reflect documentation
2019-05-10 11:29 ` [PATCHv1 1/8] arm64: dts: Fix various entry-method properties to reflect documentation Amit Kucheria
2019-05-10 12:54 ` Sudeep Holla
@ 2019-05-13 18:39 ` Bjorn Andersson
2019-05-14 5:57 ` Amit Kucheria
2019-05-14 16:11 ` Niklas Cassel
2 siblings, 1 reply; 36+ messages in thread
From: Bjorn Andersson @ 2019-05-13 18:39 UTC (permalink / raw)
To: Amit Kucheria
Cc: linux-kernel, linux-arm-msm, andy.gross, David Brown, Li Yang,
Shawn Guo, Sudeep Holla, linux-arm-kernel, devicetree
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 <sudeep.holla@arm.com>
The message looks good though, so with a new subject you have my:
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Regards,
Bjorn
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
> 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
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 1/8] arm64: dts: Fix various entry-method properties to reflect documentation
2019-05-13 18:39 ` Bjorn Andersson
@ 2019-05-14 5:57 ` Amit Kucheria
0 siblings, 0 replies; 36+ messages in thread
From: Amit Kucheria @ 2019-05-14 5:57 UTC (permalink / raw)
To: Bjorn Andersson
Cc: LKML, linux-arm-msm, Andy Gross, David Brown, Li Yang, Shawn Guo,
Sudeep Holla, lakml,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
On Tue, May 14, 2019 at 12:09 AM Bjorn Andersson
<bjorn.andersson@linaro.org> 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 <sudeep.holla@arm.com>
>
> The message looks good though, so with a new subject you have my:
>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Thanks
>
> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> > ---
> > 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
> >
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 1/8] arm64: dts: Fix various entry-method properties to reflect documentation
2019-05-10 11:29 ` [PATCHv1 1/8] arm64: dts: Fix various entry-method properties to reflect documentation Amit Kucheria
2019-05-10 12:54 ` Sudeep Holla
2019-05-13 18:39 ` Bjorn Andersson
@ 2019-05-14 16:11 ` Niklas Cassel
2 siblings, 0 replies; 36+ messages in thread
From: Niklas Cassel @ 2019-05-14 16:11 UTC (permalink / raw)
To: Amit Kucheria
Cc: linux-kernel, linux-arm-msm, bjorn.andersson, andy.gross,
David Brown, Li Yang, Shawn Guo, Sudeep Holla, linux-arm-kernel,
devicetree
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 <niklas.cassel@linaro.org>
> '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 <sudeep.holla@arm.com>
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
> 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
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 0/8] qcom: Add cpuidle to some platforms
2019-05-10 11:29 [PATCHv1 0/8] qcom: Add cpuidle to some platforms Amit Kucheria
` (7 preceding siblings ...)
2019-05-10 11:29 ` [PATCHv1 8/8] arm64: dts: qcom: sdm845: " Amit Kucheria
@ 2019-05-14 16:11 ` Niklas Cassel
8 siblings, 0 replies; 36+ messages in thread
From: Niklas Cassel @ 2019-05-14 16:11 UTC (permalink / raw)
To: Amit Kucheria
Cc: Linux Kernel Mailing List, linux-arm-msm, Bjorn Andersson,
Andy Gross, David Brown, Li Yang, Shawn Guo, devicetree,
Linux ARM
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
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 3/8] arm64: dts: qcom: msm8916: Add entry-method property for the idle-states node
2019-05-10 11:29 ` [PATCHv1 3/8] arm64: dts: qcom: msm8916: Add entry-method property for the idle-states node Amit Kucheria
@ 2019-05-14 16:12 ` Niklas Cassel
2019-05-17 15:40 ` Daniel Lezcano
1 sibling, 0 replies; 36+ messages in thread
From: Niklas Cassel @ 2019-05-14 16:12 UTC (permalink / raw)
To: Amit Kucheria
Cc: linux-kernel, linux-arm-msm, bjorn.andersson, andy.gross,
David Brown, Li Yang, Shawn Guo, devicetree
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 <amit.kucheria@linaro.org>
> ---
> arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> index 0803ca8c02da..ded1052e5693 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> @@ -158,6 +158,8 @@
> };
>
> idle-states {
> + entry-method="psci";
> +
Please add a space before and after "=".
With that:
Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
> CPU_SPC: spc {
> compatible = "arm,idle-state";
> arm,psci-suspend-param = <0x40000002>;
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 4/8] arm64: dts: qcom: msm8916: Use more generic idle state names
2019-05-10 11:29 ` [PATCHv1 4/8] arm64: dts: qcom: msm8916: Use more generic idle state names Amit Kucheria
@ 2019-05-14 16:12 ` Niklas Cassel
2019-05-15 10:13 ` Amit Kucheria
2019-05-17 15:41 ` Daniel Lezcano
1 sibling, 1 reply; 36+ messages in thread
From: Niklas Cassel @ 2019-05-14 16:12 UTC (permalink / raw)
To: Amit Kucheria
Cc: linux-kernel, linux-arm-msm, bjorn.andersson, andy.gross,
David Brown, Li Yang, Shawn Guo, devicetree
On Fri, May 10, 2019 at 04:59:42PM +0530, Amit Kucheria wrote:
> Instead of using Qualcomm-specific terminology, use generic node names
> for the idle states that are easier to understand. Move the description
> into the "idle-state-name" property.
>
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
> arch/arm64/boot/dts/qcom/msm8916.dtsi | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> index ded1052e5693..400b609bb3fd 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> @@ -110,7 +110,7 @@
> reg = <0x0>;
> next-level-cache = <&L2_0>;
> enable-method = "psci";
> - cpu-idle-states = <&CPU_SPC>;
> + cpu-idle-states = <&CPU_SLEEP_0>;
> clocks = <&apcs>;
> operating-points-v2 = <&cpu_opp_table>;
> #cooling-cells = <2>;
> @@ -122,7 +122,7 @@
> reg = <0x1>;
> next-level-cache = <&L2_0>;
> enable-method = "psci";
> - cpu-idle-states = <&CPU_SPC>;
> + cpu-idle-states = <&CPU_SLEEP_0>;
> clocks = <&apcs>;
> operating-points-v2 = <&cpu_opp_table>;
> #cooling-cells = <2>;
> @@ -134,7 +134,7 @@
> reg = <0x2>;
> next-level-cache = <&L2_0>;
> enable-method = "psci";
> - cpu-idle-states = <&CPU_SPC>;
> + cpu-idle-states = <&CPU_SLEEP_0>;
> clocks = <&apcs>;
> operating-points-v2 = <&cpu_opp_table>;
> #cooling-cells = <2>;
> @@ -146,7 +146,7 @@
> reg = <0x3>;
> next-level-cache = <&L2_0>;
> enable-method = "psci";
> - cpu-idle-states = <&CPU_SPC>;
> + cpu-idle-states = <&CPU_SLEEP_0>;
> clocks = <&apcs>;
> operating-points-v2 = <&cpu_opp_table>;
> #cooling-cells = <2>;
> @@ -160,8 +160,9 @@
> idle-states {
> entry-method="psci";
Please add a space before and after "=".
>
> - CPU_SPC: spc {
> + CPU_SLEEP_0: cpu-sleep-0 {
While I like your idea of using power state names from
Server Base System Architecture document (SBSA) where applicable,
does each qcom power state have a matching state in SBSA?
These are the qcom power states:
https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/Documentation/devicetree/bindings/arm/msm/lpm-levels.txt?h=msm-4.4#n53
Note that qcom defines:
"wfi", "retention", "gdhs", "pc", "fpc"
while SBSA simply defines "idle_standby" (aka wfi), "idle_retention", "sleep".
Unless you know the equivalent name for each qcom power state
(perhaps several qcom power states are really the same SBSA state?),
I think that you should omit the renaming from this patch series.
> compatible = "arm,idle-state";
> + idle-state-name = "standalone-power-collapse";
> arm,psci-suspend-param = <0x40000002>;
> entry-latency-us = <130>;
> exit-latency-us = <150>;
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 5/8] arm64: dts: qcom: qcs404: Add PSCI cpuidle low power states
2019-05-10 11:29 ` [PATCHv1 5/8] arm64: dts: qcom: qcs404: Add PSCI cpuidle low power states Amit Kucheria
@ 2019-05-14 16:12 ` Niklas Cassel
2019-05-17 15:42 ` Daniel Lezcano
1 sibling, 0 replies; 36+ messages in thread
From: Niklas Cassel @ 2019-05-14 16:12 UTC (permalink / raw)
To: Amit Kucheria
Cc: linux-kernel, linux-arm-msm, bjorn.andersson, andy.gross,
David Brown, Li Yang, Shawn Guo, devicetree
On Fri, May 10, 2019 at 04:59:43PM +0530, Amit Kucheria wrote:
> From: Niklas Cassel <niklas.cassel@linaro.org>
>
> Add device bindings for cpuidle states for cpu devices.
>
> [amit: rename the idle-states to more generic names and fixups]
>
> Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
> Reviewed-by: Vinod Koul <vkoul@kernel.org>
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
> arch/arm64/boot/dts/qcom/qcs404.dtsi | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> index e8fd26633d57..369c59c35bc7 100644
> --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
> +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> @@ -30,6 +30,7 @@
> compatible = "arm,cortex-a53";
> reg = <0x100>;
> enable-method = "psci";
> + cpu-idle-states = <&CPU_SLEEP_0>;
> next-level-cache = <&L2_0>;
> };
>
> @@ -38,6 +39,7 @@
> compatible = "arm,cortex-a53";
> reg = <0x101>;
> enable-method = "psci";
> + cpu-idle-states = <&CPU_SLEEP_0>;
> next-level-cache = <&L2_0>;
> };
>
> @@ -46,6 +48,7 @@
> compatible = "arm,cortex-a53";
> reg = <0x102>;
> enable-method = "psci";
> + cpu-idle-states = <&CPU_SLEEP_0>;
> next-level-cache = <&L2_0>;
> };
>
> @@ -54,6 +57,7 @@
> compatible = "arm,cortex-a53";
> reg = <0x103>;
> enable-method = "psci";
> + cpu-idle-states = <&CPU_SLEEP_0>;
> next-level-cache = <&L2_0>;
> };
>
> @@ -61,6 +65,20 @@
> compatible = "cache";
> cache-level = <2>;
> };
> +
> + idle-states {
> + entry-method="psci";
Please add a space before and after "=".
> +
> + CPU_SLEEP_0: cpu-sleep-0 {
Regarding CPU_SLEEP_0 vs CPU_PC, see my comment on patch 4/8.
> + compatible = "arm,idle-state";
> + idle-state-name = "standalone-power-collapse";
> + arm,psci-suspend-param = <0x40000003>;
> + entry-latency-us = <125>;
> + exit-latency-us = <180>;
> + min-residency-us = <595>;
> + local-timer-stop;
> + };
> + };
> };
>
> firmware {
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 6/8] arm64: dts: qcom: msm8996: Add PSCI cpuidle low power states
2019-05-10 11:29 ` [PATCHv1 6/8] arm64: dts: qcom: msm8996: " Amit Kucheria
@ 2019-05-14 16:12 ` Niklas Cassel
2019-05-17 9:07 ` Amit Kucheria
2019-05-17 15:55 ` Daniel Lezcano
1 sibling, 1 reply; 36+ messages in thread
From: Niklas Cassel @ 2019-05-14 16:12 UTC (permalink / raw)
To: Amit Kucheria
Cc: Linux Kernel Mailing List, linux-arm-msm, Bjorn Andersson,
Andy Gross, David Brown, Li Yang, Shawn Guo, devicetree
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
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 7/8] arm64: dts: qcom: msm8998: Add PSCI cpuidle low power states
2019-05-10 11:29 ` [PATCHv1 7/8] arm64: dts: qcom: msm8998: " Amit Kucheria
2019-05-10 13:15 ` Marc Gonzalez
@ 2019-05-14 16:13 ` Niklas Cassel
2019-05-17 16:11 ` Daniel Lezcano
2 siblings, 0 replies; 36+ messages in thread
From: Niklas Cassel @ 2019-05-14 16:13 UTC (permalink / raw)
To: Amit Kucheria
Cc: linux-kernel, linux-arm-msm, bjorn.andersson, andy.gross,
David Brown, Li Yang, Shawn Guo, Marc Gonzalez, devicetree
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 <marc.w.gonzalez@free.fr>
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
> 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
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 8/8] arm64: dts: qcom: sdm845: Add PSCI cpuidle low power states
2019-05-10 11:29 ` [PATCHv1 8/8] arm64: dts: qcom: sdm845: " Amit Kucheria
@ 2019-05-14 16:13 ` Niklas Cassel
2019-05-17 16:25 ` Daniel Lezcano
1 sibling, 0 replies; 36+ messages in thread
From: Niklas Cassel @ 2019-05-14 16:13 UTC (permalink / raw)
To: Amit Kucheria
Cc: linux-kernel, linux-arm-msm, bjorn.andersson, andy.gross,
David Brown, Li Yang, Shawn Guo, Raju P.L.S.S.S.N, devicetree,
mkshah, ulf.hansson
On Fri, May 10, 2019 at 04:59:46PM +0530, Amit Kucheria wrote:
> From: "Raju P.L.S.S.S.N" <rplsssn@codeaurora.org>
>
> Add device bindings for cpuidle states for cpu devices.
>
> [amit: rename the idle-states to more generic names and fixups]
>
> Cc: <devicetree@vger.kernel.org>
> Cc: <mkshah@codeaurora.org>
> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
> Reviewed-by: Evan Green <evgreen@chromium.org>
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
> 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
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 4/8] arm64: dts: qcom: msm8916: Use more generic idle state names
2019-05-14 16:12 ` Niklas Cassel
@ 2019-05-15 10:13 ` Amit Kucheria
2019-05-15 13:02 ` Niklas Cassel
0 siblings, 1 reply; 36+ messages in thread
From: Amit Kucheria @ 2019-05-15 10:13 UTC (permalink / raw)
To: Niklas Cassel
Cc: Linux Kernel Mailing List, linux-arm-msm, Bjorn Andersson,
Andy Gross, David Brown, Li Yang, Shawn Guo, DTML
On Tue, May 14, 2019 at 9:42 PM Niklas Cassel <niklas.cassel@linaro.org> wrote:
>
> On Fri, May 10, 2019 at 04:59:42PM +0530, Amit Kucheria wrote:
> > Instead of using Qualcomm-specific terminology, use generic node names
> > for the idle states that are easier to understand. Move the description
> > into the "idle-state-name" property.
> >
> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> > ---
> > arch/arm64/boot/dts/qcom/msm8916.dtsi | 11 ++++++-----
> > 1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > index ded1052e5693..400b609bb3fd 100644
> > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > @@ -110,7 +110,7 @@
> > reg = <0x0>;
> > next-level-cache = <&L2_0>;
> > enable-method = "psci";
> > - cpu-idle-states = <&CPU_SPC>;
> > + cpu-idle-states = <&CPU_SLEEP_0>;
> > clocks = <&apcs>;
> > operating-points-v2 = <&cpu_opp_table>;
> > #cooling-cells = <2>;
> > @@ -122,7 +122,7 @@
> > reg = <0x1>;
> > next-level-cache = <&L2_0>;
> > enable-method = "psci";
> > - cpu-idle-states = <&CPU_SPC>;
> > + cpu-idle-states = <&CPU_SLEEP_0>;
> > clocks = <&apcs>;
> > operating-points-v2 = <&cpu_opp_table>;
> > #cooling-cells = <2>;
> > @@ -134,7 +134,7 @@
> > reg = <0x2>;
> > next-level-cache = <&L2_0>;
> > enable-method = "psci";
> > - cpu-idle-states = <&CPU_SPC>;
> > + cpu-idle-states = <&CPU_SLEEP_0>;
> > clocks = <&apcs>;
> > operating-points-v2 = <&cpu_opp_table>;
> > #cooling-cells = <2>;
> > @@ -146,7 +146,7 @@
> > reg = <0x3>;
> > next-level-cache = <&L2_0>;
> > enable-method = "psci";
> > - cpu-idle-states = <&CPU_SPC>;
> > + cpu-idle-states = <&CPU_SLEEP_0>;
> > clocks = <&apcs>;
> > operating-points-v2 = <&cpu_opp_table>;
> > #cooling-cells = <2>;
> > @@ -160,8 +160,9 @@
> > idle-states {
> > entry-method="psci";
>
> Please add a space before and after "=".
>
> >
> > - CPU_SPC: spc {
> > + CPU_SLEEP_0: cpu-sleep-0 {
>
> While I like your idea of using power state names from
> Server Base System Architecture document (SBSA) where applicable,
> does each qcom power state have a matching state in SBSA?
>
> These are the qcom power states:
> https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/Documentation/devicetree/bindings/arm/msm/lpm-levels.txt?h=msm-4.4#n53
>
> Note that qcom defines:
> "wfi", "retention", "gdhs", "pc", "fpc"
> while SBSA simply defines "idle_standby" (aka wfi), "idle_retention", "sleep".
>
> Unless you know the equivalent name for each qcom power state
> (perhaps several qcom power states are really the same SBSA state?),
> I think that you should omit the renaming from this patch series.
That is what SLEEP_0, SLEEP_1, SLEEP_2 could be used for.
IOW, all these qcom definitions are nicely represented in the
state-name and we could simply stick to SLEEP_0, SLEEP_1 for the node
names. There is wide variability in the the names of the qcom idle
states across SoC families downstream, so I'd argue against using
those for the node names.
Just for cpu states (non-wfi) I see the use of the following names
downstream across families. The C<num> seems to come from x86
world[1]:
- C4, standalone power collapse (spc)
- C4, power collapse (fpc)
- C2D, retention
- C3, power collapse (pc)
- C4, rail power collapse (rail-pc)
[1] https://www.hardwaresecrets.com/everything-you-need-to-know-about-the-cpu-c-states-power-saving-modes/
> > compatible = "arm,idle-state";
> > + idle-state-name = "standalone-power-collapse";
> > arm,psci-suspend-param = <0x40000002>;
> > entry-latency-us = <130>;
> > exit-latency-us = <150>;
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 4/8] arm64: dts: qcom: msm8916: Use more generic idle state names
2019-05-15 10:13 ` Amit Kucheria
@ 2019-05-15 13:02 ` Niklas Cassel
2019-05-21 5:38 ` Amit Kucheria
0 siblings, 1 reply; 36+ messages in thread
From: Niklas Cassel @ 2019-05-15 13:02 UTC (permalink / raw)
To: Amit Kucheria
Cc: Linux Kernel Mailing List, linux-arm-msm, Bjorn Andersson,
Andy Gross, David Brown, Li Yang, Shawn Guo, DTML
On Wed, May 15, 2019 at 03:43:19PM +0530, Amit Kucheria wrote:
> On Tue, May 14, 2019 at 9:42 PM Niklas Cassel <niklas.cassel@linaro.org> wrote:
> >
> > On Fri, May 10, 2019 at 04:59:42PM +0530, Amit Kucheria wrote:
> > > Instead of using Qualcomm-specific terminology, use generic node names
> > > for the idle states that are easier to understand. Move the description
> > > into the "idle-state-name" property.
> > >
> > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> > > ---
> > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 11 ++++++-----
> > > 1 file changed, 6 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > index ded1052e5693..400b609bb3fd 100644
> > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > @@ -110,7 +110,7 @@
> > > reg = <0x0>;
> > > next-level-cache = <&L2_0>;
> > > enable-method = "psci";
> > > - cpu-idle-states = <&CPU_SPC>;
> > > + cpu-idle-states = <&CPU_SLEEP_0>;
> > > clocks = <&apcs>;
> > > operating-points-v2 = <&cpu_opp_table>;
> > > #cooling-cells = <2>;
> > > @@ -122,7 +122,7 @@
> > > reg = <0x1>;
> > > next-level-cache = <&L2_0>;
> > > enable-method = "psci";
> > > - cpu-idle-states = <&CPU_SPC>;
> > > + cpu-idle-states = <&CPU_SLEEP_0>;
> > > clocks = <&apcs>;
> > > operating-points-v2 = <&cpu_opp_table>;
> > > #cooling-cells = <2>;
> > > @@ -134,7 +134,7 @@
> > > reg = <0x2>;
> > > next-level-cache = <&L2_0>;
> > > enable-method = "psci";
> > > - cpu-idle-states = <&CPU_SPC>;
> > > + cpu-idle-states = <&CPU_SLEEP_0>;
> > > clocks = <&apcs>;
> > > operating-points-v2 = <&cpu_opp_table>;
> > > #cooling-cells = <2>;
> > > @@ -146,7 +146,7 @@
> > > reg = <0x3>;
> > > next-level-cache = <&L2_0>;
> > > enable-method = "psci";
> > > - cpu-idle-states = <&CPU_SPC>;
> > > + cpu-idle-states = <&CPU_SLEEP_0>;
> > > clocks = <&apcs>;
> > > operating-points-v2 = <&cpu_opp_table>;
> > > #cooling-cells = <2>;
> > > @@ -160,8 +160,9 @@
> > > idle-states {
> > > entry-method="psci";
> >
> > Please add a space before and after "=".
> >
> > >
> > > - CPU_SPC: spc {
> > > + CPU_SLEEP_0: cpu-sleep-0 {
> >
> > While I like your idea of using power state names from
> > Server Base System Architecture document (SBSA) where applicable,
> > does each qcom power state have a matching state in SBSA?
> >
> > These are the qcom power states:
> > https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/Documentation/devicetree/bindings/arm/msm/lpm-levels.txt?h=msm-4.4#n53
> >
> > Note that qcom defines:
> > "wfi", "retention", "gdhs", "pc", "fpc"
> > while SBSA simply defines "idle_standby" (aka wfi), "idle_retention", "sleep".
> >
> > Unless you know the equivalent name for each qcom power state
> > (perhaps several qcom power states are really the same SBSA state?),
> > I think that you should omit the renaming from this patch series.
>
> That is what SLEEP_0, SLEEP_1, SLEEP_2 could be used for.
Ok, sounds good to me.
>
> IOW, all these qcom definitions are nicely represented in the
> state-name and we could simply stick to SLEEP_0, SLEEP_1 for the node
> names. There is wide variability in the the names of the qcom idle
> states across SoC families downstream, so I'd argue against using
> those for the node names.
>
> Just for cpu states (non-wfi) I see the use of the following names
> downstream across families. The C<num> seems to come from x86
> world[1]:
>
> - C4, standalone power collapse (spc)
> - C4, power collapse (fpc)
> - C2D, retention
> - C3, power collapse (pc)
> - C4, rail power collapse (rail-pc)
>
> [1] https://www.hardwaresecrets.com/everything-you-need-to-know-about-the-cpu-c-states-power-saving-modes/
Indeed, there seems to be mixed names used, I've also seen "fpc-def".
So, you have convinced me.
Kind regards,
Niklas
>
> > > 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
> > >
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 6/8] arm64: dts: qcom: msm8996: Add PSCI cpuidle low power states
2019-05-14 16:12 ` Niklas Cassel
@ 2019-05-17 9:07 ` Amit Kucheria
0 siblings, 0 replies; 36+ messages in thread
From: Amit Kucheria @ 2019-05-17 9:07 UTC (permalink / raw)
To: Niklas Cassel
Cc: Linux Kernel Mailing List, linux-arm-msm, Bjorn Andersson,
Andy Gross, David Brown, Li Yang, Shawn Guo, devicetree
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
> >
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 3/8] arm64: dts: qcom: msm8916: Add entry-method property for the idle-states node
2019-05-10 11:29 ` [PATCHv1 3/8] arm64: dts: qcom: msm8916: Add entry-method property for the idle-states node Amit Kucheria
2019-05-14 16:12 ` Niklas Cassel
@ 2019-05-17 15:40 ` Daniel Lezcano
1 sibling, 0 replies; 36+ messages in thread
From: Daniel Lezcano @ 2019-05-17 15:40 UTC (permalink / raw)
To: Amit Kucheria, linux-kernel, linux-arm-msm, bjorn.andersson,
andy.gross, David Brown, Li Yang, Shawn Guo
Cc: devicetree
On 10/05/2019 13:29, Amit Kucheria wrote:
> The idle-states binding documentation[1] mentions that the
> 'entry-method' property is required on 64-bit platforms and must be set
> to "psci".
>
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> index 0803ca8c02da..ded1052e5693 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> @@ -158,6 +158,8 @@
> };
>
> idle-states {
> + entry-method="psci";
> +
> CPU_SPC: spc {
> compatible = "arm,idle-state";
> arm,psci-suspend-param = <0x40000002>;
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 4/8] arm64: dts: qcom: msm8916: Use more generic idle state names
2019-05-10 11:29 ` [PATCHv1 4/8] arm64: dts: qcom: msm8916: Use more generic idle state names Amit Kucheria
2019-05-14 16:12 ` Niklas Cassel
@ 2019-05-17 15:41 ` Daniel Lezcano
1 sibling, 0 replies; 36+ messages in thread
From: Daniel Lezcano @ 2019-05-17 15:41 UTC (permalink / raw)
To: Amit Kucheria, linux-kernel, linux-arm-msm, bjorn.andersson,
andy.gross, David Brown, Li Yang, Shawn Guo
Cc: devicetree
On 10/05/2019 13:29, Amit Kucheria wrote:
> Instead of using Qualcomm-specific terminology, use generic node names
> for the idle states that are easier to understand. Move the description
> into the "idle-state-name" property.
>
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> arch/arm64/boot/dts/qcom/msm8916.dtsi | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> index ded1052e5693..400b609bb3fd 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> @@ -110,7 +110,7 @@
> reg = <0x0>;
> next-level-cache = <&L2_0>;
> enable-method = "psci";
> - cpu-idle-states = <&CPU_SPC>;
> + cpu-idle-states = <&CPU_SLEEP_0>;
> clocks = <&apcs>;
> operating-points-v2 = <&cpu_opp_table>;
> #cooling-cells = <2>;
> @@ -122,7 +122,7 @@
> reg = <0x1>;
> next-level-cache = <&L2_0>;
> enable-method = "psci";
> - cpu-idle-states = <&CPU_SPC>;
> + cpu-idle-states = <&CPU_SLEEP_0>;
> clocks = <&apcs>;
> operating-points-v2 = <&cpu_opp_table>;
> #cooling-cells = <2>;
> @@ -134,7 +134,7 @@
> reg = <0x2>;
> next-level-cache = <&L2_0>;
> enable-method = "psci";
> - cpu-idle-states = <&CPU_SPC>;
> + cpu-idle-states = <&CPU_SLEEP_0>;
> clocks = <&apcs>;
> operating-points-v2 = <&cpu_opp_table>;
> #cooling-cells = <2>;
> @@ -146,7 +146,7 @@
> reg = <0x3>;
> next-level-cache = <&L2_0>;
> enable-method = "psci";
> - cpu-idle-states = <&CPU_SPC>;
> + cpu-idle-states = <&CPU_SLEEP_0>;
> clocks = <&apcs>;
> operating-points-v2 = <&cpu_opp_table>;
> #cooling-cells = <2>;
> @@ -160,8 +160,9 @@
> idle-states {
> entry-method="psci";
>
> - CPU_SPC: spc {
> + CPU_SLEEP_0: cpu-sleep-0 {
> compatible = "arm,idle-state";
> + idle-state-name = "standalone-power-collapse";
> arm,psci-suspend-param = <0x40000002>;
> entry-latency-us = <130>;
> exit-latency-us = <150>;
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 5/8] arm64: dts: qcom: qcs404: Add PSCI cpuidle low power states
2019-05-10 11:29 ` [PATCHv1 5/8] arm64: dts: qcom: qcs404: Add PSCI cpuidle low power states Amit Kucheria
2019-05-14 16:12 ` Niklas Cassel
@ 2019-05-17 15:42 ` Daniel Lezcano
1 sibling, 0 replies; 36+ messages in thread
From: Daniel Lezcano @ 2019-05-17 15:42 UTC (permalink / raw)
To: Amit Kucheria, linux-kernel, linux-arm-msm, bjorn.andersson,
andy.gross, David Brown, Li Yang, Shawn Guo
Cc: Niklas Cassel, devicetree
On 10/05/2019 13:29, Amit Kucheria wrote:
> From: Niklas Cassel <niklas.cassel@linaro.org>
>
> Add device bindings for cpuidle states for cpu devices.
>
> [amit: rename the idle-states to more generic names and fixups]
>
> Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>
> Reviewed-by: Vinod Koul <vkoul@kernel.org>
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> arch/arm64/boot/dts/qcom/qcs404.dtsi | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/qcs404.dtsi b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> index e8fd26633d57..369c59c35bc7 100644
> --- a/arch/arm64/boot/dts/qcom/qcs404.dtsi
> +++ b/arch/arm64/boot/dts/qcom/qcs404.dtsi
> @@ -30,6 +30,7 @@
> compatible = "arm,cortex-a53";
> reg = <0x100>;
> enable-method = "psci";
> + cpu-idle-states = <&CPU_SLEEP_0>;
> next-level-cache = <&L2_0>;
> };
>
> @@ -38,6 +39,7 @@
> compatible = "arm,cortex-a53";
> reg = <0x101>;
> enable-method = "psci";
> + cpu-idle-states = <&CPU_SLEEP_0>;
> next-level-cache = <&L2_0>;
> };
>
> @@ -46,6 +48,7 @@
> compatible = "arm,cortex-a53";
> reg = <0x102>;
> enable-method = "psci";
> + cpu-idle-states = <&CPU_SLEEP_0>;
> next-level-cache = <&L2_0>;
> };
>
> @@ -54,6 +57,7 @@
> compatible = "arm,cortex-a53";
> reg = <0x103>;
> enable-method = "psci";
> + cpu-idle-states = <&CPU_SLEEP_0>;
> next-level-cache = <&L2_0>;
> };
>
> @@ -61,6 +65,20 @@
> compatible = "cache";
> cache-level = <2>;
> };
> +
> + idle-states {
> + entry-method="psci";
> +
> + 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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 6/8] arm64: dts: qcom: msm8996: Add PSCI cpuidle low power states
2019-05-10 11:29 ` [PATCHv1 6/8] arm64: dts: qcom: msm8996: " Amit Kucheria
2019-05-14 16:12 ` Niklas Cassel
@ 2019-05-17 15:55 ` Daniel Lezcano
1 sibling, 0 replies; 36+ messages in thread
From: Daniel Lezcano @ 2019-05-17 15:55 UTC (permalink / raw)
To: Amit Kucheria, linux-kernel, linux-arm-msm, bjorn.andersson,
andy.gross, David Brown, Li Yang, Shawn Guo
Cc: devicetree
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 {
>
--
<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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 7/8] arm64: dts: qcom: msm8998: Add PSCI cpuidle low power states
2019-05-10 11:29 ` [PATCHv1 7/8] arm64: dts: qcom: msm8998: " Amit Kucheria
2019-05-10 13:15 ` Marc Gonzalez
2019-05-14 16:13 ` Niklas Cassel
@ 2019-05-17 16:11 ` Daniel Lezcano
2 siblings, 0 replies; 36+ messages in thread
From: Daniel Lezcano @ 2019-05-17 16:11 UTC (permalink / raw)
To: Amit Kucheria, linux-kernel, linux-arm-msm, bjorn.andersson,
andy.gross, David Brown, Li Yang, Shawn Guo
Cc: Marc Gonzalez, devicetree
On 10/05/2019 13:29, Amit Kucheria wrote:
> Add device bindings for cpuidle states for cpu devices.
>
> Cc: Marc Gonzalez <marc.w.gonzalez@free.fr>
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> arch/arm64/boot/dts/qcom/msm8998.dtsi | 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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 8/8] arm64: dts: qcom: sdm845: Add PSCI cpuidle low power states
2019-05-10 11:29 ` [PATCHv1 8/8] arm64: dts: qcom: sdm845: " Amit Kucheria
2019-05-14 16:13 ` Niklas Cassel
@ 2019-05-17 16:25 ` Daniel Lezcano
1 sibling, 0 replies; 36+ messages in thread
From: Daniel Lezcano @ 2019-05-17 16:25 UTC (permalink / raw)
To: Amit Kucheria, linux-kernel, linux-arm-msm, bjorn.andersson,
andy.gross, David Brown, Li Yang, Shawn Guo
Cc: Raju P.L.S.S.S.N, devicetree, mkshah
On 10/05/2019 13:29, Amit Kucheria wrote:
> From: "Raju P.L.S.S.S.N" <rplsssn@codeaurora.org>
>
> Add device bindings for cpuidle states for cpu devices.
>
> [amit: rename the idle-states to more generic names and fixups]
>
> Cc: <devicetree@vger.kernel.org>
> Cc: <mkshah@codeaurora.org>
> Signed-off-by: Raju P.L.S.S.S.N <rplsssn@codeaurora.org>
> Reviewed-by: Evan Green <evgreen@chromium.org>
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
--
<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
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 4/8] arm64: dts: qcom: msm8916: Use more generic idle state names
2019-05-15 13:02 ` Niklas Cassel
@ 2019-05-21 5:38 ` Amit Kucheria
2019-05-21 8:50 ` Niklas Cassel
0 siblings, 1 reply; 36+ messages in thread
From: Amit Kucheria @ 2019-05-21 5:38 UTC (permalink / raw)
To: Niklas Cassel
Cc: Linux Kernel Mailing List, linux-arm-msm, Bjorn Andersson,
Andy Gross, David Brown, Li Yang, Shawn Guo, DTML
On Wed, May 15, 2019 at 6:33 PM Niklas Cassel <niklas.cassel@linaro.org> wrote:
>
> On Wed, May 15, 2019 at 03:43:19PM +0530, Amit Kucheria wrote:
> > On Tue, May 14, 2019 at 9:42 PM Niklas Cassel <niklas.cassel@linaro.org> wrote:
> > >
> > > On Fri, May 10, 2019 at 04:59:42PM +0530, Amit Kucheria wrote:
> > > > Instead of using Qualcomm-specific terminology, use generic node names
> > > > for the idle states that are easier to understand. Move the description
> > > > into the "idle-state-name" property.
> > > >
> > > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> > > > ---
> > > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 11 ++++++-----
> > > > 1 file changed, 6 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > index ded1052e5693..400b609bb3fd 100644
> > > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > @@ -110,7 +110,7 @@
> > > > reg = <0x0>;
> > > > next-level-cache = <&L2_0>;
> > > > enable-method = "psci";
> > > > - cpu-idle-states = <&CPU_SPC>;
> > > > + cpu-idle-states = <&CPU_SLEEP_0>;
> > > > clocks = <&apcs>;
> > > > operating-points-v2 = <&cpu_opp_table>;
> > > > #cooling-cells = <2>;
> > > > @@ -122,7 +122,7 @@
> > > > reg = <0x1>;
> > > > next-level-cache = <&L2_0>;
> > > > enable-method = "psci";
> > > > - cpu-idle-states = <&CPU_SPC>;
> > > > + cpu-idle-states = <&CPU_SLEEP_0>;
> > > > clocks = <&apcs>;
> > > > operating-points-v2 = <&cpu_opp_table>;
> > > > #cooling-cells = <2>;
> > > > @@ -134,7 +134,7 @@
> > > > reg = <0x2>;
> > > > next-level-cache = <&L2_0>;
> > > > enable-method = "psci";
> > > > - cpu-idle-states = <&CPU_SPC>;
> > > > + cpu-idle-states = <&CPU_SLEEP_0>;
> > > > clocks = <&apcs>;
> > > > operating-points-v2 = <&cpu_opp_table>;
> > > > #cooling-cells = <2>;
> > > > @@ -146,7 +146,7 @@
> > > > reg = <0x3>;
> > > > next-level-cache = <&L2_0>;
> > > > enable-method = "psci";
> > > > - cpu-idle-states = <&CPU_SPC>;
> > > > + cpu-idle-states = <&CPU_SLEEP_0>;
> > > > clocks = <&apcs>;
> > > > operating-points-v2 = <&cpu_opp_table>;
> > > > #cooling-cells = <2>;
> > > > @@ -160,8 +160,9 @@
> > > > idle-states {
> > > > entry-method="psci";
> > >
> > > Please add a space before and after "=".
> > >
> > > >
> > > > - CPU_SPC: spc {
> > > > + CPU_SLEEP_0: cpu-sleep-0 {
> > >
> > > While I like your idea of using power state names from
> > > Server Base System Architecture document (SBSA) where applicable,
> > > does each qcom power state have a matching state in SBSA?
> > >
> > > These are the qcom power states:
> > > https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/Documentation/devicetree/bindings/arm/msm/lpm-levels.txt?h=msm-4.4#n53
> > >
> > > Note that qcom defines:
> > > "wfi", "retention", "gdhs", "pc", "fpc"
> > > while SBSA simply defines "idle_standby" (aka wfi), "idle_retention", "sleep".
> > >
> > > Unless you know the equivalent name for each qcom power state
> > > (perhaps several qcom power states are really the same SBSA state?),
> > > I think that you should omit the renaming from this patch series.
> >
> > That is what SLEEP_0, SLEEP_1, SLEEP_2 could be used for.
>
> Ok, sounds good to me.
>
> >
> > IOW, all these qcom definitions are nicely represented in the
> > state-name and we could simply stick to SLEEP_0, SLEEP_1 for the node
> > names. There is wide variability in the the names of the qcom idle
> > states across SoC families downstream, so I'd argue against using
> > those for the node names.
> >
> > Just for cpu states (non-wfi) I see the use of the following names
> > downstream across families. The C<num> seems to come from x86
> > world[1]:
> >
> > - C4, standalone power collapse (spc)
> > - C4, power collapse (fpc)
> > - C2D, retention
> > - C3, power collapse (pc)
> > - C4, rail power collapse (rail-pc)
> >
> > [1] https://www.hardwaresecrets.com/everything-you-need-to-know-about-the-cpu-c-states-power-saving-modes/
>
> Indeed, there seems to be mixed names used, I've also seen "fpc-def".
>
> So, you have convinced me.
>
>
> Kind regards,
> Niklas
Can I take that as a Reviewed-by?
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCHv1 4/8] arm64: dts: qcom: msm8916: Use more generic idle state names
2019-05-21 5:38 ` Amit Kucheria
@ 2019-05-21 8:50 ` Niklas Cassel
0 siblings, 0 replies; 36+ messages in thread
From: Niklas Cassel @ 2019-05-21 8:50 UTC (permalink / raw)
To: Amit Kucheria
Cc: Linux Kernel Mailing List, linux-arm-msm, Bjorn Andersson,
Andy Gross, David Brown, Li Yang, Shawn Guo, DTML
On Tue, May 21, 2019 at 11:08:09AM +0530, Amit Kucheria wrote:
> On Wed, May 15, 2019 at 6:33 PM Niklas Cassel <niklas.cassel@linaro.org> wrote:
> >
> > On Wed, May 15, 2019 at 03:43:19PM +0530, Amit Kucheria wrote:
> > > On Tue, May 14, 2019 at 9:42 PM Niklas Cassel <niklas.cassel@linaro.org> wrote:
> > > >
> > > > On Fri, May 10, 2019 at 04:59:42PM +0530, Amit Kucheria wrote:
> > > > > Instead of using Qualcomm-specific terminology, use generic node names
> > > > > for the idle states that are easier to understand. Move the description
> > > > > into the "idle-state-name" property.
> > > > >
> > > > > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> > > > > ---
> > > > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 11 ++++++-----
> > > > > 1 file changed, 6 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > > index ded1052e5693..400b609bb3fd 100644
> > > > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > > @@ -110,7 +110,7 @@
> > > > > reg = <0x0>;
> > > > > next-level-cache = <&L2_0>;
> > > > > enable-method = "psci";
> > > > > - cpu-idle-states = <&CPU_SPC>;
> > > > > + cpu-idle-states = <&CPU_SLEEP_0>;
> > > > > clocks = <&apcs>;
> > > > > operating-points-v2 = <&cpu_opp_table>;
> > > > > #cooling-cells = <2>;
> > > > > @@ -122,7 +122,7 @@
> > > > > reg = <0x1>;
> > > > > next-level-cache = <&L2_0>;
> > > > > enable-method = "psci";
> > > > > - cpu-idle-states = <&CPU_SPC>;
> > > > > + cpu-idle-states = <&CPU_SLEEP_0>;
> > > > > clocks = <&apcs>;
> > > > > operating-points-v2 = <&cpu_opp_table>;
> > > > > #cooling-cells = <2>;
> > > > > @@ -134,7 +134,7 @@
> > > > > reg = <0x2>;
> > > > > next-level-cache = <&L2_0>;
> > > > > enable-method = "psci";
> > > > > - cpu-idle-states = <&CPU_SPC>;
> > > > > + cpu-idle-states = <&CPU_SLEEP_0>;
> > > > > clocks = <&apcs>;
> > > > > operating-points-v2 = <&cpu_opp_table>;
> > > > > #cooling-cells = <2>;
> > > > > @@ -146,7 +146,7 @@
> > > > > reg = <0x3>;
> > > > > next-level-cache = <&L2_0>;
> > > > > enable-method = "psci";
> > > > > - cpu-idle-states = <&CPU_SPC>;
> > > > > + cpu-idle-states = <&CPU_SLEEP_0>;
> > > > > clocks = <&apcs>;
> > > > > operating-points-v2 = <&cpu_opp_table>;
> > > > > #cooling-cells = <2>;
> > > > > @@ -160,8 +160,9 @@
> > > > > idle-states {
> > > > > entry-method="psci";
> > > >
> > > > Please add a space before and after "=".
> > > >
> > > > >
> > > > > - CPU_SPC: spc {
> > > > > + CPU_SLEEP_0: cpu-sleep-0 {
> > > >
> > > > While I like your idea of using power state names from
> > > > Server Base System Architecture document (SBSA) where applicable,
> > > > does each qcom power state have a matching state in SBSA?
> > > >
> > > > These are the qcom power states:
> > > > https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/Documentation/devicetree/bindings/arm/msm/lpm-levels.txt?h=msm-4.4#n53
> > > >
> > > > Note that qcom defines:
> > > > "wfi", "retention", "gdhs", "pc", "fpc"
> > > > while SBSA simply defines "idle_standby" (aka wfi), "idle_retention", "sleep".
> > > >
> > > > Unless you know the equivalent name for each qcom power state
> > > > (perhaps several qcom power states are really the same SBSA state?),
> > > > I think that you should omit the renaming from this patch series.
> > >
> > > That is what SLEEP_0, SLEEP_1, SLEEP_2 could be used for.
> >
> > Ok, sounds good to me.
> >
> > >
> > > IOW, all these qcom definitions are nicely represented in the
> > > state-name and we could simply stick to SLEEP_0, SLEEP_1 for the node
> > > names. There is wide variability in the the names of the qcom idle
> > > states across SoC families downstream, so I'd argue against using
> > > those for the node names.
> > >
> > > Just for cpu states (non-wfi) I see the use of the following names
> > > downstream across families. The C<num> seems to come from x86
> > > world[1]:
> > >
> > > - C4, standalone power collapse (spc)
> > > - C4, power collapse (fpc)
> > > - C2D, retention
> > > - C3, power collapse (pc)
> > > - C4, rail power collapse (rail-pc)
> > >
> > > [1] https://www.hardwaresecrets.com/everything-you-need-to-know-about-the-cpu-c-states-power-saving-modes/
> >
> > Indeed, there seems to be mixed names used, I've also seen "fpc-def".
> >
> > So, you have convinced me.
> >
> >
> > Kind regards,
> > Niklas
>
> Can I take that as a Reviewed-by?
Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2019-05-21 8:50 UTC | newest]
Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-10 11:29 [PATCHv1 0/8] qcom: Add cpuidle to some platforms Amit Kucheria
2019-05-10 11:29 ` [PATCHv1 1/8] arm64: dts: Fix various entry-method properties to reflect documentation Amit Kucheria
2019-05-10 12:54 ` Sudeep Holla
2019-05-13 18:39 ` Bjorn Andersson
2019-05-14 5:57 ` Amit Kucheria
2019-05-14 16:11 ` Niklas Cassel
2019-05-10 11:29 ` [PATCHv1 2/8] Documentation: arm: Link idle-states binding to code Amit Kucheria
2019-05-10 13:02 ` Sudeep Holla
2019-05-10 11:29 ` [PATCHv1 3/8] arm64: dts: qcom: msm8916: Add entry-method property for the idle-states node Amit Kucheria
2019-05-14 16:12 ` Niklas Cassel
2019-05-17 15:40 ` Daniel Lezcano
2019-05-10 11:29 ` [PATCHv1 4/8] arm64: dts: qcom: msm8916: Use more generic idle state names Amit Kucheria
2019-05-14 16:12 ` Niklas Cassel
2019-05-15 10:13 ` Amit Kucheria
2019-05-15 13:02 ` Niklas Cassel
2019-05-21 5:38 ` Amit Kucheria
2019-05-21 8:50 ` Niklas Cassel
2019-05-17 15:41 ` Daniel Lezcano
2019-05-10 11:29 ` [PATCHv1 5/8] arm64: dts: qcom: qcs404: Add PSCI cpuidle low power states Amit Kucheria
2019-05-14 16:12 ` Niklas Cassel
2019-05-17 15:42 ` Daniel Lezcano
2019-05-10 11:29 ` [PATCHv1 6/8] arm64: dts: qcom: msm8996: " Amit Kucheria
2019-05-14 16:12 ` Niklas Cassel
2019-05-17 9:07 ` Amit Kucheria
2019-05-17 15:55 ` Daniel Lezcano
2019-05-10 11:29 ` [PATCHv1 7/8] arm64: dts: qcom: msm8998: " Amit Kucheria
2019-05-10 13:15 ` Marc Gonzalez
2019-05-10 14:12 ` Amit Kucheria
2019-05-10 15:11 ` Marc Gonzalez
2019-05-13 12:38 ` Amit Kucheria
2019-05-14 16:13 ` Niklas Cassel
2019-05-17 16:11 ` Daniel Lezcano
2019-05-10 11:29 ` [PATCHv1 8/8] arm64: dts: qcom: sdm845: " Amit Kucheria
2019-05-14 16:13 ` Niklas Cassel
2019-05-17 16:25 ` Daniel Lezcano
2019-05-14 16:11 ` [PATCHv1 0/8] qcom: Add cpuidle to some platforms Niklas Cassel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).