Dear all, Any updates or new comments on this? How should I proceed?
BRs/Baocheng Su On Thu, 2022-10-20 at 15:44 +0200, Mark Kettenis wrote: > > From: Simon Glass <s...@chromium.org> > > Date: Wed, 19 Oct 2022 08:15:35 -0600 > > > > Hi Mark, > > > > On Wed, 19 Oct 2022 at 08:08, Mark Kettenis > > <mark.kette...@xs4all.nl> wrote: > > > > > > > From: Simon Glass <s...@chromium.org> > > > > Date: Wed, 19 Oct 2022 07:18:10 -0600 > > > > > > > > Hi, > > > > > > > > On Mon, 17 Oct 2022 at 05:53, Su, Bao Cheng > > > > <baocheng...@siemens.com> wrote: > > > > > > > > > > Hi Simon, > > > > > > > > > > > > > +Tom Rini for guidance > > > > > > > > > On Sat, 2022-07-30 at 19:27 -0600, Simon Glass wrote: > > > > > > Hi Bao Cheng, > > > > > > > > > > > > On Sat, 30 Jul 2022 at 03:05, Su, Bao Cheng > > > > > > <baocheng...@siemens.com> wrote: > > > > > > > > > > > > > > Commit 71551055cbdb ("spl: fit: Load devicetree when a > > > > > > > Linux payload is > > > > > > > found") made a change to not report the spl_fit_append_fdt > > > > > > > error at all > > > > > > > if next-stage image is u-boot. > > > > > > > > > > > > > > However for u-boot image without CONFIG_OF_EMBED, the > > > > > > > error should be > > > > > > > reported to uplevel caller. Otherwise, uplevel caller > > > > > > > would think the > > > > > > > fdt is already loaded which is obviously not true. > > > > > > > > > > > > > > Signed-off-by: Baocheng Su <baocheng...@siemens.com> > > > > > > > --- > > > > > > > > > > > > > > Changes in v2: > > > > > > > - Fix the wrong wrapping > > > > > > > > > > > > > > common/spl/spl_fit.c | 8 ++++++-- > > > > > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c > > > > > > > index a35be52965..00404935cb 100644 > > > > > > > --- a/common/spl/spl_fit.c > > > > > > > +++ b/common/spl/spl_fit.c > > > > > > > @@ -770,8 +770,12 @@ int spl_load_simple_fit(struct > > > > > > > spl_image_info *spl_image, > > > > > > > */ > > > > > > > if (os_takes_devicetree(spl_image->os)) { > > > > > > > ret = spl_fit_append_fdt(spl_image, info, > > > > > > > sector, &ctx); > > > > > > > - if (ret < 0 && spl_image->os != > > > > > > > IH_OS_U_BOOT) > > > > > > > - return ret; > > > > > > > + if (ret < 0) { > > > > > > > + if (spl_image->os != IH_OS_U_BOOT) > > > > > > > + return ret; > > > > > > > + else if > > > > > > > (!IS_ENABLED(CONFIG_OF_EMBED)) > > > > > > > + return ret; > > > > > > > > > > > > This is a pretty unpleasant condition. I think we would be > > > > > > better to > > > > > > report the error and let the caller figure it out. > > > > > > > > > > > > There are no tests associated with this, so it is hard to > > > > > > know what is > > > > > > actually going on. > > > > > > > > > > > > If we must have this workaround, I suggest adding a Kconfig > > > > > > so boards > > > > > > that need it can turn it on, and other boards can use normal > > > > > > operation, which is to report errors. > > > > > > > > > > > > > > > > Since there is no particular error code stands for such kind > > > > > of > > > > > scenario, it would be hard for the caller to determine which > > > > > step has > > > > > the problem. > > > > > > > > > > Or below code is more clear? > > > > > > > > > > if (os_takes_devicetree(spl_image->os)) { > > > > > ret = spl_fit_append_fdt(spl_image, info, > > > > > sector, &ctx); > > > > > - if (ret < 0 && spl_image->os != IH_OS_U_BOOT) > > > > > - return ret; > > > > > + if (ret < 0 > > > > > + && (spl_image->os != IH_OS_U_BOOT > > > > > + || !IS_ENABLED(CONFIG_OF_EMBED))) > > > > > + return ret; > > > > > } > > > > > > > > > > Actually there is already the `CONFIG_OF_EMBED` to tell them > > > > > apart, see > > > > > the previous logic before commit 71551055cbdb: > > > > > > > > > > * Booting a next-stage U-Boot may require us to > > > > > append the FDT. > > > > > * We allow this to fail, as the U-Boot image might > > > > > embed its > > > > > FDT. > > > > > */ > > > > > - if (spl_image->os == IH_OS_U_BOOT) { > > > > > + if (os_takes_devicetree(spl_image->os)) { > > > > > ret = spl_fit_append_fdt(spl_image, info, > > > > > sector, &ctx); > > > > > - if (!IS_ENABLED(CONFIG_OF_EMBED) && ret < 0) > > > > > + if (ret < 0 && spl_image->os != IH_OS_U_BOOT) > > > > > return ret; > > > > > } > > > > > > > > > > So before the commit 71551055cbdb, the normal case would be to > > > > > report > > > > > the error, but the commit in question changed this to not > > > > > report the > > > > > error for normal spl to boot u-boot, only reports error for > > > > > SPL to boot > > > > > kernel, i.e. falcon mode. > > > > > > > > We don't (or shouldn't) have boards which use OF_EMBED in > > > > mainline, so > > > > that condition doesn't seem to make sense to me. > > > > > > We have plenty of boards that set OF_EMBED, and as some of us have > > > pointed out to you more than once before, there are several valid > > > use > > > cases for it. > > > > Can you point me to the discussion about the valid use cases? > > Not easily since there were several lengthy discussions about device > trees. > > Most of the use cases boil down to the following: > > * There is some low-level firmware or virtualization layer that can't > be changed. > > * This layer does not provide a device tree that we can use in U-Boot. > > * This layer is rather opiniated on the binaries it loads, for example > it can only load an ELF or a PE file. > > So we have to make U-Boot look like such a file and include a device > tree directly in the binary in a way such that it gets loaded as part > of that binary. This is what OF_EMBED achieves. > > Cheers, > > Mark