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

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

[Jason Liu]

The patch is to support getting FEC MAC address from fuse bank.

Signed-off-by: Jason Liu <r64343@freescale.com>

---
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];
 };
 #endif
 
@@ -313,6 +309,6 @@ struct iim_regs {
 #define WSR_UNLOCK2		0xAAAA
 
 /* FUSE bank offsets */
-#define IIM0_MAC		0x1a
+#define IIM_MAC			0x1a
 
 #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];
 };
 #endif
 
@@ -513,7 +512,7 @@ struct iim_regs {
 #define IIM_ERR_PARITYE	(1 << 1)
 
 /* Definitions for i.MX27 TO2 */
-#define IIM0_MAC		5
+#define IIM_MAC			5
 #define IIM0_SCC_KEY		11
 #define IIM1_SUID		1
 
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
+
 #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];
+};
+
 #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;
 
 	for (i = 0; i < 6; i++)
+#ifdef CONFIG_MX27
 		mac[6-1-i] = readl(&iim->iim_bank_area0[IIM0_MAC + i]);
-
-	return !is_valid_ether_addr(mac);
+#else
+		mac[i] = readl(&iim->iim_bank_area[IIM_MAC + i]);
 #endif
+	return !is_valid_ether_addr(mac);
 }
 
 static int fec_set_hwaddr(struct eth_device *dev)
@@ -710,6 +705,7 @@ static int fec_probe(bd_t *bd)
 		puts("fec_mxc: not enough malloc memory\n");
 		return -ENOMEM;
 	}
+	memset(edev, 0, sizeof(*edev));
 	edev->priv = fec;
 	edev->init = fec_init;
 	edev->send = fec_send;
@@ -753,7 +749,7 @@ static int fec_probe(bd_t *bd)
 	eth_register(edev);
 
 	if (fec_get_hwaddr(edev, ethaddr) == 0) {
-		printf("got MAC address from EEPROM: %pM\n", ethaddr);
+		printf("got MAC address from FUSE: %pM\n", ethaddr);
 		memcpy(edev->enetaddr, ethaddr, 6);
 	}
 
-- 
1.7.0.4

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

[Wolfgang Denk]

Dear Jason Liu,

In message <1289705187-11350-1-git-send-email-r64343@freescale.com> you wrote:
> The patch is to support getting FEC MAC address from fuse bank.
> 
> Signed-off-by: Jason Liu <r64343@freescale.com>

Please add the i.MX custodian on Cc:

> 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];

Please explain what exactly you are doing here.

> --- 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];

... and here.

What makes me suspicious is that you are changing the size of these
structs here - from 4224 to 5120 for i.MX25, and from 3068 to 2108 for
i.MX27.

Here it becomes more, there it becomes less?

Is this correct?

> 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
> +
>  #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];
> +};

And here we have 6144. Is this correct?

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
Never put off until tomorrow what you can put off indefinitely.

[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@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
=====================================================================

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

[Jason Liu]

2010/11/15 Stefano Babic <sbabic@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@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
>

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

[Jason Liu]

>
>> 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.
>

Do you have any good method to put the mac address into one structure
and use that filed to access it?
Currently, we only use the MAC fuse, there is a lot of fuse not used?
if we defined as bellow,  what your comments?

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 res0[0x1f1];
        u32 iim_bank_area0_1[9];
        u32 mac_addr[6];
        u32 iim_bank_area0_2[0x11];
        u32 res1[0xe0];
        u32 iim_bank_area1[0x20];
        u32 res2[0xe0];
        u32 iim_bank_area2[0x20];
        u32 res3[0xe0];
        u32 iim_bank_area3[0x20];
        u32 res4[0xe0];
};

BR,
Jason

>
> --
> =====================================================================
> 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
>

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

[Wolfgang Denk]

Dear Jason Liu,

In message <AANLkTimXrZANNdroChBFpOhu13VUWXsBdOdBUUn5fBYj@mail.gmail.com> you wrote:
>
> Do you have any good method to put the mac address into one structure
> and use that filed to access it?
> Currently, we only use the MAC fuse, there is a lot of fuse not used?
> if we defined as bellow,  what your comments?
> 
> 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 res0[0x1f1];
>         u32 iim_bank_area0_1[9];
>         u32 mac_addr[6];
>         u32 iim_bank_area0_2[0x11];
>         u32 res1[0xe0];
>         u32 iim_bank_area1[0x20];
>         u32 res2[0xe0];
>         u32 iim_bank_area2[0x20];
>         u32 res3[0xe0];
>         u32 iim_bank_area3[0x20];
>         u32 res4[0xe0];
> };

