[PATCH] ARM: OMAP2+: omap_device: call all suspend, resume callbacks when OMAP_DEVICE_NO_IDLE_ON_SUSPEND is set

[PATCH] ARM: OMAP2+: omap_device: call all suspend, resume callbacks when OMAP_DEVICE_NO_IDLE_ON_SUSPEND is set

[Paul Walmsley]

During system suspend, when OMAP_DEVICE_NO_IDLE_ON_SUSPEND is set on
an omap_device, call the corresponding driver's ->suspend() and
->suspend_noirq() callbacks (if present).  Similarly, during resume,
the driver's ->resume() and ->resume_noirq() callbacks must both be
called, if present.  (The previous code only called ->suspend_noirq()
and ->resume_noirq().)

If all of these callbacks aren't called, some important driver
suspend/resume code may not get executed.

In current mainline, the bug fixed by this patch is only a problem
under the following conditions:

- the kernel is running on an OMAP4

- an OMAP UART is used as a console

- the kernel command line parameter 'no_console_suspend' is specified

- and the system enters suspend ("echo mem > /sys/power/state").

Under this combined circumstance, the system cannot be awakened via
the serial port after commit be4b0281956c5cae4f63f31f11d07625a6988766c
("tty: serial: OMAP: block idle while the UART is transferring data in
PIO mode").  This is because the OMAP UART driver's ->suspend()
callback is never called.  The ->suspend() callback would have called
uart_suspend_port() which in turn would call enable_irq_wake().  Since
enable_irq_wake() isn't called for the UART's IRQ, check_wakeup_irqs()
would mask off the UART IRQ in the GIC.

On v3.3 kernels prior to the above commit, serial resume from suspend
presumably occurred via the PRCM interrupt.  The UART was in
smart-idle mode, so it was able to send a PRCM wakeup which in turn
would be converted into a PRCM interrupt to the GIC, waking up the
kernel.  But after the above commit, when the system is suspended in
the middle of a UART transmit, the UART IP block would be in no-idle
mode.  In no-idle mode, the UART won't generate wakeups to the PRCM
when incoming characters are received; only GIC interrupts.  But since
the UART driver's ->suspend() callback is never called,
uart_suspend_port() and enable_irq_wake() is never called; so the UART
interrupt is masked by check_wakeup_irqs() and the UART can't wake up
the MPU.

The remaining mechanism that could have awakened the system would have
been I/O chain wakeups.  These wouldn't be active because the console
UART's clocks are never disabled when no_console_suspend is used,
preventing the full chip from idling.  Also, current mainline doesn't
yet support full chip idle states for OMAP4, so I/O chain wakeups are
not enabled.

This patch is the result of a collaboration.  John Stultz
<johnstul@us.ibm.com> and Andy Green <andy.green@linaro.org> reported
the serial wakeup problem that led to the discovery of this problem.
Kevin Hilman <khilman@ti.com> narrowed the problem down to the use of
no_console_suspend.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Andy Green <andy.green@linaro.org>
Cc: Kevin Hilman <khilman@ti.com>
---
Targeted for 3.4.

 arch/arm/plat-omap/omap_device.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
index e8d9869..4988a78 100644
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -762,14 +762,12 @@ static int _od_suspend_noirq(struct device *dev)
 	struct omap_device *od = to_omap_device(pdev);
 	int ret;
 
-	if (od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND)
-		return pm_generic_suspend_noirq(dev);
-
 	ret = pm_generic_suspend_noirq(dev);
 
 	if (!ret && !pm_runtime_status_suspended(dev)) {
 		if (pm_generic_runtime_suspend(dev) == 0) {
-			omap_device_idle(pdev);
+			if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND))
+				omap_device_idle(pdev);
 			od->flags |= OMAP_DEVICE_SUSPENDED;
 		}
 	}
