[PATCH v2 0/2] serial: 8250_dw: Add ti,tda54-uart support

[PATCH v2 0/2] serial: 8250_dw: Add ti,tda54-uart support

[Moteen Shah]

Add the ti,tda54-uart compatible string to the DW APB UART binding,
and add a capabililty to skip empty FIFO read on TDA54.

Reading an empty FIFO on the IP version used in TDA54 triggers a
data abort. The interrupt storm this dummy read was meant to avoid
no longer occurs on this version of the IP, so skip the read via a
new capability. Also, guard the empty FIFO reads during startup and
shutdown of the port using the same capability.

Changes since v1:
Link: https://lore.kernel.org/all/20260911092529.306082-1-m-shah@ti.com/

- Gate the empty FIFO read during startup and shutdown of the UART
port using the new capability.
- Make use of the new capability in the driver as to skip empty
FIFO read, replacing the quirk which was being used.

Moteen Shah (2):
  dt-bindings: serial: snps-dw-apb-uart: Add ti,tda54-uart
  serial: 8250_dw: Add capability to skip empty FIFO read

 .../bindings/serial/snps-dw-apb-uart.yaml        |  1 +
 drivers/tty/serial/8250/8250.h                   |  1 +
 drivers/tty/serial/8250/8250_dw.c                | 16 +++++++++++++++-
 drivers/tty/serial/8250/8250_port.c              | 12 +++++++++---
 4 files changed, 26 insertions(+), 4 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] dt-bindings: serial: snps-dw-apb-uart: Add ti,tda54-uart

[Moteen Shah]

The TDA54 UART is compatible with the existing DesignWare APB UART
binding. Add the ti,tda54-uart compatible string.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Signed-off-by: Udit Kumar <u-kumar1@ti.com>
Signed-off-by: Moteen Shah <m-shah@ti.com>
---
 Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
index 49f51b002879..518c899c52d1 100644
--- a/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
+++ b/Documentation/devicetree/bindings/serial/snps-dw-apb-uart.yaml
@@ -78,6 +78,7 @@ properties:
               - starfive,jh7100-hsuart
               - starfive,jh7100-uart
               - starfive,jh7110-uart
+              - ti,tda54-uart
               - ultrarisc,dp1000-uart
           - const: snps,dw-apb-uart
       - const: snps,dw-apb-uart
-- 
2.34.1

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

[Moteen Shah]

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) */
 
 #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;
 
 	/* 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))
+		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.
-- 
2.34.1

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

[Ilpo Järvinen]

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.

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

[Moteen Shah]

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

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@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).
> 

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

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

[Hari Prasath G E]

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