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

[PATCH] 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, so do it from suspend_late of the software connection
manager.

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.

Assisted-by: LLM
Signed-off-by: Francisco Beltrán Millalén <fbeltranmillalen@gmail.com>
---
This replaces "PCI: Extend Apple Thunderbolt power quirk to Alpine
Ridge":
https://lore.kernel.org/all/20260924132815.24430-1-fbeltranmillalen@gmail.com/

As discussed there, both parts now live in drivers/thunderbolt, and
nothing changes in drivers/pci.  I asked Bjorn to drop the PCI version,
so the two should not be applied together.

Without SXFP(0) the ports were lost in 15 of 15 cycles with a device
attached (details in the old thread).  It runs from suspend_late
because from suspend_noirq it did not help: by then the bridges below
the controller were already inaccessible.

I tested it on a MacBookPro14,3, which has two Alpine Ridge controllers
(two domains), with the software connection manager.  The kernel was
6.18.49 with a backport of this patch.  The backport uses
&nhi->pdev->dev in place of nhi->dev and adds a module parameter to
switch it on, and the PCI quirk was switched off.  That kernel also
carried my PCI save/restore series and a local change equivalent to
the ACPICA fix mentioned below.  I have not run this patch without
them.

- With a USB 3 disk on a Type-C port: 7 suspend/resume cycles,
  sleeping from 30 seconds to 30 minutes.  The disk was still there
  after each of them, and every time I checked, it could be read and
  both upstream port links were at 8 GT/s x4.
- With nothing connected: 3 cycles, both domains came back.  Plugging
  the disk in afterwards worked normally.
- One suspend with pm_test set to "platform".  It goes through
  suspend_late and the noirq phase and returns without entering S3, so
  it is the closest I could get to a suspend aborted after SXFP(0).
  Both domains stayed up and the disk kept working.  SXFP(0) powers
  down the xHCI on the side with nothing connected, and it came back
  as well.

About bit 26: here the firmware sets it only with a fix for ACPICA
PCI_Config regions under a bridge that has no bus number yet (the
problem that open-acpica PRs #1235 and #1236 address).  Without that
fix the bit read as clear in my tests, and the patch only calls
SXFP(0), as the PCI version did.  With the fix, calling SXFP(0) with
the bit set left the controller with the disk dead after resume in all
3 cycles I tried.  Clearing the bit just before SXFP(0) gave 15 good
cycles out of 15 in earlier tests (by hand, from a test module and
from a modified PCI quirk; 12 of them with the disk attached), plus the
cycles above.

If the bit cannot be cleared, SXFP(0) is skipped.  With a device
connected, both choices lose the ports until the machine is powered
off, so I chose the one that does nothing to the controller.  I have
not tested that error path.

As with the PCI version, a device plugged in while the system is
suspended does not wake it.  I measured that with the PCI version and
the bit cleared; this patch makes the same call.

Not tested:
- Real Thunderbolt devices, only the USB 3 disk behind the
  controller's xHCI.
- Other Alpine Ridge variants.  Only the router ID of this machine is
  matched.
- An NHI that is runtime suspended when the system suspends.  Here
  both NHIs were always active at that point.  If one is runtime
  suspended, the PM core may skip its suspend_late (direct_complete),
  and then SXFP(0) is not called.

 drivers/thunderbolt/acpi.c    | 83 +++++++++++++++++++++++++++++++++++++++++++
 drivers/thunderbolt/domain.c  |  5 +++
 drivers/thunderbolt/nhi.c     |  8 +++++
 drivers/thunderbolt/tb.c      |  7 ++++
 drivers/thunderbolt/tb.h      |  5 +++
 drivers/thunderbolt/tb_regs.h |  8 +++++
 6 files changed, 116 insertions(+)

diff --git a/drivers/thunderbolt/acpi.c b/drivers/thunderbolt/acpi.c
index 53546bc477a5..6a61120b75f0 100644
--- a/drivers/thunderbolt/acpi.c
+++ b/drivers/thunderbolt/acpi.c
@@ -7,7 +7,9 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/platform_data/x86/apple.h>
 #include <linux/pm_runtime.h>
+#include <linux/suspend.h>
 
 #include "tb.h"
 
@@ -279,6 +281,87 @@ int tb_acpi_power_off_retimers(struct tb_port *port)
 	return tb_acpi_retimer_set_power(port, false);
 }
 
