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

[Dragan Simic <dsimic@manjaro.org>]

On 2024-05-06 12:29, Alexey Charkov wrote:
> On Mon, May 6, 2024 at 1:52 PM Dragan Simic <dsimic@manjaro.org> wrote:
>> 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@gmail.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@febf0030 {
>> >               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.

I see, thanks for the clarification.  Please, give me some time
to thoroughly test your patches, which I'll hopefully be able to
do in the next few days.

> [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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip