Re: [RFC PATCH 3/4] tty: serial: 8250 core: add runtime pm
* Sebastian Andrzej Siewior bige...@linutronix.de [140709 09:35]: On 07/09/2014 05:12 PM, Tony Lindgren wrote: And also please note that for runtime PM the wake-up events need to be always enabled, so the device_may_wakeup() checks should be only implemented for suspend and resume. I think I got that corrected for most part in omap-serial.c recently, but knowing that might reduce the confusion a bit :) Ehm. I also added it to omap_8250_pm() as it is done in omap-serial (in serial_omap_pm()). Should I get rid of it in the latter? Yes, I commented on that patch also. 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
Re: [RFC PATCH 3/4] tty: serial: 8250 core: add runtime pm
* One Thousand Gnomes gno...@lxorguk.ukuu.org.uk [140707 06:22]: On Fri, 4 Jul 2014 18:34:10 +0200 Sebastian Andrzej Siewior bige...@linutronix.de wrote: While comparing the OMAP-serial and the 8250 part of this I noticed that the the latter does not use runtime-pm. Yes it does, but 8250 parts (generally - omap presumably is special here ?) need to be powered on to transmit/receive not just for register access. The core uart layer implements a pm operation for this. At least for omaps the UARTs need to be idled for the SoC to hit off-idle where the SoC is powered off and rebooted during idle. So we can certainly enable this in a generic way, however, this can only be done under the following conditions: 1. We can save and restore the serial register context and detect when context was lost in the serial driver. The context loss can be detected by looking at some registers that are only initialized during init. 2. There's a way for the serial RX pin to wake the SoC. On some omaps there's a separate pin specific wake-up interrupt that can be used for that. 3. The serial PM features must be only enabled if requested by the user via sysfs. Typically extra latency and loss of the first RX character occur which is not acceptable to most applications. 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
Re: [RFC PATCH 3/4] tty: serial: 8250 core: add runtime pm
On 07/09/2014 01:17 PM, Tony Lindgren wrote: * One Thousand Gnomes gno...@lxorguk.ukuu.org.uk [140707 06:22]: On Fri, 4 Jul 2014 18:34:10 +0200 Sebastian Andrzej Siewior bige...@linutronix.de wrote: While comparing the OMAP-serial and the 8250 part of this I noticed that the the latter does not use runtime-pm. Yes it does, but 8250 parts (generally - omap presumably is special here ?) need to be powered on to transmit/receive not just for register access. The core uart layer implements a pm operation for this. At least for omaps the UARTs need to be idled for the SoC to hit off-idle where the SoC is powered off and rebooted during idle. So we can certainly enable this in a generic way, however, this can only be done under the following conditions: 1. We can save and restore the serial register context and detect when context was lost in the serial driver. The context loss can be detected by looking at some registers that are only initialized during init. A register check on restore context? DLL+DLH might be a good hint here since its value should be 0 in the running case. 2. There's a way for the serial RX pin to wake the SoC. On some omaps there's a separate pin specific wake-up interrupt that can be used for that. That one would be handled by omap separately. 3. The serial PM features must be only enabled if requested by the user via sysfs. Typically extra latency and loss of the first RX character occur which is not acceptable to most applications. Unless I'm mistaken the omap driver now initializes the timeout to -1. So nothing happens unless this is enabled separately. 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: [RFC PATCH 3/4] tty: serial: 8250 core: add runtime pm
* Sebastian Andrzej Siewior bige...@linutronix.de [140709 04:38]: On 07/09/2014 01:17 PM, Tony Lindgren wrote: * One Thousand Gnomes gno...@lxorguk.ukuu.org.uk [140707 06:22]: On Fri, 4 Jul 2014 18:34:10 +0200 Sebastian Andrzej Siewior bige...@linutronix.de wrote: While comparing the OMAP-serial and the 8250 part of this I noticed that the the latter does not use runtime-pm. Yes it does, but 8250 parts (generally - omap presumably is special here ?) need to be powered on to transmit/receive not just for register access. The core uart layer implements a pm operation for this. At least for omaps the UARTs need to be idled for the SoC to hit off-idle where the SoC is powered off and rebooted during idle. So we can certainly enable this in a generic way, however, this can only be done under the following conditions: Sorry forgot to mention why I think this can now be done in a generic way, that's because we have now runtime PM in Linux. 1. We can save and restore the serial register context and detect when context was lost in the serial driver. The context loss can be detected by looking at some registers that are only initialized during init. A register check on restore context? DLL+DLH might be a good hint here since its value should be 0 in the running case. OK yeah that would be a generic way to do it potentially ;) Note that all the context_loss_cnt stuff in omap-serial.c can then be ignored, that's still only needed for the omap3 legacy mode booting case and won't be needed much longer. 2. There's a way for the serial RX pin to wake the SoC. On some omaps there's a separate pin specific wake-up interrupt that can be used for that. That one would be handled by omap separately. OK great. 3. The serial PM features must be only enabled if requested by the user via sysfs. Typically extra latency and loss of the first RX character occur which is not acceptable to most applications. Unless I'm mistaken the omap driver now initializes the timeout to -1. So nothing happens unless this is enabled separately. Yes that's the only safe approach so the serial port behaves in the standard Linux way unless specifically requested by the user. 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
Re: [RFC PATCH 3/4] tty: serial: 8250 core: add runtime pm
On 07/09/2014 01:48 PM, Tony Lindgren wrote: So we can certainly enable this in a generic way, however, this can only be done under the following conditions: Sorry forgot to mention why I think this can now be done in a generic way, that's because we have now runtime PM in Linux. I have a problem to follow here. I need set SET_RUNTIME_PM_OPS in .pm member of the driver. This is around for a while now maybe it wasn't around by the omap-serial was written but this doesn't matter now. As for the dw driver (as one of the gnomes points out) it shouldn't matter because it is enabled / disabled based on 8250's pm callback. So while the device is active and the 8250 core grabs puts the reference, nothing should change. Still a double check from Mika wouldn't hurt. If I understand the omap case correct (and please correct me if I don't) the 8250-omap driver assumes that the device can go to sleep after the probe function completes. This does not happen because pm_runtime_set_autosuspend_delay() sets the delay to -1. If the user changes this via sysfs, then the device might go to sleep and the IP block switched of. Therefore we need the runtime pm callbacks in 8250-core before the register access because we can't access the it otherwise. The core should wake us up in case there is incoming RX action to be handled so we can restore register's content. And for TX we have the proper pm hooks. The dw driver also only disables / enables the clock. So they don't lose the register's content like omap does. Thus they don't need the one struct I exported. I don't think that it makes sense to make this restore/save part generic if this what it is about. OMAP at least has a few registers more to take care of and set-termios is already different. 1. We can save and restore the serial register context and detect when context was lost in the serial driver. The context loss can be detected by looking at some registers that are only initialized during init. A register check on restore context? DLL+DLH might be a good hint here since its value should be 0 in the running case. OK yeah that would be a generic way to do it potentially ;) Note that all the context_loss_cnt stuff in omap-serial.c can then be ignored, that's still only needed for the omap3 legacy mode booting case and won't be needed much longer. Ah. The way the code reads, it is just an optimization in case the core didn't go off after all so we don't have to restore the registers. 2. There's a way for the serial RX pin to wake the SoC. On some omaps there's a separate pin specific wake-up interrupt that can be used for that. That one would be handled by omap separately. OK great. 3. The serial PM features must be only enabled if requested by the user via sysfs. Typically extra latency and loss of the first RX character occur which is not acceptable to most applications. Unless I'm mistaken the omap driver now initializes the timeout to -1. So nothing happens unless this is enabled separately. Yes that's the only safe approach so the serial port behaves in the standard Linux way unless specifically requested by the user. 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: [RFC PATCH 3/4] tty: serial: 8250 core: add runtime pm
* Sebastian Andrzej Siewior bige...@linutronix.de [140709 05:45]: On 07/09/2014 01:48 PM, Tony Lindgren wrote: So we can certainly enable this in a generic way, however, this can only be done under the following conditions: Sorry forgot to mention why I think this can now be done in a generic way, that's because we have now runtime PM in Linux. I have a problem to follow here. I need set SET_RUNTIME_PM_OPS in .pm member of the driver. This is around for a while now maybe it wasn't around by the omap-serial was written but this doesn't matter now. Right, the lack of runtime PM was one of the reasons for doing omap-serial earlier. As for the dw driver (as one of the gnomes points out) it shouldn't matter because it is enabled / disabled based on 8250's pm callback. So while the device is active and the 8250 core grabs puts the reference, nothing should change. Still a double check from Mika wouldn't hurt. OK If I understand the omap case correct (and please correct me if I don't) the 8250-omap driver assumes that the device can go to sleep after the probe function completes. This does not happen because pm_runtime_set_autosuspend_delay() sets the delay to -1. Correct. And once the timeout is enabled, the serial driver autoidles itself using runtime PM based on the timeout value. And also please note that for runtime PM the wake-up events need to be always enabled, so the device_may_wakeup() checks should be only implemented for suspend and resume. I think I got that corrected for most part in omap-serial.c recently, but knowing that might reduce the confusion a bit :) If the user changes this via sysfs, then the device might go to sleep and the IP block switched of. Therefore we need the runtime pm callbacks in 8250-core before the register access because we can't access the it otherwise. Correct. And it's not just the 8250 idle state, it's the whole SoC getting powered down during idle and rebooted when waking to a timer interrupt or device interrupt. The core should wake us up in case there is incoming RX action to be handled so we can restore register's content. And for TX we have the proper pm hooks. The piece of code that runs in that case is the interrupt handler for the serial port. That handler could be separate from the regular serial port handler if necessary though and do a callback to the serial core code. But I don't know if that's needed. The dw driver also only disables / enables the clock. So they don't lose the register's content like omap does. Thus they don't need the one struct I exported. Right, enabling and disabling the clock is done too for omaps when the SoC hits retention idle. I don't think that it makes sense to make this restore/save part generic if this what it is about. OMAP at least has a few registers more to take care of and set-termios is already different. Yeah, and that can be done later on if others need it. 1. We can save and restore the serial register context and detect when context was lost in the serial driver. The context loss can be detected by looking at some registers that are only initialized during init. A register check on restore context? DLL+DLH might be a good hint here since its value should be 0 in the running case. OK yeah that would be a generic way to do it potentially ;) Note that all the context_loss_cnt stuff in omap-serial.c can then be ignored, that's still only needed for the omap3 legacy mode booting case and won't be needed much longer. Ah. The way the code reads, it is just an optimization in case the core didn't go off after all so we don't have to restore the registers. Well the interconnect code knows when the context was lost on omaps, but there's currently no Linux generic API for that. And since we can do it by checking some registers, it's best to implement it that way. 2. There's a way for the serial RX pin to wake the SoC. On some omaps there's a separate pin specific wake-up interrupt that can be used for that. That one would be handled by omap separately. OK great. 3. The serial PM features must be only enabled if requested by the user via sysfs. Typically extra latency and loss of the first RX character occur which is not acceptable to most applications. Unless I'm mistaken the omap driver now initializes the timeout to -1. So nothing happens unless this is enabled separately. Yes that's the only safe approach so the serial port behaves in the standard Linux way unless specifically requested by the user. 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: [RFC PATCH 3/4] tty: serial: 8250 core: add runtime pm
On 07/09/2014 05:12 PM, Tony Lindgren wrote: And also please note that for runtime PM the wake-up events need to be always enabled, so the device_may_wakeup() checks should be only implemented for suspend and resume. I think I got that corrected for most part in omap-serial.c recently, but knowing that might reduce the confusion a bit :) Ehm. I also added it to omap_8250_pm() as it is done in omap-serial (in serial_omap_pm()). Should I get rid of it in the latter? 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: [RFC PATCH 3/4] tty: serial: 8250 core: add runtime pm
On Fri, 4 Jul 2014 18:34:10 +0200 Sebastian Andrzej Siewior bige...@linutronix.de wrote: While comparing the OMAP-serial and the 8250 part of this I noticed that the the latter does not use runtime-pm. Yes it does, but 8250 parts (generally - omap presumably is special here ?) need to be powered on to transmit/receive not just for register access. The core uart layer implements a pm operation for this. As 8250_dw uses runtime pm to implement the pm operation it's not as simple as assumign it won't get triggered. I *think* this is ok because the designware and other cases would take a reference on open and drop it on close, so avoiding any confusion, but for the register accesses on a closed port it would benefit from a further double check with Mika especially as the suspend/resume on the LPSS block on some Intel devices is a little bit too interesting for comfort. Otherwise however I think this is good. Alan -- 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
[RFC PATCH 3/4] tty: serial: 8250 core: add runtime pm
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.
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 f731fff..b0b9abb 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 flags;
unsigned int lsr;
