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/11ece306_3025ebfe : PS1, Line 234: * @param mc_values List of system memory map variables.
Personally, I think to have the soc memory map codes to get a full picture of system memory map sett […]
This would mean that you'd move the entire enum into the API only to move it back in the next commit? I don't think this makes sense, it would create noise in the git history.
If you want to split the two steps (making it TOHM only, and moving the code to another file), you could first create a static function in `uncore.c` with the new interface, and then move the code in a second commit.
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.
File src/soc/intel/xeon_sp/uncore.c:
https://review.coreboot.org/c/coreboot/+/81958/comment/d603ae7c_036d3e61 : PS1, Line 306: } else {
It's hard to review this change without knowing what the implementation of `soc_add_dram_resources` […]
Sorry, I guess this was misunderstood. Your gen1 soc_add_dram_resources() has all its code in two branches of an `if` that is decided at compile time. This already makes it two different implementations of the same function.
With splitting I meant to split it into to separate soc_add_dram_resources() functions one for gen1 w/o CXL (`chip_gen1.c` seems to be a good place for that) and one for SPR which has CXL (if it's not shared between platforms, I guess it should reside somewhere in `spr/`).