On Wed, Jan 31, 2024 at 9:08 AM Dragan Simic <dsimic@xxxxxxxxxxx> 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@xxxxxxxxxx>
> > Signed-off-by: Alexey Charkov <alchark@xxxxxxxxx>
> > ---
> > 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
