[PATCH 1/2] configs: sandbox: activate DEBUG_UART

[PATCH 1/2] configs: sandbox: activate DEBUG_UART

[Patrick Delaunay]

Add CONFIG_DEBUG_UART=y for all sandbox defconfig
as it is already done in sandbox_defconfig.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 configs/sandbox64_defconfig        | 1 +
 configs/sandbox_flattree_defconfig | 1 +
 configs/sandbox_spl_defconfig      | 1 +
 3 files changed, 3 insertions(+)

diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
index dc993cd13a..3bcf472a47 100644
--- a/configs/sandbox64_defconfig
+++ b/configs/sandbox64_defconfig
@@ -5,6 +5,7 @@ CONFIG_PRE_CON_BUF_ADDR=0x100000
 CONFIG_BOOTSTAGE_STASH_ADDR=0x0
 CONFIG_DEFAULT_DEVICE_TREE="sandbox64"
 CONFIG_SANDBOX64=y
+CONFIG_DEBUG_UART=y
 CONFIG_DISTRO_DEFAULTS=y
 CONFIG_FIT=y
 CONFIG_FIT_SIGNATURE=y
diff --git a/configs/sandbox_flattree_defconfig b/configs/sandbox_flattree_defconfig
index 1f593eba8f..4ac72aa5ce 100644
--- a/configs/sandbox_flattree_defconfig
+++ b/configs/sandbox_flattree_defconfig
@@ -3,6 +3,7 @@ CONFIG_NR_DRAM_BANKS=1
 CONFIG_ENV_SIZE=0x2000
 CONFIG_BOOTSTAGE_STASH_ADDR=0x0
 CONFIG_DEFAULT_DEVICE_TREE="sandbox"
+CONFIG_DEBUG_UART=y
 CONFIG_DISTRO_DEFAULTS=y
 CONFIG_FIT=y
 CONFIG_FIT_SIGNATURE=y
diff --git a/configs/sandbox_spl_defconfig b/configs/sandbox_spl_defconfig
index 1ac843e1bc..e3b435837b 100644
--- a/configs/sandbox_spl_defconfig
+++ b/configs/sandbox_spl_defconfig
@@ -9,6 +9,7 @@ CONFIG_SPL=y
 CONFIG_BOOTSTAGE_STASH_ADDR=0x0
 CONFIG_DEFAULT_DEVICE_TREE="sandbox"
 CONFIG_SANDBOX_SPL=y
+CONFIG_DEBUG_UART=y
 CONFIG_DISTRO_DEFAULTS=y
 CONFIG_FIT=y
 CONFIG_FIT_SIGNATURE=y
-- 
2.17.1

[PATCH 2/2] console: sandbox: remove unnecessary sandbox code

[Patrick Delaunay]

Remove the specific sandbox code in console.c, as the config
CONFIG_DEBUG_UART is already supported in drivers/serial/sandbox.c
and activated by default in all sandbox defconfig
(CONFIG_DEBUG_UART_SANDBOX=y and CONFIG_DEBUG_UART=y).

This patch allows to test the console code under DEBUG_UART in sandbox
and avoids to include the file <os.h> in this u-boot generic code.

Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
---

 common/console.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/common/console.c b/common/console.c
index 3348436da6..11d3694159 100644
--- a/common/console.c
+++ b/common/console.c
@@ -13,7 +13,6 @@
 #include <iomux.h>
 #include <malloc.h>
 #include <mapmem.h>
-#include <os.h>
 #include <serial.h>
 #include <stdio_dev.h>
 #include <exports.h>
@@ -517,13 +516,6 @@ static inline void print_pre_console_buffer(int flushpoint) {}
 
 void putc(const char c)
 {
-#ifdef CONFIG_SANDBOX
-	/* sandbox can send characters to stdout before it has a console */
-	if (!gd || !(gd->flags & GD_FLG_SERIAL_READY)) {
-		os_putc(c);
-		return;
-	}
-#endif
 #ifdef CONFIG_DEBUG_UART
 	/* if we don't have a console yet, use the debug UART */
 	if (!gd || !(gd->flags & GD_FLG_SERIAL_READY)) {
@@ -565,13 +557,6 @@ void putc(const char c)
 
 void puts(const char *s)
 {
-#ifdef CONFIG_SANDBOX
-	/* sandbox can send characters to stdout before it has a console */
-	if (!gd || !(gd->flags & GD_FLG_SERIAL_READY)) {
-		os_puts(s);
-		return;
-	}
-#endif
 #ifdef CONFIG_DEBUG_UART
 	if (!gd || !(gd->flags & GD_FLG_SERIAL_READY)) {
 		while (*s) {
-- 
2.17.1

[PATCH 1/2] configs: sandbox: activate DEBUG_UART

[Simon Glass]

On Fri, 27 Nov 2020 at 03:49, Patrick Delaunay <patrick.delaunay@st.com> wrote:
>
> Add CONFIG_DEBUG_UART=y for all sandbox defconfig
> as it is already done in sandbox_defconfig.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>
>  configs/sandbox64_defconfig        | 1 +
>  configs/sandbox_flattree_defconfig | 1 +
>  configs/sandbox_spl_defconfig      | 1 +
>  3 files changed, 3 insertions(+)

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

[PATCH 2/2] console: sandbox: remove unnecessary sandbox code

[Simon Glass]

Hi Patrick,

On Fri, 27 Nov 2020 at 03:49, Patrick Delaunay <patrick.delaunay@st.com> wrote:
>
> Remove the specific sandbox code in console.c, as the config
> CONFIG_DEBUG_UART is already supported in drivers/serial/sandbox.c
> and activated by default in all sandbox defconfig
> (CONFIG_DEBUG_UART_SANDBOX=y and CONFIG_DEBUG_UART=y).
>
> This patch allows to test the console code under DEBUG_UART in sandbox
> and avoids to include the file <os.h> in this u-boot generic code.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>
>  common/console.c | 15 ---------------
>  1 file changed, 15 deletions(-)

Please see this commit as to why I put that code back, after removing it myself.

64e9b4f346f Revert "sandbox: Drop special case console code for sandbox"

Regards,
Simon

[PATCH 2/2] console: sandbox: remove unnecessary sandbox code

[Patrick DELAUNAY]

Hi Simon,

> From: Simon Glass <sjg@chromium.org>
> Sent: lundi 30 novembre 2020 21:12
> 
> Hi Patrick,
> 
> On Fri, 27 Nov 2020 at 03:49, Patrick Delaunay <patrick.delaunay@st.com>
> wrote:
> >
> > Remove the specific sandbox code in console.c, as the config
> > CONFIG_DEBUG_UART is already supported in drivers/serial/sandbox.c and
> > activated by default in all sandbox defconfig
> > (CONFIG_DEBUG_UART_SANDBOX=y and CONFIG_DEBUG_UART=y).
> >
> > This patch allows to test the console code under DEBUG_UART in sandbox
> > and avoids to include the file <os.h> in this u-boot generic code.
> >
> > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> > ---
> >
> >  common/console.c | 15 ---------------
> >  1 file changed, 15 deletions(-)
> 
> Please see this commit as to why I put that code back, after removing it myself.
> 
> 64e9b4f346f Revert "sandbox: Drop special case console code for sandbox"
>
> Regards,
> Simon

Thanks to point it, I miss this old commit.

I don't understood the issue in the commit message:

    Revert "sandbox: Drop special case console code for sandbox"
    
    While sandbox works OK without the special-case code, it does result in
    console output being stored in the pre-console buffer while sandbox starts
    up. If there is a crash or a problem then there is no indication of what
    is going on.
    
    For ease of debugging it seems better to revert this change also.

the existing code (here for putc, but it is the same for puts)  is:  

#ifdef CONFIG_SANDBOX
	/* sandbox can send characters to stdout before it has a console */
	if (!(gd->flags & GD_FLG_SERIAL_READY)) {
		os_putc(c);
		return;
	}
#endif
#ifdef CONFIG_DEBUG_UART
	/* if we don't have a console yet, use the debug UART */
	if (!(gd->flags & GD_FLG_SERIAL_READY)) {
		printch(c);
		return;
	}
#endif

For sandbox, when CONFIG_DEBUG_UART is activated
    printch => _printch => _debug_uart_putc => os_putc

For me these 2 code block are identical for sandbox when CONFIG_DEBUG_UART

And the  issue described is also solved by CONFIG_DEBUG_UART=y
(consle no use preconsole buffer when serial driver s not ready).

Your concern  is when sandbox is compiled without CONFIG_DEBUG_UART ?

Because it is no more the case with my previous patch (I activate it in sandbox*defconfig) 

but to avoid issue in futur (new sandbox*defconfig) it should be better to select (or imply)
his feature for sandbox  arch in Kconfig and not more activate it in sandbox*defconfig ?

PS: with this sandox code, I don't see how to test the pre console buffer in sandbox...
       I think that the pre console buffer is alway empty for sandbox



Regards,
Patrick

[PATCH 2/2] console: sandbox: remove unnecessary sandbox code

[Simon Glass]

Hi Patrick,

On Wed, 2 Dec 2020 at 07:08, Patrick DELAUNAY <patrick.delaunay@st.com> wrote:
>
> Hi Simon,
>
> > From: Simon Glass <sjg@chromium.org>
> > Sent: lundi 30 novembre 2020 21:12
> >
> > Hi Patrick,
> >
> > On Fri, 27 Nov 2020 at 03:49, Patrick Delaunay <patrick.delaunay@st.com>
> > wrote:
> > >
> > > Remove the specific sandbox code in console.c, as the config
> > > CONFIG_DEBUG_UART is already supported in drivers/serial/sandbox.c and
> > > activated by default in all sandbox defconfig
> > > (CONFIG_DEBUG_UART_SANDBOX=y and CONFIG_DEBUG_UART=y).
> > >
> > > This patch allows to test the console code under DEBUG_UART in sandbox
> > > and avoids to include the file <os.h> in this u-boot generic code.
> > >
> > > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> > > ---
> > >
> > >  common/console.c | 15 ---------------
> > >  1 file changed, 15 deletions(-)
> >
> > Please see this commit as to why I put that code back, after removing it myself.
> >
> > 64e9b4f346f Revert "sandbox: Drop special case console code for sandbox"
> >
> > Regards,
> > Simon
>
> Thanks to point it, I miss this old commit.
>
> I don't understood the issue in the commit message:
>
>     Revert "sandbox: Drop special case console code for sandbox"
>
>     While sandbox works OK without the special-case code, it does result in
>     console output being stored in the pre-console buffer while sandbox starts
>     up. If there is a crash or a problem then there is no indication of what
>     is going on.
>
>     For ease of debugging it seems better to revert this change also.
>
> the existing code (here for putc, but it is the same for puts)  is:
>
> #ifdef CONFIG_SANDBOX
>         /* sandbox can send characters to stdout before it has a console */
>         if (!(gd->flags & GD_FLG_SERIAL_READY)) {
>                 os_putc(c);
>                 return;
>         }
> #endif
> #ifdef CONFIG_DEBUG_UART
>         /* if we don't have a console yet, use the debug UART */
>         if (!(gd->flags & GD_FLG_SERIAL_READY)) {
>                 printch(c);
>                 return;
>         }
> #endif
>
> For sandbox, when CONFIG_DEBUG_UART is activated
>     printch => _printch => _debug_uart_putc => os_putc
>
> For me these 2 code block are identical for sandbox when CONFIG_DEBUG_UART
>
> And the  issue described is also solved by CONFIG_DEBUG_UART=y
> (consle no use preconsole buffer when serial driver s not ready).
>
> Your concern  is when sandbox is compiled without CONFIG_DEBUG_UART ?
>
> Because it is no more the case with my previous patch (I activate it in sandbox*defconfig)
>
> but to avoid issue in futur (new sandbox*defconfig) it should be better to select (or imply)
> his feature for sandbox  arch in Kconfig and not more activate it in sandbox*defconfig ?
>
> PS: with this sandox code, I don't see how to test the pre console buffer in sandbox...
>        I think that the pre console buffer is alway empty for sandbox
>

OK maybe things have changed. Previously I noticed that the banner did
not output until later. I will take another look.

But I don't want to rely on the debug UART for sandbox to work.

Regards,
Simon

[Uboot-stm32] [PATCH 2/2] console: sandbox: remove unnecessary sandbox code

[Patrick DELAUNAY]

Hi Simon,

On 12/12/20 4:39 PM, Simon Glass wrote:
> Hi Patrick,
>
> On Wed, 2 Dec 2020 at 07:08, Patrick DELAUNAY <patrick.delaunay@st.com> wrote:
>> Hi Simon,
>>
>>> From: Simon Glass <sjg@chromium.org>
>>> Sent: lundi 30 novembre 2020 21:12
>>>
>>> Hi Patrick,
>>>
>>> On Fri, 27 Nov 2020 at 03:49, Patrick Delaunay <patrick.delaunay@st.com>
>>> wrote:
>>>> Remove the specific sandbox code in console.c, as the config
>>>> CONFIG_DEBUG_UART is already supported in drivers/serial/sandbox.c and
>>>> activated by default in all sandbox defconfig
>>>> (CONFIG_DEBUG_UART_SANDBOX=y and CONFIG_DEBUG_UART=y).
>>>>
>>>> This patch allows to test the console code under DEBUG_UART in sandbox
>>>> and avoids to include the file <os.h> in this u-boot generic code.
>>>>
>>>> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
>>>> ---
>>>>
>>>>   common/console.c | 15 ---------------
>>>>   1 file changed, 15 deletions(-)
>>> Please see this commit as to why I put that code back, after removing it myself.
>>>
>>> 64e9b4f346f Revert "sandbox: Drop special case console code for sandbox"
>>>
>>> Regards,
>>> Simon
>> Thanks to point it, I miss this old commit.
>>
>> I don't understood the issue in the commit message:
>>
>>      Revert "sandbox: Drop special case console code for sandbox"
>>
>>      While sandbox works OK without the special-case code, it does result in
>>      console output being stored in the pre-console buffer while sandbox starts
>>      up. If there is a crash or a problem then there is no indication of what
>>      is going on.
>>
>>      For ease of debugging it seems better to revert this change also.
>>
>> the existing code (here for putc, but it is the same for puts)  is:
>>
>> #ifdef CONFIG_SANDBOX
>>          /* sandbox can send characters to stdout before it has a console */
>>          if (!(gd->flags & GD_FLG_SERIAL_READY)) {
>>                  os_putc(c);
>>                  return;
>>          }
>> #endif
>> #ifdef CONFIG_DEBUG_UART
>>          /* if we don't have a console yet, use the debug UART */
>>          if (!(gd->flags & GD_FLG_SERIAL_READY)) {
>>                  printch(c);
>>                  return;
>>          }
>> #endif
>>
>> For sandbox, when CONFIG_DEBUG_UART is activated
>>      printch => _printch => _debug_uart_putc => os_putc
>>
>> For me these 2 code block are identical for sandbox when CONFIG_DEBUG_UART
>>
>> And the  issue described is also solved by CONFIG_DEBUG_UART=y
>> (consle no use preconsole buffer when serial driver s not ready).
>>
>> Your concern  is when sandbox is compiled without CONFIG_DEBUG_UART ?
>>
>> Because it is no more the case with my previous patch (I activate it in sandbox*defconfig)
>>
>> but to avoid issue in futur (new sandbox*defconfig) it should be better to select (or imply)
>> his feature for sandbox  arch in Kconfig and not more activate it in sandbox*defconfig ?
>>
>> PS: with this sandox code, I don't see how to test the pre console buffer in sandbox...
>>         I think that the pre console buffer is alway empty for sandbox
>>
> OK maybe things have changed. Previously I noticed that the banner did
> not output until later. I will take another look.
>
> But I don't want to rely on the debug UART for sandbox to work.

Ok, it is not problem for me (avoid assumption on CONFIG_DEBUG_UART for 
sandbox)

you can consider that I abandon this change.

I will only replace

	#ifdef CONFIG_SANDBOX

by

	if (IS_ACTIVATED(CONFIG_SANDBOX))

in the other console the cleanup serie "console: remove #ifdef CONFIG when it is possible"

	http://patchwork.ozlabs.org/project/uboot/list/?series=218309

Regards,

Patrick

[Uboot-stm32] [PATCH 2/2] console: sandbox: remove unnecessary sandbox code

[Simon Glass]

Hi Patrick,

On Tue, 15 Dec 2020 at 07:35, Patrick DELAUNAY
<patrick.delaunay@foss.st.com> wrote:
>
> Hi Simon,
>
> On 12/12/20 4:39 PM, Simon Glass wrote:
> > Hi Patrick,
> >
> > On Wed, 2 Dec 2020 at 07:08, Patrick DELAUNAY <patrick.delaunay@st.com> wrote:
> >> Hi Simon,
> >>
> >>> From: Simon Glass <sjg@chromium.org>
> >>> Sent: lundi 30 novembre 2020 21:12
> >>>
> >>> Hi Patrick,
> >>>
> >>> On Fri, 27 Nov 2020 at 03:49, Patrick Delaunay <patrick.delaunay@st.com>
> >>> wrote:
> >>>> Remove the specific sandbox code in console.c, as the config
> >>>> CONFIG_DEBUG_UART is already supported in drivers/serial/sandbox.c and
> >>>> activated by default in all sandbox defconfig
> >>>> (CONFIG_DEBUG_UART_SANDBOX=y and CONFIG_DEBUG_UART=y).
> >>>>
> >>>> This patch allows to test the console code under DEBUG_UART in sandbox
> >>>> and avoids to include the file <os.h> in this u-boot generic code.
> >>>>
> >>>> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> >>>> ---
> >>>>
> >>>>   common/console.c | 15 ---------------
> >>>>   1 file changed, 15 deletions(-)
> >>> Please see this commit as to why I put that code back, after removing it myself.
> >>>
> >>> 64e9b4f346f Revert "sandbox: Drop special case console code for sandbox"
> >>>
> >>> Regards,
> >>> Simon
> >> Thanks to point it, I miss this old commit.
> >>
> >> I don't understood the issue in the commit message:
> >>
> >>      Revert "sandbox: Drop special case console code for sandbox"
> >>
> >>      While sandbox works OK without the special-case code, it does result in
> >>      console output being stored in the pre-console buffer while sandbox starts
> >>      up. If there is a crash or a problem then there is no indication of what
> >>      is going on.
> >>
> >>      For ease of debugging it seems better to revert this change also.
> >>
> >> the existing code (here for putc, but it is the same for puts)  is:
> >>
> >> #ifdef CONFIG_SANDBOX
> >>          /* sandbox can send characters to stdout before it has a console */
> >>          if (!(gd->flags & GD_FLG_SERIAL_READY)) {
> >>                  os_putc(c);
> >>                  return;
> >>          }
> >> #endif
> >> #ifdef CONFIG_DEBUG_UART
> >>          /* if we don't have a console yet, use the debug UART */
> >>          if (!(gd->flags & GD_FLG_SERIAL_READY)) {
> >>                  printch(c);
> >>                  return;
> >>          }
> >> #endif
> >>
> >> For sandbox, when CONFIG_DEBUG_UART is activated
> >>      printch => _printch => _debug_uart_putc => os_putc
> >>
> >> For me these 2 code block are identical for sandbox when CONFIG_DEBUG_UART
> >>
> >> And the  issue described is also solved by CONFIG_DEBUG_UART=y
> >> (consle no use preconsole buffer when serial driver s not ready).
> >>
> >> Your concern  is when sandbox is compiled without CONFIG_DEBUG_UART ?
> >>
> >> Because it is no more the case with my previous patch (I activate it in sandbox*defconfig)
> >>
> >> but to avoid issue in futur (new sandbox*defconfig) it should be better to select (or imply)
> >> his feature for sandbox  arch in Kconfig and not more activate it in sandbox*defconfig ?
> >>
> >> PS: with this sandox code, I don't see how to test the pre console buffer in sandbox...
> >>         I think that the pre console buffer is alway empty for sandbox
> >>
> > OK maybe things have changed. Previously I noticed that the banner did
> > not output until later. I will take another look.
> >
> > But I don't want to rely on the debug UART for sandbox to work.
>
> Ok, it is not problem for me (avoid assumption on CONFIG_DEBUG_UART for
> sandbox)
>
> you can consider that I abandon this change.
>
> I will only replace
>
>         #ifdef CONFIG_SANDBOX
>
> by
>
>         if (IS_ACTIVATED(CONFIG_SANDBOX))
>
> in the other console the cleanup serie "console: remove #ifdef CONFIG when it is possible"
>
>         http://patchwork.ozlabs.org/project/uboot/list/?series=218309

OK good.

Just to close the loop, if I disable the debug UART, and put a
printf() in a suitable function before console_init_f() -
e.g.initf_bootstage()

Then print_pre_console_buffer() prints out the text, but it does not
appear before that. I consider that confusing/annoying when using gdb
with sandbox, which I why I reverted my original patch.

I am not a fan of special-case sandbox code, and there is probably a
nicer way of doing it, but I do think having this special case is
worth it.

Regards,
Simon

[PATCH 1/2] configs: sandbox: activate DEBUG_UART

[Simon Glass]

On Fri, 27 Nov 2020 at 03:49, Patrick Delaunay <patrick.delaunay@st.com> wrote:
>
> Add CONFIG_DEBUG_UART=y for all sandbox defconfig
> as it is already done in sandbox_defconfig.
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com>
> ---
>
>  configs/sandbox64_defconfig        | 1 +
>  configs/sandbox_flattree_defconfig | 1 +
>  configs/sandbox_spl_defconfig      | 1 +
>  3 files changed, 3 insertions(+)

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

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