Subrata Banik 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 d […]
Ack
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/in... File src/drivers/intel/fsp2_0/include/fsp/util.h:
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/in... PS9, Line 24: enum fsp_multi_phase_action { : GET_NUMBER_OF_PHASES, : EXECUTE_PHASE : };
This is part of an ABI, it would be good to explicitly call out the integer value of each enum.
Ack
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 45: erros
errors
Ack
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/si... PS9, Line 46: !=
This isn't the correct way to test for success anyway. It should be if(!EFI_ERROR(status)).
We don't use those EFI_ERROR handling case in coreboot.
It is possible to have "successful" error codes other than EFI_SUCCESS. The MSB indicates error.
Right now its mostly 5 return types as per spec
EFI_SUCCESS FSP execution environment was initialized successfully. EFI_INVALID_PARAMETER Input parameters are invalid. EFI_UNSUPPORTED The FSP calling conditions were not met. EFI_DEVICE_ERROR FSP silicon initialization failed. FSP_STATUS_RESET_REQUIRED_* A reset is required. These status codes will not be returned during S3.
https://review.coreboot.org/c/coreboot/+/41728/9/src/drivers/intel/fsp2_0/si... handles FSP_STATUS_RESET_REQUIRED_* return type case
EFI_SUCCESS been checked here
Rest everything is error code which results into die in coreboot code
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
Ack
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_FS […]
Ack
-
Subrata Banik (Code Review)