| Message ID | 20171205132600.13796-5-dev@g0hl1n.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: fec: fix refclk enable for SMSC LAN8710/20 | expand |
From: Richard Leitner <dev@g0hl1n.net> Sent: Tuesday, December 05, 2017 9:26 PM >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 add support for the phy_reset_after_clk_enable function from >phylib to mitigate this issue. > >Generally speaking this issue is only relevant if the ref clk for the PHY is >generated by the SoC and therefore the PHY is configured to "REF_CLK In >Mode". In our specific case (PCB) this problem does occur at about every 10th >to 50th POR of an LAN8710 connected to an i.MX6SOLO 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%7C7f9fee272fc44662c2a108d53be3d1ee%7C686ea1d3bc2b4c6 >fa92cd99c5c301635%7C0%7C0%7C636480772022331090&sdata=7RdUsoWVWu >o1nM5zKwLt7%2F6U3dxgDJtBDGlQCUWC6IM%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%7C7f9fee272fc44662c2a108d53be3d1ee%7C686ea1d3bc2b4 >c6fa92cd99c5c301635%7C0%7C0%7C636480772022331090&sdata=D56KilGWD3 >kLABxc0yOI%2B44Y%2FhLfrGtdAvupCEyvI%2BI%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@skidata.com> >--- > drivers/net/ethernet/freescale/fec_main.c | 7 +++++++ > 1 file changed, 7 insertions(+) > >diff --git a/drivers/net/ethernet/freescale/fec_main.c >b/drivers/net/ethernet/freescale/fec_main.c >index 610573855213..8c3d0fb7db20 100644 >--- a/drivers/net/ethernet/freescale/fec_main.c >+++ b/drivers/net/ethernet/freescale/fec_main.c >@@ -1862,6 +1862,8 @@ 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; >+ >+ phy_reset_after_clk_enable(ndev->phydev); > } else { > clk_disable_unprepare(fep->clk_ahb); > clk_disable_unprepare(fep->clk_enet_out); >@@ -2860,6 +2862,11 @@ fec_enet_open(struct net_device *ndev) > if (ret) > goto err_enet_mii_probe; > >+ /* reset phy if needed here, due to the fact this is the first time we >+ * have the net_device to phy_driver link >+ */ >+ phy_reset_after_clk_enable(ndev->phydev); >+ The patch series look better. But why does it need to reset phy here since phy already is hard reset after clock enable. > if (fep->quirks & FEC_QUIRK_ERR006687) > imx6q_cpuidle_fec_irqs_used(); > >-- >2.11.0
Hi Andy, On 12/06/2017 02:50 AM, Andy Duan wrote: > From: Richard Leitner <dev@g0hl1n.net> Sent: Tuesday, December 05, 2017 9:26 PM >> 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 add support for the phy_reset_after_clk_enable function from >> phylib to mitigate this issue. ... >> diff --git a/drivers/net/ethernet/freescale/fec_main.c >> b/drivers/net/ethernet/freescale/fec_main.c >> index 610573855213..8c3d0fb7db20 100644 >> --- a/drivers/net/ethernet/freescale/fec_main.c >> +++ b/drivers/net/ethernet/freescale/fec_main.c >> @@ -1862,6 +1862,8 @@ 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; >> + >> + phy_reset_after_clk_enable(ndev->phydev); >> } else { >> clk_disable_unprepare(fep->clk_ahb); >> clk_disable_unprepare(fep->clk_enet_out); >> @@ -2860,6 +2862,11 @@ fec_enet_open(struct net_device *ndev) >> if (ret) >> goto err_enet_mii_probe; >> >> + /* reset phy if needed here, due to the fact this is the first time we >> + * have the net_device to phy_driver link >> + */ >> + phy_reset_after_clk_enable(ndev->phydev); >> + > > The patch series look better. > But why does it need to reset phy here since phy already is hard reset after clock enable. The problem here is that in fec_enet_open() the fec_enet_clk_enable() call is done before the phy is probed. Therefore (as mentioned in the comment) there's no link from the net_device (ndev) to the phy_driver (which holds the flags). Any suggestions for a better solution are highly appreciated here! Thanks! regards;Richard.L
From: Richard Leitner <richard.leitner@skidata.com> Sent: Wednesday, December 06, 2017 4:12 PM >To: Andy Duan <fugang.duan@nxp.com>; Richard Leitner <dev@g0hl1n.net>; >robh+dt@kernel.org; mark.rutland@arm.com; andrew@lunn.ch; >f.fainelli@gmail.com; frowand.list@gmail.com >Cc: davem@davemloft.net; geert+renesas@glider.be; >sergei.shtylyov@cogentembedded.com; baruch@tkos.co.il; david.wu@rock- >chips.com; lukma@denx.de; netdev@vger.kernel.org; >devicetree@vger.kernel.org; linux-kernel@vger.kernel.org >Subject: Re: [PATCH net-next v3 4/4] net: fec: add >phy_reset_after_clk_enable() support > >Hi Andy, > >On 12/06/2017 02:50 AM, Andy Duan wrote: >> From: Richard Leitner <dev@g0hl1n.net> Sent: Tuesday, December 05, >> 2017 9:26 PM >>> 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 add support for the phy_reset_after_clk_enable function >>> from phylib to mitigate this issue. > >... > >>> diff --git a/drivers/net/ethernet/freescale/fec_main.c >>> b/drivers/net/ethernet/freescale/fec_main.c >>> index 610573855213..8c3d0fb7db20 100644 >>> --- a/drivers/net/ethernet/freescale/fec_main.c >>> +++ b/drivers/net/ethernet/freescale/fec_main.c >>> @@ -1862,6 +1862,8 @@ 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; >>> + >>> + phy_reset_after_clk_enable(ndev->phydev); >>> } else { >>> clk_disable_unprepare(fep->clk_ahb); >>> clk_disable_unprepare(fep->clk_enet_out); >>> @@ -2860,6 +2862,11 @@ fec_enet_open(struct net_device *ndev) >>> if (ret) >>> goto err_enet_mii_probe; >>> >>> + /* reset phy if needed here, due to the fact this is the first time we >>> + * have the net_device to phy_driver link >>> + */ >>> + phy_reset_after_clk_enable(ndev->phydev); >>> + >> >> The patch series look better. >> But why does it need to reset phy here since phy already is hard reset after >clock enable. > >The problem here is that in fec_enet_open() the fec_enet_clk_enable() call is >done before the phy is probed. Therefore (as mentioned in the >comment) there's no link from the net_device (ndev) to the phy_driver >(which holds the flags). > >Any suggestions for a better solution are highly appreciated here! Thanks! > >regards;Richard.L Okay, I see. For the patch: Acked-by: Fugang Duan <fugang.duan@nxp.com>
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 610573855213..8c3d0fb7db20 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -1862,6 +1862,8 @@ 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; + + phy_reset_after_clk_enable(ndev->phydev); } else { clk_disable_unprepare(fep->clk_ahb); clk_disable_unprepare(fep->clk_enet_out); @@ -2860,6 +2862,11 @@ fec_enet_open(struct net_device *ndev) if (ret) goto err_enet_mii_probe; + /* reset phy if needed here, due to the fact this is the first time we + * have the net_device to phy_driver link + */ + phy_reset_after_clk_enable(ndev->phydev); + if (fep->quirks & FEC_QUIRK_ERR006687) imx6q_cpuidle_fec_irqs_used();