On 05.10.2020 17:41, Florian Fainelli wrote: > > > On 10/5/2020 1:53 AM, Jisheng Zhang wrote: >> 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? > > I believe you will have an easier time to add an argument to the PHY driver > suspend's function to indicate the WoL status, or to move down the check for > WoL being enabled/supported compared to adding support for shutdown into the > MDIO bus layer, and then PHY drivers.
Maybe the shutdown callback of mdio_bus_type could be implemented. It could iterate over all PHY's on the bus, check for WoL (similar to mdio_bus_phy_may_suspend) and do whatever is needed. Seems to me to be the most generic way.
