On Fri, 22 Nov 2019 16:08:14 +0100 Fabrice Gasnier <fabrice.gasnier@xxxxxx> wrote: > Enable overrun interrupt on STM32 ADC. In case data register hasn't been > read (by CPU or DMA), overrun condition is detected when there's new > conversion data available. Stop grabbing data and log an error message. > Use a threaded irq to avoid printing the error message from hard irq > context. > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@xxxxxx> Absolutely makes sense to notify the user this occurred and to stop the current process. It may be useful to give a hint on how to restart capture? Is this a condition that can occur under 'normal conditions' such as something else grabbing the CPU for too long? I'd just like to understand if we are dealing with a 'fault' case of just one where things go wrong under weird conditions. Patch itself looks fine. Thanks, Jonathan > --- > drivers/iio/adc/stm32-adc-core.c | 14 +++++------ > drivers/iio/adc/stm32-adc-core.h | 9 +++++++ > drivers/iio/adc/stm32-adc.c | 53 ++++++++++++++++++++++++++++++++++++++-- > 3 files changed, 67 insertions(+), 9 deletions(-) > > diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c > index 6537f4f..97655d7 100644 > --- a/drivers/iio/adc/stm32-adc-core.c > +++ b/drivers/iio/adc/stm32-adc-core.c > @@ -280,21 +280,21 @@ static int stm32h7_adc_clk_sel(struct platform_device *pdev, > static const struct stm32_adc_common_regs stm32f4_adc_common_regs = { > .csr = STM32F4_ADC_CSR, > .ccr = STM32F4_ADC_CCR, > - .eoc1_msk = STM32F4_EOC1, > - .eoc2_msk = STM32F4_EOC2, > - .eoc3_msk = STM32F4_EOC3, > + .eoc1_msk = STM32F4_EOC1 | STM32F4_OVR1, > + .eoc2_msk = STM32F4_EOC2 | STM32F4_OVR2, > + .eoc3_msk = STM32F4_EOC3 | STM32F4_OVR3, > .ier = STM32F4_ADC_CR1, > - .eocie_msk = STM32F4_EOCIE, > + .eocie_msk = STM32F4_EOCIE | STM32F4_OVRIE, > }; > > /* STM32H7 common registers definitions */ > static const struct stm32_adc_common_regs stm32h7_adc_common_regs = { > .csr = STM32H7_ADC_CSR, > .ccr = STM32H7_ADC_CCR, > - .eoc1_msk = STM32H7_EOC_MST, > - .eoc2_msk = STM32H7_EOC_SLV, > + .eoc1_msk = STM32H7_EOC_MST | STM32H7_OVR_MST, > + .eoc2_msk = STM32H7_EOC_SLV | STM32H7_OVR_SLV, > .ier = STM32H7_ADC_IER, > - .eocie_msk = STM32H7_EOCIE, > + .eocie_msk = STM32H7_EOCIE | STM32H7_OVRIE, > }; > > static const unsigned int stm32_adc_offset[STM32_ADC_MAX_ADCS] = { > diff --git a/drivers/iio/adc/stm32-adc-core.h b/drivers/iio/adc/stm32-adc-core.h > index 2579d51..2322809 100644 > --- a/drivers/iio/adc/stm32-adc-core.h > +++ b/drivers/iio/adc/stm32-adc-core.h > @@ -51,10 +51,12 @@ > #define STM32F4_ADC_CCR (STM32_ADCX_COMN_OFFSET + 0x04) > > /* STM32F4_ADC_SR - bit fields */ > +#define STM32F4_OVR BIT(5) > #define STM32F4_STRT BIT(4) > #define STM32F4_EOC BIT(1) > > /* STM32F4_ADC_CR1 - bit fields */ > +#define STM32F4_OVRIE BIT(26) > #define STM32F4_RES_SHIFT 24 > #define STM32F4_RES_MASK GENMASK(25, 24) > #define STM32F4_SCAN BIT(8) > @@ -72,8 +74,11 @@ > #define STM32F4_ADON BIT(0) > > /* STM32F4_ADC_CSR - bit fields */ > +#define STM32F4_OVR3 BIT(21) > #define STM32F4_EOC3 BIT(17) > +#define STM32F4_OVR2 BIT(13) > #define STM32F4_EOC2 BIT(9) > +#define STM32F4_OVR1 BIT(5) > #define STM32F4_EOC1 BIT(1) > > /* STM32F4_ADC_CCR - bit fields */ > @@ -103,10 +108,12 @@ > > /* STM32H7_ADC_ISR - bit fields */ > #define STM32MP1_VREGREADY BIT(12) > +#define STM32H7_OVR BIT(4) > #define STM32H7_EOC BIT(2) > #define STM32H7_ADRDY BIT(0) > > /* STM32H7_ADC_IER - bit fields */ > +#define STM32H7_OVRIE STM32H7_OVR > #define STM32H7_EOCIE STM32H7_EOC > > /* STM32H7_ADC_CR - bit fields */ > @@ -155,7 +162,9 @@ enum stm32h7_adc_dmngt { > #define STM32H7_LINCALFACT_MASK GENMASK(29, 0) > > /* STM32H7_ADC_CSR - bit fields */ > +#define STM32H7_OVR_SLV BIT(20) > #define STM32H7_EOC_SLV BIT(18) > +#define STM32H7_OVR_MST BIT(4) > #define STM32H7_EOC_MST BIT(2) > > /* STM32H7_ADC_CCR - bit fields */ > diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c > index 3b291d7..9361f92 100644 > --- a/drivers/iio/adc/stm32-adc.c > +++ b/drivers/iio/adc/stm32-adc.c > @@ -117,7 +117,9 @@ struct stm32_adc_regs { > * struct stm32_adc_regspec - stm32 registers definition > * @dr: data register offset > * @ier_eoc: interrupt enable register & eocie bitfield > + * @ier_ovr: interrupt enable register & overrun bitfield > * @isr_eoc: interrupt status register & eoc bitfield > + * @isr_ovr: interrupt status register & overrun bitfield > * @sqr: reference to sequence registers array > * @exten: trigger control register & bitfield > * @extsel: trigger selection register & bitfield > @@ -128,7 +130,9 @@ struct stm32_adc_regs { > struct stm32_adc_regspec { > const u32 dr; > const struct stm32_adc_regs ier_eoc; > + const struct stm32_adc_regs ier_ovr; > const struct stm32_adc_regs isr_eoc; > + const struct stm32_adc_regs isr_ovr; > const struct stm32_adc_regs *sqr; > const struct stm32_adc_regs exten; > const struct stm32_adc_regs extsel; > @@ -337,7 +341,9 @@ static const unsigned int stm32f4_adc_smp_cycles[STM32_ADC_MAX_SMP + 1] = { > static const struct stm32_adc_regspec stm32f4_adc_regspec = { > .dr = STM32F4_ADC_DR, > .ier_eoc = { STM32F4_ADC_CR1, STM32F4_EOCIE }, > + .ier_ovr = { STM32F4_ADC_CR1, STM32F4_OVRIE }, > .isr_eoc = { STM32F4_ADC_SR, STM32F4_EOC }, > + .isr_ovr = { STM32F4_ADC_SR, STM32F4_OVR }, > .sqr = stm32f4_sq, > .exten = { STM32F4_ADC_CR2, STM32F4_EXTEN_MASK, STM32F4_EXTEN_SHIFT }, > .extsel = { STM32F4_ADC_CR2, STM32F4_EXTSEL_MASK, > @@ -429,7 +435,9 @@ static const unsigned int stm32h7_adc_smp_cycles[STM32_ADC_MAX_SMP + 1] = { > static const struct stm32_adc_regspec stm32h7_adc_regspec = { > .dr = STM32H7_ADC_DR, > .ier_eoc = { STM32H7_ADC_IER, STM32H7_EOCIE }, > + .ier_ovr = { STM32H7_ADC_IER, STM32H7_OVRIE }, > .isr_eoc = { STM32H7_ADC_ISR, STM32H7_EOC }, > + .isr_ovr = { STM32H7_ADC_ISR, STM32H7_OVR }, > .sqr = stm32h7_sq, > .exten = { STM32H7_ADC_CFGR, STM32H7_EXTEN_MASK, STM32H7_EXTEN_SHIFT }, > .extsel = { STM32H7_ADC_CFGR, STM32H7_EXTSEL_MASK, > @@ -506,6 +514,18 @@ static void stm32_adc_conv_irq_disable(struct stm32_adc *adc) > adc->cfg->regs->ier_eoc.mask); > } > > +static void stm32_adc_ovr_irq_enable(struct stm32_adc *adc) > +{ > + stm32_adc_set_bits(adc, adc->cfg->regs->ier_ovr.reg, > + adc->cfg->regs->ier_ovr.mask); > +} > + > +static void stm32_adc_ovr_irq_disable(struct stm32_adc *adc) > +{ > + stm32_adc_clr_bits(adc, adc->cfg->regs->ier_ovr.reg, > + adc->cfg->regs->ier_ovr.mask); > +} > + > static void stm32_adc_set_res(struct stm32_adc *adc) > { > const struct stm32_adc_regs *res = &adc->cfg->regs->res; > @@ -1205,6 +1225,19 @@ static int stm32_adc_read_raw(struct iio_dev *indio_dev, > } > } > > +static irqreturn_t stm32_adc_threaded_isr(int irq, void *data) > +{ > + struct stm32_adc *adc = data; > + struct iio_dev *indio_dev = iio_priv_to_dev(adc); > + const struct stm32_adc_regspec *regs = adc->cfg->regs; > + u32 status = stm32_adc_readl(adc, regs->isr_eoc.reg); > + > + if (status & regs->isr_ovr.mask) > + dev_err(&indio_dev->dev, "Overrun interrupt, stopping.\n"); > + > + return IRQ_HANDLED; > +} > + > static irqreturn_t stm32_adc_isr(int irq, void *data) > { > struct stm32_adc *adc = data; > @@ -1212,6 +1245,17 @@ static irqreturn_t stm32_adc_isr(int irq, void *data) > const struct stm32_adc_regspec *regs = adc->cfg->regs; > u32 status = stm32_adc_readl(adc, regs->isr_eoc.reg); > > + if (status & regs->isr_ovr.mask) { > + /* > + * Overrun occurred on regular conversions: data for wrong > + * channel may be read. Unconditionally disable interrupts > + * to stop processing data and print error message. > + */ > + stm32_adc_ovr_irq_disable(adc); > + stm32_adc_conv_irq_disable(adc); > + return IRQ_WAKE_THREAD; > + } > + > if (status & regs->isr_eoc.mask) { > /* Reading DR also clears EOC status flag */ > adc->buffer[adc->bufi] = stm32_adc_readw(adc, regs->dr); > @@ -1441,6 +1485,8 @@ static int __stm32_adc_buffer_postenable(struct iio_dev *indio_dev) > /* Reset adc buffer index */ > adc->bufi = 0; > > + stm32_adc_ovr_irq_enable(adc); > + > if (!adc->dma_chan) > stm32_adc_conv_irq_enable(adc); > > @@ -1481,6 +1527,8 @@ static void __stm32_adc_buffer_predisable(struct iio_dev *indio_dev) > if (!adc->dma_chan) > stm32_adc_conv_irq_disable(adc); > > + stm32_adc_ovr_irq_disable(adc); > + > if (adc->dma_chan) > dmaengine_terminate_sync(adc->dma_chan); > > @@ -1818,8 +1866,9 @@ static int stm32_adc_probe(struct platform_device *pdev) > if (adc->irq < 0) > return adc->irq; > > - ret = devm_request_irq(&pdev->dev, adc->irq, stm32_adc_isr, > - 0, pdev->name, adc); > + ret = devm_request_threaded_irq(&pdev->dev, adc->irq, stm32_adc_isr, > + stm32_adc_threaded_isr, > + 0, pdev->name, adc); > if (ret) { > dev_err(&pdev->dev, "failed to request IRQ\n"); > return ret;