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

Alper Nebi Yasak
2022-05-19T11:39:50Z

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