[U-Boot] [PATCH v3 03/29] Convert CONSOLE_PRE_CONSOLE_BUFFER options to Kconfig

[Simon Glass]

Hi Siarhei,

On 30 September 2016 at 00:00, Siarhei Siamashka
<siarhei.siamashka at gmail.com> wrote:
>
> Hello Simon,
>
> On Thu, 29 Sep 2016 14:23:02 -0600
> Simon Glass <sjg at chromium.org> wrote:
>
> > Move these option to Kconfig and tidy up existing uses.
> >
> > Signed-off-by: Simon Glass <sjg at chromium.org>
> > ---
> >
> > Changes in v3: None
> > Changes in v2:
> > - Change CONFIG_PRE_CON_BUF_SZ default to 4096
> > - Change CONFIG_PRE_CON_BUF_SZ to 'int' type
> > - Drop the depend clause on the CONFIG_PRE_CON_BUF_SZ default
> > - Move CONFIG_PRE_CON_BUF_ADDR default to common/Kconfig
>
> What is the point moving these defines to Kconfig? They are neither
> user configurable, nor board specific. The location of the buffer is
> defined per platform or per SoC type and is a part of the global memory
> map. Similar to such things as the malloc heap and the stack.
>
> Allowing the users to redefine the buffer location is a dangerous thing
> because everything may go out of control very easily (it may overlap
> with some other memory buffer). IMHO it only makes sense to have a
> single user configurable boolean flag in Kconfig (the one which
> enables/disables the pre-console functionality).
>
> Regarding the buffer size. It was originally picked rather arbitrarily
> as 1MB at least for the sunxi platform:
>
>     https://patchwork.ozlabs.org/patch/426526/
>
> Just because making it several orders of magnitude larger than
> necessary makes it extremely unlikely that anyone ever gets into
> a buffer wraparound situation. Picking smallish sizes does not gain
> us anything, but just adds an extra hassle because now one needs to
> make some estimations whether the size is large enough or not.
> Especially considering that this functionality may be sometimes
> used for debugging prints when troubleshooting something. And one
> can't easily predict how much debugging output would be actually
> necessary.

All CONFIGs need to move to Kconfig or be deleted.

I also don't like the idea of specifying an address for a pre-console
buffer. As mentioned I think it is better to allocate it
automatically. But that is a separate issue from the Kconfig
conversion. if we re-litigate every CONFIG that we move, it will never
happen.

>
> >  README                            | 17 ----------------
> >  board/sunxi/Kconfig               |  3 +++
> >  common/Kconfig                    | 42 +++++++++++++++++++++++++++++++++++++++
> >  common/console.c                  |  6 +++---
> >  configs/tbs2910_defconfig         |  2 ++
> >  include/asm-generic/global_data.h |  2 +-
> >  include/configs/sunxi-common.h    |  6 ------
> >  include/configs/tbs2910.h         |  4 ----
> >  scripts/config_whitelist.txt      |  3 ---
> >  9 files changed, 51 insertions(+), 34 deletions(-)
> >
> > diff --git a/README b/README
> > index 0a1f3fe..8f93dad 100644
> > --- a/README
> > +++ b/README
> > @@ -872,23 +872,6 @@ The following options need to be configured:
> >               must be defined, to setup the maximum idle timeout for
> >               the SMC.
> >
> > -- Pre-Console Buffer:
> > -             Prior to the console being initialised (i.e. serial UART
> > -             initialised etc) all console output is silently discarded.
> > -             Defining CONFIG_PRE_CONSOLE_BUFFER will cause U-Boot to
> > -             buffer any console messages prior to the console being
> > -             initialised to a buffer of size CONFIG_PRE_CON_BUF_SZ
> > -             bytes located at CONFIG_PRE_CON_BUF_ADDR. The buffer is
> > -             a circular buffer, so if more than CONFIG_PRE_CON_BUF_SZ
> > -             bytes are output before the console is initialised, the
> > -             earlier bytes are discarded.
> > -
> > -             Note that when printing the buffer a copy is made on the
> > -             stack so CONFIG_PRE_CON_BUF_SZ must fit on the stack.
> > -
> > -             'Sane' compilers will generate smaller code if
> > -             CONFIG_PRE_CON_BUF_SZ is a power of 2
> > -
> >  - Autoboot Command:
> >               CONFIG_BOOTCOMMAND
> >               Only needed when CONFIG_BOOTDELAY is enabled;
> > diff --git a/board/sunxi/Kconfig b/board/sunxi/Kconfig
> > index b139d1c..c0ffeb3 100644
> > --- a/board/sunxi/Kconfig
> > +++ b/board/sunxi/Kconfig
> > @@ -3,6 +3,9 @@ if ARCH_SUNXI
> >  config IDENT_STRING
> >       default " Allwinner Technology"
> >
> > +config PRE_CONSOLE_BUFFER
> > +     default y
> > +
> >  config SPL_GPIO_SUPPORT
> >       default y
> >
> > diff --git a/common/Kconfig b/common/Kconfig
> > index bbd5633..6ee67ac 100644
> > --- a/common/Kconfig
> > +++ b/common/Kconfig
> > @@ -246,6 +246,48 @@ config SILENT_CONSOLE_UPDATE_ON_RELOC
> >         (e.g. NAND). This option makes the value of the 'silent'
> >         environment variable take effect at relocation.
> >
> > +config PRE_CONSOLE_BUFFER
> > +     bool "Buffer characters before the console is available"
> > +     help
> > +       Prior to the console being initialised (i.e. serial UART
> > +       initialised etc) all console output is silently discarded.
> > +       Defining CONFIG_PRE_CONSOLE_BUFFER will cause U-Boot to
> > +       buffer any console messages prior to the console being
> > +       initialised to a buffer. The buffer is a circular buffer, so
> > +       if it overflows, earlier output is discarded.
> > +
> > +       Note that this is not currently supported in SPL. It would be
> > +       useful to be able to share the pre-console buffer with SPL.
>
> We can't (easily) share this buffer with SPL. In the SPL case, this
> buffer needs to be placed somewhere in SRAM, because it is necessary
> to also have console messages even before DRAM is initialized.
> Essentially, we want to have two separate buffers. One small buffer
> somewhere in SRAM for SPL, and one very large buffer somewhere in
> DRAM for U-Boot proper. The data from the small buffer gets combined
> with the data from the large buffer at some point. The actual
> implementation is very straightforward and simple. And in fact it
> has been already available since a long time ago:
>
>     http://lists.denx.de/pipermail/u-boot/2015-January/201127.html
>     http://lists.denx.de/pipermail/u-boot/2015-January/201128.html
>     http://lists.denx.de/pipermail/u-boot/2015-January/201129.html
>
> Again, assigning the exact location of these buffers is a very platform
> specific information and is a part of the memory map. The user has no
> business overriding this via Kconfig.
>
> BTW, would you want me to rebase these old patches on the current
> U-Boot git master branch?

Yes please. I'd like to see this setup moved to board_init.c, with
just a size CONFIG options, not an address.

>
> > +
> > +config PRE_CON_BUF_SZ
> > +     int "Sets the size of the pre-console buffer"
> > +     depends on PRE_CONSOLE_BUFFER
> > +     default 4096
> > +     help
> > +       The size of the pre-console buffer affects how much console output
> > +       can be held before it overflows and starts discarding earlier
> > +       output. Normally there is very little output at this early stage,
> > +       unless debugging is enabled, so allow enough for ~10 lines of
> > +       text.
> > +
> > +       This is a useful feature if you are using a video console and
> > +       want to see the full boot output on the console. Without this
> > +       option only the post-relocation output will be displayed.
> > +
> > +config PRE_CON_BUF_ADDR
> > +     hex "Address of the pre-console buffer"
> > +     depends on PRE_CONSOLE_BUFFER
> > +     default 0x2f000000 if ARCH_SUNXI && MACH_SUN9I
> > +     default 0x4f000000 if ARCH_SUNXI && !MACH_SUN9I
> > +     help
> > +       This sets the start address of the pre-console buffer. This must
> > +       be in available memory and is accessed before relocation and
> > +       possibly before DRAM is set up. Therefore choose an address
> > +       carefully.
> > +
> > +       We should consider removing this option and allocating the memory
> > +       in board_init_f_init_reserve() instead.
>
> And what is the profit? The implementation becomes more complex, more
> fragile and defeats the whole purpose. We want to be able to print
> and accumulate debugging messages immediately, and not only after
> certain things are initialized relatively late in the boot process.
> That's exactly the reason why this pre-console buffer functionality
> exists in the first place.

We cannot print anything until we have global_data set up. This
happens in board_init_f_init_reserve() so there is no point in having
a console before then.

We should allocate all the init memory in one place, in C code.

This is very early in the boot process. Please take a look.

[...]

Regards,
Simon