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

[Daniel Lezcano]

Hi Nico,

a few comments about this description.

On 02/04/2024 05:25, Nicolas Pitre wrote:
> From: Nicolas Pitre <npitre at 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)

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

[ ... ]

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