Re: [PATCH -next v2] thunderbolt: Add quirk to reset host interface on DMA path teardown for AMD USB4 routers

Hi,

On Wed, Aug 05, 2026 at 04:48:11PM +0530, Basavaraj Natikar wrote:
> Some AMD USB4 host routers have a bug in the Host Interface where
> DMA path setup and teardown cycles may cause the Tx ring to hang.
> 
> Fix this by issuing a Host Interface Reset on every DMA path teardown
> for affected routers. The Host Interface Reset brings the registers in
> the memory BAR to their default state and clears the End-to-End Flow
> Control state, preventing the hang condition.
> 
> Co-developed-by: Sanath S <[email protected]>
> Signed-off-by: Sanath S <[email protected]>
> Signed-off-by: Basavaraj Natikar <[email protected]>
> ---
> v2:
> - Move the quirk to the NHI (nhi->quirks), set in nhi_pci_check_quirks()
>   by PCI ID following the QUIRK_AUTO_CLEAR_INT convention, instead of the
>   tb_quirks[] fabric table.
> - Declare nhi_host_interface_reset() in nhi.h instead of tb.h.
> 
> v1: https://lore.kernel.org/all/[email protected]/
>  drivers/thunderbolt/nhi.c      | 37 ++++++++++++++++++++++++++++++++++
>  drivers/thunderbolt/nhi.h      |  2 ++
>  drivers/thunderbolt/nhi_regs.h |  4 ++++
>  drivers/thunderbolt/pci.c      | 21 +++++++++++++++++++
>  drivers/thunderbolt/tb.c       |  6 ++++++
>  5 files changed, 70 insertions(+)
> 
> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index 383a36212f70..72a76db5ea68 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -1160,6 +1160,43 @@ static void nhi_reset(struct tb_nhi *nhi)
>  	dev_warn(nhi->dev, "timeout resetting host router\n");
>  }
>  
> +/**
> + * nhi_host_interface_reset() - Issue host interface reset

nhi_reset_interface()

> + * @tb: Domain whose host interface is reset

This should take nhi as paramter.

> + *
> + * Resets the host interface by setting the RST bit in the host interface
> + * reset register. This brings the registers in the memory BAR to their
> + * default state and clears the End-to-End Flow Control state. Does nothing
> + * unless the host interface is known to need this (QUIRK_HOST_INTERFACE_RESET).
> + *
> + * The control channel is stopped over the reset because the reset clears
> + * the ring state as well.
> + */

This can just reset the NHI, e.g not look into the quirks.

