Re: [PATCH 3/3] net-next: dsa: add new driver for qca8xxx family

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

On 09/13/2016 11:07 AM, John Crispin wrote:
> 
> 
> On 13/09/2016 19:09, Florian Fainelli wrote:
>> On 09/13/2016 08:59 AM, Andrew Lunn wrote:
>>>> Hi Andrew,
>>>>
>>>> this function does indeed duplicate the functionality of
>>>> phy_ethtool_get_eee() with the small difference, that e->eee_active is
>>>> also set which phy_ethtool_get_eee() does not set.
>>>>
>>>> dsa_slave_get_eee() will call phy_ethtool_get_eee() right after the
>>>> get_eee() op has been called. would it be ok to move the code setting
>>>> eee_active to  phy_ethtool_get_eee().
>>
>> Humm, AFAIR, the reason why eee_active is set outside of
>> phy_ethtool_set_eee() is because this is a MAC + PHY thing, both need to
>> agree and support that, and so while the PHY may be configured to have
>> EEE advertised and enabled, you also need to take care of the MAC
>> portion and enable EEE in there as well. Is not there such a thing for
>> the qca8k switch where the PHY needs to be configured through the
>> standard phylib calls, but the switch's transmitter/receiver also needs
>> to have EEE enabled?
>>
> 
> Hi Florian,
> 
> the switch needs to enable the eee on a per mac absis, but there is no
> way to tell if the autonegotiate worked and eee is enabled without
> reading the phys registers.

OK, that does not sound atypical here, most drivers I see do have a way
to tell if EEE is active by reading e.g: the LPI indication register, or
something that is able to reflect the negotiated result.

> 
> setting the eee_active inside phy_ethtool_get_eee() would break those
> dsa drivers that have a register telling if AN worked. if it is ok i
> will just call phy_ethtool_get_eee() inside get_eee().

Ok, let's see the code and then we can discuss from there, not very
clear on the proposed change here.
-- 
Florian