[U-Boot] net, fec_mxc: init mac address before using network commands

[U-Boot] net, fec_mxc: init mac address before using network commands

[Heiko Schocher]

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;
 }
-- 
1.6.2.5

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[U-Boot] net, fec_mxc: init mac address before using network commands

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20100330/4a1a6523/attachment.pgp 

[U-Boot] net, fec_mxc: init mac address before using network commands

[Wolfgang Denk]

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

-- 
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
HR Manager to job candidate "I see you've had no  computer  training.
Although  that  qualifies  you  for upper management, it means you're
under-qualified for our entry level positions."

[U-Boot] net, fec_mxc: init mac address before using network commands

[Detlev Zundel]

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

-- 
I think that level of generalization is too abstract for useful thinking.
             -- Richard Stallman in <E19N344-0006Q9-Bt@fencepost.gnu.org>
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de