Change in coreboot[master]: sandybridge: fix when resources are added

[Aaron Durbin (Code Review)]

Aaron Durbin has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41364 )

Change subject: sandybridge: fix when resources are added
......................................................................

sandybridge: fix when resources are added

Change-Id: I8a7081020be43da055b7de5a56dd97a7b5a9f09c
Signed-off-by: Aaron Durbin 
---
M src/northbridge/intel/sandybridge/northbridge.c
1 file changed, 8 insertions(+), 10 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/41364/1

diff --git a/src/northbridge/intel/sandybridge/northbridge.c b/src/northbridge/intel/sandybridge/northbridge.c
index 7a6372c..7057151 100644
--- a/src/northbridge/intel/sandybridge/northbridge.c
+++ b/src/northbridge/intel/sandybridge/northbridge.c
@@ -99,7 +99,7 @@
    }
 }

-static void pci_domain_set_resources_sandybridge(struct device *dev)
+static void mc_read_resources_full(struct device *mch)
 {
    uint64_t tom, me_base, touud;
    uint32_t tseg_base, uma_size, tolud;
@@ -132,8 +132,6 @@
     * 14fe00000   5368MB TOUUD
     */

-	struct device *mch = pcidev_on_root(0, 0);
-
    /* Top of Upper Usable DRAM, including remap */
    touud  = pci_read_config32(mch, TOUUD + 4);
    touud <<= 32;
@@ -202,8 +200,8 @@
    printk(BIOS_INFO, "Available memory below 4GB: %lluM\n", tomk >> 10);

/* Report the memory regions */
-	ram_resource(dev, 3, 0, legacy_hole_base_k);
-	ram_resource(dev, 4, legacy_hole_base_k + legacy_hole_size_k,
+	ram_resource(mch, 3, 0, legacy_hole_base_k);
+	ram_resource(mch, 4, legacy_hole_base_k + legacy_hole_size_k,
    	    (tomk - (legacy_hole_base_k + legacy_hole_size_k)));

/*
@@ -212,13 +210,11 @@
     */
    touud >>= 10; /* Convert to KB */
    if (touud > 4096 * 1024) {
-		ram_resource(dev, 5, 4096 * 1024, touud - (4096 * 1024));
+		ram_resource(mch, 5, 4096 * 1024, touud - (4096 * 1024));
    	printk(BIOS_INFO, "Available memory above 4GB: %lluM\n", (touud >> 10) - 4096);
    }

-	add_fixed_resources(dev, 6);
-
-	assign_resources(dev->link_list);
+	add_fixed_resources(mch, 6);
 }

static const char *northbridge_acpi_name(const struct device *dev)
@@ -243,7 +239,7 @@
  */
 static struct device_operations pci_domain_ops = {
    .read_resources    = pci_domain_read_resources,
-	.set_resources     = pci_domain_set_resources_sandybridge,
+	.set_resources     = pci_domain_set_resources,
    .scan_bus          = pci_domain_scan_bus,
    .write_acpi_tables = northbridge_write_acpi_tables,
    .acpi_name         = northbridge_acpi_name,
@@ -261,6 +257,8 @@
    	struct resource *resource = new_resource(dev, PCIEXBAR);
    	mmconf_resource_init(resource, pcie_config_base, buses);
    }
+
+	mc_read_resources_full(dev);
 }

static void northbridge_dmi_init(struct device *dev)

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41364 )

Change subject: sandybridge: fix when resources are added
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

Working on Asus P8Z77-V LX2

https://review.coreboot.org/c/coreboot/+/41364/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/41364/1//COMMIT_MSG@7 
PS1, Line 7: sandybridge: fix when resources are added
Mind adding the reasoning behind this change please?

Change in coreboot[master]: nb/intel/sandybridge: add resources during read_resources()

[Aaron Durbin (Code Review)]

Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41364

to look at the new patch set (#2).

Change subject: nb/intel/sandybridge: add resources during read_resources()
......................................................................

nb/intel/sandybridge: add resources during read_resources()

The chipset code was incorrectly adding memory resources
to the domain device after resource allocation occurred.
It's not possible to get the correct view of the address space,
and it's generally incorrect to not add resources during
read_resources(). Fix the order by hanging the resources off
of the host bridge device.

Change-Id: I8a7081020be43da055b7de5a56dd97a7b5a9f09c
Signed-off-by: Aaron Durbin 
---
M src/northbridge/intel/sandybridge/northbridge.c
1 file changed, 43 insertions(+), 51 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/41364/2

Change in coreboot[master]: nb/intel/sandybridge: add resources during read_resources()

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41364 )

Change subject: nb/intel/sandybridge: add resources during read_resources()
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/41364/2/src/northbridge/intel/sandy... 
File src/northbridge/intel/sandybridge/northbridge.c:

https://review.coreboot.org/c/coreboot/+/41364/2/src/northbridge/intel/sandy... 
PS2, Line 130: mch
Don't rename this, I'd rather use "dev" everywhere instead

https://review.coreboot.org/c/coreboot/+/41364/2/src/northbridge/intel/sandy... 
PS2, Line 139: 	pci_dev_read_resources(dev);
See, "dev" is what we usually use 😉

Change in coreboot[master]: nb/intel/sandybridge: add resources during read_resources()

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41364 )

Change subject: nb/intel/sandybridge: add resources during read_resources()
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41364/1//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/41364/1//COMMIT_MSG@7 
PS1, Line 7: sandybridge: fix when resources are added
> Mind adding the reasoning behind this change please?

Done

Change in coreboot[master]: nb/intel/sandybridge: add resources during read_resources()

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41364 )

Change subject: nb/intel/sandybridge: add resources during read_resources()
......................................................................

Patch Set 2: Code-Review+1

Build problem aside, it works

Change in coreboot[master]: nb/intel/sandybridge: add resources during read_resources()

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41364 )

Change subject: nb/intel/sandybridge: add resources during read_resources()
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41364/2/src/northbridge/intel/sandy... 
File src/northbridge/intel/sandybridge/northbridge.c:

https://review.coreboot.org/c/coreboot/+/41364/2/src/northbridge/intel/sandy... 
PS2, Line 139: 	pci_dev_read_resources(dev);
> See, "dev" is what we usually use 😉

ya. I can just make that dev.

Change in coreboot[master]: nb/intel/sandybridge: add resources during read_resources()

[Aaron Durbin (Code Review)]

Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/41364

to look at the new patch set (#3).

Change subject: nb/intel/sandybridge: add resources during read_resources()
......................................................................

nb/intel/sandybridge: add resources during read_resources()

The chipset code was incorrectly adding memory resources
to the domain device after resource allocation occurred.
It's not possible to get the correct view of the address space,
and it's generally incorrect to not add resources during
read_resources(). Fix the order by hanging the resources off
of the host bridge device.

Change-Id: I8a7081020be43da055b7de5a56dd97a7b5a9f09c
Signed-off-by: Aaron Durbin 
---
M src/northbridge/intel/sandybridge/northbridge.c
1 file changed, 48 insertions(+), 56 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/64/41364/3

Change in coreboot[master]: nb/intel/sandybridge: add resources during read_resources()

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41364 )

Change subject: nb/intel/sandybridge: add resources during read_resources()
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41364/2/src/northbridge/intel/sandy... 
File src/northbridge/intel/sandybridge/northbridge.c:

https://review.coreboot.org/c/coreboot/+/41364/2/src/northbridge/intel/sandy... 
PS2, Line 139: 	pci_dev_read_resources(dev);
> ya. I can just make that dev.

Done

Change in coreboot[master]: nb/intel/sandybridge: add resources during read_resources()

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41364 )

Change subject: nb/intel/sandybridge: add resources during read_resources()
......................................................................

Patch Set 3: Code-Review+1

Change in coreboot[master]: nb/intel/sandybridge: add resources during read_resources()

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41364 )

Change subject: nb/intel/sandybridge: add resources during read_resources()
......................................................................

Patch Set 3:

e7505 and i440bx are broken in the same way as sandybridge. I don't have patches for those currently, but they should be easy to fix following the same pattern.

Change in coreboot[master]: nb/intel/sandybridge: add resources during read_resources()

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41364 )

Change subject: nb/intel/sandybridge: add resources during read_resources()
......................................................................

Patch Set 3:
> Patch Set 3:
>
> e7505 and i440bx are broken in the same way as sandybridge. I don't have patches for those currently, but they should be easy to fix following the same pattern.

I have identified places where there is more brokenness. I will push some patches on top of yours.

Change in coreboot[master]: nb/intel/sandybridge: add resources during read_resources()

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41364 )

Change subject: nb/intel/sandybridge: add resources during read_resources()
......................................................................

Patch Set 3: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/41364/2/src/northbridge/intel/sandy... 
File src/northbridge/intel/sandybridge/northbridge.c:

https://review.coreboot.org/c/coreboot/+/41364/2/src/northbridge/intel/sandy... 
PS2, Line 130: mch
> Don't rename this, I'd rather use "dev" everywhere instead

Done

Change in coreboot[master]: nb/intel/sandybridge: add resources during read_resources()

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41364 )

Change subject: nb/intel/sandybridge: add resources during read_resources()
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: nb/intel/sandybridge: add resources during read_resources()

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41364 )

Change subject: nb/intel/sandybridge: add resources during read_resources()
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: nb/intel/sandybridge: add resources during read_resources()

[Aaron Durbin (Code Review)]

Aaron Durbin has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41364 )

Change subject: nb/intel/sandybridge: add resources during read_resources()
......................................................................

nb/intel/sandybridge: add resources during read_resources()

The chipset code was incorrectly adding memory resources
to the domain device after resource allocation occurred.
It's not possible to get the correct view of the address space,
and it's generally incorrect to not add resources during
read_resources(). Fix the order by hanging the resources off
of the host bridge device.

Change-Id: I8a7081020be43da055b7de5a56dd97a7b5a9f09c
Signed-off-by: Aaron Durbin 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/41364
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Rudolph 
Reviewed-by: Angel Pons 
Reviewed-by: Furquan Shaikh 
Reviewed-by: Nico Huber 
---
M src/northbridge/intel/sandybridge/northbridge.c
1 file changed, 48 insertions(+), 56 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, approved
  Furquan Shaikh: Looks good to me, approved
  Patrick Rudolph: Looks good to me, but someone else must approve
  Angel Pons: Looks good to me, but someone else must approve

diff --git a/src/northbridge/intel/sandybridge/northbridge.c b/src/northbridge/intel/sandybridge/northbridge.c
index 7a6372c..c1cdea5 100644
--- a/src/northbridge/intel/sandybridge/northbridge.c
+++ b/src/northbridge/intel/sandybridge/northbridge.c
@@ -71,6 +71,34 @@
    return 0;
 }

+static const char *northbridge_acpi_name(const struct device *dev)
+{
+	if (dev->path.type == DEVICE_PATH_DOMAIN)
+		return "PCI0";
+
+	if (dev->path.type != DEVICE_PATH_PCI)
+		return NULL;
+
+	switch (dev->path.pci.devfn) {
+	case PCI_DEVFN(0, 0):
+		return "MCHC";
+	}
+
+	return NULL;
+}
+
+/*
+ * TODO We could determine how many PCIe busses we need in the bar.
+ * For now, that number is hardcoded to a max of 64.
+ */
+static struct device_operations pci_domain_ops = {
+	.read_resources    = pci_domain_read_resources,
+	.set_resources     = pci_domain_set_resources,
+	.scan_bus          = pci_domain_scan_bus,
+	.write_acpi_tables = northbridge_write_acpi_tables,
+	.acpi_name         = northbridge_acpi_name,
+};
+
 static void add_fixed_resources(struct device *dev, int index)
 {
    mmio_resource(dev, index++, uma_memory_base >> 10, uma_memory_size >> 10);
@@ -99,13 +127,23 @@
    }
 }

