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@xxxxxxx>
> Sent: Monday, April 24, 2023 2:56 PM
> To: Andreas Noever <andreas.noever@xxxxxxxxx>; Michael Jamet
> <michael.jamet@xxxxxxxxx>; Mika Westerberg
> <mika.westerberg@xxxxxxxxxxxxxxx>; Yehezkel Bernat
> <YehezkelShB@xxxxxxxxx>; Limonciello, Mario
> <Mario.Limonciello@xxxxxxx>
> Cc: S, Sanath <Sanath.S@xxxxxxx>; Gong, Richard
> <Richard.Gong@xxxxxxx>; Mehta, Sanju <Sanju.Mehta@xxxxxxx>;
> Takashi Iwai <tiwai@xxxxxxx>; linux-usb@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxx>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217343
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> ---
> 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