On Fri, May 14, 2021 at 9:24 AM Andrew Lunn [off-list ref] wrote:

On Fri, May 14, 2021 at 07:58:26AM -0400, Peter Geis wrote:

quoted

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.

quoted

+#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.

quoted

+/* 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)

quoted

+
+     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.

quoted

+     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