[PATCH 1/2] net: ethernet: freescale: simplify fec_reset_phy

[PATCH 1/2] net: ethernet: freescale: simplify fec_reset_phy

[Richard Leitner]

The fec_reset_phy function allowed only one execution during probeing.
As a preparation for future patches move the dt parsing and gpio
allocation to the probe function. The parameters of the phy reset are
added to the fec_enet_private struct. As a result the fec_reset_phy
function may be called anytime after probe.

Signed-off-by: Richard Leitner <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org>
---
 drivers/net/ethernet/freescale/fec.h      |  4 ++
 drivers/net/ethernet/freescale/fec_main.c | 85 ++++++++++++++++---------------
 2 files changed, 47 insertions(+), 42 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 5ea740b..3da8084 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -534,6 +534,10 @@ struct fec_enet_private {
 	int	pause_flag;
 	int	wol_flag;
 	u32	quirks;
+	int	phy_reset;
+	int	phy_reset_duration;
+	int	phy_reset_post_delay;
+	bool	phy_reset_active_high;
 
 	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 f7c8649..cbbf7b8 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -3188,62 +3188,36 @@ static int fec_enet_init(struct net_device *ndev)
 }
 
 #ifdef CONFIG_OF
-static int fec_reset_phy(struct platform_device *pdev)
+static int fec_reset_phy(struct net_device *ndev)
 {
-	int err, phy_reset;
-	bool active_high = false;
-	int msec = 1, phy_post_delay = 0;
-	struct device_node *np = pdev->dev.of_node;
-
-	if (!np)
-		return 0;
-
-	err = of_property_read_u32(np, "phy-reset-duration", &msec);
-	/* A sane reset duration should not be longer than 1s */
-	if (!err && msec > 1000)
-		msec = 1;
+	struct fec_enet_private *fep = netdev_priv(ndev);
 
-	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
-	if (phy_reset == -EPROBE_DEFER)
-		return phy_reset;
-	else if (!gpio_is_valid(phy_reset))
+	if (!fep->phy_reset)
 		return 0;
 
-	err = of_property_read_u32(np, "phy-reset-post-delay", &phy_post_delay);
-	/* valid reset duration should be less than 1s */
-	if (!err && phy_post_delay > 1000)
-		return -EINVAL;
-
-	active_high = of_property_read_bool(np, "phy-reset-active-high");
+	gpio_set_value_cansleep(fep->phy_reset, fep->phy_reset_active_high);
 
-	err = devm_gpio_request_one(&pdev->dev, phy_reset,
-			active_high ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
-			"phy-reset");
-	if (err) {
-		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err);
-		return err;
-	}
-
-	if (msec > 20)
-		msleep(msec);
+	if (fep->phy_reset_duration > 20)
+		msleep(fep->phy_reset_duration);
 	else
-		usleep_range(msec * 1000, msec * 1000 + 1000);
+		usleep_range(fep->phy_reset_duration * 1000,
+			     fep->phy_reset_duration * 1000 + 1000);
 
-	gpio_set_value_cansleep(phy_reset, !active_high);
+	gpio_set_value_cansleep(fep->phy_reset, !fep->phy_reset_active_high);
 
-	if (!phy_post_delay)
+	if (!fep->phy_reset_post_delay)
 		return 0;
 
-	if (phy_post_delay > 20)
-		msleep(phy_post_delay);
+	if (fep->phy_reset_post_delay > 20)
+		msleep(fep->phy_reset_post_delay);
 	else
-		usleep_range(phy_post_delay * 1000,
-			     phy_post_delay * 1000 + 1000);
+		usleep_range(fep->phy_reset_post_delay * 1000,
+			     fep->phy_reset_post_delay * 1000 + 1000);
 
 	return 0;
 }
 #else /* CONFIG_OF */
-static int fec_reset_phy(struct platform_device *pdev)
+static int fec_reset_phy(struct net_device *ndev)
 {
 	/*
 	 * In case of platform probe, the reset has been done
@@ -3361,6 +3335,33 @@ fec_probe(struct platform_device *pdev)
 	}
 	fep->phy_node = phy_node;
 
+	fep->phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
+	if (gpio_is_valid(fep->phy_reset)) {
+		ret = of_property_read_u32(np, "phy-reset-duration",
+					   &fep->phy_reset_duration);
+		/* A sane reset duration should not be longer than 1s */
+		if (!ret && fep->phy_reset_post_delay > 1000)
+			fep->phy_reset_post_delay = 1;
+
+		ret = of_property_read_u32(np, "phy-reset-post-delay",
+					   &fep->phy_reset_post_delay);
+		/* valid post reset delay should be less than 1s */
+		if (!ret && fep->phy_reset_post_delay > 1000)
+			fep->phy_reset_post_delay = 1;
+
+		fep->phy_reset_active_high =  of_property_read_bool(np, "phy-reset-active-high");
+
+		ret = devm_gpio_request_one(&pdev->dev, fep->phy_reset,
+					    fep->phy_reset_active_high ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
+					    "phy-reset");
+		if (ret) {
+			dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", ret);
+			goto failed_phy;
+		}
+	} else {
+		fep->phy_reset = 0;
+	}
+
 	ret = of_get_phy_mode(pdev->dev.of_node);
 	if (ret < 0) {
 		pdata = dev_get_platdata(&pdev->dev);
@@ -3433,7 +3434,7 @@ fec_probe(struct platform_device *pdev)
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
-	ret = fec_reset_phy(pdev);
+	ret = fec_reset_phy(ndev);
 	if (ret)
 		goto failed_reset;
 
-- 
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

[PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option

[Richard Leitner]

Some PHYs (for example the LAN8710) doesn't allow turning the 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 power")
Therefore add a devictree option to perform a PHY reset and
configuration after every clock enable.

For a better understanding here's a outline of the time response of the
clock and reset lines before and after this patch:

			  v--fec_probe()              v--fec_enet_open()
			  v                           v
	w/o patch eCLK: ___||||||||____________________|||||||||||||||||
	w/o patch nRST: ----__------------------------------------------
	w/o patch CONF: _______XX_______________________________________

	w/  patch eCLK: ___||||||||____________________|||||||||||||||||
	w/  patch nRST: ----__--------------------------__--------------
	w/  patch CONF: _______XX__________________________XX___________
			  ^                           ^
			  ^--fec_probe()              ^--fec_enet_open()

In our case 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
hundred PORs of our board.

Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to sa...")
Signed-off-by: Richard Leitner <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org>
---
 Documentation/devicetree/bindings/net/fsl-fec.txt |  3 +++
 drivers/net/ethernet/freescale/fec.h              |  1 +
 drivers/net/ethernet/freescale/fec_main.c         | 16 ++++++++++++++++
 3 files changed, 20 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
index 6f55bdd..1766579 100644
--- a/Documentation/devicetree/bindings/net/fsl-fec.txt
+++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
@@ -23,6 +23,9 @@ Optional properties:
 - phy-handle : phandle to the PHY device connected to this device.
 - fixed-link : Assume a fixed link. See fixed-link.txt in the same directory.
   Use instead of phy-handle.
+- phy-reset-after-clk-enable : If present then a phy reset and configuration
+  will be performed everytime after the clocks are enabled. This is required
+  for some PHYs to work properly.
 - fsl,num-tx-queues : The property is valid for enet-avb IP, which supports
   hw multi queues. Should specify the tx queue number, otherwise set tx queue
   number to 1.
diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index 3da8084..d4f658a 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -538,6 +538,7 @@ struct fec_enet_private {
 	int	phy_reset_duration;
 	int	phy_reset_post_delay;
 	bool	phy_reset_active_high;
+	bool	phy_reset_after_clk_enable;
 
 	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 cbbf7b8..4e744f3 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -68,6 +68,7 @@
 
 static void set_multicast_list(struct net_device *ndev);
 static void fec_enet_itr_coal_init(struct net_device *ndev);
+static int fec_reset_phy(struct net_device *ndev);
 
 #define DRIVER_NAME	"fec"
 
@@ -1862,6 +1863,18 @@ 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;
+
+		/* reset the phy after clk is enabled if it's configured */
+		if (fep->phy_reset_after_clk_enable) {
+			ret = fec_reset_phy(ndev);
+			if (ret)
+				goto failed_reset;
+			if (ndev->phydev) {
+				ret = phy_init_hw(ndev->phydev);
+				if (ret)
+					goto failed_reset;
+			}
+		}
 	} else {
 		clk_disable_unprepare(fep->clk_ahb);
 		clk_disable_unprepare(fep->clk_enet_out);
@@ -1876,6 +1889,7 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
 
 	return 0;
 
+failed_reset:
 failed_clk_ref:
 	if (fep->clk_ref)
 		clk_disable_unprepare(fep->clk_ref);
@@ -3350,6 +3364,7 @@ fec_probe(struct platform_device *pdev)
 			fep->phy_reset_post_delay = 1;
 
 		fep->phy_reset_active_high =  of_property_read_bool(np, "phy-reset-active-high");
+		fep->phy_reset_after_clk_enable = of_property_read_bool(np, "phy-reset-after-clk-enable");
 
 		ret = devm_gpio_request_one(&pdev->dev, fep->phy_reset,
 					    fep->phy_reset_active_high ? GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
@@ -3360,6 +3375,7 @@ fec_probe(struct platform_device *pdev)
 		}
 	} else {
 		fep->phy_reset = 0;
+		fep->phy_reset_after_clk_enable = false;
 	}
 
 	ret = of_get_phy_mode(pdev->dev.of_node);
-- 
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

Re: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option

[Andrew Lunn]

Hi Richard

> diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> index 6f55bdd..1766579 100644
> --- a/Documentation/devicetree/bindings/net/fsl-fec.txt
> +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> @@ -23,6 +23,9 @@ Optional properties:
>  - phy-handle : phandle to the PHY device connected to this device.
>  - fixed-link : Assume a fixed link. See fixed-link.txt in the same directory.
>    Use instead of phy-handle.
> +- phy-reset-after-clk-enable : If present then a phy reset and configuration
> +  will be performed everytime after the clocks are enabled. This is required
> +  for some PHYs to work properly.

It sounds like this issue will apply to any LAN8710 which has its
clock fiddled with. So this should be a centrally documented property,
which any MAC driver can implement. Please move the above text to
ethernet.txt, and instead have:

 - phy-reset-after-clk-enable : See ethernet.txt

 Thanks
	 Andrew
--
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

Re: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option

[Richard Leitner]

On 07/06/2017 03:55 PM, Andrew Lunn wrote:
>> diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
>> index 6f55bdd..1766579 100644
>> --- a/Documentation/devicetree/bindings/net/fsl-fec.txt
>> +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
>> @@ -23,6 +23,9 @@ Optional properties:
>>  - phy-handle : phandle to the PHY device connected to this device.
>>  - fixed-link : Assume a fixed link. See fixed-link.txt in the same directory.
>>    Use instead of phy-handle.
>> +- phy-reset-after-clk-enable : If present then a phy reset and configuration
>> +  will be performed everytime after the clocks are enabled. This is required
>> +  for some PHYs to work properly.
> 
> It sounds like this issue will apply to any LAN8710 which has its
> clock fiddled with. So this should be a centrally documented property,
> which any MAC driver can implement. Please move the above text to
> ethernet.txt, and instead have:
> 
>  - phy-reset-after-clk-enable : See ethernet.txt

I haven't tested it on any other platform than an i.MX6, but IMHO you're
right. It sounds reasonable that it affects any SoC which is messing
around with the clock and not performing a reset by default.

Thanks for that hint!
I'll include that in the v2.

>  Thanks
> 	 Andrew
> 

regards,
Richard.L
--
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

RE: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option

[Andy Duan]

From: Richard Leitner <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org> Sent: Thursday, July 06, 2017 9:06 PM
>To: Andy Duan <fugang.duan-3arQi8VN3Tc@public.gmane.org>; robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org;
>mark.rutland-5wv7dgnIgG8@public.gmane.org
>Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
>kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; dev-M/VWbR8SM2SsTnJN9+BGXg@public.gmane.org; Richard Leitner
><richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org>
>Subject: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option
>
>Some PHYs (for example the LAN8710) doesn't allow turning the 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 power") Therefore add a
>devictree option to perform a PHY reset and configuration after every clock
>enable.
>
>For a better understanding here's a outline of the time response of the clock
>and reset lines before and after this patch:
>
>			  v--fec_probe()              v--fec_enet_open()
>			  v                           v
>	w/o patch eCLK:
>___||||||||____________________|||||||||||||||||
>	w/o patch nRST: ----__------------------------------------------
>	w/o patch CONF:
>_______XX_______________________________________
>
>	w/  patch eCLK:
>___||||||||____________________|||||||||||||||||
>	w/  patch nRST: ----__--------------------------__--------------
>	w/  patch CONF:
>_______XX__________________________XX___________
>			  ^                           ^
>			  ^--fec_probe()              ^--fec_enet_open()
>
>In our case 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 hundred PORs
>of our board.
>
>Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to sa...")
>Signed-off-by: Richard Leitner <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org>
>---
> Documentation/devicetree/bindings/net/fsl-fec.txt |  3 +++
> drivers/net/ethernet/freescale/fec.h              |  1 +
> drivers/net/ethernet/freescale/fec_main.c         | 16 ++++++++++++++++
> 3 files changed, 20 insertions(+)
>
>diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt
>b/Documentation/devicetree/bindings/net/fsl-fec.txt
>index 6f55bdd..1766579 100644
>--- a/Documentation/devicetree/bindings/net/fsl-fec.txt
>+++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
>@@ -23,6 +23,9 @@ Optional properties:
> - phy-handle : phandle to the PHY device connected to this device.
> - fixed-link : Assume a fixed link. See fixed-link.txt in the same directory.
>   Use instead of phy-handle.
>+- phy-reset-after-clk-enable : If present then a phy reset and
>+configuration
>+  will be performed everytime after the clocks are enabled. This is
>+required
>+  for some PHYs to work properly.
> - fsl,num-tx-queues : The property is valid for enet-avb IP, which supports
>   hw multi queues. Should specify the tx queue number, otherwise set tx
>queue
>   number to 1.
>diff --git a/drivers/net/ethernet/freescale/fec.h
>b/drivers/net/ethernet/freescale/fec.h
>index 3da8084..d4f658a 100644
>--- a/drivers/net/ethernet/freescale/fec.h
>+++ b/drivers/net/ethernet/freescale/fec.h
>@@ -538,6 +538,7 @@ struct fec_enet_private {
> 	int	phy_reset_duration;
> 	int	phy_reset_post_delay;
> 	bool	phy_reset_active_high;
>+	bool	phy_reset_after_clk_enable;
>
> 	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 cbbf7b8..4e744f3 100644
>--- a/drivers/net/ethernet/freescale/fec_main.c
>+++ b/drivers/net/ethernet/freescale/fec_main.c
>@@ -68,6 +68,7 @@
>
> static void set_multicast_list(struct net_device *ndev);  static void
>fec_enet_itr_coal_init(struct net_device *ndev);
>+static int fec_reset_phy(struct net_device *ndev);
>
> #define DRIVER_NAME	"fec"
>
>@@ -1862,6 +1863,18 @@ 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;
>+
>+		/* reset the phy after clk is enabled if it's configured */
>+		if (fep->phy_reset_after_clk_enable) {
>+			ret = fec_reset_phy(ndev);
>+			if (ret)
>+				goto failed_reset;
>+			if (ndev->phydev) {
>+				ret = phy_init_hw(ndev->phydev);
>+				if (ret)
>+					goto failed_reset;
>+			}
>+		}

Since it is common issue so long as using the PHY, can you move it into smsc phy driver like in .smsc_phy_reset() function ?
And get the reset pin from phy dts node.

Andy  

> 	} else {
> 		clk_disable_unprepare(fep->clk_ahb);
> 		clk_disable_unprepare(fep->clk_enet_out);
>@@ -1876,6 +1889,7 @@ static int fec_enet_clk_enable(struct net_device
>*ndev, bool enable)
>
> 	return 0;
>
>+failed_reset:
> failed_clk_ref:
> 	if (fep->clk_ref)
> 		clk_disable_unprepare(fep->clk_ref);
>@@ -3350,6 +3364,7 @@ fec_probe(struct platform_device *pdev)
> 			fep->phy_reset_post_delay = 1;
>
> 		fep->phy_reset_active_high =  of_property_read_bool(np,
>"phy-reset-active-high");
>+		fep->phy_reset_after_clk_enable =
>of_property_read_bool(np,
>+"phy-reset-after-clk-enable");
>
> 		ret = devm_gpio_request_one(&pdev->dev, fep->phy_reset,
> 					    fep->phy_reset_active_high ?
>GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW, @@ -3360,6 +3375,7 @@
>fec_probe(struct platform_device *pdev)
> 		}
> 	} else {
> 		fep->phy_reset = 0;
>+		fep->phy_reset_after_clk_enable = false;
> 	}
>
> 	ret = of_get_phy_mode(pdev->dev.of_node);
>--
>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

Re: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option

[Richard Leitner]

On 07/07/2017 07:30 AM, Andy Duan wrote:
> From: Richard Leitner <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org> Sent: Thursday, July 06, 2017 9:06 PM
>> To: Andy Duan <fugang.duan-3arQi8VN3Tc@public.gmane.org>; robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org;
>> mark.rutland-5wv7dgnIgG8@public.gmane.org
>> Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
>> kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; dev-M/VWbR8SM2SsTnJN9+BGXg@public.gmane.org; Richard Leitner
>> <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org>
>> Subject: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option
>>
>> Some PHYs (for example the LAN8710) doesn't allow turning the 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 power") Therefore add a
>> devictree option to perform a PHY reset and configuration after every clock
>> enable.
>>
>> For a better understanding here's a outline of the time response of the clock
>> and reset lines before and after this patch:
>>
>> 			  v--fec_probe()              v--fec_enet_open()
>> 			  v                           v
>> 	w/o patch eCLK:
>> ___||||||||____________________|||||||||||||||||
>> 	w/o patch nRST: ----__------------------------------------------
>> 	w/o patch CONF:
>> _______XX_______________________________________
>>
>> 	w/  patch eCLK:
>> ___||||||||____________________|||||||||||||||||
>> 	w/  patch nRST: ----__--------------------------__--------------
>> 	w/  patch CONF:
>> _______XX__________________________XX___________
>> 			  ^                           ^
>> 			  ^--fec_probe()              ^--fec_enet_open()
>>
>> In our case 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 hundred PORs
>> of our board.
>>
>> Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to sa...")
>> Signed-off-by: Richard Leitner <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org>

...

>> *ndev, bool enable)
>> 		ret = clk_prepare_enable(fep->clk_ref);
>> 		if (ret)
>> 			goto failed_clk_ref;
>> +
>> +		/* reset the phy after clk is enabled if it's configured */
>> +		if (fep->phy_reset_after_clk_enable) {
>> +			ret = fec_reset_phy(ndev);
>> +			if (ret)
>> +				goto failed_reset;
>> +			if (ndev->phydev) {
>> +				ret = phy_init_hw(ndev->phydev);
>> +				if (ret)
>> +					goto failed_reset;
>> +			}
>> +		}
>
> Since it is common issue so long as using the PHY, can you move it into smsc phy driver like in .smsc_phy_reset() function ?
> And get the reset pin from phy dts node.

During my first glance at this problem I had the same "feeling" that it 
should go into smsc.c. Therefore I've tried that already, but I haven't 
found a suitable "place" for that. My basic problem is: Where do I know 
(from smsc.c view) when the enetrefclk was disabled/enabled again 
without changing fec_main.c?

Some more points that come into my mind:
  - The smsc_phy_reset function is registered as "soft_reset". Would it 
be OK to use nRST in it?
  - Would it be OK to call the phy_init_hw function from within the 
smsc_phy_reset?
  - IMHO I'd have to move the reset gpio binding inside the phy node 
then. Isn't that a pretty big change doing that for all PHYs/FECs? Would 
it be "worth" it?

Sorry for these many (maybe noobish) questions, but I'm pretty new to 
the kernels fec/phy stuff ;-)

