[PATCH] gianfar: Fix half-duplex operation for non-MII/RMII interfaces

[PATCH] gianfar: Fix half-duplex operation for non-MII/RMII interfaces

[Anton Vorontsov]

It appears that gianfar driver has the same problem[1] that I
just fixed for ucc_geth.

NFS boot using 10/half link takes about 10 minutes to complete:

  eth0: Gianfar Ethernet Controller Version 1.2, 00:11:22:33:44:55
  eth0: Running with NAPI enabled
  eth0: 256/256 RX/TX BD ring size
  [...]
  Sending DHCP requests .
  PHY: mdio@e0024520:02 - Link is Up - 10/Half
  ., OK
  [...]
  Looking up port of RPC 100003/2 on 10.0.0.2
  Looking up port of RPC 100005/1 on 10.0.0.2
  VFS: Mounted root (nfs filesystem) readonly on device 0:11.
  Freeing unused kernel memory: 188k init
  nfs: server 10.0.0.2 not responding, still trying
  nfs: server 10.0.0.2 OK
  nfs: server 10.0.0.2 not responding, still trying
  nfs: server 10.0.0.2 not responding, still trying
  nfs: server 10.0.0.2 not responding, still trying
  [... few minutes of OK/not responding flood ...]

And just as with ucc_geth, statistic shows errors:

  # ethtool -S eth0 | grep -v ": 0"
  NIC statistics:
       rx-dropped-by-kernel: 2
       tx-rx-64-frames: 49
       tx-rx-65-127-frames: 1270
       tx-rx-128-255-frames: 9992
       tx-rx-256-511-frames: 75
       tx-rx-512-1023-frames: 142
       tx-rx-1024-1518-frames: 8828
       rx-bytes: 13299414
       rx-packets: 13122
       rx-jabber-frames: 9
       tx-byte-counter: 1284847
       tx-packets: 8115
       tx-broadcast-packets: 3
       tx-deferral-packets: 43
       tx-single-collision-packets: 15
       tx-multiple-collision-packets: 301
       tx-late-collision-packets: 872
       tx-total-collision: 999
       tx-undersize-frames: 6

The symptoms were observed on MPC8379E-RDB boards (eTSEC). Although
I didn't find where documentation forbids clearing Full Duplex bit
for non-MII/RMII modes, it's pretty distinct that the bit should be
set.

It's no wonder though, QE Ethernet and TSEC are pretty similar.

[1] http://lists.ozlabs.org/pipermail/linuxppc-dev/2009-June/073631.html

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/net/gianfar.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 8741bb0..15dbffa 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -1984,13 +1984,16 @@ static void adjust_link(struct net_device *dev)
 	if (phydev->link) {
 		u32 tempval = gfar_read(&regs->maccfg2);
 		u32 ecntrl = gfar_read(&regs->ecntrl);
+		phy_interface_t phyi = phydev->interface;
 
 		/* Now we make sure that we can be in full duplex mode.
 		 * If not, we operate in half-duplex mode. */
 		if (phydev->duplex != priv->oldduplex) {
 			new_state = 1;
-			if (!(phydev->duplex))
-				tempval &= ~(MACCFG2_FULL_DUPLEX);
+			if (!phydev->duplex &&
+					(phyi == PHY_INTERFACE_MODE_MII ||
+					 phyi == PHY_INTERFACE_MODE_RMII))
+				tempval &= ~MACCFG2_FULL_DUPLEX;
 			else
 				tempval |= MACCFG2_FULL_DUPLEX;
 
-- 
1.6.3.1

Re: [PATCH] gianfar: Fix half-duplex operation for non-MII/RMII interfaces

[Andy Fleming]

On Jun 24, 2009, at 1:27 PM, Anton Vorontsov wrote:

> It appears that gianfar driver has the same problem[1] that I
> just fixed for ucc_geth.
>
> NFS boot using 10/half link takes about 10 minutes to complete:
>

>
> The symptoms were observed on MPC8379E-RDB boards (eTSEC). Although
> I didn't find where documentation forbids clearing Full Duplex bit
> for non-MII/RMII modes, it's pretty distinct that the bit should be
> set.
>
> It's no wonder though, QE Ethernet and TSEC are pretty similar.

> -			if (!(phydev->duplex))
> -				tempval &= ~(MACCFG2_FULL_DUPLEX);
> +			if (!phydev->duplex &&
> +					(phyi == PHY_INTERFACE_MODE_MII ||
> +					 phyi == PHY_INTERFACE_MODE_RMII))


Hmm....have you tested this on a GMII interface?  *Technically*, full  
duplex is required for GMII, as GMII is used only for gigabit.   
However, we've been treating the GMII interface type as an indicator  
that the PHY *has* a GMII connection to the NIC.  When gianfar detects  
the speed is 10/100 it switches to the compatible MII interface via  
this code, just below:

                         case 100:
                         case 10:
                                 tempval =
                                     ((tempval & ~(MACCFG2_IF)) |  
MACCFG2_MII);


My concern is that you will be detecting the GMII interface, and  
disallowing half-duplex, despite the fact that the interface is  
actually running at 10 or 100 Mbit.

Andy

Re: [PATCH] gianfar: Fix half-duplex operation for non-MII/RMII interfaces

[Anton Vorontsov]

On Wed, Jun 24, 2009 at 03:18:59PM -0500, Andy Fleming wrote:
>
> On Jun 24, 2009, at 1:27 PM, Anton Vorontsov wrote:
>
>> It appears that gianfar driver has the same problem[1] that I
>> just fixed for ucc_geth.
>>
>> NFS boot using 10/half link takes about 10 minutes to complete:
>>
>
>>
>> The symptoms were observed on MPC8379E-RDB boards (eTSEC). Although
>> I didn't find where documentation forbids clearing Full Duplex bit
>> for non-MII/RMII modes, it's pretty distinct that the bit should be
>> set.
>>
>> It's no wonder though, QE Ethernet and TSEC are pretty similar.
>
>> -			if (!(phydev->duplex))
>> -				tempval &= ~(MACCFG2_FULL_DUPLEX);
>> +			if (!phydev->duplex &&
>> +					(phyi == PHY_INTERFACE_MODE_MII ||
>> +					 phyi == PHY_INTERFACE_MODE_RMII))
>
>
> Hmm....have you tested this on a GMII interface?

Nope, only RGMII.

>  *Technically*, full  
> duplex is required for GMII, as GMII is used only for gigabit.  However, 
> we've been treating the GMII interface type as an indicator that the PHY 
> *has* a GMII connection to the NIC.  When gianfar detects the speed is 
> 10/100 it switches to the compatible MII interface via this code, just 
> below:
>
>                         case 100:
>                         case 10:
>                                 tempval =
>                                     ((tempval & ~(MACCFG2_IF)) |  
> MACCFG2_MII);
>
>
> My concern is that you will be detecting the GMII interface, and  
> disallowing half-duplex, despite the fact that the interface is actually 
> running at 10 or 100 Mbit.

Very interesting, though I'm not sure I'm completely following. :-)

Are you saying that I should do this instead:

	if (!phydev->duplex &&
			(phyi == PHY_INTERFACE_MODE_MII ||
			 phyi == PHY_INTERFACE_MODE_RMII ||
			 (phyi == PHY_INTERFACE_MODE_GMII &&
			  phydev->speed < 1000)))
		tempval &= ~MACCFG2_FULL_DUPLEX;
	else
		tempval |= MACCFG2_FULL_DUPLEX;

i.e. we detected GMII interface initially, but it downgraded
to MII since speed is < 1000, thus we can set half-duplex in MAC?

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] gianfar: Fix half-duplex operation for non-MII/RMII interfaces

[Andy Fleming]

[-- Attachment #1: Type: text/plain, Size: 1358 bytes --]

>
>> *Technically*, full
>> duplex is required for GMII, as GMII is used only for gigabit.   
>> However,
>> we've been treating the GMII interface type as an indicator that  
>> the PHY
>> *has* a GMII connection to the NIC.  When gianfar detects the speed  
>> is
>> 10/100 it switches to the compatible MII interface via this code,  
>> just
>> below:
>>
>>                        case 100:
>>                        case 10:
>>                                tempval =
>>                                    ((tempval & ~(MACCFG2_IF)) |
>> MACCFG2_MII);
>>
>>
>> My concern is that you will be detecting the GMII interface, and
>> disallowing half-duplex, despite the fact that the interface is  
>> actually
>> running at 10 or 100 Mbit.
>
> Very interesting, though I'm not sure I'm completely following. :-)
>
> Are you saying that I should do this instead:
>
> 	if (!phydev->duplex &&
> 			(phyi == PHY_INTERFACE_MODE_MII ||
> 			 phyi == PHY_INTERFACE_MODE_RMII ||
> 			 (phyi == PHY_INTERFACE_MODE_GMII &&
> 			  phydev->speed < 1000)))
> 		tempval &= ~MACCFG2_FULL_DUPLEX;
> 	else
> 		tempval |= MACCFG2_FULL_DUPLEX;
>
> i.e. we detected GMII interface initially, but it downgraded
> to MII since speed is < 1000, thus we can set half-duplex in MAC?

