On Wed, 30 Sep 2020 13:23:29 -0700 Florian Fainelli wrote:
> > On 9/30/2020 1:11 PM, Andrew Lunn wrote: > > On Wed, Sep 30, 2020 at 01:07:19PM -0700, Florian Fainelli wrote: > >> > >> > >> On 9/30/2020 12:09 PM, Andrew Lunn wrote: > >>> On Wed, Sep 30, 2020 at 05:47:43PM +0800, Jisheng Zhang wrote: > >>>> Hi, > >>>> > >>>> A GE phy supports pad isolation which can save power in WOL mode. But > >>>> once the > >>>> isolation is enabled, the MAC can't send/receive pkts to/from the phy > >>>> because > >>>> the phy is "isolated". To make the PHY work normally, I need to move the > >>>> enabling isolation to suspend hook, so far so good. But the isolation > >>>> isn't > >>>> enabled in system shutdown case, to support this, I want to add shutdown > >>>> hook > >>>> to net phy_driver, then also enable the isolation in the shutdown hook. > >>>> Is > >>>> there any elegant solution? > >>> > >>>> Or we can break the assumption: ethernet can still send/receive pkts > >>>> after > >>>> enabling WoL, no? > >>> > >>> That is not an easy assumption to break. The MAC might be doing WOL, > >>> so it needs to be able to receive packets. > >>> > >>> What you might be able to assume is, if this PHY device has had WOL > >>> enabled, it can assume the MAC does not need to send/receive after > >>> suspend. The problem is, phy_suspend() will not call into the driver > >>> is WOL is enabled, so you have no idea when you can isolate the MAC > >>> from the PHY. > >>> > >>> So adding a shutdown in mdio_driver_register() seems reasonable. But > >>> you need to watch out for ordering. Is the MDIO bus driver still > >>> running? > >> > >> If your Ethernet MAC controller implements a shutdown callback and that > >> callback takes care of unregistering the network device which should also > >> ensure that phy_disconnect() gets called, then your PHY's suspend function > >> will be called. > > > > Hi Florian > > > > I could be missing something here, but: > > > > phy_suspend does not call into the PHY driver if WOL is enabled. So > > Jisheng needs a way to tell the PHY it should isolate itself from the > > MAC, and suspend is not that. > > I missed that part, that's right if WoL is enabled at the PHY level then > the suspend callback is not called, how about we change that and we > always call the PHY's suspend callback? This would require that we audit Hi all, The PHY's suspend callback usually calls genphy_suspend() which will set BMCR_PDOWN bit, this may break WoL. I think this is one the reason why we ignore the phydrv->suspend() when WoL is enabled. If we goes to this directly, it looks like we need to change each phy's suspend implementation, I.E if WoL is enabled, ignore genphy_suspend() and do possible isolation; If WoL is disabled, keep the code path as is. So compared with the shutdown hook, which direction is better? Thanks in advance, Jisheng > every driver that defines both .suspend and .set_wol but there are not > that many. > > Adding an additional callback to the PHY driver does not really scale > and it would require us to be extra careful and also plumb the MDIO bus > shutdown. > -- > Florian
