[PATCH] [Rev2] MPC5121 FEC support

[PATCH] [Rev2] MPC5121 FEC support

[John Rigby]

Add support for MPC512x to fs_enet driver

NOTE: This patch is not perfect, there are still some CONFIG_CPM1
ifdefs.  But in reality this is not a problem since CPM2 and FEC
are mutually exclusive.  This patch does not break MULTIPLATFORM
kernels.

  arch/powerpc/boot/dts/mpc5121ads.dts
    Fix a typo in the file header and add ethernet and
    mdio nodes.

  drivers/net/fs_enet/Kconfig
    Add FS_ENET_MPC5121_FEC which selects
        FS_ENET and PPC_CPM_NEW_BINDING.
    This seemed better than adding || PPC_MPC5121 to the
    PPC_CPM_NEW_BINDING entry in arch/powerpc/platforms/Kconfig.

  drivers/net/fs_enet/fec_mpc5121.h
    This contains a struct fec with the appropriate
    order of fields for 5121 there is no typedef fec_t
    because checkpatch.pl complains about that.

  drivers/net/fs_enet/fs_enet-main.c
    Add a routine for conditionally copying TX skb's that
    are not aligned.  Only do this if fep->fpi->align_tx_packets is set.
    In fs_enet_probe set fpi->align_tx_packets to the value of
    fsl,align-tx-packets property in the device node.
    Add a new entry to match table with compatible set to
    "fsl,mpc5121-fec".

  drivers/net/fs_enet/fs_enet.h
    Include fec_mpc5121.h.
    Change all instances of fec_t to struct fec.

  drivers/net/fs_enet/mac-fec.c
    Change all instances of fec_t to struct fec.
    Any code that is differnet between old 8xx fec
    and 512x fec is ifdef'd with CONFIG_CPM1.
    The file already had CONFIG_CPM1's in it so this isn't
    any more evil than what is already there.

  drivers/net/fs_enet/mii-fec.c
    Change all instances of fec_t to struct fec.
    Add a new entry to match table with compatible set to
    "fsl,mpc5121-fec-mdio".

  include/linux/fs_enet_pd.h
    Add an new field called align_tx_packets to
    struct fs_platform_info which is used in fs_enet_main.c
    as described above.

Signed-off-by: John Rigby <jrigby@freescale.com>
---
 arch/powerpc/boot/dts/mpc5121ads.dts |   24 ++++++++++++-
 drivers/net/fs_enet/Kconfig          |   14 ++++++--
 drivers/net/fs_enet/fec_mpc5121.h    |   64 ++++++++++++++++++++++++++++++++++
 drivers/net/fs_enet/fs_enet-main.c   |   41 +++++++++++++++++++++
 drivers/net/fs_enet/fs_enet.h        |    7 +++-
 drivers/net/fs_enet/mac-fec.c        |   52 ++++++++++++++++++---------
 drivers/net/fs_enet/mii-fec.c        |   15 ++++++--
 include/linux/fs_enet_pd.h           |    2 +
 8 files changed, 193 insertions(+), 26 deletions(-)
 create mode 100644 drivers/net/fs_enet/fec_mpc5121.h

