2017-07-06 13:16:25

by Richard Leitner

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

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 <[email protected]>
---
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


2017-07-06 13:15:49

by Richard Leitner

[permalink] [raw]
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 <[email protected]>
---
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

2017-07-06 13:56:04

by Andrew Lunn

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

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

2017-07-06 14:33:53

by Richard Leitner

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


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

2017-07-07 05:30:48

by Andy Duan

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

From: Richard Leitner <[email protected]> Sent: Thursday, July 06, 2017 9:06 PM
>To: Andy Duan <[email protected]>; [email protected];
>[email protected]
>Cc: [email protected]; [email protected]; linux-
>[email protected]; [email protected]; Richard Leitner
><[email protected]>
>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 <[email protected]>
>---
> 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

2017-07-07 06:00:56

by Richard Leitner

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


On 07/07/2017 07:30 AM, Andy Duan wrote:
> From: Richard Leitner <[email protected]> Sent: Thursday, July 06, 2017 9:06 PM
>> To: Andy Duan <[email protected]>; [email protected];
>> [email protected]
>> Cc: [email protected]; [email protected]; linux-
>> [email protected]; [email protected]; Richard Leitner
>> <[email protected]>
>> 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 <[email protected]>

...

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

2017-07-07 07:03:07

by Andy Duan

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

From: Richard Leitner <[email protected]> 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

2017-07-07 09:53:06

by Richard Leitner

[permalink] [raw]
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 <[email protected]> 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

2017-07-07 11:08:19

by Andy Duan

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

From: Richard Leitner <[email protected]> Sent: Friday, July 07, 2017 5:53 PM
>To: Andy Duan <[email protected]>; [email protected];
>[email protected]
>Cc: [email protected]; [email protected]; linux-
>[email protected]; [email protected]; Andrew Lunn <[email protected]>
>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 <[email protected]> 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

2017-07-07 11:17:00

by Richard Leitner

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

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?

2017-07-07 14:00:44

by Andrew Lunn

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

> 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




2017-07-10 13:26:52

by Rob Herring

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

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 <[email protected]>
> ---
> 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

2017-07-12 09:31:15

by Richard Leitner

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


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

2017-07-12 13:40:23

by Andrew Lunn

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

> 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