Patrick Rudolph 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:
(4 comments)
https://review.coreboot.org/c/coreboot/+/18457/37//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/18457/37//COMMIT_MSG@11 PS37, Line 11: flexibility I don't see that achieved. As the sequence is hardcoded and no functions are optional there's no flexibility.
https://review.coreboot.org/c/coreboot/+/18457/37//COMMIT_MSG@49 PS37, Line 49: custom That reads as everything introduced is optional, and SoC maintainers can do whatever they want.
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 * Why are all ops mandatory? * Why are you using function pointers? That prevents the linker from doing garbage collection * Using runtime checks prevent build time errors, that's likely not what we want, as all functions are mandatory * Giving SoC maintainers flexibility in naming scheme is likely not what we want. It will be impossible to find the corresponding SoC function in an IDE * There's only a single soc at time, so why use function pointers at all?
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?
-
Patrick Rudolph (Code Review)