[PATCH 0/5] Retrieving Ethernet PHY wireup from the OF device tree

[PATCH 0/5] Retrieving Ethernet PHY wireup from the OF device tree

[Grant Likely]

Hi all,

This series reworks some of the phylib code to allow PHY descriptions and
connections to be extracted from the OF device tree.  MDIO bus drivers gain
a common helper function for parsing the PHY data and registering new
phy_devices to match.  Ethernet controller drivers gain the ability to
resolve to a phy_device from a device tree phandle.

One notable aspect is that the Ethernet controller driver doesn't know
if the phy_device is registered before or after the Ethernet driver.  This
series adds a function to the device model core code to make it simple for
the driver to register a bus notifier (mdio_bus in this case), which gets
called both for existing devices and for future device registrations.  The
advantage of this is that the driver doesn't need to know or care when the
device actually shows up.  It just knows that its callback will get called
when the device is available.  I think this is a good approach, but I'd
appreciate some feedback on it.

Cheers,
g.

drivers/base/bus.c            |   47 +++++++++
 drivers/net/Kconfig           |    2 +-
 drivers/net/fec_mpc52xx.c     |  220 ++++++++++++++++++-----------------------
 drivers/net/fec_mpc52xx_phy.c |   30 +++---
 drivers/net/phy/mdio_bus.c    |   29 +-----
 drivers/net/phy/phy_device.c  |  161 +++++++++++++++++++++++-------
 drivers/of/Kconfig            |    6 +
 drivers/of/Makefile           |    1 +
 drivers/of/of_mdio.c          |   70 +++++++++++++
 include/linux/device.h        |    2 +
 include/linux/of_mdio.h       |   20 ++++
 include/linux/phy.h           |    6 +
 12 files changed, 388 insertions(+), 206 deletions(-)

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

[PATCH 1/5] drivers/base: Add bus_register_notifier_alldev() variant

[Grant Likely]

From: Grant Likely <grant.likely@secretlab.ca>

bus_register_notifier_alldev() is a variation on bus_register_notifier()
which also triggers the notifier callback for devices already on the bus
and already bound to drivers.

This function is useful for the case where a driver needs to get a
reference to a struct device other than the one it is bound to and
it is not known if the device will be bound before or after this
function is called.  For example, an Ethernet device connected to
a PHY that is probed separately.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
CC: linux-kernel@vger.kernel.org
CC: linuxppc-dev@ozlabs.org
CC: Greg Kroah-Hartman <gregkh@suse.de>
---

 drivers/base/bus.c     |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/device.h |    2 ++
 2 files changed, 49 insertions(+), 0 deletions(-)


diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 83f32b8..6edde85 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -962,6 +962,53 @@ int bus_register_notifier(struct bus_type *bus, struct notifier_block *nb)
 }
 EXPORT_SYMBOL_GPL(bus_register_notifier);
 
+/**
+ * bus_register_notifier_alldev_helper - internal support function
+ * Used by bus_register_notifier_alldev() to create ADD and BOUND events
+ * for devices.
+ */
+static int bus_register_notifier_alldev_helper(struct device *dev, void *data)
+{
+	struct notifier_block *nb = data;
+	nb->notifier_call(nb, BUS_NOTIFY_ADD_DEVICE, dev);
+	if (dev->driver)
+		nb->notifier_call(nb, BUS_NOTIFY_BOUND_DRIVER, dev);
+	return 0;
+}
+
+/**
+ * bus_register_notifier_alldev - Register for bus events; include existing devs
+ * @bus: pointer to bus_type
+ * @nb: pointer to notifier block to register with the bus
+ *
+ * Similar to bus_register_notifier() except it also generates events for
+ * devices already on the bus when the notifier is registered.  When this
+ * function is called the notifier is called once for each device with
+ * the BUS_NOTIFY_ADD_DEVICE event, and once for each device registered to
+ * a driver * with the BUS_NOTIFY_BOUND_DRIVER event.
+ *
+ * There is a small chance that the notifier could be called more than once
+ * for a device.  This would happen if a new device was registered on the bus
+ * or bound to a driver between the call to bus_register_notifier() and the
+ * call to bus_for_each_dev().  The only way I can see to protect against
+ * this would be to take the klist_devices spinlock while calling the
+ * notifier; but that would be a Very Bad Thing (tm).  Caller needs to be
+ * aware that a notifier called before this function returns might get
+ * called a second time on the same device.
+ */
+int bus_register_notifier_alldev(struct bus_type *b, struct notifier_block *nb)
+{
+	int ret;
+
+	ret = bus_register_notifier(b, nb);
+	if (ret == 0) {
+		bus_for_each_dev(b, NULL, nb,
+				 bus_register_notifier_alldev_helper);
+	}
+	return ret;
+}
+EXPORT_SYMBOL_GPL(bus_register_notifier_alldev);
+
 int bus_unregister_notifier(struct bus_type *bus, struct notifier_block *nb)
 {
 	return blocking_notifier_chain_unregister(&bus->p->bus_notifier, nb);
diff --git a/include/linux/device.h b/include/linux/device.h
index 47f343c..95a7d2b 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -103,6 +103,8 @@ struct notifier_block;
 
 extern int bus_register_notifier(struct bus_type *bus,
 				 struct notifier_block *nb);
+extern int bus_register_notifier_alldev(struct bus_type *b,
+					struct notifier_block *nb);
 extern int bus_unregister_notifier(struct bus_type *bus,
 				   struct notifier_block *nb);
 

[PATCH 2/5] phylib: rework to prepare for OF registration of PHYs

[Grant Likely]

From: Grant Likely <grant.likely@secretlab.ca>

This patch makes changes in preparation for supporting open firmware
device tree descriptions of MDIO busses.  Changes include:
- Cleanup handling of phy_map[] entries; they are already NULLed when
  registering and so don't need to be re-cleared, and it is good practice
  to clear them out when unregistering.
- Split phy_device registration out into a new function so that the
  OF helpers can do two stage registration (separate allocation and
  registration steps).

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
CC: linuxppc-dev@ozlabs.org
CC: netdev@vger.kernel.org
CC: Andy Fleming <afleming@freescale.com>
---

 drivers/net/phy/mdio_bus.c   |   29 +++-------------------------
 drivers/net/phy/phy_device.c |   43 ++++++++++++++++++++++++++++++++++++++----
 include/linux/phy.h          |    1 +
 3 files changed, 43 insertions(+), 30 deletions(-)


diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 811a637..3c39c7b 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -112,7 +112,6 @@ int mdiobus_register(struct mii_bus *bus)
 		bus->reset(bus);
 
 	for (i = 0; i < PHY_MAX_ADDR; i++) {
-		bus->phy_map[i] = NULL;
 		if ((bus->phy_mask & (1 << i)) == 0) {
 			struct phy_device *phydev;
 
@@ -149,6 +148,7 @@ void mdiobus_unregister(struct mii_bus *bus)
 	for (i = 0; i < PHY_MAX_ADDR; i++) {
 		if (bus->phy_map[i])
 			device_unregister(&bus->phy_map[i]->dev);
+		bus->phy_map[i] = NULL;
 	}
 }
 EXPORT_SYMBOL(mdiobus_unregister);
@@ -187,35 +187,12 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr)
 	if (IS_ERR(phydev) || phydev == NULL)
 		return phydev;
 
-	/* There's a PHY at this address
-	 * We need to set:
-	 * 1) IRQ
-	 * 2) bus_id
-	 * 3) parent
-	 * 4) bus
-	 * 5) mii_bus
-	 * And, we need to register it */
-
-	phydev->irq = bus->irq != NULL ? bus->irq[addr] : PHY_POLL;
-
-	phydev->dev.parent = bus->parent;
-	phydev->dev.bus = &mdio_bus_type;
-	dev_set_name(&phydev->dev, PHY_ID_FMT, bus->id, addr);
-
-	phydev->bus = bus;
-
-	/* Run all of the fixups for this PHY */
-	phy_scan_fixups(phydev);
-
-	err = device_register(&phydev->dev);
+	err = phy_device_register(phydev);
 	if (err) {
-		printk(KERN_ERR "phy %d failed to register\n", addr);
 		phy_device_free(phydev);
-		phydev = NULL;
+		return NULL;
 	}
 
-	bus->phy_map[addr] = phydev;
-
 	return phydev;
 }
 EXPORT_SYMBOL(mdiobus_scan);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 0a06e4f..793332f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -39,10 +39,6 @@ MODULE_DESCRIPTION("PHY library");
 MODULE_AUTHOR("Andy Fleming");
 MODULE_LICENSE("GPL");
 
