Change in coreboot[master]: soc/intel/{icl,jsl,tgl}: Remove SkipMpInit UPD as deprecated

[Subrata Banik (Code Review)]

Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44058 )

Change subject: soc/intel/{icl,jsl,tgl}: Remove SkipMpInit UPD as deprecated
......................................................................

soc/intel/{icl,jsl,tgl}: Remove SkipMpInit UPD as deprecated

FSP default UPD for SkipMpInit is set to 0 which refers to run CPU
feature programming on all cores (BSP + APs).

Setting SkipMpInit=1 is not recommended as it will only limit CPU
feature programming on BSP.

TEST=Able to perform CPU feature programming by FSP on all cores
using external MP PPI services.

Change-Id: I22e70f5f15e53c5fabd78cc3698c4d718b607af6
Signed-off-by: Subrata Banik 
---
M src/soc/intel/icelake/fsp_params.c
M src/soc/intel/jasperlake/fsp_params.c
M src/soc/intel/tigerlake/fsp_params.c
3 files changed, 2 insertions(+), 12 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/58/44058/1

diff --git a/src/soc/intel/icelake/fsp_params.c b/src/soc/intel/icelake/fsp_params.c
index e3d355d..d4485c6 100644
--- a/src/soc/intel/icelake/fsp_params.c
+++ b/src/soc/intel/icelake/fsp_params.c
@@ -73,8 +73,6 @@

/* 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);

diff --git a/src/soc/intel/jasperlake/fsp_params.c b/src/soc/intel/jasperlake/fsp_params.c
index 204364b..92ac5b8 100644
--- a/src/soc/intel/jasperlake/fsp_params.c
+++ b/src/soc/intel/jasperlake/fsp_params.c
@@ -96,12 +96,8 @@
    params->PeiGraphicsPeimInit = CONFIG(RUN_FSP_GOP) && is_dev_enabled(dev);

/* Use coreboot MP PPI services if Kconfig is enabled */
-	if (CONFIG(USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI)) {
+	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);
-	}

/* Chipset Lockdown */
    if (get_lockdown_config() == CHIPSET_LOCKDOWN_COREBOOT) {
diff --git a/src/soc/intel/tigerlake/fsp_params.c b/src/soc/intel/tigerlake/fsp_params.c
index 517d771..a61a025 100644
--- a/src/soc/intel/tigerlake/fsp_params.c
+++ b/src/soc/intel/tigerlake/fsp_params.c
@@ -104,12 +104,8 @@
    params->PeiGraphicsPeimInit = CONFIG(RUN_FSP_GOP) && is_dev_enabled(dev);

/* Use coreboot MP PPI services if Kconfig is enabled */
-	if (CONFIG(USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI)) {
+	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);
-	}

/* D3Hot and D3Cold for TCSS */
    params->D3HotEnable = !config->TcssD3HotDisable;

[Wonkyu Kim (Code Review)]

Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44058 )

Change subject: soc/intel/{icl,jsl,tgl}: Remove SkipMpInit UPD as deprecated
......................................................................

Patch Set 1: Code-Review+2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44058 )

Change subject: soc/intel/{icl,jsl,tgl}: Remove SkipMpInit UPD as deprecated
......................................................................

Patch Set 1:

(1 comment)

a

https://review.coreboot.org/c/coreboot/+/44058/1/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/44058/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 111: USE_INTEL_FSP_MP_INIT
This option won't do anything for these platforms, right? If so, we shouldn't be showing it.

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/{icl,jsl,tgl}: Remove SkipMpInit UPD as deprecated
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44058/1/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/44058/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 111: USE_INTEL_FSP_MP_INIT
> This option won't do anything for these platforms, right? If so, we shouldn't be showing it.

no... it can do entire MP init by FSP. i'm working to validate this path as well when user select USE_INTEL_FSP_MP_INIT and not the USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI, at least platform should behave in same way without any brokenness.

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44058 )

Change subject: soc/intel/{icl,jsl,tgl}: Remove SkipMpInit UPD as deprecated
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44058/1/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/44058/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 111: USE_INTEL_FSP_MP_INIT
> no... it can do entire MP init by FSP. […]

