| Message ID | 20190426093010.9609-1-fancer.lancer@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: realtek: Add rtl8211e rx/tx delays config | expand |
On Fri, Apr 26, 2019 at 12:30:10PM +0300, Serge Semin wrote: > The hidden RGMII configs register utilization was found in the > rtl8211e U-boot driver: > https://elixir.bootlin.com/u-boot/v2019.01/source/drivers/net/phy/realtek.c#L99 > > It confirms that the register bits field must control the so called > configuration pins described in the table 12-13 of the official PHY > datasheet: > 8:6 = PHY Address > 5:4 = Auto-Negotiation > 3 = Interface Mode Select > 2 = RX Delay > 1 = TX Delay > 0 = SELRGV > +static int rtl8211e_config_init(struct phy_device *phydev) > +{ > + int ret, oldpage; > + u16 val = 0; > + > + ret = genphy_config_init(phydev); > + if (ret < 0) > + return ret; > + > + /* enable TX/RX delay for rgmii-* modes, otherwise disable it */ > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) > + val = RTL8211E_TX_DELAY | RTL8211E_RX_DELAY; > + else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) > + val = RTL8211E_TX_DELAY; > + else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) > + val = RTL8211E_RX_DELAY; Hi Serge You need to look for PHY_INTERFACE_MODE_RGMII and disable the bits. For any other value, and in particular PHY_INTERFACE_MODE_NA, you should leave the bits alone. As you found out, u-boot knows how to program these bits. There are probably boards out there which rely on u-boot doing this, and the PHY driver then not messing with the bits. The way to indicate it should not mess with the bits is to not have a phy-mode property in DT, which results in PHY_INTERFACE_MODE_NA. Andrew
On 26.04.2019 11:30, Serge Semin wrote: > There are two chip pins named TXDLY and RXDLY which actually addes > the 2ns delays to TXC and RXC for TXD/RXD latching. Alas it is only > documented info regarding the RGMII timing control configurations > the PHY provides. It turnes out the same settings can be setup > via MDIO registers hidden in the extension pages layout. > Particularly the extension page 0xa4 provides a register 0x1c, > which bits 1 and 2 control the described delays. They are used > to implemet the "rgmii-{id,rxid,txid}" phy-mode. > There are few typos in the commit message. > The hidden RGMII configs register utilization was found in the > rtl8211e U-boot driver: > https://elixir.bootlin.com/u-boot/v2019.01/source/drivers/net/phy/realtek.c#L99 > > There is also a freebsd discussion regarding this register: > https://reviews.freebsd.org/D13591 > > It confirms that the register bits field must control the so called > configuration pins described in the table 12-13 of the official PHY > datasheet: > 8:6 = PHY Address > 5:4 = Auto-Negotiation > 3 = Interface Mode Select > 2 = RX Delay > 1 = TX Delay > 0 = SELRGV > > Signed-off-by: Serge Semin <fancer.lancer@gmail.com> > --- > drivers/net/phy/realtek.c | 44 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c > index 10df52ccddfe..8776b94d91ed 100644 > --- a/drivers/net/phy/realtek.c > +++ b/drivers/net/phy/realtek.c > @@ -23,11 +23,14 @@ > > #define RTL821x_INSR 0x13 > > +#define RTL821x_EXT_PAGE_SELECT 0x1e > #define RTL821x_PAGE_SELECT 0x1f > > #define RTL8211F_INSR 0x1d > > #define RTL8211F_TX_DELAY BIT(8) > +#define RTL8211E_TX_DELAY BIT(1) > +#define RTL8211E_RX_DELAY BIT(2) > > #define RTL8201F_ISR 0x1e > #define RTL8201F_IER 0x13 > @@ -174,6 +177,46 @@ static int rtl8211f_config_init(struct phy_device *phydev) > return phy_modify_paged(phydev, 0xd08, 0x11, RTL8211F_TX_DELAY, val); > } > > +static int rtl8211e_config_init(struct phy_device *phydev) > +{ > + int ret, oldpage; > + u16 val = 0; > + > + ret = genphy_config_init(phydev); There's no need to call genphy_config_init(). > + if (ret < 0) > + return ret; > + > + /* enable TX/RX delay for rgmii-* modes, otherwise disable it */ > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) > + val = RTL8211E_TX_DELAY | RTL8211E_RX_DELAY; > + else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) > + val = RTL8211E_TX_DELAY; > + else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) > + val = RTL8211E_RX_DELAY; > + > + /* According to a sample driver there is a 0x1c config register on > + * a 0xa4 extension page (0x7) layout. It can be used to disable/enable > + * the RX/TX delays otherwise controlled by hardware strobes. 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). > + */ > + oldpage = phy_select_page(phydev, 0x7); > + if (oldpage < 0) > + goto err_restore_page; > + > + ret = phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4); > + if (!ret) I think this should be: if (ret) > + goto err_restore_page; > + > + ret = phy_modify(phydev, 0x1c, RTL8211E_TX_DELAY | RTL8211E_RX_DELAY, > + val); > + > +err_restore_page: > + return phy_restore_page(phydev, oldpage, ret); > +} > + > static int rtl8211b_suspend(struct phy_device *phydev) > { > phy_write(phydev, MII_MMD_DATA, BIT(9)); > @@ -257,6 +300,7 @@ static struct phy_driver realtek_drvs[] = { > PHY_ID_MATCH_EXACT(0x001cc915), > .name = "RTL8211E Gigabit Ethernet", > .features = PHY_GBIT_FEATURES, > + .config_init = &rtl8211e_config_init, > .ack_interrupt = &rtl821x_ack_interrupt, > .config_intr = &rtl8211e_config_intr, > .suspend = genphy_suspend, >
On Fri, Apr 26, 2019 at 03:28:21PM +0200, Andrew Lunn wrote: > On Fri, Apr 26, 2019 at 12:30:10PM +0300, Serge Semin wrote: > > The hidden RGMII configs register utilization was found in the > > rtl8211e U-boot driver: > > https://elixir.bootlin.com/u-boot/v2019.01/source/drivers/net/phy/realtek.c#L99 > > > > It confirms that the register bits field must control the so called > > configuration pins described in the table 12-13 of the official PHY > > datasheet: > > 8:6 = PHY Address > > 5:4 = Auto-Negotiation > > 3 = Interface Mode Select > > 2 = RX Delay > > 1 = TX Delay > > 0 = SELRGV > > > +static int rtl8211e_config_init(struct phy_device *phydev) > > +{ > > + int ret, oldpage; > > + u16 val = 0; > > + > > + ret = genphy_config_init(phydev); > > + if (ret < 0) > > + return ret; > > + > > + /* enable TX/RX delay for rgmii-* modes, otherwise disable it */ > > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) > > + val = RTL8211E_TX_DELAY | RTL8211E_RX_DELAY; > > + else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) > > + val = RTL8211E_TX_DELAY; > > + else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) > > + val = RTL8211E_RX_DELAY; > > Hi Serge > > You need to look for PHY_INTERFACE_MODE_RGMII and disable the bits. > For any other value, and in particular PHY_INTERFACE_MODE_NA, you > should leave the bits alone. > > As you found out, u-boot knows how to program these bits. There are > probably boards out there which rely on u-boot doing this, and the PHY > driver then not messing with the bits. The way to indicate it should > not mess with the bits is to not have a phy-mode property in DT, which > results in PHY_INTERFACE_MODE_NA. > Hello Andrew Thanks for the comment. I'll alter the code the way you said. The mode will be changed by the config_init-function only if the interface is selected to be rgmii-like (rgmii, rgmii-id, rgmii-txid and rgmii-rxid), otherwise it will be left as is. But I've got a question regarding this then. What about for instance rtl8211f phy config_init-method? It setups the delay config in any case, no matter whether interface is configured to be of rgmii or another mode. Is it correct to configure rtl8211e and rtl8211f differently? Especially seeing the U-boot driver also perform the rtl8211f phy configuration. -Sergey > Andrew
> Hello Andrew > > Thanks for the comment. I'll alter the code the way you said. The mode will > be changed by the config_init-function only if the interface is selected to be > rgmii-like (rgmii, rgmii-id, rgmii-txid and rgmii-rxid), otherwise it will > be left as is. > > But I've got a question regarding this then. What about for instance rtl8211f > phy config_init-method? It setups the delay config in any case, no matter > whether interface is configured to be of rgmii or another mode. Is it correct > to configure rtl8211e and rtl8211f differently? Especially seeing the U-boot > driver also perform the rtl8211f phy configuration. Hi Sergey It is not correct and rtl8211f should be changed. Please add a second patch for that. We got burnt recently with PHY drivers doing the wrong thing with respect to RGMII delays. We have learnt from that experience, but there is code in the kernel which still does things wrong. Andrew
On Fri, Apr 26, 2019 at 07:17:44PM +0200, Heiner Kallweit wrote: > On 26.04.2019 11:30, Serge Semin wrote: > > There are two chip pins named TXDLY and RXDLY which actually addes > > the 2ns delays to TXC and RXC for TXD/RXD latching. Alas it is only > > documented info regarding the RGMII timing control configurations > > the PHY provides. It turnes out the same settings can be setup > > via MDIO registers hidden in the extension pages layout. > > Particularly the extension page 0xa4 provides a register 0x1c, > > which bits 1 and 2 control the described delays. They are used > > to implemet the "rgmii-{id,rxid,txid}" phy-mode. > > > There are few typos in the commit message. > I'll fix them in v2. Thank you. > > The hidden RGMII configs register utilization was found in the > > rtl8211e U-boot driver: > > https://elixir.bootlin.com/u-boot/v2019.01/source/drivers/net/phy/realtek.c#L99 > > > > There is also a freebsd discussion regarding this register: > > https://reviews.freebsd.org/D13591 > > > > It confirms that the register bits field must control the so called > > configuration pins described in the table 12-13 of the official PHY > > datasheet: > > 8:6 = PHY Address > > 5:4 = Auto-Negotiation > > 3 = Interface Mode Select > > 2 = RX Delay > > 1 = TX Delay > > 0 = SELRGV > > > > Signed-off-by: Serge Semin <fancer.lancer@gmail.com> > > --- > > drivers/net/phy/realtek.c | 44 +++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 44 insertions(+) > > > > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c > > index 10df52ccddfe..8776b94d91ed 100644 > > --- a/drivers/net/phy/realtek.c > > +++ b/drivers/net/phy/realtek.c > > @@ -23,11 +23,14 @@ > > > > #define RTL821x_INSR 0x13 > > > > +#define RTL821x_EXT_PAGE_SELECT 0x1e > > #define RTL821x_PAGE_SELECT 0x1f > > > > #define RTL8211F_INSR 0x1d > > > > #define RTL8211F_TX_DELAY BIT(8) > > +#define RTL8211E_TX_DELAY BIT(1) > > +#define RTL8211E_RX_DELAY BIT(2) > > > > #define RTL8201F_ISR 0x1e > > #define RTL8201F_IER 0x13 > > @@ -174,6 +177,46 @@ static int rtl8211f_config_init(struct phy_device *phydev) > > return phy_modify_paged(phydev, 0xd08, 0x11, RTL8211F_TX_DELAY, val); > > } > > > > +static int rtl8211e_config_init(struct phy_device *phydev) > > +{ > > + int ret, oldpage; > > + u16 val = 0; > > + > > + ret = genphy_config_init(phydev); > > There's no need to call genphy_config_init(). > Right. I'll fix it in v2. It's working even with calling the function though. > > + if (ret < 0) > > + return ret; > > + > > + /* enable TX/RX delay for rgmii-* modes, otherwise disable it */ > > + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) > > + val = RTL8211E_TX_DELAY | RTL8211E_RX_DELAY; > > + else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) > > + val = RTL8211E_TX_DELAY; > > + else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) > > + val = RTL8211E_RX_DELAY; > > + > > + /* According to a sample driver there is a 0x1c config register on > > + * a 0xa4 extension page (0x7) layout. It can be used to disable/enable > > + * the RX/TX delays otherwise controlled by hardware strobes. 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). > > + */ > > + oldpage = phy_select_page(phydev, 0x7); > > + if (oldpage < 0) > > + goto err_restore_page; > > + > > + ret = phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4); > > + if (!ret) > > I think this should be: if (ret) > My goodness. What a shame on me to miss this! I ported this function from an older kernel, and apparently didn't do this accurate enough. Sorry for this awful bug. I'll fix it in the next patch revision. -Sergey > > + goto err_restore_page; > > + > > + ret = phy_modify(phydev, 0x1c, RTL8211E_TX_DELAY | RTL8211E_RX_DELAY, > > + val); > > + > > +err_restore_page: > > + return phy_restore_page(phydev, oldpage, ret); > > +} > > + > > static int rtl8211b_suspend(struct phy_device *phydev) > > { > > phy_write(phydev, MII_MMD_DATA, BIT(9)); > > @@ -257,6 +300,7 @@ static struct phy_driver realtek_drvs[] = { > > PHY_ID_MATCH_EXACT(0x001cc915), > > .name = "RTL8211E Gigabit Ethernet", > > .features = PHY_GBIT_FEATURES, > > + .config_init = &rtl8211e_config_init, > > .ack_interrupt = &rtl821x_ack_interrupt, > > .config_intr = &rtl8211e_config_intr, > > .suspend = genphy_suspend, > > >
On Fri, Apr 26, 2019 at 10:05:21PM +0200, Andrew Lunn wrote: > > Hello Andrew > > > > Thanks for the comment. I'll alter the code the way you said. The mode will > > be changed by the config_init-function only if the interface is selected to be > > rgmii-like (rgmii, rgmii-id, rgmii-txid and rgmii-rxid), otherwise it will > > be left as is. > > > > But I've got a question regarding this then. What about for instance rtl8211f > > phy config_init-method? It setups the delay config in any case, no matter > > whether interface is configured to be of rgmii or another mode. Is it correct > > to configure rtl8211e and rtl8211f differently? Especially seeing the U-boot > > driver also perform the rtl8211f phy configuration. > > Hi Sergey > > It is not correct and rtl8211f should be changed. Please add a second > patch for that. > > We got burnt recently with PHY drivers doing the wrong thing with > respect to RGMII delays. We have learnt from that experience, but > there is code in the kernel which still does things wrong. > > Andrew Ok. I'll send the updated patchset with rtl8211f_config_init fixed soon. -Sergey
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c index 10df52ccddfe..8776b94d91ed 100644 --- a/drivers/net/phy/realtek.c +++ b/drivers/net/phy/realtek.c @@ -23,11 +23,14 @@ #define RTL821x_INSR 0x13 +#define RTL821x_EXT_PAGE_SELECT 0x1e #define RTL821x_PAGE_SELECT 0x1f #define RTL8211F_INSR 0x1d #define RTL8211F_TX_DELAY BIT(8) +#define RTL8211E_TX_DELAY BIT(1) +#define RTL8211E_RX_DELAY BIT(2) #define RTL8201F_ISR 0x1e #define RTL8201F_IER 0x13 @@ -174,6 +177,46 @@ static int rtl8211f_config_init(struct phy_device *phydev) return phy_modify_paged(phydev, 0xd08, 0x11, RTL8211F_TX_DELAY, val); } +static int rtl8211e_config_init(struct phy_device *phydev) +{ + int ret, oldpage; + u16 val = 0; + + ret = genphy_config_init(phydev); + if (ret < 0) + return ret; + + /* enable TX/RX delay for rgmii-* modes, otherwise disable it */ + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) + val = RTL8211E_TX_DELAY | RTL8211E_RX_DELAY; + else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) + val = RTL8211E_TX_DELAY; + else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) + val = RTL8211E_RX_DELAY; + + /* According to a sample driver there is a 0x1c config register on + * a 0xa4 extension page (0x7) layout. It can be used to disable/enable + * the RX/TX delays otherwise controlled by hardware strobes. 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). + */ + oldpage = phy_select_page(phydev, 0x7); + if (oldpage < 0) + goto err_restore_page; + + ret = phy_write(phydev, RTL821x_EXT_PAGE_SELECT, 0xa4); + if (!ret) + goto err_restore_page; + + ret = phy_modify(phydev, 0x1c, RTL8211E_TX_DELAY | RTL8211E_RX_DELAY, + val); + +err_restore_page: + return phy_restore_page(phydev, oldpage, ret); +} + static int rtl8211b_suspend(struct phy_device *phydev) { phy_write(phydev, MII_MMD_DATA, BIT(9)); @@ -257,6 +300,7 @@ static struct phy_driver realtek_drvs[] = { PHY_ID_MATCH_EXACT(0x001cc915), .name = "RTL8211E Gigabit Ethernet", .features = PHY_GBIT_FEATURES, + .config_init = &rtl8211e_config_init, .ack_interrupt = &rtl821x_ack_interrupt, .config_intr = &rtl8211e_config_intr, .suspend = genphy_suspend,
There are two chip pins named TXDLY and RXDLY which actually addes the 2ns delays to TXC and RXC for TXD/RXD latching. Alas it is only documented info regarding the RGMII timing control configurations the PHY provides. It turnes out the same settings can be setup via MDIO registers hidden in the extension pages layout. Particularly the extension page 0xa4 provides a register 0x1c, which bits 1 and 2 control the described delays. They are used to implemet the "rgmii-{id,rxid,txid}" phy-mode. The hidden RGMII configs register utilization was found in the rtl8211e U-boot driver: https://elixir.bootlin.com/u-boot/v2019.01/source/drivers/net/phy/realtek.c#L99 There is also a freebsd discussion regarding this register: https://reviews.freebsd.org/D13591 It confirms that the register bits field must control the so called configuration pins described in the table 12-13 of the official PHY datasheet: 8:6 = PHY Address 5:4 = Auto-Negotiation 3 = Interface Mode Select 2 = RX Delay 1 = TX Delay 0 = SELRGV Signed-off-by: Serge Semin <fancer.lancer@gmail.com> --- drivers/net/phy/realtek.c | 44 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)