Hi,
On Wed, Jul 29, 2026 at 02:35:24PM -0400, Atharva Tiwari wrote:
> > We should not be calling PCI functions anymore from the SW CM core. We have
> > pci.c for that.
>
> That is outside the scope of this patch, this should be another patch.
Agree. It should be preparatory patch for the series.
> > I wonder if we can put this to pci.c, rename it to tb_pci_add_links()
> > instead.
>
> Same thing as before, its outside the scope of this patch.
>
> > BTW, why you need to differentiate T2 vs. the rest of Apple x86? Don't this
> > variable do?
>
> Because the Patchset is specifically for T2 Macs.
Yes but don't x86_apple_machine is true for T2 macs as well? Why need a
separate variable?
> > I'm not fan of __free() and the like so let's not use it here.
> >
> > Also you don't need to scan all the slots. Just look for the tunneled
> > downstream ports based on their PCI IDs like we do already.
>
> Could you please point me to the "like we do already" code you're referring
> to?
Yes see below.
> > This is unrelated change.
>
> Its not. the patch specifically says T2 macs, and Titan ridge is only on T2
> Macs
> so we dont need to use has_apple_t2_chip for that.
I meant the extra empty line that your patch adds.
In addition to rename and move to pci.c (as separate patch) something like
this (completely untested) I had in mind. Does that make sense? It does not
add the Titan Ridge IDs, you can add them to the discrete part then.
diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index ddeba0861a2b..3ad6ece6b808 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -3397,32 +3397,30 @@ static const struct tb_cm_ops tb_cm_ops = {
.disconnect_xdomain_paths = tb_disconnect_xdomain_paths,
};
-/*
- * During suspend the Thunderbolt controller is reset and all PCIe
- * tunnels are lost. The NHI driver will try to reestablish all tunnels
- * during resume. This adds device links between the tunneled PCIe
- * downstream ports and the NHI so that the device core will make sure
- * NHI is resumed first before the rest.
- */
-static bool tb_apple_add_links(struct tb_nhi *nhi)
+static bool add_link(struct device *dev, struct device *nhi)
{
- struct pci_dev *nhi_pdev = to_pci_dev(nhi->dev);
- struct pci_dev *upstream, *pdev;
- bool ret;
+ const struct device_link *link;
- if (!x86_apple_machine)
- return false;
-
- switch (nhi_pdev->device) {
- case PCI_DEVICE_ID_INTEL_LIGHT_RIDGE:
- case PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C:
- case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI:
- case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI:
- break;
- default:
- return false;
+ link = device_link_add(dev, nhi, DL_FLAG_AUTOREMOVE_SUPPLIER |
+ DL_FLAG_PM_RUNTIME);
+ if (link) {
+ dev_dbg(nhi, "created link from %s\n",
+ dev_name(dev));
+ return true;
+ } else {
+ dev_warn(nhi, "device link creation from %s failed\n",
+ dev_name(dev));
}
+ return false;
+}
+
+static bool
+tb_pci_add_links_discrete(struct tb_nhi *nhi, struct pci_dev *nhi_pdev)
+{
+ struct pci_dev *upstream, *pdev;
+ bool ret;
+
upstream = pci_upstream_bridge(nhi_pdev);
while (upstream) {
if (!pci_is_pcie(upstream))
@@ -3442,30 +3440,80 @@ static bool tb_apple_add_links(struct tb_nhi *nhi)
*/
ret = false;
for_each_pci_bridge(pdev, upstream->subordinate) {
- const struct device_link *link;
-
if (!pci_is_pcie(pdev))
continue;
if (pci_pcie_type(pdev) != PCI_EXP_TYPE_DOWNSTREAM ||
!pdev->is_pciehp)
continue;
- link = device_link_add(&pdev->dev, nhi->dev,
- DL_FLAG_AUTOREMOVE_SUPPLIER |
- DL_FLAG_PM_RUNTIME);
- if (link) {
- dev_dbg(nhi->dev, "created link from %s\n",
- dev_name(&pdev->dev));
- ret = true;
- } else {
- dev_warn(nhi->dev, "device link creation from %s
failed\n",
- dev_name(&pdev->dev));
+ ret = add_link(&pdev->dev, nhi->dev);
+ }
+
+ return ret;
+}
+
+static bool
+tb_pci_add_links_integrated(struct tb_nhi *nhi, struct pci_dev *nhi_pdev)
+{
+ struct pci_bus *bus = nhi_pdev->bus;
+ struct pci_dev *pdev;
+ bool ret = false;
+
+ /*
+ * On intergrated the tunneled PCIe root ports are directly
+ * under the host bridge.
+ */
+ if (!pci_is_root_bus(bus))
+ return false;
+
+ for_each_pci_bridge(pdev, bus) {
+ switch (nhi_pdev->device) {
+ case PCI_DEVICE_ID_INTEL_ICL_NHI0:
+ if (pdev->device == 0x8a1d || pdev->device == 0x8a1fb)
+ ret = add_link(&pdev->dev, nhi->dev);
+ break;
+ case PCI_DEVICE_ID_INTEL_ICL_NHI1:
+ if (pdev->device == 0x8a21 || pdev->device == 0x8a23)
+ ret = add_link(&pdev->dev, nhi->dev);
+ break;
+ default:
+ break;
}
}
return ret;
}
+/*
+ * During suspend the Thunderbolt controller is reset and all PCIe
+ * tunnels are lost. The NHI driver will try to reestablish all tunnels
+ * during resume. This adds device links between the tunneled PCIe
+ * downstream ports and the NHI so that the device core will make sure
+ * NHI is resumed first before the rest.
+ */
+static bool tb_apple_add_links(struct tb_nhi *nhi)
+{
+ struct pci_dev *nhi_pdev = to_pci_dev(nhi->dev);
+
+ if (!x86_apple_machine)
+ return false;
+
+ switch (nhi_pdev->device) {
+ case PCI_DEVICE_ID_INTEL_LIGHT_RIDGE:
+ case PCI_DEVICE_ID_INTEL_CACTUS_RIDGE_4C:
+ case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI:
+ case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI:
+ return tb_pci_add_links_discrete(nhi, nhi_pdev);
+
+ case PCI_DEVICE_ID_INTEL_ICL_NHI0:
+ case PCI_DEVICE_ID_INTEL_ICL_NHI1:
+ return tb_pci_add_links_integrated(nhi, nhi_pdev);
+
+ default:
+ return false;
+ }
+}
+
struct tb *tb_probe(struct tb_nhi *nhi)
{
struct tb_cm *tcm;
