| Message ID | 1247780546-4426-2-git-send-email-wd@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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 > >
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
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
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
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)
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(-)