[PATCH v4] 8250-core based serial driver for OMAP

[PATCH v4] 8250-core based serial driver for OMAP

[Sebastian Andrzej Siewior]

I converted most of the omap-serial over to the 8250-core based code where it
once was forked from. I dropped the rs485 support for now.
The runtime-pm does not crash any machines because none of them shutdown the
IP core and/or enter deep idle where it would metter.

Sebastian

[PATCH 1/5] tty: serial: 8250 core: provide a function to export uart_8250_port

[Sebastian Andrzej Siewior]

There is no access to access a struct uart_8250_port for a specific
line. This is only required outside of the 8250/uart callbacks like for
devices' suspend & remove callbacks. For those the 8250-core provides
wrapper like serial8250_unregister_port() which passes the struct
to the proper function based on the line argument.

For runtime suspend I need access to this struct not only to make
serial_out() work but also to properly restore up->ier and up->mcr.

Acked-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/8250/8250.h      |  2 ++
 drivers/tty/serial/8250/8250_core.c | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 1ebf853..34c3cd1 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -110,6 +110,8 @@ static inline void serial_dl_write(struct uart_8250_port *up, int value)
 	up->dl_write(up, value);
 }
 
+struct uart_8250_port *serial8250_get_port(int line);
+
 #if defined(__alpha__) && !defined(CONFIG_PCI)
 /*
  * Digital did something really horribly wrong with the OUT1 and OUT2
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 7a91c6d..5a6af19 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -2755,6 +2755,24 @@ static struct uart_ops serial8250_pops = {
 
 static struct uart_8250_port serial8250_ports[UART_NR];
 
+/**
+ * serial8250_get_port - retrieve struct uart_8250_port
+ * @line: serial line number
+ *
+ * This function retrieves struct uart_8250_port for the specific line.
+ * This struct *must* *not* be used to perform a 8250 or serial core operation
+ * which is not accessible otherwise. Its only purpose is to make the struct
+ * accessible to the runtime-pm callbacks for context suspend/restore.
+ * The lock assumption made here is none because runtime-pm suspend/resume
+ * callbacks should not be invoked if there is any operation performed on the
+ * port.
+ */
+struct uart_8250_port *serial8250_get_port(int line)
+{
+	return &serial8250_ports[line];
+}
+EXPORT_SYMBOL_GPL(serial8250_get_port);
+
 static void (*serial8250_isa_config)(int port, struct uart_port *up,
 	unsigned short *capabilities);
 
-- 
2.0.1

[PATCH 2/5] tty: serial: 8250 core: allow to overwrite & export serial8250_startup()

[Sebastian Andrzej Siewior]

The OMAP version of the 8250 can actually use 1:1 serial8250_startup().
However it needs to be extended by a wakeup irq which should to be
requested & enabled at ->startup() time and disabled at ->shutdown() time.

v2…v3: properly copy callbacks
v1…v2: add shutdown callback

Acked-by: Alan Cox <alan@linux.intel.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/8250/8250_core.c | 26 ++++++++++++++++++++++++--
 include/linux/serial_8250.h         |  2 ++
 include/linux/serial_core.h         |  2 ++
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 5a6af19..bd0e1897 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1939,7 +1939,7 @@ static void serial8250_put_poll_char(struct uart_port *port,
 
 #endif /* CONFIG_CONSOLE_POLL */
 
-static int serial8250_startup(struct uart_port *port)
+int serial8250_do_startup(struct uart_port *port)
 {
 	struct uart_8250_port *up =
 		container_of(port, struct uart_8250_port, port);
@@ -2191,8 +2191,17 @@ static int serial8250_startup(struct uart_port *port)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(serial8250_do_startup);
 
-static void serial8250_shutdown(struct uart_port *port)
+static int serial8250_startup(struct uart_port *port)
+{
+	if (port->startup)
+		return port->startup(port);
+	else
+		return serial8250_do_startup(port);
+}
+
+void serial8250_do_shutdown(struct uart_port *port)
 {
 	struct uart_8250_port *up =
 		container_of(port, struct uart_8250_port, port);
@@ -2243,6 +2252,15 @@ static void serial8250_shutdown(struct uart_port *port)
 	if (port->irq)
 		serial_unlink_irq_chain(up);
 }
+EXPORT_SYMBOL_GPL(serial8250_do_shutdown);
+
+static void serial8250_shutdown(struct uart_port *port)
+{
+	if (port->shutdown)
+		port->shutdown(port);
+	else
+		serial8250_do_shutdown(port);
+}
 
 static unsigned int serial8250_get_divisor(struct uart_port *port, unsigned int baud)
 {
@@ -3304,6 +3322,10 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
 		/*  Possibly override set_termios call */
 		if (up->port.set_termios)
 			uart->port.set_termios = up->port.set_termios;
+		if (up->port.startup)
+			uart->port.startup = up->port.startup;
+		if (up->port.shutdown)
+			uart->port.shutdown = up->port.shutdown;
 		if (up->port.pm)
 			uart->port.pm = up->port.pm;
 		if (up->port.handle_break)
diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
index af47a8a..0ec21ec 100644
--- a/include/linux/serial_8250.h
+++ b/include/linux/serial_8250.h
@@ -114,6 +114,8 @@ extern void serial8250_early_out(struct uart_port *port, int offset, int value);
 extern int setup_early_serial8250_console(char *cmdline);
 extern void serial8250_do_set_termios(struct uart_port *port,
 		struct ktermios *termios, struct ktermios *old);
+extern int serial8250_do_startup(struct uart_port *port);
+extern void serial8250_do_shutdown(struct uart_port *port);
 extern void serial8250_do_pm(struct uart_port *port, unsigned int state,
 			     unsigned int oldstate);
 extern int fsl8250_handle_irq(struct uart_port *port);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 5bbb809..6f20ff0 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -122,6 +122,8 @@ struct uart_port {
 	void			(*set_termios)(struct uart_port *,
 				               struct ktermios *new,
 				               struct ktermios *old);
+	int			(*startup)(struct uart_port *port);
+	void			(*shutdown)(struct uart_port *port);
 	int			(*handle_irq)(struct uart_port *);
 	void			(*pm)(struct uart_port *, unsigned int state,
 				      unsigned int old);
-- 
2.0.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/5] tty: serial: 8250 core: allow to set ->throttle / ->unthrottle callbacks

[Sebastian Andrzej Siewior]

The omap uart provides support for HW assisted flow control. What is
missing is the support to throttle / unthrottle callbacks which are used
by the omap-serial driver at the moment.
This patch adds the callbacks. It should be safe to add them since they
are only invovked from the serial_core (uart_throttle()) if the feature
flags are set.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/8250/8250_core.c | 14 ++++++++++++++
 include/linux/serial_core.h         |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index bd0e1897..714f954 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1320,6 +1320,16 @@ static void serial8250_start_tx(struct uart_port *port)
 	}
 }
 
+static void serial8250_throttle(struct uart_port *port)
+{
+	port->throttle(port);
+}
+
+static void serial8250_unthrottle(struct uart_port *port)
+{
+	port->unthrottle(port);
+}
+
 static void serial8250_stop_rx(struct uart_port *port)
 {
 	struct uart_8250_port *up =
@@ -2752,6 +2762,8 @@ static struct uart_ops serial8250_pops = {
 	.get_mctrl	= serial8250_get_mctrl,
 	.stop_tx	= serial8250_stop_tx,
 	.start_tx	= serial8250_start_tx,
+	.throttle	= serial8250_throttle,
+	.unthrottle	= serial8250_unthrottle,
 	.stop_rx	= serial8250_stop_rx,
 	.enable_ms	= serial8250_enable_ms,
 	.break_ctl	= serial8250_break_ctl,
@@ -3300,6 +3312,8 @@ int serial8250_register_8250_port(struct uart_8250_port *up)
 		uart->port.fifosize	= up->port.fifosize;
 		uart->tx_loadsz		= up->tx_loadsz;
 		uart->capabilities	= up->capabilities;
+		uart->port.throttle	= up->port.throttle;
+		uart->port.unthrottle	= up->port.unthrottle;
 
 		/* Take tx_loadsz from fifosize if it wasn't set separately */
 		if (uart->port.fifosize && !uart->tx_loadsz)
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 6f20ff0..c380af2 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -124,6 +124,8 @@ struct uart_port {
 				               struct ktermios *old);
 	int			(*startup)(struct uart_port *port);
 	void			(*shutdown)(struct uart_port *port);
+	void			(*throttle)(struct uart_port *port);
+	void			(*unthrottle)(struct uart_port *port);
 	int			(*handle_irq)(struct uart_port *);
 	void			(*pm)(struct uart_port *, unsigned int state,
 				      unsigned int old);
-- 
2.0.1

[PATCH 4/5] tty: serial: 8250 core: add runtime pm

[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.

v3…v4:
	- added runtime to the console code
	- removed device_may_wakeup() from serial8250_set_sleep()

Cc: mika.westerberg@linux.intel.com
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/8250/8250_core.c | 98 ++++++++++++++++++++++++++++++++-----
 1 file changed, 85 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 714f954..aceaea1 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) {
@@ -572,6 +574,9 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
 			serial_out(p, UART_LCR, 0);
 		}
 	}
+out:
+	pm_runtime_mark_last_busy(p->port.dev);
+	pm_runtime_put_autosuspend(p->port.dev);
 }
 
 #ifdef CONFIG_SERIAL_8250_RSA
@@ -1280,6 +1285,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 +1295,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 +1304,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 +1327,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_throttle(struct uart_port *port)
@@ -1335,9 +1347,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)
@@ -1350,7 +1367,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);
 }
 
 /*
@@ -1540,9 +1560,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;
+
+	pm_runtime_get_sync(port->dev);
+
+	iir = serial_port_in(port, UART_IIR);
+	ret = serial8250_handle_irq(port, iir);
 
-	return serial8250_handle_irq(port, iir);
+	pm_runtime_mark_last_busy(port->dev);
+	pm_runtime_put_autosuspend(port->dev);
+	return ret;
 }
 
 /*
@@ -1800,11 +1828,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;
 }
 
@@ -1815,7 +1848,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)
@@ -1848,7 +1884,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)
@@ -1857,6 +1896,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;
@@ -1864,6 +1904,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);
 }
 
 /*
@@ -1908,12 +1950,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;
 
-	if (!(lsr & UART_LSR_DR))
-		return NO_POLL_CHAR;
+	pm_runtime_get_sync(port->dev);
 
-	return serial_port_in(port, UART_RX);
+	lsr = serial_port_in(port, UART_LSR);
+
+	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;
 }
 
 
@@ -1924,6 +1977,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
 	 */
@@ -1945,6 +1999,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 */
@@ -1971,6 +2028,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;
@@ -1991,7 +2049,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())
@@ -2015,7 +2072,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;
 	}
 
 	/*
@@ -2100,7 +2158,7 @@ int serial8250_do_startup(struct uart_port *port)
 	} else {
 		retval = serial_link_irq_chain(up);
 		if (retval)
-			return retval;
+			goto out;
 	}
 
 	/*
@@ -2198,8 +2256,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);
 
@@ -2217,6 +2278,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
 	 */
@@ -2256,6 +2318,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;
@@ -2375,6 +2439,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);
 
 	/*
@@ -2496,6 +2561,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);
@@ -2931,6 +2999,8 @@ serial8250_console_write(struct console *co, const char *s, unsigned int count)
 
 	touch_nmi_watchdog();
 
+	pm_runtime_get_sync(port->dev);
+
 	if (port->sysrq || oops_in_progress)
 		locked = spin_trylock_irqsave(&port->lock, flags);
 	else
@@ -2967,6 +3037,8 @@ serial8250_console_write(struct console *co, const char *s, unsigned int count)
 
 	if (locked)
 		spin_unlock_irqrestore(&port->lock, flags);
+	pm_runtime_mark_last_busy(port->dev);
+	pm_runtime_put_autosuspend(port->dev);
 }
 
 static int __init serial8250_console_setup(struct console *co, char *options)
-- 
2.0.1

Re: [PATCH 4/5] tty: serial: 8250 core: add runtime pm

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1641 bytes --]

Hi,

On Wed, Jul 16, 2014 at 04:45:02PM +0200, Sebastian Andrzej Siewior wrote:
> @@ -1280,6 +1285,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 +1295,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 +1304,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 +1327,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);

I wonder if you should get_sync() on start_tx() and only
put_autosuspend() at stop_tx(). I guess the outcome would be largely the
same, no ?

well, other than in probe and other functions which need to make sure
clocks are on, but it seems unnecessary to enable/disable in every
function.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/5] tty: serial: 8250 core: add runtime pm

[Sebastian Andrzej Siewior]

On 07/16/2014 05:16 PM, Felipe Balbi wrote:
> Hi,

Hi Felipe,

> On Wed, Jul 16, 2014 at 04:45:02PM +0200, Sebastian Andrzej Siewior
> wrote:
>> @@ -1280,6 +1285,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 +1295,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 +1304,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 +1327,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);
> 
> I wonder if you should get_sync() on start_tx() and only 
> put_autosuspend() at stop_tx(). I guess the outcome would be
> largely the same, no ?

I just opened minicom on ttyS0 and gave a try. start_tx() was invoked
each time I pressed a key (sent a character). I haven't seen stop_tx()
even after after I closed minicom. I guess stop_tx() is invoked if you
switch half-duplex communication.

> well, other than in probe and other functions which need to make
> sure clocks are on, but it seems unnecessary to enable/disable in
> every function.

What do you have in mind? Do you plan to let the uart on while the
minicom is attached but is doing nothing? In that case, ->startup() and
->shutdown() should be enough.
If you want to put the uart off while the port is open but idle then
you should cover the callbacks as they are independent of each other.
You could receive three ->set_termios() even without a single
->start_tx(). And as far as I understand the whole situation, you need
to make sure the UART is "up" while you touch a single register not
only sending characters, right?

Sebastian

Re: [PATCH 4/5] tty: serial: 8250 core: add runtime pm

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3074 bytes --]

Hi,

On Wed, Jul 16, 2014 at 05:54:56PM +0200, Sebastian Andrzej Siewior wrote:
> On 07/16/2014 05:16 PM, Felipe Balbi wrote:
> > Hi,
> 
> Hi Felipe,
> 
> > On Wed, Jul 16, 2014 at 04:45:02PM +0200, Sebastian Andrzej Siewior
> > wrote:
> >> @@ -1280,6 +1285,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 +1295,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 +1304,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 +1327,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);
> > 
> > I wonder if you should get_sync() on start_tx() and only 
> > put_autosuspend() at stop_tx(). I guess the outcome would be
> > largely the same, no ?
> 
> I just opened minicom on ttyS0 and gave a try. start_tx() was invoked
> each time I pressed a key (sent a character). I haven't seen stop_tx()
> even after after I closed minicom. I guess stop_tx() is invoked if you
> switch half-duplex communication.

that's bad, I expected stop to be called also after each character.

> > well, other than in probe and other functions which need to make
> > sure clocks are on, but it seems unnecessary to enable/disable in
> > every function.
> 
> What do you have in mind? Do you plan to let the uart on while the
> minicom is attached but is doing nothing? In that case, ->startup() and
> ->shutdown() should be enough.

no the idea was to keep it on for as long as it's transferring
characters and idle it otherwise, if that can't be done easily, then I
guess your way is the only way.

> If you want to put the uart off while the port is open but idle then
> you should cover the callbacks as they are independent of each other.
> You could receive three ->set_termios() even without a single
> ->start_tx(). And as far as I understand the whole situation, you need
> to make sure the UART is "up" while you touch a single register not
> only sending characters, right?

right, for those cases, you have to get/put around function entry/exit;
I was just assuming that we didn't have to do that for each and every
callback.

cheers

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/5] tty: serial: 8250 core: add runtime pm

[Sebastian Andrzej Siewior]

On 07/16/2014 06:06 PM, Felipe Balbi wrote:

>>> well, other than in probe and other functions which need to
>>> make sure clocks are on, but it seems unnecessary to
>>> enable/disable in every function.
>> 
>> What do you have in mind? Do you plan to let the uart on while
>> the minicom is attached but is doing nothing? In that case,
>> ->startup() and ->shutdown() should be enough.
> 
> no the idea was to keep it on for as long as it's transferring 
> characters and idle it otherwise, if that can't be done easily,
> then I guess your way is the only way.

But maybe we have to add some additional logic here to keep it up for
the transfer. I've been just (maybe over)thinking: If you send 300
bytes over DMA via 300 baud it should take 10 seconds. The PM-timeout
could hit before the transfer is complete.
Same thing with hw-flowcontrol where you could get stalled for a few
seconds.
However it doesn't seem to be a problem in current omap-serial driver.

> cheers

Sebastian

Re: [PATCH 4/5] tty: serial: 8250 core: add runtime pm

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1360 bytes --]

On Wed, Jul 16, 2014 at 06:40:01PM +0200, Sebastian Andrzej Siewior wrote:
> On 07/16/2014 06:06 PM, Felipe Balbi wrote:
> 
> >>> well, other than in probe and other functions which need to
> >>> make sure clocks are on, but it seems unnecessary to
> >>> enable/disable in every function.
> >> 
> >> What do you have in mind? Do you plan to let the uart on while
> >> the minicom is attached but is doing nothing? In that case,
> >> ->startup() and ->shutdown() should be enough.
> > 
> > no the idea was to keep it on for as long as it's transferring 
> > characters and idle it otherwise, if that can't be done easily,
> > then I guess your way is the only way.
> 
> But maybe we have to add some additional logic here to keep it up for
> the transfer. I've been just (maybe over)thinking: If you send 300
> bytes over DMA via 300 baud it should take 10 seconds. The PM-timeout
> could hit before the transfer is complete.

hmm, good point. So we _do_ need a way to get early and only put after
we know transfer has completed and I was assuming stop_tx() to be that
hint, but apparently it isn't.

> Same thing with hw-flowcontrol where you could get stalled for a few
> seconds.
> However it doesn't seem to be a problem in current omap-serial driver.

I don't think current omap-serial driver is a great example.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/5] tty: serial: 8250 core: add runtime pm

[Peter Hurley]

On 07/16/2014 12:06 PM, Felipe Balbi wrote:
> On Wed, Jul 16, 2014 at 05:54:56PM +0200, Sebastian Andrzej Siewior wrote:
>> On 07/16/2014 05:16 PM, Felipe Balbi wrote:

>>> I wonder if you should get_sync() on start_tx() and only
>>> put_autosuspend() at stop_tx(). I guess the outcome would be
>>> largely the same, no ?
>>
>> I just opened minicom on ttyS0 and gave a try. start_tx() was invoked
>> each time I pressed a key (sent a character). I haven't seen stop_tx()
>> even after after I closed minicom. I guess stop_tx() is invoked if you
>> switch half-duplex communication.
>
> that's bad, I expected stop to be called also after each character.

The 8250 core auto-stops tx when the tx ring buffer is empty (except
in the case of dma, where stopping tx isn't necessary).

Regards,
Peter Hurley

Re: [PATCH 4/5] tty: serial: 8250 core: add runtime pm

[Sebastian Andrzej Siewior]

* Peter Hurley | 2014-07-17 11:31:59 [-0400]:

>On 07/16/2014 12:06 PM, Felipe Balbi wrote:
>>On Wed, Jul 16, 2014 at 05:54:56PM +0200, Sebastian Andrzej Siewior wrote:
>>>On 07/16/2014 05:16 PM, Felipe Balbi wrote:
>
>>>>I wonder if you should get_sync() on start_tx() and only
>>>>put_autosuspend() at stop_tx(). I guess the outcome would be
>>>>largely the same, no ?
>>>
>>>I just opened minicom on ttyS0 and gave a try. start_tx() was invoked
>>>each time I pressed a key (sent a character). I haven't seen stop_tx()
>>>even after after I closed minicom. I guess stop_tx() is invoked if you
>>>switch half-duplex communication.
>>
>>that's bad, I expected stop to be called also after each character.
>
>The 8250 core auto-stops tx when the tx ring buffer is empty (except
>in the case of dma, where stopping tx isn't necessary).

This is correct. So this is what I have now for the non-dma case:

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 2e4a93b..480a1c0 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -1283,6 +1283,9 @@ static inline void __stop_tx(struct uart_8250_port *p)
 	if (p->ier & UART_IER_THRI) {
 		p->ier &= ~UART_IER_THRI;
 		serial_out(p, UART_IER, p->ier);
+
+		pm_runtime_mark_last_busy(p->port.dev);
+		pm_runtime_put_autosuspend(p->port.dev);
 	}
 }
 
@@ -1310,12 +1313,12 @@ 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)) {
 		goto out;
 	} else if (!(up->ier & UART_IER_THRI)) {
 		up->ier |= UART_IER_THRI;
+		pm_runtime_get_sync(port->dev);
 		serial_port_out(port, UART_IER, up->ier);
 
 		if (up->bugs & UART_BUG_TXEN) {
 			unsigned char lsr;
@@ -1500,9 +1503,10 @@ void serial8250_tx_chars(struct uart_8250_port *up)
 		uart_write_wakeup(port);
 
 	DEBUG_INTR("THRE...");
-
+#if 0
 	if (uart_circ_empty(xmit))
 		__stop_tx(up);
+#endif
 }
 EXPORT_SYMBOL_GPL(serial8250_tx_chars);
 

and now I need to come up with something that is not if (port != omap)
for that #if 0 block. The code disables the TX FIFO empty interrupt once
the transfer is complete. I want to call __stop_tx() once the tx fifo is
empty.
Felipe, Would a check for dev->power.use_autosuspend be the right thing
to do?

>Regards,
>Peter Hurley

Sebastian

Re: [PATCH 4/5] tty: serial: 8250 core: add runtime pm

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3170 bytes --]

Hi,

On Thu, Jul 17, 2014 at 05:43:00PM +0200, Sebastian Andrzej Siewior wrote:
> * Peter Hurley | 2014-07-17 11:31:59 [-0400]:
> 
> >On 07/16/2014 12:06 PM, Felipe Balbi wrote:
> >>On Wed, Jul 16, 2014 at 05:54:56PM +0200, Sebastian Andrzej Siewior wrote:
> >>>On 07/16/2014 05:16 PM, Felipe Balbi wrote:
> >
> >>>>I wonder if you should get_sync() on start_tx() and only
> >>>>put_autosuspend() at stop_tx(). I guess the outcome would be
> >>>>largely the same, no ?
> >>>
> >>>I just opened minicom on ttyS0 and gave a try. start_tx() was invoked
> >>>each time I pressed a key (sent a character). I haven't seen stop_tx()
> >>>even after after I closed minicom. I guess stop_tx() is invoked if you
> >>>switch half-duplex communication.
> >>
> >>that's bad, I expected stop to be called also after each character.
> >
> >The 8250 core auto-stops tx when the tx ring buffer is empty (except
> >in the case of dma, where stopping tx isn't necessary).
> 
> This is correct. So this is what I have now for the non-dma case:
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 2e4a93b..480a1c0 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -1283,6 +1283,9 @@ static inline void __stop_tx(struct uart_8250_port *p)
>  	if (p->ier & UART_IER_THRI) {
>  		p->ier &= ~UART_IER_THRI;
>  		serial_out(p, UART_IER, p->ier);
> +
> +		pm_runtime_mark_last_busy(p->port.dev);
> +		pm_runtime_put_autosuspend(p->port.dev);
>  	}
>  }
>  
> @@ -1310,12 +1313,12 @@ 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)) {
>  		goto out;
>  	} else if (!(up->ier & UART_IER_THRI)) {
>  		up->ier |= UART_IER_THRI;
> +		pm_runtime_get_sync(port->dev);
>  		serial_port_out(port, UART_IER, up->ier);
>  
>  		if (up->bugs & UART_BUG_TXEN) {
>  			unsigned char lsr;

this looks better. So we get on start_tx() and put on stop_tx().

> @@ -1500,9 +1503,10 @@ void serial8250_tx_chars(struct uart_8250_port *up)
>  		uart_write_wakeup(port);
>  
>  	DEBUG_INTR("THRE...");
> -
> +#if 0
>  	if (uart_circ_empty(xmit))
>  		__stop_tx(up);
> +#endif
>  }
>  EXPORT_SYMBOL_GPL(serial8250_tx_chars);

is it so that start_tx() gets called one and stop_tx() might be called N
times ? That looks unbalanced to me. If the calls are balanced, then you
shouldn't need to care because pm_runtime will handle reference counting
for you, right?

> and now I need to come up with something that is not if (port != omap)
> for that #if 0 block. The code disables the TX FIFO empty interrupt once
> the transfer is complete. I want to call __stop_tx() once the tx fifo is
> empty.
> Felipe, Would a check for dev->power.use_autosuspend be the right thing
> to do?

probably not, as that's internal to the pm_runtime code. But I wonder if
start/stop tx calls are balanced, if they are then we're good. Unless
I'm missing something else.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/5] tty: serial: 8250 core: add runtime pm

[Sebastian Andrzej Siewior]

On 07/17/2014 06:02 PM, Felipe Balbi wrote:
>> diff --git a/drivers/tty/serial/8250/8250_core.c
>> b/drivers/tty/serial/8250/8250_core.c index 2e4a93b..480a1c0
>> 100644 --- a/drivers/tty/serial/8250/8250_core.c +++
>> b/drivers/tty/serial/8250/8250_core.c @@ -1283,6 +1283,9 @@
>> static inline void __stop_tx(struct uart_8250_port *p) if (p->ier
>> & UART_IER_THRI) { p->ier &= ~UART_IER_THRI; serial_out(p,
>> UART_IER, p->ier); + +		pm_runtime_mark_last_busy(p->port.dev); +
>> pm_runtime_put_autosuspend(p->port.dev); } }
>> 
>> @@ -1310,12 +1313,12 @@ 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)) { goto out; } else if (!(up->ier &
>> UART_IER_THRI)) { up->ier |= UART_IER_THRI; +
>> pm_runtime_get_sync(port->dev); serial_port_out(port, UART_IER,
>> up->ier);
>> 
>> if (up->bugs & UART_BUG_TXEN) { unsigned char lsr;
> 
> this looks better. So we get on start_tx() and put on stop_tx().
> 
>> @@ -1500,9 +1503,10 @@ void serial8250_tx_chars(struct
>> uart_8250_port *up) uart_write_wakeup(port);
>> 
>> DEBUG_INTR("THRE..."); - +#if 0 if (uart_circ_empty(xmit)) 
>> __stop_tx(up); +#endif } EXPORT_SYMBOL_GPL(serial8250_tx_chars);
> 
> is it so that start_tx() gets called one and stop_tx() might be
> called N times ? That looks unbalanced to me. If the calls are
> balanced, then you shouldn't need to care because pm_runtime will
> handle reference counting for you, right?

No, this is okay. If you look, it checks for "up->ier &
UART_IER_THRI". On the second invocation it will see that this bit is
already set and therefore won't call get_sync() for the second time.
That bit is removed in the _stop_tx() path.

>> and now I need to come up with something that is not if (port !=
>> omap) for that #if 0 block. The code disables the TX FIFO empty
>> interrupt once the transfer is complete. I want to call
>> __stop_tx() once the tx fifo is empty. Felipe, Would a check for
>> dev->power.use_autosuspend be the right thing to do?
> 
> probably not, as that's internal to the pm_runtime code. But I
> wonder if start/stop tx calls are balanced, if they are then we're
> good. Unless I'm missing something else.

Do you have other ideas? It doesn't look like this is exported at all.
If we call _stop_tx() right away, then we have 64 bytes in the TX fifo
in the worst case. They should be gone "soon" but the HW-flow control
may delay it (in theory for a long time)).

Sebastian

Re: [PATCH 4/5] tty: serial: 8250 core: add runtime pm

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3748 bytes --]

Hi,

On Thu, Jul 17, 2014 at 06:06:59PM +0200, Sebastian Andrzej Siewior wrote:
> On 07/17/2014 06:02 PM, Felipe Balbi wrote:
> >> diff --git a/drivers/tty/serial/8250/8250_core.c
> >> b/drivers/tty/serial/8250/8250_core.c index 2e4a93b..480a1c0
> >> 100644 --- a/drivers/tty/serial/8250/8250_core.c +++
> >> b/drivers/tty/serial/8250/8250_core.c @@ -1283,6 +1283,9 @@
> >> static inline void __stop_tx(struct uart_8250_port *p) if (p->ier
> >> & UART_IER_THRI) { p->ier &= ~UART_IER_THRI; serial_out(p,
> >> UART_IER, p->ier); + +		pm_runtime_mark_last_busy(p->port.dev); +
> >> pm_runtime_put_autosuspend(p->port.dev); } }
> >> 
> >> @@ -1310,12 +1313,12 @@ 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)) { goto out; } else if (!(up->ier &
> >> UART_IER_THRI)) { up->ier |= UART_IER_THRI; +
> >> pm_runtime_get_sync(port->dev); serial_port_out(port, UART_IER,
> >> up->ier);
> >> 
> >> if (up->bugs & UART_BUG_TXEN) { unsigned char lsr;
> > 
> > this looks better. So we get on start_tx() and put on stop_tx().
> > 
> >> @@ -1500,9 +1503,10 @@ void serial8250_tx_chars(struct
> >> uart_8250_port *up) uart_write_wakeup(port);
> >> 
> >> DEBUG_INTR("THRE..."); - +#if 0 if (uart_circ_empty(xmit)) 
> >> __stop_tx(up); +#endif } EXPORT_SYMBOL_GPL(serial8250_tx_chars);
> > 
> > is it so that start_tx() gets called one and stop_tx() might be
> > called N times ? That looks unbalanced to me. If the calls are
> > balanced, then you shouldn't need to care because pm_runtime will
> > handle reference counting for you, right?
> 
> No, this is okay. If you look, it checks for "up->ier &
> UART_IER_THRI". On the second invocation it will see that this bit is
> already set and therefore won't call get_sync() for the second time.
> That bit is removed in the _stop_tx() path.

oh, right. But that's actually unnecessary. Calling pm_runtime_get()
multiple times will just increment the usage counter multiple times,
which means you can call __stop_tx() multiple times too and everything
gets balanced, right ?

> >> and now I need to come up with something that is not if (port !=
> >> omap) for that #if 0 block. The code disables the TX FIFO empty
> >> interrupt once the transfer is complete. I want to call
> >> __stop_tx() once the tx fifo is empty. Felipe, Would a check for
> >> dev->power.use_autosuspend be the right thing to do?
> > 
> > probably not, as that's internal to the pm_runtime code. But I
> > wonder if start/stop tx calls are balanced, if they are then we're
> > good. Unless I'm missing something else.
> 
> Do you have other ideas? It doesn't look like this is exported at all.
> If we call _stop_tx() right away, then we have 64 bytes in the TX fifo
> in the worst case. They should be gone "soon" but the HW-flow control
> may delay it (in theory for a long time)).

this can be problematic, specially for OMAP which can go into OFF while
idle. Whatever is in the FIFO would get lost. It seems like omap-serial
solved this within transmit_chars().

See how transmit_chars() is called from within IRQ handler with clocks
enabled then it conditionally calls serial_omap_stop_tx() which will
pm_runtime_get_sync() -> do_the_harlem_shake() ->
pm_runtime_put_autosuspend(). That leaves one unbalanced
pm_runtime_get() which is balanced when we're exitting the IRQ handler.

This seems work fine and dandy without DMA, but for DMA work, I think we
need to make sure this IP stays powered until we get DMA completion
callback. But that's future, I guess.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/5] tty: serial: 8250 core: add runtime pm

[Sebastian Andrzej Siewior]

On 07/17/2014 06:18 PM, Felipe Balbi wrote:

>> No, this is okay. If you look, it checks for "up->ier & 
>> UART_IER_THRI". On the second invocation it will see that this
>> bit is already set and therefore won't call get_sync() for the
>> second time. That bit is removed in the _stop_tx() path.
> 
> oh, right. But that's actually unnecessary. Calling
> pm_runtime_get() multiple times will just increment the usage
> counter multiple times, which means you can call __stop_tx()
> multiple times too and everything gets balanced, right ?

No. start_tx() will be called multiple times but only the first
invocation invoke pm_runtime_get(). Now I noticed that I forgot to
remove pm_runtime_put_autosuspend() at the bottom of it. But you get
the idea right?
pm_get() on the while the UART_IER_THRI is not yet set. pm_put() once
the fifo is completely empty.

>> Do you have other ideas? It doesn't look like this is exported at
>> all. If we call _stop_tx() right away, then we have 64 bytes in
>> the TX fifo in the worst case. They should be gone "soon" but the
>> HW-flow control may delay it (in theory for a long time)).
> 
> this can be problematic, specially for OMAP which can go into OFF
> while idle. Whatever is in the FIFO would get lost. It seems like
> omap-serial solved this within transmit_chars().

No, it didn't.

> See how transmit_chars() is called from within IRQ handler with
> clocks enabled then it conditionally calls serial_omap_stop_tx()
> which will pm_runtime_get_sync() -> do_the_harlem_shake() -> 
> pm_runtime_put_autosuspend(). That leaves one unbalanced 
> pm_runtime_get() which is balanced when we're exitting the IRQ
> handler.

omap-serial and the 8250 do the following on tx path:
- start_tx()
  -> sets UART_IER_THRI. This will generate an interrupt once the FIFO
     is empty.
- interrupt, notices the empty fifo, invokes serial8250_start_tx()/
  transmit_chars().
  Both have a while loop that fills the FIFO. This loop is left once
  the tty-buffer is empty (uart_circ_empty() is true) or the FIFO full.

Lets say you filled 64 bytes into the FIFO and then left because your
FIFO is full and tty-buffer is empty. That means you will invoke
serial_omap_stop_tx() and remove UART_IER_THRI bit.
This is okay because you are not interested in further FIFO empty
interrupts because you don't have any TX-bytes to be sent. However,
once you leave the transmit_chars() you leave serial_omap_irq() which
does the last pm_put(). That means you have data in the TX FIFO that is
about to be sent and the device is in auto-suspend.
This is "fine" as long as the timeout is greater then the time required
for the data be sent (plus assuming HW-float control does not stall it
for too long) so nobody notices a thing.

For that reason I added the hack / #if0 block in the 8250 driver. To
ensure we do not disable the TX-FIFO-empty interrupt even if there is
nothing to send. Instead we enter serial8250_tx_chars() once again with
empty FIFO and empty tty-buffer and will invoke _stop_tx() which also
finally does the pm_put().
That is the plan. The problem I have is how to figure out that the
device is using auto-suspend. If I don't then I would have to remove
the #if0 block and that would mean for everybody an extra interrupt
(which I wanted to avoid).

> This seems work fine and dandy without DMA, but for DMA work, I
> think we need to make sure this IP stays powered until we get DMA
> completion callback. But that's future, I guess.

Yes, probably. That means one get at dma start, one put at dma complete
callback. And I assume we get that callbacks once the DMA transfer is
complete, not when the FIFO is empty :) So lets leave it to the future
for now…

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

Re: [PATCH 4/5] tty: serial: 8250 core: add runtime pm

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4801 bytes --]

On Fri, Jul 18, 2014 at 10:35:10AM +0200, Sebastian Andrzej Siewior wrote:
> On 07/17/2014 06:18 PM, Felipe Balbi wrote:
> 
> >> No, this is okay. If you look, it checks for "up->ier & 
> >> UART_IER_THRI". On the second invocation it will see that this
> >> bit is already set and therefore won't call get_sync() for the
> >> second time. That bit is removed in the _stop_tx() path.
> > 
> > oh, right. But that's actually unnecessary. Calling
> > pm_runtime_get() multiple times will just increment the usage
> > counter multiple times, which means you can call __stop_tx()
> > multiple times too and everything gets balanced, right ?
> 
> No. start_tx() will be called multiple times but only the first
> invocation invoke pm_runtime_get(). Now I noticed that I forgot to

right, but that's unnecessary. You can pm_runtime_get() every time
start_tx() is called. Just make sure to put everytime stop_tx() is
called too.

> remove pm_runtime_put_autosuspend() at the bottom of it. But you get
> the idea right?
> pm_get() on the while the UART_IER_THRI is not yet set. pm_put() once
> the fifo is completely empty.
> 
> >> Do you have other ideas? It doesn't look like this is exported at
> >> all. If we call _stop_tx() right away, then we have 64 bytes in
> >> the TX fifo in the worst case. They should be gone "soon" but the
> >> HW-flow control may delay it (in theory for a long time)).
> > 
> > this can be problematic, specially for OMAP which can go into OFF
> > while idle. Whatever is in the FIFO would get lost. It seems like
> > omap-serial solved this within transmit_chars().
> 
> No, it didn't.
> 
> > See how transmit_chars() is called from within IRQ handler with
> > clocks enabled then it conditionally calls serial_omap_stop_tx()
> > which will pm_runtime_get_sync() -> do_the_harlem_shake() -> 
> > pm_runtime_put_autosuspend(). That leaves one unbalanced 
> > pm_runtime_get() which is balanced when we're exitting the IRQ
> > handler.
> 
> omap-serial and the 8250 do the following on tx path:
> - start_tx()
>   -> sets UART_IER_THRI. This will generate an interrupt once the FIFO
>      is empty.
> - interrupt, notices the empty fifo, invokes serial8250_start_tx()/
>   transmit_chars().
>   Both have a while loop that fills the FIFO. This loop is left once
>   the tty-buffer is empty (uart_circ_empty() is true) or the FIFO full.
> 
> Lets say you filled 64 bytes into the FIFO and then left because your
> FIFO is full and tty-buffer is empty. That means you will invoke
> serial_omap_stop_tx() and remove UART_IER_THRI bit.
> This is okay because you are not interested in further FIFO empty
> interrupts because you don't have any TX-bytes to be sent. However,
> once you leave the transmit_chars() you leave serial_omap_irq() which
> does the last pm_put(). That means you have data in the TX FIFO that is
> about to be sent and the device is in auto-suspend.
> This is "fine" as long as the timeout is greater then the time required
> for the data be sent (plus assuming HW-float control does not stall it
> for too long) so nobody notices a thing.

the time is set to -1 by default. I guess this only works because nobody
has ever tested long transfers with slow baud rates :-p

> For that reason I added the hack / #if0 block in the 8250 driver. To
> ensure we do not disable the TX-FIFO-empty interrupt even if there is
> nothing to send. Instead we enter serial8250_tx_chars() once again with
> empty FIFO and empty tty-buffer and will invoke _stop_tx() which also
> finally does the pm_put().
> That is the plan. The problem I have is how to figure out that the
> device is using auto-suspend. If I don't then I would have to remove
> the #if0 block and that would mean for everybody an extra interrupt
> (which I wanted to avoid).

looks like the closest you have is:

if (pm_runtime_autosuspend_expiration(dev) > 0)
	foo();

Another possibility would be to implement the ->runtime_idle() callback
and only return 0 if fifo is empty, otherwise return -EAGAIN ? then, if
the autosuspend timer expires, ->runtime_idle gets called and you can do
the right thing depending on fifo empty or not.

Take a look at
drivers/usb/core/driver.c::usb_runtime_{idle,resume,suspend} for
examples. That seems to work pretty well.

> > This seems work fine and dandy without DMA, but for DMA work, I
> > think we need to make sure this IP stays powered until we get DMA
> > completion callback. But that's future, I guess.
> 
> Yes, probably. That means one get at dma start, one put at dma complete
> callback. And I assume we get that callbacks once the DMA transfer is
> complete, not when the FIFO is empty :) So lets leave it to the future
> for now…

k

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/5] tty: serial: 8250 core: add runtime pm

[Peter Hurley]

On 07/18/2014 11:31 AM, Felipe Balbi wrote:
> On Fri, Jul 18, 2014 at 10:35:10AM +0200, Sebastian Andrzej Siewior wrote:
>> >On 07/17/2014 06:18 PM, Felipe Balbi wrote:
>> >
>>>> > >>No, this is okay. If you look, it checks for "up->ier &
>>>> > >>UART_IER_THRI". On the second invocation it will see that this
>>>> > >>bit is already set and therefore won't call get_sync() for the
>>>> > >>second time. That bit is removed in the _stop_tx() path.
>>> > >
>>> > >oh, right. But that's actually unnecessary. Calling
>>> > >pm_runtime_get() multiple times will just increment the usage
>>> > >counter multiple times, which means you can call __stop_tx()
>>> > >multiple times too and everything gets balanced, right ?
>> >
>> >No. start_tx() will be called multiple times but only the first
>> >invocation invoke pm_runtime_get(). Now I noticed that I forgot to
> right, but that's unnecessary. You can pm_runtime_get() every time
> start_tx() is called. Just make sure to put everytime stop_tx() is
> called too.

The interface is asymmetric.

start_tx() may be invoked multiple times for which only 1 interrupt
will occur, and thus only invoke __stop_tx() once.

Regards,
Peter Hurley

Re: [PATCH 4/5] tty: serial: 8250 core: add runtime pm

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1327 bytes --]

On Fri, Jul 18, 2014 at 11:53:21AM -0400, Peter Hurley wrote:
> On 07/18/2014 11:31 AM, Felipe Balbi wrote:
> >On Fri, Jul 18, 2014 at 10:35:10AM +0200, Sebastian Andrzej Siewior wrote:
> >>>On 07/17/2014 06:18 PM, Felipe Balbi wrote:
> >>>
> >>>>> >>No, this is okay. If you look, it checks for "up->ier &
> >>>>> >>UART_IER_THRI". On the second invocation it will see that this
> >>>>> >>bit is already set and therefore won't call get_sync() for the
> >>>>> >>second time. That bit is removed in the _stop_tx() path.
> >>>> >
> >>>> >oh, right. But that's actually unnecessary. Calling
> >>>> >pm_runtime_get() multiple times will just increment the usage
> >>>> >counter multiple times, which means you can call __stop_tx()
> >>>> >multiple times too and everything gets balanced, right ?
> >>>
> >>>No. start_tx() will be called multiple times but only the first
> >>>invocation invoke pm_runtime_get(). Now I noticed that I forgot to
> >right, but that's unnecessary. You can pm_runtime_get() every time
> >start_tx() is called. Just make sure to put everytime stop_tx() is
> >called too.
> 
> The interface is asymmetric.
> 
> start_tx() may be invoked multiple times for which only 1 interrupt
> will occur, and thus only invoke __stop_tx() once.

alright, thanks for the info.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 5/5] tty: serial: Add 8250-core based omap driver

[Sebastian Andrzej Siewior]

This patch provides a 8250-core based UART driver for the internal OMAP
UART. The longterm goal is to provide the same functionality as the
current OMAP uart driver and hopefully DMA support which could borrowed
from the 8250-core.

It has been only tested as console UART on am335x-evm and dra7-evm.
The device name is ttyS based instead of ttyO. If a ttyO based node name
is required please ask udev for it. If both driver are activated (this
and omap-serial) then this serial driver will take control over the
device due to the link order

v3…v4:
	- drop RS485 support
	- wire up ->throttle / ->unthrottle
v2…v3:
	- wire up startup & shutdown for wakeup-irq handling.
	- RS485 handling (well the core does).

v1…v2:
	- added runtime PM. Could somebody could plese double check
	  this?
	- added omap_8250_set_termios()

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/tty/serial/8250/8250_core.c |   8 +
 drivers/tty/serial/8250/8250_omap.c | 901 ++++++++++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig     |   9 +
 drivers/tty/serial/8250/Makefile    |   1 +
 include/uapi/linux/serial_core.h    |   3 +-
 5 files changed, 921 insertions(+), 1 deletion(-)
 create mode 100644 drivers/tty/serial/8250/8250_omap.c

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index aceaea1..7a33c30 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -261,6 +261,12 @@ static const struct serial8250_config uart_config[] = {
 		.fcr		= UART_FCR_ENABLE_FIFO | UART_FCR_R_TRIG_10,
 		.flags		= UART_CAP_FIFO | UART_CAP_AFE,
 	},
+	[PORT_OMAP_16750] = {
+		.name		= "OMAP",
+		.fifo_size	= 64,
+		.tx_loadsz	= 64,
+		.flags		= UART_CAP_FIFO | UART_CAP_EFR | UART_CAP_SLEEP,
+	},
 	[PORT_TEGRA] = {
 		.name		= "Tegra",
 		.fifo_size	= 32,
@@ -1350,6 +1356,8 @@ static void serial8250_stop_rx(struct uart_port *port)
 	pm_runtime_get_sync(port->dev);
 
 	up->ier &= ~UART_IER_RLSI;
+	if (port->type == PORT_OMAP_16750)
+		up->ier &= ~UART_IER_RDI;
 	up->port.read_status_mask &= ~UART_LSR_DR;
 	serial_port_out(port, UART_IER, up->ier);
 
diff --git a/drivers/tty/serial/8250/8250_omap.c b/drivers/tty/serial/8250/8250_omap.c
new file mode 100644
index 0000000..4d01f45
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_omap.c
@@ -0,0 +1,901 @@
+/*
+ * 8250-core based driver for the OMAP internal UART
+ *
+ *  Copyright (C) 2014 Sebastian Andrzej Siewior
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/serial_8250.h>
+#include <linux/serial_core.h>
+#include <linux/serial_reg.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/of_irq.h>
+#include <linux/delay.h>
+#include <linux/pm_runtime.h>
+#include <linux/console.h>
+#include <linux/pm_qos.h>
+
+#include "8250.h"
+
+#define UART_DLL_EM 9
+#define UART_DLM_EM 10
+
+#define DEFAULT_CLK_SPEED	48000000 /* 48 Mhz*/
+
+#define UART_ERRATA_i202_MDR1_ACCESS	(1 << 0)
+#define OMAP_UART_WER_HAS_TX_WAKEUP	(1 << 1)
+
+/* SCR register bitmasks */
+#define OMAP_UART_SCR_RX_TRIG_GRANU1_MASK	(1 << 7)
+#define OMAP_UART_SCR_TX_TRIG_GRANU1_MASK	(1 << 6)
+#define OMAP_UART_SCR_TX_EMPTY			(1 << 3)
+
+/* MVR register bitmasks */
+#define OMAP_UART_MVR_SCHEME_SHIFT	30
+#define OMAP_UART_LEGACY_MVR_MAJ_MASK	0xf0
+#define OMAP_UART_LEGACY_MVR_MAJ_SHIFT	4
+#define OMAP_UART_LEGACY_MVR_MIN_MASK	0x0f
+#define OMAP_UART_MVR_MAJ_MASK		0x700
+#define OMAP_UART_MVR_MAJ_SHIFT		8
+#define OMAP_UART_MVR_MIN_MASK		0x3f
+
+/* Enable XON/XOFF flow control on output */
+#define OMAP_UART_SW_TX		0x08
+/* Enable XON/XOFF flow control on input */
+#define OMAP_UART_SW_RX		0x02
+#define OMAP_UART_SW_CLR	0xf0
+#define OMAP_UART_TCR_TRIG	0x0f
+
+#define OMAP_UART_WER_MOD_WKUP	0x7f
+#define OMAP_UART_TX_WAKEUP_EN	(1 << 7)
+
+#define UART_BUILD_REVISION(x, y)	(((x) << 8) | (y))
+
+#define OMAP_UART_REV_46 0x0406
+#define OMAP_UART_REV_52 0x0502
+#define OMAP_UART_REV_63 0x0603
+
+struct omap8250_priv {
+	int line;
+	u32 habit;
+	u32 fcr;
+	u32 mdr1;
+	u32 efr;
+	u32 quot;
+	u32 scr;
+	u32 wer;
+
+	bool is_suspending;
+	int wakeirq;
+	int wakeups_enabled;
+	u32 latency;
+	u32 calc_latency;
+	struct pm_qos_request pm_qos_request;
+	struct work_struct qos_work;
+};
+
+static u32 uart_read(struct uart_8250_port *up, u32 reg)
+{
+	return readl(up->port.membase + (reg << up->port.regshift));
+}
+
+/*
+ * Work Around for Errata i202 (2430, 3430, 3630, 4430 and 4460)
+ * The access to uart register after MDR1 Access
+ * causes UART to corrupt data.
+ *
+ * Need a delay =
+ * 5 L4 clock cycles + 5 UART functional clock cycle (@48MHz = ~0.2uS)
+ * give 10 times as much
+ */
+static void omap_8250_mdr1_errataset(struct uart_8250_port *up, u8 mdr1)
+{
+	struct omap8250_priv *priv = up->port.private_data;
+	u8 timeout = 255;
+
+	serial_out(up, UART_OMAP_MDR1, mdr1);
+	udelay(2);
+	serial_out(up, UART_FCR, priv->fcr | UART_FCR_CLEAR_XMIT |
+			UART_FCR_CLEAR_RCVR);
+	/*
+	 * Wait for FIFO to empty: when empty, RX_FIFO_E bit is 0 and
+	 * TX_FIFO_E bit is 1.
+	 */
+	while (UART_LSR_THRE != (serial_in(up, UART_LSR) &
+				(UART_LSR_THRE | UART_LSR_DR))) {
+		timeout--;
+		if (!timeout) {
+			/* Should *never* happen. we warn and carry on */
+			dev_crit(up->port.dev, "Errata i202: timedout %x\n",
+						serial_in(up, UART_LSR));
+			break;
+		}
+		udelay(1);
+	}
+}
+
+static void omap_8250_get_divisor(struct uart_port *port, unsigned int baud,
+		struct omap8250_priv *priv)
+{
+	unsigned int uartclk = port->uartclk;
+	unsigned int div_13, div_16;
+	unsigned int abs_d13, abs_d16;
+
+	/*
+	 * Old custom speed handling.
+	 */
+	if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST) {
+		priv->quot = port->custom_divisor & 0xffff;
+		/*
+		 * I assume that nobody is using this. But hey, if somebody
+		 * would like to specify the divisor _and_ the mode then the
+		 * driver is ready and waiting for it.
+		 */
+		if (port->custom_divisor & (1 << 16))
+			priv->mdr1 = UART_OMAP_MDR1_13X_MODE;
+		else
+			priv->mdr1 = UART_OMAP_MDR1_16X_MODE;
+		return;
+	}
+	div_13 = DIV_ROUND_CLOSEST(uartclk, 13 * baud);
+	div_16 = DIV_ROUND_CLOSEST(uartclk, 16 * baud);
+
+	abs_d13 = abs(baud - port->uartclk / 13 / div_13);
+	abs_d16 = abs(baud - port->uartclk / 16 / div_16);
+
+	if (abs_d13 >= abs_d16) {
+		priv->mdr1 = UART_OMAP_MDR1_16X_MODE;
+		priv->quot = div_16;
+	} else {
+		priv->mdr1 = UART_OMAP_MDR1_13X_MODE;
+		priv->quot = div_13;
+	}
+}
+
+/*
+ * OMAP can use "CLK / (16 or 13) / div" for baud rate. And then we have have
+ * some differences in how we want to handle flow control.
+ */
+static void omap_8250_set_termios(struct uart_port *port,
+		struct ktermios *termios, struct ktermios *old)
+{
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
+	struct omap8250_priv *priv = up->port.private_data;
+	unsigned char cval = 0;
+	unsigned long flags = 0;
+	unsigned int baud;
+
+	switch (termios->c_cflag & CSIZE) {
+	case CS5:
+		cval = UART_LCR_WLEN5;
+		break;
+	case CS6:
+		cval = UART_LCR_WLEN6;
+		break;
+	case CS7:
+		cval = UART_LCR_WLEN7;
+		break;
+	default:
+	case CS8:
+		cval = UART_LCR_WLEN8;
+		break;
+	}
+
+	if (termios->c_cflag & CSTOPB)
+		cval |= UART_LCR_STOP;
+	if (termios->c_cflag & PARENB)
+		cval |= UART_LCR_PARITY;
+	if (!(termios->c_cflag & PARODD))
+		cval |= UART_LCR_EPAR;
+	if (termios->c_cflag & CMSPAR)
+		cval |= UART_LCR_SPAR;
+
+	/*
+	 * Ask the core to calculate the divisor for us.
+	 */
+	baud = uart_get_baud_rate(port, termios, old,
+			port->uartclk / 16 / 0xffff,
+			port->uartclk / 13);
+	omap_8250_get_divisor(port, baud, priv);
+
+	/*
+	 * 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);
+
+	/*
+	 * Update the per-port timeout.
+	 */
+	uart_update_timeout(port, termios->c_cflag, baud);
+
+	up->port.read_status_mask = UART_LSR_OE | UART_LSR_THRE | UART_LSR_DR;
+	if (termios->c_iflag & INPCK)
+		up->port.read_status_mask |= UART_LSR_FE | UART_LSR_PE;
+	if (termios->c_iflag & (BRKINT | PARMRK))
+		up->port.read_status_mask |= UART_LSR_BI;
+
+	/*
+	 * Characters to ignore
+	 */
+	up->port.ignore_status_mask = 0;
+	if (termios->c_iflag & IGNPAR)
+		up->port.ignore_status_mask |= UART_LSR_PE | UART_LSR_FE;
+	if (termios->c_iflag & IGNBRK) {
+		up->port.ignore_status_mask |= UART_LSR_BI;
+		/*
+		 * If we're ignoring parity and break indicators,
+		 * ignore overruns too (for real raw support).
+		 */
+		if (termios->c_iflag & IGNPAR)
+			up->port.ignore_status_mask |= UART_LSR_OE;
+	}
+
+	/*
+	 * ignore all characters if CREAD is not set
+	 */
+	if ((termios->c_cflag & CREAD) == 0)
+		up->port.ignore_status_mask |= UART_LSR_DR;
+
+	/*
+	 * Modem status interrupts
+	 */
+	up->ier &= ~UART_IER_MSI;
+	if (UART_ENABLE_MS(&up->port, termios->c_cflag))
+		up->ier |= UART_IER_MSI;
+	serial_out(up, UART_IER, up->ier);
+
+	serial_out(up, UART_LCR, cval);         /* reset DLAB */
+	up->lcr = cval;
+
+	/* Up to here it was mostly serial8250_do_set_termios() */
+
+	/* FCR can be changed only when the
+	 * baud clock is not running
+	 * DLL_REG and DLH_REG set to 0.
+	 */
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
+	serial_out(up, UART_DLL, 0);
+	serial_out(up, UART_DLM, 0);
+	serial_out(up, UART_LCR, 0);
+
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+	serial_out(up, UART_EFR, UART_EFR_ECB);
+
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
+	serial_out(up, UART_MCR, up->mcr | UART_MCR_TCRTLR);
+
+	priv->scr = OMAP_UART_SCR_RX_TRIG_GRANU1_MASK | OMAP_UART_SCR_TX_EMPTY;
+	/*
+	 * NOTE: Setting OMAP_UART_SCR_RX_TRIG_GRANU1_MASK sets Enables the
+	 * granularity of 1 for TRIGGER RX level. Along with setting RX FIFO
+	 * trigger level to 1 (as noted below, 16 characters) and TLR[3:0] to
+	 * zero this will result RX FIFO threshold level to 1 character.
+	 * Transmit FIFO threshold is set to 32 spaces. With SCR_TX_EMPTY set,
+	 * we receive an interrupt once TX FIFO (and shift) is empty as this is
+	 * what The irq routine currently expects to happen.
+	 */
+	priv->fcr = UART_FCR6_R_TRIGGER_16 | UART_FCR6_T_TRIGGER_24 |
+		UART_FCR_ENABLE_FIFO;
+
+	serial_out(up, UART_FCR, priv->fcr);
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+
+	serial_out(up, UART_OMAP_SCR, priv->scr);
+
+	/* Reset UART_MCR_TCRTLR: this must be done with the EFR_ECB bit set */
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
+	serial_out(up, UART_MCR, up->mcr);
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+	serial_out(up, UART_EFR, 0);
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
+
+	/* Protocol, Baud Rate, and Interrupt Settings */
+
+	if (priv->habit & UART_ERRATA_i202_MDR1_ACCESS)
+		omap_8250_mdr1_errataset(up, UART_OMAP_MDR1_DISABLE);
+	else
+		serial_out(up, UART_OMAP_MDR1, UART_OMAP_MDR1_DISABLE);
+
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+	serial_out(up, UART_EFR, UART_EFR_ECB);
+
+	serial_out(up, UART_LCR, 0);
+	serial_out(up, UART_IER, 0);
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+
+	serial_dl_write(up, priv->quot);
+
+	serial_out(up, UART_LCR, 0);
+	serial_out(up, UART_IER, up->ier);
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+
+	serial_out(up, UART_EFR, 0);
+	serial_out(up, UART_LCR, up->lcr);
+
+	if (priv->habit & UART_ERRATA_i202_MDR1_ACCESS)
+		omap_8250_mdr1_errataset(up, priv->mdr1);
+	else
+		serial_out(up, UART_OMAP_MDR1, priv->mdr1);
+
+	/* Configure flow control */
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+
+	/* XON1/XOFF1 accessible mode B, TCRTLR=0, ECB=0 */
+	serial_out(up, UART_XON1, termios->c_cc[VSTART]);
+	serial_out(up, UART_XOFF1, termios->c_cc[VSTOP]);
+
+	/* Enable access to TCR/TLR */
+	serial_out(up, UART_EFR, UART_EFR_ECB);
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
+	serial_out(up, UART_MCR, up->mcr | UART_MCR_TCRTLR);
+
+	serial_out(up, UART_TI752_TCR, OMAP_UART_TCR_TRIG);
+
+	priv->efr = 0;
+	if (termios->c_cflag & CRTSCTS && up->port.flags & UPF_HARD_FLOW) {
+		/* Enable AUTORTS and AUTOCTS */
+		priv->efr |= UART_EFR_CTS | UART_EFR_RTS;
+
+		/* Ensure MCR RTS is asserted */
+		up->mcr |= UART_MCR_RTS;
+	}
+
+	if (up->port.flags & UPF_SOFT_FLOW) {
+		/*
+		 * IXON Flag:
+		 * Enable XON/XOFF flow control on input.
+		 * Receiver compares XON1, XOFF1.
+		 */
+		if (termios->c_iflag & IXON)
+			priv->efr |= OMAP_UART_SW_RX;
+
+		/*
+		 * IXOFF Flag:
+		 * Enable XON/XOFF flow control on output.
+		 * Transmit XON1, XOFF1
+		 */
+		if (termios->c_iflag & IXOFF)
+			priv->efr |= OMAP_UART_SW_TX;
+
+		/*
+		 * IXANY Flag:
+		 * Enable any character to restart output.
+		 * Operation resumes after receiving any
+		 * character after recognition of the XOFF character
+		 */
+		if (termios->c_iflag & IXANY)
+			up->mcr |= UART_MCR_XONANY;
+		else
+			up->mcr &= ~UART_MCR_XONANY;
+	}
+	serial_out(up, UART_MCR, up->mcr);
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+	serial_out(up, UART_EFR, 0);
+	serial_out(up, UART_LCR, up->lcr);
+
+	port->ops->set_mctrl(port, port->mctrl);
+
+	spin_unlock_irqrestore(&up->port.lock, flags);
+	pm_runtime_mark_last_busy(port->dev);
+	pm_runtime_put_autosuspend(port->dev);
+
+	/* calculate wakeup latency constraint */
+	priv->calc_latency = (USEC_PER_SEC * up->port.fifosize) / (baud / 8);
+	priv->latency = priv->calc_latency;
+	schedule_work(&priv->qos_work);
+
+	/* Don't rewrite B0 */
+	if (tty_termios_baud_rate(termios))
+		tty_termios_encode_baud_rate(termios, baud, baud);
+}
+
+/* same as 8250 except that we may have extra flow bits set in EFR */
+static void omap_8250_pm(struct uart_port *port, unsigned int state,
+		unsigned int oldstate)
+{
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
+	struct omap8250_priv *priv = up->port.private_data;
+
+	pm_runtime_get_sync(port->dev);
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+	serial_out(up, UART_EFR, priv->efr | UART_EFR_ECB);
+	serial_out(up, UART_LCR, 0);
+
+	serial_out(up, UART_IER, (state != 0) ? UART_IERX_SLEEP : 0);
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
+	serial_out(up, UART_EFR, priv->efr);
+	serial_out(up, UART_LCR, 0);
+
+	pm_runtime_mark_last_busy(port->dev);
+	pm_runtime_put_autosuspend(port->dev);
+}
+
+static void omap_serial_fill_features_erratas(struct uart_8250_port *up,
+		struct omap8250_priv *priv)
+{
+	u32 mvr, scheme;
+	u16 revision, major, minor;
+
+	mvr = uart_read(up, UART_OMAP_MVER);
+
+	/* Check revision register scheme */
+	scheme = mvr >> OMAP_UART_MVR_SCHEME_SHIFT;
+
+	switch (scheme) {
+	case 0: /* Legacy Scheme: OMAP2/3 */
+		/* MINOR_REV[0:4], MAJOR_REV[4:7] */
+		major = (mvr & OMAP_UART_LEGACY_MVR_MAJ_MASK) >>
+			OMAP_UART_LEGACY_MVR_MAJ_SHIFT;
+		minor = (mvr & OMAP_UART_LEGACY_MVR_MIN_MASK);
+		break;
+	case 1:
+		/* New Scheme: OMAP4+ */
+		/* MINOR_REV[0:5], MAJOR_REV[8:10] */
+		major = (mvr & OMAP_UART_MVR_MAJ_MASK) >>
+			OMAP_UART_MVR_MAJ_SHIFT;
+		minor = (mvr & OMAP_UART_MVR_MIN_MASK);
+		break;
+	default:
+		dev_warn(up->port.dev,
+				"Unknown revision, defaulting to highest\n");
+		/* highest possible revision */
+		major = 0xff;
+		minor = 0xff;
+	}
+	/* normalize revision for the driver */
+	revision = UART_BUILD_REVISION(major, minor);
+
+	switch (revision) {
+	case OMAP_UART_REV_46:
+		priv->habit = UART_ERRATA_i202_MDR1_ACCESS;
+		break;
+	case OMAP_UART_REV_52:
+		priv->habit = UART_ERRATA_i202_MDR1_ACCESS |
+				OMAP_UART_WER_HAS_TX_WAKEUP;
+		break;
+	case OMAP_UART_REV_63:
+		priv->habit = UART_ERRATA_i202_MDR1_ACCESS |
+			OMAP_UART_WER_HAS_TX_WAKEUP;
+		break;
+	default:
+		break;
+	}
+}
+
+static void omap8250_uart_qos_work(struct work_struct *work)
+{
+	struct omap8250_priv *priv;
+
+	priv = container_of(work, struct omap8250_priv, qos_work);
+	pm_qos_update_request(&priv->pm_qos_request, priv->latency);
+}
+
+static irqreturn_t omap_wake_irq(int irq, void *dev_id)
+{
+	struct uart_port *port = dev_id;
+	int ret;
+
+	ret = port->handle_irq(port);
+	if (ret)
+		return IRQ_HANDLED;
+	return IRQ_NONE;
+}
+
+static int omap_8250_startup(struct uart_port *port)
+{
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
+	struct omap8250_priv *priv = port->private_data;
+
+	int ret;
+
+	if (priv->wakeirq) {
+		ret = request_irq(priv->wakeirq, omap_wake_irq,
+				port->irqflags, "wakeup irq", port);
+		if (ret)
+			return ret;
+		disable_irq(priv->wakeirq);
+	}
+
+	ret = serial8250_do_startup(port);
+	if (ret)
+		goto err;
+
+	pm_runtime_get_sync(port->dev);
+
+	/* Enable module level wake up */
+	priv->wer = OMAP_UART_WER_MOD_WKUP;
+	if (priv->habit & OMAP_UART_WER_HAS_TX_WAKEUP)
+		priv->wer |= OMAP_UART_TX_WAKEUP_EN;
+	serial_out(up, UART_OMAP_WER, priv->wer);
+
+	pm_runtime_mark_last_busy(port->dev);
+	pm_runtime_put_autosuspend(port->dev);
+	return 0;
+err:
+	if (priv->wakeirq)
+		free_irq(priv->wakeirq, port);
+	return ret;
+}
+
+static void omap_8250_shutdown(struct uart_port *port)
+{
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
+	struct omap8250_priv *priv = port->private_data;
+
+	pm_runtime_get_sync(port->dev);
+
+	serial_out(up, UART_OMAP_WER, 0);
+	serial8250_do_shutdown(port);
+
+	pm_runtime_mark_last_busy(port->dev);
+	pm_runtime_put_autosuspend(port->dev);
+
+	if (priv->wakeirq)
+		free_irq(priv->wakeirq, port);
+}
+
+static void omap_8250_throttle(struct uart_port *port)
+{
+	unsigned long flags;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
+
+	pm_runtime_get_sync(port->dev);
+
+	spin_lock_irqsave(&port->lock, flags);
+	up->ier &= ~(UART_IER_RLSI | UART_IER_RDI);
+	serial_out(up, UART_IER, up->ier);
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	pm_runtime_mark_last_busy(port->dev);
+	pm_runtime_put_autosuspend(port->dev);
+}
+
+static void omap_8250_unthrottle(struct uart_port *port)
+{
+	unsigned long flags;
+	struct uart_8250_port *up =
+		container_of(port, struct uart_8250_port, port);
+
+	pm_runtime_get_sync(port->dev);
+
+	spin_lock_irqsave(&port->lock, flags);
+	up->ier |= UART_IER_RLSI | UART_IER_RDI;
+	serial_out(up, UART_IER, up->ier);
+	spin_unlock_irqrestore(&port->lock, flags);
+
+	pm_runtime_mark_last_busy(port->dev);
+	pm_runtime_put_autosuspend(port->dev);
+}
+
+static int omap8250_probe(struct platform_device *pdev)
+{
+	struct resource *regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	struct resource *irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	struct omap8250_priv *priv;
+	struct uart_8250_port up;
+	int ret;
+	void __iomem *membase;
+
+	if (!regs || !irq) {
+		dev_err(&pdev->dev, "missing registers or irq\n");
+		return -EINVAL;
+	}
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		dev_err(&pdev->dev, "unable to allocate private data\n");
+		return -ENOMEM;
+	}
+	membase = devm_ioremap_nocache(&pdev->dev, regs->start,
+			resource_size(regs));
+	if (!membase)
+		return -ENODEV;
+
+	memset(&up, 0, sizeof(up));
+	up.port.dev = &pdev->dev;
+	up.port.mapbase = regs->start;
+	up.port.membase = membase;
+	up.port.irq = irq->start;
+	/*
+	 * It claims to be 16C750 compatible however it is a little different.
+	 * It has EFR and has no FCR7_64byte bit. The AFE which it claims to
+	 * have is enabled via EFR instead of MCR.
+	 */
+	up.port.type = PORT_OMAP_16750;
+	up.port.iotype = UPIO_MEM32;
+	up.port.flags = UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | UPF_FIXED_TYPE |
+		UPF_SOFT_FLOW | UPF_HARD_FLOW;
+	up.port.private_data = priv;
+
+	up.port.regshift = 2;
+	up.port.fifosize = 64;
+
+	up.port.set_termios = omap_8250_set_termios;
+	up.port.pm = omap_8250_pm;
+	up.port.startup = omap_8250_startup;
+	up.port.shutdown = omap_8250_shutdown;
+	up.port.throttle = omap_8250_throttle;
+	up.port.unthrottle = omap_8250_unthrottle;
+
+	if (pdev->dev.of_node) {
+		up.port.line = of_alias_get_id(pdev->dev.of_node, "serial");
+		of_property_read_u32(pdev->dev.of_node, "clock-frequency",
+				&up.port.uartclk);
+		priv->wakeirq = irq_of_parse_and_map(pdev->dev.of_node, 1);
+	} else {
+		up.port.line = pdev->id;
+	}
+
+	if (up.port.line < 0) {
+		dev_err(&pdev->dev, "failed to get alias/pdev id, errno %d\n",
+				up.port.line);
+		return -ENODEV;
+	}
+	if (!up.port.uartclk) {
+		up.port.uartclk = DEFAULT_CLK_SPEED;
+		dev_warn(&pdev->dev,
+				"No clock speed specified: using default: %d\n",
+				DEFAULT_CLK_SPEED);
+	}
+
+	priv->latency = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE;
+	priv->calc_latency = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE;
+	pm_qos_add_request(&priv->pm_qos_request,
+			PM_QOS_CPU_DMA_LATENCY, priv->latency);
+	INIT_WORK(&priv->qos_work, omap8250_uart_qos_work);
+
+	device_init_wakeup(&pdev->dev, true);
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
+
+	pm_runtime_irq_safe(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
+	pm_runtime_get_sync(&pdev->dev);
+
+	omap_serial_fill_features_erratas(&up, priv);
+
+	ret = serial8250_register_8250_port(&up);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "unable to register 8250 port\n");
+		goto err;
+	}
+	priv->line = ret;
+	platform_set_drvdata(pdev, priv);
+	pm_runtime_mark_last_busy(&pdev->dev);
+	pm_runtime_put_autosuspend(&pdev->dev);
+	return 0;
+err:
+	pm_runtime_put(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	return ret;
+}
+
+static int omap8250_remove(struct platform_device *pdev)
+{
+	struct omap8250_priv *priv = platform_get_drvdata(pdev);
+
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	serial8250_unregister_port(priv->line);
+	pm_qos_remove_request(&priv->pm_qos_request);
+	device_init_wakeup(&pdev->dev, false);
+	return 0;
+}
+
+#if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM_RUNTIME)
+
+static inline void omap8250_enable_wakeirq(struct omap8250_priv *priv,
+		bool enable)
+{
+	if (!priv->wakeirq)
+		return;
+
+	if (enable)
+		enable_irq(priv->wakeirq);
+	else
+		disable_irq_nosync(priv->wakeirq);
+}
+
+static void omap8250_enable_wakeup(struct omap8250_priv *priv,
+		bool enable)
+{
+	if (enable == priv->wakeups_enabled)
+		return;
+
+	omap8250_enable_wakeirq(priv, enable);
+	priv->wakeups_enabled = enable;
+}
+#endif
+
+#ifdef CONFIG_PM_SLEEP
+static int omap8250_prepare(struct device *dev)
+{
+	struct omap8250_priv *priv = dev_get_drvdata(dev);
+
+	if (!priv)
+		return 0;
+	priv->is_suspending = true;
+	return 0;
+}
+
+static void omap8250_complete(struct device *dev)
+{
+	struct omap8250_priv *priv = dev_get_drvdata(dev);
+
+	if (!priv)
+		return;
+	priv->is_suspending = false;
+}
+
+static int omap8250_suspend(struct device *dev)
+{
+	struct omap8250_priv *priv = dev_get_drvdata(dev);
+
+	serial8250_suspend_port(priv->line);
+	flush_work(&priv->qos_work);
+
+	if (device_may_wakeup(dev))
+		omap8250_enable_wakeup(priv, true);
+	else
+		omap8250_enable_wakeup(priv, false);
+	return 0;
+}
+
+static int omap8250_resume(struct device *dev)
+{
+	struct omap8250_priv *priv = dev_get_drvdata(dev);
+
+	if (device_may_wakeup(dev))
+		omap8250_enable_wakeup(priv, false);
+
+	serial8250_resume_port(priv->line);
+	return 0;
+}
+#else
+#define omap8250_prepare NULL
+#define omap8250_complete NULL
+#endif
+
+#ifdef CONFIG_PM_RUNTIME
+static int omap8250_lost_context(struct omap8250_priv *priv)
+{
+	struct uart_8250_port *up;
+	u32 val;
+
+	up = serial8250_get_port(priv->line);
+	val = serial_in(up, UART_OMAP_MDR1);
+	/*
+	 * If we lose context, then MDR1 is set to its reset value which is
+	 * UART_OMAP_MDR1_DISABLE. After set_termios() we set it either to 13x
+	 * or 16x but never to disable again.
+	 */
+	if (val == UART_OMAP_MDR1_DISABLE)
+		return 1;
+	return 0;
+}
+
+static int omap8250_runtime_suspend(struct device *dev)
+{
+	struct omap8250_priv *priv = dev_get_drvdata(dev);
+
+	/*
+	 * When using 'no_console_suspend', the console UART must not be
+	 * suspended. Since driver suspend is managed by runtime suspend,
+	 * preventing runtime suspend (by returning error) will keep device
+	 * active during suspend.
+	 */
+	if (priv->is_suspending && !console_suspend_enabled) {
+		struct uart_8250_port *up;
+
+		up = serial8250_get_port(priv->line);
+		if (uart_console(&up->port))
+			return -EBUSY;
+	}
+
+	omap8250_enable_wakeup(priv, true);
+
+	priv->latency = PM_QOS_CPU_DMA_LAT_DEFAULT_VALUE;
+	schedule_work(&priv->qos_work);
+	return 0;
+}
+
+static void omap8250_restore_context(struct omap8250_priv *priv)
+{
+	struct uart_8250_port *up;
+
+	up = serial8250_get_port(priv->line);
+
+	if (priv->habit & UART_ERRATA_i202_MDR1_ACCESS)
+		omap_8250_mdr1_errataset(up, UART_OMAP_MDR1_DISABLE);
+	else
+		serial_out(up, UART_OMAP_MDR1, UART_OMAP_MDR1_DISABLE);
+
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B); /* Config B mode */
+	serial_out(up, UART_EFR, UART_EFR_ECB);
+	serial_out(up, UART_LCR, 0x0); /* Operational mode */
+	serial_out(up, UART_IER, 0x0);
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B); /* Config B mode */
+
+	serial_dl_write(up, priv->quot);
+
+	serial_out(up, UART_LCR, 0x0); /* Operational mode */
+	serial_out(up, UART_IER, up->ier);
+	serial_out(up, UART_FCR, priv->fcr);
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_A);
+	serial_out(up, UART_MCR, up->mcr);
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B); /* Config B mode */
+	serial_out(up, UART_OMAP_SCR, priv->scr);
+	serial_out(up, UART_EFR, priv->efr);
+	serial_out(up, UART_LCR, up->lcr);
+	if (priv->habit & UART_ERRATA_i202_MDR1_ACCESS)
+		omap_8250_mdr1_errataset(up, priv->mdr1);
+	else
+		serial_out(up, UART_OMAP_MDR1, priv->mdr1);
+	serial_out(up, UART_OMAP_WER, priv->wer);
+}
+
+static int omap8250_runtime_resume(struct device *dev)
+{
+	struct omap8250_priv *priv = dev_get_drvdata(dev);
+	int loss_cntx;
+
+	/* In case runtime-pm tries this before we are setup */
+	if (!priv)
+		return 0;
+	omap8250_enable_wakeup(priv, false);
+	loss_cntx = omap8250_lost_context(priv);
+
+	if (loss_cntx)
+		omap8250_restore_context(priv);
+
+	priv->latency = priv->calc_latency;
+	schedule_work(&priv->qos_work);
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops omap8250_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(omap8250_suspend, omap8250_resume)
+	SET_RUNTIME_PM_OPS(omap8250_runtime_suspend,
+			omap8250_runtime_resume, NULL)
+	.prepare        = omap8250_prepare,
+	.complete       = omap8250_complete,
+};
+
+static const struct of_device_id omap8250_dt_ids[] = {
+	{ .compatible = "ti,omap2-uart" },
+	{ .compatible = "ti,omap3-uart" },
+	{ .compatible = "ti,omap4-uart" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, omap8250_dt_ids);
+
+static struct platform_driver omap8250_platform_driver = {
+	.driver = {
+		.name		= "omap8250",
+		.pm		= &omap8250_dev_pm_ops,
+		.of_match_table = omap8250_dt_ids,
+		.owner		= THIS_MODULE,
+	},
+	.probe			= omap8250_probe,
+	.remove			= omap8250_remove,
+};
+module_platform_driver(omap8250_platform_driver);
+
+MODULE_AUTHOR("Sebastian Andrzej Siewior");
+MODULE_DESCRIPTION("OMAP 8250 Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 349ee59..7a5073b 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -298,3 +298,12 @@ config SERIAL_8250_RT288X
 	  If you have a Ralink RT288x/RT305x SoC based board and want to use the
 	  serial port, say Y to this option. The driver can handle up to 2 serial
 	  ports. If unsure, say N.
+
+config SERIAL_8250_OMAP
+	tristate "Support for OMAP internal UART (8250 based driver)"
+	depends on SERIAL_8250 && ARCH_OMAP2PLUS
+	help
+	  If you have a machine based on an Texas Instruments OMAP CPU you
+	  can enable its onboard serial ports by enabling this option.
+
+	  This driver is in early stage and uses ttyS instead of ttyO.
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index 36d68d0..4bac392 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -20,3 +20,4 @@ obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
 obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
 obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o
 obj-$(CONFIG_SERIAL_8250_EM)		+= 8250_em.o
+obj-$(CONFIG_SERIAL_8250_OMAP)		+= 8250_omap.o
diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
index 5820269..74f9b11 100644
--- a/include/uapi/linux/serial_core.h
+++ b/include/uapi/linux/serial_core.h
@@ -54,7 +54,8 @@
 #define PORT_ALTR_16550_F32 26	/* Altera 16550 UART with 32 FIFOs */
 #define PORT_ALTR_16550_F64 27	/* Altera 16550 UART with 64 FIFOs */
 #define PORT_ALTR_16550_F128 28 /* Altera 16550 UART with 128 FIFOs */
-#define PORT_MAX_8250	28	/* max port ID */
+#define PORT_OMAP_16750	29	/* TI's OMAP internal 16C750 compatible UART */
+#define PORT_MAX_8250	29	/* max port ID */
 
 /*
  * ARM specific type numbers.  These are not currently guaranteed
-- 
2.0.1

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

[Tony Lindgren]

* Sebastian Andrzej Siewior <bigeasy@linutronix.de> [140716 07:47]:
> This patch provides a 8250-core based UART driver for the internal OMAP
> UART. The longterm goal is to provide the same functionality as the
> current OMAP uart driver and hopefully DMA support which could borrowed
> from the 8250-core.
> 
> It has been only tested as console UART on am335x-evm and dra7-evm.
> The device name is ttyS based instead of ttyO. If a ttyO based node name
> is required please ask udev for it. If both driver are activated (this
> and omap-serial) then this serial driver will take control over the
> device due to the link order
> 
> v3…v4:
> 	- drop RS485 support
> 	- wire up ->throttle / ->unthrottle
> v2…v3:
> 	- wire up startup & shutdown for wakeup-irq handling.
> 	- RS485 handling (well the core does).
> 
> v1…v2:
> 	- added runtime PM. Could somebody could plese double check
> 	  this?
> 	- added omap_8250_set_termios()

Seems to boot a bit further now with output from serial console
initially, then I'm getting the following error again that's probably
related to clocks not enabled when the registers are accessed:

[    1.050231] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
[    1.068664] 4806a000.serial: ttyS0 at MMIO 0x4806a000 (irq = 88, base_baud = 3000000) is a OMAP
[    1.074676] 4806c000.serial: ttyS1 at MMIO 0x4806c000 (irq = 89, base_baud = 3000000) is a OMAP
[    1.078613] console [ttyS2] disabled
[    1.079193] 49020000.serial: ttyS2 at MMIO 0x49020000 (irq = 90, base_baud = 3000000) is a OMAP
[    1.938110] console [ttyS2] enabled
[    1.946533] 49042000.serial: ttyS3 at MMIO 0x49042000 (irq = 96, base_baud = 3000000) is a OMAP
...
[    5.538421] Unhandled fault: external abort on non-linefetch (0x1028) at 0xfb020000
[    5.546142] Internal error: : 1028 [#1] SMP ARM
[    5.550720] Modules linked in:
[    5.553802] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.16.0-rc5-00070-g1f95851 #129
[    5.561584] task: ce058b00 ti: ce05a000 task.ti: ce05a000
[    5.567016] PC is at mem32_serial_in+0xc/0x1c
[    5.571411] LR is at serial8250_do_startup+0xc8/0x89c
[    5.576507] pc : [<c034731c>]    lr : [<c034a988>]    psr: 60000113
[    5.576507] sp : ce05bcf0  ip : c0a008e8  fp : ce46c400
[    5.588043] r10: 00000000  r9 : cda7a680  r8 : ce46c68c
[    5.593292] r7 : ce46c400  r6 : 00000000  r5 : ce548e10  r4 : c10abf34
[    5.599853] r3 : fb020000  r2 : 00000002  r1 : fb020000  r0 : c10abf34
[    5.606414] Flags: nZCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
[    5.613769] Control: 10c5387d  Table: 80004019  DAC: 00000015
[    5.619537] Process swapper/0 (pid: 1, stack limit = 0xce05a248)
...
[    5.835601] [<c034731c>] (mem32_serial_in) from [<c034a988>] (serial8250_do_startup+0xc8/0x89c)
[    5.844360] [<c034a988>] (serial8250_do_startup) from [<c034c58c>] (omap_8250_startup+0x5c/0xdc)
[    5.853179] [<c034c58c>] (omap_8250_startup) from [<c034b170>] (serial8250_startup+0x14/0x20)
[    5.861755] [<c034b170>] (serial8250_startup) from [<c0346584>] (uart_startup.part.3+0x7c/0x1dc)
[    5.870605] [<c0346584>] (uart_startup.part.3) from [<c03470f4>] (uart_open+0xe4/0x124)
[    5.878662] [<c03470f4>] (uart_open) from [<c032c528>] (tty_open+0x130/0x58c)
[    5.885864] [<c032c528>] (tty_open) from [<c01216ec>] (chrdev_open+0x9c/0x174)
[    5.893127] [<c01216ec>] (chrdev_open) from [<c011b8cc>] (do_dentry_open+0x1d0/0x310)
[    5.901000] [<c011b8cc>] (do_dentry_open) from [<c011bd90>] (finish_open+0x34/0x4c)
[    5.908721] [<c011bd90>] (finish_open) from [<c012a8f0>] (do_last.isra.27+0x5a4/0xb98)
[    5.916687] [<c012a8f0>] (do_last.isra.27) from [<c012af98>] (path_openat+0xb4/0x5e4)
[    5.924560] [<c012af98>] (path_openat) from [<c012b7d4>] (do_filp_open+0x2c/0x80)
[    5.932098] [<c012b7d4>] (do_filp_open) from [<c011cd0c>] (do_sys_open+0x100/0x1d0)
[    5.939788] [<c011cd0c>] (do_sys_open) from [<c07b6ce0>] (kernel_init_freeable+0x124/0x1c8)
[    5.948211] [<c07b6ce0>] (kernel_init_freeable) from [<c054ed28>] (kernel_init+0x8/0xe4)
[    5.956359] [<c054ed28>] (kernel_init) from [<c000e868>] (ret_from_fork+0x14/0x2c)
...
[    5.974792] In-band Error seen by MPU  at address 0
[    5.979675] ------------[ cut here ]------------
[    5.984344] WARNING: CPU: 0 PID: 1 at drivers/bus/omap_l3_smx.c:162 omap3_l3_app_irq+0xcc/0x124()
...

Regards,

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

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

[Sebastian Andrzej Siewior]

* Tony Lindgren | 2014-07-17 00:09:00 [-0700]:

>Seems to boot a bit further now with output from serial console
>initially, then I'm getting the following error again that's probably
>related to clocks not enabled when the registers are accessed:

It is (mostly) the same thing as before. We have additionally 
omap_8250_startup() in the backtrace but it is the same thing.
So you  say I miss a clock? Looking through serial8250_do_startup() I see:
- pm_runtime_get_sync(port->dev); which should get the clocks up
- serial8250_clear_fifos() which does a write at address + 8. Seems to 
  work. 
- serial_port_in(port, UART_LSR); does a read at address + 0x14, seems 
  to work.
- serial_port_in(port, UART_RX); does a read at address + 0. This is 
  probably the bad one.

Now comparing with omap-serial I noticed that I do a 32bit access
instead a 16bit.
Could you please try the following hack:

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 2e4a93b..94af5a3 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -420,13 +420,13 @@ static void mem_serial_out(struct uart_port *p, int offset, int value)
 static void mem32_serial_out(struct uart_port *p, int offset, int value)
 {
 	offset = offset << p->regshift;
-	writel(value, p->membase + offset);
+	writew(value, p->membase + offset);
 }
 
 static unsigned int mem32_serial_in(struct uart_port *p, int offset)
 {
 	offset = offset << p->regshift;
-	return readl(p->membase + offset);
+	return readw(p->membase + offset);
 }
 
 static unsigned int io_serial_in(struct uart_port *p, int offset)


besides that, I don't see what could be different.

>Regards,
>
>Tony

Sebastian

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

[Tony Lindgren]

* Sebastian Andrzej Siewior <bigeasy@linutronix.de> [140717 00:44]:
> * Tony Lindgren | 2014-07-17 00:09:00 [-0700]:
> 
> >Seems to boot a bit further now with output from serial console
> >initially, then I'm getting the following error again that's probably
> >related to clocks not enabled when the registers are accessed:
> 
> It is (mostly) the same thing as before. We have additionally 
> omap_8250_startup() in the backtrace but it is the same thing.
> So you  say I miss a clock? Looking through serial8250_do_startup() I see:
> - pm_runtime_get_sync(port->dev); which should get the clocks up
> - serial8250_clear_fifos() which does a write at address + 8. Seems to 
>   work. 
> - serial_port_in(port, UART_LSR); does a read at address + 0x14, seems 
>   to work.
> - serial_port_in(port, UART_RX); does a read at address + 0. This is 
>   probably the bad one.

Hmm it could be that it works for a while because the clocks are on
from the bootloader and pm_runtime calls won't do anything. This
could happen if the interconnect data based on the ti,hwmods entry
is not getting matched to the new driver. This gets initialized when
the device entry gets created in omap_device_build_from_dt().

Or maybe something now affects the clock aliases? It seems that we
are still missing the clocks entries in the .dtsi files, see the
mappings with $ git grep uart drivers/clk/ti/
 
> Now comparing with omap-serial I noticed that I do a 32bit access
> instead a 16bit.
> Could you please try the following hack:

No change with that :) 
 
Regards,

Tony

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

[Sebastian Andrzej Siewior]

On 07/17/2014 10:12 AM, Tony Lindgren wrote:
> Hmm it could be that it works for a while because the clocks are on
> from the bootloader and pm_runtime calls won't do anything. This
> could happen if the interconnect data based on the ti,hwmods entry
> is not getting matched to the new driver. This gets initialized when
> the device entry gets created in omap_device_build_from_dt().
> 
> Or maybe something now affects the clock aliases? It seems that we
> are still missing the clocks entries in the .dtsi files, see the
> mappings with $ git grep uart drivers/clk/ti/

I've been looking for something completely different while I noticed
this:

in drivers/tty/serial/omap-serial.c
| static struct platform_driver serial_omap_driver = {
|         .driver         = {
|                 .name   = DRIVER_NAME,
|         },
| };
|

and DRIVER_NAME should come from include/linux/platform_data/serial-omap.h
Looking further, I've found arch/arm/mach-omap2/serial.c:
| void __init omap_serial_init_port(struct omap_board_data *bdata,
|                         struct omap_uart_port_info *info)
| {
|         char *name
…
|	name = DRIVER_NAME;
…
|	pdev = omap_device_build(name, uart->num, oh, pdata, pdata_size);
…
|

Would this explain it?

> Regards,
> 
> Tony

Sebastian

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

[Tony Lindgren]

* Sebastian Andrzej Siewior <bigeasy@linutronix.de> [140717 03:09]:
> On 07/17/2014 10:12 AM, Tony Lindgren wrote:
> > Hmm it could be that it works for a while because the clocks are on
> > from the bootloader and pm_runtime calls won't do anything. This
> > could happen if the interconnect data based on the ti,hwmods entry
> > is not getting matched to the new driver. This gets initialized when
> > the device entry gets created in omap_device_build_from_dt().
> > 
> > Or maybe something now affects the clock aliases? It seems that we
> > are still missing the clocks entries in the .dtsi files, see the
> > mappings with $ git grep uart drivers/clk/ti/
> 
> I've been looking for something completely different while I noticed
> this:
> 
> in drivers/tty/serial/omap-serial.c
> | static struct platform_driver serial_omap_driver = {
> |         .driver         = {
> |                 .name   = DRIVER_NAME,
> |         },
> | };
> |
> 
> and DRIVER_NAME should come from include/linux/platform_data/serial-omap.h
> Looking further, I've found arch/arm/mach-omap2/serial.c:
> | void __init omap_serial_init_port(struct omap_board_data *bdata,
> |                         struct omap_uart_port_info *info)
> | {
> |         char *name
> …
> |	name = DRIVER_NAME;
> …
> |	pdev = omap_device_build(name, uart->num, oh, pdata, pdata_size);
> …
> |
> 
> Would this explain it?

That would explain it for legacy booting, but not for device tree
based booting. I can try to debug it further on Monday.

Regards,

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

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

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [140717 23:28]:
> * Sebastian Andrzej Siewior <bigeasy@linutronix.de> [140717 03:09]:
> > On 07/17/2014 10:12 AM, Tony Lindgren wrote:
> > > Hmm it could be that it works for a while because the clocks are on
> > > from the bootloader and pm_runtime calls won't do anything. This
> > > could happen if the interconnect data based on the ti,hwmods entry
> > > is not getting matched to the new driver. This gets initialized when
> > > the device entry gets created in omap_device_build_from_dt().
> > > 
> > > Or maybe something now affects the clock aliases? It seems that we
> > > are still missing the clocks entries in the .dtsi files, see the
> > > mappings with $ git grep uart drivers/clk/ti/
> > 
> > I've been looking for something completely different while I noticed
> > this:
> > 
> > in drivers/tty/serial/omap-serial.c
> > | static struct platform_driver serial_omap_driver = {
> > |         .driver         = {
> > |                 .name   = DRIVER_NAME,
> > |         },
> > | };
> > |
> > 
> > and DRIVER_NAME should come from include/linux/platform_data/serial-omap.h
> > Looking further, I've found arch/arm/mach-omap2/serial.c:
> > | void __init omap_serial_init_port(struct omap_board_data *bdata,
> > |                         struct omap_uart_port_info *info)
> > | {
> > |         char *name
> > …
> > |	name = DRIVER_NAME;
> > …
> > |	pdev = omap_device_build(name, uart->num, oh, pdata, pdata_size);
> > …
> > |
> > 
> > Would this explain it?
> 
> That would explain it for legacy booting, but not for device tree
> based booting. I can try to debug it further on Monday.

Looks like the following is needed to avoid the error, no idea why..
But this is what we also do in omap-serial.c. It may be needed in
other places too?

Also, no luck yet on runtime PM. If the console is enabled omap won't
idle, and if no serial console is enabled, it just hangs. Will try
to debug this part further, it may be also related to the above.

Regards,

Tony

--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -2066,8 +2066,8 @@ int serial8250_do_startup(struct uart_port *port)
 	/*
 	 * Clear the interrupt registers.
 	 */
-	serial_port_in(port, UART_LSR);
-	serial_port_in(port, UART_RX);
+	if (serial_port_in(port, UART_LSR) & UART_LSR_DR)
+		serial_port_in(port, UART_RX);
 	serial_port_in(port, UART_IIR);
 	serial_port_in(port, UART_MSR);
 
@@ -2228,8 +2228,8 @@ dont_test_tx_en:
 	 * saved flags to avoid getting false values from polling
 	 * routines or the previous session.
 	 */
-	serial_port_in(port, UART_LSR);
-	serial_port_in(port, UART_RX);
+	if (serial_port_in(port, UART_LSR) & UART_LSR_DR)
+		serial_port_in(port, UART_RX);
 	serial_port_in(port, UART_IIR);
 	serial_port_in(port, UART_MSR);
 	up->lsr_saved_flags = 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Sebastian Andrzej Siewior]

On 07/21/2014 11:35 AM, Tony Lindgren wrote:

> Looks like the following is needed to avoid the error, no idea why..
> But this is what we also do in omap-serial.c. It may be needed in
> other places too?

Yes, there was a third one on shutdown.

> Also, no luck yet on runtime PM. If the console is enabled omap won't
> idle, and if no serial console is enabled, it just hangs. Will try
> to debug this part further, it may be also related to the above.

My beaglebone xm won't do idle:

| omap3_pm_off_mode_enable: Core OFF disabled due to errata i583

but it would if it could, right? That means that I can't test the wake
via UART on this one?

So I will try to fix the tx-pm, fold patches and then prepare another
batch.
And I could verify that DMA works on that omap3 :)

