Hi Tom, On Thu, 6 Feb 2025 at 17:20, Tom Rini <tr...@konsulko.com> wrote: > > On Thu, Feb 06, 2025 at 05:09:41PM -0700, Simon Glass wrote: > > Hi Raymond, > > > > On Thu, 6 Feb 2025 at 15:35, Raymond Mao <raymond....@linaro.org> wrote: > > > > > > Hi Simon, > > > > > > On Thu, 6 Feb 2025 at 10:41, Simon Glass <s...@chromium.org> wrote: > > > > > > > > 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. > > > > > > > > > > Still not very clear on what concerns you have and what is the way you > > > want to go. > > > The logic is straight forward, when U-Boot has a previous loader and > > > the registers pass in valid arguments - It indicates handoff should be > > > done using the transfer list. > > > Other kconfig options decide whether to use the passed in address > > > directly or copy to a predefined address. > > > But in either way, xferlist_from_boot_arg() is doing the right thing > > > to get the transfer list from the register if it exists and is valid. > > > I don't see a reason for removing it. > > > > Here is the initial code: > > >>>> > > 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 > > * address. > > */ > > bool from_boot_arg = fixed && xpl_is_first_phase(); > > > > if (xpl_prev_phase() == PHASE_TPL && !IS_ENABLED(CONFIG_TPL_BLOBLIST)) > > from_addr = false; > > <<<< > > > > and by the way, that is my tree. In -next it is even worse: > > >>>> > > /* > > * 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 > > * address. > > */ > > bool from_boot_arg = fixed && xpl_is_first_phase(); > > > > if (xpl_prev_phase() == PHASE_TPL && !IS_ENABLED(CONFIG_TPL_BLOBLIST)) > > from_addr = 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) > > ret = bloblist_check(addr, size); > > <<<< > > > > I want to update it so that TPL creates a bloblist and passes it > > through the following phases, ending up at U-Boot. > > > > To my mind, if CONFIG_IS_ENABLED(BLOBLIST), then we should check the > > registers and always accept standard passage. If not, we should > > either: > > - create a bloblist (if this *is* the first phase with > > CONFIG_IS_ENABLED(BLOBLIST)) > > - use an existing bloblistl, which must exist (if not) > > > > I just don't think the existing logic is a good starting point as it > > is too confusing. > > > > Perhaps, putting it another way, what do you like about the current > > code (either version)? > > We need to untie the check for and create logic. And we need to add code > so that U-Boot can pass from one phase to the next without a fixed > address. That should help simplify this logic and move towards being > able to just drop the fixed case entirely.
Well, if you want me to do this, I can, but it needs to start with this revert. If someone else is going to do it, I'll cheer from the sidelines. Regards, SImon
