[PATCH v2 0/4] net: Revive fixed link support

[PATCH v2 0/4] net: Revive fixed link support

[Grant Likely]

This series is based on the "Net: Revive fixed link support" series
posted by Anton Vorontsov on June 26.

On June 26, Anton Vorontsov wrote:
> The fixed link support is broken since The Big OF MDIO Rework,
> the rework simply removed most of the code that was needed for
> fixed links.
> 
> Too bad I didn't notice this earlier, I saw a bunch of patches
> on the ml, but unfortunately I didn't look very close presuming
> that Grant knew what he was doing. :-) And obviously I didn't
> test linux-next on anything that requires a fixed link.
> 
> Anyway, here are four patches. The first one adds the fixed link
> support to a framework, otherwise we'd duplicate the code across
> the drivers (as we did previously), and I tried to keep drivers'
> changes minimal.

I took issue with the approach on the basis of
a) I find the whole dummy phy approach to be a hack and an abuse of the
   mdio bus,
b) Handling the lack of a phy is trivial and should just be done within
   the MAC driver itself, and
c) the approach caused all drivers using of_phy_connect() to parse the
   'fixed-link' property, when only three drivers actually define and
   use it.

I wrote and posted a competing patch which removed the usage of a dummy
phy and made each driver handle the lack of a phy gracefully.  However,
Anton rightly pointed out that my patch still leaves the regression of
userspace no longer being able to use ethtool to query the link speed
because the affected drivers all depend on phylib for the ethtool
ioctl implementations.

Part of the problem I think is that the phylib code merges two separate
constructs; the construct of an MDIO bus (on which many device may
reside, not all of them PHYs), and the construct of an MII link whose
speed and configuration need to be manipulated.  I've run into problems
myself on how best to handle things like Ethernet switches which
definitely do not behave like PHYs and the phylib state machine cannot
be used on them.  It seems to me that the whole 'dummy phy' approach
is just an artifact of the phylib model not being quite right yet. I
want to investigate the possibility of separating the two concepts, but
that will require a fair bit of thought and experimentation.

Right now this is a regression situation during the 2.6.31
stabilization, so I don't think it is appropriate to write a new set
of ioctl handlers at this late stage when there is another solution.
I've reworked Anton's patches to constrain the effects to just the
three drivers which use 'fixed-link'.

It kills me to leave the dummy phy approach in place, but the
alternative is to invasive and risky to apply at this stage.
It irks me to say so, but I think Anton's approach of reenabling
the dummy phy is the right think to do,

Anton, once again I don't have hardware to test this, so I rely on you
to tell be if I screwed it up.  It has been compile tested.

Cheers,
g.


--
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.

[PATCH v2 1/4] of/mdio: Add support function for Ethernet fixed-link property

[Grant Likely]

From: Anton Vorontsov <avorontsov@ru.mvista.com>

Fixed-link support is broken for the ucc_eth, gianfar, and fs_enet
device drivers.  The "OF MDIO rework" patches removed most of the
support. Instead of re-adding fixed-link stuff to the drivers, this
patch adds a support function for parsing the fixed-link property
and obtaining a dummy phy to match.

Note: the dummy phy handling in arch/powerpc is a bit of a hack and
needs to be reworked.  This function is being added now to solve the
regression in the Ethernet drivers, but it should be considered a
temporary measure until the fixed link handling can be reworked.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 drivers/of/of_mdio.c    |   42 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_mdio.h |    3 +++
 2 files changed, 45 insertions(+), 0 deletions(-)


diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index aee967d..bacaa53 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -9,6 +9,10 @@
  * out of the OpenFirmware device tree and using it to populate an mii_bus.
  */
 
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/netdevice.h>
+#include <linux/err.h>
 #include <linux/phy.h>
 #include <linux/of.h>
 #include <linux/of_mdio.h>
@@ -137,3 +141,41 @@ struct phy_device *of_phy_connect(struct net_device *dev,
 	return phy_connect_direct(dev, phy, hndlr, flags, iface) ? NULL : phy;
 }
 EXPORT_SYMBOL(of_phy_connect);
+
+/**
+ * of_phy_connect_fixed_link - Parse fixed-link property and return a dummy phy
+ * @dev: pointer to net_device claiming the phy
+ * @hndlr: Link state callback for the network device
+ * @iface: PHY data interface type
+ *
+ * This function is a temporary stop-gap and will be removed soon.  It is
+ * only to support the fs_enet, ucc_geth and gianfar Ethernet drivers.  Do
+ * not call this function from new drivers.
+ */
+struct phy_device *of_phy_connect_fixed_link(struct net_device *dev,
+					     void (*hndlr)(struct net_device *),
+					     phy_interface_t iface)
+{
+	struct device_node *net_np;
+	char bus_id[MII_BUS_ID_SIZE + 3];
+	struct phy_device *phy;
+	const u32 *phy_id;
+	int sz;
+
+	if (!dev->dev.parent)
+		return NULL;
+
+	net_np = dev_archdata_get_node(&dev->dev.parent->archdata);
+	if (!net_np)
+		return NULL;
+
+	phy_id = of_get_property(net_np, "fixed-link", &sz);
+	if (!phy_id || sz < sizeof(*phy_id))
+		return NULL;
+
+	sprintf(bus_id, PHY_ID_FMT, "0", phy_id[0]);
+
+	phy = phy_connect(dev, bus_id, hndlr, 0, iface);
+	return IS_ERR(phy) ? NULL : phy;
+}
+EXPORT_SYMBOL(of_phy_connect_fixed_link);
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index c9663c6..53b94e0 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -18,5 +18,8 @@ extern struct phy_device *of_phy_connect(struct net_device *dev,
 					 struct device_node *phy_np,
 					 void (*hndlr)(struct net_device *),
 					 u32 flags, phy_interface_t iface);
+extern struct phy_device *of_phy_connect_fixed_link(struct net_device *dev,
+					 void (*hndlr)(struct net_device *),
+					 phy_interface_t iface);
 
 #endif /* __LINUX_OF_MDIO_H */

[PATCH v2 2/4] fs_enet: Revive fixed link support

[Grant Likely]

From: Anton Vorontsov <avorontsov@ru.mvista.com>

Since commit aa73832c5a80d6c52c69b18af858d88fa595dd3c ("Rework
fs_enet driver to use of_mdio infrastructure") the fixed-link support
is broken in the fs_enet driver.

This patch fixes the support by removing a check for phy_node, and adding
a call to of_phy_connect_fixed_link().

Also set netdev parent device via SET_NETDEV_DEV() call, this is needed
so that OF MDIO core could find a node pointer for a device.

Plus, fix "if (IS_ERR(phydev))" check, in case of errors,
of_phy_connect() returns NULL, not ERR_PTR as phy_connect().

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 drivers/net/fs_enet/fs_enet-main.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)


diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c
index b892c3a..2bc2d2b 100644
--- a/drivers/net/fs_enet/fs_enet-main.c
+++ b/drivers/net/fs_enet/fs_enet-main.c
@@ -754,17 +754,16 @@ static int fs_init_phy(struct net_device *dev)
 	fep->oldlink = 0;
 	fep->oldspeed = 0;
 	fep->oldduplex = -1;