Yeah, I think that works out more correctly.  And I suspect that the  
same is true for UCC

Andy

[-- Attachment #2: Type: text/html, Size: 3986 bytes --]

Re: [PATCH] gianfar: Fix half-duplex operation for non-MII/RMII interfaces

[Anton Vorontsov]

On Wed, Jun 24, 2009 at 04:25:06PM -0500, Andy Fleming wrote:
[...]
>>> My concern is that you will be detecting the GMII interface, and
>>> disallowing half-duplex, despite the fact that the interface is  
>>> actually
>>> running at 10 or 100 Mbit.
>>
>> Very interesting, though I'm not sure I'm completely following. :-)
>>
>> Are you saying that I should do this instead:
>>
>> 	if (!phydev->duplex &&
>> 			(phyi == PHY_INTERFACE_MODE_MII ||
>> 			 phyi == PHY_INTERFACE_MODE_RMII ||
>> 			 (phyi == PHY_INTERFACE_MODE_GMII &&
>> 			  phydev->speed < 1000)))
>> 		tempval &= ~MACCFG2_FULL_DUPLEX;
>> 	else
>> 		tempval |= MACCFG2_FULL_DUPLEX;
>>
>> i.e. we detected GMII interface initially, but it downgraded
>> to MII since speed is < 1000, thus we can set half-duplex in MAC?
>
> Yeah, I think that works out more correctly.

Cool, thanks.

Do you happen to know how gianfar iface auto-detection works in HW?
I mean, if we connect 100 Mbs link to the GMII PHY, then
gfar_get_interface() would return MII, correct?

If so, then I think I'll also have to change
	phy_interface_t phyi = phydev->interface;
to
	phy_interface_t phyi = gfar_get_interface(dev);
since phydev->interface may contain outdated information.

Or this can't happen?

> And I suspect that the  same is true for UCC

Yup. Much thanks for catching this!

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: [PATCH] gianfar: Fix half-duplex operation for non-MII/RMII interfaces

[Anton Vorontsov]

On Thu, Jun 25, 2009 at 01:39:45AM +0400, Anton Vorontsov wrote:
> On Wed, Jun 24, 2009 at 04:25:06PM -0500, Andy Fleming wrote:
> [...]
> >>> My concern is that you will be detecting the GMII interface, and
> >>> disallowing half-duplex, despite the fact that the interface is  
> >>> actually
> >>> running at 10 or 100 Mbit.
> >>
> >> Very interesting, though I'm not sure I'm completely following. :-)
> >>
> >> Are you saying that I should do this instead:
> >>
> >> 	if (!phydev->duplex &&
> >> 			(phyi == PHY_INTERFACE_MODE_MII ||
> >> 			 phyi == PHY_INTERFACE_MODE_RMII ||
> >> 			 (phyi == PHY_INTERFACE_MODE_GMII &&
> >> 			  phydev->speed < 1000)))
> >> 		tempval &= ~MACCFG2_FULL_DUPLEX;
> >> 	else
> >> 		tempval |= MACCFG2_FULL_DUPLEX;
> >>
> >> i.e. we detected GMII interface initially, but it downgraded
> >> to MII since speed is < 1000, thus we can set half-duplex in MAC?
> >
> > Yeah, I think that works out more correctly.
> 
> Cool, thanks.
> 
> Do you happen to know how gianfar iface auto-detection works in HW?
> I mean, if we connect 100 Mbs link to the GMII PHY, then
> gfar_get_interface() would return MII, correct?

Stupid me. HW has nothing to do with this. GMII, just as you said,
is just a marker, comes from FSL_GIANFAR_DEV_HAS_GIGABIT flag.

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2