> +void nhi_host_interface_reset(struct tb *tb)
> +{
> +	struct tb_nhi *nhi = tb->nhi;
> +	u32 val;
> +
> +	if (!(nhi->quirks & QUIRK_HOST_INTERFACE_RESET))
> +		return;
> +
> +	val = ioread32(nhi->iobase + REG_CAPS);
> +	/* Only v1 host interfaces implement the reset */
> +	if (FIELD_GET(REG_CAPS_VERSION_MASK, val) >= REG_CAPS_VERSION_2)
> +		return;
> +
> +	dev_dbg(nhi->dev, "issuing host interface reset\n");
> +
> +	tb_ctl_stop(tb->ctl);

But don't do these here. Just the actual reset. So that we can call it
whenever we want to do host internface reset.

> +
> +	iowrite32(REG_HOST_INTERFACE_RESET_RST,
> +		  nhi->iobase + REG_HOST_INTERFACE_RESET);
> +	/* Wait for tHIReset (10 ms) to complete */
> +	usleep_range(10000, 20000);
> +
> +	tb_ctl_start(tb->ctl);
> +}
> +
>  static struct tb *nhi_select_cm(struct tb_nhi *nhi)
>  {
>  	struct tb *tb;
> diff --git a/drivers/thunderbolt/nhi.h b/drivers/thunderbolt/nhi.h
> index d488eadadfce..9be0372b837e 100644
> --- a/drivers/thunderbolt/nhi.h
> +++ b/drivers/thunderbolt/nhi.h
> @@ -36,6 +36,7 @@ irqreturn_t nhi_msi(int irq, void *data);
>  irqreturn_t ring_msix(int irq, void *data);
>  int nhi_probe(struct tb_nhi *nhi);
>  void nhi_shutdown(struct tb_nhi *nhi);
> +void nhi_host_interface_reset(struct tb *tb);

>  extern const struct dev_pm_ops nhi_pm_ops;
>  
>  /**
> @@ -121,6 +122,7 @@ struct tb_nhi_ops {
>  /* Host interface quirks */
>  #define QUIRK_AUTO_CLEAR_INT	BIT(0)
>  #define QUIRK_E2E		BIT(1)
> +#define QUIRK_HOST_INTERFACE_RESET	BIT(2)

Let's call it QUIRK_RESET_DMA_ON_TEARDOWN or something like that.

>  /*
>   * Minimal number of vectors when we use MSI-X. Two for control channel
> diff --git a/drivers/thunderbolt/nhi_regs.h b/drivers/thunderbolt/nhi_regs.h
> index d6a197fabc74..99df60b6db36 100644
> --- a/drivers/thunderbolt/nhi_regs.h
> +++ b/drivers/thunderbolt/nhi_regs.h
> @@ -115,6 +115,10 @@ struct ring_desc {
>  #define REG_CAPS_VERSION_MASK		GENMASK(23, 16)
>  #define REG_CAPS_VERSION_2		0x40
>  
> +/* Host Interface Reset - resets TX/RX rings and E2E flow control counters */
> +#define REG_HOST_INTERFACE_RESET	0x39858
> +#define REG_HOST_INTERFACE_RESET_RST	BIT(0)
> +
>  #define REG_DMA_MISC			0x39864
>  #define REG_DMA_MISC_INT_AUTO_CLEAR     BIT(2)
>  #define REG_DMA_MISC_DISABLE_AUTO_CLEAR	BIT(17)
> diff --git a/drivers/thunderbolt/pci.c b/drivers/thunderbolt/pci.c
> index 8462ccb59b7e..02534287cb43 100644
> --- a/drivers/thunderbolt/pci.c
> +++ b/drivers/thunderbolt/pci.c
> @@ -62,6 +62,27 @@ static void nhi_pci_check_quirks(struct tb_nhi_pci *nhi_pci)
>  			nhi->quirks |= QUIRK_E2E;
>  			break;
>  		}
> +	} else if (pdev->vendor == PCI_VENDOR_ID_AMD) {
> +		switch (pdev->device) {
> +		case 0x1120:
> +		case 0x1121:
> +		case 0x113b:
> +		case 0x113c:
> +		case 0x1155:
> +		case 0x1158:
> +		case 0x1159:
> +		case 0x151c:
> +		case 0x151d:
> +		case 0x158d:
> +		case 0x158e:

I would prefer if these are defined in nhi.h similarly what we do with the
Intel stuff so we have symbolic names for them.

> +			/*
> +			 * These AMD hosts may hang the Tx ring when the
> +			 * DMA paths are torn down so they need the host
> +			 * interface reset after each teardown.
> +			 */
> +			nhi->quirks |= QUIRK_HOST_INTERFACE_RESET;
> +			break;
> +		}
>  	}
>  }
>  
> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index 47753a5c0f2e..f07f2ada2346 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -2395,6 +2395,12 @@ static void __tb_disconnect_xdomain_paths(struct tb *tb, struct tb_xdomain *xd,
>  	 * the same host router USB4 downstream port.
>  	 */
>  	tb_enable_clx(sw);
> +
> +	/*
> +	 * Some hosts may hang the Tx ring after the DMA paths are torn
> +	 * down so reset the host interface to prevent that.
> +	 */
> +	nhi_host_interface_reset(tb);

Instead of here, do this in domain.c::tb_domain_disconnect_xdomain_paths()
and there you can also stop/start the control channel around this. You may
need to take the tb->lock here too so maybe add a helper.

I wanted to make tb.c (and domain.c) not call anything from the "upper"
layer (nhi.c) but I can't think of reasonable way to do it here, and there
is no way to handle this from nhi.c because it has no knowledge of XDomain
tunnels. Perhaps through indirection, nhi->ops->reset_interface?

>  }
>  
>  static int tb_disconnect_xdomain_paths(struct tb *tb, struct tb_xdomain *xd,
> -- 
> 2.34.1