Message-ID: <bd6e93cf907572e86697dc10a50bfe66@manjaro.org>
Date: Thu, 01 Feb 2024 20:43:01 +0100
From: Dragan Simic <dsimic@...jaro.org>
To: Alexey Charkov <alchark@...il.com>
Cc: wens@...nel.org, Rob Herring <robh+dt@...nel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
 Heiko Stuebner <heiko@...ech.de>, Daniel Lezcano
 <daniel.lezcano@...aro.org>, Viresh Kumar <viresh.kumar@...aro.org>,
 devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/4] arm64: dts: rockchip: enable temperature driven
 fan control on Rock 5B

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@...jaro.org> 
>> wrote:
>>> On 2024-02-01 15:26, Chen-Yu Tsai wrote:
>>> > On Wed, Jan 31, 2024 at 2:22 AM Alexey Charkov <alchark@...il.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@...aro.org>
>>> >> Signed-off-by: Alexey Charkov <alchark@...il.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.

Powered by blists - more mailing lists