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