Attention is currently required from: Felix Held, Fred Reitberger, Jason Glenesk, Marshall Dawson, Marvin Drees, Matt DeVillier, Paul Menzel, Raul Rangel.
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/64950?usp=email )
Change subject: soc/amd/noncar: Use romstage MEM in bootblock if !SEPARATE_ROMSTAGE ......................................................................
Patch Set 34:
(1 comment)
File src/soc/amd/common/block/cpu/noncar/memlayout_x86.ld:
https://review.coreboot.org/c/coreboot/+/64950/comment/b24e8ec3_f83282bf : PS27, Line 102: BOOTBLOCK(CONFIG_ROMSTAGE_ADDR, CONFIG_ROMSTAGE_SIZE)
hmm, this ends up doing the right thing, but i found it confusing when looking at it at first. maybe a comment would help? would also be good if that was somehow integrated into the diagram above to have that cover both cases. might also be good to add a comment to the ROMSTAGE_SIZE Kconfig that this will be used as bootblock size in the combined bootblock and romstage case.
a probably better option might be to have different C_ENV_BOOTBLOCK_SIZE sizes dependent on whether SEPARATE_ROMSTAGE is selected in the Kconfig and make ROMSTAGE_SIZE 0 in the !SEPARATE_ROMSTAGE case. not sure if we'd need to put the ROMSATGE macro in here in a #if CONFIG(SEPARATE_ROMSTAGE) block. the calculation of BOOTBLOCK_END and BOOTBLOCK_ADDR would also need to be reworked for that
Currently the base of bootblock gets computed from ROMSTAGE_BASE. If one just increase C_ENV_BOOTBLOCK_SIZE it might overextend downwards overlapping the PSP_SHAREDMEM_SIZE. It does reduce the amount of the amount of DRAM used as ROMSTAGE has an arbitrarily large value which you could tune if using C_ENV_BOOTBLOCK_SIZE. A better way would be to set BOOTBLOCK_BASE in Kconfig and compute romstage base based on that.
A computed bootblock and romstage base/size separatly to make it more clear. Maybe a follow-up can tune sizes better but this should basically work?
I added a comment clarifying the computation. That should do for now?