[PATCH v2] fs_enet/mii-fec.c: fix MII speed calculation

[PATCH v2] fs_enet/mii-fec.c: fix MII speed calculation

[Wolfgang Denk]

The MII speed calculation was based on the CPU clock (ppc_proc_freq),
but for MPC512x we must use the bus clock instead.

This patch makes it use the correct clock and makes sure we don't
clobber reserved bits in the MII_SPEED register.

Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: <netdev@vger.kernel.org>
---
Note 1: This patch is a rewrite of a previous posting:
        http://patchwork.ozlabs.org/patch/28584/
Note 2: Function mpc5xxx_get_mii_speed() was introduced for re-use
        in a later patch to drivers/net/fec_mpc52xx.c and
	drivers/net/fec_mpc52xx_phy.c, which also contain code to
	calculate the MII speed without taking care to check for
	overflow or to write only the bits that belong to the
	MII_SPEED field when setting the MII speed.

 arch/powerpc/sysdev/mpc5xxx_clocks.c |   37 ++++++++++++++++++++++++++++++++++
 drivers/net/fs_enet/mii-fec.c        |   13 +++++++++--
 2 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/sysdev/mpc5xxx_clocks.c b/arch/powerpc/sysdev/mpc5xxx_clocks.c
index 34e12f9..e26d12b 100644
--- a/arch/powerpc/sysdev/mpc5xxx_clocks.c
+++ b/arch/powerpc/sysdev/mpc5xxx_clocks.c
@@ -31,3 +31,40 @@ mpc5xxx_get_bus_frequency(struct device_node *node)
 	return p_bus_freq ? *p_bus_freq : 0;
 }
 EXPORT_SYMBOL(mpc5xxx_get_bus_frequency);
+
+/**
+ *	mpc5xxx_get_mii_speed - Get the MII_SPEED value
+ *	@node:	device node
+ *
+ *	Returns the MII_SPEED value for MPC512x and MPC52xx systems.
+ *	The value gets computed such that the resulting MDC frequency
+ *	is 2.5 MHz or lower.
+ */
+
+int
+mpc5xxx_get_mii_speed(struct of_device *ofdev)
+{
+	unsigned int clock, speed;
+
+	clock = mpc5xxx_get_bus_frequency(ofdev->node);
+
+	if (!clock) {
+		dev_err(&ofdev->dev, "could not determine IPS/IPB clock\n");
+		return -ENODEV;
+	}
+
+	/* scale for a MII clock <= 2.5 MHz */
+	speed = (clock + 2499999) / 2500000;
+
+	/* only 6 bits available for MII speed */
+	if (speed > 0x3F) {
+		speed = 0x3F;
+		dev_err(&ofdev->dev,
+			"MII clock (%d MHz) exceeds max (2.5 MHz)\n",
+			clock / speed);
+	}
+
+	/* Field is in bits 25:30 of MII_SPEED register */
+	return speed << 1;
+}
+EXPORT_SYMBOL(mpc5xxx_get_mii_speed);
diff --git a/drivers/net/fs_enet/mii-fec.c b/drivers/net/fs_enet/mii-fec.c
index 75a0999..a28d39f 100644
--- a/drivers/net/fs_enet/mii-fec.c
+++ b/drivers/net/fs_enet/mii-fec.c
@@ -36,6 +36,7 @@
 #include <asm/pgtable.h>
 #include <asm/irq.h>
 #include <asm/uaccess.h>
+#include <asm/mpc5xxx.h>
 
 #include "fs_enet.h"
 #include "fec.h"
@@ -103,7 +104,6 @@ static int fs_enet_fec_mii_reset(struct mii_bus *bus)
 static int __devinit fs_enet_mdio_probe(struct of_device *ofdev,
                                         const struct of_device_id *match)
 {
-	struct device_node *np = NULL;
 	struct resource res;
 	struct mii_bus *new_bus;
 	struct fec_info *fec;
@@ -133,13 +133,20 @@ static int __devinit fs_enet_mdio_probe(struct of_device *ofdev,
 	if (!fec->fecp)
 		goto out_fec;
 
-	fec->mii_speed = ((ppc_proc_freq + 4999999) / 5000000) << 1;
+	if (of_device_is_compatible(ofdev->node, "fsl,mpc5121-fec-mdio")) {
+		i = mpc5xxx_get_mii_speed(ofdev);
+		if (i < 0)
+			goto out_unmap_regs;
+		fec->mii_speed = i;
+	} else {
+		fec->mii_speed = ((ppc_proc_freq + 4999999) / 5000000) << 1;
+	}
 
 	setbits32(&fec->fecp->fec_r_cntrl, FEC_RCNTRL_MII_MODE);
 	setbits32(&fec->fecp->fec_ecntrl, FEC_ECNTRL_PINMUX |
 	                                  FEC_ECNTRL_ETHER_EN);
 	out_be32(&fec->fecp->fec_ievent, FEC_ENET_MII);
-	out_be32(&fec->fecp->fec_mii_speed, fec->mii_speed);
+	clrsetbits_be32(&fec->fecp->fec_mii_speed, 0x7E, fec->mii_speed);
 
 	new_bus->phy_mask = ~0;
 	new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL);
-- 
1.6.0.6

[PATCH 1/2 v3] fs_enet/mii-fec.c: fix MII speed calculation

[Wolfgang Denk]

The MII speed calculation was based on the CPU clock (ppc_proc_freq),
but for MPC512x we must use the bus clock instead.

This patch makes it use the correct clock and makes sure we don't
clobber reserved bits in the MII_SPEED register.

Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: <netdev@vger.kernel.org>
---
Please ignore patch v2, it's crap.
Hope this is a bit better.

 arch/powerpc/include/asm/mpc5xxx.h   |   10 +++++++++
 arch/powerpc/sysdev/mpc5xxx_clocks.c |   37 ++++++++++++++++++++++++++++++++++
 drivers/net/fs_enet/mii-fec.c        |   13 +++++++++--
 3 files changed, 57 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/mpc5xxx.h b/arch/powerpc/include/asm/mpc5xxx.h
index 5ce9c5f..86ab29f 100644
--- a/arch/powerpc/include/asm/mpc5xxx.h
+++ b/arch/powerpc/include/asm/mpc5xxx.h
@@ -15,8 +15,18 @@
 
 #ifndef __ASM_POWERPC_MPC5xxx_H__
 #define __ASM_POWERPC_MPC5xxx_H__
+#include <linux/of_platform.h>
 
 extern unsigned long mpc5xxx_get_bus_frequency(struct device_node *node);
 
+#if defined(CONFIG_PPC_MPC512x) || defined(CONFIG_PPC_MPC52xx)
+extern int mpc5xxx_get_mii_speed(struct of_device *ofdev);
+#else
+static inline int mpc5xxx_get_mii_speed(struct of_device *ofdev)
+{
+	return -1;
+}
+#endif
+
 #endif /* __ASM_POWERPC_MPC5xxx_H__ */
 
diff --git a/arch/powerpc/sysdev/mpc5xxx_clocks.c b/arch/powerpc/sysdev/mpc5xxx_clocks.c
index 34e12f9..e26d12b 100644
--- a/arch/powerpc/sysdev/mpc5xxx_clocks.c
+++ b/arch/powerpc/sysdev/mpc5xxx_clocks.c
@@ -31,3 +31,40 @@ mpc5xxx_get_bus_frequency(struct device_node *node)
 	return p_bus_freq ? *p_bus_freq : 0;
 }
 EXPORT_SYMBOL(mpc5xxx_get_bus_frequency);
+
+/**
+ *	mpc5xxx_get_mii_speed - Get the MII_SPEED value
+ *	@node:	device node
+ *
+ *	Returns the MII_SPEED value for MPC512x and MPC52xx systems.
+ *	The value gets computed such that the resulting MDC frequency
+ *	is 2.5 MHz or lower.
+ */
+
+int
+mpc5xxx_get_mii_speed(struct of_device *ofdev)
+{
+	unsigned int clock, speed;
+
+	clock = mpc5xxx_get_bus_frequency(ofdev->node);
+
+	if (!clock) {
+		dev_err(&ofdev->dev, "could not determine IPS/IPB clock\n");
+		return -ENODEV;
+	}
+
+	/* scale for a MII clock <= 2.5 MHz */
+	speed = (clock + 2499999) / 2500000;
+
+	/* only 6 bits available for MII speed */
+	if (speed > 0x3F) {
+		speed = 0x3F;
+		dev_err(&ofdev->dev,
+			"MII clock (%d MHz) exceeds max (2.5 MHz)\n",
+			clock / speed);
+	}
+
+	/* Field is in bits 25:30 of MII_SPEED register */
+	return speed << 1;
+}
+EXPORT_SYMBOL(mpc5xxx_get_mii_speed);
diff --git a/drivers/net/fs_enet/mii-fec.c b/drivers/net/fs_enet/mii-fec.c
index 75a0999..a28d39f 100644
--- a/drivers/net/fs_enet/mii-fec.c
+++ b/drivers/net/fs_enet/mii-fec.c
@@ -36,6 +36,7 @@
 #include <asm/pgtable.h>
 #include <asm/irq.h>
 #include <asm/uaccess.h>
