Re: [PATCH] net: phy: realtek: fix rtl8211e rx/tx delay config

From: Serge Semin
Date: Thu Sep 17 2020 - 06:10:48 EST


Hello Willy,
Thanks for the patch. My comments are below.

I've Cc'ed the U-boot/FreeBSD, who might be also interested in the solution
you've provided.

On Thu, Sep 17, 2020 at 09:47:33AM +0800, Willy Liu wrote:
> RGMII RX Delay and TX Delay settings will not applied if Force TX RX Delay
> Control bit is not set.
> Register bit for configuration pins:
> 13 = force Tx RX Delay controlled by bit12 bit11
> 12 = Tx Delay
> 11 = Rx Delay

This is a very useful information, but it contradicts a bit to what knowledge
we've currently got about that magical register. Current code in U-boot does
the delays configuration by means of another bits:
https://elixir.bootlin.com/u-boot/v2020.10-rc4/source/drivers/net/phy/realtek.c

Could you provide a full register layout, so we'd know for sure what that
register really does and finally close the question for good?

>
> Fixes: f81dadbcf7fd ("net: phy: realtek: Add rtl8211e rx/tx delays config")
> Signed-off-by: Willy Liu <willy.liu@xxxxxxxxxxx>
> ---
> drivers/net/phy/realtek.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
> mode change 100644 => 100755 drivers/net/phy/realtek.c
>
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> old mode 100644
> new mode 100755
> index 95dbe5e..3fddd57
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -32,9 +32,9 @@
> #define RTL8211F_TX_DELAY BIT(8)
> #define RTL8211F_RX_DELAY BIT(3)
>

> -#define RTL8211E_TX_DELAY BIT(1)
> -#define RTL8211E_RX_DELAY BIT(2)
> -#define RTL8211E_MODE_MII_GMII BIT(3)
> +#define RTL8211E_CTRL_DELAY BIT(13)
> +#define RTL8211E_TX_DELAY BIT(12)
> +#define RTL8211E_RX_DELAY BIT(11)

So, what do BIT(1) and BIT(2) control then? Could you explain?

>
> #define RTL8201F_ISR 0x1e
> #define RTL8201F_IER 0x13
> @@ -249,13 +249,13 @@ static int rtl8211e_config_init(struct phy_device *phydev)
> val = 0;
> break;
> case PHY_INTERFACE_MODE_RGMII_ID:
> - val = RTL8211E_TX_DELAY | RTL8211E_RX_DELAY;
> + val = RTL8211E_CTRL_DELAY | RTL8211E_TX_DELAY | RTL8211E_RX_DELAY;
> break;
> case PHY_INTERFACE_MODE_RGMII_RXID:
> - val = RTL8211E_RX_DELAY;
> + val = RTL8211E_CTRL_DELAY | RTL8211E_RX_DELAY;
> break;
> case PHY_INTERFACE_MODE_RGMII_TXID:
> - val = RTL8211E_TX_DELAY;
> + val = RTL8211E_CTRL_DELAY | RTL8211E_TX_DELAY;
> break;
> default: /* the rest of the modes imply leaving delays as is. */
> return 0;
> @@ -265,9 +265,8 @@ static int rtl8211e_config_init(struct phy_device *phydev)
> * 0xa4 extension page (0x7) layout. It can be used to disable/enable
> * the RX/TX delays otherwise controlled by RXDLY/TXDLY pins. 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).
> + * 13 = Force Tx RX Delay controlled by bit12 bit11,
> + * 12 = RX Delay, 11 = TX Delay

Here you've removed the register layout description and replaced it with just three
bits info. So from now the text above doesn't really corresponds to what follows.

I might have forgotten something, but AFAIR that register bits state mapped
well to what was available on the corresponding external pins. So if you've got
a sacred knowledge what configs are really hidden behind that register, please
open it up. This in-code comment would be a good place to provide the full
register description.

-Sergey

> */
> oldpage = phy_select_page(phydev, 0x7);
> if (oldpage < 0)
> @@ -277,7 +276,8 @@ static int rtl8211e_config_init(struct phy_device *phydev)
> if (ret)
> goto err_restore_page;
>
> - ret = __phy_modify(phydev, 0x1c, RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
> + ret = __phy_modify(phydev, 0x1c, RTL8211E_CTRL_DELAY
> + | RTL8211E_TX_DELAY | RTL8211E_RX_DELAY,
> val);
>
> err_restore_page:
> --
> 1.9.1
>