Re: [PATCH v3 19/27] thunderbolt: Add new Thunderbolt PCI IDs

[Lukas Wunner]

On Mon, Jun 05, 2017 at 12:32:49PM +0300, Mika Westerberg wrote:
> On Mon, Jun 05, 2017 at 10:14:37AM +0200, Lukas Wunner wrote:
> > On Fri, Jun 02, 2017 at 05:05:16PM +0300, Mika Westerberg wrote:
> > > --- a/drivers/thunderbolt/nhi.h
> > > +++ b/drivers/thunderbolt/nhi.h
> > > @@ -143,4 +143,21 @@ static inline int ring_tx(struct tb_ring *ring, struct ring_frame *frame)
> > >  	return __ring_enqueue(ring, frame);
> > >  }
> > >  
> > > +/*
> > > + * PCI IDs used in this driver from Win Ridge forward. There is no
> > > + * need for the PCI quirk anymore as we will use ICM also on Apple
> > > + * hardware.
> > > + */
> > 
> > I wrote a patch a few months ago which replaces the PCI quirk with
> > device links:
> > 
> > https://github.com/l1k/linux/commit/8e2e7eaa1163
> > 
> > I was going to upstream this sometime this year, it probably would
> > have been better if I had done that already but I wasn't expecting
> > your series.
> > 
> > In any case, all Thunderbolt PCI device IDs can then be moved to a
> > header in drivers/thunderbolt/, except for a few TB1 devices which
> > are referenced by quirk_thunderbolt_hotplug_msi() and
> > quirk_apple_poweroff_thunderbolt().
> 
> OK, but that should be a separate patch, right? On top of your device
> links patch.

Yes.  If you like the device links patch, feel free to include it
in your series or modify it as you see fit.

I forgot to mention, on Alpine Ridge there's an additional downstream
bridge with an XHCI controller below it.  A device link will also be
established from that downstream bridge to the NHI.  I'm not sure if
that is actually necessary, perhaps it's even undesirable.


> > As to using ICM on Apple hardware, I've heard from people with
> > Alpine Ridge MacBook Pros that the native (i.e. non-ICM) driver
> > at least probes fine.  I've yet to hear from folks who have
> > actually tested it with any attached TB3 devices, but my expectation
> > would be that it should work fine in native mode since the protocol
> > seems to be the same.
> 
> I have one Mac with Alpine Ridge (well 4 Thunderbolt ports, 2 Alpine
> Ridges) and it indeed works in native mode but it can tunnel only one
> device, no display port.

Yes.  Those are limitations of the native driver.  It doesn't support
chaining and DP tunnels yet.  If chained devices and DP devices are
present on boot, then the EFI NHI driver (on Macs) will establish the
tunnels and thunderbolt.ko will inherit them.  Once the devices are
unplugged and replugged, the tunnels are gone and cannot be re-established
until the machine is rebooted.


> However, starting ICM on them allows you to connect up to 6 devices
> including display port. It also allows cross-domain connections where we
> can implement things like Amir's networking driver.
> 
> That's the reason we did it this way - to get Thunderbolt working the
> same way in Linux than it works on Macs running OS X :)

Yes, it's useful to have ICM-based Thunderbolt on Macs and personally
I'm totally fine with making it the default for Macs which support it.
(I can't speak for Andreas, obviously.)

However it would be great to give people the *choice* between ICM versus
native mode, for at least two reasons:

(1) Native mode uses free software.  (I assume the ICM firmware remains
    closed source.)

(2) Native mode allows more versatility, e.g. how PCI tunnels are set up
    to chained devices: PCI fanout or PCI direct routing, see:
    https://developer.apple.com/library/content/documentation/HardwareDrivers/Conceptual/ThunderboltDevGuide/Basics/Basics.html

    Apple supports traffic prioritization to enable audio over Thunderbolt
    with higher accuracy / minimal skew, I assume their choice to use
    native mode was largely motivated by being able to support specialized
    applications like that which are difficult or perhaps impossible to
    implement in firmware:
    http://pdfpiw.uspto.gov/.piw?Docid=09015384


By the way, you wrote in one of the commit messages that ICM is
"a firmware running on the host controller".  Is it really running on
the *controller* (i.e. Thunderbolt chip)?  My understanding was that
ICM is part of the BIOS and executed on the host CPU in System Management
Mode.

Thanks,

Lukas