[PATCH v5 0/2] serial_mxc: fixing reception

[PATCH v5 0/2] serial_mxc: fixing reception

[Johannes Schneider]

while writing to a imx-serial device is probably thoroughly tested - and obviosly works for the debug-serial - using a serial driver to read data received over the serial interface does not work reliably.

the patches in this series address issues found during the implementation of a custom uboot-command to query a coprocessor, connected to an imx8mm over uart4, for mainboard-identification strings

Changes in v5:
- fix multilne-comment format
- add another 'reviewed-by'

Changes in v4:
- add 'reviewd-by'

Changes in v3:
- more verbose commit messages

Changes in v2:
- manually fix 'to' and 'cc'
- fix comment delimiter

Johannes Schneider (2):
  serial: mxc: enable the RX pipeline
  serial: mxc: have putc use the TXFIFO

 drivers/serial/serial_mxc.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH v5 1/2] serial: mxc: enable the RX pipeline

[Johannes Schneider]

on imx8(mm) the RXDMUXSEL needs to be set for data going over the wire
(as observable on a connected 'scope) to actually make it into the
RXFIFO

the reference manual is not overly clear about this, and only
mentiones that "UCR3_RXDMUXSEL should always be set." - and since the
CR3 register reverts to its reset values after setting the baudrate,
setting this bit is done during '_mxc_serial_setbgr'

Signed-off-by: Johannes Schneider <johannes.schneider@leica-geosystems.com>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
Reviewed-by: Fabio Estevam <festevam@denx.de>

---

Changes in v5:
- fix multilne-comment format
- add another 'reviewed-by'

Changes in v4:
- add 'reviewd-by'

Changes in v3:
- more verbose commit messages

Changes in v2:
- manually fix 'to' and 'cc'
- fix comment delimiter

 drivers/serial/serial_mxc.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
index 70a0e5e919..ee17a960d4 100644
--- a/drivers/serial/serial_mxc.c
+++ b/drivers/serial/serial_mxc.c
@@ -61,6 +61,11 @@
 #define UCR3_AWAKEN	(1<<4)  /* Async wake interrupt enable */
 #define UCR3_REF25	(1<<3)  /* Ref freq 25 MHz */
 #define UCR3_REF30	(1<<2)  /* Ref Freq 30 MHz */
+
+/* imx8 names these bitsfields instead: */
+#define UCR3_DTRDEN	BIT(3)  /* bit not used in this chip */
+#define UCR3_RXDMUXSEL	BIT(2)  /* RXD muxed input selected; 'should always be set' */
+
 #define UCR3_INVT	(1<<1)  /* Inverted Infrared transmission */
 #define UCR3_BPEN	(1<<0)  /* Preset registers enable */
 #define UCR4_CTSTL_32	(32<<10) /* CTS trigger level (32 chars) */
@@ -176,6 +181,14 @@ static void _mxc_serial_setbrg(struct mxc_uart *base, unsigned long clk,
 
 	writel(UCR2_WS | UCR2_IRTS | UCR2_RXEN | UCR2_TXEN | UCR2_SRST,
 	       &base->cr2);
+
+	/*
+	 * setting the baudrate triggers a reset, returning cr3 to its
+	 * reset value but UCR3_RXDMUXSEL "should always be set."
+	 * according to the imx8 reference-manual
+	 */
+	writel(readl(&base->cr3) | UCR3_RXDMUXSEL, &base->cr3);
+
 	writel(UCR1_UARTEN, &base->cr1);
 }
 
-- 
2.25.1

[PATCH v5 1/2] serial: mxc: enable the RX pipeline

[sbabic]

> on imx8(mm) the RXDMUXSEL needs to be set for data going over the wire
> (as observable on a connected 'scope) to actually make it into the
> RXFIFO
> the reference manual is not overly clear about this, and only
> mentiones that "UCR3_RXDMUXSEL should always be set." - and since the
> CR3 register reverts to its reset values after setting the baudrate,
> setting this bit is done during '_mxc_serial_setbgr'
> Signed-off-by: Johannes Schneider <johannes.schneider@leica-geosystems.com>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> Reviewed-by: Fabio Estevam <festevam@denx.de>
Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
=====================================================================

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

[Johannes Schneider]

only waiting for TXEMPTY leads to corrupted messages going over the
wire - which is fixed by making use of the FIFO

this change is following the linux kernel uart driver
(drivers/tty/serial/imx.c), which also checks UTS_TXFULL
instead of UTS_TXEMPTY

Signed-off-by: Johannes Schneider <johannes.schneider@leica-geosystems.com>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
Reviewed-by: Fabio Estevam <festevam@denx.de>
---

(no changes since v1)

 drivers/serial/serial_mxc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
index ee17a960d4..af1fd1ea9b 100644
--- a/drivers/serial/serial_mxc.c
+++ b/drivers/serial/serial_mxc.c
@@ -311,7 +311,7 @@ static int mxc_serial_putc(struct udevice *dev, const char ch)
 	struct mxc_serial_plat *plat = dev_get_plat(dev);
 	struct mxc_uart *const uart = plat->reg;
 
-	if (!(readl(&uart->ts) & UTS_TXEMPTY))
+	if (readl(&uart->ts) & UTS_TXFULL)
 		return -EAGAIN;
 
 	writel(ch, &uart->txd);
-- 
2.25.1

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

[sbabic]

> only waiting for TXEMPTY leads to corrupted messages going over the
> wire - which is fixed by making use of the FIFO
> this change is following the linux kernel uart driver
> (drivers/tty/serial/imx.c), which also checks UTS_TXFULL
> instead of UTS_TXEMPTY
> Signed-off-by: Johannes Schneider <johannes.schneider@leica-geosystems.com>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> Reviewed-by: Fabio Estevam <festevam@denx.de>
Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
=====================================================================

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

[Tim Harvey]

On Tue, Sep 6, 2022 at 5:15 AM Johannes Schneider
<johannes.schneider@leica-geosystems.com> wrote:
>
> only waiting for TXEMPTY leads to corrupted messages going over the
> wire - which is fixed by making use of the FIFO
>
> this change is following the linux kernel uart driver
> (drivers/tty/serial/imx.c), which also checks UTS_TXFULL
> instead of UTS_TXEMPTY
>
> Signed-off-by: Johannes Schneider <johannes.schneider@leica-geosystems.com>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> Reviewed-by: Fabio Estevam <festevam@denx.de>
> ---
>
> (no changes since v1)
>
>  drivers/serial/serial_mxc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
> index ee17a960d4..af1fd1ea9b 100644
> --- a/drivers/serial/serial_mxc.c
> +++ b/drivers/serial/serial_mxc.c
> @@ -311,7 +311,7 @@ static int mxc_serial_putc(struct udevice *dev, const char ch)
>         struct mxc_serial_plat *plat = dev_get_plat(dev);
>         struct mxc_uart *const uart = plat->reg;
>
> -       if (!(readl(&uart->ts) & UTS_TXEMPTY))
> +       if (readl(&uart->ts) & UTS_TXFULL)
>                 return -EAGAIN;
>
>         writel(ch, &uart->txd);
> --
> 2.25.1
>

Johannes,

Since this patch I find an issue with an IMX6 board of mine gwventana:

Prior to this patch the board boots with:
DRAM:  1 GiB
GSCv2   : v52 0x9981 RST:VIN WDT:disabled board_temp:43C
RTC     : 1970-01-01   0:56:15 UTC
Core:  67 devices, 22 uclasses, devicetree: separate
WDT:   Started watchdog@20bc000 with servicing every 1000ms (60s timeout)
NAND:  2048 MiB
...

and following this patch I get:
...
DRAM:  1 GiB
GSCv2   : v52 0x9981 RST:VIN WDT:disabled board_temp:29C
RTC     : 1970-01-01   ~�KW�'$H�$V�W��Y.KH�� uclasses, devicetree: separate
WDT:   Started watchdog@20bc000 with servicing every 1000ms (60s timeout)
NAND:  2048 MiB
...

The RTC line is displayed from drivers/misc/gsc.c and the Core: comes
from dm_announce. Somehow in between the FIFO does not get drained
before dm_announce gets called.

Adding a delay after the RTC print or reverting this patch.

Any ideas?

Best Regards,

Tim

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

[Pali Rohár]

On Tuesday 25 October 2022 12:18:56 Tim Harvey wrote:
> On Tue, Sep 6, 2022 at 5:15 AM Johannes Schneider
> <johannes.schneider@leica-geosystems.com> wrote:
> >
> > only waiting for TXEMPTY leads to corrupted messages going over the
> > wire - which is fixed by making use of the FIFO
> >
> > this change is following the linux kernel uart driver
> > (drivers/tty/serial/imx.c), which also checks UTS_TXFULL
> > instead of UTS_TXEMPTY
> >
> > Signed-off-by: Johannes Schneider <johannes.schneider@leica-geosystems.com>
> > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > Reviewed-by: Fabio Estevam <festevam@denx.de>
> > ---
> >
> > (no changes since v1)
> >
> >  drivers/serial/serial_mxc.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/serial/serial_mxc.c b/drivers/serial/serial_mxc.c
> > index ee17a960d4..af1fd1ea9b 100644
> > --- a/drivers/serial/serial_mxc.c
> > +++ b/drivers/serial/serial_mxc.c
> > @@ -311,7 +311,7 @@ static int mxc_serial_putc(struct udevice *dev, const char ch)
> >         struct mxc_serial_plat *plat = dev_get_plat(dev);
> >         struct mxc_uart *const uart = plat->reg;
> >
> > -       if (!(readl(&uart->ts) & UTS_TXEMPTY))
> > +       if (readl(&uart->ts) & UTS_TXFULL)
> >                 return -EAGAIN;
> >
> >         writel(ch, &uart->txd);
> > --
> > 2.25.1
> >
> 
> Johannes,
> 
> Since this patch I find an issue with an IMX6 board of mine gwventana:
> 
> Prior to this patch the board boots with:
> DRAM:  1 GiB
> GSCv2   : v52 0x9981 RST:VIN WDT:disabled board_temp:43C
> RTC     : 1970-01-01   0:56:15 UTC
> Core:  67 devices, 22 uclasses, devicetree: separate
> WDT:   Started watchdog@20bc000 with servicing every 1000ms (60s timeout)
> NAND:  2048 MiB
> ...
> 
> and following this patch I get:
> ...
> DRAM:  1 GiB
> GSCv2   : v52 0x9981 RST:VIN WDT:disabled board_temp:29C
> RTC     : 1970-01-01   ~�KW�'$H�$V�W��Y.KH�� uclasses, devicetree: separate
> WDT:   Started watchdog@20bc000 with servicing every 1000ms (60s timeout)
> NAND:  2048 MiB
> ...
> 
> The RTC line is displayed from drivers/misc/gsc.c and the Core: comes
> from dm_announce. Somehow in between the FIFO does not get drained
> before dm_announce gets called.
> 
> Adding a delay after the RTC print or reverting this patch.
> 
> Any ideas?
> 
> Best Regards,
> 
> Tim

Hello! I do not have any MXC hardware but I see there one issue.
mxc_serial_putc() function probably should not return -EAGAIN when
device is busy. But instead it should wait until it is ready.

Could you try to change code to following?

    while (readl(&uart->ts) & UTS_TXFULL)
        ;

    writel(ch, &uart->txd);

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

[Fabio Estevam]

Hi Pali,

On 25/10/2022 17:23, Pali Rohár wrote:

> Hello! I do not have any MXC hardware but I see there one issue.
> mxc_serial_putc() function probably should not return -EAGAIN when
> device is busy. But instead it should wait until it is ready.
> 
> Could you try to change code to following?
> 
>     while (readl(&uart->ts) & UTS_TXFULL)
>         ;
> 
>     writel(ch, &uart->txd);

Your analysis looks correct.

The kernel does like this:

static void imx_uart_console_putchar(struct uart_port *port, unsigned 
char ch)
{
	struct imx_port *sport = (struct imx_port *)port;

	while (imx_uart_readl(sport, imx_uart_uts_reg(sport)) & UTS_TXFULL)
		barrier();

	imx_uart_writel(sport, ch, URTX0);
}

Thanks

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

[Tim Harvey]

On Tue, Oct 25, 2022 at 1:37 PM Fabio Estevam <festevam@denx.de> wrote:
>
> Hi Pali,
>
> On 25/10/2022 17:23, Pali Rohár wrote:
>
> > Hello! I do not have any MXC hardware but I see there one issue.
> > mxc_serial_putc() function probably should not return -EAGAIN when
> > device is busy. But instead it should wait until it is ready.
> >
> > Could you try to change code to following?
> >
> >     while (readl(&uart->ts) & UTS_TXFULL)
> >         ;
> >
> >     writel(ch, &uart->txd);
>
> Your analysis looks correct.
>
> The kernel does like this:
>
> static void imx_uart_console_putchar(struct uart_port *port, unsigned
> char ch)
> {
>         struct imx_port *sport = (struct imx_port *)port;
>
>         while (imx_uart_readl(sport, imx_uart_uts_reg(sport)) & UTS_TXFULL)
>                 barrier();
>
>         imx_uart_writel(sport, ch, URTX0);
> }
>
> Thanks

Fabio and Pali,

Seems reasonable but this does not resolve the problem. Whatever I
print in board_init gets cutoff by the print from dm_announce.

Tim

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

[Fabio Estevam]

Hi Tim,

On 25/10/2022 18:37, Tim Harvey wrote:

> Fabio and Pali,
> 
> Seems reasonable but this does not resolve the problem. Whatever I
> print in board_init gets cutoff by the print from dm_announce.

You are right.

I managed to reproduce it:

--- a/board/warp7/warp7.c
+++ b/board/warp7/warp7.c
@@ -71,6 +71,7 @@ int board_init(void)
         /* address of boot parameters */
         gd->bd->bi_boot_params = PHYS_SDRAM + 0x100;

+       printf("******************* 
0123456789012345678901234567890123456789\n");
         return 0;
  }


It does not print correctly:

U-Boot 2022.10-00796-gf9d16f2c0daf-dirty (Oct 25 2022 - 18:46:05 -0300)

CPU:   Freescale i.MX7S rev1.2 800 MHz (running at 792 MHz)
CPU:   Extended Commercial temperature grade (-20C to 105C) at 49C
Reset cause: POR
Model: Element14 Warp i.MX7 Board
Board: WARP7 in secure mode OPTEE DRAM 0x9d000000-0xa0000000
DRAM:  464 MiB
�Core:  73 devices, 17 uclasses, devicetree: separate3456789
PMIC: PFUZE3000 DEV_ID=0x30 REV_ID=0x11
MMC:   FSL_SDHC: 3, FSL_SDHC: 0
Loading Environment from MMC... OK
In:    serial@30860000
Out:   serial@30860000
Err:   serial@30860000
SEC0:  RNG instantiated
Net:   No ethernet found.
Hit any key to stop autoboot:  0

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

[Fabio Estevam]

Hi Tim,

On 25/10/2022 18:37, Tim Harvey wrote:

> Fabio and Pali,
> 
> Seems reasonable but this does not resolve the problem. Whatever I
> print in board_init gets cutoff by the print from dm_announce.

Should we check for both TXFULL and TXEMPTY conditions?

--- a/drivers/serial/serial_mxc.c
+++ b/drivers/serial/serial_mxc.c
@@ -311,7 +311,7 @@ static int mxc_serial_putc(struct udevice *dev, 
const char ch)
         struct mxc_serial_plat *plat = dev_get_plat(dev);
         struct mxc_uart *const uart = plat->reg;

-       if (readl(&uart->ts) & UTS_TXFULL)
+       if ((readl(&uart->ts) & UTS_TXFULL) || !(readl(&uart->ts) & 
UTS_TXEMPTY))
                 return -EAGAIN;

         writel(ch, &uart->txd);

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

[Peng Fan]

Hi Fabio

On 10/26/2022 6:19 AM, Fabio Estevam wrote:
> Hi Tim,
> 
> On 25/10/2022 18:37, Tim Harvey wrote:
> 
>> Fabio and Pali,
>>
>> Seems reasonable but this does not resolve the problem. Whatever I
>> print in board_init gets cutoff by the print from dm_announce.
> 
> Should we check for both TXFULL and TXEMPTY conditions?
> 
> --- a/drivers/serial/serial_mxc.c
> +++ b/drivers/serial/serial_mxc.c
> @@ -311,7 +311,7 @@ static int mxc_serial_putc(struct udevice *dev, 
> const char ch)
>          struct mxc_serial_plat *plat = dev_get_plat(dev);
>          struct mxc_uart *const uart = plat->reg;
> 
> -       if (readl(&uart->ts) & UTS_TXFULL)
> +       if ((readl(&uart->ts) & UTS_TXFULL) || !(readl(&uart->ts) & 
> UTS_TXEMPTY))

This may bring the issue that Johannes met back

Regards,
Peng.

>                  return -EAGAIN;
> 
>          writel(ch, &uart->txd);

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

[Fabio Estevam]

Hi Peng,

On Wed, Oct 26, 2022 at 7:11 AM Peng Fan <peng.fan@oss.nxp.com> wrote:

> This may bring the issue that Johannes met back

It looks like Johannes met the problem in the FIFO full case.

Johannes,

Could you please test this change?

Thanks

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

[Pali Rohár]

On Tuesday 25 October 2022 19:19:01 Fabio Estevam wrote:
> Hi Tim,
> 
> On 25/10/2022 18:37, Tim Harvey wrote:
> 
> > Fabio and Pali,
> > 
> > Seems reasonable but this does not resolve the problem. Whatever I
> > print in board_init gets cutoff by the print from dm_announce.
> 
> Should we check for both TXFULL and TXEMPTY conditions?
> 
> --- a/drivers/serial/serial_mxc.c
> +++ b/drivers/serial/serial_mxc.c
> @@ -311,7 +311,7 @@ static int mxc_serial_putc(struct udevice *dev, const
> char ch)
>         struct mxc_serial_plat *plat = dev_get_plat(dev);
>         struct mxc_uart *const uart = plat->reg;
> 
> -       if (readl(&uart->ts) & UTS_TXFULL)
> +       if ((readl(&uart->ts) & UTS_TXFULL) || !(readl(&uart->ts) &
> UTS_TXEMPTY))
>                 return -EAGAIN;
> 
>         writel(ch, &uart->txd);

Hello! I really do not understand what is the intention of this change.
Previously there was check only for !TXEMPTY. Then patch changed
!TXEMPTY to TXFULL. And now you are changing TXFULL to TXFULL&&!TXEMPTY.
It looks really suspicious.

I'm not sure what TXEMPTY and what TXFULL means for MXC hardware, but
normally TXEMPTY means that transmit queue is empty and HW already
transmitted all data - POSIX tctrain() function. TXFULL normally means
that transmit queue is full and HW cannot take data from CPU at the
moment. So normally TXEMPTY implies !TXFULL. And TXFULL then implies
!TXMEPTY.

So could you check what your HW means for TXEMPTY and TXFULL.

And second thing, when you need to check two bits from one HW register,
you can do it just via one readl() operation by checking masked value.

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

[SCHNEIDER Johannes]

Hi

the thing with only checking !TXEMPTY is that it limits the fifo to one byte only; and as far as i understood, drivermodel code is supposed to return immediately and not block possibly waiting indefinitely for a hardware/status register to change

a colleague pointed out the use of barriers in the quoted kernel code snippet -> are we perhaps running into code-reordering issues; that checking the status flags and writing into the queue gets out of sync?

regards

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

[Pali Rohár]

On Wednesday 26 October 2022 22:22:28 SCHNEIDER Johannes wrote:
> a colleague pointed out the use of barriers in the quoted kernel code snippet -> are we perhaps running into code-reordering issues; that checking the status flags and writing into the queue gets out of sync?

I was always in impression that readl() and writel() calls in U-Boot
cannot be reordered and so no explicit barrier is needed.

Could somebody check & confirm this?

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

[Fabio Estevam]

Johannes,

On Wed, Oct 26, 2022 at 7:23 PM SCHNEIDER Johannes
<johannes.schneider@leica-geosystems.com> wrote:
>
> Hi
>
> the thing with only checking !TXEMPTY is that it limits the fifo to one byte only; and as far as i understood, drivermodel code is supposed to return immediately and t

Why does it limit the FIFO to one byte only? The FIFO has 32 bytes.

I would like to understand what problem you were trying to solve when
you did the change:

-       if (!(readl(&uart->ts) & UTS_TXEMPTY))
+       if (readl(&uart->ts) & UTS_TXFULL)

Can you share a reproducer?

We need to figure this out soon, as this is causing a regression.

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

[SCHNEIDER Johannes]

Hoi,

>> the thing with only checking !TXEMPTY is that it limits the fifo to one byte only; and as far as i understood, drivermodel >code is supposed to return immediately and t
>
>Why does it limit the FIFO to one byte only? The FIFO has 32 bytes.
>

we're in putc, so dealing with one byte/character at a time.
with the original !TXEMPTY the function would immediately return with an egain as soon as the first spot in the fifo was occupied by the previous putc, never filling the fifo up any further through the write-register call that follows the if block in mxc_serial_putc
so yes, the fifo in the imx is 32bytes long - but the code did not make any use of it

>I would like to understand what problem you were trying to solve when
>you did the change:
>
>-       if (!(readl(&uart->ts) & UTS_TXEMPTY))
>+       if (readl(&uart->ts) & UTS_TXFULL)
>
>Can you share a reproducer?
>
>We need to figure this out soon, as this is causing a regression.

sadly the previously mentioned printf with a long string (longer than 32bytes) does not trigger the issue on the debug-serial...
(running on an imx8mm -> uart3 out of 4, and with uboot/v2202.07)

but what puzzles me most is that with my current setup (different tool: old analog oscilloscope replaced by an usb logic-analyzer, and a more recent software versions also on the receiving end where the problem cropped up originally) i'm currently unable to reproduce the original issue i saw - and described in the commit message :-S
back then the situation was as follows: with the analog scope on uart4 visually comparing waveforms clearly showed different message lengths when comparing u-boot generated traffic (without the patch = short, with the patch = "correct" length) with a reference/working signal generated from within a running linux.
the receiving end did not respond to the messages from within uboot prior to the patch; but did so with the TXFULL applied and also always when communication was initiated from linux

i still would argue to keep the change, since both the linux kernel and $other bootloaders make use of the TXFULL flag

kind regard

________________________________________
From: Fabio Estevam <festevam@gmail.com>
Sent: Tuesday, November 1, 2022 21:39
To: SCHNEIDER Johannes
Cc: Pali Rohár; 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

[Some people who received this message don't often get email from festevam@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]

This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.


Johannes,

On Wed, Oct 26, 2022 at 7:23 PM SCHNEIDER Johannes
<johannes.schneider@leica-geosystems.com> wrote:
>
> Hi
>
> the thing with only checking !TXEMPTY is that it limits the fifo to one byte only; and as far as i understood, drivermodel code is supposed to return immediately and t

Why does it limit the FIFO to one byte only? The FIFO has 32 bytes.

I would like to understand what problem you were trying to solve when
you did the change:

-       if (!(readl(&uart->ts) & UTS_TXEMPTY))
+       if (readl(&uart->ts) & UTS_TXFULL)

Can you share a reproducer?

We need to figure this out soon, as this is causing a regression.

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

[Fabio Estevam]

Hi Johannes,

On Wed, Nov 2, 2022 at 2:16 AM SCHNEIDER Johannes
<johannes.schneider@leica-geosystems.com> wrote:

> sadly the previously mentioned printf with a long string (longer than 32bytes) does not trigger the issue on the debug-serial...
> (running on an imx8mm -> uart3 out of 4, and with uboot/v2202.07)
>
> but what puzzles me most is that with my current setup (different tool: old analog oscilloscope replaced by an usb logic-analyzer, and a more recent software versions also on the receiving end where the problem cropped up originally) i'm currently unable to reproduce the original issue i saw - and described in the commit message :-S
> back then the situation was as follows: with the analog scope on uart4 visually comparing waveforms clearly showed different message lengths when comparing u-boot generated traffic (without the patch = short, with the patch = "correct" length) with a reference/working signal generated from within a running linux.
> the receiving end did not respond to the messages from within uboot prior to the patch; but did so with the TXFULL applied and also always when communication was initiated from linux
>
> i still would argue to keep the change, since both the linux kernel and $other bootloaders make use of the TXFULL flag

The problem is that your change is causing a regression as reported by
Tim. It can be easily reproduced.

Please help fix it, otherwise, we will need to revert your change.

Thanks,

Fabio Estevam

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

[SCHNEIDER Johannes]

Hi Fabio,

>On 25/10/2022 18:37, Tim Harvey wrote:
>
>> Fabio and Pali,
>>
>> Seems reasonable but this does not resolve the problem. Whatever I
>> print in board_init gets cutoff by the print from dm_announce.
>>You are right.
>>
>I managed to reproduce it:
>
>--- a/board/warp7/warp7.c
>+++ b/board/warp7/warp7.c
>@@ -71,6 +71,7 @@ int board_init(void)
>         /* address of boot parameters */
>         gd->bd->bi_boot_params = PHYS_SDRAM + 0x100;
>
>+       printf("*******************0123456789012345678901234567890123456789\n");
>         return 0;
>  }

sadly that does not reproduce on imx8mm ... a long print works fine with or without the patch :-S

> The RTC line is displayed from drivers/misc/gsc.c and the Core: comes
>from dm_announce. Somehow in between the FIFO does not get drained
>before dm_announce gets called.

"flushing" shouldn't be an issue, since everything goes through the same set of registers which the imx queues into its fifo...
i suspect the rootcause lies someplace else?


regards


regards

________________________________________
From: Fabio Estevam <festevam@gmail.com>
Sent: Wednesday, November 2, 2022 13:15
To: SCHNEIDER Johannes
Cc: Pali Rohár; 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

This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.


Hi Johannes,

On Wed, Nov 2, 2022 at 2:16 AM SCHNEIDER Johannes
<johannes.schneider@leica-geosystems.com> wrote:

> sadly the previously mentioned printf with a long string (longer than 32bytes) does not trigger the issue on the debug-serial...
> (running on an imx8mm -> uart3 out of 4, and with uboot/v2202.07)
>
> but what puzzles me most is that with my current setup (different tool: old analog oscilloscope replaced by an usb logic-analyzer, and a more recent software versions also on the receiving end where the problem cropped up originally) i'm currently unable to reproduce the original issue i saw - and described in the commit message :-S
> back then the situation was as follows: with the analog scope on uart4 visually comparing waveforms clearly showed different message lengths when comparing u-boot generated traffic (without the patch = short, with the patch = "correct" length) with a reference/working signal generated from within a running linux.
> the receiving end did not respond to the messages from within uboot prior to the patch; but did so with the TXFULL applied and also always when communication was initiated from linux
>
> i still would argue to keep the change, since both the linux kernel and $other bootloaders make use of the TXFULL flag

The problem is that your change is causing a regression as reported by
Tim. It can be easily reproduced.

Please help fix it, otherwise, we will need to revert your change.

Thanks,

Fabio Estevam

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

[Fabio Estevam]

Hi Johannes,

On 02/11/2022 09:38, SCHNEIDER Johannes wrote:

> sadly that does not reproduce on imx8mm ... a long print works fine
> with or without the patch :-S

Yes, it does happen on imx8mm.

You need to add the print() inside board_init().

Here is a reproducer on imx8mm evk:

diff --git a/board/freescale/imx8mm_evk/imx8mm_evk.c 
b/board/freescale/imx8mm_evk/imx8mm_evk.c
index e0975fcda705..69ec723c616e 100644
--- a/board/freescale/imx8mm_evk/imx8mm_evk.c
+++ b/board/freescale/imx8mm_evk/imx8mm_evk.c
@@ -50,6 +50,7 @@ int board_init(void)
         if (IS_ENABLED(CONFIG_FEC_MXC))
                 setup_fec();

+       
printf("*******************0123456789012345678901234567890123456789\n");
         return 0;
  }


The result is:

U-Boot SPL 2022.10-00991-gc8d9ff634fc4-dirty (Nov 02 2022 - 11:15:14 
-0300)
No pmic
SEC0:  RNG instantiated
cyclic_register for watchdog@30280000 failed
WDT:   Failed to start watchdog@30280000
Trying to boot from MMC1
NOTICE:  BL31: v2.6(release):lf-5.15.32-2.0.0-1-g044ea08c0109
NOTICE:  BL31: Built : 11:15:02, Nov  2 2022


U-Boot 2022.10-00991-gc8d9ff634fc4-dirty (Nov 02 2022 - 11:15:14 -0300)

CPU:   Freescale i.MX8MMQ rev1.0 at 1200 MHz
Reset cause: POR
Model: FSL i.MX8MM EVK board
DRAM:  2 GiB
*******************0123456789012345678901234567890123n�KW�'$HL�� 
devices, 23 uclasses, devicetree: separate
WDT:   Started watchdog@30280000 with servicing every 1000ms (60s 
timeout)
MMC:   FSL_SDHC: 1, FSL_SDHC: 2
Loading Environment from MMC... OK
In:    serial@30890000
Out:   serial@30890000
Err:   serial@30890000
SEC0:  RNG instantiated
Net:   eth0: ethernet@30be0000
Hit any key to stop autoboot:  0
u-boot=>

As you can see the corruption is clearly seen.

>> The RTC line is displayed from drivers/misc/gsc.c and the Core: comes
>> from dm_announce. Somehow in between the FIFO does not get drained
>> before dm_announce gets called.
> 
> "flushing" shouldn't be an issue, since everything goes through the
> same set of registers which the imx queues into its fifo...
> i suspect the rootcause lies someplace else?


Reverting your change makes the issue disappear.

Regards,

Fabio Estevam
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-60 Fax: (+49)-8142-66989-80 Email: 
festevam@denx.de

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

[Pali Rohár]

On Wednesday 02 November 2022 11:20:44 Fabio Estevam wrote:
> Hi Johannes,
> 
> On 02/11/2022 09:38, SCHNEIDER Johannes wrote:
> 
> > sadly that does not reproduce on imx8mm ... a long print works fine
> > with or without the patch :-S
> 
> Yes, it does happen on imx8mm.
> 
> You need to add the print() inside board_init().
> 
> Here is a reproducer on imx8mm evk:
> 
> diff --git a/board/freescale/imx8mm_evk/imx8mm_evk.c
> b/board/freescale/imx8mm_evk/imx8mm_evk.c
> index e0975fcda705..69ec723c616e 100644
> --- a/board/freescale/imx8mm_evk/imx8mm_evk.c
> +++ b/board/freescale/imx8mm_evk/imx8mm_evk.c
> @@ -50,6 +50,7 @@ int board_init(void)
>         if (IS_ENABLED(CONFIG_FEC_MXC))
>                 setup_fec();
> 
> +
> printf("*******************0123456789012345678901234567890123456789\n");
>         return 0;
>  }
> 
> 
> The result is:
> 
> U-Boot SPL 2022.10-00991-gc8d9ff634fc4-dirty (Nov 02 2022 - 11:15:14 -0300)
> No pmic
> SEC0:  RNG instantiated
> cyclic_register for watchdog@30280000 failed
> WDT:   Failed to start watchdog@30280000
> Trying to boot from MMC1
> NOTICE:  BL31: v2.6(release):lf-5.15.32-2.0.0-1-g044ea08c0109
> NOTICE:  BL31: Built : 11:15:02, Nov  2 2022
> 
> 
> U-Boot 2022.10-00991-gc8d9ff634fc4-dirty (Nov 02 2022 - 11:15:14 -0300)
> 
> CPU:   Freescale i.MX8MMQ rev1.0 at 1200 MHz
> Reset cause: POR
> Model: FSL i.MX8MM EVK board
> DRAM:  2 GiB
> *******************0123456789012345678901234567890123n�KW�'$HL�� devices, 23
> uclasses, devicetree: separate
> WDT:   Started watchdog@30280000 with servicing every 1000ms (60s timeout)
> MMC:   FSL_SDHC: 1, FSL_SDHC: 2
> Loading Environment from MMC... OK
> In:    serial@30890000
> Out:   serial@30890000
> Err:   serial@30890000
> SEC0:  RNG instantiated
> Net:   eth0: ethernet@30be0000
> Hit any key to stop autoboot:  0
> u-boot=>
> 
> As you can see the corruption is clearly seen.
> 
> > > The RTC line is displayed from drivers/misc/gsc.c and the Core: comes
> > > from dm_announce. Somehow in between the FIFO does not get drained
> > > before dm_announce gets called.
> > 
> > "flushing" shouldn't be an issue, since everything goes through the
> > same set of registers which the imx queues into its fifo...
> > i suspect the rootcause lies someplace else?
> 
> 
> Reverting your change makes the issue disappear.
> 
> Regards,
> 
> Fabio Estevam
> -- 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-60 Fax: (+49)-8142-66989-80 Email: festevam@denx.de

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))
+		;
+
 	tmp = RFDIV << UFCR_RFDIV_SHF;
 	if (use_dte)
 		tmp |= UFCR_DCEDTE;

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

