ep93xx_eth PHY problems

ep93xx_eth PHY problems

[Oliver Martin]

Hello,

I was having problems with the ep93xx_eth driver: apparently the PHY
auto-negotiated "100/full" with the switch, but the driver didn't care
about it (or is it the other way round?). This led to lots of packets
being dropped under load. Anyway, the patch that was posted last
november (http://marc.info/?l=linux-netdev&m=119520642207001&w=4)
helped.

Now I'm having different problems: When auto-negotiation is enabled, it 
sometimes randomly changes the link state:

PHY: 0:01 - Link is Down
PHY: 0:01 - Link is Up - 100/Full

PHY: 0:01 - Link is Up - 100/Half
PHY: 0:01 - Link is Up - 100/Full

PHY: 0:01 - Link is Up - 10/Half
PHY: 0:01 - Link is Up - 100/Full

It could be related to some kind of error handling - often, when this
happens, our test (a simple RFC 862 UDP echo server, see [1]) loses 
packets, but the message seems to appear only after the test fails.

If I disable auto-negotiation with ethtool, it first changes to the mode
I choose, but seconds later changes to "1000/Full":

PHY: 0:01 - Link is Up - 100/Full
PHY: 0:01 - Link is Up - 1000/Full

This is obviously impossible, and while it mostly works, it seems to
confuse something (PHY? MAC? driver?), as there seem to be more dropped
packets than with the Cirrus 2.4 driver (on a TS-7260 with the default
TS-Linux).

Maybe some error condition isn't handled properly, causing both the 
packet loss and the PHY state change? Of course, I'm just speculating 
and it might be something entirely different.

Any ideas?

-- 
Oliver

[1] http://murli.34sp.com/o/ep93xx-eth/echo/

[PATCH] Re: ep93xx_eth PHY problems

[Oliver Martin]

Am Tue, 04 Mar 2008 14:02:15 +0100
schrieb Oliver Martin <oliver.martin@student.tuwien.ac.at>:
> 
> Now I'm having different problems: When auto-negotiation is enabled,
> it sometimes randomly changes the link state:
> 
> PHY: 0:01 - Link is Down
> PHY: 0:01 - Link is Up - 100/Full
> 

I've found at least one part of the problem. The MDIO clock divisor
wasn't set again after the reset in ep93xx_start_hw. It was left at the
default, resulting in a 12.5 MHz clock, which the PHY (Micrel KSZ8721BL)
didn't like too much. Calling ep93xx_mdio_reset after the reset fixed
it, the link is now perfectly stable in auto-negotiation mode.

There still seems to be another bug with auto-negotiation disabled: It
doesn't switch to 1000 Mbps any more, but most times it doesn't stay in
the intended configuration either. I haven't done any extensive
testing of this yet, though.

Signed-off-by: Oliver Martin <oliver.martin@student.tuwien.ac.at>

--- linux-2.6.24-phy/drivers/net/arm/ep93xx_eth.c.orig	2008-03-19 15:18:29.000000000 +0100
+++ linux-2.6.24-phy/drivers/net/arm/ep93xx_eth.c	2008-03-19 15:43:44.000000000 +0100
@@ -535,6 +535,22 @@ err:
 	return 1;
 }
 
+static int ep93xx_mdio_reset(struct mii_bus *bus)
+{
+	struct ep93xx_priv *ep = bus->priv;
+
+	u32 selfctl = rdl(ep, REG_SELFCTL);
+
+	selfctl &= ~(REG_SELFCTL_MDCDIV_MSK | REG_SELFCTL_PSPRS);
+
+	selfctl |= (ep->mdc_divisor - 1) << REG_SELFCTL_MDCDIV_OFS;
+	selfctl |= REG_SELFCTL_PSPRS;
+
+	wrl(ep, REG_SELFCTL, selfctl);
+
+	return 0;
+}
+
 static int ep93xx_start_hw(struct net_device *dev)
 {
 	struct ep93xx_priv *ep = netdev_priv(dev);
@@ -553,6 +569,9 @@ static int ep93xx_start_hw(struct net_de
 		return 1;
 	}
 
+	/* The reset cleared REG_SELFCTL, so set the MDC divisor again */
+	ep93xx_mdio_reset(&ep->mii_bus);
+
 	/* Receive descriptor ring.  */
 	addr = ep->descs_dma_addr + offsetof(struct ep93xx_descs, rdesc);
 	wrl(ep, REG_RXDQBADD, addr);
@@ -776,22 +795,6 @@ static int ep93xx_mdio_write(struct mii_
 	return 0;
 }
 
-static int ep93xx_mdio_reset(struct mii_bus *bus)
-{
-	struct ep93xx_priv *ep = bus->priv;
-
-	u32 selfctl = rdl(ep, REG_SELFCTL);
-
-	selfctl &= ~(REG_SELFCTL_MDCDIV_MSK | REG_SELFCTL_PSPRS);
-
-	selfctl |= (ep->mdc_divisor - 1) << REG_SELFCTL_MDCDIV_OFS;
-	selfctl |= REG_SELFCTL_PSPRS;
-
-	wrl(ep, REG_SELFCTL, selfctl);
-
-	return 0;
-}
-
 
 static void ep93xx_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
 {

Re: [PATCH] Re: ep93xx_eth PHY problems

[Jeff Garzik]

Oliver Martin wrote:
> Am Tue, 04 Mar 2008 14:02:15 +0100
> schrieb Oliver Martin <oliver.martin@student.tuwien.ac.at>:
>> Now I'm having different problems: When auto-negotiation is enabled,
>> it sometimes randomly changes the link state:
>>
>> PHY: 0:01 - Link is Down
>> PHY: 0:01 - Link is Up - 100/Full
>>
> 
> I've found at least one part of the problem. The MDIO clock divisor
> wasn't set again after the reset in ep93xx_start_hw. It was left at the
> default, resulting in a 12.5 MHz clock, which the PHY (Micrel KSZ8721BL)
> didn't like too much. Calling ep93xx_mdio_reset after the reset fixed
> it, the link is now perfectly stable in auto-negotiation mode.
> 
> There still seems to be another bug with auto-negotiation disabled: It
> doesn't switch to 1000 Mbps any more, but most times it doesn't stay in
> the intended configuration either. I haven't done any extensive
> testing of this yet, though.
> 
> Signed-off-by: Oliver Martin <oliver.martin@student.tuwien.ac.at>

ACKs from Lennert, others?

I don't know enough about the hardware to say...

Seems straightforward to me?

2.6.25 or 2.6.26 material?

Any more testing results?

Re: [PATCH] Re: ep93xx_eth PHY problems

[Oliver Martin]

Am Wed, 26 Mar 2008 00:42:32 -0400 schrieb Jeff Garzik:
> 
> ACKs from Lennert, others?
> 
> I don't know enough about the hardware to say...
> 
> Seems straightforward to me?
> 
> 2.6.25 or 2.6.26 material?

It depends on Herbert Valerio Riedel's ep93xx_eth phylib conversion,
which isn't in 2.6.25-rc7 (but received an ACK from Lennert some
weeks ago). I guess it should go in whenever that is merged.

> 
> Any more testing results?
> 
Sorry, I haven't had much time recently, so I haven't further
investigated the autoneg off issue. I've seen it work fine sometimes,
so it's either some very weird bug or something totally unrelated.

-- 
Oliver

Re: [PATCH] Re: ep93xx_eth PHY problems

[Lennert Buytenhek]

On Sat, Mar 29, 2008 at 07:13:07PM +0100, Oliver Martin wrote:

> > ACKs from Lennert, others?
> > 
> > I don't know enough about the hardware to say...
> > 
> > Seems straightforward to me?
> > 
> > 2.6.25 or 2.6.26 material?
> 
> It depends on Herbert Valerio Riedel's ep93xx_eth phylib conversion,
> which isn't in 2.6.25-rc7 (but received an ACK from Lennert some
> weeks ago). I guess it should go in whenever that is merged.

ACK for 2.6.26, together with Herbert's changes.

Herbert, can you resend your latest?

Re: [PATCH] Re: ep93xx_eth PHY problems

[Jeff Garzik]

Oliver Martin wrote:
> Am Tue, 04 Mar 2008 14:02:15 +0100
> schrieb Oliver Martin <oliver.martin@student.tuwien.ac.at>:
>> Now I'm having different problems: When auto-negotiation is enabled,
>> it sometimes randomly changes the link state:
>>
>> PHY: 0:01 - Link is Down
>> PHY: 0:01 - Link is Up - 100/Full
>>
> 
> I've found at least one part of the problem. The MDIO clock divisor
> wasn't set again after the reset in ep93xx_start_hw. It was left at the
> default, resulting in a 12.5 MHz clock, which the PHY (Micrel KSZ8721BL)
> didn't like too much. Calling ep93xx_mdio_reset after the reset fixed
> it, the link is now perfectly stable in auto-negotiation mode.
> 
> There still seems to be another bug with auto-negotiation disabled: It
> doesn't switch to 1000 Mbps any more, but most times it doesn't stay in
> the intended configuration either. I haven't done any extensive
> testing of this yet, though.
> 
> Signed-off-by: Oliver Martin <oliver.martin@student.tuwien.ac.at>

Testing feedback?  Lennert ack?

Re: [PATCH] Re: ep93xx_eth PHY problems

[Lennert Buytenhek]

On Sat, Sep 13, 2008 at 10:11:32PM -0400, Jeff Garzik wrote:

> >Am Tue, 04 Mar 2008 14:02:15 +0100
> >schrieb Oliver Martin <oliver.martin@student.tuwien.ac.at>:
> >>Now I'm having different problems: When auto-negotiation is enabled,
> >>it sometimes randomly changes the link state:
> >>
> >>PHY: 0:01 - Link is Down
> >>PHY: 0:01 - Link is Up - 100/Full
> >
> >I've found at least one part of the problem. The MDIO clock divisor
> >wasn't set again after the reset in ep93xx_start_hw. It was left at the
> >default, resulting in a 12.5 MHz clock, which the PHY (Micrel KSZ8721BL)
> >didn't like too much. Calling ep93xx_mdio_reset after the reset fixed
> >it, the link is now perfectly stable in auto-negotiation mode.
> >
> >There still seems to be another bug with auto-negotiation disabled: It
> >doesn't switch to 1000 Mbps any more, but most times it doesn't stay in
> >the intended configuration either. I haven't done any extensive
> >testing of this yet, though.
> >
> >Signed-off-by: Oliver Martin <oliver.martin@student.tuwien.ac.at>
> 
> Testing feedback?  Lennert ack?

ACKed on March 29.  Since this patch depends on Herbert Valerio
Riedel's ep93xx_eth phylib conversion, I asked Herbert to resend his
latest version of those patches at that time, but I didn't see that
yet.

Herbert, have you made changes to the ep93xx_eth phylib patch since
you last posted it and/or what is the latest version?

Re: [PATCH] Re: ep93xx_eth PHY problems

[Herbert Valerio Riedel]

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

On Sun, 2008-09-14 at 13:47 +0200, Lennert Buytenhek wrote:
> Herbert, have you made changes to the ep93xx_eth phylib patch since
> you last posted it and/or what is the latest version?

I have made no changes since last post, and right now I wouldn't have
the hardware available to test anything... :-/

cheers,
hvr

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Re: ep93xx_eth PHY problems

[Oliver Martin]

Am Sat, 13 Sep 2008 22:11:32 -0400 schrieb Jeff Garzik:

