[PATCH v2 0/4] Added some bug fixes for USB4

[PATCH v2 0/4] Added some bug fixes for USB4

[Sanjay R Mehta]

From: Sanjay R Mehta <sanju.mehta@amd.com>

This series adds some general USB4 bug fixes as per USB4 Spec.

Sanjay R Mehta (4):
  thunderbolt: Intel controller uses BIT(2) for intr auto
  thunderbolt: Handle ring interrupt by reading intr status
  thunderbolt: Skip port init for control adapter(0)
  thunderbolt: Fix port linking by checking all adapters

 drivers/thunderbolt/nhi.c    | 22 ++++++++++++++++------
 drivers/thunderbolt/quirks.c | 14 ++++++++++++++
 drivers/thunderbolt/switch.c |  4 ++--
 drivers/thunderbolt/tb.h     |  1 +
 4 files changed, 33 insertions(+), 8 deletions(-)

-- 
2.7.4

[PATCH v2 1/4] thunderbolt: Intel controller uses BIT(2) for intr auto

[Sanjay R Mehta]

From: Sanjay R Mehta <sanju.mehta@amd.com>

REG_DMA_MISC_INT_AUTO_CLEAR which is bit 2 in that register is
actually Intel specific.

As per the USB4 spec bit 17 is used for interrupt auto clear and
by default its enabled.

Hence limit usage of REG_DMA_MISC_INT_AUTO_CLEAR for Intel controllers
and moved this to quirk.

Fixes: 046bee1f9ab8 ("thunderbolt: Add MSI-X support")
Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
---
 drivers/thunderbolt/nhi.c    |  8 ++------
 drivers/thunderbolt/quirks.c | 14 ++++++++++++++
 drivers/thunderbolt/tb.h     |  1 +
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index fa44332..ef01aa6 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -57,8 +57,8 @@ static void ring_interrupt_active(struct tb_ring *ring, bool active)
 	u32 old, new;
 
 	if (ring->irq > 0) {
-		u32 step, shift, ivr, misc;
 		void __iomem *ivr_base;
+		u32 step, shift, ivr;
 		int index;
 
 		if (ring->is_tx)
@@ -70,11 +70,7 @@ static void ring_interrupt_active(struct tb_ring *ring, bool active)
 		 * Ask the hardware to clear interrupt status bits automatically
 		 * since we already know which interrupt was triggered.
 		 */
-		misc = ioread32(ring->nhi->iobase + REG_DMA_MISC);
-		if (!(misc & REG_DMA_MISC_INT_AUTO_CLEAR)) {
-			misc |= REG_DMA_MISC_INT_AUTO_CLEAR;
-			iowrite32(misc, ring->nhi->iobase + REG_DMA_MISC);
-		}
+		quirk_enable_intr_auto_clr(ring);
 
 		ivr_base = ring->nhi->iobase + REG_INT_VEC_ALLOC_BASE;
 		step = index / REG_INT_VEC_ALLOC_REGS * REG_INT_VEC_ALLOC_BITS;
diff --git a/drivers/thunderbolt/quirks.c b/drivers/thunderbolt/quirks.c
index b5f2ec7..af6dab9 100644
--- a/drivers/thunderbolt/quirks.c
+++ b/drivers/thunderbolt/quirks.c
@@ -6,6 +6,7 @@
  */
 
 #include "tb.h"
+#include "nhi_regs.h"
 
 static void quirk_force_power_link(struct tb_switch *sw)
 {
@@ -64,3 +65,16 @@ void tb_check_quirks(struct tb_switch *sw)
 		q->hook(sw);
 	}
 }
+
+void quirk_enable_intr_auto_clr(struct tb_ring *ring)
+{
+	u32 misc;
+
+	if (ring->nhi->pdev->vendor == PCI_VENDOR_ID_INTEL) {
+		misc = ioread32(ring->nhi->iobase + REG_DMA_MISC);
+		if (!(misc & REG_DMA_MISC_INT_AUTO_CLEAR)) {
+			misc |= REG_DMA_MISC_INT_AUTO_CLEAR;
+			iowrite32(misc, ring->nhi->iobase + REG_DMA_MISC);
+		}
+	}
+}
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index 725104c..0b8f9d3 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -1122,6 +1122,7 @@ int usb4_port_device_resume(struct usb4_port *usb4);
 #define QUIRK_FORCE_POWER_LINK_CONTROLLER		BIT(0)
 
 void tb_check_quirks(struct tb_switch *sw);
+void quirk_enable_intr_auto_clr(struct tb_ring *ring);
 
 #ifdef CONFIG_ACPI
 void tb_acpi_add_links(struct tb_nhi *nhi);
-- 
2.7.4

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

[Sanjay R Mehta]

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));
+	}
+}
+
 static irqreturn_t ring_msix(int irq, void *data)
 {
 	struct tb_ring *ring = data;
 
 	spin_lock(&ring->nhi->lock);
+	check_and_clear_intr_status(ring);
 	spin_lock(&ring->lock);
 	__ring_interrupt(ring);
 	spin_unlock(&ring->lock);
-- 
2.7.4

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

[Mika Westerberg]

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?

I'm hoping that we could make this work on all controllers without too
many special cases ;-)

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

[Mika Westerberg]

On Wed, Aug 04, 2021 at 06:48:45PM +0300, Mika Westerberg wrote:
> 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?
> 
> I'm hoping that we could make this work on all controllers without too
> many special cases ;-)

I mean if you replace patches 1 and 2 in this series with the below,
does it work with the AMD controller too?

diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index fa44332845a1..8a5656fb956f 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -71,10 +71,14 @@ static void ring_interrupt_active(struct tb_ring *ring, bool active)
 		 * since we already know which interrupt was triggered.
 		 */
 		misc = ioread32(ring->nhi->iobase + REG_DMA_MISC);
-		if (!(misc & REG_DMA_MISC_INT_AUTO_CLEAR)) {
+		/* Special bit for Intel */
+		if (ring->nhi->pdev->vendor == PCI_VENDOR_ID_INTEL &&
+		    !(misc & REG_DMA_MISC_INT_AUTO_CLEAR))
 			misc |= REG_DMA_MISC_INT_AUTO_CLEAR;
-			iowrite32(misc, ring->nhi->iobase + REG_DMA_MISC);
-		}
+		/* USB4 clear the disable auto-clear bit */
+		if (misc & BIT(17))
+			misc &= ~BIT(17);
+		iowrite32(misc, ring->nhi->iobase + REG_DMA_MISC);
 
 		ivr_base = ring->nhi->iobase + REG_INT_VEC_ALLOC_BASE;
 		step = index / REG_INT_VEC_ALLOC_REGS * REG_INT_VEC_ALLOC_BITS;

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

[Sanjay R Mehta]

On 8/5/2021 6:26 PM, Mika Westerberg wrote:
> [CAUTION: External Email]
> 
> On Wed, Aug 04, 2021 at 06:48:45PM +0300, Mika Westerberg wrote:
>> 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?
>>
>> I'm hoping that we could make this work on all controllers without too
>> many special cases ;-)
> 
> I mean if you replace patches 1 and 2 in this series with the below,
> does it work with the AMD controller too?
> 
Actually, it wont work on AMD controller because explicit read operation
of interrupt status is required to clear it.

> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index fa44332845a1..8a5656fb956f 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -71,10 +71,14 @@ static void ring_interrupt_active(struct tb_ring *ring, bool active)
>                  * since we already know which interrupt was triggered.
>                  */
>                 misc = ioread32(ring->nhi->iobase + REG_DMA_MISC);
> -               if (!(misc & REG_DMA_MISC_INT_AUTO_CLEAR)) {
> +               /* Special bit for Intel */
> +               if (ring->nhi->pdev->vendor == PCI_VENDOR_ID_INTEL &&
> +                   !(misc & REG_DMA_MISC_INT_AUTO_CLEAR))
>                         misc |= REG_DMA_MISC_INT_AUTO_CLEAR;
> -                       iowrite32(misc, ring->nhi->iobase + REG_DMA_MISC);
> -               }
> +               /* USB4 clear the disable auto-clear bit */
> +               if (misc & BIT(17))
> +                       misc &= ~BIT(17);
> +               iowrite32(misc, ring->nhi->iobase + REG_DMA_MISC);
> 
>                 ivr_base = ring->nhi->iobase + REG_INT_VEC_ALLOC_BASE;
>                 step = index / REG_INT_VEC_ALLOC_REGS * REG_INT_VEC_ALLOC_BITS;
> 

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

[Mika Westerberg]

