[PATCH] net/phy/vitesse: Configure RGMII skew on VSC8601, if needed

[PATCH] net/phy/vitesse: Configure RGMII skew on VSC8601, if needed

[Alexandru Gagniuc]

With RGMII, we need a 1.5 to 2ns skew between clock and data lines. The
VSC8601 can handle this internally. While the VSC8601 can set more
fine-grained delays, the standard skew settings work out of the box.
The same heuristic is used to determine when this skew should be enabled
as in vsc824x_config_init().

Tested on custom board with AM3352 SOC and VSC801 PHY.

Signed-off-by: Alexandru Gagniuc <alex.g@adaptrum.com>
---
 drivers/net/phy/vitesse.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/vitesse.c b/drivers/net/phy/vitesse.c
index 2e37eb3..7923831 100644
--- a/drivers/net/phy/vitesse.c
+++ b/drivers/net/phy/vitesse.c
@@ -62,6 +62,10 @@
 /* Vitesse Extended Page Access Register */
 #define MII_VSC82X4_EXT_PAGE_ACCESS	0x1f
 
+/* Vitesse VSC8601 Extended PHY Control Register 1 */
+#define MII_VSC8601_EPHY_CTL		0x17
+#define MII_VSC8601_EPHY_CTL_RGMII_SKEW	(1 << 8)
+
 #define PHY_ID_VSC8234			0x000fc620
 #define PHY_ID_VSC8244			0x000fc6c0
 #define PHY_ID_VSC8514			0x00070670
@@ -111,6 +115,31 @@ static int vsc824x_config_init(struct phy_device *phydev)
 	return err;
 }
 
