Re: [Buildroot] [PATCH v3 2/4] boot/xilinx-prebuilt: wire up xilinx-source

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

On Tue, 18 Jun 2024 08:49:20 +0100
Neal Frager <neal.frager@amd.com> wrote:

> This patch wires up the xilinx-prebuilt with xilinx-source.
> 
> Signed-off-by: Neal Frager <neal.frager@amd.com>
> ---
> V1->V3:
> - no changes
> ---
>  boot/xilinx-prebuilt/Config.in          |  3 +++
>  boot/xilinx-prebuilt/xilinx-prebuilt.mk | 13 ++++++++++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/boot/xilinx-prebuilt/Config.in b/boot/xilinx-prebuilt/Config.in
> index d3df1c06e7..34b89337fa 100644
> --- a/boot/xilinx-prebuilt/Config.in
> +++ b/boot/xilinx-prebuilt/Config.in
> @@ -22,12 +22,15 @@ choice
>  
>  config BR2_TARGET_XILINX_PREBUILT_ZYNQMP
>  	bool "zynqmp"
> +	depends on !BR2_TARGET_XILINX_SOURCE
>  
>  config BR2_TARGET_XILINX_PREBUILT_KRIA
>  	bool "kria"
> +	depends on !BR2_TARGET_XILINX_SOURCE
>  
>  config BR2_TARGET_XILINX_PREBUILT_VERSAL
>  	bool "versal"
> +	depends on BR2_PACKAGE_HOST_BOOTGEN

Is this bootgen line unrelated to this series? It appears as something
that was needed before and it is being fixed now. If it is the case,
then it should be a separate patch.

Also, I don't like very much how the "family variant" will look like if
we apply this patch. Now if enabling both xilinx-prebuilt and
xilinx-source we will have one "family variant" choice for each. And if
host-bootgen is enabled, the one for xilinx-prebuilt has no selectable
options, which looks very weird.

I'm not sure what is the best thing to do here, but it seems wrong to
have two selectable variants where having different values does not
make sense. What about moving the "family variant" choice before the
two packages and use it for both?

> diff --git a/boot/xilinx-prebuilt/xilinx-prebuilt.mk b/boot/xilinx-prebuilt/xilinx-prebuilt.mk
> index eb78ed10cd..4cb3e2a33e 100644
> --- a/boot/xilinx-prebuilt/xilinx-prebuilt.mk
> +++ b/boot/xilinx-prebuilt/xilinx-prebuilt.mk
> @@ -18,8 +18,17 @@ XILINX_PREBUILT_BOARD_DIR = $(@D)/$(XILINX_PREBUILT_BOARD)-$(XILINX_PREBUILT_FAM
>  
>  ifeq ($(BR2_TARGET_XILINX_PREBUILT_VERSAL),y)
>  # We need the *.pdi glob, because the file has different names for the
> -# different boards, but htere is only one, and it has to be named
> +# different boards, but there is only one, and it has to be named
>  # vpl_gen_fixed.pdi when installed.
> +
> +ifeq ($(BR2_TARGET_XILINX_SOURCE_VERSAL),y)
> +# The plm.elf and psmfw.elf images will come from xilinx-source, so only
> +# the *.pdi needs to be installed.
> +define XILINX_PREBUILT_INSTALL_VERSAL
> +	$(INSTALL) -D -m 0755 $(XILINX_PREBUILT_BOARD_DIR)/*.pdi \
> +		$(BINARIES_DIR)/vpl_gen_fixed.pdi

These two lines are a duplicate of...

> +endef
> +else # BR2_TARGET_XILINX_SOURCE_VERSAL
>  define XILINX_PREBUILT_INSTALL_VERSAL
>  	$(INSTALL) -D -m 0755 $(XILINX_PREBUILT_BOARD_DIR)/plm.elf \
>  		$(BINARIES_DIR)/plm.elf
> @@ -28,6 +37,8 @@ define XILINX_PREBUILT_INSTALL_VERSAL
>  	$(INSTALL) -D -m 0755 $(XILINX_PREBUILT_BOARD_DIR)/*.pdi \
>  		$(BINARIES_DIR)/vpl_gen_fixed.pdi

...these two lines. It's not a big deal, but I'd consider defining once
the lines for the pdi and conditionally the other two lines:

# if BR2_TARGET_XILINX_SOURCE_VERSAL=y the plm.elf and psmfw.elf images
# will come from xilinx-source, so only the *.pdi needs to be installed.
ifeq ($(BR2_TARGET_XILINX_SOURCE_VERSAL),)
define XILINX_PREBUILT_INSTALL_VERSAL_PLM_PSMFW
        $(INSTALL) -D -m 0755 $(XILINX_PREBUILT_BOARD_DIR)/plm.elf \
                $(BINARIES_DIR)/plm.elf
        $(INSTALL) -D -m 0755 $(XILINX_PREBUILT_BOARD_DIR)/psmfw.elf \
                $(BINARIES_DIR)/psmfw.elf
endef
endif # !BR2_TARGET_XILINX_SOURCE_VERSAL

define XILINX_PREBUILT_INSTALL_VERSAL_PDI
        $(INSTALL) -D -m 0755 $(XILINX_PREBUILT_BOARD_DIR)/*.pdi \
                $(BINARIES_DIR)/vpl_gen_fixed.pdi
#endef

...

define XILINX_PREBUILT_INSTALL_IMAGES_CMDS
        $(XILINX_PREBUILT_INSTALL_VERSAL_PLM_PSMFW)
        $(XILINX_PREBUILT_INSTALL_VERSAL_PDI)
        $(XILINX_PREBUILT_INSTALL_OTHERS)
endef

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