Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41443 )
Change subject: device: Add support for resource allocator v4 ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41443/6/src/device/resource_allocat... File src/device/resource_allocator_v4.c:
https://review.coreboot.org/c/coreboot/+/41443/6/src/device/resource_allocat... PS6, Line 272: mark_resource_invalid(resource);
First, I don't see how `base == limit` implies or disables anything. Please explain.
I think what you are looking for is limit < base so that it is disabled as per the PCI Specification?
No, I'm looking for anything. It seems to me that you just made that rule up. If not, please tell me where I'm wrong.
But please tell me how pci_set_resource() does the right thing for a non-
bridge resource. I just don't see it.
What is the definition of right thing for a non-bridge resource to be disabled? Again as per PCI specification, there are separate enable control bits that decide when to enable the decoding for I/O and mem for non-bridge resource. So, how do you really define that a resource is disabled?
For PCI it's when the command bit is not set. For coreboot, I only see a single option: don't set IORESOURCE_ASSIGNED as we always did.
My argument here is that we are bringing in the semantics of PCI devices into the resource allocator.
That's exactly what I want to prevent and why I suggest to adapt the PCI code instead of adding quirks to the allocator.
I understand that we are dealing only with PCI devices right now, but both the old and new resource allocators are written without really making assumptions about the kind of devices that are requesting these resources. In that case, we need to come up with a notion of saying how you mark a device as invalid/disabled.
We already have that. The old allocator didn't set IORESOURCE_ASSIGNED on the error path (ok, that path failed to execute but the idea was there and sound, IMHO).
If you think that needs to be changed, ok. But it's exactly this kind of change that is harder with two allocators in the tree...