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 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/36216/5//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/36216/5//COMMIT_MSG@14 PS5, Line 14: Since this will always be equal to 0
I'm not sure I understand or follow. […]
Oh, thanks for pointing this out. There was some misunderstanding because CB:21539 which introduced this reserved memory handling wasn't clear about the intention.
I'll point out where in the code we have this 0.
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; So, this is 0 by definition.
So originally, before CB:21539, the range cbmem_top()..TSEG-DPR was marked as MMIO. Currently, we reserved an empty range here (which led to some confusion).
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.
So, I guess we can get rid of the reserved size calculation (what this change suggests), but should keep the current behaviour (mark everything as reserved RAM; not MMIO).
Michael, please compare logs with the current upstream code and this patch altered so that it says `base_k = top_of_ram;` here. I would expect no difference. The above mmio_resource() could be completely dropped then. (meh, this will not be fun to put into a reasonable commit message :-/)
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 useful? e.g. what is being reserved (and not how it's calculated; that is already obvious from the code).