Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[Shilimkar, Santosh]

[-- Attachment #1: Type: text/plain, Size: 2794 bytes --]

+ Jon,

On Sat, Aug 25, 2012 at 5:14 PM, NeilBrown <neilb@suse.de> wrote:
>
>
>
> 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>
>
The patch doesn't seems to be correct. At least the 2/ gets
fixed with a proper IRQCHIP flag. Can you try the patch at
end of the email and see if it helps ? Am attaching it in case
mailer damages it.

Regards
Santosh

>From b8a38fc75e046f6462610e26c47c620cad850c24 Mon Sep 17 00:00:00 2001
From: Santosh Shilimkar <santosh.shilimkar@ti.com>
Date: Sun, 26 Aug 2012 09:39:51 +0530
Subject: [PATCH] gpio: omap: Set IRQCHIP_MASK_ON_SUSPEND to mask all
 non-wakeup gpio wakeups.

Set the irq chip flag IRQCHIP_MASK_ON_SUSPEND to cause the irq pm code
to mask all non-wake gpios in suspend, which will ensure the wakeup enable
bit is not set on non-wake gpios.

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 drivers/gpio/gpio-omap.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index e6efd77..50b4c18 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -779,6 +779,7 @@ static struct irq_chip gpio_irq_chip = {
 	.irq_unmask	= gpio_unmask_irq,
 	.irq_set_type	= gpio_irq_type,
 	.irq_set_wake	= gpio_wake_enable,
+	.flags		= IRQCHIP_MASK_ON_SUSPEND;
 };

 /*---------------------------------------------------------------------*/
-- 
1.7.9.5

[-- Attachment #2: 0001-gpio-omap-Set-IRQCHIP_MASK_ON_SUSPEND-to-mask-all-no.patch --]
[-- Type: application/octet-stream, Size: 1020 bytes --]

From b8a38fc75e046f6462610e26c47c620cad850c24 Mon Sep 17 00:00:00 2001
From: Santosh Shilimkar <santosh.shilimkar@ti.com>
Date: Sun, 26 Aug 2012 09:39:51 +0530
Subject: [PATCH] gpio: omap: Set IRQCHIP_MASK_ON_SUSPEND to mask all
 non-wakeup gpio wakeups.

Set the irq chip flag IRQCHIP_MASK_ON_SUSPEND to cause the irq pm code
to mask all non-wake gpios in suspend, which will ensure the wakeup enable
bit is not set on non-wake gpios.

Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 drivers/gpio/gpio-omap.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index e6efd77..50b4c18 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -779,6 +779,7 @@ static struct irq_chip gpio_irq_chip = {
 	.irq_unmask	= gpio_unmask_irq,
 	.irq_set_type	= gpio_irq_type,
 	.irq_set_wake	= gpio_wake_enable,
+	.flags		= IRQCHIP_MASK_ON_SUSPEND;
 };
 
 /*---------------------------------------------------------------------*/
-- 
1.7.9.5

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 4571 bytes --]

On Sun, 26 Aug 2012 09:47:50 +0530 "Shilimkar, Santosh"
<santosh.shilimkar@ti.com> wrote:

> + Jon,
> 
> On Sat, Aug 25, 2012 at 5:14 PM, NeilBrown <neilb@suse.de> wrote:
> >
> >
> >
> > 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>
> >
> The patch doesn't seems to be correct. At least the 2/ gets
> fixed with a proper IRQCHIP flag. Can you try the patch at
> end of the email and see if it helps ? Am attaching it in case
> mailer damages it.
> 
> Regards
> Santosh
> 
> >From b8a38fc75e046f6462610e26c47c620cad850c24 Mon Sep 17 00:00:00 2001
> From: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Date: Sun, 26 Aug 2012 09:39:51 +0530
> Subject: [PATCH] gpio: omap: Set IRQCHIP_MASK_ON_SUSPEND to mask all
>  non-wakeup gpio wakeups.
> 
> Set the irq chip flag IRQCHIP_MASK_ON_SUSPEND to cause the irq pm code
> to mask all non-wake gpios in suspend, which will ensure the wakeup enable
> bit is not set on non-wake gpios.
> 
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> ---
>  drivers/gpio/gpio-omap.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index e6efd77..50b4c18 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -779,6 +779,7 @@ static struct irq_chip gpio_irq_chip = {
>  	.irq_unmask	= gpio_unmask_irq,
>  	.irq_set_type	= gpio_irq_type,
>  	.irq_set_wake	= gpio_wake_enable,
> +	.flags		= IRQCHIP_MASK_ON_SUSPEND;
>  };
> 
>  /*---------------------------------------------------------------------*/


No obvious damage, unless the mailer is responsible or the ';' at the end of
the line, rather than ',' :-)

