RE: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for SMSC LAN8710/20

[Andy Duan]

From: Richard Leitner <richard.leitner@xxxxxxxxxxx> Sent: Monday, November 20, 2017 5:57 PM
>To: Andy Duan <fugang.duan@xxxxxxx>; f.fainelli@xxxxxxxxx;
>andrew@xxxxxxx
>Cc: Richard Leitner <dev@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx
>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@xxxxxxxxxx> Sent: Monday, November 20, 2017
>> 4:34 PM
>>> To: f.fainelli@xxxxxxxxx; Andy Duan <fugang.duan@xxxxxxx>;
>>> andrew@xxxxxxx
>>> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>>> richard.leitner@xxxxxxxxxxx
>>> Subject: [PATCH v2 3/3] net: ethernet: fec: fix refclk enable for
>>> SMSC
>>> LAN8710/20
>>>
>>> From: Richard Leitner <richard.leitner@xxxxxxxxxxx>
>>>
>>> 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