[PATCH 01/12] fs_enet: Use defines to set driver tunables.

[PATCH 01/12] fs_enet: Use defines to set driver tunables.

[Wolfgang Denk]

From: Piotr Ziecik <kosmo@semihalf.com>

Signed-off-by: Piotr Ziecik <kosmo@semihalf.com>
Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: <netdev@vger.kernel.org>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: John Rigby <jcrigby@gmail.com>
---
 drivers/net/fs_enet/fs_enet-main.c |   19 ++++++++++++++-----
 1 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c
index a9cbc31..f996a1a 100644
--- a/drivers/net/fs_enet/fs_enet-main.c
+++ b/drivers/net/fs_enet/fs_enet-main.c
@@ -46,6 +46,15 @@
 
 #include "fs_enet.h"
 
+/* Driver tunables */
+
+#define ENET_RX_RING_SIZE	32
+#define ENET_TX_RING_SIZE	32
+#define ENET_RX_COPYBREAK	240
+
+#define ENET_USE_NAPI		1
+#define ENET_NAPI_WEIGHT	17
+
 /*************************************************/
 
 MODULE_AUTHOR("Pantelis Antoniou <panto@intracom.gr>");
@@ -1057,11 +1066,11 @@ static int __devinit fs_enet_probe(struct of_device *ofdev,
 		fpi->cp_command = *data;
 	}
 
-	fpi->rx_ring = 32;
-	fpi->tx_ring = 32;
-	fpi->rx_copybreak = 240;
-	fpi->use_napi = 1;
-	fpi->napi_weight = 17;
+	fpi->rx_ring = ENET_RX_RING_SIZE;
+	fpi->tx_ring = ENET_TX_RING_SIZE;
+	fpi->rx_copybreak = ENET_RX_COPYBREAK;
+	fpi->use_napi = ENET_USE_NAPI;
+	fpi->napi_weight = ENET_NAPI_WEIGHT;
 
 	ret = find_phy(ofdev->node, fpi);
 	if (ret)
-- 
1.6.0.6

Re: [PATCH 01/12] fs_enet: Use defines to set driver tunables.

[Grant Likely]

On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk <wd@denx.de> wrote:
> From: Piotr Ziecik <kosmo@semihalf.com>
>
> Signed-off-by: Piotr Ziecik <kosmo@semihalf.com>
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: <netdev@vger.kernel.org>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: John Rigby <jcrigby@gmail.com>

Not seeing much benefit to this patch, and the (non-existant) patch
description doesn't really help me here either.

g.

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

Re: [PATCH 01/12] fs_enet: Use defines to set driver tunables.

[Wolfgang Denk]

Dear Grant,

In message <fa686aa40905061335q3a3c4cc7r33d77df655100531@mail.gmail.com> you wrote:
> On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk <wd@denx.de> wrote:
> > From: Piotr Ziecik <kosmo@semihalf.com>
> >
> > Signed-off-by: Piotr Ziecik <kosmo@semihalf.com>
> > Signed-off-by: Wolfgang Denk <wd@denx.de>
> > Cc: <netdev@vger.kernel.org>
> > Cc: Grant Likely <grant.likely@secretlab.ca>
> > Cc: John Rigby <jcrigby@gmail.com>
> 
> Not seeing much benefit to this patch, and the (non-existant) patch
> description doesn't really help me here either.

Please see next patch which then uses the ability to change the
defaults.

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
Program maintenance is an entropy-increasing process,  and  even  its
most skilfull execution only delays the subsidence of the system into
unfixable obsolescence.       - Fred Brooks, "The Mythical Man Month"

Re: [PATCH 01/12] fs_enet: Use defines to set driver tunables.

[Grant Likely]

On Wed, May 6, 2009 at 4:02 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Grant,
>
> In message <fa686aa40905061335q3a3c4cc7r33d77df655100531@mail.gmail.com> you wrote:
>> On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk <wd@denx.de> wrote:
>> > From: Piotr Ziecik <kosmo@semihalf.com>
>> >
>> > Signed-off-by: Piotr Ziecik <kosmo@semihalf.com>
>> > Signed-off-by: Wolfgang Denk <wd@denx.de>
>> > Cc: <netdev@vger.kernel.org>
>> > Cc: Grant Likely <grant.likely@secretlab.ca>
>> > Cc: John Rigby <jcrigby@gmail.com>
>>
>> Not seeing much benefit to this patch, and the (non-existant) patch
>> description doesn't really help me here either.
>
> Please see next patch which then uses the ability to change the
> defaults.

Please state that in the patch description when you repost it.  :-P

g.


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

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

[Wolfgang Denk]

From: John Rigby <jrigby@freescale.com>

Add support for MPC512x to fs_enet driver

    drivers/net/fs_enet/*
        Enable fs_enet driver to work 5121 FEC
        Enable it with CONFIG_FS_ENET_MPC5121_FEC

Signed-off-by: John Rigby <jrigby@freescale.com>
Signed-off-by: Piotr Ziecik <kosmo@semihalf.com>
Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: <netdev@vger.kernel.org>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: John Rigby <jcrigby@gmail.com>
---
 arch/powerpc/include/asm/mpc5121_fec.h |  111 ++++++++++++++++++++++++++++++++
 drivers/net/fs_enet/Kconfig            |   10 ++-
 drivers/net/fs_enet/fs_enet-main.c     |    7 ++
 drivers/net/fs_enet/fs_enet.h          |    6 ++
 drivers/net/fs_enet/mac-fec.c          |   30 ++++++++-
 drivers/net/fs_enet/mii-fec.c          |    7 ++
 6 files changed, 166 insertions(+), 5 deletions(-)
 create mode 100644 arch/powerpc/include/asm/mpc5121_fec.h

diff --git a/arch/powerpc/include/asm/mpc5121_fec.h b/arch/powerpc/include/asm/mpc5121_fec.h
new file mode 100644
index 0000000..6bddf0b
--- /dev/null
+++ b/arch/powerpc/include/asm/mpc5121_fec.h
@@ -0,0 +1,111 @@
+/*
+ * Copyright (C) 2007,2008 Freescale Semiconductor, Inc. All rights reserved.
+ *
+ * Author: John Rigby, <jrigby@freescale.com>
+ *
+ * Modified version of drivers/net/fec.h:
+ *
+ *	fec.h  --  Fast Ethernet Controller for Motorola ColdFire SoC
+ *		   processors.
+ *
+ *	(C) Copyright 2000-2005, Greg Ungerer (gerg@snapgear.com)
+ *	(C) Copyright 2000-2001, Lineo (www.lineo.com)
+ *
+ * This is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#ifndef MPC5121_FEC_H
+#define MPC5121_FEC_H
+
+typedef struct fec {
+	u32 fec_reserved0;
+	u32 fec_ievent;			/* Interrupt event reg */
+	u32 fec_imask;			/* Interrupt mask reg */
+	u32 fec_reserved1;
+	u32 fec_r_des_active;		/* Receive descriptor reg */
+	u32 fec_x_des_active;		/* Transmit descriptor reg */
+	u32 fec_reserved2[3];
+	u32 fec_ecntrl;			/* Ethernet control reg */
+	u32 fec_reserved3[6];
+	u32 fec_mii_data;		/* MII manage frame reg */
+	u32 fec_mii_speed;		/* MII speed control reg */
+	u32 fec_reserved4[7];
+	u32 fec_mib_ctrlstat;		/* MIB control/status reg */
+	u32 fec_reserved5[7];
+	u32 fec_r_cntrl;		/* Receive control reg */
+	u32 fec_reserved6[15];
+	u32 fec_x_cntrl;		/* Transmit Control reg */
+	u32 fec_reserved7[7];
+	u32 fec_addr_low;		/* Low 32bits MAC address */
+	u32 fec_addr_high;		/* High 16bits MAC address */
+	u32 fec_opd;			/* Opcode + Pause duration */
+	u32 fec_reserved8[10];
+	u32 fec_hash_table_high;	/* High 32bits hash table */
+	u32 fec_hash_table_low;		/* Low 32bits hash table */
+	u32 fec_grp_hash_table_high;	/* High 32bits hash table */
+	u32 fec_grp_hash_table_low;	/* Low 32bits hash table */
+	u32 fec_reserved9[7];
+	u32 fec_x_wmrk;			/* FIFO transmit water mark */
+	u32 fec_reserved10;
+	u32 fec_r_bound;		/* FIFO receive bound reg */
+	u32 fec_r_fstart;		/* FIFO receive start reg */
+	u32 fec_reserved11[11];
+	u32 fec_r_des_start;		/* Receive descriptor ring */
+	u32 fec_x_des_start;		/* Transmit descriptor ring */
+	u32 fec_r_buff_size;		/* Maximum receive buff size */
+	u32 fec_reserved12[26];
+	u32 fec_dma_control;		/* DMA Endian and other ctrl */
+} fec_t;
+
+/*
+ *	Define the buffer descriptor structure.
+ */
+typedef struct bufdesc {
+	ushort	cbd_sc;			/* Control and status info */
+	ushort	cbd_datlen;		/* Data length */
+	uint	cbd_bufaddr;		/* Buffer address */
+} cbd_t;
+
+/*
+ *	The following definitions courtesy of commproc.h, which where
+ *	Copyright (c) 1997 Dan Malek (dmalek@jlc.net).
+ */
+#define BD_SC_WRAP		((ushort)0x2000)
+
+/*
+ * Buffer descriptor control/status used by Ethernet receive.
+ */
+#define BD_ENET_RX_EMPTY	((ushort)0x8000)
+#define BD_ENET_RX_WRAP		((ushort)0x2000)
+#define BD_ENET_RX_INTR		((ushort)0x1000)
+#define BD_ENET_RX_LAST		((ushort)0x0800)
+#define BD_ENET_RX_FIRST	((ushort)0x0400)
+#define BD_ENET_RX_MISS		((ushort)0x0100)
+#define BD_ENET_RX_LG		((ushort)0x0020)
+#define BD_ENET_RX_NO		((ushort)0x0010)
+#define BD_ENET_RX_SH		((ushort)0x0008)
+#define BD_ENET_RX_CR		((ushort)0x0004)
+#define BD_ENET_RX_OV		((ushort)0x0002)
+#define BD_ENET_RX_CL		((ushort)0x0001)
+#define BD_ENET_RX_STATS	((ushort)0x013f)	/* All status bits */
+
+/*
+ * Buffer descriptor control/status used by Ethernet transmit.
+ */
+#define BD_ENET_TX_READY	((ushort)0x8000)
+#define BD_ENET_TX_PAD		((ushort)0x4000)
+#define BD_ENET_TX_WRAP		((ushort)0x2000)
+#define BD_ENET_TX_INTR		((ushort)0x1000)
+#define BD_ENET_TX_LAST		((ushort)0x0800)
+#define BD_ENET_TX_TC		((ushort)0x0400)
+#define BD_ENET_TX_DEF		((ushort)0x0200)
+#define BD_ENET_TX_HB		((ushort)0x0100)
+#define BD_ENET_TX_LC		((ushort)0x0080)
+#define BD_ENET_TX_RL		((ushort)0x0040)
+#define BD_ENET_TX_UN		((ushort)0x0002)
+#define BD_ENET_TX_CSL		((ushort)0x0001)
+#define BD_ENET_TX_STATS	((ushort)0x03ff)	/* All status bits */
+
+#endif /* MPC5121_FEC_H */
diff --git a/drivers/net/fs_enet/Kconfig b/drivers/net/fs_enet/Kconfig
index 562ea68..fc073b5 100644
--- a/drivers/net/fs_enet/Kconfig
+++ b/drivers/net/fs_enet/Kconfig
@@ -1,9 +1,13 @@
 config FS_ENET
        tristate "Freescale Ethernet Driver"
-       depends on CPM1 || CPM2
+       depends on CPM1 || CPM2 || PPC_MPC512x
        select MII
        select PHYLIB
 
+config FS_ENET_MPC5121_FEC
+	def_bool y if (FS_ENET && PPC_MPC512x)
+	select FS_ENET_HAS_FEC
+
 config FS_ENET_HAS_SCC
 	bool "Chip has an SCC usable for ethernet"
 	depends on FS_ENET && (CPM1 || CPM2)
@@ -16,13 +20,13 @@ config FS_ENET_HAS_FCC
 
 config FS_ENET_HAS_FEC
 	bool "Chip has an FEC usable for ethernet"
-	depends on FS_ENET && CPM1
+	depends on FS_ENET && (CPM1 || FS_ENET_MPC5121_FEC)
 	select FS_ENET_MDIO_FEC
 	default y
 
 config FS_ENET_MDIO_FEC
 	tristate "MDIO driver for FEC"
-	depends on FS_ENET && CPM1
+	depends on FS_ENET && (CPM1 || FS_ENET_MPC5121_FEC)
 
 config FS_ENET_MDIO_FCC
 	tristate "MDIO driver for FCC"
diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c
index f996a1a..4170d33 100644
--- a/drivers/net/fs_enet/fs_enet-main.c
+++ b/drivers/net/fs_enet/fs_enet-main.c
@@ -1183,11 +1183,18 @@ static struct of_device_id fs_enet_match[] = {
 	},
 #endif
 #ifdef CONFIG_FS_ENET_HAS_FEC
+#ifdef CONFIG_FS_ENET_MPC5121_FEC
+	{
+		.compatible = "fsl,mpc5121-fec",
+		.data = (void *)&fs_fec_ops,
+	},
+#else
 	{
 		.compatible = "fsl,pq1-fec-enet",
 		.data = (void *)&fs_fec_ops,
 	},
 #endif
+#endif
 	{}
 };
 
diff --git a/drivers/net/fs_enet/fs_enet.h b/drivers/net/fs_enet/fs_enet.h
index 85a4bab..5d8258e 100644
--- a/drivers/net/fs_enet/fs_enet.h
+++ b/drivers/net/fs_enet/fs_enet.h
@@ -13,7 +13,13 @@
 
 #ifdef CONFIG_CPM1
 #include <asm/cpm1.h>
+#endif
+
+#ifdef CONFIG_FS_ENET_MPC5121_FEC
+#include <asm/mpc5121_fec.h>
+#endif
 
+#if defined(CONFIG_CPM1) || defined(CONFIG_FS_ENET_MPC5121_FEC)
 struct fec_info {
 	fec_t __iomem *fecp;
 	u32 mii_speed;
diff --git a/drivers/net/fs_enet/mac-fec.c b/drivers/net/fs_enet/mac-fec.c
index 14e5753..b069088 100644
--- a/drivers/net/fs_enet/mac-fec.c
+++ b/drivers/net/fs_enet/mac-fec.c
@@ -44,6 +44,10 @@
 #include <asm/cpm1.h>
 #endif
 
+#ifdef CONFIG_FS_ENET_MPC5121_FEC
+#include <asm/mpc5121_fec.h>
+#endif
+
 #include "fs_enet.h"
 #include "fec.h"
 
@@ -285,7 +289,11 @@ static void restart(struct net_device *dev)
 	 * Set maximum receive buffer size.
 	 */
 	FW(fecp, r_buff_size, PKT_MAXBLR_SIZE);
+#ifdef CONFIG_FS_ENET_MPC5121_FEC
+	FW(fecp, r_cntrl, PKT_MAXBUF_SIZE << 16);
+#else
 	FW(fecp, r_hash, PKT_MAXBUF_SIZE);
+#endif
 
 	/* get physical address */
 	rx_bd_base_phys = fep->ring_mem_addr;
@@ -300,9 +308,14 @@ static void restart(struct net_device *dev)
 	fs_init_bds(dev);
 
 	/*
-	 * Enable big endian and don't care about SDMA FC.
+	 * Enable big endian.
 	 */
+#ifndef CONFIG_FS_ENET_MPC5121_FEC
+	/* Don't care about SDMA FC. */
 	FW(fecp, fun_code, 0x78000000);
+#else
+	FS(fecp, dma_control, 0xC0000000);
+#endif
 
 	/*
 	 * Set MII speed.
@@ -313,7 +326,9 @@ static void restart(struct net_device *dev)
 	 * Clear any outstanding interrupt.
 	 */
 	FW(fecp, ievent, 0xffc0);
+#ifndef CONFIG_FS_ENET_MPC5121_FEC
 	FW(fecp, ivec, (virq_to_hw(fep->interrupt) / 2) << 29);
+#endif
 
 	/*
 	 * adjust to speed (only for DUET & RMII)
@@ -344,8 +359,19 @@ static void restart(struct net_device *dev)
 	}
 #endif
 
+	/*
+	 * Enable MII
+	 */
+#ifdef CONFIG_FS_ENET_MPC5121_FEC
+	/*
+	 * Only set requested bit - do not touch maximum packet size
+	 * configured earlier.
+	 */
+	FS(fecp, r_cntrl, FEC_RCNTRL_MII_MODE);
+#else
+	FW(fecp, r_cntrl, FEC_RCNTRL_MII_MODE);
+#endif
 
-	FW(fecp, r_cntrl, FEC_RCNTRL_MII_MODE);	/* MII enable */
 	/*
 	 * adjust to duplex mode
 	 */
diff --git a/drivers/net/fs_enet/mii-fec.c b/drivers/net/fs_enet/mii-fec.c
index 28077cc..9d8bd97 100644
--- a/drivers/net/fs_enet/mii-fec.c
+++ b/drivers/net/fs_enet/mii-fec.c
@@ -32,6 +32,7 @@
 #include <linux/bitops.h>
 #include <linux/platform_device.h>
 #include <linux/of_platform.h>
+#include <linux/time.h>
 
 #include <asm/pgtable.h>
 #include <asm/irq.h>
@@ -211,9 +212,15 @@ static int fs_enet_mdio_remove(struct of_device *ofdev)
 }
 
 static struct of_device_id fs_enet_mdio_fec_match[] = {
+#ifdef CONFIG_FS_ENET_MPC5121_FEC
+	{
+		.compatible = "fsl,mpc5121-fec-mdio",
+	},
+#else
 	{
 		.compatible = "fsl,pq1-fec-mdio",
 	},
+#endif
 	{},
 };
 
-- 
1.6.0.6

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

[Grant Likely]

