[U-Boot] [PATCH v2 1/1] imx: Get fec mac address from fuse

[Stefano Babic]

On 11/14/2010 04:26 AM, Jason Liu wrote:
> The patch is to support getting FEC MAC address from fuse bank.
> 
> Signed-off-by: Jason Liu <r64343 at freescale.com>
> 

Hi Jason,

> ---
> Changes for v2:
>   - correct the mac address byte order according to review comments
>   - add memset(edev, 0. sizeof(*edev)) when do fec_probe,
>   - fix some code comments
> ---
>  arch/arm/include/asm/arch-mx25/imx-regs.h |   10 +++-------
>  arch/arm/include/asm/arch-mx27/imx-regs.h |    5 ++---
>  arch/arm/include/asm/arch-mx5/imx-regs.h  |   24 ++++++++++++++++++++++++
>  drivers/net/fec_mxc.c                     |   16 ++++++----------
>  4 files changed, 35 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch-mx25/imx-regs.h b/arch/arm/include/asm/arch-mx25/imx-regs.h
> index f5a2929..6afcfdf 100644
> --- a/arch/arm/include/asm/arch-mx25/imx-regs.h
> +++ b/arch/arm/include/asm/arch-mx25/imx-regs.h
> @@ -128,12 +128,8 @@ struct iim_regs {
>  	u32 iim_prev;
>  	u32 iim_srev;
>  	u32 iim_prog_p;
> -	u32 res1[0x1f5];
> -	u32 iim_bank_area0[0x20];
> -	u32 res2[0xe0];
> -	u32 iim_bank_area1[0x20];
> -	u32 res3[0xe0];
> -	u32 iim_bank_area2[0x20];
> +	u32 res[0x1f5];
> +	u32 iim_bank_area[0x100 * 3];

As it is set now, it is clear that each bank has only 0x20 word (with
offset 0-0x7C. In your patch, it seems that any address is part of the
fuse bank, and that is not true. I think this makes more confusion as now.

>  
>  #endif				/* _IMX_REGS_H */
> diff --git a/arch/arm/include/asm/arch-mx27/imx-regs.h b/arch/arm/include/asm/arch-mx27/imx-regs.h
> index 6ecddaa..429f893 100644
> --- a/arch/arm/include/asm/arch-mx27/imx-regs.h
> +++ b/arch/arm/include/asm/arch-mx27/imx-regs.h
> @@ -202,8 +202,7 @@ struct iim_regs {
>  	u32 iim_scs1;
>  	u32 iim_scs2;
>  	u32 iim_scs3;
> -	u32 res[0x1F0];
> -	u32 iim_bank_area0[0x100];
> +	u32 iim_bank_area[0x100 * 2];

Ditto. As I see for imx27, fuse bank 0 has offsets 0x8000-0x807c, and
fuse bank 1 0x8800-0x887c. I think it should be better to make clear
that there are no register in the range 0x80-0xFF. This address range
should be declared as reserved.

> diff --git a/arch/arm/include/asm/arch-mx5/imx-regs.h b/arch/arm/include/asm/arch-mx5/imx-regs.h
> index 0b6249a..93eef48 100644
> --- a/arch/arm/include/asm/arch-mx5/imx-regs.h
> +++ b/arch/arm/include/asm/arch-mx5/imx-regs.h
> @@ -205,6 +205,9 @@
>  #define BOARD_REV_1_0           0x0
>  #define BOARD_REV_2_0           0x1
>  
> +#define IMX_IIM_BASE		(IIM_BASE_ADDR)
> +#define IIM_MAC			0x109

Propably we can find a way to use a structure to access to the mac
address, see my last point.

> +
>  #if !(defined(__KERNEL_STRICT_NAMES) || defined(__ASSEMBLY__))
>  #include <asm/types.h>
>  
> @@ -275,6 +278,27 @@ struct src {
>  	u32	sisr;
>  	u32	simr;
>  };
> +
> +struct iim_regs {
> +	u32 stat;
> +	u32 statm;
> +	u32 err;
> +	u32 emask;
> +	u32 fctl;
> +	u32 ua;
> +	u32 la;
> +	u32 sdat;
> +	u32 prev;
> +	u32 srev;
> +	u32 preg_p;
> +	u32 scs0;
> +	u32 scs1;
> +	u32 scs2;
> +	u32 scs3;
> +	u32 res[0x1f1];
> +	u32 iim_bank_area[0x100 * 4];

I can match the offsets with the manual, but it seems hard to
understand. From your structure it seems we have 4 banks (this is true),
each of them has 100 words (this is not true). According to the manual,
each bank has 0x7C words and the reset is not available and should be
set as reserved, as stated for the other processors.

> +};
> +
>  #endif /* __ASSEMBLER__*/
>  
>  #endif				/*  __ASM_ARCH_MXC_MX51_H__ */
> diff --git a/drivers/net/fec_mxc.c b/drivers/net/fec_mxc.c
> index 3f09c2b..1e6ba06 100644
> --- a/drivers/net/fec_mxc.c
> +++ b/drivers/net/fec_mxc.c
> @@ -312,21 +312,16 @@ static void fec_rbd_clean(int last, struct fec_bd *pRbd)
>  
>  static int fec_get_hwaddr(struct eth_device *dev, unsigned char *mac)
>  {
> -/*
> - * The MX27 can store the mac address in internal eeprom
> - * This mechanism is not supported now by MX51 or MX25
> - */
> -#if defined(CONFIG_MX51) || defined(CONFIG_MX25)
> -	return -1;
> -#else
>  	struct iim_regs *iim = (struct iim_regs *)IMX_IIM_BASE;
>  	int i;

I understand what you mean, but what about to have a private function
(defined as inline function) in each imx-regs.h file ? Then you have
more freedom to implement differently the way to access to the IIM, as
required by the different i.MX implementations. You can then declare the
IIM structures specific for each processor, while now you use them as a
whole big buffer. In this way, we can get rid of accessing to the MAC in
the fuse with some magic offset numbers, as it is done now.

I'll try to explain better: something like imx_get_mac_from_fuse(), that
is called inside fec_get_hwaddr(), but declared independently inside
each imx-regs.h to make it specific for each processor, because it
accesses to different structures.

What do you mean ?

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================