Hi Raymond,
On Wed, 31 Jul 2024 at 07:59, Raymond Mao <raymond....@linaro.org> wrote:
>
> Hi Simon,
>
> On Tue, 30 Jul 2024 at 18:35, Simon Glass <s...@chromium.org> wrote:
>>
>> Hi all,
>>
>> On Fri, 21 Jun 2024 at 12:16, Simon Glass <s...@chromium.org> wrote:
>> >
>> > Hi Raymond,
>> >
>> > On Fri, 21 Jun 2024 at 10:40, Raymond Mao <raymond....@linaro.org> wrote:
>> > >
>> > > Hi Simon,
>> > >
>> > > On Fri, 21 Jun 2024 at 10:58, Simon Glass <s...@chromium.org> wrote:
>> > >>
>> > >> Hi Ilias,
>> > >>
>> > >> On Wed, 19 Jun 2024 at 06:41, Ilias Apalodimas
>> > >> <ilias.apalodi...@linaro.org> wrote:
>> > >> >
>> > >> > Hi Simon,
>> > >> >
>> > >> > On Wed, Jun 12, 2024 at 02:24:31PM -0600, Simon Glass wrote:
>> > >> > > Hi Ilias,
>> > >> > >
>> > >> > > On Wed, 12 Jun 2024 at 00:02, Ilias Apalodimas
>> > >> > > <ilias.apalodi...@linaro.org> wrote:
>> > >> > > >
>> > >> > > > [...]
>> > >> > > >
>> > >> > > > > > > >> ---
>> > >> > > > > > > >>
>> > >> > > > > > > >> lib/fdtdec.c | 12 ++++++++++--
>> > >> > > > > > > >> 1 file changed, 10 insertions(+), 2 deletions(-)
>> > >> > > > > > > >>
>> > >> > > > > > > >> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
>> > >> > > > > > > >> index b2c59ab3818..b141244e3b9 100644
>> > >> > > > > > > >> --- a/lib/fdtdec.c
>> > >> > > > > > > >> +++ b/lib/fdtdec.c
>> > >> > > > > > > >> @@ -1669,8 +1669,16 @@ int fdtdec_setup(void)
>> > >> > > > > > > >> {
>> > >> > > > > > > >> int ret = -ENOENT;
>> > >> > > > > > > >>
>> > >> > > > > > > >> - /* If allowing a bloblist, check that first */
>> > >> > > > > > > >> - if (CONFIG_IS_ENABLED(BLOBLIST)) {
>> > >> > > > > > > >> + /*
>> > >> > > > > > > >> + * If allowing a bloblist, check that first.
>> > >> > > > > > > >> This would be better
>> > >> > > > > > > >> + * handled with an OF_BLOBLIST Kconfig, but that
>> > >> > > > > > > >> caused far too much
>> > >> > > > > > > >> + * argument, so add a hack here, used e.g. by
>> > >> > > > > > > >> chromebook_coral
>> > >> > > > > > > >
>> > >> > > > > > > > I am a bit confused by this comment - It means you will
>> > >> > > > > > > > not use OF_BLOBLIST,
>> > >> > > > > > > > but actually you are using it below. Is it a typo?
>> > >> > > > > > >
>> > >> > > > > > > Basically it would be cleaner to have a separate,
>> > >> > > > > > > phase-specific
>> > >> > > > > > > Kconfig control as to whether the DT can come from the
>> > >> > > > > > > bloblist (I
>> > >> > > > > > > can't remember the Kconfig name I suggested, nor the patch
>> > >> > > > > > > as it was
>> > >> > > > > > > last year sometime). But for now I am adding this hack to
>> > >> > > > > > > get a few
>> > >> > > > > > > boards working again.
>> > >> > > > > >
>> > >> > > > > > I am a bit confused.
>> > >> > > > > > First of all the comment is innapropriate. We went through a
>> > >> > > > > > lengthy
>> > >> > > > > > discussion on BLOBLIST/OF_BLOSLIST etc and, even Tom chimed
>> > >> > > > > > in and we
>> > >> > > > > > made up our minds. Why are you adding this comment now? Why
>> > >> > > > > > do code
>> > >> > > > > > comments have to illustrate your personal opinion -- which was
>> > >> > > > > > rejected?
>> > >> > > > >
>> > >> > > > > I'm sorry for the tone of the comment. I am not trying to offend
>> > >> > > > > anyone here and I'm happy to change the language.
>> > >> > > >
>> > >> > > > Yes please a comment explaining why that piece of code is there
>> > >> > > > is far
>> > >> > > > more intuitive.
>> > >> > >
>> > >> > > OK, once we have agreed the below I can do that.
>> > >> > >
>> > >> > > >
>> > >> > > > > As I probably
>> > >> > > > > mentioned at the time, my accepted patch breaks my workflow and
>> > >> > > > > several boards. I hope you can understand how frustrating that
>> > >> > > > > sort of
>> > >> > > > > thing can be.
>> > >> > > >
>> > >> > > > Yes, I do and I am fine with a short-term patch that fixes
>> > >> > > > issues, as
>> > >> > > > long as its not too intrusive. There is a very thin line between
>> > >> > > > quick
>> > >> > > > and dirty fixes to spaghetti unreadable code. But we should have
>> > >> > > > comments and/or commit messages indicating that this needs a
>> > >> > > > proper
>> > >> > > > fix
>> > >> > >
>> > >> > > I spent a lot of time explaining this last time.
>> > >> > >
>> > >> > > >
>> > >> > > > > Also, now that I have my lab back up and running and I
>> > >> > > > > would like these boards to work again on mainline!
>> > >> > > > >
>> > >> > > > > >
>> > >> > > > > > Grepping for OF_BLOBLIST, I can't find any matches, so is the
>> > >> > > > > > above if a typo?
>> > >> > > > >
>> > >> > > > > Remember, it was a patch you rejected :-)
>> > >> > > >
>> > >> > > > I don't maintain any of that. I only gave some feedback along the
>> > >> > > > lines of "bloblist was designed to be auto-discoverable, I don't
>> > >> > > > see
>> > >> > > > how adding an explicit Kconfig helps". IIRC we eventually followed
>> > >> > > > what Tom suggested.
>> > >> > >
>> > >> > > I'm not trying to point the finger here. So far the boards are
>> > >> > > broken
>> > >> > > in mainline...I'm just trying to fix that,
>> > >> > >
>> > >> > > >
>> > >> > > > In any case, the amount of bike-shedding in the topic is too
>> > >> > > > much. Do
>> > >> > > > you mind explaining the problem in your workflow again? Perhaps
>> > >> > > > we can
>> > >> > > > find a solution that is integrated in bloblist_maybe_init()
>> > >> > > > instead of
>> > >> > > > injecting ifs on when a bloblist should or should not be searched
>> > >> > > > for
>> > >> > > > all over the place.
>> > >> > >
>> > >> > > TPL (or SPL) sets up a bloblist with bits of info in it, but no DT
>> > >> > > (which is in memory-mapped SPI flash)
>> > >> > > U-Boot proper starts up, it wants to get the bloblist but not hang
>> > >> > > if
>> > >> > > the bloblist doesn't have a DT
>> > >> >
>> > >> > Sure, that's reasonable and IIRC that's the design we agreed a while
>> > >> > back.
>> > >> > Looking at the code, if the DT isn't found in a bloblist and the
>> > >> > feature is
>> > >> > enabled, we don't crash. We just print a debug message and continue
>> > >> > to find
>> > >> > the DT as we used. Why do we have to skip the call to
>> > >> > bloblist_maybe_init()?
>> > >>
>> > >> Because at this point, if there is no bloblist, then it needs to be
>> > >> created. But creating it this early may fail, e.g. since there is no
>> > >> malloc(). The intent of this code is to locate an FDT from an existing
>> > >> bloblist. There is no point in creating a new bloblist here, since it
>> > >> obviously won't have an FDT in it.
>> > >>
>> > > Can we add a bool arg to bloblist_init for this?
>> > > Eg. int bloblist_init(bool allow_malloc);
>> >
>> > Yes, we can do anything, but that seems like a hack to me...if we init
>> > the bloblist for the first time in the current phase, then it
>> > obviously won't have an FDT inside it. It is the unconditional
>> > requirement of an FDT which is causing the problems here.
>>
>> Can we apply this patch, please? I still have several broken boards in
>> my lab due to this.
>>
> Acked-by: Raymond Mao <raymond....@linaro.org>
Thank you.
Regards,
Simon
