Re: [PATCH 2/3] thermal/drivers/amlogic: add S4 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] thermal/drivers/amlogic: missing S4 bus clock in binding and compatible
--

commit dfe5abf7967aecb95213a29d43ea6efa65ceb5d9
Author: Christian Stefan Kövér-Draxl <ckoever@outlook.de>

thermal/drivers/amlogic: add S4 support

This commit adds the amlogic,s4-cpu-thermal compatible string to the Amlogic
thermal driver match table, reusing the T7 parameter data so that the S4 CPU
thermal sensor reads its calibration via the secure monitor.

> diff --git a/drivers/thermal/amlogic_thermal.c b/drivers/thermal/amlogic_thermal.c
> index 60728de6968ac..c58bac633bedc 100644
> --- a/drivers/thermal/amlogic_thermal.c
> +++ b/drivers/thermal/amlogic_thermal.c
> @@ -292,6 +292,10 @@ static const struct of_device_id of_amlogic_thermal_match[] = {
>  		.compatible = "amlogic,a1-cpu-thermal",
>  		.data = &amlogic_thermal_a1_cpu_param,
>  	},
> +	{
> +		.compatible = "amlogic,s4-cpu-thermal",
> +		.data = &amlogic_thermal_t7_param,
> +	},

[Severity: Medium]
Should S4 use amlogic,s4-thermal (either with amlogic,t7-thermal as a
fallback compatible or added to the two-clock condition in the binding)
instead of a standalone amlogic,s4-cpu-thermal compatible?

In drivers/clk/meson/s4-peripherals.c, Amlogic S4 provides both a shared
functional clock (s4_ts_clk) and dedicated per-sensor APB bus clocks for the
PLL/CPU and DDR thermal sensors:

static S4_PCLK(s4_ts_ddr,	CLKCTRL_SYS_CLK_EN0_REG1, 15, CLK_IGNORE_UNUSED);
static S4_PCLK(s4_ts_pll,	CLKCTRL_SYS_CLK_EN0_REG1, 16, CLK_IGNORE_UNUSED);

However, in Documentation/devicetree/bindings/thermal/amlogic,thermal.yaml,
the allOf conditional block only permits two clocks when the compatible
contains amlogic,t7-thermal:

  - if:
      properties:
        compatible:
          contains:
            const: amlogic,t7-thermal
    then:
      properties:
        clocks:
          minItems: 2
      required:
        - clock-names
    else:
      properties:
        clocks:
          maxItems: 1
        clock-names: false

Because amlogic,s4-cpu-thermal falls into the else branch, the schema forbids
specifying the per-sensor APB bus clock (CLKID_TS_PLL / CLKID_TS_DDR) alongside
CLKID_TS. If the bootloader does not leave the APB bus clock enabled, or once
CLK_IGNORE_UNUSED is removed from s4_ts_pll and s4_ts_ddr, register accesses in
amlogic_thermal_enable() and amlogic_thermal_get_temp() during
amlogic_thermal_probe() can access an unclocked peripheral.

Additionally, since S4 passes the sensor index via the amlogic,secure-monitor
phandle argument rather than hardcoding u_efuse_off per compatible, does
naming the compatible amlogic,s4-cpu-thermal prevent reusing it for the S4 DDR
thermal sensor?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025192244.209256-1-christian.koever-draxl@student.uibk.ac.at?part=2