+#include <asm/mpc5xxx.h>
 
 #include "fs_enet.h"
 #include "fec.h"
@@ -103,7 +104,6 @@ static int fs_enet_fec_mii_reset(struct mii_bus *bus)
 static int __devinit fs_enet_mdio_probe(struct of_device *ofdev,
                                         const struct of_device_id *match)
 {
-	struct device_node *np = NULL;
 	struct resource res;
 	struct mii_bus *new_bus;
 	struct fec_info *fec;
@@ -133,13 +133,20 @@ static int __devinit fs_enet_mdio_probe(struct of_device *ofdev,
 	if (!fec->fecp)
 		goto out_fec;
 
-	fec->mii_speed = ((ppc_proc_freq + 4999999) / 5000000) << 1;
+	if (of_device_is_compatible(ofdev->node, "fsl,mpc5121-fec-mdio")) {
+		i = mpc5xxx_get_mii_speed(ofdev);
+		if (i < 0)
+			goto out_unmap_regs;
+		fec->mii_speed = i;
+	} else {
+		fec->mii_speed = ((ppc_proc_freq + 4999999) / 5000000) << 1;
+	}
 
 	setbits32(&fec->fecp->fec_r_cntrl, FEC_RCNTRL_MII_MODE);
 	setbits32(&fec->fecp->fec_ecntrl, FEC_ECNTRL_PINMUX |
 	                                  FEC_ECNTRL_ETHER_EN);
 	out_be32(&fec->fecp->fec_ievent, FEC_ENET_MII);
-	out_be32(&fec->fecp->fec_mii_speed, fec->mii_speed);
+	clrsetbits_be32(&fec->fecp->fec_mii_speed, 0x7E, fec->mii_speed);
 
 	new_bus->phy_mask = ~0;
 	new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL);
-- 
1.6.0.6

[PATCH 2/2] MPC52xx FEC: be more conservative when setting MII_SPEED register

[Wolfgang Denk]

This patch adds error checking and prevents clobbering unrelated bits
(reserved bits or the DIS_PREAMBLE bit) when writing the MII_SPEED
register on MPC52xx systems.

Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: <netdev@vger.kernel.org>
---
 drivers/net/fec_mpc52xx.c     |    2 +-
 drivers/net/fec_mpc52xx_phy.c |    6 ++++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/fec_mpc52xx.c b/drivers/net/fec_mpc52xx.c
index cc78633..b69d440 100644
--- a/drivers/net/fec_mpc52xx.c
+++ b/drivers/net/fec_mpc52xx.c
@@ -639,7 +639,7 @@ static void mpc52xx_fec_hw_init(struct net_device *dev)
 	/* set phy speed.
 	 * this can't be done in phy driver, since it needs to be called
 	 * before fec stuff (even on resume) */
-	out_be32(&fec->mii_speed, priv->mdio_speed);
+	clrsetbits_be32(&fec->mii_speed, 0x7E, priv->mdio_speed);
 }
 
 /**
diff --git a/drivers/net/fec_mpc52xx_phy.c b/drivers/net/fec_mpc52xx_phy.c
index 31e6d62..f733d43 100644
--- a/drivers/net/fec_mpc52xx_phy.c
+++ b/drivers/net/fec_mpc52xx_phy.c
@@ -105,8 +105,10 @@ static int mpc52xx_fec_mdio_probe(struct of_device *of,
 	dev_set_drvdata(dev, bus);
 
 	/* set MII speed */
-	out_be32(&priv->regs->mii_speed,
-		((mpc5xxx_get_bus_frequency(of->node) >> 20) / 5) << 1);
+	i = mpc5xxx_get_mii_speed(of);
+	if (i<0)
+		goto out_unmap;
+	clrsetbits_be32(&priv->regs->mii_speed, 0x7E, i);
 
 	err = of_mdiobus_register(bus, np);
 	if (err)
-- 
1.6.0.6

Re: [PATCH 1/2 v3] fs_enet/mii-fec.c: fix MII speed calculation

[Grant Likely]

On Wed, Jul 15, 2009 at 9:18 AM, Wolfgang Denk<wd@denx.de> wrote:
> The MII speed calculation was based on the CPU clock (ppc_proc_freq),
> but for MPC512x we must use the bus clock instead.
>
> This patch makes it use the correct clock and makes sure we don't
> clobber reserved bits in the MII_SPEED register.
>
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> Cc: <netdev@vger.kernel.org>
> ---
> Please ignore patch v2, it's crap.
> Hope this is a bit better.
>
>  arch/powerpc/include/asm/mpc5xxx.h   |   10 +++++++++
>  arch/powerpc/sysdev/mpc5xxx_clocks.c |   37 ++++++++++++++++++++++++++++++++++

Drop the common code bit.  The 5200 and 5121 are different devices and
it is a tiny bit of code.  I don't think there is any benefit to
having it as a common function.  Just roll the get_mii_speed function
in the mii-fec driver itself.

Also, this patch can be quite a bit simpler if you use the .data
pointer in the drivers match table to specify the function used to
return the bus clock speed.  Something like this:

static struct of_device_id fs_enet_mdio_fec_match[] = {
	{
		.compatible = "fsl,pq1-fec-mdio",
	},
#if defined(CONFIG_PPC_MPC512x)
	{
		.compatible = "fsl,mpc5121-fec-mdio",
		.data = mpc5xxx_get_bus_frequency,
	},
#endif
	{},
};

and

int *get_bus_freq(of_node *) = data;
if (get_bus_freq)
        bus_freq = get_bus_freq(np);
else
        bus_freq = ppc_proc_freq / 2;

... then do the regular calculation here and add in the additional
robustification you did in this patch.

Heck, you could even eliminate the if/else above if the normal case
you have a get_bus_speed function for the original ppc_proc_freq case,
but I'm not sure if it is worth it.


