Re: [PATCH] net: ethernet: fsl: don't en/disable refclk on open/close

From: Richard Leitner
Date: Sun Oct 22 2017 - 18:56:05 EST



On 10/22/2017 08:31 PM, Florian Fainelli wrote:

On 10/22/2017 06:11 AM, Richard Leitner wrote:
From: Richard Leitner <richard.leitner@xxxxxxxxxxx>

From: Richard Leitner <richard.leitner@xxxxxxxxxxx>

Some PHYs (for example the LAN8710) doesn't allow turning the ethernet
ref clocks off and on again without reset (according to their datasheet).
Exactly this behaviour was introduced for power saving reasons by
commit e8fcfcd5684a ("net: fec: optimize the clock management to save...")

Please don't truncate the commit subject, put it in full length.

Ok. So I can then ignore the checkpatch.pl error complaining about too long lines?


Therefore remove those en/disables of the refclk during probe, open and
close of the fec.

Generally speaking is this issue only be 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.MX6 SoC. The typical symptom of this problem is a "swinging" ethernet
link. Similar issues were experienced by users of the NXP forum:
https://community.nxp.com/thread/389902
https://community.nxp.com/message/309354
With this patch applied the issue didn't occur for at least a few
thousand PORs of our board.

FWIW, this is a common problem on most Ethernet systems attached to a
PHY because of the clock dependencies between the MAC and PHY, in either
direction really. We recently stumbled across a similar problem on the
MAC side (using bcmgenet) where after shutting down the PHY in
ndo_stop() we would not get enough clock cycles for the MAC to cleanly
shutdown and upon subsequent ndo_open() we could get the MAC in an
invalid state.

Hmm... So I'm not the only one struggling with this... Was there already discussion/ideas on how to solve this issue once and for all across the kernel?

Andrew Lunn suggested to make the PHY driver a clock driver and let it export the refclk... But IMHO that "feels" a bit strange. Due to the fact in our case the clock is generated by the SoC and the PHY is an external chip which only consumes it.

But back to this patch: Is it OK the way it fixes the issue?



Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to sa...")

Likewise here, not truncation of the commit subject necessary.

Cc: stable@xxxxxxxxxxxxxxx

netdev patches are handled directly by David Miller, see netdev-FAQ for
details.

Ok. So I shouldn't post them to stable@xxxxxxxxxxxxxxx at all?


Signed-off-by: Richard Leitner <richard.leitner@xxxxxxxxxxx>
---
drivers/net/ethernet/freescale/fec_main.c | 7 -------
1 file changed, 7 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 3dc2d771a222..8f696b53d8b8 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -2844,9 +2844,6 @@ fec_enet_open(struct net_device *ndev)
return ret;
pinctrl_pm_select_default_state(&fep->pdev->dev);
- ret = fec_enet_clk_enable(ndev, true);
- if (ret)
- goto clk_enable;
/* I should reset the ring buffers here, but I don't yet know
* a simple way to do that.
@@ -2879,8 +2876,6 @@ fec_enet_open(struct net_device *ndev)
err_enet_mii_probe:
fec_enet_free_buffers(ndev);
err_enet_alloc:
- fec_enet_clk_enable(ndev, false);
-clk_enable:
pm_runtime_mark_last_busy(&fep->pdev->dev);
pm_runtime_put_autosuspend(&fep->pdev->dev);
pinctrl_pm_select_sleep_state(&fep->pdev->dev);
@@ -2907,7 +2902,6 @@ fec_enet_close(struct net_device *ndev)
fec_enet_update_ethtool_stats(ndev);
- fec_enet_clk_enable(ndev, false);
pinctrl_pm_select_sleep_state(&fep->pdev->dev);
pm_runtime_mark_last_busy(&fep->pdev->dev);
pm_runtime_put_autosuspend(&fep->pdev->dev);
@@ -3495,7 +3489,6 @@ fec_probe(struct platform_device *pdev)
/* Carrier starts down, phylib will bring it up */
netif_carrier_off(ndev);
- fec_enet_clk_enable(ndev, false);
pinctrl_pm_select_sleep_state(&pdev->dev);
ret = register_netdev(ndev);