Re: [net-next 1/4 (V3)] net: ethtool: add the EEE support

[Giuseppe CAVALLARO <peppe.cavallaro@st.com>]

Hello Ben

On 4/29/2012 11:56 PM, Ben Hutchings wrote:
> On Sun, 2012-04-29 at 12:20 +0300, Yuval Mintz wrote:
>> On 04/27/2012 05:11 PM, Giuseppe CAVALLARO wrote:
>>
>>> On 4/26/2012 7:17 PM, Ben Hutchings wrote:
>>>> On Thu, 2012-04-26 at 09:48 +0200, Giuseppe CAVALLARO wrote:
>>>>> Hello Ben
>>>>>
>>>>> On 4/19/2012 5:30 PM, Ben Hutchings wrote:
>>>>> [snip]
>>>>>>> I'm changing the code for getting/setting the EEE capability and trying
>>>>>>> to follow your suggestions.
>>>>>>>
>>>>>>> The "get" will show the following things; this is a bit different of the
>>>>>>> points "a" "b" and "c" we had discussed. Maybe, this could also be a
>>>>>>> more complete (*) .
>>>>>>> The ethtool (see output below as example) could report the phy
>>>>>>> (supported/advertised/lp_advertised) and mac eee capabilities separately.
>>>>>> Sounds reasonable.
>>>>>>
>>>>>>> The "set" will be useful for some eth devices (like the stmmac) that can
>>>>>>> stop/enable internally the eee capability (at mac level).
>>>>>> I don't know much about EEE, but shouldn't the driver take care of
>>>>>> configuring the MAC for this whenever the PHY is set to advertise EEE
>>>>>> capability?
>>>>> Yes indeed this can be done at driver level. So could I definitely
>>>>> remove it from ethtool? What do you suggest?
>>>>>
>>>>> In case of the stmmac I could add a specific driver option via sys to
>>>>> enable/disable the eee and set timer.
>>>> Generally, ethtool doesn't distinguish MAC and PHY settings because they
>>>> have to be configured consistently for the device to do anything useful.
>>>> If there is some good use for enabling EEE in the MAC and not the PHY,
>>>> or vice versa, then this should be exposed in the ethtool interface.
>>>> But if not then I don't believe it needs to be in either an ethtool or a
>>>> driver-specific interface.
>>> Thanks Ben for this clarification: in case of the stmmac the option is
>>> useful to stop a timer to enter in lpi state for the tx.
>>> So it's worth having that and from ethtool.
> 
> I think I finally get it.  If we negotiate a 100BASE-TX link (or one of
> the various backplane modes) with EEE enabled, we allow the link partner
> to assert LPI but we might still not want to assert it in the transmit
> direction.  Right?  (Whereas for 1000BASE-T and 10GBASE-T this would be
> useless, since both sides must assert LPI before any transition can
> happen.)
> 
>> How will a user turn off EEE support using this implementation?
> 
> At the ethtool API level this would be done by clearing the EEE
> advertising mask.  At the command-line level there could be a shortcut
> for this, just as you can use 'autoneg on' and 'autoneg off' rather than
> specifying a mask of link modes.
> 
>> Are you suggesting a "set" that works similarly to the control of the pause
>> parameters - that is, a user could either shutdown EEE or only Tx, which
>> will mean to the driver "don't enter Tx LPI mode"?
>>
>> Keep in mind that if later an interface controlling the LPI timers would be
>> added (as a measure of user control to the power saving vs. latency issue),
>> it could make this 'partial' closure interface redundant.
>>
>> Perhaps "set" should only turn the EEE feature on/off entirely (adv. them or
>> not, since clearly the link will have to be re-established afterwards), and
>> we should have a different function that prevents entry into LPI mode in Tx
>> - one whose functionality could later on be extended.
> 
> It sounds like this might as well be included, even if not all
> drivers/hardware would allow the values to be changed.  So the command
> structure would have at least:
> 
> 1. EEE link mode supported flags (get-only)
> 2. EEE link mode advertising flags (get/set)
> 3. Ditto for link partner (get-only)
> 4. TX LPI enable flag (get/set)
> 5. TX LPI timer values (get/set but driver may reject changes)

Ok I'll try to rework all following the points above. Just a note for
the timer and point 5 below.

> But if it's not yet clear exactly what timer parameters will be useful,
> we could leave some reserved space and then later define them along with
> flags to indicate whether the driver understands them.

I can use and test the LPI timer parameters that I intends, in case of
the stmmac d.d., the values added in a mac core register. These two timers:
1) specify the minimum time for which the link-status from the PHY
   should be up. The default value 1 sec as defined in the IEEE
   standard.
2) specify the minimum time for which the MAC waits after it has
   stopped transmitting the LPI pattern to the PHY

Peppe

> 
> Ben.
>