Re: [PATCH 8/8] gpio/omap: fix missing check in *_runtime_suspend()
On Fri, May 18, 2012 at 5:26 AM, Kevin Hilman khil...@ti.com wrote: Tony Lindgren t...@atomide.com writes: * Kevin Hilman khil...@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 khil...@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 t...@atomide.com Cc: Tarun Kanti DebBarma tarun.ka...@ti.com Signed-off-by: Kevin Hilman khil...@ti.com --- Thanks for the Fix Kevin. Acked-by: Santosh Shilimkar santosh.shilim...@ti.com -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] gpio/omap: fix missing check in *_runtime_suspend()
On Fri, May 18, 2012 at 3:51 AM, Kevin Hilman khil...@ti.com wrote:
Tarun, Santosh,
Tarun Kanti DebBarma tarun.ka...@ti.com 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 khil...@ti.com
Cc: Tony Lindgren t...@atomide.com
Cc: Santosh Shilimkar santosh.shilim...@ti.com
Cc: Cousson, Benoit b-cous...@ti.com
Cc: Grant Likely grant.lik...@secretlab.ca
Signed-off-by: Tarun Kanti DebBarma tarun.ka...@ti.com
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.ka...@ti.com
---
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 khil...@ti.com
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
Re: [PATCH 8/8] gpio/omap: fix missing check in *_runtime_suspend()
Grant Likely grant.lik...@secretlab.ca writes: 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. Yes. Do you have any other omap patches? Do you want to send me a pull req? We just found one more fix needed. I'm collecting the acks/tested-bys and will have a branch for you shortly. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] gpio/omap: fix missing check in *_runtime_suspend()
Tarun, Santosh,
Tarun Kanti DebBarma tarun.ka...@ti.com 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 khil...@ti.com
Cc: Tony Lindgren t...@atomide.com
Cc: Santosh Shilimkar santosh.shilim...@ti.com
Cc: Cousson, Benoit b-cous...@ti.com
Cc: Grant Likely grant.lik...@secretlab.ca
Signed-off-by: Tarun Kanti DebBarma tarun.ka...@ti.com
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?
---
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 khil...@ti.com
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.ka...@ti.com
Cc: Santosh Shilimkar santosh.shilim...@ti.com
Signed-off-by: Kevin Hilman
Re: [PATCH 8/8] gpio/omap: fix missing check in *_runtime_suspend()
* Kevin Hilman khil...@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. 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. 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? Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] gpio/omap: fix missing check in *_runtime_suspend()
On Thu, 17 May 2012 15:21:07 -0700, Kevin Hilman khil...@ti.com wrote:
Tarun, Santosh,
Tarun Kanti DebBarma tarun.ka...@ti.com 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 khil...@ti.com
Cc: Tony Lindgren t...@atomide.com
Cc: Santosh Shilimkar santosh.shilim...@ti.com
Cc: Cousson, Benoit b-cous...@ti.com
Cc: Grant Likely grant.lik...@secretlab.ca
Signed-off-by: Tarun Kanti DebBarma tarun.ka...@ti.com
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?
---
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.
Yes. Do you have any other omap patches? Do you want to send me a
pull req?
g.
Kevin
From 83874df8e0dd78a3609f5ba81d608df7217fd212 Mon Sep 17 00:00:00 2001
From: Kevin Hilman khil...@ti.com
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
Re: [PATCH 8/8] gpio/omap: fix missing check in *_runtime_suspend()
Tony Lindgren t...@atomide.com writes:
* Kevin Hilman khil...@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 khil...@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 t...@atomide.com
Cc: Santosh Shilimkar santosh.shilim...@ti.com
Cc: Tarun Kanti DebBarma tarun.ka...@ti.com
Signed-off-by: Kevin Hilman khil...@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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] gpio/omap: fix missing check in *_runtime_suspend()
* Kevin Hilman khil...@ti.com [120517 17:00]: 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.) Thanks yes having both of your fixes applied fixes the issues I was seeing, so for both: Tested-by: Tony Lindgren t...@atomide.com 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. Makes sense for why it only appears on some boards. In the mean time, they're availble in my for_3.5/fixes/gpio-2 branch. I'll merge that also into l-o master for some more testing before the merge window. Regards, Tony -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] gpio/omap: fix missing check in *_runtime_suspend()
On Fri, May 18, 2012 at 5:26 AM, Kevin Hilman khil...@ti.com wrote:
Tony Lindgren t...@atomide.com writes:
* Kevin Hilman khil...@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 khil...@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 t...@atomide.com
Cc: Santosh Shilimkar santosh.shilim...@ti.com
Cc: Tarun Kanti DebBarma tarun.ka...@ti.com
Signed-off-by: Kevin Hilman khil...@ti.com
Acked-by: Tarun Kanti DebBarma tarun.ka...@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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] gpio/omap: fix missing check in *_runtime_suspend()
On Fri, May 18, 2012 at 3:51 AM, Kevin Hilman khil...@ti.com wrote:
Tarun, Santosh,
Tarun Kanti DebBarma tarun.ka...@ti.com 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 khil...@ti.com
Cc: Tony Lindgren t...@atomide.com
Cc: Santosh Shilimkar santosh.shilim...@ti.com
Cc: Cousson, Benoit b-cous...@ti.com
Cc: Grant Likely grant.lik...@secretlab.ca
Signed-off-by: Tarun Kanti DebBarma tarun.ka...@ti.com
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?
---
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 khil...@ti.com
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
Re: [PATCH 8/8] gpio/omap: fix missing check in *_runtime_suspend()
On Friday 27 April 2012 07:43 PM, Tarun Kanti DebBarma wrote: 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 khil...@ti.com Cc: Tony Lindgren t...@atomide.com Cc: Santosh Shilimkar santosh.shilim...@ti.com Cc: Cousson, Benoit b-cous...@ti.com Cc: Grant Likely grant.lik...@secretlab.ca Signed-off-by: Tarun Kanti DebBarma tarun.ka...@ti.com --- Looks good. Reviewed-by: Santosh Shilimkar santosh.shilim...@ti.com -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 8/8] gpio/omap: fix missing check in *_runtime_suspend()
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 khil...@ti.com
Cc: Tony Lindgren t...@atomide.com
Cc: Santosh Shilimkar santosh.shilim...@ti.com
Cc: Cousson, Benoit b-cous...@ti.com
Cc: Grant Likely grant.lik...@secretlab.ca
Signed-off-by: Tarun Kanti DebBarma tarun.ka...@ti.com
---
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;
- }
-
if (bank-get_context_loss_count) {
context_lost_cnt_after =
bank-get_context_loss_count(bank-dev);
@@ -1252,6 +1250,11 @@ static int omap_gpio_runtime_resume(struct device *dev)
}
}
+ if (!bank-workaround_enabled) {
+ spin_unlock_irqrestore(bank-lock, flags);
+ return 0;
+ }
+
__raw_writel(bank-context.fallingdetect,
bank-base + bank-regs-fallingdetect);
__raw_writel(bank-context.risingdetect,
--
1.7.0.4
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