The approach makes sense, but does actually work.  Should be fixable though.

When I try this I get:



[  158.114440] Checking wakeup interrupts
[  158.118408] Unhandled fault: external abort on non-linefetch (0x1028) at 0xfb054040
[  158.126403] Internal error: : 1028 [#1] PREEMPT ARM
[  158.131500] Modules linked in: ipv6 g_ether hso libertas_sdio libertas cfg80211
[  158.139190] CPU: 0    Not tainted  (3.5.0-gta04-debug+ #2)
[  158.144927] PC is at _set_gpio_triggering+0x38/0x258
[  158.150115] LR is at gpio_mask_irq+0xac/0xc0
[  158.154602] pc : [<c01d24a0>]    lr : [<c01d2f68>]    psr: 60000193
[  158.154602] sp : db521e90  ip : 00000011  fp : beeecc2c
[  158.166595] r10: c05c8ebc  r9 : daa5a858  r8 : 00000003
[  158.172027] r7 : a0000193  r6 : 00000000  r5 : fb054000  r4 : ded44e18
[  158.178863] r3 : 00000001  r2 : 00000000  r1 : ded30340  r0 : 00000040
[  158.185668] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment use

so it looks like runtime PM has turned off the iclk to the GPIO module so that
when we try to tell it to change settings, it is no longer listening to us.
The "Checking wakeup interrupts" function happens very late in the suspend
cycle, after all the suspend_late and suspend_noirq functions have run.
Maybe it needs to be moved earlier...

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[Shilimkar, Santosh]

On Sun, Aug 26, 2012 at 3:53 PM, NeilBrown <neilb@suse.de> wrote:
>
> On Sun, 26 Aug 2012 09:47:50 +0530 "Shilimkar, Santosh"
> <santosh.shilimkar@ti.com> wrote:
>
> > + Jon,
> >
> > On Sat, Aug 25, 2012 at 5:14 PM, NeilBrown <neilb@suse.de> wrote:
> > >
> > >
> > >
> > > 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>
> > >
> > The patch doesn't seems to be correct. At least the 2/ gets
> > fixed with a proper IRQCHIP flag. Can you try the patch at
> > end of the email and see if it helps ? Am attaching it in case
> > mailer damages it.
> >
> > Regards
> > Santosh
> >
> > >From b8a38fc75e046f6462610e26c47c620cad850c24 Mon Sep 17 00:00:00 2001
> > From: Santosh Shilimkar <santosh.shilimkar@ti.com>
> > Date: Sun, 26 Aug 2012 09:39:51 +0530
> > Subject: [PATCH] gpio: omap: Set IRQCHIP_MASK_ON_SUSPEND to mask all
> >  non-wakeup gpio wakeups.
> >
> > Set the irq chip flag IRQCHIP_MASK_ON_SUSPEND to cause the irq pm code
> > to mask all non-wake gpios in suspend, which will ensure the wakeup
> > enable
> > bit is not set on non-wake gpios.
> >
> > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> > ---
> >  drivers/gpio/gpio-omap.c |    1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> > index e6efd77..50b4c18 100644
> > --- a/drivers/gpio/gpio-omap.c
> > +++ b/drivers/gpio/gpio-omap.c
> > @@ -779,6 +779,7 @@ static struct irq_chip gpio_irq_chip = {
> >       .irq_unmask     = gpio_unmask_irq,
> >       .irq_set_type   = gpio_irq_type,
> >       .irq_set_wake   = gpio_wake_enable,
> > +     .flags          = IRQCHIP_MASK_ON_SUSPEND;
> >  };
> >
> >
> > /*---------------------------------------------------------------------*/
>
>
> No obvious damage, unless the mailer is responsible or the ';' at the end
> of
> the line, rather than ',' :-)
>
:-) That was typo.

