Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40722 )
Change subject: [WIP] Add Multiple Segment support ......................................................................
[WIP] Add Multiple Segment support
Test Patch for get feedback on design aspect
Change-Id: Ide7e97aa7c48fd0e993b248b38b2ad6ad94a21cb Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/intel/tglrvp/dsdt.asl M src/soc/intel/apollolake/systemagent.c M src/soc/intel/cannonlake/systemagent.c M src/soc/intel/common/block/acpi/acpi.c A src/soc/intel/common/block/acpi/acpi/extrahostbridge.asl A src/soc/intel/common/block/acpi/acpi/pcisegment.asl M src/soc/intel/common/block/include/intelblocks/systemagent.h M src/soc/intel/common/block/systemagent/Kconfig M src/soc/intel/common/block/systemagent/systemagent_def.h M src/soc/intel/common/block/systemagent/systemagent_early.c A src/soc/intel/common/block/tcss/Kconfig M src/soc/intel/icelake/systemagent.c M src/soc/intel/jasperlake/systemagent.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/systemagent.c M src/soc/intel/tigerlake/Kconfig M src/soc/intel/tigerlake/acpi/pci_irqs.asl M src/soc/intel/tigerlake/include/soc/irq.h M src/soc/intel/tigerlake/systemagent.c 19 files changed, 195 insertions(+), 51 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/40722/1
diff --git a/src/mainboard/intel/tglrvp/dsdt.asl b/src/mainboard/intel/tglrvp/dsdt.asl index af13d9f..c531baa 100644 --- a/src/mainboard/intel/tglrvp/dsdt.asl +++ b/src/mainboard/intel/tglrvp/dsdt.asl @@ -28,6 +28,9 @@ #include <soc/intel/common/block/acpi/acpi/northbridge.asl> #include <soc/intel/tigerlake/acpi/southbridge.asl> } + #if CONFIG_SA_PCI_SEGMENT_GROUPS > 1 + #include <soc/intel/common/block/acpi/acpi/pcisegment.asl> + #endif }
#if CONFIG(CHROMEOS) diff --git a/src/soc/intel/apollolake/systemagent.c b/src/soc/intel/apollolake/systemagent.c index a1b6b4d..a15200b 100644 --- a/src/soc/intel/apollolake/systemagent.c +++ b/src/soc/intel/apollolake/systemagent.c @@ -31,7 +31,8 @@ void soc_add_fixed_mmio_resources(struct device *dev, int *index) { static const struct sa_mmio_descriptor soc_fixed_resources[] = { - { PCIEXBAR, CONFIG_MMCONF_BASE_ADDRESS, CONFIG_SA_PCIEX_LENGTH, + { PCIEXBAR, CONFIG_MMCONF_BASE_ADDRESS, + PCIEXBAR_LENGTH_MiB(CONFIG_SA_PCI_SEGMENT_GROUPS), "PCIEXBAR" }, { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR" }, }; diff --git a/src/soc/intel/cannonlake/systemagent.c b/src/soc/intel/cannonlake/systemagent.c index 1ab365e..9d7e40e 100644 --- a/src/soc/intel/cannonlake/systemagent.c +++ b/src/soc/intel/cannonlake/systemagent.c @@ -20,7 +20,8 @@ void soc_add_fixed_mmio_resources(struct device *dev, int *index) { static const struct sa_mmio_descriptor soc_fixed_resources[] = { - { PCIEXBAR, CONFIG_MMCONF_BASE_ADDRESS, CONFIG_SA_PCIEX_LENGTH, + { PCIEXBAR, CONFIG_MMCONF_BASE_ADDRESS, + PCIEXBAR_LENGTH_MiB(CONFIG_SA_PCI_SEGMENT_GROUPS), "PCIEXBAR" }, { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR" }, { DMIBAR, DMI_BASE_ADDRESS, DMI_BASE_SIZE, "DMIBAR" }, diff --git a/src/soc/intel/common/block/acpi/acpi.c b/src/soc/intel/common/block/acpi/acpi.c index fe127a2..9b0457f 100644 --- a/src/soc/intel/common/block/acpi/acpi.c +++ b/src/soc/intel/common/block/acpi/acpi.c @@ -35,10 +35,13 @@
__attribute__((weak)) unsigned long acpi_fill_mcfg(unsigned long current) { - /* PCI Segment Group 0, Start Bus Number 0, End Bus Number is 255 */ - current += acpi_create_mcfg_mmconfig((void *)current, - CONFIG_MMCONF_BASE_ADDRESS, 0, 0, - (CONFIG_SA_PCIEX_LENGTH >> 20) - 1); + /* + * PCI Segment Group depends on CONFIG_SA_PCI_SEGMENT_GROUPS, + * Start Bus Number 0, End Bus Number is 255 + */ + for (int i = 0; i < CONFIG_SA_PCI_SEGMENT_GROUPS; i++) + current += acpi_create_mcfg_mmconfig((void *)current, + CONFIG_MMCONF_BASE_ADDRESS, i, 0, 255); return current; }
diff --git a/src/soc/intel/common/block/acpi/acpi/extrahostbridge.asl b/src/soc/intel/common/block/acpi/acpi/extrahostbridge.asl new file mode 100644 index 0000000..77c0248 --- /dev/null +++ b/src/soc/intel/common/block/acpi/acpi/extrahostbridge.asl @@ -0,0 +1,74 @@ +/* + * This file is part of the coreboot project. + * + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +Method (_CRS, 0, Serialized) +{ + Name (MCRS, ResourceTemplate () + { + /* Bus Numbers */ + WordBusNumber (ResourceProducer, MinFixed, MaxFixed, PosDecode, + 0x0000, 0x0000, 0x00ff, 0x0000, 0x0100) + + /* IO Region 0 */ + DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, + EntireRange, + 0x0000, 0x0000, 0x0cf7, 0x0000, 0x0cf8) + + /* IO Region 1 */ + DWordIO (ResourceProducer, MinFixed, MaxFixed, PosDecode, + EntireRange, + 0x0000, 0x0d00, 0xffff, 0x0000, 0xf300) + + /* PCI Memory Region (TLUD - 0xdfffffff) */ + DWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, + NonCacheable, ReadWrite, + 0x00000000, 0x00000000, 0xdfffffff, 0x00000000, + 0xE0000000,,, PM01) + + /* PCI Memory Region (TUUD - (TUUD + ABOVE_4G_MMIO_SIZE)) */ + QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed, + NonCacheable, ReadWrite, + 0x00000000, 0x10000, 0x1ffff, 0x00000000, + 0x10000,,, PM02) + }) + + /* Find PCI resource area in MCRS */ + CreateDwordField (MCRS, PM01._MIN, PMIN) + CreateDwordField (MCRS, PM01._MAX, PMAX) + CreateDwordField (MCRS, PM01._LEN, PLEN) + + /* + * Fix up PCI memory region + * Start with Top of Lower Usable DRAM + */ + Store (_SB.PCI0.MCHC.TLUD, PMIN) + Add (Subtract (PMAX, PMIN), 1, PLEN) + + /* Patch PM02 range based on Memory Size */ + If (LEqual (A4GS, 0)) { + CreateQwordField (MCRS, PM02._LEN, MSEN) + Store (0, MSEN) + } Else { + CreateQwordField (MCRS, PM02._MIN, MMIN) + CreateQwordField (MCRS, PM02._MAX, MMAX) + CreateQwordField (MCRS, PM02._LEN, MLEN) + /* Set 64bit MMIO resource base and length */ + Store (A4GS, MLEN) + Store (A4GB, MMIN) + Subtract (Add (MMIN, MLEN), 1, MMAX) + } + + Return (MCRS) +} diff --git a/src/soc/intel/common/block/acpi/acpi/pcisegment.asl b/src/soc/intel/common/block/acpi/acpi/pcisegment.asl new file mode 100644 index 0000000..4efaf20 --- /dev/null +++ b/src/soc/intel/common/block/acpi/acpi/pcisegment.asl @@ -0,0 +1,54 @@ +/* + * This file is part of the coreboot project. + * + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#define TBT_PCIe0_IRQ 16 +#define TBT_PCIe1_IRQ 17 +#define TBT_PCIe2_IRQ 18 +#define TBT_PCIe3_IRQ 19 + +Device (PCI1) +{ + Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */) // _HID: Hardware ID + Name (_CID, EisaId ("PNP0A03") /* PCI Bus */) // _CID: Compatible ID + Name (_SEG, 1) // _SEG: PCI Segment + Name (_UID, 1) // _UID: Unique ID + Name (_ADR, 0x00000000) + + Name (PICN, Package () { + /* SEG1: PCIe Root Port*/ + Package(){0x0007FFFF, 0, 0, 11 }, + Package(){0x0007FFFF, 1, 0, 10 }, + Package(){0x0007FFFF, 2, 0, 11 }, + Package(){0x0007FFFF, 3, 0, 11 }, + }) + + Name (PICP, Package () { + /* SEG1: PCIe Root Port*/ + Package(){0x0007FFFF, 0, 0, TBT_PCIe0_IRQ }, + Package(){0x0007FFFF, 1, 0, TBT_PCIe1_IRQ }, + Package(){0x0007FFFF, 2, 0, TBT_PCIe2_IRQ }, + Package(){0x0007FFFF, 3, 0, TBT_PCIe3_IRQ }, + }) + + Method (_PRT) + { + If (PICM) { + Return (PICP) + } Else { + Return (PICN) + } + } + #include "extrahostbridge.asl" +} diff --git a/src/soc/intel/common/block/include/intelblocks/systemagent.h b/src/soc/intel/common/block/include/intelblocks/systemagent.h index c6aa4b4..cfa96e3 100644 --- a/src/soc/intel/common/block/include/intelblocks/systemagent.h +++ b/src/soc/intel/common/block/include/intelblocks/systemagent.h @@ -4,11 +4,14 @@ #ifndef SOC_INTEL_COMMON_BLOCK_SA_H #define SOC_INTEL_COMMON_BLOCK_SA_H
+#include <commonlib/helpers.h> #include <device/device.h> #include <soc/iomap.h> #include <soc/nvs.h> #include <stddef.h>
+#define PCIEXBAR_LENGTH_MiB(x) (x * 256 * MiB) + /* Device 0:0.0 PCI configuration space */ #define MCHBAR 0x48 #define PCIEXBAR 0x60 diff --git a/src/soc/intel/common/block/systemagent/Kconfig b/src/soc/intel/common/block/systemagent/Kconfig index 6dd1f3b..759abc9 100644 --- a/src/soc/intel/common/block/systemagent/Kconfig +++ b/src/soc/intel/common/block/systemagent/Kconfig @@ -9,23 +9,18 @@ hex default 0xe0000000
-config SA_PCIEX_LENGTH +config SA_PCI_SEGMENT_GROUPS hex - default 0x10000000 if (PCIEX_LENGTH_256MB) - default 0x8000000 if (PCIEX_LENGTH_128MB) - default 0x4000000 if (PCIEX_LENGTH_64MB) - default 0x10000000 + default 1 + default 2 if TCSS_PCIE_SEGMENT help - This option allows you to select length of PCIEX region. + This option allows you to select required PCI segment.
-config PCIEX_LENGTH_256MB - bool - -config PCIEX_LENGTH_128MB - bool - -config PCIEX_LENGTH_64MB - bool + Each PCI segment supports up to 256 buses (0-255). + PCIE length selection is depends on PCI_SEGMENT_GROUPS. + Formula for PCIE Length = PCI_SEGMENT_GROUPS * 256 MB + If PCI segment is 1 then length of PCIEX region is 256MB to support 256 buses + else PCIEX region would be multiple of 256MB based on PCI_SEGMENT_GROUPS.
config SA_ENABLE_IMR bool diff --git a/src/soc/intel/common/block/systemagent/systemagent_def.h b/src/soc/intel/common/block/systemagent/systemagent_def.h index 03f4de4..ef2f7b4 100644 --- a/src/soc/intel/common/block/systemagent/systemagent_def.h +++ b/src/soc/intel/common/block/systemagent/systemagent_def.h @@ -4,7 +4,6 @@ #ifndef SOC_INTEL_COMMON_BLOCK_SA_DEF_H #define SOC_INTEL_COMMON_BLOCK_SA_DEF_H
- /* Device 0:0.0 PCI configuration space */
/* GMCH Graphics Control Register */ @@ -19,8 +18,10 @@ #define DPR_PRS (1 << 1) #define DPR_SIZE_MASK 0xff0
-#define PCIEXBAR_LENGTH_64MB 2 -#define PCIEXBAR_LENGTH_128MB 1 +#define PCIEXBAR_LENGTH_4096MB 6 +#define PCIEXBAR_LENGTH_2048MB 5 +#define PCIEXBAR_LENGTH_1024MB 4 +#define PCIEXBAR_LENGTH_512MB 3 #define PCIEXBAR_LENGTH_256MB 0 #define PCIEXBAR_PCIEXBAREN (1 << 0)
diff --git a/src/soc/intel/common/block/systemagent/systemagent_early.c b/src/soc/intel/common/block/systemagent/systemagent_early.c index cb0ed34..69b36f8 100644 --- a/src/soc/intel/common/block/systemagent/systemagent_early.c +++ b/src/soc/intel/common/block/systemagent/systemagent_early.c @@ -27,15 +27,21 @@ pci_io_write_config32(SA_DEV_ROOT, PCIEXBAR + 4, reg);
/* Get PCI Express Region Length */ - switch (CONFIG_SA_PCIEX_LENGTH) { - case 256 * MiB: + switch (CONFIG_SA_PCI_SEGMENT_GROUPS) { + case 1: pciexbar_length = PCIEXBAR_LENGTH_256MB; break; - case 128 * MiB: - pciexbar_length = PCIEXBAR_LENGTH_128MB; + case 2: + pciexbar_length = PCIEXBAR_LENGTH_512MB; break; - case 64 * MiB: - pciexbar_length = PCIEXBAR_LENGTH_64MB; + case 4: + pciexbar_length = PCIEXBAR_LENGTH_1024MB; + break; + case 8: + pciexbar_length = PCIEXBAR_LENGTH_2048MB; + break; + case 16: + pciexbar_length = PCIEXBAR_LENGTH_4096MB; break; default: pciexbar_length = PCIEXBAR_LENGTH_256MB; diff --git a/src/soc/intel/common/block/tcss/Kconfig b/src/soc/intel/common/block/tcss/Kconfig new file mode 100644 index 0000000..0f7fe7c --- /dev/null +++ b/src/soc/intel/common/block/tcss/Kconfig @@ -0,0 +1,9 @@ +config SOC_INTEL_COMMON_BLOCK_TCSS + bool + help + Intel Processor common TCSS support + +config TCSS_PCIE_SEGMENT + bool + help + Indicate the extra PCIE segment is required for iTBT or not diff --git a/src/soc/intel/icelake/systemagent.c b/src/soc/intel/icelake/systemagent.c index c07fd0a..6cf6354 100644 --- a/src/soc/intel/icelake/systemagent.c +++ b/src/soc/intel/icelake/systemagent.c @@ -16,7 +16,8 @@ void soc_add_fixed_mmio_resources(struct device *dev, int *index) { static const struct sa_mmio_descriptor soc_fixed_resources[] = { - { PCIEXBAR, CONFIG_MMCONF_BASE_ADDRESS, CONFIG_SA_PCIEX_LENGTH, + { PCIEXBAR, CONFIG_MMCONF_BASE_ADDRESS, + PCIEXBAR_LENGTH_MiB(CONFIG_SA_PCI_SEGMENT_GROUPS), "PCIEXBAR" }, { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR" }, { DMIBAR, DMI_BASE_ADDRESS, DMI_BASE_SIZE, "DMIBAR" }, diff --git a/src/soc/intel/jasperlake/systemagent.c b/src/soc/intel/jasperlake/systemagent.c index 7be471a..4c5efe7 100644 --- a/src/soc/intel/jasperlake/systemagent.c +++ b/src/soc/intel/jasperlake/systemagent.c @@ -17,7 +17,8 @@ void soc_add_fixed_mmio_resources(struct device *dev, int *index) { static const struct sa_mmio_descriptor soc_fixed_resources[] = { - { PCIEXBAR, CONFIG_MMCONF_BASE_ADDRESS, CONFIG_SA_PCIEX_LENGTH, + { PCIEXBAR, CONFIG_MMCONF_BASE_ADDRESS, + PCIEXBAR_LENGTH_MiB(CONFIG_SA_PCI_SEGMENT_GROUPS), "PCIEXBAR" }, { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR" }, { DMIBAR, DMI_BASE_ADDRESS, DMI_BASE_SIZE, "DMIBAR" }, diff --git a/src/soc/intel/skylake/acpi.c b/src/soc/intel/skylake/acpi.c index 75f6b69..5d95029 100644 --- a/src/soc/intel/skylake/acpi.c +++ b/src/soc/intel/skylake/acpi.c @@ -200,9 +200,13 @@
unsigned long acpi_fill_mcfg(unsigned long current) { - current += acpi_create_mcfg_mmconfig((acpi_mcfg_mmconfig_t *)current, - CONFIG_MMCONF_BASE_ADDRESS, 0, 0, - (CONFIG_SA_PCIEX_LENGTH >> 20) - 1); + /* + * PCI Segment Group depends on CONFIG_SA_PCI_SEGMENT_GROUPS, + * Start Bus Number 0, End Bus Number is 255 + */ + for (int i = 0; i < CONFIG_SA_PCI_SEGMENT_GROUPS; i++) + current += acpi_create_mcfg_mmconfig((void *)current, + CONFIG_MMCONF_BASE_ADDRESS, i, 0, 255); return current; }
diff --git a/src/soc/intel/skylake/systemagent.c b/src/soc/intel/skylake/systemagent.c index 9a4c4de..a71dbb2 100644 --- a/src/soc/intel/skylake/systemagent.c +++ b/src/soc/intel/skylake/systemagent.c @@ -31,7 +31,8 @@ struct device *const igd_dev = pcidev_path_on_root(SA_DEVFN_IGD);
static const struct sa_mmio_descriptor soc_fixed_resources[] = { - { PCIEXBAR, CONFIG_MMCONF_BASE_ADDRESS, CONFIG_SA_PCIEX_LENGTH, + { PCIEXBAR, CONFIG_MMCONF_BASE_ADDRESS, + PCIEXBAR_LENGTH_MiB(CONFIG_SA_PCI_SEGMENT_GROUPS), "PCIEXBAR" }, { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR" }, { DMIBAR, DMI_BASE_ADDRESS, DMI_BASE_SIZE, "DMIBAR" }, diff --git a/src/soc/intel/tigerlake/Kconfig b/src/soc/intel/tigerlake/Kconfig index a690acf..93f12f9 100644 --- a/src/soc/intel/tigerlake/Kconfig +++ b/src/soc/intel/tigerlake/Kconfig @@ -60,6 +60,7 @@ select UDK_2017_BINDING select DISPLAY_FSP_VERSION_INFO select HECI_DISABLE_USING_SMM + select TCSS_PCIE_SEGMENT
config DCACHE_RAM_BASE default 0xfef00000 diff --git a/src/soc/intel/tigerlake/acpi/pci_irqs.asl b/src/soc/intel/tigerlake/acpi/pci_irqs.asl index 62520b1..7603362 100644 --- a/src/soc/intel/tigerlake/acpi/pci_irqs.asl +++ b/src/soc/intel/tigerlake/acpi/pci_irqs.asl @@ -73,11 +73,6 @@ Package(){0x000DFFFF, 1, 0, xDCI_IRQ }, /* D8: GNA */ Package(){0x0008FFFF, 0, 0, GNA_IRQ }, - /* D7: TBT PCIe */ - Package(){0x0007FFFF, 0, 0, TBT_PCIe0_IRQ }, - Package(){0x0007FFFF, 1, 0, TBT_PCIe1_IRQ }, - Package(){0x0007FFFF, 2, 0, TBT_PCIe2_IRQ }, - Package(){0x0007FFFF, 3, 0, TBT_PCIe3_IRQ }, /* D6: PEG60 */ Package(){0x0006FFFF, 0, 0, PEG_IRQ }, /* D5: IPU Device */ @@ -142,11 +137,6 @@ Package(){0x000DFFFF, 1, 0, 10 }, /* D8: GNA */ Package(){0x0008FFFF, 0, 0, 11 }, - /* D7: TBT PCIe */ - Package(){0x0007FFFF, 0, 0, 11 }, - Package(){0x0007FFFF, 1, 0, 10 }, - Package(){0x0007FFFF, 2, 0, 11 }, - Package(){0x0007FFFF, 3, 0, 11 }, /* D6: PEG60 */ Package(){0x0006FFFF, 0, 0, 11 }, /* D5: IPU Device */ diff --git a/src/soc/intel/tigerlake/include/soc/irq.h b/src/soc/intel/tigerlake/include/soc/irq.h index 01ee10b..1057f91 100644 --- a/src/soc/intel/tigerlake/include/soc/irq.h +++ b/src/soc/intel/tigerlake/include/soc/irq.h @@ -54,11 +54,6 @@
#define ISH_IRQ 16
-#define TBT_PCIe0_IRQ 16 -#define TBT_PCIe1_IRQ 17 -#define TBT_PCIe2_IRQ 18 -#define TBT_PCIe3_IRQ 19 - #define HECI_1_IRQ 16 #define HECI_2_IRQ 17 #define HECI_3_IRQ 16 diff --git a/src/soc/intel/tigerlake/systemagent.c b/src/soc/intel/tigerlake/systemagent.c index 8859386..a6d7ae8 100644 --- a/src/soc/intel/tigerlake/systemagent.c +++ b/src/soc/intel/tigerlake/systemagent.c @@ -23,7 +23,8 @@ void soc_add_fixed_mmio_resources(struct device *dev, int *index) { static const struct sa_mmio_descriptor soc_fixed_resources[] = { - { PCIEXBAR, CONFIG_MMCONF_BASE_ADDRESS, CONFIG_SA_PCIEX_LENGTH, + { PCIEXBAR, CONFIG_MMCONF_BASE_ADDRESS, + PCIEXBAR_LENGTH_MiB(CONFIG_SA_PCI_SEGMENT_GROUPS), "PCIEXBAR" }, { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR" }, { DMIBAR, DMI_BASE_ADDRESS, DMI_BASE_SIZE, "DMIBAR" },
Hello build bot (Jenkins), Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40722
to look at the new patch set (#2).
Change subject: [WIP] Add Multiple Segment support ......................................................................
[WIP] Add Multiple Segment support
Test Patch for get feedback on design aspect
Change-Id: Ide7e97aa7c48fd0e993b248b38b2ad6ad94a21cb Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/intel/tglrvp/dsdt.asl M src/soc/intel/apollolake/systemagent.c M src/soc/intel/cannonlake/systemagent.c M src/soc/intel/common/block/acpi/acpi.c A src/soc/intel/common/block/acpi/acpi/extrahostbridge.asl A src/soc/intel/common/block/acpi/acpi/pcisegment.asl M src/soc/intel/common/block/include/intelblocks/systemagent.h M src/soc/intel/common/block/systemagent/Kconfig M src/soc/intel/common/block/systemagent/systemagent_def.h M src/soc/intel/common/block/systemagent/systemagent_early.c A src/soc/intel/common/block/tcss/Kconfig M src/soc/intel/icelake/systemagent.c M src/soc/intel/jasperlake/systemagent.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/systemagent.c M src/soc/intel/tigerlake/Kconfig M src/soc/intel/tigerlake/acpi/pci_irqs.asl M src/soc/intel/tigerlake/include/soc/irq.h M src/soc/intel/tigerlake/systemagent.c 19 files changed, 195 insertions(+), 51 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/40722/2
Hello build bot (Jenkins), Andrey Petrov, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40722
to look at the new patch set (#3).
Change subject: [WIP] Add Multiple Segment support ......................................................................
[WIP] Add Multiple Segment support
Test Patch for get feedback on design aspect
Additional PCI1 device been created based on TCSS_PCIE_SEGMENT selection from MB Kconfig
extracted build/dsdt.aml
Device (PCI1) { Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */) // _HID: Hardware ID Name (_CID, EisaId ("PNP0A03") /* PCI Bus */) // _CID: Compatible ID Name (_SEG, One) // _SEG: PCI Segment Name (_UID, One) // _UID: Unique ID Name (_ADR, Zero) // _ADR: Address .... }
Change-Id: Ide7e97aa7c48fd0e993b248b38b2ad6ad94a21cb Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/mainboard/intel/tglrvp/dsdt.asl M src/soc/intel/apollolake/systemagent.c M src/soc/intel/cannonlake/systemagent.c M src/soc/intel/common/block/acpi/acpi.c A src/soc/intel/common/block/acpi/acpi/extrahostbridge.asl A src/soc/intel/common/block/acpi/acpi/pcisegment.asl M src/soc/intel/common/block/include/intelblocks/systemagent.h M src/soc/intel/common/block/systemagent/Kconfig M src/soc/intel/common/block/systemagent/systemagent_def.h M src/soc/intel/common/block/systemagent/systemagent_early.c A src/soc/intel/common/block/tcss/Kconfig M src/soc/intel/icelake/systemagent.c M src/soc/intel/jasperlake/systemagent.c M src/soc/intel/skylake/acpi.c M src/soc/intel/skylake/systemagent.c M src/soc/intel/tigerlake/Kconfig M src/soc/intel/tigerlake/acpi/pci_irqs.asl M src/soc/intel/tigerlake/include/soc/irq.h M src/soc/intel/tigerlake/systemagent.c 19 files changed, 195 insertions(+), 51 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/22/40722/3
Subrata Banik has removed Usha P from this change. ( https://review.coreboot.org/c/coreboot/+/40722 )
Change subject: [WIP] Add Multiple Segment support ......................................................................
Removed reviewer Usha P.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40722 )
Change subject: [WIP] Add Multiple Segment support ......................................................................
Patch Set 3:
(3 comments)
Sorry for the delay, it was a busy week.
Looks good overall. I'm not 100% sure about the resources reported in _CRS. The spec says PCI segment groups are a mere software features that shouldn't affect the hardware. And in hardware there can't be a second device that would positively decode the same resources anyway. I'm not sure if we have to report these resource again. Maybe we don't need the `extrahostbridge.asl`?
I think we can do the Kconfig and what it entails first, and the ACPI changes in a separate commit.
https://review.coreboot.org/c/coreboot/+/40722/3/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/systemagent.h:
https://review.coreboot.org/c/coreboot/+/40722/3/src/soc/intel/common/block/... PS3, Line 13: x Maybe give this a name for clarity, e.g. `segments`.
Alternatively, as we seem to pass always the same value anyway:
#define PCIEXBAR_LENGTH_MIB (CONFIG_PCI_SEGMENT_GROUPS * 256 * MiB)
https://review.coreboot.org/c/coreboot/+/40722/3/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/40722/3/src/soc/intel/common/block/... PS3, Line 12: config SA_PCI_SEGMENT_GROUPS This is not Intel specific, IMO it belongs into src/device/Kconfig. Without the SA_ prefix ofc.
Only the `default 2 if TCSS_PCIE_SEGMENT` should be given here.
https://review.coreboot.org/c/coreboot/+/40722/3/src/soc/intel/common/block/... PS3, Line 13: hex Can be `int`.
Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40722 )
Change subject: [WIP] Add Multiple Segment support ......................................................................
Patch Set 3:
just my $0.02 but given the complexity of AML code we have, wouldn't it make sense to generate with acpigen?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40722 )
Change subject: [WIP] Add Multiple Segment support ......................................................................
Patch Set 3:
I'm not sure if we have to report these resource again. Maybe we don't need the `extrahostbridge.asl`?
i was referring to Intel RC code for customer where i could find the usage of "extrahostbridge.asl" for additional segment,
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40722 )
Change subject: [WIP] Add Multiple Segment support ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40722/3/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/systemagent.h:
https://review.coreboot.org/c/coreboot/+/40722/3/src/soc/intel/common/block/... PS3, Line 13: x
Maybe give this a name for clarity, e.g. `segments`. […]
Ack
https://review.coreboot.org/c/coreboot/+/40722/3/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/Kconfig:
https://review.coreboot.org/c/coreboot/+/40722/3/src/soc/intel/common/block/... PS3, Line 12: config SA_PCI_SEGMENT_GROUPS
This is not Intel specific, IMO it belongs into src/device/Kconfig. Without […]
Ack
https://review.coreboot.org/c/coreboot/+/40722/3/src/soc/intel/common/block/... PS3, Line 13: hex
Can be `int`.
Ack
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40722 )
Change subject: [WIP] Add Multiple Segment support ......................................................................
Patch Set 3:
Patch Set 3:
just my $0.02 but given the complexity of AML code we have, wouldn't it make sense to generate with acpigen?
Yes, Furquan is looking at runtime generation of northbridge.asl effort and we could leverage from that.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40722 )
Change subject: [WIP] Add Multiple Segment support ......................................................................
Patch Set 3:
Patch Set 3:
(3 comments)
Sorry for the delay, it was a busy week.
Looks good overall. I'm not 100% sure about the resources reported in _CRS. The spec says PCI segment groups are a mere software features that shouldn't affect the hardware. And in hardware there can't be a second device that would positively decode the same resources anyway. I'm not sure if we have to report these resource again. Maybe we don't need the `extrahostbridge.asl`?
I think we can do the Kconfig and what it entails first, and the ACPI changes in a separate commit.
@Nico, Thanks for your review time
As suggested split this CL into below, please help to review
Device: https://review.coreboot.org/c/coreboot/+/40335/6 Common TCSS: https://review.coreboot.org/c/coreboot/+/41010/1 Common SA https://review.coreboot.org/c/coreboot/+/40379/3 New SOC capabilities https://review.coreboot.org/c/coreboot/+/40363/5 Fixing compilation error with higher PCI segment group https://review.coreboot.org/c/coreboot/+/40336/6 ASL for Additional segment https://review.coreboot.org/c/coreboot/+/41011/1 Support for TGL board https://review.coreboot.org/c/coreboot/+/41012/1 Select from TGL SoC https://review.coreboot.org/c/coreboot/+/41013/1
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40722 )
Change subject: [WIP] Add Multiple Segment support ......................................................................
Patch Set 3: Code-Review-1
(1 comment)
https://review.coreboot.org/c/coreboot/+/40722/3/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi/pcisegment.asl:
https://review.coreboot.org/c/coreboot/+/40722/3/src/soc/intel/common/block/... PS3, Line 27: Name (_ADR, 0x00000000) A device object must contain either an _HID object or an _ADR object, but should not contain both.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40722 )
Change subject: [WIP] Add Multiple Segment support ......................................................................
Patch Set 3:
just my $0.02 but given the complexity of AML code we have, wouldn't it make sense to generate with acpigen?
We should definitely look into that. But I wouldn't start with a dynamic version before we got the static one right.
I'm not sure if we have to report these resource again. Maybe we don't need the `extrahostbridge.asl`?
i was referring to Intel RC code for customer where i could find the usage of "extrahostbridge.asl" for additional segment,
And the authors of that RC code do they have more experience with multiple PCI segment groups than we have? I don't think anybody gets such things right on the first try.
I have looked further into this and disovered the ACPI "Module" device. It groups multiple devices together and can specify shared resources for them. Coincidentally, after learning about this, it turned out that the ACPI spec uses it in the example code for _SEG ;)
Please have a look at ACPI spec 6.3, `6.5.6.1 Example`. I think this is how we should do it. Basically:
Device (ND0) { Name(_HID, "ACPI0004") /* Module Device */
Method(_CRS, ...)
Device (PCI0) { ... }
Device (PCI1) { ... } }
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40722 )
Change subject: [WIP] Add Multiple Segment support ......................................................................
Patch Set 3:
Patch Set 3:
just my $0.02 but given the complexity of AML code we have, wouldn't it make sense to generate with acpigen?
We should definitely look into that. But I wouldn't start with a dynamic version before we got the static one right.
I'm not sure if we have to report these resource again. Maybe we don't need the `extrahostbridge.asl`?
i was referring to Intel RC code for customer where i could find the usage of "extrahostbridge.asl" for additional segment,
And the authors of that RC code do they have more experience with multiple PCI segment groups than we have? I don't think anybody gets such things right on the first try.
I have looked further into this and disovered the ACPI "Module" device. It groups multiple devices together and can specify shared resources for them. Coincidentally, after learning about this, it turned out that the ACPI spec uses it in the example code for _SEG ;)
Please have a look at ACPI spec 6.3, `6.5.6.1 Example`. I think this is how we should do it. Basically:
Device (ND0) { Name(_HID, "ACPI0004") /* Module Device */ Method(_CRS, ...) Device (PCI0) { ... } Device (PCI1) { ... } }
Thanks for the point, yes, those references looks good.But i was adhering to Intel RC code which has been tested with this feature
https://github.com/otcshare/CCG-TGL-Generic-SiC/tree/master/ClientOneSilicon...
In current coreboot pci tree, it appear as below.
Scope (_SB) { Device (PCI0) { Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */) // _HID: Hardware ID Name (_CID, EisaId ("PNP0A03") /* PCI Bus */) // _CID: Compatible ID Name (_SEG, Zero) // _SEG: PCI Segment Name (_UID, Zero) // _UID: Unique ID Device (MCHC) { Name (_ADR, Zero) // _ADR: Address OperationRegion (MCHP, PCI_Config, Zero, 0x0100) Field (MCHP, DWordAcc, NoLock, Preserve) { Offset (0x40), EPEN, 1, , 11, EPBR, 20, Offset (0x48), MHEN, 1, , 14, MHBR, 17, Offset (0x60), PXEN, 1, PXSZ, 2, , 23, PXBR, 6, Offset (0x68), DIEN, 1, , 11, DIBR, 20, Offset (0xA0), TOM, 64, TUUD, 64, Offset (0xBC), TLUD, 32 } }
Method (_CRS, 0, Serialized) // _CRS: Current Resource Settings { Name (MCRS, ResourceTemplate () { ..... } .... } }
Device (PCI1) { Name (_HID, EisaId ("PNP0A08") /* PCI Express Bus */) // _HID: Hardware ID Name (_CID, EisaId ("PNP0A03") /* PCI Bus */) // _CID: Compatible ID Name (_SEG, One) // _SEG: PCI Segment Name (_UID, One) // _UID: Unique ID Method (_CRS, 0, Serialized) // _CRS: Current Resource Settings { Name (MCRS, ResourceTemplate () { ..... } .... } } }
Now we can optimize the common resources between PCI0 and PCI1 later and keep side common space for sharing.
I think we can take optimization pieces later?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40722 )
Change subject: [WIP] Add Multiple Segment support ......................................................................
Patch Set 3:
I'm not sure if we have to report these resource again. Maybe we don't need the `extrahostbridge.asl`?
i was referring to Intel RC code for customer where i could find the usage of "extrahostbridge.asl" for additional segment,
And the authors of that RC code do they have more experience with multiple PCI segment groups than we have? I don't think anybody gets such things right on the first try.
I have looked further into this and disovered the ACPI "Module" device. It groups multiple devices together and can specify shared resources for them. Coincidentally, after learning about this, it turned out that the ACPI spec uses it in the example code for _SEG ;)
Please have a look at ACPI spec 6.3, `6.5.6.1 Example`. I think this is how we should do it. Basically:
Device (ND0) { Name(_HID, "ACPI0004") /* Module Device */ Method(_CRS, ...) Device (PCI0) { ... } Device (PCI1) { ... } }Thanks for the point, yes, those references looks good.But i was adhering to Intel RC code which has been tested with this feature
Please elaborate what and how exactly was tested.
https://github.com/otcshare/CCG-TGL-Generic-SiC/tree/master/ClientOneSilicon...
404
In current coreboot pci tree, it appear as below.
Scope (_SB) {
...
}
I know, I have reviewed it.
Now we can optimize the common resources between PCI0 and PCI1 later and keep side common space for sharing.
It's not an optimization. Pretending that there are two devices positively decoding the same resources is simply wrong.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40722 )
Change subject: [WIP] Add Multiple Segment support ......................................................................
Patch Set 3:
Patch Set 3:
I'm not sure if we have to report these resource again. Maybe we don't need the `extrahostbridge.asl`?
i was referring to Intel RC code for customer where i could find the usage of "extrahostbridge.asl" for additional segment,
And the authors of that RC code do they have more experience with multiple PCI segment groups than we have? I don't think anybody gets such things right on the first try.
I have looked further into this and disovered the ACPI "Module" device. It groups multiple devices together and can specify shared resources for them. Coincidentally, after learning about this, it turned out that the ACPI spec uses it in the example code for _SEG ;)
Please have a look at ACPI spec 6.3, `6.5.6.1 Example`. I think this is how we should do it. Basically:
Device (ND0) { Name(_HID, "ACPI0004") /* Module Device */ Method(_CRS, ...) Device (PCI0) { ... } Device (PCI1) { ... } }Thanks for the point, yes, those references looks good.But i was adhering to Intel RC code which has been tested with this feature
Please elaborate what and how exactly was tested.
TBT devices are functional and any endpoint device sitting is working as expected.
https://github.com/otcshare/CCG-TGL-Generic-SiC/tree/master/ClientOneSilicon...
404
In current coreboot pci tree, it appear as below.
Scope (_SB) {
...
}
I know, I have reviewed it.
Now we can optimize the common resources between PCI0 and PCI1 later and keep side common space for sharing.
It's not an optimization. Pretending that there are two devices positively decoding the same resources is simply wrong.
For PCI device access, shoudn't they use same IO and MMIO resources like 0xCF8/0xCFC or MMIO resource via PCIE base address?
why below resource has to be different for 2 different segments ?
// // Bus Number Allocation: Bus 0 to 0xFF // WORDBusNumber(ResourceProducer,MinFixed,MaxFixed,PosDecode,0x00, 0x0000,0x00FF,0x00,0x0100,,,PB00)
// // I/O Region Allocation 0 ( 0x0000 - 0x0CF7 ) // DWordIo(ResourceProducer,MinFixed,MaxFixed,PosDecode,EntireRange, 0x00,0x0000,0x0CF7,0x00,0x0CF8,,,PI00)
// // I/O Region Allocation 1 ( 0x0D00 - 0xFFFF ) // DWordIo(ResourceProducer,MinFixed,MaxFixed,PosDecode,EntireRange, 0x00,0x0D00,0xFFFF,0x00,0xF300,,,PI01)
// // PCI Memory Region ( TOLUD - 0xDFFFFFFF ) // DWordMemory(ResourceProducer,PosDecode,MinFixed,MaxFixed,NonCacheable, ReadWrite,0x00,0x00000000,0xDFFFFFFF,0x00,0xE0000000,,,PM01)
// // PCI Memory Region ( TOUUD - (TOUUD + ABOVE_4G_MMIO_SIZE) ) // (This is dummy range for OS compatibility, will patch it in _CRS) // QWordMemory(ResourceProducer,PosDecode,MinFixed,MaxFixed,NonCacheable, ReadWrite,0x00,0x10000,0x1FFFF,0x00,0x10000,,,PM02)
These are generic resources for any PCI segment. Isn't it? Having those common make sense to avoid duplication else i don't think there might be any reason for not being same.
HAOUAS Elyes has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/40722 )
Change subject: [WIP] Add Multiple Segment support ......................................................................
Removed Code-Review-1 by HAOUAS Elyes ehaouas@noos.fr
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/40722?usp=email )
Change subject: [WIP] Add Multiple Segment support ......................................................................
Abandoned
-
Andrey Petrov (Code Review) -
HAOUAS Elyes (Code Review)
-
Nico Huber (Code Review)
-
Stefan Reinauer (Code Review)
-
Subrata Banik (Code Review)