RFC: Support for U-Boot phases in Kconfig
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
At present we do things like this when we need to control an option separately in U-Boot proper and SPL:
config SYS_STDIO_DEREGISTER bool "Allow deregistering stdio devices" depends on DM_DEVICE_REMOVE default y if USB_KEYBOARD help Generally there is no need to deregister stdio devices since they are never deactivated. But if a stdio device is used which can be removed (for example a USB keyboard) then this option can be enabled to ensure this is handled correctly.
config SPL_SYS_STDIO_DEREGISTER bool "Allow deregistering stdio devices in SPL" depends on SPL_DM_DEVICE_REMOVE help Generally there is no need to deregister stdio devices since they are never deactivated. But if a stdio device is used which can be removed (for example a USB keyboard) then this option can be enabled to ensure this is handled correctly. This is very rarely needed in SPL.
This is a pain. Apart from the duplication, sometimes the SPL version is in a different file or a different part of the file, making it hard to find related options or update them in sync.
Instead, we can add a 'phase' command to the Kconfig language, so we can do:
config SYS_STDIO_DEREGISTER bool "Allow deregistering stdio devices" phases depends on p.DM_DEVICE_REMOVE phase MAIN default y if USB_KEYBOARD help Generally there is no need to deregister stdio devices since they are never deactivated. But if a stdio device is used which can be removed (for example a USB keyboard) then this option can be enabled to ensure this is handled correctly.
'phases' means this Kconfig option exists in all phases. You can also say 'phases MAIN SPL' to select just MAIN (U-Boot) and SPL.
'p.DM_DEVICE_REMOVE' means to prefix the phase with each symbol, so for U-Boot (which uses SYS_STDIO_DEREGISTER) this means DM_DEVICE_REMOVE (p is empty) and for SPL (which uses SPL_SYS_STDIO_DEREGISTER) it means SPL_DM_DEVICE_REMOVE (p is SPL_). This is somewhat similar in style to the special-case 'depends on m' in Kconfig.
To make this work, we tell Kconfig that SPL is a phase with 'def_phase':
config SPL def_phase depends on SUPPORT_SPL prompt "Enable SPL" help If you want to build SPL as well as the normal image, say Y.
It works just the same as a bool, but kconfig also uses it to automatically add new Kconfigs for each phase. In the above example it creates both SYS_STDIO_DEREGISTER and SPL_SYS_STDIO_DEREGISTER. The option name has the text '(SPL) ' shown before the SPL option.
This can easily handle Kconfigs with similar dependencies and even different ones. If the Kconfig options are not actually very similar we can still create two separate copies instead, as now.
This allows us to substantially reduce the size and duplication in the Kconfig defintions. It also reduces the pain of adding another phase to U-Boot.
Note: This change needs to be done in Linux, which owns Kconfig upstream.
2.Rename SPL_TPL_
This Makefile variable is used to reduce the number of duplicate rules in makefiles:
obj-$(CONFIG_$(SPL_TPL_)FIT_SIGNATURE) += fdt_region.o
The SPL_TPL_ expands to empty for U-Boot and either SPL_ or TPL_ for the other phases.
This is confusing though, since CONFIG_SPL_BUILD it set even for TPL builds, so for example. with:
obj-$(CONFIG_$(SPL_)FIT_SIGNATURE) += fdt_region.o
the file is built for both SPL and TPL.
To help with this, We can rename SPL_TPL to PHASE_:
obj-$(CONFIG_$(PHASE_)FIT_SIGNATURE) += fdt_region.o
or perhaps P_ which is more readable:
obj-$(CONFIG_$(P_)FIT_SIGNATURE) += fdt_region.o
3. Rename CONFIG_IS_ENABLED()
This macro is used to determine whether an option is enabled in the current build phase:
if (CONFIG_IS_ENABLED(FIT_SIGNATURE)) { printf("## Checking hash(es) for Image %s ... ", fit_get_name(fit, node, NULL));
It is quite long-winded and people sometimes add CONFIG_ to the option inside the brakets by mistake. It is also a bit confusing that IS_ENABLED() and CONFIG_IS_ENABLED() mean completely different things.
Instead we can rename it to CONFIG:
if (CONFIG(FIT_SIGNATURE)) { printf("## Checking hash(es) for Image %s ... ", fit_get_name(fit, node, NULL));
This is shorter and looks more like CONFIG_FIT_SIGATURE so people should find it easier to understand. Being shorter is a big help when converting from use #if to if(), since an indentation is always enabled. This change makes the CONFIG() check no longer than IS_ENABLED().
It also makes CONFIG(OPTION) not much longer than CONFIG_OPTION, which makes things much more convenient, since ideally if the toolchain permitted it, we would just use CONFIG_OPTION in the code. This is not possible at present since the option may not be defined, so can cause a compiler error.
Over time, perhaps the existing IS_ENABLED() will phase out, since in many cases SPL will have its own options. We already see that CONFIG_IS_ENABLED is more popular / useful:
$ git grep -w IS_ENABLED |wc -l 902 $ git grep -w CONFIG_IS_ENABLED |wc -l 2282
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,) ... };
It still isn't wonderfully readable but it seems like an improvement. The IF_CONFIG() macros could be implemented easily with the current CONFIG_IS_EANBLED() macro.
5. IF_CONFIG_INT() or similar
See here: https://lists.denx.de/pipermail/u-boot/2020-May/412950.html
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.
Comments welcome!
- Simon
On Sat, Aug 07, 2021 at 04:23:36PM -0600, Simon Glass wrote:
Hi,
U-Boot can be configured to build the source multiple times to product multiple 'phases'. The main phase is the full U-Boot, but an 'SPL' (Secondary Program Loader) build can produce a cut-down image only suitable for loading U-Boot proper.
SPL does not want to use the same Kconfig options, since that would produce the same binary. Instead we have two copies of some options, one with an SPL prefix, that can be configured independently. In the source code we can use a macro to see if code should be run:
if (CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER)) { ... }
This expands to check either checking SYS_STDIO_DEREGISTER or SPL_SYS_STDIO_DEREGISTER, depending on which phase is being built.
U-Boot also has TPL (Tertiary Program Loader) which works a similar way. This is causing quite an expansion of the Kconfig source, with quite a bit of duplication. Each time a new feature needs to be supported in SPL, it involves a patch to add the same options again but for SPL.
Here are some proposed changes to make it easier to deal with SPL/TPL:
- Kconfig support
At present we do things like this when we need to control an option separately in U-Boot proper and SPL:
config SYS_STDIO_DEREGISTER bool "Allow deregistering stdio devices" depends on DM_DEVICE_REMOVE default y if USB_KEYBOARD help Generally there is no need to deregister stdio devices since they are never deactivated. But if a stdio device is used which can be removed (for example a USB keyboard) then this option can be enabled to ensure this is handled correctly. config SPL_SYS_STDIO_DEREGISTER bool "Allow deregistering stdio devices in SPL" depends on SPL_DM_DEVICE_REMOVE help Generally there is no need to deregister stdio devices since they are never deactivated. But if a stdio device is used which can be removed (for example a USB keyboard) then this option can be enabled to ensure this is handled correctly. This is very rarely needed in SPL.This is a pain. Apart from the duplication, sometimes the SPL version is in a different file or a different part of the file, making it hard to find related options or update them in sync.
Instead, we can add a 'phase' command to the Kconfig language, so we can do:
config SYS_STDIO_DEREGISTER bool "Allow deregistering stdio devices" phases depends on p.DM_DEVICE_REMOVE phase MAIN default y if USB_KEYBOARD help Generally there is no need to deregister stdio devices since they are never deactivated. But if a stdio device is used which can be removed (for example a USB keyboard) then this option can be enabled to ensure this is handled correctly.'phases' means this Kconfig option exists in all phases. You can also say 'phases MAIN SPL' to select just MAIN (U-Boot) and SPL.
'p.DM_DEVICE_REMOVE' means to prefix the phase with each symbol, so for U-Boot (which uses SYS_STDIO_DEREGISTER) this means DM_DEVICE_REMOVE (p is empty) and for SPL (which uses SPL_SYS_STDIO_DEREGISTER) it means SPL_DM_DEVICE_REMOVE (p is SPL_). This is somewhat similar in style to the special-case 'depends on m' in Kconfig.
To make this work, we tell Kconfig that SPL is a phase with 'def_phase':
config SPL def_phase depends on SUPPORT_SPL prompt "Enable SPL" help If you want to build SPL as well as the normal image, say Y.It works just the same as a bool, but kconfig also uses it to automatically add new Kconfigs for each phase. In the above example it creates both SYS_STDIO_DEREGISTER and SPL_SYS_STDIO_DEREGISTER. The option name has the text '(SPL) ' shown before the SPL option.
This can easily handle Kconfigs with similar dependencies and even different ones. If the Kconfig options are not actually very similar we can still create two separate copies instead, as now.
This allows us to substantially reduce the size and duplication in the Kconfig defintions. It also reduces the pain of adding another phase to U-Boot.
Note: This change needs to be done in Linux, which owns Kconfig upstream.
2.Rename SPL_TPL_
This Makefile variable is used to reduce the number of duplicate rules in makefiles:
obj-$(CONFIG_$(SPL_TPL_)FIT_SIGNATURE) += fdt_region.oThe SPL_TPL_ expands to empty for U-Boot and either SPL_ or TPL_ for the other phases.
This is confusing though, since CONFIG_SPL_BUILD it set even for TPL builds, so for example. with:
obj-$(CONFIG_$(SPL_)FIT_SIGNATURE) += fdt_region.othe file is built for both SPL and TPL.
To help with this, We can rename SPL_TPL to PHASE_:
obj-$(CONFIG_$(PHASE_)FIT_SIGNATURE) += fdt_region.oor perhaps P_ which is more readable:
obj-$(CONFIG_$(P_)FIT_SIGNATURE) += fdt_region.o
- Rename CONFIG_IS_ENABLED()
This macro is used to determine whether an option is enabled in the current build phase:
if (CONFIG_IS_ENABLED(FIT_SIGNATURE)) { printf("## Checking hash(es) for Image %s ... ", fit_get_name(fit, node, NULL));
It is quite long-winded and people sometimes add CONFIG_ to the option inside the brakets by mistake. It is also a bit confusing that IS_ENABLED() and CONFIG_IS_ENABLED() mean completely different things.
Instead we can rename it to CONFIG:
if (CONFIG(FIT_SIGNATURE)) { printf("## Checking hash(es) for Image %s ... ", fit_get_name(fit, node, NULL));
This is shorter and looks more like CONFIG_FIT_SIGATURE so people should find it easier to understand. Being shorter is a big help when converting from use #if to if(), since an indentation is always enabled. This change makes the CONFIG() check no longer than IS_ENABLED().
It also makes CONFIG(OPTION) not much longer than CONFIG_OPTION, which makes things much more convenient, since ideally if the toolchain permitted it, we would just use CONFIG_OPTION in the code. This is not possible at present since the option may not be defined, so can cause a compiler error.
Over time, perhaps the existing IS_ENABLED() will phase out, since in many cases SPL will have its own options. We already see that CONFIG_IS_ENABLED is more popular / useful:
$ git grep -w IS_ENABLED |wc -l 902 $ git grep -w CONFIG_IS_ENABLED |wc -l 2282
- 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,) ... };It still isn't wonderfully readable but it seems like an improvement. The IF_CONFIG() macros could be implemented easily with the current CONFIG_IS_EANBLED() macro.
- IF_CONFIG_INT() or similar
See here: https://lists.denx.de/pipermail/u-boot/2020-May/412950.html
- 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_idapl_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.
Comments welcome!
I think what this is really showing is that Yamada-san was right. All the games we need to do so that "make fooboard_config all" results in building the N stages needed was the wrong track. Taking khadas-edge-v-rk3399 as an example, if we had instead of khadas-edge-v-rk3399_defconfig but khadas-edge-v-rk3399_tpl_defconfig khadas-edge-v-rk3399_spl_defconfig and khadas-edge-v-rk3399_config, each of which could set CONFIG_TPL, CONFIG_SPL or neither. Then yes, to build u-boot-rockchip.bin you would need to pass in the separately build TPL and SPL stages. But, that's true of so so many other platforms. To pick another example, imx8mm_evk doesn't function without other binaries. If in theory to build khadas-edge-v-rk3399 you had to do something like: $ export CROSS_COMPILE=aarch64-linux- $ make O=obj/tpl khadas-edge-v-rk3399_tpl_config all $ make O=obj/spl khadas-edge-v-rk3399_spl_config all $ make O=obj/khadas-edge-v-rk3399 TPL=obj/tpl/u-boot.bin \ SPL=obj/spl/u-boot.bin khadas-edge-v-rk3399_config all
But it also meant that we didn't need to duplicate so so many Kconfig options and most of our obj rules would just be: obj-$(CONFIG_FOO) += foo.o
would be a win.
Hi Tom,
On Mon, 9 Aug 2021 at 13:11, Tom Rini trini@konsulko.com wrote:
On Sat, Aug 07, 2021 at 04:23:36PM -0600, Simon Glass wrote:
Hi,
U-Boot can be configured to build the source multiple times to product multiple 'phases'. The main phase is the full U-Boot, but an 'SPL' (Secondary Program Loader) build can produce a cut-down image only suitable for loading U-Boot proper.
SPL does not want to use the same Kconfig options, since that would produce the same binary. Instead we have two copies of some options, one with an SPL prefix, that can be configured independently. In the source code we can use a macro to see if code should be run:
if (CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER)) { ... }
This expands to check either checking SYS_STDIO_DEREGISTER or SPL_SYS_STDIO_DEREGISTER, depending on which phase is being built.
U-Boot also has TPL (Tertiary Program Loader) which works a similar way. This is causing quite an expansion of the Kconfig source, with quite a bit of duplication. Each time a new feature needs to be supported in SPL, it involves a patch to add the same options again but for SPL.
Here are some proposed changes to make it easier to deal with SPL/TPL:
- Kconfig support
At present we do things like this when we need to control an option separately in U-Boot proper and SPL:
config SYS_STDIO_DEREGISTER bool "Allow deregistering stdio devices" depends on DM_DEVICE_REMOVE default y if USB_KEYBOARD help Generally there is no need to deregister stdio devices since they are never deactivated. But if a stdio device is used which can be removed (for example a USB keyboard) then this option can be enabled to ensure this is handled correctly. config SPL_SYS_STDIO_DEREGISTER bool "Allow deregistering stdio devices in SPL" depends on SPL_DM_DEVICE_REMOVE help Generally there is no need to deregister stdio devices since they are never deactivated. But if a stdio device is used which can be removed (for example a USB keyboard) then this option can be enabled to ensure this is handled correctly. This is very rarely needed in SPL.This is a pain. Apart from the duplication, sometimes the SPL version is in a different file or a different part of the file, making it hard to find related options or update them in sync.
Instead, we can add a 'phase' command to the Kconfig language, so we can do:
config SYS_STDIO_DEREGISTER bool "Allow deregistering stdio devices" phases depends on p.DM_DEVICE_REMOVE phase MAIN default y if USB_KEYBOARD help Generally there is no need to deregister stdio devices since they are never deactivated. But if a stdio device is used which can be removed (for example a USB keyboard) then this option can be enabled to ensure this is handled correctly.'phases' means this Kconfig option exists in all phases. You can also say 'phases MAIN SPL' to select just MAIN (U-Boot) and SPL.
'p.DM_DEVICE_REMOVE' means to prefix the phase with each symbol, so for U-Boot (which uses SYS_STDIO_DEREGISTER) this means DM_DEVICE_REMOVE (p is empty) and for SPL (which uses SPL_SYS_STDIO_DEREGISTER) it means SPL_DM_DEVICE_REMOVE (p is SPL_). This is somewhat similar in style to the special-case 'depends on m' in Kconfig.
To make this work, we tell Kconfig that SPL is a phase with 'def_phase':
config SPL def_phase depends on SUPPORT_SPL prompt "Enable SPL" help If you want to build SPL as well as the normal image, say Y.It works just the same as a bool, but kconfig also uses it to automatically add new Kconfigs for each phase. In the above example it creates both SYS_STDIO_DEREGISTER and SPL_SYS_STDIO_DEREGISTER. The option name has the text '(SPL) ' shown before the SPL option.
This can easily handle Kconfigs with similar dependencies and even different ones. If the Kconfig options are not actually very similar we can still create two separate copies instead, as now.
This allows us to substantially reduce the size and duplication in the Kconfig defintions. It also reduces the pain of adding another phase to U-Boot.
Note: This change needs to be done in Linux, which owns Kconfig upstream.
2.Rename SPL_TPL_
This Makefile variable is used to reduce the number of duplicate rules in makefiles:
obj-$(CONFIG_$(SPL_TPL_)FIT_SIGNATURE) += fdt_region.oThe SPL_TPL_ expands to empty for U-Boot and either SPL_ or TPL_ for the other phases.
This is confusing though, since CONFIG_SPL_BUILD it set even for TPL builds, so for example. with:
obj-$(CONFIG_$(SPL_)FIT_SIGNATURE) += fdt_region.othe file is built for both SPL and TPL.
To help with this, We can rename SPL_TPL to PHASE_:
obj-$(CONFIG_$(PHASE_)FIT_SIGNATURE) += fdt_region.oor perhaps P_ which is more readable:
obj-$(CONFIG_$(P_)FIT_SIGNATURE) += fdt_region.o
- Rename CONFIG_IS_ENABLED()
This macro is used to determine whether an option is enabled in the current build phase:
if (CONFIG_IS_ENABLED(FIT_SIGNATURE)) { printf("## Checking hash(es) for Image %s ... ", fit_get_name(fit, node, NULL));
It is quite long-winded and people sometimes add CONFIG_ to the option inside the brakets by mistake. It is also a bit confusing that IS_ENABLED() and CONFIG_IS_ENABLED() mean completely different things.
Instead we can rename it to CONFIG:
if (CONFIG(FIT_SIGNATURE)) { printf("## Checking hash(es) for Image %s ... ", fit_get_name(fit, node, NULL));
This is shorter and looks more like CONFIG_FIT_SIGATURE so people should find it easier to understand. Being shorter is a big help when converting from use #if to if(), since an indentation is always enabled. This change makes the CONFIG() check no longer than IS_ENABLED().
It also makes CONFIG(OPTION) not much longer than CONFIG_OPTION, which makes things much more convenient, since ideally if the toolchain permitted it, we would just use CONFIG_OPTION in the code. This is not possible at present since the option may not be defined, so can cause a compiler error.
Over time, perhaps the existing IS_ENABLED() will phase out, since in many cases SPL will have its own options. We already see that CONFIG_IS_ENABLED is more popular / useful:
$ git grep -w IS_ENABLED |wc -l 902 $ git grep -w CONFIG_IS_ENABLED |wc -l 2282
- 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,) ... };It still isn't wonderfully readable but it seems like an improvement. The IF_CONFIG() macros could be implemented easily with the current CONFIG_IS_EANBLED() macro.
- IF_CONFIG_INT() or similar
See here: https://lists.denx.de/pipermail/u-boot/2020-May/412950.html
- 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_idapl_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.
Comments welcome!
I think what this is really showing is that Yamada-san was right. All
One thread where this was discussed was here I think:
https://yhbt.net/lore/all/20140624192425.9368.AA925319@jp.panasonic.com/T/
I cannot find all the arguments for either side now. Do you have a pointer to them?
the games we need to do so that "make fooboard_config all" results in building the N stages needed was the wrong track. Taking khadas-edge-v-rk3399 as an example, if we had instead of khadas-edge-v-rk3399_defconfig but khadas-edge-v-rk3399_tpl_defconfig khadas-edge-v-rk3399_spl_defconfig and khadas-edge-v-rk3399_config, each of which could set CONFIG_TPL, CONFIG_SPL or neither. Then yes, to build u-boot-rockchip.bin you would need to pass in the separately build TPL and SPL stages. But, that's true of so so many other platforms. To pick another example, imx8mm_evk doesn't function without other binaries. If in theory to build khadas-edge-v-rk3399 you had to do something like: $ export CROSS_COMPILE=aarch64-linux- $ make O=obj/tpl khadas-edge-v-rk3399_tpl_config all $ make O=obj/spl khadas-edge-v-rk3399_spl_config all $ make O=obj/khadas-edge-v-rk3399 TPL=obj/tpl/u-boot.bin \ SPL=obj/spl/u-boot.bin khadas-edge-v-rk3399_config all
We also need to think about the tools which are presumably built separately.
We might have to build in the opposite order, because SPL needs to grep the full devicetree, although I suppose we could just recompile it.
But it also meant that we didn't need to duplicate so so many Kconfig options and most of our obj rules would just be: obj-$(CONFIG_FOO) += foo.o
would be a win.
That definitely looks nice.
But how much of a win is this?
I understand how it could work and I think we did talk about this at the time. But there are problems too that I'd like to review if we can find them. Some I can think of:
- maintaining three or more separate defconfig files for each board - not sure how to handle dependencies between phases (e.g. SPL_BLOBLIST has ''default y if BLOBLIST', or one phase expecting an image to be in there) - running 'make menuconfig' updates one phase but not the others, making things harder to understand - splitting up of the build as you note above, making it harder for people to understand
Interesting to see this comment:
http://u-boot.10912.n7.nabble.com/PATCH-v8-0-13-Kconfig-for-U-Boot-tt185309....
"It would take really long term (one year? two year? I don't know) to migrate from board headers to Kconfig.
So, two different infractructure must coexist in the interim."
That was 2014! I think we need a way to remove old CONFIGs and let board maintainers add them back in Kconfig if needed.
Regards, Simon
On Tue, Aug 10, 2021 at 08:58:46AM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 9 Aug 2021 at 13:11, Tom Rini trini@konsulko.com wrote:
On Sat, Aug 07, 2021 at 04:23:36PM -0600, Simon Glass wrote:
Hi,
U-Boot can be configured to build the source multiple times to product multiple 'phases'. The main phase is the full U-Boot, but an 'SPL' (Secondary Program Loader) build can produce a cut-down image only suitable for loading U-Boot proper.
SPL does not want to use the same Kconfig options, since that would produce the same binary. Instead we have two copies of some options, one with an SPL prefix, that can be configured independently. In the source code we can use a macro to see if code should be run:
if (CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER)) { ... }
This expands to check either checking SYS_STDIO_DEREGISTER or SPL_SYS_STDIO_DEREGISTER, depending on which phase is being built.
U-Boot also has TPL (Tertiary Program Loader) which works a similar way. This is causing quite an expansion of the Kconfig source, with quite a bit of duplication. Each time a new feature needs to be supported in SPL, it involves a patch to add the same options again but for SPL.
Here are some proposed changes to make it easier to deal with SPL/TPL:
- Kconfig support
At present we do things like this when we need to control an option separately in U-Boot proper and SPL:
config SYS_STDIO_DEREGISTER bool "Allow deregistering stdio devices" depends on DM_DEVICE_REMOVE default y if USB_KEYBOARD help Generally there is no need to deregister stdio devices since they are never deactivated. But if a stdio device is used which can be removed (for example a USB keyboard) then this option can be enabled to ensure this is handled correctly. config SPL_SYS_STDIO_DEREGISTER bool "Allow deregistering stdio devices in SPL" depends on SPL_DM_DEVICE_REMOVE help Generally there is no need to deregister stdio devices since they are never deactivated. But if a stdio device is used which can be removed (for example a USB keyboard) then this option can be enabled to ensure this is handled correctly. This is very rarely needed in SPL.This is a pain. Apart from the duplication, sometimes the SPL version is in a different file or a different part of the file, making it hard to find related options or update them in sync.
Instead, we can add a 'phase' command to the Kconfig language, so we can do:
config SYS_STDIO_DEREGISTER bool "Allow deregistering stdio devices" phases depends on p.DM_DEVICE_REMOVE phase MAIN default y if USB_KEYBOARD help Generally there is no need to deregister stdio devices since they are never deactivated. But if a stdio device is used which can be removed (for example a USB keyboard) then this option can be enabled to ensure this is handled correctly.'phases' means this Kconfig option exists in all phases. You can also say 'phases MAIN SPL' to select just MAIN (U-Boot) and SPL.
'p.DM_DEVICE_REMOVE' means to prefix the phase with each symbol, so for U-Boot (which uses SYS_STDIO_DEREGISTER) this means DM_DEVICE_REMOVE (p is empty) and for SPL (which uses SPL_SYS_STDIO_DEREGISTER) it means SPL_DM_DEVICE_REMOVE (p is SPL_). This is somewhat similar in style to the special-case 'depends on m' in Kconfig.
To make this work, we tell Kconfig that SPL is a phase with 'def_phase':
config SPL def_phase depends on SUPPORT_SPL prompt "Enable SPL" help If you want to build SPL as well as the normal image, say Y.It works just the same as a bool, but kconfig also uses it to automatically add new Kconfigs for each phase. In the above example it creates both SYS_STDIO_DEREGISTER and SPL_SYS_STDIO_DEREGISTER. The option name has the text '(SPL) ' shown before the SPL option.
This can easily handle Kconfigs with similar dependencies and even different ones. If the Kconfig options are not actually very similar we can still create two separate copies instead, as now.
This allows us to substantially reduce the size and duplication in the Kconfig defintions. It also reduces the pain of adding another phase to U-Boot.
Note: This change needs to be done in Linux, which owns Kconfig upstream.
2.Rename SPL_TPL_
This Makefile variable is used to reduce the number of duplicate rules in makefiles:
obj-$(CONFIG_$(SPL_TPL_)FIT_SIGNATURE) += fdt_region.oThe SPL_TPL_ expands to empty for U-Boot and either SPL_ or TPL_ for the other phases.
This is confusing though, since CONFIG_SPL_BUILD it set even for TPL builds, so for example. with:
obj-$(CONFIG_$(SPL_)FIT_SIGNATURE) += fdt_region.othe file is built for both SPL and TPL.
To help with this, We can rename SPL_TPL to PHASE_:
obj-$(CONFIG_$(PHASE_)FIT_SIGNATURE) += fdt_region.oor perhaps P_ which is more readable:
obj-$(CONFIG_$(P_)FIT_SIGNATURE) += fdt_region.o
- Rename CONFIG_IS_ENABLED()
This macro is used to determine whether an option is enabled in the current build phase:
if (CONFIG_IS_ENABLED(FIT_SIGNATURE)) { printf("## Checking hash(es) for Image %s ... ", fit_get_name(fit, node, NULL));
It is quite long-winded and people sometimes add CONFIG_ to the option inside the brakets by mistake. It is also a bit confusing that IS_ENABLED() and CONFIG_IS_ENABLED() mean completely different things.
Instead we can rename it to CONFIG:
if (CONFIG(FIT_SIGNATURE)) { printf("## Checking hash(es) for Image %s ... ", fit_get_name(fit, node, NULL));
This is shorter and looks more like CONFIG_FIT_SIGATURE so people should find it easier to understand. Being shorter is a big help when converting from use #if to if(), since an indentation is always enabled. This change makes the CONFIG() check no longer than IS_ENABLED().
It also makes CONFIG(OPTION) not much longer than CONFIG_OPTION, which makes things much more convenient, since ideally if the toolchain permitted it, we would just use CONFIG_OPTION in the code. This is not possible at present since the option may not be defined, so can cause a compiler error.
Over time, perhaps the existing IS_ENABLED() will phase out, since in many cases SPL will have its own options. We already see that CONFIG_IS_ENABLED is more popular / useful:
$ git grep -w IS_ENABLED |wc -l 902 $ git grep -w CONFIG_IS_ENABLED |wc -l 2282
- 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,) ... };It still isn't wonderfully readable but it seems like an improvement. The IF_CONFIG() macros could be implemented easily with the current CONFIG_IS_EANBLED() macro.
- IF_CONFIG_INT() or similar
See here: https://lists.denx.de/pipermail/u-boot/2020-May/412950.html
- 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_idapl_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.
Comments welcome!
I think what this is really showing is that Yamada-san was right. All
One thread where this was discussed was here I think:
https://yhbt.net/lore/all/20140624192425.9368.AA925319@jp.panasonic.com/T/
I cannot find all the arguments for either side now. Do you have a pointer to them?
I don't off-hand. I'm pretty sure it's come up more than once tho.
the games we need to do so that "make fooboard_config all" results in building the N stages needed was the wrong track. Taking khadas-edge-v-rk3399 as an example, if we had instead of khadas-edge-v-rk3399_defconfig but khadas-edge-v-rk3399_tpl_defconfig khadas-edge-v-rk3399_spl_defconfig and khadas-edge-v-rk3399_config, each of which could set CONFIG_TPL, CONFIG_SPL or neither. Then yes, to build u-boot-rockchip.bin you would need to pass in the separately build TPL and SPL stages. But, that's true of so so many other platforms. To pick another example, imx8mm_evk doesn't function without other binaries. If in theory to build khadas-edge-v-rk3399 you had to do something like: $ export CROSS_COMPILE=aarch64-linux- $ make O=obj/tpl khadas-edge-v-rk3399_tpl_config all $ make O=obj/spl khadas-edge-v-rk3399_spl_config all $ make O=obj/khadas-edge-v-rk3399 TPL=obj/tpl/u-boot.bin \ SPL=obj/spl/u-boot.bin khadas-edge-v-rk3399_config all
We also need to think about the tools which are presumably built separately.
I'd like to see a way to build less host tools by default, but I fear the problem of distro folks if we go that way as it's more of a world builder / CI person problem that we build so many tools every time.
We might have to build in the opposite order, because SPL needs to grep the full devicetree, although I suppose we could just recompile it.
If we use dtb files from one stage in another stage directly (rather than being an included / packaged file) that seems like a potential problem.
But it also meant that we didn't need to duplicate so so many Kconfig options and most of our obj rules would just be: obj-$(CONFIG_FOO) += foo.o
would be a win.
That definitely looks nice.
But how much of a win is this?
I think a good sized one. Especially since it will let us remove some customization in the build code.
I understand how it could work and I think we did talk about this at the time. But there are problems too that I'd like to review if we can find them. Some I can think of:
- maintaining three or more separate defconfig files for each board
Somewhat, yes. The SPL/TPL configs should be small. With some not overly judicious use of imply's in board/.../Kconfig it shouldn't be very much. I think it might emphasize that we do need a good way / place to set defaults for SoCs and boards.
- not sure how to handle dependencies between phases (e.g.
SPL_BLOBLIST has ''default y if BLOBLIST', or one phase expecting an image to be in there)
My first, but perhaps bad idea would be that we have say TARGET_AM335X_EVM in arch/arm/mach-omap2/am33xx/Kconfig still and a new TARGET_AM335X_EVM_OPTS in board/ti/am335x/Kconfig that would select/imply things that need to be enabled in all stages.
- running 'make menuconfig' updates one phase but not the others,
making things harder to understand
I'm not sure this is a problem so much. TPL/SPL shouldn't have much configuring to them, and even less re-configuring.
- splitting up of the build as you note above, making it harder for
people to understand
This I think is debatable. That we build and configure things the way we do isn't always obvious. More and better documentation, either way, would be good.
Interesting to see this comment:
http://u-boot.10912.n7.nabble.com/PATCH-v8-0-13-Kconfig-for-U-Boot-tt185309....
"It would take really long term (one year? two year? I don't know) to migrate from board headers to Kconfig.
So, two different infractructure must coexist in the interim."
That was 2014! I think we need a way to remove old CONFIGs and let board maintainers add them back in Kconfig if needed.
I need to take another pass at converting a bunch of symbols, to see where we're at. Probably the biggest chunk of progress next would be to start converting CONFIG_SYS_xxx to SYS_xxx and moving defines out of config.h and in to something else. I'm taking a peek at some of the remaining PCI ones now.
Hi Tom,
On Tue, 10 Aug 2021 at 13:38, Tom Rini trini@konsulko.com wrote:
On Tue, Aug 10, 2021 at 08:58:46AM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 9 Aug 2021 at 13:11, Tom Rini trini@konsulko.com wrote:
On Sat, Aug 07, 2021 at 04:23:36PM -0600, Simon Glass wrote:
Hi,
U-Boot can be configured to build the source multiple times to product multiple 'phases'. The main phase is the full U-Boot, but an 'SPL' (Secondary Program Loader) build can produce a cut-down image only suitable for loading U-Boot proper.
SPL does not want to use the same Kconfig options, since that would produce the same binary. Instead we have two copies of some options, one with an SPL prefix, that can be configured independently. In the source code we can use a macro to see if code should be run:
if (CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER)) { ... }
This expands to check either checking SYS_STDIO_DEREGISTER or SPL_SYS_STDIO_DEREGISTER, depending on which phase is being built.
U-Boot also has TPL (Tertiary Program Loader) which works a similar way. This is causing quite an expansion of the Kconfig source, with quite a bit of duplication. Each time a new feature needs to be supported in SPL, it involves a patch to add the same options again but for SPL.
Here are some proposed changes to make it easier to deal with SPL/TPL:
- Kconfig support
At present we do things like this when we need to control an option separately in U-Boot proper and SPL:
config SYS_STDIO_DEREGISTER bool "Allow deregistering stdio devices" depends on DM_DEVICE_REMOVE default y if USB_KEYBOARD help Generally there is no need to deregister stdio devices since they are never deactivated. But if a stdio device is used which can be removed (for example a USB keyboard) then this option can be enabled to ensure this is handled correctly. config SPL_SYS_STDIO_DEREGISTER bool "Allow deregistering stdio devices in SPL" depends on SPL_DM_DEVICE_REMOVE help Generally there is no need to deregister stdio devices since they are never deactivated. But if a stdio device is used which can be removed (for example a USB keyboard) then this option can be enabled to ensure this is handled correctly. This is very rarely needed in SPL.This is a pain. Apart from the duplication, sometimes the SPL version is in a different file or a different part of the file, making it hard to find related options or update them in sync.
Instead, we can add a 'phase' command to the Kconfig language, so we can do:
config SYS_STDIO_DEREGISTER bool "Allow deregistering stdio devices" phases depends on p.DM_DEVICE_REMOVE phase MAIN default y if USB_KEYBOARD help Generally there is no need to deregister stdio devices since they are never deactivated. But if a stdio device is used which can be removed (for example a USB keyboard) then this option can be enabled to ensure this is handled correctly.'phases' means this Kconfig option exists in all phases. You can also say 'phases MAIN SPL' to select just MAIN (U-Boot) and SPL.
'p.DM_DEVICE_REMOVE' means to prefix the phase with each symbol, so for U-Boot (which uses SYS_STDIO_DEREGISTER) this means DM_DEVICE_REMOVE (p is empty) and for SPL (which uses SPL_SYS_STDIO_DEREGISTER) it means SPL_DM_DEVICE_REMOVE (p is SPL_). This is somewhat similar in style to the special-case 'depends on m' in Kconfig.
To make this work, we tell Kconfig that SPL is a phase with 'def_phase':
config SPL def_phase depends on SUPPORT_SPL prompt "Enable SPL" help If you want to build SPL as well as the normal image, say Y.It works just the same as a bool, but kconfig also uses it to automatically add new Kconfigs for each phase. In the above example it creates both SYS_STDIO_DEREGISTER and SPL_SYS_STDIO_DEREGISTER. The option name has the text '(SPL) ' shown before the SPL option.
This can easily handle Kconfigs with similar dependencies and even different ones. If the Kconfig options are not actually very similar we can still create two separate copies instead, as now.
This allows us to substantially reduce the size and duplication in the Kconfig defintions. It also reduces the pain of adding another phase to U-Boot.
Note: This change needs to be done in Linux, which owns Kconfig upstream.
2.Rename SPL_TPL_
This Makefile variable is used to reduce the number of duplicate rules in makefiles:
obj-$(CONFIG_$(SPL_TPL_)FIT_SIGNATURE) += fdt_region.oThe SPL_TPL_ expands to empty for U-Boot and either SPL_ or TPL_ for the other phases.
This is confusing though, since CONFIG_SPL_BUILD it set even for TPL builds, so for example. with:
obj-$(CONFIG_$(SPL_)FIT_SIGNATURE) += fdt_region.othe file is built for both SPL and TPL.
To help with this, We can rename SPL_TPL to PHASE_:
obj-$(CONFIG_$(PHASE_)FIT_SIGNATURE) += fdt_region.oor perhaps P_ which is more readable:
obj-$(CONFIG_$(P_)FIT_SIGNATURE) += fdt_region.o
- Rename CONFIG_IS_ENABLED()
This macro is used to determine whether an option is enabled in the current build phase:
if (CONFIG_IS_ENABLED(FIT_SIGNATURE)) { printf("## Checking hash(es) for Image %s ... ", fit_get_name(fit, node, NULL));
It is quite long-winded and people sometimes add CONFIG_ to the option inside the brakets by mistake. It is also a bit confusing that IS_ENABLED() and CONFIG_IS_ENABLED() mean completely different things.
Instead we can rename it to CONFIG:
if (CONFIG(FIT_SIGNATURE)) { printf("## Checking hash(es) for Image %s ... ", fit_get_name(fit, node, NULL));
This is shorter and looks more like CONFIG_FIT_SIGATURE so people should find it easier to understand. Being shorter is a big help when converting from use #if to if(), since an indentation is always enabled. This change makes the CONFIG() check no longer than IS_ENABLED().
It also makes CONFIG(OPTION) not much longer than CONFIG_OPTION, which makes things much more convenient, since ideally if the toolchain permitted it, we would just use CONFIG_OPTION in the code. This is not possible at present since the option may not be defined, so can cause a compiler error.
Over time, perhaps the existing IS_ENABLED() will phase out, since in many cases SPL will have its own options. We already see that CONFIG_IS_ENABLED is more popular / useful:
$ git grep -w IS_ENABLED |wc -l 902 $ git grep -w CONFIG_IS_ENABLED |wc -l 2282
- 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,) ... };It still isn't wonderfully readable but it seems like an improvement. The IF_CONFIG() macros could be implemented easily with the current CONFIG_IS_EANBLED() macro.
- IF_CONFIG_INT() or similar
See here: https://lists.denx.de/pipermail/u-boot/2020-May/412950.html
- 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_idapl_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.
Comments welcome!
I think what this is really showing is that Yamada-san was right. All
One thread where this was discussed was here I think:
https://yhbt.net/lore/all/20140624192425.9368.AA925319@jp.panasonic.com/T/
I cannot find all the arguments for either side now. Do you have a pointer to them?
I don't off-hand. I'm pretty sure it's come up more than once tho.
I certainly don't remember there being a strong argument either way at the time. Perhaps Masahiro can chime in.
the games we need to do so that "make fooboard_config all" results in building the N stages needed was the wrong track. Taking khadas-edge-v-rk3399 as an example, if we had instead of khadas-edge-v-rk3399_defconfig but khadas-edge-v-rk3399_tpl_defconfig khadas-edge-v-rk3399_spl_defconfig and khadas-edge-v-rk3399_config, each of which could set CONFIG_TPL, CONFIG_SPL or neither. Then yes, to build u-boot-rockchip.bin you would need to pass in the separately build TPL and SPL stages. But, that's true of so so many other platforms. To pick another example, imx8mm_evk doesn't function without other binaries. If in theory to build khadas-edge-v-rk3399 you had to do something like: $ export CROSS_COMPILE=aarch64-linux- $ make O=obj/tpl khadas-edge-v-rk3399_tpl_config all $ make O=obj/spl khadas-edge-v-rk3399_spl_config all $ make O=obj/khadas-edge-v-rk3399 TPL=obj/tpl/u-boot.bin \ SPL=obj/spl/u-boot.bin khadas-edge-v-rk3399_config all
We also need to think about the tools which are presumably built separately.
I'd like to see a way to build less host tools by default, but I fear the problem of distro folks if we go that way as it's more of a world builder / CI person problem that we build so many tools every time.
We might have to build in the opposite order, because SPL needs to grep the full devicetree, although I suppose we could just recompile it.
If we use dtb files from one stage in another stage directly (rather than being an included / packaged file) that seems like a potential problem.
Yes, also the arrangement of the build doesn't relate so closely to the Kconfig side of things. See below.
But it also meant that we didn't need to duplicate so so many Kconfig options and most of our obj rules would just be: obj-$(CONFIG_FOO) += foo.o
would be a win.
That definitely looks nice.
But how much of a win is this?
I think a good sized one. Especially since it will let us remove some customization in the build code.
I understand how it could work and I think we did talk about this at the time. But there are problems too that I'd like to review if we can find them. Some I can think of:
- maintaining three or more separate defconfig files for each board
Somewhat, yes. The SPL/TPL configs should be small. With some not overly judicious use of imply's in board/.../Kconfig it shouldn't be very much. I think it might emphasize that we do need a good way / place to set defaults for SoCs and boards.
- not sure how to handle dependencies between phases (e.g.
SPL_BLOBLIST has ''default y if BLOBLIST', or one phase expecting an image to be in there)
My first, but perhaps bad idea would be that we have say TARGET_AM335X_EVM in arch/arm/mach-omap2/am33xx/Kconfig still and a new TARGET_AM335X_EVM_OPTS in board/ti/am335x/Kconfig that would select/imply things that need to be enabled in all stages.
- running 'make menuconfig' updates one phase but not the others,
making things harder to understand
I'm not sure this is a problem so much. TPL/SPL shouldn't have much configuring to them, and even less re-configuring.
- splitting up of the build as you note above, making it harder for
people to understand
This I think is debatable. That we build and configure things the way we do isn't always obvious. More and better documentation, either way, would be good.
Having thought a bit more, perhaps we have the wrong attitude to Kconfig. The CONFIG() macro I am talking about works by building an xxx or SPL_xxx config. If we have separate autoconf.h files for each phase (autoconf_spl.h etc.) then we don't actually need this. We just need to include the correct file. Any SPL_xxx config can be written as xxx. Similarly the Makefile rules can drop the $(P) I was proposing.
We can, in fact, generate separate autoconf.h files for each phase today, with no other changes. Unless I am missing something...?
Interesting to see this comment:
http://u-boot.10912.n7.nabble.com/PATCH-v8-0-13-Kconfig-for-U-Boot-tt185309....
"It would take really long term (one year? two year? I don't know) to migrate from board headers to Kconfig.
So, two different infractructure must coexist in the interim."
That was 2014! I think we need a way to remove old CONFIGs and let board maintainers add them back in Kconfig if needed.
I need to take another pass at converting a bunch of symbols, to see where we're at. Probably the biggest chunk of progress next would be to start converting CONFIG_SYS_xxx to SYS_xxx and moving defines out of config.h and in to something else. I'm taking a peek at some of the remaining PCI ones now.
How about we set a deadline for this? It has gone on for too long and we just need to drop these CONFIGs. It's probably a higher priority than a Kconfig change.
I was expecting that the config.h files would go away and we would use Kconfig (or DT) for everything. What sort of things don't fit into that model?
Regards, Simon
On Wed, Aug 11, 2021 at 06:56:31AM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 10 Aug 2021 at 13:38, Tom Rini trini@konsulko.com wrote:
[snip]
I need to take another pass at converting a bunch of symbols, to see where we're at. Probably the biggest chunk of progress next would be to start converting CONFIG_SYS_xxx to SYS_xxx and moving defines out of config.h and in to something else. I'm taking a peek at some of the remaining PCI ones now.
How about we set a deadline for this? It has gone on for too long and we just need to drop these CONFIGs. It's probably a higher priority than a Kconfig change.
I was expecting that the config.h files would go away and we would use Kconfig (or DT) for everything. What sort of things don't fit into that model?
Environment is the hard one to move out from config.h and in to, well, I don't know what. I think there's also a handful of symbols like CONFIG_SPL_MAX_SIZE that are a little tricky to convert directly (they do math based on other symbols) rather than just as evaluate-and-set. Right now, a little more than half of the unmigrated symbols are CONFIG_SYS_xxx things and those likely should become SYS_xxx things. Of the ones that don't just go away.
Hi Tom,
On Wed, 11 Aug 2021 at 07:47, Tom Rini trini@konsulko.com wrote:
On Wed, Aug 11, 2021 at 06:56:31AM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 10 Aug 2021 at 13:38, Tom Rini trini@konsulko.com wrote:
[snip]
I need to take another pass at converting a bunch of symbols, to see where we're at. Probably the biggest chunk of progress next would be to start converting CONFIG_SYS_xxx to SYS_xxx and moving defines out of config.h and in to something else. I'm taking a peek at some of the remaining PCI ones now.
How about we set a deadline for this? It has gone on for too long and we just need to drop these CONFIGs. It's probably a higher priority than a Kconfig change.
I was expecting that the config.h files would go away and we would use Kconfig (or DT) for everything. What sort of things don't fit into that model?
Environment is the hard one to move out from config.h and in to, well, I
Well you know my views on that :-)
http://patchwork.ozlabs.org/project/uboot/patch/1382763695-2849-4-git-send-e...
I still think it makes more sense than #defines and I can resurrect that series if you like.
don't know what. I think there's also a handful of symbols like CONFIG_SPL_MAX_SIZE that are a little tricky to convert directly (they do math based on other symbols) rather than just as evaluate-and-set.
We can either evaluate them and put the answer in as the defconfig value...or perhaps ask Masahiro to support evaluation in kconfig?!
Right now, a little more than half of the unmigrated symbols are CONFIG_SYS_xxx things and those likely should become SYS_xxx things. Of the ones that don't just go away.
Do you mean things like this?
arch/m68k/include/asm/immap.h:#define CONFIG_SYS_PCI_BAR0 (0x40000000)
Assuming this doesn't move to devicetree, it should be in its own asm/ or asm/arch header file I think, not in the config.h file at all.
FSL layerscape should move CONFIG_SYS_PCIE3_PHYS_SIZE et al to devcetree.
Some of the DM migrations will help - e.g. for I2C. NAND seems to have a lot - who is the NAND maintainer?
But really what I am asking is, can we set a deadline where all config.h files will be dropped? It has been 7 years...
Regards, SImon
On Wed, Aug 11, 2021 at 08:03:00AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 11 Aug 2021 at 07:47, Tom Rini trini@konsulko.com wrote:
On Wed, Aug 11, 2021 at 06:56:31AM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 10 Aug 2021 at 13:38, Tom Rini trini@konsulko.com wrote:
[snip]
I need to take another pass at converting a bunch of symbols, to see where we're at. Probably the biggest chunk of progress next would be to start converting CONFIG_SYS_xxx to SYS_xxx and moving defines out of config.h and in to something else. I'm taking a peek at some of the remaining PCI ones now.
How about we set a deadline for this? It has gone on for too long and we just need to drop these CONFIGs. It's probably a higher priority than a Kconfig change.
I was expecting that the config.h files would go away and we would use Kconfig (or DT) for everything. What sort of things don't fit into that model?
Environment is the hard one to move out from config.h and in to, well, I
Well you know my views on that :-)
http://patchwork.ozlabs.org/project/uboot/patch/1382763695-2849-4-git-send-e...
I still think it makes more sense than #defines and I can resurrect that series if you like.
That might work, yeah. I just also want to focus on less things in progress at once. That too I think has been part of why everything is taking so long.
don't know what. I think there's also a handful of symbols like CONFIG_SPL_MAX_SIZE that are a little tricky to convert directly (they do math based on other symbols) rather than just as evaluate-and-set.
We can either evaluate them and put the answer in as the defconfig value...or perhaps ask Masahiro to support evaluation in kconfig?!
I do forget what kind of operations are allowed in Kconfig at this point, it might be possible now, yes. And if not, something worth trying.
Right now, a little more than half of the unmigrated symbols are CONFIG_SYS_xxx things and those likely should become SYS_xxx things. Of the ones that don't just go away.
Do you mean things like this?
arch/m68k/include/asm/immap.h:#define CONFIG_SYS_PCI_BAR0 (0x40000000)
Assuming this doesn't move to devicetree, it should be in its own asm/ or asm/arch header file I think, not in the config.h file at all.
FSL layerscape should move CONFIG_SYS_PCIE3_PHYS_SIZE et al to devcetree.
I started and set aside *PCI* since a bunch of that goes away once UCP1020 gets updated. But yes, there are lots of CONFIG_SYS_xxx things that live inside and outside of config.h and step one is likely a simple regex. They aren't really configurable. We can try and figure out what "get this from DT" approach makes sense after.
Some of the DM migrations will help - e.g. for I2C. NAND seems to have a lot - who is the NAND maintainer?
There's not currently a NAND maintainer.
But really what I am asking is, can we set a deadline where all config.h files will be dropped? It has been 7 years...
It's going to come down once again to figuring out what to do about older platforms. Since I picked on khadas platforms earlier, here's where meson64.h looks really good. All of the platforms use include/configs/meson64.h and that's very little outside of environment stuff. To go back to another part of the thread, it also shows how hard environment stuff is.
It also reminded me that buildman never got kconfiglib support directly and we still have genboardcfg.py to spit out boards.cfg to be parsed by buildman.
Hi Tom,
On Wed, 11 Aug 2021 at 08:17, Tom Rini trini@konsulko.com wrote:
On Wed, Aug 11, 2021 at 08:03:00AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 11 Aug 2021 at 07:47, Tom Rini trini@konsulko.com wrote:
On Wed, Aug 11, 2021 at 06:56:31AM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 10 Aug 2021 at 13:38, Tom Rini trini@konsulko.com wrote:
[snip]
I need to take another pass at converting a bunch of symbols, to see where we're at. Probably the biggest chunk of progress next would be to start converting CONFIG_SYS_xxx to SYS_xxx and moving defines out of config.h and in to something else. I'm taking a peek at some of the remaining PCI ones now.
How about we set a deadline for this? It has gone on for too long and we just need to drop these CONFIGs. It's probably a higher priority than a Kconfig change.
I was expecting that the config.h files would go away and we would use Kconfig (or DT) for everything. What sort of things don't fit into that model?
Environment is the hard one to move out from config.h and in to, well, I
Well you know my views on that :-)
http://patchwork.ozlabs.org/project/uboot/patch/1382763695-2849-4-git-send-e...
I still think it makes more sense than #defines and I can resurrect that series if you like.
That might work, yeah. I just also want to focus on less things in progress at once. That too I think has been part of why everything is taking so long.
OK I'll take a look. Other things in progress I can think of are:
- drop common.h (could be done in one series if we just go for it) - set up issue tracking so we can keep an eye on these things
don't know what. I think there's also a handful of symbols like CONFIG_SPL_MAX_SIZE that are a little tricky to convert directly (they do math based on other symbols) rather than just as evaluate-and-set.
We can either evaluate them and put the answer in as the defconfig value...or perhaps ask Masahiro to support evaluation in kconfig?!
I do forget what kind of operations are allowed in Kconfig at this point, it might be possible now, yes. And if not, something worth trying.
OK
Right now, a little more than half of the unmigrated symbols are CONFIG_SYS_xxx things and those likely should become SYS_xxx things. Of the ones that don't just go away.
Do you mean things like this?
arch/m68k/include/asm/immap.h:#define CONFIG_SYS_PCI_BAR0 (0x40000000)
Assuming this doesn't move to devicetree, it should be in its own asm/ or asm/arch header file I think, not in the config.h file at all.
FSL layerscape should move CONFIG_SYS_PCIE3_PHYS_SIZE et al to devcetree.
I started and set aside *PCI* since a bunch of that goes away once UCP1020 gets updated. But yes, there are lots of CONFIG_SYS_xxx things that live inside and outside of config.h and step one is likely a simple regex. They aren't really configurable. We can try and figure out what "get this from DT" approach makes sense after.
Yes agree we can't wait for DT move.
Some of the DM migrations will help - e.g. for I2C. NAND seems to have a lot - who is the NAND maintainer?
There's not currently a NAND maintainer.
But really what I am asking is, can we set a deadline where all config.h files will be dropped? It has been 7 years...
It's going to come down once again to figuring out what to do about older platforms. Since I picked on khadas platforms earlier, here's where meson64.h looks really good. All of the platforms use include/configs/meson64.h and that's very little outside of environment stuff. To go back to another part of the thread, it also shows how hard environment stuff is.
Well let's see.
It also reminded me that buildman never got kconfiglib support directly and we still have genboardcfg.py to spit out boards.cfg to be parsed by buildman.
Ah yes, so is buildman the only reason we have that file? I do like being able to grep it for stuff, but I suppose buildman could support that. Is there no other user?
Regards, Simon
On Wed, Aug 11, 2021 at 08:26:38AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 11 Aug 2021 at 08:17, Tom Rini trini@konsulko.com wrote:
On Wed, Aug 11, 2021 at 08:03:00AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 11 Aug 2021 at 07:47, Tom Rini trini@konsulko.com wrote:
On Wed, Aug 11, 2021 at 06:56:31AM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 10 Aug 2021 at 13:38, Tom Rini trini@konsulko.com wrote:
[snip]
I need to take another pass at converting a bunch of symbols, to see where we're at. Probably the biggest chunk of progress next would be to start converting CONFIG_SYS_xxx to SYS_xxx and moving defines out of config.h and in to something else. I'm taking a peek at some of the remaining PCI ones now.
How about we set a deadline for this? It has gone on for too long and we just need to drop these CONFIGs. It's probably a higher priority than a Kconfig change.
I was expecting that the config.h files would go away and we would use Kconfig (or DT) for everything. What sort of things don't fit into that model?
Environment is the hard one to move out from config.h and in to, well, I
Well you know my views on that :-)
http://patchwork.ozlabs.org/project/uboot/patch/1382763695-2849-4-git-send-e...
I still think it makes more sense than #defines and I can resurrect that series if you like.
That might work, yeah. I just also want to focus on less things in progress at once. That too I think has been part of why everything is taking so long.
OK I'll take a look. Other things in progress I can think of are:
- drop common.h (could be done in one series if we just go for it)
- set up issue tracking so we can keep an eye on these things
Well, all of the DM migrations that have deadlines, and the there's probably some that don't still. And the high level size concern about using DM on platforms where dynamic selection/detection isn't a functional win.
don't know what. I think there's also a handful of symbols like CONFIG_SPL_MAX_SIZE that are a little tricky to convert directly (they do math based on other symbols) rather than just as evaluate-and-set.
We can either evaluate them and put the answer in as the defconfig value...or perhaps ask Masahiro to support evaluation in kconfig?!
I do forget what kind of operations are allowed in Kconfig at this point, it might be possible now, yes. And if not, something worth trying.
OK
Right now, a little more than half of the unmigrated symbols are CONFIG_SYS_xxx things and those likely should become SYS_xxx things. Of the ones that don't just go away.
Do you mean things like this?
arch/m68k/include/asm/immap.h:#define CONFIG_SYS_PCI_BAR0 (0x40000000)
Assuming this doesn't move to devicetree, it should be in its own asm/ or asm/arch header file I think, not in the config.h file at all.
FSL layerscape should move CONFIG_SYS_PCIE3_PHYS_SIZE et al to devcetree.
I started and set aside *PCI* since a bunch of that goes away once UCP1020 gets updated. But yes, there are lots of CONFIG_SYS_xxx things that live inside and outside of config.h and step one is likely a simple regex. They aren't really configurable. We can try and figure out what "get this from DT" approach makes sense after.
Yes agree we can't wait for DT move.
Some of the DM migrations will help - e.g. for I2C. NAND seems to have a lot - who is the NAND maintainer?
There's not currently a NAND maintainer.
But really what I am asking is, can we set a deadline where all config.h files will be dropped? It has been 7 years...
It's going to come down once again to figuring out what to do about older platforms. Since I picked on khadas platforms earlier, here's where meson64.h looks really good. All of the platforms use include/configs/meson64.h and that's very little outside of environment stuff. To go back to another part of the thread, it also shows how hard environment stuff is.
Well let's see.
It also reminded me that buildman never got kconfiglib support directly and we still have genboardcfg.py to spit out boards.cfg to be parsed by buildman.
Ah yes, so is buildman the only reason we have that file? I do like being able to grep it for stuff, but I suppose buildman could support that. Is there no other user?
Ah, yes, so there's two things it does. One is buildman. Two is "spit an error out if a defconfig lacks a MAINTAINER entry". The latter is important but could be its own tool, and then of course looking at MAINTAINER files is how to see what config header a given defconfig uses.
Hi Tom,
On Wed, 11 Aug 2021 at 09:40, Tom Rini trini@konsulko.com wrote:
On Wed, Aug 11, 2021 at 08:26:38AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 11 Aug 2021 at 08:17, Tom Rini trini@konsulko.com wrote:
On Wed, Aug 11, 2021 at 08:03:00AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 11 Aug 2021 at 07:47, Tom Rini trini@konsulko.com wrote:
On Wed, Aug 11, 2021 at 06:56:31AM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 10 Aug 2021 at 13:38, Tom Rini trini@konsulko.com wrote:
[snip]
> I need to take another pass at converting a bunch of symbols, to see > where we're at. Probably the biggest chunk of progress next would be to > start converting CONFIG_SYS_xxx to SYS_xxx and moving defines out of > config.h and in to something else. I'm taking a peek at some of the > remaining PCI ones now.
How about we set a deadline for this? It has gone on for too long and we just need to drop these CONFIGs. It's probably a higher priority than a Kconfig change.
I was expecting that the config.h files would go away and we would use Kconfig (or DT) for everything. What sort of things don't fit into that model?
Environment is the hard one to move out from config.h and in to, well, I
Well you know my views on that :-)
http://patchwork.ozlabs.org/project/uboot/patch/1382763695-2849-4-git-send-e...
I still think it makes more sense than #defines and I can resurrect that series if you like.
That might work, yeah. I just also want to focus on less things in progress at once. That too I think has been part of why everything is taking so long.
OK I'll take a look. Other things in progress I can think of are:
- drop common.h (could be done in one series if we just go for it)
- set up issue tracking so we can keep an eye on these things
Well, all of the DM migrations that have deadlines, and the there's probably some that don't still. And the high level size concern about
Yes, I'm going to send a patch for serial and GPIO.
using DM on platforms where dynamic selection/detection isn't a functional win.
Assuming it is about 20KB added (plus devicetree), enabling OF_CONTROL seems worth it to me. For SPL I would like to see more adoption of your of-platdata idea.
don't know what. I think there's also a handful of symbols like CONFIG_SPL_MAX_SIZE that are a little tricky to convert directly (they do math based on other symbols) rather than just as evaluate-and-set.
We can either evaluate them and put the answer in as the defconfig value...or perhaps ask Masahiro to support evaluation in kconfig?!
I do forget what kind of operations are allowed in Kconfig at this point, it might be possible now, yes. And if not, something worth trying.
OK
Right now, a little more than half of the unmigrated symbols are CONFIG_SYS_xxx things and those likely should become SYS_xxx things. Of the ones that don't just go away.
Do you mean things like this?
arch/m68k/include/asm/immap.h:#define CONFIG_SYS_PCI_BAR0 (0x40000000)
Assuming this doesn't move to devicetree, it should be in its own asm/ or asm/arch header file I think, not in the config.h file at all.
FSL layerscape should move CONFIG_SYS_PCIE3_PHYS_SIZE et al to devcetree.
I started and set aside *PCI* since a bunch of that goes away once UCP1020 gets updated. But yes, there are lots of CONFIG_SYS_xxx things that live inside and outside of config.h and step one is likely a simple regex. They aren't really configurable. We can try and figure out what "get this from DT" approach makes sense after.
Yes agree we can't wait for DT move.
Some of the DM migrations will help - e.g. for I2C. NAND seems to have a lot - who is the NAND maintainer?
There's not currently a NAND maintainer.
But really what I am asking is, can we set a deadline where all config.h files will be dropped? It has been 7 years...
It's going to come down once again to figuring out what to do about older platforms. Since I picked on khadas platforms earlier, here's where meson64.h looks really good. All of the platforms use include/configs/meson64.h and that's very little outside of environment stuff. To go back to another part of the thread, it also shows how hard environment stuff is.
Well let's see.
It also reminded me that buildman never got kconfiglib support directly and we still have genboardcfg.py to spit out boards.cfg to be parsed by buildman.
Ah yes, so is buildman the only reason we have that file? I do like being able to grep it for stuff, but I suppose buildman could support that. Is there no other user?
Ah, yes, so there's two things it does. One is buildman. Two is "spit an error out if a defconfig lacks a MAINTAINER entry". The latter is important but could be its own tool, and then of course looking at MAINTAINER files is how to see what config header a given defconfig uses.
OK I see. The defconfig file seems to be in the Kconfig now...so I suppose I don't fully understand that.
Regards, Simon
On Wed, Aug 11, 2021 at 12:28:13PM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 11 Aug 2021 at 09:40, Tom Rini trini@konsulko.com wrote:
On Wed, Aug 11, 2021 at 08:26:38AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 11 Aug 2021 at 08:17, Tom Rini trini@konsulko.com wrote:
On Wed, Aug 11, 2021 at 08:03:00AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 11 Aug 2021 at 07:47, Tom Rini trini@konsulko.com wrote:
On Wed, Aug 11, 2021 at 06:56:31AM -0600, Simon Glass wrote: > Hi Tom, > > On Tue, 10 Aug 2021 at 13:38, Tom Rini trini@konsulko.com wrote: [snip] > > I need to take another pass at converting a bunch of symbols, to see > > where we're at. Probably the biggest chunk of progress next would be to > > start converting CONFIG_SYS_xxx to SYS_xxx and moving defines out of > > config.h and in to something else. I'm taking a peek at some of the > > remaining PCI ones now. > > How about we set a deadline for this? It has gone on for too long and > we just need to drop these CONFIGs. It's probably a higher priority > than a Kconfig change. > > I was expecting that the config.h files would go away and we would use > Kconfig (or DT) for everything. What sort of things don't fit into > that model?
Environment is the hard one to move out from config.h and in to, well, I
Well you know my views on that :-)
http://patchwork.ozlabs.org/project/uboot/patch/1382763695-2849-4-git-send-e...
I still think it makes more sense than #defines and I can resurrect that series if you like.
That might work, yeah. I just also want to focus on less things in progress at once. That too I think has been part of why everything is taking so long.
OK I'll take a look. Other things in progress I can think of are:
- drop common.h (could be done in one series if we just go for it)
- set up issue tracking so we can keep an eye on these things
Well, all of the DM migrations that have deadlines, and the there's probably some that don't still. And the high level size concern about
Yes, I'm going to send a patch for serial and GPIO.
OK. It would be good to know then how many platforms are unmigrated. And I need to cycle back to the list of non-DM migrated boards thread.
using DM on platforms where dynamic selection/detection isn't a functional win.
Assuming it is about 20KB added (plus devicetree), enabling OF_CONTROL seems worth it to me. For SPL I would like to see more adoption of your of-platdata idea.
20KiB is a lot. It's not a huge deal when we start talking about some sort of dynamic detection being useful. Or "we were passed a dtb by prior stage, use and pass along". But there's still new designs where neither of those are the case. And tons of legacy designs we still have users of.
But it's also one of those "not right this minute" topics.
[snip]
Ah, yes, so there's two things it does. One is buildman. Two is "spit an error out if a defconfig lacks a MAINTAINER entry". The latter is important but could be its own tool, and then of course looking at MAINTAINER files is how to see what config header a given defconfig uses.
OK I see. The defconfig file seems to be in the Kconfig now...so I suppose I don't fully understand that.
Er, what?
On 8/11/21 10:17 AM, Tom Rini wrote:
On Wed, Aug 11, 2021 at 08:03:00AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 11 Aug 2021 at 07:47, Tom Rini trini@konsulko.com wrote:
On Wed, Aug 11, 2021 at 06:56:31AM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 10 Aug 2021 at 13:38, Tom Rini trini@konsulko.com wrote:
[snip]
I need to take another pass at converting a bunch of symbols, to see where we're at. Probably the biggest chunk of progress next would be to start converting CONFIG_SYS_xxx to SYS_xxx and moving defines out of config.h and in to something else. I'm taking a peek at some of the remaining PCI ones now.
How about we set a deadline for this? It has gone on for too long and we just need to drop these CONFIGs. It's probably a higher priority than a Kconfig change.
I was expecting that the config.h files would go away and we would use Kconfig (or DT) for everything. What sort of things don't fit into that model?
Environment is the hard one to move out from config.h and in to, well, I
Well you know my views on that :-)
http://patchwork.ozlabs.org/project/uboot/patch/1382763695-2849-4-git-send-e...
I still think it makes more sense than #defines and I can resurrect that series if you like.
This would be nice. I was thinking about doing something like this for LIL, to make writing tests easier.
That might work, yeah. I just also want to focus on less things in progress at once. That too I think has been part of why everything is taking so long.
don't know what. I think there's also a handful of symbols like CONFIG_SPL_MAX_SIZE that are a little tricky to convert directly (they do math based on other symbols) rather than just as evaluate-and-set.
We can either evaluate them and put the answer in as the defconfig value...or perhaps ask Masahiro to support evaluation in kconfig?!
I do forget what kind of operations are allowed in Kconfig at this point, it might be possible now, yes. And if not, something worth trying.
At the moment there are conditionals for dependencies and defaults [1]. It doesn't seem like too much of a stretch to allow for arbitrary arithmetic. In particular, we could really get 80% of the functionality with just + (and perhaps & or |).
https://www.kernel.org/doc/html/latest/kbuild/kconfig-language.html#menu-dep...
--Sean
On Wed, Aug 11, 2021 at 04:14:51PM -0400, Sean Anderson wrote:
On 8/11/21 10:17 AM, Tom Rini wrote:
On Wed, Aug 11, 2021 at 08:03:00AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 11 Aug 2021 at 07:47, Tom Rini trini@konsulko.com wrote:
On Wed, Aug 11, 2021 at 06:56:31AM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 10 Aug 2021 at 13:38, Tom Rini trini@konsulko.com wrote:
[snip]
I need to take another pass at converting a bunch of symbols, to see where we're at. Probably the biggest chunk of progress next would be to start converting CONFIG_SYS_xxx to SYS_xxx and moving defines out of config.h and in to something else. I'm taking a peek at some of the remaining PCI ones now.
How about we set a deadline for this? It has gone on for too long and we just need to drop these CONFIGs. It's probably a higher priority than a Kconfig change.
I was expecting that the config.h files would go away and we would use Kconfig (or DT) for everything. What sort of things don't fit into that model?
Environment is the hard one to move out from config.h and in to, well, I
Well you know my views on that :-)
http://patchwork.ozlabs.org/project/uboot/patch/1382763695-2849-4-git-send-e...
I still think it makes more sense than #defines and I can resurrect that series if you like.
This would be nice. I was thinking about doing something like this for LIL, to make writing tests easier.
I was thinking of mentioning LIL, even, here. Converting (to start with, just a board or two) that implement distro boot from the current env scheme to another env scheme is going to go quite a ways to showing if the proposed idea is going to be workable enough or not.
That might work, yeah. I just also want to focus on less things in progress at once. That too I think has been part of why everything is taking so long.
don't know what. I think there's also a handful of symbols like CONFIG_SPL_MAX_SIZE that are a little tricky to convert directly (they do math based on other symbols) rather than just as evaluate-and-set.
We can either evaluate them and put the answer in as the defconfig value...or perhaps ask Masahiro to support evaluation in kconfig?!
I do forget what kind of operations are allowed in Kconfig at this point, it might be possible now, yes. And if not, something worth trying.
At the moment there are conditionals for dependencies and defaults [1]. It doesn't seem like too much of a stretch to allow for arbitrary arithmetic. In particular, we could really get 80% of the functionality with just + (and perhaps & or |).
https://www.kernel.org/doc/html/latest/kbuild/kconfig-language.html#menu-dep...
Yeah, the addition of +- I think really would cover everything that's not "this doesn't belong in CONFIG space really" I think. The only maybe-hangups are that we say for example on TI platforms: #define CONFIG_SPL_MAX_SIZE (SRAM_SCRATCH_SPACE_ADDR - CONFIG_SPL_TEXT_BASE)
where SRAM_SCRATCH_SPACE_ADDR isn't a CONFIG symbol, on purpose. But in turn, maybe that means we just want to re-think the symbol and have a choice instead. Some platforms end up being "automatic" size like the above. Other platforms like imx* might be "fixed" size and then define 0x10000 or whatever.
It's all the kind of details that come up when picking a symbol and digging in.
On Wed, Aug 11, 2021 at 06:56:31AM -0600, Simon Glass wrote: [snip]
Having thought a bit more, perhaps we have the wrong attitude to Kconfig. The CONFIG() macro I am talking about works by building an xxx or SPL_xxx config. If we have separate autoconf.h files for each phase (autoconf_spl.h etc.) then we don't actually need this. We just need to include the correct file. Any SPL_xxx config can be written as xxx. Similarly the Makefile rules can drop the $(P) I was proposing.
We can, in fact, generate separate autoconf.h files for each phase today, with no other changes. Unless I am missing something...?
If we can spit out {spl_,tpl_,}autoconf.h files that might help a bit. But would it help with the recent case of SPL has SATA+AHCI+!PCI while full U-Boot has SATA+AHCI+!PCI AND SATA+AHCI+PCI ? Today we can't support the SPL case without adding the handful of SPL_xxx symbols so that we can say we have SATA+AHCI without PCI.
Hi Tom,
On Wed, 11 Aug 2021 at 08:02, Tom Rini trini@konsulko.com wrote:
On Wed, Aug 11, 2021 at 06:56:31AM -0600, Simon Glass wrote: [snip]
Having thought a bit more, perhaps we have the wrong attitude to Kconfig. The CONFIG() macro I am talking about works by building an xxx or SPL_xxx config. If we have separate autoconf.h files for each phase (autoconf_spl.h etc.) then we don't actually need this. We just need to include the correct file. Any SPL_xxx config can be written as xxx. Similarly the Makefile rules can drop the $(P) I was proposing.
We can, in fact, generate separate autoconf.h files for each phase today, with no other changes. Unless I am missing something...?
If we can spit out {spl_,tpl_,}autoconf.h files that might help a bit. But would it help with the recent case of SPL has SATA+AHCI+!PCI while full U-Boot has SATA+AHCI+!PCI AND SATA+AHCI+PCI ? Today we can't support the SPL case without adding the handful of SPL_xxx symbols so that we can say we have SATA+AHCI without PCI.
My thought is that:
- where there is no SPL_xxx symbol, it we would have CONFIG_xxx=y in all autoconf.h files - where there is an SPL_xxx symbol, it we would only have it in spl_autoconf.h if the SPL_xxx symbol is enabled
So it does not reduce the power/flexibility of what we have to cover all cases. It is just a phase-specific way of presenting the configs to the build, so we can do:
obj-$(CONFIG_FOO) += foo.o
as well as
if (CONFIG(FOO))
I'm still thinking about Kconfig. To me it seems that separating the phases so completely is giving up quite a bit. There is no-longer a unified build, so dependencies between phases may become a problem. I think in fact our problem is the use of SPL_ and TPL_ prefixes on Kconfigs, which you have highlighted. Perhaps we just shouldn't do that. It would be nice if kconfig could support multiple interrelated build phases and output a separate autoconf.h for each one.
Regards, Simon
On Wed, Aug 11, 2021 at 08:11:41AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 11 Aug 2021 at 08:02, Tom Rini trini@konsulko.com wrote:
On Wed, Aug 11, 2021 at 06:56:31AM -0600, Simon Glass wrote: [snip]
Having thought a bit more, perhaps we have the wrong attitude to Kconfig. The CONFIG() macro I am talking about works by building an xxx or SPL_xxx config. If we have separate autoconf.h files for each phase (autoconf_spl.h etc.) then we don't actually need this. We just need to include the correct file. Any SPL_xxx config can be written as xxx. Similarly the Makefile rules can drop the $(P) I was proposing.
We can, in fact, generate separate autoconf.h files for each phase today, with no other changes. Unless I am missing something...?
If we can spit out {spl_,tpl_,}autoconf.h files that might help a bit. But would it help with the recent case of SPL has SATA+AHCI+!PCI while full U-Boot has SATA+AHCI+!PCI AND SATA+AHCI+PCI ? Today we can't support the SPL case without adding the handful of SPL_xxx symbols so that we can say we have SATA+AHCI without PCI.
My thought is that:
- where there is no SPL_xxx symbol, it we would have CONFIG_xxx=y in
all autoconf.h files
- where there is an SPL_xxx symbol, it we would only have it in
spl_autoconf.h if the SPL_xxx symbol is enabled
So it does not reduce the power/flexibility of what we have to cover all cases. It is just a phase-specific way of presenting the configs to the build, so we can do:
obj-$(CONFIG_FOO) += foo.o
as well as
if (CONFIG(FOO))
I'm still thinking about Kconfig. To me it seems that separating the phases so completely is giving up quite a bit. There is no-longer a unified build, so dependencies between phases may become a problem. I think in fact our problem is the use of SPL_ and TPL_ prefixes on Kconfigs, which you have highlighted. Perhaps we just shouldn't do that. It would be nice if kconfig could support multiple interrelated build phases and output a separate autoconf.h for each one.
What are the dependencies we have between phases? You've mentioned bloblist, but to me that's like BOARD_INIT and MISC_INIT_R and all of the other things you need to have select'd on a platform because they're non-optional.
And I'm really not seeing now how we would support the example I gave as for them SPL with SATA+AHCI+PCI is not desired nor possible. I asked. The answer was no, don't want it. Or do you really just mean that if we had spl_autoconf.h the only thing that would change is that we would never test on CONFIG_SPL_xxx only CONFIG_xxx, but we would still need to Kconfig SPL_xxx?
Hi Tom,
On Wed, 11 Aug 2021 at 08:31, Tom Rini trini@konsulko.com wrote:
On Wed, Aug 11, 2021 at 08:11:41AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 11 Aug 2021 at 08:02, Tom Rini trini@konsulko.com wrote:
On Wed, Aug 11, 2021 at 06:56:31AM -0600, Simon Glass wrote: [snip]
Having thought a bit more, perhaps we have the wrong attitude to Kconfig. The CONFIG() macro I am talking about works by building an xxx or SPL_xxx config. If we have separate autoconf.h files for each phase (autoconf_spl.h etc.) then we don't actually need this. We just need to include the correct file. Any SPL_xxx config can be written as xxx. Similarly the Makefile rules can drop the $(P) I was proposing.
We can, in fact, generate separate autoconf.h files for each phase today, with no other changes. Unless I am missing something...?
If we can spit out {spl_,tpl_,}autoconf.h files that might help a bit. But would it help with the recent case of SPL has SATA+AHCI+!PCI while full U-Boot has SATA+AHCI+!PCI AND SATA+AHCI+PCI ? Today we can't support the SPL case without adding the handful of SPL_xxx symbols so that we can say we have SATA+AHCI without PCI.
My thought is that:
- where there is no SPL_xxx symbol, it we would have CONFIG_xxx=y in
all autoconf.h files
- where there is an SPL_xxx symbol, it we would only have it in
spl_autoconf.h if the SPL_xxx symbol is enabled
So it does not reduce the power/flexibility of what we have to cover all cases. It is just a phase-specific way of presenting the configs to the build, so we can do:
obj-$(CONFIG_FOO) += foo.o
as well as
if (CONFIG(FOO))
I'm still thinking about Kconfig. To me it seems that separating the phases so completely is giving up quite a bit. There is no-longer a unified build, so dependencies between phases may become a problem. I think in fact our problem is the use of SPL_ and TPL_ prefixes on Kconfigs, which you have highlighted. Perhaps we just shouldn't do that. It would be nice if kconfig could support multiple interrelated build phases and output a separate autoconf.h for each one.
What are the dependencies we have between phases? You've mentioned bloblist, but to me that's like BOARD_INIT and MISC_INIT_R and all of the other things you need to have select'd on a platform because they're non-optional.
Well if you enable BLOBLIST in U-Boot proper then it had better be enabled in SPL or it won't work. Same with HANDOFF. Other things on my list in this vein are console recording through the phases. Logging is best enabled globally, with different default levels for SPL. We also have a lot of things like LOCALVERSION. The main Makefile looks at CONFIG_SPL_FRAMEWORK to decide whether to expect u-boot.img or not, so we'd have to have another symbol like CONFIG_FRAMEWORK that people keep in sync (or just complete the *&^#$&^# migration :-)
We have quite a bit of:
config SPL_SYS_ICACHE_OFF depends on SPL default SYS_ICACHE_OFF
I think we are throwing away a lot by separating them at the configuration stage. I'm not saying it's a disaster but I am worried that it might not lead to a good place.
And I'm really not seeing now how we would support the example I gave as for them SPL with SATA+AHCI+PCI is not desired nor possible. I asked. The answer was no, don't want it. Or do you really just mean that if we had spl_autoconf.h the only thing that would change is that we would never test on CONFIG_SPL_xxx only CONFIG_xxx, but we would still need to Kconfig SPL_xxx?
Yes, that's what I am saying. We can make that change now (to clean up Makefile and add CONFIG()) with no change to Kconfig.
We can support the case you mention and yes we want it and need it, of course. Otherwise we are back to the SPL #undef horror.
Regards, Simon
On Wed, Aug 11, 2021 at 08:47:24AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 11 Aug 2021 at 08:31, Tom Rini trini@konsulko.com wrote:
On Wed, Aug 11, 2021 at 08:11:41AM -0600, Simon Glass wrote:
Hi Tom,
On Wed, 11 Aug 2021 at 08:02, Tom Rini trini@konsulko.com wrote:
On Wed, Aug 11, 2021 at 06:56:31AM -0600, Simon Glass wrote: [snip]
Having thought a bit more, perhaps we have the wrong attitude to Kconfig. The CONFIG() macro I am talking about works by building an xxx or SPL_xxx config. If we have separate autoconf.h files for each phase (autoconf_spl.h etc.) then we don't actually need this. We just need to include the correct file. Any SPL_xxx config can be written as xxx. Similarly the Makefile rules can drop the $(P) I was proposing.
We can, in fact, generate separate autoconf.h files for each phase today, with no other changes. Unless I am missing something...?
If we can spit out {spl_,tpl_,}autoconf.h files that might help a bit. But would it help with the recent case of SPL has SATA+AHCI+!PCI while full U-Boot has SATA+AHCI+!PCI AND SATA+AHCI+PCI ? Today we can't support the SPL case without adding the handful of SPL_xxx symbols so that we can say we have SATA+AHCI without PCI.
My thought is that:
- where there is no SPL_xxx symbol, it we would have CONFIG_xxx=y in
all autoconf.h files
- where there is an SPL_xxx symbol, it we would only have it in
spl_autoconf.h if the SPL_xxx symbol is enabled
So it does not reduce the power/flexibility of what we have to cover all cases. It is just a phase-specific way of presenting the configs to the build, so we can do:
obj-$(CONFIG_FOO) += foo.o
as well as
if (CONFIG(FOO))
I'm still thinking about Kconfig. To me it seems that separating the phases so completely is giving up quite a bit. There is no-longer a unified build, so dependencies between phases may become a problem. I think in fact our problem is the use of SPL_ and TPL_ prefixes on Kconfigs, which you have highlighted. Perhaps we just shouldn't do that. It would be nice if kconfig could support multiple interrelated build phases and output a separate autoconf.h for each one.
What are the dependencies we have between phases? You've mentioned bloblist, but to me that's like BOARD_INIT and MISC_INIT_R and all of the other things you need to have select'd on a platform because they're non-optional.
Well if you enable BLOBLIST in U-Boot proper then it had better be enabled in SPL or it won't work. Same with HANDOFF. Other things on my list in this vein are console recording through the phases. Logging is best enabled globally, with different default levels for SPL. We also have a lot of things like LOCALVERSION. The main Makefile looks at CONFIG_SPL_FRAMEWORK to decide whether to expect u-boot.img or not, so we'd have to have another symbol like CONFIG_FRAMEWORK that people keep in sync (or just complete the *&^#$&^# migration :-)
We have quite a bit of:
config SPL_SYS_ICACHE_OFF depends on SPL default SYS_ICACHE_OFF
I think we are throwing away a lot by separating them at the configuration stage. I'm not saying it's a disaster but I am worried that it might not lead to a good place.
My first reaction is that things will be fine with either select's (a platform using HANDOFF/BLOBLIST should be select'ing that since it's require) or matching defaults. The *CACHE_OFF example isn't actually a needs to be in sync thing, there's platforms today that disable in SPL but not full.
I'm fully willing to admit there's pitfalls I'm not seeing. And I'll further say I don't think this should be our top goal right now.
And I'm really not seeing now how we would support the example I gave as for them SPL with SATA+AHCI+PCI is not desired nor possible. I asked. The answer was no, don't want it. Or do you really just mean that if we had spl_autoconf.h the only thing that would change is that we would never test on CONFIG_SPL_xxx only CONFIG_xxx, but we would still need to Kconfig SPL_xxx?
Yes, that's what I am saying. We can make that change now (to clean up Makefile and add CONFIG()) with no change to Kconfig.
We can support the case you mention and yes we want it and need it, of course. Otherwise we are back to the SPL #undef horror.
OK. We can certainly see how it works out, if you make a patch for it.
On Mon, Aug 9, 2021 at 8:11 PM Tom Rini trini@konsulko.com wrote:
On Sat, Aug 07, 2021 at 04:23:36PM -0600, Simon Glass wrote:
Comments welcome!
I think what this is really showing is that Yamada-san was right. All the games we need to do so that "make fooboard_config all" results in building the N stages needed was the wrong track. Taking khadas-edge-v-rk3399 as an example, if we had instead of khadas-edge-v-rk3399_defconfig but khadas-edge-v-rk3399_tpl_defconfig khadas-edge-v-rk3399_spl_defconfig and khadas-edge-v-rk3399_config, each of which could set CONFIG_TPL, CONFIG_SPL or neither. Then yes, to build u-boot-rockchip.bin you would need to pass in the separately build TPL and SPL stages. But, that's true of so so many other platforms. To pick another example, imx8mm_evk doesn't function without other binaries. If in theory to build khadas-edge-v-rk3399 you had to do something like: $ export CROSS_COMPILE=aarch64-linux- $ make O=obj/tpl khadas-edge-v-rk3399_tpl_config all $ make O=obj/spl khadas-edge-v-rk3399_spl_config all $ make O=obj/khadas-edge-v-rk3399 TPL=obj/tpl/u-boot.bin \ SPL=obj/spl/u-boot.bin khadas-edge-v-rk3399_config all
But it also meant that we didn't need to duplicate so so many Kconfig options and most of our obj rules would just be: obj-$(CONFIG_FOO) += foo.o
would be a win.
Oh! I quite like this. Simpler indeed. With all the different components that need to be pulled in to build a bootable image, each of the various U-Boot stages would just be another configuration that needs to be built and included in the result.
I've already been playing with a top level build tool that starts with the U-Boot config and chooses the correct configuration of TFA, OPTEE, and any other binaries needed. Basically it is a top level Makefile that includes the U-Boot config and then passes make targets through to the right build directories. This could effectively be the same thing; start with the board config, and then filter it when going into each stages build directory with O= as you have above.
If you want to look I've got the repo published here:
https://gitlab.arm.com/systemready/u-boot-tfa-build/
g.
Hi Grant,
On Wed, 11 Aug 2021 at 03:58, Grant Likely grant.likely@secretlab.ca wrote:
On Mon, Aug 9, 2021 at 8:11 PM Tom Rini trini@konsulko.com wrote:
On Sat, Aug 07, 2021 at 04:23:36PM -0600, Simon Glass wrote:
Comments welcome!
I think what this is really showing is that Yamada-san was right. All the games we need to do so that "make fooboard_config all" results in building the N stages needed was the wrong track. Taking khadas-edge-v-rk3399 as an example, if we had instead of khadas-edge-v-rk3399_defconfig but khadas-edge-v-rk3399_tpl_defconfig khadas-edge-v-rk3399_spl_defconfig and khadas-edge-v-rk3399_config, each of which could set CONFIG_TPL, CONFIG_SPL or neither. Then yes, to build u-boot-rockchip.bin you would need to pass in the separately build TPL and SPL stages. But, that's true of so so many other platforms. To pick another example, imx8mm_evk doesn't function without other binaries. If in theory to build khadas-edge-v-rk3399 you had to do something like: $ export CROSS_COMPILE=aarch64-linux- $ make O=obj/tpl khadas-edge-v-rk3399_tpl_config all $ make O=obj/spl khadas-edge-v-rk3399_spl_config all $ make O=obj/khadas-edge-v-rk3399 TPL=obj/tpl/u-boot.bin \ SPL=obj/spl/u-boot.bin khadas-edge-v-rk3399_config all
But it also meant that we didn't need to duplicate so so many Kconfig options and most of our obj rules would just be: obj-$(CONFIG_FOO) += foo.o
would be a win.
Oh! I quite like this. Simpler indeed. With all the different components that need to be pulled in to build a bootable image, each of the various U-Boot stages would just be another configuration that needs to be built and included in the result.
Have you looked at binman? It is designed to put images together, using the various U-Boot phases and anything else about.
I've already been playing with a top level build tool that starts with the U-Boot config and chooses the correct configuration of TFA, OPTEE, and any other binaries needed. Basically it is a top level Makefile that includes the U-Boot config and then passes make targets through to the right build directories. This could effectively be the same thing; start with the board config, and then filter it when going into each stages build directory with O= as you have above.
If you want to look I've got the repo published here:
https://gitlab.arm.com/systemready/u-boot-tfa-build/
g.
On Wed, Aug 11, 2021 at 1:58 PM Simon Glass sjg@chromium.org wrote:
Hi Grant,
On Wed, 11 Aug 2021 at 03:58, Grant Likely grant.likely@secretlab.ca wrote:
On Mon, Aug 9, 2021 at 8:11 PM Tom Rini trini@konsulko.com wrote:
On Sat, Aug 07, 2021 at 04:23:36PM -0600, Simon Glass wrote:
Comments welcome!
I think what this is really showing is that Yamada-san was right. All the games we need to do so that "make fooboard_config all" results in building the N stages needed was the wrong track. Taking khadas-edge-v-rk3399 as an example, if we had instead of khadas-edge-v-rk3399_defconfig but khadas-edge-v-rk3399_tpl_defconfig khadas-edge-v-rk3399_spl_defconfig and khadas-edge-v-rk3399_config, each of which could set CONFIG_TPL, CONFIG_SPL or neither. Then yes, to build u-boot-rockchip.bin you would need to pass in the separately build TPL and SPL stages. But, that's true of so so many other platforms. To pick another example, imx8mm_evk doesn't function without other binaries. If in theory to build khadas-edge-v-rk3399 you had to do something like: $ export CROSS_COMPILE=aarch64-linux- $ make O=obj/tpl khadas-edge-v-rk3399_tpl_config all $ make O=obj/spl khadas-edge-v-rk3399_spl_config all $ make O=obj/khadas-edge-v-rk3399 TPL=obj/tpl/u-boot.bin \ SPL=obj/spl/u-boot.bin khadas-edge-v-rk3399_config all
But it also meant that we didn't need to duplicate so so many Kconfig options and most of our obj rules would just be: obj-$(CONFIG_FOO) += foo.o
would be a win.
Oh! I quite like this. Simpler indeed. With all the different components that need to be pulled in to build a bootable image, each of the various U-Boot stages would just be another configuration that needs to be built and included in the result.
Have you looked at binman? It is designed to put images together, using the various U-Boot phases and anything else about.
Yup, I looked quite closely at binman, and have been planning to use it. This tool is more about building the required projects and less about assembling the final image. I think the two are complementary.
g.
Hi Grant,
On Wed, 11 Aug 2021 at 07:47, Grant Likely grant.likely@secretlab.ca wrote:
On Wed, Aug 11, 2021 at 1:58 PM Simon Glass sjg@chromium.org wrote:
Hi Grant,
On Wed, 11 Aug 2021 at 03:58, Grant Likely grant.likely@secretlab.ca wrote:
On Mon, Aug 9, 2021 at 8:11 PM Tom Rini trini@konsulko.com wrote:
On Sat, Aug 07, 2021 at 04:23:36PM -0600, Simon Glass wrote:
Comments welcome!
I think what this is really showing is that Yamada-san was right. All the games we need to do so that "make fooboard_config all" results in building the N stages needed was the wrong track. Taking khadas-edge-v-rk3399 as an example, if we had instead of khadas-edge-v-rk3399_defconfig but khadas-edge-v-rk3399_tpl_defconfig khadas-edge-v-rk3399_spl_defconfig and khadas-edge-v-rk3399_config, each of which could set CONFIG_TPL, CONFIG_SPL or neither. Then yes, to build u-boot-rockchip.bin you would need to pass in the separately build TPL and SPL stages. But, that's true of so so many other platforms. To pick another example, imx8mm_evk doesn't function without other binaries. If in theory to build khadas-edge-v-rk3399 you had to do something like: $ export CROSS_COMPILE=aarch64-linux- $ make O=obj/tpl khadas-edge-v-rk3399_tpl_config all $ make O=obj/spl khadas-edge-v-rk3399_spl_config all $ make O=obj/khadas-edge-v-rk3399 TPL=obj/tpl/u-boot.bin \ SPL=obj/spl/u-boot.bin khadas-edge-v-rk3399_config all
But it also meant that we didn't need to duplicate so so many Kconfig options and most of our obj rules would just be: obj-$(CONFIG_FOO) += foo.o
would be a win.
Oh! I quite like this. Simpler indeed. With all the different components that need to be pulled in to build a bootable image, each of the various U-Boot stages would just be another configuration that needs to be built and included in the result.
Have you looked at binman? It is designed to put images together, using the various U-Boot phases and anything else about.
Yup, I looked quite closely at binman, and have been planning to use it. This tool is more about building the required projects and less about assembling the final image. I think the two are complementary.
Yes I see. I hope that we don't have to build the projects dependent on each other. At least for U-Boot, it should not depend on other projects for its config at build time. So for example the BL31 thing should be handled post-build (by binman).
Regards, SImon
On 8/7/21 6:23 PM, Simon Glass wrote:
Hi,
U-Boot can be configured to build the source multiple times to product multiple 'phases'. The main phase is the full U-Boot, but an 'SPL' (Secondary Program Loader) build can produce a cut-down image only suitable for loading U-Boot proper.
SPL does not want to use the same Kconfig options, since that would produce the same binary. Instead we have two copies of some options, one with an SPL prefix, that can be configured independently. In the source code we can use a macro to see if code should be run:
if (CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER)) { ... }This expands to check either checking SYS_STDIO_DEREGISTER or SPL_SYS_STDIO_DEREGISTER, depending on which phase is being built.
U-Boot also has TPL (Tertiary Program Loader) which works a similar way. This is causing quite an expansion of the Kconfig source, with quite a bit of duplication. Each time a new feature needs to be supported in SPL, it involves a patch to add the same options again but for SPL.
Here are some proposed changes to make it easier to deal with SPL/TPL:
- Kconfig support
At present we do things like this when we need to control an option separately in U-Boot proper and SPL:
config SYS_STDIO_DEREGISTER bool "Allow deregistering stdio devices" depends on DM_DEVICE_REMOVE default y if USB_KEYBOARD help Generally there is no need to deregister stdio devices since they are never deactivated. But if a stdio device is used which can be removed (for example a USB keyboard) then this option can be enabled to ensure this is handled correctly. config SPL_SYS_STDIO_DEREGISTER bool "Allow deregistering stdio devices in SPL" depends on SPL_DM_DEVICE_REMOVE help Generally there is no need to deregister stdio devices since they are never deactivated. But if a stdio device is used which can be removed (for example a USB keyboard) then this option can be enabled to ensure this is handled correctly. This is very rarely needed in SPL.This is a pain. Apart from the duplication, sometimes the SPL version is in a different file or a different part of the file, making it hard to find related options or update them in sync.
Instead, we can add a 'phase' command to the Kconfig language, so we can do:
config SYS_STDIO_DEREGISTER bool "Allow deregistering stdio devices" phases depends on p.DM_DEVICE_REMOVE phase MAIN default y if USB_KEYBOARD help Generally there is no need to deregister stdio devices since they are never deactivated. But if a stdio device is used which can be removed (for example a USB keyboard) then this option can be enabled to ensure this is handled correctly.'phases' means this Kconfig option exists in all phases. You can also say 'phases MAIN SPL' to select just MAIN (U-Boot) and SPL.
'p.DM_DEVICE_REMOVE' means to prefix the phase with each symbol, so for U-Boot (which uses SYS_STDIO_DEREGISTER) this means DM_DEVICE_REMOVE (p is empty) and for SPL (which uses SPL_SYS_STDIO_DEREGISTER) it means SPL_DM_DEVICE_REMOVE (p is SPL_). This is somewhat similar in style to the special-case 'depends on m' in Kconfig.
To make this work, we tell Kconfig that SPL is a phase with 'def_phase':
config SPL def_phase depends on SUPPORT_SPL prompt "Enable SPL" help If you want to build SPL as well as the normal image, say Y.It works just the same as a bool, but kconfig also uses it to automatically add new Kconfigs for each phase. In the above example it creates both SYS_STDIO_DEREGISTER and SPL_SYS_STDIO_DEREGISTER. The option name has the text '(SPL) ' shown before the SPL option.
This can easily handle Kconfigs with similar dependencies and even different ones. If the Kconfig options are not actually very similar we can still create two separate copies instead, as now.
This allows us to substantially reduce the size and duplication in the Kconfig defintions. It also reduces the pain of adding another phase to U-Boot.
Note: This change needs to be done in Linux, which owns Kconfig upstream.
2.Rename SPL_TPL_
This Makefile variable is used to reduce the number of duplicate rules in makefiles:
obj-$(CONFIG_$(SPL_TPL_)FIT_SIGNATURE) += fdt_region.oThe SPL_TPL_ expands to empty for U-Boot and either SPL_ or TPL_ for the other phases.
This is confusing though, since CONFIG_SPL_BUILD it set even for TPL builds, so for example. with:
obj-$(CONFIG_$(SPL_)FIT_SIGNATURE) += fdt_region.othe file is built for both SPL and TPL.
To help with this, We can rename SPL_TPL to PHASE_:
obj-$(CONFIG_$(PHASE_)FIT_SIGNATURE) += fdt_region.oor perhaps P_ which is more readable:
obj-$(CONFIG_$(P_)FIT_SIGNATURE) += fdt_region.o
- Rename CONFIG_IS_ENABLED()
This macro is used to determine whether an option is enabled in the current build phase:
if (CONFIG_IS_ENABLED(FIT_SIGNATURE)) { printf("## Checking hash(es) for Image %s ... ", fit_get_name(fit, node, NULL));It is quite long-winded and people sometimes add CONFIG_ to the option inside the brakets by mistake. It is also a bit confusing that IS_ENABLED() and CONFIG_IS_ENABLED() mean completely different things.
Instead we can rename it to CONFIG:
if (CONFIG(FIT_SIGNATURE)) { printf("## Checking hash(es) for Image %s ... ", fit_get_name(fit, node, NULL));This is shorter and looks more like CONFIG_FIT_SIGATURE so people should find it easier to understand. Being shorter is a big help when converting from use #if to if(), since an indentation is always enabled. This change makes the CONFIG() check no longer than IS_ENABLED().
It also makes CONFIG(OPTION) not much longer than CONFIG_OPTION, which makes things much more convenient, since ideally if the toolchain permitted it, we would just use CONFIG_OPTION in the code. This is not possible at present since the option may not be defined, so can cause a compiler error.
Over time, perhaps the existing IS_ENABLED() will phase out, since in many cases SPL will have its own options. We already see that CONFIG_IS_ENABLED is more popular / useful:
$ git grep -w IS_ENABLED |wc -l 902 $ git grep -w CONFIG_IS_ENABLED |wc -l 2282
I like this. Making this macro shorter is a good ergonomic improvement.
- 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
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.
... };It still isn't wonderfully readable but it seems like an improvement. The IF_CONFIG() macros could be implemented easily with the current CONFIG_IS_EANBLED() macro.
- IF_CONFIG_INT() or similar
See here: https://lists.denx.de/pipermail/u-boot/2020-May/412950.html
This would be nice as well.
- 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_idapl_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?
--Sean
Hi Sean,
On Mon, 9 Aug 2021 at 16:31, Sean Anderson sean.anderson@seco.com wrote:
On 8/7/21 6:23 PM, Simon Glass wrote:
Hi,
U-Boot can be configured to build the source multiple times to product
multiple
'phases'. The main phase is the full U-Boot, but an 'SPL' (Secondary
Program
Loader) build can produce a cut-down image only suitable for loading
U-Boot
proper.
SPL does not want to use the same Kconfig options, since that would
produce the
same binary. Instead we have two copies of some options, one with an
SPL prefix,
that can be configured independently. In the source code we can use a
macro to
see if code should be run:
if (CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER)) { ... }This expands to check either checking SYS_STDIO_DEREGISTER or SPL_SYS_STDIO_DEREGISTER, depending on which phase is being built.
U-Boot also has TPL (Tertiary Program Loader) which works a similar
way. This
is causing quite an expansion of the Kconfig source, with quite a bit of duplication. Each time a new feature needs to be supported in SPL, it
involves
a patch to add the same options again but for SPL.
Here are some proposed changes to make it easier to deal with SPL/TPL:
- Kconfig support
[..]
- Add macros to help avoid more #ifdefs
We sometimes have to use #ifdefs in structs or drivers:
struct spl_image_loader { #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT const char *name; #endif ... }; UCLASS_DRIVER(spi) = { .id = UCLASS_SPI, .name = "spi", .flags = DM_UC_FLAG_SEQ_ALIAS, #if CONFIG_IS_ENABLED(OF_CONTROL) &&
!CONFIG_IS_ENABLED(OF_PLATDATA)
.post_bind = dm_scan_fdt_dev, #endif ... };This is a pain. We can add an IF_CONFIG macro to help with this:
struct spl_image_loader { IF_CONFIG(LIBCOMMON_SUPPORT, const char *name;) ... }; UCLASS_DRIVER(spi) = { .id = UCLASS_SPI, .name = "spi", .flags = DM_UC_FLAG_SEQ_ALIAS, IF_CONFIG(REAL, .post_bind = dm_scan_fdt_dev,)Wouldn't cpp eat the trailing comma here? This seems pretty tricky to use. And of course, you still need something like
Yes you're right. The comma is a pain. Needs more thought.
struct spl_image_loader *loader; loader = ... ;#if CONFIG(LIBCOMMON_SUPPORT) foo(loader->name); #endif
unless you have some other macro to wrap the name access.
Yes that's the other side of it that I forgot. I think a macro that converts the member to NULL or something might work. Needs more thought.
[..]
- Discarding static functions
We sometimes see code like this:
#if CONFIG_IS_ENABLED(OF_REAL) static const struct udevice_id apl_ns16550_serial_ids[] = { { .compatible = "intel,apl-ns16550" }, { }, }; #endif U_BOOT_DRIVER(intel_apl_ns16550) = { .name = "intel_apl_ns16550", .id = UCLASS_SERIAL, .of_match = of_match_ptr(apl_ns16550_serial_ids), .plat_auto = sizeof(struct apl_ns16550_plat), .priv_auto = sizeof(struct ns16550), ... };The of_match_ptr() avoids an #ifdef in the driver declaration since it
evaluates
to NULL if !CONFIG_IS_ENABLED(OF_REAL) but we still need an #ifdef
around the
function, since it is static and would otherwise produce a warning.
One solution is to drop the 'static'. But this is not very nice, since
the
structure clearly should not be used from another file.
We can add STATIC_IF_CONFIG() to help with this:
STATIC_IF_CONFIG(OF_REAL) const struct udevice_idapl_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
participants (4)
-
Grant Likely -
Sean Anderson -
Simon Glass -
Tom Rini