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

[Dragan Simic]

Hello Alexey,

On 2024-01-26 00:13, Dragan Simic wrote:
> On 2024-01-24 21:30, 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 55C
> >
> > Signed-off-by: Alexey Charkov <alchark@xxxxxxxxx>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 25 
> > ++++++++++++++++++++++++-
> >  1 file changed, 24 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 9b7bf6cec8bd..c4c94e0b6163 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>;
> > @@ -180,6 +180,25 @@ &cpu_l3 {
> >  	cpu-supply = <&vdd_cpu_lit_s0>;
> >  };
> >
> > +&package_thermal {
> > +	polling-delay = <1000>;
> > +
> > +	trips {
> > +		package_fan: package-fan {
> > +			temperature = <55000>;
> > +			hysteresis = <2000>;
> > +			type = "active";
> > +		};
> > +	};
> > +
> > +	cooling-maps {
> > +		map-fan {
> > +			trip = <&package_fan>;
> > +			cooling-device = <&fan THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> > +		};
> > +	};
> > +};
>
> It should be better to have two new trips and two new cooling maps
> defined, instead of having just one trip/map pair, like this:
>
> &package_thermal {
> 	polling-delay = <1000>;
>
> 	trips {
> 		package_warm: package-warm {
> 			temperature = <55000>;
> 			hysteresis = <2000>;
> 			type = "active";
> 		};
>
> 		package_hot: package-hot {
> 			temperature = <65000>;
> 			hysteresis = <2000>;
> 			type = "active";
> 		};
> 	};
>
> 	cooling-maps {
> 		mapX {
> 			trip = <&package_warm>;
> 			cooling-device = <&fan THERMAL_NO_LIMIT 1>;
> 		};
>
> 		mapY {
> 			trip = <&package_hot>;
> 			cooling-device = <&fan 2 THERMAL_NO_LIMIT>;
> 		};
> 	};
> };
>
> The idea behind this approach is to keep the fan spinning at the lowest
> available speed until the package temperature reaches the second trip's
> temperature level, at which point the fan starts ramping up.  An 
> approach
> like this is already employed by the Pine64 RockPro64 SBC.
>
> This way, we'll be doing our best to keep the fan noise down;  of 
> course,
> it will depend on the particular heatsink and fan combo how long the 
> fan
> can be kept at the lowest speed, but we should aim at supporting as 
> many
> different cooling setups as possible, and as well as possible, out of 
> the
> box and with no additional tweaking required.
>
> Please notice "mapX" and "mapY" as the names of the additional cooling 
> maps,
> where X and Y are simply the next lowest available indices, which is 
> pretty
> much the usual way to name the additional cooling maps.

Just checking, have you seen this?  Quite a few messages were exchanged
on the same day, so just wanted to make sure you didn't miss this one.

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