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
