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

[Subrata Banik (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, Tarun, Werner Zeh.

Subrata Banik 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:

(2 comments)

File src/soc/intel/apollolake/Kconfig:

https://review.coreboot.org/c/coreboot/+/84356/comment/baaa2ac5_b8fb1e41?usp... :
PS6, Line 400: 	default 5
> BTW, Subrata's first link is not accessible for everyone but I guess he is referring to something like this:
> https://github.com/coreboot/coreboot/blob/main/src/vendorcode/intel/edk2/edk...

Werner, you are right. I copied the internal link when I should have pasted the external EDK2 link to explain that what we did back in coreboot was relying on FSP 2.0 spec and was not wrong.

If Intel wants to support FSP2.4 with x64 implementation, they can add one more kconfig and define the 64-bit width Kconfig defaults in a similar manner. Inside the reset.c file, we can add a check depending on x64 or x32 support to know which reset type to check for. Adding a random index and using a wrapper might be suitable for EDK2 customers because they will use the same EDK2 code, but it may not be applicable for the coreboot community.

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

https://review.coreboot.org/c/coreboot/+/84356/comment/33700255_f68a30fe?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.

+2 here.
> 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?