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 <[email protected]> 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 <[email protected]> > > --- > > 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.
--
Tom
signature.asc
Description: PGP signature
