Re: I2C MDIO support for pasemi_mac driver

Re: I2C MDIO support for pasemi_mac driver

[Olof Johansson]

Hi,

(Adding Andy Fleming and netdev as cc)

On Fri, Mar 07, 2008 at 11:40:43AM -0600, Nathaniel Case wrote:
> Olof,
> 
> I'm working on cleaning up our i2c_mdio driver for submission upstream.
> One issue we're facing is how to uniquely number the MDIO bus ID which
> gets passed to the PHY layer and how the OFW device tree should look.
> 
> For example, pasemi_mac currently gets the PHY address from the "reg"
> property of the ethernet-phy node.  It assigns the MDIO bus ID from the
> "reg" field of the ethernet-phy node's parent.  I'm referring to the two
> components of the phy_id that gets passed to phy_connect().
> 
> As you've mentioned in the past, an I2C PHY node should go underneath
> the appropriate SMBus.  I think we could just use the lower 5 bits of
> the I2C slave address for the PHY address, but the issue of the MDIO bus
> ID is a little more tricky.
> 
> For my case, the parent of the I2C PHY node would be the i2c@1c,2 node,
> so pasemi_mac could no longer just look at "reg" of the parent node.
> Specifically, the i2c@ nodes and the mdio@ nodes don't agree on the
> "reg" property so it can't be used for determining a MDIO bus ID.
> 
> Do you have any suggestions for addressing this?  The only solution I
> can think of is adding a 'mdio-bus' property to both the mdio@ and i2C@
> nodes (if applicable), and then changing pasemi_mac to look for these
> instead.

A good start is probably to check the compatible field of the parent and
see if it's the mdio bus. Looks like the i2c device nodes lack a
compatible field, so we'd need to check device_type there (or fall back
and assume i2c on non-mdio).

For the i2c devices that are right below the controller node it's not
too hard, since we know for sure they are added to the system using
i2c_add_numbered_adapter with the same bus number as the PCI function
number.

But the generic case is tricker, since there's nothing that says the
phy isn't sitting on a (for example) muxed sub-bus instead (a gemeni in
an electra/chitra would be configured like this, but I haven't done any
phy work for them yet).

The i2c bus number should be available in the phy driver when it's
probed/registered, so you should be able to get to it there. It's awkward
that the MDIO and i2c busses have separate namespaces, it'll make it hard
if there's ever a system with both mdio and smbus-based phys. We could
control that by how the mdio bus is specified in device tree on those
systems though, and make sure the bus number for the "real" mdio bus is
sufficiently offset to give room for the smbus-based bus numbers below.

How about something like the below? Completely untested due to lack of
hardware, and needs some cleanup w.r.t. error handling.

Thoughts? Comments? Flames?


-Olof

diff --git a/drivers/net/pasemi_mac.c b/drivers/net/pasemi_mac.c
index c50f0f4..febf1b6 100644
--- a/drivers/net/pasemi_mac.c
+++ b/drivers/net/pasemi_mac.c
@@ -1086,7 +1086,7 @@ static int pasemi_mac_phy_init(struct net_device *dev)
 	struct pasemi_mac *mac = netdev_priv(dev);
 	struct device_node *dn, *phy_dn;
 	struct phy_device *phydev;
-	unsigned int phy_id;
+	unsigned int phy_id, bus_id;
 	const phandle *ph;
 	const unsigned int *prop;
 	struct resource r;
@@ -1099,12 +1099,26 @@ static int pasemi_mac_phy_init(struct net_device *dev)
 	phy_dn = of_find_node_by_phandle(*ph);
 
 	prop = of_get_property(phy_dn, "reg", NULL);
-	ret = of_address_to_resource(phy_dn->parent, 0, &r);
-	if (ret)
-		goto err;
-
 	phy_id = *prop;
-	snprintf(mac->phy_id, BUS_ID_SIZE, PHY_ID_FMT, (int)r.start, phy_id);
+	if (of_device_is_compatible(phy_dn->parent, "gpio-mdio")) {
+		ret = of_address_to_resource(phy_dn->parent, 0, &r);
+		if (ret)
+			goto err;
+
+		bus_id = (int)r.start;
+	} else if (phy_dn->parent->type && !strcmp(phy_dn->parent->type, "i2c")) {
+		struct pci_dn *pdn;
+		/* PHY is on smbus. Right now we only support them directly on root buses,
+		 * and there we know that the bus ID is the same as the PCI function.
+		 */
+		pdn = PCI_DN(phy_dn->parent);
+		bus_id = PCI_FUNC(pdn->devfn);
+	} else {
+		printk("Unknown PHY device in device tree\n");
+		bus_id = -1;
+	}
+
+	snprintf(mac->phy_id, BUS_ID_SIZE, PHY_ID_FMT, bus_id, phy_id);
 
 	of_node_put(phy_dn);
 

