Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.
Grant Likely grant.lik...@secretlab.ca writes: On Fri, 14 Dec 2012 18:05:53 +1100, NeilBrown ne...@suse.de wrote: On Mon, 10 Sep 2012 10:57:07 -0700 Kevin Hilman khil...@deeprootsystems.com wrote: OK thanks, I'll queue this up for v3.6-rc as this should qualify as a regression. I don't think this did actually get queued. At least I'm still seeing the bug in 3.7 and I cannot see a patch in the git history that looks right. But then I don't remember what we ended up with - it was 3 months ago!!! This is the last thing I can find in my email history - it still seems to apply and still seems to work. NeilBrown Kevin, let me know if I need to do anything here. Since you might have it in one of you're trees, I'm not going to do anything unless I hear from you. oops, just stumbled across this one. Sorry for the lag. Grant, feel free to apply. Seems this one never made it into my queue. 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] OMAP GPIO - don't wake from suspend unless requested.
On Fri, 14 Dec 2012 18:05:53 +1100, NeilBrown ne...@suse.de wrote: On Mon, 10 Sep 2012 10:57:07 -0700 Kevin Hilman khil...@deeprootsystems.com wrote: OK thanks, I'll queue this up for v3.6-rc as this should qualify as a regression. I don't think this did actually get queued. At least I'm still seeing the bug in 3.7 and I cannot see a patch in the git history that looks right. But then I don't remember what we ended up with - it was 3 months ago!!! This is the last thing I can find in my email history - it still seems to apply and still seems to work. NeilBrown Kevin, let me know if I need to do anything here. Since you might have it in one of you're trees, I'm not going to do anything unless I hear from you. g. -- 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] OMAP GPIO - don't wake from suspend unless requested.
On Fri, 2012-12-14 at 18:05 +1100, NeilBrown wrote:
On Mon, 10 Sep 2012 10:57:07 -0700 Kevin Hilman khil...@deeprootsystems.com
wrote:
OK thanks, I'll queue this up for v3.6-rc as this should qualify as a
regression.
I don't think this did actually get queued. At least I'm still seeing the
bug in 3.7 and I cannot see a patch in the git history that looks right.
But then I don't remember what we ended up with - it was 3 months ago!!!
This is the last thing I can find in my email history - it still seems to
apply and still seems to work.
NeilBrown
From: NeilBrown ne...@suse.de
Date: Mon, 10 Sep 2012 14:09:06 +1000
Subject: [PATCH] OMAP GPIO - don't wake from suspend unless requested.
Current kernel will wake from suspend on an event an any active
GPIO event if enable_irq_wake() wasn't called.
Should it read this way Current kernel will wake from suspend on any
active gpio event 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
in which case
recognised.
2/ while suspended the GPIO interrupt should wake from suspend if and
only if irq_wake as been enabled.
has 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.
Cc: Kevin Hilman khil...@ti.com
Cc: Tony Lindgren t...@atomide.com
Cc: Santosh Shilimkar santosh.shilim...@ti.com
Cc: Benoit Cousson b-cous...@ti.com
Cc: Grant Likely grant.lik...@secretlab.ca
Cc: Tarun Kanti DebBarma tarun.ka...@ti.com
Cc: Felipe Balbi ba...@ti.com
Cc: Govindraj.R govindraj.r...@ti.com
Signed-off-by: NeilBrown ne...@suse.de
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 4fbc208..79e1340 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -57,6 +57,7 @@ struct gpio_bank {
u16 irq;
int irq_base;
struct irq_domain *domain;
+ u32 suspend_wakeup;
u32 non_wakeup_gpios;
u32 enabled_non_wakeup_gpios;
struct gpio_regs context;
@@ -522,11 +523,9 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int
gpio, int enable)
spin_lock_irqsave(bank-lock, flags);
if (enable)
- bank-context.wake_en |= gpio_bit;
+ bank-suspend_wakeup |= gpio_bit;
else
- bank-context.wake_en = ~gpio_bit;
-
- __raw_writel(bank-context.wake_en, bank-base + bank-regs-wkup_en);
+ bank-suspend_wakeup = ~gpio_bit;
spin_unlock_irqrestore(bank-lock, flags);
return 0;
@@ -1157,6 +1156,49 @@ static int __devinit omap_gpio_probe(struct
platform_device *pdev)
#ifdef CONFIG_ARCH_OMAP2PLUS
#if defined(CONFIG_PM_RUNTIME)
+
+#if defined(CONFIG_PM_SLEEP)
+static int omap_gpio_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct gpio_bank *bank = platform_get_drvdata(pdev);
+ void __iomem *base = bank-base;
+ unsigned long flags;
+
+ if (!bank-mod_usage ||
+ !bank-regs-wkup_en ||
+ !bank-context.wake_en)
+ return 0;
+
+ spin_lock_irqsave(bank-lock, flags);
+ _gpio_rmw(base, bank-regs-wkup_en, 0x, 0);
+ _gpio_rmw(base, bank-regs-wkup_en, bank-suspend_wakeup, 1);
+ spin_unlock_irqrestore(bank-lock, flags);
+
+ return 0;
+}
+
+static int omap_gpio_resume(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct gpio_bank *bank = platform_get_drvdata(pdev);
+ void __iomem *base = bank-base;
+ unsigned long flags;
+
+ if (!bank-mod_usage ||
+ !bank-regs-wkup_en ||
+ !bank-context.wake_en)
+ return 0;
+
+ spin_lock_irqsave(bank-lock, flags);
+ _gpio_rmw(base, bank-regs-wkup_en, 0x, 0);
+ _gpio_rmw(base, bank-regs-wkup_en, bank-context.wake_en, 1);
+ spin_unlock_irqrestore(bank-lock, flags);
+
+ return 0;
+}
+#endif /* CONFIG_PM_SLEEP */
+
static void omap_gpio_restore_context(struct gpio_bank *bank);
static int omap_gpio_runtime_suspend(struct device *dev)
@@ -1386,11 +1428,14 @@ static void omap_gpio_restore_context(struct
gpio_bank *bank)
}
#endif /* CONFIG_PM_RUNTIME */
#else
+#define omap_gpio_suspend NULL
+#define omap_gpio_resume
Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.
On Mon, 10 Sep 2012 10:57:07 -0700 Kevin Hilman khil...@deeprootsystems.com
wrote:
OK thanks, I'll queue this up for v3.6-rc as this should qualify as a
regression.
I don't think this did actually get queued. At least I'm still seeing the
bug in 3.7 and I cannot see a patch in the git history that looks right.
But then I don't remember what we ended up with - it was 3 months ago!!!
This is the last thing I can find in my email history - it still seems to
apply and still seems to work.
NeilBrown
From: NeilBrown ne...@suse.de
Date: Mon, 10 Sep 2012 14:09:06 +1000
Subject: [PATCH] OMAP GPIO - don't wake from suspend unless requested.
Current kernel will wake from suspend on an event an any active
GPIO event 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.
Cc: Kevin Hilman khil...@ti.com
Cc: Tony Lindgren t...@atomide.com
Cc: Santosh Shilimkar santosh.shilim...@ti.com
Cc: Benoit Cousson b-cous...@ti.com
Cc: Grant Likely grant.lik...@secretlab.ca
Cc: Tarun Kanti DebBarma tarun.ka...@ti.com
Cc: Felipe Balbi ba...@ti.com
Cc: Govindraj.R govindraj.r...@ti.com
Signed-off-by: NeilBrown ne...@suse.de
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 4fbc208..79e1340 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -57,6 +57,7 @@ struct gpio_bank {
u16 irq;
int irq_base;
struct irq_domain *domain;
+ u32 suspend_wakeup;
u32 non_wakeup_gpios;
u32 enabled_non_wakeup_gpios;
struct gpio_regs context;
@@ -522,11 +523,9 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int
gpio, int enable)
spin_lock_irqsave(bank-lock, flags);
if (enable)
- bank-context.wake_en |= gpio_bit;
+ bank-suspend_wakeup |= gpio_bit;
else
- bank-context.wake_en = ~gpio_bit;
-
- __raw_writel(bank-context.wake_en, bank-base + bank-regs-wkup_en);
+ bank-suspend_wakeup = ~gpio_bit;
spin_unlock_irqrestore(bank-lock, flags);
return 0;
@@ -1157,6 +1156,49 @@ static int __devinit omap_gpio_probe(struct
platform_device *pdev)
#ifdef CONFIG_ARCH_OMAP2PLUS
#if defined(CONFIG_PM_RUNTIME)
+
+#if defined(CONFIG_PM_SLEEP)
+static int omap_gpio_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct gpio_bank *bank = platform_get_drvdata(pdev);
+ void __iomem *base = bank-base;
+ unsigned long flags;
+
+ if (!bank-mod_usage ||
+ !bank-regs-wkup_en ||
+ !bank-context.wake_en)
+ return 0;
+
+ spin_lock_irqsave(bank-lock, flags);
+ _gpio_rmw(base, bank-regs-wkup_en, 0x, 0);
+ _gpio_rmw(base, bank-regs-wkup_en, bank-suspend_wakeup, 1);
+ spin_unlock_irqrestore(bank-lock, flags);
+
+ return 0;
+}
+
+static int omap_gpio_resume(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct gpio_bank *bank = platform_get_drvdata(pdev);
+ void __iomem *base = bank-base;
+ unsigned long flags;
+
+ if (!bank-mod_usage ||
+ !bank-regs-wkup_en ||
+ !bank-context.wake_en)
+ return 0;
+
+ spin_lock_irqsave(bank-lock, flags);
+ _gpio_rmw(base, bank-regs-wkup_en, 0x, 0);
+ _gpio_rmw(base, bank-regs-wkup_en, bank-context.wake_en, 1);
+ spin_unlock_irqrestore(bank-lock, flags);
+
+ return 0;
+}
+#endif /* CONFIG_PM_SLEEP */
+
static void omap_gpio_restore_context(struct gpio_bank *bank);
static int omap_gpio_runtime_suspend(struct device *dev)
@@ -1386,11 +1428,14 @@ static void omap_gpio_restore_context(struct gpio_bank
*bank)
}
#endif /* CONFIG_PM_RUNTIME */
#else
+#define omap_gpio_suspend NULL
+#define omap_gpio_resume NULL
#define omap_gpio_runtime_suspend NULL
#define omap_gpio_runtime_resume NULL
#endif
static const struct dev_pm_ops gpio_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(omap_gpio_suspend, omap_gpio_resume)
SET_RUNTIME_PM_OPS(omap_gpio_runtime_suspend
Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.
On Thu, 6 Sep 2012 16:26:06 +0300 Felipe Balbi ba...@ti.com wrote:
Hi,
On Thu, Sep 06, 2012 at 05:02:45PM +1000, NeilBrown wrote:
On Thu, 6 Sep 2012 11:18:09 +0530 Shilimkar, Santosh
santosh.shilim...@ti.com wrote:
On Thu, Sep 6, 2012 at 8:35 AM, NeilBrown ne...@suse.de wrote:
On Mon, 3 Sep 2012 22:59:06 -0700 Shilimkar, Santosh
santosh.shilim...@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 ne...@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.
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
Cc: Tarun Kanti DebBarma tarun.ka...@ti.com
Cc: Felipe Balbi ba...@ti.com
Cc: Govindraj.R govindraj.r...@ti.com
Signed-off-by: NeilBrown ne...@suse.de
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 4fbc208..fdbad70 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -57,6 +57,7 @@ struct gpio_bank {
u16 irq;
int irq_base;
struct irq_domain *domain;
+ u32 suspend_wakeup;
u32 non_wakeup_gpios;
u32 enabled_non_wakeup_gpios;
struct gpio_regs context;
@@ -522,11 +523,12 @@ static int _set_gpio_wakeup(struct gpio_bank *bank,
int gpio, int enable)
spin_lock_irqsave(bank-lock, flags);
if (enable)
- bank-context.wake_en |= gpio_bit;
+ bank-suspend_wakeup |= gpio_bit;
else
- bank-context.wake_en = ~gpio_bit;
+ bank-suspend_wakeup = ~gpio_bit;
- __raw_writel(bank-context.wake_en, bank-base + bank-regs-wkup_en);
+ if (!bank-loses_context)
+ __raw_writel(bank-suspend_wakeup, bank-base +
bank-regs-wkup_en);
spin_unlock_irqrestore(bank-lock, flags);
return 0;
@@ -1157,6 +1159,51 @@ static int __devinit omap_gpio_probe(struct
platform_device *pdev)
#ifdef CONFIG_ARCH_OMAP2PLUS
#if defined(CONFIG_PM_RUNTIME)
+
+#if defined(CONFIG_PM_SLEEP)
+static int omap_gpio_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct gpio_bank *bank = platform_get_drvdata(pdev);
+ void __iomem *base = bank-base;
+ unsigned long flags;
+
+ if (!bank-mod_usage || !bank-loses_context)
+ return 0;
+
+ if (!bank-regs-wkup_en || !bank-context.wake_en)
+ return 0;
+
+ spin_lock_irqsave(bank-lock, flags);
shouldn't you be using _noirq methods instead ? Then this would become a
normal spin_lock()/spin_unlock().
I don't think
Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.
Shilimkar, Santosh santosh.shilim...@ti.com writes: On Sat, Sep 8, 2012 at 3:07 AM, Kevin Hilman khil...@deeprootsystems.com wrote: Hi Neil, NeilBrown ne...@suse.de writes: On Thu, 6 Sep 2012 11:18:09 +0530 Shilimkar, Santosh santosh.shilim...@ti.com wrote: On Thu, Sep 6, 2012 at 8:35 AM, NeilBrown ne...@suse.de wrote: On Mon, 3 Sep 2012 22:59:06 -0700 Shilimkar, Santosh santosh.shilim...@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 ne...@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 -- 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] OMAP GPIO - don't wake from suspend unless requested.
NeilBrown ne...@suse.de writes:
[...]
Yes, I see that now. Thanks.
follow patch folds those to fixes in.
NeilBrown
From bd9d5e9f8742c9cdc795e3d9b895f74defddb6d9 Mon Sep 17 00:00:00 2001
From: NeilBrown ne...@suse.de
Date: Mon, 10 Sep 2012 14:09:06 +1000
Subject: [PATCH] OMAP GPIO - don't wake from suspend unless requested.
Current kernel will wake from suspend on an event an any active
GPIO event 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.
Cc: Kevin Hilman khil...@ti.com
Cc: Tony Lindgren t...@atomide.com
Cc: Santosh Shilimkar santosh.shilim...@ti.com
Cc: Benoit Cousson b-cous...@ti.com
Cc: Grant Likely grant.lik...@secretlab.ca
Cc: Tarun Kanti DebBarma tarun.ka...@ti.com
Cc: Felipe Balbi ba...@ti.com
Cc: Govindraj.R govindraj.r...@ti.com
Signed-off-by: NeilBrown ne...@suse.de
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 4fbc208..79e1340 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -57,6 +57,7 @@ struct gpio_bank {
u16 irq;
int irq_base;
struct irq_domain *domain;
+ u32 suspend_wakeup;
u32 non_wakeup_gpios;
u32 enabled_non_wakeup_gpios;
struct gpio_regs context;
@@ -522,11 +523,9 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int
gpio, int enable)
spin_lock_irqsave(bank-lock, flags);
if (enable)
- bank-context.wake_en |= gpio_bit;
+ bank-suspend_wakeup |= gpio_bit;
else
- bank-context.wake_en = ~gpio_bit;
-
- __raw_writel(bank-context.wake_en, bank-base + bank-regs-wkup_en);
+ bank-suspend_wakeup = ~gpio_bit;
spin_unlock_irqrestore(bank-lock, flags);
return 0;
@@ -1157,6 +1156,49 @@ static int __devinit omap_gpio_probe(struct
platform_device *pdev)
#ifdef CONFIG_ARCH_OMAP2PLUS
#if defined(CONFIG_PM_RUNTIME)
+
+#if defined(CONFIG_PM_SLEEP)
+static int omap_gpio_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct gpio_bank *bank = platform_get_drvdata(pdev);
+ void __iomem *base = bank-base;
+ unsigned long flags;
+
+ if (!bank-mod_usage ||
+ !bank-regs-wkup_en ||
+ !bank-context.wake_en)
nit: the context-wake_en check isn't really needed here.
+ return 0;
+
+ spin_lock_irqsave(bank-lock, flags);
+ _gpio_rmw(base, bank-regs-wkup_en, 0x, 0);
+ _gpio_rmw(base, bank-regs-wkup_en, bank-suspend_wakeup, 1);
I know we had the duplicate read/modify/writes here before, but that causes
a bunch of unnecessary register accesses. Simply writing
bank-suspend_wakeup should suffice here...
+ spin_unlock_irqrestore(bank-lock, flags);
+
+ return 0;
+}
+
+static int omap_gpio_resume(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct gpio_bank *bank = platform_get_drvdata(pdev);
+ void __iomem *base = bank-base;
+ unsigned long flags;
+
+ if (!bank-mod_usage ||
+ !bank-regs-wkup_en ||
+ !bank-context.wake_en)
+ return 0;
+
+ spin_lock_irqsave(bank-lock, flags);
+ _gpio_rmw(base, bank-regs-wkup_en, 0x, 0);
+ _gpio_rmw(base, bank-regs-wkup_en, bank-context.wake_en, 1);
...similarily, simply writing context.wake_en should suffice here (after
removing the check for non-zero wake_en above so that we're sure that
the setting of suspend_wakeup is undone.)
Kevin
+ spin_unlock_irqrestore(bank-lock, flags);
+
+ return 0;
+}
+#endif /* CONFIG_PM_SLEEP */
+
static void omap_gpio_restore_context(struct gpio_bank *bank);
static int omap_gpio_runtime_suspend(struct device *dev)
@@ -1386,11 +1428,14 @@ static void omap_gpio_restore_context(struct
gpio_bank *bank)
}
#endif /* CONFIG_PM_RUNTIME */
#else
+#define omap_gpio_suspend NULL
+#define omap_gpio_resume NULL
#define omap_gpio_runtime_suspend NULL
#define omap_gpio_runtime_resume NULL
#endif
static const
Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.
On Fri, 07 Sep 2012 14:37:16 -0700 Kevin Hilman khil...@deeprootsystems.com
wrote:
Hi Neil,
NeilBrown ne...@suse.de writes:
On Thu, 6 Sep 2012 11:18:09 +0530 Shilimkar, Santosh
santosh.shilim...@ti.com wrote:
On Thu, Sep 6, 2012 at 8:35 AM, NeilBrown ne...@suse.de wrote:
On Mon, 3 Sep 2012 22:59:06 -0700 Shilimkar, Santosh
santosh.shilim...@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 ne...@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...
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
Cc: Tarun Kanti DebBarma tarun.ka...@ti.com
Cc: Felipe Balbi ba...@ti.com
Cc: Govindraj.R govindraj.r...@ti.com
Signed-off-by: NeilBrown ne...@suse.de
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 4fbc208..fdbad70 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -57,6 +57,7 @@ struct gpio_bank {
u16 irq;
int irq_base;
struct irq_domain *domain;
+ u32 suspend_wakeup;
u32 non_wakeup_gpios;
u32 enabled_non_wakeup_gpios;
struct gpio_regs context;
@@ -522,11 +523,12 @@ static int _set_gpio_wakeup(struct gpio_bank *bank,
int gpio, int enable)
spin_lock_irqsave(bank-lock, flags);
if (enable)
- bank-context.wake_en |= gpio_bit;
+ bank-suspend_wakeup |= gpio_bit;
else
- bank-context.wake_en = ~gpio_bit;
+ bank-suspend_wakeup = ~gpio_bit;
- __raw_writel(bank-context.wake_en, bank-base + bank-regs-wkup_en);
+ if (!bank-loses_context)
+ __raw_writel(bank-suspend_wakeup, bank-base +
bank-regs-wkup_en);
This doesn't look right for bank 1 (the only one that doesn't lose
context.) If I'm not mistaken, this will overrides the idle wake_en
settings (from context.wake_en), will disable GPIO IRQ triggering for
any of the non IRQ wake-enabled GPIO IRQs in this bank...
Yes... I was clearly confused when writing that. There was a good reason at
the time, but I doesn't seem to stand up to the cold light of day..
spin_unlock_irqrestore(bank-lock, flags);
return 0;
@@ -1157,6 +1159,51 @@ static int __devinit omap_gpio_probe(struct
platform_device *pdev)
#ifdef CONFIG_ARCH_OMAP2PLUS
#if defined(CONFIG_PM_RUNTIME)
+
+#if defined(CONFIG_PM_SLEEP)
+static int omap_gpio_suspend(struct device *dev
Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.
On Sat, Sep 8, 2012 at 3:07 AM, Kevin Hilman khil...@deeprootsystems.com wrote: Hi Neil, NeilBrown ne...@suse.de writes: On Thu, 6 Sep 2012 11:18:09 +0530 Shilimkar, Santosh santosh.shilim...@ti.com wrote: On Thu, Sep 6, 2012 at 8:35 AM, NeilBrown ne...@suse.de wrote: On Mon, 3 Sep 2012 22:59:06 -0700 Shilimkar, Santosh santosh.shilim...@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 ne...@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. 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. Regards, Santosh -- 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] OMAP GPIO - don't wake from suspend unless requested.
Hi Neil,
NeilBrown ne...@suse.de writes:
On Thu, 6 Sep 2012 11:18:09 +0530 Shilimkar, Santosh
santosh.shilim...@ti.com wrote:
On Thu, Sep 6, 2012 at 8:35 AM, NeilBrown ne...@suse.de wrote:
On Mon, 3 Sep 2012 22:59:06 -0700 Shilimkar, Santosh
santosh.shilim...@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 ne...@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...
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
Cc: Tarun Kanti DebBarma tarun.ka...@ti.com
Cc: Felipe Balbi ba...@ti.com
Cc: Govindraj.R govindraj.r...@ti.com
Signed-off-by: NeilBrown ne...@suse.de
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 4fbc208..fdbad70 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -57,6 +57,7 @@ struct gpio_bank {
u16 irq;
int irq_base;
struct irq_domain *domain;
+ u32 suspend_wakeup;
u32 non_wakeup_gpios;
u32 enabled_non_wakeup_gpios;
struct gpio_regs context;
@@ -522,11 +523,12 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int
gpio, int enable)
spin_lock_irqsave(bank-lock, flags);
if (enable)
- bank-context.wake_en |= gpio_bit;
+ bank-suspend_wakeup |= gpio_bit;
else
- bank-context.wake_en = ~gpio_bit;
+ bank-suspend_wakeup = ~gpio_bit;
- __raw_writel(bank-context.wake_en, bank-base + bank-regs-wkup_en);
+ if (!bank-loses_context)
+ __raw_writel(bank-suspend_wakeup, bank-base +
bank-regs-wkup_en);
This doesn't look right for bank 1 (the only one that doesn't lose
context.) If I'm not mistaken, this will overrides the idle wake_en
settings (from context.wake_en), will disable GPIO IRQ triggering for
any of the non IRQ wake-enabled GPIO IRQs in this bank...
spin_unlock_irqrestore(bank-lock, flags);
return 0;
@@ -1157,6 +1159,51 @@ static int __devinit omap_gpio_probe(struct
platform_device *pdev)
#ifdef CONFIG_ARCH_OMAP2PLUS
#if defined(CONFIG_PM_RUNTIME)
+
+#if defined(CONFIG_PM_SLEEP)
+static int omap_gpio_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct gpio_bank *bank = platform_get_drvdata(pdev);
+ void __iomem *base = bank-base;
+ unsigned long flags;
+
+ if (!bank-mod_usage || !bank-loses_context)
+ return 0;
... probably, we just need to drop the bank-loses_context check
Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.
On Thu, Sep 6, 2012 at 12:32 PM, NeilBrown ne...@suse.de wrote: On Thu, 6 Sep 2012 11:18:09 +0530 Shilimkar, Santosh santosh.shilim...@ti.com wrote: On Thu, Sep 6, 2012 at 8:35 AM, NeilBrown ne...@suse.de wrote: On Mon, 3 Sep 2012 22:59:06 -0700 Shilimkar, Santosh santosh.shilim...@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. Sorry I though you were talking about moving the Checking wakeup interrupts bit early as discussed on the follow up of alternate suggested patch to make use of IRQCHIP_MASK_ON_SUSPEND. I think we need to fix the issue seen with ' IRQCHIP_MASK_ON_SUSPEND' patch. That is at least the expected way to manage suspend and wakeup irq masks for a IRQCHIP. Regards Santosh -- 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] OMAP GPIO - don't wake from suspend unless requested.
On Thu, 6 Sep 2012 12:57:46 +0530 Shilimkar, Santosh santosh.shilim...@ti.com wrote: On Thu, Sep 6, 2012 at 12:32 PM, NeilBrown ne...@suse.de wrote: On Thu, 6 Sep 2012 11:18:09 +0530 Shilimkar, Santosh santosh.shilim...@ti.com wrote: On Thu, Sep 6, 2012 at 8:35 AM, NeilBrown ne...@suse.de wrote: On Mon, 3 Sep 2012 22:59:06 -0700 Shilimkar, Santosh santosh.shilim...@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. Sorry I though you were talking about moving the Checking wakeup interrupts bit early as discussed on the follow up of alternate suggested patch to make use of IRQCHIP_MASK_ON_SUSPEND. I think we need to fix the issue seen with ' IRQCHIP_MASK_ON_SUSPEND' patch. That is at least the expected way to manage suspend and wakeup irq masks for a IRQCHIP. That is what I thought at first too. But when talking to Rafael he said that IRQCHIP_MASK_ON_SUSPEND was intended mainly for clock interrupts. For other less fundamental interrupts, doing the mask/unmask in suspend/resume callbacks is sufficient and simpler... and actually works. IRQCHIP_MASK_ON_SUSPEND is currently used by precisely two drivers: arch/arm/mach-omap2/omap-wakeupgen.c and drivers/mfd/pm8xxx-irq.c which suggests that it isn't widely used and quite possibly doesn't actually work in general. The pm8xxx-irq doesn't seem to do runtime pm, so maybe it manages to work for that reason. The omap-wakeupgen code is beyond my current understanding, but it seems like it might be the sort of special case that IRQCHIP_MASK_ON_SUSPEND is intended for... Maybe we need to start a new thread and pester Rafael or Thomas Gleixner to either explain what is intended for this case, or to fix IRQCHIP_MASK_ON_SUSPEND so that it can be used in general. NeilBrown signature.asc Description: PGP signature
Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.
On Thu, Sep 6, 2012 at 1:21 PM, NeilBrown ne...@suse.de wrote: On Thu, 6 Sep 2012 12:57:46 +0530 Shilimkar, Santosh santosh.shilim...@ti.com wrote: On Thu, Sep 6, 2012 at 12:32 PM, NeilBrown ne...@suse.de wrote: On Thu, 6 Sep 2012 11:18:09 +0530 Shilimkar, Santosh santosh.shilim...@ti.com wrote: On Thu, Sep 6, 2012 at 8:35 AM, NeilBrown ne...@suse.de wrote: On Mon, 3 Sep 2012 22:59:06 -0700 Shilimkar, Santosh santosh.shilim...@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. Sorry I though you were talking about moving the Checking wakeup interrupts bit early as discussed on the follow up of alternate suggested patch to make use of IRQCHIP_MASK_ON_SUSPEND. I think we need to fix the issue seen with ' IRQCHIP_MASK_ON_SUSPEND' patch. That is at least the expected way to manage suspend and wakeup irq masks for a IRQCHIP. That is what I thought at first too. But when talking to Rafael he said that IRQCHIP_MASK_ON_SUSPEND was intended mainly for clock interrupts. For other less fundamental interrupts, doing the mask/unmask in suspend/resume callbacks is sufficient and simpler... and actually works. That is not the how I undetand IRQCHIP_MASK_ON_SUSPEND use. I thought it can be used for any IRQ chip for masking or setting wakeup on interrupts lines managed by that chip for suspend. May be I am wrong. IRQCHIP_MASK_ON_SUSPEND is currently used by precisely two drivers: arch/arm/mach-omap2/omap-wakeupgen.c and drivers/mfd/pm8xxx-irq.c which suggests that it isn't widely used and quite possibly doesn't actually work in general. I have seen lot more platforms use in downstream kernels. Also seen recently patches on the linux-arm list for couple of platforms. Maybe we need to start a new thread and pester Rafael or Thomas Gleixner to either explain what is intended for this case, or to fix IRQCHIP_MASK_ON_SUSPEND so that it can be used in general. Sounds a good idea. Since you already had discussion with Rafael, probably you can describe the issue better. Regards Santosh -- 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] OMAP GPIO - don't wake from suspend unless requested.
Hi,
On Thu, Sep 06, 2012 at 05:02:45PM +1000, NeilBrown wrote:
On Thu, 6 Sep 2012 11:18:09 +0530 Shilimkar, Santosh
santosh.shilim...@ti.com wrote:
On Thu, Sep 6, 2012 at 8:35 AM, NeilBrown ne...@suse.de wrote:
On Mon, 3 Sep 2012 22:59:06 -0700 Shilimkar, Santosh
santosh.shilim...@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 ne...@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.
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
Cc: Tarun Kanti DebBarma tarun.ka...@ti.com
Cc: Felipe Balbi ba...@ti.com
Cc: Govindraj.R govindraj.r...@ti.com
Signed-off-by: NeilBrown ne...@suse.de
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 4fbc208..fdbad70 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -57,6 +57,7 @@ struct gpio_bank {
u16 irq;
int irq_base;
struct irq_domain *domain;
+ u32 suspend_wakeup;
u32 non_wakeup_gpios;
u32 enabled_non_wakeup_gpios;
struct gpio_regs context;
@@ -522,11 +523,12 @@ static int _set_gpio_wakeup(struct gpio_bank *bank, int
gpio, int enable)
spin_lock_irqsave(bank-lock, flags);
if (enable)
- bank-context.wake_en |= gpio_bit;
+ bank-suspend_wakeup |= gpio_bit;
else
- bank-context.wake_en = ~gpio_bit;
+ bank-suspend_wakeup = ~gpio_bit;
- __raw_writel(bank-context.wake_en, bank-base + bank-regs-wkup_en);
+ if (!bank-loses_context)
+ __raw_writel(bank-suspend_wakeup, bank-base +
bank-regs-wkup_en);
spin_unlock_irqrestore(bank-lock, flags);
return 0;
@@ -1157,6 +1159,51 @@ static int __devinit omap_gpio_probe(struct
platform_device *pdev)
#ifdef CONFIG_ARCH_OMAP2PLUS
#if defined(CONFIG_PM_RUNTIME)
+
+#if defined(CONFIG_PM_SLEEP)
+static int omap_gpio_suspend(struct device *dev)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ struct gpio_bank *bank = platform_get_drvdata(pdev);
+ void __iomem *base = bank-base;
+ unsigned long flags;
+
+ if (!bank-mod_usage || !bank-loses_context)
+ return 0;
+
+ if (!bank-regs-wkup_en || !bank-context.wake_en)
+ return 0;
+
+ spin_lock_irqsave(bank-lock, flags);
shouldn't you be using _noirq methods instead ? Then this would become a
normal spin_lock()/spin_unlock().
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.
On Thursday 06 September 2012 12:32 PM, NeilBrown wrote: From: NeilBrown ne...@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. Just gave it a try fixes a similar issue on my omap4 board as well. 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 Cc: Tarun Kanti DebBarma tarun.ka...@ti.com Cc: Felipe Balbi ba...@ti.com Cc: Govindraj.R govindraj.r...@ti.com Signed-off-by: NeilBrown ne...@suse.de -- 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] OMAP GPIO - don't wake from suspend unless requested.
On Mon, 3 Sep 2012 22:59:06 -0700 Shilimkar, Santosh
santosh.shilim...@ti.com wrote:
On Sun, Aug 26, 2012 at 6:29 PM, Shilimkar, Santosh
santosh.shilim...@ti.com wrote:
On Sun, Aug 26, 2012 at 3:53 PM, NeilBrown ne...@suse.de wrote:
On Sun, 26 Aug 2012 09:47:50 +0530 Shilimkar, Santosh
santosh.shilim...@ti.com wrote:
+ Jon,
On Sat, Aug 25, 2012 at 5:14 PM, NeilBrown ne...@suse.de wrote:
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.
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
Cc: Tarun Kanti DebBarma tarun.ka...@ti.com
Cc: Felipe Balbi ba...@ti.com
Cc: Govindraj.R govindraj.r...@ti.com
Signed-off-by: NeilBrown ne...@suse.de
The patch doesn't seems to be correct. At least the 2/ gets
fixed with a proper IRQCHIP flag. Can you try the patch at
end of the email and see if it helps ? Am attaching it in case
mailer damages it.
Regards
Santosh
From b8a38fc75e046f6462610e26c47c620cad850c24 Mon Sep 17 00:00:00 2001
From: Santosh Shilimkar santosh.shilim...@ti.com
Date: Sun, 26 Aug 2012 09:39:51 +0530
Subject: [PATCH] gpio: omap: Set IRQCHIP_MASK_ON_SUSPEND to mask all
non-wakeup gpio wakeups.
Set the irq chip flag IRQCHIP_MASK_ON_SUSPEND to cause the irq pm code
to mask all non-wake gpios in suspend, which will ensure the wakeup
enable
bit is not set on non-wake gpios.
Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com
---
drivers/gpio/gpio-omap.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index e6efd77..50b4c18 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -779,6 +779,7 @@ static struct irq_chip gpio_irq_chip = {
.irq_unmask = gpio_unmask_irq,
.irq_set_type = gpio_irq_type,
.irq_set_wake = gpio_wake_enable,
+ .flags = IRQCHIP_MASK_ON_SUSPEND;
};
/*-*/
No obvious damage, unless the mailer is responsible or the ';' at the end
of
the line, rather than ',' :-)
:-) That was typo.
The approach makes sense, but does actually work. Should be fixable
though.
When I try this I get:
[ 158.114440] Checking wakeup interrupts
[ 158.118408] Unhandled fault: external abort on non-linefetch (0x1028)
at 0xfb054040
[ 158.126403] Internal error: : 1028 [#1] PREEMPT ARM
[ 158.131500] Modules linked in: ipv6 g_ether hso libertas_sdio libertas
cfg80211
[ 158.139190] CPU: 0Not tainted (3.5.0-gta04-debug+ #2)
[ 158.144927] PC is at _set_gpio_triggering+0x38/0x258
[ 158.150115] LR is at gpio_mask_irq+0xac/0xc0
[ 158.154602] pc : [c01d24a0]lr : [c01d2f68]psr: 6193
[ 158.154602] sp : db521e90 ip : 0011 fp : beeecc2c
[ 158.166595] r10: c05c8ebc r9 : daa5a858 r8 : 0003
[ 158.172027] r7 : a193 r6 : r5 : fb054000 r4 : ded44e18
[ 158.178863] r3 : 0001 r2 : r1 : ded30340 r0 : 0040
[ 158.185668] Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM
Segment use
so it looks like runtime PM has turned off the iclk to the GPIO module so
that
when we try to tell it to change settings, it is no longer listening to
us.
From the crash logs it appears like that.
The Checking wakeup interrupts function happens very late in the suspend
cycle, after all the suspend_late and suspend_noirq functions have run.
Maybe it needs to be moved earlier...
No it shouldn't be moved and it is that point for lot many good
reasons. Ofcourse
this omap gpio driver crash needs to be addressed. Need to
Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.
On Thu, Sep 6, 2012 at 8:35 AM, NeilBrown ne...@suse.de wrote:
On Mon, 3 Sep 2012 22:59:06 -0700 Shilimkar, Santosh
santosh.shilim...@ti.com wrote:
On Sun, Aug 26, 2012 at 6:29 PM, Shilimkar, Santosh
santosh.shilim...@ti.com wrote:
On Sun, Aug 26, 2012 at 3:53 PM, NeilBrown ne...@suse.de wrote:
On Sun, 26 Aug 2012 09:47:50 +0530 Shilimkar, Santosh
santosh.shilim...@ti.com wrote:
+ Jon,
On Sat, Aug 25, 2012 at 5:14 PM, NeilBrown ne...@suse.de wrote:
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.
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
Cc: Tarun Kanti DebBarma tarun.ka...@ti.com
Cc: Felipe Balbi ba...@ti.com
Cc: Govindraj.R govindraj.r...@ti.com
Signed-off-by: NeilBrown ne...@suse.de
The patch doesn't seems to be correct. At least the 2/ gets
fixed with a proper IRQCHIP flag. Can you try the patch at
end of the email and see if it helps ? Am attaching it in case
mailer damages it.
Regards
Santosh
From b8a38fc75e046f6462610e26c47c620cad850c24 Mon Sep 17 00:00:00 2001
From: Santosh Shilimkar santosh.shilim...@ti.com
Date: Sun, 26 Aug 2012 09:39:51 +0530
Subject: [PATCH] gpio: omap: Set IRQCHIP_MASK_ON_SUSPEND to mask all
non-wakeup gpio wakeups.
Set the irq chip flag IRQCHIP_MASK_ON_SUSPEND to cause the irq pm code
to mask all non-wake gpios in suspend, which will ensure the wakeup
enable
bit is not set on non-wake gpios.
Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com
---
drivers/gpio/gpio-omap.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index e6efd77..50b4c18 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -779,6 +779,7 @@ static struct irq_chip gpio_irq_chip = {
.irq_unmask = gpio_unmask_irq,
.irq_set_type = gpio_irq_type,
.irq_set_wake = gpio_wake_enable,
+ .flags = IRQCHIP_MASK_ON_SUSPEND;
};
/*-*/
No obvious damage, unless the mailer is responsible or the ';' at the end
of
the line, rather than ',' :-)
:-) That was typo.
The approach makes sense, but does actually work. Should be fixable
though.
When I try this I get:
[ 158.114440] Checking wakeup interrupts
[ 158.118408] Unhandled fault: external abort on non-linefetch (0x1028)
at 0xfb054040
[ 158.126403] Internal error: : 1028 [#1] PREEMPT ARM
[ 158.131500] Modules linked in: ipv6 g_ether hso libertas_sdio libertas
cfg80211
[ 158.139190] CPU: 0Not tainted (3.5.0-gta04-debug+ #2)
[ 158.144927] PC is at _set_gpio_triggering+0x38/0x258
[ 158.150115] LR is at gpio_mask_irq+0xac/0xc0
[ 158.154602] pc : [c01d24a0]lr : [c01d2f68]psr: 6193
[ 158.154602] sp : db521e90 ip : 0011 fp : beeecc2c
[ 158.166595] r10: c05c8ebc r9 : daa5a858 r8 : 0003
[ 158.172027] r7 : a193 r6 : r5 : fb054000 r4 : ded44e18
[ 158.178863] r3 : 0001 r2 : r1 : ded30340 r0 : 0040
[ 158.185668] Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM
Segment use
so it looks like runtime PM has turned off the iclk to the GPIO module so
that
when we try to tell it to change settings, it is no longer listening to
us.
From the crash logs it appears like that.
The Checking wakeup interrupts function happens very late in the suspend
cycle, after all the suspend_late and suspend_noirq functions have run.
Maybe it needs to be moved earlier...
No it shouldn't be moved and it is that point for lot many good
reasons. Ofcourse
Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.
On Sun, Aug 26, 2012 at 6:29 PM, Shilimkar, Santosh
santosh.shilim...@ti.com wrote:
On Sun, Aug 26, 2012 at 3:53 PM, NeilBrown ne...@suse.de wrote:
On Sun, 26 Aug 2012 09:47:50 +0530 Shilimkar, Santosh
santosh.shilim...@ti.com wrote:
+ Jon,
On Sat, Aug 25, 2012 at 5:14 PM, NeilBrown ne...@suse.de wrote:
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.
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
Cc: Tarun Kanti DebBarma tarun.ka...@ti.com
Cc: Felipe Balbi ba...@ti.com
Cc: Govindraj.R govindraj.r...@ti.com
Signed-off-by: NeilBrown ne...@suse.de
The patch doesn't seems to be correct. At least the 2/ gets
fixed with a proper IRQCHIP flag. Can you try the patch at
end of the email and see if it helps ? Am attaching it in case
mailer damages it.
Regards
Santosh
From b8a38fc75e046f6462610e26c47c620cad850c24 Mon Sep 17 00:00:00 2001
From: Santosh Shilimkar santosh.shilim...@ti.com
Date: Sun, 26 Aug 2012 09:39:51 +0530
Subject: [PATCH] gpio: omap: Set IRQCHIP_MASK_ON_SUSPEND to mask all
non-wakeup gpio wakeups.
Set the irq chip flag IRQCHIP_MASK_ON_SUSPEND to cause the irq pm code
to mask all non-wake gpios in suspend, which will ensure the wakeup
enable
bit is not set on non-wake gpios.
Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com
---
drivers/gpio/gpio-omap.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index e6efd77..50b4c18 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -779,6 +779,7 @@ static struct irq_chip gpio_irq_chip = {
.irq_unmask = gpio_unmask_irq,
.irq_set_type = gpio_irq_type,
.irq_set_wake = gpio_wake_enable,
+ .flags = IRQCHIP_MASK_ON_SUSPEND;
};
/*-*/
No obvious damage, unless the mailer is responsible or the ';' at the end
of
the line, rather than ',' :-)
:-) That was typo.
The approach makes sense, but does actually work. Should be fixable
though.
When I try this I get:
[ 158.114440] Checking wakeup interrupts
[ 158.118408] Unhandled fault: external abort on non-linefetch (0x1028)
at 0xfb054040
[ 158.126403] Internal error: : 1028 [#1] PREEMPT ARM
[ 158.131500] Modules linked in: ipv6 g_ether hso libertas_sdio libertas
cfg80211
[ 158.139190] CPU: 0Not tainted (3.5.0-gta04-debug+ #2)
[ 158.144927] PC is at _set_gpio_triggering+0x38/0x258
[ 158.150115] LR is at gpio_mask_irq+0xac/0xc0
[ 158.154602] pc : [c01d24a0]lr : [c01d2f68]psr: 6193
[ 158.154602] sp : db521e90 ip : 0011 fp : beeecc2c
[ 158.166595] r10: c05c8ebc r9 : daa5a858 r8 : 0003
[ 158.172027] r7 : a193 r6 : r5 : fb054000 r4 : ded44e18
[ 158.178863] r3 : 0001 r2 : r1 : ded30340 r0 : 0040
[ 158.185668] Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM
Segment use
so it looks like runtime PM has turned off the iclk to the GPIO module so
that
when we try to tell it to change settings, it is no longer listening to
us.
From the crash logs it appears like that.
The Checking wakeup interrupts function happens very late in the suspend
cycle, after all the suspend_late and suspend_noirq functions have run.
Maybe it needs to be moved earlier...
No it shouldn't be moved and it is that point for lot many good
reasons. Ofcourse
this omap gpio driver crash needs to be addressed. Need to think bit
more on this
issue.
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.
Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.
On Sun, 26 Aug 2012 09:47:50 +0530 Shilimkar, Santosh
santosh.shilim...@ti.com wrote:
+ Jon,
On Sat, Aug 25, 2012 at 5:14 PM, NeilBrown ne...@suse.de wrote:
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.
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
Cc: Tarun Kanti DebBarma tarun.ka...@ti.com
Cc: Felipe Balbi ba...@ti.com
Cc: Govindraj.R govindraj.r...@ti.com
Signed-off-by: NeilBrown ne...@suse.de
The patch doesn't seems to be correct. At least the 2/ gets
fixed with a proper IRQCHIP flag. Can you try the patch at
end of the email and see if it helps ? Am attaching it in case
mailer damages it.
Regards
Santosh
From b8a38fc75e046f6462610e26c47c620cad850c24 Mon Sep 17 00:00:00 2001
From: Santosh Shilimkar santosh.shilim...@ti.com
Date: Sun, 26 Aug 2012 09:39:51 +0530
Subject: [PATCH] gpio: omap: Set IRQCHIP_MASK_ON_SUSPEND to mask all
non-wakeup gpio wakeups.
Set the irq chip flag IRQCHIP_MASK_ON_SUSPEND to cause the irq pm code
to mask all non-wake gpios in suspend, which will ensure the wakeup enable
bit is not set on non-wake gpios.
Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com
---
drivers/gpio/gpio-omap.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index e6efd77..50b4c18 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -779,6 +779,7 @@ static struct irq_chip gpio_irq_chip = {
.irq_unmask = gpio_unmask_irq,
.irq_set_type = gpio_irq_type,
.irq_set_wake = gpio_wake_enable,
+ .flags = IRQCHIP_MASK_ON_SUSPEND;
};
/*-*/
No obvious damage, unless the mailer is responsible or the ';' at the end of
the line, rather than ',' :-)
The approach makes sense, but does actually work. Should be fixable though.
When I try this I get:
[ 158.114440] Checking wakeup interrupts
[ 158.118408] Unhandled fault: external abort on non-linefetch (0x1028) at
0xfb054040
[ 158.126403] Internal error: : 1028 [#1] PREEMPT ARM
[ 158.131500] Modules linked in: ipv6 g_ether hso libertas_sdio libertas
cfg80211
[ 158.139190] CPU: 0Not tainted (3.5.0-gta04-debug+ #2)
[ 158.144927] PC is at _set_gpio_triggering+0x38/0x258
[ 158.150115] LR is at gpio_mask_irq+0xac/0xc0
[ 158.154602] pc : [c01d24a0]lr : [c01d2f68]psr: 6193
[ 158.154602] sp : db521e90 ip : 0011 fp : beeecc2c
[ 158.166595] r10: c05c8ebc r9 : daa5a858 r8 : 0003
[ 158.172027] r7 : a193 r6 : r5 : fb054000 r4 : ded44e18
[ 158.178863] r3 : 0001 r2 : r1 : ded30340 r0 : 0040
[ 158.185668] Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment use
so it looks like runtime PM has turned off the iclk to the GPIO module so that
when we try to tell it to change settings, it is no longer listening to us.
The Checking wakeup interrupts function happens very late in the suspend
cycle, after all the suspend_late and suspend_noirq functions have run.
Maybe it needs to be moved earlier...
Thanks,
NeilBrown
signature.asc
Description: PGP signature
Re: [PATCH] OMAP GPIO - don't wake from suspend unless requested.
On Sun, Aug 26, 2012 at 3:53 PM, NeilBrown ne...@suse.de wrote:
On Sun, 26 Aug 2012 09:47:50 +0530 Shilimkar, Santosh
santosh.shilim...@ti.com wrote:
+ Jon,
On Sat, Aug 25, 2012 at 5:14 PM, NeilBrown ne...@suse.de wrote:
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.
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
Cc: Tarun Kanti DebBarma tarun.ka...@ti.com
Cc: Felipe Balbi ba...@ti.com
Cc: Govindraj.R govindraj.r...@ti.com
Signed-off-by: NeilBrown ne...@suse.de
The patch doesn't seems to be correct. At least the 2/ gets
fixed with a proper IRQCHIP flag. Can you try the patch at
end of the email and see if it helps ? Am attaching it in case
mailer damages it.
Regards
Santosh
From b8a38fc75e046f6462610e26c47c620cad850c24 Mon Sep 17 00:00:00 2001
From: Santosh Shilimkar santosh.shilim...@ti.com
Date: Sun, 26 Aug 2012 09:39:51 +0530
Subject: [PATCH] gpio: omap: Set IRQCHIP_MASK_ON_SUSPEND to mask all
non-wakeup gpio wakeups.
Set the irq chip flag IRQCHIP_MASK_ON_SUSPEND to cause the irq pm code
to mask all non-wake gpios in suspend, which will ensure the wakeup
enable
bit is not set on non-wake gpios.
Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com
---
drivers/gpio/gpio-omap.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index e6efd77..50b4c18 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -779,6 +779,7 @@ static struct irq_chip gpio_irq_chip = {
.irq_unmask = gpio_unmask_irq,
.irq_set_type = gpio_irq_type,
.irq_set_wake = gpio_wake_enable,
+ .flags = IRQCHIP_MASK_ON_SUSPEND;
};
/*-*/
No obvious damage, unless the mailer is responsible or the ';' at the end
of
the line, rather than ',' :-)
:-) That was typo.
The approach makes sense, but does actually work. Should be fixable
though.
When I try this I get:
[ 158.114440] Checking wakeup interrupts
[ 158.118408] Unhandled fault: external abort on non-linefetch (0x1028)
at 0xfb054040
[ 158.126403] Internal error: : 1028 [#1] PREEMPT ARM
[ 158.131500] Modules linked in: ipv6 g_ether hso libertas_sdio libertas
cfg80211
[ 158.139190] CPU: 0Not tainted (3.5.0-gta04-debug+ #2)
[ 158.144927] PC is at _set_gpio_triggering+0x38/0x258
[ 158.150115] LR is at gpio_mask_irq+0xac/0xc0
[ 158.154602] pc : [c01d24a0]lr : [c01d2f68]psr: 6193
[ 158.154602] sp : db521e90 ip : 0011 fp : beeecc2c
[ 158.166595] r10: c05c8ebc r9 : daa5a858 r8 : 0003
[ 158.172027] r7 : a193 r6 : r5 : fb054000 r4 : ded44e18
[ 158.178863] r3 : 0001 r2 : r1 : ded30340 r0 : 0040
[ 158.185668] Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM
Segment use
so it looks like runtime PM has turned off the iclk to the GPIO module so
that
when we try to tell it to change settings, it is no longer listening to
us.
From the crash logs it appears like that.
The Checking wakeup interrupts function happens very late in the suspend
cycle, after all the suspend_late and suspend_noirq functions have run.
Maybe it needs to be moved earlier...
No it shouldn't be moved and it is that point for lot many good
reasons. Ofcourse
this omap gpio driver crash needs to be addressed. Need to think bit
more on this
issue.
Regards
Santosh
--
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] OMAP GPIO - don't wake from suspend unless requested.
+ Jon,
On Sat, Aug 25, 2012 at 5:14 PM, NeilBrown ne...@suse.de wrote:
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.
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
Cc: Tarun Kanti DebBarma tarun.ka...@ti.com
Cc: Felipe Balbi ba...@ti.com
Cc: Govindraj.R govindraj.r...@ti.com
Signed-off-by: NeilBrown ne...@suse.de
The patch doesn't seems to be correct. At least the 2/ gets
fixed with a proper IRQCHIP flag. Can you try the patch at
end of the email and see if it helps ? Am attaching it in case
mailer damages it.
Regards
Santosh
From b8a38fc75e046f6462610e26c47c620cad850c24 Mon Sep 17 00:00:00 2001
From: Santosh Shilimkar santosh.shilim...@ti.com
Date: Sun, 26 Aug 2012 09:39:51 +0530
Subject: [PATCH] gpio: omap: Set IRQCHIP_MASK_ON_SUSPEND to mask all
non-wakeup gpio wakeups.
Set the irq chip flag IRQCHIP_MASK_ON_SUSPEND to cause the irq pm code
to mask all non-wake gpios in suspend, which will ensure the wakeup enable
bit is not set on non-wake gpios.
Signed-off-by: Santosh Shilimkar santosh.shilim...@ti.com
---
drivers/gpio/gpio-omap.c |1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index e6efd77..50b4c18 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -779,6 +779,7 @@ static struct irq_chip gpio_irq_chip = {
.irq_unmask = gpio_unmask_irq,
.irq_set_type = gpio_irq_type,
.irq_set_wake = gpio_wake_enable,
+ .flags = IRQCHIP_MASK_ON_SUSPEND;
};
/*-*/
--
1.7.9.5
0001-gpio-omap-Set-IRQCHIP_MASK_ON_SUSPEND-to-mask-all-no.patch
Description: Binary data
