Re: [PATCH 4/9] arm64: dts: airoha: en7581: add thermal sensor nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] arm64: dts: airoha: en7581: cpu-hot trip type prevents passive CPU throttling
- [Medium] arm64: dts: airoha: en7581: thermal-sensors cell count mismatch in cpu-thermal
- [Medium] arm64: dts: airoha: en7581: thermal-sensor reg size smaller than driver offsets
--

commit 32580fdfac9a302b9b8e002dc32934288d0d3428
Author: Christian Marangi <ansuelsmth@gmail.com>

arm64: dts: airoha: en7581: add thermal sensor nodes

This commit adds the thermal-sensor node and cpu-thermal zone to the Airoha
EN7581 device tree and links the CPUs as cooling devices.

> diff --git a/arch/arm64/boot/dts/airoha/en7581.dtsi b/arch/arm64/boot/dts/airoha/en7581.dtsi
> index fbc5ee47523ca..58bd8b318910e 100644
> --- a/arch/arm64/boot/dts/airoha/en7581.dtsi
> +++ b/arch/arm64/boot/dts/airoha/en7581.dtsi

[ ... ]

> @@ -293,6 +298,39 @@ timer {
>  			     <GIC_PPI 10 IRQ_TYPE_LEVEL_LOW>;
>  	};
>  
> +	thermal-zones {
> +		cpu_thermal: cpu-thermal {
> +			polling-delay-passive = <0>;
> +			polling-delay = <0>;
> +
> +			thermal-sensors = <&thermal 0>;

[Severity: Medium]
Should thermal-sensors in cpu_thermal be <&thermal> without the trailing 0
cell?

Below, the thermal node declares #thermal-sensor-cells = <0>, matching
Documentation/devicetree/bindings/thermal/airoha,en7581-thermal.yaml:

  '#thermal-sensor-cells':
    const: 0

When of_thermal_zone_find() in drivers/thermal/thermal_of.c parses
thermal-sensors = <&thermal 0> with #thermal-sensor-cells = <0>,
of_count_phandle_with_args() consumes 0 argument cells for &thermal and
treats the trailing 0 as a second phandle entry, returning count = 2:

	count = of_count_phandle_with_args(child, "thermal-sensors",
					   "#thermal-sensor-cells");
	if (count <= 0) {
		pr_err("%pOFP: missing thermal sensor\n", child);
		return ERR_PTR(-EINVAL);
	}

	for (i = 0; i < count; i++) {

		int ret;

		ret = of_parse_phandle_with_args(child, "thermal-sensors",
						 "#thermal-sensor-cells",
						 i, &sensor_specs);

This fails dtbs_check against thermal-zones.yaml (where thermal-sensors has
maxItems: 1) as well as dtc's thermal_sensors_property check, and causes
of_parse_phandle_with_args() at index 1 to return -ENOENT if
of_thermal_zone_find() iterates past index 0.

> +
> +			trips {
> +				cpu_hot: cpu-hot {
> +					temperature = <95000>;
> +					hysteresis = <1000>;
> +					type = "hot";
> +				};
> +
> +				cpu-critical {
> +					temperature = <120000>;
> +					hysteresis = <1000>;
> +					type = "critical";
> +				};
> +			};
> +
> +			cooling-maps {
> +				map0 {
> +					trip = <&cpu_hot>;
> +					cooling-device = <&cpu0 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +							 <&cpu1 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +							 <&cpu2 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>,
> +							 <&cpu3 THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
> +				};
> +			};

[Severity: High]
With cpu_hot configured as type = "hot" and polling-delay-passive = <0>, can
passive CPU frequency throttling ever activate at 95 C?

In drivers/thermal/thermal_core.c, thermal_trip_crossed() handles
THERMAL_TRIP_HOT via handle_critical_trips() (which only calls tz->ops.hot()
if implemented, whereas en7581_thdev_ops in drivers/thermal/airoha_thermal.c
does not define .hot) without incrementing tz->passive, and
thermal_governor_trip_crossed() returns early:

	if (trip->type == THERMAL_TRIP_HOT || trip->type == THERMAL_TRIP_CRITICAL)
		return;

All in-kernel thermal governors also skip THERMAL_TRIP_HOT trips, such as
step_wise_manage() in drivers/thermal/gov_step_wise.c:

		if (trip->temperature == THERMAL_TEMP_INVALID ||
		    trip->type == THERMAL_TRIP_CRITICAL ||
		    trip->type == THERMAL_TRIP_HOT)
			continue;

As a result, the cooling devices (&cpu0 through &cpu3) mapped to &cpu_hot in
map0 are never throttled when the sensor crosses 95000 mC.

In addition, even if cpu_hot is changed to type = "passive", setting
polling-delay-passive = <0> leaves tz->passive_delay_jiffies at 0 in
monitor_thermal_zone() in drivers/thermal/thermal_core.c:

static void monitor_thermal_zone(struct thermal_zone_device *tz)
{
	if (tz->passive > 0 && tz->passive_delay_jiffies)
		thermal_zone_device_set_polling(tz, tz->passive_delay_jiffies);
	else if (tz->polling_delay_jiffies)
		thermal_zone_device_set_polling(tz, tz->polling_delay_jiffies);
}

Once the 95000 mC trip is crossed, en7581_thermal_set_trips() in
drivers/thermal/airoha_thermal.c sets the hardware interrupt thresholds to
low = 93999 mC and high = 120000 mC, so no further interrupts fire between
95 C and 120 C; without a non-zero polling-delay-passive, an incremental
governor like step_wise will only step down by a single OPP upon crossing
95 C and will not throttle further as temperature rises toward 120 C.

Should cpu_hot use type = "passive" along with a non-zero
polling-delay-passive?

> +		};
> +	};
> +
>  	clk20m: clock-20000000 {
>  		compatible = "fixed-clock";
>  		#clock-cells = <0>;
> @@ -319,6 +357,15 @@ gic: interrupt-controller@9000000 {
>  			interrupts = <GIC_PPI 9 IRQ_TYPE_LEVEL_LOW>;
>  		};
>  
> +		thermal: thermal-sensor@1efbd000 {
> +			compatible = "airoha,en7581-thermal";
> +			reg = <0x0 0x1efbd000 0x0 0xd5c>;

[Severity: Medium]
Is the reg size of 0xd5c large enough for all registers accessed by the
airoha_thermal driver?

In drivers/thermal/airoha_thermal.c, the driver defines calibration
registers up to offset 0xf28:

#define EN7581_EFUSE_TEMP_OFFSET_REG		0xf20 /* PTPSPARE0 */
#define   EN7581_EFUSE_TEMP_OFFSET		GENMASK(31, 16)
#define EN7581_PTPSPARE1			0xf24 /* PTPSPARE1 */
#define EN7581_EFUSE_TEMP_CPU_SENSOR_REG	0xf28 /* PTPSPARE2 */

During probe, en7581_thermal_probe() maps resource 0 via
devm_platform_ioremap_resource(pdev, 0) and calls
en7581_thermal_setup_adc_val(), which reads offsets 0xf20 and 0xf28 from
priv->map:

	regmap_read(priv->map, EN7581_EFUSE_TEMP_OFFSET_REG, &efuse_calib_info);
	if (efuse_calib_info) {
		priv->default_offset = FIELD_GET(EN7581_EFUSE_TEMP_OFFSET, efuse_calib_info);
		/* Different slope are applied if the sensor is used for CPU or for package */
		regmap_read(priv->map, EN7581_EFUSE_TEMP_CPU_SENSOR_REG, &cpu_sensor);

Since 0xf28..0xf2b lies beyond the declared 0xd5c region size, should reg
have a size of at least 0xf2c (as in the binding example in
Documentation/devicetree/bindings/thermal/airoha,en7581-thermal.yaml)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026095727.31708-1-ansuelsmth@gmail.com?part=4