On Thu, Aug 05, 2021 at 06:36:17PM +0530, Sanjay R Mehta wrote:
> 
> 
> On 8/5/2021 6:26 PM, Mika Westerberg wrote:
> > [CAUTION: External Email]
> > 
> > On Wed, Aug 04, 2021 at 06:48:45PM +0300, Mika Westerberg wrote:
> >> 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?
> >>
> >> I'm hoping that we could make this work on all controllers without too
> >> many special cases ;-)
> > 
> > I mean if you replace patches 1 and 2 in this series with the below,
> > does it work with the AMD controller too?
> > 
> Actually, it wont work on AMD controller because explicit read operation
> of interrupt status is required to clear it.

Indeed, I misunderstood the bit. Please ignore this.

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

[Sanjay R Mehta]

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.

> 

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

[Mika Westerberg]

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

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.

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

[Mika Westerberg]

On Thu, Aug 05, 2021 at 05:19:39PM +0300, Mika Westerberg wrote:
> 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@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."
> 
> 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.

Looks like we cannot get it working without quirk of some kind :( I
think we can do this:

  1. Add nhi_check_quirks() to nhi.c and that one checks for
     PCI_VENDOR_ID_INTEL and sets nhi->quirks |= DMA_MISC_INT_AUTO_CLEAR.
  2. Then check it in both ring_interrupt_active() and in
     ring_clear_msix(ring) and if set handle the case accordingly.

Let's add nhi->quirks directly now because I have a feeling that we may
need additional flags in the future ;-)

Would you like to update this series with the above changes or you want
me to do that?

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

[Sanjay R Mehta]

On 8/5/2021 8:16 PM, Mika Westerberg wrote:
> [CAUTION: External Email]
> 
> On Thu, Aug 05, 2021 at 05:19:39PM +0300, Mika Westerberg wrote:
>> 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@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."
>>
>> 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.
> 
> Looks like we cannot get it working without quirk of some kind :( I
> think we can do this:
> 
>   1. Add nhi_check_quirks() to nhi.c and that one checks for
>      PCI_VENDOR_ID_INTEL and sets nhi->quirks |= DMA_MISC_INT_AUTO_CLEAR.
>   2. Then check it in both ring_interrupt_active() and in
>      ring_clear_msix(ring) and if set handle the case accordingly.
> 
> Let's add nhi->quirks directly now because I have a feeling that we may
> need additional flags in the future ;-)
> 
> Would you like to update this series with the above changes or you want
> me to do that?

Sounds good. Sure, I will update this series as per your suggestion.
Thank you :)
> 

[PATCH v2 3/4] thunderbolt: Skip port init for control adapter(0)

[Sanjay R Mehta]

From: Sanjay R Mehta <sanju.mehta@amd.com>

Adapter (0) is control adapter and as per USB4 spec in "Section 1.8",
Control Adapters do not have an Adapter Configuration Space".

Hence skip port initialization for adapter (0).

Fixes: e6b245ccd524 ("thunderbolt: Add support for host and device NVM firmware upgrade")
Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
---
 drivers/thunderbolt/switch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 83b1ef3..6447876 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -2740,7 +2740,7 @@ int tb_switch_add(struct tb_switch *sw)
 			return ret;
 		}
 
-		for (i = 0; i <= sw->config.max_port_number; i++) {
+		for (i = 1; i <= sw->config.max_port_number; i++) {
 			if (sw->ports[i].disabled) {
 				tb_port_dbg(&sw->ports[i], "disabled by eeprom\n");
 				continue;
-- 
2.7.4

[PATCH v2 4/4] thunderbolt: Fix port linking by checking all adapters

[Sanjay R Mehta]

From: Sanjay R Mehta <sanju.mehta@amd.com>

In tb_switch_default_link_ports(), while linking of ports,
only odd-numbered ports (1,3,5..) are considered and even-numbered
ports are not considered.

AMD host router has lane adapters at 2 & 3 and link ports at
adapter-2 is not considered due to which lane bonding gets disabled.

Hence added a fix such that all ports are considered during
linking of ports.

Fixes: 0d46c08d1ed4 ("thunderbolt: Add default linking between lane adapters if not provided by DROM")
Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
---
 drivers/thunderbolt/switch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 6447876..5c3d4bd 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -2456,7 +2456,7 @@ static void tb_switch_default_link_ports(struct tb_switch *sw)
 {
 	int i;
 
-	for (i = 1; i <= sw->config.max_port_number; i += 2) {
+	for (i = 1; i <= sw->config.max_port_number; i++) {
 		struct tb_port *port = &sw->ports[i];
 		struct tb_port *subordinate;
 
-- 
2.7.4