Re: [PATCH] spl: Align device tree blob address at 8-byte boundary

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 2989 bytes --]

On Mon, Jul 12, 2021 at 11:01:24AM -0500, Alex G. wrote:
> On 7/12/21 10:15 AM, Tom Rini wrote:
> > On Mon, Jul 12, 2021 at 01:36:14PM +0800, Bin Meng wrote:
> > > On Mon, Jul 12, 2021 at 1:21 PM Reuben Dowle <reuben.dowle@4rf.com> wrote:
> > > > 
> > > > I submitted an almost identical patch. See https://github.com/u-boot/u-boot/commit/eb39d8ba5f0d1468b01b89a2a464d18612d3ea76
> > > > 
> > > > This patch eventually had to be reverted (https://github.com/u-boot/u-boot/commit/5675ed7cb645f5ec13958726992daeeed16fd114), because it was causing issues on some platforms that had FIT on 32 bit boundary. However I continue to use it in production code, as without it the boot on my platform aborts.
> > > > 
> > > > I don't have time to investigate why this was happening, but you need to check this code won't just cause exactly the same faults.
> > > 
> > > Thanks for your information.
> > > 
> > > +Marek who did the revert
> > > 
> > > The revert commit message says:
> > > 
> > >      "The commit breaks booting of fitImage by SPL, the system simply
> > > hangs. This is because on arm32, the fitImage and all of its content
> > > can be aligned to 4 bytes and U-Boot expects just that."
> > > 
> > > I don't understand this. If an address is aligned to 8, it is already
> > > aligned to 4, so how did this commit make the system hang on arm32?
> > 
> > I think this had something to do with embedding contents somewhere in
> > the image?  There is a thread on the ML from then but I don't know how
> > informative it will end up being.
> 
> It's true that the flat devicetree spec requires an 8-byte alignment, even
> on 32-bit. The issues here are specific to u-boot.
> 
> SPL and u-boot have to agree where u-boot's FDT is located. We'll look at
> two cases:
> 	1) u-boot as a FIT (binary and FDT separately loaded)
> 	2) u-boot with embedded FDT
> 
> In case (1) SPL must place the FDT at a location where u-boot will find it.
> The current logic is
> 	SPL:	fdt = ALIGN_4(u_boot + u_boot_size)
> 	u-boot:	fdt = ALIGN_4(u_boot + u_boot_size)
> 
> In case (2), SPL's view of the FDT is not relevant, but instead the build
> system must place the FDT correctly:
> 	build:	fdt >> u-boot.bin
> 	u-boot:	fdt = ALIGN_4(u_boot + u_boot_size)
> 
> We have 3 places that must agree. A correct and complete patch could change
> all three, but one has to consider compatibility issues when crossing u-boot
> and SPL versions.
> 
> I had proposed in the revert discussion that SPL use r2 or similar mechanism
> to pass the location of the FDT to u-boot.

Looking at this again, and trying to figure out what we can do here most
easily, I think we need to have spl_fit_append_fdt() perhaps be split in
to "find the fdt" and then either a new function, or more logic within
the function for "ensure fdt is aligned properly".  We had made the
assumption that we can use the fdt in place in memory but that's not
always true.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]