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

[Serge Semin]

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
>