| Message ID | 20200923191105.34415-2-matthew.weber@rockwellcollins.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Support uboot file creation w/o uboot | expand |
Hi Matt, Some comments about legacy handling, but also something approximating a NACK at the end... On 23/09/2020 21:11, Matt Weber wrote: > Migrating the support for this feature to uboot-tools to gain the > ability to build env files when BR2_TARGET_UBOOT isn't selected. > > Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com> > --- > Config.in.legacy | 7 ++++ > boot/uboot/Config.in | 43 ------------------------ > boot/uboot/uboot.mk | 25 -------------- > package/uboot-tools/Config.in.host | 54 ++++++++++++++++++++++++++++-- > package/uboot-tools/uboot-tools.mk | 33 ++++++++++++++++++ > 5 files changed, 92 insertions(+), 70 deletions(-) > > diff --git a/Config.in.legacy b/Config.in.legacy > index ae583b912f..6359ca148c 100644 > --- a/Config.in.legacy > +++ b/Config.in.legacy > @@ -146,6 +146,13 @@ endif > > comment "Legacy options removed in 2020.11" > > +config BR2_TARGET_UBOOT_ENVIMAGE > + bool "u-boot env generation was moved" > + select BR2_LEGACY > + select BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE This won't work, you need to select host-uboot-tools explicitly: select BR2_PACKAGE_HOST_UBOOT_TOOLS select BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE (same for the other patch). Also, we need legacy handling for all of the suboptions as well, since you changed their name. See the section about handling strings at the beginning of Config.in.legacy. [snip] > +if BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE > + > +config BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SOURCE > + string "Source files for environment" Just to be clear: here you'll need default BR2_TARGET_UBOOT_ENVIMAGE_SOURCE if BR2_TARGET_UBOOT_ENVIMAGE_SOURCE != "" # legacy [snip] > diff --git a/package/uboot-tools/uboot-tools.mk b/package/uboot-tools/uboot-tools.mk > index a06c25998f..c26f5d332c 100644 > --- a/package/uboot-tools/uboot-tools.mk > +++ b/package/uboot-tools/uboot-tools.mk > @@ -110,10 +110,43 @@ define HOST_UBOOT_TOOLS_BUILD_CMDS > $(BR2_MAKE1) -C $(@D) $(HOST_UBOOT_TOOLS_MAKE_OPTS) tools-only > endef > > +ifeq ($(BR2_TARGET_UBOOT),y) > +define UBOOT_TOOLS_GENERATE_ENV_DEFAULTS (nitpick) since this is used in HOST_UBOOT_TOOLS_INSTALL_CMDS, it should be called HOST_UBOOT_TOOLS_GENERATE_ENV_DEFAULTS. Same for all the other variables introduced here. > + CROSS_COMPILE="$(TARGET_CROSS)" \ > + $(UBOOT_SRCDIR)/scripts/get_default_envs.sh \ > + $(UBOOT_SRCDIR) \ > + >$(@D)/boot-env-defaults.txt > +endef > +UBOOT_TOOLS_DEPENDENCIES += uboot This raised my eyebrows. For one thing, until the second patch is applied, this will create a circular dependency. But in general, we don't like it if one package peeks into the source tree of another package. So I wonder if this is the best approach. To be honest, in general I'm struggling a bit with our uboot-tools package. Ideally, if you build U-Boot, you'd like to use the same version for uboot-tools as well. So I'm thinking, maybe we should make uboot-tools a virtual package that can be implemented by uboot itself, or by a new uboot-tools-standalone package if uboot is not selected. Well, that's maybe a bit too complicated, and the perfect is the enemy of the good, and your use case makes a lot of sense, so let's merge this after all :-) > +endif > + > +ifneq ($(BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE),) > +UBOOT_TOOLS_GENERATE_ENV_FILE = $(call qstrip,$(BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SOURCE)) > +define UBOOT_TOOLS_GENERATE_ENV_IMAGE > + $(if $(UBOOT_TOOLS_GENERATE_ENV_FILE), \ > + cat $(UBOOT_TOOLS_GENERATE_ENV_FILE), \ > + cat $(@D)/boot-env-defaults.txt) \ > + >$(@D)/buildroot-env.txt Since we now always have a file, the cat becomes redundant. Instead, we can just use the appropriate file directly instead of buildroot-env.txt. > + $(HOST_DIR)/bin/mkenvimage -s $(BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SIZE) \ > + $(if $(BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_REDUNDANT),-r) \ > + $(if $(filter "BIG",$(BR2_ENDIAN)),-b) \ > + -o $(BINARIES_DIR)/uboot-env.bin \ > + $(@D)/buildroot-env.txt ... so here put the $(if $(UBOOT_TOOLS_GENERATE_ENV_FILE), ....) > +endef > + > +ifeq ($(BR_BUILDING),y) > +ifeq ($(call qstrip,$(BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SIZE)),) > +$(error Please provide U-Boot environment size (BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SIZE setting)) We also need to check here that UBOOT_TOOLS_GENERATE_ENV_FILE is non-empty unless BR2_TARGET_UBOOT is selected. Also, these new options will break the random config generation, because the string options don't get set. So we either need to provide default values for those strings (nasty IMO - you can't give a default that is sensible to the user for either of them) or we have to update genrandconfig to handle the situation (preferably by giving them a value, so this feature gets tested in the autobuilders). Given all this feedback, I've marked as Changes Requested in patchwork. Both of them since it mostly also applies to the second patch. Regards, Arnout > +endif > +endif > +endif #BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE > + > define HOST_UBOOT_TOOLS_INSTALL_CMDS > $(INSTALL) -m 0755 -D $(@D)/tools/mkimage $(HOST_DIR)/bin/mkimage > $(INSTALL) -m 0755 -D $(@D)/tools/mkenvimage $(HOST_DIR)/bin/mkenvimage > $(INSTALL) -m 0755 -D $(@D)/tools/dumpimage $(HOST_DIR)/bin/dumpimage > + $(UBOOT_TOOLS_GENERATE_ENV_DEFAULTS) > + $(UBOOT_TOOLS_GENERATE_ENV_IMAGE) > endef > > $(eval $(generic-package)) >
Hi Arnout, On Wed, Sep 23, 2020 at 4:01 PM Arnout Vandecappelle <arnout@mind.be> wrote: > > Hi Matt, > > Some comments about legacy handling, but also something approximating a NACK at > the end... > > On 23/09/2020 21:11, Matt Weber wrote: > > Migrating the support for this feature to uboot-tools to gain the > > ability to build env files when BR2_TARGET_UBOOT isn't selected. > > > > Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com> > > --- > > Config.in.legacy | 7 ++++ > > boot/uboot/Config.in | 43 ------------------------ > > boot/uboot/uboot.mk | 25 -------------- > > package/uboot-tools/Config.in.host | 54 ++++++++++++++++++++++++++++-- > > package/uboot-tools/uboot-tools.mk | 33 ++++++++++++++++++ > > 5 files changed, 92 insertions(+), 70 deletions(-) > > > > diff --git a/Config.in.legacy b/Config.in.legacy > > index ae583b912f..6359ca148c 100644 > > --- a/Config.in.legacy > > +++ b/Config.in.legacy > > @@ -146,6 +146,13 @@ endif > > > > comment "Legacy options removed in 2020.11" > > > > +config BR2_TARGET_UBOOT_ENVIMAGE > > + bool "u-boot env generation was moved" > > + select BR2_LEGACY > > + select BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE > > This won't work, you need to select host-uboot-tools explicitly: > > select BR2_PACKAGE_HOST_UBOOT_TOOLS > select BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE > > (same for the other patch). > > Also, we need legacy handling for all of the suboptions as well, since you > changed their name. See the section about handling strings at the beginning of > Config.in.legacy. > Ah good catch, I'll fix that up. > [snip] > > diff --git a/package/uboot-tools/uboot-tools.mk b/package/uboot-tools/uboot-tools.mk > > index a06c25998f..c26f5d332c 100644 > > --- a/package/uboot-tools/uboot-tools.mk > > +++ b/package/uboot-tools/uboot-tools.mk > > @@ -110,10 +110,43 @@ define HOST_UBOOT_TOOLS_BUILD_CMDS > > $(BR2_MAKE1) -C $(@D) $(HOST_UBOOT_TOOLS_MAKE_OPTS) tools-only > > endef > > > > +ifeq ($(BR2_TARGET_UBOOT),y) > > +define UBOOT_TOOLS_GENERATE_ENV_DEFAULTS > > (nitpick) since this is used in HOST_UBOOT_TOOLS_INSTALL_CMDS, it should be > called HOST_UBOOT_TOOLS_GENERATE_ENV_DEFAULTS. Same for all the other variables > introduced here. Sure > > > + CROSS_COMPILE="$(TARGET_CROSS)" \ > > + $(UBOOT_SRCDIR)/scripts/get_default_envs.sh \ > > + $(UBOOT_SRCDIR) \ > > + >$(@D)/boot-env-defaults.txt > > +endef > > +UBOOT_TOOLS_DEPENDENCIES += uboot > > This raised my eyebrows. For one thing, until the second patch is applied, this > will create a circular dependency. But in general, we don't like it if one > package peeks into the source tree of another package. So I wonder if this is > the best approach. > > To be honest, in general I'm struggling a bit with our uboot-tools package. > Ideally, if you build U-Boot, you'd like to use the same version for uboot-tools > as well. So I'm thinking, maybe we should make uboot-tools a virtual package > that can be implemented by uboot itself, or by a new uboot-tools-standalone > package if uboot is not selected. > > Well, that's maybe a bit too complicated, and the perfect is the enemy of the > good, and your use case makes a lot of sense, so let's merge this after all :-) :-) I did find that this dependency needed to be HOST_UBOOT_TOOLS_DEPENDENCIES, I had missed that in my v1 > > > > +endif > > + > > +ifneq ($(BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE),) > > +UBOOT_TOOLS_GENERATE_ENV_FILE = $(call qstrip,$(BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SOURCE)) > > +define UBOOT_TOOLS_GENERATE_ENV_IMAGE > > + $(if $(UBOOT_TOOLS_GENERATE_ENV_FILE), \ > > + cat $(UBOOT_TOOLS_GENERATE_ENV_FILE), \ > > + cat $(@D)/boot-env-defaults.txt) \ > > + >$(@D)/buildroot-env.txt > > Since we now always have a file, the cat becomes redundant. Instead, we can > just use the appropriate file directly instead of buildroot-env.txt. Sure > > + > > +ifeq ($(BR_BUILDING),y) > > +ifeq ($(call qstrip,$(BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SIZE)),) > > +$(error Please provide U-Boot environment size (BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SIZE setting)) > > We also need to check here that UBOOT_TOOLS_GENERATE_ENV_FILE is non-empty > unless BR2_TARGET_UBOOT is selected. > Sure > Also, these new options will break the random config generation, because the > string options don't get set. So we either need to provide default values for > those strings (nasty IMO - you can't give a default that is sensible to the user > for either of them) or we have to update genrandconfig to handle the situation > (preferably by giving them a value, so this feature gets tested in the > autobuilders). > I will look to add a conditional in genrandconfig to update to point at an empty file. Or a file with a single command / value. (testing now) Thanks for the review Matt
diff --git a/Config.in.legacy b/Config.in.legacy index ae583b912f..6359ca148c 100644 --- a/Config.in.legacy +++ b/Config.in.legacy @@ -146,6 +146,13 @@ endif comment "Legacy options removed in 2020.11" +config BR2_TARGET_UBOOT_ENVIMAGE + bool "u-boot env generation was moved" + select BR2_LEGACY + select BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE + help + Migrated U-Boot env generation to uboot-tools + config BR2_PACKAGE_GQVIEW bool "gqview package was removed" select BR2_LEGACY diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in index a87a642581..668806dc50 100644 --- a/boot/uboot/Config.in +++ b/boot/uboot/Config.in @@ -533,49 +533,6 @@ config BR2_TARGET_UBOOT_ALTERA_SOCFPGA_IMAGE_CRC In either case the resulting file will be given a .crc extension. -menuconfig BR2_TARGET_UBOOT_ENVIMAGE - bool "Environment image" - help - Generate a valid binary environment image from a text file - describing the key=value pairs of the environment. - - The environment image will be called uboot-env.bin. - -if BR2_TARGET_UBOOT_ENVIMAGE - -config BR2_TARGET_UBOOT_ENVIMAGE_SOURCE - string "Source files for environment" - help - Text files describing the environment. Files should have - lines of the form var=value, one per line. Blank lines and - lines starting with a # are ignored. - - Multiple source files are concatenated in the order listed. - - Leave empty to generate image from compiled-in env. - -config BR2_TARGET_UBOOT_ENVIMAGE_SIZE - string "Size of environment" - help - Size of envronment, can be prefixed with 0x for hexadecimal - values. - -config BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT - bool "Environment has two copies" - help - Some platforms define in their U-Boot configuration that the - U-Boot environment should be duplicated in two locations (for - extra safety). Check your U-Boot configuration for the - CONFIG_ENV_ADDR_REDUND and CONFIG_ENV_SIZE_REDUND settings to - see if this is the case for your platform. - - If it is the case, then you should enable this option to - ensure that the U-Boot environment image generated by - Buildroot is compatible with the "redundant environment" - mechanism of U-Boot. - -endif # BR2_TARGET_UBOOT_ENVIMAGE - config BR2_TARGET_UBOOT_BOOT_SCRIPT bool "Generate a U-Boot boot script" help diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk index 1831466780..9dbd06b64b 100644 --- a/boot/uboot/uboot.mk +++ b/boot/uboot/uboot.mk @@ -312,21 +312,6 @@ define UBOOT_BUILD_OMAP_IFT -c $(call qstrip,$(BR2_TARGET_UBOOT_OMAP_IFT_CONFIG)) endef -ifneq ($(BR2_TARGET_UBOOT_ENVIMAGE),) -UBOOT_GENERATE_ENV_FILE = $(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE)) -define UBOOT_GENERATE_ENV_IMAGE - $(if $(UBOOT_GENERATE_ENV_FILE), \ - cat $(UBOOT_GENERATE_ENV_FILE), \ - CROSS_COMPILE="$(TARGET_CROSS)" $(@D)/scripts/get_default_envs.sh $(@D)) \ - >$(@D)/buildroot-env.txt - $(HOST_DIR)/bin/mkenvimage -s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \ - $(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \ - $(if $(filter "BIG",$(BR2_ENDIAN)),-b) \ - -o $(BINARIES_DIR)/uboot-env.bin \ - $(@D)/buildroot-env.txt -endef -endif - define UBOOT_INSTALL_IMAGES_CMDS $(foreach f,$(UBOOT_BINS), \ cp -dpf $(@D)/$(f) $(BINARIES_DIR)/ @@ -338,7 +323,6 @@ define UBOOT_INSTALL_IMAGES_CMDS cp -dpf $(@D)/$(f) $(BINARIES_DIR)/ ) ) - $(UBOOT_GENERATE_ENV_IMAGE) $(if $(BR2_TARGET_UBOOT_BOOT_SCRIPT), $(MKIMAGE) -C none -A $(MKIMAGE_ARCH) -T script \ -d $(call qstrip,$(BR2_TARGET_UBOOT_BOOT_SCRIPT_SOURCE)) \ @@ -443,15 +427,6 @@ define UBOOT_KCONFIG_FIXUP_CMDS $(UBOOT_ZYNQMP_KCONFIG_PSU_INIT) endef -ifeq ($(BR2_TARGET_UBOOT_ENVIMAGE),y) -ifeq ($(BR_BUILDING),y) -ifeq ($(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SIZE)),) -$(error Please provide U-Boot environment size (BR2_TARGET_UBOOT_ENVIMAGE_SIZE setting)) -endif -endif -UBOOT_DEPENDENCIES += host-uboot-tools -endif - ifeq ($(BR2_TARGET_UBOOT_BOOT_SCRIPT),y) ifeq ($(BR_BUILDING),y) ifeq ($(call qstrip,$(BR2_TARGET_UBOOT_BOOT_SCRIPT_SOURCE)),) diff --git a/package/uboot-tools/Config.in.host b/package/uboot-tools/Config.in.host index 52a4c2ec30..cf8aed1edb 100644 --- a/package/uboot-tools/Config.in.host +++ b/package/uboot-tools/Config.in.host @@ -37,6 +37,56 @@ config BR2_PACKAGE_HOST_UBOOT_TOOLS_FIT_SIGNATURE_SUPPORT public key is stored in a non-volatile place, any image can be verified in this way. -endif +endif # BR2_PACKAGE_HOST_UBOOT_TOOLS_FIT_SUPPORT -endif +config BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE + bool "Environment image" + help + Generate a valid binary environment image from a text file + describing the key=value pairs of the environment. + + This option can be useful to build enviornment configurations + as part of a Linux / rootfs only defconfig instead of using + post scripts. This supports a hardware use case of a single + bootloader only defconfig but multiple Linux / rootfs + defconfigs with different boot environments. + + The environment image will be called uboot-env.bin. + +if BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE + +config BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SOURCE + string "Source files for environment" + help + Text files describing the environment. Files should have + lines of the form var=value, one per line. Blank lines and + lines starting with a # are ignored. + + Multiple source files are concatenated in the order listed. + + Leave empty to generate image from compiled-in env if a U-boot + target build is configured (BR2_TARGET_UBOOT) + +config BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SIZE + string "Size of environment" + help + Size of envronment, can be prefixed with 0x for hexadecimal + values. + +config BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_REDUNDANT + bool "Environment has two copies" + help + Some platforms define in their U-Boot configuration that the + U-Boot environment should be duplicated in two locations (for + extra safety). Check your U-Boot configuration for the + CONFIG_ENV_ADDR_REDUND and CONFIG_ENV_SIZE_REDUND settings to + see if this is the case for your platform. + + If it is the case, then you should enable this option to + ensure that the U-Boot environment image generated by + Buildroot is compatible with the "redundant environment" + mechanism of U-Boot. + +endif # BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE + +endif # BR2_PACKAGE_HOST_UBOOT_TOOLS diff --git a/package/uboot-tools/uboot-tools.mk b/package/uboot-tools/uboot-tools.mk index a06c25998f..c26f5d332c 100644 --- a/package/uboot-tools/uboot-tools.mk +++ b/package/uboot-tools/uboot-tools.mk @@ -110,10 +110,43 @@ define HOST_UBOOT_TOOLS_BUILD_CMDS $(BR2_MAKE1) -C $(@D) $(HOST_UBOOT_TOOLS_MAKE_OPTS) tools-only endef +ifeq ($(BR2_TARGET_UBOOT),y) +define UBOOT_TOOLS_GENERATE_ENV_DEFAULTS + CROSS_COMPILE="$(TARGET_CROSS)" \ + $(UBOOT_SRCDIR)/scripts/get_default_envs.sh \ + $(UBOOT_SRCDIR) \ + >$(@D)/boot-env-defaults.txt +endef +UBOOT_TOOLS_DEPENDENCIES += uboot +endif + +ifneq ($(BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE),) +UBOOT_TOOLS_GENERATE_ENV_FILE = $(call qstrip,$(BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SOURCE)) +define UBOOT_TOOLS_GENERATE_ENV_IMAGE + $(if $(UBOOT_TOOLS_GENERATE_ENV_FILE), \ + cat $(UBOOT_TOOLS_GENERATE_ENV_FILE), \ + cat $(@D)/boot-env-defaults.txt) \ + >$(@D)/buildroot-env.txt + $(HOST_DIR)/bin/mkenvimage -s $(BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SIZE) \ + $(if $(BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_REDUNDANT),-r) \ + $(if $(filter "BIG",$(BR2_ENDIAN)),-b) \ + -o $(BINARIES_DIR)/uboot-env.bin \ + $(@D)/buildroot-env.txt +endef + +ifeq ($(BR_BUILDING),y) +ifeq ($(call qstrip,$(BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SIZE)),) +$(error Please provide U-Boot environment size (BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE_SIZE setting)) +endif +endif +endif #BR2_PACKAGE_HOST_UBOOT_TOOLS_ENVIMAGE + define HOST_UBOOT_TOOLS_INSTALL_CMDS $(INSTALL) -m 0755 -D $(@D)/tools/mkimage $(HOST_DIR)/bin/mkimage $(INSTALL) -m 0755 -D $(@D)/tools/mkenvimage $(HOST_DIR)/bin/mkenvimage $(INSTALL) -m 0755 -D $(@D)/tools/dumpimage $(HOST_DIR)/bin/dumpimage + $(UBOOT_TOOLS_GENERATE_ENV_DEFAULTS) + $(UBOOT_TOOLS_GENERATE_ENV_IMAGE) endef $(eval $(generic-package))
Migrating the support for this feature to uboot-tools to gain the ability to build env files when BR2_TARGET_UBOOT isn't selected. Signed-off-by: Matthew Weber <matthew.weber@rockwellcollins.com> --- Config.in.legacy | 7 ++++ boot/uboot/Config.in | 43 ------------------------ boot/uboot/uboot.mk | 25 -------------- package/uboot-tools/Config.in.host | 54 ++++++++++++++++++++++++++++-- package/uboot-tools/uboot-tools.mk | 33 ++++++++++++++++++ 5 files changed, 92 insertions(+), 70 deletions(-)