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

[Hari Prasath G E <gehariprasath@ti.com>]

On 9/16/2026 6:25 PM, Moteen Shah wrote:
> 
> 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
> 

It can be cap as you have done already, just name it to something else 
like "UART_CAP_NO_RXFIFO_EMPTY_READ" and address the review comments 
received from Ilpo.

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

yes that's easy and straightforward to do like...


>>>       /* 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,

Just add capabilities here as suggested by Ilpo above

  .capabilities = UART_CAP_NO_RXFIFO_EMPTY_READ,

Regards,
Hari

>>> +};
>>> +
>>>   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.
>>>