This struct is supposed to describe some register layout defined by
the hardware - but does the hardware know anything about teh special
meaning "MAC address" of this specific field, or is this an
interpretation that we just define as we like?

If this is not a hardware given thing, we should not define this in
iim_regs; instead, we should define a separate struct that describes
how we decided to use iim_bank_area0.

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
"It follows that any commander in chief who undertakes to carry out a
plan which he considers defective is at fault; he must put forth  his
reasons,  insist  of  the  plan being changed, and finally tender his
resignation rather than be the instrument of his army's downfall."
- Napoleon, "Military Maxims and Thought"

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

[Jason Liu]

2010/11/16 Wolfgang Denk <wd@denx.de>:
> Dear Jason Liu,
>
> In message <AANLkTimXrZANNdroChBFpOhu13VUWXsBdOdBUUn5fBYj@mail.gmail.com> you wrote:
>>
>> Do you have any good method to put the mac address into one structure
>> and use that filed to access it?
>> Currently, we only use the MAC fuse, there is a lot of fuse not used?
>> if we defined as bellow, ?what your comments?
>>
>> 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 res0[0x1f1];
>> ? ? ? ? u32 iim_bank_area0_1[9];
>> ? ? ? ? u32 mac_addr[6];
>> ? ? ? ? u32 iim_bank_area0_2[0x11];
>> ? ? ? ? u32 res1[0xe0];
>> ? ? ? ? u32 iim_bank_area1[0x20];
>> ? ? ? ? u32 res2[0xe0];
>> ? ? ? ? u32 iim_bank_area2[0x20];
>> ? ? ? ? u32 res3[0xe0];
>> ? ? ? ? u32 iim_bank_area3[0x20];
>> ? ? ? ? u32 res4[0xe0];
>> };
>
> This struct is supposed to describe some register layout defined by
> the hardware - but does the hardware know anything about teh special
> meaning "MAC address" of this specific field, or is this an
> interpretation that we just define as we like?

This is defined according to the IC spec and it should be hw layout.
We can read such kind of fuse just like read register.  But currently, uboot
require that all the register array should be included into one
structure and not
use the *(offset + base_addr) method, this is also stefano suggestion. But for
fuse support, there are many fuses but we only use FEC_MAC fuse
currently, this is why
I ask how to handle it better in uboot.

Stefano, what's your comments?

Thanks for your help.

>
> If this is not a hardware given thing, we should not define this in
> iim_regs; instead, we should define a separate struct that describes
> how we decided to use iim_bank_area0.
>
> 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
> "It follows that any commander in chief who undertakes to carry out a
> plan which he considers defective is at fault; he must put forth ?his
> reasons, ?insist ?of ?the ?plan being changed, and finally tender his
> resignation rather than be the instrument of his army's downfall."
> - Napoleon, "Military Maxims and Thought"
>

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

[Stefano Babic]

On 11/16/2010 08:16 AM, Jason Liu wrote:
>>> 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 res0[0x1f1];
>>>         u32 iim_bank_area0_1[9];
>>>         u32 mac_addr[6];
>>>         u32 iim_bank_area0_2[0x11];
>>>         u32 res1[0xe0];
>>>         u32 iim_bank_area1[0x20];
>>>         u32 res2[0xe0];
>>>         u32 iim_bank_area2[0x20];
>>>         u32 res3[0xe0];
>>>         u32 iim_bank_area3[0x20];
>>>         u32 res4[0xe0];
>>> };
>>
>> This struct is supposed to describe some register layout defined by
>> the hardware - but does the hardware know anything about teh special
>> meaning "MAC address" of this specific field, or is this an
>> interpretation that we just define as we like?

The MAC address is stored at a specific address, that is different in
each i.MX implementation. There are a lot of fields the hardware is
aware of.

> 
> This is defined according to the IC spec and it should be hw layout.
> We can read such kind of fuse just like read register.  But currently, uboot
> require that all the register array should be included into one
> structure and not
> use the *(offset + base_addr) method, this is also stefano suggestion. But for
> fuse support, there are many fuses but we only use FEC_MAC fuse
> currently, this is why
> I ask how to handle it better in uboot.
> 
> Stefano, what's your comments?

