[U-Boot] [PATCH 17/30] lan91c96/smc91111/smc911x: get mac address from environment

[Mike Frysinger]

On Monday 13 July 2009 17:58:18 Wolfgang Denk wrote:
> Mike Frysinger wrote:
> > > This patch (see also commit 03f3d8d3b39c,
> > > http://git.denx.de/?p=u-boot.git;a=commit;h=03f3d8d3b39cf85c0ce7ca903b4
> > >3670 1e8aa610b) changed behaviour of some network drivers.
> > >
> > > As I just learned (sorry, I missed this in the initial review) it
> > > drops a warning printed by the old code, when there were valid MAC
> > > addresses stored both in the U-Boot environment ("ethaddr" variable)
> > >
> > > and in the controller's EEPROM:
> > > > -	if (env_present && rom_valid) { /* if both env and ROM are good */
> > > > -		if (memcmp (v_env_mac, v_rom_mac, 6) != 0) {
> > > > -			printf ("\nWarning: MAC addresses don't match:\n");
> > > > -			printf ("\tHW MAC address:  "
> > > > -				"%02X:%02X:%02X:%02X:%02X:%02X\n",
> > > > -				v_rom_mac[0], v_rom_mac[1],
> > > > -				v_rom_mac[2], v_rom_mac[3],
> > > > -				v_rom_mac[4], v_rom_mac[5] );
> > > > -			printf ("\t\"ethaddr\" value: "
> > > > -				"%02X:%02X:%02X:%02X:%02X:%02X\n",
> > > > -				v_env_mac[0], v_env_mac[1],
> > > > -				v_env_mac[2], v_env_mac[3],
> > > > -				v_env_mac[4], v_env_mac[5]) ;
> > > > -			debug ("### Set MAC addr from environment\n");
> > > > -		}
> > > > -	}
> > >
> > > This affects other drivers as well (cs8900 for example, in another
> > > patch).
> > >
> > > Can you please explain what your rationale was for removing this code?
> >
> > i thought the u-boot design was to not touch hardware it shouldnt be.  if
> > the
>
> Um...yes - but what has thjis to do with that here?

a valid mac exists in the environment already, so u-boot should not be 
checking the MAC address

> > canonical location for eth addr is the env, then what is in the attached
> > eeproms is irrelevant as it wont be used if the env is available.  it's
> > also
>
> But printing a warning message is probably still a pretty good  idea,
> especially  since  you  don't know what the driver in some OS will do
> that might be bootet later - and not only users,  even  some  servers
> and routers may get confused when a device changes MAC addresses like
> that.

makes sense to me

> > not specific to either of these drivers, so if we did choose to make this
> > behavior optional via some define, it would make more sense to do it in
> > the common eth code rather than copying & pasting it everywhere.
>
> Agreed.  Do you think you have time and resources to craft such a patch?

net/eth.c:eth_initialize() already has the logic to handle this, but that only 
applies to NET_MULTI drivers.  and most do not take advantage of it.  i think 
the documentation should be updated like so:
	in the driver function that calls eth_register(), the driver should
	initialize dev->enetaddr to the MAC found in the hardware (if possible)
and then applicable drivers should be fixed.  if we agree on this route, i can 
do a quick scan of the net drivers and post relevant patches.

in the case of a mismatch, we would see from the common eth code:
Warning: Blackfin EMAC MAC addresses don't match:
Address in SROM is         0a:0a:0a:0a:0a:0a
Address in environment is  00:e0:22:fe:44:ec

the smsc drivers however are not in the NET_MULTI category -- they dont use 
any of the common ethernet stack.  so once they are converted to NET_MULTI, 
they'll get this functionality for free (when exactly were we adding #warning 
about non-NET_MULTI usage ?).  so rather than expend effort on restoring 
duplicate code, how about interested parties convert the driver ;).
-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/20090715/8a43272b/attachment.pgp