2010/11/15 Stefano Babic <sba...@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 <r64...@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: off...@denx.de
> =====================================================================
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
