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

[Moteen Shah <m-shah@ti.com>]

On 16/09/26 16:22, 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@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).

Hi all,
I am in a bit of conflict here regarding the capability, I dont think so 
this "cant read empty FIFO" falls under either UART_CAP_* or UART_BUG*, 
ideally its a quirk. Any opinions on the naming?

Regards,
Moteen

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