[PATCH v2 0/4] RGMII RX and TX clock delays using Atheros 8035

[PATCH v2 0/4] RGMII RX and TX clock delays using Atheros 8035

[Marc Gonzalez]

Changes from v1
- Drop support for disabling RX and TX clock delays
(it breaks some boards). Document the issues instead.
- Split the MAC patch in two unrelated parts
- Fix the vantage 1172 DTS

Marc Gonzalez (4):
  net: phy: at803x: Document RGMII RX and TX clock delay issues
  net: ethernet: nb8800: Set RGMII_MODE for all RGMII modes
  net: ethernet: nb8800: Fix RGMII TX clock delay setup
  ARM: dts: tango4: Add RGMII RX and TX clock delays

 arch/arm/boot/dts/tango4-vantage-1172.dts |  2 +-
 drivers/net/ethernet/aurora/nb8800.c      |  8 +++++---
 drivers/net/phy/at803x.c                  | 12 ++++++++++++
 3 files changed, 18 insertions(+), 4 deletions(-)

-- 
2.11.0

[PATCH v2 1/4] net: phy: at803x: Document RGMII RX and TX clock delay issues

[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.

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

diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index c1e52b9dc58d..7a0954513b91 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -283,6 +283,12 @@ static int at803x_config_init(struct phy_device *phydev)
 	if (ret < 0)
 		return ret;
 
+	/*
+	 * NB: This code assumes that RGMII RX clock delay is disabled
+	 * at reset, but actually, RX clock delay is enabled@reset.
+	 * Disabling the delay if it has not been explicitly requested
+	 * breaks boards that rely on the enabled-by-default behavior.
+	 */
 	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
 			phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
 		ret = at803x_enable_rx_delay(phydev);
@@ -290,6 +296,12 @@ static int at803x_config_init(struct phy_device *phydev)
 			return ret;
 	}
 
