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

[Mika Westerberg]

On Mon, Jun 05, 2017 at 02:07:56PM +0200, Lukas Wunner wrote:
> 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.

It is kind of separate thing - here we add the the missing functionality
so that PC users can finally get their TBT devices connected.

We can do those things later when the basic functionality is in place
IMHO.

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

OK

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

Right, but do we need to do it now before we have got any feedback from
users using Macs with Alpine Ridge and ICM? We can always add the module
parameter later if really needed.

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

Yes it is. The TBT3 devices (well even TBT2 devices) supporting secure
connect for example implement it in ICM firmware.

With these patches you can even upgrade that ICM firmware on devices and
that also works if you are using the native connection manager.