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

[Marek Vasut]

On 7/13/21 4:41 PM, Tom Rini wrote:
> On Tue, Jul 13, 2021 at 04:35:38PM +0200, Marek Vasut wrote:
>> On 7/13/21 3:47 PM, Tom Rini wrote:
>>> 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 at 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.
>>>
>>> I'm not sure that we need to worry too much about mix-and-match
>>> SPL/U-Boot, but documenting what to go change if you must do it
>>> somewhere under doc/ would be good.  I think we can just switch to
>>> ALIGN(8) not ALIGN(4) and be done with it?
>>
>> Remember, there is also falcon boot. And we definitely have to be able to
>> have old u-boot (SPL) boot new fitImage and vice versa.
> 
> I don't follow you, sorry.  But since you seem to have the best
> understanding of where all of the cases something could go wrong here,
> can you perhaps post an RFC patch?  That is likely to be clearer than
> another long thread here.

I don't follow you, sorry. I believe the revert did the right thing and 
new systems should use mkimage -E when generating fitImages, to avoid 
the string alignment problem. That is all.