Attention is currently required from: Angel Pons, Arthur Heymans, Christian Walter, Felix Held, Johnny Lin, Jonathan Zhang, Lean Sheng Tan, Patrick Rudolph, Shuo Liu, Tim Chu.
Nico Huber 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 3:
(1 comment)
File src/soc/intel/xeon_sp/chip_gen1.c:
https://review.coreboot.org/c/coreboot/+/81958/comment/7d347a21_5a4051d5 : PS1, Line 234: * @param mc_values List of system memory map variables. I'll make one desperate last attempt to get this answered...
You seem to have missed my question above:
After this patch all the array entries but TOHM are handled by common code. If an implementation of soc_add_dram_resources() would try to handle other entries as well, wouldn't that cause conflicts?
IMO, this is very important for the API design: "[...], wouldn't that cause conflicts?". If it would, we should never create such an API that encourages error-prone code.
This is analyzing the code after this commit. I'll start at the bottom, in the order as the entries are processed by mc_add_dram_resource(): * VTDBAR_REG: if set, mc_add_dram_resources() will add a resource for it. What could soc_add_dram_resources() do afterwards with the value of VTDBAR_REG without causing a conflict with the resource added by mc_add_dram_resources()? * TSEG_BASE_REG: mc_add_dram_resources() adds directly below this (according to the "Host Memory Map", line 123, that's DPR or cbmem_top()) and from this base to TSEG_BASE_LIMIT (twice!). What could soc_add_dram_resources() do afterwards with the value of TSEG_BASE_REG without causing a conflict with the resources added by mc_add_dram_resources()? * TSEG_LIMIT_REG: again resources are directly below and above. What could soc_add_dram_resources() do afterwards with the value of TSEG_LIMIT_REG without causing a conflict with the resources added by mc_add_dram_resources()? * TOLM_REG: Resources are added without gap from TSEG_LIMIT_REG which should include ME_BASE/_LIMIT_REG and potentially NCMEM_BASE/_LIMIT_REG. "Host Memory Map" suggests that MMCFG_BASE_REG is adjacent, but I suppose there is a potential alignment gap. I guess soc_add_dram_resources() could put something into this gap. Is that the intention? Could that work without additional information? * MMCFG_BASE/_LIMIT_REG: Resource is added in between, according to the "Host Memory Map" other things follow above. So using MMCFG_LIMIT_REG within soc_add_dram_resources() without additional information would seem dangerous. * MMIOL_REG: seems ignored, is this a bug? * TOHM_REG: code to handle this is moved into soc_add_dram_resources(). So passing this on makes perfect sense to me.
Please tell me what I miss and how the values passed could be used to add more resources that wouldn't conflict.