Attention is currently required from: Angel Pons, Arthur Heymans, Christian Walter, Felix Held, Johnny Lin, Jonathan Zhang, Lean Sheng Tan, Nico Huber, Patrick Rudolph, Tim Chu.
Shuo Liu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81958?usp=email )
Change subject: soc/intel/xeon_sp: Add soc_add_dram_resources ......................................................................
Patch Set 1:
(3 comments)
File src/soc/intel/xeon_sp/chip_gen1.c:
https://review.coreboot.org/c/coreboot/+/81958/comment/92d4f936_53fed3bd : PS1, Line 234: * @param mc_values List of system memory map variables.
Why pass the whole array? This makes the API more complicated than necessary. […]
As of now only TOHM_REG is used, but theoretically all 64bit defined maps might be related, hence I think it still makes sense to pass the full list. Another point worth mentioning is that the SoC maps are not limited to >4G maps (though most of case current being handled are for >4G maps), hence to have the knowledge of the full map will help. I updated the commit message to reflect this. Your opinion?
static struct map_entry memory_map[NUM_MAP_ENTRIES] = { [TOHM_REG] = MAP_ENTRY_LIMIT_64(VTD_TOHM_CSR, 26, "TOHM"), [MMIOL_REG] = MAP_ENTRY_BASE_32(VTD_MMIOL_CSR, "MMIOL"), [MMCFG_BASE_REG] = MAP_ENTRY_BASE_64(VTD_MMCFG_BASE_CSR, "MMCFG_BASE"), [MMCFG_LIMIT_REG] = MAP_ENTRY_LIMIT_64(VTD_MMCFG_LIMIT_CSR, 26, "MMCFG_LIMIT"), [TOLM_REG] = MAP_ENTRY_LIMIT_32(VTD_TOLM_CSR, 26, "TOLM"), #if CONFIG(SOC_INTEL_HAS_NCMEM) [NCMEM_BASE_REG] = MAP_ENTRY_BASE_64(VTD_NCMEM_BASE_CSR, "NCMEM_BASE"), [NCMEM_LIMIT_REG] = MAP_ENTRY_LIMIT_64(VTD_NCMEM_LIMIT_CSR, 19, "NCMEM_LIMIT"), #else [ME_BASE_REG] = MAP_ENTRY_BASE_64(VTD_ME_BASE_CSR, "ME_BASE"), [ME_LIMIT_REG] = MAP_ENTRY_LIMIT_64(VTD_ME_LIMIT_CSR, 19, "ME_LIMIT"), #endif [TSEG_BASE_REG] = MAP_ENTRY_BASE_32(VTD_TSEG_BASE_CSR, "TSEGMB_BASE"), [TSEG_LIMIT_REG] = MAP_ENTRY_LIMIT_32(VTD_TSEG_LIMIT_CSR, 20, "TSEGMB_LIMIT"), [VTDBAR_REG] = MAP_ENTRY_BASE_32(VTD_BAR_CSR, "VTD_BAR"), };
File src/soc/intel/xeon_sp/uncore.c:
https://review.coreboot.org/c/coreboot/+/81958/comment/a2d2ba0f_25d81cd2 : PS1, Line 306: } else {
So there were already two different implementations. One without and one […]
Currently soc_mc_add_dram_resources holds all SoC specific mappings, CXL, >4G DRAM, and others (in future), hence the memory maps are simply split to common part and non-common part instead of a CXL part and non-CXL part. I updated the commit message to mention this.
https://review.coreboot.org/c/coreboot/+/81958/comment/c67ff5fe_2866c1ea : PS1, Line 274: index +=
Returning the count and adding it up is uncommon. And if reading the whole code, […]
Done