[PATCH] powerpc: add of_get_mac_address() and update fsl_soc.c to use it

[PATCH] powerpc: add of_get_mac_address() and update fsl_soc.c to use it

[Timur Tabi]

Add function of_get_mac_address(), which obtains the best MAC address to use
from the device tree by checking various properties in order.  The order is: 
'mac-address', then 'local-mac-address', then 'address'.  It also skips
properties that were not initialized by U-Boot.

Update gfar_of_init() and fs_enet_of_init() in fsl_soc.c to call 
of_get_mac_address().

Signed-off-by: Timur Tabi <timur@freescale.com>
---

See the comment for function of_get_mac_address() for a detailed description
of what it does.

After this patch is applied, the next step is to update the various
OF-enabled Ethernet drivers to use of_get_mac_address().

 arch/powerpc/kernel/prom.c    |   45 ++++++++++++++++++++++++++++++++++------
 arch/powerpc/sysdev/fsl_soc.c |   19 ++++++-----------
 include/asm-powerpc/prom.h    |    2 +
 3 files changed, 47 insertions(+), 19 deletions(-)

diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index 3e86e6e..4826f01 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -3,9 +3,9 @@
  *
  * Paul Mackerras	August 1996.
  * Copyright (C) 1996-2005 Paul Mackerras.
- * 
+ *
  *  Adapted for 64bit PowerPC by Dave Engebretsen and Peter Bergner.
- *    {engebret|bergner}@us.ibm.com 
+ *    {engebret|bergner}@us.ibm.com
  *
  *      This program is free software; you can redistribute it and/or
  *      modify it under the terms of the GNU General Public License
