Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35239 )
Change subject: hatch: automate creating a new variant in coreboot ......................................................................
Patch Set 12:
(15 comments)
https://review.coreboot.org/c/coreboot/+/35239/11//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35239/11//COMMIT_MSG@7 PS11, Line 7: hatch util/
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... File util/mainboard/google/hatch/Makefile.inc.tmpl:
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... PS12, Line 15: SPD_SOURCES = 4G_2400 # 0b000 : SPD_SOURCES += empty_ddr4 # 0b001 : SPD_SOURCES += 8G_2400 # 0b010 : SPD_SOURCES += 8G_2666 # 0b011 : SPD_SOURCES += 16G_2400 # 0b100 : SPD_SOURCES += 16G_2666 # 0b101 Can we just leave these as: SPD_SOURCES =
Values here depend upon whether the variant chooses DDR4 v/s LPDDR3.
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... File util/mainboard/google/hatch/create_coreboot_variant.sh:
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... PS12, Line 51: repo start Is the assumption here that this is being run from chromeos chroot? That should be stated somewhere.
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... PS12, Line 73: copy_tmpl "${SRC}/dptf.asl.tmpl" \ : "variants/${variant}/include/variant/acpi/dptf.asl" : git add "variants/${variant}/include/variant/acpi/dptf.asl" : : copy_tmpl "${SRC}/ec.h.tmpl" "variants/${variant}/include/variant/ec.h" : git add "variants/${variant}/include/variant/ec.h" : : copy_tmpl "${SRC}/gpio.h.tmpl" "variants/${variant}/include/variant/gpio.h" : git add "variants/${variant}/include/variant/gpio.h" : : copy_tmpl "${SRC}/Makefile.inc.tmpl" "variants/${variant}/Makefile.inc" : git add "variants/${variant}/Makefile.inc" : : # overridetree.cb does not have any dates in it, so we can just copy : cp "variants/${base}/overridetree.cb" "variants/${variant}" : git add "variants/${variant}/overridetree.cb" This works, but do you think we could layout the files under template/ here in the same fashion as they should be under src/mainboard/google/${base} and do a find and replace __YEAR__ to ${YEAR} in all and just do a recursive copy of those files directly to src/mainboard/google/${base}. That way if there are more files added to templates, you wouldn't have to update the script. Also, makes it easy to adapt it to more boards?
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... PS12, Line 97: KConfig Kconfig -- lowercase c
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... PS12, Line 97: KConfig.name Kconfig.name -- lowercase c
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... PS12, Line 102: none It would be good to provide an option to user to specify a bug argument.
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... PS12, Line 103: util/abuild/abuild -p none -t google/${base} -x -a : make sure the build includes GOOGLE_${variant^^}" Is this validated by the script?
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... File util/mainboard/google/hatch/kconfig.py:
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... PS12, Line 2: EC Kconfig coreboot?
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... PS12, Line 5: EC coreboot?
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... PS12, Line 44: Do we need 3 blank lines here?
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... PS12, Line 62: 'KOHAKU TEST 1953' It is confusing that the example above states kindred and the one here provides HWID for kohaku.
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... PS12, Line 73: Are 3 blank lines required?
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... PS12, Line 78: wait until we get a blank line : (signalling the end of that section) nit: I generally like to see Kconfigs and their values ordered alphabetically. Just easier to read through the configs.
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... PS12, Line 130: only has a block at : the end Is it possible to do this alphabetically? I know we let HELIOS slip in out of order, but it would be good if we can avoid that.