Re: [Buildroot] [PATCH v6 1/3] package: add rockchip-rkbin package

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

Hello Kilian,

Thanks for your work! See below some comments.

On Fri, 14 Jul 2023 08:44:11 +0200
Kilian Zinnecker via buildroot <buildroot@buildroot.org> wrote:

> This patch adds a package for the Rockchip ATF binary blobs. These
> binaries are needed to build U-Boot for some Rockchip SoCs (e.g.,
> RK3588). One can config a custom version and manually define which
> blobs (for bl31, tpl and tee) to use from the repository. The
> U-Boot package is modified so that it takes the chosen binaries and
> uses them during build.
> 
> Signed-off-by: Kilian Zinnecker <kilian.zinnecker@mail.de>

Nit: the commit title should be:

	package/rockchip-rkbin: new package

> diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
> index 085397d03d..7733f8501f 100644
> --- a/boot/uboot/Config.in
> +++ b/boot/uboot/Config.in
> @@ -262,6 +262,15 @@ config BR2_TARGET_UBOOT_NEEDS_IMX_FIRMWARE
>  	  This option makes sure that the i.MX firmwares are copied into
>  	  the U-Boot source directory.
>  
> +config BR2_TARGET_UBOOT_NEEDS_ROCKCHIP_RKBIN
> +	bool "U-Boot needs rockchip-rkbin"
> +	depends on BR2_PACKAGE_ROCKCHIP_RKBIN
> +	help
> +	  For some Rockchip SoCs U-Boot needs binary blobs from
> +	  Rockchip.
> +	  This option makes sure that the needed binary blobs are copied
> +	  into the U-Boot source directory.
> +
>  menu "U-Boot binary format"
>  
>  config BR2_TARGET_UBOOT_FORMAT_AIS
> diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
> index 4eae8e95c3..f9f50539e5 100644
> --- a/boot/uboot/uboot.mk
> +++ b/boot/uboot/uboot.mk
> @@ -207,6 +207,23 @@ endef
>  UBOOT_PRE_BUILD_HOOKS += UBOOT_COPY_IMX_FW_FILES
>  endif
>  
> +ifeq ($(BR2_TARGET_UBOOT_NEEDS_ROCKCHIP_RKBIN),y)
> +UBOOT_DEPENDENCIES += rockchip-rkbin
> +define UBOOT_INSTALL_UBOOT_ROCKCHIP_BIN
> +	$(INSTALL) -D -m 0644 $(@D)/u-boot-rockchip.bin $(BINARIES_DIR)/

We need a full destination path here: $(BINARIES_DIR)/u-boot/rockchip.bin

> +endef
> +UBOOT_POST_INSTALL_IMAGES_HOOKS += UBOOT_INSTALL_UBOOT_ROCKCHIP_BIN
> +ifeq ($(BR2_PACKAGE_ROCKCHIP_RKBIN_BL31_FILE),y)
> +UBOOT_MAKE_OPTS += BL31=$(BINARIES_DIR)/bl31${suffix $(call qstrip,$(BR2_PACKAGE_ROCKCHIP_RKBIN_BL31_FILENAME))}

What is this ${...} ?

> +endif
> +ifeq ($(BR2_PACKAGE_ROCKCHIP_RKBIN_TPL_FILE),y)
> +UBOOT_MAKE_OPTS += ROCKCHIP_TPL=$(BINARIES_DIR)/ddr${suffix $(call qstrip,$(BR2_PACKAGE_ROCKCHIP_RKBIN_TPL_FILENAME))}
> +endif
> +ifeq ($(BR2_PACKAGE_ROCKCHIP_RKBIN_TEE_FILE),y)
> +$BOOT_MAKE_OPTS += TEE=$(BINARIES_DIR)/tee${suffix $(call qstrip,$(BR2_PACKAGE_ROCKCHIP_RKBIN_TEE_FILENAME))}

   ^^ typo, should be UBOOT_MAKE_OPTS


> +endif
> +endif
> +

These changes to boot/uboot/ should be in a separate patch, after the
patch adding rockchip-rkbin.

> diff --git a/package/rockchip-rkbin/Config.in b/package/rockchip-rkbin/Config.in
> new file mode 100644
> index 0000000000..0f2126654e
> --- /dev/null
> +++ b/package/rockchip-rkbin/Config.in
> @@ -0,0 +1,70 @@
> +config BR2_PACKAGE_ROCKCHIP_RKBIN
> +	bool "Rockchip rkbin binary blobs"

Just:

	bool "rockchip-rkbin"