On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk <wd@denx.de> wrote:
> From: John Rigby <jrigby@freescale.com>
>
> Add support for MPC512x to fs_enet driver
>
>    drivers/net/fs_enet/*
>        Enable fs_enet driver to work 5121 FEC
>        Enable it with CONFIG_FS_ENET_MPC5121_FEC
>
> Signed-off-by: John Rigby <jrigby@freescale.com>
> Signed-off-by: Piotr Ziecik <kosmo@semihalf.com>
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: <netdev@vger.kernel.org>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: John Rigby <jcrigby@gmail.com>
> ---
>  arch/powerpc/include/asm/mpc5121_fec.h |  111 ++++++++++++++++++++++++++++++++
>  drivers/net/fs_enet/Kconfig            |   10 ++-
>  drivers/net/fs_enet/fs_enet-main.c     |    7 ++
>  drivers/net/fs_enet/fs_enet.h          |    6 ++
>  drivers/net/fs_enet/mac-fec.c          |   30 ++++++++-
>  drivers/net/fs_enet/mii-fec.c          |    7 ++
>  6 files changed, 166 insertions(+), 5 deletions(-)
>  create mode 100644 arch/powerpc/include/asm/mpc5121_fec.h
>
> diff --git a/arch/powerpc/include/asm/mpc5121_fec.h b/arch/powerpc/include/asm/mpc5121_fec.h
> new file mode 100644
> index 0000000..6bddf0b
> --- /dev/null
> +++ b/arch/powerpc/include/asm/mpc5121_fec.h
> @@ -0,0 +1,111 @@
> +/*
> + * Copyright (C) 2007,2008 Freescale Semiconductor, Inc. All rights reserved.
> + *
> + * Author: John Rigby, <jrigby@freescale.com>
> + *
> + * Modified version of drivers/net/fec.h:
> + *
> + *     fec.h  --  Fast Ethernet Controller for Motorola ColdFire SoC
> + *                processors.
> + *
> + *     (C) Copyright 2000-2005, Greg Ungerer (gerg@snapgear.com)
> + *     (C) Copyright 2000-2001, Lineo (www.lineo.com)
> + *
> + * This is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#ifndef MPC5121_FEC_H
> +#define MPC5121_FEC_H
> +
> +typedef struct fec {
> +       u32 fec_reserved0;
> +       u32 fec_ievent;                 /* Interrupt event reg */
> +       u32 fec_imask;                  /* Interrupt mask reg */
> +       u32 fec_reserved1;
> +       u32 fec_r_des_active;           /* Receive descriptor reg */
> +       u32 fec_x_des_active;           /* Transmit descriptor reg */
> +       u32 fec_reserved2[3];
> +       u32 fec_ecntrl;                 /* Ethernet control reg */
> +       u32 fec_reserved3[6];
> +       u32 fec_mii_data;               /* MII manage frame reg */
> +       u32 fec_mii_speed;              /* MII speed control reg */
> +       u32 fec_reserved4[7];
> +       u32 fec_mib_ctrlstat;           /* MIB control/status reg */
> +       u32 fec_reserved5[7];
> +       u32 fec_r_cntrl;                /* Receive control reg */
> +       u32 fec_reserved6[15];
> +       u32 fec_x_cntrl;                /* Transmit Control reg */
> +       u32 fec_reserved7[7];
> +       u32 fec_addr_low;               /* Low 32bits MAC address */
> +       u32 fec_addr_high;              /* High 16bits MAC address */
> +       u32 fec_opd;                    /* Opcode + Pause duration */
> +       u32 fec_reserved8[10];
> +       u32 fec_hash_table_high;        /* High 32bits hash table */
> +       u32 fec_hash_table_low;         /* Low 32bits hash table */
> +       u32 fec_grp_hash_table_high;    /* High 32bits hash table */
> +       u32 fec_grp_hash_table_low;     /* Low 32bits hash table */
> +       u32 fec_reserved9[7];
> +       u32 fec_x_wmrk;                 /* FIFO transmit water mark */
> +       u32 fec_reserved10;
> +       u32 fec_r_bound;                /* FIFO receive bound reg */
> +       u32 fec_r_fstart;               /* FIFO receive start reg */
> +       u32 fec_reserved11[11];
> +       u32 fec_r_des_start;            /* Receive descriptor ring */
> +       u32 fec_x_des_start;            /* Transmit descriptor ring */
> +       u32 fec_r_buff_size;            /* Maximum receive buff size */
> +       u32 fec_reserved12[26];
> +       u32 fec_dma_control;            /* DMA Endian and other ctrl */
> +} fec_t;
> +
> +/*
> + *     Define the buffer descriptor structure.
> + */
> +typedef struct bufdesc {
> +       ushort  cbd_sc;                 /* Control and status info */
> +       ushort  cbd_datlen;             /* Data length */
> +       uint    cbd_bufaddr;            /* Buffer address */
> +} cbd_t;
> +
> +/*
> + *     The following definitions courtesy of commproc.h, which where
> + *     Copyright (c) 1997 Dan Malek (dmalek@jlc.net).
> + */
> +#define BD_SC_WRAP             ((ushort)0x2000)
> +
> +/*
> + * Buffer descriptor control/status used by Ethernet receive.
> + */
> +#define BD_ENET_RX_EMPTY       ((ushort)0x8000)
> +#define BD_ENET_RX_WRAP                ((ushort)0x2000)
> +#define BD_ENET_RX_INTR                ((ushort)0x1000)
> +#define BD_ENET_RX_LAST                ((ushort)0x0800)
> +#define BD_ENET_RX_FIRST       ((ushort)0x0400)
> +#define BD_ENET_RX_MISS                ((ushort)0x0100)
> +#define BD_ENET_RX_LG          ((ushort)0x0020)
> +#define BD_ENET_RX_NO          ((ushort)0x0010)
> +#define BD_ENET_RX_SH          ((ushort)0x0008)
> +#define BD_ENET_RX_CR          ((ushort)0x0004)
> +#define BD_ENET_RX_OV          ((ushort)0x0002)
> +#define BD_ENET_RX_CL          ((ushort)0x0001)
> +#define BD_ENET_RX_STATS       ((ushort)0x013f)        /* All status bits */
> +
> +/*
> + * Buffer descriptor control/status used by Ethernet transmit.
> + */
> +#define BD_ENET_TX_READY       ((ushort)0x8000)
> +#define BD_ENET_TX_PAD         ((ushort)0x4000)
> +#define BD_ENET_TX_WRAP                ((ushort)0x2000)
> +#define BD_ENET_TX_INTR                ((ushort)0x1000)
> +#define BD_ENET_TX_LAST                ((ushort)0x0800)
> +#define BD_ENET_TX_TC          ((ushort)0x0400)
> +#define BD_ENET_TX_DEF         ((ushort)0x0200)
> +#define BD_ENET_TX_HB          ((ushort)0x0100)
> +#define BD_ENET_TX_LC          ((ushort)0x0080)
> +#define BD_ENET_TX_RL          ((ushort)0x0040)
> +#define BD_ENET_TX_UN          ((ushort)0x0002)
> +#define BD_ENET_TX_CSL         ((ushort)0x0001)
> +#define BD_ENET_TX_STATS       ((ushort)0x03ff)        /* All status bits */
> +
> +#endif /* MPC5121_FEC_H */
> diff --git a/drivers/net/fs_enet/Kconfig b/drivers/net/fs_enet/Kconfig
> index 562ea68..fc073b5 100644
> --- a/drivers/net/fs_enet/Kconfig
> +++ b/drivers/net/fs_enet/Kconfig
> @@ -1,9 +1,13 @@
>  config FS_ENET
>        tristate "Freescale Ethernet Driver"
> -       depends on CPM1 || CPM2
> +       depends on CPM1 || CPM2 || PPC_MPC512x
>        select MII
>        select PHYLIB
>
> +config FS_ENET_MPC5121_FEC
> +       def_bool y if (FS_ENET && PPC_MPC512x)
> +       select FS_ENET_HAS_FEC
> +
>  config FS_ENET_HAS_SCC
>        bool "Chip has an SCC usable for ethernet"
>        depends on FS_ENET && (CPM1 || CPM2)
> @@ -16,13 +20,13 @@ config FS_ENET_HAS_FCC
>
>  config FS_ENET_HAS_FEC
>        bool "Chip has an FEC usable for ethernet"
> -       depends on FS_ENET && CPM1
> +       depends on FS_ENET && (CPM1 || FS_ENET_MPC5121_FEC)
>        select FS_ENET_MDIO_FEC
>        default y
>
>  config FS_ENET_MDIO_FEC
>        tristate "MDIO driver for FEC"
> -       depends on FS_ENET && CPM1
> +       depends on FS_ENET && (CPM1 || FS_ENET_MPC5121_FEC)
>
>  config FS_ENET_MDIO_FCC
>        tristate "MDIO driver for FCC"
> diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c
> index f996a1a..4170d33 100644
> --- a/drivers/net/fs_enet/fs_enet-main.c
> +++ b/drivers/net/fs_enet/fs_enet-main.c
> @@ -1183,11 +1183,18 @@ static struct of_device_id fs_enet_match[] = {
>        },
>  #endif
>  #ifdef CONFIG_FS_ENET_HAS_FEC
> +#ifdef CONFIG_FS_ENET_MPC5121_FEC
> +       {
> +               .compatible = "fsl,mpc5121-fec",
> +               .data = (void *)&fs_fec_ops,
> +       },
> +#else
>        {
>                .compatible = "fsl,pq1-fec-enet",
>                .data = (void *)&fs_fec_ops,
>        },
>  #endif
> +#endif

Hmmm.  A lot of these #ifdefs in here.  Does this have a multiplatform
impact?  Not to mention the fact that it's just plain ugly.  :-)

g.

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

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

[Scott Wood]

Grant Likely wrote:
>>  #ifdef CONFIG_FS_ENET_HAS_FEC
>> +#ifdef CONFIG_FS_ENET_MPC5121_FEC
>> +       {
>> +               .compatible = "fsl,mpc5121-fec",
>> +               .data = (void *)&fs_fec_ops,
>> +       },
>> +#else
>>        {
>>                .compatible = "fsl,pq1-fec-enet",
>>                .data = (void *)&fs_fec_ops,
>>        },
>>  #endif
>> +#endif
> 
> Hmmm.  A lot of these #ifdefs in here.  Does this have a multiplatform
> impact?  Not to mention the fact that it's just plain ugly.  :-)

Multiplatform between 512x and 8xx is currently impossible for other 
reasons (512x and CPM2 is another matter).  That said, less ifdefs would 
be nice.

-Scott

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

[Wolfgang Denk]

Dear Grant,

in message <fa686aa40905061333q29c263c8p24856c048e30f4d0@mail.gmail.com> you wrote:
>
...
> > #ifdef CONFIG_FS_ENET_HAS_FEC
> > +#ifdef CONFIG_FS_ENET_MPC5121_FEC
> > +    {
> > +        .compatible = "fsl,mpc5121-fec",
> > +        .data = (void *)&fs_fec_ops,
> > +    },
> > +#else
> >    {
> >        .compatible = "fsl,pq1-fec-enet",
> >        .data = (void *)&fs_fec_ops,
> >    },
> > #endif
> > +#endif
>
> Hmmm.  A lot of these #ifdefs in here.  Does this have a multiplatform
> impact?  Not to mention the fact that it's just plain ugly.  :-)

Agreed that it's ugly, but duplicatio9ng the code would have been even
worse. I don't think that it has multiplatform - at least not as long
as you don't ask for one image that runs on 83xx and on 512x.

Best regards,

Wolfgang Denk

-- 
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
Our business is run on trust.  We trust you will pay in advance.

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

[Grant Likely]

On Wed, May 6, 2009 at 4:01 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Grant,
>
> in message <fa686aa40905061333q29c263c8p24856c048e30f4d0@mail.gmail.com> you wrote:
>>
> ...
>> > #ifdef CONFIG_FS_ENET_HAS_FEC
>> > +#ifdef CONFIG_FS_ENET_MPC5121_FEC
>> > +    {
>> > +        .compatible = "fsl,mpc5121-fec",
>> > +        .data = (void *)&fs_fec_ops,
>> > +    },
>> > +#else
>> >    {
>> >        .compatible = "fsl,pq1-fec-enet",
>> >        .data = (void *)&fs_fec_ops,
>> >    },
>> > #endif
>> > +#endif
>>
>> Hmmm.  A lot of these #ifdefs in here.  Does this have a multiplatform
>> impact?  Not to mention the fact that it's just plain ugly.  :-)
>
> Agreed that it's ugly, but duplicatio9ng the code would have been even
> worse. I don't think that it has multiplatform - at least not as long
> as you don't ask for one image that runs on 83xx and on 512x.

Actually, I *am* asking for one image that runs on 83xx, 52xx and
521x.  I already can and do build and test a single image which boots
on all my 52xx boards, on my 8349 board, and on my G4 Mac.

g.

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

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

[Wolfgang Denk]

Dear Grant,

In message <fa686aa40905061529u11b231afle3b5bb10a2334ad0@mail.gmail.com> you wrote:
>
> > Agreed that it's ugly, but duplicatio9ng the code would have been even
> > worse. I don't think that it has multiplatform - at least not as long
> > as you don't ask for one image that runs on 83xx and on 512x.
> 
> Actually, I *am* asking for one image that runs on 83xx, 52xx and
> 521x.  I already can and do build and test a single image which boots
> on all my 52xx boards, on my 8349 board, and on my G4 Mac.

He. I was afraid you'd say that ;-)

In this case I need a helping hand as I can't figure out how to make
this work. Any suggestions?

Best regards,

Wolfgang Denk

-- 
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
"Here's a fish hangs in the net like a poor man's right in  the  law.
'Twill hardly come out."     - Shakespeare, Pericles, Act II, Scene 1

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

[Grant Likely]

On Wed, May 6, 2009 at 4:41 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Grant,
>
> In message <fa686aa40905061529u11b231afle3b5bb10a2334ad0@mail.gmail.com> you wrote:
>>
>> > Agreed that it's ugly, but duplicatio9ng the code would have been even
>> > worse. I don't think that it has multiplatform - at least not as long
>> > as you don't ask for one image that runs on 83xx and on 512x.
>>
>> Actually, I *am* asking for one image that runs on 83xx, 52xx and
>> 521x.  I already can and do build and test a single image which boots
>> on all my 52xx boards, on my 8349 board, and on my G4 Mac.
>
> He. I was afraid you'd say that ;-)
>
> In this case I need a helping hand as I can't figure out how to make
> this work. Any suggestions?

Hmmm, it is rather ugly because the layout of fec_t is so different.
Easiest solution would be to duplicate the driver in its entirety, but
as you say it results in a lot of duplicate code.  It might be the
lesser of many weevils though.

Second easiest would be to factor out all the common code in the
driver into a separate .c file that gets included by a 'wrapper' .c
file for each config which first includes the correct definition of
fec_t.  This approach solves the duplicate code problem, but it also
fell out of the ugly tree and hit every branch on the way down.

ie: the wrappers would look something like this:

fs_enet_cpm1.c:
#include <asm/cpm1.h>
#include "fs_enet_main.c"

fs_enet_cpm2.c:
#include <asm/cpm2.h>
#include "fs_enet_main.c"

fs_enet_512x.c:
#include <asm/mpc512x.h>
#include "fs_enet_main.c"

And then the makefile would be something along the lines of:
obj-${CONFIG_FS_ENET_CPM1_ += fs_enet_cpm1.o
obj-${CONFIG_FS_ENET_CPM2_ += fs_enet_cpm2.o
obj-${CONFIG_FS_ENET_MPC512x_ += fs_enet_512x.o

A brief look at the driver suggests that access to the fec_t structure
is restricted to the fec-mii.c and mac-fec.c files.  It might be
appropriate to duplicate just those files and do some form of
fs_enet_ops to select between them.

While on the topic, it looks to me like the driver could really use
some refactoring love.  Having multiple definitions of "fec_t" is
confusing and potentially lead to hard to find bugs if the wrong
header gets included by anyone.  I'd like to see all the fec specific
stuff in arch/powerpc/include/asm moved into drivers/net/fs_enet and
renamed to reflect the hardware it is associate with.  Stuff like
"fec_t" is far to generic, not to mention that typedefs are
discouraged now.

Regardless, I feel your pain.  It is not a pretty situation.

g.

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

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

[John Rigby]

[-- Attachment #1.1: Type: text/plain, Size: 3305 bytes --]

Wolfgang,

Welcome to my world and why I gave up on this months ago.

Everyone else,

One thing to consider here is a rewrite with the goal of a new improved fec
driver that would work on both 5121 and the various mx platforms that also
have this same fec core.

Also don't forget that the register map is the same on 512x, mx and coldfire
platforms but not on the other ppc platforms so if you want to one binary to
rule them all you will need to have an offest table or some such.

John

On Thu, May 7, 2009 at 8:09 AM, Grant Likely <grant.likely@secretlab.ca>wrote:

> On Wed, May 6, 2009 at 4:41 PM, Wolfgang Denk <wd@denx.de> wrote:
> > Dear Grant,
> >
> > In message <fa686aa40905061529u11b231afle3b5bb10a2334ad0@mail.gmail.com>
> you wrote:
> >>
> >> > Agreed that it's ugly, but duplicatio9ng the code would have been even
> >> > worse. I don't think that it has multiplatform - at least not as long
> >> > as you don't ask for one image that runs on 83xx and on 512x.
> >>
> >> Actually, I *am* asking for one image that runs on 83xx, 52xx and
> >> 521x.  I already can and do build and test a single image which boots
> >> on all my 52xx boards, on my 8349 board, and on my G4 Mac.
> >
> > He. I was afraid you'd say that ;-)
> >
> > In this case I need a helping hand as I can't figure out how to make
> > this work. Any suggestions?
>
> Hmmm, it is rather ugly because the layout of fec_t is so different.
> Easiest solution would be to duplicate the driver in its entirety, but
> as you say it results in a lot of duplicate code.  It might be the
> lesser of many weevils though.
>
> Second easiest would be to factor out all the common code in the
> driver into a separate .c file that gets included by a 'wrapper' .c
> file for each config which first includes the correct definition of
> fec_t.  This approach solves the duplicate code problem, but it also
> fell out of the ugly tree and hit every branch on the way down.
>
> ie: the wrappers would look something like this:
>
> fs_enet_cpm1.c:
> #include <asm/cpm1.h>
> #include "fs_enet_main.c"
>
> fs_enet_cpm2.c:
> #include <asm/cpm2.h>
> #include "fs_enet_main.c"
>
> fs_enet_512x.c:
> #include <asm/mpc512x.h>
> #include "fs_enet_main.c"
>
> And then the makefile would be something along the lines of:
> obj-${CONFIG_FS_ENET_CPM1_ += fs_enet_cpm1.o
> obj-${CONFIG_FS_ENET_CPM2_ += fs_enet_cpm2.o
> obj-${CONFIG_FS_ENET_MPC512x_ += fs_enet_512x.o
>
> A brief look at the driver suggests that access to the fec_t structure
> is restricted to the fec-mii.c and mac-fec.c files.  It might be
> appropriate to duplicate just those files and do some form of
> fs_enet_ops to select between them.
>
> While on the topic, it looks to me like the driver could really use
> some refactoring love.  Having multiple definitions of "fec_t" is
> confusing and potentially lead to hard to find bugs if the wrong
> header gets included by anyone.  I'd like to see all the fec specific
> stuff in arch/powerpc/include/asm moved into drivers/net/fs_enet and
> renamed to reflect the hardware it is associate with.  Stuff like
> "fec_t" is far to generic, not to mention that typedefs are
> discouraged now.
>
> Regardless, I feel your pain.  It is not a pretty situation.
>
> g.
>
> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
>

[-- Attachment #1.2: Type: text/html, Size: 4196 bytes --]

[-- Attachment #2: Type: text/plain, Size: 146 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

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

[David Miller]

From: John Rigby <jcrigby@gmail.com>
Date: Thu, 7 May 2009 20:02:53 -0600

> Also don't forget that the register map is the same on 512x, mx and
> coldfire platforms but not on the other ppc platforms so if you want
> to one binary to rule them all you will need to have an offest table
> or some such.

I would suggest using ->read_reg() ->write_reg() methods for abstracting
this.  That's how we handle all of the different way ESP scsi chips
have their registers wired up.

I/O register reads take hundreds, if not thousands of CPU cycles so,
relatively speaking, the indirection costs absolutely nothing.

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

[Kumar Gala]

On May 6, 2009, at 5:01 PM, Wolfgang Denk wrote:

> Dear Grant,
>
> in message <fa686aa40905061333q29c263c8p24856c048e30f4d0@mail.gmail.com 
> > you wrote:
>>
> ...
>>> #ifdef CONFIG_FS_ENET_HAS_FEC
>>> +#ifdef CONFIG_FS_ENET_MPC5121_FEC
>>> +    {
>>> +        .compatible = "fsl,mpc5121-fec",
>>> +        .data = (void *)&fs_fec_ops,
>>> +    },
>>> +#else
>>>   {
>>>       .compatible = "fsl,pq1-fec-enet",
>>>       .data = (void *)&fs_fec_ops,
>>>   },
>>> #endif
>>> +#endif
>>
>> Hmmm.  A lot of these #ifdefs in here.  Does this have a  
>> multiplatform
>> impact?  Not to mention the fact that it's just plain ugly.  :-)
>
> Agreed that it's ugly, but duplicatio9ng the code would have been even
> worse. I don't think that it has multiplatform - at least not as long
> as you don't ask for one image that runs on 83xx and on 512x.

We do ask for that.  The fedora ppc6xx kernel should work on any 6xx/ 
7xx/7xxx based core chip.  We should NOT be breaking such capability  
in drivers.

- k

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

[David Miller]

Would you be offended if I tell you that this is a horrible patch
submission?

Your introductory email indicates 16 patches, yet the series indicates
there were 12, and that intro email is only posted to the linuxppc-dev
list for people to read.  Nobody on netdev nor other interested
parties that get CC:'d along the line are able to read what this patch
series is about.

Since only some patches are CC:'d to netdev I have no idea if I should
apply these or they are dependent on some other patches in the series
that you didn't send here to netdev.

What a mess... how can any maintainer figure out what patch is what,
and what tree you expect these patches to even get applied to?

I'm definitely sending all of the copies I have received to /dev/null,
you need to submit this work properly.

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

[Wolfgang Denk]

Dear David,

In message <20090506.134003.261424694.davem@davemloft.net> you wrote:
> 
> Would you be offended if I tell you that this is a horrible patch
> submission?
> 
> Your introductory email indicates 16 patches, yet the series indicates
> there were 12, and that intro email is only posted to the linuxppc-dev
> list for people to read.  Nobody on netdev nor other interested
> parties that get CC:'d along the line are able to read what this patch
> series is about.

Of course I am not offended, as you are absolutely right with your
comment. I'm angry with myself, as I actually intended to do exactly
what you find missing, but then forgot to write the file in the
editor :-(

> I'm definitely sending all of the copies I have received to /dev/null,
> you need to submit this work properly.

OK. What exactly should I do?

Best regards,

Wolfgang Denk

-- 
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
If I have seen further it is by standing on the shoulders of  giants.
              - Isaac Newton, Letter to Robert Hooke, 5 February 1676

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

[Scott Wood]

Wolfgang Denk wrote:
> +/*
> + *	Define the buffer descriptor structure.
> + */
> +typedef struct bufdesc {
> +	ushort	cbd_sc;			/* Control and status info */
> +	ushort	cbd_datlen;		/* Data length */
> +	uint	cbd_bufaddr;		/* Buffer address */
> +} cbd_t;
> +
> +/*
> + *	The following definitions courtesy of commproc.h, which where
> + *	Copyright (c) 1997 Dan Malek (dmalek@jlc.net).
> + */
> +#define BD_SC_WRAP		((ushort)0x2000)
> +
> +/*
> + * Buffer descriptor control/status used by Ethernet receive.
> + */
> +#define BD_ENET_RX_EMPTY	((ushort)0x8000)
> +#define BD_ENET_RX_WRAP		((ushort)0x2000)
> +#define BD_ENET_RX_INTR		((ushort)0x1000)
> +#define BD_ENET_RX_LAST		((ushort)0x0800)
> +#define BD_ENET_RX_FIRST	((ushort)0x0400)
> +#define BD_ENET_RX_MISS		((ushort)0x0100)
> +#define BD_ENET_RX_LG		((ushort)0x0020)
> +#define BD_ENET_RX_NO		((ushort)0x0010)
> +#define BD_ENET_RX_SH		((ushort)0x0008)
> +#define BD_ENET_RX_CR		((ushort)0x0004)
> +#define BD_ENET_RX_OV		((ushort)0x0002)
> +#define BD_ENET_RX_CL		((ushort)0x0001)
> +#define BD_ENET_RX_STATS	((ushort)0x013f)	/* All status bits */
> +
> +/*
> + * Buffer descriptor control/status used by Ethernet transmit.
> + */
> +#define BD_ENET_TX_READY	((ushort)0x8000)
> +#define BD_ENET_TX_PAD		((ushort)0x4000)
> +#define BD_ENET_TX_WRAP		((ushort)0x2000)
> +#define BD_ENET_TX_INTR		((ushort)0x1000)
> +#define BD_ENET_TX_LAST		((ushort)0x0800)
> +#define BD_ENET_TX_TC		((ushort)0x0400)
> +#define BD_ENET_TX_DEF		((ushort)0x0200)
> +#define BD_ENET_TX_HB		((ushort)0x0100)
> +#define BD_ENET_TX_LC		((ushort)0x0080)
> +#define BD_ENET_TX_RL		((ushort)0x0040)
> +#define BD_ENET_TX_UN		((ushort)0x0002)
> +#define BD_ENET_TX_CSL		((ushort)0x0001)
> +#define BD_ENET_TX_STATS	((ushort)0x03ff)	/* All status bits */

All of the above is duplicative (with even the same names) of stuff in 
asm/cpm.h.  Beyond just the duplication, what happens if both CPM2 and 
512x are enabled in the same kernel?

-Scott

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

[Wolfgang Denk]

Dear Scott,

in message <4A01F602.2010601@freescale.com> you wrote:
>
> All of the above is duplicative (with even the same names) of stuff in 
> asm/cpm.h.  Beyond just the duplication, what happens if both CPM2 and 

OK, I can try to reuse the definitions from that file.

> 512x are enabled in the same kernel?

Hm... both architectures look sufficiently different to me that I
don't see sense in trying such a thing. Do you think that needs to be
supported?

Best regards,

Wolfgang Denk

-- 
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
"You ain't experienced..." "Well, nor are you." "That's true. But the
point is ... the point is ... the point is we've been not experienced
for a lot longer than you. We've got  a  lot  of  experience  of  not
having any experience."           - Terry Pratchett, _Witches Abroad_

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

[Grant Likely]

On Wed, May 6, 2009 at 4:09 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Scott,
>
> in message <4A01F602.2010601@freescale.com> you wrote:
>>
>> All of the above is duplicative (with even the same names) of stuff in
>> asm/cpm.h.  Beyond just the duplication, what happens if both CPM2 and
>
> OK, I can try to reuse the definitions from that file.
>
>> 512x are enabled in the same kernel?
>
> Hm... both architectures look sufficiently different to me that I
> don't see sense in trying such a thing. Do you think that needs to be
> supported?

Yes!  :-)  It's not hard to do and it keeps the driver cleaner
(IMNSHO).  I don't think it is quite possible at the moment due to
cache coherency issues, but with Becky's recently merged dma ops
changes it should be fixable.

Cheers,
g.

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

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

[Piotr Zięcik]

Thursday 07 May 2009 00:39:25 Grant Likely napisał(a):
> >> 512x are enabled in the same kernel?
> >
> > Hm... both architectures look sufficiently different to me that I
> > don't see sense in trying such a thing. Do you think that needs to be
> > supported?
>
> Yes!  :-)  It's not hard to do and it keeps the driver cleaner
> (IMNSHO).  I don't think it is quite possible at the moment due to
> cache coherency issues, but with Becky's recently merged dma ops
> changes it should be fixable.

Could you elaborate on the cache coherency issues in MPC5121
FEC context? Especially how these issues are related to the driver
binary compatibility. 

MPC5121 support was added to drivers/net/fs_enet. MPC52xx uses
drivers/net/fec_mpc52xx.c. Do you think that creating one universal
driver from these two is now possible? You said that it should be easy,
however you also said that cache coherency issues makes this imposible.

-- 
Best Regards.
Piotr Ziecik

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

[Grant Likely]

2009/5/14 Piotr Zięcik <kosmo@semihalf.com>:
> Thursday 07 May 2009 00:39:25 Grant Likely napisał(a):
>> >> 512x are enabled in the same kernel?
>> >
>> > Hm... both architectures look sufficiently different to me that I
>> > don't see sense in trying such a thing. Do you think that needs to be
>> > supported?
>>
>> Yes!  :-)  It's not hard to do and it keeps the driver cleaner
>> (IMNSHO).  I don't think it is quite possible at the moment due to
>> cache coherency issues, but with Becky's recently merged dma ops
>> changes it should be fixable.
>
> Could you elaborate on the cache coherency issues in MPC5121
> FEC context? Especially how these issues are related to the driver
> binary compatibility.
>
> MPC5121 support was added to drivers/net/fs_enet. MPC52xx uses
> drivers/net/fec_mpc52xx.c. Do you think that creating one universal
> driver from these two is now possible? You said that it should be easy,
> however you also said that cache coherency issues makes this imposible.

Not impossible.  Hard.

The mpc5200 is a cache coherent part.  Bestcomm memory accesses are
noticed (snooped) by the cache, and it will flush out cache lines
appropriately to maintain coherency.

The mpc5121 bus design is non-coherent.  The e300 core is essentially
the same as on the 5200, and the core can do snooping, but the
multiport memory controller on the 5121 doesn't support bus snooping
so the cache is not automatically maintained in a consistent state.
On this part Linux needs to manually flush the relevant cache lines
before initiating DMA transfers, and invalidiate them afterwards.

All of this doesn't actually affect the driver code at all.  It's all
handled by the kernel and the DMA apis.  What it does affect is
multiplatform kernels.  The DMA behaviour is set at compile time, not
run time, depending on the setting of CONFIG_NON_COHERENT_CACHE (see
arch/powerpc/platforms/Kconfig.cputype).  A kernel which has it on
won't run properly on a platform which has it off, and visa-versa.

So, while the MPC5200 and MPC5121 (and MPC83xx) all have the same
core, it isn't currently possible to build a single kernel which will
boot on both the MPC5200 and the MPC5121.  The solution (i think) is
to kill CONFIG_NON_COHERENT_CACHE and have the platform setup code set
the appropriate dmaops at platform init time.

Cheers,
g.

>
> --
> Best Regards.
> Piotr Ziecik
>



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

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

[Benjamin Herrenschmidt]

On Thu, 2009-05-14 at 08:00 -0600, Grant Likely wrote:
> 
> All of this doesn't actually affect the driver code at all.  It's all
> handled by the kernel and the DMA apis.  What it does affect is
> multiplatform kernels.  The DMA behaviour is set at compile time, not
> run time, depending on the setting of CONFIG_NON_COHERENT_CACHE (see
> arch/powerpc/platforms/Kconfig.cputype).  A kernel which has it on
> won't run properly on a platform which has it off, and visa-versa.

We are close to the point where we can make this a runtime option
though, by just having a different set of dma_ops hooked in.

Cheers,
Ben.

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

[Piotr Zięcik]

Tuesday 19 May 2009 00:17:31 Benjamin Herrenschmidt wrote:
>
> We are close to the point where we can make this a runtime option
> though, by just having a different set of dma_ops hooked in.
>

Is somebody currently working on it? If yes, where we can see
code ?

-- 
Best Regards.
Piotr Zięcik

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

[Benjamin Herrenschmidt]

On Tue, 2009-05-19 at 13:26 +0200, Piotr Zięcik wrote:
> Tuesday 19 May 2009 00:17:31 Benjamin Herrenschmidt wrote:
> >
> > We are close to the point where we can make this a runtime option
> > though, by just having a different set of dma_ops hooked in.
> >
> 
> Is somebody currently working on it? If yes, where we can see
> code ?

Well, the current upstream code has the dma ops support. Becky is
massaging that more for swiotlb support which has been submitted to the
linuxppc-dev list. We haven't yet added runtime support rather than
compile time for non-cache coherent platforms, but it's becoming
reasonably easy to add now (though still needs to be done).

Cheers,
Ben

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

[Piotr Zięcik]

Thursday 14 May 2009 16:00:33 Grant Likely wrote:
> > MPC5121 support was added to drivers/net/fs_enet. MPC52xx uses
> > drivers/net/fec_mpc52xx.c. Do you think that creating one universal
> > driver from these two is now possible? You said that it should be easy,
> > however you also said that cache coherency issues makes this imposible.
>
> Not impossible.  Hard.

I thought a bit more about merging FEC drivers and I see one problem more.
Driver fs_enet works with FEC's with own DMA engine and fec_mpc52xx.c uses 
BestComm. Integration of these two drivers will need a DMA abstraction layer 
to keep everything clean. Unfortuanetly BestComm driver does not provide any 
abstraction - it only exports set of functions to deal with specific 
hardware: FEC and PATA.

More #ifdef's will be needed to remove linking with BestComm driver if kernel 
will be compiled without 52xx support and resulting code will not be much 
better than existing one. Especially that new DMA abstraction layer probably 
will be quite complex.

-- 
Best Regards.
Piotr Ziecik

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

[Grant Likely]

2009/5/21 Piotr Zięcik <kosmo@semihalf.com>:
> Thursday 14 May 2009 16:00:33 Grant Likely wrote:
>> > MPC5121 support was added to drivers/net/fs_enet. MPC52xx uses
>> > drivers/net/fec_mpc52xx.c. Do you think that creating one universal
>> > driver from these two is now possible? You said that it should be easy,
>> > however you also said that cache coherency issues makes this imposible.
>>
>> Not impossible.  Hard.
>
> I thought a bit more about merging FEC drivers and I see one problem more.
> Driver fs_enet works with FEC's with own DMA engine and fec_mpc52xx.c uses
> BestComm. Integration of these two drivers will need a DMA abstraction layer
> to keep everything clean. Unfortuanetly BestComm driver does not provide any
> abstraction - it only exports set of functions to deal with specific
> hardware: FEC and PATA.
>
> More #ifdef's will be needed to remove linking with BestComm driver if kernel
> will be compiled without 52xx support and resulting code will not be much
> better than existing one. Especially that new DMA abstraction layer probably
> will be quite complex.

If it looks too ugly, then just fork the driver.

g.

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

[PATCH 03/12] fs_enet: Add FEC TX Alignment workaround for MPC5121.

[Wolfgang Denk]

From: John Rigby <jrigby@freescale.com>

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 <jrigby@freescale.com>
Signed-off-by: Piotr Ziecik <kosmo@semihalf.com>
Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: <netdev@vger.kernel.org>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: John Rigby <jcrigby@gmail.com>
---
 drivers/net/fs_enet/Kconfig        |    3 ++
 drivers/net/fs_enet/fs_enet-main.c |   41 ++++++++++++++++++++++++++++++++++++
 drivers/net/fs_enet/mac-fec.c      |    2 +-
 3 files changed, 45 insertions(+), 1 deletions(-)

diff --git a/drivers/net/fs_enet/Kconfig b/drivers/net/fs_enet/Kconfig
index fc073b5..79afd07 100644
--- a/drivers/net/fs_enet/Kconfig
+++ b/drivers/net/fs_enet/Kconfig
@@ -8,6 +8,9 @@ config FS_ENET_MPC5121_FEC
 	def_bool y if (FS_ENET && PPC_MPC512x)
 	select FS_ENET_HAS_FEC
 
+config FS_ENET_FEC_TX_ALIGN_WORKAROUND
+	def_bool y if FS_ENET_MPC5121_FEC
+
 config FS_ENET_HAS_SCC
 	bool "Chip has an SCC usable for ethernet"
 	depends on FS_ENET && (CPM1 || CPM2)
diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c
index 4170d33..c83ffc3 100644
--- a/drivers/net/fs_enet/fs_enet-main.c
+++ b/drivers/net/fs_enet/fs_enet-main.c
@@ -594,6 +594,37 @@ void fs_cleanup_bds(struct net_device *dev)
 
 /**********************************************************************************/
 
+#ifdef CONFIG_FS_ENET_FEC_TX_ALIGN_WORKAROUND
+static struct sk_buff *tx_skb_align_workaround(struct net_device *dev,
+							struct sk_buff *skb)
+{
+	struct sk_buff *new_skb;
+
+	/* Alloc new skb */
+	new_skb = dev_alloc_skb(ENET_RX_FRSIZE + 32);
+	if (!new_skb) {
+		printk(KERN_WARNING DRV_MODULE_NAME
+				": %s Memory squeeze, dropping tx packet.\n",
+								dev->name);
+		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;
+}
+#else
+#define tx_skb_align_workaround(dev, skb) (skb)
+#endif
+
 static int fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
@@ -602,6 +633,16 @@ static int fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	u16 sc;
 	unsigned long flags;
 
+	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);
 
 	/*
diff --git a/drivers/net/fs_enet/mac-fec.c b/drivers/net/fs_enet/mac-fec.c
index b069088..3e86498 100644
--- a/drivers/net/fs_enet/mac-fec.c
+++ b/drivers/net/fs_enet/mac-fec.c
@@ -311,7 +311,7 @@ static void restart(struct net_device *dev)
 	 * Enable big endian.
 	 */
 #ifndef CONFIG_FS_ENET_MPC5121_FEC
-	/* Don't care about SDMA FC. */
+	/* Don't care about SDMA Function Code. */
 	FW(fecp, fun_code, 0x78000000);
 #else
 	FS(fecp, dma_control, 0xC0000000);
-- 
1.6.0.6

Re: [PATCH 03/12] fs_enet: Add FEC TX Alignment workaround for MPC5121.

[Grant Likely]

On Wed, May 6, 2009 at 2:15 PM, Wolfgang Denk <wd@denx.de> wrote:
> From: John Rigby <jrigby@freescale.com>
>
> 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.

OUCH!

> diff --git a/drivers/net/fs_enet/fs_enet-main.c b/drivers/net/fs_enet/fs_enet-main.c
> index 4170d33..c83ffc3 100644
> --- a/drivers/net/fs_enet/fs_enet-main.c
> +++ b/drivers/net/fs_enet/fs_enet-main.c
> @@ -594,6 +594,37 @@ void fs_cleanup_bds(struct net_device *dev)
>
>  /**********************************************************************************/
>
> +#ifdef CONFIG_FS_ENET_FEC_TX_ALIGN_WORKAROUND
> +static struct sk_buff *tx_skb_align_workaround(struct net_device *dev,
> +                                                       struct sk_buff *skb)
> +{
> +       struct sk_buff *new_skb;
> +
> +       /* Alloc new skb */
> +       new_skb = dev_alloc_skb(ENET_RX_FRSIZE + 32);
> +       if (!new_skb) {
> +               printk(KERN_WARNING DRV_MODULE_NAME
> +                               ": %s Memory squeeze, dropping tx packet.\n",
> +                                                               dev->name);
> +               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;
> +}
> +#else
> +#define tx_skb_align_workaround(dev, skb) (skb)
> +#endif

Another use of #ifdef blocks.  What is the multiplatform impact?

g.

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

Re: [PATCH 03/12] fs_enet: Add FEC TX Alignment workaround for MPC5121.

[Wolfgang Denk]

Dear Grant Likely,

In message <fa686aa40905061337w6aa82f5aj787618ba108e528f@mail.gmail.com> you 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. 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.
>
> OUCH!

Yes :-(

> > +#else
> > +#define tx_skb_align_workaround(dev, skb) (skb)
> > +#endif
>
> Another use of #ifdef blocks.  What is the multiplatform impact?

Hm... Can you recommend a better way to solve the problem? Suggestions
are welcome.

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
You don't have to worry about me. I might have been born yesterday...
but I stayed up all night.

Re: [PATCH 03/12] fs_enet: Add FEC TX Alignment workaround for MPC5121.

[Grant Likely]

On Wed, May 6, 2009 at 4:12 PM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Grant Likely,
>
> In message <fa686aa40905061337w6aa82f5aj787618ba108e528f@mail.gmail.com> you 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. 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.
>>
>> OUCH!
>
> Yes :-(
>
>> > +#else
>> > +#define tx_skb_align_workaround(dev, skb) (skb)
>> > +#endif
>>
>> Another use of #ifdef blocks.  What is the multiplatform impact?
>
> Hm... Can you recommend a better way to solve the problem? Suggestions
> are welcome.

I'd rather see a runtime selectable workaround.  ie. enable it based
on the compatible property.

g.

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

Re: [PATCH 03/12] fs_enet: Add FEC TX Alignment workaround for MPC5121.

[John Rigby]

[-- Attachment #1.1: Type: text/plain, Size: 1766 bytes --]

I was having deja-vu with this and realized that I have fixed at least some
of the objections to this patch.

Wolfgang you may want to look at the patch in my 5121 git tree here:

http://git.denx.de/?p=linux-mpc512x.git;a=commit;h=2950be3be42af7449941c3340998c27ef918f10f

It does runtime tx packet alignment  It also has fewer ifdefs and trys to
share more code.  It also has a header that explains everything including
that fact that there is not a runtime conflict sine the only other ppc that
has fec is 8xx which is not in the same family.

On Wed, May 6, 2009 at 4:42 PM, Grant Likely <grant.likely@secretlab.ca>wrote:

> On Wed, May 6, 2009 at 4:12 PM, Wolfgang Denk <wd@denx.de> wrote:
> > Dear Grant Likely,
> >
> > In message <fa686aa40905061337w6aa82f5aj787618ba108e528f@mail.gmail.com>
> you 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. 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.
> >>
> >> OUCH!
> >
> > Yes :-(
> >
> >> > +#else
> >> > +#define tx_skb_align_workaround(dev, skb) (skb)
> >> > +#endif
> >>
> >> Another use of #ifdef blocks.  What is the multiplatform impact?
> >
> > Hm... Can you recommend a better way to solve the problem? Suggestions
> > are welcome.
>
> I'd rather see a runtime selectable workaround.  ie. enable it based
> on the compatible property.
>
> g.
>
> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
>

[-- Attachment #1.2: Type: text/html, Size: 2629 bytes --]

[-- Attachment #2: Type: text/plain, Size: 146 bytes --]

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev