Marc Jones has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47560 )
Change subject: xeon_sp/cpx: Check FSP version for WW45 and die if version mismatch ......................................................................
Patch Set 1:
(4 comments)
This change is ready for review.
https://review.coreboot.org/c/coreboot/+/47560/1/src/soc/intel/xeon_sp/cpx/f... File src/soc/intel/xeon_sp/cpx/fsp.c:
https://review.coreboot.org/c/coreboot/+/47560/1/src/soc/intel/xeon_sp/cpx/f... PS1, Line 8: union { : uint32_t val; : struct { : uint8_t bld_num; : uint8_t revision; : uint8_t minor; : uint8_t major; : } rev; : } revision; this is duplicate code and should be defined in fsp/util.h. It is used in fsp/util.c and should be updated there as well.
https://review.coreboot.org/c/coreboot/+/47560/1/src/soc/intel/xeon_sp/cpx/f... PS1, Line 19: spec_version How about do the shift in the preprocessor and have a single check for major and minor version. #define CPX_SPEC_VER ...
if (hdr->spec_version != CPX_SPEC_VERSION) { print... mismatch... }
You can probably do similar for the checks below. It is easy to see what is wrong when it is printed.
https://review.coreboot.org/c/coreboot/+/47560/1/src/soc/intel/xeon_sp/cpx/f... PS1, Line 53: Print out the version.
https://review.coreboot.org/c/coreboot/+/47560/1/src/vendorcode/intel/fsp/fs... File src/vendorcode/intel/fsp/fsp2_0/cooperlake_sp/FspmUpd.h:
https://review.coreboot.org/c/coreboot/+/47560/1/src/vendorcode/intel/fsp/fs... PS1, Line 73: #define CPX_REV_BUILD 45
Instead of: […]
This should go in a new file version.h and have an explanation on the version scheme for this fsp.