Message-ID: <84c534f9dbfa7c82300863cd40e5a9b6e6e29411.camel@icenowy.me>
Date: Thu, 05 Jun 2025 18:51:43 +0800
From: Icenowy Zheng <uwu@...nowy.me>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
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

在 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).

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

The phy.rst document says:
```
* 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
```

The changed binding document says:
```
# If the PCB does not add these delays via extra long traces,
# 'rgmii-id' should be used. Here, 'id' refers to 'internal delay',
# where either the MAC or PHY adds the delay.
```

In the case of MAC inserting delays, the phy.rst document assumes it's
PHY_INTERFACE_MODE_RGMII but the changed binding document assumes it's
'rgmii-id'.

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

Powered by blists - more mailing lists