Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] Fix mt7622.dtsi thermal cpu
To:     Eric Woudstra <ericwouds@gmail.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20210619121927.32699-1-ericwouds@gmail.com>
 <e30a2d01-a200-80cb-88d9-6aea62dd49f1@linaro.org>
 <56fb5540-fb86-4e6a-a596-1276026b37e5@gmail.com>
From:   Daniel Lezcano <daniel.lezcano@linaro.org>
Message-ID: <a4e41929-6ab4-fabb-741e-f25a5fd14e3b@linaro.org>
Date:   Wed, 23 Jun 2021 17:58:29 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <56fb5540-fb86-4e6a-a596-1276026b37e5@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 23/06/2021 17:35, Eric Woudstra wrote:
> It is only useful to set 1 map with the regulated temperature for cpu
> frequency throttling. Same as in the kernel document example.
> 
> 
> It has no use to set frequency scaling on 2 different temperature
> trip points, as the lowest one makes sure the higher one(s) are never
> reached.

I looked more closely the DT and there is a misunderstanding of the
thermal framework in the definition.

There is one trip point with the passive type and the cpu cooling
device, followed by a second trip point with the active type *but* the
same cpu cooling device. That is wrong.

And finally, there is the hot trip point as a third mapping and the same
cooling device.

The hot trip point is only there to notify userspace and let it take an
immediate action to prevent an emergency shutdown when reaching the
critical temperature.

> It can be applied only at 1 trip point. Multiple trip points
> is only usefully for fan control to make sure the fan is not too
> noisy when it is not necessary to be noisy.
> 
> 
> The CPU will almost come to a dead stop when it starts to pass the
> lowest thermal map with frequency throttling.
> 
> This is why it is a bug and needs a fix, not only adjustment.

Yes, you are right. It should be something like (verbatim copy):

diff --git a/arch/arm64/boot/dts/mediatek/mt7622.dtsi
b/arch/arm64/boot/dts/mediatek/mt7622.dtsi
index 890a942ec608..88c81d24f4ff 100644
--- a/arch/arm64/boot/dts/mediatek/mt7622.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt7622.dtsi
@@ -136,24 +136,18 @@ secmon_reserved: secmon@43000000 {

 	thermal-zones {
 		cpu_thermal: cpu-thermal {
-			polling-delay-passive = <1000>;
+			polling-delay-passive = <250>;
 			polling-delay = <1000>;

 			thermal-sensors = <&thermal 0>;

 			trips {
 				cpu_passive: cpu-passive {
-					temperature = <47000>;
+					temperature = <77000>;
 					hysteresis = <2000>;
 					type = "passive";
 				};

-				cpu_active: cpu-active {
-					temperature = <67000>;
-					hysteresis = <2000>;
-					type = "active";
-				};
-
 				cpu_hot: cpu-hot {
 					temperature = <87000>;
 					hysteresis = <2000>;
@@ -173,18 +167,6 @@ map0 {
 					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
 							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
 				};
-
-				map1 {
-					trip = <&cpu_active>;
-					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
-				};
-
-				map2 {
-					trip = <&cpu_hot>;
-					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
-							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
-				};
 			};
 		};
 	};


-- 
<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