-static struct phy_driver genphy_driver;
-extern int mdio_bus_init(void);
-extern void mdio_bus_exit(void);
-
 void phy_device_free(struct phy_device *phydev)
 {
 	kfree(phydev);
@@ -53,6 +49,10 @@ static void phy_device_release(struct device *dev)
 	phy_device_free(to_phy_device(dev));
 }
 
+static struct phy_driver genphy_driver;
+extern int mdio_bus_init(void);
+extern void mdio_bus_exit(void);
+
 static LIST_HEAD(phy_fixup_list);
 static DEFINE_MUTEX(phy_fixup_lock);
 
@@ -166,6 +166,10 @@ struct phy_device* phy_device_create(struct mii_bus *bus, int addr, int phy_id)
 	dev->addr = addr;
 	dev->phy_id = phy_id;
 	dev->bus = bus;
+	dev->dev.parent = bus->parent;
+	dev->dev.bus = &mdio_bus_type;
+	dev->irq = bus->irq != NULL ? bus->irq[addr] : PHY_POLL;
+	dev_set_name(&dev->dev, PHY_ID_FMT, bus->id, addr);
 
 	dev->state = PHY_DOWN;
 
@@ -237,6 +241,37 @@ struct phy_device * get_phy_device(struct mii_bus *bus, int addr)
 }
 
 /**
+ * phy_device_register - Register the phy device on the MDIO bus
+ * @phy_device: phy_device structure to be added to the MDIO bus
+ */
+int phy_device_register(struct phy_device *phydev)
+{
+	int err;
+
+	/* Don't register a phy if one is already registered at this
+	 * address */
+	if (phydev->bus->phy_map[phydev->addr])
+		return -EINVAL;
+	phydev->bus->phy_map[phydev->addr] = phydev;
+
+	/* Run all of the fixups for this PHY */
+	phy_scan_fixups(phydev);
+
+	err = device_register(&phydev->dev);
+	if (err) {
+		pr_err("phy %d failed to register\n", phydev->addr);
+		goto out;
+	}
+
+	return 0;
+
+ out:
+	phydev->bus->phy_map[phydev->addr] = NULL;
+	return err;
+}
+EXPORT_SYMBOL(phy_device_register);
+
+/**
  * phy_prepare_link - prepares the PHY layer to monitor link status
  * @phydev: target phy_device struct
  * @handler: callback function for link status change notifications
diff --git a/include/linux/phy.h b/include/linux/phy.h
index d7e54d9..a47d64f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -439,6 +439,7 @@ static inline int phy_write(struct phy_device *phydev, u16 regnum, u16 val)
 
 int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id);
 struct phy_device* get_phy_device(struct mii_bus *bus, int addr);
+int phy_device_register(struct phy_device *phy);
 int phy_clear_interrupt(struct phy_device *phydev);
 int phy_config_interrupt(struct phy_device *phydev, u32 interrupts);
 struct phy_device * phy_attach(struct net_device *dev,

[PATCH 3/5] phylib: add *_direct() variants of phy_connect and phy_attach functions

[Grant Likely]

From: Grant Likely <grant.likely@secretlab.ca>

Add phy_connect_direct() and phy_attach_direct() functions so that
drivers can use a pointer to the phy_device instead of trying to determine
the phy's bus_id string.

This patch is useful for OF device tree descriptions of phy devices where
the driver doesn't need or know what the bus_id value in order to get a
phy_device pointer.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 drivers/net/phy/phy_device.c |  118 ++++++++++++++++++++++++++++++------------
 include/linux/phy.h          |    5 ++
 2 files changed, 90 insertions(+), 33 deletions(-)


diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 793332f..238d21e 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -290,6 +290,33 @@ void phy_prepare_link(struct phy_device *phydev,
 }
 
 /**
+ * phy_connect_direct - connect an ethernet device to a specific phy_device
+ * @dev: the network device to connect
+ * @phydev: the pointer to the phy device
+ * @handler: callback function for state change notifications
+ * @flags: PHY device's dev_flags
+ * @interface: PHY device's interface
+ */
+int phy_connect_direct(struct net_device *dev, struct phy_device *phydev,
+		       void (*handler)(struct net_device *), u32 flags,
+		       phy_interface_t interface)
+{
+	int rc;
+
+	rc = phy_attach_direct(dev, phydev, flags, interface);
+	if (rc)
+		return rc;
+
+	phy_prepare_link(phydev, handler);
+	phy_start_machine(phydev, NULL);
+	if (phydev->irq > 0)
+		phy_start_interrupts(phydev);
+
+	return 0;
+}
+EXPORT_SYMBOL(phy_connect_direct);
+
+/**
  * phy_connect - connect an ethernet device to a PHY device
  * @dev: the network device to connect
  * @bus_id: the id string of the PHY device to connect
@@ -310,18 +337,21 @@ struct phy_device * phy_connect(struct net_device *dev, const char *bus_id,
 		phy_interface_t interface)
 {
 	struct phy_device *phydev;
+	struct device *d;
+	int rc;
 
-	phydev = phy_attach(dev, bus_id, flags, interface);
-
-	if (IS_ERR(phydev))
-		return phydev;
-
-	phy_prepare_link(phydev, handler);
-
-	phy_start_machine(phydev, NULL);
+	/* Search the list of PHY devices on the mdio bus for the
+	 * PHY with the requested name */
+	d = bus_find_device_by_name(&mdio_bus_type, NULL, bus_id);
+	if (!d) {
+		pr_err("PHY %s not found\n", bus_id);
+		return ERR_PTR(-ENODEV);
+	}
+	phydev = to_phy_device(d);
 
-	if (phydev->irq > 0)
-		phy_start_interrupts(phydev);
+	rc = phy_attach_direct(dev, phydev, flags, interface);
+	if (rc)
+		return ERR_PTR(rc);
 
 	return phydev;
 }
@@ -345,9 +375,9 @@ void phy_disconnect(struct phy_device *phydev)
 EXPORT_SYMBOL(phy_disconnect);
 
 /**
- * phy_attach - attach a network device to a particular PHY device
+ * phy_attach_direct - attach a network device to a given PHY device pointer
  * @dev: network device to attach
- * @bus_id: PHY device to attach
+ * @phydev: Pointer to phy_device to attach
  * @flags: PHY device's dev_flags
  * @interface: PHY device's interface
  *
@@ -358,22 +388,10 @@ EXPORT_SYMBOL(phy_disconnect);
  *     the attaching device, and given a callback for link status
  *     change.  The phy_device is returned to the attaching driver.
  */
-struct phy_device *phy_attach(struct net_device *dev,
-		const char *bus_id, u32 flags, phy_interface_t interface)
+int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
+		      u32 flags, phy_interface_t interface)
 {
-	struct bus_type *bus = &mdio_bus_type;
-	struct phy_device *phydev;
-	struct device *d;
-
-	/* Search the list of PHY devices on the mdio bus for the
-	 * PHY with the requested name */
-	d = bus_find_device_by_name(bus, NULL, bus_id);
-	if (d) {
-		phydev = to_phy_device(d);
-	} else {
-		printk(KERN_ERR "%s not found\n", bus_id);
-		return ERR_PTR(-ENODEV);
-	}
+	struct device *d = &phydev->dev;
 
 	/* Assume that if there is no driver, that it doesn't
 	 * exist, and we should use the genphy driver. */
@@ -386,13 +404,12 @@ struct phy_device *phy_attach(struct net_device *dev,
 			err = device_bind_driver(d);
 
 		if (err)
-			return ERR_PTR(err);
+			return err;
 	}
 
 	if (phydev->attached_dev) {
-		printk(KERN_ERR "%s: %s already attached\n",
-				dev->name, bus_id);
-		return ERR_PTR(-EBUSY);
+		dev_err(&dev->dev, "PHY already attached\n");
+		return -EBUSY;
 	}
 
 	phydev->attached_dev = dev;
@@ -410,13 +427,48 @@ struct phy_device *phy_attach(struct net_device *dev,
 		err = phy_scan_fixups(phydev);
 
 		if (err < 0)
-			return ERR_PTR(err);
+			return err;
 
 		err = phydev->drv->config_init(phydev);
 
 		if (err < 0)
-			return ERR_PTR(err);
+			return err;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(phy_attach_direct);
+
+/**
+ * phy_attach - attach a network device to a particular PHY device
+ * @dev: network device to attach
+ * @bus_id: Bus ID of PHY device to attach
+ * @flags: PHY device's dev_flags
+ * @interface: PHY device's interface
+ *
+ * Description: Same as phy_attach_direct() except that a PHY bus_id
+ *     string is passed instead of a pointer to a struct phy_device.
+ */
+struct phy_device *phy_attach(struct net_device *dev,
+		const char *bus_id, u32 flags, phy_interface_t interface)
+{
+	struct bus_type *bus = &mdio_bus_type;
+	struct phy_device *phydev;
+	struct device *d;
+	int rc;
+
+	/* Search the list of PHY devices on the mdio bus for the
+	 * PHY with the requested name */
+	d = bus_find_device_by_name(bus, NULL, bus_id);
+	if (!d) {
+		pr_err("PHY %s not found\n", bus_id);
+		return ERR_PTR(-ENODEV);
 	}
+	phydev = to_phy_device(d);
+
+	rc = phy_attach_direct(dev, phydev, flags, interface);
+	if (rc)
+		return ERR_PTR(rc);
 
 	return phydev;
 }
diff --git a/include/linux/phy.h b/include/linux/phy.h
index a47d64f..97405f2 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -442,8 +442,13 @@ struct phy_device* get_phy_device(struct mii_bus *bus, int addr);
 int phy_device_register(struct phy_device *phy);
 int phy_clear_interrupt(struct phy_device *phydev);
 int phy_config_interrupt(struct phy_device *phydev, u32 interrupts);
+int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
+		u32 flags, phy_interface_t interface);
 struct phy_device * phy_attach(struct net_device *dev,
 		const char *bus_id, u32 flags, phy_interface_t interface);
+int phy_connect_direct(struct net_device *dev, struct phy_device *phydev,
+		void (*handler)(struct net_device *), u32 flags,
+		phy_interface_t interface);
 struct phy_device * phy_connect(struct net_device *dev, const char *bus_id,
 		void (*handler)(struct net_device *), u32 flags,
 		phy_interface_t interface);

[PATCH 4/5] openfirmware: Add OF phylib support code

[Grant Likely]

From: Grant Likely <grant.likely@secretlab.ca>

Add support for parsing the device tree for PHY devices on an MDIO bus

CC: Andy Fleming <afleming@freescale.com>
CC: linuxppc-dev@ozlabs.org
CC: devtree-discuss@ozlabs.org

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

 drivers/of/Kconfig      |    6 ++++
 drivers/of/Makefile     |    1 +
 drivers/of/of_mdio.c    |   70 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/of_mdio.h |   20 +++++++++++++
 4 files changed, 97 insertions(+), 0 deletions(-)
 create mode 100644 drivers/of/of_mdio.c
 create mode 100644 include/linux/of_mdio.h


diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index f821dbc..6fe043b 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -19,3 +19,9 @@ config OF_SPI
 	depends on OF && PPC_OF && SPI
 	help
 	  OpenFirmware SPI accessors
+
+config OF_MDIO
+	def_tristate PHYLIB
+	depends on OF && PHYLIB
+	help
+	  OpenFirmware MDIO bus (Ethernet PHY) accessors
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index 4c3c6f8..bdfb5f5 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -3,3 +3,4 @@ obj-$(CONFIG_OF_DEVICE) += device.o platform.o
 obj-$(CONFIG_OF_GPIO)   += gpio.o
 obj-$(CONFIG_OF_I2C)	+= of_i2c.o
 obj-$(CONFIG_OF_SPI)	+= of_spi.o
+obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
new file mode 100644
index 0000000..6a7d4b2
--- /dev/null
+++ b/drivers/of/of_mdio.c
@@ -0,0 +1,70 @@
+/*
+ * OF helpers for the MDIO (Ethernet PHY) API
+ *
+ * Copyright (c) 2009 Secret Lab Technologies, Ltd.
+ *
+ * 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
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This file provides helper functions for extracting PHY device information
+ * out of the OpenFirmware device tree and using it to populate an mii_bus.
+ */
+
+#include <linux/phy.h>
+#include <linux/of.h>
+#include <linux/of_mdio.h>
+#include <linux/module.h>
+
+void of_register_phy_devices(struct mii_bus *mdio, struct device_node *np)
+{
+	struct phy_device *phy;
+	struct device_node *child;
+	int rc;
+
+	for_each_child_of_node(np, child) {
+		const u32 *addr;
+		int len;
+
+		addr = of_get_property(child, "reg", &len);
+		if (!addr || len < sizeof(*addr) || *addr >= 32 || *addr < 0) {
+			dev_err(&mdio->dev, "%s has invalid PHY address\n",
+				child->full_name);
+			continue;
+		}
+
+		if (mdio->irq) {
+			mdio->irq[*addr] = irq_of_parse_and_map(child, 0);
+			if (!mdio->irq[*addr])
+				mdio->irq[*addr] = PHY_POLL;
+		}
+
+		phy = get_phy_device(mdio, *addr);
+		if (!phy) {
+			dev_err(&mdio->dev, "error probing PHY at address %i\n",
+				*addr);
+			continue;
+		}
+		phy_scan_fixups(phy);
+
+		/* Associate the OF node with the device structure so it
+		 * can be looked up later */
+		of_node_get(child);
+		dev_archdata_set_node(&phy->dev.archdata, child);
+
+		/* All data is now stored in the phy struct; register it */
+		rc = phy_device_register(phy);
+		if (rc) {
+			phy_device_free(phy);
+			of_node_put(child);
+			continue;
+		}
+
+		dev_dbg(&mdio->dev, "registered phy %s at address %i\n",
+			child->name, *addr);
+	}
+}
+EXPORT_SYMBOL(of_register_phy_devices);
+
+MODULE_LICENSE("GPL");
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
new file mode 100644
index 0000000..9ea9745
--- /dev/null
+++ b/include/linux/of_mdio.h
@@ -0,0 +1,20 @@
+/*
+ * OF helpers for the MDIO (Ethernet PHY) API
+ *
+ * Copyright (c) 2009 Secret Lab Technologies, Ltd.
+ *
+ * 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
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#ifndef __LINUX_OF_MDIO_H
+#define __LINUX_OF_MDIO_H
+
+#include <linux/phy.h>
+#include <linux/of.h>
+
+void of_register_phy_devices(struct mii_bus *mdio, struct device_node *np);
+
+#endif /* __LINUX_OF_MDIO_H */

[PATCH 5/5] net: make mpc5200 fec driver use of_mdio infrastructure

[Grant Likely]

From: Grant Likely <grant.likely@secretlab.ca>

The patch reworks the MPC5200 Fast Ethernet Controller (FEC) driver to
use the of_mdio infrastructure for registering PHY devices from data out
openfirmware device tree, and eliminates the assumption that the PHY
for the FEC is always attached to the FEC's own MDIO bus.  With this
patch, the FEC can use a PHY attached to any MDIO bus if it is described
in the device tree.
---

 drivers/net/Kconfig           |    2 
 drivers/net/fec_mpc52xx.c     |  220 ++++++++++++++++++-----------------------
 drivers/net/fec_mpc52xx_phy.c |   30 ++----
 3 files changed, 109 insertions(+), 143 deletions(-)


diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 6bdfd47..5b74a9a 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -1854,7 +1854,7 @@ config FEC_MPC52xx
 
 config FEC_MPC52xx_MDIO
 	bool "MPC52xx FEC MDIO bus driver"
-	depends on FEC_MPC52xx
+	depends on FEC_MPC52xx && OF_MDIO
 	default y
 	---help---
 	  The MPC5200's FEC can connect to the Ethernet either with
diff --git a/drivers/net/fec_mpc52xx.c b/drivers/net/fec_mpc52xx.c
index 049b0a7..4efcd47 100644
--- a/drivers/net/fec_mpc52xx.c
+++ b/drivers/net/fec_mpc52xx.c
@@ -43,11 +43,9 @@
 
 #define DRIVER_NAME "mpc52xx-fec"
 
-#define FEC5200_PHYADDR_NONE	(-1)
-#define FEC5200_PHYADDR_7WIRE	(-2)
-
 /* Private driver data structure */
 struct mpc52xx_fec_priv {
+	struct net_device *ndev;
 	int duplex;
 	int speed;
 	int r_irq;
@@ -59,10 +57,12 @@ struct mpc52xx_fec_priv {
 	int msg_enable;
 
 	/* MDIO link details */
-	int phy_addr;
-	unsigned int phy_speed;
+	struct notifier_block notifier;
+	unsigned int mdio_speed;
+	struct device_node *phy_node;
 	struct phy_device *phydev;
 	enum phy_state link;
+	int seven_wire_mode;
 };
 
 
@@ -210,66 +210,6 @@ static void mpc52xx_fec_adjust_link(struct net_device *dev)
 		phy_print_status(phydev);
 }
 
-static int mpc52xx_fec_init_phy(struct net_device *dev)
-{
-	struct mpc52xx_fec_priv *priv = netdev_priv(dev);
-	struct phy_device *phydev;
-	char phy_id[BUS_ID_SIZE];
-
-	snprintf(phy_id, sizeof(phy_id), "%x:%02x",
-			(unsigned int)dev->base_addr, priv->phy_addr);
-
-	priv->link = PHY_DOWN;
-	priv->speed = 0;
-	priv->duplex = -1;
-
-	phydev = phy_connect(dev, phy_id, &mpc52xx_fec_adjust_link, 0, PHY_INTERFACE_MODE_MII);
-	if (IS_ERR(phydev)) {
-		dev_err(&dev->dev, "phy_connect failed\n");
-		return PTR_ERR(phydev);
-	}
-	dev_info(&dev->dev, "attached phy %i to driver %s\n",
-			phydev->addr, phydev->drv->name);
-
-	priv->phydev = phydev;
-
-	return 0;
-}
-
-static int mpc52xx_fec_phy_start(struct net_device *dev)
-{
-	struct mpc52xx_fec_priv *priv = netdev_priv(dev);
-	int err;
-
-	if (priv->phy_addr < 0)
-		return 0;
-
-	err = mpc52xx_fec_init_phy(dev);
-	if (err) {
-		dev_err(&dev->dev, "mpc52xx_fec_init_phy failed\n");
-		return err;
-	}
-
-	/* reset phy - this also wakes it from PDOWN */
-	phy_write(priv->phydev, MII_BMCR, BMCR_RESET);
-	phy_start(priv->phydev);
-
-	return 0;
-}
-
-static void mpc52xx_fec_phy_stop(struct net_device *dev)
-{
-	struct mpc52xx_fec_priv *priv = netdev_priv(dev);
-
-	if (!priv->phydev)
-		return;
-
-	phy_disconnect(priv->phydev);
-	/* power down phy */
-	phy_stop(priv->phydev);
-	phy_write(priv->phydev, MII_BMCR, BMCR_PDOWN);
-}
-
 static int mpc52xx_fec_phy_mii_ioctl(struct mpc52xx_fec_priv *priv,
 		struct mii_ioctl_data *mii_data, int cmd)
 {
@@ -279,16 +219,6 @@ static int mpc52xx_fec_phy_mii_ioctl(struct mpc52xx_fec_priv *priv,
 	return phy_mii_ioctl(priv->phydev, mii_data, cmd);
 }
 
-static void mpc52xx_fec_phy_hw_init(struct mpc52xx_fec_priv *priv)
-{
-	struct mpc52xx_fec __iomem *fec = priv->fec;
-
-	if (priv->phydev)
-		return;
-
-	out_be32(&fec->mii_speed, priv->phy_speed);
-}
-
 static int mpc52xx_fec_open(struct net_device *dev)
 {
 	struct mpc52xx_fec_priv *priv = netdev_priv(dev);
@@ -319,9 +249,14 @@ static int mpc52xx_fec_open(struct net_device *dev)
 		goto free_irqs;
 	}
 
-	err = mpc52xx_fec_phy_start(dev);
-	if (err)
-		goto free_skbs;
+	if (priv->phydev) {
+		/* reset phy - this also wakes it from PDOWN */
+		priv->link = PHY_DOWN;
+		priv->speed = 0;
+		priv->duplex = -1;
+		phy_write(priv->phydev, MII_BMCR, BMCR_RESET);
+		phy_start(priv->phydev);
+	}
 
 	bcom_enable(priv->rx_dmatsk);
 	bcom_enable(priv->tx_dmatsk);
@@ -332,9 +267,6 @@ static int mpc52xx_fec_open(struct net_device *dev)
 
 	return 0;
 
- free_skbs:
-	mpc52xx_fec_free_rx_buffers(dev, priv->rx_dmatsk);
-
  free_irqs:
 	free_irq(priv->t_irq, dev);
  free_2irqs:
@@ -360,7 +292,11 @@ static int mpc52xx_fec_close(struct net_device *dev)
 	free_irq(priv->r_irq, dev);
 	free_irq(priv->t_irq, dev);
 
-	mpc52xx_fec_phy_stop(dev);
+	if (priv->phydev) {
+		/* power down phy */
+		phy_stop(priv->phydev);
+		phy_write(priv->phydev, MII_BMCR, BMCR_PDOWN);
+	}
 
 	return 0;
 }
@@ -700,7 +636,7 @@ static void mpc52xx_fec_hw_init(struct net_device *dev)
 	/* set phy speed.
 	 * this can't be done in phy driver, since it needs to be called
 	 * before fec stuff (even on resume) */
-	mpc52xx_fec_phy_hw_init(priv);
+	out_be32(&fec->mii_speed, priv->mdio_speed);
 }
 
 /**
@@ -736,7 +672,7 @@ static void mpc52xx_fec_start(struct net_device *dev)
 	rcntrl = FEC_RX_BUFFER_SIZE << 16;	/* max frame length */
 	rcntrl |= FEC_RCNTRL_FCE;
 
-	if (priv->phy_addr != FEC5200_PHYADDR_7WIRE)
+	if (!priv->seven_wire_mode)
 		rcntrl |= FEC_RCNTRL_MII_MODE;
 
 	if (priv->duplex == DUPLEX_FULL)
@@ -802,8 +738,6 @@ static void mpc52xx_fec_stop(struct net_device *dev)
 
 	/* Stop FEC */
 	out_be32(&fec->ecntrl, in_be32(&fec->ecntrl) & ~FEC_ECNTRL_ETHER_EN);
-
-	return;
 }
 
 /* reset fec and bestcomm tasks */
@@ -821,9 +755,11 @@ static void mpc52xx_fec_reset(struct net_device *dev)
 
 	mpc52xx_fec_hw_init(dev);
 
-	phy_stop(priv->phydev);
-	phy_write(priv->phydev, MII_BMCR, BMCR_RESET);
-	phy_start(priv->phydev);
+	if (priv->phydev) {
+		phy_stop(priv->phydev);
+		phy_write(priv->phydev, MII_BMCR, BMCR_RESET);
+		phy_start(priv->phydev);
+	}
 
 	bcom_fec_rx_reset(priv->rx_dmatsk);
 	bcom_fec_tx_reset(priv->tx_dmatsk);
@@ -888,6 +824,48 @@ static int mpc52xx_fec_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 /* ======================================================================== */
 /* OF Driver                                                                */
 /* ======================================================================== */
