gpio: omap: Fix lost edge interrupts

Commit Message

Ladislav Michl Sept. 20, 2017, 12:54 p.m. UTC

On Fri, Sep 15, 2017 at 06:21:53PM -0500, Grygorii Strashko wrote:
> On 09/15/2017 04:45 PM, Ladislav Michl wrote:
> > From: Grygorii Strashko <[e-mail]>
> > 
> > Edge interrupts are already cleared in omap_gpio_irq_handler,
> > so clearing them again in omap_gpio_ack_irq causes lost
> > interrupts.
> > 
> > Cc: [e-mail]
> > Signed-off-by: Grygorii Strashko <[e-mail]>
> > Signed-off-by: Ladislav Michl <[e-mail]>
> > ---
> >   drivers/gpio/gpio-omap.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> > index dbf869fb63ce..a25738862129 100644
> > --- a/drivers/gpio/gpio-omap.c
> > +++ b/drivers/gpio/gpio-omap.c
> > @@ -811,7 +811,12 @@ static void omap_gpio_ack_irq(struct irq_data *d)
> >   	struct gpio_bank *bank = omap_irq_data_get_bank(d);
> >   	unsigned offset = d->hwirq;
> >   
> > -	omap_clear_gpio_irqstatus(bank, offset);
> > +	/*
> > +	 * Edge GPIOs are already cleared during handling, clearing
> > +	 * them here again will cause lost interrupts.
> > +	 */
> > +	if (bank->level_mask & BIT(offset))
> > +		omap_clear_gpio_irqstatus(bank, offset);
> >   }
> >   
> >   static void omap_gpio_mask_irq(struct irq_data *d)
> > 
> 
> Huh. You are back :) Thanks a lot for testing this and for your time.
> 
> Actually, I'd like you to add one more change in this patch as it make no sense
> to enable/disable edge IRQs while clearing status.
> 
>                 /* clear edge sensitive interrupts before handler(s) are
>                 called so that we don't miss any interrupt occurred while
>                 executing them */
> -               omap_disable_gpio_irqbank(bank, isr_saved & ~level_mask);
> -               omap_clear_gpio_irqbank(bank, isr_saved & ~level_mask);
> -               omap_enable_gpio_irqbank(bank, isr_saved & ~level_mask);
> +               if (isr_saved & ~level_mask)
> +                       omap_clear_gpio_irqbank(bank, isr_saved & ~level_mask);
> 
> Also, It seems your idea to use handle_simple_irq() for edge IRQ should also work. like
> 
> @@ -518,7 +518,12 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
>         if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
>                 irq_set_handler_locked(d, handle_level_irq);
>         else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
> -               irq_set_handler_locked(d, handle_edge_irq);
> +               /*
> +                * Edge GPIOs are already cleared during handling and not
> +                * masked, clearing them here again will cause lost interrupts.
> +                * So just use handle_simple_irq.
> +                */
> +               irq_set_handler_locked(d, handle_simple_irq);
> 
> 
> So, I think it will look better If you can check above and it'll works the same.

Squashed into appended patch, verified using signal generator and scope.
I'll send v2 after objections timeout expires.

Btw, what is wa_lock protecting?

Patch

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index dbf869fb63ce..bb280b9a7f9f 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -518,7 +518,7 @@  static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
 	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
 		irq_set_handler_locked(d, handle_level_irq);
 	else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
-		irq_set_handler_locked(d, handle_edge_irq);
+		irq_set_handler_locked(d, handle_simple_irq);
 
 	return 0;
 
@@ -678,7 +678,7 @@  static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
 static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank)
 {
 	void __iomem *isr_reg = NULL;
-	u32 isr;
+	u32 enabled, isr, level_mask;
 	unsigned int bit;
 	struct gpio_bank *bank = gpiobank;
 	unsigned long wa_lock_flags;
@@ -691,23 +691,21 @@  static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank)
 	pm_runtime_get_sync(bank->chip.parent);
 
 	while (1) {
-		u32 isr_saved, level_mask = 0;
-		u32 enabled;
-
 		raw_spin_lock_irqsave(&bank->lock, lock_flags);
 
 		enabled = omap_get_gpio_irqbank_mask(bank);
-		isr_saved = isr = readl_relaxed(isr_reg) & enabled;
+		isr = readl_relaxed(isr_reg) & enabled;
 
 		if (bank->level_mask)
 			level_mask = bank->level_mask & enabled;
+		else
+			level_mask = 0;
 
 		/* clear edge sensitive interrupts before handler(s) are
 		called so that we don't miss any interrupt occurred while
 		executing them */
-		omap_disable_gpio_irqbank(bank, isr_saved & ~level_mask);
-		omap_clear_gpio_irqbank(bank, isr_saved & ~level_mask);
-		omap_enable_gpio_irqbank(bank, isr_saved & ~level_mask);
+		if (isr & ~level_mask)
+			omap_clear_gpio_irqbank(bank, isr & ~level_mask);
 
 		raw_spin_unlock_irqrestore(&bank->lock, lock_flags);
 
@@ -811,7 +809,12 @@  static void omap_gpio_ack_irq(struct irq_data *d)
 	struct gpio_bank *bank = omap_irq_data_get_bank(d);
 	unsigned offset = d->hwirq;
 
-	omap_clear_gpio_irqstatus(bank, offset);
+	/*
+	 * Edge GPIOs are already cleared during handling, clearing
+	 * them here again will cause lost interrupts.
+	 */
+	if (bank->level_mask & BIT(offset))
+		omap_clear_gpio_irqstatus(bank, offset);
 }
 
 static void omap_gpio_mask_irq(struct irq_data *d)