| Message ID | 1314967433-14199-1-git-send-email-premi@ti.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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
> -----Original Message----- > From: Wolfgang Denk [mailto:wd@denx.de] > Sent: Friday, September 02, 2011 7:14 PM > To: Premi, Sanjeev > Cc: u-boot@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@denx.de > Build a system that even a fool can use and only a fool will want to > use it. >
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
> From: Wolfgang Denk [wd@denx.de] > Sent: Saturday, September 03, 2011 3:37 AM > To: Premi, Sanjeev > Cc: u-boot@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
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; }
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(-)