Message-ID: <57bb477925efc74a0ea11b20321fa661@manjaro.org>
Date: Fri, 21 Mar 2025 03:19:30 +0100
From: Dragan Simic <dsimic@...jaro.org>
To: Sam Edwards <cfsworks@...il.com>
Cc: Heiko Stuebner <heiko@...ech.de>, linux-rockchip@...ts.infradead.org,
 linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
 Daniel Kukieła <daniel@...iela.pl>, Sven Rademakers
 <sven.rademakers@...il.com>, Joshua Riek <jjriek@...izon.net>
Subject: Re: [PATCH] arm64: dts: rockchip: Allow Turing RK1 cooling fan to
 spin down

Hello Sam,

On 2025-03-21 02:20, Sam Edwards wrote:
> On Thu, Mar 20, 2025 at 5:38 PM Dragan Simic <dsimic@...jaro.org> 
> wrote:
>> On 2025-03-15 21:48, Sam Edwards wrote:
>> > The RK3588 thermal sensor driver only receives interrupts when a
>> > higher-temperature threshold is crossed; it cannot notify when the
>> > sensor cools back off. As a result, the driver must poll for
>> > temperature
>> > changes to detect when the conditions for a thermal trip are no longer
>> > met. However, it only does so if the DT enables polling.
>> >
>> > Before this patch, the RK1 DT did not enable polling, causing the fan
>> > to
>> > continue running at the speed corresponding to the highest temperature
>> > reached.
>> >
>> > Follow suit with similar RK3588 boards by setting a polling-delay of
>> > 1000ms, enabling the driver to detect when the sensor cools back off,
>> > allowing the fan speed to decrease as appropriate.
>> >
>> > Fixes: 7c8ec5e6b9d6 ("arm64: dts: rockchip: Enable automatic fan
>> > control on Turing RK1")
>> > Signed-off-by: Sam Edwards <CFSworks@...il.com>
>> > ---
>> >  arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi | 2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi
>> > b/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi
>> > index 6bc46734cc14..0270bffce195 100644
>> > --- a/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi
>> > +++ b/arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi
>> > @@ -214,6 +214,8 @@ rgmii_phy: ethernet-phy@1 {
>> >  };
>> >
>> >  &package_thermal {
>> > +     polling-delay = <1000>;
>> > +
>> >       trips {
>> >               package_active1: trip-active1 {
>> >                       temperature = <45000>;
>> 
>> Thanks for the patch, it's looking good to me, with some related
>> thoughts below.  Please, feel free to include:
>> 
>> Reviewed-by: Dragan Simic <dsimic@...jaro.org>
>> 
>> After a quick look at the RK3588 TRM Part 1, it seems possible
>> to actually generate additional interrupts when the TSADC channel
>> temperature readouts reach predefined low thresholds.  Moreover,
> 
> It seems to be as simple as adding a `.set_cooloff_temp = ...` that
> writes the lower thresholds to TSADC_COMP#_LOW_INT and sets the
> necessary bits in TSADC_LT_EN+TSADC_LT_INT_EN. Since the driver
> already rescans all temperatures on any interrupt and acknowledges all
> interrupt status bits indiscriminately, I don't anticipate any other
> necessary changes.

Hmm, I'm afraid it isn't that simple...  See, the entire thermal
framework is rather complex and there may be hidden issues, because
such handling of the cooldown events probably isn't the most common
variant on all boards supported upstream.

Even if everything is fine there, with the dynamic reassigning of
the cooldown thresholds and everything else, which hopefully is, :)
generating and handling the LT_INT interrupts, at the first glance,
requires rather major changes to the driver because it introduces
a different class of interrupts, for which the amount of required
changes isn't exactly small.

> I can easily take care of that this weekend or next if that plan
> sounds good to you. However, since *this* patch is a Fixes:, I'd
> rather land it as-is first and handle the above separately. :)

I totally agree that this patch needs to get merged first, perhaps
even as a v2 that would include "Cc: stable" as well.

Regarding adding support for LT_INT interrupts, frankly, I'd much
rather leave that do be done as part of my upcoming major rework
of the OPPs by introducing the SoC binning.  As part of that, I'll
do a whole lot of testing on different boards with different SoCs
and their different variants, which should ensure that everything
works as expected, including the LT_INT stuff.

>> avoiding the polling would actually help the SoC cool down a tiny
>> bit faster, which makes it worth detailed investigation in my book,
>> despite not being used by the downstream kernel code.
> 
> Do you mean "spin down the fan a tiny bit faster" (since it would
> detect the cool-off without needing to poll for it), or are you
> emphasizing saving CPU cycles that would otherwise be spent polling?

Technically both, :) but of course, the primary goal would be to make
the CPU not do what it doesn't really have to, and spinning down the
fan faster would be just an additional benefit.  It isn't going to
make some huge difference in the CPU load (or better said, in the
"background noise"), but again, every tiny bit counts.

When it comes to the HT_INT interrupts, they're obviously much more
important, because there must not be any delays when (over)heating
happens.  That isn't so critical with the LT_INT interrupts, which
is probably the reason why the downstream kernel code uses HT_INT
interrupts only.

Powered by blists - more mailing lists