Hello Simon,
On Thu, 29 Sep 2016 14:23:02 -0600
Simon Glass <s...@chromium.org> wrote:
> Move these option to Kconfig and tidy up existing uses.
>
> Signed-off-by: Simon Glass <s...@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.
> 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?
> +
> +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.
> endmenu
>
> config SYS_NO_FLASH
> diff --git a/common/console.c b/common/console.c
> index 12293f3..31a9b3e 100644
> --- a/common/console.c
> +++ b/common/console.c
> @@ -201,7 +201,7 @@ static void console_putc(int file, const char c)
> }
> }
>
> -#ifdef CONFIG_PRE_CONSOLE_BUFFER
> +#if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER)
> static void console_puts_noserial(int file, const char *s)
> {
> int i;
> @@ -247,7 +247,7 @@ static inline void console_putc(int file, const char c)
> stdio_devices[file]->putc(stdio_devices[file], c);
> }
>
> -#ifdef CONFIG_PRE_CONSOLE_BUFFER
> +#if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER)
> static inline void console_puts_noserial(int file, const char *s)
> {
> if (strcmp(stdio_devices[file]->name, "serial") != 0)
> @@ -413,7 +413,7 @@ int tstc(void)
> #define PRE_CONSOLE_FLUSHPOINT1_SERIAL 0
> #define PRE_CONSOLE_FLUSHPOINT2_EVERYTHING_BUT_SERIAL 1
>
> -#ifdef CONFIG_PRE_CONSOLE_BUFFER
> +#if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER)
> #define CIRC_BUF_IDX(idx) ((idx) % (unsigned long)CONFIG_PRE_CON_BUF_SZ)
>
> static void pre_console_putc(const char c)
> diff --git a/configs/tbs2910_defconfig b/configs/tbs2910_defconfig
> index fbff9fe..0871408 100644
> --- a/configs/tbs2910_defconfig
> +++ b/configs/tbs2910_defconfig
> @@ -1,6 +1,8 @@
> CONFIG_ARM=y
> CONFIG_ARCH_MX6=y
> CONFIG_TARGET_TBS2910=y
> +CONFIG_PRE_CONSOLE_BUFFER=y
> +CONFIG_PRE_CON_BUF_ADDR=0x7c000000
> CONFIG_FIT=y
> CONFIG_BOOTDELAY=3
> CONFIG_HUSH_PARSER=y
> diff --git a/include/asm-generic/global_data.h
> b/include/asm-generic/global_data.h
> index dc4cbdb..e02863d 100644
> --- a/include/asm-generic/global_data.h
> +++ b/include/asm-generic/global_data.h
> @@ -45,7 +45,7 @@ typedef struct global_data {
> unsigned long board_type;
> #endif
> unsigned long have_console; /* serial_init() was called */
> -#ifdef CONFIG_PRE_CONSOLE_BUFFER
> +#if CONFIG_IS_ENABLED(PRE_CONSOLE_BUFFER)
> unsigned long precon_buf_idx; /* Pre-Console buffer index */
> #endif
> unsigned long env_addr; /* Address of Environment struct */
> diff --git a/include/configs/sunxi-common.h b/include/configs/sunxi-common.h
> index d261fb3..c604ce2 100644
> --- a/include/configs/sunxi-common.h
> +++ b/include/configs/sunxi-common.h
> @@ -69,7 +69,6 @@
> #define CONFIG_SYS_SDRAM_BASE 0x20000000
> #define CONFIG_SYS_LOAD_ADDR 0x22000000 /* default load address */
> #define CONFIG_SYS_TEXT_BASE 0x2a000000
> -#define CONFIG_PRE_CON_BUF_ADDR 0x2f000000
> /* Note SPL_STACK_R_ADDR is set through Kconfig, we include it here
> * since it needs to fit in with the other values. By also #defining it
> * we get warnings if the Kconfig value mismatches. */
> @@ -80,7 +79,6 @@
> #define CONFIG_SYS_SDRAM_BASE 0x40000000
> #define CONFIG_SYS_LOAD_ADDR 0x42000000 /* default load address */
> #define CONFIG_SYS_TEXT_BASE 0x4a000000
> -#define CONFIG_PRE_CON_BUF_ADDR 0x4f000000
> /* Note SPL_STACK_R_ADDR is set through Kconfig, we include it here
> * since it needs to fit in with the other values. By also #defining it
> * we get warnings if the Kconfig value mismatches. */
> @@ -373,10 +371,6 @@ extern int soft_i2c_gpio_scl;
> #ifndef CONFIG_SPL_BUILD
> #include <config_distro_defaults.h>
>
> -/* Enable pre-console buffer to get complete log on the VGA console */
> -#define CONFIG_PRE_CONSOLE_BUFFER
> -#define CONFIG_PRE_CON_BUF_SZ 4096 /* Aprox 2 80*25 screens */
> -
> #ifdef CONFIG_ARM64
> /*
> * Boards seem to come with at least 512MB of DRAM.
> diff --git a/include/configs/tbs2910.h b/include/configs/tbs2910.h
> index 85501bc..ddd53dd 100644
> --- a/include/configs/tbs2910.h
> +++ b/include/configs/tbs2910.h
> @@ -50,10 +50,6 @@
> #define CONFIG_CONSOLE_MUX
> #define CONFIG_CONS_INDEX 1
>
> -#define CONFIG_PRE_CONSOLE_BUFFER
> -#define CONFIG_PRE_CON_BUF_SZ 4096
> -#define CONFIG_PRE_CON_BUF_ADDR 0x7C000000
> -
> /* *** Command definition *** */
> #define CONFIG_CMD_BMODE
>
> diff --git a/scripts/config_whitelist.txt b/scripts/config_whitelist.txt
> index 9e2f00d..7a69be5 100644
> --- a/scripts/config_whitelist.txt
> +++ b/scripts/config_whitelist.txt
> @@ -3728,9 +3728,6 @@ CONFIG_PQ_MDS_PIB
> CONFIG_PQ_MDS_PIB_ATM
> CONFIG_PRAM
> CONFIG_PREBOOT
> -CONFIG_PRE_CONSOLE_BUFFER
> -CONFIG_PRE_CON_BUF_ADDR
> -CONFIG_PRE_CON_BUF_SZ
> CONFIG_PRIMEVIEW_V16C6448AC
> CONFIG_PRINTK
> CONFIG_PROC_FS
--
Best regards,
Siarhei Siamashka
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