>
> Andy
>

Thanks & regards,
Richard.L
--
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

RE: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option

[Andy Duan]

From: Richard Leitner <richard.leitner@skidata.com> Sent: Friday, July 07, 2017 1:51 PM
>> Since it is common issue so long as using the PHY, can you move it into smsc
>phy driver like in .smsc_phy_reset() function ?
>> And get the reset pin from phy dts node.
>
>Some more points that come into my mind:
>  - The smsc_phy_reset function is registered as "soft_reset". Would it be OK to
>use nRST in it?

It is not reasonable.

>  - Would it be OK to call the phy_init_hw function from within the
>smsc_phy_reset?

No, phy_init_hw() already call .drv->soft_reset().

>  - IMHO I'd have to move the reset gpio binding inside the phy node then. Isn't
>that a pretty big change doing that for all PHYs/FECs? Would it be "worth" it?
>
To make the change to be common, there have big change for phy driver.
Maybe somebody can give one good suggestion/solution for it.

Andy

Re: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option

[Richard Leitner]

On 07/07/2017 09:03 AM, Andy Duan wrote:
> From: Richard Leitner <richard.leitner@skidata.com> Sent: Friday, July 07, 2017 1:51 PM
>>> Since it is common issue so long as using the PHY, can you move it into smsc
>> phy driver like in .smsc_phy_reset() function ?
>>> And get the reset pin from phy dts node.
>>
>> Some more points that come into my mind:
>>  - The smsc_phy_reset function is registered as "soft_reset". Would it be OK to
>> use nRST in it?
>
> It is not reasonable.
>
>>  - Would it be OK to call the phy_init_hw function from within the
>> smsc_phy_reset?
>
> No, phy_init_hw() already call .drv->soft_reset().
>
>>  - IMHO I'd have to move the reset gpio binding inside the phy node then. Isn't
>> that a pretty big change doing that for all PHYs/FECs? Would it be "worth" it?
>>
> To make the change to be common, there have big change for phy driver.
> Maybe somebody can give one good suggestion/solution for it.

Sorry, I don't think I understood everything correctly:

1. The "phy-reset-gpios" binding should go inside the phy node. This 
will cause to *change ALL FEC and PHY drivers*. Correct?

2. Add an additonal "hard reset" function to the PHY driver which 
handles the "phy-reset-gpios". Correct?

3. Who should then trigger the "hard reset" of the PHY? phy_init_hw? The 
FEC?

The point is that the LAN8710 is currently not always working correctly, 
therefore this small change was proposed. Should we really change all 
PHY/FECs only because of this?
Furthermore one problem still remains: The enet_refclk is controlled by 
the FEC. How does the PHY recognize when it was disabled/enabled?

>
> Andy
>

kind regards,
Richard.L

RE: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option

[Andy Duan]

From: Richard Leitner <richard.leitner@skidata.com> Sent: Friday, July 07, 2017 5:53 PM
>To: Andy Duan <fugang.duan@nxp.com>; robh+dt@kernel.org;
>mark.rutland@arm.com
>Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; linux-
>kernel@vger.kernel.org; dev@g0hl1n.net; Andrew Lunn <andrew@lunn.ch>
>Subject: Re: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable
>option
>
>
>On 07/07/2017 09:03 AM, Andy Duan wrote:
>> From: Richard Leitner <richard.leitner@skidata.com> Sent: Friday, July
>> 07, 2017 1:51 PM
>>>> Since it is common issue so long as using the PHY, can you move it
>>>> into smsc
>>> phy driver like in .smsc_phy_reset() function ?
>>>> And get the reset pin from phy dts node.
>>>
>>> Some more points that come into my mind:
>>>  - The smsc_phy_reset function is registered as "soft_reset". Would
>>> it be OK to use nRST in it?
>>
>> It is not reasonable.
>>
>>>  - Would it be OK to call the phy_init_hw function from within the
>>> smsc_phy_reset?
>>
>> No, phy_init_hw() already call .drv->soft_reset().
>>
>>>  - IMHO I'd have to move the reset gpio binding inside the phy node
>>> then. Isn't that a pretty big change doing that for all PHYs/FECs? Would it be
>"worth" it?
>>>
>> To make the change to be common, there have big change for phy driver.
>> Maybe somebody can give one good suggestion/solution for it.
>
>Sorry, I don't think I understood everything correctly:
>
>1. The "phy-reset-gpios" binding should go inside the phy node. This will cause
>to *change ALL FEC and PHY drivers*. Correct?
>
The "phy-reset-gpios" binding should go inside the phy node that is more reasonable.
It is better PHY core driver handle phy hw reset.

