RFC: Support for U-Boot phases in Kconfig
Simon Glass
sjg at chromium.org
Wed Aug 11 14:56:31 CEST 2021
Hi Tom,
On Tue, 10 Aug 2021 at 13:38, Tom Rini <trini at konsulko.com> wrote:
>
> On Tue, Aug 10, 2021 at 08:58:46AM -0600, Simon Glass wrote:
> > 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?
>
> I don't off-hand. I'm pretty sure it's come up more than once tho.
I certainly don't remember there being a strong argument either way at
the time. Perhaps Masahiro can chime in.
>
> > > 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.
>
> I'd like to see a way to build less host tools by default, but I fear
> the problem of distro folks if we go that way as it's more of a world
> builder / CI person problem that we build so many tools every time.
>
> > 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.
>
> If we use dtb files from one stage in another stage directly (rather
> than being an included / packaged file) that seems like a potential
> problem.
Yes, also the arrangement of the build doesn't relate so closely to
the Kconfig side of things. See below.
>
> > > 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 think a good sized one. Especially since it will let us remove some
> customization in the build code.
>
> > 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
>
> Somewhat, yes. The SPL/TPL configs should be small. With some not
> overly judicious use of imply's in board/.../Kconfig it shouldn't be
> very much. I think it might emphasize that we do need a good way /
> place to set defaults for SoCs and boards.
>
> > - 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)
>
> My first, but perhaps bad idea would be that we have say
> TARGET_AM335X_EVM in arch/arm/mach-omap2/am33xx/Kconfig still and a new
> TARGET_AM335X_EVM_OPTS in board/ti/am335x/Kconfig that would
> select/imply things that need to be enabled in all stages.
>
> > - running 'make menuconfig' updates one phase but not the others,
> > making things harder to understand
>
> I'm not sure this is a problem so much. TPL/SPL shouldn't have much
> configuring to them, and even less re-configuring.
>
> > - splitting up of the build as you note above, making it harder for
> > people to understand
>
> This I think is debatable. That we build and configure things the way
> we do isn't always obvious. More and better documentation, either way,
> would be good.
Having thought a bit more, perhaps we have the wrong attitude to
Kconfig. The CONFIG() macro I am talking about works by building an
xxx or SPL_xxx config. If we have separate autoconf.h files for each
phase (autoconf_spl.h etc.) then we don't actually need this. We just
need to include the correct file. Any SPL_xxx config can be written as
xxx. Similarly the Makefile rules can drop the $(P) I was proposing.
We can, in fact, generate separate autoconf.h files for each phase
today, with no other changes. Unless I am missing something...?
>
> > 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.
>
> I need to take another pass at converting a bunch of symbols, to see
> where we're at. Probably the biggest chunk of progress next would be to
> start converting CONFIG_SYS_xxx to SYS_xxx and moving defines out of
> config.h and in to something else. I'm taking a peek at some of the
> remaining PCI ones now.
How about we set a deadline for this? It has gone on for too long and
we just need to drop these CONFIGs. It's probably a higher priority
than a Kconfig change.
I was expecting that the config.h files would go away and we would use
Kconfig (or DT) for everything. What sort of things don't fit into
that model?
Regards,
Simon
More information about the U-Boot
mailing list