Re: [Pasemi-linux] I2C MDIO support for pasemi_mac driver

[Olof Johansson]

On Fri, Mar 07, 2008 at 12:37:38PM -0600, Olof Johansson wrote:

> The i2c bus number should be available in the phy driver when it's
> probed/registered, so you should be able to get to it there. It's awkward
> that the MDIO and i2c busses have separate namespaces, it'll make it hard
> if there's ever a system with both mdio and smbus-based phys. We could
> control that by how the mdio bus is specified in device tree on those
> systems though, and make sure the bus number for the "real" mdio bus is
> sufficiently offset to give room for the smbus-based bus numbers below.

On second thought, it might be a better idea to change from BUS:ID to
BUSTYPE:BUS:ID in phylib, to separate the namespaces.

That, plus a way to get to an i2c bus number from a device tree node,
and we should be all set. That might be tricker though.


-Olof

Re: [Pasemi-linux] I2C MDIO support for pasemi_mac driver

[Nathaniel Case]

On Fri, 2008-03-07 at 12:48 -0600, Olof Johansson wrote:
> On second thought, it might be a better idea to change from BUS:ID to
> BUSTYPE:BUS:ID in phylib, to separate the namespaces.
> 
> That, plus a way to get to an i2c bus number from a device tree node,
> and we should be all set. That might be tricker though.

I like this idea, since it would eliminate the need for the MAC driver
to decide policy on a "global" MDIO bus numbering scheme.  I was also
trying to make it compatible with the case (however unlikely) of having
both an SMBus PHY and a bit-banged MDIO PHY.

For getting the I2C bus number from a device tree node, I've just been
parsing the node->full_name for the number following the i2c@XXXXXX with
an ugly string parsing function (xes_of_get_node_loc() in one of the
older patches I sent you).

- Nate Case <ncase@xes-inc.com>

Re: [Pasemi-linux] I2C MDIO support for pasemi_mac driver

[Nate Case]

On Fri, 2008-03-07 at 12:48 -0600, Olof Johansson wrote:
> On second thought, it might be a better idea to change from BUS:ID to
> BUSTYPE:BUS:ID in phylib, to separate the namespaces.
> 
> That, plus a way to get to an i2c bus number from a device tree node,
> and we should be all set. That might be tricker though.

This turned out to not be as invasive of a change as I thought.  Here's
my first attempt at it.  The changes to pasemi_mac are based on your
first patch but modified accordingly for the new bus type field.

Comments are welcome.  Patch is against 2.6.23 for now (though I
wouldn't expect any of these areas to have changed much since then).
After I get some feedback I can rebase against the latest netdev and do
a formal submission.

- Nate Case <ncase@xes-inc.com>

---
 arch/powerpc/platforms/pasemi/gpio_mdio.c |    1 +
 drivers/net/gianfar.c                     |    3 +-
 drivers/net/pasemi_mac.c                  |   58 ++++++++++++++++++++++++++---
 drivers/net/phy/mdio_bus.c                |    3 +-
 drivers/net/ucc_geth.c                    |    4 +-
 include/linux/phy.h                       |    7 ++-
 6 files changed, 64 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/platforms/pasemi/gpio_mdio.c b/arch/powerpc/platforms/pasemi/gpio_mdio.c
index 098450e..37fa0f5 100644
--- a/arch/powerpc/platforms/pasemi/gpio_mdio.c
+++ b/arch/powerpc/platforms/pasemi/gpio_mdio.c
@@ -239,6 +239,7 @@ static int __devinit gpio_mdio_probe(struct of_device *ofdev,
 	new_bus->read = &gpio_mdio_read;
 	new_bus->write = &gpio_mdio_write;
 	new_bus->reset = &gpio_mdio_reset;
+	new_bus->type = "pas_gpio";
 
 	prop = of_get_property(np, "reg", NULL);
 	new_bus->id = *prop;
diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index f926905..dad8703 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -458,7 +458,8 @@ static int init_phy(struct net_device *dev)
 	priv->oldspeed = 0;
 	priv->oldduplex = -1;
 
-	snprintf(phy_id, BUS_ID_SIZE, PHY_ID_FMT, priv->einfo->bus_id, priv->einfo->phy_id);
+	snprintf(phy_id, BUS_ID_SIZE, PHY_ID_FMT, "gfar", priv->einfo->bus_id,
+			priv->einfo->phy_id);
 
 	interface = gfar_get_interface(dev);
 
diff --git a/drivers/net/pasemi_mac.c b/drivers/net/pasemi_mac.c
index 79d277f..6322215 100644
--- a/drivers/net/pasemi_mac.c
+++ b/drivers/net/pasemi_mac.c
@@ -181,6 +181,37 @@ static int get_skb_hdr(struct sk_buff *skb, void **iphdr,
 	return 0;
 }
 
+/*
+ * Return the "index number" of an OF device node name.  This is for the
+ * node names of the format <device name>@<address>,<index #>
+ * (e.g., "serial@1d,1" or "i2c@1c,2").  If no index exists, 0 is
+ * returned.
+ */
+static int of_get_node_devindex(struct device_node *node)
+{
+	int i, at_index = -1, comma_index = -1;
+
+	i = strlen(node->full_name);
+	while (i > 0) {
+		if (node->full_name[i-1] == '/')
+			return 0;
+
+		if (node->full_name[i-1] == '@') {
+			at_index = i - 1;
+			break;
+		}
+
+		if (node->full_name[i-1] == ',')
+			comma_index = i -1;
+
+		i--;
+	}
+
+	if (at_index == -1 || comma_index == -1)
+		return 0;
+	
+	return simple_strtoul(&(node->full_name[comma_index+1]), NULL, 16);
+}
 
 static int mac_to_intf(const struct pasemi_mac *mac)
 {
@@ -969,9 +1000,10 @@ static int pasemi_mac_phy_init(struct net_device *dev)
 	struct pasemi_mac *mac = netdev_priv(dev);
 	struct device_node *dn, *phy_dn;
 	struct phy_device *phydev;
-	unsigned int phy_id;
+	unsigned int phy_id, bus_id;
 	const phandle *ph;
 	const unsigned int *prop;
+	const char *type;
 	struct resource r;
 	int ret;
 
@@ -982,12 +1014,26 @@ static int pasemi_mac_phy_init(struct net_device *dev)
 	phy_dn = of_find_node_by_phandle(*ph);
 
 	prop = of_get_property(phy_dn, "reg", NULL);
-	ret = of_address_to_resource(phy_dn->parent, 0, &r);
-	if (ret)
-		goto err;
+	phy_id = *prop & (PHY_MAX_ADDR - 1);
+	if (of_device_is_compatible(phy_dn->parent, "gpio-mdio")) {
+		ret = of_address_to_resource(phy_dn->parent, 0, &r);
+		if (ret)
+			goto err;
+
+		bus_id = (int)r.start;
+		type = "pas_gpio";
+	} else if (phy_dn->parent->type && !strcmp(phy_dn->parent->type,
+			"i2c")) {
+		/* PHY is on smbus.  Bus ID matches I2C bus number.*/
+		bus_id = of_get_node_devindex(phy_dn->parent);
+		type = "i2c";
+	} else {
+		printk("Unknown PHY device in device tree\n");
+		bus_id = -1;
+		type = "unknown";
+	}
 
-	phy_id = *prop;
-	snprintf(mac->phy_id, BUS_ID_SIZE, PHY_ID_FMT, (int)r.start, phy_id);
+	snprintf(mac->phy_id, BUS_ID_SIZE, PHY_ID_FMT, type, bus_id, phy_id);
 
 	of_node_put(phy_dn);
 
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index a0a706e..d873c5e 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -84,7 +84,8 @@ int mdiobus_register(struct mii_bus *bus)
 
 			phydev->dev.parent = bus->dev;
 			phydev->dev.bus = &mdio_bus_type;
-			snprintf(phydev->dev.bus_id, BUS_ID_SIZE, PHY_ID_FMT, bus->id, i);
+			snprintf(phydev->dev.bus_id, BUS_ID_SIZE, PHY_ID_FMT,
+					bus->type, bus->id, i);
 
 			phydev->bus = bus;
 
diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
index 9a38dfe..1ea20d8 100644
--- a/drivers/net/ucc_geth.c
+++ b/drivers/net/ucc_geth.c
@@ -1608,8 +1608,8 @@ static int init_phy(struct net_device *dev)
 	priv->oldspeed = 0;
 	priv->oldduplex = -1;
 
-	snprintf(phy_id, BUS_ID_SIZE, PHY_ID_FMT, priv->ug_info->mdio_bus,
-			priv->ug_info->phy_address);
+	snprintf(phy_id, BUS_ID_SIZE, PHY_ID_FMT, "ucc_geth",
+			priv->ug_info->mdio_bus, priv->ug_info->phy_address);
 
 	phydev = phy_connect(dev, phy_id, &adjust_link, 0, priv->phy_interface);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 294f4c5..10c59da 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -68,13 +68,16 @@ typedef enum {
 
 #define PHY_MAX_ADDR	32
 
-/* Used when trying to connect to a specific phy (mii bus id:phy device id) */
-#define PHY_ID_FMT "%x:%02x"
+/*
+ * Used when trying to connect to a specific phy:
+ * (mdio bus type:bus id:phy device id) */
+#define PHY_ID_FMT "%s:%x:%02x"
 
 /* The Bus class for PHYs.  Devices which provide access to
  * PHYs should register using this structure */
 struct mii_bus {
 	const char *name;
+	const char *type;
 	int id;
 	void *priv;
 	int (*read)(struct mii_bus *bus, int phy_id, int regnum);
-- 
1.5.3.3

Re: [Pasemi-linux] I2C MDIO support for pasemi_mac driver

[Andy Fleming]

On Mar 13, 2008, at 16:57, Nate Case wrote:

> On Fri, 2008-03-07 at 12:48 -0600, Olof Johansson wrote:
>> On second thought, it might be a better idea to change from BUS:ID to
>> BUSTYPE:BUS:ID in phylib, to separate the namespaces.
>>
>> That, plus a way to get to an i2c bus number from a device tree node,
>> and we should be all set. That might be tricker though.
>
> This turned out to not be as invasive of a change as I thought.   
> Here's
> my first attempt at it.  The changes to pasemi_mac are based on your
> first patch but modified accordingly for the new bus type field.
>
> Comments are welcome.  Patch is against 2.6.23 for now (though I
> wouldn't expect any of these areas to have changed much since then).
> After I get some feedback I can rebase against the latest netdev and  
> do
> a formal submission.


Sorry, I should have responded to this when it arrived.

My first instinct is that the current bus->id shouldn't be an int at  
all (well, obviously not my *first* instinct, since that's what I did  
the first time through).  So I like the idea of including a label in  
the bus, but I'm not sure if it's necessary to add a new field.   
Instead, I'd like to see bus->id converted to a char * or char [].  I  
played around with this a bit today, and it requires a little more  
work than it might at first seem, but is quite doable.

The question is whether that would solve your problem.  I think you  
would then be able to register buses with the id "pas_gpio:xxx" or  
"i2c:xxx", etc.

Anyway, I'm sending a patch which does that as an RFC in a follow-on  
email.  A part of me thinks adding bus->type might be good, whether  
it's the final solution or not.

Andy

Re: [Pasemi-linux] I2C MDIO support for pasemi_mac driver

[Nate Case]

On Wed, 2008-04-02 at 17:45 -0500, Andy Fleming wrote:
> My first instinct is that the current bus->id shouldn't be an int at  
> all (well, obviously not my *first* instinct, since that's what I did  
> the first time through).  So I like the idea of including a label in  
> the bus, but I'm not sure if it's necessary to add a new field.   
> Instead, I'd like to see bus->id converted to a char * or char [].  I  
> played around with this a bit today, and it requires a little more  
> work than it might at first seem, but is quite doable.
> 
> The question is whether that would solve your problem.  I think you  
> would then be able to register buses with the id "pas_gpio:xxx" or  
> "i2c:xxx", etc.

Thanks for the feedback.  Changing the bus ID to a string would solve my
problem, and I think this is a good way to do it.

- Nate Case <ncase@xes-inc.com>