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

[Kevin Hilman <khilman@deeprootsystems.com>]

"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