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

["Yann E. MORIN" <yann.morin.1998@free.fr>]

Neal, All,

On 2022-08-19 00:37 -0600, Neal Frager spake thusly:
> This patch adds support for downloading versal firmware binaries.
> These are necessary for booting Xilinx versal devices.
[--SNIP--]
> diff --git a/package/versal-firmware/Config.in b/package/versal-firmware/Config.in
> new file mode 100644
> index 0000000000..9ca46b1d16
> --- /dev/null
> +++ b/package/versal-firmware/Config.in
> @@ -0,0 +1,32 @@
> +config BR2_PACKAGE_VERSAL_FIRMWARE
> +	bool "versal-firmware"
> +	depends on BR2_aarch64
> +	help
> +	  Versal Firmware
> +
> +	  Pre-built binaries of the current bootloader firmware
> +
> +	  https://github.com/nealfrager/versal_boot

This looks like your personal git tree, not the official one. (Yeah, I
noticed your email address, but I would still have expected something
under https://github.com/Xilinx/).

The official images are there:
    https://xilinx-wiki.atlassian.net/wiki/spaces/A/pages/18842316/Linux+Prebuilt+Images
    https://xilinx-wiki.atlassian.net/wiki/spaces/A/pages/2347204609/2022.1+Release

If one tries to download any resource from there, they are redirected
to a login screen, e.g.:
    https://login.xilinx.com/app/xilinxinc_f5awsprod_1/exknv8ms950lm0Ldh0x7/sso/saml

The licensing information is not clearly detailed in the portal, and
being low-level firmware files, I'm afraid there might be restrictions
on the redistribution og those files...

> +if BR2_PACKAGE_VERSAL_FIRMWARE
> +
> +config BR2_PACKAGE_VERSAL_FIRMWARE_LOCATION
> +	string "versal firmware location"

The prompt should be "Custom git repository"

> +	help
> +	  Location of a versal firmware boot.bin.
> +
> +	  The value should be a git repository.

So, there is not even a default location?

> +config BR2_PACKAGE_VERSAL_FIRMWARE_BOARD
> +	string "versal board name"
> +	help
> +	  Name of versal target board.
> +
> +	  Used for installing the appropriate firmware boot.bin.
> 
> +config BR2_PACKAGE_VERSAL_FIRMWARE_VERSION
> +	string "versal firmware version"
> +	help
> +	  Release version of versal firmware.

The version entry should be just after the git tree entry.

> +endif # BR2_PACKAGE_VERSAL_FIRMWARE
> diff --git a/package/versal-firmware/versal-firmware.mk b/package/versal-firmware/versal-firmware.mk
> new file mode 100644
> index 0000000000..b465b2bd83
> --- /dev/null
> +++ b/package/versal-firmware/versal-firmware.mk
> @@ -0,0 +1,25 @@
> +################################################################################
> +#
> +# versal-firmware
> +#
> +################################################################################
> +
> +VERSAL_FIRMWARE_VERSION = $(call qstrip,$(BR2_PACKAGE_VERSAL_FIRMWARE_VERSION))
> +VERSAL_FIRMWARE_SITE = $(BR2_PACKAGE_VERSAL_FIRMWARE_LOCATION)
> +
> +VERSAL_FIRMWARE_INSTALL_IMAGES = YES
> +
> +VERSAL_FIRMWARE_FILES = \
> +	$(if $(BR2_PACKAGE_VERSAL_FIRMWARE), boot.bin)

This does not make sense: if the versal-firmware package is not enabled,
then it will not be installed, so there is no need to condition some
variables/values to the package being enabled or not.

> +define VERSAL_FIRMWARE_INSTALL_BIN
> +	$(foreach f,$(VERSAL_FIRMWARE_FILES), \
> +		$(INSTALL) -D -m 0644 $(@D)/$(BR2_PACKAGE_VERSAL_FIRMWARE_BOARD)/$(f) $(BINARIES_DIR)/$(f)

VERSAL_FIRMWARE_FILES is only ever one item, boot.bin, the loop will
always be on one item, so there is no need for the loop.

Also, no need for a seaprate macro; just add this code to
VERSAL_FIRMWARE_INSTALL_IMAGES_CMDS.

In the end, I don't think we should apply this package, as it looks like
it tries to circumvent the official distrobution restrictions on these
firmware files.

Regards,
Yann E. MORIN.

> +	)
> +endef
> +
> +define VERSAL_FIRMWARE_INSTALL_IMAGES_CMDS
> +	$(VERSAL_FIRMWARE_INSTALL_BIN)
> +endef
> +
> +$(eval $(generic-package))
> -- 
> 2.17.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot