[PATCH] tty: omap-serial: Keep the wakeup mechanism enabled by default

[PATCH] tty: omap-serial: Keep the wakeup mechanism enabled by default

[Govindraj.R]

From: "Govindraj.R" <govindraj.raja@ti.com>

The wakeups can be left enabled by default and should be disabled
only when disabled from sysfs and while entering suspend.

Thanks to Kevin Hilman <khilman@ti.com> for suggesting this.
Discussion References:
http://www.spinics.net/lists/linux-omap/msg67764.html
http://www.spinics.net/lists/linux-omap/msg67838.html

Cc: Paul Walmsley <paul@pwsan.com>
Cc: Kevin Hilman <khilman@ti.com>
Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
---
 drivers/tty/serial/omap-serial.c |   30 +++++++++++-------------------
 1 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index d00b38e..4a92447 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -930,13 +930,6 @@ serial_omap_pm(struct uart_port *port, unsigned int state,
 	serial_out(up, UART_EFR, efr);
 	serial_out(up, UART_LCR, 0);
 
-	if (!device_may_wakeup(&up->pdev->dev)) {
-		if (!state)
-			pm_runtime_forbid(&up->pdev->dev);
-		else
-			pm_runtime_allow(&up->pdev->dev);
-	}
-
 	pm_runtime_put(&up->pdev->dev);
 }
 
