[PATCH v1 0/7] iio: adc: ina2xx: Rework CNVR alignment, fix busy loops

[PATCH v1 0/7] iio: adc: ina2xx: Rework CNVR alignment, fix busy loops

[Stefan Brüns]

Currently, the INA2xx driver may end up causing 100% load on a single core
and fully loading the I2C bus, which is caused by two different issues:

The code uses a udelay to bridge the gab between two subsequent samples.
As the sampling interval may be up to 16 seconds, the CPU is busy
waiting most of the time.

The second issue manifests when using the (default) "synchronous" mode.
The code polls for a set conversion ready flag, but fails to align the
sampling interval to the raising flag. The polling interval is
(rightfully) slighly shorter than the sampling interval, so after some
samples the sampling thread is continously polling.

The patch series fixes both issues:
Patch 1 and 2 are just some small cosmetic changes.

Patch 3 removes an unnecessary read. According to the datasheet, the
CNVR flag is only cleared by reading the power register, but is cleared
by reading any of the measurement registers, thus the dummy read can
be skipped. This behaviour has been confirmed by TI technical support.

Patch 4 replaces the udelay with usleep_range.

Patch 5 reworks the delay logic. Previously the IIO timestamp clock was
used to capture entry and exit times of the work function. The
timestamp clock is user selectable and may be non-monotonic. Also,
any time spent outside the work function is not accounted for.

Patch 6 moves the timestap capture to the end of the conversion ready
status poll.

Patch 7 addresses the alignment issue. Every time an unset flag is seen
on poll loop entry, the reference timestamp is readjusted.

Both old and fixed behaviour has been verified using a logic analyzer.
In synchrounous mode, every few samples a double read of the status
register can be observed, showing the raising status flag, the other
samples are evenly spaced at sampling intervals inbetween.


Stefan Brüns (7):
  iio: adc: ina2xx: Remove bogus cast for data argument
  iio: adc: ina2xx: Clarify size requirement for data buffer
  iio: adc: ina2xx: Remove unneeded dummy read to clear CNVR flag
  iio: adc: ina2xx: Do not udelay for several seconds
  iio: adc: ina2xx: Use a monotonic clock for delay calculation
  iio: adc: ina2xx: Align timestamp with conversion ready flag
  iio: adc: ina2xx: Actually align the loop with the conversion ready
    flag

 drivers/iio/adc/ina2xx-adc.c | 104 ++++++++++++++++++++++++-------------------
 1 file changed, 59 insertions(+), 45 deletions(-)

-- 
2.15.1

Re: [PATCH v1 0/7] iio: adc: ina2xx: Rework CNVR alignment, fix busy loops

[Jonathan Cameron]

On Fri, 8 Dec 2017 18:41:45 +0100
Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:

> Currently, the INA2xx driver may end up causing 100% load on a single core
> and fully loading the I2C bus, which is caused by two different issues:
> 
> The code uses a udelay to bridge the gab between two subsequent samples.
> As the sampling interval may be up to 16 seconds, the CPU is busy
> waiting most of the time.
> 
> The second issue manifests when using the (default) "synchronous" mode.
> The code polls for a set conversion ready flag, but fails to align the
> sampling interval to the raising flag. The polling interval is
> (rightfully) slighly shorter than the sampling interval, so after some
> samples the sampling thread is continously polling.

I'm confused.  Would you mind doing an asci art example perhaps?

Thanks,

