Messages in this thread | | | From | Peter Geis <> | Date | Fri, 14 May 2021 10:14:27 -0400 | Subject | Re: [PATCH v3] net: phy: add driver for Motorcomm yt8511 phy |
| |
On Fri, May 14, 2021 at 9:24 AM Andrew Lunn <andrew@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
| |