[Xilinx Zynq]: spurious UART receive on startup

[Xilinx Zynq]: spurious UART receive on startup

[Norbert Braun]

Hi,

I am running into a problem with U-Boot v2020.07 on Xilinx Zynq
(Zedboard). If I build U-Boot with the default config
(xilinx_zynq_virt_defconfig, DEVICE_TREE=zynq-zed), autoboot will abort,
even if no key is pressed. This happens regardless of whether the USB
UART is even connected to a PC.

I am using boot.bin as generated by U-Boot as the SPL (plus u-boot.img
for U-Boot proper).

While debugging this, I noticed two things:

1. The Zynq TRM [1] notes in section 19.2.3 ("Baud generator"):
    IMPORTANT: It is essential to disable the transmitter and receiver
    before writing to the Baud Rate Generator register
    (uart.Baud_rate_gen_reg0), or the baud rate divider register
    (uart.Baud_rate_divider_reg0). A soft reset must be issued to both
    the transmitter and receiver before they are re-enabled.

However, the code in _uart_zynq_serial_setbrg() (in
drivers/serial/serial_zynq.c) does not seem to do that.

2. It seems that the Zynq BootROM actually touches the registers for
UART1. I have no idea why it does that, but table 6-22 ("BootROM
Modified Registers") in the TRM lists several UART1 registers that have
been modified from their reset value. In particular, the control
register at 0xE0001000 contains the value 0x00000114 after the BootROM ran.

As zynq_serial_probe() checks if TX is enabled (bit 4 in the control
register), and only writes to the control register if it is not, the end
result is that U-Boot never really initializes UART1 or resets its TX or
RX path.

If the debug UART functionality (CONFIG_DEBUG_UART_ZYNQ) is enabled,
_debug_uart_init() writes to the control register unconditionally and
resets the TX/RX path. Indeed, enabling the debug UART functionality
makes my problem disappear. The debug UART was enabled by default in
zynq_zed_defconfig (in v2019.10), but this changed when switching to a
single Zynq configuration (xilinx_zynq_virt_defconfig) for v2020.07.

Note that the above workaround fixes the problem for me, but I wanted to
report it in case other people run into the same issue.

Best regards,

Norbert

[1]: https://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf

[Xilinx Zynq]: spurious UART receive on startup

[Michal Simek]

Hi,

ne 16. 8. 2020 v 13:43 odes?latel Norbert Braun <norbert@xrpbot.org> napsal:
>
> Hi,
>
> I am running into a problem with U-Boot v2020.07 on Xilinx Zynq
> (Zedboard). If I build U-Boot with the default config
> (xilinx_zynq_virt_defconfig, DEVICE_TREE=zynq-zed), autoboot will abort,
> even if no key is pressed. This happens regardless of whether the USB
> UART is even connected to a PC.
>
> I am using boot.bin as generated by U-Boot as the SPL (plus u-boot.img
> for U-Boot proper).
>
> While debugging this, I noticed two things:
>
> 1. The Zynq TRM [1] notes in section 19.2.3 ("Baud generator"):
>     IMPORTANT: It is essential to disable the transmitter and receiver
>     before writing to the Baud Rate Generator register
>     (uart.Baud_rate_gen_reg0), or the baud rate divider register
>     (uart.Baud_rate_divider_reg0). A soft reset must be issued to both
>     the transmitter and receiver before they are re-enabled.
>
> However, the code in _uart_zynq_serial_setbrg() (in
> drivers/serial/serial_zynq.c) does not seem to do that.

I have reported this internally at Xilinx to fix this but none has
looked at it yet.
But definitely feel free to send a patch to fix it. I expect the
similar issue is when
you change baudrate via prompt.


> 2. It seems that the Zynq BootROM actually touches the registers for
> UART1. I have no idea why it does that, but table 6-22 ("BootROM
> Modified Registers") in the TRM lists several UART1 registers that have
> been modified from their reset value. In particular, the control
> register at 0xE0001000 contains the value 0x00000114 after the BootROM ran.
>
> As zynq_serial_probe() checks if TX is enabled (bit 4 in the control
> register), and only writes to the control register if it is not, the end
> result is that U-Boot never really initializes UART1 or resets its TX or
> RX path.

I have added this code because when reinitialization was happening there were
random chars emitted. Not sure which flow you use. But I didn't see any issue
in SPL boot flow.

>
> If the debug UART functionality (CONFIG_DEBUG_UART_ZYNQ) is enabled,
> _debug_uart_init() writes to the control register unconditionally and
> resets the TX/RX path. Indeed, enabling the debug UART functionality
> makes my problem disappear. The debug UART was enabled by default in
> zynq_zed_defconfig (in v2019.10), but this changed when switching to a
> single Zynq configuration (xilinx_zynq_virt_defconfig) for v2020.07.
>
> Note that the above workaround fixes the problem for me, but I wanted to
> report it in case other people run into the same issue.

Bootrom shouldn't touch these regs. It is code in ps7_init which does it.
It means take a look at it. It should do full uart initialization.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs

[Xilinx Zynq]: spurious UART receive on startup

[Norbert Braun]

Hi,

On 17.08.20 12:07, Michal Simek wrote:

> Hi,
>
> ne 16. 8. 2020 v 13:43 odes?latel Norbert Braun <norbert@xrpbot.org> napsal:
>> Hi,
>>
>> I am running into a problem with U-Boot v2020.07 on Xilinx Zynq
>> (Zedboard). If I build U-Boot with the default config
>> (xilinx_zynq_virt_defconfig, DEVICE_TREE=zynq-zed), autoboot will abort,
>> even if no key is pressed. This happens regardless of whether the USB
>> UART is even connected to a PC.
>>
>> I am using boot.bin as generated by U-Boot as the SPL (plus u-boot.img
>> for U-Boot proper).
>>
>> While debugging this, I noticed two things:
>>
>> 1. The Zynq TRM [1] notes in section 19.2.3 ("Baud generator"):
>>     IMPORTANT: It is essential to disable the transmitter and receiver
>>     before writing to the Baud Rate Generator register
>>     (uart.Baud_rate_gen_reg0), or the baud rate divider register
>>     (uart.Baud_rate_divider_reg0). A soft reset must be issued to both
>>     the transmitter and receiver before they are re-enabled.
>>
>> However, the code in _uart_zynq_serial_setbrg() (in
>> drivers/serial/serial_zynq.c) does not seem to do that.
> I have reported this internally at Xilinx to fix this but none has
> looked at it yet.
> But definitely feel free to send a patch to fix it. I expect the
> similar issue is when
> you change baudrate via prompt.

Ok, I will send a patch (see next mail).

The patch does not seem to make much of a difference for changing the baud rate
via prompt; it works for me with or without the patch. One thing I have noticed
is that on_baudrate prints a string ("## Switch baudrate to [...]") and then
waits for a fixed 50ms delay before changing the baud rate. If the original
baud rate is low (e.g. 4800 bps), this causes the string to be truncated, but
again, the patch makes no difference there.

>> 2. It seems that the Zynq BootROM actually touches the registers for
>> UART1. I have no idea why it does that, but table 6-22 ("BootROM
>> Modified Registers") in the TRM lists several UART1 registers that have
>> been modified from their reset value. In particular, the control
>> register at 0xE0001000 contains the value 0x00000114 after the BootROM ran.
>>
>> As zynq_serial_probe() checks if TX is enabled (bit 4 in the control
>> register), and only writes to the control register if it is not, the end
>> result is that U-Boot never really initializes UART1 or resets its TX or
>> RX path.
> I have added this code because when reinitialization was happening there were
> random chars emitted. Not sure which flow you use. But I didn't see any issue
> in SPL boot flow.

Basically, I do the following:

git checkout v2020.07
export CROSS_COMPILE=arm-linux-gnueabihf-
export ARCH=arm
export DEVICE_TREE=zynq-zed
make xilinx_zynq_virt_defconfig
make

cp u-boot.img [ to SD card ]
cp spl/boot.bin [ to SD card ]

This is sufficient to demonstrate the problem on my Zedboard. (There are no
other files on the SD card.)

One thing I should have mentioned (sorry!) is that I have a Rev. C Zedboard
with an ES (engineering sample) Zynq SoC on it. Maybe there is some difference
that is causing the problem, and production silicon is unaffected.? Still,
following the recommendation in the TRM when changing the baud rate probably
cannot hurt.

>> If the debug UART functionality (CONFIG_DEBUG_UART_ZYNQ) is enabled,
>> _debug_uart_init() writes to the control register unconditionally and
>> resets the TX/RX path. Indeed, enabling the debug UART functionality
>> makes my problem disappear. The debug UART was enabled by default in
>> zynq_zed_defconfig (in v2019.10), but this changed when switching to a
>> single Zynq configuration (xilinx_zynq_virt_defconfig) for v2020.07.
>>
>> Note that the above workaround fixes the problem for me, but I wanted to
>> report it in case other people run into the same issue.
> Bootrom shouldn't touch these regs. It is code in ps7_init which does it.
> It means take a look at it. It should do full uart initialization.

I looked at board/xilinx/zynq/zynq-zed/ps7_init_gpl.c, but it does not seem to
touch the UART registers. It does set up pinmuxing and enables the UART
peripheral clock, but it does not access any register in the 0XE0001xxx range.

> Thanks,
> Michal

Thanks,

Norbert

[PATCH] serial: zynq: disable TX and RX while changing baud rate

[Norbert Braun]

According to the Zynq-7000 TRM (UG585), the UART transmitter and
receiver must be disabled while changing the baud rate. Change
_uart_zynq_serial_setbrg accordingly.
---
 drivers/serial/serial_zynq.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/serial/serial_zynq.c b/drivers/serial/serial_zynq.c
index 0e71cada1b..4f7bab16fa 100644
--- a/drivers/serial/serial_zynq.c
+++ b/drivers/serial/serial_zynq.c
@@ -23,7 +23,9 @@
 #define ZYNQ_UART_SR_TXFULL	BIT(4) /* TX FIFO full */
 #define ZYNQ_UART_SR_RXEMPTY	BIT(1) /* RX FIFO empty */
 
+#define ZYNQ_UART_CR_TX_DIS	BIT(5) /* TX disable */
 #define ZYNQ_UART_CR_TX_EN	BIT(4) /* TX enabled */
+#define ZYNQ_UART_CR_RX_DIS	BIT(3) /* RX disable */
 #define ZYNQ_UART_CR_RX_EN	BIT(2) /* RX enabled */
 #define ZYNQ_UART_CR_TXRST	BIT(1) /* TX logic reset */
 #define ZYNQ_UART_CR_RXRST	BIT(0) /* RX logic reset */
@@ -82,8 +84,26 @@ static void _uart_zynq_serial_setbrg(struct uart_zynq *regs,
 			break;
 	}
 
-	writel(bdiv, &regs->baud_rate_divider);
-	writel(bgen, &regs->baud_rate_gen);
+	/* Check if baud rate registers actually need to be changed. */
+	if(readl(&regs->baud_rate_divider) != bdiv ||
+	   readl(&regs->baud_rate_gen) != bgen) {
+		/*
+		 * Configure the baud rate.
+		 *
+		 * This follows the procedure from the
+		 * Zynq-7000 SoC Technical Reference Manual,
+		 * UG585 (v1.12.2),
+		 * section 19.3.2, part 2.
+		 */
+
+		writel(ZYNQ_UART_CR_RX_DIS, &regs->control);
+		writel(ZYNQ_UART_CR_TX_DIS, &regs->control);
+		writel(bgen, &regs->baud_rate_gen);
+		writel(bdiv, &regs->baud_rate_divider);
+		writel(ZYNQ_UART_CR_TXRST | ZYNQ_UART_CR_RXRST, &regs->control);
+		writel(ZYNQ_UART_CR_RX_EN, &regs->control);
+		writel(ZYNQ_UART_CR_TX_EN, &regs->control);
+	}
 }
 
 /* Initialize the UART, with...some settings. */
-- 
2.17.1

[PATCH] serial: zynq: disable TX and RX while changing baud rate

[Michal Simek]

On 24. 08. 20 23:57, Norbert Braun wrote:
> According to the Zynq-7000 TRM (UG585), the UART transmitter and
> receiver must be disabled while changing the baud rate. Change
> _uart_zynq_serial_setbrg accordingly.
> ---
>  drivers/serial/serial_zynq.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/serial/serial_zynq.c b/drivers/serial/serial_zynq.c
> index 0e71cada1b..4f7bab16fa 100644
> --- a/drivers/serial/serial_zynq.c
> +++ b/drivers/serial/serial_zynq.c
> @@ -23,7 +23,9 @@
>  #define ZYNQ_UART_SR_TXFULL	BIT(4) /* TX FIFO full */
>  #define ZYNQ_UART_SR_RXEMPTY	BIT(1) /* RX FIFO empty */
>  
> +#define ZYNQ_UART_CR_TX_DIS	BIT(5) /* TX disable */
>  #define ZYNQ_UART_CR_TX_EN	BIT(4) /* TX enabled */
> +#define ZYNQ_UART_CR_RX_DIS	BIT(3) /* RX disable */
>  #define ZYNQ_UART_CR_RX_EN	BIT(2) /* RX enabled */
>  #define ZYNQ_UART_CR_TXRST	BIT(1) /* TX logic reset */
>  #define ZYNQ_UART_CR_RXRST	BIT(0) /* RX logic reset */
> @@ -82,8 +84,26 @@ static void _uart_zynq_serial_setbrg(struct uart_zynq *regs,
>  			break;
>  	}
>  
> -	writel(bdiv, &regs->baud_rate_divider);
> -	writel(bgen, &regs->baud_rate_gen);
> +	/* Check if baud rate registers actually need to be changed. */
> +	if(readl(&regs->baud_rate_divider) != bdiv ||
> +	   readl(&regs->baud_rate_gen) != bgen) {
> +		/*
> +		 * Configure the baud rate.
> +		 *
> +		 * This follows the procedure from the
> +		 * Zynq-7000 SoC Technical Reference Manual,
> +		 * UG585 (v1.12.2),
> +		 * section 19.3.2, part 2.
> +		 */
> +
> +		writel(ZYNQ_UART_CR_RX_DIS, &regs->control);
> +		writel(ZYNQ_UART_CR_TX_DIS, &regs->control);
> +		writel(bgen, &regs->baud_rate_gen);
> +		writel(bdiv, &regs->baud_rate_divider);
> +		writel(ZYNQ_UART_CR_TXRST | ZYNQ_UART_CR_RXRST, &regs->control);
> +		writel(ZYNQ_UART_CR_RX_EN, &regs->control);
> +		writel(ZYNQ_UART_CR_TX_EN, &regs->control);
> +	}
>  }
>  
>  /* Initialize the UART, with...some settings. */
> 

Looks good. I have added your SoB line and applied.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs

[PATCH] serial: zynq: disable TX and RX while changing baud rate

[Michal Simek]

po 14. 9. 2020 v 13:42 odes?latel Michal Simek <monstr@monstr.eu> napsal:
>
>
>
> On 24. 08. 20 23:57, Norbert Braun wrote:
> > According to the Zynq-7000 TRM (UG585), the UART transmitter and
> > receiver must be disabled while changing the baud rate. Change
> > _uart_zynq_serial_setbrg accordingly.
> > ---
> >  drivers/serial/serial_zynq.c | 24 ++++++++++++++++++++++--
> >  1 file changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/serial/serial_zynq.c b/drivers/serial/serial_zynq.c
> > index 0e71cada1b..4f7bab16fa 100644
> > --- a/drivers/serial/serial_zynq.c
> > +++ b/drivers/serial/serial_zynq.c
> > @@ -23,7 +23,9 @@
> >  #define ZYNQ_UART_SR_TXFULL  BIT(4) /* TX FIFO full */
> >  #define ZYNQ_UART_SR_RXEMPTY BIT(1) /* RX FIFO empty */
> >
> > +#define ZYNQ_UART_CR_TX_DIS  BIT(5) /* TX disable */
> >  #define ZYNQ_UART_CR_TX_EN   BIT(4) /* TX enabled */
> > +#define ZYNQ_UART_CR_RX_DIS  BIT(3) /* RX disable */
> >  #define ZYNQ_UART_CR_RX_EN   BIT(2) /* RX enabled */
> >  #define ZYNQ_UART_CR_TXRST   BIT(1) /* TX logic reset */
> >  #define ZYNQ_UART_CR_RXRST   BIT(0) /* RX logic reset */
> > @@ -82,8 +84,26 @@ static void _uart_zynq_serial_setbrg(struct uart_zynq *regs,
> >                       break;
> >       }
> >
> > -     writel(bdiv, &regs->baud_rate_divider);
> > -     writel(bgen, &regs->baud_rate_gen);
> > +     /* Check if baud rate registers actually need to be changed. */
> > +     if(readl(&regs->baud_rate_divider) != bdiv ||
> > +        readl(&regs->baud_rate_gen) != bgen) {
> > +             /*
> > +              * Configure the baud rate.
> > +              *
> > +              * This follows the procedure from the
> > +              * Zynq-7000 SoC Technical Reference Manual,
> > +              * UG585 (v1.12.2),
> > +              * section 19.3.2, part 2.
> > +              */
> > +
> > +             writel(ZYNQ_UART_CR_RX_DIS, &regs->control);
> > +             writel(ZYNQ_UART_CR_TX_DIS, &regs->control);
> > +             writel(bgen, &regs->baud_rate_gen);
> > +             writel(bdiv, &regs->baud_rate_divider);
> > +             writel(ZYNQ_UART_CR_TXRST | ZYNQ_UART_CR_RXRST, &regs->control);
> > +             writel(ZYNQ_UART_CR_RX_EN, &regs->control);
> > +             writel(ZYNQ_UART_CR_TX_EN, &regs->control);
> > +     }
> >  }
> >
> >  /* Initialize the UART, with...some settings. */
> >
>
> Looks good. I have added your SoB line and applied.


I am dropping this patch from my queue because it breaks travis and
gitlab CI loop. There is any issue with this patch when we run on
qemu.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs