Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34680 )
Change subject: dram: Add basic DDR4 SPD parsing ......................................................................
Patch Set 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/34680/3/src/device/dram/ddr4.c File src/device/dram/ddr4.c:
https://review.coreboot.org/c/coreboot/+/34680/3/src/device/dram/ddr4.c@45 PS3, Line 45: start with spd[0] to get "Bytes Total" and "Bytes used"
https://review.coreboot.org/c/coreboot/+/34680/3/src/device/dram/ddr4.c@56 PS3, Line 56: reg8 = spd[13] & ((1 << 4) - 1); I personally find hex values easier to read.
https://review.coreboot.org/c/coreboot/+/34680/3/src/device/dram/ddr4.c@80 PS3, Line 80: dimm->manufacturer_id = (spd[351] << 8) | spd[350]; check if 351 is within "Bytes used" in spd[0]
https://review.coreboot.org/c/coreboot/+/34680/3/src/device/dram/ddr4.c@85 PS3, Line 85: memcpy(dimm->part_number, &spd[329], SPD_DDR4_PART_LEN); make sure those are properly null terminated
https://review.coreboot.org/c/coreboot/+/34680/3/src/device/dram/ddr4.c@120 PS3, Line 120: dimm->ddr_frequency = selected_freq; besides "configured memory speed" there's also a field for "maximum memory speed". It would be great if you could parse tCK, MTB and FTB, too.