[coreboot-gerrit] [S] Change in coreboot[main]: drivers/intel/fsp2_0: Initialize CPUs only when FSP-S has completed

[Subrata Banik (Code Review)]

Attention is currently required from: Andrey Petrov, Jérémy Compostella, Ronak Kanabar, Shuo Liu.

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80691?usp=email )

Change subject: drivers/intel/fsp2_0: Initialize CPUs only when FSP-S has completed
......................................................................

Patch Set 3:

(6 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/80691/comment/7611271c_30f1cfaa :
PS3, Line 7: FSP-S 
i guess you mean to say `Add hooks to perform MP init post FSP-MultiPhase SI Init (if supported)`

File src/drivers/intel/fsp2_0/silicon_init.c:

https://review.coreboot.org/c/coreboot/+/80691/comment/8281680c_ad80102e :
PS3, Line 158: 	/* Reinitialize CPUs if FSP-S has done MP Init */
             : 	if (CONFIG(USE_INTEL_FSP_MP_INIT))
             : 		do_mpinit_after_fsp();
if the goal is to skip do_mpinit_after_fsp() immediately after FSP-S API and wait till MultiPhaseSiInit then I will say follow the below snippet.

```
    /* Reinitialize CPUs if FSP-S has done MP Init and MultiPhaseSiInit is not prsent */
    if (CONFIG(USE_INTEL_FSP_MP_INIT) && !fsp_is_multi_phase_init_enabled())
    	do_mpinit_after_fsp();
```

Platform which has MultiPhaseSiInit enabled will skip the do_mpinit_after_fsp() here and anyway till come to [line](https://review.coreboot.org/c/coreboot/+/80691/3/src/drivers/intel/fsp2_0/si...) post MultiPhaseSiInit has executed

https://review.coreboot.org/c/coreboot/+/80691/comment/18dbd9bc_1aa79d26 :
PS3, Line 159: goto fsp_init_done;
what is the point of calling `do_mpinit_after_fsp` when FSP itself is not compliant with FSP2.2 spec aka technically FSP-MultiPhase SI Init is not even there prior to FSP2.2 spec. Hence, why would we like to do MP Init again ?

https://review.coreboot.org/c/coreboot/+/80691/comment/5fdfdcaf_fc13a685 :
PS3, Line 163: goto fsp_init_done;
same as before, assume FSP Multiphase Si Init itself is not enabled hence, why would we like to do do_mpinit_after_fsp() ? ideally a return is what we need here

https://review.coreboot.org/c/coreboot/+/80691/comment/30d97220_a4d61aa3 :
PS3, Line 171: oto fsp_init_done;
again the same, if multi_phase_si_init itself is not implemented then what is the point of calling do_mpinit_after_fsp() from here.

https://review.coreboot.org/c/coreboot/+/80691/comment/c0ea41b6_2e81122d :
PS3, Line 204: USE_INTEL_FSP_MP_INIT
side question: what is the motivation of not using coreboot using MP init and using `USE_INTEL_FSP_MP_INIT`. This is ideally a wrong direction for every new SOC power-on which i have seen previously during MTL as well. the entire X2APIC issue was a mess back then.

I would never test this path as this is not a POR configuration for coreboot users (atleast i don't see any platform even bother to select this kconfig)