Message-ID: <aEGRR6kTZT_B5oYt@shell.armlinux.org.uk>
Date: Thu, 5 Jun 2025 13:44:55 +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 06:51:43PM +0800, Icenowy Zheng wrote:
> 在 2025-06-05星期四的 10:41 +0100，Russell King (Oracle)写道：
> > 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.
> 
> Well my sentence have its subject switched here. I mean the TI PRU ETH
> driver is rejecting modes.
> 
> > 
> > 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.
> 
> Well I am not sure, considering two examples I raised here (please note
> I am comparing QCOM ETHQOS and TI PRUETH two drivers, they have
> contrary handling of RGMII modes, and one matches the old binding
> document, one matches the new one).

Code sometimes sneaks in that hasn't been reviewed by the phylib
maintainers. That seems to be the case with the qcom ethqos driver.
Note the lack of tags from a phylib maintainer. However, I haven't
checked the mailing list history, maybe they put forward a good
reason for the code being as it is.

However, the fundamental fact is that the PHY interface mode is
passed into phylink and phylib unchanged, which instructs the PHY
driver to set the delays at the PHY as per that mode - as per the
phylib documentation.

One reason the code in qcom ethqos may exist is that all boards do
not insert the transmit delay, so the MAC needs to if the PHY
isn't. That may have been covered on the mailing list. I don't
know without checking, and I'm not able to check at the moment.

> > The RGMII modes have been documented in
> > Documentation/networking/phy.rst
> > (Documentation/networking/phy.txt predating) since:
> 
> I checked the document here, and it seems that it's against the changed
> binding document (it matches the original one):

I repeatedly raised the issue that the phylib documentation is the
definitive documentation, and we shouldn't be duplicating it. If you
think that the binding document is contrary to what the phylib doc
says, then the binding document is wrong.

I stand by my comment in the review. We should *not* be documenting
the same thing in two different places. You've proven my point.

I suggest we get rid of the "clarification" in the binding document
because it's just adding to the confusion. Document stuff in one
place, and one place only. Anything else is madness and leads to
exactly this problem.

-- 
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