[PATCH 1/2] bloblist: Introduce BLOBLIST_PRIOR_STAGE options
Tom Rini
trini at konsulko.com
Fri Dec 13 15:28:43 CET 2024
On Fri, Dec 13, 2024 at 07:15:09AM -0700, Simon Glass wrote:
> 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 entire reason that you needed that was never ever explained or
someone would have told you to move the bloblist to IRAM instead of DDR.
Instead we went round and round about something else.
> > 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.
If so, I'll be sure to continue to enjoy your company.
> > 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.
I mean in the Shakespeareian sense. None of us are enjoying it.
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20241213/16bf656b/attachment.sig>
More information about the U-Boot
mailing list