+	/*
+	 * NB: This code assumes that RGMII TX clock delay is disabled
+	 * at reset, but actually, TX clock delay "survives" across SW
+	 * resets. If the bootloader enables TX clock delay, Linux is
+	 * stuck with that setting.
+	 */
 	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
 			phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) {
 		ret = at803x_enable_tx_delay(phydev);
-- 
2.11.0

[PATCH v2 1/4] net: phy: at803x: Document RGMII RX and TX clock delay issues

[Timur Tabi]

On 7/21/17 6:25 AM, Marc Gonzalez wrote:
> +	 * NB: This code assumes that RGMII RX clock delay is disabled
> +	 * at reset, but actually, RX clock delay is enabled at reset.

Could we change this to say, "RX clock delay is enabled at reset on some 
systems."?  Otherwise, it implies that the code is wrong 100% of the 
time and should be fixed, not documented.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

[PATCH v2 1/4] net: phy: at803x: Document RGMII RX and TX clock delay issues

[Marc Gonzalez]

On 21/07/2017 15:20, Timur Tabi wrote:

> On 7/21/17 6:25 AM, Marc Gonzalez wrote:
>
>> +	 * NB: This code assumes that RGMII RX clock delay is disabled
>> +	 * at reset, but actually, RX clock delay is enabled at reset.
> 
> Could we change this to say, "RX clock delay is enabled at reset on some 
> systems."?  Otherwise, it implies that the code is wrong 100% of the 
> time and should be fixed, not documented.

I don't understand what you're saying.

It is a correct observation that the code enabling
RGMII RX clock delay is a NOP, since that bit will
always be set at that point.

The spec for the 8035 (I haven't checked for 8030 and 8031,
is that what you meant by "other systems"?) states that
Sel_clk125m_dsp, which is described as:
"Control bit for rgmii interface rx clock delay"
is 1 after HW reset, 1 after SW reset.

So my statement "RX clock delay is enabled at reset"
is universally true. It's not just on some systems.

What am I missing?

Regards.

[PATCH v2 1/4] net: phy: at803x: Document RGMII RX and TX clock delay issues

[Timur Tabi]

On 7/21/17 8:29 AM, Marc Gonzalez wrote:
> I don't understand what you're saying.
> 
> It is a correct observation that the code enabling
> RGMII RX clock delay is a NOP, since that bit will
> always be set at that point.
> 
> The spec for the 8035 (I haven't checked for 8030 and 8031,
> is that what you meant by "other systems"?) states that
> Sel_clk125m_dsp, which is described as:
> "Control bit for rgmii interface rx clock delay"
> is 1 after HW reset, 1 after SW reset.
> 
> So my statement "RX clock delay is enabled at reset"
> is universally true. It's not just on some systems.

Ok, taken out of context, the comment doesn't really explain why the 
code is the way it is.  I'm not really happy about the word "assumes". 
Maybe you should add a sentence explaining when the code is NOT a no-op.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

[PATCH v2 1/4] net: phy: at803x: Document RGMII RX and TX clock delay issues

[Marc Gonzalez]

On 21/07/2017 16:06, Timur Tabi wrote:

> On 7/21/17 8:29 AM, Marc Gonzalez wrote:
>
>> I don't understand what you're saying.
>>
>> It is a correct observation that the code enabling
>> RGMII RX clock delay is a NOP, since that bit will
>> always be set at that point.
>>
>> The spec for the 8035 (I haven't checked for 8030 and 8031,
>> is that what you meant by "other systems"?) states that
>> Sel_clk125m_dsp, which is described as:
>> "Control bit for rgmii interface rx clock delay"
>> is 1 after HW reset, 1 after SW reset.
>>
>> So my statement "RX clock delay is enabled at reset"
>> is universally true. It's not just on some systems.
> 
> Ok, taken out of context, the comment doesn't really explain why the 
> code is the way it is.  I'm not really happy about the word "assumes". 

If a HW setting defaults to 0 at reset, and some init is called
right after reset, then you know the setting's value is 0.
If you need that value to be 1, all you need is a function
setting it to 1. This is the current situation.

Commit 2e5f9f281ee8 assumes 0 at reset, and provides a function
setting the value to 1.

Reality is different. The HW setting defaults to 1 at reset.
So it turns out that the function setting the value to 1
is pointless, because the value is already 1. There is
however no way to set the value to 0.

Does that explain why I wrote "assume"?

Also the commit message:
"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."

> Maybe you should add a sentence explaining when the code is NOT a no-op.

The code is *NEVER* NOT a no-op.
I.e. the code enabling RX clock delay is ALWAYS a no-op.
I don't understand what you think is unclear in my comment.

Regards.

[PATCH v2 2/4] net: ethernet: nb8800: Set RGMII_MODE for all RGMII modes

[Marc Gonzalez]

There are 4 RGMII phy-modes to handle.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 drivers/net/ethernet/aurora/nb8800.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index 041cfb7952f8..ded041dbafe7 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;
-- 
2.11.0

[PATCH v2 2/4] net: ethernet: nb8800: Set RGMII_MODE for all RGMII modes

[Måns Rullgård]

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> There are 4 RGMII phy-modes to handle.
>
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>

Acked-by: Mans Rullgard <mans@mansr.com>

> ---
>  drivers/net/ethernet/aurora/nb8800.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
> index 041cfb7952f8..ded041dbafe7 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;
> -- 
> 2.11.0
>

-- 
M?ns Rullg?rd

[PATCH v2 2/4] net: ethernet: nb8800: Set RGMII_MODE for all RGMII modes

[Florian Fainelli]

On 07/21/2017 04:25 AM, Marc Gonzalez wrote:
> There are 4 RGMII phy-modes to handle.

... so make sure that the MAC is configured to set the RGMII_MODE
accordingly for all 4 RGMII mode.

> 
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>

Fixes: 52dfc8301248 ("net: ethernet: add driver for Aurora VLSI NB8800
Ethernet controller")

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/ethernet/aurora/nb8800.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
> index 041cfb7952f8..ded041dbafe7 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;
> 


-- 
Florian

[PATCH v2 3/4] 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 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)

Add TX delay in the MAC only for rgmii and rgmii-rxid.

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

diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index ded041dbafe7..f3ed320eb4ad 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -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 v2 3/4] 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 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)
>
> Add TX delay in the MAC only for rgmii and rgmii-rxid.
>
> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
> ---
>  drivers/net/ethernet/aurora/nb8800.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
> index ded041dbafe7..f3ed320eb4ad 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -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:

I still don't like this.  Having both the MAC and PHY drivers react to
the phy-connection-type property is bound to cause trouble somewhere.

The only way out of the current mess is to define new properties for
both MAC and PHY that override the existing ones if present.

-- 
M?ns Rullg?rd

[PATCH v2 3/4] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Marc Gonzalez]

On 21/07/2017 15:04, M?ns Rullg?rd wrote:

