[PATCH 0/2] Remove pl011 startup glitches and avoid dummy TX during startup

[PATCH 0/2] Remove pl011 startup glitches and avoid dummy TX during startup

[Dave Martin]

This is a major rework of a previously-posted patch [1].

This addresses a number of overlapping issues:

 * interaction of the dummy TX in pl011_startup() with hardware that
   doesn't suppress transmission when in loopback mode, causing
   glitches / corruption at the receiver;

 * lack of support for loopback mode in the SBSA Generic UART;

 * issues with the models/simulators that pretend to have an
   unreasonably high baud rate (making it impossible to fill the
   FIFO in some situations).


The general approach in this series is to remove the dummy TX and
start in a timer-based polled IO mode instead.  If and when a TX IRQ
occurs, we switch to interrupt-driven mode for future processing.

If the data written to the UART is a small enough trickle, we may
never using the TX IRQ, but the UART would not stream constantly in
any case unless there is enough data being written to stop the FIFO
ever emptying.


I've kept the second patch separate in case someone can think of a
reason why it might not work...

There is some context overlap with Jorge's probe deferral patch, but
the merge is straightforward.


Tested on ARM Juno and the ARM fast models only for now.

Any review/testing from other folks would be appreciated.

Cheers
Dave


[1] [RFC PATCH] serial: amba-pl011: Kickstart TX by explicit FIFO fill
    http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/315248.html

Dave Martin (2):
  serial/amba-pl011: Activate TX IRQ passively
  serial/amba-pl011: Leave the TX IRQ alone when the UART is not open

 drivers/tty/serial/amba-pl011.c |  164 +++++++++++++++++++++++++++------------
 1 file changed, 113 insertions(+), 51 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] serial/amba-pl011: Activate TX IRQ passively

[Dave Martin]

The current PL011 driver transmits a dummy character when the UART
is opened, to assert the TX IRQ for the first time
(see pl011_startup()).  The UART is put in loopback mode temporarily,
so the receiver presumably shouldn't see anything.

However...

At least some platforms containing a PL011 send characters down the
wire even when loopback mode is enabled.  This means that a
spurious NUL character may be seen at the receiver when the PL011 is
opened through the TTY layer.

The current code also temporarily sets the baud rate to maximum and
the character width to the minimum, to that the dummy TX completes
as quickly as possible.  If this is seen by the receiver it will
result in a framing error and can knock the receiver out of sync --
turning subsequent output into garbage until synchronisation
is reestablished.  (Particularly problematic during boot with systemd.)

To avoid spurious transmissions, this patch removes assumptions about
whether the TX IRQ will fire until at least one TX IRQ has been seen.

Instead, the UART will unmask the TX IRQ and then slow-start via
polling and timer-based soft IRQs initially.  If the TTY layer writes
enough data to fill the FIFO to the interrupt threshold in one go,
the TX IRQ should assert, at which point the driver changes to
fully interrupt-driven TX.

In this way, the TX IRQ is activated as a side-effect instead of
being done deliberately.

This should also mean that the driver works on the SBSA Generic
UART[1] (a cut-down PL011) without invasive changes.  The Generic
UART lacks some features needed for the dummy TX approach to work
(FIFO disabling and loopback).

[1] Server Base System Architecture (ARM-DEN-0029-v2.3)
    http://infocenter.arm.com/
    (click-thru required :/)

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 drivers/tty/serial/amba-pl011.c |  165 +++++++++++++++++++++++++++------------
 1 file changed, 115 insertions(+), 50 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 8d94c19..69910f7 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -58,6 +58,7 @@
 #include <linux/pinctrl/consumer.h>
 #include <linux/sizes.h>
 #include <linux/io.h>
+#include <linux/workqueue.h>
 
 #define UART_NR			14
 
@@ -156,7 +157,9 @@ struct uart_amba_port {
 	unsigned int		lcrh_tx;	/* vendor-specific */
 	unsigned int		lcrh_rx;	/* vendor-specific */
 	unsigned int		old_cr;		/* state during shutdown */
+	struct delayed_work	tx_softirq_work;
 	bool			autorts;
+	unsigned int		tx_irq_seen;	/* 0=none, 1=1, 2=2 or more */
 	char			type[12];
 #ifdef CONFIG_DMA_ENGINE
 	/* DMA stuff */
@@ -440,8 +443,9 @@ static void pl011_dma_remove(struct uart_amba_port *uap)
 		dma_release_channel(uap->dmarx.chan);
 }
 
-/* Forward declare this for the refill routine */
+/* Forward declare these for the refill routine */
 static int pl011_dma_tx_refill(struct uart_amba_port *uap);
+static void pl011_start_tx_pio(struct uart_amba_port *uap);
 
 /*
  * The current DMA TX buffer has been sent.
@@ -479,14 +483,13 @@ static void pl011_dma_tx_callback(void *data)
 		return;
 	}
 
-	if (pl011_dma_tx_refill(uap) <= 0) {
+	if (pl011_dma_tx_refill(uap) <= 0)
 		/*
 		 * We didn't queue a DMA buffer for some reason, but we
 		 * have data pending to be sent.  Re-enable the TX IRQ.
 		 */
-		uap->im |= UART011_TXIM;
-		writew(uap->im, uap->port.membase + UART011_IMSC);
-	}
+		pl011_start_tx_pio(uap);
+
 	spin_unlock_irqrestore(&uap->port.lock, flags);
 }
 
@@ -664,12 +667,10 @@ static inline bool pl011_dma_tx_start(struct uart_amba_port *uap)
 		if (!uap->dmatx.queued) {
 			if (pl011_dma_tx_refill(uap) > 0) {
 				uap->im &= ~UART011_TXIM;
-				ret = true;
-			} else {
-				uap->im |= UART011_TXIM;
+				writew(uap->im, uap->port.membase +
+				       UART011_IMSC);
+			} else
 				ret = false;
-			}
-			writew(uap->im, uap->port.membase + UART011_IMSC);
 		} else if (!(uap->dmacr & UART011_TXDMAE)) {
 			uap->dmacr |= UART011_TXDMAE;
 			writew(uap->dmacr,
@@ -1208,15 +1209,24 @@ static void pl011_stop_tx(struct uart_port *port)
 	pl011_dma_tx_stop(uap);
 }
 
+static bool pl011_tx_chars(struct uart_amba_port *uap);
+
+/* Start TX with programmed I/O only (no DMA) */
+static void pl011_start_tx_pio(struct uart_amba_port *uap)
+{
+	uap->im |= UART011_TXIM;
+	writew(uap->im, uap->port.membase + UART011_IMSC);
+	if (!uap->tx_irq_seen)
+		pl011_tx_chars(uap);
+}
+
 static void pl011_start_tx(struct uart_port *port)
 {
 	struct uart_amba_port *uap =
 	    container_of(port, struct uart_amba_port, port);
 
-	if (!pl011_dma_tx_start(uap)) {
-		uap->im |= UART011_TXIM;
-		writew(uap->im, uap->port.membase + UART011_IMSC);
-	}
+	if (!pl011_dma_tx_start(uap))
+		pl011_start_tx_pio(uap);
 }
 
 static void pl011_stop_rx(struct uart_port *port)
@@ -1274,40 +1284,87 @@ __acquires(&uap->port.lock)
 	spin_lock(&uap->port.lock);
 }
 
