[PATCH 1/2] serial: imx: edd a .freeze_noirq function

[PATCH 1/2] serial: imx: edd a .freeze_noirq function

[Martin Kaiser]

Define a .freeze_noirq function. For now, this is an identical copy of
.suspend_noirq. (We'll move the wakeup settings to .suspend_noirq
shortly.)

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/tty/serial/imx.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 145ed61..250aa26 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2269,6 +2269,23 @@ static int imx_serial_port_resume_noirq(struct device *dev)
 	return 0;
 }
 
+static int imx_serial_port_freeze_noirq(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct imx_port *sport = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = clk_enable(sport->clk_ipg);
+	if (ret)
+		return ret;
+
+	serial_imx_save_context(sport);
+
+	clk_disable(sport->clk_ipg);
+
+	return 0;
+}
+
 static int imx_serial_port_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
@@ -2326,7 +2343,7 @@ static int imx_serial_port_thaw(struct device *dev)
 static const struct dev_pm_ops imx_serial_port_pm_ops = {
 	.suspend_noirq = imx_serial_port_suspend_noirq,
 	.resume_noirq = imx_serial_port_resume_noirq,
-	.freeze_noirq = imx_serial_port_suspend_noirq,
+	.freeze_noirq = imx_serial_port_freeze_noirq,
 	.restore_noirq = imx_serial_port_resume_noirq,
 	.suspend = imx_serial_port_suspend,
 	.resume = imx_serial_port_resume,
-- 
2.1.4

[PATCH 2/2] serial: imx: fix endless loop during suspend

[Martin Kaiser]

Before we go into suspend mode, we enable the imx uart's interrupt for
the awake bit in the UART Status Register 1. If, for some reason, the
awake bit is already set before we enter suspend mode, we get an
interrupt immediately when we enable interrupts for awake. The uart's
clk_ipg is already disabled at this point. We end up in the interrupt
handler, which usually tries to clear the awake bit. This doesn't work
with the clock disabled. Therefore, we keep getting interrupts forever,
resulting in an endless loop.

Move the calls to serial_imx_enable_wakeup() into the _noirq functions,
where interrupts are disabled and clk_ipg is active. This way, we can
safely clear the awake bit and enable the imx interrupt for awake.

Now that we do the wakeup configuration in .suspend_noirq, we need
separate functions for .suspend_noirq and .freeze_noirq. However,
.resume_noirq and .restore_noirq can still be shared. We just disable
the wakeup source there, this does not conflict with hibernation.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
Cc: stable@vger.kernel.org
---
 drivers/tty/serial/imx.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 250aa26..5df9172 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2221,8 +2221,10 @@ static void serial_imx_enable_wakeup(struct imx_port *sport, bool on)
 	unsigned int val;
 
 	val = readl(sport->port.membase + UCR3);
-	if (on)
+	if (on) {
+		writel(USR1_AWAKE, sport->port.membase + USR1);
 		val |= UCR3_AWAKEN;
+	}
 	else
 		val &= ~UCR3_AWAKEN;
 	writel(val, sport->port.membase + UCR3);
@@ -2245,6 +2247,9 @@ static int imx_serial_port_suspend_noirq(struct device *dev)
 	if (ret)
 		return ret;
 
+	/* enable wakeup from i.MX UART */
+	serial_imx_enable_wakeup(sport, true);
+
 	serial_imx_save_context(sport);
 
 	clk_disable(sport->clk_ipg);
@@ -2264,6 +2269,9 @@ static int imx_serial_port_resume_noirq(struct device *dev)
 
 	serial_imx_restore_context(sport);
 
+	/* disable wakeup from i.MX UART */
+	serial_imx_enable_wakeup(sport, false);
+
 	clk_disable(sport->clk_ipg);
 
 	return 0;
@@ -2291,9 +2299,6 @@ static int imx_serial_port_suspend(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct imx_port *sport = platform_get_drvdata(pdev);
 
-	/* enable wakeup from i.MX UART */
-	serial_imx_enable_wakeup(sport, true);
-
 	uart_suspend_port(&imx_reg, &sport->port);
 	disable_irq(sport->port.irq);
 
@@ -2306,9 +2311,6 @@ static int imx_serial_port_resume(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct imx_port *sport = platform_get_drvdata(pdev);
 
-	/* disable wakeup from i.MX UART */
-	serial_imx_enable_wakeup(sport, false);
-
 	uart_resume_port(&imx_reg, &sport->port);
 	enable_irq(sport->port.irq);
 
-- 
2.1.4

Re: [PATCH 2/2] serial: imx: fix endless loop during suspend

[Fabio Estevam]

Hi Martin,

On Wed, Dec 27, 2017 at 3:27 PM, Martin Kaiser <martin@kaiser.cx> wrote:
> Before we go into suspend mode, we enable the imx uart's interrupt for
> the awake bit in the UART Status Register 1. If, for some reason, the
> awake bit is already set before we enter suspend mode, we get an
> interrupt immediately when we enable interrupts for awake. The uart's
> clk_ipg is already disabled at this point. We end up in the interrupt
> handler, which usually tries to clear the awake bit. This doesn't work
> with the clock disabled. Therefore, we keep getting interrupts forever,
> resulting in an endless loop.
>
> Move the calls to serial_imx_enable_wakeup() into the _noirq functions,
> where interrupts are disabled and clk_ipg is active. This way, we can
> safely clear the awake bit and enable the imx interrupt for awake.
>
> Now that we do the wakeup configuration in .suspend_noirq, we need
> separate functions for .suspend_noirq and .freeze_noirq. However,
> .resume_noirq and .restore_noirq can still be shared. We just disable
> the wakeup source there, this does not conflict with hibernation.
>
> Signed-off-by: Martin Kaiser <martin@kaiser.cx>
> Cc: stable@vger.kernel.org

Which i.MX SoC did you use to test this patch?

On a imx6q-cuboxi I am no longer able to enter in suspend with this
path applied:

# echo enabled > /sys/class/tty/ttymxc0/power/wakeup
# echo mem > /sys/power/state
[    9.766903] PM: suspend entry (deep)
[    9.770658] PM: Syncing filesystems ... done.
[    9.815312] Freezing user space processes ... (elapsed 0.001 seconds) done.
[    9.824744] OOM killer disabled.
[    9.827998] Freezing remaining freezable tasks ... (elapsed 0.001
seconds) done.
[    9.837351] Suspending console(s) (use no_console_suspend to debug)
[    9.915495] PM: suspend devices took 0.080 seconds
[    9.928746] PM: noirq suspend of devices failed
[   10.196232] mmc0: queuing unknown CIS tuple 0x80 (2 bytes)
[   10.198148] mmc0: queuing unknown CIS tuple 0x80 (3 bytes)
[   10.200042] mmc0: queuing unknown CIS tuple 0x80 (3 bytes)
[   10.203420] mmc0: queuing unknown CIS tuple 0x80 (7 bytes)
[   10.206812] mmc0: queuing unknown CIS tuple 0x80 (7 bytes)
[   10.263097] PM: resume devices took 0.330 seconds
[   10.266639] ata1: SATA link down (SStatus 0 SControl 300)
[   10.310305] OOM killer enabled.
[   10.313458] Restarting tasks ... done.
[   10.319568] PM: suspend exit
sh: write error: Device or resource busy

Even if I do not press anything in the console the system gets out of
suspend automatically.

Thanks

Re: [PATCH 2/2] serial: imx: fix endless loop during suspend

[Martin Kaiser]

Hi Fabio,

thanks for testing my patch. Sorry for breaking suspend on your board.

Thus wrote Fabio Estevam (festevam@gmail.com):

> Which i.MX SoC did you use to test this patch?

I tested on an imx258.

> On a imx6q-cuboxi I am no longer able to enter in suspend with this
> path applied:

> # echo enabled > /sys/class/tty/ttymxc0/power/wakeup
> # echo mem > /sys/power/state
> [    9.766903] PM: suspend entry (deep)
> [    9.770658] PM: Syncing filesystems ... done.
> [    9.815312] Freezing user space processes ... (elapsed 0.001 seconds) done.
> [    9.824744] OOM killer disabled.
> [    9.827998] Freezing remaining freezable tasks ... (elapsed 0.001
> seconds) done.
> [    9.837351] Suspending console(s) (use no_console_suspend to debug)
> [    9.915495] PM: suspend devices took 0.080 seconds
> [    9.928746] PM: noirq suspend of devices failed
> [   10.196232] mmc0: queuing unknown CIS tuple 0x80 (2 bytes)
> [   10.198148] mmc0: queuing unknown CIS tuple 0x80 (3 bytes)
> [   10.200042] mmc0: queuing unknown CIS tuple 0x80 (3 bytes)
> [   10.203420] mmc0: queuing unknown CIS tuple 0x80 (7 bytes)
> [   10.206812] mmc0: queuing unknown CIS tuple 0x80 (7 bytes)
> [   10.263097] PM: resume devices took 0.330 seconds
> [   10.266639] ata1: SATA link down (SStatus 0 SControl 300)
> [   10.310305] OOM killer enabled.
> [   10.313458] Restarting tasks ... done.
> [   10.319568] PM: suspend exit
> sh: write error: Device or resource busy

> Even if I do not press anything in the console the system gets out of
> suspend automatically.

So the suspend_noirq step failed with -EBUSY.

My guess is that the following code path is taken

suspend_devices_and_enter()
   suspend_enter()
      dpm_suspend_noirq()
         dpm_noirq_suspend_devices()
            device_suspend_noirq()
               __device_suspend_noirq()
                  pm_wakeup_pending()

and pm_wakeup_pending() returns true. We'd then have an async_error
-EBUSY.

If that's the case, I don't understand why it happens for imx6q.
We should only have a pending wakeup event if wakeup_source_activate()
or ..._deactivate() has been called.

Seeing this kind of problem, I wonder if it's ok to move setting AWAKEN
from the suspend to the suspend_noirq step. The imx uart's interrupt is
now re-enabled and IRQD_WAKEUP_ARMED is set before we configure the uart
to generate such interrupts (by setting AWAKEN), whereas before, it was
the other way around. I'd be grateful if anyone could shed some light
on this. (Or more generally: When must the hardware be configured to
generate wakeup interrupts? Is it ok to do this in supend_noirq or must
it be done before?)

Fabio, could you post the output of

cat /sys/kernel/debug/suspend_stats

after supend failed, to confirm that we're failing below
device_suspend_noirq()?

Thanks,

   Martin

Re: [PATCH 2/2] serial: imx: fix endless loop during suspend

[Fabio Estevam]

Hi Martin,

On Tue, Jan 2, 2018 at 2:15 PM, Martin Kaiser <martin@kaiser.cx> wrote:

> Fabio, could you post the output of
>
> cat /sys/kernel/debug/suspend_stats
>
> after supend failed, to confirm that we're failing below
> device_suspend_noirq()?

Here it goes:

# cat /sys/kernel/debug/suspend_stats
success: 0
fail: 1
failed_freeze: 0
failed_prepare: 0
failed_suspend: 0
failed_suspend_late: 0
failed_suspend_noirq: 1
failed_resume: 0
failed_resume_early: 0
failed_resume_noirq: 0
failures:
  last_failed_dev:

  last_failed_errno:    -16
                        0
  last_failed_step:     suspend_noirq

Re: [PATCH 2/2] serial: imx: fix endless loop during suspend

[Fabio Estevam]

Hi Martin,

On Tue, Jan 2, 2018 at 2:15 PM, Martin Kaiser <martin@kaiser.cx> wrote:

> I tested on an imx258.

I am not able to reproduce this problem on a imx25 pdk running 4.14.11
or linux-next.

Is it 100% reproducible on your board?

Care to share its dts? Do you use multiple UART ports? Do any of them
use RTS/CTS?

Do you have any logs to share?

Thanks

Re: [PATCH 2/2] serial: imx: fix endless loop during suspend

[Martin Kaiser]

Hi Fabio,

Thus wrote Fabio Estevam (festevam@gmail.com):

> I am not able to reproduce this problem on a imx25 pdk running 4.14.11
> or linux-next.

this is no surprise. The problem shows up only if the AWAKE bit in UART
Status Register 1 is set before we go into suspend. My understanding is
that this bit will be set when the signal on the rx pin goes from high
to low. 

> Is it 100% reproducible on your board?

On my board, I have one uart port that's connected to an external chip.
A power cycle of this external chip creates the falling edge on rx and
makes the imx uart set the AWAKE bit. The problem can then be reproduced
100%.

It can be argued that this is an obscure scenario, but nevertheless, it
should not put the kernel into an endless loop.

This is my understanding of what happens:
- AWAKE is set in the USR1 register
- there's no uart transfer running, the clocks are disabled
   (As I write this I'm not sure why this is the case, clk_ipg should
    still be enabled but it seems that it's not. If I enable it
    manually, the behaviour is different.)
- we enter suspend
- AWAKEN (UART control register 3) is set so that AWAKE creates an interrupt
- we get an interrupt immediately
   (the imx interrupt is not yet disabled at this point. it'll be
    disabled later and then reenabled if the uart port acts as a wakeup
    source)
   -> we get into the interrupt handler with the clocks disabled
- the interrupt handler tries to clear the AWAKE bit, this does not work
  since the clocks are off, we exit and AWAKE is still set
- we get another interrupt immediately
   -> endless loop

What I'm trying to do with my patch is to clear the AWAKE bit before we
set AWAKEEN. We don't want to be woken up by events that happened before
we started going into suspend.

To do this, I have to find a suitable place where the clocks are
enabled. That's why I tried to move clearing AWAKE and setting AWAKEEN
into suspend_noirq, where the clocks are already enabled to save all
registers before we finally suspend. While this works ok on my board, it
cuases problems on imx6q.

I'm not sure what makes my patch fail for you. I could imagine it is
because now, the imx interrupt is enabled (as a wakeup source) before
AWAKEN is set. The current code sets AWAKEN first and then enables the
interrupt for the wakeup source.

I'll try a different approach that keeps this order.

> Care to share its dts? Do you use multiple UART ports? Do any of them
> use RTS/CTS?

There's nothing special regarding the uarts, There's a couple of them,
none of which are using rts/cts.

It all depends on the awake bit.

Best regards,

   Martin

[PATCH v2] serial: imx: fix endless loop during suspend

[Martin Kaiser]

Before we go into suspend mode, we enable the imx uart's interrupt for
the awake bit in the UART Status Register 1. If, for some reason, the
awake bit is already set before we enter suspend mode, we get an
interrupt immediately when we enable interrupts for awake. The uart's
clk_ipg is disabled at this point (unless there's an ongoing transfer).
We end up in the interrupt handler, which usually tries to clear the
awake bit. This doesn't work with the clock disabled. Therefore, we
keep getting interrupts forever, resulting in an endless loop.

Clear the awake bit before setting the awaken bit to signal that we want
an imx interrupt when the awake bit will be set. This ensures that we're
not woken up by events that happened before we started going into
suspend mode.

Change the clock handling so that suspend prepares and enables the clock
and suspend_noirq disables it. Revert these operations in resume_noirq and
resume.

With these preparations in place, we can now modify awake and awaken in
the suspend function when the actual imx interrupt is disabled and the
required clk_ipg is active.

Update the thaw and freeze functions to use the new clock handling since
we share the suspend_noirq function between suspend and hibernate.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
changes in v2
 call serial_imx_enable_wakeup() in suspend, not in suspend_noirq
 keep the clock enalbed between suspend and suspend_noirq and
  between resume_noirq and resume

Hi Fabio and all,

here's a different approach for fixing the awake issue that I see on my
board. I tried to stick as much as possible to the original order in
which the operations were done. Could you do a quick check on imx6q?

Thanks,
   Martin

 drivers/tty/serial/imx.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 145ed61..e5d5250 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -2221,8 +2221,10 @@ static void serial_imx_enable_wakeup(struct imx_port *sport, bool on)
 	unsigned int val;
 
 	val = readl(sport->port.membase + UCR3);
-	if (on)
+	if (on) {
+		writel(USR1_AWAKE, sport->port.membase + USR1);
 		val |= UCR3_AWAKEN;
+	}
 	else
 		val &= ~UCR3_AWAKEN;
 	writel(val, sport->port.membase + UCR3);
@@ -2239,11 +2241,6 @@ static int imx_serial_port_suspend_noirq(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct imx_port *sport = platform_get_drvdata(pdev);
-	int ret;
-
-	ret = clk_enable(sport->clk_ipg);
-	if (ret)
-		return ret;
 
 	serial_imx_save_context(sport);
 
@@ -2264,8 +2261,6 @@ static int imx_serial_port_resume_noirq(struct device *dev)
 
 	serial_imx_restore_context(sport);
 
-	clk_disable(sport->clk_ipg);
-
 	return 0;
 }
 
@@ -2273,15 +2268,19 @@ static int imx_serial_port_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct imx_port *sport = platform_get_drvdata(pdev);
-
-	/* enable wakeup from i.MX UART */
-	serial_imx_enable_wakeup(sport, true);
+	int ret;
 
 	uart_suspend_port(&imx_reg, &sport->port);
 	disable_irq(sport->port.irq);
 
-	/* Needed to enable clock in suspend_noirq */
-	return clk_prepare(sport->clk_ipg);
+	ret = clk_prepare_enable(sport->clk_ipg);
+	if (ret)
+		return ret;
+
+	/* enable wakeup from i.MX UART */
+	serial_imx_enable_wakeup(sport, true);
+
+	return 0;
 }
 
 static int imx_serial_port_resume(struct device *dev)
@@ -2295,7 +2294,7 @@ static int imx_serial_port_resume(struct device *dev)
 	uart_resume_port(&imx_reg, &sport->port);
 	enable_irq(sport->port.irq);
 
-	clk_unprepare(sport->clk_ipg);
+	clk_disable_unprepare(sport->clk_ipg);
 
 	return 0;
 }
@@ -2307,8 +2306,7 @@ static int imx_serial_port_freeze(struct device *dev)
 
 	uart_suspend_port(&imx_reg, &sport->port);
 
-	/* Needed to enable clock in suspend_noirq */
-	return clk_prepare(sport->clk_ipg);
+	return clk_prepare_enable(sport->clk_ipg);
 }
 
 static int imx_serial_port_thaw(struct device *dev)
@@ -2318,7 +2316,7 @@ static int imx_serial_port_thaw(struct device *dev)
 
 	uart_resume_port(&imx_reg, &sport->port);
 
-	clk_unprepare(sport->clk_ipg);
+	clk_disable_unprepare(sport->clk_ipg);
 
 	return 0;
 }