> The approach makes sense, but does actually work.  Should be fixable
> though.
>
> When I try this I get:
>
>
>
> [  158.114440] Checking wakeup interrupts
> [  158.118408] Unhandled fault: external abort on non-linefetch (0x1028)
> at 0xfb054040
> [  158.126403] Internal error: : 1028 [#1] PREEMPT ARM
> [  158.131500] Modules linked in: ipv6 g_ether hso libertas_sdio libertas
> cfg80211
> [  158.139190] CPU: 0    Not tainted  (3.5.0-gta04-debug+ #2)
> [  158.144927] PC is at _set_gpio_triggering+0x38/0x258
> [  158.150115] LR is at gpio_mask_irq+0xac/0xc0
> [  158.154602] pc : [<c01d24a0>]    lr : [<c01d2f68>]    psr: 60000193
> [  158.154602] sp : db521e90  ip : 00000011  fp : beeecc2c
> [  158.166595] r10: c05c8ebc  r9 : daa5a858  r8 : 00000003
> [  158.172027] r7 : a0000193  r6 : 00000000  r5 : fb054000  r4 : ded44e18
> [  158.178863] r3 : 00000001  r2 : 00000000  r1 : ded30340  r0 : 00000040
> [  158.185668] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM
> Segment use
>
> so it looks like runtime PM has turned off the iclk to the GPIO module so
> that
> when we try to tell it to change settings, it is no longer listening to
> us.
>From the crash logs it appears like that.

> The "Checking wakeup interrupts" function happens very late in the suspend
> cycle, after all the suspend_late and suspend_noirq functions have run.
> Maybe it needs to be moved earlier...
>
No it shouldn't be moved and it is that point for lot many good
reasons. Ofcourse
this omap gpio driver crash needs to be addressed. Need to think bit
more on this
issue.

Regards
Santosh

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[Shilimkar, Santosh]

On Sun, Aug 26, 2012 at 6:29 PM, Shilimkar, Santosh
<santosh.shilimkar@ti.com> wrote:
> On Sun, Aug 26, 2012 at 3:53 PM, NeilBrown <neilb@suse.de> wrote:
>>
>> On Sun, 26 Aug 2012 09:47:50 +0530 "Shilimkar, Santosh"
>> <santosh.shilimkar@ti.com> wrote:
>>
>> > + Jon,
>> >
>> > On Sat, Aug 25, 2012 at 5:14 PM, NeilBrown <neilb@suse.de> wrote:
>> > >
>> > >
>> > >
>> > > 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>
>> > >
>> > The patch doesn't seems to be correct. At least the 2/ gets
>> > fixed with a proper IRQCHIP flag. Can you try the patch at
>> > end of the email and see if it helps ? Am attaching it in case
>> > mailer damages it.
>> >
>> > Regards
>> > Santosh
>> >
>> > >From b8a38fc75e046f6462610e26c47c620cad850c24 Mon Sep 17 00:00:00 2001
>> > From: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> > Date: Sun, 26 Aug 2012 09:39:51 +0530
>> > Subject: [PATCH] gpio: omap: Set IRQCHIP_MASK_ON_SUSPEND to mask all
>> >  non-wakeup gpio wakeups.
>> >
>> > Set the irq chip flag IRQCHIP_MASK_ON_SUSPEND to cause the irq pm code
>> > to mask all non-wake gpios in suspend, which will ensure the wakeup
>> > enable
>> > bit is not set on non-wake gpios.
>> >
>> > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> > ---
>> >  drivers/gpio/gpio-omap.c |    1 +
>> >  1 file changed, 1 insertion(+)
>> >
>> > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> > index e6efd77..50b4c18 100644
>> > --- a/drivers/gpio/gpio-omap.c
>> > +++ b/drivers/gpio/gpio-omap.c
>> > @@ -779,6 +779,7 @@ static struct irq_chip gpio_irq_chip = {
>> >       .irq_unmask     = gpio_unmask_irq,
>> >       .irq_set_type   = gpio_irq_type,
>> >       .irq_set_wake   = gpio_wake_enable,
>> > +     .flags          = IRQCHIP_MASK_ON_SUSPEND;
>> >  };
>> >
>> >
>> > /*---------------------------------------------------------------------*/
>>
>>
>> No obvious damage, unless the mailer is responsible or the ';' at the end
>> of
>> the line, rather than ',' :-)
>>
> :-) That was typo.
>
>> The approach makes sense, but does actually work.  Should be fixable
>> though.
>>
>> When I try this I get:
>>
>>
>>
>> [  158.114440] Checking wakeup interrupts
>> [  158.118408] Unhandled fault: external abort on non-linefetch (0x1028)
>> at 0xfb054040
>> [  158.126403] Internal error: : 1028 [#1] PREEMPT ARM
>> [  158.131500] Modules linked in: ipv6 g_ether hso libertas_sdio libertas
>> cfg80211
>> [  158.139190] CPU: 0    Not tainted  (3.5.0-gta04-debug+ #2)
>> [  158.144927] PC is at _set_gpio_triggering+0x38/0x258
>> [  158.150115] LR is at gpio_mask_irq+0xac/0xc0
>> [  158.154602] pc : [<c01d24a0>]    lr : [<c01d2f68>]    psr: 60000193
>> [  158.154602] sp : db521e90  ip : 00000011  fp : beeecc2c
>> [  158.166595] r10: c05c8ebc  r9 : daa5a858  r8 : 00000003
>> [  158.172027] r7 : a0000193  r6 : 00000000  r5 : fb054000  r4 : ded44e18
>> [  158.178863] r3 : 00000001  r2 : 00000000  r1 : ded30340  r0 : 00000040
>> [  158.185668] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM
>> Segment use
>>
>> so it looks like runtime PM has turned off the iclk to the GPIO module so
>> that
>> when we try to tell it to change settings, it is no longer listening to
>> us.
> From the crash logs it appears like that.
>
>> The "Checking wakeup interrupts" function happens very late in the suspend
>> cycle, after all the suspend_late and suspend_noirq functions have run.
>> Maybe it needs to be moved earlier...
>>
> No it shouldn't be moved and it is that point for lot many good
> reasons. Ofcourse
> this omap gpio driver crash needs to be addressed. Need to think bit
> more on this
> issue.
>
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.

Regards
Santosh

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 6504 bytes --]

On Mon, 3 Sep 2012 22:59:06 -0700 "Shilimkar, Santosh"
<santosh.shilimkar@ti.com> wrote:

> On Sun, Aug 26, 2012 at 6:29 PM, Shilimkar, Santosh
> <santosh.shilimkar@ti.com> wrote:
> > On Sun, Aug 26, 2012 at 3:53 PM, NeilBrown <neilb@suse.de> wrote:
> >>
> >> On Sun, 26 Aug 2012 09:47:50 +0530 "Shilimkar, Santosh"
> >> <santosh.shilimkar@ti.com> wrote:
> >>
> >> > + Jon,
> >> >
> >> > On Sat, Aug 25, 2012 at 5:14 PM, NeilBrown <neilb@suse.de> wrote:
> >> > >
> >> > >
> >> > >
> >> > > 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>
> >> > >
> >> > The patch doesn't seems to be correct. At least the 2/ gets
> >> > fixed with a proper IRQCHIP flag. Can you try the patch at
> >> > end of the email and see if it helps ? Am attaching it in case
> >> > mailer damages it.
> >> >
> >> > Regards
> >> > Santosh
> >> >
> >> > >From b8a38fc75e046f6462610e26c47c620cad850c24 Mon Sep 17 00:00:00 2001
> >> > From: Santosh Shilimkar <santosh.shilimkar@ti.com>
> >> > Date: Sun, 26 Aug 2012 09:39:51 +0530
> >> > Subject: [PATCH] gpio: omap: Set IRQCHIP_MASK_ON_SUSPEND to mask all
> >> >  non-wakeup gpio wakeups.
> >> >
> >> > Set the irq chip flag IRQCHIP_MASK_ON_SUSPEND to cause the irq pm code
> >> > to mask all non-wake gpios in suspend, which will ensure the wakeup
> >> > enable
> >> > bit is not set on non-wake gpios.
> >> >
> >> > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> >> > ---
> >> >  drivers/gpio/gpio-omap.c |    1 +
> >> >  1 file changed, 1 insertion(+)
> >> >
> >> > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> >> > index e6efd77..50b4c18 100644
> >> > --- a/drivers/gpio/gpio-omap.c
> >> > +++ b/drivers/gpio/gpio-omap.c
> >> > @@ -779,6 +779,7 @@ static struct irq_chip gpio_irq_chip = {
> >> >       .irq_unmask     = gpio_unmask_irq,
> >> >       .irq_set_type   = gpio_irq_type,
> >> >       .irq_set_wake   = gpio_wake_enable,
> >> > +     .flags          = IRQCHIP_MASK_ON_SUSPEND;
> >> >  };
> >> >
> >> >
> >> > /*---------------------------------------------------------------------*/
> >>
> >>
> >> No obvious damage, unless the mailer is responsible or the ';' at the end
> >> of
> >> the line, rather than ',' :-)
> >>
> > :-) That was typo.
> >
> >> The approach makes sense, but does actually work.  Should be fixable
> >> though.
> >>
> >> When I try this I get:
> >>
> >>
> >>
> >> [  158.114440] Checking wakeup interrupts
> >> [  158.118408] Unhandled fault: external abort on non-linefetch (0x1028)
> >> at 0xfb054040
> >> [  158.126403] Internal error: : 1028 [#1] PREEMPT ARM
> >> [  158.131500] Modules linked in: ipv6 g_ether hso libertas_sdio libertas
> >> cfg80211
> >> [  158.139190] CPU: 0    Not tainted  (3.5.0-gta04-debug+ #2)
> >> [  158.144927] PC is at _set_gpio_triggering+0x38/0x258
> >> [  158.150115] LR is at gpio_mask_irq+0xac/0xc0
> >> [  158.154602] pc : [<c01d24a0>]    lr : [<c01d2f68>]    psr: 60000193
> >> [  158.154602] sp : db521e90  ip : 00000011  fp : beeecc2c
> >> [  158.166595] r10: c05c8ebc  r9 : daa5a858  r8 : 00000003
> >> [  158.172027] r7 : a0000193  r6 : 00000000  r5 : fb054000  r4 : ded44e18
> >> [  158.178863] r3 : 00000001  r2 : 00000000  r1 : ded30340  r0 : 00000040
> >> [  158.185668] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM
> >> Segment use
> >>
> >> so it looks like runtime PM has turned off the iclk to the GPIO module so
> >> that
> >> when we try to tell it to change settings, it is no longer listening to
> >> us.
> > From the crash logs it appears like that.
> >
> >> The "Checking wakeup interrupts" function happens very late in the suspend
> >> cycle, after all the suspend_late and suspend_noirq functions have run.
> >> Maybe it needs to be moved earlier...
> >>
> > No it shouldn't be moved and it is that point for lot many good
> > reasons. Ofcourse
> > this omap gpio driver crash needs to be addressed. Need to think bit
> > more on this
> > issue.
> >
> 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.

NeilBrown


> 
> Regards
> Santosh


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[Shilimkar, Santosh]

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:
>
>> On Sun, Aug 26, 2012 at 6:29 PM, Shilimkar, Santosh
>> <santosh.shilimkar@ti.com> wrote:
>> > On Sun, Aug 26, 2012 at 3:53 PM, NeilBrown <neilb@suse.de> wrote:
>> >>
>> >> On Sun, 26 Aug 2012 09:47:50 +0530 "Shilimkar, Santosh"
>> >> <santosh.shilimkar@ti.com> wrote:
>> >>
>> >> > + Jon,
>> >> >
>> >> > On Sat, Aug 25, 2012 at 5:14 PM, NeilBrown <neilb@suse.de> wrote:
>> >> > >
>> >> > >
>> >> > >
>> >> > > 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>
>> >> > >
>> >> > The patch doesn't seems to be correct. At least the 2/ gets
>> >> > fixed with a proper IRQCHIP flag. Can you try the patch at
>> >> > end of the email and see if it helps ? Am attaching it in case
>> >> > mailer damages it.
>> >> >
>> >> > Regards
>> >> > Santosh
>> >> >
>> >> > >From b8a38fc75e046f6462610e26c47c620cad850c24 Mon Sep 17 00:00:00 2001
>> >> > From: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> >> > Date: Sun, 26 Aug 2012 09:39:51 +0530
>> >> > Subject: [PATCH] gpio: omap: Set IRQCHIP_MASK_ON_SUSPEND to mask all
>> >> >  non-wakeup gpio wakeups.
>> >> >
>> >> > Set the irq chip flag IRQCHIP_MASK_ON_SUSPEND to cause the irq pm code
>> >> > to mask all non-wake gpios in suspend, which will ensure the wakeup
>> >> > enable
>> >> > bit is not set on non-wake gpios.
>> >> >
>> >> > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> >> > ---
>> >> >  drivers/gpio/gpio-omap.c |    1 +
>> >> >  1 file changed, 1 insertion(+)
>> >> >
>> >> > diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> >> > index e6efd77..50b4c18 100644
>> >> > --- a/drivers/gpio/gpio-omap.c
>> >> > +++ b/drivers/gpio/gpio-omap.c
>> >> > @@ -779,6 +779,7 @@ static struct irq_chip gpio_irq_chip = {
>> >> >       .irq_unmask     = gpio_unmask_irq,
>> >> >       .irq_set_type   = gpio_irq_type,
>> >> >       .irq_set_wake   = gpio_wake_enable,
>> >> > +     .flags          = IRQCHIP_MASK_ON_SUSPEND;
>> >> >  };
>> >> >
>> >> >
>> >> > /*---------------------------------------------------------------------*/
>> >>
>> >>
>> >> No obvious damage, unless the mailer is responsible or the ';' at the end
>> >> of
>> >> the line, rather than ',' :-)
>> >>
>> > :-) That was typo.
>> >
>> >> The approach makes sense, but does actually work.  Should be fixable
>> >> though.
>> >>
>> >> When I try this I get:
>> >>
>> >>
>> >>
>> >> [  158.114440] Checking wakeup interrupts
>> >> [  158.118408] Unhandled fault: external abort on non-linefetch (0x1028)
>> >> at 0xfb054040
>> >> [  158.126403] Internal error: : 1028 [#1] PREEMPT ARM
>> >> [  158.131500] Modules linked in: ipv6 g_ether hso libertas_sdio libertas
>> >> cfg80211
>> >> [  158.139190] CPU: 0    Not tainted  (3.5.0-gta04-debug+ #2)
>> >> [  158.144927] PC is at _set_gpio_triggering+0x38/0x258
>> >> [  158.150115] LR is at gpio_mask_irq+0xac/0xc0
>> >> [  158.154602] pc : [<c01d24a0>]    lr : [<c01d2f68>]    psr: 60000193
>> >> [  158.154602] sp : db521e90  ip : 00000011  fp : beeecc2c
>> >> [  158.166595] r10: c05c8ebc  r9 : daa5a858  r8 : 00000003
>> >> [  158.172027] r7 : a0000193  r6 : 00000000  r5 : fb054000  r4 : ded44e18
>> >> [  158.178863] r3 : 00000001  r2 : 00000000  r1 : ded30340  r0 : 00000040
>> >> [  158.185668] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM
>> >> Segment use
>> >>
>> >> so it looks like runtime PM has turned off the iclk to the GPIO module so
>> >> that
>> >> when we try to tell it to change settings, it is no longer listening to
>> >> us.
>> > From the crash logs it appears like that.
>> >
>> >> The "Checking wakeup interrupts" function happens very late in the suspend
>> >> cycle, after all the suspend_late and suspend_noirq functions have run.
>> >> Maybe it needs to be moved earlier...
>> >>
>> > No it shouldn't be moved and it is that point for lot many good
>> > reasons. Ofcourse
>> > this omap gpio driver crash needs to be addressed. Need to think bit
>> > more on this
>> > issue.
>> >
>> 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

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 6030 bytes --]

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);
+	_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)
 };


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[Shilimkar, Santosh]

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

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3107 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[Shilimkar, Santosh]

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

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 5017 bytes --]

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().

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 6175 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[Shubhrajyoti]

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>

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[Kevin Hilman]

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)
>  };

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[Shilimkar, Santosh]

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

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[Kevin Hilman]

