Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers

[Florian Fainelli <f.fainelli@gmail.com>]

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?
-- 
Florian