Re: [PATCH v2 2/2] serial: 8250_dw: Add capability to skip empty FIFO read

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

On Wed, 16 Sep 2026, Moteen Shah wrote:

> dw8250_handle_irq() does a bogus RX read on RX_TIMEOUT with no data
> present, to avoid an interrupt storm. The UART core also performs
> unconditional reads on the empty FIFO during startup and shutdown
> of the port. On the IP version used in TDA54, that interrupt storm
> no longer occurs, but reading an empty FIFO instead triggers a data
> abort.
> 
> Add a new capability to guard against the empty FIFO reads, avoiding
> the data aborts.
> 
> Signed-off-by: Moteen Shah <m-shah@ti.com>
> ---
>  drivers/tty/serial/8250/8250.h      |  1 +
>  drivers/tty/serial/8250/8250_dw.c   | 16 +++++++++++++++-
>  drivers/tty/serial/8250/8250_port.c | 12 +++++++++---
>  3 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
> index 77fe0588fd6b..45e13c3a8c14 100644
> --- a/drivers/tty/serial/8250/8250.h
> +++ b/drivers/tty/serial/8250/8250.h
> @@ -86,6 +86,7 @@ struct serial8250_config {
>  					 * STOP PARITY EPAR SPAR WLEN5 WLEN6
>  					 */
>  #define UART_CAP_NOTEMT	BIT(18)	/* UART without interrupt on TEMT available */
> +#define UART_CAP_RXFIFO_EMPTY_READ	BIT(19) /* UART needs LSR_DR check before RX read (TDA54) */

IMO, this define naming contradicts with the comment because you 
effectively say "capable of reading Rx while receive buffer is empty", not 
that it needs DR check before issuing that read on buffer (~ named exactly 
opposite of the actual meaning it is being used in the code).

>  
>  #define UART_BUG_QUOT	BIT(0)	/* UART has buggy quot LSB */
>  #define UART_BUG_TXEN	BIT(1)	/* UART has buggy TX IIR status */
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 5fba913f3301..325b620172a7 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -28,6 +28,7 @@
>  
>  #include <linux/serial_8250.h>
>  #include <linux/serial_reg.h>
> +#include <linux/of.h>
>  
>  #include "8250_dwlib.h"
>  
> @@ -436,7 +437,7 @@ static int dw8250_handle_irq(struct uart_port *p)
>  	 * This problem has only been observed so far when not in DMA mode
>  	 * so we limit the workaround only to non-DMA mode.
>  	 */
> -	if (!up->dma && rx_timeout) {
> +	if (!(up->capabilities & UART_CAP_RXFIFO_EMPTY_READ) && !up->dma && rx_timeout) {
>  		status = serial_lsr_in(up);
>  
>  		if (!(status & (UART_LSR_DR | UART_LSR_BI)))
> @@ -758,6 +759,14 @@ static int dw8250_probe(struct platform_device *pdev)
>  
>  	if (!data->skip_autocfg)
>  		dw8250_setup_port(p);
> +	/*
> +	 * On this IP, reading UART_RX while the FIFO is empty raises a data
> +	 * abort. serial8250_clear_interrupts() and serial8250_do_shutdown()
> +	 * in the 8250 core unconditionally read UART_RX, so guard those
> +	 * reads with an LSR_DR check.
> +	 */
> +	if (of_device_is_compatible(pdev->dev.of_node, "ti,tda54-uart"))
> +		up->capabilities |= UART_CAP_RXFIFO_EMPTY_READ;

Wouldn't it be better that the extra caps would come from .data?

>  	/* If we have a valid fifosize, try hooking up DMA */
>  	if (p->fifosize) {
> @@ -888,6 +897,10 @@ static const struct dw8250_platform_data dw8250_ultrarisc_dp1000_data = {
>  	.quirks = DW_UART_QUIRK_CPR_VALUE,
>  };
>  
> +static const struct dw8250_platform_data dw8250_tda54_data = {
> +	.usr_reg = DW_UART_USR,
> +};
> +
>  static const struct of_device_id dw8250_of_match[] = {
>  	{ .compatible = "snps,dw-apb-uart", .data = &dw8250_dw_apb },
>  	{ .compatible = "cavium,octeon-3860-uart", .data = &dw8250_octeon_3860_data },
> @@ -895,6 +908,7 @@ static const struct of_device_id dw8250_of_match[] = {
>  	{ .compatible = "renesas,rzn1-uart", .data = &dw8250_renesas_rzn1_data },
>  	{ .compatible = "sophgo,sg2044-uart", .data = &dw8250_skip_set_rate_data },
>  	{ .compatible = "starfive,jh7100-uart", .data = &dw8250_skip_set_rate_data },
> +	{ .compatible = "ti,tda54-uart", .data = &dw8250_tda54_data },
>  	{ .compatible = "ultrarisc,dp1000-uart", .data = &dw8250_ultrarisc_dp1000_data },
>  	{ /* Sentinel */ }
>  };
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index e94a0802cbdd..4435df88a1b1 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -704,8 +704,13 @@ static void serial8250_set_sleep(struct uart_8250_port *p, int sleep)
>  /* Clear the interrupt registers. */
>  static void serial8250_clear_interrupts(struct uart_port *port)
>  {
> -	serial_port_in(port, UART_LSR);
> -	serial_port_in(port, UART_RX);
> +	struct uart_8250_port *up = up_to_u8250p(port);
> +	unsigned int lsr;
> +
> +	lsr = serial_port_in(port, UART_LSR);
> +	if (!(up->capabilities & UART_CAP_RXFIFO_EMPTY_READ) || (lsr & UART_LSR_DR))

Is the logic correct way around? Ah, it's actually naming issue with the 
define (see above).

> +		serial_port_in(port, UART_RX);
> +
>  	serial_port_in(port, UART_IIR);
>  	serial_port_in(port, UART_MSR);
>  }
> @@ -2421,7 +2426,8 @@ void serial8250_do_shutdown(struct uart_port *port)
>  	 * Read data port to reset things, and then unlink from
>  	 * the IRQ chain.
>  	 */
> -	serial_port_in(port, UART_RX);
> +	if (!(up->capabilities & UART_CAP_RXFIFO_EMPTY_READ) || (serial_lsr_in(up) & UART_LSR_DR))
> +		serial_port_in(port, UART_RX);
>  	/*
>  	 * LCR writes on DW UART can trigger late (unmaskable) IRQs.
>  	 * Handle them before releasing the handler.
> 

-- 
 i.