mxs-auart broken in v6.2 and onwards

mxs-auart broken in v6.2 and onwards

[Emil Kronborg]

Hi,

After updating Linux on an i.MX28 board, serial communication over AUART
broke. When I TX from the board and measure on the TX pin, it seems like
the HW fifo is not emptied before the transmission is stopped. I
bisected the bad commit to be 2d141e683e9a ("tty: serial: use
uart_port_tx() helper"). Since it concerns multiple drivers, simply
reverting it is not feasible. One solution would be to effectively
revert the commit for just mxs-auart.c, but maybe you have a better
idea? Any pointers is appreciated.

Regards,
Emil

Re: mxs-auart broken in v6.2 and onwards

[Jiri Slaby]

[-- Attachment #1: Type: text/plain, Size: 684 bytes --]

Hi,

On 27. 01. 24, 22:35, Emil Kronborg wrote:
> After updating Linux on an i.MX28 board, serial communication over AUART
> broke. When I TX from the board and measure on the TX pin, it seems like
> the HW fifo is not emptied before the transmission is stopped. I
> bisected the bad commit to be 2d141e683e9a ("tty: serial: use
> uart_port_tx() helper"). Since it concerns multiple drivers, simply
> reverting it is not feasible. One solution would be to effectively
> revert the commit for just mxs-auart.c, but maybe you have a better
> idea? Any pointers is appreciated.

Hm, the tx stop handling is weird throughout mxs. What about the 
attached patch?

thanks,
-- 
js
suse labs

[-- Attachment #2: 0001-mxs-fix.patch --]
[-- Type: text/x-patch, Size: 2848 bytes --]

From 0f5b15374953f1e4b65722b3dbbef9ce14a3c018 Mon Sep 17 00:00:00 2001
From: "Jiri Slaby (SUSE)" <jirislaby@kernel.org>
Date: Tue, 30 Jan 2024 10:36:35 +0100
Subject: [PATCH] mxs: fix

Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
---
 drivers/tty/serial/mxs-auart.c |  5 ++++-
 include/linux/serial_core.h    | 10 ++++++----
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
index 424db8466755..ec013d13cc3d 100644
--- a/drivers/tty/serial/mxs-auart.c
+++ b/drivers/tty/serial/mxs-auart.c
@@ -594,13 +594,16 @@ static void mxs_auart_tx_chars(struct mxs_auart_port *s)
 		return;
 	}
 
-	pending = uart_port_tx(&s->port, ch,
+	pending = uart_port_tx_no_stop(&s->port, ch,
 		!(mxs_read(s, REG_STAT) & AUART_STAT_TXFF),
 		mxs_write(ch, s, REG_DATA));
 	if (pending)
 		mxs_set(AUART_INTR_TXIEN, s, REG_INTR);
 	else
 		mxs_clr(AUART_INTR_TXIEN, s, REG_INTR);
+
+	if (uart_tx_stopped(&s->port))
+               mxs_auart_stop_tx(&s->port);
 }
 
 static void mxs_auart_rx_char(struct mxs_auart_port *s)
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index c5fa72c8f96e..18256fd765c9 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -769,7 +769,7 @@ struct uart_driver {
 
 void uart_write_wakeup(struct uart_port *port);
 
-#define __uart_port_tx(uport, ch, tx_ready, put_char, tx_done, for_test,      \
+#define __uart_port_tx(uport, ch, tx_ready, put_char, should_stop, tx_done, for_test,      \
 		for_post)						      \
 ({									      \
 	struct uart_port *__port = (uport);				      \
@@ -799,7 +799,7 @@ void uart_write_wakeup(struct uart_port *port);
 	if (pending < WAKEUP_CHARS) {					      \
 		uart_write_wakeup(__port);				      \
 									      \
-		if (pending == 0)					      \
+		if (should_stop && pending == 0)					      \
 			__port->ops->stop_tx(__port);			      \
 	}								      \
 									      \
@@ -834,7 +834,7 @@ void uart_write_wakeup(struct uart_port *port);
  */
 #define uart_port_tx_limited(port, ch, count, tx_ready, put_char, tx_done) ({ \
 	unsigned int __count = (count);					      \
-	__uart_port_tx(port, ch, tx_ready, put_char, tx_done, __count,	      \
+	__uart_port_tx(port, ch, tx_ready, put_char, true, tx_done, __count,	      \
 			__count--);					      \
 })
 
@@ -848,8 +848,10 @@ void uart_write_wakeup(struct uart_port *port);
  * See uart_port_tx_limited() for more details.
  */
 #define uart_port_tx(port, ch, tx_ready, put_char)			\
-	__uart_port_tx(port, ch, tx_ready, put_char, ({}), true, ({}))
+	__uart_port_tx(port, ch, tx_ready, put_char, true, ({}), true, ({}))
 
+#define uart_port_tx_no_stop(port, ch, tx_ready, put_char)		\
+	__uart_port_tx(port, ch, tx_ready, put_char, false, ({}), true, ({}))
 /*
  * Baud rate helpers.
  */
-- 
2.43.0

Re: Re: mxs-auart broken in v6.2 and onwards

[Emil Kronborg]

Hi,

On Tue, Jan 30, 2024 at 10:38 +0100, Jiri Slaby wrote:
> Hm, the tx stop handling is weird throughout mxs. What about the
> attached patch?

It seems to fix the issue, but I do not see any way to fix this just for
mxs-auart.c without also changing serial_core.h, and thus affecting a
bunch of other drivers. Would reverting the changes from 2d141e683e9a
("tty: serial: use uart_port_tx() helper") for just mxs-auart.c be okay?
If so, I can do that, test it again, and send a patch.

--
Emil

Re: mxs-auart broken in v6.2 and onwards

[Jiri Slaby]

On 30. 01. 24, 19:57, Emil Kronborg wrote:
> Hi,
> 
> On Tue, Jan 30, 2024 at 10:38 +0100, Jiri Slaby wrote:
>> Hm, the tx stop handling is weird throughout mxs. What about the
>> attached patch?
> 
> It seems to fix the issue, but I do not see any way to fix this just for
> mxs-auart.c without also changing serial_core.h, and thus affecting a
> bunch of other drivers.

Hi, the fix I sent to you does not affect any other driver. (Even when 
it touches serial_core.h.)

> Would reverting the changes from 2d141e683e9a
> ("tty: serial: use uart_port_tx() helper") for just mxs-auart.c be okay?

Nope.

> If so, I can do that, test it again, and send a patch.

I've sent proper patches to fix the issue. If you can retest, please add 
your Tested-by line in there.

thanks,
-- 
js
suse labs