Re: [PATCH 3/6] tty: serial: 8250 core: add runtime pm

2014-07-21 Thread Mika Westerberg 
On Wed, Jul 09, 2014 at 07:49:34PM +0200, Sebastian Andrzej Siewior wrote:
 While comparing the OMAP-serial and the 8250 part of this I noticed that
 the the latter does not use runtime-pm. Here are the pieces. It is
 basically a get before first register access and a last_busy + put after
 last access.
 If I understand this correct, it should do nothing as long as
 pm_runtime_use_autosuspend() + pm_runtime_enable() isn't invoked on the
 device.
 
 Cc: mika.westerb...@linux.intel.com

Sorry for the delay, just came back from vacation.

Adding Heikki, who knows the 8250_dw driver much better than me.
Unfortunately he is still on vacation for next two weeks.

 Signed-off-by: Sebastian Andrzej Siewior bige...@linutronix.de
 ---
  drivers/tty/serial/8250/8250_core.c | 101 
 +++-
  1 file changed, 88 insertions(+), 13 deletions(-)
 
 diff --git a/drivers/tty/serial/8250/8250_core.c 
 b/drivers/tty/serial/8250/8250_core.c
 index d37eb08..1a91a89 100644
 --- a/drivers/tty/serial/8250/8250_core.c
 +++ b/drivers/tty/serial/8250/8250_core.c
 @@ -38,6 +38,7 @@
  #include linux/nmi.h
  #include linux/mutex.h
  #include linux/slab.h
 +#include linux/pm_runtime.h
  #ifdef CONFIG_SPARC
  #include linux/sunserialcore.h
  #endif
 @@ -553,10 +554,11 @@ static void serial8250_set_sleep(struct uart_8250_port 
 *p, int sleep)
