Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36216 )
Change subject: soc/intel: common,skl,cnl,icl: drop reserved mmio memory size calculation ......................................................................
Patch Set 8: Code-Review+1
(4 comments)
There is no issue wrt. this change on master. The memmap calculations in general probably get some corner cases wrong. SGX disabled is not a corner case though, `sgx_enable = 0` and `PrmrrSize = 0` are the default.
Beside my latest comment on the commit message, I think this change is ready.
https://review.coreboot.org/c/coreboot/+/36216/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36216/7//COMMIT_MSG@22 PS7, Line 22: change) without adding more weird calculations. The second point seems to be just noise. And I'm not convinced by some of the details. Probably something that was tested in between the other patches?
Let's focus on the calculations, we know that: * TSEG - DPR - reserved - top_of_memory is 0 * TSEG - DPR - reserved is top_of_memory.
And hence can simplify the code here. This was confirmed by reading the code and testing.
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent.c:
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... PS5, Line 187: - reserved_mmio_size;
going by the ascii art memory map those carve outs should be taken into account. […]
Done?
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... PS5, Line 191: base_k = sa_map_values[SA_TSEG_REG] - dpr_size - reserved_mmio_size;
This should be cbmem_top() by definition. […]
Done
https://review.coreboot.org/c/coreboot/+/36216/5/src/soc/intel/common/block/... PS5, Line 180: /* TSEG - DPR -> BGSM */
While we are at it. Could we update these comments to state something […]
Done