[PATCH 0/2] Atheros 803x PHY RGMII clock delays

[PATCH 0/2] Atheros 803x PHY RGMII clock delays

[Marc Gonzalez]

These patches address the quirks of the Atheros PHY, with respect
to RGMII RX and TX clock delays.

Once the PHY driver is fixed, the second patch fixes the ethernet
controller used on tango boards.

Marc Gonzalez (2):
  net: phy: at803x: Fix RGMII RX and TX clock delays setup
  net: ethernet: nb8800: Fix RGMII TX clock delay setup

 drivers/net/ethernet/aurora/nb8800.c |  8 +++++---
 drivers/net/phy/at803x.c             | 32 ++++++++++++++++++++++++--------
 2 files changed, 29 insertions(+), 11 deletions(-)

-- 
2.11.0

[PATCH 0/2] Atheros 803x PHY RGMII clock delays

[Marc Gonzalez]

These patches address the quirks of the Atheros PHY, with respect
to RGMII RX and TX clock delays.

Once the PHY driver is fixed, the second patch fixes the ethernet
controller used on tango boards.

Marc Gonzalez (2):
  net: phy: at803x: Fix RGMII RX and TX clock delays setup
  net: ethernet: nb8800: Fix RGMII TX clock delay setup

 drivers/net/ethernet/aurora/nb8800.c |  8 +++++---
 drivers/net/phy/at803x.c             | 32 ++++++++++++++++++++++++--------
 2 files changed, 29 insertions(+), 11 deletions(-)

-- 
2.11.0

[PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Marc Gonzalez]

The current code supports enabling RGMII RX and TX clock delays.
The unstated assumption is that these settings are disabled by
default at reset, which is not the case.

RX clock delay is enabled at reset. And TX clock delay "survives"
across SW resets. Thus, if the bootloader enables TX clock delay,
it will remain enabled at reset in Linux.

Provide disable functions to configure the RGMII clock delays
exactly as specified in the fwspec.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 drivers/net/phy/at803x.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index c1e52b9dc58d..9c96eaaaa2cb 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -117,12 +117,24 @@ static inline int at803x_enable_rx_delay(struct phy_device *phydev)
 					AT803X_DEBUG_RX_CLK_DLY_EN);
 }
 
+static inline int at803x_disable_rx_delay(struct phy_device *phydev)
+{
+	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
+					AT803X_DEBUG_RX_CLK_DLY_EN, 0);
+}
+
 static inline int at803x_enable_tx_delay(struct phy_device *phydev)
 {
 	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5, 0,
 					AT803X_DEBUG_TX_CLK_DLY_EN);
 }
 
+static inline int at803x_disable_tx_delay(struct phy_device *phydev)
+{
+	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5,
+					AT803X_DEBUG_TX_CLK_DLY_EN, 0);
+}
+
 /* save relevant PHY registers to private copy */
 static void at803x_context_save(struct phy_device *phydev,
 				struct at803x_context *context)
@@ -284,18 +296,22 @@ static int at803x_config_init(struct phy_device *phydev)
 		return ret;
 
 	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
-			phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
+			phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
 		ret = at803x_enable_rx_delay(phydev);
-		if (ret < 0)
-			return ret;
-	}
+	else
+		ret = at803x_disable_rx_delay(phydev);
+
+	if (ret < 0)
+		return ret;
 
 	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
-			phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
+			phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
 		ret = at803x_enable_tx_delay(phydev);
-		if (ret < 0)
-			return ret;
-	}
+	else
+		ret = at803x_disable_tx_delay(phydev);
+
+	if (ret < 0)
+		return ret;
 
 	return 0;
 }
-- 
2.11.0

[PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Marc Gonzalez]

The current code supports enabling RGMII RX and TX clock delays.
The unstated assumption is that these settings are disabled by
default at reset, which is not the case.

RX clock delay is enabled at reset. And TX clock delay "survives"
across SW resets. Thus, if the bootloader enables TX clock delay,
it will remain enabled at reset in Linux.

Provide disable functions to configure the RGMII clock delays
exactly as specified in the fwspec.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 drivers/net/phy/at803x.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index c1e52b9dc58d..9c96eaaaa2cb 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -117,12 +117,24 @@ static inline int at803x_enable_rx_delay(struct phy_device *phydev)
 					AT803X_DEBUG_RX_CLK_DLY_EN);
 }
 
+static inline int at803x_disable_rx_delay(struct phy_device *phydev)
+{
+	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_0,
+					AT803X_DEBUG_RX_CLK_DLY_EN, 0);
+}
+
 static inline int at803x_enable_tx_delay(struct phy_device *phydev)
 {
 	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5, 0,
 					AT803X_DEBUG_TX_CLK_DLY_EN);
 }
 
+static inline int at803x_disable_tx_delay(struct phy_device *phydev)
+{
+	return at803x_debug_reg_mask(phydev, AT803X_DEBUG_REG_5,
+					AT803X_DEBUG_TX_CLK_DLY_EN, 0);
+}
+
 /* save relevant PHY registers to private copy */
 static void at803x_context_save(struct phy_device *phydev,
 				struct at803x_context *context)
@@ -284,18 +296,22 @@ static int at803x_config_init(struct phy_device *phydev)
 		return ret;
 
 	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
-			phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
+			phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
 		ret = at803x_enable_rx_delay(phydev);
-		if (ret < 0)
-			return ret;
-	}
+	else
+		ret = at803x_disable_rx_delay(phydev);
+
+	if (ret < 0)
+		return ret;
 
 	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
-			phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
+			phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
 		ret = at803x_enable_tx_delay(phydev);
-		if (ret < 0)
-			return ret;
-	}
+	else
+		ret = at803x_disable_tx_delay(phydev);
+
+	if (ret < 0)
+		return ret;
 
 	return 0;
 }
-- 
2.11.0

[PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Marc Gonzalez]

According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
("Documentation: devicetree: clarify usage of the RGMII phy-modes")
there are 4 RGMII phy-modes to handle:

"rgmii" (RX and TX delays are added by the MAC when required)
"rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
	the MAC should not add the RX or TX delays in this case)
"rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
	the MAC should not add an RX delay in this case)
"rgmii-txid" (RGMII with internal TX delay provided by the PHY,
	the MAC should not add an TX delay in this case)

Let the MAC handle TX clock delay for rgmii and rgmii-rxid.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 drivers/net/ethernet/aurora/nb8800.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index 041cfb7952f8..f3ed320eb4ad 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -609,7 +609,7 @@ static void nb8800_mac_config(struct net_device *dev)
 		mac_mode |= HALF_DUPLEX;
 
 	if (gigabit) {
-		if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
+		if (phy_interface_is_rgmii(dev->phydev))
 			mac_mode |= RGMII_MODE;
 
 		mac_mode |= GMAC_MODE;
@@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
 		break;
 
 	case PHY_INTERFACE_MODE_RGMII:
-		pad_mode = PAD_MODE_RGMII;
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
 		break;
 
+	case PHY_INTERFACE_MODE_RGMII_ID:
 	case PHY_INTERFACE_MODE_RGMII_TXID:
-		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+		pad_mode = PAD_MODE_RGMII;
 		break;
 
 	default:
-- 
2.11.0

[PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Marc Gonzalez]

According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
("Documentation: devicetree: clarify usage of the RGMII phy-modes")
there are 4 RGMII phy-modes to handle:

"rgmii" (RX and TX delays are added by the MAC when required)
"rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
	the MAC should not add the RX or TX delays in this case)
"rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
	the MAC should not add an RX delay in this case)
"rgmii-txid" (RGMII with internal TX delay provided by the PHY,
	the MAC should not add an TX delay in this case)

Let the MAC handle TX clock delay for rgmii and rgmii-rxid.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 drivers/net/ethernet/aurora/nb8800.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index 041cfb7952f8..f3ed320eb4ad 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -609,7 +609,7 @@ static void nb8800_mac_config(struct net_device *dev)
 		mac_mode |= HALF_DUPLEX;
 
 	if (gigabit) {
-		if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
+		if (phy_interface_is_rgmii(dev->phydev))
 			mac_mode |= RGMII_MODE;
 
 		mac_mode |= GMAC_MODE;
@@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
 		break;
 
 	case PHY_INTERFACE_MODE_RGMII:
-		pad_mode = PAD_MODE_RGMII;
+	case PHY_INTERFACE_MODE_RGMII_RXID:
+		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
 		break;
 
+	case PHY_INTERFACE_MODE_RGMII_ID:
 	case PHY_INTERFACE_MODE_RGMII_TXID:
-		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+		pad_mode = PAD_MODE_RGMII;
 		break;
 
 	default:
-- 
2.11.0

Re: [PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Måns Rullgård]

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
> ("Documentation: devicetree: clarify usage of the RGMII phy-modes")
> there are 4 RGMII phy-modes to handle:
>
> "rgmii" (RX and TX delays are added by the MAC when required)
> "rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
> 	the MAC should not add the RX or TX delays in this case)
> "rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
> 	the MAC should not add an RX delay in this case)
> "rgmii-txid" (RGMII with internal TX delay provided by the PHY,
> 	the MAC should not add an TX delay in this case)
>
> Let the MAC handle TX clock delay for rgmii and rgmii-rxid.
>
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
>  drivers/net/ethernet/aurora/nb8800.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
> index 041cfb7952f8..f3ed320eb4ad 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -609,7 +609,7 @@ static void nb8800_mac_config(struct net_device *dev)
>  		mac_mode |= HALF_DUPLEX;
>
>  	if (gigabit) {
> -		if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
> +		if (phy_interface_is_rgmii(dev->phydev))
>  			mac_mode |= RGMII_MODE;
>
>  		mac_mode |= GMAC_MODE;

This is a separate issue, and the change is obviously correct.

> @@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
>  		break;
>
>  	case PHY_INTERFACE_MODE_RGMII:
> -		pad_mode = PAD_MODE_RGMII;
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>  		break;
>
> +	case PHY_INTERFACE_MODE_RGMII_ID:
>  	case PHY_INTERFACE_MODE_RGMII_TXID:
> -		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
> +		pad_mode = PAD_MODE_RGMII;
>  		break;

Won't this just make it break in a different set of circumstances?

I think the only sane solution to this mess is to never configure the
MAC delay based on the existing phy-connection-type property.  If some
board requires this delay (because the PHY can't do it), a new property
should probably be introduced for that.

-- 
Måns Rullgård

[PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Måns Rullgård]

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
> ("Documentation: devicetree: clarify usage of the RGMII phy-modes")
> there are 4 RGMII phy-modes to handle:
>
> "rgmii" (RX and TX delays are added by the MAC when required)
> "rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
> 	the MAC should not add the RX or TX delays in this case)
> "rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
> 	the MAC should not add an RX delay in this case)
> "rgmii-txid" (RGMII with internal TX delay provided by the PHY,
> 	the MAC should not add an TX delay in this case)
>
> Let the MAC handle TX clock delay for rgmii and rgmii-rxid.
>
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
>  drivers/net/ethernet/aurora/nb8800.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
> index 041cfb7952f8..f3ed320eb4ad 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -609,7 +609,7 @@ static void nb8800_mac_config(struct net_device *dev)
>  		mac_mode |= HALF_DUPLEX;
>
>  	if (gigabit) {
> -		if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
> +		if (phy_interface_is_rgmii(dev->phydev))
>  			mac_mode |= RGMII_MODE;
>
>  		mac_mode |= GMAC_MODE;

This is a separate issue, and the change is obviously correct.

> @@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
>  		break;
>
>  	case PHY_INTERFACE_MODE_RGMII:
> -		pad_mode = PAD_MODE_RGMII;
> +	case PHY_INTERFACE_MODE_RGMII_RXID:
> +		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>  		break;
>
> +	case PHY_INTERFACE_MODE_RGMII_ID:
>  	case PHY_INTERFACE_MODE_RGMII_TXID:
> -		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
> +		pad_mode = PAD_MODE_RGMII;
>  		break;

Won't this just make it break in a different set of circumstances?

I think the only sane solution to this mess is to never configure the
MAC delay based on the existing phy-connection-type property.  If some
board requires this delay (because the PHY can't do it), a new property
should probably be introduced for that.

-- 
M?ns Rullg?rd

Re: [PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Mason]

On 19/07/2017 19:17, Måns Rullgård wrote:

> Marc Gonzalez writes:
> 
>> According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
>> ("Documentation: devicetree: clarify usage of the RGMII phy-modes")
>> there are 4 RGMII phy-modes to handle:
>>
>> "rgmii" (RX and TX delays are added by the MAC when required)
>> "rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
>> 	the MAC should not add the RX or TX delays in this case)
>> "rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
>> 	the MAC should not add an RX delay in this case)
>> "rgmii-txid" (RGMII with internal TX delay provided by the PHY,
>> 	the MAC should not add an TX delay in this case)
>>
>> Let the MAC handle TX clock delay for rgmii and rgmii-rxid.
>>
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>> ---
>>  drivers/net/ethernet/aurora/nb8800.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>> index 041cfb7952f8..f3ed320eb4ad 100644
>> --- a/drivers/net/ethernet/aurora/nb8800.c
>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>> @@ -609,7 +609,7 @@ static void nb8800_mac_config(struct net_device *dev)
>>  		mac_mode |= HALF_DUPLEX;
>>
>>  	if (gigabit) {
>> -		if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
>> +		if (phy_interface_is_rgmii(dev->phydev))
>>  			mac_mode |= RGMII_MODE;
>>
>>  		mac_mode |= GMAC_MODE;
> 
> This is a separate issue, and the change is obviously correct.
> 
>> @@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
>>  		break;
>>
>>  	case PHY_INTERFACE_MODE_RGMII:
>> -		pad_mode = PAD_MODE_RGMII;
>> +	case PHY_INTERFACE_MODE_RGMII_RXID:
>> +		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>>  		break;
>>
>> +	case PHY_INTERFACE_MODE_RGMII_ID:
>>  	case PHY_INTERFACE_MODE_RGMII_TXID:
>> -		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>> +		pad_mode = PAD_MODE_RGMII;
>>  		break;
> 
> Won't this just make it break in a different set of circumstances?

I don't think so, and here's my reasoning:

AFAIU, the HW block always requires a TX clock delay
(I don't know what the "safe" interval is. PHY adds
2.4 ns, MAC adds ~1 ns, both work.)
RX clock delay seems to be "Don't Care" (tested both
enabled and disabled by PHY)
By "tested", I mean ability to ping remote system.

If phy-mode is RGMII or RGMII_RXID, then don't add
TX clock delay from PHY, therefore add it from MAC.

If phy_mode is RGMII_ID or RGMII_TXID, then do add
TX clock delay from PHY, therefore don't add it from MAC.

What set of circumstances would create an issue?

Regards.

[PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Mason]

On 19/07/2017 19:17, M?ns Rullg?rd wrote:

> Marc Gonzalez writes:
> 
>> According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
>> ("Documentation: devicetree: clarify usage of the RGMII phy-modes")
>> there are 4 RGMII phy-modes to handle:
>>
>> "rgmii" (RX and TX delays are added by the MAC when required)
>> "rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
>> 	the MAC should not add the RX or TX delays in this case)
>> "rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
>> 	the MAC should not add an RX delay in this case)
>> "rgmii-txid" (RGMII with internal TX delay provided by the PHY,
>> 	the MAC should not add an TX delay in this case)
>>
>> Let the MAC handle TX clock delay for rgmii and rgmii-rxid.
>>
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>> ---
>>  drivers/net/ethernet/aurora/nb8800.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>> index 041cfb7952f8..f3ed320eb4ad 100644
>> --- a/drivers/net/ethernet/aurora/nb8800.c
>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>> @@ -609,7 +609,7 @@ static void nb8800_mac_config(struct net_device *dev)
>>  		mac_mode |= HALF_DUPLEX;
>>
>>  	if (gigabit) {
>> -		if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
>> +		if (phy_interface_is_rgmii(dev->phydev))
>>  			mac_mode |= RGMII_MODE;
>>
>>  		mac_mode |= GMAC_MODE;
> 
> This is a separate issue, and the change is obviously correct.
> 
>> @@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
>>  		break;
>>
>>  	case PHY_INTERFACE_MODE_RGMII:
>> -		pad_mode = PAD_MODE_RGMII;
>> +	case PHY_INTERFACE_MODE_RGMII_RXID:
>> +		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>>  		break;
>>
>> +	case PHY_INTERFACE_MODE_RGMII_ID:
>>  	case PHY_INTERFACE_MODE_RGMII_TXID:
>> -		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>> +		pad_mode = PAD_MODE_RGMII;
>>  		break;
> 
> Won't this just make it break in a different set of circumstances?

I don't think so, and here's my reasoning:

AFAIU, the HW block always requires a TX clock delay
(I don't know what the "safe" interval is. PHY adds
2.4 ns, MAC adds ~1 ns, both work.)
RX clock delay seems to be "Don't Care" (tested both
enabled and disabled by PHY)
By "tested", I mean ability to ping remote system.

If phy-mode is RGMII or RGMII_RXID, then don't add
TX clock delay from PHY, therefore add it from MAC.

If phy_mode is RGMII_ID or RGMII_TXID, then do add
TX clock delay from PHY, therefore don't add it from MAC.

What set of circumstances would create an issue?

Regards.

Re: [PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Timur Tabi]

On 07/19/2017 10:31 AM, Marc Gonzalez wrote:
> The current code supports enabling RGMII RX and TX clock delays.
> The unstated assumption is that these settings are disabled by
> default at reset, which is not the case.
> 
> RX clock delay is enabled at reset. And TX clock delay "survives"
> across SW resets. Thus, if the bootloader enables TX clock delay,
> it will remain enabled at reset in Linux.
> 
> Provide disable functions to configure the RGMII clock delays
> exactly as specified in the fwspec.

I only use SGMII mode, and I tested and can confirm that this patch does 
not break SGMII, so:

Acked-by: Timur Tabi <timur@codeaurora.org>

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Timur Tabi]

On 07/19/2017 10:31 AM, Marc Gonzalez wrote:
> The current code supports enabling RGMII RX and TX clock delays.
> The unstated assumption is that these settings are disabled by
> default at reset, which is not the case.
> 
> RX clock delay is enabled at reset. And TX clock delay "survives"
> across SW resets. Thus, if the bootloader enables TX clock delay,
> it will remain enabled at reset in Linux.
> 
> Provide disable functions to configure the RGMII clock delays
> exactly as specified in the fwspec.

I only use SGMII mode, and I tested and can confirm that this patch does 
not break SGMII, so:

Acked-by: Timur Tabi <timur@codeaurora.org>

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Florian Fainelli]

On 07/19/2017 10:36 AM, Mason wrote:
> On 19/07/2017 19:17, Måns Rullgård wrote:
> 
>> Marc Gonzalez writes:
>>
>>> According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
>>> ("Documentation: devicetree: clarify usage of the RGMII phy-modes")
>>> there are 4 RGMII phy-modes to handle:
>>>
>>> "rgmii" (RX and TX delays are added by the MAC when required)
>>> "rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
>>> 	the MAC should not add the RX or TX delays in this case)
>>> "rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
>>> 	the MAC should not add an RX delay in this case)
>>> "rgmii-txid" (RGMII with internal TX delay provided by the PHY,
>>> 	the MAC should not add an TX delay in this case)
>>>
>>> Let the MAC handle TX clock delay for rgmii and rgmii-rxid.
>>>
>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>> ---
>>>  drivers/net/ethernet/aurora/nb8800.c | 8 +++++---
>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>>> index 041cfb7952f8..f3ed320eb4ad 100644
>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>> @@ -609,7 +609,7 @@ static void nb8800_mac_config(struct net_device *dev)
>>>  		mac_mode |= HALF_DUPLEX;
>>>
>>>  	if (gigabit) {
>>> -		if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
>>> +		if (phy_interface_is_rgmii(dev->phydev))
>>>  			mac_mode |= RGMII_MODE;
>>>
>>>  		mac_mode |= GMAC_MODE;
>>
>> This is a separate issue, and the change is obviously correct.
>>
>>> @@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
>>>  		break;
>>>
>>>  	case PHY_INTERFACE_MODE_RGMII:
>>> -		pad_mode = PAD_MODE_RGMII;
>>> +	case PHY_INTERFACE_MODE_RGMII_RXID:
>>> +		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>>>  		break;
>>>
>>> +	case PHY_INTERFACE_MODE_RGMII_ID:
>>>  	case PHY_INTERFACE_MODE_RGMII_TXID:
>>> -		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>>> +		pad_mode = PAD_MODE_RGMII;
>>>  		break;
>>
>> Won't this just make it break in a different set of circumstances?
> 
> I don't think so, and here's my reasoning:
> 
> AFAIU, the HW block always requires a TX clock delay
> (I don't know what the "safe" interval is. PHY adds
> 2.4 ns, MAC adds ~1 ns, both work.)

The nominal delay should be 2ns because that's exactly what a 90 degrees
shift at a 125Mhz would be. The RGMII specification defines the following:

TskewT - Data to Clock output Skew (At Transmitter) Min: -500ns, Nom: 0,
Max: + 500 ns
TskewR - Data to Clock input Skew (At Receiver) Min: 1ns, Nom: 0, Max:
2.6ns (see note 1)

note 1: This implies that PC board design will require clocks to be
routed such that an additional trace delay of greater than 1.5ns and
less than 2.0ns will be added to the associated clock signal. For 10/100
the Max value is unspecified.

So it seems to me like you are borderline spec in both delays you gave
here and the "HW block always requires a TX clock delay" statement is
true for a given board design only.


> RX clock delay seems to be "Don't Care" (tested both
> enabled and disabled by PHY)
> By "tested", I mean ability to ping remote system.

Can you do something a bit more stressful than just a ping, also if you
have the ability to change the inter-packet gap, do it, and see if you
start seeing FCS or any other decoding errors.

> 
> If phy-mode is RGMII or RGMII_RXID, then don't add
> TX clock delay from PHY, therefore add it from MAC.
> 
> If phy_mode is RGMII_ID or RGMII_TXID, then do add
> TX clock delay from PHY, therefore don't add it from MAC.
> 
> What set of circumstances would create an issue?

Existing Device Tree sources that do not correspond to that description
you just did, I suppose they are all out of tree?
-- 
Florian

[PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Florian Fainelli]

On 07/19/2017 10:36 AM, Mason wrote:
> On 19/07/2017 19:17, M?ns Rullg?rd wrote:
> 
>> Marc Gonzalez writes:
>>
>>> According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
>>> ("Documentation: devicetree: clarify usage of the RGMII phy-modes")
>>> there are 4 RGMII phy-modes to handle:
>>>
>>> "rgmii" (RX and TX delays are added by the MAC when required)
>>> "rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
>>> 	the MAC should not add the RX or TX delays in this case)
>>> "rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
>>> 	the MAC should not add an RX delay in this case)
>>> "rgmii-txid" (RGMII with internal TX delay provided by the PHY,
>>> 	the MAC should not add an TX delay in this case)
>>>
>>> Let the MAC handle TX clock delay for rgmii and rgmii-rxid.
>>>
>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>> ---
>>>  drivers/net/ethernet/aurora/nb8800.c | 8 +++++---
>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>>> index 041cfb7952f8..f3ed320eb4ad 100644
>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>> @@ -609,7 +609,7 @@ static void nb8800_mac_config(struct net_device *dev)
>>>  		mac_mode |= HALF_DUPLEX;
>>>
>>>  	if (gigabit) {
>>> -		if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
>>> +		if (phy_interface_is_rgmii(dev->phydev))
>>>  			mac_mode |= RGMII_MODE;
>>>
>>>  		mac_mode |= GMAC_MODE;
>>
>> This is a separate issue, and the change is obviously correct.
>>
>>> @@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
>>>  		break;
>>>
>>>  	case PHY_INTERFACE_MODE_RGMII:
>>> -		pad_mode = PAD_MODE_RGMII;
>>> +	case PHY_INTERFACE_MODE_RGMII_RXID:
>>> +		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>>>  		break;
>>>
>>> +	case PHY_INTERFACE_MODE_RGMII_ID:
>>>  	case PHY_INTERFACE_MODE_RGMII_TXID:
>>> -		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>>> +		pad_mode = PAD_MODE_RGMII;
>>>  		break;
>>
>> Won't this just make it break in a different set of circumstances?
> 
> I don't think so, and here's my reasoning:
> 
> AFAIU, the HW block always requires a TX clock delay
> (I don't know what the "safe" interval is. PHY adds
> 2.4 ns, MAC adds ~1 ns, both work.)

The nominal delay should be 2ns because that's exactly what a 90 degrees
shift at a 125Mhz would be. The RGMII specification defines the following:

TskewT - Data to Clock output Skew (At Transmitter) Min: -500ns, Nom: 0,
Max: + 500 ns
TskewR - Data to Clock input Skew (At Receiver) Min: 1ns, Nom: 0, Max:
2.6ns (see note 1)

note 1: This implies that PC board design will require clocks to be
routed such that an additional trace delay of greater than 1.5ns and
less than 2.0ns will be added to the associated clock signal. For 10/100
the Max value is unspecified.

So it seems to me like you are borderline spec in both delays you gave
here and the "HW block always requires a TX clock delay" statement is
true for a given board design only.


> RX clock delay seems to be "Don't Care" (tested both
> enabled and disabled by PHY)
> By "tested", I mean ability to ping remote system.

Can you do something a bit more stressful than just a ping, also if you
have the ability to change the inter-packet gap, do it, and see if you
start seeing FCS or any other decoding errors.

> 
> If phy-mode is RGMII or RGMII_RXID, then don't add
> TX clock delay from PHY, therefore add it from MAC.
> 
> If phy_mode is RGMII_ID or RGMII_TXID, then do add
> TX clock delay from PHY, therefore don't add it from MAC.
> 
> What set of circumstances would create an issue?

Existing Device Tree sources that do not correspond to that description
you just did, I suppose they are all out of tree?
-- 
Florian

Re: [PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Grygorii Strashko]

Hi

On 07/19/2017 10:31 AM, Marc Gonzalez wrote:
> The current code supports enabling RGMII RX and TX clock delays.
> The unstated assumption is that these settings are disabled by
> default at reset, which is not the case.
> 
> RX clock delay is enabled at reset. And TX clock delay "survives"
> across SW resets. Thus, if the bootloader enables TX clock delay,
> it will remain enabled at reset in Linux.
> 
> Provide disable functions to configure the RGMII clock delays
> exactly as specified in the fwspec.
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
>   drivers/net/phy/at803x.c | 32 ++++++++++++++++++++++++--------
>   1 file changed, 24 insertions(+), 8 deletions(-)
This patch breaks am335x-evm networking.

To restore it I've had to apply below diff:
diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
index 200d6ab..9578bdf 100644
--- a/arch/arm/boot/dts/am335x-evm.dts
+++ b/arch/arm/boot/dts/am335x-evm.dts
@@ -724,12 +724,12 @@
 
 &cpsw_emac0 {
        phy_id = <&davinci_mdio>, <0>;
-       phy-mode = "rgmii-txid";
+       phy-mode = "rgmii-id";
 };
 
 &cpsw_emac1 {
        phy_id = <&davinci_mdio>, <1>;
-       phy-mode = "rgmii-txid";
+       phy-mode = "rgmii-id";
 };
 
 &tscadc {

Sry, can't comment here to much - not E-PHY expert.

-- 
regards,
-grygorii

[PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Grygorii Strashko]

Hi

On 07/19/2017 10:31 AM, Marc Gonzalez wrote:
> The current code supports enabling RGMII RX and TX clock delays.
> The unstated assumption is that these settings are disabled by
> default at reset, which is not the case.
> 
> RX clock delay is enabled at reset. And TX clock delay "survives"
> across SW resets. Thus, if the bootloader enables TX clock delay,
> it will remain enabled at reset in Linux.
> 
> Provide disable functions to configure the RGMII clock delays
> exactly as specified in the fwspec.
> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
>   drivers/net/phy/at803x.c | 32 ++++++++++++++++++++++++--------
>   1 file changed, 24 insertions(+), 8 deletions(-)
This patch breaks am335x-evm networking.

To restore it I've had to apply below diff:
diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
index 200d6ab..9578bdf 100644
--- a/arch/arm/boot/dts/am335x-evm.dts
+++ b/arch/arm/boot/dts/am335x-evm.dts
@@ -724,12 +724,12 @@
 
 &cpsw_emac0 {
        phy_id = <&davinci_mdio>, <0>;
-       phy-mode = "rgmii-txid";
+       phy-mode = "rgmii-id";
 };
 
 &cpsw_emac1 {
        phy_id = <&davinci_mdio>, <1>;
-       phy-mode = "rgmii-txid";
+       phy-mode = "rgmii-id";
 };
 
 &tscadc {

Sry, can't comment here to much - not E-PHY expert.

-- 
regards,
-grygorii

Re: [PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Florian Fainelli]

On 07/19/2017 12:24 PM, Grygorii Strashko wrote:
> Hi
> 
> On 07/19/2017 10:31 AM, Marc Gonzalez wrote:
>> The current code supports enabling RGMII RX and TX clock delays.
>> The unstated assumption is that these settings are disabled by
>> default at reset, which is not the case.
>>
>> RX clock delay is enabled at reset. And TX clock delay "survives"
>> across SW resets. Thus, if the bootloader enables TX clock delay,
>> it will remain enabled at reset in Linux.
>>
>> Provide disable functions to configure the RGMII clock delays
>> exactly as specified in the fwspec.
>>
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>> ---
>>   drivers/net/phy/at803x.c | 32 ++++++++++++++++++++++++--------
>>   1 file changed, 24 insertions(+), 8 deletions(-)
> This patch breaks am335x-evm networking.
> 
> To restore it I've had to apply below diff:
> diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
> index 200d6ab..9578bdf 100644
> --- a/arch/arm/boot/dts/am335x-evm.dts
> +++ b/arch/arm/boot/dts/am335x-evm.dts
> @@ -724,12 +724,12 @@
>  
>  &cpsw_emac0 {
>         phy_id = <&davinci_mdio>, <0>;
> -       phy-mode = "rgmii-txid";
> +       phy-mode = "rgmii-id";
>  };
>  
>  &cpsw_emac1 {
>         phy_id = <&davinci_mdio>, <1>;
> -       phy-mode = "rgmii-txid";
> +       phy-mode = "rgmii-id";
>  };
>  
>  &tscadc {
> 
> Sry, can't comment here to much - not E-PHY expert.

It's useful feedback, since we had poorly defined "phy-mode" semantics
for too long, this is totally expected, Marc this is exactly why Mans is
suggesting additional MAC-specific properties to define delays.
-- 
Florian

[PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Florian Fainelli]

On 07/19/2017 12:24 PM, Grygorii Strashko wrote:
> Hi
> 
> On 07/19/2017 10:31 AM, Marc Gonzalez wrote:
>> The current code supports enabling RGMII RX and TX clock delays.
>> The unstated assumption is that these settings are disabled by
>> default at reset, which is not the case.
>>
>> RX clock delay is enabled at reset. And TX clock delay "survives"
>> across SW resets. Thus, if the bootloader enables TX clock delay,
>> it will remain enabled at reset in Linux.
>>
>> Provide disable functions to configure the RGMII clock delays
>> exactly as specified in the fwspec.
>>
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>> ---
>>   drivers/net/phy/at803x.c | 32 ++++++++++++++++++++++++--------
>>   1 file changed, 24 insertions(+), 8 deletions(-)
> This patch breaks am335x-evm networking.
> 
> To restore it I've had to apply below diff:
> diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
> index 200d6ab..9578bdf 100644
> --- a/arch/arm/boot/dts/am335x-evm.dts
> +++ b/arch/arm/boot/dts/am335x-evm.dts
> @@ -724,12 +724,12 @@
>  
>  &cpsw_emac0 {
>         phy_id = <&davinci_mdio>, <0>;
> -       phy-mode = "rgmii-txid";
> +       phy-mode = "rgmii-id";
>  };
>  
>  &cpsw_emac1 {
>         phy_id = <&davinci_mdio>, <1>;
> -       phy-mode = "rgmii-txid";
> +       phy-mode = "rgmii-id";
>  };
>  
>  &tscadc {
> 
> Sry, can't comment here to much - not E-PHY expert.

It's useful feedback, since we had poorly defined "phy-mode" semantics
for too long, this is totally expected, Marc this is exactly why Mans is
suggesting additional MAC-specific properties to define delays.
-- 
Florian

Re: [PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Grygorii Strashko]

On 07/19/2017 02:30 PM, Florian Fainelli wrote:
> On 07/19/2017 12:24 PM, Grygorii Strashko wrote:
>> Hi
>>
>> On 07/19/2017 10:31 AM, Marc Gonzalez wrote:
>>> The current code supports enabling RGMII RX and TX clock delays.
>>> The unstated assumption is that these settings are disabled by
>>> default at reset, which is not the case.
>>>
>>> RX clock delay is enabled at reset. And TX clock delay "survives"
>>> across SW resets. Thus, if the bootloader enables TX clock delay,
>>> it will remain enabled at reset in Linux.
>>>
>>> Provide disable functions to configure the RGMII clock delays
>>> exactly as specified in the fwspec.
>>>
>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>> ---
>>>    drivers/net/phy/at803x.c | 32 ++++++++++++++++++++++++--------
>>>    1 file changed, 24 insertions(+), 8 deletions(-)
>> This patch breaks am335x-evm networking.
>>
>> To restore it I've had to apply below diff:
>> diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
>> index 200d6ab..9578bdf 100644
>> --- a/arch/arm/boot/dts/am335x-evm.dts
>> +++ b/arch/arm/boot/dts/am335x-evm.dts
>> @@ -724,12 +724,12 @@
>>   
>>   &cpsw_emac0 {
>>          phy_id = <&davinci_mdio>, <0>;
>> -       phy-mode = "rgmii-txid";
>> +       phy-mode = "rgmii-id";
>>   };
>>   
>>   &cpsw_emac1 {
>>          phy_id = <&davinci_mdio>, <1>;
>> -       phy-mode = "rgmii-txid";
>> +       phy-mode = "rgmii-id";
>>   };
>>   
>>   &tscadc {
>>
>> Sry, can't comment here to much - not E-PHY expert.
> 
> It's useful feedback, since we had poorly defined "phy-mode" semantics
> for too long, this is totally expected, Marc this is exactly why Mans is
> suggesting additional MAC-specific properties to define delays.
> 

Yeah. original commit is pretty old and description is not very useful

commit 6d75afe2916adf9e9de6862275cdf89b9b7e4d0e
Author: Mugunthan V N <mugunthanvnm@ti.com>
Date:   Mon Jun 3 20:10:11 2013 +0000

     ARM: dts: AM33XX: Add phy-mode to CPSW node


-- 
regards,
-grygorii

[PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Grygorii Strashko]

On 07/19/2017 02:30 PM, Florian Fainelli wrote:
> On 07/19/2017 12:24 PM, Grygorii Strashko wrote:
>> Hi
>>
>> On 07/19/2017 10:31 AM, Marc Gonzalez wrote:
>>> The current code supports enabling RGMII RX and TX clock delays.
>>> The unstated assumption is that these settings are disabled by
>>> default at reset, which is not the case.
>>>
>>> RX clock delay is enabled at reset. And TX clock delay "survives"
>>> across SW resets. Thus, if the bootloader enables TX clock delay,
>>> it will remain enabled at reset in Linux.
>>>
>>> Provide disable functions to configure the RGMII clock delays
>>> exactly as specified in the fwspec.
>>>
>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>> ---
>>>    drivers/net/phy/at803x.c | 32 ++++++++++++++++++++++++--------
>>>    1 file changed, 24 insertions(+), 8 deletions(-)
>> This patch breaks am335x-evm networking.
>>
>> To restore it I've had to apply below diff:
>> diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
>> index 200d6ab..9578bdf 100644
>> --- a/arch/arm/boot/dts/am335x-evm.dts
>> +++ b/arch/arm/boot/dts/am335x-evm.dts
>> @@ -724,12 +724,12 @@
>>   
>>   &cpsw_emac0 {
>>          phy_id = <&davinci_mdio>, <0>;
>> -       phy-mode = "rgmii-txid";
>> +       phy-mode = "rgmii-id";
>>   };
>>   
>>   &cpsw_emac1 {
>>          phy_id = <&davinci_mdio>, <1>;
>> -       phy-mode = "rgmii-txid";
>> +       phy-mode = "rgmii-id";
>>   };
>>   
>>   &tscadc {
>>
>> Sry, can't comment here to much - not E-PHY expert.
> 
> It's useful feedback, since we had poorly defined "phy-mode" semantics
> for too long, this is totally expected, Marc this is exactly why Mans is
> suggesting additional MAC-specific properties to define delays.
> 

Yeah. original commit is pretty old and description is not very useful

commit 6d75afe2916adf9e9de6862275cdf89b9b7e4d0e
Author: Mugunthan V N <mugunthanvnm@ti.com>
Date:   Mon Jun 3 20:10:11 2013 +0000

     ARM: dts: AM33XX: Add phy-mode to CPSW node


-- 
regards,
-grygorii

Re: [PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Mason]

On 19/07/2017 20:30, Florian Fainelli wrote:
> On 07/19/2017 10:36 AM, Mason wrote:
>> On 19/07/2017 19:17, Måns Rullgård wrote:
>>
>>> Marc Gonzalez writes:
>>>
>>>> According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
>>>> ("Documentation: devicetree: clarify usage of the RGMII phy-modes")
>>>> there are 4 RGMII phy-modes to handle:
>>>>
>>>> "rgmii" (RX and TX delays are added by the MAC when required)
>>>> "rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
>>>> 	the MAC should not add the RX or TX delays in this case)
>>>> "rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
>>>> 	the MAC should not add an RX delay in this case)
>>>> "rgmii-txid" (RGMII with internal TX delay provided by the PHY,
>>>> 	the MAC should not add an TX delay in this case)
>>>>
>>>> Let the MAC handle TX clock delay for rgmii and rgmii-rxid.
>>>>
>>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>>> ---
>>>>  drivers/net/ethernet/aurora/nb8800.c | 8 +++++---
>>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>>>> index 041cfb7952f8..f3ed320eb4ad 100644
>>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>>> @@ -609,7 +609,7 @@ static void nb8800_mac_config(struct net_device *dev)
>>>>  		mac_mode |= HALF_DUPLEX;
>>>>
>>>>  	if (gigabit) {
>>>> -		if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
>>>> +		if (phy_interface_is_rgmii(dev->phydev))
>>>>  			mac_mode |= RGMII_MODE;
>>>>
>>>>  		mac_mode |= GMAC_MODE;
>>>
>>> This is a separate issue, and the change is obviously correct.
>>>
>>>> @@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
>>>>  		break;
>>>>
>>>>  	case PHY_INTERFACE_MODE_RGMII:
>>>> -		pad_mode = PAD_MODE_RGMII;
>>>> +	case PHY_INTERFACE_MODE_RGMII_RXID:
>>>> +		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>>>>  		break;
>>>>
>>>> +	case PHY_INTERFACE_MODE_RGMII_ID:
>>>>  	case PHY_INTERFACE_MODE_RGMII_TXID:
>>>> -		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>>>> +		pad_mode = PAD_MODE_RGMII;
>>>>  		break;
>>>
>>> Won't this just make it break in a different set of circumstances?
>>
>> I don't think so, and here's my reasoning:
>>
>> AFAIU, the HW block always requires a TX clock delay
>> (I don't know what the "safe" interval is. PHY adds
>> 2.4 ns, MAC adds ~1 ns, both work.)
> 
> The nominal delay should be 2ns because that's exactly what a 90 degrees
> shift at a 125Mhz would be. The RGMII specification defines the following:
> 
> TskewT - Data to Clock output Skew (At Transmitter) Min: -500ns, Nom: 0,
> Max: + 500 ns
> TskewR - Data to Clock input Skew (At Receiver) Min: 1ns, Nom: 0, Max:
> 2.6ns (see note 1)
> 
> note 1: This implies that PC board design will require clocks to be
> routed such that an additional trace delay of greater than 1.5ns and
> less than 2.0ns will be added to the associated clock signal. For 10/100
> the Max value is unspecified.
> 
> So it seems to me like you are borderline spec in both delays you gave
> here and the "HW block always requires a TX clock delay" statement is
> true for a given board design only.

I must confess that my understanding of clock delays,
clock skew, routing, traces, etc is nil.

Is TskewT the TX clock delay?
And TskewR the RX clock delay?

Doesn't wire delay factor in too?
(So longer wires require more delay.)

>> RX clock delay seems to be "Don't Care" (tested both
>> enabled and disabled by PHY)
>> By "tested", I mean ability to ping remote system.
> 
> Can you do something a bit more stressful than just a ping, also if you
> have the ability to change the inter-packet gap, do it, and see if you
> start seeing FCS or any other decoding errors.

Errr... "Inter-packet gap"?
Is there supposed to be a HW knob to tweak how long
the HW waits between sending two frames?

>> If phy-mode is RGMII or RGMII_RXID, then don't add
>> TX clock delay from PHY, therefore add it from MAC.
>>
>> If phy_mode is RGMII_ID or RGMII_TXID, then do add
>> TX clock delay from PHY, therefore don't add it from MAC.
>>
>> What set of circumstances would create an issue?
> 
> Existing Device Tree sources that do not correspond to that description
> you just did, I suppose they are all out of tree?

The problem with PHY drivers is that there is no
simple compatible string to grep for.

The tango boards use "ethernet-phy-id004d.d072"
but not a single other DT uses that string.
For example, am335x-evm.dts doesn't seem to name the PHY.
Hmmm, how does the at803x probe function match for that
board?

How does one estimate the impact of driver changes in
the eth PHY layer?

Regards.

[PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Mason]

On 19/07/2017 20:30, Florian Fainelli wrote:
> On 07/19/2017 10:36 AM, Mason wrote:
>> On 19/07/2017 19:17, M?ns Rullg?rd wrote:
>>
>>> Marc Gonzalez writes:
>>>
>>>> According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
>>>> ("Documentation: devicetree: clarify usage of the RGMII phy-modes")
>>>> there are 4 RGMII phy-modes to handle:
>>>>
>>>> "rgmii" (RX and TX delays are added by the MAC when required)
>>>> "rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
>>>> 	the MAC should not add the RX or TX delays in this case)
>>>> "rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
>>>> 	the MAC should not add an RX delay in this case)
>>>> "rgmii-txid" (RGMII with internal TX delay provided by the PHY,
>>>> 	the MAC should not add an TX delay in this case)
>>>>
>>>> Let the MAC handle TX clock delay for rgmii and rgmii-rxid.
>>>>
>>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>>> ---
>>>>  drivers/net/ethernet/aurora/nb8800.c | 8 +++++---
>>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>>>> index 041cfb7952f8..f3ed320eb4ad 100644
>>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>>> @@ -609,7 +609,7 @@ static void nb8800_mac_config(struct net_device *dev)
>>>>  		mac_mode |= HALF_DUPLEX;
>>>>
>>>>  	if (gigabit) {
>>>> -		if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
>>>> +		if (phy_interface_is_rgmii(dev->phydev))
>>>>  			mac_mode |= RGMII_MODE;
>>>>
>>>>  		mac_mode |= GMAC_MODE;
>>>
>>> This is a separate issue, and the change is obviously correct.
>>>
>>>> @@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
>>>>  		break;
>>>>
>>>>  	case PHY_INTERFACE_MODE_RGMII:
>>>> -		pad_mode = PAD_MODE_RGMII;
>>>> +	case PHY_INTERFACE_MODE_RGMII_RXID:
>>>> +		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>>>>  		break;
>>>>
>>>> +	case PHY_INTERFACE_MODE_RGMII_ID:
>>>>  	case PHY_INTERFACE_MODE_RGMII_TXID:
>>>> -		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>>>> +		pad_mode = PAD_MODE_RGMII;
>>>>  		break;
>>>
>>> Won't this just make it break in a different set of circumstances?
>>
>> I don't think so, and here's my reasoning:
>>
>> AFAIU, the HW block always requires a TX clock delay
>> (I don't know what the "safe" interval is. PHY adds
>> 2.4 ns, MAC adds ~1 ns, both work.)
> 
> The nominal delay should be 2ns because that's exactly what a 90 degrees
> shift at a 125Mhz would be. The RGMII specification defines the following:
> 
> TskewT - Data to Clock output Skew (At Transmitter) Min: -500ns, Nom: 0,
> Max: + 500 ns
> TskewR - Data to Clock input Skew (At Receiver) Min: 1ns, Nom: 0, Max:
> 2.6ns (see note 1)
> 
> note 1: This implies that PC board design will require clocks to be
> routed such that an additional trace delay of greater than 1.5ns and
> less than 2.0ns will be added to the associated clock signal. For 10/100
> the Max value is unspecified.
> 
> So it seems to me like you are borderline spec in both delays you gave
> here and the "HW block always requires a TX clock delay" statement is
> true for a given board design only.

I must confess that my understanding of clock delays,
clock skew, routing, traces, etc is nil.

Is TskewT the TX clock delay?
And TskewR the RX clock delay?

Doesn't wire delay factor in too?
(So longer wires require more delay.)

>> RX clock delay seems to be "Don't Care" (tested both
>> enabled and disabled by PHY)
>> By "tested", I mean ability to ping remote system.
> 
> Can you do something a bit more stressful than just a ping, also if you
> have the ability to change the inter-packet gap, do it, and see if you
> start seeing FCS or any other decoding errors.

Errr... "Inter-packet gap"?
Is there supposed to be a HW knob to tweak how long
the HW waits between sending two frames?

>> If phy-mode is RGMII or RGMII_RXID, then don't add
>> TX clock delay from PHY, therefore add it from MAC.
>>
>> If phy_mode is RGMII_ID or RGMII_TXID, then do add
>> TX clock delay from PHY, therefore don't add it from MAC.
>>
>> What set of circumstances would create an issue?
> 
> Existing Device Tree sources that do not correspond to that description
> you just did, I suppose they are all out of tree?

The problem with PHY drivers is that there is no
simple compatible string to grep for.

The tango boards use "ethernet-phy-id004d.d072"
but not a single other DT uses that string.
For example, am335x-evm.dts doesn't seem to name the PHY.
Hmmm, how does the at803x probe function match for that
board?

How does one estimate the impact of driver changes in
the eth PHY layer?

Regards.

Re: [PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Mason]

On 19/07/2017 21:30, Florian Fainelli wrote:
> On 07/19/2017 12:24 PM, Grygorii Strashko wrote:
>> Hi
>>
>> On 07/19/2017 10:31 AM, Marc Gonzalez wrote:
>>> The current code supports enabling RGMII RX and TX clock delays.
>>> The unstated assumption is that these settings are disabled by
>>> default at reset, which is not the case.
>>>
>>> RX clock delay is enabled at reset. And TX clock delay "survives"
>>> across SW resets. Thus, if the bootloader enables TX clock delay,
>>> it will remain enabled at reset in Linux.
>>>
>>> Provide disable functions to configure the RGMII clock delays
>>> exactly as specified in the fwspec.
>>>
>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>> ---
>>>   drivers/net/phy/at803x.c | 32 ++++++++++++++++++++++++--------
>>>   1 file changed, 24 insertions(+), 8 deletions(-)
>> This patch breaks am335x-evm networking.
>>
>> To restore it I've had to apply below diff:
>> diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
>> index 200d6ab..9578bdf 100644
>> --- a/arch/arm/boot/dts/am335x-evm.dts
>> +++ b/arch/arm/boot/dts/am335x-evm.dts
>> @@ -724,12 +724,12 @@
>>  
>>  &cpsw_emac0 {
>>         phy_id = <&davinci_mdio>, <0>;
>> -       phy-mode = "rgmii-txid";
>> +       phy-mode = "rgmii-id";
>>  };
>>  
>>  &cpsw_emac1 {
>>         phy_id = <&davinci_mdio>, <1>;
>> -       phy-mode = "rgmii-txid";
>> +       phy-mode = "rgmii-id";
>>  };
>>  
>>  &tscadc {
>>
>> Sry, can't comment here to much - not E-PHY expert.
> 
> It's useful feedback, since we had poorly defined "phy-mode" semantics
> for too long, this is totally expected, Marc this is exactly why Mans is
> suggesting additional MAC-specific properties to define delays.

In the current situation, it is impossible to configure
the at803x to disable RX clock delay or TX clock delay
(in case the boot loader enabled it).

Are you saying that, because no one has had a problem
so far, it is not possible to fix it now, as it would
break boards like am335x-evm.dts which didn't request
RX clock delay, but got one anyway?

Does that mean we cannot support boards using AR8035
that need the RX and TX clock delays disabled?

I'm not sure how the MAC-specific properties can save
the day?

Regards.

[PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Mason]

On 19/07/2017 21:30, Florian Fainelli wrote:
> On 07/19/2017 12:24 PM, Grygorii Strashko wrote:
>> Hi
>>
>> On 07/19/2017 10:31 AM, Marc Gonzalez wrote:
>>> The current code supports enabling RGMII RX and TX clock delays.
>>> The unstated assumption is that these settings are disabled by
>>> default at reset, which is not the case.
>>>
>>> RX clock delay is enabled at reset. And TX clock delay "survives"
>>> across SW resets. Thus, if the bootloader enables TX clock delay,
>>> it will remain enabled at reset in Linux.
>>>
>>> Provide disable functions to configure the RGMII clock delays
>>> exactly as specified in the fwspec.
>>>
>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>> ---
>>>   drivers/net/phy/at803x.c | 32 ++++++++++++++++++++++++--------
>>>   1 file changed, 24 insertions(+), 8 deletions(-)
>> This patch breaks am335x-evm networking.
>>
>> To restore it I've had to apply below diff:
>> diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
>> index 200d6ab..9578bdf 100644
>> --- a/arch/arm/boot/dts/am335x-evm.dts
>> +++ b/arch/arm/boot/dts/am335x-evm.dts
>> @@ -724,12 +724,12 @@
>>  
>>  &cpsw_emac0 {
>>         phy_id = <&davinci_mdio>, <0>;
>> -       phy-mode = "rgmii-txid";
>> +       phy-mode = "rgmii-id";
>>  };
>>  
>>  &cpsw_emac1 {
>>         phy_id = <&davinci_mdio>, <1>;
>> -       phy-mode = "rgmii-txid";
>> +       phy-mode = "rgmii-id";
>>  };
>>  
>>  &tscadc {
>>
>> Sry, can't comment here to much - not E-PHY expert.
> 
> It's useful feedback, since we had poorly defined "phy-mode" semantics
> for too long, this is totally expected, Marc this is exactly why Mans is
> suggesting additional MAC-specific properties to define delays.

In the current situation, it is impossible to configure
the at803x to disable RX clock delay or TX clock delay
(in case the boot loader enabled it).

Are you saying that, because no one has had a problem
so far, it is not possible to fix it now, as it would
break boards like am335x-evm.dts which didn't request
RX clock delay, but got one anyway?

Does that mean we cannot support boards using AR8035
that need the RX and TX clock delays disabled?

I'm not sure how the MAC-specific properties can save
the day?

Regards.

Re: [PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Florian Fainelli]

On 07/19/2017 02:15 PM, Mason wrote:
> On 19/07/2017 20:30, Florian Fainelli wrote:
>> On 07/19/2017 10:36 AM, Mason wrote:
>>> On 19/07/2017 19:17, Måns Rullgård wrote:
>>>
>>>> Marc Gonzalez writes:
>>>>
>>>>> According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
>>>>> ("Documentation: devicetree: clarify usage of the RGMII phy-modes")
>>>>> there are 4 RGMII phy-modes to handle:
>>>>>
>>>>> "rgmii" (RX and TX delays are added by the MAC when required)
>>>>> "rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
>>>>> 	the MAC should not add the RX or TX delays in this case)
>>>>> "rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
>>>>> 	the MAC should not add an RX delay in this case)
>>>>> "rgmii-txid" (RGMII with internal TX delay provided by the PHY,
>>>>> 	the MAC should not add an TX delay in this case)
>>>>>
>>>>> Let the MAC handle TX clock delay for rgmii and rgmii-rxid.
>>>>>
>>>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>>>> ---
>>>>>  drivers/net/ethernet/aurora/nb8800.c | 8 +++++---
>>>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>>>>> index 041cfb7952f8..f3ed320eb4ad 100644
>>>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>>>> @@ -609,7 +609,7 @@ static void nb8800_mac_config(struct net_device *dev)
>>>>>  		mac_mode |= HALF_DUPLEX;
>>>>>
>>>>>  	if (gigabit) {
>>>>> -		if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
>>>>> +		if (phy_interface_is_rgmii(dev->phydev))
>>>>>  			mac_mode |= RGMII_MODE;
>>>>>
>>>>>  		mac_mode |= GMAC_MODE;
>>>>
>>>> This is a separate issue, and the change is obviously correct.
>>>>
>>>>> @@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
>>>>>  		break;
>>>>>
>>>>>  	case PHY_INTERFACE_MODE_RGMII:
>>>>> -		pad_mode = PAD_MODE_RGMII;
>>>>> +	case PHY_INTERFACE_MODE_RGMII_RXID:
>>>>> +		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>>>>>  		break;
>>>>>
>>>>> +	case PHY_INTERFACE_MODE_RGMII_ID:
>>>>>  	case PHY_INTERFACE_MODE_RGMII_TXID:
>>>>> -		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>>>>> +		pad_mode = PAD_MODE_RGMII;
>>>>>  		break;
>>>>
>>>> Won't this just make it break in a different set of circumstances?
>>>
>>> I don't think so, and here's my reasoning:
>>>
>>> AFAIU, the HW block always requires a TX clock delay
>>> (I don't know what the "safe" interval is. PHY adds
>>> 2.4 ns, MAC adds ~1 ns, both work.)
>>
>> The nominal delay should be 2ns because that's exactly what a 90 degrees
>> shift at a 125Mhz would be. The RGMII specification defines the following:
>>
>> TskewT - Data to Clock output Skew (At Transmitter) Min: -500ns, Nom: 0,
>> Max: + 500 ns
>> TskewR - Data to Clock input Skew (At Receiver) Min: 1ns, Nom: 0, Max:
>> 2.6ns (see note 1)
>>
>> note 1: This implies that PC board design will require clocks to be
>> routed such that an additional trace delay of greater than 1.5ns and
>> less than 2.0ns will be added to the associated clock signal. For 10/100
>> the Max value is unspecified.
>>
>> So it seems to me like you are borderline spec in both delays you gave
>> here and the "HW block always requires a TX clock delay" statement is
>> true for a given board design only.
> 
> I must confess that my understanding of clock delays,
> clock skew, routing, traces, etc is nil.
> 
> Is TskewT the TX clock delay?
> And TskewR the RX clock delay?
> 
> Doesn't wire delay factor in too?
> (So longer wires require more delay.)

How about you start reading the RGMII specification so we can at least,
if nothing else agree on the terminology? It's public:

http://web.archive.org/web/20160303171328/http://www.hp.com/rnd/pdfs/RGMIIv2_0_final_hp.pdf

> 
>>> RX clock delay seems to be "Don't Care" (tested both
>>> enabled and disabled by PHY)
>>> By "tested", I mean ability to ping remote system.
>>
>> Can you do something a bit more stressful than just a ping, also if you
>> have the ability to change the inter-packet gap, do it, and see if you
>> start seeing FCS or any other decoding errors.
> 
> Errr... "Inter-packet gap"?
> Is there supposed to be a HW knob to tweak how long
> the HW waits between sending two frames?

Some Ethernet controllers let you change it, some don't, if nb8800
allows it, it's good for testing in that it packs more frames per
quantum of time. If not, do you have at least a FCS error counter?

> 
>>> If phy-mode is RGMII or RGMII_RXID, then don't add
>>> TX clock delay from PHY, therefore add it from MAC.
>>>
>>> If phy_mode is RGMII_ID or RGMII_TXID, then do add
>>> TX clock delay from PHY, therefore don't add it from MAC.
>>>
>>> What set of circumstances would create an issue?
>>
>> Existing Device Tree sources that do not correspond to that description
>> you just did, I suppose they are all out of tree?
> 
> The problem with PHY drivers is that there is no
> simple compatible string to grep for.

That does not help for sure.

> 
> The tango boards use "ethernet-phy-id004d.d072"
> but not a single other DT uses that string.
> For example, am335x-evm.dts doesn't seem to name the PHY.
> Hmmm, how does the at803x probe function match for that
> board?

of_mdiobus_register() scans the Device Tree and for each populated PHY
node, reads its OUI register and and matches the PHY OUI with the driver
list that it has. Quite similar to PCI or USB in fact.

> 
> How does one estimate the impact of driver changes in
> the eth PHY layer?

It's hard, because there are many Ethernet MACs and PHY drivers that are
all inter-operable with each other, which is both a blessing and a curse.

I completely understand what you want to solve but I suspect you will
have to do it in a way where you either accept that you may not be fully
compliant with the now clarified "phy-mode" description, in order not to
break other people's set up that were already non-compliant (can't blame
them, they did not know back then), or you will have to use additional
MAC properties to override the delay settings on the MAC or PHY side.
-- 
Florian

[PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Florian Fainelli]

On 07/19/2017 02:15 PM, Mason wrote:
> On 19/07/2017 20:30, Florian Fainelli wrote:
>> On 07/19/2017 10:36 AM, Mason wrote:
>>> On 19/07/2017 19:17, M?ns Rullg?rd wrote:
>>>
>>>> Marc Gonzalez writes:
>>>>
>>>>> According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
>>>>> ("Documentation: devicetree: clarify usage of the RGMII phy-modes")
>>>>> there are 4 RGMII phy-modes to handle:
>>>>>
>>>>> "rgmii" (RX and TX delays are added by the MAC when required)
>>>>> "rgmii-id" (RGMII with internal RX and TX delays provided by the PHY,
>>>>> 	the MAC should not add the RX or TX delays in this case)
>>>>> "rgmii-rxid" (RGMII with internal RX delay provided by the PHY,
>>>>> 	the MAC should not add an RX delay in this case)
>>>>> "rgmii-txid" (RGMII with internal TX delay provided by the PHY,
>>>>> 	the MAC should not add an TX delay in this case)
>>>>>
>>>>> Let the MAC handle TX clock delay for rgmii and rgmii-rxid.
>>>>>
>>>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>>>> ---
>>>>>  drivers/net/ethernet/aurora/nb8800.c | 8 +++++---
>>>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>>>>> index 041cfb7952f8..f3ed320eb4ad 100644
>>>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>>>> @@ -609,7 +609,7 @@ static void nb8800_mac_config(struct net_device *dev)
>>>>>  		mac_mode |= HALF_DUPLEX;
>>>>>
>>>>>  	if (gigabit) {
>>>>> -		if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
>>>>> +		if (phy_interface_is_rgmii(dev->phydev))
>>>>>  			mac_mode |= RGMII_MODE;
>>>>>
>>>>>  		mac_mode |= GMAC_MODE;
>>>>
>>>> This is a separate issue, and the change is obviously correct.
>>>>
>>>>> @@ -1268,11 +1268,13 @@ static int nb8800_tangox_init(struct net_device *dev)
>>>>>  		break;
>>>>>
>>>>>  	case PHY_INTERFACE_MODE_RGMII:
>>>>> -		pad_mode = PAD_MODE_RGMII;
>>>>> +	case PHY_INTERFACE_MODE_RGMII_RXID:
>>>>> +		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>>>>>  		break;
>>>>>
>>>>> +	case PHY_INTERFACE_MODE_RGMII_ID:
>>>>>  	case PHY_INTERFACE_MODE_RGMII_TXID:
>>>>> -		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>>>>> +		pad_mode = PAD_MODE_RGMII;
>>>>>  		break;
>>>>
>>>> Won't this just make it break in a different set of circumstances?
>>>
>>> I don't think so, and here's my reasoning:
>>>
>>> AFAIU, the HW block always requires a TX clock delay
>>> (I don't know what the "safe" interval is. PHY adds
>>> 2.4 ns, MAC adds ~1 ns, both work.)
>>
>> The nominal delay should be 2ns because that's exactly what a 90 degrees
>> shift at a 125Mhz would be. The RGMII specification defines the following:
>>
>> TskewT - Data to Clock output Skew (At Transmitter) Min: -500ns, Nom: 0,
>> Max: + 500 ns
>> TskewR - Data to Clock input Skew (At Receiver) Min: 1ns, Nom: 0, Max:
>> 2.6ns (see note 1)
>>
>> note 1: This implies that PC board design will require clocks to be
>> routed such that an additional trace delay of greater than 1.5ns and
>> less than 2.0ns will be added to the associated clock signal. For 10/100
>> the Max value is unspecified.
>>
>> So it seems to me like you are borderline spec in both delays you gave
>> here and the "HW block always requires a TX clock delay" statement is
>> true for a given board design only.
> 
> I must confess that my understanding of clock delays,
> clock skew, routing, traces, etc is nil.
> 
> Is TskewT the TX clock delay?
> And TskewR the RX clock delay?
> 
> Doesn't wire delay factor in too?
> (So longer wires require more delay.)

How about you start reading the RGMII specification so we can at least,
if nothing else agree on the terminology? It's public:

http://web.archive.org/web/20160303171328/http://www.hp.com/rnd/pdfs/RGMIIv2_0_final_hp.pdf

> 
>>> RX clock delay seems to be "Don't Care" (tested both
>>> enabled and disabled by PHY)
>>> By "tested", I mean ability to ping remote system.
>>
>> Can you do something a bit more stressful than just a ping, also if you
>> have the ability to change the inter-packet gap, do it, and see if you
>> start seeing FCS or any other decoding errors.
> 
> Errr... "Inter-packet gap"?
> Is there supposed to be a HW knob to tweak how long
> the HW waits between sending two frames?

Some Ethernet controllers let you change it, some don't, if nb8800
allows it, it's good for testing in that it packs more frames per
quantum of time. If not, do you have at least a FCS error counter?

> 
>>> If phy-mode is RGMII or RGMII_RXID, then don't add
>>> TX clock delay from PHY, therefore add it from MAC.
>>>
>>> If phy_mode is RGMII_ID or RGMII_TXID, then do add
>>> TX clock delay from PHY, therefore don't add it from MAC.
>>>
>>> What set of circumstances would create an issue?
>>
>> Existing Device Tree sources that do not correspond to that description
>> you just did, I suppose they are all out of tree?
> 
> The problem with PHY drivers is that there is no
> simple compatible string to grep for.

That does not help for sure.

> 
> The tango boards use "ethernet-phy-id004d.d072"
> but not a single other DT uses that string.
> For example, am335x-evm.dts doesn't seem to name the PHY.
> Hmmm, how does the at803x probe function match for that
> board?

of_mdiobus_register() scans the Device Tree and for each populated PHY
node, reads its OUI register and and matches the PHY OUI with the driver
list that it has. Quite similar to PCI or USB in fact.

> 
> How does one estimate the impact of driver changes in
> the eth PHY layer?

It's hard, because there are many Ethernet MACs and PHY drivers that are
all inter-operable with each other, which is both a blessing and a curse.

I completely understand what you want to solve but I suspect you will
have to do it in a way where you either accept that you may not be fully
compliant with the now clarified "phy-mode" description, in order not to
break other people's set up that were already non-compliant (can't blame
them, they did not know back then), or you will have to use additional
MAC properties to override the delay settings on the MAC or PHY side.
-- 
Florian

Re: [PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Florian Fainelli]

On 07/19/2017 02:29 PM, Mason wrote:
> On 19/07/2017 21:30, Florian Fainelli wrote:
>> On 07/19/2017 12:24 PM, Grygorii Strashko wrote:
>>> Hi
>>>
>>> On 07/19/2017 10:31 AM, Marc Gonzalez wrote:
>>>> The current code supports enabling RGMII RX and TX clock delays.
>>>> The unstated assumption is that these settings are disabled by
>>>> default at reset, which is not the case.
>>>>
>>>> RX clock delay is enabled at reset. And TX clock delay "survives"
>>>> across SW resets. Thus, if the bootloader enables TX clock delay,
>>>> it will remain enabled at reset in Linux.
>>>>
>>>> Provide disable functions to configure the RGMII clock delays
>>>> exactly as specified in the fwspec.
>>>>
>>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>>> ---
>>>>   drivers/net/phy/at803x.c | 32 ++++++++++++++++++++++++--------
>>>>   1 file changed, 24 insertions(+), 8 deletions(-)
>>> This patch breaks am335x-evm networking.
>>>
>>> To restore it I've had to apply below diff:
>>> diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
>>> index 200d6ab..9578bdf 100644
>>> --- a/arch/arm/boot/dts/am335x-evm.dts
>>> +++ b/arch/arm/boot/dts/am335x-evm.dts
>>> @@ -724,12 +724,12 @@
>>>  
>>>  &cpsw_emac0 {
>>>         phy_id = <&davinci_mdio>, <0>;
>>> -       phy-mode = "rgmii-txid";
>>> +       phy-mode = "rgmii-id";
>>>  };
>>>  
>>>  &cpsw_emac1 {
>>>         phy_id = <&davinci_mdio>, <1>;
>>> -       phy-mode = "rgmii-txid";
>>> +       phy-mode = "rgmii-id";
>>>  };
>>>  
>>>  &tscadc {
>>>
>>> Sry, can't comment here to much - not E-PHY expert.
>>
>> It's useful feedback, since we had poorly defined "phy-mode" semantics
>> for too long, this is totally expected, Marc this is exactly why Mans is
>> suggesting additional MAC-specific properties to define delays.
> 
> In the current situation, it is impossible to configure
> the at803x to disable RX clock delay or TX clock delay
> (in case the boot loader enabled it).
> 
> Are you saying that, because no one has had a problem
> so far, it is not possible to fix it now, as it would
> break boards like am335x-evm.dts which didn't request
> RX clock delay, but got one anyway?

First it means that your patch as-is broke Grygorii's board, and you
need to at least integrate his patch if you plan on having your own
patch accepted. This will fix am335x-evm.dts, but we have no visibility
into the other DTSes out there that may be using an at803x PHY. If you u
break something you need to fix it, and touching how PHY delays are

> 
> Does that mean we cannot support boards using AR8035
> that need the RX and TX clock delays disabled?

No, that is not what that means, it means that you cannot change how an
existing PHY driver with active and existing deployments is interpreting
the phy_interface_t value in a way that it breaks people setups, which
your patch just did. Yes this makes it non-conforming to the revised
definition of "phy-mode", but it is just how it is, people did not know
any better before.

See below for what you could do.

> 
> I'm not sure how the MAC-specific properties can save
> the day?

If you introduced PHY and/or MAC specific properties to configure the
delays in the appropriate unit of time (say ps), you could use a
non-compliant 'phy-mode' just to satisfy the driver/PHY library and
still override the delays you need.
-- 
Florian

[PATCH 1/2] net: phy: at803x: Fix RGMII RX and TX clock delays setup

[Florian Fainelli]

On 07/19/2017 02:29 PM, Mason wrote:
> On 19/07/2017 21:30, Florian Fainelli wrote:
>> On 07/19/2017 12:24 PM, Grygorii Strashko wrote:
>>> Hi
>>>
>>> On 07/19/2017 10:31 AM, Marc Gonzalez wrote:
>>>> The current code supports enabling RGMII RX and TX clock delays.
>>>> The unstated assumption is that these settings are disabled by
>>>> default at reset, which is not the case.
>>>>
>>>> RX clock delay is enabled at reset. And TX clock delay "survives"
>>>> across SW resets. Thus, if the bootloader enables TX clock delay,
>>>> it will remain enabled at reset in Linux.
>>>>
>>>> Provide disable functions to configure the RGMII clock delays
>>>> exactly as specified in the fwspec.
>>>>
>>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>>> ---
>>>>   drivers/net/phy/at803x.c | 32 ++++++++++++++++++++++++--------
>>>>   1 file changed, 24 insertions(+), 8 deletions(-)
>>> This patch breaks am335x-evm networking.
>>>
>>> To restore it I've had to apply below diff:
>>> diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
>>> index 200d6ab..9578bdf 100644
>>> --- a/arch/arm/boot/dts/am335x-evm.dts
>>> +++ b/arch/arm/boot/dts/am335x-evm.dts
>>> @@ -724,12 +724,12 @@
>>>  
>>>  &cpsw_emac0 {
>>>         phy_id = <&davinci_mdio>, <0>;
>>> -       phy-mode = "rgmii-txid";
>>> +       phy-mode = "rgmii-id";
>>>  };
>>>  
>>>  &cpsw_emac1 {
>>>         phy_id = <&davinci_mdio>, <1>;
>>> -       phy-mode = "rgmii-txid";
>>> +       phy-mode = "rgmii-id";
>>>  };
>>>  
>>>  &tscadc {
>>>
>>> Sry, can't comment here to much - not E-PHY expert.
>>
>> It's useful feedback, since we had poorly defined "phy-mode" semantics
>> for too long, this is totally expected, Marc this is exactly why Mans is
>> suggesting additional MAC-specific properties to define delays.
> 
> In the current situation, it is impossible to configure
> the at803x to disable RX clock delay or TX clock delay
> (in case the boot loader enabled it).
> 
> Are you saying that, because no one has had a problem
> so far, it is not possible to fix it now, as it would
> break boards like am335x-evm.dts which didn't request
> RX clock delay, but got one anyway?

First it means that your patch as-is broke Grygorii's board, and you
need to at least integrate his patch if you plan on having your own
patch accepted. This will fix am335x-evm.dts, but we have no visibility
into the other DTSes out there that may be using an at803x PHY. If you u
break something you need to fix it, and touching how PHY delays are

> 
> Does that mean we cannot support boards using AR8035
> that need the RX and TX clock delays disabled?

No, that is not what that means, it means that you cannot change how an
existing PHY driver with active and existing deployments is interpreting
the phy_interface_t value in a way that it breaks people setups, which
your patch just did. Yes this makes it non-conforming to the revised
definition of "phy-mode", but it is just how it is, people did not know
any better before.

See below for what you could do.

> 
> I'm not sure how the MAC-specific properties can save
> the day?

If you introduced PHY and/or MAC specific properties to configure the
delays in the appropriate unit of time (say ps), you could use a
non-compliant 'phy-mode' just to satisfy the driver/PHY library and
still override the delays you need.
-- 
Florian

Re: [PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Mason]

On 19/07/2017 23:34, Florian Fainelli wrote:

> How about you start reading the RGMII specification so we can at least,
> if nothing else agree on the terminology? It's public:
> 
> http://web.archive.org/web/20160303171328/http://www.hp.com/rnd/pdfs/RGMIIv2_0_final_hp.pdf

Thanks for linking the spec. Having no EE training,
I am ill-equipped to interpret the timings table.

As you pointed out, the spec states that the
"Data to Clock input Skew (at Receiver)"
must be within [ 1.0, 2.6 ] ns.

I understand that 2 ns is 1/4 of a 125 MHz period,
but it's not clear to me why the above interval is
centered at 1.8 instead of 2.0 ns.

Also, the AR8035 PHY offers 4 possible TX clock delays:
{ 0.25, 1.3, 2.4, 3.4 } according to their doc.
The two extremes are outside the interval, when would
they be useful? In case the transmitter adds "bad" skew?

Why doesn't the PHY support 1.8/2.0? Is it perhaps
unable to, because of PLL limitations?

It's also not clear to me if wire length has any
influence on the required skew. I would say "no".
I think signal propagation time has nothing to do
with clock skew (as long as both wires are roughly
the same length).

> Some Ethernet controllers let you change it, some don't, if nb8800
> allows it, it's good for testing in that it packs more frames per
> quantum of time. If not, do you have at least a FCS error counter?

I'll have a closer look, and test with iPerf3.
Or is there a better benchmark? I will look for
an inter-packet gap knob and FCS error counter.

> I completely understand what you want to solve but I suspect you will
> have to do it in a way where you either accept that you may not be fully
> compliant with the now clarified "phy-mode" description, in order not to
> break other people's set up that were already non-compliant (can't blame
> them, they did not know back then), or you will have to use additional
> MAC properties to override the delay settings on the MAC or PHY side.

I think I need to give up the notion of "fixing"
the at803x driver. Some boards rely on the fact
that RX clock delay is enabled by default, like
am335x-evm using "rgmii-txid" instead of "rgmii-id".

My board needs to enable both internal delays,
so I don't need the PHY patch. I will only fix
the MAC driver and the DTS.

Regards.

[PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Mason]

On 19/07/2017 23:34, Florian Fainelli wrote:

> How about you start reading the RGMII specification so we can at least,
> if nothing else agree on the terminology? It's public:
> 
> http://web.archive.org/web/20160303171328/http://www.hp.com/rnd/pdfs/RGMIIv2_0_final_hp.pdf

Thanks for linking the spec. Having no EE training,
I am ill-equipped to interpret the timings table.

As you pointed out, the spec states that the
"Data to Clock input Skew (at Receiver)"
must be within [ 1.0, 2.6 ] ns.

I understand that 2 ns is 1/4 of a 125 MHz period,
but it's not clear to me why the above interval is
centered at 1.8 instead of 2.0 ns.

Also, the AR8035 PHY offers 4 possible TX clock delays:
{ 0.25, 1.3, 2.4, 3.4 } according to their doc.
The two extremes are outside the interval, when would
they be useful? In case the transmitter adds "bad" skew?

Why doesn't the PHY support 1.8/2.0? Is it perhaps
unable to, because of PLL limitations?

It's also not clear to me if wire length has any
influence on the required skew. I would say "no".
I think signal propagation time has nothing to do
with clock skew (as long as both wires are roughly
the same length).

> Some Ethernet controllers let you change it, some don't, if nb8800
> allows it, it's good for testing in that it packs more frames per
> quantum of time. If not, do you have at least a FCS error counter?

I'll have a closer look, and test with iPerf3.
Or is there a better benchmark? I will look for
an inter-packet gap knob and FCS error counter.

> I completely understand what you want to solve but I suspect you will
> have to do it in a way where you either accept that you may not be fully
> compliant with the now clarified "phy-mode" description, in order not to
> break other people's set up that were already non-compliant (can't blame
> them, they did not know back then), or you will have to use additional
> MAC properties to override the delay settings on the MAC or PHY side.

I think I need to give up the notion of "fixing"
the at803x driver. Some boards rely on the fact
that RX clock delay is enabled by default, like
am335x-evm using "rgmii-txid" instead of "rgmii-id".

My board needs to enable both internal delays,
so I don't need the PHY patch. I will only fix
the MAC driver and the DTS.

Regards.

Re: [PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> I will look for an inter-packet gap knob and FCS error counter.

There is an FCS error counter.  Use "ethtool -S" and look for
rx_bad_fcs_frames.  Reading the stats counters automatically resets
them to zero.

-- 
Måns Rullgård

[PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Måns Rullgård]

Mason <slash.tmp@free.fr> writes:

> I will look for an inter-packet gap knob and FCS error counter.

There is an FCS error counter.  Use "ethtool -S" and look for
rx_bad_fcs_frames.  Reading the stats counters automatically resets
them to zero.

-- 
M?ns Rullg?rd

Re: [PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Mason]

On 20/07/2017 14:33, Mason wrote:

> As [Florian] pointed out, the spec states that the
> "Data to Clock input Skew (at Receiver)"
> must be within [ 1.0, 2.6 ] ns.
> 
> I understand that 2 ns is 1/4 of a 125 MHz period,
> but it's not clear to me why the above interval is
> centered at 1.8 instead of 2.0 ns.
> 
> Also, the AR8035 PHY offers 4 possible TX clock delays:
> { 0.25, 1.3, 2.4, 3.4 } according to their doc.
> The two extremes are outside the interval, when would
> they be useful? In case the transmitter adds "bad" skew?
> 
> Why doesn't the PHY support 1.8/2.0? Is it perhaps
> unable to, because of PLL limitations?

I haven't yet found answers for these questions.

- Why is the interval centered at 1.8 instead of 2.0 ns?
- What use are 0.25 ns and 3.4 ns skew?
- Why doesn't the PHY support a "recommended" value like 1.8 ns?

Does anyone have pointers to good resources?

Regards.

[PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Mason]

On 20/07/2017 14:33, Mason wrote:

> As [Florian] pointed out, the spec states that the
> "Data to Clock input Skew (at Receiver)"
> must be within [ 1.0, 2.6 ] ns.
> 
> I understand that 2 ns is 1/4 of a 125 MHz period,
> but it's not clear to me why the above interval is
> centered at 1.8 instead of 2.0 ns.
> 
> Also, the AR8035 PHY offers 4 possible TX clock delays:
> { 0.25, 1.3, 2.4, 3.4 } according to their doc.
> The two extremes are outside the interval, when would
> they be useful? In case the transmitter adds "bad" skew?
> 
> Why doesn't the PHY support 1.8/2.0? Is it perhaps
> unable to, because of PLL limitations?

I haven't yet found answers for these questions.

- Why is the interval centered at 1.8 instead of 2.0 ns?
- What use are 0.25 ns and 3.4 ns skew?
- Why doesn't the PHY support a "recommended" value like 1.8 ns?

Does anyone have pointers to good resources?

Regards.

Re: [PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Florian Fainelli]

On 07/24/2017 02:21 PM, Mason wrote:
> On 20/07/2017 14:33, Mason wrote:
> 
>> As [Florian] pointed out, the spec states that the
>> "Data to Clock input Skew (at Receiver)"
>> must be within [ 1.0, 2.6 ] ns.
>>
>> I understand that 2 ns is 1/4 of a 125 MHz period,
>> but it's not clear to me why the above interval is
>> centered at 1.8 instead of 2.0 ns.
>>
>> Also, the AR8035 PHY offers 4 possible TX clock delays:
>> { 0.25, 1.3, 2.4, 3.4 } according to their doc.
>> The two extremes are outside the interval, when would
>> they be useful? In case the transmitter adds "bad" skew?
>>
>> Why doesn't the PHY support 1.8/2.0? Is it perhaps
>> unable to, because of PLL limitations?
> 
> I haven't yet found answers for these questions.
> 
> - Why is the interval centered at 1.8 instead of 2.0 ns?

Presumably because this is almost the middle of the available range and
it still provides a value that is within the specification...

> - What use are 0.25 ns and 3.4 ns skew?

Accounting for extreme PCB traces lengths possibly, or just exposing the
raw values that the HW supports by increments of 0.25 ns, just because
the HW supports it.

> - Why doesn't the PHY support a "recommended" value like 1.8 ns?
> 
> Does anyone have pointers to good resources?

The PHY datasheet and the RGMII specification really ought to be the
starting points, there is not much more to it. Maybe go ask your support
person at Qualcomm/Atheros about their PHY design?
-- 
Florian

[PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Florian Fainelli]

On 07/24/2017 02:21 PM, Mason wrote:
> On 20/07/2017 14:33, Mason wrote:
> 
>> As [Florian] pointed out, the spec states that the
>> "Data to Clock input Skew (at Receiver)"
>> must be within [ 1.0, 2.6 ] ns.
>>
>> I understand that 2 ns is 1/4 of a 125 MHz period,
>> but it's not clear to me why the above interval is
>> centered at 1.8 instead of 2.0 ns.
>>
>> Also, the AR8035 PHY offers 4 possible TX clock delays:
>> { 0.25, 1.3, 2.4, 3.4 } according to their doc.
>> The two extremes are outside the interval, when would
>> they be useful? In case the transmitter adds "bad" skew?
>>
>> Why doesn't the PHY support 1.8/2.0? Is it perhaps
>> unable to, because of PLL limitations?
> 
> I haven't yet found answers for these questions.
> 
> - Why is the interval centered at 1.8 instead of 2.0 ns?

Presumably because this is almost the middle of the available range and
it still provides a value that is within the specification...

> - What use are 0.25 ns and 3.4 ns skew?

Accounting for extreme PCB traces lengths possibly, or just exposing the
raw values that the HW supports by increments of 0.25 ns, just because
the HW supports it.

> - Why doesn't the PHY support a "recommended" value like 1.8 ns?
> 
> Does anyone have pointers to good resources?

The PHY datasheet and the RGMII specification really ought to be the
starting points, there is not much more to it. Maybe go ask your support
person at Qualcomm/Atheros about their PHY design?
-- 
Florian

Re: [PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Mason]

On 24/07/2017 23:49, Florian Fainelli wrote:
> On 07/24/2017 02:21 PM, Mason wrote:
>> On 20/07/2017 14:33, Mason wrote:
>>
>>> As [Florian] pointed out, the spec states that the
>>> "Data to Clock input Skew (at Receiver)"
>>> must be within [ 1.0, 2.6 ] ns.
>>>
>>> I understand that 2 ns is 1/4 of a 125 MHz period,
>>> but it's not clear to me why the above interval is
>>> centered at 1.8 instead of 2.0 ns.
>>>
>>> Also, the AR8035 PHY offers 4 possible TX clock delays:
>>> { 0.25, 1.3, 2.4, 3.4 } according to their doc.
>>> The two extremes are outside the interval, when would
>>> they be useful? In case the transmitter adds "bad" skew?
>>>
>>> Why doesn't the PHY support 1.8/2.0? Is it perhaps
>>> unable to, because of PLL limitations?
>>
>> I haven't yet found answers for these questions.
>>
>> - Why is the interval centered at 1.8 instead of 2.0 ns?
> 
> Presumably because this is almost the middle of the available range and
> it still provides a value that is within the specification...

I was talking about the RGMII spec.
If - theoretically - the best results are achieved
by having a 2 ns skew between clock and data,
it seems odd for the RGMII spec to define an
interval of [ 1.0, 2.6 ] ns for acceptable values.
I would have expected [ 1.2, 2.8 ] ns.

>> - What use are 0.25 ns and 3.4 ns skew?
> 
> Accounting for extreme PCB traces lengths possibly, or just exposing the
> raw values that the HW supports by increments of 0.25 ns, just because
> the HW supports it.

The AR8035 doesn't support increments of 0.25 ns,
it supports just 4 values: 0.25, 1.3, 2.4, 3.4
Two of which are outside the acceptable range
defined in the RGMII spec. Odd.
Giving it more thought, I don't think trace length
factors in, unless the data and clock lines have
very different length (signal propagation).

>> - Why doesn't the PHY support a "recommended" value like 1.8 ns?
>>
>> Does anyone have pointers to good resources?
> 
> The PHY datasheet and the RGMII specification really ought to be the
> starting points, there is not much more to it. Maybe go ask your support
> person at Qualcomm/Atheros about their PHY design?

Sadly, I rarely have access to support for the blocks
we use. I had to download the datasheet off the internet.
But I was only asking out of personal curiosity, since
this is outside my field. I don't think any customer
has complained about the default settings.

Regards.

[PATCH 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Mason]

On 24/07/2017 23:49, Florian Fainelli wrote:
> On 07/24/2017 02:21 PM, Mason wrote:
>> On 20/07/2017 14:33, Mason wrote:
>>
>>> As [Florian] pointed out, the spec states that the
>>> "Data to Clock input Skew (at Receiver)"
>>> must be within [ 1.0, 2.6 ] ns.
>>>
>>> I understand that 2 ns is 1/4 of a 125 MHz period,
>>> but it's not clear to me why the above interval is
>>> centered at 1.8 instead of 2.0 ns.
>>>
>>> Also, the AR8035 PHY offers 4 possible TX clock delays:
>>> { 0.25, 1.3, 2.4, 3.4 } according to their doc.
>>> The two extremes are outside the interval, when would
>>> they be useful? In case the transmitter adds "bad" skew?
>>>
>>> Why doesn't the PHY support 1.8/2.0? Is it perhaps
>>> unable to, because of PLL limitations?
>>
>> I haven't yet found answers for these questions.
>>
>> - Why is the interval centered at 1.8 instead of 2.0 ns?
> 
> Presumably because this is almost the middle of the available range and
> it still provides a value that is within the specification...

I was talking about the RGMII spec.
If - theoretically - the best results are achieved
by having a 2 ns skew between clock and data,
it seems odd for the RGMII spec to define an
interval of [ 1.0, 2.6 ] ns for acceptable values.
I would have expected [ 1.2, 2.8 ] ns.

>> - What use are 0.25 ns and 3.4 ns skew?
> 
> Accounting for extreme PCB traces lengths possibly, or just exposing the
> raw values that the HW supports by increments of 0.25 ns, just because
> the HW supports it.

The AR8035 doesn't support increments of 0.25 ns,
it supports just 4 values: 0.25, 1.3, 2.4, 3.4
Two of which are outside the acceptable range
defined in the RGMII spec. Odd.
Giving it more thought, I don't think trace length
factors in, unless the data and clock lines have
very different length (signal propagation).

>> - Why doesn't the PHY support a "recommended" value like 1.8 ns?
>>
>> Does anyone have pointers to good resources?
> 
> The PHY datasheet and the RGMII specification really ought to be the
> starting points, there is not much more to it. Maybe go ask your support
> person at Qualcomm/Atheros about their PHY design?

Sadly, I rarely have access to support for the blocks
we use. I had to download the datasheet off the internet.
But I was only asking out of personal curiosity, since
this is outside my field. I don't think any customer
has complained about the default settings.

Regards.