merged.
Bruce
In message: [linux-yocto][linux-yocto v5.15/standard/nxp-sdk-5.15/nxp-soc &
v5.15/standard/preempt-rt/nxp-sdk-5.15/nxp-soc][PATCH 2/2] serial: imx:
work-around for hardware RX flood
on 29/11/2023 Xiaolei Wang wrote:
> From: Sergey Organov <sorga...@gmail.com>
>
> commit 496a4471b7c3ae5c0be1a3fccd69e7debc127e08 from upstream.
>
> Check if hardware Rx flood is in progress, and issue soft reset to UART to
> stop the flood.
>
> A way to reproduce the flood (checked on iMX6SX) is: open iMX UART at 9600
> 8N1, and from external source send 0xf0 char at 115200 8N1. In about 90% of
> cases this starts a flood of "receiving" of 0xff characters by the iMX UART
> that is terminated by any activity on RxD line, or could be stopped by
> issuing soft reset to the UART (just stop/start of RX does not help). Note
> that in essence what we did here is sending isolated start bit about 2.4
> times shorter than it is to be if issued on the UART configured baud rate.
>
> There was earlier attempt to fix similar issue in: 'commit
> b38cb7d25711 ("serial: imx: Disable new features of autobaud detection")',
> but apparently it only gets harder to reproduce the issue after that
> commit.
>
> Signed-off-by: Sergey Organov <sorga...@gmail.com>
> Link: https://lore.kernel.org/r/20230201142700.4346-3-sorga...@gmail.com
> Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
> Signed-off-by: Xiaolei Wang <xiaolei.w...@windriver.com>
> ---
> drivers/tty/serial/imx.c | 123 ++++++++++++++++++++++++++++++---------
> 1 file changed, 95 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 8b32d8e549dc..001f88b15752 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -214,6 +214,9 @@ struct imx_port {
>
> struct mctrl_gpios *gpios;
>
> + /* counter to stop 0xff flood */
> + int idle_counter;
> +
> /* shadow registers */
> unsigned int ucr1;
> unsigned int ucr2;
> @@ -444,6 +447,8 @@ static void imx_uart_soft_reset(struct imx_port *sport)
> imx_uart_writel(sport, ubir, UBIR);
> imx_uart_writel(sport, ubmr, UBMR);
> imx_uart_writel(sport, uts, IMX21_UTS);
> +
> + sport->idle_counter = 0;
> }
>
> /* called with port.lock taken and irqs off */
> @@ -844,15 +849,66 @@ static irqreturn_t imx_uart_txint(int irq, void *dev_id)
> return IRQ_HANDLED;
> }
>
> +/* Check if hardware Rx flood is in progress, and issue soft reset to stop
> it.
> + * This is to be called from Rx ISRs only when some bytes were actually
> + * received.
> + *
> + * A way to reproduce the flood (checked on iMX6SX) is: open iMX UART at 9600
> + * 8N1, and from external source send 0xf0 char at 115200 8N1. In about 90%
> of
> + * cases this starts a flood of "receiving" of 0xff characters by the iMX6
> UART
> + * that is terminated by any activity on RxD line, or could be stopped by
> + * issuing soft reset to the UART (just stop/start of RX does not help). Note
> + * that what we do here is sending isolated start bit about 2.4 times shorter
> + * than it is to be on UART configured baud rate.
> + */
> +static void imx_uart_check_flood(struct imx_port *sport, u32 usr2)
> +{
> + /* To detect hardware 0xff flood we monitor RxD line between RX
> + * interrupts to isolate "receiving" of char(s) with no activity
> + * on RxD line, that'd never happen on actual data transfers.
> + *
> + * We use USR2_WAKE bit to check for activity on RxD line, but we have a
> + * race here if we clear USR2_WAKE when receiving of a char is in
> + * progress, so we might get RX interrupt later with USR2_WAKE bit
> + * cleared. Note though that as we don't try to clear USR2_WAKE when we
> + * detected no activity, this race may hide actual activity only once.
> + *
> + * Yet another case where receive interrupt may occur without RxD
> + * activity is expiration of aging timer, so we consider this as well.
> + *
> + * We use 'idle_counter' to ensure that we got at least so many RX
> + * interrupts without any detected activity on RxD line. 2 cases
> + * described plus 1 to be on the safe side gives us a margin of 3,
> + * below. In practice I was not able to produce a false positive to
> + * induce soft reset at regular data transfers even using 1 as the
> + * margin, so 3 is actually very strong.
> + *
> + * We count interrupts, not chars in 'idle-counter' for simplicity.
> + */
> +
> + if (usr2 & USR2_WAKE) {
> + imx_uart_writel(sport, USR2_WAKE, USR2);
> + sport->idle_counter = 0;
> + } else if (++sport->idle_counter > 3) {
> + dev_warn(sport->port.dev, "RX flood detected: soft reset.");
> + imx_uart_soft_reset(sport); /* also clears
> 'sport->idle_counter' */
> + }
> +}
> +
> static irqreturn_t __imx_uart_rxint(int irq, void *dev_id)
> {
> struct imx_port *sport = dev_id;
> unsigned int rx, flg, ignored = 0;
> struct tty_port *port = &sport->port.state->port;
> + u32 usr2;
> +
> + usr2 = imx_uart_readl(sport, USR2);
>
> - while (imx_uart_readl(sport, USR2) & USR2_RDR) {
> - u32 usr2;
> + /* If we received something, check for 0xff flood */
> + if (usr2 & USR2_RDR)
> + imx_uart_check_flood(sport, usr2);
>
> + for ( ; usr2 & USR2_RDR; usr2 = imx_uart_readl(sport, USR2)) {
> flg = TTY_NORMAL;
> sport->port.icount.rx++;
>
> @@ -1190,55 +1246,64 @@ static void imx_uart_dma_rx_callback(void *data)
> status = dmaengine_tx_status(chan, sport->rx_cookie, &state);
>
> if (status == DMA_ERROR) {
> + spin_lock(&sport->port.lock);
> imx_uart_clear_rx_errors(sport);
> + spin_unlock(&sport->port.lock);
> return;
> }
>
> - if (!(sport->port.ignore_status_mask & URXD_DUMMY_READ)) {
> + /*
> + * The state-residue variable represents the empty space
> + * relative to the entire buffer. Taking this in consideration
> + * the head is always calculated base on the buffer total
> + * length - DMA transaction residue. The UART script from the
> + * SDMA firmware will jump to the next buffer descriptor,
> + * once a DMA transaction if finalized (IMX53 RM - A.4.1.2.4).
> + * Taking this in consideration the tail is always at the
> + * beginning of the buffer descriptor that contains the head.
> + */
>
> - /*
> - * The state-residue variable represents the empty space
> - * relative to the entire buffer. Taking this in consideration
> - * the head is always calculated base on the buffer total
> - * length - DMA transaction residue. The UART script from the
> - * SDMA firmware will jump to the next buffer descriptor,
> - * once a DMA transaction if finalized (IMX53 RM - A.4.1.2.4).
> - * Taking this in consideration the tail is always at the
> - * beginning of the buffer descriptor that contains the head.
> - */
> + /* Calculate the head */
> + rx_ring->head = sg_dma_len(sgl) - state.residue;
>
> - /* Calculate the head */
> - rx_ring->head = sg_dma_len(sgl) - state.residue;
> + /* Calculate the tail. */
> + bd_size = sg_dma_len(sgl) / sport->rx_periods;
> + rx_ring->tail = ((rx_ring->head-1) / bd_size) * bd_size;
>
> - /* Calculate the tail. */
> - bd_size = sg_dma_len(sgl) / sport->rx_periods;
> - rx_ring->tail = ((rx_ring->head-1) / bd_size) * bd_size;
> + if (rx_ring->head <= sg_dma_len(sgl) &&
> + rx_ring->head > rx_ring->tail) {
>
> - if (rx_ring->head <= sg_dma_len(sgl) &&
> - rx_ring->head > rx_ring->tail) {
> + /* Move data from tail to head */
> + r_bytes = rx_ring->head - rx_ring->tail;
>
> - /* Move data from tail to head */
> - r_bytes = rx_ring->head - rx_ring->tail;
> + /* If we received something, check for 0xff flood */
> + if (r_bytes > 0) {
> + spin_lock(&sport->port.lock);
> + imx_uart_check_flood(sport, imx_uart_readl(sport,
> USR2));
> + spin_unlock(&sport->port.lock);
> + }
> +
> + if (!(sport->port.ignore_status_mask & URXD_DUMMY_READ)) {
>
> /* CPU claims ownership of RX DMA buffer */
> dma_sync_sg_for_cpu(sport->port.dev, sgl, 1,
> - DMA_FROM_DEVICE);
> + DMA_FROM_DEVICE);
>
> w_bytes = tty_insert_flip_string(port,
> - sport->rx_buf + rx_ring->tail, r_bytes);
> + sport->rx_buf +
> rx_ring->tail, r_bytes);
>
> /* UART retrieves ownership of RX DMA buffer */
> dma_sync_sg_for_device(sport->port.dev, sgl, 1,
> - DMA_FROM_DEVICE);
> + DMA_FROM_DEVICE);
>
> if (w_bytes != r_bytes)
> sport->port.icount.buf_overrun++;
>
> sport->port.icount.rx += w_bytes;
> - } else {
> - WARN_ON(rx_ring->head > sg_dma_len(sgl));
> - WARN_ON(rx_ring->head <= rx_ring->tail);
> }
> + } else {
> + WARN_ON(rx_ring->head > sg_dma_len(sgl));
> + WARN_ON(rx_ring->head <= rx_ring->tail);
> }
>
> if (w_bytes) {
> @@ -1314,6 +1379,8 @@ static void imx_uart_clear_rx_errors(struct imx_port
> *sport)
> imx_uart_writel(sport, USR2_ORE, USR2);
> }
>
> + sport->idle_counter = 0;
> +
> }
>
> #define TXTL_DEFAULT 2 /* reset default */
> --
> 2.25.1
>
-=-=-=-=-=-=-=-=-=-=-=-
Links: You receive all messages sent to this group.
View/Reply Online (#13331):
https://lists.yoctoproject.org/g/linux-yocto/message/13331
Mute This Topic: https://lists.yoctoproject.org/mt/102866413/21656
Group Owner: linux-yocto+ow...@lists.yoctoproject.org
Unsubscribe:
https://lists.yoctoproject.org/g/linux-yocto/leave/6687884/21656/624485779/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