* offset but the UART channel may only write to the corresponding
* bit.
*/
 + pm_runtime_get_sync(p-port.dev);
   if ((p-port.type == PORT_XR17V35X) ||
  (p-port.type == PORT_XR17D15X)) {
   serial_out(p, UART_EXAR_SLEEP, sleep ? 0xff : 0);
 - return;
 + goto out;
   }
  
   if (p-capabilities  UART_CAP_SLEEP) {
 @@ -571,7 +573,17 @@ static void serial8250_set_sleep(struct uart_8250_port 
 *p, int sleep)
   serial_out(p, UART_EFR, 0);
   serial_out(p, UART_LCR, 0);
   }
 +
 + if (!device_may_wakeup(p-port.dev)) {
 + if (sleep)
 + pm_runtime_forbid(p-port.dev);
 + else
 + pm_runtime_allow(p-port.dev);
 + }
   }
 +out:
 + pm_runtime_mark_last_busy(p-port.dev);
 + pm_runtime_put_autosuspend(p-port.dev);
  }
  
  #ifdef CONFIG_SERIAL_8250_RSA
 @@ -1280,6 +1292,7 @@ static void serial8250_stop_tx(struct uart_port *port)
   struct uart_8250_port *up =
   container_of(port, struct uart_8250_port, port);
  
 + pm_runtime_get_sync(port-dev);
   __stop_tx(up);
  
   /*
 @@ -1289,6 +1302,8 @@ static void serial8250_stop_tx(struct uart_port *port)
   up-acr |= UART_ACR_TXDIS;
   serial_icr_write(up, UART_ACR, up-acr);
   }
 + pm_runtime_mark_last_busy(port-dev);
 + pm_runtime_put_autosuspend(port-dev);
  }
  
  static void serial8250_start_tx(struct uart_port *port)
 @@ -1296,8 +1311,9 @@ static void serial8250_start_tx(struct uart_port *port)
   struct uart_8250_port *up =
   container_of(port, struct uart_8250_port, port);
  
 + pm_runtime_get_sync(port-dev);
   if (up-dma  !serial8250_tx_dma(up)) {
 - return;
 + goto out;
   } else if (!(up-ier  UART_IER_THRI)) {
   up-ier |= UART_IER_THRI;
   serial_port_out(port, UART_IER, up-ier);
 @@ -1318,6 +1334,9 @@ static void serial8250_start_tx(struct uart_port *port)
   up-acr = ~UART_ACR_TXDIS;
   serial_icr_write(up, UART_ACR, up-acr);
   }
 +out:
 + pm_runtime_mark_last_busy(port-dev);
 + pm_runtime_put_autosuspend(port-dev);
  }
  
  static void serial8250_stop_rx(struct uart_port *port)
 @@ -1325,9 +1344,14 @@ static void serial8250_stop_rx(struct uart_port *port)
   struct uart_8250_port *up =
   container_of(port, struct uart_8250_port, port);
  
 + pm_runtime_get_sync(port-dev);
 +
   up-ier = ~UART_IER_RLSI;
   up-port.read_status_mask = ~UART_LSR_DR;
   serial_port_out(port, UART_IER, up-ier);
 +
 + pm_runtime_mark_last_busy(port-dev);
 + pm_runtime_put_autosuspend(port-dev);
  }
  
  static void serial8250_enable_ms(struct uart_port *port)
 @@ -1340,7 +1364,10 @@ static void serial8250_enable_ms(struct uart_port 
 *port)
   return;
  
   up-ier |= UART_IER_MSI;
 + pm_runtime_get_sync(port-dev);
   serial_port_out(port, UART_IER, up-ier);
 + pm_runtime_mark_last_busy(port-dev);
 + pm_runtime_put_autosuspend(port-dev);
  }
  
  /*
 @@ -1530,9 +1557,17 @@ EXPORT_SYMBOL_GPL(serial8250_handle_irq);
  
  static int serial8250_default_handle_irq(struct uart_port *port)
  {
 - unsigned int iir = serial_port_in(port, UART_IIR);
 + unsigned int iir;
 + int ret;
  
 - return serial8250_handle_irq(port, iir);
 + pm_runtime_get_sync(port-dev);

Is this function executed in interrupt context? Calling _sync()

Re: [PATCH 3/6] tty: serial: 8250 core: add runtime pm

2014-07-16 Thread Sebastian Andrzej Siewior 
On 07/10/2014 08:28 AM, Tony Lindgren wrote:
 --- a/drivers/tty/serial/8250/8250_core.c
 +++ b/drivers/tty/serial/8250/8250_core.c
 @@ -571,7 +573,17 @@ static void serial8250_set_sleep(struct uart_8250_port 
 *p, int sleep)
  serial_out(p, UART_EFR, 0);
  serial_out(p, UART_LCR, 0);
  }
 +
 +if (!device_may_wakeup(p-port.dev)) {
 +if (sleep)
 +pm_runtime_forbid(p-port.dev);
 +else
 +pm_runtime_allow(p-port.dev);
 +}
  }
 +out:
 +pm_runtime_mark_last_busy(p-port.dev);
 +pm_runtime_put_autosuspend(p-port.dev);
  }
 
 The device_may_wakeup logic here is wrong as I described in the
 earlier thread. For runtime PM, the wake-up events should be
 always enabled. So the device_may_wakeup checks should be only
 done for suspend and resume.

Okay. I dropped it from here.

 
 Regards,
 
 Tony
 

Sebastian
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] tty: serial: 8250 core: add runtime pm

2014-07-10 Thread Tony Lindgren 
* Sebastian Andrzej Siewior bige...@linutronix.de [140709 10:52]:
 While comparing the OMAP-serial and the 8250 part of this I noticed that
 the the latter does not use runtime-pm. Here are the pieces. It is
 basically a get before first register access and a last_busy + put after
 last access.
 If I understand this correct, it should do nothing as long as
 pm_runtime_use_autosuspend() + pm_runtime_enable() isn't invoked on the
 device.
...

 --- a/drivers/tty/serial/8250/8250_core.c
 +++ b/drivers/tty/serial/8250/8250_core.c
 @@ -571,7 +573,17 @@ static void serial8250_set_sleep(struct uart_8250_port 
 *p, int sleep)
   serial_out(p, UART_EFR, 0);
   serial_out(p, UART_LCR, 0);
   }
 +
 + if (!device_may_wakeup(p-port.dev)) {
 + if (sleep)
 + pm_runtime_forbid(p-port.dev);
 + else
 + pm_runtime_allow(p-port.dev);
 + }
   }
 +out:
 + pm_runtime_mark_last_busy(p-port.dev);
 + pm_runtime_put_autosuspend(p-port.dev);
  }

The device_may_wakeup logic here is wrong as I described in the
earlier thread. For runtime PM, the wake-up events should be
always enabled. So the device_may_wakeup checks should be only
done for suspend and resume.

Regards,

Tony
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/6] tty: serial: 8250 core: add runtime pm

2014-07-09 Thread Sebastian Andrzej Siewior 
While comparing the OMAP-serial and the 8250 part of this I noticed that
the the latter does not use runtime-pm. Here are the pieces. It is
basically a get before first register access and a last_busy + put after
last access.
If I understand this correct, it should do nothing as long as
pm_runtime_use_autosuspend() + pm_runtime_enable() isn't invoked on the
device.

Cc: mika.westerb...@linux.intel.com
Signed-off-by: Sebastian Andrzej Siewior bige...@linutronix.de
---
 drivers/tty/serial/8250/8250_core.c | 101 +++-
 1 file changed, 88 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c 
b/drivers/tty/serial/8250/8250_core.c
index d37eb08..1a91a89 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -38,6 +38,7 @@
 #include linux/nmi.h
 #include linux/mutex.h
 #include linux/slab.h
+#include linux/pm_runtime.h
 #ifdef CONFIG_SPARC
 #include linux/sunserialcore.h
 #endif
@@ -553,10 +554,11 @@ static void serial8250_set_sleep(struct uart_8250_port 
*p, int sleep)
 * offset but the UART channel may only write to the corresponding
 * bit.
 */
+   pm_runtime_get_sync(p-port.dev);
if ((p-port.type == PORT_XR17V35X) ||
   (p-port.type == PORT_XR17D15X)) {
serial_out(p, UART_EXAR_SLEEP, sleep ? 0xff : 0);
-   return;
+   goto out;
}
 
if (p-capabilities  UART_CAP_SLEEP) {
@@ -571,7 +573,17 @@ static void serial8250_set_sleep(struct uart_8250_port *p, 
int sleep)
serial_out(p, UART_EFR, 0);
serial_out(p, UART_LCR, 0);
}
+
+   if (!device_may_wakeup(p-port.dev)) {
+   if (sleep)
+   pm_runtime_forbid(p-port.dev);
+   else
+   pm_runtime_allow(p-port.dev);
+   }
}
+out:
+   pm_runtime_mark_last_busy(p-port.dev);
+   pm_runtime_put_autosuspend(p-port.dev);
 }
 
 #ifdef CONFIG_SERIAL_8250_RSA
@@ -1280,6 +1292,7 @@ static void serial8250_stop_tx(struct uart_port *port)
struct uart_8250_port *up =
container_of(port, struct uart_8250_port, port);
 
+   pm_runtime_get_sync(port-dev);
__stop_tx(up);
 
/*
@@ -1289,6 +1302,8 @@ static void serial8250_stop_tx(struct uart_port *port)
up-acr |= UART_ACR_TXDIS;
serial_icr_write(up, UART_ACR, up-acr);
}
+   pm_runtime_mark_last_busy(port-dev);
+   pm_runtime_put_autosuspend(port-dev);
 }
 
 static void serial8250_start_tx(struct uart_port *port)
@@ -1296,8 +1311,9 @@ static void serial8250_start_tx(struct uart_port *port)
struct uart_8250_port *up =
container_of(port, struct uart_8250_port, port);
 
+   pm_runtime_get_sync(port-dev);
if (up-dma  !serial8250_tx_dma(up)) {
-   return;
+   goto out;
} else if (!(up-ier  UART_IER_THRI)) {
up-ier |= UART_IER_THRI;
serial_port_out(port, UART_IER, up-ier);
@@ -1318,6 +1334,9 @@ static void serial8250_start_tx(struct uart_port *port)
up-acr = ~UART_ACR_TXDIS;
serial_icr_write(up, UART_ACR, up-acr);
}
+out:
+   pm_runtime_mark_last_busy(port-dev);
+   pm_runtime_put_autosuspend(port-dev);
 }
 
 static void serial8250_stop_rx(struct uart_port *port)
@@ -1325,9 +1344,14 @@ static void serial8250_stop_rx(struct uart_port *port)
struct uart_8250_port *up =
container_of(port, struct uart_8250_port, port);
 
+   pm_runtime_get_sync(port-dev);
+
up-ier = ~UART_IER_RLSI;
up-port.read_status_mask = ~UART_LSR_DR;
serial_port_out(port, UART_IER, up-ier);
+
+   pm_runtime_mark_last_busy(port-dev);
+   pm_runtime_put_autosuspend(port-dev);
 }
 
 static void serial8250_enable_ms(struct uart_port *port)
@@ -1340,7 +1364,10 @@ static void serial8250_enable_ms(struct uart_port *port)
return;
 
up-ier |= UART_IER_MSI;
+   pm_runtime_get_sync(port-dev);
serial_port_out(port, UART_IER, up-ier);
+   pm_runtime_mark_last_busy(port-dev);
+   pm_runtime_put_autosuspend(port-dev);
 }
 
 /*
@@ -1530,9 +1557,17 @@ EXPORT_SYMBOL_GPL(serial8250_handle_irq);
 
 static int serial8250_default_handle_irq(struct uart_port *port)
 {
-   unsigned int iir = serial_port_in(port, UART_IIR);
+   unsigned int iir;
+   int ret;
 
-   return serial8250_handle_irq(port, iir);
+   pm_runtime_get_sync(port-dev);
+
+   iir = serial_port_in(port, UART_IIR);
+   ret = serial8250_handle_irq(port, iir);
+
+   pm_runtime_mark_last_busy(port-dev);
+   pm_runtime_put_autosuspend(port-dev);
+   return ret;
 }
 
 /*
@@ -1790,11 +1825,16 @@ static unsigned int serial8250_tx_empty(struct 
uart_port *port)
unsigned long