Message-ID: <d6149814-f772-418c-8fee-729d8665a8f2@lunn.ch>
Date: Tue, 15 Aug 2023 01:33:28 +0200
From: Andrew Lunn <andrew@...n.ch>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Vladimir Oltean <olteanv@...il.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next] net: dsa: mark parsed interface mode for legacy
 switch drivers

> This will be using the interface mode set in the switch port's
> configuration, rather than the interface mode that is in the CPU
> MAC node to which it is connected. This is different.
> 
> Essentially, when an ethernet driver connects to a PHY:
> 
>    MAC <---------------------------------> PHY
>     v					    v
> dt-mac-node {				dt-phy-node {
>   phy-mode = "rgmii-foo";		  /* contains no phy-mode */
> };					};
> 
> parses phy mode
> configures for RGMII mode
> ignores RGMII delays associated
>  with phy-mode
> applies any delays configured
>  by rx-internal-delay-ps and
>  tx-internal-delay-ps properties
> calls phy_attach(..., mode);
> phylib sets phy_dev->interface
> 					PHY driver uses phy_dev->interface
> 					 to configure RGMII delays
> 
> So, in this case, the dt-mac-node phy-mode property determines the
> delays at the PHY.

Mostly. There is at least one MAC driver which looks at phy-mode,
enables delays in the MAC based on the phy-mode. It then masks
phy-mode to remove the delays it has added, and passes phy_attach()
the masked value. This was i think done historically because there was
a board with a PHY which could not add the delays and the MAC
could. And that driver has got extended to other versions of the MAC
and has kept to this way of doing it.

I always push back against any new instances of this, and i don't
think any more have been added while i've been watching for it.

> The host MAC phy-mode property still has zero effect what so ever on
> the RGMII delay settings at the host MAC end of the link - and can be
> *any* of the four RGMII interface types. It really doesn't matter.

Except for the one case i outlined above.

> So, to summarise...
> 
> A host MAC connected to a phylib PHY, the host MAC's phy-mode property
> defines the RGMII delays at device on other end of the RGMII bus - which
> is the phylib PHY.

Except for the one case i outlined above. Unfortunately.

> A host MAC connected to a DSA switch, the host MAC's phy-mode property
> is irrelevant as far as RGMII delays are concerned, they have no
> effect on the device on the end of the RGMII bus.

I'm don't know if said MAC is every connected to a DSA switch....

> The big problem with RGMII delays has been this in the documentation:
> 
> "The PHY library offers different types of PHY_INTERFACE_MODE_RGMII*
> values to let the PHY driver and optionally the MAC driver, implement
> the required delay. The values of phy_interface_t must be understood
> from the perspective of the PHY device itself, leading to the
> following:"

I suspect this documentation was written to take into the account the
one oddball MAC.

> Note "and optionally the MAC driver". Well, no, there is nothing
> optional about this if one wants consistency of implementation - and
> I'll explain why in a moment, but first lets see what is expected of
> the PHY itself for each of these RGMII modes:
> 
> "* PHY_INTERFACE_MODE_RGMII: the PHY is not responsible for inserting any
>    internal delay by itself, it assumes that either the Ethernet MAC (if
>    capable) or the PCB traces insert the correct 1.5-2ns delay
> 
>  * PHY_INTERFACE_MODE_RGMII_TXID: the PHY should insert an internal delay
>    for the transmit data lines (TXD[3:0]) processed by the PHY device
> 
>  * PHY_INTERFACE_MODE_RGMII_RXID: the PHY should insert an internal delay
>    for the receive data lines (RXD[3:0]) processed by the PHY device

I guess 50% of PHYs get these two swapped around, simply because they
are never tested.

>  * PHY_INTERFACE_MODE_RGMII_ID: the PHY should insert internal delays for
>    both transmit AND receive data lines from/to the PHY device"
> 
> This is quite clear where the delay is inserted - by the *PHY* device.
> The above pre-dates my involvement in PHYLIB, and comes from a commit
> by Florian in November 2016, yet I seem to be often attributed with it.
> 
> Now, going back to that "optionally the MAC driver". Consider if we
> have, say, a PHY driver that is using host MAC M1 that has decided not
> to implement the delays (hey, they're optional!) Using
> PHY_INTERFACE_MODE_RGMII_TXID, to satisfy the above, the PHY is
> expected to insert the required delay for the transmit data lines.
> 
> Now lets say that the very same PHY driver uses host MAC M2, but that
> MAC driver has decided to implement these delays (hey, they're
> optional!) Using again PHY_INTERFACE_MODE_RGMII_TXID, the MAC driver
> decided to add delay to the transmit path. The PHY, however, also
> sees PHY_INTERFACE_MODE_RGMII_TXID and adds its own delay to the
> transmit data lines as it always has done. Now we have a double delay.

The one example of a MAC which does this also masks the value passed
to the PHY, so that PHY gets PHY_INTERFACE_MODE_RGMII. And it then all
works.

What we might want to do is document this masking. 

> So, that "and optionally the MAC driver" is what has historically led
> to problems with the various RGMII modes, with new implementations
> popping up and finding that host MAC M2 that's been in use for years
> with PHY device P1 (that hasn't implemented RGMII delays because the
> MAC driver did) now doesn't work with PHY device P2 (which does
> implement RGMII delays) that has also been in use for years.

I would actually say most problems have come about because the PHY has
not always implemented all four modes correctly. PHYs have silently
accepted one of the modes but not changed the hardware to actual do
what is requested, and kept with strapping defaults. Sometime later,
that mode has been correctly implemented, overwriting the strapping,
and breaking stuff. So it is on my checklist to ensure all four modes
set the hardware, or return -EOPNOTSUPP.

I would agree that reviewing and consistency has got better over time,
which is why we see less problems.

     Andrew

Powered by blists - more mailing lists