Change in ...coreboot[master]: soc/intel/block/cpu: Make MP init options mutually exclusive
Arthur Heymans has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33357
Change subject: soc/intel/block/cpu: Make MP init options mutually exclusive ......................................................................
soc/intel/block/cpu: Make MP init options mutually exclusive
It shouldn't be the case that multiple or no MP init paths could be selected.
Change-Id: I65b80805d3cd7b66f8c9f878d3c741b98f24288d Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/common/block/cpu/Kconfig 1 file changed, 30 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/33357/1
diff --git a/src/soc/intel/common/block/cpu/Kconfig b/src/soc/intel/common/block/cpu/Kconfig index 9ec5307..e602829 100644 --- a/src/soc/intel/common/block/cpu/Kconfig +++ b/src/soc/intel/common/block/cpu/Kconfig @@ -51,29 +51,49 @@ ENHANCED NEM guarantees that modified data is always kept in cache while clean data is replaced.
-menu "Multiple Processor (MP) Initialization Options" -config USE_COREBOOT_NATIVE_MP_INIT +choice + prompt "Multiple Processor (MP) Initialization Options" + default MP_USE_COREBOOT_NATIVE_MP_INIT if !PLATFORM_USES_FSP2_1 + default MP_USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI if PLATFORM_USES_FSP2_1 + +config MP_USE_COREBOOT_NATIVE_MP_INIT bool "Perform MP Initialization by coreboot" - default y if !PLATFORM_USES_FSP2_1 - default n + select USE_COREBOOT_NATIVE_MP_INIT help This option allows user to select native coreboot option to perform multiprocessor initialization.
-config USE_INTEL_FSP_MP_INIT +config MP_USE_INTEL_FSP_MP_INIT bool "Perform MP Initialization by FSP" - default n + select USE_INTEL_FSP_MP_INIT help This option allows FSP to perform multiprocessor initialization.
-config USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI +config MP_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 + select USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI select USE_COREBOOT_NATIVE_MP_INIT help This option allows FSP to make use of MP services PPI published by coreboot to perform multiprocessor initialization.
-endmenu # Multiple Processor (MP) Initialization Options +endchoice + +config USE_COREBOOT_NATIVE_MP_INIT + bool + help + This option allows user to select native coreboot option to perform + multiprocessor initialization. + +config USE_INTEL_FSP_MP_INIT + bool + help + This option allows FSP to perform multiprocessor initialization. + +config USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI + bool + select USE_COREBOOT_NATIVE_MP_INIT + help + This option allows FSP to make use of MP services PPI published by + coreboot to perform multiprocessor initialization.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33357 )
Change subject: soc/intel/block/cpu: Make MP init options mutually exclusive ......................................................................
Patch Set 1: Code-Review+1
do you have any mean to test this ?
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33357 )
Change subject: soc/intel/block/cpu: Make MP init options mutually exclusive ......................................................................
Patch Set 1:
Patch Set 1: Code-Review+1
do you have any mean to test this ?
It looks like these Kconfig options poorly reflect what the code does. USE_COREBOOT_NATIVE_MP_INIT is never used in the code, so selecting it or not would result in the code attempting to use the native MP init, unless USE_INTEL_FSP_MP_INIT is selected.
It is probably better do drop that option all together.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33357 )
Change subject: soc/intel/block/cpu: Make MP init options mutually exclusive ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1: Code-Review+1
do you have any mean to test this ?
It looks like these Kconfig options poorly reflect what the code does. USE_COREBOOT_NATIVE_MP_INIT is never used in the code,
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 select USE_COREBOOT_NATIVE_MP_INIT help This option allows FSP to make use of MP services PPI published by coreboot to perform multiprocessor initialization.
I don't think "USE_COREBOOT_NATIVE_MP_INIT is never used in the code," is true, see original code. Purpose of "USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI" to only run feature programming using FSP and entire MP Init still handles by Native CB driver.
so selecting it or not would result in the code attempting to use the native MP init, unless USE_INTEL_FSP_MP_INIT is selected.
It is probably better do drop that option all together.
Hello Subrata Banik, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33357
to look at the new patch set (#2).
Change subject: soc/intel/block/cpu: remove unused USE_COREBOOT_NATIVE_MP_INIT ......................................................................
soc/intel/block/cpu: remove unused USE_COREBOOT_NATIVE_MP_INIT
One CONFIG_USE_INTEL_FSP_MP_INIT makes a difference whether native MP init is used or not.
Also make USE_INTEL_FSP_MP_INIT mutually exclusive with USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI.
Change-Id: I65b80805d3cd7b66f8c9f878d3c741b98f24288d Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M src/soc/intel/common/block/cpu/Kconfig 1 file changed, 1 insertion(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/33357/2
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33357 )
Change subject: soc/intel/block/cpu: remove unused USE_COREBOOT_NATIVE_MP_INIT ......................................................................
Patch Set 2:
Patch Set 1:
Patch Set 1:
Patch Set 1: Code-Review+1
do you have any mean to test this ?
It looks like these Kconfig options poorly reflect what the code does. USE_COREBOOT_NATIVE_MP_INIT is never used in the code,
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 select USE_COREBOOT_NATIVE_MP_INIT help This option allows FSP to make use of MP services PPI published by coreboot to perform multiprocessor initialization.
I don't think "USE_COREBOOT_NATIVE_MP_INIT is never used in the code," is true, see original code. Purpose of "USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI" to only run feature programming using FSP and entire MP Init still handles by Native CB driver.
so selecting it or not would result in the code attempting to use the native MP init, unless USE_INTEL_FSP_MP_INIT is selected.
It is probably better do drop that option all together.
I meant that the code never references CONFIG_USE_COREBOOT_NATIVE_MP_INIT. It selects to initialize AP's in coreboot (with or without PPI to do feature programming) based on USE_INTEL_FSP_MP_INIT.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33357 )
Change subject: soc/intel/block/cpu: remove unused USE_COREBOOT_NATIVE_MP_INIT ......................................................................
Patch Set 1:
It looks like these Kconfig options poorly reflect what the code does. USE_COREBOOT_NATIVE_MP_INIT is never used in the code,
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 select USE_COREBOOT_NATIVE_MP_INIT help This option allows FSP to make use of MP services PPI published by coreboot to perform multiprocessor initialization.
I don't think "USE_COREBOOT_NATIVE_MP_INIT is never used in the code," is true, see original code.
It is selected, but the value is never consumed. Nothing depends on it, no code checks its setting.
so selecting it or not would result in the code attempting to use the native MP init, unless USE_INTEL_FSP_MP_INIT is selected.
It is probably better do drop that option all together.
I agree, please drop USE_COREBOOT_NATIVE_MP_INIT Kconfig first. Then, cleaning up the rest should get easier.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33357 )
Change subject: soc/intel/block/cpu: remove unused USE_COREBOOT_NATIVE_MP_INIT ......................................................................
Patch Set 2: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33357 )
Change subject: soc/intel/block/cpu: remove unused USE_COREBOOT_NATIVE_MP_INIT ......................................................................
Patch Set 2: Code-Review+2
I guess we could still make it a choice, but not sure if that's necessary:
-> coreboot MP init -> coreboot MP init w/ PPI -> FSP MP init
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33357 )
Change subject: soc/intel/block/cpu: remove unused USE_COREBOOT_NATIVE_MP_INIT ......................................................................
Patch Set 2: Code-Review-1
Please update Documentation/soc/intel/mp_init/mp_init.md
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33357 )
Change subject: soc/intel/block/cpu: remove unused USE_COREBOOT_NATIVE_MP_INIT ......................................................................
Patch Set 2:
Patch Set 2: Code-Review+2
I guess we could still make it a choice, but not sure if that's necessary:
-> coreboot MP init -> coreboot MP init w/ PPI -> FSP MP init
yeah that would be good. but actually code does the same anyway.
1. !FSP MP init is equivalent to coreboot MP init unless coreboot MP init w/ PPI been selected. 2. FSP MP init selected is another option 3. !FSP MP init with coreboot MP init w/ PPI is latest option
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33357 )
Change subject: soc/intel/block/cpu: remove unused USE_COREBOOT_NATIVE_MP_INIT ......................................................................
Patch Set 2: -Code-Review
we might need to update document as well
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33357 )
Change subject: soc/intel/block/cpu: remove unused USE_COREBOOT_NATIVE_MP_INIT ......................................................................
Patch Set 2:
Patch Set 2: Code-Review+2
I guess we could still make it a choice, but not sure if that's necessary:
-> coreboot MP init -> coreboot MP init w/ PPI -> FSP MP init
You actually still can do all those options -> coreboot MP init : don't select USE_INTEL_FSP_MP_INIT or USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI - coreboot MP init w/ PPI: select USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI -> FSP MP init: select USE_INTEL_FSP_MP_INIT
a choice would just be there for more clarity I suppose?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33357 )
Change subject: soc/intel/block/cpu: remove unused USE_COREBOOT_NATIVE_MP_INIT ......................................................................
Patch Set 2:
Patch Set 2:
Patch Set 2: Code-Review+2
I guess we could still make it a choice, but not sure if that's necessary:
-> coreboot MP init -> coreboot MP init w/ PPI -> FSP MP initYou actually still can do all those options -> coreboot MP init : don't select USE_INTEL_FSP_MP_INIT or USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI
- coreboot MP init w/ PPI: select USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI
-> FSP MP init: select USE_INTEL_FSP_MP_INIT
a choice would just be there for more clarity I suppose? [Subrata] yes
Hello Subrata Banik, Angel Pons, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33357
to look at the new patch set (#3).
Change subject: soc/intel/block/cpu: remove unused USE_COREBOOT_NATIVE_MP_INIT ......................................................................
soc/intel/block/cpu: remove unused USE_COREBOOT_NATIVE_MP_INIT
Only CONFIG_USE_INTEL_FSP_MP_INIT makes a difference whether native MP init is used or not.
Also make USE_INTEL_FSP_MP_INIT mutually exclusive with USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI as this option requires coreboot to set up AP and publish PPI based on it.
Change-Id: I65b80805d3cd7b66f8c9f878d3c741b98f24288d Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M Documentation/soc/intel/mp_init/mp_init.md M src/soc/intel/common/block/cpu/Kconfig 2 files changed, 4 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/33357/3
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33357 )
Change subject: soc/intel/block/cpu: remove unused USE_COREBOOT_NATIVE_MP_INIT ......................................................................
Patch Set 3:
I guess we could still make it a choice, but not sure if that's necessary:
-> coreboot MP init -> coreboot MP init w/ PPI -> FSP MP initYou actually still can do all those options -> coreboot MP init : don't select USE_INTEL_FSP_MP_INIT or USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI
- coreboot MP init w/ PPI: select USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI
-> FSP MP init: select USE_INTEL_FSP_MP_INIT
a choice would just be there for more clarity I suppose?
Yes, choice would only be for usability.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33357 )
Change subject: soc/intel/block/cpu: remove unused USE_COREBOOT_NATIVE_MP_INIT ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33357/3/Documentation/soc/intel/mp_init/mp_i... File Documentation/soc/intel/mp_init/mp_init.md:
https://review.coreboot.org/#/c/33357/3/Documentation/soc/intel/mp_init/mp_i... PS3, Line 32: this MP initialization method. something is missing, `of this`?
Hello Subrata Banik, Angel Pons, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33357
to look at the new patch set (#4).
Change subject: soc/intel/block/cpu: remove unused USE_COREBOOT_NATIVE_MP_INIT ......................................................................
soc/intel/block/cpu: remove unused USE_COREBOOT_NATIVE_MP_INIT
Only CONFIG_USE_INTEL_FSP_MP_INIT makes a difference whether native MP init is used or not.
Also make USE_INTEL_FSP_MP_INIT mutually exclusive with USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI as this option requires coreboot to set up AP and publish PPI based on it.
Change-Id: I65b80805d3cd7b66f8c9f878d3c741b98f24288d Signed-off-by: Arthur Heymans arthur@aheymans.xyz --- M Documentation/soc/intel/mp_init/mp_init.md M src/soc/intel/common/block/cpu/Kconfig 2 files changed, 4 insertions(+), 15 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/57/33357/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33357 )
Change subject: soc/intel/block/cpu: remove unused USE_COREBOOT_NATIVE_MP_INIT ......................................................................
Patch Set 4: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33357 )
Change subject: soc/intel/block/cpu: remove unused USE_COREBOOT_NATIVE_MP_INIT ......................................................................
Patch Set 4: Code-Review+2
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33357 )
Change subject: soc/intel/block/cpu: remove unused USE_COREBOOT_NATIVE_MP_INIT ......................................................................
soc/intel/block/cpu: remove unused USE_COREBOOT_NATIVE_MP_INIT
Only CONFIG_USE_INTEL_FSP_MP_INIT makes a difference whether native MP init is used or not.
Also make USE_INTEL_FSP_MP_INIT mutually exclusive with USE_INTEL_FSP_TO_CALL_COREBOOT_PUBLISH_MP_PPI as this option requires coreboot to set up AP and publish PPI based on it.
Change-Id: I65b80805d3cd7b66f8c9f878d3c741b98f24288d Signed-off-by: Arthur Heymans arthur@aheymans.xyz Reviewed-on: https://review.coreboot.org/c/coreboot/+/33357 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Nico Huber nico.h@gmx.de Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M Documentation/soc/intel/mp_init/mp_init.md M src/soc/intel/common/block/cpu/Kconfig 2 files changed, 4 insertions(+), 15 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve
diff --git a/Documentation/soc/intel/mp_init/mp_init.md b/Documentation/soc/intel/mp_init/mp_init.md index f81ffc1..7284e8a 100644 --- a/Documentation/soc/intel/mp_init/mp_init.md +++ b/Documentation/soc/intel/mp_init/mp_init.md @@ -27,9 +27,9 @@ initialization from coreboot + FSP space.
1. coreboot to perform complete MP initialization by its own. This includes -BSP and AP programming of CPU features mostly non-restricted one. Preferred -Kconfig is USE_COREBOOT_NATIVE_MP_INIT. SoCs like SKL, KBL, APL are okay to -make use of same Kconfig option for MP initialization. +BSP and AP programming of CPU features mostly non-restricted one. This is +the default configuration. Most SoCs like SKL, KBL, APL are okay to make use +of this MP initialization method.
2. Alternatively, SoC users also can skip coreboot doing MP initialization and make use of FSP binary to perform same task. This can be achieved by using diff --git a/src/soc/intel/common/block/cpu/Kconfig b/src/soc/intel/common/block/cpu/Kconfig index 9ec5307..8cc572d 100644 --- a/src/soc/intel/common/block/cpu/Kconfig +++ b/src/soc/intel/common/block/cpu/Kconfig @@ -51,18 +51,10 @@ ENHANCED NEM guarantees that modified data is always kept in cache while clean data is replaced.
-menu "Multiple Processor (MP) Initialization Options" -config USE_COREBOOT_NATIVE_MP_INIT - bool "Perform MP Initialization by coreboot" - default y if !PLATFORM_USES_FSP2_1 - default n - help - This option allows user to select native coreboot option to perform - multiprocessor initialization. - 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.
@@ -71,9 +63,6 @@ depends on FSP_USES_MP_SERVICES_PPI default y if PLATFORM_USES_FSP2_1 default n - select USE_COREBOOT_NATIVE_MP_INIT help This option allows FSP to make use of MP services PPI published by coreboot to perform multiprocessor initialization. - -endmenu # Multiple Processor (MP) Initialization Options
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Subrata Banik (Code Review)