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

10 Aug 2021

      Hi Sean,
On Mon, 9 Aug 2021 at 16:31, Sean Anderson sean.anderson@seco.com wrote:
...
On 8/7/21 6:23 PM, 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:

Kconfig support

[..]
...
...

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,)

Wouldn't cpp eat the trailing comma here? This seems pretty tricky to
use. And of course, you still need something like
Yes you're right. The comma is a pain. Needs more thought.
...
    struct spl_image_loader *loader;
    loader = ... ;

#if CONFIG(LIBCOMMON_SUPPORT)
        foo(loader->name);
#endif
unless you have some other macro to wrap the name access.
Yes that's the other side of it that I forgot. I think a macro that
converts the member to NULL or something might work. Needs more thought.
[..]
...
...

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.
What about __maybe_unused? Do functions marked that way get GC'd?
Yes but not that well. Making the macro expand to 'static __maybe_unused'
seems like a good idea to me.
Regards,
SImon