Message ID | 20170822040400.12166-4-icenowy@aosc.io (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, Aug 22, 2017 at 12:03:59PM +0800, Icenowy Zheng wrote: > From: Icenowy Zheng <icenowy@aosc.xyz> > > Some RTL8211E chips have broken GbE function, which needs a hack to > fix. It's said that this fix will affect the performance on not-buggy > PHYs, so it should only be enabled on boards with the broken PHY. I would not call this a broken PHY. It is a question of board design. If you have shorter tracks, you need a delay. If you have long tracks, you don't. Hence the four RGMII modes, so you can select if the delay is needed or not, depending on your board design. > Currently only some Pine64+ boards are known to have this issue. Design feature. Please remove all reference to bug. > This hack is said to disable RX relay for RTL8211E according to Realtek. > So implement it as RGMII-TXID mode. Do we know that the TX lines do have a delay after making this change? We need to be careful here. We will get into a mess if this is actually RGMII not RGMII_TXID, and somebody designs a board which needs RGMII-TXID. > +#include <linux/of.h> > #include <linux/phy.h> > #include <linux/module.h> > +static int rtl8211e_config_init(struct phy_device *phydev) > +{ > + struct device *dev = &phydev->mdio.dev; > + struct device_node *of_node = dev->of_node; You don't appear to use of_node, nor dev. Please remove, along with the header file. Andrew
Hi Icenowy, [auto build test WARNING on net-next/master] [also build test WARNING on v4.13-rc6 next-20170823] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Icenowy-Zheng/Workaround-broken-RTL8211E-on-some-Pine64-boards/20170823-234405 config: x86_64-randconfig-b0-08240928 (attached as .config) compiler: gcc-4.4 (Debian 4.4.7-8) 4.4.7 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All warnings (new ones prefixed by >>): drivers/net/phy/realtek.c: In function 'rtl8211e_config_init': >> drivers/net/phy/realtek.c:130: warning: unused variable 'of_node' vim +/of_node +130 drivers/net/phy/realtek.c 126 127 static int rtl8211e_config_init(struct phy_device *phydev) 128 { 129 struct device *dev = &phydev->mdio.dev; > 130 struct device_node *of_node = dev->of_node; 131 int ret; 132 133 ret = genphy_config_init(phydev); 134 if (ret < 0) 135 return ret; 136 137 if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { 138 /* Disable the RX internal delay here. 139 * 140 * All the magic numbers are not documented on RTL8211E 141 * datasheet. They're said to be from Realtek by Pine64. 142 */ 143 phy_write(phydev, RTL8211_PAGE_SELECT, RTL8211E_EXT_PAGE); 144 phy_write(phydev, RTL8211E_EXT_PAGE_SELECT, 0xa4); 145 phy_write(phydev, 0x1c, 0xb591); 146 147 /* Restore to default page 0 */ 148 phy_write(phydev, RTL8211_PAGE_SELECT, 0); 149 } 150 151 return 0; 152 } 153 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
在 2017-08-22 21:39,Andrew Lunn 写道: > On Tue, Aug 22, 2017 at 12:03:59PM +0800, Icenowy Zheng wrote: >> From: Icenowy Zheng <icenowy@aosc.xyz> >> >> Some RTL8211E chips have broken GbE function, which needs a hack to >> fix. It's said that this fix will affect the performance on not-buggy >> PHYs, so it should only be enabled on boards with the broken PHY. > > I would not call this a broken PHY. It is a question of board > design. If you have shorter tracks, you need a delay. If you have long > tracks, you don't. Hence the four RGMII modes, so you can select if > the delay is needed or not, depending on your board design. > >> Currently only some Pine64+ boards are known to have this issue. > > Design feature. > > Please remove all reference to bug. > >> This hack is said to disable RX relay for RTL8211E according to >> Realtek. >> So implement it as RGMII-TXID mode. > > Do we know that the TX lines do have a delay after making this change? > We need to be careful here. We will get into a mess if this is > actually RGMII not RGMII_TXID, and somebody designs a board which > needs RGMII-TXID. In fact I'm not sure -- I'm even not sure that after making this change there's no TX delay. The change is totally not documented, so I choose to use a custom property in v1. But Florian required me to use RGMII-TXID instead. > >> +#include <linux/of.h> >> #include <linux/phy.h> >> #include <linux/module.h> > >> +static int rtl8211e_config_init(struct phy_device *phydev) >> +{ >> + struct device *dev = &phydev->mdio.dev; >> + struct device_node *of_node = dev->of_node; > > You don't appear to use of_node, nor dev. Please remove, along with > the header file. > > Andrew > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >Do we know that the TX lines do have a delay after making this change? > >We need to be careful here. We will get into a mess if this is > >actually RGMII not RGMII_TXID, and somebody designs a board which > >needs RGMII-TXID. > > In fact I'm not sure -- I'm even not sure that after making this change > there's no TX delay. The change is totally not documented, so I choose > to use a custom property in v1. Can you get in touch with Realtek and ask? Or can you get a hardware guy to do some measurements? Thanks Andrew
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c index d820d00addf6..8306b6abaaa8 100644 --- a/drivers/net/phy/realtek.c +++ b/drivers/net/phy/realtek.c @@ -13,6 +13,7 @@ * option) any later version. * */ +#include <linux/of.h> #include <linux/phy.h> #include <linux/module.h> @@ -26,6 +27,8 @@ #define RTL8211_PAGE_SELECT 0x1f #define RTL8211E_INER_LINK_STATUS 0x400 +#define RTL8211E_EXT_PAGE_SELECT 0x1e +#define RTL8211E_EXT_PAGE 0x7 #define RTL8211F_INER_LINK_STATUS 0x0010 #define RTL8211F_INSR 0x1d @@ -121,6 +124,33 @@ static int rtl8211f_config_init(struct phy_device *phydev) return 0; } +static int rtl8211e_config_init(struct phy_device *phydev) +{ + struct device *dev = &phydev->mdio.dev; + struct device_node *of_node = dev->of_node; + int ret; + + ret = genphy_config_init(phydev); + if (ret < 0) + return ret; + + if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) { + /* Disable the RX internal delay here. + * + * All the magic numbers are not documented on RTL8211E + * datasheet. They're said to be from Realtek by Pine64. + */ + phy_write(phydev, RTL8211_PAGE_SELECT, RTL8211E_EXT_PAGE); + phy_write(phydev, RTL8211E_EXT_PAGE_SELECT, 0xa4); + phy_write(phydev, 0x1c, 0xb591); + + /* Restore to default page 0 */ + phy_write(phydev, RTL8211_PAGE_SELECT, 0); + } + + return 0; +} + static struct phy_driver realtek_drvs[] = { { .phy_id = 0x00008201, @@ -159,6 +189,7 @@ static struct phy_driver realtek_drvs[] = { .features = PHY_GBIT_FEATURES, .flags = PHY_HAS_INTERRUPT, .config_aneg = &genphy_config_aneg, + .config_init = rtl8211e_config_init, .read_status = &genphy_read_status, .ack_interrupt = &rtl821x_ack_interrupt, .config_intr = &rtl8211e_config_intr,