[PATCHv2 0/2] net: fec: Reset ethernet PHY whenever the enet_out clock is being enabled

[PATCHv2 0/2] net: fec: Reset ethernet PHY whenever the enet_out clock is being enabled

[Lothar Waßmann]

This patchset fixes a regression introduced by
commit e8fcfcd5684a ("net: fec: optimize the clock management to save power")
for ethernet PHYs that are using ENET_OUT as reference clock (on i.MX6 or i.MX28)

Changes vs. v1:
- fixed reference to the commit that introduced the regression.
- dropped patch to use gpiod framework. This should be added later,
  after the affected DTBs have been updated to specify the correct
  gpio_flags.

Patch overview:
1. cleanup patch to remove redundant NULL checks
2. call fec_reset_phy() after the ENET_OUT clock has been enabled


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCHv2 1/2] remove redundant struct clk NULL checks

[Lothar Waßmann]

NULL is a valid argument to clk_enable()/clk_disable(). Remove
redundant checks before calling those functions.

Signed-off-by: Lothar Waßmann <LW-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
---
 drivers/net/ethernet/freescale/fec_main.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 502da6f..592a7a4 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1874,35 +1874,30 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
 			}
 			mutex_unlock(&fep->ptp_clk_mutex);
 		}
-		if (fep->clk_ref) {
-			ret = clk_prepare_enable(fep->clk_ref);
-			if (ret)
-				goto failed_clk_ref;
-		}
+
+		ret = clk_prepare_enable(fep->clk_ref);
+		if (ret)
+			goto failed_clk_ref;
 	} else {
 		clk_disable_unprepare(fep->clk_ahb);
-		if (fep->clk_enet_out)
-			clk_disable_unprepare(fep->clk_enet_out);
+		clk_disable_unprepare(fep->clk_enet_out);
 		if (fep->clk_ptp) {
 			mutex_lock(&fep->ptp_clk_mutex);
 			clk_disable_unprepare(fep->clk_ptp);
 			fep->ptp_clk_on = false;
 			mutex_unlock(&fep->ptp_clk_mutex);
 		}
-		if (fep->clk_ref)
-			clk_disable_unprepare(fep->clk_ref);
+		clk_disable_unprepare(fep->clk_ref);
 	}
 
 	return 0;
 
 failed_clk_ref:
-	if (fep->clk_ref)
-		clk_disable_unprepare(fep->clk_ref);
+	clk_disable_unprepare(fep->clk_ref);
 failed_clk_ptp:
-	if (fep->clk_enet_out)
-		clk_disable_unprepare(fep->clk_enet_out);
+	clk_disable_unprepare(fep->clk_enet_out);
 failed_clk_enet_out:
-		clk_disable_unprepare(fep->clk_ahb);
+	clk_disable_unprepare(fep->clk_ahb);
 
 	return ret;
 }
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 1/2] remove redundant struct clk NULL checks

[Lothar Waßmann]

NULL is a valid argument to clk_enable()/clk_disable(). Remove
redundant checks before calling those functions.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/net/ethernet/freescale/fec_main.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 502da6f..592a7a4 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1874,35 +1874,30 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
 			}
 			mutex_unlock(&fep->ptp_clk_mutex);
 		}
-		if (fep->clk_ref) {
-			ret = clk_prepare_enable(fep->clk_ref);
-			if (ret)
-				goto failed_clk_ref;
-		}
+
+		ret = clk_prepare_enable(fep->clk_ref);
+		if (ret)
+			goto failed_clk_ref;
 	} else {
 		clk_disable_unprepare(fep->clk_ahb);
-		if (fep->clk_enet_out)
-			clk_disable_unprepare(fep->clk_enet_out);
+		clk_disable_unprepare(fep->clk_enet_out);
 		if (fep->clk_ptp) {
 			mutex_lock(&fep->ptp_clk_mutex);
 			clk_disable_unprepare(fep->clk_ptp);
 			fep->ptp_clk_on = false;
 			mutex_unlock(&fep->ptp_clk_mutex);
 		}
-		if (fep->clk_ref)
-			clk_disable_unprepare(fep->clk_ref);
+		clk_disable_unprepare(fep->clk_ref);
 	}
 
 	return 0;
 
 failed_clk_ref:
-	if (fep->clk_ref)
-		clk_disable_unprepare(fep->clk_ref);
+	clk_disable_unprepare(fep->clk_ref);
 failed_clk_ptp:
-	if (fep->clk_enet_out)
-		clk_disable_unprepare(fep->clk_enet_out);
+	clk_disable_unprepare(fep->clk_enet_out);
 failed_clk_enet_out:
-		clk_disable_unprepare(fep->clk_ahb);
+	clk_disable_unprepare(fep->clk_ahb);
 
 	return ret;
 }
-- 
2.1.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCHv2 2/2] net: fec: Reset ethernet PHY whenever the enet_out clock is being enabled

[Lothar Waßmann]

If a PHY uses ENET_OUT as reference clock, it may need a RESET to get
functional after the clock had been disabled.

Failure to do this results in the link state constantly toggling
between up and down:
fec 02188000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
fec 02188000.ethernet eth0: Link is Down
fec 02188000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
fec 02188000.ethernet eth0: Link is Down
[...]

Signed-off-by: Lothar Waßmann <LW-bxm8fMRDkQLDiMYJYoSAnRvVK+yQ3ZXh@public.gmane.org>
---
 drivers/net/ethernet/freescale/fec.h      |  1 +
 drivers/net/ethernet/freescale/fec_main.c | 38 +++++++++++++++++++++++++------
 2 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 99d33e2..33228df 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -519,6 +519,7 @@ struct fec_enet_private {
 	int	pause_flag;
 	int	wol_flag;
 	u32	quirks;
+	int	phy_reset_gpio;
 
 	struct	napi_struct napi;
 	int	csum_flags;
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 592a7a4..fb0b0eb 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -67,6 +67,7 @@
 
 static void set_multicast_list(struct net_device *ndev);
 static void fec_enet_itr_coal_init(struct net_device *ndev);
+static void fec_reset_phy(struct platform_device *pdev);
 
 #define DRIVER_NAME	"fec"
 
@@ -1862,6 +1863,8 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
 			ret = clk_prepare_enable(fep->clk_enet_out);
 			if (ret)
 				goto failed_clk_enet_out;
+
+			fec_reset_phy(fep->pdev);
 		}
 		if (fep->clk_ptp) {
 			mutex_lock(&fep->ptp_clk_mutex);
@@ -3216,11 +3219,12 @@ static int fec_enet_init(struct net_device *ndev)
 #ifdef CONFIG_OF
 static void fec_reset_phy(struct platform_device *pdev)
 {
-	int err, phy_reset;
-	int msec = 1;
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct fec_enet_private *fep = netdev_priv(ndev);
 	struct device_node *np = pdev->dev.of_node;
+	int msec = 1;
 
-	if (!np)
+	if (!gpio_is_valid(fep->phy_reset_gpio))
 		return;
 
 	of_property_read_u32(np, "phy-reset-duration", &msec);
@@ -3228,18 +3232,27 @@ static void fec_reset_phy(struct platform_device *pdev)
 	if (msec > 1000)
 		msec = 1;
 
+	gpio_set_value_cansleep(fep->phy_reset_gpio, 0);
+	msleep(msec);
+	gpio_set_value_cansleep(fep->phy_reset_gpio, 1);
+}
+
+static int fec_get_reset_gpio(struct platform_device *pdev)
+{
+	int err, phy_reset;
+	struct device_node *np = pdev->dev.of_node;
+
 	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
 	if (!gpio_is_valid(phy_reset))
-		return;
+		return phy_reset;
 
 	err = devm_gpio_request_one(&pdev->dev, phy_reset,
 				    GPIOF_OUT_INIT_LOW, "phy-reset");
 	if (err) {
 		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err);
-		return;
+		return err;
 	}
-	msleep(msec);
-	gpio_set_value_cansleep(phy_reset, 1);
+	return phy_reset;
 }
 #else /* CONFIG_OF */
 static void fec_reset_phy(struct platform_device *pdev)