> +	depends on BR2_arm || BR2_aarch64
> +	help
> +	  This package provides Rockchip SoC binary blobs for U-Boot.
> +
> +if BR2_PACKAGE_ROCKCHIP_RKBIN
> +
> +config BR2_PACKAGE_ROCKCHIP_RKBIN_CUSTOM_VERSION
> +	bool "Use a custom version"
> +	help
> +	  This option allows to use a specific version.
> +if BR2_PACKAGE_ROCKCHIP_RKBIN_CUSTOM_VERSION
> +
> +config BR2_PACKAGE_ROCKCHIP_RKBIN_CUSTOM_VERSION_VALUE
> +	string "Rockchip rkbin version"
> +	depends on BR2_PACKAGE_ROCKCHIP_RKBIN_CUSTOM_VERSION
> +
> +endif # BR2_PACKAGE_ROCKCHIP_RKBIN_CUSTOM_VERSION
> +
> +config BR2_PACKAGE_ROCKCHIP_RKBIN_VERSION
> +	string
> +	default "d6ccfe401ca84a98ca3b85c12b9554a1a43a166c" \
> +		if !BR2_PACKAGE_ROCKCHIP_RKBIN_CUSTOM_VERSION
> +	default BR2_PACKAGE_ROCKCHIP_RKBIN_CUSTOM_VERSION_VALUE \
> +		if BR2_PACKAGE_ROCKCHIP_RKBIN_CUSTOM_VERSION

Is it really likely that a different version, but from the same Git
repository will be needed? I see in your defconfig you're using a
different version than the default used here. Why?

> +
> +config BR2_PACKAGE_ROCKCHIP_RKBIN_TPL_FILE
> +	bool "Rockchip rkbin tpl file"
> +	default n

"default n" is never needed, as it's the default.

> +
> +if BR2_PACKAGE_ROCKCHIP_RKBIN_TPL_FILE
> +
> +config BR2_PACKAGE_ROCKCHIP_RKBIN_TPL_FILENAME
> +	string "ddr.bin file path"
> +	help
> +	  Full path to the tpl file inside the rkbin repository. The
> +	  specified file will be copied and used by U-Boot as tpl.
> +
> +endif # BR2_PACKAGE_ROCKCHIP_RKBIN_TPL_FILE
> +
> +config BR2_PACKAGE_ROCKCHIP_RKBIN_BL31_FILE
> +	bool "Rockchip rkbin bl31 file"
> +	default n
> +
> +if BR2_PACKAGE_ROCKCHIP_RKBIN_BL31_FILE
> +
> +config BR2_PACKAGE_ROCKCHIP_RKBIN_BL31_FILENAME
> +	string "bl31.elf file path"
> +	help
> +	  Full path to the bl31 file inside the rkbin repository. The
> +	  specified file will be copied and used by U-Boot as bl31.
> +
> +endif # BR2_PACKAGE_ROCKCHIP_RKBIN_BL31_FILE
> +
> +config BR2_PACKAGE_ROCKCHIP_RKBIN_TEE_FILE
> +	bool "Rockchip rkbin optee file"
> +	default n
> +
> +if BR2_PACKAGE_ROCKCHIP_RKBIN_TEE_FILE
> +
> +config BR2_PACKAGE_ROCKCHIP_RKBIN_TEE_FILENAME
> +	string "tee.elf file path"
> +	help
> +	  Full path to the TEE file inside the rkbin repository.
> +	  The specified file will be copied and used by U-Boot as
> +	  TEE.
> +endif # BR2_PACKAGE_ROCKCHIP_RKBIN_TEE_FILE

For those 3 files, do we really need a boolean + a string option? Can't
we simply rely on string options: if the value is empty, there's no
TPL/BL31/TEE, if the value is non-empty, use the file provided.

> diff --git a/package/rockchip-rkbin/rockchip-rkbin.hash b/package/rockchip-rkbin/rockchip-rkbin.hash
> new file mode 100644
> index 0000000000..5659ecf719
> --- /dev/null
> +++ b/package/rockchip-rkbin/rockchip-rkbin.hash
> @@ -0,0 +1,3 @@
> +# Locally computed
> +sha256  6f7b58fe35108101031ebfd3cc6eb7a186f258a1cdbd93c4256888997ab52c8f  rockchip-rkbin-d6ccfe401ca84a98ca3b85c12b9554a1a43a166c-br1.tar.gz
> +

Nit: useless empty new line.

> diff --git a/package/rockchip-rkbin/rockchip-rkbin.mk b/package/rockchip-rkbin/rockchip-rkbin.mk
> new file mode 100644
> index 0000000000..7b2f17f7e2
> --- /dev/null
> +++ b/package/rockchip-rkbin/rockchip-rkbin.mk
> @@ -0,0 +1,49 @@
> +################################################################################
> +#
> +# rockchip-rkbin
> +#
> +################################################################################
> +
> +

Nit: only one empty new line.

> +ROCKCHIP_RKBIN_VERSION = $(call qstrip,$(BR2_PACKAGE_ROCKCHIP_RKBIN_VERSION))
> +ROCKCHIP_RKBIN_SITE = https://github.com/rockchip-linux/rkbin.git
> +ROCKCHIP_RKBIN_SITE_METHOD = git

