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@xxxxxxxxx> 
Sent: Friday, September 18, 2020 11:33 PM
To: Andrew Lunn <andrew@xxxxxxx>; Kyle Evans <kevans@xxxxxxxxxxx>; 劉偉權 <willy.liu@xxxxxxxxxxx>
Cc: hkallweit1@xxxxxxxxx; linux@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Ryan Kao <ryankao@xxxxxxxxxxx>; Joe Hershberger <joe.hershberger@xxxxxx>; Peter Robinson <pbrobinson@xxxxxxxxx>
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.