-	if(fep->fpi->phy_node)
-		phydev = of_phy_connect(dev, fep->fpi->phy_node,
-					&fs_adjust_link, 0,
-					PHY_INTERFACE_MODE_MII);
-	else {
-		printk("No phy bus ID specified in BSP code\n");
-		return -EINVAL;
+
+	phydev = of_phy_connect(dev, fep->fpi->phy_node, &fs_adjust_link, 0,
+				PHY_INTERFACE_MODE_MII);
+	if (!phydev) {
+		phydev = of_phy_connect_fixed_link(dev, &fs_adjust_link,
+						   PHY_INTERFACE_MODE_MII);
 	}
-	if (IS_ERR(phydev)) {
-		printk(KERN_ERR "%s: Could not attach to PHY\n", dev->name);
-		return PTR_ERR(phydev);
+	if (!phydev) {
+		dev_err(&dev->dev, "Could not attach to PHY\n");
+		return -ENODEV;
 	}
 
 	fep->phydev = phydev;
@@ -1005,6 +1004,7 @@ static int __devinit fs_enet_probe(struct of_device *ofdev,
 		goto out_free_fpi;
 	}
 
+	SET_NETDEV_DEV(ndev, &ofdev->dev);
 	dev_set_drvdata(&ofdev->dev, ndev);
 
 	fep = netdev_priv(ndev);

[PATCH v2 3/4] gianfar: Revive fixed link support

[Grant Likely]

From: Anton Vorontsov <avorontsov@ru.mvista.com>

Since commit fe192a49118f5b1272317d60c7930ece4e13ae49 ("Rework gianfar
driver to use of_mdio infrastructure") the fixed-link support is
broken, the driver oopses at init_phy():

  Unable to handle kernel paging request for data at address 0x000000e4
  Faulting instruction address: 0xc01cf298
  Oops: Kernel access of bad area, sig: 11 [#1]
  [...]
  NIP [c01cf298] init_phy+0x80/0xdc
  LR [c01cf250] init_phy+0x38/0xdc
  Call Trace:
  [cf81fe80] [c01d1cf8] gfar_enet_open+0x6c/0x19c
  [cf81fea0] [c024494c] dev_open+0xfc/0x134
  [cf81fec0] [c0242edc] dev_change_flags+0x84/0x1ac
  [cf81fee0] [c0399ee0] ic_open_devs+0x168/0x2d8
  [cf81ff20] [c039b2e8] ip_auto_config+0x90/0x2a4
  [cf81ff60] [c0003884] do_one_initcall+0x34/0x1a8

This patch fixes the oops, and removes phy_node checks, and adds a call
to of_phy_connect_fixed_link() if a phy isn't attached..

Also, remove an old fixed-link code that we don't use any longer.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 drivers/net/gianfar.c |   24 ++++++++----------------
 1 files changed, 8 insertions(+), 16 deletions(-)


diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 4ae1d25..79fcc2c 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -262,15 +262,6 @@ static int gfar_of_init(struct net_device *dev)
 		priv->device_flags |= FSL_GIANFAR_DEV_HAS_MAGIC_PACKET;
 
 	priv->phy_node = of_parse_phandle(np, "phy-handle", 0);
-	if (!priv->phy_node) {
-		u32 *fixed_link;
-
-		fixed_link = (u32 *)of_get_property(np, "fixed-link", NULL);
-		if (!fixed_link) {
-			err = -ENODEV;
-			goto err_out;
-		}
-	}
 
 	/* Find the TBI PHY.  If it's not there, we don't support SGMII */
 	priv->tbi_node = of_parse_phandle(np, "tbi-handle", 0);
@@ -657,13 +648,14 @@ static int init_phy(struct net_device *dev)
 
 	interface = gfar_get_interface(dev);
 
-	if (priv->phy_node) {
-		priv->phydev = of_phy_connect(dev, priv->phy_node, &adjust_link,
-					      0, interface);
-		if (!priv->phydev) {
-			dev_err(&dev->dev, "error: Could not attach to PHY\n");
-			return -ENODEV;
-		}
+	priv->phydev = of_phy_connect(dev, priv->phy_node, &adjust_link, 0,
+				      interface);
+	if (!priv->phydev)
+		priv->phydev = of_phy_connect_fixed_link(dev, &adjust_link,
+							 interface);
+	if (!priv->phydev) {
+		dev_err(&dev->dev, "could not attach to PHY\n");
+		return -ENODEV;
 	}
 
 	if (interface == PHY_INTERFACE_MODE_SGMII)

[PATCH v2 4/4] ucc_geth: Revive fixed link support

[Grant Likely]

From: Anton Vorontsov <avorontsov@ru.mvista.com>

Since commit 0b9da337dca972e7a4144e298ec3adb8f244d4a4 ("Rework
ucc_geth driver to use of_mdio infrastructure") the fixed-link
support is broken.

This patch fixes the support by removing !ug_info->phy_node check,
and adds a call to of_phy_connect_fixed_link() if a phy is not attached
to the MAC.

Also, remove an old fixed-link code that we don't use any longer.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 drivers/net/ucc_geth.c |   23 +++++++----------------
 1 files changed, 7 insertions(+), 16 deletions(-)


diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
index 40c6eba..3b957e6 100644
--- a/drivers/net/ucc_geth.c
+++ b/drivers/net/ucc_geth.c
@@ -1590,13 +1590,13 @@ static int init_phy(struct net_device *dev)
 	priv->oldspeed = 0;
 	priv->oldduplex = -1;
 
-	if (!ug_info->phy_node)
-		return 0;
-
 	phydev = of_phy_connect(dev, ug_info->phy_node, &adjust_link, 0,
 				priv->phy_interface);
+	if (!phydev)
+		phydev = of_phy_connect_fixed_link(dev, &adjust_link,
+						   priv->phy_interface);
 	if (!phydev) {
-		printk("%s: Could not attach to PHY\n", dev->name);
+		dev_err(&dev->dev, "Could not attach to PHY\n");
 		return -ENODEV;
 	}
 
@@ -3608,9 +3608,7 @@ static int ucc_geth_probe(struct of_device* ofdev, const struct of_device_id *ma
 	struct ucc_geth_private *ugeth = NULL;
 	struct ucc_geth_info *ug_info;
 	struct resource res;
-	struct device_node *phy;
 	int err, ucc_num, max_speed = 0;
-	const u32 *fixed_link;
 	const unsigned int *prop;
 	const char *sprop;
 	const void *mac_addr;
@@ -3708,15 +3706,8 @@ static int ucc_geth_probe(struct of_device* ofdev, const struct of_device_id *ma
 
 	ug_info->uf_info.regs = res.start;
 	ug_info->uf_info.irq = irq_of_parse_and_map(np, 0);
-	fixed_link = of_get_property(np, "fixed-link", NULL);
-	if (fixed_link) {
-		phy = NULL;
-	} else {
-		phy = of_parse_phandle(np, "phy-handle", 0);
-		if (phy == NULL)
-			return -ENODEV;
-	}
-	ug_info->phy_node = phy;
+
+	ug_info->phy_node = of_parse_phandle(np, "phy-handle", 0);
 
 	/* Find the TBI PHY node.  If it's not there, we don't support SGMII */
 	ug_info->tbi_node = of_parse_phandle(np, "tbi-handle", 0);
@@ -3725,7 +3716,7 @@ static int ucc_geth_probe(struct of_device* ofdev, const struct of_device_id *ma
 	prop = of_get_property(np, "phy-connection-type", NULL);
 	if (!prop) {
 		/* handle interface property present in old trees */
-		prop = of_get_property(phy, "interface", NULL);
+		prop = of_get_property(ug_info->phy_node, "interface", NULL);
 		if (prop != NULL) {
 			phy_interface = enet_to_phy_interface[*prop];
 			max_speed = enet_to_speed[*prop];

Re: [PATCH v2 0/4] net: Revive fixed link support

[Anton Vorontsov]

On Fri, Jul 17, 2009 at 01:31:25AM -0600, Grant Likely wrote:
[...]
> Part of the problem I think is that the phylib code merges two separate
> constructs; the construct of an MDIO bus (on which many device may
> reside, not all of them PHYs), and the construct of an MII link whose
> speed and configuration need to be manipulated.  I've run into problems
> myself on how best to handle things like Ethernet switches which
> definitely do not behave like PHYs and the phylib state machine cannot
> be used on them.  It seems to me that the whole 'dummy phy' approach
> is just an artifact of the phylib model not being quite right yet.

Yep. With a bit of phylib rework we can remove all the MDIO emulation
stuff from phy/fixed.c driver, and leave there just speed/duplex/pause
assignments.

Though, I still believe that we should avoid two code paths in the
drivers. One of the code paths will be constantly broken if we do so.

> I
> want to investigate the possibility of separating the two concepts, but
> that will require a fair bit of thought and experimentation.

That would be great indeed.

[...]
> Anton, once again I don't have hardware to test this, so I rely on you
> to tell be if I screwed it up.  It has been compile tested.

Works fine here, thanks!

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

Re: [PATCH v2 0/4] net: Revive fixed link support

[Grant Likely]

On Sat, Jul 18, 2009 at 12:04 PM, Anton
Vorontsov<avorontsov@ru.mvista.com> wrote:
> On Fri, Jul 17, 2009 at 01:31:25AM -0600, Grant Likely wrote:
> [...]
>> Part of the problem I think is that the phylib code merges two separate
>> constructs; the construct of an MDIO bus (on which many device may
>> reside, not all of them PHYs), and the construct of an MII link whose
>> speed and configuration need to be manipulated. =A0I've run into problem=
s
>> myself on how best to handle things like Ethernet switches which
>> definitely do not behave like PHYs and the phylib state machine cannot
>> be used on them. =A0It seems to me that the whole 'dummy phy' approach
>> is just an artifact of the phylib model not being quite right yet.
>
> Yep. With a bit of phylib rework we can remove all the MDIO emulation
> stuff from phy/fixed.c driver, and leave there just speed/duplex/pause
> assignments.
>
> Though, I still believe that we should avoid two code paths in the
> drivers. One of the code paths will be constantly broken if we do so.

Yes, I agree.  Splitting the concepts also has the added advantage
that non-phy devices will have an interface to manipulate the link
speed without modifying drivers.

>> Anton, once again I don't have hardware to test this, so I rely on you
>> to tell be if I screwed it up. =A0It has been compile tested.
>
> Works fine here, thanks!

Awesome.  Dave, can you please pick up this series?

Thanks,
g.

--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH v2 0/4] net: Revive fixed link support

[David Miller]

From: Grant Likely <grant.likely@secretlab.ca>
Date: Sat, 18 Jul 2009 12:37:34 -0600

> Awesome.  Dave, can you please pick up this series?

Sure thing.