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

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

[Uwe Kleine-König]

Hello,

On Wed, Oct 02, 2013 at 09:31:16AM +0200, Hector Palacios wrote:
> On 10/01/2013 09:48 PM, Uwe Kleine-K?nig wrote:
> >On Tue, Oct 01, 2013 at 06:18:35PM +0200, Hector Palacios wrote:
> >>The shutdown function was not waiting for the DMA buffer 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.
> >>
> >>Signed-off-by: Hector Palacios <hector.palacios@digi.com>
> >>
> >>[...]
> >>---
> >>  drivers/tty/serial/mxs-auart.c | 16 +++++++++++++++-
> >>  1 file changed, 15 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> >>index f85b8e6..0d8b2ca 100644
> >>--- a/drivers/tty/serial/mxs-auart.c
> >>+++ b/drivers/tty/serial/mxs-auart.c
> >>@@ -750,12 +750,26 @@ 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;
> >>+
> >>+	if (auart_dma_enabled(s)) {
> >>+		/* Wait enough time to flush DMA buffer completely */
> >>+		to = u->timeout * UART_XMIT_SIZE / u->fifosize;
> >u->timeout is the time needed to send one char, right? UART_XMIT_SIZE is
> >the size of the circular buffer, fifosize is the size of the fifo. I
> >don't get what you get by dividing by the fifosize. I would have
> >expected something like:
> >
> >	u->timeout * min(UART_XMIT_SIZE, u->fifosize)
> >
> >Can you explain, maybe in a comment along the code for the next person
> >not understanding?
> 
> u->timeout is *not* the time needed to send one char but the time
> needed to flush the complete port fifo (see uart_update_timeout() in
> serial_core.c where it is set).
Ah, right. (BTW, uart_update_timeout should better round up instead of
round down, i.e.
-	port->timeout = (HZ * bits) / baud + HZ/50;
+	port->timeout = DIV_ROUND_UP(HZ * bits, baud) + HZ/50;
)

> UART_XMIT_SIZE is the number of bytes in the DMA buffer, so I divide
> the max number of bytes in the FIFO by the FIFO size and multiply by
> the FIFO timeout.
ditto here, better round up. Although I think using a completion could
shorten the timeout considerably because there are often less than
UART_XMIT_SIZE chars in the buffer?!

> >I wonder if the individual driver is the right place this must be fixed
> >in or if there should be (or even exists) some help form the serial
> >framework?!
> 
> Good question, the issue should exit in other DMA drivers as well.
Added Jiri and lakml to Cc:. Maybe someone can say something about that
topic?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Hector Palacios]

Hello Uwe,

