On Wed, 28 Jan 2026, Andy Shevchenko wrote: > On Wed, Jan 28, 2026 at 12:53:01PM +0200, Ilpo Järvinen wrote: > > 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 it those registers can always be written. > > > > There currently is dw8250_force_idle() which attempts to achieve > > 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. > > > > Ensure no Tx in ongoing while the UART is switches into the loopback > > mode (requires exporting serial8250_fifo_wait_for_lsr_thre() and adding > > DMA Tx pause/resume functions). > > > > According to tests performed by Adriana Nicolae , > > simply disabling FIFO or clearing FIFOs only once does not always > > ensure BUSY is deasserted but up to two tries may be needed. This could > > be related to ongoing Rx of a character (a guess, not known for sure). > > Therefore, retry FIFO clearing a few times (retry limit 4 is arbitrary > > number but using, e.g., p->fifosize seems overly large). Tests > > performed by others did not exhibit similar challenge but it does not > > seem harmful to leave the FIFO clearing loop in place for all DW UARTs > > with BUSY functionality. > > > > Use the new dw8250_idle_enter/exit() to do divisor writes and LCR > > writes. In case of plain LCR writes, opportunistically try to update > > LCR first and only invoke dw8250_idle_enter() if the write did not > > succeed (it has been observed that in practice most LCR writes do > > succeed without complications). > > > > This issue was first reported by qianfan Zhao who put lots of debugging > > effort into understanding the solution space. > > Some nit-picks below, otherwise seems good to go > Reviewed-by: Andy Shevchenko > > ... > > > Reported-by: qianfan Zhao > > Link: https://lore.kernel.org/linux-serial/289bb78a-7509-1c5c-2923-a04ed3b6487d@163.com/ > > Reported-by: Adriana Nicolae > > Link: https://lore.kernel.org/linux-serial/20250819182322.3451959-1-adriana@arista.com/ > > Shouldn't these Link:s be Closes: tags? To not possibly give wrong signals, until they confirm their cases are indeed solved by this patch, I'd like to keep these as Link tag only. > > + struct dw8250_data *d = to_dw8250_data(p->private_data); > > struct uart_8250_port *up = up_to_u8250p(p); > > + unsigned int usr_reg = DW_UART_USR; > > + int retries; > > + u32 lsr; > > > > + if (d->pdata) > > + usr_reg = d->pdata->usr_reg; > > I would unite this with definition above: > > unsigned int usr_reg = d->pdata ? d->pdata->usr_reg : DW_UART_USR; > > > + lsr = serial_lsr_in(up); > > > + if (lsr & UART_LSR_DR) { > > + serial_port_in(p, UART_RX); > > + up->lsr_saved_flags = 0; > > } > > This seems repeating a top of serial8250_read_char(). Perhaps we can do it > in a helper at some point? I don't see enough similarity as I'd need to deal with lsr_saved_flags somehow still here. > > + 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); > > + */ > > Does it make sense to print an error here (assuming it will work with nbcon)? > If so, maybe leave it at the end of the function, after dw8250_idle_exit() > and goto there? I think the print would be useful. I'll leave the FIXME+commented out print into the end of the function in v3. I also realized that on error, dw8250_idle_enter() should undo what it changed, that is, call dw8250_idle_exit() within which will simplify caller-side error handling slightly. -- i.