Hi Simon,
On Fri, 21 Jun 2024 at 10:58, Simon Glass sjg@chromium.org wrote:
Hi Ilias,
On Wed, 19 Jun 2024 at 06:41, Ilias Apalodimas ilias.apalodimas@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.apalodimas@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);
Regards, Raymond