| Message ID | 20191001113830.13028-9-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | thunderbolt: Add support for USB4 | expand |
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... > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > 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? > +{ > + 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. thanks, greg k-h
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@linux.intel.com> > > --- > > 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.
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) +{ + 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) +{ + switch (sw->config.device_id) { + case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_BRIDGE: + case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_BRIDGE: + case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_DD_BRIDGE: + return true; + default: + return false; + } +} + /** * tb_switch_is_icm() - Is the switch handled by ICM firmware * @sw: Switch to check
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. Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/thunderbolt/tb.c | 4 +++- drivers/thunderbolt/tb.h | 25 +++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-)