[Buildroot] [PATCH] boot/uboot: fix dependency on xilinx-prebuilt

[Buildroot] [PATCH] boot/uboot: fix dependency on xilinx-prebuilt

[Brandon Maier via buildroot]

When building with `make -j` and PER_PACKAGE_DIRECTORIES, a race
condition occurs between xilinx-prebuilt and u-boot, resulting in the
following error.

  objcopy -O binary -I elf32-little .../images/pmufw.elf .../images/pmufw.bin
  objcopy: '.../images/pmufw.elf': No such file

U-Boot registers a KCONFIG_FIXUP_CMD that uses the pmufw.elf from
xilinx-prebuilt. But KCONFIG_FIXUP_CMDS does not use the normal
DEPENDENCIES, so xilinx-prebuilt is not guaranteed to have run before
fixup.

Instead move the `objcopy` call out to a PRE_BUILD_HOOK so it will run
after xilinx-prebuilt has finished.

Signed-off-by: Brandon Maier <brandon.maier@collins.com>
---
 boot/uboot/uboot.mk | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
index 45ad1c880f..cdb9f435f7 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -460,13 +460,17 @@ endif #ifneq ($(findstring ://,$(UBOOT_ZYNQMP_PMUFW)),)
 
 endif #BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT
 
-UBOOT_ZYNQMP_PMUFW_BASENAME = $(basename $(UBOOT_ZYNQMP_PMUFW_PATH))
-
+ifeq ($(suffix $(UBOOT_ZYNQMP_PMUFW_PATH)),.elf)
+UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(basename $(UBOOT_ZYNQMP_PMUFW_PATH)).bin
+define UBOOT_ZYNQMP_PMUFW_CONVERT
+	objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_PATH) $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)
+endef
+UBOOT_PRE_BUILD_HOOKS += UBOOT_ZYNQMP_PMUFW_CONVERT
+else
+UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(UBOOT_ZYNQMP_PMUFW_PATH)
+endif
 define UBOOT_ZYNQMP_KCONFIG_PMUFW
-	$(if $(filter %.elf,$(UBOOT_ZYNQMP_PMUFW_PATH)),
-		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"),
-		$(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH)"))
+	$(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)")
 endef
 
 UBOOT_ZYNQMP_PM_CFG = $(call qstrip,$(BR2_TARGET_UBOOT_ZYNQMP_PM_CFG))

---
base-commit: 038af921dc90d54a0bf5dabad8b03327cfe44247
change-id: 20240809-boot-uboot-fix-dependency-on-xilinx-prebuilt-63f18e883df7

Best regards,
-- 
Brandon Maier <brandon.maier@collins.com>

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

Re: [Buildroot] [PATCH] boot/uboot: fix dependency on xilinx-prebuilt

[Frager, Neal via buildroot]

Hi Brandon,

> When building with `make -j` and PER_PACKAGE_DIRECTORIES, a race
> condition occurs between xilinx-prebuilt and u-boot, resulting in the
> following error.

>  objcopy -O binary -I elf32-little .../images/pmufw.elf .../images/pmufw.bin
>  objcopy: '.../images/pmufw.elf': No such file

> U-Boot registers a KCONFIG_FIXUP_CMD that uses the pmufw.elf from
> xilinx-prebuilt. But KCONFIG_FIXUP_CMDS does not use the normal
> DEPENDENCIES, so xilinx-prebuilt is not guaranteed to have run before
> fixup.

> Instead move the `objcopy` call out to a PRE_BUILD_HOOK so it will run
> after xilinx-prebuilt has finished.

> Signed-off-by: Brandon Maier <brandon.maier@collins.com>

Thank you for looking into this.  I am adding Luca on copy as he developed
the uboot.mk code you are modifying.

Shouldn't line 448 of u-boot.mk prevent this issue?
UBOOT_DEPENDENCIES += xilinx-prebuilt

Do you have the following config when you see this error?
BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT

Best regards,
Neal Frager
AMD
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH] boot/uboot: fix dependency on xilinx-prebuilt

[Brandon Maier via buildroot]

Hi Neal,

