| Message ID | 1404491651-1388-3-git-send-email-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 4 Jul 2014 18:34:10 +0200 Sebastian Andrzej Siewior <bigeasy@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
* One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> [140707 06:22]: > On Fri, 4 Jul 2014 18:34:10 +0200 > Sebastian Andrzej Siewior <bigeasy@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
On 07/09/2014 01:17 PM, Tony Lindgren wrote: > * One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> [140707 06:22]: >> On Fri, 4 Jul 2014 18:34:10 +0200 >> Sebastian Andrzej Siewior <bigeasy@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
* Sebastian Andrzej Siewior <bigeasy@linutronix.de> [140709 04:38]: > On 07/09/2014 01:17 PM, Tony Lindgren wrote: > > * One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> [140707 06:22]: > >> On Fri, 4 Jul 2014 18:34:10 +0200 > >> Sebastian Andrzej Siewior <bigeasy@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
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
* Sebastian Andrzej Siewior <bigeasy@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
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
* Sebastian Andrzej Siewior <bigeasy@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
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; + pm_runtime_get_sync(port->dev); + spin_lock_irqsave(&port->lock, flags); lsr = serial_port_in(port, UART_LSR); up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS; spin_unlock_irqrestore(&port->lock, flags); + pm_runtime_mark_last_busy(port->dev); + pm_runtime_put_autosuspend(port->dev); + return (lsr & BOTH_EMPTY) == BOTH_EMPTY ? TIOCSER_TEMT : 0; } @@ -1805,7 +1845,10 @@ static unsigned int serial8250_get_mctrl(struct uart_port *port) unsigned int status; unsigned int ret; + pm_runtime_get_sync(port->dev); status = serial8250_modem_status(up); + pm_runtime_mark_last_busy(port->dev); + pm_runtime_put_autosuspend(port->dev); ret = 0; if (status & UART_MSR_DCD) @@ -1838,7 +1881,10 @@ static void serial8250_set_mctrl(struct uart_port *port, unsigned int mctrl) mcr = (mcr & up->mcr_mask) | up->mcr_force | up->mcr; + pm_runtime_get_sync(port->dev); serial_port_out(port, UART_MCR, mcr); + pm_runtime_mark_last_busy(port->dev); + pm_runtime_put_autosuspend(port->dev); } static void serial8250_break_ctl(struct uart_port *port, int break_state) @@ -1847,6 +1893,7 @@ static void serial8250_break_ctl(struct uart_port *port, int break_state) container_of(port, struct uart_8250_port, port); unsigned long flags; + pm_runtime_get_sync(port->dev); spin_lock_irqsave(&port->lock, flags); if (break_state == -1) up->lcr |= UART_LCR_SBC; @@ -1854,6 +1901,8 @@ static void serial8250_break_ctl(struct uart_port *port, int break_state) up->lcr &= ~UART_LCR_SBC; serial_port_out(port, UART_LCR, up->lcr); spin_unlock_irqrestore(&port->lock, flags); + pm_runtime_mark_last_busy(port->dev); + pm_runtime_put_autosuspend(port->dev); } /* @@ -1898,12 +1947,23 @@ static void wait_for_xmitr(struct uart_8250_port *up, int bits) static int serial8250_get_poll_char(struct uart_port *port) { - unsigned char lsr = serial_port_in(port, UART_LSR); + unsigned char lsr; + int status; + + pm_runtime_get_sync(port->dev); - if (!(lsr & UART_LSR_DR)) - return NO_POLL_CHAR; + lsr = serial_port_in(port, UART_LSR); - return serial_port_in(port, UART_RX); + if (!(lsr & UART_LSR_DR)) { + status = NO_POLL_CHAR; + goto out; + } + + status = serial_port_in(port, UART_RX); +out: + pm_runtime_mark_last_busy(up->dev); + pm_runtime_put_autosuspend(up->dev); + return status; } @@ -1914,6 +1974,7 @@ static void serial8250_put_poll_char(struct uart_port *port, struct uart_8250_port *up = container_of(port, struct uart_8250_port, port); + pm_runtime_get_sync(up->dev); /* * First save the IER then disable the interrupts */ @@ -1935,6 +1996,9 @@ static void serial8250_put_poll_char(struct uart_port *port, */ wait_for_xmitr(up, BOTH_EMPTY); serial_port_out(port, UART_IER, ier); + pm_runtime_mark_last_busy(up->dev); + pm_runtime_put_autosuspend(up->dev); + } #endif /* CONFIG_CONSOLE_POLL */ @@ -1961,6 +2025,7 @@ int serial8250_do_startup(struct uart_port *port) if (port->iotype != up->cur_iotype) set_io_from_upio(port); + pm_runtime_get_sync(port->dev); if (port->type == PORT_16C950) { /* Wake up and initialize UART */ up->acr = 0; @@ -1981,7 +2046,6 @@ int serial8250_do_startup(struct uart_port *port) */ enable_rsa(up); #endif - /* * Clear the FIFO buffers and disable them. * (they will be reenabled in set_termios()) @@ -2005,7 +2069,8 @@ int serial8250_do_startup(struct uart_port *port) (serial_port_in(port, UART_LSR) == 0xff)) { printk_ratelimited(KERN_INFO "ttyS%d: LSR safety check engaged!\n", serial_index(port)); - return -ENODEV; + retval = -ENODEV; + goto out; } /* @@ -2090,7 +2155,7 @@ int serial8250_do_startup(struct uart_port *port) } else { retval = serial_link_irq_chain(up); if (retval) - return retval; + goto out; } /* @@ -2188,8 +2253,11 @@ int serial8250_do_startup(struct uart_port *port) outb_p(0x80, icp); inb_p(icp); } - - return 0; + retval = 0; +out: + pm_runtime_mark_last_busy(port->dev); + pm_runtime_put_autosuspend(port->dev); + return retval; } EXPORT_SYMBOL_GPL(serial8250_do_startup); @@ -2207,6 +2275,7 @@ static void serial8250_shutdown(struct uart_port *port) container_of(port, struct uart_8250_port, port); unsigned long flags; + pm_runtime_get_sync(port->dev); /* * Disable interrupts from this port */ @@ -2246,6 +2315,8 @@ static void serial8250_shutdown(struct uart_port *port) * the IRQ chain. */ serial_port_in(port, UART_RX); + pm_runtime_mark_last_busy(port->dev); + pm_runtime_put_autosuspend(port->dev); del_timer_sync(&up->timer); up->timer.function = serial8250_timeout; @@ -2356,6 +2427,7 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios, * Ok, we're now changing the port state. Do it with * interrupts disabled. */ + pm_runtime_get_sync(port->dev); spin_lock_irqsave(&port->lock, flags); /* @@ -2477,6 +2549,9 @@ serial8250_do_set_termios(struct uart_port *port, struct ktermios *termios, } serial8250_set_mctrl(port, port->mctrl); spin_unlock_irqrestore(&port->lock, flags); + pm_runtime_mark_last_busy(port->dev); + pm_runtime_put_autosuspend(port->dev); + /* Don't rewrite B0 */ if (tty_termios_baud_rate(termios)) tty_termios_encode_baud_rate(termios, baud, baud);
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 <bigeasy@linutronix.de> --- drivers/tty/serial/8250/8250_core.c | 101 +++++++++++++++++++++++++++++++----- 1 file changed, 88 insertions(+), 13 deletions(-)