On Fri, May 18, 2012 at 3:51 AM, Kevin Hilman <khilman@xxxxxx> wrote:
> Tarun, Santosh,
>
> Tarun Kanti DebBarma <tarun.kanti@xxxxxx> writes:
>
>> We do checking for bank->enabled_non_wakeup_gpios in order
>> to skip redundant operations. Somehow, the check got missed
>> while doing the cleanup series.
>>
>> Just to make sure that we do context restore correctly in
>> *_runtime_resume(), the bank->workaround_enabled check is
>> moved after context restore. Otherwise, it would prevent
>> context restore when bank->enabled_non_wakeup_gpios is 0.
>>
>> Cc: Kevin Hilman <khilman@xxxxxx>
>> Cc: Tony Lindgren <tony@xxxxxxxxxxx>
>> Cc: Santosh Shilimkar <santosh.shilimkar@xxxxxx>
>> Cc: Cousson, Benoit <b-cousson@xxxxxx>
>> Cc: Grant Likely <grant.likely@xxxxxxxxxxxx>
>> Signed-off-by: Tarun Kanti DebBarma <tarun.kanti@xxxxxx>
>
> 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?
Able to reproduce the problem. Tested on Zoom3 by enabling NFS.
Verified the time difference for the filesystem to boot. Without your
fixes it takes
around 60 seconds while with the fix it takes less than 7 seconds.
Tested-by: Tarun Kanti DebBarma <tarun.kanti@xxxxxx>
>
>> ---
>> drivers/gpio/gpio-omap.c | 13 ++++++++-----
>> 1 files changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
>> index d238f84..59a4af1 100644
>> --- a/drivers/gpio/gpio-omap.c
>> +++ b/drivers/gpio/gpio-omap.c
>> @@ -1160,6 +1160,9 @@ 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.
>> *
>> @@ -1235,11 +1238,6 @@ static int omap_gpio_runtime_resume(struct device *dev)
>> __raw_writel(bank->context.risingdetect,
>> bank->base + bank->regs->risingdetect);
>>
>> - if (!bank->workaround_enabled) {
>> - spin_unlock_irqrestore(&bank->lock, flags);
>> - return 0;
>> - }
>> -
>
> My moving this below, you exposed some buggy code that can now get
> executed in non-OFF mode, wherease before $SUBJECT patch, it would never
> be exectued in non-OFF mode.
>
>> if (bank->get_context_loss_count) {
>> context_lost_cnt_after =
>> bank->get_context_loss_count(bank->dev);
>
> ...right below this line, we have:
>
> if (context_lost_cnt_after != bank->context_loss_count ||
> !context_lost_cnt_after) {
> omap_gpio_restore_context(bank);
>
> While we've never hit off-mode, context_lost_cnt_after will always be
> zero. However, becasue of the !context_lost_cnt_after check there, we
> will *always* restore the bank context, even though context has never
> been lost.
>
> I have no idea why that !context_lost_cnt_after check is there, but
> clearly it is wrong. Now that you moved the 'workraound_enabled' check
> below this section, as long as off-mode is disabled, the some GPIO
> context will be restored here on *every* runtime PM transition.
>
> The patch below fixes the problem.
>
> Grant, after Tarun/Santosh have a chance to review/ack this, can you
> still get this in for 3.5? If not, getting it into -rc should be fine.
>
> Kevin
>
>
> From 83874df8e0dd78a3609f5ba81d608df7217fd212 Mon Sep 17 00:00:00 2001
> From: Kevin Hilman <khilman@xxxxxx>
> Date: Thu, 17 May 2012 14:52:56 -0700
> 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.
>
> Currently, the per-bank context restore happens whenever the context
> loss count is different in runtime suspend and runtime resume *and*
> whenever the per-bank contex_loss_count == 0:
>
> if (context_lost_cnt_after != bank->context_loss_count ||
> !context_lost_cnt_after) {
> omap_gpio_restore_context(bank);
>
> Restoring context when the context_lost_cnt_after == 0 is clearly
> wrong, since this will be true until the first off-mode transition
> (which could be never, if off-mode is never enabled.) This check
> causes the context to be restored on *every* runtime PM transition.
>
> Before commit 1b12870 (gpio/omap: fix missing check in
> *_runtime_suspend()), this code was never executed in non-OFF mode, so
> there were never spurious context restores happening. After that
> change though, spurious context restores could happen.
>
> To fix, simply remove the !context_lost_cnt_after check. It is not
> needed.
>
> This bug was found when noticing that the smc911x NIC on 3530/Overo
> was not working, and git bisect tracked it down to this patch. It
> seems that the spurious context restore was causing the smsc911x to
> not be properly probed on this platform.
>
> Cc: Tarun Kanti DebBarma <tarun.kanti@xxxxxx>
> Cc: Santosh Shilimkar <santosh.shilimkar@xxxxxx>
> Signed-off-by: Kevin Hilman <khilman@xxxxxx>
> ---
> drivers/gpio/gpio-omap.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 9b71f04..b570a6a 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -1238,8 +1238,7 @@ static int omap_gpio_runtime_resume(struct device *dev)
> if (bank->get_context_loss_count) {
> context_lost_cnt_after =
> bank->get_context_loss_count(bank->dev);
> - if (context_lost_cnt_after != bank->context_loss_count ||
> - !context_lost_cnt_after) {
> + if (context_lost_cnt_after != bank->context_loss_count) {
> omap_gpio_restore_context(bank);
> } else {
> spin_unlock_irqrestore(&bank->lock, flags);
> --
> 1.7.9.2
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
