[REGRESSION] imx: spl_imx_romapi: boot loops

[Marcel Ziswiler]

Hi Rasmus

On Tue, 2023-10-24 at 16:32 +0200, Rasmus Villemoes wrote:
> 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?

That did not help.


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

Yes, we use eMMC fast boot mode.

> 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?

Good question. Some more debugging revealed that we are missing 464 bytes at the beginning of the buffer. Why
would that be?

> Rasmus

Cheers

Marcel