Hi Alexandru,

On Fri, 12 Mar 2021 at 10:32, Alexandru Gagniuc mr.nuke.me@gmail.com wrote:

The correct FDT to use is described by the "fdt" property of the configuration node. When the fit_unamep argument to fit_image_load()

fit_unamep (and below

is "fdt", we get the "/images/fdt" node. This is incorrect, as it ignores the "fdt" property of the config node, and in most cases, the "/images/fdt" node doesn't exist.

When the 'fit_unamep' argument is NULL, fit_image_load() uses the IH_TYPE_FLATDT value to read the config property "fdt", which points to the correct FDT node(s).

fit_image_load() should probably be split into a function that reads an image by name, and one that reads an image by config reference. I don't make those decisions, I just point out the craziness.

Somewhere you need to add a simple sentence explaining what your change does.

Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com

common/spl/spl.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)

Reviewed-by: Simon Glass sjg@chromium.org

diff --git a/common/spl/spl.c b/common/spl/spl.c index e3d84082f4..986cfbc6fd 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -201,7 +201,6 @@ static int spl_load_fit_image(struct spl_image_info *spl_image, { bootm_headers_t images; const char *fit_uname_config = NULL;

•   const char *fit_uname_fdt = FIT_FDT_PROP;
    const char *uname;
    ulong fw_data = 0, dt_data = 0, img_data = 0;
    ulong fw_len = 0, dt_len = 0, img_len = 0;
  

@@ -230,8 +229,7 @@ static int spl_load_fit_image(struct spl_image_info *spl_image, #ifdef CONFIG_SPL_FIT_SIGNATURE images.verify = 1; #endif

•   ret = fit_image_load(&images, (ulong)header,
  

•                  &fit_uname_fdt, &fit_uname_config,
  

•   ret = fit_image_load(&images, (ulong)header, NULL, &fit_uname_config,
                   IH_ARCH_DEFAULT, IH_TYPE_FLATDT, -1,
                   FIT_LOAD_OPTIONAL, &dt_data, &dt_len);
    if (ret >= 0)
  

-- 2.26.2

On Fri, 12 Mar 2021 at 10:32, Alexandru Gagniuc mr.nuke.me@gmail.com wrote:

The information on the OS should be contained in the FIT, as the self-explanatory "os" property of a node under /images. Hard-coding this to U_BOOT might send us down the wrong path later in the boot process.

Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com

common/spl/spl.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/common/spl/spl.c b/common/spl/spl.c index 986cfbc6fd..8f6c8dba6f 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -220,8 +220,9 @@ static int spl_load_fit_image(struct spl_image_info *spl_image, spl_image->size = fw_len; spl_image->entry_point = fw_data; spl_image->load_addr = fw_data;

•   spl_image->os = IH_OS_U_BOOT;
  

Again I wonder about the code-size impact of this. Should it depend on Kconfig?

•   spl_image->name = "U-Boot";
  

•   if (!fit_image_get_os(header, ret, &spl_image->os))
  

•           spl_image->os = IH_OS_INVALID;
  

•   spl_image->name = genimg_get_os_name(spl_image->os);
  
    debug(SPL_TPL_PROMPT "payload image: %32s load addr: 0x%lx size: %d\n",
          spl_image->name, spl_image->load_addr, spl_image->size);
  

-- 2.26.2

Regards, Simon

Hi Alexandru,

On Fri, 12 Mar 2021 at 10:32, Alexandru Gagniuc mr.nuke.me@gmail.com wrote:

U-Boot expects the FDT to be located right after the _end linker symbol (see fdtdec.c: board_fdt_blob_setup())

The "basic" LOAD_FIT path is aware of this limitation, and relocates the FDT at the expected location. Guessing the expected location probably only works reliably on 32-bit arm, and it feels like a hack. One proposal would be to pass the FDT address to u-boot (e.g. using 'r2' on arm platforms).

We could do that, but the current behaviour is OK. It is required that you can joined U-Boot and its FDT and get a valid image.

The variable is named "fdt_hack" to remind future contributors that, "hey! we should fix the underlying problem". However, that is beyond the scope of this patch.

I think it is fine to require it be after U-Boot, in fact that is defined by U-Boot. So I don't think you need to think of it as a hack.

Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com

common/spl/spl.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/common/spl/spl.c b/common/spl/spl.c index 8f6c8dba6f..e63f05bb33 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -201,6 +201,7 @@ static int spl_load_fit_image(struct spl_image_info *spl_image, { bootm_headers_t images; const char *fit_uname_config = NULL;

•   uintptr_t fdt_hack;
    const char *uname;
    ulong fw_data = 0, dt_data = 0, img_data = 0;
    ulong fw_len = 0, dt_len = 0, img_len = 0;
  

@@ -233,9 +234,18 @@ static int spl_load_fit_image(struct spl_image_info *spl_image, ret = fit_image_load(&images, (ulong)header, NULL, &fit_uname_config, IH_ARCH_DEFAULT, IH_TYPE_FLATDT, -1, FIT_LOAD_OPTIONAL, &dt_data, &dt_len);

•   if (ret >= 0)
  

•   if (ret >= 0) {
            spl_image->fdt_addr = (void *)dt_data;
  
  

•           if (spl_image->os == IH_OS_U_BOOT) {
  

•                   /* HACK: U-boot expects FDT at a specific address */
  

•                   fdt_hack = spl_image->load_addr + spl_image->size;
  

•                   fdt_hack = (fdt_hack + 3) & ~3;
  

I don't think this is needed and it just confuses things. If U-Boot is not an aligned sign, it can't boot because the DT ends up in the wrong place. The build system makes sure this doesn't happen.

•                   debug("Relocating FDT to %p\n", spl_image->fdt_addr);
  

•                   memcpy((void *)fdt_hack, spl_image->fdt_addr, dt_len);
  

•           }
  

•   }
  

•   conf_noffset = fit_conf_get_node((const void *)header,
                                     fit_uname_config);
    if (conf_noffset <= 0)
  

-- 2.26.2

Regards, Simon

On 3/29/21 2:43 AM, Simon Glass wrote:

HI Alexandru,

On Fri, 12 Mar 2021 at 10:32, Alexandru Gagniuc mr.nuke.me@gmail.com wrote:

...

The 'firmware' property of a config node takes precedence over the 'kernel' property. 'standalone' is deprecated. However, give users a couple of releases where 'standalone' still works, but warns loudly.

Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com

common/spl/spl.c | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/common/spl/spl.c b/common/spl/spl.c index e63f05bb33..da4751b4ac 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -214,7 +214,24 @@ static int spl_load_fit_image(struct spl_image_info *spl_image, ret = fit_image_load(&images, (ulong)header, NULL, &fit_uname_config, IH_ARCH_DEFAULT, IH_TYPE_STANDALONE, -1,

•                        FIT_LOAD_REQUIRED, &fw_data, &fw_len);
  

•                        FIT_LOAD_OPTIONAL, &fw_data, &fw_len);
  

•   if (ret >= 0) {
  

•           printf("DEPRECATED: 'standalone = ' property.");
  

•           printf("Please use either 'firmware =' or 'kernel ='\n");
  

•   } else {
  

•           ret = fit_image_load(&images, (ulong)header, NULL,
  

•                                &fit_uname_config, IH_ARCH_DEFAULT,
  

•                                IH_TYPE_FIRMWARE, -1, FIT_LOAD_OPTIONAL,
  

•                                &fw_data, &fw_len);
  

•   }
  

•   if (ret < 0) {
  

•           ret = fit_image_load(&images, (ulong)header, NULL,
  

•                                &fit_uname_config, IH_ARCH_DEFAULT,
  

•                                IH_TYPE_KERNEL, -1, FIT_LOAD_OPTIONAL,
  

•                                &fw_data, &fw_len);
  

Should this only be for Falcon mode?

I don't know. Falcon mode relies on u-boot proper preparing an FDT and saving it to storage. Then the falcon boot reads the kernel image, and the "raw" FDT (see "spl export").

On the one hand, yes, because falcon mode loads kernels directly. On the other hand, no, because we're not using this prepared FDT.

There's a distinction between falcon mode and kernel boot from FIT. And it's very muddy. I'm hoping that people will stop using falcon mode with legacy images. Then and only then, we won't have to make this distinction.

Alex

...

•   }
  

•    if (ret < 0)
             return ret;
  
  

-- 2.26.2

Regards, Simon

On Fri, 12 Mar 2021 at 10:32, Alexandru Gagniuc mr.nuke.me@gmail.com wrote:

Consider the following FIT:

    images {
            whipple {};
    };
    configurations {
            conf-1 {
                    firmware = "whipple";
            };
    };

Getting the 'firmware' image with fit_image_load() is not possible, as it doesn't understand 'firmware =' properties. Although one could pass IH_TYPE_FIRMWARE for 'image_type', this needs to be converted to a "firmware" string for FDT lookup -- exactly what this change does.

Signed-off-by: Alexandru Gagniuc mr.nuke.me@gmail.com

common/image-fit.c | 2 ++ 1 file changed, 2 insertions(+)

Reviewed-by: Simon Glass sjg@chromium.org

diff --git a/common/image-fit.c b/common/image-fit.c index 28b3d2b191..8cd1621a18 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1955,6 +1955,8 @@ static const char *fit_get_image_type_property(int type) return FIT_FDT_PROP; case IH_TYPE_KERNEL: return FIT_KERNEL_PROP;

•   case IH_TYPE_FIRMWARE:
  

•           return FIT_FIRMWARE_PROP;
    case IH_TYPE_RAMDISK:
            return FIT_RAMDISK_PROP;
    case IH_TYPE_X86_SETUP:
  

-- 2.26.2