[v2] bootstash: Do not provide a default address for all

[Simon Glass]

Hi Tom,

On Sat, 13 Jul 2024 at 17:50, Tom Rini <trini at konsulko.com> wrote:
>
> 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 at 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 at 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.

Yes I see.
>
> [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.

Yes, got it, but my point was that the header file already has this
logic. I'll send a new version to show what I am talking about...

Regards,
Simon