Keith Short has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33379 )
Change subject: soc/intel: Provide SPD manufacturer ID and module type to SMBIOS ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/#/c/33379/1/src/soc/intel/apollolake/meminit_uti... File src/soc/intel/apollolake/meminit_util_apl.c:
https://review.coreboot.org/#/c/33379/1/src/soc/intel/apollolake/meminit_uti... PS1, Line 98: 0);
APL FSP does not expose the SpdModuleType.
Does coreboot (at any stage) read SPD directly, or is this only done by FSP? Is there value if coreboot populated the module type itself? The only thing I see in smbios is the module type sets the form factor - is this just informational to the OS?
https://review.coreboot.org/#/c/33379/1/src/soc/intel/common/smbios.c File src/soc/intel/common/smbios.c:
https://review.coreboot.org/#/c/33379/1/src/soc/intel/common/smbios.c@30 PS1, Line 30: /* Translate to DDR2 module type field that SMBIOS code expects. */ The module types included below are shared by DDR3 and DDR4 so probably good to comment that. This switch statement below seems to be a copy paste from device/sram/ddr3.c:spd_add_smbios17(). Can you make a utility routine to minimize the cleanup later?
https://review.coreboot.org/#/c/33379/1/src/soc/intel/common/smbios.c@29 PS1, Line 29: dimm->mod_id = mod_id; : /* Translate to DDR2 module type field that SMBIOS code expects. */ : switch (mod_type) { : case SPD_DIMM_TYPE_SO_DIMM: : dimm->mod_type = SPD_SODIMM; : break; : case SPD_DIMM_TYPE_72B_SO_CDIMM: : dimm->mod_type = SPD_72B_SO_CDIMM; : break; : case SPD_DIMM_TYPE_72B_SO_RDIMM: : dimm->mod_type = SPD_72B_SO_RDIMM; : break; : case SPD_DIMM_TYPE_UDIMM: : dimm->mod_type = SPD_UDIMM; : break; : case SPD_DIMM_TYPE_RDIMM: : dimm->mod_type = SPD_RDIMM; : break; : case SPD_DIMM_TYPE_UNDEFINED: : default: : dimm->mod_type = SPD_UNDEFINED; : break; : }
I checked the SMBIOS spec and it doesn't contain DDR2 specific fields.
SMBIOS spec doesn't, but src/arch/x86/smbios.c is expecting DDR2 values.