> Regards,
> 
> Tony

Sebastian

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

[Tony Lindgren]

* Sebastian Andrzej Siewior <bigeasy@linutronix.de> [140813 09:23]:
> On 07/21/2014 11:35 AM, Tony Lindgren wrote:
> 
> > Looks like the following is needed to avoid the error, no idea why..
> > But this is what we also do in omap-serial.c. It may be needed in
> > other places too?
> 
> Yes, there was a third one on shutdown.

OK
 
> > Also, no luck yet on runtime PM. If the console is enabled omap won't
> > idle, and if no serial console is enabled, it just hangs. Will try
> > to debug this part further, it may be also related to the above.
> 
> My beaglebone xm won't do idle:
> 
> | omap3_pm_off_mode_enable: Core OFF disabled due to errata i583
> 
> but it would if it could, right? That means that I can't test the wake
> via UART on this one?

Hmm OK yes that's disabled for some earlier ES revisions. I guess you
could comment out the limitations for PM_SDRC_WAKEUP_ERRATUM_i583
in both cpuidle34xx.c and pm34xx.c and see if off-idle is usable for
your tests :) It might be flakey but still usable for testing.
 
> So I will try to fix the tx-pm, fold patches and then prepare another
> batch.

OK great, let me know if you need more help.

> And I could verify that DMA works on that omap3 :)