"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

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 5007 bytes --]

On Mon, 10 Sep 2012 10:57:07 -0700 Kevin Hilman <khilman@deeprootsystems.com>
wrote:


> OK thanks, I'll queue this up for v3.6-rc as this should qualify as a
> regression.

I don't think this did actually get queued.  At least I'm still seeing the
bug in 3.7 and I cannot see a patch in the git history that looks right.
But then I don't remember what we ended up with - it was 3 months ago!!!

This is the last thing I can find in my email history - it still seems to
apply and still seems to work.

NeilBrown


From: NeilBrown <neilb@suse.de>
Date: Mon, 10 Sep 2012 14:09:06 +1000
Subject: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

Current kernel will wake from suspend on an event an any active
GPIO event 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: Benoit Cousson <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..79e1340 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,9 @@ 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;
-
-	__raw_writel(bank->context.wake_en, bank->base + bank->regs->wkup_en);
+		bank->suspend_wakeup &= ~gpio_bit;
 	spin_unlock_irqrestore(&bank->lock, flags);
 
 	return 0;
@@ -1157,6 +1156,49 @@ 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->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->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 +1428,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)
 };

[signature.asc  application/pgp-signature (828 bytes)] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[anish kumar]

On Fri, 2012-12-14 at 18:05 +1100, NeilBrown wrote:
> On Mon, 10 Sep 2012 10:57:07 -0700 Kevin Hilman <khilman@deeprootsystems.com>
> wrote:
> 
> 
> > OK thanks, I'll queue this up for v3.6-rc as this should qualify as a
> > regression.
> 
> I don't think this did actually get queued.  At least I'm still seeing the
> bug in 3.7 and I cannot see a patch in the git history that looks right.
> But then I don't remember what we ended up with - it was 3 months ago!!!
> 
> This is the last thing I can find in my email history - it still seems to
> apply and still seems to work.
> 
> NeilBrown
> 
> 
> From: NeilBrown <neilb@suse.de>
> Date: Mon, 10 Sep 2012 14:09:06 +1000
> Subject: [PATCH] OMAP GPIO - don't wake from suspend unless requested.
> 
> Current kernel will wake from suspend on an event an any active
> GPIO event if enable_irq_wake() wasn't called.
Should it read this way "Current kernel will wake from suspend on any
active gpio event 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
"in which case"
>   recognised.
> 2/ while suspended the GPIO interrupt should wake from suspend if and
>    only if irq_wake as been enabled.
"has 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: Benoit Cousson <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..79e1340 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,9 @@ 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;
> -
> -	__raw_writel(bank->context.wake_en, bank->base + bank->regs->wkup_en);
> +		bank->suspend_wakeup &= ~gpio_bit;
>  	spin_unlock_irqrestore(&bank->lock, flags);
>  
>  	return 0;
> @@ -1157,6 +1156,49 @@ 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->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->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 +1428,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)
>  };
> 
> [signature.asc  application/pgp-signature (828 bytes)] 

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[Grant Likely]

