Message-ID: <20220720210405.6wv6d3fssopnpm7x@skbuf>
Date:   Thu, 21 Jul 2022 00:04:05 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Arun.Ramadoss@...rochip.com
Cc:     andrew@...n.ch, linux-kernel@...r.kernel.org,
        UNGLinuxDriver@...rochip.com, vivien.didelot@...il.com,
        linux@...linux.org.uk, f.fainelli@...il.com, kuba@...nel.org,
        edumazet@...gle.com, pabeni@...hat.com, netdev@...r.kernel.org,
        Woojung.Huh@...rochip.com, davem@...emloft.net
Subject: Re: [RFC Patch net-next 07/10] net: dsa: microchip: apply rgmii tx
 and rx delay in phylink mac config

On Wed, Jul 20, 2022 at 02:51:42PM +0000, Arun.Ramadoss@...rochip.com wrote:
> > Why not all RGMII modes and only these 2? There was a discussion a long
> > time ago that the "_*ID" values refer to delays applied by an attached PHY.
> > Here you are refusing to apply RGMII TX delays in the "rgmii" and "rgmii-txid"
> > modes.
> 
> I have reused the code of ksz9477 cpu config function and added the dll
> configuration for lan937x family alone. And understood that if device
> tree specificies as rgmii_txid then apply the egress delay, for
> rgmii_rxid apply ingress delay, for rgmii_id apply both.
> From your comment, I am inferring that apply the mac delay for all the
> rgmii interface "rgmii*".
> Can you correct me if am I wrong and bit elaborate on it.

What we are trying is to interpret what's written in
Documentation/devicetree/bindings/net/ethernet-controller.yaml in a way
that is consistent, even though its wording makes it evident that that
it was written in simpler times.

There it says:

      # RX and TX delays are added by the MAC when required
      - 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-id

      # RGMII with internal RX delay provided by the PHY, the MAC
      # should not add an RX delay in this case
      - rgmii-rxid

      # RGMII with internal TX delay provided by the PHY, the MAC
      # should not add an TX delay in this case
      - rgmii-txid

The fact that the PHY adds delays in the directions specified by the
phy-mode value is established behavior; yet the MAC's behavior is pretty
much subject to interpretation, and this has resulted in varying
implementations in drivers.

The wise words above say that "RX and TX delays are added by the MAC
when required" - ok but when are they required? Determining this is
impossible based on the phy-mode alone, since there aren't just 2
degrees of freedom who may add delays (the PHY and the MAC). There also
exists the possibility of using serpentine traces (PCB delays), which
practically speaking, can't be described in phy-mode because then, a
potential PHY on the same link would also think they're for itself to
apply.

So the modern interpretation of phy-mode is to say that it simply does
not describe whether a MAC should apply delays in a direction or another.

So a separate set of properties was introduced, "rx-internal-delay-ps"
and "tx-internal-delay-ps". These have the advantage of being located
under the OF node of the MAC, and under the OF node of the PHY respectively.
So it's clearer which side is supposed to configure which delays, and
you also have finer control over the clock skew.

Initially when Prasanna first tried to upstream the lan937x, we discusssed
that since it's a new driver, it should support the modern interpretation
of RGMII delays, and we agreed on that strategy.

Now, with your recent refactoring that makes all switches share the same
probing logic (and OF node parsing), things are in a bit of a greyer area.

For one thing, you seem to have involuntarily inherited support for dubious
legacy bindings, such as the "phy-mode under the switch node" that is
described by commit edecfa98f602 ("net: dsa: microchip: look for
phy-mode in port nodes").

For another, you've inherited the existing interpretation of RGMII
delays from the KSZ9477 driver, which is that the driver interprets the
"phy-mode" as if it's a PHY (when in fact it's a MAC).

The KSZ9477 isn't by far the only MAC driver that applies RGMII delays
based on the phy-mode string, however in the past we made an effort to
update existing DSA drivers to the modern interpretation, see commits:

9ca482a246f0 ("net: dsa: sja1105: parse {rx, tx}-internal-delay-ps properties for RGMII delays")
5654ec78dd7e ("net: dsa: qca8k: rework rgmii delay logic and scan for cpu port 6")

There is a possibility to have a transitioning scheme: if the
"rx-internal-delay-ps" or "tx-internal-delay-ps" properties are present
under the MAC OF node, apply delays based on those. Otherwise, apply
delays based on phy-mode, and warn.

I'd appreciate if you could consider updating the KSZ common driver to
this interpretation of RGMII delays, so that the modern interpretation
becomes more widespread, and there are fewer places from which to copy a
non-standard one.

Powered by blists - more mailing lists