...

...

Hello Simon,

On Thu, 29 Sep 2016 14:23:02 -0600 Simon Glass sjg@chromium.org wrote:

...

...

Move these option to Kconfig and tidy up existing uses.

Signed-off-by: Simon Glass sjg@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.

This is a good point to bring up. As we're discussing in another thread about moving FSL things out of CONFIG_SYS_... and into Kconfig or a different namespace, we have other examples today where there's addresses in Kconfig. Looking harder still at this code, perhaps as a follow-up CONFIG_PRE_CON_BUF_SZ should just be 'PRE_CON_BUF_SIZE' in the code and 4096, and not in Kconfig at all. And for this series, make the default tbs2910 uses the default for ARCH_MX6.

I don't think that my arbitrarily chosen PRE_CON_BUF_ADDR for tbs2910 (0x7C000000) is a good default for ARCH_MX6. On tbs2910 we have 2GB of DDR memory, there are lots of other imx6 boards with less memory available. So a lower PRE_CON_BUF_ADDR (as for sunxi) might be a better choice.

The only constraint for this buffer (besides being located in DDR) is, that it must not overlap with u-boot text and data. As I understand it, this buffer is only used very early in the boot process and not needed anymore, when the vga console is set up. So this buffer can not interfere with user commands (even pre-boot commands).

...

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.

So maybe we should hide the size option under EXPERT?

Just curious, can this address be generated automatically, e.g. just below the malloc area? Or is this too complicated, e.g. because the buffer is used already before relocation?

However, if this address is configurable in Kconfig at all, for me it makes sense to hide it somehow.

I can test other PRE_CON_BUF_ADDR settings on tbs2910 if this would help to find more general default values.

Regards, Soeren