On Sat Aug 10, 2024 at 7:01 AM UTC, Frager, Neal via buildroot wrote:
> Hi Brandon,
>
> > When building with `make -j` and PER_PACKAGE_DIRECTORIES, a race
> > condition occurs between xilinx-prebuilt and u-boot, resulting in the
> > following error.
>
> >  objcopy -O binary -I elf32-little .../images/pmufw.elf .../images/pmufw.bin
> >  objcopy: '.../images/pmufw.elf': No such file
>
> > U-Boot registers a KCONFIG_FIXUP_CMD that uses the pmufw.elf from
> > xilinx-prebuilt. But KCONFIG_FIXUP_CMDS does not use the normal
> > DEPENDENCIES, so xilinx-prebuilt is not guaranteed to have run before
> > fixup.
>
> > Instead move the `objcopy` call out to a PRE_BUILD_HOOK so it will run
> > after xilinx-prebuilt has finished.
>
> > Signed-off-by: Brandon Maier <brandon.maier@collins.com>
>
> Thank you for looking into this.  I am adding Luca on copy as he developed
> the uboot.mk code you are modifying.
>
> Shouldn't line 448 of u-boot.mk prevent this issue?
> UBOOT_DEPENDENCIES += xilinx-prebuilt

It doesn't, the dependency graph looks something like this

  UBOOT_KCONFIG_DEPENDENCIES -> uboot-kconfig
  uboot-kconfig -> uboot-configure
  UBOOT_DEPENDENCIES -> uboot-configure

The `objcopy` call is happening during `uboot-kconfig`, so
UBOOT_DEPENDENCIES is not a prerequisite.

There is a UBOOT_KCONFIG_DEPENDENCIES, but it seemed more appropriate
that the `objcopy` should be a build step.

>
> Do you have the following config when you see this error?
> BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT

Yes, this happened when I was building configs/zynqmp_zcu106_defconfig
but with PER_PACKAGE_DIRECTORIES enabled, to speed up build with.

>
> Best regards,
> Neal Frager
> AMD
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH] boot/uboot: fix dependency on xilinx-prebuilt

[Frager, Neal via buildroot]

Hi Brandon,

> Hi Brandon,
>
> > When building with `make -j` and PER_PACKAGE_DIRECTORIES, a race
> > condition occurs between xilinx-prebuilt and u-boot, resulting in the
> > following error.
>
> >  objcopy -O binary -I elf32-little .../images/pmufw.elf .../images/pmufw.bin
> >  objcopy: '.../images/pmufw.elf': No such file
>
> > U-Boot registers a KCONFIG_FIXUP_CMD that uses the pmufw.elf from
> > xilinx-prebuilt. But KCONFIG_FIXUP_CMDS does not use the normal
> > DEPENDENCIES, so xilinx-prebuilt is not guaranteed to have run before
> > fixup.
>
> > Instead move the `objcopy` call out to a PRE_BUILD_HOOK so it will run
> > after xilinx-prebuilt has finished.
>
> > Signed-off-by: Brandon Maier <brandon.maier@collins.com>
>
> Thank you for looking into this.  I am adding Luca on copy as he developed
> the uboot.mk code you are modifying.
>
> Shouldn't line 448 of u-boot.mk prevent this issue?
> UBOOT_DEPENDENCIES += xilinx-prebuilt

> It doesn't, the dependency graph looks something like this

>  UBOOT_KCONFIG_DEPENDENCIES -> uboot-kconfig
>  uboot-kconfig -> uboot-configure
>  UBOOT_DEPENDENCIES -> uboot-configure

> The `objcopy` call is happening during `uboot-kconfig`, so
> UBOOT_DEPENDENCIES is not a prerequisite.

Thanks for clarifying this.  The issue is clear now.

> There is a UBOOT_KCONFIG_DEPENDENCIES, but it seemed more appropriate
> that the `objcopy` should be a build step.

Yes, I see.  objcopy is now a pre-build step.

>
> Do you have the following config when you see this error?
> BR2_TARGET_UBOOT_ZYNQMP_PMUFW_PREBUILT

> Yes, this happened when I was building configs/zynqmp_zcu106_defconfig
> but with PER_PACKAGE_DIRECTORIES enabled, to speed up build with.

Thanks for catching this issue!  I was completely unaware.

I boot tested a zcu102 and kv260 and all looks good, thanks!

Tested-by: Neal Frager <neal.frager@amd.com>
Reviewed-by: Neal Frager <neal.frager@amd.com>

Best regards,
Neal Frager
AMD
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH] boot/uboot: fix dependency on xilinx-prebuilt

