[U-Boot] [PATCH v5 08/12] SPEAr : Support for HW mac id read/write from i2c mem

[Ben Warren]

Vipin,

Vipin KUMAR wrote:
> On 1/18/2010 11:45 AM, Ben Warren wrote:
>   
>>> int misc_init_r(void)
>>>  {
>>> +#if defined(CONFIG_CMD_NET)
>>> +    uchar mac_id[6];
>>> +
>>> +    if (!eth_getenv_enetaddr("ethaddr", mac_id) &&
>>> !i2c_read_mac(mac_id))
>>> +        eth_setenv_enetaddr("ethaddr", mac_id);
>>>   
>>>       
>> It's really not a good idea to programatically set an environment
>> variable like this.  Also, there's already a well-defined and documented
>> protocol for  how MAC addresses should be read and the precedence
>> between EEPROM storage and environment storage.  Please find a way to
>> fit into that model.  Since you don't have network support yet, I have
>> no idea which network controller you're using, and recommend waiting
>> until this is really needed.
>>     
>
> I am referring to doc/README.enetaddr in the uboot source. This document says
> "If the hardware design mandates that the MAC address is stored in some special
> place (like EEPROM etc...), then the board specific init code (such as the
> board-specific misc_init_r() function) is responsible for locating the MAC
> address(es) and initializing the respective environment variable(s) from it.
> Note that this shall be done if, and only if, the environment does not already
> contain these environment variables, i.e. existing variable definitions must
> not be overwritten."
>
> Coming back to the code
>    if (!eth_getenv_enetaddr("ethaddr", mac_id) && !i2c_read_mac(mac_id))
>         eth_setenv_enetaddr("ethaddr", mac_id);
>
> first I read the env variable "ethaddr" and skip setting it if it already
> exists
> then I read mac id from i2c memory and store the same in env variable
>
> Please let me know if I am missing anything.
>
> Although the patch is not really needed right now, it will be needed soon.
> I am planning to send the net support patch during the next merging window.
> Since the current patch does not have any side effects, it can be included 
> as of now.
>
>   
You're right.  For the sake of keeping networking stuff together, I'd 
prefer to see this logic in 'board_eth_init()' once you have it coded, 
but am not going to hold up progress for such a minor point.  Sorry for 
the noise.
>>>  
>>>          if (chip->cpufreq == -1)
>>> @@ -156,6 +215,13 @@ int do_chip_config(cmd_tbl_t *cmdtp, int flag,
>>> int argc, char *argv[])
>>>          else
>>>              printf("DDR Type    = Not Known\n");
>>>  
>>> +        if (!i2c_read_mac(mac)) {
>>> +            sprintf(i2c_mac, "%pM", mac);
>>> +            printf("Ethaddr (from i2c mem) = %s\n", i2c_mac);
>>>   
>>>       
>> Maybe I'm missing some context, but why's the sprintf needed?
>>     
>
> Yes, sprintf is not needed here
> Thanks for pointing out
>
> Regards
> Vipin
>   
regards,
Ben