Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41612 )
Change subject: util/mb/google: Add spd_tools to generate SPDs for TGL boards ......................................................................
Patch Set 4:
(18 comments)
Patch Set 2:
(4 comments)
Please add it under top level `util`.
Done.
Does the coreboot SDK need to be updated to provide a Go lang toolchain?
I don't think so. We already have support for that.
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... File util/mainboard/google/spd_tools/lp4x/README.md:
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 4: Intel Tiger Lake(TGL)
Please add a space before (.
Done
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 6: recommendations(doc #616599)
Ditto.
Done
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 134: ```
yeah, but this is JSON, so does that apply?
I think it is easier to read even in the MD format with the ```: https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/changes/12/41612...
I am thinking of keeping this as is.
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... File util/mainboard/google/spd_tools/lp4x/gen_part_id.go:
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 35: if *spdDirPtr == "" {
if a flag is not optional, I think it's not really a flag. […]
Done
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 64: func readParts() []string {
so here, for example: […]
Done
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 83: func readSpdManifest() (map[string]string, map[string]int) {
func readSpdManifest() (map[string]string, map[string]int, error)
Done
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 95: for true {
you can just say for […]
Done
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 145: for i := 0; i < len(parts); i++ {
for i := range parts […]
Done
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 151: if ok == false {
if ! ok { […]
Done
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 181: filePath := *makefileDirPtr + "/" + makefileName
The Go convention is all errors should be checked, and you are missing many here. […]
Done. Thanks that was very helpful.
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... File util/mainboard/google/spd_tools/lp4x/gen_spd.go:
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 5: import "bufio"
same here, fix imports
Done
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 63: func encodeSpdSize(memAttribs *memAttributes) byte {
can't these just be const?
I went back and forth a little on this and decided to use functions to keep all byte handling consistent. I have changed these to const now.
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 510: filePath := *spdDirPtr + "/" + spdManifestFileName
always use filepath.Join […]
Done
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 546: filePath := *spdDirPtr + "/" + fileName
always use filepath. […]
Done
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 592: partsFile, err := os.Open(*memPartListPtr)
you could, also, just use ioutil.ReadFile […]
Done
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 605: func validateDensity(densityPerChannelGb int) {
return an error, then writing tests is easier
Done
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 770: if *spdDirPtr == "" {
I'd still argue fore positional arguments but...
Done
https://review.coreboot.org/c/coreboot/+/41612/2/util/mainboard/google/spd_t... PS2, Line 794: readMemoryParts(&memParts)
here is where you'd have […]
Done