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

[Tom Rini]

On Tue, Feb 14, 2023 at 03:42:53PM -0700, Simon Glass wrote:
> 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...

I'm not sure how it would work out, exactly, no. But I think having a
file for the boards SPL, and for its TPL, and for its VPL would make it
easier to edit and maintain, not harder.

A bigger problem is we have a ton of options that shouldn't ever really
be asked of the user/developer, but instead, we do. Making it easier for
the developer to say that for fooboard here are all of the frameworks
you need (like ARCH_EARLY_INIT_R, sure you can say no, but then the
platform won't work...) would be a bigger help. If you're only really
prompted about 100 options, instead of 1000 options, it's a lot easier
to work with. Especially when it's either "enable X or don't function".

> > And I really don't see the problem with SPL_TPL_ stuff.
> 
> It is ugly!

I think it's elegant, not ugly. I'm willing to change, but "it's ugly"
isn't a good reason in and of itself.

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

It probably won't be too bad in the end, but if it's avoidable, that's
even better.

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

We already get rid of the CMD ones that way, the issue is the
environment, which I suggested you handle by generating it once for PPL
and including for everyone verbatim. That will increase a few platforms
a little, but I think is a reasonable trade-off. And if there's some
case for needing a trivially tiny, or non-existent default environment
really, that's another and different problem we can solve.

But the environment and/or cmds are not the common use case of the
CONFIG_xPL_BUILD checks in Makefiles.

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

Or how to ensure that X doesn't end up in xPL. Which is why I think
having a load more
obj-$(CONFIG_$(SPL_TPL_)FOO) += bar.o (and the same for dirs)
lines would make the Makefiles a lot more elegant, rather than ugly.

I don't object to removing all of those SPL_TPL_ lines, but I object to
removing the feature where when xPL_FOO is not a symbol it defaults to
false. To repeat myself, why can't we know that if we have FOO, SPL_FOO
but neither TPL_FOO nor VPL_FOO are valid symbols, go and generate
auto.conf for each stage and have TPL_FOO=false because we knew we had
FOO and knew we did not have TPL_FOO already?

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

There would need to be some logic around defining phases, and then how
to present for each phase, each relevant option. It wouldn't be yes/no
to "DM_GPIO" and then all phases are set the same, but it would be
that's how you describe an option.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20230214/6d71462f/attachment.sig>