On Fri, Dec 06, 2024 at 12:18:07PM -0700, Simon Glass wrote:
Hi Tom,
On Fri, 6 Dec 2024 at 07:13, Tom Rini trini@konsulko.com wrote:
On Fri, Dec 06, 2024 at 06:11:12AM -0700, Simon Glass wrote:
[snip]
A comment in that function says that we assume there is space after the existing fdt to use for padding, with the padding size set to CONFIG_SYS_FDT_PAD
However, there is no guarantee that this space is available. If using the control FDT, then global_data is immediately above it, so expanding the FDT and adding FDT properties will cause U-Boot to fail.
This sounds like a generic issue and should be fixed outside of a "Pi" series.
Yes and I know you keep asking for multiple smaller series, but I've explained that keeping track of all that for months at a time is too challenging for me.
Yes, and it's also too challenging to review for the rest of us.
diff --git a/dts/Kconfig b/dts/Kconfig index 41a758e83a6..360611edd01 100644 --- a/dts/Kconfig +++ b/dts/Kconfig @@ -219,6 +219,17 @@ config OF_OMIT_DTB This is used for boards which normally provide a devicetree via a runtime mechanism (such as OF_BOARD), to avoid confusion.
+config OF_EXPAND
hex # "Amount to allow the control FDT to expand"No "# ..." because the help explains what to do here, if needed.
OK
default SYS_FDT_PAD if OF_LIBFDTdefault 0A "default 0" is almost always wrong.
In this case it is correct, since it means not to add any extra space for the FDT.
Which is still wrong? Because we don't allow for this particular device tree in memory to be read-only and so the question is "Do you want to break things on some platforms or not?".
helpSome boards make use of the control FDT to boot an OS, thus whenimage_setup_libfdt() adds extra things to the end of the FDT, thereneeds to be enough space.Set this to the number of bytes of extra space required for the FDT.We should just use SYS_FDT_PAD directly and not add a new option.
I thought I read somewhere that fdt_high == -1 should not be used. So for most boards, this padding is not needed?
That's also true, and gets back to why putting multiple patches in a single series is a Bad Idea. The function in question isn't doing some check for if we've disabled device tree relocation for the OS, it's early on, it's reserving our control fdt, yes? And then the problem ends up being later on, we use the control fdt for the OS, and modify it, but haven't ensured there's enough space for said modifications to not stomp on something else? That is a much bigger generic problem, no?