Michael Niewöhner 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 ......................................................................
Uploaded patch set 16.
(5 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
Thanks for checking. Must have twisted e1 <-> 1e. […]
exactly :)
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);
We should tested on CNL and ICL based platform where C6 DRAM been supported, i'm currently OOO till […]
outdated
https://review.coreboot.org/c/coreboot/+/35902/4/src/soc/intel/common/block/... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35902/4/src/soc/intel/common/block/... PS4, Line 334: <<
The rule is simple, we place spaces around all binary operators. […]
Done
https://review.coreboot.org/c/coreboot/+/35902/13/src/soc/intel/common/block... File src/soc/intel/common/block/cpu/cpulib.c:
https://review.coreboot.org/c/coreboot/+/35902/13/src/soc/intel/common/block... PS13, Line 345: die("Unsupported PRMRR size limit %i MiB, check your config!\n", limit);
What if PRMRR isn't supported at all? What does this MSR read back? I'm curious if this MSR always r […]
Well, AFAIK prmrr is supported on any cpu, but was named EMRR on previous families
https://review.coreboot.org/c/coreboot/+/35902/13/src/soc/intel/common/block... File src/soc/intel/common/block/include/intelblocks/cpulib.h:
https://review.coreboot.org/c/coreboot/+/35902/13/src/soc/intel/common/block... PS13, Line 165: /* Get the maximum valid PRMRR size with respect to the specified limits */
Indicate the units returned. Code inspection seems to indicate 'size in bytes'.
Done