Re: [PATCH v3 1/5] arm64: dts: rockchip: enable built-in thermal monitoring on RK3588

[Dragan Simic]

Hello Heiko,

On 2024-03-02 12:25, Heiko Stuebner wrote:
> Am Donnerstag, 29. Februar 2024, 20:26:32 CET schrieb Alexey Charkov:
> > Include thermal zones information in device tree for RK3588 variants.
> >
> > This also enables the TSADC controller unconditionally on all boards
> > to ensure that thermal protections are in place via throttling and
> > emergency reset, once OPPs are added to enable CPU DVFS.
> >
> > The default settings (using CRU as the emergency reset mechanism)
> > should work on all boards regardless of their wiring, as CRU resets
> > do not depend on any external components. Boards that have the TSHUT
> > signal wired to the reset line of the PMIC may opt to switch to GPIO
> > tshut mode instead (rockchip,hw-tshut-mode = <1>;)
> >
> > It seems though that downstream kernels don't use that, even for
> > those boards where the wiring allows for GPIO based tshut, such as
> > Radxa Rock 5B [1], [2], [3]
> >
> > [1] 
> > https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts#L540
> > [2] 
> > https://github.com/radxa/kernel/blob/stable-5.10-rock5/arch/arm64/boot/dts/rockchip/rk3588s.dtsi#L5433
> > [3] https://dl.radxa.com/rock5/5b/docs/hw/radxa_rock_5b_v1423_sch.pdf 
> > page 11 (TSADC_SHUT_H)
> >
> > Signed-off-by: Alexey Charkov <alchark@xxxxxxxxx>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3588s.dtsi | 176 
> > +++++++++++++++++++++++++++++-
> >  1 file changed, 175 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588s.dtsi 
> > b/arch/arm64/boot/dts/rockchip/rk3588s.dtsi
> > index 36b1b7acfe6a..9bf197358642 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";
> > @@ -2225,7 +2226,180 @@ tsadc: tsadc@fec00000 {
> >  		pinctrl-1 = <&tsadc_shut>;
> >  		pinctrl-names = "gpio", "otpout";
> >  		#thermal-sensor-cells = <1>;
> > -		status = "disabled";
> > +		status = "okay";
> > +	};
>
> so I've skimmed over the general discussion, though don't have a hard
> opinion in either direction yet. Still there are some low-hanging 
> fruit:
>
> - having the thermal-zones addition in a separate patch would allow to
>   merge the obvious stuff, while this discussion is still ongoing

Very good suggestion.

> - status=okay in a soc dtsi is wrong, because okay is the default 
> status
>   so if anything the status property should be removed
>
> In general I'm not that much of a fan of things just working 
> implicitly.
> So somehow, when someone submits a board devicetree, I expect them to
> having ensured stuff is enabled somewhat ok. So even seeing a simple
>
> 	&tsadc {
> 		status = "okay"
> 	};
>
> suggests that they have at least noticed the existence of thermal 
> stuff.

I agree that having such additional "signed-off markers", so to speak, 
in
a board dts is quite assuring.  I mean, someone implementing a new dts 
file
for a new board should simply know what needs to be done there, and 
there
should be no excuses for not checking the thermal throttling stuff.