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

[tarun.kanti@ti.com (DebBarma, Tarun Kanti)]

On Fri, May 18, 2012 at 5:26 AM, Kevin Hilman <khilman@ti.com> wrote:
> Tony Lindgren <tony@atomide.com> writes:
>
>> * Kevin Hilman <khilman@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.
>
>> 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.
>
> 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@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@atomide.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Tarun Kanti DebBarma <tarun.kanti@ti.com>
> Signed-off-by: Kevin Hilman <khilman@ti.com>

Acked-by: Tarun Kanti DebBarma <tarun.kanti@ti.com>

> ---
> ?drivers/gpio/gpio-omap.c | ? ?6 +++---
> ?1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index b570a6a..c4ed172 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -1157,9 +1157,6 @@ static int omap_gpio_runtime_suspend(struct device *dev)
>
> ? ? ? ?spin_lock_irqsave(&bank->lock, flags);
>
> - ? ? ? if (!bank->enabled_non_wakeup_gpios)
> - ? ? ? ? ? ? ? goto update_gpio_context_count;
> -
> ? ? ? ?/*
> ? ? ? ? * Only edges can generate a wakeup event to the PRCM.
> ? ? ? ? *
> @@ -1180,6 +1177,9 @@ static int omap_gpio_runtime_suspend(struct device *dev)
> ? ? ? ? ? ? ? ?__raw_writel(wake_hi | bank->context.risingdetect,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? bank->base + bank->regs->risingdetect);
>
> + ? ? ? if (!bank->enabled_non_wakeup_gpios)
> + ? ? ? ? ? ? ? goto update_gpio_context_count;
> +
> ? ? ? ?if (bank->power_mode != OFF_MODE) {
> ? ? ? ? ? ? ? ?bank->power_mode = 0;
> ? ? ? ? ? ? ? ?goto update_gpio_context_count;
> --
> 1.7.9.2
>