Re: [U-Boot] [PATCH v2 04/17] SPEAr : Placing ethaddr write and read within CONFIG_CMD_NET
Dear Vipin KUMAR, In message <4be24179.6010...@st.com> you wrote: > > I get your point. Though, I believe, educating the user about the mac id > storage behavior is the solution in this case. > Moreover, ethaddr is not set in environment variables by default. So, Yes, it is. Please see the docs. > the decision remains with the user as to where he wants to store the > ethaddr environment. This command has been introduced to report and save > mac id in i2c memory. > I would add a section regarding mac id storage and reporting in README.spear Please don't create any special behaviour. Do like everybody else. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de COBOL is for morons. -- E.W. Dijkstra ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 04/17] SPEAr : Placing ethaddr write and read within CONFIG_CMD_NET
On 5/4/2010 5:48 PM, Tom Rix wrote:
+#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.
I get your point. Though, I believe, educating the user about the mac id
storage behavior is the solution in this case.
Moreover, ethaddr is not set in environment variables by default. So,
the decision remains with the user as to where he wants to store the
ethaddr environment. This command has been introduced to report and save
mac id in i2c memory.
I would add a section regarding mac id storage and reporting in README.spear
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 04/17] SPEAr : Placing ethaddr write and read within CONFIG_CMD_NET
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
>>> ---
>>> 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");
>>
>
>
>
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 04/17] SPEAr : Placing ethaddr write and read within CONFIG_CMD_NET
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
>> ---
>> 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 ?
> 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");
>
>
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 04/17] SPEAr : Placing ethaddr write and read within CONFIG_CMD_NET
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
> ---
> 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
>
> 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.
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");
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v2 04/17] SPEAr : Placing ethaddr write and read within CONFIG_CMD_NET
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
---
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
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");
}
-
+#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");
--
1.6.0.2
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
