Re: [PATCH v5 6/7] arm64: dts: rockchip: Add thermal nodes to RK3576

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

Hi Alexey,

On Thursday, 5 June 2025 21:19:39 Central European Summer Time Alexey Charkov wrote:
> Hi Nicolas,
> 
> On Thu, Jun 5, 2025 at 11:07 PM Nicolas Frattaroli
> <nicolas.frattaroli@collabora.com> wrote:
> >
> > Add the TSADC node to the RK3576. Additionally, add everything the TSADC
> > needs to function, i.e. thermal zones, their trip points and maps, as
> > well as adjust the CPU cooling-cells property.
> >
> > The polling-delay properties are set to 0 as we do have interrupts for
> > this TSADC on this particular SoC.
> >
> > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3576.dtsi | 164 ++++++++++++++++++++++++++++++-
> >  1 file changed, 162 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3576.dtsi b/arch/arm64/boot/dts/rockchip/rk3576.dtsi
> > index a6bfef82d50bc9b0203a04324d61e0f232b61a65..1c07ad78c9230f1e46b0ef8817834f58b19eb86b 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3576.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3576.dtsi
> > @@ -11,6 +11,7 @@
> >  #include <dt-bindings/power/rockchip,rk3576-power.h>
> >  #include <dt-bindings/reset/rockchip,rk3576-cru.h>
> >  #include <dt-bindings/soc/rockchip,boot-mode.h>
> > +#include <dt-bindings/thermal/thermal.h>
> >
> >  / {
> >         compatible = "rockchip,rk3576";
> > @@ -113,9 +114,9 @@ cpu_l0: cpu@0 {
> >                         capacity-dmips-mhz = <485>;
> >                         clocks = <&scmi_clk SCMI_ARMCLK_L>;
> >                         operating-points-v2 = <&cluster0_opp_table>;
> > -                       #cooling-cells = <2>;
> >                         dynamic-power-coefficient = <120>;
> >                         cpu-idle-states = <&CPU_SLEEP>;
> > +                       #cooling-cells = <2>;
> >                 };
> >
> >                 cpu_l1: cpu@1 {
> > @@ -127,6 +128,7 @@ cpu_l1: cpu@1 {
> >                         clocks = <&scmi_clk SCMI_ARMCLK_L>;
> >                         operating-points-v2 = <&cluster0_opp_table>;
> >                         cpu-idle-states = <&CPU_SLEEP>;
> > +                       #cooling-cells = <2>;
> >                 };
> >
> >                 cpu_l2: cpu@2 {
> > @@ -138,6 +140,7 @@ cpu_l2: cpu@2 {
> >                         clocks = <&scmi_clk SCMI_ARMCLK_L>;
> >                         operating-points-v2 = <&cluster0_opp_table>;
> >                         cpu-idle-states = <&CPU_SLEEP>;
> > +                       #cooling-cells = <2>;
> >                 };
> >
> >                 cpu_l3: cpu@3 {
> > @@ -149,6 +152,7 @@ cpu_l3: cpu@3 {
> >                         clocks = <&scmi_clk SCMI_ARMCLK_L>;
> >                         operating-points-v2 = <&cluster0_opp_table>;
> >                         cpu-idle-states = <&CPU_SLEEP>;
> > +                       #cooling-cells = <2>;
> >                 };
> >
> >                 cpu_b0: cpu@100 {
> > @@ -159,9 +163,9 @@ cpu_b0: cpu@100 {
> >                         capacity-dmips-mhz = <1024>;
> >                         clocks = <&scmi_clk SCMI_ARMCLK_B>;
> >                         operating-points-v2 = <&cluster1_opp_table>;
> > -                       #cooling-cells = <2>;
> >                         dynamic-power-coefficient = <320>;
> >                         cpu-idle-states = <&CPU_SLEEP>;
> > +                       #cooling-cells = <2>;
> >                 };
> >
> >                 cpu_b1: cpu@101 {
> > @@ -173,6 +177,7 @@ cpu_b1: cpu@101 {
> >                         clocks = <&scmi_clk SCMI_ARMCLK_B>;
> >                         operating-points-v2 = <&cluster1_opp_table>;
> >                         cpu-idle-states = <&CPU_SLEEP>;
> > +                       #cooling-cells = <2>;
> >                 };
> >
> >                 cpu_b2: cpu@102 {
> > @@ -184,6 +189,7 @@ cpu_b2: cpu@102 {
> >                         clocks = <&scmi_clk SCMI_ARMCLK_B>;
> >                         operating-points-v2 = <&cluster1_opp_table>;
> >                         cpu-idle-states = <&CPU_SLEEP>;
> > +                       #cooling-cells = <2>;
> >                 };
> >
> >                 cpu_b3: cpu@103 {
> > @@ -195,6 +201,7 @@ cpu_b3: cpu@103 {
> >                         clocks = <&scmi_clk SCMI_ARMCLK_B>;
> >                         operating-points-v2 = <&cluster1_opp_table>;
> >                         cpu-idle-states = <&CPU_SLEEP>;
> > +                       #cooling-cells = <2>;
> >                 };
> >
> >                 idle-states {
> > @@ -436,6 +443,143 @@ psci {
> >                 method = "smc";
> >         };
> >
> > +       thermal_zones: thermal-zones {
> > +               /* sensor near the center of the SoC */
> > +               package_thermal: package-thermal {
> > +                       polling-delay-passive = <0>;
> > +                       polling-delay = <0>;
> > +                       thermal-sensors = <&tsadc 0>;
> > +
> > +                       trips {
> > +                               package_crit: package-crit {
> > +                                       temperature = <115000>;
> > +                                       hysteresis = <0>;
> > +                                       type = "critical";
> > +                               };
> > +                       };
> > +               };
> > +
> > +               /* sensor for cluster1 (big Cortex-A72 cores) */
> > +               bigcore_thermal: bigcore-thermal {
> > +                       polling-delay-passive = <0>;
> 
> I've tried these on my board, and it seems that with a zero here it
> never stops throttling the CPU even after it cools down. I believe you
> need something like <100> here, which is what I used on RK3588 for
> similar reasons.
> 
> I think it's because the TSADC only fires an interrupt when the
> temperature crosses the trip point, but the thermal governor also
> needs to observe temperature trends and step up / step down the
> cooling states depending on whether the system is cooling sufficiently
> or not. So it needs to poll the temperature once the cooling device is
> activated (passive in this case).

Thanks, good catch. I struggled to make the CPU throttle at all in my
case, so I never managed to catch this.

I'll fix it in v6, which I'll send out next week based on v6.16-rc1.

> 
> > +                       polling-delay = <0>;
> > +                       thermal-sensors = <&tsadc 1>;
> > +
> > +                       trips {
> > +                               bigcore_alert: bigcore-alert {
> > +                                       temperature = <85000>;
> > +                                       hysteresis = <2000>;
> > +                                       type = "passive";
> > +                               };
> > +
> > +                               bigcore_crit: bigcore-crit {
> > +                                       temperature = <115000>;
> > +                                       hysteresis = <0>;
> > +                                       type = "critical";
> > +                               };
> > +                       };
> > +
> > +                       cooling-maps {
> > +                               map0 {
> > +                                       trip = <&bigcore_alert>;
> > +                                       cooling-device =
> > +                                               <&cpu_b0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > +                                               <&cpu_b1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > +                                               <&cpu_b2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > +                                               <&cpu_b3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> > +                               };
> > +                       };
> > +               };
> > +
> > +               /* sensor for cluster0 (little Cortex-A53 cores) */
> > +               littlecore_thermal: littlecore-thermal {
> > +                       polling-delay-passive = <0>;
> 
> polling-delay-passive = <100>;

Will change as well, thank you

> 
> > +                       polling-delay = <0>;
> > +                       thermal-sensors = <&tsadc 2>;
> > +
> > +                       trips {
> > +                               littlecore_alert: littlecore-alert {
> > +                                       temperature = <85000>;
> > +                                       hysteresis = <2000>;
> > +                                       type = "passive";
> > +                               };
> > +
> > +                               littlecore_crit: littlecore-crit {
> > +                                       temperature = <115000>;
> > +                                       hysteresis = <0>;
> > +                                       type = "critical";
> > +                               };
> > +                       };
> > +
> > +                       cooling-maps {
> > +                               map0 {
> > +                                       trip = <&littlecore_alert>;
> > +                                       cooling-device =
> > +                                               <&cpu_l0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > +                                               <&cpu_l1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > +                                               <&cpu_l2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > +                                               <&cpu_l3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> > +                               };
> > +                       };
> > +               };
> > +
> > +               gpu_thermal: gpu-thermal {
> > +                       polling-delay-passive = <0>;
> 
> polling-delay-passive = <100>;

Will change as well, thank you

> 
> Best regards,
> Alexey
> 

Best regards,
Nicolas Frattaroli