Change in coreboot[master]: nb/intel/sandybridge: Do not hardcode resource indices

[Angel Pons (Code Review)]

Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41375 )

Change subject: nb/intel/sandybridge: Do not hardcode resource indices
......................................................................

nb/intel/sandybridge: Do not hardcode resource indices

Other northbridges use an index variable to assign monotonically
incrementing values to each resource. Do it here as well.

Change-Id: I8719a1a5973a10531cf11b3307652212cb3d4895
Signed-off-by: Angel Pons 
---
M src/northbridge/intel/sandybridge/northbridge.c
1 file changed, 6 insertions(+), 5 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/75/41375/1

diff --git a/src/northbridge/intel/sandybridge/northbridge.c b/src/northbridge/intel/sandybridge/northbridge.c
index c1cdea5..ce28c05 100644
--- a/src/northbridge/intel/sandybridge/northbridge.c
+++ b/src/northbridge/intel/sandybridge/northbridge.c
@@ -135,6 +135,7 @@
    uint32_t tseg_base, uma_size, tolud;
    uint16_t ggc;
    unsigned long long tomk;
+	unsigned long index = 3;

pci_dev_read_resources(dev);

@@ -238,9 +239,9 @@
    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,
-		    (tomk - (legacy_hole_base_k + legacy_hole_size_k)));
+	ram_resource(dev, index++, 0, legacy_hole_base_k);
+	ram_resource(dev, index++, legacy_hole_base_k + legacy_hole_size_k,
+			  (tomk - (legacy_hole_base_k + legacy_hole_size_k)));

/*
     * If >= 4GB installed, then memory from TOLUD to 4GB is remapped above TOM.
@@ -248,11 +249,11 @@
     */
    touud >>= 10; /* Convert to KB */
    if (touud > 4096 * 1024) {
-		ram_resource(dev, 5, 4096 * 1024, touud - (4096 * 1024));
+		ram_resource(dev, index++, 4096 * 1024, touud - (4096 * 1024));
    	printk(BIOS_INFO, "Available memory above 4GB: %lluM\n", (touud >> 10) - 4096);
    }

-	add_fixed_resources(dev, 6);
+	add_fixed_resources(dev, index++);
 }

static void northbridge_dmi_init(struct device *dev)

[Aaron Durbin (Code Review)]

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

Change subject: nb/intel/sandybridge: Do not hardcode resource indices
......................................................................

Patch Set 1: Code-Review+2

[Nico Huber (Code Review)]

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

Change subject: nb/intel/sandybridge: Do not hardcode resource indices
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41375/1/src/northbridge/intel/sandy... 
File src/northbridge/intel/sandybridge/northbridge.c:

https://review.coreboot.org/c/coreboot/+/41375/1/src/northbridge/intel/sandy... 
PS1, Line 138: 3
Why 3?

[Angel Pons (Code Review)]

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

Change subject: nb/intel/sandybridge: Do not hardcode resource indices
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41375/1/src/northbridge/intel/sandy... 
File src/northbridge/intel/sandybridge/northbridge.c:

https://review.coreboot.org/c/coreboot/+/41375/1/src/northbridge/intel/sandy... 
PS1, Line 138: 3
> Why 3?

Because I want to preserve the original behavior, and that's what the code starts off with.

Yes, I don't like it. But looks like other chipsets (x4x) do the same thing.

[Nico Huber (Code Review)]

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

Change subject: nb/intel/sandybridge: Do not hardcode resource indices
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41375/1/src/northbridge/intel/sandy... 
File src/northbridge/intel/sandybridge/northbridge.c:

https://review.coreboot.org/c/coreboot/+/41375/1/src/northbridge/intel/sandy... 
PS1, Line 138: 3
> Because I want to preserve the original behavior, and that's what the code starts off with. […]

I know you just preserve the behavior. I'm asking questions anyway :-P probably
should have done that on the previous commit which introduced the oddity instead
of giving it a premature +2. But it became more visible here... and I didn't
expect things to be merged out of order (started a discussion which devices
should reserve these resources on the first commit).

I guess it has to do with the resources added in pci_*_read_resources() that
is called first. None of them seem to add exactly the resources 0..2, though.
Maybe we should have a look at some very old log, it might give us a clue what
the first 3 resources were.

