Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41899 )
Change subject: device/xhci: Add an xhci driver to enumerate capabilities ......................................................................
device/xhci: Add an xhci driver to enumerate capabilities
This will allow enumerating an xHCI controller to allow dynamically generating the ACPI device nodes.
BUG=b:154756391 TEST=Boot trembyle and check the ACPI tables for XHCI nodes
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I3065c3fffad01b5378a55cfe904f971079b13d0f --- M src/device/Makefile.inc A src/device/xhci.c A src/include/device/xhci.h 3 files changed, 126 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/41899/1
diff --git a/src/device/Makefile.inc b/src/device/Makefile.inc index 2e62d42..7fc303b 100644 --- a/src/device/Makefile.inc +++ b/src/device/Makefile.inc @@ -62,3 +62,5 @@ ramstage-y += resource_allocator_common.c ramstage-$(CONFIG_RESOURCE_ALLOCATOR_V3) += resource_allocator_v3.c ramstage-$(CONFIG_RESOURCE_ALLOCATOR_V4) += resource_allocator_v4.c + +ramstage-y += xhci.c diff --git a/src/device/xhci.c b/src/device/xhci.c new file mode 100644 index 0000000..6c3dcb9 --- /dev/null +++ b/src/device/xhci.c @@ -0,0 +1,70 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include <device/xhci.h> +#include <console/console.h> +#include <device/pci_def.h> +#include <arch/mmio.h> + +union xhci_ext_caps_header { + uint32_t val; + struct { + uint32_t cap_id : 8; + uint32_t next_ptr : 8; + uint32_t reserved : 16; + }; +}; + +enum cb_err xhci_for_each_ext_cap(const struct device *device, void *context, + void (*callback)(void *context, + const struct xhci_ext_cap *cap)) +{ + struct resource *res; + uint8_t *bar; + uint32_t ext_caps_offset; + + if (!device || !callback) + return CB_ERR_ARG; + + res = find_resource(device, PCI_BASE_ADDRESS_0); + if (!res) { + printk(BIOS_ERR, "%s: Unable to find BAR resource for %s\n", __func__, + dev_path(device)); + return CB_ERR; + } + + if (res->limit > 0xFFFFFFFF) { + printk(BIOS_DEBUG, "%s: 64-bit BAR is not supported\n", __func__); + return CB_ERR; + } + + bar = (uint8_t *)(uintptr_t)res->base; + + ext_caps_offset = read16(bar + XHCI_HCCPARAMS1_XECP); + + /* Value is in units of words, so need to multiply by 4 to get the pointer value. */ + ext_caps_offset <<= 2; + + union xhci_ext_caps_header header; + struct xhci_ext_cap cap; + + while (ext_caps_offset) { + header.val = read32(bar + ext_caps_offset); + + cap.cap_id = header.cap_id; + + if (header.cap_id == XHCI_ECP_CAP_ID_SUPP) { + cap.supported_protocol.reg0 = header.val; + cap.supported_protocol.reg1 = read32(bar + ext_caps_offset + 0x4); + cap.supported_protocol.reg2 = read32(bar + ext_caps_offset + 0x8); + } + + callback(context, &cap); + + if (header.next_ptr) + ext_caps_offset += (header.next_ptr << 2); + else + ext_caps_offset = 0; + } + + return CB_SUCCESS; +} diff --git a/src/include/device/xhci.h b/src/include/device/xhci.h new file mode 100644 index 0000000..e49c1cc --- /dev/null +++ b/src/include/device/xhci.h @@ -0,0 +1,54 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#ifndef __DEVICE_XHCI_H__ +#define __DEVICE_XHCI_H__ + +#include <stdint.h> +#include <device/device.h> +#include <commonlib/bsd/cb_err.h> + +#define XHCI_HCCPARAMS1_XECP 0x12 + +#define XHCI_ECP_CAP_ID_LEGACY 1 +#define XHCI_ECP_CAP_ID_SUPP 2 + +struct xhci_supported_protocol { + union { + uint32_t reg0; + struct { + uint32_t cap_id : 8; + uint32_t next_ptr : 8; + uint32_t minor_rev : 8; + uint32_t major_rev : 8; + }; + }; + union { + uint32_t reg1; + char name[4]; + }; + union { + uint32_t reg2; + struct { + uint32_t port_offset : 8; + uint32_t port_count : 8; + uint32_t reserved : 12; + uint32_t protocol_speed_id_count : 4; + }; + }; +}; + +struct xhci_ext_cap { + uint32_t cap_id; + union { + struct xhci_supported_protocol supported_protocol; + }; +}; + +/** + * Iterates over the xHCI Extended Capabilities List. + */ +enum cb_err xhci_for_each_ext_cap(const struct device *device, void *context, + void (*callback)(void *context, + const struct xhci_ext_cap *cap)); + +#endif /* __DEVICE_XHCI_H__ */
build bot (Jenkins) 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 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41899/1/src/device/xhci.c File src/device/xhci.c:
https://review.coreboot.org/c/coreboot/+/41899/1/src/device/xhci.c@17 PS1, Line 17: enum cb_err xhci_for_each_ext_cap(const struct device *device, void *context, that open brace { should be on the previous line
Paul Menzel 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 1: Code-Review+1
(4 comments)
https://review.coreboot.org/c/coreboot/+/41899/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41899/1//COMMIT_MSG@10 PS1, Line 10: generating the ACPI device nodes. I always wonder, if coreboot has code to generate the ACPI code, why the OS/payload can’t do it itself.
https://review.coreboot.org/c/coreboot/+/41899/1/src/device/xhci.c File src/device/xhci.c:
https://review.coreboot.org/c/coreboot/+/41899/1/src/device/xhci.c@42 PS1, Line 42: ext_caps_offset = read16(bar + XHCI_HCCPARAMS1_XECP); Should a warning be printed, if that reads back 0?
https://review.coreboot.org/c/coreboot/+/41899/1/src/device/xhci.c@66 PS1, Line 66: ext_caps_offset = 0; So this basically terminates the while loop? Why not use `break`?
https://review.coreboot.org/c/coreboot/+/41899/1/src/device/xhci.c@68 PS1, Line 68: Should some debug prints be added to print the capabilities for debugging? The Linux driver allows that, I believe.
Raul Rangel 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 1:
(4 comments)
https://review.coreboot.org/c/coreboot/+/41899/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41899/1//COMMIT_MSG@10 PS1, Line 10: generating the ACPI device nodes.
I always wonder, if coreboot has code to generate the ACPI code, why the OS/payload can’t do it itse […]
Coreboot has additional information it needs to pass along for each device node: https://github.com/coreboot/coreboot/blob/master/src/mainboard/google/zork/v...
So we need to basic structure generated to add the additional data.
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?
https://review.coreboot.org/c/coreboot/+/41899/1/src/device/xhci.c File src/device/xhci.c:
https://review.coreboot.org/c/coreboot/+/41899/1/src/device/xhci.c@42 PS1, Line 42: ext_caps_offset = read16(bar + XHCI_HCCPARAMS1_XECP);
Should a warning be printed, if that reads back 0?
I guess I can make it print an error and return CB_ERR.
https://review.coreboot.org/c/coreboot/+/41899/1/src/device/xhci.c@68 PS1, Line 68:
Should some debug prints be added to print the capabilities for debugging? The Linux driver allows t […]
I can add an xhci_print_supported_protocol that the callbacks can call if they want to debug.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41899 )
Change subject: device/xhci: Add an xhci driver to enumerate capabilities ......................................................................
Uploaded patch set 2.
(3 comments)
https://review.coreboot.org/c/coreboot/+/41899/1/src/device/xhci.c File src/device/xhci.c:
https://review.coreboot.org/c/coreboot/+/41899/1/src/device/xhci.c@42 PS1, Line 42: ext_caps_offset = read16(bar + XHCI_HCCPARAMS1_XECP);
I guess I can make it print an error and return CB_ERR.
Done
https://review.coreboot.org/c/coreboot/+/41899/1/src/device/xhci.c@66 PS1, Line 66: ext_caps_offset = 0;
So this basically terminates the while loop? Why not use `break`?
Done
https://review.coreboot.org/c/coreboot/+/41899/1/src/device/xhci.c@68 PS1, Line 68:
I can add an xhci_print_supported_protocol that the callbacks can call if they want to debug.
Done
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Paul Menzel, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41899
to look at the new patch set (#2).
Change subject: device/xhci: Add an xhci driver to enumerate capabilities ......................................................................
device/xhci: Add an xhci driver to enumerate capabilities
This will allow enumerating an xHCI controller to allow dynamically generating the ACPI device nodes.
BUG=b:154756391 TEST=Boot trembyle and see capabilities printed on console xHCI Supported Protocol: Major: 0x2, Minor: 0x0, Protocol: 'USB ' Port Offset: 1, Port Count: 2 xHCI Supported Protocol: Major: 0x3, Minor: 0x10, Protocol: 'USB ' Port Offset: 3, Port Count: 1
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I3065c3fffad01b5378a55cfe904f971079b13d0f --- M src/device/Makefile.inc A src/device/xhci.c A src/include/device/xhci.h 3 files changed, 139 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/41899/2
build bot (Jenkins) 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:
(1 comment)
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@17 PS2, Line 17: enum cb_err xhci_for_each_ext_cap(const struct device *device, void *context, that open brace { should be on the previous line
Raul Rangel 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:
Not sure if this is the correct place to put this code. Suggestions welcome.
Duncan Laurie 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:
(1 comment)
I'm sure we could argue about the location but this seems ok to me.
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@60 PS2, Line 60: reg0 should these be reset to 0 if it isn't supported so the callback doesn't get old data?
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; }
Patrick Rudolph 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: Code-Review+1
(4 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@31 PS2, Line 31: find_resource Please use probe_resource. Find_resource will die instead of returning NULL
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?
It should only check res->base. The resource allocator might still allocate below 4GiB even on a 64bit bar.
https://review.coreboot.org/c/coreboot/+/41899/2/src/device/xhci.c@43 PS2, Line 43: base Use res2mmio
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.
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.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Duncan Laurie, Paul Menzel, Julius Werner, Patrick Rudolph, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41899
to look at the new patch set (#3).
Change subject: device/xhci: Add xHCI utility to enumerate capabilities ......................................................................
device/xhci: Add xHCI utility to enumerate capabilities
This will allow enumerating an xHCI controller to allow dynamically generating the ACPI device nodes.
BUG=b:154756391 TEST=Boot trembyle and see capabilities printed on console xHCI Supported Protocol: Major: 0x2, Minor: 0x0, Protocol: 'USB ' Port Offset: 1, Port Count: 2 xHCI Supported Protocol: Major: 0x3, Minor: 0x10, Protocol: 'USB ' Port Offset: 3, Port Count: 1
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I3065c3fffad01b5378a55cfe904f971079b13d0f --- M src/device/Kconfig M src/device/Makefile.inc A src/device/xhci.c A src/include/device/xhci.h 4 files changed, 144 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/41899/3
build bot (Jenkins) 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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41899/3/src/device/xhci.c File src/device/xhci.c:
https://review.coreboot.org/c/coreboot/+/41899/3/src/device/xhci.c@17 PS3, Line 17: enum cb_err xhci_for_each_ext_cap(const struct device *device, void *context, that open brace { should be on the previous line
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.
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:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41899/3/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/41899/3/src/device/Kconfig@814 PS3, Line 814: config XHCI_UTILS : def_bool n : help : Provides xHCI utility functions. In my opinion, we don't really need a Kconfig option to control this. If it is unused, linker should take care of optimizing it.
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 4.
(3 comments)
https://review.coreboot.org/c/coreboot/+/41899/3/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/41899/3/src/device/Kconfig@814 PS3, Line 814: config XHCI_UTILS : def_bool n : help : Provides xHCI utility functions.
In my opinion, we don't really need a Kconfig option to control this. […]
I know it's fast to compile, but I don't want to slowly degrade compile time by adding unnecessary work. I can remove the option if you think I'm being overly paranoid.
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@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? […]
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; : };
Ack. It would be good to add a comment here regarding that.
Done
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Duncan Laurie, Paul Menzel, Julius Werner, Patrick Rudolph, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41899
to look at the new patch set (#4).
Change subject: device/xhci: Add xHCI utility to enumerate capabilities ......................................................................
device/xhci: Add xHCI utility to enumerate capabilities
This will allow enumerating an xHCI controller to allow dynamically generating the ACPI device nodes.
BUG=b:154756391 TEST=Boot trembyle and see capabilities printed on console xHCI Supported Protocol: Major: 0x2, Minor: 0x0, Protocol: 'USB ' Port Offset: 1, Port Count: 2 xHCI Supported Protocol: Major: 0x3, Minor: 0x10, Protocol: 'USB ' Port Offset: 3, Port Count: 1
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I3065c3fffad01b5378a55cfe904f971079b13d0f --- M src/device/Kconfig M src/device/Makefile.inc A src/device/xhci.c A src/include/device/xhci.h 4 files changed, 150 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/41899/4
build bot (Jenkins) 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 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41899/4/src/device/xhci.c File src/device/xhci.c:
https://review.coreboot.org/c/coreboot/+/41899/4/src/device/xhci.c@17 PS4, Line 17: enum cb_err xhci_for_each_ext_cap(const struct device *device, void *context, that open brace { should be on the previous line
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 5: Patch Set 4 was rebased.
build bot (Jenkins) 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 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41899/5/src/device/xhci.c File src/device/xhci.c:
https://review.coreboot.org/c/coreboot/+/41899/5/src/device/xhci.c@17 PS5, Line 17: enum cb_err xhci_for_each_ext_cap(const struct device *device, void *context, that open brace { should be on the previous line
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 5: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/41899/3/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/41899/3/src/device/Kconfig@814 PS3, Line 814: config XHCI_UTILS : def_bool n : help : Provides xHCI utility functions.
I know it's fast to compile, but I don't want to slowly degrade compile time by adding unnecessary w […]
No, that's fine. It anyways is auto-selected by the XHCI driver, so mainboards don't really need to set this individually.
https://review.coreboot.org/c/coreboot/+/41899/5/src/device/xhci.c File src/device/xhci.c:
https://review.coreboot.org/c/coreboot/+/41899/5/src/device/xhci.c@54 PS5, Line 54: (uint32_t *) nit: This isn't really required.
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 6.
(1 comment)
https://review.coreboot.org/c/coreboot/+/41899/5/src/device/xhci.c File src/device/xhci.c:
https://review.coreboot.org/c/coreboot/+/41899/5/src/device/xhci.c@54 PS5, Line 54: (uint32_t *)
nit: This isn't really required.
Done
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Duncan Laurie, Paul Menzel, Julius Werner, Patrick Rudolph, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41899
to look at the new patch set (#6).
Change subject: device/xhci: Add xHCI utility to enumerate capabilities ......................................................................
device/xhci: Add xHCI utility to enumerate capabilities
This will allow enumerating an xHCI controller to allow dynamically generating the ACPI device nodes.
BUG=b:154756391 TEST=Boot trembyle and see capabilities printed on console xHCI Supported Protocol: Major: 0x2, Minor: 0x0, Protocol: 'USB ' Port Offset: 1, Port Count: 2 xHCI Supported Protocol: Major: 0x3, Minor: 0x10, Protocol: 'USB ' Port Offset: 3, Port Count: 1
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I3065c3fffad01b5378a55cfe904f971079b13d0f --- M src/device/Kconfig M src/device/Makefile.inc A src/device/xhci.c A src/include/device/xhci.h 4 files changed, 150 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/99/41899/6
build bot (Jenkins) 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 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41899/6/src/device/xhci.c File src/device/xhci.c:
https://review.coreboot.org/c/coreboot/+/41899/6/src/device/xhci.c@17 PS6, Line 17: enum cb_err xhci_for_each_ext_cap(const struct device *device, void *context, that open brace { should be on the previous line
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 6: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41899 )
Change subject: device/xhci: Add xHCI utility to enumerate capabilities ......................................................................
device/xhci: Add xHCI utility to enumerate capabilities
This will allow enumerating an xHCI controller to allow dynamically generating the ACPI device nodes.
BUG=b:154756391 TEST=Boot trembyle and see capabilities printed on console xHCI Supported Protocol: Major: 0x2, Minor: 0x0, Protocol: 'USB ' Port Offset: 1, Port Count: 2 xHCI Supported Protocol: Major: 0x3, Minor: 0x10, Protocol: 'USB ' Port Offset: 3, Port Count: 1
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I3065c3fffad01b5378a55cfe904f971079b13d0f Reviewed-on: https://review.coreboot.org/c/coreboot/+/41899 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Furquan Shaikh furquan@google.com --- M src/device/Kconfig M src/device/Makefile.inc A src/device/xhci.c A src/include/device/xhci.h 4 files changed, 150 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/device/Kconfig b/src/device/Kconfig index 79ce77d..f7f4b90 100644 --- a/src/device/Kconfig +++ b/src/device/Kconfig @@ -811,4 +811,9 @@ ranges for allocating resources. This allows allocation of resources above 4G boundary as well.
+config XHCI_UTILS + def_bool n + help + Provides xHCI utility functions. + endmenu diff --git a/src/device/Makefile.inc b/src/device/Makefile.inc index 2e62d42..2fae44a 100644 --- a/src/device/Makefile.inc +++ b/src/device/Makefile.inc @@ -62,3 +62,5 @@ ramstage-y += resource_allocator_common.c ramstage-$(CONFIG_RESOURCE_ALLOCATOR_V3) += resource_allocator_v3.c ramstage-$(CONFIG_RESOURCE_ALLOCATOR_V4) += resource_allocator_v4.c + +ramstage-$(CONFIG_XHCI_UTILS) += xhci.c diff --git a/src/device/xhci.c b/src/device/xhci.c new file mode 100644 index 0000000..ce1c1b2 --- /dev/null +++ b/src/device/xhci.c @@ -0,0 +1,86 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include <device/xhci.h> +#include <console/console.h> +#include <device/pci_def.h> +#include <arch/mmio.h> + +union xhci_ext_caps_header { + uint32_t val; + struct { + uint32_t cap_id : 8; + uint32_t next_ptr : 8; + uint32_t reserved : 16; + }; +}; + +enum cb_err xhci_for_each_ext_cap(const struct device *device, void *context, + void (*callback)(void *context, + const struct xhci_ext_cap *cap)) +{ + struct resource *res; + uint32_t *ext_cap_ptr; + uint32_t ext_caps_word_offset; + union xhci_ext_caps_header header; + struct xhci_ext_cap cap; + + if (!device || !callback) + return CB_ERR_ARG; + + res = probe_resource(device, PCI_BASE_ADDRESS_0); + if (!res) { + printk(BIOS_ERR, "%s: Unable to find BAR resource for %s\n", __func__, + dev_path(device)); + return CB_ERR; + } + + if (!(res->flags & IORESOURCE_ASSIGNED)) { + printk(BIOS_ERR, "%s: BAR is is not assigned\n", __func__); + return CB_ERR; + } + + if (res->limit > 0xFFFFFFFF) { + printk(BIOS_ERR, "%s: 64-bit BAR is not supported\n", __func__); + return CB_ERR; + } + + ext_caps_word_offset = read16(res2mmio(res, XHCI_HCCPARAMS1_XECP, 0)); + + if (!ext_caps_word_offset) { + printk(BIOS_ERR, "%s: No extended capabilities defined\n", __func__); + return CB_ERR; + } + + ext_cap_ptr = res2mmio(res, ext_caps_word_offset << 2, 0); + + while ((uintptr_t)ext_cap_ptr < (uintptr_t)res->limit) { + header.val = read32(ext_cap_ptr); + + cap.cap_id = header.cap_id; + + if (header.cap_id == XHCI_ECP_CAP_ID_SUPP) { + cap.supported_protocol.reg0 = header.val; + cap.supported_protocol.reg1 = read32(ext_cap_ptr + 1); + cap.supported_protocol.reg2 = read32(ext_cap_ptr + 2); + } + + callback(context, &cap); + + if (!header.next_ptr) + break; + + ext_cap_ptr += header.next_ptr; + } + + return CB_SUCCESS; +} + +void xhci_print_supported_protocol(const struct xhci_supported_protocol *supported_protocol) +{ + printk(BIOS_DEBUG, "xHCI Supported Protocol:\n"); + printk(BIOS_DEBUG, " Major: %#x, Minor: %#x, Protocol: '%.*s'\n", + supported_protocol->major_rev, supported_protocol->minor_rev, + (int)sizeof(supported_protocol->name), supported_protocol->name); + printk(BIOS_DEBUG, " Port Offset: %d, Port Count: %d\n", + supported_protocol->port_offset, supported_protocol->port_count); +} diff --git a/src/include/device/xhci.h b/src/include/device/xhci.h new file mode 100644 index 0000000..25f950b --- /dev/null +++ b/src/include/device/xhci.h @@ -0,0 +1,57 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#ifndef __DEVICE_XHCI_H__ +#define __DEVICE_XHCI_H__ + +#include <stdint.h> +#include <device/device.h> +#include <commonlib/bsd/cb_err.h> + +#define XHCI_HCCPARAMS1_XECP 0x12 + +#define XHCI_ECP_CAP_ID_LEGACY 1 +#define XHCI_ECP_CAP_ID_SUPP 2 + +struct xhci_supported_protocol { + union { + uint32_t reg0; + struct { + uint32_t cap_id : 8; + uint32_t next_ptr : 8; + uint32_t minor_rev : 8; + uint32_t major_rev : 8; + }; + }; + union { + uint32_t reg1; + char name[4]; + }; + union { + uint32_t reg2; + struct { + uint32_t port_offset : 8; + uint32_t port_count : 8; + uint32_t reserved : 12; + uint32_t protocol_speed_id_count : 4; + }; + }; +}; + +struct xhci_ext_cap { + uint32_t cap_id; + /* cap_id is used to select the correct struct in the union. */ + union { + struct xhci_supported_protocol supported_protocol; + }; +}; + +/** + * Iterates over the xHCI Extended Capabilities List. + */ +enum cb_err xhci_for_each_ext_cap(const struct device *device, void *context, + void (*callback)(void *context, + const struct xhci_ext_cap *cap)); + +void xhci_print_supported_protocol(const struct xhci_supported_protocol *supported_protocol); + +#endif /* __DEVICE_XHCI_H__ */
-
build bot (Jenkins) (Code Review) -
Duncan Laurie (Code Review)
-
Furquan Shaikh (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Raul Rangel (Code Review)