Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44429 )
Change subject: util: Add spd_tools to generate SPDs for TGL boards ......................................................................
Patch Set 15:
(10 comments)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... File util/spd_tools/intel/ddr4/gen_spd.go:
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 96: 32: 0x7,
Same comment as above.
Done
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 468: 8: 1 << 4,
From JESD79-4C: "The DDR4 SDRAM is a high-speed dynamic random-access memory internally configured a […]
That means that the settings for the H5ANAG6NCMR-XN part (a x16 part) is incorrect in the global json file as it's listed with 4 bank groups and 4 banks per group with deviceBusWidth of 16.
If that is true, doesn't that mean that all x4/x8 parts have one bankGroups/banksPerGroup setting and all x16 parts have a second bankGroups/banksPerGroup, then we don't need to specify banks or bank groups as part of the json, we can determine that based on deviceBusWidth?
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 472: 0: 0 << 6,
Same as above.
Done
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 490: densityGbx8x16ChannelToRowColumnEncoding
Is that for TGL? I think its fine if we don't for now. But it should be documented. […]
Currently, validateDeviceBusWidth() only allows x8 and x16 as options, otherwise it returns error "Incorrect device bus width: 4"
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 495: encodePackage
encodePackageDeviceType?
Thanks. Done.
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 649: Per Table 68 of Jedec spec 4.1.1.L-5R29 v103,
I will update this. Leaving this comment unresolved until I do.
Done
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 732: Per Table 70 of Jedec spec 4.1.1.L-5R29 v103,
I will update this. Leaving this comment unresolved until I do.
Done
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 1374: bankGroups != 0
See comment above about banks and bank groups.
Done
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 1381: && banksPerGroup != 8
Same as above.
Since it can only be 4, I've removed BanksPerGroup as a json parameter.
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 1413: dieCount <= 8
From JESD79-4C, it doesn't look like more than 2 dies are supported?
Done