Re: [PATCH v2 3/4] net: phy: realtek: add disable RX internal delay 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. Can you get in touch with Realtek and ask? Or can you get a hardware guy to do some measurements? Thanks Andrew
Re: [PATCH v2 3/4] net: phy: realtek: add disable RX internal delay mode
在 2017-08-22 21:39，Andrew Lunn 写道：
On Tue, Aug 22, 2017 at 12:03:59PM +0800, Icenowy Zheng wrote:
From: Icenowy Zheng
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
#include
#include
+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-ker...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH v2 3/4] net: phy: realtek: add disable RX internal delay mode
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 infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
Re: [PATCH v2 3/4] net: phy: realtek: add disable RX internal delay mode
On Tue, Aug 22, 2017 at 12:03:59PM +0800, Icenowy Zheng wrote:
> From: Icenowy Zheng
>
> 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
> #include
> #include
> +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
[PATCH v2 3/4] net: phy: realtek: add disable RX internal delay mode
From: Icenowy Zheng
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.
Currently only some Pine64+ boards are known to have this issue.
This hack is said to disable RX relay for RTL8211E according to Realtek.
So implement it as RGMII-TXID mode.
As this hack is not documented on the datasheet at all, it contains
magic numbers, and could not be revealed. These magic numbers are
received from Realtek via Pine64.
Signed-off-by: Icenowy Zheng
---
Changes in v2:
- Used RGMII_TXID phy mode.
drivers/net/phy/realtek.c | 31 +++
1 file changed, 31 insertions(+)
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
#include
#include
@@ -26,6 +27,8 @@
#define RTL8211_PAGE_SELECT0x1f
#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 = 0x8201,
@@ -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,
--
2.13.5
