Re: RFC: Support for U-Boot phases in Kconfig

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 15535 bytes --]

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@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.

> > 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.

> > 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.

> 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.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]