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

[Mario Limonciello]

On 4/24/23 00:58, Mika Westerberg wrote:
> Hi Mario,
>
> On Fri, Apr 21, 2023 at 09:07:24AM -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@xxxxxxx>
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217343
> > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > Tested-by: Takashi Iwai <tiwai@xxxxxxx>
> > ---
> > 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.
> Okay, so if I take this after v6.4-rc1 is released and send it forward
> to for -rc2 Greg it should apply just fine?
Yes; that's right.  I'll rework it for your below comments and that plan 
sounds good to me.
> > ---
> >   drivers/thunderbolt/nhi.c      | 28 +++++++++++++++++++++-------
> >   drivers/thunderbolt/nhi_regs.h |  1 +
> >   2 files changed, 22 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> > index d76e923fbc6a..7c543a6a5711 100644
> > --- a/drivers/thunderbolt/nhi.c
> > +++ b/drivers/thunderbolt/nhi.c
> > @@ -61,8 +61,9 @@ 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 clear = REG_RING_INTERRUPT_MASK_CLEAR_BASE + index;
> >   	int interrupt_bit = ring_interrupt_index(ring) & 31;
> >   	int mask = 1 << interrupt_bit;
> >   	u32 old, new;
> > @@ -123,7 +124,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
> > +		iowrite32(mask, ring->nhi->iobase + clear);
> Since it is doing this for all hardware, even for Intel, I will need to
> run some testing to make sure this still works.
OK.
> >   }
> >   
> >   /*
> > @@ -135,12 +140,21 @@ 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);
> > +	for (i = 0; i < RING_INTERRUPT_REG_COUNT(nhi); i++) {
> > +		if (nhi->quirks & QUIRK_AUTO_CLEAR_INT)
> > +			iowrite32(0, nhi->iobase + REG_RING_INTERRUPT_BASE + 4 * i);
> > +		else
> > +			iowrite32(0xffffffff,
> ~0
>
> > +				  nhi->iobase + REG_RING_INTERRUPT_MASK_CLEAR_BASE + 4 * i);
> Btw, we have now quite many places with
>
> 	if (nhi->quirks & QUIRK_AUTO_CLEAR_INT)
> 		// Intel stuff
> 	else
> 		// non-Intel stuff
>
> I wonder if we could move these behind a wrapper and then here (and
> similar places) just call
>
> 	nhi_mask_interrupt(nhi, ...)

Sure, I'll experiment with this, it does sound like a generally good 
improvement for the code.

> > +	}
> >   
> >   	/* clear interrupt status bits */
> > -	for (i = 0; i < RING_NOTIFY_REG_COUNT(nhi); i++)
> > -		ioread32(nhi->iobase + REG_RING_NOTIFY_BASE + 4 * i);
> > +	for (i = 0; i < RING_NOTIFY_REG_COUNT(nhi); i++) {
> > +		if (nhi->quirks & QUIRK_AUTO_CLEAR_INT)
> > +			ioread32(nhi->iobase + REG_RING_NOTIFY_BASE + 4 * i);
> > +		else
> > +			iowrite32(0xffffffff, nhi->iobase + REG_RING_INT_CLEAR + 4 * i);
> ~0
>
> 	nhi_clear_interrupt(nhi, ...)
>
> > +	}
> >   }
> >   
> >   /* ring helper methods */
> > diff --git a/drivers/thunderbolt/nhi_regs.h b/drivers/thunderbolt/nhi_regs.h
> > index faef165a919c..db95ad5d2814 100644
> > --- a/drivers/thunderbolt/nhi_regs.h
> > +++ b/drivers/thunderbolt/nhi_regs.h
> > @@ -92,6 +92,7 @@ struct ring_desc {
> >    */
> >   #define REG_RING_INTERRUPT_BASE	0x38200
> >   #define RING_INTERRUPT_REG_COUNT(nhi) ((31 + 2 * nhi->hop_count) / 32)
> Empty line here.
>
> > +#define REG_RING_INTERRUPT_MASK_CLEAR_BASE	0x38208
> >   
> >   #define REG_INT_THROTTLING_RATE	0x38c00
> >   
> > --
> > 2.34.1