Re: [Buildroot] [PATCH v2 1/3] boot/xilinx-prebuilt: add segmented config support

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

Hello Neal,

On Mon, 7 Apr 2025 16:29:09 +0100
Neal Frager <neal.frager@amd.com> wrote:

> AMD has created a new segmented configuration for Versal products.  It splits
> the Vivado hardware design into two PDI files, one containing the minimal
> required configuration for the DDR and booting the processors, and a second PDI
> file which contains the rest of the FPGA design and can be loaded at run-time
> via U-boot or Linux.
> 
> The file names generated when using the Vivado Segmented Configuration are as
> follows:
> 
> <design>_boot.pdi - Minimal DDR and PS config for booting
> <design>_pld.pdi - Remainder of FPGA design to be loaded at run-time
> 
> Since two PDI files will be included in the XSA file when using Segmented
> Configuration, the xilinx-prebuilt package needs to be updated to support
> this feature.
> 
> For Buildroot purposes, the <design>_boot.pdi is the file that needs to be
> included in the boot.bin for booting the processors, so this patch checks
> for a file named *boot*.pdi which will indicate that Segmented Configuration
> is being used and will make sure to use the <design>_boot.pdi file and not
> the <design>_pld.pdi file when generating the boot.bin image.
> 
> If no pdi files contain the word "boot" in the filename, it can be assumed
> that Segmented Configuration is not being used, so the single file *.pdi
> method is the appropriate one for this case, and this patch is thus
> backwards compatible with prior designs not using Segmented Configuration.
> 
> Also, Segmented Configuration is going to become the default mode for Versal
> products, so the xilinx-prebuilt github location will also soon have two PDI
> files for each board.  For this reason, this patch is also handling Segmented
> Configuration for files downloaded from the xilinx-prebuilt repo.
> 
> For further information about the AMD Segmented Configuration, please see the
> github tutorial below.
> 
> https://github.com/Xilinx/Vivado-Design-Tutorials/tree/2024.2/Versal/Boot_and_Config/Segmented_Configuration
> 
> Signed-off-by: Neal Frager <neal.frager@amd.com>

Thank you for the very good explanation, it's very helpful!

> ---
> V1->V2:
> - $(@D) is not available in time for wildcard evaluation outside of shell
>   script, so moved the wildcard functions into the install scripts
> ---
>  boot/xilinx-prebuilt/xilinx-prebuilt.mk | 36 ++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/boot/xilinx-prebuilt/xilinx-prebuilt.mk b/boot/xilinx-prebuilt/xilinx-prebuilt.mk
> index d0817d3cc7..448c71a3fb 100644
> --- a/boot/xilinx-prebuilt/xilinx-prebuilt.mk
> +++ b/boot/xilinx-prebuilt/xilinx-prebuilt.mk
> @@ -28,9 +28,6 @@ XILINX_PREBUILT_BOARD = $(call qstrip,$(BR2_TARGET_XILINX_PREBUILT_BOARD))
>  XILINX_PREBUILT_BOARD_DIR = $(@D)/$(XILINX_PREBUILT_BOARD)-$(XILINX_PREBUILT_FAMILY)
>  
>  ifeq ($(BR2_TARGET_XILINX_PREBUILT_VERSAL),y)
> -# We need the *.pdi glob, because the file has different names for the
> -# different boards, but there is only one, and it has to be named
> -# vpl_gen_fixed.pdi when installed.
>  ifeq ($(BR2_TARGET_XILINX_PREBUILT_VERSAL_XSA),y)
>  XILINX_PREBUILT_PLM = $(@D)/pdi_files/gen_files/plm.elf
>  # Unlike the psmfw.elf file for Xilinx development boards,
> @@ -39,11 +36,33 @@ XILINX_PREBUILT_PLM = $(@D)/pdi_files/gen_files/plm.elf
>  # so to support current and future AMD Vivado versions, the filename
>  # psm*fw.elf is used.
>  XILINX_PREBUILT_PSMFW = $(@D)/pdi_files/static_files/psm*fw.elf
> -XILINX_PREBUILT_PDI = $(@D)/*.pdi
> +# We need the *.pdi glob, because the file has different names for the
> +# different boards, and it has to be named vpl_gen_fixed.pdi when installed.
> +# If Segmented Configuration is used, there will be two pdi files and we need
> +# the file that has "boot" in the filename.
> +define XILINX_PREBUILT_INSTALL_VERSAL_XSA_BOOT_PDI
> +	$(if $(wildcard $(@D)/*boot*.pdi),

According to your commit message, a more specific wildcard can be used:
"$(@D)/*_boot.pdi".

> +		$(INSTALL) -D -m 0755 $(@D)/*boot*.pdi \
> +			$(BINARIES_DIR)/vpl_gen_fixed.pdi,
> +		$(INSTALL) -D -m 0755 $(@D)/*.pdi \
> +			$(BINARIES_DIR)/vpl_gen_fixed.pdi
> +	)

It's very unfortunate that you are adding all this code, and in two
places (here and below). The code in v1 was looking a lot nicer, but
I'm not sure there is a way to make it work. If there is none, I think
you can at least improve by using the $(if) only for the filename, not
the whole command, as in this example (untested):

define XILINX_PREBUILT_INSTALL_VERSAL_XSA_BOOT_PDI
	$(INSTALL) -D -m 0755 \
		$(if $(wildcard $(@D)/*_boot.pdi), $(@D)/*_boot.pdi, $(@D)/*.pdi) \
		$(BINARIES_DIR)/vpl_gen_fixed.pdi
#endef

Also, do you need 0755 permissions for this file? Is that an
executable? If it isn't, that should be fixed as a preliminary patch.

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