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 v2 47/48] ARM: tegra: ventana: Support CPU voltage scaling
 and thermal throttling
To:     Daniel Lezcano <daniel.lezcano@linaro.org>,
        Thierry Reding <thierry.reding@gmail.com>,
        Jonathan Hunter <jonathanh@nvidia.com>,
        Mark Brown <broonie@kernel.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Ulf Hansson <ulf.hansson@linaro.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Peter Geis <pgwipeout@gmail.com>,
        Nicolas Chauvet <kwizart@gmail.com>,
        Krzysztof Kozlowski <krzk@kernel.org>,
        "Rafael J. Wysocki" <rjw@rjwysocki.net>,
        Kevin Hilman <khilman@kernel.org>,
        Peter De Schrijver <pdeschrijver@nvidia.com>,
        Viresh Kumar <vireshk@kernel.org>,
        Stephen Boyd <sboyd@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>
Cc:     devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, dri-devel@lists.freedesktop.org,
        linux-media@vger.kernel.org, linux-tegra@vger.kernel.org,
        linux-clk@vger.kernel.org
References: <20201217180638.22748-1-digetx@gmail.com>
 <20201217180638.22748-48-digetx@gmail.com>
 <91139f8b-0b83-dd8a-ba53-8e7a499e6344@linaro.org>
From:   Dmitry Osipenko <digetx@gmail.com>
Message-ID: <b39ebfc1-42b3-1fa7-efe4-6ecbc8cfcb50@gmail.com>
Date:   Thu, 17 Dec 2020 22:01:01 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.4.2
MIME-Version: 1.0
In-Reply-To: <91139f8b-0b83-dd8a-ba53-8e7a499e6344@linaro.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

17.12.2020 21:28, Daniel Lezcano пишет:
> On 17/12/2020 19:06, Dmitry Osipenko wrote:
>> Enable CPU voltage scaling and thermal throttling on Tegra20 Ventana board.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  arch/arm/boot/dts/tegra20-ventana.dts | 40 ++++++++++++++++++++++++++-
>>  1 file changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
>> index 14ace2ef749c..c2d9f38960bc 100644
>> --- a/arch/arm/boot/dts/tegra20-ventana.dts
>> +++ b/arch/arm/boot/dts/tegra20-ventana.dts
>> @@ -2,8 +2,10 @@
>>  /dts-v1/;
>>  
>>  #include <dt-bindings/input/input.h>
>> +#include <dt-bindings/thermal/thermal.h>
>>  #include "tegra20.dtsi"
>>  #include "tegra20-cpu-opp.dtsi"
>> +#include "tegra20-cpu-opp-microvolt.dtsi"
>>  
>>  / {
>>  	model = "NVIDIA Tegra20 Ventana evaluation board";
>> @@ -527,9 +529,10 @@ ldo_rtc {
>>  			};
>>  		};
>>  
>> -		temperature-sensor@4c {
>> +		nct1008: temperature-sensor@4c {
>>  			compatible = "onnn,nct1008";
>>  			reg = <0x4c>;
>> +			#thermal-sensor-cells = <1>;
>>  		};
>>  	};
>>  
>> @@ -615,10 +618,13 @@ clk32k_in: clock@0 {
>>  
>>  	cpus {
>>  		cpu0: cpu@0 {
>> +			cpu-supply = <&vdd_cpu>;
>>  			operating-points-v2 = <&cpu0_opp_table>;
>> +			#cooling-cells = <2>;
>>  		};
>>  
>>  		cpu@1 {
>> +			cpu-supply = <&vdd_cpu>;
>>  			operating-points-v2 = <&cpu0_opp_table>;
>>  		};
>>  	};
>> @@ -717,4 +723,36 @@ sound {
>>  			 <&tegra_car TEGRA20_CLK_CDEV1>;
>>  		clock-names = "pll_a", "pll_a_out0", "mclk";
>>  	};
>> +
>> +	thermal-zones {
>> +		cpu-thermal {
>> +			polling-delay-passive = <1000>; /* milliseconds */
>> +			polling-delay = <5000>; /* milliseconds */
>> +
>> +			thermal-sensors = <&nct1008 1>;
>> +
>> +			trips {
>> +				trip0: cpu-alert0 {
>> +					/* start throttling at 50C */
>> +					temperature = <50000>;
>> +					hysteresis = <200>;
> 
> Did you mean <2000> ?

The <200> is correct.

Please see this commit for example:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2b22393b27670b45a2c870bce3df6579efd9a86a

>> +					type = "passive";
>> +				};
>> +
>> +				trip1: cpu-crit {
>> +					/* shut down at 60C */
>> +					temperature = <60000>;
>> +					hysteresis = <2000>;
> 
> I think you can drop the hysteresis here, when the critical temperature
> is reached, there is an emergency shutdown.

Yes, perhaps you're right. Hysteresis doesn't make sense for a critical
trip. I'll improve it in the next revision, thanks.

> 50°C and 60°C sound very low values, no ?

Tegra20 CPU is very cold in comparison to later generations, it should
stay under 50°C even at a full load.

>> +					type = "critical";
>> +				};
>> +			};
>> +
>> +			cooling-maps {
>> +				map0 {
>> +					trip = <&trip0>;
>> +					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> 
> You should add all CPUs here.


All CPU cores are coupled on Tegra in regards to CPUFreq, hence I think
it won't make any difference if secondary CPU cores will be added here,
isn't it?