[PATCH v2 33/35] RFC: Revert "bloblist: Load the bloblist from the previous loader"

Sun Feb 16 16:48:01 CET 2025

On Sun, Feb 16, 2025 at 09:01:23AM -0600, Tom Rini wrote:
> On Sun, Feb 16, 2025 at 07:10:31AM -0700, Simon Glass wrote:
> > Hi Tom,
> > 
> > On Sat, 15 Feb 2025 at 11:28, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Sat, Feb 15, 2025 at 10:24:50AM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Sat, 15 Feb 2025 at 08:01, Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > On Sat, Feb 15, 2025 at 05:11:45AM -0700, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Sat, 15 Feb 2025 at 05:06, Simon Glass <sjg at chromium.org> wrote:
> > > > > > >
> > > > > > > Hi Tom,
> > > > > > >
> > > > > > > On Tue, 11 Feb 2025 at 06:59, Tom Rini <trini at konsulko.com> wrote:
> > > > > > > >
> > > > > > > > On Tue, Feb 11, 2025 at 06:10:54AM -0700, Simon Glass wrote:
> > > > > > > > > Hi Tom,
> > > > > > > > >
> > > > > > > > > On Mon, 10 Feb 2025 at 07:33, Tom Rini <trini at konsulko.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On Sun, Feb 09, 2025 at 02:14:54PM -0700, Simon Glass wrote:
> > > > > > > > > >
> > > > > > > > > > > The logic of this has become too confusing.
> > > > > > > > > > >
> > > > > > > > > > > The primary issue with the patch is that U-Boot needs to set up a
> > > > > > > > > > > bloblist in the first phase where BLOBLIST is enabled. Subsequent
> > > > > > > > > > > phases can then use that bloblist.
> > > > > > > > > > >
> > > > > > > > > > > But the first phase of U-Boot cannot assume that one exists.
> > > > > > > > > > >
> > > > > > > > > > > Reverting this commit seems like a better starting point for getting
> > > > > > > > > > > things working for all use-cases.
> > > > > > > > > > >
> > > > > > > > > > > Note: The work to tidy this up is apparently underway. For this series,
> > > > > > > > > > > a revert is the easiest path.
> > > > > > > > > > >
> > > > > > > > > > > This reverts commit 66131310d8ff1ba228f989b41bd8812f43be41c3.
> > > > > > > > > > >
> > > > > > > > > > > https://lore.kernel.org/u-boot/CAPnjgZ3hMHtiH=f5ZKXNniOfV_-vFryq1Gn7QZ5hKU8Wjo8igw@mail.gmail.com/
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > > > > > > ---
> > > > > > > > > > >
> > > > > > > > > > > (no changes since v1)
> > > > > > > > > > >
> > > > > > > > > > >  common/bloblist.c  | 64 ++++++++++++++--------------------------------
> > > > > > > > > > >  include/bloblist.h | 10 --------
> > > > > > > > > > >  2 files changed, 19 insertions(+), 55 deletions(-)
> > > > > > > > > >
> > > > > > > > > > We aren't reverting this change so if you plan to move this series out of
> > > > > > > > > > your downstream fork you should start from that and stop posting it,
> > > > > > > > > > that just leads to confusion.
> > > > > > > > >
> > > > > > > > > Who is 'we', and start from what?
> > > > > > > >
> > > > > > > > We, being the community, in here, the community mainline U-Boot project
> > > > > > > > tree.
> > > > > > > >
> > > > > > > > > My goal with this series is to have something that actually boots on a
> > > > > > > > > real board, so the bloblist changes are needed for that, particularly
> > > > > > > > > as the 'vbe' board in the lab tests this on the hardware. I
> > > > > > > > > deliberately put these two patches at the end of the series so you can
> > > > > > > > > ignore them if you'd like.
> > > > > > > > >
> > > > > > > > > But for now, as I understand it, there are no users of standard
> > > > > > > > > passage in tree, so actually it would be fine to apply them.
> > > > > > > >
> > > > > > > > And you can ignore all the feedback you like for your downstream fork,
> > > > > > > > sure.
> > > > > > > >
> > > > > > > > Part of the feedback in this series already was "Yes, we can clean this
> > > > > > > > up a bit more if bloblist will do its own thing instead".
> > > > > > >
> > > > > > > So I see that Raymond's patch is a significant rewrite of what is
> > > > > > > there today, with multiple aims and changes. I would rather add my
> > > > > > > revert and start from a cleaner place.
> > > > > >
> > > > > > Also, I forgot to mention that U-Boot doesn't seem to support the
> > > > > > firmware handoff protocol in SPL? If so, could you point me to the
> > > > > > code?
> > > > >
> > > > > Yes, you never implemented handoff between stages in U-Boot via
> > > > > register. That's not a use case for the platforms that don't use U-Boot
> > > > > for SPL.
> > > >
> > > > If you let me own bloblist, I'll own it, but for now I am staying away.
> > >
> > > Would you be working against mainline again, and not breaking standard
> > > passage (as it's been split off) ?
> > 
> > Sure. The only reason I have my tree is because (I feel that) parts of
> > U-Boot have become no-go for me. So if you'd like me to maintain
> > bloblist again, I would be delighted to do so.
> > 
> > Steps I see:
> > - Get my boards working again
> 
> Already done. I'm waiting for Kever to pick up Jonas' small series that
> brings back output prior to that statement from DM.
> 
> > - Add standard passage from SPL->PPL
> 
> By who? PPL (and xPL) already reads standard passage via register.
> 
> > - Figure out what to do on x86 to also use standard passage
> 
> Standard passage, or bloblist?
> 
> > I'm not quite sure that BLOBLIST_FIXED can be removed, as it needs
> > more thought. The phase that creates the bloblist initially needs to
> > know where to put it.
> 
> When a phase is running it already has to know what memory is available
> right now or not, and can allocate as much of a list as it needs now.
> Later phases can move/grow it.

Actually, let me elaborate and then concede.

What I think the best design for BLOBLIST would be is:
- Uncouple "was I passed a bloblist" from "create a bloblist"
- Only allow passing bloblist via register(s).
- No bloblist contents can be globally mandatory until PPL.
  - Platforms may declare something mandatory (ie very first phase shall
    init certain clocks and hand that off to next stage) if necessary.
- A bloblist *should* be dynamically allocated but that's not required.

I believe this solves problems such as:
- We don't need OF_BLOBLIST because if we were not passed a bloblist we
  don't go and create one at a possibly inaccessible location.
- With fixed locations discouraged we wouldn't be forcing ourselves to
  choose between limited on-chip RAM or DRAM for everything prior to PPL
  relocating it.

With all that, if we untie standard passage from bloblist (and maybe we
want to revisit that in a year or two), bloblist can do what it likes so
long as standard passage isn't broken. And with it being uncoupled, it
should not get broken. I'll stop trying to convince you of a better
design.

-- 
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/20250216/0648f20e/attachment.sig>