Hello Chen-Yu, On 2024-08-20 05:37, Chen-Yu Tsai wrote:
On Mon, Aug 12, 2024 at 10:46 AM Dragan Simic <dsimic@xxxxxxxxxxx> wrote:Hello Icenowy, On 2024-08-12 04:40, Icenowy Zheng wrote: > 在 2024-08-12星期一的 04:00 +0200，Dragan Simic写道： >> Add thermal trips for the two GPU thermal sensors found in the >> Allwinner A64. >> There's only one GPU OPP defined since the commit 1428f0c19f9c >> ("arm64: dts: >> allwinner: a64: Run GPU at 432 MHz"), so defining only the critical >> thermal >> trips makes sense for the A64's two GPU thermal zones. >> >> Having these critical thermal trips defined ensures that no hot spots >> develop >> inside the SoC die that exceed the maximum junction temperature. >> That might >> have been possible before, although quite unlikely, because the CPU >> and GPU >> portions of the SoC are packed closely inside the SoC, so the >> overheating GPU >> would inevitably result in the heat soaking into the CPU portion of >> the SoC, >> causing the CPU thermal sensor to return high readings and trigger >> the CPU >> critical thermal trips. However, it's better not to rely on the heat >> soak >> and have the critical GPU thermal trips properly defined instead. >> >> While there, remove a few spotted comments that are rather redundant, >> because >> it's pretty much obvious what units are used in those places. > > This should be another individual patch, I think. Perhaps, which I already thought about, but it might also be best to simply drop the removal of those redundant comments entirely. Let's also see what will other people say. >> Signed-off-by: Dragan Simic <dsimic@xxxxxxxxxxx> >> --- >> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 22 ++++++++++++++--- >> -- >> 1 file changed, 16 insertions(+), 6 deletions(-) >> >> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> index e868ca5ae753..bc5d3a2e6c98 100644 >> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi >> @@ -212,7 +212,6 @@ timer { >> >> thermal-zones { >> cpu_thermal: cpu0-thermal { >> - /* milliseconds */ > > The unit of a 0 isn't not so obvious I think, so I suggest to keep > this. Quite frankly, I think it should be obvious to anyone tackling the thermal zones and trips.Sorry for the late reply.
No worries.
Not everyone writing or editing the DT would be fully familiar with all the bindings. As Icenowy mentioned, it's not even obvious when the existing value is zero. I would prefer we keep them.
That's two votes for keeping the already present comments, so I'll drop their removal in the v2.
>> polling-delay-passive = <0>; >> polling-delay = <0>; >> thermal-sensors = <&ths 0>; >> @@ -236,40 +235,51 @@ map1 { >> >> trips { >> cpu_alert0: cpu-alert0 { >> - /* milliCelsius */ >> temperature = <75000>; >> hysteresis = <2000>; >> type = "passive"; >> }; >> >> cpu_alert1: cpu-alert1 { >> - /* milliCelsius */ >> temperature = <90000>; >> hysteresis = <2000>; >> type = "hot"; >> }; >> >> cpu_crit: cpu-crit { >> - /* milliCelsius */ >> temperature = <110000>; >> hysteresis = <2000>; >> type = "critical"; >> }; >> }; >> }; >> >> gpu0_thermal: gpu0-thermal { >> - /* milliseconds */ >> polling-delay-passive = <0>; >> polling-delay = <0>; >> thermal-sensors = <&ths 1>; >> + >> + trips { >> + gpu0_crit: gpu0-crit { >> + temperature = <110000>; >> + hysteresis = <2000>; >> + type = "critical"; >> + }; >> + }; >> }; >> >> gpu1_thermal: gpu1-thermal { >> - /* milliseconds */ >> polling-delay-passive = <0>; >> polling-delay = <0>; >> thermal-sensors = <&ths 2>; >> + >> + trips { >> + gpu1_crit: gpu1-crit { >> + temperature = <110000>; >> + hysteresis = <2000>; >> + type = "critical"; >> + }; >> + }; >> }; >> }; >>
