Re: [RFC PATCH 08/22] thunderbolt: Add downstream PCIe port mappings for Alpine and Titan Ridge

[Mika Westerberg]

On Tue, Oct 01, 2019 at 02:40:54PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Oct 01, 2019 at 02:38:16PM +0300, Mika Westerberg wrote:
> > In order to keep PCIe hierarchies consistent across hotplugs, add
> > hard-coded PCIe downstream port to Thunderbolt port for Alpine Ridge and
> > Titan Ridge as well.
> 
> Oh, that makes me nervous, how could a hard-coded pcie port ever get set
> up incorrectly :)
> 
> How do we "know" that this is correct?  Is there any ACPI guarantees
> that this "always will be so"?  If not, we all know someone will mess
> this up...

For Alpine Ridge and Titan Ridge the PCIe ports are always the same.

Basically what this is about is that you have up to two Thunderbolt
ports (type-C ports). When you plug in Thunderbolt device and PCIe gets
tunneled, the PCIe hierarchy always is always extended from the same
PCIe downstream port.

If we don't do this then the PCIe device may be changing its address
each plug/unplug. Also for older generations (that code is already in
mainline) there are PCIe downstream ports that do not have enough
resources for additional devices.

> > Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/thunderbolt/tb.c |  4 +++-
> >  drivers/thunderbolt/tb.h | 25 +++++++++++++++++++++++++
> >  2 files changed, 28 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> > index dbbe9afb9fb7..704455a4f763 100644
> > --- a/drivers/thunderbolt/tb.c
> > +++ b/drivers/thunderbolt/tb.c
> > @@ -344,10 +344,12 @@ static struct tb_port *tb_find_pcie_down(struct tb_switch *sw,
> >  		 * Hard-coded Thunderbolt port to PCIe down port mapping
> >  		 * per controller.
> >  		 */
> > -		if (tb_switch_is_cr(sw))
> > +		if (tb_switch_is_cr(sw) || tb_switch_is_ar(sw))
> >  			index = !phy_port ? 6 : 7;
> >  		else if (tb_switch_is_fr(sw))
> >  			index = !phy_port ? 6 : 8;
> > +		else if (tb_switch_is_tr(sw))
> > +			index = !phy_port ? 8 : 9;
> >  		else
> >  			goto out;
> >  
> > diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> > index e641dcebd50a..dbab06551eaa 100644
> > --- a/drivers/thunderbolt/tb.h
> > +++ b/drivers/thunderbolt/tb.h
> > @@ -632,6 +632,31 @@ static inline bool tb_switch_is_fr(const struct tb_switch *sw)
> >  	}
> >  }
> >  
> > +static inline bool tb_switch_is_ar(const struct tb_switch *sw)
> 
> "ar"?  Can you spell it out?

You mean call this tb_switch_is_alpine_ridge()? Sure,  I will then do
the same for tb_switch_is_fr() and the existing ones.

> 
> > +{
> > +	switch (sw->config.device_id) {
> > +	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_BRIDGE:
> > +	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_BRIDGE:
> > +	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE:
> > +	case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_BRIDGE:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> > +static inline bool tb_switch_is_tr(const struct tb_switch *sw)
> 
> Same for "tr" please.

Sure.