Hi Baruch,

On Tue, Jan 04, 2011 at 11:59:16AM +0200, Baruch Siach wrote:

Hi Shawn,

On Tue, Jan 04, 2011 at 05:24:11PM +0800, Shawn Guo wrote:

quoted

This patch is to add mx28 dual fec support. Here are some key notes
for mx28 fec controller.

 - mx28 fec design made an assumption that it runs on a
   big-endian system, which is incorrect. As the result, the
   driver has to swap every frame going to and coming from
   the controller.
 - external phys can only be configured by fec0, which means
   fec1 can not work independently and both phys need to be
   configured by mii_bus attached on fec0.
 - mx28 fec reset will get mac address registers reset too.
 - MII/RMII mode and 10M/100M speed are configured differently
   from i.mx/mxs fec controller.
 - ETHER_EN bit must be set to get interrupt work.

Signed-off-by: Shawn Guo <redacted>
---
Changes for v2:
 - Use module parameter fec.macaddr over new kernel command line
   fec_mac to pass mac address

Since you introduce this new kernel command line parameter in patch #3 of this 
series, why not just make it right in the first place? This should make both 
patches smaller and easier for review.

quoted

 - Update comment in fec_get_mac() to stop using confusing word
   "default"
 - Fix copyright breakage in fec.h

Ditto.

Sorry for rushing to send the patch set out. All these updates
should happen on patch #3 than #5.  This is a serious problem,
and I will fix it soon and resend as v3.

quoted

 drivers/net/Kconfig |    7 ++-
 drivers/net/fec.c   |  139 ++++++++++++++++++++++++++++++++++++++++----------
 drivers/net/fec.h   |    5 +-
 include/linux/fec.h |    3 +-
 4 files changed, 120 insertions(+), 34 deletions(-)

[snip]

quoted

diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index f147508..b2b3e37 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c

@@ -17,6 +17,8 @@
  *
  * Bug fixes and cleanup by Philippe De Muyter (phdm@macqel.be)
  * Copyright (c) 2004-2006 Macq Electronique SA.
+ *
+ * Copyright (C) 2010 Freescale Semiconductor, Inc.
  */
 
 #include <linux/module.h>

@@ -45,21 +47,34 @@
 
 #include <asm/cacheflush.h>
 
-#ifndef CONFIG_ARCH_MXC
+#if !defined(CONFIG_ARCH_MXC) && !defined(CONFIG_SOC_IMX28)
 #include <asm/coldfire.h>
 #include <asm/mcfsim.h>
 #endif
 
 #include "fec.h"
 
-#ifdef CONFIG_ARCH_MXC
-#include <mach/hardware.h>

Since you now remove mach/hardware.h for ARCH_MXC, does this build for all 
i.MX variants?

Did the test build for mx25, mx27, mx3 and mx51.

quoted

+#ifdef CONFIG_SOC_IMX28
+/*
+ * mx28 does not have MIIGSK registers
+ */
+#undef FEC_MIIGSK_ENR
+#include <mach/mxs.h>
+#else
+#define cpu_is_mx28()	(0)
+#endif

This breaks kernels for multiple archs (e.g. i.MX28 and i.MX25). Please use 
run-time detection of CPU type, and do the MII/RMII etc. configuration 
accordingly.

I do not find a good way to detect cpu type.  Neither adding a new
platform data field nor using __machine_arch_type to enumerate all
mx28 based machine (though there is only one currently) seems to be
good for me.

I will try to manipulate some mx28 unique register to identify mx28
from other i.mx variants.  Hopefully, it will work.

Thanks for the comments.

-- 
Regards,
Shawn