diff --git a/arch/powerpc/boot/dts/mpc5121ads.dts b/arch/powerpc/boot/dts/mpc5121ads.dts
index 94ad7b2..bc9629e 100644
--- a/arch/powerpc/boot/dts/mpc5121ads.dts
+++ b/arch/powerpc/boot/dts/mpc5121ads.dts
@@ -1,5 +1,5 @@
 /*
- * MPC5121E MDS Device Tree Source
+ * MPC5121E ADS Device Tree Source
  *
  * Copyright 2007 Freescale Semiconductor Inc.
  *
@@ -85,6 +85,28 @@
 			reg = <0xc00 0x100>;
 		};
 
+		mdio@2800 {
+			compatible = "fsl,mpc5121-fec-mdio";
+			reg = <0x2800 0x800>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+			phy: ethernet-phy@0 {
+				reg = <1>;
+				device_type = "ethernet-phy";
+			};
+		};
+
+		ethernet@2800 {
+			device_type = "network";
+			compatible = "fsl,mpc5121-fec";
+			reg = <0x2800 0x800>;
+			local-mac-address = [ 00 00 00 00 00 00 ];
+			interrupts = <4 0x8>;
+			interrupt-parent = < &ipic >;
+			phy-handle = < &phy >;
+			fsl,align-tx-packets = <4>;
+		};
+
 		// 512x PSCs are not 52xx PSCs compatible
 		// PSC3 serial port A aka ttyPSC0
 		serial@11300 {
diff --git a/drivers/net/fs_enet/Kconfig b/drivers/net/fs_enet/Kconfig
index 562ea68..fb9abed 100644
--- a/drivers/net/fs_enet/Kconfig
+++ b/drivers/net/fs_enet/Kconfig
@@ -1,9 +1,16 @@
 config FS_ENET
        tristate "Freescale Ethernet Driver"
-       depends on CPM1 || CPM2
+       depends on CPM1 || CPM2 || FS_ENET_MPC5121_FEC
        select MII
        select PHYLIB
 
+config FS_ENET_MPC5121_FEC
+	bool "Freescale MPC512x FEC driver"
+	depends on PPC_MPC512x
+	select FS_ENET
+	select PPC_CPM_NEW_BINDING
+	default y
+
 config FS_ENET_HAS_SCC
 	bool "Chip has an SCC usable for ethernet"
 	depends on FS_ENET && (CPM1 || CPM2)
@@ -16,13 +23,14 @@ 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/fec_mpc5121.h b/drivers/net/fs_enet/fec_mpc5121.h
new file mode 100644
index 0000000..e28c783
--- /dev/null
+++ b/drivers/net/fs_enet/fec_mpc5121.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 FEC_MPC5121_H
+#define FEC_MPC5121_H
+
+#include <linux/types.h>
+#include <asm/cpm.h>
+
+struct fec {
+	u32 fec_reserved0;
+	u32 fec_ievent;		/* Interrupt event reg */
+	u32 fec_imask;		/* Interrupt mask reg */
+	u32 fec_reserved1;
+	u32 fec_r_des_active;	/* Receive descriptor reg */
+	u32 fec_x_des_active;	/* Transmit descriptor reg */
+	u32 fec_reserved2[3];
+	u32 fec_ecntrl;		/* Ethernet control reg */
+	u32 fec_reserved3[6];
+	u32 fec_mii_data;		/* MII manage frame reg */
+	u32 fec_mii_speed;		/* MII speed control reg */
+	u32 fec_reserved4[7];
+	u32 fec_mib_ctrlstat;	/* MIB control/status reg */
+	u32 fec_reserved5[7];
+	u32 fec_r_cntrl;		/* Receive control reg */
+	u32 fec_reserved6[15];
+	u32 fec_x_cntrl;		/* Transmit Control reg */
+	u32 fec_reserved7[7];
+	u32 fec_addr_low;		/* Low 32bits MAC address */
+	u32 fec_addr_high;		/* High 16bits MAC address */
+	u32 fec_opd;		/* Opcode + Pause duration */
+	u32 fec_reserved8[10];
+	u32 fec_hash_table_high;	/* High 32bits hash table */
+	u32 fec_hash_table_low;	/* Low 32bits hash table */
+	u32 fec_grp_hash_table_high;/* High 32bits hash table */
+	u32 fec_grp_hash_table_low;	/* Low 32bits hash table */
+	u32 fec_reserved9[7];
+	u32 fec_x_wmrk;		/* FIFO transmit water mark */
+	u32 fec_reserved10;
+	u32 fec_r_bound;		/* FIFO receive bound reg */
+	u32 fec_r_fstart;		/* FIFO receive start reg */
+	u32 fec_reserved11[11];
+	u32 fec_r_des_start;	/* Receive descriptor ring */
+	u32 fec_x_des_start;	/* Transmit descriptor ring */
+	u32 fec_r_buff_size;	/* Maximum receive buff size */
+	u32 fec_dma_control;	/* DMA Endian and other ctrl */
+};
+
+#endif /* FEC_MPC5121_H */
diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c
index a5baaf5..7c3391c 100644
--- a/drivers/net/fs_enet/fs_enet-main.c
+++ b/drivers/net/fs_enet/fs_enet-main.c
@@ -592,6 +592,37 @@ void fs_cleanup_bds(struct net_device *dev)
 
 /**********************************************************************************/
 
+/*
+ * The 5121 FEC doc says transmit buffers must have 4 byte alignment.
+ * We get alignement from the device tree in case this changes on future
+ * platforms.
+ */
+static struct sk_buff *align_tx_skb(struct net_device *dev, struct sk_buff *skb,
+			    int align)
+{
+	struct sk_buff *skbn;
+
+	if ((((unsigned long)skb->data) & (align-1)) == 0)
+		return skb;
+
+	skbn = dev_alloc_skb(ENET_RX_FRSIZE+align);
+	if (skbn)
+		skb_align(skbn, align);
+
+	if (!skbn) {
+		printk(KERN_WARNING DRV_MODULE_NAME
+			": %s Memory squeeze, dropping tx packet.\n",
+			dev->name);
+		dev_kfree_skb_any(skb);
+		return NULL;
+	}
+
+	skb_copy_from_linear_data(skb, skbn->data, skb->len);
+	skb_put(skbn, skb->len);
+	dev_kfree_skb_any(skb);
+	return skbn;
+}
+
 static int fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
@@ -600,6 +631,8 @@ static int fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	u16 sc;
 	unsigned long flags;
 
