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 11:
(2 comments)
File src/soc/intel/apollolake/Kconfig:
https://review.coreboot.org/c/coreboot/+/84356/comment/b4f48262_df72b5a6?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 ?
Jérémy, please do not get it wrong. Nobody is pushing you back here, at least it was not my intention. The code review is there in order to get other's point of view on a change just to improve the code quality. And the comments you received here are just proposing a different way to solve it in order to cover the coreboot use-cases you might not all have in mind. Your work is appreciated, really!
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/common/fsp_reset.c:
https://review.coreboot.org/c/coreboot/+/84356/comment/a08dd662_29037de5?usp... : PS8, Line 9: #define FSP_STATUS_GLOBAL_RESET \ : (FSP_STATUS_RESET_REQUIRED_COLD + CONFIG_FSP_STATUS_GLOBAL_RESET_INDEX - 1)
There is no "benefits". […]
Yes, this is nothing you in person can change. I was not meant to blame *you* for this.