[PATCH 0/3] net: fec: Reset ethernet PHY whenever the enet_out clock

[PATCH 0/3] net: fec: Reset ethernet PHY whenever the enet_out clock

[Lothar Waßmann]

This patchset fixes a regression introduced by
commit 8fff755e9f8d ("net: fec: Ensure clocks are enabled while using mdio bus")
for ethernet PHYs that are using ENET_OUT as reference clock (on i.MX6
or i.MX28)

The first patch is a cleanup patch that removes redundant NULL checks.
The second patch converts the driver to use the 'gpiod' framework.
The third patch makes sure, fec_reset_phy() is called whenever the
    enet_out clock has been (re-)enabled to get the PHY into a
    consistent state.

[PATCH 1/3] net: fec: Remove redundant checks for NULL clk pointer

[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 d2328fc..e17d74f 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1873,35 +1873,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

[PATCH 2/3] net: fec: convert to using gpiod framework

[Lothar Waßmann]

Use gpiod_get_optional() instead of checking for a valid GPIO number
and calling devm_gpio_request_one() conditionally.

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

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index e17d74f..1a983fc 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -3233,7 +3233,7 @@ 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;
+	struct gpio_desc *phy_reset;
 	int msec = 1;
 	struct device_node *np = pdev->dev.of_node;
 
@@ -3245,18 +3245,15 @@ static void fec_reset_phy(struct platform_device *pdev)
 	if (msec > 1000)
 		msec = 1;
 
-	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
-	if (!gpio_is_valid(phy_reset))
-		return;
-
-	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);
+	phy_reset = devm_gpiod_get_optional(&pdev->dev, "phy-reset",
+					    GPIOD_OUT_LOW);
+	if (IS_ERR(phy_reset)) {
+		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %ld\n",
+			PTR_ERR(phy_reset));
 		return;
 	}
 	msleep(msec);
-	gpio_set_value_cansleep(phy_reset, 1);
+	gpiod_set_value_cansleep(phy_reset, 1);
 }
 #else /* CONFIG_OF */
 static void fec_reset_phy(struct platform_device *pdev)
-- 
2.1.4

[PATCH 3/3] 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 | 46 ++++++++++++++++++++++++-------
 2 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 99d33e2..8ab4f7f 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;
+	struct gpio_desc *phy_reset;
 
 	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 1a983fc..7ba2bbb 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -66,6 +66,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"
 
@@ -1861,6 +1862,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);
@@ -3231,13 +3234,32 @@ static int fec_enet_init(struct net_device *ndev)
 }
 
 #ifdef CONFIG_OF
-static void fec_reset_phy(struct platform_device *pdev)
+static struct gpio_desc *fec_get_reset_gpio(struct platform_device *pdev)
 {
 	struct gpio_desc *phy_reset;
-	int msec = 1;
 	struct device_node *np = pdev->dev.of_node;
 
 	if (!np)
+		return ERR_PTR(-ENODEV);
+
+	phy_reset = devm_gpiod_get_optional(&pdev->dev, "phy-reset",
+					    GPIOD_OUT_LOW);
+	if (IS_ERR(phy_reset))
+		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %ld\n",
+			PTR_ERR(phy_reset));
+	return phy_reset;
+}
+
+static void fec_reset_phy(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct fec_enet_private *fep = netdev_priv(ndev);
+	int msec = 1;
+
+	if (!fep->phy_reset)
+		return;
+	if (!np)
 		return;
 
 	of_property_read_u32(np, "phy-reset-duration", &msec);
@@ -3245,17 +3267,16 @@ static void fec_reset_phy(struct platform_device *pdev)
 	if (msec > 1000)
 		msec = 1;
 
-	phy_reset = devm_gpiod_get_optional(&pdev->dev, "phy-reset",
-					    GPIOD_OUT_LOW);
-	if (IS_ERR(phy_reset)) {
-		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %ld\n",
-			PTR_ERR(phy_reset));
-		return;
-	}
+	gpiod_set_value_cansleep(fep->phy_reset, 0);
 	msleep(msec);
-	gpiod_set_value_cansleep(phy_reset, 1);
+	gpiod_set_value_cansleep(fep->phy_reset, 1);
 }
 #else /* CONFIG_OF */
+static void fec_get_reset_gpio(struct platform_device *pdev)
+{
+	return -EINVAL;
+}
+
 static void fec_reset_phy(struct platform_device *pdev)
 {
 	/*
@@ -3309,8 +3330,12 @@ fec_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node, *phy_node;
 	int num_tx_qs;
 	int num_rx_qs;
+	struct gpio_desc *phy_reset;
 
 	fec_enet_get_queue_num(pdev, &num_tx_qs, &num_rx_qs);
+	phy_reset = fec_get_reset_gpio(pdev);
+	if (IS_ERR(phy_reset) && PTR_ERR(phy_reset) == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
 
 	/* Init network device */
 	ndev = alloc_etherdev_mqs(sizeof(struct fec_enet_private),
@@ -3331,6 +3356,7 @@ fec_probe(struct platform_device *pdev)
 	fep->netdev = ndev;
 	fep->num_rx_queues = num_rx_qs;
 	fep->num_tx_queues = num_tx_qs;
+	fep->phy_reset = phy_reset;
 
 #if !defined(CONFIG_M5272)
 	/* default enable pause frame auto negotiation */
-- 
2.1.4

Re: [PATCH 2/3] net: fec: convert to using gpiod framework

[Fabio Estevam]

On Mon, Nov 30, 2015 at 9:32 AM, Lothar Waßmann <LW@karo-electronics.de> wrote:

> -       gpio_set_value_cansleep(phy_reset, 1);
> +       gpiod_set_value_cansleep(phy_reset, 1);

This will break some existing DTBs.

Currently the fec driver does not care whether 'phy-reset-gpios' is
active low or active high. (Yes, it was a mistake to assume in the
driver that it is always active low.)

After this change the polarity will be taken into account and may
break some dtb's.

I suggest you to send a single patch with the minimum changes to fix
the regression caused by 8fff755e9f8d ("net: fec: Ensure clocks are
enabled while using mdio bus").

RE: [PATCH 0/3] net: fec: Reset ethernet PHY whenever the enet_out clock

[Duan Andy]

From: Lothar Waßmann <LW@KARO-electronics.de> Sent: Monday, November 30, 2015 7:33 PM
> To: Andrew Lunn; David S. Miller; Estevam Fabio-R49496; Greg Ungerer;
> Kevin Hao; Lothar Waßmann; Lucas Stach; Duan Fugang-B38611; Philippe
> Reynes; Richard Cochran; Russell King; Sascha Hauer; Stefan Agner; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org; Jeff Kirsher; Uwe Kleine-
> König
> Subject: [PATCH 0/3] net: fec: Reset ethernet PHY whenever the enet_out
> clock
> 
> This patchset fixes a regression introduced by commit 8fff755e9f8d ("net:
> fec: Ensure clocks are enabled while using mdio bus") for ethernet PHYs
> that are using ENET_OUT as reference clock (on i.MX6 or i.MX28)
> 
Do you mean commit 8fff755e9f8d cause your problem ?  This commit just manage ipg clock in runtime because mdio bus can access external phy switch no matter netdev status.

I don't think the commit cause phy link up/down issue.  Phy link up/down is due to phy is not ready after power/clock on, it need to do reset.


> The first patch is a cleanup patch that removes redundant NULL checks.
> The second patch converts the driver to use the 'gpiod' framework.
> The third patch makes sure, fec_reset_phy() is called whenever the
>     enet_out clock has been (re-)enabled to get the PHY into a
>     consistent state.

RE: [PATCH 1/3] net: fec: Remove redundant checks for NULL clk pointer

[Duan Andy]

From: Lothar Waßmann <LW@KARO-electronics.de> Sent: Monday, November 30, 2015 7:33 PM
> To: Andrew Lunn; David S. Miller; Estevam Fabio-R49496; Greg Ungerer;
> Kevin Hao; Lothar Waßmann; Lucas Stach; Duan Fugang-B38611; Philippe
> Reynes; Richard Cochran; Russell King; Sascha Hauer; Stefan Agner; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org; Jeff Kirsher; Uwe Kleine-
> König
> Subject: [PATCH 1/3] net: fec: Remove redundant checks for NULL clk
> pointer
> 
> 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 d2328fc..e17d74f 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1873,35 +1873,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;

If you want to clean up the code, pls also remove "fep->clk_enet_out" check in this brace.


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

RE: [PATCH 2/3] net: fec: convert to using gpiod framework

[Duan Andy]

From: Lothar Waßmann <LW@KARO-electronics.de> Sent: Monday, November 30, 2015 7:33 PM
> To: Andrew Lunn; David S. Miller; Estevam Fabio-R49496; Greg Ungerer;
> Kevin Hao; Lothar Waßmann; Lucas Stach; Duan Fugang-B38611; Philippe
> Reynes; Richard Cochran; Russell King; Sascha Hauer; Stefan Agner; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org; Jeff Kirsher; Uwe Kleine-
> König
> Subject: [PATCH 2/3] net: fec: convert to using gpiod framework
> 
> Use gpiod_get_optional() instead of checking for a valid GPIO number and
> calling devm_gpio_request_one() conditionally.
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  drivers/net/ethernet/freescale/fec_main.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index e17d74f..1a983fc 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -3233,7 +3233,7 @@ 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;
> +	struct gpio_desc *phy_reset;
>  	int msec = 1;
>  	struct device_node *np = pdev->dev.of_node;
> 
> @@ -3245,18 +3245,15 @@ static void fec_reset_phy(struct platform_device
> *pdev)
>  	if (msec > 1000)
>  		msec = 1;
> 
> -	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
> -	if (!gpio_is_valid(phy_reset))
> -		return;
> -
> -	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);
> +	phy_reset = devm_gpiod_get_optional(&pdev->dev, "phy-reset",
> +					    GPIOD_OUT_LOW);
> +	if (IS_ERR(phy_reset)) {
> +		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %ld\n",
> +			PTR_ERR(phy_reset));
>  		return;
>  	}
>  	msleep(msec);
> -	gpio_set_value_cansleep(phy_reset, 1);
> +	gpiod_set_value_cansleep(phy_reset, 1);

This API will judge the GPIO active polarity, there many imx boards in dts files don't care the polarity.
So pls drop the patch.

Or use gpiod_set_raw_value_cansleep() instead of gpiod_set_value_cansleep().

>  }
>  #else /* CONFIG_OF */
>  static void fec_reset_phy(struct platform_device *pdev)
> --
> 2.1.4

RE: [PATCH 3/3] net: fec: Reset ethernet PHY whenever the enet_out clock is being enabled

[Duan Andy]

From: Lothar Waßmann <LW@KARO-electronics.de> Sent: Monday, November 30, 2015 7:33 PM
> To: Andrew Lunn; David S. Miller; Estevam Fabio-R49496; Greg Ungerer;
> Kevin Hao; Lothar Waßmann; Lucas Stach; Duan Fugang-B38611; Philippe
> Reynes; Richard Cochran; Russell King; Sascha Hauer; Stefan Agner; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org; Jeff Kirsher; Uwe Kleine-
> König
> Subject: [PATCH 3/3] net: fec: Reset ethernet PHY whenever the enet_out
> clock is being enabled
> 
> 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 | 46 ++++++++++++++++++++++++-
> ------
>  2 files changed, 37 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec.h
> b/drivers/net/ethernet/freescale/fec.h
> index 99d33e2..8ab4f7f 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;
> +	struct gpio_desc *phy_reset;
> 
>  	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 1a983fc..7ba2bbb 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -66,6 +66,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"
> 
> @@ -1861,6 +1862,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);
> @@ -3231,13 +3234,32 @@ static int fec_enet_init(struct net_device
> *ndev)  }
> 
>  #ifdef CONFIG_OF
> -static void fec_reset_phy(struct platform_device *pdev)
> +static struct gpio_desc *fec_get_reset_gpio(struct platform_device
> +*pdev)
>  {
>  	struct gpio_desc *phy_reset;
> -	int msec = 1;
>  	struct device_node *np = pdev->dev.of_node;
> 
>  	if (!np)
> +		return ERR_PTR(-ENODEV);

No need to check device node.

> +
> +	phy_reset = devm_gpiod_get_optional(&pdev->dev, "phy-reset",
> +					    GPIOD_OUT_LOW);
> +	if (IS_ERR(phy_reset))
> +		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %ld\n",
> +			PTR_ERR(phy_reset));
> +	return phy_reset;
> +}
> +
> +static void fec_reset_phy(struct platform_device *pdev) {
> +	struct device_node *np = pdev->dev.of_node;
> +	struct net_device *ndev = platform_get_drvdata(pdev);
> +	struct fec_enet_private *fep = netdev_priv(ndev);
> +	int msec = 1;
> +
> +	if (!fep->phy_reset)
> +		return;
> +	if (!np)
>  		return;

No need to check device node.

> 
>  	of_property_read_u32(np, "phy-reset-duration", &msec); @@ -3245,17
> +3267,16 @@ static void fec_reset_phy(struct platform_device *pdev)
>  	if (msec > 1000)
>  		msec = 1;
> 
> -	phy_reset = devm_gpiod_get_optional(&pdev->dev, "phy-reset",
> -					    GPIOD_OUT_LOW);
> -	if (IS_ERR(phy_reset)) {
> -		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %ld\n",
> -			PTR_ERR(phy_reset));
> -		return;
> -	}
> +	gpiod_set_value_cansleep(fep->phy_reset, 0);
>  	msleep(msec);
> -	gpiod_set_value_cansleep(phy_reset, 1);
> +	gpiod_set_value_cansleep(fep->phy_reset, 1);
>  }
>  #else /* CONFIG_OF */
> +static void fec_get_reset_gpio(struct platform_device *pdev) {
> +	return -EINVAL;
> +}
> +
>  static void fec_reset_phy(struct platform_device *pdev)  {
>  	/*
> @@ -3309,8 +3330,12 @@ fec_probe(struct platform_device *pdev)
>  	struct device_node *np = pdev->dev.of_node, *phy_node;
>  	int num_tx_qs;
>  	int num_rx_qs;
> +	struct gpio_desc *phy_reset;
> 
>  	fec_enet_get_queue_num(pdev, &num_tx_qs, &num_rx_qs);
> +	phy_reset = fec_get_reset_gpio(pdev);
> +	if (IS_ERR(phy_reset) && PTR_ERR(phy_reset) == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> 
>  	/* Init network device */
>  	ndev = alloc_etherdev_mqs(sizeof(struct fec_enet_private), @@ -
> 3331,6 +3356,7 @@ fec_probe(struct platform_device *pdev)
>  	fep->netdev = ndev;
>  	fep->num_rx_queues = num_rx_qs;
>  	fep->num_tx_queues = num_tx_qs;
> +	fep->phy_reset = phy_reset;
> 
>  #if !defined(CONFIG_M5272)
>  	/* default enable pause frame auto negotiation */
> --
> 2.1.4

Re: [PATCH 1/3] net: fec: Remove redundant checks for NULL clk pointer

[Lothar Waßmann]

Hi,

> From: Lothar Waßmann <LW@KARO-electronics.de> Sent: Monday, November 30, 2015 7:33 PM
> > To: Andrew Lunn; David S. Miller; Estevam Fabio-R49496; Greg Ungerer;
> > Kevin Hao; Lothar Waßmann; Lucas Stach; Duan Fugang-B38611; Philippe
> > Reynes; Richard Cochran; Russell King; Sascha Hauer; Stefan Agner; linux-
> > kernel@vger.kernel.org; netdev@vger.kernel.org; Jeff Kirsher; Uwe Kleine-
> > König
> > Subject: [PATCH 1/3] net: fec: Remove redundant checks for NULL clk
> > pointer
> > 
> > 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 d2328fc..e17d74f 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -1873,35 +1873,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;
> 
> If you want to clean up the code, pls also remove "fep->clk_enet_out" check in this brace.
> 
I would have to reintroduce it in the later patch to frame the call to
fec_reset_phy().


Lothar Waßmann

Re: [PATCH 0/3] net: fec: Reset ethernet PHY whenever the enet_out clock

[Lothar Waßmann]

Hi,

> From: Lothar Waßmann <LW@KARO-electronics.de> Sent: Monday, November 30, 2015 7:33 PM
> > To: Andrew Lunn; David S. Miller; Estevam Fabio-R49496; Greg Ungerer;
> > Kevin Hao; Lothar Waßmann; Lucas Stach; Duan Fugang-B38611; Philippe
> > Reynes; Richard Cochran; Russell King; Sascha Hauer; Stefan Agner; linux-
> > kernel@vger.kernel.org; netdev@vger.kernel.org; Jeff Kirsher; Uwe Kleine-
> > König
> > Subject: [PATCH 0/3] net: fec: Reset ethernet PHY whenever the enet_out
> > clock
> > 
> > This patchset fixes a regression introduced by commit 8fff755e9f8d ("net:
> > fec: Ensure clocks are enabled while using mdio bus") for ethernet PHYs
> > that are using ENET_OUT as reference clock (on i.MX6 or i.MX28)
> > 
> Do you mean commit 8fff755e9f8d cause your problem ?  This commit just manage ipg clock in runtime because mdio bus can access external phy switch no matter netdev status.
> 
No. Actually I meant commit e8fcfcd5684a ("net: fec: optimize the clock management to save power")
which started to disable the clocks when not in use.

Sorry for the confusion,
Lothar Waßmann

RE: [PATCH 0/3] net: fec: Reset ethernet PHY whenever the enet_out clock

[Duan Andy]

From: Lothar Waßmann <LW@KARO-electronics.de> Sent: Tuesday, December 01, 2015 3:25 PM
> To: Duan Fugang-B38611
> Cc: Andrew Lunn; David S. Miller; Estevam Fabio-R49496; Greg Ungerer;
> Kevin Hao; Lucas Stach; Philippe Reynes; Richard Cochran; Russell King;
> Sascha Hauer; Stefan Agner; linux-kernel@vger.kernel.org;
> netdev@vger.kernel.org; Jeff Kirsher; Uwe Kleine-K?nig
> Subject: Re: [PATCH 0/3] net: fec: Reset ethernet PHY whenever the
> enet_out clock
> 
> Hi,
> 
> > From: Lothar Waßmann <LW@KARO-electronics.de> Sent: Monday, November
> > 30, 2015 7:33 PM
> > > To: Andrew Lunn; David S. Miller; Estevam Fabio-R49496; Greg
> > > Ungerer; Kevin Hao; Lothar Waßmann; Lucas Stach; Duan Fugang-B38611;
> > > Philippe Reynes; Richard Cochran; Russell King; Sascha Hauer; Stefan
> > > Agner; linux- kernel@vger.kernel.org; netdev@vger.kernel.org; Jeff
> > > Kirsher; Uwe Kleine- König
> > > Subject: [PATCH 0/3] net: fec: Reset ethernet PHY whenever the
> > > enet_out clock
> > >
> > > This patchset fixes a regression introduced by commit 8fff755e9f8d
> ("net:
> > > fec: Ensure clocks are enabled while using mdio bus") for ethernet
> > > PHYs that are using ENET_OUT as reference clock (on i.MX6 or i.MX28)
> > >
> > Do you mean commit 8fff755e9f8d cause your problem ?  This commit just
> manage ipg clock in runtime because mdio bus can access external phy
> switch no matter netdev status.
> >
> No. Actually I meant commit e8fcfcd5684a ("net: fec: optimize the clock
> management to save power") which started to disable the clocks when not
> in use.
> 
Understand. Yes, disable enet_out clock for power saving is necessary.

> Sorry for the confusion,
> Lothar Waßmann

Re: [PATCH 2/3] net: fec: convert to using gpiod framework

[Lothar Waßmann]

Hi,

> From: Lothar Waßmann <LW@KARO-electronics.de> Sent: Monday, November 30, 2015 7:33 PM
> > To: Andrew Lunn; David S. Miller; Estevam Fabio-R49496; Greg Ungerer;
> > Kevin Hao; Lothar Waßmann; Lucas Stach; Duan Fugang-B38611; Philippe
> > Reynes; Richard Cochran; Russell King; Sascha Hauer; Stefan Agner; linux-
> > kernel@vger.kernel.org; netdev@vger.kernel.org; Jeff Kirsher; Uwe Kleine-
> > König
> > Subject: [PATCH 2/3] net: fec: convert to using gpiod framework
> > 
> > Use gpiod_get_optional() instead of checking for a valid GPIO number and
> > calling devm_gpio_request_one() conditionally.
> > 
> > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > ---
> >  drivers/net/ethernet/freescale/fec_main.c | 17 +++++++----------
> >  1 file changed, 7 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/freescale/fec_main.c
> > b/drivers/net/ethernet/freescale/fec_main.c
> > index e17d74f..1a983fc 100644
> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -3233,7 +3233,7 @@ 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;
> > +	struct gpio_desc *phy_reset;
> >  	int msec = 1;
> >  	struct device_node *np = pdev->dev.of_node;
> > 
> > @@ -3245,18 +3245,15 @@ static void fec_reset_phy(struct platform_device
> > *pdev)
> >  	if (msec > 1000)
> >  		msec = 1;
> > 
> > -	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
> > -	if (!gpio_is_valid(phy_reset))
> > -		return;
> > -
> > -	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);
> > +	phy_reset = devm_gpiod_get_optional(&pdev->dev, "phy-reset",
> > +					    GPIOD_OUT_LOW);
> > +	if (IS_ERR(phy_reset)) {
> > +		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %ld\n",
> > +			PTR_ERR(phy_reset));
> >  		return;
> >  	}
> >  	msleep(msec);
> > -	gpio_set_value_cansleep(phy_reset, 1);
> > +	gpiod_set_value_cansleep(phy_reset, 1);
> 
> This API will judge the GPIO active polarity, there many imx boards in dts files don't care the polarity.
> So pls drop the patch.
> 
> Or use gpiod_set_raw_value_cansleep() instead of gpiod_set_value_cansleep().
> 
I could prepare a patch that temporarily uses the raw functions and add
a comment to convert these to the proper functions once all DTB files
have been corrected and some time has passed to account for boards using
old dtbs with newer kernels.

I could also send a patch to convert all in-tree dts files to use
GPIO_ACTIVE_LOW if that is desired.


Lothar Waßmann