Patchwork [02/12] fs_enet: Add MPC5121 FEC support.

login
register
mail settings
Submitter Wolfgang Denk
Date May 6, 2009, 8:15 p.m.
Message ID <1241640919-4650-3-git-send-email-wd@denx.de>
Download mbox | patch
Permalink /patch/26927/
State Changes Requested, archived
Headers show

Comments

Wolfgang Denk - May 6, 2009, 8:15 p.m.
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
Grant Likely - May 6, 2009, 8:33 p.m.
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.
David Miller - May 6, 2009, 8:40 p.m.
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.
Scott Wood - May 6, 2009, 8:41 p.m.
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
Scott Wood - May 6, 2009, 9:08 p.m.
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
Wolfgang Denk - May 6, 2009, 10:01 p.m.
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
Wolfgang Denk - May 6, 2009, 10:06 p.m.
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
Wolfgang Denk - May 6, 2009, 10:09 p.m.
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
Grant Likely - May 6, 2009, 10:29 p.m.
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.
Grant Likely - May 6, 2009, 10:39 p.m.
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.
Wolfgang Denk - May 6, 2009, 10:41 p.m.
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
David Jander - May 7, 2009, 8:14 a.m.
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,
Kumar Gala - May 7, 2009, 1:05 p.m.
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
Grant Likely - May 7, 2009, 2:09 p.m.
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.
John Rigby - May 8, 2009, 2:02 a.m.
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.
>
David Miller - May 8, 2009, 7:52 a.m.
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.
David Jander - May 11, 2009, 6:56 a.m.
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,
Piotr Ziecik - May 14, 2009, 12:38 p.m.
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.
Grant Likely - May 14, 2009, 2 p.m.
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
>
Benjamin Herrenschmidt - May 18, 2009, 10:17 p.m.
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.
Piotr Ziecik - May 19, 2009, 11:26 a.m.
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 ?
Benjamin Herrenschmidt - May 19, 2009, 9:56 p.m.
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
Piotr Ziecik - May 21, 2009, 8:34 a.m.
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.
Grant Likely - May 21, 2009, 3:36 p.m.
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.

Patch

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
 	{},
 };