[PATCH 0/4] add fec support for imx6q

[PATCH 0/4] add fec support for imx6q

[Shawn Guo]

This series adds imx6q enet support.  The imx6q enet is a derivative of
imx28 enet controller.  It fixed the frame endian issue found on imx28,
and added 1 Gbps support.

It's based on v3.1-rc6.

Shawn Guo (4):
      net/fec: change phy-reset-gpio request warning to debug message
      net/fec: fix fec1 check in fec_enet_mii_init()
      net/fec: set phy_speed to the optimal frequency 2.5 MHz
      net/fec: add imx6q enet support

 drivers/net/Kconfig |    3 +-
 drivers/net/fec.c   |   65 ++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 52 insertions(+), 16 deletions(-)

[PATCH 1/4] net/fec: change phy-reset-gpio request warning to debug message

[Shawn Guo]

FEC can work without a phy reset on some platforms, which means not
very platform necessarily have a phy-reset gpio encoded in device tree.
So it makes more sense to have the phy-reset-gpio request failure as
a debug message rather than a warning.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/net/fec.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index e8266cc..6a638e9 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -1422,7 +1422,7 @@ static int __devinit fec_reset_phy(struct platform_device *pdev)
 	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
 	err = gpio_request_one(phy_reset, GPIOF_OUT_INIT_LOW, "phy-reset");
 	if (err) {
-		pr_warn("FEC: failed to get gpio phy-reset: %d\n", err);
+		pr_debug("FEC: failed to get gpio phy-reset: %d\n", err);
 		return err;
 	}
 	msleep(1);
-- 
1.7.4.1

[PATCH 2/4] net/fec: fix fec1 check in fec_enet_mii_init()

[Shawn Guo]

In function fec_enet_mii_init(), it uses non-zero pdev->id as part
of the condition to check the second fec instance (fec1).  This works
before the driver supports device tree probe.  But in case of device
tree probe, pdev->id is -1 which is also non-zero, so the logic becomes
broken when device tree probe gets supported.

The patch change the logic to check "pdev->id > 0" as the part of the
condition for identifying fec1.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/net/fec.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index 6a638e9..5ef0e34 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -996,7 +996,7 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 	 * mdio interface in board design, and need to be configured by
 	 * fec0 mii_bus.
 	 */
-	if ((id_entry->driver_data & FEC_QUIRK_ENET_MAC) && pdev->id) {
+	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;
-- 
1.7.4.1

[PATCH 3/4] net/fec: set phy_speed to the optimal frequency 2.5 MHz

[Shawn Guo]

With the unnecessary 1 bit left-shift on fep->phy_speed during the
calculation, the phy_speed always runs at the half frequency of the
optimal one 2.5 MHz.

The patch removes that 1 bit left-shift to get the optimal phy_speed.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/net/fec.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index 5ef0e34..04206e4 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -1007,7 +1007,7 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 	/*
 	 * Set MII speed to 2.5 MHz (= clk_get_rate() / 2 * phy_speed)
 	 */
-	fep->phy_speed = DIV_ROUND_UP(clk_get_rate(fep->clk), 5000000) << 1;
+	fep->phy_speed = DIV_ROUND_UP(clk_get_rate(fep->clk), 5000000);
 	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
 
 	fep->mii_bus = mdiobus_alloc();
-- 
1.7.4.1

[PATCH 4/4] net/fec: add imx6q enet support

[Shawn Guo]

The imx6q enet is a derivative of imx28 enet controller.  It fixed
the frame endian issue found on imx28, and added 1 Gbps support.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/net/Kconfig |    3 +-
 drivers/net/fec.c   |   59 +++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 49 insertions(+), 13 deletions(-)

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 8d0314d..fb38962 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -1937,8 +1937,7 @@ config DECLANCE
 config FEC
 	bool "FEC ethernet controller (of ColdFire and some i.MX CPUs)"
 	depends on M523x || M527x || M5272 || M528x || M520x || M532x || \
-		IMX_HAVE_PLATFORM_FEC || MXS_HAVE_PLATFORM_FEC
-	default IMX_HAVE_PLATFORM_FEC || MXS_HAVE_PLATFORM_FEC if ARM
+		ARCH_MXC || ARCH_MXS
 	select PHYLIB
 	help
 	  Say Y here if you want to use the built-in 10/100 Fast ethernet
diff --git a/drivers/net/fec.c b/drivers/net/fec.c
index 04206e4..849cb0b 100644
--- a/drivers/net/fec.c
+++ b/drivers/net/fec.c
@@ -18,7 +18,7 @@
  * Bug fixes and cleanup by Philippe De Muyter (phdm@macqel.be)
  * Copyright (c) 2004-2006 Macq Electronique SA.
  *
- * Copyright (C) 2010 Freescale Semiconductor, Inc.
+ * Copyright (C) 2010-2011 Freescale Semiconductor, Inc.
  */
 
 #include <linux/module.h>
@@ -72,6 +72,10 @@
 #define FEC_QUIRK_SWAP_FRAME		(1 << 1)
 /* Controller uses gasket */
 #define FEC_QUIRK_USE_GASKET		(1 << 2)
+/* Controller has GBIT support */
+#define FEC_QUIRK_HAS_GBIT		(1 << 3)
+/* Controller's phy_speed bit field need to minus one */
+#define FEC_QUIRK_PHY_SPEED_MINUS_ONE	(1 << 4)
 
 static struct platform_device_id fec_devtype[] = {
 	{
@@ -88,6 +92,10 @@ static struct platform_device_id fec_devtype[] = {
 		.name = "imx28-fec",
 		.driver_data = FEC_QUIRK_ENET_MAC | FEC_QUIRK_SWAP_FRAME,
 	}, {
+		.name = "imx6q-fec",
+		.driver_data = FEC_QUIRK_ENET_MAC | FEC_QUIRK_HAS_GBIT |
+			       FEC_QUIRK_PHY_SPEED_MINUS_ONE,
+	}, {
 		/* sentinel */
 	}
 };
@@ -97,12 +105,14 @@ enum imx_fec_type {
 	IMX25_FEC = 1, 	/* runs on i.mx25/50/53 */
 	IMX27_FEC,	/* runs on i.mx27/35/51 */
 	IMX28_FEC,
+	IMX6Q_FEC,
 };
 
 static const struct of_device_id fec_dt_ids[] = {
 	{ .compatible = "fsl,imx25-fec", .data = &fec_devtype[IMX25_FEC], },
 	{ .compatible = "fsl,imx27-fec", .data = &fec_devtype[IMX27_FEC], },
 	{ .compatible = "fsl,imx28-fec", .data = &fec_devtype[IMX28_FEC], },
+	{ .compatible = "fsl,imx6q-fec", .data = &fec_devtype[IMX6Q_FEC], },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, fec_dt_ids);
@@ -373,6 +383,7 @@ fec_restart(struct net_device *ndev, int duplex)
 	int i;
 	u32 temp_mac[2];
 	u32 rcntl = OPT_FRAME_SIZE | 0x04;
+	u32 ecntl = 0x2; /* ETHEREN */
 
 	/* Whack a reset.  We should wait for this. */
 	writel(1, fep->hwp + FEC_ECNTRL);
@@ -442,18 +453,23 @@ fec_restart(struct net_device *ndev, int duplex)
 		/* Enable flow control and length check */
 		rcntl |= 0x40000000 | 0x00000020;
 
-		/* MII or RMII */
+		/* RGMII, RMII or MII */
+		if (fep->phy_interface == PHY_INTERFACE_MODE_RGMII)
+			rcntl |= (1 << 6);
 		if (fep->phy_interface == PHY_INTERFACE_MODE_RMII)
 			rcntl |= (1 << 8);
 		else
 			rcntl &= ~(1 << 8);
 
-		/* 10M or 100M */
-		if (fep->phy_dev && fep->phy_dev->speed == SPEED_100)
-			rcntl &= ~(1 << 9);
-		else
-			rcntl |= (1 << 9);
-
+		/* 1G, 100M or 10M */
+		if (fep->phy_dev) {
+			if (fep->phy_dev->speed == SPEED_1000)
+				ecntl |= (1 << 8);
+			else if (fep->phy_dev->speed == SPEED_100)
+				rcntl &= ~(1 << 9);
+			else
+				rcntl |= (1 << 9);
+		}
 	} else {
 #ifdef FEC_MIIGSK_ENR
 		if (id_entry->driver_data & FEC_QUIRK_USE_GASKET) {
@@ -478,8 +494,15 @@ fec_restart(struct net_device *ndev, int duplex)
 	}
 	writel(rcntl, fep->hwp + FEC_R_CNTRL);
 
+	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) {
+		/* enable ENET endian swap */
+		ecntl |= (1 << 8);
+		/* enable ENET store and forward mode */
+		writel(1 << 8, fep->hwp + FEC_X_WMRK);
+	}
+
 	/* And last, enable the transmit and receive processing */
-	writel(2, fep->hwp + FEC_ECNTRL);
+	writel(ecntl, fep->hwp + FEC_ECNTRL);
 	writel(0, fep->hwp + FEC_R_DES_ACTIVE);
 
 	/* Enable interrupts we wish to service */
@@ -490,6 +513,8 @@ static void
 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);
 
 	/* We cannot expect a graceful transmit stop without link !!! */
 	if (fep->link) {
@@ -504,6 +529,10 @@ fec_stop(struct net_device *ndev)
 	udelay(10);
 	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
 	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)
+		writel(2, fep->hwp + FEC_ECNTRL);
 }
 
 
@@ -918,6 +947,8 @@ static int fec_enet_mdio_reset(struct mii_bus *bus)
 static int fec_enet_mii_probe(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);
 	struct phy_device *phy_dev = NULL;
 	char mdio_bus_id[MII_BUS_ID_SIZE];
 	char phy_name[MII_BUS_ID_SIZE + 3];
@@ -949,14 +980,18 @@ static int fec_enet_mii_probe(struct net_device *ndev)
 
 	snprintf(phy_name, MII_BUS_ID_SIZE, PHY_ID_FMT, mdio_bus_id, phy_id);
 	phy_dev = phy_connect(ndev, phy_name, &fec_enet_adjust_link, 0,
-		PHY_INTERFACE_MODE_MII);
+			      fep->phy_interface);
 	if (IS_ERR(phy_dev)) {
 		printk(KERN_ERR "%s: could not attach to PHY\n", ndev->name);
 		return PTR_ERR(phy_dev);
 	}
 
 	/* mask with MAC supported features */
-	phy_dev->supported &= PHY_BASIC_FEATURES;
+	if (id_entry->driver_data & FEC_QUIRK_HAS_GBIT)
+		phy_dev->supported &= PHY_GBIT_FEATURES;
+	else
+		phy_dev->supported &= PHY_BASIC_FEATURES;
+
 	phy_dev->advertising = phy_dev->supported;
 
 	fep->phy_dev = phy_dev;
@@ -1008,6 +1043,8 @@ static int fec_enet_mii_init(struct platform_device *pdev)
 	 * Set MII speed to 2.5 MHz (= clk_get_rate() / 2 * phy_speed)
 	 */
 	fep->phy_speed = DIV_ROUND_UP(clk_get_rate(fep->clk), 5000000);
+	if (id_entry->driver_data & FEC_QUIRK_PHY_SPEED_MINUS_ONE)
+		fep->phy_speed--;
 	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
 
 	fep->mii_bus = mdiobus_alloc();
-- 
1.7.4.1

Re: [PATCH 4/4] net/fec: add imx6q enet support

[Francois Romieu]

Shawn Guo <shawn.guo@linaro.org> :
[...]
> diff --git a/drivers/net/fec.c b/drivers/net/fec.c
> index 04206e4..849cb0b 100644
> --- a/drivers/net/fec.c
> +++ b/drivers/net/fec.c
> @@ -442,18 +453,23 @@ fec_restart(struct net_device *ndev, int duplex)
>  		/* Enable flow control and length check */
>  		rcntl |= 0x40000000 | 0x00000020;
>  
> -		/* MII or RMII */
> +		/* RGMII, RMII or MII */
> +		if (fep->phy_interface == PHY_INTERFACE_MODE_RGMII)
> +			rcntl |= (1 << 6);
>  		if (fep->phy_interface == PHY_INTERFACE_MODE_RMII)
           ^^^^ missing "else"

[...]
> +		/* 1G, 100M or 10M */
> +		if (fep->phy_dev) {
> +			if (fep->phy_dev->speed == SPEED_1000)
> +				ecntl |= (1 << 8);
> +			else if (fep->phy_dev->speed == SPEED_100)
> +				rcntl &= ~(1 << 9);
> +			else
> +				rcntl |= (1 << 9);
> +		}
[...]
> +	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) {
> +		/* enable ENET endian swap */
> +		ecntl |= (1 << 8);

I do not understand why the endian swap bit of ecntl needs to be
set the same in these two different paths, especially as the latter
handles the old faulty imx28 and the former the newly fixed imx6q.
Typo ?

-- 
Ueimor

Re: [PATCH 4/4] net/fec: add imx6q enet support

[Shawn Guo]

On Sun, Sep 18, 2011 at 08:09:12PM +0200, Francois Romieu wrote:
> Shawn Guo <shawn.guo@linaro.org> :
> [...]
> > diff --git a/drivers/net/fec.c b/drivers/net/fec.c
> > index 04206e4..849cb0b 100644
> > --- a/drivers/net/fec.c
> > +++ b/drivers/net/fec.c
> > @@ -442,18 +453,23 @@ fec_restart(struct net_device *ndev, int duplex)
> >  		/* Enable flow control and length check */
> >  		rcntl |= 0x40000000 | 0x00000020;
> >  
> > -		/* MII or RMII */
> > +		/* RGMII, RMII or MII */
> > +		if (fep->phy_interface == PHY_INTERFACE_MODE_RGMII)
> > +			rcntl |= (1 << 6);
> >  		if (fep->phy_interface == PHY_INTERFACE_MODE_RMII)
>            ^^^^ missing "else"
> 
Yes, my bad.

> [...]
> > +		/* 1G, 100M or 10M */
> > +		if (fep->phy_dev) {
> > +			if (fep->phy_dev->speed == SPEED_1000)
> > +				ecntl |= (1 << 8);

Right, this is a typo.  It should be (1 << 5);

> > +			else if (fep->phy_dev->speed == SPEED_100)
> > +				rcntl &= ~(1 << 9);
> > +			else
> > +				rcntl |= (1 << 9);
> > +		}
> [...]
> > +	if (id_entry->driver_data & FEC_QUIRK_ENET_MAC) {
> > +		/* enable ENET endian swap */
> > +		ecntl |= (1 << 8);
> 
> I do not understand why the endian swap bit of ecntl needs to be
> set the same in these two different paths, especially as the latter
> handles the old faulty imx28 and the former the newly fixed imx6q.
> Typo ?
> 
Nice catches.  Thanks a lot, Ueimor.

-- 
Regards,
Shawn

Re: [PATCH 3/4] net/fec: set phy_speed to the optimal frequency 2.5 MHz

[Troy Kisky]

On 9/18/2011 4:54 AM, Shawn Guo wrote:
> With the unnecessary 1 bit left-shift on fep->phy_speed during the
> calculation, the phy_speed always runs at the half frequency of the
> optimal one 2.5 MHz.
>
> The patch removes that 1 bit left-shift to get the optimal phy_speed.
>
> Signed-off-by: Shawn Guo<shawn.guo@linaro.org>
> ---
>   drivers/net/fec.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/fec.c b/drivers/net/fec.c
> index 5ef0e34..04206e4 100644
> --- a/drivers/net/fec.c
> +++ b/drivers/net/fec.c
> @@ -1007,7 +1007,7 @@ static int fec_enet_mii_init(struct platform_device *pdev)
>   	/*
>   	 * Set MII speed to 2.5 MHz (= clk_get_rate() / 2 * phy_speed)
>   	 */
> -	fep->phy_speed = DIV_ROUND_UP(clk_get_rate(fep->clk), 5000000)<<  1;
> +	fep->phy_speed = DIV_ROUND_UP(clk_get_rate(fep->clk), 5000000);
>   	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
>
>   	fep->mii_bus = mdiobus_alloc();
Do you need to round up to an even value? Is the hardware documentation 
wrong?
Does this need a quirk? What boards has this been verified to fix?

Thanks
Troy

Re: [PATCH 3/4] net/fec: set phy_speed to the optimal frequency 2.5 MHz

[Shawn Guo]

On Mon, Sep 19, 2011 at 03:39:30PM -0700, Troy Kisky wrote:
> On 9/18/2011 4:54 AM, Shawn Guo wrote:
> >With the unnecessary 1 bit left-shift on fep->phy_speed during the
> >calculation, the phy_speed always runs at the half frequency of the
> >optimal one 2.5 MHz.
> >
> >The patch removes that 1 bit left-shift to get the optimal phy_speed.
> >
> >Signed-off-by: Shawn Guo<shawn.guo@linaro.org>
> >---
> >  drivers/net/fec.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> >
> >diff --git a/drivers/net/fec.c b/drivers/net/fec.c
> >index 5ef0e34..04206e4 100644
> >--- a/drivers/net/fec.c
> >+++ b/drivers/net/fec.c
> >@@ -1007,7 +1007,7 @@ static int fec_enet_mii_init(struct platform_device *pdev)
> >  	/*
> >  	 * Set MII speed to 2.5 MHz (= clk_get_rate() / 2 * phy_speed)
> >  	 */
> >-	fep->phy_speed = DIV_ROUND_UP(clk_get_rate(fep->clk), 5000000)<<  1;
> >+	fep->phy_speed = DIV_ROUND_UP(clk_get_rate(fep->clk), 5000000);
> >  	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> >
> >  	fep->mii_bus = mdiobus_alloc();
> Do you need to round up to an even value? Is the hardware
> documentation wrong?

The round up is something existed, and the patch does not touch that
part.

> Does this need a quirk? What boards has this been verified to fix?
> 

I tested this on i.mx28, i.mx53 and i.mx6q.  Do you see problem on
your platform?

-- 
Regards,
Shawn

Re: [PATCH 3/4] net/fec: set phy_speed to the optimal frequency 2.5 MHz

[Lothar Waßmann]

Hi,

Shawn Guo writes:
> With the unnecessary 1 bit left-shift on fep->phy_speed during the
> calculation, the phy_speed always runs at the half frequency of the
> optimal one 2.5 MHz.
> 
> The patch removes that 1 bit left-shift to get the optimal phy_speed.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/net/fec.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/fec.c b/drivers/net/fec.c
> index 5ef0e34..04206e4 100644
> --- a/drivers/net/fec.c
> +++ b/drivers/net/fec.c
> @@ -1007,7 +1007,7 @@ static int fec_enet_mii_init(struct platform_device *pdev)
>  	/*
>  	 * Set MII speed to 2.5 MHz (= clk_get_rate() / 2 * phy_speed)
>  	 */
> -	fep->phy_speed = DIV_ROUND_UP(clk_get_rate(fep->clk), 5000000) << 1;
> +	fep->phy_speed = DIV_ROUND_UP(clk_get_rate(fep->clk), 5000000);
>  	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
>  
>  	fep->mii_bus = mdiobus_alloc();
>
The left shift accounts for the fact, that the MII_SPEED bitfield
starts at pos 1 in the register. Thus the divider value has to be
shifted left to occupy the correct bit positions in the register.

According to my measurements on the TX28 the original code works
correctly!
Did you measure the actual frequency on the MDC pin after you change?


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
___________________________________________________________

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/4] net/fec: set phy_speed to the optimal frequency 2.5 MHz

[Shawn Guo]

On Tue, Sep 20, 2011 at 09:50:17AM +0200, Lothar Waßmann wrote:
> Hi,
> 
> Shawn Guo writes:
> > With the unnecessary 1 bit left-shift on fep->phy_speed during the
> > calculation, the phy_speed always runs at the half frequency of the
> > optimal one 2.5 MHz.
> > 
> > The patch removes that 1 bit left-shift to get the optimal phy_speed.
> > 
> > Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> > ---
> >  drivers/net/fec.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/net/fec.c b/drivers/net/fec.c
> > index 5ef0e34..04206e4 100644
> > --- a/drivers/net/fec.c
> > +++ b/drivers/net/fec.c
> > @@ -1007,7 +1007,7 @@ static int fec_enet_mii_init(struct platform_device *pdev)
> >  	/*
> >  	 * Set MII speed to 2.5 MHz (= clk_get_rate() / 2 * phy_speed)
> >  	 */
> > -	fep->phy_speed = DIV_ROUND_UP(clk_get_rate(fep->clk), 5000000) << 1;
> > +	fep->phy_speed = DIV_ROUND_UP(clk_get_rate(fep->clk), 5000000);
> >  	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
> >  
> >  	fep->mii_bus = mdiobus_alloc();
> >
> The left shift accounts for the fact, that the MII_SPEED bitfield
> starts at pos 1 in the register. Thus the divider value has to be
> shifted left to occupy the correct bit positions in the register.
> 
Oops, I missed that.

> According to my measurements on the TX28 the original code works
> correctly!
> Did you measure the actual frequency on the MDC pin after you change?
> 
I should have done that before sending this patch.  I'm working home
these days and have not got the chance get into the lab.  Yes, I
should have sent this patch as an RFC at least.  Sorry about this,
and thank you for pointing this out.

Will drop this patch from the v2 of the series.

-- 
Regards,
Shawn

Re: [PATCH 0/4] add fec support for imx6q

[David Miller]

From: Shawn Guo <shawn.guo@linaro.org>
Date: Sun, 18 Sep 2011 19:54:08 +0800

> This series adds imx6q enet support.  The imx6q enet is a derivative of
> imx28 enet controller.  It fixed the frame endian issue found on imx28,
> and added 1 Gbps support.
> 
> It's based on v3.1-rc6.
> 
> Shawn Guo (4):
>       net/fec: change phy-reset-gpio request warning to debug message
>       net/fec: fix fec1 check in fec_enet_mii_init()
>       net/fec: set phy_speed to the optimal frequency 2.5 MHz
>       net/fec: add imx6q enet support

Please respin this against the net-next tree as that's where these
changes should be targetted and all the ethernet drivers have moved
to different directories in net-next.

Thanks.

Re: [PATCH 3/4] net/fec: set phy_speed to the optimal frequency 2.5 MHz

[David Miller]

From: Shawn Guo <shawn.guo@freescale.com>
Date: Tue, 20 Sep 2011 16:14:39 +0800

> I should have done that before sending this patch.  I'm working home
> these days and have not got the chance get into the lab.  Yes, I
> should have sent this patch as an RFC at least.  Sorry about this,
> and thank you for pointing this out.
> 
> Will drop this patch from the v2 of the series.

As mentioned you need to respin this anyways against net-next

Re: [PATCH 3/4] net/fec: set phy_speed to the optimal frequency 2.5 MHz

[Troy Kisky]

On 9/19/2011 7:57 PM, Shawn Guo wrote:
> On Mon, Sep 19, 2011 at 03:39:30PM -0700, Troy Kisky wrote:
>> On 9/18/2011 4:54 AM, Shawn Guo wrote:
>>> With the unnecessary 1 bit left-shift on fep->phy_speed during the
>>> calculation, the phy_speed always runs at the half frequency of the
>>> optimal one 2.5 MHz.
>>>
>>> The patch removes that 1 bit left-shift to get the optimal phy_speed.
>>>
>>> Signed-off-by: Shawn Guo<shawn.guo@linaro.org>
>>> ---
>>>   drivers/net/fec.c |    2 +-
>>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/net/fec.c b/drivers/net/fec.c
>>> index 5ef0e34..04206e4 100644
>>> --- a/drivers/net/fec.c
>>> +++ b/drivers/net/fec.c
>>> @@ -1007,7 +1007,7 @@ static int fec_enet_mii_init(struct platform_device *pdev)
>>>   	/*
>>>   	 * Set MII speed to 2.5 MHz (= clk_get_rate() / 2 * phy_speed)
>>>   	 */
>>> -	fep->phy_speed = DIV_ROUND_UP(clk_get_rate(fep->clk), 5000000)<<   1;
>>> +	fep->phy_speed = DIV_ROUND_UP(clk_get_rate(fep->clk), 5000000);
>>>   	writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
>>>
>>>   	fep->mii_bus = mdiobus_alloc();
>> Do you need to round up to an even value? Is the hardware
>> documentation wrong?
> The round up is something existed, and the patch does not touch that
> part.
That's not what I was referring to. Previously, phy_speed was always 
even because of the shift.
The MX53 manual says this field starts at bit 1, and bit 0 is unused. 
Therefore, maybe the
correct change would be

fep->phy_speed = DIV_ROUND_UP(clk_get_rate(fep->clk), 2500000)<<   1;

So, the question is, does this field start at bit 0 (your version is correct)
or bit 1? In other words, how did the hardware manual get it wrong? Wrong starting
bit, or divide by 2 not needed. Please document the mistake in the code.


>
>> Does this need a quirk? What boards has this been verified to fix?
>>
> I tested this on i.mx28, i.mx53 and i.mx6q.  Do you see problem on
> your platform?
>
I have not tested yet, but will sometime this week.

Re: [PATCH 3/4] net/fec: set phy_speed to the optimal frequency 2.5 MHz

[Troy Kisky]

On 9/20/2011 1:05 PM, Troy Kisky wrote:
> On 9/19/2011 7:57 PM, Shawn Guo wrote:
>> On Mon, Sep 19, 2011 at 03:39:30PM -0700, Troy Kisky wrote:
>>> On 9/18/2011 4:54 AM, Shawn Guo wrote:
>>>> With the unnecessary 1 bit left-shift on fep->phy_speed during the
>>>> calculation, the phy_speed always runs at the half frequency of the
>>>> optimal one 2.5 MHz.
>>>>
>>>> The patch removes that 1 bit left-shift to get the optimal phy_speed.
>>>>
>>>> Signed-off-by: Shawn Guo<shawn.guo@linaro.org>
>>>> ---
>>>>   drivers/net/fec.c |    2 +-
>>>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/net/fec.c b/drivers/net/fec.c
>>>> index 5ef0e34..04206e4 100644
>>>> --- a/drivers/net/fec.c
>>>> +++ b/drivers/net/fec.c
>>>> @@ -1007,7 +1007,7 @@ static int fec_enet_mii_init(struct 
>>>> platform_device *pdev)
>>>>       /*
>>>>        * Set MII speed to 2.5 MHz (= clk_get_rate() / 2 * phy_speed)
>>>>        */
>>>> -    fep->phy_speed = DIV_ROUND_UP(clk_get_rate(fep->clk), 
>>>> 5000000)<<   1;
>>>> +    fep->phy_speed = DIV_ROUND_UP(clk_get_rate(fep->clk), 5000000);
>>>>       writel(fep->phy_speed, fep->hwp + FEC_MII_SPEED);
>>>>
>>>>       fep->mii_bus = mdiobus_alloc();
>>> Do you need to round up to an even value? Is the hardware
>>> documentation wrong?
>> The round up is something existed, and the patch does not touch that
>> part.
> That's not what I was referring to. Previously, phy_speed was always 
> even because of the shift.
> The MX53 manual says this field starts at bit 1, and bit 0 is unused. 
> Therefore, maybe the
> correct change would be
>
> fep->phy_speed = DIV_ROUND_UP(clk_get_rate(fep->clk), 2500000)<<   1;

oops, I meant
fep->phy_speed = DIV_ROUND_UP(clk_get_rate(fep->clk), 2500000 * 4) <<   1;
> So, the question is, does this field start at bit 0 (your version is 
> correct)
> or bit 1? In other words, how did the hardware manual get it wrong? 
> Wrong starting
> bit, or divide by 2 not needed. Please document the mistake in the code.
>
>
>>
>>> Does this need a quirk? What boards has this been verified to fix?
>>>
>> I tested this on i.mx28, i.mx53 and i.mx6q.  Do you see problem on
>> your platform?
>>
> I have not tested yet, but will sometime this week.
>
>
>