Re: [PATCH net-next] net: dsa: mark parsed interface mode for legacy switch drivers

[Vladimir Oltean <olteanv@gmail.com>]

On Mon, Aug 14, 2023 at 11:03:00PM +0100, Russell King (Oracle) wrote:
> 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.
> 
> 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.
> 
> A DSA MAC, the DSA MAC's phy-mode property is used to configure the
> RGMII delays on the *local* end of the RGMII bus.
> 
> This is what happens with the mv88e6xxx driver, whether intentional or
> not. In the case of a DSA to host MAC link, there is no attempt by DSA
> to delve into the host MAC's DT to retrieve the phy-mode property
> there.
> 
> 
> 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:"
> 
> 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
> 
>  * 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.
> 
> 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.
> 
> It's because that "optionally" stuff at the MAC end has led people
> down the path of _sometimes_ implementing RGMII delays at the MAC
> end of the link, and other times implementing RGMII delays at the
> PHY end of the link according to the phy-mode specification at the
> host MAC.
> 
> It seems to me that since we had this understanding that RGMII delays
> are applied at the PHY end of the link for RGMII, we have had
> significantly less "my RGMII doesn't work" stuff. That's not really
> my doing - that's Florian's, by writing the specification for the
> what is expected of the PHY device for each of the RGMII phy
> interface modes back in November 2016. My only part in it was only
> later ensuring that everyone was singing off the same hymm sheet with
> what had already been decided - so we didn't get different reviewers
> telling people different things that were also different from what
> had been documented.
> 
> ... and with that consistency, we now appear to have way less issues
> with RGMII - or at least that is my impression in terms of the emails
> I see as one of the co-phylib maintainers (thus I get the emails!)
> 
> At the end of the day, what is important for inter-operability between
> PHYs and MACs is that *both* implement the RGMII delays in a consistent
> manner, so if PHYs are to insert delays and MACs not, then all PHY
> drivers need to insert delays and all MACs must not.
> 
> We had been heading to a situation where some MACs did, other MACs
> didn't, some PHY drivers did, some PHY drivers didn't...
> 
> Anyway, this seems to have turned into a very long email... wasn't
> supposed to be, but I suspect if I didn't cover everything, there
> would be a very long email thread instead... well, there probably
> will be picking this apart and disagreeing with bits of it...

Russell, I agree with your analysis that the MAC side's required
interpretation of the phy-mode is underspecified.

What has worked for me was the addition of the "rx-internal-delay-ps"
and "tx-internal-delay-ps" properties in the MAC OF node. When those are
present, I believe that the behavior of the MAC side is fully specified.

Today, the sja1105, qca8k and ksz DSA drivers check for the presence of
these 2 properties, and if found, they will use them, otherwise they
will fall back to the old school of thought - if fixed-link, apply the
delays ourselves.

In addition, rtl8365mb has only the new behavior. It does not have the
old-school logic at all.

There exists a direct migration path between one school of thought and
the other, which preserves functionality for existing device trees, just
prints a warning if rx-internal-delay-ps and tx-internal-delay-ps are
missing on fixed-link ports.

Do you believe that there is value in converting the mv88e6xxx driver
(and gradually, more and more other drivers) to work with the
fully-specified DT bindings?