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);
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
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
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
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
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
linux-stable-mirror@lists.linaro.org
-
Fabio Estevam -
Martin Kaiser