Change in coreboot[master]: [UNTESTED] skylake: do not rely on P2SB data to generate DRHD
Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35108 )
Change subject: [UNTESTED] skylake: do not rely on P2SB data to generate DRHD ......................................................................
[UNTESTED] skylake: do not rely on P2SB data to generate DRHD
The P2SB PCI device can be "hidden", which causes all sorts of nightmares and bugs. Moreover, FSP tends to hide it, so finding a good solution to this problem is impossible with FSP into the mix.
Since the values for IBDF and HBDF were already hardcoded as FSP parameters, write them with coreboot code and use these values directly to generate the DRHD, so as to avoid using the P2SB when it might be already hidden.
Note that the possibility of fireworks happening has not been ruled out.
Change-Id: I7eb20182380b953a1842083e7a3c67919d6971b9 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/bootblock/pch.c M src/soc/intel/skylake/chip_fsp20.c M src/soc/intel/skylake/include/soc/systemagent.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 5 files changed, 29 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/35108/1
diff --git a/src/soc/intel/skylake/acpi.c b/src/soc/intel/skylake/acpi.c index aa51cbe..9c5b674 100644 --- a/src/soc/intel/skylake/acpi.c +++ b/src/soc/intel/skylake/acpi.c @@ -30,7 +30,6 @@ #include <ec/google/chromeec/ec.h> #include <intelblocks/cpulib.h> #include <intelblocks/lpc_lib.h> -#include <intelblocks/p2sb.h> #include <intelblocks/sgx.h> #include <intelblocks/uart.h> #include <intelblocks/systemagent.h> @@ -575,30 +574,19 @@ acpi_dmar_rmrr_fixup(tmp, current); }
- struct device *const p2sb_dev = pcidev_path_on_root(PCH_DEVFN_P2SB); const u32 vtvc0bar = MCHBAR32(VTVC0BAR) & ~0xfff; const bool vtvc0en = MCHBAR32(VTVC0BAR) & 1;
/* General VTBAR has to be set and in 32-bit space. */ - if (p2sb_dev && vtvc0bar && vtvc0en && !MCHBAR32(VTVC0BAR + 4)) { + if (vtvc0bar && vtvc0en && !MCHBAR32(VTVC0BAR + 4)) { const unsigned long tmp = current;
- /* P2SB may already be hidden. There's no clear rule, when. */ - const u8 p2sb_hidden = - pci_read_config8(p2sb_dev, PCH_P2SB_E0 + 1); - pci_write_config8(p2sb_dev, PCH_P2SB_E0 + 1, 0); + current += acpi_create_dmar_drhd(current, DRHD_INCLUDE_PCI_ALL, 0, vtvc0bar);
- const u16 ibdf = pci_read_config16(p2sb_dev, PCH_P2SB_IBDF); - const u16 hbdf = pci_read_config16(p2sb_dev, PCH_P2SB_HBDF); - - pci_write_config8(p2sb_dev, PCH_P2SB_E0 + 1, p2sb_hidden); - - current += acpi_create_dmar_drhd(current, - DRHD_INCLUDE_PCI_ALL, 0, vtvc0bar); - current += acpi_create_dmar_ds_ioapic(current, - 2, ibdf >> 8, PCI_SLOT(ibdf), PCI_FUNC(ibdf)); - current += acpi_create_dmar_ds_msi_hpet(current, - 0, hbdf >> 8, PCI_SLOT(hbdf), PCI_FUNC(hbdf)); + current += acpi_create_dmar_ds_ioapic(current, 2, V_P2SB_IBDF_BUS, + V_P2SB_IBDF_DEV, V_P2SB_IBDF_FUN); + current += acpi_create_dmar_ds_msi_hpet(current, 0, V_P2SB_HBDF_BUS, + V_P2SB_HBDF_DEV, V_P2SB_HBDF_FUN);
acpi_dmar_drhd_fixup(tmp, current); } diff --git a/src/soc/intel/skylake/bootblock/pch.c b/src/soc/intel/skylake/bootblock/pch.c index c95a8d8..cc43415 100644 --- a/src/soc/intel/skylake/bootblock/pch.c +++ b/src/soc/intel/skylake/bootblock/pch.c @@ -35,6 +35,7 @@ #include <soc/pm.h> #include <soc/pmc.h> #include <soc/smbus.h> +#include <soc/systemagent.h>
#include "../chip.h"
@@ -50,6 +51,12 @@ fast_spi_early_init(SPI_BASE_ADDRESS); p2sb_enable_bar(); p2sb_configure_hpet(); + + // Write the IOAPIC Bus:Dev.Fun value to P2SB IBDF register + pci_write_config16(PCH_DEV_P2SB, PCH_P2SB_IBDF, V_DEFAULT_IBDF); + + // Write the HPET Bus:Dev.Fun value to P2SB HBDF register + pci_write_config16(PCH_DEV_P2SB, PCH_P2SB_HBDF, V_DEFAULT_HBDF); }
static void soc_config_acpibase(void) diff --git a/src/soc/intel/skylake/chip_fsp20.c b/src/soc/intel/skylake/chip_fsp20.c index 064f71e..fa39607 100644 --- a/src/soc/intel/skylake/chip_fsp20.c +++ b/src/soc/intel/skylake/chip_fsp20.c @@ -484,10 +484,8 @@ params->X2ApicOptOut = 0; tconfig->VtdDisable = 0;
- params->PchIoApicBdfValid = 1; - params->PchIoApicBusNumber = 250; - params->PchIoApicDeviceNumber = 31; - params->PchIoApicFunctionNumber = 0; + // We set that in coreboot already + params->PchIoApicBdfValid = 0; }
soc_irq_settings(params); diff --git a/src/soc/intel/skylake/include/soc/systemagent.h b/src/soc/intel/skylake/include/soc/systemagent.h index d7dec65..42b4eec 100644 --- a/src/soc/intel/skylake/include/soc/systemagent.h +++ b/src/soc/intel/skylake/include/soc/systemagent.h @@ -61,4 +61,16 @@ VTVC0_BASE_SIZE, "VTVC0BAR" }; + +// Hardcoded default values for PCI Bus:Dev.Fun for IOAPIC and HPET +#define V_P2SB_IBDF_BUS 250 +#define V_P2SB_IBDF_DEV 31 +#define V_P2SB_IBDF_FUN 0 +#define V_DEFAULT_IBDF ((V_P2SB_IBDF_BUS << 8) | PCI_DEVFN(V_P2SB_IBDF_DEV, V_P2SB_IBDF_FUN)) + +#define V_P2SB_HBDF_BUS 250 +#define V_P2SB_HBDF_DEV 15 +#define V_P2SB_HBDF_FUN 0 +#define V_DEFAULT_HBDF ((V_P2SB_HBDF_BUS << 8) | PCI_DEVFN(V_P2SB_HBDF_DEV, V_P2SB_HBDF_FUN)) + #endif diff --git a/src/soc/intel/skylake/romstage/romstage_fsp20.c b/src/soc/intel/skylake/romstage/romstage_fsp20.c index 8b5cd18..00cb6fe 100644 --- a/src/soc/intel/skylake/romstage/romstage_fsp20.c +++ b/src/soc/intel/skylake/romstage/romstage_fsp20.c @@ -288,10 +288,8 @@ cpu_flex_override(m_cfg);
if (!config->ignore_vtd) { - m_cfg->PchHpetBdfValid = 1; - m_cfg->PchHpetBusNumber = 250; - m_cfg->PchHpetDeviceNumber = 15; - m_cfg->PchHpetFunctionNumber = 0; + // We set that in coreboot already + m_cfg->PchHpetBdfValid = 0; } }
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35108 )
Change subject: [UNTESTED] skylake: do not rely on P2SB data to generate DRHD ......................................................................
Patch Set 1:
This change is ready for review.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35108 )
Change subject: [UNTESTED] skylake: do not rely on P2SB data to generate DRHD ......................................................................
Patch Set 1: Code-Review+1
no fireworks. booting with 'intel_iommu=on' works as expected
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35108 )
Change subject: [UNTESTED] skylake: do not rely on P2SB data to generate DRHD ......................................................................
Patch Set 1: Code-Review+1
Patch Set 1: Code-Review+1
no fireworks. booting with 'intel_iommu=on' works as expected
Tested asrock-h110 and works well
Hello Patrick Rudolph, Maxim Polyakov, Matt DeVillier, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35108
to look at the new patch set (#2).
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
soc/skylake: do not rely on P2SB data to generate DRHD
The P2SB PCI device can be "hidden", which causes all sorts of nightmares and bugs. Moreover, FSP tends to hide it, so finding a good solution to this problem is impossible with FSP into the mix.
Since the values for IBDF and HBDF were already hardcoded as FSP parameters, write them with coreboot code and use these values directly to generate the DRHD.
Granted, these P2SB writes could be done later. But for now, the safest place to do them is very early, before calling FSP code.
Change-Id: I7eb20182380b953a1842083e7a3c67919d6971b9 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/bootblock/pch.c M src/soc/intel/skylake/chip_fsp20.c M src/soc/intel/skylake/include/soc/systemagent.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 5 files changed, 29 insertions(+), 26 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/35108/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35108 )
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
Patch Set 2: -Code-Review
(1 comment)
Patch Set 1: Code-Review+1
Patch Set 1: Code-Review+1
no fireworks. booting with 'intel_iommu=on' works as expected
Tested asrock-h110 and works well
Thanks for testing :D
https://review.coreboot.org/c/coreboot/+/35108/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35108/1//COMMIT_MSG@18 PS1, Line 18: Note that the possibility of fireworks happening has not been ruled out.
Blocking because of this.
No longer a blocker, change has been tested by two people on desktop and mobile platforms.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35108 )
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
Patch Set 2:
Looks good, but it's actually two unrelated changes:
1. what the summary says 2. switching from FSP configuration to direct register writes
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35108 )
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/chip_... PS2, Line 487: // We set that in coreboot already : params->PchIoApicBdfValid = 0; if we set it in coreboot already, then why are we setting it to zero here for FSP?
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/inclu... File src/soc/intel/skylake/include/soc/systemagent.h:
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/inclu... PS2, Line 65: Hardcoded default values for PCI Bus:Dev.Fun for IOAPIC and HPET source?
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/romst... File src/soc/intel/skylake/romstage/romstage_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/romst... PS2, Line 291: // We set that in coreboot already : m_cfg->PchHpetBdfValid = 0; same as before. and why is this set in both romstage and ramstage?
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35108 )
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
Patch Set 2:
(3 comments)
Patch Set 2:
Looks good, but it's actually two unrelated changes:
- what the summary says
- switching from FSP configuration to direct register writes
I could split this into two patches, one that just hardcodes the DRHD values, and another that "eases" FSP's job. Should I reuse this change ID for DRHD hardcoding or register writes?
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/chip_... PS2, Line 487: // We set that in coreboot already : params->PchIoApicBdfValid = 0;
if we set it in coreboot already, then why are we setting it to zero here for FSP?
The FSP has four UPDs for this: The trio for Bus:Dev.Fun values, and a fourth "Valid" UPD. The FSP code checks this "Valid" flag and, if true, will then write the Bus:Dev.Fun UPD values to the P2SB register.
Since I am now writing the values with coreboot code, I don't need FSP to write them again. So I just tell FSP that the Bus:Dev.Fun UPD values are not valid, and FSP will not program them.
And if FSP misbehaves and programs them anyway, then things will break and I will blame the FSP for it >:)
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/inclu... File src/soc/intel/skylake/include/soc/systemagent.h:
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/inclu... PS2, Line 65: Hardcoded default values for PCI Bus:Dev.Fun for IOAPIC and HPET
source?
IBDF values come from chip_fsp20.c params->PchIoApicBusNumber = 250; params->PchIoApicDeviceNumber = 31; params->PchIoApicFunctionNumber = 0;
HBDF values come from romstage_fsp20.c m_cfg->PchHpetBusNumber = 250; m_cfg->PchHpetDeviceNumber = 15; m_cfg->PchHpetFunctionNumber = 0;
To be exact, the same places in which you left your other two comments :-)
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/romst... File src/soc/intel/skylake/romstage/romstage_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/romst... PS2, Line 291: // We set that in coreboot already : m_cfg->PchHpetBdfValid = 0;
same as before. […]
FSP-M writes the Bus:Dev.Fun for the HPET (done here), whereas FSP-S writes the Bus:Dev.Fun for the IOAPIC (done in chip_fsp20.c). It's not doing it twice, it's just very similar.
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35108 )
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
Patch Set 2:
(3 comments)
I could split this into two patches, one that just hardcodes the DRHD values, and another that "eases" FSP's job. Should I reuse this change ID for DRHD hardcoding or register writes?
based on the commit subject, the former IMO
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/chip_... PS2, Line 487: // We set that in coreboot already : params->PchIoApicBdfValid = 0;
The FSP has four UPDs for this: The trio for Bus:Dev.Fun values, and a fourth "Valid" UPD. […]
makes sense, but the comment and the code don't without that context. I'd argue a comment such as: "// APIC Bus:Dev.Fun set in coreboot, so ignore UPDs" would be more useful to anyone reading the code outside of this review :)
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/inclu... File src/soc/intel/skylake/include/soc/systemagent.h:
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/inclu... PS2, Line 65: Hardcoded default values for PCI Bus:Dev.Fun for IOAPIC and HPET
IBDF values come from chip_fsp20.c […]
right, but they appear to be common across multiple Intel SoC's, so I assume it's documented somewhere publicly?
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/romst... File src/soc/intel/skylake/romstage/romstage_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/romst... PS2, Line 291: // We set that in coreboot already : m_cfg->PchHpetBdfValid = 0;
FSP-M writes the Bus:Dev.Fun for the HPET (done here), whereas FSP-S writes the Bus:Dev. […]
ah, I wasn't looking carefully enough at the UPDs. My comment for the one chip_fsp20.c applies here as well, since it's unclear what coreboot sets or how setting the UPD to zero relates
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35108 )
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
Patch Set 2:
(1 comment)
I could split this into two patches, one that just hardcodes the DRHD values, and another that "eases" FSP's job. Should I reuse this change ID for DRHD hardcoding or register writes?
based on the commit subject, the former IMO
+1
The latter is really optional, in the best case a no-op and in the worst a regression (e.g. if FSP misbehaves) and will probably get some more bikeshedding about where/when it should be done.
Unrelated, what's with the comment style? I'm generally not op- posed to //, but seeing it added to a file that consistently uses the other style makes me freak out a little.
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/inclu... File src/soc/intel/skylake/include/soc/systemagent.h:
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/inclu... PS2, Line 65: Hardcoded default values for PCI Bus:Dev.Fun for IOAPIC and HPET
right, but they appear to be common across multiple Intel SoC's, so I assume it's documented somewhe […]
Not public, in BIOS Writer's Guides mostly. But doesn't matter really, they just have to be unique within the running system and sync'ed with the ACPI table.
Hello Kyösti Mälkki, Patrick Rudolph, Maxim Polyakov, Matt DeVillier, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35108
to look at the new patch set (#3).
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
soc/skylake: do not rely on P2SB data to generate DRHD
The P2SB PCI device can be "hidden", which causes all sorts of nightmares and bugs. Moreover, FSP tends to hide it, so finding a good solution to this problem is impossible with FSP into the mix.
Since the values for IBDF and HBDF were already hardcoded as FSP parameters, write them with coreboot code and use these values directly to generate the DRHD.
Granted, these P2SB writes could be done later. But for now, the safest place to do them is very early, before calling FSP code.
Change-Id: I7eb20182380b953a1842083e7a3c67919d6971b9 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/include/soc/systemagent.h 2 files changed, 16 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/35108/3
Hello Kyösti Mälkki, Patrick Rudolph, Maxim Polyakov, Matt DeVillier, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35108
to look at the new patch set (#4).
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
soc/skylake: do not rely on P2SB data to generate DRHD
The P2SB PCI device can be "hidden", which causes all sorts of nightmares and bugs. Moreover, FSP tends to hide it, so finding a good solution to this problem is impossible with FSP into the mix.
Since the values for IBDF and HBDF were already hardcoded as FSP parameters, define them as macros and use these values directly to generate the DRHD.
Change-Id: I7eb20182380b953a1842083e7a3c67919d6971b9 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip_fsp20.c M src/soc/intel/skylake/include/soc/systemagent.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 4 files changed, 22 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/35108/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35108 )
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
Patch Set 4:
(3 comments)
Patch Set 2:
(1 comment)
I could split this into two patches, one that just hardcodes the DRHD values, and another that "eases" FSP's job. Should I reuse this change ID for DRHD hardcoding or register writes?
based on the commit subject, the former IMO
+1
The latter is really optional, in the best case a no-op and in the worst a regression (e.g. if FSP misbehaves) and will probably get some more bikeshedding about where/when it should be done.
Unrelated, what's with the comment style? I'm generally not op- posed to //, but seeing it added to a file that consistently uses the other style makes me freak out a little.
Took care of all the comments, I hope.
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/chip_... File src/soc/intel/skylake/chip_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/chip_... PS2, Line 487: // We set that in coreboot already : params->PchIoApicBdfValid = 0;
makes sense, but the comment and the code don't without that context. […]
Ack
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/inclu... File src/soc/intel/skylake/include/soc/systemagent.h:
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/inclu... PS2, Line 65: Hardcoded default values for PCI Bus:Dev.Fun for IOAPIC and HPET
Not public, in BIOS Writer's Guides mostly. But doesn't matter really, they […]
Ack
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/romst... File src/soc/intel/skylake/romstage/romstage_fsp20.c:
https://review.coreboot.org/c/coreboot/+/35108/2/src/soc/intel/skylake/romst... PS2, Line 291: // We set that in coreboot already : m_cfg->PchHpetBdfValid = 0;
ah, I wasn't looking carefully enough at the UPDs. My comment for the one chip_fsp20. […]
Ack
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35108 )
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35108/4/src/soc/intel/skylake/inclu... File src/soc/intel/skylake/include/soc/systemagent.h:
https://review.coreboot.org/c/coreboot/+/35108/4/src/soc/intel/skylake/inclu... PS4, Line 66: #define V_P2SB_IBDF_BUS 250 : #define V_P2SB_IBDF_DEV 31 Ah, the inconsistency!
Hello Kyösti Mälkki, Patrick Rudolph, Maxim Polyakov, Matt DeVillier, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35108
to look at the new patch set (#5).
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
soc/skylake: do not rely on P2SB data to generate DRHD
The P2SB PCI device can be "hidden", which causes all sorts of nightmares and bugs. Moreover, FSP tends to hide it, so finding a good solution to this problem is impossible with FSP into the mix.
Since the values for IBDF and HBDF were already hardcoded as FSP parameters, define them as macros and use these values directly to generate the DRHD.
Change-Id: I7eb20182380b953a1842083e7a3c67919d6971b9 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip_fsp20.c M src/soc/intel/skylake/include/soc/systemagent.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 4 files changed, 22 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/35108/5
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35108 )
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35108/4/src/soc/intel/skylake/inclu... File src/soc/intel/skylake/include/soc/systemagent.h:
https://review.coreboot.org/c/coreboot/+/35108/4/src/soc/intel/skylake/inclu... PS4, Line 66: #define V_P2SB_IBDF_BUS 250 : #define V_P2SB_IBDF_DEV 31
Ah, the inconsistency!
Done
Hello Kyösti Mälkki, Patrick Rudolph, Maxim Polyakov, Matt DeVillier, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35108
to look at the new patch set (#6).
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
soc/skylake: do not rely on P2SB data to generate DRHD
The P2SB PCI device can be "hidden", which causes all sorts of nightmares and bugs. Moreover, FSP tends to hide it, so finding a good solution to this problem is impossible with FSP into the mix.
Since the values for IBDF and HBDF were already hardcoded as FSP parameters, define them as macros and use these values directly to generate the DRHD.
Change-Id: I7eb20182380b953a1842083e7a3c67919d6971b9 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip_fsp20.c M src/soc/intel/skylake/include/soc/systemagent.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 4 files changed, 22 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/35108/6
Hello Kyösti Mälkki, Patrick Rudolph, Maxim Polyakov, Matt DeVillier, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35108
to look at the new patch set (#7).
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
soc/skylake: do not rely on P2SB data to generate DRHD
The P2SB PCI device can be "hidden", which causes all sorts of nightmares and bugs. Moreover, FSP tends to hide it, so finding a good solution to this problem is impossible with FSP into the mix.
Since the values for IBDF and HBDF were already hardcoded as FSP parameters, define them as macros and use these values directly to generate the DRHD.
Change-Id: I7eb20182380b953a1842083e7a3c67919d6971b9 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip_fsp20.c M src/soc/intel/skylake/include/soc/systemagent.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 4 files changed, 23 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/35108/7
Hello Kyösti Mälkki, Patrick Rudolph, Maxim Polyakov, Matt DeVillier, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35108
to look at the new patch set (#8).
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
soc/skylake: do not rely on P2SB data to generate DRHD
The P2SB PCI device can be "hidden", which causes all sorts of nightmares and bugs. Moreover, FSP tends to hide it, so finding a good solution to this problem is impossible with FSP into the mix.
Since the values for IBDF and HBDF were already hardcoded as FSP parameters, define them as macros and use these values directly to generate the DRHD.
Change-Id: I7eb20182380b953a1842083e7a3c67919d6971b9 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip_fsp20.c M src/soc/intel/skylake/include/soc/systemagent.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 4 files changed, 22 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/35108/8
Hello Kyösti Mälkki, Patrick Rudolph, Maxim Polyakov, Matt DeVillier, build bot (Jenkins), Nico Huber,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35108
to look at the new patch set (#9).
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
soc/skylake: do not rely on P2SB data to generate DRHD
The P2SB PCI device can be "hidden", which causes all sorts of nightmares and bugs. Moreover, FSP tends to hide it, so finding a good solution to this problem is impossible with FSP into the mix.
Since the values for IBDF and HBDF were already hardcoded as FSP parameters, define them as macros and use these values directly to generate the DRHD.
Change-Id: I7eb20182380b953a1842083e7a3c67919d6971b9 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip_fsp20.c M src/soc/intel/skylake/include/soc/systemagent.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 4 files changed, 23 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/08/35108/9
Mimoja has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35108 )
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
Patch Set 9: Code-Review+1
Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35108 )
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
Patch Set 9: Code-Review+2
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35108 )
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
Patch Set 9: Code-Review+2
Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35108 )
Change subject: soc/skylake: do not rely on P2SB data to generate DRHD ......................................................................
soc/skylake: do not rely on P2SB data to generate DRHD
The P2SB PCI device can be "hidden", which causes all sorts of nightmares and bugs. Moreover, FSP tends to hide it, so finding a good solution to this problem is impossible with FSP into the mix.
Since the values for IBDF and HBDF were already hardcoded as FSP parameters, define them as macros and use these values directly to generate the DRHD.
Change-Id: I7eb20182380b953a1842083e7a3c67919d6971b9 Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/35108 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Mimoja coreboot@mimoja.de Reviewed-by: Maxim Polyakov max.senia.poliak@gmail.com Reviewed-by: Nico Huber nico.h@gmx.de --- M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/chip_fsp20.c M src/soc/intel/skylake/include/soc/systemagent.h M src/soc/intel/skylake/romstage/romstage_fsp20.c 4 files changed, 23 insertions(+), 24 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Mimoja: Looks good to me, but someone else must approve Maxim Polyakov: Looks good to me, approved
diff --git a/src/soc/intel/skylake/acpi.c b/src/soc/intel/skylake/acpi.c index ccfc7b7..c3757b0 100644 --- a/src/soc/intel/skylake/acpi.c +++ b/src/soc/intel/skylake/acpi.c @@ -30,7 +30,6 @@ #include <ec/google/chromeec/ec.h> #include <intelblocks/cpulib.h> #include <intelblocks/lpc_lib.h> -#include <intelblocks/p2sb.h> #include <intelblocks/sgx.h> #include <intelblocks/uart.h> #include <intelblocks/systemagent.h> @@ -39,7 +38,6 @@ #include <soc/cpu.h> #include <soc/iomap.h> #include <soc/msr.h> -#include <soc/p2sb.h> #include <soc/pci_devs.h> #include <soc/pm.h> #include <soc/ramstage.h> @@ -575,30 +573,20 @@ acpi_dmar_rmrr_fixup(tmp, current); }
- struct device *const p2sb_dev = pcidev_path_on_root(PCH_DEVFN_P2SB); const u32 vtvc0bar = MCHBAR32(VTVC0BAR) & ~0xfff; const bool vtvc0en = MCHBAR32(VTVC0BAR) & 1;
/* General VTBAR has to be set and in 32-bit space. */ - if (p2sb_dev && vtvc0bar && vtvc0en && !MCHBAR32(VTVC0BAR + 4)) { + if (vtvc0bar && vtvc0en && !MCHBAR32(VTVC0BAR + 4)) { const unsigned long tmp = current;
- /* P2SB may already be hidden. There's no clear rule, when. */ - const u8 p2sb_hidden = - pci_read_config8(p2sb_dev, PCH_P2SB_E0 + 1); - pci_write_config8(p2sb_dev, PCH_P2SB_E0 + 1, 0); + current += acpi_create_dmar_drhd(current, DRHD_INCLUDE_PCI_ALL, 0, vtvc0bar);
- const u16 ibdf = pci_read_config16(p2sb_dev, PCH_P2SB_IBDF); - const u16 hbdf = pci_read_config16(p2sb_dev, PCH_P2SB_HBDF); + current += acpi_create_dmar_ds_ioapic(current, 2, V_P2SB_IBDF_BUS, + V_P2SB_IBDF_DEV, V_P2SB_IBDF_FUN);
- pci_write_config8(p2sb_dev, PCH_P2SB_E0 + 1, p2sb_hidden); - - current += acpi_create_dmar_drhd(current, - DRHD_INCLUDE_PCI_ALL, 0, vtvc0bar); - current += acpi_create_dmar_ds_ioapic(current, - 2, ibdf >> 8, PCI_SLOT(ibdf), PCI_FUNC(ibdf)); - current += acpi_create_dmar_ds_msi_hpet(current, - 0, hbdf >> 8, PCI_SLOT(hbdf), PCI_FUNC(hbdf)); + current += acpi_create_dmar_ds_msi_hpet(current, 0, V_P2SB_HBDF_BUS, + V_P2SB_HBDF_DEV, V_P2SB_HBDF_FUN);
acpi_dmar_drhd_fixup(tmp, current); } diff --git a/src/soc/intel/skylake/chip_fsp20.c b/src/soc/intel/skylake/chip_fsp20.c index e9c37d6..de86936 100644 --- a/src/soc/intel/skylake/chip_fsp20.c +++ b/src/soc/intel/skylake/chip_fsp20.c @@ -490,9 +490,9 @@ tconfig->VtdDisable = 0;
params->PchIoApicBdfValid = 1; - params->PchIoApicBusNumber = 250; - params->PchIoApicDeviceNumber = 31; - params->PchIoApicFunctionNumber = 0; + params->PchIoApicBusNumber = V_P2SB_IBDF_BUS; + params->PchIoApicDeviceNumber = V_P2SB_IBDF_DEV; + params->PchIoApicFunctionNumber = V_P2SB_IBDF_FUN; }
soc_irq_settings(params); diff --git a/src/soc/intel/skylake/include/soc/systemagent.h b/src/soc/intel/skylake/include/soc/systemagent.h index d7dec65..565c885 100644 --- a/src/soc/intel/skylake/include/soc/systemagent.h +++ b/src/soc/intel/skylake/include/soc/systemagent.h @@ -61,4 +61,14 @@ VTVC0_BASE_SIZE, "VTVC0BAR" }; + +/* Hardcoded default values for PCI Bus:Dev.Fun for IOAPIC and HPET */ +#define V_P2SB_IBDF_BUS 250 +#define V_P2SB_IBDF_DEV 31 +#define V_P2SB_IBDF_FUN 0 + +#define V_P2SB_HBDF_BUS 250 +#define V_P2SB_HBDF_DEV 15 +#define V_P2SB_HBDF_FUN 0 + #endif diff --git a/src/soc/intel/skylake/romstage/romstage_fsp20.c b/src/soc/intel/skylake/romstage/romstage_fsp20.c index ecd1428..002a284 100644 --- a/src/soc/intel/skylake/romstage/romstage_fsp20.c +++ b/src/soc/intel/skylake/romstage/romstage_fsp20.c @@ -30,6 +30,7 @@ #include <soc/pci_devs.h> #include <soc/pm.h> #include <soc/romstage.h> +#include <soc/systemagent.h> #include <string.h> #include <security/vboot/vboot_common.h>
@@ -247,9 +248,9 @@
if (!config->ignore_vtd) { m_cfg->PchHpetBdfValid = 1; - m_cfg->PchHpetBusNumber = 250; - m_cfg->PchHpetDeviceNumber = 15; - m_cfg->PchHpetFunctionNumber = 0; + m_cfg->PchHpetBusNumber = V_P2SB_HBDF_BUS; + m_cfg->PchHpetDeviceNumber = V_P2SB_HBDF_DEV; + m_cfg->PchHpetFunctionNumber = V_P2SB_HBDF_FUN; } }
-
Angel Pons (Code Review) -
Matt DeVillier (Code Review)
-
Maxim Polyakov (Code Review)
-
Mimoja (Code Review)
-
Nico Huber (Code Review)