Re: [Buildroot] [PATCH 2/4] package/k3-image-gen: new package

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello,

Thanks for your contribution. However, as you will see below from my
review, this will require significant rework, the approach you're
taking is really not good.

On Tue, 26 Jul 2022 11:50:39 -0500
Xuanhao Shi <x-shi@ti.com> wrote:

> k3-image-gen is a package that helps support TI's k3 SoCs
> by building a separate boot binary, tiboot3.bin, on the R core.
> 
> https://git.ti.com/cgit/k3-image-gen/k3-image-gen/about/
> Signed-off-by: Xuanhao Shi <x-shi@ti.com>
> ---
>  package/Config.in                      |  1 +
>  package/k3-image-gen/Config.in         | 37 ++++++++++++++++++++++
>  package/k3-image-gen/k3-image-gen.hash |  2 ++
>  package/k3-image-gen/k3-image-gen.mk   | 43 ++++++++++++++++++++++++++
>  4 files changed, 83 insertions(+)

A new package needs an entry in the DEVELOPERS file.

Also, I think this package should probably go in boot/ instead of
package/.

> diff --git a/package/k3-image-gen/Config.in b/package/k3-image-gen/Config.in
> new file mode 100644
> index 0000000000..3522382677
> --- /dev/null
> +++ b/package/k3-image-gen/Config.in
> @@ -0,0 +1,37 @@
> +config BR2_PACKAGE_K3_IMAGE_GEN
> +	bool "k3-image-gen"
> +	help
> +	  Using TI's k3-image-gen to build a separate bare metal
> +	  boot binary from a separate spl. Currently supports
> +	  version 08.04.00.004 as default.
> +
> +	  https://git.ti.com/cgit/k3-image-gen/k3-image-gen/
> +
> +menu "U-Boot SPL Build Options"
> +	depends on BR2_PACKAGE_K3_IMAGE_GEN
> +
> +config BR2_PACKAGE_K3_IMAGE_GEN_UBOOT_SPL_DEFCONFIG
> +	string "U-Boot SPL defconfig for image gen"
> +	help
> +	  This is the defconfig without the suffix for the separate SPL.
> +	  For example, "am64x_evm_r5" for AM64X boards.

Do you need two U-Boot builds, one for the main U-Boot running on the
Cortex-A cores and one U-Boot running on the Cortex-R5 core ?


> diff --git a/package/k3-image-gen/k3-image-gen.hash b/package/k3-image-gen/k3-image-gen.hash
> new file mode 100644
> index 0000000000..393c7726cd
> --- /dev/null
> +++ b/package/k3-image-gen/k3-image-gen.hash
> @@ -0,0 +1,2 @@
> +# Locally calculated
> +sha256 8446d4f169b894304593f0c325028f25e401d0b793d6fd3aa9efa3da937222d2  k3-image-gen-08.04.00.004.tar.gz
> diff --git a/package/k3-image-gen/k3-image-gen.mk b/package/k3-image-gen/k3-image-gen.mk
> new file mode 100644
> index 0000000000..5449a2d0a5
> --- /dev/null
> +++ b/package/k3-image-gen/k3-image-gen.mk
> @@ -0,0 +1,43 @@
> +################################################################################
> +#
> +#k3-image-gen
> +#
> +################################################################################
> +
> +K3_IMAGE_GEN_VERSION = 08.04.00.004
> +K3_IMAGE_GEN_SITE = https://git.ti.com/cgit/k3-image-gen/k3-image-gen/snapshot
> +K3_IMAGE_GEN_SOURCE = k3-image-gen-$(K3_IMAGE_GEN_VERSION).tar.gz
> +K3_IMAGE_GEN_LICENSE = GPL-2.0+

Do you have a license file ?

> +K3_IMAGE_GEN_MAKE = $(BR2_MAKE)
> +
> +K3_IMAGE_GEN_UBOOT_SPL_ARCH = arm
> +K3_IMAGE_GEN_UBOOT_SPL_CROSS = \
> +$(HOST_ARM_GNU_TOOLCHAIN_INSTALL_DIR)/bin/arm-none-eabi-
> +K3_IMAGE_GEN_UBOOT_SPL_DEFCONFIG = \
> +$(call qstrip, $(BR2_PACKAGE_K3_IMAGE_GEN_UBOOT_SPL_DEFCONFIG))_defconfig
> +
> +K3_IMAGE_GEN_SOC = $(call qstrip,$(BR2_PACKAGE_K3_IMAGE_GEN_SOC))
> +K3_IMAGE_GEN_CONFIG = $(call qstrip,$(BR2_PACKAGE_K3_IMAGE_GEN_CONFIG))
> +
> +K3_IMAGE_GEN_DEPENDENCIES += host-arm-gnu-toolchain uboot
> +
> +K3_IMAGE_GEN_MAKE_OPTS += \
> +	SOC=$(K3_IMAGE_GEN_SOC) \
> +	CONFIG=$(K3_IMAGE_GEN_CONFIG) \
> +	CROSS_COMPILE=$(K3_IMAGE_GEN_UBOOT_SPL_CROSS) \
> +	SBL=$(BINARIES_DIR)/u-boot-spl.bin \
> +	O=$(BINARIES_DIR) \
> +	BIN_DIR=$(BINARIES_DIR) 
> +
> +define K3_IMAGE_GEN_BUILD_CMDS
> +	$(UBOOT_MAKE) -C $(BUILD_DIR)/uboot* ARCH=$(K3_IMAGE_GEN_UBOOT_SPL_ARCH) \
> +	CROSS_COMPILE=$(K3_IMAGE_GEN_UBOOT_SPL_CROSS) $(K3_IMAGE_GEN_UBOOT_SPL_DEFCONFIG)
> +	$(UBOOT_MAKE) -C $(BUILD_DIR)/uboot* ARCH=$(K3_IMAGE_GEN_UBOOT_SPL_ARCH) \
> +	CROSS_COMPILE=$(K3_IMAGE_GEN_UBOOT_SPL_CROSS)

This clearly is not acceptable. You can from k3-image-gen mess up with
the build directory of U-Boot. $(BUILD_DIR)/uboot* can point to
something else than U-Boot, and even in principle doing the build of
one package in another is wrong.

Could you explain what you are trying to do? Is it because you need to
build two different U-Boot configurations?

Thanks!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot