Change in ...coreboot[master]: soc/intel/cannonlake: Enable coreboot MP PPI service for WHL/CML

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/cannonlake: Enable coreboot MP PPI service for WHL/CML
......................................................................

soc/intel/cannonlake: Enable coreboot MP PPI service for WHL/CML

This patch performs MP initialization by FSP using coreboot MP
PPI service.

BUG=b:74436746
TEST=Able to perform MP initialization on WHL and CML platform.

Change-Id: I530d50e5aacc3cb9b625df14a50d4c5923e3fb4d
Signed-off-by: Subrata Banik 
---
M src/soc/intel/cannonlake/Kconfig
M src/soc/intel/cannonlake/fsp_params.c
M src/soc/intel/cannonlake/romstage/fsp_params.c
3 files changed, 17 insertions(+), 1 deletion(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/01/32301/1

diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig
index 55fef5a..52f0e41 100644
--- a/src/soc/intel/cannonlake/Kconfig
+++ b/src/soc/intel/cannonlake/Kconfig
@@ -27,6 +27,8 @@
    bool
    default n
    select SOC_INTEL_COMMON_CANNONLAKE_BASE
+	select FSP_PEIM_TO_PEIM_INTERFACE
+	select USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI
    help
      Intel Whiskeylake support

@@ -34,6 +36,12 @@
    bool
    default n
    select SOC_INTEL_COMMON_CANNONLAKE_BASE
+	# TODO:
+	# Delete FSP_PEIM_TO_PEIM_INTERFACE and
+	# USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI selection
+	# and select PLATFORM_USES_FSP2_1 once FSP support for CML is ready
+	select FSP_PEIM_TO_PEIM_INTERFACE
+	select USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI
    help
      Intel Cometlake support

diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c
index b8dba18..1fd42cd 100644
--- a/src/soc/intel/cannonlake/fsp_params.c
+++ b/src/soc/intel/cannonlake/fsp_params.c
@@ -18,7 +18,9 @@
 #include <device/device.h>
 #include <device/pci.h>
 #include <fsp/api.h>
+#include <fsp/ppi/mp_service_ppi.h>
 #include <fsp/util.h>
+#include <intelblocks/mp_init.h>
 #include <intelblocks/xdci.h>
 #include <soc/intel/common/vbt.h>
 #include <soc/pci_devs.h>
@@ -142,6 +144,9 @@
    	params->Usb3OverCurrentPin[i] = 0;
    }

+	if (CONFIG(USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI))
+		params->CpuMpPpi = (uintptr_t) mp_fill_ppi_services_data();
+
    mainboard_silicon_init_params(params);

/* Set PsysPmax if it is available from DT */
diff --git a/src/soc/intel/cannonlake/romstage/fsp_params.c b/src/soc/intel/cannonlake/romstage/fsp_params.c
index ffdcee4..4545f52 100644
--- a/src/soc/intel/cannonlake/romstage/fsp_params.c
+++ b/src/soc/intel/cannonlake/romstage/fsp_params.c
@@ -68,7 +68,10 @@
    	m_cfg->VmxEnable = config->VmxEnable;

#if CONFIG(SOC_INTEL_COMMON_CANNONLAKE_BASE)
-	m_cfg->SkipMpInit = !CONFIG_USE_INTEL_FSP_MP_INIT;
+	if (CONFIG(USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI))
+		m_cfg->SkipMpInit = 0;
+	else
+		m_cfg->SkipMpInit = !CONFIG_USE_INTEL_FSP_MP_INIT;
 #endif

/* Set CpuRatio to match existing MSR value */

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32301 )

Change subject: soc/intel/cannonlake: Enable coreboot MP PPI service for WHL/CML
......................................................................

Patch Set 1:

What is the net effect of turning this on, i.e. what will FSP do differently for WHL today that it was not doing before?

I just want to judge the risk of when to land this around an upcoming board build..

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/cannonlake: Enable coreboot MP PPI service for WHL/CML
......................................................................

Patch Set 1:
> What is the net effect of turning this on, i.e. what will FSP do
> differently for WHL today that it was not doing before?
>
> I just want to judge the risk of when to land this around an
> upcoming board build..

i could see 2 major changes with fsp running feature programming

1. C6DRAM enabling now can be done if you set EnableC6Dram UPD which was not possible earlier
2. SGX enabling, which was not possible/recommended on CNL onwards using coreboot MP init

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32301 )

Change subject: soc/intel/cannonlake: Enable coreboot MP PPI service for WHL/CML
......................................................................

Patch Set 1: Code-Review+2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32301 )

Change subject: soc/intel/cannonlake: Enable coreboot MP PPI service for WHL/CML
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32301/1/src/soc/intel/cannonlake/Kconfig 
File src/soc/intel/cannonlake/Kconfig:

https://review.coreboot.org/#/c/32301/1/src/soc/intel/cannonlake/Kconfig@31 
PS1, Line 31: USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI
This depends on PLATFORM_USES_FSP2_1 which is not selected for WHL or CML: https://review.coreboot.org/cgit/coreboot.git/tree/src/soc/intel/common/bloc...

[Subrata Banik (Code Review)]

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

Change subject: soc/intel/cannonlake: Enable coreboot MP PPI service for WHL/CML
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/32301/1/src/soc/intel/cannonlake/Kconfig 
File src/soc/intel/cannonlake/Kconfig:

https://review.coreboot.org/#/c/32301/1/src/soc/intel/cannonlake/Kconfig@31 
PS1, Line 31: USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI
> This depends on PLATFORM_USES_FSP2_1 which is not selected for WHL or CML: https://review.coreboot. […]