-- 
2.1.4

Re: [PATCH v2] serial: imx: fix endless loop during suspend

[Fabio Estevam]

Hi Martin,

On Fri, Jan 5, 2018 at 2:46 PM, Martin Kaiser <martin@kaiser.cx> wrote:

> Hi Fabio and all,
>
> here's a different approach for fixing the awake issue that I see on my
> board. I tried to stick as much as possible to the original order in
> which the operations were done. Could you do a quick check on imx6q?

suspend_stats looks better now:

#  cat /sys/kernel/debug/suspend_stats
success: 8
fail: 0
failed_freeze: 0
failed_prepare: 0
failed_suspend: 0
failed_suspend_late: 0
failed_suspend_noirq: 0
failed_resume: 0
failed_resume_early: 0
failed_resume_noirq: 0
failures:
  last_failed_dev:

  last_failed_errno:    0
                        0
  last_failed_step:

However the board is still automatically returning from resume (I
haven't entered any char in the console)

# echo mem > /sys/power/state
[  186.723470] PM: suspend entry (deep)
[  186.727107] PM: Syncing filesystems ... done.
[  186.735016] Freezing user space processes ... (elapsed 0.001 seconds) done.
[  186.743640] OOM killer disabled.
[  186.746888] Freezing remaining freezable tasks ... (elapsed 0.001
seconds) done.
[  186.756174] Suspending console(s) (use no_console_suspend to debug)
[  186.828645] PM: suspend devices took 0.060 seconds
[  186.837771] Disabling non-boot CPUs ...
[  187.014044] Enabling non-boot CPUs ...
[  187.015057] CPU1 is up
[  187.016003] CPU2 is up
[  187.016988] CPU3 is up
[  187.294341] mmc0: queuing unknown CIS tuple 0x80 (2 bytes)
[  187.296228] mmc0: queuing unknown CIS tuple 0x80 (3 bytes)
[  187.298111] mmc0: queuing unknown CIS tuple 0x80 (3 bytes)
[  187.301490] mmc0: queuing unknown CIS tuple 0x80 (7 bytes)
[  187.304873] mmc0: queuing unknown CIS tuple 0x80 (7 bytes)
[  187.354759] ata1: SATA link down (SStatus 0 SControl 300)
[  187.359840] PM: resume devices took 0.340 seconds
[  187.417332] OOM killer enabled.
[  187.420487] Restarting tasks ... done.
[  187.425435] PM: suspend exit