Re: [PATCH v2] arm64: dts: rockchip: enable built-in thermal monitoring on rk3588

[Daniel Lezcano]

Hi Alexey,


On 21/01/2024 20:57, Alexey Charkov wrote:
> On Fri, Jan 19, 2024 at 8:21 PM Daniel Lezcano
> <daniel.lezcano@xxxxxxxxxx> wrote:
> Hello Daniel,
>
> Thanks a lot for your review and comments! Please see some reflections below.
>
> > On 09/01/2024 20:19, Alexey Charkov wrote:
> > > Include thermal zones information in device tree for rk3588 variants
> > > and enable the built-in thermal sensing ADC on RADXA Rock 5B
> > >
> > > Signed-off-by: Alexey Charkov <alchark@xxxxxxxxx>
> > > ---
> > > Changes in v2:
> > >    - Dropped redundant comments
> > >    - Included all CPU cores in cooling maps
> > >    - Split cooling maps into more granular ones utilizing TSADC
> > >      channels 1-3 which measure temperature by separate CPU clusters
> > >      instead of channel 0 which measures the center of the SoC die
> > > ---
> > >    .../boot/dts/rockchip/rk3588-rock-5b.dts      |   4 +
> > >    arch/arm64/boot/dts/rockchip/rk3588s.dtsi     | 151 ++++++++++++++++++
> > >    2 files changed, 155 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > > index a5a104131403..f9d540000de3 100644
> > > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > > @@ -772,3 +772,7 @@ &usb_host1_ehci {
> > >    &usb_host1_ohci {
> > >        status = "okay";
> > >    };
> > > +
> > > +&tsadc {
> > > +     status = "okay";
> > > +};
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > > index 8aa0499f9b03..8d54998d0ecc 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";
> > > @@ -2112,6 +2113,156 @@ tsadc: tsadc@fec00000 {
> > >                status = "disabled";
> > >        };
> > >
> > > +     thermal_zones: thermal-zones {
> > > +             /* sensor near the center of the whole chip */
> > > +             soc_thermal: soc-thermal {
> > > +                     polling-delay-passive = <20>;
> >
> > There is no mitigation set for this thermal zone. It is pointless to
> > specify a passive polling.
>
> Indeed, it makes sense to me. There seems to be a catch though in that
> the driver calls the generic thermal_of_zone_register during the
> initial probe, which expects both of those polling delays to be
> present in the device tree, otherwise it simply refuses to add the
> respective thermal zone, see drivers/thermal/thermal_of.c:502

Usually:

polling-delay-passive = <0>;
polling-delay = <0>;

cf:

git grep "polling-delay = <0>" arch/arm64/boot/dts

> > > +                     polling-delay = <1000>;
> >
> > The driver is interrupt driven. No need to poll.
>
> Same here as above
>
> > > +                     sustainable-power = <2100>;
> >
> > There is no mitigation with this thermal zone. Specifying a sustainable
> > power does not make sense.
>
> Thanks, will drop this in v3!
>
> > > +                     thermal-sensors = <&tsadc 0>;
> > > +
> > > +                     trips {
> > > +                             soc_crit: soc-crit {
> > > +                                     temperature = <115000>;
> > > +                                     hysteresis = <2000>;
> >
> > This trip point leads to a system shutdown / reboot. It is not necessary
> > to specify a hysteresis.
>
> Similar to the above, the generic thermal_of code refuses to add the
> trip point if it has no hysteresis property defined (regardless of the
> trip type), see drivers/thermal/thermal_of.c:109

hysteresis = <0>;


--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog