[PATCH 0/6] Serial Omap fixes and cleanups

[PATCH 0/6] Serial Omap fixes and cleanups

[Sourav Poddar]

Hi,

This patch series contains fixes and cleanups around the issue that 
the console UART should not idled on suspend while using "no_console_suspend"
in bootargs.

The approach thought of is to modify the serial core/serial driver to bypass
runtime PM if the UART in contention is a console and we are using "no_console_suspend"
in our bootargs.

While fixing the above issue, there are other cleanups also done as part of
this series which are no longer required. This cleanups mainly include getting
rid of using "omap_device_disable_idle_on_suspend" api for both dt and non dt case 
as the serial driver will be self sufficient to handle the "no_idle_on_suspend" issue.
Serial was the only one making use of "omap_device_disable_idle_on_suspend"

Test info (except drivers: serial: mpc52xx_uart: Remove "uart_console" defintion):
Omap4430sdp:
- Tested wakeup from UART after suspend for dt and non dt case.
Omap5430evm:
- Tested wakeup from UART after suspend for dt case.


There were discussions about how to handle "no_idle_on_suspend" issue and all the
discussions are as follows:
[v3]: https://lkml.org/lkml/2013/4/5/239
[v2]: https://lkml.org/lkml/2013/4/2/350
[v1]: https://lkml.org/lkml/2013/3/18/199
      https://lkml.org/lkml/2013/3/18/295
Due to the amount of change in approach and other cleanups coming around it, I am posting
this as a new series.

This patches are based on 3.9-rc3 custom tree which has 
Santosh Shilimkar serial patch[1]
[1]: http://permalink.gmane.org/gmane.linux.ports.arm.omap/95828

Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Rajendra nayak <rnayak@ti.com>

Sourav Poddar (6):
  drivers: tty: serial: Move "uart_console" def to core header file.
  drivers: serial: mpc52xx_uart: Remove "uart_console" defintion
  driver: serial: omap: add prepare/complete callback for
    "no_console_suspend" case
  arm: mach-omap2: remove "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check
  arm: dts: am33xx: Remove "ti,no_idle_on_suspend" property.
  arm: mach-omap2: Remove "no_console_suspend"

 arch/arm/boot/dts/am33xx.dtsi     |    1 -
 arch/arm/mach-omap2/omap_device.c |   10 +---------
 arch/arm/mach-omap2/serial.c      |    7 -------
 drivers/tty/serial/mpc52xx_uart.c |   10 ----------
 drivers/tty/serial/omap-serial.c  |   20 ++++++++++++++++++++
 drivers/tty/serial/serial_core.c  |    6 ------
 include/linux/serial_core.h       |    6 ++++++
 7 files changed, 27 insertions(+), 33 deletions(-)

[PATCH 1/6] driver: tty: serial: Move "uart_console" def to core header file.

[Sourav Poddar]

Move "uart_console" definition to serial core header file, so that it can be
used by serial drivers.

Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Rajendra nayak <rnayak@ti.com>
Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
 drivers/tty/serial/serial_core.c |    6 ------
 include/linux/serial_core.h      |    6 ++++++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index a400002..b5f74bf 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -50,12 +50,6 @@ static struct lock_class_key port_lock_key;
 
 #define HIGH_BITS_OFFSET	((sizeof(long)-sizeof(int))*8)
 
-#ifdef CONFIG_SERIAL_CORE_CONSOLE
-#define uart_console(port)	((port)->cons && (port)->cons->index == (port)->line)
-#else
-#define uart_console(port)	(0)
-#endif
-
 static void uart_change_speed(struct tty_struct *tty, struct uart_state *state,
 					struct ktermios *old_termios);
 static void uart_wait_until_sent(struct tty_struct *tty, int timeout);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 87d4bbc..a6f27f2 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -31,6 +31,12 @@
 #include <linux/sysrq.h>
 #include <uapi/linux/serial_core.h>
 
+#ifdef CONFIG_SERIAL_CORE_CONSOLE
+#define uart_console(port)      ((port)->cons && (port)->cons->index == (port)->line)
+#else
+#define uart_console(port)      (0)
+#endif
+
 struct uart_port;
 struct serial_struct;
 struct device;
-- 
1.7.1

[RFT/PATCH 2/6] driver: serial: mpc52xx_uart: Remove "uart_console" defintion

[Sourav Poddar]

Since "uart_console" definition is now moved to serial core
haeder file . Serial drivers need not define them.

Cc: Sylvain Munaut <tnt@246tNt.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Rajendra nayak <rnayak@ti.com>
Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
 drivers/tty/serial/mpc52xx_uart.c |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/drivers/tty/serial/mpc52xx_uart.c b/drivers/tty/serial/mpc52xx_uart.c
index 018bad9..d74ac06 100644
--- a/drivers/tty/serial/mpc52xx_uart.c
+++ b/drivers/tty/serial/mpc52xx_uart.c
@@ -84,16 +84,6 @@ static void mpc52xx_uart_of_enumerate(void);
 static irqreturn_t mpc52xx_uart_int(int irq, void *dev_id);
 static irqreturn_t mpc5xxx_uart_process_int(struct uart_port *port);
 
-
-/* Simple macro to test if a port is console or not. This one is taken
- * for serial_core.c and maybe should be moved to serial_core.h ? */
-#ifdef CONFIG_SERIAL_CORE_CONSOLE
-#define uart_console(port) \
-	((port)->cons && (port)->cons->index == (port)->line)
-#else
-#define uart_console(port)	(0)
-#endif
-
 /* ======================================================================== */
 /* PSC fifo operations for isolating differences between 52xx and 512x      */
 /* ======================================================================== */
-- 
1.7.1

[PATCH 3/6] driver: serial: omap: add prepare/complete callback for "no_console_suspend" case

[Sourav Poddar]

The patch adapt the serial core/driver to take care of the case when "no_console_suspend"
is used in the bootargs. The 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.

Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
 drivers/tty/serial/omap-serial.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 08332f3..9ef80cf 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 && uart_console(&up->port))
+		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 && uart_console(&up->port))
+		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)
-- 
1.7.1

[PATCH 4/6] arm: mach-omap2: remove "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check

[Sourav Poddar]

Remove the "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check, since UART was the only one making
use of it. Now serial core/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.

Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
 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);
 	}
 
-- 
1.7.1

[PATCH 5/6] arm: dts: am33xx: Remove "ti,no_idle_on_suspend" property.

[Sourav Poddar]

The "ti,no_idle_on_suspend" property was required to keep ocmcram 
clocks running during idle.

But the commit below[1], added in v3.6 should prevent the
any automaic clock gating for devices without drivers bound.  Since
there is no driver for the OCM RAM block, we are not affected by
the automatic idle on suspend anyways.
[1]:
commit 72bb6f9b51c82c820ddef892455a85b115460904
Author: Kevin Hilman <khilman@ti.com>
Date:   Tue Jul 10 15:29:04 2012 -0700

    ARM: OMAP: omap_device: don't attempt late suspend if no driver bound

    Currently, the omap_device PM domain layer uses the late suspend and
    early resume callbacks to ensure devices are in their low power
    states.

    However, this is attempted even in cases where a driver probe has
    failed.  If a driver's ->probe() method fails, the driver is likely in
    a state where it is not expecting its runtime PM callbacks to be
    called, yet currently the omap_device PM domain code attempts to call
    the drivers callbacks.

    To fix, use the omap_device driver_status field to check whether a
    driver is bound to the omap_device before attempting to trigger driver
    callbacks.

Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Rajendra nayak <rnayak@ti.com>
Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
 arch/arm/boot/dts/am33xx.dtsi |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 74a8125..49a050e 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -394,7 +394,6 @@
 			compatible = "ti,am3352-ocmcram";
 			reg = <0x40300000 0x10000>;
 			ti,hwmods = "ocmcram";
-			ti,no_idle_on_suspend;
 		};
 
 		wkup_m3: wkup_m3@44d00000 {
-- 
1.7.1

[PATCH 6/6] arm: mach-omap2: Remove "no_console_suspend"

[Sourav Poddar]

Since the omap serial driver is now adapted to handle the
case where you dont need to cut the clock on suspend while
using "no_console_suspend" in the bootargs.

We can get rid of the previous implementation of setting the od->flags to
"OMAP_DEVICE_NO_IDLE_ON_SUSPEND" from platform
and omap_device files.

Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Rajendra nayak <rnayak@ti.com>
Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>
---
 arch/arm/mach-omap2/omap_device.c |    3 ---
 arch/arm/mach-omap2/serial.c      |    7 -------
 2 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
index d6dce8f..c226946 100644
--- a/arch/arm/mach-omap2/omap_device.c
+++ b/arch/arm/mach-omap2/omap_device.c
@@ -170,9 +170,6 @@ static int omap_device_build_from_dt(struct platform_device *pdev)
 			r->name = dev_name(&pdev->dev);
 	}
 
-	if (of_get_property(node, "ti,no_idle_on_suspend", NULL))
-		omap_device_disable_idle_on_suspend(pdev);
-
 	pdev->dev.pm_domain = &omap_device_pm_domain;
 
 odbfd_exit1:
diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index f660156..58d5b56 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -63,7 +63,6 @@ struct omap_uart_state {
 static LIST_HEAD(uart_list);
 static u8 num_uarts;
 static u8 console_uart_id = -1;
-static u8 no_console_suspend;
 static u8 uart_debug;
 
 #define DEFAULT_RXDMA_POLLRATE		1	/* RX DMA polling rate (us) */
@@ -207,9 +206,6 @@ static int __init omap_serial_early_init(void)
 					uart_name, uart->num);
 			}
 
-			if (cmdline_find_option("no_console_suspend"))
-				no_console_suspend = true;
-
 			/*
 			 * omap-uart can be used for earlyprintk logs
 			 * So if omap-uart is used as console then prevent
@@ -292,9 +288,6 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
 		return;
 	}
 
-	if ((console_uart_id == bdata->id) && no_console_suspend)
-		omap_device_disable_idle_on_suspend(pdev);
-
 	oh->mux = omap_hwmod_mux_init(bdata->pads, bdata->pads_cnt);
 
 	if (console_uart_id == bdata->id) {
-- 
1.7.1

Re: [RFT/PATCH 2/6] driver: serial: mpc52xx_uart: Remove "uart_console" defintion

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 503 bytes --]

On Wed, Apr 17, 2013 at 05:04:23PM +0530, Sourav Poddar wrote:
> Since "uart_console" definition is now moved to serial core
> haeder file . Serial drivers need not define them.
> 
> Cc: Sylvain Munaut <tnt@246tNt.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Rajendra nayak <rnayak@ti.com>
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>

looks like it should be merged with previous patch to avoid redefinition
errors.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/6] driver: serial: omap: add prepare/complete callback for "no_console_suspend" case

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 488 bytes --]

Hi,

On Wed, Apr 17, 2013 at 05:04:24PM +0530, Sourav Poddar wrote:
> @@ -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 CONFIG_PM_SLEEP isn't defined, this will break compilation.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFT/PATCH 2/6] driver: serial: mpc52xx_uart: Remove "uart_console" defintion

[Sourav Poddar]

On Thursday 18 April 2013 09:26 AM, Felipe Balbi wrote:
> On Wed, Apr 17, 2013 at 05:04:23PM +0530, Sourav Poddar wrote:
>> Since "uart_console" definition is now moved to serial core
>> haeder file . Serial drivers need not define them.
>>
>> Cc: Sylvain Munaut<tnt@246tNt.com>
>> Cc: Santosh Shilimkar<santosh.shilimkar@ti.com>
>> Cc: Felipe Balbi<balbi@ti.com>
>> Cc: Rajendra nayak<rnayak@ti.com>
>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com>
> looks like it should be merged with previous patch to avoid redefinition
> errors.
>
true..i can squash it with the previous patch.

Re: [RFT/PATCH 2/6] driver: serial: mpc52xx_uart: Remove "uart_console" defintion

[Russell King - ARM Linux]

On Wed, Apr 17, 2013 at 05:04:23PM +0530, Sourav Poddar wrote:
> Since "uart_console" definition is now moved to serial core
> haeder file . Serial drivers need not define them.

This needs to be part of patch 1.  Having it separate may provoke compiler
warnings.

Re: [RFT/PATCH 2/6] driver: serial: mpc52xx_uart: Remove "uart_console" defintion

[Sourav Poddar]

Hi Russell,
On Thursday 18 April 2013 04:20 PM, Russell King - ARM Linux wrote:
> On Wed, Apr 17, 2013 at 05:04:23PM +0530, Sourav Poddar wrote:
>> Since "uart_console" definition is now moved to serial core
>> haeder file . Serial drivers need not define them.
> This needs to be part of patch 1.  Having it separate may provoke compiler
> warnings.
Ok. I will fold it in the first patch in my next version.

Thanks,
Sourav

Re: [PATCH 3/6] driver: serial: omap: add prepare/complete callback for "no_console_suspend" case

[Sourav Poddar]

Hi Felipe,
On Thursday 18 April 2013 09:28 AM, Felipe Balbi wrote:
> Hi,
>
> On Wed, Apr 17, 2013 at 05:04:24PM +0530, Sourav Poddar wrote:
>> @@ -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 CONFIG_PM_SLEEP isn't defined, this will break compilation.
>
True.

Then, will it not be  a better idea to add a similar macro[1] in 
include/linux/pm.h for
prepare/complete callback as it is present for suspend/resume ?.

[1]:
#ifdef CONFIG_PM_SLEEP
#define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
         .suspend = suspend_fn, \
         .resume = resume_fn, \
         .freeze = suspend_fn, \
         .thaw = resume_fn, \
         .poweroff = suspend_fn, \
         .restore = resume_fn,
#else
#define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
#endif



~Sourav

Re: [PATCH 3/6] driver: serial: omap: add prepare/complete callback for "no_console_suspend" case

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1235 bytes --]

Hi,

On Thu, Apr 18, 2013 at 05:37:48PM +0530, Sourav Poddar wrote:
> Hi Felipe,
> On Thursday 18 April 2013 09:28 AM, Felipe Balbi wrote:
> >Hi,
> >
> >On Wed, Apr 17, 2013 at 05:04:24PM +0530, Sourav Poddar wrote:
> >>@@ -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 CONFIG_PM_SLEEP isn't defined, this will break compilation.
> >
> True.
> 
> Then, will it not be  a better idea to add a similar macro[1] in
> include/linux/pm.h for
> prepare/complete callback as it is present for suspend/resume ?.
> 
> [1]:
> #ifdef CONFIG_PM_SLEEP
> #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \
>         .suspend = suspend_fn, \
>         .resume = resume_fn, \
>         .freeze = suspend_fn, \
>         .thaw = resume_fn, \
>         .poweroff = suspend_fn, \
>         .restore = resume_fn,
> #else
> #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn)
> #endif

might be :-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/6] driver: serial: omap: add prepare/complete callback for "no_console_suspend" case

[Kevin Hilman]

Sourav Poddar <sourav.poddar@ti.com> writes:

> The patch adapt the serial core/driver to take care of the case when "no_console_suspend"
> is used in the bootargs. The 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.
>
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>

This changelog needs a rework.  The driver itself was not aware of
od->flags and omap_device stuff in general, so it's not really
relevant.  The driver is also not directly managing clocks, int's only
doing runtime PM callbacks.

What you want to say in the changelog is that the driver manages
"no_console_suspend" by preventing runtime PM during the suspend path,
which forces the console UART to stay awake.

> ---
>  drivers/tty/serial/omap-serial.c |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index 08332f3..9ef80cf 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 && uart_console(&up->port))
> +		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 && uart_console(&up->port))
> +		pm_runtime_enable(dev);
> +}
> +

For compilation with !CONFIG_PM_SLEEP, you'll also need:

#else 
#define serial_omap_prepare NULL
#define serial_omap_prepare NULL
#endif /* CONFIG_PM_SLEEP */

>  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)

Kevin

Re: [PATCH 4/6] arm: mach-omap2: remove "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check

[Kevin Hilman]

Sourav Poddar <sourav.poddar@ti.com> writes:

> Remove the "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check, since UART was the only one making
> use of it. Now serial core/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.
>
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>

NAK.  This patch will break many things...

> ---
>  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);

Why did you remove the omap_device_idle() here?

> +		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);

And the _enable() here?

>  		pm_generic_runtime_resume(dev);
>  	}

Note that the check is for when the flag is *not* set, so this patch
changes behavior for all the drivers that do not use
_NO_IDLE_ON_SUSPEND.  I think that's the opposite of what you intended.

Kevin

Re: [PATCH 6/6] arm: mach-omap2: Remove "no_console_suspend"

[Kevin Hilman]

Sourav Poddar <sourav.poddar@ti.com> writes:

> Since the omap serial driver is now adapted to handle the
> case where you dont need to cut the clock on suspend while
> using "no_console_suspend" in the bootargs.
>
> We can get rid of the previous implementation of setting the od->flags to
> "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" from platform
> and omap_device files.
>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Rajendra nayak <rnayak@ti.com>
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>

Subject should be arm: omap2+: omap_device: remove no_idle_on_suspend

Also, you should remove the flag from omap_device.h.

Kevin

> ---
>  arch/arm/mach-omap2/omap_device.c |    3 ---
>  arch/arm/mach-omap2/serial.c      |    7 -------
>  2 files changed, 0 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
> index d6dce8f..c226946 100644
> --- a/arch/arm/mach-omap2/omap_device.c
> +++ b/arch/arm/mach-omap2/omap_device.c
> @@ -170,9 +170,6 @@ static int omap_device_build_from_dt(struct platform_device *pdev)
>  			r->name = dev_name(&pdev->dev);
>  	}
>  
> -	if (of_get_property(node, "ti,no_idle_on_suspend", NULL))
> -		omap_device_disable_idle_on_suspend(pdev);
> -
>  	pdev->dev.pm_domain = &omap_device_pm_domain;
>  
>  odbfd_exit1:
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index f660156..58d5b56 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -63,7 +63,6 @@ struct omap_uart_state {
>  static LIST_HEAD(uart_list);
>  static u8 num_uarts;
>  static u8 console_uart_id = -1;
> -static u8 no_console_suspend;
>  static u8 uart_debug;
>  
>  #define DEFAULT_RXDMA_POLLRATE		1	/* RX DMA polling rate (us) */
> @@ -207,9 +206,6 @@ static int __init omap_serial_early_init(void)
>  					uart_name, uart->num);
>  			}
>  
> -			if (cmdline_find_option("no_console_suspend"))
> -				no_console_suspend = true;
> -
>  			/*
>  			 * omap-uart can be used for earlyprintk logs
>  			 * So if omap-uart is used as console then prevent
> @@ -292,9 +288,6 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
>  		return;
>  	}
>  
> -	if ((console_uart_id == bdata->id) && no_console_suspend)
> -		omap_device_disable_idle_on_suspend(pdev);
> -
>  	oh->mux = omap_hwmod_mux_init(bdata->pads, bdata->pads_cnt);
>  
>  	if (console_uart_id == bdata->id) {

Re: [PATCH 3/6] driver: serial: omap: add prepare/complete callback for "no_console_suspend" case

[Sourav Poddar]

Hi Kevin,
On Thursday 18 April 2013 11:26 PM, Kevin Hilman wrote:
> Sourav Poddar<sourav.poddar@ti.com>  writes:
>
>> The patch adapt the serial core/driver to take care of the case when "no_console_suspend"
>> is used in the bootargs. The 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.
>>
>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com>
> This changelog needs a rework.  The driver itself was not aware of
> od->flags and omap_device stuff in general, so it's not really
> relevant.  The driver is also not directly managing clocks, int's only
> doing runtime PM callbacks.
>
> What you want to say in the changelog is that the driver manages
> "no_console_suspend" by preventing runtime PM during the suspend path,
> which forces the console UART to stay awake.
>
Yes, looks to the point. Will update the changelog in the next version.
>> ---
>>   drivers/tty/serial/omap-serial.c |   20 ++++++++++++++++++++
>>   1 files changed, 20 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>> index 08332f3..9ef80cf 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&&  uart_console(&up->port))
>> +		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&&  uart_console(&up->port))
>> +		pm_runtime_enable(dev);
>> +}
>> +
> For compilation with !CONFIG_PM_SLEEP, you'll also need:
>
> #else
> #define serial_omap_prepare NULL
> #define serial_omap_prepare NULL
> #endif /* CONFIG_PM_SLEEP */
>
Ok. Will change this.
Though, just a query/proposal on this, will it be correct if we try to 
create a macro[1]
in include/linux/pm.h for prepare/complete as it is done for 
suspend/resume. ?

[1]:
#ifdef CONFIG_PM_SLEEP
#define SET_SYSTEM_SLEEP_PM_PREP_COMP_OPS(prepare_fn, complete_fn) \
         .prepare = prepare_fn, \
         .complete = complete_fn, \
#else
#define SET_SYSTEM_SLEEP_PM_PREP_COMP_OPS(prepare_fn, complete_fn)
#endif

~Sourav
>>   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)
> Kevin

Re: [PATCH 6/6] arm: mach-omap2: Remove "no_console_suspend"

[Kevin Hilman]

Sourav Poddar <sourav.poddar@ti.com> writes:

> Since the omap serial driver is now adapted to handle the
> case where you dont need to cut the clock on suspend while
> using "no_console_suspend" in the bootargs.
>
> We can get rid of the previous implementation of setting the od->flags to
> "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" from platform
> and omap_device files.
>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Rajendra nayak <rnayak@ti.com>
> Signed-off-by: Sourav Poddar <sourav.poddar@ti.com>

also, the serial.c change here should be a separate patch
subject: arm: omap2+: serial: remove no_console_suspend support

with a changelog stating that it's no longer handled in the core.

Kevin

> ---
>  arch/arm/mach-omap2/omap_device.c |    3 ---
>  arch/arm/mach-omap2/serial.c      |    7 -------
>  2 files changed, 0 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
> index d6dce8f..c226946 100644
> --- a/arch/arm/mach-omap2/omap_device.c
> +++ b/arch/arm/mach-omap2/omap_device.c
> @@ -170,9 +170,6 @@ static int omap_device_build_from_dt(struct platform_device *pdev)
>  			r->name = dev_name(&pdev->dev);
>  	}
>  
> -	if (of_get_property(node, "ti,no_idle_on_suspend", NULL))
> -		omap_device_disable_idle_on_suspend(pdev);
> -
>  	pdev->dev.pm_domain = &omap_device_pm_domain;
>  
>  odbfd_exit1:
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index f660156..58d5b56 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -63,7 +63,6 @@ struct omap_uart_state {
>  static LIST_HEAD(uart_list);
>  static u8 num_uarts;
>  static u8 console_uart_id = -1;
> -static u8 no_console_suspend;
>  static u8 uart_debug;
>  
>  #define DEFAULT_RXDMA_POLLRATE		1	/* RX DMA polling rate (us) */
> @@ -207,9 +206,6 @@ static int __init omap_serial_early_init(void)
>  					uart_name, uart->num);
>  			}
>  
> -			if (cmdline_find_option("no_console_suspend"))
> -				no_console_suspend = true;
> -
>  			/*
>  			 * omap-uart can be used for earlyprintk logs
>  			 * So if omap-uart is used as console then prevent
> @@ -292,9 +288,6 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
>  		return;
>  	}
>  
> -	if ((console_uart_id == bdata->id) && no_console_suspend)
> -		omap_device_disable_idle_on_suspend(pdev);
> -
>  	oh->mux = omap_hwmod_mux_init(bdata->pads, bdata->pads_cnt);
>  
>  	if (console_uart_id == bdata->id) {

Re: [PATCH 0/6] Serial Omap fixes and cleanups

[Kevin Hilman]

Hi Sourav,

Sourav Poddar <sourav.poddar@ti.com> writes:

> Hi,
>
> This patch series contains fixes and cleanups around the issue that 
> the console UART should not idled on suspend while using "no_console_suspend"
> in bootargs.
>

The direction of the series is right, thanks for the updated approach.
I had a comple minor comments on specific patches, but the ordering of
the series needs a little tweaking.  It should be

- core/driver changes [current 1-3/6 are ok]
- remove usage from mach-omap2/serial.c (currently part of 4/6)
- remove am33x DT usage (current 5/6 is ok)
- remove entirely from omap_device (omap_device part of 4/6 and 6/6 should be combined)

Kevin

> The approach thought of is to modify the serial core/serial driver to bypass
> runtime PM if the UART in contention is a console and we are using "no_console_suspend"
> in our bootargs.
>
> While fixing the above issue, there are other cleanups also done as part of
> this series which are no longer required. This cleanups mainly include getting
> rid of using "omap_device_disable_idle_on_suspend" api for both dt and non dt case 
> as the serial driver will be self sufficient to handle the "no_idle_on_suspend" issue.
> Serial was the only one making use of "omap_device_disable_idle_on_suspend"
>
> Test info (except drivers: serial: mpc52xx_uart: Remove "uart_console" defintion):
> Omap4430sdp:
> - Tested wakeup from UART after suspend for dt and non dt case.
> Omap5430evm:
> - Tested wakeup from UART after suspend for dt case.
>
>
> There were discussions about how to handle "no_idle_on_suspend" issue and all the
> discussions are as follows:
> [v3]: https://lkml.org/lkml/2013/4/5/239
> [v2]: https://lkml.org/lkml/2013/4/2/350
> [v1]: https://lkml.org/lkml/2013/3/18/199
>       https://lkml.org/lkml/2013/3/18/295
> Due to the amount of change in approach and other cleanups coming around it, I am posting
> this as a new series.
>
> This patches are based on 3.9-rc3 custom tree which has 
> Santosh Shilimkar serial patch[1]
> [1]: http://permalink.gmane.org/gmane.linux.ports.arm.omap/95828
>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Rajendra nayak <rnayak@ti.com>
>
> Sourav Poddar (6):
>   drivers: tty: serial: Move "uart_console" def to core header file.
>   drivers: serial: mpc52xx_uart: Remove "uart_console" defintion
>   driver: serial: omap: add prepare/complete callback for
>     "no_console_suspend" case
>   arm: mach-omap2: remove "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check
>   arm: dts: am33xx: Remove "ti,no_idle_on_suspend" property.
>   arm: mach-omap2: Remove "no_console_suspend"
>
>  arch/arm/boot/dts/am33xx.dtsi     |    1 -
>  arch/arm/mach-omap2/omap_device.c |   10 +---------
>  arch/arm/mach-omap2/serial.c      |    7 -------
>  drivers/tty/serial/mpc52xx_uart.c |   10 ----------
>  drivers/tty/serial/omap-serial.c  |   20 ++++++++++++++++++++
>  drivers/tty/serial/serial_core.c  |    6 ------
>  include/linux/serial_core.h       |    6 ++++++
>  7 files changed, 27 insertions(+), 33 deletions(-)

Re: [PATCH 4/6] arm: mach-omap2: remove "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check

[Sourav Poddar]

On Thursday 18 April 2013 11:35 PM, Kevin Hilman wrote:
> Sourav Poddar<sourav.poddar@ti.com>  writes:
>
>> Remove the "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check, since UART was the only one making
>> use of it. Now serial core/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.
>>
>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com>
> NAK.  This patch will break many things...
>
>> ---
>>   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);
> Why did you remove the omap_device_idle() here?
This patch is used along with patch3 to get rid of the issue. I posted 
them as a
seperate patch beacuse of the subject line as one goes under drivers/* 
and the other
arm/mach-omap2/*..

This check was only valid for UART, and if od->flags is set to the
"OMAP_DEVICE_NO_IDLE_ON_SUSPEND" flag, then UART will not be idled. But now,
we no longer depend on od->flag value to prevent idling of our console 
UART as the
prepare/complete apis will take care of them.

>> +		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);
> And the _enable() here?
>
>>   		pm_generic_runtime_resume(dev);
>>   	}
> Note that the check is for when the flag is *not* set, so this patch
> changes behavior for all the drivers that do not use
> _NO_IDLE_ON_SUSPEND.  I think that's the opposite of what you intended.
>
> Kevin

Re: [PATCH 6/6] arm: mach-omap2: Remove "no_console_suspend"

[Sourav Poddar]

Hi Kevin,
On Thursday 18 April 2013 11:39 PM, Kevin Hilman wrote:
> Sourav Poddar<sourav.poddar@ti.com>  writes:
>
>> Since the omap serial driver is now adapted to handle the
>> case where you dont need to cut the clock on suspend while
>> using "no_console_suspend" in the bootargs.
>>
>> We can get rid of the previous implementation of setting the od->flags to
>> "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" from platform
>> and omap_device files.
>>
>> Cc: Santosh Shilimkar<santosh.shilimkar@ti.com>
>> Cc: Felipe Balbi<balbi@ti.com>
>> Cc: Rajendra nayak<rnayak@ti.com>
>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com>
> Subject should be arm: omap2+: omap_device: remove no_idle_on_suspend
>
> Also, you should remove the flag from omap_device.h.
>
> Kevin
Ok. Will change it in the next version.

~Sourav
>> ---
>>   arch/arm/mach-omap2/omap_device.c |    3 ---
>>   arch/arm/mach-omap2/serial.c      |    7 -------
>>   2 files changed, 0 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
>> index d6dce8f..c226946 100644
>> --- a/arch/arm/mach-omap2/omap_device.c
>> +++ b/arch/arm/mach-omap2/omap_device.c
>> @@ -170,9 +170,6 @@ static int omap_device_build_from_dt(struct platform_device *pdev)
>>   			r->name = dev_name(&pdev->dev);
>>   	}
>>
>> -	if (of_get_property(node, "ti,no_idle_on_suspend", NULL))
>> -		omap_device_disable_idle_on_suspend(pdev);
>> -
>>   	pdev->dev.pm_domain =&omap_device_pm_domain;
>>
>>   odbfd_exit1:
>> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
>> index f660156..58d5b56 100644
>> --- a/arch/arm/mach-omap2/serial.c
>> +++ b/arch/arm/mach-omap2/serial.c
>> @@ -63,7 +63,6 @@ struct omap_uart_state {
>>   static LIST_HEAD(uart_list);
>>   static u8 num_uarts;
>>   static u8 console_uart_id = -1;
>> -static u8 no_console_suspend;
>>   static u8 uart_debug;
>>
>>   #define DEFAULT_RXDMA_POLLRATE		1	/* RX DMA polling rate (us) */
>> @@ -207,9 +206,6 @@ static int __init omap_serial_early_init(void)
>>   					uart_name, uart->num);
>>   			}
>>
>> -			if (cmdline_find_option("no_console_suspend"))
>> -				no_console_suspend = true;
>> -
>>   			/*
>>   			 * omap-uart can be used for earlyprintk logs
>>   			 * So if omap-uart is used as console then prevent
>> @@ -292,9 +288,6 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
>>   		return;
>>   	}
>>
>> -	if ((console_uart_id == bdata->id)&&  no_console_suspend)
>> -		omap_device_disable_idle_on_suspend(pdev);
>> -
>>   	oh->mux = omap_hwmod_mux_init(bdata->pads, bdata->pads_cnt);
>>
>>   	if (console_uart_id == bdata->id) {

Re: [PATCH 6/6] arm: mach-omap2: Remove "no_console_suspend"

[Sourav Poddar]

Hi Kevin,
On Thursday 18 April 2013 11:41 PM, Kevin Hilman wrote:
> Sourav Poddar<sourav.poddar@ti.com>  writes:
>
>> Since the omap serial driver is now adapted to handle the
>> case where you dont need to cut the clock on suspend while
>> using "no_console_suspend" in the bootargs.
>>
>> We can get rid of the previous implementation of setting the od->flags to
>> "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" from platform
>> and omap_device files.
>>
>> Cc: Santosh Shilimkar<santosh.shilimkar@ti.com>
>> Cc: Felipe Balbi<balbi@ti.com>
>> Cc: Rajendra nayak<rnayak@ti.com>
>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com>
> also, the serial.c change here should be a separate patch
> subject: arm: omap2+: serial: remove no_console_suspend support
>
> with a changelog stating that it's no longer handled in the core.
>
Ok. Will create a seperate patch and update the changelog.
> Kevin
>
>> ---
>>   arch/arm/mach-omap2/omap_device.c |    3 ---
>>   arch/arm/mach-omap2/serial.c      |    7 -------
>>   2 files changed, 0 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
>> index d6dce8f..c226946 100644
>> --- a/arch/arm/mach-omap2/omap_device.c
>> +++ b/arch/arm/mach-omap2/omap_device.c
>> @@ -170,9 +170,6 @@ static int omap_device_build_from_dt(struct platform_device *pdev)
>>   			r->name = dev_name(&pdev->dev);
>>   	}
>>
>> -	if (of_get_property(node, "ti,no_idle_on_suspend", NULL))
>> -		omap_device_disable_idle_on_suspend(pdev);
>> -
>>   	pdev->dev.pm_domain =&omap_device_pm_domain;
>>
>>   odbfd_exit1:
>> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
>> index f660156..58d5b56 100644
>> --- a/arch/arm/mach-omap2/serial.c
>> +++ b/arch/arm/mach-omap2/serial.c
>> @@ -63,7 +63,6 @@ struct omap_uart_state {
>>   static LIST_HEAD(uart_list);
>>   static u8 num_uarts;
>>   static u8 console_uart_id = -1;
>> -static u8 no_console_suspend;
>>   static u8 uart_debug;
>>
>>   #define DEFAULT_RXDMA_POLLRATE		1	/* RX DMA polling rate (us) */
>> @@ -207,9 +206,6 @@ static int __init omap_serial_early_init(void)
>>   					uart_name, uart->num);
>>   			}
>>
>> -			if (cmdline_find_option("no_console_suspend"))
>> -				no_console_suspend = true;
>> -
>>   			/*
>>   			 * omap-uart can be used for earlyprintk logs
>>   			 * So if omap-uart is used as console then prevent
>> @@ -292,9 +288,6 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
>>   		return;
>>   	}
>>
>> -	if ((console_uart_id == bdata->id)&&  no_console_suspend)
>> -		omap_device_disable_idle_on_suspend(pdev);
>> -
>>   	oh->mux = omap_hwmod_mux_init(bdata->pads, bdata->pads_cnt);
>>
>>   	if (console_uart_id == bdata->id) {

Re: [PATCH 0/6] Serial Omap fixes and cleanups

[Sourav Poddar]

Hi Kevin,
On Thursday 18 April 2013 11:53 PM, Kevin Hilman wrote:
> Hi Sourav,
>
> Sourav Poddar<sourav.poddar@ti.com>  writes:
>
>> Hi,
>>
>> This patch series contains fixes and cleanups around the issue that
>> the console UART should not idled on suspend while using "no_console_suspend"
>> in bootargs.
>>
> The direction of the series is right, thanks for the updated approach.
> I had a comple minor comments on specific patches, but the ordering of
> the series needs a little tweaking.  It should be
>
> - core/driver changes [current 1-3/6 are ok]
> - remove usage from mach-omap2/serial.c (currently part of 4/6)
> - remove am33x DT usage (current 5/6 is ok)
> - remove entirely from omap_device (omap_device part of 4/6 and 6/6 should be combined)
>
Thanks a lot for your review and comments.
I have replied to your comments on the respective patches.
Will take care of the "ordering" which you mentioned above
in the next version.

Thanks
Sourav
> Kevin
>
>> The approach thought of is to modify the serial core/serial driver to bypass
>> runtime PM if the UART in contention is a console and we are using "no_console_suspend"
>> in our bootargs.
>>
>> While fixing the above issue, there are other cleanups also done as part of
>> this series which are no longer required. This cleanups mainly include getting
>> rid of using "omap_device_disable_idle_on_suspend" api for both dt and non dt case
>> as the serial driver will be self sufficient to handle the "no_idle_on_suspend" issue.
>> Serial was the only one making use of "omap_device_disable_idle_on_suspend"
>>
>> Test info (except drivers: serial: mpc52xx_uart: Remove "uart_console" defintion):
>> Omap4430sdp:
>> - Tested wakeup from UART after suspend for dt and non dt case.
>> Omap5430evm:
>> - Tested wakeup from UART after suspend for dt case.
>>
>>
>> There were discussions about how to handle "no_idle_on_suspend" issue and all the
>> discussions are as follows:
>> [v3]: https://lkml.org/lkml/2013/4/5/239
>> [v2]: https://lkml.org/lkml/2013/4/2/350
>> [v1]: https://lkml.org/lkml/2013/3/18/199
>>        https://lkml.org/lkml/2013/3/18/295
>> Due to the amount of change in approach and other cleanups coming around it, I am posting
>> this as a new series.
>>
>> This patches are based on 3.9-rc3 custom tree which has
>> Santosh Shilimkar serial patch[1]
>> [1]: http://permalink.gmane.org/gmane.linux.ports.arm.omap/95828
>>
>> Cc: Santosh Shilimkar<santosh.shilimkar@ti.com>
>> Cc: Felipe Balbi<balbi@ti.com>
>> Cc: Rajendra nayak<rnayak@ti.com>
>>
>> Sourav Poddar (6):
>>    drivers: tty: serial: Move "uart_console" def to core header file.
>>    drivers: serial: mpc52xx_uart: Remove "uart_console" defintion
>>    driver: serial: omap: add prepare/complete callback for
>>      "no_console_suspend" case
>>    arm: mach-omap2: remove "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check
>>    arm: dts: am33xx: Remove "ti,no_idle_on_suspend" property.
>>    arm: mach-omap2: Remove "no_console_suspend"
>>
>>   arch/arm/boot/dts/am33xx.dtsi     |    1 -
>>   arch/arm/mach-omap2/omap_device.c |   10 +---------
>>   arch/arm/mach-omap2/serial.c      |    7 -------
>>   drivers/tty/serial/mpc52xx_uart.c |   10 ----------
>>   drivers/tty/serial/omap-serial.c  |   20 ++++++++++++++++++++
>>   drivers/tty/serial/serial_core.c  |    6 ------
>>   include/linux/serial_core.h       |    6 ++++++
>>   7 files changed, 27 insertions(+), 33 deletions(-)

Re: [PATCH 3/6] driver: serial: omap: add prepare/complete callback for "no_console_suspend" case

[Kevin Hilman]

Sourav Poddar <sourav.poddar@ti.com> writes:

> Hi Kevin,
> On Thursday 18 April 2013 11:26 PM, Kevin Hilman wrote:
>> Sourav Poddar<sourav.poddar@ti.com>  writes:
>>
>>> The patch adapt the serial core/driver to take care of the case when "no_console_suspend"
>>> is used in the bootargs. The 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.
>>>
>>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com>
>> This changelog needs a rework.  The driver itself was not aware of
>> od->flags and omap_device stuff in general, so it's not really
>> relevant.  The driver is also not directly managing clocks, int's only
>> doing runtime PM callbacks.
>>
>> What you want to say in the changelog is that the driver manages
>> "no_console_suspend" by preventing runtime PM during the suspend path,
>> which forces the console UART to stay awake.
>>
> Yes, looks to the point. Will update the changelog in the next version.
>>> ---
>>>   drivers/tty/serial/omap-serial.c |   20 ++++++++++++++++++++
>>>   1 files changed, 20 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>>> index 08332f3..9ef80cf 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&&  uart_console(&up->port))
>>> +		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&&  uart_console(&up->port))
>>> +		pm_runtime_enable(dev);
>>> +}
>>> +
>> For compilation with !CONFIG_PM_SLEEP, you'll also need:
>>
>> #else
>> #define serial_omap_prepare NULL
>> #define serial_omap_prepare NULL
>> #endif /* CONFIG_PM_SLEEP */
>>
> Ok. Will change this.
> Though, just a query/proposal on this, will it be correct if we try to
> create a macro[1]
> in include/linux/pm.h for prepare/complete as it is done for
> suspend/resume. ?

