correct use of PHY_INTERFACE_MODE_RGMII{,_TXID,_RXID,_ID}

correct use of PHY_INTERFACE_MODE_RGMII{,_TXID,_RXID,_ID}

[Helmut Grohne]

Hi,

I've been trying to write a dt for a board and got quite confused about
the RGMII delays. That's why I looked into it and got even more confused
by what I found. Different drivers handle this quite differently. Let me
summarize.

Some drivers handle the RGMII modes individually. This is how I expected
it to be. Examples:
* renesas/ravb_main.c
* stmicro/stmmac/dwmac-rk.c

A number of drivers handle all RGMII modes in uniformly. They don't
actually configure any dealys. Is that supposed to work?  Examples:
* apm/xgene/xgene_enet_main.c
* aurora/nb8800.c
* cadence/macb_main.c
* freescale/fman/fman_memac.c
* freescale/ucc_geth.c
* ibm/emac/rgmii.c
* renesas/sh_eth.c
* socionext/sni_ave.c
* stmicro/stmmac/dwmac-stm32.c

freescale/dpaa2/dpaa2-mac.c is interesting. It checks whether any rgmii
mode other than PHY_INTERFACE_MODE_RGMII is used and complains that
delays are not supported in that case. The above comment says that the
MAC does not support adding delays. It seems that in that case, the only
working mode should be PHY_INTERFACE_MODE_RGMII_ID rather than
PHY_INTERFACE_MODE_RGMII. Is the code mixed up or my understanding?

Another interesting one is cadence/macb_main.c. While it handles all the
RGMII modes uniformly, the Zynq GEM hardware (supported by the driver)
does not actually support adding any delays. The driver happily accepts
these modes without telling the user that it really is using
PHY_INTERFACE_MODE_RGMII_ID. Should the driver warn about or reject the
other modes? Rejecting could break existing users. Some feedback
(failure or warning) would be very useful however.

stmicro/stmmac/dwmac-sti.c has a #define IS_PHY_IF_MODE_RGMII, which
seems to be a duplicate of phy_interface_mode_is_rgmii from
<linux/phy.h>. Should that or phy_interface_is_rgmii be used instead?

Helmut

RE: correct use of PHY_INTERFACE_MODE_RGMII{,_TXID,_RXID,_ID}

[Ioana Ciornei]

Hi Helmut,

> Subject: correct use of PHY_INTERFACE_MODE_RGMII{,_TXID,_RXID,_ID}
> 
> Hi,
> 
> I've been trying to write a dt for a board and got quite confused about the
> RGMII delays. That's why I looked into it and got even more confused by what I
> found. Different drivers handle this quite differently. Let me summarize.
> 
> Some drivers handle the RGMII modes individually. This is how I expected it to
> be. Examples:
> * renesas/ravb_main.c
> * stmicro/stmmac/dwmac-rk.c
> 
> A number of drivers handle all RGMII modes in uniformly. They don't actually
> configure any dealys. Is that supposed to work?  Examples:
> * apm/xgene/xgene_enet_main.c
> * aurora/nb8800.c
> * cadence/macb_main.c
> * freescale/fman/fman_memac.c
> * freescale/ucc_geth.c
> * ibm/emac/rgmii.c
> * renesas/sh_eth.c
> * socionext/sni_ave.c
> * stmicro/stmmac/dwmac-stm32.c
> 
> freescale/dpaa2/dpaa2-mac.c is interesting. It checks whether any rgmii mode
> other than PHY_INTERFACE_MODE_RGMII is used and complains that delays are
> not supported in that case. The above comment says that the MAC does not
> support adding delays. It seems that in that case, the only working mode should
> be PHY_INTERFACE_MODE_RGMII_ID rather than
> PHY_INTERFACE_MODE_RGMII. Is the code mixed up or my understanding?
> 

The snippet that you are referring to is copied below for quick reference:

/* The MAC does not have the capability to add RGMII delays so
 * error out if the interface mode requests them and there is no PHY
 * to act upon them
 */
