Re: [PATCH net-next v3] net: phy: micrel: add phy-mode support for the KSZ9031 PHY

[Geert Uytterhoeven]

Hi Andrew,

On Wed, May 27, 2020 at 10:52 PM Andrew Lunn <andrew@xxxxxxx> wrote:
> > You may wonder what's the difference between 3 and 4? It's not just the
> > PHY driver that looks at phy-mode!
> > drivers/net/ethernet/renesas/ravb_main.c:ravb_set_delay_mode() also
> > does, and configures an additional TX clock delay of 1.8 ns if TXID is
> > enabled.
>
> That sounds like a MAC bug. Either the MAC insert the delay, or the
> PHY does. If the MAC decides it is going to insert the delay, it
> should be masking what it passes to phylib so that the PHY does not
> add a second delay.

And so I gave this a try, and modified the ravb driver to pass "rgmii"
to the PHY if it has inserted a delay.
That fixes the speed issue on R-Car M3-W!
And gets rid of the "*-skew-ps values should be used only with..."
message.

I also tried if I can get rid of "rxc-skew-ps = <1500>". After dropping
the property, DHCP failed.  Compensating by changing the PHY mode in DT
from "rgmii-txid" to "rgmii-id" makes it work again.

However, given Philippe's comment that the rgmi-*id apply to the PHY
only, I think we need new DT properties for enabling MAC internal delays.

> This whole area of RGMII delays has a number of historical bugs, which
> often counter act each other. So you fix one, and it break somewhere
> else.

Indeed...

> In this case, not allowing skews for plain RGMII is probably being too
> strict. We probably should relax that constrain in this case, for this
> PHY driver.

That description is not quite correct: the driver expects skews for
plain RGMII only. For RGMII-*ID, it prints a warning, but still applies
the supplied skew values.


To fix the issue, I came up with the following problem statement and
plan:

A. Old behavior:

  1. ravb acts upon "rgmii-*id" (on SoCs that support it[1]),
  2. ksz9031 ignored "rgmii-*id", using hardware defaults for skew
     values.

B. New behavior (broken):

  1. ravb acts upon "rgmii-*id",
  2. ksz9031 acts upon "rgmii-*id".

C. Quick fix for v5.8 (workaround, backwards-compatible with old DTB):

  1. ravb acts upon "rgmii-*id", but passes "rgmii" to phy,
  2. ksz9031 acts upon "rgmi", using new "rgmii" skew values.

D. Long-term fix:
  1. Check if new boolean "renesas,[rt]x-delay"[2] values are
      specified in DTB.
       No: ravb acts upon "rgmii-*id", but passes "rgmii" to phy, for
           backwards-compatibility,
       Yes: ravb enables TX clock delay of 2.0 ns and/or RX clock delay
            of 1.8 ns, based on "renesas,[rt]x-delay" values, and passes
            the unmodified interface type to phy,
  2. ksz9031 acts upon "rgmii*",
  3. Salvator-X(S) DTS makes things explicit by changing it from

        phy-mode = "rgmii-txid";
        rxc-skew-ps = <1500>;

     to:

        phy-mode = "rgmii";
        renesas,rx-delay = <false>;
        renesas,tx-delay = <true>;
        rxc-skew-ps = <1500>;

     or:

        phy-mode = "rgmii";
        renesas,rx-delay = <true>;
        renesas,tx-delay = <true>;

[2] Should we use numerical "renesas,[rt]x-delay-ps" instead?
     The only supported values are 0 and 2000 (TX) or 1800 (RX).

The ULCB boards are very similar to Salvator-X(S), so I guess they
behave the same, and are thus affected.

Unfortunately there are other boards that use R-Car Gen3 EtherAVB:
  - The Silicon Linux sub board for CAT874 (CAT875) connects EtherAVB to
    an RTL8211E PHY.  As it uses the "rgmii" mode, it should not be
    affected.
  - The HiHope RZ/G2M sub board connects EtherAVB to an RTL8211E PHY.
    It uses the "rgmii-txid" mode, so it will be affacted by modifying
    the ravb driver.
  - Eagle and V3MSK connect EtherAVB to a KSZ9031, and use the "rgmii-id"
    mode with rxc-skew-ps = <1500>, so they are affected.
  - Ebisu and Draak connect EtherAVB to a KSZ9031, and use the "rgmii"
    mode with rxc-skew-ps = <1500>.  However, they're limited to 100
    Mbps, as EtherAVB on R-Car E3 and D3 do not support TX clock
    internal delay mode[1], and the delays provided by KSZ9031 clock pad
    skew were deemed insufficient.

Obviously, the affected boards will need testing (I only have
Salvator-X(S) and Ebisu).

Does the above make sense?
Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds