Michał Żygowski has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34883 )
Change subject: [WIP] amd/stoneyridge: Fix CAR stack location ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
I think you've correctly identified the problem although I'm not sure what other behaviors this change might create. Short of rewriting all the gcccar.inc code, I wonder if a better solution is to make DCACHE_BSP_STACK_SIZE match gcccar's BSP_STACK_SIZE. It seems like a while back I'd tried experimenting with using the CONFIG_ values in gcccar but saw it breaking more than it fixed.
You can't do that. DCACHE_RAM_SIZE has to match BSP_STACK_SIZE. And within DCACHE_RAM_SIZE you need the room for vboot, console, timestamps and .bss too.
For the production branch, you also cannot change CAR layout from what (RO) bootblock was built with originally. And we don't know if 0x4000 is a sufficient DCACHE_BSP_STACK_SIZE, I assume the value was chosen with infamous copy-paste method.
That leaves us with the option of fixing car.ld to match gcccar.inc (or cache_as_ram.S). That is, tell the linker stack grows downwards from _car_region_end. I just hate to put a platform-specific hacks into that file... We do have that alternative definition of stack located at the end, so I don't think it would look that bad to take this approach.
Can we address the issue like that? : https://review.coreboot.org/c/coreboot/+/36914/8/src/drivers/amd/agesa/cache...