Message-ID: <b8232e33-13f1-072e-57dd-a786430189ff@axis.com>
Date:   Tue, 25 Apr 2017 11:06:36 +0200
From:   Niklas Cassel <niklas.cassel@...s.com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Giuseppe CAVALLARO <peppe.cavallaro@...com>,
        Andrew Lunn <andrew@...n.ch>,
        Yegor Yefremov <yegorslists@...glemail.com>
CC:     netdev <netdev@...r.kernel.org>,
        "linux-omap@...r.kernel.org" <linux-omap@...r.kernel.org>,
        Grygorii Strashko <grygorii.strashko@...com>,
        "N, Mugunthan V" <mugunthanvnm@...com>,
        Rami Rosen <roszenrami@...il.com>,
        Fabrice GASNIER <fabrice.gasnier@...com>,
        <rmk+kernel@...linux.org.uk>
Subject: Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE
 registers



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.

Powered by blists - more mailing lists