+static int tb_acpi_apple_clear_bit26(struct tb_switch *sw)
+{
+	unsigned int offset = sw->cap_plug_events + TB_PLUG_EVENTS_CS_20;
+	u32 val;
+	int ret;
+
+	guard(mutex)(&sw->tb->lock);
+
+	ret = tb_sw_read(sw, &val, TB_CFG_SWITCH, offset, 1);
+	if (ret)
+		return ret;
+	if (!(val & TB_PLUG_EVENTS_CS_20_BIT26))
+		return 0;
+
+	tb_sw_dbg(sw, "ACPI: clearing bit 26 of plug events CS_20 (%#x)\n", val);
+	val &= ~TB_PLUG_EVENTS_CS_20_BIT26;
+	ret = tb_sw_write(sw, &val, TB_CFG_SWITCH, offset, 1);
+	if (ret)
+		return ret;
+
+	ret = tb_sw_read(sw, &val, TB_CFG_SWITCH, offset, 1);
+	if (ret)
+		return ret;
+
+	return val & TB_PLUG_EVENTS_CS_20_BIT26 ? -EIO : 0;
+}
+
+/**
+ * tb_acpi_apple_sxfp() - Call SXFP(0) on Apple systems with Alpine Ridge
+ * @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) under the NHI is called before
+ * entering it.
+ *
+ * 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 only call SXFP(0) if
+ * it reads back as clear.
+ *
+ * Call only when entering system sleep. Does nothing on other systems.
+ */
+void tb_acpi_apple_sxfp(struct tb *tb)
+{
+	struct tb_switch *sw = tb->root_switch;
+	acpi_handle handle, sxfp;
+	acpi_status status;
+
+	if (!x86_apple_machine)
+		return;
+	/* Firmware powers the controller back on only if it was involved */
+	if (!pm_suspend_via_firmware())
+		return;
+	if (!sw || tb->nhi->going_away)
+		return;
+	if (sw->config.vendor_id != PCI_VENDOR_ID_INTEL ||
+	    sw->config.device_id != PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE)
+		return;
+
+	handle = ACPI_HANDLE(tb->nhi->dev);
+	if (!handle || ACPI_FAILURE(acpi_get_handle(handle, "SXFP", &sxfp)))
+		return;
+
+	if (!sw->cap_plug_events) {
+		tb_sw_warn(sw, "ACPI: plug events capability not found, not calling SXFP\n");
+		return;
+	}
+
+	if (tb_acpi_apple_clear_bit26(sw)) {
+		tb_sw_warn(sw, "ACPI: failed to clear bit 26 of plug events CS_20, not calling SXFP\n");
+		return;
+	}
+
+	tb_sw_dbg(sw, "ACPI: calling SXFP\n");
+	status = acpi_execute_simple_method(sxfp, NULL, 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..06a23dd0fc70 100644
--- a/drivers/thunderbolt/domain.c
+++ b/drivers/thunderbolt/domain.c
@@ -569,6 +569,11 @@ int tb_domain_suspend(struct tb *tb)
 	return tb->cm_ops->suspend ? tb->cm_ops->suspend(tb) : 0;
 }
 
+int tb_domain_suspend_late(struct tb *tb)
+{
+	return tb->cm_ops->suspend_late ? tb->cm_ops->suspend_late(tb) : 0;
+}
+
 int tb_domain_freeze_noirq(struct tb *tb)
 {
 	int ret = 0;
diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index 6d2be7734a00..f35852ab41e8 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -1163,6 +1163,13 @@ static int nhi_suspend(struct device *dev)
 	return tb_domain_suspend(tb);
 }
 
+static int nhi_suspend_late(struct device *dev)
+{
+	struct tb *tb = dev_get_drvdata(dev);
+
+	return tb_domain_suspend_late(tb);
+}
+
 static void nhi_complete(struct device *dev)
 {
 	struct tb *tb = dev_get_drvdata(dev);
@@ -1422,6 +1429,7 @@ const struct dev_pm_ops nhi_pm_ops = {
 	.thaw_noirq = nhi_thaw_noirq,
 	.restore_noirq = nhi_resume_noirq,
 	.suspend = nhi_suspend,
+	.suspend_late = nhi_suspend_late,
 	.poweroff_noirq = nhi_poweroff_noirq,
 	.poweroff = nhi_suspend,
 	.complete = nhi_complete,
diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index 7e39b067e5ff..7b73b8c9accb 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -3108,6 +3108,12 @@ static int tb_suspend_noirq(struct tb *tb)
 	return 0;
 }
 
+static int tb_suspend_late(struct tb *tb)
+{
+	tb_acpi_apple_sxfp(tb);
+	return 0;
+}
+
 static void tb_restore_children(struct tb_switch *sw)
 {
 	struct tb_port *port;
@@ -3310,6 +3316,7 @@ static const struct tb_cm_ops tb_cm_ops = {
 	.deinit = tb_deinit,
 	.suspend_noirq = tb_suspend_noirq,
 	.resume_noirq = tb_resume_noirq,
+	.suspend_late = tb_suspend_late,
 	.freeze_noirq = tb_freeze_noirq,
 	.thaw_noirq = tb_thaw_noirq,
 	.complete = tb_complete,
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index e86845762e22..4820a363f43f 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -481,6 +481,7 @@ struct tb_path {
  * @suspend_noirq: Connection manager specific suspend_noirq
  * @resume_noirq: Connection manager specific resume_noirq
  * @suspend: Connection manager specific suspend
+ * @suspend_late: Connection manager specific suspend_late
  * @freeze_noirq: Connection manager specific freeze_noirq
  * @thaw_noirq: Connection manager specific thaw_noirq
  * @complete: Connection manager specific complete
@@ -515,6 +516,7 @@ struct tb_cm_ops {
 	int (*suspend_noirq)(struct tb *tb);
 	int (*resume_noirq)(struct tb *tb);
 	int (*suspend)(struct tb *tb);
+	int (*suspend_late)(struct tb *tb);
 	int (*freeze_noirq)(struct tb *tb);
 	int (*thaw_noirq)(struct tb *tb);
 	void (*complete)(struct tb *tb);
@@ -779,6 +781,7 @@ void tb_domain_remove(struct tb *tb);
 int tb_domain_suspend_noirq(struct tb *tb);
 int tb_domain_resume_noirq(struct tb *tb);
 int tb_domain_suspend(struct tb *tb);
+int tb_domain_suspend_late(struct tb *tb);
 int tb_domain_freeze_noirq(struct tb *tb);
 int tb_domain_thaw_noirq(struct tb *tb);
 void tb_domain_complete(struct tb *tb);
@@ -1524,6 +1527,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_apple_sxfp(struct tb *tb);
 #else
 static inline bool tb_acpi_add_links(struct tb_nhi *nhi) { return false; }
 
@@ -1537,6 +1541,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_apple_sxfp(struct tb *tb) { }
 #endif
 
 #ifdef CONFIG_DEBUG_FS
diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
index 92d893634d2b..e28a9bfbfefa 100644
--- a/drivers/thunderbolt/tb_regs.h
+++ b/drivers/thunderbolt/tb_regs.h
@@ -577,6 +577,14 @@ struct tb_regs_hop {
 #define TB_PLUG_EVENTS_PCIE_CMD_TIMEOUT_MASK	BIT(31)
 #define TB_PLUG_EVENTS_PCIE_CMD_RD_DATA		0x1d
 
+/*
+ * Alpine Ridge on Apple systems (VSC_CS_20). The firmware can set bit
+ * 26 when it resumes the NHI. While it is set SXFP(0) turns the router
+ * off and it does not come back on resume.
+ */
+#define TB_PLUG_EVENTS_CS_20			0x14
+#define TB_PLUG_EVENTS_CS_20_BIT26		BIT(26)
+
 /* CP Low Power registers */
 #define TB_LOW_PWR_C1_CL1			0x1
 #define TB_LOW_PWR_C1_CL1_OBJ_MASK		GENMASK(4, 1)

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] thunderbolt: acpi: tb_acpi_apple_sxfp() in suspend_late breaks noirq suspend
--

commit bf9ec70ca8c7c6c95ce1947e1918dbc5496d5f29
Author: Francisco Beltrán Millalén <fbeltranmillalen@gmail.com>

thunderbolt: Call SXFP() on Apple systems with Alpine Ridge when suspending

This commit adds a suspend_late callback for the software connection
manager that clears bit 26 of plug events CS_20 and evaluates the SXFP(0)
ACPI method under the NHI on Apple systems with Alpine Ridge controllers
when suspending to RAM.

> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index 4e5d0578fd4bc..f12db9d8bf543 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -3120,6 +3120,12 @@ static int tb_suspend_noirq(struct tb *tb)
>  	return 0;
>  }
>  
> +static int tb_suspend_late(struct tb *tb)
> +{
> +	tb_acpi_apple_sxfp(tb);
> +	return 0;
> +}

[Severity: High]
Does calling tb_acpi_apple_sxfp() in the suspend_late phase cause
regressions during the subsequent suspend_noirq phase on mainline kernels?

During system suspend, dpm_suspend_late() runs for all devices before
dpm_suspend_noirq() begins. In dpm_suspend(), pci_pm_suspend() resumes the
Alpine Ridge xHCI from runtime suspend and sets pci_dev->state_saved = false,
while hcd_pci_suspend() -> suspend_common() only disables the PCI device and
defers pci_save_state() to hcd_pci_suspend_noirq().

When nhi_suspend_late() -> tb_domain_suspend_late() -> tb_suspend_late() ->
tb_acpi_apple_sxfp() evaluates SXFP(0) during dpm_suspend_late():

drivers/thunderbolt/acpi.c:tb_acpi_apple_sxfp()
    status = acpi_execute_simple_method(sxfp, NULL, 0);

SXFP(0) powers down the controller's xHCI when nothing is connected to that
side. Later, when dpm_suspend_noirq() runs, hcd_pci_suspend_noirq() saves the
PCI configuration space of the now powered-off xHCI:

drivers/usb/core/hcd-pci.c:hcd_pci_suspend_noirq()
    pci_save_state(pci_dev);

In mainline, pci_save_state() unconditionally reads standard PCI config
space and PCIe capability registers:

drivers/pci/pci.c:pci_save_state()
    for (i = 0; i < 16; i++) {
        pci_read_config_dword(dev, i * 4, &dev->saved_config_space[i]);
        ...
    }
    dev->state_saved = true;

Because the xHCI is already powered down, pci_save_state() records
0xffffffff into saved_config_space[] and the PCIe capability save buffers,
and marks state_saved = true. On resume, pci_pm_resume_noirq() ->
pci_pm_default_resume_early() -> pci_restore_state() writes those saved
0xffffffff values back into the xHCI's BARs, command register, and PCIe
capability registers.

Additionally, within drivers/thunderbolt/tb.c, running tb_suspend_late()
before tb_suspend_noirq() means SXFP(0) executes while tcm->hotplug_active
is still true and tb->ctl is still running, before tb_suspend_noirq() has
disconnected DisplayPort resources, exited redrive mode, and called
tb_switch_suspend(tb->root_switch, false):

drivers/thunderbolt/tb.c:tb_suspend_noirq()
    tb_disconnect_and_release_dp(tb);
    tb_switch_exit_redrive(tb->root_switch);
    tb_switch_suspend(tb->root_switch, false);
    tcm->hotplug_active = false; /* signal tb_handle_hotplug to quit */

Could evaluating SXFP(0) before tb_suspend_noirq() and hcd_pci_suspend_noirq()
corrupt the xHCI PCI state on resume or interfere with tb_switch_suspend()?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019134851.7496-1-fbeltranmillalen@gmail.com?part=1

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

[Mika Westerberg]

Hi,

On Mon, Oct 19, 2026 at 10:48:51AM -0300, Francisco Beltrán Millalén wrote:
> 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, so do it from suspend_late of the software connection
> manager.
> 
> 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.
> 
> Assisted-by: LLM

This is not needed anymore.

> Signed-off-by: Francisco Beltrán Millalén <fbeltranmillalen@gmail.com>
> ---
> This replaces "PCI: Extend Apple Thunderbolt power quirk to Alpine
> Ridge":
> https://lore.kernel.org/all/20260924132815.24430-1-fbeltranmillalen@gmail.com/
> 
> As discussed there, both parts now live in drivers/thunderbolt, and
> nothing changes in drivers/pci.  I asked Bjorn to drop the PCI version,
> so the two should not be applied together.
> 
> Without SXFP(0) the ports were lost in 15 of 15 cycles with a device
> attached (details in the old thread).  It runs from suspend_late
> because from suspend_noirq it did not help: by then the bridges below
> the controller were already inaccessible.
> 
> I tested it on a MacBookPro14,3, which has two Alpine Ridge controllers
> (two domains), with the software connection manager.  The kernel was
> 6.18.49 with a backport of this patch.  The backport uses
> &nhi->pdev->dev in place of nhi->dev and adds a module parameter to
> switch it on, and the PCI quirk was switched off.  That kernel also
> carried my PCI save/restore series and a local change equivalent to
> the ACPICA fix mentioned below.  I have not run this patch without
> them.
> 
> - With a USB 3 disk on a Type-C port: 7 suspend/resume cycles,
>   sleeping from 30 seconds to 30 minutes.  The disk was still there
>   after each of them, and every time I checked, it could be read and
>   both upstream port links were at 8 GT/s x4.
> - With nothing connected: 3 cycles, both domains came back.  Plugging
>   the disk in afterwards worked normally.
> - One suspend with pm_test set to "platform".  It goes through
>   suspend_late and the noirq phase and returns without entering S3, so
>   it is the closest I could get to a suspend aborted after SXFP(0).
>   Both domains stayed up and the disk kept working.  SXFP(0) powers
>   down the xHCI on the side with nothing connected, and it came back
>   as well.
> 
> About bit 26: here the firmware sets it only with a fix for ACPICA
> PCI_Config regions under a bridge that has no bus number yet (the
> problem that open-acpica PRs #1235 and #1236 address).  Without that
> fix the bit read as clear in my tests, and the patch only calls
> SXFP(0), as the PCI version did.  With the fix, calling SXFP(0) with
> the bit set left the controller with the disk dead after resume in all
> 3 cycles I tried.  Clearing the bit just before SXFP(0) gave 15 good
> cycles out of 15 in earlier tests (by hand, from a test module and
> from a modified PCI quirk; 12 of them with the disk attached), plus the
> cycles above.
> 
> If the bit cannot be cleared, SXFP(0) is skipped.  With a device
> connected, both choices lose the ports until the machine is powered
> off, so I chose the one that does nothing to the controller.  I have
> not tested that error path.
> 
> As with the PCI version, a device plugged in while the system is
> suspended does not wake it.  I measured that with the PCI version and
> the bit cleared; this patch makes the same call.
> 
> Not tested:
> - Real Thunderbolt devices, only the USB 3 disk behind the
>   controller's xHCI.
> - Other Alpine Ridge variants.  Only the router ID of this machine is
>   matched.

I would think the same applies to all of them.

> - An NHI that is runtime suspended when the system suspends.  Here
>   both NHIs were always active at that point.  If one is runtime
>   suspended, the PM core may skip its suspend_late (direct_complete),
>   and then SXFP(0) is not called.
> 
>  drivers/thunderbolt/acpi.c    | 83 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/thunderbolt/domain.c  |  5 +++
>  drivers/thunderbolt/nhi.c     |  8 +++++
>  drivers/thunderbolt/tb.c      |  7 ++++
>  drivers/thunderbolt/tb.h      |  5 +++
>  drivers/thunderbolt/tb_regs.h |  8 +++++
>  6 files changed, 116 insertions(+)
> 
> diff --git a/drivers/thunderbolt/acpi.c b/drivers/thunderbolt/acpi.c
> index 53546bc477a5..6a61120b75f0 100644
> --- a/drivers/thunderbolt/acpi.c
> +++ b/drivers/thunderbolt/acpi.c
> @@ -7,7 +7,9 @@
>   */
>  
>  #include <linux/acpi.h>
> +#include <linux/platform_data/x86/apple.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/suspend.h>
>  
>  #include "tb.h"
>  
> @@ -279,6 +281,87 @@ int tb_acpi_power_off_retimers(struct tb_port *port)
>  	return tb_acpi_retimer_set_power(port, false);
>  }
>  
> +static int tb_acpi_apple_clear_bit26(struct tb_switch *sw)
> +{
> +	unsigned int offset = sw->cap_plug_events + TB_PLUG_EVENTS_CS_20;
> +	u32 val;
> +	int ret;
> +
> +	guard(mutex)(&sw->tb->lock);
> +
> +	ret = tb_sw_read(sw, &val, TB_CFG_SWITCH, offset, 1);
> +	if (ret)
> +		return ret;
> +	if (!(val & TB_PLUG_EVENTS_CS_20_BIT26))
> +		return 0;
> +
> +	tb_sw_dbg(sw, "ACPI: clearing bit 26 of plug events CS_20 (%#x)\n", val);
> +	val &= ~TB_PLUG_EVENTS_CS_20_BIT26;

Empty line here

> +	ret = tb_sw_write(sw, &val, TB_CFG_SWITCH, offset, 1);
> +	if (ret)
> +		return ret;
> +
> +	ret = tb_sw_read(sw, &val, TB_CFG_SWITCH, offset, 1);
> +	if (ret)
> +		return ret;

Do you really need to read it back?

Also I think it would be bit more "generic" to make this take the value as
parameter:

tb_switch_set_bit26(struct tb_switch *sw, bool value)

> +
> +	return val & TB_PLUG_EVENTS_CS_20_BIT26 ? -EIO : 0;
> +}
> +
> +/**
> + * tb_acpi_apple_sxfp() - Call SXFP(0) on Apple systems with Alpine Ridge
> + * @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) under the NHI is called before
> + * entering it.

I would guess SXFP is "Sx Force Power" or something along those lines.

> + *
> + * 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 only call SXFP(0) if
> + * it reads back as clear.
> + *
> + * Call only when entering system sleep. Does nothing on other systems.
> + */
> +void tb_acpi_apple_sxfp(struct tb *tb)

Can we call this something not so cryptic?

Maybe tb_acpi_apple_force_power().

> +{
> +	struct tb_switch *sw = tb->root_switch;
> +	acpi_handle handle, sxfp;
> +	acpi_status status;
> +
> +	if (!x86_apple_machine)
> +		return;
> +	/* Firmware powers the controller back on only if it was involved */
> +	if (!pm_suspend_via_firmware())
> +		return;
> +	if (!sw || tb->nhi->going_away)
> +		return;

Are these really needed?

> +	if (sw->config.vendor_id != PCI_VENDOR_ID_INTEL ||
> +	    sw->config.device_id != PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_BRIDGE)
> +		return;

This can be tb_switch_is_alpine_ridge(), no?

> +
> +	handle = ACPI_HANDLE(tb->nhi->dev);

struct acpi_device adev;

adev = ACPI_COMPANION(tb->nhi->dev);
if (!adev)
	return;

> +	if (!handle || ACPI_FAILURE(acpi_get_handle(handle, "SXFP", &sxfp)))
> +		return;

Drop the handle thing and call it like:

ret = acpi_evaluate_object(adev->handle, "SXFP", ...)

btw, have you checked that this SXFP is not called from _PSx methods? Or
the corresponding power resource? It's typically called from there because
they are standard methods.

> +
> +	if (!sw->cap_plug_events) {
> +		tb_sw_warn(sw, "ACPI: plug events capability not found, not calling SXFP\n");

No need for this.

> +		return;
> +	}
> +
> +	if (tb_acpi_apple_clear_bit26(sw)) {
> +		tb_sw_warn(sw, "ACPI: failed to clear bit 26 of plug events CS_20, not calling SXFP\n");

Not sure how useful this is either.

> +		return;
> +	}
> +
> +	tb_sw_dbg(sw, "ACPI: calling SXFP\n");
> +	status = acpi_execute_simple_method(sxfp, NULL, 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..06a23dd0fc70 100644
> --- a/drivers/thunderbolt/domain.c
> +++ b/drivers/thunderbolt/domain.c
> @@ -569,6 +569,11 @@ int tb_domain_suspend(struct tb *tb)
>  	return tb->cm_ops->suspend ? tb->cm_ops->suspend(tb) : 0;
>  }
>  
> +int tb_domain_suspend_late(struct tb *tb)

Why not suspend_noirq? That's already there.

> +{
> +	return tb->cm_ops->suspend_late ? tb->cm_ops->suspend_late(tb) : 0;
> +}
> +
>  int tb_domain_freeze_noirq(struct tb *tb)
>  {
>  	int ret = 0;
> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index 6d2be7734a00..f35852ab41e8 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -1163,6 +1163,13 @@ static int nhi_suspend(struct device *dev)
>  	return tb_domain_suspend(tb);
>  }
>  
> +static int nhi_suspend_late(struct device *dev)
> +{
> +	struct tb *tb = dev_get_drvdata(dev);
> +
> +	return tb_domain_suspend_late(tb);
> +}
> +
>  static void nhi_complete(struct device *dev)
>  {
>  	struct tb *tb = dev_get_drvdata(dev);
> @@ -1422,6 +1429,7 @@ const struct dev_pm_ops nhi_pm_ops = {
>  	.thaw_noirq = nhi_thaw_noirq,
>  	.restore_noirq = nhi_resume_noirq,
>  	.suspend = nhi_suspend,
> +	.suspend_late = nhi_suspend_late,
>  	.poweroff_noirq = nhi_poweroff_noirq,
>  	.poweroff = nhi_suspend,
>  	.complete = nhi_complete,
> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index 7e39b067e5ff..7b73b8c9accb 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -3108,6 +3108,12 @@ static int tb_suspend_noirq(struct tb *tb)
>  	return 0;
>  }
>  
> +static int tb_suspend_late(struct tb *tb)
> +{
> +	tb_acpi_apple_sxfp(tb);
> +	return 0;
> +}
> +
>  static void tb_restore_children(struct tb_switch *sw)
>  {
>  	struct tb_port *port;
> @@ -3310,6 +3316,7 @@ static const struct tb_cm_ops tb_cm_ops = {
>  	.deinit = tb_deinit,
>  	.suspend_noirq = tb_suspend_noirq,
>  	.resume_noirq = tb_resume_noirq,
> +	.suspend_late = tb_suspend_late,
>  	.freeze_noirq = tb_freeze_noirq,
>  	.thaw_noirq = tb_thaw_noirq,
>  	.complete = tb_complete,
> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> index e86845762e22..4820a363f43f 100644
> --- a/drivers/thunderbolt/tb.h
> +++ b/drivers/thunderbolt/tb.h
> @@ -481,6 +481,7 @@ struct tb_path {
>   * @suspend_noirq: Connection manager specific suspend_noirq
>   * @resume_noirq: Connection manager specific resume_noirq
>   * @suspend: Connection manager specific suspend
> + * @suspend_late: Connection manager specific suspend_late
>   * @freeze_noirq: Connection manager specific freeze_noirq
>   * @thaw_noirq: Connection manager specific thaw_noirq
>   * @complete: Connection manager specific complete
> @@ -515,6 +516,7 @@ struct tb_cm_ops {
>  	int (*suspend_noirq)(struct tb *tb);
>  	int (*resume_noirq)(struct tb *tb);
>  	int (*suspend)(struct tb *tb);
> +	int (*suspend_late)(struct tb *tb);
>  	int (*freeze_noirq)(struct tb *tb);
>  	int (*thaw_noirq)(struct tb *tb);
>  	void (*complete)(struct tb *tb);
> @@ -779,6 +781,7 @@ void tb_domain_remove(struct tb *tb);
>  int tb_domain_suspend_noirq(struct tb *tb);
>  int tb_domain_resume_noirq(struct tb *tb);
>  int tb_domain_suspend(struct tb *tb);
> +int tb_domain_suspend_late(struct tb *tb);
>  int tb_domain_freeze_noirq(struct tb *tb);
>  int tb_domain_thaw_noirq(struct tb *tb);
>  void tb_domain_complete(struct tb *tb);
> @@ -1524,6 +1527,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_apple_sxfp(struct tb *tb);
>  #else
>  static inline bool tb_acpi_add_links(struct tb_nhi *nhi) { return false; }
>  
> @@ -1537,6 +1541,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_apple_sxfp(struct tb *tb) { }
>  #endif
>  
>  #ifdef CONFIG_DEBUG_FS
> diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
> index 92d893634d2b..e28a9bfbfefa 100644
> --- a/drivers/thunderbolt/tb_regs.h
> +++ b/drivers/thunderbolt/tb_regs.h
> @@ -577,6 +577,14 @@ struct tb_regs_hop {
>  #define TB_PLUG_EVENTS_PCIE_CMD_TIMEOUT_MASK	BIT(31)
>  #define TB_PLUG_EVENTS_PCIE_CMD_RD_DATA		0x1d
>  
> +/*
> + * Alpine Ridge on Apple systems (VSC_CS_20). The firmware can set bit
> + * 26 when it resumes the NHI. While it is set SXFP(0) turns the router
> + * off and it does not come back on resume.
> + */
> +#define TB_PLUG_EVENTS_CS_20			0x14
> +#define TB_PLUG_EVENTS_CS_20_BIT26		BIT(26)

Please move these before the TB_PLUG_EVENTS_PCIE_*

> +
>  /* CP Low Power registers */
>  #define TB_LOW_PWR_C1_CL1			0x1
>  #define TB_LOW_PWR_C1_CL1_OBJ_MASK		GENMASK(4, 1)