RFC: Support for U-Boot phases in Kconfig
Simon Glass
sjg at chromium.org
Tue Aug 10 16:58:46 CEST 2021
Hi Tom,
On Mon, 9 Aug 2021 at 13:11, Tom Rini <trini at konsulko.com> wrote:
>
> On Sat, Aug 07, 2021 at 04:23:36PM -0600, Simon Glass wrote:
> > Hi,
> >
> > U-Boot can be configured to build the source multiple times to product multiple
> > 'phases'. The main phase is the full U-Boot, but an 'SPL' (Secondary Program
> > Loader) build can produce a cut-down image only suitable for loading U-Boot
> > proper.
> >
> > SPL does not want to use the same Kconfig options, since that would produce the
> > same binary. Instead we have two copies of some options, one with an SPL prefix,
> > that can be configured independently. In the source code we can use a macro to
> > see if code should be run:
> >
> > if (CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER)) {
> > ...
> > }
> >
> > This expands to check either checking SYS_STDIO_DEREGISTER or
> > SPL_SYS_STDIO_DEREGISTER, depending on which phase is being built.
> >
> > U-Boot also has TPL (Tertiary Program Loader) which works a similar way. This
> > is causing quite an expansion of the Kconfig source, with quite a bit of
> > duplication. Each time a new feature needs to be supported in SPL, it involves
> > a patch to add the same options again but for SPL.
> >
> >
> > Here are some proposed changes to make it easier to deal with SPL/TPL:
> >
> > 1. Kconfig support
> >
> > At present we do things like this when we need to control an option separately
> > in U-Boot proper and SPL:
> >
> > config SYS_STDIO_DEREGISTER
> > bool "Allow deregistering stdio devices"
> > depends on DM_DEVICE_REMOVE
> > default y if USB_KEYBOARD
> > help
> > Generally there is no need to deregister stdio devices since they
> > are never deactivated. But if a stdio device is used which can be
> > removed (for example a USB keyboard) then this option can be
> > enabled to ensure this is handled correctly.
> >
> > config SPL_SYS_STDIO_DEREGISTER
> > bool "Allow deregistering stdio devices in SPL"
> > depends on SPL_DM_DEVICE_REMOVE
> > help
> > Generally there is no need to deregister stdio devices since they
> > are never deactivated. But if a stdio device is used which can be
> > removed (for example a USB keyboard) then this option can be
> > enabled to ensure this is handled correctly. This is very rarely
> > needed in SPL.
> >
> > This is a pain. Apart from the duplication, sometimes the SPL version is in a
> > different file or a different part of the file, making it hard to find related
> > options or update them in sync.
> >
> > Instead, we can add a 'phase' command to the Kconfig language, so we can do:
> >
> > config SYS_STDIO_DEREGISTER
> > bool "Allow deregistering stdio devices"
> > phases
> > depends on p.DM_DEVICE_REMOVE
> > phase MAIN default y if USB_KEYBOARD
> > help
> > Generally there is no need to deregister stdio devices since they
> > are never deactivated. But if a stdio device is used which can be
> > removed (for example a USB keyboard) then this option can be
> > enabled to ensure this is handled correctly.
> >
> > 'phases' means this Kconfig option exists in all phases. You can also say
> > 'phases MAIN SPL' to select just MAIN (U-Boot) and SPL.
> >
> > 'p.DM_DEVICE_REMOVE' means to prefix the phase with each symbol, so for U-Boot
> > (which uses SYS_STDIO_DEREGISTER) this means DM_DEVICE_REMOVE (p is empty) and
> > for SPL (which uses SPL_SYS_STDIO_DEREGISTER) it means SPL_DM_DEVICE_REMOVE
> > (p is SPL_). This is somewhat similar in style to the special-case
> > 'depends on m' in Kconfig.
> >
> > To make this work, we tell Kconfig that SPL is a phase with 'def_phase':
> >
> > config SPL
> > def_phase
> > depends on SUPPORT_SPL
> > prompt "Enable SPL"
> > help
> > If you want to build SPL as well as the normal image, say Y.
> >
> > It works just the same as a bool, but kconfig also uses it to automatically add
> > new Kconfigs for each phase. In the above example it creates both
> > SYS_STDIO_DEREGISTER and SPL_SYS_STDIO_DEREGISTER. The option name has the text
> > '(SPL) ' shown before the SPL option.
> >
> > This can easily handle Kconfigs with similar dependencies and even different
> > ones. If the Kconfig options are not actually very similar we can still
> > create two separate copies instead, as now.
> >
> > This allows us to substantially reduce the size and duplication in the Kconfig
> > defintions. It also reduces the pain of adding another phase to U-Boot.
> >
> > Note: This change needs to be done in Linux, which owns Kconfig upstream.
> >
> >
> > 2.Rename SPL_TPL_
> >
> > This Makefile variable is used to reduce the number of duplicate rules in
> > makefiles:
> >
> > obj-$(CONFIG_$(SPL_TPL_)FIT_SIGNATURE) += fdt_region.o
> >
> > The SPL_TPL_ expands to empty for U-Boot and either SPL_ or TPL_ for the other
> > phases.
> >
> > This is confusing though, since CONFIG_SPL_BUILD it set even for TPL builds, so
> > for example. with:
> >
> > obj-$(CONFIG_$(SPL_)FIT_SIGNATURE) += fdt_region.o
> >
> > the file is built for both SPL and TPL.
> >
> > To help with this, We can rename SPL_TPL to PHASE_:
> >
> > obj-$(CONFIG_$(PHASE_)FIT_SIGNATURE) += fdt_region.o
> >
> > or perhaps P_ which is more readable:
> >
> > obj-$(CONFIG_$(P_)FIT_SIGNATURE) += fdt_region.o
> >
> >
> > 3. Rename CONFIG_IS_ENABLED()
> >
> > This macro is used to determine whether an option is enabled in the current
> > build phase:
> >
> > if (CONFIG_IS_ENABLED(FIT_SIGNATURE)) {
> > printf("## Checking hash(es) for Image %s ... ",
> > fit_get_name(fit, node, NULL));
> >
> > It is quite long-winded and people sometimes add CONFIG_ to the option inside
> > the brakets by mistake. It is also a bit confusing that IS_ENABLED() and
> > CONFIG_IS_ENABLED() mean completely different things.
> >
> > Instead we can rename it to CONFIG:
> >
> > if (CONFIG(FIT_SIGNATURE)) {
> > printf("## Checking hash(es) for Image %s ... ",
> > fit_get_name(fit, node, NULL));
> >
> > This is shorter and looks more like CONFIG_FIT_SIGATURE so people should find
> > it easier to understand. Being shorter is a big help when converting from
> > use #if to if(), since an indentation is always enabled. This change makes
> > the CONFIG() check no longer than IS_ENABLED().
> >
> > It also makes CONFIG(OPTION) not much longer than CONFIG_OPTION, which makes
> > things much more convenient, since ideally if the toolchain permitted it, we
> > would just use CONFIG_OPTION in the code. This is not possible at present since
> > the option may not be defined, so can cause a compiler error.
> >
> > Over time, perhaps the existing IS_ENABLED() will phase out, since in many
> > cases SPL will have its own options. We already see that CONFIG_IS_ENABLED is
> > more popular / useful:
> >
> > $ git grep -w IS_ENABLED |wc -l
> > 902
> > $ git grep -w CONFIG_IS_ENABLED |wc -l
> > 2282
> >
> >
> > 4. Add macros to help avoid more #ifdefs
> >
> > We sometimes have to use #ifdefs in structs or drivers:
> >
> > struct spl_image_loader {
> > #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> > const char *name;
> > #endif
> > ...
> > };
> >
> > UCLASS_DRIVER(spi) = {
> > .id = UCLASS_SPI,
> > .name = "spi",
> > .flags = DM_UC_FLAG_SEQ_ALIAS,
> > #if CONFIG_IS_ENABLED(OF_CONTROL) && !CONFIG_IS_ENABLED(OF_PLATDATA)
> > .post_bind = dm_scan_fdt_dev,
> > #endif
> > ...
> > };
> >
> > This is a pain. We can add an IF_CONFIG macro to help with this:
> >
> > struct spl_image_loader {
> > IF_CONFIG(LIBCOMMON_SUPPORT, const char *name;)
> > ...
> > };
> >
> > UCLASS_DRIVER(spi) = {
> > .id = UCLASS_SPI,
> > .name = "spi",
> > .flags = DM_UC_FLAG_SEQ_ALIAS,
> > IF_CONFIG(REAL, .post_bind = dm_scan_fdt_dev,)
> > ...
> > };
> >
> > It still isn't wonderfully readable but it seems like an improvement. The
> > IF_CONFIG() macros could be implemented easily with the current
> > CONFIG_IS_EANBLED() macro.
> >
> >
> > 5. IF_CONFIG_INT() or similar
> >
> > See here: https://lists.denx.de/pipermail/u-boot/2020-May/412950.html
> >
> >
> > 6. Discarding static functions
> >
> > We sometimes see code like this:
> >
> > #if CONFIG_IS_ENABLED(OF_REAL)
> > static const struct udevice_id apl_ns16550_serial_ids[] = {
> > { .compatible = "intel,apl-ns16550" },
> > { },
> > };
> > #endif
> >
> > U_BOOT_DRIVER(intel_apl_ns16550) = {
> > .name = "intel_apl_ns16550",
> > .id = UCLASS_SERIAL,
> > .of_match = of_match_ptr(apl_ns16550_serial_ids),
> > .plat_auto = sizeof(struct apl_ns16550_plat),
> > .priv_auto = sizeof(struct ns16550),
> > ...
> > };
> >
> > The of_match_ptr() avoids an #ifdef in the driver declaration since it evaluates
> > to NULL if !CONFIG_IS_ENABLED(OF_REAL) but we still need an #ifdef around the
> > function, since it is static and would otherwise produce a warning.
> >
> > One solution is to drop the 'static'. But this is not very nice, since the
> > structure clearly should not be used from another file.
> >
> > We can add STATIC_IF_CONFIG() to help with this:
> >
> > STATIC_IF_CONFIG(OF_REAL) const struct udevice_id
> > apl_ns16550_serial_ids[] = {
> > { .compatible = "intel,apl-ns16550" },
> > { },
> > };
> > #endif
> >
> > It expands to 'static' if CONFIG(OF_REAL) is enabled, otherwise it expand to
> > nothing, in the hope that the compiler drops the data. Failing that it would
> > also be possible to have it expand to '__section(".discard.config")' so at least
> > the struct is discarded, even if the compatible string is not. The behaviour of
> > gcc/binutils in this area is not always as might be hoped.
> >
> >
> > Comments welcome!
>
> I think what this is really showing is that Yamada-san was right. All
One thread where this was discussed was here I think:
https://yhbt.net/lore/all/20140624192425.9368.AA925319@jp.panasonic.com/T/
I cannot find all the arguments for either side now. Do you have a
pointer to them?
> the games we need to do so that "make fooboard_config all" results in
> building the N stages needed was the wrong track. Taking
> khadas-edge-v-rk3399 as an example, if we had instead of
> khadas-edge-v-rk3399_defconfig but khadas-edge-v-rk3399_tpl_defconfig
> khadas-edge-v-rk3399_spl_defconfig and khadas-edge-v-rk3399_config, each
> of which could set CONFIG_TPL, CONFIG_SPL or neither. Then yes, to
> build u-boot-rockchip.bin you would need to pass in the separately build
> TPL and SPL stages. But, that's true of so so many other platforms. To
> pick another example, imx8mm_evk doesn't function without other
> binaries. If in theory to build khadas-edge-v-rk3399 you had to do
> something like:
> $ export CROSS_COMPILE=aarch64-linux-
> $ make O=obj/tpl khadas-edge-v-rk3399_tpl_config all
> $ make O=obj/spl khadas-edge-v-rk3399_spl_config all
> $ make O=obj/khadas-edge-v-rk3399 TPL=obj/tpl/u-boot.bin \
> SPL=obj/spl/u-boot.bin khadas-edge-v-rk3399_config all
We also need to think about the tools which are presumably built separately.
We might have to build in the opposite order, because SPL needs to
grep the full devicetree, although I suppose we could just recompile
it.
>
> But it also meant that we didn't need to duplicate so so many Kconfig
> options and most of our obj rules would just be:
> obj-$(CONFIG_FOO) += foo.o
>
> would be a win.
That definitely looks nice.
But how much of a win is this?
I understand how it could work and I think we did talk about this at
the time. But there are problems too that I'd like to review if we can
find them. Some I can think of:
- maintaining three or more separate defconfig files for each board
- not sure how to handle dependencies between phases (e.g.
SPL_BLOBLIST has ''default y if BLOBLIST', or one phase expecting an
image to be in there)
- running 'make menuconfig' updates one phase but not the others,
making things harder to understand
- splitting up of the build as you note above, making it harder for
people to understand
Interesting to see this comment:
http://u-boot.10912.n7.nabble.com/PATCH-v8-0-13-Kconfig-for-U-Boot-tt185309.html#a185306
"It would take really long term (one year? two year? I don't know)
to migrate from board headers to Kconfig.
So, two different infractructure must coexist in the interim."
That was 2014! I think we need a way to remove old CONFIGs and let
board maintainers add them back in Kconfig if needed.
Regards,
Simon
More information about the U-Boot
mailing list