[PATCH] gpio: gpio-omap: Fix lost edge wake-up interrupts

[PATCH] gpio: gpio-omap: Fix lost edge wake-up interrupts

[Tony Lindgren]

If an edge interrupt triggers while entering idle just before we save
GPIO datain register to saved_datain, the triggered GPIO will not be
noticed on wake-up. This is because the saved_datain and GPIO datain
are the same. Let's fix this by ignoring any pending edge interrupts
in saved_datain.

This can be somewhat easily reproduced by pinging an idle system with
smsc911x Ethernet interface configured IRQ_TYPE_EDGE_FALLING. At some
point the smsc911x interrupts will just stop triggering.

Note that in the long run we may be able to cancel entering idle by
returning an error in gpio_omap_cpu_notifier(). But let's fix the bug
first.

Also note that because of the recent clean-up efforts this patch does
not apply directly to older kernels. This does fix a long term issue
though, and can be backported as needed.

Cc: Aaro Koskinen <aaro.koskinen@iki.fi>
Cc: Grygorii Strashko <grygorii.strashko@ti.com>
Cc: Keerthy <j-keerthy@ti.com>
Cc: Ladislav Michl <ladis@linux-mips.org>
Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
Cc: Russell King <rmk+kernel@armlinux.org.uk>
Cc: Tero Kristo <t-kristo@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/gpio/gpio-omap.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1279,7 +1279,14 @@ static void omap_gpio_idle(struct gpio_bank *bank, bool may_lose_context)
 	void __iomem *base = bank->base;
 	u32 nowake;
 
+	/*
+	 * Save datain register to trigger edge interrupts on unidle for GPIOS
+	 * that are not wake-up capable. Ignore any enabled_non_wakeup_gpios
+	 * that may have just triggered as we're entering idle. Otherwise unidle
+	 * will not notice them.
+	 */
 	bank->saved_datain = readl_relaxed(base + bank->regs->datain);
+	bank->saved_datain |= bank->enabled_non_wakeup_gpios;
 
 	if (!bank->enabled_non_wakeup_gpios)
 		goto update_gpio_context_count;
-- 
2.21.0

Re: [PATCH] gpio: gpio-omap: Fix lost edge wake-up interrupts

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [190508 11:20]:
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -1279,7 +1279,14 @@ static void omap_gpio_idle(struct gpio_bank *bank, bool may_lose_context)
>  	void __iomem *base = bank->base;
>  	u32 nowake;
>  
> +	/*
> +	 * Save datain register to trigger edge interrupts on unidle for GPIOS
> +	 * that are not wake-up capable. Ignore any enabled_non_wakeup_gpios
> +	 * that may have just triggered as we're entering idle. Otherwise unidle
> +	 * will not notice them.
> +	 */
>  	bank->saved_datain = readl_relaxed(base + bank->regs->datain);
> +	bank->saved_datain |= bank->enabled_non_wakeup_gpios;

Oops, sorry this is not complete yet. We need to enable or clear
the possible pending interrupt in saved_datain based on the edge
interrupt polarity. I'll fix and resend.

Regards,

Tony

Re: [PATCH] gpio: gpio-omap: Fix lost edge wake-up interrupts

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [190508 20:40]:
> * Tony Lindgren <tony@atomide.com> [190508 11:20]:
> > --- a/drivers/gpio/gpio-omap.c
> > +++ b/drivers/gpio/gpio-omap.c
> > @@ -1279,7 +1279,14 @@ static void omap_gpio_idle(struct gpio_bank *bank, bool may_lose_context)
> >  	void __iomem *base = bank->base;
> >  	u32 nowake;
> >  
> > +	/*
> > +	 * Save datain register to trigger edge interrupts on unidle for GPIOS
> > +	 * that are not wake-up capable. Ignore any enabled_non_wakeup_gpios
> > +	 * that may have just triggered as we're entering idle. Otherwise unidle
> > +	 * will not notice them.
> > +	 */
> >  	bank->saved_datain = readl_relaxed(base + bank->regs->datain);
> > +	bank->saved_datain |= bank->enabled_non_wakeup_gpios;
> 
> Oops, sorry this is not complete yet. We need to enable or clear
> the possible pending interrupt in saved_datain based on the edge
> interrupt polarity. I'll fix and resend.

Below seems to behave for me based on light testing..
Might be worth thinking about this a bit more, anybody got
better ideas? :)

Regards,

Tony

8< --------------------
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -1277,10 +1277,24 @@ static void omap_gpio_idle(struct gpio_bank *bank, bool may_lose_context)
 {
 	struct device *dev = bank->chip.parent;
 	void __iomem *base = bank->base;
-	u32 nowake;
+	u32 mask, nowake;
 
+	/*
+	 * Save datain register to trigger edge interrupts on unidle for GPIOS
+	 * that are not wake-up capable. Ignore any enabled_non_wakeup_gpios
+	 * that may have just triggered as we're entering idle. Otherwise unidle
+	 * will not notice them. Note that this does not help with EDGE_BOTH.
+	 */
 	bank->saved_datain = readl_relaxed(base + bank->regs->datain);
 
+	mask = bank->enabled_non_wakeup_gpios & bank->context.fallingdetect;
+	mask &= ~bank->context.risingdetect;
+	bank->saved_datain |= mask;
+
+	mask = bank->enabled_non_wakeup_gpios & bank->context.risingdetect;
+	mask &= ~bank->context.fallingdetect;
+	bank->saved_datain &= ~mask;
+
 	if (!bank->enabled_non_wakeup_gpios)
 		goto update_gpio_context_count;
 
-- 
2.21.0