Recent changes to the STM32 ADC irq handling broke the STM32F4 platforms
These two patches bring it back to a working state.
Yannick Brosseau (2):
iio: adc: stm32: Iterate through all ADCs in irq handler
iio: adc: stm32: Fix check for spurious IRQs on STM32F4
drivers/iio/adc/stm32-adc-core.c | 7 ++++++-
drivers/iio/adc/stm32-adc.c | 9 ++++++---
2 files changed, 12 insertions(+), 4 deletions(-)
--
2.36.0
The check for spurious IRQs introduced in 695e2f5c289bb assumed that the bits
in the control and status registers are aligned. This is true for the H7 and MP1
version, but not the F4.
Instead of comparing both registers bitwise, we check the bit in the status and control
for each interrupt we are interested in.
Signed-off-by: Yannick Brosseau <[email protected]>
---
drivers/iio/adc/stm32-adc.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index a68ecbda6480..5b0f138333ee 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -1422,9 +1422,10 @@ static irqreturn_t stm32_adc_threaded_isr(int irq, void *data)
return IRQ_HANDLED;
}
- if (!(status & mask))
+ if(!((status & regs->isr_eoc.mask) && (mask & regs->ier_eoc.mask)) ||
+ ((status & regs->isr_ovr.mask) && (mask & regs->ier_ovr.mask)))
dev_err_ratelimited(&indio_dev->dev,
- "Unexpected IRQ: IER=0x%08x, ISR=0x%08x\n",
+ "Unexpected IRQ: CR1/IER=0x%08x, SR/ISR=0x%08x\n",
mask, status);
return IRQ_NONE;
@@ -1438,7 +1439,9 @@ static irqreturn_t stm32_adc_isr(int irq, void *data)
u32 status = stm32_adc_readl(adc, regs->isr_eoc.reg);
u32 mask = stm32_adc_readl(adc, regs->ier_eoc.reg);
- if (!(status & mask))
+ /* Check that we have the interrupt we care about are enabled and active */
+ if(!((status & regs->isr_eoc.mask) && (mask & regs->ier_eoc.mask)) ||
+ ((status & regs->isr_ovr.mask) && (mask & regs->ier_ovr.mask)))
return IRQ_WAKE_THREAD;
if (status & regs->isr_ovr.mask) {
--
2.36.0
The irq handler was only checking the mask for the first ADCs in the case of the
F4 and H7 generation, since it was using the num_irq value. This patch add
the number of ADC in the common register, which map to the number of entries of
eoc_msk and ovr_msk in stm32_adc_common_regs.
Tested on a STM32F429NIH6
Signed-off-by: Yannick Brosseau <[email protected]>
---
drivers/iio/adc/stm32-adc-core.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c
index 142656232157..11c08c56acb0 100644
--- a/drivers/iio/adc/stm32-adc-core.c
+++ b/drivers/iio/adc/stm32-adc-core.c
@@ -64,6 +64,7 @@ struct stm32_adc_priv;
* @max_clk_rate_hz: maximum analog clock rate (Hz, from datasheet)
* @has_syscfg: SYSCFG capability flags
* @num_irqs: number of interrupt lines
+ * @num_adcs: number of ADC instances in the common registers
*/
struct stm32_adc_priv_cfg {
const struct stm32_adc_common_regs *regs;
@@ -71,6 +72,7 @@ struct stm32_adc_priv_cfg {
u32 max_clk_rate_hz;
unsigned int has_syscfg;
unsigned int num_irqs;
+ unsigned int num_adcs;
};
/**
@@ -352,7 +354,7 @@ static void stm32_adc_irq_handler(struct irq_desc *desc)
* before invoking the interrupt handler (e.g. call ISR only for
* IRQ-enabled ADCs).
*/
- for (i = 0; i < priv->cfg->num_irqs; i++) {
+ for (i = 0; i < priv->cfg->num_adcs; i++) {
if ((status & priv->cfg->regs->eoc_msk[i] &&
stm32_adc_eoc_enabled(priv, i)) ||
(status & priv->cfg->regs->ovr_msk[i]))
@@ -792,6 +794,7 @@ static const struct stm32_adc_priv_cfg stm32f4_adc_priv_cfg = {
.clk_sel = stm32f4_adc_clk_sel,
.max_clk_rate_hz = 36000000,
.num_irqs = 1,
+ .num_adcs = 3,
};
static const struct stm32_adc_priv_cfg stm32h7_adc_priv_cfg = {
@@ -800,6 +803,7 @@ static const struct stm32_adc_priv_cfg stm32h7_adc_priv_cfg = {
.max_clk_rate_hz = 36000000,
.has_syscfg = HAS_VBOOSTER,
.num_irqs = 1,
+ .num_adcs = 2,
};
static const struct stm32_adc_priv_cfg stm32mp1_adc_priv_cfg = {
@@ -808,6 +812,7 @@ static const struct stm32_adc_priv_cfg stm32mp1_adc_priv_cfg = {
.max_clk_rate_hz = 40000000,
.has_syscfg = HAS_VBOOSTER | HAS_ANASWVDD,
.num_irqs = 2,
+ .num_adcs = 2,
};
static const struct of_device_id stm32_adc_of_match[] = {
--
2.36.0
Hi Yannick,
Le ven., mai 6 2022 at 18:56:15 -0400, Yannick Brosseau
<[email protected]> a ?crit :
> Recent changes to the STM32 ADC irq handling broke the STM32F4
> platforms
> These two patches bring it back to a working state.
If the STM32 ADC was broken recently, and these two patches fix it,
then I'd expect to see a Fixes: tag on both commits and Cc:
linux-stable.
Cheers,
-Paul
> Yannick Brosseau (2):
> iio: adc: stm32: Iterate through all ADCs in irq handler
> iio: adc: stm32: Fix check for spurious IRQs on STM32F4
>
> drivers/iio/adc/stm32-adc-core.c | 7 ++++++-
> drivers/iio/adc/stm32-adc.c | 9 ++++++---
> 2 files changed, 12 insertions(+), 4 deletions(-)
>
> --
> 2.36.0
>
On Sat, 07 May 2022 00:05:00 +0100
Paul Cercueil <[email protected]> wrote:
> Hi Yannick,
>
> Le ven., mai 6 2022 at 18:56:15 -0400, Yannick Brosseau
> <[email protected]> a écrit :
> > Recent changes to the STM32 ADC irq handling broke the STM32F4
> > platforms
> > These two patches bring it back to a working state.
>
> If the STM32 ADC was broken recently, and these two patches fix it,
> then I'd expect to see a Fixes: tag on both commits and Cc:
> linux-stable.
I normally add the Cc: for stable, but don't mind for obvious cases
if patches come in with it already there.
Sometimes the marking can be timing dependent (no point in sending
things to stable if they are going to hit in the same cycle etc)
Absolutely agree on fixes tag though!
Thanks,
Jonathan
>
> Cheers,
> -Paul
>
> > Yannick Brosseau (2):
> > iio: adc: stm32: Iterate through all ADCs in irq handler
> > iio: adc: stm32: Fix check for spurious IRQs on STM32F4
> >
> > drivers/iio/adc/stm32-adc-core.c | 7 ++++++-
> > drivers/iio/adc/stm32-adc.c | 9 ++++++---
> > 2 files changed, 12 insertions(+), 4 deletions(-)
> >
> > --
> > 2.36.0
> >
>
>
On 5/7/22 00:56, Yannick Brosseau wrote:
> The check for spurious IRQs introduced in 695e2f5c289bb assumed that the bits
> in the control and status registers are aligned. This is true for the H7 and MP1
> version, but not the F4.
>
> Instead of comparing both registers bitwise, we check the bit in the status and control
> for each interrupt we are interested in.
>
Hi Yannick,
I propose a different approach, see here after.
Same as for patch one,
Fixes: 695e2f5c289b ("iio: adc: stm32-adc: fix a regression when using
dma and irq")
> Signed-off-by: Yannick Brosseau <[email protected]>
> ---
> drivers/iio/adc/stm32-adc.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index a68ecbda6480..5b0f138333ee 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
> @@ -1422,9 +1422,10 @@ static irqreturn_t stm32_adc_threaded_isr(int irq, void *data)
> return IRQ_HANDLED;
> }
>
> - if (!(status & mask))
> + if(!((status & regs->isr_eoc.mask) && (mask & regs->ier_eoc.mask)) ||
> + ((status & regs->isr_ovr.mask) && (mask & regs->ier_ovr.mask)))
> dev_err_ratelimited(&indio_dev->dev,
> - "Unexpected IRQ: IER=0x%08x, ISR=0x%08x\n",
> + "Unexpected IRQ: CR1/IER=0x%08x, SR/ISR=0x%08x\n",
> mask, status);
Here, a slightly different approach could be used... There's a long
pending discussion, where Olivier or I should push further patches to
support threadirqs (hopefully soon).
In this discussion with Jonathan [1], he exposed the need to remove this
message. Words from Jonathan:
"This seems 'unusual'. If this is a spurious interrupt we should be
returning IRQ_NONE and letting the spurious interrupt protection
stuff kick in."
[1]
https://lore.kernel.org/linux-arm-kernel/20210116175333.4d8684c5@archlinux/
So basically, I suggest to completely get rid of this message:
- if (!(status & mask))
- dev_err_ratelimited(&indio_dev->dev,
- "Unexpected IRQ: IER=0x%08x, ISR=0x%08x\n",
- mask, status);
>
> return IRQ_NONE;
> @@ -1438,7 +1439,9 @@ static irqreturn_t stm32_adc_isr(int irq, void *data)
> u32 status = stm32_adc_readl(adc, regs->isr_eoc.reg);
> u32 mask = stm32_adc_readl(adc, regs->ier_eoc.reg);
>
> - if (!(status & mask))
> + /* Check that we have the interrupt we care about are enabled and active */
> + if(!((status & regs->isr_eoc.mask) && (mask & regs->ier_eoc.mask)) ||
> + ((status & regs->isr_ovr.mask) && (mask & regs->ier_ovr.mask)))
> return IRQ_WAKE_THREAD;
Here the statement becomes useless, so it could be removed:
- u32 mask = stm32_adc_readl(adc, regs->ier_eoc.reg);
-
- if (!(status & mask))
- return IRQ_WAKE_THREAD;
This would avoid some complexity here (and so headaches or regressions
like the one you've hit).
This also should serve the two purposes:
- fall into kernel generic handler for spurious IRQs (by returning
IRQ_NONE below)
- by the way fix current issue in stm32f4
I Hope this is still inline with Jonathan's words earlier ;-)
Best Regards,
Fabrice
>
> if (status & regs->isr_ovr.mask) {
On 5/7/22 00:56, Yannick Brosseau wrote:
> The irq handler was only checking the mask for the first ADCs in the case of the
> F4 and H7 generation, since it was using the num_irq value. This patch add
> the number of ADC in the common register, which map to the number of entries of
> eoc_msk and ovr_msk in stm32_adc_common_regs.
>
> Tested on a STM32F429NIH6
>
> Signed-off-by: Yannick Brosseau <[email protected]>
> ---
> drivers/iio/adc/stm32-adc-core.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
Hi Yannick,
I confirm you've identified and analyzed here a regression. This is
something that I noticed also earlier (Olivier and I had some patches in
our downstream tree for that. Shame on me that I haven't sent them right
away).
So, I'm fine with your current patch. Please add a Fixes: tag as
suggested by Jonathan and Paul in the cover letter.
Fixes: 695e2f5c289b ("iio: adc: stm32-adc: fix a regression when using
dma and irq")
While you're at it, I suggest to also mention "fix" in the commit
tittle, to make it clear: e.g. it fixes a regression in irq handling on
stm32f4 and stm32h7.
> diff --git a/drivers/iio/adc/stm32-adc-core.c b/drivers/iio/adc/stm32-adc-core.c
> index 142656232157..11c08c56acb0 100644
> --- a/drivers/iio/adc/stm32-adc-core.c
> +++ b/drivers/iio/adc/stm32-adc-core.c
> @@ -64,6 +64,7 @@ struct stm32_adc_priv;
> * @max_clk_rate_hz: maximum analog clock rate (Hz, from datasheet)
> * @has_syscfg: SYSCFG capability flags
> * @num_irqs: number of interrupt lines
> + * @num_adcs: number of ADC instances in the common registers
Minor comment here, this rather is the 'maximum' number of ADC
instances. E.g. on stm32h7, there are two ADC blocks: ADC12 with 2 ADCs
and ADC3 with 1 ADC instantiated separately. So you could update the
comment and/or variable name.
Thanks & Best Regards,
Fabrice
> */
> struct stm32_adc_priv_cfg {
> const struct stm32_adc_common_regs *regs;
> @@ -71,6 +72,7 @@ struct stm32_adc_priv_cfg {
> u32 max_clk_rate_hz;
> unsigned int has_syscfg;
> unsigned int num_irqs;
> + unsigned int num_adcs;
> };
>
> /**
> @@ -352,7 +354,7 @@ static void stm32_adc_irq_handler(struct irq_desc *desc)
> * before invoking the interrupt handler (e.g. call ISR only for
> * IRQ-enabled ADCs).
> */
> - for (i = 0; i < priv->cfg->num_irqs; i++) {
> + for (i = 0; i < priv->cfg->num_adcs; i++) {
> if ((status & priv->cfg->regs->eoc_msk[i] &&
> stm32_adc_eoc_enabled(priv, i)) ||
> (status & priv->cfg->regs->ovr_msk[i]))
> @@ -792,6 +794,7 @@ static const struct stm32_adc_priv_cfg stm32f4_adc_priv_cfg = {
> .clk_sel = stm32f4_adc_clk_sel,
> .max_clk_rate_hz = 36000000,
> .num_irqs = 1,
> + .num_adcs = 3,
> };
>
> static const struct stm32_adc_priv_cfg stm32h7_adc_priv_cfg = {
> @@ -800,6 +803,7 @@ static const struct stm32_adc_priv_cfg stm32h7_adc_priv_cfg = {
> .max_clk_rate_hz = 36000000,
> .has_syscfg = HAS_VBOOSTER,
> .num_irqs = 1,
> + .num_adcs = 2,
> };
>
> static const struct stm32_adc_priv_cfg stm32mp1_adc_priv_cfg = {
> @@ -808,6 +812,7 @@ static const struct stm32_adc_priv_cfg stm32mp1_adc_priv_cfg = {
> .max_clk_rate_hz = 40000000,
> .has_syscfg = HAS_VBOOSTER | HAS_ANASWVDD,
> .num_irqs = 2,
> + .num_adcs = 2,
> };
>
> static const struct of_device_id stm32_adc_of_match[] = {