Change in coreboot[master]: device/pci_device: Do not complain about unassigned resources for PCI...
Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41523 )
Change subject: device/pci_device: Do not complain about unassigned resources for PCI bridge ......................................................................
device/pci_device: Do not complain about unassigned resources for PCI bridge
This change prints an error for unassigned resource in pci_set_resource() only if the resource is a non-bridge resource. This is because bridge resource can remain unassigned if there are no downstream child devices requesting any resource of that type.
Change-Id: I8116cc8bdcf5a10049330600b1cb060d7880205a Signed-off-by: Furquan Shaikh furquan@google.com --- M src/device/pci_device.c 1 file changed, 10 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/23/41523/1
diff --git a/src/device/pci_device.c b/src/device/pci_device.c index 689325d..47b4613 100644 --- a/src/device/pci_device.c +++ b/src/device/pci_device.c @@ -450,9 +450,16 @@
/* Make certain the resource has actually been assigned a value. */ if (!(resource->flags & IORESOURCE_ASSIGNED)) { - printk(BIOS_ERR, "ERROR: %s %02lx %s size: 0x%010llx not " - "assigned\n", dev_path(dev), resource->index, - resource_type(resource), resource->size); + + /* + * Complain about unassigned resource only if this is not a bridge + * resource. Bridge resource can be left unassigned if the downstream child + * devices do not really need resource of a particular type. + */ + if (!(resource->flags & IORESOURCE_BRIDGE)) + printk(BIOS_ERR, "ERROR: %s %02lx %s size: 0x%010llx not " + "assigned\n", dev_path(dev), resource->index, + resource_type(resource), resource->size); return; }
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41523 )
Change subject: device/pci_device: Do not complain about unassigned resources for PCI bridge ......................................................................
Patch Set 1: Code-Review-1
The error seems valid. The size 0 case is explicitly handled to fill the bridge registers with sane values. We need a solution that doesn't bail out.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41523 )
Change subject: device/pci_device: Do not complain about unassigned resources for PCI bridge ......................................................................
Patch Set 1:
Patch Set 1: Code-Review-1
The error seems valid.
I don't understand how. If we are expecting the resource allocator to not set the IORESOURCE_ASSIGNED flag then it is expected that control would enter pci_set_resource() for a resource that has size 0 and does not have the IORESOURCE_ASSIGNED flag. In that case, the error is not really valid.
The size 0 case is explicitly handled to fill the bridge registers with sane values. We need a solution that doesn't bail out.
I believe you are referring to line 503 in the file? That makes sense to me, but it is really odd that we are saying the the resource does not have the IORESOURCE_ASSIGNED flag, but still has the IORESOURCE_STORED flag(line 499).
So, what are the semantics of these flags:
IORESOURCE_ASSIGNED --> Resource is assigned a value by the allocator IORESOURCE_STORED --> Either the assignment from allocator is stored in the device or if it is a bridge device without assignment, then assignment to disable the resource is stored in the device.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41523 )
Change subject: device/pci_device: Do not complain about unassigned resources for PCI bridge ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1: Code-Review-1
The error seems valid.
I don't understand how. If we are expecting the resource allocator to not set the IORESOURCE_ASSIGNED flag then it is expected that control would enter pci_set_resource() for a resource that has size 0 and does not have the IORESOURCE_ASSIGNED flag. In that case, the error is not really valid.
The message is not valid. But we must not bail out.
The size 0 case is explicitly handled to fill the bridge registers with sane values. We need a solution that doesn't bail out.
I believe you are referring to line 503 in the file? That makes sense to me, but it is really odd that we are saying the the resource does not have the IORESOURCE_ASSIGNED flag, but still has the IORESOURCE_STORED flag(line 499).
So, what are the semantics of these flags:
IORESOURCE_ASSIGNED --> Resource is assigned a value by the allocator IORESOURCE_STORED --> Either the assignment from allocator is stored in the device or if it is a bridge device without assignment, then assignment to disable the resource is stored in the device.
This would be the result, yes. I would rather like to bend the meaning of _STORED than that of _ASSIGNED. Currently, the comment above the former says
/* The IO resource assignment has been stored in the device */
IMHO, this wasn't true anyway. As there sometimes was no real assignment.
I'm currently refactoring this function. Will push in a few minutes.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41523 )
Change subject: device/pci_device: Do not complain about unassigned resources for PCI bridge ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1: Code-Review-1
The error seems valid.
I don't understand how. If we are expecting the resource allocator to not set the IORESOURCE_ASSIGNED flag then it is expected that control would enter pci_set_resource() for a resource that has size 0 and does not have the IORESOURCE_ASSIGNED flag. In that case, the error is not really valid.
The size 0 case is explicitly handled to fill the bridge registers with sane values. We need a solution that doesn't bail out.
I believe you are referring to line 503 in the file? That makes sense to me, but it is really odd that we are saying the the resource does not have the IORESOURCE_ASSIGNED flag, but still has the IORESOURCE_STORED flag(line 499).
So, what are the semantics of these flags:
IORESOURCE_ASSIGNED --> Resource is assigned a value by the allocator IORESOURCE_STORED --> Either the assignment from allocator is stored in the device or if it is a bridge device without assignment, then assignment to disable the resource is stored in the device.
Revisiting the code here and in the resource allocator, I am planning to abandon this patch and continue with the semantics of IORESOURCE_ASSIGNED meaning that the resource allocator has assigned a value to the base. If the resource is allocated space, base is set to whatever the base of the allocated address space is and if the resource is not allocated space, then base is set to the limit so that it is disabled.
Furquan Shaikh has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/41523 )
Change subject: device/pci_device: Do not complain about unassigned resources for PCI bridge ......................................................................
Abandoned
Not required anymore. pci_set_resource() is being refactored here: https://review.coreboot.org/c/coreboot/+/41553
-
Furquan Shaikh (Code Review) -
Nico Huber (Code Review)