>2. Add an additonal "hard reset" function to the PHY driver which handles the
>"phy-reset-gpios". Correct?
>
Correct.

>3. Who should then trigger the "hard reset" of the PHY? phy_init_hw? The FEC?
>
>The point is that the LAN8710 is currently not always working correctly,
>therefore this small change was proposed. Should we really change all
>PHY/FECs only because of this?
>Furthermore one problem still remains: The enet_refclk is controlled by the
>FEC. How does the PHY recognize when it was disabled/enabled?
>
Your patch is workaround for the issue. As you pointed out these is a common issue.
So we hope to get a better solution to handle these in common code.

Andy

Re: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option

[Richard Leitner]

Hi Andy,
thanks for the clarifications!

On 07/07/2017 01:08 PM, Andy Duan wrote:
>> 3. Who should then trigger the "hard reset" of the PHY? phy_init_hw? The FEC?
>>
>> The point is that the LAN8710 is currently not always working correctly,
>> therefore this small change was proposed. Should we really change all
>> PHY/FECs only because of this?
>> Furthermore one problem still remains: The enet_refclk is controlled by the
>> FEC. How does the PHY recognize when it was disabled/enabled?
>>
> Your patch is workaround for the issue. As you pointed out these is a common issue.
> So we hope to get a better solution to handle these in common code.