>  drivers/net/fs_enet/mii-fec.c        |   13 +++++++++--
>  3 files changed, 57 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/mpc5xxx.h b/arch/powerpc/include/asm/mpc5xxx.h
> index 5ce9c5f..86ab29f 100644
> --- a/arch/powerpc/include/asm/mpc5xxx.h
> +++ b/arch/powerpc/include/asm/mpc5xxx.h
> @@ -15,8 +15,18 @@
>
>  #ifndef __ASM_POWERPC_MPC5xxx_H__
>  #define __ASM_POWERPC_MPC5xxx_H__
> +#include <linux/of_platform.h>
>
>  extern unsigned long mpc5xxx_get_bus_frequency(struct device_node *node);
>
> +#if defined(CONFIG_PPC_MPC512x) || defined(CONFIG_PPC_MPC52xx)
> +extern int mpc5xxx_get_mii_speed(struct of_device *ofdev);
> +#else
> +static inline int mpc5xxx_get_mii_speed(struct of_device *ofdev)
> +{
> +       return -1;
> +}
> +#endif
> +
>  #endif /* __ASM_POWERPC_MPC5xxx_H__ */
>
> diff --git a/arch/powerpc/sysdev/mpc5xxx_clocks.c b/arch/powerpc/sysdev/mpc5xxx_clocks.c
> index 34e12f9..e26d12b 100644
> --- a/arch/powerpc/sysdev/mpc5xxx_clocks.c
> +++ b/arch/powerpc/sysdev/mpc5xxx_clocks.c
> @@ -31,3 +31,40 @@ mpc5xxx_get_bus_frequency(struct device_node *node)
>        return p_bus_freq ? *p_bus_freq : 0;
>  }
>  EXPORT_SYMBOL(mpc5xxx_get_bus_frequency);
> +
> +/**
> + *     mpc5xxx_get_mii_speed - Get the MII_SPEED value
> + *     @node:  device node
> + *
> + *     Returns the MII_SPEED value for MPC512x and MPC52xx systems.
> + *     The value gets computed such that the resulting MDC frequency
> + *     is 2.5 MHz or lower.
> + */
> +
> +int
> +mpc5xxx_get_mii_speed(struct of_device *ofdev)
> +{
> +       unsigned int clock, speed;
> +
> +       clock = mpc5xxx_get_bus_frequency(ofdev->node);
> +
> +       if (!clock) {
> +               dev_err(&ofdev->dev, "could not determine IPS/IPB clock\n");
> +               return -ENODEV;
> +       }
> +
> +       /* scale for a MII clock <= 2.5 MHz */
> +       speed = (clock + 2499999) / 2500000;
> +
> +       /* only 6 bits available for MII speed */
> +       if (speed > 0x3F) {
> +               speed = 0x3F;
> +               dev_err(&ofdev->dev,
> +                       "MII clock (%d MHz) exceeds max (2.5 MHz)\n",
> +                       clock / speed);
> +       }
> +
> +       /* Field is in bits 25:30 of MII_SPEED register */
> +       return speed << 1;
> +}
> +EXPORT_SYMBOL(mpc5xxx_get_mii_speed);
> diff --git a/drivers/net/fs_enet/mii-fec.c b/drivers/net/fs_enet/mii-fec.c
> index 75a0999..a28d39f 100644
> --- a/drivers/net/fs_enet/mii-fec.c
> +++ b/drivers/net/fs_enet/mii-fec.c
> @@ -36,6 +36,7 @@
>  #include <asm/pgtable.h>
>  #include <asm/irq.h>
>  #include <asm/uaccess.h>
> +#include <asm/mpc5xxx.h>
>
>  #include "fs_enet.h"
>  #include "fec.h"
> @@ -103,7 +104,6 @@ static int fs_enet_fec_mii_reset(struct mii_bus *bus)
>  static int __devinit fs_enet_mdio_probe(struct of_device *ofdev,
>                                         const struct of_device_id *match)
>  {
> -       struct device_node *np = NULL;
>        struct resource res;
>        struct mii_bus *new_bus;
>        struct fec_info *fec;
> @@ -133,13 +133,20 @@ static int __devinit fs_enet_mdio_probe(struct of_device *ofdev,
>        if (!fec->fecp)
>                goto out_fec;
>
> -       fec->mii_speed = ((ppc_proc_freq + 4999999) / 5000000) << 1;
> +       if (of_device_is_compatible(ofdev->node, "fsl,mpc5121-fec-mdio")) {
> +               i = mpc5xxx_get_mii_speed(ofdev);
> +               if (i < 0)
> +                       goto out_unmap_regs;
> +               fec->mii_speed = i;
> +       } else {
> +               fec->mii_speed = ((ppc_proc_freq + 4999999) / 5000000) << 1;
> +       }
>
>        setbits32(&fec->fecp->fec_r_cntrl, FEC_RCNTRL_MII_MODE);
>        setbits32(&fec->fecp->fec_ecntrl, FEC_ECNTRL_PINMUX |
>                                          FEC_ECNTRL_ETHER_EN);
>        out_be32(&fec->fecp->fec_ievent, FEC_ENET_MII);
> -       out_be32(&fec->fecp->fec_mii_speed, fec->mii_speed);
> +       clrsetbits_be32(&fec->fecp->fec_mii_speed, 0x7E, fec->mii_speed);
>
>        new_bus->phy_mask = ~0;
>        new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL);
> --
> 1.6.0.6
>
>



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

Re: [PATCH 2/2] MPC52xx FEC: be more conservative when setting MII_SPEED register

[Grant Likely]

On Wed, Jul 15, 2009 at 9:18 AM, Wolfgang Denk<wd@denx.de> wrote:
> This patch adds error checking and prevents clobbering unrelated bits
> (reserved bits or the DIS_PREAMBLE bit) when writing the MII_SPEED
> register on MPC52xx systems.
>
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> Cc: <netdev@vger.kernel.org>

As I mentioned in the other patch, I don't want the 5121 and 5200 FEC
devices using common code for this.  It is a tiny block of code and
they are different devices.  Just open code the needed calculation
into this driver.

g.

> ---
>  drivers/net/fec_mpc52xx.c     |    2 +-
>  drivers/net/fec_mpc52xx_phy.c |    6 ++++--
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/fec_mpc52xx.c b/drivers/net/fec_mpc52xx.c
> index cc78633..b69d440 100644
> --- a/drivers/net/fec_mpc52xx.c
> +++ b/drivers/net/fec_mpc52xx.c
> @@ -639,7 +639,7 @@ static void mpc52xx_fec_hw_init(struct net_device *dev)
>        /* set phy speed.
>         * this can't be done in phy driver, since it needs to be called
>         * before fec stuff (even on resume) */
> -       out_be32(&fec->mii_speed, priv->mdio_speed);
> +       clrsetbits_be32(&fec->mii_speed, 0x7E, priv->mdio_speed);
>  }
>
>  /**
> diff --git a/drivers/net/fec_mpc52xx_phy.c b/drivers/net/fec_mpc52xx_phy.c
> index 31e6d62..f733d43 100644
> --- a/drivers/net/fec_mpc52xx_phy.c
> +++ b/drivers/net/fec_mpc52xx_phy.c
> @@ -105,8 +105,10 @@ static int mpc52xx_fec_mdio_probe(struct of_device *of,
>        dev_set_drvdata(dev, bus);
>
>        /* set MII speed */
> -       out_be32(&priv->regs->mii_speed,
> -               ((mpc5xxx_get_bus_frequency(of->node) >> 20) / 5) << 1);
> +       i = mpc5xxx_get_mii_speed(of);
> +       if (i<0)
> +               goto out_unmap;
> +       clrsetbits_be32(&priv->regs->mii_speed, 0x7E, i);
>
>        err = of_mdiobus_register(bus, np);
>        if (err)
> --
> 1.6.0.6
>
>



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

Re: [PATCH 1/2 v3] fs_enet/mii-fec.c: fix MII speed calculation

[Wolfgang Denk]

Dear Grant Likely,

In message <fa686aa40907151017n76524708tdb028689adad4b5f@mail.gmail.com> you wrote:
> On Wed, Jul 15, 2009 at 9:18 AM, Wolfgang Denk<wd@denx.de> wrote:
> > The MII speed calculation was based on the CPU clock (ppc_proc_freq),
> > but for MPC512x we must use the bus clock instead.
> >
> > This patch makes it use the correct clock and makes sure we don't
> > clobber reserved bits in the MII_SPEED register.
...
> Drop the common code bit.  The 5200 and 5121 are different devices and
> it is a tiny bit of code.  I don't think there is any benefit to
> having it as a common function.  Just roll the get_mii_speed function
> in the mii-fec driver itself.

I don't like to see the code repeated - it makes maintenance harder
and increases the memory footprint. But if you like it that way I'll
do that.

> Also, this patch can be quite a bit simpler if you use the .data
> pointer in the drivers match table to specify the function used to
> return the bus clock speed.  Something like this:

OK, will do that, too.

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
Weekends were made for programming.                 - Karl Lehenbauer

Re: [PATCH 2/2] MPC52xx FEC: be more conservative when setting MII_SPEED register

[Wolfgang Denk]

Dear Grant Likely,

In message <fa686aa40907151018n194a154cmb8549c98c673d2bb@mail.gmail.com> you wrote:
> On Wed, Jul 15, 2009 at 9:18 AM, Wolfgang Denk<wd@denx.de> wrote:
> > This patch adds error checking and prevents clobbering unrelated bits
> > (reserved bits or the DIS_PREAMBLE bit) when writing the MII_SPEED
> > register on MPC52xx systems.
...
> As I mentioned in the other patch, I don't want the 5121 and 5200 FEC
> devices using common code for this.  It is a tiny block of code and
> they are different devices.  Just open code the needed calculation
> into this driver.

OK, will do.

[PATCH 1/2 v4] fs_enet/mii-fec.c: fix MII speed calculation

[Wolfgang Denk]

The MII speed calculation was based on the CPU clock (ppc_proc_freq),
but for MPC512x we must use the bus clock instead.

This patch makes it use the correct clock and makes sure we don't
clobber reserved bits in the MII_SPEED register.

Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: <netdev@vger.kernel.org>

Signed-off-by: Wolfgang Denk <wd@denx.de>
---
 drivers/net/fs_enet/mii-fec.c |   35 +++++++++++++++++++++++++++++++----
 1 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/net/fs_enet/mii-fec.c b/drivers/net/fs_enet/mii-fec.c
