Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35277 )
Change subject: soc/intel/{common,skl,icl,cnl}: Move some bootblock code to common location ......................................................................
Patch Set 3:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35277/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35277/3//COMMIT_MSG@7 PS3, Line 7: skl,icl,cnl I don't see apl in here. Is that intentional? What is your plan on enabling platforms other than big core? Do they fit into this model?
https://review.coreboot.org/c/coreboot/+/35277/3/src/soc/intel/common/baseco... File src/soc/intel/common/basecode/bootblock/Kconfig:
https://review.coreboot.org/c/coreboot/+/35277/3/src/soc/intel/common/baseco... PS3, Line 5: all Is that true?
https://review.coreboot.org/c/coreboot/+/35277/3/src/soc/intel/common/baseco... File src/soc/intel/common/basecode/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/35277/3/src/soc/intel/common/baseco... PS3, Line 18: soc/bootblock.h So, every SoC is expected to provide the declarations for the functions?
https://review.coreboot.org/c/coreboot/+/35277/3/src/soc/intel/common/baseco... PS3, Line 19: soc/pch.h Why is this included?
https://review.coreboot.org/c/coreboot/+/35277/3/src/soc/intel/common/baseco... PS3, Line 20: If a new platform decides to use this functionality, what are the functions that it is expected to provide? It is not clear at all from this change what is used from common/block and what is expected to be provided by SoC.
https://review.coreboot.org/c/coreboot/+/35277/3/src/soc/intel/common/baseco... PS3, Line 30: bootblock_pch_early_init This assumes that CONFIG_SOC_INTEL_COMMON_BLOCK_SA is selected by the SoC. Do you plan to auto select that based on the config selection? Similarly, whats the plan for other CONFIG_* that are assumed to be selected?