Messages in this thread | | | Date | Thu, 2 Feb 2023 21:24:38 -0800 | From | Jakub Kicinski <> | Subject | Re: [PATCH V3 1/2] net: phylink: add a function to resume phy alone to fix resume issue with WoL enabled |
| |
On Thu, 2 Feb 2023 16:15:59 +0800 Clark Wang wrote: > Issue we met: > On some platforms, mac cannot work after resumed from the suspend with WoL > enabled. > > The cause of the issue: > 1. phylink_resolve() is in a workqueue which will not be executed immediately. > This is the call sequence: > phylink_resolve()->phylink_link_up()->pl->mac_ops->mac_link_up() > For stmmac driver, mac_link_up() will set the correct speed/duplex... > values which are from link_state. > 2. In stmmac_resume(), it will call stmmac_hw_setup() after called the > phylink_resume(), because mac need phy rx_clk to do the reset. > stmmac_core_init() is called in function stmmac_hw_setup(), which will > reset the mac and set the speed/duplex... to default value. > Conclusion: Because phylink_resolve() cannot determine when it is called, it > cannot be guaranteed to be called after stmmac_core_init(). > Once stmmac_core_init() is called after phylink_resolve(), > the mac will be misconfigured and cannot be used. > > In order to avoid this problem, add a function called phylink_phy_resume() > to resume phy separately. This eliminates the need to call phylink_resume() > before stmmac_hw_setup(). > > Add another judgement before called phy_start() in phylink_start(). This way > phy_start() will not be called multiple times when resumes. At the same time, > it may not affect other drivers that do not use phylink_phy_resume(). > > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
Patch 2/2 never made it to the list. You'll need to repost. While I have you - some minor nit picks:
> +/** > + * phylink_phy_resume() - resume phy alone > + * @pl: a pointer to a &struct phylink returned from phylink_create() > + * > + * In the MAC driver using phylink, if the MAC needs the clock of the phy
You use MAC in capital letters buy phy in lower case, be consistent.
> + * when it resumes, can call this function to resume the phy separately.
missing "it" ? Otherwise the sentence is missing a subject.
> + * Then proceed to MAC resume operations. > + */ > +void phylink_phy_resume(struct phylink *pl) > +{ > + ASSERT_RTNL(); > + > + if (!test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state) > + && pl->phydev) {
&& goes at the end of the line, not start
> + phy_start(pl->phydev); > + pl->mac_resume_phy_separately = true; > + } > + > +} > +EXPORT_SYMBOL_GPL(phylink_phy_resume);
| |