| Message ID | 1289705187-11350-1-git-send-email-r64343@freescale.com |
|---|---|
| State | Superseded |
| Delegated to: | Stefano Babic |
| Headers | show |
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
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
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@denx.de > ===================================================================== > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
> >> 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@denx.de > ===================================================================== > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
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
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@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" >
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
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
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@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. 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
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@denx.de > ===================================================================== >
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); }
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(-)