@@ -782,13 +780,11 @@ static int _od_resume_noirq(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct omap_device *od = to_omap_device(pdev);
 
-	if (od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND)
-		return pm_generic_resume_noirq(dev);
-
 	if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
 	    !pm_runtime_status_suspended(dev)) {
 		od->flags &= ~OMAP_DEVICE_SUSPENDED;
-		omap_device_enable(pdev);
+		if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND))
+			omap_device_enable(pdev);
 		pm_generic_runtime_resume(dev);
 	}
 
-- 
1.7.9.1

[PATCH] ARM: OMAP2+: omap_device: call all suspend, resume callbacks when OMAP_DEVICE_NO_IDLE_ON_SUSPEND is set

[Paul Walmsley]

During system suspend, when OMAP_DEVICE_NO_IDLE_ON_SUSPEND is set on
an omap_device, call the corresponding driver's ->suspend() and
->suspend_noirq() callbacks (if present).  Similarly, during resume,
the driver's ->resume() and ->resume_noirq() callbacks must both be
called, if present.  (The previous code only called ->suspend_noirq()
and ->resume_noirq().)

If all of these callbacks aren't called, some important driver
suspend/resume code may not get executed.

In current mainline, the bug fixed by this patch is only a problem
under the following conditions:

- the kernel is running on an OMAP4

- an OMAP UART is used as a console

- the kernel command line parameter 'no_console_suspend' is specified

- and the system enters suspend ("echo mem > /sys/power/state").

Under this combined circumstance, the system cannot be awakened via
the serial port after commit be4b0281956c5cae4f63f31f11d07625a6988766c
("tty: serial: OMAP: block idle while the UART is transferring data in
PIO mode").  This is because the OMAP UART driver's ->suspend()
callback is never called.  The ->suspend() callback would have called
uart_suspend_port() which in turn would call enable_irq_wake().  Since
enable_irq_wake() isn't called for the UART's IRQ, check_wakeup_irqs()
would mask off the UART IRQ in the GIC.

On v3.3 kernels prior to the above commit, serial resume from suspend
presumably occurred via the PRCM interrupt.  The UART was in
smart-idle mode, so it was able to send a PRCM wakeup which in turn
would be converted into a PRCM interrupt to the GIC, waking up the
kernel.  But after the above commit, when the system is suspended in
the middle of a UART transmit, the UART IP block would be in no-idle
mode.  In no-idle mode, the UART won't generate wakeups to the PRCM
when incoming characters are received; only GIC interrupts.  But since
the UART driver's ->suspend() callback is never called,
uart_suspend_port() and enable_irq_wake() is never called; so the UART
interrupt is masked by check_wakeup_irqs() and the UART can't wake up
the MPU.

The remaining mechanism that could have awakened the system would have
been I/O chain wakeups.  These wouldn't be active because the console
UART's clocks are never disabled when no_console_suspend is used,
preventing the full chip from idling.  Also, current mainline doesn't
yet support full chip idle states for OMAP4, so I/O chain wakeups are
not enabled.

This patch is the result of a collaboration.  John Stultz
<johnstul@us.ibm.com> and Andy Green <andy.green@linaro.org> reported
the serial wakeup problem that led to the discovery of this problem.
Kevin Hilman <khilman@ti.com> narrowed the problem down to the use of
no_console_suspend.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Andy Green <andy.green@linaro.org>
Cc: Kevin Hilman <khilman@ti.com>
---
Targeted for 3.4.

 arch/arm/plat-omap/omap_device.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
index e8d9869..4988a78 100644
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -762,14 +762,12 @@ static int _od_suspend_noirq(struct device *dev)
 	struct omap_device *od = to_omap_device(pdev);
 	int ret;
 
-	if (od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND)
-		return pm_generic_suspend_noirq(dev);
-
 	ret = pm_generic_suspend_noirq(dev);
 
 	if (!ret && !pm_runtime_status_suspended(dev)) {
 		if (pm_generic_runtime_suspend(dev) == 0) {
-			omap_device_idle(pdev);
+			if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND))
+				omap_device_idle(pdev);
 			od->flags |= OMAP_DEVICE_SUSPENDED;
 		}
 	}
