| Message ID | 20230212231638.1134219-62-sjg@chromium.org |
|---|---|
| State | RFC |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | RFC: Migrate to split config | expand |
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@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.
Hi Tom, On Tue, 14 Feb 2023 at 09:31, Tom Rini <trini@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@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. 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
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@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@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. > 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.
Hi Tom, On Tue, 14 Feb 2023 at 10:38, Tom Rini <trini@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@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@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. We can add things to conf_nospl and then the behaviour is as you want it, isn't it? 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. 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. Regards, Simon
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@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@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@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. And I really don't see the problem with SPL_TPL_ stuff. > 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. > 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. > 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.
Hi Tom, On Tue, 14 Feb 2023 at 13:51, Tom Rini <trini@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@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@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@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
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@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@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@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@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.
diff --git a/Makefile b/Makefile index 875cdda08f2..bc101e67588 100644 --- a/Makefile +++ b/Makefile @@ -597,6 +597,12 @@ ifeq ($(dot-config),1) # To avoid any implicit rule to kick in, define an empty command $(KCONFIG_CONFIG) include/config/auto.conf.cmd: ; +# Make sure this uses the split config +has_split_config = $(shell grep CONFIG_PPL_LOCALVERSION include/config/auto.conf 2>/dev/null) +ifeq ($(has_split_config),) +include/config/auto.conf: FORCE +endif + # If .config is newer than include/config/auto.conf, someone tinkered # with it and forgot to run make oldconfig. # if auto.conf.cmd is missing then we are probably in a cleaned tree so diff --git a/scripts/kconfig/Makefile b/scripts/kconfig/Makefile index 12e525ee31f..b539054645f 100644 --- a/scripts/kconfig/Makefile +++ b/scripts/kconfig/Makefile @@ -25,6 +25,8 @@ endif # We need this, in case the user has it in its environment unexport CONFIG_ +$(obj)/conf: $(srctree)/scripts/conf_nospl + xconfig: $(obj)/qconf $< $(silent) $(Kconfig) @@ -71,8 +73,9 @@ simple-targets := oldconfig allnoconfig allyesconfig allmodconfig \ alldefconfig randconfig listnewconfig olddefconfig syncconfig PHONY += $(simple-targets) +# Pass --u-boot to get SPL behaviour $(simple-targets): $(obj)/conf - $< $(silent) --$@ $(Kconfig) + KCONFIG_SPL=1 $< $(silent) --$@ $(Kconfig) PHONY += savedefconfig defconfig diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c index 376f796f674..c4463d252c1 100644 --- a/scripts/kconfig/conf.c +++ b/scripts/kconfig/conf.c @@ -490,9 +490,13 @@ int main(int ac, char **av) const char *name, *defconfig_file = NULL /* gcc uninit */; struct stat tmpstat; int no_conf_write = 0; + bool support_spl; tty_stdio = isatty(0) && isatty(1); + /* Special handling of SPL phases for U-Boot */ + support_spl = getenv("KCONFIG_SPL"); + while ((opt = getopt_long(ac, av, "s", long_opts, NULL)) != -1) { if (opt == 's') { conf_set_message_callback(NULL); @@ -692,7 +696,7 @@ int main(int ac, char **av) fprintf(stderr, "\n*** Error during writing of the configuration.\n\n"); exit(1); } - if (conf_write_autoconf()) { + if (conf_write_autoconf(support_spl)) { fprintf(stderr, "\n*** Error during update of the configuration.\n\n"); return 1; } diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c index 73bf43bcb95..c2f49bd87f3 100644 --- a/scripts/kconfig/confdata.c +++ b/scripts/kconfig/confdata.c @@ -16,6 +16,16 @@ #include "lkc.h" +/* Number of SPL prefixes we recognise */ +#define NUM_SPLS 4 + +/* + * SPL prefixes recognised. For example CONFIG_SPL_xxx is considered to be an + * SPL version of CONFIG_xxx + */ +static const char *spl_name[NUM_SPLS] = {"SPL", "TPL", "VPL", "TOOLS"}; +int num_spls; /* 0 if not supporting SPL, else NUM_SPLS */ + /* return true if 'path' exists, false otherwise */ static bool is_present(const char *path) { @@ -531,6 +541,24 @@ static void print_makefile_sym(FILE *fp, const char *name, fprintf(fp, "%s%s=%s\n", CONFIG_, name, value); } +/** + * get_primary_name() - Prepends PPL to a symbol if it is an SPL symbol + * + * @sym: Symbol to process + * @str: Buffer space to use, of size SYMBOL_MAXLENGTH + * Returns: NULL if this is not an SPL symbol, otherwise returns the symbol + * name with PPL_ prepended + */ +static const char *get_primary_name(const struct symbol *sym, char *str) +{ + if (sym->flags & SYMBOL_SPL) + return NULL; + + snprintf(str, SYMBOL_MAXLENGTH, "PPL_%s", sym->name); + + return str; +} + /* * Kconfig configuration printer * @@ -542,6 +570,15 @@ static void print_makefile_sym(FILE *fp, const char *name, static void kconfig_print_symbol(FILE *fp, struct symbol *sym, const char *value, void *arg) { + char str[SYMBOL_MAXLENGTH]; + const char *name; + + if (num_spls) { + name = get_primary_name(sym, str); + if (name) + print_makefile_sym(fp, name, sym->type, value, arg); + } + print_makefile_sym(fp, sym->name, sym->type, value, arg != NULL); } @@ -571,6 +608,89 @@ static struct conf_printer kconfig_printer_cb = .print_comment = kconfig_print_comment, }; +/** + * get_spl_name() - Look up an SPL symbol + * + * This is used to get the name of a Kconfig option to write in an SPL context. + * If the symbol has an SPL symbol, this means it is used for U-Boot proper, so + * should not be written at all. + * + * Otherwise, this returns the name of the option. If the option is an SPL + * option, then the prefix (SPL_ or TPL_) is removed + * + * @sym: Symbol to look up + * @arg: Argument passed to the symbol function. This is void * but is actually + * an int, indicating the SPL index / type (see spl_name[]) + * @return name to write out for this symbol xxx: + * NULL (don't write) if xxx has an associated SPL symbol + * xxx if xxx is a non-SPL symbol + * xxx if SPL_xxx is an SPL symbol + */ +static const char *get_spl_name(const struct symbol *sym, const void *arg) +{ + int spl = (long)arg; + const char *name = sym->name; + + /* + * Don't print it if this has an SPL symbol because processing of the + * SPL symbol (e.g. SPL_FOO) will output CONFIG_FOO as well as + * CONFIG_SPL_FOO + */ + if (sym->flags & SYMBOL_HAS_SPL) + return NULL; + + /* + * If it is SPL, only print it if the SPL_ prefix matches + * Drop the prefix. + */ + if (sym->flags & SYMBOL_SPL) { + int len = strlen(spl_name[spl]); + + if (!strncmp(name, spl_name[spl], len) && name[len] == '_') + name += len + 1; + } + + return name; +} + +/* + * auto.conf configuration printer for SPL + * + * This is used for creating auto.conf as well as SPL files like auto_spl.conf + * + * This printer is used when generating the resulting configuration after + * kconfig invocation and `defconfig' files. Unset symbol might be omitted by + * passing a non-NULL argument to the printer. + */ +static void spl_kconfig_print_symbol(FILE *fp, struct symbol *sym, + const char *value, void *arg) +{ + char str[SYMBOL_MAXLENGTH]; + const char *name; + + name = get_primary_name(sym, str); + if (name) + print_makefile_sym(fp, name, sym->type, value, true); + + name = get_spl_name(sym, arg); + if (!name) + return; + + /* + * If this is an SPL symbol, first print the symbol without the SPL + * prefix + */ + if (name != sym->name) + print_makefile_sym(fp, name, sym->type, value, true); + if (!(sym->flags & SYMBOL_NO_SPL)) + print_makefile_sym(fp, sym->name, sym->type, value, true); +} + +static struct conf_printer spl_kconfig_printer_cb = { + .print_symbol = spl_kconfig_print_symbol, + .print_comment = kconfig_print_comment, +}; + /* Print a symbol for a header file */ static void print_header_sym(FILE *fp, const char *name, enum symbol_type type, const char *value) @@ -620,6 +740,15 @@ static void print_header_sym(FILE *fp, const char *name, enum symbol_type type, static void header_print_symbol(FILE *fp, struct symbol *sym, const char *value, void *arg) { + char str[SYMBOL_MAXLENGTH]; + const char *name; + + if (num_spls) { + name = get_primary_name(sym, str); + if (name) + print_header_sym(fp, name, sym->type, value); + } + print_header_sym(fp, sym->name, sym->type, value); } @@ -651,6 +780,37 @@ static struct conf_printer header_printer_cb = .print_comment = header_print_comment, }; +/* + * SPL header printer + * + * This printer is used when generating SPL files such as + * `include/generated/autoconf_spl.h' + */ +static void spl_header_print_symbol(FILE *fp, struct symbol *sym, + const char *value, void *arg) +{ + char str[SYMBOL_MAXLENGTH]; + const char *name; + + name = get_primary_name(sym, str); + if (name) + print_header_sym(fp, name, sym->type, value); + + name = get_spl_name(sym, arg); + if (!name) + return; + + if (name != sym->name) + print_header_sym(fp, name, sym->type, value); + if (!(sym->flags & SYMBOL_NO_SPL)) + print_header_sym(fp, sym->name, sym->type, value); +} + +static struct conf_printer spl_header_printer_cb = { + .print_symbol = spl_header_print_symbol, + .print_comment = header_print_comment, +}; + /* * Tristate printer * @@ -1022,15 +1182,24 @@ out: return res; } -int conf_write_autoconf(void) +int conf_write_autoconf(bool support_spl) { struct symbol *sym; const char *name; FILE *out, *tristate, *out_h; - int i; + FILE *out_spl[NUM_SPLS]; + FILE *out_h_spl[NUM_SPLS]; + int i, spl; sym_clear_all_valid(); + /* U-Boot: Mark symbols according to their SPL/non-SPL nature */ + if (support_spl) { + num_spls = NUM_SPLS; + if (conf_mark_symbols()) + return 1; + } + conf_write_dep("include/config/auto.conf.cmd"); if (conf_split_config()) @@ -1053,12 +1222,51 @@ int conf_write_autoconf(void) return 1; } + for (spl = 0; spl < num_spls; spl++) { + char fname[80]; + + snprintf(fname, sizeof(fname), ".tmpconfig_%s", + spl_name[spl]); + + out_spl[spl] = fopen(fname, "w"); + if (!out_spl[spl]) { + while (spl--) { + fclose(out_spl[spl]); + fclose(out_h_spl[spl]); + } + fclose(out_h); + fclose(out); + fclose(tristate); + return 1; + } + + snprintf(fname, sizeof(fname), ".tmpconfig_%s.h", + spl_name[spl]); + + out_h_spl[spl] = fopen(fname, "w"); + if (!out_h_spl[spl]) { + fclose(out_spl[spl]); + while (spl--) { + fclose(out_spl[spl]); + fclose(out_h_spl[spl]); + } + fclose(out_h); + fclose(out); + fclose(tristate); + return 1; + } + } + conf_write_heading(out, &kconfig_printer_cb, NULL); conf_write_heading(tristate, &tristate_printer_cb, NULL); conf_write_heading(out_h, &header_printer_cb, NULL); + for (spl = 0; spl < num_spls; spl++) + conf_write_heading(out_h_spl[spl], &spl_header_printer_cb, + (void *)(long)spl); + for_all_symbols(i, sym) { sym_calc_value(sym); if (!(sym->flags & SYMBOL_WRITE) || !sym->name) @@ -1070,10 +1278,24 @@ int conf_write_autoconf(void) conf_write_symbol(tristate, sym, &tristate_printer_cb, (void *)1); conf_write_symbol(out_h, sym, &header_printer_cb, NULL); + + for (spl = 0; spl < num_spls; spl++) { + /* write make variables to auto_<spl>.conf */ + conf_write_symbol(out_spl[spl], sym, + &spl_kconfig_printer_cb, + (void *)(long)spl); + + /* write #defines to autoconf_<spl>.h */ + conf_write_symbol(out_h_spl[spl], sym, + &spl_header_printer_cb, + (void *)(long)spl); + } } fclose(out); fclose(tristate); fclose(out_h); + for (spl = 0; spl < num_spls; spl++) + fclose(out_h_spl[spl]); name = getenv("KCONFIG_AUTOHEADER"); if (!name) @@ -1083,6 +1305,29 @@ int conf_write_autoconf(void) if (rename(".tmpconfig.h", name)) return 1; + for (spl = 0; spl < num_spls; spl++) { + char tmpname[80], fname[80]; + char *s; + + snprintf(tmpname, sizeof(tmpname), ".tmpconfig_%s.h", + spl_name[spl]); + snprintf(fname, sizeof(fname), + "include/generated/autoconf_%s.h", spl_name[spl]); + for (s = fname; *s; s++) + *s = tolower(*s); + if (rename(tmpname, fname)) + return 1; + + snprintf(tmpname, sizeof(tmpname), ".tmpconfig_%s", + spl_name[spl]); + snprintf(fname, sizeof(fname), + "include/config/auto_%s.conf", spl_name[spl]); + for (s = fname; *s; s++) + *s = tolower(*s); + if (rename(tmpname, fname)) + return 1; + } + name = getenv("KCONFIG_TRISTATE"); if (!name) name = "include/config/tristate.conf"; @@ -1326,3 +1571,102 @@ bool conf_set_all_new_symbols(enum conf_def_mode mode) return has_changed; } + +static bool is_spl(const char *name, int *lenp) +{ + const char *uscore; + int len; + int i; + + uscore = strchr(name, '_'); + if (!uscore) + return false; + + len = uscore - name; + for (i = 0; i < num_spls; i++) { + if (len == strlen(spl_name[i]) && + !strncmp(name, spl_name[i], len)) { + *lenp = len; + return true; + } + } + + return false; +} + +void conf_mark_spl_symbols(void) +{ + struct symbol *sym; + int i; + + for_all_symbols(i, sym) { + if (sym->name) { + int len; + bool spl = is_spl(sym->name, &len); + + if (spl) { + struct symbol *non_spl; + + sym->flags |= SYMBOL_SPL; + non_spl = sym_find(sym->name + len + 1); + if (non_spl) + non_spl->flags |= SYMBOL_HAS_SPL; + } + } + } +} + +static int u_boot_read_conf_nospl(const char *fname) +{ + static char fullname[PATH_MAX + 1]; + const char *env; + char buf[256]; + FILE *f; + char *s; + + env = getenv(SRCTREE); + if (!env) { + fprintf(stderr, "No %s environment variable\n", SRCTREE); + return 1; + } + snprintf(fullname, sizeof(fullname), "%s/scripts/%s", env, fname); + f = fopen(fullname, "r"); + if (!f) { + fprintf(stderr, "Cannot open '%s'\n", fullname); + return 1; + } + while (s = fgets(buf, sizeof(buf), f), s) { + struct symbol *sym; + int len; + + len = strlen(s); + if (len) + s[len - 1] = '\0'; + if (*s == '#' || !*s) + continue; + + sym = sym_find(s); + if (!sym) { + /* + * perhaps we could drop these in config_nospl + * + * fprintf(stderr, "Unknown symbol from '%s': %s\n", + * fullname, s); + */ + continue; + } + sym->flags |= SYMBOL_NO_SPL; + } + fclose(f); + + return 0; +} + +int conf_mark_symbols(void) +{ + conf_mark_spl_symbols(); + if (u_boot_read_conf_nospl("conf_nospl")) + return 1; + + return 0; +} diff --git a/scripts/kconfig/expr.h b/scripts/kconfig/expr.h index 7c329e17900..934f4b10b25 100644 --- a/scripts/kconfig/expr.h +++ b/scripts/kconfig/expr.h @@ -158,6 +158,34 @@ struct symbol { /* Set symbol to y if allnoconfig; used for symbols that hide others */ #define SYMBOL_ALLNOCONFIG_Y 0x200000 +/* + * U-Boot: Marks an SPL symbol, i.e. one that starts with one of the strings in + * spl_name[] + */ +#define SYMBOL_SPL 0x400000 + +/* + * U-Boot: Marks a non-SPL symbol that also has an SPL version. This flag is + * added to symbols like FOO if there is also an SPL_FOO (or TPL_FOO, etc.) + */ +#define SYMBOL_HAS_SPL 0x800000 + +/* + * U-Boot: Marks a symbol with no SPL version. These symbols cannot be computed + * by looking at the Kconfig, so there is a conf_nospl file which holds s list. + * + * Generally, options which have no SPL_ prefix (e.g. CONFIG_FOO) apply to all + * SPL build phases. This allows things like ARCH_ARM to propagate to all builds + * without the hassle of generating a separate SPL version fo each phase. But in + * some cases this is not wanted. + * + * This file lists options which don't have an SPL equivalent, but still should + * not be enabled in SPL builds. It is necessary since kconfig cannot tell (just + * by looking at the Kconfig description) whether it applies to Proper builds + * only, or to all builds. + */ +#define SYMBOL_NO_SPL 0x1000000 + #define SYMBOL_MAXLENGTH 256 #define SYMBOL_HASHSIZE 9973 diff --git a/scripts/kconfig/lkc.h b/scripts/kconfig/lkc.h index 9eb7c837cd8..592610483f0 100644 --- a/scripts/kconfig/lkc.h +++ b/scripts/kconfig/lkc.h @@ -71,6 +71,15 @@ void sym_add_change_count(int count); bool conf_set_all_new_symbols(enum conf_def_mode mode); void set_all_choice_values(struct symbol *csym); +/** + * conf_mark_symbols() - Mark symbols with U-Boot flags + * + * Symbols which don't have an SPL symbol are marked with SYMBOL_PROPER_ONLY and + * those with only SPL symbols are marked withSYMBOL_SPL_ONLY, so we know to + * avoid writing them to the wrong autoconf.h files. + */ +int conf_mark_symbols(void); + /* confdata.c and expr.c */ static inline void xfwrite(const void *str, size_t len, size_t count, FILE *out) { diff --git a/scripts/kconfig/lkc_proto.h b/scripts/kconfig/lkc_proto.h index cf4510a2bdc..4707a9155f7 100644 --- a/scripts/kconfig/lkc_proto.h +++ b/scripts/kconfig/lkc_proto.h @@ -7,7 +7,23 @@ int conf_read(const char *name); int conf_read_simple(const char *name, int); int conf_write_defconfig(const char *name); int conf_write(const char *name); -int conf_write_autoconf(void); + +/** + * conf_write_autoconf() - Write out the autoconf files + * + * @support_spl: Support generation of files for additional secondary + * program-loader builds, as used by U-Boot + * + * Writes out: + * auto.conf which contains CONFIG definitions for inclusion by make + * autoconf.h which contains CONFIG definitions for inclusion by C code + * + * if @support_spl then separate files are also created for SPL builds (spl, + * tpl, vpl, tools), with symbols like SPL_FOO being written to the SPL file + * as just FOO + */ +int conf_write_autoconf(bool support_spl); + bool conf_get_changed(void); void conf_set_changed_callback(void (*fn)(void)); void conf_set_message_callback(void (*fn)(const char *s));
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@chromium.org> --- Changes in v3: - Make sure that the config is rebuilt with this change - Move header-file changes into this patch Changes in v2: - Redo commit message - Introduce CONFIG_PPL approach Makefile | 6 + scripts/kconfig/Makefile | 5 +- scripts/kconfig/conf.c | 6 +- scripts/kconfig/confdata.c | 348 +++++++++++++++++++++++++++++++++++- scripts/kconfig/expr.h | 28 +++ scripts/kconfig/lkc.h | 9 + scripts/kconfig/lkc_proto.h | 18 +- 7 files changed, 415 insertions(+), 5 deletions(-)