Change in coreboot[master]: Device enumeration based on IORESOURCE_ASSIGNED

[Jonathan Zhang (Code Review)]

Jonathan Zhang has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38547 )

Change subject: Device enumeration based on IORESOURCE_ASSIGNED
......................................................................

Device enumeration based on IORESOURCE_ASSIGNED

Intel Xeon Scalable Processors has Split IIO architecture, and also
in order to support multiple sockets, it is necessary to run device
enumeration in consideration of IORESOURCE_ASSIGNED status.

Signed-off-by: Jonathan Zhang 
Signed-off-by: Reddy Chagam 
Change-Id: I69e100ea658fefec905321197b5bd6da1e0e647a
---
M src/device/device.c
1 file changed, 18 insertions(+), 10 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/38547/1

diff --git a/src/device/device.c b/src/device/device.c
index 236b768..879382c 100644
--- a/src/device/device.c
+++ b/src/device/device.c
@@ -380,6 +380,18 @@
    	if (!resource->size)
    		continue;

+		/* Skip if IO resource was assigned */
+		if (resource->flags & IORESOURCE_ASSIGNED) {
+			/* Warn if it looks like APICs aren't declared. */
+			if(resource->limit == 0xffffffff) {
+				printk(BIOS_ERR,
+					"Resource limit looks wrong! (no APIC?)\n");
+				printk(BIOS_ERR, "%s %02lx limit %08llx\n",
+					dev_path(dev), resource->index, resource->limit);
+			}
+			continue;
+		}
+
    	/* Propagate the resource alignment to the bridge resource. */
    	if (resource->align > bridge->align)
    		bridge->align = resource->align;
@@ -388,15 +400,6 @@
    	if (bridge->limit > resource->limit)
    		bridge->limit = resource->limit;

