Re: [Buildroot] [PATCH v8 1/3] package/rockchip-rkbin: new package

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

Killian, All,

On 2023-07-26 20:04 +0200, Kilian Zinnecker via buildroot spake thusly:
> 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 optee) to use from the repository.

So, there was no rationale about why we sould need a custom version [0],
and the defconfig you provided was actually using the same version as
the predefined one, so we did not have any usage for it in-tree.

[0] yes, I read your reply about the question from Thomas, but
discussing with him we conluded that we indeed did not want to have a
custom version for now. So I dropped it.

If and when the need arises, then it will be time to add it back; see
below.

> Signed-off-by: Kilian Zinnecker <kilian.zinnecker@mail.de>
[--SNIP--]
> diff --git a/package/rockchip-rkbin/Config.in b/package/rockchip-rkbin/Config.in
> new file mode 100644
> index 0000000000..85c30c3745
> --- /dev/null
> +++ b/package/rockchip-rkbin/Config.in
> @@ -0,0 +1,47 @@
> +config BR2_PACKAGE_ROCKCHIP_RKBIN
> +	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

So, BR2_PACKAGE_ROCKCHIP_RKBIN_CUSTOM_VERSION_VALUE is guarded twice:
once by the if-block, and once be a "depends on". That's sure a strong
guard! ;-)

Nonethless, I've dropped it.

[--SNIP--]
> +config BR2_PACKAGE_ROCKCHIP_RKBIN_TPL_FILENAME
> +	string "Rockchip rkbin tpl file path"
> +	help
> +	  Full path to the tpl file inside the rkbin repository. The

I've driopped the "Full" qualifier, because that is not a full path, as
it is relative to the top of the rockchip-rkbin directory.

Ditto for the other help texts.

[--SNIP--]
> diff --git a/package/rockchip-rkbin/rockchip-rkbin.hash b/package/rockchip-rkbin/rockchip-rkbin.hash
> new file mode 100644
> index 0000000000..cb71226556
> --- /dev/null
> +++ b/package/rockchip-rkbin/rockchip-rkbin.hash
> @@ -0,0 +1,2 @@
> +# Locally computed
> +sha256  bd8d19ace202ff26d1c0b4d7744cd467cd0093801dc674dde57290159eedee2b  rockchip-rkbin-b4558da0860ca48bf1a571dd33ccba580b9abe23-br1.tar.gz

I've added a hash for the license file. Please check with:

    $ make legal-info
or:
    $ make rockchip-rkbin-legal-info

> diff --git a/package/rockchip-rkbin/rockchip-rkbin.mk b/package/rockchip-rkbin/rockchip-rkbin.mk
> new file mode 100644
> index 0000000000..74787585e8
> --- /dev/null
> +++ b/package/rockchip-rkbin/rockchip-rkbin.mk
> @@ -0,0 +1,46 @@
> +################################################################################
> +#
> +# rockchip-rkbin
> +#
> +################################################################################
> +
> +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
> +ROCKCHIP_RKBIN_LICENSE = PROPRIETARY
> +ROCKCHIP_RKBIN_LICENSE_FILES = LICENSE

Since the license file was only recently added, it should not be defined
except for the known version.

Furthermore, I think hadling the custom version, if we re-introduce it
later, should be done with:

Config.in:

    config BR2_PACKAGE_ROCKCHIP_RKBIN_CUSTOM_VERSION
        bool "Use a custom version"
        help
          Some binaries for older hipsets mey get pruned from the
          latest revisions of the repository. Say 'y' here if your
          SoC uses such older binaries.

    config BR2_PACKAGE_ROCKCHIP_RKBIN_CUSTOM_VERSION_VALUE
        string "Custom version"
        depends on BR2_PACKAGE_ROCKCHIP_RKBIN_CUSTOM_VERSION

rocjchip-rkbin.mk:

    ifeq ($(BR2_PACKAGE_ROCKCHIP_RKBIN_CUSTOM_VERSION),)
    ROCKCHIP_RKBIN_VERSION = b4558da0860ca48bf1a571dd33ccba580b9abe23
    ROCKCHIP_RKBIN_LICENSE_FILES = LICENSE
    else
    ROCKCHIP_RKBIN_VERSION = $(call qstrip,$(BR2_PACKAGE_ROCKCHIP_RKBIN_CUSTOM_VERSION_VALUE))
    ifeq ($(ROCKCHIP_RKBIN_VERSION),)
    $(error blabla check config blabla)
    endif
    BR_NO_CHECK_HASH_FOR += $(ROCKCHIP_RKBIN_SOURCE)
    endif

> +ROCKCHIP_RKBIN_INSTALL_IMAGES = YES
> +ROCKCHIP_RKBIN_INSTALL_TARGET = NO
> +
> +ifneq ($(BR2_PACKAGE_ROCKCHIP_RKBIN_BL31_FILENAME),"")
> +ROCKCHIP_RKBIN_BL31_FILENAME = $(call qstrip,$(BR2_PACKAGE_ROCKCHIP_RKBIN_BL31_FILENAME))
> +endif

No need to test before setting the variable.

> +ifneq ($(BR2_PACKAGE_ROCKCHIP_RKBIN_TPL_FILENAME),"")
> +ROCKCHIP_RKBIN_TPL_FILENAME = $(call qstrip,$(BR2_PACKAGE_ROCKCHIP_RKBIN_TPL_FILENAME))
> +endif
> +
> +ifneq ($(BR2_PACKAGE_ROCKCHIP_RKBIN_TEE_FILENAME),"")
> +ROCKCHIP_RKBIN_TEE_FILENAME = $(call qstrip,$(BR2_PACKAGE_ROCKCHIP_RKBIN_TEE_FILENAME))
> +endif
> +
> +define ROCKCHIP_RKBIN_INSTALL_IMAGES_CMDS
> +	$(if $(ROCKCHIP_RKBIN_BL31_FILENAME), \
> +		cp $(@D)/$(ROCKCHIP_RKBIN_BL31_FILENAME) $(BINARIES_DIR)/${ROCKCHIP_RKBIN_BL31_FILENAME##*/})
> +	$(if $(ROCKCHIP_RKBIN_TPL_FILENAME), \
> +		cp $(@D)/$(ROCKCHIP_RKBIN_TPL_FILENAME) $(BINARIES_DIR)/${ROCKCHIP_RKBIN_TPL_FILENAME##*/})
> +	$(if $(ROCKCHIP_RKBIN_TEE_FILENAME), \
> +		cp $(@D)/$(ROCKCHIP_RKBIN_TEE_FILENAME) $(BINARIES_DIR)/${ROCKCHIP_RKBIN_TEE_FILENAME##*/})

I've simplified this with a $(foreach...)

Applied to master with these changes, thanks.

Regards,
Yann E. MORIN.

> +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
> +ifeq ($(BR_BUILDING),y)
> +BR_NO_CHECK_HASH_FOR += rockchip-rkbin-$(ROCKCHIP_RKBIN_VERSION)-br1.tar.gz
> +endif
> +endif
> +
> +$(eval $(generic-package))
> -- 
> 2.34.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