| Message ID | 1445962239-14213-1-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Joe Hershberger |
| Headers | show |
Hi Bin, On Tue, Oct 27, 2015 at 11:10 AM, Bin Meng <bmeng.cn@gmail.com> wrote: > Currently in fdt_fixup_ethernet() the MAC address fix up is > handled in a loop of which the exit condition is to test the > "eth%daddr" env is not NULL. However this creates unnecessary > constrains that those "eth%daddr" env variables must be > sequential even if "ethernet%d" does not start from 0 in the > "/aliases" node. For example, with "/aliases" node below: > > aliases { > ethernet3 = &enet3; > ethernet4 = &enet4; > }; > > "ethaddr", "eth1addr", "eth2addr" must exist in order to fix > up ethernet3's MAC address successfully. > > Now we change the loop logic to iterate the properties in the > "/aliases" node. For each property, test if it is in a format > of "ethernet%d", then get its MAC address from corresponding > "eth%daddr" env and fix it up in the dtb. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> Acked-by: Joe Hershberger <joe.hershberger@ni.com>
Hi Joe, On Thu, Oct 29, 2015 at 4:52 AM, Joe Hershberger <joe.hershberger@gmail.com> wrote: > Hi Bin, > > On Tue, Oct 27, 2015 at 11:10 AM, Bin Meng <bmeng.cn@gmail.com> wrote: >> Currently in fdt_fixup_ethernet() the MAC address fix up is >> handled in a loop of which the exit condition is to test the >> "eth%daddr" env is not NULL. However this creates unnecessary >> constrains that those "eth%daddr" env variables must be >> sequential even if "ethernet%d" does not start from 0 in the >> "/aliases" node. For example, with "/aliases" node below: >> >> aliases { >> ethernet3 = &enet3; >> ethernet4 = &enet4; >> }; >> >> "ethaddr", "eth1addr", "eth2addr" must exist in order to fix >> up ethernet3's MAC address successfully. >> >> Now we change the loop logic to iterate the properties in the >> "/aliases" node. For each property, test if it is in a format >> of "ethernet%d", then get its MAC address from corresponding >> "eth%daddr" env and fix it up in the dtb. >> >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > Acked-by: Joe Hershberger <joe.hershberger@ni.com> Thanks! Do you have any comments regarding to "usbethaddr" env that I raised here [1]? I originally wanted to remove "usbethaddr" handling completely in fdt_fixup_ethernet(), but did not do that when I submitted this patch. [1] http://lists.denx.de/pipermail/u-boot/2015-October/231791.html Regards, Bin
On Tue, Oct 27, 2015 at 09:10:39AM -0700, Bin Meng wrote: > Currently in fdt_fixup_ethernet() the MAC address fix up is > handled in a loop of which the exit condition is to test the > "eth%daddr" env is not NULL. However this creates unnecessary > constrains that those "eth%daddr" env variables must be > sequential even if "ethernet%d" does not start from 0 in the > "/aliases" node. For example, with "/aliases" node below: > > aliases { > ethernet3 = &enet3; > ethernet4 = &enet4; > }; > > "ethaddr", "eth1addr", "eth2addr" must exist in order to fix > up ethernet3's MAC address successfully. > > Now we change the loop logic to iterate the properties in the > "/aliases" node. For each property, test if it is in a format > of "ethernet%d", then get its MAC address from corresponding > "eth%daddr" env and fix it up in the dtb. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> Reviewed-by: Tom Rini <trini@konsulko.com>
On Thu, Oct 29, 2015 at 09:40:43AM +0800, Bin Meng wrote: > Hi Joe, > > On Thu, Oct 29, 2015 at 4:52 AM, Joe Hershberger > <joe.hershberger@gmail.com> wrote: > > Hi Bin, > > > > On Tue, Oct 27, 2015 at 11:10 AM, Bin Meng <bmeng.cn@gmail.com> wrote: > >> Currently in fdt_fixup_ethernet() the MAC address fix up is > >> handled in a loop of which the exit condition is to test the > >> "eth%daddr" env is not NULL. However this creates unnecessary > >> constrains that those "eth%daddr" env variables must be > >> sequential even if "ethernet%d" does not start from 0 in the > >> "/aliases" node. For example, with "/aliases" node below: > >> > >> aliases { > >> ethernet3 = &enet3; > >> ethernet4 = &enet4; > >> }; > >> > >> "ethaddr", "eth1addr", "eth2addr" must exist in order to fix > >> up ethernet3's MAC address successfully. > >> > >> Now we change the loop logic to iterate the properties in the > >> "/aliases" node. For each property, test if it is in a format > >> of "ethernet%d", then get its MAC address from corresponding > >> "eth%daddr" env and fix it up in the dtb. > >> > >> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > > > Acked-by: Joe Hershberger <joe.hershberger@ni.com> > > Thanks! Do you have any comments regarding to "usbethaddr" env that I > raised here [1]? I originally wanted to remove "usbethaddr" handling > completely in fdt_fixup_ethernet(), but did not do that when I > submitted this patch. I need to think about that post still, sorry, but we can't remove it, it would break various systems that have on-board USB eth (pandaboard, others).
Hi, On 28 October 2015 at 19:40, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Joe, > > On Thu, Oct 29, 2015 at 4:52 AM, Joe Hershberger > <joe.hershberger@gmail.com> wrote: >> Hi Bin, >> >> On Tue, Oct 27, 2015 at 11:10 AM, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Currently in fdt_fixup_ethernet() the MAC address fix up is >>> handled in a loop of which the exit condition is to test the >>> "eth%daddr" env is not NULL. However this creates unnecessary >>> constrains that those "eth%daddr" env variables must be >>> sequential even if "ethernet%d" does not start from 0 in the >>> "/aliases" node. For example, with "/aliases" node below: >>> >>> aliases { >>> ethernet3 = &enet3; >>> ethernet4 = &enet4; >>> }; >>> >>> "ethaddr", "eth1addr", "eth2addr" must exist in order to fix >>> up ethernet3's MAC address successfully. >>> >>> Now we change the loop logic to iterate the properties in the >>> "/aliases" node. For each property, test if it is in a format >>> of "ethernet%d", then get its MAC address from corresponding >>> "eth%daddr" env and fix it up in the dtb. >>> >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> >> Acked-by: Joe Hershberger <joe.hershberger@ni.com> > > Thanks! Do you have any comments regarding to "usbethaddr" env that I > raised here [1]? I originally wanted to remove "usbethaddr" handling > completely in fdt_fixup_ethernet(), but did not do that when I > submitted this patch. > > [1] http://lists.denx.de/pipermail/u-boot/2015-October/231791.html My understanding is that we were going to drop the usbethaddr variables and just use ethaddr. Regards, Simon
Hi Simon, On Thu, Oct 29, 2015 at 9:53 AM, Simon Glass <sjg@chromium.org> wrote: > Hi, > > On 28 October 2015 at 19:40, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Joe, >> >> On Thu, Oct 29, 2015 at 4:52 AM, Joe Hershberger >> <joe.hershberger@gmail.com> wrote: >>> Hi Bin, >>> >>> On Tue, Oct 27, 2015 at 11:10 AM, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> Currently in fdt_fixup_ethernet() the MAC address fix up is >>>> handled in a loop of which the exit condition is to test the >>>> "eth%daddr" env is not NULL. However this creates unnecessary >>>> constrains that those "eth%daddr" env variables must be >>>> sequential even if "ethernet%d" does not start from 0 in the >>>> "/aliases" node. For example, with "/aliases" node below: >>>> >>>> aliases { >>>> ethernet3 = &enet3; >>>> ethernet4 = &enet4; >>>> }; >>>> >>>> "ethaddr", "eth1addr", "eth2addr" must exist in order to fix >>>> up ethernet3's MAC address successfully. >>>> >>>> Now we change the loop logic to iterate the properties in the >>>> "/aliases" node. For each property, test if it is in a format >>>> of "ethernet%d", then get its MAC address from corresponding >>>> "eth%daddr" env and fix it up in the dtb. >>>> >>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>> >>> Acked-by: Joe Hershberger <joe.hershberger@ni.com> >> >> Thanks! Do you have any comments regarding to "usbethaddr" env that I >> raised here [1]? I originally wanted to remove "usbethaddr" handling >> completely in fdt_fixup_ethernet(), but did not do that when I >> submitted this patch. >> >> [1] http://lists.denx.de/pipermail/u-boot/2015-October/231791.html > > My understanding is that we were going to drop the usbethaddr > variables and just use ethaddr. > Yep, I agree, and driver model eth device only uses "ethaddr" as the MAC address source. I guess when we introduced "usbethaddr" to U-Boot at the beginning we didn't think about MAC address fix up. Later commit b1f49ab was added to only handle "usbethaddr" assuming that there is only one usb ethernet on the board, which is not necessarily the case. Using two sources ("usbethaddr" and "ethaddr") just causes trouble in the logic in fdt_fixup_ethernet(). Regards, Bin
Hi Bin, On Wed, Oct 28, 2015 at 8:40 PM, Bin Meng <bmeng.cn@gmail.com> wrote: > Hi Joe, > > On Thu, Oct 29, 2015 at 4:52 AM, Joe Hershberger > <joe.hershberger@gmail.com> wrote: >> Hi Bin, >> >> On Tue, Oct 27, 2015 at 11:10 AM, Bin Meng <bmeng.cn@gmail.com> wrote: >>> Currently in fdt_fixup_ethernet() the MAC address fix up is >>> handled in a loop of which the exit condition is to test the >>> "eth%daddr" env is not NULL. However this creates unnecessary >>> constrains that those "eth%daddr" env variables must be >>> sequential even if "ethernet%d" does not start from 0 in the >>> "/aliases" node. For example, with "/aliases" node below: >>> >>> aliases { >>> ethernet3 = &enet3; >>> ethernet4 = &enet4; >>> }; >>> >>> "ethaddr", "eth1addr", "eth2addr" must exist in order to fix >>> up ethernet3's MAC address successfully. >>> >>> Now we change the loop logic to iterate the properties in the >>> "/aliases" node. For each property, test if it is in a format >>> of "ethernet%d", then get its MAC address from corresponding >>> "eth%daddr" env and fix it up in the dtb. >>> >>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> >> Acked-by: Joe Hershberger <joe.hershberger@ni.com> > > Thanks! Do you have any comments regarding to "usbethaddr" env that I > raised here [1]? I originally wanted to remove "usbethaddr" handling > completely in fdt_fixup_ethernet(), but did not do that when I > submitted this patch. > > [1] http://lists.denx.de/pipermail/u-boot/2015-October/231791.html I'm in agreement with you. See here: http://lists.denx.de/pipermail/u-boot/2015-July/218601.html It would be great if you completely remove it. Cheers, -Joe
Hi Joe, On Fri, Oct 30, 2015 at 3:38 AM, Joe Hershberger <joe.hershberger@gmail.com> wrote: > Hi Bin, > > On Wed, Oct 28, 2015 at 8:40 PM, Bin Meng <bmeng.cn@gmail.com> wrote: >> Hi Joe, >> >> On Thu, Oct 29, 2015 at 4:52 AM, Joe Hershberger >> <joe.hershberger@gmail.com> wrote: >>> Hi Bin, >>> >>> On Tue, Oct 27, 2015 at 11:10 AM, Bin Meng <bmeng.cn@gmail.com> wrote: >>>> Currently in fdt_fixup_ethernet() the MAC address fix up is >>>> handled in a loop of which the exit condition is to test the >>>> "eth%daddr" env is not NULL. However this creates unnecessary >>>> constrains that those "eth%daddr" env variables must be >>>> sequential even if "ethernet%d" does not start from 0 in the >>>> "/aliases" node. For example, with "/aliases" node below: >>>> >>>> aliases { >>>> ethernet3 = &enet3; >>>> ethernet4 = &enet4; >>>> }; >>>> >>>> "ethaddr", "eth1addr", "eth2addr" must exist in order to fix >>>> up ethernet3's MAC address successfully. >>>> >>>> Now we change the loop logic to iterate the properties in the >>>> "/aliases" node. For each property, test if it is in a format >>>> of "ethernet%d", then get its MAC address from corresponding >>>> "eth%daddr" env and fix it up in the dtb. >>>> >>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >>> >>> Acked-by: Joe Hershberger <joe.hershberger@ni.com> >> >> Thanks! Do you have any comments regarding to "usbethaddr" env that I >> raised here [1]? I originally wanted to remove "usbethaddr" handling >> completely in fdt_fixup_ethernet(), but did not do that when I >> submitted this patch. >> >> [1] http://lists.denx.de/pipermail/u-boot/2015-October/231791.html > > I'm in agreement with you. See here: > http://lists.denx.de/pipermail/u-boot/2015-July/218601.html > > It would be great if you completely remove it. > Good. I will prepare a patch to remove "usbethaddr" in fdt_fixup_ethernet(). Regards, Bin
On Fri, Oct 30, 2015 at 10:16:49AM +0800, Bin Meng wrote: > Hi Joe, > > On Fri, Oct 30, 2015 at 3:38 AM, Joe Hershberger > <joe.hershberger@gmail.com> wrote: > > Hi Bin, > > > > On Wed, Oct 28, 2015 at 8:40 PM, Bin Meng <bmeng.cn@gmail.com> wrote: > >> Hi Joe, > >> > >> On Thu, Oct 29, 2015 at 4:52 AM, Joe Hershberger > >> <joe.hershberger@gmail.com> wrote: > >>> Hi Bin, > >>> > >>> On Tue, Oct 27, 2015 at 11:10 AM, Bin Meng <bmeng.cn@gmail.com> wrote: > >>>> Currently in fdt_fixup_ethernet() the MAC address fix up is > >>>> handled in a loop of which the exit condition is to test the > >>>> "eth%daddr" env is not NULL. However this creates unnecessary > >>>> constrains that those "eth%daddr" env variables must be > >>>> sequential even if "ethernet%d" does not start from 0 in the > >>>> "/aliases" node. For example, with "/aliases" node below: > >>>> > >>>> aliases { > >>>> ethernet3 = &enet3; > >>>> ethernet4 = &enet4; > >>>> }; > >>>> > >>>> "ethaddr", "eth1addr", "eth2addr" must exist in order to fix > >>>> up ethernet3's MAC address successfully. > >>>> > >>>> Now we change the loop logic to iterate the properties in the > >>>> "/aliases" node. For each property, test if it is in a format > >>>> of "ethernet%d", then get its MAC address from corresponding > >>>> "eth%daddr" env and fix it up in the dtb. > >>>> > >>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > >>> > >>> Acked-by: Joe Hershberger <joe.hershberger@ni.com> > >> > >> Thanks! Do you have any comments regarding to "usbethaddr" env that I > >> raised here [1]? I originally wanted to remove "usbethaddr" handling > >> completely in fdt_fixup_ethernet(), but did not do that when I > >> submitted this patch. > >> > >> [1] http://lists.denx.de/pipermail/u-boot/2015-October/231791.html > > > > I'm in agreement with you. See here: > > http://lists.denx.de/pipermail/u-boot/2015-July/218601.html > > > > It would be great if you completely remove it. > > > > Good. I will prepare a patch to remove "usbethaddr" in fdt_fixup_ethernet(). So long as you don't break the boards that need it :)
Hi Tom, On Fri, Oct 30, 2015 at 10:25 AM, Tom Rini <trini@konsulko.com> wrote: > On Fri, Oct 30, 2015 at 10:16:49AM +0800, Bin Meng wrote: >> Hi Joe, >> >> On Fri, Oct 30, 2015 at 3:38 AM, Joe Hershberger >> <joe.hershberger@gmail.com> wrote: >> > Hi Bin, >> > >> > On Wed, Oct 28, 2015 at 8:40 PM, Bin Meng <bmeng.cn@gmail.com> wrote: >> >> Hi Joe, >> >> >> >> On Thu, Oct 29, 2015 at 4:52 AM, Joe Hershberger >> >> <joe.hershberger@gmail.com> wrote: >> >>> Hi Bin, >> >>> >> >>> On Tue, Oct 27, 2015 at 11:10 AM, Bin Meng <bmeng.cn@gmail.com> wrote: >> >>>> Currently in fdt_fixup_ethernet() the MAC address fix up is >> >>>> handled in a loop of which the exit condition is to test the >> >>>> "eth%daddr" env is not NULL. However this creates unnecessary >> >>>> constrains that those "eth%daddr" env variables must be >> >>>> sequential even if "ethernet%d" does not start from 0 in the >> >>>> "/aliases" node. For example, with "/aliases" node below: >> >>>> >> >>>> aliases { >> >>>> ethernet3 = &enet3; >> >>>> ethernet4 = &enet4; >> >>>> }; >> >>>> >> >>>> "ethaddr", "eth1addr", "eth2addr" must exist in order to fix >> >>>> up ethernet3's MAC address successfully. >> >>>> >> >>>> Now we change the loop logic to iterate the properties in the >> >>>> "/aliases" node. For each property, test if it is in a format >> >>>> of "ethernet%d", then get its MAC address from corresponding >> >>>> "eth%daddr" env and fix it up in the dtb. >> >>>> >> >>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> >>> >> >>> Acked-by: Joe Hershberger <joe.hershberger@ni.com> >> >> >> >> Thanks! Do you have any comments regarding to "usbethaddr" env that I >> >> raised here [1]? I originally wanted to remove "usbethaddr" handling >> >> completely in fdt_fixup_ethernet(), but did not do that when I >> >> submitted this patch. >> >> >> >> [1] http://lists.denx.de/pipermail/u-boot/2015-October/231791.html >> > >> > I'm in agreement with you. See here: >> > http://lists.denx.de/pipermail/u-boot/2015-July/218601.html >> > >> > It would be great if you completely remove it. >> > >> >> Good. I will prepare a patch to remove "usbethaddr" in fdt_fixup_ethernet(). > > So long as you don't break the boards that need it :) > I will only remove the MAC address fix up in fdt_fixup_ethernet(). This needs these affected boards to configure "ethaddr" in their environment. For other instances of "usbethaddr" in U-Boot, they should be removed when these drivers/boards have been converted to driver model. Regards, Bin
diff --git a/common/fdt_support.c b/common/fdt_support.c index f86365e..5cdf185 100644 --- a/common/fdt_support.c +++ b/common/fdt_support.c @@ -476,10 +476,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64 size) void fdt_fixup_ethernet(void *fdt) { int node, i, j; - char enet[16], *tmp, *end; + char *tmp, *end; char mac[16]; const char *path; unsigned char mac_addr[6]; + int offset; node = fdt_path_offset(fdt, "/aliases"); if (node < 0) @@ -496,27 +497,39 @@ void fdt_fixup_ethernet(void *fdt) strcpy(mac, "ethaddr"); } - i = 0; - while ((tmp = getenv(mac)) != NULL) { - sprintf(enet, "ethernet%d", i); - path = fdt_getprop(fdt, node, enet, NULL); - if (!path) { - debug("No alias for %s\n", enet); - sprintf(mac, "eth%daddr", ++i); - continue; - } + for (offset = fdt_first_property_offset(fdt, node); + offset > 0; + offset = fdt_next_property_offset(fdt, offset)) { + const char *name; + int len = strlen("ethernet"); + + path = fdt_getprop_by_offset(fdt, offset, &name, NULL); + if (!strncmp(name, "ethernet", len)) { + i = trailing_strtol(name); + if (i != -1) { + if (i == 0) + strcpy(mac, "ethaddr"); + else + sprintf(mac, "eth%daddr", i); + } else { + continue; + } + tmp = getenv(mac); + if (!tmp) + continue; + + for (j = 0; j < 6; j++) { + mac_addr[j] = tmp ? + simple_strtoul(tmp, &end, 16) : 0; + if (tmp) + tmp = (*end) ? end + 1 : end; + } - 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", + &mac_addr, 6, 0); + do_fixup_by_path(fdt, path, "local-mac-address", + &mac_addr, 6, 1); } - - do_fixup_by_path(fdt, path, "mac-address", &mac_addr, 6, 0); - do_fixup_by_path(fdt, path, "local-mac-address", - &mac_addr, 6, 1); - - sprintf(mac, "eth%daddr", ++i); } }
Currently in fdt_fixup_ethernet() the MAC address fix up is handled in a loop of which the exit condition is to test the "eth%daddr" env is not NULL. However this creates unnecessary constrains that those "eth%daddr" env variables must be sequential even if "ethernet%d" does not start from 0 in the "/aliases" node. For example, with "/aliases" node below: aliases { ethernet3 = &enet3; ethernet4 = &enet4; }; "ethaddr", "eth1addr", "eth2addr" must exist in order to fix up ethernet3's MAC address successfully. Now we change the loop logic to iterate the properties in the "/aliases" node. For each property, test if it is in a format of "ethernet%d", then get its MAC address from corresponding "eth%daddr" env and fix it up in the dtb. Signed-off-by: Bin Meng <bmeng.cn@gmail.com> --- common/fdt_support.c | 53 ++++++++++++++++++++++++++++++++-------------------- 1 file changed, 33 insertions(+), 20 deletions(-)