if (of_phy_is_fixed_link(dpmac_node) &&
    (mac->if_mode == PHY_INTERFACE_MODE_RGMII_ID ||
     mac->if_mode == PHY_INTERFACE_MODE_RGMII_RXID ||
     mac->if_mode == PHY_INTERFACE_MODE_RGMII_TXID)) {
	netdev_err(net_dev, "RGMII delay not supported\n");

The important part which you seem to be missing is that a functional RGMII link can
have the delays inserted by the PHY, the MAC or by PCB traces (in this exact order
of preference). Here we check for any RGMII interface mode that
requests delays to be added when the interface is in fixed link mode
(using of_phy_is_fixed_link()), thus there is no PHY to act upon them.
This restriction, as the comment says, comes from the fact that the MAC
is not able to add RGMII delays.

When we are dealing with a fixed link, the only solution for DPAA2 is to use plain
PHY_INTERFACE_MODE_RGMII and to hope that somebody external to this Linux
system made sure that the delays have been inserted (be those PCB delays, or
internal delays added by the link partner).

Ioana

> Another interesting one is cadence/macb_main.c. While it handles all the RGMII
> modes uniformly, the Zynq GEM hardware (supported by the driver) does not
> actually support adding any delays. The driver happily accepts these modes
> without telling the user that it really is using PHY_INTERFACE_MODE_RGMII_ID.
> Should the driver warn about or reject the other modes? Rejecting could break
> existing users. Some feedback (failure or warning) would be very useful
> however.
> 
> stmicro/stmmac/dwmac-sti.c has a #define IS_PHY_IF_MODE_RGMII, which
> seems to be a duplicate of phy_interface_mode_is_rgmii from <linux/phy.h>.
> Should that or phy_interface_is_rgmii be used instead?
> 
> Helmut

Re: correct use of PHY_INTERFACE_MODE_RGMII{,_TXID,_RXID,_ID}

[Helmut Grohne]

Hi Ioana,

On Wed, Jun 10, 2020 at 11:10:23AM +0200, Ioana Ciornei wrote:
> > freescale/dpaa2/dpaa2-mac.c is interesting. It checks whether any rgmii mode
> > other than PHY_INTERFACE_MODE_RGMII is used and complains that delays are
> > not supported in that case. The above comment says that the MAC does not
> > support adding delays. It seems that in that case, the only working mode should
> > be PHY_INTERFACE_MODE_RGMII_ID rather than
> > PHY_INTERFACE_MODE_RGMII. Is the code mixed up or my understanding?
> 
> The snippet that you are referring to is copied below for quick reference:
> 
> /* The MAC does not have the capability to add RGMII delays so
>  * error out if the interface mode requests them and there is no PHY
>  * to act upon them
>  */
> if (of_phy_is_fixed_link(dpmac_node) &&
>     (mac->if_mode == PHY_INTERFACE_MODE_RGMII_ID ||
>      mac->if_mode == PHY_INTERFACE_MODE_RGMII_RXID ||
>      mac->if_mode == PHY_INTERFACE_MODE_RGMII_TXID)) {
> 	netdev_err(net_dev, "RGMII delay not supported\n");
> 
> The important part which you seem to be missing is that a functional RGMII link can
> have the delays inserted by the PHY, the MAC or by PCB traces (in this exact order
> of preference). Here we check for any RGMII interface mode that
> requests delays to be added when the interface is in fixed link mode
> (using of_phy_is_fixed_link()), thus there is no PHY to act upon them.
> This restriction, as the comment says, comes from the fact that the MAC
> is not able to add RGMII delays.
> 
> When we are dealing with a fixed link, the only solution for DPAA2 is to use plain
> PHY_INTERFACE_MODE_RGMII and to hope that somebody external to this Linux
> system made sure that the delays have been inserted (be those PCB delays, or
> internal delays added by the link partner).

If I am reading this correctly, you are saying that the DPAA2 driver is
operating as a PHY, not as a MAC here. Is that correct?

This distinction is a bit difficult (in particular for fixed links)
since RGMII is symmetric, but it is important for understanding the
definitions from
Documentation/devicetree/bindings/net/ethernet-controller.yaml:

|       # RX and TX delays are added by the MAC when required
|       - rgmii
| 
|       # 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-id
| 
|       # RGMII with internal RX delay provided by the PHY, the MAC
|       # should not add an RX delay in this case
|       - rgmii-rxid
| 
|       # RGMII with internal TX delay provided by the PHY, the MAC
|       # should not add an TX delay in this case
|       - rgmii-txid

These are turned into the matching PHY_INTERFACE_MODE_* by the OF code.

My understanding is that the delays are always described as seen by the
PHY. When it says that an "internal delay" (id) is present, the delay is
internal to the PHY, not the MAC. So unless DPAA2 is operating as a PHY,
it still seems reversed to me.

If we think of DPAA2 as a MAC (which seems more natural to me), it
should only allow rgmii-id, becaue it does not support adding delays
according to the comment.

Helmut

RE: correct use of PHY_INTERFACE_MODE_RGMII{,_TXID,_RXID,_ID}

[Ioana Ciornei]

> Subject: Re: correct use of PHY_INTERFACE_MODE_RGMII{,_TXID,_RXID,_ID}
> 
> Hi Ioana,
> 
> On Wed, Jun 10, 2020 at 11:10:23AM +0200, Ioana Ciornei wrote:
> > > freescale/dpaa2/dpaa2-mac.c is interesting. It checks whether any
> > > rgmii mode other than PHY_INTERFACE_MODE_RGMII is used and complains
> > > that delays are not supported in that case. The above comment says
> > > that the MAC does not support adding delays. It seems that in that
> > > case, the only working mode should be PHY_INTERFACE_MODE_RGMII_ID
> > > rather than PHY_INTERFACE_MODE_RGMII. Is the code mixed up or my
> understanding?
> >
> > The snippet that you are referring to is copied below for quick reference:
> >
> > /* The MAC does not have the capability to add RGMII delays so
> >  * error out if the interface mode requests them and there is no PHY
> >  * to act upon them
> >  */
> > if (of_phy_is_fixed_link(dpmac_node) &&
> >     (mac->if_mode == PHY_INTERFACE_MODE_RGMII_ID ||
> >      mac->if_mode == PHY_INTERFACE_MODE_RGMII_RXID ||
> >      mac->if_mode == PHY_INTERFACE_MODE_RGMII_TXID)) {
> > 	netdev_err(net_dev, "RGMII delay not supported\n");
> >
> > The important part which you seem to be missing is that a functional
> > RGMII link can have the delays inserted by the PHY, the MAC or by PCB
> > traces (in this exact order of preference). Here we check for any
> > RGMII interface mode that requests delays to be added when the
> > interface is in fixed link mode (using of_phy_is_fixed_link()), thus there is no
> PHY to act upon them.
> > This restriction, as the comment says, comes from the fact that the
> > MAC is not able to add RGMII delays.
> >
> > When we are dealing with a fixed link, the only solution for DPAA2 is
> > to use plain PHY_INTERFACE_MODE_RGMII and to hope that somebody
> > external to this Linux system made sure that the delays have been
> > inserted (be those PCB delays, or internal delays added by the link partner).
> 
> If I am reading this correctly, you are saying that the DPAA2 driver is operating
> as a PHY, not as a MAC here. Is that correct?
> 

Not at all. The dpaa2-mac driver operates as a MAC.

> This distinction is a bit difficult (in particular for fixed links) since RGMII is
> symmetric, but it is important for understanding the definitions from
> Documentation/devicetree/bindings/net/ethernet-controller.yaml:
> 
> |       # RX and TX delays are added by the MAC when required
> |       - rgmii
> |
> |       # 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-id
> |
> |       # RGMII with internal RX delay provided by the PHY, the MAC
> |       # should not add an RX delay in this case
> |       - rgmii-rxid
> |
> |       # RGMII with internal TX delay provided by the PHY, the MAC
> |       # should not add an TX delay in this case
> |       - rgmii-txid
> 
> These are turned into the matching PHY_INTERFACE_MODE_* by the OF code.
> 
> My understanding is that the delays are always described as seen by the PHY.
> When it says that an "internal delay" (id) is present, the delay is internal to the
> PHY, not the MAC. So unless DPAA2 is operating as a PHY, it still seems reversed
> to me.
> 
> If we think of DPAA2 as a MAC (which seems more natural to me), it should only
> allow rgmii-id, becaue it does not support adding delays according to the
> comment.
> 
> Helmut

I do see your point. According to the DT bindings document description, the phy-mode
property is imperative for a PHY (rgmii-rxid => apply RGMII RX delays) and also imperative
for the MAC but in the reverse direction (rgmii-rxid => apply RGMII TX delays). That
would be a reasonable interpretation, which some drivers take (dwmac_rk.c, as you
pointed out).*

* by the way, the second driver you highlighted to behave "as expected", ravb_main.c,
does _not_ do that, it applies all delays by itself (and _not_ in the reverse direction)
and then just masks them out (see "iface = PHY_INTERFACE_MODE_RGMII"
right before calling of_phy_connect), so that the PHY driver doesn't apply them again.
Arguably, this should be illegal if we were to quote the DT bindings document.

What is actually complicating these phy-mode values for RGMII is the fact that PCB
traces exist, but they aren't accounted for by the phy-mode description. So if you would
have delays introduced by PCB traces in a MAC-to-PHY setup, you would have no way
to describe that correctly and have a functional link at the same time (i.e. you couldn't
prevent the PHY _and_ the MAC from applying delays). 

Others have stared at that DT bindings description document too, it seems, and they took
what appears to be a more practical approach:
https://patchwork.ozlabs.org/project/netdev/patch/20190410005700.31582-19-olteanv@gmail.com/
https://patchwork.ozlabs.org/project/netdev/patch/20190413012822.30931-21-olteanv@gmail.com/

Interpreting the PHY modes as informative for the MAC when there is a PHY attached
allows us to account for PCB traces. Only if there is no PHY, the PHY mode becomes
imperative for the MAC, and PCB traces can be accounted for in that case as well.

Ioana

Re: correct use of PHY_INTERFACE_MODE_RGMII{,_TXID,_RXID,_ID}

[Andrew Lunn]

On Wed, Jun 10, 2020 at 10:12:37AM +0200, Helmut Grohne wrote:
> Hi,
> 
> I've been trying to write a dt for a board and got quite confused about
> the RGMII delays. That's why I looked into it and got even more confused
> by what I found. Different drivers handle this quite differently. Let me
> summarize.

Hi Helmut

In general, in DT, put what you want the PHY to do. If the PCB does
not add the delay, use rgmii-id. If the PCB does add the delay, then
use rgmii.

It is quite confusing, we have had bugs, and some drivers just do odd
things. In general, the MAC should not add delays. The PHY should add
delays, based on what is passed via DT. This assumes the PHY actually
implements the delays, which most PHYs do.

What exact hardware are you using?

     Andrew

Re: correct use of PHY_INTERFACE_MODE_RGMII{,_TXID,_RXID,_ID}

[Florian Fainelli]

On 6/10/2020 1:12 PM, Andrew Lunn wrote:
> On Wed, Jun 10, 2020 at 10:12:37AM +0200, Helmut Grohne wrote:
>> Hi,
>>
>> I've been trying to write a dt for a board and got quite confused about
>> the RGMII delays. That's why I looked into it and got even more confused
>> by what I found. Different drivers handle this quite differently. Let me
>> summarize.
> 
> Hi Helmut
> 
> In general, in DT, put what you want the PHY to do. If the PCB does
> not add the delay, use rgmii-id. If the PCB does add the delay, then
> use rgmii.
> 
> It is quite confusing, we have had bugs, and some drivers just do odd
> things. In general, the MAC should not add delays. The PHY should add
> delays, based on what is passed via DT. This assumes the PHY actually
> implements the delays, which most PHYs do.

In a MAC to MAC connection, one of the MAC, or both, or the PCB must
ensure that appropriate delays are inserted in order for
the RGMII connection to be functional. Or put differently, you could
view one of the MAC abiding to the 'phy-mode' property as if it was a PHY.

This is the reason why you may find Device Trees like
arch/arm/boot/dts/sun7i-a20-lamobo-r1.dts where we have a stmmac
connected to an external BCM53125 switch and the stmmac sets phy-mode =
'rgmii' while the switch sets phy-mode = 'rgmii-txid' in order to
account for the necessary delay.

The essential problem we have today is that there is no consistent, fool
proof and programmatic way to ensure that when a MAC and PHY or MAC and
MAC are interfaced to each other, that the delays are correct, this is
almost always a trial and error process which is irritating.
-- 
Florian