Ok. I'm fine with moving the phy-reset-gpios binding into the PHY. But 
one question still remains: Who should then trigger the "hard reset" of 
the PHY?

The PHY itself doesn't "know" when the refclk is turned off/on. So the 
FEC still must call some function of the PHY after the clock was enabled 
again. So the phy-reset-after-clk-enable property will remain in the fec 
node? Or am I missing something?

Re: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option

[Andrew Lunn]

> Ok. I'm fine with moving the phy-reset-gpios binding into the PHY.
> But one question still remains: Who should then trigger the "hard
> reset" of the PHY?

Hi Richard

I think i see a few whys to do this, but first i need to check
something. Is the clock which is causing a problem this one:

        /* clk_ref is optional, depends on board */
        fep->clk_ref = devm_clk_get(&pdev->dev, "enet_clk_ref");
        if (IS_ERR(fep->clk_ref))
                fep->clk_ref = NULL;

Possible solutions:

1) clocks are referenced counted. If it is turned on twice, it needs
   to be turned off twice before it is actually turned off. So, make
   the PHY driver also clk_prepare_enable() this clock. When the FEC
   tries to turn it off, it will stay ticking. Problem avoided, at the
   expense of some power.

2) More complex, but make the PHY driver also a clock driver. Have the
   PHY driver export a clock which the FEC use, as "enet_clk_ref". The
   implementation of this clock, would both turn the real clock on,
   and the perform the reset.

Both require no changes to the FEC, or any other MAC driver using this
PHY, so long as the MAC driver uses the common clock infrastructure to
control the clock to the PHY.

	Andrew


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

Re: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option

[Richard Leitner]

On 07/07/2017 04:00 PM, Andrew Lunn wrote:
>> Ok. I'm fine with moving the phy-reset-gpios binding into the PHY.
>> But one question still remains: Who should then trigger the "hard
>> reset" of the PHY?
> 
> Hi Richard
> 
> I think i see a few whys to do this, but first i need to check
> something. Is the clock which is causing a problem this one:
> 
>         /* clk_ref is optional, depends on board */
>         fep->clk_ref = devm_clk_get(&pdev->dev, "enet_clk_ref");
>         if (IS_ERR(fep->clk_ref))
>                 fep->clk_ref = NULL;

Yes. It's this one.

> 
> Possible solutions:
> 
> 1) clocks are referenced counted. If it is turned on twice, it needs
>    to be turned off twice before it is actually turned off. So, make
>    the PHY driver also clk_prepare_enable() this clock. When the FEC
>    tries to turn it off, it will stay ticking. Problem avoided, at the
>    expense of some power.

Somehow this approach triggers a "workaround-feeling" for me...
Furthermore as you say it "wastes" (at least some) power. For exactly
this reason the disabling of the clock was implemented in commit
e8fcfcd5684a ("net: fec: optimize the clock management to save power").

