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

[Furquan Shaikh (Code Review)]

Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41368 )

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

nb/intel/i440bx: 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(). This change fixes the order by adding resources in
read_resources().

Signed-off-by: Furquan Shaikh 
Change-Id: I84c1ba8645b548248a8bb8bf5bc4953d3be12475
---
M src/northbridge/intel/i440bx/northbridge.c
1 file changed, 3 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/41368/1

diff --git a/src/northbridge/intel/i440bx/northbridge.c b/src/northbridge/intel/i440bx/northbridge.c
index aefd026..cc1f7ed 100644
--- a/src/northbridge/intel/i440bx/northbridge.c
+++ b/src/northbridge/intel/i440bx/northbridge.c
@@ -27,7 +27,7 @@
    .device = 0x7190,
 };

-static void i440bx_domain_set_resources(struct device *dev)
+static void i440bx_domain_read_resources(struct device *dev)
 {
    struct device *mc_dev;
    uint32_t pci_tolm;
@@ -62,12 +62,11 @@
    	ram_resource(dev, idx++, 0, 640);
    	ram_resource(dev, idx++, 768, tolmk - 768);
    }
-	assign_resources(dev->link_list);
 }

static struct device_operations pci_domain_ops = {
-	.read_resources		= pci_domain_read_resources,
-	.set_resources		= i440bx_domain_set_resources,
+	.read_resources		= i440bx_domain_read_resources,
+	.set_resources		= pci_domain_set_resources,
    .scan_bus		= pci_domain_scan_bus,
 };

[Furquan Shaikh (Code Review)]

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

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

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41368/1/src/northbridge/intel/i440b... 
File src/northbridge/intel/i440bx/northbridge.c:

https://review.coreboot.org/c/coreboot/+/41368/1/src/northbridge/intel/i440b... 
PS1, Line 34: 
pci_domain_read_resources() is missing here.

[Furquan Shaikh (Code Review)]

Hello build bot (Jenkins), Aaron Durbin, Patrick Rudolph,

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

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

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

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

nb/intel/i440bx: 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(). This change fixes the order by adding resources in
read_resources().

Signed-off-by: Furquan Shaikh 
Change-Id: I84c1ba8645b548248a8bb8bf5bc4953d3be12475
---
M src/northbridge/intel/i440bx/northbridge.c
1 file changed, 5 insertions(+), 4 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/68/41368/2

[Furquan Shaikh (Code Review)]

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

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

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41368/1/src/northbridge/intel/i440b... 
File src/northbridge/intel/i440bx/northbridge.c:

https://review.coreboot.org/c/coreboot/+/41368/1/src/northbridge/intel/i440b... 
PS1, Line 34:
> pci_domain_read_resources() is missing here.

Done

[Aaron Durbin (Code Review)]

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

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

Patch Set 2: Code-Review+2

[Angel Pons (Code Review)]

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

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

Patch Set 2: Code-Review+1

Hi Keith, could you please boot-test this change? Thanks in advance.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41368 )

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

Patch Set 2:
> Patch Set 2: Code-Review+1
>
> Hi Keith, could you please boot-test this change? Thanks in advance.

He reported on the list, that SeaBIOS doesn’t hang anymore, but the storage devices are not found by SeaBIOS now.

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41368 )

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

Patch Set 2: Code-Review+1

SeaBIOS didn’t display any graphics anymore. This change-set fixes this.

[Aaron Durbin (Code Review)]

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

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

Patch Set 2: -Code-Review

(1 comment)

https://review.coreboot.org/c/coreboot/+/41368/2/src/northbridge/intel/i440b... 
File src/northbridge/intel/i440bx/northbridge.c:

https://review.coreboot.org/c/coreboot/+/41368/2/src/northbridge/intel/i440b... 
PS2, Line 37: 	pci_tolm = find_pci_tolm(dev->link_list);
This is incorrect. It's implicitly deriving the existence of ram using find_pci_tolm() walking the resource list that isn't fully seeded at read_resource() time. The check below of tolmk doesn't make sense. I'm not sure why find_pci_tolm() exists. i945 seems wrong in this regard as well, but i945 is just reporting the value. Here the code is trying to use the value to implicit measure ram capacity.

[Aaron Durbin (Code Review)]

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

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

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41368/2/src/northbridge/intel/i440b... 
File src/northbridge/intel/i440bx/northbridge.c:

https://review.coreboot.org/c/coreboot/+/41368/2/src/northbridge/intel/i440b... 
PS2, Line 37: 	pci_tolm = find_pci_tolm(dev->link_list);
> This is incorrect. […]

Looks like this chipset has a max of 512MiB of ram (or 1GiB with registered dimms). And of reading the DRB register description I don't see why DRB7 wouldn't be the  SDRAM size in the system.

