Attention is currently required from: Sean Rhodes, Martin Roth. Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61620 )
Change subject: payloads/tianocore: Rework Makefile ......................................................................
Patch Set 21:
(8 comments)
File payloads/external/tianocore/Kconfig:
https://review.coreboot.org/c/coreboot/+/61620/comment/1a0cbec6_028259d4 PS20, Line 5: $(obj) how does this resolve to payloads/external/tianocore/output ?
https://review.coreboot.org/c/coreboot/+/61620/comment/5a7fae03_701ce88b PS20, Line 9: TIANOCORE_REPOSITORY worth adding 9elements and/or System76's repos (possibly in a follow-up commit)?
https://review.coreboot.org/c/coreboot/+/61620/comment/3349b238_5d3b8236 PS20, Line 12: TIANOCORE_UPSTREAM should be TIANOCORE_COREBOOTPAYLOAD
https://review.coreboot.org/c/coreboot/+/61620/comment/e730b052_77ae23f0 PS20, Line 113: config TIANOCORE_BOOT_MANAGER_ESCAPE : bool "Use Escape key for Boot Manager" : default n : help : Use Escape as the hot-key to access the Boot Manager. This replaces : the default key of F2. add this and other new Kconfig / build params in a follow-on commit?
File payloads/external/tianocore/Makefile:
https://review.coreboot.org/c/coreboot/+/61620/comment/88ab22d8_e6155223 PS20, Line 9: BUILD_STR = -q : BUILD_STR += -a IA32 -a X64 -t COREBOOT why set these on separate lines? Also, worth adding a toggle for the -q so compilation errors can be more easily debugged without manual editing?
https://review.coreboot.org/c/coreboot/+/61620/comment/76e778de_68eebaed PS20, Line 22: # OPTION = DEFAULT_VALUE : # : # ABOVE_4G_MEMORY = TRUE : ifneq ($(CONFIG_TIANOCORE_ABOVE_4G_MEMORY),y) : BUILD_STR += -D ABOVE_4G_MEMORY=FALSE : endif : # BOOTSPLASH_IMAGE = FALSE : ifneq ($(CONFIG_TIANOCORE_BOOTSPLASH_FILE),) : BUILD_STR += -D BOOTSPLASH_IMAGE=TRUE : endif : # BOOT_MANAGER_ESCAPE = FALSE : ifeq ($(CONFIG_TIANOCORE_BOOT_MANAGER_ESCAPE),y) : BUILD_STR += -D BOOT_MANAGER_ESCAPE=TRUE : endif : # BUILD_TARGETS = RELEASE : ifeq ($(CONFIG_TIANOCORE_DEBUG),y) : BUILD_STR += -b DEBUG : endif : # FOLLOW_BGRT_SPEC = FALSE : ifeq ($(CONFIG_TIANOCORE_FOLLOW_BGRT_SPEC),y) : BUILD_STR += -D FOLLOW_BGRT_SPEC=TRUE : endif : # PS2_KEYBOARD_ENABLE = FALSE : ifeq ($(CONFIG_TIANOCORE_PS2_SUPPORT),y) : BUILD_STR += -D PS2_KEYBOARD_ENABLE=TRUE : endif : # PLATFORM_BOOT_TIMEOUT = 3 : ifneq ($(TIANOCORE_BOOT_TIMEOUT),) : BUILD_STR += -D PLATFORM_BOOT_TIMEOUT=$(CONFIG_TIANOCORE_BOOT_TIMEOUT) : endif : # SIO_BUS_ENABLE = FALSE : ifeq ($(CONFIG_TIANOCORE_PS2_KEYBOARD),y) : BUILD_STR += -D SIO_BUS_ENABLE=TRUE : endif : # SHELL_TYPE = BUILD_SHELL : ifneq ($(CONFIG_TIANOCORE_HAVE_EFI_SHELL),y) : BUILD_STR += -D SHELL_TYPE=NONE : endif : # USE_CBMEM_FOR_CONSOLE = FALSE : ifeq ($(CONFIG_TIANOCORE_CBMEM_LOGGING),y) : BUILD_STR += -D USE_CBMEM_FOR_CONSOLE=TRUE : endif I'm not sure leaving these unset for the default is ideal, since if the repo/branch being built from changes, the default could as well. I think it better that coreboot always explicitly set these.
https://review.coreboot.org/c/coreboot/+/61620/comment/429a0648_fa3edb8f PS20, Line 146: */
As it is now variable, if you change config's halfway through, the original directory wouldn't be re […]
I'm good with it either way, but I'd lean towards distclean should remove everything not part of the coreboot repo (so keeping the change)
File src/mainboard/starlabs/labtop/Kconfig:
https://review.coreboot.org/c/coreboot/+/61620/comment/9ce2be52_32599f36 PS20, Line 84: depends on TIANOCORE_BOOTSPLASH_IMAGE separate commit since outside the scope of payloads/ ?
-
Matt DeVillier (Code Review)