[PATCH v4] thunderbolt: Call SXFP() on Apple systems with Alpine Ridge when suspending

[PATCH v4] thunderbolt: Call SXFP() on Apple systems with Alpine Ridge when suspending

[Francisco Beltrán Millalén]

On Apple systems with Alpine Ridge, suspending to RAM with a device
connected to a Type-C port leaves the PCIe link to the upstream port of
the controller down on resume, and the ports are lost until reboot.
Calling the SXFP(0) ACPI method under the NHI before entering sleep
avoids that.

The firmware can set bit 26 of plug events CS_20 when it resumes the
NHI. While it is set SXFP(0) turns the router off and it does not come
back on resume, so clear the bit first and skip SXFP(0) if that fails.

Clearing the bit needs the control channel, which is already stopped
when the existing suspend_noirq hook of the NHI runs. Add a new NHI
hook, pre_suspend_noirq, that is called before the domain is suspended,
and use it for Alpine Ridge.

Signed-off-by: Francisco Beltrán Millalén <fbeltranmillalen@gmail.com>
---
Thanks for the review, Mika, and for the pointer to next.

v4:
- Call pre_suspend_noirq from __nhi_suspend_noirq() in nhi.c instead of
  from tb_domain_suspend_noirq(), so the domain code no longer calls
  into the NHI.  tb_acpi_force_power() now takes tb->lock itself.
- Dropped .reset_interface from ar_nhi_ops, as it is gone from next.
- Rebased on thunderbolt.git next (v7.4-rc1).

With the call moved, tb->lock is not held between SXFP(0) and the
suspend of the domain.  The control channel interrupt stays enabled
and hotplug_active is only cleared in tb_suspend_noirq(), so a hotplug
event in that window would still be handled.  I found this by reading
the code; in the cycles below the window was at most a few milliseconds
and no event came in.  How would you like this handled?

The note on v2 about SXFP(0) and the xHCI on the side with nothing
connected still applies.  In one of the cycles below SXFP(0) ran before
that xHCI was put into D3hot, which then failed with "device
inaccessible", and it resumed fine.

v3: https://lore.kernel.org/all/20261021141444.10632-1-fbeltranmillalen@gmail.com/
v2: https://lore.kernel.org/all/20261019202900.39062-1-fbeltranmillalen@gmail.com/
v1: https://lore.kernel.org/all/20261019134851.7496-1-fbeltranmillalen@gmail.com/

Testing:
I tested it the same way as v3, on a MacBookPro14,3 with 6.18.49 and a
backport of this patch, plus the PCI series and the ACPICA change
described in v3.  All cycles were 30 second suspends to RAM.

- Three cycles with a USB 3 disk on one side or the other, after a
  first one that I don't count.  Both domains came back each time, with
  both upstream port links at 8 GT/s x4, and the disk could be read.
- One cycle with nothing connected.  Both domains came back, and the
  disk came up at USB 3 speed when plugged in on either side afterwards.

With dynamic debug enabled, in every cycle "calling SXFP" on each NHI
was followed directly by "suspending..." from the same domain.  There
was no kernel WARNING during the whole boot.  The version for next was
only built (with W=1), not booted.  The cases listed as not tested in
v3 are still not tested.

 drivers/thunderbolt/acpi.c    | 49 +++++++++++++++++++++++++++++++++++
 drivers/thunderbolt/nhi.c     |  3 +++
 drivers/thunderbolt/nhi.h     |  2 ++
 drivers/thunderbolt/pci.c     | 44 +++++++++++++++++++++++++------
 drivers/thunderbolt/switch.c  | 32 +++++++++++++++++++++++
 drivers/thunderbolt/tb.h      |  4 +++
 drivers/thunderbolt/tb_regs.h |  3 +++
 7 files changed, 129 insertions(+), 8 deletions(-)

diff --git a/drivers/thunderbolt/acpi.c b/drivers/thunderbolt/acpi.c
index be91bc604900..7efe02385e01 100644
--- a/drivers/thunderbolt/acpi.c
+++ b/drivers/thunderbolt/acpi.c
@@ -8,6 +8,7 @@
 
 #include <linux/acpi.h>
 #include <linux/pm_runtime.h>
+#include <linux/suspend.h>
 
 #include "tb.h"
 
@@ -280,6 +281,54 @@ int tb_acpi_power_off_retimers(struct tb_port *port)
 	return tb_acpi_retimer_set_power(port, false);
 }
 
+/**
+ * tb_acpi_force_power() - Call SXFP(0) when entering system sleep
+ * @tb: Domain that is being suspended
+ *
+ * On Apple systems with Alpine Ridge the PCIe link to the upstream port
+ * of the controller does not come up on resume from system sleep if a
+ * device is connected, unless SXFP(0) ("Sx Force Power") under the NHI
+ * is called before entering it. Bit 26 of plug events CS_20 must be
+ * clear when SXFP(0) is called, otherwise the router is turned off and
+ * does not come back on resume.
+ *
+ * Must be called before the domain is suspended: clearing the bit needs
+ * the control channel, which is stopped by then.
+ */
+void tb_acpi_force_power(struct tb *tb)
+{
+	struct tb_switch *sw = tb->root_switch;
+	struct acpi_device *adev;
+	acpi_status status;
+
+	if (tb_switch_is_icm(sw))
+		return;
+
+	/*
+	 * Only when firmware handles system suspend, as in
+	 * quirk_apple_poweroff_thunderbolt(): the OS does not know how to
+	 * power the controller back on. pm_suspend_in_progress() excludes
+	 * hibernation.
+	 */
+	if (!pm_suspend_in_progress() || !pm_suspend_via_firmware())
+		return;
+
+	adev = ACPI_COMPANION(tb->nhi->dev);
+	if (!adev || !acpi_has_method(adev->handle, "SXFP"))
+		return;
+
+	guard(mutex)(&tb->lock);
+
+	if (tb_switch_set_bit26(sw, false))
+		return;
+
+	tb_sw_dbg(sw, "ACPI: calling SXFP\n");
+	status = acpi_execute_simple_method(adev->handle, "SXFP", 0);
+	if (ACPI_FAILURE(status))
+		tb_sw_warn(sw, "ACPI: SXFP failed: %s\n",
+			   acpi_format_exception(status));
+}
+
 static bool tb_acpi_bus_match(struct device *dev)
 {
 	return tb_is_switch(dev) || tb_is_usb4_port_device(dev);
diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index e99a3fcc4a29..61ebb64cd16f 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -1085,6 +1085,9 @@ static int __nhi_suspend_noirq(struct device *dev, bool wakeup)
 	struct tb_nhi *nhi = tb->nhi;
 	int ret;
 
+	if (nhi->ops->pre_suspend_noirq)
+		nhi->ops->pre_suspend_noirq(nhi);
+
 	ret = tb_domain_suspend_noirq(tb);
 	if (ret)
 		return ret;
diff --git a/drivers/thunderbolt/nhi.h b/drivers/thunderbolt/nhi.h
index 88af9ab9fde7..1f4b774c7ec5 100644
--- a/drivers/thunderbolt/nhi.h
+++ b/drivers/thunderbolt/nhi.h
@@ -42,6 +42,7 @@ extern const struct dev_pm_ops nhi_pm_ops;
  * struct tb_nhi_ops - NHI specific optional operations
  * @init: NHI specific initialization
  * @add_links: Setup device links for tunneling native protocols
+ * @pre_suspend_noirq: NHI specific hook called before the domain is suspended
  * @suspend_noirq: NHI specific suspend_noirq hook
  * @resume_noirq: NHI specific resume_noirq hook
  * @runtime_suspend: NHI specific runtime_suspend hook
@@ -57,6 +58,7 @@ extern const struct dev_pm_ops nhi_pm_ops;
 struct tb_nhi_ops {
 	int (*init)(struct tb_nhi *nhi);
 	bool (*add_links)(struct tb_nhi *nhi);
+	void (*pre_suspend_noirq)(struct tb_nhi *nhi);
 	int (*suspend_noirq)(struct tb_nhi *nhi, bool wakeup);
 	int (*resume_noirq)(struct tb_nhi *nhi);
 	int (*runtime_suspend)(struct tb_nhi *nhi);
diff --git a/drivers/thunderbolt/pci.c b/drivers/thunderbolt/pci.c
index 4408229d8ac7..9715ce1ec731 100644
--- a/drivers/thunderbolt/pci.c
+++ b/drivers/thunderbolt/pci.c
@@ -338,6 +338,26 @@ static const struct tb_nhi_ops pci_nhi_default_ops = {
 	.init_interrupts = nhi_pci_init_msi,
 };
 
+/* Alpine Ridge specific NHI operations */
+
+static void ar_nhi_pre_suspend_noirq(struct tb_nhi *nhi)
+{
+	if (x86_apple_machine)
+		tb_acpi_force_power(dev_get_drvdata(nhi->dev));
+}
+
+static const struct tb_nhi_ops ar_nhi_ops = {
+	.add_links = nhi_pci_add_links,
+	.pre_suspend_noirq = ar_nhi_pre_suspend_noirq,
+	.pre_nvm_auth = nhi_pci_start_dma_port,
+	.post_nvm_auth = nhi_pci_complete_dma_port,
+	.request_ring_irq = nhi_pci_ring_request_msix,
+	.release_ring_irq = nhi_pci_ring_release_msix,
+	.shutdown = nhi_pci_release_irq,
+	.is_present = nhi_pci_is_present,
+	.init_interrupts = nhi_pci_init_msi,
+};
+
 /* Ice Lake specific NHI operations */
 
 #define ICL_LC_MAILBOX_TIMEOUT	500 /* ms */
@@ -654,14 +674,22 @@ static struct pci_device_id nhi_ids[] = {
 	},
 
 	/* Thunderbolt 3 */
-	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_NHI) },
-	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_NHI) },
-	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_USBONLY_NHI) },
-	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_NHI) },
-	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_USBONLY_NHI) },
-	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_NHI) },
-	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_NHI) },
-	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_USBONLY_NHI) },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_2C_NHI),
+	  .driver_data = (kernel_ulong_t)&ar_nhi_ops },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_4C_NHI),
+	  .driver_data = (kernel_ulong_t)&ar_nhi_ops },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_USBONLY_NHI),
+	  .driver_data = (kernel_ulong_t)&ar_nhi_ops },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_NHI),
+	  .driver_data = (kernel_ulong_t)&ar_nhi_ops },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_LP_USBONLY_NHI),
+	  .driver_data = (kernel_ulong_t)&ar_nhi_ops },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_NHI),
+	  .driver_data = (kernel_ulong_t)&ar_nhi_ops },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_NHI),
+	  .driver_data = (kernel_ulong_t)&ar_nhi_ops },
+	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_USBONLY_NHI),
+	  .driver_data = (kernel_ulong_t)&ar_nhi_ops },
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI) },
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI) },
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ICL_NHI0),
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index d22d8db8f890..0b29e6ebb597 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -4034,6 +4034,38 @@ int tb_switch_xhci_connect(struct tb_switch *sw)
 	return 0;
 }
 
+/**
+ * tb_switch_set_bit26() - Set or clear bit 26 of plug events CS_20
+ * @sw: Alpine Ridge router
+ * @value: %true to set the bit, %false to clear it
+ *
+ * On Apple systems the firmware can set this bit when it resumes the
+ * NHI. While it is set, SXFP(0) turns the router off and it does not
+ * come back on resume.
+ *
+ * Return: %0 on success, negative errno otherwise.
+ */
+int tb_switch_set_bit26(struct tb_switch *sw, bool value)
+{
+	unsigned int offset = sw->cap_plug_events + TB_PLUG_EVENTS_CS_20;
+	u32 val;
+	int ret;
+
+	ret = tb_sw_read(sw, &val, TB_CFG_SWITCH, offset, 1);
+	if (ret)
+		return ret;
+
+	if (!!(val & TB_PLUG_EVENTS_CS_20_BIT26) == value)
+		return 0;
+
+	if (value)
+		val |= TB_PLUG_EVENTS_CS_20_BIT26;
+	else
+		val &= ~TB_PLUG_EVENTS_CS_20_BIT26;
+
+	return tb_sw_write(sw, &val, TB_CFG_SWITCH, offset, 1);
+}
+
 /**
  * tb_switch_xhci_disconnect() - Disconnect internal xHCI
  * @sw: Router whose xHCI to disconnect
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index e86845762e22..9900d0018174 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -1099,6 +1099,8 @@ int tb_switch_pcie_l1_enable(struct tb_switch *sw);
 int tb_switch_xhci_connect(struct tb_switch *sw);
 void tb_switch_xhci_disconnect(struct tb_switch *sw);
 
+int tb_switch_set_bit26(struct tb_switch *sw, bool value);
+
 int tb_port_state(struct tb_port *port);
 int tb_wait_for_port(struct tb_port *port, bool wait_if_unplugged);
 int tb_port_add_nfc_credits(struct tb_port *port, int credits);
@@ -1524,6 +1526,7 @@ int tb_acpi_init(void);
 void tb_acpi_exit(void);
 int tb_acpi_power_on_retimers(struct tb_port *port);
 int tb_acpi_power_off_retimers(struct tb_port *port);
+void tb_acpi_force_power(struct tb *tb);
 #else
 static inline bool tb_acpi_add_links(struct tb_nhi *nhi) { return false; }
 
@@ -1537,6 +1540,7 @@ static inline int tb_acpi_init(void) { return 0; }
 static inline void tb_acpi_exit(void) { }
 static inline int tb_acpi_power_on_retimers(struct tb_port *port) { return 0; }
 static inline int tb_acpi_power_off_retimers(struct tb_port *port) { return 0; }
+static inline void tb_acpi_force_power(struct tb *tb) { }
 #endif
 
 #ifdef CONFIG_DEBUG_FS
diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
index 92d893634d2b..0940e8b12d89 100644
--- a/drivers/thunderbolt/tb_regs.h
+++ b/drivers/thunderbolt/tb_regs.h
@@ -563,6 +563,9 @@ struct tb_regs_hop {
 #define TB_PLUG_EVENTS_CS_1_LOW_DPIN_DISABLE	BIT(5)
 #define TB_PLUG_EVENTS_CS_1_HIGH_DPIN_DISABLE	BIT(6)
 
+#define TB_PLUG_EVENTS_CS_20			0x14
+#define TB_PLUG_EVENTS_CS_20_BIT26		BIT(26)
+
 #define TB_PLUG_EVENTS_PCIE_WR_DATA		0x1b
 #define TB_PLUG_EVENTS_PCIE_CMD			0x1c
 #define TB_PLUG_EVENTS_PCIE_CMD_DW_OFFSET_MASK	GENMASK(9, 0)

base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4