On Thu, Aug 05, 2021 at 06:29:36PM +0530, Sanjay R Mehta wrote:
>
>
> On 8/4/2021 9:18 PM, Mika Westerberg wrote:
> > [CAUTION: External Email]
> >
> > Hi,
> >
> > On Tue, Aug 03, 2021 at 07:34:54AM -0500, Sanjay R Mehta wrote:
> >> From: Sanjay R Mehta <sanju.mehta@xxxxxxx>
> >>
> >> As per USB4 spec by default "Disable ISR Auto-Clear" bit is set to 0,
> >> and the Tx/Rx ring interrupt status is needs to be cleared.
> >>
> >> Hence handling it by reading the "Interrupt status" register in the ISR.
> >>
> >> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@xxxxxxx>
> >> Signed-off-by: Sanjay R Mehta <sanju.mehta@xxxxxxx>
> >> ---
> >> drivers/thunderbolt/nhi.c | 14 ++++++++++++++
> >> 1 file changed, 14 insertions(+)
> >>
> >> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> >> index ef01aa6..7ad2202 100644
> >> --- a/drivers/thunderbolt/nhi.c
> >> +++ b/drivers/thunderbolt/nhi.c
> >> @@ -373,11 +373,25 @@ 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)
> >> +{
> >> + if (!(ring->nhi->pdev->vendor == PCI_VENDOR_ID_INTEL)) {
> >> + if (ring->is_tx)
> >> + ioread32(ring->nhi->iobase
> >> + + REG_RING_NOTIFY_BASE);
> >> + else
> >> + ioread32(ring->nhi->iobase
> >> + + REG_RING_NOTIFY_BASE
> >> + + 4 * (ring->nhi->hop_count / 32));
> >> + }
> >> +}
> >
> > I'm now playing with this series on Intel hardware. I wanted to check
> > from you whether the AMD controller implements the Auto-Clear feature? I
> > mean if we always clear bit 17 of the Host Interface Control register do
> > you still need to call the above or it is cleared automatically?
> >
> Yes, AMD implements Auto-Clear and a read operation is required to clear
> the interrupt status.
>
> It is explicitly described in the Spec, Section "12.6.3.4.1 -> "Table
> 12-27. Interrupt Status" as below
>
> "If the Disable ISR Auto-Clear bit is set to 0b, then a read operation
> returns the current value and then clears the register to 0."
D'oh, right. It is about auto clear of the ISS register on read or not.
I misunderstood the whole bit.
>
> > I'm hoping that we could make this work on all controllers without too
> > many special cases ;-)
>
> Will it be good idea to have a separate variable in "struct tb_nhi" as
> "nhi->is_intr_autoclr" so that we can set in the
> "quirk_enable_intr_auto_clr()" as required which can be used in above
> check_and_clear_intr_status() function instead of vendor check.
Probably that would work better. Let me try to figure out if we can
somehow do the same in Intel controller too so we would only have single
path here.
