[PATCH 8/8] gpio/omap: fix missing check in *_runtime_suspend()

[Shilimkar, Santosh]

On Fri, May 18, 2012 at 5:26 AM, Kevin Hilman <khilman at ti.com> wrote:
> Tony Lindgren <tony at atomide.com> writes:
>
>> * Kevin Hilman <khilman at ti.com> [120517 15:29]:
>>>
>>> I just noticed that this patch has caused some strange problems, notably
>>> with the GPIO IRQ used by smsc911x NIC (Overo, Zoom3, 2430SDP, etc. etc.)
>>>
>>> The patch itself is OK, but it has exposed a bug in other parts of the
>>> context restore path that was previously hidden.
>>>
>>> We seem to have been finding lots of GPIO bugs by just testing the
>>> network chips with GPIO IRQs.  Can I suggest that a platform with a GPIO
>>> IRQ NIC be added to the test platforms you're using?
>>
>> Yes considering the breakage it's pretty obvious the original series was
>> never properly tested on omaps.
>
> Agreed.
>
Actually OMAP4 network interface as well uses the GPIO as a interrupt line but
that didn't show the issue. But I agree more and more test scenario's are needed
for infrastructure components like GPIO/DMA/TImer because of their multiple
types of usages.

>> Regarding this fix, using gpio/next + this patch fixes nfsroot for 2430sdp,
>> and gets zoom3 nfsroot booting going a bit better.
>>
>> However, at least on zoom3 nfsroot now takes several _minutes_ to get to
>> login: with gpio/next + this patch. The system is totally unusable.
>>
>> It seems that the GPIO interrupt wake-up events are not properly working
>> now?
>>
>> Reverting the "gpio/omap: fix missing check in *_runtime_suspend()"
>> patch seems to fix the issue.
>
> Argh, then $SUBJECT patch here has caused brokeness in multiple ways.
> It managed to break both runtime suspend and runtime resume at the same
> time. :(
>
> The change added by this patch to runtime_suspend effectively disables
> the fix I did in 68942edb09 (gpio/omap: fix wakeups on level-triggered GPIOs)
> causing the sluggish network problems to reappear, since that GPIO IRQ
> is no longer causing wakeups.
>
That's pretty bad.

> Simple fix is below, which just moves the check added in $SUBJECT patch
> below the workaround for the edge/level fix.  Can you confirm it works
> on Zoom3 (applies on gpio/next + my previous fix.)
>
> I didn't notice the same problem on Overo, but I guess it's because
> Overo had some enabled non-wakeup GPIOs in the same bank, so it didn't
> bypass the level-triggered IRQ fix.
>
>>> Subject: [PATCH] gpio/omap: fix broken context restore for non-OFF mode
>>>  transitions
>>>
>>> The fix in commit 1b12870 (gpio/omap: fix missing check in
>>> *_runtime_suspend()) exposed another bug in the context restore path.
>>
>> Kevin, looks like commit 1b12870 does not exist in gpio/next?
>
> Will update the changelog.
>
> Because of this new problem, I have to add the patch below too, so
> I'll get them both queued up for Grant
>
> In the mean time, they're availble in my for_3.5/fixes/gpio-2 branch.
>
> Kevin
>
> [1]
> From afb4f0836dc3c432aa999fc98a80bf75e1481e04 Mon Sep 17 00:00:00 2001
> From: Kevin Hilman <khilman at ti.com>
> Date: Thu, 17 May 2012 16:42:16 -0700
> Subject: [PATCH] gpio/omap: (re)fix wakeups on level-triggered GPIOs
>
> commit 1b1287032 (gpio/omap: fix missing check in *_runtime_suspend())
> broke wakeups on level-triggered GPIOs by adding the enabled
> non-wakeup GPIO check before the workaround that enables wakeups
> on level-triggered IRQs, effectively disabling that workaround.
>
> To fix, move the enabled non-wakeup GPIO check after the
> level-triggered IRQ workaround.
>
> Reported-by: Tony Lindgren <tony at atomide.com>
>
> Cc: Tarun Kanti DebBarma <tarun.kanti at ti.com>
> Signed-off-by: Kevin Hilman <khilman at ti.com>
> ---
Thanks for the Fix Kevin.
Acked-by: Santosh Shilimkar <santosh.shilimkar at ti.com>