From ICL onward all platform supported with FSP2.1 can select this config as is, but WHL/CML might not compatible with FSP2.1 spec right now hence we can't select the same config. Rather we are using USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI to enable MP service.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32301 )

Change subject: soc/intel/cannonlake: Enable coreboot MP PPI service for WHL/CML
......................................................................

Patch Set 1: Code-Review+2

(1 comment)

https://review.coreboot.org/#/c/32301/1/src/soc/intel/cannonlake/Kconfig 
File src/soc/intel/cannonlake/Kconfig:

https://review.coreboot.org/#/c/32301/1/src/soc/intel/cannonlake/Kconfig@31 
PS1, Line 31: USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI
> From ICL onward all platform supported with FSP2. […]

Aah okay.

[Maulik V Vaghela (Code Review)]

Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32301 )

Change subject: soc/intel/cannonlake: Enable coreboot MP PPI service for WHL/CML
......................................................................

Patch Set 1: Code-Review+1

[Maulik V Vaghela (Code Review)]

Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32301 )

Change subject: soc/intel/cannonlake: Enable coreboot MP PPI service for WHL/CML
......................................................................

Patch Set 1: Code-Review+2

[Subrata Banik (Code Review)]

Subrata Banik has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32301 )

Change subject: soc/intel/cannonlake: Enable coreboot MP PPI service for WHL/CML
......................................................................

soc/intel/cannonlake: Enable coreboot MP PPI service for WHL/CML

This patch performs MP initialization by FSP using coreboot MP
PPI service.

BUG=b:74436746
TEST=Able to perform MP initialization on WHL and CML platform.

Change-Id: I530d50e5aacc3cb9b625df14a50d4c5923e3fb4d
Signed-off-by: Subrata Banik 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32301
Tested-by: build bot (Jenkins) 
Reviewed-by: Duncan Laurie 
Reviewed-by: Furquan Shaikh 
Reviewed-by: Maulik V Vaghela 
---
M src/soc/intel/cannonlake/Kconfig
M src/soc/intel/cannonlake/fsp_params.c
M src/soc/intel/cannonlake/romstage/fsp_params.c
3 files changed, 17 insertions(+), 1 deletion(-)

Approvals:
  build bot (Jenkins): Verified
  Duncan Laurie: Looks good to me, approved
  Furquan Shaikh: Looks good to me, approved
  Maulik V Vaghela: Looks good to me, approved

diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig
index c30b562..14404f7 100644
--- a/src/soc/intel/cannonlake/Kconfig
+++ b/src/soc/intel/cannonlake/Kconfig
@@ -27,6 +27,8 @@
    bool
    default n
    select SOC_INTEL_COMMON_CANNONLAKE_BASE
+	select FSP_PEIM_TO_PEIM_INTERFACE
+	select USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI
    help
      Intel Whiskeylake support

@@ -34,6 +36,12 @@
    bool
    default n
    select SOC_INTEL_COMMON_CANNONLAKE_BASE
+	# TODO:
+	# Delete FSP_PEIM_TO_PEIM_INTERFACE and
+	# USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI selection
+	# and select PLATFORM_USES_FSP2_1 once FSP support for CML is ready
+	select FSP_PEIM_TO_PEIM_INTERFACE
+	select USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI
    help
      Intel Cometlake support

diff --git a/src/soc/intel/cannonlake/fsp_params.c b/src/soc/intel/cannonlake/fsp_params.c
index b8dba18..1fd42cd 100644
--- a/src/soc/intel/cannonlake/fsp_params.c
+++ b/src/soc/intel/cannonlake/fsp_params.c
@@ -18,7 +18,9 @@
 #include <device/device.h>
 #include <device/pci.h>
 #include <fsp/api.h>
+#include <fsp/ppi/mp_service_ppi.h>
 #include <fsp/util.h>
+#include <intelblocks/mp_init.h>
 #include <intelblocks/xdci.h>
 #include <soc/intel/common/vbt.h>
 #include <soc/pci_devs.h>
@@ -142,6 +144,9 @@
    	params->Usb3OverCurrentPin[i] = 0;
    }

+	if (CONFIG(USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI))
+		params->CpuMpPpi = (uintptr_t) mp_fill_ppi_services_data();
+
    mainboard_silicon_init_params(params);

/* Set PsysPmax if it is available from DT */
diff --git a/src/soc/intel/cannonlake/romstage/fsp_params.c b/src/soc/intel/cannonlake/romstage/fsp_params.c
index ffdcee4..4545f52 100644
--- a/src/soc/intel/cannonlake/romstage/fsp_params.c
+++ b/src/soc/intel/cannonlake/romstage/fsp_params.c
@@ -68,7 +68,10 @@
    	m_cfg->VmxEnable = config->VmxEnable;

#if CONFIG(SOC_INTEL_COMMON_CANNONLAKE_BASE)
-	m_cfg->SkipMpInit = !CONFIG_USE_INTEL_FSP_MP_INIT;
+	if (CONFIG(USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI))
+		m_cfg->SkipMpInit = 0;
+	else
+		m_cfg->SkipMpInit = !CONFIG_USE_INTEL_FSP_MP_INIT;
 #endif

/* Set CpuRatio to match existing MSR value */

[Lijian Zhao (Code Review)]

Lijian Zhao has created a revert of this change. ( https://review.coreboot.org/c/coreboot/+/32301 )

Change subject: soc/intel/cannonlake: Enable coreboot MP PPI service for WHL/CML
......................................................................