Change in coreboot[master]: soc/intel/common: Fix/cleanup USB4 PCIe virtual/generic driver
Duncan Laurie has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46544 )
Change subject: soc/intel/common: Fix/cleanup USB4 PCIe virtual/generic driver ......................................................................
soc/intel/common: Fix/cleanup USB4 PCIe virtual/generic driver
This driver is for the root port device and needs to reference the parent device for its ACPI scope. Similarly for the debug output it needs to use the parent device, and fall back to the chip name if config->desc is not provided in the devicetree.
The UID property is removed. This value is not the same as the port number; according to some docs it should be unique but it is not fully clear what it should be tied to. Regardless, it is not used by the Thunderbolt driver in the kernel.
I also renamed some functions/structures to be clear that this is just an ACPI driver for the PCIe root port and not a driver for the root port itself. As part of this I removed the PCI based resource operations and the scan bus function since this device does not have children itself.
Finally I added a detaled comment with an example describing what the driver is for and what properties it generates.
TEST=boot on volteer and ensure the USB4 root port device and properties are added to the SSDT as described by the comment in chip.h.
Signed-off-by: Duncan Laurie dlaurie@google.com Change-Id: Id6069a0fb7a0fc6836ddff1dbeca5915e444ee18 --- M src/soc/intel/common/block/usb4/chip.h M src/soc/intel/common/block/usb4/pcie.c 2 files changed, 67 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/46544/1
diff --git a/src/soc/intel/common/block/usb4/chip.h b/src/soc/intel/common/block/usb4/chip.h index f2eee4d..41f4ed6 100644 --- a/src/soc/intel/common/block/usb4/chip.h +++ b/src/soc/intel/common/block/usb4/chip.h @@ -3,10 +3,62 @@ #ifndef __DRIVERS_INTEL_USB4_PCIE_H__ #define __DRIVERS_INTEL_USB4_PCIE_H__
+/* + * This virtual generic driver provides the ACPI properties for an + * Intel USB4/TBT PCIe Root Port which is already declared in the DSDT, + * + * The associated USB4 port number is obtained from the generic ID and + * the related host interface (DMA) device is provided by the devicetree. + * + * The "ExternalFacingPort", and "HotPlugSupportInD3" properties are defined at + * https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-r... + * + * Example: PCIe Root Port 2 via USB4 host interface 1 port 0: + * + * device pci 0d.3 alias tbt_dma1 on end # _SB.PCI0.TDM1 + * device pci 07.2 alias tbt_pcie_rp2 on # _SB.PCI0.TRP2 + * chip soc/intel/common/block/usb4 + * use tbt_dma1 as usb4_port # USB4 host interface for this root port + * device generic 0 on end # USB4 port number on this host interface + * end + * end + * + * The host interface and PCIe Root Port are declared in the DSDT: + * + * Scope (_SB.PCI0) { + * Device (TDM1) { + * Name (_ADR, 0x000d0003) + * } + * Device (TRP2) { + * Name (_ADR, 0x00070002) + * } + * } + * + * This driver will provide the following properties in the SSDT: + * + * Scope (_SB.PCI0.TRP2) { + * Name (_DSD, Package () { + * ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), + * Package () { + * Package () { "usb4-host-interface", _SB.PCI0.TDM1 }, + * Package () { "usb4-port-number", 0 }, + * }, + * ToUUID ("6211e2c0-58a3-4af3-90e1-927a4e0c55a4"), + * Package () { + * Package () { "HotPlugSupportInD3", 1 }, + * }, + * ToUUID ("efcc06cc-73ac-4bc3-bff0-76143807c389"), + * Package () { + * Package () { "ExternalFacingPort", 1 }, + * }, + * } + * } + */ + struct soc_intel_common_block_usb4_config { const char *desc;
- /* Pointer to USB4 device that this PCIe root port is routed to. */ + /* USB4 host interface (DMA device) that this PCIe root port is routed to. */ DEVTREE_CONST struct device *usb4_port; };
diff --git a/src/soc/intel/common/block/usb4/pcie.c b/src/soc/intel/common/block/usb4/pcie.c index e37d5f4..eae9027 100644 --- a/src/soc/intel/common/block/usb4/pcie.c +++ b/src/soc/intel/common/block/usb4/pcie.c @@ -4,19 +4,14 @@ #include <console/console.h> #include <device/device.h> #include <device/path.h> -#include <device/pci.h> -#include <device/pci_def.h> -#include <device/pci_ids.h> -#include <stdlib.h> #include <string.h> -#include <types.h> #include "chip.h"
#define PCI_HOTPLUG_IN_D3_UUID "6211E2C0-58A3-4AF3-90E1-927A4E0C55A4" #define PCI_EXTERNAL_PORT_UUID "EFCC06CC-73AC-4BC3-BFF0-76143807C389"
#if CONFIG(HAVE_ACPI_TABLES) -static void usb4_pcie_fill_ssdt(const struct device *dev) +static void usb4_pcie_acpi_fill_ssdt(const struct device *dev) { const struct soc_intel_common_block_usb4_config *config; const struct device *parent; @@ -43,14 +38,15 @@ /* Get ACPI path to USB4 device. */ usb4_path = acpi_device_path(config->usb4_port); if (!usb4_path) { - printk(BIOS_ERR, "%s: Unable to find ACPI path for usb4_port\n", __func__); + printk(BIOS_ERR, "%s: Unable to find ACPI path for usb4_port %s\n", + __func__, dev_path(config->usb4_port)); return; }
usb4_path = strdup(usb4_path); port_id = dev->path.generic.id;
- acpigen_write_scope(acpi_device_path(dev)); + acpigen_write_scope(acpi_device_path(parent));
/* Add pointer to USB4 port controller. */ dsd = acpi_dp_new_table("_DSD"); @@ -65,33 +61,31 @@ /* Indicate that port is external. */ pkg = acpi_dp_new_table(PCI_EXTERNAL_PORT_UUID); acpi_dp_add_integer(pkg, "ExternalFacingPort", 1); - acpi_dp_add_integer(pkg, "UID", port_id);
acpi_dp_add_package(dsd, pkg); acpi_dp_write(dsd);
acpigen_pop_len(); /* Scope */
- printk(BIOS_INFO, "%s: %s at %s\n", acpi_device_path(dev), config->desc, dev_path(dev)); + printk(BIOS_INFO, "%s: %s at %s\n", acpi_device_path(parent), + config->desc ? : dev->chip_ops->name, dev_path(parent)); } #endif
-static struct device_operations usb4_dev_ops = { - .read_resources = pci_dev_read_resources, - .set_resources = pci_dev_set_resources, - .enable_resources = pci_dev_enable_resources, - .scan_bus = scan_static_bus, +static struct device_operations usb4_pcie_acpi_dev_ops = { + .read_resources = noop_read_resources, + .set_resources = noop_set_resources, #if CONFIG(HAVE_ACPI_TABLES) - .acpi_fill_ssdt = usb4_pcie_fill_ssdt, + .acpi_fill_ssdt = usb4_pcie_acpi_fill_ssdt, #endif };
-static void pcie_enable(struct device *dev) +static void usb4_pcie_acpi_enable(struct device *dev) { - dev->ops = &usb4_dev_ops; + dev->ops = &usb4_pcie_acpi_dev_ops; }
struct chip_operations soc_intel_common_block_usb4_ops = { - CHIP_NAME("Intel USB4 Root Port") - .enable_dev = pcie_enable + CHIP_NAME("Intel USB4 PCIe Root Port") + .enable_dev = usb4_pcie_acpi_enable };
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46544 )
Change subject: soc/intel/common: Fix/cleanup USB4 PCIe virtual/generic driver ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46544/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46544/1//COMMIT_MSG@7 PS1, Line 7: cleanup clean up
https://review.coreboot.org/c/coreboot/+/46544/1//COMMIT_MSG@24 PS1, Line 24: detaled detailed
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46544 )
Change subject: soc/intel/common: Fix/cleanup USB4 PCIe virtual/generic driver ......................................................................
Patch Set 1: Code-Review+2
Hello build bot (Jenkins), Furquan Shaikh, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46544
to look at the new patch set (#2).
Change subject: soc/intel/common: Fix/clean up USB4 PCIe virtual/generic driver ......................................................................
soc/intel/common: Fix/clean up USB4 PCIe virtual/generic driver
This driver is for the root port device and needs to reference the parent device for its ACPI scope. Similarly for the debug output it needs to use the parent device, and fall back to the chip name if config->desc is not provided in the devicetree.
The UID property is removed. This value is not the same as the port number; according to some docs it should be unique but it is not fully clear what it should be tied to. Regardless, it is not used by the Thunderbolt driver in the kernel.
I also renamed some functions/structures to be clear that this is just an ACPI driver for the PCIe root port and not a driver for the root port itself. As part of this I removed the PCI based resource operations and the scan bus function since this device does not have children itself.
Finally I added a detailed comment with an example describing what the driver is for and what properties it generates.
TEST=boot on volteer and ensure the USB4 root port device and properties are added to the SSDT as described by the comment in chip.h.
Signed-off-by: Duncan Laurie dlaurie@google.com Change-Id: Id6069a0fb7a0fc6836ddff1dbeca5915e444ee18 --- M src/soc/intel/common/block/usb4/chip.h M src/soc/intel/common/block/usb4/pcie.c 2 files changed, 67 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/44/46544/2
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46544 )
Change subject: soc/intel/common: Fix/clean up USB4 PCIe virtual/generic driver ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46544/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46544/1//COMMIT_MSG@7 PS1, Line 7: cleanup
clean up
Done
https://review.coreboot.org/c/coreboot/+/46544/1//COMMIT_MSG@24 PS1, Line 24: detaled
detailed
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46544 )
Change subject: soc/intel/common: Fix/clean up USB4 PCIe virtual/generic driver ......................................................................
Patch Set 2: Code-Review+2
Duncan Laurie has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46544 )
Change subject: soc/intel/common: Fix/clean up USB4 PCIe virtual/generic driver ......................................................................
soc/intel/common: Fix/clean up USB4 PCIe virtual/generic driver
This driver is for the root port device and needs to reference the parent device for its ACPI scope. Similarly for the debug output it needs to use the parent device, and fall back to the chip name if config->desc is not provided in the devicetree.
The UID property is removed. This value is not the same as the port number; according to some docs it should be unique but it is not fully clear what it should be tied to. Regardless, it is not used by the Thunderbolt driver in the kernel.
I also renamed some functions/structures to be clear that this is just an ACPI driver for the PCIe root port and not a driver for the root port itself. As part of this I removed the PCI based resource operations and the scan bus function since this device does not have children itself.
Finally I added a detailed comment with an example describing what the driver is for and what properties it generates.
TEST=boot on volteer and ensure the USB4 root port device and properties are added to the SSDT as described by the comment in chip.h.
Signed-off-by: Duncan Laurie dlaurie@google.com Change-Id: Id6069a0fb7a0fc6836ddff1dbeca5915e444ee18 Reviewed-on: https://review.coreboot.org/c/coreboot/+/46544 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/common/block/usb4/chip.h M src/soc/intel/common/block/usb4/pcie.c 2 files changed, 67 insertions(+), 21 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/soc/intel/common/block/usb4/chip.h b/src/soc/intel/common/block/usb4/chip.h index f2eee4d..41f4ed6 100644 --- a/src/soc/intel/common/block/usb4/chip.h +++ b/src/soc/intel/common/block/usb4/chip.h @@ -3,10 +3,62 @@ #ifndef __DRIVERS_INTEL_USB4_PCIE_H__ #define __DRIVERS_INTEL_USB4_PCIE_H__
+/* + * This virtual generic driver provides the ACPI properties for an + * Intel USB4/TBT PCIe Root Port which is already declared in the DSDT, + * + * The associated USB4 port number is obtained from the generic ID and + * the related host interface (DMA) device is provided by the devicetree. + * + * The "ExternalFacingPort", and "HotPlugSupportInD3" properties are defined at + * https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-r... + * + * Example: PCIe Root Port 2 via USB4 host interface 1 port 0: + * + * device pci 0d.3 alias tbt_dma1 on end # _SB.PCI0.TDM1 + * device pci 07.2 alias tbt_pcie_rp2 on # _SB.PCI0.TRP2 + * chip soc/intel/common/block/usb4 + * use tbt_dma1 as usb4_port # USB4 host interface for this root port + * device generic 0 on end # USB4 port number on this host interface + * end + * end + * + * The host interface and PCIe Root Port are declared in the DSDT: + * + * Scope (_SB.PCI0) { + * Device (TDM1) { + * Name (_ADR, 0x000d0003) + * } + * Device (TRP2) { + * Name (_ADR, 0x00070002) + * } + * } + * + * This driver will provide the following properties in the SSDT: + * + * Scope (_SB.PCI0.TRP2) { + * Name (_DSD, Package () { + * ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), + * Package () { + * Package () { "usb4-host-interface", _SB.PCI0.TDM1 }, + * Package () { "usb4-port-number", 0 }, + * }, + * ToUUID ("6211e2c0-58a3-4af3-90e1-927a4e0c55a4"), + * Package () { + * Package () { "HotPlugSupportInD3", 1 }, + * }, + * ToUUID ("efcc06cc-73ac-4bc3-bff0-76143807c389"), + * Package () { + * Package () { "ExternalFacingPort", 1 }, + * }, + * } + * } + */ + struct soc_intel_common_block_usb4_config { const char *desc;
- /* Pointer to USB4 device that this PCIe root port is routed to. */ + /* USB4 host interface (DMA device) that this PCIe root port is routed to. */ DEVTREE_CONST struct device *usb4_port; };
diff --git a/src/soc/intel/common/block/usb4/pcie.c b/src/soc/intel/common/block/usb4/pcie.c index e37d5f4..eae9027 100644 --- a/src/soc/intel/common/block/usb4/pcie.c +++ b/src/soc/intel/common/block/usb4/pcie.c @@ -4,19 +4,14 @@ #include <console/console.h> #include <device/device.h> #include <device/path.h> -#include <device/pci.h> -#include <device/pci_def.h> -#include <device/pci_ids.h> -#include <stdlib.h> #include <string.h> -#include <types.h> #include "chip.h"
#define PCI_HOTPLUG_IN_D3_UUID "6211E2C0-58A3-4AF3-90E1-927A4E0C55A4" #define PCI_EXTERNAL_PORT_UUID "EFCC06CC-73AC-4BC3-BFF0-76143807C389"
#if CONFIG(HAVE_ACPI_TABLES) -static void usb4_pcie_fill_ssdt(const struct device *dev) +static void usb4_pcie_acpi_fill_ssdt(const struct device *dev) { const struct soc_intel_common_block_usb4_config *config; const struct device *parent; @@ -43,14 +38,15 @@ /* Get ACPI path to USB4 device. */ usb4_path = acpi_device_path(config->usb4_port); if (!usb4_path) { - printk(BIOS_ERR, "%s: Unable to find ACPI path for usb4_port\n", __func__); + printk(BIOS_ERR, "%s: Unable to find ACPI path for usb4_port %s\n", + __func__, dev_path(config->usb4_port)); return; }
usb4_path = strdup(usb4_path); port_id = dev->path.generic.id;
- acpigen_write_scope(acpi_device_path(dev)); + acpigen_write_scope(acpi_device_path(parent));
/* Add pointer to USB4 port controller. */ dsd = acpi_dp_new_table("_DSD"); @@ -65,33 +61,31 @@ /* Indicate that port is external. */ pkg = acpi_dp_new_table(PCI_EXTERNAL_PORT_UUID); acpi_dp_add_integer(pkg, "ExternalFacingPort", 1); - acpi_dp_add_integer(pkg, "UID", port_id);
acpi_dp_add_package(dsd, pkg); acpi_dp_write(dsd);
acpigen_pop_len(); /* Scope */
- printk(BIOS_INFO, "%s: %s at %s\n", acpi_device_path(dev), config->desc, dev_path(dev)); + printk(BIOS_INFO, "%s: %s at %s\n", acpi_device_path(parent), + config->desc ? : dev->chip_ops->name, dev_path(parent)); } #endif
-static struct device_operations usb4_dev_ops = { - .read_resources = pci_dev_read_resources, - .set_resources = pci_dev_set_resources, - .enable_resources = pci_dev_enable_resources, - .scan_bus = scan_static_bus, +static struct device_operations usb4_pcie_acpi_dev_ops = { + .read_resources = noop_read_resources, + .set_resources = noop_set_resources, #if CONFIG(HAVE_ACPI_TABLES) - .acpi_fill_ssdt = usb4_pcie_fill_ssdt, + .acpi_fill_ssdt = usb4_pcie_acpi_fill_ssdt, #endif };
-static void pcie_enable(struct device *dev) +static void usb4_pcie_acpi_enable(struct device *dev) { - dev->ops = &usb4_dev_ops; + dev->ops = &usb4_pcie_acpi_dev_ops; }
struct chip_operations soc_intel_common_block_usb4_ops = { - CHIP_NAME("Intel USB4 Root Port") - .enable_dev = pcie_enable + CHIP_NAME("Intel USB4 PCIe Root Port") + .enable_dev = usb4_pcie_acpi_enable };
-
Duncan Laurie (Code Review) -
Furquan Shaikh (Code Review)
-
Paul Menzel (Code Review)