-static void pl011_tx_chars(struct uart_amba_port *uap)
+/*
+ * Transmit a character
+ * There must be at least one free entry in the TX FIFO to accept the char.
+ *
+ * Returns true if the FIFO might have space in it afterwards;
+ * returns false if the FIFO definitely became full.
+ */
+static bool pl011_tx_char(struct uart_amba_port *uap, unsigned char c)
+{
+	writew(c, uap->port.membase + UART01x_DR);
+	uap->port.icount.tx++;
+
+	if (likely(uap->tx_irq_seen > 1))
+		return true;
+
+	return !(readw(uap->port.membase + UART01x_FR) & UART01x_FR_TXFF);
+}
+
+static bool pl011_tx_chars(struct uart_amba_port *uap)
 {
 	struct circ_buf *xmit = &uap->port.state->xmit;
 	int count;
 
+	if (unlikely(uap->tx_irq_seen < 2))
+		/*
+		 * Initial FIFO fill level unknown: we must check TXFF
+		 * after each write, so just try to fill up the FIFO.
+		 */
+		count = uap->fifosize;
+	else /* tx_irq_seen >= 2 */
+		/*
+		 * FIFO initially at least half-empty, so we can simply
+		 * write half the FIFO without polling TXFF.
+
+		 * Note: the *first* TX IRQ can still race with
+		 * pl011_start_tx_pio(), which can result in the FIFO
+		 * being fuller than expected in that case.
+		 */
+		count = uap->fifosize >> 1;
+
+	/*
+	 * If the FIFO is full we're guaranteed a TX IRQ at some later point,
+	 * and can't transmit immediately in any case:
+	 */
+	if (unlikely(uap->tx_irq_seen < 2 &&
+		     readw(uap->port.membase + UART01x_FR) & UART01x_FR_TXFF))
+		return false;
+
 	if (uap->port.x_char) {
-		writew(uap->port.x_char, uap->port.membase + UART01x_DR);
-		uap->port.icount.tx++;
+		pl011_tx_char(uap, uap->port.x_char);
 		uap->port.x_char = 0;
-		return;
+		--count;
 	}
 	if (uart_circ_empty(xmit) || uart_tx_stopped(&uap->port)) {
 		pl011_stop_tx(&uap->port);
-		return;
+		goto done;
 	}
 
 	/* If we are using DMA mode, try to send some characters. */
 	if (pl011_dma_tx_irq(uap))
-		return;
+		goto done;
 
-	count = uap->fifosize >> 1;
-	do {
-		writew(xmit->buf[xmit->tail], uap->port.membase + UART01x_DR);
+	while (count-- > 0 && pl011_tx_char(uap, xmit->buf[xmit->tail])) {
 		xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
-		uap->port.icount.tx++;
 		if (uart_circ_empty(xmit))
 			break;
-	} while (--count > 0);
+	}
 
 	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
 		uart_write_wakeup(&uap->port);
 
-	if (uart_circ_empty(xmit))
+	if (uart_circ_empty(xmit)) {
 		pl011_stop_tx(&uap->port);
+		goto done;
+	}
+
+	if (unlikely(!uap->tx_irq_seen))
+		schedule_delayed_work(&uap->tx_softirq_work, uap->port.timeout);
+
+done:
+	return false;
 }
 
 static void pl011_modem_status(struct uart_amba_port *uap)
@@ -1334,6 +1391,28 @@ static void pl011_modem_status(struct uart_amba_port *uap)
 	wake_up_interruptible(&uap->port.state->port.delta_msr_wait);
 }
 
