[PATCH][NET] several cleanups and bugfixes for fec.c

[PATCH][NET] several cleanups and bugfixes for fec.c

[Lothar Waßmann]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 642 bytes --]

The following set of patches provides some cleanup and bugfixes for
drivers/net/ethernet/freescale/fec.c and makes the driver buildable as
a module.

Lothar Waßmann (8):
  misc cleanups
  set con_id in clk_get() call to NULL
  prevent dobule restart of interface on FDX/HDX change
  don't request invalid IRQ
  don't munge MAC address from platform data
  preserve MII/RMII setting in fec_stop()
  fix the .remove code
  make FEC driver buildable as module

 drivers/net/ethernet/freescale/Kconfig |    2 +-
 drivers/net/ethernet/freescale/fec.c   |   63 ++++++++++++++++++++++----------
 2 files changed, 44 insertions(+), 21 deletions(-)

[PATCH][NET] several cleanups and bugfixes for fec.c: misc cleanups

[Lothar Waßmann]

 - remove some bogus whitespace
 - remove line wraps from printk messages

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/net/ethernet/freescale/fec.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index 1124ce0..f224e58 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -99,7 +99,7 @@ static struct platform_device_id fec_devtype[] = {
 MODULE_DEVICE_TABLE(platform, fec_devtype);
 
 enum imx_fec_type {
-	IMX25_FEC = 1, 	/* runs on i.mx25/50/53 */
+	IMX25_FEC = 1,	/* runs on i.mx25/50/53 */
 	IMX27_FEC,	/* runs on i.mx27/35/51 */
 	IMX28_FEC,
 	IMX6Q_FEC,
@@ -132,7 +132,7 @@ MODULE_PARM_DESC(macaddr, "FEC Ethernet MAC address");
 #elif defined (CONFIG_M5272C3)
 #define	FEC_FLASHMAC	(0xffe04000 + 4)
 #elif defined(CONFIG_MOD5272)
-#define FEC_FLASHMAC 	0xffc0406b
+#define FEC_FLASHMAC	0xffc0406b
 #else
 #define	FEC_FLASHMAC	0
 #endif
@@ -972,8 +972,9 @@ static int fec_enet_mii_probe(struct net_device *ndev)
 	}
 
 	if (phy_id >= PHY_MAX_ADDR) {
-		printk(KERN_INFO "%s: no PHY, assuming direct connection "
-			"to switch\n", ndev->name);
+		printk(KERN_INFO
+			"%s: no PHY, assuming direct connection to switch\n",
+			ndev->name);
 		strncpy(mdio_bus_id, "0", MII_BUS_ID_SIZE);
 		phy_id = 0;
 	}
@@ -998,8 +999,9 @@ static int fec_enet_mii_probe(struct net_device *ndev)
 	fep->link = 0;
 	fep->full_duplex = 0;
 
-	printk(KERN_INFO "%s: Freescale FEC PHY driver [%s] "
-		"(mii_bus:phy_addr=%s, irq=%d)\n", ndev->name,
+	printk(KERN_INFO
+		"%s: Freescale FEC PHY driver [%s] (mii_bus:phy_addr=%s, irq=%d)\n",
+		ndev->name,
 		fep->phy_dev->drv->name, dev_name(&fep->phy_dev->dev),
 		fep->phy_dev->irq);
 
-- 
1.5.6.5

Re: [PATCH][NET] several cleanups and bugfixes for fec.c: misc cleanups

[Shawn Guo]

On Tue, Dec 06, 2011 at 11:27:09AM +0100, Lothar Waßmann wrote:
>  - remove some bogus whitespace
>  - remove line wraps from printk messages
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>

Other than subject,

Acked-by: Shawn Guo <shawn.guo@linaro.org>

-- 
Regards,
Shawn

[PATCH][NET] several cleanups and bugfixes for fec.c: set con_id in clk_get() call to NULL

[Lothar Waßmann]

The con_id is actually not needed for clk_get().

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/net/ethernet/freescale/fec.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index f224e58..65ee506 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -1585,7 +1585,7 @@ fec_probe(struct platform_device *pdev)
 		}
 	}
 
-	fep->clk = clk_get(&pdev->dev, "fec_clk");
+	fep->clk = clk_get(&pdev->dev, NULL);
 	if (IS_ERR(fep->clk)) {
 		ret = PTR_ERR(fep->clk);
 		goto failed_clk;
-- 
1.5.6.5

Re: [PATCH][NET] several cleanups and bugfixes for fec.c: set con_id in clk_get() call to NULL

[Shawn Guo]

On Tue, Dec 06, 2011 at 11:27:10AM +0100, Lothar Waßmann wrote:
> The con_id is actually not needed for clk_get().
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>

Other than subject,

Acked-by: Shawn Guo <shawn.guo@linaro.org>

-- 
Regards,
Shawn

[PATCH][NET] several cleanups and bugfixes for fec.c: prevent dobule restart of interface on FDX/HDX change

[Lothar Waßmann]

Upon detection of a FDX/HDX change the interface is restarted twice.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/net/ethernet/freescale/fec.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index 65ee506..7ef408f 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -865,6 +865,8 @@ static void fec_enet_adjust_link(struct net_device *ndev)
 	if (phy_dev->link) {
 		if (fep->full_duplex != phy_dev->duplex) {
 			fec_restart(ndev, phy_dev->duplex);
+			/* prevent unnecessary second fec_restart() below */
+			fep->link = phy_dev->link;
 			status_change = 1;
 		}
 	}
-- 
1.5.6.5

Re: [PATCH][NET] several cleanups and bugfixes for fec.c: prevent dobule restart of interface on FDX/HDX change

[Ben Hutchings]

You shouldn't repeat the subject of the whole series in the subject of
each patch.  Just put the name of the component you're changing (in this
case, 'fec: prevent dobule restart of interface on FDX/HDX change').

(Nitpick: typo in 'double'.)

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH][NET] several cleanups and bugfixes for fec.c: don't request invalid IRQ

[Lothar Waßmann]

prevent calling request_irq() with a known invalid IRQ number and
preserve the return value of the platform_get_irq() function

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/net/ethernet/freescale/fec.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index 7ef408f..e2b5ce6 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -1575,8 +1575,12 @@ fec_probe(struct platform_device *pdev)
 
 	for (i = 0; i < FEC_IRQ_NUM; i++) {
 		irq = platform_get_irq(pdev, i);
-		if (i && irq < 0)
-			break;
+		if (irq < 0) {
+			if (i)
+				break;
+			ret = irq;
+			goto failed_irq;
+		}
 		ret = request_irq(irq, fec_enet_interrupt, IRQF_DISABLED, pdev->name, ndev);
 		if (ret) {
 			while (--i >= 0) {
-- 
1.5.6.5

Re: [PATCH][NET] several cleanups and bugfixes for fec.c: don't request invalid IRQ

[Shawn Guo]

On Tue, Dec 06, 2011 at 11:27:12AM +0100, Lothar Waßmann wrote:
> prevent calling request_irq() with a known invalid IRQ number and
> preserve the return value of the platform_get_irq() function
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>

Other than subject,

Acked-by: Shawn Guo <shawn.guo@linaro.org>

-- 
Regards,
Shawn

[PATCH][NET] several cleanups and bugfixes for fec.c: don't munge MAC address from platform data

[Lothar Waßmann]

When the MAC address is supplied via platform_data it should be OK as
it is and should not be modified in case of a dual FEC setup.
Also copying the MAC from platform_data to the single 'macaddr'
variable will overwrite the MAC for the first interface in case of a
dual FEC setup.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/net/ethernet/freescale/fec.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index e2b5ce6..11534b9 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -818,7 +818,7 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
 			iap = (unsigned char *)FEC_FLASHMAC;
 #else
 		if (pdata)
-			memcpy(iap, pdata->mac, ETH_ALEN);
+			iap = (unsigned char *)&pdata->mac;
 #endif
 	}
 
-- 
1.5.6.5

Re: [PATCH][NET] several cleanups and bugfixes for fec.c: don't munge MAC address from platform data

[Baruch Siach]

Hi Lothar,

On Tue, Dec 06, 2011 at 11:27:13AM +0100, Lothar Waßmann wrote:
> When the MAC address is supplied via platform_data it should be OK as
> it is and should not be modified in case of a dual FEC setup.
> Also copying the MAC from platform_data to the single 'macaddr'
> variable will overwrite the MAC for the first interface in case of a
> dual FEC setup.
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  drivers/net/ethernet/freescale/fec.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> index e2b5ce6..11534b9 100644
> --- a/drivers/net/ethernet/freescale/fec.c
> +++ b/drivers/net/ethernet/freescale/fec.c
> @@ -818,7 +818,7 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
>  			iap = (unsigned char *)FEC_FLASHMAC;
>  #else
>  		if (pdata)
> -			memcpy(iap, pdata->mac, ETH_ALEN);
> +			iap = (unsigned char *)&pdata->mac;

Since pdata might point to __initdata struct, you must hold a copy of its 
content.

baruch

>  #endif
>  	}
>  
> -- 

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH][NET] several cleanups and bugfixes for fec.c: don't munge MAC address from platform data

[Lothar Waßmann]

Hi,

Baruch Siach writes:
> Hi Lothar,
> 
> On Tue, Dec 06, 2011 at 11:27:13AM +0100, Lothar Waßmann wrote:
> > When the MAC address is supplied via platform_data it should be OK as
> > it is and should not be modified in case of a dual FEC setup.
> > Also copying the MAC from platform_data to the single 'macaddr'
> > variable will overwrite the MAC for the first interface in case of a
> > dual FEC setup.
> > 
> > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > ---
> >  drivers/net/ethernet/freescale/fec.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> > index e2b5ce6..11534b9 100644
> > --- a/drivers/net/ethernet/freescale/fec.c
> > +++ b/drivers/net/ethernet/freescale/fec.c
> > @@ -818,7 +818,7 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
> >  			iap = (unsigned char *)FEC_FLASHMAC;
> >  #else
> >  		if (pdata)
> > -			memcpy(iap, pdata->mac, ETH_ALEN);
> > +			iap = (unsigned char *)&pdata->mac;
> 
> Since pdata might point to __initdata struct, you must hold a copy of its 
> content.
> 
No. platform_data must be present during the life time of a driver
using it and thus must never be __initdata!


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH][NET] several cleanups and bugfixes for fec.c: don't munge MAC address from platform data

[Shawn Guo]

On Tue, Dec 06, 2011 at 02:44:44PM +0100, Lothar Waßmann wrote:
> Hi,
> 
> Baruch Siach writes:
> > Hi Lothar,
> > 
> > On Tue, Dec 06, 2011 at 11:27:13AM +0100, Lothar Waßmann wrote:
> > > When the MAC address is supplied via platform_data it should be OK as
> > > it is and should not be modified in case of a dual FEC setup.
> > > Also copying the MAC from platform_data to the single 'macaddr'
> > > variable will overwrite the MAC for the first interface in case of a
> > > dual FEC setup.
> > > 
> > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > > ---
> > >  drivers/net/ethernet/freescale/fec.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> > > index e2b5ce6..11534b9 100644
> > > --- a/drivers/net/ethernet/freescale/fec.c
> > > +++ b/drivers/net/ethernet/freescale/fec.c
> > > @@ -818,7 +818,7 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
> > >  			iap = (unsigned char *)FEC_FLASHMAC;
> > >  #else
> > >  		if (pdata)
> > > -			memcpy(iap, pdata->mac, ETH_ALEN);
> > > +			iap = (unsigned char *)&pdata->mac;
> > 
> > Since pdata might point to __initdata struct, you must hold a copy of its 
> > content.
> > 

As iap will anyway be copied to ndev->dev_addr after that, it may still
be fine to take this patch, for obviously for different reason.

> No. platform_data must be present during the life time of a driver
> using it and thus must never be __initdata!
> 
Then we need to fix a lot of imx/mxc platform codes, mach-mx28evk.c
is the one for this case.

-- 
Regards,
Shawn

Re: [PATCH][NET] several cleanups and bugfixes for fec.c: don't munge MAC address from platform data

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> On Tue, Dec 06, 2011 at 02:44:44PM +0100, Lothar Waßmann wrote:
> > Hi,
> > 
> > Baruch Siach writes:
> > > Hi Lothar,
> > > 
> > > On Tue, Dec 06, 2011 at 11:27:13AM +0100, Lothar Waßmann wrote:
> > > > When the MAC address is supplied via platform_data it should be OK as
> > > > it is and should not be modified in case of a dual FEC setup.
> > > > Also copying the MAC from platform_data to the single 'macaddr'
> > > > variable will overwrite the MAC for the first interface in case of a
> > > > dual FEC setup.
> > > > 
> > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > > > ---
> > > >  drivers/net/ethernet/freescale/fec.c |    2 +-
> > > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > > 
> > > > diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> > > > index e2b5ce6..11534b9 100644
> > > > --- a/drivers/net/ethernet/freescale/fec.c
> > > > +++ b/drivers/net/ethernet/freescale/fec.c
> > > > @@ -818,7 +818,7 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
> > > >  			iap = (unsigned char *)FEC_FLASHMAC;
> > > >  #else
> > > >  		if (pdata)
> > > > -			memcpy(iap, pdata->mac, ETH_ALEN);
> > > > +			iap = (unsigned char *)&pdata->mac;
> > > 
> > > Since pdata might point to __initdata struct, you must hold a copy of its 
> > > content.
> > > 
> 
> As iap will anyway be copied to ndev->dev_addr after that, it may still
> be fine to take this patch, for obviously for different reason.
> 
> > No. platform_data must be present during the life time of a driver
> > using it and thus must never be __initdata!
> > 
> Then we need to fix a lot of imx/mxc platform codes, mach-mx28evk.c
> is the one for this case.
> 
Nope. The platform_data provided there is only a template that is
copied to kmalloc'ed memory by the mxs_add...() functions.

With actual platform_data in init memory no driver built as module
would work.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH][NET] several cleanups and bugfixes for fec.c: don't munge MAC address from platform data

[Shawn Guo]

On Tue, Dec 06, 2011 at 11:27:13AM +0100, Lothar Waßmann wrote:
> When the MAC address is supplied via platform_data it should be OK as
> it is and should not be modified in case of a dual FEC setup.
> Also copying the MAC from platform_data to the single 'macaddr'
> variable will overwrite the MAC for the first interface in case of a
> dual FEC setup.

Hmm, I do not follow that.  If 'macaddr' holds a valid mac address,
the code path has no chance to be hit at all.  Otherwise, 'macaddr'
is just a place holder for copying mac address from pdata, in which
case the mac address will be fixed up at the end of the function for
dual FEC setup.

Regards,
Shawn

> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  drivers/net/ethernet/freescale/fec.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> index e2b5ce6..11534b9 100644
> --- a/drivers/net/ethernet/freescale/fec.c
> +++ b/drivers/net/ethernet/freescale/fec.c
> @@ -818,7 +818,7 @@ static void __inline__ fec_get_mac(struct net_device *ndev)
>  			iap = (unsigned char *)FEC_FLASHMAC;
>  #else
>  		if (pdata)
> -			memcpy(iap, pdata->mac, ETH_ALEN);
> +			iap = (unsigned char *)&pdata->mac;
>  #endif
>  	}
>  
> -- 
> 1.5.6.5

Re: [PATCH][NET] several cleanups and bugfixes for fec.c: don't munge MAC address from platform data

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> On Tue, Dec 06, 2011 at 11:27:13AM +0100, Lothar Waßmann wrote:
> > When the MAC address is supplied via platform_data it should be OK as
> > it is and should not be modified in case of a dual FEC setup.
> > Also copying the MAC from platform_data to the single 'macaddr'
> > variable will overwrite the MAC for the first interface in case of a
> > dual FEC setup.
> 
> Hmm, I do not follow that.  If 'macaddr' holds a valid mac address,
> the code path has no chance to be hit at all.  Otherwise, 'macaddr'
> is just a place holder for copying mac address from pdata, in which
> case the mac address will be fixed up at the end of the function for
> dual FEC setup.
> 
In case of dual FEC and MAC address provided by platform_data the
first mac address will be copied to the 'macaddr' variable with no
chance for the second interface to get its mac address assigned from
platform_data too.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH][NET] several cleanups and bugfixes for fec.c: don't munge MAC address from platform data

[Shawn Guo]

On Wed, Dec 07, 2011 at 09:47:37AM +0100, Lothar Waßmann wrote:
> Hi,
> 
> Shawn Guo writes:
> > On Tue, Dec 06, 2011 at 11:27:13AM +0100, Lothar Waßmann wrote:
> > > When the MAC address is supplied via platform_data it should be OK as
> > > it is and should not be modified in case of a dual FEC setup.
> > > Also copying the MAC from platform_data to the single 'macaddr'
> > > variable will overwrite the MAC for the first interface in case of a
> > > dual FEC setup.
> > 
> > Hmm, I do not follow that.  If 'macaddr' holds a valid mac address,
> > the code path has no chance to be hit at all.  Otherwise, 'macaddr'
> > is just a place holder for copying mac address from pdata, in which
> > case the mac address will be fixed up at the end of the function for
> > dual FEC setup.
> > 
> In case of dual FEC and MAC address provided by platform_data the
> first mac address will be copied to the 'macaddr' variable with no
> chance for the second interface to get its mac address assigned from
> platform_data too.
> 
Indeed.  So again other than subject,

Acked-by: Shawn Guo <shawn.guo@linaro.org>

-- 
Regards,
Shawn

[PATCH][NET] several cleanups and bugfixes for fec.c: preserve MII/RMII setting in fec_stop()

[Lothar Waßmann]

Additionally to setting the ETHER_EN bit in FEC_ECNTRL the MII/RMII
setting in FEC_R_CNTRL needs to be preserved to keep the MII interface
functional.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/net/ethernet/freescale/fec.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index 11534b9..ab0afb5 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -515,6 +515,7 @@ fec_stop(struct net_device *ndev)
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	const struct platform_device_id *id_entry =
 				platform_get_device_id(fep->pdev);
+	u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8);
 
 	/* We cannot expect a graceful transmit stop without link !!! */
 	if (fep->link) {
@@ -531,8 +532,10 @@ fec_stop(struct net_device *ndev)
 	writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
 
 	/* We have to keep ENET enabled to have MII interrupt stay working */
-	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC)
+	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) {
 		writel(2, fep->hwp + FEC_ECNTRL);
+		writel(rmii_mode, fep->hwp + FEC_R_CNTRL);
+	}
 }
 
 
-- 
1.5.6.5

Re: [PATCH][NET] several cleanups and bugfixes for fec.c: preserve MII/RMII setting in fec_stop()

[Shawn Guo]

On Tue, Dec 06, 2011 at 11:27:14AM +0100, Lothar Waßmann wrote:
> Additionally to setting the ETHER_EN bit in FEC_ECNTRL the MII/RMII
> setting in FEC_R_CNTRL needs to be preserved to keep the MII interface

s/MII/RMII?  From what I see from imx28 and imx6q RM, the reset state
for this setting is MII mode.

> functional.
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  drivers/net/ethernet/freescale/fec.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)

I assume this is fixing a problem you are seeing on imx28 only.
Do you see the problem on imx53/51?

> 
> diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> index 11534b9..ab0afb5 100644
> --- a/drivers/net/ethernet/freescale/fec.c
> +++ b/drivers/net/ethernet/freescale/fec.c
> @@ -515,6 +515,7 @@ fec_stop(struct net_device *ndev)
>  	struct fec_enet_private *fep = netdev_priv(ndev);
>  	const struct platform_device_id *id_entry =
>  				platform_get_device_id(fep->pdev);
> +	u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8);

This bit is only available on ENET (imx28 and imx6q).  Do we want to
do the same thing for FEC (imx25/27/35/51/53)?

>  
>  	/* We cannot expect a graceful transmit stop without link !!! */
>  	if (fep->link) {
> @@ -531,8 +532,10 @@ fec_stop(struct net_device *ndev)
>  	writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
>  
>  	/* We have to keep ENET enabled to have MII interrupt stay working */
> -	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC)
> +	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) {
>  		writel(2, fep->hwp + FEC_ECNTRL);
> +		writel(rmii_mode, fep->hwp + FEC_R_CNTRL);
> +	}
>  }

On imx6q, we have two bits, bit 6 and 8 of FEC_R_CNTRL, to select MII
interface among MII, RMII and RGMII modes.  I'm not sure if we will
run into the same problem for RGMII mode.  What's your test setup?
I would try to reproduce it here.

-- 
Regards,
Shawn

Re: [PATCH][NET] several cleanups and bugfixes for fec.c: preserve MII/RMII setting in fec_stop()

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> On Tue, Dec 06, 2011 at 11:27:14AM +0100, Lothar Waßmann wrote:
> > Additionally to setting the ETHER_EN bit in FEC_ECNTRL the MII/RMII
> > setting in FEC_R_CNTRL needs to be preserved to keep the MII interface
> 
> s/MII/RMII?  From what I see from imx28 and imx6q RM, the reset state
> for this setting is MII mode.
> 
> > functional.
> > 
> > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > ---
> >  drivers/net/ethernet/freescale/fec.c |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> I assume this is fixing a problem you are seeing on imx28 only.
> Do you see the problem on imx53/51?
> 
No. i.MX53 uses the RMII gasket which is not affected by resetting the
controller. And imMX51 does not support RMII at all.

> > 
> > diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> > index 11534b9..ab0afb5 100644
> > --- a/drivers/net/ethernet/freescale/fec.c
> > +++ b/drivers/net/ethernet/freescale/fec.c
> > @@ -515,6 +515,7 @@ fec_stop(struct net_device *ndev)
> >  	struct fec_enet_private *fep = netdev_priv(ndev);
> >  	const struct platform_device_id *id_entry =
> >  				platform_get_device_id(fep->pdev);
> > +	u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8);
> 
> This bit is only available on ENET (imx28 and imx6q).  Do we want to
> do the same thing for FEC (imx25/27/35/51/53)?
> 
No. AFAICT that's not necessary there.

> >  	/* We cannot expect a graceful transmit stop without link !!! */
> >  	if (fep->link) {
> > @@ -531,8 +532,10 @@ fec_stop(struct net_device *ndev)
> >  	writel(FEC_DEFAULT_IMASK, fep->hwp + FEC_IMASK);
> >  
> >  	/* We have to keep ENET enabled to have MII interrupt stay working */
> > -	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC)
> > +	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) {
> >  		writel(2, fep->hwp + FEC_ECNTRL);
> > +		writel(rmii_mode, fep->hwp + FEC_R_CNTRL);
> > +	}
> >  }
> 
> On imx6q, we have two bits, bit 6 and 8 of FEC_R_CNTRL, to select MII
> interface among MII, RMII and RGMII modes.  I'm not sure if we will
> run into the same problem for RGMII mode.  What's your test setup?
> I would try to reproduce it here.
> 
I'm using a TX28 which has the Ethernet PHY connected in RMII
mode. You should see the problem, if you unplug the ethernet cable on
a configured link. Without this patch the kernel will not detect when
the cable is plugged back in.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH][NET] several cleanups and bugfixes for fec.c: preserve MII/RMII setting in fec_stop()

[Shawn Guo]

On Wed, Dec 07, 2011 at 11:42:28AM +0100, Lothar Waßmann wrote:
> Hi,
> 
> Shawn Guo writes:
> > On Tue, Dec 06, 2011 at 11:27:14AM +0100, Lothar Waßmann wrote:
> > > Additionally to setting the ETHER_EN bit in FEC_ECNTRL the MII/RMII
> > > setting in FEC_R_CNTRL needs to be preserved to keep the MII interface
> > 
> > s/MII/RMII?  From what I see from imx28 and imx6q RM, the reset state
> > for this setting is MII mode.
> > 
> > > functional.
> > > 
> > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > > ---
> > >  drivers/net/ethernet/freescale/fec.c |    5 ++++-
> > >  1 files changed, 4 insertions(+), 1 deletions(-)
> > 
> > I assume this is fixing a problem you are seeing on imx28 only.
> > Do you see the problem on imx53/51?
> > 
> No. i.MX53 uses the RMII gasket which is not affected by resetting the
> controller. And imMX51 does not support RMII at all.
> 
> > > 
> > > diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> > > index 11534b9..ab0afb5 100644
> > > --- a/drivers/net/ethernet/freescale/fec.c
> > > +++ b/drivers/net/ethernet/freescale/fec.c
> > > @@ -515,6 +515,7 @@ fec_stop(struct net_device *ndev)
> > >  	struct fec_enet_private *fep = netdev_priv(ndev);
> > >  	const struct platform_device_id *id_entry =
> > >  				platform_get_device_id(fep->pdev);
> > > +	u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8);
> > 
> > This bit is only available on ENET (imx28 and imx6q).  Do we want to
> > do the same thing for FEC (imx25/27/35/51/53)?
> > 
> No. AFAICT that's not necessary there.
> 
So you need to check it's actually running on ENET before accessing
the bit.

-- 
Regards,
Shawn

Re: [PATCH][NET] several cleanups and bugfixes for fec.c: preserve MII/RMII setting in fec_stop()

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> On Wed, Dec 07, 2011 at 11:42:28AM +0100, Lothar Waßmann wrote:
> > Hi,
> > 
> > Shawn Guo writes:
> > > On Tue, Dec 06, 2011 at 11:27:14AM +0100, Lothar Waßmann wrote:
> > > > Additionally to setting the ETHER_EN bit in FEC_ECNTRL the MII/RMII
> > > > setting in FEC_R_CNTRL needs to be preserved to keep the MII interface
> > > 
> > > s/MII/RMII?  From what I see from imx28 and imx6q RM, the reset state
> > > for this setting is MII mode.
> > > 
> > > > functional.
> > > > 
> > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > > > ---
> > > >  drivers/net/ethernet/freescale/fec.c |    5 ++++-
> > > >  1 files changed, 4 insertions(+), 1 deletions(-)
> > > 
> > > I assume this is fixing a problem you are seeing on imx28 only.
> > > Do you see the problem on imx53/51?
> > > 
> > No. i.MX53 uses the RMII gasket which is not affected by resetting the
> > controller. And imMX51 does not support RMII at all.
> > 
> > > > 
> > > > diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> > > > index 11534b9..ab0afb5 100644
> > > > --- a/drivers/net/ethernet/freescale/fec.c
> > > > +++ b/drivers/net/ethernet/freescale/fec.c
> > > > @@ -515,6 +515,7 @@ fec_stop(struct net_device *ndev)
> > > >  	struct fec_enet_private *fep = netdev_priv(ndev);
> > > >  	const struct platform_device_id *id_entry =
> > > >  				platform_get_device_id(fep->pdev);
> > > > +	u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8);
> > > 
> > > This bit is only available on ENET (imx28 and imx6q).  Do we want to
> > > do the same thing for FEC (imx25/27/35/51/53)?
> > > 
> > No. AFAICT that's not necessary there.
> > 
> So you need to check it's actually running on ENET before accessing
> the bit.
> 
That's done in the place where the register is being written:
|	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) {
|		writel(2, fep->hwp + FEC_ECNTRL);
|		writel(rmii_mode, fep->hwp + FEC_R_CNTRL);
|	}

We could save one register read by doing the check also for the read,
but that would further complicate the code.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH][NET] several cleanups and bugfixes for fec.c: preserve MII/RMII setting in fec_stop()

[Shawn Guo]

On Wed, Dec 07, 2011 at 11:49:09AM +0100, Lothar Waßmann wrote:
> Hi,
> 
> Shawn Guo writes:
> > On Wed, Dec 07, 2011 at 11:42:28AM +0100, Lothar Waßmann wrote:
> > > Hi,
> > > 
> > > Shawn Guo writes:
> > > > On Tue, Dec 06, 2011 at 11:27:14AM +0100, Lothar Waßmann wrote:
> > > > > Additionally to setting the ETHER_EN bit in FEC_ECNTRL the MII/RMII
> > > > > setting in FEC_R_CNTRL needs to be preserved to keep the MII interface
> > > > 
> > > > s/MII/RMII?  From what I see from imx28 and imx6q RM, the reset state
> > > > for this setting is MII mode.
> > > > 
> > > > > functional.
> > > > > 
> > > > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > > > > ---
> > > > >  drivers/net/ethernet/freescale/fec.c |    5 ++++-
> > > > >  1 files changed, 4 insertions(+), 1 deletions(-)
> > > > 
> > > > I assume this is fixing a problem you are seeing on imx28 only.
> > > > Do you see the problem on imx53/51?
> > > > 
> > > No. i.MX53 uses the RMII gasket which is not affected by resetting the
> > > controller. And imMX51 does not support RMII at all.
> > > 
> > > > > 
> > > > > diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> > > > > index 11534b9..ab0afb5 100644
> > > > > --- a/drivers/net/ethernet/freescale/fec.c
> > > > > +++ b/drivers/net/ethernet/freescale/fec.c
> > > > > @@ -515,6 +515,7 @@ fec_stop(struct net_device *ndev)
> > > > >  	struct fec_enet_private *fep = netdev_priv(ndev);
> > > > >  	const struct platform_device_id *id_entry =
> > > > >  				platform_get_device_id(fep->pdev);
> > > > > +	u32 rmii_mode = readl(fep->hwp + FEC_R_CNTRL) & (1 << 8);
> > > > 
> > > > This bit is only available on ENET (imx28 and imx6q).  Do we want to
> > > > do the same thing for FEC (imx25/27/35/51/53)?
> > > > 
> > > No. AFAICT that's not necessary there.
> > > 
> > So you need to check it's actually running on ENET before accessing
> > the bit.
> > 
> That's done in the place where the register is being written:
> |	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) {
> |		writel(2, fep->hwp + FEC_ECNTRL);
> |		writel(rmii_mode, fep->hwp + FEC_R_CNTRL);
> |	}
> 
> We could save one register read by doing the check also for the read,
> but that would further complicate the code.
> 
Ok.  Will have a test and then get back to you.

-- 
Regards,
Shawn

Re: [PATCH][NET] several cleanups and bugfixes for fec.c: preserve MII/RMII setting in fec_stop()

[Shawn Guo]

On Wed, Dec 07, 2011 at 11:42:28AM +0100, Lothar Waßmann wrote:
> I'm using a TX28 which has the Ethernet PHY connected in RMII
> mode. You should see the problem, if you unplug the ethernet cable on
> a configured link. Without this patch the kernel will not detect when
> the cable is plugged back in.
> 
Tested-by: Shawn Guo <shawn.guo@linaro.org>

-- 
Regards,
Shawn

[PATCH][NET] several cleanups and bugfixes for fec.c: fix the .remove code

[Lothar Waßmann]

The .remove code is broken in several ways.
 - mdiobus_unregister() is called twice for the same object in case of dual FEC
 - phy_disconnect() is being called when the PHY is already disconnected
 - the requested IRQ(s) are not freed
 - fec_stop() is being called with the inteface already stopped

 All of those lead to kernel crashes if the remove function is actually used.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/net/ethernet/freescale/fec.c |   30 +++++++++++++++++++++---------
 1 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
index ab0afb5..70ec7ec 100644
--- a/drivers/net/ethernet/freescale/fec.c
+++ b/drivers/net/ethernet/freescale/fec.c
@@ -235,6 +235,7 @@ struct fec_enet_private {
 
 	/* Phylib and MDIO interface */
 	struct	mii_bus *mii_bus;
+	int	mii_cnt;
 	struct	phy_device *phy_dev;
 	int	mii_timeout;
 	uint	phy_speed;
@@ -1040,8 +1041,12 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 	 */
 	if ((id_entry->driver_data & FEC_QUIRK_ENET_MAC) && pdev->id > 0) {
 		/* fec1 uses fec0 mii_bus */
-		fep->mii_bus = fec0_mii_bus;
-		return 0;
+		if (fep->mii_cnt && fec0_mii_bus) {
+			fep->mii_bus = fec0_mii_bus;
+			fep->mii_cnt++;
+			return 0;
+		}
+		return -ENOENT;
 	}
 
 	fep->mii_timeout = 0;
@@ -1086,6 +1091,8 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 	if (mdiobus_register(fep->mii_bus))
 		goto err_out_free_mdio_irq;
 
+	fep->mii_cnt++;
+
 	/* save fec0 mii_bus */
 	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC)
 		fec0_mii_bus = fep->mii_bus;
@@ -1102,11 +1109,11 @@ err_out:
 
 static void fec_enet_mii_remove(struct fec_enet_private *fep)
 {
-	if (fep->phy_dev)
-		phy_disconnect(fep->phy_dev);
-	mdiobus_unregister(fep->mii_bus);
-	kfree(fep->mii_bus->irq);
-	mdiobus_free(fep->mii_bus);
+	if (--fep->mii_cnt == 0) {
+		mdiobus_unregister(fep->mii_bus);
+		kfree(fep->mii_bus->irq);
+		mdiobus_free(fep->mii_bus);
+	}
 }
 
 static int fec_enet_get_settings(struct net_device *ndev,
@@ -1646,13 +1653,18 @@ fec_drv_remove(struct platform_device *pdev)
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct fec_enet_private *fep = netdev_priv(ndev);
 	struct resource *r;
+	int i;
 
-	fec_stop(ndev);
+	unregister_netdev(ndev);
 	fec_enet_mii_remove(fep);
+	for (i = 0; i < FEC_IRQ_NUM; i++) {
+		int irq = platform_get_irq(pdev, i);
+		if (irq > 0)
+			free_irq(irq, ndev);
+	}
 	clk_disable(fep->clk);
 	clk_put(fep->clk);
 	iounmap(fep->hwp);
-	unregister_netdev(ndev);
 	free_netdev(ndev);
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-- 
1.5.6.5

Re: [PATCH][NET] several cleanups and bugfixes for fec.c: fix the .remove code

[Shawn Guo]

On Tue, Dec 06, 2011 at 11:27:15AM +0100, Lothar Waßmann wrote:
> The .remove code is broken in several ways.
>  - mdiobus_unregister() is called twice for the same object in case of dual FEC
>  - phy_disconnect() is being called when the PHY is already disconnected
>  - the requested IRQ(s) are not freed
>  - fec_stop() is being called with the inteface already stopped
> 
>  All of those lead to kernel crashes if the remove function is actually used.
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  drivers/net/ethernet/freescale/fec.c |   30 +++++++++++++++++++++---------
>  1 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> index ab0afb5..70ec7ec 100644
> --- a/drivers/net/ethernet/freescale/fec.c
> +++ b/drivers/net/ethernet/freescale/fec.c
> @@ -235,6 +235,7 @@ struct fec_enet_private {
>  
>  	/* Phylib and MDIO interface */
>  	struct	mii_bus *mii_bus;
> +	int	mii_cnt;
>  	struct	phy_device *phy_dev;
>  	int	mii_timeout;
>  	uint	phy_speed;
> @@ -1040,8 +1041,12 @@ static int fec_enet_mii_init(struct platform_device *pdev)
>  	 */
>  	if ((id_entry->driver_data & FEC_QUIRK_ENET_MAC) && pdev->id > 0) {
>  		/* fec1 uses fec0 mii_bus */
> -		fep->mii_bus = fec0_mii_bus;
> -		return 0;
> +		if (fep->mii_cnt && fec0_mii_bus) {

This seems broken.  The second fec has its own fep and fep->mii_cnt is
always 0 here.

Regards,
Shawn

> +			fep->mii_bus = fec0_mii_bus;
> +			fep->mii_cnt++;
> +			return 0;
> +		}
> +		return -ENOENT;
>  	}
>  
>  	fep->mii_timeout = 0;
> @@ -1086,6 +1091,8 @@ static int fec_enet_mii_init(struct platform_device *pdev)
>  	if (mdiobus_register(fep->mii_bus))
>  		goto err_out_free_mdio_irq;
>  
> +	fep->mii_cnt++;
> +
>  	/* save fec0 mii_bus */
>  	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC)
>  		fec0_mii_bus = fep->mii_bus;
> @@ -1102,11 +1109,11 @@ err_out:
>  
>  static void fec_enet_mii_remove(struct fec_enet_private *fep)
>  {
> -	if (fep->phy_dev)
> -		phy_disconnect(fep->phy_dev);
> -	mdiobus_unregister(fep->mii_bus);
> -	kfree(fep->mii_bus->irq);
> -	mdiobus_free(fep->mii_bus);
> +	if (--fep->mii_cnt == 0) {
> +		mdiobus_unregister(fep->mii_bus);
> +		kfree(fep->mii_bus->irq);
> +		mdiobus_free(fep->mii_bus);
> +	}
>  }
>  
>  static int fec_enet_get_settings(struct net_device *ndev,
> @@ -1646,13 +1653,18 @@ fec_drv_remove(struct platform_device *pdev)
>  	struct net_device *ndev = platform_get_drvdata(pdev);
>  	struct fec_enet_private *fep = netdev_priv(ndev);
>  	struct resource *r;
> +	int i;
>  
> -	fec_stop(ndev);
> +	unregister_netdev(ndev);
>  	fec_enet_mii_remove(fep);
> +	for (i = 0; i < FEC_IRQ_NUM; i++) {
> +		int irq = platform_get_irq(pdev, i);
> +		if (irq > 0)
> +			free_irq(irq, ndev);
> +	}
>  	clk_disable(fep->clk);
>  	clk_put(fep->clk);
>  	iounmap(fep->hwp);
> -	unregister_netdev(ndev);
>  	free_netdev(ndev);
>  
>  	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -- 
> 1.5.6.5

Re: [PATCH][NET] several cleanups and bugfixes for fec.c: fix the .remove code

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> On Tue, Dec 06, 2011 at 11:27:15AM +0100, Lothar Waßmann wrote:
> > The .remove code is broken in several ways.
> >  - mdiobus_unregister() is called twice for the same object in case of dual FEC
> >  - phy_disconnect() is being called when the PHY is already disconnected
> >  - the requested IRQ(s) are not freed
> >  - fec_stop() is being called with the inteface already stopped
> > 
> >  All of those lead to kernel crashes if the remove function is actually used.
> > 
> > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > ---
> >  drivers/net/ethernet/freescale/fec.c |   30 +++++++++++++++++++++---------
> >  1 files changed, 21 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> > index ab0afb5..70ec7ec 100644
> > --- a/drivers/net/ethernet/freescale/fec.c
> > +++ b/drivers/net/ethernet/freescale/fec.c
> > @@ -235,6 +235,7 @@ struct fec_enet_private {
> >  
> >  	/* Phylib and MDIO interface */
> >  	struct	mii_bus *mii_bus;
> > +	int	mii_cnt;
> >  	struct	phy_device *phy_dev;
> >  	int	mii_timeout;
> >  	uint	phy_speed;
> > @@ -1040,8 +1041,12 @@ static int fec_enet_mii_init(struct platform_device *pdev)
> >  	 */
> >  	if ((id_entry->driver_data & FEC_QUIRK_ENET_MAC) && pdev->id > 0) {
> >  		/* fec1 uses fec0 mii_bus */
> > -		fep->mii_bus = fec0_mii_bus;
> > -		return 0;
> > +		if (fep->mii_cnt && fec0_mii_bus) {
> 
> This seems broken.  The second fec has its own fep and fep->mii_cnt is
> always 0 here.
> 
I already noticed that myself.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH][NET] several cleanups and bugfixes for fec.c: fix the .remove code

[Shawn Guo]

On Wed, Dec 07, 2011 at 02:26:34PM +0100, Lothar Waßmann wrote:
> Hi,
> 
> Shawn Guo writes:
> > On Tue, Dec 06, 2011 at 11:27:15AM +0100, Lothar Waßmann wrote:
> > > The .remove code is broken in several ways.
> > >  - mdiobus_unregister() is called twice for the same object in case of dual FEC
> > >  - phy_disconnect() is being called when the PHY is already disconnected
> > >  - the requested IRQ(s) are not freed
> > >  - fec_stop() is being called with the inteface already stopped
> > > 
> > >  All of those lead to kernel crashes if the remove function is actually used.
> > > 
> > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > > ---
> > >  drivers/net/ethernet/freescale/fec.c |   30 +++++++++++++++++++++---------
> > >  1 files changed, 21 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/net/ethernet/freescale/fec.c b/drivers/net/ethernet/freescale/fec.c
> > > index ab0afb5..70ec7ec 100644
> > > --- a/drivers/net/ethernet/freescale/fec.c
> > > +++ b/drivers/net/ethernet/freescale/fec.c
> > > @@ -235,6 +235,7 @@ struct fec_enet_private {
> > >  
> > >  	/* Phylib and MDIO interface */
> > >  	struct	mii_bus *mii_bus;
> > > +	int	mii_cnt;
> > >  	struct	phy_device *phy_dev;
> > >  	int	mii_timeout;
> > >  	uint	phy_speed;
> > > @@ -1040,8 +1041,12 @@ static int fec_enet_mii_init(struct platform_device *pdev)
> > >  	 */
> > >  	if ((id_entry->driver_data & FEC_QUIRK_ENET_MAC) && pdev->id > 0) {
> > >  		/* fec1 uses fec0 mii_bus */
> > > -		fep->mii_bus = fec0_mii_bus;
> > > -		return 0;
> > > +		if (fep->mii_cnt && fec0_mii_bus) {
> > 
> > This seems broken.  The second fec has its own fep and fep->mii_cnt is
> > always 0 here.
> > 
> I already noticed that myself.
> 
With a quick fix here, the last two patches: 

Tested-by: Shawn Guo <shawn.guo@linaro.org>

-- 
Regards,
Shawn

[PATCH][NET] several cleanups and bugfixes for fec.c: make FEC driver buildable as module

[Lothar Waßmann]

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/net/ethernet/freescale/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/freescale/Kconfig b/drivers/net/ethernet/freescale/Kconfig
index c520cfd..b02e315 100644
--- a/drivers/net/ethernet/freescale/Kconfig
+++ b/drivers/net/ethernet/freescale/Kconfig
@@ -21,7 +21,7 @@ config NET_VENDOR_FREESCALE
 if NET_VENDOR_FREESCALE
 
 config FEC
-	bool "FEC ethernet controller (of ColdFire and some i.MX CPUs)"
+	tristate "FEC ethernet controller (of ColdFire and some i.MX CPUs)"
 	depends on (M523x || M527x || M5272 || M528x || M520x || M532x || \
 		   ARCH_MXC || ARCH_MXS)
 	select PHYLIB
-- 
1.5.6.5

Re: [PATCH][NET] several cleanups and bugfixes for fec.c

[David Miller]

Please use a more reasonable Subject line for your patches.

Saying "several cleanups and bugfixes for fec.c:" in every subject
line of a whole series of patches isn't very helpful.

Just "fec: set con_id in clk_get() call to NULL", as one example, would be fine.

You will need to resubmit this entire patch series, afresh, with this fixup.
Do not try to fix it up by replying to the individual patch postings.

Re: [PATCH][NET] several cleanups and bugfixes for fec.c

[Dirk Behme]

On 06.12.2011 11:27, Lothar Waßmann wrote:
> The following set of patches provides some cleanup and bugfixes for
> drivers/net/ethernet/freescale/fec.c and makes the driver buildable as
> a module.
>
> Lothar Waßmann (8):
>    misc cleanups
>    set con_id in clk_get() call to NULL
>    prevent dobule restart of interface on FDX/HDX change
>    don't request invalid IRQ
>    don't munge MAC address from platform data
>    preserve MII/RMII setting in fec_stop()
>    fix the .remove code
>    make FEC driver buildable as module
>
>   drivers/net/ethernet/freescale/Kconfig |    2 +-
>   drivers/net/ethernet/freescale/fec.c   |   63 ++++++++++++++++++++++----------
>   2 files changed, 44 insertions(+), 21 deletions(-)

What's the status of this patch series?

If I remember correctly, most of them got an Ack or Tested-by from 
Shawn? But one or two need an update (not sure, though)? And the 
subject should be updated?

Best regards

Dirk

Re: [PATCH][NET] several cleanups and bugfixes for fec.c

[Shawn Guo]

On Thu, Dec 29, 2011 at 07:38:28AM +0100, Dirk Behme wrote:
> On 06.12.2011 11:27, Lothar Waßmann wrote:
> >The following set of patches provides some cleanup and bugfixes for
> >drivers/net/ethernet/freescale/fec.c and makes the driver buildable as
> >a module.
> >
> >Lothar Waßmann (8):
> >   misc cleanups
> >   set con_id in clk_get() call to NULL
> >   prevent dobule restart of interface on FDX/HDX change
> >   don't request invalid IRQ
> >   don't munge MAC address from platform data
> >   preserve MII/RMII setting in fec_stop()
> >   fix the .remove code
> >   make FEC driver buildable as module
> >
> >  drivers/net/ethernet/freescale/Kconfig |    2 +-
> >  drivers/net/ethernet/freescale/fec.c   |   63 ++++++++++++++++++++++----------
> >  2 files changed, 44 insertions(+), 21 deletions(-)
> 
> What's the status of this patch series?
> 
> If I remember correctly, most of them got an Ack or Tested-by from
> Shawn? But one or two need an update (not sure, though)? And the
> subject should be updated?
> 
The series with update has been picked up by David on net-next.
The LAKML was just missed from the updated posting.

-- 
Regards,
Shawn