Re: [PATCH v5 2/2] thunderbolt: Add device links for Apple T2 NHI

[Mika Westerberg <mika.westerberg@linux.intel.com>]

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;