[PATCH v1 0/7] iio: adc: ina2xx: Rework CNVR alignment, fix busy loops
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
On Fri, 8 Dec 2017 18:41:45 +0100 Stefan Brüns 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
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 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) __0123456789 ...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: __0123456789 ...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 signature.asc Description: This is a digitally signed message part.
Re: [PATCH v1 0/7] iio: adc: ina2xx: Rework CNVR alignment, fix busy loops
On Sun, 10 Dec 2017 21:22:13 +0100 Stefan Brüns 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 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) > > __0123456789 > ...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: > __0123456789 > ...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 >