[U-Boot] [PATCH] serial: lpc32xx: send CR before LF

[U-Boot] [PATCH] serial: lpc32xx: send CR before LF

[Vladimir Zapolskiy]

For LPC32XX high-speed UART it is required to send a carriage return
symbol along with line feed. The problem was introduced in e503f90a
commit.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
Cc: Marek Vasut <marex@denx.de>
---
 drivers/serial/lpc32xx_hsuart.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/serial/lpc32xx_hsuart.c b/drivers/serial/lpc32xx_hsuart.c
index 9c7c621..c8926a8 100644
--- a/drivers/serial/lpc32xx_hsuart.c
+++ b/drivers/serial/lpc32xx_hsuart.c
@@ -38,6 +38,9 @@ static int lpc32xx_serial_getc(void)
 
 static void lpc32xx_serial_putc(const char c)
 {
+	if (c == '\n')
+		serial_putc('\r');
+
 	writel(c, &hsuart->tx);
 
 	/* Wait for character to be sent */
-- 
1.7.10.4

[U-Boot] [PATCH] serial: lpc32xx: send CR before LF

[Marek Vasut]

Dear Vladimir Zapolskiy,

> For LPC32XX high-speed UART it is required to send a carriage return
> symbol along with line feed.

Why ?

> The problem was introduced in e503f90a
> commit.
> 
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> Cc: Marek Vasut <marex@denx.de>
> ---
>  drivers/serial/lpc32xx_hsuart.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/serial/lpc32xx_hsuart.c
> b/drivers/serial/lpc32xx_hsuart.c index 9c7c621..c8926a8 100644
> --- a/drivers/serial/lpc32xx_hsuart.c
> +++ b/drivers/serial/lpc32xx_hsuart.c
> @@ -38,6 +38,9 @@ static int lpc32xx_serial_getc(void)
> 
>  static void lpc32xx_serial_putc(const char c)
>  {
> +	if (c == '\n')
> +		serial_putc('\r');
> +
>  	writel(c, &hsuart->tx);
> 
>  	/* Wait for character to be sent */

btw. I have a feeling each and every driver does send CR-LF combo and emulates 
it this way. Maybe this should eventually go into common code ? But that's just 
a suggestion for future improvement.

Best regards,
Marek Vasut

[U-Boot] [PATCH] serial: lpc32xx: send CR before LF

[Vladimir Zapolskiy]

Hi Marek,

On 11/30/13 17:42, Marek Vasut wrote:
> Dear Vladimir Zapolskiy,
>
>> For LPC32XX high-speed UART it is required to send a carriage return
>> symbol along with line feed.
>
> Why ?

good question, that's given in experience.

Without CR symbol the output is crumpled:

----8<----
DRAM:  64 MiB
              WARNING: Caches not enabled
                                         Flash: 4 MiB
                                                     Using default 
environment
----8<----

Also note there are similar hooks in several other serial drivers.

>> The problem was introduced in e503f90a
>> commit.
>>
>> Signed-off-by: Vladimir Zapolskiy<vz@mleia.com>
>> Cc: Marek Vasut<marex@denx.de>
>> ---
>>   drivers/serial/lpc32xx_hsuart.c |    3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/serial/lpc32xx_hsuart.c
>> b/drivers/serial/lpc32xx_hsuart.c index 9c7c621..c8926a8 100644
>> --- a/drivers/serial/lpc32xx_hsuart.c
>> +++ b/drivers/serial/lpc32xx_hsuart.c
>> @@ -38,6 +38,9 @@ static int lpc32xx_serial_getc(void)
>>
>>   static void lpc32xx_serial_putc(const char c)
>>   {
>> +	if (c == '\n')
>> +		serial_putc('\r');
>> +
>>   	writel(c,&hsuart->tx);
>>
>>   	/* Wait for character to be sent */
>
> btw. I have a feeling each and every driver does send CR-LF combo and emulates
> it this way. Maybe this should eventually go into common code ? But that's just
> a suggestion for future improvement.
>
> Best regards,
> Marek Vasut

With best wishes,
Vladimir

[U-Boot] [PATCH] serial: lpc32xx: send CR before LF

[Vladimir Zapolskiy]

On 11/30/13 17:42, Marek Vasut wrote:
> Dear Vladimir Zapolskiy,
>
>> For LPC32XX high-speed UART it is required to send a carriage return
>> symbol along with line feed.
>
> Why ?
>
>> The problem was introduced in e503f90a
>> commit.
>>
>> Signed-off-by: Vladimir Zapolskiy<vz@mleia.com>
>> Cc: Marek Vasut<marex@denx.de>
>> ---
>>   drivers/serial/lpc32xx_hsuart.c |    3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/serial/lpc32xx_hsuart.c
>> b/drivers/serial/lpc32xx_hsuart.c index 9c7c621..c8926a8 100644
>> --- a/drivers/serial/lpc32xx_hsuart.c
>> +++ b/drivers/serial/lpc32xx_hsuart.c
>> @@ -38,6 +38,9 @@ static int lpc32xx_serial_getc(void)
>>
>>   static void lpc32xx_serial_putc(const char c)
>>   {
>> +	if (c == '\n')
>> +		serial_putc('\r');
>> +
>>   	writel(c,&hsuart->tx);
>>
>>   	/* Wait for character to be sent */
>
> btw. I have a feeling each and every driver does send CR-LF combo and emulates
> it this way. Maybe this should eventually go into common code ? But that's just
> a suggestion for future improvement.

No objections from my side, however it seems that not every particular 
serial
controller requires such a hook, for code generalization it may be good 
to have
it for all drivers, but strict enforcement may be redundant for some subset.

>
> Best regards,
> Marek Vasut

With best wishes,
Vladimir

[U-Boot] [PATCH] serial: lpc32xx: send CR before LF

[Wolfgang Denk]

Dear Marek Vasut,

In message <201311301642.36531.marex@denx.de> you wrote:
> Dear Vladimir Zapolskiy,
> 
> > For LPC32XX high-speed UART it is required to send a carriage return
> > symbol along with line feed.
> 
> Why ?

Because we are actually emulating a classical type writer here, and to
start printing at the beginning of a new line requires two separate
actions: performing a line feed (i. e. scrolling the paper one line
up) and a crriage return (i. e. repositioning the drum such that the
next character will be printed in column 1.

In the strict sense, the ASCII characters represent the line feed and
'\r' the carriage return, respectively.  To print the equivalent of
the "new line" as it is interpreted by the C standard, we have to
translate the single C character into a two-character sequence.  You
can trace this back to the very beginnings of Unix; you can see the
same in Unix version 6 drivers, for example.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A well-written program is its own heaven; a poorly-written program is
its own hell.             -- Geoffrey James, "The Tao of Programming"

[U-Boot] [PATCH] serial: lpc32xx: send CR before LF

[Marek Vasut]

Dear Wolfgang Denk,

> Dear Marek Vasut,
> 
> In message <201311301642.36531.marex@denx.de> you wrote:
> > Dear Vladimir Zapolskiy,
> > 
> > > For LPC32XX high-speed UART it is required to send a carriage return
> > > symbol along with line feed.
> > 
> > Why ?
> 
> Because we are actually emulating a classical type writer here, and to
> start printing at the beginning of a new line requires two separate
> actions: performing a line feed (i. e. scrolling the paper one line
> up) and a crriage return (i. e. repositioning the drum such that the
> next character will be printed in column 1.
> 
> In the strict sense, the ASCII characters represent the line feed and
> '\r' the carriage return, respectively.  To print the equivalent of
> the "new line" as it is interpreted by the C standard, we have to
> translate the single C character into a two-character sequence.  You
> can trace this back to the very beginnings of Unix; you can see the
> same in Unix version 6 drivers, for example.

Thanks for such a nice explanations ;-) I still have to wonder, why do we not do 
this emulation in the serial subsystem core now, but still have such a 
duplication of code in drivers ? I guess I can answer this myself though:

a) We didn't clean this up, even if we do now have a CONFIG_SERIAL_MULTI enabled 
by default
b) There might still be people invoking the driver functions directly

There might be even more reasons, but OK, I guess I get it now.

Best regards,
Marek Vasut

[U-Boot] [PATCH] serial: lpc32xx: send CR before LF

[Marek Vasut]

Dear Vladimir Zapolskiy,

> On 11/30/13 17:42, Marek Vasut wrote:
> > Dear Vladimir Zapolskiy,
> > 
> >> For LPC32XX high-speed UART it is required to send a carriage return
> >> symbol along with line feed.
> > 
> > Why ?
> > 
> >> The problem was introduced in e503f90a
> >> commit.
> >> 
> >> Signed-off-by: Vladimir Zapolskiy<vz@mleia.com>
> >> Cc: Marek Vasut<marex@denx.de>
> >> ---
> >> 
> >>   drivers/serial/lpc32xx_hsuart.c |    3 +++
> >>   1 file changed, 3 insertions(+)
> >> 
> >> diff --git a/drivers/serial/lpc32xx_hsuart.c
> >> b/drivers/serial/lpc32xx_hsuart.c index 9c7c621..c8926a8 100644
> >> --- a/drivers/serial/lpc32xx_hsuart.c
> >> +++ b/drivers/serial/lpc32xx_hsuart.c
> >> @@ -38,6 +38,9 @@ static int lpc32xx_serial_getc(void)
> >> 
> >>   static void lpc32xx_serial_putc(const char c)
> >>   {
> >> 
> >> +	if (c == '\n')
> >> +		serial_putc('\r');
> >> +
> >> 
> >>   	writel(c,&hsuart->tx);
> >>   	
> >>   	/* Wait for character to be sent */
> > 
> > btw. I have a feeling each and every driver does send CR-LF combo and
> > emulates it this way. Maybe this should eventually go into common code ?
> > But that's just a suggestion for future improvement.
> 
> No objections from my side, however it seems that not every particular
> serial
> controller requires such a hook, for code generalization it may be good
> to have
> it for all drivers, but strict enforcement may be redundant for some
> subset.

This would need further investigation, very detailed one.

Best regards,
Marek Vasut

[U-Boot] [PATCH] serial: lpc32xx: send CR before LF

[Vladimir Zapolskiy]

Dear Marek Vasut,

On 12/01/13 01:06, Marek Vasut wrote:
> Dear Vladimir Zapolskiy,
>
>> On 11/30/13 17:42, Marek Vasut wrote:
>>> Dear Vladimir Zapolskiy,
>>>
>>>> For LPC32XX high-speed UART it is required to send a carriage return
>>>> symbol along with line feed.
>>>
>>> Why ?
>>>
>>>> The problem was introduced in e503f90a
>>>> commit.
>>>>
>>>> Signed-off-by: Vladimir Zapolskiy<vz@mleia.com>
>>>> Cc: Marek Vasut<marex@denx.de>
>>>> ---
>>>>
>>>>    drivers/serial/lpc32xx_hsuart.c |    3 +++
>>>>    1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/serial/lpc32xx_hsuart.c
>>>> b/drivers/serial/lpc32xx_hsuart.c index 9c7c621..c8926a8 100644
>>>> --- a/drivers/serial/lpc32xx_hsuart.c
>>>> +++ b/drivers/serial/lpc32xx_hsuart.c
>>>> @@ -38,6 +38,9 @@ static int lpc32xx_serial_getc(void)
>>>>
>>>>    static void lpc32xx_serial_putc(const char c)
>>>>    {
>>>>
>>>> +	if (c == '\n')
>>>> +		serial_putc('\r');
>>>> +
>>>>
>>>>    	writel(c,&hsuart->tx);
>>>>    	
>>>>    	/* Wait for character to be sent */
>>>
>>> btw. I have a feeling each and every driver does send CR-LF combo and
>>> emulates it this way. Maybe this should eventually go into common code ?
>>> But that's just a suggestion for future improvement.
>>
>> No objections from my side, however it seems that not every particular
>> serial
>> controller requires such a hook, for code generalization it may be good
>> to have
>> it for all drivers, but strict enforcement may be redundant for some
>> subset.
>
> This would need further investigation, very detailed one.

if you anticipate that such investigation and testing will take significant
amount of time, I'd be glad, if you let the regression fix in before doing
code generalization.

With best wishes,
Vladimir

[U-Boot] [PATCH] serial: lpc32xx: send CR before LF

[Marek Vasut]

Dear Vladimir Zapolskiy,

> Dear Marek Vasut,
> 
> On 12/01/13 01:06, Marek Vasut wrote:
> > Dear Vladimir Zapolskiy,
> > 
> >> On 11/30/13 17:42, Marek Vasut wrote:
> >>> Dear Vladimir Zapolskiy,
> >>> 
> >>>> For LPC32XX high-speed UART it is required to send a carriage return
> >>>> symbol along with line feed.
> >>> 
> >>> Why ?
> >>> 
> >>>> The problem was introduced in e503f90a
> >>>> commit.
> >>>> 
> >>>> Signed-off-by: Vladimir Zapolskiy<vz@mleia.com>
> >>>> Cc: Marek Vasut<marex@denx.de>
> >>>> ---
> >>>> 
> >>>>    drivers/serial/lpc32xx_hsuart.c |    3 +++
> >>>>    1 file changed, 3 insertions(+)
> >>>> 
> >>>> diff --git a/drivers/serial/lpc32xx_hsuart.c
> >>>> b/drivers/serial/lpc32xx_hsuart.c index 9c7c621..c8926a8 100644
> >>>> --- a/drivers/serial/lpc32xx_hsuart.c
> >>>> +++ b/drivers/serial/lpc32xx_hsuart.c
> >>>> @@ -38,6 +38,9 @@ static int lpc32xx_serial_getc(void)
> >>>> 
> >>>>    static void lpc32xx_serial_putc(const char c)
> >>>>    {
> >>>> 
> >>>> +	if (c == '\n')
> >>>> +		serial_putc('\r');
> >>>> +
> >>>> 
> >>>>    	writel(c,&hsuart->tx);
> >>>>    	
> >>>>    	/* Wait for character to be sent */
> >>> 
> >>> btw. I have a feeling each and every driver does send CR-LF combo and
> >>> emulates it this way. Maybe this should eventually go into common code
> >>> ? But that's just a suggestion for future improvement.
> >> 
> >> No objections from my side, however it seems that not every particular
> >> serial
> >> controller requires such a hook, for code generalization it may be good
> >> to have
> >> it for all drivers, but strict enforcement may be redundant for some
> >> subset.
> > 
> > This would need further investigation, very detailed one.
> 
> if you anticipate that such investigation and testing will take significant
> amount of time, I'd be glad, if you let the regression fix in before doing
> code generalization.

By all means,

Acked-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

[U-Boot] serial: lpc32xx: send CR before LF

[Tom Rini]

On Sat, Nov 30, 2013 at 04:47:01PM +0200, Vladimir Zapolskiy wrote:

> For LPC32XX high-speed UART it is required to send a carriage return
> symbol along with line feed. The problem was introduced in e503f90a
> commit.
> 
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> Cc: Marek Vasut <marex@denx.de>
> Acked-by: Marek Vasut <marex@denx.de>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20131216/667ec170/attachment.pgp>