Re: [PATCH] serial: 8250 dw: clear FIFO before writting LCR

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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

On Wed, 8 Oct 2025, Adriana Nicolae wrote:
> Hello, Sorry for missing an update, the exact root cause is not clear, currently the
> brute force method of draining FIFO right before setting or clearing DLAB was stable
> during tests.
> 
> The serial stuck seems to be a failed attempt to clear the DLAB.
> This operation fails because the USR indicates the hardware is
> still busy, even though the UART is in loopback mode and should
> be inactive.
> 
> To isolate the issue, I tried the following without success:
> - Added delays: I inserted 100 repeated ndelay(p->frame_time)
> calls before and after enabling loopback mode to allow the FIFO
> to clear.
> - Cleared FIFO: I explicitly cleared the FIFO in addition to
> adding the delay.
> - Checked status: I printed the LSR just before the DLAB clear
> attempt and checked the USB busy bit.

Okay, so the BUSY must be stuck asserted.

Another idea, maybe test tx + rx over loopback to see if that manages to 
de-freeze the BUSY bit. A patch to that effect below.

(I've only added the new block into dw8250_idle_enter() compared with the 
old patch but rebasing to tty-next resulted in some other changes due to 
conflicts.)


Only thing besides BUSY being stuck asserted is something Tx'ing after the 
idle enter sequence. I think we could trap/check that too in 
dw8250_serial_out() by using something along these lines:

	if (d->in_idle && offset == UART_TX) {
		WARN_ON_ONCE(1);
		return;
	}

(I think that should catch even console writes but I'm not 100% sure of 
that and it will should get us where the rogue Tx originates from).

> The critical finding was that immediately before the DLAB clear
> operation (p->serial_out(p, UART_LCR, up->lcr);), the LSR value
> was 0x6a and the USR busy bit [0] was set. This confirms the UART
> is busy, which blocks the DLAB modification.
> 
> This is occurring on a device with a single UART console. The setup
> does not use DMA or modem control; only the Rx/Tx lines are connected.
> 
> The trace below, from a single process, shows one successful DLAB
> clear followed by a failing one. The second attempt repeatedly logs
> "USR still busy" before eventually succeeding. This can lead to
> subsequent failures in dw8250_check_lcr: dw8250_idle_entered.
> 
> Trace Log:
> 
> <...>-15440  8583.592533: dw8250_idle_enter: in_idle = 1
> login-15440  8583.713817: dw8250_idle_enter: in loopback mode
> login-15440  8583.835099: dw8250_idle_enter: LSR in loopback mode is 0x63
> login-15440  8583.835103: dw8250_set_divisor: UART_LCR_DLAB cleared 13
> login-15440  8583.835104: dw8250_idle_exit: idle exit
> login-15440  8583.835105: dw8250_idle_exit: out of loopback mode
> login-15440  8583.835105: dw8250_idle_exit: in_idle = 0
> login-15440  8583.835352: dw8250_idle_enter: in_idle = 1
> login-15440  8583.956633: dw8250_idle_enter: in loopback mode
> login-15440  8583.956638: dw8250_idle_enter: LSR in loopback mode is 0x6a
> login-15440  8583.963918: dw8250_set_divisor: USR still busy dl_write      
> login-15440  8584.000332: dw8250_set_divisor: USR still busy dl_write
> login-15440  8584.040385: dw8250_set_divisor: USR still busy dl_write
> login-15440  8584.078012: dw8250_set_divisor: UART_LCR_DLAB cleared 93
> login-15440  8584.078013: dw8250_idle_exit: idle exit
> login-15440  8584.078014: dw8250_idle_exit: out of loopback mode
> login-15440  8584.078015: dw8250_idle_exit: in_idle = 0



--
From 01df58736a10f7f34aca895ef08e5519953f8572 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Wed, 8 Oct 2025 15:40:19 +0300
Subject: [PATCH 1/1] serial: 8250_dw: Ensure BUSY is deasserted
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

DW UART cannot write to LCR, DLL, and DLH while BUSY is asserted.
Existance of BUSY depends on uart_16550_compatible, if UART HW is
configured with 16550 compatible those registers can always be
written.

There currently is dw8250_force_idle() which attempts to archive
non-BUSY state by disabling FIFO, however, the solution is unreliable
when Rx keeps getting more and more characters.

