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.

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

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.

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

[Måns Rullgård]

Mason  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 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

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

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 
 ---
  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 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 
>>> ---
>>>  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 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 
>> ---
>>  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 2/2] net: ethernet: nb8800: Fix RGMII TX clock delay setup

[Måns Rullgård]

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

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