[REGRESSION] imx: spl_imx_romapi: boot loops

[Rasmus Villemoes]

On 24/10/2023 15.15, Marcel Ziswiler wrote:

Hi Marcel

tl;dr: can you try

@@ -330,7 +335,7 @@ static int spl_romapi_load_image_stream(struct
spl_image_info *spl_image,

                ss.base = phdr;
                ss.end = p;
-               ss.pagesize = pagesize;
+               ss.pagesize = 1024;

                memset(&load, 0, sizeof(load));
                load.bl_len = 1;

and also put 1024 in the binman node so u-boot.itb is also 1024-byte
aligned?

This will likely break usb boot (where pagesize==1), so if you want the
binary to work with usb you can make the RHS instead "(pagesize > 1 ?
1024 : 1)" or something like that.

I don't think it will work, but OTOH my analysis below doesn't find any
other (fundamental) difference between the old and new code.

> 
> On Tue, 2023-10-24 at 13:17 +0200, Rasmus Villemoes wrote:
> 
>>
>> and this works just fine. But in my case ss->pagesize is 1, whereas you
>> have 512.
>>
>> Just exactly how are you booting? It says "Boot Stage: Primary boot"
>> whereas I'm doing serial download with uuu (i.e. "Boot Stage: USB boot").
> 
> Yes, regular eMMC boot.

[snip]

> 
> At least I am not aware that we would be doing anything special for eMMC boot.

Well, I/we usually do boot from eMMC (except during bootstrap where we
use uuu), more precisely from the eMMC boot partitions, but that doesn't
end up using the spl_romapi_load_image_stream() but instead the expected
spl_romapi_load_image_seekable().

Perhaps some NXP folks can explain the logic behind

static int is_boot_from_stream_device(u32 boot)
{
	u32 interface;

	interface = boot >> 16;
	if (interface >= BT_DEV_TYPE_USB)
		return 1;

	if (interface == BT_DEV_TYPE_MMC && (boot & 1))
		return 1;

	return 0;
}

Apparently that "boot & 1" is an indication that "eMMC fast boot mode"
is enabled (not to be confused with fastboot which is a USB protocol
that only becomes relevant once U-Boot proper is up and running).

As I've complained about previously, the ROM API is almost entirely
undocumented. There's something in imx93rm.pdf (which is where I have
that eMMC fast boot info from), but I have no idea if that applies to
imx8/imx8mp as well.

Are there any alignment requirements on the destination and/or the size
parameter? Is there some timing requirements so that if we read too late
the data is lost? So could e.g. a bunch of printf debugging break boot?

Because I really can't see the fundamental difference between now and
then. Both before and after the commit in question, the code does:

- read 1024 bytes at a time, search each chunk for FDT magic

This then gives the "Find img info 0x4802b000"

- make sure the leftover within that 1024 chunk is a full FDT header
(usually ok, so no output)

- read the FDT size from that header, round up to a multiple of 1024,
fetch that - that's the "Need continue download 1024".

And then the flow deviates.

The old code would do the fake spl_fit_load to figure out the maximum
offset (thus the real total .itb size), then take that remaining size,
round up to ->pagesize (512), and fetch that with one final
rom_api_download_image(p, 0, imagesize);.

My code instead does a number of rom_api_download_image() calls, where
each size argument is rounded up to the ->pagesize from the boot_info
query (i.e. 512). So it's possible that for one of those calls, the
destination is only 512-byte aligned (because a previous call fetched an
odd number of 512 byte blocks), and that doesn't happen in the previous
case where all but the last rom_api_download_image() have fetched 1024
byte aligned chunks.

What am I missing?

Rasmus