[PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options

Fri Dec 13 15:15:09 CET 2024

Hi Tom,

On Fri, 13 Dec 2024 at 07:03, Tom Rini <trini at konsulko.com> wrote:
>
> On Thu, Dec 12, 2024 at 08:38:32PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Thu, 12 Dec 2024 at 10:29, Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > > On Thu, 12 Dec 2024 at 19:27, Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Thu, Dec 12, 2024 at 07:08:34PM +0200, Ilias Apalodimas wrote:
> > > > > On Thu, 12 Dec 2024 at 19:02, Tom Rini <trini at konsulko.com> wrote:
> > > > > >
> > > > > > On Thu, Dec 12, 2024 at 06:34:44PM +0200, Ilias Apalodimas wrote:
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Thu, Dec 12, 2024 at 09:11:41AM -0600, Tom Rini wrote:
> > > > > > > > Introduce an option to control if we expect that a prior stage has
> > > > > > > > created a bloblist already and thus it is safe to scan the address. We
> > > > > > > > need to have this be configurable because we do not (and cannot) know if
> > > > > > > > the address at CONFIG_BLOBLIST_ADDR is in regular DRAM or some special
> > > > > > > > on-chip memory and so it may or may not be safe to read from this
> > > > > > > > address this early.
> > > > > > >
> > > > > > > Yes this patch makes instead of making the OF_SEPARATE implicitly take
> > > > > > > priority
> > > > > > >
> > > > > > > >
> > > > > > > > Signed-off-by: Tom Rini <trini at konsulko.com>
> > > > > > > > ---
> > > > > > > > Cc: Simon Glass <sjg at chromium.org>
> > > > > > > > ---
> > > > > > > >  common/Kconfig | 32 ++++++++++++++++++++++++++++++++
> > > > > > > >  lib/fdtdec.c   | 11 +++--------
> > > > > > > >  2 files changed, 35 insertions(+), 8 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/common/Kconfig b/common/Kconfig
> > > > > > > > index e8d89bf6eb9d..e8febad0f212 100644
> > > > > > > > --- a/common/Kconfig
> > > > > > > > +++ b/common/Kconfig
> > > > > > > > @@ -1077,6 +1077,14 @@ config BLOBLIST_SIZE
> > > > > > > >       is set up in the first part of U-Boot to run (TPL, SPL or U-Boot
> > > > > > > >       proper), and this sane bloblist is used for subsequent phases.
> > > > > > > >
> > > > > > > > +config BLOBLIST_PRIOR_STAGE
> > > > > > > > +   bool "Bloblist was created in a stage prior to U-Boot"
> > > > > > > > +   default y
> > > > > > > > +   depends on BLOBLIST_FIXED
> > > > > > > > +   help
> > > > > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created
> > > > > > > > +     before U-Boot is started.
> > > > > > > > +
> > > > > > > >  config BLOBLIST_SIZE_RELOC
> > > > > > > >     hex "Size of bloblist after relocation"
> > > > > > > >     default BLOBLIST_SIZE if BLOBLIST_FIXED || BLOBLIST_ALLOC
> > > > > > > > @@ -1117,6 +1125,14 @@ config SPL_BLOBLIST_ALLOC
> > > > > > > >
> > > > > > > >  endchoice
> > > > > > > >
> > > > > > > > +config SPL_BLOBLIST_PRIOR_STAGE
> > > > > > > > +   bool "Bloblist was created in a stage prior to SPL"
> > > > > > > > +   default y
> > > > > > > > +   depends on SPL_BLOBLIST_FIXED
> > > > > > > > +   help
> > > > > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > > > > > > +     U-Boot SPL is started.
> > > > > > > > +
> > > > > > > >  endif # SPL_BLOBLIST
> > > > > > > >
> > > > > > > >  if TPL_BLOBLIST
> > > > > > > > @@ -1146,6 +1162,14 @@ config TPL_BLOBLIST_ALLOC
> > > > > > > >
> > > > > > > >  endchoice
> > > > > > > >
> > > > > > > > +config TPL_BLOBLIST_PRIOR_STAGE
> > > > > > > > +   bool "Bloblist was created in a stage prior to TPL"
> > > > > > > > +   default y
> > > > > > > > +   depends on TPL_BLOBLIST_FIXED
> > > > > > > > +   help
> > > > > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > > > > > > +     U-Boot TPL is started.
> > > > > > > > +
> > > > > > > >  endif # TPL_BLOBLIST
> > > > > > > >
> > > > > > > >  if VPL_BLOBLIST
> > > > > > > > @@ -1175,6 +1199,14 @@ config VPL_BLOBLIST_ALLOC
> > > > > > > >
> > > > > > > >  endchoice
> > > > > > > >
> > > > > > > > +config VPL_BLOBLIST_PRIOR_STAGE
> > > > > > > > +   bool "Bloblist was created in a stage prior to VPL"
> > > > > > > > +   default y
> > > > > > > > +   depends on VPL_BLOBLIST_FIXED
> > > > > > > > +   help
> > > > > > > > +     Select this if the bloblist at CONFIG_BLOBLIST_ADDR was created before
> > > > > > > > +     U-Boot VPL is started.
> > > > > > > > +
> > > > > > > >  endif # VPL_BLOBLIST
> > > > > > > >
> > > > > > > >  endmenu
> > > > > > > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > > > > > > > index b06559880296..29bddab4150c 100644
> > > > > > > > --- a/lib/fdtdec.c
> > > > > > > > +++ b/lib/fdtdec.c
> > > > > > > > @@ -1669,15 +1669,10 @@ int fdtdec_setup(void)
> > > > > > > >     int ret = -ENOENT;
> > > > > > > >
> > > > > > > >     /*
> > > > > > > > -    * If allowing a bloblist, check that first. There was discussion about
> > > > > > > > -    * adding an OF_BLOBLIST Kconfig, but this was rejected.
> > > > > > > > -    *
> > > > > > > > -    * The necessary test is whether the previous phase passed a bloblist,
> > > > > > > > -    * not whether this phase creates one.
> > > > > > > > +    * Only scan for a bloblist and then if that bloblist contains a device
> > > > > > > > +    * tree if we have been configured to expect one.
> > > > > > > >      */
> > > > > > > > -   if (CONFIG_IS_ENABLED(BLOBLIST) &&
> > > > > > > > -       (xpl_prev_phase() != PHASE_TPL ||
> > > > > > > > -        !IS_ENABLED(CONFIG_TPL_BLOBLIST))) {
> > > > > > > > +   if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE)) {
> > > > > > >
> > > > > > > Without wider context I am not sure but if that check can fold in
> > > > > > > bloblist_maybe_init() and return -X if the config isn't enabled, it's going
> > > > > > > to be easier to read
> > > > > >
> > > > > > Hmm. We'd have:
> > > > > >         if (CONFIG_IS_ENABLED(BLOBLIST_PRIOR_STAGE) &&
> > > > > >             !(ret = bloblist_maybe_init()) {
> > > > > >                 /* We have a bloblist, see if it has FDT */
> > > > >
> > > > > I mean move the check inside bloblist_maybe_init()
> > > >
> > > > As we talked about on IRC, no, because the "maybe" in that function
> > > > means "find or create". So the expected usage here is that we have
> > > > platforms that create the bloblist in SPL (post DRAM init) and use that
> > > > with the HANDOFF mechanism to pass information to full U-Boot.
> > > >
> > >
> > > Yes thanks
> > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> >
> > I am struggling to see how this is different from the patch I sent a year ago
> >
> > https://patchwork.ozlabs.org/project/uboot/patch/20231226094625.221671-1-sjg@chromium.org/
> >
> > Is it just a rebase and a rename?
>
> Well, no. I came up with this after debugging the failure on Bob / Kevin
> via Gitlab. This in fact allows for the device tree to NOT come from the
> bloblist.

Yes that was the point of my patch, to allow me to disable OF_BLOBLIST

> The biggest problem with the "OF_BLOBLIST" thread from last
> year is that your explanation of the problem doesn't match the problem
> you have.

At this point I am worried you might be one of those people who is never wrong.

>
> What's really tragically funny now is that Jonas' add TPL to Bob/Kevin
> and init DRAM there means that we don't need this series for that
> platform and only Coral is still doing the likely ill-advised thing of
> saying the bloblist exists in DRAM without ensuring that DRAM will have
> been initialized already.

At least someone is enjoying all this. The whole thing is a bad memory
and makes me sick to think of it.

Regards,
Simon