Nick Vaccaro has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44429 )
Change subject: util: Add spd_tools to generate DDR4 SPDs for TGL boards ......................................................................
Patch Set 26:
(9 comments)
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... File util/spd_tools/intel/ddr4/gen_spd.go:
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 513: 4284
Ah okay. […]
Done
https://review.coreboot.org/c/coreboot/+/44429/26/util/spd_tools/intel/ddr4/... File util/spd_tools/intel/ddr4/gen_spd.go:
https://review.coreboot.org/c/coreboot/+/44429/26/util/spd_tools/intel/ddr4/... PS26, Line 265: case 4: : width = 0
This is not really supported, right?
No, it's not. I'll remove it.
https://review.coreboot.org/c/coreboot/+/44429/26/util/spd_tools/intel/ddr4/... PS26, Line 530:
nit: extra blank line not required.
Done
https://review.coreboot.org/c/coreboot/+/44429/26/util/spd_tools/intel/ddr4/... PS26, Line 537:
nit: extra blank line not required.
Done
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... File util/spd_tools/intel/ddr4/global_ddr4_mem_parts.json.txt:
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 13: "signalLoading": "not specified",
See my comment on the README on patchset 9. Signal loading can be derived from package type i.e. […]
Done
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 15: 9 10 11 12 13 14 15 16 17 19 19 20
I don't think this is correct. […]
The speed bin 3200AA is SUPPOSED to support 9 thru 22 and 24. The spec for this part said it only supported those listed above (i.e. it doesn't support 21, 22, or 24, even though it's binned as a 22-22-22 3200AA part, so the default setting is overridden here (see page 3 of the part spec sheet here : https://drive.google.com/corp/drive/folders/177abEFOZkA9iLEMJMXMj2WHKBCr9966...)
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 25: "pageSize": "2KB",
Please see my comment on patchset 9. I don't think we need this separate attribute. […]
Done
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 30: 10 11 12 13 14 15 16 17 19 19 20 21 22
Where are these values coming from? I think this part also supports all the standard latencies for 3 […]
see comment above, they're from the part spec sheet.
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 45: 9 10 11 12 13 14 15 16 17 19 19 20
same here as above.
see comment above