[Keith Hui (Code Review)]

Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41368 )

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

Patch Set 2:

Here are the i440BX limits:

Max 1GB RAM
Max 4 DIMM slots
4 DIMM slots only supported using registered DIMMs
Max 256MB per DIMM (and of the x8 type only; the kind that has 256MB with chips on one side only are not supported, although our ram init can use them partially.)

So, without registered DIMMs, the most memory "i can haz" is 768MB, which is what I'm testing with.

[Aaron Durbin (Code Review)]

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

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

Patch Set 2:
> Patch Set 2:
>
> Here are the i440BX limits:
>
> Max 1GB RAM
> Max 4 DIMM slots
> 4 DIMM slots only supported using registered DIMMs
> Max 256MB per DIMM (and of the x8 type only; the kind that has 256MB with chips on one side only are not supported, although our ram init can use them partially.)
>
> So, without registered DIMMs, the most memory "i can haz" is 768MB, which is what I'm testing with.

Thanks, Keith, for the info. find_pci_tolm() is still wrong based on my understanding of the chipset in that the register will reflect the correct answer since DRB7 << 23 can maximally be 1GiB.

[Keith Hui (Code Review)]

Keith Hui has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41368 )

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

Patch Set 2: Code-Review+1

This patch plus CB:41363 fixed booting on ASUS P3B-F.

[Mike Banon (Code Review)]

Mike Banon has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41368 )

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

Patch Set 2:

Perhaps AMD AGESA would also need something like this

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41368 )

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

Patch Set 2:
> Patch Set 2:
>
> Perhaps AMD AGESA would also need something like this

Thank you for testing, but this is the wrong change-set to discuss AMD AGESA boards. The list is the correct forum. Please apply all related change-sets (listed on the right side, you might pick the top one and download (d) it using the checkout option).

[Aaron Durbin (Code Review)]

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

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

Patch Set 2: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/41368/2/src/northbridge/intel/i440b... 
File src/northbridge/intel/i440bx/northbridge.c:

https://review.coreboot.org/c/coreboot/+/41368/2/src/northbridge/intel/i440b... 
PS2, Line 37: 	pci_tolm = find_pci_tolm(dev->link_list);
> Looks like this chipset has a max of 512MiB of ram (or 1GiB with registered dimms). […]

I think this is wrong, but in practice from a device configurations that exist we can land this change then remove find_pci_tolm() from this function as it doesn't serve a proper purpose.

There is no resource when there is an overlap anyway -- though that would be a major bug in an of itself. I'll send up an CL to remove this as a follow up.

[Aaron Durbin (Code Review)]

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

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

nb/intel/i440bx: 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(). This change fixes the order by adding resources in
read_resources().

Signed-off-by: Furquan Shaikh 
Change-Id: I84c1ba8645b548248a8bb8bf5bc4953d3be12475
Reviewed-on: https://review.coreboot.org/c/coreboot/+/41368
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
Reviewed-by: Paul Menzel 
Reviewed-by: Keith Hui 
Reviewed-by: Aaron Durbin 
---
M src/northbridge/intel/i440bx/northbridge.c
1 file changed, 5 insertions(+), 4 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Paul Menzel: Looks good to me, but someone else must approve
  Aaron Durbin: Looks good to me, approved
  Keith Hui: 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/i440bx/northbridge.c b/src/northbridge/intel/i440bx/northbridge.c
index aefd026..2659288 100644
--- a/src/northbridge/intel/i440bx/northbridge.c
+++ b/src/northbridge/intel/i440bx/northbridge.c
@@ -27,11 +27,13 @@
    .device = 0x7190,
 };

-static void i440bx_domain_set_resources(struct device *dev)
+static void i440bx_domain_read_resources(struct device *dev)
 {
    struct device *mc_dev;
    uint32_t pci_tolm;

+	pci_domain_read_resources(dev);
+
    pci_tolm = find_pci_tolm(dev->link_list);
    mc_dev = dev->link_list->children;
    if (mc_dev) {
@@ -62,12 +64,11 @@
    	ram_resource(dev, idx++, 0, 640);
    	ram_resource(dev, idx++, 768, tolmk - 768);
    }
-	assign_resources(dev->link_list);
 }

static struct device_operations pci_domain_ops = {
-	.read_resources		= pci_domain_read_resources,
-	.set_resources		= i440bx_domain_set_resources,
+	.read_resources		= i440bx_domain_read_resources,
+	.set_resources		= pci_domain_set_resources,
    .scan_bus		= pci_domain_scan_bus,
 };

[9elements QA (Code Review)]

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

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

Patch Set 3:

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/3427
"QEMU x86 q35/ich9" using payload SeaBIOS : FAIL : https://lava.9esec.io/r/3426
"QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3425
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/3424

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