[PATCH v2 0/2] serial: mxs-auart: wait for DMA buffer to flush before shutdown

When DMA is enabled (with hardware flow control enabled) the FIFO size
must be set to the size of the DMA buffer, as this is the size the tty
subsystem can use.

Signed-off-by: Hector Palacios <redacted>
---
 drivers/tty/serial/mxs-auart.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index f85b8e6..9f046177 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c

@@ -40,6 +40,7 @@
 #include <asm/cacheflush.h>
 
 #define MXS_AUART_PORTS 5
+#define MXS_AUART_FIFO_SIZE		16
 
 #define AUART_CTRL0			0x00000000
 #define AUART_CTRL0_SET			0x00000004

@@ -549,6 +550,9 @@ static int mxs_auart_dma_init(struct mxs_auart_port *s)
 	s->flags |= MXS_AUART_DMA_ENABLED;
 	dev_dbg(s->dev, "enabled the DMA support.");
 
+	/* The DMA buffer is now the FIFO the TTY subsystem can use */
+	s->port.fifosize = UART_XMIT_SIZE;
+
 	return 0;
 
 err_out:

@@ -741,6 +745,9 @@ static int mxs_auart_startup(struct uart_port *u)
 	writel(AUART_INTR_RXIEN | AUART_INTR_RTIEN | AUART_INTR_CTSMIEN,
 			u->membase + AUART_INTR);
 
+	/* Reset FIFO size (it could have changed if DMA was enabled) */
+	u->fifosize = MXS_AUART_FIFO_SIZE;
+
 	/*
 	 * Enable fifo so all four bytes of a DMA word are written to
 	 * output (otherwise, only the LSB is written, ie. 1 in 4 bytes)

@@ -1062,7 +1069,7 @@ static int mxs_auart_probe(struct platform_device *pdev)
 	s->port.membase = ioremap(r->start, resource_size(r));
 	s->port.ops = &mxs_auart_ops;
 	s->port.iotype = UPIO_MEM;
-	s->port.fifosize = 16;
+	s->port.fifosize = MXS_AUART_FIFO_SIZE;
 	s->port.uartclk = clk_get_rate(s->clk);
 	s->port.type = PORT_IMX;
 	s->port.dev = s->dev = &pdev->dev;

The shutdown function was not waiting for the FIFO (which may be the
real 16 byte FIFO or the DMA buffer, if DMA is enabled) to flush
before disabling the AUART.
This lead to many bytes not being transferred (specially at low
baudrates), as they were still in the DMA buffer when the AUART was
shutdown.
This patch also adds the check for the BUSY flag before disabling
the AUART.

Signed-off-by: Hector Palacios <redacted>
---
 drivers/tty/serial/mxs-auart.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 9f046177..589b595 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c

@@ -757,9 +757,22 @@ static int mxs_auart_startup(struct uart_port *u)
 	return 0;
 }
 
+static unsigned int mxs_auart_tx_empty(struct uart_port *u);
+
 static void mxs_auart_shutdown(struct uart_port *u)
 {
 	struct mxs_auart_port *s = to_auart_port(u);
+	unsigned int to;
+
+	/* Wait for the FIFO to flush */
+	to = u->timeout;
+	while (!mxs_auart_tx_empty(u) && to--)
+		mdelay(1);
+
+	/* Wait for UART to become idle ... */
+	to = u->timeout;
+	while ((readl(u->membase + AUART_STAT) & AUART_STAT_BUSY) && to--)
+		mdelay(1);

On 10/02/2013 02:22 PM, Uwe Kleine-K?nig wrote:

quoted

On Wed, Oct 02, 2013 at 02:02:44PM +0200, Hector Palacios wrote:

quoted

The shutdown function was not waiting for the FIFO (which may be the
real 16 byte FIFO or the DMA buffer, if DMA is enabled) to flush
before disabling the AUART.
This lead to many bytes not being transferred (specially at low
baudrates), as they were still in the DMA buffer when the AUART was
shutdown.
This patch also adds the check for the BUSY flag before disabling
the AUART.

Signed-off-by: Hector Palacios <redacted>
---
 drivers/tty/serial/mxs-auart.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 9f046177..589b595 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c

@@ -757,9 +757,22 @@ static int mxs_auart_startup(struct uart_port *u)
 	return 0;
 }

+static unsigned int mxs_auart_tx_empty(struct uart_port *u);
+
 static void mxs_auart_shutdown(struct uart_port *u)
 {
 	struct mxs_auart_port *s = to_auart_port(u);
+	unsigned int to;
+
+	/* Wait for the FIFO to flush */
+	to = u->timeout;
+	while (!mxs_auart_tx_empty(u) && to--)
+		mdelay(1);
+
+	/* Wait for UART to become idle ... */
+	to = u->timeout;
+	while ((readl(u->membase + AUART_STAT) & AUART_STAT_BUSY) && to--)
+		mdelay(1);

If the 2nd loop is needed the tx_empty callback is buggy. According to
Documentation/serial/driver tx_empty "tests whether the transmitter fifo
and shifter for the port [...] is empty". I guess it only tests for the
fifo part? Time for another fix ...

Do you mean the tx_empty should check both for TX_FIFO and !BUSY, right?
I did it so because I thought the BUSY was also set during rx, but
it is really only used to signal tx operation.

That would turn this patch into this:

@@ -755,13 +755,21 @@ static int mxs_auart_startup(struct uart_port *u)
 	writel(AUART_LINECTRL_FEN, u->membase + AUART_LINECTRL_SET);

 	return 0;
 }

+static unsigned int mxs_auart_tx_empty(struct uart_port *u);

+
 static void mxs_auart_shutdown(struct uart_port *u)
 {
 	struct mxs_auart_port *s = to_auart_port(u);
+	unsigned int to;
+
+	/* Wait for the FIFO to flush */
+	to = u->timeout;
+	while (!mxs_auart_tx_empty(u) && to--)
+		mdelay(1);

 	if (auart_dma_enabled(s))
 		mxs_auart_dma_exit(s);

 	writel(AUART_CTRL2_UARTEN, u->membase + AUART_CTRL2_CLR);

@@ -774,14 +782,15 @@ static void mxs_auart_shutdown(struct uart_port *u)
 	clk_disable_unprepare(s->clk);
 }

 static unsigned int mxs_auart_tx_empty(struct uart_port *u)
 {
-	if (readl(u->membase + AUART_STAT) & AUART_STAT_TXFE)
-		return TIOCSER_TEMT;
-	else
-		return 0;
+	if (!((readl(u->membase + AUART_STAT) & AUART_STAT_BUSY)))
+		if (readl(u->membase + AUART_STAT) & AUART_STAT_TXFE)
+			return TIOCSER_TEMT;