+	if (fep->fpi->align_tx_packets)
+		skb = align_tx_skb(dev, skb, fep->fpi->align_tx_packets);
 	spin_lock_irqsave(&fep->tx_lock, flags);
 
 	/*
@@ -1239,6 +1272,10 @@ static int __devinit fs_enet_probe(struct of_device *ofdev,
 		fpi->cp_command = *data;
 	}
 
+	data = of_get_property(ofdev->node, "fsl,align-tx-packets", &len);
+	if (data && len == 4)
+		fpi->align_tx_packets = *data;
+
 	fpi->rx_ring = 32;
 	fpi->tx_ring = 32;
 	fpi->rx_copybreak = 240;
@@ -1366,6 +1403,10 @@ static struct of_device_id fs_enet_match[] = {
 		.compatible = "fsl,pq1-fec-enet",
 		.data = (void *)&fs_fec_ops,
 	},
+	{
+		.compatible = "fsl,mpc5121-fec",
+		.data = (void *)&fs_fec_ops,
+	},
 #endif
 	{}
 };
diff --git a/drivers/net/fs_enet/fs_enet.h b/drivers/net/fs_enet/fs_enet.h
index e05389c..0b600fd 100644
--- a/drivers/net/fs_enet/fs_enet.h
+++ b/drivers/net/fs_enet/fs_enet.h
@@ -10,12 +10,17 @@
 
 #include <linux/fs_enet_pd.h>
 #include <asm/fs_pd.h>
+#ifdef CONFIG_FS_ENET_MPC5121_FEC
+#include "fec_mpc5121.h"
+#endif
 
 #ifdef CONFIG_CPM1
 #include <asm/cpm1.h>
+#endif
 
+#if defined(CONFIG_FS_ENET_HAS_FEC)
 struct fec_info {
-	fec_t __iomem *fecp;
+	struct fec __iomem *fecp;
 	u32 mii_speed;
 };
 #endif
diff --git a/drivers/net/fs_enet/mac-fec.c b/drivers/net/fs_enet/mac-fec.c
index 8a311d1..a48a550 100644
--- a/drivers/net/fs_enet/mac-fec.c
+++ b/drivers/net/fs_enet/mac-fec.c
@@ -42,6 +42,9 @@
 #include <asm/mpc8xx.h>
 #include <asm/cpm1.h>
 #endif
+#ifdef CONFIG_FS_ENET_MPC5121_FEC
+#include "fec_mpc5121.h"
+#endif
 
 #ifdef CONFIG_PPC_CPM_NEW_BINDING
 #include <asm/of_device.h>
@@ -83,7 +86,7 @@
  */
 #define FEC_RESET_DELAY		50
 
-static int whack_reset(fec_t __iomem *fecp)
+static int whack_reset(struct fec __iomem *fecp)
 {
 	int i;
 
@@ -189,7 +192,7 @@ static void cleanup_data(struct net_device *dev)
 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 fec __iomem *fecp = fep->fec.fecp;
 
 	FS(fecp, r_cntrl, FEC_RCNTRL_PROM);
 }
@@ -237,7 +240,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 fec __iomem *fecp = fep->fec.fecp;
 
 	/* if all multi or too many multicasts; just enable all */
 	if ((dev->flags & IFF_ALLMULTI) != 0 ||
@@ -271,7 +274,7 @@ static void restart(struct net_device *dev)
 	u32 cptr;
 #endif
 	struct fs_enet_private *fep = netdev_priv(dev);
-	fec_t __iomem *fecp = fep->fec.fecp;
+	struct fec __iomem *fecp = fep->fec.fecp;
 	const struct fs_platform_info *fpi = fep->fpi;
 	dma_addr_t rx_bd_base_phys, tx_bd_base_phys;
 	int r;
@@ -306,7 +309,9 @@ static void restart(struct net_device *dev)
 	 * Set maximum receive buffer size.
 	 */
 	FW(fecp, r_buff_size, PKT_MAXBLR_SIZE);
+#ifdef CONFIG_CPM1
 	FW(fecp, r_hash, PKT_MAXBUF_SIZE);
+#endif
 
 	/* get physical address */
 	rx_bd_base_phys = fep->ring_mem_addr;
@@ -320,10 +325,17 @@ static void restart(struct net_device *dev)
 
 	fs_init_bds(dev);
 
+#ifdef CONFIG_CPM1
 	/*
 	 * Enable big endian and don't care about SDMA FC.
 	 */
 	FW(fecp, fun_code, 0x78000000);
+#else
+	/*
+	 * Set DATA_BO and DESC_BO and leave the rest unchanged
+	 */
+	FS(fecp, dma_control, 0xc0000000);
+#endif
 
 	/*
 	 * Set MII speed.
@@ -334,11 +346,13 @@ static void restart(struct net_device *dev)
 	 * Clear any outstanding interrupt.
 	 */
 	FW(fecp, ievent, 0xffc0);
+#ifdef CONFIG_CPM1
 #ifndef CONFIG_PPC_MERGE
 	FW(fecp, ivec, (fep->interrupt / 2) << 29);
 #else
 	FW(fecp, ivec, (virq_to_hw(fep->interrupt) / 2) << 29);
 #endif
+#endif
 
 	/*
 	 * adjust to speed (only for DUET & RMII)
@@ -368,9 +382,13 @@ static void restart(struct net_device *dev)
 		out_be32(&immap->im_cpm.cp_cptr, cptr);
 	}
 #endif
-
-
+#ifdef CONFIG_CPM1
 	FW(fecp, r_cntrl, FEC_RCNTRL_MII_MODE);	/* MII enable */
+#else
+	FW(fecp, r_cntrl, PKT_MAXBUF_SIZE<<16);	/* max frame size */
+	FS(fecp, r_cntrl, FEC_RCNTRL_MII_MODE); /* MII enable */
+#endif
+
 	/*
 	 * adjust to duplex mode
 	 */
@@ -399,7 +417,7 @@ 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 fec __iomem *fecp = fep->fec.fecp;
 
 	struct fec_info* feci= fep->phydev->bus->priv;
 
@@ -461,7 +479,7 @@ static void post_free_irq(struct net_device *dev, int irq)
 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 fec __iomem *fecp = fep->fec.fecp;
 
 	FW(fecp, ievent, FEC_NAPI_RX_EVENT_MSK);
 }