+static int mpc52xx_fec_notifier_phy_add(struct notifier_block *nb,
+					unsigned long event, void *_dev)
+{
+	struct device *dev = _dev;
+	struct mpc52xx_fec_priv *priv;
+	unsigned long flags;
+	int rc;
+
+	priv = container_of(nb, struct mpc52xx_fec_priv, notifier);
+
+	/* Check if this is the phy that we're interested in */
+	if ((event != BUS_NOTIFY_ADD_DEVICE) ||
+	    (dev_archdata_get_node(&dev->archdata) != priv->phy_node))
+		return 0;
+
+	spin_lock_irqsave(&priv->lock, flags);
+	if (priv->phydev) {
+		spin_unlock_irqrestore(&priv->lock, flags);
+		return 0;
+	}
+	priv->phydev = container_of(dev, struct phy_device, dev);
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	rc = bus_unregister_notifier(&mdio_bus_type, &priv->notifier);
+	if (rc)
+		dev_warn(dev, "bus_unregister_notifier() failed\n");
+
+	rc = phy_connect_direct(priv->ndev, priv->phydev,
+				mpc52xx_fec_adjust_link, 0, 0);
+	if (rc) {
+		dev_err(dev, "phy_connect_direct() failed\n");
+		return 0;
+	}
+
+	rc = register_netdev(priv->ndev);
+	if (rc) {
+		phy_disconnect(priv->phydev);
+		dev_err(dev, "register_netdev() failed\n");
+	}
+
+	return 0;
+}
 
 static int __devinit
 mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *match)
@@ -896,7 +874,6 @@ mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *match)
 	struct net_device *ndev;
 	struct mpc52xx_fec_priv *priv = NULL;
 	struct resource mem;
-	struct device_node *phy_node;
 	const phandle *phy_handle;
 	const u32 *prop;
 	int prop_size;
@@ -910,6 +887,7 @@ mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *match)
 		return -ENOMEM;
 
 	priv = netdev_priv(ndev);
+	priv->ndev = ndev;
 
 	/* Reserve FEC control zone */
 	rv = of_address_to_resource(op->node, 0, &mem);
@@ -943,6 +921,8 @@ mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *match)
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	ndev->poll_controller = mpc52xx_fec_poll_controller;
 #endif
+	SET_NETDEV_DEV(ndev, &op->dev);
+
 
 	priv->t_irq = priv->r_irq = ndev->irq = NO_IRQ; /* IRQ are free for now */
 
@@ -992,14 +972,9 @@ mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *match)
 	 */
 
 	/* Start with safe defaults for link connection */
-	priv->phy_addr = FEC5200_PHYADDR_NONE;
 	priv->speed = 100;
 	priv->duplex = DUPLEX_HALF;
-	priv->phy_speed = ((mpc52xx_find_ipb_freq(op->node) >> 20) / 5) << 1;
-
-	/* the 7-wire property means don't use MII mode */
-	if (of_find_property(op->node, "fsl,7-wire-mode", NULL))
-		priv->phy_addr = FEC5200_PHYADDR_7WIRE;
+	priv->mdio_speed = ((mpc52xx_find_ipb_freq(op->node) >> 20) / 5) << 1;
 
 	/* The current speed preconfigures the speed of the MII link */
 	prop = of_get_property(op->node, "current-speed", &prop_size);
@@ -1009,40 +984,37 @@ mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *match)
 	}
 
 	/* If there is a phy handle, setup link to that phy */
+	priv->notifier.notifier_call = mpc52xx_fec_notifier_phy_add;
 	phy_handle = of_get_property(op->node, "phy-handle", &prop_size);
-	if (phy_handle && (prop_size >= sizeof(phandle))) {
-		phy_node = of_find_node_by_phandle(*phy_handle);
-		prop = of_get_property(phy_node, "reg", &prop_size);
-		if (prop && (prop_size >= sizeof(u32)))
-			if ((*prop >= 0) && (*prop < PHY_MAX_ADDR))
-				priv->phy_addr = *prop;
-		of_node_put(phy_node);
+	if (phy_handle && (prop_size >= sizeof(phandle)))
+		priv->phy_node = of_find_node_by_phandle(*phy_handle);
+	if (priv->phy_node)
+		bus_register_notifier_alldev(&mdio_bus_type, &priv->notifier);
+
+	/* the 7-wire property means don't use MII mode */
+	if (of_find_property(op->node, "fsl,7-wire-mode", NULL)) {
+		if (priv->phy_node)
+			dev_err(&op->dev, "warning: 'fsl,7-wire-mode' is"
+				" illegal when 'phy-handle' is present\n");
+		else
+			priv->seven_wire_mode = 1;
 	}
 
 	/* Hardware init */
 	mpc52xx_fec_hw_init(ndev);
-
 	mpc52xx_fec_reset_stats(ndev);
 
-	SET_NETDEV_DEV(ndev, &op->dev);
-
-	/* Register the new network device */
-	rv = register_netdev(ndev);
-	if (rv < 0)
-		goto probe_error;
-
-	/* Now report the link setup */
-	switch (priv->phy_addr) {
-	 case FEC5200_PHYADDR_NONE:
-		dev_info(&ndev->dev, "Fixed speed MII link: %i%cD\n",
-			 priv->speed, priv->duplex ? 'F' : 'H');
-		break;
-	 case FEC5200_PHYADDR_7WIRE:
-		dev_info(&ndev->dev, "using 7-wire PHY mode\n");
-		break;
-	 default:
-		dev_info(&ndev->dev, "Using PHY at MDIO address %i\n",
-			 priv->phy_addr);
+	/* Register the new network device immediately if we don't need
+	 * to wait for a phy_device first. */
+	if (!priv->phy_node) {
+		if (priv->seven_wire_mode)
+			dev_info(&ndev->dev, "using 7-wire PHY mode\n");
+		else
+			dev_info(&ndev->dev, "Fixed speed MII link: %i%cD\n",
+				 priv->speed, priv->duplex ? 'F' : 'H');
+		rv = register_netdev(ndev);
+		if (rv < 0)
+			goto probe_error;
 	}
 
 	/* We're done ! */
@@ -1050,10 +1022,8 @@ mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *match)
 
 	return 0;
 
-
 	/* Error handling - free everything that might be allocated */
 probe_error:
-
 	irq_dispose_mapping(ndev->irq);
 
 	if (priv->rx_dmatsk)
diff --git a/drivers/net/fec_mpc52xx_phy.c b/drivers/net/fec_mpc52xx_phy.c
index dd9bfa4..261a5e7 100644
--- a/drivers/net/fec_mpc52xx_phy.c
+++ b/drivers/net/fec_mpc52xx_phy.c
@@ -14,12 +14,14 @@
 #include <linux/netdevice.h>
 #include <linux/phy.h>
 #include <linux/of_platform.h>
+#include <linux/of_mdio.h>
 #include <asm/io.h>
 #include <asm/mpc52xx.h>
 #include "fec_mpc52xx.h"
 
 struct mpc52xx_fec_mdio_priv {
 	struct mpc52xx_fec __iomem *regs;
+	int mdio_irqs[PHY_MAX_ADDR];
 };
 
 static int mpc52xx_fec_mdio_transfer(struct mii_bus *bus, int phy_id,
@@ -27,7 +29,7 @@ static int mpc52xx_fec_mdio_transfer(struct mii_bus *bus, int phy_id,
 {
 	struct mpc52xx_fec_mdio_priv *priv = bus->priv;
 	struct mpc52xx_fec __iomem *fec;
-	int tries = 100;
+	int tries = 3;
 
 	value |= (phy_id << FEC_MII_DATA_PA_SHIFT) & FEC_MII_DATA_PA_MSK;
 	value |= (reg << FEC_MII_DATA_RA_SHIFT) & FEC_MII_DATA_RA_MSK;
@@ -38,7 +40,7 @@ static int mpc52xx_fec_mdio_transfer(struct mii_bus *bus, int phy_id,
 
 	/* wait for it to finish, this takes about 23 us on lite5200b */
 	while (!(in_be32(&fec->ievent) & FEC_IEVENT_MII) && --tries)
-		udelay(5);
+		msleep(1);
 
 	if (!tries)
 		return -ETIMEDOUT;
@@ -64,7 +66,6 @@ static int mpc52xx_fec_mdio_probe(struct of_device *of,
 {
 	struct device *dev = &of->dev;
 	struct device_node *np = of->node;
-	struct device_node *child = NULL;
 	struct mii_bus *bus;
 	struct mpc52xx_fec_mdio_priv *priv;
 	struct resource res = {};
@@ -85,23 +86,10 @@ static int mpc52xx_fec_mdio_probe(struct of_device *of,
 	bus->write = mpc52xx_fec_mdio_write;
 
 	/* setup irqs */
-	bus->irq = kmalloc(sizeof(bus->irq[0]) * PHY_MAX_ADDR, GFP_KERNEL);
-	if (bus->irq == NULL) {
-		err = -ENOMEM;
-		goto out_free;
-	}
+	bus->irq = priv->mdio_irqs;
 	for (i=0; i<PHY_MAX_ADDR; i++)
 		bus->irq[i] = PHY_POLL;
 
-	while ((child = of_get_next_child(np, child)) != NULL) {
-		int irq = irq_of_parse_and_map(child, 0);
-		if (irq != NO_IRQ) {
-			const u32 *id = of_get_property(child, "reg", NULL);
-			if (id)
-				bus->irq[*id] = irq;
-		}
-	}
-
 	/* setup registers */
 	err = of_address_to_resource(np, 0, &res);
 	if (err)
@@ -122,10 +110,18 @@ static int mpc52xx_fec_mdio_probe(struct of_device *of,
 	out_be32(&priv->regs->mii_speed,
 		((mpc52xx_find_ipb_freq(of->node) >> 20) / 5) << 1);
 
+	/* Mask out all from auto probing.  Instead the PHYs listed in
+	 * device tree are populated after the bus has been registered
+	 */
+	bus->phy_mask = -1;
+
 	err = mdiobus_register(bus);
 	if (err)
 		goto out_unmap;
 
+	/* Populate the PHY devices from the device tree */
+	of_register_phy_devices(bus, np);
+
 	return 0;
 
  out_unmap:

Re: [PATCH 5/5] net: make mpc5200 fec driver use of_mdio infrastructure

[Anton Vorontsov]

On Tue, Mar 10, 2009 at 09:22:24AM -0600, Grant Likely wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
[...]
> +static int mpc52xx_fec_notifier_phy_add(struct notifier_block *nb,
> +					unsigned long event, void *_dev)
> +{
[...]
> +	rc = phy_connect_direct(priv->ndev, priv->phydev,
> +				mpc52xx_fec_adjust_link, 0, 0);
> +	if (rc) {
> +		dev_err(dev, "phy_connect_direct() failed\n");
> +		return 0;
> +	}
> +
> +	rc = register_netdev(priv->ndev);
> +	if (rc) {
> +		phy_disconnect(priv->phydev);
> +		dev_err(dev, "register_netdev() failed\n");
> +	}
> +
> +	return 0;
> +}
[...]
>  static int __devinit
>  mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *match)
> @@ -896,7 +874,6 @@ mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *match)
[...]
> +	/* Register the new network device immediately if we don't need
> +	 * to wait for a phy_device first. */
> +	if (!priv->phy_node) {
> +		if (priv->seven_wire_mode)
> +			dev_info(&ndev->dev, "using 7-wire PHY mode\n");
> +		else
> +			dev_info(&ndev->dev, "Fixed speed MII link: %i%cD\n",
> +				 priv->speed, priv->duplex ? 'F' : 'H');
> +		rv = register_netdev(ndev);
> +		if (rv < 0)
> +			goto probe_error;
>  	}
[...]

Two registration points for the netdev... That's ugly. :-/

What problem are you trying to solve w/ these patches, btw?

`ifconfig ethX up` is safe even w/o PHY attached.

All the (user-visible) changes is that we no longer have "ethX"
until PHY is registered, and I can't say that this is good either.

Previously you'd have ethX all the time, and `ifconfig ethX up`
would report user-friendly "PHY not attached" error. Now we have
to guess why ethX isn't there.

I can't say that the probing code is much prettier or easier to
understand... But maybe there are some other problems that you're
solving, which I don't see so far?

That is, can you explain why the changes are needed? Did you
consider other solutions?


Thanks!

p.s.
> eliminates the assumption that the PHY for the FEC is always
> attached to the FEC's own MDIO bus. With this patch, the FEC can
> use a PHY attached to any MDIO bus if it is described in the device
> tree.

AFAIK, Gianfar and UCC Geth drivers can do this too, so I'm assuming
that this isn't the cause for these major changes.

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

Re: [PATCH 5/5] net: make mpc5200 fec driver use of_mdio infrastructure

[Grant Likely]

On Tue, Mar 10, 2009 at 1:16 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Tue, Mar 10, 2009 at 09:22:24AM -0600, Grant Likely wrote:
>> From: Grant Likely <grant.likely@secretlab.ca>
> [...]
>> +static int mpc52xx_fec_notifier_phy_add(struct notifier_block *nb,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0 unsigned long event, void *_dev)
>> +{
> [...]
>> + =A0 =A0 rc =3D phy_connect_direct(priv->ndev, priv->phydev,
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 mpc52xx_fec_ad=
just_link, 0, 0);
>> + =A0 =A0 if (rc) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 dev_err(dev, "phy_connect_direct() failed\n");
>> + =A0 =A0 =A0 =A0 =A0 =A0 return 0;
>> + =A0 =A0 }
>> +
>> + =A0 =A0 rc =3D register_netdev(priv->ndev);
>> + =A0 =A0 if (rc) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 phy_disconnect(priv->phydev);
>> + =A0 =A0 =A0 =A0 =A0 =A0 dev_err(dev, "register_netdev() failed\n");
>> + =A0 =A0 }
>> +
>> + =A0 =A0 return 0;
>> +}
> [...]
>> =A0static int __devinit
>> =A0mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *ma=
tch)
>> @@ -896,7 +874,6 @@ mpc52xx_fec_probe(struct of_device *op, const struct=
 of_device_id *match)
> [...]
>> + =A0 =A0 /* Register the new network device immediately if we don't nee=
d
>> + =A0 =A0 =A0* to wait for a phy_device first. */
>> + =A0 =A0 if (!priv->phy_node) {
>> + =A0 =A0 =A0 =A0 =A0 =A0 if (priv->seven_wire_mode)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_info(&ndev->dev, "using 7-=
wire PHY mode\n");
>> + =A0 =A0 =A0 =A0 =A0 =A0 else
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_info(&ndev->dev, "Fixed sp=
eed MII link: %i%cD\n",
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0priv->speed=
, priv->duplex ? 'F' : 'H');
>> + =A0 =A0 =A0 =A0 =A0 =A0 rv =3D register_netdev(ndev);
>> + =A0 =A0 =A0 =A0 =A0 =A0 if (rv < 0)
>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto probe_error;
>> =A0 =A0 =A0 }
> [...]
>
> Two registration points for the netdev... That's ugly. :-/
>
> What problem are you trying to solve w/ these patches, btw?
>
> `ifconfig ethX up` is safe even w/o PHY attached.
>
> All the (user-visible) changes is that we no longer have "ethX"
> until PHY is registered, and I can't say that this is good either.

Fair enough.  If it is okay to register the PHY after registering the
netdev, then I have no problem with it.  I'll change the patch.

> I can't say that the probing code is much prettier or easier to
> understand... But maybe there are some other problems that you're
> solving, which I don't see so far?

Primary problem is that this driver currently does not work for a PHY
on a different MDIO bus.  Secondary is that current code depends on
phys being registered before the FEC.

> That is, can you explain why the changes are needed? Did you
> consider other solutions?

Yes, I considered doing some kind of platform function to call and get
the name of the PHY, but any such thing turned out to be fragile and
rather platform specific.  The bus notifier approach seemed to be the
simplest way to defer part of initialization while waiting for the PHY
to become available.  I want to be using common code here as I've got
another Ethernet driver (ll_temac; not posted yet) that needs to do
the same thing.

>> eliminates the assumption that the PHY for the FEC is always
>> attached to the FEC's own MDIO bus. With this patch, the FEC can
>> use a PHY attached to any MDIO bus if it is described in the device
>> tree.
>
> AFAIK, Gianfar and UCC Geth drivers can do this too, so I'm assuming
> that this isn't the cause for these major changes.

Certainly the mpc5200-fec driver's original phy code certainly wasn't
as robust as the ucc_geth and gianfar phy handling.

ucc_geth open codes a solution to decode the phy_device name from
several nodes in the device tree and doesn't handle the case where the
ucc_geth is initialized before the phy_device is registered.  gianfar
open codes the same thing.  This solution uses common code to locate
the phy_device, and it works regardless of what order devices are
registered in.

That being said, the 5200 driver originally using probe() time to
connect to the phy.  If I change it to be connected at open time, then
does the registration order issue become irrelevant?  Regardless, I
think all the drivers should be using common code for obtaining the
phy_device from the device tree.

g.

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

Re: [PATCH 5/5] net: make mpc5200 fec driver use of_mdio infrastructure

[Anton Vorontsov]

On Tue, Mar 10, 2009 at 01:48:26PM -0600, Grant Likely wrote:
[...]
> >> eliminates the assumption that the PHY for the FEC is always
> >> attached to the FEC's own MDIO bus. With this patch, the FEC can
> >> use a PHY attached to any MDIO bus if it is described in the device
> >> tree.
> >
> > AFAIK, Gianfar and UCC Geth drivers can do this too, so I'm assuming
> > that this isn't the cause for these major changes.
> 
> Certainly the mpc5200-fec driver's original phy code certainly wasn't
> as robust as the ucc_geth and gianfar phy handling.
> 
> ucc_geth open codes a solution to decode the phy_device name from
> several nodes in the device tree and doesn't handle the case where the
> ucc_geth is initialized before the phy_device is registered.  gianfar
> open codes the same thing.  This solution uses common code to locate
> the phy_device, and it works regardless of what order devices are
> registered in.
> 
> That being said, the 5200 driver originally using probe() time to
> connect to the phy.  If I change it to be connected at open time, then
> does the registration order issue become irrelevant?

Yup. `ifconfig ethX up' calls ->open(). If it fails (and prints
nice error), you can try `ifconfig ethX up' again later.

> Regardless, I
> think all the drivers should be using common code for obtaining the
> phy_device from the device tree.

Not necessary `struct phy_device'. All we need is some common
routine for translating PHY's "mdio_node->full_name + phy id" to
phy_bus_id.

That is, you can just factor out this code from the gianfar driver,

void gfar_mdio_bus_name(char *name, struct device_node *np)
{
        const u32 *reg;

        reg = of_get_property(np, "reg", NULL);

        snprintf(name, MII_BUS_ID_SIZE, "%s@%x", np->name, reg ? *reg : 0);
}

...
        gfar_mdio_bus_name(bus_name, mdio);
        snprintf(priv->phy_bus_id, sizeof(priv->phy_bus_id), "%s:%02x",
                 bus_name, *id);
...

And make sure FEC MDIO driver does mdio_bus->parent = &ofdev->dev;

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

Re: [PATCH 5/5] net: make mpc5200 fec driver use of_mdio infrastructure

[Grant Likely]

On Tue, Mar 10, 2009 at 2:29 PM, Anton Vorontsov
<avorontsov@ru.mvista.com> wrote:
> On Tue, Mar 10, 2009 at 01:48:26PM -0600, Grant Likely wrote:
>> Regardless, I
>> think all the drivers should be using common code for obtaining the
>> phy_device from the device tree.
>
> Not necessary `struct phy_device'. All we need is some common
> routine for translating PHY's "mdio_node->full_name + phy id" to
> phy_bus_id.

This only works if the network driver knows how the MDIO bus is named.
 The current code assumes that the MDIO bus name is the register
address, but this is a driver implementation detail and MDIO bus
drivers can deviate from this.  What about MDIO busses that don't have
a reg property?  For example, fs_enet/mii-bitbang.c sets the bus name
to "CPM2 Bitbanged MII".  There is no one-size-fits-all way to figure
out the phy bus id from the ethernet driver side of things.

The sure and simple way to guarantee a match between the PHY device
node and the phy_device is to use the PHY device_node pointer itself
as the search key.

I concede that my first attempt at this was overly complex, but I've
reworked the code and I think it makes thinks considerably simpler.
I'll post a new series tomorrow.  I've got patches to make the
ucc_eth, gianfar and fs_enet drivers to use the device_node method
also.

g.

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