[PATCH v5 0/2] serial_mxc: fixing reception
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(-)
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); }
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
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);
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
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
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);
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
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
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
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);
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);
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
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.
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
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?
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.
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.
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
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
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
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;
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.
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.
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.
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 TXFIFOwhile(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.
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.
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
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
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
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.
participants (9)
-
Fabio Estevam -
Fabio Estevam -
Johannes Schneider -
Pali Rohár -
Peng Fan -
sbabic＠denx.de -
SCHNEIDER Johannes
-
TERTYCHNYI Grygorii -
Tim Harvey