[PATCH 0/4] Add support for AMD USB4 and bug fixes

[PATCH 0/4] Add support for AMD USB4 and bug fixes

[Sanjay R Mehta]

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

This series adds support for AMD USB4 host router and
some general USB4 bug fixes.

Sanjay R Mehta (4):
  PCI: Add AMD USB4 host router device IDs
  thunderbolt: Handle INTR when Disable ISR auto clear bit set
  thunderbolt: Fix adapter init handling during switch add
  thunderbolt: Fix port linking by checking all adapters

 drivers/thunderbolt/nhi.c    | 30 +++++++++++++++++++++++++++++-
 drivers/thunderbolt/switch.c |  5 +++--
 include/linux/pci_ids.h      |  2 ++
 include/linux/thunderbolt.h  |  1 +
 4 files changed, 35 insertions(+), 3 deletions(-)

-- 
2.7.4

[PATCH 1/4] PCI: Add AMD USB4 host router device IDs

[Sanjay R Mehta]

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

This patch enables support for AMD USB4 host router.

Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
---
 drivers/thunderbolt/nhi.c | 4 ++++
 include/linux/pci_ids.h   | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index fa44332..d7d9c4b 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -1338,6 +1338,10 @@ static struct pci_device_id nhi_ids[] = {
 	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ADL_NHI1),
 	  .driver_data = (kernel_ulong_t)&icl_nhi_ops },
 
+	/* AMD USB4 host */
+	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_USB4_HIA0) },
+	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_USB4_HIA1) },
+
 	/* Any USB4 compliant host */
 	{ PCI_DEVICE_CLASS(PCI_CLASS_SERIAL_USB_USB4, ~0) },
 
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 4bac183..6d50019 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -604,6 +604,8 @@
 #define PCI_DEVICE_ID_AMD_HUDSON2_SMBUS		0x780b
 #define PCI_DEVICE_ID_AMD_HUDSON2_IDE		0x780c
 #define PCI_DEVICE_ID_AMD_KERNCZ_SMBUS  0x790b
+#define PCI_DEVICE_ID_AMD_USB4_HIA0	0x162e
+#define PCI_DEVICE_ID_AMD_USB4_HIA1	0x162f
 
 #define PCI_VENDOR_ID_TRIDENT		0x1023
 #define PCI_DEVICE_ID_TRIDENT_4DWAVE_DX	0x2000
-- 
2.7.4

Re: [PATCH 1/4] PCI: Add AMD USB4 host router device IDs

[Mika Westerberg]

On Mon, Aug 02, 2021 at 07:58:17AM -0500, Sanjay R Mehta wrote:
> From: Sanjay R Mehta <sanju.mehta@amd.com>
> 
> This patch enables support for AMD USB4 host router.
> 
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
> ---
>  drivers/thunderbolt/nhi.c | 4 ++++
>  include/linux/pci_ids.h   | 2 ++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index fa44332..d7d9c4b 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -1338,6 +1338,10 @@ static struct pci_device_id nhi_ids[] = {
>  	{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ADL_NHI1),
>  	  .driver_data = (kernel_ulong_t)&icl_nhi_ops },
>  
> +	/* AMD USB4 host */
> +	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_USB4_HIA0) },
> +	{ PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_USB4_HIA1) },
> +
>  	/* Any USB4 compliant host */
>  	{ PCI_DEVICE_CLASS(PCI_CLASS_SERIAL_USB_USB4, ~0) },
>  
> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 4bac183..6d50019 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -604,6 +604,8 @@
>  #define PCI_DEVICE_ID_AMD_HUDSON2_SMBUS		0x780b
>  #define PCI_DEVICE_ID_AMD_HUDSON2_IDE		0x780c
>  #define PCI_DEVICE_ID_AMD_KERNCZ_SMBUS  0x790b
> +#define PCI_DEVICE_ID_AMD_USB4_HIA0	0x162e
> +#define PCI_DEVICE_ID_AMD_USB4_HIA1	0x162f

No need to add them here (and you actually should not since these IDs
are not shared between multiple drivers, see the top level comment in
this header).

I suggest adding these to drivers/thunderbolt/nhi.h instead.

>  
>  #define PCI_VENDOR_ID_TRIDENT		0x1023
>  #define PCI_DEVICE_ID_TRIDENT_4DWAVE_DX	0x2000
> -- 
> 2.7.4

Re: [PATCH 1/4] PCI: Add AMD USB4 host router device IDs

[Sanjay R Mehta]

On 8/2/2021 8:42 PM, Mika Westerberg wrote:
> [CAUTION: External Email]
> 
> On Mon, Aug 02, 2021 at 07:58:17AM -0500, Sanjay R Mehta wrote:
>> From: Sanjay R Mehta <sanju.mehta@amd.com>
>>
>> This patch enables support for AMD USB4 host router.
>>
>> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>> Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
>> ---
>>  drivers/thunderbolt/nhi.c | 4 ++++
>>  include/linux/pci_ids.h   | 2 ++
>>  2 files changed, 6 insertions(+)
>>
>> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
>> index fa44332..d7d9c4b 100644
>> --- a/drivers/thunderbolt/nhi.c
>> +++ b/drivers/thunderbolt/nhi.c
>> @@ -1338,6 +1338,10 @@ static struct pci_device_id nhi_ids[] = {
>>       { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ADL_NHI1),
>>         .driver_data = (kernel_ulong_t)&icl_nhi_ops },
>>
>> +     /* AMD USB4 host */
>> +     { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_USB4_HIA0) },
>> +     { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_USB4_HIA1) },
>> +
>>       /* Any USB4 compliant host */
>>       { PCI_DEVICE_CLASS(PCI_CLASS_SERIAL_USB_USB4, ~0) },
>>
>> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
>> index 4bac183..6d50019 100644
>> --- a/include/linux/pci_ids.h
>> +++ b/include/linux/pci_ids.h
>> @@ -604,6 +604,8 @@
>>  #define PCI_DEVICE_ID_AMD_HUDSON2_SMBUS              0x780b
>>  #define PCI_DEVICE_ID_AMD_HUDSON2_IDE                0x780c
>>  #define PCI_DEVICE_ID_AMD_KERNCZ_SMBUS  0x790b
>> +#define PCI_DEVICE_ID_AMD_USB4_HIA0  0x162e
>> +#define PCI_DEVICE_ID_AMD_USB4_HIA1  0x162f
> 
> No need to add them here (and you actually should not since these IDs
> are not shared between multiple drivers, see the top level comment in
> this header).
> 
> I suggest adding these to drivers/thunderbolt/nhi.h instead.
> 
Thanks Mika. Agree and will add it in drivers/thunderbolt/nhi.h .

>>
>>  #define PCI_VENDOR_ID_TRIDENT                0x1023
>>  #define PCI_DEVICE_ID_TRIDENT_4DWAVE_DX      0x2000
>> --
>> 2.7.4

Re: [PATCH 1/4] PCI: Add AMD USB4 host router device IDs

[Mika Westerberg]

Hi,

I have one more comment, see below.

On Mon, Aug 02, 2021 at 11:39:32PM +0530, Sanjay R Mehta wrote:
> 
> 
> On 8/2/2021 8:42 PM, Mika Westerberg wrote:
> > [CAUTION: External Email]
> > 
> > On Mon, Aug 02, 2021 at 07:58:17AM -0500, Sanjay R Mehta wrote:
> >> From: Sanjay R Mehta <sanju.mehta@amd.com>
> >>
> >> This patch enables support for AMD USB4 host router.
> >>
> >> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> >> Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
> >> ---
> >>  drivers/thunderbolt/nhi.c | 4 ++++
> >>  include/linux/pci_ids.h   | 2 ++
> >>  2 files changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> >> index fa44332..d7d9c4b 100644
> >> --- a/drivers/thunderbolt/nhi.c
> >> +++ b/drivers/thunderbolt/nhi.c
> >> @@ -1338,6 +1338,10 @@ static struct pci_device_id nhi_ids[] = {
> >>       { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ADL_NHI1),
> >>         .driver_data = (kernel_ulong_t)&icl_nhi_ops },
> >>
> >> +     /* AMD USB4 host */
> >> +     { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_USB4_HIA0) },
> >> +     { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_USB4_HIA1) },
> >> +

I wonder if AMD USB4 controller exposes the USB4 PCI class ID? If that's
the case and you don't need any special quirking like Intel does then we
don't need to add any PCI IDs here and allow the below line to match.