> Oliver Martin wrote:
> > Am Tue, 04 Mar 2008 14:02:15 +0100
> > schrieb Oliver Martin <oliver.martin@student.tuwien.ac.at>:
> >> Now I'm having different problems: When auto-negotiation is
> >> enabled, it sometimes randomly changes the link state:
> >>
> >> PHY: 0:01 - Link is Down
> >> PHY: 0:01 - Link is Up - 100/Full
> >>
> > 
> > I've found at least one part of the problem. The MDIO clock divisor
> > wasn't set again after the reset in ep93xx_start_hw. It was left at
> > the default, resulting in a 12.5 MHz clock, which the PHY (Micrel
> > KSZ8721BL) didn't like too much. Calling ep93xx_mdio_reset after
> > the reset fixed it, the link is now perfectly stable in
> > auto-negotiation mode.
> > 
> > There still seems to be another bug with auto-negotiation disabled:
> > It doesn't switch to 1000 Mbps any more, but most times it doesn't
> > stay in the intended configuration either. I haven't done any
> > extensive testing of this yet, though.
> > 
> > Signed-off-by: Oliver Martin <oliver.martin@student.tuwien.ac.at>
> 
> Testing feedback?  Lennert ack?

We're currently using a variation of my original patch, which didn't
handle ifdown correctly (there's a similar reset there). With the
current setup, everything seems stable, but we still have some rather
hackish workarounds for the original issue in our startup scripts.
I don't have the hardware right now, but we'll specifically test
without them both with and without auto-negotiation next week and
report the results.


Index: linux-2.6.26/drivers/net/arm/ep93xx_eth.c
===================================================================
--- linux-2.6.26.orig/drivers/net/arm/ep93xx_eth.c	2008-07-25 03:13:33.000000000 +0200
+++ linux-2.6.26/drivers/net/arm/ep93xx_eth.c	2008-07-25 03:14:52.000000000 +0200
@@ -535,6 +535,22 @@
 	return 1;
 }
 
+static int ep93xx_mdio_reset(struct mii_bus *bus)
+{
+	struct ep93xx_priv *ep = bus->priv;
+
+	u32 selfctl = rdl(ep, REG_SELFCTL);
+
+	selfctl &= ~(REG_SELFCTL_MDCDIV_MSK | REG_SELFCTL_PSPRS);
+
+	selfctl |= (ep->mdc_divisor - 1) << REG_SELFCTL_MDCDIV_OFS;
+	selfctl |= REG_SELFCTL_PSPRS;
+
+	wrl(ep, REG_SELFCTL, selfctl);
+
+	return 0;
+}
+
 static int ep93xx_start_hw(struct net_device *dev)
 {
 	struct ep93xx_priv *ep = netdev_priv(dev);
@@ -553,6 +569,9 @@
 		return 1;
 	}
 
+	/* The reset cleared REG_SELFCTL, so set the MDC divisor again */
+	ep93xx_mdio_reset(&ep->mii_bus);
+
 	/* Receive descriptor ring.  */
 	addr = ep->descs_dma_addr + offsetof(struct ep93xx_descs, rdesc);
 	wrl(ep, REG_RXDQBADD, addr);
@@ -625,6 +644,9 @@
 
 	if (i == 10)
 		printk(KERN_CRIT DRV_MODULE_NAME ": hw failed to reset\n");
+
+	/* The reset cleared REG_SELFCTL, so set the MDC divisor again */
+	ep93xx_mdio_reset(&ep->mii_bus);
 }
 
 static int ep93xx_open(struct net_device *dev)
@@ -776,22 +798,6 @@
 	return 0;
 }
 
