Message-ID: <aEFmNMSvffMvNA8I@shell.armlinux.org.uk>
Date: Thu, 5 Jun 2025 10:41:08 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Icenowy Zheng <uwu@...nowy.me>
Cc: Andrew Lunn <andrew@...n.ch>, Rob Herring <robh@...nel.org>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Chaoyi Chen <chaoyi.chen@...k-chips.com>,
	Matthias Schiffer <matthias.schiffer@...tq-group.com>,
	Heiner Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net v2] dt-bindings: net: ethernet-controller: Add
 informative text about RGMII delays

On Thu, Jun 05, 2025 at 05:06:43PM +0800, Icenowy Zheng wrote:
> In addition, analyzing existing Ethernet drivers, I found two drivers
> with contradition: stmicro/stmmac/dwmac-qcom-ethqos.c and
> ti/icssg/icssg_prueth.c .
> 
> The QCOM ETHQOS driver enables the MAC's TX delay if the phy_mode is
> rgmii or rgmii-rxid, and the PRU ETH driver, which works on some MAC
> with hardcoded TX delay, rejects rgmii and rgmii-rxid, and patches
> rgmii-id or rgmii-txid to remove the txid part.

No, this is wrong.

First, it does not reject any RGMII mode. See qcom_ethqos_probe() and
the switch() in there. All four RGMII modes are accepted.

The code in ethqos_rgmii_macro_init() is the questionable bit, but
again, does _not_ do any rejection of any RGMII mode. It simply sets
the transmit clock phase shift according to the mode, and the only
way this can work is if the board does not provide the required delay.

This code was not reviewed by phylib maintainers, so has slipped
through the review process. It ought to be using the delay properties
to configure the MAC.

> The logic of QCOM ETHQOS clearly follows the original DT binding, which

Let's make this clear. "original DT binding" - no, nothing has
*actually* changed with the DT binding - the meaning of the RGMII
modes have not changed. The problem is one of interpretation, and
I can tell you from personal experience that getting stuff documented
so that everyone gets the same understanding is nigh on impossible.
People will pick holes, and deliberately interpret whatever is written
in ways that it isn't meant to - and the more words that are used the
more this happens.

The RGMII modes have been documented in Documentation/networking/phy.rst
(Documentation/networking/phy.txt predating) since:

commit bf8f6952a233f5084431b06f49dc0e1d8907969e
Author: Florian Fainelli <f.fainelli@...il.com>
Date:   Sun Nov 27 18:45:14 2016 -0800

    Documentation: net: phy: Add blurb about RGMII

    RGMII is a recurring source of pain for people with Gigabit Ethernet
    hardware since it may require PHY driver and MAC driver level
    configuration hints. Document what are the expectations from PHYLIB and
    what options exist.

    Reviewed-by: Martin Blumenstingl <martin.blumenstingl@...glemail.com>
    Signed-off-by: Florian Fainelli <f.fainelli@...il.com>
    Signed-off-by: David S. Miller <davem@...emloft.net>

> describes "rgmii-id" as `RGMII with internal RX and TX delays provided
> by the PHY, the MAC should not add the RX or TX delays in this case`
> (the driver skips the delay for rgmii-id). The logic of PRU ETH follows
> the logic of the new DT binding. This shows that the DT binding patch
> is not a simple clarification, but a change of meanings.

Let me say again. Nothing has changed. There is no "old binding" or
"new binding". If you think there is, then it's down to
misinterpretation.

This is precisely why I've been opposed to documenting these properties
in the binding document _and_ Documentation/networking/phy.* because
keeping them both in sync is going to be a pain, leading to ambiguity
and misinterpretation.

> > If you want the kernel to not touch the PHY, use
> > 
> > phy-mode = 'internal'
> 
> This sounds weird, and may introduce side effect on the MAC side.
> 
> Well we might need to allow PHY to have phy-mode property in addition
> to MAC, in this case MAC phy-mode='rgmii*' and PHY phy-mode='internal'
> might work?

I'm not convinced that adding more possibilities to the problem (i.o.w.
the idea that phy=mode = "internal" can be used to avoid the delays
being messed with) is a good idea - not at this point, because as you
point out MACs (and PHYs) won't know that they need to be configured
for RGMII mode. "internal" doesn't state this, and if we do start doing
this, we'll end up with "internal" selecting RGMII mode which may work
for some platforms but not all.

So, IMHO this is a bad idea.

> > > In addition, the Linux kernel contains a "Generic PHY" driver for
> > > any
> > > 802.1 c22 PHYs to work, without setting any delays.
> > 
> > genphy is best effort, cross your fingers, it might work if you are
> > luckily. Given the increasing complexity of PHYs, it is becoming less
> > and less likely to work. From a Maintainers perspective, i only care
> > if the system works with the proper PHY driver for the
> > hardware. Anything else is unmaintainable.
> 
> Well this sounds unfortunate but reasonable.

We're already in this state with PHYs faster than gigabit, because
IEEE 802.3 in their wisdom did not define where the 1000BASE-T
autoneg parameters appear in the register space. As a result, vendors
have done their own thing, and every vendor / PHY is different.
Without access to this key data, phylib has no way to know the
negotiation results. Thus, a generic PHY driver that works correctly
for PHYs > 1G just isn't possible.

I expect that in years to come, we'll see IEEE 802.3 updated with
the 1G registers for Clause 45 PHYs, but the boat has already sailed
so this would be totally pointless as there will be too many PHYs
out there doing their own thing for whatever IEEE 802.3 says about
this to have any relevence what so ever. Just like they did with
2500BASE-X, which is a similar mess due to IEEE 802.3 being way too
late.

I hope that there isn't going to be more of this, because each time
it happens, the IEEE 802.3 "standard" less relevant.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists