serial: mvebu_uart: fix tx lost characters

PATCH New Gabriel Matni 3 participants 7 messages Feb 22, 2018 – Mar 6, 2018

Subsystems (2)

TTY layer and serial drivers Marvell armada 3700 serial driver

lore

Share & tools

lore

Gabriel Matni

author

2018-02-22 20:30 UTC #0 lore raw

PATCH 1/1 serial: mvebu_uart: fix tx lost characters

From: Gabriel Matni <[email protected]>

Fixes missing characters on kernel console at low baud rates (i.e.9600). The driver should poll TX_RDY instead of TX_EMPTY to ensure that the transmitter holding is ready to receive a new byte. Polling TX_EMPTY isn't enough as the hardware buffer can become empty but not yet ready for CPU to write the next byte.

Signed-off-by Gabriel Matni <[email protected]>

---
drivers/tty/serial/mvebu-uart.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

drivers/tty/serial/mvebu-uart.c +1 -1

index a100e98259d7..f0df0640208e 100644
@@ -618,7 +618,7 @@ static void wait_for_xmitr(struct uart_port *port)
   u32 val;
 
   readl_poll_timeout_atomic(port->membase + UART_STAT, val,
-             (val & STAT_TX_EMP), 1, 10000);
+             (val & STAT_TX_RDY(port)), 1, 10000);

}

static void mvebu_uart_console_putchar(struct uart_port *port, int ch) --
2.7.4

Miquel Raynal

2018-02-27 13:12 UTC #1 lore raw

PATCH 1/1 serial: mvebu_uart: fix tx lost characters

Hi Gabriel,

Quoted 7 lines · Gabriel Matni

From: Gabriel Matni <[email protected]>
Fixes missing characters on kernel console at low baud rates (i.e.9600). The driver should poll TX_RDY instead of TX_EMPTY to ensure that the transmitter holding is ready to receive a new byte. Polling TX_EMPTY isn't enough as the hardware buffer can become empty but not yet ready for CPU to write the next byte.

I am kind of sceptic with the explanation. My understanding is that:
- TX_EMPTY means the FIFO is empty
- TX_RDY means the FIFO is not full, neither empty, it is a "half loaded" state.
Polling TX_RDY instead of TX_EMPTY should work too, but I don't see why it would fix the loss of character by filling the FIFO when there are bytes in it rather than when it is fully empty.

Quoted 11 lines

Signed-off-by Gabriel Matni <[email protected]>
---
drivers/tty/serial/mvebu-uart.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
drivers/tty/serial/mvebu-uart.c +0 -0
index a100e98259d7..f0df0640208e 100644

drivers/tty/serial/mvebu-uart.c static void wait_for_xmitr(struct uart_port *port) · line 621

Quoted 5 lines

drivers/tty/serial/mvebu-uart.c +1 -1

@@ -618,7 +618,7 @@ static void wait_for_xmitr(struct uart_port *port)
   u32 val;
 
   readl_poll_timeout_atomic(port->membase + UART_STAT, val,
-             (val & STAT_TX_EMP), 1, 10000);
+             (val & STAT_TX_RDY(port)), 1, 10000);

However, this change might have brought one noticeable difference: the bit polled is a per-port bit while the TX_EMPTY bit looks global. So maybe you faced the issue because you are using the ports quite intensively in parallel?

Also it could explain why you only face the issue at slow baudrates:
bytes will remain longer in the FIFO, increasing the probability of collision.

Quoted 5 lines

drivers/tty/serial/mvebu-uart.c +0 -1

 }
 
 static void mvebu_uart_console_putchar(struct uart_port *port, int ch)
--

2.7.4

Thanks,
Miqu?l

Signature · 3 lines

Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Gabriel Matni

author

2018-02-27 21:56 UTC #2 lore raw

PATCH 1/1 serial: mvebu_uart: fix tx lost characters

Hi Miqu?l,

Quoted 22 lines

-----Original Message-----
From: Miquel Raynal [mailto:miquel.raynal at bootlin.com]
Sent: February 27, 2018 8:13 AM
To: Gabriel Matni <[email protected]>
Cc: linux-arm-kernel at lists.infradead.org; gregkh at linuxfoundation.org; Gr?gory Clement <[email protected]>; Thomas Petazzoni <[email protected]>
Subject: Re: [PATCH] serial: mvebu_uart: fix tx lost characters
Hi Gabriel,
Quoted 7 lines · Gabriel Matni
From: Gabriel Matni <[email protected]>
Fixes missing characters on kernel console at low baud rates (i.e.9600). The driver should poll TX_RDY instead of TX_EMPTY to ensure that the transmitter holding is ready to receive a new byte. Polling TX_EMPTY isn't enough as the hardware buffer can become empty but not yet ready for CPU to write the next byte.

Quoted 7 lines

I am kind of sceptic with the explanation. My understanding is that:
- TX_EMPTY means the FIFO is empty
- TX_RDY means the FIFO is not full, neither empty, it is a "half loaded" state.
Polling TX_RDY instead of TX_EMPTY should work too, but I don't see why it would fix the loss of character by filling the FIFO when there are bytes in it rather than when it is fully empty.

