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.do...@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. > > > > > > 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. -- Tom
signature.asc
Description: PGP signature