Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41728 )
Change subject: drivers/intel/fsp2_0: Add FSP 2.2 specific support ......................................................................
Patch Set 9:
(6 comments)
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/in... File src/drivers/intel/fsp2_0/include/fsp/info_header.h:
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/in... PS9, Line 33: #if CONFIG(PLATFORM_USES_FSP2_2) this doesn't seem to be used as a packed structure (each field read separately in fsp_identify) so does this need to be #ifdef?
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/si... File src/drivers/intel/fsp2_0/silicon_init.c:
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/si... PS9, Line 46: != if you made this == and returned here the indent could be removed and strings wouldn't need to be awkwardly split.
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/si... PS9, Line 59: FSP_MULTI_PHASE_SI_INIT_GET_NUMBER_OF_PHASES_API Should we attempt to continue to boot without multiphase in this case rather than treat this as fatal?
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/si... PS9, Line 66: " prefix with "FspMultiPhaseSiInit ExecutePhase" (for consistency with other output)
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/si... PS9, Line 146: (void *) I don't think you need to cast here
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/ut... File src/drivers/intel/fsp2_0/util.c:
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/ut... PS9, Line 52: #if CONFIG(PLATFORM_USES_FSP2_2) if the header didn't use #if to guard this field the code could just be "if (CONFIG(PLATFORM_USES_FSP2_2))" instead of using #if
-
Duncan Laurie (Code Review)