Hi Tom, On Mon, 2 Dec 2024 at 14:59, Tom Rini <tr...@konsulko.com> wrote: > > On Sun, Dec 01, 2024 at 08:28:05AM -0700, Simon Glass wrote: > > > The reserve_stack_aligned() function already ensures that the resulting > > address is aligned to a 16-byte boundary. The comment seems to suggest > > that 16 is passed reserve_stack_aligned() to make it aligned. > > > > Change the value to 0, since the stack can start at the current address, > > if it is suitably aligned already. > > > > Signed-off-by: Simon Glass <s...@chromium.org> > > Er: > /* > * reserve after start_addr_sp the requested size and make the stack pointer > * 16-byte aligned, this alignment is needed for cast on the reserved memory > * ref = x86_64 ABI: https://reviews.llvm.org/D30049: 16 bytes > * = ARMv8 Instruction Set Overview: quad word, 16 bytes > */ > static unsigned long reserve_stack_aligned(size_t size) > { > return ALIGN_DOWN(gd->start_addr_sp - size, 16); > } > > is what the code is today. > > > --- > > > > (no changes since v1) > > > > common/board_f.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/common/board_f.c b/common/board_f.c > > index 98dc2591e1d..677e37d93c0 100644 > > --- a/common/board_f.c > > +++ b/common/board_f.c > > @@ -601,7 +601,7 @@ __weak int arch_reserve_stacks(void) > > static int reserve_stacks(void) > > { > > /* make stack pointer 16-byte aligned */ > > - gd->start_addr_sp = reserve_stack_aligned(16); > > + gd->start_addr_sp = reserve_stack_aligned(0); > > > > /* > > * let the architecture-specific code tailor gd->start_addr_sp and > > And this is the last call to reserve_stack_aligned(). So, this is going > to save us between 16 and 0 bytes of memory. If we're going to make a > change here we need to comment in the code more what we're up to at this > point, and the commit message should be more authoritatively worded too.
So, other than that, what do you think? Should I respin it? Note that this is part of a series which I doubt will be applied to your tree. It would be nice if other archs could weigh in, but I see that I have not copied them. Still, the current code is confusing (in that the comment doesn't match the code), so we can likely figure this out in -next Heinrich expressed some concerns with RISC-V, but there doesn't seem to be any breakage in CI. meminfo reports that the devicetree is above the stack and it semes intact: $ qemu-system-riscv32 -nographic -machine virt -bios /tmp/b/qemu-riscv32/u-boot.bin U-Boot 2025.01-rc3-00068-g6f392929ea11-dirty (Dec 03 2024 - 06:51:44 -0700) CPU: riscv Model: riscv-virtio,qemu DRAM: 128 MiB Core: 26 devices, 13 uclasses, devicetree: board Flash: 32 MiB Loading Environment from nowhere... OK In: serial,usbkbd Out: serial,vidconsole Err: serial,vidconsole No USB controllers found Net: No ethernet found. Hit any key to stop autoboot: 0 => meminfo DRAM: 128 MiB Region Base Size End Gap ------------------------------------------------ video 87800000 800000 88000000 code 87740000 bf2d8 877ff2d8 d28 malloc 86f20000 820000 87740000 0 board_info 86f1ffc0 40 86f20000 0 global_data 86f1fe50 170 86f1ffc0 0 devicetree 86f1ef30 f02 86f1fe32 1e stack 85ede000 1000000 86ede000 40f30 lmb 85ede000 0 85ede000 0 lmb 85edb000 3000 85ede000 0 free 80000000 85edb000 5edb000 80000000 => md 86f1ef30 86f1ef30: edfe0dd0 020f0000 38000000 980d0000 ...........8.... 86f1ef40: 28000000 11000000 10000000 00000000 ...(............ 86f1ef50: 6a010000 600d0000 00000000 00000000 ...j...`........ Regards, Simon
