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

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

[Willy Liu]

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

Fixes: f81dadbcf7fd ("net: phy: realtek: Add rtl8211e rx/tx delays config")
Signed-off-by: Willy Liu <willy.liu@realtek.com>
---
 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)
 
 #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
 	 */
 	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

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@realtek.com>
> ---
>  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
> 

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@gmail.com> 
Sent: Thursday, September 17, 2020 6:11 PM
To: 劉偉權 <willy.liu@realtek.com>
Cc: andrew@lunn.ch; hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net; kuba@kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Ryan Kao <ryankao@realtek.com>; Kyle Evans <kevans@FreeBSD.org>; Joe Hershberger <joe.hershberger@ni.com>; Peter Robinson <pbrobinson@gmail.com>
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@realtek.com>
> ---
>  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.

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

[Andrew Lunn]

On Fri, Sep 18, 2020 at 06:55:16AM +0000, 劉偉權 wrote:
> 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.

...

> >  	 * 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 itq
> 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
> stateq mapped well to what was available on the corresponding
> external pins.

Hi Willy

So it appears bits 3 to 8 have been reverse engineered. Unless you
know from your confidential datasheet that these are wrong, please
leave the comment alone.

If you confidential datasheet says that the usage of bits 0-2 is
wrong, then please do correct that part.

       Andrew

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

[Serge Semin]

Hello Andrew.

On Fri, Sep 18, 2020 at 03:54:03PM +0200, Andrew Lunn wrote:
> On Fri, Sep 18, 2020 at 06:55:16AM +0000, 劉偉權 wrote:
> > 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.
> 
> ...
> 
> > >  	 * 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 itq
> > 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
> > stateq mapped well to what was available on the corresponding
> > external pins.
> 
> Hi Willy
> 

> So it appears bits 3 to 8 have been reverse engineered. Unless you
> know from your confidential datasheet that these are wrong, please
> leave the comment alone.
> 
> If you confidential datasheet says that the usage of bits 0-2 is
> wrong, then please do correct that part.

I've got that info from Kyle post here:
https://reviews.freebsd.org/D13591

My work with that problem has been done more than a year ago, so I don't
remember all the details. But IIRC the very first nine bits [8:0] must be a copy
of what is set on the external configuration pins as I described in the comment.
AFAIR I tried to manually change these pins [1] and detected that change right
there in the register. That fully fitted to what Kyle wrote in his post. Alas I
can't repeat it to be completely sure at the moment due to the lack of the
hardware. So I might have missed something, and Willy' confirmation about that
would have been more than welcome. What we can say now for sure I didn't use
the magic number in my patch. That could have been a mistake from what Willy
says in the commit-log...

Anyway aside with the Magic bits settings (which by Willy' patch appears to be
the Tx/Rx delays + Force Tx/Rx delay setting) Kyle also clears the bits 1-2 with
a comment "Ensure both internal delays are turned off". Willy, what can you say
about that? Can we really leave them out from the change? Kyle, could you give
us a comment about your experience with that?

[1] RTL8211E-VB-CG, RTL8211E-VL-CG, RTL8211E-VL-CG, "INTEGRATED 10/100/1000M ETHERNET
    TRANSCEIVER" Datasheet, Table 13. Configuration Register Definition, Rev. 1.6,
    03 April 2012, Track ID: JATR-3375-16, p.16

-Sergey

> 
>        Andrew

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

[劉偉權]

Hi Andrew,
I removed below register layout descriptions because these descriptions did not match register definitions for rtl8211e extension page 164 reg 0x1c at all.
8:6 = PHY Address
5:4 = Auto-Negotiation
3 = Mode
2 = RXD
1 = TXD
0 = SELRGV1
I think it is a misunderstanding. These definitions are mapped from datasheet rtl8211e table13" Configuration Register Definition". However this table should be HW pin configurations not register descriptions. 

Users can config RXD/TXD via register setting(extension page 164 reg 0x1c). But bit map for these two settings should be below: 
13 = Force Tx RX Delay controlled by bit12 bit11,
12 = RX Delay, 11 = TX Delay

Hi Sergey,
I saw the summary from https://reviews.freebsd.org/D13591. This patch is to reconfigure the RTL8211E used to force off TXD/RXD (RXD is defaulting to on, in my checks) and turn on some bits in the configuration register for this PHY that are undocumented.
The default value for "extension pg 0xa4 reg 0x1c" is 0x8148, and bit1-2 should be 0. In my opinion, this patch should be worked based on the magic number (0xb400). It seems RX delay was set and package did not lost for Some pine64 models. I am not sure if some models got different default value(not 0x8148) because the summary says (RXD is defaulting to on). What I mean is that this patch is actually turn on RX Delay not turn off RX delay. I hope we can correct the meaning of this register. 

B.R.
Willy

-----Original Message-----
From: Serge Semin <fancer.lancer@gmail.com> 
Sent: Friday, September 18, 2020 11:33 PM
To: Andrew Lunn <andrew@lunn.ch>; Kyle Evans <kevans@FreeBSD.org>; 劉偉權 <willy.liu@realtek.com>
Cc: hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net; kuba@kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Ryan Kao <ryankao@realtek.com>; Joe Hershberger <joe.hershberger@ni.com>; Peter Robinson <pbrobinson@gmail.com>
Subject: Re: [PATCH] net: phy: realtek: fix rtl8211e rx/tx delay config

Hello Andrew.

On Fri, Sep 18, 2020 at 03:54:03PM +0200, Andrew Lunn wrote:
> On Fri, Sep 18, 2020 at 06:55:16AM +0000, 劉偉權 wrote:
> > 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.
> 
> ...
> 
> > >  	 * 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 itq 
> > 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 
> > stateq mapped well to what was available on the corresponding 
> > external pins.
> 
> Hi Willy
> 

> So it appears bits 3 to 8 have been reverse engineered. Unless you 
> know from your confidential datasheet that these are wrong, please 
> leave the comment alone.
> 
> If you confidential datasheet says that the usage of bits 0-2 is 
> wrong, then please do correct that part.

I've got that info from Kyle post here:
https://reviews.freebsd.org/D13591

My work with that problem has been done more than a year ago, so I don't remember all the details. But IIRC the very first nine bits [8:0] must be a copy of what is set on the external configuration pins as I described in the comment.
AFAIR I tried to manually change these pins [1] and detected that change right there in the register. That fully fitted to what Kyle wrote in his post. Alas I can't repeat it to be completely sure at the moment due to the lack of the hardware. So I might have missed something, and Willy' confirmation about that would have been more than welcome. What we can say now for sure I didn't use the magic number in my patch. That could have been a mistake from what Willy says in the commit-log...

Anyway aside with the Magic bits settings (which by Willy' patch appears to be the Tx/Rx delays + Force Tx/Rx delay setting) Kyle also clears the bits 1-2 with a comment "Ensure both internal delays are turned off". Willy, what can you say about that? Can we really leave them out from the change? Kyle, could you give us a comment about your experience with that?

[1] RTL8211E-VB-CG, RTL8211E-VL-CG, RTL8211E-VL-CG, "INTEGRATED 10/100/1000M ETHERNET
    TRANSCEIVER" Datasheet, Table 13. Configuration Register Definition, Rev. 1.6,
    03 April 2012, Track ID: JATR-3375-16, p.16

-Sergey

> 
>        Andrew

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

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

[Andrew Lunn]

On Mon, Sep 21, 2020 at 07:00:00AM +0000, 劉偉權 wrote:
> Hi Andrew,

> I removed below register layout descriptions because these
> descriptions did not match register definitions for rtl8211e
> extension page 164 reg 0x1c at all.
> 8:6 = PHY Address
> 5:4 = Auto-Negotiation
> 3 = Mode
> 2 = RXD
> 1 = TXD
> 0 = SELRGV1

> I think it is a misunderstanding. These definitions are mapped from
> datasheet rtl8211e table13" Configuration Register
> Definition". However this table should be HW pin configurations not
> register descriptions.

So these are just how the device is strapped. So lets add that to the
description, rather than remove it.

> Users can config RXD/TXD via register setting(extension page 164 reg
> 0x1c). But bit map for these two settings should be below:
> 13 = Force Tx RX Delay controlled by bit12 bit11,
> 12 = RX Delay, 11 = TX Delay

> Hi Sergey,

> I saw the summary from https://reviews.freebsd.org/D13591. This
> patch is to reconfigure the RTL8211E used to force off TXD/RXD (RXD
> is defaulting to on, in my checks) and turn on some bits in the
> configuration register for this PHY that are undocumented.

> The default value for "extension pg 0xa4 reg 0x1c" is 0x8148, and
> bit1-2 should be 0. In my opinion, this patch should be worked based
> on the magic number (0xb400).

Magic numbers are always bad. Please document what these bits mean.

      Andrew

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

[劉偉權]

Hi Andrew,
I summary table 12,13 from RTL8211E-VB datasheet as below.
[Table 12]
RTL8211E-VB Pin       Pin Name
LED0                 PHYAD[0]
LED1                 PHYAD[1]
RXCTL                PHYAD[2]
RXD2                 AN[0]
RXD3                 AN[1]
 -                    Mode
LED2                 RX Delay
RXD1                 TX Delay
RXD0                 SELRGV
To set the CONFIG pins, an external pull-high or pull-low resistor is required.
[Table 13]
PHYAD[2:0]: PHY Address
AN[1:0]: Auto-negotiation Configuration
Mode: Interface Mode select
RX Delay: RGMII Transmit clock timing control
    1: Add 2ns delay to RXC for RXD latching(via 4.7k-ohm to 3.3V)
    0: No delay(via 4.7k-ohm to GND)
TX Delay: RGMII Transmit clock timing control
    1: Add 2ns delay to TXC for TXD latching(via 4.7k-ohm to 3.3V)
    0: No delay(via 4.7k-ohm to GND)
SELRGV: 3.3V or 2.5V RGMII/GMII Selection

These two tables descript how to config it via external pull-high or pull-low resistor on PCB circuit.

Below patch gives table 13 another meaning and maps to register setting.
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=f81dadbcf7fd06
8:6 = PHY Address
5:4 = Auto-Negotiation
3 = Interface Mode Select
2 = RX Delay
1 = TX Delay
0 = SELRGV
Sync from https://reviews.freebsd.org/D13591

Should I add how to config RX/TX Delay via pull high/down resistor in patch description?

Willy

-----Original Message-----
From: Andrew Lunn <andrew@lunn.ch> 
Sent: Monday, September 21, 2020 11:13 PM
To: 劉偉權 <willy.liu@realtek.com>
Cc: Serge Semin <fancer.lancer@gmail.com>; Kyle Evans <kevans@FreeBSD.org>; hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net; kuba@kernel.org; netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Ryan Kao <ryankao@realtek.com>; Joe Hershberger <joe.hershberger@ni.com>; Peter Robinson <pbrobinson@gmail.com>
Subject: Re: [PATCH] net: phy: realtek: fix rtl8211e rx/tx delay config

On Mon, Sep 21, 2020 at 07:00:00AM +0000, 劉偉權 wrote:
> Hi Andrew,

> I removed below register layout descriptions because these 
> descriptions did not match register definitions for rtl8211e extension 
> page 164 reg 0x1c at all.
> 8:6 = PHY Address
> 5:4 = Auto-Negotiation
> 3 = Mode
> 2 = RXD
> 1 = TXD
> 0 = SELRGV1

> I think it is a misunderstanding. These definitions are mapped from 
> datasheet rtl8211e table13" Configuration Register Definition". 
> However this table should be HW pin configurations not register 
> descriptions.

So these are just how the device is strapped. So lets add that to the description, rather than remove it.

> Users can config RXD/TXD via register setting(extension page 164 reg 
> 0x1c). But bit map for these two settings should be below:
> 13 = Force Tx RX Delay controlled by bit12 bit11,
> 12 = RX Delay, 11 = TX Delay

> Hi Sergey,

> I saw the summary from https://reviews.freebsd.org/D13591. This patch 
> is to reconfigure the RTL8211E used to force off TXD/RXD (RXD is 
> defaulting to on, in my checks) and turn on some bits in the 
> configuration register for this PHY that are undocumented.

> The default value for "extension pg 0xa4 reg 0x1c" is 0x8148, and
> bit1-2 should be 0. In my opinion, this patch should be worked based 
> on the magic number (0xb400).

Magic numbers are always bad. Please document what these bits mean.

      Andrew

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