[Angel Pons (Code Review)]

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

Change subject: nb/intel/sandybridge: Do not hardcode resource indices
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41375/1/src/northbridge/intel/sandy... 
File src/northbridge/intel/sandybridge/northbridge.c:

https://review.coreboot.org/c/coreboot/+/41375/1/src/northbridge/intel/sandy... 
PS1, Line 138: 3
> I know you just preserve the behavior. I'm asking questions anyway :-P probably […]

I checked, and this had been hardcoded since forever (ever since sandybridge was added)

I don't know how to handle that without doing some major refactoring, though. If you want, I can add FIXMEs to all hardcoded indices

[Nico Huber (Code Review)]

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

Change subject: nb/intel/sandybridge: Do not hardcode resource indices
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/41375/1/src/northbridge/intel/sandy... 
File src/northbridge/intel/sandybridge/northbridge.c:

https://review.coreboot.org/c/coreboot/+/41375/1/src/northbridge/intel/sandy... 
PS1, Line 138: 3
> I checked, and this had been hardcoded since forever (ever since sandybridge was added) […]

Hmmm, maybe just keep it in mind. Some infrastructure to allocate the
index for random resources would be nice. I've looked at many northbridges,
and all I can tell is that some hardcoded 10, some hardcoded 3.

For not so random resources, the index is determined by the BAR reg's offset.
So we have an upper limit for PCI, where the registers start at 16. But I
can't find no clue why we don't start at 0.

[Angel Pons (Code Review)]

Angel Pons has removed a vote from this change. ( https://review.coreboot.org/c/coreboot/+/41375 )

Change subject: nb/intel/sandybridge: Do not hardcode resource indices
......................................................................

Removed Verified+1 by build bot (Jenkins)

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41375 )

Change subject: nb/intel/sandybridge: Do not hardcode resource indices
......................................................................

nb/intel/sandybridge: Do not hardcode resource indices

Other northbridges use an index variable to assign monotonically
incrementing values to each resource. Do it here as well.

Change-Id: I8719a1a5973a10531cf11b3307652212cb3d4895
Signed-off-by: Angel Pons 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/41375
Tested-by: build bot (Jenkins) 
Reviewed-by: Aaron Durbin 
---
M src/northbridge/intel/sandybridge/northbridge.c
1 file changed, 6 insertions(+), 5 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Aaron Durbin: Looks good to me, approved

diff --git a/src/northbridge/intel/sandybridge/northbridge.c b/src/northbridge/intel/sandybridge/northbridge.c
index c1cdea5..ce28c05 100644
--- a/src/northbridge/intel/sandybridge/northbridge.c
+++ b/src/northbridge/intel/sandybridge/northbridge.c
@@ -135,6 +135,7 @@
    uint32_t tseg_base, uma_size, tolud;
    uint16_t ggc;
    unsigned long long tomk;
+	unsigned long index = 3;

pci_dev_read_resources(dev);

@@ -238,9 +239,9 @@
    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,
-		    (tomk - (legacy_hole_base_k + legacy_hole_size_k)));
+	ram_resource(dev, index++, 0, legacy_hole_base_k);
+	ram_resource(dev, index++, legacy_hole_base_k + legacy_hole_size_k,
+			  (tomk - (legacy_hole_base_k + legacy_hole_size_k)));

/*
     * If >= 4GB installed, then memory from TOLUD to 4GB is remapped above TOM.
@@ -248,11 +249,11 @@
     */
    touud >>= 10; /* Convert to KB */
    if (touud > 4096 * 1024) {
-		ram_resource(dev, 5, 4096 * 1024, touud - (4096 * 1024));
+		ram_resource(dev, index++, 4096 * 1024, touud - (4096 * 1024));
    	printk(BIOS_INFO, "Available memory above 4GB: %lluM\n", (touud >> 10) - 4096);
    }

-	add_fixed_resources(dev, 6);
+	add_fixed_resources(dev, index++);
 }

static void northbridge_dmi_init(struct device *dev)

[9elements QA (Code Review)]

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

Change subject: nb/intel/sandybridge: Do not hardcode resource indices
......................................................................

Patch Set 3:

Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4
Emulation targets:
"QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/3552
"QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3551
"QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/3550
"QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/3549

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