From mboxrd@z Thu Jan 1 00:00:00 1970 From: shawn.guo@freescale.com (Shawn Guo) Date: Wed, 5 Jan 2011 17:40:18 +0800 Subject: [PATCH v2 05/10] net/fec: add dual fec support for mx28 In-Reply-To: <20110105090312.GO25121@pengutronix.de> References: <1294133056-21195-1-git-send-email-shawn.guo@freescale.com> <1294133056-21195-6-git-send-email-shawn.guo@freescale.com> <20110104095916.GE2987@jasper.tkos.co.il> <20110104141259.GA21274@freescale.com> <20110105084513.GA26617@pengutronix.de> <20110105090312.GO25121@pengutronix.de> Message-ID: <20110105094011.GA5814@freescale.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Jan 05, 2011 at 10:03:12AM +0100, Uwe Kleine-K?nig wrote: > Hello Shawn, > > On Wed, Jan 05, 2011 at 09:45:13AM +0100, Sascha Hauer wrote: > > On Tue, Jan 04, 2011 at 10:13:09PM +0800, Shawn Guo wrote: > > > 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: > > > > > 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 > > > > > --- > > > > > 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. > > > > > > > > > - 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. > > > > > > > > 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] > > > > > > > > > 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 at macqel.be) > > > > > * Copyright (c) 2004-2006 Macq Electronique SA. > > > > > + * > > > > > + * Copyright (C) 2010 Freescale Semiconductor, Inc. > > > > > */ > > > > > > > > > > #include > > > > > @@ -45,21 +47,34 @@ > > > > > > > > > > #include > > > > > > > > > > -#ifndef CONFIG_ARCH_MXC > > > > > +#if !defined(CONFIG_ARCH_MXC) && !defined(CONFIG_SOC_IMX28) > > > > > #include > > > > > #include > > > > > #endif > > > > > > > > > > #include "fec.h" > > > > > > > > > > -#ifdef CONFIG_ARCH_MXC > > > > > -#include > > > > > > > > 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. > > > > > > > > +#ifdef CONFIG_SOC_IMX28 > > > > > +/* > > > > > + * mx28 does not have MIIGSK registers > > > > > + */ > > > > > +#undef FEC_MIIGSK_ENR > > > > > +#include > > > > > +#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. > > > > There won't be a register which you can safely read on all i.MX > > variants.FEC_TRUNC_FL Register FEC_TRUNC_FL (offset 0x1b0) is one I found I can use to distinguish ENET-MAC from FEC. This address is being reserved on FEC, and I knew from designer that access the address on FEC will not generate exception as it's valid in FEC address range. I proved it on mx51. /* * Detect ENET-MAC by writing and reading back on TRUNC_FL register, * which is accessible on ENET-MAC while always return 0 on FEC. */ writel(0x7ff, fep->hwp + 0x1b0); if (readl(fep->hwp + 0x1b0) == 0x7ff) fec_is_enetmac = 1; But it does not matter. I more like the solution that offered by Uwe below. Thanks, Sascha. > > Why don't you implement it the same way the other i.MX do? They do not > > need SoC detection and the macro expands to 0 at compile time when the > > cpu is not enabled. > Alternatively you can use the approach I used for spi-imx and identify > the device by name. > Thanks, Uwe. > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-K?nig | > Industrial Linux Solutions | http://www.pengutronix.de/ | > -- Regards, Shawn