Re: [PATCH v2 2/2] serial: 8250_dw: Add capability to skip empty FIFO read
From: Hari Prasath G E
Date: Wed Sep 16 2026 - 10:38:07 EST
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 dataIMO, this define naming contradicts with the comment because you
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) */
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 */Wouldn't it be better that the extra caps would come from .data?
#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;
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
+};Is the logic correct way around? Ah, it's actually naming issue with the
+
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))
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.