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

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 <[email protected]>

Tested-by Philippe R?tornaz <[email protected]>

---
 drivers/serial/imx.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

drivers/serial/imx.c +9 -1

index 18130f1..f7d2da5 100644
@@ -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)) {

Signature · 7 lines

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

CCing linux-serial

Quoted 5 lines · 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.

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

Quoted 8 lines · Wolfram Sang

CCing linux-serial

Quoted 5 lines · 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.

Quoted 2 lines

? 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.

Quoted 5 lines · Valentin Longchamp

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.)

Quoted 21 lines

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.

Quoted 3 lines

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.

Quoted 8 lines · Jamie Lokier

Quoted 6 lines · Valentin Longchamp

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.

Quoted 4 lines

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.)

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

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 <[email protected]>

Tested-by Philippe R?tornaz <[email protected]>

---
 drivers/serial/imx.c |   10 +++++++++-
 1 files changed, 9 insertions(+), 1 deletions(-)

drivers/serial/imx.c +2 -1

index 18130f1..f7d2da5 100644
@@ -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 */

drivers/serial/imx.c +3 -0

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

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<[email protected]>

Tested-by Philippe Rétornaz<[email protected]>

Acked-by Wolfram Sang<[email protected]>

---
  drivers/serial/imx.c |   10 +++++++++-
  1 files changed, 9 insertions(+), 1 deletions(-)

drivers/serial/imx.c +9 -1

index 60d665a..071c614 100644
@@ -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)) {

Hello,

I had sent this patch more than 3 months ago and noone has pushed it into mainline. It is very important for our use and I think it may affect other people later.

Is there a linux-serial tree where it could be added to be pulled from for mainline then ? What is the usual path for serial patches ?