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

[Florian Fainelli <f.fainelli@gmail.com>]

On 10/22/2017 06:11 AM, Richard Leitner wrote:
> From: Richard Leitner <richard.leitner@skidata.com>
> 
> From: Richard Leitner <richard.leitner@skidata.com>
> 
> 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.

> 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.

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

Likewise here, not truncation of the commit subject necessary.

> Cc: stable@vger.kernel.org

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

> Signed-off-by: Richard Leitner <richard.leitner@skidata.com>
> ---
>  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);
> 

-- 
Florian