@@ -782,13 +780,11 @@ static int _od_resume_noirq(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct omap_device *od = to_omap_device(pdev);
 
-	if (od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND)
-		return pm_generic_resume_noirq(dev);
-
 	if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
 	    !pm_runtime_status_suspended(dev)) {
 		od->flags &= ~OMAP_DEVICE_SUSPENDED;
-		omap_device_enable(pdev);
+		if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND))
+			omap_device_enable(pdev);
 		pm_generic_runtime_resume(dev);
 	}
 
-- 
1.7.9.1

Re: [PATCH] ARM: OMAP2+: omap_device: call all suspend, resume callbacks when OMAP_DEVICE_NO_IDLE_ON_SUSPEND is set

[Kevin Hilman]

Paul Walmsley <paul@pwsan.com> writes:

> During system suspend, when OMAP_DEVICE_NO_IDLE_ON_SUSPEND is set on
> an omap_device, call the corresponding driver's ->suspend() and
> ->suspend_noirq() callbacks (if present).  Similarly, during resume,
> the driver's ->resume() and ->resume_noirq() callbacks must both be
> called, if present.  (The previous code only called ->suspend_noirq()
> and ->resume_noirq().)
>
> If all of these callbacks aren't called, some important driver
> suspend/resume code may not get executed.
>
> In current mainline, the bug fixed by this patch is only a problem
> under the following conditions:
>
> - the kernel is running on an OMAP4
>
> - an OMAP UART is used as a console
>
> - the kernel command line parameter 'no_console_suspend' is specified
>
> - and the system enters suspend ("echo mem > /sys/power/state").
>
> Under this combined circumstance, the system cannot be awakened via
> the serial port after commit be4b0281956c5cae4f63f31f11d07625a6988766c
> ("tty: serial: OMAP: block idle while the UART is transferring data in
> PIO mode").  This is because the OMAP UART driver's ->suspend()
> callback is never called.  The ->suspend() callback would have called
> uart_suspend_port() which in turn would call enable_irq_wake().  Since
> enable_irq_wake() isn't called for the UART's IRQ, check_wakeup_irqs()
> would mask off the UART IRQ in the GIC.
>
> On v3.3 kernels prior to the above commit, serial resume from suspend
> presumably occurred via the PRCM interrupt.  The UART was in
> smart-idle mode, so it was able to send a PRCM wakeup which in turn
> would be converted into a PRCM interrupt to the GIC, waking up the
> kernel.  But after the above commit, when the system is suspended in
> the middle of a UART transmit, the UART IP block would be in no-idle
> mode.  In no-idle mode, the UART won't generate wakeups to the PRCM
> when incoming characters are received; only GIC interrupts.  But since
> the UART driver's ->suspend() callback is never called,
> uart_suspend_port() and enable_irq_wake() is never called; so the UART
> interrupt is masked by check_wakeup_irqs() and the UART can't wake up
> the MPU.
>
> The remaining mechanism that could have awakened the system would have
> been I/O chain wakeups.  These wouldn't be active because the console
> UART's clocks are never disabled when no_console_suspend is used,
> preventing the full chip from idling.  Also, current mainline doesn't
> yet support full chip idle states for OMAP4, so I/O chain wakeups are
> not enabled.
>
> This patch is the result of a collaboration.  John Stultz
> <johnstul@us.ibm.com> and Andy Green <andy.green@linaro.org> reported
> the serial wakeup problem that led to the discovery of this problem.
> Kevin Hilman <khilman@ti.com> narrowed the problem down to the use of
> no_console_suspend.
>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: John Stultz <johnstul@us.ibm.com>
> Cc: Andy Green <andy.green@linaro.org>
> Cc: Kevin Hilman <khilman@ti.com>

Looks right.

Acked-by: Kevin Hilman <khilman@ti.com>

Tony, this fix is needed for v3.3.

Kevin

[PATCH] ARM: OMAP2+: omap_device: call all suspend, resume callbacks when OMAP_DEVICE_NO_IDLE_ON_SUSPEND is set

[Kevin Hilman]

Paul Walmsley <paul@pwsan.com> writes:

> During system suspend, when OMAP_DEVICE_NO_IDLE_ON_SUSPEND is set on
> an omap_device, call the corresponding driver's ->suspend() and
> ->suspend_noirq() callbacks (if present).  Similarly, during resume,
> the driver's ->resume() and ->resume_noirq() callbacks must both be
> called, if present.  (The previous code only called ->suspend_noirq()
> and ->resume_noirq().)
>
> If all of these callbacks aren't called, some important driver
> suspend/resume code may not get executed.
>
> In current mainline, the bug fixed by this patch is only a problem
> under the following conditions:
>
> - the kernel is running on an OMAP4
>
> - an OMAP UART is used as a console
>
> - the kernel command line parameter 'no_console_suspend' is specified
>
> - and the system enters suspend ("echo mem > /sys/power/state").
>
> Under this combined circumstance, the system cannot be awakened via
> the serial port after commit be4b0281956c5cae4f63f31f11d07625a6988766c
> ("tty: serial: OMAP: block idle while the UART is transferring data in
> PIO mode").  This is because the OMAP UART driver's ->suspend()
> callback is never called.  The ->suspend() callback would have called
> uart_suspend_port() which in turn would call enable_irq_wake().  Since
> enable_irq_wake() isn't called for the UART's IRQ, check_wakeup_irqs()
> would mask off the UART IRQ in the GIC.
>
> On v3.3 kernels prior to the above commit, serial resume from suspend
> presumably occurred via the PRCM interrupt.  The UART was in
> smart-idle mode, so it was able to send a PRCM wakeup which in turn
> would be converted into a PRCM interrupt to the GIC, waking up the
> kernel.  But after the above commit, when the system is suspended in
> the middle of a UART transmit, the UART IP block would be in no-idle
> mode.  In no-idle mode, the UART won't generate wakeups to the PRCM
> when incoming characters are received; only GIC interrupts.  But since
> the UART driver's ->suspend() callback is never called,
> uart_suspend_port() and enable_irq_wake() is never called; so the UART
> interrupt is masked by check_wakeup_irqs() and the UART can't wake up
> the MPU.
>
> The remaining mechanism that could have awakened the system would have
> been I/O chain wakeups.  These wouldn't be active because the console
> UART's clocks are never disabled when no_console_suspend is used,
> preventing the full chip from idling.  Also, current mainline doesn't
> yet support full chip idle states for OMAP4, so I/O chain wakeups are
> not enabled.
>
> This patch is the result of a collaboration.  John Stultz
> <johnstul@us.ibm.com> and Andy Green <andy.green@linaro.org> reported
> the serial wakeup problem that led to the discovery of this problem.
> Kevin Hilman <khilman@ti.com> narrowed the problem down to the use of
> no_console_suspend.
>
> Signed-off-by: Paul Walmsley <paul@pwsan.com>
> Cc: John Stultz <johnstul@us.ibm.com>
> Cc: Andy Green <andy.green@linaro.org>
> Cc: Kevin Hilman <khilman@ti.com>

Looks right.

Acked-by: Kevin Hilman <khilman@ti.com>

Tony, this fix is needed for v3.3.

Kevin

Re: [PATCH] ARM: OMAP2+: omap_device: call all suspend, resume callbacks when OMAP_DEVICE_NO_IDLE_ON_SUSPEND is set

[Paul Walmsley]

On Mon, 5 Mar 2012, Kevin Hilman wrote:

> Tony, this fix is needed for v3.3.

Of course, I don't have any objection to this patch going into the v3.3-rc 
kernels.  But given that some maintainers seem to be pushing back against 
late -rc patches, this one could probably be delayed to 3.4 if required.  
no_console_suspend is a debugging-only option and bugs involving it are 
unlikely to affect most users.


- Paul

[PATCH] ARM: OMAP2+: omap_device: call all suspend, resume callbacks when OMAP_DEVICE_NO_IDLE_ON_SUSPEND is set

[Paul Walmsley]

On Mon, 5 Mar 2012, Kevin Hilman wrote:

> Tony, this fix is needed for v3.3.

Of course, I don't have any objection to this patch going into the v3.3-rc 
kernels.  But given that some maintainers seem to be pushing back against 
late -rc patches, this one could probably be delayed to 3.4 if required.  
no_console_suspend is a debugging-only option and bugs involving it are 
unlikely to affect most users.


- Paul

Re: [PATCH] ARM: OMAP2+: omap_device: call all suspend, resume callbacks when OMAP_DEVICE_NO_IDLE_ON_SUSPEND is set

[Tony Lindgren]

* Paul Walmsley <paul@pwsan.com> [120305 11:44]:
> On Mon, 5 Mar 2012, Kevin Hilman wrote:
> 
> > Tony, this fix is needed for v3.3.
> 
> Of course, I don't have any objection to this patch going into the v3.3-rc 
> kernels.  But given that some maintainers seem to be pushing back against 
> late -rc patches, this one could probably be delayed to 3.4 if required.  
> no_console_suspend is a debugging-only option and bugs involving it are 
> unlikely to affect most users.

I'm fine with your queueing it for v3.4 merge window.

Regards,

Tomny

[PATCH] ARM: OMAP2+: omap_device: call all suspend, resume callbacks when OMAP_DEVICE_NO_IDLE_ON_SUSPEND is set

[Tony Lindgren]

* Paul Walmsley <paul@pwsan.com> [120305 11:44]:
> On Mon, 5 Mar 2012, Kevin Hilman wrote:
> 
> > Tony, this fix is needed for v3.3.
> 
> Of course, I don't have any objection to this patch going into the v3.3-rc 
> kernels.  But given that some maintainers seem to be pushing back against 
> late -rc patches, this one could probably be delayed to 3.4 if required.  
> no_console_suspend is a debugging-only option and bugs involving it are 
> unlikely to affect most users.

I'm fine with your queueing it for v3.4 merge window.

Regards,

Tomny

Re: [PATCH] ARM: OMAP2+: omap_device: call all suspend, resume callbacks when OMAP_DEVICE_NO_IDLE_ON_SUSPEND is set

[Paul Walmsley]

On Mon, 5 Mar 2012, Tony Lindgren wrote:

> * Paul Walmsley <paul@pwsan.com> [120305 11:44]:
> > On Mon, 5 Mar 2012, Kevin Hilman wrote:
> > 
> > > Tony, this fix is needed for v3.3.
> > 
> > Of course, I don't have any objection to this patch going into the v3.3-rc 
> > kernels.  But given that some maintainers seem to be pushing back against 
> > late -rc patches, this one could probably be delayed to 3.4 if required.  
> > no_console_suspend is a debugging-only option and bugs involving it are 
> > unlikely to affect most users.
> 
> I'm fine with your queueing it for v3.4 merge window.

Just chatted with Kevin, he's going to take this one for 3.4 with some 
other omap_device patches he's got queued.


- Paul

[PATCH] ARM: OMAP2+: omap_device: call all suspend, resume callbacks when OMAP_DEVICE_NO_IDLE_ON_SUSPEND is set

[Paul Walmsley]

On Mon, 5 Mar 2012, Tony Lindgren wrote:

> * Paul Walmsley <paul@pwsan.com> [120305 11:44]:
> > On Mon, 5 Mar 2012, Kevin Hilman wrote:
> > 
> > > Tony, this fix is needed for v3.3.
> > 
> > Of course, I don't have any objection to this patch going into the v3.3-rc 
> > kernels.  But given that some maintainers seem to be pushing back against 
> > late -rc patches, this one could probably be delayed to 3.4 if required.  
> > no_console_suspend is a debugging-only option and bugs involving it are 
> > unlikely to affect most users.
> 
> I'm fine with your queueing it for v3.4 merge window.

Just chatted with Kevin, he's going to take this one for 3.4 with some 
other omap_device patches he's got queued.


- Paul