| Message ID | 1499346330-12166-2-git-send-email-richard.leitner@skidata.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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
From: Richard Leitner <richard.leitner@skidata.com> Sent: Thursday, July 06, 2017 9:06 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; Richard Leitner ><richard.leitner@skidata.com> >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@skidata.com> >--- > 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
On 07/07/2017 07:30 AM, Andy Duan wrote: > From: Richard Leitner <richard.leitner@skidata.com> Sent: Thursday, July 06, 2017 9:06 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; Richard Leitner >> <richard.leitner@skidata.com> >> 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@skidata.com> ... >> *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
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
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
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
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?
> 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
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@skidata.com> > --- > 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
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
> 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
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);
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@skidata.com> --- 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(+)