On 05.10.2020 18:00, Florian Fainelli wrote: > > > On 10/5/2020 8:54 AM, Heiner Kallweit wrote: >> 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. > > OK and we optionally call into a PHY device's shutdown function if defined so > it can perform PHY specific work? That would work for me.
If suspend and shutdown procedure are the same for the PHY, then we may not need a shutdown hook in phy_driver. This seems to be the case here. I just wonder what the actual use case is, because typically MAC drivers call phy_stop (that calls phy_suspend) in their shutdown hook.
