Message-ID: <fdc02e46e4906ba92b562f8d2516901adc85659b.camel@ew.tq-group.com>
Date: Mon, 28 Apr 2025 13:29:06 +0200
From: Matthias Schiffer <matthias.schiffer@...tq-group.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>, Andrew Lunn
	 <andrew@...n.ch>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>,  Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
 <pabeni@...hat.com>, Rob Herring <robh@...nel.org>,  Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Andy Whitcroft
 <apw@...onical.com>, Dwaipayan Ray <dwaipayanray1@...il.com>, Lukas Bulwahn
 <lukas.bulwahn@...il.com>, Joe Perches <joe@...ches.com>, Jonathan Corbet
 <corbet@....net>, Nishanth Menon <nm@...com>, Vignesh Raghavendra
 <vigneshr@...com>, Siddharth Vadapalli <s-vadapalli@...com>, Roger Quadros
 <rogerq@...nel.org>, Tero Kristo <kristo@...nel.org>,
 linux-doc@...r.kernel.org,  linux-kernel@...r.kernel.org,
 netdev@...r.kernel.org, devicetree@...r.kernel.org, 
 linux-arm-kernel@...ts.infradead.org, linux@...tq-group.com
Subject: Re: [PATCH net-next 1/4] dt-bindings: net: ethernet-controller:
 update descriptions of RGMII modes

On Tue, 2025-04-22 at 16:31 +0100, Russell King (Oracle) wrote:
> ********************
> Achtung externe E-Mail: Öffnen Sie Anhänge und Links nur, wenn Sie wissen, dass diese aus einer sicheren Quelle stammen und sicher sind. Leiten Sie die E-Mail im Zweifelsfall zur Prüfung an den IT-Helpdesk weiter.
> Attention external email: Open attachments and links only if you know that they are from a secure source and are safe. In doubt forward the email to the IT-Helpdesk to check it.
> ********************
> 
> On Tue, Apr 22, 2025 at 05:00:37PM +0200, Andrew Lunn wrote:
> > On Mon, Apr 21, 2025 at 08:20:29PM +0100, Russell King (Oracle) wrote:
> > > On Tue, Apr 15, 2025 at 12:18:01PM +0200, Matthias Schiffer wrote:
> > > > diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > > > index 45819b2358002..2ddc1ce2439a6 100644
> > > > --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > > > +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
> > > > @@ -74,19 +74,21 @@ properties:
> > > >        - rev-rmii
> > > >        - moca
> > > >  
> > > > -      # RX and TX delays are added by the MAC when required
> > > > +      # RX and TX delays are part of the board design (through PCB traces). MAC
> > > > +      # and PHY must not add delays.
> > > >        - rgmii
> > > >  
> > > > -      # RGMII with internal RX and TX delays provided by the PHY,
> > > > -      # the MAC should not add the RX or TX delays in this case
> > > > +      # RGMII with internal RX and TX delays provided by the MAC or PHY. No
> > > > +      # delays are included in the board design; this is the most common case
> > > > +      # in modern designs.
> > > >        - rgmii-id
> > > >  
> > > > -      # RGMII with internal RX delay provided by the PHY, the MAC
> > > > -      # should not add an RX delay in this case
> > > > +      # RGMII with internal RX delay provided by the MAC or PHY. TX delay is
> > > > +      # part of the board design.
> > > >        - rgmii-rxid
> > > >  
> > > > -      # RGMII with internal TX delay provided by the PHY, the MAC
> > > > -      # should not add an TX delay in this case
> > > > +      # RGMII with internal TX delay provided by the MAC or PHY. RX delay is
> > > > +      # part of the board design.
> > > >        - rgmii-txid
> > > >        - rtbi
> > > >        - smii
> > > 
> > > Sorry, but I don't think this wording improves the situation - in fact,
> > > I think it makes the whole thing way more confusing.
> > > 
> > > Scenario 1: I'm a network device driver author. I read the above, Okay,
> > > I have a RGMII interface, but I need delays to be added. I'll detect
> > > when RGMII-ID is used, and cause the MAC driver to add the delays, but
> > > still pass PHY_INTERFACE_MODE_RGMII_ID to phylib.
> > > 
> > > Scenario 2: I'm writing a DT file for a board. Hmm, so if I specify
> > > rgmii because the delays are implemented in the traces, but I need to
> > > fine-tune them. However, the documentation says that delays must not
> > > be added by the MAC or the PHY so how do I adjust them. I know, I'll
> > > use rgmii-id because that allows delays!
> > > 
> > > I suspect neither of these two are really want you mean, but given
> > > this wording, that's exactly where it leads - which is more
> > > confusion and less proper understanding.
> > 
> > These DT documents are supposed to be normative and OS agnostic. I
> > wounder what the DT Maintainers will say if we add an Informative
> > section afterwards giving a detailed description of how Linux actually
> > implements these normative statements? It will need to open with a
> > clear statement that it is describing Linux behaviour, and other OSes
> > can implement the normative part in other ways and still be compliant,
> > but that Linux has seen a lot of broken implementations and so wants
> > to add Informative information to guide Linux developers.
> 
> Well, looking at ePAPR, the only thing that was defined back then was
> the presence of a property to describe the interface type between the
> ethernet device and PHY. The values were left to the implementation
> to decide upon, but with some recommendations.
> 
> What that means is that the values to this property are not part of
> the DT standard, but are a matter for the implementation.
> 
> However, with the yaml stuff, if that is basically becoming "DT
> specification" then it needs to be clearly defined what each value
> actually means for the system, and not this vague airy-fairy thing
> we have now.
> 
> We've learnt the hard way in the kernel where that gets us with
> the number of back-compat breaking changes we've had to make where
> the RGMII delays have been totally wrongly interpreted and leaving
> it vague means that other implementations will suffer the same pain.

