[PATCH 5/9] fdt: Correct condition for bloblist existing

[Simon Glass]

Hi Ilias,

On Wed, 12 Jun 2024 at 00:02, Ilias Apalodimas
<ilias.apalodimas at 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

Regards,
Simon