On Fri, 14 Dec 2012 18:05:53 +1100, NeilBrown <neilb@suse.de> wrote:
> On Mon, 10 Sep 2012 10:57:07 -0700 Kevin Hilman <khilman@deeprootsystems.com>
> wrote:
> 
> 
> > OK thanks, I'll queue this up for v3.6-rc as this should qualify as a
> > regression.
> 
> I don't think this did actually get queued.  At least I'm still seeing the
> bug in 3.7 and I cannot see a patch in the git history that looks right.
> But then I don't remember what we ended up with - it was 3 months ago!!!
> 
> This is the last thing I can find in my email history - it still seems to
> apply and still seems to work.
> 
> NeilBrown

Kevin, let me know if I need to do anything here. Since you might have
it in one of you're trees, I'm not going to do anything unless I hear
from you.

g.

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[Kevin Hilman]

Grant Likely <grant.likely@secretlab.ca> writes:

> On Fri, 14 Dec 2012 18:05:53 +1100, NeilBrown <neilb@suse.de> wrote:
>> On Mon, 10 Sep 2012 10:57:07 -0700 Kevin Hilman <khilman@deeprootsystems.com>
>> wrote:
>> 
>> 
>> > OK thanks, I'll queue this up for v3.6-rc as this should qualify as a
>> > regression.
>> 
>> I don't think this did actually get queued.  At least I'm still seeing the
>> bug in 3.7 and I cannot see a patch in the git history that looks right.
>> But then I don't remember what we ended up with - it was 3 months ago!!!
>> 
>> This is the last thing I can find in my email history - it still seems to
>> apply and still seems to work.
>> 
>> NeilBrown
>
> Kevin, let me know if I need to do anything here. Since you might have
> it in one of you're trees, I'm not going to do anything unless I hear
> from you.