[Thomas Petazzoni via buildroot]

Hello Brandon,

On Fri, 09 Aug 2024 16:30:18 +0000
Brandon Maier via buildroot <buildroot@buildroot.org> wrote:

> When building with `make -j` and PER_PACKAGE_DIRECTORIES, a race
> condition occurs between xilinx-prebuilt and u-boot, resulting in the
> following error.
> 
>   objcopy -O binary -I elf32-little .../images/pmufw.elf .../images/pmufw.bin
>   objcopy: '.../images/pmufw.elf': No such file
> 
> U-Boot registers a KCONFIG_FIXUP_CMD that uses the pmufw.elf from
> xilinx-prebuilt. But KCONFIG_FIXUP_CMDS does not use the normal
> DEPENDENCIES, so xilinx-prebuilt is not guaranteed to have run before
> fixup.
> 
> Instead move the `objcopy` call out to a PRE_BUILD_HOOK so it will run
> after xilinx-prebuilt has finished.
> 
> Signed-off-by: Brandon Maier <brandon.maier@collins.com>

Thanks for fixing this, I have applied to master. I have a few
questions/suggestions below.

> +ifeq ($(suffix $(UBOOT_ZYNQMP_PMUFW_PATH)),.elf)
> +UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(basename $(UBOOT_ZYNQMP_PMUFW_PATH)).bin
> +define UBOOT_ZYNQMP_PMUFW_CONVERT
> +	objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_PATH) $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)

Should we be using $(TARGET_OBJCOPY) instead of objcopy to use the
target toolchain objcopy?

> +endef
> +UBOOT_PRE_BUILD_HOOKS += UBOOT_ZYNQMP_PMUFW_CONVERT
> +else
> +UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(UBOOT_ZYNQMP_PMUFW_PATH)
> +endif
>  define UBOOT_ZYNQMP_KCONFIG_PMUFW
> -	$(if $(filter %.elf,$(UBOOT_ZYNQMP_PMUFW_PATH)),
> -		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"),
> -		$(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH)"))
> +	$(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)")

This means this KCONFIG_SET_OPT is not unconditionally if I'm not
wrong (so even on platforms that are not Xilinx ones). Shouldn't it
only be done if $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL) is not empty?

Best regards,

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH] boot/uboot: fix dependency on xilinx-prebuilt

[Frager, Neal via buildroot]

[-- Attachment #1.1: Type: text/plain, Size: 2228 bytes --]

Hi Thomas,

> When building with `make -j` and PER_PACKAGE_DIRECTORIES, a race
> condition occurs between xilinx-prebuilt and u-boot, resulting in the
> following error.
>
>   objcopy -O binary -I elf32-little .../images/pmufw.elf .../images/pmufw.bin
>   objcopy: '.../images/pmufw.elf': No such file
>
> U-Boot registers a KCONFIG_FIXUP_CMD that uses the pmufw.elf from
> xilinx-prebuilt. But KCONFIG_FIXUP_CMDS does not use the normal
> DEPENDENCIES, so xilinx-prebuilt is not guaranteed to have run before
> fixup.
>
> Instead move the `objcopy` call out to a PRE_BUILD_HOOK so it will run
> after xilinx-prebuilt has finished.
>
> Signed-off-by: Brandon Maier <brandon.maier@collins.com>

> Thanks for fixing this, I have applied to master. I have a few
> questions/suggestions below.

> +ifeq ($(suffix $(UBOOT_ZYNQMP_PMUFW_PATH)),.elf)
> +UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(basename $(UBOOT_ZYNQMP_PMUFW_PATH)).bin
> +define UBOOT_ZYNQMP_PMUFW_CONVERT
> +     objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_PATH) $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)

> Should we be using $(TARGET_OBJCOPY) instead of objcopy to use the
> target toolchain objcopy?

$(TARGET_OBJCOPY) would still be wrong. The pmufw.elf is a microblaze image.

> +endef
> +UBOOT_PRE_BUILD_HOOKS += UBOOT_ZYNQMP_PMUFW_CONVERT
> +else
> +UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(UBOOT_ZYNQMP_PMUFW_PATH)
> +endif
>  define UBOOT_ZYNQMP_KCONFIG_PMUFW
> -     $(if $(filter %.elf,$(UBOOT_ZYNQMP_PMUFW_PATH)),
> -             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"),
> -             $(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH)"))
> +     $(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)")

> This means this KCONFIG_SET_OPT is not unconditionally if I'm not
> wrong (so even on platforms that are not Xilinx ones). Shouldn't it
> only be done if $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL) is not empty?