On 10/02/2013 09:44 AM, Uwe Kleine-K?nig wrote:
> Hello,
>
> On Wed, Oct 02, 2013 at 09:31:16AM +0200, Hector Palacios wrote:
>> On 10/01/2013 09:48 PM, Uwe Kleine-K?nig wrote:
>>> On Tue, Oct 01, 2013 at 06:18:35PM +0200, Hector Palacios wrote:
>>>> The shutdown function was not waiting for the DMA buffer 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.
>>>>
>>>> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
>>>>
>>>> [...]
>>>> ---
>>>>   drivers/tty/serial/mxs-auart.c | 16 +++++++++++++++-
>>>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
>>>> index f85b8e6..0d8b2ca 100644
>>>> --- a/drivers/tty/serial/mxs-auart.c
>>>> +++ b/drivers/tty/serial/mxs-auart.c
>>>> @@ -750,12 +750,26 @@ 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;
>>>> +
>>>> +	if (auart_dma_enabled(s)) {
>>>> +		/* Wait enough time to flush DMA buffer completely */
>>>> +		to = u->timeout * UART_XMIT_SIZE / u->fifosize;
>>> u->timeout is the time needed to send one char, right? UART_XMIT_SIZE is
>>> the size of the circular buffer, fifosize is the size of the fifo. I
>>> don't get what you get by dividing by the fifosize. I would have
>>> expected something like:
>>>
>>> 	u->timeout * min(UART_XMIT_SIZE, u->fifosize)
>>>
>>> Can you explain, maybe in a comment along the code for the next person
>>> not understanding?
>>
>> u->timeout is *not* the time needed to send one char but the time
>> needed to flush the complete port fifo (see uart_update_timeout() in
>> serial_core.c where it is set).
> Ah, right. (BTW, uart_update_timeout should better round up instead of
> round down, i.e.
> -	port->timeout = (HZ * bits) / baud + HZ/50;
> +	port->timeout = DIV_ROUND_UP(HZ * bits, baud) + HZ/50;
> )
>
>> UART_XMIT_SIZE is the number of bytes in the DMA buffer, so I divide
>> the max number of bytes in the FIFO by the FIFO size and multiply by
>> the FIFO timeout.
> ditto here, better round up. Although I think using a completion could
> shorten the timeout considerably because there are often less than
> UART_XMIT_SIZE chars in the buffer?!

I'm not really waiting the full timeout. I'm checking for TX fifo empty every 1ms and 
for a maximum of 'to' ms.


>>> I wonder if the individual driver is the right place this must be fixed
>>> in or if there should be (or even exists) some help form the serial
>>> framework?!
>>
>> Good question, the issue should exit in other DMA drivers as well.
> Added Jiri and lakml to Cc:. Maybe someone can say something about that
> topic?
>
> Best regards
> Uwe
>


Best regards,
--
Hector Palacios

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

[Uwe Kleine-König]

Hello Hector,

On Wed, Oct 02, 2013 at 10:09:10AM +0200, Hector Palacios wrote:
> On 10/02/2013 09:44 AM, Uwe Kleine-K?nig wrote:
> >On Wed, Oct 02, 2013 at 09:31:16AM +0200, Hector Palacios wrote:
> >>On 10/01/2013 09:48 PM, Uwe Kleine-K?nig wrote:
> >>>On Tue, Oct 01, 2013 at 06:18:35PM +0200, Hector Palacios wrote:
> >>>>+		/* Wait enough time to flush DMA buffer completely */
> >>>>+		to = u->timeout * UART_XMIT_SIZE / u->fifosize;
> >>>u->timeout is the time needed to send one char, right? UART_XMIT_SIZE is
> >>>the size of the circular buffer, fifosize is the size of the fifo. I
> >>>don't get what you get by dividing by the fifosize. I would have
> >>>expected something like:
> >>>
> >>>	u->timeout * min(UART_XMIT_SIZE, u->fifosize)
> >>>
> >>>Can you explain, maybe in a comment along the code for the next person
> >>>not understanding?
> >>
> >>u->timeout is *not* the time needed to send one char but the time
> >>needed to flush the complete port fifo (see uart_update_timeout() in
> >>serial_core.c where it is set).
> >Ah, right. (BTW, uart_update_timeout should better round up instead of
> >round down, i.e.
> >-	port->timeout = (HZ * bits) / baud + HZ/50;
> >+	port->timeout = DIV_ROUND_UP(HZ * bits, baud) + HZ/50;
> >)
> >
> >>UART_XMIT_SIZE is the number of bytes in the DMA buffer, so I divide
> >>the max number of bytes in the FIFO by the FIFO size and multiply by
> >>the FIFO timeout.
> >ditto here, better round up. Although I think using a completion could
> >shorten the timeout considerably because there are often less than
> >UART_XMIT_SIZE chars in the buffer?!
> 
> I'm not really waiting the full timeout. I'm checking for TX fifo
> empty every 1ms and for a maximum of 'to' ms.
Correct. Still using DIV_ROUND_UP would be better, right?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Hector Palacios]

Hi Uwe,

On 10/02/2013 10:30 AM, Uwe Kleine-K?nig wrote:
> Hello Hector,
>
> On Wed, Oct 02, 2013 at 10:09:10AM +0200, Hector Palacios wrote:
>> On 10/02/2013 09:44 AM, Uwe Kleine-K?nig wrote:
>>> On Wed, Oct 02, 2013 at 09:31:16AM +0200, Hector Palacios wrote:
>>>> On 10/01/2013 09:48 PM, Uwe Kleine-K?nig wrote:
>>>>> On Tue, Oct 01, 2013 at 06:18:35PM +0200, Hector Palacios wrote:
>>>>>> +		/* Wait enough time to flush DMA buffer completely */
>>>>>> +		to = u->timeout * UART_XMIT_SIZE / u->fifosize;
>>>>> u->timeout is the time needed to send one char, right? UART_XMIT_SIZE is
>>>>> the size of the circular buffer, fifosize is the size of the fifo. I
>>>>> don't get what you get by dividing by the fifosize. I would have
>>>>> expected something like:
>>>>>
>>>>> 	u->timeout * min(UART_XMIT_SIZE, u->fifosize)
>>>>>
>>>>> Can you explain, maybe in a comment along the code for the next person
>>>>> not understanding?
>>>>
>>>> u->timeout is *not* the time needed to send one char but the time
>>>> needed to flush the complete port fifo (see uart_update_timeout() in
>>>> serial_core.c where it is set).
>>> Ah, right. (BTW, uart_update_timeout should better round up instead of
>>> round down, i.e.
>>> -	port->timeout = (HZ * bits) / baud + HZ/50;
>>> +	port->timeout = DIV_ROUND_UP(HZ * bits, baud) + HZ/50;
>>> )
>>>
>>>> UART_XMIT_SIZE is the number of bytes in the DMA buffer, so I divide
>>>> the max number of bytes in the FIFO by the FIFO size and multiply by
>>>> the FIFO timeout.
>>> ditto here, better round up. Although I think using a completion could
>>> shorten the timeout considerably because there are often less than
>>> UART_XMIT_SIZE chars in the buffer?!
>>
>> I'm not really waiting the full timeout. I'm checking for TX fifo
>> empty every 1ms and for a maximum of 'to' ms.
> Correct. Still using DIV_ROUND_UP would be better, right?

