| Message ID | 20240605032521.1142768-6-sjg@chromium.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Bug-fixes for a few boards | expand |
Hi Simon, On Wed, 5 Jun 2024 at 06:26, Simon Glass <sjg@chromium.org> wrote: > > On some boards, the bloblist is created in SPL once SDRAM is ready. It > cannot be accessed until that point, so is not available early in SPL. > > Add a condition to avoid a hang in this case. > > This fixes a hang in chromebook_coral > > Fixes: 70fe2385943 ("fdt: Allow the devicetree to come from a bloblist") > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > 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 > + * The necessary test is whether the previous stage passed a bloblist, > + * not whether this one creates one. > + */ > + if (CONFIG_IS_ENABLED(OF_BLOBLIST) && > + (spl_prev_phase() != PHASE_TPL || > + !IS_ENABLED(CONFIG_TPL_BLOBLIST))) { The same condition exists in common/bloblist.c. Carve out a function --e.g bool can can_enable_bloblist(void) return .... instead of open coding that Thanks /Ilias > ret = bloblist_maybe_init(); > if (!ret) { > gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0); > -- > 2.34.1 >
Hi Ilias, On Tue, 4 Jun 2024 at 23:33, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > On Wed, 5 Jun 2024 at 06:26, Simon Glass <sjg@chromium.org> wrote: > > > > On some boards, the bloblist is created in SPL once SDRAM is ready. It > > cannot be accessed until that point, so is not available early in SPL. > > > > Add a condition to avoid a hang in this case. > > > > This fixes a hang in chromebook_coral > > > > Fixes: 70fe2385943 ("fdt: Allow the devicetree to come from a bloblist") > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > 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 > > + * The necessary test is whether the previous stage passed a bloblist, > > + * not whether this one creates one. > > + */ > > + if (CONFIG_IS_ENABLED(OF_BLOBLIST) && > > + (spl_prev_phase() != PHASE_TPL || > > + !IS_ENABLED(CONFIG_TPL_BLOBLIST))) { > > The same condition exists in common/bloblist.c. > Carve out a function --e.g > > bool can can_enable_bloblist(void) > return .... > > instead of open coding that Unfortunately it looks like the conditions are different, with the one you mention being: if (spl_prev_phase() == PHASE_TPL && !IS_ENABLED(CONFIG_TPL_BLOBLIST)) from_addr = false; (is that the one you mean?) So I don't think I can combine them into a helper function. Regards, Simon
Hi Simon, On Tue, 4 Jun 2024 at 23:27, Simon Glass <sjg@chromium.org> wrote: > On some boards, the bloblist is created in SPL once SDRAM is ready. It > cannot be accessed until that point, so is not available early in SPL. > > Add a condition to avoid a hang in this case. > > This fixes a hang in chromebook_coral > > Fixes: 70fe2385943 ("fdt: Allow the devicetree to come from a bloblist") > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > 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? > + * The necessary test is whether the previous stage passed a > bloblist, > + * not whether this one creates one. > + */ > + if (CONFIG_IS_ENABLED(OF_BLOBLIST) && > + (spl_prev_phase() != PHASE_TPL || > + !IS_ENABLED(CONFIG_TPL_BLOBLIST))) { > ret = bloblist_maybe_init(); > if (!ret) { > gd->fdt_blob = > bloblist_find(BLOBLISTT_CONTROL_FDT, 0); > -- > 2.34.1 > > Regards, Raymond
Hi Raymond, On Mon, 10 Jun 2024 at 13:13, Raymond Mao <raymond.mao@linaro.org> wrote: > > Hi Simon, > > On Tue, 4 Jun 2024 at 23:27, Simon Glass <sjg@chromium.org> wrote: >> >> On some boards, the bloblist is created in SPL once SDRAM is ready. It >> cannot be accessed until that point, so is not available early in SPL. >> >> Add a condition to avoid a hang in this case. >> >> This fixes a hang in chromebook_coral >> >> Fixes: 70fe2385943 ("fdt: Allow the devicetree to come from a bloblist") >> >> Signed-off-by: Simon Glass <sjg@chromium.org> >> --- >> >> 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. > >> >> + * The necessary test is whether the previous stage passed a bloblist, >> + * not whether this one creates one. >> + */ >> + if (CONFIG_IS_ENABLED(OF_BLOBLIST) && >> + (spl_prev_phase() != PHASE_TPL || >> + !IS_ENABLED(CONFIG_TPL_BLOBLIST))) { >> ret = bloblist_maybe_init(); >> if (!ret) { >> gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0); >> -- >> 2.34.1 >> > > Regards, > Raymond Regards, Simon
On Tue, 11 Jun 2024 at 13:54, Simon Glass <sjg@chromium.org> wrote: > > Hi Raymond, > > On Mon, 10 Jun 2024 at 13:13, Raymond Mao <raymond.mao@linaro.org> wrote: > > > > Hi Simon, > > > > On Tue, 4 Jun 2024 at 23:27, Simon Glass <sjg@chromium.org> wrote: > >> > >> On some boards, the bloblist is created in SPL once SDRAM is ready. It > >> cannot be accessed until that point, so is not available early in SPL. > >> > >> Add a condition to avoid a hang in this case. > >> > >> This fixes a hang in chromebook_coral > >> > >> Fixes: 70fe2385943 ("fdt: Allow the devicetree to come from a bloblist") > >> > >> Signed-off-by: Simon Glass <sjg@chromium.org> > >> --- > >> > >> 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? Grepping for OF_BLOBLIST, I can't find any matches, so is the above if a typo? Thanks /Ilias > > > > >> > >> + * The necessary test is whether the previous stage passed a bloblist, > >> + * not whether this one creates one. > >> + */ > >> + if (CONFIG_IS_ENABLED(OF_BLOBLIST) && > >> + (spl_prev_phase() != PHASE_TPL || > >> + !IS_ENABLED(CONFIG_TPL_BLOBLIST))) { > >> ret = bloblist_maybe_init(); > >> if (!ret) { > >> gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0); > >> -- > >> 2.34.1 > >> > > > > Regards, > > Raymond > > Regards, > Simon
Hi Ilias, On Tue, 11 Jun 2024 at 08:27, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > On Tue, 11 Jun 2024 at 13:54, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Raymond, > > > > On Mon, 10 Jun 2024 at 13:13, Raymond Mao <raymond.mao@linaro.org> wrote: > > > > > > Hi Simon, > > > > > > On Tue, 4 Jun 2024 at 23:27, Simon Glass <sjg@chromium.org> wrote: > > >> > > >> On some boards, the bloblist is created in SPL once SDRAM is ready. It > > >> cannot be accessed until that point, so is not available early in SPL. > > >> > > >> Add a condition to avoid a hang in this case. > > >> > > >> This fixes a hang in chromebook_coral > > >> > > >> Fixes: 70fe2385943 ("fdt: Allow the devicetree to come from a bloblist") > > >> > > >> Signed-off-by: Simon Glass <sjg@chromium.org> > > >> --- > > >> > > >> 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. 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. 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 :-) Regards, Simon > > > Thanks > /Ilias > > > > > > > >> > > >> + * The necessary test is whether the previous stage passed a bloblist, > > >> + * not whether this one creates one. > > >> + */ > > >> + if (CONFIG_IS_ENABLED(OF_BLOBLIST) && > > >> + (spl_prev_phase() != PHASE_TPL || > > >> + !IS_ENABLED(CONFIG_TPL_BLOBLIST))) { > > >> ret = bloblist_maybe_init(); > > >> if (!ret) { > > >> gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0); > > >> -- > > >> 2.34.1 > > >> > > > > > > Regards, > > > Raymond > > > > Regards, > > Simon
[...] > > > >> --- > > > >> > > > >> 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. > 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 > 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. 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. Regards /Ilias > > Regards, > Simon > > > > > > > > > Thanks > > /Ilias > > > > > > > > > > >> > > > >> + * The necessary test is whether the previous stage passed a bloblist, > > > >> + * not whether this one creates one. > > > >> + */ > > > >> + if (CONFIG_IS_ENABLED(OF_BLOBLIST) && > > > >> + (spl_prev_phase() != PHASE_TPL || > > > >> + !IS_ENABLED(CONFIG_TPL_BLOBLIST))) { > > > >> ret = bloblist_maybe_init(); > > > >> if (!ret) { > > > >> gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0); > > > >> -- > > > >> 2.34.1 > > > >> > > > > > > > > Regards, > > > > Raymond > > > > > > Regards, > > > Simon
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 Regards, Simon
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()? Thanks /Ilias > > Regards, > Simon
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. Regards, Simon
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
Hi Raymond, On Fri, 21 Jun 2024 at 10:40, Raymond Mao <raymond.mao@linaro.org> wrote: > > 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); 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. Regards, Simon
Hi all, On Fri, 21 Jun 2024 at 12:16, Simon Glass <sjg@chromium.org> wrote: > > Hi Raymond, > > On Fri, 21 Jun 2024 at 10:40, Raymond Mao <raymond.mao@linaro.org> wrote: > > > > 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); > > 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. Regards, Simon
Hi Simon, On Tue, 30 Jul 2024 at 18:35, Simon Glass <sjg@chromium.org> wrote: > Hi all, > > On Fri, 21 Jun 2024 at 12:16, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Raymond, > > > > On Fri, 21 Jun 2024 at 10:40, Raymond Mao <raymond.mao@linaro.org> > wrote: > > > > > > 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); > > > > 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.mao@linaro.org> Regards, Raymond
Hi Simon, On Wed, 31 Jul 2024 at 01:35, Simon Glass <sjg@chromium.org> wrote: > > Hi all, > > On Fri, 21 Jun 2024 at 12:16, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Raymond, > > > > On Fri, 21 Jun 2024 at 10:40, Raymond Mao <raymond.mao@linaro.org> wrote: > > > > > > 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); > > > > 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. Raymond acked the patch, I don't mind merging it Regards /Ilias > > Regards, > Simon
Hi Raymond, On Wed, 31 Jul 2024 at 07:59, Raymond Mao <raymond.mao@linaro.org> wrote: > > Hi Simon, > > On Tue, 30 Jul 2024 at 18:35, Simon Glass <sjg@chromium.org> wrote: >> >> Hi all, >> >> On Fri, 21 Jun 2024 at 12:16, Simon Glass <sjg@chromium.org> wrote: >> > >> > Hi Raymond, >> > >> > On Fri, 21 Jun 2024 at 10:40, Raymond Mao <raymond.mao@linaro.org> wrote: >> > > >> > > 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); >> > >> > 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.mao@linaro.org> Thank you. Regards, Simon
Hi Ilias, On Wed, 31 Jul 2024 at 14:09, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > On Wed, 31 Jul 2024 at 01:35, Simon Glass <sjg@chromium.org> wrote: > > > > Hi all, > > > > On Fri, 21 Jun 2024 at 12:16, Simon Glass <sjg@chromium.org> wrote: > > > > > > Hi Raymond, > > > > > > On Fri, 21 Jun 2024 at 10:40, Raymond Mao <raymond.mao@linaro.org> wrote: > > > > > > > > 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); > > > > > > 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. > > Raymond acked the patch, I don't mind merging it OK thank you. There is another one too - see the note at the end. https://patchwork.ozlabs.org/project/uboot/patch/20240721100940.3202027-6-sjg@chromium.org/ Regards, Simon
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 + * The necessary test is whether the previous stage passed a bloblist, + * not whether this one creates one. + */ + if (CONFIG_IS_ENABLED(OF_BLOBLIST) && + (spl_prev_phase() != PHASE_TPL || + !IS_ENABLED(CONFIG_TPL_BLOBLIST))) { ret = bloblist_maybe_init(); if (!ret) { gd->fdt_blob = bloblist_find(BLOBLISTT_CONTROL_FDT, 0);
On some boards, the bloblist is created in SPL once SDRAM is ready. It cannot be accessed until that point, so is not available early in SPL. Add a condition to avoid a hang in this case. This fixes a hang in chromebook_coral Fixes: 70fe2385943 ("fdt: Allow the devicetree to come from a bloblist") Signed-off-by: Simon Glass <sjg@chromium.org> --- lib/fdtdec.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)