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

[Wolfgang Denk]

Dear Jason Liu,

In message <1290002970-8002-1-git-send-email-r64343 at freescale.com> you wrote:
> The patch is to support getting FEC MAC address from fuse bank.
> 
> Signed-off-by: Jason Liu <r64343 at freescale.com>
...
> --- a/arch/arm/cpu/arm926ejs/mx27/generic.c
> +++ b/arch/arm/cpu/arm926ejs/mx27/generic.c
> @@ -313,6 +313,16 @@ void mx27_fec_init_pins(void)
>  	for (i = 0; i < ARRAY_SIZE(mode); i++)
>  		imx_gpio_mode(mode[i]);
>  }
> +
> +void imx_get_mac_from_fuse(unsigned char *mac)
> +{
> +	int i;
> +	struct iim_regs *iim = (struct iim_regs *)IMX_IIM_BASE;
> +	struct fuse_bank0 *fuse = (struct fuse_bank0 *)&iim->bank[0];

I think this is wrong. You don't want the address of the bank, but of
the "fuse_regs" element.

> +        for (i = 0; i < 6; i++)
> +		mac[6-1-i] = readl(&fuse->mac_addr[i]);

Please use TABs for indentation.

> +#if defined(CONFIG_FEC_MXC)
> +void imx_get_mac_from_fuse(unsigned char *mac)
> +{
> +	int i;
> +	struct iim_regs *iim = (struct iim_regs *)IMX_IIM_BASE;
> +	struct fuse_bank1 *fuse = (struct fuse_bank1 *)&iim->bank[1];

Again, this shouldbe "fuse_regs".

> +        for (i = 0; i < 6; i++)
> +                mac[i] = readl(&fuse->mac_addr[i]);

Ditto.

> +	struct {
> +		u32 fuse_regs[0x20];
> +		u32 fuse_rsvd[0xe0];
> +	} bank[3];
>  };
> +
> +struct fuse_bank0 {
> +	u32 fuse1_26[0x1a];
> +	u32 mac_addr[6];
> +};

This is misleading. "fuse_bank0" does not descibe the whole bank0, but
only the "fuse_regs" part of it, so you better rename this into
"fuse_bank0_regs" or similar.

"fuse1_26" is a strange name. I think I understand what you mean, but
then it should probably be "fuse0_25" ?  or "fuse0_0x19"? 
Or "unused" or "reserved" or ... ?

> +struct fuse_bank0 {
> +	u32 fuse1_4[4];
> +	u32 mac_addr[6];
> +	u32 word11_32[0x16];
>  };

Please use consistent names. "word..." does not fit here at all.

Again, think the names fuse1_4 and fuse11_32 are probaly misleading
(0 - 3 and 10 - 31).

And also, I think this should be "fuse_bank0_fuse_regs".

> +struct fuse_bank1 {
> +	u32	fuse1_9[9];
> +	u32 	mac_addr[6];
> +	u32 	fuse6_32[0x11];
> +};

Same comments again.


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: wd at denx.de
You have the capacity to learn from  mistakes.  You'll  learn  a  lot
today.