* [PATCH] thunderbolt: Add support for INTEL_FALCON_RIDGE_2C controller @ 2016-07-12 19:38 Xavier Gnata 2016-07-12 21:13 ` Lukas Wunner 0 siblings, 1 reply; 6+ messages in thread From: Xavier Gnata @ 2016-07-12 19:38 UTC (permalink / raw) To: linux-kernel, Andreas Noever; +Cc: linux-pci Add support to INTEL_FALCON_RIDGE_2C controller and corresponding quirk to support suspend/resume. Tested against 4.7 master on a MacBook Air 11" 2015 --- drivers/thunderbolt/nhi.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c index 9c15344..b3d55ec 100644 --- a/drivers/thunderbolt/nhi.c +++ b/drivers/thunderbolt/nhi.c @@ -654,6 +654,12 @@ static struct pci_device_id nhi_ids[] = { .device = PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI, .subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID, }, + { + .class = PCI_CLASS_SYSTEM_OTHER << 8, .class_mask = ~0, + .vendor = PCI_VENDOR_ID_INTEL, + .device = PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI, + .subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID, + }, { 0,} }; -- 2.7.4 --- drivers/pci/quirks.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index ee72ebe..e280351 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -3320,15 +3320,19 @@ static void quirk_apple_wait_for_thunderbolt(struct pci_dev *dev) if (!sibling || !sibling->subordinate) goto out; nhi = pci_get_slot(sibling->subordinate, 0x0); - if (!nhi) - goto out; - if (nhi->vendor != PCI_VENDOR_ID_INTEL - || (nhi->device != PCI_DEVICE_ID_INTEL_LIGHT_RIDGE && - nhi->device != PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C && - nhi->device != PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI) - || nhi->subsystem_vendor != 0x2222 - || nhi->subsystem_device != 0x1111) + if (!nhi || nhi->vendor != PCI_VENDOR_ID_INTEL) goto out; + if (nhi->device != PCI_DEVICE_ID_INTEL_LIGHT_RIDGE && + nhi->device != PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C && + nhi->device != PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI && + nhi->device != PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI) + goto out; + if((nhi->device == PCI_DEVICE_ID_INTEL_LIGHT_RIDGE || + nhi->device == PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C || + nhi->device == PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI) + && (nhi->subsystem_vendor != 0x222 + || nhi->subsystem_device != 0x1111)) + goto out; dev_info(&dev->dev, "quirk: waiting for thunderbolt to reestablish PCI tunnels...\n"); device_pm_wait_for_dev(&dev->dev, &nhi->dev); out: @@ -3344,6 +3348,9 @@ DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_INTEL, DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_BRIDGE, quirk_apple_wait_for_thunderbolt); +DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_INTEL, + PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_BRIDGE, + quirk_apple_wait_for_thunderbolt); #endif static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f, -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] thunderbolt: Add support for INTEL_FALCON_RIDGE_2C controller 2016-07-12 19:38 [PATCH] thunderbolt: Add support for INTEL_FALCON_RIDGE_2C controller Xavier Gnata @ 2016-07-12 21:13 ` Lukas Wunner 2016-07-12 21:21 ` Lukas Wunner 2016-07-12 22:17 ` Andreas Noever 0 siblings, 2 replies; 6+ messages in thread From: Lukas Wunner @ 2016-07-12 21:13 UTC (permalink / raw) To: Xavier Gnata; +Cc: linux-kernel, Andreas Noever, linux-pci On Tue, Jul 12, 2016 at 09:38:51PM +0200, Xavier Gnata wrote: > Add support to INTEL_FALCON_RIDGE_2C controller and corresponding quirk to > support suspend/resume. > Tested against 4.7 master on a MacBook Air 11" 2015 Nice, thanks for doing this. See below for some comments. > > --- > drivers/thunderbolt/nhi.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c > index 9c15344..b3d55ec 100644 > --- a/drivers/thunderbolt/nhi.c > +++ b/drivers/thunderbolt/nhi.c > @@ -654,6 +654,12 @@ static struct pci_device_id nhi_ids[] = { > .device = PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI, > .subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID, > }, > + { > + .class = PCI_CLASS_SYSTEM_OTHER << 8, .class_mask = ~0, > + .vendor = PCI_VENDOR_ID_INTEL, > + .device = PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI, > + .subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID, > + }, > { 0,} > }; > > -- > 2.7.4 > It looks like the indentation got mangled here, perhaps a Thunderbird issue or copy-pasta. > > --- > drivers/pci/quirks.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index ee72ebe..e280351 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -3320,15 +3320,19 @@ static void quirk_apple_wait_for_thunderbolt(struct > pci_dev *dev) > if (!sibling || !sibling->subordinate) > goto out; > nhi = pci_get_slot(sibling->subordinate, 0x0); > - if (!nhi) > - goto out; > - if (nhi->vendor != PCI_VENDOR_ID_INTEL > - || (nhi->device != PCI_DEVICE_ID_INTEL_LIGHT_RIDGE && > - nhi->device != PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C > && > - nhi->device != > PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI) > - || nhi->subsystem_vendor != 0x2222 > - || nhi->subsystem_device != 0x1111) > + if (!nhi || nhi->vendor != PCI_VENDOR_ID_INTEL) > goto out; > + if (nhi->device != PCI_DEVICE_ID_INTEL_LIGHT_RIDGE && > + nhi->device != PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C && > + nhi->device != PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI && > + nhi->device != PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI) > + goto out; > + if((nhi->device == PCI_DEVICE_ID_INTEL_LIGHT_RIDGE || > + nhi->device == PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C || > + nhi->device == PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI) > + && (nhi->subsystem_vendor != 0x222 ^ You're missing a 2 here ----------------------------+ The subsystem vendor/device should also not be checked on Falcon Ridge 4C, commit a42fb351ca1f ("thunderbolt: Allow loading of module on recent Apple MacBooks with thunderbolt 2 controller") removed that from the pci_device_id list but forgot to amend the quirk. Actually I'm wondering if we need to check the subsystem vendor/device id at all. If the motivation is to execute the quirk only for host controllers, this should probably be achieved by checking if the parent of the parent of the NHI has PCI_EXP_TYPE_ROOT_PORT set. Please also add PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI to tb_switch_alloc() so that the message "unsupported switch device id" is not printed for this controller. Thanks, Lukas > + || nhi->subsystem_device != 0x1111)) > + goto out; > dev_info(&dev->dev, "quirk: waiting for thunderbolt to reestablish > PCI tunnels...\n"); > device_pm_wait_for_dev(&dev->dev, &nhi->dev); > out: > @@ -3344,6 +3348,9 @@ DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_INTEL, > DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_INTEL, > PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_BRIDGE, > quirk_apple_wait_for_thunderbolt); > +DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_INTEL, > + PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_BRIDGE, > + quirk_apple_wait_for_thunderbolt); > #endif > > static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f, > -- > 2.7.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] thunderbolt: Add support for INTEL_FALCON_RIDGE_2C controller 2016-07-12 21:13 ` Lukas Wunner @ 2016-07-12 21:21 ` Lukas Wunner 2016-07-12 22:17 ` Andreas Noever 1 sibling, 0 replies; 6+ messages in thread From: Lukas Wunner @ 2016-07-12 21:21 UTC (permalink / raw) To: Xavier Gnata; +Cc: linux-kernel, Andreas Noever, linux-pci On Tue, Jul 12, 2016 at 11:13:24PM +0200, Lukas Wunner wrote: > On Tue, Jul 12, 2016 at 09:38:51PM +0200, Xavier Gnata wrote: > > Add support to INTEL_FALCON_RIDGE_2C controller and corresponding quirk to > > support suspend/resume. > > Tested against 4.7 master on a MacBook Air 11" 2015 [...] > Please also add PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI to tb_switch_alloc() > so that the message "unsupported switch device id" is not printed for this > controller. Sorry, I meant PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_BRIDGE rather than _NHI. And Falcon Ridge 4C is also missing there. Thanks, Lukas ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] thunderbolt: Add support for INTEL_FALCON_RIDGE_2C controller 2016-07-12 21:13 ` Lukas Wunner 2016-07-12 21:21 ` Lukas Wunner @ 2016-07-12 22:17 ` Andreas Noever 2016-07-13 17:04 ` Lukas Wunner 1 sibling, 1 reply; 6+ messages in thread From: Andreas Noever @ 2016-07-12 22:17 UTC (permalink / raw) To: Lukas Wunner; +Cc: Xavier Gnata, linux-kernel, linux-pci On Tue, Jul 12, 2016 at 11:13 PM, Lukas Wunner <lukas@wunner.de> wrote: > On Tue, Jul 12, 2016 at 09:38:51PM +0200, Xavier Gnata wrote: >> Add support to INTEL_FALCON_RIDGE_2C controller and corresponding quirk to >> support suspend/resume. >> Tested against 4.7 master on a MacBook Air 11" 2015 > > Nice, thanks for doing this. See below for some comments. > > >> >> --- >> drivers/thunderbolt/nhi.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c >> index 9c15344..b3d55ec 100644 >> --- a/drivers/thunderbolt/nhi.c >> +++ b/drivers/thunderbolt/nhi.c >> @@ -654,6 +654,12 @@ static struct pci_device_id nhi_ids[] = { >> .device = PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI, >> .subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID, >> }, >> + { >> + .class = PCI_CLASS_SYSTEM_OTHER << 8, .class_mask = ~0, >> + .vendor = PCI_VENDOR_ID_INTEL, >> + .device = PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI, >> + .subvendor = PCI_ANY_ID, .subdevice = PCI_ANY_ID, >> + }, >> { 0,} >> }; >> >> -- >> 2.7.4 >> > > It looks like the indentation got mangled here, perhaps a Thunderbird > issue or copy-pasta. > > >> >> --- >> drivers/pci/quirks.c | 23 +++++++++++++++-------- >> 1 file changed, 15 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c >> index ee72ebe..e280351 100644 >> --- a/drivers/pci/quirks.c >> +++ b/drivers/pci/quirks.c >> @@ -3320,15 +3320,19 @@ static void quirk_apple_wait_for_thunderbolt(struct >> pci_dev *dev) >> if (!sibling || !sibling->subordinate) >> goto out; >> nhi = pci_get_slot(sibling->subordinate, 0x0); >> - if (!nhi) >> - goto out; >> - if (nhi->vendor != PCI_VENDOR_ID_INTEL >> - || (nhi->device != PCI_DEVICE_ID_INTEL_LIGHT_RIDGE && >> - nhi->device != PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C >> && >> - nhi->device != >> PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI) >> - || nhi->subsystem_vendor != 0x2222 >> - || nhi->subsystem_device != 0x1111) >> + if (!nhi || nhi->vendor != PCI_VENDOR_ID_INTEL) >> goto out; >> + if (nhi->device != PCI_DEVICE_ID_INTEL_LIGHT_RIDGE && >> + nhi->device != PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C && >> + nhi->device != PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI && >> + nhi->device != PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI) >> + goto out; >> + if((nhi->device == PCI_DEVICE_ID_INTEL_LIGHT_RIDGE || >> + nhi->device == PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C || >> + nhi->device == PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI) >> + && (nhi->subsystem_vendor != 0x222 > ^ > You're missing a 2 here ----------------------------+ > > The subsystem vendor/device should also not be checked on Falcon Ridge 4C, > commit a42fb351ca1f ("thunderbolt: Allow loading of module on recent Apple > MacBooks with thunderbolt 2 controller") removed that from the pci_device_id > list but forgot to amend the quirk. Yeah, we noticed that (off list). I was going to send a follow up patch to fix that (just to keep the two things "Support FC_2C" and "Fix suspend/resume for FC_4C" separate). > Actually I'm wondering if we need to check the subsystem vendor/device id > at all. If the motivation is to execute the quirk only for host controllers, > this should probably be achieved by checking if the parent of the parent > of the NHI has PCI_EXP_TYPE_ROOT_PORT set. My idea was to check the device class - external thunderbolt devices do not expose their controller through pci (even if they support chaining). So all devices which match the device id and class should be actual controllers. Are thunderbolt controllers always installed directly below the root port? In theory there could be more bridges in between (a candidate for such a topology would be the mac pro which has 3 controllers). Andreas > > Please also add PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI to tb_switch_alloc() > so that the message "unsupported switch device id" is not printed for this > controller. > > Thanks, > > Lukas > >> + || nhi->subsystem_device != 0x1111)) >> + goto out; >> dev_info(&dev->dev, "quirk: waiting for thunderbolt to reestablish >> PCI tunnels...\n"); >> device_pm_wait_for_dev(&dev->dev, &nhi->dev); >> out: >> @@ -3344,6 +3348,9 @@ DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_INTEL, >> DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_INTEL, >> PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_BRIDGE, >> quirk_apple_wait_for_thunderbolt); >> +DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_INTEL, >> + PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_BRIDGE, >> + quirk_apple_wait_for_thunderbolt); >> #endif >> >> static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f, >> -- >> 2.7.4 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] thunderbolt: Add support for INTEL_FALCON_RIDGE_2C controller 2016-07-12 22:17 ` Andreas Noever @ 2016-07-13 17:04 ` Lukas Wunner 2016-07-13 21:46 ` Xavier Gnata 0 siblings, 1 reply; 6+ messages in thread From: Lukas Wunner @ 2016-07-13 17:04 UTC (permalink / raw) To: Andreas Noever; +Cc: Xavier Gnata, linux-kernel, linux-pci On Wed, Jul 13, 2016 at 12:17:33AM +0200, Andreas Noever wrote: > Are thunderbolt controllers always installed directly below the root > port? In theory there could be more bridges in between (a candidate > for such a topology would be the mac pro which has 3 controllers). Hm, good point. I failed to find lspci or dmesg output for a MacPro6,1 but I did find this diagram: http://i.imgur.com/ItIqxDY.png Turns out the 3 controllers are connected to a PCIe switch. And according to the PCIe spec, a switch consists of an upstream bridge and downstream bridges. So the parent of the Thunderbolt upstream port would be a downstream port and not a root port. :-/ Another idea would be to detect if the parent of the Thunderbolt upstream port has the VSEC 0x1234. This is only present on Thunderbolt devices, so a host controller is identifiable by the non-presence of that VSEC on its parent. Patch [01/13] of my runpm series adds a convenient is_thunderbolt flag to detect the VSEC: https://github.com/l1k/linux/commit/8148c395ef6e Generally I think it would be beneficial to replace the PCI quirk with code that lives in drivers/thunderbolt/. Here's an example what I have in mind, this is based on top of the runpm series and ensures that the NHI resumes before the hotplug ports by waking it directly from the upstream bridge: https://github.com/l1k/linux/commit/c596932608cd An even better approach would probably be Rafael's "device links" series which allows the PM core to take care of device dependencies beyond the mere parent/child relationship: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1170039.html Best regards, Lukas ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] thunderbolt: Add support for INTEL_FALCON_RIDGE_2C controller 2016-07-13 17:04 ` Lukas Wunner @ 2016-07-13 21:46 ` Xavier Gnata 0 siblings, 0 replies; 6+ messages in thread From: Xavier Gnata @ 2016-07-13 21:46 UTC (permalink / raw) To: Lukas Wunner, Andreas Noever; +Cc: linux-kernel, linux-pci > On Wed, Jul 13, 2016 at 12:17:33AM +0200, Andreas Noever wrote: >> Are thunderbolt controllers always installed directly below the root >> port? In theory there could be more bridges in between (a candidate >> for such a topology would be the mac pro which has 3 controllers). > > Hm, good point. I failed to find lspci or dmesg output for a MacPro6,1 > but I did find this diagram: > http://i.imgur.com/ItIqxDY.png > > Turns out the 3 controllers are connected to a PCIe switch. > And according to the PCIe spec, a switch consists of an upstream > bridge and downstream bridges. So the parent of the Thunderbolt > upstream port would be a downstream port and not a root port. :-/ > > > Another idea would be to detect if the parent of the Thunderbolt > upstream port has the VSEC 0x1234. This is only present on Thunderbolt > devices, so a host controller is identifiable by the non-presence of > that VSEC on its parent. Patch [01/13] of my runpm series adds a > convenient is_thunderbolt flag to detect the VSEC: > https://github.com/l1k/linux/commit/8148c395ef6e > > Generally I think it would be beneficial to replace the PCI quirk > with code that lives in drivers/thunderbolt/. Here's an example what > I have in mind, this is based on top of the runpm series and ensures > that the NHI resumes before the hotplug ports by waking it directly > from the upstream bridge: > https://github.com/l1k/linux/commit/c596932608cd > > An even better approach would probably be Rafael's "device links" > series which allows the PM core to take care of device dependencies > beyond the mere parent/child relationship: > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1170039.html > > Best regards, > > Lukas > I agree that it would be a good idea to get rid of both the subsystem vendor/device id and the quirk. I quite like the approach of https://github.com/l1k/linux/commit/c596932608cd : Do you want me to give it a try on my hardware? Rafael's approach is more ambitious and complex but even cleaner. Maybe a two step approach : First we get rid of the quirk and we support INTEL_FALCON_RIDGE_2C controller and, in a longer run, we try to implement these functional dependencies in PCI core. Anyway you are the experts :) Xavier ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-07-13 21:46 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-12 19:38 [PATCH] thunderbolt: Add support for INTEL_FALCON_RIDGE_2C controller Xavier Gnata 2016-07-12 21:13 ` Lukas Wunner 2016-07-12 21:21 ` Lukas Wunner 2016-07-12 22:17 ` Andreas Noever 2016-07-13 17:04 ` Lukas Wunner 2016-07-13 21:46 ` Xavier Gnata
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).
