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.
5 comments:
Patchset:
quick feedback
1. 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.
> 2. specify the FSP 2.4 spec linkI 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:
Patch Set #3, 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:
Patch Set #3, 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
Patch Set #3, 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/
Patch Set #3, 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.
To view, visit change 80275. To unsubscribe, or for help writing mail filters, visit settings.