[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 9:

(4 comments)

File src/drivers/intel/fsp2_0/Kconfig:

https://review.coreboot.org/c/coreboot/+/84356/comment/3245629b_dddab1f7?usp... :
PS6, Line 347: 3
> It is in the way different compared to the previous implementation as before the default was 0xfffff […]

As documented in the commit message, all the platforms making use of this feature are defining a Global reset status value. If a new platform without global reset support is added one day, we can always add a mechanism for it. If you insist that it is mandatory to have one right away even though there are no use-cases, I can design something tomorrow.

File src/soc/intel/apollolake/Kconfig:

https://review.coreboot.org/c/coreboot/+/84356/comment/3620c71d_f5f9ee18?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`. `EFI_STATUS` error codes are encoded as `(MAX_BIT | (a))`. For example, `EFI_INVALID_PARAMETER` being `0x80000002` or `0x8000000000000002` on 32-bit or 64-bit  respectively. A similar logic is  applied to the OEM Status Code: `(MAX_BIT >> 1 | (a))`. This is actually documented in FSP 2.0 specification section **11.2.2 OEM Status Code** (cf. quote below) so technically this is not new.
> The range of status code that have the highest bit clear and the next to highest bit set
> are reserved for use by OEMs.

I am having a hard time understanding why cleaning it up now is getting so much push back. Is there an actual use-case I am breaking that I don't see ?

The change I am suggesting (see latest update) is about having a simple Kconfig with the suffix X of `FSP_STATUS_RESET_REQUIRED_X`). `fsp_reset.c` constructs the name of the constant by macro expansion and concatenation (simplified: `FSP_STATUS_RESET_REQUIRED ## FSP_STATUS_GLOBAL_RESET_SUFFIX`) and uses it.

File src/soc/intel/apollolake/Kconfig:

https://review.coreboot.org/c/coreboot/+/84356/comment/b8644aa1_30364965?usp... :
PS8, Line 399: INDEX
> I would rather call it "REQUEST" or "TYPE" instead of INDEDX. […]

I went with `SUFFIX` as it is what it is about having the suffix of `FSP_STATUS_RESET_REQUIRED_`.

What do you think ?

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

https://review.coreboot.org/c/coreboot/+/84356/comment/7e1f6e19_8e4cd73b?usp... :
PS8, Line 9: #define FSP_STATUS_GLOBAL_RESET \
           : 	(FSP_STATUS_RESET_REQUIRED_COLD + CONFIG_FSP_STATUS_GLOBAL_RESET_INDEX - 1)
> > This is again very intel specific behavior. […]

There is no "benefits".

I am not a FSP designer but it seems to be following a simple logic. The type is `EFI_STATUS`. `EFI_STATUS` error codes are encoded as `(MAX_BIT | (a))`. For example, `EFI_INVALID_PARAMETER` being `0x80000002` or `0x8000000000000002` on 32-bit or 64-bit  respectively. A similar logic is  applied to the OEM Status Code: `(MAX_BIT >> 1 | (a))`. But since the FSPs < 2.4 do not support 64-bits mode the full logic was not needed and the final values were provided.