Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 10:
(7 comments)
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... File src/soc/intel/icelake/Kconfig:
PS9: `HAVE_EXT_GFX` should be put into `src/device/Kconfig`, so it’s common between all boards.
It’s still not clear to me, why the option `VGA_ROM_RUN` is not enough for your use case? That means, the user can just configure the build as you want it to, right?
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 212: bool Is it selected by default or not?
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 213: select ALWAYS_LOAD_OPROM I guess Kconfig isn’t selecting this automatically when selecting selecting `ALWAYS_RUN_OPROM`?
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 216: select FRAMEBUFFER_SET_VESA_MODE Why does this need to be selected?
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/romst... File src/soc/intel/icelake/romstage/fsp_params.c:
PS9: If the changes to this file would be in a separate commit, could it submitted already?
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/romst... PS9, Line 40: /* Set IGD stolen size to 60MB. */ Off-topic: Why 60 MB? Shouldn’t this also be configurable?
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/romst... PS9, Line 52: dev = pcidev_on_root(PCH_DEV_SLOT_ESPI, 3); Please do this in a separate commit.