[net-next-2.6 PATCH 0/3] Support for MPC512x FEC

[net-next-2.6 PATCH 0/3] Support for MPC512x FEC

[Anatolij Gustschin]

These patches attempt to provide support for the Freescale MPC512x
FEC in the fs_enet driver. The first cleanup patch replaces printk
by dev_xxx. The second and third attemt to support MPC5121 FEC
in the FEC driver. The first version of the last two patches
was previously submitted as part of the patch series for updated
MPC5121 support. Now these are separated and also address comments
to the first version. Additionally a cleanup patch is added
to this new series. The patches are based on net-next-2.6.

[1/3] fs_enet: use dev_xxx instead of printk
[2/3] fs_enet: Add support for MPC512x to fs_enet driver
[3/3] fs_enet: Add FEC TX Alignment workaround for MPC5121

The code has been tested on the Freescale/STX "MPC5121ADS" board
(board rev. 4) with a MPC5121e Rev. 2.

Cc: <linuxppc-dev@ozlabs.org>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: John Rigby <jcrigby@gmail.com>
---

[net-next-2.6 PATCH 1/3] fs_enet: use dev_xxx instead of printk

[Anatolij Gustschin]

Signed-off-by: Anatolij Gustschin <agust@denx.de>
Cc: <linuxppc-dev@ozlabs.org>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: John Rigby <jcrigby@gmail.com>
Cc: Piotr Ziecik <kosmo@semihalf.com>
Cc: Wolfgang Denk <wd@denx.de>
---
 drivers/net/fs_enet/fs_enet-main.c |   39 +++++++++++++----------------------
 drivers/net/fs_enet/mac-fcc.c      |    5 ++-
 drivers/net/fs_enet/mac-fec.c      |   12 ++++------
 drivers/net/fs_enet/mac-scc.c      |    9 +++----
 4 files changed, 27 insertions(+), 38 deletions(-)

diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c
index ec2f503..c34a7e0 100644
--- a/drivers/net/fs_enet/fs_enet-main.c
+++ b/drivers/net/fs_enet/fs_enet-main.c
@@ -108,9 +108,7 @@ static int fs_enet_rx_napi(struct napi_struct *napi, int budget)
 		 * the last indicator should be set.
 		 */
 		if ((sc & BD_ENET_RX_LAST) == 0)
-			printk(KERN_WARNING DRV_MODULE_NAME
-			       ": %s rcv is not +last\n",
-			       dev->name);
+			dev_warn(fep->dev, "rcv is not +last\n");
 
 		/*
 		 * Check for errors.
@@ -178,9 +176,8 @@ static int fs_enet_rx_napi(struct napi_struct *napi, int budget)
 				received++;
 				netif_receive_skb(skb);
 			} else {
-				printk(KERN_WARNING DRV_MODULE_NAME
-				       ": %s Memory squeeze, dropping packet.\n",
-				       dev->name);
+				dev_warn(fep->dev,
+					 "Memory squeeze, dropping packet.\n");
 				fep->stats.rx_dropped++;
 				skbn = skb;
 			}
@@ -242,9 +239,7 @@ static int fs_enet_rx_non_napi(struct net_device *dev)
 		 * the last indicator should be set.
 		 */
 		if ((sc & BD_ENET_RX_LAST) == 0)
-			printk(KERN_WARNING DRV_MODULE_NAME
-			       ": %s rcv is not +last\n",
-			       dev->name);
+			dev_warn(fep->dev, "rcv is not +last\n");
 
 		/*
 		 * Check for errors.
@@ -313,9 +308,8 @@ static int fs_enet_rx_non_napi(struct net_device *dev)
 				received++;
 				netif_rx(skb);
 			} else {
-				printk(KERN_WARNING DRV_MODULE_NAME
-				       ": %s Memory squeeze, dropping packet.\n",
-				       dev->name);
+				dev_warn(fep->dev,
+					 "Memory squeeze, dropping packet.\n");
 				fep->stats.rx_dropped++;
 				skbn = skb;
 			}
@@ -388,10 +382,10 @@ static void fs_enet_tx(struct net_device *dev)
 		} else
 			fep->stats.tx_packets++;
 
-		if (sc & BD_ENET_TX_READY)
-			printk(KERN_WARNING DRV_MODULE_NAME
-			       ": %s HEY! Enet xmit interrupt and TX_READY.\n",
-			       dev->name);
+		if (sc & BD_ENET_TX_READY) {
+			dev_warn(fep->dev,
+				 "HEY! Enet xmit interrupt and TX_READY.\n");
+		}
 
 		/*
 		 * Deferred means some collisions occurred during transmit,
@@ -511,9 +505,8 @@ void fs_init_bds(struct net_device *dev)
 	for (i = 0, bdp = fep->rx_bd_base; i < fep->rx_ring; i++, bdp++) {
 		skb = dev_alloc_skb(ENET_RX_FRSIZE);
 		if (skb == NULL) {
-			printk(KERN_WARNING DRV_MODULE_NAME
-			       ": %s Memory squeeze, unable to allocate skb\n",
-			       dev->name);
+			dev_warn(fep->dev,
+				 "Memory squeeze, unable to allocate skb\n");
 			break;
 		}
 		skb_align(skb, ENET_RX_ALIGN);
@@ -610,8 +603,7 @@ static int fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		 * Ooops.  All transmit buffers are full.  Bail out.
 		 * This should not happen, since the tx queue should be stopped.
 		 */
-		printk(KERN_WARNING DRV_MODULE_NAME
-		       ": %s tx queue full!.\n", dev->name);
+		dev_warn(fep->dev, "tx queue full!.\n");
 		return NETDEV_TX_BUSY;
 	}
 
