Re: [Buildroot] [PATCH v5 2/3] boot/ti-k3-image-gen: add new package

[Romain Naour <romain.naour@gmail.com>]

Hello Anand,

Le 23/09/2022 à 22:55, Anand Gadiyar a écrit :
> From: Xuanhao Shi <x-shi@ti.com>
> 
> This is the image generator that builds the full boot binary,
> tiboot3.bin, for the R5 core on TI's k3 devices.
> This requires the R5 spl output from the ti-k3-r5-loader package.
> 
> https://git.ti.com/cgit/k3-image-gen/k3-image-gen
> 
> Signed-off-by: Xuanhao Shi <x-shi@ti.com>
> Signed-off-by: Anand Gadiyar <gadiyar@ti.com>
> Reviewed-by: Giulio Benetti <giulio.benetti@benettiengineering.com>
> ---
>  DEVELOPERS                                |  2 ++
>  boot/Config.in                            |  1 +
>  boot/ti-k3-image-gen/Config.in            | 24 +++++++++++++++++
>  boot/ti-k3-image-gen/ti-k3-image-gen.hash |  2 ++
>  boot/ti-k3-image-gen/ti-k3-image-gen.mk   | 33 +++++++++++++++++++++++
>  5 files changed, 62 insertions(+)
>  create mode 100644 boot/ti-k3-image-gen/Config.in
>  create mode 100644 boot/ti-k3-image-gen/ti-k3-image-gen.hash
>  create mode 100644 boot/ti-k3-image-gen/ti-k3-image-gen.mk
> 
> diff --git a/DEVELOPERS b/DEVELOPERS
> index 5f36cbf535..81f095f799 100644
> --- a/DEVELOPERS
> +++ b/DEVELOPERS
> @@ -129,6 +129,7 @@ F:	package/libxmlrpc/
>  F:	package/python-docopt/
>  
>  N:	Anand Gadiyar <gadiyar@ti.com>
> +F:	boot/ti-k3-image-gen/
>  F:	boot/ti-k3-r5-loader/
>  
>  N:	André Zwing <nerv@dawncrow.de>
> @@ -3067,6 +3068,7 @@ N:	Wojciech Niziński <niziak@spox.org>
>  F:	package/fwup/
>  
>  N:	Xuanhao Shi <X15000177@gmail.com>
> +F:	boot/ti-k3-image-gen/
>  F:	boot/ti-k3-r5-loader/

Actually this email is bouncing. please remove it.

>  
>  N:	Yair Ben Avraham <yairba@protonmail.com>
> diff --git a/boot/Config.in b/boot/Config.in
> index ce17b2df6b..1b25bacfee 100644
> --- a/boot/Config.in
> +++ b/boot/Config.in
> @@ -22,6 +22,7 @@ source "boot/s500-bootloader/Config.in"
>  source "boot/shim/Config.in"
>  source "boot/sun20i-d1-spl/Config.in"
>  source "boot/syslinux/Config.in"
> +source "boot/ti-k3-image-gen/Config.in"
>  source "boot/ti-k3-r5-loader/Config.in"
>  source "boot/uboot/Config.in"
>  source "boot/vexpress-firmware/Config.in"
> diff --git a/boot/ti-k3-image-gen/Config.in b/boot/ti-k3-image-gen/Config.in
> new file mode 100644
> index 0000000000..00416dcefc
> --- /dev/null
> +++ b/boot/ti-k3-image-gen/Config.in
> @@ -0,0 +1,24 @@
> +config BR2_TARGET_TI_K3_IMAGE_GEN
> +	bool "ti-k3-image-gen"
> +	select BR2_TARGET_TI_K3_R5_LOADER
> +	help
> +	  Use TI's k3-image-gen to build a separate bare metal
> +	  boot binary from a separate spl. Currently supports
> +	  version 08.04.00.005 as default.
> +
> +	  https://git.ti.com/cgit/k3-image-gen/k3-image-gen/

Same comment as for the patch 1/3, use "if BR2_TARGET_TI_K3_IMAGE_GEN" here.

