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

[PATCH v3] 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 suspend_noirq hook of the NHI runs. So add a new NHI hook,
pre_suspend_noirq, that runs just 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 idea of moving this to pci.c.

I did not use the suspend_noirq hook in the end, because it runs too
late.  By the time it is called, the domain has been suspended and the
control channel is stopped, so bit 26 can no longer be cleared.  I also
tried clearing the bit earlier and calling SXFP(0) from suspend_noirq,
but then the side with the disk did not come back on resume.  I only
ran that once, so I did not go further with it.

Instead, this adds a new hook that runs just before the domain is
suspended, while the control channel still works.  That is about the
same point where v2 called SXFP(0).  ar_nhi_ops provides it with the
code you suggested.  If you would prefer another way, please let me
know.

v3:
- Renamed the function to tb_acpi_force_power(), and moved the Apple
  and Alpine Ridge checks to ar_nhi_ops in pci.c, as you suggested.
  tb.c is no longer changed.
- Use the new pre_suspend_noirq hook instead of suspend_noirq (see
  above).
- Return early when the ICM firmware is in use, as v2 only ran with the
  software connection manager.
- ar_nhi_ops keeps the other default NHI operations.
- Moved the declaration and added the empty line you asked for.

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

The note on v2 about SXFP(0) and the xHCI on the side with nothing
connected still applies, as SXFP(0) is called at about the same point.

Testing:
I tested it on a MacBookPro14,3 (two Alpine Ridge controllers, software
connection manager) with 6.18.49 and a backport of this patch.  The kernel also carried the PCI series I sent
alongside v2:
https://lore.kernel.org/all/20261019202445.38259-1-fbeltranmillalen@gmail.com/
and a local change equivalent to the ACPICA fix mentioned in v1,
without which the firmware does not set bit 26 here.  The old PCI
quirk was switched off.  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.  In each cycle SXFP(0) was called on both NHIs before their
  control channel was stopped.
- One cycle with nothing connected.  Both domains came back, and the
  disk worked at USB 3 speed when plugged into either port on one side
  afterwards.  In the lower port it first came up at USB 2 through the
  PCH xHCI, twice; with the connector flipped it came up at USB 3 on
  the controller's xHCI.  I think that is the cable.

There was no kernel WARNING during the whole boot.  The version for
your tree was only built (with W=1), not booted, so the default NHI
operations copied into ar_nhi_ops were not run here.

Not tested:
- Real Thunderbolt devices, only the USB 3 disk behind the
  controller's xHCI.
- Other Alpine Ridge variants.
- A suspend aborted after SXFP(0).
- An NHI that is runtime suspended when the system suspends.  Here
  both NHIs were always active.  If one is runtime suspended, the PM
  core may skip its suspend_noirq (direct_complete), and then SXFP(0)
  is not called.
- Suspend-to-idle and hibernation; the checks in
  tb_acpi_force_power() keep the patch out of both.

As noted in v1, a device plugged in while the system is suspended does
not wake it.  I did not repeat that with this version.

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

diff --git a/drivers/thunderbolt/acpi.c b/drivers/thunderbolt/acpi.c
index 53546bc477a5..824938a459d5 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"
 
@@ -279,6 +280,52 @@ 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 with @tb->lock held, before the domain is suspended:
+ * clearing the bit needs the control channel, and SXFP(0) does not help
+ * once the router has been put to sleep.
+ */
+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;
+
+	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/domain.c b/drivers/thunderbolt/domain.c
index 12c88509a54f..a9a6d63bf414 100644
--- a/drivers/thunderbolt/domain.c
+++ b/drivers/thunderbolt/domain.c
@@ -533,6 +533,8 @@ int tb_domain_suspend_noirq(struct tb *tb)
 	 * we actually have stopped the domain and the control channel.
 	 */
 	mutex_lock(&tb->lock);
+	if (tb->nhi->ops->pre_suspend_noirq)
+		tb->nhi->ops->pre_suspend_noirq(tb->nhi);
 	if (tb->cm_ops->suspend_noirq)
 		ret = tb->cm_ops->suspend_noirq(tb);
 	if (!ret)
diff --git a/drivers/thunderbolt/nhi.h b/drivers/thunderbolt/nhi.h
index b2e2e2c413b2..6f66a2608d06 100644
--- a/drivers/thunderbolt/nhi.h
+++ b/drivers/thunderbolt/nhi.h
@@ -44,6 +44,8 @@ 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: hook to run in suspend_noirq before the domain is
+ *		       suspended, while the control channel is still running
  * @suspend_noirq: NHI specific suspend_noirq hook
  * @resume_noirq: NHI specific resume_noirq hook
  * @runtime_suspend: NHI specific runtime_suspend hook
@@ -60,6 +62,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 e40d4d6af071..c25f33221df1 100644
--- a/drivers/thunderbolt/pci.c
+++ b/drivers/thunderbolt/pci.c
@@ -360,6 +360,27 @@ static const struct tb_nhi_ops pci_nhi_default_ops = {
 	.reset_interface = nhi_reset_interface,
 };
 
+/* 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,
+	.reset_interface = nhi_reset_interface,
+};
+
 /* Ice Lake specific NHI operations */
 
 #define ICL_LC_MAILBOX_TIMEOUT	500 /* ms */
@@ -677,14 +698,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 4b8bcba7bd2f..44141b19c1f2 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -4027,6 +4027,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)