Re: [PATCHv3] driver: serial: prevent UART console idle on suspendwhile using "no_console_suspend"

[Sourav Poddar]

Hi Kevin,
On Friday 05 April 2013 11:10 PM, Kevin Hilman wrote:
> Sourav Poddar<sourav.poddar@xxxxxx>  writes:
>
> > With dt boot, uart wakeup after suspend is non functional while using
> > "no_console_suspend" in the bootargs. With "no_console_suspend" used, we
> > should prevent the runtime suspend of the uart port which is getting used
> > as an console.
> >
> > Cc: Santosh Shilimkar<santosh.shilimkar@xxxxxx>
> > Cc: Felipe Balbi<balbi@xxxxxx>
> > Cc: Rajendra nayak<rnayak@xxxxxx>
> > Tested on omap5430evm, omap4430sdp.
> >
> > Signed-off-by: Sourav Poddar<sourav.poddar@xxxxxx>
> Rather than make these special checks inside the driver's runtime PM
> callbacks, you should just disable runtime PM (pm_runtime_disable())
>
> Then, this should be broken into 2 patches.
>
> 1) serial core: add the '->is_console' flag.  (nit on naming: don't call
>     it port_is_console, since the struct is already a uart_port)
>
> 2) In the OMAP UART driver's ->prepare callback, check the is_console flag
>     and pm_runtime_disable() accordingly  (then pm_runtime_enable() in
>     the drivers's ->complete callback.
>
> Kevin

I was working on your above suggestions, but realised there is not only 
console
uart which has the requirement of keeping the clocks enabled while going on
suspend.

If you see arch/arm/boot/dts/am33xx.dtsi, there is a ocmcram which has
"no_idle_on_suspend" property used.

    ocmcram: ocmcram@40300000 {
                        compatible = "ti,am3352-ocmcram";
                        reg = <0x40300000 0x10000>;
                        ti,hwmods = "ocmcram";
                        ti,no_idle_on_suspend;
        };
This property gets checked in omap_device file and correspondingly 
od->flags is set.

Based on your above inputs, the patches which I cooked up is inlined[1]. 
Though, the below
patches works fine for uart case. The patches will effect ocmcram case 
and I am inling them
"just for discussion".

I am not sure, if there is any other cleaner way of getting around this 
"no_idle_on_suspend" flag
as they are getting used for ocmcram also. ?

Thanks,
Sourav

[1]:
From: Sourav Poddar <sourav.poddar@xxxxxx>
Date: Wed, 13 Mar 2013 20:32:36 +0530
Subject: [PATCH 1/2] arm: mach-omap2: remove 
"OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check

Remove the "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check, since serial core and 
driver
takes care of the case when "no_console_suspend" is used in the bootargs and
you need to keep the clock enable for console even while suspend.

Tested on omap5430evm, omap4430sdp.

Signed-off-by: Sourav Poddar <sourav.poddar@xxxxxx>
---
 arch/arm/mach-omap2/omap_device.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_device.c 
b/arch/arm/mach-omap2/omap_device.c
index 381be7a..d6dce8f 100644
--- a/arch/arm/mach-omap2/omap_device.c
+++ b/arch/arm/mach-omap2/omap_device.c
@@ -620,11 +620,8 @@ static int _od_suspend_noirq(struct device *dev)
        ret = pm_generic_suspend_noirq(dev);

        if (!ret && !pm_runtime_status_suspended(dev)) {
-               if (pm_generic_runtime_suspend(dev) == 0) {
-                       if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND))
-                               omap_device_idle(pdev);
+               if (pm_generic_runtime_suspend(dev) == 0)
                        od->flags |= OMAP_DEVICE_SUSPENDED;
-               }
        }

        return ret;
@@ -638,8 +635,6 @@ static int _od_resume_noirq(struct device *dev)
        if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
            !pm_runtime_status_suspended(dev)) {
                od->flags &= ~OMAP_DEVICE_SUSPENDED;
-               if (!(od->flags & OMAP_DEVICE_NO_IDLE_ON_SUSPEND))
-                       omap_device_enable(pdev);
                pm_generic_runtime_resume(dev);
        }

--

From: Sourav Poddar <sourav.poddar@xxxxxx>
Date: Wed, 13 Mar 2013 20:32:36 +0530
Subject: [PATCH 2/2] driver: serial: omap: add prepare/complete callback 
for "no_console_suspend" case

This patch adapt the serial core/driver to take care of the case when 
"no_console_suspend"
is used in the bootargs. This patch will remove dependency to set 
od->flags to
"OMAP_DEVICE_NO_IDLE_ON_SUSPEND" in serial.c(non dt case) and 
omap_device.c(dt case).

Prepare and complete callbacks will ensure that clocks remain active for 
the console
uart when "no_console_suspend" is used in the bootargs.

Tested on omap5430evm, omap4430sdp.

Signed-off-by: Sourav Poddar <sourav.poddar@xxxxxx>
---
 drivers/tty/serial/omap-serial.c |   20 ++++++++++++++++++++
 drivers/tty/serial/serial_core.c |    3 +++
 include/linux/serial_core.h      |    1 +
 3 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/serial/omap-serial.c 
b/drivers/tty/serial/omap-serial.c
index 08332f3..b726b2b 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -1278,6 +1278,24 @@ static struct uart_driver serial_omap_reg = {
 };

 #ifdef CONFIG_PM_SLEEP
+static int serial_omap_prepare(struct device *dev)
+{
+       struct uart_omap_port *up = dev_get_drvdata(dev);
+
+       if (!console_suspend_enabled && up->port.is_console)
+               pm_runtime_disable(dev);
+
+       return 0;
+}
+
+static void serial_omap_complete(struct device *dev)
+{
+       struct uart_omap_port *up = dev_get_drvdata(dev);
+
+       if (!console_suspend_enabled && up->port.is_console)
+               pm_runtime_enable(dev);
+}
+
 static int serial_omap_suspend(struct device *dev)
 {
        struct uart_omap_port *up = dev_get_drvdata(dev);
@@ -1632,6 +1650,8 @@ static const struct dev_pm_ops 
serial_omap_dev_pm_ops = {
        SET_SYSTEM_SLEEP_PM_OPS(serial_omap_suspend, serial_omap_resume)
        SET_RUNTIME_PM_OPS(serial_omap_runtime_suspend,
                                serial_omap_runtime_resume, NULL)
+       .prepare        = serial_omap_prepare,
+       .complete       = serial_omap_complete,
 };

 #if defined(CONFIG_OF)
diff --git a/drivers/tty/serial/serial_core.c 
b/drivers/tty/serial/serial_core.c
index a400002..c4d9328 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2594,6 +2594,9 @@ int uart_add_one_port(struct uart_driver *drv, 
struct uart_port *uport)
        uport->cons = drv->cons;
        uport->state = state;

+       if (uart_console(uport))
+               uport->is_console = true;
+
        /*
         * If this port is a console, then the spinlock is already
         * initialised.
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 87d4bbc..7fcdd90 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -194,6 +194,7 @@ struct uart_port {
        unsigned char           irq_wake;
        unsigned char           unused[2];
        void                    *private_data;          /* generic 
platform data pointer */
+       bool                    is_console;
 };

 static inline int serial_port_in(struct uart_port *up, int offset)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/