On 30/06/16 20:42, Jonathan Cameron wrote:
> 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!
Ah. It crossed with the clock selection patch so have
fixed up the missing param to the timestamp read.
Thanks,
Jonathan
>
> 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
>
--
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