OK

Regards,

Tony

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

Hi,

On Wed, Jul 16, 2014 at 04:45:03PM +0200, Sebastian Andrzej Siewior wrote:
> +static int omap_8250_startup(struct uart_port *port)
> +{
> +	struct uart_8250_port *up =
> +		container_of(port, struct uart_8250_port, port);
> +	struct omap8250_priv *priv = port->private_data;
> +
> +	int ret;
> +
> +	if (priv->wakeirq) {
> +		ret = request_irq(priv->wakeirq, omap_wake_irq,
> +				port->irqflags, "wakeup irq", port);
> +		if (ret)
> +			return ret;
> +		disable_irq(priv->wakeirq);
> +	}
> +
> +	ret = serial8250_do_startup(port);
> +	if (ret)
> +		goto err;
> +
> +	pm_runtime_get_sync(port->dev);

should this pm_runtime_get_sync() be placed above
serial8250_do_startup() call ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Sebastian Andrzej Siewior]

On 07/17/2014 04:54 PM, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Jul 16, 2014 at 04:45:03PM +0200, Sebastian Andrzej Siewior wrote:
>> +static int omap_8250_startup(struct uart_port *port)
>> +{
>> +	struct uart_8250_port *up =
>> +		container_of(port, struct uart_8250_port, port);
>> +	struct omap8250_priv *priv = port->private_data;
>> +
>> +	int ret;
>> +
>> +	if (priv->wakeirq) {
>> +		ret = request_irq(priv->wakeirq, omap_wake_irq,
>> +				port->irqflags, "wakeup irq", port);
>> +		if (ret)
>> +			return ret;
>> +		disable_irq(priv->wakeirq);
>> +	}
>> +
>> +	ret = serial8250_do_startup(port);
>> +	if (ret)
>> +		goto err;
>> +
>> +	pm_runtime_get_sync(port->dev);
> 
> should this pm_runtime_get_sync() be placed above
> serial8250_do_startup() call ?

I don't think it matters since serial8250_do_startup() has its own
pm_runtime_get_sync().

->startup(), ->shutdown() are called via omap callbacks so we could
spare in the 8250-core if we do it in the omap code before invoking the
function. The same goes for serial8250_set_termios() which is not used
by omap but has those runtime-pm stuff, too.
It would be wrong if someone would use the serial8250_do_startup()
without his own runtime-pm get but it is omap only which does this
things.
So it is not used by anyone else (right now) and if you want to keep it
to a minimum I could remove them from those places.

Sebastian

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1679 bytes --]

