[PATCH v2 2/4] arm64: dts: rockchip: enable temperature driven fan control on Rock 5B
Dragan Simic
dsimic at manjaro.org
Thu Feb 1 11:43:01 PST 2024
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 at 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 at 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 at linaro.org>
>>> >> Signed-off-by: Alexey Charkov <alchark at 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.
More information about the linux-arm-kernel
mailing list