Re: [PATCH 4/4] arm64: dts: rockchip: update the thermal zones for RK3399 SoCs

[Heiko Stuebner]

Hi Caesar,

Am Mittwoch, 12. Juli 2017, 14:29:30 CEST schrieb Caesar Wang:
> As RK3399 had used the Power allocator thermal governor by default,
> enabled this to manage thermals by dynamically allocating and limiting
> power to devices.

Does this still run with other thermal governors? The devicetree describes
the hardware, but should not mandate or exclude specific implementations.


> Also, this patch supported the dynamic-power-coefficient/sustainable_power
> and GPU's power model for needed parameters with thermal IPA.

As written below, this doesn't look like a reviewed binding (otherwise
please point me to the binding patch), but even if it is a real binding
it should get its separate patch.


> Signed-off-by: Caesar Wang 
> 
> ---
> 
>  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 62 
> +++-
>  1 file changed, 29 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi 
> b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> index 8c6438b..139f58c 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -147,7 +147,7 @@
>   enable-method = "psci";
>   #cooling-cells = <2>; /* min followed by max */
>   clocks = < ARMCLKB>;
> - dynamic-power-coefficient = <100>;
> + dynamic-power-coefficient = <436>;
>   };
>  
>   cpu_b1: cpu@101 {
> @@ -156,7 +156,7 @@
>   reg = <0x0 0x101>;
>   enable-method = "psci";
>   clocks = < ARMCLKB>;
> - dynamic-power-coefficient = <100>;
> + dynamic-power-coefficient = <436>;
>   };
>   };
>  
> @@ -690,24 +690,25 @@
>   };
>  
>   thermal_zones: thermal-zones {
> - cpu_thermal: cpu {
> + soc_thermal: soc-thermal {
>   polling-delay-passive = <100>;
>   polling-delay = <1000>;
> + sustainable-power = <1000>;
>  
>   thermal-sensors = < 0>;
>  
>   trips {
> - cpu_alert0: cpu_alert0 {
> + threshold: trip-point@0 {

foo@0 will produce warnings when used without reg property. Also,
why all that renaming, the previous names sounded fine to me.


>   temperature = <7>;
>   hysteresis = <2000>;
>   type = "passive";
>   };
> - cpu_alert1: cpu_alert1 {
> - temperature = <75000>;
> + target: trip-point@1 {
> + temperature = <85000>;

When raising the target-temperature to 85 degrees I really
do expect some sort of reassurement in the commit message
why that is really safe - especially when the old limit was 10 degrees
lower.

>   hysteresis = <2000>;
>   type = "passive";
>   };
> - cpu_crit: cpu_crit {
> + soc_crit: soc-crit {
>   temperature = <95000>;
>   hysteresis = <2000>;
>   type = "critical";
> @@ -716,45 +717,31 @@
>  
>   cooling-maps {
>   map0 {
> - trip = <_alert0>;
> + trip = <>;
>   cooling-device =
> - <_b0 THERMAL_NO_LIMIT 
> THERMAL_NO_LIMIT>;
> + <_l0 THERMAL_NO_LIMIT 
> THERMAL_NO_LIMIT>;
> + contribution = <4096>;
>   };
>   map1 {
> - trip = <_alert1>;
> + trip = <>;

Is it correct to use the _same_ trip point all the time? ... what about
the threshold and soc_crit ones?

>   cooling-device =
> - <_l0 THERMAL_NO_LIMIT 
> THERMAL_NO_LIMIT>,
>   <_b0 THERMAL_NO_LIMIT 
> THERMAL_NO_LIMIT>;
> + contribution = <1024>;
> + };
> + map2 {
> + trip = <>;
> + cooling-device =
> + < THERMAL_NO_LIMIT 
> THERMAL_NO_LIMIT>;
> + contribution = 

Re: [PATCH 4/4] arm64: dts: rockchip: update the thermal zones for RK3399 SoCs

[Heiko Stuebner]

Hi Caesar,

Am Mittwoch, 12. Juli 2017, 14:29:30 CEST schrieb Caesar Wang:
> As RK3399 had used the Power allocator thermal governor by default,
> enabled this to manage thermals by dynamically allocating and limiting
> power to devices.

Does this still run with other thermal governors? The devicetree describes
the hardware, but should not mandate or exclude specific implementations.


> Also, this patch supported the dynamic-power-coefficient/sustainable_power
> and GPU's power model for needed parameters with thermal IPA.

As written below, this doesn't look like a reviewed binding (otherwise
please point me to the binding patch), but even if it is a real binding
it should get its separate patch.


> Signed-off-by: Caesar Wang 
> 
> ---
> 
>  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 62 
> +++-
>  1 file changed, 29 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi 
> b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> index 8c6438b..139f58c 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -147,7 +147,7 @@
>   enable-method = "psci";
>   #cooling-cells = <2>; /* min followed by max */
>   clocks = < ARMCLKB>;
> - dynamic-power-coefficient = <100>;
> + dynamic-power-coefficient = <436>;
>   };
>  
>   cpu_b1: cpu@101 {
> @@ -156,7 +156,7 @@
>   reg = <0x0 0x101>;
>   enable-method = "psci";
>   clocks = < ARMCLKB>;
> - dynamic-power-coefficient = <100>;
> + dynamic-power-coefficient = <436>;
>   };
>   };
>  
> @@ -690,24 +690,25 @@
>   };
>  
>   thermal_zones: thermal-zones {
> - cpu_thermal: cpu {
> + soc_thermal: soc-thermal {
>   polling-delay-passive = <100>;
>   polling-delay = <1000>;
> + sustainable-power = <1000>;
>  
>   thermal-sensors = < 0>;
>  
>   trips {
> - cpu_alert0: cpu_alert0 {
> + threshold: trip-point@0 {

foo@0 will produce warnings when used without reg property. Also,
why all that renaming, the previous names sounded fine to me.


>   temperature = <7>;
>   hysteresis = <2000>;
>   type = "passive";
>   };
> - cpu_alert1: cpu_alert1 {
> - temperature = <75000>;
> + target: trip-point@1 {
> + temperature = <85000>;

When raising the target-temperature to 85 degrees I really
do expect some sort of reassurement in the commit message
why that is really safe - especially when the old limit was 10 degrees
lower.

>   hysteresis = <2000>;
>   type = "passive";
>   };
> - cpu_crit: cpu_crit {
> + soc_crit: soc-crit {
>   temperature = <95000>;
>   hysteresis = <2000>;
>   type = "critical";
> @@ -716,45 +717,31 @@
>  
>   cooling-maps {
>   map0 {
> - trip = <_alert0>;
> + trip = <>;
>   cooling-device =
> - <_b0 THERMAL_NO_LIMIT 
> THERMAL_NO_LIMIT>;
> + <_l0 THERMAL_NO_LIMIT 
> THERMAL_NO_LIMIT>;
> + contribution = <4096>;
>   };
>   map1 {
> - trip = <_alert1>;
> + trip = <>;

Is it correct to use the _same_ trip point all the time? ... what about
the threshold and soc_crit ones?

>   cooling-device =
> - <_l0 THERMAL_NO_LIMIT 
> THERMAL_NO_LIMIT>,
>   <_b0 THERMAL_NO_LIMIT 
> THERMAL_NO_LIMIT>;
> + contribution = <1024>;
> + };
> + map2 {
> + trip = <>;
> + cooling-device =
> + < THERMAL_NO_LIMIT 
> THERMAL_NO_LIMIT>;
> + contribution = <4096>;
> 

Re: [PATCH 4/4] arm64: dts: rockchip: update the thermal zones for RK3399 SoCs

[Brian Norris]

Hi Caesar,

On Wed, Jul 12, 2017 at 02:29:30PM +0800, Caesar Wang wrote:
> As RK3399 had used the Power allocator thermal governor by default,
> enabled this to manage thermals by dynamically allocating and limiting
> power to devices.
> 
> Also, this patch supported the dynamic-power-coefficient/sustainable_power
> and GPU's power model for needed parameters with thermal IPA.
> 
> Signed-off-by: Caesar Wang 
> 
> ---
> 
>  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 62 
> +++-
>  1 file changed, 29 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi 
> b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> index 8c6438b..139f58c 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -147,7 +147,7 @@
>   enable-method = "psci";
>   #cooling-cells = <2>; /* min followed by max */
>   clocks = < ARMCLKB>;
> - dynamic-power-coefficient = <100>;
> + dynamic-power-coefficient = <436>;
>   };
>  
>   cpu_b1: cpu@101 {
> @@ -156,7 +156,7 @@
>   reg = <0x0 0x101>;
>   enable-method = "psci";
>   clocks = < ARMCLKB>;
> - dynamic-power-coefficient = <100>;
> + dynamic-power-coefficient = <436>;

There are 6 of these properties (1 for each core now; not just 1 for
each cluster), and you're only changing 2 of them.

BTW, are these values determined from measurement this time? And heavily
tested? The previous values were suspiciously round, but they'd been
heavily tested so I didn't mind :)

>   };
>   };
>  

...

Brian

Re: [PATCH 4/4] arm64: dts: rockchip: update the thermal zones for RK3399 SoCs

[Brian Norris]

Hi Caesar,

On Wed, Jul 12, 2017 at 02:29:30PM +0800, Caesar Wang wrote:
> As RK3399 had used the Power allocator thermal governor by default,
> enabled this to manage thermals by dynamically allocating and limiting
> power to devices.
> 
> Also, this patch supported the dynamic-power-coefficient/sustainable_power
> and GPU's power model for needed parameters with thermal IPA.
> 
> Signed-off-by: Caesar Wang 
> 
> ---
> 
>  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 62 
> +++-
>  1 file changed, 29 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi 
> b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> index 8c6438b..139f58c 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -147,7 +147,7 @@
>   enable-method = "psci";
>   #cooling-cells = <2>; /* min followed by max */
>   clocks = < ARMCLKB>;
> - dynamic-power-coefficient = <100>;
> + dynamic-power-coefficient = <436>;
>   };
>  
>   cpu_b1: cpu@101 {
> @@ -156,7 +156,7 @@
>   reg = <0x0 0x101>;
>   enable-method = "psci";
>   clocks = < ARMCLKB>;
> - dynamic-power-coefficient = <100>;
> + dynamic-power-coefficient = <436>;

There are 6 of these properties (1 for each core now; not just 1 for
each cluster), and you're only changing 2 of them.

BTW, are these values determined from measurement this time? And heavily
tested? The previous values were suspiciously round, but they'd been
heavily tested so I didn't mind :)

>   };
>   };
>  

...

Brian

[PATCH 4/4] arm64: dts: rockchip: update the thermal zones for RK3399 SoCs

[Caesar Wang]

As RK3399 had used the Power allocator thermal governor by default,
enabled this to manage thermals by dynamically allocating and limiting
power to devices.

Also, this patch supported the dynamic-power-coefficient/sustainable_power
and GPU's power model for needed parameters with thermal IPA.

Signed-off-by: Caesar Wang 

---

 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 62 +++-
 1 file changed, 29 insertions(+), 33 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi 
b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index 8c6438b..139f58c 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -147,7 +147,7 @@
enable-method = "psci";
#cooling-cells = <2>; /* min followed by max */
clocks = < ARMCLKB>;
-   dynamic-power-coefficient = <100>;
+   dynamic-power-coefficient = <436>;
};
 
cpu_b1: cpu@101 {
@@ -156,7 +156,7 @@
reg = <0x0 0x101>;
enable-method = "psci";
clocks = < ARMCLKB>;
-   dynamic-power-coefficient = <100>;
+   dynamic-power-coefficient = <436>;
};
};
 
@@ -690,24 +690,25 @@
};
 
thermal_zones: thermal-zones {
-   cpu_thermal: cpu {
+   soc_thermal: soc-thermal {
polling-delay-passive = <100>;
polling-delay = <1000>;
+   sustainable-power = <1000>;
 
thermal-sensors = < 0>;
 
trips {
-   cpu_alert0: cpu_alert0 {
+   threshold: trip-point@0 {
temperature = <7>;
hysteresis = <2000>;
type = "passive";
};
-   cpu_alert1: cpu_alert1 {
-   temperature = <75000>;
+   target: trip-point@1 {
+   temperature = <85000>;
hysteresis = <2000>;
type = "passive";
};
-   cpu_crit: cpu_crit {
+   soc_crit: soc-crit {
temperature = <95000>;
hysteresis = <2000>;
type = "critical";
@@ -716,45 +717,31 @@
 
cooling-maps {
map0 {
-   trip = <_alert0>;
+   trip = <>;
cooling-device =
-   <_b0 THERMAL_NO_LIMIT 
THERMAL_NO_LIMIT>;
+   <_l0 THERMAL_NO_LIMIT 
THERMAL_NO_LIMIT>;
+   contribution = <4096>;
};
map1 {
-   trip = <_alert1>;
+   trip = <>;
cooling-device =
-   <_l0 THERMAL_NO_LIMIT 
THERMAL_NO_LIMIT>,
<_b0 THERMAL_NO_LIMIT 
THERMAL_NO_LIMIT>;
+   contribution = <1024>;
+   };
+   map2 {
+   trip = <>;
+   cooling-device =
+   < THERMAL_NO_LIMIT 
THERMAL_NO_LIMIT>;
+   contribution = <4096>;
};
};
};
 
-   gpu_thermal: gpu {
+   gpu_thermal: gpu-thermal {
polling-delay-passive = <100>;
polling-delay = <1000>;
 
thermal-sensors = < 1>;
-
-   trips {
-   gpu_alert0: gpu_alert0 {
-   temperature = <75000>;
-   hysteresis = <2000>;
-   type = "passive";
-   };
-   gpu_crit: gpu_crit {
-   temperature = <95000>;
-   hysteresis = <2000>;
-   type = "critical";
-   };
-  

[PATCH 4/4] arm64: dts: rockchip: update the thermal zones for RK3399 SoCs

[Caesar Wang]

As RK3399 had used the Power allocator thermal governor by default,
enabled this to manage thermals by dynamically allocating and limiting
power to devices.

Also, this patch supported the dynamic-power-coefficient/sustainable_power
and GPU's power model for needed parameters with thermal IPA.

Signed-off-by: Caesar Wang 

---

 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 62 +++-
 1 file changed, 29 insertions(+), 33 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi 
b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index 8c6438b..139f58c 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -147,7 +147,7 @@
enable-method = "psci";
#cooling-cells = <2>; /* min followed by max */
clocks = < ARMCLKB>;
-   dynamic-power-coefficient = <100>;
+   dynamic-power-coefficient = <436>;
};
 
cpu_b1: cpu@101 {
@@ -156,7 +156,7 @@
reg = <0x0 0x101>;
enable-method = "psci";
clocks = < ARMCLKB>;
-   dynamic-power-coefficient = <100>;
+   dynamic-power-coefficient = <436>;
};
};
 
@@ -690,24 +690,25 @@
};
 
thermal_zones: thermal-zones {
-   cpu_thermal: cpu {
+   soc_thermal: soc-thermal {
polling-delay-passive = <100>;
polling-delay = <1000>;
+   sustainable-power = <1000>;
 
thermal-sensors = < 0>;
 
trips {
-   cpu_alert0: cpu_alert0 {
+   threshold: trip-point@0 {
temperature = <7>;
hysteresis = <2000>;
type = "passive";
};
-   cpu_alert1: cpu_alert1 {
-   temperature = <75000>;
+   target: trip-point@1 {
+   temperature = <85000>;
hysteresis = <2000>;
type = "passive";
};
-   cpu_crit: cpu_crit {
+   soc_crit: soc-crit {
temperature = <95000>;
hysteresis = <2000>;
type = "critical";
@@ -716,45 +717,31 @@
 
cooling-maps {
map0 {
-   trip = <_alert0>;
+   trip = <>;
cooling-device =
-   <_b0 THERMAL_NO_LIMIT 
THERMAL_NO_LIMIT>;
+   <_l0 THERMAL_NO_LIMIT 
THERMAL_NO_LIMIT>;
+   contribution = <4096>;
};
map1 {
-   trip = <_alert1>;
+   trip = <>;
cooling-device =
-   <_l0 THERMAL_NO_LIMIT 
THERMAL_NO_LIMIT>,
<_b0 THERMAL_NO_LIMIT 
THERMAL_NO_LIMIT>;
+   contribution = <1024>;
+   };
+   map2 {
+   trip = <>;
+   cooling-device =
+   < THERMAL_NO_LIMIT 
THERMAL_NO_LIMIT>;
+   contribution = <4096>;
};
};
};
 
-   gpu_thermal: gpu {
+   gpu_thermal: gpu-thermal {
polling-delay-passive = <100>;
polling-delay = <1000>;
 
thermal-sensors = < 1>;
-
-   trips {
-   gpu_alert0: gpu_alert0 {
-   temperature = <75000>;
-   hysteresis = <2000>;
-   type = "passive";
-   };
-   gpu_crit: gpu_crit {
-   temperature = <95000>;
-   hysteresis = <2000>;
-   type = "critical";
-   };
-   };