Sure, you could do that, but personally I don't think it's as broadly
useful (otherwise, it would be there already.)

Kevin

> [1]:
> #ifdef CONFIG_PM_SLEEP
> #define SET_SYSTEM_SLEEP_PM_PREP_COMP_OPS(prepare_fn, complete_fn) \
>         .prepare = prepare_fn, \
>         .complete = complete_fn, \
> #else
> #define SET_SYSTEM_SLEEP_PM_PREP_COMP_OPS(prepare_fn, complete_fn)
> #endif
>
> ~Sourav
>>>   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)
>> Kevin

Re: [PATCH 4/6] arm: mach-omap2: remove "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check

[Kevin Hilman]

Sourav Poddar <sourav.poddar@ti.com> writes:

> On Thursday 18 April 2013 11:35 PM, Kevin Hilman wrote:
>> Sourav Poddar<sourav.poddar@ti.com>  writes:
>>
>>> Remove the "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check, since UART was the only one making
>>> use of it. Now serial core/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.
>>>
>>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com>
>> NAK.  This patch will break many things...
>>
>>> ---
>>>   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);
>> Why did you remove the omap_device_idle() here?
> This patch is used along with patch3 to get rid of the issue. I posted
> them as a
> seperate patch beacuse of the subject line as one goes under drivers/*
> and the other
> arm/mach-omap2/*..
>
> This check was only valid for UART, and if od->flags is set to the
> "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" flag, then UART will not be idled. 

correct, but *every other device* would be idled (if not already idle.)

> But now, we no longer depend on od->flag value to prevent idling of
> our console UART as the prepare/complete apis will take care of them.

Right, so removing the check on od->flags is fine, but what I asked
about is why you removed the omap_device_idle() call.

Remember that this code is called for *every* omap_device during
suspend, not just UART.

What you did stops *every* device from being idled during suspend.

You didn't read my whole message.  Specifically this part:

>> Note that the check is for when the flag is *not* set, so this patch
>> changes behavior for all the drivers that do not use
>> _NO_IDLE_ON_SUSPEND.  I think that's the opposite of what you intended.

Kevin

Re: [PATCH 0/6] Serial Omap fixes and cleanups

[Grygorii Strashko]

On 04/18/2013 10:17 PM, Sourav Poddar wrote:
> Hi Kevin,
> On Thursday 18 April 2013 11:53 PM, Kevin Hilman wrote:
>> Hi Sourav,
>>
>> Sourav Poddar<sourav.poddar@ti.com>  writes:
>>
>>> Hi,
>>>
>>> This patch series contains fixes and cleanups around the issue that
>>> the console UART should not idled on suspend while using 
>>> "no_console_suspend"
>>> in bootargs.
>>>
>> The direction of the series is right, thanks for the updated approach.
>> I had a comple minor comments on specific patches, but the ordering of
>> the series needs a little tweaking.  It should be
>>
>> - core/driver changes [current 1-3/6 are ok]
>> - remove usage from mach-omap2/serial.c (currently part of 4/6)
>> - remove am33x DT usage (current 5/6 is ok)
>> - remove entirely from omap_device (omap_device part of 4/6 and 6/6 
>> should be combined)
>>
> Thanks a lot for your review and comments.
> I have replied to your comments on the respective patches.
> Will take care of the "ordering" which you mentioned above
> in the next version.
>
> Thanks
> Sourav
Hi Sourav

I'd like to clarify some points regarding this series:

1) I'm not sure that removing OMAP_DEVICE_NO_IDLE_ON_SUSPEND is fine.
FYI - 
review.omapzoom.org/#/q/status:open,n,zI9e21bf4432a537a4ccb217cf9c425b0e4e499ce8
"ASoC: omap-abe: Allow no idle on suspend"
The above "voice call" use case allows Audio playback while system is in 
"suspend" state.
In addition HSI and SmartReflex may need to be active after 
suspend_noirq stage.
By removing this flag such functionality will be broken (in case of 
porting it
on newest Kernel or MainLine).
So, How it can be handled without OMAP_DEVICE_NO_IDLE_ON_SUSPEND?

2) Runtime PM vs System suspend
- The commit 88d26136a "PM: Prevent runtime suspend during system resume"
   block pm_runtime_put_xx() while suspending/resuming, so if your UART 
will became active
   (at least one call to pm_runtime_get_xx()) while system is suspending 
it will stay active
   until suspend_noirq stage.
- At suspend_noirq stage OMAP device framework will finally (brutally) 
disable it to reach
   system suspend state (in _od_suspend_noirq).
- At resume_noirq stage OMAP device framework will re-enable device if 
it was disabled in
   _od_suspend_noirq() to keep device state and Runtime PM in sync.
- In addition, the commit 9f6d8f6 "PM: Move disabling/enabling runtime 
PM to late suspend/early resume"
   will disable Runtime PM at suspend_late and enable it resume_early 
stages.

As, result:
- serial_omap_prepare()/serial_omap_complete() not needed - usually 
console is active.
   Or you may call pm_runtime_get_xxx() in serial_omap_prepare() to be 
sure (that's all)
- removing OMAP_DEVICE_NO_IDLE_ON_SUSPEND will not help, because to 
handle your use case
   omap_device_idle() need to be removed from od_suspend_noirq().
   !! Which, in turn, will kill OMAP suspend !! (see Kevin's comments)

Unfortunately, I see no way to avoid usage 
OMAP_DEVICE_NO_IDLE_ON_SUSPEND with the current
OMAP device framework.

Regards,
-grygorii
>> Kevin
>>
>>> The approach thought of is to modify the serial core/serial driver 
>>> to bypass
>>> runtime PM if the UART in contention is a console and we are using 
>>> "no_console_suspend"
>>> in our bootargs.
>>>
>>> While fixing the above issue, there are other cleanups also done as 
>>> part of
>>> this series which are no longer required. This cleanups mainly 
>>> include getting
>>> rid of using "omap_device_disable_idle_on_suspend" api for both dt 
>>> and non dt case
>>> as the serial driver will be self sufficient to handle the 
>>> "no_idle_on_suspend" issue.
>>> Serial was the only one making use of 
>>> "omap_device_disable_idle_on_suspend"
>>>
>>> Test info (except drivers: serial: mpc52xx_uart: Remove 
>>> "uart_console" defintion):
>>> Omap4430sdp:
>>> - Tested wakeup from UART after suspend for dt and non dt case.
>>> Omap5430evm:
>>> - Tested wakeup from UART after suspend for dt case.
>>>
>>>
>>> There were discussions about how to handle "no_idle_on_suspend" 
>>> issue and all the
>>> discussions are as follows:
>>> [v3]: https://lkml.org/lkml/2013/4/5/239
>>> [v2]: https://lkml.org/lkml/2013/4/2/350
>>> [v1]: https://lkml.org/lkml/2013/3/18/199
>>>        https://lkml.org/lkml/2013/3/18/295
>>> Due to the amount of change in approach and other cleanups coming 
>>> around it, I am posting
>>> this as a new series.
>>>
>>> This patches are based on 3.9-rc3 custom tree which has
>>> Santosh Shilimkar serial patch[1]
>>> [1]: http://permalink.gmane.org/gmane.linux.ports.arm.omap/95828
>>>
>>> Cc: Santosh Shilimkar<santosh.shilimkar@ti.com>
>>> Cc: Felipe Balbi<balbi@ti.com>
>>> Cc: Rajendra nayak<rnayak@ti.com>
>>>
>>> Sourav Poddar (6):
>>>    drivers: tty: serial: Move "uart_console" def to core header file.
>>>    drivers: serial: mpc52xx_uart: Remove "uart_console" defintion
>>>    driver: serial: omap: add prepare/complete callback for
>>>      "no_console_suspend" case
>>>    arm: mach-omap2: remove "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check
>>>    arm: dts: am33xx: Remove "ti,no_idle_on_suspend" property.
>>>    arm: mach-omap2: Remove "no_console_suspend"
>>>
>>>   arch/arm/boot/dts/am33xx.dtsi     |    1 -
>>>   arch/arm/mach-omap2/omap_device.c |   10 +---------
>>>   arch/arm/mach-omap2/serial.c      |    7 -------
>>>   drivers/tty/serial/mpc52xx_uart.c |   10 ----------
>>>   drivers/tty/serial/omap-serial.c  |   20 ++++++++++++++++++++
>>>   drivers/tty/serial/serial_core.c  |    6 ------
>>>   include/linux/serial_core.h       |    6 ++++++
>>>   7 files changed, 27 insertions(+), 33 deletions(-)
>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] arm: mach-omap2: remove "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check

[Sourav Poddar]

Hi Kevin,
On Friday 19 April 2013 03:33 AM, Kevin Hilman wrote:
> Sourav Poddar<sourav.poddar@ti.com>  writes:
>
>> On Thursday 18 April 2013 11:35 PM, Kevin Hilman wrote:
>>> Sourav Poddar<sourav.poddar@ti.com>   writes:
>>>
>>>> Remove the "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check, since UART was the only one making
>>>> use of it. Now serial core/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.
>>>>
>>>> Signed-off-by: Sourav Poddar<sourav.poddar@ti.com>
>>> NAK.  This patch will break many things...
>>>
>>>> ---
>>>>    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);
>>> Why did you remove the omap_device_idle() here?
>> This patch is used along with patch3 to get rid of the issue. I posted
>> them as a
>> seperate patch beacuse of the subject line as one goes under drivers/*
>> and the other
>> arm/mach-omap2/*..
>>
>> This check was only valid for UART, and if od->flags is set to the
>> "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" flag, then UART will not be idled.
> correct, but *every other device* would be idled (if not already idle.)
>
>> But now, we no longer depend on od->flag value to prevent idling of
>> our console UART as the prepare/complete apis will take care of them.
> Right, so removing the check on od->flags is fine, but what I asked
> about is why you removed the omap_device_idle() call.
>
> Remember that this code is called for *every* omap_device during
> suspend, not just UART.
>
> What you did stops *every* device from being idled during suspend.
>
> You didn't read my whole message.  Specifically this part:
>
Yes, got your point. omap_device_idle should not be called only
for console uart.

Just did a quick testing by including the following hunk on top of my 
patch series..

diff --git a/arch/arm/mach-omap2/omap_device.c 
b/arch/arm/mach-omap2/omap_device.c
index c226946..7480e87 100644
--- a/arch/arm/mach-omap2/omap_device.c
+++ b/arch/arm/mach-omap2/omap_device.c
@@ -617,8 +617,10 @@ 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 (pm_generic_runtime_suspend(dev) == 0) {
+                       omap_device_idle(pdev);
                         od->flags |= OMAP_DEVICE_SUSPENDED;
+               }
         }

         return ret;
@@ -631,6 +633,7 @@ static int _od_resume_noirq(struct device *dev)

         if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
             !pm_runtime_status_suspended(dev)) {
+               omap_device_enable(pdev);
                 od->flags &= ~OMAP_DEVICE_SUSPENDED;
                 pm_generic_runtime_resume(dev);
         }
And found the wakeup from UART is no more functional.
So, the omap_device_idle gets called for console UART also, thereby
preventing the "no_idle_on_suspend" theory.

Hence, merely putting prepare/complete callback the way I did is not 
helping.
We need to delete omap_device_idle also, which I agree is not correct.
So, we need a way to bypass this "omap_device_idle"
call for console UART. ?
What my understanding was that after modifying serial driver,
bypass the entire hunk[1] for console UART?


>>> Note that the check is for when the flag is *not* set, so this patch
>>> changes behavior for all the drivers that do not use
>>> _NO_IDLE_ON_SUSPEND.  I think that's the opposite of what you intended.
> Kevin

Re: [PATCH 0/6] Serial Omap fixes and cleanups

[Sourav Poddar]

Hi Grygorii,
On Friday 19 April 2013 05:32 PM, Grygorii Strashko wrote:
> On 04/18/2013 10:17 PM, Sourav Poddar wrote:
>> Hi Kevin,
>> On Thursday 18 April 2013 11:53 PM, Kevin Hilman wrote:
>>> Hi Sourav,
>>>
>>> Sourav Poddar<sourav.poddar@ti.com>  writes:
>>>
>>>> Hi,
>>>>
>>>> This patch series contains fixes and cleanups around the issue that
>>>> the console UART should not idled on suspend while using 
>>>> "no_console_suspend"
>>>> in bootargs.
>>>>
>>> The direction of the series is right, thanks for the updated approach.
>>> I had a comple minor comments on specific patches, but the ordering of
>>> the series needs a little tweaking.  It should be
>>>
>>> - core/driver changes [current 1-3/6 are ok]
>>> - remove usage from mach-omap2/serial.c (currently part of 4/6)
>>> - remove am33x DT usage (current 5/6 is ok)
>>> - remove entirely from omap_device (omap_device part of 4/6 and 6/6 
>>> should be combined)
>>>
>> Thanks a lot for your review and comments.
>> I have replied to your comments on the respective patches.
>> Will take care of the "ordering" which you mentioned above
>> in the next version.
>>
>> Thanks
>> Sourav
> Hi Sourav
>
> I'd like to clarify some points regarding this series:
>
> 1) I'm not sure that removing OMAP_DEVICE_NO_IDLE_ON_SUSPEND is fine.
> FYI - 
> review.omapzoom.org/#/q/status:open,n,zI9e21bf4432a537a4ccb217cf9c425b0e4e499ce8
> "ASoC: omap-abe: Allow no idle on suspend"
> The above "voice call" use case allows Audio playback while system is 
> in "suspend" state.
> In addition HSI and SmartReflex may need to be active after 
> suspend_noirq stage.
> By removing this flag such functionality will be broken (in case of 
> porting it
> on newest Kernel or MainLine).
> So, How it can be handled without OMAP_DEVICE_NO_IDLE_ON_SUSPEND?
>
Yes, if we have other use cases, we need to see if there is any way of 
handling it through the
respective drivers.
> 2) Runtime PM vs System suspend
> - The commit 88d26136a "PM: Prevent runtime suspend during system resume"
>   block pm_runtime_put_xx() while suspending/resuming, so if your UART 
> will became active
>   (at least one call to pm_runtime_get_xx()) while system is 
> suspending it will stay active
>   until suspend_noirq stage.
> - At suspend_noirq stage OMAP device framework will finally (brutally) 
> disable it to reach
>   system suspend state (in _od_suspend_noirq).
> - At resume_noirq stage OMAP device framework will re-enable device if 
> it was disabled in
>   _od_suspend_noirq() to keep device state and Runtime PM in sync.
> - In addition, the commit 9f6d8f6 "PM: Move disabling/enabling runtime 
> PM to late suspend/early resume"
>   will disable Runtime PM at suspend_late and enable it resume_early 
> stages.
>
> As, result:
> - serial_omap_prepare()/serial_omap_complete() not needed - usually 
> console is active.
>   Or you may call pm_runtime_get_xxx() in serial_omap_prepare() to be 
> sure (that's all)
> - removing OMAP_DEVICE_NO_IDLE_ON_SUSPEND will not help, because to 
> handle your use case
>   omap_device_idle() need to be removed from od_suspend_noirq().
>   !! Which, in turn, will kill OMAP suspend !! (see Kevin's comments)
>
Yes, I have seen kevin's comments and indeed we need to remove 
"omap_device_idle"
along with prep/complete to get my issue fixed(which indeed is not correct).

> Unfortunately, I see no way to avoid usage 
> OMAP_DEVICE_NO_IDLE_ON_SUSPEND with the current
> OMAP device framework.
>
> Regards,
> -grygorii
>>> Kevin
>>>
>>>> The approach thought of is to modify the serial core/serial driver 
>>>> to bypass
>>>> runtime PM if the UART in contention is a console and we are using 
>>>> "no_console_suspend"
>>>> in our bootargs.
>>>>
>>>> While fixing the above issue, there are other cleanups also done as 
>>>> part of
>>>> this series which are no longer required. This cleanups mainly 
>>>> include getting
>>>> rid of using "omap_device_disable_idle_on_suspend" api for both dt 
>>>> and non dt case
>>>> as the serial driver will be self sufficient to handle the 
>>>> "no_idle_on_suspend" issue.
>>>> Serial was the only one making use of 
>>>> "omap_device_disable_idle_on_suspend"
>>>>
>>>> Test info (except drivers: serial: mpc52xx_uart: Remove 
>>>> "uart_console" defintion):
>>>> Omap4430sdp:
>>>> - Tested wakeup from UART after suspend for dt and non dt case.
>>>> Omap5430evm:
>>>> - Tested wakeup from UART after suspend for dt case.
>>>>
>>>>
>>>> There were discussions about how to handle "no_idle_on_suspend" 
>>>> issue and all the
>>>> discussions are as follows:
>>>> [v3]: https://lkml.org/lkml/2013/4/5/239
>>>> [v2]: https://lkml.org/lkml/2013/4/2/350
>>>> [v1]: https://lkml.org/lkml/2013/3/18/199
>>>>        https://lkml.org/lkml/2013/3/18/295
>>>> Due to the amount of change in approach and other cleanups coming 
>>>> around it, I am posting
>>>> this as a new series.
>>>>
>>>> This patches are based on 3.9-rc3 custom tree which has
>>>> Santosh Shilimkar serial patch[1]
>>>> [1]: http://permalink.gmane.org/gmane.linux.ports.arm.omap/95828
>>>>
>>>> Cc: Santosh Shilimkar<santosh.shilimkar@ti.com>
>>>> Cc: Felipe Balbi<balbi@ti.com>
>>>> Cc: Rajendra nayak<rnayak@ti.com>
>>>>
>>>> Sourav Poddar (6):
>>>>    drivers: tty: serial: Move "uart_console" def to core header file.
>>>>    drivers: serial: mpc52xx_uart: Remove "uart_console" defintion
>>>>    driver: serial: omap: add prepare/complete callback for
>>>>      "no_console_suspend" case
>>>>    arm: mach-omap2: remove "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check
>>>>    arm: dts: am33xx: Remove "ti,no_idle_on_suspend" property.
>>>>    arm: mach-omap2: Remove "no_console_suspend"
>>>>
>>>>   arch/arm/boot/dts/am33xx.dtsi     |    1 -
>>>>   arch/arm/mach-omap2/omap_device.c |   10 +---------
>>>>   arch/arm/mach-omap2/serial.c      |    7 -------
>>>>   drivers/tty/serial/mpc52xx_uart.c |   10 ----------
>>>>   drivers/tty/serial/omap-serial.c  |   20 ++++++++++++++++++++
>>>>   drivers/tty/serial/serial_core.c  |    6 ------
>>>>   include/linux/serial_core.h       |    6 ++++++
>>>>   7 files changed, 27 insertions(+), 33 deletions(-)
>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 4/6] arm: mach-omap2: remove "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check

[Kevin Hilman]

Sourav Poddar <sourav.poddar@ti.com> writes:

[...]

> Yes, got your point. omap_device_idle should not be called only
> for console uart.
>
> Just did a quick testing by including the following hunk on top of my
> patch series..
>
> diff --git a/arch/arm/mach-omap2/omap_device.c
> b/arch/arm/mach-omap2/omap_device.c
> index c226946..7480e87 100644
> --- a/arch/arm/mach-omap2/omap_device.c
> +++ b/arch/arm/mach-omap2/omap_device.c
> @@ -617,8 +617,10 @@ 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 (pm_generic_runtime_suspend(dev) == 0) {
> +                       omap_device_idle(pdev);
>                         od->flags |= OMAP_DEVICE_SUSPENDED;
> +               }
>         }
>
>         return ret;
> @@ -631,6 +633,7 @@ static int _od_resume_noirq(struct device *dev)
>
>         if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
>             !pm_runtime_status_suspended(dev)) {
> +               omap_device_enable(pdev);
>                 od->flags &= ~OMAP_DEVICE_SUSPENDED;
>                 pm_generic_runtime_resume(dev);
>         }
> And found the wakeup from UART is no more functional.
> So, the omap_device_idle gets called for console UART also, thereby
> preventing the "no_idle_on_suspend" theory.
>
> Hence, merely putting prepare/complete callback the way I did is not
> helping.
> We need to delete omap_device_idle also, which I agree is not correct.
> So, we need a way to bypass this "omap_device_idle"
> call for console UART. ?

OK, I see what's happening now.

How about this: rather than using prepare/complete callbacks, can you
use the runtime_suspend callback to return an error code during suspend?
(only for the console, and only when no_console_suspend is enabled, and
only during suspend)

Since _od_suspend_noirq checks to be sure the drivers ->runtime_suspend
callback succeeds before it calls omap_device_idle(), if you report a
failure, omap_device_idle will not be called.

Kevin

Re: [PATCH 4/6] arm: mach-omap2: remove "OMAP_DEVICE_NO_IDLE_ON_SUSPEND" check

[Sourav Poddar]

Hi Kevin,
On Friday 19 April 2013 08:22 PM, Kevin Hilman wrote:
> Sourav Poddar<sourav.poddar@ti.com>  writes:
>
> [...]
>
>> Yes, got your point. omap_device_idle should not be called only
>> for console uart.
>>
>> Just did a quick testing by including the following hunk on top of my
>> patch series..
>>
>> diff --git a/arch/arm/mach-omap2/omap_device.c
>> b/arch/arm/mach-omap2/omap_device.c
>> index c226946..7480e87 100644
>> --- a/arch/arm/mach-omap2/omap_device.c
>> +++ b/arch/arm/mach-omap2/omap_device.c
>> @@ -617,8 +617,10 @@ 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 (pm_generic_runtime_suspend(dev) == 0) {
>> +                       omap_device_idle(pdev);
>>                          od->flags |= OMAP_DEVICE_SUSPENDED;
>> +               }
>>          }
>>
>>          return ret;
>> @@ -631,6 +633,7 @@ static int _od_resume_noirq(struct device *dev)
>>
>>          if ((od->flags&  OMAP_DEVICE_SUSPENDED)&&
>>              !pm_runtime_status_suspended(dev)) {
>> +               omap_device_enable(pdev);
>>                  od->flags&= ~OMAP_DEVICE_SUSPENDED;
>>                  pm_generic_runtime_resume(dev);
>>          }
>> And found the wakeup from UART is no more functional.
>> So, the omap_device_idle gets called for console UART also, thereby
>> preventing the "no_idle_on_suspend" theory.
>>
>> Hence, merely putting prepare/complete callback the way I did is not
>> helping.
>> We need to delete omap_device_idle also, which I agree is not correct.
>> So, we need a way to bypass this "omap_device_idle"
>> call for console UART. ?
> OK, I see what's happening now.
>
> How about this: rather than using prepare/complete callbacks, can you
> use the runtime_suspend callback to return an error code during suspend?
Yes, that can be done.
> (only for the console, and only when no_console_suspend is enabled, and
> only during suspend)
>
> Since _od_suspend_noirq checks to be sure the drivers ->runtime_suspend
> callback succeeds before it calls omap_device_idle(), if you report a
> failure, omap_device_idle will not be called.
>
True.

~Sourav
> Kevin
>
>