Create a sequence of operations to enforce that ensures UART cannot
keep BUSY asserted indefinitely. The new sequence relies on enabling
loopback mode temporarily to prevent incoming Rx characters keeping
UART BUSY.

Use the new dw8250_idle_enter/exit() to do divisor writes and LCR
writes.

This issue was reported by qianfan Zhao who put lots of debugging
effort into understanding the solution space.

Reported-by: qianfan Zhao <qianfanguijin@163.com>
Reported-by: Adriana Nicolae <adriana@arista.com>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dw.c | 159 +++++++++++++++++++++---------
 1 file changed, 115 insertions(+), 44 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index a53ba04d9770..8e25dfe8e653 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -42,6 +42,8 @@
 /* DesignWare specific register fields */
 #define DW_UART_MCR_SIRE		BIT(6)
 
+#define DW_UART_USR_BUSY		BIT(0)
+
 /* Renesas specific register fields */
 #define RZN1_UART_xDMACR_DMA_EN		BIT(0)
 #define RZN1_UART_xDMACR_1_WORD_BURST	(0 << 1)
@@ -77,6 +79,7 @@ struct dw8250_data {
 
 	unsigned int		skip_autocfg:1;
 	unsigned int		uart_16550_compatible:1;
+	unsigned int		in_idle:1;
 };
 
 static inline struct dw8250_data *to_dw8250_data(struct dw8250_port_data *data)
@@ -108,36 +111,103 @@ static inline u32 dw8250_modify_msr(struct uart_port *p, unsigned int offset, u3
 }
 
 /*
- * This function is being called as part of the uart_port::serial_out()
- * routine. Hence, it must not call serial_port_out() or serial_out()
- * against the modified registers here, i.e. LCR.
+ * Ensure BUSY is not asserted. If DW UART is configured with
+ * !uart_16550_compatible, the writes to LCR, DLL, and DLH fail while
+ * BUSY is asserted.
+ *
+ * Context: port's lock must be held
  */
-static void dw8250_force_idle(struct uart_port *p)
+static int dw8250_idle_enter(struct uart_port *p)
 {
+	struct dw8250_data *d = to_dw8250_data(p->private_data);
 	struct uart_8250_port *up = up_to_u8250p(p);
-	unsigned int lsr;
+	u32 lsr;
 
-	/*
-	 * The following call currently performs serial_out()
-	 * against the FCR register. Because it differs to LCR
-	 * there will be no infinite loop, but if it ever gets
-	 * modified, we might need a new custom version of it
-	 * that avoids infinite recursion.
-	 */
-	serial8250_clear_and_reinit_fifos(up);
+	if (d->uart_16550_compatible)
+		return 0;
+
+	d->in_idle = 1;
+
+	/* Prevent triggering interrupt from RBR filling */
+	p->serial_out(p, UART_IER, 0);
+
+	serial8250_rx_dma_flush(up);
+	// What about Tx DMA? Should probably pause that too and resume
+	// afterwards.
+
+	p->serial_out(p, UART_MCR, up->mcr | UART_MCR_LOOP);
+	if (up->capabilities & UART_CAP_FIFO)
+		p->serial_out(p, UART_FCR, 0);
+
+	if (p->serial_in(p, d->pdata->usr_reg) & DW_UART_USR_BUSY)
+		ndelay(p->frame_time);
+
+	lsr = serial_lsr_in(up);
+	if (lsr & UART_LSR_DR) {
+		p->serial_in(p, UART_RX);
+		up->lsr_saved_flags = 0;
+	}
 
 	/*
-	 * With PSLVERR_RESP_EN parameter set to 1, the device generates an
-	 * error response when an attempt to read an empty RBR with FIFO
-	 * enabled.
+	 * BUSY might still be frozen to asserted, try to de-freeze it by
+	 * sending a character over the loopback and receiving it.
 	 */
-	if (up->fcr & UART_FCR_ENABLE_FIFO) {
-		lsr = serial_port_in(p, UART_LSR);
-		if (!(lsr & UART_LSR_DR))
-			return;
+	if (p->serial_in(p, d->pdata->usr_reg) & DW_UART_USR_BUSY) {
+		p->serial_out(p, UART_TX, 0);
+		ndelay(p->frame_time);
+		lsr = serial_lsr_in(up);
+		if (lsr & UART_LSR_DR) {
+			p->serial_in(p, UART_RX);
+			up->lsr_saved_flags = 0;
+		}
 	}
 
-	serial_port_in(p, UART_RX);
+	/* Now guaranteed to have BUSY deasserted? Just sanity check */
+	if (p->serial_in(p, d->pdata->usr_reg) & DW_UART_USR_BUSY)
+		return -EBUSY;
+
+	return 0;
+}
+
+static void dw8250_idle_exit(struct uart_port *p)
+{
+	struct dw8250_data *d = to_dw8250_data(p->private_data);
+	struct uart_8250_port *up = up_to_u8250p(p);
+
+	if (d->uart_16550_compatible)
+		return;
+
+	if (up->capabilities & UART_CAP_FIFO)
+		p->serial_out(p, UART_FCR, up->fcr);
+	p->serial_out(p, UART_MCR, up->mcr);
+	p->serial_out(p, UART_IER, up->ier);
+
+	// Maybe move the DMA Rx restart check in dma_rx_complete() to own
+	// function (serial8250_rx_dma_restart()) and call it from here.
+	// DMA Tx resume
+
+	d->in_idle = 0;
+}
+
+static void dw8250_set_divisor(struct uart_port *p, unsigned int baud,
+			       unsigned int quot, unsigned int quot_frac)
+{
+	struct uart_8250_port *up = up_to_u8250p(p);
+	int ret;
+
+	ret = dw8250_idle_enter(p);
+	if (ret < 0)
+		goto idle_failed;
+
+	p->serial_out(p, UART_LCR, up->lcr | UART_LCR_DLAB);
+	if (!(p->serial_in(p, UART_LCR) & UART_LCR_DLAB))
+		goto idle_failed;
+
+	serial_dl_write(up, quot);
+	p->serial_out(p, UART_LCR, up->lcr);
+
+idle_failed:
+	dw8250_idle_exit(p);
 }
 
 /*
@@ -148,37 +218,37 @@ static void dw8250_force_idle(struct uart_port *p)
 static void dw8250_check_lcr(struct uart_port *p, unsigned int offset, u32 value)
 {
 	struct dw8250_data *d = to_dw8250_data(p->private_data);
-	void __iomem *addr = p->membase + (offset << p->regshift);
-	int tries = 1000;
+	u32 lcr = p->serial_in(p, UART_LCR);
+	int ret;
 
 	if (offset != UART_LCR || d->uart_16550_compatible)
 		return;
 
 	/* Make sure LCR write wasn't ignored */
-	while (tries--) {
-		u32 lcr = serial_port_in(p, offset);
-
-		if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
-			return;
+	if ((value & ~UART_LCR_SPAR) == (lcr & ~UART_LCR_SPAR))
+		return;
 
-		dw8250_force_idle(p);
+	if (d->in_idle) {
+		/*
+		 * FIXME: this deadlocks if port->lock is already held
+		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
+		 */
+		return;
+	}
 
-#ifdef CONFIG_64BIT
-		if (p->type == PORT_OCTEON)
-			__raw_writeq(value & 0xff, addr);
-		else
-#endif
-		if (p->iotype == UPIO_MEM32)
-			writel(value, addr);
-		else if (p->iotype == UPIO_MEM32BE)
-			iowrite32be(value, addr);
-		else
-			writeb(value, addr);
+	ret = dw8250_idle_enter(p);
+	if (ret < 0) {
+		/*
+		 * FIXME: this deadlocks if port->lock is already held
+		 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
+		 */
+		goto idle_failed;
 	}
-	/*
-	 * FIXME: this deadlocks if port->lock is already held
-	 * dev_err(p->dev, "Couldn't set LCR to %d\n", value);
-	 */
+
+	p->serial_out(p, UART_LCR, value);
+
+idle_failed:
+	dw8250_idle_exit(p);
 }
 
 /* Returns once the transmitter is empty or we run out of retries */
@@ -547,6 +617,7 @@ static int dw8250_probe(struct platform_device *pdev)
 	p->dev		= dev;
 	p->set_ldisc	= dw8250_set_ldisc;
 	p->set_termios	= dw8250_set_termios;
+	p->set_divisor	= dw8250_set_divisor;
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data)

base-commit: d21b26cad33250be758ea9d860ff9d5c3992c459
-- 
2.39.5