-static int ep93xx_mdio_reset(struct mii_bus *bus)
-{
-	struct ep93xx_priv *ep = bus->priv;
-
-	u32 selfctl = rdl(ep, REG_SELFCTL);
-
-	selfctl &= ~(REG_SELFCTL_MDCDIV_MSK | REG_SELFCTL_PSPRS);
-
-	selfctl |= (ep->mdc_divisor - 1) << REG_SELFCTL_MDCDIV_OFS;
-	selfctl |= REG_SELFCTL_PSPRS;
-
-	wrl(ep, REG_SELFCTL, selfctl);
-
-	return 0;
-}
-
 
 static void ep93xx_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
 {

Re: [PATCH] Re: ep93xx_eth PHY problems

[Lennert Buytenhek]

On Sun, Sep 14, 2008 at 02:11:10PM +0200, Oliver Martin wrote:

> > Oliver Martin wrote:
> > > Am Tue, 04 Mar 2008 14:02:15 +0100
> > > schrieb Oliver Martin <oliver.martin@student.tuwien.ac.at>:
> > >> Now I'm having different problems: When auto-negotiation is
> > >> enabled, it sometimes randomly changes the link state:
> > >>
> > >> PHY: 0:01 - Link is Down
> > >> PHY: 0:01 - Link is Up - 100/Full
> > >>
> > > 
> > > I've found at least one part of the problem. The MDIO clock divisor
> > > wasn't set again after the reset in ep93xx_start_hw. It was left at
> > > the default, resulting in a 12.5 MHz clock, which the PHY (Micrel
> > > KSZ8721BL) didn't like too much. Calling ep93xx_mdio_reset after
> > > the reset fixed it, the link is now perfectly stable in
> > > auto-negotiation mode.
> > > 
> > > There still seems to be another bug with auto-negotiation disabled:
> > > It doesn't switch to 1000 Mbps any more, but most times it doesn't
> > > stay in the intended configuration either. I haven't done any
> > > extensive testing of this yet, though.
> > > 
> > > Signed-off-by: Oliver Martin <oliver.martin@student.tuwien.ac.at>
> > 
> > Testing feedback?  Lennert ack?
> 
> We're currently using a variation of my original patch, which didn't
> handle ifdown correctly (there's a similar reset there). With the
> current setup, everything seems stable, but we still have some rather
> hackish workarounds for the original issue in our startup scripts.
> I don't have the hardware right now, but we'll specifically test
> without them both with and without auto-negotiation next week and
> report the results.

Can you stick this somewhere together with the version of the phylib
patch you are using?

Re: [PATCH] Re: ep93xx_eth PHY problems

[Oliver Martin]

Am Sun, 14 Sep 2008 14:12:26 +0200 schrieb Lennert Buytenhek:
> > 
> > We're currently using a variation of my original patch, which didn't
> > handle ifdown correctly (there's a similar reset there). With the
> > current setup, everything seems stable, but we still have some
> > rather hackish workarounds for the original issue in our startup
> > scripts. I don't have the hardware right now, but we'll
> > specifically test without them both with and without
> > auto-negotiation next week and report the results.
> 
> Can you stick this somewhere together with the version of the phylib
> patch you are using?

I've only changed a single line for 2.6.26, specifically the change of
mii_bus.id from int to string. Anyway, here it is:


Currently, the ep93xx_eth driver doesn't care about the PHY state,
but it should, in order to tell the MAC when full duplex operation is
required; failure to do so causes degraded performance on full duplex
links. This patch implements proper PHY handling via the phylib framework:

 - clean up ep93xx_mdio_{read,write} to conform to ep93xx manual
 - convert ep93xx_eth driver to phylib framework
 - set full duplex bit in configuration of MAC when FDX link detected
 - convert to use print_mac()

Signed-off-by: Herbert Valerio Riedel <hvr@gnu.org>
Cc: Lennert Buytenhek <buytenh@wantstofly.org>
Cc: Andy Fleming <afleming@freescale.com>
---
If you need me to split this patch, please let me know into 
what parts

 drivers/net/arm/Kconfig      |    1 +
 drivers/net/arm/ep93xx_eth.c |  354 +++++++++++++++++++++++++++++++++---------
 2 files changed, 282 insertions(+), 73 deletions(-)

Index: linux-2.6.26/drivers/net/arm/Kconfig
===================================================================
--- linux-2.6.26.orig/drivers/net/arm/Kconfig	2008-07-13 23:51:29.000000000 +0200
+++ linux-2.6.26/drivers/net/arm/Kconfig	2008-07-25 02:46:06.000000000 +0200
@@ -44,6 +44,7 @@
 	tristate "EP93xx Ethernet support"
 	depends on ARM && ARCH_EP93XX
 	select MII
+	select PHYLIB
 	help
 	  This is a driver for the ethernet hardware included in EP93xx CPUs.
 	  Say Y if you are building a kernel for EP93xx based devices.
Index: linux-2.6.26/drivers/net/arm/ep93xx_eth.c
===================================================================
--- linux-2.6.26.orig/drivers/net/arm/ep93xx_eth.c	2008-07-13 23:51:29.000000000 +0200
+++ linux-2.6.26/drivers/net/arm/ep93xx_eth.c	2008-07-25 03:04:51.000000000 +0200
@@ -2,6 +2,7 @@
  * EP93xx ethernet network device driver
  * Copyright (C) 2006 Lennert Buytenhek <buytenh@wantstofly.org>
  * Dedicated to Marija Kulikova.
+ * Copyright (C) 2007 Herbert Valerio Riedel <hvr@gnu.org>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -14,6 +15,7 @@
 #include <linux/kernel.h>
 #include <linux/netdevice.h>
 #include <linux/mii.h>
+#include <linux/phy.h>
 #include <linux/etherdevice.h>
 #include <linux/ethtool.h>
 #include <linux/init.h>
@@ -37,6 +39,8 @@
 #define  REG_RXCTL_DEFAULT	0x00073800
 #define REG_TXCTL		0x0004
 #define  REG_TXCTL_ENABLE	0x00000001
+#define REG_TESTCTL		0x0008
+#define  REG_TESTCTL_MFDX	0x00000040
 #define REG_MIICMD		0x0010
 #define  REG_MIICMD_READ	0x00008000
 #define  REG_MIICMD_WRITE	0x00004000
@@ -45,6 +49,9 @@
 #define  REG_MIISTS_BUSY	0x00000001
 #define REG_SELFCTL		0x0020
 #define  REG_SELFCTL_RESET	0x00000001
+#define  REG_SELFCTL_MDCDIV_MSK 0x00007e00
+#define  REG_SELFCTL_MDCDIV_OFS 9
+#define  REG_SELFCTL_PSPRS	0x00000100
 #define REG_INTEN		0x0024
 #define  REG_INTEN_TX		0x00000008
 #define  REG_INTEN_RX		0x00000007
@@ -174,8 +181,14 @@
 
 	struct net_device_stats	stats;
 
-	struct mii_if_info	mii;
 	u8			mdc_divisor;
+	int			phy_supports_mfps:1;
+
+	struct mii_bus		mii_bus;
+	struct phy_device	*phy_dev;
+	int			speed;
+	int			duplex;
+	int			link;
 };
 
 #define rdb(ep, off)		__raw_readb((ep)->base_addr + (off))
@@ -185,8 +198,6 @@
 #define wrw(ep, off, val)	__raw_writew((val), (ep)->base_addr + (off))
 #define wrl(ep, off, val)	__raw_writel((val), (ep)->base_addr + (off))
 
-static int ep93xx_mdio_read(struct net_device *dev, int phy_id, int reg);
-
 static struct net_device_stats *ep93xx_get_stats(struct net_device *dev)
 {
 	struct ep93xx_priv *ep = netdev_priv(dev);
@@ -542,12 +553,6 @@
 		return 1;
 	}
 
-	wrl(ep, REG_SELFCTL, ((ep->mdc_divisor - 1) << 9));
-
-	/* Does the PHY support preamble suppress?  */
-	if ((ep93xx_mdio_read(dev, ep->mii.phy_id, MII_BMSR) & 0x0040) != 0)
-		wrl(ep, REG_SELFCTL, ((ep->mdc_divisor - 1) << 9) | (1 << 8));
-
 	/* Receive descriptor ring.  */
 	addr = ep->descs_dma_addr + offsetof(struct ep93xx_descs, rdesc);
 	wrl(ep, REG_RXDQBADD, addr);
@@ -631,12 +636,11 @@
 		return -ENOMEM;
 
 	if (is_zero_ether_addr(dev->dev_addr)) {
+		DECLARE_MAC_BUF(mac_buf);
+
 		random_ether_addr(dev->dev_addr);
-		printk(KERN_INFO "%s: generated random MAC address "
-			"%.2x:%.2x:%.2x:%.2x:%.2x:%.2x.\n", dev->name,
-			dev->dev_addr[0], dev->dev_addr[1],
-			dev->dev_addr[2], dev->dev_addr[3],
-			dev->dev_addr[4], dev->dev_addr[5]);
+		dev_info(&dev->dev, "generated random MAC address %s\n",
+			 print_mac(mac_buf, dev->dev_addr));
 	}
 
 	napi_enable(&ep->napi);
@@ -664,6 +668,8 @@
 
 	wrl(ep, REG_GIINTMSK, REG_GIINTMSK_ENABLE);
 
+	phy_start(ep->phy_dev);
+
 	netif_start_queue(dev);
 
 	return 0;
@@ -676,6 +682,9 @@
 	napi_disable(&ep->napi);
 	netif_stop_queue(dev);
 
+	if (ep->phy_dev)
+		phy_stop(ep->phy_dev);
+
 	wrl(ep, REG_GIINTMSK, 0);
 	free_irq(ep->irq, dev);
 	ep93xx_stop_hw(dev);
@@ -687,53 +696,103 @@
 static int ep93xx_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 {
 	struct ep93xx_priv *ep = netdev_priv(dev);
-	struct mii_ioctl_data *data = if_mii(ifr);
 
-	return generic_mii_ioctl(&ep->mii, data, cmd, NULL);
+	return phy_mii_ioctl(ep->phy_dev, if_mii(ifr), cmd);
 }
 
-static int ep93xx_mdio_read(struct net_device *dev, int phy_id, int reg)
+/* common MII transactions should take < 100 iterations */
+#define EP93XX_PHY_TIMEOUT 2000
+
+static int ep93xx_mdio_wait(struct mii_bus *bus)
 {
-	struct ep93xx_priv *ep = netdev_priv(dev);
-	int data;
-	int i;
+	struct ep93xx_priv *ep = bus->priv;
+	unsigned int timeout = EP93XX_PHY_TIMEOUT;
 
-	wrl(ep, REG_MIICMD, REG_MIICMD_READ | (phy_id << 5) | reg);
+	while ((rdl(ep, REG_MIISTS) & REG_MIISTS_BUSY)
+	      && timeout--)
+		cpu_relax();
 
-	for (i = 0; i < 10; i++) {
-		if ((rdl(ep, REG_MIISTS) & REG_MIISTS_BUSY) == 0)
-			break;
-		msleep(1);
+	if (timeout <= 0) {
+		dev_err(bus->dev, "MII operation timed out\n");
+		return -ETIMEDOUT;
 	}
 
-	if (i == 10) {
-		printk(KERN_INFO DRV_MODULE_NAME ": mdio read timed out\n");
-		data = 0xffff;
-	} else {
-		data = rdl(ep, REG_MIIDATA);
-	}
+	return 0;
+}
+
+
+static int ep93xx_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
+{
+	struct ep93xx_priv *ep = bus->priv;
+	u32 selfctl;
+	u32 data;
+
+	if (ep93xx_mdio_wait(bus) < 0)
+		return -ETIMEDOUT;
+
+	selfctl = rdl(ep, REG_SELFCTL);
+
+	if (ep->phy_supports_mfps)
+	  wrl(ep, REG_SELFCTL, selfctl | REG_SELFCTL_PSPRS);
+	else
+	  wrl(ep, REG_SELFCTL, selfctl & ~REG_SELFCTL_PSPRS);
+
+	wrl(ep, REG_MIICMD, REG_MIICMD_READ | (mii_id << 5) | regnum);
+
+	if (ep93xx_mdio_wait(bus) < 0)
+		return -ETIMEDOUT;
+
+	data =  rdl(ep, REG_MIIDATA);
+
+	wrl(ep, REG_SELFCTL, selfctl);
 
 	return data;
 }
 
-static void ep93xx_mdio_write(struct net_device *dev, int phy_id, int reg, int data)
+static int ep93xx_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
+			     u16 value)
 {
-	struct ep93xx_priv *ep = netdev_priv(dev);
-	int i;
+	struct ep93xx_priv *ep = bus->priv;
+	u32 selfctl;
 
-	wrl(ep, REG_MIIDATA, data);
-	wrl(ep, REG_MIICMD, REG_MIICMD_WRITE | (phy_id << 5) | reg);
+	if (ep93xx_mdio_wait(bus) < 0)
+		return -ETIMEDOUT;
 
-	for (i = 0; i < 10; i++) {
-		if ((rdl(ep, REG_MIISTS) & REG_MIISTS_BUSY) == 0)
-			break;
-		msleep(1);
-	}
+	selfctl = rdl(ep, REG_SELFCTL);
 
-	if (i == 10)
-		printk(KERN_INFO DRV_MODULE_NAME ": mdio write timed out\n");
+	if (ep->phy_supports_mfps)
+	  wrl(ep, REG_SELFCTL, selfctl | REG_SELFCTL_PSPRS);
+	else
+	  wrl(ep, REG_SELFCTL, selfctl & ~REG_SELFCTL_PSPRS);
+
+	wrl(ep, REG_MIIDATA, value);
+	wrl(ep, REG_MIICMD, REG_MIICMD_WRITE | (mii_id << 5) | regnum);
+
+	if (ep93xx_mdio_wait(bus) < 0)
+		return -ETIMEDOUT;
+
+	wrl(ep, REG_SELFCTL, selfctl);
+
+	return 0;
 }
 
+static int ep93xx_mdio_reset(struct mii_bus *bus)
+{
+	struct ep93xx_priv *ep = bus->priv;
+
+	u32 selfctl = rdl(ep, REG_SELFCTL);
+
+	selfctl &= ~(REG_SELFCTL_MDCDIV_MSK | REG_SELFCTL_PSPRS);
+
+	selfctl |= (ep->mdc_divisor - 1) << REG_SELFCTL_MDCDIV_OFS;
+	selfctl |= REG_SELFCTL_PSPRS;
+
+	wrl(ep, REG_SELFCTL, selfctl);
+
+	return 0;
+}
+
+
 static void ep93xx_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
 {
 	strcpy(info->driver, DRV_MODULE_NAME);
@@ -743,33 +802,30 @@
 static int ep93xx_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
 	struct ep93xx_priv *ep = netdev_priv(dev);
-	return mii_ethtool_gset(&ep->mii, cmd);
+	struct phy_device *phydev = ep->phy_dev;
+
+	if (!phydev)
+		return -ENODEV;
+
+	return phy_ethtool_gset(phydev, cmd);
 }
 
 static int ep93xx_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
 	struct ep93xx_priv *ep = netdev_priv(dev);
-	return mii_ethtool_sset(&ep->mii, cmd);
-}
+	struct phy_device *phydev = ep->phy_dev;
 
-static int ep93xx_nway_reset(struct net_device *dev)
-{
-	struct ep93xx_priv *ep = netdev_priv(dev);
-	return mii_nway_restart(&ep->mii);
-}
+	if (!phydev)
+		return -ENODEV;
 
-static u32 ep93xx_get_link(struct net_device *dev)
-{
-	struct ep93xx_priv *ep = netdev_priv(dev);
-	return mii_link_ok(&ep->mii);
+	return phy_ethtool_sset(phydev, cmd);
 }
 
 static struct ethtool_ops ep93xx_ethtool_ops = {
 	.get_drvinfo		= ep93xx_get_drvinfo,
 	.get_settings		= ep93xx_get_settings,
 	.set_settings		= ep93xx_set_settings,
-	.nway_reset		= ep93xx_nway_reset,
-	.get_link		= ep93xx_get_link,
+	.get_link		= ethtool_op_get_link,
 };
 
 struct net_device *ep93xx_dev_alloc(struct ep93xx_eth_data *data)
@@ -824,15 +880,127 @@
 	return 0;
 }
 
