[coreboot-gerrit] [M] Change in coreboot[main]: drivers/intel/fsp2_0: Simplify FSP global reset definition

[Jérémy Compostella (Code Review)]

Attention is currently required from: Andrey Petrov, Dinesh Gehlot, Eran Mitrani, Jakub Czapiga, Jayvik Desai, Kapil Porwal, Lean Sheng Tan, Nick Vaccaro, Paul Menzel, Pranava Y N, Rishika Raj, Ronak Kanabar, Sean Rhodes, Subrata Banik, Tarun, Werner Zeh.

Jérémy Compostella has posted comments on this change by Jérémy Compostella. ( https://review.coreboot.org/c/coreboot/+/84356?usp=email )

Change subject: drivers/intel/fsp2_0: Simplify FSP global reset definition
......................................................................

Patch Set 12:

(2 comments)

File src/soc/intel/apollolake/Kconfig:

https://review.coreboot.org/c/coreboot/+/84356/comment/ce444ecf_c17a3af4?usp... :
PS6, Line 400: 	default 5
> > I am not a FSP designer but it seems to be following a simple logic. The type is `EFI_STATUS`. […]

Could you please explain to me which coreboot use-cases this implementation is breaking ? I am having a hard time figuring it out.

File src/soc/intel/common/fsp_reset.c:

https://review.coreboot.org/c/coreboot/+/84356/comment/01e2d22b_fcc55e34?usp... :
PS11, Line 35: CONFIG_FSP_STATUS_GLOBAL_RESET
> can we implement a wrapper here instead which can convert 32-bit FSP status into 64-bit looking at i […]

IMO, it is adding yet another layer of potential disconnect between definitions in Kconfig and header files. Someone looking at the values in Kconfig would not understand why 0x40000003 work on 64-bits when looking at the FSP specification. To me, it looks like a band-aid on a implementation not taking into account the details of FSP 2.x specification.

What is wrong with having a solution which automatically comply with the specification ?