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