Re: [PATCH net-next v3 06/12] dt-bindings: net: dsa: lantiq,gswip: add support for MII delay properties

[Daniel Golle <daniel@makrotopia.org>]

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