oops, just stumbled across this one.  Sorry for the lag.

Grant, feel free to apply.  Seems this one never made it into my queue.

Kevin

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 10391 bytes --]

On Fri, 07 Sep 2012 14:37:16 -0700 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...
> 
> > 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...

Yes... I was clearly confused when writing that.  There was a good reason at
the time, but I doesn't seem to stand up to the cold light of day..


> 
> >  	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.
> 

Yes,  I see that now.  Thanks.

follow patch folds those to fixes in.

NeilBrown

From bd9d5e9f8742c9cdc795e3d9b895f74defddb6d9 Mon Sep 17 00:00:00 2001
From: NeilBrown <neilb@suse.de>
Date: Mon, 10 Sep 2012 14:09:06 +1000
Subject: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

Current kernel will wake from suspend on an event an any active
GPIO event 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: Benoit Cousson <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..79e1340 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,9 @@ 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;
-
-	__raw_writel(bank->context.wake_en, bank->base + bank->regs->wkup_en);
+		bank->suspend_wakeup &= ~gpio_bit;
 	spin_unlock_irqrestore(&bank->lock, flags);
 
 	return 0;
@@ -1157,6 +1156,49 @@ 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->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->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 +1428,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)
 };

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.

[Kevin Hilman]

NeilBrown <neilb@suse.de> writes:

