| Message ID | 20120906170245.10e97bdd@notabene.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Sep 6, 2012 at 12:32 PM, NeilBrown <neilb@suse.de> wrote: > On Thu, 6 Sep 2012 11:18:09 +0530 "Shilimkar, Santosh" > <santosh.shilimkar@ti.com> wrote: > >> On Thu, Sep 6, 2012 at 8:35 AM, NeilBrown <neilb@suse.de> wrote: >> > On Mon, 3 Sep 2012 22:59:06 -0700 "Shilimkar, Santosh" >> > <santosh.shilimkar@ti.com> wrote: > >> >> After thinking bit more on this, the problem seems to be coming >> >> mainly because the gpio device is runtime suspended bit early than >> >> it should be. Similar issue seen with i2c driver as well. The i2c issue >> >> was discussed with Rafael at LPC last week. The idea is to move >> >> the pm_runtime_enable/disable() calls entirely up to the >> >> _late/_early stage of device suspend/resume. >> >> Will update this thread once I have further update. >> > >> > This won't be late enough. IRQCHIP_MASK_ON_SUSPEND takes effect after all >> > the _late callbacks have been called. >> > I, too, spoke to Rafael about this in San Diego. He seemed to agree with me >> > that the interrupt needs to be masked in the ->suspend callback. any later >> > is too late. >> > >> Thanks for information about your discussion. Will wait for the patch then. >> >> Regards >> santosh > > I already sent a patch - that is what started this thread :-) > > I include it below. > You said "The patch doesn't seems to be correct" but didn't expand on why. > Do you still think it is not correct? I wouldn't be surprised if there is > some case that it doesn't handle quite right, but it seems right to me. > Sorry I though you were talking about moving the "Checking wakeup interrupts" bit early as discussed on the follow up of alternate suggested patch to make use of IRQCHIP_MASK_ON_SUSPEND. I think we need to fix the issue seen with ' IRQCHIP_MASK_ON_SUSPEND' patch. That is at least the expected way to manage suspend and wakeup irq masks for a IRQCHIP. Regards Santosh -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 6 Sep 2012 12:57:46 +0530 "Shilimkar, Santosh" <santosh.shilimkar@ti.com> wrote: > On Thu, Sep 6, 2012 at 12:32 PM, NeilBrown <neilb@suse.de> wrote: > > On Thu, 6 Sep 2012 11:18:09 +0530 "Shilimkar, Santosh" > > <santosh.shilimkar@ti.com> wrote: > > > >> On Thu, Sep 6, 2012 at 8:35 AM, NeilBrown <neilb@suse.de> wrote: > >> > On Mon, 3 Sep 2012 22:59:06 -0700 "Shilimkar, Santosh" > >> > <santosh.shilimkar@ti.com> wrote: > > > >> >> After thinking bit more on this, the problem seems to be coming > >> >> mainly because the gpio device is runtime suspended bit early than > >> >> it should be. Similar issue seen with i2c driver as well. The i2c issue > >> >> was discussed with Rafael at LPC last week. The idea is to move > >> >> the pm_runtime_enable/disable() calls entirely up to the > >> >> _late/_early stage of device suspend/resume. > >> >> Will update this thread once I have further update. > >> > > >> > This won't be late enough. IRQCHIP_MASK_ON_SUSPEND takes effect after all > >> > the _late callbacks have been called. > >> > I, too, spoke to Rafael about this in San Diego. He seemed to agree with me > >> > that the interrupt needs to be masked in the ->suspend callback. any later > >> > is too late. > >> > > >> Thanks for information about your discussion. Will wait for the patch then. > >> > >> Regards > >> santosh > > > > I already sent a patch - that is what started this thread :-) > > > > I include it below. > > You said "The patch doesn't seems to be correct" but didn't expand on why. > > Do you still think it is not correct? I wouldn't be surprised if there is > > some case that it doesn't handle quite right, but it seems right to me. > > > Sorry I though you were talking about moving the "Checking wakeup interrupts" > bit early as discussed on the follow up of alternate suggested patch to make > use of IRQCHIP_MASK_ON_SUSPEND. > > I think we need to fix the issue seen with ' IRQCHIP_MASK_ON_SUSPEND' > patch. That is at least the expected way to manage suspend and wakeup > irq masks for a IRQCHIP. That is what I thought at first too. But when talking to Rafael he said that IRQCHIP_MASK_ON_SUSPEND was intended mainly for clock interrupts. For other less fundamental interrupts, doing the mask/unmask in suspend/resume callbacks is sufficient and simpler... and actually works. IRQCHIP_MASK_ON_SUSPEND is currently used by precisely two drivers: arch/arm/mach-omap2/omap-wakeupgen.c and drivers/mfd/pm8xxx-irq.c which suggests that it isn't widely used and quite possibly doesn't actually work in general. The pm8xxx-irq doesn't seem to do runtime pm, so maybe it manages to work for that reason. The omap-wakeupgen code is beyond my current understanding, but it seems like it might be the sort of special case that IRQCHIP_MASK_ON_SUSPEND is intended for... Maybe we need to start a new thread and pester Rafael or Thomas Gleixner to either explain what is intended for this case, or to fix IRQCHIP_MASK_ON_SUSPEND so that it can be used in general. NeilBrown
On Thu, Sep 6, 2012 at 1:21 PM, NeilBrown <neilb@suse.de> wrote: > On Thu, 6 Sep 2012 12:57:46 +0530 "Shilimkar, Santosh" > <santosh.shilimkar@ti.com> wrote: > >> On Thu, Sep 6, 2012 at 12:32 PM, NeilBrown <neilb@suse.de> wrote: >> > On Thu, 6 Sep 2012 11:18:09 +0530 "Shilimkar, Santosh" >> > <santosh.shilimkar@ti.com> wrote: >> > >> >> On Thu, Sep 6, 2012 at 8:35 AM, NeilBrown <neilb@suse.de> wrote: >> >> > On Mon, 3 Sep 2012 22:59:06 -0700 "Shilimkar, Santosh" >> >> > <santosh.shilimkar@ti.com> wrote: >> > >> >> >> After thinking bit more on this, the problem seems to be coming >> >> >> mainly because the gpio device is runtime suspended bit early than >> >> >> it should be. Similar issue seen with i2c driver as well. The i2c issue >> >> >> was discussed with Rafael at LPC last week. The idea is to move >> >> >> the pm_runtime_enable/disable() calls entirely up to the >> >> >> _late/_early stage of device suspend/resume. >> >> >> Will update this thread once I have further update. >> >> > >> >> > This won't be late enough. IRQCHIP_MASK_ON_SUSPEND takes effect after all >> >> > the _late callbacks have been called. >> >> > I, too, spoke to Rafael about this in San Diego. He seemed to agree with me >> >> > that the interrupt needs to be masked in the ->suspend callback. any later >> >> > is too late. >> >> > >> >> Thanks for information about your discussion. Will wait for the patch then. >> >> >> >> Regards >> >> santosh >> > >> > I already sent a patch - that is what started this thread :-) >> > >> > I include it below. >> > You said "The patch doesn't seems to be correct" but didn't expand on why. >> > Do you still think it is not correct? I wouldn't be surprised if there is >> > some case that it doesn't handle quite right, but it seems right to me. >> > >> Sorry I though you were talking about moving the "Checking wakeup interrupts" >> bit early as discussed on the follow up of alternate suggested patch to make >> use of IRQCHIP_MASK_ON_SUSPEND. >> >> I think we need to fix the issue seen with ' IRQCHIP_MASK_ON_SUSPEND' >> patch. That is at least the expected way to manage suspend and wakeup >> irq masks for a IRQCHIP. > > That is what I thought at first too. But when talking to Rafael he said that > IRQCHIP_MASK_ON_SUSPEND was intended mainly for clock interrupts. For other > less fundamental interrupts, doing the mask/unmask in suspend/resume > callbacks is sufficient and simpler... and actually works. > That is not the how I undetand IRQCHIP_MASK_ON_SUSPEND use. I thought it can be used for any IRQ chip for masking or setting wakeup on interrupts lines managed by that chip for suspend. May be I am wrong. > IRQCHIP_MASK_ON_SUSPEND is currently used by precisely two drivers: > > arch/arm/mach-omap2/omap-wakeupgen.c > and > drivers/mfd/pm8xxx-irq.c > > which suggests that it isn't widely used and quite possibly doesn't actually > work in general. I have seen lot more platforms use in downstream kernels. Also seen recently patches on the linux-arm list for couple of platforms. > > Maybe we need to start a new thread and pester Rafael or Thomas Gleixner > to either explain what is intended for this case, or to fix > IRQCHIP_MASK_ON_SUSPEND so that it can be used in general. > Sounds a good idea. Since you already had discussion with Rafael, probably you can describe the issue better. Regards Santosh -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, Sep 06, 2012 at 05:02:45PM +1000, NeilBrown wrote: > On Thu, 6 Sep 2012 11:18:09 +0530 "Shilimkar, Santosh" > <santosh.shilimkar@ti.com> wrote: > > > On Thu, Sep 6, 2012 at 8:35 AM, NeilBrown <neilb@suse.de> wrote: > > > On Mon, 3 Sep 2012 22:59:06 -0700 "Shilimkar, Santosh" > > > <santosh.shilimkar@ti.com> wrote: > > > >> After thinking bit more on this, the problem seems to be coming > > >> mainly because the gpio device is runtime suspended bit early than > > >> it should be. Similar issue seen with i2c driver as well. The i2c issue > > >> was discussed with Rafael at LPC last week. The idea is to move > > >> the pm_runtime_enable/disable() calls entirely up to the > > >> _late/_early stage of device suspend/resume. > > >> Will update this thread once I have further update. > > > > > > This won't be late enough. IRQCHIP_MASK_ON_SUSPEND takes effect after all > > > the _late callbacks have been called. > > > I, too, spoke to Rafael about this in San Diego. He seemed to agree with me > > > that the interrupt needs to be masked in the ->suspend callback. any later > > > is too late. > > > > > Thanks for information about your discussion. Will wait for the patch then. > > > > Regards > > santosh > > I already sent a patch - that is what started this thread :-) > > I include it below. > You said "The patch doesn't seems to be correct" but didn't expand on why. > Do you still think it is not correct? I wouldn't be surprised if there is > some case that it doesn't handle quite right, but it seems right to me. > > Thanks, > NeilBrown > > > From: NeilBrown <neilb@suse.de> > Subject: [PATCH] OMAP GPIO - don't wake from suspend unless requested. > > Current kernel will wake from suspend on an event on any active > GPIO even if enable_irq_wake() wasn't called. > > There are two reasons that the hardware wake-enable bit should be set: > > 1/ while non-suspended the CPU might go into a deep sleep (off_mode) > in which the wake-enable bit is needed for an interrupt to be > recognised. > 2/ while suspended the GPIO interrupt should wake from suspend if and > only if irq_wake as been enabled. > > The code currently doesn't keep these two reasons separate so they get > confused and sometimes the wakeup flags is set incorrectly. > > This patch reverts: > commit 9c4ed9e6c01e7a8bd9079da8267e1f03cb4761fc > gpio/omap: remove suspend/resume callbacks > and > commit 0aa2727399c0b78225021413022c164cb99fbc5e > gpio/omap: remove suspend_wakeup field from struct gpio_bank > > and makes some minor changes so that we have separate flags for "GPIO > should wake from deep idle" and "GPIO should wake from suspend". > > With this patch, the GPIO from my touch screen doesn't wake my device > any more, which is what I want. > > Cc: Kevin Hilman <khilman@ti.com> > Cc: Tony Lindgren <tony@atomide.com> > Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Cousson Benoit <b-cousson@ti.com> > Cc: Grant Likely <grant.likely@secretlab.ca> > Cc: Tarun Kanti DebBarma <tarun.kanti@ti.com> > Cc: Felipe Balbi <balbi@ti.com> > Cc: Govindraj.R <govindraj.raja@ti.com> > > Signed-off-by: NeilBrown <neilb@suse.de> > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index 4fbc208..fdbad70 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -57,6 +57,7 @@ struct gpio_bank { > u16 irq; > int irq_base; > struct irq_domain *domain; > + u32 suspend_wakeup; > u32 non_wakeup_gpios; > u32 enabled_non_wakeup_gpios; > struct gpio_regs context; > @@ -522,11 +523,12 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable) > > spin_lock_irqsave(&bank->lock, flags); > if (enable) > - bank->context.wake_en |= gpio_bit; > + bank->suspend_wakeup |= gpio_bit; > else > - bank->context.wake_en &= ~gpio_bit; > + bank->suspend_wakeup &= ~gpio_bit; > > - __raw_writel(bank->context.wake_en, bank->base + bank->regs->wkup_en); > + if (!bank->loses_context) > + __raw_writel(bank->suspend_wakeup, bank->base + bank->regs->wkup_en); > spin_unlock_irqrestore(&bank->lock, flags); > > return 0; > @@ -1157,6 +1159,51 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev) > #ifdef CONFIG_ARCH_OMAP2PLUS > > #if defined(CONFIG_PM_RUNTIME) > + > +#if defined(CONFIG_PM_SLEEP) > +static int omap_gpio_suspend(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct gpio_bank *bank = platform_get_drvdata(pdev); > + void __iomem *base = bank->base; > + unsigned long flags; > + > + if (!bank->mod_usage || !bank->loses_context) > + return 0; > + > + if (!bank->regs->wkup_en || !bank->context.wake_en) > + return 0; > + > + spin_lock_irqsave(&bank->lock, flags); shouldn't you be using _noirq methods instead ? Then this would become a normal spin_lock()/spin_unlock().
On Thursday 06 September 2012 12:32 PM, NeilBrown wrote: > From: NeilBrown <neilb@suse.de> > Subject: [PATCH] OMAP GPIO - don't wake from suspend unless requested. > > Current kernel will wake from suspend on an event on any active > GPIO even if enable_irq_wake() wasn't called. > > There are two reasons that the hardware wake-enable bit should be set: > > 1/ while non-suspended the CPU might go into a deep sleep (off_mode) > in which the wake-enable bit is needed for an interrupt to be > recognised. > 2/ while suspended the GPIO interrupt should wake from suspend if and > only if irq_wake as been enabled. > > The code currently doesn't keep these two reasons separate so they get > confused and sometimes the wakeup flags is set incorrectly. > > This patch reverts: > commit 9c4ed9e6c01e7a8bd9079da8267e1f03cb4761fc > gpio/omap: remove suspend/resume callbacks > and > commit 0aa2727399c0b78225021413022c164cb99fbc5e > gpio/omap: remove suspend_wakeup field from struct gpio_bank > > and makes some minor changes so that we have separate flags for "GPIO > should wake from deep idle" and "GPIO should wake from suspend". > > With this patch, the GPIO from my touch screen doesn't wake my device > any more, which is what I want. Just gave it a try fixes a similar issue on my omap4 board as well. > > Cc: Kevin Hilman <khilman@ti.com> > Cc: Tony Lindgren <tony@atomide.com> > Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Cousson Benoit <b-cousson@ti.com> > Cc: Grant Likely <grant.likely@secretlab.ca> > Cc: Tarun Kanti DebBarma <tarun.kanti@ti.com> > Cc: Felipe Balbi <balbi@ti.com> > Cc: Govindraj.R <govindraj.raja@ti.com> > > Signed-off-by: NeilBrown <neilb@suse.de> -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Neil, NeilBrown <neilb@suse.de> writes: > On Thu, 6 Sep 2012 11:18:09 +0530 "Shilimkar, Santosh" > <santosh.shilimkar@ti.com> wrote: > >> On Thu, Sep 6, 2012 at 8:35 AM, NeilBrown <neilb@suse.de> wrote: >> > On Mon, 3 Sep 2012 22:59:06 -0700 "Shilimkar, Santosh" >> > <santosh.shilimkar@ti.com> wrote: > >> >> After thinking bit more on this, the problem seems to be coming >> >> mainly because the gpio device is runtime suspended bit early than >> >> it should be. Similar issue seen with i2c driver as well. The i2c issue >> >> was discussed with Rafael at LPC last week. The idea is to move >> >> the pm_runtime_enable/disable() calls entirely up to the >> >> _late/_early stage of device suspend/resume. >> >> Will update this thread once I have further update. >> > >> > This won't be late enough. IRQCHIP_MASK_ON_SUSPEND takes effect after all >> > the _late callbacks have been called. >> > I, too, spoke to Rafael about this in San Diego. He seemed to agree with me >> > that the interrupt needs to be masked in the ->suspend callback. any later >> > is too late. >> > >> Thanks for information about your discussion. Will wait for the patch then. >> >> Regards >> santosh > > I already sent a patch - that is what started this thread :-) > > I include it below. > You said "The patch doesn't seems to be correct" but didn't expand on why. > Do you still think it is not correct? I wouldn't be surprised if there is > some case that it doesn't handle quite right, but it seems right to me. > > Thanks, > NeilBrown > > > From: NeilBrown <neilb@suse.de> > Subject: [PATCH] OMAP GPIO - don't wake from suspend unless requested. > > Current kernel will wake from suspend on an event on any active > GPIO even if enable_irq_wake() wasn't called. > > There are two reasons that the hardware wake-enable bit should be set: > > 1/ while non-suspended the CPU might go into a deep sleep (off_mode) > in which the wake-enable bit is needed for an interrupt to be > recognised. > 2/ while suspended the GPIO interrupt should wake from suspend if and > only if irq_wake as been enabled. > > The code currently doesn't keep these two reasons separate so they get > confused and sometimes the wakeup flags is set incorrectly. > > This patch reverts: > commit 9c4ed9e6c01e7a8bd9079da8267e1f03cb4761fc > gpio/omap: remove suspend/resume callbacks > and > commit 0aa2727399c0b78225021413022c164cb99fbc5e > gpio/omap: remove suspend_wakeup field from struct gpio_bank > > and makes some minor changes so that we have separate flags for "GPIO > should wake from deep idle" and "GPIO should wake from suspend". > > With this patch, the GPIO from my touch screen doesn't wake my device > any more, which is what I want. I think the direction is right here. We never should've separated the handling of idle vs suspend wakeups. However, I have a few questions/doubts below... > Cc: Kevin Hilman <khilman@ti.com> > Cc: Tony Lindgren <tony@atomide.com> > Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> > Cc: Cousson Benoit <b-cousson@ti.com> > Cc: Grant Likely <grant.likely@secretlab.ca> > Cc: Tarun Kanti DebBarma <tarun.kanti@ti.com> > Cc: Felipe Balbi <balbi@ti.com> > Cc: Govindraj.R <govindraj.raja@ti.com> > > Signed-off-by: NeilBrown <neilb@suse.de> > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > index 4fbc208..fdbad70 100644 > --- a/drivers/gpio/gpio-omap.c > +++ b/drivers/gpio/gpio-omap.c > @@ -57,6 +57,7 @@ struct gpio_bank { > u16 irq; > int irq_base; > struct irq_domain *domain; > + u32 suspend_wakeup; > u32 non_wakeup_gpios; > u32 enabled_non_wakeup_gpios; > struct gpio_regs context; > @@ -522,11 +523,12 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable) > > spin_lock_irqsave(&bank->lock, flags); > if (enable) > - bank->context.wake_en |= gpio_bit; > + bank->suspend_wakeup |= gpio_bit; > else > - bank->context.wake_en &= ~gpio_bit; > + bank->suspend_wakeup &= ~gpio_bit; > > - __raw_writel(bank->context.wake_en, bank->base + bank->regs->wkup_en); > + if (!bank->loses_context) > + __raw_writel(bank->suspend_wakeup, bank->base + bank->regs->wkup_en); This doesn't look right for bank 1 (the only one that doesn't lose context.) If I'm not mistaken, this will overrides the idle wake_en settings (from context.wake_en), will disable GPIO IRQ triggering for any of the non IRQ wake-enabled GPIO IRQs in this bank... > spin_unlock_irqrestore(&bank->lock, flags); > > return 0; > @@ -1157,6 +1159,51 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev) > #ifdef CONFIG_ARCH_OMAP2PLUS > > #if defined(CONFIG_PM_RUNTIME) > + > +#if defined(CONFIG_PM_SLEEP) > +static int omap_gpio_suspend(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct gpio_bank *bank = platform_get_drvdata(pdev); > + void __iomem *base = bank->base; > + unsigned long flags; > + > + if (!bank->mod_usage || !bank->loses_context) > + return 0; ... probably, we just need to drop the bank->loses_context check here, so the late writing of bank->suspend_wakeup will happen even for bank 0. Kevin > + > + if (!bank->regs->wkup_en || !bank->context.wake_en) > + return 0; > + spin_lock_irqsave(&bank->lock, flags); > + _gpio_rmw(base, bank->regs->wkup_en, 0xffffffff, 0); > + _gpio_rmw(base, bank->regs->wkup_en, bank->suspend_wakeup, 1); > + spin_unlock_irqrestore(&bank->lock, flags); > + > + return 0; > +} > + > +static int omap_gpio_resume(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct gpio_bank *bank = platform_get_drvdata(pdev); > + void __iomem *base = bank->base; > + unsigned long flags; > + > + if (!bank->mod_usage || !bank->loses_context) > + return 0; > + if (!bank->regs->wkup_en || !bank->context.wake_en) > + return 0; > + spin_lock_irqsave(&bank->lock, flags); > + _gpio_rmw(base, bank->regs->wkup_en, 0xffffffff, 0); > + _gpio_rmw(base, bank->regs->wkup_en, bank->context.wake_en, 1); > + spin_unlock_irqrestore(&bank->lock, flags); > + > + return 0; > +} > +#endif /* CONFIG_PM_SLEEP */ > + > static void omap_gpio_restore_context(struct gpio_bank *bank); > > static int omap_gpio_runtime_suspend(struct device *dev) > @@ -1386,11 +1433,14 @@ static void omap_gpio_restore_context(struct gpio_bank *bank) > } > #endif /* CONFIG_PM_RUNTIME */ > #else > +#define omap_gpio_suspend NULL > +#define omap_gpio_resume NULL > #define omap_gpio_runtime_suspend NULL > #define omap_gpio_runtime_resume NULL > #endif > > static const struct dev_pm_ops gpio_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(omap_gpio_suspend, omap_gpio_resume) > SET_RUNTIME_PM_OPS(omap_gpio_runtime_suspend, omap_gpio_runtime_resume, > NULL) > }; -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Sep 8, 2012 at 3:07 AM, Kevin Hilman <khilman@deeprootsystems.com> wrote: > Hi Neil, > > NeilBrown <neilb@suse.de> writes: > >> On Thu, 6 Sep 2012 11:18:09 +0530 "Shilimkar, Santosh" >> <santosh.shilimkar@ti.com> wrote: >> >>> On Thu, Sep 6, 2012 at 8:35 AM, NeilBrown <neilb@suse.de> wrote: >>> > On Mon, 3 Sep 2012 22:59:06 -0700 "Shilimkar, Santosh" >>> > <santosh.shilimkar@ti.com> wrote: >> >>> >> After thinking bit more on this, the problem seems to be coming >>> >> mainly because the gpio device is runtime suspended bit early than >>> >> it should be. Similar issue seen with i2c driver as well. The i2c issue >>> >> was discussed with Rafael at LPC last week. The idea is to move >>> >> the pm_runtime_enable/disable() calls entirely up to the >>> >> _late/_early stage of device suspend/resume. >>> >> Will update this thread once I have further update. >>> > >>> > This won't be late enough. IRQCHIP_MASK_ON_SUSPEND takes effect after all >>> > the _late callbacks have been called. >>> > I, too, spoke to Rafael about this in San Diego. He seemed to agree with me >>> > that the interrupt needs to be masked in the ->suspend callback. any later >>> > is too late. >>> > >>> Thanks for information about your discussion. Will wait for the patch then. >>> >>> Regards >>> santosh >> >> I already sent a patch - that is what started this thread :-) >> >> I include it below. >> You said "The patch doesn't seems to be correct" but didn't expand on why. >> Do you still think it is not correct? I wouldn't be surprised if there is >> some case that it doesn't handle quite right, but it seems right to me. >> >> Thanks, >> NeilBrown >> >> >> From: NeilBrown <neilb@suse.de> >> Subject: [PATCH] OMAP GPIO - don't wake from suspend unless requested. >> >> Current kernel will wake from suspend on an event on any active >> GPIO even if enable_irq_wake() wasn't called. >> >> There are two reasons that the hardware wake-enable bit should be set: >> >> 1/ while non-suspended the CPU might go into a deep sleep (off_mode) >> in which the wake-enable bit is needed for an interrupt to be >> recognised. >> 2/ while suspended the GPIO interrupt should wake from suspend if and >> only if irq_wake as been enabled. >> >> The code currently doesn't keep these two reasons separate so they get >> confused and sometimes the wakeup flags is set incorrectly. >> >> This patch reverts: >> commit 9c4ed9e6c01e7a8bd9079da8267e1f03cb4761fc >> gpio/omap: remove suspend/resume callbacks >> and >> commit 0aa2727399c0b78225021413022c164cb99fbc5e >> gpio/omap: remove suspend_wakeup field from struct gpio_bank >> >> and makes some minor changes so that we have separate flags for "GPIO >> should wake from deep idle" and "GPIO should wake from suspend". >> >> With this patch, the GPIO from my touch screen doesn't wake my device >> any more, which is what I want. > > I think the direction is right here. We never should've separated the > handling of idle vs suspend wakeups. However, I have a few > questions/doubts below... > I thought irq_set_wake() is suspend only wakeup functionality. In idle, the driver IRQs are not disabled/masked so there no need of any special wakeup calls for idle. More ever patch is adding the suspend hooks for wakeups. I have no objection on the subject patch, but the suspend wakeup facility is easy enough to implement for IRQCHIPS and that is what I was proposing it. Infact the mask on suspend patch almost works and it fails only because the GPIO driver is suspended earlier than the IRQ framework expect it to be. Anyways I step back here since the proposed patch already fixes the issue seen. Assuming the IRQCHIP mask on suspend doesn't seems to work well with drivers as Neil mentioned, the $subject patch seems to be the right option. Regards, Santosh -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 6 Sep 2012 16:26:06 +0300 Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Thu, Sep 06, 2012 at 05:02:45PM +1000, NeilBrown wrote: > > On Thu, 6 Sep 2012 11:18:09 +0530 "Shilimkar, Santosh" > > <santosh.shilimkar@ti.com> wrote: > > > > > On Thu, Sep 6, 2012 at 8:35 AM, NeilBrown <neilb@suse.de> wrote: > > > > On Mon, 3 Sep 2012 22:59:06 -0700 "Shilimkar, Santosh" > > > > <santosh.shilimkar@ti.com> wrote: > > > > > >> After thinking bit more on this, the problem seems to be coming > > > >> mainly because the gpio device is runtime suspended bit early than > > > >> it should be. Similar issue seen with i2c driver as well. The i2c issue > > > >> was discussed with Rafael at LPC last week. The idea is to move > > > >> the pm_runtime_enable/disable() calls entirely up to the > > > >> _late/_early stage of device suspend/resume. > > > >> Will update this thread once I have further update. > > > > > > > > This won't be late enough. IRQCHIP_MASK_ON_SUSPEND takes effect after all > > > > the _late callbacks have been called. > > > > I, too, spoke to Rafael about this in San Diego. He seemed to agree with me > > > > that the interrupt needs to be masked in the ->suspend callback. any later > > > > is too late. > > > > > > > Thanks for information about your discussion. Will wait for the patch then. > > > > > > Regards > > > santosh > > > > I already sent a patch - that is what started this thread :-) > > > > I include it below. > > You said "The patch doesn't seems to be correct" but didn't expand on why. > > Do you still think it is not correct? I wouldn't be surprised if there is > > some case that it doesn't handle quite right, but it seems right to me. > > > > Thanks, > > NeilBrown > > > > > > From: NeilBrown <neilb@suse.de> > > Subject: [PATCH] OMAP GPIO - don't wake from suspend unless requested. > > > > Current kernel will wake from suspend on an event on any active > > GPIO even if enable_irq_wake() wasn't called. > > > > There are two reasons that the hardware wake-enable bit should be set: > > > > 1/ while non-suspended the CPU might go into a deep sleep (off_mode) > > in which the wake-enable bit is needed for an interrupt to be > > recognised. > > 2/ while suspended the GPIO interrupt should wake from suspend if and > > only if irq_wake as been enabled. > > > > The code currently doesn't keep these two reasons separate so they get > > confused and sometimes the wakeup flags is set incorrectly. > > > > This patch reverts: > > commit 9c4ed9e6c01e7a8bd9079da8267e1f03cb4761fc > > gpio/omap: remove suspend/resume callbacks > > and > > commit 0aa2727399c0b78225021413022c164cb99fbc5e > > gpio/omap: remove suspend_wakeup field from struct gpio_bank > > > > and makes some minor changes so that we have separate flags for "GPIO > > should wake from deep idle" and "GPIO should wake from suspend". > > > > With this patch, the GPIO from my touch screen doesn't wake my device > > any more, which is what I want. > > > > Cc: Kevin Hilman <khilman@ti.com> > > Cc: Tony Lindgren <tony@atomide.com> > > Cc: Santosh Shilimkar <santosh.shilimkar@ti.com> > > Cc: Cousson Benoit <b-cousson@ti.com> > > Cc: Grant Likely <grant.likely@secretlab.ca> > > Cc: Tarun Kanti DebBarma <tarun.kanti@ti.com> > > Cc: Felipe Balbi <balbi@ti.com> > > Cc: Govindraj.R <govindraj.raja@ti.com> > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > > > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c > > index 4fbc208..fdbad70 100644 > > --- a/drivers/gpio/gpio-omap.c > > +++ b/drivers/gpio/gpio-omap.c > > @@ -57,6 +57,7 @@ struct gpio_bank { > > u16 irq; > > int irq_base; > > struct irq_domain *domain; > > + u32 suspend_wakeup; > > u32 non_wakeup_gpios; > > u32 enabled_non_wakeup_gpios; > > struct gpio_regs context; > > @@ -522,11 +523,12 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable) > > > > spin_lock_irqsave(&bank->lock, flags); > > if (enable) > > - bank->context.wake_en |= gpio_bit; > > + bank->suspend_wakeup |= gpio_bit; > > else > > - bank->context.wake_en &= ~gpio_bit; > > + bank->suspend_wakeup &= ~gpio_bit; > > > > - __raw_writel(bank->context.wake_en, bank->base + bank->regs->wkup_en); > > + if (!bank->loses_context) > > + __raw_writel(bank->suspend_wakeup, bank->base + bank->regs->wkup_en); > > spin_unlock_irqrestore(&bank->lock, flags); > > > > return 0; > > @@ -1157,6 +1159,51 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev) > > #ifdef CONFIG_ARCH_OMAP2PLUS > > > > #if defined(CONFIG_PM_RUNTIME) > > + > > +#if defined(CONFIG_PM_SLEEP) > > +static int omap_gpio_suspend(struct device *dev) > > +{ > > + struct platform_device *pdev = to_platform_device(dev); > > + struct gpio_bank *bank = platform_get_drvdata(pdev); > > + void __iomem *base = bank->base; > > + unsigned long flags; > > + > > + if (!bank->mod_usage || !bank->loses_context) > > + return 0; > > + > > + if (!bank->regs->wkup_en || !bank->context.wake_en) > > + return 0; > > + > > + spin_lock_irqsave(&bank->lock, flags); > > shouldn't you be using _noirq methods instead ? Then this would become a > normal spin_lock()/spin_unlock(). > I don't think it is appropriate to move functionality between the different suspend call-backs just because it seems to make the code easier. Each callback has a purpose and we should stick to that purpose. The 'suspend' callback should transition the device to a quiescent state, and I think that includes ensuring that unwanted interrupts won't fire. 'suspend_late' should almost always be the same as runtime_suspend - it should power-off the device. 'suspend_noirq' ... doesn't seem to have a clear role any more since the introduction of suspend_late. Hopefully everything will transition over and suspend_noirq can disappear. More pragmatically: By the time we get to suspend_noirq, I think the iclk will have been turned off and so it is too late to try to clear the wkup flags. Thanks, NeilBrown
"Shilimkar, Santosh" <santosh.shilimkar@ti.com> writes: > On Sat, Sep 8, 2012 at 3:07 AM, Kevin Hilman > <khilman@deeprootsystems.com> wrote: >> Hi Neil, >> >> NeilBrown <neilb@suse.de> writes: >> >>> On Thu, 6 Sep 2012 11:18:09 +0530 "Shilimkar, Santosh" >>> <santosh.shilimkar@ti.com> wrote: >>> >>>> On Thu, Sep 6, 2012 at 8:35 AM, NeilBrown <neilb@suse.de> wrote: >>>> > On Mon, 3 Sep 2012 22:59:06 -0700 "Shilimkar, Santosh" >>>> > <santosh.shilimkar@ti.com> wrote: >>> >>>> >> After thinking bit more on this, the problem seems to be coming >>>> >> mainly because the gpio device is runtime suspended bit early than >>>> >> it should be. Similar issue seen with i2c driver as well. The i2c issue >>>> >> was discussed with Rafael at LPC last week. The idea is to move >>>> >> the pm_runtime_enable/disable() calls entirely up to the >>>> >> _late/_early stage of device suspend/resume. >>>> >> Will update this thread once I have further update. >>>> > >>>> > This won't be late enough. IRQCHIP_MASK_ON_SUSPEND takes effect after all >>>> > the _late callbacks have been called. >>>> > I, too, spoke to Rafael about this in San Diego. He seemed to agree with me >>>> > that the interrupt needs to be masked in the ->suspend callback. any later >>>> > is too late. >>>> > >>>> Thanks for information about your discussion. Will wait for the patch then. >>>> >>>> Regards >>>> santosh >>> >>> I already sent a patch - that is what started this thread :-) >>> >>> I include it below. >>> You said "The patch doesn't seems to be correct" but didn't expand on why. >>> Do you still think it is not correct? I wouldn't be surprised if there is >>> some case that it doesn't handle quite right, but it seems right to me. >>> >>> Thanks, >>> NeilBrown >>> >>> >>> From: NeilBrown <neilb@suse.de> >>> Subject: [PATCH] OMAP GPIO - don't wake from suspend unless requested. >>> >>> Current kernel will wake from suspend on an event on any active >>> GPIO even if enable_irq_wake() wasn't called. >>> >>> There are two reasons that the hardware wake-enable bit should be set: >>> >>> 1/ while non-suspended the CPU might go into a deep sleep (off_mode) >>> in which the wake-enable bit is needed for an interrupt to be >>> recognised. >>> 2/ while suspended the GPIO interrupt should wake from suspend if and >>> only if irq_wake as been enabled. >>> >>> The code currently doesn't keep these two reasons separate so they get >>> confused and sometimes the wakeup flags is set incorrectly. >>> >>> This patch reverts: >>> commit 9c4ed9e6c01e7a8bd9079da8267e1f03cb4761fc >>> gpio/omap: remove suspend/resume callbacks >>> and >>> commit 0aa2727399c0b78225021413022c164cb99fbc5e >>> gpio/omap: remove suspend_wakeup field from struct gpio_bank >>> >>> and makes some minor changes so that we have separate flags for "GPIO >>> should wake from deep idle" and "GPIO should wake from suspend". >>> >>> With this patch, the GPIO from my touch screen doesn't wake my device >>> any more, which is what I want. >> >> I think the direction is right here. We never should've separated the >> handling of idle vs suspend wakeups. However, I have a few >> questions/doubts below... >> > I thought irq_set_wake() is suspend only wakeup functionality. In idle, the > driver IRQs are not disabled/masked so there no need of any special > wakeup calls for idle. More ever patch is adding the suspend hooks > for wakeups. > > I have no objection on the subject patch, but the suspend wakeup > facility is easy enough to implement for IRQCHIPS and that is > what I was proposing it. Infact the mask on suspend patch almost > works and it fails only because the GPIO driver is suspended earlier > than the IRQ framework expect it to be. That is a pretty big problem to overcome. :) That being said, I don't see how simply using MASK_ON_SUSPEND can work for GPIO. AFAICT, that flag is for the whole irq_chip, not for individual IRQs. We really need to keep track at the bank/IRQ level, as in the proposed patch from Neil (actually, we used to have this featur, but I screwed up by not catching this removal when reviewing the GPIO cleanup/reorg series.) Because of retention/off in idle, we set *all* GPIOs with IRQ triggering to be wakeup enabled so they will cause wakeups during idle. During suspend, we only want the irq_set_wake() ones to cause wakeups. > Anyways I step back here since the proposed patch already fixes > the issue seen. Assuming the IRQCHIP mask on suspend doesn't > seems to work well with drivers as Neil mentioned, the $subject patch > seems to be the right option. OK thanks, I'll queue this up for v3.6-rc as this should qualify as a regression. Also, the IRQCHIP mask feature seems to have been designed for IRQ chips without the control registers to handle this. We have the control registers to handle it, so I believe it's better to keep this handled in the driver itself. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" 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 4fbc208..fdbad70 100644 --- a/drivers/gpio/gpio-omap.c +++ b/drivers/gpio/gpio-omap.c @@ -57,6 +57,7 @@ struct gpio_bank { u16 irq; int irq_base; struct irq_domain *domain; + u32 suspend_wakeup; u32 non_wakeup_gpios; u32 enabled_non_wakeup_gpios; struct gpio_regs context; @@ -522,11 +523,12 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int gpio, int enable) spin_lock_irqsave(&bank->lock, flags); if (enable) - bank->context.wake_en |= gpio_bit; + bank->suspend_wakeup |= gpio_bit; else - bank->context.wake_en &= ~gpio_bit; + bank->suspend_wakeup &= ~gpio_bit; - __raw_writel(bank->context.wake_en, bank->base + bank->regs->wkup_en); + if (!bank->loses_context) + __raw_writel(bank->suspend_wakeup, bank->base + bank->regs->wkup_en); spin_unlock_irqrestore(&bank->lock, flags); return 0; @@ -1157,6 +1159,51 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev) #ifdef CONFIG_ARCH_OMAP2PLUS #if defined(CONFIG_PM_RUNTIME) + +#if defined(CONFIG_PM_SLEEP) +static int omap_gpio_suspend(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct gpio_bank *bank = platform_get_drvdata(pdev); + void __iomem *base = bank->base; + unsigned long flags; + + if (!bank->mod_usage || !bank->loses_context) + return 0; + + if (!bank->regs->wkup_en || !bank->context.wake_en) + return 0; + + spin_lock_irqsave(&bank->lock, flags); + _gpio_rmw(base, bank->regs->wkup_en, 0xffffffff, 0); + _gpio_rmw(base, bank->regs->wkup_en, bank->suspend_wakeup, 1); + spin_unlock_irqrestore(&bank->lock, flags); + + return 0; +} + +static int omap_gpio_resume(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct gpio_bank *bank = platform_get_drvdata(pdev); + void __iomem *base = bank->base; + unsigned long flags; + + if (!bank->mod_usage || !bank->loses_context) + return 0; + + if (!bank->regs->wkup_en || !bank->context.wake_en) + return 0; + + spin_lock_irqsave(&bank->lock, flags); + _gpio_rmw(base, bank->regs->wkup_en, 0xffffffff, 0); + _gpio_rmw(base, bank->regs->wkup_en, bank->context.wake_en, 1); + spin_unlock_irqrestore(&bank->lock, flags); + + return 0; +} +#endif /* CONFIG_PM_SLEEP */ + static void omap_gpio_restore_context(struct gpio_bank *bank); static int omap_gpio_runtime_suspend(struct device *dev) @@ -1386,11 +1433,14 @@ static void omap_gpio_restore_context(struct gpio_bank *bank) } #endif /* CONFIG_PM_RUNTIME */ #else +#define omap_gpio_suspend NULL +#define omap_gpio_resume NULL #define omap_gpio_runtime_suspend NULL #define omap_gpio_runtime_resume NULL #endif static const struct dev_pm_ops gpio_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(omap_gpio_suspend, omap_gpio_resume) SET_RUNTIME_PM_OPS(omap_gpio_runtime_suspend, omap_gpio_runtime_resume, NULL) };