Re: [PATCH v2 7/7] serial: 8250_dw: Ensure BUSY is deasserted

[Andy Shevchenko]

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 <adriana@xxxxxxxxxx>,
> 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 <andriy.shevchenko@xxxxxxxxxxxxxxx>

...

> Reported-by: qianfan Zhao <qianfanguijin@xxxxxxx>
> Link: https://lore.kernel.org/linux-serial/289bb78a-7509-1c5c-2923-a04ed3b6487d@xxxxxxx/
> Reported-by: Adriana Nicolae <adriana@xxxxxxxxxx>
> Link: https://lore.kernel.org/linux-serial/20250819182322.3451959-1-adriana@xxxxxxxxxx/

Shouldn't these Link:s be Closes: tags?

...

> +	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?

...

> +	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?

> +		return;
> +	}

-- 
With Best Regards,
Andy Shevchenko