On Tue, 6 Oct 2020 07:45:10 +0200 Heiner Kallweit <hkallwe...@gmail.com> wrote:
> > 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. Thank you all, I will follow this direction and send out an RFC for review. Thanks a lot, Jisheng
