Re: [PATCH v2 2/4] thunderbolt: Handle ring interrupt by reading intr status

[Sanjay R Mehta <sanmehta@amd.com>]

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@amd.com>
>>
>> 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@amd.com>
>> Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
>> ---
>>  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."


> 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.

>