I think it is not really needed, after all the code is adding HZ/50 of slop which acts 
like a round up, doesn't it?

	/*
	 * Figure the timeout to send the above number of bits.
	 * Add .02 seconds of slop
	 */
	port->timeout = (HZ * bits) / baud + HZ/50;

Best regards,
--
Hector Palacios

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

[Uwe Kleine-König]

Hi Hector,

On Wed, Oct 02, 2013 at 10:46:28AM +0200, Hector Palacios wrote:
> On 10/02/2013 10:30 AM, Uwe Kleine-K?nig wrote:
> >On Wed, Oct 02, 2013 at 10:09:10AM +0200, Hector Palacios wrote:
> >>On 10/02/2013 09:44 AM, Uwe Kleine-K?nig wrote:
> >>>Ah, right. (BTW, uart_update_timeout should better round up instead of
> >>>round down, i.e.
> >>>-	port->timeout = (HZ * bits) / baud + HZ/50;
> >>>+	port->timeout = DIV_ROUND_UP(HZ * bits, baud) + HZ/50;
> >>>)
> >>>
> >>>>UART_XMIT_SIZE is the number of bytes in the DMA buffer, so I divide
> >>>>the max number of bytes in the FIFO by the FIFO size and multiply by
> >>>>the FIFO timeout.
> >>>ditto here, better round up. Although I think using a completion could
> >>>shorten the timeout considerably because there are often less than
> >>>UART_XMIT_SIZE chars in the buffer?!
> >>
> >>I'm not really waiting the full timeout. I'm checking for TX fifo
> >>empty every 1ms and for a maximum of 'to' ms.
> >Correct. Still using DIV_ROUND_UP would be better, right?
> 
> I think it is not really needed, after all the code is adding HZ/50
> of slop which acts like a round up, doesn't it?
> 
> 	/*
> 	 * Figure the timeout to send the above number of bits.
> 	 * Add .02 seconds of slop
> 	 */
> 	port->timeout = (HZ * bits) / baud + HZ/50;
According to my understanding (which could well be wrong) the slop of
HZ/50 isn't there to fix rounding issues in the previous division. But
yes, if it's calculated generously there is no technical need to fix the
rounding issue.

Still it might be worth to fix it to be a better reference. I don't care
much.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

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

[Hector Palacios]

On 10/02/2013 10:36 AM, Uwe Kleine-K?nig wrote:
> Hello Hector,
>
> On Wed, Oct 02, 2013 at 09:31:16AM +0200, Hector Palacios wrote:
>> On 10/01/2013 09:48 PM, Uwe Kleine-K?nig wrote:
>>> Hello Hector,
>>>
>>> On Tue, Oct 01, 2013 at 06:18:35PM +0200, Hector Palacios wrote:
>>>> The shutdown function was not waiting for the DMA buffer 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.
>>>>
>>>> Signed-off-by: Hector Palacios <hector.palacios@digi.com>
>>>> ---
>>>>   drivers/tty/serial/mxs-auart.c | 16 +++++++++++++++-
>>>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
>>>> index f85b8e6..0d8b2ca 100644
>>>> --- a/drivers/tty/serial/mxs-auart.c
>>>> +++ b/drivers/tty/serial/mxs-auart.c
>>>> @@ -750,12 +750,26 @@ 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;
>>>> +
>>>> +	if (auart_dma_enabled(s)) {
>>>> +		/* Wait enough time to flush DMA buffer completely */
>>>> +		to = u->timeout * UART_XMIT_SIZE / u->fifosize;
>>> u->timeout is the time needed to send one char, right? UART_XMIT_SIZE is
>>> the size of the circular buffer, fifosize is the size of the fifo. I
>>> don't get what you get by dividing by the fifosize. I would have
>>> expected something like:
>>>
>>> 	u->timeout * min(UART_XMIT_SIZE, u->fifosize)
>>>
>>> Can you explain, maybe in a comment along the code for the next person
>>> not understanding?
>>
>> u->timeout is *not* the time needed to send one char but the time
>> needed to flush the complete port fifo (see uart_update_timeout() in
>> serial_core.c where it is set).
>> UART_XMIT_SIZE is the number of bytes in the DMA buffer, so I divide
>> the max number of bytes in the FIFO by the FIFO size and multiply by
>> the FIFO timeout.
> Another thought: Does that mean that from the framework's POV the
> fifosize is UART_XMIT_SIZE instead in the DMA case? That would enable
> you to just use u->timeout.

Hum, I think you hit the point.
I just saw they do this in amba_pl011.c driver:

	/* The DMA buffer is now the FIFO the TTY subsystem can use */
	uap->port.fifosize = PL011_DMA_BUFFER_SIZE;

I'll rework the patch and try.

Best regards,
--
Hector Palacios