[PATCH v2 0/4] RK3588 and Rock 5B dts additions: thermal, OPP and fan

[PATCH v2 0/4] RK3588 and Rock 5B dts additions: thermal, OPP and fan

[Alexey Charkov]

This is an assortment of device tree additions for RK3588(s) and their
enablement on Radxa Rock 5B.

Thermal zone information and cooling maps is the follow-up to feedback
received on v3 patch version [1] - thanks a lot to Daniel for continued
review of these!
Changes in v4:
 - Set higher 'polling-delay-passive' (100 instead of 20)
 - Name all cooling maps starting from map0 in each respective zone
 - Drop 'contribution' properties from passive cooling maps

Fan control on Rock 5B has been split into two intervals: let it spin
at the minimum cooling state between 55C and 65C, and then accelerate
if the system crosses the 65C mark - thanks to Dragan for suggesting.
This lets some cooling setups with beefier heatsinks and/or larger
fan fins to stay in the quietest non-zero fan state while still
gaining potential benefits from the airflow it generates, and
possibly avoiding noisy speeds altogether for some workloads.

OPPs help actually scale CPU frequencies up and down for both cooling
and performance - tested on Rock 5B under varied loads. I've split
the patch into two parts: the first containing those OPPs that seem
to be no-regret with general consensus during v1 review [2], while
the second contains OPPs that cause frequency reductions without
accompanying decrease in CPU voltage. There seems to be a slight
performance gain in some workload scenarios when using these, but
previous discussion was inconclusive as to whether they should be
included or not. Having them as separate patches enables easier
comparison and partial reversion if people want to test it under
their workloads, and also enables the first 'no-regret' part to be
merged to -next while the jury is still out on the second one.

[1] https://lore.kernel.org/linux-rockchip/1824717.EqSB1tO5pr@bagend/T/#ma2ab949da2235a8e759eab22155fb2bc397d8aea
[2] https://lore.kernel.org/linux-rockchip/CABjd4YxqarUCbZ-a2XLe3TWJ-qjphGkyq=wDnctnEhdoSdPPpw@mail.gmail.com/T/#m49d2b94e773f5b532a0bb5d3d7664799ff28cc2c

Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
Changes in v2:
- Dropped the rfkill patch which Heiko has already applied
- Incorporate feedback received on the thermal and OPP code (see above)
- Link to v1: https://lore.kernel.org/r/20240125-rk-dts-additions-v1-0-5879275db36f@gmail.com

---
Alexey Charkov (4):
      arm64: dts: rockchip: enable built-in thermal monitoring on rk3588
      arm64: dts: rockchip: enable temperature driven fan control on Rock 5B
      arm64: dts: rockchip: Add OPP data for CPU cores on RK3588
      arm64: dts: rockchip: Add further granularity in RK3588 CPU OPPs

 arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts |  34 ++-
 arch/arm64/boot/dts/rockchip/rk3588s.dtsi       | 371 ++++++++++++++++++++++++
 2 files changed, 404 insertions(+), 1 deletion(-)
---
base-commit: 8a696a29c6905594e4abf78eaafcb62165ac61f1
change-id: 20240124-rk-dts-additions-a6d7b52787b9

Best regards,
-- 
Alexey Charkov <alchark@gmail.com>

[PATCH v2 1/4] arm64: dts: rockchip: enable built-in thermal monitoring on rk3588

[Alexey Charkov]

Include thermal zones information in device tree for rk3588 variants

Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
 arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 162 ++++++++++++++++++++++++++++++
 1 file changed, 162 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
index 36b1b7acfe6a..696cb72d75d0 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
@@ -10,6 +10,7 @@
 #include <dt-bindings/reset/rockchip,rk3588-cru.h>
 #include <dt-bindings/phy/phy.h>
 #include <dt-bindings/ata/ahci.h>
