| Message ID | 20210519000704.3661773-2-andrew@aj.id.au (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | serial: 8250: Mitigate Tx stall risk for Aspeed VUARTs | expand |
On Wed, 19 May 2021 at 00:07, Andrew Jeffery <andrew@aj.id.au> wrote: > > Aspeed Virtual UARTs directly bridge e.g. the system console UART on the > LPC bus to the UART interface on the BMC's internal APB. As such there's > no RS-232 signalling involved - the UART interfaces on each bus are > directly connected as the producers and consumers of the one set of > FIFOs. > > The APB in the AST2600 generally runs at 100MHz while the LPC bus peaks > at 33MHz. The difference in clock speeds exposes a race in the VUART > design where a Tx data burst on the APB interface can result in a byte > lost on the LPC interface. The symptom is LSR[DR] remains clear on the > LPC interface despite data being present in its Rx FIFO, while LSR[THRE] > remains clear on the APB interface as the host has not consumed the data > the BMC has transmitted. In this state, the UART has stalled and no > further data can be transmitted without manual intervention (e.g. > resetting the FIFOs, resulting in loss of data). > > The recommended work-around is to insert a read cycle on the APB > interface between writes to THR. > > Cc: ChiaWei Wang <chiawei_wang@aspeedtech.com> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> Reviewed-by: Joel Stanley <joel@jms.id.au> > --- > drivers/tty/serial/8250/8250.h | 1 + > drivers/tty/serial/8250/8250_aspeed_vuart.c | 1 + > drivers/tty/serial/8250/8250_port.c | 10 ++++++++++ > 3 files changed, 12 insertions(+) > > diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h > index 52bb21205bb6..34aa2714f3c9 100644 > --- a/drivers/tty/serial/8250/8250.h > +++ b/drivers/tty/serial/8250/8250.h > @@ -88,6 +88,7 @@ struct serial8250_config { > #define UART_BUG_NOMSR (1 << 2) /* UART has buggy MSR status bits (Au1x00) */ > #define UART_BUG_THRE (1 << 3) /* UART has buggy THRE reassertion */ > #define UART_BUG_PARITY (1 << 4) /* UART mishandles parity if FIFO enabled */ > +#define UART_BUG_TXRACE (1 << 5) /* UART Tx fails to set remote DR */ > > > #ifdef CONFIG_SERIAL_8250_SHARE_IRQ > diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c > index a28a394ba32a..4caab8714e2c 100644 > --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c > +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c > @@ -440,6 +440,7 @@ static int aspeed_vuart_probe(struct platform_device *pdev) > port.port.status = UPSTAT_SYNC_FIFO; > port.port.dev = &pdev->dev; > port.port.has_sysrq = IS_ENABLED(CONFIG_SERIAL_8250_CONSOLE); > + port.bugs |= UART_BUG_TXRACE; A future enhancement would be to have this depend on the ast2600 compatible string, so we don't enable the feature for ast2400/ast2500. That would also mean adding a compatible string for the ast2600. Cheers, Joel
On Wed, 19 May 2021, at 10:28, Joel Stanley wrote: > On Wed, 19 May 2021 at 00:07, Andrew Jeffery <andrew@aj.id.au> wrote: > > > > Aspeed Virtual UARTs directly bridge e.g. the system console UART on the > > LPC bus to the UART interface on the BMC's internal APB. As such there's > > no RS-232 signalling involved - the UART interfaces on each bus are > > directly connected as the producers and consumers of the one set of > > FIFOs. > > > > The APB in the AST2600 generally runs at 100MHz while the LPC bus peaks > > at 33MHz. The difference in clock speeds exposes a race in the VUART > > design where a Tx data burst on the APB interface can result in a byte > > lost on the LPC interface. The symptom is LSR[DR] remains clear on the > > LPC interface despite data being present in its Rx FIFO, while LSR[THRE] > > remains clear on the APB interface as the host has not consumed the data > > the BMC has transmitted. In this state, the UART has stalled and no > > further data can be transmitted without manual intervention (e.g. > > resetting the FIFOs, resulting in loss of data). > > > > The recommended work-around is to insert a read cycle on the APB > > interface between writes to THR. > > > > Cc: ChiaWei Wang <chiawei_wang@aspeedtech.com> > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > Reviewed-by: Joel Stanley <joel@jms.id.au> > > > --- > > drivers/tty/serial/8250/8250.h | 1 + > > drivers/tty/serial/8250/8250_aspeed_vuart.c | 1 + > > drivers/tty/serial/8250/8250_port.c | 10 ++++++++++ > > 3 files changed, 12 insertions(+) > > > > diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h > > index 52bb21205bb6..34aa2714f3c9 100644 > > --- a/drivers/tty/serial/8250/8250.h > > +++ b/drivers/tty/serial/8250/8250.h > > @@ -88,6 +88,7 @@ struct serial8250_config { > > #define UART_BUG_NOMSR (1 << 2) /* UART has buggy MSR status bits (Au1x00) */ > > #define UART_BUG_THRE (1 << 3) /* UART has buggy THRE reassertion */ > > #define UART_BUG_PARITY (1 << 4) /* UART mishandles parity if FIFO enabled */ > > +#define UART_BUG_TXRACE (1 << 5) /* UART Tx fails to set remote DR */ > > > > > > #ifdef CONFIG_SERIAL_8250_SHARE_IRQ > > diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c > > index a28a394ba32a..4caab8714e2c 100644 > > --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c > > +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c > > @@ -440,6 +440,7 @@ static int aspeed_vuart_probe(struct platform_device *pdev) > > port.port.status = UPSTAT_SYNC_FIFO; > > port.port.dev = &pdev->dev; > > port.port.has_sysrq = IS_ENABLED(CONFIG_SERIAL_8250_CONSOLE); > > + port.bugs |= UART_BUG_TXRACE; > > A future enhancement would be to have this depend on the ast2600 > compatible string, so we don't enable the feature for ast2400/ast2500. > > That would also mean adding a compatible string for the ast2600. Yep, I'll sort out some cleanups in that regard in a separate series. Andrew
On 19. 05. 21, 2:07, Andrew Jeffery wrote: > Aspeed Virtual UARTs directly bridge e.g. the system console UART on the > LPC bus to the UART interface on the BMC's internal APB. As such there's > no RS-232 signalling involved - the UART interfaces on each bus are > directly connected as the producers and consumers of the one set of > FIFOs. > > The APB in the AST2600 generally runs at 100MHz while the LPC bus peaks > at 33MHz. The difference in clock speeds exposes a race in the VUART > design where a Tx data burst on the APB interface can result in a byte > lost on the LPC interface. The symptom is LSR[DR] remains clear on the > LPC interface despite data being present in its Rx FIFO, while LSR[THRE] > remains clear on the APB interface as the host has not consumed the data > the BMC has transmitted. In this state, the UART has stalled and no > further data can be transmitted without manual intervention (e.g. > resetting the FIFOs, resulting in loss of data). > > The recommended work-around is to insert a read cycle on the APB > interface between writes to THR. > > Cc: ChiaWei Wang <chiawei_wang@aspeedtech.com> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > --- > drivers/tty/serial/8250/8250.h | 1 + > drivers/tty/serial/8250/8250_aspeed_vuart.c | 1 + > drivers/tty/serial/8250/8250_port.c | 10 ++++++++++ > 3 files changed, 12 insertions(+) > > diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h > index 52bb21205bb6..34aa2714f3c9 100644 > --- a/drivers/tty/serial/8250/8250.h > +++ b/drivers/tty/serial/8250/8250.h > @@ -88,6 +88,7 @@ struct serial8250_config { > #define UART_BUG_NOMSR (1 << 2) /* UART has buggy MSR status bits (Au1x00) */ > #define UART_BUG_THRE (1 << 3) /* UART has buggy THRE reassertion */ > #define UART_BUG_PARITY (1 << 4) /* UART mishandles parity if FIFO enabled */ > +#define UART_BUG_TXRACE (1 << 5) /* UART Tx fails to set remote DR */ > > > #ifdef CONFIG_SERIAL_8250_SHARE_IRQ > diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c > index a28a394ba32a..4caab8714e2c 100644 > --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c > +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c > @@ -440,6 +440,7 @@ static int aspeed_vuart_probe(struct platform_device *pdev) > port.port.status = UPSTAT_SYNC_FIFO; > port.port.dev = &pdev->dev; > port.port.has_sysrq = IS_ENABLED(CONFIG_SERIAL_8250_CONSOLE); > + port.bugs |= UART_BUG_TXRACE; > > rc = sysfs_create_group(&vuart->dev->kobj, &aspeed_vuart_attr_group); > if (rc < 0) > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c > index d45dab1ab316..9d44b2b2ff18 100644 > --- a/drivers/tty/serial/8250/8250_port.c > +++ b/drivers/tty/serial/8250/8250_port.c > @@ -1809,6 +1809,16 @@ void serial8250_tx_chars(struct uart_8250_port *up) > count = up->tx_loadsz; > do { > serial_out(up, UART_TX, xmit->buf[xmit->tail]); > + if (up->bugs & UART_BUG_TXRACE) { > + /* The Aspeed BMC virtual UARTs have a bug where data This is not how a multiline comment should start. It should have been: /* * The Aspeed BMC virtual... > + * may get stuck in the BMC's Tx FIFO from bursts of > + * writes on the APB interface. > + * > + * Delay back-to-back writes by a read cycle to avoid > + * stalling the VUART. > + */ > + (void)serial_in(up, UART_SCR); (void) is useless here. It's only syntactic sugar which wouldn't even filter out a warning about unused result (if serial_in was marked w/ __must_check/warn_unused_result attribute). > + } > xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); > port->icount.tx++; > if (uart_circ_empty(xmit)) > thanks,
On Wed, 19 May 2021, at 15:40, Jiri Slaby wrote: > On 19. 05. 21, 2:07, Andrew Jeffery wrote: > > Aspeed Virtual UARTs directly bridge e.g. the system console UART on the > > LPC bus to the UART interface on the BMC's internal APB. As such there's > > no RS-232 signalling involved - the UART interfaces on each bus are > > directly connected as the producers and consumers of the one set of > > FIFOs. > > > > The APB in the AST2600 generally runs at 100MHz while the LPC bus peaks > > at 33MHz. The difference in clock speeds exposes a race in the VUART > > design where a Tx data burst on the APB interface can result in a byte > > lost on the LPC interface. The symptom is LSR[DR] remains clear on the > > LPC interface despite data being present in its Rx FIFO, while LSR[THRE] > > remains clear on the APB interface as the host has not consumed the data > > the BMC has transmitted. In this state, the UART has stalled and no > > further data can be transmitted without manual intervention (e.g. > > resetting the FIFOs, resulting in loss of data). > > > > The recommended work-around is to insert a read cycle on the APB > > interface between writes to THR. > > > > Cc: ChiaWei Wang <chiawei_wang@aspeedtech.com> > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au> > > --- > > drivers/tty/serial/8250/8250.h | 1 + > > drivers/tty/serial/8250/8250_aspeed_vuart.c | 1 + > > drivers/tty/serial/8250/8250_port.c | 10 ++++++++++ > > 3 files changed, 12 insertions(+) > > > > diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h > > index 52bb21205bb6..34aa2714f3c9 100644 > > --- a/drivers/tty/serial/8250/8250.h > > +++ b/drivers/tty/serial/8250/8250.h > > @@ -88,6 +88,7 @@ struct serial8250_config { > > #define UART_BUG_NOMSR (1 << 2) /* UART has buggy MSR status bits (Au1x00) */ > > #define UART_BUG_THRE (1 << 3) /* UART has buggy THRE reassertion */ > > #define UART_BUG_PARITY (1 << 4) /* UART mishandles parity if FIFO enabled */ > > +#define UART_BUG_TXRACE (1 << 5) /* UART Tx fails to set remote DR */ > > > > > > #ifdef CONFIG_SERIAL_8250_SHARE_IRQ > > diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c > > index a28a394ba32a..4caab8714e2c 100644 > > --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c > > +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c > > @@ -440,6 +440,7 @@ static int aspeed_vuart_probe(struct platform_device *pdev) > > port.port.status = UPSTAT_SYNC_FIFO; > > port.port.dev = &pdev->dev; > > port.port.has_sysrq = IS_ENABLED(CONFIG_SERIAL_8250_CONSOLE); > > + port.bugs |= UART_BUG_TXRACE; > > > > rc = sysfs_create_group(&vuart->dev->kobj, &aspeed_vuart_attr_group); > > if (rc < 0) > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c > > index d45dab1ab316..9d44b2b2ff18 100644 > > --- a/drivers/tty/serial/8250/8250_port.c > > +++ b/drivers/tty/serial/8250/8250_port.c > > @@ -1809,6 +1809,16 @@ void serial8250_tx_chars(struct uart_8250_port *up) > > count = up->tx_loadsz; > > do { > > serial_out(up, UART_TX, xmit->buf[xmit->tail]); > > + if (up->bugs & UART_BUG_TXRACE) { > > + /* The Aspeed BMC virtual UARTs have a bug where data > > This is not how a multiline comment should start. It should have been: > /* > * The Aspeed BMC virtual... Sure. > > > + * may get stuck in the BMC's Tx FIFO from bursts of > > + * writes on the APB interface. > > + * > > + * Delay back-to-back writes by a read cycle to avoid > > + * stalling the VUART. > > + */ > > + (void)serial_in(up, UART_SCR); > > (void) is useless here. It's only syntactic sugar which wouldn't even > filter out a warning about unused result (if serial_in was marked w/ > __must_check/warn_unused_result attribute). Yes. It was just meant as a visual indicator that we didn't care for the result. I will remove it. Thanks, Andrew
diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h index 52bb21205bb6..34aa2714f3c9 100644 --- a/drivers/tty/serial/8250/8250.h +++ b/drivers/tty/serial/8250/8250.h @@ -88,6 +88,7 @@ struct serial8250_config { #define UART_BUG_NOMSR (1 << 2) /* UART has buggy MSR status bits (Au1x00) */ #define UART_BUG_THRE (1 << 3) /* UART has buggy THRE reassertion */ #define UART_BUG_PARITY (1 << 4) /* UART mishandles parity if FIFO enabled */ +#define UART_BUG_TXRACE (1 << 5) /* UART Tx fails to set remote DR */ #ifdef CONFIG_SERIAL_8250_SHARE_IRQ diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c index a28a394ba32a..4caab8714e2c 100644 --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c @@ -440,6 +440,7 @@ static int aspeed_vuart_probe(struct platform_device *pdev) port.port.status = UPSTAT_SYNC_FIFO; port.port.dev = &pdev->dev; port.port.has_sysrq = IS_ENABLED(CONFIG_SERIAL_8250_CONSOLE); + port.bugs |= UART_BUG_TXRACE; rc = sysfs_create_group(&vuart->dev->kobj, &aspeed_vuart_attr_group); if (rc < 0) diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c index d45dab1ab316..9d44b2b2ff18 100644 --- a/drivers/tty/serial/8250/8250_port.c +++ b/drivers/tty/serial/8250/8250_port.c @@ -1809,6 +1809,16 @@ void serial8250_tx_chars(struct uart_8250_port *up) count = up->tx_loadsz; do { serial_out(up, UART_TX, xmit->buf[xmit->tail]); + if (up->bugs & UART_BUG_TXRACE) { + /* The Aspeed BMC virtual UARTs have a bug where data + * may get stuck in the BMC's Tx FIFO from bursts of + * writes on the APB interface. + * + * Delay back-to-back writes by a read cycle to avoid + * stalling the VUART. + */ + (void)serial_in(up, UART_SCR); + } xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1); port->icount.tx++; if (uart_circ_empty(xmit))
Aspeed Virtual UARTs directly bridge e.g. the system console UART on the LPC bus to the UART interface on the BMC's internal APB. As such there's no RS-232 signalling involved - the UART interfaces on each bus are directly connected as the producers and consumers of the one set of FIFOs. The APB in the AST2600 generally runs at 100MHz while the LPC bus peaks at 33MHz. The difference in clock speeds exposes a race in the VUART design where a Tx data burst on the APB interface can result in a byte lost on the LPC interface. The symptom is LSR[DR] remains clear on the LPC interface despite data being present in its Rx FIFO, while LSR[THRE] remains clear on the APB interface as the host has not consumed the data the BMC has transmitted. In this state, the UART has stalled and no further data can be transmitted without manual intervention (e.g. resetting the FIFOs, resulting in loss of data). The recommended work-around is to insert a read cycle on the APB interface between writes to THR. Cc: ChiaWei Wang <chiawei_wang@aspeedtech.com> Signed-off-by: Andrew Jeffery <andrew@aj.id.au> --- drivers/tty/serial/8250/8250.h | 1 + drivers/tty/serial/8250/8250_aspeed_vuart.c | 1 + drivers/tty/serial/8250/8250_port.c | 10 ++++++++++ 3 files changed, 12 insertions(+)