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

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

[Brandon Maier via buildroot]

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(+)

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),)
-- 
2.38.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Luca Ceresoli via buildroot]

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

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

[Frager, Neal via buildroot]

[-- Attachment #1: Type: text/plain, Size: 2998 bytes --]

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

[-- Attachment #2: winmail.dat --]
[-- Type: application/ms-tnef, Size: 15006 bytes --]

[-- Attachment #3: Type: text/plain, Size: 150 bytes --]

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot