[U-Boot] [PATCH v2] fdt: rework fdt_fixup_ethernet() to use env instead of bd_t
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);
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
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.
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
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.
-Scott
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
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
On Aug 18, 2008, at 3:55 PM, Wolfgang Denk wrote:
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.
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
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
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
participants (4)
-
Jerry Van Baren -
Kumar Gala -
Scott Wood -
Wolfgang Denk