diff mbox

[U-Boot] serial: stm32: Automatically generate CR when LF is observed

Message ID 1431458726-2615-1-git-send-email-daniel.thompson@linaro.org
State Superseded
Delegated to: Tom Rini
Headers show

Commit Message

Daniel Thompson May 12, 2015, 7:25 p.m. UTC
Currently the u-boot output looks rather odd when running the minicom
terminal emulator in its default mode (just a string of "random" looking
text down the right hand side of the screen).

This is caused by a combination of minicom not automatically wrapping
lines and the stm32 serial driver never sending a carriage return.

Issue is trivially solved by automatically generating a CR whenever a LF
is transmitted, Several other serial drivers implement this behaviour.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
Cc: Kamil Lulko <rev13@wp.pl>
---
 drivers/serial/serial_stm32.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

rev13@wp.pl May 12, 2015, 8:35 p.m. UTC | #1
Strange, this was already posted by Kunhua Huang - then reverted in
commit 698a12bef9e782dcd99c555a739c16eec8669f14. Anyway, yes this
carriage return should be added there. I simply forgot it since I had
implicit CR for each LF turned on in my terminal. Never thought this
would cause so much havoc for users ;)

/Kamil

2015-05-12 21:25 GMT+02:00 Daniel Thompson <daniel.thompson@linaro.org>:
> Currently the u-boot output looks rather odd when running the minicom
> terminal emulator in its default mode (just a string of "random" looking
> text down the right hand side of the screen).
>
> This is caused by a combination of minicom not automatically wrapping
> lines and the stm32 serial driver never sending a carriage return.
>
> Issue is trivially solved by automatically generating a CR whenever a LF
> is transmitted, Several other serial drivers implement this behaviour.
>
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> Cc: Kamil Lulko <rev13@wp.pl>
> ---
>  drivers/serial/serial_stm32.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/serial/serial_stm32.c b/drivers/serial/serial_stm32.c
> index 3c80096..8c613db 100644
> --- a/drivers/serial/serial_stm32.c
> +++ b/drivers/serial/serial_stm32.c
> @@ -81,6 +81,10 @@ static int stm32_serial_getc(void)
>  static void stm32_serial_putc(const char c)
>  {
>         struct stm32_serial *usart = (struct stm32_serial *)USART_BASE;
> +
> +       if (c == '\n')
> +               stm32_serial_putc('\r');
> +
>         while ((readl(&usart->sr) & USART_SR_FLAG_TXE) == 0)
>                 ;
>         writel(c, &usart->dr);
> --
> 2.1.0
>
>
Tom Rini May 12, 2015, 9:56 p.m. UTC | #2
On Tue, May 12, 2015 at 10:35:55PM +0200, Kamil Lulko wrote:

> Strange, this was already posted by Kunhua Huang - then reverted in
> commit 698a12bef9e782dcd99c555a739c16eec8669f14. Anyway, yes this
> carriage return should be added there. I simply forgot it since I had
> implicit CR for each LF turned on in my terminal. Never thought this
> would cause so much havoc for users ;)

I reverted it since the author said it wasn't needed with the other
patch they did being applied.  Daniel, can you confirm the odd behavior
exists with top of tree? Thanks!
kunhuahuang May 13, 2015, 4:41 a.m. UTC | #3
On Tue, May 12, 2015 at 05:56:36PM -0400, Tom Rini wrote:
> On Tue, May 12, 2015 at 10:35:55PM +0200, Kamil Lulko wrote:
>
> > Strange, this was already posted by Kunhua Huang - then reverted in
> > commit 698a12bef9e782dcd99c555a739c16eec8669f14. Anyway, yes this
> > carriage return should be added there. I simply forgot it since I had
> > implicit CR for each LF turned on in my terminal. Never thought this
> > would cause so much havoc for users ;)
>
> I reverted it since the author said it wasn't needed with the other
> patch they did being applied.  Daniel, can you confirm the odd behavior
> exists with top of tree? Thanks!
>
> --
> Tom

Sorry about my wrong expression.

In these two patch "[U-Boot] [PATCH v2] stm32f4: fix serial output"
and "[U-Boot] [PATCH v2] stm32f4: add serial print port"
, there are same and similar code as below.


In "[U-Boot] [PATCH v2] stm32f4: fix serial output" :

	struct stm32_serial *usart = (struct stm32_serial *)USART_BASE;
	+
	+	if (c == '\n')
	+		stm32_serial_putc('\r');
	+
	while ((readl(&usart->sr) & USART_SR_FLAG_TXE) == 0)
			;
	writel(c, &usart->dr);

In "[U-Boot] [PATCH v2] stm32f4: add serial print port" :

	-	struct stm32_serial *usart = (struct stm32_serial *)USART_BASE;
	+	struct stm32_serial *usart =
	+		(struct stm32_serial *)usart_base[USART_PORT];
	+
	+	if (c == '\n')
	+		stm32_serial_putc('\r');
	+
		while ((readl(&usart->sr) & USART_SR_FLAG_TXE) == 0)
				;
		writel(c, &usart->dr);

If these "two patch" patch at the same time, it may happened
conflict.

I have reply the mail in "[U-Boot] [U-Boot,v2] stm32f4: add serial print
port" as below,
>Sorry about the mistake of this patch.
>This patch has already include "[PATCH v2] stm32f4: fix serial output".
>If adopt this patch, patch "[PATCH v2] stm32f4: fix serial output" need
>to be discarded.

This means that "add serial print port" has already include
"stm32f4: fix serial output".
So, if patch "add serial print port", then patch "stm32f4: fix serial output"
should not be patch.

But patch these "two patch" and then revert it. It will cause the code
disappear.

Thanks to Daniel Thompson.
Daniel Thompson May 13, 2015, 2:11 p.m. UTC | #4
On 12/05/15 22:56, Tom Rini wrote:
> On Tue, May 12, 2015 at 10:35:55PM +0200, Kamil Lulko wrote:
>
>> Strange, this was already posted by Kunhua Huang - then reverted in
>> commit 698a12bef9e782dcd99c555a739c16eec8669f14. Anyway, yes this
>> carriage return should be added there. I simply forgot it since I had
>> implicit CR for each LF turned on in my terminal. Never thought this
>> would cause so much havoc for users ;)
>
> I reverted it since the author said it wasn't needed with the other
> patch they did being applied.  Daniel, can you confirm the odd behavior
> exists with top of tree? Thanks!

Yes, the odd behavior still exists with top of tree.

That said, I lucked out here.

In truth I had the problem because my git tree was slightly more out of 
date than I thought it was (and because a google search for "serial 
u-boot stm32" before posting my patch didn't notice the code from Kunhua).

So... my patch won't apply to HEAD anyway but reverting the revert would 
be very welcome!


Daniel.
diff mbox

Patch

diff --git a/drivers/serial/serial_stm32.c b/drivers/serial/serial_stm32.c
index 3c80096..8c613db 100644
--- a/drivers/serial/serial_stm32.c
+++ b/drivers/serial/serial_stm32.c
@@ -81,6 +81,10 @@  static int stm32_serial_getc(void)
 static void stm32_serial_putc(const char c)
 {
 	struct stm32_serial *usart = (struct stm32_serial *)USART_BASE;
+
+	if (c == '\n')
+		stm32_serial_putc('\r');
+
 	while ((readl(&usart->sr) & USART_SR_FLAG_TXE) == 0)
 		;
 	writel(c, &usart->dr);