e1000: Work around 82571 completion timout on Pseries HW

PATCH In review Auke Kok 7 participants 13 messages May 16, 2007 – Jun 23, 2007

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Auke Kok

author

2007-05-16 15:50 UTC #0 lore raw

PATCH 1/1 e1000: Work around 82571 completion timout on Pseries HW

Our 82571 (first PCI-E hardware) causes P-Series hardware to throw issues. Disabling PCI-E completion timeouts in our NIC resolves the issue.

Signed-off-by Auke Kok <[email protected]>

Cc Wen Xiong <[email protected]>

---

 drivers/net/e1000/e1000_main.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

drivers/net/e1000/e1000_main.c +10 -0

index 49be393..830d851 100644
@@ -819,6 +819,16 @@ e1000_reset(struct e1000_adapter *adapter)
 		E1000_WRITE_REG(&adapter->hw, CTRL, ctrl);
 	}
 
+#if defined(CONFIG_PPC64) || defined(CONFIG_PPC)
+#define E1000_GCR_DISABLE_TIMEOUT_MECHANISM 0x80000000
+	if (adapter->hw.mac.type == e1000_82571) {
+		/* work around pSeries hardware by disabling timeouts */
+		u32 gcr = E1000_READ_REG(&adapter->hw, E1000_GCR);
+		gcr |= E1000_GCR_DISABLE_TIMEOUT_MECHANISM;
+		E1000_WRITE_REG(&adapter->hw, E1000_GCR, gcr);
+	}
+#endif
+
 	/* Enable h/w to recognize an 802.1Q VLAN Ethernet packet */
 	E1000_WRITE_REG(&adapter->hw, VET, ETHERNET_IEEE_VLAN_TYPE);

Is pSeries PCI-E implementation problematic? Or is this a e1000 PCI-E hardware bug? If the former, I wonder if this is the best way to work around the issue, especially since it might crop up in other PCI-E boards besides e1000. If the latter, it seems like we should disable the feature on all platforms.

Jeff

Wen Xiong

2007-05-16 22:11 UTC #2 lore raw

Hi Jeff,

little history, on PCI-X adapters we did not time dma reads being
returned to the adapter. On PCIe unfortunately the spec says that adapters need to time the dma reads the spec is not specific on the range but the timers can range from worst case 50 usecs to 10 ms with some adapters supporting upto 50ms. The PCIe spec has approved an ECR to the PCIe 1.0a spec that will support the ability to disable the timer. To work aroung the adapter doesn't support ECR. We need the driver to disable the timer.

Thanks,
wendy

Jeff Garzik <[email protected]>
05/16/2007 11:00 AM

To
Auke Kok <[email protected]>
cc
Wen Xiong/Austin/IBM@IBMUS, [email protected], [email protected], Benjamin Herrenschmidt <[email protected]> Subject
Re: [PATCH] e1000: Work around 82571 completion timout on Pseries HW

Quoted 4 lines · Auke Kok

Our 82571 (first PCI-E hardware) causes P-Series hardware to throw issues. Disabling PCI-E completion timeouts in our NIC resolves the issue.

Review

Quoted 8 lines

Signed-off-by Auke Kok <[email protected]>
Cc Wen Xiong <[email protected]>
---
 drivers/net/e1000/e1000_main.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)
(quoted diff) +0 -0

b/drivers/net/e1000/e1000_main.c

drivers/net/e1000/e1000_main.c e1000_reset(struct e1000_adapter *adapter) · line 819

Quoted 5 lines

index 49be393..830d851 100644

drivers/net/e1000/e1000_main.c +0 -0

@@ -819,6 +819,16 @@ e1000_reset(struct e1000_adapter *adapter)
                               E1000_WRITE_REG(&adapter->hw, CTRL,

ctrl);

drivers/net/e1000/e1000_main.c e1000_reset(struct e1000_adapter *adapter)

Quoted 6 lines

drivers/net/e1000/e1000_main.c +4 -0

               }
 
+#if defined(CONFIG_PPC64) || defined(CONFIG_PPC)
+#define E1000_GCR_DISABLE_TIMEOUT_MECHANISM 0x80000000
+              if (adapter->hw.mac.type == e1000_82571) {
+                              /* work around pSeries hardware by

disabling timeouts */

drivers/net/e1000/e1000_main.c e1000_reset(struct e1000_adapter *adapter) · line 826

Quoted 1 line

drivers/net/e1000/e1000_main.c +1 -0
+                              u32 gcr = E1000_READ_REG(&adapter->hw, 

E1000_GCR);

drivers/net/e1000/e1000_main.c e1000_reset(struct e1000_adapter *adapter) · line 827

Quoted 1 line

drivers/net/e1000/e1000_main.c +1 -0
+                              gcr |= 

E1000_GCR_DISABLE_TIMEOUT_MECHANISM;

drivers/net/e1000/e1000_main.c e1000_reset(struct e1000_adapter *adapter) · line 828

Quoted 1 line

drivers/net/e1000/e1000_main.c +1 -0
+                              E1000_WRITE_REG(&adapter->hw, E1000_GCR, 

gcr);

Why is this needed?

Jeff

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel

Benjamin Herrenschmidt

2007-05-16 22:31 UTC #3 lore raw

Quoted 8 lines · Wen Xiong

little history, on PCI-X adapters we did not time dma reads being
returned to the adapter. On PCIe unfortunately the spec says that adapters need to time the dma reads the spec is not specific on the range but the timers can range from worst case 50 usecs to 10 ms with some adapters supporting upto 50ms. The PCIe spec has approved an ECR to the PCIe 1.0a spec that will support the ability to disable the timer. To work aroung the adapter doesn't support ECR. We need the driver to disable the timer.

You do get more than 10ms delay for DMA replies on pSeries ? Ouch...

Ben.

Kumar Gala

2007-05-17 00:45 UTC #4 lore raw

In reply to Auke Kok

Quoted 16 lines · Auke Kok

Our 82571 (first PCI-E hardware) causes P-Series hardware to throw issues. Disabling PCI-E completion timeouts in our NIC resolves the issue.
Signed-off-by Auke Kok <[email protected]>
Cc Wen Xiong <[email protected]>
---
 drivers/net/e1000/e1000_main.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)
drivers/net/e1000/e1000_main.c +0 -0
index 49be393..830d851 100644

drivers/net/e1000/e1000_main.c e1000_reset(struct e1000_adapter *adapter) · line 822

Quoted 4 lines

drivers/net/e1000/e1000_main.c +1 -0

@@ -819,6 +819,16 @@ e1000_reset(struct e1000_adapter *adapter)
 		E1000_WRITE_REG(&adapter->hw, CTRL, ctrl);
 	}
 
+#if defined(CONFIG_PPC64) || defined(CONFIG_PPC)

If this is really on need on pseries HW this should be #ifdef CONFIG_PPC_PSERIES. No reason to force this on all PPC.

Quoted 16 lines

drivers/net/e1000/e1000_main.c +9 -1

+#define E1000_GCR_DISABLE_TIMEOUT_MECHANISM 0x80000000
+	if (adapter->hw.mac.type == e1000_82571) {
+		/* work around pSeries hardware by disabling timeouts */
+		u32 gcr = E1000_READ_REG(&adapter->hw, E1000_GCR);
+		gcr |= E1000_GCR_DISABLE_TIMEOUT_MECHANISM;
+		E1000_WRITE_REG(&adapter->hw, E1000_GCR, gcr);
+	}
+#endif
+
 	/* Enable h/w to recognize an 802.1Q VLAN Ethernet packet */
 	E1000_WRITE_REG(&adapter->hw, VET, ETHERNET_IEEE_VLAN_TYPE);
 
-

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Christoph Hellwig

2007-05-17 09:01 UTC #5 lore raw

Quoted 11 lines · Kumar Gala

drivers/net/e1000/e1000_main.c e1000_reset(struct e1000_adapter *adapter) · line 822
Quoted 8 lines
index 49be393..830d851 100644
drivers/net/e1000/e1000_main.c +1 -0
@@ -819,6 +819,16 @@ e1000_reset(struct e1000_adapter *adapter)
 		E1000_WRITE_REG(&adapter->hw, CTRL, ctrl);
 	}
 
