Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37582 )
Change subject: drivers/intel/fsp2_0: Make FSP_USE_REPO an SOC opt-in instead of list dependency ......................................................................
Patch Set 6: Code-Review+1
(10 comments)
https://review.coreboot.org/c/coreboot/+/37582/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37582/6//COMMIT_MSG@7 PS6, Line 7: SOC nit: I usually see SoC
https://review.coreboot.org/c/coreboot/+/37582/6//COMMIT_MSG@7 PS6, Line 7: drivers/intel/fsp2_0: Make FSP_USE_REPO an SOC opt-in instead of list dependency
Summary has 50 characters limit
Can remove "instead of list dependency"
https://review.coreboot.org/c/coreboot/+/37582/6//COMMIT_MSG@10 PS6, Line 10: ee Only one `e`
https://review.coreboot.org/c/coreboot/+/37582/6//COMMIT_MSG@11 PS6, Line 11: prune typo: pr*o*ne
https://review.coreboot.org/c/coreboot/+/37582/6//COMMIT_MSG@13 PS6, Line 13: is introducing Use the same tense as the commit summary: introduces
https://review.coreboot.org/c/coreboot/+/37582/6//COMMIT_MSG@14 PS6, Line 14: g trailing `g`
https://review.coreboot.org/c/coreboot/+/37582/6/src/soc/intel/Kconfig File src/soc/intel/Kconfig:
https://review.coreboot.org/c/coreboot/+/37582/6/src/soc/intel/Kconfig@53 PS6, Line 53: "Use FSP Blobs from fsp submodule" This shouldn't have a prompt
https://review.coreboot.org/c/coreboot/+/37582/6/src/soc/intel/Kconfig@55 PS6, Line 55: This SOC has FSP binaries living in 3rdparty/fsp. How about:
Select this if the platform has FSP binaries are publicly available in 3rdparty/fsp.
https://review.coreboot.org/c/coreboot/+/37582/6/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/37582/6/src/soc/intel/cannonlake/Kc... PS6, Line 68: select HAVE_INTEL_FSP_REPO
You also could just use `if !SOC_INTEL_CANNONLAKE`
Not sure if CannonLake just uses the CoffeeLake FSP package, or if it is not supported at all
https://review.coreboot.org/c/coreboot/+/37582/6/src/soc/intel/xeon_sp/Kconf... File src/soc/intel/xeon_sp/Kconfig:
https://review.coreboot.org/c/coreboot/+/37582/6/src/soc/intel/xeon_sp/Kconf... PS6, Line 41: select HAVE_INTEL_FSP_REPO Um, I don't think so.