@@ -3249,6 +3262,11 @@ static void fec_reset_phy(struct platform_device *pdev)
 	 * by machine code.
 	 */
 }
+
+static inline int fec_get_reset_gpio(struct platform_device *pdev)
+{
+	return -EINVAL;
+}
 #endif /* CONFIG_OF */
 
 static void
@@ -3339,6 +3357,11 @@ fec_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, ndev);
 
+	ret = fec_get_reset_gpio(pdev);
+	if (ret == -EPROBE_DEFER)
+		goto gpio_defer;
+	fep->phy_reset_gpio = ret;
+
 	if (of_get_property(np, "fsl,magic-packet", NULL))
 		fep->wol_flag |= FEC_WOL_HAS_MAGIC_PACKET;
 
@@ -3493,6 +3516,7 @@ failed_clk_ipg:
 failed_clk:
 failed_phy:
 	of_node_put(phy_node);
+gpio_defer:
 failed_ioremap:
 	free_netdev(ndev);
 
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 2/2] net: fec: Reset ethernet PHY whenever the enet_out clock is being enabled

[Lothar Waßmann]

If a PHY uses ENET_OUT as reference clock, it may need a RESET to get
functional after the clock had been disabled.

Failure to do this results in the link state constantly toggling
between up and down:
fec 02188000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
fec 02188000.ethernet eth0: Link is Down
fec 02188000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
fec 02188000.ethernet eth0: Link is Down
[...]

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/net/ethernet/freescale/fec.h      |  1 +
 drivers/net/ethernet/freescale/fec_main.c | 38 +++++++++++++++++++++++++------
 2 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 99d33e2..33228df 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -519,6 +519,7 @@ struct fec_enet_private {
 	int	pause_flag;
 	int	wol_flag;
 	u32	quirks;
+	int	phy_reset_gpio;
 
 	struct	napi_struct napi;
 	int	csum_flags;
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 592a7a4..fb0b0eb 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -67,6 +67,7 @@
 
 static void set_multicast_list(struct net_device *ndev);
 static void fec_enet_itr_coal_init(struct net_device *ndev);
+static void fec_reset_phy(struct platform_device *pdev);
 
 #define DRIVER_NAME	"fec"
 
@@ -1862,6 +1863,8 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
 			ret = clk_prepare_enable(fep->clk_enet_out);
 			if (ret)
 				goto failed_clk_enet_out;
+
+			fec_reset_phy(fep->pdev);
 		}
 		if (fep->clk_ptp) {
 			mutex_lock(&fep->ptp_clk_mutex);
@@ -3216,11 +3219,12 @@ static int fec_enet_init(struct net_device *ndev)
 #ifdef CONFIG_OF
 static void fec_reset_phy(struct platform_device *pdev)
 {
-	int err, phy_reset;
-	int msec = 1;
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct fec_enet_private *fep = netdev_priv(ndev);
 	struct device_node *np = pdev->dev.of_node;
+	int msec = 1;
 
-	if (!np)
+	if (!gpio_is_valid(fep->phy_reset_gpio))
 		return;
 
 	of_property_read_u32(np, "phy-reset-duration", &msec);
@@ -3228,18 +3232,27 @@ static void fec_reset_phy(struct platform_device *pdev)
 	if (msec > 1000)
 		msec = 1;
 
+	gpio_set_value_cansleep(fep->phy_reset_gpio, 0);
+	msleep(msec);
+	gpio_set_value_cansleep(fep->phy_reset_gpio, 1);
+}
+
+static int fec_get_reset_gpio(struct platform_device *pdev)
+{
+	int err, phy_reset;
+	struct device_node *np = pdev->dev.of_node;
+
 	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
 	if (!gpio_is_valid(phy_reset))
-		return;
+		return phy_reset;
 
 	err = devm_gpio_request_one(&pdev->dev, phy_reset,
 				    GPIOF_OUT_INIT_LOW, "phy-reset");
 	if (err) {
 		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err);
-		return;
+		return err;
 	}
-	msleep(msec);
-	gpio_set_value_cansleep(phy_reset, 1);
+	return phy_reset;
 }
 #else /* CONFIG_OF */
 static void fec_reset_phy(struct platform_device *pdev)
@@ -3249,6 +3262,11 @@ static void fec_reset_phy(struct platform_device *pdev)
 	 * by machine code.
 	 */
 }
+
+static inline int fec_get_reset_gpio(struct platform_device *pdev)
+{
+	return -EINVAL;
+}
 #endif /* CONFIG_OF */
 
 static void
@@ -3339,6 +3357,11 @@ fec_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, ndev);
 
+	ret = fec_get_reset_gpio(pdev);
+	if (ret == -EPROBE_DEFER)
+		goto gpio_defer;
+	fep->phy_reset_gpio = ret;
+
 	if (of_get_property(np, "fsl,magic-packet", NULL))
 		fep->wol_flag |= FEC_WOL_HAS_MAGIC_PACKET;
 
@@ -3493,6 +3516,7 @@ failed_clk_ipg:
 failed_clk:
 failed_phy:
 	of_node_put(phy_node);
+gpio_defer:
 failed_ioremap:
 	free_netdev(ndev);
 
-- 
2.1.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 2/2] net: fec: Reset ethernet PHY whenever the enet_out clock is being enabled

[Lucas Stach]

Am Dienstag, den 12.01.2016, 16:17 +0100 schrieb Lothar Waßmann:
> If a PHY uses ENET_OUT as reference clock, it may need a RESET to get
> functional after the clock had been disabled.
> 
> Failure to do this results in the link state constantly toggling
> between up and down:
> fec 02188000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
> fec 02188000.ethernet eth0: Link is Down
> fec 02188000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx
> fec 02188000.ethernet eth0: Link is Down
> [...]
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  drivers/net/ethernet/freescale/fec.h      |  1 +
>  drivers/net/ethernet/freescale/fec_main.c | 38 +++++++++++++++++++++++++------
>  2 files changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index 99d33e2..33228df 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -519,6 +519,7 @@ struct fec_enet_private {
>  	int	pause_flag;
>  	int	wol_flag;
>  	u32	quirks;
> +	int	phy_reset_gpio;
>  
>  	struct	napi_struct napi;
>  	int	csum_flags;
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 592a7a4..fb0b0eb 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -67,6 +67,7 @@
>  
>  static void set_multicast_list(struct net_device *ndev);
>  static void fec_enet_itr_coal_init(struct net_device *ndev);
> +static void fec_reset_phy(struct platform_device *pdev);
>  
>  #define DRIVER_NAME	"fec"
>  
> @@ -1862,6 +1863,8 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
>  			ret = clk_prepare_enable(fep->clk_enet_out);
>  			if (ret)
>  				goto failed_clk_enet_out;
> +
> +			fec_reset_phy(fep->pdev);
>  		}
>  		if (fep->clk_ptp) {
>  			mutex_lock(&fep->ptp_clk_mutex);
> @@ -3216,11 +3219,12 @@ static int fec_enet_init(struct net_device *ndev)
>  #ifdef CONFIG_OF
>  static void fec_reset_phy(struct platform_device *pdev)
>  {
> -	int err, phy_reset;
> -	int msec = 1;
> +	struct net_device *ndev = platform_get_drvdata(pdev);
> +	struct fec_enet_private *fep = netdev_priv(ndev);
>  	struct device_node *np = pdev->dev.of_node;
> +	int msec = 1;
>  
> -	if (!np)
> +	if (!gpio_is_valid(fep->phy_reset_gpio))
>  		return;
>  
>  	of_property_read_u32(np, "phy-reset-duration", &msec);

While this isn't in a hot path I still want to see this
of_property_read() moved to the fec_get_reset_gpio() function below.
It's really inconsistent otherwise.


> @@ -3228,18 +3232,27 @@ static void fec_reset_phy(struct platform_device *pdev)
>  	if (msec > 1000)
>  		msec = 1;
>  
> +	gpio_set_value_cansleep(fep->phy_reset_gpio, 0);
> +	msleep(msec);
> +	gpio_set_value_cansleep(fep->phy_reset_gpio, 1);
> +}
> +
> +static int fec_get_reset_gpio(struct platform_device *pdev)
> +{
> +	int err, phy_reset;
> +	struct device_node *np = pdev->dev.of_node;
> +
>  	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
>  	if (!gpio_is_valid(phy_reset))
> -		return;
> +		return phy_reset;
>  
>  	err = devm_gpio_request_one(&pdev->dev, phy_reset,
>  				    GPIOF_OUT_INIT_LOW, "phy-reset");
>  	if (err) {
>  		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err);
> -		return;
> +		return err;
>  	}
> -	msleep(msec);
> -	gpio_set_value_cansleep(phy_reset, 1);
> +	return phy_reset;
>  }
>  #else /* CONFIG_OF */
>  static void fec_reset_phy(struct platform_device *pdev)
> @@ -3249,6 +3262,11 @@ static void fec_reset_phy(struct platform_device *pdev)
>  	 * by machine code.
>  	 */
>  }
> +
> +static inline int fec_get_reset_gpio(struct platform_device *pdev)
> +{
> +	return -EINVAL;
> +}
>  #endif /* CONFIG_OF */
>  
>  static void
> @@ -3339,6 +3357,11 @@ fec_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, ndev);
>  
> +	ret = fec_get_reset_gpio(pdev);
> +	if (ret == -EPROBE_DEFER)
> +		goto gpio_defer;
> +	fep->phy_reset_gpio = ret;
> +
>  	if (of_get_property(np, "fsl,magic-packet", NULL))
>  		fep->wol_flag |= FEC_WOL_HAS_MAGIC_PACKET;
>  
> @@ -3493,6 +3516,7 @@ failed_clk_ipg:
>  failed_clk:
>  failed_phy:
>  	of_node_put(phy_node);
> +gpio_defer:
>  failed_ioremap:
>  	free_netdev(ndev);
>  

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCHv2 1/2] remove redundant struct clk NULL checks

[Lucas Stach]

Am Dienstag, den 12.01.2016, 16:17 +0100 schrieb Lothar Waßmann:
> NULL is a valid argument to clk_enable()/clk_disable(). Remove
> redundant checks before calling those functions.
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
>  drivers/net/ethernet/freescale/fec_main.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 502da6f..592a7a4 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1874,35 +1874,30 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
>  			}
>  			mutex_unlock(&fep->ptp_clk_mutex);
>  		}
> -		if (fep->clk_ref) {
> -			ret = clk_prepare_enable(fep->clk_ref);
> -			if (ret)
> -				goto failed_clk_ref;
> -		}
> +
> +		ret = clk_prepare_enable(fep->clk_ref);
> +		if (ret)
> +			goto failed_clk_ref;
>  	} else {
>  		clk_disable_unprepare(fep->clk_ahb);
> -		if (fep->clk_enet_out)
> -			clk_disable_unprepare(fep->clk_enet_out);
> +		clk_disable_unprepare(fep->clk_enet_out);
>  		if (fep->clk_ptp) {
>  			mutex_lock(&fep->ptp_clk_mutex);
>  			clk_disable_unprepare(fep->clk_ptp);
>  			fep->ptp_clk_on = false;
>  			mutex_unlock(&fep->ptp_clk_mutex);
>  		}
> -		if (fep->clk_ref)
> -			clk_disable_unprepare(fep->clk_ref);
> +		clk_disable_unprepare(fep->clk_ref);
>  	}
>  
>  	return 0;
>  
>  failed_clk_ref:
> -	if (fep->clk_ref)
> -		clk_disable_unprepare(fep->clk_ref);
> +	clk_disable_unprepare(fep->clk_ref);
>  failed_clk_ptp:
> -	if (fep->clk_enet_out)
> -		clk_disable_unprepare(fep->clk_enet_out);
> +	clk_disable_unprepare(fep->clk_enet_out);
>  failed_clk_enet_out:
> -		clk_disable_unprepare(fep->clk_ahb);
> +	clk_disable_unprepare(fep->clk_ahb);
>  
>  	return ret;
>  }

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCHv2 0/2] net: fec: Reset ethernet PHY whenever the enet_out clock is being enabled

