Attention is currently required from: Arthur Heymans, Christian Walter, Felix Held, Johnny Lin, Jonathan Zhang, Patrick Rudolph, Shuo Liu, Tim Chu.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81958?usp=email )
The change is no longer submittable: All-Comments-Resolved is unsatisfied now.
Change subject: soc/intel/xeon_sp: Add soc_add_dram_resources ......................................................................
Patch Set 1:
(4 comments)
Patchset:
PS1: Please do not submit before the new implementation is under review. I always find refactorings hard to assess when new implementations are unknown.
File src/soc/intel/xeon_sp/chip_gen1.c:
https://review.coreboot.org/c/coreboot/+/81958/comment/0ca9e30c_86eadb34 : 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. AIUI, the purpose of this functions is to add DRAM regions between 4G and the absolute top. This would only need the top value.
File src/soc/intel/xeon_sp/uncore.c:
https://review.coreboot.org/c/coreboot/+/81958/comment/9b20d96c_8f4cc357 : PS1, Line 306: } else { So there were already two different implementations. One without and one with CXL. IOW, a gen1 and a gen4 implementation. If we extract this into a separate function, with API and everything. Shouldn't we be consistent and split this right away?
https://review.coreboot.org/c/coreboot/+/81958/comment/f4b2d25b_ac764074 : PS1, Line 274: index += Returning the count and adding it up is uncommon. And if reading the whole code, it looks odd: Last thing before the return is to subtract the old index, and the very next thing here is to add it again. It's more common to return the new value or passing a pointer to change the variable directly.
It's also odd to have one style for mc_add_dram_resources() (passing a pointer) and then doing it differently for soc_add_dram_resources() (returning the count). Please choose one way of doing it.