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

Nicolas Frattaroli
2025-06-06T08:31:44Z

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