Attention is currently required from: Angel Pons, Arthur Heymans, Christian Walter, Jincheng Li, Johnny Lin, Jonathan Zhang, Lean Sheng Tan, Patrick Rudolph, Tim Chu.
Shuo Liu has posted comments on this change by Shuo Liu. ( https://review.coreboot.org/c/coreboot/+/83325?usp=email )
Change subject: soc/intel/xeon_sp: Share save_dimm_info among Xeon-SP SoCs ......................................................................
Patch Set 5:
(4 comments)
File src/soc/intel/xeon_sp/gnr/romstage.c:
https://review.coreboot.org/c/coreboot/+/83325/comment/778a840f_79bfd8d1?usp... : PS3, Line 90: uint8_t get_dram_type(void) : { : const struct SystemMemoryMapHob *hob = get_system_memory_map(); : if (hob && hob->DramType == SPD_MEMORY_TYPE_DDR5_SDRAM)
The code that has been factored out (made shared) uses `struct SystemMemoryMapHob` already. […]
Done
File src/soc/intel/xeon_sp/romstage.c:
https://review.coreboot.org/c/coreboot/+/83325/comment/adde63e1_85d86e62?usp... : PS3, Line 43: #if CONFIG(SOC_INTEL_SKYLAKE_SP)
Hmmm, instead of this, why not place `save_dimm_info()` in a separate `dimm_info. […]
It is because SKX doesn't support the HOB context needed to obtain the DIMM info, but the SKX will still get save_dimm_info() called from the common codes (mainboard_romstage_entry). Since the common code will always call save_dimm_info for SKX, it looks like putting save_dimm_info() to a separate file still not mitigate this issue. Or let us define save_dimm_info() as __weak and override to a null implementation for SKX?
https://review.coreboot.org/c/coreboot/+/83325/comment/399c9fd0_4ceaee3a?usp... : PS3, Line 69: : for (int skt = 0; skt < CONFIG_MAX_SOCKET; skt++) { : for (int ch = 0; ch < MAX_CH; ch++) { : for (int dimm = 0; dimm < get_max_dimm_count(); dimm++) {
nit: if `get_max_dimm_count()` always returns the same value, I would add a helper constant: […]
Done
File src/soc/intel/xeon_sp/spr/romstage.c:
https://review.coreboot.org/c/coreboot/+/83325/comment/83b7ed7d_a5c63b85?usp... : PS3, Line 132: uint32_t get_max_capacity_mib(void)
Move this next to the other functions?
Done