[Fabio Estevam]

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.

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

[SCHNEIDER Johannes]

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.

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

[TERTYCHNYI Grygorii]

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.

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

[Pali Rohár]

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.

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

[Fabio Estevam]

On Thu, Nov 3, 2022 at 3:17 AM SCHNEIDER Johannes
<johannes.schneider@leica-geosystems.com> wrote:
>
> 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++);

There is a typo here: it should be mxc_serial_putc() instead.

No, it does not fix the issue.

Not sure why you mentioned imx6. The issue can be reproduced on imx8mm as well.

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

[Fabio Estevam]

Hi Johannes and Grygorii,

On Thu, Nov 3, 2022 at 8:14 AM Fabio Estevam <festevam@gmail.com> wrote:

> There is a typo here: it should be mxc_serial_putc() instead.
>
> No, it does not fix the issue.
>
> Not sure why you mentioned imx6. The issue can be reproduced on imx8mm as well.

Do you plan to submit a patch fixing the regression?

Thanks

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

[SCHNEIDER Johannes]

Hi Fabio,


gave it another round, and discussed it with Grygorii - he pointed out that it would be better to do the "fifo empty" or even better "byte sent" checks in "our" code instead of adding more complexity to the serial_mxc for handling a buffer/TXFIFO to be properly flushed prior to each of the multiple calls to set_baudrate during startup between spl and uboot;
i tried a couple of approaches which would add checks to "pending" to either a new "puts"-method, or "set_bgr" with partial success - the problem remains that long printfs during board_init - which happens before the serial device is actually fully initialized - get cut of near the end... anyway: added complexity -> !KISS

so i'd say go for the revert; which reduces use of the fifo back to just the first byte


sorry for the trouble
Johannes


________________________________________
From: Fabio Estevam <festevam@gmail.com>
Sent: Monday, November 7, 2022 22:17
To: SCHNEIDER Johannes; TERTYCHNYI Grygorii
Cc: Pali Rohár; 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

This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.


Hi Johannes and Grygorii,

On Thu, Nov 3, 2022 at 8:14 AM Fabio Estevam <festevam@gmail.com> wrote:

> There is a typo here: it should be mxc_serial_putc() instead.
>
> No, it does not fix the issue.
>
> Not sure why you mentioned imx6. The issue can be reproduced on imx8mm as well.

Do you plan to submit a patch fixing the regression?

Thanks

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

[Fabio Estevam]

Hi Johannes,

On Tue, Nov 8, 2022 at 6:37 AM SCHNEIDER Johannes
<johannes.schneider@leica-geosystems.com> wrote:

> gave it another round, and discussed it with Grygorii - he pointed out that it would be better to do the "fifo empty" or even better "byte sent" checks in "our" code instead of adding more complexity to the serial_mxc for handling a buffer/TXFIFO to be properly flushed prior to each of the multiple calls to set_baudrate during startup between spl and uboot;
> i tried a couple of approaches which would add checks to "pending" to either a new "puts"-method, or "set_bgr" with partial success - the problem remains that long printfs during board_init - which happens before the serial device is actually fully initialized - get cut of near the end... anyway: added complexity -> !KISS
>
> so i'd say go for the revert; which reduces use of the fifo back to just the first byte

Thanks for investigating it. I have sent the revert.

Regards,

Fabio Estevam

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

[Pali Rohár]

On Tuesday 25 October 2022 17:37:01 Fabio Estevam wrote:
> Hi Pali,
> 
> On 25/10/2022 17:23, Pali Rohár wrote:
> 
> > Hello! I do not have any MXC hardware but I see there one issue.
> > mxc_serial_putc() function probably should not return -EAGAIN when
> > device is busy. But instead it should wait until it is ready.
> > 
> > Could you try to change code to following?
> > 
> >     while (readl(&uart->ts) & UTS_TXFULL)
> >         ;
> > 
> >     writel(ch, &uart->txd);
> 
> Your analysis looks correct.
> 
> The kernel does like this:
> 
> static void imx_uart_console_putchar(struct uart_port *port, unsigned char
> ch)
> {
> 	struct imx_port *sport = (struct imx_port *)port;
> 
> 	while (imx_uart_readl(sport, imx_uart_uts_reg(sport)) & UTS_TXFULL)
> 		barrier();
> 
> 	imx_uart_writel(sport, ch, URTX0);
> }
> 
> Thanks

Well, "waiting for HW to be ready" vs. "returning -EAGAIN" is serial API
detail. Kernel (or other implementation) may have different API and
driver always have to implement what API expects. So aligning return
value with kernel is not the argument for fixing issue.


Anyway, I think that my change is not correct.

serial-uclass.c is already calling:

	do {
		err = ops->putc(dev, ch);
	} while (err == -EAGAIN);

Which means that function is called again.