Re: [PATCH v2] 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 05:29:00PM -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_noirq 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.
> 
> Signed-off-by: Francisco Beltrán Millalén <fbeltranmillalen@gmail.com>
> ---
> Thanks for the review, Mika.

Looking good, few more comments see below.

> I am sending this alongside "PCI/PM: Keep the saved config space of an
> inaccessible device":
> https://lore.kernel.org/all/20261019202445.38259-1-fbeltranmillalen@gmail.com/
> 
> SXFP(0) also powers down the xHCI on the side of the controller with
> nothing connected.  The automated review of v1 pointed out that if this
> happens before the xHCI saves its state in its own suspend_noirq,
> pci_save_state() stores all ones today, and resume writes them back.
> I found the same by reading the code.  The xHCI and the NHI suspend
> asynchronously, so their order is not fixed.  In my tests below the
> xHCI always saved first, except when I forced SXFP(0) earlier; then the
> PCI patch kept the previous snapshot and the xHCI came back.
> 
> v2:
> - Call SXFP(0) from tb_suspend_noirq() instead of adding a suspend_late
>   callback.  In v1 I wrote that suspend_noirq was too late.  That was
>   measured from the suspend_noirq of the upstream port of the
>   controller, which runs after the NHI and the other devices below it.
>   From the suspend_noirq of the NHI it works (tests below).
> - Renamed to tb_acpi_apple_force_power().
> - Use tb_switch_is_alpine_ridge(), so all Alpine Ridge variants are
>   covered.
> - Use ACPI_COMPANION() and call SXFP through adev->handle, with
>   acpi_execute_simple_method() since it takes a single integer.
> - Added tb_switch_set_bit26(sw, value) in switch.c, without the read
>   back.  It only writes when the bit has to change.  tb->lock is already
>   held by tb_domain_suspend_noirq().
> - Dropped the !sw and going_away checks and the two warnings, moved the
>   register definitions before TB_PLUG_EVENTS_PCIE_*, and dropped the
>   Assisted-by tag.
> - Kept pm_suspend_via_firmware(), as in the Cactus Ridge quirk
>   (quirk_apple_poweroff_thunderbolt()): the OS does not know how to power
>   the controller back on.  Added pm_suspend_in_progress(), because
>   __nhi_suspend_noirq() also runs from nhi_poweroff_noirq() when entering
>   hibernation, where pm_suspend_via_firmware() is true.  Together they
>   limit the call to suspend-to-RAM via firmware.
> 
> v1: https://lore.kernel.org/all/20261019134851.7496-1-fbeltranmillalen@gmail.com/
> 
> About calling SXFP from _PSx or a power resource: I checked the ACPI
> tables of this machine.  SXFP is defined once under each NHI and
> nothing calls it.  The _PS3 of the NHI is empty.
> 
> 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, which
> uses &nhi->pdev->dev in place of nhi->dev.  The kernel also carried the
> PCI patch above, and a local change equivalent to the ACPICA fix
> mentioned in v1, without which the firmware does not set bit 26 here.
> The PCI quirk from the earlier version was switched off, except where
> noted.  All cycles were 30 second suspends to RAM.
> 
> - Three cycles with a USB 3 disk on one side or the other.  Both
>   domains came back each time, with both upstream port links at 8 GT/s
>   x4, and the disk could be read.
> - One cycle removing the disk while suspended, and one with nothing
>   connected.  Both domains came back, and the disk worked when plugged
>   in afterwards (on both sides after the first, on one after the
>   second).
> - Three more cycles with the disk attached and the old PCI quirk
>   switched on as well, so that SXFP(0) was called earlier, in the PCI
>   suspend phase, to check the PCI patch.  This patch then calls it a
>   second time.  All three resumed with both domains and the disk
>   working, and the disk also worked when moved afterwards to the side
>   that had been empty.
> 
> 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).  With v1 I tried it with pm_test set
>   to "platform", but not with this version.
> - 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 above keep the patch out
>   of both.
> 
> As noted in v1, a device plugged in while the system is suspended does
> not wake it.  I measured that with the PCI quirk; this patch makes the
> same call, and I did not repeat it.
> 
>  drivers/thunderbolt/acpi.c    | 46 +++++++++++++++++++++++++++++++++++
>  drivers/thunderbolt/switch.c  | 32 ++++++++++++++++++++++++
>  drivers/thunderbolt/tb.c      |  1 +
>  drivers/thunderbolt/tb.h      |  3 +++
>  drivers/thunderbolt/tb_regs.h |  2 ++
>  5 files changed, 84 insertions(+)
> 
> diff --git a/drivers/thunderbolt/acpi.c b/drivers/thunderbolt/acpi.c
> index 53546bc477a5..7a25f89ba14b 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,50 @@ int tb_acpi_power_off_retimers(struct tb_port *port)
>  	return tb_acpi_retimer_set_power(port, false);
>  }
>  
> +/**
> + * tb_acpi_apple_force_power() - 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) ("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.
> + *
> + * Call only when entering system sleep. Does nothing on other systems.
> + */
> +void tb_acpi_apple_force_power(struct tb *tb)
> +{
> +	struct tb_switch *sw = tb->root_switch;
> +	struct acpi_device *adev;
> +	acpi_status status;
> +
> +	if (!x86_apple_machine || !tb_switch_is_alpine_ridge(sw))

I was thinking about this bit more. We can actually call this function
tb_acpi_force_power() and drop both checks from here. Instead call this
from pci.c where you add nhi_ops for the AR similarly than what we have for
ICL and the related.

So there would be ar_nhi_ops that provides the suspend_noirq hook that then
does:

	if (x86_apple_machine)
		tb_acpi_force_power(dev_get_drvdata(nhi->dev));

> +		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/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.c b/drivers/thunderbolt/tb.c
> index 7e39b067e5ff..dd3da25453fa 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -3099,6 +3099,7 @@ static int tb_suspend_noirq(struct tb *tb)
>  	struct tb_cm *tcm = tb_priv(tb);
>  
>  	tb_dbg(tb, "suspending...\n");
> +	tb_acpi_apple_force_power(tb);
>  	tb_disconnect_and_release_dp(tb);
>  	tb_switch_exit_redrive(tb->root_switch);
>  	tb_switch_suspend(tb->root_switch, false);
> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> index e86845762e22..472e7fbe1a19 100644
> --- a/drivers/thunderbolt/tb.h
> +++ b/drivers/thunderbolt/tb.h
> @@ -1097,6 +1097,7 @@ static inline bool tb_switch_clx_is_enabled(const struct tb_switch *sw,
>  int tb_switch_pcie_l1_enable(struct tb_switch *sw);
>  
>  int tb_switch_xhci_connect(struct tb_switch *sw);
> +int tb_switch_set_bit26(struct tb_switch *sw, bool value);

Please move this after the xhci functions:

>  void tb_switch_xhci_disconnect(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);
> @@ -1524,6 +1525,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_force_power(struct tb *tb);
>  #else
>  static inline bool tb_acpi_add_links(struct tb_nhi *nhi) { return false; }
>  
> @@ -1537,6 +1539,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_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..58c8757882f5 100644
> --- a/drivers/thunderbolt/tb_regs.h
> +++ b/drivers/thunderbolt/tb_regs.h
> @@ -562,6 +562,8 @@ struct tb_regs_hop {
>  #define TB_PLUG_EVENTS_CS_1_DPOUT_DISABLE	BIT(4)
>  #define TB_PLUG_EVENTS_CS_1_LOW_DPIN_DISABLE	BIT(5)
>  #define TB_PLUG_EVENTS_CS_1_HIGH_DPIN_DISABLE	BIT(6)

Empty line here

> +#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