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

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

[Hector Palacios]

This fixes a bug whereby the AUART was shutdown while bytes 
were still waiting in the DMA buffer to be transmistted.

Changes since v1:
 * Split the patch in two: one to set the fifosize to the 
   DMA buffer size, and another to wait for the FIFO to empty
   before disabling the AUART.
 * The wait for the fifo to empty is now independent of DMA
   being enabled.
 * Fix bug on readl() call
 * Remove bad URL in commit log

Hector Palacios (2):
  serial: mxs-auart: set the FIFO size to DMA buffer size
  serial: mxs-auart: wait for FIFO to flush before shutdown

 drivers/tty/serial/mxs-auart.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

-- 
1.8.4

[PATCH v2 1/2] serial: mxs-auart: set the FIFO size to DMA buffer size

[Hector Palacios]

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 <hector.palacios@digi.com>
---
 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;
-- 
1.8.4

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

[Hector Palacios]

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 <hector.palacios@digi.com>
---
 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 (auart_dma_enabled(s))
 		mxs_auart_dma_exit(s);
-- 
1.8.4

Re: [PATCH v2 2/2] serial: mxs-auart: wait for FIFO to flush before shutdown

[Uwe Kleine-König]

Hello Hector,

On Wed, Oct 02, 2013 at 02:02:44PM +0200, Hector Palacios wrote:
> 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 <hector.palacios@digi.com>
> ---
>  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 ...

Best regards
Uwe

>  	if (auart_dma_enabled(s))
>  		mxs_auart_dma_exit(s);
> -- 
> 1.8.4
> 
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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 v2 1/2] serial: mxs-auart: set the FIFO size to DMA buffer size

[Uwe Kleine-König]

Hallo Hector,

On Wed, Oct 02, 2013 at 02:02:43PM +0200, Hector Palacios wrote:
> 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 <hector.palacios@digi.com>
> ---
>  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;
I don't know if something is surprised when fifosize is changed by
set_termios, but that's how it is. So:

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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 v2 0/2] serial: mxs-auart: wait for DMA buffer to flush before shutdown

[Marek Vasut]

Dear Hector Palacios,

> This fixes a bug whereby the AUART was shutdown while bytes
> were still waiting in the DMA buffer to be transmistted.
> 
> Changes since v1:
>  * Split the patch in two: one to set the fifosize to the
>    DMA buffer size, and another to wait for the FIFO to empty
>    before disabling the AUART.
>  * The wait for the fifo to empty is now independent of DMA
>    being enabled.
>  * Fix bug on readl() call
>  * Remove bad URL in commit log
> 
> Hector Palacios (2):
>   serial: mxs-auart: set the FIFO size to DMA buffer size
>   serial: mxs-auart: wait for FIFO to flush before shutdown
> 
>  drivers/tty/serial/mxs-auart.c | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)

Looks OK

Reviewed-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

Re: [PATCH v2 2/2] serial: mxs-auart: wait for FIFO to flush before shutdown

[Hector Palacios]

Hello Uwe,

On 10/02/2013 02:22 PM, Uwe Kleine-König wrote:
> Hello Hector,
>
> On Wed, Oct 02, 2013 at 02:02:44PM +0200, Hector Palacios wrote:
>> 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 <hector.palacios@digi.com>
>> ---
>>   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;
+
+	return 0;
  }

  static void mxs_auart_start_tx(struct uart_port *u)
  {
  	struct mxs_auart_port *s = to_auart_port(u);


Do you agree?

Best regards,
--
Hector Palacios
--
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 v2 2/2] serial: mxs-auart: wait for FIFO to flush before shutdown

[Uwe Kleine-König]

Hello Hector,

On Wed, Oct 02, 2013 at 04:01:47PM +0200, Hector Palacios wrote:
> On 10/02/2013 02:22 PM, Uwe Kleine-König wrote:
> >On Wed, Oct 02, 2013 at 02:02:44PM +0200, Hector Palacios wrote:
> >>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 <hector.palacios@digi.com>
> >>---
> >>  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);
There is no need for a forward declaration if you move the whole
function.

> +
>  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;
if you do
	stat = readl(u->membase + AUART_STAT);
	if ((stat & (AUART_STAT_BUSY | AUART_STAT_TXFE)) == AUART_STAT_TXFE)
		return TIOCSER_TEMT;

you save one register read and a small race condition.

Best regards
Uwe


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
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