I have to think about this, but maybe Brandon has an answer.

Best regards,
Neal Frager
AMD

[-- Attachment #1.2: Type: text/html, Size: 3299 bytes --]

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

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

Re: [Buildroot] [PATCH] boot/uboot: fix dependency on xilinx-prebuilt

[Brandon Maier via buildroot]

Hi Thomas,

On Thu Aug 15, 2024 at 9:14 AM UTC, Thomas Petazzoni via buildroot wrote:
> Hello Brandon,
>
> On Fri, 09 Aug 2024 16:30:18 +0000
> Brandon Maier via buildroot <buildroot@buildroot.org> wrote:
>
> > When building with `make -j` and PER_PACKAGE_DIRECTORIES, a race
> > condition occurs between xilinx-prebuilt and u-boot, resulting in the
> > following error.
> >
> >   objcopy -O binary -I elf32-little .../images/pmufw.elf .../images/pmufw.bin
> >   objcopy: '.../images/pmufw.elf': No such file
> >
> > U-Boot registers a KCONFIG_FIXUP_CMD that uses the pmufw.elf from
> > xilinx-prebuilt. But KCONFIG_FIXUP_CMDS does not use the normal
> > DEPENDENCIES, so xilinx-prebuilt is not guaranteed to have run before
> > fixup.
> >
> > Instead move the `objcopy` call out to a PRE_BUILD_HOOK so it will run
> > after xilinx-prebuilt has finished.
> >
> > Signed-off-by: Brandon Maier <brandon.maier@collins.com>
>
> Thanks for fixing this, I have applied to master. I have a few
> questions/suggestions below.
>
> > +ifeq ($(suffix $(UBOOT_ZYNQMP_PMUFW_PATH)),.elf)
> > +UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(basename $(UBOOT_ZYNQMP_PMUFW_PATH)).bin
> > +define UBOOT_ZYNQMP_PMUFW_CONVERT
> > +	objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_PATH) $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)
>
> Should we be using $(TARGET_OBJCOPY) instead of objcopy to use the
> target toolchain objcopy?

I'm not certain, as Neal pointed out the PMU.elf is a microblaze. I'm
guessing this works today as `objcopy` probably doesn't need to support
the target architecture to convert an ELF to a binary.

If that is true, then maybe we should use $(TARGET_OBJCOPY) anyway. So
we aren't relying on the build system's version of `objcopy`.

>
> > +endef
> > +UBOOT_PRE_BUILD_HOOKS += UBOOT_ZYNQMP_PMUFW_CONVERT
> > +else
> > +UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(UBOOT_ZYNQMP_PMUFW_PATH)
> > +endif
> >  define UBOOT_ZYNQMP_KCONFIG_PMUFW
> > -	$(if $(filter %.elf,$(UBOOT_ZYNQMP_PMUFW_PATH)),
> > -		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"),
> > -		$(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH)"))
> > +	$(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)")
>
> This means this KCONFIG_SET_OPT is not unconditionally if I'm not
> wrong (so even on platforms that are not Xilinx ones). Shouldn't it
> only be done if $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL) is not empty?

This whole section is inside a `ifeq ($(BR2_TARGET_UBOOT_ZYNQMP),y)`
block, so it will only run on Xilinx ZynqMP platforms. However it's
possible to set the PMUFW to an empty string and then it will set
CONFIG_PMUFW_INIT_FILE to an empty string.

If a developer set CONFIG_PMUFW_INIT_FILE in a custom U-Boot defconfig
or frag, then this could overwrite it. Though I'm not sure if there's
any practical use-case where this is an issue.

I can send a patch to add the empty string check though, it seems
harmless to add.

Thanks,
Brandon Maier

>
> Best regards,
>
> Thomas
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH] boot/uboot: fix dependency on xilinx-prebuilt

[Luca Ceresoli via buildroot]

Hello Brandon,

On Thu, 15 Aug 2024 13:58:07 +0000
Brandon Maier via buildroot <buildroot@buildroot.org> wrote:

> Hi Thomas,
> 
> On Thu Aug 15, 2024 at 9:14 AM UTC, Thomas Petazzoni via buildroot wrote:
> > Hello Brandon,
> >
> > On Fri, 09 Aug 2024 16:30:18 +0000
> > Brandon Maier via buildroot <buildroot@buildroot.org> wrote:
> >  
> > > When building with `make -j` and PER_PACKAGE_DIRECTORIES, a race
> > > condition occurs between xilinx-prebuilt and u-boot, resulting in the
> > > following error.
> > >
> > >   objcopy -O binary -I elf32-little .../images/pmufw.elf .../images/pmufw.bin
> > >   objcopy: '.../images/pmufw.elf': No such file
> > >
> > > U-Boot registers a KCONFIG_FIXUP_CMD that uses the pmufw.elf from
> > > xilinx-prebuilt. But KCONFIG_FIXUP_CMDS does not use the normal
> > > DEPENDENCIES, so xilinx-prebuilt is not guaranteed to have run before
> > > fixup.
> > >
> > > Instead move the `objcopy` call out to a PRE_BUILD_HOOK so it will run
> > > after xilinx-prebuilt has finished.

Thanks for having fixed this! I apologize for not having reviewed this
earlier -- vacation time + inbox full :(

> > > +ifeq ($(suffix $(UBOOT_ZYNQMP_PMUFW_PATH)),.elf)
> > > +UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(basename $(UBOOT_ZYNQMP_PMUFW_PATH)).bin
> > > +define UBOOT_ZYNQMP_PMUFW_CONVERT
> > > +	objcopy -O binary -I elf32-little $(UBOOT_ZYNQMP_PMUFW_PATH) $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)  
> >
> > Should we be using $(TARGET_OBJCOPY) instead of objcopy to use the
> > target toolchain objcopy?  
> 
> I'm not certain, as Neal pointed out the PMU.elf is a microblaze. I'm
> guessing this works today as `objcopy` probably doesn't need to support
> the target architecture to convert an ELF to a binary.
> 
> If that is true, then maybe we should use $(TARGET_OBJCOPY) anyway. So
> we aren't relying on the build system's version of `objcopy`.

While both are "wrong but working", I agree using $(TARGET_OBJCOPY)
would make the process more reproducible and predictable. While there,
adding an explanatory comment would be a good idea.

> > > +endef
> > > +UBOOT_PRE_BUILD_HOOKS += UBOOT_ZYNQMP_PMUFW_CONVERT
> > > +else
> > > +UBOOT_ZYNQMP_PMUFW_PATH_FINAL = $(UBOOT_ZYNQMP_PMUFW_PATH)
> > > +endif
> > >  define UBOOT_ZYNQMP_KCONFIG_PMUFW
> > > -	$(if $(filter %.elf,$(UBOOT_ZYNQMP_PMUFW_PATH)),
> > > -		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"),
> > > -		$(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH)"))
> > > +	$(call KCONFIG_SET_OPT,CONFIG_PMUFW_INIT_FILE,"$(UBOOT_ZYNQMP_PMUFW_PATH_FINAL)")  
> >
> > This means this KCONFIG_SET_OPT is not unconditionally if I'm not
> > wrong (so even on platforms that are not Xilinx ones). Shouldn't it
> > only be done if $(UBOOT_ZYNQMP_PMUFW_PATH_FINAL) is not empty?  
> 
> This whole section is inside a `ifeq ($(BR2_TARGET_UBOOT_ZYNQMP),y)`
> block, so it will only run on Xilinx ZynqMP platforms. However it's
> possible to set the PMUFW to an empty string and then it will set
> CONFIG_PMUFW_INIT_FILE to an empty string.
> 
> If a developer set CONFIG_PMUFW_INIT_FILE in a custom U-Boot defconfig
> or frag, then this could overwrite it. Though I'm not sure if there's
> any practical use-case where this is an issue.
> 
> I can send a patch to add the empty string check though, it seems
> harmless to add.

By a cursory look at the U-Boot code, it looks like the U-Boot build
would fail. If that's the case, I don't think we need an extra check
here: empty string would be just one of all possible incorrect settings
that just cause a build failure.

IOW, I'd only add a check if it can cause subtle issues that are
hard to notice and debug, like a "successful" build (returning zero)
producing non-working binaries.

Luca

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