I would like to propose a structure to better clarify the internal
layout. Let me explain and check if this can be suitable for you. We
could use a union to define the different layouts for the fuse banks,
such as (the example is for the i.MX51):

typedef union fuse_bank {
	struct {
		u32 fuse_lock;
		u32 osc_jtag;
		u32 reserved0;
		u32 bt
		.....
		u32 mac_addr[6];
		....
		u32 reserved_filled[..]; /* to fill the 0x80-0xFF*/
	};
	/* Now the layout for the other fuse banks */
	struct {
		....
	};
	/*
	 * If we do not want to set now the layout, we can distinguish
	 * only between real register and reserved addresses
	 * as you already did
	 */
	 struct {
		u32 fuse_regs[0x20];
		u32 reserved[0xe0];
	}
}

And in your structure you can have something like this:

        ....
        u32 scs3;
        u32 res0[0x1f1];
	fuse_bank fuses[4];
}

You do not need to set the layout for the fuse banks 1-3, if you do not
want, but the structure is prepared if someone else will add functions
to manage the fuses.

What do you think about it ?

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] [PATCH v2 1/1] imx: Get fec mac address from fuse

[Jason Liu]

2010/11/16 Stefano Babic <sbabic@denx.de>:
> I would like to propose a structure to better clarify the internal
> layout. Let me explain and check if this can be suitable for you. We
> could use a union to define the different layouts for the fuse banks,
> such as (the example is for the i.MX51):
>
> typedef union fuse_bank {
> ? ? ? ?struct {
> ? ? ? ? ? ? ? ?u32 fuse_lock;
> ? ? ? ? ? ? ? ?u32 osc_jtag;
> ? ? ? ? ? ? ? ?u32 reserved0;
> ? ? ? ? ? ? ? ?u32 bt
> ? ? ? ? ? ? ? ?.....
> ? ? ? ? ? ? ? ?u32 mac_addr[6];
> ? ? ? ? ? ? ? ?....
> ? ? ? ? ? ? ? ?u32 reserved_filled[..]; /* to fill the 0x80-0xFF*/
> ? ? ? ?};
> ? ? ? ?/* Now the layout for the other fuse banks */
> ? ? ? ?struct {
> ? ? ? ? ? ? ? ?....
> ? ? ? ?};
> ? ? ? ?/*
> ? ? ? ? * If we do not want to set now the layout, we can distinguish
> ? ? ? ? * only between real register and reserved addresses
> ? ? ? ? * as you already did
> ? ? ? ? */
> ? ? ? ? struct {
> ? ? ? ? ? ? ? ?u32 fuse_regs[0x20];
> ? ? ? ? ? ? ? ?u32 reserved[0xe0];
> ? ? ? ?}
> }
>
> And in your structure you can have something like this:
>
> ? ? ? ?....
> ? ? ? ?u32 scs3;
> ? ? ? ?u32 res0[0x1f1];
> ? ? ? ?fuse_bank fuses[4];
> }
>
> You do not need to set the layout for the fuse banks 1-3, if you do not
> want, but the structure is prepared if someone else will add functions
> to manage the fuses.
>
> What do you think about it ?

It's OK, I think. But there will change a lot of code for the platform
other than i.mx51.
In fact, the rule of my every commit patch is to solve one problem or
add one feature with the minimum code change to the exist code  base.
If need some code clean-up or restructure, then use another commit to
fix it. This will give us a clear track of the code change. If you
insist on changing it in this patch, I will follow your rule to change
it.

BR,
Jason

>
> 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] [PATCH v2 1/1] imx: Get fec mac address from fuse

[Stefano Babic]

On 11/17/2010 03:22 AM, Jason Liu wrote:

> It's OK, I think. But there will change a lot of code for the platform
> other than i.mx51.

You have already posted changes for the whole i.MX family we currently
support.

However, I understand you do not want to specify the whole layout for
all processors, that means more effort. At least you must specify in the
structure where we cann access the mac address.

You could do in this way

typedef union fuse_bank {
        struct {
                u32 fuses_0[..]
                u32 mac_addr[6];
                u32 other_fuses[...];
                u32 reserved_filled[..]; /* to fill the 0x80-0xFF*/
        };
        /*
         * If we do not want to set now the layout, we can distinguish
         * only between real register and reserved addresses
         * as you already did
         */
         struct {
                u32 fuse_regs[0x20];
                u32 reserved[0xe0];
        }
 }

This still allows to have an array of fuse_banks in the iim structure.