@@ -1184,10 +1177,16 @@ static struct uart_driver serial_omap_reg = {
 static int serial_omap_suspend(struct device *dev)
 {
 	struct uart_omap_port *up = dev_get_drvdata(dev);
+	struct omap_uart_port_info *pdata = dev->platform_data;
 
 	if (up) {
 		uart_suspend_port(&serial_omap_reg, &up->port);
 		flush_work_sync(&up->qos_work);
+
+		if (!device_may_wakeup(dev)) {
+			pdata->enable_wakeup(up->pdev, false);
+			up->wakeups_enabled = false;
+		}
 	}
 
 	return 0;
@@ -1582,18 +1581,6 @@ static int serial_omap_runtime_suspend(struct device *dev)
 	if (pdata->get_context_loss_count)
 		up->context_loss_cnt = pdata->get_context_loss_count(dev);
 
-	if (device_may_wakeup(dev)) {
-		if (!up->wakeups_enabled) {
-			pdata->enable_wakeup(up->pdev, true);
-			up->wakeups_enabled = true;
-		}
-	} else {
-		if (up->wakeups_enabled) {
-			pdata->enable_wakeup(up->pdev, false);
-			up->wakeups_enabled = false;
-		}
-	}
-
 	/* Errata i291 */
 	if (up->use_dma && pdata->set_forceidle &&
 			(up->errata & UART_ERRATA_i291_DMA_FORCEIDLE))
@@ -1618,6 +1605,11 @@ static int serial_omap_runtime_resume(struct device *dev)
 				serial_omap_restore_context(up);
 		}
 
+		if (!up->wakeups_enabled) {
+			pdata->enable_wakeup(up->pdev, true);
+			up->wakeups_enabled = true;
+		}
+
 		/* Errata i291 */
 		if (up->use_dma && pdata->set_noidle &&
 				(up->errata & UART_ERRATA_i291_DMA_FORCEIDLE))
-- 
1.7.9

[PATCH] tty: omap-serial: Keep the wakeup mechanism enabled by default

[Kevin Hilman]

"Govindraj.R" <govindraj.raja@ti.com> writes:

> From: "Govindraj.R" <govindraj.raja@ti.com>
>
> The wakeups can be left enabled by default and should be disabled
> only when disabled from sysfs and while entering suspend.

Left enabled?  That assumes something else has initizlied them, but we
can't make that assumption.

First, wakeups should be disabled when ->probe has finished.  Then,
they should be enabled whenever driver is in use, and disabled when
the driver is not in use.

I'm not familiar enough with uart_ops, but it looks like they should
probably be enabled in uart_ops->startup and disabled in
uart_ops->shutdown.  

I've hacked up a test patch[1] which was required for module-wakeups to
work for me.  I tested module-level wakeups by disabling all C-states
except C1 using the new sysfs control for disabling C-states[2].

More comments below...

> Thanks to Kevin Hilman <khilman@ti.com> for suggesting this.
> Discussion References:
> http://www.spinics.net/lists/linux-omap/msg67764.html
> http://www.spinics.net/lists/linux-omap/msg67838.html
>
> Cc: Paul Walmsley <paul@pwsan.com>
> Cc: Kevin Hilman <khilman@ti.com>
> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
> ---
>  drivers/tty/serial/omap-serial.c |   30 +++++++++++-------------------
>  1 files changed, 11 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index d00b38e..4a92447 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -930,13 +930,6 @@ serial_omap_pm(struct uart_port *port, unsigned int state,
>  	serial_out(up, UART_EFR, efr);
>  	serial_out(up, UART_LCR, 0);
>  
> -	if (!device_may_wakeup(&up->pdev->dev)) {
> -		if (!state)
> -			pm_runtime_forbid(&up->pdev->dev);
> -		else
> -			pm_runtime_allow(&up->pdev->dev);
> -	}
> -
>  	pm_runtime_put(&up->pdev->dev);
>  }
>  
> @@ -1184,10 +1177,16 @@ static struct uart_driver serial_omap_reg = {
>  static int serial_omap_suspend(struct device *dev)
>  {
>  	struct uart_omap_port *up = dev_get_drvdata(dev);
> +	struct omap_uart_port_info *pdata = dev->platform_data;
>  
>  	if (up) {
>  		uart_suspend_port(&serial_omap_reg, &up->port);
>  		flush_work_sync(&up->qos_work);
> +
> +		if (!device_may_wakeup(dev)) {
> +			pdata->enable_wakeup(up->pdev, false);

Should check for the presence of pdata->enable_wakeup() before calling.  
Same thing below.

> +			up->wakeups_enabled = false;
> +		}
>  	}
>  
>  	return 0;
> @@ -1582,18 +1581,6 @@ static int serial_omap_runtime_suspend(struct device *dev)
>  	if (pdata->get_context_loss_count)
>  		up->context_loss_cnt = pdata->get_context_loss_count(dev);
>  
> -	if (device_may_wakeup(dev)) {
> -		if (!up->wakeups_enabled) {
> -			pdata->enable_wakeup(up->pdev, true);
> -			up->wakeups_enabled = true;
> -		}
> -	} else {
> -		if (up->wakeups_enabled) {
> -			pdata->enable_wakeup(up->pdev, false);
> -			up->wakeups_enabled = false;
> -		}
> -	}
> -
>  	/* Errata i291 */
>  	if (up->use_dma && pdata->set_forceidle &&
>  			(up->errata & UART_ERRATA_i291_DMA_FORCEIDLE))
> @@ -1618,6 +1605,11 @@ static int serial_omap_runtime_resume(struct device *dev)
>  				serial_omap_restore_context(up);
>  		}
>  
> +		if (!up->wakeups_enabled) {
> +			pdata->enable_wakeup(up->pdev, true);
> +			up->wakeups_enabled = true;
> +		}

You put the disable in ->suspend, but the enable in ->runtime_resume.

Doesn't this belong in ->resume ?

Kevin

>  		/* Errata i291 */
>  		if (up->use_dma && pdata->set_noidle &&
>  				(up->errata & UART_ERRATA_i291_DMA_FORCEIDLE))



[1] Feel free to fold this into your original patch.

>From 8a4a24998aaf35240f0010b172537da64351a7d6 Mon Sep 17 00:00:00 2001
From: Kevin Hilman <khilman@ti.com>
Date: Tue, 17 Apr 2012 16:24:05 -0700
Subject: [PATCH] omap-serial: fix default wakeup settings

---
 drivers/tty/serial/omap-serial.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 4a92447..6458ec9 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -511,6 +511,7 @@ static void serial_omap_break_ctl(struct uart_port *port, int break_state)
 static int serial_omap_startup(struct uart_port *port)
 {
 	struct uart_omap_port *up = (struct uart_omap_port *)port;
+	struct omap_uart_port_info *pdata = up->pdev->dev.platform_data;
 	unsigned long flags = 0;
 	int retval;
 
@@ -525,6 +526,9 @@ static int serial_omap_startup(struct uart_port *port)
 	dev_dbg(up->port.dev, "serial_omap_startup+%d\n", up->port.line);
 
 	pm_runtime_get_sync(&up->pdev->dev);
+	if (pdata->enable_wakeup)
+		pdata->enable_wakeup(up->pdev, true);
+
 	/*
 	 * Clear the FIFO buffers and disable them.
 	 * (they will be reenabled in set_termios())
@@ -589,6 +593,7 @@ static int serial_omap_startup(struct uart_port *port)
 static void serial_omap_shutdown(struct uart_port *port)
 {
 	struct uart_omap_port *up = (struct uart_omap_port *)port;
+	struct omap_uart_port_info *pdata = up->pdev->dev.platform_data;
 	unsigned long flags = 0;
 
 	dev_dbg(up->port.dev, "serial_omap_shutdown+%d\n", up->port.line);
@@ -628,6 +633,8 @@ static void serial_omap_shutdown(struct uart_port *port)
 		up->uart_dma.rx_buf = NULL;
 	}
 
+	if (pdata->enable_wakeup)
+		pdata->enable_wakeup(up->pdev, false);
 	pm_runtime_put(&up->pdev->dev);
 	free_irq(up->port.irq, up);
 }
@@ -1475,6 +1482,9 @@ static int serial_omap_probe(struct platform_device *pdev)
 	if (ret != 0)
 		goto err_add_port;
 
+	if (omap_up_info->enable_wakeup)
+		omap_up_info->enable_wakeup(pdev, false);
+
 	pm_runtime_put(&pdev->dev);
 	platform_set_drvdata(pdev, up);
 	return 0;
-- 
1.7.9.2



[1] shell snippit for disabling C-states

# CPUidle: disable everything but C1
cd /sys/devices/system/cpu/cpu0/cpuidle
for state in state[1-6]*; do
  if [ -e ${state} ]; then 
    echo 1 > cat ${state}/disable
  fi
done

[PATCH] tty: omap-serial: Keep the wakeup mechanism enabled by default

[Raja, Govindraj]

On Wed, Apr 18, 2012 at 4:55 AM, Kevin Hilman <khilman@ti.com> wrote:
> "Govindraj.R" <govindraj.raja@ti.com> writes:
>
>> From: "Govindraj.R" <govindraj.raja@ti.com>
>>
>> The wakeups can be left enabled by default and should be disabled
>> only when disabled from sysfs and while entering suspend.
>
> Left enabled? ?That assumes something else has initizlied them, but we
> can't make that assumption.
>
> First, wakeups should be disabled when ->probe has finished. ?Then,
> they should be enabled whenever driver is in use, and disabled when
> the driver is not in use.
>
> I'm not familiar enough with uart_ops, but it looks like they should
> probably be enabled in uart_ops->startup and disabled in
> uart_ops->shutdown.

uart_ops->shutdown gets called in suspend path also
serial_omap_suspend => uart_suspend_port = > ops->shutdown(uport);

This will leave uart wakeup disabled in suspend path.

--
Thanks,
Govindraj.R

[PATCH] tty: omap-serial: Keep the wakeup mechanism enabled by default

[Kevin Hilman]

"Raja, Govindraj" <govindraj.raja@ti.com> writes:

> On Wed, Apr 18, 2012 at 4:55 AM, Kevin Hilman <khilman@ti.com> wrote:
>> "Govindraj.R" <govindraj.raja@ti.com> writes:
>>
>>> From: "Govindraj.R" <govindraj.raja@ti.com>
>>>
>>> The wakeups can be left enabled by default and should be disabled
>>> only when disabled from sysfs and while entering suspend.
>>
>> Left enabled? ?That assumes something else has initizlied them, but we
>> can't make that assumption.
>>
>> First, wakeups should be disabled when ->probe has finished. ?Then,
>> they should be enabled whenever driver is in use, and disabled when
>> the driver is not in use.
>>
>> I'm not familiar enough with uart_ops, but it looks like they should
>> probably be enabled in uart_ops->startup and disabled in
>> uart_ops->shutdown.
>
> uart_ops->shutdown gets called in suspend path also
> serial_omap_suspend => uart_suspend_port = > ops->shutdown(uport);
>
> This will leave uart wakeup disabled in suspend path.

As I said, I'm not familiar enough with uart_ops to know which are the
right ones.

Maybe ->request_port and ->release_port are the right ones?

The point is that wakeups should be enabled whenever driver is in use,
and disabled when the driver is not in use.

Kevin

[PATCH] tty: omap-serial: Keep the wakeup mechanism enabled by default

[Russell King - ARM Linux]

On Wed, Apr 18, 2012 at 07:21:33AM -0700, Kevin Hilman wrote:
> "Raja, Govindraj" <govindraj.raja@ti.com> writes:
> 
> > On Wed, Apr 18, 2012 at 4:55 AM, Kevin Hilman <khilman@ti.com> wrote:
> >> "Govindraj.R" <govindraj.raja@ti.com> writes:
> >>
> >>> From: "Govindraj.R" <govindraj.raja@ti.com>
> >>>
> >>> The wakeups can be left enabled by default and should be disabled
> >>> only when disabled from sysfs and while entering suspend.
> >>
> >> Left enabled? ?That assumes something else has initizlied them, but we
> >> can't make that assumption.
> >>
> >> First, wakeups should be disabled when ->probe has finished. ?Then,
> >> they should be enabled whenever driver is in use, and disabled when
> >> the driver is not in use.
> >>
> >> I'm not familiar enough with uart_ops, but it looks like they should
> >> probably be enabled in uart_ops->startup and disabled in
> >> uart_ops->shutdown.
> >
> > uart_ops->shutdown gets called in suspend path also
> > serial_omap_suspend => uart_suspend_port = > ops->shutdown(uport);
> >
> > This will leave uart wakeup disabled in suspend path.
> 
> As I said, I'm not familiar enough with uart_ops to know which are the
> right ones.
> 
> Maybe ->request_port and ->release_port are the right ones?

No, the clue's there in the name - these are supposed to be for resource
management.  They only get called when the port is first acquired by the
driver, and released when the port is no longer required (iow, they
cover the span in time where the driver is capable of using the device.)
They don't tell you about the lifetime that the user has the port open.

> The point is that wakeups should be enabled whenever driver is in use,
> and disabled when the driver is not in use.

Well, we don't actually tell low level drivers that kind of detail (we
assume they're rather dumb - which maybe we shouldn't.)

It looks to me like there is the possibility of checking the TTY port
flags to see whether ASYNC_INITIALIZED is set - this will be clear in
->shutdown() method if the user is closing the port, otherwise it will
be set.  Same goes for the ->startup() method.

I'm not sure whether Alan would allow that kind of knowledge in low
level serial drivers though...

[PATCH] tty: omap-serial: Keep the wakeup mechanism enabled by default

[Alan Cox]

> The point is that wakeups should be enabled whenever driver is in use,
> and disabled when the driver is not in use.


Which is the tty_port methods for initialisation and shutdown, which are
mutex protected against each other and hang up.

Not sure how the uart layer glue exposes that.

Alan

[PATCH] tty: omap-serial: Keep the wakeup mechanism enabled by default

[Raja, Govindraj]

On Thu, Apr 19, 2012 at 12:38 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> The point is that wakeups should be enabled whenever driver is in use,
>> and disabled when the driver is not in use.
>
>
> Which is the tty_port methods for initialisation and shutdown, which are
> mutex protected against each other and hang up.
>
> Not sure how the uart layer glue exposes that.

Is it okay to read the port flags to identify the port state in
driver whether the serial port shutdown ops is called from
port_suspend or due to port closure.

Since port_shutdown gets called even from uart_suspend_port

Is things like

test_bit(ASYNCB_SUSPENDED, &port->flags);
or
test_bit(ASYNCB_CLOSING, &port->flags);

allowed in low level driver ?

--
Thanks,
Govindraj.R

[PATCH] tty: omap-serial: Keep the wakeup mechanism enabled by default

[Alan Cox]

On Thu, 19 Apr 2012 20:00:12 +0530
"Raja, Govindraj" <govindraj.raja@ti.com> wrote:

> On Thu, Apr 19, 2012 at 12:38 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> >> The point is that wakeups should be enabled whenever driver is in use,
> >> and disabled when the driver is not in use.
> >
> >
> > Which is the tty_port methods for initialisation and shutdown, which are
> > mutex protected against each other and hang up.
> >
> > Not sure how the uart layer glue exposes that.
> 
> Is it okay to read the port flags to identify the port state in
> driver whether the serial port shutdown ops is called from
> port_suspend or due to port closure.
> 
> Since port_shutdown gets called even from uart_suspend_port

That sounds like someone needs to fix the uart code to avoid muddling up
suspend/resume/open/close then, or at least pass the needed information
down.

Don't rely on port->flags ideally, they may well go away in part as they
cease to be needed by the lock changes.

Alan