Change in coreboot[master]: device: Allow virtual/generic devices under PCI in devicetree
Duncan Laurie has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46542 )
Change subject: device: Allow virtual/generic devices under PCI in devicetree ......................................................................
device: Allow virtual/generic devices under PCI in devicetree
This change allows a generic device to be described in the devicetree under a PCI device, such as a root port.
Previously any device under a PCI device was expected to also be a PCI device and that does not allow for a virtual/generic device to be present, for example to provide ACPI properties for a root port.
The changes are: - Ignore non-PCI devices found under a PCI device when scanning and do not print an error for each devfn scanned. - Don't treat non-PCI devices as leftover and remove them, instead enable them as a static device. - Don't attempt to configure a static device in the tree that is not a PCIe device type.
With these changes it is now possible to have a generic device under a PCI device, for example in a USB4/TBT root port (PCIe hotplug device) this generic device will add ACPI properties for the PCIe tunnel routed to the external port:
device pci 07.0 on chip soc/intel/common/block/pcie device generic 0 on end end end
TEST=boot on volteer with the USB4 root port devices in chipset.cb and ensure they are enabled properly and there are no errors printed in the coreboot log, and that the device properties are created in the SSDT.
Signed-off-by: Duncan Laurie dlaurie@google.com Change-Id: I56a491808067dc862a7adfd46852f0bd6b41cd95 --- M src/device/pci_device.c M src/device/pciexp_device.c 2 files changed, 20 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/46542/1
diff --git a/src/device/pci_device.c b/src/device/pci_device.c index ce3e509..691ad6b 100644 --- a/src/device/pci_device.c +++ b/src/device/pci_device.c @@ -1006,16 +1006,11 @@
prev = &bus->children; for (dev = bus->children; dev; dev = dev->sibling) { - if (dev->path.type == DEVICE_PATH_PCI) { - if (dev->path.pci.devfn == devfn) { - /* Unlink from the list. */ - *prev = dev->sibling; - dev->sibling = NULL; - break; - } - } else { - printk(BIOS_ERR, "child %s not a PCI device\n", - dev_path(dev)); + if (dev->path.type == DEVICE_PATH_PCI && dev->path.pci.devfn == devfn) { + /* Unlink from the list. */ + *prev = dev->sibling; + dev->sibling = NULL; + break; } prev = &dev->sibling; } @@ -1283,6 +1278,16 @@
prev = &bus->children; for (dev = bus->children; dev; dev = dev->sibling) { + + /* + * If static device is not PCI then enable it here and don't + * treat it as a leftover device. + */ + if (dev->path.type != DEVICE_PATH_PCI) { + enable_static_device(dev); + continue; + } + /* * The device is only considered leftover if it is not hidden * and it has a Vendor ID of 0 (the default for a device that diff --git a/src/device/pciexp_device.c b/src/device/pciexp_device.c index f04d865..4b723ac 100644 --- a/src/device/pciexp_device.c +++ b/src/device/pciexp_device.c @@ -180,6 +180,8 @@
for (bus = dev->link_list ; bus ; bus = bus->next) { for (child = bus->children; child; child = child->sibling) { + if (child->path.type != DEVICE_PATH_PCI) + continue; pciexp_configure_ltr(child); if (child->ops && child->ops->scan_bus) pciexp_enable_ltr(child); @@ -443,6 +445,9 @@ struct device *root = dev->bus->dev; unsigned int root_cap, cap;
+ if (dev->path.type != DEVICE_PATH_PCI) + return; + cap = pci_find_capability(dev, PCI_CAP_ID_PCIE); if (!cap) return;
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46542 )
Change subject: device: Allow virtual/generic devices under PCI in devicetree ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46542/1/src/device/pciexp_device.c File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/46542/1/src/device/pciexp_device.c@... PS1, Line 486: Should the check for child->path.type be added here so that this function doesn't even look at child->path.pci.devfn?
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46542
to look at the new patch set (#2).
Change subject: device: Allow virtual/generic devices under PCI in devicetree ......................................................................
device: Allow virtual/generic devices under PCI in devicetree
This change allows a generic device to be described in the devicetree under a PCI device, such as a root port.
Previously any device under a PCI device was expected to also be a PCI device and that does not allow for a virtual/generic device to be present, for example to provide ACPI properties for a root port.
The changes are: - Ignore non-PCI devices found under a PCI device when scanning and do not print an error for each devfn scanned. - Don't treat non-PCI devices as leftover and remove them, instead enable them as a static device. - Don't attempt to configure a static device in the tree that is not a PCIe device type.
With these changes it is now possible to have a generic device under a PCI device, for example in a USB4/TBT root port (PCIe hotplug device) this generic device will add ACPI properties for the PCIe tunnel routed to the external port:
device pci 07.0 on chip soc/intel/common/block/pcie device generic 0 on end end end
TEST=boot on volteer with the USB4 root port devices in chipset.cb and ensure they are enabled properly and there are no errors printed in the coreboot log, and that the device properties are created in the SSDT.
Signed-off-by: Duncan Laurie dlaurie@google.com Change-Id: I56a491808067dc862a7adfd46852f0bd6b41cd95 --- M src/device/pci_device.c M src/device/pciexp_device.c 2 files changed, 19 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/46542/2
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46542 )
Change subject: device: Allow virtual/generic devices under PCI in devicetree ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46542/1/src/device/pciexp_device.c File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/46542/1/src/device/pciexp_device.c@... PS1, Line 486:
Should the check for child->path. […]
Done
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46542 )
Change subject: device: Allow virtual/generic devices under PCI in devicetree ......................................................................
Patch Set 2: Code-Review+2
Duncan Laurie has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46542 )
Change subject: device: Allow virtual/generic devices under PCI in devicetree ......................................................................
device: Allow virtual/generic devices under PCI in devicetree
This change allows a generic device to be described in the devicetree under a PCI device, such as a root port.
Previously any device under a PCI device was expected to also be a PCI device and that does not allow for a virtual/generic device to be present, for example to provide ACPI properties for a root port.
The changes are: - Ignore non-PCI devices found under a PCI device when scanning and do not print an error for each devfn scanned. - Don't treat non-PCI devices as leftover and remove them, instead enable them as a static device. - Don't attempt to configure a static device in the tree that is not a PCIe device type.
With these changes it is now possible to have a generic device under a PCI device, for example in a USB4/TBT root port (PCIe hotplug device) this generic device will add ACPI properties for the PCIe tunnel routed to the external port:
device pci 07.0 on chip soc/intel/common/block/pcie device generic 0 on end end end
TEST=boot on volteer with the USB4 root port devices in chipset.cb and ensure they are enabled properly and there are no errors printed in the coreboot log, and that the device properties are created in the SSDT.
Signed-off-by: Duncan Laurie dlaurie@google.com Change-Id: I56a491808067dc862a7adfd46852f0bd6b41cd95 Reviewed-on: https://review.coreboot.org/c/coreboot/+/46542 Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/device/pci_device.c M src/device/pciexp_device.c 2 files changed, 19 insertions(+), 10 deletions(-)
Approvals: build bot (Jenkins): Verified Furquan Shaikh: Looks good to me, approved
diff --git a/src/device/pci_device.c b/src/device/pci_device.c index ce3e509..691ad6b 100644 --- a/src/device/pci_device.c +++ b/src/device/pci_device.c @@ -1006,16 +1006,11 @@
prev = &bus->children; for (dev = bus->children; dev; dev = dev->sibling) { - if (dev->path.type == DEVICE_PATH_PCI) { - if (dev->path.pci.devfn == devfn) { - /* Unlink from the list. */ - *prev = dev->sibling; - dev->sibling = NULL; - break; - } - } else { - printk(BIOS_ERR, "child %s not a PCI device\n", - dev_path(dev)); + if (dev->path.type == DEVICE_PATH_PCI && dev->path.pci.devfn == devfn) { + /* Unlink from the list. */ + *prev = dev->sibling; + dev->sibling = NULL; + break; } prev = &dev->sibling; } @@ -1283,6 +1278,16 @@
prev = &bus->children; for (dev = bus->children; dev; dev = dev->sibling) { + + /* + * If static device is not PCI then enable it here and don't + * treat it as a leftover device. + */ + if (dev->path.type != DEVICE_PATH_PCI) { + enable_static_device(dev); + continue; + } + /* * The device is only considered leftover if it is not hidden * and it has a Vendor ID of 0 (the default for a device that diff --git a/src/device/pciexp_device.c b/src/device/pciexp_device.c index f04d865..8d4bb98 100644 --- a/src/device/pciexp_device.c +++ b/src/device/pciexp_device.c @@ -180,6 +180,8 @@
for (bus = dev->link_list ; bus ; bus = bus->next) { for (child = bus->children; child; child = child->sibling) { + if (child->path.type != DEVICE_PATH_PCI) + continue; pciexp_configure_ltr(child); if (child->ops && child->ops->scan_bus) pciexp_enable_ltr(child); @@ -478,6 +480,8 @@ pci_scan_bus(bus, min_devfn, max_devfn);
for (child = bus->children; child; child = child->sibling) { + if (child->path.type != DEVICE_PATH_PCI) + continue; if ((child->path.pci.devfn < min_devfn) || (child->path.pci.devfn > max_devfn)) { continue;
-
Duncan Laurie (Code Review) -
Furquan Shaikh (Code Review)