| Message ID | 20230705062142.10968-1-kilian.zinnecker@mail.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3,1/3] package: add rockchip-rkbin package | expand |
Hi Kilian, all ... > Signed-off-by: Kilian Zinnecker <kilian.zinnecker@mail.de> ... [v3 1-3] Reviewed-by: Andreas Ziegler <br015@umbiko.net> Tested-by: Andreas Ziegler <br015@umbiko.net> build test only, (l)ubuntu 22.04.02 LTS on qemu-system-x86_64 Kind regards, Andreas
Hi Kilian, Thanks for the patch. On 7/5/23 08:21, Kilian Zinnecker via buildroot wrote: > This patch adds a package for the Rockchip ATF binary blobs. These > binaries are currently needed if U-Boot is built for the Rockchip > RK3588 SoC. In addition the uboot package is modified such, that it > can depend on the rockchip-rkbin package and in that case > automatically obtain the needed binary blobs. > > Signed-off-by: Kilian Zinnecker <kilian.zinnecker@mail.de> > --- > DEVELOPERS | 3 +++ > boot/uboot/Config.in | 9 +++++++++ > boot/uboot/uboot.mk | 10 ++++++++++ > package/Config.in | 1 + > package/rockchip-rkbin/Config.in | 6 ++++++ > package/rockchip-rkbin/rockchip-rkbin.mk | 23 +++++++++++++++++++++++ > 6 files changed, 52 insertions(+) > create mode 100644 package/rockchip-rkbin/Config.in > create mode 100644 package/rockchip-rkbin/rockchip-rkbin.mk > > diff --git a/DEVELOPERS b/DEVELOPERS > index 188c579010..8e603ff3f8 100644 > --- a/DEVELOPERS > +++ b/DEVELOPERS > @@ -1776,6 +1776,9 @@ F: package/ramsmp/ > N: Kieran Bingham <kieran.bingham@ideasonboard.com> > F: package/libcamera/ > > +N: Kilian Zinnecker <kilian.zinnecker@mail.de> > +F: package/rockchip-rkbin/ > + > N: Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com> > F: package/wqy-zenhei/ > > diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in > index 085397d03d..8ec095a735 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 > + U-Boot currently needs binary blobs during build for the > + Rockchip RK3588 SoC. This isn't specific to RK3588. It is in fact how Rockchip starts with each of their new SoCs. This currently applies to RK3568 as well and since there is at least one Radxa board (Rock 3A if I'm not mistaken), I guess we're gonna see patches for this one soon? Also, users can decide to use blobs instead of compiled-from-sources binaries for TF-A, OP-TEE OS and TPL for those SoCs like RK3399 which have upstream support. I would suggest to just say that this provides binary blobs (such as DDR init (usually called TPL in U-Boot world), TF-A and OP-TEE OS) typically used for new SoCs designs until different upstream projects get support for the new SoCs. > + 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..4ef0a7bc2a 100644 > --- a/boot/uboot/uboot.mk > +++ b/boot/uboot/uboot.mk > @@ -207,6 +207,16 @@ 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_COPY_RKBIN_FW_FILES > + cp $(BINARIES_DIR)/bl31.bin $(@D) > + cp $(BINARIES_DIR)/ddr.bin $(@D) This is an issue with supporting multiple Rockchip SoCs with the same package since we would need to have an option in RKBIN package to select the original file to rename to bl31.bin and ddr.bin. How bad of an idea is it to parse the BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS variable for BL31, ROCKCHIP_TPL and TEE variables to find out the exact name of the file to use? > +endef > +UBOOT_PRE_BUILD_HOOKS += UBOOT_COPY_RKBIN_FW_FILES > +endif > + > ifeq ($(BR2_TARGET_UBOOT_NEEDS_DTC),y) > UBOOT_DEPENDENCIES += host-dtc > endif > diff --git a/package/Config.in b/package/Config.in > index bff090a661..80221d0406 100644 > --- a/package/Config.in > +++ b/package/Config.in > @@ -581,6 +581,7 @@ endmenu > source "package/read-edid/Config.in" > source "package/rng-tools/Config.in" > source "package/rockchip-mali/Config.in" > + source "package/rockchip-rkbin/Config.in" > source "package/rpi-userland/Config.in" > source "package/rs485conf/Config.in" > source "package/rtc-tools/Config.in" > diff --git a/package/rockchip-rkbin/Config.in b/package/rockchip-rkbin/Config.in > new file mode 100644 > index 0000000000..d8072002ca > --- /dev/null > +++ b/package/rockchip-rkbin/Config.in > @@ -0,0 +1,6 @@ > +config BR2_PACKAGE_ROCKCHIP_RKBIN > + bool "Rockchip RKBIN binary blobs" > + depends on BR2_arm || BR2_aarch64 > + help > + This package provides binary blobs, currently needed by > + u-boot for the Rockchip RK3588 SoC. Don't specify the Rockchip SoC part number, this is already not encompassing all SoCs that need RKBIN (such as RK3568). You can however say that this is typically needed by U-Boot for the U-Boot proper fitimage. > diff --git a/package/rockchip-rkbin/rockchip-rkbin.mk b/package/rockchip-rkbin/rockchip-rkbin.mk > new file mode 100644 > index 0000000000..ac50c8931d > --- /dev/null > +++ b/package/rockchip-rkbin/rockchip-rkbin.mk > @@ -0,0 +1,23 @@ > +################################################################################ > +# > +# rockchip-rkbin > +# > +################################################################################ > + > + > +ROCKCHIP_RKBIN_VERSION = d6ccfe401ca84a98ca3b85c12b9554a1a43a166c > +ROCKCHIP_RKBIN_SITE = https://github.com/rockchip-linux/rkbin.git > +ROCKCHIP_RKBIN_SITE_METHOD = git > + > +ROCKCHIP_RKBIN_INSTALL_STAGING = YES > +ROCKCHIP_RKBIN_INSTALL_TARGET = NO > + > +RK3588_DDRFW = rk3588_ddr_lp4_2112MHz_lp5_2736MHz_v1.11.bin > +RK3588_BL3FW = rk3588_bl31_v1.38.elf > + > +define ROCKCHIP_RKBIN_INSTALL_STAGING_CMDS > + cp $(@D)/bin/rk35/$(RK3588_BL3FW) $(BINARIES_DIR)/bl31.bin I would highly recommend to keep the same file extension to not confuse people since the OP-TEE binary apparently has different formats which we can (more easily) distinguish from their file extension (though IIRC, reading the header may be enough). > + cp $(@D)/bin/rk35/$(RK3588_DDRFW) $(BINARIES_DIR)/ddr.bin I would highly recommend to not have a so common name which abstracts the SoC part number. I can think of two ways of doing this: create a subdir after each SoC part number in BINARIES_DIR (e.g. BINARIES_DIR/rk3588/bl31.elf) or have the part number in the filename (e.g. rk3588_bl31.elf). This should allow to install the binary blobs from all SoC supported in RKBIN without having to deal with Kconfig options. The rename to remove the possibly changing filename is a good idea though, so we don't need to update the BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS variable in the defconfig to reflect updates to RKBIN. Also, OP-TEE OS is available as a blob in there (see bl32 file), maybe making it available would be good too? Ok so another thought now... In boot/u-boot.mk, we already have ``` ifeq ($(BR2_TARGET_UBOOT_NEEDS_ATF_BL31),y) UBOOT_DEPENDENCIES += arm-trusted-firmware ifeq ($(BR2_TARGET_UBOOT_NEEDS_ATF_BL31_ELF),y) UBOOT_MAKE_OPTS += BL31=$(BINARIES_DIR)/bl31.elf define UBOOT_COPY_ATF_FIRMWARE cp $(BINARIES_DIR)/bl31.elf $(@D)/ endef UBOOT_PRE_BUILD_HOOKS += UBOOT_COPY_ATF_FIRMWARE else UBOOT_MAKE_OPTS += BL31=$(BINARIES_DIR)/bl31.bin define UBOOT_COPY_ATF_FIRMWARE cp $(BINARIES_DIR)/bl31.bin $(@D)/ endef UBOOT_PRE_BUILD_HOOKS += UBOOT_COPY_ATF_FIRMWARE endif endif ifeq ($(BR2_TARGET_UBOOT_NEEDS_OPTEE_TEE),y) UBOOT_DEPENDENCIES += optee-os UBOOT_MAKE_OPTS += TEE=$(BINARIES_DIR)/tee.elf endif ``` which should set everything up correctly for bl31 and tee-os, without needing to add BL31 and TEE to BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS manually. We would however probably need to go with a virtual package (or whatever the proper name for Buildroot would be for two packages providing the same functionality and have a selection mechanism in place in Kconfig for it) for TF-A and tee-os so that UBOOT_DEPENDENCIES picks up the correct dependency. RKBIN would be one of the possible provides for arm-trusted-firmware and optee-os for example. Looking at the code above, if we want to keep it a bit automagical, maybe having the RKBIN binaries called bl31.bin and tee.elf and have a Kconfig option for the RKBIN package to select which SoC binary blobs to install would be best. We would still however need a special case for the TPL coming from RKBIN package since there's no piping for this at the moment in boot/u-boot.mk. I feel like RKBIN package should be able to handle multiple SoCs (either by providing all SoCs binary blobs at once, or have a Kconfig option for it) and we should make it not too error-prone on the user-side for the U-Boot configuration. At the same time, I don't know if it's worth the added complexity. Cheers, Quentin
Hello Quentin, All, > Thanks for the patch. You're welcome and thanks for your feedback. > > +config BR2_TARGET_UBOOT_NEEDS_ROCKCHIP_RKBIN > > + bool "U-Boot needs rockchip-rkbin" > > + depends on BR2_PACKAGE_ROCKCHIP_RKBIN > > + help > > + U-Boot currently needs binary blobs during build for the > > + Rockchip RK3588 SoC. > > This isn't specific to RK3588. It is in fact how Rockchip starts with > each of their new SoCs. This currently applies to RK3568 as well and > since there is at least one Radxa board (Rock 3A if I'm not mistaken), I > guess we're gonna see patches for this one soon? Yes, correct. Do you refer to buildroot patches for support of further Radxa boards or to u-boot patches to support further Rockchip SoCs? Personally I currently don't have any plans to provide patches to bulidroot regarding board support of other Radxa boards. Still I think the concern is of course legit, i.e., that someone wants to use this rockchip-rkbin buildroot package for other Rockchip SoCs, but it (in its current form) does not provide functionality for that. > Also, users can decide to use blobs instead of compiled-from-sources > binaries for TF-A, OP-TEE OS and TPL for those SoCs like RK3399 which > have upstream support. I agree. > I would suggest to just say that this provides binary blobs (such as DDR > init (usually called TPL in U-Boot world), TF-A and OP-TEE OS) typically > used for new SoCs designs until different upstream projects get support > for the new SoCs. Ok, I will change it. > > UBOOT_PRE_BUILD_HOOKS += UBOOT_COPY_IMX_FW_FILES > > endif > > > > +ifeq ($(BR2_TARGET_UBOOT_NEEDS_ROCKCHIP_RKBIN),y) > > +UBOOT_DEPENDENCIES += rockchip-rkbin > > + > > +define UBOOT_COPY_RKBIN_FW_FILES > > + cp $(BINARIES_DIR)/bl31.bin $(@D) > > + cp $(BINARIES_DIR)/ddr.bin $(@D) > > This is an issue with supporting multiple Rockchip SoCs with the same > package since we would need to have an option in RKBIN package to select > the original file to rename to bl31.bin and ddr.bin. Yes, I agree. For my purpose (i.e., only support for Rock 5B) I guess it is ok this way, but if the rockchip-rkbin package should also be usable for other SoCs, some way to identify the particular binaries would be needed. > How bad of an idea is it to parse the BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS > variable for BL31, ROCKCHIP_TPL and TEE variables to find out the exact > name of the file to use? I guess this would work but I don't know if it is a good idea. > > +config BR2_PACKAGE_ROCKCHIP_RKBIN > > + bool "Rockchip RKBIN binary blobs" > > + depends on BR2_arm || BR2_aarch64 > > + help > > + This package provides binary blobs, currently needed by > > + u-boot for the Rockchip RK3588 SoC. > > Don't specify the Rockchip SoC part number, this is already not > encompassing all SoCs that need RKBIN (such as RK3568). You can however > say that this is typically needed by U-Boot for the U-Boot proper fitimage. Ok. > > +RK3588_DDRFW = rk3588_ddr_lp4_2112MHz_lp5_2736MHz_v1.11.bin > > +RK3588_BL3FW = rk3588_bl31_v1.38.elf > > + > > +define ROCKCHIP_RKBIN_INSTALL_STAGING_CMDS > > + cp $(@D)/bin/rk35/$(RK3588_BL3FW) $(BINARIES_DIR)/bl31.bin > > I would highly recommend to keep the same file extension to not confuse > people since the OP-TEE binary apparently has different formats which we > can (more easily) distinguish from their file extension (though IIRC, > reading the header may be enough). Thanks. > > + cp $(@D)/bin/rk35/$(RK3588_DDRFW) $(BINARIES_DIR)/ddr.bin > > I would highly recommend to not have a so common name which abstracts > the SoC part number. Why is the SoC part number so important to you? > I can think of two ways of doing this: create a > subdir after each SoC part number in BINARIES_DIR (e.g. > BINARIES_DIR/rk3588/bl31.elf) or have the part number in the filename > (e.g. rk3588_bl31.elf). > This should allow to install the binary blobs from all SoC supported in > RKBIN without having to deal with Kconfig options. How so? Because we would then copy all blobs at once into the BINARIES_DIR everytime? > The rename to remove the possibly changing filename is a good idea > though, so we don't need to update the BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS > variable in the defconfig to reflect updates to RKBIN. I agree. > Also, OP-TEE OS is available as a blob in there (see bl32 file), maybe > making it available would be good too? Agreed. > Ok so another thought now... > > In boot/u-boot.mk, we already have > ``` > ifeq ($(BR2_TARGET_UBOOT_NEEDS_ATF_BL31),y) > UBOOT_DEPENDENCIES += arm-trusted-firmware > ifeq ($(BR2_TARGET_UBOOT_NEEDS_ATF_BL31_ELF),y) > UBOOT_MAKE_OPTS += BL31=$(BINARIES_DIR)/bl31.elf > define UBOOT_COPY_ATF_FIRMWARE > cp $(BINARIES_DIR)/bl31.elf $(@D)/ > endef > UBOOT_PRE_BUILD_HOOKS += UBOOT_COPY_ATF_FIRMWARE > else > UBOOT_MAKE_OPTS += BL31=$(BINARIES_DIR)/bl31.bin > define UBOOT_COPY_ATF_FIRMWARE > cp $(BINARIES_DIR)/bl31.bin $(@D)/ > endef > UBOOT_PRE_BUILD_HOOKS += UBOOT_COPY_ATF_FIRMWARE > endif > endif > > ifeq ($(BR2_TARGET_UBOOT_NEEDS_OPTEE_TEE),y) > UBOOT_DEPENDENCIES += optee-os > UBOOT_MAKE_OPTS += TEE=$(BINARIES_DIR)/tee.elf > endif > ``` > > which should set everything up correctly for bl31 and tee-os, without > needing to add BL31 and TEE to BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS manually. > > We would however probably need to go with a virtual package (or whatever > the proper name for Buildroot would be for two packages providing the > same functionality and have a selection mechanism in place in Kconfig > for it) for TF-A and tee-os so that UBOOT_DEPENDENCIES picks up the > correct dependency. RKBIN would be one of the possible provides for > arm-trusted-firmware and optee-os for example. Or I just add the UBOOT_MAKE_OPTS += BL31=$(BINARIES_DIR)/bl31.bin to the BR2_UBOOT_NEEDS_ROCKCHIP_RKBIN block? > Looking at the code above, if we want to keep it a bit automagical, > maybe having the RKBIN binaries called bl31.bin and tee.elf and have a > Kconfig option for the RKBIN package to select which SoC binary blobs to > install would be best. In the readme of the rkbin repository they present their file naming scheme. However, I don't think I like making use of it: 1. We introduce a lot of Kconfig code for some logic that adds choices and selects files within the rkbin repository. But in the end the user still has to make the decision, which files are to use, anyway. Then the user can also just directly specify the true filename in the board defconfig or .config. 2. We add a lot dependency: Rockchip may remove or rename files in their git reblobitory as they like. If we (or any other users) then want to use a newer commit of their reblobitory, e.g., to use newer blobs, and the file name / path scheme breaks, we have to fix that again. > We would still however need a special case for the TPL coming from RKBIN > package since there's no piping for this at the moment in boot/u-boot.mk. > > I feel like RKBIN package should be able to handle multiple SoCs (either > by providing all SoCs binary blobs at once, or have a Kconfig option for > it) and we should make it not too error-prone on the user-side for the > U-Boot configuration. > > At the same time, I don't know if it's worth the added complexity. Thinking out loud: For the rockchip-rkbin package we could feature: * custom git version option (then defconfigs could freeze the repo version) * option to specify tpl file name * option to specify bl31 file name * option to specify op-tee file name * logic, which copies the specified files to BINARIES_DIR, while renaming them to generic names (e.g., "bl31.elf") For the u-boot package we could add BR2_TARGET_UBOOT_NEEDS_ROCKCHIP_RKBIN in uboot.mk: It adds the dependency for the package rockchip-rkbin and introduces code, which parses the rockchip-rkbin BR2 variables and adds the needed options to UBOOT_MAKE_OPTS accordingly. So it is very similar to BR2_TARGET_UBOOT_NEEDS_ATF_BL31, but knows from the sprecified rockchip-rkbin variables which uboot make opts to add (or not to add). What do you think? Best regards, Kilian
Hi Kilian, On 7/7/23 00:56, Kilian Zinnecker wrote: > Hello Quentin, All, > >> Thanks for the patch. > > You're welcome and thanks for your feedback. > >>> +config BR2_TARGET_UBOOT_NEEDS_ROCKCHIP_RKBIN >>> + bool "U-Boot needs rockchip-rkbin" >>> + depends on BR2_PACKAGE_ROCKCHIP_RKBIN >>> + help >>> + U-Boot currently needs binary blobs during build for the >>> + Rockchip RK3588 SoC. >> >> This isn't specific to RK3588. It is in fact how Rockchip starts with >> each of their new SoCs. This currently applies to RK3568 as well and >> since there is at least one Radxa board (Rock 3A if I'm not mistaken), I >> guess we're gonna see patches for this one soon? > > Yes, correct. Do you refer to buildroot patches for support of further Radxa > boards or to u-boot patches to support further Rockchip SoCs? Personally I > currently don't have any plans to provide patches to bulidroot regarding board > support of other Radxa boards. Still I think the concern is of course legit, > i.e., that someone wants to use this rockchip-rkbin buildroot package for > other Rockchip SoCs, but it (in its current form) does not provide > functionality for that. > U-Boot, for all RK3568 and RK3588 based boards, currently requires blobs from rkbin. Therefore, to build U-Boot properly, we need this package. Since there are publicly available boards based on RK3568, I assume we may eventually get support by someone (not necessarily you) for those. Therefore, I suggested we remove the mention to RK3588 as this is not specific to this SoC from Rockchip. No expectation from me on what you're going to do next :) [...] >>> +RK3588_DDRFW = rk3588_ddr_lp4_2112MHz_lp5_2736MHz_v1.11.bin >>> +RK3588_BL3FW = rk3588_bl31_v1.38.elf >>> + >>> +define ROCKCHIP_RKBIN_INSTALL_STAGING_CMDS >>> + cp $(@D)/bin/rk35/$(RK3588_BL3FW) $(BINARIES_DIR)/bl31.bin >> >> I would highly recommend to keep the same file extension to not confuse >> people since the OP-TEE binary apparently has different formats which we >> can (more easily) distinguish from their file extension (though IIRC, >> reading the header may be enough). > > Thanks. > Just to make things a bit clearer here, I commented on bl31.bin file extension while saying something about OP-TEE binary, the former is TF-A blob while the latter is bl32.bin. Also, I was wrong, the file extension isn't used for detecting the file format of OP-TEE OS blob, U-Boot tries to detect an ELF, if not, starts to parse the blob "header" to detect if it's v1 or v2 format. But better keep the same file extension though :) >>> + cp $(@D)/bin/rk35/$(RK3588_DDRFW) $(BINARIES_DIR)/ddr.bin >> >> I would highly recommend to not have a so common name which abstracts >> the SoC part number. > > Why is the SoC part number so important to you? > >> I can think of two ways of doing this: create a >> subdir after each SoC part number in BINARIES_DIR (e.g. >> BINARIES_DIR/rk3588/bl31.elf) or have the part number in the filename >> (e.g. rk3588_bl31.elf). >> This should allow to install the binary blobs from all SoC supported in >> RKBIN without having to deal with Kconfig options. > > How so? Because we would then copy all blobs at once into the BINARIES_DIR > everytime? > Indeed. Then we wouldn't need to care on the rkbin side which blobs to install in BINARIES_DIR. Inside u-boot.mk either, as I would pass the proper path as part of BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS like you did here? [...] >> Ok so another thought now... >> >> In boot/u-boot.mk, we already have >> ``` >> ifeq ($(BR2_TARGET_UBOOT_NEEDS_ATF_BL31),y) >> UBOOT_DEPENDENCIES += arm-trusted-firmware >> ifeq ($(BR2_TARGET_UBOOT_NEEDS_ATF_BL31_ELF),y) >> UBOOT_MAKE_OPTS += BL31=$(BINARIES_DIR)/bl31.elf >> define UBOOT_COPY_ATF_FIRMWARE >> cp $(BINARIES_DIR)/bl31.elf $(@D)/ >> endef >> UBOOT_PRE_BUILD_HOOKS += UBOOT_COPY_ATF_FIRMWARE >> else >> UBOOT_MAKE_OPTS += BL31=$(BINARIES_DIR)/bl31.bin >> define UBOOT_COPY_ATF_FIRMWARE >> cp $(BINARIES_DIR)/bl31.bin $(@D)/ >> endef >> UBOOT_PRE_BUILD_HOOKS += UBOOT_COPY_ATF_FIRMWARE >> endif >> endif >> >> ifeq ($(BR2_TARGET_UBOOT_NEEDS_OPTEE_TEE),y) >> UBOOT_DEPENDENCIES += optee-os >> UBOOT_MAKE_OPTS += TEE=$(BINARIES_DIR)/tee.elf >> endif >> ``` >> >> which should set everything up correctly for bl31 and tee-os, without >> needing to add BL31 and TEE to BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS manually. >> >> We would however probably need to go with a virtual package (or whatever >> the proper name for Buildroot would be for two packages providing the >> same functionality and have a selection mechanism in place in Kconfig >> for it) for TF-A and tee-os so that UBOOT_DEPENDENCIES picks up the >> correct dependency. RKBIN would be one of the possible provides for >> arm-trusted-firmware and optee-os for example. > > Or I just add the UBOOT_MAKE_OPTS += BL31=$(BINARIES_DIR)/bl31.bin to the > BR2_UBOOT_NEEDS_ROCKCHIP_RKBIN block? > Yeah, but technically U-Boot does need BR2_TARGET_UBOOT_NEEDS_ATF_BL31 for Aarch64 platforms. If the user selects BR2_TARGET_UBOOT_NEEDS_ATF_BL31, it'll add trusted-firmware-arm as a a dependency, which is unnecessary since we only have a blob. >> Looking at the code above, if we want to keep it a bit automagical, >> maybe having the RKBIN binaries called bl31.bin and tee.elf and have a >> Kconfig option for the RKBIN package to select which SoC binary blobs to >> install would be best. > > In the readme of the rkbin repository they present their file naming scheme. > However, I don't think I like making use of it: > > 1. We introduce a lot of Kconfig code for some logic that adds choices and > selects files within the rkbin repository. But in the end the user still has to > make the decision, which files are to use, anyway. Then the user can also just > directly specify the true filename in the board defconfig or .config. > What I had in mind was to have a select Kconfig option: choice config BR2_RK3588 config BR2_RK3568 enchoide and then in rockchip-rkbin (pseudo-code): if BR2_RK3588 install bin/rk35/rk3588_bl31_v1.38.elf $(BINARIES_DIR)/bl31.elf install bin/rk35/rk3588_bl32_v1.13.bin $(BINARIES_DIR)/tee.elf #add support for tee.bin in U-Boot mk? install bin/rk35/rk3588_ddr_lp4_2112MHz_lp5_2736MHz_v1.11.bin $(BINARIES_DIR)/ddr.bin elif BR2_RK3568 .... fi The important part would be for the destination filename to match what's expected in boot/u-boot/u-boot.mk: UBOOT_MAKE_OPTS += BL31=$(BINARIES_DIR)/bl31.elf [...] UBOOT_MAKE_OPTS += BL31=$(BINARIES_DIR)/bl31.bin in that scenario, we wouldn't even need to adapt U-Boot makefile other than allowing to depend on rockchip-rkbin rather than trusted-firmware-arm. > 2. We add a lot dependency: Rockchip may remove or rename files in their git > reblobitory as they like. If we (or any other users) then want to use a newer > commit of their reblobitory, e.g., to use newer blobs, and the file name / path > scheme breaks, we have to fix that again. > This is the responsibility of the package maintainer, like for any other package. I do agree though that the name of the installed binaries should stay consistent between new versions, so that we only need to update rockchip-rkbin and not any other package that depends on it. >> We would still however need a special case for the TPL coming from RKBIN >> package since there's no piping for this at the moment in boot/u-boot.mk. >> >> I feel like RKBIN package should be able to handle multiple SoCs (either >> by providing all SoCs binary blobs at once, or have a Kconfig option for >> it) and we should make it not too error-prone on the user-side for the >> U-Boot configuration. >> >> At the same time, I don't know if it's worth the added complexity. > > Thinking out loud: > > For the rockchip-rkbin package we could feature: > > * custom git version option (then defconfigs could freeze the repo version) > * option to specify tpl file name > * option to specify bl31 file name > * option to specify op-tee file name Well.. that's one thing I completely forgot about. Maybe people don't want to update rockchip-rkbin for their boards... SO yeah, I guess those 4 options actually make more sense than trying to be smart and have a select for each SoC. > * logic, which copies the specified files to BINARIES_DIR, while renaming them > to generic names (e.g., "bl31.elf") > > For the u-boot package we could add BR2_TARGET_UBOOT_NEEDS_ROCKCHIP_RKBIN in > uboot.mk: It adds the dependency for the package rockchip-rkbin and introduces > code, which parses the rockchip-rkbin BR2 variables and adds the needed > options to UBOOT_MAKE_OPTS accordingly. So it is very similar to > BR2_TARGET_UBOOT_NEEDS_ATF_BL31, but knows from the sprecified rockchip-rkbin > variables which uboot make opts to add (or not to add). > The thing I wanted to suggest was to reuse the code written for handling trusted-firmware-arm and optee-os dependencies on Buildroot level + environment variable setting for U-Boot building. The issue is that BR2_TARGET_UBOOT_NEEDS_ATF_BL31 depends on BR2_TARGET_ARM_TRUSTED_FIRMWARE so it'll bring unnecessary packages. If we give the user the choice of the ATF provider (from source, from blob, if blob from which package), then we can remove this dependency on trusted-firmware-arm package. ``` ifeq ($(BR2_TARGET_UBOOT_NEEDS_ATF_BL31),y) UBOOT_DEPENDENCIES += arm-trusted-firmware-provider # TODO add virtual package ifeq ($(BR2_TARGET_UBOOT_NEEDS_ATF_BL31_ELF),y) UBOOT_MAKE_OPTS += BL31=$(BINARIES_DIR)/bl31.elf define UBOOT_COPY_ATF_FIRMWARE cp $(BINARIES_DIR)/bl31.elf $(@D)/ endef UBOOT_PRE_BUILD_HOOKS += UBOOT_COPY_ATF_FIRMWARE else UBOOT_MAKE_OPTS += BL31=$(BINARIES_DIR)/bl31.bin define UBOOT_COPY_ATF_FIRMWARE cp $(BINARIES_DIR)/bl31.bin $(@D)/ endef UBOOT_PRE_BUILD_HOOKS += UBOOT_COPY_ATF_FIRMWARE endif endif ifeq ($(BR2_TARGET_UBOOT_NEEDS_OPTEE_TEE),y) UBOOT_DEPENDENCIES += optee-os-provider # TODO add virtual package UBOOT_MAKE_OPTS += TEE=$(BINARIES_DIR)/tee.elf endif ``` Then create optee-os-provider and trusted-firmware-arm-provider virtual packages the same way we do for openssl providers (libressl, libopenssl): package/openssl/Config.in package/openssl/openssl.mk Cheers, Quentin
Hi Quentin, > U-Boot, for all RK3568 and RK3588 based boards, currently requires blobs > from rkbin. Therefore, to build U-Boot properly, we need this package. > Since there are publicly available boards based on RK3568, I assume we > may eventually get support by someone (not necessarily you) for those. > Therefore, I suggested we remove the mention to RK3588 as this is not > specific to this SoC from Rockchip. No expectation from me on what > you're going to do next :) I think we agree here and as you know the rockchip-rkbin package is now able to also be used for other Rockchip SoCs. > >> I would highly recommend to keep the same file extension to not confuse > >> people since the OP-TEE binary apparently has different formats which we > >> can (more easily) distinguish from their file extension (though IIRC, > >> reading the header may be enough). > > > > Thanks. > > Just to make things a bit clearer here, I commented on bl31.bin file > extension while saying something about OP-TEE binary, the former is TF-A > blob while the latter is bl32.bin. > > Also, I was wrong, the file extension isn't used for detecting the file > format of OP-TEE OS blob, U-Boot tries to detect an ELF, if not, starts > to parse the blob "header" to detect if it's v1 or v2 format. > > But better keep the same file extension though :) Yes, I agree: it is still beneficial, if the true file extension is kept, even if it is technically not needed. In v6 of the patches I implemented it such, that the file extension is kept. > >> I can think of two ways of doing this: create a > >> subdir after each SoC part number in BINARIES_DIR (e.g. > >> BINARIES_DIR/rk3588/bl31.elf) or have the part number in the filename > >> (e.g. rk3588_bl31.elf). > >> This should allow to install the binary blobs from all SoC supported in > >> RKBIN without having to deal with Kconfig options. > > > > How so? Because we would then copy all blobs at once into the BINARIES_DIR > > everytime? > > Indeed. Then we wouldn't need to care on the rkbin side which blobs to > install in BINARIES_DIR. Inside u-boot.mk either, as I would pass the > proper path as part of BR2_TARGET_UBOOT_CUSTOM_MAKEOPTS like you did here? I actually don't like copying ALL Rockchip blobs into BINARIES_DIR everytime: Imagine this would become the common case for multiple packages. It would create a mess in my opinion. And it also is not needed: The package is now implemented such, that a user can optionally specify which blobs are needed and only those specified binaries are copied into BINARIES_DIR, with a generic name, while keeping the original file extension and also uboot now automatically identifies the binary, even so that the file extension can be different. What's not to like? ;) > >> Looking at the code above, if we want to keep it a bit automagical, > >> maybe having the RKBIN binaries called bl31.bin and tee.elf and have a > >> Kconfig option for the RKBIN package to select which SoC binary blobs to > >> install would be best. > > > > In the readme of the rkbin repository they present their file naming > > scheme. However, I don't think I like making use of it: > > > > 1. We introduce a lot of Kconfig code for some logic that adds choices and > > selects files within the rkbin repository. But in the end the user still > > has to make the decision, which files are to use, anyway. Then the user > > can also just directly specify the true filename in the board defconfig > > or .config. > What I had in mind was to have a select Kconfig option: > choice > config BR2_RK3588 > config BR2_RK3568 > enchoide > > and then in rockchip-rkbin (pseudo-code): > > if BR2_RK3588 > install bin/rk35/rk3588_bl31_v1.38.elf $(BINARIES_DIR)/bl31.elf > install bin/rk35/rk3588_bl32_v1.13.bin $(BINARIES_DIR)/tee.elf #add > support for tee.bin in U-Boot mk? > install bin/rk35/rk3588_ddr_lp4_2112MHz_lp5_2736MHz_v1.11.bin > $(BINARIES_DIR)/ddr.bin > elif BR2_RK3568 > .... > fi I don't like this: This would mean that we basically write down into the mk each binary for each SoC. If then rockchip decides to "update" a binary by simply renaming it (they have done this before, look at the git history), then the package is broken until some maintainer fixes it again. We would produce alot of code with imho no real benefit. In my oppinion: Whoever wants to use a blob from the repository should simply define which blob is the desired one. > The important part would be for the destination filename to match what's > expected in boot/u-boot/u-boot.mk: > > UBOOT_MAKE_OPTS += BL31=$(BINARIES_DIR)/bl31.elf > [...] > UBOOT_MAKE_OPTS += BL31=$(BINARIES_DIR)/bl31.bin > > in that scenario, we wouldn't even need to adapt U-Boot makefile other > than allowing to depend on rockchip-rkbin rather than trusted-firmware-arm. > > > 2. We add a lot dependency: Rockchip may remove or rename files in their > > git reblobitory as they like. If we (or any other users) then want to use > > a newer commit of their reblobitory, e.g., to use newer blobs, and the > > file name / path scheme breaks, we have to fix that again. > > This is the responsibility of the package maintainer, like for any other > package. I do agree though that the name of the installed binaries > should stay consistent between new versions, so that we only need to > update rockchip-rkbin and not any other package that depends on it. This takes unnecessary burden on the maintainer, and that is a bad thing in my opinion. I really think it is a huge plus, if a package is designed such, that it does not need much maintenance and that it is unlikely by design that it breaks due to external reasons (e.g., Rockchip unpredictably deciding to rename their blobs). After all, you never know, whether there is still a maintainer in the future who has time to maintain (or even wants to). > > > > Thinking out loud: > > > > For the rockchip-rkbin package we could feature: > > > > * custom git version option (then defconfigs could freeze the repo > > version) > > * option to specify tpl file name > > * option to specify bl31 file name > > * option to specify op-tee file name > > Well.. that's one thing I completely forgot about. Maybe people don't > want to update rockchip-rkbin for their boards... SO yeah, I guess those > 4 options actually make more sense than trying to be smart and have a > select for each SoC. Yes, I like this, too. I implemented it like this in v6 (and v5). Best regards, Kilian
diff --git a/DEVELOPERS b/DEVELOPERS index 188c579010..8e603ff3f8 100644 --- a/DEVELOPERS +++ b/DEVELOPERS @@ -1776,6 +1776,9 @@ F: package/ramsmp/ N: Kieran Bingham <kieran.bingham@ideasonboard.com> F: package/libcamera/ +N: Kilian Zinnecker <kilian.zinnecker@mail.de> +F: package/rockchip-rkbin/ + N: Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com> F: package/wqy-zenhei/ diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in index 085397d03d..8ec095a735 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 + U-Boot currently needs binary blobs during build for the + Rockchip RK3588 SoC. + 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..4ef0a7bc2a 100644 --- a/boot/uboot/uboot.mk +++ b/boot/uboot/uboot.mk @@ -207,6 +207,16 @@ 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_COPY_RKBIN_FW_FILES + cp $(BINARIES_DIR)/bl31.bin $(@D) + cp $(BINARIES_DIR)/ddr.bin $(@D) +endef +UBOOT_PRE_BUILD_HOOKS += UBOOT_COPY_RKBIN_FW_FILES +endif + ifeq ($(BR2_TARGET_UBOOT_NEEDS_DTC),y) UBOOT_DEPENDENCIES += host-dtc endif diff --git a/package/Config.in b/package/Config.in index bff090a661..80221d0406 100644 --- a/package/Config.in +++ b/package/Config.in @@ -581,6 +581,7 @@ endmenu source "package/read-edid/Config.in" source "package/rng-tools/Config.in" source "package/rockchip-mali/Config.in" + source "package/rockchip-rkbin/Config.in" source "package/rpi-userland/Config.in" source "package/rs485conf/Config.in" source "package/rtc-tools/Config.in" diff --git a/package/rockchip-rkbin/Config.in b/package/rockchip-rkbin/Config.in new file mode 100644 index 0000000000..d8072002ca --- /dev/null +++ b/package/rockchip-rkbin/Config.in @@ -0,0 +1,6 @@ +config BR2_PACKAGE_ROCKCHIP_RKBIN + bool "Rockchip RKBIN binary blobs" + depends on BR2_arm || BR2_aarch64 + help + This package provides binary blobs, currently needed by + u-boot for the Rockchip RK3588 SoC. diff --git a/package/rockchip-rkbin/rockchip-rkbin.mk b/package/rockchip-rkbin/rockchip-rkbin.mk new file mode 100644 index 0000000000..ac50c8931d --- /dev/null +++ b/package/rockchip-rkbin/rockchip-rkbin.mk @@ -0,0 +1,23 @@ +################################################################################ +# +# rockchip-rkbin +# +################################################################################ + + +ROCKCHIP_RKBIN_VERSION = d6ccfe401ca84a98ca3b85c12b9554a1a43a166c +ROCKCHIP_RKBIN_SITE = https://github.com/rockchip-linux/rkbin.git +ROCKCHIP_RKBIN_SITE_METHOD = git + +ROCKCHIP_RKBIN_INSTALL_STAGING = YES +ROCKCHIP_RKBIN_INSTALL_TARGET = NO + +RK3588_DDRFW = rk3588_ddr_lp4_2112MHz_lp5_2736MHz_v1.11.bin +RK3588_BL3FW = rk3588_bl31_v1.38.elf + +define ROCKCHIP_RKBIN_INSTALL_STAGING_CMDS + cp $(@D)/bin/rk35/$(RK3588_BL3FW) $(BINARIES_DIR)/bl31.bin + cp $(@D)/bin/rk35/$(RK3588_DDRFW) $(BINARIES_DIR)/ddr.bin +endef + +$(eval $(generic-package))
This patch adds a package for the Rockchip ATF binary blobs. These binaries are currently needed if U-Boot is built for the Rockchip RK3588 SoC. In addition the uboot package is modified such, that it can depend on the rockchip-rkbin package and in that case automatically obtain the needed binary blobs. Signed-off-by: Kilian Zinnecker <kilian.zinnecker@mail.de> --- DEVELOPERS | 3 +++ boot/uboot/Config.in | 9 +++++++++ boot/uboot/uboot.mk | 10 ++++++++++ package/Config.in | 1 + package/rockchip-rkbin/Config.in | 6 ++++++ package/rockchip-rkbin/rockchip-rkbin.mk | 23 +++++++++++++++++++++++ 6 files changed, 52 insertions(+) create mode 100644 package/rockchip-rkbin/Config.in create mode 100644 package/rockchip-rkbin/rockchip-rkbin.mk