-static void pci_domain_set_resources_sandybridge(struct device *dev)
+static void mc_read_resources(struct device *dev)
 {
+	u32 pcie_config_base;
+	int buses;
    uint64_t tom, me_base, touud;
    uint32_t tseg_base, uma_size, tolud;
    uint16_t ggc;
    unsigned long long tomk;

+	pci_dev_read_resources(dev);
+
+	buses = get_pcie_bar(&pcie_config_base);
+	if (buses) {
+		struct resource *resource = new_resource(dev, PCIEXBAR);
+		mmconf_resource_init(resource, pcie_config_base, buses);
+	}
+
    /* Total Memory 2GB example:
     *
     *  00000000  0000MB-1992MB  1992MB  RAM     (writeback)
@@ -132,28 +170,26 @@
     * 14fe00000   5368MB TOUUD
     */

-	struct device *mch = pcidev_on_root(0, 0);
-
    /* Top of Upper Usable DRAM, including remap */
-	touud  = pci_read_config32(mch, TOUUD + 4);
+	touud  = pci_read_config32(dev, TOUUD + 4);
    touud <<= 32;
-	touud |= pci_read_config32(mch, TOUUD);
+	touud |= pci_read_config32(dev, TOUUD);

/* Top of Lower Usable DRAM */
-	tolud = pci_read_config32(mch, TOLUD);
+	tolud = pci_read_config32(dev, TOLUD);

/* Top of Memory - does not account for any UMA */
-	tom  = pci_read_config32(mch, TOM + 4);
+	tom  = pci_read_config32(dev, TOM + 4);
    tom <<= 32;
-	tom |= pci_read_config32(mch, TOM);
+	tom |= pci_read_config32(dev, TOM);

printk(BIOS_DEBUG, "TOUUD 0x%llx TOLUD 0x%08x TOM 0x%llx\n",
           touud, tolud, tom);

/* ME UMA needs excluding if total memory < 4GB */
-	me_base  = pci_read_config32(mch, MESEG_BASE + 4);
+	me_base  = pci_read_config32(dev, MESEG_BASE + 4);
    me_base <<= 32;
-	me_base |= pci_read_config32(mch, MESEG_BASE);
+	me_base |= pci_read_config32(dev, MESEG_BASE);

printk(BIOS_DEBUG, "MEBASE 0x%llx\n", me_base);

@@ -172,7 +208,7 @@
    }

/* Graphics memory comes next */
-	ggc = pci_read_config16(mch, GGC);
+	ggc = pci_read_config16(dev, GGC);
    if (!(ggc & 2)) {
    	printk(BIOS_DEBUG, "IGD decoded, subtracting ");

@@ -192,7 +228,7 @@
    }

/* Calculate TSEG size from its base which must be below GTT */
-	tseg_base = pci_read_config32(mch, TSEGMB);
+	tseg_base = pci_read_config32(dev, TSEGMB);
    uma_size = (uma_memory_base - tseg_base) >> 10;
    tomk -= uma_size;
    uma_memory_base = tomk * 1024ULL;
@@ -217,50 +253,6 @@
    }

add_fixed_resources(dev, 6);
-
-	assign_resources(dev->link_list);
-}
-
-static const char *northbridge_acpi_name(const struct device *dev)
-{
-	if (dev->path.type == DEVICE_PATH_DOMAIN)
-		return "PCI0";
-
-	if (dev->path.type != DEVICE_PATH_PCI)
-		return NULL;
-
-	switch (dev->path.pci.devfn) {
-	case PCI_DEVFN(0, 0):
-		return "MCHC";
-	}
-
-	return NULL;
-}
-
-/*
- * TODO We could determine how many PCIe busses we need in the bar.
- * For now, that number is hardcoded to a max of 64.
- */
-static struct device_operations pci_domain_ops = {
-	.read_resources    = pci_domain_read_resources,
-	.set_resources     = pci_domain_set_resources_sandybridge,
-	.scan_bus          = pci_domain_scan_bus,
-	.write_acpi_tables = northbridge_write_acpi_tables,
-	.acpi_name         = northbridge_acpi_name,
-};
-
-static void mc_read_resources(struct device *dev)
-{
-	u32 pcie_config_base;
-	int buses;
-
-	pci_dev_read_resources(dev);
-
-	buses = get_pcie_bar(&pcie_config_base);
-	if (buses) {
-		struct resource *resource = new_resource(dev, PCIEXBAR);
-		mmconf_resource_init(resource, pcie_config_base, buses);
-	}
 }

static void northbridge_dmi_init(struct device *dev)

Change in coreboot[master]: nb/intel/sandybridge: add resources during read_resources()

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41364 )

Change subject: nb/intel/sandybridge: add resources during read_resources()
......................................................................

Patch Set 4:

Automatic boot test returned (PASS/FAIL/TOTAL): 2/2/4
Emulation targets:
"QEMU x86 q35/ich9" using payload TianoCore : FAIL : https://lava.9esec.io/r/3419
"QEMU x86 q35/ich9" using payload SeaBIOS : FAIL : https://lava.9esec.io/r/3418
"QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3417
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/3416

Please note: This test is under development and might not be accurate at all!