Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41899 )
Change subject: device/xhci: Add an xhci driver to enumerate capabilities ......................................................................
Patch Set 2:
(5 comments)
Patch Set 2:
(1 comment)
I'm sure we could argue about the location but this seems ok to me.
Yeah, this location seems okay.
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.h so that struct xhci_ext_cap can use it?
https://review.coreboot.org/c/coreboot/+/41899/2/src/device/xhci.c@38 PS2, Line 38: res->limit > 0xFFFFFFFF Should res->base be checked as well?
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.
Should we use ext_caps_offset_words?
https://review.coreboot.org/c/coreboot/+/41899/2/src/device/xhci.c@60 PS2, Line 60: reg0
should these be reset to 0 if it isn't supported so the callback doesn't get old data?
+1.
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. Why is there a separate uint32_t cap_id in this structure? Should this instead be:
union xhci_ext_cap { union xhci_ext_caps_header hdr; struct xhci_supported_protocol supported_protocol; }