[PATCH 0/4 for 2.6.31] NET: Revive fixed link support

[PATCH 0/4 for 2.6.31] NET: Revive fixed link support

[Anton Vorontsov]

Hi all,

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.

Patches 2 and 3 are for the drivers that I tested in run-time,
with normal PHYs, and fixed links.

Patch #4 was build-tested, but I believe it should work in
run-time too.


Thanks,

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

[PATCH 1/4] of/mdio: Add fixed link support

[Anton Vorontsov]

Currently the fixed link support is broken for all OF ethernet drivers,
an "OF MDIO rework" removed most of the support. Instead of re-adding
fixed-link stuff to the drivers, add the support to a framework, so we
won't duplicate any code.

With this patch, if a node pointer is NULL, then of_phy_connect() will
try to find ethernet device's node, then will look for fixed-link
property, and if specified, it connects PHY as usual, via bus_id (fixed
link PHYs do not have any device tree nodes associated with them).

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/of/of_mdio.c |   45 +++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index aee967d..cfd876a 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>
@@ -129,11 +133,44 @@ struct phy_device *of_phy_connect(struct net_device *dev,
 				  void (*hndlr)(struct net_device *), u32 flags,
 				  phy_interface_t iface)
 {
-	struct phy_device *phy = of_phy_find_device(phy_np);
+	struct phy_device *phy = NULL;
+
+	if (phy_np) {
+		int ret;
+
+		phy = of_phy_find_device(phy_np);
+		if (!phy)
+			return NULL;
+
+		ret = phy_connect_direct(dev, phy, hndlr, flags, iface);
+		if (ret)
+			return NULL;
+	} else if (dev->dev.parent) {
+		struct device_node *net_np;
+		const u32 *phy_id;
+		char *bus_id;
+		int sz;
+
+		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;
+
+		bus_id = kasprintf(GFP_KERNEL, PHY_ID_FMT, "0", phy_id[0]);
+		if (!bus_id) {
+			dev_err(&dev->dev, "could not allocate memory\n");
+			return NULL;
+		}
 
-	if (!phy)
-		return NULL;
+		phy = phy_connect(dev, bus_id, hndlr, 0, iface);
+		kfree(bus_id);
+		if (IS_ERR(phy))
+			return NULL;
+	}
 
-	return phy_connect_direct(dev, phy, hndlr, flags, iface) ? NULL : phy;
+	return phy;
 }
 EXPORT_SYMBOL(of_phy_connect);
-- 
1.6.3.1

Re: [PATCH 1/4] of/mdio: Add fixed link support

[Grant Likely]

On Fri, Jun 26, 2009 at 4:29 PM, Anton
Vorontsov<avorontsov@ru.mvista.com> wrote:
> Currently the fixed link support is broken for all OF ethernet drivers,
> an "OF MDIO rework" removed most of the support. Instead of re-adding
> fixed-link stuff to the drivers, add the support to a framework, so we
> won't duplicate any code.
>
> With this patch, if a node pointer is NULL, then of_phy_connect() will
> try to find ethernet device's node, then will look for fixed-link
> property, and if specified, it connects PHY as usual, via bus_id (fixed
> link PHYs do not have any device tree nodes associated with them).
>
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

Ugh.  I do not like this approach.  I did not intend to break fixed
links, but I do not think that this approach is the right fix.  There
are several problems.

I don't like the fixed.c approach of creating a dummy phy to begin
with.  I think it is an abuse of the device model to register a dummy
mii_bus and have dummy devices registered on it.  It is a lot of code
for what should be a very simple thing.  In particular, if a PHY is
not specified, then the driver should use a static link configuration.
 This is trivial to implement in an Ethernet driver and I do not think
the dummy phy adds anything.

It hooks into the initialization path of *all* OF enabled net drivers,
whether it wants it or not.  ie. The MPC5200 FEC driver does not want
it because the fixed-link property is not part of the mpc5200-fec
binding; it uses a current-speed property instead.  'fixed-link' has
not been agreed upon to be applicable to all Ethernet bindings, and
I'm not convinced that the format of it won't need to be changed for
future Ethernet bindings.  A function for parsing fixed-link should be
a library function that a driver can choose to call out to.  It should
not be welded into the init path.

