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

Heiner Kallweit
2019-04-26T17:17:59Z

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,
>