| Message ID | 20190301190252.20615-1-tony@atomide.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: gpio-omap: fix level interrupt idling | expand |
On 01.03.19 21:02, Tony Lindgren wrote: > From: Russell King <rmk+kernel@armlinux.org.uk> > > Tony notes that the GPIO module does not idle when level interrupts are > in use, as the wakeup appears to get stuck. > > After extensive investigation, it appears that the wakeup will only be > cleared if the interrupt status register is cleared while the interrupt > is enabled. However, we are currently clearing it with the interrupt > disabled for level-based interrupts. > > It is acknowledged that this observed behaviour conflicts with a > statement in the TRM: > > CAUTION > After servicing the interrupt, the status bit in the interrupt status > register (GPIOi.GPIO_IRQSTATUS_0 or GPIOi.GPIO_IRQSTATUS_1) must be > reset and the interrupt line released (by setting the corresponding > bit of the interrupt status register to 1) before enabling an > interrupt for the GPIO channel in the interrupt-enable register > (GPIOi.GPIO_IRQSTATUS_SET_0 or GPIOi.GPIO_IRQSTATUS_SET_1) to prevent > the occurrence of unexpected interrupts when enabling an interrupt > for the GPIO channel. > > However, this does not appear to be a practical problem. > > Further, as reported by Grygorii Strashko <grygorii.strashko@ti.com>, > the TI Android kernel tree has an earlier similar patch as "GPIO: OMAP: > Fix the sequence to clear the IRQ status" saying: > > if the status is cleared after disabling the IRQ then sWAKEUP will not > be cleared and gates the module transition > > When we unmask the level interrupt after the interrupt has been handled, > enable the interrupt and only then clear the interrupt. If the interrupt > is still pending, the hardware will re-assert the interrupt status. > > Should the caution note in the TRM prove to be a problem, we could > use a clear-enable-clear sequence instead. > > Cc: Aaro Koskinen <aaro.koskinen@iki.fi> > Cc: Grygorii Strashko <grygorii.strashko@ti.com> > Cc: Keerthy <j-keerthy@ti.com> > Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > [tony@atomide.com: updated comments based on an earlier TI patch] > Signed-off-by: Tony Lindgren <tony@atomide.com> > --- > > Obviously no rush to get this into v5.0-rc cycle, but this should be > tagged with Cc stable after people have reviewed and tested this. > > --- > drivers/gpio/gpio-omap.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > Thank you. Acked-by: Grygorii Strashko <grygorii.strashko@ti.com>
On Fri, Mar 1, 2019 at 8:03 PM Tony Lindgren <tony@atomide.com> wrote: > From: Russell King <rmk+kernel@armlinux.org.uk> > > Tony notes that the GPIO module does not idle when level interrupts are > in use, as the wakeup appears to get stuck. > > After extensive investigation, it appears that the wakeup will only be > cleared if the interrupt status register is cleared while the interrupt > is enabled. However, we are currently clearing it with the interrupt > disabled for level-based interrupts. > > It is acknowledged that this observed behaviour conflicts with a > statement in the TRM: > > CAUTION > After servicing the interrupt, the status bit in the interrupt status > register (GPIOi.GPIO_IRQSTATUS_0 or GPIOi.GPIO_IRQSTATUS_1) must be > reset and the interrupt line released (by setting the corresponding > bit of the interrupt status register to 1) before enabling an > interrupt for the GPIO channel in the interrupt-enable register > (GPIOi.GPIO_IRQSTATUS_SET_0 or GPIOi.GPIO_IRQSTATUS_SET_1) to prevent > the occurrence of unexpected interrupts when enabling an interrupt > for the GPIO channel. > > However, this does not appear to be a practical problem. > > Further, as reported by Grygorii Strashko <grygorii.strashko@ti.com>, > the TI Android kernel tree has an earlier similar patch as "GPIO: OMAP: > Fix the sequence to clear the IRQ status" saying: > > if the status is cleared after disabling the IRQ then sWAKEUP will not > be cleared and gates the module transition > > When we unmask the level interrupt after the interrupt has been handled, > enable the interrupt and only then clear the interrupt. If the interrupt > is still pending, the hardware will re-assert the interrupt status. > > Should the caution note in the TRM prove to be a problem, we could > use a clear-enable-clear sequence instead. > > Cc: Aaro Koskinen <aaro.koskinen@iki.fi> > Cc: Grygorii Strashko <grygorii.strashko@ti.com> > Cc: Keerthy <j-keerthy@ti.com> > Cc: Peter Ujfalusi <peter.ujfalusi@ti.com> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > [tony@atomide.com: updated comments based on an earlier TI patch] > Signed-off-by: Tony Lindgren <tony@atomide.com> Patch applied! Really nice debugging here. Yours, Linus Walleij
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 @@ -883,14 +883,16 @@ static void omap_gpio_unmask_irq(struct irq_data *d) if (trigger) omap_set_gpio_triggering(bank, offset, trigger); - /* For level-triggered GPIOs, the clearing must be done after - * the HW source is cleared, thus after the handler has run */ - if (bank->level_mask & BIT(offset)) { - omap_set_gpio_irqenable(bank, offset, 0); + omap_set_gpio_irqenable(bank, offset, 1); + + /* + * For level-triggered GPIOs, clearing must be done after the source + * is cleared, thus after the handler has run. OMAP4 needs this done + * after enabing the interrupt to clear the wakeup status. + */ + if (bank->level_mask & BIT(offset)) omap_clear_gpio_irqstatus(bank, offset); - } - omap_set_gpio_irqenable(bank, offset, 1); raw_spin_unlock_irqrestore(&bank->lock, flags); }