| Message ID | 1316346852-17090-4-git-send-email-shawn.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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?
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
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.
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
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.
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. > > >
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();
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(-)