Change in coreboot[master]: soc/amd/common/blocks/xhci: Add an AMD block to generate xHCI ACPI nodes
Raul Rangel has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: soc/amd/common/blocks/xhci: Add an AMD block to generate xHCI ACPI nodes ......................................................................
soc/amd/common/blocks/xhci: Add an AMD block to generate xHCI ACPI nodes
We can use xhci_for_each_ext_cap to inspect the xHC so we generate the correct number of device nodes.
BUG=b:154756391 TEST=Boot trembyle and look at ACPI table. See all xHCI nodes.
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I44ebaef342e45923bc181ceebef882358d33f0d1 --- A src/soc/amd/common/block/xhci/Kconfig A src/soc/amd/common/block/xhci/Makefile.inc A src/soc/amd/common/block/xhci/xhci.c 3 files changed, 128 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/41900/1
diff --git a/src/soc/amd/common/block/xhci/Kconfig b/src/soc/amd/common/block/xhci/Kconfig new file mode 100644 index 0000000..5751907 --- /dev/null +++ b/src/soc/amd/common/block/xhci/Kconfig @@ -0,0 +1,5 @@ +config SOC_AMD_COMMON_BLOCK_XHCI + bool + default n + help + Select this option to use AMD common xhci driver support. diff --git a/src/soc/amd/common/block/xhci/Makefile.inc b/src/soc/amd/common/block/xhci/Makefile.inc new file mode 100644 index 0000000..2387450 --- /dev/null +++ b/src/soc/amd/common/block/xhci/Makefile.inc @@ -0,0 +1 @@ +ramstage-$(CONFIG_SOC_AMD_COMMON_BLOCK_XHCI) += xhci.c diff --git a/src/soc/amd/common/block/xhci/xhci.c b/src/soc/amd/common/block/xhci/xhci.c new file mode 100644 index 0000000..72bb9bb --- /dev/null +++ b/src/soc/amd/common/block/xhci/xhci.c @@ -0,0 +1,122 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <acpi/acpigen.h> +#include <amdblocks/gpio_banks.h> +#include <console/console.h> +#include <device/device.h> +#include <device/pci.h> +#include <device/pci_ids.h> +#include <device/pci_ops.h> +#include <device/pci_ehci.h> +#include <soc/acpi.h> +#include <soc/pci_devs.h> +#include <soc/smi.h> +#include <soc/southbridge.h> +#include <amdblocks/acpimmio.h> +#include <device/xhci.h> + +#define XHCI_GEVENT GEVENT_31 + +struct port_counts { + unsigned int high_speed; + unsigned int super_speed; +}; + +static void handle_xhci_ext_cap(void *context, const struct xhci_ext_cap *cap) +{ + const struct xhci_supported_protocol *data; + const char *format; + char buf[16]; + struct port_counts *counts = context; + unsigned int *dev_num; + + if (cap->cap_id != XHCI_ECP_CAP_ID_SUPP) + return; + + data = &cap->supported_protocol; + + if (memcmp(data->name, "USB ", 4)) { + printk(BIOS_INFO, "%s: Unknown Protocol: %.*s\n", __func__, + (int)sizeof(data->name), data->name); + return; + } + + if (data->major_rev == 3) { + format = "SS%02d"; + dev_num = &counts->super_speed; + } else if (data->major_rev == 2) { + format = "HS%02d"; + dev_num = &counts->high_speed; + } else { + printk(BIOS_INFO, "%s: Unknown USB Version: %#x\n", __func__, data->major_rev); + return; + } + + for (unsigned int i = 0; i < data->port_count; ++i) { + snprintf(buf, sizeof(buf), format, ++(*dev_num)); + acpigen_write_device(buf); + acpigen_write_name_byte("_ADR", data->port_offset + i); + acpigen_pop_len(); + } +} + +static void xhci_add_devices(const struct device *device) +{ + struct port_counts counts = {0, 0}; + + acpigen_write_device("RHUB"); + acpigen_write_name_integer("_ADR", 0x00000000); + + xhci_for_each_ext_cap(device, &counts, handle_xhci_ext_cap); + + /* Exit RHUB device */ + acpigen_pop_len(); +} + +static void xhci_fill_ssdt_generator(const struct device *device) +{ + printk(BIOS_INFO, "xHCI SSDT generation\n"); + + /* Write SSDT entry for xHCI controller */ + acpigen_write_scope(acpi_device_scope(device)); + acpigen_write_device(acpi_device_name(device)); + acpigen_write_ADR_pci_device(device); + acpigen_write_STA(acpi_device_status(device)); + acpigen_write_PRW(XHCI_GEVENT, 3); + + xhci_add_devices(device); + + acpigen_write_name_integer("_S0W", 0); + acpigen_write_name_integer("_S3W", 4); + acpigen_write_name_integer("_S4W", 4); + + acpigen_pop_len(); // xHCI device + + acpigen_pop_len(); // Bridge scope +} + +static struct pci_operations lops_pci = { + .set_subsystem = pci_dev_set_subsystem, +}; + +static struct device_operations usb_ops = { + .read_resources = pci_dev_read_resources, + .set_resources = pci_dev_set_resources, + .enable_resources = pci_dev_enable_resources, + .scan_bus = scan_static_bus, + .ops_pci = &lops_pci, + .acpi_fill_ssdt = xhci_fill_ssdt_generator, +}; + +static const unsigned short pci_device_ids[] = { + PCI_DEVICE_ID_AMD_FAM17H_MODEL18H_XHCI0, + PCI_DEVICE_ID_AMD_FAM17H_MODEL18H_XHCI1, + PCI_DEVICE_ID_AMD_FAM17H_MODEL20H_XHCI0, + 0 +}; + +static const struct pci_driver usb_0_driver __pci_driver = { + .ops = &usb_ops, + .vendor = PCI_VENDOR_ID_AMD, + .devices = pci_device_ids, +};
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: soc/amd/common/blocks/xhci: Add an AMD block to generate xHCI ACPI nodes ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41900/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41900/1//COMMIT_MSG@13 PS1, Line 13: TEST=Boot trembyle and look at ACPI table. See all xHCI nodes. I like, how Ducan often pastes the generated ASL code into the commit message. Does this make sense here too?
https://review.coreboot.org/c/coreboot/+/41900/1/src/soc/amd/common/block/xh... File src/soc/amd/common/block/xhci/Kconfig:
https://review.coreboot.org/c/coreboot/+/41900/1/src/soc/amd/common/block/xh... PS1, Line 5: xhci xHCI
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41900
to look at the new patch set (#2).
Change subject: soc/amd/common/blocks/xhci: Add an AMD block to generate xHCI ACPI nodes ......................................................................
soc/amd/common/blocks/xhci: Add an AMD block to generate xHCI ACPI nodes
We can use xhci_for_each_ext_cap to inspect the xHC so we generate the correct number of device nodes.
Scope (_SB.PCI0.PBRA) { Device (XHC1) { Name (_ADR, 0x0000000000000004) // _ADR: Address Method (_STA, 0, NotSerialized) // _STA: Status { Return (0x0F) }
Name (_PRW, Package (0x02) // _PRW: Power Resources for Wake { 0x1F, 0x03 }) Device (RHUB) { Name (_ADR, Zero) // _ADR: Address Device (HS01) { Name (_ADR, 0x01) // _ADR: Address }
Device (HS02) { Name (_ADR, 0x02) // _ADR: Address }
Device (SS01) { Name (_ADR, 0x03) // _ADR: Address } }
Name (_S0W, Zero) // _S0W: S0 Device Wake State Name (_S3W, 0x04) // _S3W: S3 Device Wake State Name (_S4W, 0x04) // _S4W: S4 Device Wake State } }
BUG=b:154756391 TEST=Boot trembyle and look at ACPI table. See all xHCI nodes.
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I44ebaef342e45923bc181ceebef882358d33f0d1 --- A src/soc/amd/common/block/xhci/Kconfig A src/soc/amd/common/block/xhci/Makefile.inc A src/soc/amd/common/block/xhci/xhci.c 3 files changed, 121 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/41900/2
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: soc/amd/common/blocks/xhci: Add an AMD block to generate xHCI ACPI nodes ......................................................................
Uploaded patch set 2.
(2 comments)
https://review.coreboot.org/c/coreboot/+/41900/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41900/1//COMMIT_MSG@13 PS1, Line 13: TEST=Boot trembyle and look at ACPI table. See all xHCI nodes.
I like, how Ducan often pastes the generated ASL code into the commit message. […]
Done
https://review.coreboot.org/c/coreboot/+/41900/1/src/soc/amd/common/block/xh... File src/soc/amd/common/block/xhci/Kconfig:
https://review.coreboot.org/c/coreboot/+/41900/1/src/soc/amd/common/block/xh... PS1, Line 5: xhci
xHCI
Done
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: soc/amd/common/blocks/xhci: Add an AMD block to generate xHCI ACPI nodes ......................................................................
Patch Set 2:
(1 comment)
I like this idea, it looks like a lot of it could go in common code in drivers/usb even.
https://review.coreboot.org/c/coreboot/+/41900/2/src/soc/amd/common/block/xh... File src/soc/amd/common/block/xhci/xhci.c:
https://review.coreboot.org/c/coreboot/+/41900/2/src/soc/amd/common/block/xh... PS2, Line 102: }; maybe also handle acpi_name in here too
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: soc/amd/common/blocks/xhci: Add an AMD block to generate xHCI ACPI nodes ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41900/2/src/soc/amd/common/block/xh... File src/soc/amd/common/block/xhci/xhci.c:
https://review.coreboot.org/c/coreboot/+/41900/2/src/soc/amd/common/block/xh... PS2, Line 16: handle_xhci_ext_cap This works fine. But I am curious: Can we not use PORT_CONTROL_CNTR0 and PORT_CONTROL_CNTR1 to extract the number of USB3 and USB2 ports for each controller? Just saves the effort of having to parse the extended capability list.
https://review.coreboot.org/c/coreboot/+/41900/2/src/soc/amd/common/block/xh... PS2, Line 100: lops_pci This can be set to &pci_dev_ops_pci since there is nothing special being done as part of lops_pci.
https://review.coreboot.org/c/coreboot/+/41900/2/src/soc/amd/common/block/xh... PS2, Line 102: };
maybe also handle acpi_name in here too
+1. acpi_name should also be moved to this file.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41900
to look at the new patch set (#3).
Change subject: drivers/xhci/acpi: Add a driver to generate xHCI ACPI nodes ......................................................................
drivers/xhci/acpi: Add a driver to generate xHCI ACPI nodes
We can use xhci_for_each_ext_cap to inspect the xHC so we generate the correct number of device nodes.
Scope (_SB.PCI0.PBRA) { Device (XHC1) { Name (_ADR, 0x0000000000000004) // _ADR: Address Method (_STA, 0, NotSerialized) // _STA: Status { Return (0x0F) }
Name (_PRW, Package (0x02) // _PRW: Power Resources for Wake { 0x1F, 0x03 }) Device (RHUB) { Name (_ADR, Zero) // _ADR: Address Device (HS01) { Name (_ADR, 0x01) // _ADR: Address }
Device (HS02) { Name (_ADR, 0x02) // _ADR: Address }
Device (SS01) { Name (_ADR, 0x03) // _ADR: Address } }
Name (_S0W, Zero) // _S0W: S0 Device Wake State Name (_S3W, 0x04) // _S3W: S3 Device Wake State Name (_S4W, 0x04) // _S4W: S4 Device Wake State } }
BUG=b:154756391 TEST=Boot trembyle and look at ACPI table. See all xHCI nodes.
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I44ebaef342e45923bc181ceebef882358d33f0d1 --- A src/drivers/xhci/acpi/Kconfig A src/drivers/xhci/acpi/Makefile.inc A src/drivers/xhci/acpi/chip.h A src/drivers/xhci/acpi/xhci_acpi.c 4 files changed, 171 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/41900/3
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: drivers/xhci/acpi: Add a driver to generate xHCI ACPI nodes ......................................................................
Uploaded patch set 3.
(3 comments)
https://review.coreboot.org/c/coreboot/+/41900/2/src/soc/amd/common/block/xh... File src/soc/amd/common/block/xhci/xhci.c:
https://review.coreboot.org/c/coreboot/+/41900/2/src/soc/amd/common/block/xh... PS2, Line 16: handle_xhci_ext_cap
This works fine. […]
I guess I could have done that. Didn't know about that register.
https://review.coreboot.org/c/coreboot/+/41900/2/src/soc/amd/common/block/xh... PS2, Line 100: lops_pci
This can be set to &pci_dev_ops_pci since there is nothing special being done as part of lops_pci.
Done
https://review.coreboot.org/c/coreboot/+/41900/2/src/soc/amd/common/block/xh... PS2, Line 102: };
+1. acpi_name should also be moved to this file.
Done.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: drivers/xhci/acpi: Add a driver to generate xHCI ACPI nodes ......................................................................
Patch Set 3:
Patch Set 2:
(1 comment)
I like this idea, it looks like a lot of it could go in common code in drivers/usb even.
Made it more generic and moved it to drivers/xhci.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: drivers/xhci/acpi: Add a driver to generate xHCI ACPI nodes ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/41900/3/src/drivers/xhci/acpi/Makef... File src/drivers/xhci/acpi/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41900/3/src/drivers/xhci/acpi/Makef... PS3, Line 1: xhci_acpi I don't think this is dealing just with ACPI. Also, should this be placed under drivers/usb? Something like drivers/usb/pci_xhci/xhci.c
https://review.coreboot.org/c/coreboot/+/41900/3/src/drivers/xhci/acpi/xhci_... File src/drivers/xhci/acpi/xhci_acpi.c:
https://review.coreboot.org/c/coreboot/+/41900/3/src/drivers/xhci/acpi/xhci_... PS3, Line 92: counts How does counts get used? I see that handle_xhci_ext_cap() is setting it. But, it doesn't really get used anywhere.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41900
to look at the new patch set (#4).
Change subject: drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes ......................................................................
drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes
We can use xhci_for_each_ext_cap to inspect the xHC so we generate the correct number of device nodes.
Scope (_SB.PCI0.PBRA) { Device (XHC1) { Name (_ADR, 0x0000000000000004) // _ADR: Address Method (_STA, 0, NotSerialized) // _STA: Status { Return (0x0F) }
Name (_PRW, Package (0x02) // _PRW: Power Resources for Wake { 0x1F, 0x03 }) Device (RHUB) { Name (_ADR, Zero) // _ADR: Address Device (HS01) { Name (_ADR, 0x01) // _ADR: Address }
Device (HS02) { Name (_ADR, 0x02) // _ADR: Address }
Device (SS01) { Name (_ADR, 0x03) // _ADR: Address } }
Name (_S0W, Zero) // _S0W: S0 Device Wake State Name (_S3W, 0x04) // _S3W: S3 Device Wake State Name (_S4W, 0x04) // _S4W: S4 Device Wake State } }
BUG=b:154756391 TEST=Boot trembyle and look at ACPI table. See all xHCI nodes.
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I44ebaef342e45923bc181ceebef882358d33f0d1 --- A src/drivers/usb/pci_xhci/Kconfig A src/drivers/usb/pci_xhci/Makefile.inc A src/drivers/usb/pci_xhci/chip.h A src/drivers/usb/pci_xhci/pci_xhci.c 4 files changed, 189 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/41900/4
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes ......................................................................
Uploaded patch set 4.
(2 comments)
https://review.coreboot.org/c/coreboot/+/41900/3/src/drivers/xhci/acpi/Makef... File src/drivers/xhci/acpi/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/41900/3/src/drivers/xhci/acpi/Makef... PS3, Line 1: xhci_acpi
I don't think this is dealing just with ACPI. […]
Done
https://review.coreboot.org/c/coreboot/+/41900/3/src/drivers/xhci/acpi/xhci_... File src/drivers/xhci/acpi/xhci_acpi.c:
https://review.coreboot.org/c/coreboot/+/41900/3/src/drivers/xhci/acpi/xhci_... PS3, Line 92: counts
How does counts get used? I see that handle_xhci_ext_cap() is setting it. […]
I added a comment, but see line 83.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes ......................................................................
Patch Set 4:
(1 comment)
I also converted the driver to use malloc to avoid any possible strange behavior with using static buffers.
https://review.coreboot.org/c/coreboot/+/41900/4/src/drivers/usb/pci_xhci/pc... File src/drivers/usb/pci_xhci/pci_xhci.c:
https://review.coreboot.org/c/coreboot/+/41900/4/src/drivers/usb/pci_xhci/pc... PS4, Line 1: GPL-2.0-only Should I be using GPL-2.0-or-later ?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41900/3/src/drivers/xhci/acpi/xhci_... File src/drivers/xhci/acpi/xhci_acpi.c:
https://review.coreboot.org/c/coreboot/+/41900/3/src/drivers/xhci/acpi/xhci_... PS3, Line 92: counts
I added a comment, but see line 83.
Aah! You are basically using it to generate unique HS/SS names. But is that really required? If ports live under different XHC* hierarchies, can't we just allocate names XHC0.HS0, XHC0.HS1, XHC1.HS0, XHC1.HS1?
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41900/3/src/drivers/xhci/acpi/xhci_... File src/drivers/xhci/acpi/xhci_acpi.c:
https://review.coreboot.org/c/coreboot/+/41900/3/src/drivers/xhci/acpi/xhci_... PS3, Line 92: counts
Aah! You are basically using it to generate unique HS/SS names. […]
That's exactly what this does. xhci_add_devices is called once per XHC.
We need to keep state for the callback to count the number of ports we have allocated. For Picasso at least, the USB3 descriptors only describe 1 port each.
xHCI SSDT generation xHCI Supported Protocol: Major: 0x2, Minor: 0x0, Protocol: 'USB ' Port Offset: 1, Port Count: 4 xHCI Supported Protocol: Major: 0x3, Minor: 0x10, Protocol: 'USB ' Port Offset: 5, Port Count: 1 xHCI Supported Protocol: Major: 0x3, Minor: 0x10, Protocol: 'USB ' Port Offset: 6, Port Count: 1 xHCI Supported Protocol: Major: 0x3, Minor: 0x10, Protocol: 'USB ' Port Offset: 7, Port Count: 1 xHCI Supported Protocol: Major: 0x3, Minor: 0x10, Protocol: 'USB ' Port Offset: 8, Port Count: 1
xHCI SSDT generation 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 EC returned error result code 2 PS2K: Bad resp from EC. Vivaldi disabled!
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41900/3/src/drivers/xhci/acpi/xhci_... File src/drivers/xhci/acpi/xhci_acpi.c:
https://review.coreboot.org/c/coreboot/+/41900/3/src/drivers/xhci/acpi/xhci_... PS3, Line 92: counts
That's exactly what this does. xhci_add_devices is called once per XHC. […]
Thanks Raul! That makes sense.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes ......................................................................
Uploaded patch set 5: Patch Set 4 was rebased.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes ......................................................................
Uploaded patch set 6: Patch Set 5 was rebased.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41900/5/src/drivers/usb/pci_xhci/ch... File src/drivers/usb/pci_xhci/chip.h:
https://review.coreboot.org/c/coreboot/+/41900/5/src/drivers/usb/pci_xhci/ch... PS5, Line 11: __XHCI_ACPI_CHIP_H__ __USB_XHCI_PCI_CHIP_H__
https://review.coreboot.org/c/coreboot/+/41900/5/src/drivers/usb/pci_xhci/pc... File src/drivers/usb/pci_xhci/pci_xhci.c:
https://review.coreboot.org/c/coreboot/+/41900/5/src/drivers/usb/pci_xhci/pc... PS5, Line 167: I was thinking some more about this. PCI drivers in coreboot add an array of PCI device IDs ending in 0 which is used to match the driver to a particular device. Why not use that here as well?
https://review.coreboot.org/c/coreboot/+/41900/6/src/drivers/usb/pci_xhci/pc... File src/drivers/usb/pci_xhci/pci_xhci.c:
https://review.coreboot.org/c/coreboot/+/41900/6/src/drivers/usb/pci_xhci/pc... PS6, Line 167: Sorry about the delay in review here. I was giving this some thought. All PCI drivers in coreboot provide a PCI device ID table with a list of IDs that the driver supports. XHCI controller is a property/attribute of the SoC itself and not mainboard dependent. I am thinking that: 1. We should add a list of PCI device IDs here that determines which XHCI controllers are supported by this driver. 2. DRIVERS_USB_PCI_XHCI should really be selected by the SoC. 3. This also keeps the organization within device tree consistent i.e. pci devices don't generally live under chip drivers of their own. 4. I think the reason you need the chip driver here is because of the wake property being passed into this driver. What do you think about having a SoC callback here soc_get_controller_wake_gpe(const struct device *controller, int *gpe) which returns -1 using a weak definition in this file and can be overridden by SoC if required. For platforms that have a fixed GPE for XHCI wake can return that GPE# directly in SoC code. For platforms that do not have a fixed GPE (e.g. picasso) can allow mainboards to set xhci_wake_gpe in devicetree under chip/soc/ and use that to return the right value for the callback.
Thoughts?
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41900/6/src/drivers/usb/pci_xhci/pc... File src/drivers/usb/pci_xhci/pci_xhci.c:
https://review.coreboot.org/c/coreboot/+/41900/6/src/drivers/usb/pci_xhci/pc... PS6, Line 167:
Sorry about the delay in review here. I was giving this some thought. […]
1. I didn't want to maintain another list of PCI_IDs. When any other SoC wants to use this driver, it requires then to touch this file which I think is unnecessary. This driver uses standard xHCI registers so it should support any xHCI controller. I guess that's the standard practice though. 2. Ok 3. I'm fine with that, just though it removed some of the magic and made it clearer to have it in the device tree. 4. If we add soc_get_controller_wake_gpe I think it should be in a more common location. I don't think that this driver should implement the weak method since this is pci_xhci and not soc code. I'm just not sure where to place it... Do you have a suggestion?
Another option to remove the chip from device tree: We could have the picasso/chip.c:enable_dev manually set the chip for the PCI device. Additionally it could construct the `chip_info`. We could have it call a `mainboard_get_device_gpe()` method to get the GPE. Though this seems like a lot of magic too...
Thoughts?
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/41900/6/src/drivers/usb/pci_xhci/pc... File src/drivers/usb/pci_xhci/pci_xhci.c:
https://review.coreboot.org/c/coreboot/+/41900/6/src/drivers/usb/pci_xhci/pc... PS6, Line 167:
I didn't want to maintain another list of PCI_IDs.
It is already done in a number of drivers in coreboot. This might one of the first ones outside of SoC directory, but I think it helps to keep the devicetree consistent.
If we add soc_get_controller_wake_gpe I think it should be in a more common location.
Do you mean the header file for declaration? Probably `device/xhci.h`?
I don't think that this driver should implement the weak method since this is pci_xhci and not soc code.
Weak definition that returns -1 can be placed here. It is for those SoCs that do not really have a GPE associated with XHCI wake (though I haven't really seen one). We can also completely skip the weak definition and always require SoC to provide this callback if using pci_xhci driver.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41900
to look at the new patch set (#7).
Change subject: drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes ......................................................................
drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes
We can use xhci_for_each_ext_cap to inspect the xHC so we generate the correct number of device nodes.
Scope (_SB.PCI0.PBRA) { Device (XHC1) { Name (_ADR, 0x0000000000000004) // _ADR: Address Method (_STA, 0, NotSerialized) // _STA: Status { Return (0x0F) }
Name (_PRW, Package (0x02) // _PRW: Power Resources for Wake { 0x1F, 0x03 }) Device (RHUB) { Name (_ADR, Zero) // _ADR: Address Device (HS01) { Name (_ADR, 0x01) // _ADR: Address }
Device (HS02) { Name (_ADR, 0x02) // _ADR: Address }
Device (SS01) { Name (_ADR, 0x03) // _ADR: Address } }
Name (_S0W, Zero) // _S0W: S0 Device Wake State Name (_S3W, 0x04) // _S3W: S3 Device Wake State Name (_S4W, 0x04) // _S4W: S4 Device Wake State } }
BUG=b:154756391 TEST=Boot trembyle and look at ACPI table. See all xHCI nodes.
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I44ebaef342e45923bc181ceebef882358d33f0d1 --- A src/drivers/usb/pci_xhci/Kconfig A src/drivers/usb/pci_xhci/Makefile.inc A src/drivers/usb/pci_xhci/pci_xhci.c A src/drivers/usb/pci_xhci/pci_xhci.h 4 files changed, 206 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/41900/7
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/c/coreboot/+/41900/5/src/drivers/usb/pci_xhci/ch... File src/drivers/usb/pci_xhci/chip.h:
https://review.coreboot.org/c/coreboot/+/41900/5/src/drivers/usb/pci_xhci/ch... PS5, Line 11: __XHCI_ACPI_CHIP_H__
__USB_XHCI_PCI_CHIP_H__
Done
https://review.coreboot.org/c/coreboot/+/41900/4/src/drivers/usb/pci_xhci/pc... File src/drivers/usb/pci_xhci/pci_xhci.c:
https://review.coreboot.org/c/coreboot/+/41900/4/src/drivers/usb/pci_xhci/pc... PS4, Line 1: GPL-2.0-only
Should I be using GPL-2. […]
Ack
https://review.coreboot.org/c/coreboot/+/41900/5/src/drivers/usb/pci_xhci/pc... File src/drivers/usb/pci_xhci/pci_xhci.c:
https://review.coreboot.org/c/coreboot/+/41900/5/src/drivers/usb/pci_xhci/pc... PS5, Line 167:
I was thinking some more about this. […]
Done
https://review.coreboot.org/c/coreboot/+/41900/6/src/drivers/usb/pci_xhci/pc... File src/drivers/usb/pci_xhci/pci_xhci.c:
https://review.coreboot.org/c/coreboot/+/41900/6/src/drivers/usb/pci_xhci/pc... PS6, Line 167:
I didn't want to maintain another list of PCI_IDs. […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes ......................................................................
Patch Set 7: Code-Review+2
(4 comments)
https://review.coreboot.org/c/coreboot/+/41900/7/src/drivers/usb/pci_xhci/pc... File src/drivers/usb/pci_xhci/pci_xhci.c:
https://review.coreboot.org/c/coreboot/+/41900/7/src/drivers/usb/pci_xhci/pc... PS7, Line 134: 3 SLP_TYP_S3
https://review.coreboot.org/c/coreboot/+/41900/7/src/drivers/usb/pci_xhci/pc... PS7, Line 135: 0 ACPI_DEVICE_SLEEP_D0
https://review.coreboot.org/c/coreboot/+/41900/7/src/drivers/usb/pci_xhci/pc... PS7, Line 136: 4 ACPI_DEVICE_SLEEP_D3_COLD
https://review.coreboot.org/c/coreboot/+/41900/7/src/drivers/usb/pci_xhci/pc... PS7, Line 137: 4 ACPI_DEVICE_SLEEP_D3_COLD
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Aaron Durbin, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/41900
to look at the new patch set (#8).
Change subject: drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes ......................................................................
drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes
We can use xhci_for_each_ext_cap to inspect the xHC so we generate the correct number of device nodes.
Scope (_SB.PCI0.PBRA) { Device (XHC1) { Name (_ADR, 0x0000000000000004) // _ADR: Address Method (_STA, 0, NotSerialized) // _STA: Status { Return (0x0F) }
Name (_PRW, Package (0x02) // _PRW: Power Resources for Wake { 0x1F, 0x03 }) Device (RHUB) { Name (_ADR, Zero) // _ADR: Address Device (HS01) { Name (_ADR, 0x01) // _ADR: Address }
Device (HS02) { Name (_ADR, 0x02) // _ADR: Address }
Device (SS01) { Name (_ADR, 0x03) // _ADR: Address } }
Name (_S0W, Zero) // _S0W: S0 Device Wake State Name (_S3W, 0x04) // _S3W: S3 Device Wake State Name (_S4W, 0x04) // _S4W: S4 Device Wake State } }
BUG=b:154756391 TEST=Boot trembyle and look at ACPI table. See all xHCI nodes.
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I44ebaef342e45923bc181ceebef882358d33f0d1 --- A src/drivers/usb/pci_xhci/Kconfig A src/drivers/usb/pci_xhci/Makefile.inc A src/drivers/usb/pci_xhci/pci_xhci.c A src/drivers/usb/pci_xhci/pci_xhci.h 4 files changed, 206 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/00/41900/8
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes ......................................................................
Patch Set 8:
(4 comments)
https://review.coreboot.org/c/coreboot/+/41900/7/src/drivers/usb/pci_xhci/pc... File src/drivers/usb/pci_xhci/pci_xhci.c:
https://review.coreboot.org/c/coreboot/+/41900/7/src/drivers/usb/pci_xhci/pc... PS7, Line 134: 3
SLP_TYP_S3
Done
https://review.coreboot.org/c/coreboot/+/41900/7/src/drivers/usb/pci_xhci/pc... PS7, Line 135: 0
ACPI_DEVICE_SLEEP_D0
Done
https://review.coreboot.org/c/coreboot/+/41900/7/src/drivers/usb/pci_xhci/pc... PS7, Line 136: 4
ACPI_DEVICE_SLEEP_D3_COLD
Done
https://review.coreboot.org/c/coreboot/+/41900/7/src/drivers/usb/pci_xhci/pc... PS7, Line 137: 4
ACPI_DEVICE_SLEEP_D3_COLD
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes ......................................................................
Patch Set 8: Code-Review+2
Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41900 )
Change subject: drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes ......................................................................
drivers/usb/pci_xhci: Add a driver to generate xHCI ACPI nodes
We can use xhci_for_each_ext_cap to inspect the xHC so we generate the correct number of device nodes.
Scope (_SB.PCI0.PBRA) { Device (XHC1) { Name (_ADR, 0x0000000000000004) // _ADR: Address Method (_STA, 0, NotSerialized) // _STA: Status { Return (0x0F) }
Name (_PRW, Package (0x02) // _PRW: Power Resources for Wake { 0x1F, 0x03 }) Device (RHUB) { Name (_ADR, Zero) // _ADR: Address Device (HS01) { Name (_ADR, 0x01) // _ADR: Address }
Device (HS02) { Name (_ADR, 0x02) // _ADR: Address }
Device (SS01) { Name (_ADR, 0x03) // _ADR: Address } }
Name (_S0W, Zero) // _S0W: S0 Device Wake State Name (_S3W, 0x04) // _S3W: S3 Device Wake State Name (_S4W, 0x04) // _S4W: S4 Device Wake State } }
BUG=b:154756391 TEST=Boot trembyle and look at ACPI table. See all xHCI nodes.
Signed-off-by: Raul E Rangel rrangel@chromium.org Change-Id: I44ebaef342e45923bc181ceebef882358d33f0d1 Reviewed-on: https://review.coreboot.org/c/coreboot/+/41900 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- A src/drivers/usb/pci_xhci/Kconfig A src/drivers/usb/pci_xhci/Makefile.inc A src/drivers/usb/pci_xhci/pci_xhci.c A src/drivers/usb/pci_xhci/pci_xhci.h 4 files changed, 206 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/drivers/usb/pci_xhci/Kconfig b/src/drivers/usb/pci_xhci/Kconfig new file mode 100644 index 0000000..715af5b --- /dev/null +++ b/src/drivers/usb/pci_xhci/Kconfig @@ -0,0 +1,6 @@ +config DRIVERS_USB_PCI_XHCI + def_bool n + depends on HAVE_ACPI_TABLES + select XHCI_UTILS + help + PCI driver that generates ACPI nodes for an xHCI compatible controller. diff --git a/src/drivers/usb/pci_xhci/Makefile.inc b/src/drivers/usb/pci_xhci/Makefile.inc new file mode 100644 index 0000000..73f6b06 --- /dev/null +++ b/src/drivers/usb/pci_xhci/Makefile.inc @@ -0,0 +1 @@ +ramstage-$(CONFIG_DRIVERS_USB_PCI_XHCI) += pci_xhci.c diff --git a/src/drivers/usb/pci_xhci/pci_xhci.c b/src/drivers/usb/pci_xhci/pci_xhci.c new file mode 100644 index 0000000..cbd3c34 --- /dev/null +++ b/src/drivers/usb/pci_xhci/pci_xhci.c @@ -0,0 +1,184 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include "pci_xhci.h" +#include <acpi/acpigen.h> +#include <console/console.h> +#include <device/pci.h> +#include <device/pci_ids.h> +#include <device/xhci.h> +#include <stdlib.h> +#include <string.h> + +#define PCI_XHCI_CLASSCODE 0x0c0330 /* USB3.0 xHCI controller */ + +static unsigned int controller_count; +static const struct device_operations xhci_pci_ops; + +struct port_counts { + unsigned int high_speed; + unsigned int super_speed; +}; + +__weak enum cb_err pci_xhci_get_wake_gpe(const struct device *dev, int *gpe) +{ + *gpe = -1; + return CB_SUCCESS; +} + +static const char *xhci_acpi_name(const struct device *dev) +{ + char *name; + unsigned int port_id; + + if (dev->path.type == DEVICE_PATH_USB) { + /* Ports index start at 1 */ + port_id = dev->path.usb.port_id + 1; + + switch (dev->path.usb.port_type) { + case 0: + return "RHUB"; + case 2: + name = malloc(ACPI_NAME_BUFFER_SIZE); + snprintf(name, ACPI_NAME_BUFFER_SIZE, "HS%02d", port_id); + name[4] = '\0'; + + return name; + case 3: + name = malloc(ACPI_NAME_BUFFER_SIZE); + snprintf(name, ACPI_NAME_BUFFER_SIZE, "SS%02d", port_id); + name[4] = '\0'; + return name; + } + } else if (dev->ops == &xhci_pci_ops) { + return dev->name; + } + + printk(BIOS_ERR, "%s: Unknown device %s\n", __func__, dev_path(dev)); + return NULL; +} + +static void handle_xhci_ext_cap(void *context, const struct xhci_ext_cap *cap) +{ + const struct xhci_supported_protocol *data; + const char *format; + char buf[16]; + struct port_counts *counts = context; + unsigned int *dev_num; + + if (cap->cap_id != XHCI_ECP_CAP_ID_SUPP) + return; + + data = &cap->supported_protocol; + + xhci_print_supported_protocol(data); + + if (memcmp(data->name, "USB ", 4)) { + printk(BIOS_INFO, "%s: Unknown Protocol: %.*s\n", __func__, + (int)sizeof(data->name), data->name); + return; + } + + if (data->major_rev == 3) { + format = "SS%02d"; + dev_num = &counts->super_speed; + } else if (data->major_rev == 2) { + format = "HS%02d"; + dev_num = &counts->high_speed; + } else { + printk(BIOS_INFO, "%s: Unknown USB Version: %#x\n", __func__, data->major_rev); + return; + } + + for (unsigned int i = 0; i < data->port_count; ++i) { + snprintf(buf, sizeof(buf), format, ++(*dev_num)); + acpigen_write_device(buf); + acpigen_write_name_byte("_ADR", data->port_offset + i); + acpigen_pop_len(); + } +} + +static void xhci_add_devices(const struct device *dev) +{ + /* Used by the callback to track how many ports have been seen. */ + struct port_counts counts = {0, 0}; + + acpigen_write_device("RHUB"); + acpigen_write_name_integer("_ADR", 0x00000000); + + xhci_for_each_ext_cap(dev, &counts, handle_xhci_ext_cap); + + acpigen_pop_len(); +} + +static void xhci_fill_ssdt(const struct device *dev) +{ + int gpe; + + printk(BIOS_DEBUG, "xHCI SSDT generation\n"); + + acpigen_write_scope(acpi_device_scope(dev)); + acpigen_write_device(acpi_device_name(dev)); + + acpigen_write_ADR_pci_device(dev); + acpigen_write_name_string("_DDN", "xHC - Extensible Host Controller"); + acpigen_write_STA(acpi_device_status(dev)); + + if (pci_xhci_get_wake_gpe(dev, &gpe) == CB_SUCCESS) { + printk(BIOS_DEBUG, "%s: Got GPE %d for %s\n", __func__, gpe, dev_path(dev)); + } else { + printk(BIOS_ERR, "%s: Error getting GPE for : %s\n", __func__, dev_path(dev)); + gpe = -1; + } + + if (gpe > 0) { + acpigen_write_PRW(gpe, SLP_TYP_S3); + acpigen_write_name_integer("_S0W", ACPI_DEVICE_SLEEP_D0); + acpigen_write_name_integer("_S3W", ACPI_DEVICE_SLEEP_D3_COLD); + acpigen_write_name_integer("_S4W", ACPI_DEVICE_SLEEP_D3_COLD); + } + + xhci_add_devices(dev); + + acpigen_pop_len(); + acpigen_pop_len(); +} + +static void xhci_enable(struct device *dev) +{ + char *name; + uint32_t class = pci_read_config32(dev, PCI_CLASS_REVISION); + /* Class code, the upper 3 bytes of PCI_CLASS_REVISION. */ + class >>= 8; + + if (class != PCI_XHCI_CLASSCODE) { + printk(BIOS_ERR, "Incorrect xHCI class code: %#x\n", class); + dev->enabled = 0; + return; + } + + name = malloc(ACPI_NAME_BUFFER_SIZE); + snprintf(name, ACPI_NAME_BUFFER_SIZE, "XHC%d", controller_count++); + dev->name = name; +} + +static const struct device_operations xhci_pci_ops = { + .read_resources = pci_dev_read_resources, + .set_resources = pci_dev_set_resources, + .enable_resources = pci_dev_enable_resources, + .init = pci_dev_init, + .scan_bus = scan_static_bus, + .enable = xhci_enable, + .ops_pci = &pci_dev_ops_pci, + .acpi_fill_ssdt = xhci_fill_ssdt, + .acpi_name = xhci_acpi_name, +}; + +static const unsigned short amd_pci_device_ids[] = { + 0 +}; + +static const struct pci_driver xhci_pci_driver __pci_driver = { + .ops = &xhci_pci_ops, + .vendor = PCI_VENDOR_ID_AMD, + .devices = amd_pci_device_ids, +}; diff --git a/src/drivers/usb/pci_xhci/pci_xhci.h b/src/drivers/usb/pci_xhci/pci_xhci.h new file mode 100644 index 0000000..a923351 --- /dev/null +++ b/src/drivers/usb/pci_xhci/pci_xhci.h @@ -0,0 +1,15 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __DRIVERS_USB_PCI_XHCI__ +#define __DRIVERS_USB_PCI_XHCI__ + +#include <commonlib/bsd/cb_err.h> +#include <device/device.h> + +/* + * Returns the wake GPE for the Extensible Host Controller. + * Set gpe to -1 if there is no GPE is available. + */ +enum cb_err pci_xhci_get_wake_gpe(const struct device *dev, int *gpe); + +#endif /* __DRIVERS_USB_PCI_XHCI__ */
-
Duncan Laurie (Code Review) -
Felix Held (Code Review)
-
Furquan Shaikh (Code Review)
-
Paul Menzel (Code Review)
-
Raul Rangel (Code Review)