Re: [PATCH v7 06/11] net: pch_gbe: Only enable MAC when PHY link is active

[Andrew Lunn <andrew@lunn.ch>]

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