RE: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC LAN8710/20

[Andy Duan]

From: Richard Leitner <dev@xxxxxxxxxx> Sent: Monday, November 20, 2017 4:34 PM
>To: f.fainelli@xxxxxxxxx; Andy Duan <fugang.duan@xxxxxxx>;
>andrew@xxxxxxx
>Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>richard.leitner@xxxxxxxxxxx
>Subject: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC
>LAN8710/20
>
>From: Richard Leitner <richard.leitner@xxxxxxxxxxx>
>
>Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow turning
>the refclk on and off again during operation (according to their datasheet).
>Nonetheless exactly this behaviour was introduced for power saving reasons
>by commit e8fcfcd5684a ("net: fec: optimize the clock management to save
>power").
>Therefore after enabling the refclk we detect if an affected PHY is attached. If
>so reset and initialize it again.
>
>For a better understanding here's a outline of the time response of the clock
>and reset lines before and after this patch:
>
>			  v--fec_probe()              v--fec_enet_open()
>			  v                           v
>	w/o patch eCLK:
>___||||||||____________________|||||||||||||||||
>	w/o patch nRST: ----__------------------------------------------
>	w/o patch CONF:
>_______XX_______________________________________
>
>	w/  patch eCLK:
>___||||||||____________________|||||||||||||||||
>	w/  patch nRST: ----__-----------------------------__-----------
>	w/  patch CONF:
>_______XX_____________________________XX________
>			  ^                           ^
>			  ^--fec_probe()              ^--fec_enet_open()
>
>Generally speaking this issue is only relevant if the ref clk for the PHY is
>generated by the SoC. In our specific case (PCB) this problem does occur at
>about every 10th to 50th POR of an LAN8710 connected to an i.MX6DL SoC.
>The typical symptom of this problem is a "swinging"
>ethernet link. Similar issues were reported by users of the NXP forum:
>	https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F
>%2Fcommunity.nxp.com%2Fthread%2F389902&data=02%7C01%7Cfugang.du
>an%40nxp.com%7C507c7aafbd4744f81ebf08d52ff1acff%7C686ea1d3bc2b4c6f
>a92cd99c5c301635%7C0%7C0%7C636467637399145483&sdata=RNXVGpPrlrcyL
>0SoQl8%2BI0k8Oc8BM0Iwykd1O%2Bjmvcc%3D&reserved=0
>	https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F
>%2Fcommunity.nxp.com%2Fmessage%2F309354&data=02%7C01%7Cfugang.d
>uan%40nxp.com%7C507c7aafbd4744f81ebf08d52ff1acff%7C686ea1d3bc2b4c6
>fa92cd99c5c301635%7C0%7C0%7C636467637399145483&sdata=pjeJEZGuBpb9
>uCMKGr70qa%2FmsNoak6v3nCID2vbNAeg%3D&reserved=0
>With this patch applied the issue didn't occur for at least a few hundret PORs
>of our board.
>
>Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to save
>power")
>Signed-off-by: Richard Leitner <richard.leitner@xxxxxxxxxxx>
>---
> drivers/net/ethernet/freescale/fec_main.c | 37
>+++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
>diff --git a/drivers/net/ethernet/freescale/fec_main.c
>b/drivers/net/ethernet/freescale/fec_main.c
>index 06a7caca0cee..52ec9b29a70e 100644
>--- a/drivers/net/ethernet/freescale/fec_main.c
>+++ b/drivers/net/ethernet/freescale/fec_main.c
>@@ -68,6 +68,7 @@
>
> static void set_multicast_list(struct net_device *ndev);  static void
>fec_enet_itr_coal_init(struct net_device *ndev);
>+static int fec_reset_phy(struct net_device *ndev);
>
> #define DRIVER_NAME	"fec"
>
>@@ -1833,6 +1834,32 @@ static int fec_enet_mdio_write(struct mii_bus *bus,
>int mii_id, int regnum,
> 	return ret;
> }
>
>+static int fec_enet_clk_ref_enable_reset_phy_quirk(struct net_device
>+*ndev) {
>+	struct phy_device *phy_dev = ndev->phydev;
>+	u32 real_phy_id;
>+	int ret;
>+
>+	/* some PHYs need a reset after the refclk was enabled, so we
>+	 * reset them here
>+	 */
>+	if (!phy_dev)
>+		return 0;
>+	if (!phy_dev->drv)
>+		return 0;
>+	real_phy_id = phy_dev->drv->phy_id & phy_dev->drv->phy_id_mask;
>+	switch (real_phy_id) {
>+	case 0x0007c0f0: /* SMSC LAN8710/LAN8720 */

Don't hard code here...
I believe there have many other phys also do such operation, hardcode is unacceptable...

And these code can be put into phy_device.c as common interface.

>+		ret = fec_reset_phy(ndev);
>+		if (ret)
>+			return ret;
>+		ret = phy_init_hw(phy_dev);
>+		if (ret)
>+			return ret;
>+	}
>+	return 0;
>+}
>+
> static int fec_enet_clk_enable(struct net_device *ndev, bool enable)  {
> 	struct fec_enet_private *fep = netdev_priv(ndev); @@ -1862,6
>+1889,10 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool
>enable)
> 		ret = clk_prepare_enable(fep->clk_ref);
> 		if (ret)
> 			goto failed_clk_ref;
>+
>+		ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev);
>+		if (ret)
>+			netdev_warn(ndev, "Resetting PHY failed, connection
>may be
>+unstable\n");
> 	} else {
> 		clk_disable_unprepare(fep->clk_ahb);
> 		clk_disable_unprepare(fep->clk_enet_out);
>@@ -2860,11 +2891,17 @@ fec_enet_open(struct net_device *ndev)
> 	if (ret)
> 		goto err_enet_mii_probe;
>
>+	/* as the PHY is connected now, trigger the reset quirk again */
>+	ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev);
>+	if (ret)
>+		netdev_warn(ndev, "Resetting PHY failed, connection may be
>+unstable\n");
>+
> 	if (fep->quirks & FEC_QUIRK_ERR006687)
> 		imx6q_cpuidle_fec_irqs_used();
>
> 	napi_enable(&fep->napi);
> 	phy_start(ndev->phydev);
>+

No need blank line here...
> 	netif_tx_start_all_queues(ndev);
>
> 	device_set_wakeup_enable(&ndev->dev, fep->wol_flag &
>--
>2.11.0