Skip to contentSkip to conversation

netdev: octeon_mgmt minor fixes.

PATCH Applied David Daney 6 participants 10 messages Jun 20, 2013 – Jun 27, 2013

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

cover netdev: octeon_mgmt minor fixes.

From: David Daney <[email protected]>

I have two small change here for 3.11 (I hope).

David Daney (2):
netdev: octeon_mgmt: Correct tx IFG workaround.
netdev: octeon_mgmt: Fix structure layout for little-endian.

 drivers/net/ethernet/octeon/octeon_mgmt.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)
Signature · 1 line
1.7.11.7
#1 lore raw

PATCH 1/2 netdev: octeon_mgmt: Correct tx IFG workaround.

From: David Daney <[email protected]>

The previous fix was still too agressive to meet ieee specs. Increase to (14, 10).

---
 drivers/net/ethernet/octeon/octeon_mgmt.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/net/ethernet/octeon/octeon_mgmt.c +6 -3
index e6e0292..1ef4148 100644
@@ -1141,10 +1141,13 @@ static int octeon_mgmt_open(struct net_device *netdev)
 		/* For compensation state to lock. */
 		ndelay(1040 * NS_PER_PHY_CLK);
 
-		/* Some Ethernet switches cannot handle standard
-		 * Interframe Gap, increase to 16 bytes.
+		/* Default Interframe Gaps are too small.  Recommended
+		 * workaround is.
+		 *
+		 * AGL_GMX_TX_IFG[IFG1]=14
+		 * AGL_GMX_TX_IFG[IFG2]=10
 		 */
-		cvmx_write_csr(CVMX_AGL_GMX_TX_IFG, 0x88);
+		cvmx_write_csr(CVMX_AGL_GMX_TX_IFG, 0xae);
 	}
 
 	octeon_mgmt_rx_fill_ring(netdev);
Signature · 1 line
1.7.11.7
#2 lore raw

Re: [PATCH 1/2] netdev: octeon_mgmt: Correct tx IFG workaround.

Quoted 4 lines · David Daney

From: David Daney <[email protected]>

The previous fix was still too agressive to meet ieee specs. Increase to (14, 10).

[]

drivers/net/ethernet/octeon/octeon_mgmt.c

Quoted 1 line
drivers/net/ethernet/octeon/octeon_mgmt.c +0 -0

[]

drivers/net/ethernet/octeon/octeon_mgmt.c static int octeon_mgmt_open(struct net_device *netdev) · line 1148

Quoted 11 lines
drivers/net/ethernet/octeon/octeon_mgmt.c +5 -2
@@ -1141,10 +1141,13 @@ static int octeon_mgmt_open(struct net_device *netdev)
 		/* For compensation state to lock. */
 		ndelay(1040 * NS_PER_PHY_CLK);
 
-		/* Some Ethernet switches cannot handle standard
-		 * Interframe Gap, increase to 16 bytes.
+		/* Default Interframe Gaps are too small.  Recommended
+		 * workaround is.
+		 *
+		 * AGL_GMX_TX_IFG[IFG1]=14
+		 * AGL_GMX_TX_IFG[IFG2]=10

Why isn't the TX IFG just 96 bit times?

I'm also confused a bit here by the difference between the bsd implementation and yours.

http://fxr.watson.org/fxr/source/contrib/octeon-sdk/cvmx-csr-typedefs.h?v=FREEBSD8

 2628  * * Programming IFG1 and IFG2.
 2629  * 
 2630  *   For half-duplex systems that require IEEE 802.3 compatibility, IFG1 must
 2631  *   be in the range of 1-8, IFG2 must be in the range of 4-12, and the
 2632  *   IFG1+IFG2 sum must be 12.
 2633  * 
 2634  *   For full-duplex systems that require IEEE 802.3 compatibility, IFG1 must
 2635  *   be in the range of 1-11, IFG2 must be in the range of 1-11, and the
 2636  *   IFG1+IFG2 sum must be 12.
 2637  * 
 2638  *   For all other systems, IFG1 and IFG2 can be any value in the range of
 2639  *   1-15.  Allowing for a total possible IFG sum of 2-30.
 2640  * 
 2641  * Additionally reset when both MIX0/1_CTL[RESET] are set to 1.

drivers/net/ethernet/octeon/octeon_mgmt.c static int octeon_mgmt_open(struct net_device *netdev) · line 1153

Quoted 6 lines
drivers/net/ethernet/octeon/octeon_mgmt.c +1 -1
 		 */