+static void pl011_tx_softirq(struct work_struct *work)
+{
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct uart_amba_port *uap =
+		container_of(dwork, struct uart_amba_port, tx_softirq_work);
+
+	spin_lock(&uap->port.lock);
+	while (pl011_tx_chars(uap)) ;
+	spin_unlock(&uap->port.lock);
+}
+
+static void pl011_tx_irq_seen(struct uart_amba_port *uap)
+{
+	if (likely(uap->tx_irq_seen > 1))
+		return;
+
+	uap->tx_irq_seen++;
+	if (uap->tx_irq_seen < 2)
+		/* first TX IRQ */
+		cancel_delayed_work(&uap->tx_softirq_work);
+}
+
 static irqreturn_t pl011_int(int irq, void *dev_id)
 {
 	struct uart_amba_port *uap = dev_id;
@@ -1372,8 +1451,10 @@ static irqreturn_t pl011_int(int irq, void *dev_id)
 			if (status & (UART011_DSRMIS|UART011_DCDMIS|
 				      UART011_CTSMIS|UART011_RIMIS))
 				pl011_modem_status(uap);
-			if (status & UART011_TXIS)
+			if (status & UART011_TXIS) {
+				pl011_tx_irq_seen(uap);
 				pl011_tx_chars(uap);
+			}
 
 			if (pass_counter-- == 0)
 				break;
@@ -1577,7 +1658,7 @@ static int pl011_startup(struct uart_port *port)
 {
 	struct uart_amba_port *uap =
 	    container_of(port, struct uart_amba_port, port);
-	unsigned int cr, lcr_h, fbrd, ibrd;
+	unsigned int cr;
 	int retval;
 
 	retval = pl011_hwinit(port);
@@ -1595,29 +1676,10 @@ static int pl011_startup(struct uart_port *port)
 
 	writew(uap->vendor->ifls, uap->port.membase + UART011_IFLS);
 
-	/*
-	 * Provoke TX FIFO interrupt into asserting. Taking care to preserve
-	 * baud rate and data format specified by FBRD, IBRD and LCRH as the
-	 * UART may already be in use as a console.
-	 */
-	spin_lock_irq(&uap->port.lock);
-
-	fbrd = readw(uap->port.membase + UART011_FBRD);
-	ibrd = readw(uap->port.membase + UART011_IBRD);
-	lcr_h = readw(uap->port.membase + uap->lcrh_rx);
-
-	cr = UART01x_CR_UARTEN | UART011_CR_TXE | UART011_CR_LBE;
-	writew(cr, uap->port.membase + UART011_CR);
-	writew(0, uap->port.membase + UART011_FBRD);
-	writew(1, uap->port.membase + UART011_IBRD);
-	pl011_write_lcr_h(uap, 0);
-	writew(0, uap->port.membase + UART01x_DR);
-	while (readw(uap->port.membase + UART01x_FR) & UART01x_FR_BUSY)
-		barrier();
+	/* Assume that TX IRQ doesn't work until we see one: */
+	uap->tx_irq_seen = 0;
 
-	writew(fbrd, uap->port.membase + UART011_FBRD);
-	writew(ibrd, uap->port.membase + UART011_IBRD);
-	pl011_write_lcr_h(uap, lcr_h);
+	spin_lock_irq(&uap->port.lock);
 
 	/* restore RTS and DTR */
 	cr = uap->old_cr & (UART011_CR_RTS | UART011_CR_DTR);
@@ -1672,6 +1734,8 @@ static void pl011_shutdown(struct uart_port *port)
 	    container_of(port, struct uart_amba_port, port);
 	unsigned int cr;
 
+	cancel_delayed_work_sync(&uap->tx_softirq_work);
+
 	/*
 	 * disable all interrupts
 	 */
@@ -2218,6 +2282,7 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
 	uap->port.ops = &amba_pl011_pops;
 	uap->port.flags = UPF_BOOT_AUTOCONF;
 	uap->port.line = i;
+	INIT_DELAYED_WORK(&uap->tx_softirq_work, pl011_tx_softirq);
 	pl011_dma_probe(&dev->dev, uap);
 
 	/* Ensure interrupts from this UART are masked and cleared */
-- 
1.7.10.4

[PATCH 2/2] serial/amba-pl011: Leave the TX IRQ alone when the UART is not open

[Dave Martin]

Getting the TX IRQ re-asserted from scratch can be inefficient in
some setups.

This patch avoids clearing the TX IRQ across pl011_shutdown()...
pl011_startup(), so that if the port is closed and reopened, the
IRQ will still work afterwards without having to bootstrap it again.

The TX IRQ continues to be masked in IMSC when the UART is not in
use.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 drivers/tty/serial/amba-pl011.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 69910f7..92783fc 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -1676,9 +1676,6 @@ static int pl011_startup(struct uart_port *port)
 
 	writew(uap->vendor->ifls, uap->port.membase + UART011_IFLS);
 
-	/* Assume that TX IRQ doesn't work until we see one: */
-	uap->tx_irq_seen = 0;
-
 	spin_lock_irq(&uap->port.lock);
 
 	/* restore RTS and DTR */
@@ -1742,7 +1739,7 @@ static void pl011_shutdown(struct uart_port *port)
 	spin_lock_irq(&uap->port.lock);
 	uap->im = 0;
 	writew(uap->im, uap->port.membase + UART011_IMSC);
-	writew(0xffff, uap->port.membase + UART011_ICR);
+	writew(0xffff & ~UART011_TXIS, uap->port.membase + UART011_ICR);
 	spin_unlock_irq(&uap->port.lock);
 
 	pl011_dma_shutdown(uap);
-- 
1.7.10.4