| Message ID | 20220516110712.178958-3-andrew@mirx.dev |
|---|---|
| State | RFC |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | Build Rockchip final images using binman | expand |
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
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.
On 22/05/2022 03:55, Andrew Abbott wrote: > On Thu May 19, 2022 at 9:36 PM AEST, Alper Nebi Yasak wrote: >> 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! Honestly, I don't know. I was hoping someone else would comment as well. I'm inclined to say we don't need it, as we would ideally be able to extract/replace the 'idbloader.img' from/in working images with binman commands when needed. >> 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. I think it's meant to be a step towards unifying the build artifacts and names across the board, which I'd like if it eventually happened. We would have different definitions of the 'u-boot.rom' image for different whatevers, but for every board "Build and write u-boot.rom to SPI flash if it exists" would be valid advice.
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 {}; + }; + }; + + sdmmc-image { filename = "u-boot-rockchip.bin"; pad-byte = <0xff>; - blob { + idbloader { filename = "idbloader.img"; + type = "blob"; }; - u-boot-img { +#ifdef CONFIG_ARM64 + u-boot-fit { + filename = "u-boot.itb"; + type = "blob"; offset = <CONFIG_SPL_PAD_TO>; }; +#else + u-boot-img {}; +#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 { + filename = "u-boot.itb"; + offset = <CONFIG_SYS_SPI_U_BOOT_OFFS>; + }; +#else + u-boot-img {}; #endif + }; +}; +#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. config LNX_KRNL_IMG_TEXT_OFFSET_BASE default SYS_TEXT_BASE
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). This commit adds binman definitions to produce these images: - idbloader.img - rksd-formatted [TPL + ] SPL, as before. - 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. 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? 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(-)