On Mon, Jul 12, 2021 at 05:51:29PM +0200, Marek Vasut wrote:
On 7/12/21 5:43 PM, Tom Rini wrote:
On Mon, Jul 12, 2021 at 05:38:33PM +0200, Marek Vasut wrote:
On 7/12/21 5:15 PM, 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/eb39d8ba5f0d1468b01b89a2a464d18612d3...
This patch eventually had to be reverted (https://github.com/u-boot/u-boot/commit/5675ed7cb645f5ec13958726992daeeed16f...), 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 simplyhangs. 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.
If I recall this correctly, DT node alignment is 4 byte and that is what DTC emits. If you have fitImage with embedded data, you basically end up with
/ { prop1 = "string1"; prop2 = "string2"; };
where the "string2" is aligned to 4 bytes. And that is what U-Boot expects when it tries to access those data in-place in SPL.
The problem with the reverted patch was that it made U-Boot assume the alignment is 8 bytes, and that actually works only if you use fitImage with external data (mkimage -E), but with embedded data (mkimage default) not so much. That caused off-by-4 error in some cases and that made the SPL hang.
Note, as I indicated in this patch, now with libfdt 1.6.1, the alignment to 8 byte is a must-have. So we have to do such alignment anyway.
@Tom may fill in why libfdt commit commit 5e735860c478 ("libfdt: Check for 8-byte address alignment in fdt_ro_probe_()") was made to have the 8-byte alignment requirement.
Note that it's not so much since libfdt 1.6.1 but that since always the device tree has required 8 byte alignment.
DT alignment was always 4 byte , no ?
I'm pretty sure, no, 8 byte base alignment is a pretty much always thing. I don't have a reference handy but I also know I couldn't have convinced dgibson to add the check otherwise.
DTSpec rev 0.3 says the following and I think you got confused by section 5.6 which talks about alignment of the entire tree, not its nodes:
5.4 Structure Block " Each token in the structure block, and thus the structure block itself, shall be located at a 4-byte aligned offset from the beginning of the devicetree blob (see 5.6). "
5.4.2 Tree structure " For each property of the node: ... – FDT_PROP token ...
- [zeroed padding bytes to align to a 4-byte boundary]
"
5.5 Strings Block " The strings block contains strings representing all the property names used in the tree. These null terminated strings are simply concatenated together in this section, and referred to from the structure block by an offset into the strings block. The strings block has no alignment constraints and may appear at any offset from the beginning of the devicetree blob. "
5.6 Alignment " As described in the previous sections, the structure and strings blocks shall have aligned offsets from the beginning of the devicetree blob. To ensure the in-memory alignment of the blocks, it is sufficient to ensure that the devicetree as a whole is loaded at an address aligned to the largest alignment of any of the subblocks, that is, to an 8-byte boundary.
Right. A device tree must start at an 8-byte boundary and U-Boot was violating this both with: - All of the boards that use fdt_high=0xffffffff to disable relocation, and then then place things at arbitrary spots in memory that may or may not violate these requirements. - Perhaps some of the FIT internals where we have a device tree inside a device tree? And we need to fixup whatever we're doing there that's wrong.