Message ID | 20221121201638.30951-1-brandon.maier@collins.com |
---|---|
State | Superseded |
Headers | show |
Series | [1/1] boot/uboot/uboot.mk: fix zynqmp without pmufw | expand |
Hi Brandon, On Mon, 21 Nov 2022 14:16:38 -0600 Brandon Maier <brandon.maier@collins.com> wrote: > Commit d07e6b70 (boot/uboot/uboot.mk: add pmufw.elf support) broke > configurations where the UBOOT_ZYNQMP_PMUFW was blank. Previously it > would set the U-Boot CONFIG_PMUFW_INIT_FILE to the blank string, but now > it will set it to ".bin" which causes U-Boot to fail to build. > > To fix, disable all the pmufw code if the UBOOT_ZYNQMP_PMUFW is blank. > > Signed-off-by: Brandon Maier <brandon.maier@collins.com> You are right, this was missed during patch review. Thanks for reporting! > diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk > index 0439ec5e4b..b86248e1f7 100644 > --- a/boot/uboot/uboot.mk > +++ b/boot/uboot/uboot.mk > @@ -383,6 +383,7 @@ endef > ifeq ($(BR2_TARGET_UBOOT_ZYNQMP),y) > > UBOOT_ZYNQMP_PMUFW = $(call qstrip,$(BR2_TARGET_UBOOT_ZYNQMP_PMUFW)) > +ifneq ($(UBOOT_ZYNQMP_PMUFW),) > I don't like an empty line after the ifneq unless we have one before _and_ one before the endif too. But this is minor, stylish nitpicking, see below for something a bit more interesting. > ifneq ($(findstring ://,$(UBOOT_ZYNQMP_PMUFW)),) > UBOOT_EXTRA_DOWNLOADS += $(UBOOT_ZYNQMP_PMUFW) > @@ -398,6 +399,7 @@ define UBOOT_ZYNQMP_KCONFIG_PMUFW > objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_BASENAME).elf $(UBOOT_ZYNQMP_PMUFW_BASENAME).bin) > $(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_BASENAME).bin") > endef > +endif # UBOOT_ZYNQMP_PMUFW > > UBOOT_ZYNQMP_PM_CFG = $(call qstrip,$(BR2_TARGET_UBOOT_ZYNQMP_PM_CFG)) > ifneq ($(UBOOT_ZYNQMP_PM_CFG),) This would be an effective fix for the known cases (empty string, .elf, .bin), but it would break again whenever Xilinx or anybody else invents another extension. Not likely maybe, but overall I would prefer a fix like (untested): define UBOOT_ZYNQMP_KCONFIG_PMUFW + $(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH)") $(if $(filter %.elf,$(UBOOT_ZYNQMP_PMUFW_PATH)), - objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_BASENAME).elf $(UBOOT_ZYNQMP_PMUFW_BASENAME).bin) + objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_BASENAME).elf $(UBOOT_ZYNQMP_PMUFW_BASENAME).bin + $(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_BASENAME).bin"))) endef The above reverts to always setting CONFIG_PMUFW_INIT_FILE to the unmodified value of UBOOT_ZYNQMP_PMUFW_PATH as we used to do before d07e6b70, then mangles it only in the special "*.elf" case. Would you mind testing that and sending a patch?
Hi Brandon, > Commit d07e6b70 (boot/uboot/uboot.mk: add pmufw.elf support) broke > configurations where the UBOOT_ZYNQMP_PMUFW was blank. Previously it > would set the U-Boot CONFIG_PMUFW_INIT_FILE to the blank string, but > now it will set it to ".bin" which causes U-Boot to fail to build. > > To fix, disable all the pmufw code if the UBOOT_ZYNQMP_PMUFW is blank. > > Signed-off-by: Brandon Maier <brandon.maier@collins.com> > You are right, this was missed during patch review. Thanks for reporting! > diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk index > 0439ec5e4b..b86248e1f7 100644 > --- a/boot/uboot/uboot.mk > +++ b/boot/uboot/uboot.mk > @@ -383,6 +383,7 @@ endef > ifeq ($(BR2_TARGET_UBOOT_ZYNQMP),y) > > UBOOT_ZYNQMP_PMUFW = $(call qstrip,$(BR2_TARGET_UBOOT_ZYNQMP_PMUFW)) > +ifneq ($(UBOOT_ZYNQMP_PMUFW),) > > I don't like an empty line after the ifneq unless we have one before _and_ one before the endif too. But this is minor, stylish nitpicking, see below for something a bit more interesting. > ifneq ($(findstring ://,$(UBOOT_ZYNQMP_PMUFW)),) > UBOOT_EXTRA_DOWNLOADS += $(UBOOT_ZYNQMP_PMUFW) @@ -398,6 +399,7 @@ > define UBOOT_ZYNQMP_KCONFIG_PMUFW > objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_BASENAME).elf $(UBOOT_ZYNQMP_PMUFW_BASENAME).bin) > $(call > KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_BASENAME) > .bin") > endef > +endif # UBOOT_ZYNQMP_PMUFW > > UBOOT_ZYNQMP_PM_CFG = $(call > qstrip,$(BR2_TARGET_UBOOT_ZYNQMP_PM_CFG)) > ifneq ($(UBOOT_ZYNQMP_PM_CFG),) > This would be an effective fix for the known cases (empty string, .elf, .bin), but it would break again whenever Xilinx or anybody else invents another extension. Not likely maybe, but overall I would prefer a fix like (untested): > define UBOOT_ZYNQMP_KCONFIG_PMUFW > + $(call > + KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH)") > $(if $(filter %.elf,$(UBOOT_ZYNQMP_PMUFW_PATH)), > - objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_BASENAME).elf $(UBOOT_ZYNQMP_PMUFW_BASENAME).bin) > + objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_BASENAME).elf $(UBOOT_ZYNQMP_PMUFW_BASENAME).bin > + $(call > + KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_BASENAME) > + .bin"))) > endef > The above reverts to always setting CONFIG_PMUFW_INIT_FILE to the unmodified value of UBOOT_ZYNQMP_PMUFW_PATH as we used to do before d07e6b70, then mangles it only in the special "*.elf" case. Thank you for catching this issue. I agree with Luca that this would be a better solution to keep the modification specific to only the special "*.elf" case. I have nothing further to offer as Luca already provided a very complete review. I will mark this patch as reviewed when Luca's feedback has been taken into account. Thank you for your support! Best regards, Neal Frager AMD
diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk index 0439ec5e4b..b86248e1f7 100644 --- a/boot/uboot/uboot.mk +++ b/boot/uboot/uboot.mk @@ -383,6 +383,7 @@ endef ifeq ($(BR2_TARGET_UBOOT_ZYNQMP),y) UBOOT_ZYNQMP_PMUFW = $(call qstrip,$(BR2_TARGET_UBOOT_ZYNQMP_PMUFW)) +ifneq ($(UBOOT_ZYNQMP_PMUFW),) ifneq ($(findstring ://,$(UBOOT_ZYNQMP_PMUFW)),) UBOOT_EXTRA_DOWNLOADS += $(UBOOT_ZYNQMP_PMUFW) @@ -398,6 +399,7 @@ define UBOOT_ZYNQMP_KCONFIG_PMUFW objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_BASENAME).elf $(UBOOT_ZYNQMP_PMUFW_BASENAME).bin) $(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_BASENAME).bin") endef +endif # UBOOT_ZYNQMP_PMUFW UBOOT_ZYNQMP_PM_CFG = $(call qstrip,$(BR2_TARGET_UBOOT_ZYNQMP_PM_CFG)) ifneq ($(UBOOT_ZYNQMP_PM_CFG),)
Commit d07e6b70 (boot/uboot/uboot.mk: add pmufw.elf support) broke configurations where the UBOOT_ZYNQMP_PMUFW was blank. Previously it would set the U-Boot CONFIG_PMUFW_INIT_FILE to the blank string, but now it will set it to ".bin" which causes U-Boot to fail to build. To fix, disable all the pmufw code if the UBOOT_ZYNQMP_PMUFW is blank. Signed-off-by: Brandon Maier <brandon.maier@collins.com> --- boot/uboot/uboot.mk | 2 ++ 1 file changed, 2 insertions(+)