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 2:
(2 comments)
File src/soc/intel/xeon_sp/chip_gen1.c:
https://review.coreboot.org/c/coreboot/+/81958/comment/5337b1e5_8bf35f77 : 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
Can you give an example?
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?
static struct map_entry memory_map[NUM_MAP_ENTRIES] = { ...
Why did you copy this? Is there anything particular you want to show? I see no difference to the current code.
File src/soc/intel/xeon_sp/uncore.c:
https://review.coreboot.org/c/coreboot/+/81958/comment/7e6f2f38_e532af64 : PS1, Line 306: } else {
Currently soc_mc_add_dram_resources holds all SoC specific mappings, CXL, >4G DRAM, and others (in f […]
You could still split it further. Now you have an `if (CXL)` in `chip_gen1.c`. This is not necessary, as you could split it into two separate implementations of soc_add_dram_resources().
For consistency, because you have two mechanisms to have alternative implementations: 1) `if`, 2) multiple soc_add_dram_resources(). I don't think it's wise to mix them.