[U-Boot] [PATCH] debug_uart: output CR along with LF

[U-Boot] [PATCH] debug_uart: output CR along with LF

[Tim Chick]

Hi Masahiro,

This patch breaks the debug_uart on my MIPS board. It means printascii now uses the stack, and my board does not have a stack when debug_uart_init is called. debug_uart_init calls printascii if DEBUG_UART_ANNOUNCE is defined.

The patch below fixes it, and keeps your change:

Thanks,
Tim


---

diff --git a/include/debug_uart.h b/include/debug_uart.h
index 0d640b9..2980ae6 100644
--- a/include/debug_uart.h
+++ b/include/debug_uart.h
@@ -115,17 +115,23 @@ void printhex8(uint value);
  * Now define some functions - this should be inserted into the serial driver
  */
 #define DEBUG_UART_FUNCS \
-	void printch(int ch) \
+\
+	static inline void _printch(int ch) \
 	{ \
 		if (ch == '\n') \
 			_debug_uart_putc('\r'); \
 		_debug_uart_putc(ch); \
 	} \
 \
+	void printch(int ch) \
+	{ \
+		_printch(ch); \
+	} \
+\
 	void printascii(const char *str) \
 	{ \
 		while (*str) \
-			printch(*str++); \
+			_printch(*str++); \
 	} \
 \
 	static inline void printhex1(uint digit) \

[U-Boot] [PATCH] debug_uart: output CR along with LF

[Masahiro Yamada]

Hi Tim,

2016-04-05 0:16 GMT+09:00 Tim Chick <Tim.Chick@mediatek.com>:
> Hi Masahiro,
>
> This patch breaks the debug_uart on my MIPS board. It means printascii now uses the stack, and my board does not have a stack when debug_uart_init is called. debug_uart_init calls printascii if DEBUG_UART_ANNOUNCE is defined.
>
> The patch below fixes it, and keeps your change:
>

I think the current implementation of debug_uart
requires the stack, anyway.

I am not sure your patch is the right approach
because your solution seems dependent on your compiler behavior.

I tried your patch with CONFIG_DEBUG_UART_ANNOUNCE=y,
but my ARM compiler still produced debug_uart_init
code with the stack.


00101004 <printascii>:
  101004:       e92d4038        push    {r3, r4, r5, lr}
  101008:       e2405001        sub     r5, r0, #1
  10100c:       ea000005        b       101028 <printascii+0x24>
  101010:       e354000a        cmp     r4, #10
  101014:       1a000001        bne     101020 <printascii+0x1c>
  101018:       e3a0000d        mov     r0, #13
  10101c:       ebffffdf        bl      100fa0 <_debug_uart_putc>
  101020:       e1a00004        mov     r0, r4
  101024:       ebffffdd        bl      100fa0 <_debug_uart_putc>
  101028:       e5f54001        ldrb    r4, [r5, #1]!
  10102c:       e3540000        cmp     r4, #0
  101030:       1afffff6        bne     101010 <printascii+0xc>
  101034:       e8bd8038        pop     {r3, r4, r5, pc}

00101038 <debug_uart_init>:
  101038:       e92d4008        push    {r3, lr}
  10103c:       ebffffdf        bl      100fc0 <_debug_uart_init>
  101040:       e59f0004        ldr     r0, [pc, #4]    ; 10104c
<debug_uart_init+0x14>
  101044:       e8bd4008        pop     {r3, lr}
  101048:       eaffffed        b       101004 <printascii>
  10104c:       0010a079        .word   0x0010a079




-- 
Best Regards
Masahiro Yamada

[U-Boot] [PATCH] debug_uart: output CR along with LF

[Daniel Schwierzeck]

Hi Tim,

2016-04-04 17:16 GMT+02:00 Tim Chick <Tim.Chick@mediatek.com>:
> Hi Masahiro,
>
> This patch breaks the debug_uart on my MIPS board. It means printascii now uses the stack, and my board does not have a stack when debug_uart_init is called. debug_uart_init calls printascii if DEBUG_UART_ANNOUNCE is defined.

do you call debug_uart_init() in lowlevel_init()?

>
> The patch below fixes it, and keeps your change:
>
> Thanks,
> Tim
>
>
> ---
>
> diff --git a/include/debug_uart.h b/include/debug_uart.h
> index 0d640b9..2980ae6 100644
> --- a/include/debug_uart.h
> +++ b/include/debug_uart.h
> @@ -115,17 +115,23 @@ void printhex8(uint value);
>   * Now define some functions - this should be inserted into the serial driver
>   */
>  #define DEBUG_UART_FUNCS \
> -       void printch(int ch) \
> +\
> +       static inline void _printch(int ch) \
>         { \
>                 if (ch == '\n') \
>                         _debug_uart_putc('\r'); \
>                 _debug_uart_putc(ch); \
>         } \
>  \
> +       void printch(int ch) \
> +       { \
> +               _printch(ch); \
> +       } \
> +\
>         void printascii(const char *str) \
>         { \
>                 while (*str) \
> -                       printch(*str++); \
> +                       _printch(*str++); \
>         } \
>  \
>         static inline void printhex1(uint digit) \
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot



-- 
- Daniel

[U-Boot] [PATCH] debug_uart: output CR along with LF

[Tim Chick]

Please see below:

> 
> 
> Hi Tim,
> 
> 2016-04-05 0:16 GMT+09:00 Tim Chick <Tim.Chick@mediatek.com>:
>> Hi Masahiro,
>>
>> This patch breaks the debug_uart on my MIPS board. It means printascii now uses the stack, and my board does not have a stack when debug_uart_init is called. debug_uart_init calls printascii if DEBUG_UART_ANNOUNCE is defined.
>>
>> The patch below fixes it, and keeps your change:
>>
> 
> I think the current implementation of debug_uart
> requires the stack, anyway.
> 
No, I'm quite sure it does not, on MIPS at least. I do not have a stack.
My platform has no SRAM, so I don't have a stack until I can get the
DRAM working.

> I am not sure your patch is the right approach
> because your solution seems dependent on your compiler behavior.
>
I believe it just relies on inline, which is used elsewhere to enable
debug_uart to work without a stack.

> I tried your patch with CONFIG_DEBUG_UART_ANNOUNCE=y,
> but my ARM compiler still produced debug_uart_init
> code with the stack.
> 

It shouldn't do, it should honor the inline keyword I think. I'm using
gcc on mips, and get (with my code):

9c023598 <printascii>:
9c023598:       2406000a        li      a2,10
9c02359c:       3c02b000        lui     v0,0xb000
9c0235a0:       1000000e        b       9c0235dc <printascii+0x44>
9c0235a4:       2405000d        li      a1,13
9c0235a8:       14660006        bne     v1,a2,9c0235c4 <printascii+0x2c>
9c0235ac:       24840001        addiu   a0,a0,1
9c0235b0:       90480c14        lbu     t0,3092(v0)

static inline void _debug_uart_putc(int ch)
{
        struct NS16550 *com_port = (struct NS16550 *)CONFIG_DEBUG_UART_BASE;

        while (!(serial_din(&com_port->lsr) & UART_LSR_THRE))
9c0235b4:       31080020        andi    t0,t0,0x20
9c0235b8:       1100fffd        beqz    t0,9c0235b0 <printascii+0x18>
9c0235bc:       00000000        nop
9c0235c0:       a0450c00        sb      a1,3072(v0)
9c0235c4:       90480c14        lbu     t0,3092(v0)

...


You can see it inlines the code. With your change, it pushed data to the
stack and called printascii as a subroutine.

Thanks,
Tim

[U-Boot] [PATCH] debug_uart: output CR along with LF

[Tim Chick]

Sorry for top posting. Not in the office at the moment.

Yes, I call debug_uart_init() before I have SDRAM, in lowlevel_init(). I need the debug uart to help me debug lowlevel_init!

Thanks,
Tim


-----Original Message-----
From: Daniel Schwierzeck [mailto:daniel.schwierzeck at gmail.com] 
Sent: 07 April 2016 17:48
To: Tim Chick <Tim.Chick@mediatek.com>
Cc: yamada.masahiro at socionext.com; u-boot at lists.denx.de; Simon Glass <sjg@chromium.org>; Stefan Roese <sr@denx.de>
Subject: Re: [U-Boot] [PATCH] debug_uart: output CR along with LF

Hi Tim,

2016-04-04 17:16 GMT+02:00 Tim Chick <Tim.Chick@mediatek.com>:
> Hi Masahiro,
>
> This patch breaks the debug_uart on my MIPS board. It means printascii now uses the stack, and my board does not have a stack when debug_uart_init is called. debug_uart_init calls printascii if DEBUG_UART_ANNOUNCE is defined.

do you call debug_uart_init() in lowlevel_init()?

>
> The patch below fixes it, and keeps your change:
>
> Thanks,
> Tim
>
>
> ---
>
> diff --git a/include/debug_uart.h b/include/debug_uart.h
> index 0d640b9..2980ae6 100644
> --- a/include/debug_uart.h
> +++ b/include/debug_uart.h
> @@ -115,17 +115,23 @@ void printhex8(uint value);
>   * Now define some functions - this should be inserted into the serial driver
>   */
>  #define DEBUG_UART_FUNCS \
> -       void printch(int ch) \
> +\
> +       static inline void _printch(int ch) \
>         { \
>                 if (ch == '\n') \
>                         _debug_uart_putc('\r'); \
>                 _debug_uart_putc(ch); \
>         } \
>  \
> +       void printch(int ch) \
> +       { \
> +               _printch(ch); \
> +       } \
> +\
>         void printascii(const char *str) \
>         { \
>                 while (*str) \
> -                       printch(*str++); \
> +                       _printch(*str++); \
>         } \
>  \
>         static inline void printhex1(uint digit) \
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot



-- 
- Daniel

[U-Boot] [PATCH] debug_uart: output CR along with LF

[Simon Glass]

Hi Tim,

On 7 April 2016 at 11:20, Tim Chick <Tim.Chick@mediatek.com> wrote:
> Sorry for top posting. Not in the office at the moment.
>
> Yes, I call debug_uart_init() before I have SDRAM, in lowlevel_init(). I
> need the debug uart to help me debug lowlevel_init!
>
> Thanks,
> Tim
>
>
> -----Original Message-----
> From: Daniel Schwierzeck [mailto:daniel.schwierzeck at gmail.com]
> Sent: 07 April 2016 17:48
> To: Tim Chick <Tim.Chick@mediatek.com>
> Cc: yamada.masahiro at socionext.com; u-boot at lists.denx.de; Simon Glass
> <sjg@chromium.org>; Stefan Roese <sr@denx.de>
> Subject: Re: [U-Boot] [PATCH] debug_uart: output CR along with LF
>
> Hi Tim,
>
> 2016-04-04 17:16 GMT+02:00 Tim Chick <Tim.Chick@mediatek.com>:
>> Hi Masahiro,
>>
>> This patch breaks the debug_uart on my MIPS board. It means printascii now
>> uses the stack, and my board does not have a stack when debug_uart_init is
>> called. debug_uart_init calls printascii if DEBUG_UART_ANNOUNCE is defined.
>
> do you call debug_uart_init() in lowlevel_init()?
>
>>
>> The patch below fixes it, and keeps your change:

Yes your patch looks correct to me. I have also used the debug UART
without a stack.

Reviewed-by: Simon Glass <sjg@chromium.org>

>>
>> Thanks,
>> Tim
>>
>>
>> ---
>>
>> diff --git a/include/debug_uart.h b/include/debug_uart.h
>> index 0d640b9..2980ae6 100644
>> --- a/include/debug_uart.h
>> +++ b/include/debug_uart.h
>> @@ -115,17 +115,23 @@ void printhex8(uint value);
>>   * Now define some functions - this should be inserted into the serial
>> driver
>>   */
>>  #define DEBUG_UART_FUNCS \
>> -       void printch(int ch) \
>> +\
>> +       static inline void _printch(int ch) \
>>         { \
>>                 if (ch == '\n') \
>>                         _debug_uart_putc('\r'); \
>>                 _debug_uart_putc(ch); \
>>         } \
>>  \
>> +       void printch(int ch) \
>> +       { \
>> +               _printch(ch); \
>> +       } \
>> +\
>>         void printascii(const char *str) \
>>         { \
>>                 while (*str) \
>> -                       printch(*str++); \
>> +                       _printch(*str++); \
>>         } \
>>  \
>>         static inline void printhex1(uint digit) \
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>
>
>
> --
> - Daniel
>
> ************* Email Confidentiality Notice ********************
> The information contained in this e-mail message (including any
> attachments) may be confidential, proprietary, privileged, or otherwise
> exempt from disclosure under applicable laws. It is intended to be
> conveyed only to the designated recipient(s). Any use, dissemination,
> distribution, printing, retaining or copying of this e-mail (including its
> attachments) by unintended recipient(s) is strictly prohibited and may
> be unlawful. If you are not an intended recipient of this e-mail, or believe
> that you have received this e-mail in error, please notify the sender
> immediately (by replying to this e-mail), delete any and all copies of
> this e-mail (including any attachments) from your system, and do not
> disclose the content of this e-mail to any other person. Thank you!

[U-Boot] [PATCH] debug_uart: output CR along with LF

[Tim Chick]

On 20/04/2016 15:40, Simon Glass wrote:
> Hi Tim,
> 
> On 7 April 2016 at 11:20, Tim Chick <Tim.Chick@mediatek.com> wrote:
>> Sorry for top posting. Not in the office at the moment.
>>
>> Yes, I call debug_uart_init() before I have SDRAM, in lowlevel_init(). I
>> need the debug uart to help me debug lowlevel_init!
>>> The patch below fixes it, and keeps your change:
> 
> Yes your patch looks correct to me. I have also used the debug UART
> without a stack.
>
OK. What needs to be done to get it applied?

Shall I submit as a "normal" patch?

Thanks,
Tim


> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
>>>
>>> Thanks,
>>> Tim
>>>
>>>
>>> ---
>>>
>>> diff --git a/include/debug_uart.h b/include/debug_uart.h
>>> index 0d640b9..2980ae6 100644
>>> --- a/include/debug_uart.h
>>> +++ b/include/debug_uart.h
>>> @@ -115,17 +115,23 @@ void printhex8(uint value);
>>>   * Now define some functions - this should be inserted into the serial
>>> driver
>>>   */
>>>  #define DEBUG_UART_FUNCS \
>>> -       void printch(int ch) \
>>> +\
>>> +       static inline void _printch(int ch) \
>>>         { \
>>>                 if (ch == '\n') \
>>>                         _debug_uart_putc('\r'); \
>>>                 _debug_uart_putc(ch); \
>>>         } \
>>>  \
>>> +       void printch(int ch) \
>>> +       { \
>>> +               _printch(ch); \
>>> +       } \
>>> +\
>>>         void printascii(const char *str) \
>>>         { \
>>>                 while (*str) \
>>> -                       printch(*str++); \
>>> +                       _printch(*str++); \
>>>         } \
>>>  \
>>>         static inline void printhex1(uint digit) \
>>> _______________________________________________
>>> U-Boot mailing list
>>> U-Boot at lists.denx.de
>>> http://lists.denx.de/mailman/listinfo/u-boot
>>
>>
>>

[U-Boot] [PATCH] debug_uart: output CR along with LF

[Simon Glass]

Hi Tom,

On 22 April 2016 at 12:19, Tim Chick <Tim.Chick@mediatek.com> wrote:
>
> On 20/04/2016 15:40, Simon Glass wrote:
>> Hi Tim,
>>
>> On 7 April 2016 at 11:20, Tim Chick <Tim.Chick@mediatek.com> wrote:
>>> Sorry for top posting. Not in the office at the moment.
>>>
>>> Yes, I call debug_uart_init() before I have SDRAM, in lowlevel_init(). I
>>> need the debug uart to help me debug lowlevel_init!
>>>> The patch below fixes it, and keeps your change:
>>
>> Yes your patch looks correct to me. I have also used the debug UART
>> without a stack.
>>
> OK. What needs to be done to get it applied?
>
> Shall I submit as a "normal" patch?

Yes, try using patman.

Regards,
Simon

>
> Thanks,
> Tim
>
>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>>>>
>>>> Thanks,
>>>> Tim
>>>>
>>>>
>>>> ---
>>>>
>>>> diff --git a/include/debug_uart.h b/include/debug_uart.h
>>>> index 0d640b9..2980ae6 100644
>>>> --- a/include/debug_uart.h
>>>> +++ b/include/debug_uart.h
>>>> @@ -115,17 +115,23 @@ void printhex8(uint value);
>>>>   * Now define some functions - this should be inserted into the serial
>>>> driver
>>>>   */
>>>>  #define DEBUG_UART_FUNCS \
>>>> -       void printch(int ch) \
>>>> +\
>>>> +       static inline void _printch(int ch) \
>>>>         { \
>>>>                 if (ch == '\n') \
>>>>                         _debug_uart_putc('\r'); \
>>>>                 _debug_uart_putc(ch); \
>>>>         } \
>>>>  \
>>>> +       void printch(int ch) \
>>>> +       { \
>>>> +               _printch(ch); \
>>>> +       } \
>>>> +\
>>>>         void printascii(const char *str) \
>>>>         { \
>>>>                 while (*str) \
>>>> -                       printch(*str++); \
>>>> +                       _printch(*str++); \
>>>>         } \
>>>>  \
>>>>         static inline void printhex1(uint digit) \
>>>> _______________________________________________
>>>> U-Boot mailing list
>>>> U-Boot at lists.denx.de
>>>> http://lists.denx.de/mailman/listinfo/u-boot
>>>
>>>
>>>
>
> ************* Email Confidentiality Notice ********************
> The information contained in this e-mail message (including any
> attachments) may be confidential, proprietary, privileged, or otherwise
> exempt from disclosure under applicable laws. It is intended to be
> conveyed only to the designated recipient(s). Any use, dissemination,
> distribution, printing, retaining or copying of this e-mail (including its
> attachments) by unintended recipient(s) is strictly prohibited and may
> be unlawful. If you are not an intended recipient of this e-mail, or believe
> that you have received this e-mail in error, please notify the sender
> immediately (by replying to this e-mail), delete any and all copies of
> this e-mail (including any attachments) from your system, and do not
> disclose the content of this e-mail to any other person. Thank you!

[U-Boot] [PATCH] debug_uart: output CR along with LF

[Simon Glass]

On 8 March 2016 at 16:33, Simon Glass <sjg@chromium.org> wrote:
> On 8 March 2016 at 02:19, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>>
>> The serial output from the debug UART carries on going far to the
>> right in the console.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>>  include/debug_uart.h | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>
>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot-dm/next, thanks!

[U-Boot] [PATCH] debug_uart: output CR along with LF

[Simon Glass]

On 8 March 2016 at 02:19, Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
> The serial output from the debug UART carries on going far to the
> right in the console.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
>  include/debug_uart.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)


Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH] debug_uart: output CR along with LF

[Stefan Roese]

On 08.03.2016 10:19, Masahiro Yamada wrote:
> The serial output from the debug UART carries on going far to the
> right in the console.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Yes, this has bugged me as well a few times as well. Thanks!

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH] debug_uart: output CR along with LF

[Masahiro Yamada]

The serial output from the debug UART carries on going far to the
right in the console.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 include/debug_uart.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/debug_uart.h b/include/debug_uart.h
index 5d5349b..0d640b9 100644
--- a/include/debug_uart.h
+++ b/include/debug_uart.h
@@ -117,13 +117,15 @@ void printhex8(uint value);
 #define DEBUG_UART_FUNCS \
 	void printch(int ch) \
 	{ \
+		if (ch == '\n') \
+			_debug_uart_putc('\r'); \
 		_debug_uart_putc(ch); \
 	} \
 \
 	void printascii(const char *str) \
 	{ \
 		while (*str) \
-			_debug_uart_putc(*str++); \
+			printch(*str++); \
 	} \
 \
 	static inline void printhex1(uint digit) \
-- 
1.9.1