On Tue, 14 Jun 2022 10:13:17 +0000
[off-list ref] wrote:

On 11.06.2022 21:15, Jonathan Cameron wrote:

quoted

EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe

On Thu, 9 Jun 2022 11:32:10 +0300
Claudiu Beznea [off-list ref] wrote:
  

quoted

SAMA7G5 has a temperature sensor embedded that is connected to channel 31
of ADC. Temperature sensor provides 2 outputs: VTEMP and VBG. VTEMP is
proportional to the absolute temperature voltage, VBG is quasi-temperature
independent voltage. The calibration data for temperature sensor are
retrieved from OTP memory specific to SAMA7G5. The formula to calculate
the junction temperature is as follows:

P1 + (Vref * (Vtemp - P6 - P4 * Vbg)) / (Vbg * VTEMP_DT)

where Pi are calibration data retrieved from OTP memory.

For better resolution before reading the temperature certain settings
for oversampling ratio, sample frequency, EMR.TRACKX, MR.TRACKTIM are
applied. The initial settings are reapplied at the end of temperature
reading. Current support is not integrated with trigger buffers.

Signed-off-by: Claudiu Beznea <redacted>  

This is a complex driver, so I got a bit lost figuring out what happens
about buffered capture of this channel.  What ends up in the buffer?  

With this implementation nothing should end up in the buffer as the patch
skips channel enable/disable if its about an IIO_TEMP channel (see
functions functions at91_adc_buffer_prepare(),
at91_adc_buffer_postdisable()). More details above.

According to datasheet the temperature channel behaves the same as the
other channels. On temperature channel are multiplexed both VBG and VTEMP.

          `
          | \      +-----+
VBG   --->|  |ch31 |     |
Vtemp --->|  |---->| ADC |
          |  /     +-----+
          | /
          .

And both are needed to be measured in order to determine the correct in SoC
temperature.

At a moment of time only one of these could be measured, the selection
being done with bit SRCLCH bit of ACR register.

According to datasheet there is no special treatment for channel 31 of ADC
in case triggers are enabled. So, in case of a buffer capture the buffer
for channel 31 will contain either the converted value for VBG or VTEMP
(depending on the value of bit SRCLCH bit of ACR register), if channel 31
is enabled for that. But on this implementation we skip the enable/disable
of IIO_TEMP channels (functions at91_adc_buffer_prepare(),
at91_adc_buffer_postdisable()).

Great explanation. Perhaps we can capture some of it either as comments,
or patch description etc.

Hardware implements a special mechanism for reading the temperature when
triggers are enabled as follows: the hardware provides a RTC trigger wich
fires every 1 second and starts a conversion on channel 31. This could
permit to have a temperature value once every 2 seconds (in the 1st RTC
trigger VBG could be read, in the 2nd RTC trigger Vtemp could be read, or
the other way arround). For this, configuration needs to be done propertly
in the RTC memory spaces and on Linux side something should be implemented
for the interaction b/w RTC and IIO subsystems. But this is for future
development.

quoted

Most processed channels are not useable with that mode (and hence have
a scanindex == -1 which ensures they aren't exposed as an option for
userspace to enable).  

OK, I haven't been aware of that. I only did some basic research on how
this could be achieved. As we are using the thermal support on SAMA7G5 with
driver at drivers/thermal/thermal-generic-adc.c which reads processed value
at periodic intervals one idea was to take advantage of the RTC trigger
mechanism for channel 31 and have the converted values of VBG and VTEMP
kept only inside the at91-sama5d2_adc.c thus when receiving requests from
themal-generic-adc.c and buffers are enabled to use those cached values in
computation formula.

Sure, that might work ok. It's a bit of a hack, but would let you keep the
more interesting stuff hidden way in one place.

...

quoted

quoted

+#define AT91_SAMA5D2_CHAN_TEMP(num, name, addr)                              \
+     {                                                               \
+             .type = IIO_TEMP,                                       \
+             .channel = num,                                         \
+             .address =  addr,                                       \
+             .scan_index = num,                                      \
+             .scan_type = {                                          \
+                     .sign = 'u',                                    \
+                     .realbits = 16,                                 \
+                     .storagebits = 16,                              \  

So this is unusual.  Normally a processed channel isn't suitable for buffered
capture because they tend not to fit in nice compact storage.  In this case
what actually goes in the buffer?  Perhaps a comment would be useful here.  

At the moment we don't allow the enabling of channel 31 when enabling the
buffers (we skip IIO_TEMP channels in at91_adc_buffer_prepare(),
at91_adc_buffer_postdisable()). At the moment when buffers are enabled the
IIO_TEMP consumer (drivers/thermal/thermal-generic-adc.c) will fail on
readings due to iio_buffer_enabled() in at91_adc_read_temp() or
iio_device_claim_direct_mode() in at91_adc_read_info_raw().

I suspected as much. If so, a bunch of this makes not sense. Channels that
can't be in that buffer have magic scan_index = -1 and .scan_type is
probably not used (occasionally it is helpful for non buffered paths, though
rarely all the info in there).

quoted

quoted

+static int at91_adc_read_temp(struct iio_dev *indio_dev,
+                           struct iio_chan_spec const *chan, int *val)
+{
+     struct at91_adc_state *st = iio_priv(indio_dev);
+     struct at91_adc_temp_sensor_clb *clb = &st->soc_info.temp_sensor_clb;
+     u64 div1, div2;
+     u32 tmp;
+     int ret, vbg, vtemp;
+
+     if (!st->soc_info.platform->temp_sensor || !st->temp_st.init)
+             return -EPERM;  

You shouldn't register the channel if it's not readable.  Hence this
should never happen.  

I kept this as an indicator for temperature consumer that something wrong
happend on probing path of temperature sensor. In function
at91_adc_temp_sensor_init() the following could fail:
- devm_nvmem_cell_get()
- nvmem_cell_read()
- invalid length for NVMEM cell
Thus to keep the ADC probe going on in case temperature sensor probing init
failed I've added this st->temp_st.init. On the field there might be
devices that don't have proper information in NVMEM memory for temperature
sensor.

If those fail, don't register the channel.  It should just be invisible
to userspace / in kernel consumers.  That may require reorganizing how
you register channels to know if these worked or not before the
point of registering channels.


Thanks,

Jonathan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel