RFC: Support for U-Boot phases in Kconfig

[Simon Glass]

Hi Sean,

On Mon, 9 Aug 2021 at 16:31, Sean Anderson <sean.anderson at 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:
> >
> > 1. Kconfig support

[..]

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

[..]

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