linux-kernel - Re: [PATCH v3 1/5] arm64: dts: rockchip: enable built-in thermal monitoring 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]

Message-ID: <3f73d847fbe9d7f6dd05802eb7e47d49@manjaro.org>
Date: Sat, 02 Mar 2024 19:38:14 +0100
From: Dragan Simic <dsimic@...jaro.org>
To: Heiko Stuebner <heiko@...ech.de>
Cc: Rob Herring <robh+dt@...nel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
 Alexey Charkov <alchark@...il.com>, Daniel Lezcano
 <daniel.lezcano@...aro.org>, Viresh Kumar <viresh.kumar@...aro.org>, Chen-Yu
 Tsai <wens@...nel.org>, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/5] arm64: dts: rockchip: enable built-in thermal
 monitoring on RK3588

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@...il.com>
>> ---
>>  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@...00000 {
>>  		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.