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

[Jason Liu]

2010/11/15 Stefano Babic <sbabic at denx.de>:
> 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 ?

I will consider your comments and do some change accordingly. Thanks,

>
> 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
> =====================================================================
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>