[Russell King - ARM Linux]

On Tue, Jan 12, 2016 at 04:17:54PM +0100, Lothar Waßmann wrote:
> This patchset fixes a regression introduced by
> commit e8fcfcd5684a ("net: fec: optimize the clock management to save power")
> for ethernet PHYs that are using ENET_OUT as reference clock (on i.MX6 or i.MX28)
> 
> Changes vs. v1:
> - fixed reference to the commit that introduced the regression.
> - dropped patch to use gpiod framework. This should be added later,
>   after the affected DTBs have been updated to specify the correct
>   gpio_flags.
> 
> Patch overview:
> 1. cleanup patch to remove redundant NULL checks
> 2. call fec_reset_phy() after the ENET_OUT clock has been enabled

I definitely want to test these on my SolidRun boards before these get
merged: the AR8035 on there is configured via pin-straps, and then
further tweaked with PHY quirks.  Resetting with the iMX6 in the
wrong state may result in the AR8035 being reconfigured (even jumping
to a different MDIO address) and certainly would need the PHY quirks
re-running.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCHv2 0/2] net: fec: Reset ethernet PHY whenever the enet_out clock is being enabled

[Lothar Waßmann]

Hi,

> On Tue, Jan 12, 2016 at 04:17:54PM +0100, Lothar Waßmann wrote:
> > This patchset fixes a regression introduced by
> > commit e8fcfcd5684a ("net: fec: optimize the clock management to save power")
> > for ethernet PHYs that are using ENET_OUT as reference clock (on i.MX6 or i.MX28)
> > 
> > Changes vs. v1:
> > - fixed reference to the commit that introduced the regression.
> > - dropped patch to use gpiod framework. This should be added later,
> >   after the affected DTBs have been updated to specify the correct
> >   gpio_flags.
> > 
> > Patch overview:
> > 1. cleanup patch to remove redundant NULL checks
> > 2. call fec_reset_phy() after the ENET_OUT clock has been enabled
> 
> I definitely want to test these on my SolidRun boards before these get
> merged: the AR8035 on there is configured via pin-straps, and then
> further tweaked with PHY quirks.  Resetting with the iMX6 in the
> wrong state may result in the AR8035 being reconfigured (even jumping
> to a different MDIO address) and certainly would need the PHY quirks
> re-running.
> 
As far as I can tell, all SolidRun boards do not specify the enet_out
clock in the dtb, so the PHY reset behaviour should be unaffected by
this patch on those boards, since the additional fec_reset_phy() call is
framed by:
if (fep->clk_enet_out) {
	...
}

But verifying this explicitly is of course a good idea.


Lothar Waßmann

Re: [PATCHv2 0/2] net: fec: Reset ethernet PHY whenever the enet_out clock is being enabled

[Uwe Kleine-König]

Hello,

