[U-Boot,6/8] AM35xx: Read and set ethaddr for EMAC

From: Steve Kipisz <s-kipisz2@ti.com>

Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
Signed-off-by: Tom Rini <trini@ti.com>
---
 arch/arm/cpu/armv7/omap3/emac.c             |   20 +++++++++++++++++++-
 arch/arm/include/asm/arch-omap3/emac_defs.h |    3 +++
 2 files changed, 22 insertions(+), 1 deletions(-)

Hi Steve, Tom,

On 12/06/11 17:49, Tom Rini wrote:
> From: Steve Kipisz <s-kipisz2@ti.com>
> 
> Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
> Signed-off-by: Tom Rini <trini@ti.com>
> ---
>  arch/arm/cpu/armv7/omap3/emac.c             |   20 +++++++++++++++++++-
>  arch/arm/include/asm/arch-omap3/emac_defs.h |    3 +++
>  2 files changed, 22 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/omap3/emac.c b/arch/arm/cpu/armv7/omap3/emac.c
> index 14667f1..d400bef 100644
> --- a/arch/arm/cpu/armv7/omap3/emac.c
> +++ b/arch/arm/cpu/armv7/omap3/emac.c
> @@ -26,6 +26,7 @@
>  #include <netdev.h>
>  #include <asm/io.h>
>  #include <asm/arch/am35x_def.h>
> +#include <asm/arch/emac_defs.h>
>  
>  /*
>   * Initializes on-chip ethernet controllers.
> @@ -33,12 +34,29 @@
>   */
>  int cpu_eth_init(bd_t *bis)
>  {
> -	u32 reset;
> +	u32 reset, msb, lsb;
> +	u_int8_t macaddr[6];
> +	int i;
>  
>  	/* ensure that the module is out of reset */
>  	reset = readl(&am35x_scm_general_regs->ip_sw_reset);
>  	reset &= ~CPGMACSS_SW_RST;
>  	writel(reset, &am35x_scm_general_regs->ip_sw_reset);
>  
> +	/* Read MAC address */
> +	msb = readl(EMAC_MACADDR_MSB);
> +	lsb = readl(EMAC_MACADDR_LSB);
> +
> +	for (i = 0; i < 3; i++) {
> +		macaddr[5 - i] = lsb & 0xFF;
> +		lsb >>= 8;
> +	}
> +
> +	for (i = 0; i < 3; i++) {
> +		macaddr[2 - i] = msb & 0xFF;
> +		msb >>= 8;
> +	}
> +	eth_setenv_enetaddr("ethaddr", macaddr);
> +

This is a wrong place for this code...
You force every board to use the EFUSE'd MAC address - this is wrong.
The board must have a freedom to choose what MAC address it wants to use.
You don't even check if ethaddr variable is already set...

What you can do is put this implementation into a separate function
and let board to make a decision if it wants to call it or use another
MAC address.
Something like:
int am3517_get_efuse_enetaddr(u8 *enetaddr)
{
	/* read the address and shift or whatever */
	...
	return is_valid_ether_addr(enetaddr);
}

>  	return davinci_emac_initialize();
>  }
> diff --git a/arch/arm/include/asm/arch-omap3/emac_defs.h b/arch/arm/include/asm/arch-omap3/emac_defs.h
> index 8506c55..c3c96c0 100644
> --- a/arch/arm/include/asm/arch-omap3/emac_defs.h
> +++ b/arch/arm/include/asm/arch-omap3/emac_defs.h
> @@ -42,6 +42,9 @@
>  #define EMAC_MDIO_BASE_ADDR            0x5C030000
>  #define EMAC_HW_RAM_ADDR               0x01E20000
>  
> +#define EMAC_MACADDR_LSB	       0x48002380
> +#define EMAC_MACADDR_MSB	       0x48002384
> +
>  #define EMAC_MDIO_BUS_FREQ             166000000       /* 166 MHZ check */
>  #define EMAC_MDIO_CLOCK_FREQ           1000000         /* 2.0 MHz */
>

On 12/07/2011 07:48 AM, Igor Grinberg wrote:
> Hi Steve, Tom,
> 
> On 12/06/11 17:49, Tom Rini wrote:
>> From: Steve Kipisz <s-kipisz2@ti.com>
>>
>> Signed-off-by: Steve Kipisz <s-kipisz2@ti.com>
>> Signed-off-by: Tom Rini <trini@ti.com>
>> ---
>>  arch/arm/cpu/armv7/omap3/emac.c             |   20 +++++++++++++++++++-
>>  arch/arm/include/asm/arch-omap3/emac_defs.h |    3 +++
>>  2 files changed, 22 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/arm/cpu/armv7/omap3/emac.c b/arch/arm/cpu/armv7/omap3/emac.c
>> index 14667f1..d400bef 100644
>> --- a/arch/arm/cpu/armv7/omap3/emac.c
>> +++ b/arch/arm/cpu/armv7/omap3/emac.c
>> @@ -26,6 +26,7 @@
>>  #include <netdev.h>
>>  #include <asm/io.h>
>>  #include <asm/arch/am35x_def.h>
>> +#include <asm/arch/emac_defs.h>
>>  
>>  /*
>>   * Initializes on-chip ethernet controllers.
>> @@ -33,12 +34,29 @@
>>   */
>>  int cpu_eth_init(bd_t *bis)
>>  {
>> -	u32 reset;
>> +	u32 reset, msb, lsb;
>> +	u_int8_t macaddr[6];
>> +	int i;
>>  
>>  	/* ensure that the module is out of reset */
>>  	reset = readl(&am35x_scm_general_regs->ip_sw_reset);
>>  	reset &= ~CPGMACSS_SW_RST;
>>  	writel(reset, &am35x_scm_general_regs->ip_sw_reset);
>>  
>> +	/* Read MAC address */
>> +	msb = readl(EMAC_MACADDR_MSB);
>> +	lsb = readl(EMAC_MACADDR_LSB);
>> +
>> +	for (i = 0; i < 3; i++) {
>> +		macaddr[5 - i] = lsb & 0xFF;
>> +		lsb >>= 8;
>> +	}
>> +
>> +	for (i = 0; i < 3; i++) {
>> +		macaddr[2 - i] = msb & 0xFF;
>> +		msb >>= 8;
>> +	}
>> +	eth_setenv_enetaddr("ethaddr", macaddr);
>> +
> 
> This is a wrong place for this code...
> You force every board to use the EFUSE'd MAC address - this is wrong.
> The board must have a freedom to choose what MAC address it wants to use.
> You don't even check if ethaddr variable is already set...
> 
> What you can do is put this implementation into a separate function
> and let board to make a decision if it wants to call it or use another
> MAC address.
> Something like:
> int am3517_get_efuse_enetaddr(u8 *enetaddr)
> {
> 	/* read the address and shift or whatever */
> 	...
> 	return is_valid_ether_addr(enetaddr);
> }

That's reasonable enough.  I wasn't sure where the custom boards were
getting their MAC as the EVM and Crane both need this change.

Dear Tom Rini,

In message <4EDF7E34.1070007@ti.com> you wrote:
>
> > This is a wrong place for this code...
> > You force every board to use the EFUSE'd MAC address - this is wrong.
> > The board must have a freedom to choose what MAC address it wants to use.
> > You don't even check if ethaddr variable is already set...
> > 
> > What you can do is put this implementation into a separate function
> > and let board to make a decision if it wants to call it or use another
> > MAC address.
> > Something like:
> > int am3517_get_efuse_enetaddr(u8 *enetaddr)
> > {
> > 	/* read the address and shift or whatever */
> > 	...
> > 	return is_valid_ether_addr(enetaddr);
> > }
> 
> That's reasonable enough.  I wasn't sure where the custom boards were
> getting their MAC as the EVM and Crane both need this change.

The rule is that the settings in the environment always have
precedence, and you must never overwrite an existing "eth*addr" in
the environment.

Best regards,

Wolfgang Denk