From: Andy Duan <fugang.duan@nxp.com>
To: Richard Leitner <richard.leitner@skidata.com>,
        Richard Leitner <dev@g0hl1n.net>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "mark.rutland@arm.com" <mark.rutland@arm.com>,
        "andrew@lunn.ch" <andrew@lunn.ch>,
        "f.fainelli@gmail.com" <f.fainelli@gmail.com>,
        "frowand.list@gmail.com" <frowand.list@gmail.com>
CC: "davem@davemloft.net" <davem@davemloft.net>,
        "geert+renesas@glider.be" <geert+renesas@glider.be>,
        "sergei.shtylyov@cogentembedded.com" 
        <sergei.shtylyov@cogentembedded.com>,
        "baruch@tkos.co.il" <baruch@tkos.co.il>,
        "david.wu@rock-chips.com" <david.wu@rock-chips.com>,
        "lukma@denx.de" <lukma@denx.de>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH net-next v3 4/4] net: fec: add
 phy_reset_after_clk_enable() support
Thread-Topic: [PATCH net-next v3 4/4] net: fec: add
 phy_reset_after_clk_enable() support
Thread-Index: AQHTbcywCErLmvbit0yBnahadCjONaM1im7wgABt0QCAAAe0sA==
Date: Wed, 6 Dec 2017 08:41:03 +0000
Message-ID: <HE1PR0401MB2265094A8760DE458ED57A2FFF320@HE1PR0401MB2265.eurprd04.prod.outlook.com>
References: <20171205132600.13796-1-dev@g0hl1n.net>
 <20171205132600.13796-5-dev@g0hl1n.net>
 <DB6PR0401MB2264B5060F745CEB5405FFBCFF320@DB6PR0401MB2264.eurprd04.prod.outlook.com>
 <1e7b3e7f-251d-4101-1441-702e891f2ca9@skidata.com>
In-Reply-To: <1e7b3e7f-251d-4101-1441-702e891f2ca9@skidata.com>
Accept-Language: zh-CN, en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 20da6502-0176-4548-55a6-08d53c8515a0
X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Dec 2017 08:41:03.2376
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR0401MB2267
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by nfs id vB68fH3R024600
Content-Length: 2632
Lines: 68

From: Richard Leitner <richard.leitner@skidata.com> Sent: Wednesday, December 06, 2017 4:12 PM
>To: Andy Duan <fugang.duan@nxp.com>; Richard Leitner <dev@g0hl1n.net>;
>robh+dt@kernel.org; mark.rutland@arm.com; andrew@lunn.ch;
>f.fainelli@gmail.com; frowand.list@gmail.com
>Cc: davem@davemloft.net; geert+renesas@glider.be;
>sergei.shtylyov@cogentembedded.com; baruch@tkos.co.il; david.wu@rock-
>chips.com; lukma@denx.de; netdev@vger.kernel.org;
>devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH net-next v3 4/4] net: fec: add
>phy_reset_after_clk_enable() support
>
>Hi Andy,
>
>On 12/06/2017 02:50 AM, Andy Duan wrote:
>> From: Richard Leitner <dev@g0hl1n.net> Sent: Tuesday, December 05,
>> 2017 9:26 PM
>>> 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 add support for the phy_reset_after_clk_enable function
>>> from phylib to mitigate this issue.
>
>...
>
>>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>>> b/drivers/net/ethernet/freescale/fec_main.c
>>> index 610573855213..8c3d0fb7db20 100644
>>> --- a/drivers/net/ethernet/freescale/fec_main.c
>>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>>> @@ -1862,6 +1862,8 @@ 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;
>>> +
>>> +		phy_reset_after_clk_enable(ndev->phydev);
>>> 	} else {
>>> 		clk_disable_unprepare(fep->clk_ahb);
>>> 		clk_disable_unprepare(fep->clk_enet_out);
>>> @@ -2860,6 +2862,11 @@ fec_enet_open(struct net_device *ndev)
>>> 	if (ret)
>>> 		goto err_enet_mii_probe;
>>>
>>> +	/* reset phy if needed here, due to the fact this is the first time we
>>> +	 * have the net_device to phy_driver link
>>> +	 */
>>> +	phy_reset_after_clk_enable(ndev->phydev);
>>> +
>>
>> The patch series look better.
>> But why does it need to reset phy here since phy already is hard reset after
>clock enable.
>
>The problem here is that in fec_enet_open() the fec_enet_clk_enable() call is
>done before the phy is probed. Therefore (as mentioned in the
>comment) there's no link from the net_device (ndev) to the phy_driver
>(which holds the flags).
>
>Any suggestions for a better solution are highly appreciated here! Thanks!
>
>regards;Richard.L

Okay, I see.

For the patch: Acked-by: Fugang Duan <fugang.duan@nxp.com>