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@xxxxxxx> 
Sent: Monday, September 21, 2020 11:13 PM
To: 劉偉權 <willy.liu@xxxxxxxxxxx>
Cc: Serge Semin <fancer.lancer@xxxxxxxxx>; Kyle Evans <kevans@xxxxxxxxxxx>; 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

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.