Jeremy Soller has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: driver/thunderbolt: Driver for allocating hotplug resources ......................................................................
driver/thunderbolt: Driver for allocating hotplug resources
This adds a new driver which can be selected with DRIVERS_THUNDERBOLT that, by default, adds 32 PCI subordinate numbers, 256 MiB of both prefetchable and non-prefetchable memory, and 8 KiB of I/O space to matching devices. It currently supports the JHL7540 Thunderbolt 3 bridge, but other devices can be added easily.
In order to support the allocation of hotplugged PCI buses, a new field was added to struct device called hotplug_buses. This is defaulted to zero, but when set, it adds the hotplug_buses value to the subordinate value of the PCI bridge. This allows devices to be plugged in and unplugged after boot.
This code was tested on the System76 Darter Pro (darp6). Before this change, there are not enough resources allocated to the Thunderbolt PCI bridge to allow plugging in new devices after boot. This can be worked around in the Linux kernel by passing a boot param such as: pci=assign-buses,hpbussize=32,realloc
This change makes it possible to use Thunderbolt hotplugging without kernel parameters, and attempts to match closely what our motherboard manufacturer's firmware does by default.
Signed-off-by: Jeremy Soller jeremy@system76.com Change-Id: I500191626584b83e6a8ae38417fd324b5e803afc --- M src/device/pci_device.c A src/drivers/thunderbolt/Kconfig A src/drivers/thunderbolt/Makefile.inc A src/drivers/thunderbolt/thunderbolt.c M src/include/device/device.h 5 files changed, 117 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/35946/1
diff --git a/src/device/pci_device.c b/src/device/pci_device.c index c043dd6..1796d81 100644 --- a/src/device/pci_device.c +++ b/src/device/pci_device.c @@ -1217,7 +1217,7 @@
if (state == PCI_ROUTE_SCAN) { link->secondary = parent->subordinate + 1; - link->subordinate = link->secondary; + link->subordinate = link->secondary + dev->hotplug_buses; }
if (state == PCI_ROUTE_CLOSE) { diff --git a/src/drivers/thunderbolt/Kconfig b/src/drivers/thunderbolt/Kconfig new file mode 100644 index 0000000..f64e6cb --- /dev/null +++ b/src/drivers/thunderbolt/Kconfig @@ -0,0 +1,4 @@ +config DRIVERS_THUNDERBOLT + bool + help + Thunderbolt support diff --git a/src/drivers/thunderbolt/Makefile.inc b/src/drivers/thunderbolt/Makefile.inc new file mode 100644 index 0000000..623b897 --- /dev/null +++ b/src/drivers/thunderbolt/Makefile.inc @@ -0,0 +1 @@ +ramstage-$(CONFIG_DRIVERS_THUNDERBOLT) += thunderbolt.c diff --git a/src/drivers/thunderbolt/thunderbolt.c b/src/drivers/thunderbolt/thunderbolt.c new file mode 100644 index 0000000..db463e0e --- /dev/null +++ b/src/drivers/thunderbolt/thunderbolt.c @@ -0,0 +1,110 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 System76. + * Copyright (C) 2017-2018 Intel Corporation. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <console/console.h> +#include <device/device.h> +#include <device/pci.h> +#include <device/pciexp.h> +#include <device/pci_def.h> +#include <device/pci_ids.h> +#include <device/pci_ops.h> + +static void slot_dev_read_resources(struct device *dev) +{ + struct resource *resource; + + // Add 256 MiB of memory space + resource = new_resource(dev, 0x10); + resource->size = 1 << 28; + resource->align = 22; + resource->gran = 22; + resource->limit = 0xffffffff; + resource->flags |= IORESOURCE_MEM; + + // Add 256 MiB of prefetchable memory space + resource = new_resource(dev, 0x14); + resource->size = 1 << 28; + resource->align = 22; + resource->gran = 22; + resource->limit = 0xffffffff; + resource->flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH; + + // Add 8 KiB of I/O space + resource = new_resource(dev, 0x18); + resource->size = 1 << 13; + resource->align = 12; + resource->gran = 12; + resource->limit = 0xffff; + resource->flags |= IORESOURCE_IO; +} + +static struct device_operations slot_dev_ops = { + .read_resources = slot_dev_read_resources, +}; + +static bool tbt_is_hotplug_bridge(struct device *dev) { + return PCI_SLOT(dev->path.pci.devfn) == 1; +} + +static void tbt_pciexp_scan_bridge(struct device *dev) { + printk(BIOS_DEBUG, "%s: %s: scan bridge\n", __func__, dev_path(dev)); + + bool is_hotplug = tbt_is_hotplug_bridge(dev); + if (is_hotplug) { + /* Add hotplug buses, must happen before bus scan */ + printk(BIOS_DEBUG, "%s: %s: add hotplug buses\n", __func__, dev_path(dev)); + dev->hotplug_buses = 32; + } + + /* Normal PCIe Scan */ + pciexp_scan_bridge(dev); + + if (is_hotplug) { + /* Add dummy slot to preserve resources, must happen after bus scan */ + printk(BIOS_DEBUG, "%s: %s: add dummy device\n", __func__, dev_path(dev)); + struct device *slot; + struct device_path slot_path = { .type = DEVICE_PATH_NONE }; + slot = alloc_dev(dev->link_list, &slot_path); + slot->ops = &slot_dev_ops; + } +} + +static struct pci_operations pcie_ops = { + .set_subsystem = pci_dev_set_subsystem, +}; + +static struct device_operations device_ops = { + .read_resources = pci_bus_read_resources, + .set_resources = pci_dev_set_resources, + .enable_resources = pci_bus_enable_resources, + .init = 0, + .scan_bus = tbt_pciexp_scan_bridge, + .enable = 0, + .reset_bus = pci_bus_reset, + .ops_pci = &pcie_ops, +}; + +static const unsigned short pcie_device_ids[] = { + // JHL7540 Thunderbolt 3 Bridge + 0x15e7, + 0 +}; + +static const struct pci_driver tbt_pcie __pci_driver = { + .ops = &device_ops, + .vendor = PCI_VENDOR_ID_INTEL, + .devices = pcie_device_ids, +}; diff --git a/src/include/device/device.h b/src/include/device/device.h index cb37c09..f22bed5 100644 --- a/src/include/device/device.h +++ b/src/include/device/device.h @@ -129,6 +129,7 @@ unsigned int disable_pcie_aspm : 1; unsigned int hidden : 1; /* set if we should hide from UI */ u8 command; + uint16_t hotplug_buses; /* hotplug buses to allocate */
/* Base registers for this device. I/O, MEM and Expansion ROM */ DEVTREE_CONST struct resource *resource_list;
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: driver/thunderbolt: Driver for allocating hotplug resources ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... File src/drivers/thunderbolt/thunderbolt.c:
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 58: static bool tbt_is_hotplug_bridge(struct device *dev) { open brace '{' following function definitions go on the next line
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 62: static void tbt_pciexp_scan_bridge(struct device *dev) { open brace '{' following function definitions go on the next line
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: driver/thunderbolt: Driver for allocating hotplug resources ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... File src/drivers/thunderbolt/thunderbolt.c:
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 25: static void slot_dev_read_resources(struct device *dev) that code should be shared with the existing southbridge code.
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 69: dev->hotplug_buses = 32; I'd prefer a virtual PCI bridge instead, each having assinged one downstream device of type DEVICE_PATH_NONE. That way every bridge would be assigned a bus number and you don't have to touch device.c
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 80: slot = alloc_dev(dev->link_list, &slot_path); don't you need to allocate "hotplug_buses" devices, as every potential device could have two 256MiB bars and I/O memory?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: driver/thunderbolt: Driver for allocating hotplug resources ......................................................................
Patch Set 1:
I would rather see the hotplug-part in device/ as a separate commit.
I think there is a hotplug-capability bit in one of the PCIe standard capability blocks that should be evaluated as well. Can you paste lspci -xxxx output of that thunderbolt device somewhere (mailing list would be fine), maybe there is a more generic solution that does not depend on PCI device ID.
The hotplug code should also hook with cardbus bridges.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: driver/thunderbolt: Driver for allocating hotplug resources ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... File src/drivers/thunderbolt/thunderbolt.c:
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 59: return PCI_SLOT(dev->path.pci.devfn) == 1; Is this defined in some standard or just happens to be the case for the JHL7540?
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 69: dev->hotplug_buses = 32;
I'd prefer a virtual PCI bridge instead, each having assinged one downstream device of type DEVICE_P […]
While I like this idea. The `hotplug_buses` implementation seems clean and small enough?
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 80: slot = alloc_dev(dev->link_list, &slot_path);
don't you need to allocate "hotplug_buses" devices, as every potential device could have two 256MiB […]
I don't follow, are you suggesting to allocate a total of 2x 8GiB? That's not even possible with non-prefetchable resources (standard bridges have only a 32-bit window, IIRC). And our allocator is also still limited to 32 bit.
Also, AIUI, 256MiB is just an arbitrary number, it's not a real limit, is it?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: driver/thunderbolt: Driver for allocating hotplug resources ......................................................................
Patch Set 1:
(13 comments)
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/Kco... File src/drivers/thunderbolt/Kconfig:
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/Kco... PS1, Line 1: config DRIVERS_THUNDERBOLT USB4 is the new hotness. We may want to figure out how to surface that noun since that one is an open standard.
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... File src/drivers/thunderbolt/thunderbolt.c:
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 30: 0x10) These should be using the PCI macros for the registers.
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 31: 1 << 28 256 * MiB
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 33: resource->gran = 22; Where did the alignment come from?
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 51: resource->flags |= IORESOURCE_IO; It's not clear to me what the rationale is for the resource sizes being speculatively allocated under the device.
Likewise can you please explain why we have to preallocate buses as well? Are you assuming the code running after coreboot cannot allocate buses and mmio spaces?
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 59: return PCI_SLOT(dev->path.pci.devfn) == 1;
Is this defined in some standard or just happens to be the case for the […]
What does the device tree look like to support this device? I assume this is the router? Are there multiple pci functions with the same did but only one is the router?
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 69: 32 Why are we hard coding the number of buses? Shouldn't we make this configurable?
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 93: .init = 0, not needed
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 94: .scan_bus = tbt_pciexp_scan_bridge, I believe thunderbolt and usb4 controllers need to export capabilities in pci config space. Can we not handle the special devices based on that?
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 95: .enable = 0, not needed
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 102: 0x15e7 move to pci_ids.h
https://review.coreboot.org/c/coreboot/+/35946/1/src/include/device/device.h File src/include/device/device.h:
https://review.coreboot.org/c/coreboot/+/35946/1/src/include/device/device.h... PS1, Line 132: hotplug buses number?
https://review.coreboot.org/c/coreboot/+/35946/1/src/include/device/device.h... PS1, Line 132: uint16_t This doesn't need to be fixed width.
Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: driver/thunderbolt: Driver for allocating hotplug resources ......................................................................
Patch Set 1:
Thanks for the replies to this patch, they have been very helpful. I now see there are some ways to make this more generic - for supporting any hotplug bridge using the HotPlug PCI slot capability as an indicator. This would also allow hotplugging to be supported correctly for U.2 devices, which again may boot without a device connected and then could have any PCIe device plugged in to them when the OS is loaded.
I have also found a nice way to emulate hotpluggable PCIe devices in QEMU: https://github.com/qemu/qemu/blob/master/docs/pcie_pci_bridge.txt
I will work to have a more generic solution and resubmit it as a different driver, since the name thunderbolt is specific to only one technology - Thunderbolt 3 - and that will soon be obsoleted by USB 4.
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: driver/thunderbolt: Driver for allocating hotplug resources ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/35946/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35946/1//COMMIT_MSG@7 PS1, Line 7: driver/thunderbolt: Driver for allocating hotplug resources Please make it a statement by adding a verb (in imperative mood):
Add driver for allocating hotplug resources
https://review.coreboot.org/c/coreboot/+/35946/1//COMMIT_MSG@16 PS1, Line 16: was is
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: driver/thunderbolt: Driver for allocating hotplug resources ......................................................................
Patch Set 1:
Please also participate in the discussion on the mailing list *PCIe Hotplug for Thunderbolt and U.2* [1].
[1]: https://mail.coreboot.org/hyperkitty/list/coreboot@coreboot.org/thread/QW3NV...
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: driver/thunderbolt: Driver for allocating hotplug resources ......................................................................
Patch Set 1:
(5 comments)
only had a brief look, but i like where this is going. splitting the patch into the (pci_)device part and the TB-specific part would be a good, since those two are separate changes
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... File src/drivers/thunderbolt/thunderbolt.c:
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 51: resource->flags |= IORESOURCE_IO;
It's not clear to me what the rationale is for the resource sizes being speculatively allocated unde […]
In order for the hot-plugging of devices and busses to work, we need to reserve MMIO and IO space for the potential devices behind this bridge for the allocator, so a big enough MMIO window gets reserved for the hot-pluggeable devices and get routed through the bridges between this one and the PCIe root. Since we don't know what devices will get hot-plugged after coreboot configured things, we'll just have to reserve enough resources here. The MMIO decode windows in the bridges between this bridge and the root port is the big issue that is addressed here. If this isn't done here, the system would have to move around MMIO BARs of other PCIe devices to be able to map the device's MMIO behind the TB bridge while the other devices still need to work, but you'll probably need to unload the corresponding drivers, move the BARs around and then reload the device drivers which isn't very practical and might even not be implemented on operating systems.
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 69: dev->hotplug_buses = 32;
While I like this idea. The `hotplug_buses` implementation seems clean […]
I also like the hotplug_buses approach and it's rather noninvasive
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 69: 32
Why are we hard coding the number of buses? Shouldn't we make this configurable?
might be worth putting this into the device tree and only defaulting to 32 when there's no setting in the device tree
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 73: pciexp_scan_bridge(dev); haven't looked very closely at the code, but what does happen if there are devices already connected behind the TB bridge at boot time? do the 256MiB MMIO space get allocated additionally?
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 80: slot = alloc_dev(dev->link_list, &slot_path);
I don't follow, are you suggesting to allocate a total of 2x 8GiB? […]
IIRC the 256MiB was what the vendor firmware does; not sure if this is mandated by the TB spec. 256MiB prefetchable MMIO space sounds reasonable to me; 256MiB non-prefetchable MMIO sounds rather big to me though (and the non-prefetchable can't be mapped above 4G)
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: driver/thunderbolt: Driver for allocating hotplug resources ......................................................................
Patch Set 1:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... File src/drivers/thunderbolt/thunderbolt.c:
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 51: resource->flags |= IORESOURCE_IO;
In order for the hot-plugging of devices and busses to work, we need to reserve MMIO and IO space fo […]
I understand the requirements in order to set up a pci topology. However, my question was bout the assumptions that are being made about the OS or program running after coreboot. This patch is assuming a static allocation is sufficient when it's definitely not going to be correct 100% of the time.
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 73: pciexp_scan_bridge(dev);
haven't looked very closely at the code, but what does happen if there are devices already connected […]
That's assuming thunderbolt links are already provisioned and pcie tunneling is happening. Otherwise one couldn't see the devices on the other side of the link.
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 80: slot = alloc_dev(dev->link_list, &slot_path);
IIRC the 256MiB was what the vendor firmware does; not sure if this is mandated by the TB spec. 256MiB prefetchable MMIO space sounds reasonable to me; 256MiB non-prefetchable MMIO sounds rather big to me though (and the non-prefetchable can't be mapped above 4G)
What are you basing your assumptions on? Video cards and small BARs on a nic?
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: driver/thunderbolt: Driver for allocating hotplug resources ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... File src/drivers/thunderbolt/thunderbolt.c:
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 51: resource->flags |= IORESOURCE_IO;
I understand the requirements in order to set up a pci topology. […]
since you don't know what's getting hot-plugged during runtime, you'll probably just have to reserve a big enough mmio area the bridges decode. To change the mmio decode windows of the bridge, you'd probably have to move around other allocated resources which is either really difficult or more or less impossible when the driver for the other devices are loaded. On why the non-prefetchable region can be smaller than the prefetchable: non-prefetchable is mainly used for register spaces in devices where reads and the sequence of writes can change the state of the device; if it just maps to some memory in the device it can be made prefetchable. So the prefetchable regions are bigger than the non-prefetchable ones
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: driver/thunderbolt: Driver for allocating hotplug resources ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... File src/drivers/thunderbolt/thunderbolt.c:
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 51: resource->flags |= IORESOURCE_IO;
since you don't know what's getting hot-plugged during runtime, you'll probably just have to reserve […]
I think we should make the sizing configurable and not open/hard coded as it is currently. Likewise, we'll be seeing multiple usb4/thunderbolt devices in systems so these pre-allocations will be multiplied.
Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: driver/thunderbolt: Driver for allocating hotplug resources ......................................................................
Patch Set 1:
(1 comment)
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... File src/drivers/thunderbolt/thunderbolt.c:
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 51: resource->flags |= IORESOURCE_IO;
I think we should make the sizing configurable and not open/hard coded as it is currently. […]
I agree, I will have an updated patch with this in the next few days. I have been working on other things
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: driver/thunderbolt: Driver for allocating hotplug resources ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... File src/drivers/thunderbolt/thunderbolt.c:
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 51: resource->flags |= IORESOURCE_IO;
I agree, I will have an updated patch with this in the next few days. […]
sounds good to me
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35946
to look at the new patch set (#2).
Change subject: pciexp: Support for allocating PCI express hotplug resources ......................................................................
pciexp: Support for allocating PCI express hotplug resources
This adds support for allocating resources for PCI express hotplug bridges when PCIEXP_HOTPLUG is selected. By default, this will add 32 PCI subordinate numbers (buses), 256 MiB of prefetchable memory, 8 MiB of non-prefetchable memory, and 8 KiB of I/O space to any device with the PCI_EXP_SLTCAP_HPC bit set in the PCI_EXP_SLTCAP register, which indicates hot-plugging capability. The resource allocation is configurable, please see the PCIEXP_HOTPLUG_* variables in src/device/Kconfig.
In order to support the allocation of hotplugged PCI buses, a new field was added to struct device called hotplug_buses. This is defaulted to zero, but when set, it adds the hotplug_buses value to the subordinate value of the PCI bridge. This allows devices to be plugged in and unplugged after boot.
This code was tested on the System76 Darter Pro (darp6). Before this change, there are not enough resources allocated to the Thunderbolt PCI bridge to allow plugging in new devices after boot. This can be worked around in the Linux kernel by passing a boot param such as: pci=assign-buses,hpbussize=32,realloc
This change makes it possible to use Thunderbolt hotplugging without kernel parameters, and attempts to match closely what our motherboard manufacturer's firmware does by default.
Signed-off-by: Jeremy Soller jeremy@system76.com Change-Id: I500191626584b83e6a8ae38417fd324b5e803afc --- M src/device/Kconfig M src/device/pci_device.c M src/device/pciexp_device.c M src/include/device/device.h M src/include/device/pci_def.h M src/include/device/pciexp.h 6 files changed, 134 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/35946/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: pciexp: Support for allocating PCI express hotplug resources ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pci_device.c@886 PS2, Line 886: } else else is not generally useful after a break or return
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pci_device.c@886 PS2, Line 886: } else suspect code indent for conditional statements (24, 24)
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c@... PS2, Line 500: printk(BIOS_DEBUG, "%s: add 0x%llx of prefetch memory space\n", __func__, resource->size); line over 96 characters
Hello build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35946
to look at the new patch set (#3).
Change subject: pciexp: Add support for allocating PCI express hotplug resources ......................................................................
pciexp: Add support for allocating PCI express hotplug resources
This change adds support for allocating resources for PCI express hotplug bridges when PCIEXP_HOTPLUG is selected. By default, this will add 32 PCI subordinate numbers (buses), 256 MiB of prefetchable memory, 8 MiB of non-prefetchable memory, and 8 KiB of I/O space to any device with the PCI_EXP_SLTCAP_HPC bit set in the PCI_EXP_SLTCAP register, which indicates hot-plugging capability. The resource allocation is configurable, please see the PCIEXP_HOTPLUG_* variables in src/device/Kconfig.
In order to support the allocation of hotplugged PCI buses, a new field is added to struct device called hotplug_buses. This is defaulted to zero, but when set, it adds the hotplug_buses value to the subordinate value of the PCI bridge. This allows devices to be plugged in and unplugged after boot.
This code was tested on the System76 Darter Pro (darp6). Before this change, there are not enough resources allocated to the Thunderbolt PCI bridge to allow plugging in new devices after boot. This can be worked around in the Linux kernel by passing a boot param such as: pci=assign-buses,hpbussize=32,realloc
This change makes it possible to use Thunderbolt hotplugging without kernel parameters, and attempts to match closely what our motherboard manufacturer's firmware does by default.
Signed-off-by: Jeremy Soller jeremy@system76.com Change-Id: I500191626584b83e6a8ae38417fd324b5e803afc --- M src/device/Kconfig M src/device/pci_device.c M src/device/pciexp_device.c M src/include/device/device.h M src/include/device/pci_def.h M src/include/device/pciexp.h 6 files changed, 120 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/35946/3
Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: pciexp: Add support for allocating PCI express hotplug resources ......................................................................
Patch Set 2:
(20 comments)
A large number of the comments made before were solved with the latest changeset (3). Please review this changeset, it is no longer specific to thunderbolt 3 devices but encompases any PCI express hotplug bridges.
https://review.coreboot.org/c/coreboot/+/35946/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35946/1//COMMIT_MSG@7 PS1, Line 7: driver/thunderbolt: Driver for allocating hotplug resources
Please make it a statement by adding a verb (in imperative mood): […]
Done
https://review.coreboot.org/c/coreboot/+/35946/1//COMMIT_MSG@16 PS1, Line 16: was
is
Done
https://review.coreboot.org/c/coreboot/+/35946/1//COMMIT_MSG@16 PS1, Line 16: was
is
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/Kco... File src/drivers/thunderbolt/Kconfig:
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/Kco... PS1, Line 1: config DRIVERS_THUNDERBOLT
USB4 is the new hotness. […]
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... File src/drivers/thunderbolt/thunderbolt.c:
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 25: static void slot_dev_read_resources(struct device *dev)
that code should be shared with the existing southbridge code.
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 30: 0x10)
These should be using the PCI macros for the registers.
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 31: 1 << 28
256 * MiB
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 33: resource->gran = 22;
Where did the alignment come from?
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 51: resource->flags |= IORESOURCE_IO;
sounds good to me
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 59: return PCI_SLOT(dev->path.pci.devfn) == 1;
What does the device tree look like to support this device? I assume this is the router? Are there m […]
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 69: dev->hotplug_buses = 32;
I also like the hotplug_buses approach and it's rather noninvasive
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 69: 32
might be worth putting this into the device tree and only defaulting to 32 when there's no setting i […]
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 73: pciexp_scan_bridge(dev);
That's assuming thunderbolt links are already provisioned and pcie tunneling is happening. […]
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 80: slot = alloc_dev(dev->link_list, &slot_path);
IIRC the 256MiB was what the vendor firmware does; not sure if this is mandated by the TB spec. […]
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 93: .init = 0,
not needed
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 94: .scan_bus = tbt_pciexp_scan_bridge,
I believe thunderbolt and usb4 controllers need to export capabilities in pci config space. […]
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 95: .enable = 0,
not needed
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/drivers/thunderbolt/thu... PS1, Line 102: 0x15e7
move to pci_ids. […]
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/include/device/device.h File src/include/device/device.h:
https://review.coreboot.org/c/coreboot/+/35946/1/src/include/device/device.h... PS1, Line 132: hotplug buses
number?
Done
https://review.coreboot.org/c/coreboot/+/35946/1/src/include/device/device.h... PS1, Line 132: uint16_t
This doesn't need to be fixed width.
Done
Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: pciexp: Add support for allocating PCI express hotplug resources ......................................................................
Patch Set 3: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c@... PS2, Line 485: 0x10 Does this handle BAR0? If yes you could use PCI_BASE_ADDRESS_0 instead of 0x10 her.
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c@... PS2, Line 494: 0x14 Same question here with BAR1
https://review.coreboot.org/c/coreboot/+/35946/2/src/include/device/device.h File src/include/device/device.h:
https://review.coreboot.org/c/coreboot/+/35946/2/src/include/device/device.h... PS2, Line 133: /* hotplug buses to allocate */ Maybe better: Number of hotplug buses to allocate?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: pciexp: Add support for allocating PCI express hotplug resources ......................................................................
Patch Set 3:
(7 comments)
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pci_device.c@886 PS2, Line 886: } else
else is not generally useful after a break or return
You should write this without the preprocessor #if.
I am not sure what we decided about the asymmetric use of braces
if () { stuff1; stuff2; } else stuff3;
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pci_device.c@126... PS2, Line 1269: link->subordinate = link->secondary + dev->hotplug_buses; It is too early to adjust link->subordinate here. Downstream PCI bridges will reference this as the recursive scan proceeds.
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c@... PS2, Line 485: 0x10
Does this handle BAR0? If yes you could use PCI_BASE_ADDRESS_0 instead of 0x10 her.
These are just dummy index numbers and the resources will not be written to hardware?
I think it would be cleaner to just use 'i++' here instead of fooling us to think these would end up written in any PCI hardware BARs. Also I think the dummy device we add should be considered as a pseudo PCI bridge with PCI Header Type 1 and the BAR numbers used here are for Type 0.
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c@... PS2, Line 488: resource->gran = 22; I believe both .align and .gran need to conform with the requirements of the configurable .size.
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c@... PS2, Line 527: pciexp_scan_bridge(dev); It should be possible to adjust dev->link_list->subordinate here to either max N or additional N PCI buses without adding hotplug_buses in struct device.
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c@... PS2, Line 538: dummy->ops = &pciexp_hotplug_dummy_ops; I like the dummy device approach, however it does make an additional reserve instead of guaranteeing a minimal resource space. We cannot assume all hotpluggable devices to be disconnected for the enumeration, those connected will already claim resources and we risk running out of MMIO if we also add the reserve.
https://review.coreboot.org/c/coreboot/+/35946/2/src/include/device/pciexp.h File src/include/device/pciexp.h:
https://review.coreboot.org/c/coreboot/+/35946/2/src/include/device/pciexp.h... PS2, Line 29: #if CONFIG(PCIEXP_HOTPLUG) No quard needed.
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: pciexp: Add support for allocating PCI express hotplug resources ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35946/3/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/35946/3/src/device/Kconfig@576 PS3, Line 576: PCIEXP_HOTPLUG_MEM this should probably be PCIEXP_HOTPLUG_PREFETCH_MEM
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: pciexp: Add support for allocating PCI express hotplug resources ......................................................................
Patch Set 3:
@Jeremy any update on this CL? I would like to see the usb4 support and additional patches like this one getting merged
Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: pciexp: Add support for allocating PCI express hotplug resources ......................................................................
Patch Set 3:
(1 comment)
Patch Set 3:
@Jeremy any update on this CL? I would like to see the usb4 support and additional patches like this one getting merged
I will see about addressing the comments today.
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c@... PS2, Line 488: resource->gran = 22;
I believe both .align and .gran need to conform with the requirements of the configurable .size.
I'm not sure this needs to be anything other than page aligned. I could also round the size to page aligned.
Bill XIE has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: pciexp: Add support for allocating PCI express hotplug resources ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/35946/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35946/3//COMMIT_MSG@27 PS3, Line 27: assign-buses It should be "assign-busses", as Documentation/x86/x86_64/boot-options.rst of Linux kernel states.
Hello Werner Zeh, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35946
to look at the new patch set (#4).
Change subject: pciexp: Add support for allocating PCI express hotplug resources ......................................................................
pciexp: Add support for allocating PCI express hotplug resources
This change adds support for allocating resources for PCI express hotplug bridges when PCIEXP_HOTPLUG is selected. By default, this will add 32 PCI subordinate numbers (buses), 256 MiB of prefetchable memory, 8 MiB of non-prefetchable memory, and 8 KiB of I/O space to any device with the PCI_EXP_SLTCAP_HPC bit set in the PCI_EXP_SLTCAP register, which indicates hot-plugging capability. The resource allocation is configurable, please see the PCIEXP_HOTPLUG_* variables in src/device/Kconfig.
In order to support the allocation of hotplugged PCI buses, a new field is added to struct device called hotplug_buses. This is defaulted to zero, but when set, it adds the hotplug_buses value to the subordinate value of the PCI bridge. This allows devices to be plugged in and unplugged after boot.
This code was tested on the System76 Darter Pro (darp6). Before this change, there are not enough resources allocated to the Thunderbolt PCI bridge to allow plugging in new devices after boot. This can be worked around in the Linux kernel by passing a boot param such as: pci=assign-busses,hpbussize=32,realloc
This change makes it possible to use Thunderbolt hotplugging without kernel parameters, and attempts to match closely what our motherboard manufacturer's firmware does by default.
Signed-off-by: Jeremy Soller jeremy@system76.com Change-Id: I500191626584b83e6a8ae38417fd324b5e803afc --- M src/device/Kconfig M src/device/pci_device.c M src/device/pciexp_device.c M src/include/device/device.h M src/include/device/pci_def.h M src/include/device/pciexp.h 6 files changed, 120 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/35946/4
Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: pciexp: Add support for allocating PCI express hotplug resources ......................................................................
Patch Set 4:
(3 comments)
I addressed some of the comments in patch set 4. I am working on a variety of GPUs and expect to have further updates as necessary.
https://review.coreboot.org/c/coreboot/+/35946/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35946/3//COMMIT_MSG@27 PS3, Line 27: assign-buses
It should be "assign-busses", as Documentation/x86/x86_64/boot-options.rst of Linux kernel states.
Done
https://review.coreboot.org/c/coreboot/+/35946/3/src/device/Kconfig File src/device/Kconfig:
https://review.coreboot.org/c/coreboot/+/35946/3/src/device/Kconfig@576 PS3, Line 576: PCIEXP_HOTPLUG_MEM
this should probably be PCIEXP_HOTPLUG_PREFETCH_MEM
Done
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c@... PS2, Line 488: resource->gran = 22;
I'm not sure this needs to be anything other than page aligned. […]
Done
Brandon Breitenstein has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: pciexp: Add support for allocating PCI express hotplug resources ......................................................................
Patch Set 4:
What other review is needed here? I am working on upstreaming TBT/USB4 code for Tigerlake and want to make sure this is merged so I can refactor my code to use this.
Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: pciexp: Add support for allocating PCI express hotplug resources ......................................................................
Patch Set 4:
(2 comments)
I would also like to know what is absolutely required for merging this patch. We at System76 have been using a similar patch successfully on production systems with Thunderbolt 3 for a few months.
It would help to reiterate what is left that must be fixed before this can get a +2.
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pci_device.c File src/device/pci_device.c:
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pci_device.c@126... PS2, Line 1269: link->subordinate = link->secondary + dev->hotplug_buses;
It is too early to adjust link->subordinate here. […]
I have not seen this issue. Adjusting it later on causes the value to not take effect.
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c@... PS2, Line 527: pciexp_scan_bridge(dev);
It should be possible to adjust dev->link_list->subordinate here to either max N or additional N PCI […]
When I attempted to do this earlier, it did not take effect.
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: pciexp: Add support for allocating PCI express hotplug resources ......................................................................
Patch Set 4: Code-Review+2
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: pciexp: Add support for allocating PCI express hotplug resources ......................................................................
Patch Set 4:
@Jeremy Can you resolve the comments?
Hello Werner Zeh, Philipp Deppenwiese, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/35946
to look at the new patch set (#5).
Change subject: pciexp: Add support for allocating PCI express hotplug resources ......................................................................
pciexp: Add support for allocating PCI express hotplug resources
This change adds support for allocating resources for PCI express hotplug bridges when PCIEXP_HOTPLUG is selected. By default, this will add 32 PCI subordinate numbers (buses), 256 MiB of prefetchable memory, 8 MiB of non-prefetchable memory, and 8 KiB of I/O space to any device with the PCI_EXP_SLTCAP_HPC bit set in the PCI_EXP_SLTCAP register, which indicates hot-plugging capability. The resource allocation is configurable, please see the PCIEXP_HOTPLUG_* variables in src/device/Kconfig.
In order to support the allocation of hotplugged PCI buses, a new field is added to struct device called hotplug_buses. This is defaulted to zero, but when set, it adds the hotplug_buses value to the subordinate value of the PCI bridge. This allows devices to be plugged in and unplugged after boot.
This code was tested on the System76 Darter Pro (darp6). Before this change, there are not enough resources allocated to the Thunderbolt PCI bridge to allow plugging in new devices after boot. This can be worked around in the Linux kernel by passing a boot param such as: pci=assign-busses,hpbussize=32,realloc
This change makes it possible to use Thunderbolt hotplugging without kernel parameters, and attempts to match closely what our motherboard manufacturer's firmware does by default.
Signed-off-by: Jeremy Soller jeremy@system76.com Change-Id: I500191626584b83e6a8ae38417fd324b5e803afc --- M src/device/Kconfig M src/device/pci_device.c M src/device/pciexp_device.c M src/include/device/device.h M src/include/device/pci_def.h M src/include/device/pciexp.h 6 files changed, 120 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/46/35946/5
Jeremy Soller has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: pciexp: Add support for allocating PCI express hotplug resources ......................................................................
Patch Set 5:
(5 comments)
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c@... PS2, Line 485: 0x10
These are just dummy index numbers and the resources will not be written to hardware? […]
This came from src/southbridge/intel/common/pciehp.c
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c@... PS2, Line 494: 0x14
Same question here with BAR1
This came from src/southbridge/intel/common/pciehp.c
https://review.coreboot.org/c/coreboot/+/35946/2/src/device/pciexp_device.c@... PS2, Line 538: dummy->ops = &pciexp_hotplug_dummy_ops;
I like the dummy device approach, however it does make an additional reserve instead of guaranteeing […]
With Thunderbolt 3 currently coreboot does not set a security settings so it defaults to having no PCIe capability, meaning the bridges will not have any extra allocation by accident.
https://review.coreboot.org/c/coreboot/+/35946/2/src/include/device/device.h File src/include/device/device.h:
https://review.coreboot.org/c/coreboot/+/35946/2/src/include/device/device.h... PS2, Line 133: /* hotplug buses to allocate */
Maybe better: Number of hotplug buses to allocate?
Done
https://review.coreboot.org/c/coreboot/+/35946/2/src/include/device/pciexp.h File src/include/device/pciexp.h:
https://review.coreboot.org/c/coreboot/+/35946/2/src/include/device/pciexp.h... PS2, Line 29: #if CONFIG(PCIEXP_HOTPLUG)
No quard needed.
I think it is better to not declare the items inside this guard if they are not defined
Philipp Deppenwiese has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: pciexp: Add support for allocating PCI express hotplug resources ......................................................................
Patch Set 5: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: pciexp: Add support for allocating PCI express hotplug resources ......................................................................
pciexp: Add support for allocating PCI express hotplug resources
This change adds support for allocating resources for PCI express hotplug bridges when PCIEXP_HOTPLUG is selected. By default, this will add 32 PCI subordinate numbers (buses), 256 MiB of prefetchable memory, 8 MiB of non-prefetchable memory, and 8 KiB of I/O space to any device with the PCI_EXP_SLTCAP_HPC bit set in the PCI_EXP_SLTCAP register, which indicates hot-plugging capability. The resource allocation is configurable, please see the PCIEXP_HOTPLUG_* variables in src/device/Kconfig.
In order to support the allocation of hotplugged PCI buses, a new field is added to struct device called hotplug_buses. This is defaulted to zero, but when set, it adds the hotplug_buses value to the subordinate value of the PCI bridge. This allows devices to be plugged in and unplugged after boot.
This code was tested on the System76 Darter Pro (darp6). Before this change, there are not enough resources allocated to the Thunderbolt PCI bridge to allow plugging in new devices after boot. This can be worked around in the Linux kernel by passing a boot param such as: pci=assign-busses,hpbussize=32,realloc
This change makes it possible to use Thunderbolt hotplugging without kernel parameters, and attempts to match closely what our motherboard manufacturer's firmware does by default.
Signed-off-by: Jeremy Soller jeremy@system76.com Change-Id: I500191626584b83e6a8ae38417fd324b5e803afc Reviewed-on: https://review.coreboot.org/c/coreboot/+/35946 Reviewed-by: Philipp Deppenwiese zaolin.daisuki@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/device/Kconfig M src/device/pci_device.c M src/device/pciexp_device.c M src/include/device/device.h M src/include/device/pci_def.h M src/include/device/pciexp.h 6 files changed, 120 insertions(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Philipp Deppenwiese: Looks good to me, approved
diff --git a/src/device/Kconfig b/src/device/Kconfig index 0bd9fe1..a25bb91 100644 --- a/src/device/Kconfig +++ b/src/device/Kconfig @@ -555,6 +555,50 @@ help Detect and enable ASPM on PCIe links.
+config PCIEXP_HOTPLUG + prompt "Enable PCIe Hotplug Support" + bool + default n + help + Allocate resources for PCIe hotplug bridges + +if PCIEXP_HOTPLUG + +config PCIEXP_HOTPLUG_BUSES + int "PCI Express Hotplug Buses" + default 32 + help + This is the number of buses allocated for hotplug PCI express + bridges, for use by hotplugged child devices. The default is 32 + buses. + +config PCIEXP_HOTPLUG_MEM + hex "PCI Express Hotplug Memory" + default 0x800000 + help + This is the amount of memory space, in bytes, to allocate to + hotplug PCI express bridges, for use by hotplugged child devices. + This size should be page-aligned. The default is 8 MiB. + +config PCIEXP_HOTPLUG_PREFETCH_MEM + hex "PCI Express Hotplug Prefetch Memory" + default 0x10000000 + help + This is the amount of pre-fetchable memory space, in bytes, to + allocate to hot-plug PCI express bridges, for use by hotplugged + child devices. This size should be page-aligned. The default is + 256 MiB. + +config PCIEXP_HOTPLUG_IO + hex "PCI Express Hotplug I/O Space" + default 0x2000 + help + This is the amount of I/O space to allocate to hot-plug PCI + express bridges, for use by hotplugged child devices. The default + is 8 KiB. + +endif # PCIEXP_HOTPLUG + endif # PCIEXP_PLUGIN_SUPPORT
config EARLY_PCI_BRIDGE diff --git a/src/device/pci_device.c b/src/device/pci_device.c index 36b7c82..47c0e9f 100644 --- a/src/device/pci_device.c +++ b/src/device/pci_device.c @@ -878,6 +878,14 @@ case PCI_EXP_TYPE_DOWNSTREAM: printk(BIOS_DEBUG, "%s subordinate bus PCI Express\n", dev_path(dev)); +#if CONFIG(PCIEXP_HOTPLUG) + u16 sltcap; + sltcap = pci_read_config16(dev, pciexpos + PCI_EXP_SLTCAP); + if (sltcap & PCI_EXP_SLTCAP_HPC) { + printk(BIOS_DEBUG, "%s hot-plug capable\n", dev_path(dev)); + return &default_pciexp_hotplug_ops_bus; + } +#endif /* CONFIG(PCIEXP_HOTPLUG) */ return &default_pciexp_ops_bus; case PCI_EXP_TYPE_PCI_BRIDGE: printk(BIOS_DEBUG, "%s subordinate PCI\n", @@ -1259,7 +1267,7 @@
if (state == PCI_ROUTE_SCAN) { link->secondary = parent->subordinate + 1; - link->subordinate = link->secondary; + link->subordinate = link->secondary + dev->hotplug_buses; }
if (state == PCI_ROUTE_CLOSE) { diff --git a/src/device/pciexp_device.c b/src/device/pciexp_device.c index 479891c..b0ad145 100644 --- a/src/device/pciexp_device.c +++ b/src/device/pciexp_device.c @@ -518,3 +518,62 @@ .reset_bus = pci_bus_reset, .ops_pci = &pciexp_bus_ops_pci, }; + +#if CONFIG(PCIEXP_HOTPLUG) + +static void pciexp_hotplug_dummy_read_resources(struct device *dev) +{ + struct resource *resource; + + // Add extra memory space + resource = new_resource(dev, 0x10); + resource->size = CONFIG_PCIEXP_HOTPLUG_MEM; + resource->align = 12; + resource->gran = 12; + resource->limit = 0xffffffff; + resource->flags |= IORESOURCE_MEM; + + // Add extra prefetchable memory space + resource = new_resource(dev, 0x14); + resource->size = CONFIG_PCIEXP_HOTPLUG_PREFETCH_MEM; + resource->align = 12; + resource->gran = 12; + resource->limit = 0xffffffffffffffff; + resource->flags |= IORESOURCE_MEM | IORESOURCE_PREFETCH; + + // Add extra I/O space + resource = new_resource(dev, 0x18); + resource->size = CONFIG_PCIEXP_HOTPLUG_IO; + resource->align = 12; + resource->gran = 12; + resource->limit = 0xffff; + resource->flags |= IORESOURCE_IO; +} + +static struct device_operations pciexp_hotplug_dummy_ops = { + .read_resources = pciexp_hotplug_dummy_read_resources, +}; + +void pciexp_hotplug_scan_bridge(struct device *dev) +{ + dev->hotplug_buses = CONFIG_PCIEXP_HOTPLUG_BUSES; + + /* Normal PCIe Scan */ + pciexp_scan_bridge(dev); + + /* Add dummy slot to preserve resources, must happen after bus scan */ + struct device *dummy; + struct device_path dummy_path = { .type = DEVICE_PATH_NONE }; + dummy = alloc_dev(dev->link_list, &dummy_path); + dummy->ops = &pciexp_hotplug_dummy_ops; +} + +struct device_operations default_pciexp_hotplug_ops_bus = { + .read_resources = pci_bus_read_resources, + .set_resources = pci_dev_set_resources, + .enable_resources = pci_bus_enable_resources, + .scan_bus = pciexp_hotplug_scan_bridge, + .reset_bus = pci_bus_reset, + .ops_pci = &pciexp_bus_ops_pci, +}; +#endif /* CONFIG(PCIEXP_HOTPLUG) */ diff --git a/src/include/device/device.h b/src/include/device/device.h index 2d7400b..c3a1106 100644 --- a/src/include/device/device.h +++ b/src/include/device/device.h @@ -121,6 +121,7 @@ unsigned int disable_pcie_aspm : 1; unsigned int hidden : 1; /* set if we should hide from UI */ u8 command; + uint16_t hotplug_buses; /* Number of hotplug buses to allocate */
/* Base registers for this device. I/O, MEM and Expansion ROM */ DEVTREE_CONST struct resource *resource_list; diff --git a/src/include/device/pci_def.h b/src/include/device/pci_def.h index d906445..07ba4a2 100644 --- a/src/include/device/pci_def.h +++ b/src/include/device/pci_def.h @@ -435,6 +435,7 @@ #define PCI_EXP_LNKSTA_LT 0x800 /* Link Training */ #define PCI_EXP_LNKSTA_SLC 0x1000 /* Slot Clock Configuration */ #define PCI_EXP_SLTCAP 20 /* Slot Capabilities */ +#define PCI_EXP_SLTCAP_HPC 0x0040 /* Hot-Plug Capable */ #define PCI_EXP_SLTCTL 24 /* Slot Control */ #define PCI_EXP_SLTSTA 26 /* Slot Status */ #define PCI_EXP_RTCTL 28 /* Root Control */ diff --git a/src/include/device/pciexp.h b/src/include/device/pciexp.h index 3a9825d..4491406 100644 --- a/src/include/device/pciexp.h +++ b/src/include/device/pciexp.h @@ -26,5 +26,11 @@
extern struct device_operations default_pciexp_ops_bus;
+#if CONFIG(PCIEXP_HOTPLUG) +void pciexp_hotplug_scan_bridge(struct device *dev); + +extern struct device_operations default_pciexp_hotplug_ops_bus; +#endif /* CONFIG(PCIEXP_HOTPLUG) */ + unsigned int pciexp_find_extended_cap(struct device *dev, unsigned int cap); #endif /* DEVICE_PCIEXP_H */
Attention is currently required from: Nico Huber, Jeremy Soller. Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35946 )
Change subject: pciexp: Add support for allocating PCI express hotplug resources ......................................................................
Patch Set 6:
(1 comment)
File src/device/pciexp_device.c:
https://review.coreboot.org/c/coreboot/+/35946/comment/56aac701_33950bf8 PS6, Line 563: : /* Add dummy slot to preserve resources, must happen after bus scan */ : struct device *dummy; : struct device_path dummy_path = { .type = DEVICE_PATH_NONE }; : dummy = alloc_dev(dev->link_list, &dummy_path); : dummy->ops = &pciexp_hotplug_dummy_ops; : This causes an error: `[ERROR] NONE missing set_resources`
I have no idea if `set_resources` is really required for the dummy device, or if it's enough to introduce `DEVICE_PATH_DUMMY` which doesn't print an error during resource allocation.
@Nico, ideas?
-
Aaron Durbin (Code Review) -
Bill XIE (Code Review)
-
Brandon Breitenstein (Code Review)
-
build bot (Jenkins) (Code Review)
-
Felix Held (Code Review)
-
Jeremy Soller (Code Review)
-
Kyösti Mälkki (Code Review)
-
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Philipp Deppenwiese (Code Review)
-
Werner Zeh (Code Review)