On Tue, 14 Jun 2022 10:13:17 +0000
<Claudiu.Beznea@xxxxxxxxxxxxx> wrote:
> On 11.06.2022 21:15, Jonathan Cameron wrote:
> > 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 <claudiu.beznea@xxxxxxxxxxxxx> wrote:
> >
> >> 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 <claudiu.beznea@xxxxxxxxxxxxx>
> >
> > 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.
>
> > 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.
>
...
> >> +#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).
>
> >> +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
