Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39226 )
Change subject: soc/intel: Enable CSE Firmware Custom SKU for hatch ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39226/4/src/mainboard/google/hatch/... File src/mainboard/google/hatch/Kconfig:
PS4: Mainboard changes should be separated from SoC changes. Also, we don't really want to enable this for hatch right away. This should just be a Draft change for testing.
https://review.coreboot.org/c/coreboot/+/39226/4/src/soc/intel/cannonlake/Kc... File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/c/coreboot/+/39226/4/src/soc/intel/cannonlake/Kc... PS4, Line 87: SOC_INTEL_COMMON_BASECODE I don't think we should be adding cse related code to basecode. Please see my comment here: https://review.coreboot.org/c/coreboot/+/35403/67/src/soc/intel/common/basec...
https://review.coreboot.org/c/coreboot/+/39226/4/src/soc/intel/cannonlake/Kc... PS4, Line 144: : config INTEL_CSE_FW_SKU_CUSTOM : bool : default n : help : Enables CSE Firmware SKU Custom. I don't think this should be added to every Intel SoC. Instead it should be put in soc/intel/common/block/cse/Kconfig -- something like : SOC_INTEL_CSE_CUSTOM_SKU. And mainboards that use this SKU can select it accordingly.
https://review.coreboot.org/c/coreboot/+/39226/4/src/soc/intel/cannonlake/me... File src/soc/intel/cannonlake/me.c:
https://review.coreboot.org/c/coreboot/+/39226/4/src/soc/intel/cannonlake/me... PS4, Line 178: enable_cse_fw_sku_custom I don't think we need to add this to specific SoC. It can simply be put in soc/intel/common/block/cse/custom_sku.c
Thus we don't need to add an indirection in specific SoC.
https://review.coreboot.org/c/coreboot/+/39226/4/src/soc/intel/cannonlake/me... PS4, Line 183: BS_DEV_ENABLE What is the reason behind making this call during this specific boot state?