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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux