[U-Boot] [PATCH V2] AT91: Small fix on AT91 USART initialization code

[U-Boot] [PATCH V2] AT91: Small fix on AT91 USART initialization code

[Hong Xu]

Before reset dbgu transmitter, we just wait TXEMPTY to drain the
transmitter register. If not doing this, we may sometimes see several
weird characters from DBGU.

A short delay is also added to make sure the new serial settings are
settled.

Signed-off-by: Hong Xu <hong.xu@atmel.com>
---
Change since V1:
- Fix comment for easy reading

 drivers/serial/atmel_usart.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/drivers/serial/atmel_usart.c b/drivers/serial/atmel_usart.c
index e326b2b..6f9c2de 100644
--- a/drivers/serial/atmel_usart.c
+++ b/drivers/serial/atmel_usart.c
@@ -49,17 +49,23 @@ int serial_init(void)
 {
 	atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
 
+	/* Just in case: drain transmitter register */
+	while (!(readl(&usart->csr) & USART3_BIT(TXEMPTY)))
+		;
+
 	writel(USART3_BIT(RSTRX) | USART3_BIT(RSTTX), &usart->cr);
 
 	serial_setbrg();
 
-	writel(USART3_BIT(RXEN) | USART3_BIT(TXEN), &usart->cr);
 	writel((USART3_BF(USART_MODE, USART3_USART_MODE_NORMAL)
 			   | USART3_BF(USCLKS, USART3_USCLKS_MCK)
 			   | USART3_BF(CHRL, USART3_CHRL_8)
 			   | USART3_BF(PAR, USART3_PAR_NONE)
 			   | USART3_BF(NBSTOP, USART3_NBSTOP_1)),
 			   &usart->mr);
+	writel(USART3_BIT(RXEN) | USART3_BIT(TXEN), &usart->cr);
+	/* 100us is enough for the new settings to be settled */
+	__udelay(100);
 
 	return 0;
 }
-- 
1.7.3.3

[U-Boot] [PATCH V2] AT91: Small fix on AT91 USART initialization code

[Reinhard Meyer]

Dear Hong Xu,
> Before reset dbgu transmitter, we just wait TXEMPTY to drain the
> transmitter register. If not doing this, we may sometimes see several
> weird characters from DBGU.
>
> A short delay is also added to make sure the new serial settings are
> settled.
>
> Signed-off-by: Hong Xu<hong.xu@atmel.com>
> ---
> Change since V1:
> - Fix comment for easy reading
>
>   drivers/serial/atmel_usart.c |    8 +++++++-
>   1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/serial/atmel_usart.c b/drivers/serial/atmel_usart.c
> index e326b2b..6f9c2de 100644
> --- a/drivers/serial/atmel_usart.c
> +++ b/drivers/serial/atmel_usart.c
> @@ -49,17 +49,23 @@ int serial_init(void)
>   {
>   	atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
>
> +	/* Just in case: drain transmitter register */
> +	while (!(readl(&usart->csr)&  USART3_BIT(TXEMPTY)))
> +		;
> +

You still have not addressed my concern about a possible hang situation here!
I rather have _some_ weird characters than an apparently dead board...
When do we have the possibility of weird characters anyway?
Only if a preloader makes output and transfers to u-boot before its output
has been flushed. Any other situations?

Best Regards,
Reinhard

[U-Boot] [PATCH V2] AT91: Small fix on AT91 USART initialization code

[Hong Xu]

Hi Reinhard,

On 08/01/2011 10:39 PM, Reinhard Meyer wrote:
> Dear Hong Xu,
>  > Before reset dbgu transmitter, we just wait TXEMPTY to drain the
>  > transmitter register. If not doing this, we may sometimes see several
>  > weird characters from DBGU.
>  >
>  > A short delay is also added to make sure the new serial settings are
>  > settled.
>  >
>  > Signed-off-by: Hong Xu<hong.xu@atmel.com>
>  > ---
>  > Change since V1:
>  > - Fix comment for easy reading
>  >
>  > drivers/serial/atmel_usart.c | 8 +++++++-
>  > 1 files changed, 7 insertions(+), 1 deletions(-)
>  >
>  > diff --git a/drivers/serial/atmel_usart.c b/drivers/serial/atmel_usart.c
>  > index e326b2b..6f9c2de 100644
>  > --- a/drivers/serial/atmel_usart.c
>  > +++ b/drivers/serial/atmel_usart.c
>  > @@ -49,17 +49,23 @@ int serial_init(void)
>  > {
>  > atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
>  >
>  > + /* Just in case: drain transmitter register */
>  > + while (!(readl(&usart->csr)& USART3_BIT(TXEMPTY)))
>  > + ;
>  > +
>
> You still have not addressed my concern about a possible hang situation
> here!
> I rather have _some_ weird characters than an apparently dead board...
> When do we have the possibility of weird characters anyway?
> Only if a preloader makes output and transfers to u-boot before its output
> has been flushed. Any other situations?

This is the main cause. I monitored some versions of bootstraps check 
TXRDY before sending data to THR (Transmitter holding register). And for 
the last character, they won't wait till the character has been sent 
out. (And maybe there are similar places in U-Boot before calling 
serial_init, I'm not sure)

The possibility of hang, I don't think so. Reason,
TXEMPTY means "There are no characters in DBGU_THR and there are no 
characters being processed by the transmitter."

Theoretically this situation shall be met here. But of course a timeout 
can be added defensively (chip goes mad?  :)

What's your opinion?

BR,
Eric

>
> Best Regards,
> Reinhard
>

[U-Boot] [PATCH V2] AT91: Small fix on AT91 USART initialization code

[Reinhard Meyer]

Dear Hong Xu,
> >  > diff --git a/drivers/serial/atmel_usart.c b/drivers/serial/atmel_usart.c
> >  > index e326b2b..6f9c2de 100644
> >  > --- a/drivers/serial/atmel_usart.c
> >  > +++ b/drivers/serial/atmel_usart.c
> >  > @@ -49,17 +49,23 @@ int serial_init(void)
> >  > {
> >  > atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
> >  >
> >  > + /* Just in case: drain transmitter register */
> >  > + while (!(readl(&usart->csr)& USART3_BIT(TXEMPTY)))
> >  > + ;
> >  > +
> >
> > You still have not addressed my concern about a possible hang situation
> > here!
> > I rather have _some_ weird characters than an apparently dead board...
> > When do we have the possibility of weird characters anyway?
> > Only if a preloader makes output and transfers to u-boot before its output
> > has been flushed. Any other situations?
> 
> This is the main cause. I monitored some versions of bootstraps check 
> TXRDY before sending data to THR (Transmitter holding register). And for 
> the last character, they won't wait till the character has been sent 
> out. (And maybe there are similar places in U-Boot before calling 
> serial_init, I'm not sure)

There should be no output made by u-boot BEFORE serial_init() is called!

> 
> The possibility of hang, I don't think so. Reason,
> TXEMPTY means "There are no characters in DBGU_THR and there are no 
> characters being processed by the transmitter."

What if there is no Baud Clock?
What if the USART is unconfigured or misconfigured?

> 
> Theoretically this situation shall be met here. But of course a timeout 
> can be added defensively (chip goes mad?  :)
> 
> What's your opinion?

Timeout is complicated at the pre-relocation level.
What is MOST IMPORTANT at this point is that u-boot must made be able to
print messages. Anything that could perhaps impair that should not be
done.
Keep the code simple and live with occasional weird characters.
End users of a system will usually not look at the serial debug output.
Or just use a __udelay(1000) - that should give time to drain any char
at 9600 bit/s and above.

Best Regards,
Reinhard

[U-Boot] [PATCH V2] AT91: Small fix on AT91 USART initialization code

[Hong Xu]

Hi Reinhard,

On 08/02/2011 05:54 PM, Reinhard Meyer wrote:
> Dear Hong Xu,
>  > > > diff --git a/drivers/serial/atmel_usart.c
> b/drivers/serial/atmel_usart.c
>  > > > index e326b2b..6f9c2de 100644
>  > > > --- a/drivers/serial/atmel_usart.c
>  > > > +++ b/drivers/serial/atmel_usart.c
>  > > > @@ -49,17 +49,23 @@ int serial_init(void)
>  > > > {
>  > > > atmel_usart3_t *usart = (atmel_usart3_t *)CONFIG_USART_BASE;
>  > > >
>  > > > + /* Just in case: drain transmitter register */
>  > > > + while (!(readl(&usart->csr)& USART3_BIT(TXEMPTY)))
>  > > > + ;
>  > > > +
>  > >
>  > > You still have not addressed my concern about a possible hang situation
>  > > here!
>  > > I rather have _some_ weird characters than an apparently dead board...
>  > > When do we have the possibility of weird characters anyway?
>  > > Only if a preloader makes output and transfers to u-boot before its
> output
>  > > has been flushed. Any other situations?
>  >
>  > This is the main cause. I monitored some versions of bootstraps check
>  > TXRDY before sending data to THR (Transmitter holding register). And for
>  > the last character, they won't wait till the character has been sent
>  > out. (And maybe there are similar places in U-Boot before calling
>  > serial_init, I'm not sure)
>
> There should be no output made by u-boot BEFORE serial_init() is called!
>
>  >
>  > The possibility of hang, I don't think so. Reason,
>  > TXEMPTY means "There are no characters in DBGU_THR and there are no
>  > characters being processed by the transmitter."
>
> What if there is no Baud Clock?
> What if the USART is unconfigured or misconfigured?
>
>  >
>  > Theoretically this situation shall be met here. But of course a timeout
>  > can be added defensively (chip goes mad? :)
>  >
>  > What's your opinion?
>
> Timeout is complicated at the pre-relocation level.
> What is MOST IMPORTANT at this point is that u-boot must made be able to
> print messages. Anything that could perhaps impair that should not be
> done.
> Keep the code simple and live with occasional weird characters.
> End users of a system will usually not look at the serial debug output.
> Or just use a __udelay(1000) - that should give time to drain any char
> at 9600 bit/s and above.

Agree. Actually I tried __udelay to overcome the weird character issue 
firstly. And later found the TXEMPTY was not set when serial_init is 
called. I'll send another patch soon (by using __udelay, simple but 
useful enough :-).

Thanks

BR,
Eric

> Best Regards,
> Reinhard
>
>