Message-ID: <20251028013944.lmjboagptxl4dob7@skbuf>
Date: Tue, 28 Oct 2025 03:39:44 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Daniel Golle <daniel@...rotopia.org>
Cc: Hauke Mehrtens <hauke@...ke-m.de>, Andrew Lunn <andrew@...n.ch>,
	"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>, Simon Horman <horms@...nel.org>,
	Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	Andreas Schirm <andreas.schirm@...mens.com>,
	Lukas Stockmann <lukas.stockmann@...mens.com>,
	Alexander Sverdlin <alexander.sverdlin@...mens.com>,
	Peter Christen <peter.christen@...mens.com>,
	Avinash Jayaraman <ajayaraman@...linear.com>,
	Bing tao Xu <bxu@...linear.com>, Liang Xu <lxu@...linear.com>,
	Juraj Povazanec <jpovazanec@...linear.com>,
	"Fanni (Fang-Yi) Chan" <fchan@...linear.com>,
	"Benny (Ying-Tsan) Weng" <yweng@...linear.com>,
	"Livia M. Rosu" <lrosu@...linear.com>,
	John Crispin <john@...ozen.org>
Subject: Re: [PATCH net-next v3 06/12] dt-bindings: net: dsa: lantiq,gswip:
 add support for MII delay properties

On Mon, Oct 27, 2025 at 11:41:10PM +0000, Daniel Golle wrote:
> On Tue, Oct 28, 2025 at 01:04:39AM +0200, Vladimir Oltean wrote:
> > On Sun, Oct 26, 2025 at 11:45:19PM +0000, Daniel Golle wrote:
> > > Add support for standard tx-internal-delay-ps and rx-internal-delay-ps
> > > properties on port nodes to allow fine-tuning of RGMII clock delays.
> > > 
> > > The GSWIP switch hardware supports delay values in 500 picosecond
> > > increments from 0 to 3500 picoseconds, with a default of 2000
> > > picoseconds for both TX and RX delays.
> > > 
> > > This corresponds to the driver changes that allow adjusting MII delays
> > > using Device Tree properties instead of relying solely on the PHY
> > > interface mode.
> > > 
> > > Signed-off-by: Daniel Golle <daniel@...rotopia.org>
> > > ---
> > > v3:
> > >  * redefine ports node so properties are defined actually apply
> > >  * RGMII port with 2ps delay is 'rgmii-id' mode
> > > 
> > >  .../bindings/net/dsa/lantiq,gswip.yaml        | 29 +++++++++++++++++--
> > >  1 file changed, 26 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/net/dsa/lantiq,gswip.yaml b/Documentation/devicetree/bindings/net/dsa/lantiq,gswip.yaml
> > > index f3154b19af78..b0227b80716c 100644
> > > --- a/Documentation/devicetree/bindings/net/dsa/lantiq,gswip.yaml
> > > +++ b/Documentation/devicetree/bindings/net/dsa/lantiq,gswip.yaml
> > > @@ -6,8 +6,29 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
> > >  
> > >  title: Lantiq GSWIP Ethernet switches
> > >  
> > > -allOf:
> > > -  - $ref: dsa.yaml#/$defs/ethernet-ports
> > > +$ref: dsa.yaml#
> > > +
> > > +patternProperties:
> > > +  "^(ethernet-)?ports$":
> > > +    type: object
> > > +    patternProperties:
> > > +      "^(ethernet-)?port@[0-6]$":
> > > +        $ref: dsa-port.yaml#
> > > +        unevaluatedProperties: false
> > > +
> > > +        properties:
> > > +          tx-internal-delay-ps:
> > > +            enum: [0, 500, 1000, 1500, 2000, 2500, 3000, 3500]
> > > +            default: 2000
> > 
> > No. This is confusing and wrong. I looked at the driver implementation
> > code, wanting to note that it has the potential of being a breaking
> > change for device trees without the "tx-internal-delay-ps" and
> > "rx-internal-delay-ps" properties.
> > 
> > But then I saw that the driver implementation is subtly different.
> > "tx-internal-delay-ps" defaults to 2000 only if "rx-internal-delay-ps" is set, and
> > "rx-internal-delay-ps" defaults to 2000 only if "tx-internal-delay-ps" is set.
> > 
> > So when implemented in this way, it won't cause the regressions I was
> > concerned about, but it is misrepresented in the schema.
> > 
> > Why overcomplicate this and just not set a default? Modify the RX clock
> > skew if set, and the TX clock skew if set.
> 
> The problem is that before adding support for both *-internal-delay-ps
> properties the internal delays would be set exclusively based on the
> interface mode -- and are inverted logic:
> 
> ```
>          switch (state->interface) {
>          case PHY_INTERFACE_MODE_RGMII_ID:
>                  gswip_mii_mask_pcdu(priv, GSWIP_MII_PCDU_TXDLY_MASK |
>                                            GSWIP_MII_PCDU_RXDLY_MASK, 0, port);
>                  break;
>          case PHY_INTERFACE_MODE_RGMII_RXID:
>                  gswip_mii_mask_pcdu(priv, GSWIP_MII_PCDU_RXDLY_MASK, 0, port);
>                  break;
>          case PHY_INTERFACE_MODE_RGMII_TXID:
>                  gswip_mii_mask_pcdu(priv, GSWIP_MII_PCDU_TXDLY_MASK, 0, port);
>                  break;
>          default:
>                  break;
>          }
> ```
> 
> As you can see the delays are set to 0 in case of the interface mode
> being RGMII_ID (and the same for RGMII_RXID and RGMII_TXID
> respectively).
> 
> This is probably the result of the delays being initialized to 2000ps by
> default, and if the **PHY connected to the switch port** is set to take
> care of the clk/data delay then the switch port RGMII interface doesn't
> have to do it.
> 
> From my understanding this is a bit awkward as "internal delay" usually
> means the delay is taken care of by the PHY rather than by discrete
> parts of the board design. Here, however, it is *never* part of the
> board design and always handled by either the switch RGMII interface
> (MAC side) or the connected PHY.
> 
> So in order to not break existing board device trees expecting this
> behavior I've decided to only fall-back to adjust the delay based on the
> interface mode in case both properties are missing.
> 
> Please correct me if that's the wrong thing to do or if my understanding
> is flawed in any way.

Ok, I missed the fact that there's RGMII delay handling outside of
gswip_mii_delay_setup() too (a bit bizarre).

So "why overcomplicate this" has a good reason. You have legacy to
maintain for xrx200, xrx300, xrx330 - essentially the same situation as
documented in sja1105_parse_rgmii_delays(). But no legacy for the newly
introduced switches though?  I don't see why you'd opt them into this
behaviour of applying MAC delays based on phy-mode.

Also, the point still stands that your documented default delay value
is incorrect. What happens in lack of one property depends on the
presence of the other, and on the phy-mode. I think deleting the
default value from the schema is much better than having wrong
documentation for it.

Powered by blists - more mailing lists