Re: [PATCH] net: phy: realtek: Add rtl8211e rx/tx delays config

[Heiner Kallweit <hkallweit1@gmail.com>]

On 26.04.2019 11:30, Serge Semin wrote:
> There are two chip pins named TXDLY and RXDLY which actually addes
> the 2ns delays to TXC and RXC for TXD/RXD latching. Alas it is only
> documented info regarding the RGMII timing control configurations
> the PHY provides. It turnes out the same settings can be setup
> via MDIO registers hidden in the extension pages layout.
> Particularly the extension page 0xa4 provides a register 0x1c,
> which bits 1 and 2 control the described delays. They are used
> to implemet the "rgmii-{id,rxid,txid}" phy-mode.
> 
There are few typos in the commit message.

> The hidden RGMII configs register utilization was found in the
> rtl8211e U-boot driver:
> https://elixir.bootlin.com/u-boot/v2019.01/source/drivers/net/phy/realtek.c#L99
> 
> There is also a freebsd discussion regarding this register:
> https://reviews.freebsd.org/D13591
> 
> It confirms that the register bits field must control the so called
> configuration pins described in the table 12-13 of the official PHY
> datasheet:
> 8:6 = PHY Address
> 5:4 = Auto-Negotiation
> 3 = Interface Mode Select
> 2 = RX Delay
> 1 = TX Delay
> 0 = SELRGV
> 
> Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/net/phy/realtek.c | 44 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 10df52ccddfe..8776b94d91ed 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -23,11 +23,14 @@
>  
>  #define RTL821x_INSR				0x13
>  
> +#define RTL821x_EXT_PAGE_SELECT			0x1e
>  #define RTL821x_PAGE_SELECT			0x1f
>  
>  #define RTL8211F_INSR				0x1d
>  
>  #define RTL8211F_TX_DELAY			BIT(8)
> +#define RTL8211E_TX_DELAY			BIT(1)
> +#define RTL8211E_RX_DELAY			BIT(2)
>  
>  #define RTL8201F_ISR				0x1e
>  #define RTL8201F_IER				0x13
> @@ -174,6 +177,46 @@ static int rtl8211f_config_init(struct phy_device *phydev)
>  	return phy_modify_paged(phydev, 0xd08, 0x11, RTL8211F_TX_DELAY, val);
>  }
>  
> +static int rtl8211e_config_init(struct phy_device *phydev)
> +{
> +	int ret, oldpage;
> +	u16 val = 0;
> +
> +	ret = genphy_config_init(phydev);

There's no need to call genphy_config_init().

> +	if (ret < 0)
> +		return ret;
> +
> +	/* enable TX/RX delay for rgmii-* modes, otherwise disable it */
> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
> +		val = RTL8211E_TX_DELAY | RTL8211E_RX_DELAY;
> +	else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)
> +		val = RTL8211E_TX_DELAY;
> +	else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID)
> +		val = RTL8211E_RX_DELAY;
> +
> +	/* According to a sample driver there is a 0x1c config register on
> +	 * a 0xa4 extension page (0x7) layout. It can be used to disable/enable
> +	 * the RX/TX delays otherwise controlled by hardware strobes. It can
> +	 * also be used to customize the whole configuration register:
> +	 * 8:6 = PHY Address, 5:4 = Auto-Negotiation, 3 = Interface Mode Select,
> +	 * 2 = RX Delay, 1 = TX Delay, 0 = SELRGV (see original PHY datasheet
> +	 * for details).
> +	 */
> +	oldpage = phy_select_page(phydev, 0x7);
> +	if (oldpage < 0)
> +		goto err_restore_page;
> +
> +	ret = phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4);
> +	if (!ret)

I think this should be: if (ret)

> +		goto err_restore_page;
> +
> +	ret = phy_modify(phydev, 0x1c, RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
> +			 val);
> +
> +err_restore_page:
> +	return phy_restore_page(phydev, oldpage, ret);
> +}
> +
>  static int rtl8211b_suspend(struct phy_device *phydev)
>  {
>  	phy_write(phydev, MII_MMD_DATA, BIT(9));
> @@ -257,6 +300,7 @@ static struct phy_driver realtek_drvs[] = {
>  		PHY_ID_MATCH_EXACT(0x001cc915),
>  		.name		= "RTL8211E Gigabit Ethernet",
>  		.features	= PHY_GBIT_FEATURES,
> +		.config_init	= &rtl8211e_config_init,
>  		.ack_interrupt	= &rtl821x_ack_interrupt,
>  		.config_intr	= &rtl8211e_config_intr,
>  		.suspend	= genphy_suspend,
>