| Message ID | 3e694101e5fa1551e07b5ecf92157087922e952f.1463134231.git.lukas@wunner.de (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, May 13, 2016 at 01:15:31PM +0200, Lukas Wunner wrote: > A Thunderbolt controller is a PCIe switch which, as defined in the PCIe > spec, appears to the OS "as a collection of virtual PCI-to-PCI bridges". > > We're about to add support for Apple's nonstandard ACPI methods to power > Thunderbolt controllers up and down. To facilitate that, allocate a > port service for every PCI bridge belonging to a Thunderbolt controller. > > This port service might come in handy for other use cases, e.g. device > initialization of Thunderbolt controllers. > > To understand when and how this port service will be allocated, consider > the PCI devices exposed by a Thunderbolt host controller: > > (Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI > +-- Downstream Bridge 1 -- > +-- Downstream Bridge 2 -- > ... > > The upstream and downstream bridges represent the PCIe switch and a > Thunderbolt port service will be allocated for each of them. Hotplugged > devices will appear behind the downstream bridges. The NHI (Native Host > Interface) is a virtual PCI device to manage the switch fabric and is > not relevant here. It uses class 0x88000, so it is not a PCIe port. > > Next, consider the PCI devices exposed by Thunderbolt controllers built > into hotplugged devices: > > -- Upstream Bridge ---- Downstream Bridge ---- Hotplugged device > > Again, Thunderbolt port services will be allocated for the upstream and > downstream bridge, but not for the hotplugged device, which might use > e.g. class 0x20000 if it's a Thunderbolt Ethernet adapter. I don't really *like* the portdrv infrastructure, even though we're sort of stuck with it now. It seems like all it really does is allow multiple sub-drivers to attach to a single device and share interrupts between them. And we get some extra devices in sysfs that don't fit the regular PCI model. We used to support loadable sub-drivers (pciehp, aer, etc.), but we decided that didn't really make sense (though I notice you do support thunderbolt as a module). I think we would be better off if the PCIe services (hotplug, AER, etc.) were directly integrated into the PCI core without the portdrv abstraction in the middle. But anyway, we do have portdrv, and the only question here is whether extending it for Thunderbolt is the right thing. So the question for Thunderbolt is what benefit you get from being a portdrv sub-driver. It seems like basically a way for you to hook on to PCI bridges that happen to be Thunderbolt controllers. I don't think you really use any portdrv services (other than forwarding the PM ops down to you, which a regular PCI device driver would get for free). upstream.c does a lot of ACPI stuff; I can't tell whether it has more affinity with ACPI or with PCI. I don't see any PNP IDs though, so I guess you just look for the magic method names in the ACPI device associated with some PCI device. That seems a little bit "back-door" to me; from an ASL point of view, I would think you'd want to start from a _HID and interpret the device based on that. > Signed-off-by: Lukas Wunner <lukas@wunner.de> > --- > drivers/pci/pcie/portdrv.h | 2 +- > drivers/pci/pcie/portdrv_core.c | 2 ++ > include/linux/pcieport_if.h | 2 ++ > 3 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h > index 587aef3..a0d9973 100644 > --- a/drivers/pci/pcie/portdrv.h > +++ b/drivers/pci/pcie/portdrv.h > @@ -11,7 +11,7 @@ > > #include <linux/compiler.h> > > -#define PCIE_PORT_DEVICE_MAXSERVICES 5 > +#define PCIE_PORT_DEVICE_MAXSERVICES 6 > /* > * According to the PCI Express Base Specification 2.0, the indices of > * the MSI-X table entries used by port services must not exceed 31 > diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c > index d04fb58..8cd9db8 100644 > --- a/drivers/pci/pcie/portdrv_core.c > +++ b/drivers/pci/pcie/portdrv_core.c > @@ -310,6 +310,8 @@ static int get_port_device_capability(struct pci_dev *dev) > } > if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC)) > services |= PCIE_PORT_SERVICE_DPC; > + if (dev->is_thunderbolt) > + services |= PCIE_PORT_SERVICE_TBT; > > return services; > } > diff --git a/include/linux/pcieport_if.h b/include/linux/pcieport_if.h > index afcd130..d205bd6 100644 > --- a/include/linux/pcieport_if.h > +++ b/include/linux/pcieport_if.h > @@ -23,6 +23,8 @@ > #define PCIE_PORT_SERVICE_VC (1 << PCIE_PORT_SERVICE_VC_SHIFT) > #define PCIE_PORT_SERVICE_DPC_SHIFT 4 /* Downstream Port Containment */ > #define PCIE_PORT_SERVICE_DPC (1 << PCIE_PORT_SERVICE_DPC_SHIFT) > +#define PCIE_PORT_SERVICE_TBT_SHIFT 5 /* Thunderbolt */ > +#define PCIE_PORT_SERVICE_TBT (1 << PCIE_PORT_SERVICE_TBT_SHIFT) > > struct pcie_device { > int irq; /* Service IRQ/MSI/MSI-X Vector */ > -- > 2.8.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, June 17, 2016 05:51:52 PM Bjorn Helgaas wrote: > On Fri, May 13, 2016 at 01:15:31PM +0200, Lukas Wunner wrote: > > A Thunderbolt controller is a PCIe switch which, as defined in the PCIe > > spec, appears to the OS "as a collection of virtual PCI-to-PCI bridges". > > > > We're about to add support for Apple's nonstandard ACPI methods to power > > Thunderbolt controllers up and down. To facilitate that, allocate a > > port service for every PCI bridge belonging to a Thunderbolt controller. > > > > This port service might come in handy for other use cases, e.g. device > > initialization of Thunderbolt controllers. > > > > To understand when and how this port service will be allocated, consider > > the PCI devices exposed by a Thunderbolt host controller: > > > > (Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI > > +-- Downstream Bridge 1 -- > > +-- Downstream Bridge 2 -- > > ... > > > > The upstream and downstream bridges represent the PCIe switch and a > > Thunderbolt port service will be allocated for each of them. Hotplugged > > devices will appear behind the downstream bridges. The NHI (Native Host > > Interface) is a virtual PCI device to manage the switch fabric and is > > not relevant here. It uses class 0x88000, so it is not a PCIe port. > > > > Next, consider the PCI devices exposed by Thunderbolt controllers built > > into hotplugged devices: > > > > -- Upstream Bridge ---- Downstream Bridge ---- Hotplugged device > > > > Again, Thunderbolt port services will be allocated for the upstream and > > downstream bridge, but not for the hotplugged device, which might use > > e.g. class 0x20000 if it's a Thunderbolt Ethernet adapter. > > I don't really *like* the portdrv infrastructure, even though we're > sort of stuck with it now. It seems like all it really does is allow > multiple sub-drivers to attach to a single device and share interrupts > between them. And we get some extra devices in sysfs that don't fit > the regular PCI model. We used to support loadable sub-drivers > (pciehp, aer, etc.), but we decided that didn't really make sense > (though I notice you do support thunderbolt as a module). > > I think we would be better off if the PCIe services (hotplug, AER, > etc.) were directly integrated into the PCI core without the portdrv > abstraction in the middle. But anyway, we do have portdrv, and the > only question here is whether extending it for Thunderbolt is the > right thing. > > So the question for Thunderbolt is what benefit you get from being a > portdrv sub-driver. It seems like basically a way for you to hook on > to PCI bridges that happen to be Thunderbolt controllers. I don't > think you really use any portdrv services (other than forwarding the > PM ops down to you, which a regular PCI device driver would get for > free). Moreover, this approach creates sort of a priority inversion between the Thunderbolt driver and the ports, because the driver, which really is a superior entity (as it corresponds to the switch as a whole) is caused to bind to children of the ports (ie. PCIe service devices). That leads to ordering issues in probe and then suspend/resume etc. > upstream.c does a lot of ACPI stuff; I can't tell whether it has more > affinity with ACPI or with PCI. I don't see any PNP IDs though, so I > guess you just look for the magic method names in the ACPI device > associated with some PCI device. That seems a little bit "back-door" > to me; from an ASL point of view, I would think you'd want to start > from a _HID and interpret the device based on that. Or from _ADR if the device object in question maps to a PCI device. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 17, 2016 at 05:51:52PM -0500, Bjorn Helgaas wrote: > On Fri, May 13, 2016 at 01:15:31PM +0200, Lukas Wunner wrote: > > A Thunderbolt controller is a PCIe switch which, as defined in the PCIe > > spec, appears to the OS "as a collection of virtual PCI-to-PCI bridges". > > > > We're about to add support for Apple's nonstandard ACPI methods to power > > Thunderbolt controllers up and down. To facilitate that, allocate a > > port service for every PCI bridge belonging to a Thunderbolt controller. > > > > This port service might come in handy for other use cases, e.g. device > > initialization of Thunderbolt controllers. > > > > To understand when and how this port service will be allocated, consider > > the PCI devices exposed by a Thunderbolt host controller: > > > > (Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI > > +-- Downstream Bridge 1 -- > > +-- Downstream Bridge 2 -- > > ... > > > > The upstream and downstream bridges represent the PCIe switch and a > > Thunderbolt port service will be allocated for each of them. Hotplugged > > devices will appear behind the downstream bridges. The NHI (Native Host > > Interface) is a virtual PCI device to manage the switch fabric and is > > not relevant here. It uses class 0x88000, so it is not a PCIe port. > > > > Next, consider the PCI devices exposed by Thunderbolt controllers built > > into hotplugged devices: > > > > -- Upstream Bridge ---- Downstream Bridge ---- Hotplugged device > > > > Again, Thunderbolt port services will be allocated for the upstream and > > downstream bridge, but not for the hotplugged device, which might use > > e.g. class 0x20000 if it's a Thunderbolt Ethernet adapter. > > I don't really *like* the portdrv infrastructure, even though we're > sort of stuck with it now. It seems like all it really does is allow > multiple sub-drivers to attach to a single device and share interrupts > between them. And we get some extra devices in sysfs that don't fit > the regular PCI model. We used to support loadable sub-drivers > (pciehp, aer, etc.), but we decided that didn't really make sense > (though I notice you do support thunderbolt as a module). > > I think we would be better off if the PCIe services (hotplug, AER, > etc.) were directly integrated into the PCI core without the portdrv > abstraction in the middle. But anyway, we do have portdrv, and the > only question here is whether extending it for Thunderbolt is the > right thing. > > So the question for Thunderbolt is what benefit you get from being a > portdrv sub-driver. It seems like basically a way for you to hook on > to PCI bridges that happen to be Thunderbolt controllers. I don't > think you really use any portdrv services (other than forwarding the > PM ops down to you, which a regular PCI device driver would get for > free). The assessment above is entirely correct, I'm sort of abusing the portdrv infrastructure as a way to bind to the upstream bridge. For comparison, Optimus GPUs are also suspended to D3cold with a non-standard method (i.e., not by the ACPI platform). The way we handle that is to assign a dev_pm_domain to the device using dev_pm_domain_set(). You can just think of "dev_pm_domain" as a fancy name for overriding the callbacks in pci_dev_pm_ops: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/vga/vga_switcheroo.c#n1014 I cannot use that method with Thunderbolt because dev_pm_domain_set() can only be called for unbound devices. And the upstream bridge will already have been bound (to portdrv) when thunderbolt.ko loads. I'm waiting for Rafael to weigh in if the dev_pm_domain_set() method is the right thing to do for devices which are suspended to D3cold in a non-standard way, and whether the "device not bound" restriction on dev_pm_domain_set() can be lifted. If so, I could rework this series to use that instead of binding to portdrv. > upstream.c does a lot of ACPI stuff; I can't tell whether it has more > affinity with ACPI or with PCI. I don't see any PNP IDs though, so I > guess you just look for the magic method names in the ACPI device > associated with some PCI device. That seems a little bit "back-door" > to me; from an ASL point of view, I would think you'd want to start > from a _HID and interpret the device based on that. Apple's ACPI methods to power the controller up/down are located below the NHI device in the namespace. I just use the ACPI_HANDLE() macro to get from the NHI's PCI device to its ACPI companion's handle, then find the methods below that. This avoids the need to search the namespace for a _HID or _ADR: https://github.com/l1k/linux/commit/65f56e6c8446#diff-66575f0946b607aa866a23518687f8b1R281 Best regards, Lukas -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/pcie/portdrv.h b/drivers/pci/pcie/portdrv.h index 587aef3..a0d9973 100644 --- a/drivers/pci/pcie/portdrv.h +++ b/drivers/pci/pcie/portdrv.h @@ -11,7 +11,7 @@ #include <linux/compiler.h> -#define PCIE_PORT_DEVICE_MAXSERVICES 5 +#define PCIE_PORT_DEVICE_MAXSERVICES 6 /* * According to the PCI Express Base Specification 2.0, the indices of * the MSI-X table entries used by port services must not exceed 31 diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c index d04fb58..8cd9db8 100644 --- a/drivers/pci/pcie/portdrv_core.c +++ b/drivers/pci/pcie/portdrv_core.c @@ -310,6 +310,8 @@ static int get_port_device_capability(struct pci_dev *dev) } if (pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DPC)) services |= PCIE_PORT_SERVICE_DPC; + if (dev->is_thunderbolt) + services |= PCIE_PORT_SERVICE_TBT; return services; } diff --git a/include/linux/pcieport_if.h b/include/linux/pcieport_if.h index afcd130..d205bd6 100644 --- a/include/linux/pcieport_if.h +++ b/include/linux/pcieport_if.h @@ -23,6 +23,8 @@ #define PCIE_PORT_SERVICE_VC (1 << PCIE_PORT_SERVICE_VC_SHIFT) #define PCIE_PORT_SERVICE_DPC_SHIFT 4 /* Downstream Port Containment */ #define PCIE_PORT_SERVICE_DPC (1 << PCIE_PORT_SERVICE_DPC_SHIFT) +#define PCIE_PORT_SERVICE_TBT_SHIFT 5 /* Thunderbolt */ +#define PCIE_PORT_SERVICE_TBT (1 << PCIE_PORT_SERVICE_TBT_SHIFT) struct pcie_device { int irq; /* Service IRQ/MSI/MSI-X Vector */
A Thunderbolt controller is a PCIe switch which, as defined in the PCIe spec, appears to the OS "as a collection of virtual PCI-to-PCI bridges". We're about to add support for Apple's nonstandard ACPI methods to power Thunderbolt controllers up and down. To facilitate that, allocate a port service for every PCI bridge belonging to a Thunderbolt controller. This port service might come in handy for other use cases, e.g. device initialization of Thunderbolt controllers. To understand when and how this port service will be allocated, consider the PCI devices exposed by a Thunderbolt host controller: (Root Port) ---- Upstream Bridge --+-- Downstream Bridge 0 ---- NHI +-- Downstream Bridge 1 -- +-- Downstream Bridge 2 -- ... The upstream and downstream bridges represent the PCIe switch and a Thunderbolt port service will be allocated for each of them. Hotplugged devices will appear behind the downstream bridges. The NHI (Native Host Interface) is a virtual PCI device to manage the switch fabric and is not relevant here. It uses class 0x88000, so it is not a PCIe port. Next, consider the PCI devices exposed by Thunderbolt controllers built into hotplugged devices: -- Upstream Bridge ---- Downstream Bridge ---- Hotplugged device Again, Thunderbolt port services will be allocated for the upstream and downstream bridge, but not for the hotplugged device, which might use e.g. class 0x20000 if it's a Thunderbolt Ethernet adapter. Signed-off-by: Lukas Wunner <lukas@wunner.de> --- drivers/pci/pcie/portdrv.h | 2 +- drivers/pci/pcie/portdrv_core.c | 2 ++ include/linux/pcieport_if.h | 2 ++ 3 files changed, 5 insertions(+), 1 deletion(-)