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

[Julien Olivain <ju.o@free.fr>]

Hi Anand,

On 06/12/2022 18:17, Anand Gadiyar via buildroot wrote:
> 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>

Reviewed-by: Julien Olivain <ju.o@free.fr>

> Acked-by: Andrew Davis <afd@ti.com>
> Tested-by: Bryan Brattlof <bb@ti.com>
> Cc: Romain Naour <romain.naour@gmail.com>
> Cc: Francois Perrad <francois.perrad@gadz.org>
> ---
>  DEVELOPERS                                |  2 ++
>  boot/Config.in                            |  1 +
>  boot/ti-k3-image-gen/Config.in            | 30 ++++++++++++++++
>  boot/ti-k3-image-gen/ti-k3-image-gen.hash |  2 ++
>  boot/ti-k3-image-gen/ti-k3-image-gen.mk   | 42 +++++++++++++++++++++++
>  5 files changed, 77 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 62d77b6c4b..b5b6cd0a99 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>
> @@ -3059,6 +3060,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/
> 
>  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..02018f2f78
> --- /dev/null
> +++ b/boot/ti-k3-image-gen/Config.in
> @@ -0,0 +1,30 @@
> +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.05.00.004 as default.

Why putting the version in the Config.in? Buildroot package usually 
don't
include the version here. It may be missed later, when the package is
updated. Is there anything special about this version?
If not, consider removing the version here.

> +
> +	  https://git.ti.com/cgit/k3-image-gen/k3-image-gen/
> +
> +if BR2_TARGET_TI_K3_IMAGE_GEN
> +config BR2_TARGET_TI_K3_IMAGE_GEN_SOC
> +	string "SOC type for image gen"
> +	help
> +	  The target SoC option for image gen.
> +	  For example, "am64x" for AM64X boards.
> +
> +config BR2_TARGET_TI_K3_IMAGE_GEN_SOC_TYPE
> +	string "SOC security type for image gen"
> +	help
> +	  The security type option for image gen.
> +	  Options are "gp", "hs-fs", or "hs-se".

Comment says: Options are "gp", "hs-fs", or "hs-se".

If those are the only choices, why not using a Kconfig "choice" instead?

Moreover, when trying with a SK-AM64B board, I needed to set "hs-fs" 
(because B board
seems to include a Silicon Revision 2.0, which requires "hs-fs"). In 
that case,
compilation failed with:

ti-k3-image-gen 08.05.00.004 Building
Makefile:67: *** TI_SECURE_DEV_PKG must be set for HS, defaults will not 
work.  Stop.

This "hs-fs" seems to be broken, for now.

A note could be added here, about the meaning of "gp" (General Purpose), 
and
"hs-fs" High-Security Field Securable and how it is linked to some 
silicon revision.
Ex:
https://e2e.ti.com/support/processors-group/processors/f/processors-forum/1158018/faq-processor-sdk-am64x-am64x-am243x-sr-2-0-device-type-transition

> +
> +config BR2_TARGET_TI_K3_IMAGE_GEN_CONFIG
> +	string "CONFIG type for image gen"
> +	help
> +	  The board config option for image gen.
> +	  Usually "sk" or "evm".
> +endif
> +

The empty line at end of Config.in file generate "make check-package" 
warning:

boot/ti-k3-image-gen/Config.in:30: empty line at end of file
...
1 warnings generated
make[1]: *** [Makefile:1257: check-package] Error 1
make: *** [Makefile:84: _all] Error 2

Please remove it.

> 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..82c5b17060
> --- /dev/null
> +++ b/boot/ti-k3-image-gen/ti-k3-image-gen.hash
> @@ -0,0 +1,2 @@
> +# Locally calculated
> +sha256
> d13e9556bfba32d14071e172589683e48f6fea705a0b05ddd7fe984002089888
> k3-image-gen-08.05.00.004.tar.gz

"make legal-info" shows:
ti-k3-image-gen 08.05.00.004 Collecting legal info
ERROR: No hash found for LICENSE

Please add a hash for the LICENSE file declared.

> 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..f8e6105bfb
> --- /dev/null
> +++ b/boot/ti-k3-image-gen/ti-k3-image-gen.mk
> @@ -0,0 +1,42 @@
> +################################################################################
> +#
> +# ti-k3-image-gen
> +#
> +################################################################################
> +
> +TI_K3_IMAGE_GEN_VERSION = 08.05.00.004

There is a newer version 08.05.00.007 available. See:
https://git.ti.com/cgit/k3-image-gen/k3-image-gen/tag/?h=08.05.00.007
Consider taking the latest version in the next patch version, unless 
there is a
specific reason for taking the proposed one (in that case, please add a 
comment).

> +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 = LICENSE
> +TI_K3_IMAGE_GEN_INSTALL_IMAGES = YES
> +
> +# ti-k3-image-gen is used to build tiboot3.bin, using the
> r5-u-boot-spl.bin file
> +# from the ti-k3-r5-loader package. Hence the dependency on 
> ti-k3-r5-loader.
> +TI_K3_IMAGE_GEN_DEPENDENCIES = host-arm-gnu-toolchain ti-k3-r5-loader
> +
> +# The ti-k3-image-gen makefiles seem to need some feature from Make 
> v4.0,
> +# similar to u-boot. Explicitly use $(BR2_MAKE) here, as the build
> +# otherwise fails with some misleading error message.
> +TI_K3_IMAGE_GEN_MAKE = $(BR2_MAKE)
> +TI_K3_IMAGE_GEN_SOC = $(call qstrip,$(BR2_TARGET_TI_K3_IMAGE_GEN_SOC))
> +TI_K3_IMAGE_GEN_SOC_TYPE = $(call
> qstrip,$(BR2_TARGET_TI_K3_IMAGE_GEN_SOC_TYPE))
> +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) \
> +	SOC_TYPE=$(TI_K3_IMAGE_GEN_SOC_TYPE) \
> +	CONFIG=$(TI_K3_IMAGE_GEN_CONFIG) \
> +	CROSS_COMPILE=$(HOST_DIR)/bin/arm-none-eabi- \
> +	SBL=$(BINARIES_DIR)/r5-u-boot-spl.bin \
> +	O=$(@D)/tmp \
> +	BIN_DIR=$(@D)
> +
> +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
> +
> +$(eval $(generic-package))
> --
> 2.34.1

Best regards,

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