[RFC PATCH v2 2/8] rockchip: Add binman definitions for final images

[Alper Nebi Yasak]

On 16/05/2022 14:07, Andrew Abbott wrote:
> Currently, building for Rockchip targets produces:
> 
> - idbloader.img
>     - rksd-formatted TPL with SPL appended; or
>     - rksd-formatted SPL
> - u-boot.itb
>     - U-Boot Proper FIT image
> - u-boot-rockchip.bin
>     - idbloader.img + u-boot.itb, padded the correct amount for SD/MMC
>       usage.
> 
> For RK3399 targets:
> 
> - u-boot.rom
>     - SPI image specific to the bob Chromebook target (see
>       c4cea2bbf995764f325a907061c22ecd6768cf7b).

I'm not sure we need anything special for the rk3399 chromebooks. I
think it's meant to be your 'u-boot-rockchip-spi.bin', but is just
incomplete (and partially broken, e.g. it should use u-boot.itb).

I'll try to test things on my chromebook_kevin later on, but probably
not before you can send a v3...

> This commit adds binman definitions to produce these images:
> 
> - idbloader.img
>     - rksd-formatted [TPL + ] SPL, as before.

Do we need the 'idbloader.img' as a build output, assuming we have a
working 'u-boot-rockchip.bin'? I'm asking because Simon was trying to
drop it in a similar patch [1].

[1] rockchip: Support building the all output files in binman
https://lore.kernel.org/u-boot/20220223230040.159317-24-sjg@chromium.org/

> - u-boot-rockchip.bin
>     - [TPL + ] SPL all rksd-formatted + u-boot.itb padded for SD/MMC
>       usage, as before.
> - u-boot-rockchip-spi.bin
>     - [TPL + ] SPL all rkspi-formatted + u-boot.itb padded for SPI
>       usage.
> 
> This commit also generalizes the CONFIG_ROCKCHIP_SPI_IMAGE config
> setting - it now means to generate a generic SPI flash image, in
> addition to the generic SD/MMC image.

With what I said above, I think you should rename this to 'u-boot.rom'
and remove the definitions in {rk3288,rk3399}-u-boot.dtsi.

> 
> Signed-off-by: Andrew Abbott <andrew at mirx.dev>
> ---
> Question: Does this break/not play nicely with rockchip-optee
> generation? It creates u-boot.itb for rk3288 targets. That would need to
> run before what I've implemented here?

The 'u-boot.itb' entries you add here are guarded with CONFIG_ARM64, so
don't apply to RK3288. I think that's fine until we can convert these
'u-boot.itb's to be completely binman-built as well.

OTOH, things appear work fine if I:

- Replace CONFIG_ARM64 with CONFIG_SPL_FIT below
- Include rockchip-optee before rockchip-u-boot in rk3288-u-boot.dtsi
- Move the empty binman node to rk3288-u-boot.dtsi above both includes

at least, until the patch that forces Makefile to build 'u-boot.itb'.

> 
> Changes in v2:
> - Revert u-boot-rockchip-sdmmc.bin name to u-boot-rockchip.bin, to
>   keep the name the same as before.
> - Fix whitespace issues.
> 
>  arch/arm/dts/rockchip-u-boot.dtsi | 53 +++++++++++++++++++++++++++++--
>  arch/arm/mach-rockchip/Kconfig    |  7 ++--
>  2 files changed, 53 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/dts/rockchip-u-boot.dtsi b/arch/arm/dts/rockchip-u-boot.dtsi
> index eae3ee715d..9354b0f5a7 100644
> --- a/arch/arm/dts/rockchip-u-boot.dtsi
> +++ b/arch/arm/dts/rockchip-u-boot.dtsi
> @@ -13,17 +13,64 @@
>  
>  #ifdef CONFIG_SPL
>  &binman {
> -	simple-bin {
> +	sdmmc-idbloader {
> +		filename = "idbloader.img";
> +
> +		mkimage {
> +			args = "-n", CONFIG_SYS_SOC, "-T", "rksd";
> +
> +#ifdef CONFIG_TPL
> +			u-boot-tpl {};
> +#endif
> +			u-boot-spl {};

The closing braces should be on their own line, more instances below as
well.

> +		};
> +	};
> +
> +	sdmmc-image {
>  		filename = "u-boot-rockchip.bin";
>  		pad-byte = <0xff>;
>  
> -		blob {
> +		idbloader {
>  			filename = "idbloader.img";
> +			type = "blob";
>  		};
>  
> -		u-boot-img {
> +#ifdef CONFIG_ARM64

Probably "#if defined(CONFIG_SPL_FIT) && defined(CONFIG_ARM64)" is
better, like in Simon's patch. Likewise in the SPI image below.

> +		u-boot-fit {
> +			filename = "u-boot.itb";
> +			type = "blob";
>  			offset = <CONFIG_SPL_PAD_TO>;
>  		};
> +#else
> +		u-boot-img {};

I think this still needs the "offset = <CONFIG_SPL_PAD_TO>;" which
shouldn't be removed.

> +#endif
>  	};
>  };
> +
> +#ifdef CONFIG_ROCKCHIP_SPI_IMAGE
> +&binman {
> +	spi-image {
> +		filename = "u-boot-rockchip-spi.bin";
> +		pad-byte = <0xff>;
> +
> +		mkimage {
> +			args = "-n", CONFIG_SYS_SOC, "-T", "rkspi";
> +
> +#ifdef CONFIG_TPL
> +			u-boot-tpl {};
> +#endif
> +			u-boot-spl {};
> +		};
> +
> +#ifdef CONFIG_ARM64
> +		blob {

This should have the same name & type as the one in the other image.

> +			filename = "u-boot.itb";
> +			offset = <CONFIG_SYS_SPI_U_BOOT_OFFS>;
> +		};
> +#else
> +		u-boot-img {};

I think this also needs "offset = <CONFIG_SYS_SPI_U_BOOT_OFFS>;".

>  #endif
> +	};
> +};

Also consider adding a 'fdtmap' entry to both SPI and SD/MMC images,
it's useful for some interactive binman features.

> +#endif // CONFIG_ROCKCHIP_SPI_IMAGE
> +#endif // CONFIG_SPL
> diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
> index 18aff5480b..7149b9a530 100644
> --- a/arch/arm/mach-rockchip/Kconfig
> +++ b/arch/arm/mach-rockchip/Kconfig
> @@ -415,12 +415,11 @@ config SPL_MMC
>  
>  config ROCKCHIP_SPI_IMAGE
>  	bool "Build a SPI image for rockchip"
> -	depends on HAS_ROM
>  	help
>  	  Some Rockchip SoCs support booting from SPI flash. Enable this
> -	  option to produce a 4MB SPI-flash image (called u-boot.rom)
> -	  containing U-Boot. The image is built by binman. U-Boot sits near
> -	  the start of the image.
> +	  option to produce an SPI-flash image (called u-boot-rockchip-spi.bin)
> +	  containing TPL (if enabled) and SPL, and U-Boot proper at the offset
> +	  CONFIG_SYS_SPI_U_BOOT_OFFS. The image is built by binman.

While renaming 'u-boot-rockchip-spi.bin' to 'u-boot.rom' you should drop
these Kconfig changes as well, and select HAS_ROM wherever you select
ROCKCHIP_SPI_IMAGE (i.e. in the last patch).

>  
>  config LNX_KRNL_IMG_TEXT_OFFSET_BASE
>  	default SYS_TEXT_BASE