Furquan Shaikh 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 9:
(12 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: /* Unsure about this ??? Check with Intel!!! */
Is there a bug filed to get this info?
I have asked for clarification here: https://b.corp.google.com/issues/147321551#comment34
https://review.coreboot.org/c/coreboot/+/41612/3/util/mainboard/google/spd_t... PS3, Line 280: Intel
I only see this mention of Intel. […]
This is byte 16 (signal loading).
https://review.coreboot.org/c/coreboot/+/41612/3/util/mainboard/google/spd_t... PS3, Line 510: "/"
os. […]
This is fixed in latest patchset.
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?
Done
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. […]
Done
https://review.coreboot.org/c/coreboot/+/41612/8/util/spd_tools/intel/lp4x/g... File util/spd_tools/intel/lp4x/gen_part_id.go:
https://review.coreboot.org/c/coreboot/+/41612/8/util/spd_tools/intel/lp4x/g... PS8, Line 28: SPD_DIR_ARG = 1
So sorry I did not notice this. Go frowns on this type of constant. […]
Done
https://review.coreboot.org/c/coreboot/+/41612/8/util/spd_tools/intel/lp4x/g... PS8, Line 44: func checkArgs() error {
Honest opinion here? I am not convinced you need this at all. […]
Done
https://review.coreboot.org/c/coreboot/+/41612/8/util/spd_tools/intel/lp4x/g... File util/spd_tools/intel/lp4x/gen_spd.go:
https://review.coreboot.org/c/coreboot/+/41612/8/util/spd_tools/intel/lp4x/g... PS8, Line 28: SPD_DIR_ARG = 1
same comment re const names
Done
https://review.coreboot.org/c/coreboot/+/41612/8/util/spd_tools/intel/lp4x/g... PS8, Line 333: SPD_INDEX_OPTIONAL_FEATURES = 7
Probably want to encode byte 9 SDRAM optiona features as 0x80 or 0xc0? They reflect as reserved. […]
As per what Intel mentioned, that byte is not really used by MRC: https://docs.google.com/spreadsheets/d/1LA4eoYuA1ANBX6N5-RC6UOSPQgeh4A0XcE5c...
https://review.coreboot.org/c/coreboot/+/41612/8/util/spd_tools/intel/lp4x/g... PS8, Line 335: SPD_INDEX_BUS_WIDTH = 13
I am not sure if none of the memory parts support Thermal sensor. […]
Byte 14 is not used by MRC: https://docs.google.com/spreadsheets/d/1LA4eoYuA1ANBX6N5-RC6UOSPQgeh4A0XcE5c...
https://review.coreboot.org/c/coreboot/+/41612/8/util/spd_tools/intel/lp4x/g... PS8, Line 425: SPD_VALUE_TCK_MAX = 0xff
Any impact of fixing on the max TCK? Is it a good candidate for optional attribute?
JEDEC spec says that max TCK should be 100ns. I see that all parts follow the same. I was thinking of keeping it as constant especially since we can't really encode 100ns in the SPD byte and instead have to stick to using 0xff i.e. 31.875ns. We can always make it optional if we see a part that needs a smaller value.
https://review.coreboot.org/c/coreboot/+/41612/8/util/spd_tools/intel/lp4x/g... PS8, Line 438: SPD_INDEX_SIZE: { constVal: SPD_VALUE_SIZE },
also you could do SPDIndexSize but OTOH maybe this is the hardware name, in which case you'd get a p […]
Done