Re: [PATCH V2] imx: imx93_11x11_evk: using binman to pack images

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

On 13/06/2022 09:13, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Use BINMAN to generate flash.bin
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> 
> Based on
> https://patchwork.ozlabs.org/project/uboot/cover/20220611132035.32698-1-peng.fan@oss.nxp.com/
> 
>  V2:
>   Typo correct
> 
>  arch/arm/dts/imx93-11x11-evk-u-boot.dtsi |  2 +
>  arch/arm/dts/imx93-u-boot.dtsi           | 84 ++++++++++++++++++++++++
>  arch/arm/mach-imx/Makefile               | 24 +++++++
>  arch/arm/mach-imx/imx9/Kconfig           |  1 +
>  arch/arm/mach-imx/imx9/container.cfg     | 11 ++++
>  arch/arm/mach-imx/imx9/imximage.cfg      | 15 +++++
>  board/freescale/imx93_evk/Kconfig        |  6 ++
>  7 files changed, 143 insertions(+)
>  create mode 100644 arch/arm/dts/imx93-u-boot.dtsi
>  create mode 100644 arch/arm/mach-imx/imx9/container.cfg
>  create mode 100644 arch/arm/mach-imx/imx9/imximage.cfg
> 
> diff --git a/arch/arm/dts/imx93-11x11-evk-u-boot.dtsi b/arch/arm/dts/imx93-11x11-evk-u-boot.dtsi
> index 6f02b389893..e5912a85ca2 100644
> --- a/arch/arm/dts/imx93-11x11-evk-u-boot.dtsi
> +++ b/arch/arm/dts/imx93-11x11-evk-u-boot.dtsi
> @@ -3,6 +3,8 @@
>   * Copyright 2022 NXP
>   */
>  
> +#include "imx93-u-boot.dtsi"
> +
>  / {
>  	wdt-reboot {
>  		compatible = "wdt-reboot";
> diff --git a/arch/arm/dts/imx93-u-boot.dtsi b/arch/arm/dts/imx93-u-boot.dtsi
> new file mode 100644
> index 00000000000..2b6bfd0cb44
> --- /dev/null
> +++ b/arch/arm/dts/imx93-u-boot.dtsi
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright 2022 NXP
> + * Peng Fan <peng.fan@nxp.com>
> + */
> +
> +/ {
> +	binman: binman {
> +		multiple-images;
> +	};
> +};
> +
> +&binman {
> +	 u-boot-spl-ddr {
> +		filename = "u-boot-spl-ddr.bin";
> +		pad-byte = <0xff>;
> +		align-size = <4>;
> +		align = <4>;
> +
> +		u-boot-spl {
> +			align-end = <4>;
> +		};
> +
> +		blob_1: blob-ext@1 {
> +			filename = "lpddr4_imem_1d_v202201.bin";
> +			size = <0x8000>;
> +		};
> +
> +		blob_2: blob-ext@2 {
> +			filename = "lpddr4_dmem_1d_v202201.bin";
> +			size = <0x4000>;
> +		};
> +
> +		blob_3: blob-ext@3 {
> +			filename = "lpddr4_imem_2d_v202201.bin";
> +			size = <0x8000>;
> +		};
> +
> +		blob_4: blob-ext@4 {
> +			filename = "lpddr4_dmem_2d_v202201.bin";
> +			size = <0x4000>;
> +		};

These look like the same kind of files as those in your i.MX8M binman
symbols series. So I suggest these can be named like 'ddr-1d-imem-fw' as
well.

> +	};
> +
> +	spl {
> +		filename = "spl.bin";
> +
> +		mkimage {
> +			args = "-n spl/u-boot-spl.cfgout -T imx8image -e 0x2049a000";
> +
> +			blob {
> +				filename = "u-boot-spl-ddr.bin";
> +			};
> +		};
> +	};
> +
> +	u-boot-container {
> +		filename = "u-boot-container.bin";
> +
> +		mkimage {
> +			args = "-n u-boot-container.cfgout -T imx8image -e 0x0";
> +
> +			blob {
> +				filename = "u-boot.bin";
> +			};
> +		};
> +	};

The entire thing looks similar to i.MX8M, except for this custom
container. Why not a FIT image here?

> +
> +	imx-boot {
> +		filename = "flash.bin";
> +		pad-byte = <0x00>;
> +
> +		spl: blob-ext@1 {
> +			filename = "spl.bin";
> +			offset = <0x0>;
> +			align-size = <0x400>;
> +			align = <0x400>;
> +		};
> +
> +		uboot: blob-ext@2 {
> +			filename = "u-boot-container.bin";
> +		};
> +	};
> +};
> diff --git a/arch/arm/mach-imx/Makefile b/arch/arm/mach-imx/Makefile
> index 71ac8e6ffde..2f51226e0d9 100644
> --- a/arch/arm/mach-imx/Makefile
> +++ b/arch/arm/mach-imx/Makefile
> @@ -120,6 +120,9 @@ DEPFILE_EXISTS := $(shell $(CPP) $(cpp_flags) -x c -o u-boot-dtb.cfgout $(srctre
>  else ifeq ($(CONFIG_ARCH_IMX8M), y)
>  IMAGE_TYPE := imx8mimage
>  DEPFILE_EXISTS := 0
> +else ifeq ($(CONFIG_ARCH_IMX9), y)
> +IMAGE_TYPE := imx8image
> +DEPFILE_EXISTS := 0
>  else
>  IMAGE_TYPE := imximage
>  DEPFILE_EXISTS := 0
> @@ -171,6 +174,27 @@ flash.bin: spl/u-boot-spl-ddr.bin u-boot.itb FORCE
>  	$(call if_changed,mkimage)
>  endif
>  
> +ifeq ($(CONFIG_ARCH_IMX9), y)
> +
> +SPL: spl/u-boot-spl.bin spl/u-boot-spl.cfgout u-boot-container.cfgout FORCE
> +
> +MKIMAGEFLAGS_flash.bin = -n spl/u-boot-spl.cfgout -T $(IMAGE_TYPE) -e $(CONFIG_SPL_TEXT_BASE)
> +flash.bin: MKIMAGEOUTPUT = flash.log
> +
> +spl/u-boot-spl.cfgout: $(IMX_CONFIG) FORCE
> +	$(Q)mkdir -p $(dir $@)
> +	$(call if_changed_dep,cpp_cfg)
> +
> +spl/u-boot-spl-ddr.bin: spl/u-boot-spl.bin spl/u-boot-spl.cfgout FORCE
> +
> +u-boot-container.cfgout: $(IMX_CONTAINER_CFG) FORCE
> +	$(Q)mkdir -p $(dir $@)
> +	$(call if_changed_dep,cpp_cfg)
> +
> +flash.bin: spl/u-boot-spl-ddr.bin container.cfgout FORCE
> +	$(call if_changed,mkimage)
> +endif
> +

I have a feeling these rules shouldn't be here when using binman, at
least those for the files binman builds. I see similar rules for IMX8
and IMX8M too. Maybe all should check for CONFIG_BINMAN?

>  ifeq ($(CONFIG_ARCH_IMX8), y)
>  SPL:
>  
> diff --git a/arch/arm/mach-imx/imx9/Kconfig b/arch/arm/mach-imx/imx9/Kconfig
> index c06102bae07..0b965376987 100644
> --- a/arch/arm/mach-imx/imx9/Kconfig
> +++ b/arch/arm/mach-imx/imx9/Kconfig
> @@ -25,6 +25,7 @@ choice
>  config TARGET_IMX93_11X11_EVK
>  	bool "imx93_11x11_evk"
>  	select IMX93
> +	select BINMAN
>  
>  endchoice
>  
> diff --git a/arch/arm/mach-imx/imx9/container.cfg b/arch/arm/mach-imx/imx9/container.cfg
> new file mode 100644
> index 00000000000..baaa17748ff
> --- /dev/null
> +++ b/arch/arm/mach-imx/imx9/container.cfg
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright 2022 NXP
> + */
> +
> +/* This file is to create a container image could be loaded by SPL */
> +BOOT_FROM SD 0x400
> +SOC_TYPE IMX9
> +CONTAINER
> +IMAGE A55 bl31.bin	0x204E0000
> +IMAGE A55 u-boot.bin	CONFIG_SYS_TEXT_BASE
> diff --git a/arch/arm/mach-imx/imx9/imximage.cfg b/arch/arm/mach-imx/imx9/imximage.cfg
> new file mode 100644
> index 00000000000..fae0c64245f
> --- /dev/null
> +++ b/arch/arm/mach-imx/imx9/imximage.cfg
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright 2022 NXP
> + */
> +
> +/* Boot from SD, sector size 0x400 */
> +BOOT_FROM SD 0x400
> +/* SoC type IMX9 */
> +SOC_TYPE IMX9
> +/* Append sentinel container image */
> +APPEND mx93a0-ahab-container.img

I tried building for imx93_11x11_evk, but it fails because I don't have
this file:

  ValueError: Error 1 running 'mkimage -d ./mkimage.spl.mkimage \
    -n spl/u-boot-spl.cfgout -T imx8image -e 0x2049a000 \
    ./mkimage-out.spl.mkimage': \
    Fail open first container file mx93a0-ahab-container.img

This is hard to handle because binman doesn't know that you use this
file (it's only mentioned in the cfg), and can't create a 'fake' file
when it's missing. Even if it did, it looks like mkimage parses this
file for a header, so a fake file doesn't work...

I don't know how best to solve this, I have to think more. Is the file
optional? Because I see similar ahab-container.img used for some i.MX8
(only Q?) but not for i.MX8M.

An imx-image binman entry type would make it easier. For example you
could put the file as a blob-ext inside the imx-image entry, then
imx-image could know when the blob is missing and do something else.

> +/* Create the 2nd container */
> +CONTAINER
> +/* Add spl with exec attribute */
> +IMAGE A55 u-boot-spl-ddr.bin 0x2049A000
> diff --git a/board/freescale/imx93_evk/Kconfig b/board/freescale/imx93_evk/Kconfig
> index 032e523198d..17209d939d2 100644
> --- a/board/freescale/imx93_evk/Kconfig
> +++ b/board/freescale/imx93_evk/Kconfig
> @@ -16,6 +16,12 @@ config IMX93_EVK_LPDDR4X
>  	help
>  	  Select the LPDDR4X timing and 0.6V VDDQ
>  
> +config IMX_CONFIG
> +	default "arch/arm/mach-imx/imx9/imximage.cfg"
> +
> +config IMX_CONTAINER_CFG
> +	default "arch/arm/mach-imx/imx9/container.cfg"
> +
>  source "board/freescale/common/Kconfig"
>  
>  endif