Messages in this thread | | | Subject | Re: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC LAN8710/20 | From | Richard Leitner <> | Date | Mon, 20 Nov 2017 13:55:14 +0100 |
| |
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
| |