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 21:
(20 comments)
https://review.coreboot.org/c/coreboot/+/44429/19/util/spd_tools/intel/ddr4/... File util/spd_tools/intel/ddr4/README.md:
https://review.coreboot.org/c/coreboot/+/44429/19/util/spd_tools/intel/ddr4/... PS19, Line 82: picosecond units, accept for "CASLatencies", which would be represented as a
except
Done
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 100: 61
61 is only for RFC2
Done
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 134: 2000000
This is 2000000 because it is basically 2 * 1000000 where 1000000 is to convert from milliseconds to […]
Done
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 227: var loading byte : : /* : * If die count = 1, signal loading = "not specified" = 0 : * If die count > 1, signal loading = "multi" = 2 : */ : if dies == 1 { : loading = 0 : } else { : loading = 1 : } : : return loading
nit: This can be moved in encodeSignalLoadingFromDieCount() since it doesn't really do anything else […]
Done
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 348: tRCD is same as tAA for all combinations
This is not right. […]
Agreed. It was left over from a prior implementation before I realized it should probably be overridable. Fixed.
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 353: /* tRCD is same as tAA for all combinations */
Same as above.
Done
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 444: error
This function is always returning nil. […]
Done
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 452: error
same here
Done
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 460: error
same here.
Done
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 468: error
same here.
Done
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 503: error
Same here.
Done
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 513: 4284
I see a value of 4.2ns in Table 169 which would be 4200ps. […]
tRRD_S(1K) = Max(4nCK,4.2ns), where CK=1.071, so max = 4 * 1.071 = 4.284ns
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 514: 4284
From table 169 in JESD79-4C, this is 3. […]
Thanks for the catch !! I was looking at the wrong table value on this, should not have been 4284. It should be Max(4nCK,3.7ns) = 4 * 0.937 = 3.748nS
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 539: error
Same comment as above about not requiring any return value for the next two functions.
Done
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 1101: CASSecondByte
CASThirdByte
Great catch !
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 1112: CL_nRCD_nRP - <first_CL_in_speed_series> + <total_enties_prior>
Do we really need this? It just seems more complicated than required. […]
Done
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 1137: "9 11 12",
There is one extra entry here which will result in this function returning wrong values for all the […]
I went through entire table to comment it based on Rob's review request, found that and another issue in the table in doing so. I since abandoned the comments requested by Rob in lieu of the alternate approach of switches of switches, which no longer required the comments.
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 1188: error
All these functions returning nil can simply drop the return type.
Done
https://review.coreboot.org/c/coreboot/+/44429/19/util/spd_tools/intel/ddr4/... File util/spd_tools/intel/ddr4/gen_spd.go:
https://review.coreboot.org/c/coreboot/+/44429/19/util/spd_tools/intel/ddr4/... PS19, Line 1133: func getCASLatencies(memAttribs *memAttributes) string {
I suggest adding a validator to check if the override casLatencies matches the default casLatencies […]
Good suggestion.
https://review.coreboot.org/c/coreboot/+/44429/19/util/spd_tools/intel/ddr4/... PS19, Line 1149: "9 10 11 12 13 14 15 16 17 18 19 20",
nit: adding a comment with the index or speed bin would be helpful here.
Good suggestion. Done.