Ah, I forgot this is now a choice between three possible values

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/{icl,jsl,tgl}: Remove SkipMpInit UPD as deprecated
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44058/1/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/44058/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 111: USE_INTEL_FSP_MP_INIT
> Ah, I forgot this is now a choice between three possible values

Yes and honestly speaking, i have figured out that this path is broken 😭 
will work on this from now. i have gathered data.

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/{icl,jsl,tgl}: Remove SkipMpInit UPD as deprecated
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44058/1/src/soc/intel/tigerlake/fsp... 
File src/soc/intel/tigerlake/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/44058/1/src/soc/intel/tigerlake/fsp... 
PS1, Line 111: USE_INTEL_FSP_MP_INIT
> Yes and honestly speaking, i have figured out that this path is broken 😭  […]

@Angel, here is CL to fix such issue
https://review.coreboot.org/c/coreboot/+/44076/2
https://review.coreboot.org/c/coreboot/+/44077/2

[Subrata Banik (Code Review)]

Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44058 )

Change subject: soc/intel/{icl,jsl,tgl}: Remove SkipMpInit UPD as deprecated
......................................................................

soc/intel/{icl,jsl,tgl}: Remove SkipMpInit UPD as deprecated

FSP default UPD for SkipMpInit is set to 0 which refers to run CPU
feature programming on all cores (BSP + APs).

Setting SkipMpInit=1 is not recommended as it will only limit CPU
feature programming on BSP.

TEST=Able to perform CPU feature programming by FSP on all cores
using external MP PPI services.

Change-Id: I22e70f5f15e53c5fabd78cc3698c4d718b607af6
Signed-off-by: Subrata Banik 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/44058
Tested-by: build bot (Jenkins) 
Reviewed-by: Wonkyu Kim 
---
M src/soc/intel/icelake/fsp_params.c
M src/soc/intel/jasperlake/fsp_params.c
M src/soc/intel/tigerlake/fsp_params.c
3 files changed, 2 insertions(+), 12 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Wonkyu Kim: Looks good to me, approved

diff --git a/src/soc/intel/icelake/fsp_params.c b/src/soc/intel/icelake/fsp_params.c
index e3d355d..d4485c6 100644
--- a/src/soc/intel/icelake/fsp_params.c
+++ b/src/soc/intel/icelake/fsp_params.c
@@ -73,8 +73,6 @@

/* 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);

diff --git a/src/soc/intel/jasperlake/fsp_params.c b/src/soc/intel/jasperlake/fsp_params.c
index 204364b..92ac5b8 100644
--- a/src/soc/intel/jasperlake/fsp_params.c
+++ b/src/soc/intel/jasperlake/fsp_params.c
@@ -96,12 +96,8 @@
    params->PeiGraphicsPeimInit = CONFIG(RUN_FSP_GOP) && is_dev_enabled(dev);

/* Use coreboot MP PPI services if Kconfig is enabled */
-	if (CONFIG(USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI)) {
+	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);
-	}

/* Chipset Lockdown */
    if (get_lockdown_config() == CHIPSET_LOCKDOWN_COREBOOT) {
diff --git a/src/soc/intel/tigerlake/fsp_params.c b/src/soc/intel/tigerlake/fsp_params.c
index 517d771..a61a025 100644
--- a/src/soc/intel/tigerlake/fsp_params.c
+++ b/src/soc/intel/tigerlake/fsp_params.c
@@ -104,12 +104,8 @@
    params->PeiGraphicsPeimInit = CONFIG(RUN_FSP_GOP) && is_dev_enabled(dev);

/* Use coreboot MP PPI services if Kconfig is enabled */
-	if (CONFIG(USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI)) {
+	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);
-	}

/* D3Hot and D3Cold for TCSS */
    params->D3HotEnable = !config->TcssD3HotDisable;

[Michael Niewöhner (Code Review)]

Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44058 )

Change subject: soc/intel/{icl,jsl,tgl}: Remove SkipMpInit UPD as deprecated
......................................................................

Patch Set 2:

so, what is SkipMpInitPreMem meant for, then?

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/{icl,jsl,tgl}: Remove SkipMpInit UPD as deprecated
......................................................................

Patch Set 2:
> Patch Set 2:
>
> so, what is SkipMpInitPreMem meant for, then?

if i'm not wrong then there was some effort to move SkipMpInit at pre mem stage, do you see such then its better to remove that as well