[M] Change in coreboot[main]: drivers/intel/fsp2_0: Add limited to 32-bits FSP 2.4 support

[Subrata Banik (Code Review)]

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
> > 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 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.



-- 
To view, visit https://review.coreboot.org/c/coreboot/+/80275?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: main
Gerrit-Change-Id: I1c24d26e105c3dcbd9cca0e7197ab1362344aa97
Gerrit-Change-Number: 80275
Gerrit-PatchSet: 6
Gerrit-Owner: Jérémy Compostella <[e-mail]>
Gerrit-Reviewer: Andrey Petrov <[e-mail]>
Gerrit-Reviewer: Bora Guvendik <[e-mail]>
Gerrit-Reviewer: Dinesh Gehlot <[e-mail]>
Gerrit-Reviewer: Eran Mitrani <[e-mail]>
Gerrit-Reviewer: Jakub Czapiga <[e-mail]>
Gerrit-Reviewer: Kapil Porwal <[e-mail]>
Gerrit-Reviewer: Nick Vaccaro <[e-mail]>
Gerrit-Reviewer: Ronak Kanabar <[e-mail]>
Gerrit-Reviewer: Tarun <[e-mail]>
Gerrit-Reviewer: Wonkyu Kim <[e-mail]>
Gerrit-Reviewer: build bot (Jenkins) <[e-mail]>
Gerrit-CC: Arthur Heymans <[e-mail]>
Gerrit-CC: Ashish Kumar Mishra <[e-mail]>
Gerrit-CC: Subrata Banik <[e-mail]>
Gerrit-Attention: Bora Guvendik <[e-mail]>
Gerrit-Attention: Eran Mitrani <[e-mail]>
Gerrit-Attention: Jakub Czapiga <[e-mail]>
Gerrit-Attention: Wonkyu Kim <[e-mail]>
Gerrit-Attention: Jérémy Compostella <[e-mail]>
Gerrit-Attention: Kapil Porwal <[e-mail]>
Gerrit-Attention: Dinesh Gehlot <[e-mail]>
Gerrit-Attention: Nick Vaccaro <[e-mail]>
Gerrit-Attention: Ronak Kanabar <[e-mail]>
Gerrit-Attention: Andrey Petrov <[e-mail]>
Gerrit-Attention: Tarun <[e-mail]>
Gerrit-Comment-Date: Thu, 01 Feb 2024 03:31:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Subrata Banik <[e-mail]>
Comment-In-Reply-To: Jérémy Compostella <[e-mail]>
Gerrit-MessageType: comment