@@ -111,7 +111,7 @@ int __init of_scan_flat_dt(int (*it)(uns
 	do {
 		u32 tag = *((u32 *)p);
 		char *pathp;
-		
+
 		p += 4;
 		if (tag == OF_DT_END_NODE) {
 			depth --;
@@ -148,7 +148,7 @@ int __init of_scan_flat_dt(int (*it)(uns
 		}
 		rc = it(p, pathp, depth, data);
 		if (rc != 0)
-			break;		
+			break;
 	} while(1);
 
 	return rc;
@@ -791,7 +791,7 @@ static int __init early_init_dt_scan_roo
 	prop = of_get_flat_dt_prop(node, "#address-cells", NULL);
 	dt_root_addr_cells = (prop == NULL) ? 2 : *prop;
 	DBG("dt_root_addr_cells = %x\n", dt_root_addr_cells);
-	
+
 	/* break now */
 	return 1;
 }
@@ -927,7 +927,7 @@ static void __init early_reserve_mem(voi
 	lmb_reserve(self_base, self_size);
 
 #ifdef CONFIG_PPC32
-	/* 
+	/*
 	 * Handle the case where we might be booting from an old kexec
 	 * image that setup the mem_rsvmap as pairs of 32-bit values
 	 */
@@ -1613,13 +1613,44 @@ const void *get_property(const struct de
 EXPORT_SYMBOL(get_property);
 
 /*
+ * Search the device tree for the best MAC address to use.  'mac-address' is
+ * checked first, because that is supposed to contain to "most recent" MAC
+ * address. If that isn't set, then 'local-mac-address' is checked next,
+ * because that is the default address.  If that isn't set, then the obsolete
+ * 'address' is checked, just in case we're using an old device tree.
+ *
+ * All-zero MAC addresses are rejected, because those could be properties that
+ * exist in the device tree, but were not set by U-Boot.  For example, the
+ * DTS could define 'mac-address' and 'local-mac-address', with zero MAC
+ * addresses.  Some older U-Boots only initialized 'local-mac-address'.  In
+ * this case, the real MAC is in 'local-mac-address', and 'mac-address' exists
+ * but is all zeros.
+*/
+const void *of_get_mac_address(struct device_node *np)
+{
+	static const u8 null_mac_address[6] = { 0 };
+	const void *mac_addr;
+
+	mac_addr = get_property(np, "mac-address", NULL);
+	if (mac_addr && (memcmp(mac_addr, null_mac_address, 6) != 0))
+		return mac_addr;
+
+	mac_addr = get_property(np, "local-mac-address", NULL);
+	if (mac_addr && (memcmp(mac_addr, null_mac_address, 6) != 0))
+		return mac_addr;
+
+	return get_property(np, "address", NULL);
+}
+EXPORT_SYMBOL(of_get_mac_address);
+
+/*
  * Add a property to a node
  */
 int prom_add_property(struct device_node* np, struct property* prop)
 {
 	struct property **next;
 
-	prop->next = NULL;	
+	prop->next = NULL;
 	write_lock(&devtree_lock);
 	next = &np->properties;
 	while (*next) {
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 34161bc..d20f029 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -233,14 +233,7 @@ static int __init gfar_of_init(void)
 			goto err;
 		}
 
-		mac_addr = get_property(np, "local-mac-address", NULL);
-		if (mac_addr == NULL)
-			mac_addr = get_property(np, "mac-address", NULL);
-		if (mac_addr == NULL) {
-			/* Obsolete */
-			mac_addr = get_property(np, "address", NULL);
-		}
-
+		mac_addr = of_get_mac_address(np);
 		if (mac_addr)
 			memcpy(gfar_data.mac_addr, mac_addr, 6);
 
@@ -646,8 +639,9 @@ static int __init fs_enet_of_init(void)
 			goto unreg;
 		}
 
-		mac_addr = get_property(np, "mac-address", NULL);
-		memcpy(fs_enet_data.macaddr, mac_addr, 6);
+		mac_addr = of_get_mac_address(np);
+		if (mac_addr)
+			memcpy(fs_enet_data.macaddr, mac_addr, 6);
 
 		ph = get_property(np, "phy-handle", NULL);
 		phy = of_find_node_by_phandle(*ph);
@@ -931,8 +925,9 @@ static int __init fs_enet_of_init(void)
 			goto err;
 		r[0].name = enet_regs;
 
-		mac_addr = (void *)get_property(np, "mac-address", NULL);
-		memcpy(fs_enet_data.macaddr, mac_addr, 6);
+		mac_addr = of_get_mac_address(np);
+		if (mac_addr)
+			memcpy(fs_enet_data.macaddr, mac_addr, 6);
 
 		ph = (phandle *) get_property(np, "phy-handle", NULL);
 		if (ph != NULL)
diff --git a/include/asm-powerpc/prom.h b/include/asm-powerpc/prom.h
index 0afee17..020ed01 100644
--- a/include/asm-powerpc/prom.h
+++ b/include/asm-powerpc/prom.h
@@ -255,6 +255,8 @@ extern void kdump_move_device_tree(void)
 /* CPU OF node matching */
 struct device_node *of_get_cpu_node(int cpu, unsigned int *thread);
 
+/* Get the MAC address */
+extern const void *of_get_mac_address(struct device_node *np);
 
 /*
  * OF interrupt mapping
-- 
1.4.4

Re: [PATCH] powerpc: add of_get_mac_address() and update fsl_soc.c to use it

[Benjamin Herrenschmidt]

> +	return get_property(np, "address", NULL);

Since there is so much room for confusion with "address", please at
least do some basic sanity checking, like check that it's size is 6
bytes and maybe that the multicast bit isn't set.

Ben.

Re: [PATCH] powerpc: add of_get_mac_address() and update fsl_soc.c to use it

[Olof Johansson]

On Wed, Feb 14, 2007 at 11:00:19AM +1100, Benjamin Herrenschmidt wrote:
> 
> > +	return get_property(np, "address", NULL);
> 
> Since there is so much room for confusion with "address", please at
> least do some basic sanity checking, like check that it's size is 6
> bytes and maybe that the multicast bit isn't set.

No need to reinvent the wheel, is_valid_ether_addr() is easy to use.

Might still want to check the property length though.


-Olof

Re: [PATCH] powerpc: add of_get_mac_address() and update fsl_soc.c to use it

[Timur Tabi]

Benjamin Herrenschmidt wrote:
>> +	return get_property(np, "address", NULL);
> 
> Since there is so much room for confusion with "address", please at
> least do some basic sanity checking, like check that it's size is 6
> bytes and maybe that the multicast bit isn't set.

Are you saying that some device trees have something *other* than a MAC 
address in the 'address' property?

Re: [PATCH] powerpc: add of_get_mac_address() and update fsl_soc.c to use it

[Benjamin Herrenschmidt]

On Tue, 2007-02-13 at 23:17 -0600, Timur Tabi wrote:
> Benjamin Herrenschmidt wrote:
> >> +	return get_property(np, "address", NULL);
> > 
> > Since there is so much room for confusion with "address", please at
> > least do some basic sanity checking, like check that it's size is 6
> > bytes and maybe that the multicast bit isn't set.
> 
> Are you saying that some device trees have something *other* than a MAC 
> address in the 'address' property?

Yes, old Apple gear in some nodes, though I don't think in network
devices...

Ben.

Re: [PATCH] powerpc: add of_get_mac_address() and update fsl_soc.c to use it

[Timur Tabi]

Benjamin Herrenschmidt wrote:

>> Are you saying that some device trees have something *other* than a MAC 
>> address in the 'address' property?
> 
> Yes, old Apple gear in some nodes, though I don't think in network
> devices...

Ok, then I will respin the patch with some more error checking.  Do you 
want me to just check the 'address' property, or all of the properties?

Re: [PATCH] powerpc: add of_get_mac_address() and update fsl_soc.c to use it

[Sergei Shtylyov]

Hello.

Timur Tabi wrote:

>>> +    return get_property(np, "address", NULL);

>> Since there is so much room for confusion with "address", please at
>> least do some basic sanity checking, like check that it's size is 6
>> bytes and maybe that the multicast bit isn't set.

> Are you saying that some device trees have something *other* than a MAC 
> address in the 'address' property?

    'address' property was originally defined to hold a virtual address of the 
device registers (with 'reg' holding the physical address) until FSL decided 
to "reinvent" the property. :-)

WBR, Sergei

Re: [PATCH] powerpc: add of_get_mac_address() and update fsl_soc.c to use it

[Segher Boessenkool]

>> +	return get_property(np, "address", NULL);
>
> Since there is so much room for confusion with "address", please at
> least do some basic sanity checking, like check that it's size is 6
> bytes and maybe that the multicast bit isn't set.

Preferably, don't use "address" at all -- there shouldn't
be any trees in the wild that use it.  If there actually
are some, the should use a quirk to change it to "local-
mac-address" instead.

This holds in general: workarounds for gross errors in
device trees should be applied much more selectively
than is done now, it just doesn't scale this way.


Segher

Re: [PATCH] powerpc: add of_get_mac_address() and update fsl_soc.c to use it

[Timur Tabi]

Segher Boessenkool wrote:
>>> +    return get_property(np, "address", NULL);
>>
>> Since there is so much room for confusion with "address", please at
>> least do some basic sanity checking, like check that it's size is 6
>> bytes and maybe that the multicast bit isn't set.
> 
> Preferably, don't use "address" at all -- there shouldn't

Unfortunately, I really don't have that luxury.  I *must* support 'address'.

> be any trees in the wild that use it.  If there actually
> are some, the should use a quirk to change it to "local-
> mac-address" instead.

The whole point behind checking for 'address' is to support older device trees 
that have only that property.  We have a problem where some installations use 
'address', and if we don't support it here in the kernel, then the kernel won't 
boot.

> This holds in general: workarounds for gross errors in
> device trees should be applied much more selectively
> than is done now, it just doesn't scale this way.

There are trees in the kernel today that have 'address' in them.  Before I can 
fix the trees, though, I need to update U-Boot and the kernel to support 
local-mac-address *without* breaking support for older trees.  That's why this 
patch is written the way it is.

I have patches for U-Boot already out there, although currently none of the 
U-Boot maintainers have indicated a willingness to apply them.

This whole local-mac-address mess needs to be fixed in an iterative process. 
One day, we can remove the check for 'address' from of_get_mac_address(), but 
not yet.

I've implemented all the other suggestion so far.  I *really* need this patch to 
go in as is.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] powerpc: add of_get_mac_address() and update fsl_soc.c to use it

[Segher Boessenkool]

> There are trees in the kernel today that have 'address' in them.  
> Before I can fix the trees, though, I need to update U-Boot and the 
> kernel to support local-mac-address *without* breaking support for 
> older trees.  That's why this patch is written the way it is.

Yes I understand.  My suggestion was to somehow recognise
those trees and only for those try the "address" thing.

Not too important in this case I guess, since almost all
correct trees will have a "local-mac-address" property,
and the correct (standard-conform) use of "address" is
pretty much obsoleted.

Since you are planning to at a later date (hopefully soon :-) )
remove this handling of "address" completely again, I'm sure I
can live with it -- objection withdrawn ;-)


Segher

Re: [PATCH] powerpc: add of_get_mac_address() and update fsl_soc.c to use it

[Timur Tabi]

Segher Boessenkool wrote:

> Since you are planning to at a later date (hopefully soon :-) )
> remove this handling of "address" completely again, I'm sure I
> can live with it -- objection withdrawn ;-)

Thank you.  Once U-Boot and the kernel is updated with my patches, the 
next step is to fix all of the DTS files to only define local-mac-address.