linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] Added some bug fixes for USB4
@ 2021-08-03 12:34 Sanjay R Mehta
  2021-08-03 12:34 ` [PATCH v2 1/4] thunderbolt: Intel controller uses BIT(2) for intr auto Sanjay R Mehta
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Sanjay R Mehta @ 2021-08-03 12:34 UTC (permalink / raw)
  To: mika.westerberg, andreas.noever, michael.jamet, YehezkelShB
  Cc: Basavaraj.Natikar, linux-usb, 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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 1/4] thunderbolt: Intel controller uses BIT(2) for intr auto
  2021-08-03 12:34 [PATCH v2 0/4] Added some bug fixes for USB4 Sanjay R Mehta
@ 2021-08-03 12:34 ` Sanjay R Mehta
  2021-08-03 12:34 ` [PATCH v2 2/4] thunderbolt: Handle ring interrupt by reading intr status Sanjay R Mehta
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Sanjay R Mehta @ 2021-08-03 12:34 UTC (permalink / raw)
  To: mika.westerberg, andreas.noever, michael.jamet, YehezkelShB
  Cc: Basavaraj.Natikar, linux-usb, 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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 2/4] thunderbolt: Handle ring interrupt by reading intr status
  2021-08-03 12:34 [PATCH v2 0/4] Added some bug fixes for USB4 Sanjay R Mehta
  2021-08-03 12:34 ` [PATCH v2 1/4] thunderbolt: Intel controller uses BIT(2) for intr auto Sanjay R Mehta
@ 2021-08-03 12:34 ` Sanjay R Mehta
  2021-08-04 15:48   ` Mika Westerberg
  2021-08-03 12:34 ` [PATCH v2 3/4] thunderbolt: Skip port init for control adapter(0) Sanjay R Mehta
  2021-08-03 12:34 ` [PATCH v2 4/4] thunderbolt: Fix port linking by checking all adapters Sanjay R Mehta
  3 siblings, 1 reply; 13+ messages in thread
From: Sanjay R Mehta @ 2021-08-03 12:34 UTC (permalink / raw)
  To: mika.westerberg, andreas.noever, michael.jamet, YehezkelShB
  Cc: Basavaraj.Natikar, linux-usb, 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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 3/4] thunderbolt: Skip port init for control adapter(0)
  2021-08-03 12:34 [PATCH v2 0/4] Added some bug fixes for USB4 Sanjay R Mehta
  2021-08-03 12:34 ` [PATCH v2 1/4] thunderbolt: Intel controller uses BIT(2) for intr auto Sanjay R Mehta
  2021-08-03 12:34 ` [PATCH v2 2/4] thunderbolt: Handle ring interrupt by reading intr status Sanjay R Mehta
@ 2021-08-03 12:34 ` Sanjay R Mehta
  2021-08-03 12:34 ` [PATCH v2 4/4] thunderbolt: Fix port linking by checking all adapters Sanjay R Mehta
  3 siblings, 0 replies; 13+ messages in thread
From: Sanjay R Mehta @ 2021-08-03 12:34 UTC (permalink / raw)
  To: mika.westerberg, andreas.noever, michael.jamet, YehezkelShB
  Cc: Basavaraj.Natikar, linux-usb, 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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 4/4] thunderbolt: Fix port linking by checking all adapters
  2021-08-03 12:34 [PATCH v2 0/4] Added some bug fixes for USB4 Sanjay R Mehta
                   ` (2 preceding siblings ...)
  2021-08-03 12:34 ` [PATCH v2 3/4] thunderbolt: Skip port init for control adapter(0) Sanjay R Mehta
@ 2021-08-03 12:34 ` Sanjay R Mehta
  3 siblings, 0 replies; 13+ messages in thread
From: Sanjay R Mehta @ 2021-08-03 12:34 UTC (permalink / raw)
  To: mika.westerberg, andreas.noever, michael.jamet, YehezkelShB
  Cc: Basavaraj.Natikar, linux-usb, 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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] thunderbolt: Handle ring interrupt by reading intr status
  2021-08-03 12:34 ` [PATCH v2 2/4] thunderbolt: Handle ring interrupt by reading intr status Sanjay R Mehta
@ 2021-08-04 15:48   ` Mika Westerberg
  2021-08-05 12:56     ` Mika Westerberg
  2021-08-05 12:59     ` Sanjay R Mehta
  0 siblings, 2 replies; 13+ messages in thread
From: Mika Westerberg @ 2021-08-04 15:48 UTC (permalink / raw)
  To: Sanjay R Mehta
  Cc: andreas.noever, michael.jamet, YehezkelShB, Basavaraj.Natikar, linux-usb

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 ;-)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] thunderbolt: Handle ring interrupt by reading intr status
  2021-08-04 15:48   ` Mika Westerberg
