On Sat, Jul 13, 2024 at 04:13:50PM +0100, Simon Glass wrote:
Hi Tom,
On Thu, 11 Jul 2024 at 22:27, Tom Rini trini@konsulko.com wrote:
A valid memory location to stash bootstage information at will be architecture dependent. Move the existing defaults to the main Kconfig file for this option and set 0x0 as the default only for sandbox.
Signed-off-by: Tom Rini trini@konsulko.com
Changes in v2:
- Seeing that BOOTSTAGE_STASH_ADDR did not depend on BOOTSTAGE_STASH in turn lead to discovering that (minor) BOOTSTAGE_STASH_SIZE was also missing a depends on line and then that a number of places built code with BOOTSTAGE_STASH_ADDR=0x0 as a compiles-but-likely-fails option. Rework a number of spots to guard usage around BOOTSTAGE_STASH being enabled.
arch/arm/mach-rockchip/tpl.c | 4 ++-- arch/arm/mach-stm32mp/Kconfig.13x | 3 --- arch/arm/mach-stm32mp/Kconfig.15x | 3 --- arch/arm/mach-stm32mp/Kconfig.25x | 3 --- arch/x86/cpu/cpu.c | 2 ++ boot/Kconfig | 6 +++++- cmd/bootstage.c | 8 +++++++- common/board_f.c | 2 ++ common/bootstage.c | 2 ++ common/spl/spl.c | 2 ++ include/bootstage.h | 14 ++++++++------ 11 files changed, 30 insertions(+), 19 deletions(-)
There is quite a bit going on in this patch.
Yes, there was unfortunately some underlying bugs.
diff --git a/arch/arm/mach-rockchip/tpl.c b/arch/arm/mach-rockchip/tpl.c index 50f04f9474a0..a47cba5163ab 100644 --- a/arch/arm/mach-rockchip/tpl.c +++ b/arch/arm/mach-rockchip/tpl.c @@ -92,10 +92,10 @@ void board_init_f(ulong dummy) int board_return_to_bootrom(struct spl_image_info *spl_image, struct spl_boot_device *bootdev) { -#ifdef CONFIG_BOOTSTAGE_STASH
int ret;
int __maybe_unused ret; bootstage_mark_name(BOOTSTAGE_ID_END_TPL, "end tpl");+#if IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
It should be enough to just call bootstage_stash_default() here, unconditionally. It does nothing if not enabled.
Nope, we don't have ADDR/SIZE defined. The current defaulting them to 0 leads to the case today where platforms which don't enable stash have ~700 bytes of stash related code being kept.
[snip]
diff --git a/common/spl/spl.c b/common/spl/spl.c index 7794ddccade1..47db4ead5050 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -472,11 +472,13 @@ static int spl_common_init(bool setup_malloc) ret); return ret; } +#if CONFIG_IS_ENABLED(BOOTSTAGE)
I don't think this #ifdef is needed.
As part of being able to discard the stash functionality, it is.
if (!u_boot_first_phase()) { ret = bootstage_unstash_default(); if (ret) log_debug("Failed to unstash bootstage: ret=%d\n", ret); }+#endif bootstage_mark_name(get_bootstage_id(true), spl_phase_name(spl_phase())); #if CONFIG_IS_ENABLED(LOG) diff --git a/include/bootstage.h b/include/bootstage.h index f4e77b09d747..2d4987f31414 100644 --- a/include/bootstage.h +++ b/include/bootstage.h @@ -462,18 +462,20 @@ int _bootstage_unstash_default(void);
static inline int bootstage_stash_default(void) {
if (CONFIG_IS_ENABLED(BOOTSTAGE) && IS_ENABLED(CONFIG_BOOTSTAGE_STASH))return _bootstage_stash_default();+#if CONFIG_IS_ENABLED(BOOTSTAGE) && IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
return _bootstage_stash_default();+#else return 0; +#endif
I believe you can leave this as it is.
}
static inline int bootstage_unstash_default(void) {
if (CONFIG_IS_ENABLED(BOOTSTAGE) && IS_ENABLED(CONFIG_BOOTSTAGE_STASH))return _bootstage_unstash_default();+#if CONFIG_IS_ENABLED(BOOTSTAGE) && IS_ENABLED(CONFIG_BOOTSTAGE_STASH)
return _bootstage_unstash_default();+#else return 0; +#endif
and this
Nope, this too is required to discard "stash" when not enabled.