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

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

Hi,

On Wed, Oct 21, 2026 at 11:14:44AM -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.
> 
> 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);

We should not call NHI things in TB side of code (that's domain and the
routers, adapters etc). So this should be moved elsewhere.

>  	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,

This hook is now so please check that the patch applies to the latest next
branch.

> +};
> +
>  /* 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)