TX_READY is set whenever the UART Transmitter Holding register is ready to receive a new byte. It gets cleared as soon as a new byte is written to the register. If the FIFO is empty or still has room, the ready will be set.

TX_EMPTY tells us when it is possible to send a break sequence via SND_BRK_SEQ. While this also indicates that both the THR and the TSR are empty, it does not guarantee that a new byte can be written just yet. I am unsure about the FIFO status in this case as I can encounter TX_FIFO_EMP=0 while TX_EMP=1, which suggests that the FIFO isn't necessarily empty when TX_EMP is set.

Therefore, the driver can either poll TX_FIFO_EMP or TX_READY to know whether it can output a new byte. I personally like TX_READY as it takes advantage of the FIFO.

Quoted 22 lines

Quoted 12 lines
Signed-off-by Gabriel Matni <[email protected]>
---
drivers/tty/serial/mvebu-uart.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/tty/serial/mvebu-uart.c index a100e98259d7..f0df0640208e 100644
drivers/tty/serial/mvebu-uart.c static void wait_for_xmitr(struct uart_port *port) · line 621
Quoted 5 lines
drivers/tty/serial/mvebu-uart.c +1 -1
@@ -618,7 +618,7 @@ static void wait_for_xmitr(struct uart_port *port)
   u32 val;
 
   readl_poll_timeout_atomic(port->membase + UART_STAT, val,
-             (val & STAT_TX_EMP), 1, 10000);
+             (val & STAT_TX_RDY(port)), 1, 10000);
However, this change might have brought one noticeable difference: the bit polled is a per-port bit while the TX_EMPTY bit looks global. So maybe you faced the issue because you are using the ports quite intensively in parallel?

Quoted 3 lines

Also it could explain why you only face the issue at slow baudrates:
bytes will remain longer in the FIFO, increasing the probability of collision.

In both cases, they are per-port bits, however since UARTS are not identical (normal vs extended), the driver implementation treats them differently. For instance, STAT_TX_EMP maps to bit 6 of the status register regardless of the UART, however TX_RDY(port) maps to SR bit 5 on UART1 and SR bit 15 on UART2.

Quoted 6 lines

Quoted 5 lines
drivers/tty/serial/mvebu-uart.c +0 -1
 }
 
 static void mvebu_uart_console_putchar(struct uart_port *port, int ch)
--
2.7.4

Quoted 6 lines

Thanks,
Miqu?l
--
Miquel Raynal, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com

Regards,
Gabriel

Miquel Raynal

2018-03-05 09:47 UTC #3 lore raw

PATCH 1/1 serial: mvebu_uart: fix tx lost characters

Hi Gabriel,

Quoted 26 lines · Gabriel Matni

Hi Miqu?l,
Quoted 24 lines
-----Original Message-----
From: Miquel Raynal [mailto:miquel.raynal at bootlin.com]
Sent: February 27, 2018 8:13 AM
To: Gabriel Matni <[email protected]>
Cc: linux-arm-kernel at lists.infradead.org; gregkh at linuxfoundation.org; Gr?gory Clement <[email protected]>; Thomas Petazzoni <[email protected]>
Subject: Re: [PATCH] serial: mvebu_uart: fix tx lost characters
Hi Gabriel,
Quoted 7 lines · Gabriel Matni
From: Gabriel Matni <[email protected]>
Fixes missing characters on kernel console at low baud rates (i.e.9600). The driver should poll TX_RDY instead of TX_EMPTY to ensure that the transmitter holding is ready to receive a new byte. Polling TX_EMPTY isn't enough as the hardware buffer can become empty but not yet ready for CPU to write the next byte.
I am kind of sceptic with the explanation. My understanding is that:
- TX_EMPTY means the FIFO is empty

I had a deeper look into the spec : TX_EMPTY != TX_FIFO_EMPTY. I was referring to TX_FIFO_EMPTY here so my understanding of your solution was wrong.

Quoted 6 lines

Quoted 5 lines
- TX_RDY means the FIFO is not full, neither empty, it is a "half loaded" state.
Polling TX_RDY instead of TX_EMPTY should work too, but I don't see why it would fix the loss of character by filling the FIFO when there are bytes in it rather than when it is fully empty.

Quoted 7 lines

TX_READY is set whenever the UART Transmitter Holding register is ready to receive a new byte. It gets cleared as soon as a new byte is written to the register. If the FIFO is empty or still has room, the ready will be set.
TX_EMPTY tells us when it is possible to send a break sequence via SND_BRK_SEQ. While this also indicates that both the THR and the TSR are empty, it does not guarantee that a new byte can be written just yet.

I do agree with this statement. You are right that polling on TX_EMPTY looks wrong and we should definitively poll on TX_READY instead.

Quoted 4 lines

I am
unsure about the FIFO status in this case as I can encounter TX_FIFO_EMP=0 while TX_EMP=1, which suggests that the FIFO isn't necessarily empty when TX_EMP is set.

