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

From: Thomas Petazzoni via buildroot <buildroot@buildroot.org>
To: Brandon Maier via buildroot <buildroot@buildroot.org>
Cc: Brandon Maier <brandon.maier@collins.com>,
	Neal Frager <neal.frager@amd.com>
Subject: Re: [Buildroot] [PATCH] boot/uboot: fix dependency on xilinx-prebuilt
Date: Thu, 15 Aug 2024 11:14:15 +0200	[thread overview]
Message-ID: <20240815111415.25844e3a@windsurf> (raw)
In-Reply-To: <20240809-boot-uboot-fix-dependency-on-xilinx-prebuilt-v1-1-7014697db0c6@collins.com>

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