| Message ID | 20171120083417.32558-4-dev@g0hl1n.net |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: ethernet: fec: fix refclk enable for SMSC LAN8710/20 | expand |
From: Richard Leitner <dev@g0hl1n.net> Sent: Monday, November 20, 2017 4:34 PM >To: f.fainelli@gmail.com; Andy Duan <fugang.duan@nxp.com>; >andrew@lunn.ch >Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; >richard.leitner@skidata.com >Subject: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC >LAN8710/20 > >From: Richard Leitner <richard.leitner@skidata.com> > >Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow turning >the refclk on and off again during operation (according to their datasheet). >Nonetheless exactly this behaviour was introduced for power saving reasons >by commit e8fcfcd5684a ("net: fec: optimize the clock management to save >power"). >Therefore after enabling the refclk we detect if an affected PHY is attached. If >so reset and initialize it again. > >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() > >Generally speaking this issue is only relevant if the ref clk for the PHY is >generated by the SoC. In our specific case (PCB) this problem does occur at >about every 10th to 50th POR of an LAN8710 connected to an i.MX6DL SoC. >The typical symptom of this problem is a "swinging" >ethernet link. Similar issues were reported by users of the NXP forum: > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F >%2Fcommunity.nxp.com%2Fthread%2F389902&data=02%7C01%7Cfugang.du >an%40nxp.com%7C507c7aafbd4744f81ebf08d52ff1acff%7C686ea1d3bc2b4c6f >a92cd99c5c301635%7C0%7C0%7C636467637399145483&sdata=RNXVGpPrlrcyL >0SoQl8%2BI0k8Oc8BM0Iwykd1O%2Bjmvcc%3D&reserved=0 > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F >%2Fcommunity.nxp.com%2Fmessage%2F309354&data=02%7C01%7Cfugang.d >uan%40nxp.com%7C507c7aafbd4744f81ebf08d52ff1acff%7C686ea1d3bc2b4c6 >fa92cd99c5c301635%7C0%7C0%7C636467637399145483&sdata=pjeJEZGuBpb9 >uCMKGr70qa%2FmsNoak6v3nCID2vbNAeg%3D&reserved=0 >With this patch applied the issue didn't occur for at least a few hundret PORs >of our board. > >Fixes: e8fcfcd5684a ("net: fec: optimize the clock management to save >power") >Signed-off-by: Richard Leitner <richard.leitner@skidata.com> >--- > drivers/net/ethernet/freescale/fec_main.c | 37 >+++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > >diff --git a/drivers/net/ethernet/freescale/fec_main.c >b/drivers/net/ethernet/freescale/fec_main.c >index 06a7caca0cee..52ec9b29a70e 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" > >@@ -1833,6 +1834,32 @@ static int fec_enet_mdio_write(struct mii_bus *bus, >int mii_id, int regnum, > return ret; > } > >+static int fec_enet_clk_ref_enable_reset_phy_quirk(struct net_device >+*ndev) { >+ struct phy_device *phy_dev = ndev->phydev; >+ u32 real_phy_id; >+ int ret; >+ >+ /* some PHYs need a reset after the refclk was enabled, so we >+ * reset them here >+ */ >+ if (!phy_dev) >+ return 0; >+ if (!phy_dev->drv) >+ return 0; >+ real_phy_id = phy_dev->drv->phy_id & phy_dev->drv->phy_id_mask; >+ switch (real_phy_id) { >+ case 0x0007c0f0: /* SMSC LAN8710/LAN8720 */ Don't hard code here... I believe there have many other phys also do such operation, hardcode is unacceptable... And these code can be put into phy_device.c as common interface. >+ ret = fec_reset_phy(ndev); >+ if (ret) >+ return ret; >+ ret = phy_init_hw(phy_dev); >+ if (ret) >+ return ret; >+ } >+ return 0; >+} >+ > static int fec_enet_clk_enable(struct net_device *ndev, bool enable) { > struct fec_enet_private *fep = netdev_priv(ndev); @@ -1862,6 >+1889,10 @@ 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; >+ >+ ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev); >+ if (ret) >+ netdev_warn(ndev, "Resetting PHY failed, connection >may be >+unstable\n"); > } else { > clk_disable_unprepare(fep->clk_ahb); > clk_disable_unprepare(fep->clk_enet_out); >@@ -2860,11 +2891,17 @@ fec_enet_open(struct net_device *ndev) > if (ret) > goto err_enet_mii_probe; > >+ /* as the PHY is connected now, trigger the reset quirk again */ >+ ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev); >+ if (ret) >+ netdev_warn(ndev, "Resetting PHY failed, connection may be >+unstable\n"); >+ > if (fep->quirks & FEC_QUIRK_ERR006687) > imx6q_cpuidle_fec_irqs_used(); > > napi_enable(&fep->napi); > phy_start(ndev->phydev); >+ No need blank line here... > netif_tx_start_all_queues(ndev); > > device_set_wakeup_enable(&ndev->dev, fep->wol_flag & >-- >2.11.0
On 11/20/2017 10:47 AM, Andy Duan wrote: > From: Richard Leitner <dev@g0hl1n.net> Sent: Monday, November 20, 2017 4:34 PM >> To: f.fainelli@gmail.com; Andy Duan <fugang.duan@nxp.com>; >> andrew@lunn.ch >> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; >> richard.leitner@skidata.com >> Subject: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC >> LAN8710/20 >> >> From: Richard Leitner <richard.leitner@skidata.com> >> >> Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow turning >> the refclk on and off again during operation (according to their datasheet). >> Nonetheless exactly this behaviour was introduced for power saving reasons >> by commit e8fcfcd5684a ("net: fec: optimize the clock management to save >> power"). >> Therefore after enabling the refclk we detect if an affected PHY is attached. If >> so reset and initialize it again. ... >> +static int fec_enet_clk_ref_enable_reset_phy_quirk(struct net_device >> +*ndev) { >> + struct phy_device *phy_dev = ndev->phydev; >> + u32 real_phy_id; >> + int ret; >> + >> + /* some PHYs need a reset after the refclk was enabled, so we >> + * reset them here >> + */ >> + if (!phy_dev) >> + return 0; >> + if (!phy_dev->drv) >> + return 0; >> + real_phy_id = phy_dev->drv->phy_id & phy_dev->drv->phy_id_mask; >> + switch (real_phy_id) { >> + case 0x0007c0f0: /* SMSC LAN8710/LAN8720 */ > > Don't hard code here... > I believe there have many other phys also do such operation, hardcode is unacceptable... > > And these code can be put into phy_device.c as common interface. Ok. Thank you for the feedback. So it would be fine to hardcode the affected phy_id's in a common function in phy_device.c? Another possible solution that came to my mind is to add a flag called something like "PHY_RST_AFTER_CLK_EN" to the flags variable in struct phy_driver. This flag could then be set in the smsc PHY driver for affected PHYs. Then instead of comparing the phy_id in the MAC driver this flag could be checked: if (phydev->drv->flags & PHY_RST_AFTER_CLK_EN) { ret = fec_reset_phy(ndev); ... } Would checking the flag be OK in fec_main.c? What would be the "better" approach? > >> + ret = fec_reset_phy(ndev); >> + if (ret) >> + return ret; >> + ret = phy_init_hw(phy_dev); >> + if (ret) >> + return ret; >> + } >> + return 0; >> +} >> + >> static int fec_enet_clk_enable(struct net_device *ndev, bool enable) { >> struct fec_enet_private *fep = netdev_priv(ndev); @@ -1862,6 >> +1889,10 @@ 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; >> + >> + ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev); >> + if (ret) >> + netdev_warn(ndev, "Resetting PHY failed, connection >> may be >> +unstable\n"); >> } else { >> clk_disable_unprepare(fep->clk_ahb); >> clk_disable_unprepare(fep->clk_enet_out); >> @@ -2860,11 +2891,17 @@ fec_enet_open(struct net_device *ndev) >> if (ret) >> goto err_enet_mii_probe; >> >> + /* as the PHY is connected now, trigger the reset quirk again */ >> + ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev); >> + if (ret) >> + netdev_warn(ndev, "Resetting PHY failed, connection may be >> +unstable\n"); >> + >> if (fep->quirks & FEC_QUIRK_ERR006687) >> imx6q_cpuidle_fec_irqs_used(); >> >> napi_enable(&fep->napi); >> phy_start(ndev->phydev); >> + > > No need blank line here... >> netif_tx_start_all_queues(ndev); >> >> device_set_wakeup_enable(&ndev->dev, fep->wol_flag & >> -- >> 2.11.0
From: Richard Leitner <richard.leitner@skidata.com> Sent: Monday, November 20, 2017 5:57 PM >To: Andy Duan <fugang.duan@nxp.com>; f.fainelli@gmail.com; >andrew@lunn.ch >Cc: Richard Leitner <dev@g0hl1n.net>; netdev@vger.kernel.org; linux- >kernel@vger.kernel.org >Subject: Re: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC >LAN8710/20 > > >On 11/20/2017 10:47 AM, Andy Duan wrote: >> From: Richard Leitner <dev@g0hl1n.net> Sent: Monday, November 20, 2017 >> 4:34 PM >>> To: f.fainelli@gmail.com; Andy Duan <fugang.duan@nxp.com>; >>> andrew@lunn.ch >>> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; >>> richard.leitner@skidata.com >>> Subject: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for >>> SMSC >>> LAN8710/20 >>> >>> From: Richard Leitner <richard.leitner@skidata.com> >>> >>> Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow >>> turning the refclk on and off again during operation (according to their >datasheet). >>> Nonetheless exactly this behaviour was introduced for power saving >>> reasons by commit e8fcfcd5684a ("net: fec: optimize the clock >>> management to save power"). >>> Therefore after enabling the refclk we detect if an affected PHY is >>> attached. If so reset and initialize it again. > >... > >>> +static int fec_enet_clk_ref_enable_reset_phy_quirk(struct net_device >>> +*ndev) { >>> + struct phy_device *phy_dev = ndev->phydev; >>> + u32 real_phy_id; >>> + int ret; >>> + >>> + /* some PHYs need a reset after the refclk was enabled, so we >>> + * reset them here >>> + */ >>> + if (!phy_dev) >>> + return 0; >>> + if (!phy_dev->drv) >>> + return 0; >>> + real_phy_id = phy_dev->drv->phy_id & phy_dev->drv->phy_id_mask; >>> + switch (real_phy_id) { >>> + case 0x0007c0f0: /* SMSC LAN8710/LAN8720 */ >> >> Don't hard code here... >> I believe there have many other phys also do such operation, hardcode is >unacceptable... >> >> And these code can be put into phy_device.c as common interface. > >Ok. Thank you for the feedback. >So it would be fine to hardcode the affected phy_id's in a common function in >phy_device.c? > > >Another possible solution that came to my mind is to add a flag called >something like "PHY_RST_AFTER_CLK_EN" to the flags variable in struct >phy_driver. This flag could then be set in the smsc PHY driver for affected >PHYs. > >Then instead of comparing the phy_id in the MAC driver this flag could be >checked: > >if (phydev->drv->flags & PHY_RST_AFTER_CLK_EN) { > ret = fec_reset_phy(ndev); > ... >} > >Would checking the flag be OK in fec_main.c? Yes, it is better than previous solution. But add new common API in phy_device.c is much better like: 1. add a flag called "PHY_RST_AFTER_CLK_EN" to the flags variable in struct phy_driver, all phy driver that need reset can set the flag. 2. add new common api interface phy_reset_after_clk_enable() in phy_device.c driver 3. add reset gpio descriptor for common phy device driver. 4. then any mac driver can directly call the common interface .phy_reset_after_clk_enable(). That is only my suggestion, maybe there have better idea. Thanks. > >What would be the "better" approach? > >> >>> + ret = fec_reset_phy(ndev); >>> + if (ret) >>> + return ret; >>> + ret = phy_init_hw(phy_dev); >>> + if (ret) >>> + return ret; >>> + } >>> + return 0; >>> +} >>> + >>> static int fec_enet_clk_enable(struct net_device *ndev, bool enable) { >>> struct fec_enet_private *fep = netdev_priv(ndev); @@ -1862,6 >>> +1889,10 @@ 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; >>> + >>> + ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev); >>> + if (ret) >>> + netdev_warn(ndev, "Resetting PHY failed, connection >>> may be >>> +unstable\n"); >>> } else { >>> clk_disable_unprepare(fep->clk_ahb); >>> clk_disable_unprepare(fep->clk_enet_out); >>> @@ -2860,11 +2891,17 @@ fec_enet_open(struct net_device *ndev) >>> if (ret) >>> goto err_enet_mii_probe; >>> >>> + /* as the PHY is connected now, trigger the reset quirk again */ >>> + ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev); >>> + if (ret) >>> + netdev_warn(ndev, "Resetting PHY failed, connection may be >>> +unstable\n"); >>> + >>> if (fep->quirks & FEC_QUIRK_ERR006687) >>> imx6q_cpuidle_fec_irqs_used(); >>> >>> napi_enable(&fep->napi); >>> phy_start(ndev->phydev); >>> + >> >> No need blank line here... >>> netif_tx_start_all_queues(ndev); >>> >>> device_set_wakeup_enable(&ndev->dev, fep->wol_flag & >>> -- >>> 2.11.0
On 11/20/2017 11:35 AM, Andy Duan wrote: > From: Richard Leitner <richard.leitner@skidata.com> Sent: Monday, November 20, 2017 5:57 PM >> To: Andy Duan <fugang.duan@nxp.com>; f.fainelli@gmail.com; >> andrew@lunn.ch >> Cc: Richard Leitner <dev@g0hl1n.net>; netdev@vger.kernel.org; linux- >> kernel@vger.kernel.org >> Subject: Re: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC >> LAN8710/20 >> >> >> On 11/20/2017 10:47 AM, Andy Duan wrote: >>> From: Richard Leitner <dev@g0hl1n.net> Sent: Monday, November 20, 2017 >>> 4:34 PM >>>> To: f.fainelli@gmail.com; Andy Duan <fugang.duan@nxp.com>; >>>> andrew@lunn.ch >>>> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; >>>> richard.leitner@skidata.com >>>> Subject: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for >>>> SMSC >>>> LAN8710/20 >>>> >>>> From: Richard Leitner <richard.leitner@skidata.com> >>>> >>>> Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow >>>> turning the refclk on and off again during operation (according to their >> datasheet). >>>> Nonetheless exactly this behaviour was introduced for power saving >>>> reasons by commit e8fcfcd5684a ("net: fec: optimize the clock >>>> management to save power"). >>>> Therefore after enabling the refclk we detect if an affected PHY is >>>> attached. If so reset and initialize it again. >> ... >> >>>> +static int fec_enet_clk_ref_enable_reset_phy_quirk(struct net_device >>>> +*ndev) { >>>> + struct phy_device *phy_dev = ndev->phydev; >>>> + u32 real_phy_id; >>>> + int ret; >>>> + >>>> + /* some PHYs need a reset after the refclk was enabled, so we >>>> + * reset them here >>>> + */ >>>> + if (!phy_dev) >>>> + return 0; >>>> + if (!phy_dev->drv) >>>> + return 0; >>>> + real_phy_id = phy_dev->drv->phy_id & phy_dev->drv->phy_id_mask; >>>> + switch (real_phy_id) { >>>> + case 0x0007c0f0: /* SMSC LAN8710/LAN8720 */ >>> Don't hard code here... >>> I believe there have many other phys also do such operation, hardcode is >> unacceptable... >>> And these code can be put into phy_device.c as common interface. >> Ok. Thank you for the feedback. >> So it would be fine to hardcode the affected phy_id's in a common function in >> phy_device.c? >> >> >> Another possible solution that came to my mind is to add a flag called >> something like "PHY_RST_AFTER_CLK_EN" to the flags variable in struct >> phy_driver. This flag could then be set in the smsc PHY driver for affected >> PHYs. >> >> Then instead of comparing the phy_id in the MAC driver this flag could be >> checked: >> >> if (phydev->drv->flags & PHY_RST_AFTER_CLK_EN) { >> ret = fec_reset_phy(ndev); >> ... >> } >> >> Would checking the flag be OK in fec_main.c? > Yes, it is better than previous solution. > But add new common API in phy_device.c is much better like: > 1. add a flag called "PHY_RST_AFTER_CLK_EN" to the flags variable in struct phy_driver, all phy driver that need reset can set the flag. OK. > 2. add new common api interface phy_reset_after_clk_enable() in phy_device.c driver OK. But see below... > 3. add reset gpio descriptor for common phy device driver. ... if I understood it correctly the patch called "Teach phylib hard-resetting devices" by Geert and Sergei is exactly doing this: https://patchwork.ozlabs.org/cover/828503/ https://lkml.org/lkml/2017/10/20/166 So I'll implement the phy_reset_after_clk_enable function atop of this patch-set and add a note that my patch-series depends on it. Would that be OK? > 4. then any mac driver can directly call the common interface .phy_reset_after_clk_enable(). Sounds reasonable :-) > > That is only my suggestion, maybe there have better idea. > Thanks. > Thanks for your quick feedback. regards Richard.L
Hi Richard, On Mon, Nov 20, 2017 at 1:55 PM, Richard Leitner <richard.leitner@skidata.com> wrote: > On 11/20/2017 11:35 AM, Andy Duan wrote: >> 3. add reset gpio descriptor for common phy device driver. > > ... if I understood it correctly the patch called "Teach phylib > hard-resetting devices" by Geert and Sergei is exactly doing this: > https://patchwork.ozlabs.org/cover/828503/ > https://lkml.org/lkml/2017/10/20/166 > > So I'll implement the phy_reset_after_clk_enable function atop of this > patch-set and add a note that my patch-series depends on it. Would that > be OK? I will update and respin that patch series after the merge window has closed. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 11/20/2017 02:13 PM, Geert Uytterhoeven wrote: > Hi Richard, > > On Mon, Nov 20, 2017 at 1:55 PM, Richard Leitner > <richard.leitner@skidata.com> wrote: >> On 11/20/2017 11:35 AM, Andy Duan wrote: >>> 3. add reset gpio descriptor for common phy device driver. >> >> ... if I understood it correctly the patch called "Teach phylib >> hard-resetting devices" by Geert and Sergei is exactly doing this: >> https://patchwork.ozlabs.org/cover/828503/ >> https://lkml.org/lkml/2017/10/20/166 >> >> So I'll implement the phy_reset_after_clk_enable function atop of this >> patch-set and add a note that my patch-series depends on it. Would that >> be OK? > > I will update and respin that patch series after the merge window has closed. Ok. Thank you for the quick response an this information. For the Freescale Fast Ethernet Controller (FEC) there are currently (in addition to the reset gpio) two additional optional dt properties for the reset: - phy-reset-duration : Reset duration in milliseconds. - phy-reset-post-delay : Post reset delay in milliseconds. IMHO it would make sense to include them also in the phylib implementation. What do you think about it? Should I include it in my patch-series? kind regards; Richard.L > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds >
Hi Richard, On Mon, Nov 20, 2017 at 2:21 PM, Richard Leitner <richard.leitner@skidata.com> wrote: > On 11/20/2017 02:13 PM, Geert Uytterhoeven wrote: >> On Mon, Nov 20, 2017 at 1:55 PM, Richard Leitner >> <richard.leitner@skidata.com> wrote: >>> On 11/20/2017 11:35 AM, Andy Duan wrote: >>>> 3. add reset gpio descriptor for common phy device driver. >>> >>> ... if I understood it correctly the patch called "Teach phylib >>> hard-resetting devices" by Geert and Sergei is exactly doing this: >>> https://patchwork.ozlabs.org/cover/828503/ >>> https://lkml.org/lkml/2017/10/20/166 >>> >>> So I'll implement the phy_reset_after_clk_enable function atop of this >>> patch-set and add a note that my patch-series depends on it. Would that >>> be OK? >> >> I will update and respin that patch series after the merge window has closed. > > Ok. Thank you for the quick response an this information. > > For the Freescale Fast Ethernet Controller (FEC) there are currently (in > addition to the reset gpio) two additional optional dt properties for > the reset: > - phy-reset-duration : Reset duration in milliseconds. > - phy-reset-post-delay : Post reset delay in milliseconds. > > IMHO it would make sense to include them also in the phylib > implementation. What do you think about it? Should I include it in my > patch-series? Sure, you can always extend phylib, building on top of our simple reset implementation. BTW, I think phy-reset-{duration,post-delay} should be moved to the phy node as well, dropping the "phy-" prefix in the process. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
From: Richard Leitner <richard.leitner@skidata.com> Sent: Monday, November 20, 2017 8:55 PM >On 11/20/2017 11:35 AM, Andy Duan wrote: >> From: Richard Leitner <richard.leitner@skidata.com> Sent: Monday, >> November 20, 2017 5:57 PM >>> To: Andy Duan <fugang.duan@nxp.com>; f.fainelli@gmail.com; >>> andrew@lunn.ch >>> Cc: Richard Leitner <dev@g0hl1n.net>; netdev@vger.kernel.org; linux- >>> kernel@vger.kernel.org >>> Subject: Re: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for >>> SMSC >>> LAN8710/20 >>> >>> >>> On 11/20/2017 10:47 AM, Andy Duan wrote: >>>> From: Richard Leitner <dev@g0hl1n.net> Sent: Monday, November 20, >>>> 2017 >>>> 4:34 PM >>>>> To: f.fainelli@gmail.com; Andy Duan <fugang.duan@nxp.com>; >>>>> andrew@lunn.ch >>>>> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; >>>>> richard.leitner@skidata.com >>>>> Subject: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for >>>>> SMSC >>>>> LAN8710/20 >>>>> >>>>> From: Richard Leitner <richard.leitner@skidata.com> >>>>> >>>>> Some PHYs (for example the SMSC LAN8710/LAN8720) doesn't allow >>>>> turning the refclk on and off again during operation (according to >>>>> their >>> datasheet). >>>>> Nonetheless exactly this behaviour was introduced for power saving >>>>> reasons by commit e8fcfcd5684a ("net: fec: optimize the clock >>>>> management to save power"). >>>>> Therefore after enabling the refclk we detect if an affected PHY is >>>>> attached. If so reset and initialize it again. >>> ... >>> >>>>> +static int fec_enet_clk_ref_enable_reset_phy_quirk(struct >>>>> +net_device >>>>> +*ndev) { >>>>> + struct phy_device *phy_dev = ndev->phydev; >>>>> + u32 real_phy_id; >>>>> + int ret; >>>>> + >>>>> + /* some PHYs need a reset after the refclk was enabled, so we >>>>> + * reset them here >>>>> + */ >>>>> + if (!phy_dev) >>>>> + return 0; >>>>> + if (!phy_dev->drv) >>>>> + return 0; >>>>> + real_phy_id = phy_dev->drv->phy_id & phy_dev->drv->phy_id_mask; >>>>> + switch (real_phy_id) { >>>>> + case 0x0007c0f0: /* SMSC LAN8710/LAN8720 */ >>>> Don't hard code here... >>>> I believe there have many other phys also do such operation, >>>> hardcode is >>> unacceptable... >>>> And these code can be put into phy_device.c as common interface. >>> Ok. Thank you for the feedback. >>> So it would be fine to hardcode the affected phy_id's in a common >>> function in phy_device.c? >>> >>> >>> Another possible solution that came to my mind is to add a flag >>> called something like "PHY_RST_AFTER_CLK_EN" to the flags variable in >>> struct phy_driver. This flag could then be set in the smsc PHY driver >>> for affected PHYs. >>> >>> Then instead of comparing the phy_id in the MAC driver this flag >>> could be >>> checked: >>> >>> if (phydev->drv->flags & PHY_RST_AFTER_CLK_EN) { >>> ret = fec_reset_phy(ndev); >>> ... >>> } >>> >>> Would checking the flag be OK in fec_main.c? >> Yes, it is better than previous solution. >> But add new common API in phy_device.c is much better like: >> 1. add a flag called "PHY_RST_AFTER_CLK_EN" to the flags variable in struct >phy_driver, all phy driver that need reset can set the flag. > >OK. > >> 2. add new common api interface phy_reset_after_clk_enable() in >> phy_device.c driver > >OK. But see below... > >> 3. add reset gpio descriptor for common phy device driver. > >... if I understood it correctly the patch called "Teach phylib hard-resetting >devices" by Geert and Sergei is exactly doing this: > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F >%2Fpatchwork.ozlabs.org%2Fcover%2F828503%2F&data=02%7C01%7Cfugang >.duan%40nxp.com%7C087010d8b4784e3c88c608d53015f2ad%7C686ea1d3bc2 >b4c6fa92cd99c5c301635%7C0%7C0%7C636467793180108636&sdata=yY9thX8Q >CCVteoF5vvUoAYYxGH0gg4wOUq7TQKtkiok%3D&reserved=0 > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F >%2Flkml.org%2Flkml%2F2017%2F10%2F20%2F166&data=02%7C01%7Cfugang. >duan%40nxp.com%7C087010d8b4784e3c88c608d53015f2ad%7C686ea1d3bc2b >4c6fa92cd99c5c301635%7C0%7C0%7C636467793180108636&sdata=rxV12dum1 >VmbWLWvSACDuZevFSFbUoWr9AiUtVSsV6w%3D&reserved=0 > >So I'll implement the phy_reset_after_clk_enable function atop of this patch- >set and add a note that my patch-series depends on it. Would that be OK? > Yes, it is the best solution. >> 4. then any mac driver can directly call the common >interface .phy_reset_after_clk_enable(). > >Sounds reasonable :-) > >> >> That is only my suggestion, maybe there have better idea. >> Thanks. >> > >Thanks for your quick feedback. > >regards >Richard.L
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 06a7caca0cee..52ec9b29a70e 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" @@ -1833,6 +1834,32 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum, return ret; } +static int fec_enet_clk_ref_enable_reset_phy_quirk(struct net_device *ndev) +{ + struct phy_device *phy_dev = ndev->phydev; + u32 real_phy_id; + int ret; + + /* some PHYs need a reset after the refclk was enabled, so we + * reset them here + */ + if (!phy_dev) + return 0; + if (!phy_dev->drv) + return 0; + real_phy_id = phy_dev->drv->phy_id & phy_dev->drv->phy_id_mask; + switch (real_phy_id) { + case 0x0007c0f0: /* SMSC LAN8710/LAN8720 */ + ret = fec_reset_phy(ndev); + if (ret) + return ret; + ret = phy_init_hw(phy_dev); + if (ret) + return ret; + } + return 0; +} + static int fec_enet_clk_enable(struct net_device *ndev, bool enable) { struct fec_enet_private *fep = netdev_priv(ndev); @@ -1862,6 +1889,10 @@ 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; + + ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev); + if (ret) + netdev_warn(ndev, "Resetting PHY failed, connection may be unstable\n"); } else { clk_disable_unprepare(fep->clk_ahb); clk_disable_unprepare(fep->clk_enet_out); @@ -2860,11 +2891,17 @@ fec_enet_open(struct net_device *ndev) if (ret) goto err_enet_mii_probe; + /* as the PHY is connected now, trigger the reset quirk again */ + ret = fec_enet_clk_ref_enable_reset_phy_quirk(ndev); + if (ret) + netdev_warn(ndev, "Resetting PHY failed, connection may be unstable\n"); + if (fep->quirks & FEC_QUIRK_ERR006687) imx6q_cpuidle_fec_irqs_used(); napi_enable(&fep->napi); phy_start(ndev->phydev); + netif_tx_start_all_queues(ndev); device_set_wakeup_enable(&ndev->dev, fep->wol_flag &