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 19:
(22 comments)
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.
https://review.coreboot.org/c/coreboot/+/44429/17/util/spd_tools/intel/ddr4/... PS17, Line 80: Do To?
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
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 picoseconds. It might be helpful to have a comment here.
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.
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. You already ensure that TRCDMinPs is set to TAAMinPs if no value provided for TRCDMinPs in json file. Here, you should be using convPsToMtbByte(memAttribs.TRCDMinPs)
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.
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 444: error This function is always returning nil. So, is a return type really required?
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 452: error same here
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 460: error same here.
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 468: error same here.
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 503: error Same here.
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. Is 4284 in a different part of the document?
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.7ns
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.
https://review.coreboot.org/c/coreboot/+/44429/18/util/spd_tools/intel/ddr4/... PS18, Line 1101: CASSecondByte CASThirdByte
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.
It would be much easier to read having cases for each CL_nRCD_nRP under each speed grade: ``` switch memAttribs.SpeedMTps { case 1600: switch memAttribs.CL_nRCD_nRP { case 10: return "9 10 11 12" case 11: return "9 11 12" case 12: return "10 12" } case 1866: .... ```
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 entries henceforth.
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.
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:
I don't think this is correct. From the speed bin table at the end of the datasheet, it looks like this part supports all the CAS Latencies as mentioned in the spec. Also, the initial SPD that you had checked in for the part https://review.coreboot.org/c/coreboot/+/44272/1/src/mainboard/google/voltee... shows that all CAS Latencies are supported.
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 3200 (22-22-22)
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.