Re: [PATCH] boot: image-fdt: Restore suppression of irrelevant ERROR message

[Randolph Sapp <rs@ti.com>]

On Mon Jun 29, 2026 at 1:14 PM CDT, Jonas Karlman wrote:
> Hi Randolph,
>
> On 6/29/2026 7:33 PM, Randolph Sapp wrote:
>> Hey, glad people are testing this. 
>> 
>> On Sat Jun 27, 2026 at 3:48 PM CDT, Jonas Karlman wrote:
>>> The commit 623f6c5b6ab7 ("boot: image-fdt: free old dtb reservations")
>>> removed the suppression of ERROR messages when -EINVAL is returned due
>>> to the memory region not being part of the LMB memory map.
>>>
>>> This causes an irrelevant ERROR message during boot, e.g.:
>>>
>>>   Model: Radxa ROCK 3B
>>>   [...]
>>>   ERROR: reserving fdt memory region failed (addr=10f000 size=100 flags=2): -22
>>>
>>> or
>>>
>>>   Model: Rockchip RK3288 Asus Tinker Board S
>>>   [...]
>>>   ERROR: reserving fdt memory region failed (addr=fe000000 size=1000000 flags=4): -22
>>>
>>> FDT correctly contains reserved-memory for 10f000 or fe000000 and U-Boot
>>> correctly does not make these regions available in the LMB memory map:
>>>
>>>   memory[0]      [0x200000-0xefffffff], 0xefe00000 bytes, flags: none
>>>   memory[1]      [0x100000000-0x1ffffffff], 0x100000000 bytes, flags: none
>>>
>>> or
>>>
>>>   memory[0]      [0x0-0x7fffffff], 0x80000000 bytes, flags: none
>>>
>>> Ignore -EINVAL errors to restore suppression of this irrelevant ERROR
>>> message when memory region is not part of the LMB memory map.
>>>
>>> Fixes: 623f6c5b6ab7 ("boot: image-fdt: free old dtb reservations")
>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>> ---
>>>  boot/image-fdt.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
>>> index 9e0e0f93edd3..603294215b89 100644
>>> --- a/boot/image-fdt.c
>>> +++ b/boot/image-fdt.c
>>> @@ -95,7 +95,7 @@ static void boot_fdt_handle_region(u64 addr, u64 size, u32 flags, bool free)
>>>  		debug("   %s fdt memory region: addr=%llx size=%llx flags=%x\n",
>>>  		      free ? "freed" : "reserved", (unsigned long long)addr,
>>>  		      (unsigned long long)size, flags);
>>> -	} else {
>>> +	} else if (free || ret != -EINVAL) {
>> 
>> Seems like you messed up the free error reporting here.
>
> Not sure what you mean, it is only lmb_alloc_mem() that documents:
>
>   -EINVAL if the requested memory region is not part of the LMB memory map
>
> and seems to now help print an verbose ERROR message during normal boot.
>
> But maybe we should also ignore free errors?
>
>> 
>>>  		printf("ERROR: %s fdt memory region failed (addr=%llx size=%llx flags=%x): %ld\n",
>>>  		       free ? "freeing" : "reserving", (unsigned long long)addr,
>>>  		       (unsigned long long)size, flags, ret);
>> 
>> Got a few questions about this particular case though if you don't mind:
>> 
>> Why do you have reserved regions defined in areas that don't overlap with DDR?
>
> E.g. fe000000-ffffffff or similar region is reserved for hw regs, on
> boards with e.g. 4 GiB DRAM this area is typically unreachable, on a
> 2 GiB DRAM board there is no overlap.
>
> The 10f000 is shmem used for SCMI, so should be defined in DT.
>
>> Shouldn't those areas be unused anyway? Why don't we just explicitly ignore
>> those reservations instead of forwarding them to an allocator that doesn't care?
>
> That is what the net effect was prior to the changes in 623f6c5b6ab7
> ("boot: image-fdt: free old dtb reservations"). Rockchip platform
> dynamically declare DRAM banks excluding the unusable ranges, and the
> LMB allocator ignores ranges outside the declared DRAM banks.

Not quite. I mean, we should ignore the region explicitly. As in, we should
never even request to reserve regions outside of LMB.

>> Why mask a bunch of potentially valid error cases for this explicit case?
>
> That is what the code have been doing up until now, a different option
> could be to change the error message to a debug message? Here I just
> tried to do a limited revert to the old behavior.
>
> Why should we print verbose ERROR messages for something that is not
> always considered an error?

Because under normal circumstances it should always be an error. Occasionally
asking for a reservation you know is going to fail and assuming all failures of
that type are OK is a little odd.

>> 
>> Why do none of the examples listed above have a memory node in dts? I've seen
>> platforms that expect memory length to be defined by the bootloader, but they
>> normally still place a dummy node in the tree at the correct starting address.
>
> Rockchip platform dynamically check size of DRAM at runtime and declare
> DRAM banks for the usable areas. Possible a fixup adds a memory node as
> part of boot into OS.
>
> Regards,
> Jonas

Ah, thanks for the information.