[U-Boot] [PATCH v2] fdt: rework fdt_fixup_ethernet() to use env instead of bd_t

[U-Boot] [PATCH v2] fdt: rework fdt_fixup_ethernet() to use env instead of bd_t

[Kumar Gala]

Move to using the environment variables 'ethaddr', 'eth1addr', etc..
instead of bd->bi_enetaddr, bi_enet1addr, etc.

This makes the code a bit more flexible to the number of ethernet
interfaces.  Right now we assume a max of 10 interfaces.

Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---

Since we can support 10 interfaces and aren't coupled to the bd we can remove
the conditionality of HAS_ETHn for fdt_fixup_ethernet.

- k

 common/fdt_support.c  |   69 ++++++++++++++++++++++---------------------------
 cpu/74xx_7xx/cpu.c    |    2 +-
 cpu/mpc512x/cpu.c     |    2 +-
 cpu/mpc8260/cpu.c     |    2 +-
 cpu/mpc83xx/fdt.c     |    2 +-
 cpu/mpc85xx/fdt.c     |    2 +-
 cpu/mpc86xx/fdt.c     |    2 +-
 cpu/mpc8xx/fdt.c      |    2 +-
 cpu/ppc4xx/fdt.c      |    2 +-
 include/fdt_support.h |    2 +-
 10 files changed, 40 insertions(+), 47 deletions(-)

diff --git a/common/fdt_support.c b/common/fdt_support.c
index 93b144e..b8f6fde 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -368,55 +368,48 @@ int fdt_fixup_memory(void *blob, u64 start, u64 size)
 	return 0;
 }

-#if defined(CONFIG_HAS_ETH0) || defined(CONFIG_HAS_ETH1) ||\
-    defined(CONFIG_HAS_ETH2) || defined(CONFIG_HAS_ETH3)
-
-void fdt_fixup_ethernet(void *fdt, bd_t *bd)
+#define CFG_MAX_NUM_ETH	(10)
+void fdt_fixup_ethernet(void *fdt)
 {
+	int i, j;
 	int node;
 	const char *path;

 	node = fdt_path_offset(fdt, "/aliases");
 	if (node >= 0) {
-#if defined(CONFIG_HAS_ETH0)
-		path = fdt_getprop(fdt, node, "ethernet0", NULL);
-		if (path) {
-			do_fixup_by_path(fdt, path, "mac-address",
-				bd->bi_enetaddr, 6, 0);
-			do_fixup_by_path(fdt, path, "local-mac-address",
-				bd->bi_enetaddr, 6, 1);
-		}
-#endif
-#if defined(CONFIG_HAS_ETH1)
-		path = fdt_getprop(fdt, node, "ethernet1", NULL);
-		if (path) {
-			do_fixup_by_path(fdt, path, "mac-address",
-				bd->bi_enet1addr, 6, 0);
-			do_fixup_by_path(fdt, path, "local-mac-address",
-				bd->bi_enet1addr, 6, 1);
-		}
-#endif
-#if defined(CONFIG_HAS_ETH2)
-		path = fdt_getprop(fdt, node, "ethernet2", NULL);
-		if (path) {
-			do_fixup_by_path(fdt, path, "mac-address",
-				bd->bi_enet2addr, 6, 0);
-			do_fixup_by_path(fdt, path, "local-mac-address",
-				bd->bi_enet2addr, 6, 1);
-		}
-#endif
-#if defined(CONFIG_HAS_ETH3)
-		path = fdt_getprop(fdt, node, "ethernet3", NULL);
-		if (path) {
+		char enet[12], mac[10], *tmp, *end;
+		unsigned char mac_addr[6];
+
+		for (i = 0; i < CFG_MAX_NUM_ETH; i++) {
+			sprintf(enet, "ethernet%d", i);
+			sprintf(mac, "eth%daddr",i);
+
+			sprintf(mac, i ? "eth%daddr" : "ethaddr", i);
+			tmp = getenv(mac);
+			path = fdt_getprop(fdt, node, enet, NULL);
+
+			if (!path) {
+				debug("No alias for %s\n", enet);
+				continue;
+			}
+			if (!tmp) {
+				debug("No environment variable for %s\n", mac);
+				continue;
+			}
+
+			for (j = 0; j < 6; j++) {
+				mac_addr[j] = tmp ? simple_strtoul(tmp, &end, 16) : 0;
+				if (tmp)
+					tmp = (*end) ? end+1 : end;
+			}
+
 			do_fixup_by_path(fdt, path, "mac-address",
-				bd->bi_enet3addr, 6, 0);
+					&mac_addr, 6, 0);
 			do_fixup_by_path(fdt, path, "local-mac-address",
-				bd->bi_enet3addr, 6, 1);
+					&mac_addr, 6, 1);
 		}
-#endif
 	}
 }
-#endif

 #ifdef CONFIG_HAS_FSL_DR_USB
 void fdt_fixup_dr_usb(void *blob, bd_t *bd)
diff --git a/cpu/74xx_7xx/cpu.c b/cpu/74xx_7xx/cpu.c
index ea43c9a..c007abc 100644
--- a/cpu/74xx_7xx/cpu.c
+++ b/cpu/74xx_7xx/cpu.c
@@ -314,7 +314,7 @@ void ft_cpu_setup(void *blob, bd_t *bd)

 	fdt_fixup_memory(blob, (u64)bd->bi_memstart, (u64)bd->bi_memsize);

-	fdt_fixup_ethernet(blob, bd);
+	fdt_fixup_ethernet(blob);
 }
 #endif
 /* ------------------------------------------------------------------------- */
diff --git a/cpu/mpc512x/cpu.c b/cpu/mpc512x/cpu.c
index 703e188..1f39ac4 100644
--- a/cpu/mpc512x/cpu.c
+++ b/cpu/mpc512x/cpu.c
@@ -191,7 +191,7 @@ void ft_cpu_setup(void *blob, bd_t *bd)
 #endif
 	ft_clock_setup(blob, bd);
 #ifdef CONFIG_HAS_ETH0
-	fdt_fixup_ethernet(blob, bd);
+	fdt_fixup_ethernet(blob);
 #endif
 }
 #endif
diff --git a/cpu/mpc8260/cpu.c b/cpu/mpc8260/cpu.c
index 4d5d141..efb8ed6 100644
--- a/cpu/mpc8260/cpu.c
+++ b/cpu/mpc8260/cpu.c
@@ -307,7 +307,7 @@ void ft_cpu_setup (void *blob, bd_t *bd)

 #if defined(CONFIG_HAS_ETH0) || defined(CONFIG_HAS_ETH1) ||\
     defined(CONFIG_HAS_ETH2) || defined(CONFIG_HAS_ETH3)
-	fdt_fixup_ethernet(blob, bd);
+	fdt_fixup_ethernet(blob);
 #endif

 	do_fixup_by_path_u32(blob, cpu_path, "bus-frequency", bd->bi_busfreq, 1);
diff --git a/cpu/mpc83xx/fdt.c b/cpu/mpc83xx/fdt.c
index fda85c1..39bd9dc 100644
--- a/cpu/mpc83xx/fdt.c
+++ b/cpu/mpc83xx/fdt.c
@@ -53,7 +53,7 @@ void ft_cpu_setup(void *blob, bd_t *bd)

 #if defined(CONFIG_HAS_ETH0) || defined(CONFIG_HAS_ETH1) ||\
     defined(CONFIG_HAS_ETH2) || defined(CONFIG_HAS_ETH3)
-	fdt_fixup_ethernet(blob, bd);
+	fdt_fixup_ethernet(blob);
 #endif

 	do_fixup_by_prop_u32(blob, "device_type", "cpu", 4,
diff --git a/cpu/mpc85xx/fdt.c b/cpu/mpc85xx/fdt.c
index c159934..bc1550d 100644
--- a/cpu/mpc85xx/fdt.c
+++ b/cpu/mpc85xx/fdt.c
@@ -212,7 +212,7 @@ void ft_cpu_setup(void *blob, bd_t *bd)

 #if defined(CONFIG_HAS_ETH0) || defined(CONFIG_HAS_ETH1) ||\
     defined(CONFIG_HAS_ETH2) || defined(CONFIG_HAS_ETH3)
-	fdt_fixup_ethernet(blob, bd);
+	fdt_fixup_ethernet(blob);
 #endif

 	do_fixup_by_prop_u32(blob, "device_type", "cpu", 4,
diff --git a/cpu/mpc86xx/fdt.c b/cpu/mpc86xx/fdt.c
index 80a5c78..12d9052 100644
--- a/cpu/mpc86xx/fdt.c
+++ b/cpu/mpc86xx/fdt.c
@@ -25,7 +25,7 @@ void ft_cpu_setup(void *blob, bd_t *bd)

 #if defined(CONFIG_HAS_ETH0) || defined(CONFIG_HAS_ETH1) \
     || defined(CONFIG_HAS_ETH2) || defined(CONFIG_HAS_ETH3)
-	fdt_fixup_ethernet(blob, bd);
+	fdt_fixup_ethernet(blob);
 #endif

 #ifdef CFG_NS16550
diff --git a/cpu/mpc8xx/fdt.c b/cpu/mpc8xx/fdt.c
index 567094a..7130983 100644
--- a/cpu/mpc8xx/fdt.c
+++ b/cpu/mpc8xx/fdt.c
@@ -40,7 +40,7 @@ void ft_cpu_setup(void *blob, bd_t *bd)
 		gd->brg_clk, 1);

 	/* Fixup ethernet MAC addresses */
-	fdt_fixup_ethernet(blob, bd);
+	fdt_fixup_ethernet(blob);

 	fdt_fixup_memory(blob, (u64)bd->bi_memstart, (u64)bd->bi_memsize);
 }
diff --git a/cpu/ppc4xx/fdt.c b/cpu/ppc4xx/fdt.c
index 0323dc5..a97484f 100644
--- a/cpu/ppc4xx/fdt.c
+++ b/cpu/ppc4xx/fdt.c
@@ -130,7 +130,7 @@ void ft_cpu_setup(void *blob, bd_t *bd)
 	 * Fixup all ethernet nodes
 	 * Note: aliases in the dts are required for this
 	 */
-	fdt_fixup_ethernet(blob, bd);
+	fdt_fixup_ethernet(blob);

 	/*
 	 * Fixup all available PCIe nodes by setting the device_type property
diff --git a/include/fdt_support.h b/include/fdt_support.h
index a7c6326..f2f2cd5 100644
--- a/include/fdt_support.h
+++ b/include/fdt_support.h
@@ -45,7 +45,7 @@ void do_fixup_by_compat(void *fdt, const char *compat,
 void do_fixup_by_compat_u32(void *fdt, const char *compat,
 			    const char *prop, u32 val, int create);
 int fdt_fixup_memory(void *blob, u64 start, u64 size);
-void fdt_fixup_ethernet(void *fdt, bd_t *bd);
+void fdt_fixup_ethernet(void *fdt);
 int fdt_find_and_setprop(void *fdt, const char *node, const char *prop,
 			 const void *val, int len, int create);
 void fdt_fixup_qe_firmware(void *fdt);
-- 
1.5.5.1

[U-Boot] [PATCH v2] fdt: rework fdt_fixup_ethernet() to use env instead of bd_t

[Wolfgang Denk]

Dear Kumar Gala,

In message <Pine.LNX.4.64.0808181422320.28491@blarg.am.freescale.net> you wrote:
> Move to using the environment variables 'ethaddr', 'eth1addr', etc..
> instead of bd->bi_enetaddr, bi_enet1addr, etc.
> 
> This makes the code a bit more flexible to the number of ethernet
> interfaces.  Right now we assume a max of 10 interfaces.

Hm... where exactly is this artificial limit coming from? Do we really
need it?

> +		for (i = 0; i < CFG_MAX_NUM_ETH; i++) {
> +			sprintf(enet, "ethernet%d", i);
> +			sprintf(mac, "eth%daddr",i);
> +
> +			sprintf(mac, i ? "eth%daddr" : "ethaddr", i);
> +			tmp = getenv(mac);

for efficientcy, make this the first action in the loop.

> +			path = fdt_getprop(fdt, node, enet, NULL);
> +
> +			if (!path) {
> +				debug("No alias for %s\n", enet);
> +				continue;
> +			}
> +			if (!tmp) {
> +				debug("No environment variable for %s\n", mac);
> +				continue;
> +			}

If we assume,  that  all  existing  interfaces  must  have  addresses
assigned, we could use a "break" here instead of the "continue". That
would be (1) much faster on most boards and (2) would allow us to get
rid of the artifical limit of 10.

What do you think?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Let the programmers be many and the managers few -- then all will  be
productive.               -- Geoffrey James, "The Tao of Programming"

[U-Boot] [PATCH v2] fdt: rework fdt_fixup_ethernet() to use env instead of bd_t

[Kumar Gala]

On Aug 18, 2008, at 2:30 PM, Wolfgang Denk wrote:

> Dear Kumar Gala,
>
> In message <Pine.LNX. 
> 4.64.0808181422320.28491 at blarg.am.freescale.net> you wrote:
>> Move to using the environment variables 'ethaddr', 'eth1addr', etc..
>> instead of bd->bi_enetaddr, bi_enet1addr, etc.
>>
>> This makes the code a bit more flexible to the number of ethernet
>> interfaces.  Right now we assume a max of 10 interfaces.
>
> Hm... where exactly is this artificial limit coming from? Do we really
> need it?

We need some upper limit to stop checking at.

>> +		for (i = 0; i < CFG_MAX_NUM_ETH; i++) {
>> +			sprintf(enet, "ethernet%d", i);
>> +			sprintf(mac, "eth%daddr",i);
>> +
>> +			sprintf(mac, i ? "eth%daddr" : "ethaddr", i);
>> +			tmp = getenv(mac);
>
> for efficientcy, make this the first action in the loop.

done, and removed the duplicated sprintf that snuck in.

>> +			path = fdt_getprop(fdt, node, enet, NULL);
>> +
>> +			if (!path) {
>> +				debug("No alias for %s\n", enet);
>> +				continue;
>> +			}
>> +			if (!tmp) {
>> +				debug("No environment variable for %s\n", mac);
>> +				continue;
>> +			}
>
> If we assume,  that  all  existing  interfaces  must  have  addresses
> assigned, we could use a "break" here instead of the "continue". That
> would be (1) much faster on most boards and (2) would allow us to get
> rid of the artifical limit of 10.
>
> What do you think?

I dont like making this assumption and do think its too much work to  
check 10 possible aliases and skip to the next one if it doesn't exist.

- k

[U-Boot] [PATCH v2] fdt: rework fdt_fixup_ethernet() to use env instead of bd_t

[Scott Wood]

Kumar Gala wrote:
> On Aug 18, 2008, at 2:30 PM, Wolfgang Denk wrote:
> 
>> Dear Kumar Gala,
>>
>> In message <Pine.LNX. 
>> 4.64.0808181422320.28491 at blarg.am.freescale.net> you wrote:
>>> Move to using the environment variables 'ethaddr', 'eth1addr', etc..
>>> instead of bd->bi_enetaddr, bi_enet1addr, etc.
>>>
>>> This makes the code a bit more flexible to the number of ethernet
>>> interfaces.  Right now we assume a max of 10 interfaces.
>> Hm... where exactly is this artificial limit coming from? Do we really
>> need it?
> 
> We need some upper limit to stop checking at.

It might be better (and more efficent) to iterate over the environment, 
and check each name against the eth*addr pattern.

-Scott

[U-Boot] [PATCH v2] fdt: rework fdt_fixup_ethernet() to use env instead of bd_t

[Kumar Gala]

On Aug 18, 2008, at 2:49 PM, Scott Wood wrote:

> Kumar Gala wrote:
>> On Aug 18, 2008, at 2:30 PM, Wolfgang Denk wrote:
>>> Dear Kumar Gala,
>>>
>>> In message <Pine.LNX. 4.64.0808181422320.28491@blarg.am.freescale.net 
>>> > you wrote:
>>>> Move to using the environment variables 'ethaddr', 'eth1addr',  
>>>> etc..
>>>> instead of bd->bi_enetaddr, bi_enet1addr, etc.
>>>>
>>>> This makes the code a bit more flexible to the number of ethernet
>>>> interfaces.  Right now we assume a max of 10 interfaces.
>>> Hm... where exactly is this artificial limit coming from? Do we  
>>> really
>>> need it?
>> We need some upper limit to stop checking at.
>
> It might be better (and more efficent) to iterate over the  
> environment, and check each name against the eth*addr pattern.

Its probably easier/less work to iterate over the aliases looking for  
"ethernet*" pattern, I can do that if its desired.

- k

[U-Boot] [PATCH v2] fdt: rework fdt_fixup_ethernet() to use env instead of bd_t

[Wolfgang Denk]

Dear Kumar Gala,

In message <119C6E28-E979-4B97-87AD-9603CD5FFDAA@kernel.crashing.org> you wrote:
> 
> >> This makes the code a bit more flexible to the number of ethernet
> >> interfaces.  Right now we assume a max of 10 interfaces.
> >
> > Hm... where exactly is this artificial limit coming from? Do we really
> > need it?
> 
> We need some upper limit to stop checking at.

The upper limit should be the real  (configured)  number  of  network
interfaces, not some artificial limit which is either too high or too
low.

> > If we assume,  that  all  existing  interfaces  must  have  addresses
> > assigned, we could use a "break" here instead of the "continue". That
> > would be (1) much faster on most boards and (2) would allow us to get
> > rid of the artifical limit of 10.
> >
> > What do you think?
> 
> I dont like making this assumption and do think its too much work to  
> check 10 possible aliases and skip to the next one if it doesn't exist.

I do not want to see any  such  hard-coded  limits  if  they  can  be
avoided. Which problem do you see to stop here at the first interface
that has no MAC address assigned to it?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"To IBM, 'open' means there is a modicum  of  interoperability  among
some of their equipment."                            - Harv Masterson

[U-Boot] [PATCH v2] fdt: rework fdt_fixup_ethernet() to use env instead of bd_t

[Wolfgang Denk]

Dear Kumar Gala,

In message <594AB220-FA74-4A51-BF54-C79EBD696A51@kernel.crashing.org> you wrote:
> 
> > It might be better (and more efficent) to iterate over the  
> > environment, and check each name against the eth*addr pattern.
> 
> Its probably easier/less work to iterate over the aliases looking for  
> "ethernet*" pattern, I can do that if its desired.

No, I agree with Scott here.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Build a system that even a fool can use and only a fool will want  to
use it.

[U-Boot] [PATCH v2] fdt: rework fdt_fixup_ethernet() to use env instead of bd_t

[Kumar Gala]

On Aug 18, 2008, at 3:55 PM, Wolfgang Denk wrote:

> Dear Kumar Gala,
>
> In message <594AB220-FA74-4A51-BF54- 
> C79EBD696A51 at kernel.crashing.org> you wrote:
>>
>>> It might be better (and more efficent) to iterate over the
>>> environment, and check each name against the eth*addr pattern.
>>
>> Its probably easier/less work to iterate over the aliases looking for
>> "ethernet*" pattern, I can do that if its desired.
>
> No, I agree with Scott here.

Really?  It seems like parsing the env is a lot more checking than  
aliases.

If you guys think the env is easier/better I'll look at doing that.

- k

[U-Boot] [PATCH v2] fdt: rework fdt_fixup_ethernet() to use env instead of bd_t

[Jerry Van Baren]

Wolfgang Denk wrote:
> Dear Kumar Gala,
> 
> In message <119C6E28-E979-4B97-87AD-9603CD5FFDAA@kernel.crashing.org> you wrote:
>>>> This makes the code a bit more flexible to the number of ethernet
>>>> interfaces.  Right now we assume a max of 10 interfaces.
>>> Hm... where exactly is this artificial limit coming from? Do we really
>>> need it?
>> We need some upper limit to stop checking at.
> 
> The upper limit should be the real  (configured)  number  of  network
> interfaces, not some artificial limit which is either too high or too
> low.

It is (was) - CFG_MAX_NUM_ETH:
+		for (i = 0; i < CFG_MAX_NUM_ETH; i++) {

Actually, I don't see any arbitrary upper limit in the code, including 
Kumar's value of 10 (well, until you overflow the strings, anyway, but 
that is 100 interfaces).

>>> If we assume,  that  all  existing  interfaces  must  have  addresses
>>> assigned, we could use a "break" here instead of the "continue". That
>>> would be (1) much faster on most boards and (2) would allow us to get
>>> rid of the artifical limit of 10.

CFG_MAX_NUM_ETH would presumably be the physical max and the 
/aliases/ethernet (and associated env variables) should *not* be sparse, 
therefore I agree with the the "break" recommendation.

>>> What do you think?
>> I dont like making this assumption and do think its too much work to  
>> check 10 possible aliases and skip to the next one if it doesn't exist.
> 
> I do not want to see any  such  hard-coded  limits  if  they  can  be
> avoided. Which problem do you see to stop here at the first interface
> that has no MAC address assigned to it?

I originally wrote to support sparse ethernet MAC addresses, but on 
reflection I don't think that is an issue because we will have 
/aliases/ethernet[0-9]+ which won't be sparse, even if the actual SOC 
(e.g. PowerQuicc) channels that are used for ethernet are used in a 
sparse manner.

> Best regards,
> 
> Wolfgang Denk

Best regards,
gvb