Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38424 )
Change subject: cbfs: Enable CBFS mcache on most chipsets ......................................................................
Patch Set 15:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38424/15/src/mainboard/emulation/qe... File src/mainboard/emulation/qemu-aarch64/memlayout.ld:
https://review.coreboot.org/c/coreboot/+/38424/15/src/mainboard/emulation/qe... PS15, Line 23: CBFS_MCACHE(0x6002D800, 8K)
Should this obey the Kconfig option? If not, should it throw an error if Kconfig is set for no cbfs […]
We generally make the non-x86 memlayouts static (and not dynamic with using '.' for the address everywhere, like car.ld and what you did for Picasso). The reasoning is that you usually need to find some way to fit all the optional features in together anyway, and once you have that there's no need to "save" that space when the option is disabled either, it's not like it would really be useful for anything else at that point. So this will still create a _cbfs_mcache region and there'll just be no code doing anything with it if the Kconfig is disabled (which follows established practice, e.g. with FMAP_CACHE() below or VBOOT2_WORK() on other boards).
Also, it's not really optional for this board. I intentionally made NO_CBFS_MCACHE a Kconfig that isn't user-selectable in menuconfig. Either the board really can't fit it and then it has to 'select' that option, or it can fit it and then there should be no reason why a normal user would want to disable it.
https://review.coreboot.org/c/coreboot/+/38424/15/src/soc/amd/picasso/memlay... File src/soc/amd/picasso/memlayout_x86.ld:
https://review.coreboot.org/c/coreboot/+/38424/15/src/soc/amd/picasso/memlay... PS15, Line 82: VBOOT2_WORK(., VB2_FIRMWARE_WORKBUF_RECOMMENDED_SIZE) Not related to my patch, but shouldn't all the ALIGN_COUNTER(64) in memlayout_psp_verstage.ld also be reflected here? Otherwise, if one of these sizes was actually not divisible by 64, the internal offsets in your transfer buffer would all get out of sync...
(BTW, have you considered just making a separate memlayout_transfer_buffer.inc to list all these regions that you #include both here and in memlayout_psp_verstage.ld? That would save you the pain of manually making sure that you always keep everything in sync in this block between the two files.)