index 75a0999..62b2d7a 100644
--- a/drivers/net/fs_enet/mii-fec.c
+++ b/drivers/net/fs_enet/mii-fec.c
@@ -103,11 +103,11 @@ static int fs_enet_fec_mii_reset(struct mii_bus *bus)
 static int __devinit fs_enet_mdio_probe(struct of_device *ofdev,
                                         const struct of_device_id *match)
 {
-	struct device_node *np = NULL;
 	struct resource res;
 	struct mii_bus *new_bus;
 	struct fec_info *fec;
-	int ret = -ENOMEM, i;
+	int (*get_bus_freq)(struct device_node *) = match->data;
+	int ret = -ENOMEM, clock, speed;
 
 	new_bus = mdiobus_alloc();
 	if (!new_bus)
@@ -133,13 +133,34 @@ static int __devinit fs_enet_mdio_probe(struct of_device *ofdev,
 	if (!fec->fecp)
 		goto out_fec;
 
-	fec->mii_speed = ((ppc_proc_freq + 4999999) / 5000000) << 1;
+	if (get_bus_freq) {
+		clock = get_bus_freq(ofdev->node);
+
+		if (!clock) {
+			dev_err(&ofdev->dev, "could not determine IPS/IPB clock\n");
+			goto out_unmap_regs;
+		}
+	} else
+		clock = ppc_proc_freq;
+
+	/* scale for a MII clock <= 2.5 MHz */
+	speed = (clock + 2499999) / 2500000;
+
+	/* only 6 bits (25:30) available for MII speed */
+	if (speed > 0x3F) {
+		speed = 0x3F;
+		dev_err(&ofdev->dev,
+			"MII clock (%d Hz) exceeds max (2.5 MHz)\n",
+			clock / speed);
+	}
+
+	fec->mii_speed = speed << 1;
 
 	setbits32(&fec->fecp->fec_r_cntrl, FEC_RCNTRL_MII_MODE);
 	setbits32(&fec->fecp->fec_ecntrl, FEC_ECNTRL_PINMUX |
 	                                  FEC_ECNTRL_ETHER_EN);
 	out_be32(&fec->fecp->fec_ievent, FEC_ENET_MII);
-	out_be32(&fec->fecp->fec_mii_speed, fec->mii_speed);
+	clrsetbits_be32(&fec->fecp->fec_mii_speed, 0x7E, fec->mii_speed);
 
 	new_bus->phy_mask = ~0;
 	new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL);
@@ -188,6 +209,12 @@ static struct of_device_id fs_enet_mdio_fec_match[] = {
 	{
 		.compatible = "fsl,pq1-fec-mdio",
 	},
+#if defined(CONFIG_PPC_MPC512x)
+	{
+		.compatible = "fsl,mpc5121-fec-mdio",
+		.data = mpc5xxx_get_bus_frequency,
+	},
+#endif
 	{},
 };
 
-- 
1.6.0.6

[PATCH 2/2 v2] MPC52xx FEC: be more conservative when setting MII_SPEED register

[Wolfgang Denk]

This patch adds error checking and prevents clobbering unrelated bits
(reserved bits or the DIS_PREAMBLE bit) when writing the MII_SPEED
register on MPC52xx systems.

Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: <netdev@vger.kernel.org>
---
 drivers/net/fec_mpc52xx.c     |    2 +-
 drivers/net/fec_mpc52xx_phy.c |   21 ++++++++++++++++++---
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/net/fec_mpc52xx.c b/drivers/net/fec_mpc52xx.c
index cc78633..b69d440 100644
--- a/drivers/net/fec_mpc52xx.c
+++ b/drivers/net/fec_mpc52xx.c
@@ -639,7 +639,7 @@ static void mpc52xx_fec_hw_init(struct net_device *dev)
 	/* set phy speed.
 	 * this can't be done in phy driver, since it needs to be called
 	 * before fec stuff (even on resume) */
-	out_be32(&fec->mii_speed, priv->mdio_speed);
+	clrsetbits_be32(&fec->mii_speed, 0x7E, priv->mdio_speed);
 }
 
 /**
diff --git a/drivers/net/fec_mpc52xx_phy.c b/drivers/net/fec_mpc52xx_phy.c
index 31e6d62..4c33dc5 100644
--- a/drivers/net/fec_mpc52xx_phy.c
+++ b/drivers/net/fec_mpc52xx_phy.c
@@ -70,7 +70,7 @@ static int mpc52xx_fec_mdio_probe(struct of_device *of,
 	struct mpc52xx_fec_mdio_priv *priv;
 	struct resource res = {};
 	int err;
-	int i;
+	int i, clock, speed;
 
 	bus = mdiobus_alloc();
 	if (bus == NULL)
@@ -105,8 +105,23 @@ static int mpc52xx_fec_mdio_probe(struct of_device *of,
 	dev_set_drvdata(dev, bus);
 
 	/* set MII speed */
-	out_be32(&priv->regs->mii_speed,
-		((mpc5xxx_get_bus_frequency(of->node) >> 20) / 5) << 1);
+	clock = mpc5xxx_get_bus_frequency(of->node);
+	if (!clock) {
+		dev_err(&of->dev, "could not determine IPS/IPB clock\n");
+		goto out_unmap;
+	}
+
+	/* scale for a MII clock <= 2.5 MHz */
+	speed = (clock + 2499999) / 2500000;
+
+	/* only 6 bits (25:30) available for MII speed */
+	if (speed > 0x3F) {
+		speed = 0x3F;
+		dev_err(&of->dev, "MII clock (%d Hz) exceeds max (2.5 MHz)\n",
+			clock / speed);
+	}
+
+	clrsetbits_be32(&priv->regs->mii_speed, 0x7E, speed << 1);
 
 	err = of_mdiobus_register(bus, np);
 	if (err)
-- 
1.6.0.6

Re: [PATCH 1/2 v3] fs_enet/mii-fec.c: fix MII speed calculation

[Grant Likely]

On Thu, Jul 16, 2009 at 3:21 PM, Wolfgang Denk<wd@denx.de> wrote:
> Dear Grant Likely,
>
> In message <fa686aa40907151017n76524708tdb028689adad4b5f@mail.gmail.com> you wrote:
>> On Wed, Jul 15, 2009 at 9:18 AM, Wolfgang Denk<wd@denx.de> wrote:
>> > The MII speed calculation was based on the CPU clock (ppc_proc_freq),
>> > but for MPC512x we must use the bus clock instead.
>> >
>> > This patch makes it use the correct clock and makes sure we don't
>> > clobber reserved bits in the MII_SPEED register.
> ...
>> Drop the common code bit.  The 5200 and 5121 are different devices and
>> it is a tiny bit of code.  I don't think there is any benefit to
>> having it as a common function.  Just roll the get_mii_speed function
>> in the mii-fec driver itself.
>
> I don't like to see the code repeated - it makes maintenance harder
> and increases the memory footprint. But if you like it that way I'll
> do that.

Neither do I, but in this case has some mitigating factors.  diff stat
is interesting:

Old:
 arch/powerpc/include/asm/mpc5xxx.h   |   10 +++++++++
 arch/powerpc/sysdev/mpc5xxx_clocks.c |   37 ++++++++++++++++++++++++++++++++++
 drivers/net/fs_enet/mii-fec.c        |   13 +++++++++--
 drivers/net/fec_mpc52xx.c     |    2 +-
 drivers/net/fec_mpc52xx_phy.c |    6 ++++--
 5 files changed, 62 insertions(+), 6 deletions(-)

New:
 drivers/net/fs_enet/mii-fec.c |   35 +++++++++++++++++++++++++++++++----
 drivers/net/fec_mpc52xx.c     |    2 +-
 drivers/net/fec_mpc52xx_phy.c |   21 ++++++++++++++++++---
 3 files changed, 50 insertions(+), 8 deletions(-)

If the two devices were somewhat related then my opinion might be
different.  However the combination of the tiny amount of calculation
code, the drivers being kept completely separate (or at least as
separate as they were before), the smaller code impact, and the lower
driver complexity (because the calculation code is inline instead of
in a different file) makes me like the second approach is better.

g.

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

Re: [PATCH 1/2 v4] fs_enet/mii-fec.c: fix MII speed calculation

[Grant Likely]

On Thu, Jul 16, 2009 at 3:42 PM, Wolfgang Denk<wd@denx.de> wrote:
> The MII speed calculation was based on the CPU clock (ppc_proc_freq),
> but for MPC512x we must use the bus clock instead.
>
> This patch makes it use the correct clock and makes sure we don't
> clobber reserved bits in the MII_SPEED register.
>
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> Cc: <netdev@vger.kernel.org>

Looks good to me.  Thanks for the work!

I assume this is tested.  I have not tested this on my board, and I've
got one question below, but otherwise I think I can say....
Acked-by: Grant Likely <grant.likely@secretlab.ca>

> -       fec->mii_speed = ((ppc_proc_freq + 4999999) / 5000000) << 1;
> +       if (get_bus_freq) {
> +               clock = get_bus_freq(ofdev->node);
> +
> +               if (!clock) {
> +                       dev_err(&ofdev->dev, "could not determine IPS/IPB clock\n");
> +                       goto out_unmap_regs;
> +               }
> +       } else
> +               clock = ppc_proc_freq;
> +
> +       /* scale for a MII clock <= 2.5 MHz */
> +       speed = (clock + 2499999) / 2500000;

The calculation has changed here for non mpc5121 users.  Shouldn't the
"clock = ppc_proc_freq;" line above be "clock = ppc_proc_freq / 2;"?
Or was this also a bug in the original driver?

g.

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

Re: [PATCH 2/2 v2] MPC52xx FEC: be more conservative when setting MII_SPEED register

[Grant Likely]

On Thu, Jul 16, 2009 at 3:42 PM, Wolfgang Denk<wd@denx.de> wrote:
> This patch adds error checking and prevents clobbering unrelated bits
> (reserved bits or the DIS_PREAMBLE bit) when writing the MII_SPEED
> register on MPC52xx systems.
>
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> Cc: <netdev@vger.kernel.org>

Mostly good.  One comment below.  When it's resolved, feel free to add
my acked-by line.  Thanks for getting this done.

