Skip to contentSkip to conversation

arm64: dts: rockchip: remove wrongly added idle states on rk3368

PATCH New Heiko Stuebner 3 participants 5 messages Mar 1, 2017 – Mar 16, 2017

Subsystems (2)
Device tree bindings Arm Rockchip
lore
Share & tools
lore

Heiko Stuebner

author maintainer
#0 lore raw

PATCH 1/1 arm64: dts: rockchip: remove wrongly added idle states on rk3368

As reported by Lorenzo, the residency/latency values defined in the idle-state for rk3368 "make no sense". When introducing them I simply took the idle-state node from the vendor kernel in error as I didn't look up if these values were sane in the first place.

Talking to people and determining why they were used in this way showed that it was meant to make sure the cpu_suspend callback got initialized which at the 3.10 time was somehow required even for wfi-based idle handling.

Of course the generic arch_cpu_idle() now does wfi-based idle-handling already, so there is no need for this.

Reported-by Lorenzo Pieralisi <[email protected]>
Signed-off-by Heiko Stuebner <[email protected]>

---
This has been sitting ere for way to long :-(, but finally I got to finish the change.

 arch/arm64/boot/dts/rockchip/rk3368.dtsi | 20 --------------------
 1 file changed, 20 deletions(-)
arch/arm64/boot/dts/rockchip/rk3368.dtsi +0 -20
index 49d1191..5471ace 100644
@@ -107,23 +107,10 @@
 			};
 		};
 
-		idle-states {
-			entry-method = "psci";
-
-			cpu_sleep: cpu-sleep-0 {
-				compatible = "arm,idle-state";
-				arm,psci-suspend-param = <0x1010000>;
-				entry-latency-us = <0x3fffffff>;
-				exit-latency-us = <0x40000000>;
-				min-residency-us = <0xffffffff>;
-			};
-		};
-
 		cpu_l0: cpu at 0 {
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x0>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 
 			#cooling-cells = <2>; /* min followed by max */

@@ -133,7 +120,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x1>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 

@@ -141,7 +127,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x2>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 

@@ -149,7 +134,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x3>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 

@@ -157,7 +141,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x100>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 
 			#cooling-cells = <2>; /* min followed by max */

@@ -167,7 +150,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x101>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 

@@ -175,7 +157,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x102>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 

@@ -183,7 +164,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x103>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 	};
Signature · 1 line
2.6.4
#1 lore raw

PATCH 1/1 arm64: dts: rockchip: remove wrongly added idle states on rk3368

Hi Heiko:

Quoted 99 lines · Heiko Stuebner

As reported by Lorenzo, the residency/latency values defined in the idle-state for rk3368 "make no sense". When introducing them I simply took the idle-state node from the vendor kernel in error as I didn't look up if these values were sane in the first place.

Talking to people and determining why they were used in this way showed that it was meant to make sure the cpu_suspend callback got initialized which at the 3.10 time was somehow required even for wfi-based idle handling.

Of course the generic arch_cpu_idle() now does wfi-based idle-handling already, so there is no need for this.

Reported-by Lorenzo Pieralisi <[email protected]>
Signed-off-by Heiko Stuebner <[email protected]>

---
This has been sitting ere for way to long :-(, but finally I got to finish the change.

 arch/arm64/boot/dts/rockchip/rk3368.dtsi | 20 --------------------
 1 file changed, 20 deletions(-)
arch/arm64/boot/dts/rockchip/rk3368.dtsi +0 -19
index 49d1191..5471ace 100644
@@ -107,23 +107,10 @@
 			};
 		};
 
-		idle-states {
-			entry-method = "psci";
-
-			cpu_sleep: cpu-sleep-0 {
-				compatible = "arm,idle-state";
-				arm,psci-suspend-param = <0x1010000>;
-				entry-latency-us = <0x3fffffff>;
-				exit-latency-us = <0x40000000>;
-				min-residency-us = <0xffffffff>;
-			};
-		};
-
 		cpu_l0: cpu at 0 {
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x0>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 
 			#cooling-cells = <2>; /* min followed by max */

@@ -133,7 +120,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x1>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 

@@ -141,7 +127,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x2>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 

@@ -149,7 +134,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x3>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 

@@ -157,7 +141,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x100>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 
 			#cooling-cells = <2>; /* min followed by max */

@@ -167,7 +150,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x101>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 

@@ -175,7 +157,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x102>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};

arch/arm64/boot/dts/rockchip/rk3368.dtsi line 169

Quoted 7 lines
arch/arm64/boot/dts/rockchip/rk3368.dtsi +0 -1
@@ -183,7 +164,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x103>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 	};

Looks good to me.
There are another redundant node - timer at ff810000, which can not probe successfully because of missing clocks property. RK3368 don't support power down core while idle, so there are not need add broadcast timer. Should we delete the timer node or add missing clocks?
If add clocks, we should fix clk drivers first because timer clk is not exported.
I suggest we should delete the timer node.

author maintainer
#2 lore raw

PATCH 1/1 arm64: dts: rockchip: remove wrongly added idle states on rk3368

Am Dienstag, 7. M?rz 2017, 15:20:28 CET schrieb Huang, Tao:

Quoted 144 lines

Hi Heiko:

Quoted 141 lines · Heiko Stuebner

As reported by Lorenzo, the residency/latency values defined in the idle-state for rk3368 "make no sense". When introducing them I simply took the idle-state node from the vendor kernel in error as I didn't look up if these values were sane in the first place.

Talking to people and determining why they were used in this way showed that it was meant to make sure the cpu_suspend callback got initialized which at the 3.10 time was somehow required even for wfi-based idle handling.

Of course the generic arch_cpu_idle() now does wfi-based idle-handling already, so there is no need for this.

Reported-by Lorenzo Pieralisi <[email protected]>
Signed-off-by Heiko Stuebner <[email protected]>

---
This has been sitting ere for way to long :-(, but finally I got to finish the change.

 arch/arm64/boot/dts/rockchip/rk3368.dtsi | 20 --------------------
 1 file changed, 20 deletions(-)
(quoted diff) +0 -0

b/arch/arm64/boot/dts/rockchip/rk3368.dtsi index 49d1191..5471ace 100644

arch/arm64/boot/dts/rockchip/rk3368.dtsi +0 -14
@@ -107,23 +107,10 @@
 
 			};
 
 		};
 
-		idle-states {
-			entry-method = "psci";
-
-			cpu_sleep: cpu-sleep-0 {
-				compatible = "arm,idle-state";
-				arm,psci-suspend-param = <0x1010000>;
-				entry-latency-us = <0x3fffffff>;
-				exit-latency-us = <0x40000000>;
-				min-residency-us = <0xffffffff>;
-			};
-		};
-
 
 		cpu_l0: cpu at 0 {
 
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x0>;
 
-			cpu-idle-states = <&cpu_sleep>;
 
 			enable-method = "psci";
 
 			#cooling-cells = <2>; /* min followed by max */

@@ -133,7 +120,6 @@
 
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x1>;
 
-			cpu-idle-states = <&cpu_sleep>;
arch/arm64/boot/dts/rockchip/rk3368.dtsi +0 -1
 			enable-method = "psci";
 
 		};

@@ -141,7 +127,6 @@
 
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x2>;
 
-			cpu-idle-states = <&cpu_sleep>;
arch/arm64/boot/dts/rockchip/rk3368.dtsi +0 -1
 			enable-method = "psci";
 
 		};

@@ -149,7 +134,6 @@
 
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x3>;
 
-			cpu-idle-states = <&cpu_sleep>;
arch/arm64/boot/dts/rockchip/rk3368.dtsi +0 -1
 			enable-method = "psci";
 
 		};

@@ -157,7 +141,6 @@
 
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x100>;
 
-			cpu-idle-states = <&cpu_sleep>;
arch/arm64/boot/dts/rockchip/rk3368.dtsi +0 -1
 			enable-method = "psci";
 
 			#cooling-cells = <2>; /* min followed by max */

@@ -167,7 +150,6 @@
 
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x101>;
 
-			cpu-idle-states = <&cpu_sleep>;
arch/arm64/boot/dts/rockchip/rk3368.dtsi +0 -1
 			enable-method = "psci";
 
 		};

@@ -175,7 +157,6 @@
 
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x102>;
 
-			cpu-idle-states = <&cpu_sleep>;
arch/arm64/boot/dts/rockchip/rk3368.dtsi +0 -1
 			enable-method = "psci";
 
 		};

@@ -183,7 +164,6 @@
 
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x103>;
 
-			cpu-idle-states = <&cpu_sleep>;
arch/arm64/boot/dts/rockchip/rk3368.dtsi +0 -0
 			enable-method = "psci";
 
 		};
Quoted 10 lines
Quoted 1 line

};

Looks good to me.
There are another redundant node - timer at ff810000, which can not probe successfully because of missing clocks property. RK3368 don't support power down core while idle, so there are not need add broadcast timer. Should we delete the timer node or add missing clocks?
If add clocks, we should fix clk drivers first because timer clk is not exported.
I suggest we should delete the timer node.

Elaine seems to want to add time-specific clock ids - see patches from today and I tend to agree with her and simply fixup the timer dts node after that.

Afterall the dt is a hardware description, not a description of what we currently like to have in the kernel :-)

Heiko

#3 lore raw

PATCH 1/1 arm64: dts: rockchip: remove wrongly added idle states on rk3368

In reply to Heiko Stuebner
Quoted 10 lines · Heiko Stuebner

As reported by Lorenzo, the residency/latency values defined in the idle-state for rk3368 "make no sense". When introducing them I simply took the idle-state node from the vendor kernel in error as I didn't look up if these values were sane in the first place.

