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

[劉偉權]

Hi Serge,
Thanks for your reply. There is a confidential issue that realtek doesn't offer the detail of a full register layout for configuration register. 

The default setting for this configuration register should be 0x8148. Basically, no need to change it. If you need to enable RGMII RX Delay or RGMII TX Delay via register setting, you also need to enable Force Tx RX Delay controlled as well.
13 = force Tx RX Delay controlled by bit12 bit11
12 = Tx Delay
11 = Rx Delay 

Current code in U-boot could change the register value from 0x8148 to 0xb548 (0x8148|0xb400). Bit12 and Bit13 are set, and  RGMII TX delay enabled.
https://elixir.bootlin.com/u-boot/v2020.10-rc4/source/drivers/net/phy/realtek.c
	
I hope this information could help a bit.
B.R.
Willy

-----Original Message-----
From: Serge Semin <fancer.lancer@xxxxxxxxx> 
Sent: Thursday, September 17, 2020 6:11 PM
To: 劉偉權 <willy.liu@xxxxxxxxxxx>
Cc: andrew@xxxxxxx; hkallweit1@xxxxxxxxx; linux@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Ryan Kao <ryankao@xxxxxxxxxxx>; Kyle Evans <kevans@xxxxxxxxxxx>; Joe Hershberger <joe.hershberger@xxxxxx>; Peter Robinson <pbrobinson@xxxxxxxxx>
Subject: Re: [PATCH] net: phy: realtek: fix rtl8211e rx/tx delay config

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
> 

------Please consider the environment before printing this e-mail.