| Message ID | 1241640919-4650-3-git-send-email-wd@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk <wd@denx.de> wrote: > From: John Rigby <jrigby@freescale.com> > > Add support for MPC512x to fs_enet driver > > drivers/net/fs_enet/* > Enable fs_enet driver to work 5121 FEC > Enable it with CONFIG_FS_ENET_MPC5121_FEC > > Signed-off-by: John Rigby <jrigby@freescale.com> > Signed-off-by: Piotr Ziecik <kosmo@semihalf.com> > Signed-off-by: Wolfgang Denk <wd@denx.de> > Cc: <netdev@vger.kernel.org> > Cc: Grant Likely <grant.likely@secretlab.ca> > Cc: John Rigby <jcrigby@gmail.com> > --- > arch/powerpc/include/asm/mpc5121_fec.h | 111 ++++++++++++++++++++++++++++++++ > drivers/net/fs_enet/Kconfig | 10 ++- > drivers/net/fs_enet/fs_enet-main.c | 7 ++ > drivers/net/fs_enet/fs_enet.h | 6 ++ > drivers/net/fs_enet/mac-fec.c | 30 ++++++++- > drivers/net/fs_enet/mii-fec.c | 7 ++ > 6 files changed, 166 insertions(+), 5 deletions(-) > create mode 100644 arch/powerpc/include/asm/mpc5121_fec.h > > diff --git a/arch/powerpc/include/asm/mpc5121_fec.h b/arch/powerpc/include/asm/mpc5121_fec.h > new file mode 100644 > index 0000000..6bddf0b > --- /dev/null > +++ b/arch/powerpc/include/asm/mpc5121_fec.h > @@ -0,0 +1,111 @@ > +/* > + * Copyright (C) 2007,2008 Freescale Semiconductor, Inc. All rights reserved. > + * > + * Author: John Rigby, <jrigby@freescale.com> > + * > + * Modified version of drivers/net/fec.h: > + * > + * fec.h -- Fast Ethernet Controller for Motorola ColdFire SoC > + * processors. > + * > + * (C) Copyright 2000-2005, Greg Ungerer (gerg@snapgear.com) > + * (C) Copyright 2000-2001, Lineo (www.lineo.com) > + * > + * This is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > +#ifndef MPC5121_FEC_H > +#define MPC5121_FEC_H > + > +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_reserved12[26]; > + u32 fec_dma_control; /* DMA Endian and other ctrl */ > +} fec_t; > + > +/* > + * Define the buffer descriptor structure. > + */ > +typedef struct bufdesc { > + ushort cbd_sc; /* Control and status info */ > + ushort cbd_datlen; /* Data length */ > + uint cbd_bufaddr; /* Buffer address */ > +} cbd_t; > + > +/* > + * The following definitions courtesy of commproc.h, which where > + * Copyright (c) 1997 Dan Malek (dmalek@jlc.net). > + */ > +#define BD_SC_WRAP ((ushort)0x2000) > + > +/* > + * Buffer descriptor control/status used by Ethernet receive. > + */ > +#define BD_ENET_RX_EMPTY ((ushort)0x8000) > +#define BD_ENET_RX_WRAP ((ushort)0x2000) > +#define BD_ENET_RX_INTR ((ushort)0x1000) > +#define BD_ENET_RX_LAST ((ushort)0x0800) > +#define BD_ENET_RX_FIRST ((ushort)0x0400) > +#define BD_ENET_RX_MISS ((ushort)0x0100) > +#define BD_ENET_RX_LG ((ushort)0x0020) > +#define BD_ENET_RX_NO ((ushort)0x0010) > +#define BD_ENET_RX_SH ((ushort)0x0008) > +#define BD_ENET_RX_CR ((ushort)0x0004) > +#define BD_ENET_RX_OV ((ushort)0x0002) > +#define BD_ENET_RX_CL ((ushort)0x0001) > +#define BD_ENET_RX_STATS ((ushort)0x013f) /* All status bits */ > + > +/* > + * Buffer descriptor control/status used by Ethernet transmit. > + */ > +#define BD_ENET_TX_READY ((ushort)0x8000) > +#define BD_ENET_TX_PAD ((ushort)0x4000) > +#define BD_ENET_TX_WRAP ((ushort)0x2000) > +#define BD_ENET_TX_INTR ((ushort)0x1000) > +#define BD_ENET_TX_LAST ((ushort)0x0800) > +#define BD_ENET_TX_TC ((ushort)0x0400) > +#define BD_ENET_TX_DEF ((ushort)0x0200) > +#define BD_ENET_TX_HB ((ushort)0x0100) > +#define BD_ENET_TX_LC ((ushort)0x0080) > +#define BD_ENET_TX_RL ((ushort)0x0040) > +#define BD_ENET_TX_UN ((ushort)0x0002) > +#define BD_ENET_TX_CSL ((ushort)0x0001) > +#define BD_ENET_TX_STATS ((ushort)0x03ff) /* All status bits */ > + > +#endif /* MPC5121_FEC_H */ > diff --git a/drivers/net/fs_enet/Kconfig b/drivers/net/fs_enet/Kconfig > index 562ea68..fc073b5 100644 > --- a/drivers/net/fs_enet/Kconfig > +++ b/drivers/net/fs_enet/Kconfig > @@ -1,9 +1,13 @@ > config FS_ENET > tristate "Freescale Ethernet Driver" > - depends on CPM1 || CPM2 > + depends on CPM1 || CPM2 || PPC_MPC512x > select MII > select PHYLIB > > +config FS_ENET_MPC5121_FEC > + def_bool y if (FS_ENET && PPC_MPC512x) > + select FS_ENET_HAS_FEC > + > config FS_ENET_HAS_SCC > bool "Chip has an SCC usable for ethernet" > depends on FS_ENET && (CPM1 || CPM2) > @@ -16,13 +20,13 @@ config FS_ENET_HAS_FCC > > config FS_ENET_HAS_FEC > bool "Chip has an FEC usable for ethernet" > - depends on FS_ENET && CPM1 > + depends on FS_ENET && (CPM1 || FS_ENET_MPC5121_FEC) > select FS_ENET_MDIO_FEC > default y > > config FS_ENET_MDIO_FEC > tristate "MDIO driver for FEC" > - depends on FS_ENET && CPM1 > + depends on FS_ENET && (CPM1 || FS_ENET_MPC5121_FEC) > > config FS_ENET_MDIO_FCC > tristate "MDIO driver for FCC" > diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c > index f996a1a..4170d33 100644 > --- a/drivers/net/fs_enet/fs_enet-main.c > +++ b/drivers/net/fs_enet/fs_enet-main.c > @@ -1183,11 +1183,18 @@ static struct of_device_id fs_enet_match[] = { > }, > #endif > #ifdef CONFIG_FS_ENET_HAS_FEC > +#ifdef CONFIG_FS_ENET_MPC5121_FEC > + { > + .compatible = "fsl,mpc5121-fec", > + .data = (void *)&fs_fec_ops, > + }, > +#else > { > .compatible = "fsl,pq1-fec-enet", > .data = (void *)&fs_fec_ops, > }, > #endif > +#endif Hmmm. A lot of these #ifdefs in here. Does this have a multiplatform impact? Not to mention the fact that it's just plain ugly. :-) g.
Would you be offended if I tell you that this is a horrible patch submission? Your introductory email indicates 16 patches, yet the series indicates there were 12, and that intro email is only posted to the linuxppc-dev list for people to read. Nobody on netdev nor other interested parties that get CC:'d along the line are able to read what this patch series is about. Since only some patches are CC:'d to netdev I have no idea if I should apply these or they are dependent on some other patches in the series that you didn't send here to netdev. What a mess... how can any maintainer figure out what patch is what, and what tree you expect these patches to even get applied to? I'm definitely sending all of the copies I have received to /dev/null, you need to submit this work properly.
Wolfgang Denk wrote: > +/* > + * Define the buffer descriptor structure. > + */ > +typedef struct bufdesc { > + ushort cbd_sc; /* Control and status info */ > + ushort cbd_datlen; /* Data length */ > + uint cbd_bufaddr; /* Buffer address */ > +} cbd_t; > + > +/* > + * The following definitions courtesy of commproc.h, which where > + * Copyright (c) 1997 Dan Malek (dmalek@jlc.net). > + */ > +#define BD_SC_WRAP ((ushort)0x2000) > + > +/* > + * Buffer descriptor control/status used by Ethernet receive. > + */ > +#define BD_ENET_RX_EMPTY ((ushort)0x8000) > +#define BD_ENET_RX_WRAP ((ushort)0x2000) > +#define BD_ENET_RX_INTR ((ushort)0x1000) > +#define BD_ENET_RX_LAST ((ushort)0x0800) > +#define BD_ENET_RX_FIRST ((ushort)0x0400) > +#define BD_ENET_RX_MISS ((ushort)0x0100) > +#define BD_ENET_RX_LG ((ushort)0x0020) > +#define BD_ENET_RX_NO ((ushort)0x0010) > +#define BD_ENET_RX_SH ((ushort)0x0008) > +#define BD_ENET_RX_CR ((ushort)0x0004) > +#define BD_ENET_RX_OV ((ushort)0x0002) > +#define BD_ENET_RX_CL ((ushort)0x0001) > +#define BD_ENET_RX_STATS ((ushort)0x013f) /* All status bits */ > + > +/* > + * Buffer descriptor control/status used by Ethernet transmit. > + */ > +#define BD_ENET_TX_READY ((ushort)0x8000) > +#define BD_ENET_TX_PAD ((ushort)0x4000) > +#define BD_ENET_TX_WRAP ((ushort)0x2000) > +#define BD_ENET_TX_INTR ((ushort)0x1000) > +#define BD_ENET_TX_LAST ((ushort)0x0800) > +#define BD_ENET_TX_TC ((ushort)0x0400) > +#define BD_ENET_TX_DEF ((ushort)0x0200) > +#define BD_ENET_TX_HB ((ushort)0x0100) > +#define BD_ENET_TX_LC ((ushort)0x0080) > +#define BD_ENET_TX_RL ((ushort)0x0040) > +#define BD_ENET_TX_UN ((ushort)0x0002) > +#define BD_ENET_TX_CSL ((ushort)0x0001) > +#define BD_ENET_TX_STATS ((ushort)0x03ff) /* All status bits */ All of the above is duplicative (with even the same names) of stuff in asm/cpm.h. Beyond just the duplication, what happens if both CPM2 and 512x are enabled in the same kernel? -Scott
Grant Likely wrote: >> #ifdef CONFIG_FS_ENET_HAS_FEC >> +#ifdef CONFIG_FS_ENET_MPC5121_FEC >> + { >> + .compatible = "fsl,mpc5121-fec", >> + .data = (void *)&fs_fec_ops, >> + }, >> +#else >> { >> .compatible = "fsl,pq1-fec-enet", >> .data = (void *)&fs_fec_ops, >> }, >> #endif >> +#endif > > Hmmm. A lot of these #ifdefs in here. Does this have a multiplatform > impact? Not to mention the fact that it's just plain ugly. :-) Multiplatform between 512x and 8xx is currently impossible for other reasons (512x and CPM2 is another matter). That said, less ifdefs would be nice. -Scott
Dear Grant, in message <fa686aa40905061333q29c263c8p24856c048e30f4d0@mail.gmail.com> you wrote: > ... > > #ifdef CONFIG_FS_ENET_HAS_FEC > > +#ifdef CONFIG_FS_ENET_MPC5121_FEC > > + { > > + .compatible = "fsl,mpc5121-fec", > > + .data = (void *)&fs_fec_ops, > > + }, > > +#else > > { > > .compatible = "fsl,pq1-fec-enet", > > .data = (void *)&fs_fec_ops, > > }, > > #endif > > +#endif > > Hmmm. A lot of these #ifdefs in here. Does this have a multiplatform > impact? Not to mention the fact that it's just plain ugly. :-) Agreed that it's ugly, but duplicatio9ng the code would have been even worse. I don't think that it has multiplatform - at least not as long as you don't ask for one image that runs on 83xx and on 512x. Best regards, Wolfgang Denk
Dear David, In message <20090506.134003.261424694.davem@davemloft.net> you wrote: > > Would you be offended if I tell you that this is a horrible patch > submission? > > Your introductory email indicates 16 patches, yet the series indicates > there were 12, and that intro email is only posted to the linuxppc-dev > list for people to read. Nobody on netdev nor other interested > parties that get CC:'d along the line are able to read what this patch > series is about. Of course I am not offended, as you are absolutely right with your comment. I'm angry with myself, as I actually intended to do exactly what you find missing, but then forgot to write the file in the editor :-( > I'm definitely sending all of the copies I have received to /dev/null, > you need to submit this work properly. OK. What exactly should I do? Best regards, Wolfgang Denk
Dear Scott, in message <4A01F602.2010601@freescale.com> you wrote: > > All of the above is duplicative (with even the same names) of stuff in > asm/cpm.h. Beyond just the duplication, what happens if both CPM2 and OK, I can try to reuse the definitions from that file. > 512x are enabled in the same kernel? Hm... both architectures look sufficiently different to me that I don't see sense in trying such a thing. Do you think that needs to be supported? Best regards, Wolfgang Denk
On Wed, May 6, 2009 at 4:01 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Grant, > > in message <fa686aa40905061333q29c263c8p24856c048e30f4d0@mail.gmail.com> you wrote: >> > ... >> > #ifdef CONFIG_FS_ENET_HAS_FEC >> > +#ifdef CONFIG_FS_ENET_MPC5121_FEC >> > + { >> > + .compatible = "fsl,mpc5121-fec", >> > + .data = (void *)&fs_fec_ops, >> > + }, >> > +#else >> > { >> > .compatible = "fsl,pq1-fec-enet", >> > .data = (void *)&fs_fec_ops, >> > }, >> > #endif >> > +#endif >> >> Hmmm. A lot of these #ifdefs in here. Does this have a multiplatform >> impact? Not to mention the fact that it's just plain ugly. :-) > > Agreed that it's ugly, but duplicatio9ng the code would have been even > worse. I don't think that it has multiplatform - at least not as long > as you don't ask for one image that runs on 83xx and on 512x. Actually, I *am* asking for one image that runs on 83xx, 52xx and 521x. I already can and do build and test a single image which boots on all my 52xx boards, on my 8349 board, and on my G4 Mac. g.
On Wed, May 6, 2009 at 4:09 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Scott, > > in message <4A01F602.2010601@freescale.com> you wrote: >> >> All of the above is duplicative (with even the same names) of stuff in >> asm/cpm.h. Beyond just the duplication, what happens if both CPM2 and > > OK, I can try to reuse the definitions from that file. > >> 512x are enabled in the same kernel? > > Hm... both architectures look sufficiently different to me that I > don't see sense in trying such a thing. Do you think that needs to be > supported? Yes! :-) It's not hard to do and it keeps the driver cleaner (IMNSHO). I don't think it is quite possible at the moment due to cache coherency issues, but with Becky's recently merged dma ops changes it should be fixable. Cheers, g.
Dear Grant, In message <fa686aa40905061529u11b231afle3b5bb10a2334ad0@mail.gmail.com> you wrote: > > > Agreed that it's ugly, but duplicatio9ng the code would have been even > > worse. I don't think that it has multiplatform - at least not as long > > as you don't ask for one image that runs on 83xx and on 512x. > > Actually, I *am* asking for one image that runs on 83xx, 52xx and > 521x. I already can and do build and test a single image which boots > on all my 52xx boards, on my 8349 board, and on my G4 Mac. He. I was afraid you'd say that ;-) In this case I need a helping hand as I can't figure out how to make this work. Any suggestions? Best regards, Wolfgang Denk
On Thursday 07 May 2009 00:29:59 Grant Likely wrote: > On Wed, May 6, 2009 at 4:01 PM, Wolfgang Denk <wd@denx.de> wrote: > > Dear Grant, > > > > in message <fa686aa40905061333q29c263c8p24856c048e30f4d0@mail.gmail.com> > > you wrote: > > > > ... > > > >> > #ifdef CONFIG_FS_ENET_HAS_FEC > >> > +#ifdef CONFIG_FS_ENET_MPC5121_FEC > >> > + { > >> > + .compatible = "fsl,mpc5121-fec", > >> > + .data = (void *)&fs_fec_ops, > >> > + }, > >> > +#else > >> > { > >> > .compatible = "fsl,pq1-fec-enet", > >> > .data = (void *)&fs_fec_ops, > >> > }, > >> > #endif > >> > +#endif > >> > >> Hmmm. A lot of these #ifdefs in here. Does this have a multiplatform > >> impact? Not to mention the fact that it's just plain ugly. :-) > > > > Agreed that it's ugly, but duplicatio9ng the code would have been even > > worse. I don't think that it has multiplatform - at least not as long > > as you don't ask for one image that runs on 83xx and on 512x. > > Actually, I *am* asking for one image that runs on 83xx, 52xx and > 521x. I already can and do build and test a single image which boots > on all my 52xx boards, on my 8349 board, and on my G4 Mac. Cool! I also want that! We have different boards with 5200 and 5121e's and it would be terrific if one day we'd be able to use just one kernel for all of them! (Sorry for being a me-too-er) Best regards,
On May 6, 2009, at 5:01 PM, Wolfgang Denk wrote: > Dear Grant, > > in message <fa686aa40905061333q29c263c8p24856c048e30f4d0@mail.gmail.com > > you wrote: >> > ... >>> #ifdef CONFIG_FS_ENET_HAS_FEC >>> +#ifdef CONFIG_FS_ENET_MPC5121_FEC >>> + { >>> + .compatible = "fsl,mpc5121-fec", >>> + .data = (void *)&fs_fec_ops, >>> + }, >>> +#else >>> { >>> .compatible = "fsl,pq1-fec-enet", >>> .data = (void *)&fs_fec_ops, >>> }, >>> #endif >>> +#endif >> >> Hmmm. A lot of these #ifdefs in here. Does this have a >> multiplatform >> impact? Not to mention the fact that it's just plain ugly. :-) > > Agreed that it's ugly, but duplicatio9ng the code would have been even > worse. I don't think that it has multiplatform - at least not as long > as you don't ask for one image that runs on 83xx and on 512x. We do ask for that. The fedora ppc6xx kernel should work on any 6xx/ 7xx/7xxx based core chip. We should NOT be breaking such capability in drivers. - k
On Wed, May 6, 2009 at 4:41 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Grant, > > In message <fa686aa40905061529u11b231afle3b5bb10a2334ad0@mail.gmail.com> you wrote: >> >> > Agreed that it's ugly, but duplicatio9ng the code would have been even >> > worse. I don't think that it has multiplatform - at least not as long >> > as you don't ask for one image that runs on 83xx and on 512x. >> >> Actually, I *am* asking for one image that runs on 83xx, 52xx and >> 521x. I already can and do build and test a single image which boots >> on all my 52xx boards, on my 8349 board, and on my G4 Mac. > > He. I was afraid you'd say that ;-) > > In this case I need a helping hand as I can't figure out how to make > this work. Any suggestions? Hmmm, it is rather ugly because the layout of fec_t is so different. Easiest solution would be to duplicate the driver in its entirety, but as you say it results in a lot of duplicate code. It might be the lesser of many weevils though. Second easiest would be to factor out all the common code in the driver into a separate .c file that gets included by a 'wrapper' .c file for each config which first includes the correct definition of fec_t. This approach solves the duplicate code problem, but it also fell out of the ugly tree and hit every branch on the way down. ie: the wrappers would look something like this: fs_enet_cpm1.c: #include <asm/cpm1.h> #include "fs_enet_main.c" fs_enet_cpm2.c: #include <asm/cpm2.h> #include "fs_enet_main.c" fs_enet_512x.c: #include <asm/mpc512x.h> #include "fs_enet_main.c" And then the makefile would be something along the lines of: obj-${CONFIG_FS_ENET_CPM1_ += fs_enet_cpm1.o obj-${CONFIG_FS_ENET_CPM2_ += fs_enet_cpm2.o obj-${CONFIG_FS_ENET_MPC512x_ += fs_enet_512x.o A brief look at the driver suggests that access to the fec_t structure is restricted to the fec-mii.c and mac-fec.c files. It might be appropriate to duplicate just those files and do some form of fs_enet_ops to select between them. While on the topic, it looks to me like the driver could really use some refactoring love. Having multiple definitions of "fec_t" is confusing and potentially lead to hard to find bugs if the wrong header gets included by anyone. I'd like to see all the fec specific stuff in arch/powerpc/include/asm moved into drivers/net/fs_enet and renamed to reflect the hardware it is associate with. Stuff like "fec_t" is far to generic, not to mention that typedefs are discouraged now. Regardless, I feel your pain. It is not a pretty situation. g.
Wolfgang, Welcome to my world and why I gave up on this months ago. Everyone else, One thing to consider here is a rewrite with the goal of a new improved fec driver that would work on both 5121 and the various mx platforms that also have this same fec core. Also don't forget that the register map is the same on 512x, mx and coldfire platforms but not on the other ppc platforms so if you want to one binary to rule them all you will need to have an offest table or some such. John On Thu, May 7, 2009 at 8:09 AM, Grant Likely <grant.likely@secretlab.ca>wrote: > On Wed, May 6, 2009 at 4:41 PM, Wolfgang Denk <wd@denx.de> wrote: > > Dear Grant, > > > > In message <fa686aa40905061529u11b231afle3b5bb10a2334ad0@mail.gmail.com> > you wrote: > >> > >> > Agreed that it's ugly, but duplicatio9ng the code would have been even > >> > worse. I don't think that it has multiplatform - at least not as long > >> > as you don't ask for one image that runs on 83xx and on 512x. > >> > >> Actually, I *am* asking for one image that runs on 83xx, 52xx and > >> 521x. I already can and do build and test a single image which boots > >> on all my 52xx boards, on my 8349 board, and on my G4 Mac. > > > > He. I was afraid you'd say that ;-) > > > > In this case I need a helping hand as I can't figure out how to make > > this work. Any suggestions? > > Hmmm, it is rather ugly because the layout of fec_t is so different. > Easiest solution would be to duplicate the driver in its entirety, but > as you say it results in a lot of duplicate code. It might be the > lesser of many weevils though. > > Second easiest would be to factor out all the common code in the > driver into a separate .c file that gets included by a 'wrapper' .c > file for each config which first includes the correct definition of > fec_t. This approach solves the duplicate code problem, but it also > fell out of the ugly tree and hit every branch on the way down. > > ie: the wrappers would look something like this: > > fs_enet_cpm1.c: > #include <asm/cpm1.h> > #include "fs_enet_main.c" > > fs_enet_cpm2.c: > #include <asm/cpm2.h> > #include "fs_enet_main.c" > > fs_enet_512x.c: > #include <asm/mpc512x.h> > #include "fs_enet_main.c" > > And then the makefile would be something along the lines of: > obj-${CONFIG_FS_ENET_CPM1_ += fs_enet_cpm1.o > obj-${CONFIG_FS_ENET_CPM2_ += fs_enet_cpm2.o > obj-${CONFIG_FS_ENET_MPC512x_ += fs_enet_512x.o > > A brief look at the driver suggests that access to the fec_t structure > is restricted to the fec-mii.c and mac-fec.c files. It might be > appropriate to duplicate just those files and do some form of > fs_enet_ops to select between them. > > While on the topic, it looks to me like the driver could really use > some refactoring love. Having multiple definitions of "fec_t" is > confusing and potentially lead to hard to find bugs if the wrong > header gets included by anyone. I'd like to see all the fec specific > stuff in arch/powerpc/include/asm moved into drivers/net/fs_enet and > renamed to reflect the hardware it is associate with. Stuff like > "fec_t" is far to generic, not to mention that typedefs are > discouraged now. > > Regardless, I feel your pain. It is not a pretty situation. > > g. > > -- > Grant Likely, B.Sc., P.Eng. > Secret Lab Technologies Ltd. >
From: John Rigby <jcrigby@gmail.com> Date: Thu, 7 May 2009 20:02:53 -0600 > Also don't forget that the register map is the same on 512x, mx and > coldfire platforms but not on the other ppc platforms so if you want > to one binary to rule them all you will need to have an offest table > or some such. I would suggest using ->read_reg() ->write_reg() methods for abstracting this. That's how we handle all of the different way ESP scsi chips have their registers wired up. I/O register reads take hundreds, if not thousands of CPU cycles so, relatively speaking, the indirection costs absolutely nothing.
On Friday 08 May 2009 09:52:51 David Miller wrote: > From: John Rigby <jcrigby@gmail.com> > Date: Thu, 7 May 2009 20:02:53 -0600 > > > Also don't forget that the register map is the same on 512x, mx and > > coldfire platforms but not on the other ppc platforms so if you want > > to one binary to rule them all you will need to have an offest table > > or some such. > > I would suggest using ->read_reg() ->write_reg() methods for abstracting > this. That's how we handle all of the different way ESP scsi chips > have their registers wired up. > > I/O register reads take hundreds, if not thousands of CPU cycles so, > relatively speaking, the indirection costs absolutely nothing. I fear the memory-mapped I/O of the PowerPC SoC is *slightly* faster, so in terms of cycle count, this WILL matter, although depending on how much register-I/O the driver does, overall performance impact _may_ still be negligible. I suggest testing this (benchmarks) before and after the change. Best regsards,
Thursday 07 May 2009 00:39:25 Grant Likely napisał(a): > >> 512x are enabled in the same kernel? > > > > Hm... both architectures look sufficiently different to me that I > > don't see sense in trying such a thing. Do you think that needs to be > > supported? > > Yes! :-) It's not hard to do and it keeps the driver cleaner > (IMNSHO). I don't think it is quite possible at the moment due to > cache coherency issues, but with Becky's recently merged dma ops > changes it should be fixable. Could you elaborate on the cache coherency issues in MPC5121 FEC context? Especially how these issues are related to the driver binary compatibility. MPC5121 support was added to drivers/net/fs_enet. MPC52xx uses drivers/net/fec_mpc52xx.c. Do you think that creating one universal driver from these two is now possible? You said that it should be easy, however you also said that cache coherency issues makes this imposible.
2009/5/14 Piotr Zięcik <kosmo@semihalf.com>: > Thursday 07 May 2009 00:39:25 Grant Likely napisał(a): >> >> 512x are enabled in the same kernel? >> > >> > Hm... both architectures look sufficiently different to me that I >> > don't see sense in trying such a thing. Do you think that needs to be >> > supported? >> >> Yes! :-) It's not hard to do and it keeps the driver cleaner >> (IMNSHO). I don't think it is quite possible at the moment due to >> cache coherency issues, but with Becky's recently merged dma ops >> changes it should be fixable. > > Could you elaborate on the cache coherency issues in MPC5121 > FEC context? Especially how these issues are related to the driver > binary compatibility. > > MPC5121 support was added to drivers/net/fs_enet. MPC52xx uses > drivers/net/fec_mpc52xx.c. Do you think that creating one universal > driver from these two is now possible? You said that it should be easy, > however you also said that cache coherency issues makes this imposible. Not impossible. Hard. The mpc5200 is a cache coherent part. Bestcomm memory accesses are noticed (snooped) by the cache, and it will flush out cache lines appropriately to maintain coherency. The mpc5121 bus design is non-coherent. The e300 core is essentially the same as on the 5200, and the core can do snooping, but the multiport memory controller on the 5121 doesn't support bus snooping so the cache is not automatically maintained in a consistent state. On this part Linux needs to manually flush the relevant cache lines before initiating DMA transfers, and invalidiate them afterwards. All of this doesn't actually affect the driver code at all. It's all handled by the kernel and the DMA apis. What it does affect is multiplatform kernels. The DMA behaviour is set at compile time, not run time, depending on the setting of CONFIG_NON_COHERENT_CACHE (see arch/powerpc/platforms/Kconfig.cputype). A kernel which has it on won't run properly on a platform which has it off, and visa-versa. So, while the MPC5200 and MPC5121 (and MPC83xx) all have the same core, it isn't currently possible to build a single kernel which will boot on both the MPC5200 and the MPC5121. The solution (i think) is to kill CONFIG_NON_COHERENT_CACHE and have the platform setup code set the appropriate dmaops at platform init time. Cheers, g. > > -- > Best Regards. > Piotr Ziecik >
On Thu, 2009-05-14 at 08:00 -0600, Grant Likely wrote: > > All of this doesn't actually affect the driver code at all. It's all > handled by the kernel and the DMA apis. What it does affect is > multiplatform kernels. The DMA behaviour is set at compile time, not > run time, depending on the setting of CONFIG_NON_COHERENT_CACHE (see > arch/powerpc/platforms/Kconfig.cputype). A kernel which has it on > won't run properly on a platform which has it off, and visa-versa. We are close to the point where we can make this a runtime option though, by just having a different set of dma_ops hooked in. Cheers, Ben.
Tuesday 19 May 2009 00:17:31 Benjamin Herrenschmidt wrote: > > We are close to the point where we can make this a runtime option > though, by just having a different set of dma_ops hooked in. > Is somebody currently working on it? If yes, where we can see code ?
On Tue, 2009-05-19 at 13:26 +0200, Piotr Zięcik wrote: > Tuesday 19 May 2009 00:17:31 Benjamin Herrenschmidt wrote: > > > > We are close to the point where we can make this a runtime option > > though, by just having a different set of dma_ops hooked in. > > > > Is somebody currently working on it? If yes, where we can see > code ? Well, the current upstream code has the dma ops support. Becky is massaging that more for swiotlb support which has been submitted to the linuxppc-dev list. We haven't yet added runtime support rather than compile time for non-cache coherent platforms, but it's becoming reasonably easy to add now (though still needs to be done). Cheers, Ben
Thursday 14 May 2009 16:00:33 Grant Likely wrote: > > MPC5121 support was added to drivers/net/fs_enet. MPC52xx uses > > drivers/net/fec_mpc52xx.c. Do you think that creating one universal > > driver from these two is now possible? You said that it should be easy, > > however you also said that cache coherency issues makes this imposible. > > Not impossible. Hard. I thought a bit more about merging FEC drivers and I see one problem more. Driver fs_enet works with FEC's with own DMA engine and fec_mpc52xx.c uses BestComm. Integration of these two drivers will need a DMA abstraction layer to keep everything clean. Unfortuanetly BestComm driver does not provide any abstraction - it only exports set of functions to deal with specific hardware: FEC and PATA. More #ifdef's will be needed to remove linking with BestComm driver if kernel will be compiled without 52xx support and resulting code will not be much better than existing one. Especially that new DMA abstraction layer probably will be quite complex.
2009/5/21 Piotr Zięcik <kosmo@semihalf.com>: > Thursday 14 May 2009 16:00:33 Grant Likely wrote: >> > MPC5121 support was added to drivers/net/fs_enet. MPC52xx uses >> > drivers/net/fec_mpc52xx.c. Do you think that creating one universal >> > driver from these two is now possible? You said that it should be easy, >> > however you also said that cache coherency issues makes this imposible. >> >> Not impossible. Hard. > > I thought a bit more about merging FEC drivers and I see one problem more. > Driver fs_enet works with FEC's with own DMA engine and fec_mpc52xx.c uses > BestComm. Integration of these two drivers will need a DMA abstraction layer > to keep everything clean. Unfortuanetly BestComm driver does not provide any > abstraction - it only exports set of functions to deal with specific > hardware: FEC and PATA. > > More #ifdef's will be needed to remove linking with BestComm driver if kernel > will be compiled without 52xx support and resulting code will not be much > better than existing one. Especially that new DMA abstraction layer probably > will be quite complex. If it looks too ugly, then just fork the driver. g.
diff --git a/arch/powerpc/include/asm/mpc5121_fec.h b/arch/powerpc/include/asm/mpc5121_fec.h new file mode 100644 index 0000000..6bddf0b --- /dev/null +++ b/arch/powerpc/include/asm/mpc5121_fec.h @@ -0,0 +1,111 @@ +/* + * Copyright (C) 2007,2008 Freescale Semiconductor, Inc. All rights reserved. + * + * Author: John Rigby, <jrigby@freescale.com> + * + * Modified version of drivers/net/fec.h: + * + * fec.h -- Fast Ethernet Controller for Motorola ColdFire SoC + * processors. + * + * (C) Copyright 2000-2005, Greg Ungerer (gerg@snapgear.com) + * (C) Copyright 2000-2001, Lineo (www.lineo.com) + * + * This is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ +#ifndef MPC5121_FEC_H +#define MPC5121_FEC_H + +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_reserved12[26]; + u32 fec_dma_control; /* DMA Endian and other ctrl */ +} fec_t; + +/* + * Define the buffer descriptor structure. + */ +typedef struct bufdesc { + ushort cbd_sc; /* Control and status info */ + ushort cbd_datlen; /* Data length */ + uint cbd_bufaddr; /* Buffer address */ +} cbd_t; + +/* + * The following definitions courtesy of commproc.h, which where + * Copyright (c) 1997 Dan Malek (dmalek@jlc.net). + */ +#define BD_SC_WRAP ((ushort)0x2000) + +/* + * Buffer descriptor control/status used by Ethernet receive. + */ +#define BD_ENET_RX_EMPTY ((ushort)0x8000) +#define BD_ENET_RX_WRAP ((ushort)0x2000) +#define BD_ENET_RX_INTR ((ushort)0x1000) +#define BD_ENET_RX_LAST ((ushort)0x0800) +#define BD_ENET_RX_FIRST ((ushort)0x0400) +#define BD_ENET_RX_MISS ((ushort)0x0100) +#define BD_ENET_RX_LG ((ushort)0x0020) +#define BD_ENET_RX_NO ((ushort)0x0010) +#define BD_ENET_RX_SH ((ushort)0x0008) +#define BD_ENET_RX_CR ((ushort)0x0004) +#define BD_ENET_RX_OV ((ushort)0x0002) +#define BD_ENET_RX_CL ((ushort)0x0001) +#define BD_ENET_RX_STATS ((ushort)0x013f) /* All status bits */ + +/* + * Buffer descriptor control/status used by Ethernet transmit. + */ +#define BD_ENET_TX_READY ((ushort)0x8000) +#define BD_ENET_TX_PAD ((ushort)0x4000) +#define BD_ENET_TX_WRAP ((ushort)0x2000) +#define BD_ENET_TX_INTR ((ushort)0x1000) +#define BD_ENET_TX_LAST ((ushort)0x0800) +#define BD_ENET_TX_TC ((ushort)0x0400) +#define BD_ENET_TX_DEF ((ushort)0x0200) +#define BD_ENET_TX_HB ((ushort)0x0100) +#define BD_ENET_TX_LC ((ushort)0x0080) +#define BD_ENET_TX_RL ((ushort)0x0040) +#define BD_ENET_TX_UN ((ushort)0x0002) +#define BD_ENET_TX_CSL ((ushort)0x0001) +#define BD_ENET_TX_STATS ((ushort)0x03ff) /* All status bits */ + +#endif /* MPC5121_FEC_H */ diff --git a/drivers/net/fs_enet/Kconfig b/drivers/net/fs_enet/Kconfig index 562ea68..fc073b5 100644 --- a/drivers/net/fs_enet/Kconfig +++ b/drivers/net/fs_enet/Kconfig @@ -1,9 +1,13 @@ config FS_ENET tristate "Freescale Ethernet Driver" - depends on CPM1 || CPM2 + depends on CPM1 || CPM2 || PPC_MPC512x select MII select PHYLIB +config FS_ENET_MPC5121_FEC + def_bool y if (FS_ENET && PPC_MPC512x) + select FS_ENET_HAS_FEC + config FS_ENET_HAS_SCC bool "Chip has an SCC usable for ethernet" depends on FS_ENET && (CPM1 || CPM2) @@ -16,13 +20,13 @@ config FS_ENET_HAS_FCC config FS_ENET_HAS_FEC bool "Chip has an FEC usable for ethernet" - depends on FS_ENET && CPM1 + depends on FS_ENET && (CPM1 || FS_ENET_MPC5121_FEC) select FS_ENET_MDIO_FEC default y config FS_ENET_MDIO_FEC tristate "MDIO driver for FEC" - depends on FS_ENET && CPM1 + depends on FS_ENET && (CPM1 || FS_ENET_MPC5121_FEC) config FS_ENET_MDIO_FCC tristate "MDIO driver for FCC" diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c index f996a1a..4170d33 100644 --- a/drivers/net/fs_enet/fs_enet-main.c +++ b/drivers/net/fs_enet/fs_enet-main.c @@ -1183,11 +1183,18 @@ static struct of_device_id fs_enet_match[] = { }, #endif #ifdef CONFIG_FS_ENET_HAS_FEC +#ifdef CONFIG_FS_ENET_MPC5121_FEC + { + .compatible = "fsl,mpc5121-fec", + .data = (void *)&fs_fec_ops, + }, +#else { .compatible = "fsl,pq1-fec-enet", .data = (void *)&fs_fec_ops, }, #endif +#endif {} }; diff --git a/drivers/net/fs_enet/fs_enet.h b/drivers/net/fs_enet/fs_enet.h index 85a4bab..5d8258e 100644 --- a/drivers/net/fs_enet/fs_enet.h +++ b/drivers/net/fs_enet/fs_enet.h @@ -13,7 +13,13 @@ #ifdef CONFIG_CPM1 #include <asm/cpm1.h> +#endif + +#ifdef CONFIG_FS_ENET_MPC5121_FEC +#include <asm/mpc5121_fec.h> +#endif +#if defined(CONFIG_CPM1) || defined(CONFIG_FS_ENET_MPC5121_FEC) struct fec_info { fec_t __iomem *fecp; u32 mii_speed; diff --git a/drivers/net/fs_enet/mac-fec.c b/drivers/net/fs_enet/mac-fec.c index 14e5753..b069088 100644 --- a/drivers/net/fs_enet/mac-fec.c +++ b/drivers/net/fs_enet/mac-fec.c @@ -44,6 +44,10 @@ #include <asm/cpm1.h> #endif +#ifdef CONFIG_FS_ENET_MPC5121_FEC +#include <asm/mpc5121_fec.h> +#endif + #include "fs_enet.h" #include "fec.h" @@ -285,7 +289,11 @@ static void restart(struct net_device *dev) * Set maximum receive buffer size. */ FW(fecp, r_buff_size, PKT_MAXBLR_SIZE); +#ifdef CONFIG_FS_ENET_MPC5121_FEC + FW(fecp, r_cntrl, PKT_MAXBUF_SIZE << 16); +#else FW(fecp, r_hash, PKT_MAXBUF_SIZE); +#endif /* get physical address */ rx_bd_base_phys = fep->ring_mem_addr; @@ -300,9 +308,14 @@ static void restart(struct net_device *dev) fs_init_bds(dev); /* - * Enable big endian and don't care about SDMA FC. + * Enable big endian. */ +#ifndef CONFIG_FS_ENET_MPC5121_FEC + /* Don't care about SDMA FC. */ FW(fecp, fun_code, 0x78000000); +#else + FS(fecp, dma_control, 0xC0000000); +#endif /* * Set MII speed. @@ -313,7 +326,9 @@ static void restart(struct net_device *dev) * Clear any outstanding interrupt. */ FW(fecp, ievent, 0xffc0); +#ifndef CONFIG_FS_ENET_MPC5121_FEC FW(fecp, ivec, (virq_to_hw(fep->interrupt) / 2) << 29); +#endif /* * adjust to speed (only for DUET & RMII) @@ -344,8 +359,19 @@ static void restart(struct net_device *dev) } #endif + /* + * Enable MII + */ +#ifdef CONFIG_FS_ENET_MPC5121_FEC + /* + * Only set requested bit - do not touch maximum packet size + * configured earlier. + */ + FS(fecp, r_cntrl, FEC_RCNTRL_MII_MODE); +#else + FW(fecp, r_cntrl, FEC_RCNTRL_MII_MODE); +#endif - FW(fecp, r_cntrl, FEC_RCNTRL_MII_MODE); /* MII enable */ /* * adjust to duplex mode */ diff --git a/drivers/net/fs_enet/mii-fec.c b/drivers/net/fs_enet/mii-fec.c index 28077cc..9d8bd97 100644 --- a/drivers/net/fs_enet/mii-fec.c +++ b/drivers/net/fs_enet/mii-fec.c @@ -32,6 +32,7 @@ #include <linux/bitops.h> #include <linux/platform_device.h> #include <linux/of_platform.h> +#include <linux/time.h> #include <asm/pgtable.h> #include <asm/irq.h> @@ -211,9 +212,15 @@ static int fs_enet_mdio_remove(struct of_device *ofdev) } static struct of_device_id fs_enet_mdio_fec_match[] = { +#ifdef CONFIG_FS_ENET_MPC5121_FEC + { + .compatible = "fsl,mpc5121-fec-mdio", + }, +#else { .compatible = "fsl,pq1-fec-mdio", }, +#endif {}, };