Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35880 )
Change subject: src/soc/intel: FSP2.0 platforms: add mainboard option to skip graphics init ......................................................................
Patch Set 2:
(6 comments)
Started to review, but erm, sorry, this is a mess. There's a list in the commit message. Which is asking me to ask you to split it into separate commits.
https://review.coreboot.org/c/coreboot/+/35880/2/src/mainboard/supermicro/x1... File src/mainboard/supermicro/x10slm-f/Kconfig:
https://review.coreboot.org/c/coreboot/+/35880/2/src/mainboard/supermicro/x1... PS2, Line 22: select CPU_INTEL_HASWELL I doubt this board is using FSP at all.
https://review.coreboot.org/c/coreboot/+/35880/2/src/soc/intel/apollolake/ch... File src/soc/intel/apollolake/chip.c:
https://review.coreboot.org/c/coreboot/+/35880/2/src/soc/intel/apollolake/ch... PS2, Line 754: #if CONFIG(HAVE_FSP_GOP) We don't use preprocessor #if's unless necessary. Also, we know that it is selected...
https://review.coreboot.org/c/coreboot/+/35880/2/src/soc/intel/apollolake/ch... PS2, Line 757: RUN_FSP_GOP ...and this even covers HAVE_FSP_GOP too...
https://review.coreboot.org/c/coreboot/+/35880/2/src/soc/intel/apollolake/ch... PS2, Line 759: params->GtFreqMax = 2; Does it exist?
https://review.coreboot.org/c/coreboot/+/35880/2/src/soc/intel/apollolake/ch... PS2, Line 760: params->CdClock = 3; Seems like a random choice? and what APL device needs it this high?
https://review.coreboot.org/c/coreboot/+/35880/2/src/soc/intel/apollolake/ch... PS2, Line 761: } else { ...and why would we not run the else path without HAVE_FSP_GOP?