-		cvmx_write_csr(CVMX_AGL_GMX_TX_IFG, 0x88);
+		cvmx_write_csr(CVMX_AGL_GMX_TX_IFG, 0xae);
 	}
 
 	octeon_mgmt_rx_fill_ring(netdev);

I don't have a datasheet. Is one available?

#3 lore raw

Re: [PATCH 1/2] netdev: octeon_mgmt: Correct tx IFG workaround.

Quoted 12 lines · Joe Perches
Quoted 4 lines · David Daney

From: David Daney <[email protected]>

The previous fix was still too agressive to meet ieee specs. Increase to (14, 10).

[]

drivers/net/ethernet/octeon/octeon_mgmt.c

Quoted 1 line
drivers/net/ethernet/octeon/octeon_mgmt.c +0 -0

[]

Quoted 4 lines
drivers/net/ethernet/octeon/octeon_mgmt.c +0 -0
@@ -1141,10 +1141,13 @@ static int octeon_mgmt_open(struct net_device *netdev)
  		/* For compensation state to lock. */
  		ndelay(1040 * NS_PER_PHY_CLK);
 
Quoted 9 lines

drivers/net/ethernet/octeon/octeon_mgmt.c static int octeon_mgmt_open(struct net_device *netdev) · line 1148

Quoted 7 lines
drivers/net/ethernet/octeon/octeon_mgmt.c +5 -2
-		/* Some Ethernet switches cannot handle standard
-		 * Interframe Gap, increase to 16 bytes.
+		/* Default Interframe Gaps are too small.  Recommended
+		 * workaround is.
+		 *
+		 * AGL_GMX_TX_IFG[IFG1]=14
+		 * AGL_GMX_TX_IFG[IFG2]=10

Why isn't the TX IFG just 96 bit times?

I don't have a full understanding of how the transistors are wired up on the chip, so I cannot accurately answer your question. But I can say that after I empirically found the previous values to get the thing to work, the hardware designers independently found that the values supplied in this patch are required to achieve industry standard IFGs with this hardware.

Quoted 6 lines

I'm also confused a bit here by the difference between the bsd implementation and yours.

http://fxr.watson.org/fxr/source/contrib/octeon-sdk/cvmx-csr-typedefs.h?v=FREEBSD8

Quoted 15 lines
  2628  * * Programming IFG1 and IFG2.
  2629  *
  2630  *   For half-duplex systems that require IEEE 802.3 compatibility, IFG1 must
  2631  *   be in the range of 1-8, IFG2 must be in the range of 4-12, and the
  2632  *   IFG1+IFG2 sum must be 12.
  2633  *
  2634  *   For full-duplex systems that require IEEE 802.3 compatibility, IFG1 must
  2635  *   be in the range of 1-11, IFG2 must be in the range of 1-11, and the
  2636  *   IFG1+IFG2 sum must be 12.
  2637  *
  2638  *   For all other systems, IFG1 and IFG2 can be any value in the range of
  2639  *   1-15.  Allowing for a total possible IFG sum of 2-30.
  2640  *
  2641  * Additionally reset when both MIX0/1_CTL[RESET] are set to 1.

The advice in that particular comment in the BSD source code has been found to be incorrect, that is why we are overriding the default value of this register in the first place.

Quoted 9 lines

drivers/net/ethernet/octeon/octeon_mgmt.c static int octeon_mgmt_open(struct net_device *netdev) · line 1153

Quoted 6 lines
drivers/net/ethernet/octeon/octeon_mgmt.c +1 -1
  		 */
