Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41899 )
Change subject: device/xhci: Add xHCI utility to enumerate capabilities ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41899/2/src/device/xhci.c File src/device/xhci.c:
https://review.coreboot.org/c/coreboot/+/41899/2/src/device/xhci.c@8 PS2, Line 8: xhci_ext_caps_header
I want to keep this one as an implementation detail since the reserved bytes don't provide any value […]
Ack.
https://review.coreboot.org/c/coreboot/+/41899/2/src/device/xhci.c@38 PS2, Line 38: res->limit > 0xFFFFFFFF
If the limit is below 4GiB doesn't that means the base is guaranteed to be below 4GiB?
Yes, the resource allocator updates the limit after allocation. I meant to say that there could potentially be more conditions where the base is not valid,. Ideally, we should check res->flags & IORESOURCE_ASSIGNED and res->limit <= 0xFFFFFFFF to ensure that the resource is actually assigned below 4G.
I was trying to catch the case where base is below 4GiB, and limit is above 4GiB.
Currently, resource allocator doesn't allow allocations across 4G boundary. But, I agree its better not to encode those assumptions here.
https://review.coreboot.org/c/coreboot/+/41899/2/src/include/device/xhci.h File src/include/device/xhci.h:
https://review.coreboot.org/c/coreboot/+/41899/2/src/include/device/xhci.h@4... PS2, Line 41: uint32_t cap_id; : union { : struct xhci_supported_protocol supported_protocol; : };
This cap_id is so you can cleanly switch on the union. […]
Ack. It would be good to add a comment here regarding that.