This is actually what I hope we get eventually in Intel HW too.

> >>       /* Any USB4 compliant host */
> >>       { PCI_DEVICE_CLASS(PCI_CLASS_SERIAL_USB_USB4, ~0) },

Re: [PATCH 1/4] PCI: Add AMD USB4 host router device IDs

[Sanjay R Mehta]

On 8/3/2021 3:15 PM, Mika Westerberg wrote:
> [CAUTION: External Email]
> 
> Hi,
> 
> I have one more comment, see below.
> 
> On Mon, Aug 02, 2021 at 11:39:32PM +0530, Sanjay R Mehta wrote:
>>
>>
>> On 8/2/2021 8:42 PM, Mika Westerberg wrote:
>>> [CAUTION: External Email]
>>>
>>> On Mon, Aug 02, 2021 at 07:58:17AM -0500, Sanjay R Mehta wrote:
>>>> From: Sanjay R Mehta <sanju.mehta@amd.com>
>>>>
>>>> This patch enables support for AMD USB4 host router.
>>>>
>>>> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>>>> Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
>>>> ---
>>>>  drivers/thunderbolt/nhi.c | 4 ++++
>>>>  include/linux/pci_ids.h   | 2 ++
>>>>  2 files changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
>>>> index fa44332..d7d9c4b 100644
>>>> --- a/drivers/thunderbolt/nhi.c
>>>> +++ b/drivers/thunderbolt/nhi.c
>>>> @@ -1338,6 +1338,10 @@ static struct pci_device_id nhi_ids[] = {
>>>>       { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_ADL_NHI1),
>>>>         .driver_data = (kernel_ulong_t)&icl_nhi_ops },
>>>>
>>>> +     /* AMD USB4 host */
>>>> +     { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_USB4_HIA0) },
>>>> +     { PCI_VDEVICE(AMD, PCI_DEVICE_ID_AMD_USB4_HIA1) },
>>>> +
> 
> I wonder if AMD USB4 controller exposes the USB4 PCI class ID? If that's
> the case and you don't need any special quirking like Intel does then we
> don't need to add any PCI IDs here and allow the below line to match.
> 
> This is actually what I hope we get eventually in Intel HW too.

yes, make sense. Will remove this from the series.

Thanks,
Sanjay

> 
>>>>       /* Any USB4 compliant host */
>>>>       { PCI_DEVICE_CLASS(PCI_CLASS_SERIAL_USB_USB4, ~0) },

[PATCH 2/4] thunderbolt: Handle INTR when Disable ISR auto clear bit set

[Sanjay R Mehta]

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.

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

Re: [PATCH 2/4] thunderbolt: Handle INTR when Disable ISR auto clear bit set

[Mika Westerberg]

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

Re: [PATCH 2/4] thunderbolt: Handle INTR when Disable ISR auto clear bit set

[Sanjay R Mehta]

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

[PATCH 3/4] thunderbolt: Fix adapter init handling during switch add

[Sanjay R Mehta]

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

Adapter0 (Port0) is the control adapter on the AMD USB4 host router.
As per USB4 spec in "Section 1.8", Control Adapters do not
have an Adapter Configuration Space".

The read requests on Adapter0 time's out and driver initialization fails.

Hence Disabling the Adapter in case of read-request timeout and continuing
the driver init.

Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
---
 drivers/thunderbolt/switch.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 83b1ef3..effbfe4 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -2747,8 +2747,9 @@ int tb_switch_add(struct tb_switch *sw)
 			}
 			ret = tb_init_port(&sw->ports[i]);
 			if (ret) {
+				sw->ports[i].disabled = true;
 				dev_err(&sw->dev, "failed to initialize port %d\n", i);
-				return ret;
+				continue;
 			}
 		}
 
-- 
2.7.4

Re: [PATCH 3/4] thunderbolt: Fix adapter init handling during switch add

[Mika Westerberg]