@@ -469,7 +487,7 @@ static void napi_clear_rx_event(struct net_device *dev)
 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 fec __iomem *fecp = fep->fec.fecp;
 
 	FS(fecp, imask, FEC_NAPI_RX_EVENT_MSK);
 }
@@ -477,7 +495,7 @@ static void napi_enable_rx(struct net_device *dev)
 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 fec __iomem *fecp = fep->fec.fecp;
 
 	FC(fecp, imask, FEC_NAPI_RX_EVENT_MSK);
 }
@@ -485,7 +503,7 @@ static void napi_disable_rx(struct net_device *dev)
 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 fec __iomem *fecp = fep->fec.fecp;
 
 	FW(fecp, r_des_active, 0x01000000);
 }
@@ -493,7 +511,7 @@ static void rx_bd_done(struct net_device *dev)
 static void tx_kickstart(struct net_device *dev)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
-	fec_t __iomem *fecp = fep->fec.fecp;
+	struct fec __iomem *fecp = fep->fec.fecp;
 
 	FW(fecp, x_des_active, 0x01000000);
 }
@@ -501,7 +519,7 @@ static void tx_kickstart(struct net_device *dev)
 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 fec __iomem *fecp = fep->fec.fecp;
 
 	return FR(fecp, ievent) & FR(fecp, imask);
 }
@@ -509,7 +527,7 @@ static u32 get_int_events(struct net_device *dev)
 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 fec __iomem *fecp = fep->fec.fecp;
 
 	FW(fecp, ievent, int_events);
 }
@@ -524,17 +542,17 @@ static int get_regs(struct net_device *dev, void *p, int *sizep)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
 
-	if (*sizep < sizeof(fec_t))
+	if (*sizep < sizeof(struct fec))
 		return -EINVAL;
 
-	memcpy_fromio(p, fep->fec.fecp, sizeof(fec_t));
+	memcpy_fromio(p, fep->fec.fecp, sizeof(struct fec));
 
 	return 0;
 }
 
 static int get_regs_len(struct net_device *dev)
 {
-	return sizeof(fec_t);
+	return sizeof(struct fec);
 }
 
 static void tx_restart(struct net_device *dev)
diff --git a/drivers/net/fs_enet/mii-fec.c b/drivers/net/fs_enet/mii-fec.c
index f0014cf..2adb0c8 100644
--- a/drivers/net/fs_enet/mii-fec.c
+++ b/drivers/net/fs_enet/mii-fec.c
@@ -70,7 +70,7 @@ static int match_has_phy (struct device *dev, void* data)
 static int fs_mii_fec_init(struct fec_info* fec, struct fs_mii_fec_platform_info *fmpi)
 {
 	struct resource *r;
-	fec_t __iomem *fecp;
+	struct fec __iomem *fecp;
 	char* name = "fsl-cpm-fec";
 
 	/* we need fec in order to be useful */
@@ -85,7 +85,7 @@ static int fs_mii_fec_init(struct fec_info* fec, struct fs_mii_fec_platform_info
 
 	r = platform_get_resource_byname(fec_pdev, IORESOURCE_MEM, "regs");
 
-	fec->fecp = fecp = ioremap(r->start,sizeof(fec_t));
+	fec->fecp = fecp = ioremap(r->start, sizeof(struct fec));
 	fec->mii_speed = fmpi->mii_speed;
 
 	setbits32(&fecp->fec_r_cntrl, FEC_RCNTRL_MII_MODE);	/* MII enable */
@@ -100,7 +100,7 @@ static int fs_mii_fec_init(struct fec_info* fec, struct fs_mii_fec_platform_info
 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;
+	struct fec __iomem *fecp = fec->fecp;
 	int i, ret = -1;
 
 	if ((in_be32(&fecp->fec_r_cntrl) & FEC_RCNTRL_MII_MODE) == 0)
@@ -124,7 +124,7 @@ 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;
+	struct fec __iomem *fecp = fec->fecp;
 	int i;
 
 	/* this must never happen */
@@ -261,9 +261,16 @@ static int fs_enet_mdio_remove(struct of_device *ofdev)
 }
 
 static struct of_device_id fs_enet_mdio_fec_match[] = {
+#ifdef CONFIG_FS_ENET_FEC
 	{
 		.compatible = "fsl,pq1-fec-mdio",
 	},
+#endif
+#ifdef CONFIG_FS_ENET_MPC5121_FEC
+	{
+		.compatible = "fsl,mpc5121-fec-mdio",
+	},
+#endif
 	{},
 };
 
diff --git a/include/linux/fs_enet_pd.h b/include/linux/fs_enet_pd.h
index 9bc045b..4042f23 100644
--- a/include/linux/fs_enet_pd.h
+++ b/include/linux/fs_enet_pd.h
@@ -152,6 +152,8 @@ struct fs_platform_info {
 
 	int use_rmii;		/* use RMII mode 	       */
 	int has_phy;            /* if the network is phy container as well...*/
+
+	int align_tx_packets;	/* align transmit skb's */
 };
 struct fs_mii_fec_platform_info {
 	u32 irq[32];
-- 
1.5.6.rc0.46.gd2b3

Re: [PATCH] [Rev2] MPC5121 FEC support

[Sam Ravnborg]

Hi John - a few nitpicks.

>  
> +config FS_ENET_MPC5121_FEC
> +	bool "Freescale MPC512x FEC driver"
> +	depends on PPC_MPC512x
> +	select FS_ENET
> +	select PPC_CPM_NEW_BINDING
> +	default y

help is missing. I assume you can help the casual reader a bit by being
a bit more verbose. Especially so some random MPC user knows this is not for him.


> +/*
> + * The 5121 FEC doc says transmit buffers must have 4 byte alignment.
> + * We get alignement from the device tree in case this changes on future
> + * platforms.
> + */
> +static struct sk_buff *align_tx_skb(struct net_device *dev, struct sk_buff *skb,
> +			    int align)
> +{
> +	struct sk_buff *skbn;
> +
> +	if ((((unsigned long)skb->data) & (align-1)) == 0)
> +		return skb;
Use one of the available ALIGN macros?
Spaces arounf operators please.

> +
> +	skbn = dev_alloc_skb(ENET_RX_FRSIZE+align);
spaces around operatiors please.

> +	if (!skbn) {
> +		printk(KERN_WARNING DRV_MODULE_NAME
> +			": %s Memory squeeze, dropping tx packet.\n",
> +			dev->name);
> +		dev_kfree_skb_any(skb);
> +		return NULL;

Consider using goto for error handling like we do in many
other places.

> +	data = of_get_property(ofdev->node, "fsl,align-tx-packets", &len);
> +	if (data && len == 4)
> +		fpi->align_tx_packets = *data;
> +
Where did "4" come from. USe a define with a desriptive name.

>  	fpi->rx_ring = 32;
>  	fpi->tx_ring = 32;
Same for "32"
>  	fpi->rx_copybreak = 240;
Same for "240".

> --- a/drivers/net/fs_enet/fs_enet.h
> +++ b/drivers/net/fs_enet/fs_enet.h
> @@ -10,12 +10,17 @@
>  
>  #include <linux/fs_enet_pd.h>
>  #include <asm/fs_pd.h>
> +#ifdef CONFIG_FS_ENET_MPC5121_FEC
> +#include "fec_mpc5121.h"
> +#endif

Which is this include ifdeffed - looks like some wrong concept.

>  
>  #ifdef CONFIG_CPM1
>  #include <asm/cpm1.h>
> +#endif
>  
> +#if defined(CONFIG_FS_ENET_HAS_FEC)
>  struct fec_info {
> -	fec_t __iomem *fecp;
> +	struct fec __iomem *fecp;
>  	u32 mii_speed;

I we have a conversion from fec_t to struct fec
then this is a separate patch in the normal case.

> diff --git a/drivers/net/fs_enet/mac-fec.c b/drivers/net/fs_enet/mac-fec.c
> index 8a311d1..a48a550 100644
> --- a/drivers/net/fs_enet/mac-fec.c
> +++ b/drivers/net/fs_enet/mac-fec.c
> @@ -42,6 +42,9 @@
>  #include <asm/mpc8xx.h>
>  #include <asm/cpm1.h>
>  #endif
> +#ifdef CONFIG_FS_ENET_MPC5121_FEC
> +#include "fec_mpc5121.h"
> +#endif
The ifdeffed include shows up again...

> -static int whack_reset(fec_t __iomem *fecp)
> +static int whack_reset(struct fec __iomem *fecp)
And so does fec_t => struct fec...

>  {
And so does fec_t => struct fec...

>  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 fec __iomem *fecp = fep->fec.fecp;
Again.. (will drop mention it for now. But this is truly
a unrelated change.

The amount of ifdef introduced looks bad..
And try to run the patch through scripts/checkpatch.pl
And try to split it up a bit.

	Sam

Re: [PATCH] [Rev2] MPC5121 FEC support

[Scott Wood]

Sam Ravnborg wrote:
>> +	data = of_get_property(ofdev->node, "fsl,align-tx-packets", &len);
>> +	if (data && len == 4)
>> +		fpi->align_tx_packets = *data;
>> +
> Where did "4" come from. USe a define with a desriptive name.

It's sizeof(u32), i.e. one device tree cell.  This is fairly normal.

>>  	fpi->rx_ring = 32;
>>  	fpi->tx_ring = 32;
> Same for "32"
>>  	fpi->rx_copybreak = 240;
> Same for "240".

They're arbitrary tuning parameters.  How is a #define any more 
descriptive than the field name?

Besides, that's pre-existing, and has nothing to do with this patch.

>> --- a/drivers/net/fs_enet/fs_enet.h
>> +++ b/drivers/net/fs_enet/fs_enet.h
>> @@ -10,12 +10,17 @@
>>  
>>  #include <linux/fs_enet_pd.h>
>>  #include <asm/fs_pd.h>
>> +#ifdef CONFIG_FS_ENET_MPC5121_FEC
>> +#include "fec_mpc5121.h"
>> +#endif
> 
> Which is this include ifdeffed - looks like some wrong concept.

This has already been discussed.  There are two similar but different 
ethernet controllers that are being targeted, and the chips they are a 
part of (8xx and 512x) are already mutually exclusive with respect to 
multiplatform kernels due to core differences.

> The amount of ifdef introduced looks bad..

Yes, it's bad -- but it's a matter of which is the lesser evil, a few 
ifdefs or large amounts of mostly duplicated code.

> And try to run the patch through scripts/checkpatch.pl
> And try to split it up a bit.

Other than the fec_t thing, I don't see any needed splitting...

-Scott

Re: [PATCH] [Rev2] MPC5121 FEC support

[Scott Wood]

John Rigby wrote:
>  config FS_ENET
>         tristate "Freescale Ethernet Driver"
> -       depends on CPM1 || CPM2
> +       depends on CPM1 || CPM2 || FS_ENET_MPC5121_FEC
>         select MII
>         select PHYLIB
>  
> +config FS_ENET_MPC5121_FEC
> +	bool "Freescale MPC512x FEC driver"
> +	depends on PPC_MPC512x
> +	select FS_ENET
> +	select PPC_CPM_NEW_BINDING
> +	default y

No default y.

-Scott

Re: [PATCH] [Rev2] MPC5121 FEC support

[John Rigby]

Thanks Sam,  Scott already addressed most of your issues.  The 
background that you are likely missing and I will add to the patch 
header is that the 5121 has an FEC core with all the same registers as 
8xx except the order of the registers is scrambled.  Fortunately the old 
FEC only exists on CPM1 machines so we can use CONFIG_CPM1 to 
differentiate between the two.


>
> I we have a conversion from fec_t to struct fec
> then this is a separate patch in the normal case.
>   
I'll split the patch into a clean-up patch followed by a 5121 patch.

Re: [PATCH] [Rev2] MPC5121 FEC support

[Sam Ravnborg]

On Tue, Jun 17, 2008 at 02:31:31PM -0500, Scott Wood wrote:
> Sam Ravnborg wrote:
> >>+	data = of_get_property(ofdev->node, "fsl,align-tx-packets", &len);
> >>+	if (data && len == 4)
> >>+		fpi->align_tx_packets = *data;
> >>+
> >Where did "4" come from. USe a define with a desriptive name.
> 
> It's sizeof(u32), i.e. one device tree cell.  This is fairly normal.
Not fpr me at least - but I just review the patch out of context.
If it is sizeof(u32) why not write so?

> 
> >> 	fpi->rx_ring = 32;
> >> 	fpi->tx_ring = 32;
> >Same for "32"
> >> 	fpi->rx_copybreak = 240;
> >Same for "240".
> 
> They're arbitrary tuning parameters.  How is a #define any more 
> descriptive than the field name?
So it is clear they are tuneing parameters.


> 
> Besides, that's pre-existing, and has nothing to do with this patch.
>
OK.
 
> >>--- a/drivers/net/fs_enet/fs_enet.h
> >>+++ b/drivers/net/fs_enet/fs_enet.h
> >>@@ -10,12 +10,17 @@
> >> 
> >> #include <linux/fs_enet_pd.h>
> >> #include <asm/fs_pd.h>
> >>+#ifdef CONFIG_FS_ENET_MPC5121_FEC
> >>+#include "fec_mpc5121.h"
> >>+#endif
> >
> >Which is this include ifdeffed - looks like some wrong concept.
> 
> This has already been discussed.  There are two similar but different 
> ethernet controllers that are being targeted, and the chips they are a 
> part of (8xx and 512x) are already mutually exclusive with respect to 
> multiplatform kernels due to core differences.
OK - had not seen it (or forgot).

> 
> >The amount of ifdef introduced looks bad..
> 
> Yes, it's bad -- but it's a matter of which is the lesser evil, a few 
> ifdefs or large amounts of mostly duplicated code.
> 
> >And try to run the patch through scripts/checkpatch.pl
> >And try to split it up a bit.
> 
> Other than the fec_t thing, I don't see any needed splitting...
It was only the fec_t => struct fec change I had in mind.

	Sam

Re: [PATCH] [Rev2] MPC5121 FEC support

[Sam Ravnborg]

On Tue, Jun 17, 2008 at 02:33:28PM -0500, Scott Wood wrote:
> John Rigby wrote:
> > config FS_ENET
> >        tristate "Freescale Ethernet Driver"
> >-       depends on CPM1 || CPM2
> >+       depends on CPM1 || CPM2 || FS_ENET_MPC5121_FEC
> >        select MII
> >        select PHYLIB
> > 
> >+config FS_ENET_MPC5121_FEC
> >+	bool "Freescale MPC512x FEC driver"
> >+	depends on PPC_MPC512x
> >+	select FS_ENET
> >+	select PPC_CPM_NEW_BINDING
> >+	default y
> 
> No default y.
I by the way do not see the need for the prompt of FS_ENET.
Do you ever want to change it if one of the dependencies
are selected?

	Sam

Re: [PATCH] [Rev2] MPC5121 FEC support

[Sam Ravnborg]

On Tue, Jun 17, 2008 at 01:43:40PM -0600, John Rigby wrote:
> Thanks Sam,  Scott already addressed most of your issues.  The 
> background that you are likely missing and I will add to the patch 
> header is that the 5121 has an FEC core with all the same registers as 
> 8xx except the order of the registers is scrambled.  Fortunately the old 
> FEC only exists on CPM1 machines so we can use CONFIG_CPM1 to 
> differentiate between the two.
> 
> 
> >
> >I we have a conversion from fec_t to struct fec
> >then this is a separate patch in the normal case.
> >  
> I'll split the patch into a clean-up patch followed by a 5121 patch.

Thanks John.

	Sam

Re: [PATCH] [Rev2] MPC5121 FEC support

[Scott Wood]

Sam Ravnborg wrote:
> On Tue, Jun 17, 2008 at 02:33:28PM -0500, Scott Wood wrote:
>> John Rigby wrote:
>>> config FS_ENET
>>>        tristate "Freescale Ethernet Driver"
>>> -       depends on CPM1 || CPM2
>>> +       depends on CPM1 || CPM2 || FS_ENET_MPC5121_FEC
>>>        select MII
>>>        select PHYLIB
>>>
>>> +config FS_ENET_MPC5121_FEC
>>> +	bool "Freescale MPC512x FEC driver"
>>> +	depends on PPC_MPC512x
>>> +	select FS_ENET
>>> +	select PPC_CPM_NEW_BINDING
>>> +	default y
>> No default y.
> I by the way do not see the need for the prompt of FS_ENET.

Agreed, especially since it's overly broad (there is Freescale ethernet 
hardware that this driver doesn't support).  We'd need to change depends 
into selects in the more specific entries.

> Do you ever want to change it if one of the dependencies
> are selected?

Do you mean if CPM1 or CPM2 is selected?  Yes, it's quite possible that 
the user has no need for the CPM ethernet and would rather reclaim the 
memory (especially on CPM1, which has boards as small as 8MiB).

-Scott

Re: [PATCH] [Rev2] MPC5121 FEC support

[Sam Ravnborg]

On Tue, Jun 17, 2008 at 03:03:54PM -0500, Scott Wood wrote:
> Sam Ravnborg wrote:
> >On Tue, Jun 17, 2008 at 02:33:28PM -0500, Scott Wood wrote:
> >>John Rigby wrote:
> >>>config FS_ENET
> >>>       tristate "Freescale Ethernet Driver"
> >>>-       depends on CPM1 || CPM2
> >>>+       depends on CPM1 || CPM2 || FS_ENET_MPC5121_FEC
> >>>       select MII
> >>>       select PHYLIB
> >>>
> >>>+config FS_ENET_MPC5121_FEC
> >>>+	bool "Freescale MPC512x FEC driver"
> >>>+	depends on PPC_MPC512x
> >>>+	select FS_ENET
> >>>+	select PPC_CPM_NEW_BINDING
> >>>+	default y
> >>No default y.
> >I by the way do not see the need for the prompt of FS_ENET.
> 
> Agreed, especially since it's overly broad (there is Freescale ethernet 
> hardware that this driver doesn't support).  We'd need to change depends 
> into selects in the more specific entries.
> 
> >Do you ever want to change it if one of the dependencies
> >are selected?
> 
> Do you mean if CPM1 or CPM2 is selected?  Yes, it's quite possible that 
> the user has no need for the CPM ethernet and would rather reclaim the 
> memory (especially on CPM1, which has boards as small as 8MiB).

I took a closer look now.
And I can see that FS_ENET is indeed a driver and CPM1, CPM2,  FS_ENET_MPC5121_FEC
are all booleans that say that it may make sense to use this driver.

But I was misguided by:
>+config FS_ENET_MPC5121_FEC
>+  select FS_ENET
This is not good.

In general when you select a symbol that has dependencies you are almost
always on the wrong track.
Use a dependency here with a sane default - then people can 
set it to 'n' if they really do not want this driver.

Spreading selects too much is just causing you pain in the long run.

	Sam

Re: [PATCH] [Rev2] MPC5121 FEC support

[Scott Wood]

Sam Ravnborg wrote:
> But I was misguided by:
>> +config FS_ENET_MPC5121_FEC
>> +  select FS_ENET
> This is not good.

Why not, if we get rid of the prompt on FS_ENET?

> In general when you select a symbol that has dependencies you are almost
> always on the wrong track.

The dependencies on FS_ENET could continue to be valuable as 
documentation and verification of the conditions under which the driver 
will build, but the more specific options should make sure that they 
never select it when the dependencies aren't met.

> Use a dependency here with a sane default - then people can 
> set it to 'n' if they really do not want this driver.
> 
> Spreading selects too much is just causing you pain in the long run.

I'm not sure I understand what you're looking for, but I don't see 
anything wrong with something like this (apart from missing help text):

config FS_ENET
	bool
	select MII
	select PHYLIB

config FS_ENET_HAS_SCC
	bool "Freescale CPM SCC Ethernet"
	depends on CPM1 || CPM2
	select FS_ENET

config FS_ENET_HAS_FCC
	bool "Freescale CPM FCC Ethernet"
	depends on CPM2
	select FS_ENET

config FS_ENET_HAS_FEC
	bool "Freescale Fast Ethernet Controller"
	depends on CPM1 || PPC_MPC512x
	select FS_ENET

config FS_ENET_MDIO_FEC
	bool "Freescale FEC MDIO"
	depends on FS_ENET_HAS_FEC

config FS_ENET_MDIO_BITBANG
	bool "Freescale CPM Bitbanged MDIO"
	depends on CPM2

-Scott

Re: [PATCH] [Rev2] MPC5121 FEC support

[Trent Piepho]

On Tue, 17 Jun 2008, Scott Wood wrote:
> Sam Ravnborg wrote:
>>  In general when you select a symbol that has dependencies you are almost
>>  always on the wrong track.
>
> more specific options should make sure that they never select it when the 
> dependencies aren't met.

Sure, in theory that would work, but in practice this ends up being a constant
source of broken builds.

>>  Use a dependency here with a sane default - then people can set it to 'n'
>>  if they really do not want this driver.
>>
>>  Spreading selects too much is just causing you pain in the long run.
>
> I'm not sure I understand what you're looking for, but I don't see anything 
> wrong with something like this (apart from missing help text):
>
> config FS_ENET
> 	 bool
> 	 select MII
> 	 select PHYLIB
>
> config FS_ENET_HAS_SCC
> 	 bool "Freescale CPM SCC Ethernet"
> 	 depends on CPM1 || CPM2
> 	 select FS_ENET

What prevents me from turning on FS_ENET_HAS_SCC without MII or PHYLIB?  Why
is FS_ENET_HAS_SCC a bool, and not tristate?

Re: [PATCH] [Rev2] MPC5121 FEC support

[Sam Ravnborg]

> >
> >I'm not sure I understand what you're looking for, but I don't see 
> >anything wrong with something like this (apart from missing help text):
> >
> >config FS_ENET
> >	 bool
> >	 select MII
> >	 select PHYLIB
> >
> >config FS_ENET_HAS_SCC
> >	 bool "Freescale CPM SCC Ethernet"
> >	 depends on CPM1 || CPM2
> >	 select FS_ENET
> 
> What prevents me from turning on FS_ENET_HAS_SCC without MII or PHYLIB?
Nothing.
But if set FS_ENET_HAS_SCC to 'y' then kconfig will select FS_ENET
which again will select MII and PHYLIB.

	Sam

Re: [PATCH] [Rev2] MPC5121 FEC support

[Scott Wood]

On Tue, Jun 17, 2008 at 04:52:25PM -0700, Trent Piepho wrote:
> Why is FS_ENET_HAS_SCC a bool, and not tristate?

That was an oversight on my part -- they should be tristate.

-Scott