[3/4] net/fec: set phy_speed to the optimal frequency 2.5 MHz

Message ID 1316346852-17090-4-git-send-email-shawn.guo@linaro.org
State New
Headers show

Commit Message

Shawn Guo Sept. 18, 2011, 11:54 a.m.
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(-)

Comments

Troy Kisky Sept. 19, 2011, 10:39 p.m. | #1
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
Shawn Guo Sept. 20, 2011, 2:57 a.m. | #2
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?
Lothar Waßmann Sept. 20, 2011, 7:50 a.m. | #3
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
Shawn Guo Sept. 20, 2011, 8:14 a.m. | #4
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.
David Miller Sept. 20, 2011, 7:11 p.m. | #5
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
Troy Kisky Sept. 20, 2011, 8:05 p.m. | #6
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.
Troy Kisky Sept. 20, 2011, 8:10 p.m. | #7
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.
>
>
>

Patch

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