* [PATCH] arm64: dts: allwinner: Add GPU thermal trips to the SoC dtsi for A64
@ 2024-08-12 2:00 Dragan Simic
2024-08-12 2:40 ` Icenowy Zheng
0 siblings, 1 reply; 7+ messages in thread
From: Dragan Simic @ 2024-08-12 2:00 UTC (permalink / raw)
To: linux-sunxi
Cc: wens, jernej.skrabec, samuel, linux-arm-kernel, devicetree, robh,
krzk+dt, conor+dt, linux-kernel, uwu, wenst, broonie
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.
Signed-off-by: Dragan Simic <dsimic@manjaro.org>
---
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 */
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";
+ };
+ };
};
};
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: dts: allwinner: Add GPU thermal trips to the SoC dtsi for A64
2024-08-12 2:00 [PATCH] arm64: dts: allwinner: Add GPU thermal trips to the SoC dtsi for A64 Dragan Simic
@ 2024-08-12 2:40 ` Icenowy Zheng
2024-08-12 2:46 ` Dragan Simic
0 siblings, 1 reply; 7+ messages in thread
From: Icenowy Zheng @ 2024-08-12 2:40 UTC (permalink / raw)
To: Dragan Simic, linux-sunxi
Cc: wens, jernej.skrabec, samuel, linux-arm-kernel, devicetree, robh,
krzk+dt, conor+dt, linux-kernel, wenst, broonie
在 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.
>
> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
> ---
> 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.
> 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";
> + };
> + };
> };
> };
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: dts: allwinner: Add GPU thermal trips to the SoC dtsi for A64
2024-08-12 2:40 ` Icenowy Zheng
@ 2024-08-12 2:46 ` Dragan Simic
2024-08-19 15:42 ` Daniel Lezcano
2024-08-20 3:37 ` Chen-Yu Tsai
0 siblings, 2 replies; 7+ messages in thread
From: Dragan Simic @ 2024-08-12 2:46 UTC (permalink / raw)
To: Icenowy Zheng
Cc: linux-sunxi, wens, jernej.skrabec, samuel, linux-arm-kernel,
devicetree, robh, krzk+dt, conor+dt, linux-kernel, wenst, broonie
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@manjaro.org>
>> ---
>> 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.
>> 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";
>> + };
>> + };
>> };
>> };
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: dts: allwinner: Add GPU thermal trips to the SoC dtsi for A64
2024-08-12 2:46 ` Dragan Simic
@ 2024-08-19 15:42 ` Daniel Lezcano
2024-08-20 3:26 ` Dragan Simic
2024-08-20 3:37 ` Chen-Yu Tsai
1 sibling, 1 reply; 7+ messages in thread
From: Daniel Lezcano @ 2024-08-19 15:42 UTC (permalink / raw)
To: Dragan Simic, Icenowy Zheng
Cc: linux-sunxi, wens, jernej.skrabec, samuel, linux-arm-kernel,
devicetree, robh, krzk+dt, conor+dt, linux-kernel, wenst, broonie
On 12/08/2024 04:46, Dragan Simic 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@manjaro.org>
>>> ---
>>> 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.
You can remove also polling-delay-passive and polling-passive when they
are equal to zero. If they are absent they will be set to zero by default.
That said, I take the opportunity to spot some inconsistency in this DT
not related to this change.
1. There is a passive trip point and one cooling device mapped to it.
With a polling-delay-passive=0, the mitigation will fail
2. There is a second mapping for the hot trip point. That does not make
sense, it is not possible because there is no mitigation for 'hot' and
'critical' trip points.
--
<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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: dts: allwinner: Add GPU thermal trips to the SoC dtsi for A64
2024-08-19 15:42 ` Daniel Lezcano
@ 2024-08-20 3:26 ` Dragan Simic
0 siblings, 0 replies; 7+ messages in thread
From: Dragan Simic @ 2024-08-20 3:26 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Icenowy Zheng, linux-sunxi, wens, jernej.skrabec, samuel,
linux-arm-kernel, devicetree, robh, krzk+dt, conor+dt,
linux-kernel, wenst, broonie
Hello Daniel,
On 2024-08-19 17:42, Daniel Lezcano wrote:
> On 12/08/2024 04:46, Dragan Simic wrote:
>> 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@manjaro.org>
>>>> ---
>>>> 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.
>
> You can remove also polling-delay-passive and polling-passive when
> they are equal to zero. If they are absent they will be set to zero by
> default.
Good point, thanks! Though, I'd rather leave those "... = <0>;"
removals for a small follow-up series, because those changes touch
more actual code than just the comments, so it's better to keep them
as separate changes for easier bisection later, if it's ever needed.
Hopefully never. :)
I just made a note for myself to create and submit those follow-up
cleanup patches later, for all affected Allwinner and Rockchip SoC
dtsi files.
> That said, I take the opportunity to spot some inconsistency in this
> DT not related to this change.
>
> 1. There is a passive trip point and one cooling device mapped to it.
> With a polling-delay-passive=0, the mitigation will fail
Huh, how is the CPU throttling working then? Thanks for pointing it
out, I'll address this issue in the follow-up patches.
> 2. There is a second mapping for the hot trip point. That does not
> make sense, it is not possible because there is no mitigation for
> 'hot' and 'critical' trip points.
Yup, I see no special handling of tz->ops.hot, so having the hot trip
point makes no sense. Thanks again for pointing it out, I'll address
this issue in the follow-up patches as well.
I'll send the v2 soon, as a small patch series, and I'll send a few
follow-up patches later.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: dts: allwinner: Add GPU thermal trips to the SoC dtsi for A64
2024-08-12 2:46 ` Dragan Simic
2024-08-19 15:42 ` Daniel Lezcano
@ 2024-08-20 3:37 ` Chen-Yu Tsai
2024-08-20 3:41 ` Dragan Simic
1 sibling, 1 reply; 7+ messages in thread
From: Chen-Yu Tsai @ 2024-08-20 3:37 UTC (permalink / raw)
To: Dragan Simic
Cc: Icenowy Zheng, linux-sunxi, jernej.skrabec, samuel,
linux-arm-kernel, devicetree, robh, krzk+dt, conor+dt,
linux-kernel, wenst, broonie
On Mon, Aug 12, 2024 at 10:46 AM Dragan Simic <dsimic@manjaro.org> 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@manjaro.org>
> >> ---
> >> 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.
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.
ChenYu
> >> 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";
> >> + };
> >> + };
> >> };
> >> };
> >>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] arm64: dts: allwinner: Add GPU thermal trips to the SoC dtsi for A64
2024-08-20 3:37 ` Chen-Yu Tsai
@ 2024-08-20 3:41 ` Dragan Simic
0 siblings, 0 replies; 7+ messages in thread
From: Dragan Simic @ 2024-08-20 3:41 UTC (permalink / raw)
To: wens
Cc: Icenowy Zheng, linux-sunxi, jernej.skrabec, samuel,
linux-arm-kernel, devicetree, robh, krzk+dt, conor+dt,
linux-kernel, wenst, broonie
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@manjaro.org>
> 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@manjaro.org>
>> >> ---
>> >> 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";
>> >> + };
>> >> + };
>> >> };
>> >> };
>> >>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-08-20 3:41 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-12 2:00 [PATCH] arm64: dts: allwinner: Add GPU thermal trips to the SoC dtsi for A64 Dragan Simic
2024-08-12 2:40 ` Icenowy Zheng
2024-08-12 2:46 ` Dragan Simic
2024-08-19 15:42 ` Daniel Lezcano
2024-08-20 3:26 ` Dragan Simic
2024-08-20 3:37 ` Chen-Yu Tsai
2024-08-20 3:41 ` Dragan Simic
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox