Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41899 )
Change subject: device/xhci: Add xHCI utility to enumerate capabilities ......................................................................
Uploaded patch set 3.
(9 comments)
https://review.coreboot.org/c/coreboot/+/41899/1/src/device/Makefile.inc File src/device/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41899/1/src/device/Makefile.inc@66 PS1, Line 66: y
Should I add a Kconfig for this?
Done
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
Shouldn't this be put in xhci. […]
I want to keep this one as an implementation detail since the reserved bytes don't provide any value.
https://review.coreboot.org/c/coreboot/+/41899/2/src/device/xhci.c@31 PS2, Line 31: find_resource
Please use probe_resource. […]
Done
https://review.coreboot.org/c/coreboot/+/41899/2/src/device/xhci.c@38 PS2, Line 38: res->limit > 0xFFFFFFFF
It should only check res->base. […]
If the limit is below 4GiB doesn't that means the base is guaranteed to be below 4GiB? I was trying to catch the case where base is below 4GiB, and limit is above 4GiB. If that happens then I can't address the full range. I know it's unlikely, but was being defensive. Did I make a wrong assumption?
https://review.coreboot.org/c/coreboot/+/41899/2/src/device/xhci.c@43 PS2, Line 43: base
Use res2mmio
Done. Though not sure it makes it anymore readable.
https://review.coreboot.org/c/coreboot/+/41899/2/src/device/xhci.c@45 PS2, Line 45: ext_caps_offset
Just a note: This is in units of words. […]
Done
https://review.coreboot.org/c/coreboot/+/41899/2/src/device/xhci.c@60 PS2, Line 60: reg0
+1.
I figured it didn't matter. It's an error for the callback to look at supported_protocol unless the cap_id == XHCI_ECP_CAP_ID_SUPP.
https://review.coreboot.org/c/coreboot/+/41899/2/src/device/xhci.c@67 PS2, Line 67: ext_cap_ptr
Check that you are still within the BAR.
Done
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 is a little confusing to me. struct xhci_supported_protocol already has cap_id in it. […]
This cap_id is so you can cleanly switch on the union. You shouldn't access any of the union members without checking the cap_id. It's essentially a oneof in protobuf land.