On 29/06/16 14:14, Linus Walleij wrote:
> Leonard Crestez observed the following phenomenon: when using
> hard interrupt triggers (the DRDY line coming out of an ST
> sensor) sometimes a new value would arrive while reading the
> previous value, due to latencies in the system.
>
> We discovered that the ST hardware as far as can be observed
> is designed for level interrupts: the DRDY line will be held
> asserted as long as there are new values coming. The interrupt
> handler should be re-entered until we're out of values to
> handle from the sensor.
>
> If interrupts were handled as occurring on the edges (usually
> low-to-high) new values could appear and the line be held
> asserted after that, and these values would be missed, the
> interrupt handler would also lock up as new data was
> available, but as no new edges occurs on the DRDY signal,
> nothing happens: the edge detector only detects edges.
>
> To counter this, do the following:
>
> - Accept interrupt lines to be flagged as level interrupts
> using IRQF_TRIGGER_HIGH and IRQF_TRIGGER_LOW. If the line
> is marked like this (in the device tree node or ACPI
> table or similar) it will be utilized as a level IRQ.
> We mark the line with IRQF_ONESHOT and mask the IRQ
> while processing a sample, then the top half will be
> entered again if new values are available.
>
> - If we are flagged as using edge interrupts with
> IRQF_TRIGGER_RISING or IRQF_TRIGGER_FALLING: remove
> IRQF_ONESHOT so that the interrupt line is not
> masked while running the thread part of the interrupt.
> This way we will never miss an interrupt, then introduce
> a loop that polls the data ready registers repeatedly
> until no new samples are available, then exit the
> interrupt handler. This way we know no new values are
> available when the interrupt handler exits and
> new (edge) interrupts will be triggered when data arrives.
> Take some extra care to update the timestamp in the poll
> loop if this happens. The timestamp will not be 100%
> perfect, but it will at least be closer to the actual
> events. Usually the extra poll loop will handle the new
> samples, but once in a blue moon, we get a new IRQ
> while exiting the loop, before returning from the
> thread IRQ bottom half with IRQ_HANDLED. On these rare
> occasions, the removal of IRQF_ONESHOT means the
> interrupt will immediately fire again.
>
> - If no interrupt type is indicated from the DT/ACPI,
> choose IRQF_TRIGGER_RISING as default, as this is necessary
> for legacy boards.
>
> Tested successfully on the LIS331DL and L3G4200D by setting
> sampling frequency to 400Hz/800Hz and stressing the system:
> extra reads in the threaded interrupt handler occurs.
>
> Cc: Giuseppe Barba <giuseppe.barba@xxxxxx>
> Cc: Denis Ciocca <denis.ciocca@xxxxxx>
> Tested-by: Crestez Dan Leonard <cdleonard@xxxxxxxxx>
> Reported-by: Crestez Dan Leonard <cdleonard@xxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
Applied to the togreg branch of iio.git.
Thanks for your persistence on this Linus!
Jonathan
> ---
> ChangeLog v8->v9:
> - Make rising edges the default interrupt request as we have
> PXA27x users in the tree with inspecified IRQF, leading them
> to request active high IRQs which the driver does not support.
> So use the old rising edge default.
> ChangeLog v7->v8:
> - Remove the 10 times iteration loop: instead allow the thread
> to turn into a polling loop when there is always new data
> available.
> - To stop the thread from going into an eternal loop: make it
> respect sdata->hw_irq_enabled: if the interrupt is disabled,
> we bail out of the loop.
> ChangeLog v6->v7:
> - Incorporate Leonards handling of level interrupts into
> the code. If we have level IRQs: use them.
> - Default to level interrupt handling.
> - If we only have edge interrupts: enable the polling loop.
> - Leonard it would be awesome if you could test this patch,
> maybe both with level and edge irqs on your system? I
> could only test the edges.
> ChangeLog v5->v6:
> - Add a loop counter to the threaded value poll function: let's
> just loop here for at maximum 10 loops before we exit and
> let the thread re-trigger if more interrupts arrived.
> ChangeLog v4->v5:
> - Remove the IRQF_ONESHOT flag from the interrupt: this makes
> it possible to trigger the top half of the interrupt even
> though the bottom half is processing an earlier interrupt.
> This makes it possible to gracefully handle interrupts that
> come in during sensor reading.
> - Add better descriptive comments.
> ChangeLog v3->v4:
> - Include this patch with the threaded interrupt fix patch.
> Adapte the same patch numbering system.
>
> If this works I suggest it be applied to mainline as a fix on
> top of the threading patch version 3, so we handle this annoying
> lockup bug.
> ---
> drivers/iio/common/st_sensors/st_sensors_buffer.c | 7 +-
> drivers/iio/common/st_sensors/st_sensors_trigger.c | 154 +++++++++++++++------
> include/linux/iio/common/st_sensors.h | 2 +
> 3 files changed, 117 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/iio/common/st_sensors/st_sensors_buffer.c b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> index f1693dbebb8a..f6873919f188 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_buffer.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_buffer.c
> @@ -59,7 +59,12 @@ irqreturn_t st_sensors_trigger_handler(int irq, void *p)
> struct st_sensor_data *sdata = iio_priv(indio_dev);
> s64 timestamp;
>
> - /* If we do timetamping here, do it before reading the values */
> + /*
> + * If we do timetamping here, do it before reading the values, because
> + * once we've read the values, new interrupts can occur (when using
> + * the hardware trigger) and the hw_timestamp may get updated.
> + * By storing it in a local variable first, we are safe.
> + */
> if (sdata->hw_irq_trigger)
> timestamp = sdata->hw_timestamp;
> else
> diff --git a/drivers/iio/common/st_sensors/st_sensors_trigger.c b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> index 296e4ff19ae8..5edc4b5885f5 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_trigger.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_trigger.c
> @@ -18,6 +18,50 @@
> #include "st_sensors_core.h"
>
> /**
> + * st_sensors_new_samples_available() - check if more samples came in
> + * returns:
> + * 0 - no new samples available
> + * 1 - new samples available
> + * negative - error or unknown
> + */
> +static int st_sensors_new_samples_available(struct iio_dev *indio_dev,
> + struct st_sensor_data *sdata)
> +{
> + u8 status;
> + int ret;
> +
> + /* How would I know if I can't check it? */
> + if (!sdata->sensor_settings->drdy_irq.addr_stat_drdy)
> + return -EINVAL;
> +
> + /* No scan mask, no interrupt */
> + if (!indio_dev->active_scan_mask)
> + return 0;
> +
> + ret = sdata->tf->read_byte(&sdata->tb, sdata->dev,
> + sdata->sensor_settings->drdy_irq.addr_stat_drdy,
> + &status);
> + if (ret < 0) {
> + dev_err(sdata->dev,
> + "error checking samples available\n");
> + return ret;
> + }
> + /*
> + * the lower bits of .active_scan_mask[0] is directly mapped
> + * to the channels on the sensor: either bit 0 for
> + * one-dimensional sensors, or e.g. x,y,z for accelerometers,
> + * gyroscopes or magnetometers. No sensor use more than 3
> + * channels, so cut the other status bits here.
> + */
> + status &= 0x07;
> +
> + if (status & (u8)indio_dev->active_scan_mask[0])
> + return 1;
> +
> + return 0;
> +}
> +
> +/**
> * st_sensors_irq_handler() - top half of the IRQ-based triggers
> * @irq: irq number
> * @p: private handler data
> @@ -43,44 +87,43 @@ irqreturn_t st_sensors_irq_thread(int irq, void *p)
> struct iio_trigger *trig = p;
> struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> struct st_sensor_data *sdata = iio_priv(indio_dev);
> - int ret;
>
> /*
> * If this trigger is backed by a hardware interrupt and we have a
> - * status register, check if this IRQ came from us
> + * status register, check if this IRQ came from us. Notice that
> + * we will process also if st_sensors_new_samples_available()
> + * returns negative: if we can't check status, then poll
> + * unconditionally.
> */
> - if (sdata->sensor_settings->drdy_irq.addr_stat_drdy) {
> - u8 status;
> -
> - ret = sdata->tf->read_byte(&sdata->tb, sdata->dev,
> - sdata->sensor_settings->drdy_irq.addr_stat_drdy,
> - &status);
> - if (ret < 0) {
> - dev_err(sdata->dev, "could not read channel status\n");
> - goto out_poll;
> - }
> - /*
> - * the lower bits of .active_scan_mask[0] is directly mapped
> - * to the channels on the sensor: either bit 0 for
> - * one-dimensional sensors, or e.g. x,y,z for accelerometers,
> - * gyroscopes or magnetometers. No sensor use more than 3
> - * channels, so cut the other status bits here.
> - */
> - status &= 0x07;
> + if (sdata->hw_irq_trigger &&
> + st_sensors_new_samples_available(indio_dev, sdata)) {
> + iio_trigger_poll_chained(p);
> + } else {
> + dev_dbg(sdata->dev, "spurious IRQ\n");
> + return IRQ_NONE;
> + }
>
> - /*
> - * If this was not caused by any channels on this sensor,
> - * return IRQ_NONE
> - */
> - if (!indio_dev->active_scan_mask)
> - return IRQ_NONE;
> - if (!(status & (u8)indio_dev->active_scan_mask[0]))
> - return IRQ_NONE;
> + /*
> + * If we have proper level IRQs the handler will be re-entered if
> + * the line is still active, so return here and come back in through
> + * the top half if need be.
> + */
> + if (!sdata->edge_irq)
> + return IRQ_HANDLED;
> +
> + /*
> + * If we are using egde IRQs, new samples arrived while processing
> + * the IRQ and those may be missed unless we pick them here, so poll
> + * again. If the sensor delivery frequency is very high, this thread
> + * turns into a polled loop handler.
> + */
> + while (sdata->hw_irq_trigger &&
> + st_sensors_new_samples_available(indio_dev, sdata)) {
> + dev_dbg(sdata->dev, "more samples came in during polling\n");
> + sdata->hw_timestamp = iio_get_time_ns();
> + iio_trigger_poll_chained(p);
> }
>
> -out_poll:
> - /* It's our IRQ: proceed to handle the register polling */
> - iio_trigger_poll_chained(p);
> return IRQ_HANDLED;
> }
>
> @@ -107,13 +150,18 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
> * If the IRQ is triggered on falling edge, we need to mark the
> * interrupt as active low, if the hardware supports this.
> */
> - if (irq_trig == IRQF_TRIGGER_FALLING) {
> + switch(irq_trig) {
> + case IRQF_TRIGGER_FALLING:
> + case IRQF_TRIGGER_LOW:
> if (!sdata->sensor_settings->drdy_irq.addr_ihl) {
> dev_err(&indio_dev->dev,
> - "falling edge specified for IRQ but hardware "
> - "only support rising edge, will request "
> - "rising edge\n");
> - irq_trig = IRQF_TRIGGER_RISING;
> + "falling/low specified for IRQ "
> + "but hardware only support rising/high: "
> + "will request rising/high\n");
> + if (irq_trig == IRQF_TRIGGER_FALLING)
> + irq_trig = IRQF_TRIGGER_RISING;
> + if (irq_trig == IRQF_TRIGGER_LOW)
> + irq_trig = IRQF_TRIGGER_HIGH;
> } else {
> /* Set up INT active low i.e. falling edge */
> err = st_sensors_write_data_with_mask(indio_dev,
> @@ -122,20 +170,39 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
> if (err < 0)
> goto iio_trigger_free;
> dev_info(&indio_dev->dev,
> - "interrupts on the falling edge\n");
> + "interrupts on the falling edge or "
> + "active low level\n");
> }
> - } else if (irq_trig == IRQF_TRIGGER_RISING) {
> + break;
> + case IRQF_TRIGGER_RISING:
> dev_info(&indio_dev->dev,
> "interrupts on the rising edge\n");
> -
> - } else {
> + break;
> + case IRQF_TRIGGER_HIGH:
> + dev_info(&indio_dev->dev,
> + "interrupts active high level\n");
> + break;
> + default:
> + /* This is the most preferred mode, if possible */
> dev_err(&indio_dev->dev,
> - "unsupported IRQ trigger specified (%lx), only "
> - "rising and falling edges supported, enforce "
> + "unsupported IRQ trigger specified (%lx), enforce "
> "rising edge\n", irq_trig);
> irq_trig = IRQF_TRIGGER_RISING;
> }
>
> + /* Tell the interrupt handler that we're dealing with edges */
> + if (irq_trig == IRQF_TRIGGER_FALLING ||
> + irq_trig == IRQF_TRIGGER_RISING)
> + sdata->edge_irq = true;
> + else
> + /*
> + * If we're not using edges (i.e. level interrupts) we
> + * just mask off the IRQ, handle one interrupt, then
> + * if the line is still low, we return to the
> + * interrupt handler top half again and start over.
> + */
> + irq_trig |= IRQF_ONESHOT;
> +
> /*
> * If the interrupt pin is Open Drain, by definition this
> * means that the interrupt line may be shared with other
> @@ -148,9 +215,6 @@ int st_sensors_allocate_trigger(struct iio_dev *indio_dev,
> sdata->sensor_settings->drdy_irq.addr_stat_drdy)
> irq_trig |= IRQF_SHARED;
>
> - /* Let's create an interrupt thread masking the hard IRQ here */
> - irq_trig |= IRQF_ONESHOT;
> -
> err = request_threaded_irq(sdata->get_irq_data_ready(indio_dev),
> st_sensors_irq_handler,
> st_sensors_irq_thread,
> diff --git a/include/linux/iio/common/st_sensors.h b/include/linux/iio/common/st_sensors.h
> index 99403b19092f..c9efe0f809e5 100644
> --- a/include/linux/iio/common/st_sensors.h
> +++ b/include/linux/iio/common/st_sensors.h
> @@ -223,6 +223,7 @@ struct st_sensor_settings {
> * @get_irq_data_ready: Function to get the IRQ used for data ready signal.
> * @tf: Transfer function structure used by I/O operations.
> * @tb: Transfer buffers and mutex used by I/O operations.
> + * @edge_irq: the IRQ triggers on edges and need special handling.
> * @hw_irq_trigger: if we're using the hardware interrupt on the sensor.
> * @hw_timestamp: Latest timestamp from the interrupt handler, when in use.
> */
> @@ -250,6 +251,7 @@ struct st_sensor_data {
> const struct st_sensor_transfer_function *tf;
> struct st_sensor_transfer_buffer tb;
>
> + bool edge_irq;
> bool hw_irq_trigger;
> s64 hw_timestamp;
> };
>
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