On Thu, Jul 17, 2014 at 05:11:58PM +0200, Sebastian Andrzej Siewior wrote:
> On 07/17/2014 04:54 PM, Felipe Balbi wrote:
> > Hi,
> > 
> > On Wed, Jul 16, 2014 at 04:45:03PM +0200, Sebastian Andrzej Siewior wrote:
> >> +static int omap_8250_startup(struct uart_port *port)
> >> +{
> >> +	struct uart_8250_port *up =
> >> +		container_of(port, struct uart_8250_port, port);
> >> +	struct omap8250_priv *priv = port->private_data;
> >> +
> >> +	int ret;
> >> +
> >> +	if (priv->wakeirq) {
> >> +		ret = request_irq(priv->wakeirq, omap_wake_irq,
> >> +				port->irqflags, "wakeup irq", port);
> >> +		if (ret)
> >> +			return ret;
> >> +		disable_irq(priv->wakeirq);
> >> +	}
> >> +
> >> +	ret = serial8250_do_startup(port);
> >> +	if (ret)
> >> +		goto err;
> >> +
> >> +	pm_runtime_get_sync(port->dev);
> > 
> > should this pm_runtime_get_sync() be placed above
> > serial8250_do_startup() call ?
> 
> I don't think it matters since serial8250_do_startup() has its own
> pm_runtime_get_sync().

oh right, saw that now.

> ->startup(), ->shutdown() are called via omap callbacks so we could
> spare in the 8250-core if we do it in the omap code before invoking the
> function. The same goes for serial8250_set_termios() which is not used
> by omap but has those runtime-pm stuff, too.
> It would be wrong if someone would use the serial8250_do_startup()
> without his own runtime-pm get but it is omap only which does this
> things.
> So it is not used by anyone else (right now) and if you want to keep it
> to a minimum I could remove them from those places.

I don't think it matters as long as the calls are balanced.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]