Aw: Re: [PATCH] arm: dts: mt7623: increase passive cooling trip

[kurt at va1der.ca]

Hi Frank,

On 2022-03-05 12:37, Frank Wunderlich wrote:

> does your Patch fixes the issue? if yes you can send a Patch based on
> current mainline dts.

I should apologize.  I didn't put the kind of scrutiny into this that I 
should have before proposing a kernel patch.

Looking at this dts more deeply, I see that I was mislead by Mediatek's 
use of "active" and "passive" trip points.  An active trip point is 
supposed to only be for fans and the like.  But in this case, looking at 
the cooling maps the trip points reference, I see passive and active 
(and hot as well) are all mapped to the same exact CPU scaling 
mechanism.  They even all have the same unlimited scaling range.  Which 
means two of them can't ever have any effect.  I suspect at one point 
they had hardware fan support in their development cycle which was 
deleted.

So we can't swap passive/active with any effect.  But I still think even 
57°C is a little low to lock the CPU at, since it's not a dangerous 
temperature and it's not difficult to get devices that warm.  I suggest 
we delete the current "Active" trip point entirely, and then have only 
three trip points starting at 67°C.  I have two test boards (BPI R2 and 
UniElec U7623) each running a kernel with this setup now and it works 
quite well.

    Kurt

> 
> it looks a bit weired to me having active before passive...but i'm no
> expert in the cooling trips.
> i just increased the temps :)
> 
> please see notes below
> 
>> On 2021-07-25 1:34 p.m., Frank Wunderlich wrote:
>> > From: Frank Wunderlich <frank-w at public-files.de>
>> >
>> > MT7623/BPI-R2 has idle temperature after bootup from 48 degrees celsius
>> > increase the passive trip temp threshold to not trottle CPU frequency at
>> > this temperature
>> 
>> The dts file for the mt7623 has four trip points:
>> - Trip point 0, Type=Passive @ 47°C (originally, now 57°C)
>> - Trip point 1, Type=Active @ 67°C
>> - Trip point 2, Type=Hot @ 87°C
>> - Trip point 3, Type=Critical @ 107°C
>> 
>> It makes little sense to have passive CPU throttling employed *before*
>> engaging a fan.  Generally users want a fan (if present) to be engaged
>> before intentional performance degradation is employed.
>> 
>> >   					cpu_passive: cpu-passive {
>> > -						temperature = <47000>;
>> > +						temperature = <57000>;
>> 
>> The old temperature of 47°C appears, in practice, to be a good
>> temperature for an active cooling trip point.  It is just above the
>> temperature that a generally idle mt7623 sits at in enclosures where
>> this SoC is employed, but also a temperature that a CPU under load 
>> will
>> quickly reach.
>> 
>> Therefore I propose the types of trip points 0 and 1 be reversed and 
>> the
>> temperatures left as they were.
>> 
>> 
>> --- a/arch/arm/boot/dts/mt7623.dtsi	2022-02-27 18:36:33 -0400
>> +++ b/arch/arm/boot/dts/mt7623.dtsi	2022-03-05 11:37:52 -0400
>> @@ -158,19 +158,19 @@
>> 
>>   				thermal-sensors = <&thermal 0>;
>> 
>>   				trips {
>>   					cpu_passive: cpu-passive {
>> -						temperature = <57000>;
>> +						temperature = <47000>;
>>   						hysteresis = <2000>;
>> -						type = "passive";
>> +						type = "active";
>>   					};
> 
> if type is different, imho the name+label should be changed too
> 
>>   					cpu_active: cpu-active {
>>   						temperature = <67000>;
>>   						hysteresis = <2000>;
>> -						type = "active";
>> +						type = "passive";
>>   					};
> 
> dito
> 
>>   					cpu_hot: cpu-hot {
>>   						temperature = <87000>;
>>   						hysteresis = <2000>;
> 
> regards Frank