On Mon, Aug 02, 2021 at 07:58:19AM -0500, Sanjay R Mehta wrote:
> From: Sanjay R Mehta <sanju.mehta@amd.com>
> 
> Adapter0 (Port0) is the control adapter on the AMD USB4 host router.
> As per USB4 spec in "Section 1.8", Control Adapters do not
> have an Adapter Configuration Space".
> 
> The read requests on Adapter0 time's out and driver initialization fails.
> 
> Hence Disabling the Adapter in case of read-request timeout and continuing
> the driver init.
> 
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
> ---
>  drivers/thunderbolt/switch.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> index 83b1ef3..effbfe4 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -2747,8 +2747,9 @@ int tb_switch_add(struct tb_switch *sw)
>  			}
>  			ret = tb_init_port(&sw->ports[i]);
>  			if (ret) {
> +				sw->ports[i].disabled = true;
>  				dev_err(&sw->dev, "failed to initialize port %d\n", i);
> -				return ret;
> +				continue;

Instead of this, would it work if we start the loop at 1? In case of the
control adapter (0) tb_port_init() does not do anything useful anyway
and it actually would simplify that function too if we get rid of the
special casing.

>  			}
>  		}
>  
> -- 
> 2.7.4

Re: [PATCH 3/4] thunderbolt: Fix adapter init handling during switch add

[Sanjay R Mehta]

On 8/2/2021 8:56 PM, Mika Westerberg wrote:
> [CAUTION: External Email]
> 
> On Mon, Aug 02, 2021 at 07:58:19AM -0500, Sanjay R Mehta wrote:
>> From: Sanjay R Mehta <sanju.mehta@amd.com>
>>
>> Adapter0 (Port0) is the control adapter on the AMD USB4 host router.
>> As per USB4 spec in "Section 1.8", Control Adapters do not
>> have an Adapter Configuration Space".
>>
>> The read requests on Adapter0 time's out and driver initialization fails.
>>
>> Hence Disabling the Adapter in case of read-request timeout and continuing
>> the driver init.
>>
>> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>> Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
>> ---
>>  drivers/thunderbolt/switch.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
>> index 83b1ef3..effbfe4 100644
>> --- a/drivers/thunderbolt/switch.c
>> +++ b/drivers/thunderbolt/switch.c
>> @@ -2747,8 +2747,9 @@ int tb_switch_add(struct tb_switch *sw)
>>                       }
>>                       ret = tb_init_port(&sw->ports[i]);
>>                       if (ret) {
>> +                             sw->ports[i].disabled = true;
>>                               dev_err(&sw->dev, "failed to initialize port %d\n", i);
>> -                             return ret;
>> +                             continue;
> 
> Instead of this, would it work if we start the loop at 1? In case of the
> control adapter (0) tb_port_init() does not do anything useful anyway
> and it actually would simplify that function too if we get rid of the
> special casing.
> 
Hi Mika,

If we start loop from 1, it will work for host router
but this will skip port (0) on device router which may be valid port.

Yes, as you said, for host router adapter (0) we can skip tb_port_init().

Will send the changes accordingly.

Thanks,
Sanjay

>>                       }
>>               }
>>
>> --
>> 2.7.4

Re: [PATCH 3/4] thunderbolt: Fix adapter init handling during switch add

[Mika Westerberg]

Hi,

