From 10237640b15ada313c3ac3021d7cc9aeb774d5c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= Date: Mon, 14 Apr 2025 14:23:36 +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 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 Signed-off-by: Ilpo Järvinen --- drivers/tty/serial/8250/8250_dw.c | 149 +++++++++++++++++++++--------- 1 file changed, 103 insertions(+), 46 deletions(-) diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c index 1902f29444a1..8a9dffc85fe3 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,89 @@ static inline int dw8250_modify_msr(struct uart_port *p, int offset, int value) } /* - * 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; - /* - * 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. - */ - if (up->fcr & UART_FCR_ENABLE_FIFO) { - lsr = serial_port_in(p, UART_LSR); - if (!(lsr & UART_LSR_DR)) - return; + 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; } - 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 +204,37 @@ static void dw8250_force_idle(struct uart_port *p) static void dw8250_check_lcr(struct uart_port *p, int offset, int value) { struct dw8250_data *d = to_dw8250_data(p->private_data); - void __iomem *addr = p->membase + (offset << p->regshift); - int tries = 1000; + unsigned int 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--) { - unsigned int 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 +603,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: 043806bc9dbc6597dd15e6ca9220ae2746425f2f -- 2.39.5