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

["Juan Manuel López Carrillo" <juanmanuellopezcarrillo@gmail.com>]

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