Re: [PATCH v4 2/2] thermal: amlogic: Add support for A9 thermal controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Critical] Unclocked MMIO register access in probe and resume causes bus faults on legacy SoCs.
- [High] A9-specific hardware over-temperature reset is unconditionally enabled on all legacy SoCs with incorrect bitmask.
- [Medium] Missing error check for `clk_prepare_enable()` and missing clock unwind in `amlogic_thermal_resume()`.

Pre-existing issues:
- [High] Invalid manual two's complement on unsigned type breaks negative temperature calculations on 32-bit systems.
- [High] Thermal zone registered and polled before hardware clocks are enabled.
--

--- Patch [2]: [PATCH v4 2/2] thermal: amlogic: Add support for A9 thermal controller ---
commit 722761d3d79caeb86bd5f0b65fa45692c9e109e1
Author: Xianwei Zhao <xianwei.zhao@amlogic.com>

thermal: amlogic: Add support for A9 thermal controller

Add support for the Amlogic A9 thermal controller with different
calibration parameters, separate core/bus clocks, and hardware reset
capabilities.

> diff --git a/drivers/thermal/amlogic_thermal.c b/drivers/thermal/amlogic_thermal.c
> index a0b530624b60c..74f0bc49c737b 100644
> --- a/drivers/thermal/amlogic_thermal.c
> +++ b/drivers/thermal/amlogic_thermal.c

[ ... ]

> @@ -138,6 +157,49 @@ static int amlogic_thermal_code_to_millicelsius(struct amlogic_thermal *pdata,
>  	return temp;
>  }
>  
> +/*
> + * Calculate a temperature code from a temperature value .
> + * The unit of the temperature is degree milliCelsius.
> + */
> +static u32 amlogic_thermal_millicelsius_to_code(struct amlogic_thermal *pdata, int millicelsius)
> +{
> +	const struct amlogic_thermal_soc_calib_data *param =
> +					pdata->data->calibration_parameters;
> +	s64 factor, uptat, uefuse;
> +	u32 temp_code;
> +
> +	uefuse = pdata->trim_info & TSENSOR_TRIM_SIGN_MASK ?
> +			     ~(pdata->trim_info & TSENSOR_TRIM_TEMP_MASK) + 1 :
> +			     (pdata->trim_info & TSENSOR_TRIM_TEMP_MASK);

[Severity: High]
This is a pre-existing issue, but could this manual two's complement logic
break negative temperature calculations on 32-bit systems?

Because TSENSOR_TRIM_TEMP_MASK is derived from GENMASK (an unsigned long),
the bitwise NOT results in an unsigned 32-bit integer (e.g., 0xFFFFFFFB
for -5). When assigned to the 64-bit signed integer uefuse, it gets
zero-extended to 4294967291 instead of sign-extended to -5.

Since this bug was duplicated into the new amlogic_thermal_millicelsius_to_code
function from amlogic_thermal_code_to_millicelsius, should both locations
be fixed to prevent over-temperature protection from miscalculating?

> +
> +	factor = param->B + div_s64(millicelsius, 100);

[ ... ]

> +static void amlogic_tsensor_setup_hw_reset(struct amlogic_thermal *data)
> +{
> +	regmap_update_bits(data->regmap, TSENSOR_CFG_REG2, TSENSOR_CFG_REG2_REBOOT_CODE,
> +			   data->temp_code << 0x4);
> +	regmap_update_bits(data->regmap, TSENSOR_CFG_REG2,
> +			   TSENSOR_CFG_REG2_ENABLE, TSENSOR_CFG_REG2_ENABLE);

[Severity: High]
Does this write an incorrect bitmask into the TSENSOR_CFG_REG2 register?

The macro TSENSOR_CFG_REG2_ENABLE includes TSENSOR_CFG_REG2_REBOOT_TIME,
which is defined as GENMASK(23, 16). Passing this macro as both the mask
and the value to regmap_update_bits() sets all bits of GENMASK(23, 16)
to 1, hardcoding the maximum reboot time (0xFF) instead of leaving it as
a configured value.

> +}
> +

[ ... ]

> @@ -346,6 +437,17 @@ static int amlogic_thermal_probe(struct platform_device *pdev)
>  
>  	devm_thermal_add_hwmon_sysfs(&pdev->dev, pdata->tzd);

[Severity: High]
This is a pre-existing issue, but does registering the thermal zone slightly
earlier in this probe function trigger a synchronous external abort?

Calling devm_thermal_of_zone_register() causes the thermal core to
synchronously invoke the .get_temp callback to initialize trip points. This
attempts an MMIO read via regmap_read(pdata->regmap, TSENSOR_STAT0) before
amlogic_thermal_enable() is called to enable the clocks.

> +	if (pdata->data->reset_temp) {
> +		reset_temp = pdata->data->reset_temp;
> +	} else {
> +		dev_dbg(dev, "using default hardware reset temperature\n");
> +		reset_temp = TSENSOR_HW_RESET_DEFAULT_TEMP;
> +	}

[Severity: High]
Does this apply an A9-specific hardware over-temperature reset configuration
unconditionally to all legacy SoCs (G12A, A1, T7)?

Older platforms may not support the TSENSOR_CFG_REG2 register, which could
lead to hardware misconfiguration or unintended reboots.

> +
> +	pdata->temp_code = amlogic_thermal_millicelsius_to_code(pdata, reset_temp);
> +
> +	amlogic_tsensor_setup_hw_reset(pdata);
> +
>  	ret = amlogic_thermal_enable(pdata);

[Severity: Critical]
Will calling amlogic_tsensor_setup_hw_reset() before amlogic_thermal_enable()
trigger a synchronous external abort (bus hang) on legacy SoCs?

amlogic_tsensor_setup_hw_reset() performs an MMIO regmap write to
TSENSOR_CFG_REG2. On platforms where the thermal IP relies entirely on
pdata->clk, this clock is left disabled until amlogic_thermal_enable()
is called on the very next line, which means we are writing to hardware
registers while the IP's clock is gated.

>  
>  	return ret;

[ ... ]

> @@ -371,6 +475,9 @@ static int amlogic_thermal_resume(struct device *dev)
>  {
>  	struct amlogic_thermal *data = dev_get_drvdata(dev);
>  
> +	if (data->data->has_sysclk)
> +		clk_prepare_enable(data->sysclk);

[Severity: Medium]
Is an error check missing for clk_prepare_enable() here?

If it fails, the driver will proceed to access hardware, resulting in a bus
fault.

Additionally, if sysclk is enabled successfully but the subsequent
amlogic_thermal_enable() fails, the function returns the error code without
disabling sysclk, which leaks the clock enable count.

> +	amlogic_tsensor_setup_hw_reset(data);
>  	return amlogic_thermal_enable(data);

[Severity: Critical]
Similar to the probe function, does amlogic_tsensor_setup_hw_reset()
perform an unclocked MMIO write here?

amlogic_thermal_enable() prepares and enables the core thermal clock, but
it is called after the hardware reset configuration is written.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825-a9-thermal-v4-0-d8fc9859cf22@amlogic.com?part=2