On Wed, Jan 31, 2024 at 9:05 AM Dragan Simic <dsimic@xxxxxxxxxxx> 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@xxxxxxxxx> > > --- > > 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>;