On Tue, Jan 12, 2016 at 05:04:44PM +0100, Lothar Waßmann wrote:
> > On Tue, Jan 12, 2016 at 04:17:54PM +0100, Lothar Waßmann wrote:
> > > This patchset fixes a regression introduced by
> > > commit e8fcfcd5684a ("net: fec: optimize the clock management to save power")
> > > for ethernet PHYs that are using ENET_OUT as reference clock (on i.MX6 or i.MX28)
> > > 
> > > Changes vs. v1:
> > > - fixed reference to the commit that introduced the regression.
> > > - dropped patch to use gpiod framework. This should be added later,
> > >   after the affected DTBs have been updated to specify the correct
> > >   gpio_flags.
> > > 
> > > Patch overview:
> > > 1. cleanup patch to remove redundant NULL checks
> > > 2. call fec_reset_phy() after the ENET_OUT clock has been enabled
> > 
> > I definitely want to test these on my SolidRun boards before these get
> > merged: the AR8035 on there is configured via pin-straps, and then
> > further tweaked with PHY quirks.  Resetting with the iMX6 in the
> > wrong state may result in the AR8035 being reconfigured (even jumping
> > to a different MDIO address) and certainly would need the PHY quirks
> > re-running.
> > 
> As far as I can tell, all SolidRun boards do not specify the enet_out
> clock in the dtb, so the PHY reset behaviour should be unaffected by
> this patch on those boards, since the additional fec_reset_phy() call is
> framed by:
> if (fep->clk_enet_out) {
> 	...
> }
> 
> But verifying this explicitly is of course a good idea.

If the SolidRun boards don't do this, this doesn't mean it's safe in
general. The problem is real, isn't it?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCHv2 0/2] net: fec: Reset ethernet PHY whenever the enet_out clock is being enabled

[Jörg Krause]

Hi all,

On Tue, 2016-01-12 at 21:30 +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Jan 12, 2016 at 05:04:44PM +0100, Lothar Waßmann wrote:
> > 
> > > 
> > > On Tue, Jan 12, 2016 at 04:17:54PM +0100, Lothar Waßmann wrote:
> > > > 
> > > > This patchset fixes a regression introduced by
> > > > commit e8fcfcd5684a ("net: fec: optimize the clock management
> > > > to save power")
> > > > for ethernet PHYs that are using ENET_OUT as reference clock
> > > > (on i.MX6 or i.MX28)
> > > > 
> > > > Changes vs. v1:
> > > > - fixed reference to the commit that introduced the regression.
> > > > - dropped patch to use gpiod framework. This should be added
> > > > later,
> > > >   after the affected DTBs have been updated to specify the
> > > > correct
> > > >   gpio_flags.
> > > > 
> > > > Patch overview:
> > > > 1. cleanup patch to remove redundant NULL checks
> > > > 2. call fec_reset_phy() after the ENET_OUT clock has been
> > > > enabled
> > > 
> > > I definitely want to test these on my SolidRun boards before
> > > these get
> > > merged: the AR8035 on there is configured via pin-straps, and
> > > then
> > > further tweaked with PHY quirks.  Resetting with the iMX6 in the
> > > wrong state may result in the AR8035 being reconfigured (even
> > > jumping
> > > to a different MDIO address) and certainly would need the PHY
> > > quirks
> > > re-running.
> > > 
> > As far as I can tell, all SolidRun boards do not specify the
> > enet_out
> > clock in the dtb, so the PHY reset behaviour should be unaffected
> > by
> > this patch on those boards, since the additional fec_reset_phy()
> > call is
> > framed by:
> > if (fep->clk_enet_out) {
> > 	...
> > }
> > 
> > But verifying this explicitly is of course a good idea.
> 
> If the SolidRun boards don't do this, this doesn't mean it's safe in
> general. The problem is real, isn't it?

Anything new on this topic? I am facing the same issue with Linux
Kernel 4.7 and the MCSC LAN8720A on an i.MX28 board.

I found this patch on patchwork and it works for me.

Best regard
Jörg Krause

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCHv2 0/2] net: fec: Reset ethernet PHY whenever the enet_out clock is being enabled

[Lothar Waßmann]

This patchset fixes a regression introduced by
commit e8fcfcd5684a ("net: fec: optimize the clock management to save power")
for ethernet PHYs that are using ENET_OUT as reference clock (on i.MX6 or i.MX28)

Changes vs. v1:
- fixed reference to the commit that introduced the regression.
- dropped patch to use gpiod framework. This should be added later,
  after the affected DTBs have been updated to specify the correct
  gpio_flags.

Patch overview:
1. cleanup patch to remove redundant NULL checks
2. call fec_reset_phy() after the ENET_OUT clock has been enabled

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html