linux-kernel - Re: [PATCH v4 1/6] arm64: dts: rockchip: add thermal zones information on RK3588

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 6 May 2024 14:29:41 +0400
From: Alexey Charkov <alchark@...il.com>
To: Dragan Simic <dsimic@...jaro.org>
Cc: 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>, Chen-Yu Tsai <wens@...nel.org>, 
	Diederik de Haas <didi.debian@...ow.org>, devicetree@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 1/6] arm64: dts: rockchip: add thermal zones
 information on RK3588

Hello Dragan,

On Mon, May 6, 2024 at 1:52 PM Dragan Simic <dsimic@...jaro.org> wrote:
>
> Hello Alexey,
>
> Thanks for submitting the v4 of this series!  Please, see a couple
> of my comments below.
>
> On 2024-05-06 11:36, Alexey Charkov wrote:
> > This includes the necessary device tree data to allow thermal
> > monitoring on RK3588(s) using the on-chip TSADC device, along with
> > trip points for automatic thermal management.
> >
> > Each of the CPU clusters (one for the little cores and two for
> > the big cores) get a passive cooling trip point at 85C, which
> > will trigger DVFS throttling of the respective cluster upon
> > reaching a high temperature condition.
> >
> > All zones also have a critical trip point at 115C, which will
> > trigger a reset.
> >
> > Signed-off-by: Alexey Charkov <alchark@...il.com>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 147
> > ++++++++++++++++++++++++++++++
> >  1 file changed, 147 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > index 6ac5ac8b48ab..ef06c1f742e8 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > @@ -10,6 +10,7 @@
> >  #include <dt-bindings/reset/rockchip,rk3588-cru.h>
> >  #include <dt-bindings/phy/phy.h>
> >  #include <dt-bindings/ata/ahci.h>
> > +#include <dt-bindings/thermal/thermal.h>
> >
> >  / {
> >       compatible = "rockchip,rk3588";
> > @@ -2368,6 +2369,152 @@ pwm15: pwm@...f0030 {
> >               status = "disabled";
> >       };
> >
> > +     thermal_zones: thermal-zones {
> > +             /* sensor near the center of the SoC */
> > +             package_thermal: package-thermal {
> > +                     polling-delay-passive = <0>;
> > +                     polling-delay = <0>;
> > +                     thermal-sensors = <&tsadc 0>;
> > +
> > +                     trips {
> > +                             package_crit: package-crit {
> > +                                     temperature = <115000>;
> > +                                     hysteresis = <0>;
> > +                                     type = "critical";
> > +                             };
> > +                     };
> > +             };
> > +
> > +             /* sensor between A76 cores 0 and 1 */
> > +             bigcore0_thermal: bigcore0-thermal {
> > +                     polling-delay-passive = <100>;
> > +                     polling-delay = <0>;
> > +                     thermal-sensors = <&tsadc 1>;
> > +
> > +                     trips {
> > +                             bigcore0_alert: bigcore0-alert {
> > +                                     temperature = <85000>;
> > +                                     hysteresis = <2000>;
> > +                                     type = "passive";
> > +                             };
>
> Doesn't removing the second passive trip, which was present in the v3,
> result in confusing the IPA governor?

Not really - it will just treat the missing trip as 0C for its initial
PID calculations [1], and will continually run the governor as opposed
to putting it to rest when the temperature is below the "switch on"
value [2].

Getting the power allocation governor to work optimally (i.e. to
provide tangible benefits over, say, stepwise) is much more involved
than defining an arbitrary switch-on trip point, as it requires an
accurate estimate of sustainable power per thermal zone (which we
don't have for RK3588 in general, and furthermore it must depend a lot
on a particular cooling setup), and ideally some userspace
power/thermal model capable of tuning the PID coefficients and
updating them via sysfs based on how a particular system accumulates
and dissipates heat under different load.

So after thinking over it for a while I decided that those extra
passive trips were rather self-deceiving, as they are only useful in
the context of a power allocation governor but we do not have any of
the other pieces in place for the power allocation governor to work.
Better not to clutter the device tree IMO.

Best regards,
Alexey

[1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/thermal/gov_power_allocator.c#n156
[2] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/thermal/gov_power_allocator.c#n487