Paul Fagerburg 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/
Done
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: […]
Done
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.
Yes, it has to run inside the chroot. Added code to detect the chroot and exit if we're not in the chroot.
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 t […]
I'll work on this. It's a combination of finding the files in a template directory local to the script, then stripping off a common prefix on the path name so I can create the new directory structure and copy_tmpl into that directory.
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... PS12, Line 97: KConfig
Kconfig -- lowercase c
Done
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... PS12, Line 97: KConfig.name
Kconfig. […]
Done
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.
Next version. I added it to the bug so we don't lose track.
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?
Not in this round, no. Part of the issue is that the human has to scan the output to see if the new variant got built. I've added to the bug so we don't lose track.
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?
Done
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... PS12, Line 5: EC
coreboot?
Done
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... PS12, Line 44:
Do we need 3 blank lines here?
Old coding habits die hard. Changed to 2 lines per style guide.
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.
Changed all 'kindred' to 'kohaku'. My notes from manually doing this has all the CLs for kindred, but I've used kohaku in other places, hence the inconsistency.
https://review.coreboot.org/c/coreboot/+/35239/12/util/mainboard/google/hatc... PS12, Line 73:
Are 3 blank lines required?
Done
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. […]
I'm not aware of a tool that will let me read in a Kconfig, walk the structure to find the node and add the value, and then write it out again. I suggest that this will work for now, and we can focus on changing from append to insert-sorted in a new rev. I did add this to the bug so we don't lose track of it.
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 […]
Same comment as above.