Jonathan
> 
> The patch series fixes both issues:
> Patch 1 and 2 are just some small cosmetic changes.
> 
> Patch 3 removes an unnecessary read. According to the datasheet, the
> CNVR flag is only cleared by reading the power register, but is cleared
> by reading any of the measurement registers, thus the dummy read can
> be skipped. This behaviour has been confirmed by TI technical support.
> 
> Patch 4 replaces the udelay with usleep_range.
> 
> Patch 5 reworks the delay logic. Previously the IIO timestamp clock was
> used to capture entry and exit times of the work function. The
> timestamp clock is user selectable and may be non-monotonic. Also,
> any time spent outside the work function is not accounted for.
> 
> Patch 6 moves the timestap capture to the end of the conversion ready
> status poll.
> 
> Patch 7 addresses the alignment issue. Every time an unset flag is seen
> on poll loop entry, the reference timestamp is readjusted.
> 
> Both old and fixed behaviour has been verified using a logic analyzer.
> In synchrounous mode, every few samples a double read of the status
> register can be observed, showing the raising status flag, the other
> samples are evenly spaced at sampling intervals inbetween.
> 
> 
> Stefan Brüns (7):
>   iio: adc: ina2xx: Remove bogus cast for data argument
>   iio: adc: ina2xx: Clarify size requirement for data buffer
>   iio: adc: ina2xx: Remove unneeded dummy read to clear CNVR flag
>   iio: adc: ina2xx: Do not udelay for several seconds
>   iio: adc: ina2xx: Use a monotonic clock for delay calculation
>   iio: adc: ina2xx: Align timestamp with conversion ready flag
>   iio: adc: ina2xx: Actually align the loop with the conversion ready
>     flag
> 
>  drivers/iio/adc/ina2xx-adc.c | 104 ++++++++++++++++++++++++-------------------
>  1 file changed, 59 insertions(+), 45 deletions(-)
> 

Re: [PATCH v1 0/7] iio: adc: ina2xx: Rework CNVR alignment, fix busy loops

[Stefan Brüns]

[-- Attachment #1: Type: text/plain, Size: 2970 bytes --]

On Sunday, December 10, 2017 6:36:54 PM CET Jonathan Cameron wrote:
> On Fri, 8 Dec 2017 18:41:45 +0100
> 
> Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:
> > Currently, the INA2xx driver may end up causing 100% load on a single core
> > and fully loading the I2C bus, which is caused by two different issues:
> > 
> > The code uses a udelay to bridge the gab between two subsequent samples.
> > As the sampling interval may be up to 16 seconds, the CPU is busy
> > waiting most of the time.
> > 
> > The second issue manifests when using the (default) "synchronous" mode.
> > The code polls for a set conversion ready flag, but fails to align the
> > sampling interval to the raising flag. The polling interval is
> > (rightfully) slighly shorter than the sampling interval, so after some
> > samples the sampling thread is continously polling.
> 
> I'm confused.  Would you mind doing an asci art example perhaps?

Lets assume the conversion interval is set to 2 ms. If the polling is done at 
the sampling frequency, it might be slightly to long due to differences 
between the host and device clocks, so the polling has to run somewhat faster. 
Somewhat faster means 200 us, this is kept unchanged.

In case the CNVR flag is not set, the status register is read again until the 
flag raises. The time instant the flag raises is the reference for later 
reads.

The following shows the timing for the fixed code. Each character corresponds 
to 200us, first row: real time (ms), second row: conversion finished by 
device, third row: register read
s: status, CNVR not set
S: status, CNVR set
S: value register, e.g. shunt voltage
_: bus busy (each reads needs 400 us @ 100 kBit/s)

__0____1____2____3____4____5____6____7____8____9
...C.........C.........C.........C.........C....
..s_S_V_.....S_V_......S_V_....s_S_V_......S_V_.

At 0 ms, the conversion has not yet finished, but a 0.4 it has. Further reads 
are done at 0.4 + n * (2 - 0.2) ms. The next read happens at 2.2 ms, the third 
should at 4.0, but happens slightly late at 4.2. The read at 5.8 gets an unset 
CNVR flag, so the sampling is readjusted to happen at 6.2 + n' (2 - 0.2) ms.

The old code does the following:
__0____1____2____3____4____5____6____7____8____9
...C.........C.........C.........C.........C....
..s_S_V_...s_S_V_...s_s_S_V_.s_s_S_V_.s_s_s_S_V_

The first read happens at 0 ms, it measures the time for the reading (1.2 ms), 
sleeps for the remainder (0.6 ms) and reads again. The third read takes 1.6, 
so sleep for 0.2 ms.

The old code does not differentiate between time spent in the status poll and 
time spent for reading. Time spent in the status poll should not be subtracted 
from the delay until the next read (well, halfway, only the time spent with 
the poll returning !CNVR).

Kind regards,

Stefan

-- 
Stefan Brüns  /  Bergstraße 21  /  52062 Aachen
home: +49 241 53809034     mobile: +49 151 50412019

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v1 0/7] iio: adc: ina2xx: Rework CNVR alignment, fix busy loops