-		cvmx_write_csr(CVMX_AGL_GMX_TX_IFG, 0x88);
+		cvmx_write_csr(CVMX_AGL_GMX_TX_IFG, 0xae);
  	}
 
  	octeon_mgmt_rx_fill_ring(netdev);

I don't have a datasheet. Is one available?

I don't believe the datasheets are publicly available, but they do exist. If you feel you have a compelling reason to have one, and don't mind jumping through hoops, you could contact me privately.

David Daney

#4 lore raw

Re: [PATCH 1/2] netdev: octeon_mgmt: Correct tx IFG workaround.

Quoted 23 lines · David Daney
Quoted 12 lines · Joe Perches
Quoted 4 lines · David Daney

From: David Daney <[email protected]>

The previous fix was still too agressive to meet ieee specs. Increase to (14, 10).

[]

drivers/net/ethernet/octeon/octeon_mgmt.c

Quoted 1 line
drivers/net/ethernet/octeon/octeon_mgmt.c +0 -0

[]

Quoted 4 lines
drivers/net/ethernet/octeon/octeon_mgmt.c +0 -0
@@ -1141,10 +1141,13 @@ static int octeon_mgmt_open(struct net_device *netdev)
  		/* For compensation state to lock. */
  		ndelay(1040 * NS_PER_PHY_CLK);
 
Quoted 9 lines

drivers/net/ethernet/octeon/octeon_mgmt.c static int octeon_mgmt_open(struct net_device *netdev) · line 1148

Quoted 7 lines
drivers/net/ethernet/octeon/octeon_mgmt.c +5 -2
-		/* Some Ethernet switches cannot handle standard
-		 * Interframe Gap, increase to 16 bytes.
+		/* Default Interframe Gaps are too small.  Recommended
+		 * workaround is.
+		 *
+		 * AGL_GMX_TX_IFG[IFG1]=14
+		 * AGL_GMX_TX_IFG[IFG2]=10

Why isn't the TX IFG just 96 bit times?

Quoted 6 lines

I don't have a full understanding of how the transistors are wired up on the chip, so I cannot accurately answer your question. But I can say that after I empirically found the previous values to get the thing to work, the hardware designers independently found that the values supplied in this patch are required to achieve industry standard IFGs with this hardware.

For one specific chip or for the Octeon entire family?

#5 lore raw

Re: [PATCH 1/2] netdev: octeon_mgmt: Correct tx IFG workaround.

Quoted 24 lines · Joe Perches
Quoted 23 lines · David Daney
Quoted 21 lines · Joe Perches
Quoted 4 lines · David Daney

From: David Daney <[email protected]>

The previous fix was still too agressive to meet ieee specs. Increase to (14, 10).

[]

drivers/net/ethernet/octeon/octeon_mgmt.c

Quoted 1 line
drivers/net/ethernet/octeon/octeon_mgmt.c +0 -0

[]

drivers/net/ethernet/octeon/octeon_mgmt.c static int octeon_mgmt_open(struct net_device *netdev) · line 1148

Quoted 11 lines
drivers/net/ethernet/octeon/octeon_mgmt.c +5 -2
@@ -1141,10 +1141,13 @@ static int octeon_mgmt_open(struct net_device *netdev)
   		/* For compensation state to lock. */
   		ndelay(1040 * NS_PER_PHY_CLK);
 
