Message ID | 1460458351-24187-1-git-send-email-grygorii.strashko@ti.com |
---|---|
State | New |
Headers | show |
On 4/12/2016 3:52 AM, Grygorii Strashko wrote: > Now GPIO IRQ loss is observed on dra7-evm after suspend/resume cycle > in the following case: > extcon_usb1(id_irq) -> pcf8575.gpio1 -> omapgpio6.gpio11 -> gic > > the extcon_usb1 is wake up source and it enables IRQ wake up for > id_irq by calling enable/disable_irq_wake() during suspend/resume > which, in turn, causes execution of omap_gpio_wake_enable(). And > omap_gpio_wake_enable() will set/clear corresponding bit in > GPIO_IRQWAKEN_x register. > > omapgpio6 configuration after boot - wakeup is enabled for GPIO IRQs > by default from omap_gpio_irq_type: > GPIO_IRQSTATUS_SET_0 | 0x00000400 > GPIO_IRQSTATUS_CLR_0 | 0x00000400 > GPIO_IRQWAKEN_0 | 0x00000400 > GPIO_RISINGDETECT | 0x00000000 > GPIO_FALLINGDETECT | 0x00000400 > > omapgpio6 configuration after after suspend/resume cycle: > GPIO_IRQSTATUS_SET_0 | 0x00000400 > GPIO_IRQSTATUS_CLR_0 | 0x00000400 > GPIO_IRQWAKEN_0 | 0x00000000 <--- > GPIO_RISINGDETECT | 0x00000000 > GPIO_FALLINGDETECT | 0x00000400 > > As result, system will start to lose interrupts from pcf8575 GPIO > expander, because when OMAP GPIO IP is in smart-idle wakeup mode, there > is no guarantee that transition(s) on input non wake up GPIO pin will > trigger asynchronous wake-up request to PRCM and then IRQ generation. > IRQ will be generated when GPIO is in active mode - for example, some > time after accessing GPIO bank registers IRQs will be generated > normally, but issue will happen again once PRCM will put GPIO in low > power smart-idle wakeup mode. > > Note 1. Issue is not reproduced if debounce clk is enabled for GPIO > bank. > > Note 2. Issue hardly reproducible if GPIO pins group contains both > wakeup/non-wakeup gpios - for example, it will be hard to reproduce > issue with pin2 if GPIO_IRQWAKEN_0=0x1 GPIO_IRQSTATUS_SET_0=0x3 > GPIO_FALLINGDETECT = 0x3 (TRM "Power Saving by Grouping the Edge/Level > Detection"). > > Note 3. There nothing common bitween System wake up and OMAP GPIO bank > IP wake up logic - the last one defines how the GPIO bank ON-IDLE-ON > transition will happen inside SoC under control of PRCM. > > Hence, fix the problem by removing omap_set_gpio_wakeup() function > completely and so keeping always in sync GPIO IRQ mask/unmask > (IRQSTATUS_SET) and wake up enable (GPIO_IRQWAKEN) bits; and adding > IRQCHIP_MASK_ON_SUSPEND flag in OMAP GPIO irqchip. That way non wakeup > GPIO IRQs will be properly masked/unmask by IRQ PM core during > suspend/resume cycle. > > Cc: Roger Quadros <rogerq@ti.com> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > --- GPIO IP has two levels of controls for wakeups and you are just removing the SYSCFG wakeup and relying on the IRQ line wakeup. I like usage of "IRQCHIP_MASK_ON_SUSPEND" but please be acreful this change which might break older OMAPs. > drivers/gpio/gpio-omap.c | 42 ++---------------------------------------- > 1 file changed, 2 insertions(+), 40 deletions(-) > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index 551dfa9..b98ede7 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -611,51 +611,12 @@ static inline void omap_set_gpio_irqenable(struct gpio_bank *bank, > omap_disable_gpio_irqbank(bank, BIT(offset)); > } > > -/* > - * Note that ENAWAKEUP needs to be enabled in GPIO_SYSCONFIG register. > - * 1510 does not seem to have a wake-up register. If JTAG is connected > - * to the target, system will wake up always on GPIO events. While > - * system is running all registered GPIO interrupts need to have wake-up > - * enabled. When system is suspended, only selected GPIO interrupts need > - * to have wake-up enabled. > - */ > -static int omap_set_gpio_wakeup(struct gpio_bank *bank, unsigned offset, > - int enable) > -{ > - u32 gpio_bit = BIT(offset); > - unsigned long flags; > - > - if (bank->non_wakeup_gpios & gpio_bit) { > - dev_err(bank->chip.parent, > - "Unable to modify wakeup on non-wakeup GPIO%d\n", > - offset); > - return -EINVAL; > - } > - > - raw_spin_lock_irqsave(&bank->lock, flags); > - if (enable) > - bank->context.wake_en |= gpio_bit; > - else > - bank->context.wake_en &= ~gpio_bit; > - > - writel_relaxed(bank->context.wake_en, bank->base + bank->regs->wkup_en); > - raw_spin_unlock_irqrestore(&bank->lock, flags); > - > - return 0; > -} > - > /* Use disable_irq_wake() and enable_irq_wake() functions from drivers */ > static int omap_gpio_wake_enable(struct irq_data *d, unsigned int enable) > { > struct gpio_bank *bank = omap_irq_data_get_bank(d); > - unsigned offset = d->hwirq; > - int ret; > > - ret = omap_set_gpio_wakeup(bank, offset, enable); > - if (!ret) > - ret = irq_set_irq_wake(bank->irq, enable); > - > - return ret; > + return irq_set_irq_wake(bank->irq, enable); > } > > static int omap_gpio_request(struct gpio_chip *chip, unsigned offset) > @@ -1187,6 +1148,7 @@ static int omap_gpio_probe(struct platform_device *pdev) > irqc->irq_bus_lock = omap_gpio_irq_bus_lock, > irqc->irq_bus_sync_unlock = gpio_irq_bus_sync_unlock, > irqc->name = dev_name(&pdev->dev); > + irqc->flags = IRQCHIP_MASK_ON_SUSPEND; > > bank->irq = platform_get_irq(pdev, 0); > if (bank->irq <= 0) { > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/12/2016 07:44 PM, santosh shilimkar wrote: > On 4/12/2016 3:52 AM, Grygorii Strashko wrote: >> Now GPIO IRQ loss is observed on dra7-evm after suspend/resume cycle >> in the following case: >> extcon_usb1(id_irq) -> pcf8575.gpio1 -> omapgpio6.gpio11 -> gic >> >> the extcon_usb1 is wake up source and it enables IRQ wake up for >> id_irq by calling enable/disable_irq_wake() during suspend/resume >> which, in turn, causes execution of omap_gpio_wake_enable(). And >> omap_gpio_wake_enable() will set/clear corresponding bit in >> GPIO_IRQWAKEN_x register. >> >> omapgpio6 configuration after boot - wakeup is enabled for GPIO IRQs >> by default from omap_gpio_irq_type: >> GPIO_IRQSTATUS_SET_0 | 0x00000400 >> GPIO_IRQSTATUS_CLR_0 | 0x00000400 >> GPIO_IRQWAKEN_0 | 0x00000400 >> GPIO_RISINGDETECT | 0x00000000 >> GPIO_FALLINGDETECT | 0x00000400 >> >> omapgpio6 configuration after after suspend/resume cycle: >> GPIO_IRQSTATUS_SET_0 | 0x00000400 >> GPIO_IRQSTATUS_CLR_0 | 0x00000400 >> GPIO_IRQWAKEN_0 | 0x00000000 <--- >> GPIO_RISINGDETECT | 0x00000000 >> GPIO_FALLINGDETECT | 0x00000400 >> >> As result, system will start to lose interrupts from pcf8575 GPIO >> expander, because when OMAP GPIO IP is in smart-idle wakeup mode, there >> is no guarantee that transition(s) on input non wake up GPIO pin will >> trigger asynchronous wake-up request to PRCM and then IRQ generation. >> IRQ will be generated when GPIO is in active mode - for example, some >> time after accessing GPIO bank registers IRQs will be generated >> normally, but issue will happen again once PRCM will put GPIO in low >> power smart-idle wakeup mode. >> >> Note 1. Issue is not reproduced if debounce clk is enabled for GPIO >> bank. >> >> Note 2. Issue hardly reproducible if GPIO pins group contains both >> wakeup/non-wakeup gpios - for example, it will be hard to reproduce >> issue with pin2 if GPIO_IRQWAKEN_0=0x1 GPIO_IRQSTATUS_SET_0=0x3 >> GPIO_FALLINGDETECT = 0x3 (TRM "Power Saving by Grouping the Edge/Level >> Detection"). >> >> Note 3. There nothing common bitween System wake up and OMAP GPIO bank >> IP wake up logic - the last one defines how the GPIO bank ON-IDLE-ON >> transition will happen inside SoC under control of PRCM. >> >> Hence, fix the problem by removing omap_set_gpio_wakeup() function >> completely and so keeping always in sync GPIO IRQ mask/unmask >> (IRQSTATUS_SET) and wake up enable (GPIO_IRQWAKEN) bits; ^^^ only omap_set_gpio_wakeup() is removed, which shouldn't touch GPIO_IRQWAKEN register (note 3). IRQchip .irq_set_irq_wake() in our case should just mark irq parent as System wake up source, so it will be kept unmasked during System suspend. GPIO Irq by itself will be marked by IRQ Core. All other GPIO IRQs must be masked during System suspend (and it is done gracefully by using IRQCHIP_MASK_ON_SUSPEND:). >> and adding >> IRQCHIP_MASK_ON_SUSPEND flag in OMAP GPIO irqchip. That way non wakeup >> GPIO IRQs will be properly masked/unmask by IRQ PM core during >> suspend/resume cycle. >> >> Cc: Roger Quadros <rogerq@ti.com> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >> --- > GPIO IP has two levels of controls for wakeups and you are > just removing the SYSCFG wakeup and relying on the IRQ > line wakeup. No. I don't remove SYSCFG wakeup. SYSCFG.ENAWAKEUP will be configured by OMAP hwmod and this patch do not change that. Also, GPIO_IRQWAKEN will also be configured properly - It alway configured from omap_set_gpio_trigger(). In addition: omap_gpio_mask_irq() -> omap_set_gpio_trigger(IRQ_TYPE_NONE) : ---> irq masked, GPIO_IRQWAKEN bit cleared, irq type is IRQ_TYPE_NONE omap_gpio_unmask_irq() -> omap_set_gpio_trigger(IRQ_TYPE_xxx) : ---> irq unmasked, GPIO_IRQWAKEN bit is set, irq type is IRQ_TYPE_xxx > line wakeup. I like usage of "IRQCHIP_MASK_ON_SUSPEND" but > please be acreful this change which might break older OMAPs. I expect no issues (only if some platforms expect to wake up from gpio irq, but do not configure this irq as wakeup irq). ^ and this will be a bug which need to be fixed. > >> drivers/gpio/gpio-omap.c | 42 >> ++---------------------------------------- >> 1 file changed, 2 insertions(+), 40 deletions(-) >> >> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c >> index 551dfa9..b98ede7 100644 >> --- a/drivers/gpio/gpio-omap.c >> +++ b/drivers/gpio/gpio-omap.c >> @@ -611,51 +611,12 @@ static inline void >> omap_set_gpio_irqenable(struct gpio_bank *bank, >> omap_disable_gpio_irqbank(bank, BIT(offset)); >> } >> >> -/* >> - * Note that ENAWAKEUP needs to be enabled in GPIO_SYSCONFIG register. >> - * 1510 does not seem to have a wake-up register. If JTAG is connected >> - * to the target, system will wake up always on GPIO events. While >> - * system is running all registered GPIO interrupts need to have wake-up >> - * enabled. When system is suspended, only selected GPIO interrupts need >> - * to have wake-up enabled. >> - */ >> -static int omap_set_gpio_wakeup(struct gpio_bank *bank, unsigned offset, >> - int enable) >> -{ >> - u32 gpio_bit = BIT(offset); >> - unsigned long flags; >> - >> - if (bank->non_wakeup_gpios & gpio_bit) { >> - dev_err(bank->chip.parent, >> - "Unable to modify wakeup on non-wakeup GPIO%d\n", >> - offset); >> - return -EINVAL; >> - } >> - >> - raw_spin_lock_irqsave(&bank->lock, flags); >> - if (enable) >> - bank->context.wake_en |= gpio_bit; >> - else >> - bank->context.wake_en &= ~gpio_bit; >> - >> - writel_relaxed(bank->context.wake_en, bank->base + >> bank->regs->wkup_en); >> - raw_spin_unlock_irqrestore(&bank->lock, flags); >> - >> - return 0; >> -} >> - >> /* Use disable_irq_wake() and enable_irq_wake() functions from >> drivers */ >> static int omap_gpio_wake_enable(struct irq_data *d, unsigned int >> enable) >> { >> struct gpio_bank *bank = omap_irq_data_get_bank(d); >> - unsigned offset = d->hwirq; >> - int ret; >> >> - ret = omap_set_gpio_wakeup(bank, offset, enable); >> - if (!ret) >> - ret = irq_set_irq_wake(bank->irq, enable); >> - >> - return ret; >> + return irq_set_irq_wake(bank->irq, enable); >> } >> >> static int omap_gpio_request(struct gpio_chip *chip, unsigned offset) >> @@ -1187,6 +1148,7 @@ static int omap_gpio_probe(struct >> platform_device *pdev) >> irqc->irq_bus_lock = omap_gpio_irq_bus_lock, >> irqc->irq_bus_sync_unlock = gpio_irq_bus_sync_unlock, >> irqc->name = dev_name(&pdev->dev); >> + irqc->flags = IRQCHIP_MASK_ON_SUSPEND; >> >> bank->irq = platform_get_irq(pdev, 0); >> if (bank->irq <= 0) { >>
* Grygorii Strashko <grygorii.strashko@ti.com> [160412 11:11]: > On 04/12/2016 07:44 PM, santosh shilimkar wrote: > I expect no issues (only if some platforms expect to wake up from > gpio irq, but do not configure this irq as wakeup irq). > ^ and this will be a bug which need to be fixed. This seems like a safe assumption to me. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 12, 2016 at 12:52 PM, Grygorii Strashko <grygorii.strashko@ti.com> wrote: > Now GPIO IRQ loss is observed on dra7-evm after suspend/resume cycle (...) > Cc: Roger Quadros <rogerq@ti.com> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> Can I get some explicit ACK / Tested-by tags for this patch? Is it a serious regression that will need to go in as a fix and tagged for stable? Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/15/2016 11:32 AM, Linus Walleij wrote: > On Tue, Apr 12, 2016 at 12:52 PM, Grygorii Strashko > <grygorii.strashko@ti.com> wrote: > >> Now GPIO IRQ loss is observed on dra7-evm after suspend/resume cycle > (...) >> Cc: Roger Quadros <rogerq@ti.com> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > > Can I get some explicit ACK / Tested-by tags for this patch? Roger's promised to test it once suspend regression will be fixed for dra7-evm, probably next rc. > > Is it a serious regression that will need to go in as a fix and > tagged for stable? > This issue is here since 2012, so I think it's not very critical - It seems bits combination which causing the issue is rare. Regarding stable: 4.4 - good to have, simple merge conflict 4.1 - some merge resolution is required older kernel - it will be hard to backport it due to significant changes in omap gpio driver Santosh, Tony, do you want me to perform any additional actions regarding this patch?
* Grygorii Strashko <grygorii.strashko@ti.com> [160415 02:27]: > On 04/15/2016 11:32 AM, Linus Walleij wrote: > > On Tue, Apr 12, 2016 at 12:52 PM, Grygorii Strashko > > <grygorii.strashko@ti.com> wrote: > > > >> Now GPIO IRQ loss is observed on dra7-evm after suspend/resume cycle > > (...) > >> Cc: Roger Quadros <rogerq@ti.com> > >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> > > > > Can I get some explicit ACK / Tested-by tags for this patch? > > Roger's promised to test it once suspend regression will be fixed > for dra7-evm, probably next rc. > > > > > Is it a serious regression that will need to go in as a fix and > > tagged for stable? > > > > This issue is here since 2012, so I think it's not very critical - > It seems bits combination which causing the issue is rare. > > Regarding stable: > 4.4 - good to have, simple merge conflict > 4.1 - some merge resolution is required > older kernel - it will be hard to backport it due to significant > changes in omap gpio driver > > Santosh, Tony, do you want me to perform any additional actions regarding this patch? Well maybe update the comments regarding non-interrupt GPIO lines and wake-up events. Santosh may have other comments. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/15/2016 2:26 AM, Grygorii Strashko wrote: > On 04/15/2016 11:32 AM, Linus Walleij wrote: >> On Tue, Apr 12, 2016 at 12:52 PM, Grygorii Strashko >> <grygorii.strashko@ti.com> wrote: >> >>> Now GPIO IRQ loss is observed on dra7-evm after suspend/resume cycle >> (...) >>> Cc: Roger Quadros <rogerq@ti.com> >>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> >> >> Can I get some explicit ACK / Tested-by tags for this patch? > > Roger's promised to test it once suspend regression will be fixed > for dra7-evm, probably next rc. > >> >> Is it a serious regression that will need to go in as a fix and >> tagged for stable? >> > > This issue is here since 2012, so I think it's not very critical - > It seems bits combination which causing the issue is rare. > > Regarding stable: > 4.4 - good to have, simple merge conflict > 4.1 - some merge resolution is required > older kernel - it will be hard to backport it due to significant > changes in omap gpio driver > > Santosh, Tony, do you want me to perform any additional actions regarding this patch? > This patch should be run across family of SOCs to make sure wakeup works on all of those if not done already -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* santosh shilimkar <santosh.shilimkar@oracle.com> [160415 08:22]: > On 4/15/2016 2:26 AM, Grygorii Strashko wrote: > > > >Santosh, Tony, do you want me to perform any additional actions regarding this patch? > > > This patch should be run across family of SOCs to make > sure wakeup works on all of those if not done already Also, I'm not sure if we can just drop this code in question. After this patch, what function updates the GPIO wkup_en registers depending on enable_irq_wake()/disable_irq_wake()? Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/15/2016 09:54 PM, Tony Lindgren wrote: > * santosh shilimkar <santosh.shilimkar@oracle.com> [160415 08:22]: >> On 4/15/2016 2:26 AM, Grygorii Strashko wrote: >>> >>> Santosh, Tony, do you want me to perform any additional actions regarding this patch? >>> >> This patch should be run across family of SOCs to make >> sure wakeup works on all of those if not done already > > Also, I'm not sure if we can just drop this code in question. > > After this patch, what function updates the GPIO wkup_en registers > depending on enable_irq_wake()/disable_irq_wake()? > The main purpose of this patch is to *not* modify GPIO wkup_en registers depending of enable_irq_wake()/disable_irq_wake() :), instead all non wake up IRQs should be masked during suspend. The GPIO wkup_en registers should be always in sync with GPIO irq_en when GPIO IP is in smart-idle wakeup mode. And this is done now from omap_gpio_unmask_irq/omap_gpio_mask_irq(). See also [1]. In general, it is more or less similar to GIC + wakeupgen: - during normal work (including cpuidle) GIC irq_en and Wakeupgen wkup_en should be in sync always - during suspend - only IRQs, marked as wake up sources, should be left unmasked. Also, I've found old thread [2] where Santosh proposed to use IRQCHIP_MASK_ON_SUSPEND. And it was not possible, at that time, but now IRQCHIP_MASK_ON_SUSPEND can be used :), because OMAP GPIO driver was switched to use generic irq handler instead of chained, so now OMAP GPIO irqs are properly handled by IRQ PM core. [chained irqs (and chained irq handles) are not disabled during suspend/resume and they are not maintained by IRQ PM core as result they can trigger way too early on resume when OMAP GPIO is not ready/powered.] I've tested it on: am57x-evm, am437x-idk-evm, omap4-panda [1] https://lkml.org/lkml/2016/4/12/676 [2] https://lkml.org/lkml/2012/8/26/1 https://groups.google.com/forum/#!msg/linux.kernel/iXJ5Y568B3Q/hZ39bSlcs0kJ
* Grygorii Strashko <grygorii.strashko@ti.com> [160418 08:59]: > On 04/15/2016 09:54 PM, Tony Lindgren wrote: > > * santosh shilimkar <santosh.shilimkar@oracle.com> [160415 08:22]: > >> On 4/15/2016 2:26 AM, Grygorii Strashko wrote: > >>> > >>> Santosh, Tony, do you want me to perform any additional actions regarding this patch? > >>> > >> This patch should be run across family of SOCs to make > >> sure wakeup works on all of those if not done already > > > > Also, I'm not sure if we can just drop this code in question. > > > > After this patch, what function updates the GPIO wkup_en registers > > depending on enable_irq_wake()/disable_irq_wake()? > > > > The main purpose of this patch is to *not* modify GPIO wkup_en registers > depending of enable_irq_wake()/disable_irq_wake() :), instead all > non wake up IRQs should be masked during suspend. OK that makes sense. > The GPIO wkup_en registers should be always in sync with GPIO irq_en when > GPIO IP is in smart-idle wakeup mode. And this is done now from > omap_gpio_unmask_irq/omap_gpio_mask_irq(). See also [1]. > > In general, it is more or less similar to GIC + wakeupgen: > - during normal work (including cpuidle) GIC irq_en and Wakeupgen wkup_en > should be in sync always > - during suspend - only IRQs, marked as wake up sources, should be left > unmasked. > > Also, I've found old thread [2] where Santosh proposed to use IRQCHIP_MASK_ON_SUSPEND. > And it was not possible, at that time, but now IRQCHIP_MASK_ON_SUSPEND can be used :), > because OMAP GPIO driver was switched to use generic irq handler instead of chained, so > now OMAP GPIO irqs are properly handled by IRQ PM core. > [chained irqs (and chained irq handles) are not disabled during suspend/resume and they are > not maintained by IRQ PM core as result they can trigger way too early on resume when > OMAP GPIO is not ready/powered.] OK. For my tests this patch does not change anything. I noticed however that we still have some additional bug somewhere where GPIO wake up events work fine for omap3 PM runtime, but are flakey for suspend. > I've tested it on: am57x-evm, am437x-idk-evm, omap4-panda OK thanks! Based on my tests and the above: Acked-by: Tony Lindgren <tony@atomide.com> Regards, Tony > [1] https://lkml.org/lkml/2016/4/12/676 > [2] https://lkml.org/lkml/2012/8/26/1 > https://groups.google.com/forum/#!msg/linux.kernel/iXJ5Y568B3Q/hZ39bSlcs0kJ -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/18/2016 4:36 PM, Tony Lindgren wrote: > * Grygorii Strashko <grygorii.strashko@ti.com> [160418 08:59]: >> On 04/15/2016 09:54 PM, Tony Lindgren wrote: >>> * santosh shilimkar <santosh.shilimkar@oracle.com> [160415 08:22]: >>>> On 4/15/2016 2:26 AM, Grygorii Strashko wrote: >>>>> >>>>> Santosh, Tony, do you want me to perform any additional actions regarding this patch? >>>>> >>>> This patch should be run across family of SOCs to make >>>> sure wakeup works on all of those if not done already >>> >>> Also, I'm not sure if we can just drop this code in question. >>> >>> After this patch, what function updates the GPIO wkup_en registers >>> depending on enable_irq_wake()/disable_irq_wake()? >>> >> >> The main purpose of this patch is to *not* modify GPIO wkup_en registers >> depending of enable_irq_wake()/disable_irq_wake() :), instead all >> non wake up IRQs should be masked during suspend. > > OK that makes sense. > >> The GPIO wkup_en registers should be always in sync with GPIO irq_en when >> GPIO IP is in smart-idle wakeup mode. And this is done now from >> omap_gpio_unmask_irq/omap_gpio_mask_irq(). See also [1]. >> >> In general, it is more or less similar to GIC + wakeupgen: >> - during normal work (including cpuidle) GIC irq_en and Wakeupgen wkup_en >> should be in sync always >> - during suspend - only IRQs, marked as wake up sources, should be left >> unmasked. >> >> Also, I've found old thread [2] where Santosh proposed to use IRQCHIP_MASK_ON_SUSPEND. >> And it was not possible, at that time, but now IRQCHIP_MASK_ON_SUSPEND can be used :), >> because OMAP GPIO driver was switched to use generic irq handler instead of chained, so >> now OMAP GPIO irqs are properly handled by IRQ PM core. >> [chained irqs (and chained irq handles) are not disabled during suspend/resume and they are >> not maintained by IRQ PM core as result they can trigger way too early on resume when >> OMAP GPIO is not ready/powered.] > > OK. For my tests this patch does not change anything. I noticed however > that we still have some additional bug somewhere where GPIO wake up > events work fine for omap3 PM runtime, but are flakey for suspend. > >> I've tested it on: am57x-evm, am437x-idk-evm, omap4-panda > > OK thanks! Based on my tests and the above: > > Acked-by: Tony Lindgren <tony@atomide.com> > If all works then consider my ack as well :-) -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 12, 2016 at 12:52 PM, Grygorii Strashko <grygorii.strashko@ti.com> wrote: > Now GPIO IRQ loss is observed on dra7-evm after suspend/resume cycle > in the following case: > extcon_usb1(id_irq) -> pcf8575.gpio1 -> omapgpio6.gpio11 -> gic > > the extcon_usb1 is wake up source and it enables IRQ wake up for > id_irq by calling enable/disable_irq_wake() during suspend/resume > which, in turn, causes execution of omap_gpio_wake_enable(). And > omap_gpio_wake_enable() will set/clear corresponding bit in > GPIO_IRQWAKEN_x register. > > omapgpio6 configuration after boot - wakeup is enabled for GPIO IRQs > by default from omap_gpio_irq_type: > GPIO_IRQSTATUS_SET_0 | 0x00000400 > GPIO_IRQSTATUS_CLR_0 | 0x00000400 > GPIO_IRQWAKEN_0 | 0x00000400 > GPIO_RISINGDETECT | 0x00000000 > GPIO_FALLINGDETECT | 0x00000400 > > omapgpio6 configuration after after suspend/resume cycle: > GPIO_IRQSTATUS_SET_0 | 0x00000400 > GPIO_IRQSTATUS_CLR_0 | 0x00000400 > GPIO_IRQWAKEN_0 | 0x00000000 <--- > GPIO_RISINGDETECT | 0x00000000 > GPIO_FALLINGDETECT | 0x00000400 > > As result, system will start to lose interrupts from pcf8575 GPIO > expander, because when OMAP GPIO IP is in smart-idle wakeup mode, there > is no guarantee that transition(s) on input non wake up GPIO pin will > trigger asynchronous wake-up request to PRCM and then IRQ generation. > IRQ will be generated when GPIO is in active mode - for example, some > time after accessing GPIO bank registers IRQs will be generated > normally, but issue will happen again once PRCM will put GPIO in low > power smart-idle wakeup mode. > > Note 1. Issue is not reproduced if debounce clk is enabled for GPIO > bank. > > Note 2. Issue hardly reproducible if GPIO pins group contains both > wakeup/non-wakeup gpios - for example, it will be hard to reproduce > issue with pin2 if GPIO_IRQWAKEN_0=0x1 GPIO_IRQSTATUS_SET_0=0x3 > GPIO_FALLINGDETECT = 0x3 (TRM "Power Saving by Grouping the Edge/Level > Detection"). > > Note 3. There nothing common bitween System wake up and OMAP GPIO bank > IP wake up logic - the last one defines how the GPIO bank ON-IDLE-ON > transition will happen inside SoC under control of PRCM. > > Hence, fix the problem by removing omap_set_gpio_wakeup() function > completely and so keeping always in sync GPIO IRQ mask/unmask > (IRQSTATUS_SET) and wake up enable (GPIO_IRQWAKEN) bits; and adding > IRQCHIP_MASK_ON_SUSPEND flag in OMAP GPIO irqchip. That way non wakeup > GPIO IRQs will be properly masked/unmask by IRQ PM core during > suspend/resume cycle. > > Cc: Roger Quadros <rogerq@ti.com> > Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> Patch applied with the arrived ACKs Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c index 551dfa9..b98ede7 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -611,51 +611,12 @@ static inline void omap_set_gpio_irqenable(struct gpio_bank *bank, omap_disable_gpio_irqbank(bank, BIT(offset)); } -/* - * Note that ENAWAKEUP needs to be enabled in GPIO_SYSCONFIG register. - * 1510 does not seem to have a wake-up register. If JTAG is connected - * to the target, system will wake up always on GPIO events. While - * system is running all registered GPIO interrupts need to have wake-up - * enabled. When system is suspended, only selected GPIO interrupts need - * to have wake-up enabled. - */ -static int omap_set_gpio_wakeup(struct gpio_bank *bank, unsigned offset, - int enable) -{ - u32 gpio_bit = BIT(offset); - unsigned long flags; - - if (bank->non_wakeup_gpios & gpio_bit) { - dev_err(bank->chip.parent, - "Unable to modify wakeup on non-wakeup GPIO%d\n", - offset); - return -EINVAL; - } - - raw_spin_lock_irqsave(&bank->lock, flags); - if (enable) - bank->context.wake_en |= gpio_bit; - else - bank->context.wake_en &= ~gpio_bit; - - writel_relaxed(bank->context.wake_en, bank->base + bank->regs->wkup_en); - raw_spin_unlock_irqrestore(&bank->lock, flags); - - return 0; -} - /* Use disable_irq_wake() and enable_irq_wake() functions from drivers */ static int omap_gpio_wake_enable(struct irq_data *d, unsigned int enable) { struct gpio_bank *bank = omap_irq_data_get_bank(d); - unsigned offset = d->hwirq; - int ret; - ret = omap_set_gpio_wakeup(bank, offset, enable); - if (!ret) - ret = irq_set_irq_wake(bank->irq, enable); - - return ret; + return irq_set_irq_wake(bank->irq, enable); } static int omap_gpio_request(struct gpio_chip *chip, unsigned offset) @@ -1187,6 +1148,7 @@ static int omap_gpio_probe(struct platform_device *pdev) irqc->irq_bus_lock = omap_gpio_irq_bus_lock, irqc->irq_bus_sync_unlock = gpio_irq_bus_sync_unlock, irqc->name = dev_name(&pdev->dev); + irqc->flags = IRQCHIP_MASK_ON_SUSPEND; bank->irq = platform_get_irq(pdev, 0); if (bank->irq <= 0) {
Now GPIO IRQ loss is observed on dra7-evm after suspend/resume cycle in the following case: extcon_usb1(id_irq) -> pcf8575.gpio1 -> omapgpio6.gpio11 -> gic the extcon_usb1 is wake up source and it enables IRQ wake up for id_irq by calling enable/disable_irq_wake() during suspend/resume which, in turn, causes execution of omap_gpio_wake_enable(). And omap_gpio_wake_enable() will set/clear corresponding bit in GPIO_IRQWAKEN_x register. omapgpio6 configuration after boot - wakeup is enabled for GPIO IRQs by default from omap_gpio_irq_type: GPIO_IRQSTATUS_SET_0 | 0x00000400 GPIO_IRQSTATUS_CLR_0 | 0x00000400 GPIO_IRQWAKEN_0 | 0x00000400 GPIO_RISINGDETECT | 0x00000000 GPIO_FALLINGDETECT | 0x00000400 omapgpio6 configuration after after suspend/resume cycle: GPIO_IRQSTATUS_SET_0 | 0x00000400 GPIO_IRQSTATUS_CLR_0 | 0x00000400 GPIO_IRQWAKEN_0 | 0x00000000 <--- GPIO_RISINGDETECT | 0x00000000 GPIO_FALLINGDETECT | 0x00000400 As result, system will start to lose interrupts from pcf8575 GPIO expander, because when OMAP GPIO IP is in smart-idle wakeup mode, there is no guarantee that transition(s) on input non wake up GPIO pin will trigger asynchronous wake-up request to PRCM and then IRQ generation. IRQ will be generated when GPIO is in active mode - for example, some time after accessing GPIO bank registers IRQs will be generated normally, but issue will happen again once PRCM will put GPIO in low power smart-idle wakeup mode. Note 1. Issue is not reproduced if debounce clk is enabled for GPIO bank. Note 2. Issue hardly reproducible if GPIO pins group contains both wakeup/non-wakeup gpios - for example, it will be hard to reproduce issue with pin2 if GPIO_IRQWAKEN_0=0x1 GPIO_IRQSTATUS_SET_0=0x3 GPIO_FALLINGDETECT = 0x3 (TRM "Power Saving by Grouping the Edge/Level Detection"). Note 3. There nothing common bitween System wake up and OMAP GPIO bank IP wake up logic - the last one defines how the GPIO bank ON-IDLE-ON transition will happen inside SoC under control of PRCM. Hence, fix the problem by removing omap_set_gpio_wakeup() function completely and so keeping always in sync GPIO IRQ mask/unmask (IRQSTATUS_SET) and wake up enable (GPIO_IRQWAKEN) bits; and adding IRQCHIP_MASK_ON_SUSPEND flag in OMAP GPIO irqchip. That way non wakeup GPIO IRQs will be properly masked/unmask by IRQ PM core during suspend/resume cycle. Cc: Roger Quadros <rogerq@ti.com> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com> --- drivers/gpio/gpio-omap.c | 42 ++---------------------------------------- 1 file changed, 2 insertions(+), 40 deletions(-)