Change in coreboot[master]: soc/intel/cannonlake: Set IgdDvmt50PreAlloc accordingly to InternalGfx
Christian Walter has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Set IgdDvmt50PreAlloc accordingly to InternalGfx ......................................................................
soc/intel/cannonlake: Set IgdDvmt50PreAlloc accordingly to InternalGfx
Set IgdDvmt50PreAlloc accordingly to InternalGfx. Also parse the InternalGfx which is coming from the devicetree.
Change-Id: I6f9e0f9855224614471d8ed23bf2a9786386ddca Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/soc/intel/cannonlake/romstage/fsp_params.c 1 file changed, 8 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/39454/1
diff --git a/src/soc/intel/cannonlake/romstage/fsp_params.c b/src/soc/intel/cannonlake/romstage/fsp_params.c index 3c5be30..3c4b547 100644 --- a/src/soc/intel/cannonlake/romstage/fsp_params.c +++ b/src/soc/intel/cannonlake/romstage/fsp_params.c @@ -33,8 +33,14 @@ uint32_t mask = 0; const struct device *dev = pcidev_path_on_root(PCH_DEVFN_ISH);
- /* Set IGD stolen size to 64MB. */ - m_cfg->IgdDvmt50PreAlloc = 2; + if (config->InternalGfx == 1) { + /* Set IGD stolen size to 64MB. */ + m_cfg->InternalGfx = 1; + m_cfg->IgdDvmt50PreAlloc = 2; + } else { + m_cfg->InternalGfx = 0; + m_cfg->IgdDvmt50PreAlloc = 0; + } m_cfg->TsegSize = CONFIG_SMM_TSEG_SIZE; m_cfg->IedSize = CONFIG_IED_REGION_SIZE; m_cfg->SaGv = config->SaGv;
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Set IgdDvmt50PreAlloc accordingly to InternalGfx ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39454/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39454/1//COMMIT_MSG@7 PS1, Line 7: accordingly according
https://review.coreboot.org/c/coreboot/+/39454/1//COMMIT_MSG@7 PS1, Line 7: soc/intel/cannonlake: Set IgdDvmt50PreAlloc accordingly to InternalGfx
soc/intel/cannonlake: Steal no memory for disabled IGD?
https://review.coreboot.org/c/coreboot/+/39454/1//COMMIT_MSG@9 PS1, Line 9: Also Why *Also*? Isn’t that implied by the first sentence.
https://review.coreboot.org/c/coreboot/+/39454/1//COMMIT_MSG@11 PS1, Line 11: 1. What does that fix? 2. Tested how?
https://review.coreboot.org/c/coreboot/+/39454/1/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/1/src/soc/intel/cannonlake/ro... PS1, Line 38: m_cfg->InternalGfx = 1; With this line missing, did this work before at all?
https://review.coreboot.org/c/coreboot/+/39454/1/src/soc/intel/cannonlake/ro... PS1, Line 43: } Why does
m_cfg->InternalGfx = config->InternalGfx; m_cfg->IgdDvmt50PreAlloc = (config->InternalGfx) ? 2 : 0;
not work?
Hello build bot (Jenkins), Nate DeSimone, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39454
to look at the new patch set (#2).
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
soc/intel/cannonlake: Steal no memory for disabled IGD
Set IgdDvmt50PreAlloc according to InternalGfx. Also parse the InternalGfx which is coming from the devicetree.
Change-Id: I6f9e0f9855224614471d8ed23bf2a9786386ddca Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/soc/intel/cannonlake/romstage/fsp_params.c 1 file changed, 8 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/39454/2
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 1:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39454/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39454/1//COMMIT_MSG@7 PS1, Line 7: soc/intel/cannonlake: Set IgdDvmt50PreAlloc accordingly to InternalGfx
soc/intel/cannonlake: Steal no memory for disabled IGD?
Ack
https://review.coreboot.org/c/coreboot/+/39454/1//COMMIT_MSG@7 PS1, Line 7: accordingly
according
Ack
https://review.coreboot.org/c/coreboot/+/39454/1//COMMIT_MSG@9 PS1, Line 9: Also
Why *Also*? Isn’t that implied by the first sentence.
Yeah - still the functionality was added.
https://review.coreboot.org/c/coreboot/+/39454/1//COMMIT_MSG@11 PS1, Line 11:
- What does that fix? […]
It does what it says. It sets IgdDvmt50PreAlloc to zero if InternalGfx is disabled. It is 'correct' to do it like this, otherwise the FSP would always allocate memory for the IGD even if it is disabled.
https://review.coreboot.org/c/coreboot/+/39454/1/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/1/src/soc/intel/cannonlake/ro... PS1, Line 38: m_cfg->InternalGfx = 1;
With this line missing, did this work before at all?
Right now it seems to work ONLY for CPUs with IGD. Still it was not able to disable the InternalGfx via UPDs because they did not get parsed yet.
https://review.coreboot.org/c/coreboot/+/39454/1/src/soc/intel/cannonlake/ro... PS1, Line 43: }
Why does […]
Would work.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39454/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39454/1//COMMIT_MSG@11 PS1, Line 11:
It does what it says. It sets IgdDvmt50PreAlloc to zero if InternalGfx is disabled. […]
Sorry, it was only clear after reading the diff. Your explanation hear is more precise what is actually done. Maybe use that instead.
Hello build bot (Jenkins), Nate DeSimone, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39454
to look at the new patch set (#3).
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
soc/intel/cannonlake: Steal no memory for disabled IGD
Set IgdDvmt50PreAlloc to zero if InternalGfx is disabled. It is 'correct' to do it like this, otherwise the FSP would always allocate memory for the IGD even if it is disabled.
Change-Id: I6f9e0f9855224614471d8ed23bf2a9786386ddca Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/soc/intel/cannonlake/romstage/fsp_params.c 1 file changed, 8 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/39454/3
Hello build bot (Jenkins), Nate DeSimone, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39454
to look at the new patch set (#4).
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
soc/intel/cannonlake: Steal no memory for disabled IGD
Set IgdDvmt50PreAlloc to zero if InternalGfx is disabled. It is 'correct' to do it like this, otherwise the FSP would always allocate memory for the IGD even if it is disabled.
Tested on: * CFL platform with IGD * CFL platform without IGD
Not working: * FSP-M crashes if InternalGfx is set to zero or CPU without IGD is used. Seems to be an error within the FSP-M.
Change-Id: I6f9e0f9855224614471d8ed23bf2a9786386ddca Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/soc/intel/cannonlake/romstage/fsp_params.c 1 file changed, 8 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/39454/4
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39454/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39454/1//COMMIT_MSG@9 PS1, Line 9: Also
Yeah - still the functionality was added.
Ack
https://review.coreboot.org/c/coreboot/+/39454/1/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/1/src/soc/intel/cannonlake/ro... PS1, Line 43: }
Would work.
Ack
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39454/4/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/4/src/soc/intel/cannonlake/ro... PS4, Line 36: InternalGfx Did you check all CFL/CML/CNL boards that they select InternalGfx=1? If they don't it would introduce a regression as they won't boot any more.
Hello build bot (Jenkins), Nate DeSimone, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39454
to look at the new patch set (#5).
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
soc/intel/cannonlake: Steal no memory for disabled IGD
Set IgdDvmt50PreAlloc to zero if InternalGfx is disabled. It is 'correct' to do it like this, otherwise the FSP would always allocate memory for the IGD even if it is disabled.
Tested on: * CFL platform with IGD * CFL platform without IGD
Not working: * FSP-M crashes if InternalGfx is set to zero or CPU without IGD is used. Seems to be an error within the FSP-M.
Change-Id: I6f9e0f9855224614471d8ed23bf2a9786386ddca Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/soc/intel/cannonlake/romstage/fsp_params.c 1 file changed, 8 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/39454/5
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39454/4/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/4/src/soc/intel/cannonlake/ro... PS4, Line 36: InternalGfx
Did you check all CFL/CML/CNL boards that they select InternalGfx=1? If they don't it would introduc […]
That would solve the problem - correct?
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39454/4/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/4/src/soc/intel/cannonlake/ro... PS4, Line 36: InternalGfx
That would solve the problem - correct?
Setting InternalGfx=1 on all affected platforms is required to not introduce a regression.
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 5:
Patch Set 5:
(1 comment)
It does set InternalGfx=1 for all affected platforms (when no InternalGfx is set).
Hello build bot (Jenkins), Nate DeSimone, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39454
to look at the new patch set (#6).
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
soc/intel/cannonlake: Steal no memory for disabled IGD
Set IgdDvmt50PreAlloc to zero if InternalGfx is disabled. It is 'correct' to do it like this, otherwise the FSP would always allocate memory for the IGD even if it is disabled.
Tested on: * CFL platform with IGD * CFL platform without IGD
Not working: * FSP-M crashes if InternalGfx is set to zero or CPU without IGD is used. Seems to be an error within the FSP-M.
Change-Id: I6f9e0f9855224614471d8ed23bf2a9786386ddca Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/soc/intel/cannonlake/romstage/fsp_params.c 1 file changed, 12 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/39454/6
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39454/6/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/6/src/soc/intel/cannonlake/ro... PS6, Line 36: if (config->InternalGfx == 0) { struct device *dev = pcidev_path_on_root(SA_DEVFN_IGD); if (config->InternalGfx == 0 || pci_read_config32(dev, PCI_VENDOR_ID) == 0xffff) {
https://review.coreboot.org/c/coreboot/+/39454/6/src/soc/intel/cannonlake/ro... PS6, Line 158: /* Fix broken FSPM */ add FSP version number
https://review.coreboot.org/c/coreboot/+/39454/6/src/soc/intel/cannonlake/ro... PS6, Line 159: if (config->InternalGfx == 0) m_cfg->InternalGfx == 0
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39454/6/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/6/src/soc/intel/cannonlake/ro... PS6, Line 36: if (config->InternalGfx == 0) {
struct device *dev = pcidev_path_on_root(SA_DEVFN_IGD); […]
Looking at skylake, it's even better. Instead of using config->InternalGfx the "pci 2.0" device in the devicetree.cb could be used to enable/disable graphics. Something similar to:
struct device *dev = pcidev_path_on_root(SA_DEVFN_IGD); if (!dev || !dev->enabled || (dev && pci_read_config32(dev, PCI_VENDOR_ID) == 0xffff)) )
Hello build bot (Jenkins), Nate DeSimone, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39454
to look at the new patch set (#7).
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
soc/intel/cannonlake: Steal no memory for disabled IGD
Set IgdDvmt50PreAlloc to zero if InternalGfx is disabled. It is 'correct' to do it like this, otherwise the FSP would always allocate memory for the IGD even if it is disabled.
Tested on: * CFL platform with IGD * CFL platform without IGD
Not working: * FSP-M crashes if InternalGfx is set to zero or CPU without IGD is used. Seems to be an error within the FSP-M.
Change-Id: I6f9e0f9855224614471d8ed23bf2a9786386ddca Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/soc/intel/cannonlake/romstage/fsp_params.c 1 file changed, 17 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/39454/7
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 7:
(2 comments)
Rebasing on master should fix the build failure.
https://review.coreboot.org/c/coreboot/+/39454/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39454/7//COMMIT_MSG@18 PS7, Line 18: * FSP-M crashes if InternalGfx is set to zero or CPU without IGD is needs update that a workaround has been found
https://review.coreboot.org/c/coreboot/+/39454/7/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/7/src/soc/intel/cannonlake/ro... PS7, Line 39: SA_DEV_IGD dev
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 7:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39454/7//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39454/7//COMMIT_MSG@18 PS7, Line 18: * FSP-M crashes if InternalGfx is set to zero or CPU without IGD is
needs update that a workaround has been found
Ack
https://review.coreboot.org/c/coreboot/+/39454/6/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/6/src/soc/intel/cannonlake/ro... PS6, Line 36: if (config->InternalGfx == 0) {
Looking at skylake, it's even better. […]
Ack
https://review.coreboot.org/c/coreboot/+/39454/6/src/soc/intel/cannonlake/ro... PS6, Line 158: /* Fix broken FSPM */
add FSP version number
Ack
https://review.coreboot.org/c/coreboot/+/39454/6/src/soc/intel/cannonlake/ro... PS6, Line 159: if (config->InternalGfx == 0)
m_cfg->InternalGfx == 0
Ack
https://review.coreboot.org/c/coreboot/+/39454/7/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/7/src/soc/intel/cannonlake/ro... PS7, Line 39: SA_DEV_IGD
dev
Ack
Hello build bot (Jenkins), Nate DeSimone, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39454
to look at the new patch set (#8).
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
soc/intel/cannonlake: Steal no memory for disabled IGD
Set IgdDvmt50PreAlloc to zero if InternalGfx is disabled. It is 'correct' to do it like this, otherwise the FSP would always allocate memory for the IGD even if it is disabled.
Tested on: * CFL platform with IGD * CFL platform without IGD
Change-Id: I6f9e0f9855224614471d8ed23bf2a9786386ddca Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/soc/intel/cannonlake/romstage/fsp_params.c 1 file changed, 17 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/39454/8
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 8: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 8:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39454/8/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/8/src/soc/intel/cannonlake/ro... PS8, Line 38: (dev && (pci_read_config16(SA_DEV_IGD, PCI_VENDOR_ID) == 0xffff))) { Why is this extra read needed?
https://review.coreboot.org/c/coreboot/+/39454/8/src/soc/intel/cannonlake/ro... PS8, Line 38: dev Isn’t that guaranteed by the `!dev` in the beginning?
https://review.coreboot.org/c/coreboot/+/39454/8/src/soc/intel/cannonlake/ro... PS8, Line 163: tconfig->PanelPowerEnable = 0; This is not mentioned in the commit message? Any URL for the FSP bug report?
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 8:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39454/8/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/8/src/soc/intel/cannonlake/ro... PS8, Line 38: (dev && (pci_read_config16(SA_DEV_IGD, PCI_VENDOR_ID) == 0xffff))) {
Why is this extra read needed?
IMHO a device can be enabled in the devicetree but physically not be there. Correct me if I am wrong. If the CPU has no IGD the read will return 0xffff. Otherwise the FSP would crash if the device is enabled in the devicetree but the CPU has not IGD.
https://review.coreboot.org/c/coreboot/+/39454/8/src/soc/intel/cannonlake/ro... PS8, Line 38: dev
Isn’t that guaranteed by the `!dev` in the beginning?
See other message.
https://review.coreboot.org/c/coreboot/+/39454/8/src/soc/intel/cannonlake/ro... PS8, Line 163: tconfig->PanelPowerEnable = 0;
This is not mentioned in the commit message? Any URL for the FSP bug report?
https://github.com/IntelFsp/FSP/issues/49
I can add it to the commit message - But I doubt that this will be fixed.
Hello Praveen Hodagatta pranesh, Lijian Zhao, build bot (Jenkins), Jamie Chen, Nate DeSimone, Maulik V Vaghela, John Zhao, Duncan Laurie, Aamir Bohra, Kane Chen, Rizwan Qureshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39454
to look at the new patch set (#9).
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
soc/intel/cannonlake: Steal no memory for disabled IGD
Set IgdDvmt50PreAlloc to zero if InternalGfx is disabled. It is 'correct' to do it like this, otherwise the FSP would always allocate memory for the IGD even if it is disabled. In addition the FSP enables the graphics panel even if no IGD is present which leads to a crashing FSP. Thus, if no IGD is present we switch of the panel via UPDs.
Tested on: * CFL platform with IGD * CFL platform without IGD
Change-Id: I6f9e0f9855224614471d8ed23bf2a9786386ddca Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/soc/intel/cannonlake/romstage/fsp_params.c 1 file changed, 17 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/39454/9
Patrick Rudolph has uploaded a new patch set (#10) to the change originally created by Christian Walter. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
soc/intel/cannonlake: Steal no memory for disabled IGD
Set IgdDvmt50PreAlloc to zero if InternalGfx is disabled. It is 'correct' to do it like this, otherwise the FSP would always allocate memory for the IGD even if it is disabled. In addition the FSP enables the graphics panel even if no IGD is present which leads to a crashing FSP. Thus, if no IGD is present we switch of the panel via UPDs.
Also apply a workaround to prevent a crash, see https://github.com/IntelFsp/FSP/issues/49 for details.
Tested on: * CFL platform with IGD * CFL platform without IGD
Change-Id: I6f9e0f9855224614471d8ed23bf2a9786386ddca Signed-off-by: Christian Walter christian.walter@9elements.com --- M Documentation/soc/intel/fsp/index.md M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 27 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/39454/10
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39454/8/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/8/src/soc/intel/cannonlake/ro... PS8, Line 38: (dev && (pci_read_config16(SA_DEV_IGD, PCI_VENDOR_ID) == 0xffff))) {
IMHO a device can be enabled in the devicetree but physically not be there. […]
Explained in a comment.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 10: Code-Review+2
Hello build bot (Jenkins), Lijian Zhao, Jamie Chen, Patrick Rudolph, Maulik V Vaghela, Duncan Laurie, John Zhao, Aamir Bohra, Kane Chen, Patrick Rudolph, Praveen Hodagatta pranesh, Nate DeSimone, Rizwan Qureshi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39454
to look at the new patch set (#11).
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
soc/intel/cannonlake: Steal no memory for disabled IGD
Set IgdDvmt50PreAlloc to zero if InternalGfx is disabled. It is 'correct' to do it like this, otherwise the FSP would always allocate memory for the IGD even if it is disabled. In addition the FSP enables the graphics panel even if no IGD is present which leads to a crashing FSP. Thus, if no IGD is present we switch of the panel via UPDs.
Tested on: * CFL platform with IGD * CFL platform without IGD
Change-Id: I6f9e0f9855224614471d8ed23bf2a9786386ddca Signed-off-by: Christian Walter christian.walter@9elements.com --- M src/soc/intel/cannonlake/romstage/fsp_params.c 1 file changed, 17 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/39454/11
Hello build bot (Jenkins), Lijian Zhao, Jamie Chen, Patrick Rudolph, Maulik V Vaghela, Duncan Laurie, John Zhao, Aamir Bohra, Kane Chen, Patrick Rudolph, Praveen Hodagatta pranesh, Nate DeSimone, Rizwan Qureshi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39454
to look at the new patch set (#12).
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
soc/intel/cannonlake: Steal no memory for disabled IGD
Set IgdDvmt50PreAlloc to zero if InternalGfx is disabled. It is 'correct' to do it like this, otherwise the FSP would always allocate memory for the IGD even if it is disabled. In addition the FSP enables the graphics panel even if no IGD is present which leads to a crashing FSP. Thus, if no IGD is present we switch of the panel via UPDs.
Also apply a workaround to prevent a crash, see https://github.com/IntelFsp/FSP/issues/49 for details.
Tested on: * CFL platform with IGD * CFL platform without IGD
Change-Id: I6f9e0f9855224614471d8ed23bf2a9786386ddca Signed-off-by: Christian Walter christian.walter@9elements.com --- M Documentation/soc/intel/fsp/index.md M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 27 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/39454/12
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 12:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39454/8/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/8/src/soc/intel/cannonlake/ro... PS8, Line 38: dev
See other message.
Done
https://review.coreboot.org/c/coreboot/+/39454/8/src/soc/intel/cannonlake/ro... PS8, Line 38: (dev && (pci_read_config16(SA_DEV_IGD, PCI_VENDOR_ID) == 0xffff))) {
Explained in a comment.
Patrick, do you mean to add a comment to the code?
https://review.coreboot.org/c/coreboot/+/39454/8/src/soc/intel/cannonlake/ro... PS8, Line 163: tconfig->PanelPowerEnable = 0;
Thank you for updating the commit message.
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39454/8/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/8/src/soc/intel/cannonlake/ro... PS8, Line 38: (dev && (pci_read_config16(SA_DEV_IGD, PCI_VENDOR_ID) == 0xffff))) {
Patrick, do you mean to add a comment to the code?
Not sure why we would need an extra comment to explain this.
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39454/8/src/soc/intel/cannonlake/ro... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/8/src/soc/intel/cannonlake/ro... PS8, Line 38: (dev && (pci_read_config16(SA_DEV_IGD, PCI_VENDOR_ID) == 0xffff))) {
Not sure why we would need an extra comment to explain this.
Ack
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 12: Code-Review+1
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 12: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39454/12/src/soc/intel/cannonlake/r... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/12/src/soc/intel/cannonlake/r... PS12, Line 38: active activate?
Hello build bot (Jenkins), Lijian Zhao, Jamie Chen, Patrick Rudolph, Maulik V Vaghela, Duncan Laurie, John Zhao, Aamir Bohra, Kane Chen, Patrick Rudolph, Praveen Hodagatta pranesh, Nate DeSimone, Rizwan Qureshi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39454
to look at the new patch set (#13).
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
soc/intel/cannonlake: Steal no memory for disabled IGD
Set IgdDvmt50PreAlloc to zero if InternalGfx is disabled. It is 'correct' to do it like this, otherwise the FSP would always allocate memory for the IGD even if it is disabled. In addition the FSP enables the graphics panel even if no IGD is present which leads to a crashing FSP. Thus, if no IGD is present we switch of the panel via UPDs.
Also apply a workaround to prevent a crash, see https://github.com/IntelFsp/FSP/issues/49 for details.
Tested on: * CFL platform with IGD * CFL platform without IGD
Change-Id: I6f9e0f9855224614471d8ed23bf2a9786386ddca Signed-off-by: Christian Walter christian.walter@9elements.com --- M Documentation/soc/intel/fsp/index.md M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 27 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/39454/13
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 13:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... PS13, Line 42: pci_read_config16(SA_DEV_IGD, PCI_VENDOR_ID) == 0xffff)) if dev and dev->enabled are true, can this still be false? And wouldn't we have to mark the device as disabled then?
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... PS13, Line 42: dev not needed
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... PS13, Line 98: dev "dev" was declared const above...
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39454/12/src/soc/intel/cannonlake/r... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/12/src/soc/intel/cannonlake/r... PS12, Line 38: active
activate?
Done
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 13:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... PS13, Line 42: pci_read_config16(SA_DEV_IGD, PCI_VENDOR_ID) == 0xffff))
if dev and dev->enabled are true, can this still be false? And wouldn't we have to mark the device a […]
If dev, but there is no IGD (Read from PCI_VENDOR_ID == 0xffff) it should be disabled, otherwise it will crash.
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... PS13, Line 42: dev
not needed
Why? We should only do this if dev. If dev is not enabled anyways we do need to read this.
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... PS13, Line 98: dev
"dev" was declared const above...
Seems like a common rule to override this.. e.g. dev = pcidev_path_on_root(PCH_DEVFN_HDA); dev = pcidev_path_on_root(SA_DEVFN_IPU);
so either we have to fix all of these or _ignore_ the const.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 13:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... PS13, Line 39: * to prevent a crash in FSP-M. Please don't add comments about what you do. AIUI, it was asked for a comment, because the details why it is done in this particular way were unclear (not why it is done at all).
So
!dev || !dev->enabled
this is to check if the device was unmentioned in the devicetree or explicitly turned `off`,
(dev &&
this is redundant and should be removed. By the evaluation rules of `||` the right-hand side is only evaluated if the left-hand side is false. If the left-hand side here (`!dev || !dev->enabled`) is false, it implies `dev` is non-NULL.
(pci_read_config16(SA_DEV_IGD, PCI_VENDOR_ID) == 0xffff)
this checks if the device is actually there. If it isn't, the devicetree is inconsistent (says `on` for a device that doesn't even exist). IMHO, this is the part that needs to be commented. Why assume inconsistency? because we might not know at compile time if the CPU that is put into a socket supports it. <-- this I can only assume, because it isn't mentioned in the comment.
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... PS13, Line 167: tconfig->PanelPowerEnable = 0; Please merge this with the original check above. I know that involves passing `tconfig`, but that's just how things are with FSP...
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... PS13, Line 98: dev
"dev" was declared const above...
No, it wasn't. The data pointed to is declared `const`.
const struct device *dev
vs
struct device *const dev
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... PS13, Line 39: * to prevent a crash in FSP-M.
Please don't add comments about what you do. AIUI, it was asked for a comment, […]
Just another thought, it might be easier to read if we'd check the positive case:
if (dev && dev->enabled && pci_read_config16(SA_DEV_IGD, PCI_VENDOR_ID) != 0xffff)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 13:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39454/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39454/13//COMMIT_MSG@9 PS13, Line 9: It is nit: use `it's` so that this line is under 72 chars
https://review.coreboot.org/c/coreboot/+/39454/13//COMMIT_MSG@13 PS13, Line 13: of ofF
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... PS13, Line 42: dev
Why? We should only do this if dev. If dev is not enabled anyways we do need to read this.
The `||` operator is short-circuiting. If `dev` is null, the first check for `!dev` will succeed and the block of code will be run. If the check is false, it means `dev` is true, so it is redundant.
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... PS13, Line 42: pci_read_config16(SA_DEV_IGD, PCI_VENDOR_ID) == 0xffff))
If dev, but there is no IGD (Read from PCI_VENDOR_ID == 0xffff) it should be disabled, otherwise it […]
Ack
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... PS13, Line 98: dev
"dev" was declared const above... […]
Ack, forgot my own rule to understand const on pointers... Silly me.
Hello build bot (Jenkins), Lijian Zhao, Jamie Chen, Patrick Rudolph, Maulik V Vaghela, Duncan Laurie, John Zhao, Aamir Bohra, Kane Chen, Patrick Rudolph, Praveen Hodagatta pranesh, Nate DeSimone, Rizwan Qureshi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39454
to look at the new patch set (#14).
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
soc/intel/cannonlake: Steal no memory for disabled IGD
Set IgdDvmt50PreAlloc to zero if InternalGfx is disabled. It's 'correct' to do it like this, otherwise the FSP would always allocate memory for the IGD even if it is disabled. In addition the FSP enables the graphics panel even if no IGD is present which leads to a crashing FSP. Thus, if no IGD is present we switch off the panel via UPDs.
Also apply a workaround to prevent a crash, see https://github.com/IntelFsp/FSP/issues/49 for details.
Tested on: * CFL platform with IGD * CFL platform without IGD
Change-Id: I6f9e0f9855224614471d8ed23bf2a9786386ddca Signed-off-by: Christian Walter christian.walter@9elements.com --- M Documentation/soc/intel/fsp/index.md M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 27 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/39454/14
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 14:
(5 comments)
https://review.coreboot.org/c/coreboot/+/39454/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39454/13//COMMIT_MSG@9 PS13, Line 9: It is
nit: use `it's` so that this line is under 72 chars
Ack
https://review.coreboot.org/c/coreboot/+/39454/13//COMMIT_MSG@13 PS13, Line 13: of
ofF
Ack
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... PS13, Line 39: * to prevent a crash in FSP-M.
Just another thought, it might be easier to read if we'd check the positive […]
Ack
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... PS13, Line 42: dev
The `||` operator is short-circuiting. […]
Ack
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... PS13, Line 167: tconfig->PanelPowerEnable = 0;
Please merge this with the original check above. I know that involves passing […]
Why do we need to merge this? This would be that we have yo update the implementation which is always the same for soc_memory_init_params (except Apollolake).
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 14: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... PS13, Line 167: tconfig->PanelPowerEnable = 0;
Why do we need to merge this? This would be that we have yo update the implementation which is alway […]
This could be done in a separate change, IMHO.
https://review.coreboot.org/c/coreboot/+/39454/14/src/soc/intel/cannonlake/r... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/14/src/soc/intel/cannonlake/r... PS14, Line 28: * to prevent a crash in FSP-M. Maybe also mention that disabling the panel power controls in tconfig is required?
https://review.coreboot.org/c/coreboot/+/39454/14/src/soc/intel/cannonlake/r... PS14, Line 31: pci_read_config16(SA_DEV_IGD, PCI_VENDOR_ID) != 0xffff) { I thiiink it fits in one line now
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39454/14/src/soc/intel/cannonlake/r... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/14/src/soc/intel/cannonlake/r... PS14, Line 28: * to prevent a crash in FSP-M.
Maybe also mention that disabling the panel power controls in tconfig is required?
Ack
https://review.coreboot.org/c/coreboot/+/39454/14/src/soc/intel/cannonlake/r... PS14, Line 31: pci_read_config16(SA_DEV_IGD, PCI_VENDOR_ID) != 0xffff) {
I thiiink it fits in one line now
Ack
Hello build bot (Jenkins), Lijian Zhao, Jamie Chen, Patrick Rudolph, Maulik V Vaghela, Duncan Laurie, John Zhao, Angel Pons, Aamir Bohra, Kane Chen, Patrick Rudolph, Praveen Hodagatta pranesh, Nate DeSimone, Rizwan Qureshi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39454
to look at the new patch set (#15).
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
soc/intel/cannonlake: Steal no memory for disabled IGD
Set IgdDvmt50PreAlloc to zero if InternalGfx is disabled. It's 'correct' to do it like this, otherwise the FSP would always allocate memory for the IGD even if it is disabled. In addition the FSP enables the graphics panel even if no IGD is present which leads to a crashing FSP. Thus, if no IGD is present we switch off the panel via UPDs.
Also apply a workaround to prevent a crash, see https://github.com/IntelFsp/FSP/issues/49 for details.
Tested on: * CFL platform with IGD * CFL platform without IGD
Change-Id: I6f9e0f9855224614471d8ed23bf2a9786386ddca Signed-off-by: Christian Walter christian.walter@9elements.com --- M Documentation/soc/intel/fsp/index.md M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 26 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/39454/15
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 15:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... PS13, Line 167: tconfig->PanelPowerEnable = 0;
This could be done in a separate change, IMHO.
Done
Hello build bot (Jenkins), Lijian Zhao, Jamie Chen, Patrick Rudolph, Maulik V Vaghela, Duncan Laurie, John Zhao, Angel Pons, Aamir Bohra, Kane Chen, Patrick Rudolph, Praveen Hodagatta pranesh, Nate DeSimone, Rizwan Qureshi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39454
to look at the new patch set (#16).
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
soc/intel/cannonlake: Steal no memory for disabled IGD
Set IgdDvmt50PreAlloc to zero if InternalGfx is disabled. It's 'correct' to do it like this, otherwise the FSP would always allocate memory for the IGD even if it is disabled. In addition the FSP enables the graphics panel even if no IGD is present which leads to a crashing FSP. Thus, if no IGD is present we switch off the panel via UPDs.
Also apply a workaround to prevent a crash, see https://github.com/IntelFsp/FSP/issues/49 for details.
Tested on: * CFL platform with IGD * CFL platform without IGD
Change-Id: I6f9e0f9855224614471d8ed23bf2a9786386ddca Signed-off-by: Christian Walter christian.walter@9elements.com --- M Documentation/soc/intel/fsp/index.md M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 28 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/39454/16
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 16: Code-Review+2
(4 comments)
Last round, I promise! 😄
https://review.coreboot.org/c/coreboot/+/39454/16//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39454/16//COMMIT_MSG@12 PS16, Line 12: panel panel *power*
https://review.coreboot.org/c/coreboot/+/39454/16//COMMIT_MSG@15 PS16, Line 15: Also apply a workaround to prevent a crash, see : https://github.com/IntelFsp/FSP/issues/49 : for details. Maybe word it this way:
Refer to this issue on IntelFSP for details: https://github.com/IntelFsp/FSP/issues/49
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/13/src/soc/intel/cannonlake/r... PS13, Line 167: tconfig->PanelPowerEnable = 0;
Done
Oh, it was much easier than I thought. I didn't check where `soc_memory_init_params` was called from
https://review.coreboot.org/c/coreboot/+/39454/16/src/soc/intel/cannonlake/r... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/16/src/soc/intel/cannonlake/r... PS16, Line 30: power panel "panel power" ?
Hello build bot (Jenkins), Lijian Zhao, Jamie Chen, Patrick Rudolph, Maulik V Vaghela, Duncan Laurie, John Zhao, Angel Pons, Aamir Bohra, Kane Chen, Patrick Rudolph, Praveen Hodagatta pranesh, Nate DeSimone, Rizwan Qureshi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39454
to look at the new patch set (#17).
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
soc/intel/cannonlake: Steal no memory for disabled IGD
Set IgdDvmt50PreAlloc to zero if InternalGfx is disabled. It's 'correct' to do it like this, otherwise the FSP would always allocate memory for the IGD even if it is disabled. In addition the FSP enables the graphics panel power even if no IGD is present which leads to a crashing FSP. Thus, if no IGD is present we switch off the panel via UPDs.
Refer to this issue on IntelFSP for details: https://github.com/IntelFsp/FSP/issues/49
Tested on: * CFL platform with IGD * CFL platform without IGD
Change-Id: I6f9e0f9855224614471d8ed23bf2a9786386ddca Signed-off-by: Christian Walter christian.walter@9elements.com --- M Documentation/soc/intel/fsp/index.md M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 28 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/54/39454/17
Christian Walter has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 17:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39454/16//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/39454/16//COMMIT_MSG@12 PS16, Line 12: panel
panel *power*
Ack
https://review.coreboot.org/c/coreboot/+/39454/16//COMMIT_MSG@15 PS16, Line 15: Also apply a workaround to prevent a crash, see : https://github.com/IntelFsp/FSP/issues/49 : for details.
Maybe word it this way: […]
Ack
https://review.coreboot.org/c/coreboot/+/39454/16/src/soc/intel/cannonlake/r... File src/soc/intel/cannonlake/romstage/fsp_params.c:
https://review.coreboot.org/c/coreboot/+/39454/16/src/soc/intel/cannonlake/r... PS16, Line 30: power panel
"panel power" ?
Ack
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 17: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 17: Code-Review+1
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 17: Code-Review+2
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
soc/intel/cannonlake: Steal no memory for disabled IGD
Set IgdDvmt50PreAlloc to zero if InternalGfx is disabled. It's 'correct' to do it like this, otherwise the FSP would always allocate memory for the IGD even if it is disabled. In addition the FSP enables the graphics panel power even if no IGD is present which leads to a crashing FSP. Thus, if no IGD is present we switch off the panel via UPDs.
Refer to this issue on IntelFSP for details: https://github.com/IntelFsp/FSP/issues/49
Tested on: * CFL platform with IGD * CFL platform without IGD
Change-Id: I6f9e0f9855224614471d8ed23bf2a9786386ddca Signed-off-by: Christian Walter christian.walter@9elements.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/39454 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net Reviewed-by: Nico Huber nico.h@gmx.de --- M Documentation/soc/intel/fsp/index.md M src/soc/intel/cannonlake/romstage/fsp_params.c 2 files changed, 28 insertions(+), 5 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Paul Menzel: Looks good to me, but someone else must approve Angel Pons: Looks good to me, approved
diff --git a/Documentation/soc/intel/fsp/index.md b/Documentation/soc/intel/fsp/index.md index 769b98b..912c44b 100644 --- a/Documentation/soc/intel/fsp/index.md +++ b/Documentation/soc/intel/fsp/index.md @@ -45,6 +45,11 @@ * Workaround: Disable internal UART manually after calling FSP * Issue on public tracker: [Issue 10]
+### CoffeeLakeFsp +* Disabling the internal graphics causes a crash in FSP-M + * 7.0.68.40 and older version + * Workaround: Set "tconfig->PanelPowerEnable = 0" + * Issue on public tracker: [Issue 49]
## Open Source Intel FSP specification
@@ -72,4 +77,5 @@ [Issue 22]: https://github.com/IntelFsp/FSP/issues/22 [Issue 35]: https://github.com/IntelFsp/FSP/issues/35 [Issue 41]: https://github.com/IntelFsp/FSP/issues/41 +[Issue 49]: https://github.com/IntelFsp/FSP/issues/49
diff --git a/src/soc/intel/cannonlake/romstage/fsp_params.c b/src/soc/intel/cannonlake/romstage/fsp_params.c index c84e351..010d152c 100644 --- a/src/soc/intel/cannonlake/romstage/fsp_params.c +++ b/src/soc/intel/cannonlake/romstage/fsp_params.c @@ -2,6 +2,8 @@ /* This file is part of the coreboot project. */
#include <assert.h> +#include <device/pci_def.h> +#include <device/pci.h> #include <cpu/x86/msr.h> #include <console/console.h> #include <fsp/util.h> @@ -15,14 +17,28 @@
#include "../chip.h"
-static void soc_memory_init_params(FSP_M_CONFIG *m_cfg, const config_t *config) +static void soc_memory_init_params(FSPM_UPD *mupd, const config_t *config) { + FSP_M_CONFIG *m_cfg = &mupd->FspmConfig; + FSP_M_TEST_CONFIG *tconfig = &mupd->FspmTestConfig; + unsigned int i; uint32_t mask = 0; - const struct device *dev = pcidev_path_on_root(PCH_DEVFN_ISH); + const struct device *dev = pcidev_path_on_root(SA_DEVFN_IGD);
- /* Set IGD stolen size to 64MB. */ - m_cfg->IgdDvmt50PreAlloc = 2; + /* + * Probe for no IGD and disable InternalGfx and panel power to prevent a + * crash in FSP-M. + */ + if (dev && dev->enabled && pci_read_config16(SA_DEV_IGD, PCI_VENDOR_ID) != 0xffff) { + /* Set IGD stolen size to 64MB. */ + m_cfg->InternalGfx = 1; + m_cfg->IgdDvmt50PreAlloc = 2; + } else { + m_cfg->InternalGfx = 0; + m_cfg->IgdDvmt50PreAlloc = 0; + tconfig->PanelPowerEnable = 0; + } m_cfg->TsegSize = CONFIG_SMM_TSEG_SIZE; m_cfg->IedSize = CONFIG_IED_REGION_SIZE; m_cfg->SaGv = config->SaGv; @@ -71,6 +87,7 @@ m_cfg->CpuRatio = (flex_ratio.lo >> 8) & 0xff; }
+ dev = pcidev_path_on_root(PCH_DEVFN_ISH); /* If ISH is enabled, enable ISH elements */ if (!dev) m_cfg->PchIshEnable = 0; @@ -122,7 +139,7 @@ FSP_M_CONFIG *m_cfg = &mupd->FspmConfig; FSP_M_TEST_CONFIG *tconfig = &mupd->FspmTestConfig;
- soc_memory_init_params(m_cfg, config); + soc_memory_init_params(mupd, config);
/* Enable SMBus controller based on config */ if (!smbus)
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39454 )
Change subject: soc/intel/cannonlake: Steal no memory for disabled IGD ......................................................................
Patch Set 18:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/2213 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2212 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/2211
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Christian Walter (Code Review)
-
Duncan Laurie (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)