[U-Boot] [PATCH v2 04/17] SPEAr : Placing ethaddr write and read within CONFIG_CMD_NET

[Tom Rix]

Vipin KUMAR wrote:
> On 5/3/2010 5:06 AM, Tom Rix wrote:
>> Vipin KUMAR wrote:
>>> ethaddr can be optionally read from i2c memory. So, chip_config
>>> command supports
>>> reading/writing hw mac id into i2c memory. Placing this code within
>>> CONFIG_CMD_NET as this would only be needed when network interface is
>>> configured
>>>
>>> Signed-off-by: Vipin Kumar <vipin.kumar at st.com>
>>> ---
>>>  board/spear/common/spr_misc.c |   23 ++++++++++++++++++++---
>>>  1 files changed, 20 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/board/spear/common/spr_misc.c
>>> b/board/spear/common/spr_misc.c
>>> index e356912..4af9436 100644
>>> --- a/board/spear/common/spr_misc.c
>>> +++ b/board/spear/common/spr_misc.c
>>> @@ -38,6 +38,10 @@
>>>  DECLARE_GLOBAL_DATA_PTR;
>>>  static struct chip_data chip_data;
>>>  
>>> +#if defined(CONFIG_CMD_NET)
>>> +static int i2c_read_mac(uchar *buffer);
>>> +#endif
>>> +
>>>  int dram_init(void)
>>>  {
>>>      struct xloader_table *xloader_tb =
>>> @@ -166,6 +170,7 @@ int spear_board_init(ulong mach_type)
>>>      return 0;
>>>  }
>>>  
>>> +#if defined(CONFIG_CMD_NET)
>>>  static int i2c_read_mac(uchar *buffer)
>>>  {
>>>      u8 buf[2];
>>> @@ -205,15 +210,20 @@ static int write_mac(uchar *mac)
>>>      puts("I2C EEPROM writing failed \n");
>>>      return -1;
>>>  }
>>> +#endif
>>>  
>>>  int do_chip_config(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>>>  {
>>>      void (*sram_setfreq) (unsigned int, unsigned int);
>>>      struct chip_data *chip = &chip_data;
>>> -    unsigned char mac[6];
>>> -    unsigned int reg, frequency;
>>> +    unsigned int frequency;
>>> +
>>> +#if defined(CONFIG_CMD_NET)
>>> +    unsigned int reg;
>>>      char *s, *e;
>>>      char i2c_mac[20];
>>> +    unsigned char mac[6];
>>> +#endif
>> These variable should be moved to "etheraddr" if-check scope
>> This will clean up having multiple #if defined(CONFIG_CMD_NET) checks
> 
> unsigned char mac[] has a larger scope so it is not moved. 
> Other variable definitions moved to "etheraddr" if-check scope as suggested
> 
>>>  
>>>      if ((argc > 3) || (argc < 2)) {
>>>          cmd_usage(cmdtp);
>>> @@ -244,6 +254,8 @@ int do_chip_config(cmd_tbl_t *cmdtp, int flag, int
>>> argc, char *argv[])
>>>          }
>>>  
>>>          return 0;
>>> +
>>> +#if defined(CONFIG_CMD_NET)
>>>      } else if (!strcmp(argv[1], "ethaddr")) {
>>>  
>>>          s = argv[2];
>>> @@ -255,6 +267,7 @@ int do_chip_config(cmd_tbl_t *cmdtp, int flag, int
>>> argc, char *argv[])
>>>          write_mac(mac);
>>>  
>>>          return 0;
>>> +#endif
>>>      } else if (!strcmp(argv[1], "print")) {
>>>  
>>>          if (chip->cpufreq == -1)
>>> @@ -274,13 +287,14 @@ int do_chip_config(cmd_tbl_t *cmdtp, int flag,
>>> int argc, char *argv[])
>>>          else
>>>              printf("DDR Type    = Not Known\n");
>>>  
>>> +#if defined(CONFIG_CMD_NET)
>>>          if (!i2c_read_mac(mac)) {
>>>              sprintf(i2c_mac, "%pM", mac);
>>>              printf("Ethaddr (from i2c mem) = %s\n", i2c_mac);
>>>          } else {
>>>              printf("Ethaddr (from i2c mem) = Not set\n");
>>>          }
>> From misc_init_r, the mac_id is stored in the enviromement.
>> Because the enviroment has precendence over the i2c.
>> Reporting the i2c mac address may be inconsistent or wrong.
>>
> The print in the chip_config print clearly says that this mac address 
> is set in i2c memory and in fact this is also the intention.
> 
> Probably, I should add a comment saying ethaddr in environment variable 
> may be different and it is the one used as mac id in network interface
> OR should I add a print itself that this is the mac id stored in i2c 
> memory and may be stale ?

The most useful one should be printed out.

If the user is setting up a network service list dhcp that depends on
the mac address, then the mac used by the u-boot network ip should be
returned.

If this is providing some board id and the i2c mac is unique, then
the i2c mac should be used.

What you don't want to happen is to confuse the user by reporting one
mac and using another.   Just printing out the the i2c mac may or may not
be stale will also confuse the user.

Tom


> 
>> Tom
>>
>>
>>> -
>>> +#endif
>>>          printf("Xloader Rev = %s\n", chip->version);
>>>  
>>>          return 0;
>>> @@ -293,4 +307,7 @@ int do_chip_config(cmd_tbl_t *cmdtp, int flag, int
>>> argc, char *argv[])
>>>  U_BOOT_CMD(chip_config, 3, 1, do_chip_config,
>>>         "configure chip",
>>>         "chip_config cpufreq/ddrfreq frequency\n"
>>> +#if defined(CONFIG_CMD_NET)
>>> +       "chip_config ethaddr XX:XX:XX:XX:XX:XX\n"
>>> +#endif
>>>         "chip_config print");
>>
> 
> 
>