diff mbox series

[v5,4/5] thermal/drivers/sun8i: Add support for A523 THS0/1 controllers

Message ID 20260704171411.1413349-5-iuncuim@gmail.com (mailing list archive)
State New
Headers
Series Allwinner: A523: add support for A523 THS0/1 controllers |

Commit Message

Mikhail Kalashnikov July 4, 2026, 5:14 p.m. UTC 
The A523 processor has two temperature controllers, THS0 and THS1.
THS0 has only one temperature sensor, which is located in the DRAM
controller. THS1 does have 3 sensors:
ths1_0 - "big" cores
ths1_1 - "little" cores
ths1_2 - gpu

The datasheet mentions a fourth sensor in the NPU, but lacks any registers
for operation other than calibration registers. The vendor code reads the
value from ths1_2, but uses separate calibration data, so we get two
different values from real one.

Signed-off-by: Mikhail Kalashnikov <iuncuim@gmail.com>
Reviewed-by: Chen-Yu Tsai <wens@kernel.org>
---
 drivers/thermal/sun8i_thermal.c | 133 ++++++++++++++++++++++++++++++++
 1 file changed, 133 insertions(+)

Comments

sashiko-bot@kernel.org July 4, 2026, 5:29 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The calibration check `if (!caldata[0])` will always fail and abort calibration if bits 0-15 of the A523 eFuse are zero.
- [High] NULL pointer dereference if the `nvmem-cell-names` property is missing from the Device Tree.
- [Medium] Out-of-bounds heap read due to missing `callen` validation in `sun55i_a523_ths_calibrate`.
- [Medium] Calibration logic incorrectly hardcodes `SUN55I_A523_SCALE_ABOVE` to convert temperature error into a register offset.
--

commit ed04dff3819d46435d0d0c6fa95dfd782738eb1e
Author: Mikhail Kalashnikov <iuncuim@gmail.com>

thermal/drivers/sun8i: Add support for A523 THS0/1 controllers

The A523 processor has two temperature controllers, THS0 and THS1.
THS0 has only one temperature sensor, which is located in the DRAM
controller. THS1 does have 3 sensors:
ths1_0 - "big" cores
ths1_1 - "little" cores
ths1_2 - gpu

The datasheet mentions a fourth sensor in the NPU, but lacks any registers
for operation other than calibration registers. The vendor code reads the
value from ths1_2, but uses separate calibration data, so we get two
different values from real one.

> diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
> index 3bdd62aa8bdf1..f48ed9eae6c4f 100644
> --- a/drivers/thermal/sun8i_thermal.c
> +++ b/drivers/thermal/sun8i_thermal.c

[ ... ]

