[PATCH] serial imx.c: fix CTS trigger level lower to avoid lost chars

[PATCH] serial imx.c: fix CTS trigger level lower to avoid lost chars

[Valentin Longchamp]

The imx CTS trigger level is left at its reset value that is 32
chars. Since the RX FIFO has 32 entries, when CTS is raised, the
FIFO already is full. However, some serial port devices first empty
their TX FIFO before stopping when CTS is raised, resulting in lost
chars.

This patch sets the trigger level lower so that other chars arrive
after CTS is raised, there is still room for 16 of them.

Signed-off-by: Valentin Longchamp <valentin.longchamp@epfl.ch>
Tested-by: Philippe R?tornaz <philippe.retornaz@epfl.ch>
---
 drivers/serial/imx.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/drivers/serial/imx.c b/drivers/serial/imx.c
index 18130f1..f7d2da5 100644
--- a/drivers/serial/imx.c
+++ b/drivers/serial/imx.c
@@ -119,7 +119,8 @@
 #define  MX2_UCR3_RXDMUXSEL	 (1<<2)  /* RXD Muxed Input Select, on mx2/mx3 */
 #define  UCR3_INVT  	 (1<<1)  /* Inverted Infrared transmission */
 #define  UCR3_BPEN  	 (1<<0)  /* Preset registers enable */
-#define  UCR4_CTSTL_32   (32<<10) /* CTS trigger level (32 chars) */
+#define  UCR4_CTSTL_SHF  10      /* CTS trigger level shift */
+#define  UCR4_CTSTL_MASK 0x3F    /* CTS trigger is 6 bits wide */
 #define  UCR4_INVR  	 (1<<9)  /* Inverted infrared reception */
 #define  UCR4_ENIRI 	 (1<<8)  /* Serial infrared interrupt enable */
 #define  UCR4_WKEN  	 (1<<7)  /* Wake interrupt enable */
@@ -590,6 +591,8 @@ static int imx_setup_ufcr(struct imx_port *sport, unsigned int mode)
 	return 0;
 }
 
+#define CTSTL 16
+
 static int imx_startup(struct uart_port *port)
 {
 	struct imx_port *sport = (struct imx_port *)port;
@@ -606,6 +609,11 @@ static int imx_startup(struct uart_port *port)
 	if (USE_IRDA(sport))
 		temp |= UCR4_IRSC;
 
+	/* set the trigger level for CTS to 16 by default
+	 */
+	temp &= ~(UCR4_CTSTL_MASK << UCR4_CTSTL_SHF);
+	temp |= CTSTL << UCR4_CTSTL_SHF;
+
 	writel(temp & ~UCR4_DREN, sport->port.membase + UCR4);
 
 	if (USE_IRDA(sport)) {
-- 
1.6.3.3

[PATCH] serial imx.c: fix CTS trigger level lower to avoid lost chars

[Wolfram Sang]

CCing linux-serial

On Thu, Jan 21, 2010 at 10:26:03PM +0100, Valentin Longchamp wrote:
> The imx CTS trigger level is left at its reset value that is 32
> chars. Since the RX FIFO has 32 entries, when CTS is raised, the
> FIFO already is full. However, some serial port devices first empty
> their TX FIFO before stopping when CTS is raised, resulting in lost
> chars.

? Isn't that a flaw of the other side? Have you spotted other serial drivers
doing the same as your patch?

> 
> This patch sets the trigger level lower so that other chars arrive
> after CTS is raised, there is still room for 16 of them.
> 
> Signed-off-by: Valentin Longchamp <valentin.longchamp@epfl.ch>
> Tested-by: Philippe R?tornaz <philippe.retornaz@epfl.ch>
> ---
>  drivers/serial/imx.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/serial/imx.c b/drivers/serial/imx.c
> index 18130f1..f7d2da5 100644
> --- a/drivers/serial/imx.c
> +++ b/drivers/serial/imx.c
> @@ -119,7 +119,8 @@
>  #define  MX2_UCR3_RXDMUXSEL	 (1<<2)  /* RXD Muxed Input Select, on mx2/mx3 */
>  #define  UCR3_INVT  	 (1<<1)  /* Inverted Infrared transmission */
>  #define  UCR3_BPEN  	 (1<<0)  /* Preset registers enable */
> -#define  UCR4_CTSTL_32   (32<<10) /* CTS trigger level (32 chars) */
> +#define  UCR4_CTSTL_SHF  10      /* CTS trigger level shift */
> +#define  UCR4_CTSTL_MASK 0x3F    /* CTS trigger is 6 bits wide */
>  #define  UCR4_INVR  	 (1<<9)  /* Inverted infrared reception */
>  #define  UCR4_ENIRI 	 (1<<8)  /* Serial infrared interrupt enable */
>  #define  UCR4_WKEN  	 (1<<7)  /* Wake interrupt enable */
> @@ -590,6 +591,8 @@ static int imx_setup_ufcr(struct imx_port *sport, unsigned int mode)
>  	return 0;
>  }
>  
> +#define CTSTL 16
> +
>  static int imx_startup(struct uart_port *port)
>  {
>  	struct imx_port *sport = (struct imx_port *)port;
> @@ -606,6 +609,11 @@ static int imx_startup(struct uart_port *port)
>  	if (USE_IRDA(sport))
>  		temp |= UCR4_IRSC;
>  
> +	/* set the trigger level for CTS to 16 by default
> +	 */
> +	temp &= ~(UCR4_CTSTL_MASK << UCR4_CTSTL_SHF);
> +	temp |= CTSTL << UCR4_CTSTL_SHF;
> +
>  	writel(temp & ~UCR4_DREN, sport->port.membase + UCR4);
>  
>  	if (USE_IRDA(sport)) {
> -- 
> 1.6.3.3
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100121/c5fcc27e/attachment.sig>

[PATCH] serial imx.c: fix CTS trigger level lower to avoid lost chars

[Russell King - ARM Linux]

On Thu, Jan 21, 2010 at 11:06:53PM +0100, Wolfram Sang wrote:
> CCing linux-serial
> 
> On Thu, Jan 21, 2010 at 10:26:03PM +0100, Valentin Longchamp wrote:
> > The imx CTS trigger level is left at its reset value that is 32
> > chars. Since the RX FIFO has 32 entries, when CTS is raised, the
> > FIFO already is full. However, some serial port devices first empty
> > their TX FIFO before stopping when CTS is raised, resulting in lost
> > chars.
> 
> ? Isn't that a flaw of the other side? Have you spotted other serial drivers
> doing the same as your patch?

Arguably, but it's common behaviour of 16550A's and similar.

[PATCH] serial imx.c: fix CTS trigger level lower to avoid lost chars

[Valentin Longchamp]

Russell King - ARM Linux wrote:
> On Thu, Jan 21, 2010 at 11:06:53PM +0100, Wolfram Sang wrote:
>> CCing linux-serial
>>
>> On Thu, Jan 21, 2010 at 10:26:03PM +0100, Valentin Longchamp wrote:
>>> The imx CTS trigger level is left at its reset value that is 32
>>> chars. Since the RX FIFO has 32 entries, when CTS is raised, the
>>> FIFO already is full. However, some serial port devices first empty
>>> their TX FIFO before stopping when CTS is raised, resulting in lost
>>> chars.
>> ? Isn't that a flaw of the other side? Have you spotted other serial drivers
>> doing the same as your patch?
> 
> Arguably, but it's common behaviour of 16550A's and similar.

First I wanted to fix the fact that the imx serial hardware only raises 
the CTS pin when the oferflowing char begins to be set. I would have 
solved this by setting the CTS trigger value to 31 instead of 32. But 
when I talked with one colleague he told me about 16550A (that is 
present nearly everywhere) that empty their TX FIFO, so I lowered it to 16.

Val

-- 
Valentin Longchamp, PhD Student, EPFL-STI-LSRO1
valentin.longchamp at epfl.ch, Phone: +41216937827
http://people.epfl.ch/valentin.longchamp
MEB3494, Station 9, CH-1015 Lausanne

[PATCH] serial imx.c: fix CTS trigger level lower to avoid lost chars

[Jamie Lokier]

Valentin Longchamp wrote:
> First I wanted to fix the fact that the imx serial hardware only raises 
> the CTS pin when the oferflowing char begins to be set. I would have 
> solved this by setting the CTS trigger value to 31 instead of 32. But 
> when I talked with one colleague he told me about 16550A (that is 
> present nearly everywhere) that empty their TX FIFO, so I lowered it to 16.

That seems quite peculiar because a 16550A only has a 16 byte receive
buffer, doesn't it?  And therefore it would have trouble talking
reliably to another 16550A if it worked like that.  I wonder what I'm
not seeing in this picture.

-- Jamie

[PATCH] serial imx.c: fix CTS trigger level lower to avoid lost chars

[Russell King - ARM Linux]

On Fri, Jan 22, 2010 at 11:07:01AM +0000, Jamie Lokier wrote:
> Valentin Longchamp wrote:
> > First I wanted to fix the fact that the imx serial hardware only raises 
> > the CTS pin when the oferflowing char begins to be set. I would have 
> > solved this by setting the CTS trigger value to 31 instead of 32. But 
> > when I talked with one colleague he told me about 16550A (that is 
> > present nearly everywhere) that empty their TX FIFO, so I lowered it to 16.
> 
> That seems quite peculiar because a 16550A only has a 16 byte receive
> buffer, doesn't it?  And therefore it would have trouble talking
> reliably to another 16550A if it worked like that.  I wonder what I'm
> not seeing in this picture.

What you're assuming is that flow control was there to prevent overruns
on the hardware receiver.  That's not the way it works on these devices;
flow control is entirely managed in software - there is no hardware
assistance.

The flow control implemented for non-FIFO and non-hardware assisted
UARTs is purely to do with preventing the software buffer behind the
UART from overflowing - it can't prevent the device's receiver buffering
from overrunning.  (Non-FIFO devices have the shift register and a
buffer register - complete reception of a second character before the
first is read causes an overrun condition.)

If you have overruns on the receiver, that's an interrupt latency
problem and its an error that's reported to the receiver side only.

Yes, later devices have the ability in hardware to deassert RTS when
the receiver FIFO gets above a certain threshold to /help/ prevent
overruns occuring when there's high interrupt latency - but normal
system operation should still ensure that the FIFO is emptied in a
timely manner.

They also gained the ability to stop the transmitter once CTS is
deasserted, mainly because the transmitter FIFOs in these devices
is soo large (maybe 32 to 128 bytes deep.)

[PATCH] serial imx.c: fix CTS trigger level lower to avoid lost chars

[Wolfram Sang]

> What you're assuming is that flow control was there to prevent overruns
> on the hardware receiver.  That's not the way it works on these devices;
> flow control is entirely managed in software - there is no hardware
> assistance.
> 
> The flow control implemented for non-FIFO and non-hardware assisted
> UARTs is purely to do with preventing the software buffer behind the
> UART from overflowing - it can't prevent the device's receiver buffering
> from overrunning.  (Non-FIFO devices have the shift register and a
> buffer register - complete reception of a second character before the
> first is read causes an overrun condition.)
> 
> If you have overruns on the receiver, that's an interrupt latency
> problem and its an error that's reported to the receiver side only.
> 
> Yes, later devices have the ability in hardware to deassert RTS when
> the receiver FIFO gets above a certain threshold to /help/ prevent
> overruns occuring when there's high interrupt latency - but normal
> system operation should still ensure that the FIFO is emptied in a
> timely manner.
> 
> They also gained the ability to stop the transmitter once CTS is
> deasserted, mainly because the transmitter FIFOs in these devices
> is soo large (maybe 32 to 128 bytes deep.)

Russell, thanks for this explanation. Patch seems principally OK to me, then.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100122/5e70afe5/attachment-0001.sig>

[PATCH] serial imx.c: fix CTS trigger level lower to avoid lost chars

[Jamie Lokier]

Wolfram Sang wrote:
> > What you're assuming is that flow control was there to prevent overruns
> > on the hardware receiver.  That's not the way it works on these devices;
> > flow control is entirely managed in software - there is no hardware
> > assistance.
> > 
> > The flow control implemented for non-FIFO and non-hardware assisted
> > UARTs is purely to do with preventing the software buffer behind the
> > UART from overflowing - it can't prevent the device's receiver buffering
> > from overrunning.  (Non-FIFO devices have the shift register and a
> > buffer register - complete reception of a second character before the
> > first is read causes an overrun condition.)
> > 
> > If you have overruns on the receiver, that's an interrupt latency
> > problem and its an error that's reported to the receiver side only.
> > 
> > Yes, later devices have the ability in hardware to deassert RTS when
> > the receiver FIFO gets above a certain threshold to /help/ prevent
> > overruns occuring when there's high interrupt latency - but normal
> > system operation should still ensure that the FIFO is emptied in a
> > timely manner.
> > 
> > They also gained the ability to stop the transmitter once CTS is
> > deasserted, mainly because the transmitter FIFOs in these devices
> > is soo large (maybe 32 to 128 bytes deep.)
> 
> Russell, thanks for this explanation. Patch seems principally OK to me, then.

I agree.

-- Jamie

[PATCH] serial imx.c: fix CTS trigger level lower to avoid lost chars

[Jamie Lokier]

Russell King - ARM Linux wrote:
> On Fri, Jan 22, 2010 at 11:07:01AM +0000, Jamie Lokier wrote:
> > Valentin Longchamp wrote:
> > > First I wanted to fix the fact that the imx serial hardware only raises 
> > > the CTS pin when the oferflowing char begins to be set. I would have 
> > > solved this by setting the CTS trigger value to 31 instead of 32. But 
> > > when I talked with one colleague he told me about 16550A (that is 
> > > present nearly everywhere) that empty their TX FIFO, so I
> > > lowered it to 16.
> > 
> > That seems quite peculiar because a 16550A only has a 16 byte receive
> > buffer, doesn't it?  And therefore it would have trouble talking
> > reliably to another 16550A if it worked like that.  I wonder what I'm
> > not seeing in this picture.
> 
> What you're assuming is that flow control was there to prevent overruns
> on the hardware receiver.  That's not the way it works on these devices;
> flow control is entirely managed in software - there is no hardware
> assistance.
> 
> The flow control implemented for non-FIFO and non-hardware assisted
> UARTs is purely to do with preventing the software buffer behind the
> UART from overflowing - it can't prevent the device's receiver buffering
> from overrunning.  (Non-FIFO devices have the shift register and a
> buffer register - complete reception of a second character before the
> first is read causes an overrun condition.)

Thanks.  That is an excellent explanation.

> If you have overruns on the receiver, that's an interrupt latency
> problem and its an error that's reported to the receiver side only.

Yeah :-/ On an ARM-related note, I have a 166MHz ARM device here which
cannot receive reliably at 38400 baud.  The serial port isn't an ARM
one, but it's quite straightforward and has a 16 byte FIFO, which
translates to 240 irqs per second.  I believe the problem is an
interrupt latency problem, especially when it's busy doing other
things too like disk and network access, but surprisingly even when
it's not very active with other things.

Any tips on how to make the serial receive irq latency more reliable
on ARM, with a boringly generic serial driver?

> Yes, later devices have the ability in hardware to deassert RTS when
> the receiver FIFO gets above a certain threshold to /help/ prevent
> overruns occuring when there's high interrupt latency - but normal
> system operation should still ensure that the FIFO is emptied in a
> timely manner.
> 
> They also gained the ability to stop the transmitter once CTS is
> deasserted, mainly because the transmitter FIFOs in these devices
> is soo large (maybe 32 to 128 bytes deep.)

If only they were all like that. :-/

-- Jamie

[PATCH] serial imx.c: fix CTS trigger level lower to avoid lost chars

[Russell King - ARM Linux]

On Fri, Jan 22, 2010 at 04:47:13PM +0000, Jamie Lokier wrote:
> Any tips on how to make the serial receive irq latency more reliable
> on ARM, with a boringly generic serial driver?

What you could do is arrange for a backtrace to be printed when
you read an overrun condition from the UART - if interrupts were
disabled by something taking a long time, and then enabled, you
should see the point where they were just enabled in the backtrace.

It could be down to another IRQF_DISABLED interrupt - in which case
you won't see it in the backtrace.

Of course, printing a backtrace will cause subsequent overruns...

[PATCH] serial imx.c: fix CTS trigger level lower to avoid lost chars

[Wolfram Sang]

On Thu, Jan 21, 2010 at 10:26:03PM +0100, Valentin Longchamp wrote:
> The imx CTS trigger level is left at its reset value that is 32
> chars. Since the RX FIFO has 32 entries, when CTS is raised, the
> FIFO already is full. However, some serial port devices first empty
> their TX FIFO before stopping when CTS is raised, resulting in lost
> chars.
> 
> This patch sets the trigger level lower so that other chars arrive
> after CTS is raised, there is still room for 16 of them.
> 
> Signed-off-by: Valentin Longchamp <valentin.longchamp@epfl.ch>
> Tested-by: Philippe R?tornaz <philippe.retornaz@epfl.ch>

If you fix the comment, feel free to add my:

Acked-by: Wolfram Sang <w.sang@pengutronix.de>

> ---
>  drivers/serial/imx.c |   10 +++++++++-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/serial/imx.c b/drivers/serial/imx.c
> index 18130f1..f7d2da5 100644
> --- a/drivers/serial/imx.c
> +++ b/drivers/serial/imx.c
> @@ -119,7 +119,8 @@
>  #define  MX2_UCR3_RXDMUXSEL	 (1<<2)  /* RXD Muxed Input Select, on mx2/mx3 */
>  #define  UCR3_INVT  	 (1<<1)  /* Inverted Infrared transmission */
>  #define  UCR3_BPEN  	 (1<<0)  /* Preset registers enable */
> -#define  UCR4_CTSTL_32   (32<<10) /* CTS trigger level (32 chars) */
> +#define  UCR4_CTSTL_SHF  10      /* CTS trigger level shift */
> +#define  UCR4_CTSTL_MASK 0x3F    /* CTS trigger is 6 bits wide */
>  #define  UCR4_INVR  	 (1<<9)  /* Inverted infrared reception */
>  #define  UCR4_ENIRI 	 (1<<8)  /* Serial infrared interrupt enable */
>  #define  UCR4_WKEN  	 (1<<7)  /* Wake interrupt enable */
> @@ -590,6 +591,8 @@ static int imx_setup_ufcr(struct imx_port *sport, unsigned int mode)
>  	return 0;
>  }
>  
> +#define CTSTL 16
> +

Maybe a comment that it is set to half the buffer size? Dunno...

>  static int imx_startup(struct uart_port *port)
>  {
>  	struct imx_port *sport = (struct imx_port *)port;
> @@ -606,6 +609,11 @@ static int imx_startup(struct uart_port *port)
>  	if (USE_IRDA(sport))
>  		temp |= UCR4_IRSC;
>  
> +	/* set the trigger level for CTS to 16 by default
> +	 */

Make this comment a one-liner. Also remove the "to 16 by default" part as this
value is #defined and, who knows, it may change.

> +	temp &= ~(UCR4_CTSTL_MASK << UCR4_CTSTL_SHF);
> +	temp |= CTSTL << UCR4_CTSTL_SHF;
> +
>  	writel(temp & ~UCR4_DREN, sport->port.membase + UCR4);
>  
>  	if (USE_IRDA(sport)) {

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100122/7de68086/attachment.sig>

[PATCH] serial imx.c: fix CTS trigger level lower to avoid lost chars

[Valentin Longchamp]

The imx CTS trigger level is left at its reset value that is 32
chars. Since the RX FIFO has 32 entries, when CTS is raised, the
FIFO already is full. However, some serial port devices first empty
their TX FIFO before stopping when CTS is raised, resulting in lost
chars.

This patch sets the trigger level lower so that other chars arrive
after CTS is raised, there is still room for 16 of them.

Signed-off-by: Valentin Longchamp <valentin.longchamp@epfl.ch>
Tested-by: Philippe R?tornaz <philippe.retornaz@epfl.ch>
Acked-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/serial/imx.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/drivers/serial/imx.c b/drivers/serial/imx.c
index 60d665a..071c614 100644
--- a/drivers/serial/imx.c
+++ b/drivers/serial/imx.c
@@ -119,7 +119,8 @@
 #define  MX2_UCR3_RXDMUXSEL	 (1<<2)  /* RXD Muxed Input Select, on mx2/mx3 */
 #define  UCR3_INVT  	 (1<<1)  /* Inverted Infrared transmission */
 #define  UCR3_BPEN  	 (1<<0)  /* Preset registers enable */
-#define  UCR4_CTSTL_32   (32<<10) /* CTS trigger level (32 chars) */
+#define  UCR4_CTSTL_SHF  10      /* CTS trigger level shift */
+#define  UCR4_CTSTL_MASK 0x3F    /* CTS trigger is 6 bits wide */
 #define  UCR4_INVR  	 (1<<9)  /* Inverted infrared reception */
 #define  UCR4_ENIRI 	 (1<<8)  /* Serial infrared interrupt enable */
 #define  UCR4_WKEN  	 (1<<7)  /* Wake interrupt enable */
@@ -590,6 +591,9 @@ static int imx_setup_ufcr(struct imx_port *sport, unsigned int mode)
 	return 0;
 }
 
+/* half the RX buffer size */
+#define CTSTL 16
+
 static int imx_startup(struct uart_port *port)
 {
 	struct imx_port *sport = (struct imx_port *)port;
@@ -606,6 +610,10 @@ static int imx_startup(struct uart_port *port)
 	if (USE_IRDA(sport))
 		temp |= UCR4_IRSC;
 
+	/* set the trigger level for CTS */
+	temp &= ~(UCR4_CTSTL_MASK << UCR4_CTSTL_SHF);
+	temp |= CTSTL << UCR4_CTSTL_SHF;
+
 	writel(temp & ~UCR4_DREN, sport->port.membase + UCR4);
 
 	if (USE_IRDA(sport)) {
-- 
1.6.3.3