Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41612 )
Change subject: util: Add spd_tools to generate SPDs for TGL boards ......................................................................
Patch Set 8: Code-Review+1
(6 comments)
https://review.coreboot.org/c/coreboot/+/41612/3/util/mainboard/google/spd_t... File util/mainboard/google/spd_tools/lp4x/gen_spd.go:
https://review.coreboot.org/c/coreboot/+/41612/3/util/mainboard/google/spd_t... PS3, Line 280: Intel I only see this mention of Intel. Which fields are non-conforming to the JEDEC spec?
https://review.coreboot.org/c/coreboot/+/41612/3/util/mainboard/google/spd_t... PS3, Line 280: /* Unsure about this ??? Check with Intel!!! */ Is there a bug filed to get this info?
https://review.coreboot.org/c/coreboot/+/41612/3/util/mainboard/google/spd_t... PS3, Line 510: "/" os.PathSeparator
https://review.coreboot.org/c/coreboot/+/41612/3/util/mainboard/google/spd_t... PS3, Line 617: func validateChannels(channels int) { What is a channel in LPDDR4x? Likewise, what does datawidth represent? Per die?
https://review.coreboot.org/c/coreboot/+/41612/3/util/mainboard/google/spd_t... PS3, Line 700: encodeLatencies(latency, &memAttribs.casFirstByte, &memAttribs.casSecondByte, &memAttribs.casThirdByte) why not just pass the object pointer?
https://review.coreboot.org/c/coreboot/+/41612/3/util/mainboard/google/spd_t... PS3, Line 801: if generateSpd(dedupedParts, &memParts.MemParts[i], &spdId) == true { You could just return the new spdId value. Or just pull the dedupe check into its own function and call it from here.