[...]

> Yes,  I see that now.  Thanks.
>
> follow patch folds those to fixes in.
>
> NeilBrown
>
> From bd9d5e9f8742c9cdc795e3d9b895f74defddb6d9 Mon Sep 17 00:00:00 2001
> From: NeilBrown <neilb@suse.de>
> Date: Mon, 10 Sep 2012 14:09:06 +1000
> Subject: [PATCH] OMAP GPIO - don't wake from suspend unless requested.
>
> Current kernel will wake from suspend on an event an any active
> GPIO event 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: Benoit Cousson <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..79e1340 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,9 @@ 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;
> -
> -	__raw_writel(bank->context.wake_en, bank->base + bank->regs->wkup_en);
> +		bank->suspend_wakeup &= ~gpio_bit;
>  	spin_unlock_irqrestore(&bank->lock, flags);
>  
>  	return 0;
> @@ -1157,6 +1156,49 @@ 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->regs->wkup_en ||
> +	    !bank->context.wake_en)

nit: the context->wake_en check isn't really needed here.

> +		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);

I know we had the duplicate read/modify/writes here before, but that causes
a bunch of unnecessary register accesses.  Simply writing
bank->suspend_wakeup should suffice here...

> +	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->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);

...similarily, simply writing context.wake_en should suffice here (after
removing the check for non-zero wake_en above so that we're sure that
the setting of suspend_wakeup is undone.)

Kevin

> +	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 +1428,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)
>  };