[PATCHv6 0/3] various memory related fixups

[PATCHv6 0/3] various memory related fixups

[rs]

From: Randolph Sapp <rs@ti.com>

Nitpicks and fixes from the discovery thread on adding PocketBeagle2 support
[1]. This does a lot of general setup required for the device, but these
modifications themselves aren't device specific. For those specifically
interested in PocketBeagle2 support and don't care about these details, my
development branch is public [2].

That second patch may provoke some opinions, but honestly if that warning was
still present I wouldn't have spent a week poking holes in both the EFI and LMB
allocations systems. Please let me know if there is a specific usecase that it
breaks though.

[1] https://lore.kernel.org/all/DHHC66BBMD27.YHGIH43C6XBK@ti.com/
[2] https://github.com/StaticRocket/u-boot/tree/feature/pocketbeagle2

v2:
	- Remove additional increment and decrement in lmb_free_fdt_regions
	- Drop the patch to backfill EFI_CONVENTIONAL_MEMORY
	- Adjust the removal loop nitpick patch description
	- Change the reserve memory patch to use new end_addr_sp
v3:
	- Update lmb flags to use the macro documentation for constants
	- Change efi_mem_sort to use list_for_each_entry_safe
v4:
	- Fix typos in LMB allocation flags macro documentation
	- Rename end_addr_sp to initial_relocaddr
	- Keep the map_sysmem dance in the efi u-boot reservation
	- Use the active device tree pointed to by gd->fdt_blob to clean up old
	  reservations
v5:
	- Keep return value as long in boot_fdt_reserve_region
	- Fix formatting in initial_relocaddr patch
v6:
	- Drop patches that have been picked up already
	- Add board_get_usable_ram_top for the sandbox
	- s/boot_fdt_reserve_region/boot_fdt_handle_region/

Randolph Sapp (3):
  sandbox: add board_get_usable_ram_top
  boot_fdt_add_mem_rsv_regions: free old dtb reservations
  memory: reserve from start_addr_sp to initial_relocaddr

 arch/mips/lib/bootm.c             |  2 +-
 board/sandbox/sandbox.c           |  5 +++
 boot/bootm.c                      |  2 +-
 boot/bootm_os.c                   |  2 +-
 boot/image-board.c                |  2 +-
 boot/image-fdt.c                  | 55 +++++++++++++++++++++----------
 common/board_f.c                  |  9 ++++-
 include/asm-generic/global_data.h |  7 ++++
 include/image.h                   |  2 +-
 lib/efi_loader/efi_memory.c       |  2 +-
 lib/lmb.c                         | 41 ++++-------------------
 11 files changed, 71 insertions(+), 58 deletions(-)

-- 
2.54.0

[PATCHv6 1/3] sandbox: add board_get_usable_ram_top

[rs]

From: Randolph Sapp <rs@ti.com>

Add a board_get_usable_ram_top definition, since currently ram_top is
equal to ram_size. Attempting to actually map ram_size with map_sysmem
results in a fault.

Signed-off-by: Randolph Sapp <rs@ti.com>
---
 board/sandbox/sandbox.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/board/sandbox/sandbox.c b/board/sandbox/sandbox.c
index 13006a0ffc2..88c7636d3ca 100644
--- a/board/sandbox/sandbox.c
+++ b/board/sandbox/sandbox.c
@@ -105,6 +105,11 @@ int dram_init(void)
 	return 0;
 }
 
+phys_addr_t board_get_usable_ram_top(phys_size_t total_size)
+{
+	return gd->ram_size - 1;
+}
+
 int ft_board_setup(void *fdt, struct bd_info *bd)
 {
 	/* Create an arbitrary reservation to allow testing OF_BOARD_SETUP.*/
-- 
2.54.0

Re: [PATCHv6 1/3] sandbox: add board_get_usable_ram_top

[Simon Glass]

Hi Randolph,

On 2026-05-08T22:29:09, Randolph Sapp <rs@ti.com> wrote:
> sandbox: add board_get_usable_ram_top
>
> Add a board_get_usable_ram_top definition, since currently ram_top is
> equal to ram_size. Attempting to actually map ram_size with map_sysmem
> results in a fault.
>
> Signed-off-by: Randolph Sapp <rs@ti.com>
>
> board/sandbox/sandbox.c | 5 +++++
>  1 file changed, 5 insertions(+)

> diff --git a/board/sandbox/sandbox.c b/board/sandbox/sandbox.c
> @@ -105,6 +105,11 @@ int dram_init(void)
> +phys_addr_t board_get_usable_ram_top(phys_size_t total_size)
> +{
> +     return gd->ram_size - 1;
> +}
> +

Please describe which caller actually faults on map_sysmem(), since
that is the real motivation.

But this contradicts the documented API. From include/init.h:

   The returned address is exclusive (i.e. 1 byte above the
   last usable address).

See the weak function in common/board_f.c

If the real problem is callers doing map_sysmem(ram_top, ...) and
faulting, you could fix it in those callers (use ram_top - 1 when you
need the last valid byte). But I could take a look if you point to the
code.

Also, this uses gd->ram_size as if it were an address; it should
really be gd->ram_base + gd->ram_size. It happens to work because
sandbox's ram_base is 0, but it reads as a type confusion.

Regards,
Simon

Re: [PATCHv6 1/3] sandbox: add board_get_usable_ram_top

[Randolph Sapp]

On Mon May 11, 2026 at 1:26 PM CDT, Simon Glass wrote:
> Hi Randolph,
>
> On 2026-05-08T22:29:09, Randolph Sapp <rs@ti.com> wrote:
>> sandbox: add board_get_usable_ram_top
>>
>> Add a board_get_usable_ram_top definition, since currently ram_top is
>> equal to ram_size. Attempting to actually map ram_size with map_sysmem
>> results in a fault.
>>
>> Signed-off-by: Randolph Sapp <rs@ti.com>
>>
>> board/sandbox/sandbox.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>
>> diff --git a/board/sandbox/sandbox.c b/board/sandbox/sandbox.c
>> @@ -105,6 +105,11 @@ int dram_init(void)
>> +phys_addr_t board_get_usable_ram_top(phys_size_t total_size)
>> +{
>> +     return gd->ram_size - 1;
>> +}
>> +
>
> Please describe which caller actually faults on map_sysmem(), since
> that is the real motivation.
>
> But this contradicts the documented API. From include/init.h:
>
>    The returned address is exclusive (i.e. 1 byte above the
>    last usable address).
>
> See the weak function in common/board_f.c

Ugh. Why is *usable* ram top exclusive? That's returning an explicitly
*unusable* address. Fun. Suppose it's my fault for not reading that first.

> If the real problem is callers doing map_sysmem(ram_top, ...) and
> faulting, you could fix it in those callers (use ram_top - 1 when you
> need the last valid byte). But I could take a look if you point to the
> code.
>
> Also, this uses gd->ram_size as if it were an address; it should
> really be gd->ram_base + gd->ram_size. It happens to work because
> sandbox's ram_base is 0, but it reads as a type confusion.
>
> Regards,
> Simon

Fair enough, but it seems that this board definition will not be required given
the above feedback.

Re: [PATCHv6 1/3] sandbox: add board_get_usable_ram_top

[Simon Glass]

Hi Randolph,

On Mon, 11 May 2026 at 12:44, Randolph Sapp <rs@ti.com> wrote:
>
> On Mon May 11, 2026 at 1:26 PM CDT, Simon Glass wrote:
> > Hi Randolph,
> >
> > On 2026-05-08T22:29:09, Randolph Sapp <rs@ti.com> wrote:
> >> sandbox: add board_get_usable_ram_top
> >>
> >> Add a board_get_usable_ram_top definition, since currently ram_top is
> >> equal to ram_size. Attempting to actually map ram_size with map_sysmem
> >> results in a fault.
> >>
> >> Signed-off-by: Randolph Sapp <rs@ti.com>
> >>
> >> board/sandbox/sandbox.c | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >
> >> diff --git a/board/sandbox/sandbox.c b/board/sandbox/sandbox.c
> >> @@ -105,6 +105,11 @@ int dram_init(void)
> >> +phys_addr_t board_get_usable_ram_top(phys_size_t total_size)
> >> +{
> >> +     return gd->ram_size - 1;
> >> +}
> >> +
> >
> > Please describe which caller actually faults on map_sysmem(), since
> > that is the real motivation.
> >
> > But this contradicts the documented API. From include/init.h:
> >
> >    The returned address is exclusive (i.e. 1 byte above the
> >    last usable address).
> >
> > See the weak function in common/board_f.c
>
> Ugh. Why is *usable* ram top exclusive? That's returning an explicitly
> *unusable* address. Fun. Suppose it's my fault for not reading that first.

It's just the way it is defined. It avoids lots of ffff's in
addresses. I know that ACPI tends to use the last valid address, but
that's not typically how U-Boot works.

>
> > If the real problem is callers doing map_sysmem(ram_top, ...) and
> > faulting, you could fix it in those callers (use ram_top - 1 when you
> > need the last valid byte). But I could take a look if you point to the
> > code.
> >
> > Also, this uses gd->ram_size as if it were an address; it should
> > really be gd->ram_base + gd->ram_size. It happens to work because
> > sandbox's ram_base is 0, but it reads as a type confusion.
> >
> > Regards,
> > Simon
>
> Fair enough, but it seems that this board definition will not be required given
> the above feedback.

Ah OK...but I'm still interested in knowing which caller caused this problem.

Regards,
Simon

[PATCHv6 2/3] boot_fdt_add_mem_rsv_regions: free old dtb reservations

[rs]

From: Randolph Sapp <rs@ti.com>

Add a free flag and an initial call to free allocations covered by the
global FDT. This assumes that all calls to boot_fdt_add_mem_rsv_regions
occur before the transition to the new device tree, thus we can access
the currently active device tree through the global data pointer.

This allows us to clearly indicate to the user when a device tree
reservation fails. How we handle this can still use some improvement.
Right now we'll keep the default behavior and try to boot anyway.

This functionality was broken in:
5a6aa7d ("boot: fdt: Handle already reserved memory in boot_fdt_reserve_region()")

Signed-off-by: Randolph Sapp <rs@ti.com>
Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 arch/mips/lib/bootm.c |  2 +-
 boot/bootm.c          |  2 +-
 boot/bootm_os.c       |  2 +-
 boot/image-board.c    |  2 +-
 boot/image-fdt.c      | 55 ++++++++++++++++++++++++++++++-------------
 include/image.h       |  2 +-
 lib/lmb.c             |  2 +-
 7 files changed, 44 insertions(+), 23 deletions(-)

diff --git a/arch/mips/lib/bootm.c b/arch/mips/lib/bootm.c
index 066c830f3fa..546e888630e 100644
--- a/arch/mips/lib/bootm.c
+++ b/arch/mips/lib/bootm.c
@@ -210,7 +210,7 @@ static int boot_reloc_fdt(struct bootm_headers *images)
 	}
 
 #if CONFIG_IS_ENABLED(MIPS_BOOT_FDT) && CONFIG_IS_ENABLED(OF_LIBFDT)
-	boot_fdt_add_mem_rsv_regions(images->ft_addr);
+	boot_fdt_add_mem_rsv_regions(images->ft_addr, false);
 	return boot_relocate_fdt(&images->ft_addr, &images->ft_len);
 #else
 	return 0;
diff --git a/boot/bootm.c b/boot/bootm.c
index 4836d6b2d41..394a256e1ab 100644
--- a/boot/bootm.c
+++ b/boot/bootm.c
@@ -1040,7 +1040,7 @@ int bootm_run_states(struct bootm_info *bmi, int states)
 #endif
 #if CONFIG_IS_ENABLED(OF_LIBFDT) && CONFIG_IS_ENABLED(LMB)
 	if (!ret && (states & BOOTM_STATE_FDT)) {
-		boot_fdt_add_mem_rsv_regions(images->ft_addr);
+		boot_fdt_add_mem_rsv_regions(images->ft_addr, false);
 		ret = boot_relocate_fdt(&images->ft_addr, &images->ft_len);
 	}
 #endif
diff --git a/boot/bootm_os.c b/boot/bootm_os.c
index ae20b555f5c..48f68941ff8 100644
--- a/boot/bootm_os.c
+++ b/boot/bootm_os.c
@@ -262,7 +262,7 @@ static void do_bootvx_fdt(struct bootm_headers *images)
 	char **of_flat_tree = &images->ft_addr;
 
 	if (*of_flat_tree) {
-		boot_fdt_add_mem_rsv_regions(*of_flat_tree);
+		boot_fdt_add_mem_rsv_regions(*of_flat_tree, false);
 
 		ret = boot_relocate_fdt(of_flat_tree, &of_size);
 		if (ret)
diff --git a/boot/image-board.c b/boot/image-board.c
index 005d60caf5c..55aaa741826 100644
--- a/boot/image-board.c
+++ b/boot/image-board.c
@@ -914,7 +914,7 @@ int image_setup_linux(struct bootm_headers *images)
 	if (!CONFIG_IS_ENABLED(LMB))
 		return -EFAULT;
 	if (CONFIG_IS_ENABLED(OF_LIBFDT))
-		boot_fdt_add_mem_rsv_regions(*of_flat_tree);
+		boot_fdt_add_mem_rsv_regions(*of_flat_tree, false);
 
 	if (IS_ENABLED(CONFIG_SYS_BOOT_GET_CMDLINE)) {
 		ret = boot_get_cmdline(&images->cmdline_start,
diff --git a/boot/image-fdt.c b/boot/image-fdt.c
index a3a4fb8b558..c7def755098 100644
--- a/boot/image-fdt.c
+++ b/boot/image-fdt.c
@@ -69,35 +69,51 @@ static const struct legacy_img_hdr *image_get_fdt(ulong fdt_addr)
 }
 #endif
 
-static void boot_fdt_reserve_region(u64 addr, u64 size, u32 flags)
+/**
+ * boot_fdt_handle_region - Reserve or free a given FDT region in LMB
+ * @addr: Reservation base address
+ * @size: Reservation size
+ * @flags: Reservation flags
+ * @free: Indicate if region is being freed or allocated
+ *
+ * Add or free a given reservation from LMB. This reports to the user if any
+ * errors occurred during either operation.
+ */
+static void boot_fdt_handle_region(u64 addr, u64 size, u32 flags, bool free)
 {
 	long ret;
 	phys_addr_t rsv_addr;
 
 	rsv_addr = (phys_addr_t)addr;
-	ret = lmb_alloc_mem(LMB_MEM_ALLOC_ADDR, 0, &rsv_addr, size, flags);
+	if (free)
+		ret = lmb_free(rsv_addr, size, flags);
+	else
+		ret = lmb_alloc_mem(LMB_MEM_ALLOC_ADDR, 0, &rsv_addr, size,
+				    flags);
+
 	if (!ret) {
-		debug("   reserving fdt memory region: addr=%llx size=%llx flags=%x\n",
-		      (unsigned long long)addr,
+		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 if (ret != -EEXIST && ret != -EINVAL) {
-		puts("ERROR: reserving fdt memory region failed ");
-		printf("(addr=%llx size=%llx flags=%x)\n",
-		       (unsigned long long)addr,
-		       (unsigned long long)size, flags);
+	} else {
+		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);
 	}
 }
 
 /**
- * boot_fdt_add_mem_rsv_regions - Mark the memreserve and reserved-memory
- * sections as unusable
+ * boot_fdt_add_mem_rsv_regions - Handle FDT memreserve and reserved-memory
+ * sections
  * @fdt_blob: pointer to fdt blob base address
+ * @free: indicate if regions are being freed
  *
- * Adds the and reserved-memorymemreserve regions in the dtb to the lmb block.
- * Adding the memreserve regions prevents u-boot from using them to store the
- * initrd or the fdt blob.
+ * Adds or removes reserved-memory and memreserve regions in the dtb to the lmb
+ * block. Adding the memreserve regions prevents u-boot from using them to store
+ * the initrd or the fdt blob. This function will attempt to clean the currently
+ * active reservations if a new device tree blob is given.
  */
-void boot_fdt_add_mem_rsv_regions(void *fdt_blob)
+void boot_fdt_add_mem_rsv_regions(void *fdt_blob, bool free)
 {
 	uint64_t addr, size;
 	int i, total, ret;
@@ -108,12 +124,16 @@ void boot_fdt_add_mem_rsv_regions(void *fdt_blob)
 	if (fdt_check_header(fdt_blob) != 0)
 		return;
 
+	/* Remove old regions */
+	if (gd->fdt_blob != fdt_blob)
+		boot_fdt_add_mem_rsv_regions((void *)gd->fdt_blob, true);
+
 	/* process memreserve sections */
 	total = fdt_num_mem_rsv(fdt_blob);
 	for (i = 0; i < total; i++) {
 		if (fdt_get_mem_rsv(fdt_blob, i, &addr, &size) != 0)
 			continue;
-		boot_fdt_reserve_region(addr, size, LMB_NOOVERWRITE);
+		boot_fdt_handle_region(addr, size, LMB_NOOVERWRITE, free);
 	}
 
 	/* process reserved-memory */
@@ -131,7 +151,8 @@ void boot_fdt_add_mem_rsv_regions(void *fdt_blob)
 					flags = LMB_NOMAP;
 				addr = res.start;
 				size = res.end - res.start + 1;
-				boot_fdt_reserve_region(addr, size, flags);
+				boot_fdt_handle_region(addr, size, flags,
+							free);
 			}
 
 			subnode = fdt_next_subnode(fdt_blob, subnode);
diff --git a/include/image.h b/include/image.h
index 34efac6056d..68dfa4716ab 100644
--- a/include/image.h
+++ b/include/image.h
@@ -827,7 +827,7 @@ int boot_get_fdt(void *buf, const char *select, uint arch,
 		 struct bootm_headers *images, char **of_flat_tree,
 		 ulong *of_size);
 
-void boot_fdt_add_mem_rsv_regions(void *fdt_blob);
+void boot_fdt_add_mem_rsv_regions(void *fdt_blob, bool free);
 int boot_relocate_fdt(char **of_flat_tree, ulong *of_size);
 
 int boot_ramdisk_high(ulong rd_data, ulong rd_len, ulong *initrd_start,
diff --git a/lib/lmb.c b/lib/lmb.c
index 8f12c6ad8e5..9a8c70b778a 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -581,7 +581,7 @@ static void lmb_reserve_common(void *fdt_blob)
 	lmb_reserve_uboot_region();
 
 	if (CONFIG_IS_ENABLED(OF_LIBFDT) && fdt_blob)
-		boot_fdt_add_mem_rsv_regions(fdt_blob);
+		boot_fdt_add_mem_rsv_regions(fdt_blob, false);
 }
 
 static __maybe_unused void lmb_reserve_common_spl(void)
-- 
2.54.0

Re: [PATCHv6 2/3] boot_fdt_add_mem_rsv_regions: free old dtb reservations

[Simon Glass]

Hi Randolph,

On 2026-05-08T22:29:09, Randolph Sapp <rs@ti.com> wrote:
> boot_fdt_add_mem_rsv_regions: free old dtb reservations
>
> Add a free flag and an initial call to free allocations covered by the
> global FDT. This assumes that all calls to boot_fdt_add_mem_rsv_regions
> occur before the transition to the new device tree, thus we can access
> the currently active device tree through the global data pointer.
>
> This allows us to clearly indicate to the user when a device tree
> reservation fails. How we handle this can still use some improvement.
> Right now we'll keep the default behavior and try to boot anyway.
>
> This functionality was broken in:
> 5a6aa7d ("boot: fdt: Handle already reserved memory in boot_fdt_reserve_region()")
>
> Signed-off-by: Randolph Sapp <rs@ti.com>
> Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>
> arch/mips/lib/bootm.c |  2 +-
>  boot/bootm.c          |  2 +-
>  boot/bootm_os.c       |  2 +-
>  boot/image-board.c    |  2 +-
>  boot/image-fdt.c      | 55 +++++++++++++++++++++++++++++++++++----------------
>  include/image.h       |  2 +-
>  lib/lmb.c             |  2 +-
>  7 files changed, 44 insertions(+), 23 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

with some thoughts below

> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> @@ -108,12 +124,16 @@ void boot_fdt_add_mem_rsv_regions(void *fdt_blob)
> +     /* Remove old regions */
> +     if (gd->fdt_blob != fdt_blob)
> +             boot_fdt_add_mem_rsv_regions((void *)gd->fdt_blob, true);
> +

Recursing through the public entry point with a flipped bool is hard
to follow, and it relies on the implicit invariant that the recursive
call will not itself recurse. I'd find this clearer as two named
helpers. For example you could have boot_fdt_reserve_regions() and
boot_fdt_free_regions() sharing a static walker. Then the intent at
the call site would be obvious.

Please also add a comment explaining the assumption from the commit
message: this must run before gd->fdt_blob is swapped to the new tree
(since nothing in the code itself enforces it)

> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> @@ -69,35 +69,51 @@ static const struct legacy_img_hdr *image_get_fdt(ulong fdt_addr)
> -     } else if (ret != -EEXIST && ret != -EINVAL) {
> -             puts("ERROR: reserving fdt memory region failed ");
> -             printf("(addr=%llx size=%llx flags=%x)\n",
> -                    (unsigned long long)addr,
> -                    (unsigned long long)size, flags);
> +     } else {
> +             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);
>       }

Dropping the -EEXIST/-EINVAL filter only works on the reserve path
once the free path always succeeds. On the free side, anything in the
old fdt's reserved-memory that wasn't actually picked up by
lmb_reserve_common() (a non-enabled subnode, or a region since
split/coalesced inside LMB) will now produce a spurious "ERROR:
freeing fdt memory region failed" line.

I'm not sure if this actually happens? You could try it on a board
whose memreserve/reserved-memory layout differs between the U-Boot dtb
and the kernel dtb? Perhaps for now, best to tolerate  -EINVAL on the
free path at least.

> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> @@ -108,12 +124,16 @@ void boot_fdt_add_mem_rsv_regions(void *fdt_blob)
> +     /* Remove old regions */
> +     if (gd->fdt_blob != fdt_blob)
> +             boot_fdt_add_mem_rsv_regions((void *)gd->fdt_blob, true);

Just to check - the cast drops the const from gd->fdt_blob and we then
walk it read-only, which is fine, but boot_fdt_add_mem_rsv_regions()
takes a non-const void * for no good reason that I can see? Worth a
brief comment so a future reader doesn't try to 'fix' the cast.

Regards,
Simon

Re: [PATCHv6 2/3] boot_fdt_add_mem_rsv_regions: free old dtb reservations

[Randolph Sapp]

On Mon May 11, 2026 at 1:27 PM CDT, Simon Glass wrote:
> Hi Randolph,
>
> On 2026-05-08T22:29:09, Randolph Sapp <rs@ti.com> wrote:
>> boot_fdt_add_mem_rsv_regions: free old dtb reservations
>>
>> Add a free flag and an initial call to free allocations covered by the
>> global FDT. This assumes that all calls to boot_fdt_add_mem_rsv_regions
>> occur before the transition to the new device tree, thus we can access
>> the currently active device tree through the global data pointer.
>>
>> This allows us to clearly indicate to the user when a device tree
>> reservation fails. How we handle this can still use some improvement.
>> Right now we'll keep the default behavior and try to boot anyway.
>>
>> This functionality was broken in:
>> 5a6aa7d ("boot: fdt: Handle already reserved memory in boot_fdt_reserve_region()")
>>
>> Signed-off-by: Randolph Sapp <rs@ti.com>
>> Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>
>> arch/mips/lib/bootm.c |  2 +-
>>  boot/bootm.c          |  2 +-
>>  boot/bootm_os.c       |  2 +-
>>  boot/image-board.c    |  2 +-
>>  boot/image-fdt.c      | 55 +++++++++++++++++++++++++++++++++++----------------
>>  include/image.h       |  2 +-
>>  lib/lmb.c             |  2 +-
>>  7 files changed, 44 insertions(+), 23 deletions(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> with some thoughts below
>
>> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
>> @@ -108,12 +124,16 @@ void boot_fdt_add_mem_rsv_regions(void *fdt_blob)
>> +     /* Remove old regions */
>> +     if (gd->fdt_blob != fdt_blob)
>> +             boot_fdt_add_mem_rsv_regions((void *)gd->fdt_blob, true);
>> +
>
> Recursing through the public entry point with a flipped bool is hard
> to follow, and it relies on the implicit invariant that the recursive
> call will not itself recurse. I'd find this clearer as two named
> helpers. For example you could have boot_fdt_reserve_regions() and
> boot_fdt_free_regions() sharing a static walker. Then the intent at
> the call site would be obvious.
>
> Please also add a comment explaining the assumption from the commit
> message: this must run before gd->fdt_blob is swapped to the new tree
> (since nothing in the code itself enforces it)

Fair point. Moved the walking logic to a static boot_fdt_handle_mem_rsv_regions
and made boot_fdt_add_mem_rsv_regions call into it.

>> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
>> @@ -69,35 +69,51 @@ static const struct legacy_img_hdr *image_get_fdt(ulong fdt_addr)
>> -     } else if (ret != -EEXIST && ret != -EINVAL) {
>> -             puts("ERROR: reserving fdt memory region failed ");
>> -             printf("(addr=%llx size=%llx flags=%x)\n",
>> -                    (unsigned long long)addr,
>> -                    (unsigned long long)size, flags);
>> +     } else {
>> +             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);
>>       }
>
> Dropping the -EEXIST/-EINVAL filter only works on the reserve path
> once the free path always succeeds. On the free side, anything in the
> old fdt's reserved-memory that wasn't actually picked up by
> lmb_reserve_common() (a non-enabled subnode, or a region since
> split/coalesced inside LMB) will now produce a spurious "ERROR:
> freeing fdt memory region failed" line.