@@ -788,8 +780,7 @@ static int fs_enet_open(struct net_device *dev)
 	r = request_irq(fep->interrupt, fs_enet_interrupt, IRQF_SHARED,
 			"fs_enet-mac", dev);
 	if (r != 0) {
-		printk(KERN_ERR DRV_MODULE_NAME
-		       ": %s Could not allocate FS_ENET IRQ!", dev->name);
+		dev_err(fep->dev, "Could not allocate FS_ENET IRQ!");
 		if (fep->fpi->use_napi)
 			napi_disable(&fep->napi);
 		return -EINVAL;
@@ -1053,7 +1044,7 @@ static int __devinit fs_enet_probe(struct of_device *ofdev,
 	if (ret)
 		goto out_free_bd;
 
-	printk(KERN_INFO "%s: fs_enet: %pM\n", ndev->name, ndev->dev_addr);
+	pr_info("%s: fs_enet: %pM\n", ndev->name, ndev->dev_addr);
 
 	return 0;
 
diff --git a/drivers/net/fs_enet/mac-fcc.c b/drivers/net/fs_enet/mac-fcc.c
index 22e5a84..dd78640 100644
--- a/drivers/net/fs_enet/mac-fcc.c
+++ b/drivers/net/fs_enet/mac-fcc.c
@@ -476,8 +476,9 @@ static void clear_int_events(struct net_device *dev, u32 int_events)
 
 static void ev_error(struct net_device *dev, u32 int_events)
 {
-	printk(KERN_WARNING DRV_MODULE_NAME
-	       ": %s FS_ENET ERROR(s) 0x%x\n", dev->name, int_events);
+	struct fs_enet_private *fep = netdev_priv(dev);
+
+	dev_warn(fep->dev, "FS_ENET ERROR(s) 0x%x\n", int_events);
 }
 
 static int get_regs(struct net_device *dev, void *p, int *sizep)
diff --git a/drivers/net/fs_enet/mac-fec.c b/drivers/net/fs_enet/mac-fec.c
index ca7bcb8..a664aa1 100644
--- a/drivers/net/fs_enet/mac-fec.c
+++ b/drivers/net/fs_enet/mac-fec.c
@@ -257,8 +257,7 @@ static void restart(struct net_device *dev)
 
 	r = whack_reset(fep->fec.fecp);
 	if (r != 0)
-		printk(KERN_ERR DRV_MODULE_NAME
-				": %s FEC Reset FAILED!\n", dev->name);
+		dev_err(fep->dev, "FEC Reset FAILED!\n");
 	/*
 	 * Set station address.
 	 */
@@ -355,9 +354,7 @@ static void stop(struct net_device *dev)
 		udelay(1);
 
 	if (i == FEC_RESET_DELAY)
-		printk(KERN_WARNING DRV_MODULE_NAME
-		       ": %s FEC timeout on graceful transmit stop\n",
-		       dev->name);
+		dev_warn(fep->dev, "FEC timeout on graceful transmit stop\n");
 	/*
 	 * Disable FEC. Let only MII interrupts.
 	 */
@@ -433,8 +430,9 @@ static void clear_int_events(struct net_device *dev, u32 int_events)
 
 static void ev_error(struct net_device *dev, u32 int_events)
 {
-	printk(KERN_WARNING DRV_MODULE_NAME
-	       ": %s FEC ERROR(s) 0x%x\n", dev->name, int_events);
+	struct fs_enet_private *fep = netdev_priv(dev);
+
+	dev_warn(fep->dev, "FEC ERROR(s) 0x%x\n", int_events);
 }
 
 static int get_regs(struct net_device *dev, void *p, int *sizep)
diff --git a/drivers/net/fs_enet/mac-scc.c b/drivers/net/fs_enet/mac-scc.c
index 008cdd9..2d8917f 100644
--- a/drivers/net/fs_enet/mac-scc.c
+++ b/drivers/net/fs_enet/mac-scc.c
@@ -367,9 +367,7 @@ static void stop(struct net_device *dev)
 		udelay(1);
 
 	if (i == SCC_RESET_DELAY)
-		printk(KERN_WARNING DRV_MODULE_NAME
-		       ": %s SCC timeout on graceful transmit stop\n",
-		       dev->name);
+		dev_warn(fep->dev, "SCC timeout on graceful transmit stop\n");
 
 	W16(sccp, scc_sccm, 0);
 	C32(sccp, scc_gsmrl, SCC_GSMRL_ENR | SCC_GSMRL_ENT);
@@ -429,8 +427,9 @@ static void clear_int_events(struct net_device *dev, u32 int_events)
 
 static void ev_error(struct net_device *dev, u32 int_events)
 {
-	printk(KERN_WARNING DRV_MODULE_NAME
-	       ": %s SCC ERROR(s) 0x%x\n", dev->name, int_events);
+	struct fs_enet_private *fep = netdev_priv(dev);
+
+	dev_warn(fep->dev, "SCC ERROR(s) 0x%x\n", int_events);
 }
 
 static int get_regs(struct net_device *dev, void *p, int *sizep)
-- 
1.5.6.3

[net-next-2.6 PATCH 2/3] fs_enet: Add support for MPC512x to fs_enet driver

[Anatolij Gustschin]

    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 <jcrigby@gmail.com>
Signed-off-by: Piotr Ziecik <kosmo@semihalf.com>
Signed-off-by: Wolfgang Denk <wd@denx.de>
Signed-off-by: Anatolij Gustschin <agust@denx.de>
Cc: <linuxppc-dev@ozlabs.org>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
Changes since previous submited version:

- explicit type usage in register tables.
- don't use same variable name "fecp" for variables of
  different types.
- avoid re-checking the compatible by passing data pointer
  in the match struct.

 drivers/net/fs_enet/Kconfig        |   10 +-
 drivers/net/fs_enet/fs_enet-main.c |    4 +
 drivers/net/fs_enet/fs_enet.h      |   40 +++++++-
 drivers/net/fs_enet/mac-fec.c      |  212 +++++++++++++++++++++++++-----------
 drivers/net/fs_enet/mii-fec.c      |   76 ++++++++++---
 drivers/net/fs_enet/mpc5121_fec.h  |   64 +++++++++++
 drivers/net/fs_enet/mpc8xx_fec.h   |   37 ++++++
 7 files changed, 356 insertions(+), 87 deletions(-)
 create mode 100644 drivers/net/fs_enet/mpc5121_fec.h
 create mode 100644 drivers/net/fs_enet/mpc8xx_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 c34a7e0..6bce5c8 100644
--- a/drivers/net/fs_enet/fs_enet-main.c
+++ b/drivers/net/fs_enet/fs_enet-main.c
@@ -1095,6 +1095,10 @@ static struct of_device_id fs_enet_match[] = {
 #endif
 #ifdef CONFIG_FS_ENET_HAS_FEC
 	{
+		.compatible = "fsl,mpc5121-fec",
+		.data = (void *)&fs_fec_ops,
+	},
+	{
 		.compatible = "fsl,pq1-fec-enet",
 		.data = (void *)&fs_fec_ops,
 	},
diff --git a/drivers/net/fs_enet/fs_enet.h b/drivers/net/fs_enet/fs_enet.h
index ef01e09..df935e8 100644
--- a/drivers/net/fs_enet/fs_enet.h
+++ b/drivers/net/fs_enet/fs_enet.h
@@ -13,11 +13,47 @@
 
 #ifdef CONFIG_CPM1
 #include <asm/cpm1.h>
+#endif
+
+#if defined(CONFIG_FS_ENET_HAS_FEC)
+#include <asm/cpm.h>
+#include "mpc8xx_fec.h"
+#include "mpc5121_fec.h"
 
 struct fec_info {
-	fec_t __iomem *fecp;
+	void __iomem *fecp;
+	u32 __iomem *fec_r_cntrl;
+	u32 __iomem *fec_ecntrl;
+	u32 __iomem *fec_ievent;
+	u32 __iomem *fec_mii_data;
+	u32 __iomem *fec_mii_speed;
 	u32 mii_speed;
 };
+
+struct reg_tbl {
+	u32 __iomem *fec_ievent;
+	u32 __iomem *fec_imask;
+	u32 __iomem *fec_r_des_active;
+	u32 __iomem *fec_x_des_active;
+	u32 __iomem *fec_r_des_start;
+	u32 __iomem *fec_x_des_start;
+	u32 __iomem *fec_r_cntrl;
+	u32 __iomem *fec_ecntrl;
+	u32 __iomem *fec_ivec;
+	u32 __iomem *fec_mii_speed;
+	u32 __iomem *fec_addr_low;
+	u32 __iomem *fec_addr_high;
+	u32 __iomem *fec_hash_table_high;
+	u32 __iomem *fec_hash_table_low;
+	u32 __iomem *fec_r_buff_size;
+	u32 __iomem *fec_r_bound;
+	u32 __iomem *fec_r_fstart;
+	u32 __iomem *fec_x_fstart;
+	u32 __iomem *fec_fun_code;
+	u32 __iomem *fec_r_hash;
+	u32 __iomem *fec_x_cntrl;
+	u32 __iomem *fec_dma_control;
+};
 #endif
 
 #ifdef CONFIG_CPM2
@@ -113,7 +149,9 @@ struct fs_enet_private {
 		struct {
 			int idx;		/* FEC1 = 0, FEC2 = 1  */
 			void __iomem *fecp;	/* hw registers        */
+			struct reg_tbl *rtbl;	/* used registers table */
 			u32 hthi, htlo;		/* state for multicast */
+			u32 fec_id;
 		} fec;
 
 		struct {
diff --git a/drivers/net/fs_enet/mac-fec.c b/drivers/net/fs_enet/mac-fec.c
index a664aa1..fe9e368 100644
--- a/drivers/net/fs_enet/mac-fec.c
+++ b/drivers/net/fs_enet/mac-fec.c
@@ -64,29 +64,40 @@
 #endif
 
 /* write */
-#define FW(_fecp, _reg, _v) __fs_out32(&(_fecp)->fec_ ## _reg, (_v))
+#define FW(_regp, _reg, _v) __fs_out32((_regp)->fec_ ## _reg, (_v))
 
 /* read */
-#define FR(_fecp, _reg)	__fs_in32(&(_fecp)->fec_ ## _reg)
+#define FR(_regp, _reg)	__fs_in32((_regp)->fec_ ## _reg)
 
 /* set bits */
-#define FS(_fecp, _reg, _v) FW(_fecp, _reg, FR(_fecp, _reg) | (_v))
+#define FS(_regp, _reg, _v) FW(_regp, _reg, FR(_regp, _reg) | (_v))
 
 /* clear bits */
-#define FC(_fecp, _reg, _v) FW(_fecp, _reg, FR(_fecp, _reg) & ~(_v))
+#define FC(_regp, _reg, _v) FW(_regp, _reg, FR(_regp, _reg) & ~(_v))
+
+/* register address macros */
+#define fec_reg_addr(_type, _reg) \
+	(fep->fec.rtbl->fec_##_reg = (u32 __iomem *)((u32)fep->fec.fecp + \
+				(u32)&((__typeof__(_type) *)NULL)->fec_##_reg))
+
+#define fec_reg_mpc8xx(_reg) \
+	fec_reg_addr(struct mpc8xx_fec, _reg)
+
+#define fec_reg_mpc5121(_reg) \
+	fec_reg_addr(struct mpc5121_fec, _reg)
 
 /*
  * Delay to wait for FEC reset command to complete (in us)
  */
 #define FEC_RESET_DELAY		50
 
-static int whack_reset(fec_t __iomem *fecp)
+static int whack_reset(struct reg_tbl *regp)
 {
 	int i;
 
-	FW(fecp, ecntrl, FEC_ECNTRL_PINMUX | FEC_ECNTRL_RESET);
+	FW(regp, ecntrl, FEC_ECNTRL_PINMUX | FEC_ECNTRL_RESET);
 	for (i = 0; i < FEC_RESET_DELAY; i++) {
-		if ((FR(fecp, ecntrl) & FEC_ECNTRL_RESET) == 0)
+		if ((FR(regp, ecntrl) & FEC_ECNTRL_RESET) == 0)
 			return 0;	/* OK */
 		udelay(1);
 	}
@@ -106,6 +117,50 @@ static int do_pd_setup(struct fs_enet_private *fep)
 	if (!fep->fcc.fccp)
 		return -EINVAL;
 
+	fep->fec.rtbl = kzalloc(sizeof(*fep->fec.rtbl), GFP_KERNEL);
+	if (!fep->fec.rtbl) {
+		iounmap(fep->fec.fecp);
+		return -ENOMEM;
+	}
+
+	if (of_device_is_compatible(ofdev->node, "fsl,mpc5121-fec")) {
+		fep->fec.fec_id = FS_ENET_MPC5121_FEC;
+		fec_reg_mpc5121(ievent);
+		fec_reg_mpc5121(imask);
+		fec_reg_mpc5121(r_cntrl);
+		fec_reg_mpc5121(ecntrl);
+		fec_reg_mpc5121(r_des_active);
+		fec_reg_mpc5121(x_des_active);
+		fec_reg_mpc5121(r_des_start);
+		fec_reg_mpc5121(x_des_start);
+		fec_reg_mpc5121(addr_low);
+		fec_reg_mpc5121(addr_high);
+		fec_reg_mpc5121(hash_table_high);
+		fec_reg_mpc5121(hash_table_low);
+		fec_reg_mpc5121(r_buff_size);
+		fec_reg_mpc5121(mii_speed);
+		fec_reg_mpc5121(x_cntrl);
+		fec_reg_mpc5121(dma_control);
+	} else {
+		fec_reg_mpc8xx(ievent);
+		fec_reg_mpc8xx(imask);
+		fec_reg_mpc8xx(r_cntrl);
+		fec_reg_mpc8xx(ecntrl);
+		fec_reg_mpc8xx(mii_speed);
+		fec_reg_mpc8xx(r_des_active);
+		fec_reg_mpc8xx(x_des_active);
+		fec_reg_mpc8xx(r_des_start);
+		fec_reg_mpc8xx(x_des_start);
+		fec_reg_mpc8xx(ivec);
+		fec_reg_mpc8xx(addr_low);
+		fec_reg_mpc8xx(addr_high);
+		fec_reg_mpc8xx(hash_table_high);
+		fec_reg_mpc8xx(hash_table_low);
+		fec_reg_mpc8xx(r_buff_size);
+		fec_reg_mpc8xx(x_fstart);
+		fec_reg_mpc8xx(r_hash);
+		fec_reg_mpc8xx(x_cntrl);
+	}
 	return 0;
 }
 
@@ -162,15 +217,17 @@ static void free_bd(struct net_device *dev)
 
 static void cleanup_data(struct net_device *dev)
 {
-	/* nothing */
+	struct fs_enet_private *fep = netdev_priv(dev);
+
+	kfree(fep->fec.rtbl);
 }
 
 static void set_promiscuous_mode(struct net_device *dev)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
-	fec_t __iomem *fecp = fep->fec.fecp;
+	struct reg_tbl *regp = fep->fec.rtbl;
 
-	FS(fecp, r_cntrl, FEC_RCNTRL_PROM);
+	FS(regp, r_cntrl, FEC_RCNTRL_PROM);
 }
 
 static void set_multicast_start(struct net_device *dev)
@@ -216,7 +273,7 @@ static void set_multicast_one(struct net_device *dev, const u8 *mac)
 static void set_multicast_finish(struct net_device *dev)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
-	fec_t __iomem *fecp = fep->fec.fecp;
+	struct reg_tbl *regp = fep->fec.rtbl;
 
 	/* if all multi or too many multicasts; just enable all */
 	if ((dev->flags & IFF_ALLMULTI) != 0 ||
@@ -225,9 +282,9 @@ static void set_multicast_finish(struct net_device *dev)
 		fep->fec.htlo = 0xffffffffU;
 	}
 
-	FC(fecp, r_cntrl, FEC_RCNTRL_PROM);
-	FW(fecp, hash_table_high, fep->fec.hthi);
-	FW(fecp, hash_table_low, fep->fec.htlo);
+	FC(regp, r_cntrl, FEC_RCNTRL_PROM);
+	FW(regp, hash_table_high, fep->fec.hthi);
+	FW(regp, hash_table_low, fep->fec.htlo);
 }
 
 static void set_multicast_list(struct net_device *dev)
@@ -246,7 +303,7 @@ static void set_multicast_list(struct net_device *dev)
 static void restart(struct net_device *dev)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
-	fec_t __iomem *fecp = fep->fec.fecp;
+	struct reg_tbl *regp = fep->fec.rtbl;
 	const struct fs_platform_info *fpi = fep->fpi;
 	dma_addr_t rx_bd_base_phys, tx_bd_base_phys;
 	int r;
@@ -255,7 +312,7 @@ static void restart(struct net_device *dev)
 	struct mii_bus* mii = fep->phydev->bus;
 	struct fec_info* fec_inf = mii->priv;
 
-	r = whack_reset(fep->fec.fecp);
+	r = whack_reset(regp);
 	if (r != 0)
 		dev_err(fep->dev, "FEC Reset FAILED!\n");
 	/*
@@ -267,20 +324,23 @@ static void restart(struct net_device *dev)
 		  (u32) dev->dev_addr[3];
 	addrlo = ((u32) dev->dev_addr[4] << 24) |
 		 ((u32) dev->dev_addr[5] << 16);
-	FW(fecp, addr_low, addrhi);
-	FW(fecp, addr_high, addrlo);
+	FW(regp, addr_low, addrhi);
+	FW(regp, addr_high, addrlo);
 
 	/*
 	 * Reset all multicast.
 	 */
-	FW(fecp, hash_table_high, fep->fec.hthi);
-	FW(fecp, hash_table_low, fep->fec.htlo);
+	FW(regp, hash_table_high, fep->fec.hthi);
+	FW(regp, hash_table_low, fep->fec.htlo);
 
 	/*
 	 * Set maximum receive buffer size.
 	 */
-	FW(fecp, r_buff_size, PKT_MAXBLR_SIZE);
-	FW(fecp, r_hash, PKT_MAXBUF_SIZE);
+	FW(regp, r_buff_size, PKT_MAXBLR_SIZE);
+	if (fep->fec.fec_id == FS_ENET_MPC5121_FEC)
+		FW(regp, r_cntrl, PKT_MAXBUF_SIZE << 16);
+	else
+		FW(regp, r_hash, PKT_MAXBUF_SIZE);
 
 	/* get physical address */
 	rx_bd_base_phys = fep->ring_mem_addr;
@@ -289,67 +349,82 @@ static void restart(struct net_device *dev)
 	/*
 	 * Set receive and transmit descriptor base.
 	 */
-	FW(fecp, r_des_start, rx_bd_base_phys);
-	FW(fecp, x_des_start, tx_bd_base_phys);
+	FW(regp, r_des_start, rx_bd_base_phys);
+	FW(regp, x_des_start, tx_bd_base_phys);
 
 	fs_init_bds(dev);
 
 	/*
-	 * Enable big endian and don't care about SDMA FC.
+	 * Enable big endian.
 	 */
-	FW(fecp, fun_code, 0x78000000);
+	if (fep->fec.fec_id == FS_ENET_MPC5121_FEC)
+		FS(regp, dma_control, 0xC0000000);
+	else
+		/* Don't care about SDMA Function Code. */
+		FW(regp, fun_code, 0x78000000);
 
 	/*
 	 * Set MII speed.
 	 */
-	FW(fecp, mii_speed, fec_inf->mii_speed);
+	FW(regp, mii_speed, fec_inf->mii_speed);
 
 	/*
 	 * Clear any outstanding interrupt.
 	 */
-	FW(fecp, ievent, 0xffc0);
-	FW(fecp, ivec, (virq_to_hw(fep->interrupt) / 2) << 29);
+	FW(regp, ievent, 0xffc0);
+	if (fep->fec.fec_id != FS_ENET_MPC5121_FEC)
+		FW(regp, ivec, (virq_to_hw(fep->interrupt) / 2) << 29);
+
+	/* MII enable */
+	if (fep->fec.fec_id == FS_ENET_MPC5121_FEC) {
+		/*
+		 * Only set requested bit - do not touch maximum packet
+		 * size configured earlier.
+		 */
+		FS(regp, r_cntrl, FEC_RCNTRL_MII_MODE);
+	} else {
+		FW(regp, r_cntrl, FEC_RCNTRL_MII_MODE);
+	}
 
-	FW(fecp, r_cntrl, FEC_RCNTRL_MII_MODE);	/* MII enable */
 	/*
 	 * adjust to duplex mode
 	 */
 	if (fep->phydev->duplex) {
-		FC(fecp, r_cntrl, FEC_RCNTRL_DRT);
-		FS(fecp, x_cntrl, FEC_TCNTRL_FDEN);	/* FD enable */
+		FC(regp, r_cntrl, FEC_RCNTRL_DRT);
+		FS(regp, x_cntrl, FEC_TCNTRL_FDEN);	/* FD enable */
 	} else {
-		FS(fecp, r_cntrl, FEC_RCNTRL_DRT);
-		FC(fecp, x_cntrl, FEC_TCNTRL_FDEN);	/* FD disable */
+		FS(regp, r_cntrl, FEC_RCNTRL_DRT);
+		FC(regp, x_cntrl, FEC_TCNTRL_FDEN);	/* FD disable */
 	}
 
 	/*
 	 * Enable interrupts we wish to service.
 	 */
-	FW(fecp, imask, FEC_ENET_TXF | FEC_ENET_TXB |
+	FW(regp, imask, FEC_ENET_TXF | FEC_ENET_TXB |
 	   FEC_ENET_RXF | FEC_ENET_RXB);
 
 	/*
 	 * And last, enable the transmit and receive processing.
 	 */
-	FW(fecp, ecntrl, FEC_ECNTRL_PINMUX | FEC_ECNTRL_ETHER_EN);
-	FW(fecp, r_des_active, 0x01000000);
+	FW(regp, ecntrl, FEC_ECNTRL_PINMUX | FEC_ECNTRL_ETHER_EN);
+	FW(regp, r_des_active, 0x01000000);
 }
 
 static void stop(struct net_device *dev)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
 	const struct fs_platform_info *fpi = fep->fpi;
-	fec_t __iomem *fecp = fep->fec.fecp;
+	struct reg_tbl *regp = fep->fec.rtbl;
 
 	struct fec_info* feci= fep->phydev->bus->priv;
 
 	int i;
 
-	if ((FR(fecp, ecntrl) & FEC_ECNTRL_ETHER_EN) == 0)
+	if ((FR(regp, ecntrl) & FEC_ECNTRL_ETHER_EN) == 0)
 		return;		/* already down */
 
-	FW(fecp, x_cntrl, 0x01);	/* Graceful transmit stop */
-	for (i = 0; ((FR(fecp, ievent) & 0x10000000) == 0) &&
+	FW(regp, x_cntrl, 0x01);	/* Graceful transmit stop */
+	for (i = 0; ((FR(regp, ievent) & 0x10000000) == 0) &&
 	     i < FEC_RESET_DELAY; i++)
 		udelay(1);
 
@@ -358,74 +433,74 @@ static void stop(struct net_device *dev)
 	/*
 	 * Disable FEC. Let only MII interrupts.
 	 */
-	FW(fecp, imask, 0);
-	FC(fecp, ecntrl, FEC_ECNTRL_ETHER_EN);
+	FW(regp, imask, 0);
+	FC(regp, ecntrl, FEC_ECNTRL_ETHER_EN);
 
 	fs_cleanup_bds(dev);
 
 	/* shut down FEC1? that's where the mii bus is */
 	if (fpi->has_phy) {
-		FS(fecp, r_cntrl, FEC_RCNTRL_MII_MODE);	/* MII enable */
-		FS(fecp, ecntrl, FEC_ECNTRL_PINMUX | FEC_ECNTRL_ETHER_EN);
-		FW(fecp, ievent, FEC_ENET_MII);
-		FW(fecp, mii_speed, feci->mii_speed);
+		FS(regp, r_cntrl, FEC_RCNTRL_MII_MODE);	/* MII enable */
+		FS(regp, ecntrl, FEC_ECNTRL_PINMUX | FEC_ECNTRL_ETHER_EN);
+		FW(regp, ievent, FEC_ENET_MII);
+		FW(regp, mii_speed, feci->mii_speed);
 	}
 }
 
 static void napi_clear_rx_event(struct net_device *dev)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
-	fec_t __iomem *fecp = fep->fec.fecp;
+	struct reg_tbl *regp = fep->fec.rtbl;
 
-	FW(fecp, ievent, FEC_NAPI_RX_EVENT_MSK);
+	FW(regp, ievent, FEC_NAPI_RX_EVENT_MSK);
 }
 
 static void napi_enable_rx(struct net_device *dev)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
-	fec_t __iomem *fecp = fep->fec.fecp;
+	struct reg_tbl *regp = fep->fec.rtbl;
 
-	FS(fecp, imask, FEC_NAPI_RX_EVENT_MSK);
+	FS(regp, imask, FEC_NAPI_RX_EVENT_MSK);
 }
 
 static void napi_disable_rx(struct net_device *dev)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
-	fec_t __iomem *fecp = fep->fec.fecp;
+	struct reg_tbl *regp = fep->fec.rtbl;
 
-	FC(fecp, imask, FEC_NAPI_RX_EVENT_MSK);
+	FC(regp, imask, FEC_NAPI_RX_EVENT_MSK);
 }
 
 static void rx_bd_done(struct net_device *dev)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
-	fec_t __iomem *fecp = fep->fec.fecp;
+	struct reg_tbl *regp = fep->fec.rtbl;
 
-	FW(fecp, r_des_active, 0x01000000);
+	FW(regp, r_des_active, 0x01000000);
 }
 
 static void tx_kickstart(struct net_device *dev)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
-	fec_t __iomem *fecp = fep->fec.fecp;
+	struct reg_tbl *regp = fep->fec.rtbl;
 
-	FW(fecp, x_des_active, 0x01000000);
+	FW(regp, x_des_active, 0x01000000);
 }
 
 static u32 get_int_events(struct net_device *dev)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
-	fec_t __iomem *fecp = fep->fec.fecp;
+	struct reg_tbl *regp = fep->fec.rtbl;
 
-	return FR(fecp, ievent) & FR(fecp, imask);
+	return FR(regp, ievent) & FR(regp, imask);
 }
 
 static void clear_int_events(struct net_device *dev, u32 int_events)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
-	fec_t __iomem *fecp = fep->fec.fecp;
+	struct reg_tbl *regp = fep->fec.rtbl;
 
-	FW(fecp, ievent, int_events);
+	FW(regp, ievent, int_events);
 }
 
 static void ev_error(struct net_device *dev, u32 int_events)
@@ -438,18 +513,26 @@ static void ev_error(struct net_device *dev, u32 int_events)
 static int get_regs(struct net_device *dev, void *p, int *sizep)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
+	int size;
 
-	if (*sizep < sizeof(fec_t))
+	size = fep->fec.fec_id ? sizeof(struct mpc5121_fec) :
+				 sizeof(struct mpc8xx_fec);
+	if (*sizep < size)
 		return -EINVAL;
 
-	memcpy_fromio(p, fep->fec.fecp, sizeof(fec_t));
+	memcpy_fromio(p, fep->fec.fecp, size);
 
 	return 0;
 }
 
 static int get_regs_len(struct net_device *dev)
 {
-	return sizeof(fec_t);
+	struct fs_enet_private *fep = netdev_priv(dev);
+
+	if (fep->fec.fec_id == FS_ENET_MPC5121_FEC)
+		return sizeof(struct mpc5121_fec);
+	else
+		return sizeof(struct mpc8xx_fec);
 }
 
 static void tx_restart(struct net_device *dev)
@@ -479,4 +562,3 @@ const struct fs_ops fs_fec_ops = {
 	.allocate_bd		= allocate_bd,
 	.free_bd		= free_bd,
 };
-
diff --git a/drivers/net/fs_enet/mii-fec.c b/drivers/net/fs_enet/mii-fec.c
index 96eba42..eac18ab 100644
--- a/drivers/net/fs_enet/mii-fec.c
+++ b/drivers/net/fs_enet/mii-fec.c
@@ -49,24 +49,38 @@
 
 #define FEC_MII_LOOPS	10000
 
+#define fec_reg_addr(_type, _reg) \
+	(fec->fec_##_reg = (u32 __iomem *)((u32)fec->fecp + \
+				(u32)&((__typeof__(_type) *)NULL)->fec_##_reg))
+
+#define fec_reg_mpc8xx(_reg) \
+	fec_reg_addr(struct mpc8xx_fec, _reg)
+
+#define fec_reg_mpc5121(_reg) \
+	fec_reg_addr(struct mpc5121_fec, _reg)
+
+struct fs_enet_mdio_fec_match_data {
+	unsigned long (*get_bus_freq)(struct device_node *);
+	unsigned int fec_id;
+};
+
 static int fs_enet_fec_mii_read(struct mii_bus *bus , int phy_id, int location)
 {
 	struct fec_info* fec = bus->priv;
-	fec_t __iomem *fecp = fec->fecp;
 	int i, ret = -1;
 
-	BUG_ON((in_be32(&fecp->fec_r_cntrl) & FEC_RCNTRL_MII_MODE) == 0);
+	BUG_ON((in_be32(fec->fec_r_cntrl) & FEC_RCNTRL_MII_MODE) == 0);
 
 	/* Add PHY address to register command.  */
-	out_be32(&fecp->fec_mii_data, (phy_id << 23) | mk_mii_read(location));
+	out_be32(fec->fec_mii_data, (phy_id << 23) | mk_mii_read(location));
 
 	for (i = 0; i < FEC_MII_LOOPS; i++)
-		if ((in_be32(&fecp->fec_ievent) & FEC_ENET_MII) != 0)
+		if ((in_be32(fec->fec_ievent) & FEC_ENET_MII) != 0)
 			break;
 
 	if (i < FEC_MII_LOOPS) {
-		out_be32(&fecp->fec_ievent, FEC_ENET_MII);
-		ret = in_be32(&fecp->fec_mii_data) & 0xffff;
+		out_be32(fec->fec_ievent, FEC_ENET_MII);
+		ret = in_be32(fec->fec_mii_data) & 0xffff;
 	}
 
 	return ret;
@@ -75,21 +89,21 @@ static int fs_enet_fec_mii_read(struct mii_bus *bus , int phy_id, int location)
 static int fs_enet_fec_mii_write(struct mii_bus *bus, int phy_id, int location, u16 val)
 {
 	struct fec_info* fec = bus->priv;
-	fec_t __iomem *fecp = fec->fecp;
 	int i;
 
 	/* this must never happen */
-	BUG_ON((in_be32(&fecp->fec_r_cntrl) & FEC_RCNTRL_MII_MODE) == 0);
+	BUG_ON((in_be32(fec->fec_r_cntrl) & FEC_RCNTRL_MII_MODE) == 0);
 
 	/* Add PHY address to register command.  */
-	out_be32(&fecp->fec_mii_data, (phy_id << 23) | mk_mii_write(location, val));
+	out_be32(fec->fec_mii_data, (phy_id << 23) |
+				    mk_mii_write(location, val));
 
 	for (i = 0; i < FEC_MII_LOOPS; i++)
-		if ((in_be32(&fecp->fec_ievent) & FEC_ENET_MII) != 0)
+		if ((in_be32(fec->fec_ievent) & FEC_ENET_MII) != 0)
 			break;
 
 	if (i < FEC_MII_LOOPS)
-		out_be32(&fecp->fec_ievent, FEC_ENET_MII);
+		out_be32(fec->fec_ievent, FEC_ENET_MII);
 
 	return 0;
 
@@ -107,7 +121,8 @@ static int __devinit fs_enet_mdio_probe(struct of_device *ofdev,
 	struct resource res;
 	struct mii_bus *new_bus;
 	struct fec_info *fec;
-	int (*get_bus_freq)(struct device_node *) = match->data;
+	unsigned long (*get_bus_freq)(struct device_node *) = NULL;
+	struct fs_enet_mdio_fec_match_data *md;
 	int ret = -ENOMEM, clock, speed;
 
 	new_bus = mdiobus_alloc();
@@ -134,6 +149,24 @@ static int __devinit fs_enet_mdio_probe(struct of_device *ofdev,
 	if (!fec->fecp)
 		goto out_fec;
 
+	md = match->data;
+	if (md)
+		get_bus_freq = md->get_bus_freq;
+
+	if (md && md->fec_id == FS_ENET_MPC5121_FEC) {
+		fec_reg_mpc5121(ecntrl);
+		fec_reg_mpc5121(ievent);
+		fec_reg_mpc5121(mii_data);
+		fec_reg_mpc5121(mii_speed);
+		fec_reg_mpc5121(r_cntrl);
+	} else {
+		fec_reg_mpc8xx(ecntrl);
+		fec_reg_mpc8xx(ievent);
+		fec_reg_mpc8xx(mii_data);
+		fec_reg_mpc8xx(mii_speed);
+		fec_reg_mpc8xx(r_cntrl);
+	}
+
 	if (get_bus_freq) {
 		clock = get_bus_freq(ofdev->node);
 		if (!clock) {
@@ -158,11 +191,11 @@ static int __devinit fs_enet_mdio_probe(struct of_device *ofdev,
 
 	fec->mii_speed = speed << 1;
 
-	setbits32(&fec->fecp->fec_r_cntrl, FEC_RCNTRL_MII_MODE);
-	setbits32(&fec->fecp->fec_ecntrl, FEC_ECNTRL_PINMUX |
-	                                  FEC_ECNTRL_ETHER_EN);
-	out_be32(&fec->fecp->fec_ievent, FEC_ENET_MII);
-	clrsetbits_be32(&fec->fecp->fec_mii_speed, 0x7E, fec->mii_speed);
+	setbits32(fec->fec_r_cntrl, FEC_RCNTRL_MII_MODE);
+	setbits32(fec->fec_ecntrl, FEC_ECNTRL_PINMUX |
+				   FEC_ECNTRL_ETHER_EN);
+	out_be32(fec->fec_ievent, FEC_ENET_MII);
+	clrsetbits_be32(fec->fec_mii_speed, 0x7E, fec->mii_speed);
 
 	new_bus->phy_mask = ~0;
 	new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL);
@@ -207,6 +240,13 @@ static int fs_enet_mdio_remove(struct of_device *ofdev)
 	return 0;
 }
 
+#if defined(CONFIG_PPC_MPC512x)
+static struct fs_enet_mdio_fec_match_data mpc5121_match_data = {
+	.get_bus_freq = mpc5xxx_get_bus_frequency,
+	.fec_id = FS_ENET_MPC5121_FEC,
+};
+#endif
+
 static struct of_device_id fs_enet_mdio_fec_match[] = {
 	{
 		.compatible = "fsl,pq1-fec-mdio",
@@ -214,7 +254,7 @@ static struct of_device_id fs_enet_mdio_fec_match[] = {
 #if defined(CONFIG_PPC_MPC512x)
 	{
 		.compatible = "fsl,mpc5121-fec-mdio",
-		.data = mpc5xxx_get_bus_frequency,
+		.data = (void *)&mpc5121_match_data,
 	},
 #endif
 	{},
diff --git a/drivers/net/fs_enet/mpc5121_fec.h b/drivers/net/fs_enet/mpc5121_fec.h
new file mode 100644
index 0000000..18d4fb3
--- /dev/null
+++ b/drivers/net/fs_enet/mpc5121_fec.h
@@ -0,0 +1,64 @@
+/*
+ * 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
+
+struct mpc5121_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 */
+};
+
+#define FS_ENET_MPC5121_FEC	0x1
+
+#endif /* MPC5121_FEC_H */
diff --git a/drivers/net/fs_enet/mpc8xx_fec.h b/drivers/net/fs_enet/mpc8xx_fec.h
new file mode 100644
index 0000000..aa78445
--- /dev/null
+++ b/drivers/net/fs_enet/mpc8xx_fec.h
@@ -0,0 +1,37 @@
+/* MPC860T Fast Ethernet Controller.  It isn't part of the CPM, but
+ * it fits within the address space.
+ */
+
+struct mpc8xx_fec {
+	uint	fec_addr_low;		/* lower 32 bits of station address */
+	ushort	fec_addr_high;		/* upper 16 bits of station address */
+	ushort	res1;			/* reserved			    */
+	uint	fec_hash_table_high;	/* upper 32-bits of hash table	    */
+	uint	fec_hash_table_low;	/* lower 32-bits of hash table	    */
+	uint	fec_r_des_start;	/* beginning of Rx descriptor ring  */
+	uint	fec_x_des_start;	/* beginning of Tx descriptor ring  */
+	uint	fec_r_buff_size;	/* Rx buffer size		    */
+	uint	res2[9];		/* reserved			    */
+	uint	fec_ecntrl;		/* ethernet control register	    */
+	uint	fec_ievent;		/* interrupt event register	    */
+	uint	fec_imask;		/* interrupt mask register	    */
+	uint	fec_ivec;		/* interrupt level and vector status */
+	uint	fec_r_des_active;	/* Rx ring updated flag		    */
+	uint	fec_x_des_active;	/* Tx ring updated flag		    */
+	uint	res3[10];		/* reserved			    */
+	uint	fec_mii_data;		/* MII data register		    */
+	uint	fec_mii_speed;		/* MII speed control register	    */
+	uint	res4[17];		/* reserved			    */
+	uint	fec_r_bound;		/* end of RAM (read-only)	    */
+	uint	fec_r_fstart;		/* Rx FIFO start address	    */
+	uint	res5[6];		/* reserved			    */
+	uint	fec_x_fstart;		/* Tx FIFO start address	    */
+	uint	res6[17];		/* reserved			    */
+	uint	fec_fun_code;		/* fec SDMA function code	    */
+	uint	res7[3];		/* reserved			    */
+	uint	fec_r_cntrl;		/* Rx control register		    */
+	uint	fec_r_hash;		/* Rx hash register		    */
+	uint	res8[14];		/* reserved			    */
+	uint	fec_x_cntrl;		/* Tx control register		    */
+	uint	res9[0x1e];		/* reserved			    */
+};
-- 
1.5.6.3

[net-next-2.6 PATCH 3/3] fs_enet: Add FEC TX Alignment workaround for MPC5121

[Anatolij Gustschin]

The FEC on 5121 has problems with misaligned tx buffers.
The RM says any alignment is ok but empirical results
show that packet buffers ending in 0x1E will sometimes
hang the FEC.  Other bad alignment does not hang but will
cause silent TX failures resulting in about a 1% packet
loss as tested by ping -f from a remote host.

This patch is a work around that copies every tx packet
to an aligned skb before sending.

Signed-off-by: John Rigby <jcrigby@gmail.com>
Signed-off-by: Piotr Ziecik <kosmo@semihalf.com>
Signed-off-by: Wolfgang Denk <wd@denx.de>
Signed-off-by: Anatolij Gustschin <agust@denx.de>
Cc: <linuxppc-dev@ozlabs.org>
Cc: Grant Likely <grant.likely@secretlab.ca>
---
Changes since previous submited version:

- don't use printk any more and use dev_warn().

 drivers/net/fs_enet/fs_enet-main.c |   38 ++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c
index 6bce5c8..7f0bd5d 100644
--- a/drivers/net/fs_enet/fs_enet-main.c
+++ b/drivers/net/fs_enet/fs_enet-main.c
@@ -580,6 +580,32 @@ void fs_cleanup_bds(struct net_device *dev)
 
 /**********************************************************************************/
 
+static struct sk_buff *tx_skb_align_workaround(struct net_device *dev,
+					       struct sk_buff *skb)
+{
+	struct sk_buff *new_skb;
+	struct fs_enet_private *fep = netdev_priv(dev);
+
+	/* Alloc new skb */
+	new_skb = dev_alloc_skb(ENET_RX_FRSIZE + 32);
+	if (!new_skb) {
+		dev_warn(fep->dev, "Memory squeeze, dropping tx packet.\n");
+		return NULL;
+	}
+
+	/* Make sure new skb is properly aligned */
+	skb_align(new_skb, 32);
+
+	/* Copy data to new skb ... */
+	skb_copy_from_linear_data(skb, new_skb->data, skb->len);
+	skb_put(new_skb, skb->len);
+
+	/* ... and free an old one */
+	dev_kfree_skb_any(skb);
+
+	return new_skb;
+}
+
 static int fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
@@ -588,6 +614,18 @@ static int fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	u16 sc;
 	unsigned long flags;
 
+	if (fep->fec.fec_id == FS_ENET_MPC5121_FEC) {
+		skb = tx_skb_align_workaround(dev, skb);
+		if (!skb) {
+			/*
+			 * We have lost packet due to memory allocation error
+			 * in tx_skb_align_workaround(). Hopefully original skb
+			 * is still valid, so try transmit it later.
+			 */
+			return NETDEV_TX_BUSY;
+		}
+	}
+
 	spin_lock_irqsave(&fep->tx_lock, flags);
 
 	/*
-- 
1.5.6.3

Re: [net-next-2.6 PATCH 2/3] fs_enet: Add support for MPC512x to fs_enet driver

[David Miller]

From: Anatolij Gustschin <agust@denx.de>
Date: Thu, 21 Jan 2010 03:13:18 +0100

>  struct fec_info {
> -	fec_t __iomem *fecp;
> +	void __iomem *fecp;
 ...
>  /* write */
> -#define FW(_fecp, _reg, _v) __fs_out32(&(_fecp)->fec_ ## _reg, (_v))
> +#define FW(_regp, _reg, _v) __fs_out32((_regp)->fec_ ## _reg, (_v))
 ...
> +/* register address macros */
> +#define fec_reg_addr(_type, _reg) \
> +	(fep->fec.rtbl->fec_##_reg = (u32 __iomem *)((u32)fep->fec.fecp + \
> +				(u32)&((__typeof__(_type) *)NULL)->fec_##_reg))
> +
> +#define fec_reg_mpc8xx(_reg) \
> +	fec_reg_addr(struct mpc8xx_fec, _reg)
> +
> +#define fec_reg_mpc5121(_reg) \
> +	fec_reg_addr(struct mpc5121_fec, _reg)

This is a step backwards in my view.

If you use the "fec_t __iomem *" type for the register
pointer, you simply use &p->fecp->XXX to get the I/O
address of register XXX and that's what you pass to
the appropriate I/O accessor routines.

Now you've made it typeless, and then you have to walk
through all of these contortions to get the offset.

I don't want to apply this, sorry...

Re: [net-next-2.6 PATCH 2/3] fs_enet: Add support for MPC512x to fs_enet driver

[Anatolij Gustschin]

On Thu, 21 Jan 2010 01:22:35 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Anatolij Gustschin <agust@denx.de>
> Date: Thu, 21 Jan 2010 03:13:18 +0100
> 
> >  struct fec_info {
> > -	fec_t __iomem *fecp;
> > +	void __iomem *fecp;
>  ...
> >  /* write */
> > -#define FW(_fecp, _reg, _v) __fs_out32(&(_fecp)->fec_ ## _reg, (_v))
> > +#define FW(_regp, _reg, _v) __fs_out32((_regp)->fec_ ## _reg, (_v))
>  ...
> > +/* register address macros */
> > +#define fec_reg_addr(_type, _reg) \
> > +	(fep->fec.rtbl->fec_##_reg = (u32 __iomem *)((u32)fep->fec.fecp + \
> > +				(u32)&((__typeof__(_type) *)NULL)->fec_##_reg))
> > +
> > +#define fec_reg_mpc8xx(_reg) \
> > +	fec_reg_addr(struct mpc8xx_fec, _reg)
> > +
> > +#define fec_reg_mpc5121(_reg) \
> > +	fec_reg_addr(struct mpc5121_fec, _reg)
> 
> This is a step backwards in my view.
> 
> If you use the "fec_t __iomem *" type for the register
> pointer, you simply use &p->fecp->XXX to get the I/O
> address of register XXX and that's what you pass to
> the appropriate I/O accessor routines.
> 
> Now you've made it typeless, and then you have to walk
> through all of these contortions to get the offset.
 
OK, i give up

Re: [net-next-2.6 PATCH 2/3] fs_enet: Add support for MPC512x to fs_enet driver

[Wolfgang Grandegger]

David Miller wrote:
> From: Anatolij Gustschin <agust@denx.de>
> Date: Thu, 21 Jan 2010 03:13:18 +0100
> 
>>  struct fec_info {
>> -	fec_t __iomem *fecp;
>> +	void __iomem *fecp;

To avoid confusion, the name "base_addr" seems more appropriate as it's
just used to calculate register offsets and for iomap/unmap.

>  ...
>>  /* write */
>> -#define FW(_fecp, _reg, _v) __fs_out32(&(_fecp)->fec_ ## _reg, (_v))
>> +#define FW(_regp, _reg, _v) __fs_out32((_regp)->fec_ ## _reg, (_v))
>  ...
>> +/* register address macros */
>> +#define fec_reg_addr(_type, _reg) \
>> +	(fep->fec.rtbl->fec_##_reg = (u32 __iomem *)((u32)fep->fec.fecp + \
>> +				(u32)&((__typeof__(_type) *)NULL)->fec_##_reg))
>> +
>> +#define fec_reg_mpc8xx(_reg) \
>> +	fec_reg_addr(struct mpc8xx_fec, _reg)
>> +
>> +#define fec_reg_mpc5121(_reg) \
>> +	fec_reg_addr(struct mpc5121_fec, _reg)
> 
> This is a step backwards in my view.
> 
> If you use the "fec_t __iomem *" type for the register
> pointer, you simply use &p->fecp->XXX to get the I/O
> address of register XXX and that's what you pass to
> the appropriate I/O accessor routines.
> 
> Now you've made it typeless, and then you have to walk
> through all of these contortions to get the offset.
> 
> I don't want to apply this, sorry...

The major problem that Anatolij tries to solve are the different
register layouts of the supported SOCs, MPC52xx and MPC8xx. They use the
same registers but at *different* offsets. Therefore we cannot handle
this with a single "fec_t" struct to allow building a single kernel
image. Instead it's handled by filling a table with register addresses:

	if (of_device_is_compatible(ofdev->node, "fsl,mpc5121-fec")) {
		fep->fec.fec_id = FS_ENET_MPC5121_FEC;
		fec_reg_mpc5121(ievent);
		fec_reg_mpc5121(imask);
		...
	} else {
		fec_reg_mpc8xx(ievent);
		fec_reg_mpc8xx(imask);
		...
	}

Do you see a more clever solution to this problem? Nevertheless, the
code could be improved by using "offsetof", I think.

Wolfgang.

Re: [net-next-2.6 PATCH 1/3] fs_enet: use dev_xxx instead of printk

[Grant Likely]

On Wed, Jan 20, 2010 at 7:13 PM, Anatolij Gustschin <agust@denx.de> wrote:
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> Cc: <linuxppc-dev@ozlabs.org>
> Cc: Grant Likely <grant.likely@secretlab.ca>

Acked-by: Grant Likely <grant.likely@secretlab.ca>

> Cc: John Rigby <jcrigby@gmail.com>
> Cc: Piotr Ziecik <kosmo@semihalf.com>
> Cc: Wolfgang Denk <wd@denx.de>
> ---
> =A0drivers/net/fs_enet/fs_enet-main.c | =A0 39 +++++++++++++-------------=
---------
> =A0drivers/net/fs_enet/mac-fcc.c =A0 =A0 =A0| =A0 =A05 ++-
> =A0drivers/net/fs_enet/mac-fec.c =A0 =A0 =A0| =A0 12 ++++------
> =A0drivers/net/fs_enet/mac-scc.c =A0 =A0 =A0| =A0 =A09 +++----
> =A04 files changed, 27 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_=
enet-main.c
> index ec2f503..c34a7e0 100644
> --- a/drivers/net/fs_enet/fs_enet-main.c
> +++ b/drivers/net/fs_enet/fs_enet-main.c
> @@ -108,9 +108,7 @@ static int fs_enet_rx_napi(struct napi_struct *napi, =
int budget)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 * the last indicator should be set.
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 */
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if ((sc & BD_ENET_RX_LAST) =3D=3D 0)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_WARNING DRV_MOD=
ULE_NAME
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0": %s rcv is=
 not +last\n",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev->name);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_warn(fep->dev, "rcv is =
not +last\n");
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/*
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 * Check for errors.
> @@ -178,9 +176,8 @@ static int fs_enet_rx_napi(struct napi_struct *napi, =
int budget)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0received++=
;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0netif_rece=
ive_skb(skb);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} else {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN=
_WARNING DRV_MODULE_NAME
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0": %s Memory squeeze, dropping packet.\n",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0dev->name);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_warn(fe=
p->dev,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0"Memory squeeze, dropping packet.\n");
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fep->stats=
.rx_dropped++;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0skbn =3D s=
kb;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> @@ -242,9 +239,7 @@ static int fs_enet_rx_non_napi(struct net_device *dev=
)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 * the last indicator should be set.
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 */
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if ((sc & BD_ENET_RX_LAST) =3D=3D 0)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_WARNING DRV_MOD=
ULE_NAME
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0": %s rcv is=
 not +last\n",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev->name);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_warn(fep->dev, "rcv is =
not +last\n");
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/*
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 * Check for errors.
> @@ -313,9 +308,8 @@ static int fs_enet_rx_non_napi(struct net_device *dev=
)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0received++=
;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0netif_rx(s=
kb);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} else {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN=
_WARNING DRV_MODULE_NAME
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0": %s Memory squeeze, dropping packet.\n",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0dev->name);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_warn(fe=
p->dev,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0"Memory squeeze, dropping packet.\n");
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fep->stats=
.rx_dropped++;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0skbn =3D s=
kb;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> @@ -388,10 +382,10 @@ static void fs_enet_tx(struct net_device *dev)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} else
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0fep->stats.tx_packets++;
>
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (sc & BD_ENET_TX_READY)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_WARNING DRV_MOD=
ULE_NAME
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0": %s HEY! E=
net xmit interrupt and TX_READY.\n",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev->name);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (sc & BD_ENET_TX_READY) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_warn(fep->dev,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"HEY! En=
et xmit interrupt and TX_READY.\n");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
>
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/*
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 * Deferred means some collisions occurred=
 during transmit,
> @@ -511,9 +505,8 @@ void fs_init_bds(struct net_device *dev)
> =A0 =A0 =A0 =A0for (i =3D 0, bdp =3D fep->rx_bd_base; i < fep->rx_ring; i=
++, bdp++) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0skb =3D dev_alloc_skb(ENET_RX_FRSIZE);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (skb =3D=3D NULL) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_WARNING DRV_MOD=
ULE_NAME
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0": %s Memory=
 squeeze, unable to allocate skb\n",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev->name);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_warn(fep->dev,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"Memory =
squeeze, unable to allocate skb\n");
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0break;
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0skb_align(skb, ENET_RX_ALIGN);
> @@ -610,8 +603,7 @@ static int fs_enet_start_xmit(struct sk_buff *skb, st=
ruct net_device *dev)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 * Ooops. =A0All transmit buffers are full=
. =A0Bail out.
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 * This should not happen, since the tx qu=
eue should be stopped.
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 */
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_WARNING DRV_MODULE_NAME
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0": %s tx queue full!.\n", de=
v->name);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_warn(fep->dev, "tx queue full!.\n");
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return NETDEV_TX_BUSY;
> =A0 =A0 =A0 =A0}
>
> @@ -788,8 +780,7 @@ static int fs_enet_open(struct net_device *dev)
> =A0 =A0 =A0 =A0r =3D request_irq(fep->interrupt, fs_enet_interrupt, IRQF_=
SHARED,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0"fs_enet-mac", dev);
> =A0 =A0 =A0 =A0if (r !=3D 0) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_ERR DRV_MODULE_NAME
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0": %s Could not allocate FS_=
ENET IRQ!", dev->name);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(fep->dev, "Could not allocate FS_EN=
ET IRQ!");
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (fep->fpi->use_napi)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0napi_disable(&fep->napi);
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return -EINVAL;
> @@ -1053,7 +1044,7 @@ static int __devinit fs_enet_probe(struct of_device=
 *ofdev,
> =A0 =A0 =A0 =A0if (ret)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto out_free_bd;
>
> - =A0 =A0 =A0 printk(KERN_INFO "%s: fs_enet: %pM\n", ndev->name, ndev->de=
v_addr);
> + =A0 =A0 =A0 pr_info("%s: fs_enet: %pM\n", ndev->name, ndev->dev_addr);
>
> =A0 =A0 =A0 =A0return 0;
>
> diff --git a/drivers/net/fs_enet/mac-fcc.c b/drivers/net/fs_enet/mac-fcc.=
c
> index 22e5a84..dd78640 100644
> --- a/drivers/net/fs_enet/mac-fcc.c
> +++ b/drivers/net/fs_enet/mac-fcc.c
> @@ -476,8 +476,9 @@ static void clear_int_events(struct net_device *dev, =
u32 int_events)
>
> =A0static void ev_error(struct net_device *dev, u32 int_events)
> =A0{
> - =A0 =A0 =A0 printk(KERN_WARNING DRV_MODULE_NAME
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0": %s FS_ENET ERROR(s) 0x%x\n", dev->name, i=
nt_events);
> + =A0 =A0 =A0 struct fs_enet_private *fep =3D netdev_priv(dev);
> +
> + =A0 =A0 =A0 dev_warn(fep->dev, "FS_ENET ERROR(s) 0x%x\n", int_events);
> =A0}
>
> =A0static int get_regs(struct net_device *dev, void *p, int *sizep)
> diff --git a/drivers/net/fs_enet/mac-fec.c b/drivers/net/fs_enet/mac-fec.=
c
> index ca7bcb8..a664aa1 100644
> --- a/drivers/net/fs_enet/mac-fec.c
> +++ b/drivers/net/fs_enet/mac-fec.c
> @@ -257,8 +257,7 @@ static void restart(struct net_device *dev)
>
> =A0 =A0 =A0 =A0r =3D whack_reset(fep->fec.fecp);
> =A0 =A0 =A0 =A0if (r !=3D 0)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_ERR DRV_MODULE_NAME
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ": %s FEC R=
eset FAILED!\n", dev->name);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(fep->dev, "FEC Reset FAILED!\n");
> =A0 =A0 =A0 =A0/*
> =A0 =A0 =A0 =A0 * Set station address.
> =A0 =A0 =A0 =A0 */
> @@ -355,9 +354,7 @@ static void stop(struct net_device *dev)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0udelay(1);
>
> =A0 =A0 =A0 =A0if (i =3D=3D FEC_RESET_DELAY)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_WARNING DRV_MODULE_NAME
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0": %s FEC timeout on gracefu=
l transmit stop\n",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev->name);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_warn(fep->dev, "FEC timeout on graceful=
 transmit stop\n");
> =A0 =A0 =A0 =A0/*
> =A0 =A0 =A0 =A0 * Disable FEC. Let only MII interrupts.
> =A0 =A0 =A0 =A0 */
> @@ -433,8 +430,9 @@ static void clear_int_events(struct net_device *dev, =
u32 int_events)
>
> =A0static void ev_error(struct net_device *dev, u32 int_events)
> =A0{
> - =A0 =A0 =A0 printk(KERN_WARNING DRV_MODULE_NAME
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0": %s FEC ERROR(s) 0x%x\n", dev->name, int_e=
vents);
> + =A0 =A0 =A0 struct fs_enet_private *fep =3D netdev_priv(dev);
> +
> + =A0 =A0 =A0 dev_warn(fep->dev, "FEC ERROR(s) 0x%x\n", int_events);
> =A0}
>
> =A0static int get_regs(struct net_device *dev, void *p, int *sizep)
> diff --git a/drivers/net/fs_enet/mac-scc.c b/drivers/net/fs_enet/mac-scc.=
c
> index 008cdd9..2d8917f 100644
> --- a/drivers/net/fs_enet/mac-scc.c
> +++ b/drivers/net/fs_enet/mac-scc.c
> @@ -367,9 +367,7 @@ static void stop(struct net_device *dev)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0udelay(1);
>
> =A0 =A0 =A0 =A0if (i =3D=3D SCC_RESET_DELAY)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 printk(KERN_WARNING DRV_MODULE_NAME
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0": %s SCC timeout on gracefu=
l transmit stop\n",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev->name);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_warn(fep->dev, "SCC timeout on graceful=
 transmit stop\n");
>
> =A0 =A0 =A0 =A0W16(sccp, scc_sccm, 0);
> =A0 =A0 =A0 =A0C32(sccp, scc_gsmrl, SCC_GSMRL_ENR | SCC_GSMRL_ENT);
> @@ -429,8 +427,9 @@ static void clear_int_events(struct net_device *dev, =
u32 int_events)
>
> =A0static void ev_error(struct net_device *dev, u32 int_events)
> =A0{
> - =A0 =A0 =A0 printk(KERN_WARNING DRV_MODULE_NAME
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0": %s SCC ERROR(s) 0x%x\n", dev->name, int_e=
vents);
> + =A0 =A0 =A0 struct fs_enet_private *fep =3D netdev_priv(dev);
> +
> + =A0 =A0 =A0 dev_warn(fep->dev, "SCC ERROR(s) 0x%x\n", int_events);
> =A0}
>
> =A0static int get_regs(struct net_device *dev, void *p, int *sizep)
> --
> 1.5.6.3
>
>



--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [net-next-2.6 PATCH 3/3] fs_enet: Add FEC TX Alignment workaround for MPC5121

[Grant Likely]

On Wed, Jan 20, 2010 at 7:13 PM, Anatolij Gustschin <agust@denx.de> wrote:
> The FEC on 5121 has problems with misaligned tx buffers.
> The RM says any alignment is ok but empirical results
> show that packet buffers ending in 0x1E will sometimes
> hang the FEC. =A0Other bad alignment does not hang but will
> cause silent TX failures resulting in about a 1% packet
> loss as tested by ping -f from a remote host.
>
> This patch is a work around that copies every tx packet
> to an aligned skb before sending.
>
> Signed-off-by: John Rigby <jcrigby@gmail.com>
> Signed-off-by: Piotr Ziecik <kosmo@semihalf.com>
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> Cc: <linuxppc-dev@ozlabs.org>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> ---
> Changes since previous submited version:
>
> - don't use printk any more and use dev_warn().
>
> =A0drivers/net/fs_enet/fs_enet-main.c | =A0 38 ++++++++++++++++++++++++++=
++++++++++
> =A01 files changed, 38 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_=
enet-main.c
> index 6bce5c8..7f0bd5d 100644
> --- a/drivers/net/fs_enet/fs_enet-main.c
> +++ b/drivers/net/fs_enet/fs_enet-main.c
> @@ -580,6 +580,32 @@ void fs_cleanup_bds(struct net_device *dev)
>
> =A0/*********************************************************************=
*************/
>
> +static struct sk_buff *tx_skb_align_workaround(struct net_device *dev,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0 =A0 =A0 =A0struct sk_buff *skb)
> +{
> + =A0 =A0 =A0 struct sk_buff *new_skb;
> + =A0 =A0 =A0 struct fs_enet_private *fep =3D netdev_priv(dev);
> +
> + =A0 =A0 =A0 /* Alloc new skb */
> + =A0 =A0 =A0 new_skb =3D dev_alloc_skb(ENET_RX_FRSIZE + 32);
> + =A0 =A0 =A0 if (!new_skb) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_warn(fep->dev, "Memory squeeze, droppin=
g tx packet.\n");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return NULL;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 /* Make sure new skb is properly aligned */
> + =A0 =A0 =A0 skb_align(new_skb, 32);
> +
> + =A0 =A0 =A0 /* Copy data to new skb ... */
> + =A0 =A0 =A0 skb_copy_from_linear_data(skb, new_skb->data, skb->len);
> + =A0 =A0 =A0 skb_put(new_skb, skb->len);
> +
> + =A0 =A0 =A0 /* ... and free an old one */
> + =A0 =A0 =A0 dev_kfree_skb_any(skb);
> +
> + =A0 =A0 =A0 return new_skb;
> +}

This looks expensive.  It's allocating a new skb on *every* packet
transmission, rather than just on the ones that fail the alignment
condition.

> +
> =A0static int fs_enet_start_xmit(struct sk_buff *skb, struct net_device *=
dev)
> =A0{
> =A0 =A0 =A0 =A0struct fs_enet_private *fep =3D netdev_priv(dev);
> @@ -588,6 +614,18 @@ static int fs_enet_start_xmit(struct sk_buff *skb, s=
truct net_device *dev)
> =A0 =A0 =A0 =A0u16 sc;
> =A0 =A0 =A0 =A0unsigned long flags;
>
> + =A0 =A0 =A0 if (fep->fec.fec_id =3D=3D FS_ENET_MPC5121_FEC) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 skb =3D tx_skb_align_workaround(dev, skb);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!skb) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /*
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* We have lost packet du=
e to memory allocation error
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* in tx_skb_align_workar=
ound(). Hopefully original skb
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* is still valid, so try=
 transmit it later.
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return NETDEV_TX_BUSY;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 }
> +

Rather than performing this test every time through on a fast path,
consider creating a new start_xmit hook and registering the correct
one in the ops structure at probe time.

> =A0 =A0 =A0 =A0spin_lock_irqsave(&fep->tx_lock, flags);
>
> =A0 =A0 =A0 =A0/*
> --
> 1.5.6.3
>
>



--=20
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [net-next-2.6 PATCH 2/3] fs_enet: Add support for MPC512x to fs_enet driver

[Wolfgang Grandegger]

Hi Anatolij,

I had a close look...

Anatolij Gustschin wrote:
>     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 <jcrigby@gmail.com>
> Signed-off-by: Piotr Ziecik <kosmo@semihalf.com>
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> Cc: <linuxppc-dev@ozlabs.org>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> ---
> Changes since previous submited version:
> 
> - explicit type usage in register tables.
> - don't use same variable name "fecp" for variables of
>   different types.
> - avoid re-checking the compatible by passing data pointer
>   in the match struct.
> 
>  drivers/net/fs_enet/Kconfig        |   10 +-
>  drivers/net/fs_enet/fs_enet-main.c |    4 +
>  drivers/net/fs_enet/fs_enet.h      |   40 +++++++-
>  drivers/net/fs_enet/mac-fec.c      |  212 +++++++++++++++++++++++++-----------
>  drivers/net/fs_enet/mii-fec.c      |   76 ++++++++++---
>  drivers/net/fs_enet/mpc5121_fec.h  |   64 +++++++++++
>  drivers/net/fs_enet/mpc8xx_fec.h   |   37 ++++++
>  7 files changed, 356 insertions(+), 87 deletions(-)
>  create mode 100644 drivers/net/fs_enet/mpc5121_fec.h
>  create mode 100644 drivers/net/fs_enet/mpc8xx_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 c34a7e0..6bce5c8 100644
> --- a/drivers/net/fs_enet/fs_enet-main.c
> +++ b/drivers/net/fs_enet/fs_enet-main.c
> @@ -1095,6 +1095,10 @@ static struct of_device_id fs_enet_match[] = {
>  #endif
>  #ifdef CONFIG_FS_ENET_HAS_FEC
>  	{
> +		.compatible = "fsl,mpc5121-fec",
> +		.data = (void *)&fs_fec_ops,
> +	},
> +	{
>  		.compatible = "fsl,pq1-fec-enet",
>  		.data = (void *)&fs_fec_ops,
>  	},
> diff --git a/drivers/net/fs_enet/fs_enet.h b/drivers/net/fs_enet/fs_enet.h
> index ef01e09..df935e8 100644
> --- a/drivers/net/fs_enet/fs_enet.h
> +++ b/drivers/net/fs_enet/fs_enet.h
> @@ -13,11 +13,47 @@
>  
>  #ifdef CONFIG_CPM1
>  #include <asm/cpm1.h>
> +#endif
> +
> +#if defined(CONFIG_FS_ENET_HAS_FEC)
> +#include <asm/cpm.h>
> +#include "mpc8xx_fec.h"
> +#include "mpc5121_fec.h"

Do we really need the new header files? Why not adding the struct
definitions here or use "struct fec" from 8xx_immap.h. See below.

>  struct fec_info {
> -	fec_t __iomem *fecp;
> +	void __iomem *fecp;

A name like fec_base or base_addr would help to avoid confusion with a
pointer to the old fec struct.

> +	u32 __iomem *fec_r_cntrl;
> +	u32 __iomem *fec_ecntrl;
> +	u32 __iomem *fec_ievent;
> +	u32 __iomem *fec_mii_data;
> +	u32 __iomem *fec_mii_speed;
>  	u32 mii_speed;
>  };
> +
> +struct reg_tbl {

A more specific name would be nice, e.g. "fec_reg_tbl" or "fec_regs".

> +	u32 __iomem *fec_ievent;
> +	u32 __iomem *fec_imask;
> +	u32 __iomem *fec_r_des_active;
> +	u32 __iomem *fec_x_des_active;
> +	u32 __iomem *fec_r_des_start;
> +	u32 __iomem *fec_x_des_start;
> +	u32 __iomem *fec_r_cntrl;
> +	u32 __iomem *fec_ecntrl;
> +	u32 __iomem *fec_ivec;
> +	u32 __iomem *fec_mii_speed;
> +	u32 __iomem *fec_addr_low;
> +	u32 __iomem *fec_addr_high;
> +	u32 __iomem *fec_hash_table_high;
> +	u32 __iomem *fec_hash_table_low;
> +	u32 __iomem *fec_r_buff_size;
> +	u32 __iomem *fec_r_bound;
> +	u32 __iomem *fec_r_fstart;
> +	u32 __iomem *fec_x_fstart;
> +	u32 __iomem *fec_fun_code;
> +	u32 __iomem *fec_r_hash;
> +	u32 __iomem *fec_x_cntrl;
> +	u32 __iomem *fec_dma_control;
> +};
>  #endif
>  
>  #ifdef CONFIG_CPM2
> @@ -113,7 +149,9 @@ struct fs_enet_private {
>  		struct {
>  			int idx;		/* FEC1 = 0, FEC2 = 1  */
>  			void __iomem *fecp;	/* hw registers        */

See above.

> +			struct reg_tbl *rtbl;	/* used registers table */
>  			u32 hthi, htlo;		/* state for multicast */
> +			u32 fec_id;
>  		} fec;
>  
>  		struct {
> diff --git a/drivers/net/fs_enet/mac-fec.c b/drivers/net/fs_enet/mac-fec.c
> index a664aa1..fe9e368 100644
> --- a/drivers/net/fs_enet/mac-fec.c
> +++ b/drivers/net/fs_enet/mac-fec.c
> @@ -64,29 +64,40 @@
>  #endif
>  
>  /* write */
> -#define FW(_fecp, _reg, _v) __fs_out32(&(_fecp)->fec_ ## _reg, (_v))
> +#define FW(_regp, _reg, _v) __fs_out32((_regp)->fec_ ## _reg, (_v))
>  
>  /* read */
> -#define FR(_fecp, _reg)	__fs_in32(&(_fecp)->fec_ ## _reg)
> +#define FR(_regp, _reg)	__fs_in32((_regp)->fec_ ## _reg)
>  
>  /* set bits */
> -#define FS(_fecp, _reg, _v) FW(_fecp, _reg, FR(_fecp, _reg) | (_v))
> +#define FS(_regp, _reg, _v) FW(_regp, _reg, FR(_regp, _reg) | (_v))
>  
>  /* clear bits */
> -#define FC(_fecp, _reg, _v) FW(_fecp, _reg, FR(_fecp, _reg) & ~(_v))
> +#define FC(_regp, _reg, _v) FW(_regp, _reg, FR(_regp, _reg) & ~(_v))
> +
> +/* register address macros */
> +#define fec_reg_addr(_type, _reg) \
> +	(fep->fec.rtbl->fec_##_reg = (u32 __iomem *)((u32)fep->fec.fecp + \
> +				(u32)&((__typeof__(_type) *)NULL)->fec_##_reg))

I think you don't need the cast in the first line and using "offsetof"
would simplify the macro further. I would also use _fep as first
argument to make this macro function more transparent.

> +#define fec_reg_mpc8xx(_reg) \
> +	fec_reg_addr(struct mpc8xx_fec, _reg)
> +
> +#define fec_reg_mpc5121(_reg) \
> +	fec_reg_addr(struct mpc5121_fec, _reg)

Also, s/fec_reg_addr/fec_set_reg_addr/ would give the three macros above
a more appropriate name.

>  /*
>   * Delay to wait for FEC reset command to complete (in us)
>   */
>  #define FEC_RESET_DELAY		50
>  
> -static int whack_reset(fec_t __iomem *fecp)
> +static int whack_reset(struct reg_tbl *regp)
>  {
>  	int i;
>  
> -	FW(fecp, ecntrl, FEC_ECNTRL_PINMUX | FEC_ECNTRL_RESET);
> +	FW(regp, ecntrl, FEC_ECNTRL_PINMUX | FEC_ECNTRL_RESET);
>  	for (i = 0; i < FEC_RESET_DELAY; i++) {
> -		if ((FR(fecp, ecntrl) & FEC_ECNTRL_RESET) == 0)
> +		if ((FR(regp, ecntrl) & FEC_ECNTRL_RESET) == 0)
>  			return 0;	/* OK */
>  		udelay(1);
>  	}
> @@ -106,6 +117,50 @@ static int do_pd_setup(struct fs_enet_private *fep)
>  	if (!fep->fcc.fccp)
>  		return -EINVAL;
>  
> +	fep->fec.rtbl = kzalloc(sizeof(*fep->fec.rtbl), GFP_KERNEL);
> +	if (!fep->fec.rtbl) {
> +		iounmap(fep->fec.fecp);
> +		return -ENOMEM;
> +	}

Any reason why not adding the struct directly to fep->fec? It would save
some code for alloc/free and the addresses would be "closer" (cache wise).

> +	if (of_device_is_compatible(ofdev->node, "fsl,mpc5121-fec")) {
> +		fep->fec.fec_id = FS_ENET_MPC5121_FEC;
> +		fec_reg_mpc5121(ievent);
> +		fec_reg_mpc5121(imask);
> +		fec_reg_mpc5121(r_cntrl);
> +		fec_reg_mpc5121(ecntrl);
> +		fec_reg_mpc5121(r_des_active);
> +		fec_reg_mpc5121(x_des_active);
> +		fec_reg_mpc5121(r_des_start);
> +		fec_reg_mpc5121(x_des_start);
> +		fec_reg_mpc5121(addr_low);
> +		fec_reg_mpc5121(addr_high);
> +		fec_reg_mpc5121(hash_table_high);
> +		fec_reg_mpc5121(hash_table_low);
> +		fec_reg_mpc5121(r_buff_size);
> +		fec_reg_mpc5121(mii_speed);
> +		fec_reg_mpc5121(x_cntrl);
> +		fec_reg_mpc5121(dma_control);
> +	} else {
> +		fec_reg_mpc8xx(ievent);
> +		fec_reg_mpc8xx(imask);
> +		fec_reg_mpc8xx(r_cntrl);
> +		fec_reg_mpc8xx(ecntrl);
> +		fec_reg_mpc8xx(mii_speed);
> +		fec_reg_mpc8xx(r_des_active);
> +		fec_reg_mpc8xx(x_des_active);
> +		fec_reg_mpc8xx(r_des_start);
> +		fec_reg_mpc8xx(x_des_start);
> +		fec_reg_mpc8xx(ivec);
> +		fec_reg_mpc8xx(addr_low);
> +		fec_reg_mpc8xx(addr_high);
> +		fec_reg_mpc8xx(hash_table_high);
> +		fec_reg_mpc8xx(hash_table_low);
> +		fec_reg_mpc8xx(r_buff_size);
> +		fec_reg_mpc8xx(x_fstart);
> +		fec_reg_mpc8xx(r_hash);
> +		fec_reg_mpc8xx(x_cntrl);
> +	}
>  	return 0;
>  }
>  
> @@ -162,15 +217,17 @@ static void free_bd(struct net_device *dev)
>  
>  static void cleanup_data(struct net_device *dev)
>  {
> -	/* nothing */
> +	struct fs_enet_private *fep = netdev_priv(dev);
> +
> +	kfree(fep->fec.rtbl);
>  }

See above.

[snip]
> +++ b/drivers/net/fs_enet/mpc5121_fec.h
> @@ -0,0 +1,64 @@
> +/*
> + * 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
> +
> +struct mpc5121_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 */
> +};
> +
> +#define FS_ENET_MPC5121_FEC	0x1
> +
> +#endif /* MPC5121_FEC_H */
> diff --git a/drivers/net/fs_enet/mpc8xx_fec.h b/drivers/net/fs_enet/mpc8xx_fec.h
> new file mode 100644
> index 0000000..aa78445
> --- /dev/null
> +++ b/drivers/net/fs_enet/mpc8xx_fec.h
> @@ -0,0 +1,37 @@
> +/* MPC860T Fast Ethernet Controller.  It isn't part of the CPM, but
> + * it fits within the address space.
> + */
> +

The usual "#ifndef" stuff is missing, in case you keep it.

> +struct mpc8xx_fec {
> +	uint	fec_addr_low;		/* lower 32 bits of station address */
> +	ushort	fec_addr_high;		/* upper 16 bits of station address */
> +	ushort	res1;			/* reserved			    */
> +	uint	fec_hash_table_high;	/* upper 32-bits of hash table	    */
> +	uint	fec_hash_table_low;	/* lower 32-bits of hash table	    */
> +	uint	fec_r_des_start;	/* beginning of Rx descriptor ring  */
> +	uint	fec_x_des_start;	/* beginning of Tx descriptor ring  */
> +	uint	fec_r_buff_size;	/* Rx buffer size		    */
> +	uint	res2[9];		/* reserved			    */
> +	uint	fec_ecntrl;		/* ethernet control register	    */
> +	uint	fec_ievent;		/* interrupt event register	    */
> +	uint	fec_imask;		/* interrupt mask register	    */
> +	uint	fec_ivec;		/* interrupt level and vector status */
> +	uint	fec_r_des_active;	/* Rx ring updated flag		    */
> +	uint	fec_x_des_active;	/* Tx ring updated flag		    */
> +	uint	res3[10];		/* reserved			    */
> +	uint	fec_mii_data;		/* MII data register		    */
> +	uint	fec_mii_speed;		/* MII speed control register	    */
> +	uint	res4[17];		/* reserved			    */
> +	uint	fec_r_bound;		/* end of RAM (read-only)	    */
> +	uint	fec_r_fstart;		/* Rx FIFO start address	    */
> +	uint	res5[6];		/* reserved			    */
> +	uint	fec_x_fstart;		/* Tx FIFO start address	    */
> +	uint	res6[17];		/* reserved			    */
> +	uint	fec_fun_code;		/* fec SDMA function code	    */
> +	uint	res7[3];		/* reserved			    */
> +	uint	fec_r_cntrl;		/* Rx control register		    */
> +	uint	fec_r_hash;		/* Rx hash register		    */
> +	uint	res8[14];		/* reserved			    */
> +	uint	fec_x_cntrl;		/* Tx control register		    */
> +	uint	res9[0x1e];		/* reserved			    */
> +};

As mentioned above, I do not see a need for two extra header files. The
struct(s) could be added to fec.h. Also a similar "struct fec" is
already defined in "arch/powerpc/include/asm/8xx_immap.h", which could
be used instead of "struct mpc8xx_fec" above.

Wolfgang.

Re: [net-next-2.6 PATCH 2/3] fs_enet: Add support for MPC512x to fs_enet driver

[David Miller]

From: Wolfgang Grandegger <wg@grandegger.com>
Date: Thu, 21 Jan 2010 16:25:38 +0100

> Do you see a more clever solution to this problem?

See how we handle this in the ESP scsi driver.  We have a set of
defines for the register offsets, and a set of methods a chip driver
implements for register accesses.

If the offsets differ, the register access method can translate the
generic register offsets into whatever layout their implementation
actually uses.

> Nevertheless, the code could be improved by using "offsetof", I
> think.

At a minimum :-)

Re: [net-next-2.6 PATCH 2/3] fs_enet: Add support for MPC512x to fs_enet driver

[Wolfgang Grandegger]

David Miller wrote:
> From: Wolfgang Grandegger <wg@grandegger.com>
> Date: Thu, 21 Jan 2010 16:25:38 +0100
> 
>> Do you see a more clever solution to this problem?
> 
> See how we handle this in the ESP scsi driver.  We have a set of
> defines for the register offsets, and a set of methods a chip driver
> implements for register accesses.
> 
> If the offsets differ, the register access method can translate the
> generic register offsets into whatever layout their implementation
> actually uses.

I think you speak about:

    void (*esp_write8)(struct esp *esp, u8 val, unsigned long reg);
    u8 (*esp_read8)(struct esp *esp, unsigned long reg);

But still we need to translate the *generic* offset (reg) into the real
offset, which requires a lookup/table to get it. For me this seems not
really more efficient and less transparent as it bends the offsets.

Wolfgang.

Re: [net-next-2.6 PATCH 2/3] fs_enet: Add support for MPC512x to fs_enet driver

[Arnd Bergmann]

On Thursday 21 January 2010, Wolfgang Grandegger wrote:
> The major problem that Anatolij tries to solve are the different
> register layouts of the supported SOCs, MPC52xx and MPC8xx. They use the
> same registers but at different offsets. Therefore we cannot handle
> this with a single "fec_t" struct to allow building a single kernel
> image. Instead it's handled by filling a table with register addresses:
> 
>         if (of_device_is_compatible(ofdev->node, "fsl,mpc5121-fec")) {
>                 fep->fec.fec_id = FS_ENET_MPC5121_FEC;
>                 fec_reg_mpc5121(ievent);
>                 fec_reg_mpc5121(imask);
>                 ...
>         } else {
>                 fec_reg_mpc8xx(ievent);
>                 fec_reg_mpc8xx(imask);
>                 ...
>         }
> 
> Do you see a more clever solution to this problem? Nevertheless, the
> code could be improved by using "offsetof", I think.

Is there any chance of building a kernel that runs on both mpc8xx and
mpc5121? AFAIK, the 5121 is built on a 6xx core which is fundamentally
incompatible with 8xx due to different memory management etc.

Since this makes it all a compile-time decision, it should be solvable
with a very small number of carefully placed #ifdef in the header files
an no runtime detection at all.

Obviously this approach would not work for drivers that want to be portable
across different register layouts on otherwise compatible platforms.

	Arnd

Re: [net-next-2.6 PATCH 2/3] fs_enet: Add support for MPC512x to fs_enet driver

[Wolfgang Grandegger]

Arnd Bergmann wrote:
> On Thursday 21 January 2010, Wolfgang Grandegger wrote:
>> The major problem that Anatolij tries to solve are the different
>> register layouts of the supported SOCs, MPC52xx and MPC8xx. They use the
>> same registers but at different offsets. Therefore we cannot handle
>> this with a single "fec_t" struct to allow building a single kernel
>> image. Instead it's handled by filling a table with register addresses:
>>
>>         if (of_device_is_compatible(ofdev->node, "fsl,mpc5121-fec")) {
>>                 fep->fec.fec_id = FS_ENET_MPC5121_FEC;
>>                 fec_reg_mpc5121(ievent);
>>                 fec_reg_mpc5121(imask);
>>                 ...
>>         } else {
>>                 fec_reg_mpc8xx(ievent);
>>                 fec_reg_mpc8xx(imask);
>>                 ...
>>         }
>>
>> Do you see a more clever solution to this problem? Nevertheless, the
>> code could be improved by using "offsetof", I think.
> 
> Is there any chance of building a kernel that runs on both mpc8xx and
> mpc5121? AFAIK, the 5121 is built on a 6xx core which is fundamentally
> incompatible with 8xx due to different memory management etc.
> 
> Since this makes it all a compile-time decision, it should be solvable
> with a very small number of carefully placed #ifdef in the header files
> an no runtime detection at all.
> 
> Obviously this approach would not work for drivers that want to be portable
> across different register layouts on otherwise compatible platforms.

You are probably right and your proposal would likely result in more
transparent (less ugly) code. There has been some discussion about
unifying FEC drivers when the patches (with the same subject) have been
submitted for the first time in May last year, but it was not about 512x
and 8xx, IIRC.

Wolfgang.

Re: [net-next-2.6 PATCH 2/3] fs_enet: Add support for MPC512x to fs_enet driver

[Wolfgang Denk]

Dear Wolfgang & Arnd,

In message <4B5C5BDF.6020001@grandegger.com> you wrote:
>
> Arnd Bergmann wrote:
...
> > Is there any chance of building a kernel that runs on both mpc8xx and
> > mpc5121? AFAIK, the 5121 is built on a 6xx core which is fundamentally
> > incompatible with 8xx due to different memory management etc.

It is my understanding as well that you cannot have a single image
that boots both on 8xx and on 6xx cores. The focus was more on things
like supporting MPC5200 and MPC512x with the same image.

> > Since this makes it all a compile-time decision, it should be solvable
> > with a very small number of carefully placed #ifdef in the header files
> > an no runtime detection at all.
> > 
> > Obviously this approach would not work for drivers that want to be portable
> > across different register layouts on otherwise compatible platforms.
> 
> You are probably right and your proposal would likely result in more
> transparent (less ugly) code. There has been some discussion about
> unifying FEC drivers when the patches (with the same subject) have been
> submitted for the first time in May last year, but it was not about 512x
> and 8xx, IIRC.

You can re-read this discussion here:

http://patchwork.ozlabs.org/patch/26927/

ee especiall Grant's note of 2009-05-21 15:36:11: "If it looks too
ugly, then just fork the driver."

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Immortality consists largely of boredom.
	-- Zefrem Cochrane, "Metamorphosis", stardate 3219.8

Re: [net-next-2.6 PATCH 2/3] fs_enet: Add support for MPC512x to fs_enet driver

[Arnd Bergmann]

On Sunday 24 January 2010, Wolfgang Denk wrote:
> In message <4B5C5BDF.6020001@grandegger.com> you wrote:
> > 
> > You are probably right and your proposal would likely result in more
> > transparent (less ugly) code. There has been some discussion about
> > unifying FEC drivers when the patches (with the same subject) have been
> > submitted for the first time in May last year, but it was not about 512x
> > and 8xx, IIRC.
> 
> You can re-read this discussion here:
> 
> http://patchwork.ozlabs.org/patch/26927/
> 
> ee especiall Grant's note of 2009-05-21 15:36:11: "If it looks too
> ugly, then just fork the driver."

Ok. I fully agree with what Grant said in that thread, especially the
way the files could be split. Forking the entire driver would work
as an easy way to get it running at first, and we still have the option
of reorganizing the duplicate parts later in a saner way if that's seen
as helpful. I'd assume that at least some parts of it could become a
lib_fs_enet module that can be shared by all of them.

	Arnd

Re: [net-next-2.6 PATCH 2/3] fs_enet: Add support for MPC512x to fs_enet driver

[Wolfgang Grandegger]

Arnd Bergmann wrote:
> On Sunday 24 January 2010, Wolfgang Denk wrote:
>> In message <4B5C5BDF.6020001@grandegger.com> you wrote:
>>> You are probably right and your proposal would likely result in more
>>> transparent (less ugly) code. There has been some discussion about
>>> unifying FEC drivers when the patches (with the same subject) have been
>>> submitted for the first time in May last year, but it was not about 512x
>>> and 8xx, IIRC.
>> You can re-read this discussion here:
>>
>> http://patchwork.ozlabs.org/patch/26927/
>>
>> ee especiall Grant's note of 2009-05-21 15:36:11: "If it looks too
>> ugly, then just fork the driver."
> 
> Ok. I fully agree with what Grant said in that thread, especially the
> way the files could be split. Forking the entire driver would work
> as an easy way to get it running at first, and we still have the option
> of reorganizing the duplicate parts later in a saner way if that's seen
> as helpful. I'd assume that at least some parts of it could become a
> lib_fs_enet module that can be shared by all of them.

Yes, I also vote for forking the driver allowing a clean implementation.
 I don't think it makes sense to share a driver with the 8xx for the
reasons you already mentioned. And the 8xx is a dying out arch anyway.

Wolfgang.

Re: [net-next-2.6 PATCH 2/3] fs_enet: Add support for MPC512x to fs_enet driver

[Anatolij Gustschin]

On Thu, 21 Jan 2010 18:03:11 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Wolfgang Grandegger <wg@grandegger.com>
> Date: Thu, 21 Jan 2010 16:25:38 +0100
> 
> > Do you see a more clever solution to this problem?
> 
> See how we handle this in the ESP scsi driver.  We have a set of
> defines for the register offsets, and a set of methods a chip driver
> implements for register accesses.
> 
> If the offsets differ, the register access method can translate the
> generic register offsets into whatever layout their implementation
> actually uses.

First of all thanks for your suggestion. I have seen how you
handle register access in the ESP scsi driver. The reason I didn't
try to implement register access using similar approach is that
we have different sort of problem.

In my understanding, in the ESP scsi driver the set of defines for
the register offsets is common for all chip drivers. The chip driver
methods for register access translate the offsets because the
registers on some chips are at different intervals (4-byte, 1-byte,
16-byte for mac_esp.c). But the register order is the same for
different chips.

In our case non only the register order is not the same for 8xx
FEC and 5121 FEC, but there are also other differences, different
reserved areas between several registers, some registers are
available only on 8xx and some only on 5121.

Now at least tree people suggested to fork the driver. My question
is if you would accept a forked 5121 FEC specific driver realised
similar to drivers/net/fs_enet/mac-fec.c and
drivers/net/fs_enet/mii-fec.c drivers?
 
Thanks,

Anatolij

Re: [net-next-2.6 PATCH 2/3] fs_enet: Add support for MPC512x to fs_enet driver

[David Miller]

From: Anatolij Gustschin <agust@denx.de>
Date: Tue, 9 Feb 2010 15:23:17 +0100

> In my understanding, in the ESP scsi driver the set of defines for
> the register offsets is common for all chip drivers. The chip driver
> methods for register access translate the offsets because the
> registers on some chips are at different intervals (4-byte, 1-byte,
> 16-byte for mac_esp.c). But the register order is the same for
> different chips.
> 
> In our case non only the register order is not the same for 8xx
> FEC and 5121 FEC, but there are also other differences, different
> reserved areas between several registers, some registers are
> available only on 8xx and some only on 5121.

That only means you would need to use a table based register address
translation scheme, rather than a simple calculation.  Something
like:

static unsigned int chip_xxx_table[] =
{
	[GENERIC_REG_FOO]	 = CHIP_XXX_FOO,
	...
};

static u32 chip_xxx_read_reg(struct chip *p, unsigned int reg)
{
	unsigned int reg_off = chip_xxx_table[reg];

	return readl(p->regs + reg_off);
}

And this table can have special tokens in entries for
registers which do not exist on a chip, so you can trap
attempted access to them in these read/write handlers.

Please stop looking for excuses to fork this driver, a
unified driver I think can be done cleanly.

Re: [net-next-2.6 PATCH 2/3] fs_enet: Add support for MPC512x to fs_enet driver

[Wolfgang Grandegger]

Hi David,

David Miller wrote:
> From: Anatolij Gustschin <agust@denx.de>
> Date: Tue, 9 Feb 2010 15:23:17 +0100
> 
>> In my understanding, in the ESP scsi driver the set of defines for
>> the register offsets is common for all chip drivers. The chip driver
>> methods for register access translate the offsets because the
>> registers on some chips are at different intervals (4-byte, 1-byte,
>> 16-byte for mac_esp.c). But the register order is the same for
>> different chips.
>>
>> In our case non only the register order is not the same for 8xx
>> FEC and 5121 FEC, but there are also other differences, different
>> reserved areas between several registers, some registers are
>> available only on 8xx and some only on 5121.
> 
> That only means you would need to use a table based register address
> translation scheme, rather than a simple calculation.  Something
> like:
> 
> static unsigned int chip_xxx_table[] =
> {
> 	[GENERIC_REG_FOO]	 = CHIP_XXX_FOO,
> 	...
> };
> 
> static u32 chip_xxx_read_reg(struct chip *p, unsigned int reg)
> {
> 	unsigned int reg_off = chip_xxx_table[reg];
> 
> 	return readl(p->regs + reg_off);
> }
> 
> And this table can have special tokens in entries for
> registers which do not exist on a chip, so you can trap
> attempted access to them in these read/write handlers.

Yes, that could be done, but to honest, I do not see any improvement in
respect to the previous patch where the register offset were defined via
pointers within a structure.

> Please stop looking for excuses to fork this driver, a
> unified driver I think can be done cleanly.

Other people suggested to fork the driver because it's getting too ugly.

Wolfgang.

Re: [net-next-2.6 PATCH 2/3] fs_enet: Add support for MPC512x to fs_enet driver

[Wolfgang Grandegger]

Wolfgang Grandegger wrote:
> Hi David,
> 
> David Miller wrote:
>> From: Anatolij Gustschin <agust@denx.de>
>> Date: Tue, 9 Feb 2010 15:23:17 +0100
>>
>>> In my understanding, in the ESP scsi driver the set of defines for
>>> the register offsets is common for all chip drivers. The chip driver
>>> methods for register access translate the offsets because the
>>> registers on some chips are at different intervals (4-byte, 1-byte,
>>> 16-byte for mac_esp.c). But the register order is the same for
>>> different chips.
>>>
>>> In our case non only the register order is not the same for 8xx
>>> FEC and 5121 FEC, but there are also other differences, different
>>> reserved areas between several registers, some registers are
>>> available only on 8xx and some only on 5121.
>> That only means you would need to use a table based register address
>> translation scheme, rather than a simple calculation.  Something
>> like:
>>
>> static unsigned int chip_xxx_table[] =
>> {
>> 	[GENERIC_REG_FOO]	 = CHIP_XXX_FOO,
>> 	...
>> };
>>
>> static u32 chip_xxx_read_reg(struct chip *p, unsigned int reg)
>> {
>> 	unsigned int reg_off = chip_xxx_table[reg];
>>
>> 	return readl(p->regs + reg_off);
>> }
>>
>> And this table can have special tokens in entries for
>> registers which do not exist on a chip, so you can trap
>> attempted access to them in these read/write handlers.
> 
> Yes, that could be done, but to honest, I do not see any improvement in
> respect to the previous patch where the register offset were defined via
> pointers within a structure.
> 
>> Please stop looking for excuses to fork this driver, a
>> unified driver I think can be done cleanly.
> 
> Other people suggested to fork the driver because it's getting too ugly.

That said, I think there is consensus that it does not make sense, and
it's even not possible, to provide a kernel image which runs on both,
the 8xx and the mpc512x. Therefore, there is also no need for sharing
this driver at run time. Compile time selection would allow a more
elegant and transparent implementation. Would that be an acceptable
solution?

Wolfgang.

Re: [net-next-2.6 PATCH 2/3] fs_enet: Add support for MPC512x to fs_enet driver

[Grant Likely]

On Wed, Feb 10, 2010 at 3:20 AM, Wolfgang Grandegger <wg@grandegger.com> wr=
ote:
> Wolfgang Grandegger wrote:
>> Hi David,
>>
>> David Miller wrote:
>>> From: Anatolij Gustschin <agust@denx.de>
>>> Date: Tue, 9 Feb 2010 15:23:17 +0100
>>>
>>>> In my understanding, in the ESP scsi driver the set of defines for
>>>> the register offsets is common for all chip drivers. The chip driver
>>>> methods for register access translate the offsets because the
>>>> registers on some chips are at different intervals (4-byte, 1-byte,
>>>> 16-byte for mac_esp.c). But the register order is the same for
>>>> different chips.
>>>>
>>>> In our case non only the register order is not the same for 8xx
>>>> FEC and 5121 FEC, but there are also other differences, different
>>>> reserved areas between several registers, some registers are
>>>> available only on 8xx and some only on 5121.
>>> That only means you would need to use a table based register address
>>> translation scheme, rather than a simple calculation. =A0Something
>>> like:
>>>
>>> static unsigned int chip_xxx_table[] =3D
>>> {
>>> =A0 =A0 =A0[GENERIC_REG_FOO] =A0 =A0 =A0 =A0=3D CHIP_XXX_FOO,
>>> =A0 =A0 =A0...
>>> };
>>>
>>> static u32 chip_xxx_read_reg(struct chip *p, unsigned int reg)
>>> {
>>> =A0 =A0 =A0unsigned int reg_off =3D chip_xxx_table[reg];
>>>
>>> =A0 =A0 =A0return readl(p->regs + reg_off);
>>> }
>>>
>>> And this table can have special tokens in entries for
>>> registers which do not exist on a chip, so you can trap
>>> attempted access to them in these read/write handlers.
>>
>> Yes, that could be done, but to honest, I do not see any improvement in
>> respect to the previous patch where the register offset were defined via
>> pointers within a structure.
>>
>>> Please stop looking for excuses to fork this driver, a
>>> unified driver I think can be done cleanly.
>>
>> Other people suggested to fork the driver because it's getting too ugly.
>
> That said, I think there is consensus that it does not make sense, and
> it's even not possible, to provide a kernel image which runs on both,
> the 8xx and the mpc512x. Therefore, there is also no need for sharing
> this driver at run time. Compile time selection would allow a more
> elegant and transparent implementation. Would that be an acceptable
> solution?

I'm okay with compile time selection.

g.