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

[Oleksij Rempel <o.rempel@pengutronix.de>]

On Thu, May 28, 2020 at 06:08:39PM +0200, Andrew Lunn wrote:
> On Thu, May 28, 2020 at 03:10:06PM +0200, Geert Uytterhoeven wrote:
> > Hi Andrew,
> > 
> > On Wed, May 27, 2020 at 10:52 PM Andrew Lunn <andrew@lunn.ch> 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.
> 
> > 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?
> 
> > 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.
> 
> O.K. so not so bad.
> 
> > 
> > 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?

Some MACs, for example the Atheros AG71XX support delay configuration as
well. But it support also the clock direction. It means (please correct me if
i'm wrong), the MAC can be configured to act as PHY. The same is about
switches, the MAC attached to CPU is act as a PHY and should care about proper
delay configuration.

Regards,
Oleksij

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |