[v2,1/2] thunderbolt: Clear registers properly when auto clear isn't in use
Commit Message
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(-)
Comments
[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
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!
@@ -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 */
@@ -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 */