| Message ID | 1627909100-83338-3-git-send-email-Sanju.Mehta@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for AMD USB4 and bug fixes | expand |
Hi, On Mon, Aug 02, 2021 at 07:58:18AM -0500, Sanjay R Mehta wrote: > From: Sanjay R Mehta <sanju.mehta@amd.com> > > when the "Disable ISR Auto-Clear" bit is set, the Tx/Rx ring > interrupt status is not cleared. Hence handling it by setting > the "Interrupt status clear" register to clear the > corresponding Tx/Rx ring interrupt. I think REG_DMA_MISC_INT_AUTO_CLEAR which is bit 2 in that register is actually Intel specific and not the same as in the USB4 spec bit 17. I guess the AMD controller works fine as is if this bit (2) is not set? If that's the case we can simply limit this behavior for Intel controllers. > > Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> > Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com> > --- > drivers/thunderbolt/nhi.c | 26 +++++++++++++++++++++++++- > include/linux/thunderbolt.h | 1 + > 2 files changed, 26 insertions(+), 1 deletion(-) > > diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c > index d7d9c4b..63bbabf 100644 > --- a/drivers/thunderbolt/nhi.c > +++ b/drivers/thunderbolt/nhi.c > @@ -74,7 +74,11 @@ static void ring_interrupt_active(struct tb_ring *ring, bool active) > if (!(misc & REG_DMA_MISC_INT_AUTO_CLEAR)) { > misc |= REG_DMA_MISC_INT_AUTO_CLEAR; > iowrite32(misc, ring->nhi->iobase + REG_DMA_MISC); > - } > + misc = ioread32(ring->nhi->iobase + REG_DMA_MISC); > + if (misc & REG_DMA_MISC_INT_AUTO_CLEAR) > + ring->nhi->is_intr_autoclr = true; > + } else > + ring->nhi->is_intr_autoclr = true; > > ivr_base = ring->nhi->iobase + REG_INT_VEC_ALLOC_BASE; > step = index / REG_INT_VEC_ALLOC_REGS * REG_INT_VEC_ALLOC_BITS; > @@ -377,11 +381,31 @@ void tb_ring_poll_complete(struct tb_ring *ring) > } > EXPORT_SYMBOL_GPL(tb_ring_poll_complete); > > +static void check_and_clear_intr_status(struct tb_ring *ring, int int_pos) > +{ > + u32 value; > + > + if (!ring->nhi->is_intr_autoclr) { > + value = ioread32(ring->nhi->iobase > + + REG_RING_NOTIFY_BASE > + + 4 * (int_pos / 32)); > + iowrite32(value, ring->nhi->iobase > + + (REG_RING_NOTIFY_BASE + 8) > + + 4 * (int_pos / 32)); > + } > +} > + > static irqreturn_t ring_msix(int irq, void *data) > { > struct tb_ring *ring = data; > > spin_lock(&ring->nhi->lock); > + > + if (ring->is_tx) > + check_and_clear_intr_status(ring, 0); > + else > + check_and_clear_intr_status(ring, ring->nhi->hop_count); > + > spin_lock(&ring->lock); > __ring_interrupt(ring); > spin_unlock(&ring->lock); > diff --git a/include/linux/thunderbolt.h b/include/linux/thunderbolt.h > index e7c96c3..bbe7c7e 100644 > --- a/include/linux/thunderbolt.h > +++ b/include/linux/thunderbolt.h > @@ -478,6 +478,7 @@ struct tb_nhi { > struct tb_ring **rx_rings; > struct ida msix_ida; > bool going_away; > + bool is_intr_autoclr; > struct work_struct interrupt_work; > u32 hop_count; > }; > -- > 2.7.4
On 8/2/2021 8:52 PM, Mika Westerberg wrote: > [CAUTION: External Email] > > Hi, > > On Mon, Aug 02, 2021 at 07:58:18AM -0500, Sanjay R Mehta wrote: >> From: Sanjay R Mehta <sanju.mehta@amd.com> >> >> when the "Disable ISR Auto-Clear" bit is set, the Tx/Rx ring >> interrupt status is not cleared. Hence handling it by setting >> the "Interrupt status clear" register to clear the >> corresponding Tx/Rx ring interrupt. > > I think REG_DMA_MISC_INT_AUTO_CLEAR which is bit 2 in that register is > actually Intel specific and not the same as in the USB4 spec bit 17. I > guess the AMD controller works fine as is if this bit (2) is not set? If > that's the case we can simply limit this behavior for Intel controllers. > Yes, you are right Mika. With bit 17 it's working for us and we didn't wanted to break Intel's functionality , hence we added this change which works for both :). Yes, as you suggested, we can limit this to Intel controllers. Will send the changes accordingly. Thanks, Sanjay >> >> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com> >> Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com> >> --- >> drivers/thunderbolt/nhi.c | 26 +++++++++++++++++++++++++- >> include/linux/thunderbolt.h | 1 + >> 2 files changed, 26 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c >> index d7d9c4b..63bbabf 100644 >> --- a/drivers/thunderbolt/nhi.c >> +++ b/drivers/thunderbolt/nhi.c >> @@ -74,7 +74,11 @@ static void ring_interrupt_active(struct tb_ring *ring, bool active) >> if (!(misc & REG_DMA_MISC_INT_AUTO_CLEAR)) { >> misc |= REG_DMA_MISC_INT_AUTO_CLEAR; >> iowrite32(misc, ring->nhi->iobase + REG_DMA_MISC); >> - } >> + misc = ioread32(ring->nhi->iobase + REG_DMA_MISC); >> + if (misc & REG_DMA_MISC_INT_AUTO_CLEAR) >> + ring->nhi->is_intr_autoclr = true; >> + } else >> + ring->nhi->is_intr_autoclr = true; >> >> ivr_base = ring->nhi->iobase + REG_INT_VEC_ALLOC_BASE; >> step = index / REG_INT_VEC_ALLOC_REGS * REG_INT_VEC_ALLOC_BITS; >> @@ -377,11 +381,31 @@ void tb_ring_poll_complete(struct tb_ring *ring) >> } >> EXPORT_SYMBOL_GPL(tb_ring_poll_complete); >> >> +static void check_and_clear_intr_status(struct tb_ring *ring, int int_pos) >> +{ >> + u32 value; >> + >> + if (!ring->nhi->is_intr_autoclr) { >> + value = ioread32(ring->nhi->iobase >> + + REG_RING_NOTIFY_BASE >> + + 4 * (int_pos / 32)); >> + iowrite32(value, ring->nhi->iobase >> + + (REG_RING_NOTIFY_BASE + 8) >> + + 4 * (int_pos / 32)); >> + } >> +} >> + >> static irqreturn_t ring_msix(int irq, void *data) >> { >> struct tb_ring *ring = data; >> >> spin_lock(&ring->nhi->lock); >> + >> + if (ring->is_tx) >> + check_and_clear_intr_status(ring, 0); >> + else >> + check_and_clear_intr_status(ring, ring->nhi->hop_count); >> + >> spin_lock(&ring->lock); >> __ring_interrupt(ring); >> spin_unlock(&ring->lock); >> diff --git a/include/linux/thunderbolt.h b/include/linux/thunderbolt.h >> index e7c96c3..bbe7c7e 100644 >> --- a/include/linux/thunderbolt.h >> +++ b/include/linux/thunderbolt.h >> @@ -478,6 +478,7 @@ struct tb_nhi { >> struct tb_ring **rx_rings; >> struct ida msix_ida; >> bool going_away; >> + bool is_intr_autoclr; >> struct work_struct interrupt_work; >> u32 hop_count; >> }; >> -- >> 2.7.4
diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c index d7d9c4b..63bbabf 100644 --- a/drivers/thunderbolt/nhi.c +++ b/drivers/thunderbolt/nhi.c @@ -74,7 +74,11 @@ static void ring_interrupt_active(struct tb_ring *ring, bool active) if (!(misc & REG_DMA_MISC_INT_AUTO_CLEAR)) { misc |= REG_DMA_MISC_INT_AUTO_CLEAR; iowrite32(misc, ring->nhi->iobase + REG_DMA_MISC); - } + misc = ioread32(ring->nhi->iobase + REG_DMA_MISC); + if (misc & REG_DMA_MISC_INT_AUTO_CLEAR) + ring->nhi->is_intr_autoclr = true; + } else + ring->nhi->is_intr_autoclr = true; ivr_base = ring->nhi->iobase + REG_INT_VEC_ALLOC_BASE; step = index / REG_INT_VEC_ALLOC_REGS * REG_INT_VEC_ALLOC_BITS; @@ -377,11 +381,31 @@ void tb_ring_poll_complete(struct tb_ring *ring) } EXPORT_SYMBOL_GPL(tb_ring_poll_complete); +static void check_and_clear_intr_status(struct tb_ring *ring, int int_pos) +{ + u32 value; + + if (!ring->nhi->is_intr_autoclr) { + value = ioread32(ring->nhi->iobase + + REG_RING_NOTIFY_BASE + + 4 * (int_pos / 32)); + iowrite32(value, ring->nhi->iobase + + (REG_RING_NOTIFY_BASE + 8) + + 4 * (int_pos / 32)); + } +} + static irqreturn_t ring_msix(int irq, void *data) { struct tb_ring *ring = data; spin_lock(&ring->nhi->lock); + + if (ring->is_tx) + check_and_clear_intr_status(ring, 0); + else + check_and_clear_intr_status(ring, ring->nhi->hop_count); + spin_lock(&ring->lock); __ring_interrupt(ring); spin_unlock(&ring->lock); diff --git a/include/linux/thunderbolt.h b/include/linux/thunderbolt.h index e7c96c3..bbe7c7e 100644 --- a/include/linux/thunderbolt.h +++ b/include/linux/thunderbolt.h @@ -478,6 +478,7 @@ struct tb_nhi { struct tb_ring **rx_rings; struct ida msix_ida; bool going_away; + bool is_intr_autoclr; struct work_struct interrupt_work; u32 hop_count; };