Re: [PATCH v5 2/2] serial: mxc: have putc use the TXFIFO

["Pali Rohár" <pali@kernel.org>]

FYI, in past I was debugging issue with very similar symptoms:
https://lore.kernel.org/u-boot/20220623121356.4149-1-pali@kernel.org/t/#u

So if in your case the issue is also that u-boot drops TX buffer then
you just need to put in mxc driver at correct places (all!) code which
waits until all data are transmitted. But as I do not have this hardware
I'm just guessing where are those places.

Btw, it is possible that some other board code or other parts of u-boot
touches mxc registers, so maybe mxc serial driver is not the only
"affected" place.

If you have some more powerful HW debugger you could probably add
watchpoint on uart registers and monitor when u-boot tries to write
here. But this setup is probably rare...

On Thursday 03 November 2022 08:35:27 TERTYCHNYI Grygorii wrote:
> I think Pali is right,
> 
> mxc_serial_setbrg() is called _after_ board_init(), and because setbrg()
> touches bmr and cr registers it causes loosing of FIFO - even for the
> very long string, "only" last 32 chars are potentially corrupted.
> 
> Here I printed bmr value before and after (looks like setbrg() is called twice after board_init):
> ==========================================
> U-Boot SPL 2022.10-dirty (Nov 03 2022 - 09:21:16 +0100)
> No pmic
> SEC0:  RNG instantiated
> Normal Boot
> WDT:   Started watchdog@30280000 with servicing (60s timeout)
> Trying to boot from MMC1
> NOTICE:  BL31: v2.2(release):rel_imx_5.4.47_2.2.0-0-gc949a888e909
> NOTICE:  BL31: Built : 15:21:46, Nov  2 2022
> 
> before: 0x0
> after:  0x68
> 
> before: 0x68
> after:  0x68
> 
> 
> U-Boot 2022.10-dirty (Nov 03 2022 - 09:21:16 +0100)
> 
> CPU:   Freescale i.MX8MMQ rev1.0 at 1200 MHz
> Reset cause: POR
> Model: FSL i.MX8MM EVK board
> DRAM:  2 GiB
> 1:12345678901234567890123456789012:FIFO*****+
> 2:12345678901234567890123456789012:FIFO*****+
> 3:12345678901234567890123456789012:FIFO*****+
> 4:12345678901234567890123456789012:FIFO
> before: 0x0
> after:  0x68
> 
> before: 0x68
> after:  0x68
> Core:  160 devices, 20 uclasses, devicetree: separate
> WDT:   Started watchdog@30280000 with servicing (60s timeout)
> MMC:   FSL_SDHC: 1, FSL_SDHC: 2
> Loading Environment from MMC... *** Warning - bad CRC, using default environment
> 
> In:    serial@30890000
> Out:   serial@30890000
> Err:   serial@30890000
> SEC0:  RNG instantiated
> Net:   eth0: ethernet@30be0000
> Hit any key to stop autoboot:  0 
> 
> ==========================================
> 
> diff --git a/board/freescale/imx8mm_evk/imx8mm_evk.c b/board/freescale/imx8mm_evk/imx8mm_evk.c
> index e0975fcda705..837f1286b4e8 100644
> --- a/board/freescale/imx8mm_evk/imx8mm_evk.c
> +++ b/board/freescale/imx8mm_evk/imx8mm_evk.c
> @@ -50,6 +50,11 @@ int board_init(void)
>         if (IS_ENABLED(CONFIG_FEC_MXC))
>                 setup_fec();
>  
> +       puts("1:12345678901234567890123456789012:FIFO*****+\n");
> +       puts("2:12345678901234567890123456789012:FIFO*****+\n");
> +       puts("3:12345678901234567890123456789012:FIFO*****+\n");
> +       puts("4:12345678901234567890123456789012:FIFO*****+\n");
> +
>         return 0;
>  }
>  
> diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
> index af1fd1ea9bc7..daadd0605123 100644
> --- a/drivers/serial/serial_mxc.c
> +++ b/drivers/serial/serial_mxc.c
> @@ -164,10 +164,25 @@ static void _mxc_serial_init(struct mxc_uart *base, int use_dte)
>         writel(0, &base->ts);
>  }
>  
> +static void print_str(struct mxc_uart *base, const char *str)
> +{
> +    while (*str) {
> +        writel(*str, &base->txd);
> +               while (!(readl(&base->ts) & UTS_TXEMPTY)) {
> +                       ; /* wait */
> +        }
> +        ++str;
> +    }
> +}
> +
>  static void _mxc_serial_setbrg(struct mxc_uart *base, unsigned long clk,
>                                unsigned long baudrate, bool use_dte)
>  {
>         u32 tmp;
> +    u32 bmr;
> +    char bmrstr[16];
> +
> +    bmr = readl(&base->bmr);
>  
>         tmp = RFDIV << UFCR_RFDIV_SHF;
>         if (use_dte)
> @@ -190,6 +205,15 @@ static void _mxc_serial_setbrg(struct mxc_uart *base, unsigned long clk,
>         writel(readl(&base->cr3) | UCR3_RXDMUXSEL, &base->cr3);
>  
>         writel(UCR1_UARTEN, &base->cr1);
> +
> +    sprintf(bmrstr, "0x%x", bmr);
> +    print_str(base, "\nbefore: ");
> +    print_str(base, bmrstr);
> +
> +    sprintf(bmrstr, "0x%x", readl(&base->bmr));
> +    print_str(base, "\nafter:  ");
> +    print_str(base, bmrstr);
> +    print_str(base, "\n");
>  }
>  
>  #if !CONFIG_IS_ENABLED(DM_SERIAL)
> 
> 
> 
> If board_init() flushes FIFO, everything works. Unfortunately, it means,
> FIFO cannot be used unless CONFIG_SERIAL_PUTS is enabled?
> 
> diff --git a/board/freescale/imx8mm_evk/imx8mm_evk.c b/board/freescale/imx8mm_evk/imx8mm_evk.c
> index e0975fcda705..1c50db7789ec 100644
> --- a/board/freescale/imx8mm_evk/imx8mm_evk.c
> +++ b/board/freescale/imx8mm_evk/imx8mm_evk.c
> @@ -50,6 +50,12 @@ int board_init(void)
>         if (IS_ENABLED(CONFIG_FEC_MXC))
>                 setup_fec();
>  
> +       puts("12345678901234567890123456789012:");
> +       puts("12345678901234567890123456789012:");
> +       puts("12345678901234567890123456789012:");
> +       puts("12345678901234567890123456789012:");
> +       puts("%");
> +
>         return 0;
>  }
>  
> diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
> index af1fd1ea9bc7..0e85ecaae9c0 100644
> --- a/drivers/serial/serial_mxc.c
> +++ b/drivers/serial/serial_mxc.c
> @@ -316,6 +316,13 @@ static int mxc_serial_putc(struct udevice *dev, const char ch)
>  
>         writel(ch, &uart->txd);
>  
> +       if (ch == '%') {
> +               /* flush FIFO */
> +               while (!(readl(&uart->ts) & UTS_TXEMPTY)) {
> +                       ; /* wait */
> +               }
> +       }
> +
>         return 0;
>  }
> 
> Regards,
> Grygorii
> ________________________________________
> From: SCHNEIDER Johannes <johannes.schneider@leica-geosystems.com>
> Sent: Thursday, November 3, 2022 07:17
> To: Fabio Estevam; Pali Rohár
> Cc: Fabio Estevam; Tim Harvey; u-boot@lists.denx.de; peng.fan@oss.nxp.com; sbabic@denx.de; trini; GEO-CHHER-bsp-development; Peng Fan
> Subject: Re: [PATCH v5 2/2] serial: mxc: have putc use the TXFIFO
> 
> Hi all,
> 
> flushing and waiting... maybe you're onto something: what if one printf races another since it thinks considers its buffer handed to the FIFO as "done" a bit too soon?
> 
> might the below variation on "waiting for the fifo" solve the symptoms on imx6?
> 
> regards
> Johannes
> 
> 
> diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
> index 4207650503..dfd7670f7e 100644
> --- a/drivers/serial/serial_mxc.c
> +++ b/drivers/serial/serial_mxc.c
> @@ -329,8 +329,23 @@ static int mxc_serial_pending(struct udevice *dev, bool input)
>                 return sr2 & USR2_TXDC ? 0 : 1;
>  }
> 
> +static ssize_t mxc_serial_puts(struct udevice *dev, const char *s, size_t len)
> +{
> +       struct mxc_serial_plat *plat = dev_get_plat(dev);
> +       struct mxc_uart *const uart = plat->reg;
> +
> +       while (*s)
> +               mcx_serial_putc(dev, *s++);
> +
> +       // flush the TXFIFO
> +       while(mxc_serial_pending(dev, false));
> +
> +       return len;
> +}
> +
>  static const struct dm_serial_ops mxc_serial_ops = {
>         .putc = mxc_serial_putc,
> +       .puts = mxc_serial_puts,
>         .pending = mxc_serial_pending,
>         .getc = mxc_serial_getc,
>         .setbrg = mxc_serial_setbrg,
> 
> 
> 
> 
> 
> 
> ________________________________________
> From: Fabio Estevam <festevam@gmail.com>
> Sent: Wednesday, November 2, 2022 19:37
> To: Pali Rohár
> Cc: Fabio Estevam; SCHNEIDER Johannes; Tim Harvey; u-boot@lists.denx.de; peng.fan@oss.nxp.com; sbabic@denx.de; trini; GEO-CHHER-bsp-development; Peng Fan
> Subject: Re: [PATCH v5 2/2] serial: mxc: have putc use the TXFIFO
> 
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
> 
> 
> On Wed, Nov 2, 2022 at 2:24 PM Pali Rohár <pali@kernel.org> wrote:
> 
> > Hello! This log just cleanly shows that UART TX FIFO is either broken or
> > something drops its content prior all bytes are properly transmitted.
> > Dropping HW TX FIFO is on most UARTs possible by resetting registers or
> > reconfiguring buadrate.
> >
> > I have an idea, cannot some u-boot code calls some mxc function which
> > changes parameters of UART and this will cause loosing of FIFO?
> >
> > For example I see two functions which are doing it. Could you try to add
> > code which waits until content of FIFO is transmitted prior changing
> > UART parameters?
> >
> > diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
> > index 4cf79c1ca24f..9611d9bc8a00 100644
> > --- a/drivers/serial/serial_mxc.c
> > +++ b/drivers/serial/serial_mxc.c
> > @@ -146,6 +146,9 @@ struct mxc_uart {
> >
> >  static void _mxc_serial_init(struct mxc_uart *base, int use_dte)
> >  {
> > +       while (!(readl(&base->ts) & UTS_TXEMPTY))
> > +               ;
> > +
> >         writel(0, &base->cr1);
> >         writel(0, &base->cr2);
> >
> > @@ -169,6 +172,9 @@ static void _mxc_serial_setbrg(struct mxc_uart *base, unsigned long clk,
> >  {
> >         u32 tmp;
> >
> > +       while (!(readl(&base->ts) & UTS_TXEMPTY))
> > +               ;
> > +
> 
> Tried your suggestion, but the print() content I added inside
> board_init() is no longer printed.