Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 14 May 2021 15:24:43 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     Peter Geis <pgwipeout@gmail.com>
Cc:     Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, linux-rockchip@lists.infradead.org
Subject: Re: [PATCH v3] net: phy: add driver for Motorcomm yt8511 phy
Message-ID: <YJ56G23e930pg4Iv@lunn.ch>
References: <20210514115826.3025223-1-pgwipeout@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210514115826.3025223-1-pgwipeout@gmail.com>
Precedence: bulk

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

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

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

	 Andrew