Attention is currently required from: Andrey Petrov, Arthur Heymans, Chen, Gang C, David Hendricks, Johnny Lin, Lean Sheng Tan, Patrick Rudolph, Ronak Kanabar, Shuo Liu.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80328?usp=email )
Change subject: drivers/intel/fsp2_0: Add FSP_DOES_NOT_NEED_TEMP_RAM ......................................................................
Patch Set 2: Code-Review-1
Copied votes on follow-up patch sets have been updated: * Code-Review-1 has been copied to patch set 3 (copy condition: "changekind:NO_CHANGE OR changekind:NO_CODE_CHANGE OR changekind:TRIVIAL_REBASE OR is:MIN").
(2 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/80328/comment/f7b36b91_469de38c : PS1, Line 9: FSP allocates temporary ram in its reserved memory ranges : instead of requesting coreboot to do the allocation. This : is supported by some FSP implementations for Xeon SP. Are you sure that is what is going on? It's just using the default settings which avoid this heap being in .bss which is above the stack?
I think it's more prudent to still explicitly set the values and use Kconfig params to do that, but to explain what is going on. Memory management is something you want explicit control over and not leave up to defaults. The end result is still the same location with for the heap, but at least it's visible what is going on and not an implicit default.
File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/80328/comment/63318fb4_3e3ab9db : PS1, Line 156: goto temp_ram_assigned; goto is considered bad taste. Just add an if clause below to skip the UPD setup.
But I'd still prefer to set values explicitly: Add a new Kconfig setting 'XEON_SP_FSP_HEAP_WORKAROUND' and based on that set base at 'CONFIG_DCACHE_RAM_BASE + CONFIG_FSP_M_RC_HEAP_SIZE - CONFIG_FSP_TEMP_RAM_SIZE' and size to CONFIG_FSP_TEMP_RAM_SIZE, while still increasing CONFIG_FSP_M_RC_HEAP_SIZE to encompass both. Change the help text of CONFIG_FSP_M_RC_HEAP_SIZE that it includes both heaps.