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

[Kumar, Udit]

Thanks Ilpo,

On 9/16/2026 4:22 PM, Ilpo Järvinen wrote:
> 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@xxxxxx>
>> ---
>>  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).
> 

I echo your comments; the naming should reflect that the IP does not
allow reading an empty FIFO.

At the same time, I am thinking, reading the FIFO based on the
UART_LSR_DR bit being set in the serial8250_clear_interrupts and
serial8250_do_shutdown functions.
Do you see any side effects to this?


>>  
>>  #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.
>>
>