Re: GPIO level IRQ fires twice each time.

[Markus Schneider-Pargmann <msp@xxxxxxxxxxxx>]

Hi everyone,

On Thu, Apr 14, 2022 at 08:39:21AM +0000, Markus Mirevik wrote:
> Hi Grygorii
> 
> > On 24/01/2022 09:58, Lars-Peter Clausen wrote:
> > > On 1/24/22 08:56, Lars-Peter Clausen wrote:
> > >> On 1/24/22 08:12, Markus Mirevik wrote:
> > >>>> On Fri, Jan 21, 2022 at 09:03:43AM +0000, Markus Mirevik wrote:
> > >>>>> I have a problem with a custom bord based on SoC am335x and a
> > >>>>> driver
> > >>>> utilizing a GPIO line for interrupts.
> > >>>>> I have two mcp2518fd chip connected on one SPI line and everything
> > >>>> works, but it's hogs a lot of CPU.
> > >>>>> In the current setup only one chip is connected and it only receives
> > packets.
> > >>>>>
> > >>>>> The mcp2518fd is connected with 2 interrupt lines one "main" and
> > >>>>> one for
> > >>>> rx frames.
> > >>>>> The problem is that for every frame received the interrupt handler
> > >>>>> is run
> > >>>> twice, which is kind of expensive since it's a SPI call to the chip
> > >>>> to check interrupt registers.
> > >>>>> To me it looks like the interrupt is fired again as soon as it's unmasked.
> > >>>> Either because it's queued? or maybe not cleared internally?
> > >>>>> I have scoped the interrupt signal and its real good without any
> > glitches.
> > >>>>>
> > >>>>> I'm currently running a yocto build:
> > >>>>> Linux botekcc 5.10.79-yocto-tiny #1 SMP Tue Nov 16 03:57:43 UTC
> > >>>>> 2021 armv7l armv7l armv7l GNU/Linux
> > >>>>>
> > >>>>> But the mcp251xfd driver is from net-next/master
> > >>>>>
> > >>>>> mcp251xfd_irq is the irqhandler for the mcp2518fd and is added like
> > this:
> > >>>>> err = request_threaded_irq(spi->irq, NULL, mcp251xfd_irq,
> > >>>>>                                     IRQF_SHARED | IRQF_ONESHOT,
> > >>>>> dev_name(&spi->dev), priv);
> > >>>>>
> > >>>> You haven't set a IRQF_TRIGGER flag, so you are getting the
> > >>>> "as-already- configured" behaviour, which on your setup is both
> > edges?
> > >>>> Try adding IRQF_TRIGGER_RISING, IRQF_TRIGGER_FALLING,
> > >>>> IRQF_TRIGGER_HIGH or IRQF_TRIGGER_LOW, as appropriate to your
> > use
> > >>>> case, to your flags.
> > >>>>
> > >>>> Cheers,
> > >>>> Kent.
> > >>> I have tried with the IRQF_TRIGGGER_LOW flag as well. With same
> > result. i.e the interrupt is fired again as soon as the handler is ready. Even if
> > the interrupt line is deactivated.
> > >>> However if I change the trigger to edge falling the interrupt will only fire
> > once. But his will inevitably lead to a missed edge eventually.
> > >>
> > >> Depending on how the mcp2518 GPIO controller works internally its
> > >> driver might have to use the handle_fasteoi_irq() flow to avoid this.
> > >> It is not uncommon to have hardware which needs a level IRQ acked
> > >> after the interrupt handler has run, rather than before like the
> > >> handle_level_irq() does. E.g.
> > >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/co
> > >> mmit/drivers/gpio/gpio-
> > zynq.c?id=6dd859508336f0fd078fd62f3b9fe42a32aa
> > >> 38e2
> > >>
> > >> - Lars
> > >>
> > > Sorry, I meant `Depending on how the am335x interrupt controller
> > > works...`
> > >
> > 
> > 
> > Could  you try to test below diff (may not be applied cleanly):
> > 
> > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index
> > 2a4a11634dd1..41ec54c3609f 100644
> > --- a/drivers/gpio/gpio-omap.c
> > +++ b/drivers/gpio/gpio-omap.c
> > @@ -896,6 +896,8 @@ static void omap_gpio_unmask_irq(struct irq_data
> > *d)
> >          raw_spin_lock_irqsave(&bank->lock, flags);
> >          omap_set_gpio_irqenable(bank, offset, 1);
> > 
> > +       if (trigger)
> > +               omap_set_gpio_triggering(bank, offset, trigger);
> >          /*
> >           * For level-triggered GPIOs, clearing must be done after the source
> >           * is cleared, thus after the handler has run. OMAP4 needs this done
> > @@ -905,9 +907,6 @@ static void omap_gpio_unmask_irq(struct irq_data
> > *d)
> >              trigger & (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
> >                  omap_clear_gpio_irqstatus(bank, offset);
> > 
> > -       if (trigger)
> > -               omap_set_gpio_triggering(bank, offset, trigger);
> > -
> >          raw_spin_unlock_irqrestore(&bank->lock, flags);
> >   }
> > 
> > Assumption - clearing IRQ status may require IRQ type configured.
> > 
> 
> I  have moved that part suggested in your diff and it looks like it has solved the problem! 
> 
> The interrupt associated with the GPIO module still fires twice
> 
> (cat /proc/interrupts)
> 19:     147227      INTC  96 Level     44e07000.gpio
> 
> But the interrupt in the mcp251xfd driver is only fired once. 
> 
> 64:      76673  44e07000.gpio  22 Level     spi1.1
> 
> And since it's the spi call that worries me the most I think this is fine. 
> Thank you!!

I just found this thread searching for the exact same issue, and the
patch works for me as well. Is there a patch somewhere already? Checking
6.1-rc it doesn't seem to have been submitted?

I am not familiar with the gpio-omap driver at the moment, so can we
submit this patch or is it just a hack?

Best,
Markus