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

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

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.

> 
>>>> [snip]
>>>>
>>>> 	Current message level: 0x0000003f (63)
>>>> 			       drv probe link timer ifdown ifup
>>>> 	Link detected: yes
>>>> 	Energy-Efficient Ethernet:           -------------------------
>>>> 		MAC supports: yes            |-> related to MAC side  |
>>>> 		phy supports modes: ...      |-> from MMD 3.20        |
>>>> 		phy advertising modes: ...   |-> from MMD 7.60        |
>>>> 		LP advertising modes: ...    |-> from MMD 7.61        |
>>>> 					     --------------------------
>>>>                                                     (*)
>>>> PS. The "..." above means that we can actually dump: 100BASE-TX EEE etc
>>>> for each advertising modes and also for phy support (reg 3.20).
>>>
>>> Yes, that's the sort of information I would expect to see (but try to be
>>> consistent with the wording used for AN).:
>>
>> e.g. SUPPORTED_100baseT_EEE ... ADVERTISED_<...>
> 
> I meant the wording used in the ethtool output: 'supported',
> 'advertised', 'link partner advertised' rather than 'phy supports',
> 'phy advertising', 'LP advertising'.

ok :-)

> 
>>> The EEE advertising mask presumably should be changeable similarly to
>>> the AN advertising mask ('ethtool -s <devname> eeeadv <mask>').  But I
>>> don't know how the two advertising masks interact.  Is one supposed to
>>> be a subset of the other?  Currently ethtool automatically changes the
>>> AN advertising mask in response to a speed/duplex change; should it also
>>> try to change the EEE advertising mask?
>>
>> I've just verified the IEEE (Table 45–150a—EEE advertisement register
>> (Register 7.60) bit definitions) and sorry for my delay in reply but I
>> was in trouble because looking at the registers for the phy (I am using)
>> the reg 7.60 was in RO and I couldn't understand how to set the mask.
>> I confirm that the Adv reg from the std is R/W and the mask as you
>> suggest could be set according to the speed.
>> The EEE should work on duplex mode only.
>>
>> I wonder so if if the final patch I should have no new option for the
>> ethtool command and EEE info are directly passed from the kernel like
>> speed and duplex when call get_settings.
> 
> Are you suggesting to define EEE mode flags in the existing supported,
> advertising and lp_advertising masks?

Yes but I was wrong, I can use the existing flags.

Regards
Peppe

> 
> Ben.
>