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

[Werner Zeh (Code Review)]

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

Werner Zeh 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 8:

(4 comments)

File src/drivers/intel/fsp2_0/Kconfig:

https://review.coreboot.org/c/coreboot/+/84356/comment/848b7124_e436e722?usp... :
PS6, Line 347: 3
> It is explained in the documentation right below. No magic involved here.

It is in the way different compared to the previous implementation as before the default was 0xfffffff and now it becomes ```index 3```. When new platforms are added they will default to 3 now instead of "unsupported".
Is it a good plan?

File src/soc/intel/apollolake/Kconfig:

https://review.coreboot.org/c/coreboot/+/84356/comment/7fb62952_5066822b?usp... :
PS6, Line 400: 	default 5
> > I disagree. The previous implementation was unmanageable. […]

I guess what Subrata means here is that in the current implementation you will see FSP_STATUS_GLOBAL_RESET_REQUIRED_COLD and now it would be FSP_STATUS_GLOBAL_RESET_INDEX which is set to 1, so COLD or WARM is getting lost in this transition.

File src/soc/intel/apollolake/Kconfig:

https://review.coreboot.org/c/coreboot/+/84356/comment/a992d553_d084feb9?usp... :
PS8, Line 399: INDEX
I would rather call it "REQUEST" or "TYPE" instead of INDEDX. To me, INDEX is quite misleading as it usually is used to address a cell which can have a changing value. Here you just want to distinguish between different values only.

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

https://review.coreboot.org/c/coreboot/+/84356/comment/b804b339_01eed85e?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. You could have extended to 64 bit by keeping the value untouched and extending it with 32 leading zero bits. Everything could just have worked by maybe just adjusting the data type.
Instead the 64 bit extension was done in the upper bits while 32 additional, unused bits where added in the middle. Where is the benefit?