> Marc Gonzalez wrote:
> 
>> According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
>> ("Documentation: devicetree: clarify usage of the RGMII phy-modes")
>> there are 4 RGMII 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)
>>
>> Add TX delay in the MAC only for rgmii and rgmii-rxid.
>>
>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>> ---
>>  drivers/net/ethernet/aurora/nb8800.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>> index ded041dbafe7..f3ed320eb4ad 100644
>> --- a/drivers/net/ethernet/aurora/nb8800.c
>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>> @@ -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:
> 
> I still don't like this.  Having both the MAC and PHY drivers react to
> the phy-connection-type property is bound to cause trouble somewhere.
> 
> The only way out of the current mess is to define new properties for
> both MAC and PHY that override the existing ones if present.

Do you mean defining 4 new bindings and their corresponding
phy_interface_t enum values? For example:

"rgmii-v2"
"rgmii-id-v2"
"rgmii-rxid-v2"
"rgmii-txid-v2"

	PHY_INTERFACE_MODE_RGMII_V2,
	PHY_INTERFACE_MODE_RGMII_ID_V2,
	PHY_INTERFACE_MODE_RGMII_RXID_V2,
	PHY_INTERFACE_MODE_RGMII_TXID_V2,

And then handling these new enums in the at803x and nb8800 drivers?

FWIW, PAD_MODE_GTX_CLK_DELAY is broken in tango5 (doesn't add any
delay). I'm considering removing MAC-side TX delay altogether.

Regards.

[PATCH v2 3/4] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Måns Rullgård]

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> On 21/07/2017 15:04, M?ns Rullg?rd wrote:
>
>> Marc Gonzalez wrote:
>> 
>>> According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
>>> ("Documentation: devicetree: clarify usage of the RGMII phy-modes")
>>> there are 4 RGMII 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)
>>>
>>> Add TX delay in the MAC only for rgmii and rgmii-rxid.
>>>
>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>> ---
>>>  drivers/net/ethernet/aurora/nb8800.c | 6 ++++--
>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>>> index ded041dbafe7..f3ed320eb4ad 100644
>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>> @@ -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:
>> 
>> I still don't like this.  Having both the MAC and PHY drivers react to
>> the phy-connection-type property is bound to cause trouble somewhere.
>> 
>> The only way out of the current mess is to define new properties for
>> both MAC and PHY that override the existing ones if present.
>
> Do you mean defining 4 new bindings and their corresponding
> phy_interface_t enum values? For example:
>
> "rgmii-v2"
> "rgmii-id-v2"
> "rgmii-rxid-v2"
> "rgmii-txid-v2"
>
> 	PHY_INTERFACE_MODE_RGMII_V2,
> 	PHY_INTERFACE_MODE_RGMII_ID_V2,
> 	PHY_INTERFACE_MODE_RGMII_RXID_V2,
> 	PHY_INTERFACE_MODE_RGMII_TXID_V2,
>
> And then handling these new enums in the at803x and nb8800 drivers?

It has already been suggested to add new properties specifying desired
delays in picoseconds.  If present on the MAC node, the MAC driver
should attempt to provide the delay, and if present on the PHY node, the
PHY driver is responsible.

-- 
M?ns Rullg?rd

[PATCH v2 3/4] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Marc Gonzalez]

On 21/07/2017 15:47, M?ns Rullg?rd wrote:

> Marc Gonzalez wrote:
> 
>> On 21/07/2017 15:04, M?ns Rullg?rd wrote:
>>
>>> Marc Gonzalez wrote:
>>>
>>>> According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
>>>> ("Documentation: devicetree: clarify usage of the RGMII phy-modes")
>>>> there are 4 RGMII 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)
>>>>
>>>> Add TX delay in the MAC only for rgmii and rgmii-rxid.
>>>>
>>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>>> ---
>>>>  drivers/net/ethernet/aurora/nb8800.c | 6 ++++--
>>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>>>> index ded041dbafe7..f3ed320eb4ad 100644
>>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>>> @@ -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:
>>>
>>> I still don't like this.  Having both the MAC and PHY drivers react to
>>> the phy-connection-type property is bound to cause trouble somewhere.
>>>
>>> The only way out of the current mess is to define new properties for
>>> both MAC and PHY that override the existing ones if present.
>>
>> Do you mean defining 4 new bindings and their corresponding
>> phy_interface_t enum values? For example:
>>
>> "rgmii-v2"
>> "rgmii-id-v2"
>> "rgmii-rxid-v2"
>> "rgmii-txid-v2"
>>
>> 	PHY_INTERFACE_MODE_RGMII_V2,
>> 	PHY_INTERFACE_MODE_RGMII_ID_V2,
>> 	PHY_INTERFACE_MODE_RGMII_RXID_V2,
>> 	PHY_INTERFACE_MODE_RGMII_TXID_V2,
>>
>> And then handling these new enums in the at803x and nb8800 drivers?
> 
> It has already been suggested to add new properties specifying desired
> delays in picoseconds.  If present on the MAC node, the MAC driver
> should attempt to provide the delay, and if present on the PHY node, the
> PHY driver is responsible.

Sorry, I had already forgotten about Florian's suggestion:
> 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.

So we would need two properties (RX and TX).
"rgmii_rx_skew_ps" and "rgmii_tx_skew_ps"

but it's not clear to me how the MAC probe function communicates
the arguments to the phy driver. Looks like we would need to add
two fields to struct phy_device, and maybe define a new phy-mode
to instruct the PHY driver to look for the two fields.

I don't have time to work on that for now, but I do need to
fix the nb8800 driver now. Can we apply the following patch
in the interim?

diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
index ded041dbafe7..e94159507847 100644
--- a/drivers/net/ethernet/aurora/nb8800.c
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -1268,11 +1268,10 @@ static int nb8800_tangox_init(struct net_device *dev)
                break;
 
        case PHY_INTERFACE_MODE_RGMII:
-               pad_mode = PAD_MODE_RGMII;
-               break;
-
+       case PHY_INTERFACE_MODE_RGMII_ID:
+       case PHY_INTERFACE_MODE_RGMII_RXID:
        case PHY_INTERFACE_MODE_RGMII_TXID:
-               pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+               pad_mode = PAD_MODE_RGMII;
                break;
 
        default:

Regards.

[PATCH v2 3/4] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Måns Rullgård]

Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:

> On 21/07/2017 15:47, M?ns Rullg?rd wrote:
>
>> Marc Gonzalez wrote:
>> 
>>> On 21/07/2017 15:04, M?ns Rullg?rd wrote:
>>>
>>>> Marc Gonzalez wrote:
>>>>
>>>>> According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
>>>>> ("Documentation: devicetree: clarify usage of the RGMII phy-modes")
>>>>> there are 4 RGMII 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)
>>>>>
>>>>> Add TX delay in the MAC only for rgmii and rgmii-rxid.
>>>>>
>>>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>>>> ---
>>>>>  drivers/net/ethernet/aurora/nb8800.c | 6 ++++--
>>>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>>>>> index ded041dbafe7..f3ed320eb4ad 100644
>>>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>>>> @@ -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:
>>>>
>>>> I still don't like this.  Having both the MAC and PHY drivers react to
>>>> the phy-connection-type property is bound to cause trouble somewhere.
>>>>
>>>> The only way out of the current mess is to define new properties for
>>>> both MAC and PHY that override the existing ones if present.
>>>
>>> Do you mean defining 4 new bindings and their corresponding
>>> phy_interface_t enum values? For example:
>>>
>>> "rgmii-v2"
>>> "rgmii-id-v2"
>>> "rgmii-rxid-v2"
>>> "rgmii-txid-v2"
>>>
>>> 	PHY_INTERFACE_MODE_RGMII_V2,
>>> 	PHY_INTERFACE_MODE_RGMII_ID_V2,
>>> 	PHY_INTERFACE_MODE_RGMII_RXID_V2,
>>> 	PHY_INTERFACE_MODE_RGMII_TXID_V2,
>>>
>>> And then handling these new enums in the at803x and nb8800 drivers?
>> 
>> It has already been suggested to add new properties specifying desired
>> delays in picoseconds.  If present on the MAC node, the MAC driver
>> should attempt to provide the delay, and if present on the PHY node, the
>> PHY driver is responsible.
>
> Sorry, I had already forgotten about Florian's suggestion:
>> 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.
>
> So we would need two properties (RX and TX).
> "rgmii_rx_skew_ps" and "rgmii_tx_skew_ps"
>
> but it's not clear to me how the MAC probe function communicates
> the arguments to the phy driver. Looks like we would need to add
> two fields to struct phy_device, and maybe define a new phy-mode
> to instruct the PHY driver to look for the two fields.

