Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS
This patch updates system agent related registers bit definitions as per EDS.
For an example: As per EDS MCHBAR register base is between bit 16-38 but coreboot programming was not align with EDS previously.
Also provide provision to program 64bit values as per SA EDS definitions
TEST=Dump MCHBAR in coreboot and ASL shows same 32 bit value.
Change-Id: I37340408fe89c94ce81953c751c8d7e22bc81a42 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/apollolake/romstage.c M src/soc/intel/apollolake/systemagent.c M src/soc/intel/cannonlake/include/soc/systemagent.h M src/soc/intel/cannonlake/romstage/systemagent.c M src/soc/intel/cannonlake/systemagent.c M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/include/intelblocks/systemagent.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/icelake/romstage/systemagent.c M src/soc/intel/icelake/systemagent.c M src/soc/intel/skylake/acpi/systemagent.asl M src/soc/intel/skylake/include/soc/systemagent.h M src/soc/intel/skylake/romstage/systemagent.c M src/soc/intel/skylake/systemagent.c M src/soc/intel/tigerlake/romstage/systemagent.c M src/soc/intel/tigerlake/systemagent.c 16 files changed, 88 insertions(+), 75 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/38387/1
diff --git a/src/soc/intel/apollolake/romstage.c b/src/soc/intel/apollolake/romstage.c index 258f4ff..44eb453 100644 --- a/src/soc/intel/apollolake/romstage.c +++ b/src/soc/intel/apollolake/romstage.c @@ -79,7 +79,7 @@ static void soc_early_romstage_init(void) { static const struct sa_mmio_descriptor soc_fixed_pci_resources[] = { - { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR" }, + { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR", true }, };
/* Set Fixed MMIO address into PCI configuration space */ diff --git a/src/soc/intel/apollolake/systemagent.c b/src/soc/intel/apollolake/systemagent.c index fd91082..f51595f 100644 --- a/src/soc/intel/apollolake/systemagent.c +++ b/src/soc/intel/apollolake/systemagent.c @@ -36,8 +36,8 @@ { static const struct sa_mmio_descriptor soc_fixed_resources[] = { { PCIEXBAR, CONFIG_MMCONF_BASE_ADDRESS, CONFIG_SA_PCIEX_LENGTH, - "PCIEXBAR" }, - { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR" }, + "PCIEXBAR", true }, + { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR", true }, };
sa_add_fixed_mmio_resources(dev, index, soc_fixed_resources, diff --git a/src/soc/intel/cannonlake/include/soc/systemagent.h b/src/soc/intel/cannonlake/include/soc/systemagent.h index 3bda9e8..ed86a99 100644 --- a/src/soc/intel/cannonlake/include/soc/systemagent.h +++ b/src/soc/intel/cannonlake/include/soc/systemagent.h @@ -53,14 +53,14 @@ #if CONFIG(SOC_INTEL_COFFEELAKE) || CONFIG(SOC_INTEL_WHISKEYLAKE) \ || CONFIG(SOC_INTEL_COMETLAKE) static const struct sa_mmio_descriptor soc_vtd_resources[] = { - { GFXVTBAR, GFXVT_BASE_ADDRESS, GFXVT_BASE_SIZE, "GFXVTBAR" }, - { VTVC0BAR, VTVC0_BASE_ADDRESS, VTVC0_BASE_SIZE, "VTVC0BAR" }, + { GFXVTBAR, GFXVT_BASE_ADDRESS, GFXVT_BASE_SIZE, "GFXVTBAR", true }, + { VTVC0BAR, VTVC0_BASE_ADDRESS, VTVC0_BASE_SIZE, "VTVC0BAR", true }, }; #else static const struct sa_mmio_descriptor soc_vtd_resources[] = { - { GFXVTBAR, GFXVT_BASE_ADDRESS, GFXVT_BASE_SIZE, "GFXVTBAR" }, - { IPUVTBAR, IPUVT_BASE_ADDRESS, IPUVT_BASE_SIZE, "IPUVTBAR" }, - { VTVC0BAR, VTVC0_BASE_ADDRESS, VTVC0_BASE_SIZE, "VTVC0BAR" }, + { GFXVTBAR, GFXVT_BASE_ADDRESS, GFXVT_BASE_SIZE, "GFXVTBAR", true }, + { IPUVTBAR, IPUVT_BASE_ADDRESS, IPUVT_BASE_SIZE, "IPUVTBAR", true }, + { VTVC0BAR, VTVC0_BASE_ADDRESS, VTVC0_BASE_SIZE, "VTVC0BAR", true }, }; #endif
diff --git a/src/soc/intel/cannonlake/romstage/systemagent.c b/src/soc/intel/cannonlake/romstage/systemagent.c index 61db22e..0e50c12 100644 --- a/src/soc/intel/cannonlake/romstage/systemagent.c +++ b/src/soc/intel/cannonlake/romstage/systemagent.c @@ -24,14 +24,14 @@ void systemagent_early_init(void) { static const struct sa_mmio_descriptor soc_fixed_pci_resources[] = { - { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR" }, - { DMIBAR, DMI_BASE_ADDRESS, DMI_BASE_SIZE, "DMIBAR" }, - { EPBAR, EP_BASE_ADDRESS, EP_BASE_SIZE, "EPBAR" }, + { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR", true }, + { DMIBAR, DMI_BASE_ADDRESS, DMI_BASE_SIZE, "DMIBAR", true }, + { EPBAR, EP_BASE_ADDRESS, EP_BASE_SIZE, "EPBAR", true }, };
static const struct sa_mmio_descriptor soc_fixed_mch_resources[] = { - { REGBAR, REG_BASE_ADDRESS, REG_BASE_SIZE, "REGBAR" }, - { EDRAMBAR, EDRAM_BASE_ADDRESS, EDRAM_BASE_SIZE, "EDRAMBAR" }, + { REGBAR, REG_BASE_ADDRESS, REG_BASE_SIZE, "REGBAR", true }, + { EDRAMBAR, EDRAM_BASE_ADDRESS, EDRAM_BASE_SIZE, "EDRAMBAR", true }, };
/* Set Fixed MMIO address into PCI configuration space */ diff --git a/src/soc/intel/cannonlake/systemagent.c b/src/soc/intel/cannonlake/systemagent.c index 3f01f14..84513a0 100644 --- a/src/soc/intel/cannonlake/systemagent.c +++ b/src/soc/intel/cannonlake/systemagent.c @@ -35,12 +35,12 @@ { static const struct sa_mmio_descriptor soc_fixed_resources[] = { { PCIEXBAR, CONFIG_MMCONF_BASE_ADDRESS, CONFIG_SA_PCIEX_LENGTH, - "PCIEXBAR" }, - { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR" }, - { DMIBAR, DMI_BASE_ADDRESS, DMI_BASE_SIZE, "DMIBAR" }, - { EPBAR, EP_BASE_ADDRESS, EP_BASE_SIZE, "EPBAR" }, - { REGBAR, REG_BASE_ADDRESS, REG_BASE_SIZE, "REGBAR" }, - { EDRAMBAR, EDRAM_BASE_ADDRESS, EDRAM_BASE_SIZE, "EDRAMBAR" }, + "PCIEXBAR", true }, + { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR", true }, + { DMIBAR, DMI_BASE_ADDRESS, DMI_BASE_SIZE, "DMIBAR", true }, + { EPBAR, EP_BASE_ADDRESS, EP_BASE_SIZE, "EPBAR", true }, + { REGBAR, REG_BASE_ADDRESS, REG_BASE_SIZE, "REGBAR", true }, + { EDRAMBAR, EDRAM_BASE_ADDRESS, EDRAM_BASE_SIZE, "EDRAMBAR", true }, /* * PMC pci device gets hidden from PCI bus due to Silicon * policy hence binding PMCBAR aka PWRMBASE (offset 0x10) with @@ -51,7 +51,7 @@ * under this device space. */ { PCI_BASE_ADDRESS_0, PCH_PWRM_BASE_ADDRESS, PCH_PWRM_BASE_SIZE, - "PMCBAR" }, + "PMCBAR", true }, };
sa_add_fixed_mmio_resources(dev, index, soc_fixed_resources, diff --git a/src/soc/intel/common/block/acpi/acpi/northbridge.asl b/src/soc/intel/common/block/acpi/acpi/northbridge.asl index d271dda..ab50942 100644 --- a/src/soc/intel/common/block/acpi/acpi/northbridge.asl +++ b/src/soc/intel/common/block/acpi/acpi/northbridge.asl @@ -31,23 +31,23 @@ Offset(0x40), /* EPBAR (0:0:0:40) */ EPEN, 1, /* Enable */ , 11, - EPBR, 20, /* EPBAR [31:12] */ + EPBR, 27, /* EPBAR [38:12] */
Offset(0x48), /* MCHBAR (0:0:0:48) */ MHEN, 1, /* Enable */ - , 14, - MHBR, 17, /* MCHBAR [31:15] */ + , 15, + MHBR, 23, /* MCHBAR [38:16] */
Offset(0x60), /* PCIEXBAR (0:0:0:60) */ PXEN, 1, /* Enable */ PXSZ, 2, /* PCI Express Size */ - , 23, - PXBR, 6, /* PCI Express BAR [31:26] */ + , 25, + PXBR, 11, /* PCI Express BAR [38:28] */
Offset(0x68), /* DMIBAR (0:0:0:68) */ DIEN, 1, /* Enable */ , 11, - DIBR, 20, /* DMIBAR [31:12] */ + DIBR, 27, /* DMIBAR [38:12] */
Offset (0xa0), TOM, 64, /* Top of Used Memory */ @@ -220,7 +220,7 @@ /* Get MCH BAR */ Method (GMHB, 0, Serialized) { - ShiftLeft (_SB.PCI0.MCHC.MHBR, 15, Local0) + ShiftLeft (_SB.PCI0.MCHC.MHBR, 16, Local0) Return (Local0) }
@@ -234,7 +234,7 @@ /* Get PCIe BAR */ Method (GPCB, 0, Serialized) { - ShiftLeft (_SB.PCI0.MCHC.PXBR, 26, Local0) + ShiftLeft (_SB.PCI0.MCHC.PXBR, 28, Local0) Return (Local0) }
diff --git a/src/soc/intel/common/block/include/intelblocks/systemagent.h b/src/soc/intel/common/block/include/intelblocks/systemagent.h index c605958..1d69e68 100644 --- a/src/soc/intel/common/block/include/intelblocks/systemagent.h +++ b/src/soc/intel/common/block/include/intelblocks/systemagent.h @@ -43,15 +43,17 @@ * Fixed MMIO range * INDEX = Either PCI configuration space registers or MMIO offsets * mapped from REG. - * BASE = 32 bit Address. + * BASE = Address. * SIZE = base length * DESCRIPTION = Name of the register/offset. + * IS_64_BIT = If registers/offset is 64 bit. */ struct sa_mmio_descriptor { unsigned int index; uintptr_t base; size_t size; const char *description; + bool is_64_bit; };
/* API to set Fixed MMIO address into PCI configuration space */ diff --git a/src/soc/intel/common/block/systemagent/systemagent_early.c b/src/soc/intel/common/block/systemagent/systemagent_early.c index d6f129d..8697dd5 100644 --- a/src/soc/intel/common/block/systemagent/systemagent_early.c +++ b/src/soc/intel/common/block/systemagent/systemagent_early.c @@ -1,7 +1,7 @@ /* * This file is part of the coreboot project. * - * Copyright (C) 2017 Intel Corporation. + * Copyright (C) 2017-2020 Intel Corporation. * * 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 @@ -71,12 +71,18 @@ int i;
for (i = 0; i < count; i++) { - uintptr_t base; + uint64_t base = 0; unsigned int index; + bool is64bit = fixed_set_resources[i].is_64_bit;
index = fixed_set_resources[i].index; + /* Check if PCI BAR already enabled */ - base = pci_read_config32(SA_DEV_ROOT, index); + if (is64bit) { + base = pci_read_config32(SA_DEV_ROOT, index + 4); + base <<= 32; + } + base |= pci_read_config32(SA_DEV_ROOT, index);
/* If enabled don't program it. */ if (base & 0x1) @@ -85,6 +91,9 @@ base = fixed_set_resources[i].base;
pci_write_config32(SA_DEV_ROOT, index, base | 1); + if (is64bit) { + pci_write_config32(SA_DEV_ROOT, index + 4, base >> 32); + } } }
diff --git a/src/soc/intel/icelake/romstage/systemagent.c b/src/soc/intel/icelake/romstage/systemagent.c index fc046a6..cea38a1 100644 --- a/src/soc/intel/icelake/romstage/systemagent.c +++ b/src/soc/intel/icelake/romstage/systemagent.c @@ -22,14 +22,14 @@ void systemagent_early_init(void) { static const struct sa_mmio_descriptor soc_fixed_pci_resources[] = { - { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR" }, - { DMIBAR, DMI_BASE_ADDRESS, DMI_BASE_SIZE, "DMIBAR" }, - { EPBAR, EP_BASE_ADDRESS, EP_BASE_SIZE, "EPBAR" }, + { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR", true }, + { DMIBAR, DMI_BASE_ADDRESS, DMI_BASE_SIZE, "DMIBAR", true }, + { EPBAR, EP_BASE_ADDRESS, EP_BASE_SIZE, "EPBAR", true }, };
static const struct sa_mmio_descriptor soc_fixed_mch_resources[] = { - { REGBAR, REG_BASE_ADDRESS, REG_BASE_SIZE, "REGBAR" }, - { EDRAMBAR, EDRAM_BASE_ADDRESS, EDRAM_BASE_SIZE, "EDRAMBAR" }, + { REGBAR, REG_BASE_ADDRESS, REG_BASE_SIZE, "REGBAR", true }, + { EDRAMBAR, EDRAM_BASE_ADDRESS, EDRAM_BASE_SIZE, "EDRAMBAR", true }, };
/* Set Fixed MMIO address into PCI configuration space */ diff --git a/src/soc/intel/icelake/systemagent.c b/src/soc/intel/icelake/systemagent.c index 930e78e..e57f2e8 100644 --- a/src/soc/intel/icelake/systemagent.c +++ b/src/soc/intel/icelake/systemagent.c @@ -29,12 +29,12 @@ { static const struct sa_mmio_descriptor soc_fixed_resources[] = { { PCIEXBAR, CONFIG_MMCONF_BASE_ADDRESS, CONFIG_SA_PCIEX_LENGTH, - "PCIEXBAR" }, - { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR" }, - { DMIBAR, DMI_BASE_ADDRESS, DMI_BASE_SIZE, "DMIBAR" }, - { EPBAR, EP_BASE_ADDRESS, EP_BASE_SIZE, "EPBAR" }, - { REGBAR, REG_BASE_ADDRESS, REG_BASE_SIZE, "REGBAR" }, - { EDRAMBAR, EDRAM_BASE_ADDRESS, EDRAM_BASE_SIZE, "EDRAMBAR" }, + "PCIEXBAR", true }, + { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR", true }, + { DMIBAR, DMI_BASE_ADDRESS, DMI_BASE_SIZE, "DMIBAR", true }, + { EPBAR, EP_BASE_ADDRESS, EP_BASE_SIZE, "EPBAR", true }, + { REGBAR, REG_BASE_ADDRESS, REG_BASE_SIZE, "REGBAR", true }, + { EDRAMBAR, EDRAM_BASE_ADDRESS, EDRAM_BASE_SIZE, "EDRAMBAR", true }, /* * PMC pci device gets hidden from PCI bus due to Silicon * policy hence binding PMCBAR aka PWRMBASE (offset 0x10) with @@ -45,7 +45,7 @@ * under this device space. */ { PCI_BASE_ADDRESS_0, PCH_PWRM_BASE_ADDRESS, PCH_PWRM_BASE_SIZE, - "PMCBAR" }, + "PMCBAR", true }, };
sa_add_fixed_mmio_resources(dev, index, soc_fixed_resources, diff --git a/src/soc/intel/skylake/acpi/systemagent.asl b/src/soc/intel/skylake/acpi/systemagent.asl index 89380aa..ca79090 100644 --- a/src/soc/intel/skylake/acpi/systemagent.asl +++ b/src/soc/intel/skylake/acpi/systemagent.asl @@ -30,23 +30,23 @@ Offset(0x40), /* EPBAR (0:0:0:40) */ EPEN, 1, /* Enable */ , 11, - EPBR, 20, /* EPBAR [31:12] */ + EPBR, 27, /* EPBAR [38:12] */
Offset(0x48), /* MCHBAR (0:0:0:48) */ MHEN, 1, /* Enable */ , 14, - MHBR, 17, /* MCHBAR [31:15] */ + MHBR, 24, /* MCHBAR [38:15] */
Offset(0x60), /* PCIEXBAR (0:0:0:60) */ PXEN, 1, /* Enable */ PXSZ, 2, /* PCI Express Size */ - , 23, - PXBR, 6, /* PCI Express BAR [31:26] */ + , 25, + PXBR, 11, /* PCI Express BAR [38:28] */
Offset(0x68), /* DMIBAR (0:0:0:68) */ DIEN, 1, /* Enable */ , 11, - DIBR, 20, /* DMIBAR [31:12] */ + DIBR, 27, /* DMIBAR [38:12] */
Offset (0x70), /* ME Base Address */ MEBA, 64, @@ -244,7 +244,7 @@ /* Get PCIe BAR */ Method (GPCB, 0, Serialized) { - ShiftLeft (_SB.PCI0.MCHC.PXBR, 26, Local0) + ShiftLeft (_SB.PCI0.MCHC.PXBR, 28, Local0) Return (Local0) }
diff --git a/src/soc/intel/skylake/include/soc/systemagent.h b/src/soc/intel/skylake/include/soc/systemagent.h index 91209c8..bf0acc2 100644 --- a/src/soc/intel/skylake/include/soc/systemagent.h +++ b/src/soc/intel/skylake/include/soc/systemagent.h @@ -52,14 +52,16 @@ GFXVTBAR, GFXVT_BASE_ADDRESS, GFXVT_BASE_SIZE, - "GFXVTBAR" + "GFXVTBAR", + true };
static const struct sa_mmio_descriptor soc_vtvc0_mmio_descriptor = { VTVC0BAR, VTVC0_BASE_ADDRESS, VTVC0_BASE_SIZE, - "VTVC0BAR" + "VTVC0BAR", + true };
/* Hardcoded default values for PCI Bus:Dev.Fun for IOAPIC and HPET */ diff --git a/src/soc/intel/skylake/romstage/systemagent.c b/src/soc/intel/skylake/romstage/systemagent.c index e1272a1..1316b5a 100644 --- a/src/soc/intel/skylake/romstage/systemagent.c +++ b/src/soc/intel/skylake/romstage/systemagent.c @@ -52,14 +52,14 @@ void systemagent_early_init(void) { static const struct sa_mmio_descriptor soc_fixed_pci_resources[] = { - { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR" }, - { DMIBAR, DMI_BASE_ADDRESS, DMI_BASE_SIZE, "DMIBAR" }, - { EPBAR, EP_BASE_ADDRESS, EP_BASE_SIZE, "EPBAR" }, + { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR", true }, + { DMIBAR, DMI_BASE_ADDRESS, DMI_BASE_SIZE, "DMIBAR", true }, + { EPBAR, EP_BASE_ADDRESS, EP_BASE_SIZE, "EPBAR", true }, };
static const struct sa_mmio_descriptor soc_fixed_mch_resources[] = { - { GDXCBAR, GDXC_BASE_ADDRESS, GDXC_BASE_SIZE, "GDXCBAR" }, - { EDRAMBAR, EDRAM_BASE_ADDRESS, EDRAM_BASE_SIZE, "EDRAMBAR" }, + { GDXCBAR, GDXC_BASE_ADDRESS, GDXC_BASE_SIZE, "GDXCBAR", true }, + { EDRAMBAR, EDRAM_BASE_ADDRESS, EDRAM_BASE_SIZE, "EDRAMBAR", true }, };
/* Set Fixed MMIO address into PCI configuration space */ diff --git a/src/soc/intel/skylake/systemagent.c b/src/soc/intel/skylake/systemagent.c index 410265f..a9168f1 100644 --- a/src/soc/intel/skylake/systemagent.c +++ b/src/soc/intel/skylake/systemagent.c @@ -46,12 +46,12 @@
static const struct sa_mmio_descriptor soc_fixed_resources[] = { { PCIEXBAR, CONFIG_MMCONF_BASE_ADDRESS, CONFIG_SA_PCIEX_LENGTH, - "PCIEXBAR" }, - { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR" }, - { DMIBAR, DMI_BASE_ADDRESS, DMI_BASE_SIZE, "DMIBAR" }, - { EPBAR, EP_BASE_ADDRESS, EP_BASE_SIZE, "EPBAR" }, - { GDXCBAR, GDXC_BASE_ADDRESS, GDXC_BASE_SIZE, "GDXCBAR" }, - { EDRAMBAR, EDRAM_BASE_ADDRESS, EDRAM_BASE_SIZE, "EDRAMBAR" }, + "PCIEXBAR", true }, + { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR", true }, + { DMIBAR, DMI_BASE_ADDRESS, DMI_BASE_SIZE, "DMIBAR", true }, + { EPBAR, EP_BASE_ADDRESS, EP_BASE_SIZE, "EPBAR", true }, + { GDXCBAR, GDXC_BASE_ADDRESS, GDXC_BASE_SIZE, "GDXCBAR", true }, + { EDRAMBAR, EDRAM_BASE_ADDRESS, EDRAM_BASE_SIZE, "EDRAMBAR", true }, }; const struct soc_intel_skylake_config *const config = config_of(dev);
diff --git a/src/soc/intel/tigerlake/romstage/systemagent.c b/src/soc/intel/tigerlake/romstage/systemagent.c index 183089e..db721c3 100644 --- a/src/soc/intel/tigerlake/romstage/systemagent.c +++ b/src/soc/intel/tigerlake/romstage/systemagent.c @@ -27,14 +27,14 @@ void systemagent_early_init(void) { static const struct sa_mmio_descriptor soc_fixed_pci_resources[] = { - { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR" }, - { DMIBAR, DMI_BASE_ADDRESS, DMI_BASE_SIZE, "DMIBAR" }, - { EPBAR, EP_BASE_ADDRESS, EP_BASE_SIZE, "EPBAR" }, + { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR", true }, + { DMIBAR, DMI_BASE_ADDRESS, DMI_BASE_SIZE, "DMIBAR", true }, + { EPBAR, EP_BASE_ADDRESS, EP_BASE_SIZE, "EPBAR", true }, };
static const struct sa_mmio_descriptor soc_fixed_mch_resources[] = { - { REGBAR, REG_BASE_ADDRESS, REG_BASE_SIZE, "REGBAR" }, - { EDRAMBAR, EDRAM_BASE_ADDRESS, EDRAM_BASE_SIZE, "EDRAMBAR" }, + { REGBAR, REG_BASE_ADDRESS, REG_BASE_SIZE, "REGBAR", true }, + { EDRAMBAR, EDRAM_BASE_ADDRESS, EDRAM_BASE_SIZE, "EDRAMBAR", true }, };
/* Set Fixed MMIO address into PCI configuration space */ diff --git a/src/soc/intel/tigerlake/systemagent.c b/src/soc/intel/tigerlake/systemagent.c index 9c8f645..e0065e0 100644 --- a/src/soc/intel/tigerlake/systemagent.c +++ b/src/soc/intel/tigerlake/systemagent.c @@ -35,12 +35,12 @@ { static const struct sa_mmio_descriptor soc_fixed_resources[] = { { PCIEXBAR, CONFIG_MMCONF_BASE_ADDRESS, CONFIG_SA_PCIEX_LENGTH, - "PCIEXBAR" }, - { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR" }, - { DMIBAR, DMI_BASE_ADDRESS, DMI_BASE_SIZE, "DMIBAR" }, - { EPBAR, EP_BASE_ADDRESS, EP_BASE_SIZE, "EPBAR" }, - { REGBAR, REG_BASE_ADDRESS, REG_BASE_SIZE, "REGBAR" }, - { EDRAMBAR, EDRAM_BASE_ADDRESS, EDRAM_BASE_SIZE, "EDRAMBAR" }, + "PCIEXBAR", true }, + { MCHBAR, MCH_BASE_ADDRESS, MCH_BASE_SIZE, "MCHBAR", true }, + { DMIBAR, DMI_BASE_ADDRESS, DMI_BASE_SIZE, "DMIBAR", true }, + { EPBAR, EP_BASE_ADDRESS, EP_BASE_SIZE, "EPBAR", true }, + { REGBAR, REG_BASE_ADDRESS, REG_BASE_SIZE, "REGBAR", true }, + { EDRAMBAR, EDRAM_BASE_ADDRESS, EDRAM_BASE_SIZE, "EDRAMBAR", true }, /* * PMC pci device gets hidden from PCI bus due to Silicon * policy hence binding PMCBAR aka PWRMBASE (offset 0x10) with @@ -51,7 +51,7 @@ * under this device space. */ { PCI_BASE_ADDRESS_0, PCH_PWRM_BASE_ADDRESS, PCH_PWRM_BASE_SIZE, - "PMCBAR" }, + "PMCBAR", true }, };
sa_add_fixed_mmio_resources(dev, index, soc_fixed_resources,
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38387/1/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/38387/1/src/soc/intel/common/block/... PS1, Line 94: if (is64bit) { braces {} are not necessary for single statement blocks
Hello Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38387
to look at the new patch set (#2).
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS
This patch updates system agent related registers bit definitions as per EDS.
For an example: As per EDS MCHBAR register base is between bit 16-38 but coreboot programming was not align with EDS previously.
Also provide provision to program 64bit values as per SA EDS definitions
TEST=Dump MCHBAR in coreboot and ASL shows same 32 bit value.
Change-Id: I37340408fe89c94ce81953c751c8d7e22bc81a42 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/apollolake/romstage.c M src/soc/intel/apollolake/systemagent.c M src/soc/intel/cannonlake/include/soc/systemagent.h M src/soc/intel/cannonlake/romstage/systemagent.c M src/soc/intel/cannonlake/systemagent.c M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/include/intelblocks/systemagent.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/icelake/romstage/systemagent.c M src/soc/intel/icelake/systemagent.c M src/soc/intel/skylake/acpi/systemagent.asl M src/soc/intel/skylake/include/soc/systemagent.h M src/soc/intel/skylake/romstage/systemagent.c M src/soc/intel/skylake/systemagent.c M src/soc/intel/tigerlake/romstage/systemagent.c M src/soc/intel/tigerlake/systemagent.c 16 files changed, 90 insertions(+), 76 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/38387/2
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38387/1/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/38387/1/src/soc/intel/common/block/... PS1, Line 94: if (is64bit) {
braces {} are not necessary for single statement blocks
Ack
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/systemagent.h:
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/common/block/... PS2, Line 56: is_64_bit Is there any case in which this is false?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/systemagent.h:
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/common/block/... PS2, Line 56: is_64_bit
Is there any case in which this is false?
looks like right now all resources that we are passing are 64 bit width but their might be some fixed resource which possible to be 32 bit width as well hence i have created this additional field else it would be hardcoding a lot
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/systemagent.h:
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/common/block/... PS2, Line 56: is_64_bit
looks like right now all resources that we are passing are 64 bit width but their might be some fixe […]
Ack
Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/systemagent.h:
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/cannonlake/in... PS2, Line 56: true Using something like sa_mmio_descriptor_64 to move the "true" into header file?
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/common/block/... PS2, Line 94: if (is64bit) Without that condition? Writing zero shall be no harm?
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/common/block/... PS2, Line 80: /* Check if PCI BAR already enabled */ : if (is64bit) { : base = pci_read_config32(SA_DEV_ROOT, index + 4); : base <<= 32; : } : base |= pci_read_config32(SA_DEV_ROOT, index); : : /* If enabled don't program it. */ : if (base & 0x1) : return; : : base = fixed_set_resources[i].base; : : pci_write_config32(SA_DEV_ROOT, index, base | 1); : if (is64bit) : pci_write_config32(SA_DEV_ROOT, index + 4, base >> 32); : } My personal opinion will have two separated body, one for 32 bit and another for 64 bit will be more easier for reading. But that's just my 2 cents.
Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 2:
How come it had been working so far without point out by Furquan?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 2:
Patch Set 2:
How come it had been working so far without point out by Furquan?
Yes Lance, i have dig into this and answer is here https://review.coreboot.org/c/coreboot/+/38154/4/src/soc/intel/cannonlake/ac...
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/cannonlake/in... File src/soc/intel/cannonlake/include/soc/systemagent.h:
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/cannonlake/in... PS2, Line 56: true
Using something like sa_mmio_descriptor_64 to move the "true" into header file?
yes, this is possible to create but then we might not be able to use a single function due to 2 different structure dependency in argument hence decided of having single function which will act based on is_64_bit structure variable. Right now all these fixed resources are 64 bit wide that might be the reason it looks like little odd as multiple true but avoiding that might required more work to be done to support dedicated structure and separation in calling function from soc to common code as i have explained in common file review comments
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/common/block/... PS2, Line 94: if (is64bit)
Without that condition? Writing zero shall be no harm?
Code here is to check if BAR has be implemented or not
uint64_t base = 0; unsigned int index; bool is64bit = fixed_set_resources[i].is_64_bit;
index = fixed_set_resources[i].index; /* Check if PCI BAR already enabled */ if (is64bit) { base = pci_read_config32(SA_DEV_ROOT, index + 4); base <<= 32; } base |= pci_read_config32(SA_DEV_ROOT, index);
And writing into register would only depends on base = fixed_set_resources[i].base; so i don/'t think we are writing all zero ? isn't it ?
https://review.coreboot.org/c/coreboot/+/38387/2/src/soc/intel/common/block/... PS2, Line 80: /* Check if PCI BAR already enabled */ : if (is64bit) { : base = pci_read_config32(SA_DEV_ROOT, index + 4); : base <<= 32; : } : base |= pci_read_config32(SA_DEV_ROOT, index); : : /* If enabled don't program it. */ : if (base & 0x1) : return; : : base = fixed_set_resources[i].base; : : pci_write_config32(SA_DEV_ROOT, index, base | 1); : if (is64bit) : pci_write_config32(SA_DEV_ROOT, index + 4, base >> 32); : }
My personal opinion will have two separated body, one for 32 bit and another for 64 bit will be more […]
yes we can do that, but then it would be redundant and caller also might not 100% aware which function to call 32bit or 64 bit write hence decided of having single function which will act based on is_64_bit structure variable.
Lance Zhao has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 3:
Patch Set 2:
(3 comments)
Alright, but the "true" showed up looks not that easily to understand without checking the definition first. Using enum to have _64 and _32 to replace the "true" and "false"?
I was talking about upper registers with +4, if 32 bit, upper 32bit of base will be zero anyway. However that's a bad idea, I can't grantee a redundant write will cause anything.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 3:
Patch Set 3:
Patch Set 2:
(3 comments)
Alright, but the "true" showed up looks not that easily to understand without checking the definition first. Using enum to have _64 and _32 to replace the "true" and "false"?
Sure, let me take a look on this
I was talking about upper registers with +4, if 32 bit, upper 32bit of base will be zero anyway. However that's a bad idea, I can't grantee a redundant write will cause anything.
Agree with you
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Wonkyu Kim, Lance Zhao, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38387
to look at the new patch set (#4).
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS
This patch updates system agent related registers bit definitions as per EDS.
For an example: As per EDS MCHBAR register base is between bit 16-38 but coreboot programming was not align with EDS previously.
Also provide provision to program 64bit values as per SA EDS definitions
TEST=Dump MCHBAR in coreboot and ASL shows same 32 bit value.
Change-Id: I37340408fe89c94ce81953c751c8d7e22bc81a42 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/apollolake/romstage.c M src/soc/intel/apollolake/systemagent.c M src/soc/intel/cannonlake/include/soc/systemagent.h M src/soc/intel/cannonlake/romstage/systemagent.c M src/soc/intel/cannonlake/systemagent.c M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/include/intelblocks/systemagent.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/icelake/romstage/systemagent.c M src/soc/intel/icelake/systemagent.c M src/soc/intel/skylake/acpi/systemagent.asl M src/soc/intel/skylake/include/soc/systemagent.h M src/soc/intel/skylake/romstage/systemagent.c M src/soc/intel/skylake/systemagent.c M src/soc/intel/tigerlake/romstage/systemagent.c M src/soc/intel/tigerlake/systemagent.c 16 files changed, 148 insertions(+), 76 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/38387/4
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/c/coreboot/+/38387/4/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/systemagent.h:
https://review.coreboot.org/c/coreboot/+/38387/4/src/soc/intel/common/block/... PS4, Line 48: RESOURCE_TYPE_32BIT, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38387/4/src/soc/intel/common/block/... PS4, Line 48: RESOURCE_TYPE_32BIT, please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38387/4/src/soc/intel/common/block/... PS4, Line 49: RESOURCE_TYPE_64BIT, code indent should use tabs where possible
https://review.coreboot.org/c/coreboot/+/38387/4/src/soc/intel/common/block/... PS4, Line 49: RESOURCE_TYPE_64BIT, please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38387/4/src/soc/intel/skylake/syste... File src/soc/intel/skylake/systemagent.c:
https://review.coreboot.org/c/coreboot/+/38387/4/src/soc/intel/skylake/syste... PS4, Line 49: "PCIEXBAR",RESOURCE_TYPE_64BIT }, space required after that ',' (ctx:VxV)
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Wonkyu Kim, Lance Zhao, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38387
to look at the new patch set (#5).
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS
This patch updates system agent related registers bit definitions as per EDS.
For an example: As per EDS MCHBAR register base is between bit 16-38 but coreboot programming was not align with EDS previously.
Also provide provision to program 64bit values as per SA EDS definitions
TEST=Dump MCHBAR in coreboot and ASL shows same 32 bit value.
Change-Id: I37340408fe89c94ce81953c751c8d7e22bc81a42 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/apollolake/romstage.c M src/soc/intel/apollolake/systemagent.c M src/soc/intel/cannonlake/include/soc/systemagent.h M src/soc/intel/cannonlake/romstage/systemagent.c M src/soc/intel/cannonlake/systemagent.c M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/include/intelblocks/systemagent.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/icelake/romstage/systemagent.c M src/soc/intel/icelake/systemagent.c M src/soc/intel/skylake/acpi/systemagent.asl M src/soc/intel/skylake/include/soc/systemagent.h M src/soc/intel/skylake/romstage/systemagent.c M src/soc/intel/skylake/systemagent.c M src/soc/intel/tigerlake/romstage/systemagent.c M src/soc/intel/tigerlake/systemagent.c 16 files changed, 148 insertions(+), 76 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/38387/5
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/c/coreboot/+/38387/4/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/systemagent.h:
https://review.coreboot.org/c/coreboot/+/38387/4/src/soc/intel/common/block/... PS4, Line 48: RESOURCE_TYPE_32BIT,
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/38387/4/src/soc/intel/common/block/... PS4, Line 48: RESOURCE_TYPE_32BIT,
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/38387/4/src/soc/intel/common/block/... PS4, Line 49: RESOURCE_TYPE_64BIT,
code indent should use tabs where possible
Done
https://review.coreboot.org/c/coreboot/+/38387/4/src/soc/intel/common/block/... PS4, Line 49: RESOURCE_TYPE_64BIT,
please, no spaces at the start of a line
Done
https://review.coreboot.org/c/coreboot/+/38387/4/src/soc/intel/skylake/syste... File src/soc/intel/skylake/systemagent.c:
https://review.coreboot.org/c/coreboot/+/38387/4/src/soc/intel/skylake/syste... PS4, Line 49: "PCIEXBAR",RESOURCE_TYPE_64BIT },
space required after that ',' (ctx:VxV)
Ack
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 5:
Patch Set 3:
Patch Set 3:
Patch Set 2:
(3 comments)
Alright, but the "true" showed up looks not that easily to understand without checking the definition first. Using enum to have _64 and _32 to replace the "true" and "false"?
Sure, let me take a look on this
Addressed your review comments, kindly review
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 5:
(13 comments)
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi/northbridge.asl:
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 39: 38:16 Shouldn't this be 38:15? At least that is what I see in KBL and CML EDS.
BTW, is this really correct for TGL? I see a different offset there. Can you please confirm?
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 43: PXSZ, 2, Is this correct for TGL?
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 45: 38:28 Actually, this is 38:26 on all. Sorry, I had read this incorrectly before.
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 223: 16 Can you please confirm if this is correct for TGL?
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 237: 28 Why does this not take PXSZ into consideration? Wouldn't the shift change because of that?
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 244: ShiftRight (0x10000000, _SB.PCI0.MCHC.PXSZ, Local0) Is this correct fot TGL?
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 268: 0x08000 Is this correct for TGL?
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/systemagent.h:
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 63: uintptr_t Shouldn't this be uint64_t then?
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 81: if (is64bit) { : base = pci_read_config32(SA_DEV_ROOT, index + 4); : base <<= 32; : } Why is this required? Check below only cares about whether the base is enabled which is bit 0.
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 93: pci_write_config32(SA_DEV_ROOT, index, base | 1); : if (is64bit) : pci_write_config32(SA_DEV_ROOT, index + 4, base >> 32); Do we really need the flag is64bit? Can this be done something like:
pci_write_config32(SA_DEV_ROOT, index, (base & 0xffffffff) | 1); if (base >> 32) pci_write_config32(SA_DEV_ROOT, index + 4, base >> 32);
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 115: write32((void *)(MCH_BASE_ADDRESS + index), base | 1); : if (fixed_set_resources[i].is_64_bit) : write32((void *)(MCH_BASE_ADDRESS + index + 4), base >> 32); Same comment as above.
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/skylake/acpi/... File src/soc/intel/skylake/acpi/systemagent.asl:
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/skylake/acpi/... PS5, Line 44: 38:28 38:26
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/skylake/acpi/... PS5, Line 247: 28 Same comment as the common northbridge.asl file
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 5:
(13 comments)
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi/northbridge.asl:
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 39: 38:16
Shouldn't this be 38:15? At least that is what I see in KBL and CML EDS. […]
send you register details
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 43: PXSZ, 2,
Is this correct for TGL?
send you register details
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 45: 38:28
Actually, this is 38:26 on all. Sorry, I had read this incorrectly before.
all PCIE BAR is 28:38 except APL/GLK
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 223: 16
Can you please confirm if this is correct for TGL?
this is correct as per my observation
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 237: 28
Why does this not take PXSZ into consideration? Wouldn't the shift change because of that?
i don't understand this one Furquan. PCIE BAR is Bit 28 to 38 hence shifting by 28.
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 244: ShiftRight (0x10000000, _SB.PCI0.MCHC.PXSZ, Local0)
Is this correct fot TGL?
this would be correct for TGL along as ONFIG_SA_PCIEX_LENGTH=0x10000000 in Kconfig
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 268: 0x08000
Is this correct for TGL?
this is mistake looks like from myside, this value should be 0x10000
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/systemagent.h:
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 63: uintptr_t
Shouldn't this be uint64_t then?
Done
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 81: if (is64bit) { : base = pci_read_config32(SA_DEV_ROOT, index + 4); : base <<= 32; : }
Why is this required? Check below only cares about whether the base is enabled which is bit 0.
got it
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 93: pci_write_config32(SA_DEV_ROOT, index, base | 1); : if (is64bit) : pci_write_config32(SA_DEV_ROOT, index + 4, base >> 32);
Do we really need the flag is64bit? Can this be done something like: […]
good idea
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 115: write32((void *)(MCH_BASE_ADDRESS + index), base | 1); : if (fixed_set_resources[i].is_64_bit) : write32((void *)(MCH_BASE_ADDRESS + index + 4), base >> 32);
Same comment as above.
Ack
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/skylake/acpi/... File src/soc/intel/skylake/acpi/systemagent.asl:
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/skylake/acpi/... PS5, Line 44: 38:28
38:26
its 28-38 as per register details
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/skylake/acpi/... PS5, Line 247: 28
Same comment as the common northbridge. […]
i have asked some question there
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Wonkyu Kim, Lance Zhao, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38387
to look at the new patch set (#6).
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS
This patch updates system agent related registers bit definitions as per EDS.
For an example: As per EDS MCHBAR register base is between bit 16-38 but coreboot programming was not align with EDS previously.
Also provide provision to program 64bit values as per SA EDS definitions
TEST=Dump MCHBAR in coreboot and ASL shows same 32 bit value.
Change-Id: I37340408fe89c94ce81953c751c8d7e22bc81a42 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/include/intelblocks/systemagent.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/skylake/acpi/systemagent.asl 4 files changed, 27 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/38387/6
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Wonkyu Kim, Lance Zhao, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38387
to look at the new patch set (#7).
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS
This patch updates system agent related registers bit definitions as per EDS.
For an example: As per EDS MCHBAR register base is between bit 16-38 but coreboot programming was not align with EDS previously.
Also provide provision to program 64bit values as per SA EDS definitions
TEST=Dump MCHBAR in coreboot and ASL shows same 32 bit value.
Change-Id: I37340408fe89c94ce81953c751c8d7e22bc81a42 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/include/intelblocks/systemagent.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/skylake/acpi/systemagent.asl 4 files changed, 26 insertions(+), 22 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/38387/7
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38387/7/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/38387/7/src/soc/intel/common/block/... PS7, Line 89: pci_write_config32(SA_DEV_ROOT, index + 4, base >> 32); Should be written before the enable bit above.
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Wonkyu Kim, Lance Zhao, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38387
to look at the new patch set (#8).
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS
This patch updates system agent related registers bit definitions as per EDS.
For an example: As per EDS MCHBAR register base is between bit 16-38 but coreboot programming was not align with EDS previously.
Also provide provision to program 64bit values as per SA EDS definitions
TEST=Dump MCHBAR in coreboot and ASL shows same 32 bit value.
Change-Id: I37340408fe89c94ce81953c751c8d7e22bc81a42 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/include/intelblocks/systemagent.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/skylake/acpi/systemagent.asl 4 files changed, 26 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/38387/8
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38387/7/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/38387/7/src/soc/intel/common/block/... PS7, Line 89: pci_write_config32(SA_DEV_ROOT, index + 4, base >> 32);
Should be written before the enable bit above.
valid point
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38387/7/src/soc/intel/common/block/... File src/soc/intel/common/block/systemagent/systemagent_early.c:
https://review.coreboot.org/c/coreboot/+/38387/7/src/soc/intel/common/block/... PS7, Line 89: pci_write_config32(SA_DEV_ROOT, index + 4, base >> 32);
valid point
Ack
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 8: Code-Review+1
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 8: Code-Review+1
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 8:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38387/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38387/8//COMMIT_MSG@13 PS8, Line 13: EDS Which one? All of them and this was wrong from the beginning?
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Wonkyu Kim, Lance Zhao, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38387
to look at the new patch set (#9).
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS
This patch updates system agent related registers bit definitions as per EDS.
For an example: As per CML/CNL/ICL EDS MCHBAR register base is between bit 16-38 but coreboot programming was not align with EDS previously.
CNL EDS doc number: 566216
Also provide provision to program 64bit values as per SA EDS definitions
TEST=Dump MCHBAR in coreboot and ASL shows same 32 bit value.
Change-Id: I37340408fe89c94ce81953c751c8d7e22bc81a42 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/include/intelblocks/systemagent.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/skylake/acpi/systemagent.asl 4 files changed, 26 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/38387/9
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38387/8//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38387/8//COMMIT_MSG@13 PS8, Line 13: EDS
Which one? All of them and this was wrong from the beginning?
i should mention the soc eds name
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Wonkyu Kim, Lance Zhao, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38387
to look at the new patch set (#10).
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS
This patch updates system agent related registers bit definitions as per EDS.
For an example: As per CML/CNL/ICL EDS MCHBAR register base is between bit 16-38 but coreboot programming was not align with EDS previously.
CNL EDS doc number: 566216
Also provide provision to program 64bit values as per SA EDS definitions
TEST=Dump MCHBAR in coreboot and ASL shows same 32 bit value.
Change-Id: I37340408fe89c94ce81953c751c8d7e22bc81a42 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/acpi/acpi/northbridge.asl M src/soc/intel/common/block/include/intelblocks/systemagent.h M src/soc/intel/common/block/systemagent/systemagent_early.c M src/soc/intel/skylake/acpi/systemagent.asl 4 files changed, 27 insertions(+), 24 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/38387/10
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/{apl,cnl,icl,skl,tgl}: Update SA bit fields as per EDS ......................................................................
Patch Set 10:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... File src/soc/intel/common/block/acpi/acpi/northbridge.asl:
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 39: 38:16
send you register details
Ack
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 43: PXSZ, 2,
send you register details
Ack
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 45: 38:28
all PCIE BAR is 28:38 except APL/GLK
Ack
https://review.coreboot.org/c/coreboot/+/38387/5/src/soc/intel/common/block/... PS5, Line 223: 16
this is correct as per my observation
Ack
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Wonkyu Kim, Lance Zhao, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38387
to look at the new patch set (#12).
Change subject: soc/intel/common: Update SA bit fields as per EDS ......................................................................
soc/intel/common: Update SA bit fields as per EDS
This patch updates system agent related registers bit definitions as per EDS.
For an example: As per CNL/ICL EDS MCHBAR register base is between bit 16-38 but coreboot programming was not align with EDS previously.
CNL EDS doc number: 566216
Also provide provision to program 64bit values as per SA EDS definitions
TEST=Dump MCHBAR in coreboot and ASL shows same 32 bit value.
Change-Id: I37340408fe89c94ce81953c751c8d7e22bc81a42 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/include/intelblocks/systemagent.h M src/soc/intel/common/block/systemagent/systemagent_early.c 2 files changed, 11 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/38387/12
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/common: Update SA bit fields as per EDS ......................................................................
Patch Set 13: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/38387/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38387/13//COMMIT_MSG@12 PS13, Line 12: For an example: For example:
https://review.coreboot.org/c/coreboot/+/38387/13//COMMIT_MSG@14 PS13, Line 14: align aligned
Hello Patrick Rudolph, Angel Pons, Arthur Heymans, Wonkyu Kim, Lance Zhao, build bot (Jenkins), Nico Huber, Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38387
to look at the new patch set (#14).
Change subject: soc/intel/common: Update SA bit fields as per EDS ......................................................................
soc/intel/common: Update SA bit fields as per EDS
This patch updates system agent related registers bit definitions as per EDS.
For example: As per CNL/ICL EDS MCHBAR register base is between bit 16-38 but coreboot programming was not aligned with EDS previously.
CNL EDS doc number: 566216
Also provide provision to program 64bit values as per SA EDS definitions
TEST=Dump MCHBAR in coreboot and ASL shows same 32 bit value.
Change-Id: I37340408fe89c94ce81953c751c8d7e22bc81a42 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/soc/intel/common/block/include/intelblocks/systemagent.h M src/soc/intel/common/block/systemagent/systemagent_early.c 2 files changed, 11 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/38387/14
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/common: Update SA bit fields as per EDS ......................................................................
Patch Set 14:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38387/13//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38387/13//COMMIT_MSG@12 PS13, Line 12: For an example:
For example:
Ack
https://review.coreboot.org/c/coreboot/+/38387/13//COMMIT_MSG@14 PS13, Line 14: align
aligned
Ack
Subrata Banik has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/common: Update SA bit fields as per EDS ......................................................................
soc/intel/common: Update SA bit fields as per EDS
This patch updates system agent related registers bit definitions as per EDS.
For example: As per CNL/ICL EDS MCHBAR register base is between bit 16-38 but coreboot programming was not aligned with EDS previously.
CNL EDS doc number: 566216
Also provide provision to program 64bit values as per SA EDS definitions
TEST=Dump MCHBAR in coreboot and ASL shows same 32 bit value.
Change-Id: I37340408fe89c94ce81953c751c8d7e22bc81a42 Signed-off-by: Subrata Banik subrata.banik@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38387 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/soc/intel/common/block/include/intelblocks/systemagent.h M src/soc/intel/common/block/systemagent/systemagent_early.c 2 files changed, 11 insertions(+), 8 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/soc/intel/common/block/include/intelblocks/systemagent.h b/src/soc/intel/common/block/include/intelblocks/systemagent.h index c605958..a11bf64 100644 --- a/src/soc/intel/common/block/include/intelblocks/systemagent.h +++ b/src/soc/intel/common/block/include/intelblocks/systemagent.h @@ -43,13 +43,13 @@ * Fixed MMIO range * INDEX = Either PCI configuration space registers or MMIO offsets * mapped from REG. - * BASE = 32 bit Address. + * BASE = 64 bit Address. * SIZE = base length * DESCRIPTION = Name of the register/offset. */ struct sa_mmio_descriptor { unsigned int index; - uintptr_t base; + uint64_t base; size_t size; const char *description; }; diff --git a/src/soc/intel/common/block/systemagent/systemagent_early.c b/src/soc/intel/common/block/systemagent/systemagent_early.c index d6f129d..1273c0f 100644 --- a/src/soc/intel/common/block/systemagent/systemagent_early.c +++ b/src/soc/intel/common/block/systemagent/systemagent_early.c @@ -1,7 +1,7 @@ /* * This file is part of the coreboot project. * - * Copyright (C) 2017 Intel Corporation. + * Copyright (C) 2017-2020 Intel Corporation. * * 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 @@ -71,7 +71,7 @@ int i;
for (i = 0; i < count; i++) { - uintptr_t base; + uint64_t base; unsigned int index;
index = fixed_set_resources[i].index; @@ -83,8 +83,9 @@ return;
base = fixed_set_resources[i].base; - - pci_write_config32(SA_DEV_ROOT, index, base | 1); + if (base >> 32) + pci_write_config32(SA_DEV_ROOT, index + 4, base >> 32); + pci_write_config32(SA_DEV_ROOT, index, (base & 0xffffffff) | 1); } }
@@ -99,12 +100,14 @@ int i;
for (i = 0; i < count; i++) { - uintptr_t base; + uint64_t base; unsigned int index;
base = fixed_set_resources[i].base; index = fixed_set_resources[i].index; - write32((void *)(MCH_BASE_ADDRESS + index), base | 1); + if (base >> 32) + write32((void *)(MCH_BASE_ADDRESS + index + 4), base >> 32); + write32((void *)(MCH_BASE_ADDRESS + index), (base & 0xffffffff) | 1); } }
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38387 )
Change subject: soc/intel/common: Update SA bit fields as per EDS ......................................................................
Patch Set 15:
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/228 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/227 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/226
Please note: This test is under development and might not be accurate at all!