Re: serial: imx: sudden rx flood: hardware bug?

[Sergey Organov <sorganov@gmail.com>]

Hi Fabio,

Fabio Estevam <festevam@gmail.com> writes:

> Hi Sergey,
>
> On Thu, Dec 15, 2022 at 5:57 PM Sergey Organov <sorganov@gmail.com> wrote:
>
>> An effective method to reproduce the issue is to send isolated start bit
>> at baud rate that is about 2.4 times higher than the one configured on
>> the iMX UART while corresponding TTY is open on iMX. At these
>> conditions the problem appears with about 90% probability, i.e., about 9
>> out of 10 "sent" 0xff chars provoke continuous "receiving" of 0xff
>> chars by the UART, at intervals corresponding to the UART baud rate,
>
> I recall seeing this storm of receiving 0xff  before.
>
> I fixed it a long time ago with the following commit:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.1&id=b38cb7d2571197b56cefae8967f9db15c9361113

This patch looks correct to me, and it's there in my version as well.

> Looking at the current code, I see that the UCR3_ADNIMP bit is only
> set conditionally.

I don't see how it is relevant, as the bit is set on both ways of
corresponding condition, i.e., correctly set anyway.

>
> Could you try the change below?

This change looks wrong though, as the bit is now first set, and then is
cleared afterwards, see below.

Anyway, I checked that UCR3_ADNIMP bit is set in UCR3 register when the
problem appears in my case. Overall, I figure the problem is still there
even if UCR3_ADNIMP is set, and is only more difficult to reproduce.

>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 757825edb0cd..997681ec354f 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -2399,6 +2399,12 @@ static int imx_uart_probe(struct platform_device *pdev)
>                 imx_uart_writel(sport, ucr2, UCR2);
>         }
>
> +       if (!imx_uart_is_imx1(sport)) {
> +               u32 ucr3 = imx_uart_readl(sport, UCR3);
> +               ucr3 |= UCR3_ADNIMP;
> +               imx_uart_writel(sport, ucr2, UCR3);
> +       }
> +

Here we set the bit in UCR3.

>         if (!imx_uart_is_imx1(sport) && sport->dte_mode) {
>                 /*
>                  * The DCEDTE bit changes the direction of DSR, DCD, DTR and RI
> @@ -2416,8 +2422,7 @@ static int imx_uart_probe(struct platform_device *pdev)
>                  * (confirmed on i.MX25) which makes them unusable.
>                  */
>                 imx_uart_writel(sport,
> -                               IMX21_UCR3_RXDMUXSEL | UCR3_ADNIMP | UCR3_DSR,
> -                               UCR3);
> +                               IMX21_UCR3_RXDMUXSEL | UCR3_DSR, UCR3);

And here it's now cleared.

>
>         } else {
>                 u32 ucr3 = UCR3_DSR;
> @@ -2426,7 +2431,7 @@ static int imx_uart_probe(struct platform_device *pdev)
>                         imx_uart_writel(sport, ufcr & ~UFCR_DCEDTE, UFCR);
>
>                 if (!imx_uart_is_imx1(sport))
> -                       ucr3 |= IMX21_UCR3_RXDMUXSEL | UCR3_ADNIMP;
> +                       ucr3 |= IMX21_UCR3_RXDMUXSEL;
>                 imx_uart_writel(sport, ucr3, UCR3);

And here it's cleared as well, as we don't read-modify-write UCR3 here.

Thanks,
-- Sergey Organov