[PATCH v2 1/2] thunderbolt: Clear registers properly when auto clear isn't in use

[PATCH v2 1/2] thunderbolt: Clear registers properly when auto clear isn't in use

[Mario Limonciello]

When `QUIRK_AUTO_CLEAR_INT` isn't set, interrupt masking should be
cleared by writing to Interrupt Mask Clear (IMR) and interrupt
status should be cleared properly at shutdown/init.

This fixes an error where interrupts are left enabled during resume
from hibernation with `CONFIG_USB4=y`.

Fixes: 468c49f44759 ("thunderbolt: Disable interrupt auto clear for rings")
Reported-by: Takashi Iwai <tiwai@suse.de>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=217343
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v1->v2:
 * Whitespace changes
 * Add new static functions for nhi_mask_interrupt() and
   nhi_clear_interrupt()

I tried to base this off thunderbolt.git/next (tag: thunderbolt-for-v6.4-rc1)
but the following 3 commits are missing from that branch but are in 6.3-rc7:

58cdfe6f58b3 thunderbolt: Rename shadowed variables bit to interrupt_bit and auto_clear_bit
468c49f44759 thunderbolt: Disable interrupt auto clear for rings
1716efdb0793 thunderbolt: Use const qualifier for `ring_interrupt_index`

I cherry picked them first as this patch builds on them. It's expected that
this patch should apply on top of 6.4-rc1 properly.
---
 drivers/thunderbolt/nhi.c      | 29 ++++++++++++++++++++++++-----
 drivers/thunderbolt/nhi_regs.h |  2 ++
 2 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index d76e923fbc6a..c0aee5dc5237 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -54,6 +54,21 @@ static int ring_interrupt_index(const struct tb_ring *ring)
 	return bit;
 }
 
+static void nhi_mask_interrupt(struct tb_nhi *nhi, int mask, int ring)
+{
+	if (nhi->quirks & QUIRK_AUTO_CLEAR_INT)
+		return;
+	iowrite32(mask, nhi->iobase + REG_RING_INTERRUPT_MASK_CLEAR_BASE + ring);
+}
+
+static void nhi_clear_interrupt(struct tb_nhi *nhi, int ring)
+{
+	if (nhi->quirks & QUIRK_AUTO_CLEAR_INT)
+		ioread32(nhi->iobase + REG_RING_NOTIFY_BASE + ring);
+	else
+		iowrite32(~0, nhi->iobase + REG_RING_INT_CLEAR + ring);
+}
+
 /*
  * ring_interrupt_active() - activate/deactivate interrupts for a single ring
  *
@@ -61,8 +76,8 @@ static int ring_interrupt_index(const struct tb_ring *ring)
  */
 static void ring_interrupt_active(struct tb_ring *ring, bool active)
 {
-	int reg = REG_RING_INTERRUPT_BASE +
-		  ring_interrupt_index(ring) / 32 * 4;
+	int index = ring_interrupt_index(ring) / 32 * 4;
+	int reg = REG_RING_INTERRUPT_BASE + index;
 	int interrupt_bit = ring_interrupt_index(ring) & 31;
 	int mask = 1 << interrupt_bit;
 	u32 old, new;
@@ -123,7 +138,11 @@ static void ring_interrupt_active(struct tb_ring *ring, bool active)
 					 "interrupt for %s %d is already %s\n",
 					 RING_TYPE(ring), ring->hop,
 					 active ? "enabled" : "disabled");
-	iowrite32(new, ring->nhi->iobase + reg);
+
+	if (active)
+		iowrite32(new, ring->nhi->iobase + reg);
+	else
+		nhi_mask_interrupt(ring->nhi, mask, index);
 }
 
 /*
@@ -136,11 +155,11 @@ static void nhi_disable_interrupts(struct tb_nhi *nhi)
 	int i = 0;
 	/* disable interrupts */
 	for (i = 0; i < RING_INTERRUPT_REG_COUNT(nhi); i++)
-		iowrite32(0, nhi->iobase + REG_RING_INTERRUPT_BASE + 4 * i);
+		nhi_mask_interrupt(nhi, ~0, 4 * i);
 
 	/* clear interrupt status bits */
 	for (i = 0; i < RING_NOTIFY_REG_COUNT(nhi); i++)
-		ioread32(nhi->iobase + REG_RING_NOTIFY_BASE + 4 * i);
+		nhi_clear_interrupt(nhi, 4 * i);
 }
 
 /* ring helper methods */
