Hi Raymond,
On Wed, 5 Feb 2025 at 08:25, Raymond Mao <raymond....@linaro.org> wrote:
>
> +CC Ilias,
>
> Hi Simon,
>
> On Tue, 4 Feb 2025 at 20:57, Simon Glass <s...@chromium.org> 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.
> >
> > This reverts commit 66131310d8ff1ba228f989b41bd8812f43be41c3.
> >
> > https://lore.kernel.org/u-boot/CAPnjgZ3hMHtiH=f5zkxnniofv_-vfryq1gn7qz5hku8wjo8...@mail.gmail.com/
> > Signed-off-by: Simon Glass <s...@chromium.org>
> > ---
> >
>
> If my understanding is correct, you want to add some logic to control
> when the U-Boot should or should not get the bloblist from the
> existing register argument.
> But xferlist_from_boot_arg() should be called when a valid register
> argument is there, I didn't see this in your patch.
> Maybe you plan to do this with other patch series, but simply
> reverting this results in a breaking of handoff policy and the
> firmware handoff won't work.
Yes, I certainly did not want to revert it, but the current code is
too hard to understand and I did not look at it at the time it went
in. I've had three tries at working with what you have here, but each
turns to spaghetti.
I would like to build on this and get something running in CI which
uses standard passage. As Tom suggests, perhaps we should disconnect
bloblist and standard passage?
On the CI point, is there a board we could add that uses the
xferlist_from_boot_arg() call?
Regards,
Simon
>
> Regards,
> Raymond
>
> > common/bloblist.c | 64 ++++++++++++++--------------------------------
> > include/bloblist.h | 10 --------
> > 2 files changed, 19 insertions(+), 55 deletions(-)
> >
> > diff --git a/common/bloblist.c b/common/bloblist.c
> > index e8acfc74331..7eda94ecdf9 100644
> > --- a/common/bloblist.c
> > +++ b/common/bloblist.c
> > @@ -487,57 +487,37 @@ int bloblist_reloc(void *to, uint to_size)
> > return 0;
> > }
> >
> > -/*
> > - * Weak default function for getting bloblist from boot args.
> > - */
> > -int __weak xferlist_from_boot_arg(ulong __always_unused addr,
> > - ulong __always_unused size)
> > -{
> > - return -ENOENT;
> > -}
> > -
> > int bloblist_init(void)
> > {
> > bool fixed = IS_ENABLED(CONFIG_BLOBLIST_FIXED);
> > int ret = -ENOENT;
> > - ulong addr = 0, size;
> > - /*
> > - * If U-Boot is not in the first phase, an existing bloblist must be
> > - * at a fixed address.
> > - */
> > - bool from_addr = fixed && !xpl_is_first_phase();
> > - /*
> > - * If U-Boot is in the first phase that an arch custom routine
> > should
> > - * install the bloblist passed from previous loader to this fixed
> > + ulong addr, size;
> > + bool expected;
> > +
> > + /**
> > + * We don't expect to find an existing bloblist in the first phase
> > of
> > + * U-Boot that runs. Also we have no way to receive the address of
> > an
> > + * allocated bloblist from a previous stage, so it must be at a
> > fixed
> > * address.
> > */
> > - bool from_boot_arg = fixed && xpl_is_first_phase();
> > -
> > + expected = fixed && !xpl_is_first_phase();
> > if (xpl_prev_phase() == PHASE_TPL &&
> > !IS_ENABLED(CONFIG_TPL_BLOBLIST))
> > - from_addr = false;
> > + expected = false;
> > if (fixed)
> > addr = IF_ENABLED_INT(CONFIG_BLOBLIST_FIXED,
> > CONFIG_BLOBLIST_ADDR);
> > size = CONFIG_BLOBLIST_SIZE;
> > -
> > - if (from_boot_arg)
> > - ret = xferlist_from_boot_arg(addr, size);
> > - else if (from_addr)
> > + if (expected) {
> > ret = bloblist_check(addr, size);
> > -
> > - if (ret)
> > - log_warning("Bloblist at %lx not found (err=%d)\n",
> > - addr, ret);
> > - else
> > - /* Get the real size */
> > - size = gd->bloblist->total_size;
> > -
> > + if (ret) {
> > + log_warning("Expected bloblist at %lx not found
> > (err=%d)\n",
> > + addr, ret);
> > + } else {
> > + /* Get the real size, if it is not what we expected
> > */
> > + size = gd->bloblist->total_size;
> > + }
> > + }
> > if (ret) {
> > - /*
> > - * If we don't have a bloblist from a fixed address, or the
> > one
> > - * in the fixed address is not valid. we must allocate the
> > - * memory for it now.
> > - */
> > if (CONFIG_IS_ENABLED(BLOBLIST_ALLOC)) {
> > void *ptr = memalign(BLOBLIST_ALIGN, size);
> >
> > @@ -545,8 +525,7 @@ int bloblist_init(void)
> > return log_msg_ret("alloc", -ENOMEM);
> > addr = map_to_sysmem(ptr);
> > } else if (!fixed) {
> > - return log_msg_ret("BLOBLIST_FIXED is not enabled",
> > - ret);
> > + return log_msg_ret("!fixed", ret);
> > }
> > log_debug("Creating new bloblist size %lx at %lx\n", size,
> > addr);
> > @@ -559,11 +538,6 @@ int bloblist_init(void)
> > return log_msg_ret("ini", ret);
> > gd->flags |= GD_FLG_BLOBLIST_READY;
> >
> > -#ifdef DEBUG
> > - bloblist_show_stats();
> > - bloblist_show_list();
> > -#endif
> > -
> > return 0;
> > }
> >
> > diff --git a/include/bloblist.h b/include/bloblist.h
> > index 1e1ca34aa92..5063ab66e4a 100644
> > --- a/include/bloblist.h
> > +++ b/include/bloblist.h
> > @@ -482,14 +482,4 @@ static inline int bloblist_maybe_init(void)
> > */
> > int bloblist_check_reg_conv(ulong rfdt, ulong rzero, ulong rsig);
> >
> > -/**
> > - * xferlist_from_boot_arg() - Get bloblist from the boot args and relocate
> > it
> > - * to the specified address.
> > - *
> > - * @addr: Address for the bloblist
> > - * @size: Size of space reserved for the bloblist
> > - * Return: 0 if OK, else on error
> > - */
> > -int xferlist_from_boot_arg(ulong addr, ulong size);
> > -
> > #endif /* __BLOBLIST_H */
> > --
> > 2.43.0
> >