Talking to people and determining why they were used in this way showed that it was meant to make sure the cpu_suspend callback got initialized which at the 3.10 time was somehow required even for wfi-based idle handling.

Quoted 8 lines

Of course the generic arch_cpu_idle() now does wfi-based idle-handling already, so there is no need for this.

Reported-by Lorenzo Pieralisi <[email protected]>
Signed-off-by Heiko Stuebner <[email protected]>

---
This has been sitting ere for way to long :-(, but finally I got to finish the change.

Well, the commit log needs updating, you should describe what it does and why, if you want to add a link to the discussion in the commit log use a Link: tag as per kernel documentation.

So, there are not any idle states implemented in this platform ?

I probably asked before, if there are instead of removing the states you can just update the latencies with sane values.

If wfi is the only FW/HW implemented quiescent state then:

Acked-by Lorenzo Pieralisi <[email protected]>
Quoted 90 lines
 arch/arm64/boot/dts/rockchip/rk3368.dtsi | 20 --------------------
 1 file changed, 20 deletions(-)
arch/arm64/boot/dts/rockchip/rk3368.dtsi +0 -20
index 49d1191..5471ace 100644
@@ -107,23 +107,10 @@
 			};
 		};
 
-		idle-states {
-			entry-method = "psci";
-
-			cpu_sleep: cpu-sleep-0 {
-				compatible = "arm,idle-state";
-				arm,psci-suspend-param = <0x1010000>;
-				entry-latency-us = <0x3fffffff>;
-				exit-latency-us = <0x40000000>;
-				min-residency-us = <0xffffffff>;
-			};
-		};
-
 		cpu_l0: cpu at 0 {
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x0>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 
 			#cooling-cells = <2>; /* min followed by max */

@@ -133,7 +120,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x1>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 

@@ -141,7 +127,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x2>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 

@@ -149,7 +134,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x3>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 

@@ -157,7 +141,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x100>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 
 			#cooling-cells = <2>; /* min followed by max */

@@ -167,7 +150,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x101>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 

@@ -175,7 +157,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x102>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 

@@ -183,7 +164,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x103>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 	};
Signature · 1 line
2.6.4
author maintainer
#4 lore raw

PATCH 1/1 arm64: dts: rockchip: remove wrongly added idle states on rk3368

In reply to Heiko Stuebner

Am Mittwoch, 1. M?rz 2017, 23:38:49 CET schrieb Heiko Stuebner:

Quoted 10 lines

As reported by Lorenzo, the residency/latency values defined in the idle-state for rk3368 "make no sense". When introducing them I simply took the idle-state node from the vendor kernel in error as I didn't look up if these values were sane in the first place.

Talking to people and determining why they were used in this way showed that it was meant to make sure the cpu_suspend callback got initialized which at the 3.10 time was somehow required even for wfi-based idle handling.

Quoted 5 lines

Of course the generic arch_cpu_idle() now does wfi-based idle-handling already, so there is no need for this.

Reported-by Lorenzo Pieralisi <[email protected]>
Signed-off-by Heiko Stuebner <[email protected]>

applied for 4.12 with Lorenzo's Ack after clarifying in the last paragraph:

"Of course the generic arch_cpu_idle() now does wfi-based idle-handling already and the rk3368 does not implement any other idle states than the default WFI, so these wrong idle-states should go away."

Quoted 91 lines

---
This has been sitting ere for way to long :-(, but finally I got to finish the change.

 arch/arm64/boot/dts/rockchip/rk3368.dtsi | 20 --------------------
 1 file changed, 20 deletions(-)
(quoted diff) +0 -0

b/arch/arm64/boot/dts/rockchip/rk3368.dtsi index 49d1191..5471ace 100644

arch/arm64/boot/dts/rockchip/rk3368.dtsi +0 -20
@@ -107,23 +107,10 @@
 			};
 		};
 
-		idle-states {
-			entry-method = "psci";
-
-			cpu_sleep: cpu-sleep-0 {
-				compatible = "arm,idle-state";
-				arm,psci-suspend-param = <0x1010000>;
-				entry-latency-us = <0x3fffffff>;
-				exit-latency-us = <0x40000000>;
-				min-residency-us = <0xffffffff>;
-			};
-		};
-
 		cpu_l0: cpu at 0 {
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x0>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 
 			#cooling-cells = <2>; /* min followed by max */

@@ -133,7 +120,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x1>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 

@@ -141,7 +127,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x2>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 

@@ -149,7 +134,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x3>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 

@@ -157,7 +141,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x100>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 
 			#cooling-cells = <2>; /* min followed by max */

@@ -167,7 +150,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x101>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 

@@ -175,7 +157,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x102>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 

@@ -183,7 +164,6 @@
 			device_type = "cpu";
 			compatible = "arm,cortex-a53", "arm,armv8";
 			reg = <0x0 0x103>;
-			cpu-idle-states = <&cpu_sleep>;
 			enable-method = "psci";
 		};
 	};