Re: [net-next 1/4] phy: add the EEE support and the way to access to the MMD regs

[Ben Hutchings <bhutchings@solarflare.com>]

On Tue, 2012-03-06 at 09:28 +0100, Giuseppe CAVALLARO wrote:
> This patch adds the initial support for the Energy-Efficient
> Ethernet (EEE). It has been tested on IC+101G device on ST STB.
> 
> To support the EEE we have to access to the MMD registers 3.20 and
> 3.60/61. So I added two new functions to read/write the MMD
> registers (clause 45)
> 
> The upper-layer will invoke the phy_check_eee to properly check
> if the EEE is supported by the PHYs.
> 
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
>  drivers/net/phy/phy_device.c |  136 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mdio.h         |    5 ++
>  include/linux/mii.h          |   11 ++++
>  include/linux/phy.h          |    3 +
>  4 files changed, 155 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index f320f46..c269492e 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
[...]
> @@ -898,6 +899,141 @@ int genphy_resume(struct phy_device *phydev)
>  }
>  EXPORT_SYMBOL(genphy_resume);
>  
> +static inline void mmd_phy_cl45(struct mii_bus *bus, int prtad, int devad,
> +				int addr)
[...]
> +/**
> + * read_phy_mmd - reads data from the MMC register (clause 22 to access to
> + * clause 45)
[...]
> +/**
> + * write_phy_mmd - writes data to the MMC register (clause 22 to access to
> + * clause 45)

These names need to be changed as phylib also supports (or is intended
to support) real clause 45 PHYs.

[...]
> diff --git a/include/linux/mdio.h b/include/linux/mdio.h
> index dfb9479..a2dfe86 100644
> --- a/include/linux/mdio.h
> +++ b/include/linux/mdio.h
> @@ -43,7 +43,11 @@
>  #define MDIO_PKGID2		15
>  #define MDIO_AN_ADVERTISE	16	/* AN advertising (base page) */
>  #define MDIO_AN_LPA		19	/* AN LP abilities (base page) */
> +#define MDIO_EEE_CAP		20	/* EEE Capability register */
> +#define MDIO_EEE_WK_ERR		22	/* EEE wake error counter */
>  #define MDIO_PHYXS_LNSTAT	24	/* PHY XGXS lane state */
> +#define MDIO_EEE_ADV		60	/* EEE advertisement */
> +#define MDIO_EEE_PART_LINK	61	/* EEE link partner ability */
>  
>  /* Media-dependent registers. */
>  #define MDIO_PMA_10GBT_SWAPPOL	130	/* 10GBASE-T pair swap & polarity */

The convention I tried to set here was that register numbers defined for
only one MMD are named beginning with MDIO_<mmd>_.  I also used 'LP' for
link partner and 'ABLE' for capability/ability.

So for consistency I think those should be named MDIO_PCS_EEE_ABLE,
MDIO_PCS_EEE_WK_ERR, MDIO_AN_EEE_ADV, MDIO_AN_EEE_LPABLE.

> @@ -82,6 +86,7 @@
>  #define MDIO_AN_CTRL1_RESTART		BMCR_ANRESTART
>  #define MDIO_AN_CTRL1_ENABLE		BMCR_ANENABLE
>  #define MDIO_AN_CTRL1_XNP		0x2000	/* Enable extended next page */
> +#define MDIO_PCS_CLK_STOP_ENABLE	0x400	/* Stop the clock during LPI */

Register field mask names should begin with the register number name, so
that it's easier to see whether you're using them correctly.  So this
should be something like MDIO_PCS_CTRL1_CLK_STOP_ENABLE or (slightly
shorter) MDIO_PCS_CTRL1_CLKSTOP_EN.
 
>  /* 10 Gb/s */
>  #define MDIO_CTRL1_SPEED10G		(MDIO_CTRL1_SPEEDSELEXT | 0x00)
> diff --git a/include/linux/mii.h b/include/linux/mii.h
> index 2783eca..18457add 100644
> --- a/include/linux/mii.h
> +++ b/include/linux/mii.h
> @@ -21,6 +21,8 @@
>  #define MII_EXPANSION		0x06	/* Expansion register          */
>  #define MII_CTRL1000		0x09	/* 1000BASE-T control          */
>  #define MII_STAT1000		0x0a	/* 1000BASE-T status           */
> +#define	MII_MMD_CRTL		0x0d	/* MMD Access Control Register */

Typo: CRTL should be CTRL.

> +#define	MII_MMD_DATA		0x0e	/* MMD Access Data Register */
>  #define MII_ESTATUS		0x0f	/* Extended Status             */
>  #define MII_DCOUNTER		0x12	/* Disconnect counter          */
>  #define MII_FCSCOUNTER		0x13	/* False carrier counter       */
> @@ -141,6 +143,15 @@
>  #define FLOW_CTRL_TX		0x01
>  #define FLOW_CTRL_RX		0x02
>  
> +/* MMD Access Control register fields */
> +#define MII_MMD_CRTL_DEVAD_MASK			0x1f	/* Mask MMD DEVAD*/
> +#define MII_MMD_CRTL_FUNC_ADDR			0x0000	/* Address */
[...]

Again here.

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.