Re: [Buildroot] [PATCH 1/1] boot/uboot/uboot.mk: fix zynqmp without pmufw

[Luca Ceresoli via buildroot <buildroot@buildroot.org>]

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?

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot