Received-SPF: pass (google.com: domain of linux-kernel+bounces-39401-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
Date: Fri, 26 Jan 2024 00:13:05 +0100
From: Dragan Simic <dsimic@manjaro.org>
To: Alexey Charkov <alchark@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>,
 Heiko Stuebner <heiko@sntech.de>, Daniel Lezcano
 <daniel.lezcano@linaro.org>, devicetree@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org, linux-rockchip@lists.infradead.org,
 linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/4] arm64: dts: rockchip: enable temperature driven fan
 control on Rock 5B
In-Reply-To: <20240125-rk-dts-additions-v1-3-5879275db36f@gmail.com>
References: <20240125-rk-dts-additions-v1-0-5879275db36f@gmail.com>
 <20240125-rk-dts-additions-v1-3-5879275db36f@gmail.com>
Message-ID: <df062818d21f3318c033859d0e95efc7@manjaro.org>
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit

Hello Alexey,

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@gmail.com>
> ---
>  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.

>  &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";