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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
