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

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

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)