> @@ -105,8 +105,23 @@ static int mpc52xx_fec_mdio_probe(struct of_device *of,
>        dev_set_drvdata(dev, bus);
>
>        /* set MII speed */
> -       out_be32(&priv->regs->mii_speed,
> -               ((mpc5xxx_get_bus_frequency(of->node) >> 20) / 5) << 1);
> +       clock = mpc5xxx_get_bus_frequency(of->node);
> +       if (!clock) {
> +               dev_err(&of->dev, "could not determine IPS/IPB clock\n");
> +               goto out_unmap;
> +       }

Just thought of something.  If it cannot find the clock, then wouldn't
it be better to just use the maximum divider and print a warning
instead of bailing completely?  This goes for the other patch as well.

> +
> +       /* scale for a MII clock <= 2.5 MHz */
> +       speed = (clock + 2499999) / 2500000;
> +
> +       /* only 6 bits (25:30) available for MII speed */
> +       if (speed > 0x3F) {
> +               speed = 0x3F;
> +               dev_err(&of->dev, "MII clock (%d Hz) exceeds max (2.5 MHz)\n",
> +                       clock / speed);
> +       }
> +
> +       clrsetbits_be32(&priv->regs->mii_speed, 0x7E, speed << 1);
>
>        err = of_mdiobus_register(bus, np);
>        if (err)
> --
> 1.6.0.6
>
>



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

Re: [PATCH 1/2 v4] fs_enet/mii-fec.c: fix MII speed calculation

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3411 bytes --]

Hi,

On Thu, Jul 16, 2009 at 11:42:25PM +0200, Wolfgang Denk wrote:
> The MII speed calculation was based on the CPU clock (ppc_proc_freq),
> but for MPC512x we must use the bus clock instead.
> 
> This patch makes it use the correct clock and makes sure we don't
> clobber reserved bits in the MII_SPEED register.
> 
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> Cc: <netdev@vger.kernel.org>
> 
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> ---
>  drivers/net/fs_enet/mii-fec.c |   35 +++++++++++++++++++++++++++++++----
>  1 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/fs_enet/mii-fec.c b/drivers/net/fs_enet/mii-fec.c
> index 75a0999..62b2d7a 100644
> --- a/drivers/net/fs_enet/mii-fec.c
> +++ b/drivers/net/fs_enet/mii-fec.c
> @@ -103,11 +103,11 @@ static int fs_enet_fec_mii_reset(struct mii_bus *bus)
>  static int __devinit fs_enet_mdio_probe(struct of_device *ofdev,
>                                          const struct of_device_id *match)
>  {
> -	struct device_node *np = NULL;
>  	struct resource res;
>  	struct mii_bus *new_bus;
>  	struct fec_info *fec;
> -	int ret = -ENOMEM, i;
> +	int (*get_bus_freq)(struct device_node *) = match->data;
> +	int ret = -ENOMEM, clock, speed;
>  
>  	new_bus = mdiobus_alloc();
>  	if (!new_bus)
> @@ -133,13 +133,34 @@ static int __devinit fs_enet_mdio_probe(struct of_device *ofdev,
>  	if (!fec->fecp)
>  		goto out_fec;
>  
> -	fec->mii_speed = ((ppc_proc_freq + 4999999) / 5000000) << 1;
> +	if (get_bus_freq) {
> +		clock = get_bus_freq(ofdev->node);
> +
> +		if (!clock) {
> +			dev_err(&ofdev->dev, "could not determine IPS/IPB clock\n");
> +			goto out_unmap_regs;
> +		}
> +	} else
> +		clock = ppc_proc_freq;
> +
> +	/* scale for a MII clock <= 2.5 MHz */
> +	speed = (clock + 2499999) / 2500000;
> +
> +	/* only 6 bits (25:30) available for MII speed */
> +	if (speed > 0x3F) {
> +		speed = 0x3F;
> +		dev_err(&ofdev->dev,
> +			"MII clock (%d Hz) exceeds max (2.5 MHz)\n",
> +			clock / speed);
> +	}
> +
> +	fec->mii_speed = speed << 1;
>  
>  	setbits32(&fec->fecp->fec_r_cntrl, FEC_RCNTRL_MII_MODE);
>  	setbits32(&fec->fecp->fec_ecntrl, FEC_ECNTRL_PINMUX |
>  	                                  FEC_ECNTRL_ETHER_EN);
>  	out_be32(&fec->fecp->fec_ievent, FEC_ENET_MII);
> -	out_be32(&fec->fecp->fec_mii_speed, fec->mii_speed);
> +	clrsetbits_be32(&fec->fecp->fec_mii_speed, 0x7E, fec->mii_speed);
>  
>  	new_bus->phy_mask = ~0;
>  	new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL);
> @@ -188,6 +209,12 @@ static struct of_device_id fs_enet_mdio_fec_match[] = {
>  	{
>  		.compatible = "fsl,pq1-fec-mdio",
>  	},
> +#if defined(CONFIG_PPC_MPC512x)
> +	{
> +		.compatible = "fsl,mpc5121-fec-mdio",
> +		.data = mpc5xxx_get_bus_frequency,
> +	},
> +#endif

Grepping through 'drivers/*' I see that #ifdefing compatible-entries is highly
uncommon (just 3 hits). I think a guideline would be useful. Most people like
to avoid #ifdefs at any cost, while I personally think it doesn't spoil
readability too much here. Other opinions?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 2/2 v2] MPC52xx FEC: be more conservative when setting MII_SPEED register

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2892 bytes --]


Hi Wolfgang,

On Thu, Jul 16, 2009 at 11:42:26PM +0200, Wolfgang Denk wrote:
> This patch adds error checking and prevents clobbering unrelated bits
> (reserved bits or the DIS_PREAMBLE bit) when writing the MII_SPEED
> register on MPC52xx systems.
> 
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> Cc: <netdev@vger.kernel.org>
> ---
>  drivers/net/fec_mpc52xx.c     |    2 +-
>  drivers/net/fec_mpc52xx_phy.c |   21 ++++++++++++++++++---
>  2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/fec_mpc52xx.c b/drivers/net/fec_mpc52xx.c
> index cc78633..b69d440 100644
> --- a/drivers/net/fec_mpc52xx.c
> +++ b/drivers/net/fec_mpc52xx.c
> @@ -639,7 +639,7 @@ static void mpc52xx_fec_hw_init(struct net_device *dev)
>  	/* set phy speed.
>  	 * this can't be done in phy driver, since it needs to be called
>  	 * before fec stuff (even on resume) */
> -	out_be32(&fec->mii_speed, priv->mdio_speed);
> +	clrsetbits_be32(&fec->mii_speed, 0x7E, priv->mdio_speed);
>  }

In the probe-function when mdio_speed is set, there is still the old formula
used. Wouldn't that be better in sync?

>  
>  /**
> diff --git a/drivers/net/fec_mpc52xx_phy.c b/drivers/net/fec_mpc52xx_phy.c
> index 31e6d62..4c33dc5 100644
> --- a/drivers/net/fec_mpc52xx_phy.c
> +++ b/drivers/net/fec_mpc52xx_phy.c
> @@ -70,7 +70,7 @@ static int mpc52xx_fec_mdio_probe(struct of_device *of,
>  	struct mpc52xx_fec_mdio_priv *priv;
>  	struct resource res = {};
>  	int err;
> -	int i;
> +	int i, clock, speed;
>  
>  	bus = mdiobus_alloc();
>  	if (bus == NULL)
> @@ -105,8 +105,23 @@ static int mpc52xx_fec_mdio_probe(struct of_device *of,
>  	dev_set_drvdata(dev, bus);
>  
>  	/* set MII speed */
> -	out_be32(&priv->regs->mii_speed,
> -		((mpc5xxx_get_bus_frequency(of->node) >> 20) / 5) << 1);
> +	clock = mpc5xxx_get_bus_frequency(of->node);
> +	if (!clock) {
> +		dev_err(&of->dev, "could not determine IPS/IPB clock\n");
> +		goto out_unmap;
> +	}
> +
> +	/* scale for a MII clock <= 2.5 MHz */
> +	speed = (clock + 2499999) / 2500000;
> +
> +	/* only 6 bits (25:30) available for MII speed */
> +	if (speed > 0x3F) {
> +		speed = 0x3F;
> +		dev_err(&of->dev, "MII clock (%d Hz) exceeds max (2.5 MHz)\n",
> +			clock / speed);
> +	}
> +
> +	clrsetbits_be32(&priv->regs->mii_speed, 0x7E, speed << 1);
>  
>  	err = of_mdiobus_register(bus, np);
>  	if (err)
> -- 
> 1.6.0.6
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH 1/2 v4] fs_enet/mii-fec.c: fix MII speed calculation

[Wolfgang Denk]

Dear Grant Likely,

In message <fa686aa40907161544je92317dy7abea6819746847@mail.gmail.com> you wrote:
>
> I assume this is tested.  I have not tested this on my board, and I've

It was tested, but as it turns out not quite well :-(

> got one question below, but otherwise I think I can say....
...
> The calculation has changed here for non mpc5121 users.  Shouldn't the
> "clock = ppc_proc_freq;" line above be "clock = ppc_proc_freq / 2;"?
> Or was this also a bug in the original driver?

You are right. That's a bug in the new code. Fixed in next version of
the patch.

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
Swap read error.  You lose your mind.

Re: [PATCH 2/2 v2] MPC52xx FEC: be more conservative when setting MII_SPEED register

[Wolfgang Denk]

Dear Grant Likely,

In message <fa686aa40907161548n1658afaei8dce2d893222019c@mail.gmail.com> you wrote:
> 
> Mostly good.  One comment below.  When it's resolved, feel free to add
> my acked-by line.  Thanks for getting this done.
...
> Just thought of something.  If it cannot find the clock, then wouldn't
> it be better to just use the maximum divider and print a warning
> instead of bailing completely?  This goes for the other patch as well.

Will change this for both patches. Thanks.

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"

[PATCH 1/2 v5] fs_enet/mii-fec.c: fix MII speed calculation

[Wolfgang Denk]

The MII speed calculation was based on the CPU clock (ppc_proc_freq),
but for MPC512x we must use the bus clock instead.

This patch makes it use the correct clock and makes sure we don't
clobber reserved bits in the MII_SPEED register.

Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: <netdev@vger.kernel.org>
---
v5: - fix divider so we really use 2.5 MHz (instead of 1.25)
    - use maximum divider in case MPC512x IPS clock is unknown

 drivers/net/fs_enet/mii-fec.c |   37 +++++++++++++++++++++++++++++++++----
 1 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/net/fs_enet/mii-fec.c b/drivers/net/fs_enet/mii-fec.c
index 75a0999..5176986 100644
--- a/drivers/net/fs_enet/mii-fec.c
+++ b/drivers/net/fs_enet/mii-fec.c
@@ -36,6 +36,7 @@
 #include <asm/pgtable.h>
 #include <asm/irq.h>
 #include <asm/uaccess.h>
+#include <asm/mpc5xxx.h>
 
 #include "fs_enet.h"
 #include "fec.h"
@@ -103,11 +104,11 @@ static int fs_enet_fec_mii_reset(struct mii_bus *bus)
 static int __devinit fs_enet_mdio_probe(struct of_device *ofdev,
                                         const struct of_device_id *match)
 {
-	struct device_node *np = NULL;
 	struct resource res;
 	struct mii_bus *new_bus;
 	struct fec_info *fec;
-	int ret = -ENOMEM, i;
+	int (*get_bus_freq)(struct device_node *) = match->data;
+	int ret = -ENOMEM, clock, speed;
 
 	new_bus = mdiobus_alloc();
 	if (!new_bus)
@@ -133,13 +134,35 @@ static int __devinit fs_enet_mdio_probe(struct of_device *ofdev,
 	if (!fec->fecp)
 		goto out_fec;
 
-	fec->mii_speed = ((ppc_proc_freq + 4999999) / 5000000) << 1;
+	if (get_bus_freq) {
+		clock = get_bus_freq(ofdev->node);
+		if (!clock) {
+	 		/* Use maximum divider if clock is unknown */
+			dev_warn(&ofdev->dev, "could not determine IPS clock\n");
+			clock = 0x3F * 5000000;
+		}
+	} else
+		clock = ppc_proc_freq;
+
+	/*
+	 * Scale for a MII clock <= 2.5 MHz
+	 * Note that only 6 bits (25:30) are available for MII speed.
+	 */
+	speed = (clock + 4999999) / 5000000;
+	if (speed > 0x3F) {
+		speed = 0x3F;
+		dev_err(&ofdev->dev,
+			"MII clock (%d Hz) exceeds max (2.5 MHz)\n",
+			clock / speed);
+	}
+
+	fec->mii_speed = speed << 1;
 
 	setbits32(&fec->fecp->fec_r_cntrl, FEC_RCNTRL_MII_MODE);
 	setbits32(&fec->fecp->fec_ecntrl, FEC_ECNTRL_PINMUX |
 	                                  FEC_ECNTRL_ETHER_EN);
 	out_be32(&fec->fecp->fec_ievent, FEC_ENET_MII);
-	out_be32(&fec->fecp->fec_mii_speed, fec->mii_speed);
+	clrsetbits_be32(&fec->fecp->fec_mii_speed, 0x7E, fec->mii_speed);
 
 	new_bus->phy_mask = ~0;
 	new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL);
@@ -188,6 +211,12 @@ static struct of_device_id fs_enet_mdio_fec_match[] = {
 	{
 		.compatible = "fsl,pq1-fec-mdio",
 	},
+#if defined(CONFIG_PPC_MPC512x)
+	{
+		.compatible = "fsl,mpc5121-fec-mdio",
+		.data = mpc5xxx_get_bus_frequency,
+	},
+#endif
 	{},
 };
 
-- 
1.6.0.6

[PATCH 2/2 v3] MPC52xx FEC: be more conservative when setting MII_SPEED register

[Wolfgang Denk]

This patch adds error checking and prevents clobbering unrelated bits
(reserved bits or the DIS_PREAMBLE bit) when writing the MII_SPEED
register on MPC52xx systems.

Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: <netdev@vger.kernel.org>
---
v3: - use maximum divider in case MPC512x IPS clock is unknown

 drivers/net/fec_mpc52xx.c     |    2 +-
 drivers/net/fec_mpc52xx_phy.c |   23 ++++++++++++++++++++---
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/net/fec_mpc52xx.c b/drivers/net/fec_mpc52xx.c
index cc78633..b69d440 100644
--- a/drivers/net/fec_mpc52xx.c
+++ b/drivers/net/fec_mpc52xx.c
@@ -639,7 +639,7 @@ static void mpc52xx_fec_hw_init(struct net_device *dev)
 	/* set phy speed.
 	 * this can't be done in phy driver, since it needs to be called
 	 * before fec stuff (even on resume) */
-	out_be32(&fec->mii_speed, priv->mdio_speed);
+	clrsetbits_be32(&fec->mii_speed, 0x7E, priv->mdio_speed);
 }
 
 /**
diff --git a/drivers/net/fec_mpc52xx_phy.c b/drivers/net/fec_mpc52xx_phy.c
index 31e6d62..d3537e1 100644
--- a/drivers/net/fec_mpc52xx_phy.c
+++ b/drivers/net/fec_mpc52xx_phy.c
@@ -70,7 +70,7 @@ static int mpc52xx_fec_mdio_probe(struct of_device *of,
 	struct mpc52xx_fec_mdio_priv *priv;
 	struct resource res = {};
 	int err;
-	int i;
+	int i, clock, speed;
 
 	bus = mdiobus_alloc();
 	if (bus == NULL)
@@ -105,8 +105,25 @@ static int mpc52xx_fec_mdio_probe(struct of_device *of,
 	dev_set_drvdata(dev, bus);
 
 	/* set MII speed */
-	out_be32(&priv->regs->mii_speed,
-		((mpc5xxx_get_bus_frequency(of->node) >> 20) / 5) << 1);
+	clock = mpc5xxx_get_bus_frequency(of->node);
+	if (!clock) {
+		/* Use maximum divider if clock is unknown */
+		dev_err(&of->dev, "could not determine IPB clock\n");
+		clock = 0x3F * 5000000;
+	}
+
+	/*
+	 * Scale for a MII clock <= 2.5 MHz
+	 * Note that only 6 bits (25:30) are available for MII speed.
+	 */
+	speed = (clock + 4999999) / 5000000;
+	if (speed > 0x3F) {
+		speed = 0x3F;
+		dev_err(&of->dev, "MII clock (%d Hz) exceeds max (2.5 MHz)\n",
+			clock / speed);
+	}
+
+	clrsetbits_be32(&priv->regs->mii_speed, 0x7E, speed << 1);
 
 	err = of_mdiobus_register(bus, np);
 	if (err)
-- 
1.6.0.6

Re: [PATCH 1/2 v4] fs_enet/mii-fec.c: fix MII speed calculation

[Wolfgang Denk]

Dear Wolfram Sang,

In message <20090717093307.GB3150@pengutronix.de> you wrote:
> 
...
> > @@ -188,6 +209,12 @@ static struct of_device_id fs_enet_mdio_fec_match[] = {
> >  	{
> >  		.compatible = "fsl,pq1-fec-mdio",
> >  	},
> > +#if defined(CONFIG_PPC_MPC512x)
> > +	{
> > +		.compatible = "fsl,mpc5121-fec-mdio",
> > +		.data = mpc5xxx_get_bus_frequency,
> > +	},
> > +#endif
>
> Grepping through 'drivers/*' I see that #ifdefing compatible-entries is highly
> uncommon (just 3 hits). I think a guideline would be useful. Most people like
> to avoid #ifdefs at any cost, while I personally think it doesn't spoil
> readability too much here. Other opinions?

An older version of the patch tried to "hide" the ifdef in a 512x
specific header, so at least common code would remain clean, but I
agree with Grant that this current version looks cleaner globally.

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 can have my Unix system when you  pry  it  from  my  cold,  dead
fingers."                                                - Cal Keegan

Re: [PATCH 2/2 v2] MPC52xx FEC: be more conservative when setting MII_SPEED register

[Wolfgang Denk]

Dear Wolfram Sang,

In message <20090717094725.GC3150@pengutronix.de> you wrote:
> 
> In the probe-function when mdio_speed is set, there is still the old formula
> used. Wouldn't that be better in sync?

Good point. Next version of patch following soon.

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
Prof:        So the American government went to IBM to come up with a
             data encryption standard and they came up with ...
Student:     EBCDIC!

[PATCH 2/2 v4] MPC52xx FEC: be more conservative when setting MII_SPEED register

[Wolfgang Denk]

This patch adds error checking and prevents clobbering unrelated bits
(reserved bits or the DIS_PREAMBLE bit) when writing the MII_SPEED
register on MPC52xx systems.

Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: <netdev@vger.kernel.org>
---
v3: - use maximum divider in case MPC512x IPS clock is unknown
v4: - use the same code in the probe-function, too

 drivers/net/fec_mpc52xx.c     |   25 ++++++++++++++++++++++---
 drivers/net/fec_mpc52xx_phy.c |   23 ++++++++++++++++++++---
 2 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/drivers/net/fec_mpc52xx.c b/drivers/net/fec_mpc52xx.c
index cc78633..eed8d2b 100644
--- a/drivers/net/fec_mpc52xx.c
+++ b/drivers/net/fec_mpc52xx.c
@@ -639,7 +639,7 @@ static void mpc52xx_fec_hw_init(struct net_device *dev)
 	/* set phy speed.
 	 * this can't be done in phy driver, since it needs to be called
 	 * before fec stuff (even on resume) */
-	out_be32(&fec->mii_speed, priv->mdio_speed);
+	clrsetbits_be32(&fec->mii_speed, 0x7E, priv->mdio_speed);
 }
 
 /**
@@ -863,7 +863,7 @@ mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *match)
 	struct mpc52xx_fec_priv *priv = NULL;
 	struct resource mem;
 	const u32 *prop;
-	int prop_size;
+	int prop_size, clock, speed;
 
 	phys_addr_t rx_fifo;
 	phys_addr_t tx_fifo;
@@ -948,7 +948,26 @@ mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *match)
 	/* Start with safe defaults for link connection */
 	priv->speed = 100;
 	priv->duplex = DUPLEX_HALF;
-	priv->mdio_speed = ((mpc5xxx_get_bus_frequency(op->node) >> 20) / 5) << 1;
+
+	/* MII speed */
+	clock = mpc5xxx_get_bus_frequency(op->node);
+	if (!clock) {
+		/* Use maximum divider if clock is unknown */
+		dev_err(&op->dev, "could not determine IPB clock\n");
+		clock = 0x3F * 5000000;
+	}
+
+	/*
+	 * Scale for a MII clock <= 2.5 MHz
+	 * Note that only 6 bits (25:30) are available for MII speed.
+	 */
+	speed = (clock + 4999999) / 5000000;
+	if (speed > 0x3F) {
+		speed = 0x3F;
+		dev_err(&op->dev, "MII clock (%d Hz) exceeds max (2.5 MHz)\n",
+			clock / speed);
+	}
+	priv->mdio_speed = speed << 1;
 
 	/* The current speed preconfigures the speed of the MII link */
 	prop = of_get_property(op->node, "current-speed", &prop_size);
diff --git a/drivers/net/fec_mpc52xx_phy.c b/drivers/net/fec_mpc52xx_phy.c
index 31e6d62..d3537e1 100644
--- a/drivers/net/fec_mpc52xx_phy.c
+++ b/drivers/net/fec_mpc52xx_phy.c
@@ -70,7 +70,7 @@ static int mpc52xx_fec_mdio_probe(struct of_device *of,
 	struct mpc52xx_fec_mdio_priv *priv;
 	struct resource res = {};
 	int err;
-	int i;
+	int i, clock, speed;
 
 	bus = mdiobus_alloc();
 	if (bus == NULL)
@@ -105,8 +105,25 @@ static int mpc52xx_fec_mdio_probe(struct of_device *of,
 	dev_set_drvdata(dev, bus);
 
 	/* set MII speed */
-	out_be32(&priv->regs->mii_speed,
-		((mpc5xxx_get_bus_frequency(of->node) >> 20) / 5) << 1);
+	clock = mpc5xxx_get_bus_frequency(of->node);
+	if (!clock) {
+		/* Use maximum divider if clock is unknown */
+		dev_err(&of->dev, "could not determine IPB clock\n");
+		clock = 0x3F * 5000000;
+	}
+
+	/*
+	 * Scale for a MII clock <= 2.5 MHz
+	 * Note that only 6 bits (25:30) are available for MII speed.
+	 */
+	speed = (clock + 4999999) / 5000000;
+	if (speed > 0x3F) {
+		speed = 0x3F;
+		dev_err(&of->dev, "MII clock (%d Hz) exceeds max (2.5 MHz)\n",
+			clock / speed);
+	}
+
+	clrsetbits_be32(&priv->regs->mii_speed, 0x7E, speed << 1);
 
 	err = of_mdiobus_register(bus, np);
 	if (err)
-- 
1.6.0.6

Re: [PATCH 1/2 v5] fs_enet/mii-fec.c: fix MII speed calculation

[Grant Likely]

On Fri, Jul 17, 2009 at 6:27 AM, Wolfgang Denk<wd@denx.de> wrote:
> The MII speed calculation was based on the CPU clock (ppc_proc_freq),
> but for MPC512x we must use the bus clock instead.
>
> This patch makes it use the correct clock and makes sure we don't
> clobber reserved bits in the MII_SPEED register.
>
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> Cc: <netdev@vger.kernel.org>

Acked-by: Grant Likely <grant.likely@secretlab.ca>

David, this isn't a critical bug fix or a regression, so I think it
should be merged for -next.

g.

> ---
> v5: - fix divider so we really use 2.5 MHz (instead of 1.25)
>    - use maximum divider in case MPC512x IPS clock is unknown
>
>  drivers/net/fs_enet/mii-fec.c |   37 +++++++++++++++++++++++++++++++++----
>  1 files changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/fs_enet/mii-fec.c b/drivers/net/fs_enet/mii-fec.c
> index 75a0999..5176986 100644
> --- a/drivers/net/fs_enet/mii-fec.c
> +++ b/drivers/net/fs_enet/mii-fec.c
> @@ -36,6 +36,7 @@
>  #include <asm/pgtable.h>
>  #include <asm/irq.h>
>  #include <asm/uaccess.h>
> +#include <asm/mpc5xxx.h>
>
>  #include "fs_enet.h"
>  #include "fec.h"
> @@ -103,11 +104,11 @@ static int fs_enet_fec_mii_reset(struct mii_bus *bus)
>  static int __devinit fs_enet_mdio_probe(struct of_device *ofdev,
>                                         const struct of_device_id *match)
>  {
> -       struct device_node *np = NULL;
>        struct resource res;
>        struct mii_bus *new_bus;
>        struct fec_info *fec;
> -       int ret = -ENOMEM, i;
> +       int (*get_bus_freq)(struct device_node *) = match->data;
> +       int ret = -ENOMEM, clock, speed;
>
>        new_bus = mdiobus_alloc();
>        if (!new_bus)
> @@ -133,13 +134,35 @@ static int __devinit fs_enet_mdio_probe(struct of_device *ofdev,
>        if (!fec->fecp)
>                goto out_fec;
>
> -       fec->mii_speed = ((ppc_proc_freq + 4999999) / 5000000) << 1;
> +       if (get_bus_freq) {
> +               clock = get_bus_freq(ofdev->node);
> +               if (!clock) {
> +                       /* Use maximum divider if clock is unknown */
> +                       dev_warn(&ofdev->dev, "could not determine IPS clock\n");
> +                       clock = 0x3F * 5000000;
> +               }
> +       } else
> +               clock = ppc_proc_freq;
> +
> +       /*
> +        * Scale for a MII clock <= 2.5 MHz
> +        * Note that only 6 bits (25:30) are available for MII speed.
> +        */
> +       speed = (clock + 4999999) / 5000000;
> +       if (speed > 0x3F) {
> +               speed = 0x3F;
> +               dev_err(&ofdev->dev,
> +                       "MII clock (%d Hz) exceeds max (2.5 MHz)\n",
> +                       clock / speed);
> +       }
> +
> +       fec->mii_speed = speed << 1;
>
>        setbits32(&fec->fecp->fec_r_cntrl, FEC_RCNTRL_MII_MODE);
>        setbits32(&fec->fecp->fec_ecntrl, FEC_ECNTRL_PINMUX |
>                                          FEC_ECNTRL_ETHER_EN);
>        out_be32(&fec->fecp->fec_ievent, FEC_ENET_MII);
> -       out_be32(&fec->fecp->fec_mii_speed, fec->mii_speed);
> +       clrsetbits_be32(&fec->fecp->fec_mii_speed, 0x7E, fec->mii_speed);
>
>        new_bus->phy_mask = ~0;
>        new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL);
> @@ -188,6 +211,12 @@ static struct of_device_id fs_enet_mdio_fec_match[] = {
>        {
>                .compatible = "fsl,pq1-fec-mdio",
>        },
> +#if defined(CONFIG_PPC_MPC512x)
> +       {
> +               .compatible = "fsl,mpc5121-fec-mdio",
> +               .data = mpc5xxx_get_bus_frequency,
> +       },
> +#endif
>        {},
>  };
>
> --
> 1.6.0.6
>
>



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

Re: [PATCH 2/2 v4] MPC52xx FEC: be more conservative when setting MII_SPEED register

[Grant Likely]

On Fri, Jul 17, 2009 at 6:59 AM, Wolfgang Denk<wd@denx.de> wrote:
> This patch adds error checking and prevents clobbering unrelated bits
> (reserved bits or the DIS_PREAMBLE bit) when writing the MII_SPEED
> register on MPC52xx systems.
>
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> Cc: <netdev@vger.kernel.org>
> ---
> v3: - use maximum divider in case MPC512x IPS clock is unknown
> v4: - use the same code in the probe-function, too
>
>  drivers/net/fec_mpc52xx.c     |   25 ++++++++++++++++++++++---
>  drivers/net/fec_mpc52xx_phy.c |   23 ++++++++++++++++++++---

Blech.  now this block of duplicated code I don't like.  This is all
one device, so surely the mdio speed can be calculated once and used
for both drivers.... let me think about this for a bit.  'course this
problem is all rolled up in the nastiness of having two drivers
working on the same device.  I suspect is was a mistake to split up
all the powerpc ethernet drivers into separate of_platform drivers.

g.

>  2 files changed, 42 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/fec_mpc52xx.c b/drivers/net/fec_mpc52xx.c
> index cc78633..eed8d2b 100644
> --- a/drivers/net/fec_mpc52xx.c
> +++ b/drivers/net/fec_mpc52xx.c
> @@ -639,7 +639,7 @@ static void mpc52xx_fec_hw_init(struct net_device *dev)
>        /* set phy speed.
>         * this can't be done in phy driver, since it needs to be called
>         * before fec stuff (even on resume) */
> -       out_be32(&fec->mii_speed, priv->mdio_speed);
> +       clrsetbits_be32(&fec->mii_speed, 0x7E, priv->mdio_speed);
>  }
>
>  /**
> @@ -863,7 +863,7 @@ mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *match)
>        struct mpc52xx_fec_priv *priv = NULL;
>        struct resource mem;
>        const u32 *prop;
> -       int prop_size;
> +       int prop_size, clock, speed;
>
>        phys_addr_t rx_fifo;
>        phys_addr_t tx_fifo;
> @@ -948,7 +948,26 @@ mpc52xx_fec_probe(struct of_device *op, const struct of_device_id *match)
>        /* Start with safe defaults for link connection */
>        priv->speed = 100;
>        priv->duplex = DUPLEX_HALF;
> -       priv->mdio_speed = ((mpc5xxx_get_bus_frequency(op->node) >> 20) / 5) << 1;
> +
> +       /* MII speed */
> +       clock = mpc5xxx_get_bus_frequency(op->node);
> +       if (!clock) {
> +               /* Use maximum divider if clock is unknown */
> +               dev_err(&op->dev, "could not determine IPB clock\n");
> +               clock = 0x3F * 5000000;
> +       }
> +
> +       /*
> +        * Scale for a MII clock <= 2.5 MHz
> +        * Note that only 6 bits (25:30) are available for MII speed.
> +        */
> +       speed = (clock + 4999999) / 5000000;
> +       if (speed > 0x3F) {
> +               speed = 0x3F;
> +               dev_err(&op->dev, "MII clock (%d Hz) exceeds max (2.5 MHz)\n",
> +                       clock / speed);
> +       }
> +       priv->mdio_speed = speed << 1;
>
>        /* The current speed preconfigures the speed of the MII link */
>        prop = of_get_property(op->node, "current-speed", &prop_size);
> diff --git a/drivers/net/fec_mpc52xx_phy.c b/drivers/net/fec_mpc52xx_phy.c
> index 31e6d62..d3537e1 100644
> --- a/drivers/net/fec_mpc52xx_phy.c
> +++ b/drivers/net/fec_mpc52xx_phy.c
> @@ -70,7 +70,7 @@ static int mpc52xx_fec_mdio_probe(struct of_device *of,
>        struct mpc52xx_fec_mdio_priv *priv;
>        struct resource res = {};
>        int err;
> -       int i;
> +       int i, clock, speed;
>
>        bus = mdiobus_alloc();
>        if (bus == NULL)
> @@ -105,8 +105,25 @@ static int mpc52xx_fec_mdio_probe(struct of_device *of,
>        dev_set_drvdata(dev, bus);
>
>        /* set MII speed */
> -       out_be32(&priv->regs->mii_speed,
> -               ((mpc5xxx_get_bus_frequency(of->node) >> 20) / 5) << 1);
> +       clock = mpc5xxx_get_bus_frequency(of->node);
> +       if (!clock) {
> +               /* Use maximum divider if clock is unknown */
> +               dev_err(&of->dev, "could not determine IPB clock\n");
> +               clock = 0x3F * 5000000;
> +       }
> +
> +       /*
> +        * Scale for a MII clock <= 2.5 MHz
> +        * Note that only 6 bits (25:30) are available for MII speed.
> +        */
> +       speed = (clock + 4999999) / 5000000;
> +       if (speed > 0x3F) {
> +               speed = 0x3F;
> +               dev_err(&of->dev, "MII clock (%d Hz) exceeds max (2.5 MHz)\n",
> +                       clock / speed);
> +       }
> +
> +       clrsetbits_be32(&priv->regs->mii_speed, 0x7E, speed << 1);
>
>        err = of_mdiobus_register(bus, np);
>        if (err)
> --
> 1.6.0.6
>
>



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

Re: [PATCH 1/2 v5] fs_enet/mii-fec.c: fix MII speed calculation

[David Miller]

From: Grant Likely <grant.likely@secretlab.ca>
Date: Fri, 17 Jul 2009 08:41:08 -0600

> David, this isn't a critical bug fix or a regression, so I think it
> should be merged for -next.

Ok.

Re: [PATCH 1/2 v5] fs_enet/mii-fec.c: fix MII speed calculation

[David Miller]

From: Grant Likely <grant.likely@secretlab.ca>
Date: Fri, 17 Jul 2009 08:41:08 -0600

> On Fri, Jul 17, 2009 at 6:27 AM, Wolfgang Denk<wd@denx.de> wrote:
>> The MII speed calculation was based on the CPU clock (ppc_proc_freq),
>> but for MPC512x we must use the bus clock instead.
>>
>> This patch makes it use the correct clock and makes sure we don't
>> clobber reserved bits in the MII_SPEED register.
>>
>> Signed-off-by: Wolfgang Denk <wd@denx.de>
>> Cc: Grant Likely <grant.likely@secretlab.ca>
>> Cc: Kumar Gala <galak@kernel.crashing.org>
>> Cc: <netdev@vger.kernel.org>
> 
> Acked-by: Grant Likely <grant.likely@secretlab.ca>

Applied to net-next-2.6

Re: [PATCH 2/2 v4] MPC52xx FEC: be more conservative when setting MII_SPEED register

[Wolfgang Denk]

Dear Grant Likely,

In message <fa686aa40907170745h6c5fae6bia0cff0926c93393c@mail.gmail.com> you wrote:
>
> >  drivers/net/fec_mpc52xx.c     |   25 ++++++++++++++++++++++---
> >  drivers/net/fec_mpc52xx_phy.c |   23 ++++++++++++++++++++---
>
> Blech.  now this block of duplicated code I don't like.  This is all
> one device, so surely the mdio speed can be calculated once and used
> for both drivers.... let me think about this for a bit.  'course this
> problem is all rolled up in the nastiness of having two drivers
> working on the same device.  I suspect is was a mistake to split up
> all the powerpc ethernet drivers into separate of_platform drivers.

If you like, I can re-introduce the mpc5xxx_get_mii_speed() I unrolled
upon your request (with or without re-using it for the MPC512x case).


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
What is mind?  No matter.  What is matter?  Never mind.
                                      -- Thomas Hewitt Key, 1799-1875

Re: [PATCH 2/2 v4] MPC52xx FEC: be more conservative when setting MII_SPEED register

[Grant Likely]

On Fri, Jul 17, 2009 at 11:51 AM, Wolfgang Denk<wd@denx.de> wrote:
> Dear Grant Likely,
>
> In message <fa686aa40907170745h6c5fae6bia0cff0926c93393c@mail.gmail.com> you wrote:
>>
>> >  drivers/net/fec_mpc52xx.c     |   25 ++++++++++++++++++++++---
>> >  drivers/net/fec_mpc52xx_phy.c |   23 ++++++++++++++++++++---
>>
>> Blech.  now this block of duplicated code I don't like.  This is all
>> one device, so surely the mdio speed can be calculated once and used
>> for both drivers.... let me think about this for a bit.  'course this
>> problem is all rolled up in the nastiness of having two drivers
>> working on the same device.  I suspect is was a mistake to split up
>> all the powerpc ethernet drivers into separate of_platform drivers.
>
> If you like, I can re-introduce the mpc5xxx_get_mii_speed() I unrolled
> upon your request (with or without re-using it for the MPC512x case).

No, there's a deeper issues here.  I'm bothered that the mac driver is
impacting the mdio driver.  I'd like to find a way for this whole
thing to be handled cleaner, and not have two different drivers
calculate it.

g.

>
>
> 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
> What is mind?  No matter.  What is matter?  Never mind.
>                                      -- Thomas Hewitt Key, 1799-1875
>



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