Re: [net-next.git 1/4 (v5)] phy: add the EEE support and the way to access to the MMD registers.

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

Hello Ben

On 6/14/2012 1:28 AM, Ben Hutchings wrote:

[snip]

>> > + */
>> > +int phy_init_eee(struct phy_device *phydev, bool clk_stop_enable)
>> > +{
>> > +	int ret = -EPROTONOSUPPORT;
>> > +
>> > +	/* According to 802.3az,the EEE is supported only in full duplex-mode.
>> > +	 * Also EEE feature is active when core is operating with MII, GMII
>> > +	 * or RGMII.
>> > +	 */
>> > +	if ((phydev->duplex == DUPLEX_FULL) &&
>> > +	    ((phydev->interface == PHY_INTERFACE_MODE_MII) ||
>> > +	    (phydev->interface == PHY_INTERFACE_MODE_GMII) ||
>> > +	    (phydev->interface == PHY_INTERFACE_MODE_RGMII))) {
>> > +		int eee_cap, eee_link;
>> > +
>> > +		/* EEE ability must be supported in both local and remote
>> > +		 * PHY devices.
>> > +		 */
>> > +		eee_cap = phy_read_mmd_indirect(phydev->bus, MDIO_AN_EEE_LPABLE,
>> > +						MDIO_MMD_AN, phydev->addr);
>> > +		if (eee_cap < 0)
>> > +			return eee_cap;
>> > +
>> > +		eee_link = phy_read_mmd_indirect(phydev->bus, MDIO_PCS_EEE_ABLE,
>> > +						 MDIO_MMD_PCS, phydev->addr);
>> > +		if (eee_link < 0)
>> > +			return eee_link;
>> > +
>> > +		if (eee_cap && eee_link) {
> I don't see any harm in setting the 'clock stop' bit if requested, even
> if EEE is not supported and therefore we will never receive LPI from the
> link partner.

ok

> But you also use this condition to decide whether to enable TX LPI, so
> it's important that it does match the specification (§78.3) for whether
> EEE is supported - but it doesn't.  You need to work out what mode was
> autonegotiated, then check that the relevant bit is set in both our EEE
> advertising (*not* supported) and the LP EEE advertising masks.

I've some doubts and, before resending the patch, I kindly ask you some
further details just on this point.

In the code, I check if the EEE is supported and on GMII, MII and RGMII
and duplex mode; in case of success the Ethernet driver can enable the
TX LPI.
Indeed, I am only using the 3.20 and 7.61 registers w/o looking at the
7.60. So this should be fixed, shouldn't it?
Am I missing anything else?
What do you mean when say that it doesn't match the specification
(§78.3)? I'm pointing to the '78.3 Capabilities Negotiation' chapter of
the IEEE802-3az, is it ok?\x02\x03\x03\x03\x04\x05\x06\a\x04\b

\x03\x03\x03\x04Thanks for your feedback in advance.

peppe