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