Re: [PATCH 2/2] Re-added support for FEC on MPC5121 from Freescale LTIB to current head
On Jun 12, 2008, at 6:45 AM, David Jander wrote: Your commit message isn't exactly helpful as most people dont know what LTIB is and its not terribly relevant. It just seems like you are adding support for the FEC on MPC5121 and this point. Signed-off-by: David Jander <[EMAIL PROTECTED]> --- arch/powerpc/platforms/Kconfig |2 +- drivers/net/fec.h | 43 drivers/net/fs_enet/Kconfig| 22 +- drivers/net/fs_enet/fs_enet-main.c | 76 +++ +++-- drivers/net/fs_enet/fs_enet.h | 17 +--- drivers/net/fs_enet/mac-fec.c | 22 +- drivers/net/fs_enet/mii-fec.c | 10 - 7 files changed, 167 insertions(+), 25 deletions(-) diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/ Kconfig index 87454c5..a96937f 100644 --- a/arch/powerpc/platforms/Kconfig +++ b/arch/powerpc/platforms/Kconfig @@ -288,7 +288,7 @@ config CPM2 config PPC_CPM_NEW_BINDING bool - depends on CPM1 || CPM2 + depends on CPM1 || CPM2 || FS_ENET_MPC5121_FEC default y config AXON_RAM diff --git a/drivers/net/fec.h b/drivers/net/fec.h index 292719d..5c9fe34 100644 --- a/drivers/net/fec.h +++ b/drivers/net/fec.h @@ -59,6 +59,7 @@ typedef struct fec { } fec_t; #else +#if !defined(CONFIG_FS_ENET_MPC5121_FEC) /* * Define device register set address map. @@ -97,6 +98,48 @@ typedef struct fec { unsigned long fec_fifo_ram[112]; /* FIFO RAM buffer */ } fec_t; +#else /* CONFIG_FS_ENET_MPC5121_FEC */ + +typedef struct fec { + u32 fec_reserved0; + u32 fec_ievent; /* Interrupt event reg */ + u32 fec_imask; /* Interrupt mask reg */ + u32 fec_reserved1; + u32 fec_r_des_active; /* Receive descriptor reg */ + u32 fec_x_des_active; /* Transmit descriptor reg */ + u32 fec_reserved2[3]; + u32 fec_ecntrl; /* Ethernet control reg */ + u32 fec_reserved3[6]; + u32 fec_mii_data; /* MII manage frame reg */ + u32 fec_mii_speed; /* MII speed control reg */ + u32 fec_reserved4[7]; + u32 fec_mib_ctrlstat; /* MIB control/status reg */ + u32 fec_reserved5[7]; + u32 fec_r_cntrl;/* Receive control reg */ + u32 fec_reserved6[15]; + u32 fec_x_cntrl;/* Transmit Control reg */ + u32 fec_reserved7[7]; + u32 fec_addr_low; /* Low 32bits MAC address */ + u32 fec_addr_high; /* High 16bits MAC address */ + u32 fec_opd;/* Opcode + Pause duration */ + u32 fec_reserved8[10]; + u32 fec_hash_table_high;/* High 32bits hash table */ + u32 fec_hash_table_low; /* Low 32bits hash table */ + u32 fec_grp_hash_table_high;/* High 32bits hash table */ + u32 fec_grp_hash_table_low; /* Low 32bits hash table */ + u32 fec_reserved9[7]; + u32 fec_x_wmrk; /* FIFO transmit water mark */ + u32 fec_reserved10; + u32 fec_r_bound;/* FIFO receive bound reg */ + u32 fec_r_fstart; /* FIFO receive start reg */ + u32 fec_reserved11[11]; + u32 fec_r_des_start;/* Receive descriptor ring */ + u32 fec_x_des_start;/* Transmit descriptor ring */ + u32 fec_r_buff_size;/* Maximum receive buff size */ + u32 fec_dma_control;/* DMA Endian and other ctrl */ +} fec_t; + +#endif /* CONFIG_FS_ENET_MPC5121_FEC */ #endif /* CONFIG_M5272 */ I'm not exactly clear as to why this was done this way but this not acceptable as it means we can't build a multiplatform kernel that needs this driver. I'm also not clear to me if the MPC5121 FEC is really the same device or close to it that it should be sharing this driver or have its own. - k ___ Linuxppc-embedded mailing list Linuxppc-embedded@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-embedded
Re: [PATCH 2/2] Re-added support for FEC on MPC5121 from Freescale LTIB to current head
On Thu, Jun 12, 2008 at 6:12 AM, Kumar Gala <[EMAIL PROTECTED]> wrote: > > On Jun 12, 2008, at 6:45 AM, David Jander wrote: > > Your commit message isn't exactly helpful as most people dont know what LTIB > is and its not terribly relevant. It just seems like you are adding support > for the FEC on MPC5121 and this point. Agreed. Also, you need to CC: the netdev mailing list for network drivers. >> Signed-off-by: David Jander <[EMAIL PROTECTED]> >> --- >> arch/powerpc/platforms/Kconfig |2 +- >> drivers/net/fec.h | 43 >> drivers/net/fs_enet/Kconfig| 22 +- >> drivers/net/fs_enet/fs_enet-main.c | 76 >> ++-- >> drivers/net/fs_enet/fs_enet.h | 17 +--- >> drivers/net/fs_enet/mac-fec.c | 22 +- >> drivers/net/fs_enet/mii-fec.c | 10 - >> 7 files changed, 167 insertions(+), 25 deletions(-) >> >> diff --git a/arch/powerpc/platforms/Kconfig >> b/arch/powerpc/platforms/Kconfig >> index 87454c5..a96937f 100644 >> --- a/arch/powerpc/platforms/Kconfig >> +++ b/arch/powerpc/platforms/Kconfig >> @@ -288,7 +288,7 @@ config CPM2 >> >> config PPC_CPM_NEW_BINDING >>bool >> - depends on CPM1 || CPM2 >> + depends on CPM1 || CPM2 || FS_ENET_MPC5121_FEC >>default y >> >> config AXON_RAM >> diff --git a/drivers/net/fec.h b/drivers/net/fec.h >> index 292719d..5c9fe34 100644 >> --- a/drivers/net/fec.h >> +++ b/drivers/net/fec.h >> @@ -59,6 +59,7 @@ typedef struct fec { >> } fec_t; >> >> #else >> +#if !defined(CONFIG_FS_ENET_MPC5121_FEC) >> >> /* >> * Define device register set address map. >> @@ -97,6 +98,48 @@ typedef struct fec { >>unsigned long fec_fifo_ram[112]; /* FIFO RAM buffer */ >> } fec_t; >> >> +#else /* CONFIG_FS_ENET_MPC5121_FEC */ >> + >> +typedef struct fec { >> + u32 fec_reserved0; >> + u32 fec_ievent; /* Interrupt event reg */ >> + u32 fec_imask; /* Interrupt mask reg */ >> + u32 fec_reserved1; >> + u32 fec_r_des_active; /* Receive descriptor reg */ >> + u32 fec_x_des_active; /* Transmit descriptor reg */ >> + u32 fec_reserved2[3]; >> + u32 fec_ecntrl; /* Ethernet control reg */ >> + u32 fec_reserved3[6]; >> + u32 fec_mii_data; /* MII manage frame reg */ >> + u32 fec_mii_speed; /* MII speed control reg */ >> + u32 fec_reserved4[7]; >> + u32 fec_mib_ctrlstat; /* MIB control/status reg */ >> + u32 fec_reserved5[7]; >> + u32 fec_r_cntrl;/* Receive control reg */ >> + u32 fec_reserved6[15]; >> + u32 fec_x_cntrl;/* Transmit Control reg */ >> + u32 fec_reserved7[7]; >> + u32 fec_addr_low; /* Low 32bits MAC address */ >> + u32 fec_addr_high; /* High 16bits MAC address */ >> + u32 fec_opd;/* Opcode + Pause duration */ >> + u32 fec_reserved8[10]; >> + u32 fec_hash_table_high;/* High 32bits hash table */ >> + u32 fec_hash_table_low; /* Low 32bits hash table */ >> + u32 fec_grp_hash_table_high;/* High 32bits hash table */ >> + u32 fec_grp_hash_table_low; /* Low 32bits hash table */ >> + u32 fec_reserved9[7]; >> + u32 fec_x_wmrk; /* FIFO transmit water mark */ >> + u32 fec_reserved10; >> + u32 fec_r_bound;/* FIFO receive bound reg */ >> + u32 fec_r_fstart; /* FIFO receive start reg */ >> + u32 fec_reserved11[11]; >> + u32 fec_r_des_start;/* Receive descriptor ring */ >> + u32 fec_x_des_start;/* Transmit descriptor ring */ >> + u32 fec_r_buff_size;/* Maximum receive buff size */ >> + u32 fec_dma_control;/* DMA Endian and other ctrl */ >> +} fec_t; >> + >> +#endif /* CONFIG_FS_ENET_MPC5121_FEC */ >> #endif /* CONFIG_M5272 */ > > I'm not exactly clear as to why this was done this way but this not > acceptable as it means we can't build a multiplatform kernel that needs this > driver. > > I'm also not clear to me if the MPC5121 FEC is really the same device or > close to it that it should be sharing this driver or have its own. Indeed. If it is not easy to isolate the differences for runtime binding then it may be best to just clone the driver as a starting point and make the 5121 specific changes. Cheers, g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-embedded mailing list Linuxppc-embedded@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-embedded
Re: [PATCH 2/2] Re-added support for FEC on MPC5121 from Freescale LTIB to current head
On Thu, Jun 12, 2008 at 5:45 AM, David Jander <[EMAIL PROTECTED]> wrote: > > > /* write */ > -#define CBDW_SC(_cbd, _sc) __cbd_out16(&(_cbd)->cbd_sc, (_sc)) > -#define CBDW_DATLEN(_cbd, _datlen) __cbd_out16(&(_cbd)->cbd_datlen, > (_datlen)) > -#define CBDW_BUFADDR(_cbd, _bufaddr) __cbd_out32(&(_cbd)->cbd_bufaddr, > (_bufaddr)) > +#define CBDW_SC(_cbd, _sc) __cbd_out16((volatile void __iomem > *)&(_cbd)->cbd_sc, (_sc)) > +#define CBDW_DATLEN(_cbd, _datlen) __cbd_out16((volatile void __iomem > *)&(_cbd)->cbd_datlen, (_datlen)) > +#define CBDW_BUFADDR(_cbd, _bufaddr) __cbd_out32((volatile void __iomem > *)&(_cbd)->cbd_bufaddr, (_bufaddr)) > > /* read */ > -#define CBDR_SC(_cbd) __cbd_in16(&(_cbd)->cbd_sc) > -#define CBDR_DATLEN(_cbd) __cbd_in16(&(_cbd)->cbd_datlen) > -#define CBDR_BUFADDR(_cbd) __cbd_in32(&(_cbd)->cbd_bufaddr) > +#define CBDR_SC(_cbd) __cbd_in16((volatile void __iomem > *)&(_cbd)->cbd_sc) > +#define CBDR_DATLEN(_cbd) __cbd_in16((volatile void __iomem > *)&(_cbd)->cbd_datlen) > +#define CBDR_BUFADDR(_cbd) __cbd_in32((volatile void __iomem > *)&(_cbd)->cbd_bufaddr) Another comment: This really doesn't look right. The _cbd pointer passed in should already be tagged with __iomem. Trying to fix it here is a band-aid and unsafe. Cheers, g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ___ Linuxppc-embedded mailing list Linuxppc-embedded@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-embedded
Re: [PATCH 2/2] Re-added support for FEC on MPC5121 from Freescale LTIB to current head
On Thursday 12 June 2008 14:12:15 you wrote: > On Jun 12, 2008, at 6:45 AM, David Jander wrote: > > Your commit message isn't exactly helpful as most people dont know > what LTIB is and its not terribly relevant. It just seems like you > are adding support for the FEC on MPC5121 and this point. > >[...] > > --- a/drivers/net/fec.h > > +++ b/drivers/net/fec.h > > @@ -59,6 +59,7 @@ typedef struct fec { > > } fec_t; > > > > #else > > +#if !defined(CONFIG_FS_ENET_MPC5121_FEC) > > > > /* > > * Define device register set address map. > > @@ -97,6 +98,48 @@ typedef struct fec { > > unsigned long fec_fifo_ram[112]; /* FIFO RAM buffer */ > > } fec_t; > > > > +#else /* CONFIG_FS_ENET_MPC5121_FEC */ > > + > > +typedef struct fec { > > [...] > > +} fec_t; > > + > > +#endif /* CONFIG_FS_ENET_MPC5121_FEC */ > > #endif /* CONFIG_M5272 */ > > I'm not exactly clear as to why this was done this way but this not > acceptable as it means we can't build a multiplatform kernel that > needs this driver. Well, it wouldn't be possible either, since CONFIG_M5272 is a Cold-fire processor, and CONFIG_FS_ENET_MPC5121_FEC is for a PowerPC processor. In this case. Otherwise you are right, the driver breaks MPC83xx/MPC5121 multiplatform builds. > I'm also not clear to me if the MPC5121 FEC is really the same device > or close to it that it should be sharing this driver or have its own. I am coming to the conclusion that it should have its own driver. Altough a lot of code could be shared, there are still enough differences, so that writing just ONE driver without some #ifdef's that would break multiplatform builds, would instead end up with a much bigger amount of if's, that would make it unreadable, unmaintainable and inefficient. Here's why: The above struct fet_t for instance is mapped to a set of registers in the FEC. For processors with a CPM1, a CPM2 or without CPM (i.e. MPC5121) the register mapping seems to be significantly different, nevertheless the structs are all called "struct fec_t". How can one fix this at runtime without changing the name of the structs and then just use a lot of "if's" or a combination of macro's and if's everywhere a register of the FEC is accessed? I fear it will be a mess. So I think it's either a separate driver, or break multiplatform builds. Since I am learning from you that breaking multiplatform builds is a no-go, I'll settle for splitting up the driver. Any suggestion on where to put that split-off? How to name it? I would suggest drivers/net/fec_mpc512x/* I just resubmitted PATCH 1/2 again without part 2 (which hasn't much to do with it anyway), so Grant may have a final look at it (hopefully I did it right this time). Part 2 (MPC5121_FEC) will have to wait until monday or so, since it will take me a while, and I have to do other things in between. Any suggestions on how to solve the puzzle are of course welcome... Thanks a lot for reviewing. Best regards, -- David Jander Protonic Holland. ___ Linuxppc-embedded mailing list Linuxppc-embedded@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-embedded