[U-Boot,v3] net: allow setting env enetaddr from net device setting

From: Rob Herring <rob.herring@calxeda.com>

If the net driver has setup a valid ethernet address and an ethernet
address is not set in the environment already, then set the environment
variables from the net driver setting.

This enables pxe booting on boards which don't set ethaddr env variable.

Signed-off-by: Rob Herring <rob.herring@calxeda.com>
---
Wolfgang,

This now prints a warning. If both env and device mac's are set, then the
behavior is unchanged and the env setting is used.

v3:
- print a warning if using mac address from the net device

v2:
- Re-wrote to always setup ethaddr env variables

Rob

 doc/README.enetaddr |    4 +++-
 net/eth.c           |   21 ++++++++++++++++++---
 2 files changed, 21 insertions(+), 4 deletions(-)

Wolfgang, Mike,

On 02/01/2012 05:27 PM, Rob Herring wrote:
> From: Rob Herring <rob.herring@calxeda.com>
> 
> If the net driver has setup a valid ethernet address and an ethernet
> address is not set in the environment already, then set the environment
> variables from the net driver setting.
> 
> This enables pxe booting on boards which don't set ethaddr env variable.
> 
> Signed-off-by: Rob Herring <rob.herring@calxeda.com>
> ---
> Wolfgang,
> 
> This now prints a warning. If both env and device mac's are set, then the
> behavior is unchanged and the env setting is used.
> 
> v3:
> - print a warning if using mac address from the net device
> 
> v2:
> - Re-wrote to always setup ethaddr env variables
> 

Any comments on this?

Rob

> Rob
> 
>  doc/README.enetaddr |    4 +++-
>  net/eth.c           |   21 ++++++++++++++++++---
>  2 files changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/doc/README.enetaddr b/doc/README.enetaddr
> index 2d8e24f..6c61817 100644
> --- a/doc/README.enetaddr
> +++ b/doc/README.enetaddr
> @@ -32,7 +32,9 @@ Correct flow of setting up the MAC address (summarized):
>  
>  1. Read from hardware in initialize() function
>  2. Read from environment in net/eth.c after initialize()
> -3. Give priority to the value in the environment if a conflict
> +3. Write value to environment if setup in struct eth_device->enetaddr by driver
> +   initialize() function. Give priority to the value in the environment if a
> +   conflict.
>  4. Program the address into hardware if the following conditions are met:
>  	a) The relevant driver has a 'write_addr' function
>  	b) The user hasn't set an 'ethmacskip' environment variable
> diff --git a/net/eth.c b/net/eth.c
> index b4b9b43..f75a944 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -62,6 +62,15 @@ int eth_getenv_enetaddr_by_index(const char *base_name, int index,
>  	return eth_getenv_enetaddr(enetvar, enetaddr);
>  }
>  
> +int eth_setenv_enetaddr_by_index(const char *base_name, int index,
> +				 uchar *enetaddr)
> +{
> +	char enetvar[32];
> +	sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
> +	return eth_setenv_enetaddr(enetvar, enetaddr);
> +}
> +
> +
>  static int eth_mac_skip(int index)
>  {
>  	char enetvar[15];
> @@ -175,9 +184,15 @@ int eth_write_hwaddr(struct eth_device *dev, const char *base_name,
>  	unsigned char env_enetaddr[6];
>  	int ret = 0;
>  
> -	if (!eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr))
> -		return -1;
> -
> +	if (!eth_getenv_enetaddr_by_index(base_name, eth_number, env_enetaddr)) {
> +		if (!is_valid_ether_addr(dev->enetaddr))
> +			return -1;
> +		eth_setenv_enetaddr_by_index(base_name, eth_number,
> +					     dev->enetaddr);
> +		printf("\nWarning: %s using MAC address from net device\n",
> +			dev->name);
> +		return 0;
> +	}
>  	if (memcmp(env_enetaddr, "\0\0\0\0\0\0", 6)) {
>  		if (memcmp(dev->enetaddr, "\0\0\0\0\0\0", 6) &&
>  			memcmp(dev->enetaddr, env_enetaddr, 6)) {

Dear Rob Herring,

In message <1328138854-28612-1-git-send-email-robherring2@gmail.com> you wrote:
>
> --- a/doc/README.enetaddr
> +++ b/doc/README.enetaddr
> @@ -32,7 +32,9 @@ Correct flow of setting up the MAC address (summarized):
>  
>  1. Read from hardware in initialize() function
>  2. Read from environment in net/eth.c after initialize()
> -3. Give priority to the value in the environment if a conflict
> +3. Write value to environment if setup in struct eth_device->enetaddr by driver
> +   initialize() function. Give priority to the value in the environment if a
> +   conflict.

Sorry, but this description is not correct.  You say here that the
environment variable should always be written, but this is not the
case.  Only if it does not exist it shall be set.  If it exists, it
shall only be read, and in case of inconsistencies a warning shall be
printed.


Best regards,

Wolfgang Denk

On 03/06/2012 01:30 PM, Wolfgang Denk wrote:
> Dear Rob Herring,
> 
> In message <1328138854-28612-1-git-send-email-robherring2@gmail.com> you wrote:
>>
>> --- a/doc/README.enetaddr
>> +++ b/doc/README.enetaddr
>> @@ -32,7 +32,9 @@ Correct flow of setting up the MAC address (summarized):
>>  
>>  1. Read from hardware in initialize() function
>>  2. Read from environment in net/eth.c after initialize()
>> -3. Give priority to the value in the environment if a conflict
>> +3. Write value to environment if setup in struct eth_device->enetaddr by driver
>> +   initialize() function. Give priority to the value in the environment if a
>> +   conflict.
> 
> Sorry, but this description is not correct.  You say here that the
> environment variable should always be written, but this is not the
> case.  Only if it does not exist it shall be set.  If it exists, it
> shall only be read, and in case of inconsistencies a warning shall be
> printed.
> 

How about this:

3. Always use the value in the environment if there is a conflict. If
the environment variable is not set and the driver initialized struct
eth_device->enetaddr, then print a warning and set the environment
variable to initialized by the driver.

Rob

> 
> Best regards,
> 
> Wolfgang Denk
>

Dear Rob,

In message <4F566D05.5020809@gmail.com> you wrote:
>
> >> +3. Write value to environment if setup in struct eth_device->enetaddr by driver
> >> +   initialize() function. Give priority to the value in the environment if a
> >> +   conflict.
> > 
> > Sorry, but this description is not correct.  You say here that the
> > environment variable should always be written, but this is not the
> > case.  Only if it does not exist it shall be set.  If it exists, it
> > shall only be read, and in case of inconsistencies a warning shall be
> > printed.
> 
> How about this:
> 
> 3. Always use the value in the environment if there is a conflict. If
> the environment variable is not set and the driver initialized struct
> eth_device->enetaddr, then print a warning and set the environment
> variable to initialized by the driver.

I find you make it difficult to read without need by explaining it
backwards.

	The environment variable will be compared to the driver
	initialized struct eth_device->enetaddr. If they differ, a
	warning is printed, an the environment variable will be used
	unchanged.

	If the environment variable is not set, it will be initialized
	from eth_device->enetaddr, and a warning will be printed.

Best regards,

Wolfgang Denk