[U-Boot] net, fec_mxc: init mac address before using network commands
initialize mac address with the value from "ethaddr", before doing some network commands. This is not in line with u-boot design principle "not to initalize not used devices", and maybe should go away, if there is a solution for passing the mac address to arm linux kernels.
Tested on the magnesium board.
Signed-off-by: Heiko Schocher hs@denx.de --- posting this patch as a result of this discussion:
http://lists.denx.de/pipermail/u-boot/2010-March/069025.html
drivers/net/fec_mxc.c | 15 +++++++++++---- 1 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index 5af9cdb..9029490 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -749,11 +749,18 @@ static int fec_probe(bd_t *bd)
eth_register(edev);
- if (fec_get_hwaddr(edev, ethaddr) == 0) { - printf("got MAC address from EEPROM: %pM\n", ethaddr); - memcpy(edev->enetaddr, ethaddr, 6); - fec_set_hwaddr(edev); + if (!eth_getenv_enetaddr("ethaddr", ethaddr)) { + /* "ethaddr" is not set in the environment */ + if (fec_get_hwaddr(edev, ethaddr) == 0) { + printf("got MAC address from EEPROM: %pM\n", ethaddr); + eth_setenv_enetaddr("ethaddr", ethaddr); + } else { + printf ("no MAC found\n"); + return -1; + } } + memcpy(edev->enetaddr, ethaddr, 6); + fec_set_hwaddr(edev);
return 0; }
On Tuesday 30 March 2010 01:38:33 Heiko Schocher wrote:
initialize mac address with the value from "ethaddr", before doing some network commands. This is not in line with u-boot design principle "not to initalize not used devices", and maybe should go away, if there is a solution for passing the mac address to arm linux kernels.
NACK like i already said -- net drivers should only ever be reading eth_driver->enetaddr. they should never touch the environment as the common net code takes care of this.
i would instead fix the fec_probe() to stop writing "ethaddr" since it already writes to eth_driver->enetaddr. -mike
Dear Heiko Schocher,
In message 4BB18E59.5000004@denx.de you wrote:
initialize mac address with the value from "ethaddr", before doing some network commands. This is not in line with u-boot design principle "not to initalize not used devices", and maybe should go away, if there is a solution for passing the mac address to arm linux kernels.
Tested on the magnesium board.
Note 1: I would have expected that this commit is marked as a follow-up to your earlier posting from Wed, 24 Mar 2010, titled "[PATCH] net, fec_mxc: use mac address stored in env before looking in eeprom" (http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/76173) Unfortunately your new posting contains no indication of a previous patch (i. e. there is no "v2" or similar in the Subject, nor is there a description of the changes against the previous version below the "---" line. In addition, the Subject has been changed which makes it even more difficult to match this to the older posting.
Note 2: I think this description is actually incomplete. You are mixing two independent modifications here.
diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c index 5af9cdb..9029490 100644 --- a/drivers/net/fec_mxc.c +++ b/drivers/net/fec_mxc.c @@ -749,11 +749,18 @@ static int fec_probe(bd_t *bd)
eth_register(edev);
- if (fec_get_hwaddr(edev, ethaddr) == 0) {
printf("got MAC address from EEPROM: %pM\n", ethaddr);memcpy(edev->enetaddr, ethaddr, 6);fec_set_hwaddr(edev);
- if (!eth_getenv_enetaddr("ethaddr", ethaddr)) {
/* "ethaddr" is not set in the environment */if (fec_get_hwaddr(edev, ethaddr) == 0) {printf("got MAC address from EEPROM: %pM\n", ethaddr);eth_setenv_enetaddr("ethaddr", ethaddr);} else {printf ("no MAC found\n");return -1;}
This part of the commit is the previously dicussed bug fix: without this change, the board would, under certain conditions, ignore the "ethaddr" setting stored in the environment. This is a clear bug fix and as such should get added to the current code. As far as I can tell, this part does not violate any design rules.
}
- memcpy(edev->enetaddr, ethaddr, 6);
- fec_set_hwaddr(edev);
This is a completely different issue. Here you add new code to change the system behaviour in a way that indeed violates the design rules.
Please split this patch into two separate commits. The first part is obviously a fix. I will ACK it and even pull it in the upcoming release (assuming you are fast enough).
I dislike the second part, but I will not actively block it either. Let's see what others (especially Ben) say about it.
Best regards,
Wolfgang Denk
Hello Heiko,
initialize mac address with the value from "ethaddr", before doing some network commands. This is not in line with u-boot design principle "not to initalize not used devices", and maybe should go away, if there is a solution for passing the mac address to arm linux kernels.
Tested on the magnesium board.
Signed-off-by: Heiko Schocher hs@denx.de
Acked-by: Detlev Zundel dzu@denx.de
Cheers Detlev
participants (4)
-
Detlev Zundel -
Heiko Schocher -
Mike Frysinger -
Wolfgang Denk