+static void ep93xx_adjust_link(struct net_device *dev)
+{
+	struct ep93xx_priv *ep = netdev_priv(dev);
+	struct phy_device *phydev = ep->phy_dev;
+
+	int status_change = 0;
+
+	if (phydev->link) {
+		if ((ep->speed != phydev->speed) ||
+		    (ep->duplex != phydev->duplex)) {
+			/* speed and/or duplex state changed */
+			u32 testctl = rdl(ep, REG_TESTCTL);
+
+			if (DUPLEX_FULL == phydev->duplex)
+				testctl |= REG_TESTCTL_MFDX;
+			else
+				testctl &= ~(REG_TESTCTL_MFDX);
+
+			wrl(ep, REG_TESTCTL, testctl);
+
+			ep->speed = phydev->speed;
+			ep->duplex = phydev->duplex;
+			status_change = 1;
+		}
+	}
+
+	/* test for online/offline link transition */
+	if (phydev->link != ep->link) {
+		if (phydev->link) /* link went online */
+			netif_schedule(dev);
+		else { /* link went offline */
+			ep->speed = 0;
+			ep->duplex = -1;
+		}
+		ep->link = phydev->link;
+
+		status_change = 1;
+	}
+
+	if (status_change)
+		phy_print_status(phydev);
+}
+
+static int ep93xx_mii_probe(struct net_device *dev, int phy_addr)
+{
+	struct ep93xx_priv *ep = netdev_priv(dev);
+	struct phy_device *phydev = NULL;
+	int val;
+
+	if (phy_addr >= 0 && phy_addr < PHY_MAX_ADDR)
+		phydev = ep->mii_bus.phy_map[phy_addr];
+
+	if (!phydev) {
+		dev_info(&dev->dev,
+			 "PHY not found at specified address,"
+			 " trying autodetection\n");
+
+		/* find the first phy */
+		for (phy_addr = 0; phy_addr < PHY_MAX_ADDR; phy_addr++) {
+			if (ep->mii_bus.phy_map[phy_addr]) {
+				phydev = ep->mii_bus.phy_map[phy_addr];
+				break;
+			}
+		}
+	}
+
+	if (!phydev) {
+		dev_err(&dev->dev, "no PHY found\n");
+		return -ENODEV;
+	}
+
+	phydev = phy_connect(dev, phydev->dev.bus_id,
+			     ep93xx_adjust_link, 0, PHY_INTERFACE_MODE_MII);
+
+	if (IS_ERR(phydev)) {
+		dev_err(&dev->dev, "Could not attach to PHY\n");
+		return PTR_ERR(phydev);
+	}
+
+	ep->phy_supports_mfps = 0;
+
+	val = phy_read(phydev, MII_BMSR);
+	if (val < 0) {
+		dev_err(&phydev->dev, "failed to read MII register\n");
+		return val;
+	}
+
+	if (val & 0x0040) {
+		dev_info(&phydev->dev,
+			 "PHY supports MII frame preamble suppression\n");
+		ep->phy_supports_mfps = 1;
+	}
+
+	phydev->supported &= PHY_BASIC_FEATURES;
+
+	phydev->advertising = phydev->supported;
+
+	ep->link = 0;
+	ep->speed = 0;
+	ep->duplex = -1;
+	ep->phy_dev = phydev;
+
+	dev_info(&dev->dev, "attached PHY driver [%s] "
+		 "(mii_bus:phy_addr=%s, irq=%d)\n",
+		 phydev->drv->name, phydev->dev.bus_id, phydev->irq);
+
+	return 0;
+}
+
+
 static int ep93xx_eth_probe(struct platform_device *pdev)
 {
 	struct ep93xx_eth_data *data;
 	struct net_device *dev;
 	struct ep93xx_priv *ep;
-	int err;
+	DECLARE_MAC_BUF(mac_buf);
+	int err, i;
 
 	if (pdev == NULL)
 		return -ENODEV;
+
 	data = pdev->dev.platform_data;
 
 	dev = ep93xx_dev_alloc(data);
@@ -852,7 +1020,7 @@
 	if (ep->res == NULL) {
 		dev_err(&pdev->dev, "Could not reserve memory region\n");
 		err = -ENOMEM;
-		goto err_out;
+		goto err_out_request_mem_region;
 	}
 
 	ep->base_addr = ioremap(pdev->resource[0].start,
@@ -860,34 +1028,74 @@
 	if (ep->base_addr == NULL) {
 		dev_err(&pdev->dev, "Failed to ioremap ethernet registers\n");
 		err = -EIO;
-		goto err_out;
+		goto err_out_ioremap;
 	}
 	ep->irq = pdev->resource[1].start;
 
-	ep->mii.phy_id = data->phy_id;
-	ep->mii.phy_id_mask = 0x1f;
-	ep->mii.reg_num_mask = 0x1f;
-	ep->mii.dev = dev;
-	ep->mii.mdio_read = ep93xx_mdio_read;
-	ep->mii.mdio_write = ep93xx_mdio_write;
+	/* mdio/mii bus */
+	ep->mii_bus.name = "ep93xx_mii_bus";
+	snprintf(ep->mii_bus.id, MII_BUS_ID_SIZE, "%x", 0);
+
+	ep->mii_bus.read = ep93xx_mdio_read;
+	ep->mii_bus.write = ep93xx_mdio_write;
+	ep->mii_bus.reset = ep93xx_mdio_reset;
+
+	ep->mii_bus.phy_mask = 0;
+
+	ep->mii_bus.priv = ep;
+	ep->mii_bus.dev = &dev->dev;
+
+	ep->mii_bus.irq = kmalloc(sizeof(int)*PHY_MAX_ADDR, GFP_KERNEL);
+	if (NULL == ep->mii_bus.irq) {
+		dev_err(&pdev->dev, "Could not allocate memory\n");
+		err = -ENOMEM;
+		goto err_out_mii_bus_irq_kmalloc;
+	}
+
+	for (i = 0; i < PHY_MAX_ADDR; i++)
+		ep->mii_bus.irq[i] = PHY_POLL;
+
 	ep->mdc_divisor = 40;	/* Max HCLK 100 MHz, min MDIO clk 2.5 MHz.  */
+	ep->phy_supports_mfps = 0; /* probe without preamble suppression */
 
 	err = register_netdev(dev);
 	if (err) {
 		dev_err(&pdev->dev, "Failed to register netdev\n");
-		goto err_out;
+		goto err_out_register_netdev;
+	}
+
+	err = mdiobus_register(&ep->mii_bus);
+	if (err) {
+		dev_err(&dev->dev, "Could not register MII bus\n");
+		goto err_out_mdiobus_register;
+	}
+
+	err = ep93xx_mii_probe(dev, data->phy_id);
+	if (err) {
+		dev_err(&dev->dev, "failed to probe MII bus\n");
+		goto err_out_mii_probe;
 	}
 
-	printk(KERN_INFO "%s: ep93xx on-chip ethernet, IRQ %d, "
-			 "%.2x:%.2x:%.2x:%.2x:%.2x:%.2x.\n", dev->name,
-			ep->irq, data->dev_addr[0], data->dev_addr[1],
-			data->dev_addr[2], data->dev_addr[3],
-			data->dev_addr[4], data->dev_addr[5]);
+	dev_info(&dev->dev, "ep93xx on-chip ethernet, IRQ %d, %s\n",
+		 ep->irq, print_mac(mac_buf, dev->dev_addr));
 
 	return 0;
 
+err_out_mii_probe:
+	mdiobus_unregister(&ep->mii_bus);
+err_out_mdiobus_register:
+	unregister_netdev(dev);
+err_out_register_netdev:
+	kfree(ep->mii_bus.irq);
+err_out_mii_bus_irq_kmalloc:
+	iounmap(ep->base_addr);
+err_out_ioremap:
+	release_resource(ep->res);
+	kfree(ep->res);
+err_out_request_mem_region:
+	free_netdev(dev);
 err_out:
-	ep93xx_eth_remove(pdev);
+
 	return err;
 }
 

Re: [PATCH] Re: ep93xx_eth PHY problems

[Lennert Buytenhek]

On Sun, Sep 14, 2008 at 02:11:10PM +0200, Oliver Martin wrote:

> > > Am Tue, 04 Mar 2008 14:02:15 +0100
> > > schrieb Oliver Martin <oliver.martin@student.tuwien.ac.at>:
> > >> Now I'm having different problems: When auto-negotiation is
> > >> enabled, it sometimes randomly changes the link state:
> > >>
> > >> PHY: 0:01 - Link is Down
> > >> PHY: 0:01 - Link is Up - 100/Full
> > >>
> > > 
> > > I've found at least one part of the problem. The MDIO clock divisor
> > > wasn't set again after the reset in ep93xx_start_hw. It was left at
> > > the default, resulting in a 12.5 MHz clock, which the PHY (Micrel
> > > KSZ8721BL) didn't like too much. Calling ep93xx_mdio_reset after
> > > the reset fixed it, the link is now perfectly stable in
> > > auto-negotiation mode.
> > > 
> > > There still seems to be another bug with auto-negotiation disabled:
> > > It doesn't switch to 1000 Mbps any more, but most times it doesn't
> > > stay in the intended configuration either. I haven't done any
> > > extensive testing of this yet, though.
> > > 
> > > Signed-off-by: Oliver Martin <oliver.martin@student.tuwien.ac.at>
> > 
> > Testing feedback?  Lennert ack?
> 
> We're currently using a variation of my original patch, which didn't
> handle ifdown correctly (there's a similar reset there). With the
> current setup, everything seems stable, but we still have some rather
> hackish workarounds for the original issue in our startup scripts.
> I don't have the hardware right now, but we'll specifically test
> without them both with and without auto-negotiation next week and
> report the results.

OK, can you provide a commit message for this so that I can add it to
http://git.wantstofly.org/?p=ep93xx_eth.git/.git;a=summary and ask Jeff
to pull it?


> Index: linux-2.6.26/drivers/net/arm/ep93xx_eth.c
> ===================================================================
> --- linux-2.6.26.orig/drivers/net/arm/ep93xx_eth.c	2008-07-25 03:13:33.000000000 +0200
> +++ linux-2.6.26/drivers/net/arm/ep93xx_eth.c	2008-07-25 03:14:52.000000000 +0200
> @@ -535,6 +535,22 @@
>  	return 1;
>  }
>  
> +static int ep93xx_mdio_reset(struct mii_bus *bus)
> +{
> +	struct ep93xx_priv *ep = bus->priv;
> +
> +	u32 selfctl = rdl(ep, REG_SELFCTL);
> +
> +	selfctl &= ~(REG_SELFCTL_MDCDIV_MSK | REG_SELFCTL_PSPRS);
> +
> +	selfctl |= (ep->mdc_divisor - 1) << REG_SELFCTL_MDCDIV_OFS;
> +	selfctl |= REG_SELFCTL_PSPRS;
> +
> +	wrl(ep, REG_SELFCTL, selfctl);
> +
> +	return 0;
> +}
> +
>  static int ep93xx_start_hw(struct net_device *dev)
>  {
>  	struct ep93xx_priv *ep = netdev_priv(dev);
> @@ -553,6 +569,9 @@
>  		return 1;
>  	}
>  
> +	/* The reset cleared REG_SELFCTL, so set the MDC divisor again */
> +	ep93xx_mdio_reset(&ep->mii_bus);
> +
>  	/* Receive descriptor ring.  */
>  	addr = ep->descs_dma_addr + offsetof(struct ep93xx_descs, rdesc);
>  	wrl(ep, REG_RXDQBADD, addr);
> @@ -625,6 +644,9 @@
>  
>  	if (i == 10)
>  		printk(KERN_CRIT DRV_MODULE_NAME ": hw failed to reset\n");
> +
> +	/* The reset cleared REG_SELFCTL, so set the MDC divisor again */
> +	ep93xx_mdio_reset(&ep->mii_bus);
>  }
>  
>  static int ep93xx_open(struct net_device *dev)
> @@ -776,22 +798,6 @@
>  	return 0;
>  }
>  
> -static int ep93xx_mdio_reset(struct mii_bus *bus)
> -{
> -	struct ep93xx_priv *ep = bus->priv;
> -
> -	u32 selfctl = rdl(ep, REG_SELFCTL);
> -
> -	selfctl &= ~(REG_SELFCTL_MDCDIV_MSK | REG_SELFCTL_PSPRS);
> -
> -	selfctl |= (ep->mdc_divisor - 1) << REG_SELFCTL_MDCDIV_OFS;
> -	selfctl |= REG_SELFCTL_PSPRS;
> -
> -	wrl(ep, REG_SELFCTL, selfctl);
> -
> -	return 0;
> -}
> -
>  
>  static void ep93xx_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
>  {

Re: [PATCH] Re: ep93xx_eth PHY problems

[Oliver Martin]

Am Tue, 16 Sep 2008 14:51:25 +0200 schrieb Lennert Buytenhek:
> 
> OK, can you provide a commit message for this so that I can add it to
> http://git.wantstofly.org/?p=ep93xx_eth.git/.git;a=summary and ask
> Jeff to pull it?
> 

Previously, the MDIO clock divisor wasn't set again after the reset in
ep93xx_start_hw. It was left at the default, resulting in a 12.5 MHz
clock, which the PHY on our TS-7260 (Micrel KSZ8721BL) didn't like too
much. Calling ep93xx_mdio_reset after the reset fixed the problems we
were having. This small update to the previous patch also adds a call to
ep93xx_mdio_reset in ep93xx_stop_hw.

Signed-off-by: Oliver Martin <oliver.martin@student.tuwien.ac.at>

Index: linux-2.6.26/drivers/net/arm/ep93xx_eth.c
===================================================================
--- linux-2.6.26.orig/drivers/net/arm/ep93xx_eth.c	2008-07-25 03:13:33.000000000 +0200
+++ linux-2.6.26/drivers/net/arm/ep93xx_eth.c	2008-07-25 03:14:52.000000000 +0200
@@ -535,6 +535,22 @@
 	return 1;
 }
 