-		/* Some Ethernet switches cannot handle standard
-		 * Interframe Gap, increase to 16 bytes.
+		/* Default Interframe Gaps are too small.  Recommended
+		 * workaround is.
+		 *
+		 * AGL_GMX_TX_IFG[IFG1]=14
+		 * AGL_GMX_TX_IFG[IFG2]=10

Why isn't the TX IFG just 96 bit times?

Quoted 9 lines
Quoted 6 lines

I don't have a full understanding of how the transistors are wired up on the chip, so I cannot accurately answer your question. But I can say that after I empirically found the previous values to get the thing to work, the hardware designers independently found that the values supplied in this patch are required to achieve industry standard IFGs with this hardware.

For one specific chip or for the Octeon entire family?

You will notice, if you look at the code, that there is an if statement that controls which chips get the special IFG treatment.

But to summarize: Only chips that have 1Gig MII ports are affected. Older versions (that only support 10M and 100M) do not get the adjustment.

David Daney

#6 lore raw

PATCH 2/2 netdev: octeon_mgmt: Fix structure layout for little-endian.

In reply to David Daney

From: David Daney <[email protected]>

The C ABI reverses the bitfield fill order when compiled as little-endian.

---
 drivers/net/ethernet/octeon/octeon_mgmt.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)
drivers/net/ethernet/octeon/octeon_mgmt.c +15 -7
index 1ef4148..622aa75 100644
@@ -46,17 +46,25 @@
 union mgmt_port_ring_entry {
 	u64 d64;
 	struct {
-		u64    reserved_62_63:2;
+#define RING_ENTRY_CODE_DONE 0xf
+#define RING_ENTRY_CODE_MORE 0x10
+#ifdef __BIG_ENDIAN_BITFIELD
+		u64 reserved_62_63:2;
 		/* Length of the buffer/packet in bytes */
-		u64    len:14;
+		u64 len:14;
 		/* For TX, signals that the packet should be timestamped */
-		u64    tstamp:1;
+		u64 tstamp:1;
 		/* The RX error code */
-		u64    code:7;
-#define RING_ENTRY_CODE_DONE 0xf
-#define RING_ENTRY_CODE_MORE 0x10
+		u64 code:7;
 		/* Physical address of the buffer */
-		u64    addr:40;
+		u64 addr:40;
+#else
+		u64 addr:40;
+		u64 code:7;
+		u64 tstamp:1;
+		u64 len:14;
+		u64 reserved_62_63:2;
+#endif
 	} s;
 };
 
Signature · 1 line
1.7.11.7
#7 lore raw

RE: [PATCH 2/2] netdev: octeon_mgmt: Fix structure layout for little-endian.

Quoted 2 lines

The C ABI reverses the bitfield fill order when compiled as little-endian.

No - it is completely implementation defined. The general concensus is not to use bitfields if you care at all about the bit assignments.

David

#8 lore raw

Re: [PATCH 2/2] netdev: octeon_mgmt: Fix structure layout for little-endian.

Quoted 6 lines · David Laight
Quoted 2 lines

The C ABI reverses the bitfield fill order when compiled as little-endian.

No - it is completely implementation defined. The general concensus is not to use bitfields if you care at all about the bit assignments.

FWIW, bitfields often alow things to be expressed more nicely. Just the endian-dependent definition suck, so I came up with this little hack for arch/mips/include/uapi/asm/inst.h:

#ifdef __MIPSEB__

#define BITFIELD_FIELD(field, more)                                     \
        field;                                                          \
        more

#elif defined(__MIPSEL__)

#define BITFIELD_FIELD(field, more)                                     \
        more                                                            \
        field;

#endif

struct i_format { /* signed immediate format */
BITFIELD_FIELD(unsigned int opcode : 6, BITFIELD_FIELD(unsigned int rs : 5, BITFIELD_FIELD(unsigned int rt : 5, BITFIELD_FIELD(signed int simmediate : 16, ;))))
};

Ralf

maintainer Applied
#9 lore raw
In reply to David Daney

From: David Daney <[email protected]>
Date: Wed, 19 Jun 2013 17:40:18 -0700

Quoted 3 lines

David Daney (2):
netdev: octeon_mgmt: Correct tx IFG workaround.
netdev: octeon_mgmt: Fix structure layout for little-endian.

Applied to net, thanks.