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

[Kilian Zinnecker via buildroot <buildroot@buildroot.org>]

Hello Yann, all

thanks for your review, fixes and applying the patches. And sorry for the late 
answer. What I write now is imho not so important anymore, still I want to 
give some reply:

> > 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.

I personally still favor it, but its not so important to me and I can 
understand, that you don't see much need at the moment and probably prefer to 
keep it simple, so I am totally fine with dropping the custom version stuff. As 
you say: If need arises, we can then revaluate.

[--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.

Agreed. However, you introduced some typo on one occasion, by snipping "Full" 
and "path" to "Fath ;)

https://gitlab.com/buildroot.org/buildroot/-/blob/
8d1180aa483b624f1be27bd0858c2f6665ebf36c/package/rockchip-rkbin/Config.in#L12

> > 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

Thanks, seems to work on my PC.

> > 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

Great, thanks! I did actually have in mind, that the custom version and the 
(in older versions not existing) LICENSE file may cause problems. But I didn't 
know what to do about it and forgot to ask here. Thanks!

[--SNIP--]

> > +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...)

Thanks, much better of course!

> Applied to master with these changes, thanks.

Thanks for reviewing!

Best regards,
Kilian




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