Any reason not to use:

ROCKCHIP_RKBIN_SITE = $(call github,rockchip-linux,rkbin,$(ROCKCHIP_RKBIN_VERSION))

 ?

> +ROCKCHIP_RKBIN_LICENSE = PROPRIETARY
> +ROCKCHIP_RKBIN_REDISTRIBUTE = NO

This is the problematic part I believe. What is the license of this
stuff? Without any license, nobody is allowed to redistribute it. So as
it is, all images produced by Buildroot with this package cannot be
redistributed, making them quite useless, unless it's used by a
hobbyist building his own images for his own usage, without any
redistribution.

We have the same issue with some Amlogic blobs I believe. Blobs are
annoying, but they are even more annoying when there's no license
attached to them that allows redistribution.

> +ROCKCHIP_RKBIN_INSTALL_IMAGES = YES
> +ROCKCHIP_RKBIN_INSTALL_TARGET = NO
> +
> +ifeq ($(BR2_PACKAGE_ROCKCHIP_RKBIN_BL31_FILE),y)
> +ROCKCHIP_RKBIN_BL31_FILENAME = $(call qstrip,$(BR2_PACKAGE_ROCKCHIP_RKBIN_BL31_FILENAME))
> +BL31_EXTENSION=${suffix $(ROCKCHIP_RKBIN_BL31_FILENAME)}

All variables must be prefixed with the package name. Again this
${suffix ...} thing, weird.

> +endif
> +
> +ifeq ($(BR2_PACKAGE_ROCKCHIP_RKBIN_TPL_FILE),y)
> +ROCKCHIP_RKBIN_TPL_FILENAME = $(call qstrip,$(BR2_PACKAGE_ROCKCHIP_RKBIN_TPL_FILENAME))
> +TPL_EXTENSION=${suffix $(ROCKCHIP_RKBIN_TPL_FILENAME)}

All variables must be prefixed with the package name.

> +endif
> +
> +ifeq ($(BR2_PACKAGE_ROCKCHIP_RKBIN_TEE_FILE),y)
> +ROCKCHIP_RKBIN_TEE_FILENAME = $(call qstrip,$(BR2_PACKAGE_ROCKCHIP_RKBIN_TEE_FILENAME))
> +TEE_EXTENSION=${suffix $(ROCKCHIP_RKBIN_TEE_FILENAME)}

All variables must be prefixed with the package name.

> +endif
> +
> +define ROCKCHIP_RKBIN_INSTALL_IMAGES_CMDS
> +	$(if $(filter y, $(BR2_PACKAGE_ROCKCHIP_RKBIN_BL31_FILE)), \
> +		cp $(@D)/$(ROCKCHIP_RKBIN_BL31_FILENAME) $(BINARIES_DIR)/bl31$(BL31_EXTENSION))
> +	$(if $(filter y, $(BR2_PACKAGE_ROCKCHIP_RKBIN_TPL_FILE)), \
> +		cp $(@D)/$(ROCKCHIP_RKBIN_TPL_FILENAME) $(BINARIES_DIR)/ddr$(TPL_EXTENSION))
> +	$(if $(filter y, $(BR2_PACKAGE_ROCKCHIP_RKBIN_TEE_FILE)), \
> +		cp $(@D)/$(ROCKCHIP_RKBIN_TEE_FILENAME) $(BINARIES_DIR)/tee$(TEE_EXTENSION))

The filter y construct is a bit useless, you can just do:

	$(if $(BR2_PACKAGE_ROCKCHIP_RKBIN_TEE_FILE),\
		do something)

> +endef
> +
> +ifeq ($(BR2_PACKAGE_ROCKCHIP_RKBIN_CUSTOM_VERSION),y)
> +ifeq ($(call qstrip,$(BR2_PACKAGE_ROCKCHIP_RKBIN_CUSTOM_VERSION_VALUE)),)
> +$(error No custom rockchip-rkbin version specified. Check your BR2_PACKAGE_ROCKCHIP_RKBIN_CUSTOM_VERSION_VALUE setting)
> +endif
> +ROCKCHIP_SOURCE = rockchip-rkbin-$(ROCKCHIP_RKBIN_VERSION)-br1.tar.gz

Hum, I wonder why this needs to be defined. It seems to work for U-Boot
without having to define UBOOT_SOURCE in uboot.mk, the infrastructure
does it for you.

> +BR_NO_CHECK_HASH_FOR += $(ROCKCHIP_SOURCE)
> +endif

This check should be enclosed in a:

ifeq ($(BR_BUILDING),y)
...
endif

condition.

Overall, mostly cosmetic comments that I could have solved myself, but
the licensing problem is much more significant and I cannot resolve it.

Best regards,

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