[PATCH v2 0/5] Serial: imx: various fixes

Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH v2 0/5] Serial: imx: various fixes
       [not found] <20190530152950.25377-1-sorganov@gmail.com>
@ 2019-08-28 18:37 ` Sergey Organov
  2019-08-28 18:37   ` [PATCH v2 1/5] serial: imx: get rid of unbounded busy-waiting loop Sergey Organov
                     ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Sergey Organov @ 2019-08-28 18:37 UTC (permalink / raw)
  To: linux-serial
  Cc: Greg Kroah-Hartman, Sascha Hauer, Sergey Organov, NXP Linux Team,
	Pengutronix Kernel Team, Uwe Kleine-König, linux-arm-kernel

The original problem that caused these changes was broken bytes being
sent/received at random on RTS/CTS handshake switch (i.e., on
setting/clearing termios CRTSCTS bit).

As it went, a few other problems were found and fixed, and then the
fix for original issue has been split into multiple patches that seem
to make sense by themselves. Thus, the "serial: imx: fix data breakage
on termios change", that finally fixes the issue, depends on 2
preceding patches.

The last patch in the series, "serial: imx: use Tx ready rather than
Tx empty irq" is independent of the rest and doesn't fix any serious
issue, but it should get rid of holes in continuous output,
specifically in PIO mode.

Changes in v2:
  - Removed wrong [PATCH 1/8] serial: imx: fix DTR inversion
  - Rebased on top of "tty-next"

Sergey Organov (5):
  serial: imx: get rid of unbounded busy-waiting loop
  serial: imx: do not stop Rx/Tx on termios change
  serial: imx: do not disable individual irqs during termios change
  serial: imx: fix data breakage on termios change
  serial: imx: use Tx ready rather than Tx empty irq

 drivers/tty/serial/imx.c | 56 ++++++++++++++++++++++--------------------------
 1 file changed, 26 insertions(+), 30 deletions(-)

-- 
2.10.0.1.g57b01a3

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v2 1/5] serial: imx: get rid of unbounded busy-waiting loop
  2019-08-28 18:37 ` [PATCH v2 0/5] Serial: imx: various fixes Sergey Organov
@ 2019-08-28 18:37   ` Sergey Organov
  2019-08-28 18:37   ` [PATCH v2 2/5] serial: imx: do not stop Rx/Tx on termios change Sergey Organov
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Sergey Organov @ 2019-08-28 18:37 UTC (permalink / raw)
  To: linux-serial
  Cc: Greg Kroah-Hartman, Sascha Hauer, Sergey Organov, NXP Linux Team,
	Pengutronix Kernel Team, Uwe Kleine-König, linux-arm-kernel

imx_set_termios(): remove busy-waiting "drain Tx FIFO" loop. Worse
yet, it was potentially unbounded wait due to RTS/CTS (hardware)
handshake.

Let user space ensure draining is done before termios change, if
draining is needed in the first place.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index d9a73c7..47b6156 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1644,7 +1644,7 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 	uart_update_timeout(port, termios->c_cflag, baud);
 
 	/*
-	 * disable interrupts and drain transmitter
+	 * disable interrupts
 	 */
 	old_ucr1 = imx_uart_readl(sport, UCR1);
 	imx_uart_writel(sport,
@@ -1652,9 +1652,6 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 			UCR1);
 	imx_uart_writel(sport, old_ucr2 & ~UCR2_ATEN, UCR2);
 
-	while (!(imx_uart_readl(sport, USR2) & USR2_TXDC))
-		barrier();
-
 	/* then, disable everything */
 	imx_uart_writel(sport, old_ucr2 & ~(UCR2_TXEN | UCR2_RXEN | UCR2_ATEN), UCR2);
 
-- 
2.10.0.1.g57b01a3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 2/5] serial: imx: do not stop Rx/Tx on termios change
  2019-08-28 18:37 ` [PATCH v2 0/5] Serial: imx: various fixes Sergey Organov
  2019-08-28 18:37   ` [PATCH v2 1/5] serial: imx: get rid of unbounded busy-waiting loop Sergey Organov
@ 2019-08-28 18:37   ` Sergey Organov
  2019-08-28 18:37   ` [PATCH v2 3/5] serial: imx: do not disable individual irqs during " Sergey Organov
                     ` (2 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Sergey Organov @ 2019-08-28 18:37 UTC (permalink / raw)
  To: linux-serial
  Cc: Greg Kroah-Hartman, Sascha Hauer, Sergey Organov, NXP Linux Team,
	Pengutronix Kernel Team, Uwe Kleine-König, linux-arm-kernel

imx_set_termios(): stopping receiver and transmitter does harm when
something that doesn't touch transmission format/rate changes, such as
RTS/CTS handshake.

OTOH, it does no good on baud rate or format change, as
synchronization on upper-level protocols is still required to do it
right.

Therefore, just stop doing it.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 47b6156..fa723a9a 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1652,9 +1652,6 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 			UCR1);
 	imx_uart_writel(sport, old_ucr2 & ~UCR2_ATEN, UCR2);
 
