Attention is currently required from: Andrey Petrov, Bora Guvendik, Dinesh Gehlot, Eran Mitrani, Jakub Czapiga, Jérémy Compostella, Kapil Porwal, Nick Vaccaro, Ronak Kanabar, Tarun, Wonkyu Kim.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80275?usp=email )
Change subject: drivers/intel/fsp2_0: Add limited to 32-bits FSP 2.4 support ......................................................................
Patch Set 6:
(5 comments)
Patchset:
PS3:
quick feedback
- This CL doesn't mentioned the test vehicle used to verify
Test vehicle is not public. Considering the 2.4 specification is public, we can provide the implementation.
it doesn't matter but a TEST statement is basic req for any CL to land. Like I'm able to test this CL on some XYZ platform.
- specify the FSP 2.4 spec link
I gave the document number but I did not add the link as links can change.
There are two doc numbers in the commit msg hence, I'm unable to understand which one is the spec.
File src/drivers/intel/fsp2_0/Kconfig:
https://review.coreboot.org/c/coreboot/+/80275/comment/76a035f2_9bc0a445 : PS3, Line 58: default n if PLATFORM_USES_FSP2_4
2.4 also supports 32-bits.
if FSP 2.4 supports both 32/64bit execution hence, I'm unable to follow how one can still select `PLATFORM_USES_FSP2_X86_32` when PLATFORM_USES_FSP2_4 is enabled ?
I had an impression that FSP2.4 enforces only 64-bit execution
I can see patchset 6 fixes this problem.
File src/drivers/intel/fsp2_0/memory_init.c:
https://review.coreboot.org/c/coreboot/+/80275/comment/c7bbd33e_3bfe2a68 : PS3, Line 282: error_handler
Considering we are in memory_init.c and this is static function I don't see the benefit of surcharging the function name with unnecessary prefix.
the `error_hanlder` function name is very ambiguous as it doesn't clarify the intention of calling this function. adding `fsp_` prefix at minimum is req to make this complete
https://review.coreboot.org/c/coreboot/+/80275/comment/8809c7c1_a73eac5d : PS3, Line 297: multi_phase_init
Considering we are in memory_init.c and this is static function I don't see the benefit of surcharging the function name with unnecessary prefix.
this to improve the code readability also, https://review.coreboot.org/c/coreboot/+/80275/comment/01d3d332_2dba7f2e/
https://review.coreboot.org/c/coreboot/+/80275/comment/608cd31b_70929ec3 : PS3, Line 437: if (hdr->fsp_multi_phase_mem_init_entry_offset)
Wouldn't it be dangerous to look outside after the limit of the data structure ? These fields are added with 2.4.
in that case, check the FSP version index and call into the `fsp_multi_phase_init`. preprocessor macro is not required here IMO and we don't use such unless absolute necessary.