@ 2021-08-05 12:56     ` Mika Westerberg
  2021-08-05 13:06       ` Sanjay R Mehta
  2021-08-05 12:59     ` Sanjay R Mehta
  1 sibling, 1 reply; 13+ messages in thread
From: Mika Westerberg @ 2021-08-05 12:56 UTC (permalink / raw)
  To: Sanjay R Mehta
  Cc: andreas.noever, michael.jamet, YehezkelShB, Basavaraj.Natikar, linux-usb

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;

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] thunderbolt: Handle ring interrupt by reading intr status
  2021-08-04 15:48   ` Mika Westerberg
  2021-08-05 12:56     ` Mika Westerberg
@ 2021-08-05 12:59     ` Sanjay R Mehta
  2021-08-05 14:19       ` Mika Westerberg
  1 sibling, 1 reply; 13+ messages in thread
From: Sanjay R Mehta @ 2021-08-05 12:59 UTC (permalink / raw)
  To: Mika Westerberg, Sanjay R Mehta
  Cc: andreas.noever, michael.jamet, YehezkelShB, Basavaraj.Natikar, linux-usb



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.

> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] thunderbolt: Handle ring interrupt by reading intr status
  2021-08-05 12:56     ` Mika Westerberg
@ 2021-08-05 13:06       ` Sanjay R Mehta
  2021-08-05 14:20         ` Mika Westerberg
  0 siblings, 1 reply; 13+ messages in thread
From: Sanjay R Mehta @ 2021-08-05 13:06 UTC (permalink / raw)
  To: Mika Westerberg, Sanjay R Mehta
  Cc: andreas.noever, michael.jamet, YehezkelShB, Basavaraj.Natikar, linux-usb



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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] thunderbolt: Handle ring interrupt by reading intr status
  2021-08-05 12:59     ` Sanjay R Mehta
@ 2021-08-05 14:19       ` Mika Westerberg
  2021-08-05 14:46         ` Mika Westerberg
  0 siblings, 1 reply; 13+ messages in thread
From: Mika Westerberg @ 2021-08-05 14:19 UTC (permalink / raw)
  To: Sanjay R Mehta
  Cc: Sanjay R Mehta, andreas.noever, michael.jamet, YehezkelShB,
	Basavaraj.Natikar, linux-usb

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] thunderbolt: Handle ring interrupt by reading intr status
  2021-08-05 13:06       ` Sanjay R Mehta
@ 2021-08-05 14:20         ` Mika Westerberg
  0 siblings, 0 replies; 13+ messages in thread
From: Mika Westerberg @ 2021-08-05 14:20 UTC (permalink / raw)
  To: Sanjay R Mehta
  Cc: Sanjay R Mehta, andreas.noever, michael.jamet, YehezkelShB,
	Basavaraj.Natikar, linux-usb

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] thunderbolt: Handle ring interrupt by reading intr status
  2021-08-05 14:19       ` Mika Westerberg
@ 2021-08-05 14:46         ` Mika Westerberg
  2021-08-05 18:03           ` Sanjay R Mehta
  0 siblings, 1 reply; 13+ messages in thread
From: Mika Westerberg @ 2021-08-05 14:46 UTC (permalink / raw)
  To: Sanjay R Mehta
  Cc: Sanjay R Mehta, andreas.noever, michael.jamet, YehezkelShB,
	Basavaraj.Natikar, linux-usb

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?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 2/4] thunderbolt: Handle ring interrupt by reading intr status
  2021-08-05 14:46         ` Mika Westerberg
@ 2021-08-05 18:03           ` Sanjay R Mehta
  0 siblings, 0 replies; 13+ messages in thread
From: Sanjay R Mehta @ 2021-08-05 18:03 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Sanjay R Mehta, andreas.noever, michael.jamet, YehezkelShB,
	Basavaraj.Natikar, linux-usb, Shah, Nehal-bakulchandra,
	Shyam Sundar S K



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 :)
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-08-05 18:03 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-03 12:34 [PATCH v2 0/4] Added some bug fixes for USB4 Sanjay R Mehta
2021-08-03 12:34 ` [PATCH v2 1/4] thunderbolt: Intel controller uses BIT(2) for intr auto Sanjay R Mehta
2021-08-03 12:34 ` [PATCH v2 2/4] thunderbolt: Handle ring interrupt by reading intr status Sanjay R Mehta
2021-08-04 15:48   ` Mika Westerberg
2021-08-05 12:56     ` Mika Westerberg
2021-08-05 13:06       ` Sanjay R Mehta
2021-08-05 14:20         ` Mika Westerberg
2021-08-05 12:59     ` Sanjay R Mehta
2021-08-05 14:19       ` Mika Westerberg
2021-08-05 14:46         ` Mika Westerberg
2021-08-05 18:03           ` Sanjay R Mehta
2021-08-03 12:34 ` [PATCH v2 3/4] thunderbolt: Skip port init for control adapter(0) Sanjay R Mehta
2021-08-03 12:34 ` [PATCH v2 4/4] thunderbolt: Fix port linking by checking all adapters Sanjay R Mehta

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).