+static int ep93xx_mdio_reset(struct mii_bus *bus)
+{
+	struct ep93xx_priv *ep = bus->priv;
+
+	u32 selfctl = rdl(ep, REG_SELFCTL);
+
+	selfctl &= ~(REG_SELFCTL_MDCDIV_MSK | REG_SELFCTL_PSPRS);
+
+	selfctl |= (ep->mdc_divisor - 1) << REG_SELFCTL_MDCDIV_OFS;
+	selfctl |= REG_SELFCTL_PSPRS;
+
+	wrl(ep, REG_SELFCTL, selfctl);
+
+	return 0;
+}
+
 static int ep93xx_start_hw(struct net_device *dev)
 {
 	struct ep93xx_priv *ep = netdev_priv(dev);
@@ -553,6 +569,9 @@
 		return 1;
 	}
 
+	/* The reset cleared REG_SELFCTL, so set the MDC divisor again */
+	ep93xx_mdio_reset(&ep->mii_bus);
+
 	/* Receive descriptor ring.  */
 	addr = ep->descs_dma_addr + offsetof(struct ep93xx_descs, rdesc);
 	wrl(ep, REG_RXDQBADD, addr);
@@ -625,6 +644,9 @@
 
 	if (i == 10)
 		printk(KERN_CRIT DRV_MODULE_NAME ": hw failed to reset\n");
+
+	/* The reset cleared REG_SELFCTL, so set the MDC divisor again */
+	ep93xx_mdio_reset(&ep->mii_bus);
 }
 
 static int ep93xx_open(struct net_device *dev)
@@ -776,22 +798,6 @@
 	return 0;
 }
 
-static int ep93xx_mdio_reset(struct mii_bus *bus)
-{
-	struct ep93xx_priv *ep = bus->priv;
-
-	u32 selfctl = rdl(ep, REG_SELFCTL);
-
-	selfctl &= ~(REG_SELFCTL_MDCDIV_MSK | REG_SELFCTL_PSPRS);
-
-	selfctl |= (ep->mdc_divisor - 1) << REG_SELFCTL_MDCDIV_OFS;
-	selfctl |= REG_SELFCTL_PSPRS;
-
-	wrl(ep, REG_SELFCTL, selfctl);
-
-	return 0;
-}
-
 
 static void ep93xx_get_drvinfo(struct net_device *dev, struct ethtool_drvinfo *info)
 {