When someone will implement functions to manage the fuses, he will
change the fuse_bank structure naming all fields as specified in the
i.MX manuals.

> In fact, the rule of my every commit patch is to solve one problem or
> add one feature with the minimum code change to the exist code  base.

To solve one single problem, yes. With minimum code change, no. This
rule does not apply. Changing code searching for the best solution, yes.
Even if we have to change much more code.

> If need some code clean-up or restructure, then use another commit to
> fix it. This will give us a clear track of the code change. If you
> insist on changing it in this patch, I will follow your rule to change
> it.

Check my proposal. I think you need to change slightly the code.

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] [PATCH v2 1/1] imx: Get fec mac address from fuse

[Jason Liu]

2010/11/17 Stefano Babic <sbabic@denx.de>:
> On 11/17/2010 03:22 AM, Jason Liu wrote:
>
>> It's OK, I think. But there will change a lot of code for the platform
>> other than i.mx51.
>
> You have already posted changes for the whole i.MX family we currently
> support.
>
> However, I understand you do not want to specify the whole layout for
> all processors, that means more effort. At least you must specify in the
> structure where we cann access the mac address.
>
> You could do in this way
>
> typedef union fuse_bank {
> ? ? ? ?struct {
> ? ? ? ? ? ? ? ?u32 fuses_0[..]
> ? ? ? ? ? ? ? ?u32 mac_addr[6];
> ? ? ? ? ? ? ? ?u32 other_fuses[...];
> ? ? ? ? ? ? ? ?u32 reserved_filled[..]; /* to fill the 0x80-0xFF*/
> ? ? ? ?};
> ? ? ? ?/*
> ? ? ? ? * If we do not want to set now the layout, we can distinguish
> ? ? ? ? * only between real register and reserved addresses
> ? ? ? ? * as you already did
> ? ? ? ? */
> ? ? ? ? struct {
> ? ? ? ? ? ? ? ?u32 fuse_regs[0x20];
> ? ? ? ? ? ? ? ?u32 reserved[0xe0];
> ? ? ? ?}
> ?}
>
> This still allows to have an array of fuse_banks in the iim structure.

what about the following code snip which consider you and Wolfgang's proposal.

+struct fuse_bank {
+       u32     used[0x20];
+       u32     resv[0xe0];
+};
+
+struct fuse_bank1 {
+       u32     fuse_prot;
+       u32     srk_hash;
+       u32     sjc_resp[7];
+       u32     mac_addr[6];
+       u32     other_fuse[...]
+       u32     reserved_filled[...];
+};
+
+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     res0[0x1f1];
+       struct  fuse_bank fuse_banks[4];
+};
+

>
> When someone will implement functions to manage the fuses, he will
> change the fuse_bank structure naming all fields as specified in the
> i.MX manuals.

OK, agree.

>
>> In fact, the rule of my every commit patch is to solve one problem or
>> add one feature with the minimum code change to the exist code ?base.
>
> To solve one single problem, yes. With minimum code change, no. This
> rule does not apply. Changing code searching for the best solution, yes.
> Even if we have to change much more code.

This is different with my working style. OK, I will take care later.

>
>> If need some code clean-up or restructure, then use another commit to
>> fix it. This will give us a clear track of the code change. If you
>> insist on changing it in this patch, I will follow your rule to change
>> it.
>
> Check my proposal. I think you need to change slightly the code.

OK, thanks for your proposal.

>
> 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] [PATCH v2 1/1] imx: Get fec mac address from fuse

[Wolfgang Denk]

Dear Jason Liu,

In message <AANLkTi=NZurD4NkjCC6+v0A5rKmXoG6tpFZQqhkyFmFH@mail.gmail.com> you wrote:
>
...
> This is defined according to the IC spec and it should be hw layout.
> We can read such kind of fuse just like read register.  But currently, uboot
> require that all the register array should be included into one
> structure and not
> use the *(offset + base_addr) method, this is also stefano suggestion. But for
> fuse support, there are many fuses but we only use FEC_MAC fuse
> currently, this is why
> I ask how to handle it better in uboot.

I still suggest to have an unstructured entry iim_bank_area0[] in
iim_regs; actually this allows to collapse the 4 areas into an array
of 4 identical ones.

Then provide a separate struct description which declares the usage on
iim_bank_area0.


Um... and can you please stop top-posting / full-quoting? Try and read
http://www.netmeister.org/news/learn2quote.html

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
 The software required `Windows 95 or better', so I installed Linux.