Re: [RESEND, RFC 8/8] binman: dtsi: tispl: j721e: Use binman to package tispl.bin

[Alper Nebi Yasak <alpernebiyasak@gmail.com>]

On 06/04/2022 15:29, Neha Malcom Francis wrote:
> Explicit make commands were earlier used to generate tispl.bin image,
> now it is replaced using binman.
> 
> Binman picks up and packages entries according to the description of
> entries given in the binman node in the device tree. The make commands
> that were earlier responsible for generating tispl.bin has been removed.
> 
> j721e-a72-binman.dtsi has been introduced for A72 specific binman node.
> It can be included in files that require it like
> k3-j721e-common-proc-board-u-boot.dtsi.
> 
> Note that make commands for secure devices has also been removed as
> focus is on general purpose devices at present time.
> 
> Signed-off-by: Tarun Sahu <t-sahu@ti.com>
> [n-francis@ti.com: prepared patch for upstreaming]
> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
> ---
>  arch/arm/dts/j721e-a72-binman.dtsi            | 92 +++++++++++++++++++
>  .../k3-j721e-common-proc-board-u-boot.dtsi    |  1 +
>  arch/arm/mach-k3/config.mk                    | 33 -------
>  board/ti/j721e/Kconfig                        |  1 +
>  scripts/Makefile.spl                          |  4 -
>  5 files changed, 94 insertions(+), 37 deletions(-)
>  create mode 100644 arch/arm/dts/j721e-a72-binman.dtsi
> 
> diff --git a/arch/arm/dts/j721e-a72-binman.dtsi b/arch/arm/dts/j721e-a72-binman.dtsi

I'd like a 'k3-' prefix also for this file.