Nah, coalesced regions can be freed without issue. The region is simply split if
needed in _lmb_free.

If the free or allocation doesn't succeed for any reason the user must be *at
least* be informed. This hidden warning stuff was the start of all of this. I
don't like keeping the users in the dark.

> I'm not sure if this actually happens? You could try it on a board
> whose memreserve/reserved-memory layout differs between the U-Boot dtb
> and the kernel dtb? Perhaps for now, best to tolerate  -EINVAL on the
> free path at least.

To humor this, the pocketbeagle2 actually has 2 regions back to back.
memory@9da00000 and memory@9db00000. Allocation and freeing of these regions
works without fault. Removing memory@9db00000 from the kernel dtb, but keeping
it in the u-boot dtb results in no warnings. Removing memory@9db00000 in u-boot
and keeping it in the kernel fdt also resulted in no warnings.

The only thing that would produce logs would be:
1. If someone actually switched gd->fdt_blob before calling this function
2. If someone called this function without switching gd->fdt_blob afterward

Both of these *should* produce warnings, considering they would permanently and
incorrectly change the LMB regions. That would have been unusual in the old
path, we're just making it explicitly bad now.

>> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
>> @@ -108,12 +124,16 @@ void boot_fdt_add_mem_rsv_regions(void *fdt_blob)
>> +     /* Remove old regions */
>> +     if (gd->fdt_blob != fdt_blob)
>> +             boot_fdt_add_mem_rsv_regions((void *)gd->fdt_blob, true);
>
> Just to check - the cast drops the const from gd->fdt_blob and we then
> walk it read-only, which is fine, but boot_fdt_add_mem_rsv_regions()
> takes a non-const void * for no good reason that I can see? Worth a
> brief comment so a future reader doesn't try to 'fix' the cast.
>
> Regards,
> Simon

