| Message ID | 20250315204852.1247992-1-CFSworks@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: rockchip: Allow Turing RK1 cooling fan to spin down | expand |
Hello Sam, 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@gmail.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@manjaro.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, 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.
On Thu, Mar 20, 2025 at 5:38 PM Dragan Simic <dsimic@manjaro.org> wrote: > > Hello Sam, > > 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@gmail.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@manjaro.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, Hi Dragan, Ooh, I see that now! 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. 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. :) > 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? Thanks for the tip, Sam
Hello Sam, On 2025-03-21 02:20, Sam Edwards wrote: > On Thu, Mar 20, 2025 at 5:38 PM Dragan Simic <dsimic@manjaro.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@gmail.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@manjaro.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.
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>;
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@gmail.com> --- arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi | 2 ++ 1 file changed, 2 insertions(+)