There's no need for the drivers to communicate.  The location of the
properties in the device tree determines which driver should deal with
it.

> I don't have time to work on that for now, but I do need to
> fix the nb8800 driver now. Can we apply the following patch
> in the interim?
>
> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
> index ded041dbafe7..e94159507847 100644
> --- a/drivers/net/ethernet/aurora/nb8800.c
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -1268,11 +1268,10 @@ static int nb8800_tangox_init(struct net_device *dev)
>                 break;
>
>         case PHY_INTERFACE_MODE_RGMII:
> -               pad_mode = PAD_MODE_RGMII;
> -               break;
> -
> +       case PHY_INTERFACE_MODE_RGMII_ID:
> +       case PHY_INTERFACE_MODE_RGMII_RXID:
>         case PHY_INTERFACE_MODE_RGMII_TXID:
> -               pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
> +               pad_mode = PAD_MODE_RGMII;
>                 break;
>
>         default:

Simply stop reacting to the delay aspect of the phy-connection-type
property?  Yes, I'm fine with that.

-- 
M?ns Rullg?rd

[PATCH v2 3/4] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Florian Fainelli]

On 07/21/2017 07:24 AM, M?ns Rullg?rd wrote:
> Marc Gonzalez <marc_gonzalez@sigmadesigns.com> writes:
> 
>> On 21/07/2017 15:47, M?ns Rullg?rd wrote:
>>
>>> Marc Gonzalez wrote:
>>>
>>>> On 21/07/2017 15:04, M?ns Rullg?rd wrote:
>>>>
>>>>> Marc Gonzalez wrote:
>>>>>
>>>>>> According to commit e5f3a4a56ce2a707b2fb8ce37e4414dcac89c672
>>>>>> ("Documentation: devicetree: clarify usage of the RGMII phy-modes")
>>>>>> there are 4 RGMII 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)
>>>>>>
>>>>>> Add TX delay in the MAC only for rgmii and rgmii-rxid.
>>>>>>
>>>>>> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
>>>>>> ---
>>>>>>  drivers/net/ethernet/aurora/nb8800.c | 6 ++++--
>>>>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>>>>>> index ded041dbafe7..f3ed320eb4ad 100644
>>>>>> --- a/drivers/net/ethernet/aurora/nb8800.c
>>>>>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>>>>>> @@ -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:
>>>>>
>>>>> I still don't like this.  Having both the MAC and PHY drivers react to
>>>>> the phy-connection-type property is bound to cause trouble somewhere.
>>>>>
>>>>> The only way out of the current mess is to define new properties for
>>>>> both MAC and PHY that override the existing ones if present.
>>>>
>>>> Do you mean defining 4 new bindings and their corresponding
>>>> phy_interface_t enum values? For example:
>>>>
>>>> "rgmii-v2"
>>>> "rgmii-id-v2"
>>>> "rgmii-rxid-v2"
>>>> "rgmii-txid-v2"
>>>>
>>>> 	PHY_INTERFACE_MODE_RGMII_V2,
>>>> 	PHY_INTERFACE_MODE_RGMII_ID_V2,
>>>> 	PHY_INTERFACE_MODE_RGMII_RXID_V2,
>>>> 	PHY_INTERFACE_MODE_RGMII_TXID_V2,
>>>>
>>>> And then handling these new enums in the at803x and nb8800 drivers?
>>>
>>> It has already been suggested to add new properties specifying desired
>>> delays in picoseconds.  If present on the MAC node, the MAC driver
>>> should attempt to provide the delay, and if present on the PHY node, the
>>> PHY driver is responsible.
>>
>> Sorry, I had already forgotten about Florian's suggestion:
>>> 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.
>>
>> So we would need two properties (RX and TX).
>> "rgmii_rx_skew_ps" and "rgmii_tx_skew_ps"
>>
>> but it's not clear to me how the MAC probe function communicates
>> the arguments to the phy driver. Looks like we would need to add
>> two fields to struct phy_device, and maybe define a new phy-mode
>> to instruct the PHY driver to look for the two fields.
> 
> There's no need for the drivers to communicate.  The location of the
> properties in the device tree determines which driver should deal with
> it.

Exactly. I really like how the PHY delay properties are defined in
Documentation/devicetree/bindings/net/micrel-ksz90x1.txt because they
are quite generic and for a MAC counterpart those defined in
Documentation/devicetree/bindings/net/dwmac-sun8i.txt and
Documentation/devicetree/bindings/net/meson-dwmac.txt are also good
examples.

> 
>> I don't have time to work on that for now, but I do need to
>> fix the nb8800 driver now. Can we apply the following patch
>> in the interim?
>>
>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>> index ded041dbafe7..e94159507847 100644
>> --- a/drivers/net/ethernet/aurora/nb8800.c
>> +++ b/drivers/net/ethernet/aurora/nb8800.c
>> @@ -1268,11 +1268,10 @@ static int nb8800_tangox_init(struct net_device *dev)
>>                 break;
>>
>>         case PHY_INTERFACE_MODE_RGMII:
>> -               pad_mode = PAD_MODE_RGMII;
>> -               break;
>> -
>> +       case PHY_INTERFACE_MODE_RGMII_ID:
>> +       case PHY_INTERFACE_MODE_RGMII_RXID:
>>         case PHY_INTERFACE_MODE_RGMII_TXID:
>> -               pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
>> +               pad_mode = PAD_MODE_RGMII;
>>                 break;
>>
>>         default:
> 
> Simply stop reacting to the delay aspect of the phy-connection-type
> property?  Yes, I'm fine with that.
> 


-- 
Florian

[PATCH v2 4/4] ARM: dts: tango4: Add RGMII RX and TX clock delays

[Marc Gonzalez]

RX and TX clock delays are required.

Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com>
---
 arch/arm/boot/dts/tango4-vantage-1172.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tango4-vantage-1172.dts b/arch/arm/boot/dts/tango4-vantage-1172.dts
index 86d8df98802f..13bcc460bcb2 100644
--- a/arch/arm/boot/dts/tango4-vantage-1172.dts
+++ b/arch/arm/boot/dts/tango4-vantage-1172.dts
@@ -22,7 +22,7 @@
 };
 
 &eth0 {
-	phy-connection-type = "rgmii";
+	phy-connection-type = "rgmii-id";
 	phy-handle = <&eth0_phy>;
 	#address-cells = <1>;
 	#size-cells = <0>;
-- 
2.11.0

[PATCH v2 0/4] RGMII RX and TX clock delays using Atheros 8035

[Marc Gonzalez]

On 21/07/2017 13:22, Marc Gonzalez wrote:

> Changes from v1
> - Drop support for disabling RX and TX clock delays
> (it breaks some boards). Document the issues instead.
> - Split the MAC patch in two unrelated parts
> - Fix the vantage 1172 DTS
> 
> Marc Gonzalez (4):
>   net: phy: at803x: Document RGMII RX and TX clock delay issues
>   net: ethernet: nb8800: Set RGMII_MODE for all RGMII modes
>   net: ethernet: nb8800: Fix RGMII TX clock delay setup
>   ARM: dts: tango4: Add RGMII RX and TX clock delays
> 
>  arch/arm/boot/dts/tango4-vantage-1172.dts |  2 +-
>  drivers/net/ethernet/aurora/nb8800.c      |  8 +++++---
>  drivers/net/phy/at803x.c                  | 12 ++++++++++++
>  3 files changed, 18 insertions(+), 4 deletions(-)

Rudimentary test:

ip addr add 172.27.64.45/18 brd 172.27.127.255 dev eth0
ip link set eth0 up
sleep 10 ## autoneg should be complete
time fping -c 10000 -b 1450 -p 1 -i 1 -q 172.27.64.1
ethtool -S eth0
cat /proc/interrupts

# test_eth.sh 
[   13.276242] ENTER at803x_enable_rx_delay
[   13.280283] BEFORE=82ee
[   13.282735]  AFTER=82ee
[   13.285462] ENTER at803x_enable_tx_delay
[   13.289567] BEFORE=2d47
[   13.292018]  AFTER=2d47
[   16.663266] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
172.27.64.1 : xmt/rcv/%loss = 10000/10000/0%, min/avg/max = 0.18/0.21/3.79

real    0m10.688s
user    0m0.106s
sys     0m0.472s

NIC statistics:
     rx_bytes_ok: 14960128
     rx_frames_ok: 10002
     rx_undersize_frames: 0
     rx_fragment_frames: 0
     rx_64_byte_frames: 2
     rx_127_byte_frames: 0
     rx_255_byte_frames: 0
     rx_511_byte_frames: 0
     rx_1023_byte_frames: 0
     rx_max_size_frames: 10000
     rx_oversize_frames: 0
     rx_bad_fcs_frames: 0
     rx_broadcast_frames: 0
     rx_multicast_frames: 0
     rx_control_frames: 0
     rx_pause_frames: 0
     rx_unsup_control_frames: 0
     rx_align_error_frames: 0
     rx_overrun_frames: 0
     rx_jabber_frames: 0
     rx_bytes: 14960128
     rx_frames: 10002
     tx_bytes_ok: 14960128
     tx_frames_ok: 10002
     tx_64_byte_frames: 2
     tx_127_byte_frames: 0
     tx_255_byte_frames: 0
     tx_511_byte_frames: 0
     tx_1023_byte_frames: 0
     tx_max_size_frames: 10000
     tx_oversize_frames: 0
     tx_broadcast_frames: 1
     tx_multicast_frames: 0
     tx_control_frames: 0
     tx_pause_frames: 0
     tx_underrun_frames: 0
     tx_single_collision_frames: 0
     tx_multi_collision_frames: 0
     tx_deferred_collision_frames: 0
     tx_late_collision_frames: 0
     tx_excessive_collision_frames: 0
     tx_bytes: 14960128
     tx_frames: 10002
     tx_collisions: 0

            CPU0       CPU1       
  19:      10825        983     GIC-0  29 Edge      twd
  20:         85          0      irq0   1 Level     ttyS0
  21:          0          0      irq0  60 Level     mmc0
  22:        228          0      irq0   8 Level     mmc1
  25:      20005          0      irq0  38 Level     eth0
  28:          1          0      irq0  37 Edge      26000.nb8800-mii:04
IPI0:          0          0  CPU wakeup interrupts
IPI1:          0          0  Timer broadcast interrupts
IPI2:       1183       1725  Rescheduling interrupts
IPI3:          0          1  Function call interrupts
IPI4:          0          0  CPU stop interrupts
IPI5:          1          0  IRQ work interrupts
IPI6:          0          0  completion interrupts
Err:           0


fping caps at 1000 packets per second, which limits
its usefulness as a benchmarking tool.

Regards.

[PATCH v2 0/4] RGMII RX and TX clock delays using Atheros 8035

[Marc Gonzalez]

On 21/07/2017 14:47, Marc Gonzalez wrote:

> fping caps at 1000 packets per second, which limits
> its usefulness as a benchmarking tool.

"Normal" ping is slightly faster (5000 packets per second).

time ping -f -c 60000 -s 1450 -q 172.27.64.1

--- 172.27.64.1 ping statistics ---
60000 packets transmitted, 60000 received, 0% packet loss, time 12059ms
rtt min/avg/max/mdev = 0.159/0.167/3.536/0.021 ms, ipg/ewma 0.201/0.166 ms

real    0m12.067s
user    0m0.899s
sys     0m1.886s

NIC statistics:
     rx_bytes_ok: 89760375
     rx_frames_ok: 60003
     rx_undersize_frames: 0
     rx_fragment_frames: 0
     rx_64_byte_frames: 2
     rx_127_byte_frames: 0
     rx_255_byte_frames: 1
     rx_511_byte_frames: 0
     rx_1023_byte_frames: 0
     rx_max_size_frames: 60000
     rx_oversize_frames: 0
     rx_bad_fcs_frames: 0
     rx_broadcast_frames: 1
     rx_multicast_frames: 0
     rx_control_frames: 0
     rx_pause_frames: 0
     rx_unsup_control_frames: 0
     rx_align_error_frames: 0
     rx_overrun_frames: 0
     rx_jabber_frames: 0
     rx_bytes: 89760375
     rx_frames: 60003
     tx_bytes_ok: 89760128
     tx_frames_ok: 60002
     tx_64_byte_frames: 2
     tx_127_byte_frames: 0
     tx_255_byte_frames: 0
     tx_511_byte_frames: 0
     tx_1023_byte_frames: 0
     tx_max_size_frames: 60000
     tx_oversize_frames: 0
     tx_broadcast_frames: 1
     tx_multicast_frames: 0
     tx_control_frames: 0
     tx_pause_frames: 0
     tx_underrun_frames: 0
     tx_single_collision_frames: 0
     tx_multi_collision_frames: 0
     tx_deferred_collision_frames: 0
     tx_late_collision_frames: 0
     tx_excessive_collision_frames: 0
     tx_bytes: 89760128
     tx_frames: 60002
     tx_collisions: 0