+#if defined(CONFIG_PPC64) || defined(CONFIG_PPC)
If this is really on need on pseries HW this should be #ifdef CONFIG_PPC_PSERIES. No reason to force this on all PPC.

No, it shouldn't. powerpc kernels are multi-platform.

It really shouldn't be there at all because something in either the intel or pseries hardware is totally buggy and we should disable features in the buggy one completely.

Kumar Gala

2007-05-17 13:12 UTC #6 lore raw

Quoted 8 lines · Christoph Hellwig

Quoted 7 lines · Kumar Gala
Quoted 7 lines
index 49be393..830d851 100644
drivers/net/e1000/e1000_main.c +0 -0
@@ -819,6 +819,16 @@ e1000_reset(struct e1000_adapter *adapter)
 		E1000_WRITE_REG(&adapter->hw, CTRL, ctrl);
 	}
 

Quoted 6 lines

Quoted 4 lines
drivers/net/e1000/e1000_main.c e1000_reset(struct e1000_adapter *adapter) · line 822
Quoted 1 line
drivers/net/e1000/e1000_main.c +1 -0
+#if defined(CONFIG_PPC64) || defined(CONFIG_PPC)
If this is really on need on pseries HW this should be #ifdef CONFIG_PPC_PSERIES. No reason to force this on all PPC.
No, it shouldn't. powerpc kernels are multi-platform.

Right, but if you build a multi-platform kernel with pseries support you'll get CONFIG_PPC_PSERIES set.

Quoted 3 lines

It really shouldn't be there at all because something in either the intel or pseries hardware is totally buggy and we should disable features in the buggy one completely.

Agreed.

- k

Wen Xiong

2007-05-17 14:58 UTC #7 lore raw

In reply to Christoph Hellwig

Quoted 1 line

Quoted 1 line
It really shouldn't be there at all because something in either the

intel

Quoted 2 lines

Quoted 2 lines
or pseries hardware is totally buggy and we should disable features in the buggy one completely.

Hi,

Here there are not hardware issue on both Intel or PPC. The patch is to work around a loop hold on early version of PCI SGI spec. The later PCI Sgi have spec have corrected it. We can just implement it for PPC only. Other vendor may have the same issue.

Thanks,
wendy

Christoph Hellwig <[email protected]>
05/17/2007 04:01 AM

To
Kumar Gala <[email protected]>
cc
Auke Kok <[email protected]>, [email protected], Wen Xiong/Austin/IBM@IBMUS, [email protected], [email protected]
Subject
Re: [PATCH] e1000: Work around 82571 completion timout on Pseries HW

Quoted 5 lines · Kumar Gala

drivers/net/e1000/e1000_main.c e1000_reset(struct e1000_adapter *adapter) · line 819
Quoted 5 lines
index 49be393..830d851 100644
drivers/net/e1000/e1000_main.c +0 -0
@@ -819,6 +819,16 @@ e1000_reset(struct e1000_adapter *adapter)
                             E1000_WRITE_REG(&adapter->hw, CTRL, 

ctrl);

Quoted 6 lines

drivers/net/e1000/e1000_main.c e1000_reset(struct e1000_adapter *adapter) · line 822
Quoted 3 lines
drivers/net/e1000/e1000_main.c +1 -0
             }
 
+#if defined(CONFIG_PPC64) || defined(CONFIG_PPC)
If this is really on need on pseries HW this should be #ifdef CONFIG_PPC_PSERIES. No reason to force this on all PPC.

No, it shouldn't. powerpc kernels are multi-platform.

It really shouldn't be there at all because something in either the intel or pseries hardware is totally buggy and we should disable features in the buggy one completely.

_______________________________________________
E1000-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/e1000-devel

Jeff Garzik

2007-05-17 21:05 UTC #8 lore raw

Quoted 1 line · Wen Xiong

Here there are not hardware issue on both Intel or PPC. The patch is to work around a loop hold on early version of PCI SGI spec. The later PCI Sgi have spec have corrected it. We can just implement it for PPC only. Other vendor may have the same issue.

If it is not a PPC-specific issue, then we should not limit it to PPC.

Jeff

wendy xiong

2007-05-17 21:44 UTC #9 lore raw

Quoted 7 lines · Jeff Garzik

Quoted 4 lines · Wen Xiong
Here there are not hardware issue on both Intel or PPC. The patch is to work around a loop hold on early version of PCI SGI spec. The later PCI Sgi have spec have corrected it. We can just implement it for PPC only. Other vendor may have the same issue.

Quoted 5 lines

If it is not a PPC-specific issue, then we should not limit it to PPC.
Jeff

Hi Jeff,

We have tested the patch on PowerPC CEC and IO drawers. We really want to fix this for PPC. If you can accept for vendors, we are ok.

Thanks for your help!
wendy

Christoph Hellwig

2007-06-23 08:20 UTC #10 lore raw

In reply to Wen Xiong

Quoted 11 lines · Wen Xiong

Quoted 1 line
Quoted 1 line
It really shouldn't be there at all because something in either the
intel
Quoted 2 lines
Quoted 2 lines
or pseries hardware is totally buggy and we should disable features in the buggy one completely.
Hi,
Here there are not hardware issue on both Intel or PPC. The patch is to work around a loop hold on early version of PCI SGI spec. The later PCI Sgi have spec have corrected it. We can just implement it for PPC only. Other vendor may have the same issue.

In this case we should add a blacklist for implementations of the old spec. There should be a way to find specific bridges in the OF firmware tree on powerpc and similar things on other platforms aswell.

Kok, Auke

author

2007-06-23 17:05 UTC #11 lore raw

Quoted 7 lines · Christoph Hellwig

Quoted 6 lines · Wen Xiong
Quoted 1 line
Quoted 1 line
It really shouldn't be there at all because something in either the
intel
Quoted 2 lines
Quoted 2 lines
or pseries hardware is totally buggy and we should disable features in the buggy one completely.
Hi,

Quoted 8 lines

Quoted 4 lines
Here there are not hardware issue on both Intel or PPC. The patch is to work around a loop hold on early version of PCI SGI spec. The later PCI Sgi have spec have corrected it. We can just implement it for PPC only. Other vendor may have the same issue.
In this case we should add a blacklist for implementations of the old spec. There should be a way to find specific bridges in the OF firmware tree on powerpc and similar things on other platforms aswell.

Yes, this is almost what we did.

IBM is currently testing my patches that implement a generic pci quirk that will be enabled for only selected root complex ID's that require the (1.0a spec) device to disable the completion timeouts.

They are currently validating this test on the affected hardware. I expect to get the results within a week and then I will post the patch.

Since this is one of the few holes in between the two specs (where manual intervention is needed) I think that a single quirk is a fairly sane approach.

Cheers,

Auke

Jeff Garzik

2007-05-18 00:47 UTC #12 lore raw

In reply to Auke Kok

Quoted 18 lines · Auke Kok

Our 82571 (first PCI-E hardware) causes P-Series hardware to throw issues. Disabling PCI-E completion timeouts in our NIC resolves the issue.
Signed-off-by Auke Kok <[email protected]>
Cc Wen Xiong <[email protected]>
---
 drivers/net/e1000/e1000_main.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)
drivers/net/e1000/e1000_main.c +0 -0
index 49be393..830d851 100644
@@ -819,6 +819,16 @@ e1000_reset(struct e1000_adapter *adapter)
 		E1000_WRITE_REG(&adapter->hw, CTRL, ctrl);
 	}

drivers/net/e1000/e1000_main.c e1000_reset(struct e1000_adapter *adapter) · line 830

Quoted 10 lines

drivers/net/e1000/e1000_main.c +9 -0

 
+#if defined(CONFIG_PPC64) || defined(CONFIG_PPC)
+#define E1000_GCR_DISABLE_TIMEOUT_MECHANISM 0x80000000
+	if (adapter->hw.mac.type == e1000_82571) {
+		/* work around pSeries hardware by disabling timeouts */
+		u32 gcr = E1000_READ_REG(&adapter->hw, E1000_GCR);
+		gcr |= E1000_GCR_DISABLE_TIMEOUT_MECHANISM;
+		E1000_WRITE_REG(&adapter->hw, E1000_GCR, gcr);
+	}
+#endif

not applied. based on the conversation, it sounds like this is not specific to PPC

Jeff