Date:   Wed, 10 Jun 2020 11:44:05 +0000
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     Helmut Grohne <helmut.grohne@...enta.de>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>
Subject: RE: correct use of PHY_INTERFACE_MODE_RGMII{,_TXID,_RXID,_ID}

> 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

Powered by blists - more mailing lists