Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/18457 )
Change subject: soc/intel/common: Add bootblock common stage file ......................................................................
Patch Set 38:
(3 comments)
https://review.coreboot.org/c/coreboot/+/18457/37/src/soc/intel/common/basec... File src/soc/intel/common/basecode/bootblock/bootblock.c:
https://review.coreboot.org/c/coreboot/+/18457/37/src/soc/intel/common/basec... PS37, Line 39: bootblock_soc_early_init(void)
This reminds me on UEFI implementations that use function pointers everywhere […]
I think this is just to allow arbitrary function names. To allow that, I would just use thunk functions inside SoC-specific code:
void bootblock_soc_early_init(void) { bootblock_my_ultra_awesome_soc_early_init(); }
In the above example, things can be simplified by a rename. However, when multiple SoCs are supported with the same code, things get more complex. For instance, soc/intel/cannonlake supports Cannonlake, Coffeelake, Whiskeylake and Cometlake. The code on that SoC could be something like this:
void bootblock_soc_early_init(void) { switch (get_soc_generation()) { case SOC_GENERATION_CANNONLAKE: bootblock_cnl_early_init(); break; case SOC_GENERATION_COFFEELAKE: case SOC_GENERATION_WHISKEYLAKE: bootblock_cfl_whl_early_init(); break; case SOC_GENERATION_COMETLAKE: bootblock_cml_early_init(); break; default: die("%s: Unsupported SoC generation!", __func__); } }
This is a good idea if the same coreboot binary has to support more than one generation of SoC, as the specific generation can only be determined at runtime.
However, if only one generation is supported, it is known at build-time, so things can be improved even further using something like the variants mechanism for the mainboards. This allows building only the necessary files, and enables build-time checking.
Using build-time checking, the Makefiles sanity-check the selected Kconfig options and abort the build process if they are invalid. Also, if a SoC generation has not implemented the required functions, it will result in a linker error. And thanks to Jenkins, this will be checked on every change, so accidentally breaking something is harder :)
https://review.coreboot.org/c/coreboot/+/18457/37/src/soc/intel/common/basec... PS37, Line 46: die("%s has not been implemented by SoC", __func__); With build-time checking, these errors would just be linker errors.
https://review.coreboot.org/c/coreboot/+/18457/37/src/soc/intel/common/basec... PS37, Line 102: (CONFIG(PAGING_IN_CACHE_AS_RAM))
Why is there APL/GLK specific code here? Wasn't the idea to make it more flexible?
I think APL/GLK differ too much from the big core platforms to have common code everywhere. If the next little core SoCs also need this, I would then consider an abstraction, but not now.