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