Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756104AbdGLJbP (ORCPT ); Wed, 12 Jul 2017 05:31:15 -0400 Received: from mail2.skidata.com ([91.230.2.91]:19794 "EHLO mail2.skidata.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750738AbdGLJbN (ORCPT ); Wed, 12 Jul 2017 05:31:13 -0400 X-Greylist: delayed 604 seconds by postgrey-1.27 at vger.kernel.org; Wed, 12 Jul 2017 05:31:13 EDT X-IronPort-AV: E=Sophos;i="5.40,349,1496095200"; d="scan'208";a="772175" Subject: Re: [PATCH 2/2] net: ethernet: fsl: add phy reset after clk enable option To: Andrew Lunn , Andy Duan , "robh+dt@kernel.org" CC: "mark.rutland@arm.com" , "netdev@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "dev@g0hl1n.net" , Richard Leitner References: <1499346330-12166-1-git-send-email-richard.leitner@skidata.com> <1499346330-12166-2-git-send-email-richard.leitner@skidata.com> <81105c77-d48f-271b-2de1-c877b9413184@skidata.com> <6de114cb-4521-4bb2-d0a3-4aea32936bd3@skidata.com> <20170707140010.GD24237@lunn.ch> From: Richard Leitner Message-ID: Date: Wed, 12 Jul 2017 11:21:06 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170707140010.GD24237@lunn.ch> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit X-Originating-IP: [172.16.60.30] X-ClientProxiedBy: sdex1srv.skidata.net (172.16.10.92) To sdex1srv.skidata.net (172.16.10.92) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2378 Lines: 60 On 07/07/2017 04:00 PM, Andrew Lunn wrote: >> Ok. I'm fine with moving the phy-reset-gpios binding into the PHY. >> But one question still remains: Who should then trigger the "hard >> reset" of the PHY? > > Hi Richard > > I think i see a few whys to do this, but first i need to check > something. Is the clock which is causing a problem this one: > > /* clk_ref is optional, depends on board */ > fep->clk_ref = devm_clk_get(&pdev->dev, "enet_clk_ref"); > if (IS_ERR(fep->clk_ref)) > fep->clk_ref = NULL; Yes. It's this one. > > Possible solutions: > > 1) clocks are referenced counted. If it is turned on twice, it needs > to be turned off twice before it is actually turned off. So, make > the PHY driver also clk_prepare_enable() this clock. When the FEC > tries to turn it off, it will stay ticking. Problem avoided, at the > expense of some power. Somehow this approach triggers a "workaround-feeling" for me... Furthermore as you say it "wastes" (at least some) power. For exactly this reason the disabling of the clock was implemented in commit e8fcfcd5684a ("net: fec: optimize the clock management to save power"). > > 2) More complex, but make the PHY driver also a clock driver. Have the > PHY driver export a clock which the FEC use, as "enet_clk_ref". The > implementation of this clock, would both turn the real clock on, > and the perform the reset. This seems as a good solution to me. Furthermore IMHO it would be good to move all PHY related dt bindings (reset-gpio, clk, etc.) from the MAC into the PHY node. Or are there any reasons/arguments against this approach? > > Both require no changes to the FEC, or any other MAC driver using this > PHY, so long as the MAC driver uses the common clock infrastructure to > control the clock to the PHY. As (IMHO) the new approach likely won't be backported to stable releases I want to stress again the point that commit e8fcfcd5684a ("net: fec: optimize the clock management to save power") introduced this problem and therefore "broke the PHY" for our board. So would it be possible to add a "quick" bugfix patch (maybe this patch or another one removing the clk disable) so this fix can be backported to stable? Otherwise our board is only working with another "out-of-tree" patch (which I want to avoid)... kind regards, Richard.L