> 
> 2) More complex, but make the PHY driver also a clock driver. Have the
>    PHY driver export a clock which the FEC use, as "enet_clk_ref". The
>    implementation of this clock, would both turn the real clock on,
>    and the perform the reset.

This seems as a good solution to me. Furthermore IMHO it would be good
to move all PHY related dt bindings (reset-gpio, clk, etc.) from the MAC
into the PHY node.

Or are there any reasons/arguments against this approach?

> 
> Both require no changes to the FEC, or any other MAC driver using this
> PHY, so long as the MAC driver uses the common clock infrastructure to
> control the clock to the PHY.
As (IMHO) the new approach likely won't be backported to stable releases
I want to stress again the point that commit e8fcfcd5684a
("net: fec: optimize the clock management to save power") introduced
this problem and therefore "broke the PHY" for our board.

So would it be possible to add a "quick" bugfix patch (maybe this patch
or another one removing the clk disable) so this fix can be backported
to stable? Otherwise our board is only working with another
"out-of-tree" patch (which I want to avoid)...

kind regards,
Richard.L

Re: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option

[Andrew Lunn]

> So would it be possible to add a "quick" bugfix patch (maybe this patch
> or another one removing the clk disable) so this fix can be backported
> to stable? Otherwise our board is only working with another
> "out-of-tree" patch (which I want to avoid)...

Hi Richard

It is a clear regression, so a minimal fix would be accepted to
stable.

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

Re: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option

[Rob Herring]

On Thu, Jul 06, 2017 at 03:05:30PM +0200, Richard Leitner wrote:
> Some PHYs (for example the LAN8710) doesn't allow turning the 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 power")
> Therefore add a devictree option to perform a PHY reset and
> configuration after every clock enable.
> 
> For a better understanding here's a outline of the time response of the
> clock and reset lines before and after this patch:
> 
> 			  v--fec_probe()              v--fec_enet_open()
> 			  v                           v
> 	w/o patch eCLK: ___||||||||____________________|||||||||||||||||
> 	w/o patch nRST: ----__------------------------------------------
> 	w/o patch CONF: _______XX_______________________________________
> 
> 	w/  patch eCLK: ___||||||||____________________|||||||||||||||||
> 	w/  patch nRST: ----__--------------------------__--------------
> 	w/  patch CONF: _______XX__________________________XX___________
> 			  ^                           ^
> 			  ^--fec_probe()              ^--fec_enet_open()
> 
> In our case 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
> hundred PORs of our board.
> 
> Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to sa...")
> Signed-off-by: Richard Leitner <richard.leitner-WcANXNA0UjBBDgjK7y7TUQ@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/net/fsl-fec.txt |  3 +++
>  drivers/net/ethernet/freescale/fec.h              |  1 +
>  drivers/net/ethernet/freescale/fec_main.c         | 16 ++++++++++++++++
>  3 files changed, 20 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/fsl-fec.txt b/Documentation/devicetree/bindings/net/fsl-fec.txt
> index 6f55bdd..1766579 100644
> --- a/Documentation/devicetree/bindings/net/fsl-fec.txt
> +++ b/Documentation/devicetree/bindings/net/fsl-fec.txt
> @@ -23,6 +23,9 @@ Optional properties:
>  - phy-handle : phandle to the PHY device connected to this device.
>  - fixed-link : Assume a fixed link. See fixed-link.txt in the same directory.
>    Use instead of phy-handle.
> +- phy-reset-after-clk-enable : If present then a phy reset and configuration
> +  will be performed everytime after the clocks are enabled. This is required
> +  for some PHYs to work properly.

Maybe this is not needed based on the discussion, but just to make 
sure. Since this is a property of the phy, it should be implied from the 
phy's compatible string.

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