[PATCH v2] spl: fit: Report fdt error for loading u-boot
Simon Glass
sjg at chromium.org
Wed Apr 19 03:49:49 CEST 2023
Hi,
On Tue, 11 Apr 2023 at 04:24, Su, Bao Cheng <baocheng.su at siemens.com> wrote:
>
> Dear all,
>
> Any updates or new comments on this? How should I proceed?
This expands error reporting, so I think this is better than what we have.
Reviewed-by: Simon Glass <sjg at chromium.org>
More comments below.
>
> BRs/Baocheng Su
>
> On Thu, 2022-10-20 at 15:44 +0200, Mark Kettenis wrote:
> > > From: Simon Glass <sjg at chromium.org>
> > > Date: Wed, 19 Oct 2022 08:15:35 -0600
> > >
> > > Hi Mark,
> > >
> > > On Wed, 19 Oct 2022 at 08:08, Mark Kettenis
> > > <mark.kettenis at xs4all.nl> wrote:
> > > >
> > > > > From: Simon Glass <sjg at chromium.org>
> > > > > Date: Wed, 19 Oct 2022 07:18:10 -0600
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Mon, 17 Oct 2022 at 05:53, Su, Bao Cheng
> > > > > <baocheng.su at 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.su at 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.su at 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.
We can provide a devicetree in u-boot.bin
To me it seems that they all boil down to the late item, and even then
only ELF, since presumably we cannot build U-Boot as PE...or is that
UEFI?
I am working on a standard loader format (cue your n +1 cartoon :-)
and I hope that this can resolve this issue. In the meantime, for the
record, OF_EMBED is enabled by the following boards:
axs101
axs103
bcm947622
bcm94908
bcm94912
bcm963138
bcm963146
bcm963148
bcm963158
bcm963178
bcm96756
bcm96813
bcm96846
bcm96855
bcm96856
bcm96858
bcm96878
draco
efi-x86_app32
efi-x86_app64
emsdp
etamin
hsdk
iot_devkit
ls2080aqds_nand
ls2080aqds_qspi
ls2080aqds_sdcard
microblaze-generic
nsim_700
nsim_700be
nsim_hs38
nsim_hs38be
openpiton_riscv64
pxm2
rastaban
rpi
rpi_0_w
rpi_2
rpi_3
rpi_3_32b
rpi_3_b_plus
rut
smartweb
tb100
tbs2910
thuban
topic_miami
topic_miamilite
topic_miamiplus
vexpress_ca9x4
xilinx_versal_mini_emmc0
xilinx_versal_mini_emmc1
xilinx_versal_net_mini
xilinx_zynqmp_mini
xilinx_zynqmp_mini_emmc0
xilinx_zynqmp_mini_emmc1
xilinx_zynqmp_mini_nand
xilinx_zynqmp_mini_nand_single
xilinx_zynqmp_mini_qspi
xilinx_zynqmp_r5
zynq_cse_nand
zynq_cse_nor
zynq_cse_qspi
Regards,
Simon
More information about the U-Boot
mailing list