Re: [PATCH v2 1/7] rockchip: generate idbloader.img content for u-boot-rockchip.bin with binman for ARM

[Johan Jonker <jbx6244@gmail.com>]

Hi Quentin and others,

Some comments. Have a look if it's useful.
It works, but is in need for some improvement...

Johan

On 7/22/22 13:34, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> 
> idbloader.img content - currently created by way of Makefile - can be
> created by binman directly.
> 
> So let's do that for Rockchip ARM platforms.
> 
> Cc: Quentin Schulz <foss+uboot@0leil.net>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>  Makefile                          |  2 +-
>  arch/arm/dts/rockchip-u-boot.dtsi | 15 +++++++++++++--
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index d76ec69b52..f780bfe211 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1005,7 +1005,7 @@ endif
>  else
>  ifeq ($(CONFIG_SPL),y)
>  # Generate these inputs for binman which will create the output files
> -INPUTS-y += idbloader.img u-boot.img
> +INPUTS-y += u-boot.img
>  endif
>  endif
>  endif
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index eae3ee715d..0362c97e0b 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -17,9 +17,20 @@
>  		filename = "u-boot-rockchip.bin";
>  		pad-byte = <0xff>;
>  

> -		blob {
> -			filename = "idbloader.img";

"u-boot-rockchip.bin" is a combination image of mkimage(TPL/SPL) + "u-boot.img". Not everyone suites this fixed GPT format.
People may still want to write them separate while testing or whatever, so "idbloader.img" and "u-boot.img" must be kept available after this change!

===

rockchip.rst and README.rockchip and elsewhere on the internet still refer to "idbloader.img" so it should come back, but then made by binman.


	idbloader {
		filename = "idbloader.img";

		mkimage {
			args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
#ifdef CONFIG_TPL
			multiple-data-files;

			u-boot-tpl {
			};
#endif
			u-boot-spl {
			};
		};
	};

===

After this patch serie "idbloader.img" is not removed.

make clean

After:
  CLEAN   include/generated/env.in u-boot-nodtb.bin u-boot.lds u-boot.cfg.configs u-boot.bin u-boot-dtb.bin u-boot.img u-boot-dtb.img u-boot.sym u-boot.map u-boot.srec u-boot.cfg u-boot.dtb.out u-boot-tpl.dtb.out u-boot u-boot-spl.dtb.out u-boot.dtb u-boot-rockchip.bin System.map

Before:
  CLEAN   include/generated/env.in u-boot-nodtb.bin u-boot.lds u-boot.cfg.configs u-boot.bin u-boot-dtb.bin u-boot.img u-boot-dtb.img u-boot.sym u-boot.map u-boot.srec u-boot.cfg u-boot.dtb.out u-boot-tpl.dtb.out u-boot u-boot-spl.dtb.out u-boot.dtb u-boot-rockchip.bin System.map idbloader.img

===

Your serie generates zombie files. PLease remove after use.
Same for others like:

simple-bin.map
mkimage-out.rom.mkimage
mkimage.rom.mkimage
rom.map
tools/boot/bootm.c
tools/boot/fdt_region.c
tools/boot/image-cipher.c
tools/boot/image-fit-sig.c
tools/boot/image-fit.c
tools/boot/image-host.c
tools/boot/image.c
u_boot_logo.S


Untracked files:
  (use "git add <file>..." to include in what will be committed)

	mkimage-out.simple-bin.mkimage
	mkimage.simple-bin.mkimage

===



===

> +		mkimage {
> +			args = "-n", CONFIG_SYS_SOC, "-T", "rksd";

> +#ifndef CONFIG_TPL
> +			u-boot-spl {
> +			};
>  		};
> +#else
> +			u-boot-tpl {
> +			};
> +		};
> +

u-boot-spl is the input for mkimage and should be a subnode.

> +		u-boot-spl {
> +		};
> +#endif

Fix your dts format:

	simple-bin {
		filename = "u-boot-rockchip.bin";
		pad-byte = <0xff>;

		mkimage {
			args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
#ifdef CONFIG_TPL
			multiple-data-files;

			u-boot-tpl {
			};
#endif
			u-boot-spl {
			};
		};

#ifdef CONFIG_ARM64
		blob {
			filename = "u-boot.itb";
#else
		u-boot-img {
#endif
			offset = <((CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR - 64) * 512)>;
		};
	};
===

CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR is only related to MMC!

There are other boot mediums like NAND or USB, so don't assume that CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_SECTOR is defined.

See my patch serie that still is in need for review:

https://lore.kernel.org/u-boot/20220508150825.21711-7-jbx6244@gmail.com/

Without it generates a warning:

Error: arch/arm/dts/rockchip-u-boot.dtsi:54.16-17 syntax error
FATAL ERROR: Unable to parse input tree

Add more compile conditions!

===

RK3066:
For NAND the "idbloader.img" might be useful for my serie (in need for review) when it gets TPL/SPL and rc4 right:

[PATCH v2 00/11] Add Rockchip IDB device
https://lore.kernel.org/u-boot/a1458a7b-2043-6397-3107-2d1fdf08c8e1@gmail.com/

In mk808_defconfig change:

CONFIG_TPL_TEXT_BASE=0x10080C04

to:

CONFIG_TPL_TEXT_BASE=0x10080C00

In rockchip.rst change:

        printf "RK30" > tplspl.bin
        dd if=u-boot-tpl.bin >> tplspl.bin

to:
        printf "RK30" > tplspl.bin
        dd if=u-boot-tpl.bin ibs=1 skip=4 >> tplspl.bin

The NAND can be programmed simular to MMC with:

rkdeveloptool wlx loader1 idbloader.img

TODO:

rk30 usbplug (open source)
===
>  
>  		u-boot-img {
>  			offset = <CONFIG_SPL_PAD_TO>;