| Message ID | 1247835545-18650-1-git-send-email-wd@denx.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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 > >
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
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 >
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)
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(-)