diff --git a/drivers/thunderbolt/nhi_regs.h b/drivers/thunderbolt/nhi_regs.h
index faef165a919c..6ba295815477 100644
--- a/drivers/thunderbolt/nhi_regs.h
+++ b/drivers/thunderbolt/nhi_regs.h
@@ -93,6 +93,8 @@ struct ring_desc {
 #define REG_RING_INTERRUPT_BASE	0x38200
 #define RING_INTERRUPT_REG_COUNT(nhi) ((31 + 2 * nhi->hop_count) / 32)
 
+#define REG_RING_INTERRUPT_MASK_CLEAR_BASE	0x38208
+
 #define REG_INT_THROTTLING_RATE	0x38c00
 
 /* Interrupt Vector Allocation */
-- 
2.34.1

[PATCH v2 2/2] thunderbolt: Move Intel quirks into quirks.c

[Mario Limonciello]

There are two Intel specific quirks for auto clear and end to end
that are not specified in the quirks file.  Move them to this location
instead.

This does change it so that they're initialized at a different time,
than quirks currently run but no intended functional impacts.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
v1->v2:
 * New patch

I tried to base this off thunderbolt.git/next (tag: thunderbolt-for-v6.4-rc1)
but the following 2 commits are missing from that branch but are in 6.3-rc7:

7af9da8ce8f9 ("thunderbolt: Add quirk to disable CLx")
f0a57dd33b3e ("thunderbolt: Limit USB3 bandwidth of certain Intel USB4 host routers")

I cherry picked them first as this patch builds on them. It's expected that
this patch should apply on top of 6.4-rc1 properly.

I don't have the matching hardware so this patch is only compile tested.
---
 drivers/thunderbolt/nhi.c    | 27 ---------------------------
 drivers/thunderbolt/quirks.c | 27 +++++++++++++++++++++++++++
 drivers/thunderbolt/tb.h     |  4 ++++
 3 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index c0aee5dc5237..1f8545d03030 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -43,8 +43,6 @@
 #define NHI_MAILBOX_TIMEOUT	500 /* ms */
 
 /* Host interface quirks */
-#define QUIRK_AUTO_CLEAR_INT	BIT(0)
-#define QUIRK_E2E		BIT(1)
 
 static int ring_interrupt_index(const struct tb_ring *ring)
 {
@@ -1147,30 +1145,6 @@ static void nhi_shutdown(struct tb_nhi *nhi)
 		nhi->ops->shutdown(nhi);
 }
 
-static void nhi_check_quirks(struct tb_nhi *nhi)
-{
-	if (nhi->pdev->vendor == PCI_VENDOR_ID_INTEL) {
-		/*
-		 * Intel hardware supports auto clear of the interrupt
-		 * status register right after interrupt is being
-		 * issued.
-		 */
-		nhi->quirks |= QUIRK_AUTO_CLEAR_INT;
-
-		switch (nhi->pdev->device) {
-		case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI:
-		case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI:
-			/*
-			 * Falcon Ridge controller needs the end-to-end
-			 * flow control workaround to avoid losing Rx
-			 * packets when RING_FLAG_E2E is set.
-			 */
-			nhi->quirks |= QUIRK_E2E;
-			break;
-		}
-	}
-}
-
 static int nhi_check_iommu_pdev(struct pci_dev *pdev, void *data)
 {
 	if (!pdev->external_facing ||
@@ -1322,7 +1296,6 @@ static int nhi_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (!nhi->tx_rings || !nhi->rx_rings)
 		return -ENOMEM;
 
-	nhi_check_quirks(nhi);
 	nhi_check_iommu(nhi);
 
 	res = nhi_init_msi(nhi);
diff --git a/drivers/thunderbolt/quirks.c b/drivers/thunderbolt/quirks.c
index 1157b8869bcc..da851e0760b7 100644
--- a/drivers/thunderbolt/quirks.c
+++ b/drivers/thunderbolt/quirks.c
@@ -26,6 +26,18 @@ static void quirk_clx_disable(struct tb_switch *sw)
 	tb_sw_dbg(sw, "disabling CL states\n");
 }
 
+static void quirk_auto_clear_interrupts(struct tb_switch *sw)
+{
+	sw->quirks |= QUIRK_AUTO_CLEAR_INT;
+	tb_sw_dbg(sw, "setting auto clear interrupts\n");
+}
+
+static void quirk_e2e(struct tb_switch *sw)
+{
+	sw->quirks |= QUIRK_E2E;
+	tb_sw_dbg(sw, "setting E2E flow control\n");
+}
+
 static void quirk_usb3_maximum_bandwidth(struct tb_switch *sw)
 {
 	struct tb_port *port;
@@ -51,6 +63,21 @@ static const struct tb_quirk tb_quirks[] = {
 	/* Dell WD19TB supports self-authentication on unplug */
 	{ 0x0000, 0x0000, 0x00d4, 0xb070, quirk_force_power_link },
 	{ 0x0000, 0x0000, 0x00d4, 0xb071, quirk_force_power_link },
+	/*
+	 * Intel hardware supports auto clear of the interrupt
+	 * status register right after interrupt is being
+	 * issued.
+	 */
+	{ 0x8087, 0x0000, 0x0000, 0x0000, quirk_auto_clear_interrupts },
+	/*
+	 * Falcon Ridge controller needs the end-to-end
+	 * flow control workaround to avoid losing Rx
+	 * packets when RING_FLAG_E2E is set.
+	 */
+	{ 0x8087, PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI, 0x0000, 0x0000,
+		  quirk_e2e },
+	{ 0x8087, PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI, 0x0000, 0x0000,
+		  quirk_e2e },
 	/*
 	 * Intel Goshen Ridge NVM 27 and before report wrong number of
 	 * DP buffers.
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index ce0a035800ab..07b19b17f6b5 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -27,6 +27,10 @@
 #define QUIRK_FORCE_POWER_LINK_CONTROLLER		BIT(0)
 /* Disable CLx if not supported */
 #define QUIRK_NO_CLX					BIT(1)
+/* Automatically clears interrupts status register */
+#define QUIRK_AUTO_CLEAR_INT				BIT(2)
+/* Needs end to end flow control */
+#define QUIRK_E2E					BIT(3)
 
 /**
  * struct tb_nvm - Structure holding NVM information
-- 
2.34.1

Re: [PATCH v2 2/2] thunderbolt: Move Intel quirks into quirks.c

[Mika Westerberg]

Hi,

On Mon, Apr 24, 2023 at 02:55:55PM -0500, Mario Limonciello wrote:
> There are two Intel specific quirks for auto clear and end to end
> that are not specified in the quirks file.  Move them to this location
> instead.

quirks.c is for USB4 domain quirks (router, retimer, anything actually
connected to the USB4 domain).

nhi.c is the correct place for host interface quirks for now.

RE: [PATCH v2 2/2] thunderbolt: Move Intel quirks into quirks.c

[Limonciello, Mario]

[Public]

> Hi,
> 
> On Mon, Apr 24, 2023 at 02:55:55PM -0500, Mario Limonciello wrote:
> > There are two Intel specific quirks for auto clear and end to end
> > that are not specified in the quirks file.  Move them to this location
> > instead.
> 
> quirks.c is for USB4 domain quirks (router, retimer, anything actually
> connected to the USB4 domain).
> 
> nhi.c is the correct place for host interface quirks for now.

OK, thanks.  If no other changes are needed for patch 1 please just drop
this one.  If changes end up being needed I'll drop this when I submit a v3.

RE: [PATCH v2 1/2] thunderbolt: Clear registers properly when auto clear isn't in use

[Limonciello, Mario]

[AMD Official Use Only - General]

> -----Original Message-----
> From: Limonciello, Mario <Mario.Limonciello@amd.com>
> Sent: Monday, April 24, 2023 2:56 PM
> To: Andreas Noever <andreas.noever@gmail.com>; Michael Jamet
> <michael.jamet@intel.com>; Mika Westerberg
> <mika.westerberg@linux.intel.com>; Yehezkel Bernat
> <YehezkelShB@gmail.com>; Limonciello, Mario
> <Mario.Limonciello@amd.com>
> Cc: S, Sanath <Sanath.S@amd.com>; Gong, Richard
> <Richard.Gong@amd.com>; Mehta, Sanju <Sanju.Mehta@amd.com>;
> Takashi Iwai <tiwai@suse.de>; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: [PATCH v2 1/2] thunderbolt: Clear registers properly when auto
> clear isn't in use
>
> When `QUIRK_AUTO_CLEAR_INT` isn't set, interrupt masking should be
> cleared by writing to Interrupt Mask Clear (IMR) and interrupt
> status should be cleared properly at shutdown/init.
>
> This fixes an error where interrupts are left enabled during resume
> from hibernation with `CONFIG_USB4=y`.
>
> Fixes: 468c49f44759 ("thunderbolt: Disable interrupt auto clear for rings")
> Reported-by: Takashi Iwai <tiwai@suse.de>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217343
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v1->v2:
>  * Whitespace changes
>  * Add new static functions for nhi_mask_interrupt() and
>    nhi_clear_interrupt()
>
> I tried to base this off thunderbolt.git/next (tag: thunderbolt-for-v6.4-rc1)
> but the following 3 commits are missing from that branch but are in 6.3-rc7:
>
> 58cdfe6f58b3 thunderbolt: Rename shadowed variables bit to interrupt_bit
> and auto_clear_bit
> 468c49f44759 thunderbolt: Disable interrupt auto clear for rings
> 1716efdb0793 thunderbolt: Use const qualifier for `ring_interrupt_index`
>
> I cherry picked them first as this patch builds on them. It's expected that
> this patch should apply on top of 6.4-rc1 properly.

Mika,

Since 6.4-rc1 is out, you should be able to test and apply this patch now.
If you need any changes I'll send a v3 dropping the second patch.

Thanks!

> ---
>  drivers/thunderbolt/nhi.c      | 29 ++++++++++++++++++++++++-----
>  drivers/thunderbolt/nhi_regs.h |  2 ++
>  2 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index d76e923fbc6a..c0aee5dc5237 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -54,6 +54,21 @@ static int ring_interrupt_index(const struct tb_ring
> *ring)
>       return bit;
>  }
>
> +static void nhi_mask_interrupt(struct tb_nhi *nhi, int mask, int ring)
> +{
> +     if (nhi->quirks & QUIRK_AUTO_CLEAR_INT)
> +             return;
> +     iowrite32(mask, nhi->iobase +
> REG_RING_INTERRUPT_MASK_CLEAR_BASE + ring);
> +}
> +
> +static void nhi_clear_interrupt(struct tb_nhi *nhi, int ring)
> +{
> +     if (nhi->quirks & QUIRK_AUTO_CLEAR_INT)
> +             ioread32(nhi->iobase + REG_RING_NOTIFY_BASE + ring);
> +     else
> +             iowrite32(~0, nhi->iobase + REG_RING_INT_CLEAR + ring);
> +}
> +
>  /*
>   * ring_interrupt_active() - activate/deactivate interrupts for a single ring
>   *
> @@ -61,8 +76,8 @@ static int ring_interrupt_index(const struct tb_ring
> *ring)
>   */
>  static void ring_interrupt_active(struct tb_ring *ring, bool active)
>  {
> -     int reg = REG_RING_INTERRUPT_BASE +
> -               ring_interrupt_index(ring) / 32 * 4;
> +     int index = ring_interrupt_index(ring) / 32 * 4;
> +     int reg = REG_RING_INTERRUPT_BASE + index;
>       int interrupt_bit = ring_interrupt_index(ring) & 31;
>       int mask = 1 << interrupt_bit;
>       u32 old, new;
> @@ -123,7 +138,11 @@ static void ring_interrupt_active(struct tb_ring *ring,
> bool active)
>                                        "interrupt for %s %d is already
> %s\n",
>                                        RING_TYPE(ring), ring->hop,
>                                        active ? "enabled" : "disabled");
> -     iowrite32(new, ring->nhi->iobase + reg);
> +
> +     if (active)
> +             iowrite32(new, ring->nhi->iobase + reg);
> +     else
> +             nhi_mask_interrupt(ring->nhi, mask, index);
>  }
>
>  /*
> @@ -136,11 +155,11 @@ static void nhi_disable_interrupts(struct tb_nhi
> *nhi)
>       int i = 0;
>       /* disable interrupts */
>       for (i = 0; i < RING_INTERRUPT_REG_COUNT(nhi); i++)
> -             iowrite32(0, nhi->iobase + REG_RING_INTERRUPT_BASE + 4
> * i);
> +             nhi_mask_interrupt(nhi, ~0, 4 * i);
>
>       /* clear interrupt status bits */
>       for (i = 0; i < RING_NOTIFY_REG_COUNT(nhi); i++)
> -             ioread32(nhi->iobase + REG_RING_NOTIFY_BASE + 4 * i);
> +             nhi_clear_interrupt(nhi, 4 * i);
>  }
>
>  /* ring helper methods */
> diff --git a/drivers/thunderbolt/nhi_regs.h b/drivers/thunderbolt/nhi_regs.h
> index faef165a919c..6ba295815477 100644
> --- a/drivers/thunderbolt/nhi_regs.h
> +++ b/drivers/thunderbolt/nhi_regs.h
> @@ -93,6 +93,8 @@ struct ring_desc {
>  #define REG_RING_INTERRUPT_BASE      0x38200
>  #define RING_INTERRUPT_REG_COUNT(nhi) ((31 + 2 * nhi->hop_count) /
> 32)
>
> +#define REG_RING_INTERRUPT_MASK_CLEAR_BASE   0x38208
> +
>  #define REG_INT_THROTTLING_RATE      0x38c00
>
>  /* Interrupt Vector Allocation */
> --
> 2.34.1

Re: [PATCH v2 1/2] thunderbolt: Clear registers properly when auto clear isn't in use

[Mika Westerberg]

Hi Mario,

On Mon, Apr 24, 2023 at 02:55:54PM -0500, Mario Limonciello wrote:
> When `QUIRK_AUTO_CLEAR_INT` isn't set, interrupt masking should be
> cleared by writing to Interrupt Mask Clear (IMR) and interrupt
> status should be cleared properly at shutdown/init.
> 
> This fixes an error where interrupts are left enabled during resume
> from hibernation with `CONFIG_USB4=y`.
> 
> Fixes: 468c49f44759 ("thunderbolt: Disable interrupt auto clear for rings")
> Reported-by: Takashi Iwai <tiwai@suse.de>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217343
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>

Applied to fixes, thanks!