Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35902 )
Change subject: soc/intel/apl,skl,cnl,icl: implement a PRMRR size selection algorithm ......................................................................
Patch Set 8:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35902/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35902/7//COMMIT_MSG@34 PS7, Line 34: Todo: needs testing
Testers are welcome! :-) Well, currently nobody knows, what FSP-M does... […]
I think all we have to do is to give FSP-M a size setting that the CPU supports. Then we can be sure that it will use exactly that setting.
Then, the only thing we have to know about FSP-M is how it will align PRMRR. (And only if we keep mimicking the whole FSP memmap. As we use the EBDA to cache TOLUD now, I assume we could also get rid of that and store the address returned by FSP instead.)
Subrata, in the coreboot code we currently have 128MiB alignment for the big cores and no alignment but 2x the configured size for GLK. Can you confirm the alignment, please? I don't have access to all the sources. A grep for `MAX_PRMRR` should help to find the respective code.
I'm rather sure the alignment is 256MiB for newer SoCs. Maybe Kaby Lake is the exception, though, and really aligns to 128MiB.
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/apollolake/ch... File src/soc/intel/apollolake/chip.h:
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/apollolake/ch... PS8, Line 201: size_t get_prmrr_size(void); `chip.h` is dedicated to the devicetree configuration. A better place would be one of the include/soc/ headers. I can't find any good match, though, maybe add a `include/soc/memmap.h`?
If you intend to call this function from common code, it should be prefixed with `soc_`.
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/cannonlake/me... File src/soc/intel/cannonlake/memmap.c:
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/cannonlake/me... PS8, Line 83: prmrr_base = ALIGN_DOWN(prmrr_base, 16 * MiB); This case needs further analysis. The last code I've seen seems to try to fill the gap for TSEG alignment, but doesn't explicitly align down. As no board sets `enable_c6dram`, I assume this case is untested.
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/common/block/... PS8, Line 346: MiB As far as I followed the code, it's in MiB already.
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/common/block/... PS8, Line 346: valid_size limit?
https://review.coreboot.org/c/coreboot/+/35902/8/src/soc/intel/common/block/... PS8, Line 348: MiB As far as I followed the code, it's in MiB already.