diff mbox

[U-Boot,2/2] net: phy: micrel: fix divisor value for KSZ9031 phy skew

Message ID 1453931160-32264-2-git-send-email-dinguyen@opensource.altera.com
State Accepted
Commit ff7bd212cb8a0a80a113e25af7616ef0a24abdfc
Delegated to: Joe Hershberger
Headers show

Commit Message

Dinh Nguyen Jan. 27, 2016, 9:46 p.m. UTC 
From: Dinh Nguyen <dinguyen@opensource.altera.com>

The picoseconds to register value divisor(ps_to_regval) should be 60 and not
200. Linux has KSZ9031_PS_TO_REG defined to be 60 as well. 60 is the correct
divisor because the 4-bit skew values are defined from 0x0000(-420ps) to
0xffff(480ps), increments of 60.

For example, a DTS skew value of 420, represents 0ps delay, which should be 0x7.
With the previous divisor of 200, it would result in 0x2, which represents a
-300ps delay.

With this patch, ethernet on the SoCFPGA DE0 Atlas is now able to work with
1Gb ethernet.

References:
http://www.micrel.com/_PDF/Ethernet/datasheets/KSZ9031RNX.pdf -> page 26

Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
---
 drivers/net/phy/micrel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Marek Vasut Jan. 27, 2016, 10:07 p.m. UTC | #1 
On Wednesday, January 27, 2016 at 10:46:00 PM, dinguyen@opensource.altera.com 
wrote:
> From: Dinh Nguyen <dinguyen@opensource.altera.com>
> 
> The picoseconds to register value divisor(ps_to_regval) should be 60 and
> not 200. Linux has KSZ9031_PS_TO_REG defined to be 60 as well. 60 is the
> correct divisor because the 4-bit skew values are defined from
> 0x0000(-420ps) to 0xffff(480ps), increments of 60.
> 
> For example, a DTS skew value of 420, represents 0ps delay, which should be
> 0x7. With the previous divisor of 200, it would result in 0x2, which
> represents a -300ps delay.
> 
> With this patch, ethernet on the SoCFPGA DE0 Atlas is now able to work with
> 1Gb ethernet.
> 
> References:
> http://www.micrel.com/_PDF/Ethernet/datasheets/KSZ9031RNX.pdf -> page 26
> 
> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>

This is fine, thanks for spotting it.

Acked-by: Marek Vasut <marex@denx.de>

Joe, will you pick these two and push for 2016.03 or shall I pick them ?

Best regards,
Marek Vasut
Joe Hershberger Jan. 27, 2016, 10:53 p.m. UTC | #2 
Hi Marek,

On Wed, Jan 27, 2016 at 4:07 PM, Marek Vasut <marex@denx.de> wrote:
> On Wednesday, January 27, 2016 at 10:46:00 PM, dinguyen@opensource.altera.com
> wrote:
>> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>>
>> The picoseconds to register value divisor(ps_to_regval) should be 60 and
>> not 200. Linux has KSZ9031_PS_TO_REG defined to be 60 as well. 60 is the
>> correct divisor because the 4-bit skew values are defined from
>> 0x0000(-420ps) to 0xffff(480ps), increments of 60.
>>
>> For example, a DTS skew value of 420, represents 0ps delay, which should be
>> 0x7. With the previous divisor of 200, it would result in 0x2, which
>> represents a -300ps delay.
>>
>> With this patch, ethernet on the SoCFPGA DE0 Atlas is now able to work with
>> 1Gb ethernet.
>>
>> References:
>> http://www.micrel.com/_PDF/Ethernet/datasheets/KSZ9031RNX.pdf -> page 26
>>
>> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
>
> This is fine, thanks for spotting it.
>
> Acked-by: Marek Vasut <marex@denx.de>
>
> Joe, will you pick these two and push for 2016.03 or shall I pick them ?

I'll get them.

-Joe
Joe Hershberger Jan. 27, 2016, 10:54 p.m. UTC | #3 
On Wed, Jan 27, 2016 at 3:46 PM,  <dinguyen@opensource.altera.com> wrote:
> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>
> The picoseconds to register value divisor(ps_to_regval) should be 60 and not
> 200. Linux has KSZ9031_PS_TO_REG defined to be 60 as well. 60 is the correct
> divisor because the 4-bit skew values are defined from 0x0000(-420ps) to
> 0xffff(480ps), increments of 60.
>
> For example, a DTS skew value of 420, represents 0ps delay, which should be 0x7.
> With the previous divisor of 200, it would result in 0x2, which represents a
> -300ps delay.
>
> With this patch, ethernet on the SoCFPGA DE0 Atlas is now able to work with
> 1Gb ethernet.
>
> References:
> http://www.micrel.com/_PDF/Ethernet/datasheets/KSZ9031RNX.pdf -> page 26
>
> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>
Marek Vasut Jan. 27, 2016, 11:43 p.m. UTC | #4 
On Wednesday, January 27, 2016 at 11:53:29 PM, Joe Hershberger wrote:
> Hi Marek,
> 
> On Wed, Jan 27, 2016 at 4:07 PM, Marek Vasut <marex@denx.de> wrote:
> > On Wednesday, January 27, 2016 at 10:46:00 PM,
> > dinguyen@opensource.altera.com
> > 
> > wrote:
> >> From: Dinh Nguyen <dinguyen@opensource.altera.com>
> >> 
> >> The picoseconds to register value divisor(ps_to_regval) should be 60 and
> >> not 200. Linux has KSZ9031_PS_TO_REG defined to be 60 as well. 60 is the
> >> correct divisor because the 4-bit skew values are defined from
> >> 0x0000(-420ps) to 0xffff(480ps), increments of 60.
> >> 
> >> For example, a DTS skew value of 420, represents 0ps delay, which should
> >> be 0x7. With the previous divisor of 200, it would result in 0x2, which
> >> represents a -300ps delay.
> >> 
> >> With this patch, ethernet on the SoCFPGA DE0 Atlas is now able to work
> >> with 1Gb ethernet.
> >> 
> >> References:
> >> http://www.micrel.com/_PDF/Ethernet/datasheets/KSZ9031RNX.pdf -> page 26
> >> 
> >> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> > 
> > This is fine, thanks for spotting it.
> > 
> > Acked-by: Marek Vasut <marex@denx.de>
> > 
> > Joe, will you pick these two and push for 2016.03 or shall I pick them ?
> 
> I'll get them.

Roger, thanks!

Best regards,
Marek Vasut
Måns Rullgård Jan. 28, 2016, 1:26 a.m. UTC | #5 
<dinguyen@opensource.altera.com> writes:

> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>
> The picoseconds to register value divisor(ps_to_regval) should be 60 and not
> 200. Linux has KSZ9031_PS_TO_REG defined to be 60 as well. 60 is the correct
> divisor because the 4-bit skew values are defined from 0x0000(-420ps) to
> 0xffff(480ps), increments of 60.
>
> For example, a DTS skew value of 420, represents 0ps delay, which should be 0x7.
> With the previous divisor of 200, it would result in 0x2, which represents a
> -300ps delay.
>
> With this patch, ethernet on the SoCFPGA DE0 Atlas is now able to work with
> 1Gb ethernet.

Is this expected to make any difference on the Altera socdk?  Both with
and without the patch, it takes a very long time (sometimes minutes) to
negotiate a link, but once it does it works fine.

> References:
> http://www.micrel.com/_PDF/Ethernet/datasheets/KSZ9031RNX.pdf -> page 26
>
> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> ---
>  drivers/net/phy/micrel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 19b6bc7..2530a5b 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -211,7 +211,7 @@ static int ksz90x1_of_config_group(struct phy_device *phydev,
>  {
>  	struct udevice *dev = phydev->dev;
>  	struct phy_driver *drv = phydev->drv;
> -	const int ps_to_regval = 200;
> +	const int ps_to_regval = 60;
>  	int val[4];
>  	int i, changed = 0, offset, max;
>  	u16 regval = 0;
> -- 
> 2.6.2
Joe Hershberger Jan. 29, 2016, 9:27 p.m. UTC | #6 
Hi Dinh,

https://patchwork.ozlabs.org/patch/574448/ was applied to u-boot-net.git.

Thanks!
-Joe
Dinh Nguyen Feb. 2, 2016, 2:54 p.m. UTC | #7 
On 01/27/2016 07:26 PM, Måns Rullgård wrote:
> <dinguyen@opensource.altera.com> writes:
> 
>> From: Dinh Nguyen <dinguyen@opensource.altera.com>
>>
>> The picoseconds to register value divisor(ps_to_regval) should be 60 and not
>> 200. Linux has KSZ9031_PS_TO_REG defined to be 60 as well. 60 is the correct
>> divisor because the 4-bit skew values are defined from 0x0000(-420ps) to
>> 0xffff(480ps), increments of 60.
>>
>> For example, a DTS skew value of 420, represents 0ps delay, which should be 0x7.
>> With the previous divisor of 200, it would result in 0x2, which represents a
>> -300ps delay.
>>
>> With this patch, ethernet on the SoCFPGA DE0 Atlas is now able to work with
>> 1Gb ethernet.
> 
> Is this expected to make any difference on the Altera socdk?  Both with
> and without the patch, it takes a very long time (sometimes minutes) to
> negotiate a link, but once it does it works fine.
> 

The Altera socdk uses a different PHY, KSZ9021, so no, this patch will
not affect that hardware.

I'll check out your link issues on the socdk when I get a chance.

Dinh
diff mbox

Patch

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 19b6bc7..2530a5b 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -211,7 +211,7 @@  static int ksz90x1_of_config_group(struct phy_device *phydev,
 {
 	struct udevice *dev = phydev->dev;
 	struct phy_driver *drv = phydev->drv;
-	const int ps_to_regval = 200;
+	const int ps_to_regval = 60;
 	int val[4];
 	int i, changed = 0, offset, max;
 	u16 regval = 0;