Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Use Kconfig option per variant ......................................................................
soc/intel/tigerlake: Use Kconfig option per variant
Change-Id: I575f28c7eaa82ed5d52ba7c9328a52e76d71dc5b Signed-off-by: Felix Singer felixsinger@posteo.net --- M src/mainboard/google/deltaur/Kconfig M src/mainboard/google/volteer/Kconfig M src/mainboard/intel/tglrvp/Kconfig M src/soc/intel/tigerlake/Kconfig M src/soc/intel/tigerlake/Makefile.inc 5 files changed, 15 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/49055/1
diff --git a/src/mainboard/google/deltaur/Kconfig b/src/mainboard/google/deltaur/Kconfig index 8d95849..0dfec23 100644 --- a/src/mainboard/google/deltaur/Kconfig +++ b/src/mainboard/google/deltaur/Kconfig @@ -14,7 +14,7 @@ select MAINBOARD_HAS_I2C_TPM_CR50 select MAINBOARD_HAS_TPM2 select MAINBOARD_USES_IFD_EC_REGION - select SOC_INTEL_TIGERLAKE + select SOC_INTEL_TIGERLAKE_CLIENT select SYSTEM_TYPE_LAPTOP select MAINBOARD_USES_IFD_GBE_REGION if BOARD_GOOGLE_DELTAN select SOC_INTEL_COMMON_BLOCK_HDA_VERB diff --git a/src/mainboard/google/volteer/Kconfig b/src/mainboard/google/volteer/Kconfig index c776ba1..a602932 100644 --- a/src/mainboard/google/volteer/Kconfig +++ b/src/mainboard/google/volteer/Kconfig @@ -30,7 +30,7 @@ select MAINBOARD_HAS_I2C_TPM_CR50 if BOARD_GOOGLE_VOLTEER2_TI50 select MAINBOARD_HAS_TPM2 select PCIEXP_HOTPLUG - select SOC_INTEL_TIGERLAKE + select SOC_INTEL_TIGERLAKE_CLIENT select HAVE_SPD_IN_CBFS
if BOARD_GOOGLE_BASEBOARD_VOLTEER diff --git a/src/mainboard/intel/tglrvp/Kconfig b/src/mainboard/intel/tglrvp/Kconfig index 9df542d..8aedefc 100644 --- a/src/mainboard/intel/tglrvp/Kconfig +++ b/src/mainboard/intel/tglrvp/Kconfig @@ -13,7 +13,8 @@ select DRIVERS_INTEL_PMC select DRIVERS_USB_ACPI select DRIVERS_SPI_ACPI - select SOC_INTEL_TIGERLAKE + select SOC_INTEL_TIGERLAKE_IOT if BOARD_INTEL_TGLRVP_UP3 + select SOC_INTEL_TIGERLAKE_CLIENT if BOARD_INTEL_TGLRVP_UP4 select SOC_INTEL_COMMON_BLOCK_DTT select INTEL_LPSS_UART_FOR_CONSOLE select DRIVERS_INTEL_ISH diff --git a/src/soc/intel/tigerlake/Kconfig b/src/soc/intel/tigerlake/Kconfig index c7e10a9..0267252 100644 --- a/src/soc/intel/tigerlake/Kconfig +++ b/src/soc/intel/tigerlake/Kconfig @@ -1,9 +1,17 @@ -config SOC_INTEL_TIGERLAKE +config SOC_INTEL_TIGERLAKE_COMMON bool help Intel Tigerlake support
-if SOC_INTEL_TIGERLAKE +config SOC_INTEL_TIGERLAKE_IOT + bool + select SOC_INTEL_TIGERLAKE_COMMON + +config SOC_INTEL_TIGERLAKE_CLIENT + bool + select SOC_INTEL_TIGERLAKE_COMMON + +if SOC_INTEL_TIGERLAKE_COMMON
config CPU_SPECIFIC_OPTIONS def_bool y diff --git a/src/soc/intel/tigerlake/Makefile.inc b/src/soc/intel/tigerlake/Makefile.inc index 4a41812..d695a24 100644 --- a/src/soc/intel/tigerlake/Makefile.inc +++ b/src/soc/intel/tigerlake/Makefile.inc @@ -1,4 +1,4 @@ -ifeq ($(CONFIG_SOC_INTEL_TIGERLAKE),y) +ifeq ($(CONFIG_SOC_INTEL_TIGERLAKE_COMMON),y)
subdirs-y += romstage subdirs-y += ../../../cpu/intel/microcode
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Set Ready For Review
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Patch Set 3: Code-Review+1
LGTM, but I'd like to see if others also like it.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/49055/3/src/mainboard/intel/tglrvp/... File src/mainboard/intel/tglrvp/Kconfig:
https://review.coreboot.org/c/coreboot/+/49055/3/src/mainboard/intel/tglrvp/... PS3, Line 16: select SOC_INTEL_TIGERLAKE_IOT if BOARD_INTEL_TGLRVP_UP3 : select SOC_INTEL_TIGERLAKE_CLIENT if BOARD_INTEL_TGLRVP_UP4 i don't think we have such details anywhere captured, i know Client design with UP3 as well
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/49055/3/src/mainboard/intel/tglrvp/... File src/mainboard/intel/tglrvp/Kconfig:
https://review.coreboot.org/c/coreboot/+/49055/3/src/mainboard/intel/tglrvp/... PS3, Line 16: select SOC_INTEL_TIGERLAKE_IOT if BOARD_INTEL_TGLRVP_UP3 : select SOC_INTEL_TIGERLAKE_CLIENT if BOARD_INTEL_TGLRVP_UP4 From the FSP `README.md`:
Client | Processors for client platforms TGL_IOT | UP3-Series processors for IoT platforms
Does this mean we have to always use `TGL_IOT` with UP3? or are there UP3 that should use the `Client` version?
In the former case, we could make this SOC_INTEL_TIGERLAKE_UP[34].
Nathaniel L Desimone has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/49055/3/src/mainboard/intel/tglrvp/... File src/mainboard/intel/tglrvp/Kconfig:
https://review.coreboot.org/c/coreboot/+/49055/3/src/mainboard/intel/tglrvp/... PS3, Line 16: select SOC_INTEL_TIGERLAKE_IOT if BOARD_INTEL_TGLRVP_UP3 : select SOC_INTEL_TIGERLAKE_CLIENT if BOARD_INTEL_TGLRVP_UP4 Most of the time we should use the client FSP for TGLRVP_UP3. The comment in that table is trying to point out that the IoT SKUs of TGL are 100% UP3 (there are no UP4 IoT SKUs) whereas the client SKUs have both UP3 and UP4. I wrote the following snippet in README.md to describe when the IoT FSP should be used:
Client and IoT (Internet of Things) SKUs of 11th Generation Intel® Core™ (formerly Tiger Lake) processors can be differentiated by checking the processor number. Processor numbers ending in a "E" are IoT SKUs, processor numbers that do not end in "E" are client SKUs. For example, the Intel® Core™ i7-1185G7 is a client SKU and the Intel® Core™ i7-1185G7E is an IoT SKU.
The vast majority of TGL UP3 RVPs will be built with a client SKU, so client FSP should definitely be the default. I think it would be reasonable to provide a mechanism for someone to override this with the IoT FSP because ~5% of TGL UP3 RVPs will be built with an IoT SKU. The client FSP will still boot on an IoT SKU, however IoT SKU specific chipset features like TCC (Time Coordinated Computing) won't work without the IoT FSP. So I think keeping client as default makes sense. Using the IoT FSP should be an explicit Kconfig override for the rare cases where you want it.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Patch Set 3:
Thanks, Nate, for the elaborate answer.
I think we should add a choice with a prompt for the FSP version to `soc/intel/tigerlake/Kconfig` that always defaults to "Client".
In a more advanced solution, both options of the choice could depend on `SOC_INTEL_TIGERLAKE_(CLIENT|IOT)` each, so we don't show invalid options if we know exactly what a board ships with (e.g. for something called UP4, we could hide the IoT option).
Attention is currently required from: Nico Huber, Furquan Shaikh, Martin Roth, Tim Wawrzynczak, Subrata Banik, Angel Pons, Michael Niewöhner, Patrick Rudolph, Nathaniel L Desimone. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Set Ready For Review
Attention is currently required from: Nico Huber, Furquan Shaikh, Martin Roth, Tim Wawrzynczak, Subrata Banik, Angel Pons, Michael Niewöhner, Patrick Rudolph, Nathaniel L Desimone. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
Thanks, Nate, for the elaborate answer.
I think we should add a choice with a prompt for the FSP version to `soc/intel/tigerlake/Kconfig` that always defaults to "Client".
In a more advanced solution, both options of the choice could depend on `SOC_INTEL_TIGERLAKE_(CLIENT|IOT)` each, so we don't show invalid options if we know exactly what a board ships with (e.g. for something called UP4, we could hide the IoT option).
I renamed the options to *_UP[34] and added the choice menu in CB:48713.
Attention is currently required from: Felix Singer, Furquan Shaikh, Tim Wawrzynczak, Subrata Banik. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
Patchset:
PS4: LGTM, assuming somebody can verify the UP4 for Chrome devices.
Attention is currently required from: Felix Singer, Nico Huber, Furquan Shaikh, Ravishankar Sarawadi, Srinidhi N Kaushik, Raj Astekar. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
Patchset:
PS4:
LGTM, assuming somebody can verify the UP4 for Chrome devices.
Verified, we use the Client FSP, not IoT.
Attention is currently required from: Felix Singer, Furquan Shaikh, Ravishankar Sarawadi, Tim Wawrzynczak, Srinidhi N Kaushik, Raj Astekar. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
Verified, we use the Client FSP, not IoT.
That much we knew already :) but Client FSP could also mean UP3. If not mentioned explicitly, it could be guessed from the ball-out. UP4 should be BGA1598 and UP3 should be BGA1449, AFAICS.
Attention is currently required from: Felix Singer, Nico Huber, Furquan Shaikh, Ravishankar Sarawadi, Srinidhi N Kaushik, Raj Astekar. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
That much we knew already :)
good guess 😉
but Client FSP could also mean UP3. If not mentioned explicitly, it could be guessed from the ball-out. UP4 should be BGA1598 and UP3 should be BGA1449, AFAICS.
In this patch train, the Kconfig is just used to select the FSP version; however, the volteer variants are using combinations of UP3 & UP4, if we want to be more specific.
Here's the current breakdown:
UP4 --- copano halvor terrador todor voema
UP3 --- everything else 😊
Attention is currently required from: Felix Singer, Furquan Shaikh, Ravishankar Sarawadi, Tim Wawrzynczak, Srinidhi N Kaushik, Raj Astekar. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
That much we knew already :) […]
So do we want to try to select UP3/UP4 correctly, or let the mainboard only decide about the client/iot FSP? For the current use case, the latter may even be more expressive. For instance, for boards with UP3 where we know that only a specific FSP makes sense. Something like
select SOC_INTEL_TIGERLAKE_CLIENT_SKUS_ONLY
vs.
select SOC_INTEL_TIGERLAKE_IOT_SKUS_ONLY
or nothing selected if the user should have the choice.
Attention is currently required from: Felix Singer, Nico Huber, Furquan Shaikh, Ravishankar Sarawadi, Srinidhi N Kaushik, Raj Astekar. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Patch Set 4: -Code-Review
(1 comment)
Patchset:
PS4:
So do we want to try to select UP3/UP4 correctly, or let the mainboard […]
I think it should be up to the mainboard; if we're only using this to select an FSP version, why hide it beneath the guise of SoC SKU? Let the developer choose which FSP is appropriate for their board; I think Nate's point about the IoT one being less common is probably pretty spot-on. If we can think of another use-case for the UP3/UP4 distinction besides FSP, then it may make sense, but I can't think of anything right now
Attention is currently required from: Nico Huber, Furquan Shaikh, Ravishankar Sarawadi, Tim Wawrzynczak, Srinidhi N Kaushik, Raj Astekar. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
I think it should be up to the mainboard; if we're only using this to select an FSP version, why hid […]
I agree with Tim. We should just let the developers decide.
Attention is currently required from: Nico Huber, Furquan Shaikh, Ravishankar Sarawadi, Tim Wawrzynczak, Srinidhi N Kaushik, Raj Astekar. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
I agree with Tim. We should just let the developers decide.
On the other hand, this way it is more modular. I don't know what is better :/
Attention is currently required from: Felix Singer, Furquan Shaikh, Ravishankar Sarawadi, Tim Wawrzynczak, Srinidhi N Kaushik, Raj Astekar. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
On the other hand, this way it is more modular. […]
I too agree with Tim. Although, my interpretation may differ. :)
We should have something selectable to decide the FSP version. Or better to set the default in the choice (in the follow-up change). Instead of going the indirect route via SKUs, it should just reflect what it effectively does, e.g.
config SOC_INTEL_TIGERLAKE_CLIENT_FSP_DEFAULT bool
config SOC_INTEL_TIGERLAKE_IOT_FSP_DEFAULT bool
Or even just one and an implicit default for the other.
These would be additional configs, on top of the SOC_INTEL_TIGERLAKE selection. Thus, should be part of the follow-up change, and IMHO we can abandon this one.
(Overthinking it: To fully model reality, we would need both, the SKU selection and the FSP version. The SKU selection would decide which options are visible in the choice and the FSP option which one would be the default if multiple options are visible. Adding an SKU selection may be error-prone, though. Something more to main- tain that doesn't have a direct effect, and thus could result in subtle, hard to debug issues. It would be a different story if we did it globally in coreboot. I.e. always select exactly the chip SKUs that are expected to be soldered on a board. This would also have other advantages like better default choices of added microcode updates. But without training us first (hence better do it globally) it could be too error-prone.)
Attention is currently required from: Felix Singer, Nico Huber, Furquan Shaikh, Ravishankar Sarawadi, Srinidhi N Kaushik, Raj Astekar. Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
I too agree with Tim. Although, my interpretation may differ. :) […]
I think we should avoid SKU selection if possible, that seems like it could get really hairy... I would prefer to stick with features (use FSP A vs. FSP B), and I don't think it's too onerous to have to select one or the other for a TGL mainboard, as long as we have the right help text 😊
Attention is currently required from: Felix Singer, Nico Huber, Furquan Shaikh, Ravishankar Sarawadi, Raj Astekar. Srinidhi N Kaushik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Patch Set 4:
(1 comment)
File src/mainboard/google/volteer/Kconfig:
https://review.coreboot.org/c/coreboot/+/49055/comment/72102525_3022e747 PS4, Line 33: select SOC_INTEL_TIGERLAKE_UP4 This should be SOC_INTEL_TIGERLAKE_UP3. Currently its not making any difference as both UP3 and UP4 select COMMON but UP3 is the right config for volteer.
Attention is currently required from: Felix Singer, Nico Huber, Ravishankar Sarawadi, Tim Wawrzynczak, Raj Astekar. Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
I think we should avoid SKU selection if possible, that seems like it could get really hairy... […]
+1 to what Nico and Tim said. Felix - are you planning on updating the CLs to address Nico and Tim's comments?
Attention is currently required from: Felix Singer, Nico Huber, Furquan Shaikh, Ravishankar Sarawadi, Tim Wawrzynczak, Raj Astekar. Srinidhi N Kaushik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Patch Set 4:
(1 comment)
Patchset:
PS4:
+1 to what Nico and Tim said. […]
I have updated https://review.coreboot.org/c/coreboot/+/48713 to include the review comments. I dont think we need this CL anymore, we can abandon this in that case.
Felix Singer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/49055 )
Change subject: soc/intel/tigerlake: Add Kconfig options for each platform ......................................................................
Abandoned
-
Angel Pons (Code Review) -
Felix Singer (Code Review)
-
Furquan Shaikh (Code Review)
-
Nathaniel L Desimone (Code Review)
-
Nico Huber (Code Review)
-
Srinidhi N Kaushik (Code Review)
-
Subrata Banik (Code Review)
-
Tim Wawrzynczak (Code Review)