thunderbolt: Add support for INTEL_FALCON_RIDGE_2C controller

PATCH In review Xavier Gnata 3 participants 6 messages Jul 12, 2016 – Jul 13, 2016

Subsystems (2)

PCI Thunderbolt driver

lore

Share & tools

lore

Xavier Gnata

author

2016-07-12 19:38 UTC #0 lore raw

PATCH 1/1 thunderbolt: Add support for INTEL_FALCON_RIDGE_2C controller

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(+)

drivers/thunderbolt/nhi.c +0 -0

index 9c15344..b3d55ec 100644
@@ -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,}
};

Signature · 6 lines

2.7.4


---
 drivers/pci/quirks.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

drivers/pci/quirks.c +12 -8

index ee72ebe..e280351 100644
@@ -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,

Signature · 1 line

2.7.4

Lukas Wunner

2016-07-12 21:13 UTC #1 lore raw

Quoted 3 lines · Xavier Gnata

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.

Quoted 25 lines

---
drivers/thunderbolt/nhi.c | 6 ++++++

1 file changed, 6 insertions(+)

drivers/thunderbolt/nhi.c +6 -0

index 9c15344..b3d55ec 100644
@@ -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,}
 };

Signature · 1 line

2.7.4

It looks like the indentation got mangled here, perhaps a Thunderbird issue or copy-pasta.

Quoted 25 lines

---
 drivers/pci/quirks.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

drivers/pci/quirks.c +0 -5

index ee72ebe..e280351 100644
@@ -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)

drivers/pci/quirks.c +1 -2

-                   || nhi->subsystem_vendor != 0x2222
-                   || nhi->subsystem_device != 0x1111)
+       if (!nhi || nhi->vendor != PCI_VENDOR_ID_INTEL)

drivers/pci/quirks.c static void quirk_apple_wait_for_thunderbolt(struct pci_dev *dev) · line 3333

Quoted 10 lines

drivers/pci/quirks.c +9 -0

                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

Quoted 18 lines

drivers/pci/quirks.c +2 -0

+               || 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:

drivers/pci/quirks.c +3 -0

@@ -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,

Signature · 1 line

2.7.4

Lukas Wunner

2016-07-12 21:21 UTC #2 lore raw

Quoted 4 lines · Lukas Wunner

Quoted 3 lines · Xavier Gnata
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

[...]

Quoted 3 lines

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

Andreas Noever

maintainer

2016-07-12 22:18 UTC #3 lore raw

In reply to Lukas Wunner

Quoted 74 lines · Lukas Wunner

Quoted 3 lines · Xavier Gnata

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.

Quoted 23 lines

---
drivers/thunderbolt/nhi.c | 6 ++++++

1 file changed, 6 insertions(+)

drivers/thunderbolt/nhi.c +6 -1

index 9c15344..b3d55ec 100644
@@ -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,}
 };
 
--

Quoted 2 lines

2.7.4

It looks like the indentation got mangled here, perhaps a Thunderbird issue or copy-pasta.

Quoted 25 lines

---
 drivers/pci/quirks.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

drivers/pci/quirks.c +0 -5

index ee72ebe..e280351 100644
@@ -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)

drivers/pci/quirks.c +1 -2

-                   || nhi->subsystem_vendor != 0x2222
-                   || nhi->subsystem_device != 0x1111)
+       if (!nhi || nhi->vendor != PCI_VENDOR_ID_INTEL)

drivers/pci/quirks.c static void quirk_apple_wait_for_thunderbolt(struct pci_dev *dev) · line 3333

Quoted 10 lines

drivers/pci/quirks.c +9 -0

                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

Quoted 6 lines

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).

Quoted 4 lines

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

Quoted 27 lines

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

Quoted 18 lines

drivers/pci/quirks.c +2 -0

+               || 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:

drivers/pci/quirks.c +3 -1

@@ -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

Lukas Wunner

2016-07-13 17:05 UTC #4 lore raw

Quoted 3 lines · Andreas Noever

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/[email protected]/msg1170039.html

Best regards,

Lukas

Xavier Gnata

author

2016-07-13 21:46 UTC #5 lore raw

Quoted 30 lines · Andreas Noever

Quoted 3 lines
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

Quoted 9 lines

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/[email protected]/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