On 04/18/2017 06:40 PM, Florian Fainelli wrote: > On 04/18/2017 06:23 AM, Niklas Cassel wrote: >> On 01/04/2017 03:33 PM, Florian Fainelli wrote: >>> On 12/02/2016 09:48 AM, Florian Fainelli wrote: >>>>>> Peppe, any thoughts on this? >>>>> >>>>> I share what you say. >>>>> >>>>> In sum, the EEE management inside the stmmac is: >>>>> >>>>> - the driver looks at own HW cap register if EEE is supported >>>>> >>>>> (indeed the user could keep disable EEE if bugged on some HW >>>>> + Alex, Fabrice: we had some patches for this to propose where we >>>>> called the phy_ethtool_set_eee to disable feature at phy >>>>> level >>>>> >>>>> - then the stmmac asks PHY layer to understand if transceiver and >>>>> partners are EEE capable. >>>>> >>>>> - If all matches the EEE is actually initialized. >>>>> >>>>> the logic above should be respected when use ethtool, hmm, I will >>>>> check the stmmac_ethtool_op_set_eee asap. >>>>> >>>>> Hoping this is useful >>>> >>>> This is definitively useful, the only part that I am struggling to >>>> understand in phy_init_eee() is this: >>>> >>>> eee_adv = phy_read_mmd_indirect(phydev, MDIO_AN_EEE_ADV, >>>> MDIO_MMD_AN); >>>> if (eee_adv <= 0) >>>> goto eee_exit_err; >>>> >>>> if we are not already advertising EEE in the PHY's MMIO_MMD_AN page, by >>>> the time we call phy_init_eee(), then we cannot complete the EEE >>>> configuration at the PHY level, and presumably we should abort the EEE >>>> configuration at the MAC level. >>>> >>>> While this condition makes sense if e.g: you are re-negotiating the link >>>> with your partner for instance and if EEE was already advertised, the >>>> very first time this function is called, it seems to be like we should >>>> skip the check, because phy_init_eee() should actually tell us if, as a >>>> result of a successful check, we should be setting EEE as something we >>>> advertise? >>>> >>>> Do you remember what was the logic behind this check when you added it? >>> >>> Peppe, can you remember why phy_init_eee() was written in a way that you >>> need to have already locally advertised EEE for the function to >>> successfully return? Thank you! >>> >> >> I'm curious about this as well. >> >> I can get EEE to work with stmmac, but to be able to turn EEE on, >> I need to set eee advertise via ethtool first. >> (Tested with 2 different PHYs from different vendors, with their >> PHY specific driver enabled.) >> >> Is this the same for all PHYs or are there certain PHYs/PHY drivers >> that actually advertise eee by default? > > It depends on whether the PHY driver takes care of the EEE advertisement > part for your or not, most drivers probably don't do that. > >> (From reading this mail thread there seems to be a suggestion that >> the broadcom PHY driver might advertise eee by default.) > > As written before, some (not all) Broadcom PHY drivers (cygnus, 7xxx) do > advertise EEE by default in order to validate the first check done in > phy_init_eee(), but that's the only reason really. > > Since we have not been able to get a straight answer from Peppe about > why there is this initial check, I think the cleanest path moving > forward is the following: > > - rename phy_init_eee() into something like: phy_can_do_eee() and remove > the first check on whether EEE is already advertised because that's > precisely what we are trying to determine with this function > > - Ethernet MAC drivers keep calling phy_can_do_eee() (formerly > phy_init_eee()) during their adjust_link callback in order to > re-negotiate EEE with their link partner, just like they should call > phy_ethtool_set_eee() to really enable EEE the first time they want to > enable EEE with the link partner > > - remove the part from phy_init_eee() that tries to stop the PHY TX > clock and provide a set of helpers: phy_can_stop_tx_clk() and > phy_set_stop_tx_clk() which will take care of that > > Does that look reasonable? Sounds very reasonable to me. However, if I look specifically at the stmmac driver, stmmac_eee_init() is called from adjust_link callback. If we replace phy_init_eee() with a phy_can_do_eee() in stmmac_eee_init(), then the driver will enable EEE in the IP, and setup timers etc. If I understand you correctly, the code in the adjust_link callback should call phy_can_do_eee() so that the PHY re-negotiate EEE with the link partner. You will still need to use ethtool to actually enable it in the PHY (call the new phy_init_eee()). (Which sounds good, since we probably do not want to suddenly enable EEE by default in a lot of drivers.) The issue that I see is that we probably do not want to setup timers, etc. in the adjust_link callback before EEE has actually been enabled, so it might not be as easy as just replacing phy_init_eee() with phy_can_do_eee() in some drivers. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
