[PATCH v3] net: phy: add driver for Motorcomm yt8511 phy

[Peter Geis]

On Fri, May 14, 2021 at 9:24 AM Andrew Lunn <andrew at lunn.ch> wrote:
>
> On Fri, May 14, 2021 at 07:58:26AM -0400, Peter Geis wrote:
> > Add a driver for the Motorcomm yt8511 phy that will be used in the
> > production Pine64 rk3566-quartz64 development board.
> > It supports gigabit transfer speeds, rgmii, and 125mhz clk output.
>
> Thanks for adding RGMII support.
>
> > +#define PHY_ID_YT8511                0x0000010a
>
> No OUI in the PHY ID?
>
> Humm, the datasheet says it defaults to zero. That is not very
> good. This could be a source of problems in the future, if some other
> manufacture also does not use an OUI.
>
> > +/* RX Delay enabled = 1.8ns 1000T, 8ns 10/100T */
> > +#define YT8511_DELAY_RX              BIT(0)
> > +
> > +/* TX Delay is bits 7:4, default 0x5
> > + * Delay = 150ps * N - 250ps, Default = 500ps
> > + */
> > +#define YT8511_DELAY_TX              (0x5 << 4)
>
> > +
> > +     switch (phydev->interface) {
> > +     case PHY_INTERFACE_MODE_RGMII:
> > +             val &= ~(YT8511_DELAY_RX | YT8511_DELAY_TX);
> > +             break;
>
> This is not correct. YT8511_DELAY_TX will only mask the 2 bits in 0x5,
> not all the bits in 7:4. And since the formula is
>
> Delay = 150ps * N - 250ps
>
> setting N to 0 is not what you want. You probably want N=2, so you end up with 50ps

The manufacturer's driver set this to <0> to disable, but I agree the
datasheet disagrees with this.

>
> > +     case PHY_INTERFACE_MODE_RGMII_ID:
> > +             val |= YT8511_DELAY_RX | YT8511_DELAY_TX;
> > +             break;
> > +     case PHY_INTERFACE_MODE_RGMII_RXID:
> > +             val &= ~(YT8511_DELAY_TX);
> > +             val |= YT8511_DELAY_RX;
>
> The delay should be around 2ns. For RX you only have 1.8ns, which is
> probably good enough. But for TX you have more flexibility. You are
> setting it to the default of 500ps which is too small. I would suggest
> 1.85ns, N=14, so it is the same as RX.

Makes sense, these are the insights I was hoping for!

>
> I also wonder about bits 15:12 of PHY EXT ODH: Delay and driver
> strength CFG register.

The default value *works*, and from an emi perspective we want the
lowest strength single that is reliable.
Unfortunately I don't have the equipment to *test* the actual output
strengths and the datasheet isn't explicitly clear about them either.
This is one of the values I was considering having defined in the devicetree.

>
>          Andrew