Message-ID: <1339630137.2612.83.camel@bwh-desktop.uk.solarflarecom.com>
Date:	Thu, 14 Jun 2012 00:28:57 +0100
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Giuseppe CAVALLARO <peppe.cavallaro@...com>
CC:	<netdev@...r.kernel.org>, <eric.dumazet@...il.com>,
	<rayagond@...avyalabs.com>, <davem@...emloft.net>,
	<yuvalmin@...adcom.com>
Subject: Re: [net-next.git 1/4 (v5)] phy: add the EEE support and the way to
 access to the MMD registers.

On Wed, 2012-06-13 at 10:01 +0200, Giuseppe CAVALLARO wrote:
> This patch adds the support for the Energy-Efficient Ethernet (EEE)
> to the Physical Abstraction Layer.
> To support the EEE we have to access to the MMD registers 3.20 and
> 7.60/61. So two new functions have been added to read/write the MMD
> registers (clause 45).
> 
> An Ethernet driver (I tested the stmmac) can invoke the phy_init_eee to properly
> check if the EEE is supported by the PHYs and it can also set the clock
> stop enable bit in the 3.0 register.
> The phy_get_eee_err can be used for reporting the number of time where
> the PHY failed to complete its normal wake sequence.
> 
> In the end, this patch also adds the EEE ethtool support implementing:
>  o phy_ethtool_set_eee
>  o phy_ethtool_get_eee
> 
> v1: initial patch
> v2: fixed some errors especially on naming convention
> v3: renamed again the mmd read/write functions thank to Ben's feedback
> v4: moved file to phy.c and added the ethtool support.
> v5: fixed phy_adv_to_eee, phy_eee_to_supported, phy_eee_to_adv return
>     values according to ethtool API (thanks to Ben's feedback).
>     Renamed some macros to avoid too long names.

Sorry, I spotted some more little issues:

[...]
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
[...]
> +/**
> + * phy_read_mmd_indirect - reads data from the MMC register (clause 22 to
> + * access to clause 45)

The short description has to fit on the same line as the name; the
kernel-doc processing tools will not unwrap these two lines.

> + * @bus: the target MII bus
> + * @prtad: MMD Address
> + * @devad: MMD DEVAD
> + * @addr: PHY address on the MII bus
> + *
> + * Description: Reads data from the MMD regisetrs of the

Typo: regisetrs -> registers.

> + * phy addr. To read these register we have:
> + * 1) Write reg 13 // DEVAD
> + * 2) Write reg 14 // MMD Address
> + * 3) Write reg 13 // MMD Data Command for MMD DEVAD
> + * 3) Read  reg 14 // Read MMD data
> + */
> +static int phy_read_mmd_indirect(struct mii_bus *bus, int prtad, int devad,
> +				 int addr)
> +{
> +	u32 ret;
> +
> +	mmd_phy_indirect(bus, prtad, devad, addr);
> +
> +	/* Read the content of the MMD's selected register */
> +	ret = bus->read(bus, addr, MII_MMD_DATA);
> +
> +	return ret;
> +}
> +
> +/**
> + * phy_write_mmd_indirect - writes data to the MMC register (clause 22 to
> + * access to clause 45)

Same line-wrapping problem here.

> + * @bus: the target MII bus
> + * @prtad: MMD Address
> + * @devad: MMD DEVAD
> + * @addr: PHY address on the MII bus
> + * @data: data to write in the MMD register
> + *
> + * Description: Reads data from the MMD regisetrs of the
> + * phy addr. To read these register we have:
> + * 1) Write reg 13 // DEVAD
> + * 2) Write reg 14 // MMD Address
> + * 3) Write reg 13 // MMD Data Command for MMD DEVAD
> + * 3) Write reg 14 // Write MMD data
> + */
> +static void phy_write_mmd_indirect(struct mii_bus *bus, int prtad, int devad,
> +				   int addr, u32 data)
> +{
> +	mmd_phy_indirect(bus, prtad, devad, addr);
> +
> +	/* Write the data into MMD's selected register */
> +	bus->write(bus, addr, MII_MMD_DATA, data);
> +}
> +
> +/* phy_init_eee

If this is meant to be a kernel-doc comment, it needs to start with
'/**\n' and have a short summary must be added after the function name.

> + * @phydev: target phy_device struct
> + * @clk_stop_enable: PHY may stop the clock during LPI
> + *
> + * Description: it checks if the Energy-Efficient Ethernet (EEE)
> + * is supported by looking at the MMD registers 3.20 and 7.60/61
> + * and it programs the MMD register 3.0 setting the "Clock stop enable"
> + * bit if required.
> + * In fact, the clk_stop_enable can be passed to:
> + *  1 = The PHY may stop the clock during LPI
> + *  0 = Clock not stoppable

I think these last three lines are redundant with the short description
of clk_stop_enable.

> + */
> +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.

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.

[...]
> +/* phy_get_eee_err
[...]
> +/* phy_ethtool_get_eee
[...]
> +/* phy_ethtool_set_eee
[...]

Also not valid kernel-doc comments.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists