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 Regards, Simon