On Tue, Aug 03, 2021 at 12:23:44AM +0530, Sanjay R Mehta wrote:
> 
> 
> On 8/2/2021 8:56 PM, Mika Westerberg wrote:
> > [CAUTION: External Email]
> > 
> > On Mon, Aug 02, 2021 at 07:58:19AM -0500, Sanjay R Mehta wrote:
> >> From: Sanjay R Mehta <sanju.mehta@amd.com>
> >>
> >> Adapter0 (Port0) is the control adapter on the AMD USB4 host router.
> >> As per USB4 spec in "Section 1.8", Control Adapters do not
> >> have an Adapter Configuration Space".
> >>
> >> The read requests on Adapter0 time's out and driver initialization fails.
> >>
> >> Hence Disabling the Adapter in case of read-request timeout and continuing
> >> the driver init.
> >>
> >> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> >> Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
> >> ---
> >>  drivers/thunderbolt/switch.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> >> index 83b1ef3..effbfe4 100644
> >> --- a/drivers/thunderbolt/switch.c
> >> +++ b/drivers/thunderbolt/switch.c
> >> @@ -2747,8 +2747,9 @@ int tb_switch_add(struct tb_switch *sw)
> >>                       }
> >>                       ret = tb_init_port(&sw->ports[i]);
> >>                       if (ret) {
> >> +                             sw->ports[i].disabled = true;
> >>                               dev_err(&sw->dev, "failed to initialize port %d\n", i);
> >> -                             return ret;
> >> +                             continue;
> > 
> > Instead of this, would it work if we start the loop at 1? In case of the
> > control adapter (0) tb_port_init() does not do anything useful anyway
> > and it actually would simplify that function too if we get rid of the
> > special casing.
> > 
> Hi Mika,
> 
> If we start loop from 1, it will work for host router
> but this will skip port (0) on device router which may be valid port.

For device router adapter 0 is also contror adapter so I think we can
just skip it here unconditionally.

Re: [PATCH 3/4] thunderbolt: Fix adapter init handling during switch add

[Sanjay R Mehta]

On 8/3/2021 2:35 PM, Mika Westerberg wrote:
> [CAUTION: External Email]
> 
> Hi,
> 
> On Tue, Aug 03, 2021 at 12:23:44AM +0530, Sanjay R Mehta wrote:
>>
>>
>> On 8/2/2021 8:56 PM, Mika Westerberg wrote:
>>> [CAUTION: External Email]
>>>
>>> On Mon, Aug 02, 2021 at 07:58:19AM -0500, Sanjay R Mehta wrote:
>>>> From: Sanjay R Mehta <sanju.mehta@amd.com>
>>>>
>>>> Adapter0 (Port0) is the control adapter on the AMD USB4 host router.
>>>> As per USB4 spec in "Section 1.8", Control Adapters do not
>>>> have an Adapter Configuration Space".
>>>>
>>>> The read requests on Adapter0 time's out and driver initialization fails.
>>>>
>>>> Hence Disabling the Adapter in case of read-request timeout and continuing
>>>> the driver init.
>>>>
>>>> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>>>> Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
>>>> ---
>>>>  drivers/thunderbolt/switch.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
>>>> index 83b1ef3..effbfe4 100644
>>>> --- a/drivers/thunderbolt/switch.c
>>>> +++ b/drivers/thunderbolt/switch.c
>>>> @@ -2747,8 +2747,9 @@ int tb_switch_add(struct tb_switch *sw)
>>>>                       }
>>>>                       ret = tb_init_port(&sw->ports[i]);
>>>>                       if (ret) {
>>>> +                             sw->ports[i].disabled = true;
>>>>                               dev_err(&sw->dev, "failed to initialize port %d\n", i);
>>>> -                             return ret;
>>>> +                             continue;
>>>
>>> Instead of this, would it work if we start the loop at 1? In case of the
>>> control adapter (0) tb_port_init() does not do anything useful anyway
>>> and it actually would simplify that function too if we get rid of the
>>> special casing.
>>>
>> Hi Mika,
>>
>> If we start loop from 1, it will work for host router
>> but this will skip port (0) on device router which may be valid port.
> 
> For device router adapter 0 is also contror adapter so I think we can
> just skip it here unconditionally.

Sure Mika. Will send the updated changes.

Thanks,
Sanjay

> 

[PATCH 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 effbfe4..8b2fe67 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

Re: [PATCH 0/4] Add support for AMD USB4 and bug fixes

[Mika Westerberg]

Hi Sanjay,

On Mon, Aug 02, 2021 at 07:58:16AM -0500, Sanjay R Mehta wrote:
> From: Sanjay R Mehta <sanju.mehta@amd.com>
> 
> This series adds support for AMD USB4 host router and
> some general USB4 bug fixes.

Nice to see AMD support being added! :) I have few comments on the
series. I'll comment on the separate patches.

In general looks already good.

> 
> Sanjay R Mehta (4):
>   PCI: Add AMD USB4 host router device IDs
>   thunderbolt: Handle INTR when Disable ISR auto clear bit set
>   thunderbolt: Fix adapter init handling during switch add
>   thunderbolt: Fix port linking by checking all adapters
> 
>  drivers/thunderbolt/nhi.c    | 30 +++++++++++++++++++++++++++++-
>  drivers/thunderbolt/switch.c |  5 +++--
>  include/linux/pci_ids.h      |  2 ++
>  include/linux/thunderbolt.h  |  1 +
>  4 files changed, 35 insertions(+), 3 deletions(-)
> 
> -- 
> 2.7.4