[U-Boot] [PATCH] omap3evm: Set environment variable 'ethaddr'

[U-Boot] [PATCH] omap3evm: Set environment variable 'ethaddr'

[Sanjeev Premi]

It is now responsibility of the board specific init
code to set the environment variable corresponding
to the MAC address.

Signed-off-by: Sanjeev Premi <premi@ti.com>
---

 Tested on omap3evm at against latest master at:
 bd061a5 : Merge branch 'master' of git://git.denx.de/u-boot-sh

 board/ti/evm/evm.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/board/ti/evm/evm.c b/board/ti/evm/evm.c
index 30c1c57..07db40c 100644
--- a/board/ti/evm/evm.c
+++ b/board/ti/evm/evm.c
@@ -216,7 +216,17 @@ int board_eth_init(bd_t *bis)
 {
 	int rc = 0;
 #ifdef CONFIG_SMC911X
+	struct eth_device *dev;
+
 	rc = smc911x_initialize(0, CONFIG_SMC911X_BASE);
+
+	dev = eth_get_dev_by_index(0);
+	if (dev) {
+		eth_setenv_enetaddr("ethaddr", dev->enetaddr);
+	} else {
+		printf("omap3evm: Couldn't get eth device\n");
+		rc = -1;
+	}
 #endif
 	return rc;
 }
-- 
1.7.0.4

[U-Boot] [PATCH] omap3evm: Set environment variable 'ethaddr'

[Wolfgang Denk]

Dear Sanjeev Premi,

In message <1314967433-14199-1-git-send-email-premi@ti.com> you wrote:
> It is now responsibility of the board specific init
> code to set the environment variable corresponding
> to the MAC address.
> 
> Signed-off-by: Sanjeev Premi <premi@ti.com>

This looks all wrong to me.  In U-Boot, the "ethaddr" environment
variable is normally the primary storage for the MAC address.  Only
when this variable is not set, other potential storage locations may
be referenced to initialize this value.

Your patch always and unconditionally overwrites any existing
"ethaddr" settings.  This is not acceptable.

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
Build a system that even a fool can use and only a fool will want  to
use it.

[U-Boot] [PATCH] omap3evm: Set environment variable 'ethaddr'

[Premi, Sanjeev]

> -----Original Message-----
> From: Wolfgang Denk [mailto:wd at denx.de] 
> Sent: Friday, September 02, 2011 7:14 PM
> To: Premi, Sanjeev
> Cc: u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH] omap3evm: Set environment 
> variable 'ethaddr'
> 
> Dear Sanjeev Premi,
> 
> In message <1314967433-14199-1-git-send-email-premi@ti.com> you wrote:
> > It is now responsibility of the board specific init
> > code to set the environment variable corresponding
> > to the MAC address.
> > 
> > Signed-off-by: Sanjeev Premi <premi@ti.com>
> 
> This looks all wrong to me.  In U-Boot, the "ethaddr" environment
> variable is normally the primary storage for the MAC address.  Only
> when this variable is not set, other potential storage locations may
> be referenced to initialize this value.
> 
> Your patch always and unconditionally overwrites any existing
> "ethaddr" settings.  This is not acceptable.

For the EVM, MAC address is always probed from the chip. Hence, I
assumed it safe to set the ethaddr - without checking for env var.
It was unlikely that someone would be setting it - to different
value.

But, I see your point that I should be checking for existence of a
valid ethaddr before setting/overwriting it.

~sanjeev

> 
> 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
> Build a system that even a fool can use and only a fool will want  to
> use it.
> 

[U-Boot] [PATCH] omap3evm: Set environment variable 'ethaddr'

[Wolfgang Denk]

Dear "Premi, Sanjeev",

In message <B85A65D85D7EB246BE421B3FB0FBB5930257371E07@dbde02.ent.ti.com> you wrote:
>
> > Your patch always and unconditionally overwrites any existing
> > "ethaddr" settings.  This is not acceptable.
> 
> For the EVM, MAC address is always probed from the chip. Hence, I
> assumed it safe to set the ethaddr - without checking for env var.

This is not what we do in U-Boot, so please fix that.

> It was unlikely that someone would be setting it - to different
> value.

Unlikely?  Not so.  This is expected standard behaviour.  Please don't
try to invent a wheel (in a different shape).

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
Save energy:  Drive a smaller shell.

[U-Boot] [PATCH] omap3evm: Set environment variable 'ethaddr'

[Premi, Sanjeev]

> From: Wolfgang Denk [wd at denx.de]
> Sent: Saturday, September 03, 2011 3:37 AM
> To: Premi, Sanjeev
> Cc: u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH] omap3evm: Set environment variable 'ethaddr'
> 
> Dear "Premi, Sanjeev",
> 
> In message <B85A65D85D7EB246BE421B3FB0FBB5930257371E07@dbde02.ent.ti.com> you wrote:
> >
> > > Your patch always and unconditionally overwrites any existing
> > > "ethaddr" settings.  This is not acceptable.
> >
> > For the EVM, MAC address is always probed from the chip. Hence, I
> > assumed it safe to set the ethaddr - without checking for env var.
> 
> This is not what we do in U-Boot, so please fix that.
> 
> > It was unlikely that someone would be setting it - to different
> > value.
> 
> Unlikely?  Not so.  This is expected standard behaviour.  Please don't
> try to invent a wheel (in a different shape).

[sp] I agreed with your comments in the next para. Had only
     described my thoughts before creating this patch.

     You seem to have missed the v2 of the same patch where I
     believe I have taken care of this behavior:
     http://marc.info/?l=u-boot&m=131497907619918&w=2
     Do let me know if this is better...
     
~sanjeev

> 
> Best regards,
> 
> Wolfgang Denk