Furquan Shaikh 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 33:
(17 comments)
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... File util/spd_tools/intel/ddr4/README.md:
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 101:
Just curious: Do we really need all these optional attributes defined right away? If some or most of […]
Done
https://review.coreboot.org/c/coreboot/+/44429/17/util/spd_tools/intel/ddr4/... File util/spd_tools/intel/ddr4/README.md:
https://review.coreboot.org/c/coreboot/+/44429/17/util/spd_tools/intel/ddr4/... PS17, Line 41: Intel : MRC
just say SoC since this will be used by more than just Intel.
Done
https://review.coreboot.org/c/coreboot/+/44429/17/util/spd_tools/intel/ddr4/... PS17, Line 80: Do
To?
Done
https://review.coreboot.org/c/coreboot/+/44429/29/util/spd_tools/intel/ddr4/... File util/spd_tools/intel/ddr4/gen_part_id.go:
PS29:
We should drop "intel" from the path i.e. move util/spd_tools/intel/ddr4 to util/spd_tools/ddr4. […]
I will address this as a follow-up.
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 91: 1: 0x2,
The JEDEC SPD spec definitions get used by more than just DDR4. […]
Done
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 468: 8: 1 << 4,
That means that the settings for the H5ANAG6NCMR-XN part (a x16 part) is incorrect in the global j […]
Done
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 1134: SPDIndexManufacturerPartNumberStartByte = 329
Why not fix the CRC too? Perhaps nobody checks it in practice, but someone might.
We can add that as follow-up. Currently, LP4x also ignores the CRC field. We can add it if required later on.
https://review.coreboot.org/c/coreboot/+/44429/4/util/spd_tools/intel/ddr4/g... File util/spd_tools/intel/ddr4/global_ddr4_mem_parts.json.txt:
https://review.coreboot.org/c/coreboot/+/44429/4/util/spd_tools/intel/ddr4/g... PS4, Line 7: "bankGroups": 4,
Should be 2 banks for 512Mb x 16.
This field is dropped in latest patchset.
https://review.coreboot.org/c/coreboot/+/44429/4/util/spd_tools/intel/ddr4/g... PS4, Line 24: "pageSize": 4,
Some data sheets state "Page size = 2COLBITS × ORG/8". […]
Done.
https://review.coreboot.org/c/coreboot/+/44429/4/util/spd_tools/intel/ddr4/g... PS4, Line 38: "pageSize": 2,
Page size should be 2KB (4 512B). Perhaps the format for this attribute should be 512B, 1KB or 2KB.
This field is dropped in latest patchset.
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 15: 9 10 11 12 13 14 15 16 17 19 19 20
Are you referring to the line "Programmable CAS latency 9, 10... and 20 supported". […]
Done
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
see comment above
Done
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... File util/spd_tools/intel/ddr4/global_ddr4_mem_parts.json.txt:
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 12: 9 10 11 12 13 14 15 16 17 19 19 20
Reposting comment from patchset#11: […]
Done
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 24: 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 […]
Done
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 36: 9 10 11 12 13 14 15 16 17 19 19 20
same here.
Done
https://review.coreboot.org/c/coreboot/+/44429/19/util/spd_tools/intel/ddr4/... File util/spd_tools/intel/ddr4/global_ddr4_mem_parts.json.txt:
https://review.coreboot.org/c/coreboot/+/44429/19/util/spd_tools/intel/ddr4/... PS19, Line 12: "CASLatencies": "9 10 11 12 13 14 15 16 17 19 19 20"
19 is listed twice.
Done
https://review.coreboot.org/c/coreboot/+/44429/26/util/spd_tools/intel/ddr4/... File util/spd_tools/intel/ddr4/global_ddr4_mem_parts.json.txt:
https://review.coreboot.org/c/coreboot/+/44429/26/util/spd_tools/intel/ddr4/... PS26, Line 12: 9 10 11 12 13 14 15 16 17 18 19 20
Please see comments on patchset 18 and 11.
Done