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

(2 comments)

Patchset:

PS12: 
A pity to see a good patch wasted.

Instead of arguing how to handle FSP minutiae inside coreboot, I'd
like to suggest an alternative:

Encapsulation.

Just don't even try to handle FSP ABI issues all over coreboot. If we'd
encapsulate the ABI instead, we would only need a mapping from FSP ABI
to coreboot. In this particular example this could be a proper, abstract
`enum` for the reset type, where bits and values don't matter. Forcing
us to always perform a conversion right after calling into FSP would
also force us to check for unknown values right away (fsp_die_...) and
then none of the coreboot internal code would have to preserve bit
patterns of an alien ABI. I believe we could all have an easier job
and a better time this way. Reading through the discussion here, I
wondered: How can we expect to figure out what is right for coreboot
when 80% of the discussion is referencing FSP specs? Some things
don't mix well. Encapsulation would allow to discuss them separately,
hopefully leading to nicer, simpler discussions.

In the general case this could mean to forbid any `*fsp*` and `*efi*`
in src/soc/intel/, also to forbid any #include of such headers. About
everywhere where I've seen such includes, sooner or later there was
trouble for a new platform. Probably because FSP/EFI is still a fork-
per-platform endeavor while coreboot is not. Mixing these too without
encapsulation is just asking for trouble.

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

https://review.coreboot.org/c/coreboot/+/84356/comment/d2253894_250953e4?usp... :
PS11, Line 35: CONFIG_FSP_STATUS_GLOBAL_RESET
> > > The updated spec recommends removing the need for the 32-bit hardcoded macros.
> >
> > Unfair as it does not. FSP 2.0 stated the use of highest bit and provided for the values for 32-bit only because FSP 2.0 is limited to 32-bits.
>
> If that's the case, why did Intel publish the reset type hardcoded macro between 0x40000001-0x40000008 in the FSP 2.3 spec? Why didn't they follow the same calculation they introduced in FSP 2.0 through FSP 2.3 for FSP 2.4?

Subrata, what you reference is just a listing. A listing that tells you what
values were chosen, not why. Jeremy has explained multiple times that they
*did* use the same calculation (as the spec states in the paragraphs right
before the listing). So it's completely futile to ask why they didn't. If
you don't have the time to fully follow the spec, that's ok, but then you
shouldn't argue.