Re: [PATCH v6 1/6] net: phy: at803x: Export at803x_debug_reg_mask()

[Andrew Lunn <andrew@lunn.ch>]

On Fri, May 11, 2018 at 03:22:39PM -0700, Paul Burton wrote:
> Hi Andrew,
> 
> On Fri, May 11, 2018 at 09:24:46PM +0200, Andrew Lunn wrote:
> > > I could reorder the probe function a little to initialize the PHY before
> > > performing the MAC reset, drop this patch and the AR803X hibernation
> > > stuff from patch 2 if you like. But again, I can't actually test the
> > > result on the affected hardware.
> > 
> > Hi Paul
> > 
> > I don't like a MAC driver poking around in PHY registers.
> > 
> > So if you can rearrange the code, that would be great.
> > 
> >    Thanks
> > 	Andrew
> 
> Sure, I'll give it a shot.
> 
> After digging into it I see 2 ways to go here:
> 
>   1) We could just always reset the PHY before we reset the MAC. That
>      would give us a window of however long the PHY takes to enter its
>      low power state & stop providing the RX clock during which we'd
>      need the MAC reset to complete. In the case of the AR8031 that's
>      "about 10 seconds" according to its data sheet. In this particular
>      case that feels like plenty, but it does also feel a bit icky to
>      rely on the timing chosen by the PHY manufacturer to line up with
>      that of the MAC reset.
> 
>   2) We could introduce a couple of new phy_* functions to disable &
>      enable low power states like the AR8031's hibernation feature, by
>      calling new function pointers in struct phy_driver. Then pch_gbe &
>      other MACs could call those to have the PHY driver disable
>      hibernation at times where we know we'll need the RX clock and
>      re-enable it afterwards.

Hi Paul

When there is no link, you don't need the MAC running. My assumption
is that the PHY is designed around that idea, you leave the MAC idle
until there is a link. When the phylib calls the link_change handler,
the MAC should then be started/stopped depending on the state of the
link. You are then guaranteed to have the clock when you need it.

I've no idea how easy this is to implement given the current code...

     Andrew