[Buildroot] [PATCH v1 1/3] add package/versal-firmware

[Frager, Neal]

Hi Thomas,

> Commit title should be:

> 	package/versal-firmware: new package

Ok.  No problem.

> diff --git a/DEVELOPERS b/DEVELOPERS
> index c8183b2290..ed696f4cd0 100644
> --- a/DEVELOPERS
> +++ b/DEVELOPERS
> @@ -2176,6 +2176,7 @@ F:	configs/zynqmp_zcu102_defconfig
>  F:	configs/zynqmp_zcu106_defconfig
>  F:	configs/zynqmp_kria_kv260_defconfig
>  F:	package/bootgen
> +F:	package/versal-firmware

> Final / needed here.

I saw this as well, and have fixed it for v2.

> diff --git a/package/versal-firmware/Config.in 
> b/package/versal-firmware/Config.in
> new file mode 100644
> index 0000000000..e184ba1925
> --- /dev/null
> +++ b/package/versal-firmware/Config.in
> @@ -0,0 +1,22 @@
> +config BR2_PACKAGE_VERSAL_FIRMWARE
> +	bool "versal-firmware"
> +	depends on BR2_aarch64
> +	help
> +	  Pre-built firmware files for Xilinx Versal boards.
> +
> +	  https://github.com/nealfrager/buildroot-firmware
> +
> +if BR2_PACKAGE_VERSAL_FIRMWARE
> +
> +config BR2_PACKAGE_VERSAL_FIRMWARE_VERSION
> +	string "versal firmware version"
> +	help
> +	  Release version of Versal firmware.

> Does it make sense to have the version configurable? If the version is configurable, then probably the location (Git repository) should also be configurable.

I can imagine users may not want to always update the boot firmware version.  
While I plan to keep the versal defconfig examples on the latest version available, I expect many users may want to build with older versions available on the repo.

We could make the repo location configurable as well, but normally, it should be coming from the Xilinx official github source which will be available soon.
My plan is to migrate this package to the Xilinx firmware repo as soon as it is available.

> How do we expect this to be used by people who design their own boards based on this SoC? Will they fork this repository? Do they have the source code to create their own firmware?

> Should the version have a default value?

> +config BR2_PACKAGE_VERSAL_FIRMWARE_BOARD
> +	string "versal board name"

> Should the board have a default value?

Yes, this is probably a good idea to make sure something gets picked up in case not specified.
For now, I will set the default to vck190 in the next update.

> Think of what happens when people just enable this package, without using a specific Buildroot defconfig. In particular, our autobuilders generate random Buildroot configurations, so they should build.

Ok.  Clear point.  I will set vck190 to the default, so that it always builds even if not specified.

> +	help
> +	  Name of Versal target board.
> +	  Used for installing the appropriate firmware boot.bin.
> +
> +endif # BR2_PACKAGE_VERSAL_FIRMWARE
> diff --git a/package/versal-firmware/versal-firmware.hash 
> b/package/versal-firmware/versal-firmware.hash
> new file mode 100644
> index 0000000000..7a8ea04c91
> --- /dev/null
> +++ b/package/versal-firmware/versal-firmware.hash
> @@ -0,0 +1,3 @@
> +# Locally calculated
> +sha256  
> +14c505cac0216637ab2d08590acffb4861446e480bdbf7417e699816048ab39b  
> +versal-firmware-v2022.2.tar.gz

> Having this hash value is a bit useless when the version is configurable.

Ok, I will remove the hash file, since the version is configurable.
I will also set the default version to latest available.

> +sha256  
> +f9796efcd98f47fb3e1d5d4c23e224613e91c70207b759a2087de368be50c315  
> +LICENSE
> diff --git a/package/versal-firmware/versal-firmware.mk 
> b/package/versal-firmware/versal-firmware.mk
> new file mode 100644
> index 0000000000..35dfbaa512
> --- /dev/null
> +++ b/package/versal-firmware/versal-firmware.mk
> @@ -0,0 +1,24 @@
> +#####################################################################
> +###########
> +#
> +# versal-firmware
> +#
> +#####################################################################
> +###########
> +
> +VERSAL_FIRMWARE_VERSION = $(call 
> +qstrip,$(BR2_PACKAGE_VERSAL_FIRMWARE_VERSION))
> +VERSAL_FIRMWARE_SITE = $(call 
> +github,nealfrager,buildroot-firmware,$(BR2_PACKAGE_VERSAL_FIRMWARE_VE
> +RSION)) VERSAL_FIRMWARE_LICENSE = Xilinx-Binary-Only 
> +VERSAL_FIRMWARE_LICENSE_FILES = LICENSE
> +
> +VERSAL_FIRMWARE_INSTALL_TARGET = NO
> +VERSAL_FIRMWARE_INSTALL_IMAGES = YES
> +
> +define VERSAL_FIRMWARE_INSTALL_IMAGES_CMDS
> +	$(INSTALL) -D -m 0755 $(@D)/$(BR2_PACKAGE_VERSAL_FIRMWARE_BOARD)/$(BR2_PACKAGE_VERSAL_FIRMWARE_BOARD)_plm.elf \
> +	$(BINARIES_DIR)/$(BR2_PACKAGE_VERSAL_FIRMWARE_BOARD)_plm.elf
> +	$(INSTALL) -D -m 0755 $(@D)/$(BR2_PACKAGE_VERSAL_FIRMWARE_BOARD)/$(BR2_PACKAGE_VERSAL_FIRMWARE_BOARD)_psmfw.elf \
> +	$(BINARIES_DIR)/$(BR2_PACKAGE_VERSAL_FIRMWARE_BOARD)_psmfw.elf
> +	$(INSTALL) -D -m 0755 $(@D)/$(BR2_PACKAGE_VERSAL_FIRMWARE_BOARD)/$(BR2_PACKAGE_VERSAL_FIRMWARE_BOARD)_vpl_gen_fixed.pdi \
> +	
> +$(BINARIES_DIR)/$(BR2_PACKAGE_VERSAL_FIRMWARE_BOARD)_vpl_gen_fixed.pd
> +i

> Please indent the continuation line. But maybe this could be handle in a slightly better way:

>	$(foreach f,plm.elf psmfw.elf vpl_gen_fixed.pdi,\
> 		$(INSTALL) -D -m 0755 $(@D)/$(BR2_PACKAGE_VERSAL_FIRMWARE_BOARD)/$(BR2_PACKAGE_VERSAL_FIRMWARE_BOARD)_$(f) \
>			$(BINARIES_DIR)/$(BR2_PACKAGE_VERSAL_FIRMWARE_BOARD)_$(f)
>	)

Cool.  Thank you for this!  Looks much nicer this way.

Best regards,
Neal Frager
AMD