Re: [RESEND, RFC 6/8] binman: dtsi: sysfw: j721e: Use binman to package sysfw.itb

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

On 06/04/2022 15:29, Neha Malcom Francis wrote:
> By providing entries in the binman node of the device tree, binman will
> be able to find and package board config binary artifacts generated by
> TIBoardConfig with sysfw.bin and generate the final image sysfw.itb.
> 
> j721e-r5-binman.dtsi has been introduced for R5 specific binman node. It
> can be then be include by files that require it like
> k3-j721e-r5-common-proc-board-u-boot.dtsi.
> 
> 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-r5-binman.dtsi             | 75 +++++++++++++++++++
>  .../k3-j721e-r5-common-proc-board-u-boot.dtsi |  1 +
>  board/ti/j721e/Kconfig                        |  1 +
>  3 files changed, 77 insertions(+)
>  create mode 100644 arch/arm/dts/j721e-r5-binman.dtsi
> 
> diff --git a/arch/arm/dts/j721e-r5-binman.dtsi b/arch/arm/dts/j721e-r5-binman.dtsi
> new file mode 100644
> index 0000000000..6e69084eaa
> --- /dev/null
> +++ b/arch/arm/dts/j721e-r5-binman.dtsi

You should prefix this name with 'k3-' for consistency with other files.

> @@ -0,0 +1,75 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +// Copyright (C) 2022 Texas Instruments Incorporated - https://www.ti.com/
> +
> +#include <config.h>
> +
> +/ {
> +	binman: binman {
> +		multiple-images;
> +	};
> +};
> +
> +&binman {
> +	binary {
> +		filename = "sysfw.bin";
> +		sysfw {
> +			filename = "ti-fs-firmware-j721e-gp.bin";
> +			device = "j721e";
> +			load = <0x0040000>;
> +		};
> +	};

This confuses me a bit, because so far I've understood 'sysfw.bin' to be
the unsigned file (as in your entry type's test).

Also, doesn't look like the 'device' property is used in the entry type
at all. What is it meant to do?

> +	itb {
> +		filename = "sysfw.itb";
> +		fit {
> +			description = "SYSFW and Config Fragments";
> +			#address-cells = <1>;
> +			images {
> +				sysfw.bin {
> +					description = "sysfw";
> +					type = "firmware";
> +					arch = "arm";
> +					compression = "none";
> +					blob-ext {
> +						filename = "sysfw.bin";
> +					};

This is meant to be the signed 'sysfw.bin' you're creating with binman
above, right? I'm not sure if including binman-built files like this is
guaranteed to work.

> +				};
> +				board-cfg.bin {
> +					description = "board-cfg";
> +					type = "firmware";
> +					arch = "arm";
> +					compression = "none";
> +					blob-ext {
> +						filename = "board-cfg.bin";
> +					};
> +				};
> +				pm-cfg.bin {
> +					description = "pm-cfg";
> +					type = "firmware";
> +					arch = "arm";
> +					compression = "none";
> +					blob-ext {
> +						filename = "pm-cfg.bin";
> +					};
> +				};
> +				rm-cfg.bin {
> +					description = "rm-cfg";
> +					type = "firmware";
> +					arch = "arm";
> +					compression = "none";
> +					blob-ext {
> +						filename = "rm-cfg.bin";
> +					};
> +				};
> +				sec-cfg.bin {
> +					description = "sec-cfg";
> +					type = "firmware";
> +					arch = "arm";
> +					compression = "none";
> +					blob-ext {
> +						filename = "sec-cfg.bin";
> +					};
> +				};

Since you're generating these in the build now, 'blob' might be more
appropriate than 'blob-ext'.

> +			};
> +		};
> +	};
> +};
> diff --git a/arch/arm/dts/k3-j721e-r5-common-proc-board-u-boot.dtsi b/arch/arm/dts/k3-j721e-r5-common-proc-board-u-boot.dtsi
> index 48c6ddf672..75fae60a97 100644
> --- a/arch/arm/dts/k3-j721e-r5-common-proc-board-u-boot.dtsi
> +++ b/arch/arm/dts/k3-j721e-r5-common-proc-board-u-boot.dtsi
> @@ -4,6 +4,7 @@
>   */
>  
>  #include "k3-j721e-common-proc-board-u-boot.dtsi"
> +#include "j721e-r5-binman.dtsi"
>  
>  / {
>  	chosen {
> diff --git a/board/ti/j721e/Kconfig b/board/ti/j721e/Kconfig
> index c28752a658..a3a9d504ae 100644
> --- a/board/ti/j721e/Kconfig
> +++ b/board/ti/j721e/Kconfig
> @@ -24,6 +24,7 @@ config TARGET_J721E_R5_EVM
>  	select RAM
>  	select SPL_RAM
>  	select K3_DDRSS
> +	select BINMAN
>  	imply SYS_K3_SPL_ATF
>  	imply TI_I2C_BOARD_DETECT
>