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 12:
(41 comments)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/44429/9/util/spd_tools/intel/ddr4/R... File util/spd_tools/intel/ddr4/README.md:
https://review.coreboot.org/c/coreboot/+/44429/9/util/spd_tools/intel/ddr4/R... PS9, Line 51: `name`:
Name is not really under `attribs`. It is already captured on line 37 above.
Done
https://review.coreboot.org/c/coreboot/+/44429/9/util/spd_tools/intel/ddr4/R... PS9, Line 54: `"1600J", "1600K", "1600L", "1866L", "1866M", "1866N", "2133N", "2133P", : "2133R", "2400P", "2400R", "2400T", "2400U", "2666T", "2666U", "2666V", : "2666W", "2933V", "2933W", "2933Y", "2933AA", "3200W", "3200AA", "3200AC"`
Based on the values here, it looks like two things are being combined: […]
There are multiple traits that differ based on what you are calling "Min CL supported", including tAAmin, tRCDmin, tRPmin, tRASmin, tRCmin, tCKmin, tCKmax, and CASLatencies. Without this Speed Bin categorization that they use in the Speed Bin tables, how would the other values be determined?
https://review.coreboot.org/c/coreboot/+/44429/9/util/spd_tools/intel/ddr4/R... PS9, Line 59: `0, 2, 4` bank groups.
Doesn't DDR4 support only 2/4 bank groups? (Reference: Section 3. […]
It supports 0 bank groups.
Section 8.1.5 of JEDEC spec 4.1.2.L-5 R29 has 00 = 0 (no bank groups), 01 = 1 (2 bank groups), 10 = 2 (4 bank groups), and 11 = (reserved) as options for SPD byte 4.
https://review.coreboot.org/c/coreboot/+/44429/9/util/spd_tools/intel/ddr4/R... PS9, Line 62: `4, 8` banks per group.
Doesn't DDR4 support only 4 banks per group? (Reference: Section 3. […]
Section 8.1.5 of JEDEC spec 4.1.2.L-5 R29 has 00 = (4 banks), 01 = (8 banks), all others reserved as options for SPD byte 4.
https://review.coreboot.org/c/coreboot/+/44429/9/util/spd_tools/intel/ddr4/R... PS9, Line 65: 32
Is 32Gb really supported per die for DDR4?
Section 8.1.5 of JEDEC spec 4.1.2.L-5 R29 states that DDR4 SPD byte 4 supports options for 256 Mb all the way to 32 Gb.
https://review.coreboot.org/c/coreboot/+/44429/9/util/spd_tools/intel/ddr4/R... PS9, Line 65: 1,
Isn't the minimum capacity per die for DDR4 is 2Gb?
See above comment.
https://review.coreboot.org/c/coreboot/+/44429/9/util/spd_tools/intel/ddr4/R... PS9, Line 67: `pageSize`
I think this can be determined based on the deviceBusWidth i.e. […]
Excellent tip, I'll implement it.
https://review.coreboot.org/c/coreboot/+/44429/9/util/spd_tools/intel/ddr4/R... PS9, Line 73: 1, 2, 3, 4, 5, 6, 7, 8`
Doesn't DDR4 support only single or dual dies per package?
Section 8.1.7 of JEDEC spec 4.1.2.L-5 R29 has a setting for anywhere from a single die to 8 die.
https://review.coreboot.org/c/coreboot/+/44429/9/util/spd_tools/intel/ddr4/R... PS9, Line 75: Number of bits of the device's address bus.
Isn't this the column width / DQ width? And the valid values would be 4, 8 and 16?
Is device bus width not the same as DQ width? If not, what is the term that should be used for this attribute?
We won't be supporting 4-bit (would have to be a 4-tall stack on Volteer, we'll only support a 2-tall stack). The code currently only supports x8 and x16 as options, I will update this readme to match.
https://review.coreboot.org/c/coreboot/+/44429/9/util/spd_tools/intel/ddr4/R... PS9, Line 78: signalLoading`: Refers to loading on signals at the SDRAM balls. Loading on : certain signals (CKE, ODT, etc.) per specification of device stacking as : defined in JESD79-4CNumber of bits of the device's address bus. Valid values: : `"not specified", "single", "multi"`
My understanding is that DDR4 supports only single and dual die packages. […]
Will we support 3DS? If not, then you are correct, single die could only map to "not specified". so we could determine this based on die count.
https://review.coreboot.org/c/coreboot/+/44429/9/util/spd_tools/intel/ddr4/R... PS9, Line 89: `1, 2, 3, 4` package ranks.
Isn't only 1 and 2 supported here?
Section 8.1.13 of JEDEC spec 4.1.2.L-5 R29 has a setting for anywhere from 1 package ranks to 8 package ranks. My comment on line 83 states that rev 23 supported 4 package ranks, but I see in the newer R29 spec that it was updated to support 8 package ranks.
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 31: PlatformJSL = 1
This is not required.
Done
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 91: 1: 0x2,
I don't think DDR4 supports this.
According to Table 18 in section 8.1.5 of the JESD 4.1.2.L-5 R29 spec, it supports 256Mb, 512Mb, and 1GB capacity per die, but I didn't include support for 256Mb or the 512Mb parts. Should I also not support or allow 1 Gb parts?
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 96: 32: 0x7,
This is not supported by DDR4.
Section 8.1.5 in Jedec Spec 4.1.2.L-5 R29 shows a "32Gb" option for "Total SDRAM capacity per die". Should I not allow a 32 Gb option?
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 101: density per physical channel
density per die?
Done
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 104: Channel
die?
made it "DieCapacity"
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 141: const ( : /* SDRAM Configuration Types */ : SDRAMConfig256Mbx8 = 0 : SDRAMConfig512Mbx8 = 1 : SDRAMConfig1Gbx8 = 2 : SDRAMConfig2Gbx8 = 3 : SDRAMConfig128Mbx16 = 4 : SDRAMConfig256Mbx16 = 5 : SDRAMConfig512Mbx16 = 6 : SDRAMConfig1Gbx16 = 7 : )
Not used anywhere. I don't think this is required.
Done
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 154: /* SPEED Bin Grades */
+1. Most of the datasheets I read did not explicitly state the speed grade sub category. […]
Done
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 223: memAttributes
Rather than using memAttributes, I think we can use a structure with only the fields that have to be […]
Done
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 225: TAAMinPs: 12500,
Do we really need to store this in the table? I think this can be calculated based on two things: […]
Done
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 226: TRCDMinPs: 12500,
TRcd is the same as Taa for all combinations. Again, I don't think we need to store this separately.
Done
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 227: TRPMinPs: 12500,
Same for Trp as well. It is the same as Taa.
Done
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 229: TRCMinPs
Trc is just the sum of Taa and TRAS. So, it does not need to be stored separately.
Done
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 230: TCKMinPs
TckMin is calculated using speed bin. i.e. 2/speed.
Done
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 468: 8: 1 << 4,
DDR4 supports 4 banks only. This table is not really required.
JEDEC spec 4.1.2.L-5 R29, section 8.1.5 shows SPD options for [5:4] 00 (4 banks) and [5:4] 01 (8 banks)
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 472: 0: 0 << 6,
DDR4 does not support this.
JEDEC spec 4.1.2.L-5 R29, section 8.1.5 shows SPD options for bank group bits [7:6] : 00 (no bank groups), 01 (2 bank groups), 10 (4 bank groups)
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 490: densityGbx8x16ChannelToRowColumnEncoding
What about x4?
We do not support x4 as it would require too many pieces of ddr4.
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 495: encodePackage
encodePackageType?
encodePackageType() is the one that encodes the entire SPD byte and is defined below. This function only encodes what the 4.1.2.L-5 R29 spec calls bit 7, "Primary SDRAM Package Type", but since they named the bit the same as the entire byte, I shorted the bit name in this case and used encodePackage().
Do you have a better suggestion for a name?
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 518: var signalLoadingEncode = map[string]byte { : "not specified": 0, : "single": 1, : "multi": 2, : } :
This is not required. If die count is 1, then "not specified", else "multi".
Now that decision has been made to not support 3DS< that is true. I removed SignalLoading required json parameters.
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 533: encodePackage
This is called here as well as in encodeDiesPerPackage.
Removed encodeDiesPerPackage().
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 539: (bitWidthPerDevice / 8)
This is not really correct. For 32, it is 3 and not 4. […]
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 think that is just an example. We need to be looking at Table 169, 170 in JESD79-4c. […]
I will update this. Leaving this comment unresolved until I do.
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 725: 3DS
Isn't 3DS used only in soldered DIMM and not for memory down?
We will not support 3DS.
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,
Same as above.
I will update this. Leaving this comment unresolved until I do.
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 1165: TGL
What about other platforms?
First version of this tool only supports TGL. Once format of tool is approved and initial version of tool merged, additional platforms could be added.
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 1374: bankGroups != 0
This is not correct.
The Jedec DDR4 SPD Spec 4.1.2.L-5 R29, section 8.1.5 has a bit setting option for bits 7:6 that includes a setting option for "no bank groups". Should I not allow support or allow "no bank groups"?
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 1381: && banksPerGroup != 8
This is not correct.
The Jedec DDR4 SPD Spec 4.1.2.L-5 R29, section 8.1.5 has a bit setting option for bits 5:3 that include "00 = 4 banks, 01 = 8 banks, all others reserved". Should I not support or allow 8 banks per group?
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 1388: switch capacityPerDieGb {
Instead of switch: […]
Done
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 1413: dieCount <= 8
I don't think there can be more than 2 dies.
The SPD spec options support up to 8. Should I limit support of this tool to 2 die max?
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 1420: width != 8
I think 4 is valid too?
Yes, x4 is valid, but they would have to stack too many DDR4 parts to use x4 parts (16 DDR4 parts soldered down), so the tool doesn't support it as an option. Should it?
https://review.coreboot.org/c/coreboot/+/44429/11/util/spd_tools/intel/ddr4/... PS11, Line 1434: ranks <= 4
Isn't this limited to 2?
Section 8.1.13 of SPD spec shows options in bits 5:3 that support from 1 to 8 package ranks, inclusive. Originally, I was thinking that max ranks per package seems to be 2 (from the DDR4 part spec sheets I've seen), but if they were stacked 2-high, I was thinking we could get 4. But thinking about it more, they would need to connect the ranks for the stacked parts such that the two stacked parts shared the same 2 ranks, so I think you are correct that we would likely not see ranks > 2.
I reduced upper bound to ranks <= 2.