> @@ -299,6 +314,97 @@ static int sun50i_h6_ths_calibrate(struct ths_device *tmdev,

[ ... ]

> +static int sun55i_a523_ths_calibrate(struct ths_device *tmdev,
> +				     u16 *caldata, int callen)
> +{
> +	struct device *dev = tmdev->dev;
> +	int i, ft_temp;
> +
> +	if (!caldata[0])

[Severity: High]
If the device tree lacks the nvmem-cell-names property, the loop inside
sun8i_ths_calibrate() will be skipped entirely. This leaves caldata as
NULL when it is passed to the chip-specific calibrate callback:

sun8i_ths_calibrate() {
    ...
    of_property_for_each_string(...) {
        ...
    }
    tmdev->chip->calibrate(tmdev, (u16 *)caldata, callen);
    ...
}

Will checking !caldata[0] here cause a NULL pointer dereference in that
scenario?

[Severity: High]
According to the eFuse layout diagram in the comment block just above this
function, bits 0-15 are empty/unused, while the actual factory temperature
is stored in bits 24-35 (caldata[1] and caldata[2]).

If those unused eFuse bits are typically zero, will this check silently
abort the calibration process on the A523?

> +		return -EINVAL;
> +
> +	ft_temp = (((caldata[2] << 8) | (caldata[1] >> 8)) & FT_TEMP_MASK) * 100;
> +
> +	for (i = 0; i < tmdev->chip->sensor_num; i++) {
> +		int sensor_reg, sensor_temp, cdata, offset;
> +		/*
> +		 * Chips ths0 and ths1 have common parameters for value
> +		 * calibration. To separate them we can use the number of
> +		 * temperature sensors on each chip.
> +		 * For ths0 this value is 1.
> +		 */
> +		if (tmdev->chip->sensor_num == 1) {
> +			sensor_reg = ((caldata[5] >> 8) | (caldata[6] << 8)) & TEMP_CALIB_MASK;

[Severity: Medium]
Is there a chance we might read out of bounds here if the provided nvmem
data is smaller than expected?

If the device tree or nvmem cell provides fewer bytes than the 14 bytes
required to access caldata[6], this could result in an out-of-bounds heap
read.

Should we validate callen before accessing these offsets?

> +		} else {
> +			switch (i) {
> +			case 0:
> +				sensor_reg = (caldata[2] >> 4) & TEMP_CALIB_MASK;
> +				break;

[ ... ]

> +		sensor_temp = tmdev->chip->calc_temp(tmdev, i, sensor_reg);
> +
> +		/*
> +		 * Calibration data is CALIBRATE_DEFAULT - (calculated
> +		 * temperature from sensor reading at factory temperature
> +		 * minus actual factory temperature) * X (scale from
> +		 * temperature to register values)
> +		 */
> +		cdata = CALIBRATE_DEFAULT -
> +			((sensor_temp - ft_temp) / SUN55I_A523_SCALE_ABOVE);

[Severity: Medium]
The sun55i_a523_calc_temp() function uses a piecewise formula depending
on the register value, applying either SUN55I_A523_SCALE_ABOVE or
SUN55I_A523_SCALE_BELOW.

However, the calibration offset calculation here unconditionally divides
the temperature error by SUN55I_A523_SCALE_ABOVE.

If the factory temperature corresponds to a register value in the
SUN55I_A523_SCALE_BELOW region, will dividing by the SCALE_ABOVE
value result in an inaccurate calibration offset?

> +
> +		if (cdata & ~TEMP_CALIB_MASK) {
> +			/*
> +			 * Calibration value more than 12-bit, but calibration
> +			 * register is 12-bit. In this case, ths hardware can
Chen-Yu Tsai July 21, 2026, 3:23 p.m. UTC | #2 
On Sun, Jul 5, 2026 at 1:15 AM Mikhail Kalashnikov <iuncuim@gmail.com> wrote:
>
> The A523 processor has two temperature controllers, THS0 and THS1.
> THS0 has only one temperature sensor, which is located in the DRAM
> controller. THS1 does have 3 sensors:
> ths1_0 - "big" cores
> ths1_1 - "little" cores
> ths1_2 - gpu
>
> The datasheet mentions a fourth sensor in the NPU, but lacks any registers
> for operation other than calibration registers. The vendor code reads the
> value from ths1_2, but uses separate calibration data, so we get two
> different values from real one.

Are you sure? The calibration data is written to the sensor register.

The BSP simply reads the sensor data from the GPU sensor, and passes it
off as the value for the NPU sensor. No extra calculation involving the
calibration data is done.

>
> Signed-off-by: Mikhail Kalashnikov <iuncuim@gmail.com>
> Reviewed-by: Chen-Yu Tsai <wens@kernel.org>
> ---
>  drivers/thermal/sun8i_thermal.c | 133 ++++++++++++++++++++++++++++++++
>  1 file changed, 133 insertions(+)
>
> diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
> index 3bdd62aa8..f48ed9eae 100644
> --- a/drivers/thermal/sun8i_thermal.c
> +++ b/drivers/thermal/sun8i_thermal.c
> @@ -59,6 +59,12 @@
>  #define SUN50I_H6_THS_PC_TEMP_PERIOD(x)                ((GENMASK(19, 0) & (x)) << 12)
>  #define SUN50I_H6_THS_DATA_IRQ_STS(x)          BIT(x)
>
> +#define SUN55I_A523_DELIMITER                  0x7c8
> +#define SUN55I_A523_OFFSET_ABOVE               2736
> +#define SUN55I_A523_OFFSET_BELOW               2825
> +#define SUN55I_A523_SCALE_ABOVE                        74
> +#define SUN55I_A523_SCALE_BELOW                        65
> +
>  struct tsensor {
>         struct ths_device               *tmdev;
>         struct thermal_zone_device      *tzd;
> @@ -114,6 +120,15 @@ static int sun50i_h5_calc_temp(struct ths_device *tmdev,
>                 return -1590 * reg / 10 + 276000;
>  }
>
> +static int sun55i_a523_calc_temp(struct ths_device *tmdev,
> +                                int id, int reg)
> +{
> +       if (reg >= SUN55I_A523_DELIMITER)
> +               return SUN55I_A523_SCALE_ABOVE * (SUN55I_A523_OFFSET_ABOVE - reg);
> +       else
> +               return SUN55I_A523_SCALE_BELOW * (SUN55I_A523_OFFSET_BELOW - reg);
> +}
> +
>  static int sun8i_ths_get_temp(struct thermal_zone_device *tz, int *temp)
>  {
>         struct tsensor *s = thermal_zone_device_priv(tz);
> @@ -299,6 +314,97 @@ static int sun50i_h6_ths_calibrate(struct ths_device *tmdev,
>         return 0;
>  }
>
> +/*
> + * The A523 nvmem calibration values. The ths1_3 is not used as it
> + * doesn't have its own sensor and doesn't have any internal switch.
> + * Instead, the value from the ths1_2 sensor is used, which gives the
> + * illusion of an independent sensor for NPU and GPU when using
> + * different calibration values.
> + *
> + * efuse layout 0x38-0x3F (caldata[0..3]):
> + *     caldata[0]      caldata[1]      caldata[2]      caldata[3]
> + * 0               16      24      32  36          48          60 64
> + * +---------------+---------------+---------------+---------------+
> + * |               |       |   temp    |  ths1_0   |  ths1_1   |   +
> + * +---------------+---------------+---------------+---------------+
> + *
> + * efuse layout 0x44-0x4B (caldata[4..7]):
> + *     caldata[4]      caldata[5]      caldata[6]      caldata[7]
> + * 0           12  16      24      32  36          48             64
> + * +---------------+---------------+---------------+---------------+
> + * |  ths1_2   |  ths1_3   |    ths0   |           |               +
> + * +---------------+---------------+---------------+---------------+
> + */
> +static int sun55i_a523_ths_calibrate(struct ths_device *tmdev,
> +                                    u16 *caldata, int callen)
> +{
> +       struct device *dev = tmdev->dev;
> +       int i, ft_temp;
> +
> +       if (!caldata[0])
> +               return -EINVAL;
> +
> +       ft_temp = (((caldata[2] << 8) | (caldata[1] >> 8)) & FT_TEMP_MASK) * 100;
> +
> +       for (i = 0; i < tmdev->chip->sensor_num; i++) {
> +               int sensor_reg, sensor_temp, cdata, offset;
> +               /*
> +                * Chips ths0 and ths1 have common parameters for value
> +                * calibration. To separate them we can use the number of
> +                * temperature sensors on each chip.
> +                * For ths0 this value is 1.
> +                */
> +               if (tmdev->chip->sensor_num == 1) {
> +                       sensor_reg = ((caldata[5] >> 8) | (caldata[6] << 8)) & TEMP_CALIB_MASK;
> +               } else {
> +                       switch (i) {
> +                       case 0:
> +                               sensor_reg = (caldata[2] >> 4) & TEMP_CALIB_MASK;
> +                               break;
> +                       case 1:
> +                               sensor_reg = caldata[3] & TEMP_CALIB_MASK;
> +                               break;
> +                       case 2:
> +                               sensor_reg = caldata[4] & TEMP_CALIB_MASK;
> +                               break;
> +                       default:
> +                               sensor_reg = 0;
> +                               break;
> +                       }
> +               }
> +
> +               sensor_temp = tmdev->chip->calc_temp(tmdev, i, sensor_reg);
> +
> +               /*
> +                * Calibration data is CALIBRATE_DEFAULT - (calculated
> +                * temperature from sensor reading at factory temperature
> +                * minus actual factory temperature) * X (scale from
> +                * temperature to register values)
> +                */
> +               cdata = CALIBRATE_DEFAULT -
> +                       ((sensor_temp - ft_temp) / SUN55I_A523_SCALE_ABOVE);

The BSP version works out to:

    cdata = CALIBRATE_DEFAULT -
            (sensor_temp - ft_temp - 5000) / SU55I_A523_SCALE_BELOW

Any particular reason why it's different?

> +               if (cdata & ~TEMP_CALIB_MASK) {
> +                       /*
> +                        * Calibration value more than 12-bit, but calibration
> +                        * register is 12-bit. In this case, ths hardware can
> +                        * still work without calibration, although the data
> +                        * won't be so accurate.
> +                        */
> +                       dev_warn(dev, "sensor%d is not calibrated.\n", i);
> +                       continue;
> +               }
> +
> +               offset = (i % 2) * 16;
> +               regmap_update_bits(tmdev->regmap,
> +                                  SUN50I_H6_THS_TEMP_CALIB + (i / 2 * 4),
> +                                  TEMP_CALIB_MASK << offset,
> +                                  cdata << offset);
> +       }
> +
> +       return 0;
> +}
> +
>  static int sun8i_ths_calibrate(struct ths_device *tmdev)
>  {
>         struct nvmem_cell *calcell = NULL;
> @@ -717,6 +823,31 @@ static const struct ths_thermal_chip sun50i_h616_ths = {
>         .calc_temp = sun8i_ths_calc_temp,
>  };
>
> +/* The A523 has a shared reset line for both chips */
> +static const struct ths_thermal_chip sun55i_a523_ths0 = {
> +       .sensor_num = 1,
> +       .has_bus_clk_reset = true,
> +       .has_mod_clk = true,
> +       .ft_deviation = 5000,

In the BSP this is 0.

> +       .temp_data_base = SUN50I_H6_THS_TEMP_DATA,
> +       .calibrate = sun55i_a523_ths_calibrate,
> +       .init = sun50i_h6_thermal_init,
> +       .irq_ack = sun50i_h6_irq_ack,
> +       .calc_temp = sun55i_a523_calc_temp,
> +};
> +
> +static const struct ths_thermal_chip sun55i_a523_ths1 = {
> +       .sensor_num = 3,
> +       .has_bus_clk_reset = true,
> +       .has_mod_clk = true,
> +       .ft_deviation = 5000,

In the BSP this is 0.


ChenYu

> +       .temp_data_base = SUN50I_H6_THS_TEMP_DATA,
> +       .calibrate = sun55i_a523_ths_calibrate,
> +       .init = sun50i_h6_thermal_init,
> +       .irq_ack = sun50i_h6_irq_ack,
> +       .calc_temp = sun55i_a523_calc_temp,
> +};
> +
>  static const struct of_device_id of_ths_match[] = {
>         { .compatible = "allwinner,sun8i-a83t-ths", .data = &sun8i_a83t_ths },
>         { .compatible = "allwinner,sun8i-h3-ths", .data = &sun8i_h3_ths },
> @@ -727,6 +858,8 @@ static const struct of_device_id of_ths_match[] = {
>         { .compatible = "allwinner,sun50i-h6-ths", .data = &sun50i_h6_ths },
>         { .compatible = "allwinner,sun20i-d1-ths", .data = &sun20i_d1_ths },
>         { .compatible = "allwinner,sun50i-h616-ths", .data = &sun50i_h616_ths },
> +       { .compatible = "allwinner,sun55i-a523-ths0", .data = &sun55i_a523_ths0 },
> +       { .compatible = "allwinner,sun55i-a523-ths1", .data = &sun55i_a523_ths1 },
>         { /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, of_ths_match);
> --
> 2.55.0
>
Juan Manuel López Carrillo Aug. 10, 2026, 8:01 p.m. UTC | #3 
Hi Mikhail, Chen-Yu,

Andre pointed me at this series. I have a T527 (A523 family) Orange Pi 4A
here, so I applied v5 on top of v7.2-rc7 and ran it on the board. It works:

  thermal_zone0  cpu4-thermal   43.0 C idle   49.1 C under load
  thermal_zone1  cpu0-thermal   43.2 C idle   49.2 C under load
  thermal_zone2  gpu-thermal    42.8 C idle   47.2 C under load
  thermal_zone3  ddr-thermal    43.8 C idle   49.3 C under load

(60 s of busy loops on all eight cores.) Boot is clean, no errors from the
driver, and the readings track load sensibly, so:

Tested-by: Juan Manuel López Carrillo <juanmanuellopezcarrillo@gmail.com>

While testing I looked into the open question about the extra sensor,
because I had reached a different number in my own out-of-tree work: I use
sensor_num = 4 for this controller, and this series uses 3.

> Are you sure? The calibration data is written to the sensor register.
> The BSP simply reads the sensor data from the GPU sensor, and passes it
> off as the value for the NPU sensor. No extra calculation involving the
> calibration data is done.

What the BSP does in software and what the hardware has look like two
different things here. The T527 User Manual v0.92 documents four sensors on
this controller: THS_EN (offset 0x04, page 452) has THS0_EN..THS3_EN in bits
3:0 with 31:4 reserved, and page 459 lists four data registers, THS0_DATA
0xC0, THS1_DATA 0xC4, THS2_DATA 0xC8 and THS3_DATA 0xCC, the last described
as "Temperature measurement data of Sensor3". THS_DATA_INTC, THS_SHUT_INTC
and THS_ALARM_INTC likewise have four per-sensor bits.

A documented register is not a wired-up sensor, so I measured it. With this
series running, THS_EN reads 0x7, so sensor 3 is off and THS3_DATA is 0x0,
as expected. Setting bit 3 and reading the four data registers once per
second (raw 12-bit values; lower means hotter on this part):

  idle          THS0 0x875   THS1 0x874   THS2 0x87C   THS3 0x85E
  under load    THS0 0x83C   THS1 0x83C   THS2 0x852   THS3 0x836
  delta          -57          -56          -42          -40

Sensor 3 starts reading as soon as it is enabled, sits at its own offset of
about 0x1C from sensor 2 instead of mirroring it, fluctuates independently
between consecutive reads, and follows the load. So there are four live
channels on this controller and the series reads three. I restored THS_EN to
0x7 afterwards.

What I cannot tell you is which channel belongs to which block, and I want
to be honest that my own tree does not settle it either. I followed a vendor
recipe that goes the other way round from what you describe: it treats the
GPU channel (id 2) as unreliable and reads channel 3 in its place. Your
reading is that the vendor takes the GPU channel and reports it as NPU. Both
cannot be right, and I have no way to tell which block each channel actually
sits next to. All I am claiming is the electrical part: channel 3 is alive
and independent.

One more thing you may want to check, and I am less confident about this
one. Applying your calibration code to my SID gives 0x90D, 0x90D and 0x90E
for sensors 0..2, and 0x92B for the ths0/DDR sensor. In the bits your switch
does not consume, between caldata[4] and the ths0 field, there is a further
12-bit value of 0x919, in the same numeric range as the other four. That
would fit the factory having calibrated four sensors on this controller, but
I have not verified the exact packing, so please take it as a hint and not
as a claim.

If you do end up reading channel 3, there is a trap worth knowing about.
Right after the block is enabled that channel can still be at 0 while its
first conversion is pending. With the two-slope formula a raw 0 works out to
about 183 C, which is above the 110 C critical trip, so the board powers off
2.8 s into boot while it is actually at ~50 C. I hit exactly that and had to
guard against a zero reading before using it.

I am happy to run whatever test would help on this board, including reading
channel 3 with proper calibration if you respin with sensor_num = 4.

Finally, a data point for the naming issue Chen-Yu raised on 5/5. On this
boot the kernel prints:

  thermal_sys: No trip points found for thermal-sensor@2009400 id=2

The gpu-thermal zone does define three trips, but inside a container named
"gpu-trips", so the thermal core silently ignores all of them. Renaming the
containers to "trips" is not only what the binding requires, it is what makes
those trips take effect at all.

Cheers,
Juan
Mikhail Kalashnikov Sept. 6, 2026, 8:57 a.m. UTC | #4 
On 7/21/26 23:23, Chen-Yu Tsai wrote:
> On Sun, Jul 5, 2026 at 1:15 AM Mikhail Kalashnikov <iuncuim@gmail.com> wrote:
>>
>> The A523 processor has two temperature controllers, THS0 and THS1.
>> THS0 has only one temperature sensor, which is located in the DRAM
>> controller. THS1 does have 3 sensors:
>> ths1_0 - "big" cores
>> ths1_1 - "little" cores
>> ths1_2 - gpu
>>
>> The datasheet mentions a fourth sensor in the NPU, but lacks any registers
>> for operation other than calibration registers. The vendor code reads the
>> value from ths1_2, but uses separate calibration data, so we get two
>> different values from real one.
> 
> Are you sure? The calibration data is written to the sensor register.
> 
> The BSP simply reads the sensor data from the GPU sensor, and passes it
> off as the value for the NPU sensor. No extra calculation involving the
> calibration data is done.
> 
Yes, I think you're right. And as Juan pointed out, my current
implementation doesn't match the BSP, since:

#define SUN55IW3_GPU_SENSOR_ID (2)
#define SUN55IW3_NPU_SENSOR_ID (3)
...
/* use NPU sensor data instead of GPU */
if (id == SUN55IW3_GPU_SENSOR_ID) {
     id = SUN55IW3_NPU_SENSOR_ID;
     ...
}

In other words, in the BSP driver, the NPU sensor is used for the GPU. 
However, based on the A523 user manual—which describes registers for
only three sensors, I had argued the opposite. The user manual for the
T527 includes a description of a fourth sensor.

Thus, it seems that all four sensors are working, but the purpose of
this workaround in the BSP remains unclear to me.

>>
>> Signed-off-by: Mikhail Kalashnikov <iuncuim@gmail.com>
>> Reviewed-by: Chen-Yu Tsai <wens@kernel.org>
>> ---
>>   drivers/thermal/sun8i_thermal.c | 133 ++++++++++++++++++++++++++++++++
>>   1 file changed, 133 insertions(+)
>>
>> diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
>> index 3bdd62aa8..f48ed9eae 100644
>> --- a/drivers/thermal/sun8i_thermal.c
>> +++ b/drivers/thermal/sun8i_thermal.c
>> @@ -59,6 +59,12 @@
>>   #define SUN50I_H6_THS_PC_TEMP_PERIOD(x)                ((GENMASK(19, 0) & (x)) << 12)
>>   #define SUN50I_H6_THS_DATA_IRQ_STS(x)          BIT(x)
>>
>> +#define SUN55I_A523_DELIMITER                  0x7c8
>> +#define SUN55I_A523_OFFSET_ABOVE               2736
>> +#define SUN55I_A523_OFFSET_BELOW               2825
>> +#define SUN55I_A523_SCALE_ABOVE                        74
>> +#define SUN55I_A523_SCALE_BELOW                        65
>> +
>>   struct tsensor {
>>          struct ths_device               *tmdev;
>>          struct thermal_zone_device      *tzd;
>> @@ -114,6 +120,15 @@ static int sun50i_h5_calc_temp(struct ths_device *tmdev,
>>                  return -1590 * reg / 10 + 276000;
>>   }
>>
>> +static int sun55i_a523_calc_temp(struct ths_device *tmdev,
>> +                                int id, int reg)
>> +{
>> +       if (reg >= SUN55I_A523_DELIMITER)
>> +               return SUN55I_A523_SCALE_ABOVE * (SUN55I_A523_OFFSET_ABOVE - reg);
>> +       else
>> +               return SUN55I_A523_SCALE_BELOW * (SUN55I_A523_OFFSET_BELOW - reg);
>> +}
>> +
>>   static int sun8i_ths_get_temp(struct thermal_zone_device *tz, int *temp)
>>   {
>>          struct tsensor *s = thermal_zone_device_priv(tz);
>> @@ -299,6 +314,97 @@ static int sun50i_h6_ths_calibrate(struct ths_device *tmdev,
>>          return 0;
>>   }
>>
>> +/*
>> + * The A523 nvmem calibration values. The ths1_3 is not used as it
>> + * doesn't have its own sensor and doesn't have any internal switch.
>> + * Instead, the value from the ths1_2 sensor is used, which gives the
>> + * illusion of an independent sensor for NPU and GPU when using
>> + * different calibration values.
>> + *
>> + * efuse layout 0x38-0x3F (caldata[0..3]):
>> + *     caldata[0]      caldata[1]      caldata[2]      caldata[3]
>> + * 0               16      24      32  36          48          60 64
>> + * +---------------+---------------+---------------+---------------+
>> + * |               |       |   temp    |  ths1_0   |  ths1_1   |   +
>> + * +---------------+---------------+---------------+---------------+
>> + *
>> + * efuse layout 0x44-0x4B (caldata[4..7]):
>> + *     caldata[4]      caldata[5]      caldata[6]      caldata[7]
>> + * 0           12  16      24      32  36          48             64
>> + * +---------------+---------------+---------------+---------------+
>> + * |  ths1_2   |  ths1_3   |    ths0   |           |               +
>> + * +---------------+---------------+---------------+---------------+
>> + */
>> +static int sun55i_a523_ths_calibrate(struct ths_device *tmdev,
>> +                                    u16 *caldata, int callen)
>> +{
>> +       struct device *dev = tmdev->dev;
>> +       int i, ft_temp;
>> +
>> +       if (!caldata[0])
>> +               return -EINVAL;
>> +
>> +       ft_temp = (((caldata[2] << 8) | (caldata[1] >> 8)) & FT_TEMP_MASK) * 100;
>> +
>> +       for (i = 0; i < tmdev->chip->sensor_num; i++) {
>> +               int sensor_reg, sensor_temp, cdata, offset;
>> +               /*
>> +                * Chips ths0 and ths1 have common parameters for value
>> +                * calibration. To separate them we can use the number of
>> +                * temperature sensors on each chip.
>> +                * For ths0 this value is 1.
>> +                */
>> +               if (tmdev->chip->sensor_num == 1) {
>> +                       sensor_reg = ((caldata[5] >> 8) | (caldata[6] << 8)) & TEMP_CALIB_MASK;
>> +               } else {
>> +                       switch (i) {
>> +                       case 0:
>> +                               sensor_reg = (caldata[2] >> 4) & TEMP_CALIB_MASK;
>> +                               break;
>> +                       case 1:
>> +                               sensor_reg = caldata[3] & TEMP_CALIB_MASK;
>> +                               break;
>> +                       case 2:
>> +                               sensor_reg = caldata[4] & TEMP_CALIB_MASK;
>> +                               break;
>> +                       default:
>> +                               sensor_reg = 0;
>> +                               break;
>> +                       }
>> +               }
>> +
>> +               sensor_temp = tmdev->chip->calc_temp(tmdev, i, sensor_reg);
>> +
>> +               /*
>> +                * Calibration data is CALIBRATE_DEFAULT - (calculated
>> +                * temperature from sensor reading at factory temperature
>> +                * minus actual factory temperature) * X (scale from
>> +                * temperature to register values)
>> +                */
>> +               cdata = CALIBRATE_DEFAULT -
>> +                       ((sensor_temp - ft_temp) / SUN55I_A523_SCALE_ABOVE);
> 
> The BSP version works out to:
> 
>      cdata = CALIBRATE_DEFAULT -
>              (sensor_temp - ft_temp - 5000) / SU55I_A523_SCALE_BELOW
> 
> Any particular reason why it's different?

In the early versions, I didn't use 'define' for constants, but later
added them based on the logic of the value read from the register: a
value below the threshold is 'BELOW', and a value above is 'ABOVE'. The
BSP driver uses a name for the temperature, but the temperature
decreases as the value read from the register increases; therefore, the
formula uses SCALE constants with different names but identical values.

In the next version, I’ll align them with the BSP values and add a 
comment explaining that the constants refer to the final temperature 
value, not the value read from the register.

Regarding SUN55IW3_CAL_COM 5000 — when I first looked at the driver
code, it seemed to me that this was the value intended for ft_deviation,
but now that I’ve double-checked, the version of the calibration formula
with SUN55IW3_CAL_COM and without ft_deviation gives the same result as
the version without SUN55IW3_CAL_COM but with ft_deviation=5000.

But if you'd rather I match BSP literally (5000 in the calibration
numerator, ft_deviation = 0), I can do that instead.
> 
>> +               if (cdata & ~TEMP_CALIB_MASK) {
>> +                       /*
>> +                        * Calibration value more than 12-bit, but calibration
>> +                        * register is 12-bit. In this case, ths hardware can
>> +                        * still work without calibration, although the data
>> +                        * won't be so accurate.
>> +                        */
>> +                       dev_warn(dev, "sensor%d is not calibrated.\n", i);
>> +                       continue;
>> +               }
>> +
>> +               offset = (i % 2) * 16;
>> +               regmap_update_bits(tmdev->regmap,
>> +                                  SUN50I_H6_THS_TEMP_CALIB + (i / 2 * 4),
>> +                                  TEMP_CALIB_MASK << offset,
>> +                                  cdata << offset);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>   static int sun8i_ths_calibrate(struct ths_device *tmdev)
>>   {
>>          struct nvmem_cell *calcell = NULL;
>> @@ -717,6 +823,31 @@ static const struct ths_thermal_chip sun50i_h616_ths = {
>>          .calc_temp = sun8i_ths_calc_temp,
>>   };
>>
>> +/* The A523 has a shared reset line for both chips */
>> +static const struct ths_thermal_chip sun55i_a523_ths0 = {
>> +       .sensor_num = 1,
>> +       .has_bus_clk_reset = true,
>> +       .has_mod_clk = true,
>> +       .ft_deviation = 5000,
> 
> In the BSP this is 0.

> 
>> +       .temp_data_base = SUN50I_H6_THS_TEMP_DATA,
>> +       .calibrate = sun55i_a523_ths_calibrate,
>> +       .init = sun50i_h6_thermal_init,
>> +       .irq_ack = sun50i_h6_irq_ack,
>> +       .calc_temp = sun55i_a523_calc_temp,
>> +};
>> +
>> +static const struct ths_thermal_chip sun55i_a523_ths1 = {
>> +       .sensor_num = 3,
>> +       .has_bus_clk_reset = true,
>> +       .has_mod_clk = true,
>> +       .ft_deviation = 5000,
> 
> In the BSP this is 0.
> 
> 
> ChenYu
> 
>> +       .temp_data_base = SUN50I_H6_THS_TEMP_DATA,
>> +       .calibrate = sun55i_a523_ths_calibrate,
>> +       .init = sun50i_h6_thermal_init,
>> +       .irq_ack = sun50i_h6_irq_ack,
>> +       .calc_temp = sun55i_a523_calc_temp,
>> +};
>> +
>>   static const struct of_device_id of_ths_match[] = {
>>          { .compatible = "allwinner,sun8i-a83t-ths", .data = &sun8i_a83t_ths },
>>          { .compatible = "allwinner,sun8i-h3-ths", .data = &sun8i_h3_ths },
>> @@ -727,6 +858,8 @@ static const struct of_device_id of_ths_match[] = {
>>          { .compatible = "allwinner,sun50i-h6-ths", .data = &sun50i_h6_ths },
>>          { .compatible = "allwinner,sun20i-d1-ths", .data = &sun20i_d1_ths },
>>          { .compatible = "allwinner,sun50i-h616-ths", .data = &sun50i_h616_ths },
>> +       { .compatible = "allwinner,sun55i-a523-ths0", .data = &sun55i_a523_ths0 },
>> +       { .compatible = "allwinner,sun55i-a523-ths1", .data = &sun55i_a523_ths1 },
>>          { /* sentinel */ },
>>   };
>>   MODULE_DEVICE_TABLE(of, of_ths_match);
>> --
>> 2.55.0
>>
diff mbox series

Patch

diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
index 3bdd62aa8..f48ed9eae 100644
--- a/drivers/thermal/sun8i_thermal.c
+++ b/drivers/thermal/sun8i_thermal.c
@@ -59,6 +59,12 @@ 
 #define SUN50I_H6_THS_PC_TEMP_PERIOD(x)		((GENMASK(19, 0) & (x)) << 12)
 #define SUN50I_H6_THS_DATA_IRQ_STS(x)		BIT(x)
 
+#define SUN55I_A523_DELIMITER			0x7c8
+#define SUN55I_A523_OFFSET_ABOVE		2736
+#define SUN55I_A523_OFFSET_BELOW		2825
+#define SUN55I_A523_SCALE_ABOVE			74
+#define SUN55I_A523_SCALE_BELOW			65
+
 struct tsensor {
 	struct ths_device		*tmdev;
 	struct thermal_zone_device	*tzd;
@@ -114,6 +120,15 @@  static int sun50i_h5_calc_temp(struct ths_device *tmdev,
 		return -1590 * reg / 10 + 276000;
 }
 
+static int sun55i_a523_calc_temp(struct ths_device *tmdev,
+				 int id, int reg)
+{
+	if (reg >= SUN55I_A523_DELIMITER)
+		return SUN55I_A523_SCALE_ABOVE * (SUN55I_A523_OFFSET_ABOVE - reg);
+	else
+		return SUN55I_A523_SCALE_BELOW * (SUN55I_A523_OFFSET_BELOW - reg);
+}
+
 static int sun8i_ths_get_temp(struct thermal_zone_device *tz, int *temp)
 {
 	struct tsensor *s = thermal_zone_device_priv(tz);
@@ -299,6 +314,97 @@  static int sun50i_h6_ths_calibrate(struct ths_device *tmdev,
 	return 0;
 }
 
+/*
+ * The A523 nvmem calibration values. The ths1_3 is not used as it
+ * doesn't have its own sensor and doesn't have any internal switch.
+ * Instead, the value from the ths1_2 sensor is used, which gives the
+ * illusion of an independent sensor for NPU and GPU when using
+ * different calibration values.
+ *
+ * efuse layout 0x38-0x3F (caldata[0..3]):
+ *     caldata[0]      caldata[1]      caldata[2]      caldata[3]
+ * 0               16      24      32  36          48          60 64
+ * +---------------+---------------+---------------+---------------+
+ * |               |       |   temp    |  ths1_0   |  ths1_1   |   +
+ * +---------------+---------------+---------------+---------------+
+ *
+ * efuse layout 0x44-0x4B (caldata[4..7]):
+ *     caldata[4]      caldata[5]      caldata[6]      caldata[7]
+ * 0           12  16      24      32  36          48             64
+ * +---------------+---------------+---------------+---------------+
+ * |  ths1_2   |  ths1_3   |    ths0   |           |               +
+ * +---------------+---------------+---------------+---------------+
+ */
+static int sun55i_a523_ths_calibrate(struct ths_device *tmdev,
+				     u16 *caldata, int callen)
+{
+	struct device *dev = tmdev->dev;
+	int i, ft_temp;
+
+	if (!caldata[0])
+		return -EINVAL;
+
+	ft_temp = (((caldata[2] << 8) | (caldata[1] >> 8)) & FT_TEMP_MASK) * 100;
+
+	for (i = 0; i < tmdev->chip->sensor_num; i++) {
+		int sensor_reg, sensor_temp, cdata, offset;
+		/*
+		 * Chips ths0 and ths1 have common parameters for value
+		 * calibration. To separate them we can use the number of
+		 * temperature sensors on each chip.
+		 * For ths0 this value is 1.
+		 */
+		if (tmdev->chip->sensor_num == 1) {
+			sensor_reg = ((caldata[5] >> 8) | (caldata[6] << 8)) & TEMP_CALIB_MASK;
+		} else {
+			switch (i) {
+			case 0:
+				sensor_reg = (caldata[2] >> 4) & TEMP_CALIB_MASK;
+				break;
+			case 1:
+				sensor_reg = caldata[3] & TEMP_CALIB_MASK;
+				break;
+			case 2:
+				sensor_reg = caldata[4] & TEMP_CALIB_MASK;
+				break;
+			default:
+				sensor_reg = 0;
+				break;
+			}
+		}
+
+		sensor_temp = tmdev->chip->calc_temp(tmdev, i, sensor_reg);
+
+		/*
+		 * Calibration data is CALIBRATE_DEFAULT - (calculated
+		 * temperature from sensor reading at factory temperature
+		 * minus actual factory temperature) * X (scale from
+		 * temperature to register values)
+		 */
+		cdata = CALIBRATE_DEFAULT -
+			((sensor_temp - ft_temp) / SUN55I_A523_SCALE_ABOVE);
+
+		if (cdata & ~TEMP_CALIB_MASK) {
+			/*
+			 * Calibration value more than 12-bit, but calibration
+			 * register is 12-bit. In this case, ths hardware can
+			 * still work without calibration, although the data
+			 * won't be so accurate.
+			 */
+			dev_warn(dev, "sensor%d is not calibrated.\n", i);
+			continue;
+		}
+
+		offset = (i % 2) * 16;
+		regmap_update_bits(tmdev->regmap,
+				   SUN50I_H6_THS_TEMP_CALIB + (i / 2 * 4),
+				   TEMP_CALIB_MASK << offset,
+				   cdata << offset);
+	}
+
+	return 0;
+}
+
 static int sun8i_ths_calibrate(struct ths_device *tmdev)
 {
 	struct nvmem_cell *calcell = NULL;
@@ -717,6 +823,31 @@  static const struct ths_thermal_chip sun50i_h616_ths = {
 	.calc_temp = sun8i_ths_calc_temp,
 };
 
+/* The A523 has a shared reset line for both chips */
+static const struct ths_thermal_chip sun55i_a523_ths0 = {
+	.sensor_num = 1,
+	.has_bus_clk_reset = true,
+	.has_mod_clk = true,
+	.ft_deviation = 5000,
+	.temp_data_base = SUN50I_H6_THS_TEMP_DATA,
+	.calibrate = sun55i_a523_ths_calibrate,
+	.init = sun50i_h6_thermal_init,
+	.irq_ack = sun50i_h6_irq_ack,
+	.calc_temp = sun55i_a523_calc_temp,
+};
+
+static const struct ths_thermal_chip sun55i_a523_ths1 = {
+	.sensor_num = 3,
+	.has_bus_clk_reset = true,
+	.has_mod_clk = true,
+	.ft_deviation = 5000,
+	.temp_data_base = SUN50I_H6_THS_TEMP_DATA,
+	.calibrate = sun55i_a523_ths_calibrate,
+	.init = sun50i_h6_thermal_init,
+	.irq_ack = sun50i_h6_irq_ack,
+	.calc_temp = sun55i_a523_calc_temp,
+};
+
 static const struct of_device_id of_ths_match[] = {
 	{ .compatible = "allwinner,sun8i-a83t-ths", .data = &sun8i_a83t_ths },
 	{ .compatible = "allwinner,sun8i-h3-ths", .data = &sun8i_h3_ths },
@@ -727,6 +858,8 @@  static const struct of_device_id of_ths_match[] = {
 	{ .compatible = "allwinner,sun50i-h6-ths", .data = &sun50i_h6_ths },
 	{ .compatible = "allwinner,sun20i-d1-ths", .data = &sun20i_d1_ths },
 	{ .compatible = "allwinner,sun50i-h616-ths", .data = &sun50i_h616_ths },
+	{ .compatible = "allwinner,sun55i-a523-ths0", .data = &sun55i_a523_ths0 },
+	{ .compatible = "allwinner,sun55i-a523-ths1", .data = &sun55i_a523_ths1 },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, of_ths_match);