Hi Simon,

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@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@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.

Regards, Simon

From: Simon Glass sjg@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@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@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@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.

From: Simon Glass sjg@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@xs4all.nl wrote:

...

...

From: Simon Glass sjg@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@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@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@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

Hi,

On Tue, 11 Apr 2023 at 04:24, Su, Bao Cheng baocheng.su@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@chromium.org

More comments below.

BRs/Baocheng Su

On Thu, 2022-10-20 at 15:44 +0200, Mark Kettenis wrote:

...

...

From: Simon Glass sjg@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@xs4all.nl wrote:

...

...

From: Simon Glass sjg@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@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@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@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