+#include <dt-bindings/thermal/thermal.h>
 
 / {
 	compatible = "rockchip,rk3588";
@@ -2228,6 +2229,167 @@ tsadc: tsadc@fec00000 {
 		status = "disabled";
 	};
 
+	thermal_zones: thermal-zones {
+		/* sensor near the center of the whole chip */
+		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 between A76 cores 0 and 1 */
+		bigcore0_thermal: bigcore0-thermal {
+			polling-delay-passive = <100>;
+			polling-delay = <0>;
+			thermal-sensors = <&tsadc 1>;
+
+			trips {
+				bigcore0_alert0: bigcore0-alert0 {
+					temperature = <75000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+				bigcore0_alert1: bigcore0-alert1 {
+					temperature = <85000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+				bigcore0_crit: bigcore0-crit {
+					temperature = <115000>;
+					hysteresis = <0>;
+					type = "critical";
+				};
+			};
+			cooling-maps {
+				map0 {
+					trip = <&bigcore0_alert1>;
+					cooling-device =
+						<&cpu_b0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+						<&cpu_b1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+			};
+		};
+
+		/* sensor between A76 cores 2 and 3 */
+		bigcore2_thermal: bigcore2-thermal {
+			polling-delay-passive = <100>;
+			polling-delay = <0>;
+			thermal-sensors = <&tsadc 2>;
+
+			trips {
+				bigcore2_alert0: bigcore2-alert0 {
+					temperature = <75000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+				bigcore2_alert1: bigcore2-alert1 {
+					temperature = <85000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+				bigcore2_crit: bigcore2-crit {
+					temperature = <115000>;
+					hysteresis = <0>;
+					type = "critical";
+				};
+			};
+			cooling-maps {
+				map0 {
+					trip = <&bigcore2_alert1>;
+					cooling-device =
+						<&cpu_b2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
+						<&cpu_b3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+			};
+		};
+
+		/* sensor between the four A55 cores */
+		little_core_thermal: littlecore-thermal {
+			polling-delay-passive = <100>;
+			polling-delay = <0>;
+			thermal-sensors = <&tsadc 3>;
+
+			trips {
+				littlecore_alert0: littlecore-alert0 {
+					temperature = <75000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+				littlecore_alert1: littlecore-alert1 {
+					temperature = <85000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+				littlecore_crit: littlecore-crit {
+					temperature = <115000>;
+					hysteresis = <0>;
+					type = "critical";
+				};
+			};
+			cooling-maps {
+				map0 {
+					trip = <&littlecore_alert1>;
+					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>;
+				};
+			};
+		};
+
+		/* sensor near the PD_CENTER power domain */
+		center_thermal: center-thermal {
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+			thermal-sensors = <&tsadc 4>;
+
+			trips {
+				center_crit: center-crit {
+					temperature = <115000>;
+					hysteresis = <0>;
+					type = "critical";
+				};
+			};
+		};
+
+		gpu_thermal: gpu-thermal {
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+			thermal-sensors = <&tsadc 5>;
+
+			trips {
+				gpu_crit: gpu-crit {
+					temperature = <115000>;
+					hysteresis = <0>;
+					type = "critical";
+				};
+			};
+		};
+
+		npu_thermal: npu-thermal {
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+			thermal-sensors = <&tsadc 6>;
+
+			trips {
+				npu_crit: npu-crit {
+					temperature = <115000>;
+					hysteresis = <0>;
+					type = "critical";
+				};
+			};
+		};
+	};
+
 	saradc: adc@fec10000 {
 		compatible = "rockchip,rk3588-saradc";
 		reg = <0x0 0xfec10000 0x0 0x10000>;

-- 
2.43.0

Re: [PATCH v2 1/4] arm64: dts: rockchip: enable built-in thermal monitoring on rk3588

[Dragan Simic]

Hello Alexey,

Some small nitpicks below, please have a look.

On 2024-01-30 19:21, Alexey Charkov wrote:
> Include thermal zones information in device tree for rk3588 variants

Please use "RK3588" instead of "rk3588", both here and in the
patch subject.  Looks much better.

> Signed-off-by: Alexey Charkov <alchark@gmail.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 162 
> ++++++++++++++++++++++++++++++
>  1 file changed, 162 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> index 36b1b7acfe6a..696cb72d75d0 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> @@ -10,6 +10,7 @@
>  #include <dt-bindings/reset/rockchip,rk3588-cru.h>
>  #include <dt-bindings/phy/phy.h>
>  #include <dt-bindings/ata/ahci.h>
> +#include <dt-bindings/thermal/thermal.h>
> 
>  / {
>  	compatible = "rockchip,rk3588";
> @@ -2228,6 +2229,167 @@ tsadc: tsadc@fec00000 {
>  		status = "disabled";
>  	};
> 
> +	thermal_zones: thermal-zones {
> +		/* sensor near the center of the whole chip */

It would be good to replace "whole chip" with "SoC".  Simpler and
IIRC closer to the official description of the sensor.

> +		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 between A76 cores 0 and 1 */
> +		bigcore0_thermal: bigcore0-thermal {
> +			polling-delay-passive = <100>;
> +			polling-delay = <0>;
> +			thermal-sensors = <&tsadc 1>;
> +
> +			trips {

Please add the following comment here, to make it clear what's
the purpose of this thermal trip when the IPA thermal governor
is used (more similar comments below):

                                 /* IPA threshold */

> +				bigcore0_alert0: bigcore0-alert0 {
> +					temperature = <75000>;
> +					hysteresis = <2000>;
> +					type = "passive";
> +				};

Please add the following comment here:

                                 /* IPA target */

> +				bigcore0_alert1: bigcore0-alert1 {
> +					temperature = <85000>;
> +					hysteresis = <2000>;
> +					type = "passive";
> +				};
> +				bigcore0_crit: bigcore0-crit {
> +					temperature = <115000>;
> +					hysteresis = <0>;
> +					type = "critical";
> +				};
> +			};
> +			cooling-maps {
> +				map0 {
> +					trip = <&bigcore0_alert1>;
> +					cooling-device =
> +						<&cpu_b0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +						<&cpu_b1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> +				};
> +			};
> +		};
> +
> +		/* sensor between A76 cores 2 and 3 */
> +		bigcore2_thermal: bigcore2-thermal {
> +			polling-delay-passive = <100>;
> +			polling-delay = <0>;
> +			thermal-sensors = <&tsadc 2>;
> +
> +			trips {

Please add the following comment here:

                                 /* IPA threshold */

> +				bigcore2_alert0: bigcore2-alert0 {
> +					temperature = <75000>;
> +					hysteresis = <2000>;
> +					type = "passive";
> +				};

Please add the following comment here:

                                 /* IPA target */

> +				bigcore2_alert1: bigcore2-alert1 {
> +					temperature = <85000>;
> +					hysteresis = <2000>;
> +					type = "passive";
> +				};
> +				bigcore2_crit: bigcore2-crit {
> +					temperature = <115000>;
> +					hysteresis = <0>;
> +					type = "critical";
> +				};
> +			};
> +			cooling-maps {
> +				map0 {
> +					trip = <&bigcore2_alert1>;
> +					cooling-device =
> +						<&cpu_b2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +						<&cpu_b3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> +				};
> +			};
> +		};
> +
> +		/* sensor between the four A55 cores */
> +		little_core_thermal: littlecore-thermal {
> +			polling-delay-passive = <100>;
> +			polling-delay = <0>;
> +			thermal-sensors = <&tsadc 3>;
> +
> +			trips {

Please add the following comment here:

                                 /* IPA threshold */

> +				littlecore_alert0: littlecore-alert0 {
> +					temperature = <75000>;
> +					hysteresis = <2000>;
> +					type = "passive";
> +				};

Please add the following comment here:

                                 /* IPA target */

> +				littlecore_alert1: littlecore-alert1 {
> +					temperature = <85000>;
> +					hysteresis = <2000>;
> +					type = "passive";
> +				};
> +				littlecore_crit: littlecore-crit {
> +					temperature = <115000>;
> +					hysteresis = <0>;
> +					type = "critical";
> +				};
> +			};
> +			cooling-maps {
> +				map0 {
> +					trip = <&littlecore_alert1>;
> +					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>;
> +				};
> +			};
> +		};
> +
> +		/* sensor near the PD_CENTER power domain */
> +		center_thermal: center-thermal {
> +			polling-delay-passive = <0>;
> +			polling-delay = <0>;
> +			thermal-sensors = <&tsadc 4>;
> +
> +			trips {
> +				center_crit: center-crit {
> +					temperature = <115000>;
> +					hysteresis = <0>;
> +					type = "critical";
> +				};
> +			};
> +		};
> +
> +		gpu_thermal: gpu-thermal {
> +			polling-delay-passive = <0>;
> +			polling-delay = <0>;
> +			thermal-sensors = <&tsadc 5>;
> +
> +			trips {
> +				gpu_crit: gpu-crit {
> +					temperature = <115000>;
> +					hysteresis = <0>;
> +					type = "critical";
> +				};
> +			};
> +		};
> +
> +		npu_thermal: npu-thermal {
> +			polling-delay-passive = <0>;
> +			polling-delay = <0>;
> +			thermal-sensors = <&tsadc 6>;
> +
> +			trips {
> +				npu_crit: npu-crit {
> +					temperature = <115000>;
> +					hysteresis = <0>;
> +					type = "critical";
> +				};
> +			};
> +		};
> +	};
> +
>  	saradc: adc@fec10000 {
>  		compatible = "rockchip,rk3588-saradc";
>  		reg = <0x0 0xfec10000 0x0 0x10000>;

Re: [PATCH v2 1/4] arm64: dts: rockchip: enable built-in thermal monitoring on rk3588

[Alexey Charkov]

On Wed, Jan 31, 2024 at 9:05 AM Dragan Simic <dsimic@manjaro.org> wrote:
>
> Hello Alexey,
>
> Some small nitpicks below, please have a look.
>
> On 2024-01-30 19:21, Alexey Charkov wrote:
> > Include thermal zones information in device tree for rk3588 variants
>
> Please use "RK3588" instead of "rk3588", both here and in the
> patch subject.  Looks much better.

Noted, thanks!

> > Signed-off-by: Alexey Charkov <alchark@gmail.com>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 162
> > ++++++++++++++++++++++++++++++
> >  1 file changed, 162 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > index 36b1b7acfe6a..696cb72d75d0 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > @@ -10,6 +10,7 @@
> >  #include <dt-bindings/reset/rockchip,rk3588-cru.h>
> >  #include <dt-bindings/phy/phy.h>
> >  #include <dt-bindings/ata/ahci.h>
> > +#include <dt-bindings/thermal/thermal.h>
> >
> >  / {
> >       compatible = "rockchip,rk3588";
> > @@ -2228,6 +2229,167 @@ tsadc: tsadc@fec00000 {
> >               status = "disabled";
> >       };
> >
> > +     thermal_zones: thermal-zones {
> > +             /* sensor near the center of the whole chip */
>
> It would be good to replace "whole chip" with "SoC".  Simpler and
> IIRC closer to the official description of the sensor.

The TRM only says "near chip center" (sic) :)

> > +             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 between A76 cores 0 and 1 */
> > +             bigcore0_thermal: bigcore0-thermal {
> > +                     polling-delay-passive = <100>;
> > +                     polling-delay = <0>;
> > +                     thermal-sensors = <&tsadc 1>;
> > +
> > +                     trips {
>
> Please add the following comment here, to make it clear what's
> the purpose of this thermal trip when the IPA thermal governor
> is used (more similar comments below):
>
>                                  /* IPA threshold */

Not so sure about this one: shouldn't the device tree be
implementation agnostic, and just describe the hardware and its
expectations? Sounds like references to a particular thermal governor
would be out of scope.

Best regards,
Alexey

> > +                             bigcore0_alert0: bigcore0-alert0 {
> > +                                     temperature = <75000>;
> > +                                     hysteresis = <2000>;
> > +                                     type = "passive";
> > +                             };
>
> Please add the following comment here:
>
>                                  /* IPA target */
>
> > +                             bigcore0_alert1: bigcore0-alert1 {
> > +                                     temperature = <85000>;
> > +                                     hysteresis = <2000>;
> > +                                     type = "passive";
> > +                             };
> > +                             bigcore0_crit: bigcore0-crit {
> > +                                     temperature = <115000>;
> > +                                     hysteresis = <0>;
> > +                                     type = "critical";
> > +                             };
> > +                     };
> > +                     cooling-maps {
> > +                             map0 {
> > +                                     trip = <&bigcore0_alert1>;
> > +                                     cooling-device =
> > +                                             <&cpu_b0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > +                                             <&cpu_b1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> > +                             };
> > +                     };
> > +             };
> > +
> > +             /* sensor between A76 cores 2 and 3 */
> > +             bigcore2_thermal: bigcore2-thermal {
> > +                     polling-delay-passive = <100>;
> > +                     polling-delay = <0>;
> > +                     thermal-sensors = <&tsadc 2>;
> > +
> > +                     trips {
>
> Please add the following comment here:
>
>                                  /* IPA threshold */
>
> > +                             bigcore2_alert0: bigcore2-alert0 {
> > +                                     temperature = <75000>;
> > +                                     hysteresis = <2000>;
> > +                                     type = "passive";
> > +                             };
>
> Please add the following comment here:
>
>                                  /* IPA target */
>
> > +                             bigcore2_alert1: bigcore2-alert1 {
> > +                                     temperature = <85000>;
> > +                                     hysteresis = <2000>;
> > +                                     type = "passive";
> > +                             };
> > +                             bigcore2_crit: bigcore2-crit {
> > +                                     temperature = <115000>;
> > +                                     hysteresis = <0>;
> > +                                     type = "critical";
> > +                             };
> > +                     };
> > +                     cooling-maps {
> > +                             map0 {
> > +                                     trip = <&bigcore2_alert1>;
> > +                                     cooling-device =
> > +                                             <&cpu_b2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> > +                                             <&cpu_b3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> > +                             };
> > +                     };
> > +             };
> > +
> > +             /* sensor between the four A55 cores */
> > +             little_core_thermal: littlecore-thermal {
> > +                     polling-delay-passive = <100>;
> > +                     polling-delay = <0>;
> > +                     thermal-sensors = <&tsadc 3>;
> > +
> > +                     trips {
>
> Please add the following comment here:
>
>                                  /* IPA threshold */
>
> > +                             littlecore_alert0: littlecore-alert0 {
> > +                                     temperature = <75000>;
> > +                                     hysteresis = <2000>;
> > +                                     type = "passive";
> > +                             };
>
> Please add the following comment here:
>
>                                  /* IPA target */
>
> > +                             littlecore_alert1: littlecore-alert1 {
> > +                                     temperature = <85000>;
> > +                                     hysteresis = <2000>;
> > +                                     type = "passive";
> > +                             };
> > +                             littlecore_crit: littlecore-crit {
> > +                                     temperature = <115000>;
> > +                                     hysteresis = <0>;
> > +                                     type = "critical";
> > +                             };
> > +                     };
> > +                     cooling-maps {
> > +                             map0 {
> > +                                     trip = <&littlecore_alert1>;
> > +                                     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>;
> > +                             };
> > +                     };
> > +             };
> > +
> > +             /* sensor near the PD_CENTER power domain */
> > +             center_thermal: center-thermal {
> > +                     polling-delay-passive = <0>;
> > +                     polling-delay = <0>;
> > +                     thermal-sensors = <&tsadc 4>;
> > +
> > +                     trips {
> > +                             center_crit: center-crit {
> > +                                     temperature = <115000>;
> > +                                     hysteresis = <0>;
> > +                                     type = "critical";
> > +                             };
> > +                     };
> > +             };
> > +
> > +             gpu_thermal: gpu-thermal {
> > +                     polling-delay-passive = <0>;
> > +                     polling-delay = <0>;
> > +                     thermal-sensors = <&tsadc 5>;
> > +
> > +                     trips {
> > +                             gpu_crit: gpu-crit {
> > +                                     temperature = <115000>;
> > +                                     hysteresis = <0>;
> > +                                     type = "critical";
> > +                             };
> > +                     };
> > +             };
> > +
> > +             npu_thermal: npu-thermal {
> > +                     polling-delay-passive = <0>;
> > +                     polling-delay = <0>;
> > +                     thermal-sensors = <&tsadc 6>;
> > +
> > +                     trips {
> > +                             npu_crit: npu-crit {
> > +                                     temperature = <115000>;
> > +                                     hysteresis = <0>;
> > +                                     type = "critical";
> > +                             };
> > +                     };
> > +             };
> > +     };
> > +
> >       saradc: adc@fec10000 {
> >               compatible = "rockchip,rk3588-saradc";
> >               reg = <0x0 0xfec10000 0x0 0x10000>;

Re: [PATCH v2 1/4] arm64: dts: rockchip: enable built-in thermal monitoring on rk3588

[Dragan Simic]

On 2024-01-31 10:56, Alexey Charkov wrote:
> On Wed, Jan 31, 2024 at 9:05 AM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> Some small nitpicks below, please have a look.
>> 
>> On 2024-01-30 19:21, Alexey Charkov wrote:
>> > Include thermal zones information in device tree for rk3588 variants
>> 
>> Please use "RK3588" instead of "rk3588", both here and in the
>> patch subject.  Looks much better.
> 
> Noted, thanks!
> 
>> > Signed-off-by: Alexey Charkov <alchark@gmail.com>
>> > ---
>> >  arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 162
>> > ++++++++++++++++++++++++++++++
>> >  1 file changed, 162 insertions(+)
>> >
>> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>> > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>> > index 36b1b7acfe6a..696cb72d75d0 100644
>> > --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>> > +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>> > @@ -10,6 +10,7 @@
>> >  #include <dt-bindings/reset/rockchip,rk3588-cru.h>
>> >  #include <dt-bindings/phy/phy.h>
>> >  #include <dt-bindings/ata/ahci.h>
>> > +#include <dt-bindings/thermal/thermal.h>
>> >
>> >  / {
>> >       compatible = "rockchip,rk3588";
>> > @@ -2228,6 +2229,167 @@ tsadc: tsadc@fec00000 {
>> >               status = "disabled";
>> >       };
>> >
>> > +     thermal_zones: thermal-zones {
>> > +             /* sensor near the center of the whole chip */
>> 
>> It would be good to replace "whole chip" with "SoC".  Simpler and
>> IIRC closer to the official description of the sensor.
> 
> The TRM only says "near chip center" (sic) :)

In that case, it would be good to just replace "whole" with
"entire" in the original wording. :)

>> > +             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 between A76 cores 0 and 1 */
>> > +             bigcore0_thermal: bigcore0-thermal {
>> > +                     polling-delay-passive = <100>;
>> > +                     polling-delay = <0>;
>> > +                     thermal-sensors = <&tsadc 1>;
>> > +
>> > +                     trips {
>> 
>> Please add the following comment here, to make it clear what's
>> the purpose of this thermal trip when the IPA thermal governor
>> is used (more similar comments below):
>> 
>>                                  /* IPA threshold */
> 
> Not so sure about this one: shouldn't the device tree be
> implementation agnostic, and just describe the hardware and its
> expectations? Sounds like references to a particular thermal governor
> would be out of scope.

I'd agree on that, but having two passive thermal trips is already
kinda out of place, because only one is actually needed, so we should
describe the reason why there are multiples of pairs.

I have some plans for getting this resolved in a way that's much
more governor-agnostic, but it will take some time.  In the meantime,
having the comments can only help anyone reading the dtsi file to
understand the need for additional thermal trips.

I hope you agree.

>> > +                             bigcore0_alert0: bigcore0-alert0 {
>> > +                                     temperature = <75000>;
>> > +                                     hysteresis = <2000>;
>> > +                                     type = "passive";
>> > +                             };
>> 
>> Please add the following comment here:
>> 
>>                                  /* IPA target */
>> 
>> > +                             bigcore0_alert1: bigcore0-alert1 {
>> > +                                     temperature = <85000>;
>> > +                                     hysteresis = <2000>;
>> > +                                     type = "passive";
>> > +                             };
>> > +                             bigcore0_crit: bigcore0-crit {
>> > +                                     temperature = <115000>;
>> > +                                     hysteresis = <0>;
>> > +                                     type = "critical";
>> > +                             };
>> > +                     };
>> > +                     cooling-maps {
>> > +                             map0 {
>> > +                                     trip = <&bigcore0_alert1>;
>> > +                                     cooling-device =
>> > +                                             <&cpu_b0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
>> > +                                             <&cpu_b1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
>> > +                             };
>> > +                     };
>> > +             };
>> > +
>> > +             /* sensor between A76 cores 2 and 3 */
>> > +             bigcore2_thermal: bigcore2-thermal {
>> > +                     polling-delay-passive = <100>;
>> > +                     polling-delay = <0>;
>> > +                     thermal-sensors = <&tsadc 2>;
>> > +
>> > +                     trips {
>> 
>> Please add the following comment here:
>> 
>>                                  /* IPA threshold */
>> 
>> > +                             bigcore2_alert0: bigcore2-alert0 {
>> > +                                     temperature = <75000>;
>> > +                                     hysteresis = <2000>;
>> > +                                     type = "passive";
>> > +                             };
>> 
>> Please add the following comment here:
>> 
>>                                  /* IPA target */
>> 
>> > +                             bigcore2_alert1: bigcore2-alert1 {
>> > +                                     temperature = <85000>;
>> > +                                     hysteresis = <2000>;
>> > +                                     type = "passive";
>> > +                             };
>> > +                             bigcore2_crit: bigcore2-crit {
>> > +                                     temperature = <115000>;
>> > +                                     hysteresis = <0>;
>> > +                                     type = "critical";
>> > +                             };
>> > +                     };
>> > +                     cooling-maps {
>> > +                             map0 {
>> > +                                     trip = <&bigcore2_alert1>;
>> > +                                     cooling-device =
>> > +                                             <&cpu_b2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
>> > +                                             <&cpu_b3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
>> > +                             };
>> > +                     };
>> > +             };
>> > +
>> > +             /* sensor between the four A55 cores */
>> > +             little_core_thermal: littlecore-thermal {
>> > +                     polling-delay-passive = <100>;
>> > +                     polling-delay = <0>;
>> > +                     thermal-sensors = <&tsadc 3>;
>> > +
>> > +                     trips {
>> 
>> Please add the following comment here:
>> 
>>                                  /* IPA threshold */
>> 
>> > +                             littlecore_alert0: littlecore-alert0 {
>> > +                                     temperature = <75000>;
>> > +                                     hysteresis = <2000>;
>> > +                                     type = "passive";
>> > +                             };
>> 
>> Please add the following comment here:
>> 
>>                                  /* IPA target */
>> 
>> > +                             littlecore_alert1: littlecore-alert1 {
>> > +                                     temperature = <85000>;
>> > +                                     hysteresis = <2000>;
>> > +                                     type = "passive";
>> > +                             };
>> > +                             littlecore_crit: littlecore-crit {
>> > +                                     temperature = <115000>;
>> > +                                     hysteresis = <0>;
>> > +                                     type = "critical";
>> > +                             };
>> > +                     };
>> > +                     cooling-maps {
>> > +                             map0 {
>> > +                                     trip = <&littlecore_alert1>;
>> > +                                     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>;
>> > +                             };
>> > +                     };
>> > +             };
>> > +
>> > +             /* sensor near the PD_CENTER power domain */
>> > +             center_thermal: center-thermal {
>> > +                     polling-delay-passive = <0>;
>> > +                     polling-delay = <0>;
>> > +                     thermal-sensors = <&tsadc 4>;
>> > +
>> > +                     trips {
>> > +                             center_crit: center-crit {
>> > +                                     temperature = <115000>;
>> > +                                     hysteresis = <0>;
>> > +                                     type = "critical";
>> > +                             };
>> > +                     };
>> > +             };
>> > +
>> > +             gpu_thermal: gpu-thermal {
>> > +                     polling-delay-passive = <0>;
>> > +                     polling-delay = <0>;
>> > +                     thermal-sensors = <&tsadc 5>;
>> > +
>> > +                     trips {
>> > +                             gpu_crit: gpu-crit {
>> > +                                     temperature = <115000>;
>> > +                                     hysteresis = <0>;
>> > +                                     type = "critical";
>> > +                             };
>> > +                     };
>> > +             };
>> > +
>> > +             npu_thermal: npu-thermal {
>> > +                     polling-delay-passive = <0>;
>> > +                     polling-delay = <0>;
>> > +                     thermal-sensors = <&tsadc 6>;
>> > +
>> > +                     trips {
>> > +                             npu_crit: npu-crit {
>> > +                                     temperature = <115000>;
>> > +                                     hysteresis = <0>;
>> > +                                     type = "critical";
>> > +                             };
>> > +                     };
>> > +             };
>> > +     };
>> > +
>> >       saradc: adc@fec10000 {
>> >               compatible = "rockchip,rk3588-saradc";
>> >               reg = <0x0 0xfec10000 0x0 0x10000>;

[PATCH v2 2/4] arm64: dts: rockchip: enable temperature driven fan control on Rock 5B

[Alexey Charkov]

This enables thermal monitoring on Radxa Rock 5B and links the PWM
fan as an active cooling device managed automatically by the thermal
subsystem, with a target SoC temperature of 65C and a minimum-spin
interval from 55C to 65C to ensure airflow when the system gets warm

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
 arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 34 ++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
index a0e303c3a1dc..b485edeef876 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
@@ -52,7 +52,7 @@ led_rgb_b {
 
 	fan: pwm-fan {
 		compatible = "pwm-fan";
-		cooling-levels = <0 95 145 195 255>;
+		cooling-levels = <0 120 150 180 210 240 255>;
 		fan-supply = <&vcc5v0_sys>;
 		pwms = <&pwm1 0 50000 0>;
 		#cooling-cells = <2>;
@@ -173,6 +173,34 @@ &cpu_l3 {
 	cpu-supply = <&vdd_cpu_lit_s0>;
 };
 
+&package_thermal {
+	polling-delay = <1000>;
+
+	trips {
+		package_fan0: package-fan0 {
+			temperature = <55000>;
+			hysteresis = <2000>;
+			type = "active";
+		};
+		package_fan1: package-fan1 {
+			temperature = <65000>;
+			hysteresis = <2000>;
+			type = "active";
+		};
+	};
+
+	cooling-maps {
+		map0 {
+			trip = <&package_fan0>;
+			cooling-device = <&fan THERMAL_NO_LIMIT 1>;
+		};
+		map1 {
+			trip = <&package_fan1>;
+			cooling-device = <&fan 1 THERMAL_NO_LIMIT>;
+		};
+	};
+};
+
 &i2c0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&i2c0m2_xfer>;
@@ -731,6 +759,10 @@ regulator-state-mem {
 	};
 };
 
+&tsadc {
+	status = "okay";
+};
+
 &uart2 {
 	pinctrl-0 = <&uart2m0_xfer>;
 	status = "okay";

-- 
2.43.0

Re: [PATCH v2 2/4] arm64: dts: rockchip: enable temperature driven fan control on Rock 5B

[Dragan Simic]

Hello Alexey,

Some notes below, please have a look.

On 2024-01-30 19:21, Alexey Charkov wrote:
> This enables thermal monitoring on Radxa Rock 5B and links the PWM
> fan as an active cooling device managed automatically by the thermal
> subsystem, with a target SoC temperature of 65C and a minimum-spin
> interval from 55C to 65C to ensure airflow when the system gets warm

I'd suggest that you replace "temperature driven fan control" with
"active cooling" in the patch subject.  More concise and reads better.

> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Alexey Charkov <alchark@gmail.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 34 
> ++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> index a0e303c3a1dc..b485edeef876 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> @@ -52,7 +52,7 @@ led_rgb_b {
> 
>  	fan: pwm-fan {
>  		compatible = "pwm-fan";
> -		cooling-levels = <0 95 145 195 255>;
> +		cooling-levels = <0 120 150 180 210 240 255>;
>  		fan-supply = <&vcc5v0_sys>;
>  		pwms = <&pwm1 0 50000 0>;
>  		#cooling-cells = <2>;
> @@ -173,6 +173,34 @@ &cpu_l3 {
>  	cpu-supply = <&vdd_cpu_lit_s0>;
>  };
> 
> +&package_thermal {
> +	polling-delay = <1000>;
> +
> +	trips {
> +		package_fan0: package-fan0 {
> +			temperature = <55000>;
> +			hysteresis = <2000>;
> +			type = "active";
> +		};
> +		package_fan1: package-fan1 {
> +			temperature = <65000>;
> +			hysteresis = <2000>;
> +			type = "active";
> +		};
> +	};
> +
> +	cooling-maps {
> +		map0 {

Should be "map1" instead of "map0".  There's already "map0"
defined for "package_thermal" in the RK3588(s) dtsi file.

> +			trip = <&package_fan0>;
> +			cooling-device = <&fan THERMAL_NO_LIMIT 1>;
> +		};
> +		map1 {

Should be "map2" instead of "map1".

> +			trip = <&package_fan1>;
> +			cooling-device = <&fan 1 THERMAL_NO_LIMIT>;

Should be "cooling-device = <&fan 2 THERMAL_NO_LIMIT>;"
(i.e., "2 THERMAL_NO_LIMIT" instead of "1 THERMAL_NO_LIMIT").

The first fan speed is already covered by the first cooling map.
The second cooling map takes over from the second fan speed.

> +		};
> +	};
> +};
> +
>  &i2c0 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&i2c0m2_xfer>;
> @@ -731,6 +759,10 @@ regulator-state-mem {
>  	};
>  };
> 
> +&tsadc {
> +	status = "okay";
> +};
> +
>  &uart2 {
>  	pinctrl-0 = <&uart2m0_xfer>;
>  	status = "okay";

Re: [PATCH v2 2/4] arm64: dts: rockchip: enable temperature driven fan control on Rock 5B

[Alexey Charkov]

On Wed, Jan 31, 2024 at 9:08 AM Dragan Simic <dsimic@manjaro.org> wrote:
>
> Hello Alexey,
>
> Some notes below, please have a look.
>
> On 2024-01-30 19:21, Alexey Charkov wrote:
> > This enables thermal monitoring on Radxa Rock 5B and links the PWM
> > fan as an active cooling device managed automatically by the thermal
> > subsystem, with a target SoC temperature of 65C and a minimum-spin
> > interval from 55C to 65C to ensure airflow when the system gets warm
>
> I'd suggest that you replace "temperature driven fan control" with
> "active cooling" in the patch subject.  More concise and reads better.

Agreed, thanks!

> > Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Signed-off-by: Alexey Charkov <alchark@gmail.com>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 34
> > ++++++++++++++++++++++++-
> >  1 file changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > index a0e303c3a1dc..b485edeef876 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > @@ -52,7 +52,7 @@ led_rgb_b {
> >
> >       fan: pwm-fan {
> >               compatible = "pwm-fan";
> > -             cooling-levels = <0 95 145 195 255>;
> > +             cooling-levels = <0 120 150 180 210 240 255>;
> >               fan-supply = <&vcc5v0_sys>;
> >               pwms = <&pwm1 0 50000 0>;
> >               #cooling-cells = <2>;
> > @@ -173,6 +173,34 @@ &cpu_l3 {
> >       cpu-supply = <&vdd_cpu_lit_s0>;
> >  };
> >
> > +&package_thermal {
> > +     polling-delay = <1000>;
> > +
> > +     trips {
> > +             package_fan0: package-fan0 {
> > +                     temperature = <55000>;
> > +                     hysteresis = <2000>;
> > +                     type = "active";
> > +             };
> > +             package_fan1: package-fan1 {
> > +                     temperature = <65000>;
> > +                     hysteresis = <2000>;
> > +                     type = "active";
> > +             };
> > +     };
> > +
> > +     cooling-maps {
> > +             map0 {
>
> Should be "map1" instead of "map0".  There's already "map0"
> defined for "package_thermal" in the RK3588(s) dtsi file.

Indeed. I got overzealous renaming everything to be zero-based.

> > +                     trip = <&package_fan0>;
> > +                     cooling-device = <&fan THERMAL_NO_LIMIT 1>;
> > +             };
> > +             map1 {
>
> Should be "map2" instead of "map1".

Noted, thanks!

> > +                     trip = <&package_fan1>;
> > +                     cooling-device = <&fan 1 THERMAL_NO_LIMIT>;
>
> Should be "cooling-device = <&fan 2 THERMAL_NO_LIMIT>;"
> (i.e., "2 THERMAL_NO_LIMIT" instead of "1 THERMAL_NO_LIMIT").
>
> The first fan speed is already covered by the first cooling map.
> The second cooling map takes over from the second fan speed.

Makes sense, will adjust, thank you!

Best regards,
Alexey

Re: [PATCH v2 2/4] arm64: dts: rockchip: enable temperature driven fan control on Rock 5B

[Chen-Yu Tsai]

On Wed, Jan 31, 2024 at 2:22 AM Alexey Charkov <alchark@gmail.com> wrote:
>
> This enables thermal monitoring on Radxa Rock 5B and links the PWM
> fan as an active cooling device managed automatically by the thermal
> subsystem, with a target SoC temperature of 65C and a minimum-spin
> interval from 55C to 65C to ensure airflow when the system gets warm
>
> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Signed-off-by: Alexey Charkov <alchark@gmail.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 34 ++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> index a0e303c3a1dc..b485edeef876 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> @@ -52,7 +52,7 @@ led_rgb_b {
>
>         fan: pwm-fan {
>                 compatible = "pwm-fan";
> -               cooling-levels = <0 95 145 195 255>;
> +               cooling-levels = <0 120 150 180 210 240 255>;
>                 fan-supply = <&vcc5v0_sys>;
>                 pwms = <&pwm1 0 50000 0>;
>                 #cooling-cells = <2>;
> @@ -173,6 +173,34 @@ &cpu_l3 {
>         cpu-supply = <&vdd_cpu_lit_s0>;
>  };
>
> +&package_thermal {
> +       polling-delay = <1000>;
> +
> +       trips {
> +               package_fan0: package-fan0 {
> +                       temperature = <55000>;
> +                       hysteresis = <2000>;
> +                       type = "active";
> +               };
> +               package_fan1: package-fan1 {
> +                       temperature = <65000>;
> +                       hysteresis = <2000>;
> +                       type = "active";
> +               };
> +       };
> +
> +       cooling-maps {
> +               map0 {
> +                       trip = <&package_fan0>;
> +                       cooling-device = <&fan THERMAL_NO_LIMIT 1>;
> +               };
> +               map1 {
> +                       trip = <&package_fan1>;
> +                       cooling-device = <&fan 1 THERMAL_NO_LIMIT>;
> +               };
> +       };
> +};
> +
>  &i2c0 {
>         pinctrl-names = "default";
>         pinctrl-0 = <&i2c0m2_xfer>;
> @@ -731,6 +759,10 @@ regulator-state-mem {
>         };
>  };
>
> +&tsadc {
> +       status = "okay";
> +};
> +

Is there any reason this can't be enabled by default in the .dtsi file?
The thermal sensor doesn't depend on anything external, so there should
be no reason to push this down to the board level.

ChenYu

>  &uart2 {
>         pinctrl-0 = <&uart2m0_xfer>;
>         status = "okay";
>
> --
> 2.43.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/4] arm64: dts: rockchip: enable temperature driven fan control on Rock 5B

[Dragan Simic]

Hello Chen-Yu,

On 2024-02-01 15:26, Chen-Yu Tsai wrote:
> On Wed, Jan 31, 2024 at 2:22 AM Alexey Charkov <alchark@gmail.com> 
> wrote:
>> 
>> This enables thermal monitoring on Radxa Rock 5B and links the PWM
>> fan as an active cooling device managed automatically by the thermal
>> subsystem, with a target SoC temperature of 65C and a minimum-spin
>> interval from 55C to 65C to ensure airflow when the system gets warm
>> 
>> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Signed-off-by: Alexey Charkov <alchark@gmail.com>
>> ---
>>  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 34 
>> ++++++++++++++++++++++++-
>>  1 file changed, 33 insertions(+), 1 deletion(-)
>> 
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts 
>> b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>> index a0e303c3a1dc..b485edeef876 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>> @@ -52,7 +52,7 @@ led_rgb_b {
>> 
>>         fan: pwm-fan {
>>                 compatible = "pwm-fan";
>> -               cooling-levels = <0 95 145 195 255>;
>> +               cooling-levels = <0 120 150 180 210 240 255>;
>>                 fan-supply = <&vcc5v0_sys>;
>>                 pwms = <&pwm1 0 50000 0>;
>>                 #cooling-cells = <2>;
>> @@ -173,6 +173,34 @@ &cpu_l3 {
>>         cpu-supply = <&vdd_cpu_lit_s0>;
>>  };
>> 
>> +&package_thermal {
>> +       polling-delay = <1000>;
>> +
>> +       trips {
>> +               package_fan0: package-fan0 {
>> +                       temperature = <55000>;
>> +                       hysteresis = <2000>;
>> +                       type = "active";
>> +               };
>> +               package_fan1: package-fan1 {
>> +                       temperature = <65000>;
>> +                       hysteresis = <2000>;
>> +                       type = "active";
>> +               };
>> +       };
>> +
>> +       cooling-maps {
>> +               map0 {
>> +                       trip = <&package_fan0>;
>> +                       cooling-device = <&fan THERMAL_NO_LIMIT 1>;
>> +               };
>> +               map1 {
>> +                       trip = <&package_fan1>;
>> +                       cooling-device = <&fan 1 THERMAL_NO_LIMIT>;
>> +               };
>> +       };
>> +};
>> +
>>  &i2c0 {
>>         pinctrl-names = "default";
>>         pinctrl-0 = <&i2c0m2_xfer>;
>> @@ -731,6 +759,10 @@ regulator-state-mem {
>>         };
>>  };
>> 
>> +&tsadc {
>> +       status = "okay";
>> +};
>> +
> 
> Is there any reason this can't be enabled by default in the .dtsi file?
> The thermal sensor doesn't depend on anything external, so there should
> be no reason to push this down to the board level.

Actually, there is a reason.  Different boards can handle the critical
overheating differently, by letting the CRU or the PMIC handle it.  This
was also the case for the RK3399.

Please, have a look at the following DT properties, which are consumed
by drivers/thermal/rockchip_thermal.c:
   - "rockchip,hw-tshut-mode"
   - "rockchip,hw-tshut-polarity"

See also page 1,372 of the RK3588 TRM v1.0.

This has also reminded me to check how is the Rock 5B actually wired,
just to make sure.  We actually need to provide the two DT properties
listed above, at least to avoid emitting the warnings.

>>  &uart2 {
>>         pinctrl-0 = <&uart2m0_xfer>;
>>         status = "okay";
>> 
>> --
>> 2.43.0

Re: [PATCH v2 2/4] arm64: dts: rockchip: enable temperature driven fan control on Rock 5B

[Alexey Charkov]

On Thu, Feb 1, 2024 at 9:34 PM Dragan Simic <dsimic@manjaro.org> wrote:
>
> Hello Chen-Yu,
>
> On 2024-02-01 15:26, Chen-Yu Tsai wrote:
> > On Wed, Jan 31, 2024 at 2:22 AM Alexey Charkov <alchark@gmail.com>
> > wrote:
> >>
> >> This enables thermal monitoring on Radxa Rock 5B and links the PWM
> >> fan as an active cooling device managed automatically by the thermal
> >> subsystem, with a target SoC temperature of 65C and a minimum-spin
> >> interval from 55C to 65C to ensure airflow when the system gets warm
> >>
> >> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> Signed-off-by: Alexey Charkov <alchark@gmail.com>
> >> ---
> >>  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 34
> >> ++++++++++++++++++++++++-
> >>  1 file changed, 33 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >> b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >> index a0e303c3a1dc..b485edeef876 100644
> >> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >> @@ -52,7 +52,7 @@ led_rgb_b {
> >>
> >>         fan: pwm-fan {
> >>                 compatible = "pwm-fan";
> >> -               cooling-levels = <0 95 145 195 255>;
> >> +               cooling-levels = <0 120 150 180 210 240 255>;
> >>                 fan-supply = <&vcc5v0_sys>;
> >>                 pwms = <&pwm1 0 50000 0>;
> >>                 #cooling-cells = <2>;
> >> @@ -173,6 +173,34 @@ &cpu_l3 {
> >>         cpu-supply = <&vdd_cpu_lit_s0>;
> >>  };
> >>
> >> +&package_thermal {
> >> +       polling-delay = <1000>;
> >> +
> >> +       trips {
> >> +               package_fan0: package-fan0 {
> >> +                       temperature = <55000>;
> >> +                       hysteresis = <2000>;
> >> +                       type = "active";
> >> +               };
> >> +               package_fan1: package-fan1 {
> >> +                       temperature = <65000>;
> >> +                       hysteresis = <2000>;
> >> +                       type = "active";
> >> +               };
> >> +       };
> >> +
> >> +       cooling-maps {
> >> +               map0 {
> >> +                       trip = <&package_fan0>;
> >> +                       cooling-device = <&fan THERMAL_NO_LIMIT 1>;
> >> +               };
> >> +               map1 {
> >> +                       trip = <&package_fan1>;
> >> +                       cooling-device = <&fan 1 THERMAL_NO_LIMIT>;
> >> +               };
> >> +       };
> >> +};
> >> +
> >>  &i2c0 {
> >>         pinctrl-names = "default";
> >>         pinctrl-0 = <&i2c0m2_xfer>;
> >> @@ -731,6 +759,10 @@ regulator-state-mem {
> >>         };
> >>  };
> >>
> >> +&tsadc {
> >> +       status = "okay";
> >> +};
> >> +
> >
> > Is there any reason this can't be enabled by default in the .dtsi file?
> > The thermal sensor doesn't depend on anything external, so there should
> > be no reason to push this down to the board level.
>
> Actually, there is a reason.  Different boards can handle the critical
> overheating differently, by letting the CRU or the PMIC handle it.  This
> was also the case for the RK3399.
>
> Please, have a look at the following DT properties, which are consumed
> by drivers/thermal/rockchip_thermal.c:
>    - "rockchip,hw-tshut-mode"
>    - "rockchip,hw-tshut-polarity"
>
> See also page 1,372 of the RK3588 TRM v1.0.
>
> This has also reminded me to check how is the Rock 5B actually wired,
> just to make sure.  We actually need to provide the two DT properties
> listed above, at least to avoid emitting the warnings.

Well the defaults are already provided in rk3588s.dtsi, so there won't
be any warnings (see lines 2222-2223 in Linus' master version), and
according to the vendor kernel those are also what Rock 5B uses.

This made me think however: what if a board doesn't enable TSADC, but
has OPPs in place for higher voltage and frequency states? There won't
be any throttling (as there won't be any thermal monitoring) and there
might not be a critical shutdown at all if it heats up - possibly even
causing hardware damage. In this case it seems that having TSADC
enabled by default would at least trigger passive cooling, hopefully
avoiding the critical shutdown altogether and making those properties
irrelevant in 99% cases.

Best regards,
Alexey

Re: [PATCH v2 2/4] arm64: dts: rockchip: enable temperature driven fan control on Rock 5B

[Dragan Simic]

On 2024-02-01 20:15, Alexey Charkov wrote:
> On Thu, Feb 1, 2024 at 9:34 PM Dragan Simic <dsimic@manjaro.org> wrote:
>> On 2024-02-01 15:26, Chen-Yu Tsai wrote:
>> > On Wed, Jan 31, 2024 at 2:22 AM Alexey Charkov <alchark@gmail.com>
>> > wrote:
>> >>
>> >> This enables thermal monitoring on Radxa Rock 5B and links the PWM
>> >> fan as an active cooling device managed automatically by the thermal
>> >> subsystem, with a target SoC temperature of 65C and a minimum-spin
>> >> interval from 55C to 65C to ensure airflow when the system gets warm
>> >>
>> >> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> >> Signed-off-by: Alexey Charkov <alchark@gmail.com>
>> >> ---
>> >>  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 34
>> >> ++++++++++++++++++++++++-
>> >>  1 file changed, 33 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>> >> b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>> >> index a0e303c3a1dc..b485edeef876 100644
>> >> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>> >> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>> >> @@ -52,7 +52,7 @@ led_rgb_b {
>> >>
>> >>         fan: pwm-fan {
>> >>                 compatible = "pwm-fan";
>> >> -               cooling-levels = <0 95 145 195 255>;
>> >> +               cooling-levels = <0 120 150 180 210 240 255>;
>> >>                 fan-supply = <&vcc5v0_sys>;
>> >>                 pwms = <&pwm1 0 50000 0>;
>> >>                 #cooling-cells = <2>;
>> >> @@ -173,6 +173,34 @@ &cpu_l3 {
>> >>         cpu-supply = <&vdd_cpu_lit_s0>;
>> >>  };
>> >>
>> >> +&package_thermal {
>> >> +       polling-delay = <1000>;
>> >> +
>> >> +       trips {
>> >> +               package_fan0: package-fan0 {
>> >> +                       temperature = <55000>;
>> >> +                       hysteresis = <2000>;
>> >> +                       type = "active";
>> >> +               };
>> >> +               package_fan1: package-fan1 {
>> >> +                       temperature = <65000>;
>> >> +                       hysteresis = <2000>;
>> >> +                       type = "active";
>> >> +               };
>> >> +       };
>> >> +
>> >> +       cooling-maps {
>> >> +               map0 {
>> >> +                       trip = <&package_fan0>;
>> >> +                       cooling-device = <&fan THERMAL_NO_LIMIT 1>;
>> >> +               };
>> >> +               map1 {
>> >> +                       trip = <&package_fan1>;
>> >> +                       cooling-device = <&fan 1 THERMAL_NO_LIMIT>;
>> >> +               };
>> >> +       };
>> >> +};
>> >> +
>> >>  &i2c0 {
>> >>         pinctrl-names = "default";
>> >>         pinctrl-0 = <&i2c0m2_xfer>;
>> >> @@ -731,6 +759,10 @@ regulator-state-mem {
>> >>         };
>> >>  };
>> >>
>> >> +&tsadc {
>> >> +       status = "okay";
>> >> +};
>> >> +
>> >
>> > Is there any reason this can't be enabled by default in the .dtsi file?
>> > The thermal sensor doesn't depend on anything external, so there should
>> > be no reason to push this down to the board level.
>> 
>> Actually, there is a reason.  Different boards can handle the critical
>> overheating differently, by letting the CRU or the PMIC handle it.  
>> This
>> was also the case for the RK3399.
>> 
>> Please, have a look at the following DT properties, which are consumed
>> by drivers/thermal/rockchip_thermal.c:
>>    - "rockchip,hw-tshut-mode"
>>    - "rockchip,hw-tshut-polarity"
>> 
>> See also page 1,372 of the RK3588 TRM v1.0.
>> 
>> This has also reminded me to check how is the Rock 5B actually wired,
>> just to make sure.  We actually need to provide the two DT properties
>> listed above, at least to avoid emitting the warnings.
> 
> Well the defaults are already provided in rk3588s.dtsi, so there won't
> be any warnings (see lines 2222-2223 in Linus' master version), and
> according to the vendor kernel those are also what Rock 5B uses.

Yes, I noticed the same a couple of minutes after sending my last
message, but didn't want to make more noise about it. :)  I would've
mentioned it in my next message, of course.

> This made me think however: what if a board doesn't enable TSADC, but
> has OPPs in place for higher voltage and frequency states? There won't
> be any throttling (as there won't be any thermal monitoring) and there
> might not be a critical shutdown at all if it heats up - possibly even
> causing hardware damage. In this case it seems that having TSADC
> enabled by default would at least trigger passive cooling, hopefully
> avoiding the critical shutdown altogether and making those properties
> irrelevant in 99% cases.

Those are very good questions.  Thumbs up!

The trouble is that the boards can use different wiring to handle the
thermal runaways, by expecting the PMIC to handle it or not.  Thus,
it's IMHO better to simply leave that to be tested and enabled on a
board-by-board basis, whenever a new RK3588(s)-based board is added.

Thus, the only right way at this point would be to merge the addition
of the OPPs and the enabling of the TSADC for all currently supported
RK3588(s)-based boards at once, instead of just for the Rock 5B.

I can handle the required changes for the QuartzPro64 dts file.  For
other supported RK3588(s)-based boards, if there are no people having
access to them and willing to perform the dts changes and the testing,
I'd be willing to go through the board schematics, to enable the
TSADC for them as well.

Re: [PATCH v2 2/4] arm64: dts: rockchip: enable temperature driven fan control on Rock 5B

[Dragan Simic]

On 2024-02-01 20:31, Dragan Simic wrote:
> On 2024-02-01 20:15, Alexey Charkov wrote:
>> On Thu, Feb 1, 2024 at 9:34 PM Dragan Simic <dsimic@manjaro.org> 
>> wrote:
>>> On 2024-02-01 15:26, Chen-Yu Tsai wrote:
>>> > On Wed, Jan 31, 2024 at 2:22 AM Alexey Charkov <alchark@gmail.com>
>>> > wrote:
>>> >>
>>> >> This enables thermal monitoring on Radxa Rock 5B and links the PWM
>>> >> fan as an active cooling device managed automatically by the thermal
>>> >> subsystem, with a target SoC temperature of 65C and a minimum-spin
>>> >> interval from 55C to 65C to ensure airflow when the system gets warm
>>> >>
>>> >> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> >> Signed-off-by: Alexey Charkov <alchark@gmail.com>
>>> >> ---
>>> >>  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 34
>>> >> ++++++++++++++++++++++++-
>>> >>  1 file changed, 33 insertions(+), 1 deletion(-)
>>> >>
>>> >> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>> >> b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>> >> index a0e303c3a1dc..b485edeef876 100644
>>> >> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>> >> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>> >> @@ -52,7 +52,7 @@ led_rgb_b {
>>> >>
>>> >>         fan: pwm-fan {
>>> >>                 compatible = "pwm-fan";
>>> >> -               cooling-levels = <0 95 145 195 255>;
>>> >> +               cooling-levels = <0 120 150 180 210 240 255>;
>>> >>                 fan-supply = <&vcc5v0_sys>;
>>> >>                 pwms = <&pwm1 0 50000 0>;
>>> >>                 #cooling-cells = <2>;
>>> >> @@ -173,6 +173,34 @@ &cpu_l3 {
>>> >>         cpu-supply = <&vdd_cpu_lit_s0>;
>>> >>  };
>>> >>
>>> >> +&package_thermal {
>>> >> +       polling-delay = <1000>;
>>> >> +
>>> >> +       trips {
>>> >> +               package_fan0: package-fan0 {
>>> >> +                       temperature = <55000>;
>>> >> +                       hysteresis = <2000>;
>>> >> +                       type = "active";
>>> >> +               };
>>> >> +               package_fan1: package-fan1 {
>>> >> +                       temperature = <65000>;
>>> >> +                       hysteresis = <2000>;
>>> >> +                       type = "active";
>>> >> +               };
>>> >> +       };
>>> >> +
>>> >> +       cooling-maps {
>>> >> +               map0 {
>>> >> +                       trip = <&package_fan0>;
>>> >> +                       cooling-device = <&fan THERMAL_NO_LIMIT 1>;
>>> >> +               };
>>> >> +               map1 {
>>> >> +                       trip = <&package_fan1>;
>>> >> +                       cooling-device = <&fan 1 THERMAL_NO_LIMIT>;
>>> >> +               };
>>> >> +       };
>>> >> +};
>>> >> +
>>> >>  &i2c0 {
>>> >>         pinctrl-names = "default";
>>> >>         pinctrl-0 = <&i2c0m2_xfer>;
>>> >> @@ -731,6 +759,10 @@ regulator-state-mem {
>>> >>         };
>>> >>  };
>>> >>
>>> >> +&tsadc {
>>> >> +       status = "okay";
>>> >> +};
>>> >> +
>>> >
>>> > Is there any reason this can't be enabled by default in the .dtsi file?
>>> > The thermal sensor doesn't depend on anything external, so there should
>>> > be no reason to push this down to the board level.
>>> 
>>> Actually, there is a reason.  Different boards can handle the 
>>> critical
>>> overheating differently, by letting the CRU or the PMIC handle it.  
>>> This
>>> was also the case for the RK3399.
>>> 
>>> Please, have a look at the following DT properties, which are 
>>> consumed
>>> by drivers/thermal/rockchip_thermal.c:
>>>    - "rockchip,hw-tshut-mode"
>>>    - "rockchip,hw-tshut-polarity"
>>> 
>>> See also page 1,372 of the RK3588 TRM v1.0.
>>> 
>>> This has also reminded me to check how is the Rock 5B actually wired,
>>> just to make sure.  We actually need to provide the two DT properties
>>> listed above, at least to avoid emitting the warnings.
>> 
>> Well the defaults are already provided in rk3588s.dtsi, so there won't
>> be any warnings (see lines 2222-2223 in Linus' master version), and
>> according to the vendor kernel those are also what Rock 5B uses.
> 
> Yes, I noticed the same a couple of minutes after sending my last
> message, but didn't want to make more noise about it. :)  I would've
> mentioned it in my next message, of course.

Just checked the Rock 5B schematic and it expects the CRU to be used
to perform the hardware reset in case of a thermal runaway, so the
defaults in the RK3588s dtsi are fine.  I had to double-check it. :)

However, now I have some open questions related to interrupt-driven
operation.  I'll research it further and come back with an update.

>> This made me think however: what if a board doesn't enable TSADC, but
>> has OPPs in place for higher voltage and frequency states? There won't
>> be any throttling (as there won't be any thermal monitoring) and there
>> might not be a critical shutdown at all if it heats up - possibly even
>> causing hardware damage. In this case it seems that having TSADC
>> enabled by default would at least trigger passive cooling, hopefully
>> avoiding the critical shutdown altogether and making those properties
>> irrelevant in 99% cases.
> 
> Those are very good questions.  Thumbs up!
> 
> The trouble is that the boards can use different wiring to handle the
> thermal runaways, by expecting the PMIC to handle it or not.  Thus,
> it's IMHO better to simply leave that to be tested and enabled on a
> board-by-board basis, whenever a new RK3588(s)-based board is added.
> 
> Thus, the only right way at this point would be to merge the addition
> of the OPPs and the enabling of the TSADC for all currently supported
> RK3588(s)-based boards at once, instead of just for the Rock 5B.
> 
> I can handle the required changes for the QuartzPro64 dts file.  For
> other supported RK3588(s)-based boards, if there are no people having
> access to them and willing to perform the dts changes and the testing,
> I'd be willing to go through the board schematics, to enable the
> TSADC for them as well.

Please, let me know are you fine with the above-described approach.

Re: [PATCH v2 2/4] arm64: dts: rockchip: enable temperature driven fan control on Rock 5B

[Alexey Charkov]

On Thu, Feb 1, 2024 at 11:43 PM Dragan Simic <dsimic@manjaro.org> wrote:
>
> On 2024-02-01 20:31, Dragan Simic wrote:
> > On 2024-02-01 20:15, Alexey Charkov wrote:
> >> On Thu, Feb 1, 2024 at 9:34 PM Dragan Simic <dsimic@manjaro.org>
> >> wrote:
> >>> On 2024-02-01 15:26, Chen-Yu Tsai wrote:
> >>> > On Wed, Jan 31, 2024 at 2:22 AM Alexey Charkov <alchark@gmail.com>
> >>> > wrote:
> >>> >>
> >>> >> This enables thermal monitoring on Radxa Rock 5B and links the PWM
> >>> >> fan as an active cooling device managed automatically by the thermal
> >>> >> subsystem, with a target SoC temperature of 65C and a minimum-spin
> >>> >> interval from 55C to 65C to ensure airflow when the system gets warm
> >>> >>
> >>> >> Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>> >> Signed-off-by: Alexey Charkov <alchark@gmail.com>
> >>> >> ---
> >>> >>  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 34
> >>> >> ++++++++++++++++++++++++-
> >>> >>  1 file changed, 33 insertions(+), 1 deletion(-)
> >>> >>
> >>> >> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >>> >> b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >>> >> index a0e303c3a1dc..b485edeef876 100644
> >>> >> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >>> >> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> >>> >> @@ -52,7 +52,7 @@ led_rgb_b {
> >>> >>
> >>> >>         fan: pwm-fan {
> >>> >>                 compatible = "pwm-fan";
> >>> >> -               cooling-levels = <0 95 145 195 255>;
> >>> >> +               cooling-levels = <0 120 150 180 210 240 255>;
> >>> >>                 fan-supply = <&vcc5v0_sys>;
> >>> >>                 pwms = <&pwm1 0 50000 0>;
> >>> >>                 #cooling-cells = <2>;
> >>> >> @@ -173,6 +173,34 @@ &cpu_l3 {
> >>> >>         cpu-supply = <&vdd_cpu_lit_s0>;
> >>> >>  };
> >>> >>
> >>> >> +&package_thermal {
> >>> >> +       polling-delay = <1000>;
> >>> >> +
> >>> >> +       trips {
> >>> >> +               package_fan0: package-fan0 {
> >>> >> +                       temperature = <55000>;
> >>> >> +                       hysteresis = <2000>;
> >>> >> +                       type = "active";
> >>> >> +               };
> >>> >> +               package_fan1: package-fan1 {
> >>> >> +                       temperature = <65000>;
> >>> >> +                       hysteresis = <2000>;
> >>> >> +                       type = "active";
> >>> >> +               };
> >>> >> +       };
> >>> >> +
> >>> >> +       cooling-maps {
> >>> >> +               map0 {
> >>> >> +                       trip = <&package_fan0>;
> >>> >> +                       cooling-device = <&fan THERMAL_NO_LIMIT 1>;
> >>> >> +               };
> >>> >> +               map1 {
> >>> >> +                       trip = <&package_fan1>;
> >>> >> +                       cooling-device = <&fan 1 THERMAL_NO_LIMIT>;
> >>> >> +               };
> >>> >> +       };
> >>> >> +};
> >>> >> +
> >>> >>  &i2c0 {
> >>> >>         pinctrl-names = "default";
> >>> >>         pinctrl-0 = <&i2c0m2_xfer>;
> >>> >> @@ -731,6 +759,10 @@ regulator-state-mem {
> >>> >>         };
> >>> >>  };
> >>> >>
> >>> >> +&tsadc {
> >>> >> +       status = "okay";
> >>> >> +};
> >>> >> +
> >>> >
> >>> > Is there any reason this can't be enabled by default in the .dtsi file?
> >>> > The thermal sensor doesn't depend on anything external, so there should
> >>> > be no reason to push this down to the board level.
> >>>
> >>> Actually, there is a reason.  Different boards can handle the
> >>> critical
> >>> overheating differently, by letting the CRU or the PMIC handle it.
> >>> This
> >>> was also the case for the RK3399.
> >>>
> >>> Please, have a look at the following DT properties, which are
> >>> consumed
> >>> by drivers/thermal/rockchip_thermal.c:
> >>>    - "rockchip,hw-tshut-mode"
> >>>    - "rockchip,hw-tshut-polarity"
> >>>
> >>> See also page 1,372 of the RK3588 TRM v1.0.
> >>>
> >>> This has also reminded me to check how is the Rock 5B actually wired,
> >>> just to make sure.  We actually need to provide the two DT properties
> >>> listed above, at least to avoid emitting the warnings.
> >>
> >> Well the defaults are already provided in rk3588s.dtsi, so there won't
> >> be any warnings (see lines 2222-2223 in Linus' master version), and
> >> according to the vendor kernel those are also what Rock 5B uses.
> >
> > Yes, I noticed the same a couple of minutes after sending my last
> > message, but didn't want to make more noise about it. :)  I would've
> > mentioned it in my next message, of course.
>
> Just checked the Rock 5B schematic and it expects the CRU to be used
> to perform the hardware reset in case of a thermal runaway, so the
> defaults in the RK3588s dtsi are fine.  I had to double-check it. :)

I've just looked at Rock 5B, Rock 5A and NanoPC T6 schematics, and
they all seem to have the TSADC_SHUT line connected to RESET_L. At the
same time, Radxa's device tree uses the default CRU-based option. To
me this seems to imply that the CRU option should always work, by the
virtue of CRU being on-chip. At the same time, if the right GPIOs are
wired to the PMIC reset line for a particular board, the board may
also choose to use the GPIO option - or stick with CRU.

If that interpretation is correct, then I suggest we enable TSADC by
default in the .dtsi, and let it handle both throttling and CRU-based
critical resets on all boards. Those who know what they are doing may
then elect to switch their board to GPIO-PMIC based reset.

What do you think?

> However, now I have some open questions related to interrupt-driven
> operation.  I'll research it further and come back with an update.
>
> >> This made me think however: what if a board doesn't enable TSADC, but
> >> has OPPs in place for higher voltage and frequency states? There won't
> >> be any throttling (as there won't be any thermal monitoring) and there
> >> might not be a critical shutdown at all if it heats up - possibly even
> >> causing hardware damage. In this case it seems that having TSADC
> >> enabled by default would at least trigger passive cooling, hopefully
> >> avoiding the critical shutdown altogether and making those properties
> >> irrelevant in 99% cases.
> >
> > Those are very good questions.  Thumbs up!
> >
> > The trouble is that the boards can use different wiring to handle the
> > thermal runaways, by expecting the PMIC to handle it or not.  Thus,
> > it's IMHO better to simply leave that to be tested and enabled on a
> > board-by-board basis, whenever a new RK3588(s)-based board is added.
> >
> > Thus, the only right way at this point would be to merge the addition
> > of the OPPs and the enabling of the TSADC for all currently supported
> > RK3588(s)-based boards at once, instead of just for the Rock 5B.

If we can agree on a workable 'default-on' configuration for TSADC to
be included in the .dtsi I think that would be preferable, because it
would enable all boards to benefit from higher OPPs and throttling.

This would also save us from a scenario when OPPs get included in the
default .dtsi, but TSADC is off by default, and then some poor soul
tries to add a new board with a minimal .dts, forgetting to enable
TSADC and having their SoC fried under high load...

> > I can handle the required changes for the QuartzPro64 dts file.  For
> > other supported RK3588(s)-based boards, if there are no people having
> > access to them and willing to perform the dts changes and the testing,
> > I'd be willing to go through the board schematics, to enable the
> > TSADC for them as well.
>
> Please, let me know are you fine with the above-described approach.

I believe it's great if we can go through the schematics no matter
what! Although if we agree that CRU is an always-working default
option for all, then why don't we just enable TSADC for all, and leave
the conversion to GPIO-PMIC resets for later and for where it's
needed?

Best regards,
Alexey

Re: [PATCH v2 2/4] arm64: dts: rockchip: enable temperature driven fan control on Rock 5B

[Dragan Simic]

Hello Alexey,

On 2024-02-02 15:42, Alexey Charkov wrote:
> On Thu, Feb 1, 2024 at 11:43 PM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> On 2024-02-01 20:31, Dragan Simic wrote:
>>> On 2024-02-01 20:15, Alexey Charkov wrote:
>>>> On Thu, Feb 1, 2024 at 9:34 PM Dragan Simic <dsimic@manjaro.org> 
>>>> wrote:
>>>>> On 2024-02-01 15:26, Chen-Yu Tsai wrote:
>>>>>> Is there any reason this can't be enabled by default in the .dtsi 
>>>>>> file?
>>>>>> The thermal sensor doesn't depend on anything external, so there 
>>>>>> should
>>>>>> be no reason to push this down to the board level.
>>>>> 
>>>>> Actually, there is a reason.  Different boards can handle the
>>>>> critical overheating differently, by letting the CRU or the PMIC
>>>>> handle it.  This was also the case for the RK3399.
>>>>> 
>>>>> Please, have a look at the following DT properties, which are
>>>>> consumed by drivers/thermal/rockchip_thermal.c:
>>>>>    - "rockchip,hw-tshut-mode"
>>>>>    - "rockchip,hw-tshut-polarity"
>>>>> 
>>>>> See also page 1,372 of the RK3588 TRM v1.0.
>>>>> 
>>>>> This has also reminded me to check how is the Rock 5B actually 
>>>>> wired,
>>>>> just to make sure.  We actually need to provide the two DT 
>>>>> properties
>>>>> listed above, at least to avoid emitting the warnings.
>>>> 
>>>> Well the defaults are already provided in rk3588s.dtsi, so there 
>>>> won't
>>>> be any warnings (see lines 2222-2223 in Linus' master version), and
>>>> according to the vendor kernel those are also what Rock 5B uses.
>>> 
>>> Yes, I noticed the same a couple of minutes after sending my last
>>> message, but didn't want to make more noise about it. :)  I would've
>>> mentioned it in my next message, of course.
>> 
>> Just checked the Rock 5B schematic and it expects the CRU to be used
>> to perform the hardware reset in case of a thermal runaway, so the
>> defaults in the RK3588s dtsi are fine.  I had to double-check it. :)
> 
> I've just looked at Rock 5B, Rock 5A and NanoPC T6 schematics, and
> they all seem to have the TSADC_SHUT line connected to RESET_L.

Ah, I see it now in the Rock 5B schematic, thank you for the
correction.  I somehow managed to miss it initially;  here's
the link to a screenshot from the Rock 5B schematic v1.423, for
future reference:  https://i.imgur.com/IGAPPgl.png .

> At the same time, Radxa's device tree uses the default
> CRU-based option.

Here's the link to a screenshot from the RK3588 Hardware
Design Guide v1.0, which shows the recommended reset signal
paths for RK3588 boards:  https://i.imgur.com/DNqhjfP.png .

As visible in the Rock 5B schematic, it expectedly follows
this recommendation from Rockchip, so we should actually use
GPIO-based handling for the thermal runaways on the Rock 5B,
to have the PMIC reset as well.  Here's the link to another
screenshot from the Rock 5B schematic v1.423, for future
reference:  https://i.imgur.com/BdgZ30C.png .

Of course, it should be tested to make sure that the thermal
runaway resets work fine.

It isn't uncommon for downstream DTs to sometimes contain
some small mistakes that somehow remained undetected.

> To me this seems to imply that the CRU option should always work, by
> the virtue of CRU being on-chip. At the same time, if the right GPIOs
> are wired to the PMIC reset line for a particular board, the board
> may also choose to use the GPIO option - or stick with CRU.
> 
> If that interpretation is correct, then I suggest we enable TSADC by
> default in the .dtsi, and let it handle both throttling and CRU-based
> critical resets on all boards. Those who know what they are doing may
> then elect to switch their board to GPIO-PMIC based reset.
> 
> What do you think?

I think that, if we choose to enable CRU-based thermal runaway
resets without going into too much detail for each board (but
we should go into the publicly available board schematics, as
also noted in my last comment in this message), we should do
that in the board dts files, instead of just enabling the TSADC
in the RK3588(s) SoC dtsi.

That way, we would clearly indicate the TSADC to be a board-
specific feature, and hopefully gain more attention from the
people interested in the boards, to perform some additional
testing, etc.

>> However, now I have some open questions related to interrupt-driven
>> operation.  I'll research it further and come back with an update.
>> 
>>>> This made me think however: what if a board doesn't enable TSADC, 
>>>> but
>>>> has OPPs in place for higher voltage and frequency states? There 
>>>> won't
>>>> be any throttling (as there won't be any thermal monitoring) and 
>>>> there
>>>> might not be a critical shutdown at all if it heats up - possibly 
>>>> even
>>>> causing hardware damage. In this case it seems that having TSADC
>>>> enabled by default would at least trigger passive cooling, hopefully
>>>> avoiding the critical shutdown altogether and making those 
>>>> properties
>>>> irrelevant in 99% cases.
>>> 
>>> Those are very good questions.  Thumbs up!
>>> 
>>> The trouble is that the boards can use different wiring to handle the
>>> thermal runaways, by expecting the PMIC to handle it or not.  Thus,
>>> it's IMHO better to simply leave that to be tested and enabled on a
>>> board-by-board basis, whenever a new RK3588(s)-based board is added.
>>> 
>>> Thus, the only right way at this point would be to merge the addition
>>> of the OPPs and the enabling of the TSADC for all currently supported
>>> RK3588(s)-based boards at once, instead of just for the Rock 5B.
> 
> If we can agree on a workable 'default-on' configuration for TSADC to
> be included in the .dtsi I think that would be preferable, because it
> would enable all boards to benefit from higher OPPs and throttling.

Please, see my other comments.  I hope we can agree on that.

> This would also save us from a scenario when OPPs get included in the
> default .dtsi, but TSADC is off by default, and then some poor soul
> tries to add a new board with a minimal .dts, forgetting to enable
> TSADC and having their SoC fried under high load...

Well, those poor souls can't escape the need to know what are
they doing. :)  Also, I think it's much more likely that adding
a new RK3588-based board would actually start by editing the
board dts of some already supported RK3588 board, which the way
I propose this to be handled would already have the TSADC enabled,
eliminating the risks yout pointed out.

Please note that the TSADC has been disabled in the RK3399 SoC
dtsi and enabled on a per-RK3399-board-dtsi basis, so we'd also
have some consistency by following the same approach with the
RK3588(s) SoC dtsi.  Consistency is good, if you agree.

>>> I can handle the required changes for the QuartzPro64 dts file.  For
>>> other supported RK3588(s)-based boards, if there are no people having
>>> access to them and willing to perform the dts changes and the 
>>> testing,
>>> I'd be willing to go through the board schematics, to enable the
>>> TSADC for them as well.
>> 
>> Please, let me know are you fine with the above-described approach.
> 
> I believe it's great if we can go through the schematics no matter
> what! Although if we agree that CRU is an always-working default
> option for all, then why don't we just enable TSADC for all, and leave
> the conversion to GPIO-PMIC resets for later and for where it's
> needed?

Great!  We can surely go through the supported RK3588(s) boards
that make their schematics publicly available, and enable the
TSADC in their board dts files accordingly.

For the remaining RK3588(s) boards that remain "black boxes" to
us, we can enable the TSADC in their board dts files with the
let-the-CRU-handle-thermal-runaway defaults, and leave any future
refinements to the people interested in those boards.

That would be a rather clean approach, if you agree.

[PATCH v2 3/4] arm64: dts: rockchip: Add OPP data for CPU cores on RK3588

[Alexey Charkov]

By default the CPUs on RK3588 start up in a conservative performance
mode. Add frequency and voltage mappings to the device tree to enable
dynamic scaling via cpufreq

Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
 arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 122 ++++++++++++++++++++++++++++++
 1 file changed, 122 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
index 696cb72d75d0..af8b932a04c1 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
@@ -97,6 +97,7 @@ cpu_l0: cpu@0 {
 			clocks = <&scmi_clk SCMI_CLK_CPUL>;
 			assigned-clocks = <&scmi_clk SCMI_CLK_CPUL>;
 			assigned-clock-rates = <816000000>;
+			operating-points-v2 = <&cluster0_opp_table>;
 			cpu-idle-states = <&CPU_SLEEP>;
 			i-cache-size = <32768>;
 			i-cache-line-size = <64>;
@@ -116,6 +117,7 @@ cpu_l1: cpu@100 {
 			enable-method = "psci";
 			capacity-dmips-mhz = <530>;
 			clocks = <&scmi_clk SCMI_CLK_CPUL>;
+			operating-points-v2 = <&cluster0_opp_table>;
 			cpu-idle-states = <&CPU_SLEEP>;
 			i-cache-size = <32768>;
 			i-cache-line-size = <64>;
@@ -135,6 +137,7 @@ cpu_l2: cpu@200 {
 			enable-method = "psci";
 			capacity-dmips-mhz = <530>;
 			clocks = <&scmi_clk SCMI_CLK_CPUL>;
+			operating-points-v2 = <&cluster0_opp_table>;
 			cpu-idle-states = <&CPU_SLEEP>;
 			i-cache-size = <32768>;
 			i-cache-line-size = <64>;
@@ -154,6 +157,7 @@ cpu_l3: cpu@300 {
 			enable-method = "psci";
 			capacity-dmips-mhz = <530>;
 			clocks = <&scmi_clk SCMI_CLK_CPUL>;
+			operating-points-v2 = <&cluster0_opp_table>;
 			cpu-idle-states = <&CPU_SLEEP>;
 			i-cache-size = <32768>;
 			i-cache-line-size = <64>;
@@ -175,6 +179,7 @@ cpu_b0: cpu@400 {
 			clocks = <&scmi_clk SCMI_CLK_CPUB01>;
 			assigned-clocks = <&scmi_clk SCMI_CLK_CPUB01>;
 			assigned-clock-rates = <816000000>;
+			operating-points-v2 = <&cluster1_opp_table>;
 			cpu-idle-states = <&CPU_SLEEP>;
 			i-cache-size = <65536>;
 			i-cache-line-size = <64>;
@@ -194,6 +199,7 @@ cpu_b1: cpu@500 {
 			enable-method = "psci";
 			capacity-dmips-mhz = <1024>;
 			clocks = <&scmi_clk SCMI_CLK_CPUB01>;
+			operating-points-v2 = <&cluster1_opp_table>;
 			cpu-idle-states = <&CPU_SLEEP>;
 			i-cache-size = <65536>;
 			i-cache-line-size = <64>;
@@ -215,6 +221,7 @@ cpu_b2: cpu@600 {
 			clocks = <&scmi_clk SCMI_CLK_CPUB23>;
 			assigned-clocks = <&scmi_clk SCMI_CLK_CPUB23>;
 			assigned-clock-rates = <816000000>;
+			operating-points-v2 = <&cluster2_opp_table>;
 			cpu-idle-states = <&CPU_SLEEP>;
 			i-cache-size = <65536>;
 			i-cache-line-size = <64>;
@@ -234,6 +241,7 @@ cpu_b3: cpu@700 {
 			enable-method = "psci";
 			capacity-dmips-mhz = <1024>;
 			clocks = <&scmi_clk SCMI_CLK_CPUB23>;
+			operating-points-v2 = <&cluster2_opp_table>;
 			cpu-idle-states = <&CPU_SLEEP>;
 			i-cache-size = <65536>;
 			i-cache-line-size = <64>;
@@ -348,6 +356,120 @@ l3_cache: l3-cache {
 		};
 	};
 
+	cluster0_opp_table: opp-table-cluster0 {
+		compatible = "operating-points-v2";
+		opp-shared;
+
+		opp-1008000000 {
+			opp-hz = /bits/ 64 <1008000000>;
+			opp-microvolt = <675000 675000 950000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-1200000000 {
+			opp-hz = /bits/ 64 <1200000000>;
+			opp-microvolt = <712500 712500 950000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-1416000000 {
+			opp-hz = /bits/ 64 <1416000000>;
+			opp-microvolt = <762500 762500 950000>;
+			clock-latency-ns = <40000>;
+			opp-suspend;
+		};
+		opp-1608000000 {
+			opp-hz = /bits/ 64 <1608000000>;
+			opp-microvolt = <850000 850000 950000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-1800000000 {
+			opp-hz = /bits/ 64 <1800000000>;
+			opp-microvolt = <950000 950000 950000>;
+			clock-latency-ns = <40000>;
+		};
+	};
+
+	cluster1_opp_table: opp-table-cluster1 {
+		compatible = "operating-points-v2";
+		opp-shared;
+
+		opp-1200000000 {
+			opp-hz = /bits/ 64 <1200000000>;
+			opp-microvolt = <675000 675000 1000000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-1416000000 {
+			opp-hz = /bits/ 64 <1416000000>;
+			opp-microvolt = <725000 725000 1000000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-1608000000 {
+			opp-hz = /bits/ 64 <1608000000>;
+			opp-microvolt = <762500 762500 1000000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-1800000000 {
+			opp-hz = /bits/ 64 <1800000000>;
+			opp-microvolt = <850000 850000 1000000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-2016000000 {
+			opp-hz = /bits/ 64 <2016000000>;
+			opp-microvolt = <925000 925000 1000000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-2208000000 {
+			opp-hz = /bits/ 64 <2208000000>;
+			opp-microvolt = <987500 987500 1000000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-2400000000 {
+			opp-hz = /bits/ 64 <2400000000>;
+			opp-microvolt = <1000000 1000000 1000000>;
+			clock-latency-ns = <40000>;
+		};
+	};
+
+	cluster2_opp_table: opp-table-cluster2 {
+		compatible = "operating-points-v2";
+		opp-shared;
+
+		opp-1200000000 {
+			opp-hz = /bits/ 64 <1200000000>;
+			opp-microvolt = <675000 675000 1000000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-1416000000 {
+			opp-hz = /bits/ 64 <1416000000>;
+			opp-microvolt = <725000 725000 1000000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-1608000000 {
+			opp-hz = /bits/ 64 <1608000000>;
+			opp-microvolt = <762500 762500 1000000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-1800000000 {
+			opp-hz = /bits/ 64 <1800000000>;
+			opp-microvolt = <850000 850000 1000000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-2016000000 {
+			opp-hz = /bits/ 64 <2016000000>;
+			opp-microvolt = <925000 925000 1000000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-2208000000 {
+			opp-hz = /bits/ 64 <2208000000>;
+			opp-microvolt = <987500 987500 1000000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-2400000000 {
+			opp-hz = /bits/ 64 <2400000000>;
+			opp-microvolt = <1000000 1000000 1000000>;
+			clock-latency-ns = <40000>;
+		};
+	};
+
 	firmware {
 		optee: optee {
 			compatible = "linaro,optee-tz";

-- 
2.43.0

Re: [PATCH v2 3/4] arm64: dts: rockchip: Add OPP data for CPU cores on RK3588

[Heiko Stübner]

Am Dienstag, 30. Januar 2024, 19:21:15 CET schrieb Alexey Charkov:
> By default the CPUs on RK3588 start up in a conservative performance
> mode. Add frequency and voltage mappings to the device tree to enable
> dynamic scaling via cpufreq

Please add a paragraph describing where the opp values comes from.
Probably just the vendor kernel, which is fine, but I really like to
document that these values have some sort of grounds ;-)


Thanks
Heiko

Re: [PATCH v2 3/4] arm64: dts: rockchip: Add OPP data for CPU cores on RK3588

[Alexey Charkov]

On Wed, Jan 31, 2024 at 1:12 PM Heiko Stübner <heiko@sntech.de> wrote:
>
> Am Dienstag, 30. Januar 2024, 19:21:15 CET schrieb Alexey Charkov:
> > By default the CPUs on RK3588 start up in a conservative performance
> > mode. Add frequency and voltage mappings to the device tree to enable
> > dynamic scaling via cpufreq
>
> Please add a paragraph describing where the opp values comes from.
> Probably just the vendor kernel, which is fine, but I really like to
> document that these values have some sort of grounds ;-)

Will do, thank you! Yes, these are from the vendor kernel, namely from
Radxa's version [1].

[1] https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588s.dtsi

Best regards,
Alexey

[PATCH v2 4/4] arm64: dts: rockchip: Add further granularity in RK3588 CPU OPPs

[Alexey Charkov]

This introduces additional OPPs that share the same voltage as
another OPP already present in the .dtsi but with lower frequency.

The idea is to try and limit system throughput more gradually upon
reaching the throttling condition for workloads that are close to
sustainable power already, thus avoiding needless performance loss.

My limited synthetic benchmarking [1] showed around 3.8% performance
benefit when these are in place, other things equal (not meant to
be comprehensive though).

[1] https://lore.kernel.org/linux-rockchip/CABjd4YxqarUCbZ-a2XLe3TWJ-qjphGkyq=wDnctnEhdoSdPPpw@mail.gmail.com/T/#me92aa0ee25e6eeb1d1501ce85f5af4e58b3b13c5

Signed-off-by: Alexey Charkov <alchark@gmail.com>
---
 arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 87 +++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
index af8b932a04c1..506676985a7e 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
@@ -360,6 +360,21 @@ cluster0_opp_table: opp-table-cluster0 {
 		compatible = "operating-points-v2";
 		opp-shared;
 
+		opp-408000000 {
+			opp-hz = /bits/ 64 <408000000>;
+			opp-microvolt = <675000 675000 950000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-600000000 {
+			opp-hz = /bits/ 64 <600000000>;
+			opp-microvolt = <675000 675000 950000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-816000000 {
+			opp-hz = /bits/ 64 <816000000>;
+			opp-microvolt = <675000 675000 950000>;
+			clock-latency-ns = <40000>;
+		};
 		opp-1008000000 {
 			opp-hz = /bits/ 64 <1008000000>;
 			opp-microvolt = <675000 675000 950000>;
@@ -392,6 +407,27 @@ cluster1_opp_table: opp-table-cluster1 {
 		compatible = "operating-points-v2";
 		opp-shared;
 
+		opp-408000000 {
+			opp-hz = /bits/ 64 <408000000>;
+			opp-microvolt = <675000 675000 1000000>;
+			clock-latency-ns = <40000>;
+			opp-suspend;
+		};
+		opp-600000000 {
+			opp-hz = /bits/ 64 <600000000>;
+			opp-microvolt = <675000 675000 1000000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-816000000 {
+			opp-hz = /bits/ 64 <816000000>;
+			opp-microvolt = <675000 675000 1000000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-1008000000 {
+			opp-hz = /bits/ 64 <1008000000>;
+			opp-microvolt = <675000 675000 1000000>;
+			clock-latency-ns = <40000>;
+		};
 		opp-1200000000 {
 			opp-hz = /bits/ 64 <1200000000>;
 			opp-microvolt = <675000 675000 1000000>;
@@ -422,6 +458,21 @@ opp-2208000000 {
 			opp-microvolt = <987500 987500 1000000>;
 			clock-latency-ns = <40000>;
 		};
+		opp-2256000000 {
+			opp-hz = /bits/ 64 <2256000000>;
+			opp-microvolt = <1000000 1000000 1000000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-2304000000 {
+			opp-hz = /bits/ 64 <2304000000>;
+			opp-microvolt = <1000000 1000000 1000000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-2352000000 {
+			opp-hz = /bits/ 64 <2352000000>;
+			opp-microvolt = <1000000 1000000 1000000>;
+			clock-latency-ns = <40000>;
+		};
 		opp-2400000000 {
 			opp-hz = /bits/ 64 <2400000000>;
 			opp-microvolt = <1000000 1000000 1000000>;
@@ -433,6 +484,27 @@ cluster2_opp_table: opp-table-cluster2 {
 		compatible = "operating-points-v2";
 		opp-shared;
 
+		opp-408000000 {
+			opp-hz = /bits/ 64 <408000000>;
+			opp-microvolt = <675000 675000 1000000>;
+			clock-latency-ns = <40000>;
+			opp-suspend;
+		};
+		opp-600000000 {
+			opp-hz = /bits/ 64 <600000000>;
+			opp-microvolt = <675000 675000 1000000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-816000000 {
+			opp-hz = /bits/ 64 <816000000>;
+			opp-microvolt = <675000 675000 1000000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-1008000000 {
+			opp-hz = /bits/ 64 <1008000000>;
+			opp-microvolt = <675000 675000 1000000>;
+			clock-latency-ns = <40000>;
+		};
 		opp-1200000000 {
 			opp-hz = /bits/ 64 <1200000000>;
 			opp-microvolt = <675000 675000 1000000>;
@@ -463,6 +535,21 @@ opp-2208000000 {
 			opp-microvolt = <987500 987500 1000000>;
 			clock-latency-ns = <40000>;
 		};
+		opp-2256000000 {
+			opp-hz = /bits/ 64 <2256000000>;
+			opp-microvolt = <1000000 1000000 1000000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-2304000000 {
+			opp-hz = /bits/ 64 <2304000000>;
+			opp-microvolt = <1000000 1000000 1000000>;
+			clock-latency-ns = <40000>;
+		};
+		opp-2352000000 {
+			opp-hz = /bits/ 64 <2352000000>;
+			opp-microvolt = <1000000 1000000 1000000>;
+			clock-latency-ns = <40000>;
+		};
 		opp-2400000000 {
 			opp-hz = /bits/ 64 <2400000000>;
 			opp-microvolt = <1000000 1000000 1000000>;

-- 
2.43.0

Re: [PATCH v2 4/4] arm64: dts: rockchip: Add further granularity in RK3588 CPU OPPs

[Dragan Simic]

Hello Alexey,

On 2024-01-30 19:21, Alexey Charkov wrote:
> This introduces additional OPPs that share the same voltage as
> another OPP already present in the .dtsi but with lower frequency.
> 
> The idea is to try and limit system throughput more gradually upon
> reaching the throttling condition for workloads that are close to
> sustainable power already, thus avoiding needless performance loss.
> 
> My limited synthetic benchmarking [1] showed around 3.8% performance
> benefit when these are in place, other things equal (not meant to
> be comprehensive though).

I'm fine with this two-patch approach, so this important new feature
can be merged quicker, hopefully in the current merge window.  We can
add more OPPs later, after the additional testing is performed, of
course if all checks out as expected.

> [1] 
> https://lore.kernel.org/linux-rockchip/CABjd4YxqarUCbZ-a2XLe3TWJ-qjphGkyq=wDnctnEhdoSdPPpw@mail.gmail.com/T/#me92aa0ee25e6eeb1d1501ce85f5af4e58b3b13c5
> 
> Signed-off-by: Alexey Charkov <alchark@gmail.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 87 
> +++++++++++++++++++++++++++++++
>  1 file changed, 87 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> index af8b932a04c1..506676985a7e 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> @@ -360,6 +360,21 @@ cluster0_opp_table: opp-table-cluster0 {
>  		compatible = "operating-points-v2";
>  		opp-shared;
> 
> +		opp-408000000 {
> +			opp-hz = /bits/ 64 <408000000>;
> +			opp-microvolt = <675000 675000 950000>;
> +			clock-latency-ns = <40000>;
> +		};
> +		opp-600000000 {
> +			opp-hz = /bits/ 64 <600000000>;
> +			opp-microvolt = <675000 675000 950000>;
> +			clock-latency-ns = <40000>;
> +		};
> +		opp-816000000 {
> +			opp-hz = /bits/ 64 <816000000>;
> +			opp-microvolt = <675000 675000 950000>;
> +			clock-latency-ns = <40000>;
> +		};
>  		opp-1008000000 {
>  			opp-hz = /bits/ 64 <1008000000>;
>  			opp-microvolt = <675000 675000 950000>;
> @@ -392,6 +407,27 @@ cluster1_opp_table: opp-table-cluster1 {
>  		compatible = "operating-points-v2";
>  		opp-shared;
> 
> +		opp-408000000 {
> +			opp-hz = /bits/ 64 <408000000>;
> +			opp-microvolt = <675000 675000 1000000>;
> +			clock-latency-ns = <40000>;
> +			opp-suspend;
> +		};
> +		opp-600000000 {
> +			opp-hz = /bits/ 64 <600000000>;
> +			opp-microvolt = <675000 675000 1000000>;
> +			clock-latency-ns = <40000>;
> +		};
> +		opp-816000000 {
> +			opp-hz = /bits/ 64 <816000000>;
> +			opp-microvolt = <675000 675000 1000000>;
> +			clock-latency-ns = <40000>;
> +		};
> +		opp-1008000000 {
> +			opp-hz = /bits/ 64 <1008000000>;
> +			opp-microvolt = <675000 675000 1000000>;
> +			clock-latency-ns = <40000>;
> +		};
>  		opp-1200000000 {
>  			opp-hz = /bits/ 64 <1200000000>;
>  			opp-microvolt = <675000 675000 1000000>;
> @@ -422,6 +458,21 @@ opp-2208000000 {
>  			opp-microvolt = <987500 987500 1000000>;
>  			clock-latency-ns = <40000>;
>  		};
> +		opp-2256000000 {
> +			opp-hz = /bits/ 64 <2256000000>;
> +			opp-microvolt = <1000000 1000000 1000000>;
> +			clock-latency-ns = <40000>;
> +		};
> +		opp-2304000000 {
> +			opp-hz = /bits/ 64 <2304000000>;
> +			opp-microvolt = <1000000 1000000 1000000>;
> +			clock-latency-ns = <40000>;
> +		};
> +		opp-2352000000 {
> +			opp-hz = /bits/ 64 <2352000000>;
> +			opp-microvolt = <1000000 1000000 1000000>;
> +			clock-latency-ns = <40000>;
> +		};
>  		opp-2400000000 {
>  			opp-hz = /bits/ 64 <2400000000>;
>  			opp-microvolt = <1000000 1000000 1000000>;
> @@ -433,6 +484,27 @@ cluster2_opp_table: opp-table-cluster2 {
>  		compatible = "operating-points-v2";
>  		opp-shared;
> 
> +		opp-408000000 {
> +			opp-hz = /bits/ 64 <408000000>;
> +			opp-microvolt = <675000 675000 1000000>;
> +			clock-latency-ns = <40000>;
> +			opp-suspend;
> +		};
> +		opp-600000000 {
> +			opp-hz = /bits/ 64 <600000000>;
> +			opp-microvolt = <675000 675000 1000000>;
> +			clock-latency-ns = <40000>;
> +		};
> +		opp-816000000 {
> +			opp-hz = /bits/ 64 <816000000>;
> +			opp-microvolt = <675000 675000 1000000>;
> +			clock-latency-ns = <40000>;
> +		};
> +		opp-1008000000 {
> +			opp-hz = /bits/ 64 <1008000000>;
> +			opp-microvolt = <675000 675000 1000000>;
> +			clock-latency-ns = <40000>;
> +		};
>  		opp-1200000000 {
>  			opp-hz = /bits/ 64 <1200000000>;
>  			opp-microvolt = <675000 675000 1000000>;
> @@ -463,6 +535,21 @@ opp-2208000000 {
>  			opp-microvolt = <987500 987500 1000000>;
>  			clock-latency-ns = <40000>;
>  		};
> +		opp-2256000000 {
> +			opp-hz = /bits/ 64 <2256000000>;
> +			opp-microvolt = <1000000 1000000 1000000>;
> +			clock-latency-ns = <40000>;
> +		};
> +		opp-2304000000 {
> +			opp-hz = /bits/ 64 <2304000000>;
> +			opp-microvolt = <1000000 1000000 1000000>;
> +			clock-latency-ns = <40000>;
> +		};
> +		opp-2352000000 {
> +			opp-hz = /bits/ 64 <2352000000>;
> +			opp-microvolt = <1000000 1000000 1000000>;
> +			clock-latency-ns = <40000>;
> +		};
>  		opp-2400000000 {
>  			opp-hz = /bits/ 64 <2400000000>;
>  			opp-microvolt = <1000000 1000000 1000000>;

Re: [PATCH v2 4/4] arm64: dts: rockchip: Add further granularity in RK3588 CPU OPPs

[Dragan Simic]

Hello Alexey,

On 2024-01-31 06:08, Dragan Simic wrote:
> On 2024-01-30 19:21, Alexey Charkov wrote:
>> This introduces additional OPPs that share the same voltage as
>> another OPP already present in the .dtsi but with lower frequency.
>> 
>> The idea is to try and limit system throughput more gradually upon
>> reaching the throttling condition for workloads that are close to
>> sustainable power already, thus avoiding needless performance loss.
>> 
>> My limited synthetic benchmarking [1] showed around 3.8% performance
>> benefit when these are in place, other things equal (not meant to
>> be comprehensive though).
> 
> I'm fine with this two-patch approach, so this important new feature
> can be merged quicker, hopefully in the current merge window.  We can
> add more OPPs later, after the additional testing is performed, of
> course if all checks out as expected.

Thanks to Radxa providing a sample Rock 5B to me, I'll be able to
join the testing in the new few days, or maybe early next week.
Looking forward to the test results. :)

>> [1] 
>> https://lore.kernel.org/linux-rockchip/CABjd4YxqarUCbZ-a2XLe3TWJ-qjphGkyq=wDnctnEhdoSdPPpw@mail.gmail.com/T/#me92aa0ee25e6eeb1d1501ce85f5af4e58b3b13c5
>> 
>> Signed-off-by: Alexey Charkov <alchark@gmail.com>
>> ---
>>  arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 87 
>> +++++++++++++++++++++++++++++++
>>  1 file changed, 87 insertions(+)
>> 
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>> b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>> index af8b932a04c1..506676985a7e 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
>> @@ -360,6 +360,21 @@ cluster0_opp_table: opp-table-cluster0 {
>>  		compatible = "operating-points-v2";
>>  		opp-shared;
>> 
>> +		opp-408000000 {
>> +			opp-hz = /bits/ 64 <408000000>;
>> +			opp-microvolt = <675000 675000 950000>;
>> +			clock-latency-ns = <40000>;
>> +		};
>> +		opp-600000000 {
>> +			opp-hz = /bits/ 64 <600000000>;
>> +			opp-microvolt = <675000 675000 950000>;
>> +			clock-latency-ns = <40000>;
>> +		};
>> +		opp-816000000 {
>> +			opp-hz = /bits/ 64 <816000000>;
>> +			opp-microvolt = <675000 675000 950000>;
>> +			clock-latency-ns = <40000>;
>> +		};
>>  		opp-1008000000 {
>>  			opp-hz = /bits/ 64 <1008000000>;
>>  			opp-microvolt = <675000 675000 950000>;
>> @@ -392,6 +407,27 @@ cluster1_opp_table: opp-table-cluster1 {
>>  		compatible = "operating-points-v2";
>>  		opp-shared;
>> 
>> +		opp-408000000 {
>> +			opp-hz = /bits/ 64 <408000000>;
>> +			opp-microvolt = <675000 675000 1000000>;
>> +			clock-latency-ns = <40000>;
>> +			opp-suspend;
>> +		};
>> +		opp-600000000 {
>> +			opp-hz = /bits/ 64 <600000000>;
>> +			opp-microvolt = <675000 675000 1000000>;
>> +			clock-latency-ns = <40000>;
>> +		};
>> +		opp-816000000 {
>> +			opp-hz = /bits/ 64 <816000000>;
>> +			opp-microvolt = <675000 675000 1000000>;
>> +			clock-latency-ns = <40000>;
>> +		};
>> +		opp-1008000000 {
>> +			opp-hz = /bits/ 64 <1008000000>;
>> +			opp-microvolt = <675000 675000 1000000>;
>> +			clock-latency-ns = <40000>;
>> +		};
>>  		opp-1200000000 {
>>  			opp-hz = /bits/ 64 <1200000000>;
>>  			opp-microvolt = <675000 675000 1000000>;
>> @@ -422,6 +458,21 @@ opp-2208000000 {
>>  			opp-microvolt = <987500 987500 1000000>;
>>  			clock-latency-ns = <40000>;
>>  		};
>> +		opp-2256000000 {
>> +			opp-hz = /bits/ 64 <2256000000>;
>> +			opp-microvolt = <1000000 1000000 1000000>;
>> +			clock-latency-ns = <40000>;
>> +		};
>> +		opp-2304000000 {
>> +			opp-hz = /bits/ 64 <2304000000>;
>> +			opp-microvolt = <1000000 1000000 1000000>;
>> +			clock-latency-ns = <40000>;
>> +		};
>> +		opp-2352000000 {
>> +			opp-hz = /bits/ 64 <2352000000>;
>> +			opp-microvolt = <1000000 1000000 1000000>;
>> +			clock-latency-ns = <40000>;
>> +		};
>>  		opp-2400000000 {
>>  			opp-hz = /bits/ 64 <2400000000>;
>>  			opp-microvolt = <1000000 1000000 1000000>;
>> @@ -433,6 +484,27 @@ cluster2_opp_table: opp-table-cluster2 {
>>  		compatible = "operating-points-v2";
>>  		opp-shared;
>> 
>> +		opp-408000000 {
>> +			opp-hz = /bits/ 64 <408000000>;
>> +			opp-microvolt = <675000 675000 1000000>;
>> +			clock-latency-ns = <40000>;
>> +			opp-suspend;
>> +		};
>> +		opp-600000000 {
>> +			opp-hz = /bits/ 64 <600000000>;
>> +			opp-microvolt = <675000 675000 1000000>;
>> +			clock-latency-ns = <40000>;
>> +		};
>> +		opp-816000000 {
>> +			opp-hz = /bits/ 64 <816000000>;
>> +			opp-microvolt = <675000 675000 1000000>;
>> +			clock-latency-ns = <40000>;
>> +		};
>> +		opp-1008000000 {
>> +			opp-hz = /bits/ 64 <1008000000>;
>> +			opp-microvolt = <675000 675000 1000000>;
>> +			clock-latency-ns = <40000>;
>> +		};
>>  		opp-1200000000 {
>>  			opp-hz = /bits/ 64 <1200000000>;
>>  			opp-microvolt = <675000 675000 1000000>;
>> @@ -463,6 +535,21 @@ opp-2208000000 {
>>  			opp-microvolt = <987500 987500 1000000>;
>>  			clock-latency-ns = <40000>;
>>  		};
>> +		opp-2256000000 {
>> +			opp-hz = /bits/ 64 <2256000000>;
>> +			opp-microvolt = <1000000 1000000 1000000>;
>> +			clock-latency-ns = <40000>;
>> +		};
>> +		opp-2304000000 {
>> +			opp-hz = /bits/ 64 <2304000000>;
>> +			opp-microvolt = <1000000 1000000 1000000>;
>> +			clock-latency-ns = <40000>;
>> +		};
>> +		opp-2352000000 {
>> +			opp-hz = /bits/ 64 <2352000000>;
>> +			opp-microvolt = <1000000 1000000 1000000>;
>> +			clock-latency-ns = <40000>;
>> +		};
>>  		opp-2400000000 {
>>  			opp-hz = /bits/ 64 <2400000000>;
>>  			opp-microvolt = <1000000 1000000 1000000>;
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip