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