-	/* then, disable everything */
-	imx_uart_writel(sport, old_ucr2 & ~(UCR2_TXEN | UCR2_RXEN | UCR2_ATEN), UCR2);
-
 	/* custom-baudrate handling */
 	div = sport->port.uartclk / (baud * 16);
 	if (baud == 38400 && quot != div)
-- 
2.10.0.1.g57b01a3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 3/5] serial: imx: do not disable individual irqs during termios change
  2019-08-28 18:37 ` [PATCH v2 0/5] Serial: imx: various fixes Sergey Organov
  2019-08-28 18:37   ` [PATCH v2 1/5] serial: imx: get rid of unbounded busy-waiting loop Sergey Organov
  2019-08-28 18:37   ` [PATCH v2 2/5] serial: imx: do not stop Rx/Tx on termios change Sergey Organov
@ 2019-08-28 18:37   ` Sergey Organov
  2019-08-28 18:37   ` [PATCH v2 4/5] serial: imx: fix data breakage on " Sergey Organov
  2019-08-28 18:37   ` [PATCH v2 5/5] serial: imx: use Tx ready rather than Tx empty irq Sergey Organov
  4 siblings, 0 replies; 6+ messages in thread
From: Sergey Organov @ 2019-08-28 18:37 UTC (permalink / raw)
  To: linux-serial
  Cc: Greg Kroah-Hartman, Sascha Hauer, Sergey Organov, NXP Linux Team,
	Pengutronix Kernel Team, Uwe Kleine-König, linux-arm-kernel

imx_set_termios(): disabling individual interrupt requests in UART for
duration of the routine is pointless. Get rid of it.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index fa723a9a..cc3783c 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1541,7 +1541,7 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 {
 	struct imx_port *sport = (struct imx_port *)port;
 	unsigned long flags;
-	u32 ucr2, old_ucr1, old_ucr2, ufcr;
+	u32 ucr2, old_ucr2, ufcr;
 	unsigned int baud, quot;
 	unsigned int old_csize = old ? old->c_cflag & CSIZE : CS8;
 	unsigned long div;
@@ -1643,15 +1643,6 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 	 */
 	uart_update_timeout(port, termios->c_cflag, baud);
 
-	/*
-	 * disable interrupts
-	 */
-	old_ucr1 = imx_uart_readl(sport, UCR1);
-	imx_uart_writel(sport,
-			old_ucr1 & ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN),
-			UCR1);
-	imx_uart_writel(sport, old_ucr2 & ~UCR2_ATEN, UCR2);
-
 	/* custom-baudrate handling */
 	div = sport->port.uartclk / (baud * 16);
 	if (baud == 38400 && quot != div)
@@ -1686,8 +1677,6 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 		imx_uart_writel(sport, sport->port.uartclk / div / 1000,
 				IMX21_ONEMS);
 
-	imx_uart_writel(sport, old_ucr1, UCR1);
-
 	imx_uart_writel(sport, ucr2, UCR2);
 
 	if (UART_ENABLE_MS(&sport->port, termios->c_cflag))
