| Message ID | 1294297998-26930-6-git-send-email-shawn.guo@freescale.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, Jan 06, 2011 at 03:13:13PM +0800, Shawn Guo wrote: > This patch is to add mx28 dual fec support. Here are some key notes > for mx28 fec controller. > > - The mx28 fec controller naming ENET-MAC is a different IP from FEC > used on other i.mx variants. But they are basically compatible > on software interface, so it's possible to share the same driver. > - ENET-MAC design on mx28 made an improper assumption that it runs > on a big-endian system. As the result, driver has to swap every > frame going to and coming from the controller. > - The external phys can only be configured by fec0, which means fec1 > can not work independently and both phys need to be configured by > mii_bus attached on fec0. > - ENET-MAC reset will get mac address registers reset too. > - ENET-MAC MII/RMII mode and 10M/100M speed are configured > differently FEC. > - ETHER_EN bit must be set to get ENET-MAC interrupt work. > > Signed-off-by: Shawn Guo <shawn.guo@freescale.com> > --- > Changes for v4: > - Use #ifndef CONFIG_ARM to include ColdFire header files > - Define quirk bits in id_entry.driver_data to handle controller > difference, which is more scalable than using device name > - Define fec0_mii_bus as a static function in fec_enet_mii_init > to fold the mii_bus instance attached on fec0 > - Use cpu_to_be32 over __swab32 in function swap_buffer > > Changes for v3: > - Move v2 changes into patch #3 > - Use device name to check if it's running on ENET-MAC > > drivers/net/Kconfig | 7 ++- > drivers/net/fec.c | 148 +++++++++++++++++++++++++++++++++++++++++++++------ > drivers/net/fec.h | 5 +- > 3 files changed, 139 insertions(+), 21 deletions(-) > > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig > index 4f1755b..f34629b 100644 > --- a/drivers/net/Kconfig > +++ b/drivers/net/Kconfig > @@ -1944,18 +1944,19 @@ config 68360_ENET > config FEC > bool "FEC ethernet controller (of ColdFire and some i.MX CPUs)" > depends on M523x || M527x || M5272 || M528x || M520x || M532x || \ > - MACH_MX27 || ARCH_MX35 || ARCH_MX25 || ARCH_MX5 > + MACH_MX27 || ARCH_MX35 || ARCH_MX25 || ARCH_MX5 || SOC_IMX28 > select PHYLIB > help > Say Y here if you want to use the built-in 10/100 Fast ethernet > controller on some Motorola ColdFire and Freescale i.MX processors. > > config FEC2 > - bool "Second FEC ethernet controller (on some ColdFire CPUs)" > + bool "Second FEC ethernet controller" > depends on FEC > help > Say Y here if you want to use the second built-in 10/100 Fast > - ethernet controller on some Motorola ColdFire processors. > + ethernet controller on some Motorola ColdFire and Freescale > + i.MX processors. This option is used nowhere and should be removed. Certainly it does not have the effect of enabling the second ethernet controller. Sascha
On Tue, Jan 11, 2011 at 11:27:17AM +0100, Sascha Hauer wrote: > On Thu, Jan 06, 2011 at 03:13:13PM +0800, Shawn Guo wrote: > > This patch is to add mx28 dual fec support. Here are some key notes > > for mx28 fec controller. > > > > - The mx28 fec controller naming ENET-MAC is a different IP from FEC > > used on other i.mx variants. But they are basically compatible > > on software interface, so it's possible to share the same driver. > > - ENET-MAC design on mx28 made an improper assumption that it runs > > on a big-endian system. As the result, driver has to swap every > > frame going to and coming from the controller. > > - The external phys can only be configured by fec0, which means fec1 > > can not work independently and both phys need to be configured by > > mii_bus attached on fec0. > > - ENET-MAC reset will get mac address registers reset too. > > - ENET-MAC MII/RMII mode and 10M/100M speed are configured > > differently FEC. > > - ETHER_EN bit must be set to get ENET-MAC interrupt work. > > > > Signed-off-by: Shawn Guo <shawn.guo@freescale.com> > > --- > > Changes for v4: > > - Use #ifndef CONFIG_ARM to include ColdFire header files > > - Define quirk bits in id_entry.driver_data to handle controller > > difference, which is more scalable than using device name > > - Define fec0_mii_bus as a static function in fec_enet_mii_init > > to fold the mii_bus instance attached on fec0 > > - Use cpu_to_be32 over __swab32 in function swap_buffer > > > > Changes for v3: > > - Move v2 changes into patch #3 > > - Use device name to check if it's running on ENET-MAC > > > > drivers/net/Kconfig | 7 ++- > > drivers/net/fec.c | 148 +++++++++++++++++++++++++++++++++++++++++++++------ > > drivers/net/fec.h | 5 +- > > 3 files changed, 139 insertions(+), 21 deletions(-) > > > > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig > > index 4f1755b..f34629b 100644 > > --- a/drivers/net/Kconfig > > +++ b/drivers/net/Kconfig > > @@ -1944,18 +1944,19 @@ config 68360_ENET > > config FEC > > bool "FEC ethernet controller (of ColdFire and some i.MX CPUs)" > > depends on M523x || M527x || M5272 || M528x || M520x || M532x || \ > > - MACH_MX27 || ARCH_MX35 || ARCH_MX25 || ARCH_MX5 > > + MACH_MX27 || ARCH_MX35 || ARCH_MX25 || ARCH_MX5 || SOC_IMX28 > > select PHYLIB > > help > > Say Y here if you want to use the built-in 10/100 Fast ethernet > > controller on some Motorola ColdFire and Freescale i.MX processors. > > > > config FEC2 > > - bool "Second FEC ethernet controller (on some ColdFire CPUs)" > > + bool "Second FEC ethernet controller" > > depends on FEC > > help > > Say Y here if you want to use the second built-in 10/100 Fast > > - ethernet controller on some Motorola ColdFire processors. > > + ethernet controller on some Motorola ColdFire and Freescale > > + i.MX processors. > > This option is used nowhere and should be removed. Certainly it does not > have the effect of enabling the second ethernet controller. > As David has merged it, I would send a follow-up patch to remove it.
On 11/01/11 20:27, Sascha Hauer wrote: > On Thu, Jan 06, 2011 at 03:13:13PM +0800, Shawn Guo wrote: >> This patch is to add mx28 dual fec support. Here are some key notes >> for mx28 fec controller. >> >> - The mx28 fec controller naming ENET-MAC is a different IP from FEC >> used on other i.mx variants. But they are basically compatible >> on software interface, so it's possible to share the same driver. >> - ENET-MAC design on mx28 made an improper assumption that it runs >> on a big-endian system. As the result, driver has to swap every >> frame going to and coming from the controller. >> - The external phys can only be configured by fec0, which means fec1 >> can not work independently and both phys need to be configured by >> mii_bus attached on fec0. >> - ENET-MAC reset will get mac address registers reset too. >> - ENET-MAC MII/RMII mode and 10M/100M speed are configured >> differently FEC. >> - ETHER_EN bit must be set to get ENET-MAC interrupt work. >> >> Signed-off-by: Shawn Guo<shawn.guo@freescale.com> >> --- >> Changes for v4: >> - Use #ifndef CONFIG_ARM to include ColdFire header files >> - Define quirk bits in id_entry.driver_data to handle controller >> difference, which is more scalable than using device name >> - Define fec0_mii_bus as a static function in fec_enet_mii_init >> to fold the mii_bus instance attached on fec0 >> - Use cpu_to_be32 over __swab32 in function swap_buffer >> >> Changes for v3: >> - Move v2 changes into patch #3 >> - Use device name to check if it's running on ENET-MAC >> >> drivers/net/Kconfig | 7 ++- >> drivers/net/fec.c | 148 +++++++++++++++++++++++++++++++++++++++++++++------ >> drivers/net/fec.h | 5 +- >> 3 files changed, 139 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig >> index 4f1755b..f34629b 100644 >> --- a/drivers/net/Kconfig >> +++ b/drivers/net/Kconfig >> @@ -1944,18 +1944,19 @@ config 68360_ENET >> config FEC >> bool "FEC ethernet controller (of ColdFire and some i.MX CPUs)" >> depends on M523x || M527x || M5272 || M528x || M520x || M532x || \ >> - MACH_MX27 || ARCH_MX35 || ARCH_MX25 || ARCH_MX5 >> + MACH_MX27 || ARCH_MX35 || ARCH_MX25 || ARCH_MX5 || SOC_IMX28 >> select PHYLIB >> help >> Say Y here if you want to use the built-in 10/100 Fast ethernet >> controller on some Motorola ColdFire and Freescale i.MX processors. >> >> config FEC2 >> - bool "Second FEC ethernet controller (on some ColdFire CPUs)" >> + bool "Second FEC ethernet controller" >> depends on FEC >> help >> Say Y here if you want to use the second built-in 10/100 Fast >> - ethernet controller on some Motorola ColdFire processors. >> + ethernet controller on some Motorola ColdFire and Freescale >> + i.MX processors. > > This option is used nowhere and should be removed. Certainly it does not > have the effect of enabling the second ethernet controller. It does for a ColdFire platform... grep -r CONFIG_FEC2 * arch/m68knommu/configs/m5275evb_defconfig:CONFIG_FEC2=y arch/m68knommu/platform/527x/config.c:#ifdef CONFIG_FEC2 arch/m68knommu/platform/527x/config.c:#ifdef CONFIG_FEC2 Regards Greg ------------------------------------------------------------------------ Greg Ungerer -- Principal Engineer EMAIL: gerg@snapgear.com SnapGear Group, McAfee PHONE: +61 7 3435 2888 8 Gardner Close, FAX: +61 7 3891 3630 Milton, QLD, 4064, Australia WEB: http://www.SnapGear.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Greg, On Tue, Jan 11, 2011 at 10:24:12PM +1000, Greg Ungerer wrote: > On 11/01/11 20:27, Sascha Hauer wrote: > >On Thu, Jan 06, 2011 at 03:13:13PM +0800, Shawn Guo wrote: > >This option is used nowhere and should be removed. Certainly it does not > >have the effect of enabling the second ethernet controller. > > It does for a ColdFire platform... > > grep -r CONFIG_FEC2 * > > arch/m68knommu/configs/m5275evb_defconfig:CONFIG_FEC2=y > arch/m68knommu/platform/527x/config.c:#ifdef CONFIG_FEC2 > arch/m68knommu/platform/527x/config.c:#ifdef CONFIG_FEC2 IMHO Sascha's comment[1] applies here, too. And someone might want to do what he suggested soon or the patch removing CONFIG_FEC2[2] needs to be commented accordingly. Best regards Uwe [1] http://thread.gmane.org/gmane.linux.network/182929/focus=183378 [2] http://mid.gmane.org/1294747672-4433-1-git-send-email-shawn.guo@freescale.com
Hi Uwe, On 11/01/11 23:07, Uwe Kleine-König wrote: > Hi Greg, > > On Tue, Jan 11, 2011 at 10:24:12PM +1000, Greg Ungerer wrote: >> On 11/01/11 20:27, Sascha Hauer wrote: >>> On Thu, Jan 06, 2011 at 03:13:13PM +0800, Shawn Guo wrote: >>> This option is used nowhere and should be removed. Certainly it does not >>> have the effect of enabling the second ethernet controller. >> >> It does for a ColdFire platform... >> >> grep -r CONFIG_FEC2 * >> >> arch/m68knommu/configs/m5275evb_defconfig:CONFIG_FEC2=y >> arch/m68knommu/platform/527x/config.c:#ifdef CONFIG_FEC2 >> arch/m68knommu/platform/527x/config.c:#ifdef CONFIG_FEC2 > IMHO Sascha's comment[1] applies here, too. I am not arguing that it doesn't :-) Simply that removing that config option and doing nothing else would not be the right thing to do. Regards Greg > And someone might want to do what he suggested soon or the patch > removing CONFIG_FEC2[2] needs to be commented accordingly. > > Best regards > Uwe > > [1] http://thread.gmane.org/gmane.linux.network/182929/focus=183378 > [2] http://mid.gmane.org/1294747672-4433-1-git-send-email-shawn.guo@freescale.com >
Hi Greg, On Tue, Jan 11, 2011 at 11:25:41PM +1000, Greg Ungerer wrote: > On 11/01/11 23:07, Uwe Kleine-König wrote: > >On Tue, Jan 11, 2011 at 10:24:12PM +1000, Greg Ungerer wrote: > >>On 11/01/11 20:27, Sascha Hauer wrote: > >>>On Thu, Jan 06, 2011 at 03:13:13PM +0800, Shawn Guo wrote: > >>>This option is used nowhere and should be removed. Certainly it does not > >>>have the effect of enabling the second ethernet controller. > >> > >>It does for a ColdFire platform... > >> > >>grep -r CONFIG_FEC2 * > >> > >>arch/m68knommu/configs/m5275evb_defconfig:CONFIG_FEC2=y > >>arch/m68knommu/platform/527x/config.c:#ifdef CONFIG_FEC2 > >>arch/m68knommu/platform/527x/config.c:#ifdef CONFIG_FEC2 > >IMHO Sascha's comment[1] applies here, too. > > I am not arguing that it doesn't :-) > > Simply that removing that config option and doing nothing > else would not be the right thing to do. > > Regards > Greg > > > >And someone might want to do what he suggested soon or the patch > >removing CONFIG_FEC2[2] needs to be commented accordingly. note that davem took the patch removing CONFIG_FEC2 now. Best regards Uwe
Hello, hmm, this review comes to late, probably I will post a follow-up patch for the low-hanging fruits at least. On Thu, Jan 06, 2011 at 03:13:13PM +0800, Shawn Guo wrote: > This patch is to add mx28 dual fec support. Here are some key notes > for mx28 fec controller. > > - The mx28 fec controller naming ENET-MAC is a different IP from FEC > used on other i.mx variants. But they are basically compatible > on software interface, so it's possible to share the same driver. > - ENET-MAC design on mx28 made an improper assumption that it runs > on a big-endian system. As the result, driver has to swap every > frame going to and coming from the controller. > - The external phys can only be configured by fec0, which means fec1 > can not work independently and both phys need to be configured by > mii_bus attached on fec0. > - ENET-MAC reset will get mac address registers reset too. > - ENET-MAC MII/RMII mode and 10M/100M speed are configured > differently FEC. > - ETHER_EN bit must be set to get ENET-MAC interrupt work. > > Signed-off-by: Shawn Guo <shawn.guo@freescale.com> > --- > Changes for v4: > - Use #ifndef CONFIG_ARM to include ColdFire header files I intended you to not use CONFIG_ARCH_MXS at all, at least up to now CONFIG_ARM works quite well. > - Define quirk bits in id_entry.driver_data to handle controller > difference, which is more scalable than using device name > - Define fec0_mii_bus as a static function in fec_enet_mii_init > to fold the mii_bus instance attached on fec0 IMHO not very good. At least the current code doesn't allow to have two dual-fecs, because the 2nd dual-fec's slave would be attached to the 1st dual-fec's mii_bus. Don't know a nice solution though. Probably you either need a slave pointer in platform_data or have to treat a dual-fec as only a single device. > - Use cpu_to_be32 over __swab32 in function swap_buffer > > Changes for v3: > - Move v2 changes into patch #3 > - Use device name to check if it's running on ENET-MAC > > drivers/net/Kconfig | 7 ++- > drivers/net/fec.c | 148 +++++++++++++++++++++++++++++++++++++++++++++------ > drivers/net/fec.h | 5 +- > 3 files changed, 139 insertions(+), 21 deletions(-) > > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig > index 4f1755b..f34629b 100644 > --- a/drivers/net/Kconfig > +++ b/drivers/net/Kconfig > @@ -1944,18 +1944,19 @@ config 68360_ENET > config FEC > bool "FEC ethernet controller (of ColdFire and some i.MX CPUs)" > depends on M523x || M527x || M5272 || M528x || M520x || M532x || \ > - MACH_MX27 || ARCH_MX35 || ARCH_MX25 || ARCH_MX5 > + MACH_MX27 || ARCH_MX35 || ARCH_MX25 || ARCH_MX5 || SOC_IMX28 IMX_HAVE_PLATFORM_FEC || MXS_HAVE_PLATFORM_FEC ? Again this calls for a more global approach for these registration facilities. > select PHYLIB > help > Say Y here if you want to use the built-in 10/100 Fast ethernet > controller on some Motorola ColdFire and Freescale i.MX processors. > > config FEC2 > - bool "Second FEC ethernet controller (on some ColdFire CPUs)" > + bool "Second FEC ethernet controller" > depends on FEC > help > Say Y here if you want to use the second built-in 10/100 Fast > - ethernet controller on some Motorola ColdFire processors. > + ethernet controller on some Motorola ColdFire and Freescale > + i.MX processors. > > config FEC_MPC52xx > tristate "MPC52xx FEC driver" > diff --git a/drivers/net/fec.c b/drivers/net/fec.c > index 8a1c51f..2a71373 100644 > --- a/drivers/net/fec.c > +++ b/drivers/net/fec.c > @@ -17,6 +17,8 @@ > * > * Bug fixes and cleanup by Philippe De Muyter (phdm@macqel.be) > * Copyright (c) 2004-2006 Macq Electronique SA. > + * > + * Copyright (C) 2010 Freescale Semiconductor, Inc. > */ > > #include <linux/module.h> > @@ -45,20 +47,36 @@ > > #include <asm/cacheflush.h> > > -#ifndef CONFIG_ARCH_MXC > +#ifndef CONFIG_ARM > #include <asm/coldfire.h> > #include <asm/mcfsim.h> > #endif > > #include "fec.h" > > -#ifdef CONFIG_ARCH_MXC > -#include <mach/hardware.h> > +#if defined(CONFIG_ARCH_MXC) || defined(CONFIG_SOC_IMX28) > #define FEC_ALIGNMENT 0xf > #else > #define FEC_ALIGNMENT 0x3 > #endif > > +#define DRIVER_NAME "fec" > + > +/* Controller is ENET-MAC */ > +#define FEC_QUIRK_ENET_MAC (1 << 0) does this really qualify to be a quirk? > +/* Controller needs driver to swap frame */ > +#define FEC_QUIRK_SWAP_FRAME (1 << 1) IMHO this is a bit misnamed. FEC_QUIRK_NEEDS_BE_DATA or similar would be more accurate. > +static struct platform_device_id fec_devtype[] = { > + { > + .name = DRIVER_NAME, > + .driver_data = 0, > + }, { > + .name = "imx28-fec", > + .driver_data = FEC_QUIRK_ENET_MAC | FEC_QUIRK_SWAP_FRAME, > + } > +}; > + > static unsigned char macaddr[ETH_ALEN]; > module_param_array(macaddr, byte, NULL, 0); > MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); > @@ -129,7 +147,8 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); > * account when setting it. > */ > #if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \ > - defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARCH_MXC) > + defined(CONFIG_M520x) || defined(CONFIG_M532x) || \ > + defined(CONFIG_ARCH_MXC) || defined(CONFIG_SOC_IMX28) I wonder what is excluded here. FEC depends on M523x || M527x || M5272 || M528x || M520x || M532x || \ MACH_MX27 || ARCH_MX35 || ARCH_MX25 || ARCH_MX5 || SOC_IMX28 so the only difference is that the latter lists M5272 which seems a bit redundant in the presence of M527x. > #define OPT_FRAME_SIZE (PKT_MAXBUF_SIZE << 16) > #else > #define OPT_FRAME_SIZE 0 > @@ -208,10 +227,23 @@ static void fec_stop(struct net_device *dev); > /* Transmitter timeout */ > #define TX_TIMEOUT (2 * HZ) > > +static void *swap_buffer(void *bufaddr, int len) > +{ > + int i; > + unsigned int *buf = bufaddr; > + > + for (i = 0; i < (len + 3) / 4; i++, buf++) > + *buf = cpu_to_be32(*buf); if len isn't a multiple of 4 this accesses bytes behind len. Is this generally OK here? (E.g. because skbs always have a length that is a multiple of 4?) > + > + return bufaddr; > +} > + > static netdev_tx_t > fec_enet_start_xmit(struct sk_buff *skb, struct net_device *dev) > { > struct fec_enet_private *fep = netdev_priv(dev); > + const struct platform_device_id *id_entry = > + platform_get_device_id(fep->pdev); > struct bufdesc *bdp; > void *bufaddr; > unsigned short status; > @@ -256,6 +288,14 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *dev) > bufaddr = fep->tx_bounce[index]; > } > > + /* > + * Some design made an incorrect assumption on endian mode of > + * the system that it's running on. As the result, driver has to > + * swap every frame going to and coming from the controller. > + */ > + if (id_entry->driver_data & FEC_QUIRK_SWAP_FRAME) > + swap_buffer(bufaddr, skb->len); > + > /* Save skb pointer */ > fep->tx_skbuff[fep->skb_cur] = skb; > > @@ -424,6 +464,8 @@ static void > fec_enet_rx(struct net_device *dev) > { > struct fec_enet_private *fep = netdev_priv(dev); > + const struct platform_device_id *id_entry = > + platform_get_device_id(fep->pdev); > struct bufdesc *bdp; > unsigned short status; > struct sk_buff *skb; > @@ -487,6 +529,9 @@ fec_enet_rx(struct net_device *dev) > dma_unmap_single(NULL, bdp->cbd_bufaddr, bdp->cbd_datlen, > DMA_FROM_DEVICE); > > + if (id_entry->driver_data & FEC_QUIRK_SWAP_FRAME) > + swap_buffer(data, pkt_len); > + > /* This does 16 byte alignment, exactly what we need. > * The packet length includes FCS, but we don't want to > * include that when passing upstream as it messes up > @@ -689,6 +734,7 @@ static int fec_enet_mii_probe(struct net_device *dev) > char mdio_bus_id[MII_BUS_ID_SIZE]; > char phy_name[MII_BUS_ID_SIZE + 3]; > int phy_id; > + int dev_id = fep->pdev->id; > > fep->phy_dev = NULL; > > @@ -700,6 +746,8 @@ static int fec_enet_mii_probe(struct net_device *dev) > continue; > if (fep->mii_bus->phy_map[phy_id]->phy_id == 0) > continue; > + if (dev_id--) > + continue; > strncpy(mdio_bus_id, fep->mii_bus->id, MII_BUS_ID_SIZE); > break; > } > @@ -737,10 +785,35 @@ static int fec_enet_mii_probe(struct net_device *dev) > > static int fec_enet_mii_init(struct platform_device *pdev) > { > + static struct mii_bus *fec0_mii_bus; > struct net_device *dev = platform_get_drvdata(pdev); > struct fec_enet_private *fep = netdev_priv(dev); > + const struct platform_device_id *id_entry = > + platform_get_device_id(fep->pdev); > int err = -ENXIO, i; > > + /* > + * The dual fec interfaces are not equivalent with enet-mac. > + * Here are the differences: > + * > + * - fec0 supports MII & RMII modes while fec1 only supports RMII > + * - fec0 acts as the 1588 time master while fec1 is slave > + * - external phys can only be configured by fec0 > + * > + * That is to say fec1 can not work independently. It only works > + * when fec0 is working. The reason behind this design is that the > + * second interface is added primarily for Switch mode. > + * > + * Because of the last point above, both phys are attached on fec0 > + * mdio interface in board design, and need to be configured by > + * fec0 mii_bus. > + */ > + if ((id_entry->driver_data & FEC_QUIRK_ENET_MAC) && pdev->id) { > + /* fec1 uses fec0 mii_bus */ > + fep->mii_bus = fec0_mii_bus; > + return 0; What happens if imx28-fec.1 is probed before imx28-fec.0? > + } > + > fep->mii_timeout = 0; > > /* > @@ -777,6 +850,10 @@ static int fec_enet_mii_init(struct platform_device *pdev) > if (mdiobus_register(fep->mii_bus)) > goto err_out_free_mdio_irq; > > + /* save fec0 mii_bus */ > + if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) > + fec0_mii_bus = fep->mii_bus; > + > return 0; > > err_out_free_mdio_irq: > @@ -1148,12 +1225,25 @@ static void > fec_restart(struct net_device *dev, int duplex) > { > struct fec_enet_private *fep = netdev_priv(dev); > + const struct platform_device_id *id_entry = > + platform_get_device_id(fep->pdev); > int i; > + u32 val, temp_mac[2]; > > /* Whack a reset. We should wait for this. */ > writel(1, fep->hwp + FEC_ECNTRL); > udelay(10); > > + /* > + * enet-mac reset will reset mac address registers too, > + * so need to reconfigure it. > + */ > + if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) { > + memcpy(&temp_mac, dev->dev_addr, ETH_ALEN); > + writel(cpu_to_be32(temp_mac[0]), fep->hwp + FEC_ADDR_LOW); > + writel(cpu_to_be32(temp_mac[1]), fep->hwp + FEC_ADDR_HIGH); > + } > + > /* Clear any outstanding interrupt. */ > writel(0xffc00000, fep->hwp + FEC_IEVENT); > > @@ -1200,20 +1290,45 @@ fec_restart(struct net_device *dev, int duplex) > /* Set MII speed */ > writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED); > > -#ifdef FEC_MIIGSK_ENR > - if (fep->phy_interface == PHY_INTERFACE_MODE_RMII) { > - /* disable the gasket and wait */ > - writel(0, fep->hwp + FEC_MIIGSK_ENR); > - while (readl(fep->hwp + FEC_MIIGSK_ENR) & 4) > - udelay(1); > + /* > + * The phy interface and speed need to get configured > + * differently on enet-mac. > + */ > + if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) { > + val = readl(fep->hwp + FEC_R_CNTRL); > > - /* configure the gasket: RMII, 50 MHz, no loopback, no echo */ > - writel(1, fep->hwp + FEC_MIIGSK_CFGR); > + /* MII or RMII */ > + if (fep->phy_interface == PHY_INTERFACE_MODE_RMII) > + val |= (1 << 8); Can we have a #define for 1 << 8 please? > + else > + val &= ~(1 << 8); Best regards Uwe
Hello again, On Thu, Jan 06, 2011 at 03:13:13PM +0800, Shawn Guo wrote: > static netdev_tx_t > fec_enet_start_xmit(struct sk_buff *skb, struct net_device *dev) > { > struct fec_enet_private *fep = netdev_priv(dev); > + const struct platform_device_id *id_entry = > + platform_get_device_id(fep->pdev); > struct bufdesc *bdp; > void *bufaddr; > unsigned short status; > @@ -256,6 +288,14 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *dev) > bufaddr = fep->tx_bounce[index]; > } > > + /* > + * Some design made an incorrect assumption on endian mode of > + * the system that it's running on. As the result, driver has to > + * swap every frame going to and coming from the controller. > + */ > + if (id_entry->driver_data & FEC_QUIRK_SWAP_FRAME) > + swap_buffer(bufaddr, skb->len); > + Is that save here? bufaddr either points to a bounce buffer (which should be OK definitely) or skb->data. Or asked differently: Is the skb here owned by the driver such that it is allowed to write to it? Does the driver eventually need to restore the original data? Just before this if, there is some bounce buffer handling. If it is not OK to modify skb->data, the call to swap_buffer can easily be moved in there. > /* Save skb pointer */ > fep->tx_skbuff[fep->skb_cur] = skb; > > @@ -424,6 +464,8 @@ static void > fec_enet_rx(struct net_device *dev) > { > struct fec_enet_private *fep = netdev_priv(dev); > + const struct platform_device_id *id_entry = > + platform_get_device_id(fep->pdev); > struct bufdesc *bdp; > unsigned short status; > struct sk_buff *skb; > @@ -487,6 +529,9 @@ fec_enet_rx(struct net_device *dev) > dma_unmap_single(NULL, bdp->cbd_bufaddr, bdp->cbd_datlen, > DMA_FROM_DEVICE); > > + if (id_entry->driver_data & FEC_QUIRK_SWAP_FRAME) > + swap_buffer(data, pkt_len); > + Here I guess it's OK, the hardware just wrote to the buffer, so the skb cannot be shared to anything else and the write is all right. Best regards Uwe
Hi Uwe, On Thu, Jan 13, 2011 at 03:48:05PM +0100, Uwe Kleine-König wrote: [...] > > +/* Controller is ENET-MAC */ > > +#define FEC_QUIRK_ENET_MAC (1 << 0) > does this really qualify to be a quirk? > My understanding is that ENET-MAC is a type of "quirky" FEC controller. > > +/* Controller needs driver to swap frame */ > > +#define FEC_QUIRK_SWAP_FRAME (1 << 1) > IMHO this is a bit misnamed. FEC_QUIRK_NEEDS_BE_DATA or similar would > be more accurate. > When your make this change, you may want to pick a better name for function swap_buffer too. [...] > > +static void *swap_buffer(void *bufaddr, int len) > > +{ > > + int i; > > + unsigned int *buf = bufaddr; > > + > > + for (i = 0; i < (len + 3) / 4; i++, buf++) > > + *buf = cpu_to_be32(*buf); > if len isn't a multiple of 4 this accesses bytes behind len. Is this > generally OK here? (E.g. because skbs always have a length that is a > multiple of 4?) The len may not be a multiple of 4. But I believe bufaddr is always a buffer allocated in a length that is a multiple of 4, and the 1~3 bytes exceeding the len very likely has no data that matters. But yes, it deserves a safer implementation. [...] > > + /* > > + * The dual fec interfaces are not equivalent with enet-mac. > > + * Here are the differences: > > + * > > + * - fec0 supports MII & RMII modes while fec1 only supports RMII > > + * - fec0 acts as the 1588 time master while fec1 is slave > > + * - external phys can only be configured by fec0 > > + * > > + * That is to say fec1 can not work independently. It only works > > + * when fec0 is working. The reason behind this design is that the > > + * second interface is added primarily for Switch mode. > > + * > > + * Because of the last point above, both phys are attached on fec0 > > + * mdio interface in board design, and need to be configured by > > + * fec0 mii_bus. > > + */ > > + if ((id_entry->driver_data & FEC_QUIRK_ENET_MAC) && pdev->id) { > > + /* fec1 uses fec0 mii_bus */ > > + fep->mii_bus = fec0_mii_bus; > > + return 0; > What happens if imx28-fec.1 is probed before imx28-fec.0? It's something that generally should not happen, as these two fec are not equivalent, and fec.1 should always be added after fec.0 if you intend to get dual interfaces. But yes, we should add error checking for this case in the driver.
On Fri, Jan 14, 2011 at 01:48:40PM +0800, Shawn Guo wrote: > Hi Uwe, > > On Thu, Jan 13, 2011 at 03:48:05PM +0100, Uwe Kleine-König wrote: > > [...] > > > > +/* Controller is ENET-MAC */ > > > +#define FEC_QUIRK_ENET_MAC (1 << 0) > > does this really qualify to be a quirk? > > > My understanding is that ENET-MAC is a type of "quirky" FEC > controller. > > > > +/* Controller needs driver to swap frame */ > > > +#define FEC_QUIRK_SWAP_FRAME (1 << 1) > > IMHO this is a bit misnamed. FEC_QUIRK_NEEDS_BE_DATA or similar would > > be more accurate. > > > When your make this change, you may want to pick a better name for > function swap_buffer too. > > [...] > > > > +static void *swap_buffer(void *bufaddr, int len) > > > +{ > > > + int i; > > > + unsigned int *buf = bufaddr; > > > + > > > + for (i = 0; i < (len + 3) / 4; i++, buf++) > > > + *buf = cpu_to_be32(*buf); > > if len isn't a multiple of 4 this accesses bytes behind len. Is this > > generally OK here? (E.g. because skbs always have a length that is a > > multiple of 4?) > The len may not be a multiple of 4. But I believe bufaddr is always > a buffer allocated in a length that is a multiple of 4, and the 1~3 > bytes exceeding the len very likely has no data that matters. But > yes, it deserves a safer implementation. Did you test what happens if bufaddr isn't aligned? Does it work at all then? Best regards Uwe
On Fri, Jan 14, 2011 at 08:52:23AM +0100, Uwe Kleine-König wrote: > On Fri, Jan 14, 2011 at 01:48:40PM +0800, Shawn Guo wrote: > > Hi Uwe, > > > > On Thu, Jan 13, 2011 at 03:48:05PM +0100, Uwe Kleine-König wrote: > > > > [...] > > > > > > +/* Controller is ENET-MAC */ > > > > +#define FEC_QUIRK_ENET_MAC (1 << 0) > > > does this really qualify to be a quirk? > > > > > My understanding is that ENET-MAC is a type of "quirky" FEC > > controller. > > > > > > +/* Controller needs driver to swap frame */ > > > > +#define FEC_QUIRK_SWAP_FRAME (1 << 1) > > > IMHO this is a bit misnamed. FEC_QUIRK_NEEDS_BE_DATA or similar would > > > be more accurate. > > > > > When your make this change, you may want to pick a better name for > > function swap_buffer too. > > > > [...] > > > > > > +static void *swap_buffer(void *bufaddr, int len) > > > > +{ > > > > + int i; > > > > + unsigned int *buf = bufaddr; > > > > + > > > > + for (i = 0; i < (len + 3) / 4; i++, buf++) > > > > + *buf = cpu_to_be32(*buf); > > > if len isn't a multiple of 4 this accesses bytes behind len. Is this > > > generally OK here? (E.g. because skbs always have a length that is a > > > multiple of 4?) > > The len may not be a multiple of 4. But I believe bufaddr is always > > a buffer allocated in a length that is a multiple of 4, and the 1~3 > > bytes exceeding the len very likely has no data that matters. But > > yes, it deserves a safer implementation. > Did you test what happens if bufaddr isn't aligned? Does it work at all > then? > I see many calls passing a len that is not a multiple of 4, but it works good.
Hi, Shawn Guo writes: > On Fri, Jan 14, 2011 at 08:52:23AM +0100, Uwe Kleine-König wrote: > > On Fri, Jan 14, 2011 at 01:48:40PM +0800, Shawn Guo wrote: > > > Hi Uwe, > > > > > > On Thu, Jan 13, 2011 at 03:48:05PM +0100, Uwe Kleine-König wrote: > > > > > > [...] > > > > > > > > +/* Controller is ENET-MAC */ > > > > > +#define FEC_QUIRK_ENET_MAC (1 << 0) > > > > does this really qualify to be a quirk? > > > > > > > My understanding is that ENET-MAC is a type of "quirky" FEC > > > controller. > > > > > > > > +/* Controller needs driver to swap frame */ > > > > > +#define FEC_QUIRK_SWAP_FRAME (1 << 1) > > > > IMHO this is a bit misnamed. FEC_QUIRK_NEEDS_BE_DATA or similar would > > > > be more accurate. > > > > > > > When your make this change, you may want to pick a better name for > > > function swap_buffer too. > > > > > > [...] > > > > > > > > +static void *swap_buffer(void *bufaddr, int len) > > > > > +{ > > > > > + int i; > > > > > + unsigned int *buf = bufaddr; > > > > > + > > > > > + for (i = 0; i < (len + 3) / 4; i++, buf++) > > > > > + *buf = cpu_to_be32(*buf); > > > > if len isn't a multiple of 4 this accesses bytes behind len. Is this > > > > generally OK here? (E.g. because skbs always have a length that is a > > > > multiple of 4?) > > > The len may not be a multiple of 4. But I believe bufaddr is always > > > a buffer allocated in a length that is a multiple of 4, and the 1~3 > > > bytes exceeding the len very likely has no data that matters. But > > > yes, it deserves a safer implementation. > > Did you test what happens if bufaddr isn't aligned? Does it work at all > > then? > > > I see many calls passing a len that is not a multiple of 4, but it > works good. > That does not prove anything, actually. Anyway "bufaddr isn't aligned" != "len is not a multiple of 4". Is there any guarantee that the function cannot be called with a non-aligned buffer address? Lothar Waßmann
Hi, On Mon, Jan 17, 2011 at 09:16:22AM +0100, Lothar Waßmann wrote: > Shawn Guo writes: > > On Fri, Jan 14, 2011 at 08:52:23AM +0100, Uwe Kleine-König wrote: > > > On Fri, Jan 14, 2011 at 01:48:40PM +0800, Shawn Guo wrote: > > > > Hi Uwe, > > > > > > > > On Thu, Jan 13, 2011 at 03:48:05PM +0100, Uwe Kleine-König wrote: > > > > > > > > [...] > > > > > > > > > > +/* Controller is ENET-MAC */ > > > > > > +#define FEC_QUIRK_ENET_MAC (1 << 0) > > > > > does this really qualify to be a quirk? > > > > > > > > > My understanding is that ENET-MAC is a type of "quirky" FEC > > > > controller. > > > > > > > > > > +/* Controller needs driver to swap frame */ > > > > > > +#define FEC_QUIRK_SWAP_FRAME (1 << 1) > > > > > IMHO this is a bit misnamed. FEC_QUIRK_NEEDS_BE_DATA or similar would > > > > > be more accurate. > > > > > > > > > When your make this change, you may want to pick a better name for > > > > function swap_buffer too. > > > > > > > > [...] > > > > > > > > > > +static void *swap_buffer(void *bufaddr, int len) > > > > > > +{ > > > > > > + int i; > > > > > > + unsigned int *buf = bufaddr; > > > > > > + > > > > > > + for (i = 0; i < (len + 3) / 4; i++, buf++) > > > > > > + *buf = cpu_to_be32(*buf); > > > > > if len isn't a multiple of 4 this accesses bytes behind len. Is this > > > > > generally OK here? (E.g. because skbs always have a length that is a > > > > > multiple of 4?) > > > > The len may not be a multiple of 4. But I believe bufaddr is always > > > > a buffer allocated in a length that is a multiple of 4, and the 1~3 > > > > bytes exceeding the len very likely has no data that matters. But > > > > yes, it deserves a safer implementation. > > > Did you test what happens if bufaddr isn't aligned? Does it work at all > > > then? > > > > > I see many calls passing a len that is not a multiple of 4, but it > > works good. > > > That does not prove anything, actually. > > Anyway "bufaddr isn't aligned" != "len is not a multiple of 4". > Is there any guarantee that the function cannot be called with a > non-aligned buffer address? Over the weekend I wondered if we could reach alignment via a dma-mask setting. Didn't check yet how this is configured. Best regards Uwe
Hello, On Thu, Jan 13, 2011 at 03:48:05PM +0100, Uwe Kleine-König wrote: > > #if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \ > > - defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARCH_MXC) > > + defined(CONFIG_M520x) || defined(CONFIG_M532x) || \ > > + defined(CONFIG_ARCH_MXC) || defined(CONFIG_SOC_IMX28) > I wonder what is excluded here. FEC depends on > > M523x || M527x || M5272 || M528x || M520x || M532x || \ > MACH_MX27 || ARCH_MX35 || ARCH_MX25 || ARCH_MX5 || SOC_IMX28 > > so the only difference is that the latter lists M5272 which seems a bit > redundant in the presence of M527x. M527x = {M5271, M5275}, so it seems to me that only M5272 is excluded here. I don't know if it's possible to have a kernel supporting M5272 and (e.g.) M527x. If yes, does the driver work correct on M5272 then? Greg, it seems to me that M5272 is the exception here, not all the others. Would it make sense to make the above read: #if !defined(CONFIG_M5272) ? Best regards Uwe
Hi Lothar, On Mon, Jan 17, 2011 at 09:16:22AM +0100, Lothar Waßmann wrote: > Hi, > > Shawn Guo writes: > > On Fri, Jan 14, 2011 at 08:52:23AM +0100, Uwe Kleine-König wrote: > > > On Fri, Jan 14, 2011 at 01:48:40PM +0800, Shawn Guo wrote: > > > > Hi Uwe, > > > > > > > > On Thu, Jan 13, 2011 at 03:48:05PM +0100, Uwe Kleine-König wrote: > > > > > > > > [...] > > > > > > > > > > +/* Controller is ENET-MAC */ > > > > > > +#define FEC_QUIRK_ENET_MAC (1 << 0) > > > > > does this really qualify to be a quirk? > > > > > > > > > My understanding is that ENET-MAC is a type of "quirky" FEC > > > > controller. > > > > > > > > > > +/* Controller needs driver to swap frame */ > > > > > > +#define FEC_QUIRK_SWAP_FRAME (1 << 1) > > > > > IMHO this is a bit misnamed. FEC_QUIRK_NEEDS_BE_DATA or similar would > > > > > be more accurate. > > > > > > > > > When your make this change, you may want to pick a better name for > > > > function swap_buffer too. > > > > > > > > [...] > > > > > > > > > > +static void *swap_buffer(void *bufaddr, int len) > > > > > > +{ > > > > > > + int i; > > > > > > + unsigned int *buf = bufaddr; > > > > > > + > > > > > > + for (i = 0; i < (len + 3) / 4; i++, buf++) > > > > > > + *buf = cpu_to_be32(*buf); > > > > > if len isn't a multiple of 4 this accesses bytes behind len. Is this > > > > > generally OK here? (E.g. because skbs always have a length that is a > > > > > multiple of 4?) > > > > The len may not be a multiple of 4. But I believe bufaddr is always > > > > a buffer allocated in a length that is a multiple of 4, and the 1~3 > > > > bytes exceeding the len very likely has no data that matters. But > > > > yes, it deserves a safer implementation. > > > Did you test what happens if bufaddr isn't aligned? Does it work at all > > > then? > > > > > I see many calls passing a len that is not a multiple of 4, but it > > works good. > > > That does not prove anything, actually. > > Anyway "bufaddr isn't aligned" != "len is not a multiple of 4". > Is there any guarantee that the function cannot be called with a > non-aligned buffer address? > Oops, I misunderstood the comment. With bounce buffer alignment handling removed, the driver stops working. But at least, mx28 fec driver can work with FEC_ALIGNMENT 0x3 and not necessarily with 0xf. I hope this is what you intended to know.
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig index 4f1755b..f34629b 100644 --- a/drivers/net/Kconfig +++ b/drivers/net/Kconfig @@ -1944,18 +1944,19 @@ config 68360_ENET config FEC bool "FEC ethernet controller (of ColdFire and some i.MX CPUs)" depends on M523x || M527x || M5272 || M528x || M520x || M532x || \ - MACH_MX27 || ARCH_MX35 || ARCH_MX25 || ARCH_MX5 + MACH_MX27 || ARCH_MX35 || ARCH_MX25 || ARCH_MX5 || SOC_IMX28 select PHYLIB help Say Y here if you want to use the built-in 10/100 Fast ethernet controller on some Motorola ColdFire and Freescale i.MX processors. config FEC2 - bool "Second FEC ethernet controller (on some ColdFire CPUs)" + bool "Second FEC ethernet controller" depends on FEC help Say Y here if you want to use the second built-in 10/100 Fast - ethernet controller on some Motorola ColdFire processors. + ethernet controller on some Motorola ColdFire and Freescale + i.MX processors. config FEC_MPC52xx tristate "MPC52xx FEC driver" diff --git a/drivers/net/fec.c b/drivers/net/fec.c index 8a1c51f..2a71373 100644 --- a/drivers/net/fec.c +++ b/drivers/net/fec.c @@ -17,6 +17,8 @@ * * Bug fixes and cleanup by Philippe De Muyter (phdm@macqel.be) * Copyright (c) 2004-2006 Macq Electronique SA. + * + * Copyright (C) 2010 Freescale Semiconductor, Inc. */ #include <linux/module.h> @@ -45,20 +47,36 @@ #include <asm/cacheflush.h> -#ifndef CONFIG_ARCH_MXC +#ifndef CONFIG_ARM #include <asm/coldfire.h> #include <asm/mcfsim.h> #endif #include "fec.h" -#ifdef CONFIG_ARCH_MXC -#include <mach/hardware.h> +#if defined(CONFIG_ARCH_MXC) || defined(CONFIG_SOC_IMX28) #define FEC_ALIGNMENT 0xf #else #define FEC_ALIGNMENT 0x3 #endif +#define DRIVER_NAME "fec" + +/* Controller is ENET-MAC */ +#define FEC_QUIRK_ENET_MAC (1 << 0) +/* Controller needs driver to swap frame */ +#define FEC_QUIRK_SWAP_FRAME (1 << 1) + +static struct platform_device_id fec_devtype[] = { + { + .name = DRIVER_NAME, + .driver_data = 0, + }, { + .name = "imx28-fec", + .driver_data = FEC_QUIRK_ENET_MAC | FEC_QUIRK_SWAP_FRAME, + } +}; + static unsigned char macaddr[ETH_ALEN]; module_param_array(macaddr, byte, NULL, 0); MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); @@ -129,7 +147,8 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address"); * account when setting it. */ #if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \ - defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARCH_MXC) + defined(CONFIG_M520x) || defined(CONFIG_M532x) || \ + defined(CONFIG_ARCH_MXC) || defined(CONFIG_SOC_IMX28) #define OPT_FRAME_SIZE (PKT_MAXBUF_SIZE << 16) #else #define OPT_FRAME_SIZE 0 @@ -208,10 +227,23 @@ static void fec_stop(struct net_device *dev); /* Transmitter timeout */ #define TX_TIMEOUT (2 * HZ) +static void *swap_buffer(void *bufaddr, int len) +{ + int i; + unsigned int *buf = bufaddr; + + for (i = 0; i < (len + 3) / 4; i++, buf++) + *buf = cpu_to_be32(*buf); + + return bufaddr; +} + static netdev_tx_t fec_enet_start_xmit(struct sk_buff *skb, struct net_device *dev) { struct fec_enet_private *fep = netdev_priv(dev); + const struct platform_device_id *id_entry = + platform_get_device_id(fep->pdev); struct bufdesc *bdp; void *bufaddr; unsigned short status; @@ -256,6 +288,14 @@ fec_enet_start_xmit(struct sk_buff *skb, struct net_device *dev) bufaddr = fep->tx_bounce[index]; } + /* + * Some design made an incorrect assumption on endian mode of + * the system that it's running on. As the result, driver has to + * swap every frame going to and coming from the controller. + */ + if (id_entry->driver_data & FEC_QUIRK_SWAP_FRAME) + swap_buffer(bufaddr, skb->len); + /* Save skb pointer */ fep->tx_skbuff[fep->skb_cur] = skb; @@ -424,6 +464,8 @@ static void fec_enet_rx(struct net_device *dev) { struct fec_enet_private *fep = netdev_priv(dev); + const struct platform_device_id *id_entry = + platform_get_device_id(fep->pdev); struct bufdesc *bdp; unsigned short status; struct sk_buff *skb; @@ -487,6 +529,9 @@ fec_enet_rx(struct net_device *dev) dma_unmap_single(NULL, bdp->cbd_bufaddr, bdp->cbd_datlen, DMA_FROM_DEVICE); + if (id_entry->driver_data & FEC_QUIRK_SWAP_FRAME) + swap_buffer(data, pkt_len); + /* This does 16 byte alignment, exactly what we need. * The packet length includes FCS, but we don't want to * include that when passing upstream as it messes up @@ -689,6 +734,7 @@ static int fec_enet_mii_probe(struct net_device *dev) char mdio_bus_id[MII_BUS_ID_SIZE]; char phy_name[MII_BUS_ID_SIZE + 3]; int phy_id; + int dev_id = fep->pdev->id; fep->phy_dev = NULL; @@ -700,6 +746,8 @@ static int fec_enet_mii_probe(struct net_device *dev) continue; if (fep->mii_bus->phy_map[phy_id]->phy_id == 0) continue; + if (dev_id--) + continue; strncpy(mdio_bus_id, fep->mii_bus->id, MII_BUS_ID_SIZE); break; } @@ -737,10 +785,35 @@ static int fec_enet_mii_probe(struct net_device *dev) static int fec_enet_mii_init(struct platform_device *pdev) { + static struct mii_bus *fec0_mii_bus; struct net_device *dev = platform_get_drvdata(pdev); struct fec_enet_private *fep = netdev_priv(dev); + const struct platform_device_id *id_entry = + platform_get_device_id(fep->pdev); int err = -ENXIO, i; + /* + * The dual fec interfaces are not equivalent with enet-mac. + * Here are the differences: + * + * - fec0 supports MII & RMII modes while fec1 only supports RMII + * - fec0 acts as the 1588 time master while fec1 is slave + * - external phys can only be configured by fec0 + * + * That is to say fec1 can not work independently. It only works + * when fec0 is working. The reason behind this design is that the + * second interface is added primarily for Switch mode. + * + * Because of the last point above, both phys are attached on fec0 + * mdio interface in board design, and need to be configured by + * fec0 mii_bus. + */ + if ((id_entry->driver_data & FEC_QUIRK_ENET_MAC) && pdev->id) { + /* fec1 uses fec0 mii_bus */ + fep->mii_bus = fec0_mii_bus; + return 0; + } + fep->mii_timeout = 0; /* @@ -777,6 +850,10 @@ static int fec_enet_mii_init(struct platform_device *pdev) if (mdiobus_register(fep->mii_bus)) goto err_out_free_mdio_irq; + /* save fec0 mii_bus */ + if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) + fec0_mii_bus = fep->mii_bus; + return 0; err_out_free_mdio_irq: @@ -1148,12 +1225,25 @@ static void fec_restart(struct net_device *dev, int duplex) { struct fec_enet_private *fep = netdev_priv(dev); + const struct platform_device_id *id_entry = + platform_get_device_id(fep->pdev); int i; + u32 val, temp_mac[2]; /* Whack a reset. We should wait for this. */ writel(1, fep->hwp + FEC_ECNTRL); udelay(10); + /* + * enet-mac reset will reset mac address registers too, + * so need to reconfigure it. + */ + if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) { + memcpy(&temp_mac, dev->dev_addr, ETH_ALEN); + writel(cpu_to_be32(temp_mac[0]), fep->hwp + FEC_ADDR_LOW); + writel(cpu_to_be32(temp_mac[1]), fep->hwp + FEC_ADDR_HIGH); + } + /* Clear any outstanding interrupt. */ writel(0xffc00000, fep->hwp + FEC_IEVENT); @@ -1200,20 +1290,45 @@ fec_restart(struct net_device *dev, int duplex) /* Set MII speed */ writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED); -#ifdef FEC_MIIGSK_ENR - if (fep->phy_interface == PHY_INTERFACE_MODE_RMII) { - /* disable the gasket and wait */ - writel(0, fep->hwp + FEC_MIIGSK_ENR); - while (readl(fep->hwp + FEC_MIIGSK_ENR) & 4) - udelay(1); + /* + * The phy interface and speed need to get configured + * differently on enet-mac. + */ + if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) { + val = readl(fep->hwp + FEC_R_CNTRL); - /* configure the gasket: RMII, 50 MHz, no loopback, no echo */ - writel(1, fep->hwp + FEC_MIIGSK_CFGR); + /* MII or RMII */ + if (fep->phy_interface == PHY_INTERFACE_MODE_RMII) + val |= (1 << 8); + else + val &= ~(1 << 8); - /* re-enable the gasket */ - writel(2, fep->hwp + FEC_MIIGSK_ENR); - } + /* 10M or 100M */ + if (fep->phy_dev && fep->phy_dev->speed == SPEED_100) + val &= ~(1 << 9); + else + val |= (1 << 9); + + writel(val, fep->hwp + FEC_R_CNTRL); + } else { +#ifdef FEC_MIIGSK_ENR + if (fep->phy_interface == PHY_INTERFACE_MODE_RMII) { + /* disable the gasket and wait */ + writel(0, fep->hwp + FEC_MIIGSK_ENR); + while (readl(fep->hwp + FEC_MIIGSK_ENR) & 4) + udelay(1); + + /* + * configure the gasket: + * RMII, 50 MHz, no loopback, no echo + */ + writel(1, fep->hwp + FEC_MIIGSK_CFGR); + + /* re-enable the gasket */ + writel(2, fep->hwp + FEC_MIIGSK_ENR); + } #endif + } /* And last, enable the transmit and receive processing */ writel(2, fep->hwp + FEC_ECNTRL); @@ -1410,12 +1525,13 @@ static const struct dev_pm_ops fec_pm_ops = { static struct platform_driver fec_driver = { .driver = { - .name = "fec", + .name = DRIVER_NAME, .owner = THIS_MODULE, #ifdef CONFIG_PM .pm = &fec_pm_ops, #endif }, + .id_table = fec_devtype, .probe = fec_probe, .remove = __devexit_p(fec_drv_remove), }; diff --git a/drivers/net/fec.h b/drivers/net/fec.h index 2c48b25..ace318d 100644 --- a/drivers/net/fec.h +++ b/drivers/net/fec.h @@ -14,7 +14,8 @@ /****************************************************************************/ #if defined(CONFIG_M523x) || defined(CONFIG_M527x) || defined(CONFIG_M528x) || \ - defined(CONFIG_M520x) || defined(CONFIG_M532x) || defined(CONFIG_ARCH_MXC) + defined(CONFIG_M520x) || defined(CONFIG_M532x) || \ + defined(CONFIG_ARCH_MXC) || defined(CONFIG_SOC_IMX28) /* * Just figures, Motorola would have to change the offsets for * registers in the same peripheral device on different models @@ -78,7 +79,7 @@ /* * Define the buffer descriptor structure. */ -#ifdef CONFIG_ARCH_MXC +#if defined(CONFIG_ARCH_MXC) || defined(CONFIG_SOC_IMX28) struct bufdesc { unsigned short cbd_datlen; /* Data length */ unsigned short cbd_sc; /* Control and status info */
This patch is to add mx28 dual fec support. Here are some key notes for mx28 fec controller. - The mx28 fec controller naming ENET-MAC is a different IP from FEC used on other i.mx variants. But they are basically compatible on software interface, so it's possible to share the same driver. - ENET-MAC design on mx28 made an improper assumption that it runs on a big-endian system. As the result, driver has to swap every frame going to and coming from the controller. - The external phys can only be configured by fec0, which means fec1 can not work independently and both phys need to be configured by mii_bus attached on fec0. - ENET-MAC reset will get mac address registers reset too. - ENET-MAC MII/RMII mode and 10M/100M speed are configured differently FEC. - ETHER_EN bit must be set to get ENET-MAC interrupt work. Signed-off-by: Shawn Guo <shawn.guo@freescale.com> --- Changes for v4: - Use #ifndef CONFIG_ARM to include ColdFire header files - Define quirk bits in id_entry.driver_data to handle controller difference, which is more scalable than using device name - Define fec0_mii_bus as a static function in fec_enet_mii_init to fold the mii_bus instance attached on fec0 - Use cpu_to_be32 over __swab32 in function swap_buffer Changes for v3: - Move v2 changes into patch #3 - Use device name to check if it's running on ENET-MAC drivers/net/Kconfig | 7 ++- drivers/net/fec.c | 148 +++++++++++++++++++++++++++++++++++++++++++++------ drivers/net/fec.h | 5 +- 3 files changed, 139 insertions(+), 21 deletions(-)