I agree with Russell that it seems preferable to make it unambiguous whether
delays are added on the MAC or PHY side, in particular for fine-tuning. If
anything is left to the implementation, we should make the range of acceptable
driver behavior very clear in the documentation.

Historically, there appear to exist at least 4 different ways to handle the
RGMII modes in MAC drivers:

(I'm using the terms "id" and "noid" in the following to refer to modes with and
without delays independently of the direction; a single driver may fall into
different categories for the RX and TX side)

1) MAC ignores the mode, does not add delays, passes the mode to the PHY
  - Mode "noid" is used when delays are added by the PCB
  - PHY sees the same interface mode that is specified in the DT
  - Does not match the old wording of the DT binding docs (as the MAC never adds
    delays)

2) MAC adds delays in "noid" mode, passes the mode to the PHY unchanged
  - Delays added by the PCB can only be supported via driver-specific
    fine-tuning properties on the MAC or PHY side
  - Without driver-specific properties, none of the modes allow for delays on
    the PCB; every mode adds delays either on the MAC or the PHY side
  - PHY sees the same interface mode that is specified in the DT
  - Matches the old wording of the DT binding docs (which don't allow for delays
    added by the PCB)

3) MAC has a fixed delay, but also passes the interface mode unchanged to the
  PHY (example: TX delays in TI CPSW AM65)
  - No support for delays on the PCB due to hardware limitation
  - PHY sees the same interface mode that is specified in the DT
  - For the direction in which the delays are added by the MAC, you need to
    specify "noid" in the DT even though there is an internal delay

4) MAC adds delays in "id" mode and fixes it up so "noid" is passed to
  the PHY (example: TX delays in TI ICSSG)
  - No support for delays on the PCB due to hardware limitation
  - PHY does NOT see the same interface mode that is specified in the DT
  - Fine-tuning options may be confusing because of the fixup

Of all of these variants:
- 1 and 2 appear to be most common in existing MAC drivers
- 2 and maybe 3 match the old wording of the DT binding docs
- 1, 2 and 3 pass the interface mode unchanged to the PHY
- 1 and 4 match my proposed new wording of the DT binding docs based on Andrew's
  input
- 1 allows for designs that have a delay on the PCB without driver-specific
  fine-tuning
- 2 and maybe 3 and 4 allow for designs where the PHY can't add delays and none
  exist on the PCB (or there is no PHY - could be two SoCs directly connected
  via RGMII, or a switch IC we can't control)

And of course, things become even more muddled when driver-specific properties
for fine-tuning etc. are involved.

Any change to existing drivers will need to be made in a backwards-compatible
way, meaning that we can't break compatibility with old Device Trees. If we
somehow want to clean up this mess, and also support delays on the PCB, designs
without (configurable) PHY, and unambiguously specify where delays are added, I
don't think the existing rgmii(-*id) modes are going to suffice. I think
something along the lines of the following might be a possible way forward:

- Introduce new DT properties to specify whether delays should be added on the
MAC or PHY side, for RX and TX independently. Could also replace some driver-
specific fine-tuning properties.
- In the presence of the new properties, only "rgmii" can be specified as phy-
mode, the delays are not part of the PHY mode property anymore
- MAC and PHY drivers must reject delay configurations they can't satisfy
- Not specifying the new properties results in a deprecation warning on existing
drivers. New drivers make the new properties mandatory.
- Not specifying the new properties causes drivers to interpret "rgmii(-*id)"
however they have done in the past to maintain backwards compatibility

Implementation of the new properties would need to be done in a way that allows
MAC and PHY drivers to be converted one by one, and to only print deprecation
warnings asking for the DT to be fixed once the conversion has been done.
Attempting to specify the new properties when not both MAC and PHY driver
support them must be rejected, so the drivers gaining support for them can't
result in a breaking change.

I would be very happy to hear some feedback on these ideas - of course,
alternative ideas that are easier to implement would be even more welcome...

Best,
Matthias


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
https://www.tq-group.com/

Powered by blists - more mailing lists