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

[Thomas Petazzoni]

Hello,

Commit title should be:

	package/versal-firmware: new package

On Mon, 24 Oct 2022 08:22:14 -0600
Neal Frager <neal.frager at amd.com> wrote:


> 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.

> 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.

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?

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.



> +	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.

> +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_VERSION))
> +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.pdi

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)
	)

Thanks!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com