Re: [PATCH v2 15/15] arm64: dts: mediatek: mt8188: add default thermal zones

[Julien Panis <jpanis@baylibre.com>]

Hello Daniel,

On 4/4/24 17:16, Daniel Lezcano wrote:
>
> Hi Nico,
>
> a few comments about this description.
>
> On 02/04/2024 05:25, Nicolas Pitre wrote:
>> From: Nicolas Pitre <npitre@baylibre.com>
>>
>> Inspired by the vendor kernel but adapted to the upstream thermal
>> driver version.
>
> [ ... ]
>
>> +    thermal_zones: thermal-zones {
>> +        cpu-little0-thermal {
>> +            polling-delay = <1000>;
>
> Except if I'm wrong, the driver supports the interrupt mode, so it not necessary to poll 
> constantly when there is no mitigation. You can remove the line and everywhere else.
>
>> +            polling-delay-passive = <250>;
>
> As little CPU, 200ms or 150ms may be more adequate.
>
>> +            thermal-sensors = <&lvts_mcu MT8188_MCU_LITTLE_CPU0>;
>> +
>> +            trips {
>> +                cpu_little0_alert: trip-alert {
>> +                    temperature = <85000>;
>> +                    hysteresis = <2000>;
>> +                    type = "passive";
>> +                };
>
> You may want to add a 'hot' trip point in between, so the userspace can be notified and take an 
> action before reaching 'critical' (like unplugging a CPU)

There's no "notify" function in the driver.
Do you think it's worth adding such 'hot' trip point, though ?

>
>> +                cpu_little0_crit: trip-crit {
>> +                    temperature = <100000>;
>> +                    hysteresis = <2000>;
>
> critical is a point of no return. Hysteresis does not make sense.
>
> These comments apply to all thermal zones.
>
> [ .. ]
>
>> +        cpu_big0-thermal {
>> +            polling-delay = <1000>;
>> +            polling-delay-passive = <250>;
>
> Same comments as the little but may be an even lower value. eg. 100ms.
>
>> +            thermal-sensors = <&lvts_mcu MT8188_MCU_BIG_CPU0>;
>> +
>> +            trips {
>> +                cpu_big0_alert: trip-alert {
>> +                    temperature = <85000>;
>> +                    hysteresis = <2000>;
>> +                    type = "passive";
>> +                };
>> +
>> +                cpu_big0_crit: trip-crit {
>> +                    temperature = <100000>;
>> +                    hysteresis = <2000>;
>> +                    type = "critical";
>> +                };
>> +            };
>> +
>> +            cooling-maps {
>> +                map0 {
>> +                    trip = <&cpu_big0_alert>;
>> +                    cooling-device = <&cpu6 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
>> +                             <&cpu7 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
>> +                };
>> +            };
>> +        };
>
> [ ... ]
>
>> +        gpu1-thermal {
>> +            polling-delay = <1000>;
>> +            polling-delay-passive = <250>;
>> +            thermal-sensors = <&lvts_ap MT8188_AP_GPU1>;
>> +
>> +            trips {
>> +                gpu1_alert: trip-alert {
>> +                    temperature = <85000>;
>> +                    hysteresis = <2000>;
>> +                    type = "passive";
>> +                };
>> +
>> +                gpu1_crit: trip-crit {
>> +                    temperature = <100000>;
>> +                    hysteresis = <2000>;
>> +                    type = "critical";
>> +                };
>> +            };
>> +        };
>> +
>> +        gpu2-thermal {
>> +            polling-delay = <1000>;
>> +            polling-delay-passive = <250>;
>> +            thermal-sensors = <&lvts_ap MT8188_AP_GPU2>;
>> +
>> +            trips {
>> +                gpu2_alert: trip-alert {
>> +                    temperature = <85000>;
>> +                    hysteresis = <2000>;
>> +                    type = "passive";
>> +                };
>> +
>> +                gpu2_crit: trip-crit {
>> +                    temperature = <100000>;
>> +                    hysteresis = <2000>;
>> +                    type = "critical";
>> +                };
>> +            };
>
> You can add a devfreq cooling device for the GPU here.

I tried but realized then that GPU support has not been added in the DT yet.
So, I added the gpu node (from vendor's kernel), but there are many missing
dependencies. I guess it should be part of another series.
As a result, I can't add such cooling device here.

Julien