[coreboot-gerrit] Change in coreboot[master]: soc/intel/icelake: Make CpuMpPpi implementation default for ICL

[Subrata Banik (Code Review)]

Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36839 )

Change subject: soc/intel/icelake: Make CpuMpPpi implementation default for ICL
......................................................................

Patch Set 1:
> Patch Set 1:
> > Patch Set 1:
> > > Patch Set 1:
> > > > Patch Set 1:
> > > > > Patch Set 1:
> > > > > > Patch Set 1: Code-Review-1
> > > > > >
> > > > > > > Patch Set 1:
> > > > > > > 
> > > > > > > > Patch Set 1:
> > > > > > > > 
> > > > > > > > > Patch Set 1:
> > > > > > > > > 
> > > > > > > > > > Patch Set 1:
> > > > > > > > > > 
> > > > > > > > > > Not for now. If someone just implements it publicly as open-source solution we can undo the changes
> > > > > > > > > 
> > > > > > > > > I guess the problem is that, some CPU programming has to be performed between FSP calls so until entire FSP-S is not open source i guess this going to be our only solution :(
> > > > > > > > 
> > > > > > > > In what sense does it has to be performed between FSP calls? Can it be implemented in coreboot, but Intel wants to hide some of the details in FSP (or is not interested in doing it in open source code for other reasons) or does FSP have an artificial reliance on being able to CPU programming itself. If the latter is the case, please document it.
> > > > > > > 
> > > > > > > HI Arthur, i believe we have document here https://github.com/coreboot/coreboot/blob/master/Documentation/soc/intel/mp_... and we had great discussion in past while enabling this MP PPI implemetation.
> > > > > >
> > > > > > Yes I read it, but your statement that FSP needs to be fully open sourced confused me into thinking that FSP had some undocumented reliance on MP_PPI.
> > > > > >
> > > > > > The Kconfig options still allow FSP-S to do the MP init fully. You need to clean that up too.
> > > > >
> > > > > i'm cleaning this because other else option doesn't make any sense in ICL. as i have documented the feature won't be able to work unless we do MP PPI in ICL
> > > >
> > > > i meant below code doesn't make any sense as either SKipMpInit=1 would allow to skip FSP-S doing MP Init but now on ICL thats not an option also SkipMpinit is getting deprecated
> > > > params->SkipMpInit = !CONFIG_USE_INTEL_FSP_MP_INIT;
> > > >
> > > > if you could see TODO that I have written, SkipMpInit is going to deprecate in next FSP releases.
> > >
> > > You need to make sure that Kconfig does not present no-op options to the user. Now I can still go in menuconfig and select between FSP_MP_INIT, use PPI and use just coreboot, while in this CL only PPI ever gets used.
> >
> > Sorry Arthur, can you please help me to understand how we can do that ?
>
> select USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI in the ICL Kconfig?

i guess on ICL onwards in FSP we don't have any other option to perform dedicated MP init using FSP and not using coreboot.

this clean up code will make sure coreboot publishes the Coreboot MP PPI and FSP consumes that else if user doesn't fill MP PPI UPD in coreboot then FSP will read NULL in CpuMpPpi and install its own MP ppi handler to perform MP Init. I guess we all want to avoid that hence i'm making sure we are mandatory selecting CpuMpPpi using coreboot API implementation. Sorry not sure where is the concern ? Any way coreboot is doing MP init in ICL only calling FSP to run some feature programming which can't be open right now for sure. I guess this CL is not removing the control from coreboot rather providing the option to only run CB API during MP Init from FSP. isn't it?

Also keeping a placeholder to clean SkipMpInit as its getting removed from FSP code