> +
> +config BR2_TARGET_TI_K3_IMAGE_GEN_SOC
> +	string "SOC type for image gen"
> +	depends on BR2_TARGET_TI_K3_IMAGE_GEN
> +	help
> +	  The target SoC option for image gen.
> +	  For example, "am64x" for AM64X boards.
> +
> +config BR2_TARGET_TI_K3_IMAGE_GEN_CONFIG
> +	string "CONFIG type for image gen"
> +	depends on BR2_TARGET_TI_K3_IMAGE_GEN
> +	help
> +	  The board config option for image gen.
> +	  Usually "sk" or "evm".
> +
> diff --git a/boot/ti-k3-image-gen/ti-k3-image-gen.hash b/boot/ti-k3-image-gen/ti-k3-image-gen.hash
> new file mode 100644
> index 0000000000..e307a02a67
> --- /dev/null
> +++ b/boot/ti-k3-image-gen/ti-k3-image-gen.hash
> @@ -0,0 +1,2 @@
> +# Locally calculated
> +sha256  e585dc868ada21ef3389159541d669b88bc406b453470e92da85d9222d271c96  k3-image-gen-08.04.00.005.tar.gz

Since it's a tool released by TI, it would be great if the hash can be provided
in the release note.

> diff --git a/boot/ti-k3-image-gen/ti-k3-image-gen.mk b/boot/ti-k3-image-gen/ti-k3-image-gen.mk
> new file mode 100644
> index 0000000000..1f97490392
> --- /dev/null
> +++ b/boot/ti-k3-image-gen/ti-k3-image-gen.mk
> @@ -0,0 +1,33 @@
> +################################################################################
> +#
> +# ti-k3-image-gen
> +#
> +################################################################################
> +
> +TI_K3_IMAGE_GEN_VERSION = 08.04.00.005
> +TI_K3_IMAGE_GEN_SITE = https://git.ti.com/cgit/k3-image-gen/k3-image-gen/snapshot
> +TI_K3_IMAGE_GEN_SOURCE = k3-image-gen-$(TI_K3_IMAGE_GEN_VERSION).tar.gz
> +TI_K3_IMAGE_GEN_LICENSE = BSD-3-Clause
> +TI_K3_IMAGE_GEN_LICENSE_FILES = system-firmware-image-gen-1.0-manifest.html

It seems the BSD-3-Clause is not the only license of this tool. The manifest
file is also about GPL-2.0, TI Text File  License, TI Commercial License.

Can you clarify the licensing info ?

Also the manifest file hash should be listed in ti-k3-image-gen.hash

(Actually having an html file as license file may not really practical in the
end, can you provide a LICENSE or COPYING file ?)

> +TI_K3_IMAGE_GEN_INSTALL_IMAGES = YES
> +TI_K3_IMAGE_GEN_DEPENDENCIES = host-arm-gnu-toolchain ti-k3-r5-loader

The build dependency on ti-k3-r5-loader may not be obvious, maybe add a comment
to say that ti-k3-image needs r5-u-boot-spl.bin to build.

> +TI_K3_IMAGE_GEN_MAKE = $(BR2_MAKE)

Why do you really need this? This is only required when the build system
requires make 4.0+. If yes, add a small comment.

> +TI_K3_IMAGE_GEN_SOC = $(call qstrip,$(BR2_TARGET_TI_K3_IMAGE_GEN_SOC))
> +TI_K3_IMAGE_GEN_CONFIG = $(call qstrip,$(BR2_TARGET_TI_K3_IMAGE_GEN_CONFIG))
> +TI_K3_IMAGE_GEN_MAKE_OPTS = \
> +	SOC=$(TI_K3_IMAGE_GEN_SOC) \
> +	CONFIG=$(TI_K3_IMAGE_GEN_CONFIG) \
> +	CROSS_COMPILE=$(HOST_ARM_GNU_TOOLCHAIN_INSTALL_DIR)/bin/arm-none-eabi- \

Use $(HOST_DIR) here

> +	SBL=$(BINARIES_DIR)/r5-u-boot-spl.bin \
> +	O=$(BINARIES_DIR) \
> +	BIN_DIR=$(BINARIES_DIR)
> +
> +define TI_K3_IMAGE_GEN_BUILD_CMDS
> +	$(TI_K3_IMAGE_GEN_MAKE) -C $(@D) $(TI_K3_IMAGE_GEN_MAKE_OPTS)
> +endef
> +
> +define TI_K3_IMAGE_GEN_INSTALL_IMAGES_CMDS
> +	cp $(@D)/tiboot3.bin $(BINARIES_DIR)
> +endef


Best regards,
Romain


> +
> +$(eval $(generic-package))

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot