Re: [PATCH 3/4] net: phy: realtek: add disable RX delay hack for RTL8211E

[Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 04/21/2017 04:24 PM, Icenowy Zheng wrote:
> From: Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
> 
> 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.
> 
> Enable this hack when a certain device tree property is set.
> 
> 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 <icenowy-ymACFijhrKM@public.gmane.org>
> ---
>  drivers/net/phy/realtek.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index d820d00addf6..880022160cd2 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,38 @@ 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 (of_node &&
> +	    of_property_read_bool(of_node, "realtek,disable-rx-delay")) {
> +		/* All these magic numbers are retrieved from Pine64, and
> +		 * they're said to be originated from Realtek.
> +		 *
> +		 * The datasheet of RTL8211E didn't cover this ext page.
> +		 *
> +		 * Select extension page 0xa4 here.
> +		 */
> +		phy_write(phydev, RTL8211_PAGE_SELECT, RTL8211E_EXT_PAGE)
> +		phy_write(phydev, RTL8211E_EXT_PAGE_SELECT, 0xa4);
> +
> +		/* Write the magic number */
> +		phy_write(phydev, 0x1c, 0xb591);
> +
> +		/* Restore to default page 0 */
> +		phy_write(phydev, RTL8211_PAGE_SELECT, 0);
> +	}

The code itself is probably fine, but the way you are checking whether
the RX delay should be disabled is not. As mentioned in patch 2, using
the correct "phy-mode" property would translate in the proper
phydev->interface value here (presumably PHY_INTERFACE_MODE_RGMII_TXID)
is that you want here that would allow you to check whether the RX delay
should, or should not be disabled.

Thank you
-- 
Florian