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

[Nico Huber (Code Review)]

Attention is currently required from: Jérémy Compostella.

Nico Huber 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:

(1 comment)

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

https://review.coreboot.org/c/coreboot/+/84356/comment/4c8258f3_2d7d727d?usp... :
PS11, Line 35: CONFIG_FSP_STATUS_GLOBAL_RESET
I have a hard time to follow all of your thoughts. I'm not saying anything
is wrong, but I also don't see that everything relates to the code, neither
the current nor the state after this patch.

For instance:
> Therefore, it is important for me to understand that FSP and coreboot are using the same reset numbers without any need for conversion.

Why bring it up? Does this change with this patch?
> Finally, if FSP can pass 0x4000000a as a reset type, coreboot won't complain because it will find a match using the FSP wrapper that converts any index into the reset type value and issues a global reset.

Where do you see this in the code?
> Earlier, if the reset type was warm or cold, we used to select the respective Kconfig from the SoC code. Therefore, during code review, we understood that this platform expects a cold reset while FSP asked to perform a global reset.

How? The selectable Kconfigs that are removed by this patch start at `_3`, i.e.
no way to select a cold (`_1`) or warm reset (`_2`). Please point to the code
(in coreboot history if necessary) where "we used to select" this.
> you can always ignore it as I haven't marked CR-2 (like many code reviewers often do)

Yes, you did not give a -2. But there are other ways to give a change owner
or a reviewer a bad time. For instance repeatedly making a lot of wrong or
unrelated arguments can sometimes be considered harassment.
> can you please show me if there is any section that says on 32-bit FSP the OEM status code should be a 32-bit width value?

I don't think it does. I guess FSP is not the kind of spec that you can take
literally and hope it all works.
> The spec states, "The range of status code that has the highest bit clear and the next to highest bit set are reserved for use by OEMs."
>
> Hence, I'm more specific on the listing that the below-listed value is all 32-bit. Therefore, we should use the exact value in the coreboot to check the matched FSP reset type.
>
> Similarly, I have requested if we can specify the direct 64-bit value.

Seems unnecessary boilerplate if we can instead cover all FSP 2.x versions
with a simple abstraction.
> If we can explicitly use the MAX_BIT definition associated with a FSP supported architecture, then converting the FSP reset type from index is generic. We can avoid having specific Kconfig that lists the hardcoded 32-bit value.

I don't understand what you are trying to tell us with this. It's roughly
what this patch achieves, just indirectly by using the edk headers, isn't it?