[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 at 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 at st.com> wrote:
> >> Hi Simon,
> >>
> >>> From: Simon Glass <sjg at chromium.org>
> >>> Sent: lundi 30 novembre 2020 21:12
> >>>
> >>> Hi Patrick,
> >>>
> >>> On Fri, 27 Nov 2020 at 03:49, Patrick Delaunay <patrick.delaunay at 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 at 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