I also think parsing the device tree at device open time (when
of_phy_connect is usually called) is best to be avoided.  fixed-link
parsing should really happen at probe time and the values cached IMHO.
 It's probably not significant, but I'd like to keep device tree reads
constrained in the cold path (driver probe time) as opposed to the hot
(or slightly less cool) device open path.

Instead, I think that each driver should be more graceful about
missing phy pointers and the init path should call out to a fixed-link
parser function that sets the initial link settings.  Probably less
than 5 lines of code per driver.

I'm sorry about breaking it.  It was my fault, and I'd be happy to fix
it if you'd like me to, but I don't think that this patch is the right
approach.

g.

> ---
> =A0drivers/of/of_mdio.c | =A0 45 ++++++++++++++++++++++++++++++++++++++++=
+----
> =A01 files changed, 41 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index aee967d..cfd876a 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -9,6 +9,10 @@
> =A0* out of the OpenFirmware device tree and using it to populate an mii_=
bus.
> =A0*/
>
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/netdevice.h>
> +#include <linux/err.h>
> =A0#include <linux/phy.h>
> =A0#include <linux/of.h>
> =A0#include <linux/of_mdio.h>
> @@ -129,11 +133,44 @@ struct phy_device *of_phy_connect(struct net_device=
 *dev,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0void (=
*hndlr)(struct net_device *), u32 flags,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0phy_in=
terface_t iface)
> =A0{
> - =A0 =A0 =A0 struct phy_device *phy =3D of_phy_find_device(phy_np);
> + =A0 =A0 =A0 struct phy_device *phy =3D NULL;
> +
> + =A0 =A0 =A0 if (phy_np) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 int ret;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 phy =3D of_phy_find_device(phy_np);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!phy)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return NULL;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D phy_connect_direct(dev, phy, hndlr,=
 flags, iface);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ret)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return NULL;
> + =A0 =A0 =A0 } else if (dev->dev.parent) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct device_node *net_np;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 const u32 *phy_id;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 char *bus_id;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 int sz;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 net_np =3D dev_archdata_get_node(&dev->dev.=
parent->archdata);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!net_np)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return NULL;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 phy_id =3D of_get_property(net_np, "fixed-l=
ink", &sz);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!phy_id || sz < sizeof(*phy_id))
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return NULL;
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 bus_id =3D kasprintf(GFP_KERNEL, PHY_ID_FMT=
, "0", phy_id[0]);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!bus_id) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&dev->dev, "could n=
ot allocate memory\n");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return NULL;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>
> - =A0 =A0 =A0 if (!phy)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return NULL;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 phy =3D phy_connect(dev, bus_id, hndlr, 0, =
iface);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 kfree(bus_id);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (IS_ERR(phy))
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return NULL;
> + =A0 =A0 =A0 }
>
> - =A0 =A0 =A0 return phy_connect_direct(dev, phy, hndlr, flags, iface) ? =
NULL : phy;
> + =A0 =A0 =A0 return phy;
> =A0}
> =A0EXPORT_SYMBOL(of_phy_connect);
> --
> 1.6.3.1
>
>



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

Re: [PATCH 1/4] of/mdio: Add fixed link support

[Anton Vorontsov]

On Fri, Jun 26, 2009 at 05:33:26PM -0600, Grant Likely wrote:
> On Fri, Jun 26, 2009 at 4:29 PM, Anton
> Vorontsov<avorontsov@ru.mvista.com> wrote:
> > Currently the fixed link support is broken for all OF ethernet drivers,
> > an "OF MDIO rework" removed most of the support. Instead of re-adding
> > fixed-link stuff to the drivers, add the support to a framework, so we
> > won't duplicate any code.
> >
> > With this patch, if a node pointer is NULL, then of_phy_connect() will
> > try to find ethernet device's node, then will look for fixed-link
> > property, and if specified, it connects PHY as usual, via bus_id (fixed
> > link PHYs do not have any device tree nodes associated with them).
> >
> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> 
> Ugh.  I do not like this approach.  I did not intend to break fixed
> links, but I do not think that this approach is the right fix.  There
> are several problems.
> 
> I don't like the fixed.c approach of creating a dummy phy to begin
> with.  I think it is an abuse of the device model to register a dummy
> mii_bus and have dummy devices registered on it.  It is a lot of code
> for what should be a very simple thing.  In particular, if a PHY is
> not specified, then the driver should use a static link configuration.
>  This is trivial to implement in an Ethernet driver and I do not think
> the dummy phy adds anything.

Dummy PHYs add more than you think, for example you'll have to
refactor or duplicate the code that is responsible for MAC settings
for a given mode (10/100/100, duplex, pause), and you'll have to
do netif_carrier_* handling yourself.

Not a problem per se, but you'll have to address this, and you'll
have two paths in the drivers.

> It hooks into the initialization path of *all* OF enabled net drivers,
> whether it wants it or not.  ie. The MPC5200 FEC driver does not want
> it because the fixed-link property is not part of the mpc5200-fec
> binding; it uses a current-speed property instead.  'fixed-link' has
> not been agreed upon to be applicable to all Ethernet bindings, and
> I'm not convinced that the format of it won't need to be changed for
> future Ethernet bindings.  A function for parsing fixed-link should be
> a library function that a driver can choose to call out to.  It should
> not be welded into the init path.
> 
> I also think parsing the device tree at device open time (when
> of_phy_connect is usually called) is best to be avoided.  fixed-link
> parsing should really happen at probe time and the values cached IMHO.
>  It's probably not significant, but I'd like to keep device tree reads
> constrained in the cold path (driver probe time) as opposed to the hot
> (or slightly less cool) device open path.

open() time isn't a hot path at all.

> Instead, I think that each driver should be more graceful about
> missing phy pointers and the init path should call out to a fixed-link
> parser function that sets the initial link settings.  Probably less
> than 5 lines of code per driver.
> 
> I'm sorry about breaking it.  It was my fault, and I'd be happy to fix
> it if you'd like me to,

Please do.

> but I don't think that this patch is the right
> approach.

OK, fine by me if you think you can do this stuff better. :-)

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

[PATCH 2/4] gianfar: Revive fixed link support

[Anton Vorontsov]

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 (of_phy_connect
call now tries to find fixed PHYs if the phy_node is NULL).

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

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

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 4ae1d25..41b2107 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,11 @@ 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) {
+		dev_err(&dev->dev, "could not attach to PHY\n");
+		return -ENODEV;
 	}
 
 	if (interface == PHY_INTERFACE_MODE_SGMII)
-- 
1.6.3.1

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

[Anton Vorontsov]

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,
today the of_phy_connect() will try to find a fixed PHY if the
phy_node appears to be NULL.

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

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/net/ucc_geth.c |   18 +++---------------
 1 files changed, 3 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
index 40c6eba..2dc83b1 100644
--- a/drivers/net/ucc_geth.c
+++ b/drivers/net/ucc_geth.c
@@ -1590,9 +1590,6 @@ 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) {
@@ -3608,9 +3605,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 +3703,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 +3713,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];
-- 
1.6.3.1

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

[Anton Vorontsov]

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, today
of_phy_connect() tries to find fixed PHY if phy_node appears to be
NULL.

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>
---
 drivers/net/fs_enet/fs_enet-main.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c
index b892c3a..1fea39e 100644
--- a/drivers/net/fs_enet/fs_enet-main.c
+++ b/drivers/net/fs_enet/fs_enet-main.c
@@ -754,15 +754,10 @@ 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;
-	}
-	if (IS_ERR(phydev)) {
+
+	phydev = of_phy_connect(dev, fep->fpi->phy_node, &fs_adjust_link, 0,
+				PHY_INTERFACE_MODE_MII);
+	if (!phydev) {
 		printk(KERN_ERR "%s: Could not attach to PHY\n", dev->name);
 		return PTR_ERR(phydev);
 	}
@@ -1005,6 +1000,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);
-- 
1.6.3.1

Re: [PATCH 0/4 for 2.6.31] NET: Revive fixed link support

[Grant Likely]

On Fri, Jun 26, 2009 at 4:29 PM, Anton
Vorontsov<avorontsov@ru.mvista.com> wrote:
> Hi all,
>
> 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.

Apparently I didn't.  sorry.

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

As I described in my previous email, I don't think this is a good
approach and I don't think it should be merged.

g.

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