* [PATCH v2 0/2] serial: 8250_dw: Add ti,tda54-uart support @ 2026-09-16 8:55 Moteen Shah 2026-09-16 8:55 ` [PATCH v2 1/2] dt-bindings: serial: snps-dw-apb-uart: Add ti,tda54-uart Moteen Shah 2026-09-16 8:55 ` [PATCH v2 2/2] serial: 8250_dw: Add capability to skip empty FIFO read Moteen Shah 0 siblings, 2 replies; 8+ messages in thread From: Moteen Shah @ 2026-09-16 8:55 UTC (permalink / raw) To: ilpo.jarvinen, gregkh, krzk+dt, linux-serial, linux-kernel, jirislaby, devicetree Cc: u-kumar1, gehariprasath, m-shah, vigneshr, nm, a-limaye, y-abhilashchandra 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/2] dt-bindings: serial: snps-dw-apb-uart: Add ti,tda54-uart 2026-09-16 8:55 [PATCH v2 0/2] serial: 8250_dw: Add ti,tda54-uart support Moteen Shah @ 2026-09-16 8:55 ` Moteen Shah 2026-09-16 8:55 ` [PATCH v2 2/2] serial: 8250_dw: Add capability to skip empty FIFO read Moteen Shah 1 sibling, 0 replies; 8+ messages in thread From: Moteen Shah @ 2026-09-16 8:55 UTC (permalink / raw) To: ilpo.jarvinen, gregkh, krzk+dt, linux-serial, linux-kernel, jirislaby, devicetree Cc: u-kumar1, gehariprasath, m-shah, vigneshr, nm, a-limaye, y-abhilashchandra 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] serial: 8250_dw: Add capability to skip empty FIFO read 2026-09-16 8:55 [PATCH v2 0/2] serial: 8250_dw: Add ti,tda54-uart support Moteen Shah 2026-09-16 8:55 ` [PATCH v2 1/2] dt-bindings: serial: snps-dw-apb-uart: Add ti,tda54-uart Moteen Shah @ 2026-09-16 8:55 ` Moteen Shah 2026-09-16 10:52 ` Ilpo Järvinen 1 sibling, 1 reply; 8+ messages in thread From: Moteen Shah @ 2026-09-16 8:55 UTC (permalink / raw) To: ilpo.jarvinen, gregkh, krzk+dt, linux-serial, linux-kernel, jirislaby, devicetree Cc: u-kumar1, gehariprasath, m-shah, vigneshr, nm, a-limaye, y-abhilashchandra 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] serial: 8250_dw: Add capability to skip empty FIFO read 2026-09-16 8:55 ` [PATCH v2 2/2] serial: 8250_dw: Add capability to skip empty FIFO read Moteen Shah @ 2026-09-16 10:52 ` Ilpo Järvinen 2026-09-16 12:55 ` Moteen Shah 2026-09-16 13:47 ` Kumar, Udit 0 siblings, 2 replies; 8+ messages in thread From: Ilpo Järvinen @ 2026-09-16 10:52 UTC (permalink / raw) To: Moteen Shah Cc: Greg Kroah-Hartman, krzk+dt, linux-serial, LKML, Jiri Slaby, devicetree, u-kumar1, gehariprasath, vigneshr, nm, a-limaye, y-abhilashchandra 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] serial: 8250_dw: Add capability to skip empty FIFO read 2026-09-16 10:52 ` Ilpo Järvinen @ 2026-09-16 12:55 ` Moteen Shah 2026-09-16 14:32 ` Hari Prasath G E 2026-09-16 13:47 ` Kumar, Udit 1 sibling, 1 reply; 8+ messages in thread From: Moteen Shah @ 2026-09-16 12:55 UTC (permalink / raw) To: Ilpo Järvinen Cc: Greg Kroah-Hartman, krzk+dt, linux-serial, LKML, Jiri Slaby, devicetree, u-kumar1, gehariprasath, vigneshr, nm, a-limaye, y-abhilashchandra 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. >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] serial: 8250_dw: Add capability to skip empty FIFO read 2026-09-16 12:55 ` Moteen Shah @ 2026-09-16 14:32 ` Hari Prasath G E 0 siblings, 0 replies; 8+ messages in thread From: Hari Prasath G E @ 2026-09-16 14:32 UTC (permalink / raw) To: Moteen Shah, Ilpo Järvinen Cc: Greg Kroah-Hartman, krzk+dt, linux-serial, LKML, Jiri Slaby, devicetree, u-kumar1, vigneshr, nm, a-limaye, y-abhilashchandra 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. >>> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] serial: 8250_dw: Add capability to skip empty FIFO read 2026-09-16 10:52 ` Ilpo Järvinen 2026-09-16 12:55 ` Moteen Shah @ 2026-09-16 13:47 ` Kumar, Udit 2026-09-17 9:40 ` Ilpo Järvinen 1 sibling, 1 reply; 8+ messages in thread From: Kumar, Udit @ 2026-09-16 13:47 UTC (permalink / raw) To: Ilpo Järvinen, Moteen Shah Cc: Greg Kroah-Hartman, krzk+dt, linux-serial, LKML, Jiri Slaby, devicetree, gehariprasath, vigneshr, nm, a-limaye, y-abhilashchandra, u-kumar1 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. >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] serial: 8250_dw: Add capability to skip empty FIFO read 2026-09-16 13:47 ` Kumar, Udit @ 2026-09-17 9:40 ` Ilpo Järvinen 0 siblings, 0 replies; 8+ messages in thread From: Ilpo Järvinen @ 2026-09-17 9:40 UTC (permalink / raw) To: Kumar, Udit Cc: Moteen Shah, Greg Kroah-Hartman, krzk+dt, linux-serial, LKML, Jiri Slaby, devicetree, gehariprasath, vigneshr, nm, a-limaye, y-abhilashchandra [-- Attachment #1: Type: text/plain, Size: 2437 bytes --] On Wed, 16 Sep 2026, Kumar, Udit wrote: > 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. I now recalled we've also the pending RBR -> SRBR change: https://lore.kernel.org/linux-serial/20260908141524.20302-1-yang.yicong@picoheart.com/#t I wonder if a similar thing would help in this case as well? I understand this patch covers more ground than the SRBR patch but can the IP used in this case read SRBR without aborting? > 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? -- i. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-09-17 9:40 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-09-16 8:55 [PATCH v2 0/2] serial: 8250_dw: Add ti,tda54-uart support Moteen Shah 2026-09-16 8:55 ` [PATCH v2 1/2] dt-bindings: serial: snps-dw-apb-uart: Add ti,tda54-uart Moteen Shah 2026-09-16 8:55 ` [PATCH v2 2/2] serial: 8250_dw: Add capability to skip empty FIFO read Moteen Shah 2026-09-16 10:52 ` Ilpo Järvinen 2026-09-16 12:55 ` Moteen Shah 2026-09-16 14:32 ` Hari Prasath G E 2026-09-16 13:47 ` Kumar, Udit 2026-09-17 9:40 ` Ilpo Järvinen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®