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

[Geert Uytterhoeven]

Hi Andrew,

On Thu, May 28, 2020 at 6:08 PM Andrew Lunn <andrew@xxxxxxx> wrote:
> On Thu, May 28, 2020 at 03:10:06PM +0200, Geert Uytterhoeven wrote:
> > 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.
>
> In general, i suggest that the PHY implements the delay, not the MAC.
> Most PHYs support it, where as most MACs don't. It keeps maintenance
> and understanding easier, if everything is the same. But there are
> cases where the PHY does not have the needed support, and the MAC does
> the delays.

I can confirm disabling the MAC delay ("phy-mode = "rgmii""), and adding
a PHY delay ("txc-skew-ps = <1500>") also fixes the slowness on
Salvator-X with R-Car M3-W ES1.0.

However, I would like to be a cit cautious here: on Ebisu with R-Car E3,
the hardware engineers advised to add "max-speed = <100>", as EtherAVB
on R-Car E3 does not support the MAC delay, and the KSZ9031 does not
allow sufficient delay, leading to unreliable communication.
Nevertheless, I never had problems without that limitation, and 1 Gbps
still seems to work after removing it, with and without "txc-skew-ps =
<1500>".

> > 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.
>
> Do you actually need MAC internal delays?

Given the Ebisu issue, I think we do.
Note that the EtherAVB MAC TX delay, when enabled, is 2.0 ns, and
KSZ9031 supports 0..1860 ps, with 900 ps being the centerpoint, so AFAIU
that is -900..960 ps, i.e. much less than 2.0 ns.

> > 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.
>
> So two bugs which cancelled each other out :-)
>
> > 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:
>
> I don't know if it is possible, but i would prefer that ravb does
> nothing and the PHY does the delay. The question is, can you get to
> this state without more things breaking?

While that seems to work for me, the delay would be a bit too small to work
reliably, according to the hardware engineers.

Hence my proposal for C now, to fix the regressions, and D later.

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