diff mbox

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

Message ID 1371688820-4585-2-git-send-email-ddaney.cavm@gmail.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

David Daney June 20, 2013, 12:40 a.m. UTC 
From: David Daney <david.daney@cavium.com>

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

Signed-off-by: David Daney <david.daney@cavium.com>
---
 drivers/net/ethernet/octeon/octeon_mgmt.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Joe Perches June 20, 2013, 1:08 a.m. UTC | #1 
On Wed, 2013-06-19 at 17:40 -0700, David Daney wrote:
> From: David Daney <david.daney@cavium.com>
> 
> The previous fix was still too agressive to meet ieee specs.  Increase
> to (14, 10).
[]
> diff --git a/drivers/net/ethernet/octeon/octeon_mgmt.c b/drivers/net/ethernet/octeon/octeon_mgmt.c
[]
> @@ -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.

>  		 */
> -		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?

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Daney June 20, 2013, 1:28 a.m. UTC | #2 
On 06/19/2013 06:08 PM, Joe Perches wrote:
> On Wed, 2013-06-19 at 17:40 -0700, David Daney wrote:
>> From: David Daney <david.daney@cavium.com>
>>
>> The previous fix was still too agressive to meet ieee specs.  Increase
>> to (14, 10).
> []
>> diff --git a/drivers/net/ethernet/octeon/octeon_mgmt.c b/drivers/net/ethernet/octeon/octeon_mgmt.c
> []
>> @@ -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 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.


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

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.

>>   		 */
>> -		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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches June 20, 2013, 1:37 a.m. UTC | #3 
On Wed, 2013-06-19 at 18:28 -0700, David Daney wrote:
> On 06/19/2013 06:08 PM, Joe Perches wrote:
> > On Wed, 2013-06-19 at 17:40 -0700, David Daney wrote:
> >> From: David Daney <david.daney@cavium.com>
> >>
> >> The previous fix was still too agressive to meet ieee specs.  Increase
> >> to (14, 10).
> > []
> >> diff --git a/drivers/net/ethernet/octeon/octeon_mgmt.c b/drivers/net/ethernet/octeon/octeon_mgmt.c
> > []
> >> @@ -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 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?


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Daney June 20, 2013, 1:47 a.m. UTC | #4 
On 06/19/2013 06:37 PM, Joe Perches wrote:
> On Wed, 2013-06-19 at 18:28 -0700, David Daney wrote:
>> On 06/19/2013 06:08 PM, Joe Perches wrote:
>>> On Wed, 2013-06-19 at 17:40 -0700, David Daney wrote:
>>>> From: David Daney <david.daney@cavium.com>
>>>>
>>>> The previous fix was still too agressive to meet ieee specs.  Increase
>>>> to (14, 10).
>>> []
>>>> diff --git a/drivers/net/ethernet/octeon/octeon_mgmt.c b/drivers/net/ethernet/octeon/octeon_mgmt.c
>>> []
>>>> @@ -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 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

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

Patch

diff --git a/drivers/net/ethernet/octeon/octeon_mgmt.c b/drivers/net/ethernet/octeon/octeon_mgmt.c
index e6e0292..1ef4148 100644
--- a/drivers/net/ethernet/octeon/octeon_mgmt.c
+++ b/drivers/net/ethernet/octeon/octeon_mgmt.c
@@ -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);