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

[Andrew Abbott]

On Thu May 19, 2022 at 9:36 PM AEST, Alper Nebi Yasak wrote:

> > 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].

I was keeping it for backwards compatibility, mainly because it's
mentioned in 'rockchip.rst' and it implies that 'idbloader.img' goes
on a separate partition to 'u-boot.itb' for targets supporting Fastboot.
If we can drop it, then I'll gladly do so!

> [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.

Makes sense to me - I just wonder if the name 'u-boot.rom' is too
generic, since it will be an image specifically for Rockchip targets.
Then again, perhaps the original 'u-boot-rockchip.bin' name was
redundant, since you know what target you're building for by using a
specific defconfig in the first place.

> > 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.

Oh, good point.

> 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'.

In that case, I'll just leave it as is for now.

> > +#ifdef CONFIG_TPL
> > +			u-boot-tpl {};
> > +#endif
> > +			u-boot-spl {};
>
> The closing braces should be on their own line, more instances below as
> well.

> > -		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.

> > +#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.

> > 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).

Thanks for your comments, I'll make the changes for the next version.