[PATCH v3 61/95] kconfig: Support writing separate SPL files

[Simon Glass]

Hi Tom,

On Tue, 14 Feb 2023 at 13:51, Tom Rini <trini at konsulko.com> wrote:
>
> On Tue, Feb 14, 2023 at 12:59:34PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Tue, 14 Feb 2023 at 10:38, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Tue, Feb 14, 2023 at 09:58:59AM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Tue, 14 Feb 2023 at 09:31, Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > On Sun, Feb 12, 2023 at 04:16:04PM -0700, Simon Glass wrote:
> > > > >
> > > > > > At present kconfig writes out several files, including:
> > > > > >
> > > > > >    auto.conf  - CONFIG settings used by make
> > > > > >    autoconf.h - header file used by C code
> > > > > >
> > > > > > This works well but is a bit ugly in places, for example requiring the use
> > > > > > of a SPL_TPL_ macro in Makefiles to distinguish between options intended
> > > > > > for SPL and U-Boot proper.
> > > > > >
> > > > > > Update the kconfig tool to also output separate files for each phase: e.g.
> > > > > > auto_spl.conf and autoconf_spl.h
> > > > > >
> > > > > > These are similar to the existing files, but drop the SPL_ prefix so that
> > > > > > SPL_TPL_ is not needed. It also allows the CONFIG_IS_ENABLED() macro to be
> > > > > > simplified, in a later patch, eventually replacing it with IS_ENABLED().
> > > > > >
> > > > > > When CONFIG_FOO is used within SPL, it means that FOO is enabled in that
> > > > > > SPL phase. For example if CONFIG_SPL_FOO is enabled in the Kconfig, that
> > > > > > means that CONFIG_FOO will be enabled in the SPL phase. So the SPL builds
> > > > > > can just use CONFIG_FOO to check it. There is no need to use
> > > > > > CONFIG_SPL_FOO or CONFIG_IS_ENABLED() anymore.
> > > > > >
> > > > > > This of course means that if there is a need to access a PPL symbol from
> > > > > > an SPL build, there is no way to do it. To copy with that, we need a
> > > > > > CONFIG_PPL_FOO to be visibilty to all SPL builds.
> > > > > >
> > > > > > So this change also adds new PPL_ output for U-Boot proper (Primary
> > > > > > Program Loader). So every CONFIG_FOO that is enabled in PPL also has a
> > > > > > CONFIG_PPL_FOO
> > > > > >
> > > > > > This allows SPL to access the TEXT_BASE for U-Boot proper, for example, so
> > > > > > it knows where to load it. There are about 30 places where this is needed,
> > > > > > in addition to TEXT_BASE. The environment has the same problem, adding
> > > > > > another dozen or so caes in include/config_distro_bootcmd.h but it has
> > > > > > been decided to ignore that for now.
> > > > > >
> > > > > > The feature is controlled by an environment variable, since it seems to be
> > > > > > bad form to add flags to the conf tool.
> > > > > >
> > > > > > Rebuild the autoconf files if the split config is not present. This allows
> > > > > > building this commit as part of a chain, without generating build errors.
> > > > > >
> > > > > > These changes may benefit from some reworking to send upstream, e.g. to
> > > > > > use a struct for the 'arg' parameter.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > >
> > > > > This patch, I think, is where my largest problem is. We go from being
> > > > > able to say "if CONFIG_SPL_FOO is undefined, it is false" to "we must
> > > > > define CONFIG_SPL_FOO to false". There's around 150 cases of this, with
> > > > > the series. Why can we not extend the PPL logic (which I'm not super
> > > > > happy with, but, I understand and I think an audit of everything
> > > > > not-TEXT_BASE should be fairly straight forward), to say that if
> > > > > CONFIG_FOO exists and CONFIG_SPL_FOO does not exist, say CONFIG_SPL_FOO
> > > > > is now false.
> > > >
> > > > Well it is all about choices.
> > > >
> > > > We don't have to add a CONFIG_SPL_xxx to tell Kconfig that the PPL
> > > > symbol controls all phases. We can use the conf_nospl file instead.
> > > > But then the entire description is not in Kconfig. Of course, we might
> > > > expect that some of those things in conf_nospl might end up needing to
> > > > be controlled in SPL, so perhaps that file would shrink? Not sure
> > > > about that, though.
> > > >
> > > > It isn't just SPL , BTW. We might have any xPL symbol defined.
> > > >
> > > > I suppose you are thinking of something like:
> > > >
> > > > #define CONFIG_IS_ENABLED(x)  IS_ENABLED(CONFIG_ ## xpl_prefix ## x) ||
> > > >    ( IS_ENABLED(CONFIG_ ## x) && !IS_ENABLED(CONFIG_ SPL_ ## x) &&
> > > > !IS_ENABLED(CONFIG_TPL_ ##x) ..)
> > > >
> > > > But how do we deal with Makefiles? We still end up with the SPL_TPL_ stuff.
> > >
> > > I see it as a very useful feature that today if we don't set
> > > CONFIG_xPL_FOO, it evaluates to false, anywhere we need it to. In all of
> > > the prep work for split config, I think we've seen one case where we got
> > > things wrong (in that it lead to failure). Split config build is doing a
> > > whole bunch of things to then remove this feature. And the series
> > > intentionally ignores the Makefile design issue / feature of wrapping
> > > large chunks with a check for being/not-being in an xPL_BUILD phase.
> >
> > In another thread you suggested moving to separate defconfig files for
> > each phase (I think). Presumably that would operate the same way as
> > this series, in that you would not be able to handle the special case
> > you mention. So I see this series as the best of both worlds - a
> > unified Kconfig (and .config) but with separate auto.conf files for
> > each phase. Plus we can drop the SPL_TPL_ stuff.
>
> What I keep thinking of is that we would end up with separate configs.
> So while yes, we might need CONFIG_PPL_TEXT_BASE, we would not need
> CONFIG_SPL_DM_GPIO for example, because fooboard_spl_defconfig would set
> CONFIG_SPL=y and CONFIG_DM_GPIO=y.  I don't know how workable this ends
> up being as we have about 7k symbols for full U-Boot and about 450 for
> SPL.

I'm not sure it would be very nice. We end up having to edit three
files to change a board. It would be hard to compare them. We would
set an option in SPL that cannot be set because it is not actually
supported in SPL...

>
> And I really don't see the problem with SPL_TPL_ stuff.

It is ugly!

>
> > We can add things to conf_nospl and then the behaviour is as you want
> > it, isn't it?
>
> I'd also rather not have a file that's going to get conflicted on, even
> if all of the comments started with "#" and so we could pass it through
> sort. But yes, that's better than adding unselectable options.

OK. I would hope that conflicts would be small as it is sorted.
Actually that is my mistake leaving out the # lines. Ooops.

>
> > For the Makefile xPL ifdefs, is it essential to do anything there? We
> > could remove them, using the same techniques in this series. I would
> > quite like to, but we don't have to, if you like them. My reason for
> > wanting to remove them is that we end up refactoring things to enable
> > features in xPL, instead of just adding an xPL option.
>
> My point with the xPL ifdefs is that a number of config_nospl/def_bool n
> options could be resolved the same way. To me, the problem isn't that we
> have lines like:
> obj-$(CONFIG_$(SPL_TPL_)FOO += foo.o
> as I've encouraged people to add them, in the case of the MULTIPLEXER
> case I'm pretty sure, rather than finding the existing block of:
> ifneq ($(CONFIG_SPL_BUILD)$(CONFIG_TPL_BUILD),y)
> ...
> endif
> for the cases where we need to have something not built in SPL/TPL.
> Removing those guards would lead to a big increase in config_nospl
> lines, I suspect.

Probably. If we didn't rely on CONFIG_CMD_USB and the like we could
use a wildcard and get rid of the CONFIG_CMD ones that way.  But
really a Kconfig annotation would be better.

If you are saying that we should have Makefiles without ifdefs, I
think I agree...or at least it is worth trying out. It means that we
don't have to trawl around to figure out why enabling X in SPL does
not in fact compile the code!

>
> > Finally, with the series I have, it is quite easy to add a new phase
> > if we want to.
> >
> > >
> > > > Yes, adding PPL moves a step forward and reduces the audit to only
> > > > uses of PPL, instead of the whole Kconfig.
> > > >
> > > > This series does get us closer to separate configs (it is really easy
> > > > to see what is enabled in each build just by looking at the
> > > > auto_xpl.conf files) and I think those 150 exceptions are not a big
> > > > price to pay.
> > > >
> > > > Ultimately I am coming to the view that we should extend the Kconfig
> > > > language to support multiple build phases, using a 'phase' property.
> > > > Zephyr is going to need it too, fairly soon[1].
> > > >
> > > > Regards,
> > > > Simon
> > > >
> > > > [1] https://github.com/zephyrproject-rtos/zephyr/issues/54534
> > >
> > > Yes, if the Kconfig language moves in a new direction, it would be good
> > > to follow and use that.
> >
> > Well I am thinking of moving it there...but would need to start by
> > upstreaming the two patches in this series.
>
> Well, honestly, I suspect if you can work with upstreams on Kconfig the
> language supporting the concepts of phases, natively, my concerns will
> be addressed, one way or another.  Because something like:
> config DM_GPIO
>         bool "DM GPIO support"
>         phases PPL SPL TPL
>
> config ODDBALL_THING
>         # phases PPL is implied when it's only valid there
>         bool "Do this one weird trick in PPL"
>
> Means that we can yes/no options like DM_GPIO for each phase, and get
> xPL_ODDBALL_THING=n for free is much cleaner.

Yes...that is what I'd like to see. The only complication is that
sometimes you want SPL_DM_GPIO to default y if DM_GPIO, iwc you may
want to spell out the options separately anyway?

Regards,
Simon