From: Richard Leitner <richard.leit...@skidata.com> Sent: Monday, November 20, 2017 5:57 PM >To: Andy Duan <fugang.d...@nxp.com>; f.faine...@gmail.com; >and...@lunn.ch >Cc: Richard Leitner <d...@g0hl1n.net>; netdev@vger.kernel.org; linux- >ker...@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 <d...@g0hl1n.net> Sent: Monday, November 20, 2017 >> 4:34 PM >>> To: f.faine...@gmail.com; Andy Duan <fugang.d...@nxp.com>; >>> and...@lunn.ch >>> Cc: netdev@vger.kernel.org; linux-ker...@vger.kernel.org; >>> richard.leit...@skidata.com >>> Subject: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for >>> SMSC >>> LAN8710/20 >>> >>> From: Richard Leitner <richard.leit...@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