diff mbox

thunderbolt: Add support for INTEL_FALCON_RIDGE_2C controller

Message ID 5785474B.6070608@gmail.com
State Not Applicable
Headers show

Commit Message

Xavier Gnata July 12, 2016, 7:38 p.m. UTC 
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(+)

         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,

Comments

Lukas Wunner July 12, 2016, 9:13 p.m. UTC | #1 
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
--
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
Lukas Wunner July 12, 2016, 9:21 p.m. UTC | #2 
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
--
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
Andreas Noever July 12, 2016, 10:17 p.m. UTC | #3 
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
--
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
Lukas Wunner July 13, 2016, 5:04 p.m. UTC | #4 
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
--
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
Xavier Gnata July 13, 2016, 9:46 p.m. UTC | #5 
> 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
--
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
diff mbox

Patch

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;