> new file mode 100644
> index 0000000000..a01b1fcc6d
> --- /dev/null
> +++ b/arch/arm/dts/j721e-a72-binman.dtsi
> @@ -0,0 +1,92 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// Copyright (C) 2022 Texas Instruments Incorporated - https://www.ti.com/
> +
> +#include <config.h>
> +
> +#ifdef CONFIG_ARM64
> +/ {
> +	binman: binman {
> +		multiple-images;
> +	};
> +};
> +
> +&binman {
> +	tispl {
> +		filename = "tispl.bin";
> +		fit {
> +			description = "FIT IMAGE";
> +			#address-cells = <1>;
> +			fit,fdt-list = "of-list";

I guess this prop isn't necessary as you aren't using @fdt-SEQ etc.

> +			images {
> +				atf {
> +					description = "ARM Trusted Firmware";
> +					type = "firmware";
> +					arch = "arm64";
> +					compression = "none";
> +					os = "arm-trusted-firmware";
> +					load = <CONFIG_K3_ATF_LOAD_ADDR>;
> +					entry = <CONFIG_K3_ATF_LOAD_ADDR>;
> +					atf-bl31 {
> +						filename = "bl31.bin";
> +					};

AFAICT, these named-by-arg blobs shouldn't need you to specify a
filename, they can be handled by environment variables like BL31 etc.
But I see a few instances of this in other files so I'm not sure.

> +				};
> +				tee {
> +					description = "OPTEE";
> +					type = "tee";
> +					arch = "arm64";
> +					compression = "none";
> +					os = "tee";
> +					load = <0x9e800000>;
> +					entry = <0x9e800000>;
> +					tee-os {
> +						filename = "tee-pager.bin";
> +						missing-msg = "tee-os";
> +					};
> +				};
> +				dm {
> +					description = "DM binary";
> +					type = "firmware";
> +					arch = "arm32";
> +					compression = "none";
> +					os = "DM";
> +					load = <0x89000000>;
> +					entry = <0x89000000>;
> +					ti-dm {
> +						filename = "dm.bin";
> +					};
> +				};
> +				spl {
> +					description = "SPL (64-bit)";
> +					type = "standalone";
> +					os = "U-Boot";
> +					arch = "arm64";
> +					compression = "none";
> +					load = <0x80080000>;
> +					entry = <0x80080000>;

Maybe these could be CONFIG_SPL_TEXT_BASE? I don't exactly know.

> +					blob-ext {
> +						filename = "spl/u-boot-spl-nodtb.bin";
> +					};

This should be a u-boot-spl-nodtb entry.

> +				};
> +				k3-j721e-common-proc-board.dtb {
> +					description = "k3-j721e-common-proc-board";
> +					type = "flat_dt";
> +					arch = "arm";
> +					compression = "none";
> +					blob-ext {
> +					filename = "spl/dts/k3-j721e-common-proc-board.dtb";
> +					};

And this should be a u-boot-spl-dtb entry, assuming this dtb is the same
as spl/u-boot-spl.dtb.

> +				};
> +			};
> +			configurations {
> +				default = "conf";
> +				conf {
> +					description = "k3-j721e-common-proc-board";
> +					firmware = "atf";
> +					loadables = "tee", "dm", "spl";
> +					fdt = "k3-j721e-common-proc-board.dtb";
> +				};
> +			};
> +		};
> +	};
> +};
> +#endif
> diff --git a/arch/arm/dts/k3-j721e-common-proc-board-u-boot.dtsi b/arch/arm/dts/k3-j721e-common-proc-board-u-boot.dtsi
> index 677a72d2a2..78ec6b1d48 100644
> --- a/arch/arm/dts/k3-j721e-common-proc-board-u-boot.dtsi
> +++ b/arch/arm/dts/k3-j721e-common-proc-board-u-boot.dtsi
> @@ -4,6 +4,7 @@
>   */
>  
>  #include <dt-bindings/net/ti-dp83867.h>
> +#include "j721e-a72-binman.dtsi"
>  
>  / {
>  	chosen {
> diff --git a/arch/arm/mach-k3/config.mk b/arch/arm/mach-k3/config.mk
> index 5491fc4dc8..e4b94564b8 100644
> --- a/arch/arm/mach-k3/config.mk
> +++ b/arch/arm/mach-k3/config.mk
> @@ -72,38 +72,5 @@ ifeq ($(CONFIG_SOC_K3_J721E),)
>  export DM := /dev/null
>  endif
>  
> -ifeq ($(CONFIG_TI_SECURE_DEVICE),y)
> -SPL_ITS := u-boot-spl-k3_HS.its
> -$(SPL_ITS): export IS_HS=1
> -INPUTS-y	+= tispl.bin_HS
> -else
> -SPL_ITS := u-boot-spl-k3.its
> -INPUTS-y	+= tispl.bin
> -endif
> -
> -ifeq ($(CONFIG_SPL_OF_LIST),)
> -LIST_OF_DTB := $(CONFIG_DEFAULT_DEVICE_TREE)
> -else
> -LIST_OF_DTB := $(CONFIG_SPL_OF_LIST)
>  endif
> -
> -quiet_cmd_k3_mkits = MKITS   $@
> -cmd_k3_mkits = \
> -	$(srctree)/tools/k3_fit_atf.sh \

Doesn't look like anything else uses k3_fit_atf.sh, you might want to
remove the script in a new patch.

> -	$(CONFIG_K3_ATF_LOAD_ADDR) \
> -	$(patsubst %,$(obj)/dts/%.dtb,$(subst ",,$(LIST_OF_DTB))) > $@
> -
> -$(SPL_ITS): FORCE
> -	$(call cmd,k3_mkits)
>  endif
> -
> -else
> -
> -ifeq ($(CONFIG_TI_SECURE_DEVICE),y)
> -INPUTS-y	+= u-boot.img_HS
> -else
> -INPUTS-y	+= u-boot.img
> -endif
> -endif
> -
> -include $(srctree)/arch/arm/mach-k3/config_secure.mk
> diff --git a/board/ti/j721e/Kconfig b/board/ti/j721e/Kconfig
> index a3a9d504ae..3cf05f0d3b 100644
> --- a/board/ti/j721e/Kconfig
> +++ b/board/ti/j721e/Kconfig
> @@ -14,6 +14,7 @@ config TARGET_J721E_A72_EVM
>  	select BOARD_LATE_INIT
>  	imply TI_I2C_BOARD_DETECT
>  	select SYS_DISABLE_DCACHE_OPS
> +	select BINMAN
>  
>  config TARGET_J721E_R5_EVM
>  	bool "TI K3 based J721E EVM running on R5"
> diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl
> index 83a95ee4aa..21dc434449 100644
> --- a/scripts/Makefile.spl
> +++ b/scripts/Makefile.spl
> @@ -574,7 +574,3 @@ $(obj)/$(SPL_BIN).multidtb.fit.gz: $(obj)/$(SPL_BIN).multidtb.fit
>  $(obj)/$(SPL_BIN).multidtb.fit.lzo: $(obj)/$(SPL_BIN).multidtb.fit
>  	@lzop -f9 $< > $@
>  
> -ifdef CONFIG_ARCH_K3
> -tispl.bin: $(obj)/u-boot-spl-nodtb.bin $(SHRUNK_ARCH_DTB) $(SPL_ITS) FORCE
> -	$(call if_changed,mkfitimage)
> -endif