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 2:
(2 comments)
File src/soc/intel/xeon_sp/chip_gen1.c:
https://review.coreboot.org/c/coreboot/+/81958/comment/d1b9d814_21587c8f : PS1, Line 234: * @param mc_values List of system memory map variables.
As of now only TOHM_REG is used, but theoretically all 64bit defined maps might be related […]
Personally, I think to have the soc memory map codes to get a full picture of system memory map settings, a.k.a. the full list, would be helpful for potential usages, given its original design intention.
(P.S. I copy the full memory map list to show that, like TOHM_REG defined as a 64bit map item, there are other 64bit map items which might impact >4G maps in future as well.)
As a code movement patch, my personal preference is to keep the full list first. I can have a another patch to change the parameter to TOHM item only (but in future, we might need to add other items as parameters based on needs). Your opinion?
File src/soc/intel/xeon_sp/uncore.c:
https://review.coreboot.org/c/coreboot/+/81958/comment/2445e74a_beb5f3d8 : PS1, Line 306: } else {
You could still split it further. Now you have an `if (CXL)` in `chip_gen1.c`. This […]
Maybe to have soc_add_dram_resources(), soc_add_cxl_mem_resources() and soc_add_misc_resources()? Since this is a code movement patch, I could have a separate patch for the further splits.