Fair point, I just switched boot_fdt_add_mem_rsv_regions to use const.

[PATCHv6 3/3] memory: reserve from start_addr_sp to initial_relocaddr

[rs]

From: Randolph Sapp <rs@ti.com>

Add a new global data struct member called initial_relocaddr. This
stores the original value of relocaddr, directly from setup_dest_addr.
This is specifically to avoid any adjustments made by other init
functions.

Reserve the memory from gd->start_addr_sp - CONFIG_STACK_SIZE to
gd->initial_relocaddr instead of gd->ram_top. This allows platform
specific relocation addresses to work without unnecessarily painting
over a large range.

Since PRAM comes out of this initial area up to initial_relocaddr, we no
longer need to account for it separately.

Signed-off-by: Randolph Sapp <rs@ti.com>
---
 common/board_f.c                  |  9 ++++++-
 include/asm-generic/global_data.h |  7 ++++++
 lib/efi_loader/efi_memory.c       |  2 +-
 lib/lmb.c                         | 39 +++++--------------------------
 4 files changed, 22 insertions(+), 35 deletions(-)

diff --git a/common/board_f.c b/common/board_f.c
index ce87c619e68..31c6eb725fb 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -330,6 +330,8 @@ __weak int arch_setup_dest_addr(void)
 
 static int setup_dest_addr(void)
 {
+	int ret;
+
 	debug("Monitor len: %08x\n", gd->mon_len);
 	/*
 	 * Ram is setup, size stored in gd !!
@@ -356,7 +358,12 @@ static int setup_dest_addr(void)
 	gd->relocaddr = gd->ram_top;
 	debug("Ram top: %08llX\n", (unsigned long long)gd->ram_top);
 
-	return arch_setup_dest_addr();
+	ret = arch_setup_dest_addr();
+	if (ret != 0)
+		return ret;
+
+	gd->initial_relocaddr = gd->relocaddr;
+	return ret;
 }
 
 #ifdef CFG_PRAM
diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
index 745d2c3a966..6c60a79c2ab 100644
--- a/include/asm-generic/global_data.h
+++ b/include/asm-generic/global_data.h
@@ -107,6 +107,13 @@ struct global_data {
 	 * GDB using the 'add-symbol-file u-boot <relocaddr>' command.
 	 */
 	unsigned long relocaddr;
+	/**
+	 * @initial_relocaddr: end of memory currently in use by uboot
+	 *
+	 * This should be the original value of relocaddr before any other
+	 * allocations or reservations shift it.
+	 */
+	unsigned long initial_relocaddr;
 	/**
 	 * @irq_sp: IRQ stack pointer
 	 */
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 046a2bb4641..ece05c142f0 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -869,7 +869,7 @@ static void add_u_boot_and_runtime(void)
 	/* Add U-Boot */
 	uboot_start = ((uintptr_t)map_sysmem(gd->start_addr_sp, 0) -
 		       uboot_stack_size) & ~EFI_PAGE_MASK;
-	uboot_pages = ((uintptr_t)map_sysmem(gd->ram_top - 1, 0) -
+	uboot_pages = ((uintptr_t)map_sysmem(gd->initial_relocaddr, 0) -
 		       uboot_start + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
 	efi_update_memory_map(uboot_start, uboot_pages, EFI_BOOT_SERVICES_CODE,
 			      false, false);
diff --git a/lib/lmb.c b/lib/lmb.c
index 9a8c70b778a..e5d4d726ab1 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -534,46 +534,19 @@ static long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags)
 
 static void lmb_reserve_uboot_region(void)
 {
-	int bank;
-	ulong end, bank_end;
+	ulong size;
 	phys_addr_t rsv_start;
-	ulong pram = 0;
 
 	rsv_start = gd->start_addr_sp - CONFIG_STACK_SIZE;
-	end = gd->ram_top;
+	size = gd->initial_relocaddr - rsv_start;
 
-	/*
-	 * Reserve memory from aligned address below the bottom of U-Boot stack
-	 * until end of RAM area to prevent LMB from overwriting that memory.
-	 */
-	debug("## Current stack ends at 0x%08lx ", (ulong)rsv_start);
+	debug("## Current stack ends at 0x%08lx\n", (ulong)rsv_start);
 
-#ifdef CFG_PRAM
-	pram = env_get_ulong("pram", 10, CFG_PRAM);
-	pram = pram << 10; /* size is in kB */
-#endif
-
-	for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
-		if (!gd->bd->bi_dram[bank].size ||
-		    rsv_start < gd->bd->bi_dram[bank].start)
-			continue;
-		/* Watch out for RAM at end of address space! */
-		bank_end = gd->bd->bi_dram[bank].start +
-			gd->bd->bi_dram[bank].size - 1;
-		if (rsv_start > bank_end)
-			continue;
-		if (bank_end > end)
-			bank_end = end - 1;
+	lmb_reserve(rsv_start, size, LMB_NOOVERWRITE);
 
-		lmb_reserve(rsv_start, bank_end - rsv_start - pram + 1,
+	if (gd->flags & GD_FLG_SKIP_RELOC)
+		lmb_reserve((phys_addr_t)(uintptr_t)_start, gd->mon_len,
 			    LMB_NOOVERWRITE);
-
-		if (gd->flags & GD_FLG_SKIP_RELOC)
-			lmb_reserve((phys_addr_t)(uintptr_t)_start,
-				    gd->mon_len, LMB_NOOVERWRITE);
-
-		break;
-	}
 }
 
 static void lmb_reserve_common(void *fdt_blob)
-- 
2.54.0

Re: [PATCHv6 3/3] memory: reserve from start_addr_sp to initial_relocaddr

[Simon Glass]

Hi Randolph,

On 2026-05-08T22:29:09, Randolph Sapp <rs@ti.com> wrote:
> memory: reserve from start_addr_sp to initial_relocaddr
>
> Add a new global data struct member called initial_relocaddr. This
> stores the original value of relocaddr, directly from setup_dest_addr.
> This is specifically to avoid any adjustments made by other init
> functions.
>
> Reserve the memory from gd->start_addr_sp - CONFIG_STACK_SIZE to
> gd->initial_relocaddr instead of gd->ram_top. This allows platform
> specific relocation addresses to work without unnecessarily painting
> over a large range.
>
> Since PRAM comes out of this initial area up to initial_relocaddr, we no
> longer need to account for it separately.
>
> Signed-off-by: Randolph Sapp <rs@ti.com>
>
> common/board_f.c                  |  9 ++++++++-
>  include/asm-generic/global_data.h |  7 +++++++
>  lib/efi_loader/efi_memory.c       |  2 +-
>  lib/lmb.c                         | 39 ++++++---------------------------------
>  4 files changed, 22 insertions(+), 35 deletions(-)

BTW I am missing a change long in this patch, so a bit of guesswork here.

> diff --git a/common/board_f.c b/common/board_f.c
> @@ -330,6 +330,8 @@ __weak int arch_setup_dest_addr(void)
>
>  static int setup_dest_addr(void)
>  {
> +     int ret;
> +
>       debug("Monitor len: %08x\n", gd->mon_len);
>       /*
>        * Ram is setup, size stored in gd !!
> @@ -356,7 +358,12 @@ static int setup_dest_addr(void)
>       gd->relocaddr = gd->ram_top;
>       debug("Ram top: %08llX\n", (unsigned long long)gd->ram_top);
>
> -     return arch_setup_dest_addr();
> +     ret = arch_setup_dest_addr();
> +     if (ret != 0)
> +             return ret;
> +
> +     gd->initial_relocaddr = gd->relocaddr;
> +     return ret;
>  }

Please use 'if (ret)' to match U-Boot style. The trailing 'return
ret;' is misleading since ret is known to be zero - make it 'return
0;'.

> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
> @@ -107,6 +107,13 @@ struct global_data {
>        * GDB using the 'add-symbol-file u-boot <relocaddr>' command.
>        */
>       unsigned long relocaddr;
> +     /**
> +      * @initial_relocaddr: end of memory currently in use by uboot
> +      *
> +      * This should be the original value of relocaddr before any other
> +      * allocations or reservations shift it.
> +      */
> +     unsigned long initial_relocaddr;

The kerneldoc summary is misleading: 'currently in use' suggests live
state, but this is written once at the end of setup_dest_addr() and
never updated. Please reword to something like 'initial top of the
U-Boot reservation' and note that it is set once in setup_dest_addr().
Also 'uboot' should be 'U-Boot'.

> diff --git a/lib/lmb.c b/lib/lmb.c
> @@ -534,46 +534,19 @@ static long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags)
>
>  static void lmb_reserve_uboot_region(void)
>  {
> -     int bank;
> -     ulong end, bank_end;
> +     ulong size;
>       phys_addr_t rsv_start;
> -     ulong pram = 0;
>
>       rsv_start = gd->start_addr_sp - CONFIG_STACK_SIZE;
> -     end = gd->ram_top;
> +     size = gd->initial_relocaddr - rsv_start;
>
> -     /*
> -      * Reserve memory from aligned address below the bottom of U-Boot stack
> -      * until end of RAM area to prevent LMB from overwriting that memory.
> -      */
> -     debug("## Current stack ends at 0x%08lx ", (ulong)rsv_start);
> +     debug("## Current stack ends at 0x%08lx\n", (ulong)rsv_start);

Please keep a trimmed version of the explanatory comment - it
documents the intent of this reservation and is more useful than the
surrounding context.

> diff --git a/lib/lmb.c b/lib/lmb.c
> @@ -534,46 +534,19 @@ static long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags)
> -     for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
> -             if (!gd->bd->bi_dram[bank].size ||
> -                 rsv_start < gd->bd->bi_dram[bank].start)
> -                     continue;
> -             /* Watch out for RAM at end of address space! */
> -             bank_end = gd->bd->bi_dram[bank].start +
> -                     gd->bd->bi_dram[bank].size - 1;
> -             if (rsv_start > bank_end)
> -                     continue;
> -             if (bank_end > end)
> -                     bank_end = end - 1;
> +     lmb_reserve(rsv_start, size, LMB_NOOVERWRITE);

The old code walked CONFIG_NR_DRAM_BANKS and clamped the reservation
to the bank containing rsv_start. The new code drops that and just
reserves [rsv_start, initial_relocaddr). On platforms with
non-contiguous banks (where U-Boot occupies the top bank and there is
a hole below it), this can mark a range that is not all RAM. Is that
intentional? If so the commit message should call it out; if not,
please re-add a sanity check so we do not reserve across holes.

> diff --git a/lib/lmb.c b/lib/lmb.c
> @@ -534,46 +534,19 @@ static long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags)
> -#ifdef CFG_PRAM
> -     pram = env_get_ulong('pram', 10, CFG_PRAM);
> -     pram = pram << 10; /* size is in kB */
> -#endif

PRAM previously sat above the LMB reservation (the old code subtracted
pram from the size so PRAM was left out of used_mem). With this patch,
PRAM ends up inside the [rsv_start, initial_relocaddr) range and is
now marked LMB_NOOVERWRITE - likely this is more correct, but it is a
behaviour change for boards setting pram at runtime to be larger than
CFG_PRAM - those bytes used to be available to LMB clients and now are
not.

Please spell this out in the commit message rather than the brief
'PRAM comes out of this initial area' line, so reviewers on PRAM-using
boards know to look.

> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> @@ -869,7 +869,7 @@ static void add_u_boot_and_runtime(void)
>       /* Add U-Boot */
>       uboot_start = ((uintptr_t)map_sysmem(gd->start_addr_sp, 0) -
>                      uboot_stack_size) & ~EFI_PAGE_MASK;
> -     uboot_pages = ((uintptr_t)map_sysmem(gd->ram_top - 1, 0) -
> +     uboot_pages = ((uintptr_t)map_sysmem(gd->initial_relocaddr, 0) -
>                      uboot_start + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;

Hmmm, the current expression uses ram_top - 1 (inclusive top byte) and
then rounded up via EFI_PAGE_MASK. The new one passes
initial_relocaddr without the - 1 - so when initial_relocaddr is
page-aligned this adds an extra page. What is the intention here?

Regards,
Simon

Re: [PATCHv6 3/3] memory: reserve from start_addr_sp to initial_relocaddr

[Randolph Sapp]

On Mon May 11, 2026 at 1:28 PM CDT, Simon Glass wrote:
> Hi Randolph,
>
> On 2026-05-08T22:29:09, Randolph Sapp <rs@ti.com> wrote:
>> memory: reserve from start_addr_sp to initial_relocaddr
>>
>> Add a new global data struct member called initial_relocaddr. This
>> stores the original value of relocaddr, directly from setup_dest_addr.
>> This is specifically to avoid any adjustments made by other init
>> functions.
>>
>> Reserve the memory from gd->start_addr_sp - CONFIG_STACK_SIZE to
>> gd->initial_relocaddr instead of gd->ram_top. This allows platform
>> specific relocation addresses to work without unnecessarily painting
>> over a large range.
>>
>> Since PRAM comes out of this initial area up to initial_relocaddr, we no
>> longer need to account for it separately.
>>
>> Signed-off-by: Randolph Sapp <rs@ti.com>
>>
>> common/board_f.c                  |  9 ++++++++-
>>  include/asm-generic/global_data.h |  7 +++++++
>>  lib/efi_loader/efi_memory.c       |  2 +-
>>  lib/lmb.c                         | 39 ++++++---------------------------------
>>  4 files changed, 22 insertions(+), 35 deletions(-)
>
> BTW I am missing a change long in this patch, so a bit of guesswork here.

I tend to keep the changelogs for a series in the series cover letter. I assume
tracking per patch is the preference for this list based on this message.

>> diff --git a/common/board_f.c b/common/board_f.c
>> @@ -330,6 +330,8 @@ __weak int arch_setup_dest_addr(void)
>>
>>  static int setup_dest_addr(void)
>>  {
>> +     int ret;
>> +
>>       debug("Monitor len: %08x\n", gd->mon_len);
>>       /*
>>        * Ram is setup, size stored in gd !!
>> @@ -356,7 +358,12 @@ static int setup_dest_addr(void)
>>       gd->relocaddr = gd->ram_top;
>>       debug("Ram top: %08llX\n", (unsigned long long)gd->ram_top);
>>
>> -     return arch_setup_dest_addr();
>> +     ret = arch_setup_dest_addr();
>> +     if (ret != 0)
>> +             return ret;
>> +
>> +     gd->initial_relocaddr = gd->relocaddr;
>> +     return ret;
>>  }
>
> Please use 'if (ret)' to match U-Boot style. The trailing 'return
> ret;' is misleading since ret is known to be zero - make it 'return
> 0;'.
>
>> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
>> @@ -107,6 +107,13 @@ struct global_data {
>>        * GDB using the 'add-symbol-file u-boot <relocaddr>' command.
>>        */
>>       unsigned long relocaddr;
>> +     /**
>> +      * @initial_relocaddr: end of memory currently in use by uboot
>> +      *
>> +      * This should be the original value of relocaddr before any other
>> +      * allocations or reservations shift it.
>> +      */
>> +     unsigned long initial_relocaddr;
>
> The kerneldoc summary is misleading: 'currently in use' suggests live
> state, but this is written once at the end of setup_dest_addr() and
> never updated. Please reword to something like 'initial top of the
> U-Boot reservation' and note that it is set once in setup_dest_addr().
> Also 'uboot' should be 'U-Boot'.
>
>> diff --git a/lib/lmb.c b/lib/lmb.c
>> @@ -534,46 +534,19 @@ static long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags)
>>
>>  static void lmb_reserve_uboot_region(void)
>>  {
>> -     int bank;
>> -     ulong end, bank_end;
>> +     ulong size;
>>       phys_addr_t rsv_start;
>> -     ulong pram = 0;
>>
>>       rsv_start = gd->start_addr_sp - CONFIG_STACK_SIZE;
>> -     end = gd->ram_top;
>> +     size = gd->initial_relocaddr - rsv_start;
>>
>> -     /*
>> -      * Reserve memory from aligned address below the bottom of U-Boot stack
>> -      * until end of RAM area to prevent LMB from overwriting that memory.
>> -      */
>> -     debug("## Current stack ends at 0x%08lx ", (ulong)rsv_start);
>> +     debug("## Current stack ends at 0x%08lx\n", (ulong)rsv_start);
>
> Please keep a trimmed version of the explanatory comment - it
> documents the intent of this reservation and is more useful than the
> surrounding context.
>
>> diff --git a/lib/lmb.c b/lib/lmb.c
>> @@ -534,46 +534,19 @@ static long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags)
>> -     for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
>> -             if (!gd->bd->bi_dram[bank].size ||
>> -                 rsv_start < gd->bd->bi_dram[bank].start)
>> -                     continue;
>> -             /* Watch out for RAM at end of address space! */
>> -             bank_end = gd->bd->bi_dram[bank].start +
>> -                     gd->bd->bi_dram[bank].size - 1;
>> -             if (rsv_start > bank_end)
>> -                     continue;
>> -             if (bank_end > end)
>> -                     bank_end = end - 1;
>> +     lmb_reserve(rsv_start, size, LMB_NOOVERWRITE);
>
> The old code walked CONFIG_NR_DRAM_BANKS and clamped the reservation
> to the bank containing rsv_start. The new code drops that and just
> reserves [rsv_start, initial_relocaddr). On platforms with
> non-contiguous banks (where U-Boot occupies the top bank and there is
> a hole below it), this can mark a range that is not all RAM. Is that
> intentional? If so the commit message should call it out; if not,
> please re-add a sanity check so we do not reserve across holes.

I had a few thoughts about this in the past week. Initially this bank walking
behavior seemed more than a little unusual to me. It appears here and in 2 other
places. (FDT and kernel image loading, if I recall correctly.)

I was mulling over adding it back in as-is, but this really feels like something
LMB should be aware of and should handle on it's own during the reservation of
those regions. Something akin to the EFI allocator's conventional memory overlap
check.

Seems like most of the infrastructure for that is already there. We have a
separate list tracking memory banks already. It was just bothering me that there
are quite a few instances where the allocator is blindly receiving external
information instead of taking more agency in the act of reserving that memory.

Suppose I'll just reinstate that old logic though as this series has drug out
much longer than I initially intended.

>> diff --git a/lib/lmb.c b/lib/lmb.c
>> @@ -534,46 +534,19 @@ static long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags)
>> -#ifdef CFG_PRAM
>> -     pram = env_get_ulong('pram', 10, CFG_PRAM);
>> -     pram = pram << 10; /* size is in kB */
>> -#endif
>
> PRAM previously sat above the LMB reservation (the old code subtracted
> pram from the size so PRAM was left out of used_mem). With this patch,
> PRAM ends up inside the [rsv_start, initial_relocaddr) range and is
> now marked LMB_NOOVERWRITE - likely this is more correct, but it is a
> behaviour change for boards setting pram at runtime to be larger than
> CFG_PRAM - those bytes used to be available to LMB clients and now are
> not.
>
> Please spell this out in the commit message rather than the brief
> 'PRAM comes out of this initial area' line, so reviewers on PRAM-using
> boards know to look.
>
>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>> @@ -869,7 +869,7 @@ static void add_u_boot_and_runtime(void)
>>       /* Add U-Boot */
>>       uboot_start = ((uintptr_t)map_sysmem(gd->start_addr_sp, 0) -
>>                      uboot_stack_size) & ~EFI_PAGE_MASK;
>> -     uboot_pages = ((uintptr_t)map_sysmem(gd->ram_top - 1, 0) -
>> +     uboot_pages = ((uintptr_t)map_sysmem(gd->initial_relocaddr, 0) -
>>                      uboot_start + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
>
> Hmmm, the current expression uses ram_top - 1 (inclusive top byte) and
> then rounded up via EFI_PAGE_MASK. The new one passes
> initial_relocaddr without the - 1 - so when initial_relocaddr is
> page-aligned this adds an extra page. What is the intention here?
>
> Regards,
> Simon

This stemmed from the misinterpretation of board_get_usable_ram_top being
exclusive. This is to be adjusted in the next revision.

Re: [PATCHv6 3/3] memory: reserve from start_addr_sp to initial_relocaddr

[Simon Glass]

Hi Randolph,

On Mon, 11 May 2026 at 13:41, Randolph Sapp <rs@ti.com> wrote:
>
> On Mon May 11, 2026 at 1:28 PM CDT, Simon Glass wrote:
> > Hi Randolph,
> >
> > On 2026-05-08T22:29:09, Randolph Sapp <rs@ti.com> wrote:
> >> memory: reserve from start_addr_sp to initial_relocaddr
> >>
> >> Add a new global data struct member called initial_relocaddr. This
> >> stores the original value of relocaddr, directly from setup_dest_addr.
> >> This is specifically to avoid any adjustments made by other init
> >> functions.
> >>
> >> Reserve the memory from gd->start_addr_sp - CONFIG_STACK_SIZE to
> >> gd->initial_relocaddr instead of gd->ram_top. This allows platform
> >> specific relocation addresses to work without unnecessarily painting
> >> over a large range.
> >>
> >> Since PRAM comes out of this initial area up to initial_relocaddr, we no
> >> longer need to account for it separately.
> >>
> >> Signed-off-by: Randolph Sapp <rs@ti.com>
> >>
> >> common/board_f.c                  |  9 ++++++++-
> >>  include/asm-generic/global_data.h |  7 +++++++
> >>  lib/efi_loader/efi_memory.c       |  2 +-
> >>  lib/lmb.c                         | 39 ++++++---------------------------------
> >>  4 files changed, 22 insertions(+), 35 deletions(-)
> >
> > BTW I am missing a change long in this patch, so a bit of guesswork here.
>
> I tend to keep the changelogs for a series in the series cover letter. I assume
> tracking per patch is the preference for this list based on this message.

I'm not sure about others, but for me I struggle with a change log in
another place. It's OK to have it in both places (like patman does),
but trying to read a different email and figure out what change
relates to what patch is hard, particularly when it might have been a
day or a week since I last saw the patch.

>
> >> diff --git a/common/board_f.c b/common/board_f.c
> >> @@ -330,6 +330,8 @@ __weak int arch_setup_dest_addr(void)
> >>
> >>  static int setup_dest_addr(void)
> >>  {
> >> +     int ret;
> >> +
> >>       debug("Monitor len: %08x\n", gd->mon_len);
> >>       /*
> >>        * Ram is setup, size stored in gd !!
> >> @@ -356,7 +358,12 @@ static int setup_dest_addr(void)
> >>       gd->relocaddr = gd->ram_top;
> >>       debug("Ram top: %08llX\n", (unsigned long long)gd->ram_top);
> >>
> >> -     return arch_setup_dest_addr();
> >> +     ret = arch_setup_dest_addr();
> >> +     if (ret != 0)
> >> +             return ret;
> >> +
> >> +     gd->initial_relocaddr = gd->relocaddr;
> >> +     return ret;
> >>  }
> >
> > Please use 'if (ret)' to match U-Boot style. The trailing 'return
> > ret;' is misleading since ret is known to be zero - make it 'return
> > 0;'.
> >
> >> diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h
> >> @@ -107,6 +107,13 @@ struct global_data {
> >>        * GDB using the 'add-symbol-file u-boot <relocaddr>' command.
> >>        */
> >>       unsigned long relocaddr;
> >> +     /**
> >> +      * @initial_relocaddr: end of memory currently in use by uboot
> >> +      *
> >> +      * This should be the original value of relocaddr before any other
> >> +      * allocations or reservations shift it.
> >> +      */
> >> +     unsigned long initial_relocaddr;
> >
> > The kerneldoc summary is misleading: 'currently in use' suggests live
> > state, but this is written once at the end of setup_dest_addr() and
> > never updated. Please reword to something like 'initial top of the
> > U-Boot reservation' and note that it is set once in setup_dest_addr().
> > Also 'uboot' should be 'U-Boot'.
> >
> >> diff --git a/lib/lmb.c b/lib/lmb.c
> >> @@ -534,46 +534,19 @@ static long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags)
> >>
> >>  static void lmb_reserve_uboot_region(void)
> >>  {
> >> -     int bank;
> >> -     ulong end, bank_end;
> >> +     ulong size;
> >>       phys_addr_t rsv_start;
> >> -     ulong pram = 0;
> >>
> >>       rsv_start = gd->start_addr_sp - CONFIG_STACK_SIZE;
> >> -     end = gd->ram_top;
> >> +     size = gd->initial_relocaddr - rsv_start;
> >>
> >> -     /*
> >> -      * Reserve memory from aligned address below the bottom of U-Boot stack
> >> -      * until end of RAM area to prevent LMB from overwriting that memory.
> >> -      */
> >> -     debug("## Current stack ends at 0x%08lx ", (ulong)rsv_start);
> >> +     debug("## Current stack ends at 0x%08lx\n", (ulong)rsv_start);
> >
> > Please keep a trimmed version of the explanatory comment - it
> > documents the intent of this reservation and is more useful than the
> > surrounding context.
> >
> >> diff --git a/lib/lmb.c b/lib/lmb.c
> >> @@ -534,46 +534,19 @@ static long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags)
> >> -     for (bank = 0; bank < CONFIG_NR_DRAM_BANKS; bank++) {
> >> -             if (!gd->bd->bi_dram[bank].size ||
> >> -                 rsv_start < gd->bd->bi_dram[bank].start)
> >> -                     continue;
> >> -             /* Watch out for RAM at end of address space! */
> >> -             bank_end = gd->bd->bi_dram[bank].start +
> >> -                     gd->bd->bi_dram[bank].size - 1;
> >> -             if (rsv_start > bank_end)
> >> -                     continue;
> >> -             if (bank_end > end)
> >> -                     bank_end = end - 1;
> >> +     lmb_reserve(rsv_start, size, LMB_NOOVERWRITE);
> >
> > The old code walked CONFIG_NR_DRAM_BANKS and clamped the reservation
> > to the bank containing rsv_start. The new code drops that and just
> > reserves [rsv_start, initial_relocaddr). On platforms with
> > non-contiguous banks (where U-Boot occupies the top bank and there is
> > a hole below it), this can mark a range that is not all RAM. Is that
> > intentional? If so the commit message should call it out; if not,
> > please re-add a sanity check so we do not reserve across holes.
>
> I had a few thoughts about this in the past week. Initially this bank walking
> behavior seemed more than a little unusual to me. It appears here and in 2 other
> places. (FDT and kernel image loading, if I recall correctly.)
>
> I was mulling over adding it back in as-is, but this really feels like something
> LMB should be aware of and should handle on it's own during the reservation of
> those regions. Something akin to the EFI allocator's conventional memory overlap
> check.
>
> Seems like most of the infrastructure for that is already there. We have a
> separate list tracking memory banks already. It was just bothering me that there
> are quite a few instances where the allocator is blindly receiving external
> information instead of taking more agency in the act of reserving that memory.
>
> Suppose I'll just reinstate that old logic though as this series has drug out
> much longer than I initially intended.

I think Ilias would be the right person to sort out the LMB stuff...I
don't really have much insight on that.

>
> >> diff --git a/lib/lmb.c b/lib/lmb.c
> >> @@ -534,46 +534,19 @@ static long lmb_reserve(phys_addr_t base, phys_size_t size, u32 flags)
> >> -#ifdef CFG_PRAM
> >> -     pram = env_get_ulong('pram', 10, CFG_PRAM);
> >> -     pram = pram << 10; /* size is in kB */
> >> -#endif
> >
> > PRAM previously sat above the LMB reservation (the old code subtracted
> > pram from the size so PRAM was left out of used_mem). With this patch,
> > PRAM ends up inside the [rsv_start, initial_relocaddr) range and is
> > now marked LMB_NOOVERWRITE - likely this is more correct, but it is a
> > behaviour change for boards setting pram at runtime to be larger than
> > CFG_PRAM - those bytes used to be available to LMB clients and now are
> > not.
> >
> > Please spell this out in the commit message rather than the brief
> > 'PRAM comes out of this initial area' line, so reviewers on PRAM-using
> > boards know to look.
> >
> >> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> >> @@ -869,7 +869,7 @@ static void add_u_boot_and_runtime(void)
> >>       /* Add U-Boot */
> >>       uboot_start = ((uintptr_t)map_sysmem(gd->start_addr_sp, 0) -
> >>                      uboot_stack_size) & ~EFI_PAGE_MASK;
> >> -     uboot_pages = ((uintptr_t)map_sysmem(gd->ram_top - 1, 0) -
> >> +     uboot_pages = ((uintptr_t)map_sysmem(gd->initial_relocaddr, 0) -
> >>                      uboot_start + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
> >
> > Hmmm, the current expression uses ram_top - 1 (inclusive top byte) and
> > then rounded up via EFI_PAGE_MASK. The new one passes
> > initial_relocaddr without the - 1 - so when initial_relocaddr is
> > page-aligned this adds an extra page. What is the intention here?
> >
> > Regards,
> > Simon
>
> This stemmed from the misinterpretation of board_get_usable_ram_top being
> exclusive. This is to be adjusted in the next revision.

OK ta.

REgards,
Simon

Re: [PATCHv6,0/3] various memory related fixups

[Simon Glass]

Hi Randolph,

On 2026-05-08T22:29:09, Randolph Sapp <rs@ti.com> wrote:

> That second patch may provoke some opinions, but honestly if that warning was still present I wouldn't have spent a week poking holes in both the EFI and LMB allocations systems.

Thanks for sticking with this.

The series really needs test coverage in test/lib/lmb.c - e.g.
exercise  lmb_reserve_uboot_region(), PRAM case and the new
initial_relocaddr semantics, so the next regression will be noticed at
the time.

Regards,
Simon