That is weird but maybe the TX_FIFO_EMPTY is asserted only when all bits have been shifted, which is a 10-bit period after TSR is cleared, while TX_EMPTY would not wait for these bits to be actually shifted out and would be asserted a bit earlier, as soon as TSR is cleared. That is just and idea.

Quoted 4 lines

Therefore, the driver can either poll TX_FIFO_EMP or TX_READY to know whether it can output a new byte. I personally like TX_READY as it takes advantage of the FIFO.

True.

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
Signed-off-by Gabriel Matni <[email protected]>

Reviewed-by Miquel Raynal <[email protected]>

Thanks,
Miqu?l

Signature · 3 lines

Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Miquel Raynal

2018-03-05 10:10 UTC #4 lore raw

PATCH 1/1 serial: mvebu_uart: fix tx lost characters

Hi Gabriel,

Quoted 15 lines · Miquel Raynal

Hi Gabriel,
Quoted 10 lines · Gabriel Matni
Hi Miqu?l,
Quoted 8 lines
-----Original Message-----
From: Miquel Raynal [mailto:miquel.raynal at bootlin.com]
Sent: February 27, 2018 8:13 AM
To: Gabriel Matni <[email protected]>
Cc: linux-arm-kernel at lists.infradead.org; gregkh at linuxfoundation.org; Gr?gory Clement <[email protected]>; Thomas Petazzoni <[email protected]>
Subject: Re: [PATCH] serial: mvebu_uart: fix tx lost characters

BTW, prefix should be "serial: mvebu-uart:".

And maybe you should CC: stable too?

Thanks for fixing it anyway.
Miqu?l

Gregory CLEMENT

2018-03-05 13:42 UTC #5 lore raw

PATCH 1/1 serial: mvebu_uart: fix tx lost characters

Hi Gabriel,

Quoted 20 lines · Miquel Raynal

Hi Gabriel,
Quoted 15 lines · Miquel Raynal
Hi Gabriel,
Quoted 10 lines · Gabriel Matni
Hi Miqu?l,
Quoted 8 lines
-----Original Message-----
From: Miquel Raynal [mailto:miquel.raynal at bootlin.com]
Sent: February 27, 2018 8:13 AM
To: Gabriel Matni <[email protected]>
Cc: linux-arm-kernel at lists.infradead.org; gregkh at linuxfoundation.org; Gr?gory Clement <[email protected]>; Thomas Petazzoni <[email protected]>
Subject: Re: [PATCH] serial: mvebu_uart: fix tx lost characters

Given the review done by Miquel and the information you add, you can also add my

Acked-by Gregory CLEMENT <[email protected]>

Quoted 4 lines

BTW, prefix should be "serial: mvebu-uart:".
And maybe you should CC: stable too?

And also adding the following fixes tag:

Fixes 30530791a7a0 ("serial: mvebu-uart: initial support for

Armada-3700 serial port")

Thanks,

Gregory

Quoted 3 lines

Thanks for fixing it anyway.
Miqu?l

Signature · 3 lines

Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

Gabriel Matni

author

2018-03-06 15:39 UTC #6 lore raw

PATCH 1/1 serial: mvebu_uart: fix tx lost characters

Thank you Miqu?l and Gregory for your valuable input. I will resubmit the patch with the fixes.

Regards,
Gabriel

Quoted 63 lines

-----Original Message-----
From: Gregory CLEMENT [mailto:gregory.clement at bootlin.com]
Sent: March 5, 2018 8:43 AM
To: Gabriel Matni <[email protected]>; Miquel Raynal <[email protected]>
Cc: linux-arm-kernel at lists.infradead.org; gregkh at linuxfoundation.org; Thomas Petazzoni <[email protected]>
Subject: Re: [PATCH] serial: mvebu_uart: fix tx lost characters
Hi Gabriel,
Quoted 17 lines · Miquel Raynal
Hi Gabriel,
Quoted 12 lines · Miquel Raynal
Hi Gabriel,
Quoted 7 lines · Gabriel Matni
Hi Miqu?l,
Quoted 5 lines
-----Original Message-----
From: Miquel Raynal [mailto:miquel.raynal at bootlin.com]
Sent: February 27, 2018 8:13 AM
To: Gabriel Matni <[email protected]>
Cc: linux-arm-kernel at lists.infradead.org;
gregkh at linuxfoundation.org;
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Gr?gory Clement <[email protected]>; Thomas Petazzoni <[email protected]>
Subject: Re: [PATCH] serial: mvebu_uart: fix tx lost characters
Given the review done by Miquel and the information you add, you can also add my
Acked-by Gregory CLEMENT <[email protected]>
Quoted 4 lines
BTW, prefix should be "serial: mvebu-uart:".
And maybe you should CC: stable too?
And also adding the following fixes tag:
Fixes 30530791a7a0 ("serial: mvebu-uart: initial support for
Armada-3700 serial port")
Thanks,
Gregory
Quoted 3 lines
Thanks for fixing it anyway.
Miqu?l
--
Gregory Clement, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering
http://bootlin.com