Message ID | 20210708173032.27999-1-marek.behun@nic.cz |
---|---|
Headers | show |
Series | kwboot / kwbimage improvements | expand |
Hi Marek & Pali, On 08.07.21 19:30, Marek Behún wrote: > Hi Stefan and others, > > this is a series of improvements to kwboot, kwbimage and mvebu. > > The main goal of this series is to correctly use BootROM's code > for loading U-Boot from NOR / NAND: currently only SPL is read by > BootROM and the main U-Boot is read by SPL. By using BootROM to also > load main U-Boot we can reduce the size of SPL image, since it does > not need to contain code for reading NOR / NAND. Before going into a review of the patches, let me ask about the motivation of this patchset. Is the reduction of the SPL image size the main motivation for this series? Or did you experiece some problems with the SPL code for U-Boot proper loading? BTW: This patch / mail subject "kwboot / kwbimage improvements" does not really match its content AFAIU. Here, the SPL returns always back to the BootROM for U-Boot proper loading part is missing. Or do I misunderstand something? BTW2: Could you please list the affected MVEBU SoC's that are affected by this series so that this is clear? > Before merging, this series should be tested on as many relevant > boards as possible. I fully agree. I very much welcome any "Tested-by" tags and reviews from others. Thanks, Stefan > Marek & Pali > > Marek Behún (2): > tools: kwbimage: Add constant for SDIO bootfrom > tools: kwbimage: Cosmetic fix - remove redundant space character > > Pali Rohár (29): > tools: kwbimage: Fix compilation without CONFIG_SYS_U_BOOT_OFFS > tools: kwbimage: Simplify aligning and calculating checksum > tools: kwbimage: Align SPI and NAND images to 256 bytes > tools: kwbimage: Fix generation of SATA, SDIO and PCIe images > tools: kwbimage: Don't crash when binary file name does not contain > '/' > tools: kwbimage: Fix check for v0 extended header checksum > tools: kwbimage: Validate extended headers of v1 images > tools: kwbimage: Validate data checksum of v1 images > tools: kwbimage: Print size of binary header in > kwbimage_print_header() > tools: kwboot: Fix wrong parameter passed to read() > tools: kwboot: Fix restoring terminal > tools: kwboot: Print trailing newline after terminal is terminated > tools: kwboot: Cosmetic fix - add missing curly brackets > tools: kwboot: Check for v1 header size > tools: kwbimage: Use -a parameter (load address) for v1 images > arm: mvebu: Fix return_to_bootrom() > arm: mvebu: Mark return_to_bootrom() as a noreturn function > arm: mvebu: Implement return_to_bootrom() via U-Boot's SPL framework > arm: mvebu: Use U-Boot's SPL BootROM framework for booting from > NAND/UART > arm: mvebu: Always use BootROM for loading the rest of U-Boot's binary > arm: mvebu: gdsys: Remove custom spl_board_init() > arm: mvebu: Remove legacy U-Boot header from kwbimage v1 files > tools: kwbimage: Remove v1 kwbimage SPL padding to > CONFIG_SYS_U_BOOT_OFFS bytes > arm: mvebu: Remove unused macro CONFIG_SYS_U_BOOT_OFFS > tools: kwbimage: Add support for more BINARY headers > tools: kwbimage: Don't parse PAYLOAD keyword > tools: kwbimage: Add support for DATA command also for v1 images > tools: kwbimage: Add support for a new DATA_DELAY command > tools: kwbimage: Do not hide usage of secure header under > CONFIG_ARMADA_38X > > Makefile | 2 +- > arch/arm/mach-mvebu/Kconfig | 16 +- > arch/arm/mach-mvebu/include/mach/cpu.h | 2 +- > arch/arm/mach-mvebu/lowlevel_spl.S | 3 +- > arch/arm/mach-mvebu/spl.c | 90 +------ > board/gdsys/a38x/Makefile | 2 +- > board/gdsys/a38x/spl.c | 20 -- > include/configs/clearfog.h | 6 +- > include/configs/controlcenterdc.h | 8 +- > include/configs/db-88f6720.h | 3 - > include/configs/db-88f6820-amc.h | 5 - > include/configs/db-88f6820-gp.h | 6 - > include/configs/db-mv784mp-gp.h | 3 - > include/configs/ds414.h | 5 - > include/configs/helios4.h | 6 +- > include/configs/theadorable.h | 3 - > include/configs/turris_omnia.h | 6 - > include/configs/x530.h | 3 - > scripts/config_whitelist.txt | 1 - > tools/Makefile | 8 - > tools/kwbimage.c | 339 +++++++++++++++++-------- > tools/kwbimage.h | 30 ++- > tools/kwboot.c | 14 +- > 23 files changed, 296 insertions(+), 285 deletions(-) > delete mode 100644 board/gdsys/a38x/spl.c > Viele Grüße, Stefan
Hi Stefan On Fri, 9 Jul 2021 08:05:40 +0200 Stefan Roese <sr@denx.de> wrote: > > The main goal of this series is to correctly use BootROM's code > > for loading U-Boot from NOR / NAND: currently only SPL is read by > > BootROM and the main U-Boot is read by SPL. By using BootROM to also > > load main U-Boot we can reduce the size of SPL image, since it does > > not need to contain code for reading NOR / NAND. > > Before going into a review of the patches, let me ask about the > motivation of this patchset. Is the reduction of the SPL image size > the main motivation for this series? Or did you experiece some > problems with the SPL code for U-Boot proper loading? The motivation is reduction of SPL code size. Pali started working on something different (adding support for higher baudrates to kwboot, so that a bricked board can be booted over UART faster), and he is still working on it, but when he started studying the kwbimage/kwboot code and A38x documentation, he noticed that these things could be improved, so first he did this series. > BTW: This patch / mail subject "kwboot / kwbimage improvements" does > not really match its content AFAIU. Here, the SPL returns always back > to the BootROM for U-Boot proper loading part is missing. Or do I > misunderstand something? You are right, the subject of the series could have been better. I shall change it in v2. > > BTW2: Could you please list the affected MVEBU SoC's that are affected > by this series so that this is clear? OK. Marek
Hi Marek, On 09.07.21 13:22, Marek Behún wrote: > Hi Stefan > > On Fri, 9 Jul 2021 08:05:40 +0200 > Stefan Roese <sr@denx.de> wrote: > >>> The main goal of this series is to correctly use BootROM's code >>> for loading U-Boot from NOR / NAND: currently only SPL is read by >>> BootROM and the main U-Boot is read by SPL. By using BootROM to also >>> load main U-Boot we can reduce the size of SPL image, since it does >>> not need to contain code for reading NOR / NAND. >> >> Before going into a review of the patches, let me ask about the >> motivation of this patchset. Is the reduction of the SPL image size >> the main motivation for this series? Or did you experiece some >> problems with the SPL code for U-Boot proper loading? > > The motivation is reduction of SPL code size. > Pali started working on something different (adding support for higher > baudrates to kwboot, so that a bricked board can be booted over UART > faster), and he is still working on it, but when he started studying the > kwbimage/kwboot code and A38x documentation, he noticed that these > things could be improved, so first he did this series. > >> BTW: This patch / mail subject "kwboot / kwbimage improvements" does >> not really match its content AFAIU. Here, the SPL returns always back >> to the BootROM for U-Boot proper loading part is missing. Or do I >> misunderstand something? > > You are right, the subject of the series could have been better. I > shall change it in v2. > >> >> BTW2: Could you please list the affected MVEBU SoC's that are affected >> by this series so that this is clear? > > OK. Thanks. And could you please also do one (or more) boot-time comparisons, old vs. new version? This way we can see, if and how the boot-time is affected (perhaps improved, which would be great) by this patch series. Thanks, Stefan > Marek > Viele Grüße, Stefan
On Fri, 9 Jul 2021 14:35:15 +0200 Stefan Roese <sr@denx.de> wrote: > Thanks. And could you please also do one (or more) boot-time > comparisons, old vs. new version? This way we can see, if and how the > boot-time is affected (perhaps improved, which would be great) by this > patch series. For Turris Omnia, the size of the u-boot-spl.kwb binary decreases by ~37 KiB, mainly due to not needing padding anymore. The size of the SPL binary itself (u-boot-spl.bin) decreases by 7.5 KiB (due to code reduction - no SPI NOR code). Unfortunately the boot time increases by 815 ms, from 2341 ms (on average) to 3156 ms (on average). This is probably because BootROM read the memory differently (lower frequency or some other reason). I think this 815 ms increase in boot time into U-Boot prompt is worth the 37 KiB saved space for U-Boot code (mainly because on Omnia we have 960 KiB for U-Boot and currently we are at 860 KiB, so enabling some other features in the future may push us to the limit), but others may not agree. I am going to put this information also into v2 cover letter. Marek
Hi Marek, Pali, On Thu, Jul 08 2021, Marek Behún wrote: > Hi Stefan and others, > > this is a series of improvements to kwboot, kwbimage and mvebu. > > The main goal of this series is to correctly use BootROM's code > for loading U-Boot from NOR / NAND: currently only SPL is read by > BootROM and the main U-Boot is read by SPL. By using BootROM to also > load main U-Boot we can reduce the size of SPL image, since it does > not need to contain code for reading NOR / NAND. > > Before merging, this series should be tested on as many relevant > boards as possible. Is this series available for pull at a public git repo? If not, what commit is this series based on? What platforms have you tested? baruch > Marek Behún (2): > tools: kwbimage: Add constant for SDIO bootfrom > tools: kwbimage: Cosmetic fix - remove redundant space character > > Pali Rohár (29): > tools: kwbimage: Fix compilation without CONFIG_SYS_U_BOOT_OFFS > tools: kwbimage: Simplify aligning and calculating checksum > tools: kwbimage: Align SPI and NAND images to 256 bytes > tools: kwbimage: Fix generation of SATA, SDIO and PCIe images > tools: kwbimage: Don't crash when binary file name does not contain > '/' > tools: kwbimage: Fix check for v0 extended header checksum > tools: kwbimage: Validate extended headers of v1 images > tools: kwbimage: Validate data checksum of v1 images > tools: kwbimage: Print size of binary header in > kwbimage_print_header() > tools: kwboot: Fix wrong parameter passed to read() > tools: kwboot: Fix restoring terminal > tools: kwboot: Print trailing newline after terminal is terminated > tools: kwboot: Cosmetic fix - add missing curly brackets > tools: kwboot: Check for v1 header size > tools: kwbimage: Use -a parameter (load address) for v1 images > arm: mvebu: Fix return_to_bootrom() > arm: mvebu: Mark return_to_bootrom() as a noreturn function > arm: mvebu: Implement return_to_bootrom() via U-Boot's SPL framework > arm: mvebu: Use U-Boot's SPL BootROM framework for booting from > NAND/UART > arm: mvebu: Always use BootROM for loading the rest of U-Boot's binary > arm: mvebu: gdsys: Remove custom spl_board_init() > arm: mvebu: Remove legacy U-Boot header from kwbimage v1 files > tools: kwbimage: Remove v1 kwbimage SPL padding to > CONFIG_SYS_U_BOOT_OFFS bytes > arm: mvebu: Remove unused macro CONFIG_SYS_U_BOOT_OFFS > tools: kwbimage: Add support for more BINARY headers > tools: kwbimage: Don't parse PAYLOAD keyword > tools: kwbimage: Add support for DATA command also for v1 images > tools: kwbimage: Add support for a new DATA_DELAY command > tools: kwbimage: Do not hide usage of secure header under > CONFIG_ARMADA_38X > > Makefile | 2 +- > arch/arm/mach-mvebu/Kconfig | 16 +- > arch/arm/mach-mvebu/include/mach/cpu.h | 2 +- > arch/arm/mach-mvebu/lowlevel_spl.S | 3 +- > arch/arm/mach-mvebu/spl.c | 90 +------ > board/gdsys/a38x/Makefile | 2 +- > board/gdsys/a38x/spl.c | 20 -- > include/configs/clearfog.h | 6 +- > include/configs/controlcenterdc.h | 8 +- > include/configs/db-88f6720.h | 3 - > include/configs/db-88f6820-amc.h | 5 - > include/configs/db-88f6820-gp.h | 6 - > include/configs/db-mv784mp-gp.h | 3 - > include/configs/ds414.h | 5 - > include/configs/helios4.h | 6 +- > include/configs/theadorable.h | 3 - > include/configs/turris_omnia.h | 6 - > include/configs/x530.h | 3 - > scripts/config_whitelist.txt | 1 - > tools/Makefile | 8 - > tools/kwbimage.c | 339 +++++++++++++++++-------- > tools/kwbimage.h | 30 ++- > tools/kwboot.c | 14 +- > 23 files changed, 296 insertions(+), 285 deletions(-) > delete mode 100644 board/gdsys/a38x/spl.c
On Fri, 09 Jul 2021 17:54:25 +0300 Baruch Siach <baruch@tkos.co.il> wrote: > Hi Marek, Pali, > > On Thu, Jul 08 2021, Marek Behún wrote: > > Hi Stefan and others, > > > > this is a series of improvements to kwboot, kwbimage and mvebu. > > > > The main goal of this series is to correctly use BootROM's code > > for loading U-Boot from NOR / NAND: currently only SPL is read by > > BootROM and the main U-Boot is read by SPL. By using BootROM to also > > load main U-Boot we can reduce the size of SPL image, since it does > > not need to contain code for reading NOR / NAND. > > > > Before merging, this series should be tested on as many relevant > > boards as possible. > > Is this series available for pull at a public git repo? If not, what > commit is this series based on? > > What platforms have you tested? Based on current u-boot master, commit fd075f77ca56ffb07e0b1979f0cb47fc8831600f Tested only on Turris Omnia (Armada 385). Marek
Hi Marek, On 09.07.21 16:08, Marek Behún wrote: > On Fri, 9 Jul 2021 14:35:15 +0200 > Stefan Roese <sr@denx.de> wrote: > >> Thanks. And could you please also do one (or more) boot-time >> comparisons, old vs. new version? This way we can see, if and how the >> boot-time is affected (perhaps improved, which would be great) by this >> patch series. > > For Turris Omnia, the size of the u-boot-spl.kwb binary decreases by > ~37 KiB, mainly due to not needing padding anymore. The size of the SPL > binary itself (u-boot-spl.bin) decreases by 7.5 KiB (due to code > reduction - no SPI NOR code). > > Unfortunately the boot time increases by 815 ms, from 2341 ms (on > average) to 3156 ms (on average). Ups. This boot-time increase is quite big. And there might be some Armada / MVEBU U-Boot targets that are tuned for fast booting. I can speak for the Armada XP theadorable board, where boot-time is quite critical. I remember that we tuned and optimized here and an increase of perhaps 100ms would be acceptable. But an increase of nearly 1 second is definitely not acceptable here, I'm afraid. Please see below. > This is probably because BootROM read the memory differently (lower > frequency or some other reason). > > I think this 815 ms increase in boot time into U-Boot prompt is worth > the 37 KiB saved space for U-Boot code (mainly because on Omnia we have > 960 KiB for U-Boot and currently we are at 860 KiB, so enabling some > other features in the future may push us to the limit), but others may > not agree. Yes, I disagree here. Boot-time is critical for some platforms and this increase is just too high. I might be able to do some tests on theadorable to get actual numbers for this platform. Could you perhaps add this "SPL returns to BootROM" support as an optional feature that can be selected on a per-board basis? Thanks, Stefan > I am going to put this information also into v2 cover letter. > > Marek >
On Saturday 10 July 2021 02:31:32 Stefan Roese wrote: > Could you perhaps add this "SPL returns to BootROM" support as an > optional feature that can be selected on a per-board basis? Hi Stefan! I was thinking about it and it should not be hard. Based on defconfig options you decide if you want to use SPL NOR code or stick with return to BootROM. It is acceptable? What is needed is just to extend SPL SPI NOR code to parse also kwbimage header (at offset zero) instead of legacy U-Boot header which is removing by this patch series and which was on variable offset.
On Saturday 10 July 2021 02:43:12 Pali Rohár wrote: > On Saturday 10 July 2021 02:31:32 Stefan Roese wrote: > > Could you perhaps add this "SPL returns to BootROM" support as an > > optional feature that can be selected on a per-board basis? > > Hi Stefan! I was thinking about it and it should not be hard. Based on > defconfig options you decide if you want to use SPL NOR code or stick > with return to BootROM. It is acceptable? > > What is needed is just to extend SPL SPI NOR code to parse also kwbimage > header (at offset zero) instead of legacy U-Boot header which is > removing by this patch series and which was on variable offset. Anyway, this switch "return to BootROM" is done in patch 22. Previous patches 01 - 21 can be applied and used without later patches. So meanwhile could you look at patches 01 - 21 if they are OK and if they could be merged independently of "return to BootROM" patches?
On 10.07.21 02:43, Pali Rohár wrote: > On Saturday 10 July 2021 02:31:32 Stefan Roese wrote: >> Could you perhaps add this "SPL returns to BootROM" support as an >> optional feature that can be selected on a per-board basis? > > Hi Stefan! I was thinking about it and it should not be hard. Based on > defconfig options you decide if you want to use SPL NOR code or stick > with return to BootROM. It is acceptable? I think so, yes. If you patches add features / enhancements for some boards and this can be selected on a per-board basis to not affect other targets then yes, this can be accepted AFAICT. > What is needed is just to extend SPL SPI NOR code to parse also kwbimage > header (at offset zero) instead of legacy U-Boot header which is > removing by this patch series and which was on variable offset. It would be helpful, if you could add a Kconfig option to selectively enable "return to BootROM" - disabled per default and enabled for your targets to this patchset. Thanks, Stefan
On 10.07.21 02:59, Pali Rohár wrote: > On Saturday 10 July 2021 02:43:12 Pali Rohár wrote: >> On Saturday 10 July 2021 02:31:32 Stefan Roese wrote: >>> Could you perhaps add this "SPL returns to BootROM" support as an >>> optional feature that can be selected on a per-board basis? >> >> Hi Stefan! I was thinking about it and it should not be hard. Based on >> defconfig options you decide if you want to use SPL NOR code or stick >> with return to BootROM. It is acceptable? >> >> What is needed is just to extend SPL SPI NOR code to parse also kwbimage >> header (at offset zero) instead of legacy U-Boot header which is >> removing by this patch series and which was on variable offset. > > Anyway, this switch "return to BootROM" is done in patch 22. Previous > patches 01 - 21 can be applied and used without later patches. So > meanwhile could you look at patches 01 - 21 if they are OK and if they > could be merged independently of "return to BootROM" patches? I'll try to find some time next week to review these patches. Thanks, Stefan
On Fri, Jul 9, 2021 at 5:30 AM Marek Behún <marek.behun@nic.cz> wrote: > > Hi Stefan and others, > > this is a series of improvements to kwboot, kwbimage and mvebu. > > The main goal of this series is to correctly use BootROM's code > for loading U-Boot from NOR / NAND: currently only SPL is read by > BootROM and the main U-Boot is read by SPL. By using BootROM to also > load main U-Boot we can reduce the size of SPL image, since it does > not need to contain code for reading NOR / NAND. > > Before merging, this series should be tested on as many relevant > boards as possible. I've given the series a look over. Aside from a couple of typos in one commit message which I replied to, the rest looked pretty good. Review-by: Chris Packham <judge.packham@gmail.com> For DB-88f6820-AMC and x530 Tested-by: Chris Packham <judge.packham@gmail.com> > > Marek & Pali > > Marek Behún (2): > tools: kwbimage: Add constant for SDIO bootfrom > tools: kwbimage: Cosmetic fix - remove redundant space character > > Pali Rohár (29): > tools: kwbimage: Fix compilation without CONFIG_SYS_U_BOOT_OFFS > tools: kwbimage: Simplify aligning and calculating checksum > tools: kwbimage: Align SPI and NAND images to 256 bytes > tools: kwbimage: Fix generation of SATA, SDIO and PCIe images > tools: kwbimage: Don't crash when binary file name does not contain > '/' > tools: kwbimage: Fix check for v0 extended header checksum > tools: kwbimage: Validate extended headers of v1 images > tools: kwbimage: Validate data checksum of v1 images > tools: kwbimage: Print size of binary header in > kwbimage_print_header() > tools: kwboot: Fix wrong parameter passed to read() > tools: kwboot: Fix restoring terminal > tools: kwboot: Print trailing newline after terminal is terminated > tools: kwboot: Cosmetic fix - add missing curly brackets > tools: kwboot: Check for v1 header size > tools: kwbimage: Use -a parameter (load address) for v1 images > arm: mvebu: Fix return_to_bootrom() > arm: mvebu: Mark return_to_bootrom() as a noreturn function > arm: mvebu: Implement return_to_bootrom() via U-Boot's SPL framework > arm: mvebu: Use U-Boot's SPL BootROM framework for booting from > NAND/UART > arm: mvebu: Always use BootROM for loading the rest of U-Boot's binary > arm: mvebu: gdsys: Remove custom spl_board_init() > arm: mvebu: Remove legacy U-Boot header from kwbimage v1 files > tools: kwbimage: Remove v1 kwbimage SPL padding to > CONFIG_SYS_U_BOOT_OFFS bytes > arm: mvebu: Remove unused macro CONFIG_SYS_U_BOOT_OFFS > tools: kwbimage: Add support for more BINARY headers > tools: kwbimage: Don't parse PAYLOAD keyword > tools: kwbimage: Add support for DATA command also for v1 images > tools: kwbimage: Add support for a new DATA_DELAY command > tools: kwbimage: Do not hide usage of secure header under > CONFIG_ARMADA_38X > > Makefile | 2 +- > arch/arm/mach-mvebu/Kconfig | 16 +- > arch/arm/mach-mvebu/include/mach/cpu.h | 2 +- > arch/arm/mach-mvebu/lowlevel_spl.S | 3 +- > arch/arm/mach-mvebu/spl.c | 90 +------ > board/gdsys/a38x/Makefile | 2 +- > board/gdsys/a38x/spl.c | 20 -- > include/configs/clearfog.h | 6 +- > include/configs/controlcenterdc.h | 8 +- > include/configs/db-88f6720.h | 3 - > include/configs/db-88f6820-amc.h | 5 - > include/configs/db-88f6820-gp.h | 6 - > include/configs/db-mv784mp-gp.h | 3 - > include/configs/ds414.h | 5 - > include/configs/helios4.h | 6 +- > include/configs/theadorable.h | 3 - > include/configs/turris_omnia.h | 6 - > include/configs/x530.h | 3 - > scripts/config_whitelist.txt | 1 - > tools/Makefile | 8 - > tools/kwbimage.c | 339 +++++++++++++++++-------- > tools/kwbimage.h | 30 ++- > tools/kwboot.c | 14 +- > 23 files changed, 296 insertions(+), 285 deletions(-) > delete mode 100644 board/gdsys/a38x/spl.c > > -- > 2.31.1 >