Change in coreboot[master]: soc/intel/icelake: Make CpuMpPpi implementation default for ICL
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36839 )
Change subject: soc/intel/icelake: Make CpuMpPpi implementation default for ICL ......................................................................
soc/intel/icelake: Make CpuMpPpi implementation default for ICL
TEST=Could able to build and boot ICL DE system
Change-Id: Icd71ec99f06434896c73cff5a52cd3a5ad6ce5f3 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/fsp_params.c 1 file changed, 4 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/36839/1
diff --git a/src/soc/intel/icelake/fsp_params.c b/src/soc/intel/icelake/fsp_params.c index 62c69da..448b82c 100644 --- a/src/soc/intel/icelake/fsp_params.c +++ b/src/soc/intel/icelake/fsp_params.c @@ -84,12 +84,10 @@ for (i = 0; i < ARRAY_SIZE(params->Usb3OverCurrentPin); i++) params->Usb3OverCurrentPin[i] = 0;
- if (CONFIG(USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI)) { - params->CpuMpPpi = (uintptr_t) mp_fill_ppi_services_data(); - params->SkipMpInit = 0; - } else { - params->SkipMpInit = !CONFIG_USE_INTEL_FSP_MP_INIT; - } + /* Mandatory to make use of CpuMpPpi implementation from ICL onwards */ + params->CpuMpPpi = (uintptr_t) mp_fill_ppi_services_data(); + /* TODO: Remove me as SkipMpInit is getting deprecated */ + params->SkipMpInit = 0;
mainboard_silicon_init_params(params);
Maulik V Vaghela 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: Code-Review+2
V Sowmya 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: Code-Review+2
Patrick Georgi 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:
Is there really no way around it? :-(
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:
Is there really no way around it? :-(
No Patrick :(
Philipp Deppenwiese 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:
Not for now. If someone just implements it publicly as open-source solution we can undo the changes
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:
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 :(
Arthur Heymans 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:
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.
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:
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.
Arthur Heymans 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: 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.
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: 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
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: 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.
Arthur Heymans 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: 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.
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: 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 ?
Arthur Heymans 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: 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?
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
Arthur Heymans 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:
> 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 ?
My concern is not at all with this code. Kconfig still presents the option to not select USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI and even use FSP MP init, which the code does not respect. So you please make sure those confusing bogus options don't present to the user.
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?
I'm fine with removing FSP doing the full MP init.
Also keeping a placeholder to clean SkipMpInit as its getting removed from FSP code
ok
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:
My concern is not at all with this code. Kconfig still presents the option to not select USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI and even use FSP MP init, which the code does not respect. So you please make sure those confusing bogus options don't present to the user.
Sorry for bring late in response, Got your point. But looks like its part of common code, where we have all sorts of option listed and we can't have SOC specific guard here. But have you see below Kconfig has "PLATFORM_USES_FSP2_1" to default enable USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI and not select USE_INTEL_FSP_MP_INIT. ICL onwards all platform are PLATFORM_USES_FSP2_1 so isn't that says USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI is default option ? i guess thats what you want to make sure what ever this CL is doing and Kconfig shoudl honor the same ? isn't both the same in that way ?
config USE_INTEL_FSP_MP_INIT bool "Perform MP Initialization by FSP" default n depends on !USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI help This option allows FSP to perform multiprocessor initialization.
config USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI bool "Perform MP Initialization by FSP using coreboot MP PPI service" depends on FSP_USES_MP_SERVICES_PPI default y if PLATFORM_USES_FSP2_1 default n help This option allows FSP to make use of MP services PPI published by coreboot to perform multiprocessor initialization.
Arthur Heymans 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:
My concern is not at all with this code. Kconfig still presents the option to not select USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI and even use FSP MP init, which the code does not respect. So you please make sure those confusing bogus options don't present to the user.
Sorry for bring late in response, Got your point. But looks like its part of common code, where we have all sorts of option listed and we can't have SOC specific guard here. But have you see below Kconfig has "PLATFORM_USES_FSP2_1" to default enable USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI and not select USE_INTEL_FSP_MP_INIT. ICL onwards all platform are PLATFORM_USES_FSP2_1 so isn't that says USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI is default option ? i guess thats what you want to make sure what ever this CL is doing and Kconfig shoudl honor the same ? isn't both the same in that way ?
config USE_INTEL_FSP_MP_INIT bool "Perform MP Initialization by FSP" default n depends on !USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI help This option allows FSP to perform multiprocessor initialization.
config USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI bool "Perform MP Initialization by FSP using coreboot MP PPI service" depends on FSP_USES_MP_SERVICES_PPI default y if PLATFORM_USES_FSP2_1 default n help This option allows FSP to make use of MP services PPI published by coreboot to perform multiprocessor initialization.
It's not about default options, it's about not presenting invalid options regardless of the defaults.
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:
My concern is not at all with this code. Kconfig still presents the option to not select USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI and even use FSP MP init, which the code does not respect. So you please make sure those confusing bogus options don't present to the user.
Sorry for bring late in response, Got your point. But looks like its part of common code, where we have all sorts of option listed and we can't have SOC specific guard here. But have you see below Kconfig has "PLATFORM_USES_FSP2_1" to default enable USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI and not select USE_INTEL_FSP_MP_INIT. ICL onwards all platform are PLATFORM_USES_FSP2_1 so isn't that says USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI is default option ? i guess thats what you want to make sure what ever this CL is doing and Kconfig shoudl honor the same ? isn't both the same in that way ?
config USE_INTEL_FSP_MP_INIT bool "Perform MP Initialization by FSP" default n depends on !USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI help This option allows FSP to perform multiprocessor initialization.
config USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI bool "Perform MP Initialization by FSP using coreboot MP PPI service" depends on FSP_USES_MP_SERVICES_PPI default y if PLATFORM_USES_FSP2_1 default n help This option allows FSP to make use of MP services PPI published by coreboot to perform multiprocessor initialization.
It's not about default options, it's about not presenting invalid options regardless of the defaults.
agree but how we can come out of this now ? any idea?
Arthur Heymans 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:
My concern is not at all with this code. Kconfig still presents the option to not select USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI and even use FSP MP init, which the code does not respect. So you please make sure those confusing bogus options don't present to the user.
Sorry for bring late in response, Got your point. But looks like its part of common code, where we have all sorts of option listed and we can't have SOC specific guard here. But have you see below Kconfig has "PLATFORM_USES_FSP2_1" to default enable USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI and not select USE_INTEL_FSP_MP_INIT. ICL onwards all platform are PLATFORM_USES_FSP2_1 so isn't that says USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI is default option ? i guess thats what you want to make sure what ever this CL is doing and Kconfig shoudl honor the same ? isn't both the same in that way ?
config USE_INTEL_FSP_MP_INIT bool "Perform MP Initialization by FSP" default n depends on !USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI help This option allows FSP to perform multiprocessor initialization.
config USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI bool "Perform MP Initialization by FSP using coreboot MP PPI service" depends on FSP_USES_MP_SERVICES_PPI default y if PLATFORM_USES_FSP2_1 default n help This option allows FSP to make use of MP services PPI published by coreboot to perform multiprocessor initialization.
It's not about default options, it's about not presenting invalid options regardless of the defaults.
agree but how we can come out of this now ? any idea?
Just select USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI in the ICL Kconfig? That should block other MP init options from showing.
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:
My concern is not at all with this code. Kconfig still presents the option to not select USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI and even use FSP MP init, which the code does not respect. So you please make sure those confusing bogus options don't present to the user.
Sorry for bring late in response, Got your point. But looks like its part of common code, where we have all sorts of option listed and we can't have SOC specific guard here. But have you see below Kconfig has "PLATFORM_USES_FSP2_1" to default enable USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI and not select USE_INTEL_FSP_MP_INIT. ICL onwards all platform are PLATFORM_USES_FSP2_1 so isn't that says USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI is default option ? i guess thats what you want to make sure what ever this CL is doing and Kconfig shoudl honor the same ? isn't both the same in that way ?
config USE_INTEL_FSP_MP_INIT bool "Perform MP Initialization by FSP" default n depends on !USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI help This option allows FSP to perform multiprocessor initialization.
config USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI bool "Perform MP Initialization by FSP using coreboot MP PPI service" depends on FSP_USES_MP_SERVICES_PPI default y if PLATFORM_USES_FSP2_1 default n help This option allows FSP to make use of MP services PPI published by coreboot to perform multiprocessor initialization.
It's not about default options, it's about not presenting invalid options regardless of the defaults.
agree but how we can come out of this now ? any idea?
Just select USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI in the ICL Kconfig? That should block other MP init options from showing.
make sense
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Aamir Bohra, Wonkyu Kim, Maulik V Vaghela, V Sowmya, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/36839
to look at the new patch set (#2).
Change subject: soc/intel/icelake: Make CpuMpPpi implementation default for ICL ......................................................................
soc/intel/icelake: Make CpuMpPpi implementation default for ICL
TEST=Could able to build and boot ICL DE system
Change-Id: Icd71ec99f06434896c73cff5a52cd3a5ad6ce5f3 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/Kconfig M src/soc/intel/icelake/fsp_params.c 2 files changed, 5 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/36839/2
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 2:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
Patch Set 1:
My concern is not at all with this code. Kconfig still presents the option to not select USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI and even use FSP MP init, which the code does not respect. So you please make sure those confusing bogus options don't present to the user.
Sorry for bring late in response, Got your point. But looks like its part of common code, where we have all sorts of option listed and we can't have SOC specific guard here. But have you see below Kconfig has "PLATFORM_USES_FSP2_1" to default enable USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI and not select USE_INTEL_FSP_MP_INIT. ICL onwards all platform are PLATFORM_USES_FSP2_1 so isn't that says USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI is default option ? i guess thats what you want to make sure what ever this CL is doing and Kconfig shoudl honor the same ? isn't both the same in that way ?
config USE_INTEL_FSP_MP_INIT bool "Perform MP Initialization by FSP" default n depends on !USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI help This option allows FSP to perform multiprocessor initialization.
config USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI bool "Perform MP Initialization by FSP using coreboot MP PPI service" depends on FSP_USES_MP_SERVICES_PPI default y if PLATFORM_USES_FSP2_1 default n help This option allows FSP to make use of MP services PPI published by coreboot to perform multiprocessor initialization.
It's not about default options, it's about not presenting invalid options regardless of the defaults.
agree but how we can come out of this now ? any idea?
Just select USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI in the ICL Kconfig? That should block other MP init options from showing.
make sense
Done
Arthur Heymans 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 2: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/36839 )
Change subject: soc/intel/icelake: Make CpuMpPpi implementation default for ICL ......................................................................
soc/intel/icelake: Make CpuMpPpi implementation default for ICL
TEST=Could able to build and boot ICL DE system
Change-Id: Icd71ec99f06434896c73cff5a52cd3a5ad6ce5f3 Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/36839 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/icelake/Kconfig M src/soc/intel/icelake/fsp_params.c 2 files changed, 5 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved
diff --git a/src/soc/intel/icelake/Kconfig b/src/soc/intel/icelake/Kconfig index cb9de14..a2fe5ed 100644 --- a/src/soc/intel/icelake/Kconfig +++ b/src/soc/intel/icelake/Kconfig @@ -60,6 +60,7 @@ select UDK_2017_BINDING select DISPLAY_FSP_VERSION_INFO select HECI_DISABLE_USING_SMM + select USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI
config DCACHE_RAM_BASE default 0xfef00000 diff --git a/src/soc/intel/icelake/fsp_params.c b/src/soc/intel/icelake/fsp_params.c index 62c69da..448b82c 100644 --- a/src/soc/intel/icelake/fsp_params.c +++ b/src/soc/intel/icelake/fsp_params.c @@ -84,12 +84,10 @@ for (i = 0; i < ARRAY_SIZE(params->Usb3OverCurrentPin); i++) params->Usb3OverCurrentPin[i] = 0;
- if (CONFIG(USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI)) { - params->CpuMpPpi = (uintptr_t) mp_fill_ppi_services_data(); - params->SkipMpInit = 0; - } else { - params->SkipMpInit = !CONFIG_USE_INTEL_FSP_MP_INIT; - } + /* Mandatory to make use of CpuMpPpi implementation from ICL onwards */ + params->CpuMpPpi = (uintptr_t) mp_fill_ppi_services_data(); + /* TODO: Remove me as SkipMpInit is getting deprecated */ + params->SkipMpInit = 0;
mainboard_silicon_init_params(params);
Nico Huber 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 3:
Sorry for being late. But I don't follow the argumentation here.
If `SkipMpInit` is only deprecated, i.e. not removed yet, why is it suddenly mandatory to set it to 0? Is `SkipMpInit` broken in current ICL FSP? Before this change, did something break if CONFIG_USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI was deselected?
Also, the commit message doesn't match the change. It was already the default. This change enforces it.
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 3:
Patch Set 3:
Sorry for being late. But I don't follow the argumentation here.
If `SkipMpInit` is only deprecated, i.e. not removed yet, why is it suddenly mandatory to set it to 0? Is `SkipMpInit` broken in current ICL FSP? Before this change, did something break if CONFIG_USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI was deselected?
Also, the commit message doesn't match the change. It was already the default. This change enforces it.
i don't have any plan to uprev the ICL headers in recent time hence just made the provision for other platform to remove SKipMpInit. SKipMpInit has removed from all other ICL onward platform and selecting anything else that MP_PPI won't help to meet the all CPU feature programming as documented.
Nico Huber 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 3:
Sorry for being late. But I don't follow the argumentation here.
If `SkipMpInit` is only deprecated, i.e. not removed yet, why is it suddenly mandatory to set it to 0? Is `SkipMpInit` broken in current ICL FSP? Before this change, did something break if CONFIG_USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI was deselected?
Also, the commit message doesn't match the change. It was already the default. This change enforces it.
i don't have any plan to uprev the ICL headers in recent time hence just made the provision for other platform to remove SKipMpInit. SKipMpInit has removed from all other ICL onward platform
I don't understand, it's still there in the official release: https://github.com/IntelFsp/FSP/blob/master/IceLakeFspBinPkg/Include/FspsUpd...
and selecting anything else that MP_PPI won't help to meet the all CPU feature programming as documented.
What is this feature programming? Something new or something that was done in open-source code so far?
Michael Niewöhner 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 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36839/3/src/soc/intel/icelake/Kconf... File src/soc/intel/icelake/Kconfig:
https://review.coreboot.org/c/coreboot/+/36839/3/src/soc/intel/icelake/Kconf... PS3, Line 46: select SOC_INTEL_COMMON_BLOCK_CPU_MPINIT why was that not remove if cb mpinit isn't used anymore?
Michael Niewöhner 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 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36839/3/src/soc/intel/icelake/Kconf... File src/soc/intel/icelake/Kconfig:
https://review.coreboot.org/c/coreboot/+/36839/3/src/soc/intel/icelake/Kconf... PS3, Line 46: select SOC_INTEL_COMMON_BLOCK_CPU_MPINIT
why was that not remove if cb mpinit isn't used anymore?
erm. forget it... I was confused
-
Arthur Heymans (Code Review) -
Maulik V Vaghela (Code Review)
-
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Philipp Deppenwiese (Code Review)
-
Subrata Banik (Code Review)
-
V Sowmya (Code Review)