by Sekhar Nori

[permalink] [raw]

Subject: Re: [PATCH 6/6] tty: serial: Add 8250-core based omap driver

On Wednesday 16 July 2014 06:02 PM, Tony Lindgren wrote:
> * Sebastian Andrzej Siewior <[email protected]> [140716 05:14]:
>> On 07/11/2014 08:41 AM, Tony Lindgren wrote:
>>>
>>> If the RX pin does not wake it up, you need to configure the
>>> pinctrl-single entry for it, and configure that pin as a wake-up
>>> interrupt. See the interrupts-extended entry in omap3-beagle-xm.dts.
>>
>> This does not help. Checking the manual, there is not something like
>> PIN_OFF_WAKEUPENABLE for am33xx. There is just INPUT/OUTPUT, pull
>> up/down + enabled/disabled and the mux_mode. For the interrupt, the HW
>> referenced as omap3_pmx_core touches some bits in the pinmux register
>> which do not exists on am33xx.
>
> Right, on am33xx there's no IO chain wake-up path. AFAIK the only way
> to provide wake events on am33xx would be to mux the RX pin temporarily
> to a GPIO input. And sounds like that may work only if the GPIO is
> in the first GPIO bank that's always on. I don't know if the other
> GPIO banks on am33xx can provide wake up events.

No, they cannot.

Thanks,
Sekhar

2014-07-21 13:35:22

by Mika Westerberg

[permalink] [raw]

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

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: [email protected]

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 <[email protected]>
> ---
> 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() variant
might sleep here. At least if the RTPM of the device is backed by ACPI
methods.

> +
> + 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 @@ void serial8250_do_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 @@ void serial8250_do_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;
> @@ -2365,6 +2436,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);
>
> /*
> @@ -2486,6 +2558,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);
> --
> 2.0.1