Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45798 )
Change subject: soc/intel/xeon_sp/cpx: Add save_dimm_info for SMBIOS type 17 ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45798/4/src/soc/intel/xeon_sp/cpx/r... File src/soc/intel/xeon_sp/cpx/romstage.c:
https://review.coreboot.org/c/coreboot/+/45798/4/src/soc/intel/xeon_sp/cpx/r... PS4, Line 41: save_dimm_info
that's a candidate for soc/intel/common, isn't it?
The save_dimm_info() and the SystemMemoryMapHob struct are both Cooper Lake specific, they cannot be used for other Intel platforms so I'm not sure if it can be moved to intel/common.
https://review.coreboot.org/c/coreboot/+/45798/4/src/soc/intel/xeon_sp/cpx/r... PS4, Line 79: 0x1a
Why hardcode? actKeyByte2 might work.
actKeyByte2 is SPD Byte 3: Module type information for SPD_RDIMM, SPD_UDIMM, SPD_SODIMM., etc.
https://review.coreboot.org/c/coreboot/+/45798/4/src/soc/intel/xeon_sp/cpx/r... PS4, Line 88: 1200
Looks like FSP might store this in the SystemMemoryMapHob struct but headers expose it as reserved.
Yes you are right, but CPX does not support DDR5 and DDR4 should be the only option in use, I am not sure if adding the code for them are beneficial. If it's preferred then I will need to expose more hob data.