Change in ...coreboot[master]: soc/intel/icelake: Add option to enable display over PCI external GFX
Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33739
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
soc/intel/icelake: Add option to enable display over PCI external GFX
This patch selects FSP UPDs required for internal and external GFX bring up based on CONFIG_ENABLE_EXT_GFX Kconfig.
Change-Id: I34a02bff112f922cabd48c23bc76370892ec62d9 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/fsp_params.c M src/soc/intel/icelake/romstage/fsp_params.c 2 files changed, 38 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/33739/1
diff --git a/src/soc/intel/icelake/fsp_params.c b/src/soc/intel/icelake/fsp_params.c index ac7edd2..2792d26 100644 --- a/src/soc/intel/icelake/fsp_params.c +++ b/src/soc/intel/icelake/fsp_params.c @@ -80,9 +80,24 @@
mainboard_silicon_init_params(params);
- params->PeiGraphicsPeimInit = 1; - params->GtFreqMax = 2; - params->CdClock = 3; + dev = pcidev_on_root(2, 0); + + if (!dev || !dev->enabled) { + /* + * If iGPU is disabled or not defined in the devicetree.cb, + * the FSP does not initialize this device + */ + params->PeiGraphicsPeimInit = 0; + if (CONFIG(ENABLE_EXT_GFX)) { + params->Enable8254ClockGating = 0; + params->Enable8254ClockGatingOnS3 = 0; + } + } else { + params->PeiGraphicsPeimInit = 1; + params->GtFreqMax = 2; + params->CdClock = 3; + } + /* Unlock upper 8 bytes of RTC RAM */ params->PchLockDownRtcMemoryLock = 0;
diff --git a/src/soc/intel/icelake/romstage/fsp_params.c b/src/soc/intel/icelake/romstage/fsp_params.c index 3c49fee..415c039 100644 --- a/src/soc/intel/icelake/romstage/fsp_params.c +++ b/src/soc/intel/icelake/romstage/fsp_params.c @@ -25,17 +25,34 @@ const struct soc_intel_icelake_config *config) { unsigned int i; - const struct device *dev = pcidev_on_root(0, 0); + const struct device *dev = pcidev_on_root(2, 0); uint32_t mask = 0;
- /* Set IGD stolen size to 60MB. */ - m_cfg->IgdDvmt50PreAlloc = 0xFE; + if (!dev || !dev->enabled) { + /* + * If iGPU is disabled or not defined in the devicetree.cb, + * the FSP does not initialize this device + */ + m_cfg->InternalGfx = 0; + m_cfg->IgdDvmt50PreAlloc = 0; + if (CONFIG(ENABLE_EXT_GFX)) { + m_cfg->ScanExtGfxForLegacyOpRom = 1; + m_cfg->PrimaryDisplay = 2; + } + } else { + m_cfg->InternalGfx = 1; + /* Set IGD stolen size to 60MB. */ + m_cfg->IgdDvmt50PreAlloc = 0xFE; + } + m_cfg->TsegSize = CONFIG_SMM_TSEG_SIZE; m_cfg->IedSize = CONFIG_IED_REGION_SIZE; m_cfg->SaGv = config->SaGv; m_cfg->UserBd = BOARD_TYPE_ULT_ULX; m_cfg->RMT = config->RMT; m_cfg->SkipMbpHob = 1; + + dev = pcidev_on_root(0x1f, 3); /* If Audio Codec is enabled, enable FSP UPD */ if (!dev) m_cfg->PchHdaEnable = 0;
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33739
to look at the new patch set (#3).
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
soc/intel/icelake: Add option to enable display over PCI external GFX
This patch selects FSP UPDs required for internal and external GFX bring up based on CONFIG_ENABLE_EXT_GFX Kconfig.
Change-Id: I34a02bff112f922cabd48c23bc76370892ec62d9 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/Kconfig M src/soc/intel/icelake/fsp_params.c M src/soc/intel/icelake/romstage/fsp_params.c 3 files changed, 39 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/33739/3
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 3:
Please elaborate, why it cannot be detected at run-time? It should also be run-time configurable.
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 5:
(1 comment)
Please explain why those UPDs are required.
https://review.coreboot.org/#/c/33739/5/src/soc/intel/icelake/romstage/fsp_p... File src/soc/intel/icelake/romstage/fsp_params.c:
https://review.coreboot.org/#/c/33739/5/src/soc/intel/icelake/romstage/fsp_p... PS5, Line 39: m_cfg->ScanExtGfxForLegacyOpRom Why does the FSP need those settings? GFX init is done in coreboot, isn't it? This Kconfig has no effect if the IGD is enabled. That might not be what the user expects when this Kconfig is selected.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/33739/5/src/soc/intel/icelake/romstage/fsp_p... File src/soc/intel/icelake/romstage/fsp_params.c:
https://review.coreboot.org/#/c/33739/5/src/soc/intel/icelake/romstage/fsp_p... PS5, Line 39: m_cfg->ScanExtGfxForLegacyOpRom
Why does the FSP need those settings?
After MRC init, FSP does memory management i mean allocating initial resources that time (GSM, DSM, TSEG, PCI reserve memory size), FSP like to know if any device present in platform might have higher memory requirement so it can help to reserved that much memory under PCI reserved memory range.
GFX init is done in coreboot, isn't it?
Launching OpRom is CB responsibility.
This Kconfig has no effect if the IGD is enabled. That might not be what the user expects when this Kconfig is selected.
yes, thats true, do you see this code violating that expectation ?
below piece of code for IGD present and above if is for IGD not selected. I have also added help text inside kconfig to tell what does this option does. isn;t it ? else { m_cfg->InternalGfx = 1; /* Set IGD stolen size to 60MB. */ m_cfg->IgdDvmt50PreAlloc = 0xFE; }
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/33739/5/src/soc/intel/icelake/romstage/fsp_p... File src/soc/intel/icelake/romstage/fsp_params.c:
https://review.coreboot.org/#/c/33739/5/src/soc/intel/icelake/romstage/fsp_p... PS5, Line 39: m_cfg->ScanExtGfxForLegacyOpRom
Why does the FSP need those settings? […]
1. The help text in Kconfig doesn't even mention the dependency to IGD. 2. Higher memory requirement also applies if IGD is enabled and an user wants to enable one or more dGPUs. 3. There's no need to dynamically adjust MMIO size in FSP. I tried to implement in coreboot, but nobody wants that feature, so it was decided to always use 2 GiB MMIO size on boards that allow the use of a dGPU. Hardcoding 2GiB should be fine and in that case we don't even need a new Kconfig for that.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/33739/5/src/soc/intel/icelake/romstage/fsp_p... File src/soc/intel/icelake/romstage/fsp_params.c:
https://review.coreboot.org/#/c/33739/5/src/soc/intel/icelake/romstage/fsp_p... PS5, Line 55: dev = pcidev_on_root(0x1f, 3); Please patch the HDA separately.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/33739/5/src/soc/intel/icelake/romstage/fsp_p... File src/soc/intel/icelake/romstage/fsp_params.c:
https://review.coreboot.org/#/c/33739/5/src/soc/intel/icelake/romstage/fsp_p... PS5, Line 39: m_cfg->ScanExtGfxForLegacyOpRom
- The help text in Kconfig doesn't even mention the dependency to IGD.
I will fix this. Thanks for pointing.
- Higher memory requirement also applies if IGD is enabled and an user wants to enable one or more dGPUs.
- There's no need to dynamically adjust MMIO size in FSP.
I can pass this feedback but i'm just telling what i do see inside FSP today and i think its pointing to your #2 comment her.
I tried to implement in coreboot, but nobody wants that feature, so it was decided to always use 2 GiB MMIO size on boards that allow the use of a dGPU. Hardcoding 2GiB should be fine and in that case we don't even need a new Kconfig for that.
This kconfig is not for allocating static 2GB, rather its telling FSP to not configure IGD and make IGD disable also don't turn off 8254 timer as oprom might need this
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33739
to look at the new patch set (#6).
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
soc/intel/icelake: Add option to enable display over PCI external GFX
This patch selects FSP UPDs required for internal and external GFX bring up based on CONFIG_ENABLE_EXT_GFX Kconfig.
Change-Id: I34a02bff112f922cabd48c23bc76370892ec62d9 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/Kconfig M src/soc/intel/icelake/fsp_params.c M src/soc/intel/icelake/romstage/fsp_params.c 3 files changed, 37 insertions(+), 7 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/33739/6
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/#/c/33739/5/src/soc/intel/icelake/romstage/fsp_p... File src/soc/intel/icelake/romstage/fsp_params.c:
https://review.coreboot.org/#/c/33739/5/src/soc/intel/icelake/romstage/fsp_p... PS5, Line 55: dev = pcidev_on_root(0x1f, 3);
Please patch the HDA separately.
https://review.coreboot.org/c/coreboot/+/33933
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/33739/7/src/soc/intel/icelake/fsp_params.c File src/soc/intel/icelake/fsp_params.c:
https://review.coreboot.org/#/c/33739/7/src/soc/intel/icelake/fsp_params.c@9... PS7, Line 92: params->Enable8254ClockGating = 0; In change https://review.coreboot.org/c/coreboot/+/33512 it was fixed on cannonlake. Making use of CONFIG_USE_LEGACY_8254_TIMER seems fine, as it can be selected by PAYLOAD_SEABIOS or VGA_ROM_RUN. Also there's no need to have a dependency to disabled IGD. One might run dGPU oprom even with IGD enabled.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33739
to look at the new patch set (#8).
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
soc/intel/icelake: Add option to enable display over PCI external GFX
This patch selects FSP UPDs required for internal and external GFX bring up based on CONFIG_ENABLE_EXT_GFX Kconfig.
Change-Id: I34a02bff112f922cabd48c23bc76370892ec62d9 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/Kconfig M src/soc/intel/icelake/fsp_params.c M src/soc/intel/icelake/romstage/fsp_params.c 3 files changed, 32 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/33739/8
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33739/7/src/soc/intel/icelake/fsp_p... File src/soc/intel/icelake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/33739/7/src/soc/intel/icelake/fsp_p... PS7, Line 92: params->Enable8254ClockGating = 0;
In change https://review.coreboot.org/c/coreboot/+/33512 it was fixed on cannonlake. […]
Done
Hello Patrick Rudolph, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33739
to look at the new patch set (#9).
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
soc/intel/icelake: Add option to enable display over PCI external GFX
This patch updates required FSP UPDs to get display either through internal or external GFX card.
Also creates a new kconfig option to bring display over external PCI based GFX card. CONFIG_HAVE_EXT_GFX to select all required kconfig to launch legacy oprom from pci based GFX card.
Change-Id: I34a02bff112f922cabd48c23bc76370892ec62d9 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/Kconfig M src/soc/intel/icelake/fsp_params.c M src/soc/intel/icelake/romstage/fsp_params.c 3 files changed, 44 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/33739/9
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 9: Code-Review-1
(3 comments)
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... File src/soc/intel/icelake/Kconfig:
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 211: config HAVE_EXT_GFX 1) The Kconfig is poorly named. Just because an EXT GFX *exists*, doens't mean you want to use it as primary VGA device. For example on Nvidia Optimus enabled laptops, that do not support hybrid graphics, the Intel GPU is always connected to the panel. It has an ext gfx, but it's never used as primary VGA device. 2) Also by selecting this on mainboard level you force the use of untrusted legacy blobs. 3) It's not even visible to the user why this board selects the method as the bool has no name. 4) I don't see why this is icelace specific.
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/fsp_p... File src/soc/intel/icelake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/fsp_p... PS9, Line 89: if (!dev || !dev->enabled) good
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/romst... File src/soc/intel/icelake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/romst... PS9, Line 31: if (!dev || !dev->enabled) { good
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... File src/soc/intel/icelake/Kconfig:
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 211: config HAVE_EXT_GFX
- The Kconfig is poorly named. […]
1) Its a kconfig and it selects bunch of required kconfig to load/launch/execute OpRom code if mainboard selects that.
For example on Nvidia Optimus enabled laptops, that do not support hybrid graphics, the Intel GPU is always connected to the panel. It has an ext gfx, but it's never used as primary VGA device.
This is platform policy that OEM decides, i'm not OEM so i'm not making any policy here. I'm just adding my options to launch oprom if mb selects so.
2) This is not any valid point, we have proper protection in CB code and FSP to run opRom. its not a new things. Many such device has oprom and BIOS has to launch that. SOC/platform owner has to take protection before launching OpRom thats been cover already 3) its a choice for mainboard to select external GFX over internal gfx. 4) i wish to enable external GFX support for my ICL RVP hence added this config. its in SoC, so it doesn't create problem for other
Hello Patrick Rudolph, Aamir Bohra, Patrick Rudolph, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33739
to look at the new patch set (#10).
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
soc/intel/icelake: Add option to enable display over PCI external GFX
This patch updates required FSP UPDs to get display either through internal or external GFX card.
Also creates a new kconfig option to bring display over external PCI based GFX card. CONFIG_HAVE_EXT_GFX to select all required kconfig to launch legacy oprom from pci based GFX card.
Change-Id: I34a02bff112f922cabd48c23bc76370892ec62d9 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/Kconfig M src/soc/intel/icelake/fsp_params.c M src/soc/intel/icelake/romstage/fsp_params.c 3 files changed, 44 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/33739/10
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 10: Code-Review-1
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 10:
Patch Set 10: Code-Review-1
whats now ?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 10:
(1 comment)
I don't like to pick sides, but I agree to Patrick in every point.
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... File src/soc/intel/icelake/Kconfig:
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 211: config HAVE_EXT_GFX
- This is not any valid point, we have proper protection in CB code and FSP to run opRom. its not a new things. Many such device has oprom and BIOS has to launch that. SOC/platform owner has to take protection before launching OpRom thats been cover already
There is protection and protection. What you call "proper protection" is the bare minimum for security minded people. Have you noticed the Yabel code? that would come closer to proper protection. But real protection is simply to not run unknown code from some ROM on an extension card.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... File src/soc/intel/icelake/Kconfig:
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 211: config HAVE_EXT_GFX
- This is not any valid point, we have proper protection in CB code and FSP to run opRom. […]
can you please help to write the same for all PCIE card vendors NVDIA, AMD, ATI whoever and say please stop selling cards with OpRom, that would help. Right now my problem statement is that, i need to bring display over PCIE GFX for my board and i need to run OpRom, what is my priority.
I have protection taken to lock down all my protected range registers what any OpRom can manipulate, i have all Chipset registers lockdown as part of POSTBOOT_SAI thats the whole purpose of security that no 3rd party code can break into chipset space.
What you mean by not running OpRom code. still today there are many PC running thousands of OpRom code, all are BAD ? and they should stop their business. Please try to be reasonable with other patches and understand what is the need of the hour.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 10:
(7 comments)
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... File src/soc/intel/icelake/Kconfig:
PS9: `HAVE_EXT_GFX` should be put into `src/device/Kconfig`, so it’s common between all boards.
It’s still not clear to me, why the option `VGA_ROM_RUN` is not enough for your use case? That means, the user can just configure the build as you want it to, right?
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 212: bool Is it selected by default or not?
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 213: select ALWAYS_LOAD_OPROM I guess Kconfig isn’t selecting this automatically when selecting selecting `ALWAYS_RUN_OPROM`?
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 216: select FRAMEBUFFER_SET_VESA_MODE Why does this need to be selected?
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/romst... File src/soc/intel/icelake/romstage/fsp_params.c:
PS9: If the changes to this file would be in a separate commit, could it submitted already?
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/romst... PS9, Line 40: /* Set IGD stolen size to 60MB. */ Off-topic: Why 60 MB? Shouldn’t this also be configurable?
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/romst... PS9, Line 52: dev = pcidev_on_root(PCH_DEV_SLOT_ESPI, 3); Please do this in a separate commit.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 10:
(6 comments)
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... File src/soc/intel/icelake/Kconfig:
PS9:
`HAVE_EXT_GFX` should be put into `src/device/Kconfig`, so it’s common between all boards. […]
I have explained below, please check, VGA_ROM_RUN is just to check below code if need to look for OpRom
/** Default handler: only runs the relevant PCI BIOS. */ void pci_dev_init(struct device *dev) { struct rom_header *rom, *ram;
if (!CONFIG(VGA_ROM_RUN)) return;
/* Only execute VGA ROMs. */ if (((dev->class >> 8) != PCI_CLASS_DISPLAY_VGA)) return;
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 212: bool
Is it selected by default or not?
no, its not default enable
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 213: select ALWAYS_LOAD_OPROM
I guess Kconfig isn’t selecting this automatically when selecting selecting `ALWAYS_RUN_OPROM`?
i don't get you. Once mb selects HAVE_EXT_GFX kconfig option, then only required kconfig will get auto selected
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 216: select FRAMEBUFFER_SET_VESA_MODE
Why does this need to be selected?
CONFIG_VGA_ROM_RUN is just to check if cb will look for OpRom or not. thats all there is not much scope of this config. followed by there are dependent kconfig's like ALWAYS_LOAD_OPROM or ALWAYS_RUN_OPROM without those OpRom won't load or execute. if user miss to select any of those then purpose won't be solve. Hence i was trying to create a single kconfig which helps to select all required supported kconfig
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/romst... File src/soc/intel/icelake/romstage/fsp_params.c:
PS9:
If the changes to this file would be in a separate commit, could it submitted already?
i didn't get u
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/romst... PS9, Line 40: /* Set IGD stolen size to 60MB. */
Off-topic: Why 60 MB? Shouldn’t this also be configurable?
its UPD choice right hence we have decided to set 60MB
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 10:
(3 comments)
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... File src/soc/intel/icelake/Kconfig:
PS9:
I have explained below, please check, VGA_ROM_RUN is just to check below code if need to look for Op […]
Thank you for the explanation. In my opinion VGA_ROM_RUN should imply `ALWAYS_RUN_OPROM`. I do not see how the current logic is helpful. Could you rather fix that up?
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 216: select FRAMEBUFFER_SET_VESA_MODE
CONFIG_VGA_ROM_RUN is just to check if cb will look for OpRom or not. […]
Sorry, for being unclear. I meant `FRAMEBUFFER_SET_VESA_MODE` specifically.
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/romst... File src/soc/intel/icelake/romstage/fsp_params.c:
PS9:
i didn't get u
Can the changes to this file be put into a separate commit, so that the discussion about the Kconfig options do not hold up this functional change (which does not use the Kconfig symbol)?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 10:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... File src/soc/intel/icelake/Kconfig:
PS9:
Thank you for the explanation. In my opinion VGA_ROM_RUN should imply `ALWAYS_RUN_OPROM`. […]
its selecting ALWAYS_RUN_OPROM but not selecting ALWAYS_RUN_OPROM sas i remember last while debugging
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 216: select FRAMEBUFFER_SET_VESA_MODE
Sorry, for being unclear. I meant `FRAMEBUFFER_SET_VESA_MODE` specifically.
Its because with depthcharge pre os screen it needs GFX framebuffer in VESA mode to dump bmlblk image
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Add option to enable display over PCI external GFX ......................................................................
Patch Set 10:
(5 comments)
There seems to be no need for the Kconfig. Subrata please don't push reviews with words like `required` or `need`. Or at least check the code before you talk about require- ments to avoid lies.
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... File src/soc/intel/icelake/Kconfig:
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 211: config HAVE_EXT_GFX
can you please help to write the same for all PCIE card vendors NVDIA, AMD, ATI whoever and say please stop selling cards with OpRom, that would help.
If they wanted to do that. I would help.
Right now my problem statement is that, i need to bring display over PCIE GFX for my board and i need to run OpRom, what is my priority.
That's just fine. But you are mixing too many things. The name HAVE_EXT_GFX implies that it is generally useful for external graphics cards. But it is actually only useful for your specific case. If you would just find a better name and place for it, you would be much closer to get it through a review.
I have protection taken to lock down all my protected range registers what any OpRom can manipulate, i have all Chipset registers lockdown as part of POSTBOOT_SAI thats the whole purpose of security that no 3rd party code can break into chipset space.
That's the problem with Intel folks. There's some guide that tells what to lock before executing oproms. It seems it makes people believe that there is nothing else to consider and that they don't have to think. But that's just wrong. If you pass execution to an OpRom, it can do what it wants. It doesn't have to return execution to coreboot. For instance, it can bypass any signature veri- fication of an OS kernel and just load whatever OS it likes. Those security guides are still useful to understand the basics, but they can't protect deve- lopers from other mistakes.
What you mean by not running OpRom code. still today there are many PC running thousands of OpRom code, all are BAD ?
Obviously, yes. You might know that most of these PCs running OpRoms don't have open-source firmware. Of course, that's all BAD. Maybe nobody told you, but be- fore FSP, we could boot Intel PCs without any proprietary code. Everything Intel does seems to be a step backwards.
and they should stop their business. Please try to be reasonable with other patches and understand what is the need of the hour.
The need of the hour is to calm and slow down. There is exactly one thing to do Kconfig wise, select VGA_ROM_RUN. That you call the other options "required" only shows that you didn't take the time to understand them. So why should we hurry to review your half-backed patches?
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 213: select ALWAYS_LOAD_OPROM
i don't get you. […]
Not needed. Loading obviously is always implied by running it.
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 214: select ALWAYS_RUN_OPROM Not generally needed. It ignores the vboot decision to skip gfx init. It was added to help broken OS drivers that don't work if the firmware didn't run it.
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 215: select ON_DEVICE_ROM_LOAD defaults to y anyway
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 216: select FRAMEBUFFER_SET_VESA_MODE
Its because with depthcharge pre os screen it needs GFX framebuffer in VESA mode to dump bmlblk imag […]
That's just your use case. How about a `default y if CHROMEOS` instead?
Hello Patrick Rudolph, Aamir Bohra, Patrick Rudolph, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33739
to look at the new patch set (#11).
Change subject: soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree ......................................................................
soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree
This patch sets required FSP UPDs to skip IGD initialziation if devicetree has disable IGD.
Change-Id: I34a02bff112f922cabd48c23bc76370892ec62d9 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/fsp_params.c M src/soc/intel/icelake/romstage/fsp_params.c 2 files changed, 28 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/33739/11
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree ......................................................................
Patch Set 11:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... File src/soc/intel/icelake/Kconfig:
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 215: select ON_DEVICE_ROM_LOAD
defaults to y anyway
Ack
https://review.coreboot.org/c/coreboot/+/33739/9/src/soc/intel/icelake/Kconf... PS9, Line 216: select FRAMEBUFFER_SET_VESA_MODE
That's just your use case. […]
Done
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree ......................................................................
Patch Set 11: Code-Review+2
The question rises, why was this broken platform code submitted and needs to be fixed now?
Hello Patrick Rudolph, Aamir Bohra, Wonkyu Kim, Patrick Rudolph, Maulik V Vaghela, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33739
to look at the new patch set (#12).
Change subject: soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree ......................................................................
soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree
This patch sets required FSP UPDs to skip IGD initialziation if devicetree has disable IGD.
Change-Id: I34a02bff112f922cabd48c23bc76370892ec62d9 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/fsp_params.c M src/soc/intel/icelake/romstage/fsp_params.c 2 files changed, 28 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/33739/12
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree ......................................................................
Patch Set 12:
Patch Set 11: Code-Review+2
The question rises, why was this broken platform code submitted and needs to be fixed now?
you are right, we never thought that IGD might also need to disable sometime and if it does, one needs to take care of these FSP-UPD. its good we can taking care of it now :)
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree ......................................................................
Patch Set 12:
Patch Set 12:
Patch Set 11: Code-Review+2
The question rises, why was this broken platform code submitted and needs to be fixed now?
you are right, we never thought that IGD might also need to disable sometime and if it does, one needs to take care of these FSP-UPD. its good we can taking care of it now :)
btw lost your +2 in meantime
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33739/12/src/soc/intel/icelake/fsp_... File src/soc/intel/icelake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/33739/12/src/soc/intel/icelake/fsp_... PS12, Line 87: * the FSP does not initialize this device This is written as a conclusion, not a statement, please fix.
https://review.coreboot.org/c/coreboot/+/33739/12/src/soc/intel/icelake/fsp_... PS12, Line 91: else { Please use braces in accordance with our coding style.
Hello Patrick Rudolph, Aamir Bohra, Wonkyu Kim, Patrick Rudolph, Maulik V Vaghela, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33739
to look at the new patch set (#13).
Change subject: soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree ......................................................................
soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree
This patch sets required FSP UPDs to skip IGD initialziation if devicetree has disable IGD.
Change-Id: I34a02bff112f922cabd48c23bc76370892ec62d9 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/fsp_params.c M src/soc/intel/icelake/romstage/fsp_params.c 2 files changed, 28 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/33739/13
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree ......................................................................
Patch Set 13:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33739/12/src/soc/intel/icelake/fsp_... File src/soc/intel/icelake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/33739/12/src/soc/intel/icelake/fsp_... PS12, Line 87: * the FSP does not initialize this device
This is written as a conclusion, not a statement, please fix.
Done
https://review.coreboot.org/c/coreboot/+/33739/12/src/soc/intel/icelake/fsp_... PS12, Line 91: else {
Please use braces in accordance with our coding style.
Done
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree ......................................................................
Patch Set 13: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree ......................................................................
Patch Set 13: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree ......................................................................
Patch Set 13: Code-Review-2
(2 comments)
Please take more care during review before you give +2.
https://review.coreboot.org/c/coreboot/+/33739/13/src/soc/intel/icelake/fsp_... File src/soc/intel/icelake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/33739/13/src/soc/intel/icelake/fsp_... PS13, Line 87: incase in case
https://review.coreboot.org/c/coreboot/+/33739/13/src/soc/intel/icelake/fsp_... PS13, Line 88: * in the devicetree.cb Please end sentences with a full stop.
Hello Patrick Rudolph, Aamir Bohra, Wonkyu Kim, Patrick Rudolph, Maulik V Vaghela, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33739
to look at the new patch set (#14).
Change subject: soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree ......................................................................
soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree
This patch sets required FSP UPDs to skip IGD initialziation if devicetree has disable IGD.
Change-Id: I34a02bff112f922cabd48c23bc76370892ec62d9 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/icelake/fsp_params.c M src/soc/intel/icelake/romstage/fsp_params.c 2 files changed, 28 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/39/33739/14
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/c/coreboot/+/33739/13/src/soc/intel/icelake/fsp_... File src/soc/intel/icelake/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/33739/13/src/soc/intel/icelake/fsp_... PS13, Line 87: incase
in case
Done
https://review.coreboot.org/c/coreboot/+/33739/13/src/soc/intel/icelake/fsp_... PS13, Line 88: * in the devicetree.cb
Please end sentences with a full stop.
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree ......................................................................
Patch Set 14: Code-Review+2
Aamir Bohra has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree ......................................................................
Patch Set 14: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree ......................................................................
Patch Set 14: -Code-Review
Wonkyu Kim has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree ......................................................................
Patch Set 14: Code-Review+2
Subrata Banik has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33739 )
Change subject: soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree ......................................................................
soc/intel/icelake: Update FSP UPDs if IGD is disable in devicetree
This patch sets required FSP UPDs to skip IGD initialziation if devicetree has disable IGD.
Change-Id: I34a02bff112f922cabd48c23bc76370892ec62d9 Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/33739 Reviewed-by: Furquan Shaikh furquan@google.com Reviewed-by: Aamir Bohra aamir.bohra@intel.com Reviewed-by: Wonkyu Kim wonkyu.kim@intel.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/icelake/fsp_params.c M src/soc/intel/icelake/romstage/fsp_params.c 2 files changed, 28 insertions(+), 6 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved Aamir Bohra: Looks good to me, approved 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 03b00d9..382b184 100644 --- a/src/soc/intel/icelake/fsp_params.c +++ b/src/soc/intel/icelake/fsp_params.c @@ -80,9 +80,20 @@
mainboard_silicon_init_params(params);
- params->PeiGraphicsPeimInit = 1; - params->GtFreqMax = 2; - params->CdClock = 3; + dev = pcidev_path_on_root(SA_DEVFN_IGD); + + if (!dev || !dev->enabled) { + /* + * Skip IGD initialization in FSP in case device is disabled + * in the devicetree.cb. + */ + params->PeiGraphicsPeimInit = 0; + } else { + params->PeiGraphicsPeimInit = 1; + params->GtFreqMax = 2; + params->CdClock = 3; + } + /* Unlock upper 8 bytes of RTC RAM */ params->PchLockDownRtcMemoryLock = 0;
diff --git a/src/soc/intel/icelake/romstage/fsp_params.c b/src/soc/intel/icelake/romstage/fsp_params.c index 4801bd9..89dc99a 100644 --- a/src/soc/intel/icelake/romstage/fsp_params.c +++ b/src/soc/intel/icelake/romstage/fsp_params.c @@ -25,11 +25,22 @@ const struct soc_intel_icelake_config *config) { unsigned int i; - const struct device *dev; + const struct device *dev = pcidev_path_on_root(SA_DEVFN_IGD); uint32_t mask = 0;
- /* Set IGD stolen size to 60MB. */ - m_cfg->IgdDvmt50PreAlloc = 0xFE; + if (!dev || !dev->enabled) { + /* + * Skip IGD initialization in FSP if device + * is disable in devicetree.cb. + */ + m_cfg->InternalGfx = 0; + m_cfg->IgdDvmt50PreAlloc = 0; + } else { + m_cfg->InternalGfx = 1; + /* Set IGD stolen size to 60MB. */ + m_cfg->IgdDvmt50PreAlloc = 0xFE; + } + m_cfg->TsegSize = CONFIG_SMM_TSEG_SIZE; m_cfg->IedSize = CONFIG_IED_REGION_SIZE; m_cfg->SaGv = config->SaGv;
-
Aamir Bohra (Code Review) -
Furquan Shaikh (Code Review)
-
Kyösti Mälkki (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Subrata Banik (Code Review)
-
Wonkyu Kim (Code Review)