[Buildroot] [PATCH] boot/uboot: add support for bundling TEE in BIN format into U-Boot

[Arnout Vandecappelle]

On 01/02/2022 10:13, Michael Trimarchi wrote:
> Some U-Boot platforms (e.g. nxp) can bundle OPTEE's tee.bin
> into the U-Boot image using binman. This patch brings the necessary changes to
> enable this feature.
> 
> Signed-off-by: Michael Trimarchi <michael at amarulasolutions.com>
> ---
>   boot/uboot/Config.in | 12 ++++++++++++
>   boot/uboot/uboot.mk  | 12 ++++++++++++
>   2 files changed, 24 insertions(+)
> 
> diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
> index c630fc6552..117bbd3faf 100644
> --- a/boot/uboot/Config.in
> +++ b/boot/uboot/Config.in
> @@ -237,6 +237,18 @@ config BR2_TARGET_UBOOT_NEEDS_OPTEE_TEE
>   	  U-Boot. This option makes sure optee-os gets built prior to
>   	  U-Boot, and that the TEE variable pointing to OPTEE's
>   	  tee.elf, is passed during the Buildroot build.
           ^^^^^^^ This is inconsistent with the choice below. Perhaps 
reformulate like this:

	  U-Boot, and that the TEE variable pointing to OPTEE's binary
	  is passed during the Buildroot build.


> +choice
> +	prompt "U-Boot OPTEE BL32 format"
> +	default BR2_TARGET_UBOOT_NEEDS_OPTEE_BIN
> +	depends on BR2_TARGET_UBOOT_NEEDS_OPTEE_TEE

  (nitpick) IMHO it's nicer to write this with if...endif around the choice.

  More importantly: there should be a help text.

> +
> +config BR2_TARGET_UBOOT_NEEDS_OPTEE_BIN
> +	bool "tee.bin"
> +
> +config BR2_TARGET_UBOOT_NEEDS_OPTEE_ELF
> +	bool "tee.elf"
> +
> +endchoice
>   
>   config BR2_TARGET_UBOOT_NEEDS_OPENSBI
>   	bool "U-Boot needs OpenSBI"
> diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
> index 574fc7089a..210fa219ed 100644
> --- a/boot/uboot/uboot.mk
> +++ b/boot/uboot/uboot.mk
> @@ -177,7 +177,19 @@ endif
>   
>   ifeq ($(BR2_TARGET_UBOOT_NEEDS_OPTEE_TEE),y)
>   UBOOT_DEPENDENCIES += optee-os
> +ifeq ($(BR2_TARGET_UBOOT_NEEDS_OPTEE_ELF),y)
>   UBOOT_MAKE_OPTS += TEE=$(BINARIES_DIR)/tee.elf
> +define UBOOT_COPY_TEE_FIRMWARE
> +	cp $(BINARIES_DIR)/tee.elf $(@D)/

  As Yann explained but it didn't appear to get through: before, it supposedly 
wasn't necessary to make this copy, so why is it necessary now?

  Perhaps it is necessary to copy it in more recent versions of U-Boot (i.e. 
when using binman). In that case, I think it would be good to also update the 
make option to TEE=tee.elf, to make it more consistent.


> +endef
> +UBOOT_PRE_BUILD_HOOKS += UBOOT_COPY_TEE_FIRMWARE
> +else
> +UBOOT_MAKE_OPTS += BL32=$(BINARIES_DIR)/tee.bin
> +define UBOOT_COPY_TEE_FIRMWARE
> +	cp $(BINARIES_DIR)/tee-raw.bin $(@D)/tee.bin

  Again, it's very weird that you pass in a BL32= that refers to a different 
file (really different this time, because it's not the tee-raw.bin but the full 
tee.bin).

  There may be good reasons to do it this way, but that should be explained in 
detail in the commit message, and also briefly in a comment because it's simply 
too weird to understand like this.


  I've marked the patch as Changes Requested.

  Regards,
  Arnout


> +endef
> +UBOOT_PRE_BUILD_HOOKS += UBOOT_COPY_TEE_FIRMWARE
> +endif
>   endif
>   
>   ifeq ($(BR2_TARGET_UBOOT_NEEDS_OPENSBI),y)