[Jonathan Cameron]

On Sun, 10 Dec 2017 21:22:13 +0100
Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:

> On Sunday, December 10, 2017 6:36:54 PM CET Jonathan Cameron wrote:
> > On Fri, 8 Dec 2017 18:41:45 +0100
> > 
> > Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:  
> > > Currently, the INA2xx driver may end up causing 100% load on a single core
> > > and fully loading the I2C bus, which is caused by two different issues:
> > > 
> > > The code uses a udelay to bridge the gab between two subsequent samples.
> > > As the sampling interval may be up to 16 seconds, the CPU is busy
> > > waiting most of the time.
> > > 
> > > The second issue manifests when using the (default) "synchronous" mode.
> > > The code polls for a set conversion ready flag, but fails to align the
> > > sampling interval to the raising flag. The polling interval is
> > > (rightfully) slighly shorter than the sampling interval, so after some
> > > samples the sampling thread is continously polling.  
> > 
> > I'm confused.  Would you mind doing an asci art example perhaps?  
> 
> Lets assume the conversion interval is set to 2 ms. If the polling is done at 
> the sampling frequency, it might be slightly to long due to differences 
> between the host and device clocks, so the polling has to run somewhat faster. 
> Somewhat faster means 200 us, this is kept unchanged.
> 
> In case the CNVR flag is not set, the status register is read again until the 
> flag raises. The time instant the flag raises is the reference for later 
> reads.
> 
> The following shows the timing for the fixed code. Each character corresponds 
> to 200us, first row: real time (ms), second row: conversion finished by 
> device, third row: register read
> s: status, CNVR not set
> S: status, CNVR set
> S: value register, e.g. shunt voltage
> _: bus busy (each reads needs 400 us @ 100 kBit/s)
> 
> __0____1____2____3____4____5____6____7____8____9
> ...C.........C.........C.........C.........C....
> ..s_S_V_.....S_V_......S_V_....s_S_V_......S_V_.
> 
> At 0 ms, the conversion has not yet finished, but a 0.4 it has. Further reads 
> are done at 0.4 + n * (2 - 0.2) ms. The next read happens at 2.2 ms, the third 
> should at 4.0, but happens slightly late at 4.2. The read at 5.8 gets an unset 
> CNVR flag, so the sampling is readjusted to happen at 6.2 + n' (2 - 0.2) ms.
> 
> The old code does the following:
> __0____1____2____3____4____5____6____7____8____9
> ...C.........C.........C.........C.........C....
> ..s_S_V_...s_S_V_...s_s_S_V_.s_s_S_V_.s_s_s_S_V_
> 
> The first read happens at 0 ms, it measures the time for the reading (1.2 ms), 
> sleeps for the remainder (0.6 ms) and reads again. The third read takes 1.6, 
> so sleep for 0.2 ms.
> 
> The old code does not differentiate between time spent in the status poll and 
> time spent for reading. Time spent in the status poll should not be subtracted 
> from the delay until the next read (well, halfway, only the time spent with 
> the poll returning !CNVR).

Thanks - got it now.  Makes complete sense.  There are probably other
drivers where we can do something similar but certainly good to
improve this one.

Jonathan
> 
> Kind regards,
> 
> Stefan
>