From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Lunn Subject: Re: [PATCH v7 06/11] net: pch_gbe: Only enable MAC when PHY link is active Date: Thu, 28 Jun 2018 09:36:28 +0200 Message-ID: <20180628073628.GA15034@lunn.ch> References: <20180627000612.27263-1-paul.burton@mips.com> <20180627000612.27263-7-paul.burton@mips.com> <20180627173014.GD885@lunn.ch> <20180627175428.qfmnxnreuuywplcl@pburton-laptop> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, "David S . Miller" To: Paul Burton Return-path: Received: from vps0.lunn.ch ([185.16.172.187]:45312 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933094AbeF1Hgb (ORCPT ); Thu, 28 Jun 2018 03:36:31 -0400 Content-Disposition: inline In-Reply-To: <20180627175428.qfmnxnreuuywplcl@pburton-laptop> Sender: netdev-owner@vger.kernel.org List-ID: On Wed, Jun 27, 2018 at 10:54:28AM -0700, Paul Burton wrote: > Hi Andrew, > > On Wed, Jun 27, 2018 at 07:30:14PM +0200, Andrew Lunn wrote: > > On Tue, Jun 26, 2018 at 05:06:07PM -0700, Paul Burton wrote: > > > When using a PHY connected via RGMII, as the pch_gbe driver presumes is > > > the case, the RX clock is provided by the PHY to the MAC. Various PHYs, > > > including both the AR8031 used by the Minnowboard & the RTL8211E used by > > > the MIPS Boston development board, will stop generating the RX clock > > > when the ethernet link is down (eg. the ethernet cable is unplugged). > > > > > > Various pieces of functionality in the EG20T MAC, ranging from basics > > > like completing a MAC reset to programming MAC addresses, rely upon the > > > RX clock being provided. When the clock is not provided these pieces of > > > functionality simply never complete, and the busy bits that indicate > > > they're in progress remain set indefinitely. > > > > > > The pch_gbe driver currently requires that the RX clock is always > > > provided, and attempts to enforce this by disabling the hibernation > > > feature of the AR8031 PHY to keep it generating the RX clock. This patch > > > moves us away from this model by only configuring the MAC when the PHY > > > indicates that the ethernet link is up. When the link is up we should be > > > able to safely expect that the RX clock is being provided, and therefore > > > safely reset & configure the MAC. > > > > Hi Paul > > > > I like the concept, but the implementation is not clear. Maybe it just > > needs more details in the commit message. What has the watchdog got to > > do with link up? > > pch_gbe_watchdog() polls for the link coming up or going down, so that's > where we find out that the link is up. I was thinking it would be something like that. So could you please explain this in the commit message. Does the watchdog later become the adjust_link callback for phylib? Having a name based around adjust_link would make this clearer. That is the norm. But i understand this is a preparation step, so the rename might happen later? > > And what happens on link down? Does the MAC need shutting down? I > > don't see such code here. > > Well, depending upon the PHY the RX clock might stop which will prevent > parts of the MAC from functioning properly. The datasheet for the Atheros PHY suggests the clock will stop after a while. So again, commenting why you think nothing extra is needed would be good. Basically, there is a lot of non-obvious stuff going on here, and it helps both reviewer and future debugger to have a fuller explanation. Thanks Andrew