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
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
