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

[NeilBrown]

On Thu, 6 Sep 2012 12:57:46 +0530 "Shilimkar, Santosh"
<santosh.shilimkar@xxxxxx> wrote:

> On Thu, Sep 6, 2012 at 12:32 PM, NeilBrown <neilb@xxxxxxx> wrote:
> > On Thu, 6 Sep 2012 11:18:09 +0530 "Shilimkar, Santosh"
> > <santosh.shilimkar@xxxxxx> wrote:
> >
> >> On Thu, Sep 6, 2012 at 8:35 AM, NeilBrown <neilb@xxxxxxx> wrote:
> >> > On Mon, 3 Sep 2012 22:59:06 -0700 "Shilimkar, Santosh"
> >> > <santosh.shilimkar@xxxxxx> 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: signature.asc
Description: PGP signature