-- 
2.10.0.1.g57b01a3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 4/5] serial: imx: fix data breakage on termios change
  2019-08-28 18:37 ` [PATCH v2 0/5] Serial: imx: various fixes Sergey Organov
                     ` (2 preceding siblings ...)
  2019-08-28 18:37   ` [PATCH v2 3/5] serial: imx: do not disable individual irqs during " Sergey Organov
@ 2019-08-28 18:37   ` Sergey Organov
  2019-08-28 18:37   ` [PATCH v2 5/5] serial: imx: use Tx ready rather than Tx empty irq Sergey Organov
  4 siblings, 0 replies; 6+ messages in thread
From: Sergey Organov @ 2019-08-28 18:37 UTC (permalink / raw)
  To: linux-serial
  Cc: Greg Kroah-Hartman, Sascha Hauer, Sergey Organov, NXP Linux Team,
	Pengutronix Kernel Team, Uwe Kleine-König, linux-arm-kernel

imx_set_termios(): avoid writing baud rate divider registers when the
values to be written are the same as current. Any writing seems to
restart transmission/receiving logic in the hardware, that leads to
data breakage even when rate doesn't in fact change. E.g., user
switches RTS/CTS handshake and suddenly gets broken bytes.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index cc3783c..e89045a 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1545,7 +1545,7 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 	unsigned int baud, quot;
 	unsigned int old_csize = old ? old->c_cflag & CSIZE : CS8;
 	unsigned long div;
-	unsigned long num, denom;
+	unsigned long num, denom, old_ubir, old_ubmr;
 	uint64_t tdiv64;
 
 	/*
@@ -1670,8 +1670,21 @@ imx_uart_set_termios(struct uart_port *port, struct ktermios *termios,
 	ufcr = (ufcr & (~UFCR_RFDIV)) | UFCR_RFDIV_REG(div);
 	imx_uart_writel(sport, ufcr, UFCR);
 
-	imx_uart_writel(sport, num, UBIR);
-	imx_uart_writel(sport, denom, UBMR);
+	/*
+	 *  Two registers below should always be written both and in this
+	 *  particular order. One consequence is that we need to check if any of
+	 *  them changes and then update both. We do need the check for change
+	 *  as even writing the same values seem to "restart"
+	 *  transmission/receiving logic in the hardware, that leads to data
+	 *  breakage even when rate doesn't in fact change. E.g., user switches
+	 *  RTS/CTS handshake and suddenly gets broken bytes.
+	 */
+	old_ubir = imx_uart_readl(sport, UBIR);
+	old_ubmr = imx_uart_readl(sport, UBMR);
+	if (old_ubir != num || old_ubmr != denom) {
+		imx_uart_writel(sport, num, UBIR);
+		imx_uart_writel(sport, denom, UBMR);
+	}
 
 	if (!imx_uart_is_imx1(sport))
 		imx_uart_writel(sport, sport->port.uartclk / div / 1000,
-- 
2.10.0.1.g57b01a3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 5/5] serial: imx: use Tx ready rather than Tx empty irq
  2019-08-28 18:37 ` [PATCH v2 0/5] Serial: imx: various fixes Sergey Organov
                     ` (3 preceding siblings ...)
  2019-08-28 18:37   ` [PATCH v2 4/5] serial: imx: fix data breakage on " Sergey Organov
@ 2019-08-28 18:37   ` Sergey Organov
  4 siblings, 0 replies; 6+ messages in thread
From: Sergey Organov @ 2019-08-28 18:37 UTC (permalink / raw)
  To: linux-serial
  Cc: Greg Kroah-Hartman, Sascha Hauer, Sergey Organov, NXP Linux Team,
	Pengutronix Kernel Team, Uwe Kleine-König, linux-arm-kernel

This should help to avoid unnecessary gaps in transmission while
adding little overhead due to low default Tx threshold level (2
bytes).

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 drivers/tty/serial/imx.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index e89045a..87c58f9 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -439,7 +439,7 @@ static void imx_uart_stop_tx(struct uart_port *port)
 		return;
 
 	ucr1 = imx_uart_readl(sport, UCR1);
-	imx_uart_writel(sport, ucr1 & ~UCR1_TXMPTYEN, UCR1);
+	imx_uart_writel(sport, ucr1 & ~UCR1_TRDYEN, UCR1);
 
 	/* in rs485 mode disable transmitter if shifter is empty */
 	if (port->rs485.flags & SER_RS485_ENABLED &&
@@ -517,7 +517,7 @@ static inline void imx_uart_transmit_buffer(struct imx_port *sport)
 		 * and the TX IRQ is disabled.
 		 **/
 		ucr1 = imx_uart_readl(sport, UCR1);
-		ucr1 &= ~UCR1_TXMPTYEN;
+		ucr1 &= ~UCR1_TRDYEN;
 		if (sport->dma_is_txing) {
 			ucr1 |= UCR1_TXDMAEN;
 			imx_uart_writel(sport, ucr1, UCR1);
@@ -679,7 +679,7 @@ static void imx_uart_start_tx(struct uart_port *port)
 
 	if (!sport->dma_is_enabled) {
 		ucr1 = imx_uart_readl(sport, UCR1);
-		imx_uart_writel(sport, ucr1 | UCR1_TXMPTYEN, UCR1);
+		imx_uart_writel(sport, ucr1 | UCR1_TRDYEN, UCR1);
 	}
 
 	if (sport->dma_is_enabled) {
@@ -688,7 +688,7 @@ static void imx_uart_start_tx(struct uart_port *port)
 			 * disable TX DMA to let TX interrupt to send X-char */
 			ucr1 = imx_uart_readl(sport, UCR1);
 			ucr1 &= ~UCR1_TXDMAEN;
-			ucr1 |= UCR1_TXMPTYEN;
+			ucr1 |= UCR1_TRDYEN;
 			imx_uart_writel(sport, ucr1, UCR1);
 			return;
 		}
@@ -874,7 +874,7 @@ static irqreturn_t imx_uart_int(int irq, void *dev_id)
 		usr1 &= ~USR1_RRDY;
 	if ((ucr2 & UCR2_ATEN) == 0)
 		usr1 &= ~USR1_AGTIM;
-	if ((ucr1 & UCR1_TXMPTYEN) == 0)
+	if ((ucr1 & UCR1_TRDYEN) == 0)
 		usr1 &= ~USR1_TRDY;
 	if ((ucr4 & UCR4_TCEN) == 0)
 		usr2 &= ~USR2_TXDC;
@@ -1474,7 +1474,7 @@ static void imx_uart_shutdown(struct uart_port *port)
 
 	spin_lock_irqsave(&sport->port.lock, flags);
 	ucr1 = imx_uart_readl(sport, UCR1);
-	ucr1 &= ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN | UCR1_UARTEN | UCR1_RXDMAEN | UCR1_ATDMAEN);
+	ucr1 &= ~(UCR1_TRDYEN | UCR1_RRDYEN | UCR1_RTSDEN | UCR1_UARTEN | UCR1_RXDMAEN | UCR1_ATDMAEN);
 
 	imx_uart_writel(sport, ucr1, UCR1);
 	spin_unlock_irqrestore(&sport->port.lock, flags);
@@ -1778,7 +1778,7 @@ static int imx_uart_poll_init(struct uart_port *port)
 		ucr1 |= IMX1_UCR1_UARTCLKEN;
 
 	ucr1 |= UCR1_UARTEN;
-	ucr1 &= ~(UCR1_TXMPTYEN | UCR1_RTSDEN | UCR1_RRDYEN);
+	ucr1 &= ~(UCR1_TRDYEN | UCR1_RTSDEN | UCR1_RRDYEN);
 
 	ucr2 |= UCR2_RXEN;
 	ucr2 &= ~UCR2_ATEN;
@@ -1938,7 +1938,7 @@ imx_uart_console_write(struct console *co, const char *s, unsigned int count)
 	if (imx_uart_is_imx1(sport))
 		ucr1 |= IMX1_UCR1_UARTCLKEN;
 	ucr1 |= UCR1_UARTEN;
-	ucr1 &= ~(UCR1_TXMPTYEN | UCR1_RRDYEN | UCR1_RTSDEN);
+	ucr1 &= ~(UCR1_TRDYEN | UCR1_RRDYEN | UCR1_RTSDEN);
 
 	imx_uart_writel(sport, ucr1, UCR1);
 
@@ -2294,7 +2294,7 @@ static int imx_uart_probe(struct platform_device *pdev)
 	/* Disable interrupts before requesting them */
 	ucr1 = imx_uart_readl(sport, UCR1);
 	ucr1 &= ~(UCR1_ADEN | UCR1_TRDYEN | UCR1_IDEN | UCR1_RRDYEN |
-		 UCR1_TXMPTYEN | UCR1_RTSDEN);
+		 UCR1_TRDYEN | UCR1_RTSDEN);
 	imx_uart_writel(sport, ucr1, UCR1);
 
 	if (!imx_uart_is_imx1(sport) && sport->dte_mode) {
-- 
2.10.0.1.g57b01a3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-08-28 18:39 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20190530152950.25377-1-sorganov@gmail.com>
2019-08-28 18:37 ` [PATCH v2 0/5] Serial: imx: various fixes Sergey Organov
2019-08-28 18:37   ` [PATCH v2 1/5] serial: imx: get rid of unbounded busy-waiting loop Sergey Organov
2019-08-28 18:37   ` [PATCH v2 2/5] serial: imx: do not stop Rx/Tx on termios change Sergey Organov
2019-08-28 18:37   ` [PATCH v2 3/5] serial: imx: do not disable individual irqs during " Sergey Organov
2019-08-28 18:37   ` [PATCH v2 4/5] serial: imx: fix data breakage on " Sergey Organov
2019-08-28 18:37   ` [PATCH v2 5/5] serial: imx: use Tx ready rather than Tx empty irq Sergey Organov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox