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

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(-)

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
>
>

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

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.