+static int vsc8601_add_skew(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_read(phydev, MII_VSC8601_EPHY_CTL);
+	if (ret < 0)
+		return ret;
+
+	ret |= MII_VSC8601_EPHY_CTL_RGMII_SKEW;
+	return phy_write(phydev, MII_VSC8601_EPHY_CTL, ret);
+}
+
+static int vsc8601_config_init(struct phy_device *phydev)
+{
+	int ret = 0;
+
+	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
+		ret = vsc8601_add_skew(phydev);
+
+	if (ret < 0)
+		return ret;
+
+	return genphy_config_init(phydev);
+}
+
 static int vsc824x_ack_interrupt(struct phy_device *phydev)
 {
 	int err = 0;
@@ -275,7 +304,7 @@ static struct phy_driver vsc82xx_driver[] = {
 	.phy_id_mask    = 0x000ffff0,
 	.features       = PHY_GBIT_FEATURES,
 	.flags          = PHY_HAS_INTERRUPT,
-	.config_init    = &genphy_config_init,
+	.config_init    = &vsc8601_config_init,
 	.config_aneg    = &genphy_config_aneg,
 	.read_status    = &genphy_read_status,
 	.ack_interrupt  = &vsc824x_ack_interrupt,
-- 
2.7.4

Re: [PATCH] net/phy/vitesse: Configure RGMII skew on VSC8601, if needed

[David Miller]

From: Alexandru Gagniuc <alex.g@adaptrum.com>
Date: Sat, 12 Nov 2016 15:32:13 -0800

> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
> +		ret = vsc8601_add_skew(phydev);

I think you should use phy_interface_is_rgmii() here.

Re: [PATCH] net/phy/vitesse: Configure RGMII skew on VSC8601, if needed

[Florian Fainelli]

On 11/14/2016 01:18 PM, David Miller wrote:
> From: Alexandru Gagniuc <alex.g@adaptrum.com>
> Date: Sat, 12 Nov 2016 15:32:13 -0800
> 
>> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
>> +		ret = vsc8601_add_skew(phydev);
> 
> I think you should use phy_interface_is_rgmii() here.
> 

This would include all RGMII modes, here I think the intent is to check
for PHY_INTERFACE_MODE_RGMII_ID and PHY_INTERFACE_MODE_RGMII_TXID (or
RXID),  Alexandru, what direction does the skew settings apply to?
-- 
Florian

Re: [PATCH] net/phy/vitesse: Configure RGMII skew on VSC8601, if needed

[Alex]

On 11/14/2016 01:25 PM, Florian Fainelli wrote:
> On 11/14/2016 01:18 PM, David Miller wrote:
>> From: Alexandru Gagniuc <alex.g@adaptrum.com>
>> Date: Sat, 12 Nov 2016 15:32:13 -0800
>>
>>> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
>>> +		ret = vsc8601_add_skew(phydev);
>>
>> I think you should use phy_interface_is_rgmii() here.
>>
>
> This would include all RGMII modes, here I think the intent is to check
> for PHY_INTERFACE_MODE_RGMII_ID and PHY_INTERFACE_MODE_RGMII_TXID (or
> RXID),

That is correct.

>  Alexandru, what direction does the skew settings apply to?

It applies a skew in both TX and RX directions.

Alex

Re: [PATCH] net/phy/vitesse: Configure RGMII skew on VSC8601, if needed

[David Miller]

From: Alex <alex.g@adaptrum.com>
Date: Mon, 14 Nov 2016 13:54:57 -0800

> 
> 
> On 11/14/2016 01:25 PM, Florian Fainelli wrote:
>> On 11/14/2016 01:18 PM, David Miller wrote:
>>> From: Alexandru Gagniuc <alex.g@adaptrum.com>
>>> Date: Sat, 12 Nov 2016 15:32:13 -0800
>>>
>>>> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
>>>> +		ret = vsc8601_add_skew(phydev);
>>>
>>> I think you should use phy_interface_is_rgmii() here.
>>>
>>
>> This would include all RGMII modes, here I think the intent is to
>> check
>> for PHY_INTERFACE_MODE_RGMII_ID and PHY_INTERFACE_MODE_RGMII_TXID (or
>> RXID),
> 
> That is correct.
> 
>>  Alexandru, what direction does the skew settings apply to?
> 
> It applies a skew in both TX and RX directions.

Please repost your patch, making the intent clear either in the
commit message or a code comment.

Thanks.

[PATCH v2] net/phy/vitesse: Configure RGMII skew on VSC8601, if needed

[Alexandru Gagniuc]

With RGMII, we need a 1.5 to 2ns skew between clock and data lines. The
VSC8601 can handle this internally. While the VSC8601 can set more
fine-grained delays, the standard skew settings work out of the box.
The same heuristic is used to determine when this skew should be enabled
as in vsc824x_config_init().

Tested on custom board with AM3352 SOC and VSC801 PHY.

Signed-off-by: Alexandru Gagniuc <alex.g@adaptrum.com>
---
Changes since v1:
 * Added comment detailing applicability to different RGMII interfaces.

 drivers/net/phy/vitesse.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/vitesse.c b/drivers/net/phy/vitesse.c
index 2e37eb3..24b4a09 100644
--- a/drivers/net/phy/vitesse.c
+++ b/drivers/net/phy/vitesse.c
@@ -62,6 +62,10 @@
 /* Vitesse Extended Page Access Register */
 #define MII_VSC82X4_EXT_PAGE_ACCESS	0x1f
 
+/* Vitesse VSC8601 Extended PHY Control Register 1 */
+#define MII_VSC8601_EPHY_CTL		0x17
+#define MII_VSC8601_EPHY_CTL_RGMII_SKEW	(1 << 8)
+
 #define PHY_ID_VSC8234			0x000fc620
 #define PHY_ID_VSC8244			0x000fc6c0
 #define PHY_ID_VSC8514			0x00070670
@@ -111,6 +115,34 @@ static int vsc824x_config_init(struct phy_device *phydev)
 	return err;
 }
 
+/* This adds a skew for both TX and RX clocks, so the skew should only be
+ * applied to "rgmii-id" interfaces. It may not work as expected
+ * on "rgmii-txid", "rgmii-rxid" or "rgmii" interfaces. */
+static int vsc8601_add_skew(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_read(phydev, MII_VSC8601_EPHY_CTL);
+	if (ret < 0)
+		return ret;
+
+	ret |= MII_VSC8601_EPHY_CTL_RGMII_SKEW;
+	return phy_write(phydev, MII_VSC8601_EPHY_CTL, ret);
+}
+
+static int vsc8601_config_init(struct phy_device *phydev)
+{
+	int ret = 0;
+
+	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
+		ret = vsc8601_add_skew(phydev);
+
+	if (ret < 0)
+		return ret;
+
+	return genphy_config_init(phydev);
+}
+
 static int vsc824x_ack_interrupt(struct phy_device *phydev)
 {
 	int err = 0;
@@ -275,7 +307,7 @@ static struct phy_driver vsc82xx_driver[] = {
 	.phy_id_mask    = 0x000ffff0,
 	.features       = PHY_GBIT_FEATURES,
 	.flags          = PHY_HAS_INTERRUPT,
-	.config_init    = &genphy_config_init,
+	.config_init    = &vsc8601_config_init,
 	.config_aneg    = &genphy_config_aneg,
 	.read_status    = &genphy_read_status,
 	.ack_interrupt  = &vsc824x_ack_interrupt,
-- 
2.7.4

Re: [PATCH v2] net/phy/vitesse: Configure RGMII skew on VSC8601, if needed

[Andrew Lunn]

On Wed, Nov 16, 2016 at 01:02:33AM -0800, Alexandru Gagniuc wrote:
> With RGMII, we need a 1.5 to 2ns skew between clock and data lines. The
> VSC8601 can handle this internally. While the VSC8601 can set more
> fine-grained delays, the standard skew settings work out of the box.
> The same heuristic is used to determine when this skew should be enabled
> as in vsc824x_config_init().
> 
> Tested on custom board with AM3352 SOC and VSC801 PHY.
> 
> Signed-off-by: Alexandru Gagniuc <alex.g@adaptrum.com>
> ---
> Changes since v1:
>  * Added comment detailing applicability to different RGMII interfaces.
> 
>  drivers/net/phy/vitesse.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/vitesse.c b/drivers/net/phy/vitesse.c
> index 2e37eb3..24b4a09 100644
> --- a/drivers/net/phy/vitesse.c
> +++ b/drivers/net/phy/vitesse.c
> @@ -62,6 +62,10 @@
>  /* Vitesse Extended Page Access Register */
>  #define MII_VSC82X4_EXT_PAGE_ACCESS	0x1f
>  
> +/* Vitesse VSC8601 Extended PHY Control Register 1 */
> +#define MII_VSC8601_EPHY_CTL		0x17
> +#define MII_VSC8601_EPHY_CTL_RGMII_SKEW	(1 << 8)
> +
>  #define PHY_ID_VSC8234			0x000fc620
>  #define PHY_ID_VSC8244			0x000fc6c0
>  #define PHY_ID_VSC8514			0x00070670
> @@ -111,6 +115,34 @@ static int vsc824x_config_init(struct phy_device *phydev)
>  	return err;
>  }
>  
> +/* This adds a skew for both TX and RX clocks, so the skew should only be
> + * applied to "rgmii-id" interfaces. It may not work as expected
> + * on "rgmii-txid", "rgmii-rxid" or "rgmii" interfaces. */

Hi Alexandru

You should be able to make "rgmii" work as expected. If that is the
phy mode, disable the skew.

    Andrew

Re: [PATCH v2] net/phy/vitesse: Configure RGMII skew on VSC8601, if needed

[Alex]

On 11/16/2016 05:50 AM, Andrew Lunn wrote:
> On Wed, Nov 16, 2016 at 01:02:33AM -0800, Alexandru Gagniuc wrote:
>> With RGMII, we need a 1.5 to 2ns skew between clock and data lines. The
>> VSC8601 can handle this internally. While the VSC8601 can set more
>> fine-grained delays, the standard skew settings work out of the box.
>> The same heuristic is used to determine when this skew should be enabled
>> as in vsc824x_config_init().
>>
>> +/* This adds a skew for both TX and RX clocks, so the skew should only be
>> + * applied to "rgmii-id" interfaces. It may not work as expected
>> + * on "rgmii-txid", "rgmii-rxid" or "rgmii" interfaces. */
>
> Hi Alexandru
>
> You should be able to make "rgmii" work as expected. If that is the
> phy mode, disable the skew.

And that's exactly the implemented behavior. See vsc8601_config_init() 
below.

Alex

Re: [PATCH v2] net/phy/vitesse: Configure RGMII skew on VSC8601, if needed

[Andrew Lunn]

On Wed, Nov 16, 2016 at 08:44:30AM -0800, Alex wrote:
> 
> 
> On 11/16/2016 05:50 AM, Andrew Lunn wrote:
> >On Wed, Nov 16, 2016 at 01:02:33AM -0800, Alexandru Gagniuc wrote:
> >>With RGMII, we need a 1.5 to 2ns skew between clock and data lines. The
> >>VSC8601 can handle this internally. While the VSC8601 can set more
> >>fine-grained delays, the standard skew settings work out of the box.
> >>The same heuristic is used to determine when this skew should be enabled
> >>as in vsc824x_config_init().
> >>
> >>+/* This adds a skew for both TX and RX clocks, so the skew should only be
> >>+ * applied to "rgmii-id" interfaces. It may not work as expected
> >>+ * on "rgmii-txid", "rgmii-rxid" or "rgmii" interfaces. */
> >
> >Hi Alexandru
> >
> >You should be able to make "rgmii" work as expected. If that is the
> >phy mode, disable the skew.
> 
> And that's exactly the implemented behavior. See
> vsc8601_config_init() below.

I don't think so. vsc8601_config_init() will not cause the skew to be
cleared if the phy-mode is "rgmii" and something else like the
bootloader could of set the skew. So saying that "rgmii" might not
work as expected is true. But with a minor change, you can make it
work as expected.

	Andrew

Re: [PATCH v2] net/phy/vitesse: Configure RGMII skew on VSC8601, if needed

[Alex]

On 11/16/2016 08:54 AM, Andrew Lunn wrote:
> On Wed, Nov 16, 2016 at 08:44:30AM -0800, Alex wrote:
>>
>>
>> On 11/16/2016 05:50 AM, Andrew Lunn wrote:
>>> On Wed, Nov 16, 2016 at 01:02:33AM -0800, Alexandru Gagniuc wrote:
>>>> With RGMII, we need a 1.5 to 2ns skew between clock and data lines. The
>>>> VSC8601 can handle this internally. While the VSC8601 can set more
>>>> fine-grained delays, the standard skew settings work out of the box.
>>>> The same heuristic is used to determine when this skew should be enabled
>>>> as in vsc824x_config_init().
>>>>
>>>> +/* This adds a skew for both TX and RX clocks, so the skew should only be
>>>> + * applied to "rgmii-id" interfaces. It may not work as expected
>>>> + * on "rgmii-txid", "rgmii-rxid" or "rgmii" interfaces. */
>>>
>>> Hi Alexandru
>>>
>>> You should be able to make "rgmii" work as expected. If that is the
>>> phy mode, disable the skew.
>>
>> And that's exactly the implemented behavior. See
>> vsc8601_config_init() below.
>
> I don't think so. vsc8601_config_init() will not cause the skew to be
> cleared if the phy-mode is "rgmii" and something else like the
> bootloader could of set the skew. So saying that "rgmii" might not
> work as expected is true. But with a minor change, you can make it
> work as expected.

That's not within the scope of this change. The scope is to make 
rgmii-id work. Any additional changes would be untested.

Alex

Re: [PATCH v2] net/phy/vitesse: Configure RGMII skew on VSC8601, if needed

[David Miller]

From: Alexandru Gagniuc <alex.g@adaptrum.com>
Date: Wed, 16 Nov 2016 01:02:33 -0800

> With RGMII, we need a 1.5 to 2ns skew between clock and data lines. The
> VSC8601 can handle this internally. While the VSC8601 can set more
> fine-grained delays, the standard skew settings work out of the box.
> The same heuristic is used to determine when this skew should be enabled
> as in vsc824x_config_init().
> 
> Tested on custom board with AM3352 SOC and VSC801 PHY.
> 
> Signed-off-by: Alexandru Gagniuc <alex.g@adaptrum.com>
> ---
> Changes since v1:
>  * Added comment detailing applicability to different RGMII interfaces.

Applied.