-		/* Warn if it looks like APICs aren't declared. */
-		if ((resource->limit == 0xffffffff) &&
-		    (resource->flags & IORESOURCE_ASSIGNED)) {
-			printk(BIOS_ERR,
-			       "Resource limit looks wrong! (no APIC?)\n");
-			printk(BIOS_ERR, "%s %02lx limit %08llx\n",
-			       dev_path(dev), resource->index, resource->limit);
-		}
-
    	if (resource->flags & IORESOURCE_IO) {
    		/*
    		 * Don't allow potential aliases over the legacy PCI
@@ -434,7 +437,7 @@
     * address positively decoded by the bridge.
     */
    bridge->size = round(base, bridge->gran) -
-		       round(bridge->base, bridge->align);
+		round(bridge->base, bridge->align);

printk(BIOS_SPEW, "%s %s: base: %llx size: %llx align: %d gran: %d"
           " limit: %llx done\n", dev_path(bus->dev),
@@ -717,6 +720,9 @@
    	if ((res->flags & IORESOURCE_FIXED))
    		continue;

+		if ((res->flags & IORESOURCE_ASSIGNED))
+			continue;
+
    	lim = resource_limit(&limits, res);
    	if (!lim)
    		continue;
@@ -1065,6 +1071,8 @@
    	for (res = child->resource_list; res; res = res->next) {
    		if (res->flags & IORESOURCE_FIXED)
    			continue;
+			if (res->flags & IORESOURCE_ASSIGNED)
+				continue;
    		if (res->flags & IORESOURCE_MEM) {
    			allocate_resources(child->link_list,
    					   res, IORESOURCE_TYPE_MASK, IORESOURCE_MEM);

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38547 )

Change subject: Device enumeration based on IORESOURCE_ASSIGNED
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38547/1/src/device/device.c 
File src/device/device.c:

https://review.coreboot.org/c/coreboot/+/38547/1/src/device/device.c@386 
PS1, Line 386: 			if(resource->limit == 0xffffffff) {
space required before the open parenthesis '('

[Jonathan Zhang (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: Device enumeration based on IORESOURCE_ASSIGNED
......................................................................

Device enumeration based on IORESOURCE_ASSIGNED

Intel Xeon Scalable Processors has Split IIO architecture, and also
in order to support multiple sockets, it is necessary to run device
enumeration in consideration of IORESOURCE_ASSIGNED status.

Signed-off-by: Jonathan Zhang 
Signed-off-by: Reddy Chagam 
Change-Id: I69e100ea658fefec905321197b5bd6da1e0e647a
---
M src/device/device.c
1 file changed, 18 insertions(+), 10 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/38547/2

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38547 )

Change subject: Device enumeration based on IORESOURCE_ASSIGNED
......................................................................

Patch Set 1:

(1 comment)
> Patch Set 1:
>
> (1 comment)

https://review.coreboot.org/c/coreboot/+/38547/1/src/device/device.c 
File src/device/device.c:

https://review.coreboot.org/c/coreboot/+/38547/1/src/device/device.c@386 
PS1, Line 386: 			if(resource->limit == 0xffffffff) {
> space required before the open parenthesis '('

Done

[Angel Pons (Code Review)]

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

Change subject: Device enumeration based on IORESOURCE_ASSIGNED
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38547/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38547/2//COMMIT_MSG@7 
PS2, Line 7: Device enumeration based on IORESOURCE_ASSIGNED
This summary does not have a present-tense verb. What does this commit do with the device enumeration?

[Paul Menzel (Code Review)]

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

Change subject: Device enumeration based on IORESOURCE_ASSIGNED
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38547/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38547/2//COMMIT_MSG@7 
PS2, Line 7: Device enumeration based on IORESOURCE_ASSIGNED
Maybe:
> device: Base device enumeration on IORESOURCE_ASSIGNED

https://review.coreboot.org/c/coreboot/+/38547/2/src/device/device.c 
File src/device/device.c:

https://review.coreboot.org/c/coreboot/+/38547/2/src/device/device.c@440 
PS2, Line 440: 		round(bridge->base, bridge->align);
Unrelated.

[Angel Pons (Code Review)]

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

Change subject: Device enumeration based on IORESOURCE_ASSIGNED
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38547/2/src/device/device.c 
File src/device/device.c:

https://review.coreboot.org/c/coreboot/+/38547/2/src/device/device.c@440 
PS2, Line 440: 		round(bridge->base, bridge->align);
> Unrelated.

I guess this was changed back and forth when trying things out? In any case, it would fit on a single line, I think.

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38547 )

Change subject: Device enumeration based on IORESOURCE_ASSIGNED
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38547/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38547/2//COMMIT_MSG@7 
PS2, Line 7: Device enumeration based on IORESOURCE_ASSIGNED
> Maybe: […]

Done

https://review.coreboot.org/c/coreboot/+/38547/2/src/device/device.c 
File src/device/device.c:

https://review.coreboot.org/c/coreboot/+/38547/2/src/device/device.c@440 
PS2, Line 440: 		round(bridge->base, bridge->align);
> I guess this was changed back and forth when trying things out? In any case, it would fit on a singl […]

Done

Change in coreboot[master]: device: Enumerate device based on IORESOURCE_ASSIGNED

[Jonathan Zhang (Code Review)]

Hello David Hendricks, build bot (Jenkins), Anjaneya "Reddy" Chagam,

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

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

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

Change subject: device: Enumerate device based on IORESOURCE_ASSIGNED
......................................................................

device: Enumerate device based on IORESOURCE_ASSIGNED

Intel Xeon Scalable Processors has Split IIO architecture, and also
in order to support multiple sockets, it is necessary to run device
enumeration in consideration of IORESOURCE_ASSIGNED status.

Signed-off-by: Jonathan Zhang 
Signed-off-by: Reddy Chagam 
Change-Id: I69e100ea658fefec905321197b5bd6da1e0e647a
---
M src/device/device.c
1 file changed, 17 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/38547/3

Change in coreboot[master]: device: Enumerate device based on IORESOURCE_ASSIGNED

[Jonathan Zhang (Code Review)]

Hello David Hendricks, build bot (Jenkins), Anjaneya "Reddy" Chagam,

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

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

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

Change subject: device: Enumerate device based on IORESOURCE_ASSIGNED
......................................................................

device: Enumerate device based on IORESOURCE_ASSIGNED

Intel Xeon Scalable Processors has Split IIO architecture, and also
in order to support multiple sockets, it is necessary to run device
enumeration in consideration of IORESOURCE_ASSIGNED status.

Signed-off-by: Jonathan Zhang 
Signed-off-by: Reddy Chagam 
Change-Id: I69e100ea658fefec905321197b5bd6da1e0e647a
---
M src/device/device.c
1 file changed, 17 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/38547/4

Change in coreboot[master]: device: Enumerate device based on IORESOURCE_ASSIGNED

[Jonathan Zhang (Code Review)]

Hello David Hendricks, build bot (Jenkins), Anjaneya "Reddy" Chagam,

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

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

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

Change subject: device: Enumerate device based on IORESOURCE_ASSIGNED
......................................................................

device: Enumerate device based on IORESOURCE_ASSIGNED

Intel Xeon Scalable Processors has Split IIO architecture, and also
in order to support multiple sockets, it is necessary to run device
enumeration in consideration of IORESOURCE_ASSIGNED status.

Signed-off-by: Jonathan Zhang 
Signed-off-by: Reddy Chagam 
Change-Id: I69e100ea658fefec905321197b5bd6da1e0e647a
---
M src/device/device.c
1 file changed, 17 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/38547/5

Change in coreboot[master]: device: Enumerate device based on IORESOURCE_ASSIGNED

[Jonathan Zhang (Code Review)]

Hello David Hendricks, build bot (Jenkins), Anjaneya "Reddy" Chagam,

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

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

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

Change subject: device: Enumerate device based on IORESOURCE_ASSIGNED
......................................................................

device: Enumerate device based on IORESOURCE_ASSIGNED

Intel Xeon Scalable Processors has Split IIO architecture, and also
in order to support multiple sockets, it is necessary to run device
enumeration in consideration of IORESOURCE_ASSIGNED status.

Signed-off-by: Jonathan Zhang 
Signed-off-by: Reddy Chagam 
Change-Id: I69e100ea658fefec905321197b5bd6da1e0e647a
---
M src/device/device.c
1 file changed, 17 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/38547/6

Change in coreboot[master]: device: Enumerate device based on IORESOURCE_ASSIGNED

[Aaron Durbin (Code Review)]

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

Change subject: device: Enumerate device based on IORESOURCE_ASSIGNED
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38547/6/src/device/device.c 
File src/device/device.c:

https://review.coreboot.org/c/coreboot/+/38547/6/src/device/device.c@383 
PS6, Line 383: 		/* Skip if IO resource was assigned */
What is the reasoning? It's not clear from the commit description or the commentary. Does 'assigned' mean don't touch it? Is that what you're trying to accomplish?

Change in coreboot[master]: device: Enumerate device based on IORESOURCE_ASSIGNED

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38547 )

Change subject: device: Enumerate device based on IORESOURCE_ASSIGNED
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38547/6/src/device/device.c 
File src/device/device.c:

https://review.coreboot.org/c/coreboot/+/38547/6/src/device/device.c@383 
PS6, Line 383: 		/* Skip if IO resource was assigned */
> What is the reasoning? It's not clear from the commit description or the commentary. […]

Correct.

Change in coreboot[master]: device: Enumerate device based on IORESOURCE_ASSIGNED

[Aaron Durbin (Code Review)]

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

Change subject: device: Enumerate device based on IORESOURCE_ASSIGNED
......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38547/6/src/device/device.c 
File src/device/device.c:

https://review.coreboot.org/c/coreboot/+/38547/6/src/device/device.c@383 
PS6, Line 383: 		/* Skip if IO resource was assigned */
> Correct.

Please elaborate both in comments as well as patch description the problem and solution for achieving that. That way people understand the intent and solution to the problem.

Change in coreboot[master]: device: Enumerate device based on IORESOURCE_ASSIGNED

[Jonathan Zhang (Code Review)]

Hello Kyösti Mälkki, Duncan Laurie, David Hendricks, build bot (Jenkins), Anjaneya "Reddy" Chagam,

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

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

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

Change subject: device: Enumerate device based on IORESOURCE_ASSIGNED
......................................................................

device: Enumerate device based on IORESOURCE_ASSIGNED

Intel Xeon Scalable Processors has Split IIO architecture, and also
in order to support multiple sockets, it is necessary to run device
enumeration in consideration of IORESOURCE_ASSIGNED status.

Signed-off-by: Jonathan Zhang 
Signed-off-by: Reddy Chagam 
Change-Id: I69e100ea658fefec905321197b5bd6da1e0e647a
---
M src/device/device.c
1 file changed, 17 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/38547/8

Change in coreboot[master]: device: Enumerate device based on IORESOURCE_ASSIGNED

[Jonathan Zhang (Code Review)]

Hello Kyösti Mälkki, Duncan Laurie, David Hendricks, build bot (Jenkins), Anjaneya "Reddy" Chagam,

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

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

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

Change subject: device: Enumerate device based on IORESOURCE_ASSIGNED
......................................................................

device: Enumerate device based on IORESOURCE_ASSIGNED

Intel Xeon Scalable Processors has Split IIO architecture, and also
in order to support multiple sockets, it is necessary to run device
enumeration in consideration of IORESOURCE_ASSIGNED status.

Signed-off-by: Jonathan Zhang 
Signed-off-by: Reddy Chagam 
Change-Id: I69e100ea658fefec905321197b5bd6da1e0e647a
---
M src/device/device.c
1 file changed, 17 insertions(+), 9 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/38547/9

Change in coreboot[master]: device: Enumerate device based on IORESOURCE_ASSIGNED

[David Hendricks (Code Review)]

David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38547 )

Change subject: device: Enumerate device based on IORESOURCE_ASSIGNED
......................................................................

Patch Set 9: Code-Review-1

(2 comments)

https://review.coreboot.org/c/coreboot/+/38547/6/src/device/device.c 
File src/device/device.c:

https://review.coreboot.org/c/coreboot/+/38547/6/src/device/device.c@383 
PS6, Line 383: 		/* Skip if IO resource was assigned */
> Please elaborate both in comments as well as patch description the problem and solution for achievin […]

I'm not sure that changing compute_resources() is what we really want. This is just the first pass where the allocator is trying to figure out the topology so that it can assign resources in the second pass (allocate_resources()). Ignoring pre-assigned resources just means it won't have that info and can mess up in the second pass.

https://review.coreboot.org/c/coreboot/+/38547/6/src/device/device.c@498 
PS6, Line 498: 
I think this is where we'd want to skip pre-assigned resources:
if (resource->flags & IORESOURCE_ASSIGNED)
    continue;

Change in coreboot[master]: device: Enumerate device based on IORESOURCE_ASSIGNED

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38547 )

Change subject: device: Enumerate device based on IORESOURCE_ASSIGNED
......................................................................

Patch Set 9:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38547/6/src/device/device.c 
File src/device/device.c:

https://review.coreboot.org/c/coreboot/+/38547/6/src/device/device.c@383 
PS6, Line 383: 		/* Skip if IO resource was assigned */
> I'm not sure that changing compute_resources() is what we really want. […]

If a device has IO resource assigned which is needed for Xeon-SP processors, it should not be taken into account when calculating the bridge's size/alignment requirements.

https://review.coreboot.org/c/coreboot/+/38547/6/src/device/device.c@498 
PS6, Line 498:
> I think this is where we'd want to skip pre-assigned resources: […]

I tried your code change. The boot hang at Linuxboot kernel with following message:
NR_IRQS: 8448, nr_irqs: 2360, preallocated irqs: 0
In comparison, with original change, the system boots successfully with corresponding message:
NR_IRQS: 4352, nr_irqs: 1256, preallocated irqs: 16

Change in coreboot[master]: device: Enumerate device based on IORESOURCE_ASSIGNED

[Jonathan Zhang (Code Review)]

Hello Kyösti Mälkki, David Hendricks, Duncan Laurie, build bot (Jenkins), Anjaneya "Reddy" Chagam,

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

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

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

Change subject: device: Enumerate device based on IORESOURCE_ASSIGNED
......................................................................

device: Enumerate device based on IORESOURCE_ASSIGNED

Intel Xeon Scalable Processors has split IIO architecture. IIO
(Integrated IO) is split into multiple units referred to
as "stacks". Each stack behaves as a root port and resides on its
own physical bus.

Once resources of a device is assigned, resource computation and
configuration operations cannot be repeated for them, similar like
device with FIXED resources.

Signed-off-by: Jonathan Zhang 
Signed-off-by: Reddy Chagam 
Change-Id: I69e100ea658fefec905321197b5bd6da1e0e647a
---
M src/device/device.c
1 file changed, 22 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/38547/10

Change in coreboot[master]: device: Enumerate device based on IORESOURCE_ASSIGNED

[Nico Huber (Code Review)]

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

Change subject: device: Enumerate device based on IORESOURCE_ASSIGNED
......................................................................

Patch Set 10:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38547/10//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38547/10//COMMIT_MSG@16 
PS10, Line 16: device with FIXED resources.
Please describe what different semantics you need, compared to
IORESOURCE_FIXED.

From device/resource.h:

/* An IO resource the allocator must not change */
    #define IORESOURCE_FIXED        0x80000000

From what you described so far, and what I see in this patch, I
would have guessed that you should use that. If it can't be used,
why?

Change in coreboot[master]: device: Enumerate device based on IORESOURCE_ASSIGNED

[David Hendricks (Code Review)]

David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38547 )

Change subject: device: Enumerate device based on IORESOURCE_ASSIGNED
......................................................................

Patch Set 10: Code-Review-1

(1 comment)

https://review.coreboot.org/c/coreboot/+/38547/6/src/device/device.c 
File src/device/device.c:

https://review.coreboot.org/c/coreboot/+/38547/6/src/device/device.c@383 
PS6, Line 383: 		/* Skip if IO resource was assigned */
> If a device has IO resource assigned which is needed for Xeon-SP processors, it should not be taken  […]

Why not just avoid calling this function all together by adding a condition in dev_configure(), like you did for the setting the resources? And if IORESOURCE_FIXED is correct as Nico suggests, then we might not need to make any changes to this file at all.

Change in coreboot[master]: device: Enumerate device based on IORESOURCE_ASSIGNED

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38547 )

Change subject: device: Enumerate device based on IORESOURCE_ASSIGNED
......................................................................

Patch Set 10:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38547/10//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38547/10//COMMIT_MSG@16 
PS10, Line 16: device with FIXED resources.
> Please describe what different semantics you need, compared to […]

As quoted below from resource.h:
/* An IO resource that has been assigned a value */
#define IORESOURCE_ASSIGNED 0x40000000
/* An IO resource the allocator must not change */
#define IORESOURCE_FIXED  0x80000000
These two macros are different.

https://review.coreboot.org/c/coreboot/+/38547/6/src/device/device.c 
File src/device/device.c:

https://review.coreboot.org/c/coreboot/+/38547/6/src/device/device.c@383 
PS6, Line 383: 		/* Skip if IO resource was assigned */
> Why not just avoid calling this function all together by adding a condition in dev_configure(), like […]

Do you mean avoid calling compute_resources()? Because we need to walk through all the resources on the current bus and compute the amount of address space taken by them, I am not sure how this can be avoided. We are just skip resources that are already assigned.

Change in coreboot[master]: device: Enumerate device based on IORESOURCE_ASSIGNED

[Nico Huber (Code Review)]

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

Change subject: device: Enumerate device based on IORESOURCE_ASSIGNED
......................................................................

Patch Set 12:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38547/10//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38547/10//COMMIT_MSG@16 
PS10, Line 16: device with FIXED resources.
> These two macros are different.

Yes, they are. And that's why I was asking.

I'll try asking a different way: *After* your patch, what differences
between the two are left?

Change in coreboot[master]: device: Enumerate device based on IORESOURCE_ASSIGNED

[Jonathan Zhang (Code Review)]

Hello Kyösti Mälkki, David Hendricks, Duncan Laurie, build bot (Jenkins), Anjaneya "Reddy" Chagam,

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

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

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

Change subject: device: Enumerate device based on IORESOURCE_ASSIGNED
......................................................................

device: Enumerate device based on IORESOURCE_ASSIGNED

Intel Xeon Scalable Processors has split IIO architecture. IIO
(Integrated IO) is split into multiple units referred to
as "stacks". Each stack behaves as a root port and resides on its
own physical bus.

Once resources of a device is assigned, resource computation and
configuration operations cannot be repeated for them, similar like
device with FIXED resources.

Signed-off-by: Jonathan Zhang 
Signed-off-by: Reddy Chagam 
Tested-by: [e-mail]
Change-Id: I69e100ea658fefec905321197b5bd6da1e0e647a
---
M src/device/device.c
1 file changed, 22 insertions(+), 11 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/47/38547/15

Change in coreboot[master]: device: Enumerate device based on IORESOURCE_ASSIGNED

[Aaron Durbin (Code Review)]

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

Change subject: device: Enumerate device based on IORESOURCE_ASSIGNED
......................................................................

Patch Set 15:

(2 comments)

Thanks for more context, but it'm still not clear.

https://review.coreboot.org/c/coreboot/+/38547/15//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38547/15//COMMIT_MSG@12 
PS15, Line 12: own physical bus.
How are the two 'stacks' being assigned resources? Are you presenting a virtual root/bus topology? And if so, doesn't that solve the problem you are describing if one 'stack' is assigned resources prior to the other? I'm trying to map this to my previous experience with Quickpath. But those were fixed bus numbers sitting at 254 and 255, iirc.

https://review.coreboot.org/c/coreboot/+/38547/15//COMMIT_MSG@16 
PS15, Line 16: device with FIXED resources.
I'm having a hard time understanding the current mechanics of this. If there's two 'stacks', how are the devices being grouped? And why after assigning resources to one 'stack' would those devices be included for the other 'stack'?

Change in coreboot[master]: device: Enumerate device based on IORESOURCE_ASSIGNED

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38547 )

Change subject: device: Enumerate device based on IORESOURCE_ASSIGNED
......................................................................

Patch Set 15:
> Patch Set 15:
>
> (2 comments)
>
> Thanks for more context, but it'm still not clear.

Thanks for the review! The SoC patch was reworked (in particular src/soc/intel/xeon_sp/chip.c), now this patch is not needed anymore.

Change in coreboot[master]: device: Enumerate device based on IORESOURCE_ASSIGNED

[Jonathan Zhang (Code Review)]

Jonathan Zhang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38547 )

Change subject: device: Enumerate device based on IORESOURCE_ASSIGNED
......................................................................

Patch Set 15:

(1 comment)

Thanks for the review! The SoC patch was reworked (in particular src/soc/intel/xeon_sp/chip.c), now this patch is not needed anymore. I will abandon it.

https://review.coreboot.org/c/coreboot/+/38547/10//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38547/10//COMMIT_MSG@16 
PS10, Line 16: device with FIXED resources.
> > These two macros are different. […]

Thanks for the review! The SoC patch was reworked (in particular src/soc/intel/xeon_sp/chip.c), now this patch is not needed anymore.

Change in coreboot[master]: device: Enumerate device based on IORESOURCE_ASSIGNED

[Jonathan Zhang (Code Review)]

Jonathan Zhang has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38547 )

Change subject: device: Enumerate device based on IORESOURCE_ASSIGNED
......................................................................

Abandoned

Not needed any more.