[RFC PATCH 0/17] Miscellaneous generic PHY layer improvements

[RFC PATCH 0/17] Miscellaneous generic PHY layer improvements

[Kyle Moffett]

Hello all,

I have had the following series of phylib improvements and bugfixes
kicking around in a GIT repository for over a year and I finally
decided to get around to submitting them.

They are some preparatory work for later patches that I am still
working on to support unidirectional ethernet links in phylib.

These patches are currently based on v3.1-rc9; I apologize if these
conflict with any existing ongoing work.  Please let me know and I am
happy to respin against the appropriate tree.

Cheers,
Kyle Moffett

--
Curious about my work on the Debian powerpcspe port?
I'm keeping a blog here: http://pureperl.blogspot.com/

[RFC PATCH 01/17] et1011c: Replaced PHY driver by a small dm646x board fixup

[Kyle Moffett]

The et1011c PHY driver has several noticeable code smells:

  (1) It uses a "static int speed" variable to see if the speed changed
      between calls to et1011c_read_status().  This obviously breaks if
      more than one PHY is present in a system.

  (2) The "GMII_INTERFACE" and "SYS_CLK_EN" bits should be properly set
      at reset-time by hardwired pins on the ET1011C chip, as they are
      specific to the wiring on the board.  They should NOT be set by a
      generic PHY driver, and at best belong in a board-specific fixup.

  (3) The FIFO bits are "changed" to the default reset value specified
      in the datasheet.

  (4) The driver does not appear to contain code anywhere which undoes
      any of the above changes if the interface drops from 1000BaseT to
      100BaseTX without a chip reset.  Instead it appears to perform an
      extraneous BMCR_RESET in its ->config_aneg() method, which would
      wipe out any settings applied by phy_register_fixup() and friends.

This PHY should be handled entirely by the genphy driver with only a
minimal board-specific "phy_register_fixup()" in the DM646x code.

NOTE: Completely untested. Needs somebody with hardware to try it out.

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---
 arch/arm/mach-davinci/dm646x.c |   24 ++++++++
 drivers/net/phy/Kconfig        |    5 --
 drivers/net/phy/Makefile       |    1 -
 drivers/net/phy/et1011c.c      |  119 ----------------------------------------
 4 files changed, 24 insertions(+), 125 deletions(-)
 delete mode 100644 drivers/net/phy/et1011c.c

diff --git a/arch/arm/mach-davinci/dm646x.c b/arch/arm/mach-davinci/dm646x.c
index 1802e71..afcdf37 100644
--- a/arch/arm/mach-davinci/dm646x.c
+++ b/arch/arm/mach-davinci/dm646x.c
@@ -14,6 +14,7 @@
 #include <linux/serial_8250.h>
 #include <linux/platform_device.h>
 #include <linux/gpio.h>
+#include <linux/phy.h>
 
 #include <asm/mach/map.h>
 
@@ -908,6 +909,27 @@ void __init dm646x_init(void)
 	davinci_common_init(&davinci_soc_info_dm646x);
 }
 
+/* Apparently the PHY bootstrap pin wiring on the board is wrong */
+#define ET1011C_CONFIG_REG (0x16)
+#define ET1011C_TX_FIFO_MASK		(0x3000)
+#define ET1011C_TX_FIFO_DEPTH_16	(0x1000)
+#define ET1011C_SYS_CLK_EN		(0x0010)
+#define ET1011C_INTERFACE_MASK		(0x0007)
+#define ET1011C_INTERFACE_GMII_GTX_CLK	(0x0002)
+static int dm646x_et1011c_phy_fixup(struct phy_device *phydev)
+{
+	int val = phy_read(phydev, ET1011C_CONFIG_REG);
+	if (val < 0)
+		return val;
+
+	val &= ~ET1011C_TX_FIFO_MASK;
+	val |=  ET1011C_TX_FIFO_DEPTH_16;
+	val |=  ET1011C_SYS_CLK_EN;
+	val &= ~ET1011C_INTERFACE_MASK
+	val |=  ET1011C_INTERFACE_GMII_GTX_CLK;
+	return phy_write(phydev, ET1011C_CONFIG_REG, val);
+}
+
 static int __init dm646x_init_devices(void)
 {
 	if (!cpu_is_davinci_dm646x())
@@ -917,6 +939,8 @@ static int __init dm646x_init_devices(void)
 	platform_device_register(&dm646x_emac_device);
 	clk_add_alias(NULL, dev_name(&dm646x_mdio_device.dev),
 		      NULL, &dm646x_emac_device.dev);
+	phy_register_fixup_for_uid(0x0282f014, 0xfffffff0,
+			&dm646x_et1011c_phy_fixup);
 
 	return 0;
 }
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index a702443..fdd2ace 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -82,11 +82,6 @@ config STE10XP
 	---help---
 	  This is the driver for the STe100p and STe101p PHYs.
 
-config LSI_ET1011C_PHY
-	tristate "Driver for LSI ET1011C PHY"
-	---help---
-	  Supports the LSI ET1011C PHY.
-
 config MICREL_PHY
 	tristate "Driver for Micrel PHYs"
 	---help---
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 2333215..d4c0bd0 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -14,7 +14,6 @@ obj-$(CONFIG_BROADCOM_PHY)	+= broadcom.o
 obj-$(CONFIG_BCM63XX_PHY)	+= bcm63xx.o
 obj-$(CONFIG_ICPLUS_PHY)	+= icplus.o
 obj-$(CONFIG_REALTEK_PHY)	+= realtek.o
-obj-$(CONFIG_LSI_ET1011C_PHY)	+= et1011c.o
 obj-$(CONFIG_FIXED_PHY)		+= fixed.o
 obj-$(CONFIG_MDIO_BITBANG)	+= mdio-bitbang.o
 obj-$(CONFIG_MDIO_GPIO)		+= mdio-gpio.o
diff --git a/drivers/net/phy/et1011c.c b/drivers/net/phy/et1011c.c
deleted file mode 100644
index a8eb19e..0000000
--- a/drivers/net/phy/et1011c.c
+++ /dev/null
@@ -1,119 +0,0 @@
-/*
- * drivers/net/phy/et1011c.c
- *
- * Driver for LSI ET1011C PHYs
- *
- * Author: Chaithrika U S
- *
- * Copyright (c) 2008 Texas Instruments
- *
- * This program is free software; you can redistribute  it and/or modify it
- * under  the terms of  the GNU General  Public License as published by the
- * Free Software Foundation;  either version 2 of the  License, or (at your
- * option) any later version.
- *
- */
-#include <linux/kernel.h>
-#include <linux/string.h>
-#include <linux/errno.h>
-#include <linux/unistd.h>
-#include <linux/interrupt.h>
-#include <linux/init.h>
-#include <linux/delay.h>
-#include <linux/netdevice.h>
-#include <linux/etherdevice.h>
-#include <linux/skbuff.h>
-#include <linux/spinlock.h>
-#include <linux/mm.h>
-#include <linux/module.h>
-#include <linux/mii.h>
-#include <linux/ethtool.h>
-#include <linux/phy.h>
-#include <linux/io.h>
-#include <linux/uaccess.h>
-#include <asm/irq.h>
-
-#define ET1011C_STATUS_REG	(0x1A)
-#define ET1011C_CONFIG_REG	(0x16)
-#define ET1011C_SPEED_MASK		(0x0300)
-#define ET1011C_GIGABIT_SPEED		(0x0200)
-#define ET1011C_TX_FIFO_MASK		(0x3000)
-#define ET1011C_TX_FIFO_DEPTH_8		(0x0000)
-#define ET1011C_TX_FIFO_DEPTH_16	(0x1000)
-#define ET1011C_INTERFACE_MASK		(0x0007)
-#define ET1011C_GMII_INTERFACE		(0x0002)
-#define ET1011C_SYS_CLK_EN		(0x01 << 4)
-
-
-MODULE_DESCRIPTION("LSI ET1011C PHY driver");
-MODULE_AUTHOR("Chaithrika U S");
-MODULE_LICENSE("GPL");
-
-static int et1011c_config_aneg(struct phy_device *phydev)
-{
-	int ctl = 0;
-	ctl = phy_read(phydev, MII_BMCR);
-	if (ctl < 0)
-		return ctl;
-	ctl &= ~(BMCR_FULLDPLX | BMCR_SPEED100 | BMCR_SPEED1000 |
-		 BMCR_ANENABLE);
-	/* First clear the PHY */
-	phy_write(phydev, MII_BMCR, ctl | BMCR_RESET);
-
-	return genphy_config_aneg(phydev);
-}
-
-static int et1011c_read_status(struct phy_device *phydev)
-{
-	int ret;
-	u32 val;
-	static int speed;
-	ret = genphy_read_status(phydev);
-
-	if (speed != phydev->speed) {
-		speed = phydev->speed;
-		val = phy_read(phydev, ET1011C_STATUS_REG);
-		if ((val & ET1011C_SPEED_MASK) ==
-					ET1011C_GIGABIT_SPEED) {
-			val = phy_read(phydev, ET1011C_CONFIG_REG);
-			val &= ~ET1011C_TX_FIFO_MASK;
-			phy_write(phydev, ET1011C_CONFIG_REG, val\
-					| ET1011C_GMII_INTERFACE\
-					| ET1011C_SYS_CLK_EN\
-					| ET1011C_TX_FIFO_DEPTH_16);
-
-		}
-	}
-	return ret;
-}
-
-static struct phy_driver et1011c_driver = {
-	.phy_id		= 0x0282f014,
-	.name		= "ET1011C",
-	.phy_id_mask	= 0xfffffff0,
-	.features	= (PHY_BASIC_FEATURES | SUPPORTED_1000baseT_Full),
-	.flags		= PHY_POLL,
-	.config_aneg	= et1011c_config_aneg,
-	.read_status	= et1011c_read_status,
-	.driver 	= { .owner = THIS_MODULE,},
-};
-
-static int __init et1011c_init(void)
-{
-	return phy_driver_register(&et1011c_driver);
-}
-
-static void __exit et1011c_exit(void)
-{
-	phy_driver_unregister(&et1011c_driver);
-}
-
-module_init(et1011c_init);
-module_exit(et1011c_exit);
-
-static struct mdio_device_id __maybe_unused et1011c_tbl[] = {
-	{ 0x0282f014, 0xfffffff0 },
-	{ }
-};
-
-MODULE_DEVICE_TABLE(mdio, et1011c_tbl);
-- 
1.7.2.5

[RFC PATCH 02/17] of_mdio: Don't phy_scan_fixups() twice

[Kyle Moffett]

The "phy_device_register()" call 5 lines down already calls
phy_scan_fixups(), there's no need to do it a second time.

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---
 drivers/of/of_mdio.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index d35e300..980c079 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -83,7 +83,6 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
 				addr);
 			continue;
 		}
-		phy_scan_fixups(phy);
 
 		/* Associate the OF node with the device structure so it
 		 * can be looked up later */
-- 
1.7.2.5

Re: [RFC PATCH 02/17] of_mdio: Don't phy_scan_fixups() twice

[Grant Likely]

On Thu, Oct 20, 2011 at 05:00:09PM -0400, Kyle Moffett wrote:
> The "phy_device_register()" call 5 lines down already calls
> phy_scan_fixups(), there's no need to do it a second time.
> 
> Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>

Applied, thanks.

g.

> ---
>  drivers/of/of_mdio.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index d35e300..980c079 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -83,7 +83,6 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)
>  				addr);
>  			continue;
>  		}
> -		phy_scan_fixups(phy);
>  
>  		/* Associate the OF node with the device structure so it
>  		 * can be looked up later */
> -- 
> 1.7.2.5
> 

[RFC PATCH 03/17] greth: Allow PHYs to override ->read_status

[Kyle Moffett]

Instead of manually calling genphy_read_status(), the greth driver
should call phy_read_status() to allow the PHY driver to override the
read_status method with its own version.

NOTE: Completely untested. Needs somebody with hardware to try it out.

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---
 drivers/net/greth.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/greth.c b/drivers/net/greth.c
index 52a3900..e7f268f 100644
--- a/drivers/net/greth.c
+++ b/drivers/net/greth.c
@@ -1367,7 +1367,7 @@ static int greth_mdio_init(struct greth_private *greth)
 		timeout = jiffies + 6*HZ;
 		while (!phy_aneg_done(greth->phy) && time_before(jiffies, timeout)) {
 		}
-		genphy_read_status(greth->phy);
+		phy_read_status(greth->phy);
 		greth_link_change(greth->netdev);
 	}
 
-- 
1.7.2.5

[RFC PATCH 04/17] phy/icplus: Fix read_status/config_aneg error handling

[Kyle Moffett]

Fixes the icplus PHY driver to propagate the return values of the
functions genphy_read_status() and genphy_config_aneg() instead of
ignoring them.

NOTE: Completely untested. Needs somebody with hardware to try it out.

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---
 drivers/net/phy/icplus.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index d4cbc29..2969dac 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c
@@ -115,19 +115,19 @@ static int ip1001_config_init(struct phy_device *phydev)
 static int ip175c_read_status(struct phy_device *phydev)
 {
 	if (phydev->addr == 4) /* WAN port */
-		genphy_read_status(phydev);
-	else
-		/* Don't need to read status for switch ports */
-		phydev->irq = PHY_IGNORE_INTERRUPT;
+		return genphy_read_status(phydev);
 
+	/* Don't need to read status for switch ports */
+	phydev->irq = PHY_IGNORE_INTERRUPT;
 	return 0;
 }
 
 static int ip175c_config_aneg(struct phy_device *phydev)
 {
 	if (phydev->addr == 4) /* WAN port */
-		genphy_config_aneg(phydev);
+		return genphy_config_aneg(phydev);
 
+	/* Don't need to do anything for switch ports */
 	return 0;
 }
 
-- 
1.7.2.5

[RFC PATCH 05/17] phy_driver: Make .read_status()/.config_aneg() optional

[Kyle Moffett]

Approximately 90% of the PHY drivers follow the PHY layer docs and
simply use &genphy_read_status and &genphy_config_aneg.  There would
seem to be little point in requiring them all to manually specify those
functions.

This patch makes it much easier for subsequent patches to split and
refactor the functionality of the .config_aneg() method.

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---
 Documentation/networking/phy.txt |   13 +++++--------
 drivers/net/phy/bcm63xx.c        |    4 ----
 drivers/net/phy/broadcom.c       |   20 --------------------
 drivers/net/phy/cicada.c         |    4 ----
 drivers/net/phy/davicom.c        |    4 ----
 drivers/net/phy/icplus.c         |    2 --
 drivers/net/phy/lxt.c            |    5 -----
 drivers/net/phy/marvell.c        |    6 ------
 drivers/net/phy/micrel.c         |   10 ----------
 drivers/net/phy/national.c       |    2 --
 drivers/net/phy/phy.c            |    5 ++++-
 drivers/net/phy/phy_device.c     |    2 --
 drivers/net/phy/qsemi.c          |    2 --
 drivers/net/phy/realtek.c        |    2 --
 drivers/net/phy/smsc.c           |   10 ----------
 drivers/net/phy/ste10Xp.c        |    4 ----
 drivers/net/phy/vitesse.c        |    4 ----
 include/linux/phy.h              |    8 +++++---
 18 files changed, 14 insertions(+), 93 deletions(-)

diff --git a/Documentation/networking/phy.txt b/Documentation/networking/phy.txt
index 9eb1ba5..62f890e 100644
--- a/Documentation/networking/phy.txt
+++ b/Documentation/networking/phy.txt
@@ -261,14 +261,11 @@ Writing a PHY driver
    config_intr: Enable or disable interrupts
    remove: Does any driver take-down
 
- Of these, only config_aneg and read_status are required to be
- assigned by the driver code.  The rest are optional.  Also, it is
- preferred to use the generic phy driver's versions of these two
- functions if at all possible: genphy_read_status and
- genphy_config_aneg.  If this is not possible, it is likely that
- you only need to perform some actions before and after invoking
- these functions, and so your functions will wrap the generic
- ones.
+ All of these functions are optional.  It is strongly preferred to use the
+ generic phy driver's versions of these two functions if at all possible:
+ genphy_read_status and genphy_config_aneg.  If this is not possible, it is
+ likely that you only need to perform some actions before and after invoking
+ these functions, and so your functions will wrap the generic ones.
 
  Feel free to look at the Marvell, Cicada, and Davicom drivers in
  drivers/net/phy/ for examples (the lxt and qsemi drivers have
diff --git a/drivers/net/phy/bcm63xx.c b/drivers/net/phy/bcm63xx.c
index e16f98c..c455f02 100644
--- a/drivers/net/phy/bcm63xx.c
+++ b/drivers/net/phy/bcm63xx.c
@@ -82,8 +82,6 @@ static struct phy_driver bcm63xx_1_driver = {
 	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause),
 	.flags		= PHY_HAS_INTERRUPT,
 	.config_init	= bcm63xx_config_init,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.ack_interrupt	= bcm63xx_ack_interrupt,
 	.config_intr	= bcm63xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
@@ -97,8 +95,6 @@ static struct phy_driver bcm63xx_2_driver = {
 	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause),
 	.flags		= PHY_HAS_INTERRUPT,
 	.config_init	= bcm63xx_config_init,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.ack_interrupt	= bcm63xx_ack_interrupt,
 	.config_intr	= bcm63xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index d84c422..f220264 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -692,8 +692,6 @@ static struct phy_driver bcm5411_driver = {
 			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= bcm54xx_config_init,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.ack_interrupt	= bcm54xx_ack_interrupt,
 	.config_intr	= bcm54xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
@@ -707,8 +705,6 @@ static struct phy_driver bcm5421_driver = {
 			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= bcm54xx_config_init,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.ack_interrupt	= bcm54xx_ack_interrupt,
 	.config_intr	= bcm54xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
@@ -722,8 +718,6 @@ static struct phy_driver bcm5461_driver = {
 			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= bcm54xx_config_init,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.ack_interrupt	= bcm54xx_ack_interrupt,
 	.config_intr	= bcm54xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
@@ -737,8 +731,6 @@ static struct phy_driver bcm5464_driver = {
 			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= bcm54xx_config_init,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.ack_interrupt	= bcm54xx_ack_interrupt,
 	.config_intr	= bcm54xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
@@ -753,7 +745,6 @@ static struct phy_driver bcm5481_driver = {
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= bcm54xx_config_init,
 	.config_aneg	= bcm5481_config_aneg,
-	.read_status	= genphy_read_status,
 	.ack_interrupt	= bcm54xx_ack_interrupt,
 	.config_intr	= bcm54xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
@@ -767,7 +758,6 @@ static struct phy_driver bcm5482_driver = {
 			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= bcm5482_config_init,
-	.config_aneg	= genphy_config_aneg,
 	.read_status	= bcm5482_read_status,
 	.ack_interrupt	= bcm54xx_ack_interrupt,
 	.config_intr	= bcm54xx_config_intr,
@@ -782,8 +772,6 @@ static struct phy_driver bcm50610_driver = {
 			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= bcm54xx_config_init,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.ack_interrupt	= bcm54xx_ack_interrupt,
 	.config_intr	= bcm54xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
@@ -797,8 +785,6 @@ static struct phy_driver bcm50610m_driver = {
 			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= bcm54xx_config_init,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.ack_interrupt	= bcm54xx_ack_interrupt,
 	.config_intr	= bcm54xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
@@ -812,8 +798,6 @@ static struct phy_driver bcm57780_driver = {
 			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= bcm54xx_config_init,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.ack_interrupt	= bcm54xx_ack_interrupt,
 	.config_intr	= bcm54xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
@@ -827,8 +811,6 @@ static struct phy_driver bcmac131_driver = {
 			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= brcm_fet_config_init,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.ack_interrupt	= brcm_fet_ack_interrupt,
 	.config_intr	= brcm_fet_config_intr,
 	.driver		= { .owner = THIS_MODULE },
@@ -842,8 +824,6 @@ static struct phy_driver bcm5241_driver = {
 			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= brcm_fet_config_init,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.ack_interrupt	= brcm_fet_ack_interrupt,
 	.config_intr	= brcm_fet_config_intr,
 	.driver		= { .owner = THIS_MODULE },
diff --git a/drivers/net/phy/cicada.c b/drivers/net/phy/cicada.c
index d281731..c409ca2 100644
--- a/drivers/net/phy/cicada.c
+++ b/drivers/net/phy/cicada.c
@@ -109,8 +109,6 @@ static struct phy_driver cis8201_driver = {
 	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT,
 	.config_init	= &cis820x_config_init,
-	.config_aneg	= &genphy_config_aneg,
-	.read_status	= &genphy_read_status,
 	.ack_interrupt	= &cis820x_ack_interrupt,
 	.config_intr	= &cis820x_config_intr,
 	.driver 	= { .owner = THIS_MODULE,},
@@ -124,8 +122,6 @@ static struct phy_driver cis8204_driver = {
 	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT,
 	.config_init	= &cis820x_config_init,
-	.config_aneg	= &genphy_config_aneg,
-	.read_status	= &genphy_read_status,
 	.ack_interrupt	= &cis820x_ack_interrupt,
 	.config_intr	= &cis820x_config_intr,
 	.driver 	= { .owner = THIS_MODULE,},
diff --git a/drivers/net/phy/davicom.c b/drivers/net/phy/davicom.c
index 2f774ac..5249e1e 100644
--- a/drivers/net/phy/davicom.c
+++ b/drivers/net/phy/davicom.c
@@ -156,7 +156,6 @@ static struct phy_driver dm9161e_driver = {
 	.features	= PHY_BASIC_FEATURES,
 	.config_init	= dm9161_config_init,
 	.config_aneg	= dm9161_config_aneg,
-	.read_status	= genphy_read_status,
 	.driver		= { .owner = THIS_MODULE,},
 };
 
@@ -167,7 +166,6 @@ static struct phy_driver dm9161a_driver = {
 	.features	= PHY_BASIC_FEATURES,
 	.config_init	= dm9161_config_init,
 	.config_aneg	= dm9161_config_aneg,
-	.read_status	= genphy_read_status,
 	.driver		= { .owner = THIS_MODULE,},
 };
 
@@ -177,8 +175,6 @@ static struct phy_driver dm9131_driver = {
 	.phy_id_mask	= 0x0ffffff0,
 	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.ack_interrupt	= dm9161_ack_interrupt,
 	.config_intr	= dm9161_config_intr,
 	.driver		= { .owner = THIS_MODULE,},
diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index 2969dac..28a190e 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c
@@ -151,8 +151,6 @@ static struct phy_driver ip1001_driver = {
 	.features	= PHY_GBIT_FEATURES | SUPPORTED_Pause |
 			  SUPPORTED_Asym_Pause,
 	.config_init	= &ip1001_config_init,
-	.config_aneg	= &genphy_config_aneg,
-	.read_status	= &genphy_read_status,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 	.driver		= { .owner = THIS_MODULE,},
diff --git a/drivers/net/phy/lxt.c b/drivers/net/phy/lxt.c
index 6f6e8b6..0ed7e51 100644
--- a/drivers/net/phy/lxt.c
+++ b/drivers/net/phy/lxt.c
@@ -156,8 +156,6 @@ static struct phy_driver lxt970_driver = {
 	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT,
 	.config_init	= lxt970_config_init,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.ack_interrupt	= lxt970_ack_interrupt,
 	.config_intr	= lxt970_config_intr,
 	.driver 	= { .owner = THIS_MODULE,},
@@ -169,8 +167,6 @@ static struct phy_driver lxt971_driver = {
 	.phy_id_mask	= 0xfffffff0,
 	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.ack_interrupt	= lxt971_ack_interrupt,
 	.config_intr	= lxt971_config_intr,
 	.driver 	= { .owner = THIS_MODULE,},
@@ -184,7 +180,6 @@ static struct phy_driver lxt973_driver = {
 	.flags		= 0,
 	.probe		= lxt973_probe,
 	.config_aneg	= lxt973_config_aneg,
-	.read_status	= genphy_read_status,
 	.driver 	= { .owner = THIS_MODULE,},
 };
 
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index e8b9c53..e4beb96 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -725,7 +725,6 @@ static struct phy_driver marvell_drivers[] = {
 		.features = PHY_GBIT_FEATURES,
 		.flags = PHY_HAS_INTERRUPT,
 		.config_aneg = &marvell_config_aneg,
-		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
 		.driver = { .owner = THIS_MODULE },
@@ -738,7 +737,6 @@ static struct phy_driver marvell_drivers[] = {
 		.flags = PHY_HAS_INTERRUPT,
 		.config_init = &m88e1111_config_init,
 		.config_aneg = &marvell_config_aneg,
-		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
 		.driver = { .owner = THIS_MODULE },
@@ -764,7 +762,6 @@ static struct phy_driver marvell_drivers[] = {
 		.flags = PHY_HAS_INTERRUPT,
 		.config_init = &m88e1118_config_init,
 		.config_aneg = &m88e1118_config_aneg,
-		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
 		.driver = {.owner = THIS_MODULE,},
@@ -803,7 +800,6 @@ static struct phy_driver marvell_drivers[] = {
 		.flags = PHY_HAS_INTERRUPT,
 		.config_init = &m88e1145_config_init,
 		.config_aneg = &marvell_config_aneg,
-		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
 		.driver = { .owner = THIS_MODULE },
@@ -816,7 +812,6 @@ static struct phy_driver marvell_drivers[] = {
 		.flags = PHY_HAS_INTERRUPT,
 		.config_init = &m88e1149_config_init,
 		.config_aneg = &m88e1118_config_aneg,
-		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
 		.driver = { .owner = THIS_MODULE },
@@ -829,7 +824,6 @@ static struct phy_driver marvell_drivers[] = {
 		.flags = PHY_HAS_INTERRUPT,
 		.config_init = &m88e1111_config_init,
 		.config_aneg = &marvell_config_aneg,
-		.read_status = &genphy_read_status,
 		.ack_interrupt = &marvell_ack_interrupt,
 		.config_intr = &marvell_config_intr,
 		.driver = { .owner = THIS_MODULE },
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 590f902..1404b3c 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -121,8 +121,6 @@ static struct phy_driver ks8737_driver = {
 	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause),
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= kszphy_config_init,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.ack_interrupt	= kszphy_ack_interrupt,
 	.config_intr	= ks8737_config_intr,
 	.driver		= { .owner = THIS_MODULE,},
@@ -136,8 +134,6 @@ static struct phy_driver ks8041_driver = {
 				| SUPPORTED_Asym_Pause),
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= kszphy_config_init,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.ack_interrupt	= kszphy_ack_interrupt,
 	.config_intr	= kszphy_config_intr,
 	.driver		= { .owner = THIS_MODULE,},
@@ -151,8 +147,6 @@ static struct phy_driver ks8051_driver = {
 				| SUPPORTED_Asym_Pause),
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= ks8051_config_init,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.ack_interrupt	= kszphy_ack_interrupt,
 	.config_intr	= kszphy_config_intr,
 	.driver		= { .owner = THIS_MODULE,},
@@ -165,8 +159,6 @@ static struct phy_driver ks8001_driver = {
 	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause),
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= kszphy_config_init,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.ack_interrupt	= kszphy_ack_interrupt,
 	.config_intr	= kszphy_config_intr,
 	.driver		= { .owner = THIS_MODULE,},
@@ -180,8 +172,6 @@ static struct phy_driver ksz9021_driver = {
 				| SUPPORTED_Asym_Pause),
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= kszphy_config_init,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.ack_interrupt	= kszphy_ack_interrupt,
 	.config_intr	= ksz9021_config_intr,
 	.driver		= { .owner = THIS_MODULE, },
diff --git a/drivers/net/phy/national.c b/drivers/net/phy/national.c
index 04bb8fc..9eca50e 100644
--- a/drivers/net/phy/national.c
+++ b/drivers/net/phy/national.c
@@ -132,8 +132,6 @@ static struct phy_driver dp83865_driver = {
 	.features = PHY_GBIT_FEATURES | SUPPORTED_Pause | SUPPORTED_Asym_Pause,
 	.flags = PHY_HAS_INTERRUPT,
 	.config_init = ns_config_init,
-	.config_aneg = genphy_config_aneg,
-	.read_status = genphy_read_status,
 	.ack_interrupt = ns_ack_interrupt,
 	.config_intr = ns_config_intr,
 	.driver = {.owner = THIS_MODULE,}
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 3cbda08..cc7f353 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -395,7 +395,10 @@ int phy_start_aneg(struct phy_device *phydev)
 	if (AUTONEG_DISABLE == phydev->autoneg)
 		phy_sanitize_settings(phydev);
 
-	err = phydev->drv->config_aneg(phydev);
+	if (phydev->drv->config_aneg)
+		err = phydev->drv->config_aneg(phydev);
+	else
+		err = genphy_config_aneg(phydev);
 
 	if (err < 0)
 		goto out_unlock;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index ff109fe..f1d8352 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1018,8 +1018,6 @@ static struct phy_driver genphy_driver = {
 	.name		= "Generic PHY",
 	.config_init	= genphy_config_init,
 	.features	= 0,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 	.driver		= {.owner= THIS_MODULE, },
diff --git a/drivers/net/phy/qsemi.c b/drivers/net/phy/qsemi.c
index fe0d0a1..5a120a8c 100644
--- a/drivers/net/phy/qsemi.c
+++ b/drivers/net/phy/qsemi.c
@@ -118,8 +118,6 @@ static struct phy_driver qs6612_driver = {
 	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT,
 	.config_init	= qs6612_config_init,
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.ack_interrupt	= qs6612_ack_interrupt,
 	.config_intr	= qs6612_config_intr,
 	.driver 	= { .owner = THIS_MODULE,},
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index a4eae75..1a00deb 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -55,8 +55,6 @@ static struct phy_driver rtl821x_driver = {
 	.phy_id_mask	= 0x001fffff,
 	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT,
-	.config_aneg	= &genphy_config_aneg,
-	.read_status	= &genphy_read_status,
 	.ack_interrupt	= &rtl821x_ack_interrupt,
 	.config_intr	= &rtl821x_config_intr,
 	.driver		= { .owner = THIS_MODULE,},
diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index 342505c..a8aa088 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -90,8 +90,6 @@ static struct phy_driver lan83c185_driver = {
 	.flags		= PHY_HAS_INTERRUPT | PHY_HAS_MAGICANEG,
 
 	/* basic functions */
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.config_init	= smsc_phy_config_init,
 
 	/* IRQ related */
@@ -114,8 +112,6 @@ static struct phy_driver lan8187_driver = {
 	.flags		= PHY_HAS_INTERRUPT | PHY_HAS_MAGICANEG,
 
 	/* basic functions */
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.config_init	= smsc_phy_config_init,
 
 	/* IRQ related */
@@ -138,8 +134,6 @@ static struct phy_driver lan8700_driver = {
 	.flags		= PHY_HAS_INTERRUPT | PHY_HAS_MAGICANEG,
 
 	/* basic functions */
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.config_init	= smsc_phy_config_init,
 
 	/* IRQ related */
@@ -162,8 +156,6 @@ static struct phy_driver lan911x_int_driver = {
 	.flags		= PHY_HAS_INTERRUPT | PHY_HAS_MAGICANEG,
 
 	/* basic functions */
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.config_init	= lan911x_config_init,
 
 	/* IRQ related */
@@ -186,8 +178,6 @@ static struct phy_driver lan8710_driver = {
 	.flags		= PHY_HAS_INTERRUPT | PHY_HAS_MAGICANEG,
 
 	/* basic functions */
-	.config_aneg	= genphy_config_aneg,
-	.read_status	= genphy_read_status,
 	.config_init	= smsc_phy_config_init,
 
 	/* IRQ related */
diff --git a/drivers/net/phy/ste10Xp.c b/drivers/net/phy/ste10Xp.c
index 187a2fa..45cde8f 100644
--- a/drivers/net/phy/ste10Xp.c
+++ b/drivers/net/phy/ste10Xp.c
@@ -88,8 +88,6 @@ static struct phy_driver ste101p_pdriver = {
 	.features = PHY_BASIC_FEATURES | SUPPORTED_Pause,
 	.flags = PHY_HAS_INTERRUPT,
 	.config_init = ste10Xp_config_init,
-	.config_aneg = genphy_config_aneg,
-	.read_status = genphy_read_status,
 	.ack_interrupt = ste10Xp_ack_interrupt,
 	.config_intr = ste10Xp_config_intr,
 	.suspend = genphy_suspend,
@@ -104,8 +102,6 @@ static struct phy_driver ste100p_pdriver = {
 	.features = PHY_BASIC_FEATURES | SUPPORTED_Pause,
 	.flags = PHY_HAS_INTERRUPT,
 	.config_init = ste10Xp_config_init,
-	.config_aneg = genphy_config_aneg,
-	.read_status = genphy_read_status,
 	.ack_interrupt = ste10Xp_ack_interrupt,
 	.config_intr = ste10Xp_config_intr,
 	.suspend = genphy_suspend,
diff --git a/drivers/net/phy/vitesse.c b/drivers/net/phy/vitesse.c
index 5d8f6e1..20ea438 100644
--- a/drivers/net/phy/vitesse.c
+++ b/drivers/net/phy/vitesse.c
@@ -136,8 +136,6 @@ static struct phy_driver vsc8244_driver = {
 	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT,
 	.config_init	= &vsc824x_config_init,
-	.config_aneg	= &genphy_config_aneg,
-	.read_status	= &genphy_read_status,
 	.ack_interrupt	= &vsc824x_ack_interrupt,
 	.config_intr	= &vsc82xx_config_intr,
 	.driver 	= { .owner = THIS_MODULE,},
@@ -163,8 +161,6 @@ static struct phy_driver vsc8221_driver = {
 	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT,
 	.config_init	= &vsc8221_config_init,
-	.config_aneg	= &genphy_config_aneg,
-	.read_status	= &genphy_read_status,
 	.ack_interrupt	= &vsc824x_ack_interrupt,
 	.config_intr	= &vsc82xx_config_intr,
 	.driver 	= { .owner = THIS_MODULE,},
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 54fc413..a55a6c4 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -349,8 +349,7 @@ struct phy_device {
  * flags: A bitfield defining certain other features this PHY
  *   supports (like interrupts)
  *
- * The drivers must implement config_aneg and read_status.  All
- * other functions are optional. Note that none of these
+ * All functions are optional.  Note that none of these
  * functions should be called from interrupt time.  The goal is
  * for the bus read/write functions to be able to block when the
  * bus transaction is happening, and be freed up by an interrupt
@@ -493,7 +492,10 @@ int phy_start_aneg(struct phy_device *phydev);
 int phy_stop_interrupts(struct phy_device *phydev);
 
 static inline int phy_read_status(struct phy_device *phydev) {
-	return phydev->drv->read_status(phydev);
+	if (phydev->drv->read_status)
+		return phydev->drv->read_status(phydev);
+	else
+		return genphy_read_status(phydev);
 }
 
 int genphy_restart_aneg(struct phy_device *phydev);
-- 
1.7.2.5

Re: [RFC PATCH 05/17] phy_driver: Make .read_status()/.config_aneg() optional

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1165 bytes --]

On Thursday 20 October 2011 17:00:12 Kyle Moffett wrote:
> Approximately 90% of the PHY drivers follow the PHY layer docs and
> simply use &genphy_read_status and &genphy_config_aneg.  There would
> seem to be little point in requiring them all to manually specify those
> functions.

well, it does make sense if you think about the compile vs build time 
overhead.  yes, your patch does make things much nicer to read, and a little 
easier to maintain the source.  however, it adds runtime overhead (checking 
the func pointers) while the func pointer storage is unchanged (it's now a 
NULL pointer instead of pointing to the genphy funcs).  personally, i think 
the savings in runtime and smaller compiled code is more important.  so i'm 
going to NAK this.  sorry.

> This patch makes it much easier for subsequent patches to split and
> refactor the functionality of the .config_aneg() method.
> 
> Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
> ---
>  Documentation/networking/phy.txt |   13 +++++--------
>  drivers/net/phy/bcm63xx.c        |    4 ----

hrm, what tree are you using ?  this driver is not in mainline.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 05/17] phy_driver: Make .read_status()/.config_aneg() optional

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 1601 bytes --]

On Thursday 20 October 2011 17:10:12 Mike Frysinger wrote:
> On Thursday 20 October 2011 17:00:12 Kyle Moffett wrote:
> > Approximately 90% of the PHY drivers follow the PHY layer docs and
> > simply use &genphy_read_status and &genphy_config_aneg.  There would
> > seem to be little point in requiring them all to manually specify those
> > functions.
> 
> well, it does make sense if you think about the compile vs build time
> overhead.  yes, your patch does make things much nicer to read, and a
> little easier to maintain the source.  however, it adds runtime overhead
> (checking the func pointers) while the func pointer storage is unchanged
> (it's now a NULL pointer instead of pointing to the genphy funcs). 
> personally, i think the savings in runtime and smaller compiled code is
> more important.  so i'm going to NAK this.  sorry.
> 
> > This patch makes it much easier for subsequent patches to split and
> > refactor the functionality of the .config_aneg() method.
> > 
> > Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
> > ---
> > 
> >  Documentation/networking/phy.txt |   13 +++++--------
> >  drivers/net/phy/bcm63xx.c        |    4 ----
> 
> hrm, what tree are you using ?  this driver is not in mainline.

ah, sorry, i was thinking this was u-boot since we were just having 
conversations there.

since this is Linux, and i don't have real standing in the general netdev 
community, i can't really NAK here.  but i think my comment still stands in 
that this patch makes things much worse than the minor code style improvement.
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 05/17] phy_driver: Make .read_status()/.config_aneg() optional

[Kyle Moffett]

Oops, I just realized that I may have misused
scripts/get_maintainer.pl... it seems to have added about 20 CCs to
this email, sorry!

On Thu, Oct 20, 2011 at 17:14, Mike Frysinger <vapier@gentoo.org> wrote:
> On Thursday 20 October 2011 17:10:12 Mike Frysinger wrote:
>> On Thursday 20 October 2011 17:00:12 Kyle Moffett wrote:
>> > Approximately 90% of the PHY drivers follow the PHY layer docs and
>> > simply use &genphy_read_status and &genphy_config_aneg.  There would
>> > seem to be little point in requiring them all to manually specify those
>> > functions.
>>
>> well, it does make sense if you think about the compile vs build time
>> overhead.  yes, your patch does make things much nicer to read, and a
>> little easier to maintain the source.  however, it adds runtime overhead
>> (checking the func pointers) while the func pointer storage is unchanged
>> (it's now a NULL pointer instead of pointing to the genphy funcs).
>> personally, i think the savings in runtime and smaller compiled code is
>> more important.  so i'm going to NAK this.  sorry.
>
> ah, sorry, i was thinking this was u-boot since we were just having
> conversations there.
>
> since this is Linux, and i don't have real standing in the general netdev
> community, i can't really NAK here.  but i think my comment still stands in
> that this patch makes things much worse than the minor code style improvement.

I would argue that the PHY layer itself is not remotely a hot-path,
especially not to the level of caring about an extra if statement.  A
single phy_read() is a sleeping call which goes over a slow serial
bus, so code maintainability is much more important.

At the higher level, the reason for this change is that currently
genphy_config_aneg() is currently responsible both for configuring
auto-negotiation *AND* counterintuitively for configuring forced-mode
links.  A lot of PHY drivers need to override just one or the other,
which results in either a lot of copy-pasted code in individual PHY
drivers or in a duplicate test for autoneg-vs-fixed-mode before
calling the primary genphy_config_aneg() function.

In order to split that up and refactor it, I would like to minimize
the number of callers and references to config_aneg() to minimize the
number of places that have to be changed to match.  Since all the rest
of the PHY functions are already conditionals, making these two also
conditional didn't seem to be a big deal.

Cheers,
Kyle Moffett

-- 
Curious about my work on the Debian powerpcspe port?
I'm keeping a blog here: http://pureperl.blogspot.com/

Re: [RFC PATCH 05/17] phy_driver: Make .read_status()/.config_aneg() optional

[Mike Frysinger]

On Fri, Oct 21, 2011 at 01:13, Kyle Moffett wrote:
> On Thu, Oct 20, 2011 at 17:14, Mike Frysinger wrote:
>> On Thursday 20 October 2011 17:10:12 Mike Frysinger wrote:
>>> On Thursday 20 October 2011 17:00:12 Kyle Moffett wrote:
>>> > Approximately 90% of the PHY drivers follow the PHY layer docs and
>>> > simply use &genphy_read_status and &genphy_config_aneg.  There would
>>> > seem to be little point in requiring them all to manually specify those
>>> > functions.
>>>
>>> well, it does make sense if you think about the compile vs build time
>>> overhead.  yes, your patch does make things much nicer to read, and a
>>> little easier to maintain the source.  however, it adds runtime overhead
>>> (checking the func pointers) while the func pointer storage is unchanged
>>> (it's now a NULL pointer instead of pointing to the genphy funcs).
>>> personally, i think the savings in runtime and smaller compiled code is
>>> more important.  so i'm going to NAK this.  sorry.
>>
>> ah, sorry, i was thinking this was u-boot since we were just having
>> conversations there.
>>
>> since this is Linux, and i don't have real standing in the general netdev
>> community, i can't really NAK here.  but i think my comment still stands in
>> that this patch makes things much worse than the minor code style improvement.
>
> I would argue that the PHY layer itself is not remotely a hot-path,
> especially not to the level of caring about an extra if statement.  A
> single phy_read() is a sleeping call which goes over a slow serial
> bus, so code maintainability is much more important.

i disagree.  ignoring that, what you ultimately desire can be
accomplished without bloating the kernel.

option 1: this can be done in the registration func just like the mtd
layer.  if (!func_pointer) func_pointer = default;

option 2: introduce a new macro in the common phy header similar to:
  #define PHY_DRIVER_DEFAULT_FUNCS \
    .config_aneg = genphy_config_aneg, \
    .read_status = genphy_read_status
and then use that in the phy_driver init structs:
  struct phy_driver bcm5411_driver = {
    PHY_DRIVER_DEFAULT_FUNCS,
    .config_aneg = bcm5411_config_aneg,
    ...

however, imo, these func pointer arrays really should be in the
.rodata section with proper const markers.  this would require
breaking out into a new phy_driver_opts struct since the phy_driver
struct has read/write fields (like the list structure).  option 2
should allow this to work.
-mike

[RFC PATCH 06/17] phy_driver: Add and use phy_driver_[un]register_multiple()

[Kyle Moffett]

Several of the PHY drivers are registering dozens of "phy_driver"
structures at load-time, with different IDs and variations on methods.
As a result, the error handling is exceptionally ugly.

The "marvell" driver already uses an array of "struct phy_driver" and
iterates over that to add the drivers, but even then its error handling
contains a bug (it will not unregister array item 0 on failure).

To resolve these problems, new functions phy_driver_register_multiple()
and phy_driver_unregister_multiple() are added which take an array and a
number of drivers and iterate over them internally.

All of the PHY driver files which register more than one "phy_driver"
are modified to use the new helpers.  This is a sizable net removal of
about 260 lines of code.

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---
 drivers/net/phy/bcm63xx.c    |   30 +++--------
 drivers/net/phy/broadcom.c   |  118 ++++++------------------------------------
 drivers/net/phy/cicada.c     |   32 +++---------
 drivers/net/phy/davicom.c    |   40 +++-----------
 drivers/net/phy/icplus.c     |   21 +++-----
 drivers/net/phy/lxt.c        |   40 +++-----------
 drivers/net/phy/marvell.c    |   22 ++------
 drivers/net/phy/micrel.c     |   61 ++++------------------
 drivers/net/phy/phy_device.c |   32 +++++++++++
 drivers/net/phy/realtek.c    |    6 +--
 drivers/net/phy/smsc.c       |   63 ++++-------------------
 drivers/net/phy/ste10Xp.c    |   20 +++-----
 drivers/net/phy/vitesse.c    |   45 ++++++----------
 include/linux/phy.h          |    2 +
 14 files changed, 138 insertions(+), 394 deletions(-)

diff --git a/drivers/net/phy/bcm63xx.c b/drivers/net/phy/bcm63xx.c
index c455f02..3b4258d 100644
--- a/drivers/net/phy/bcm63xx.c
+++ b/drivers/net/phy/bcm63xx.c
@@ -74,7 +74,7 @@ static int bcm63xx_config_intr(struct phy_device *phydev)
 	return err;
 }
 
-static struct phy_driver bcm63xx_1_driver = {
+static struct phy_driver bcm63xx_drivers[] = { {
 	.phy_id		= 0x00406000,
 	.phy_id_mask	= 0xfffffc00,
 	.name		= "Broadcom BCM63XX (1)",
@@ -85,10 +85,8 @@ static struct phy_driver bcm63xx_1_driver = {
 	.ack_interrupt	= bcm63xx_ack_interrupt,
 	.config_intr	= bcm63xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
-};
-
-/* same phy as above, with just a different OUI */
-static struct phy_driver bcm63xx_2_driver = {
+}, {
+	/* same phy as above, with just a different OUI */
 	.phy_id		= 0x002bdc00,
 	.phy_id_mask	= 0xfffffc00,
 	.name		= "Broadcom BCM63XX (2)",
@@ -98,30 +96,18 @@ static struct phy_driver bcm63xx_2_driver = {
 	.ack_interrupt	= bcm63xx_ack_interrupt,
 	.config_intr	= bcm63xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
-};
+} };
 
 static int __init bcm63xx_phy_init(void)
 {
-	int ret;
-
-	ret = phy_driver_register(&bcm63xx_1_driver);
-	if (ret)
-		goto out_63xx_1;
-	ret = phy_driver_register(&bcm63xx_2_driver);
-	if (ret)
-		goto out_63xx_2;
-	return ret;
-
-out_63xx_2:
-	phy_driver_unregister(&bcm63xx_1_driver);
-out_63xx_1:
-	return ret;
+	return phy_driver_register_multiple(bcm63xx_drivers,
+			ARRAY_SIZE(bcm63xx_drivers));
 }
 
 static void __exit bcm63xx_phy_exit(void)
 {
-	phy_driver_unregister(&bcm63xx_1_driver);
-	phy_driver_unregister(&bcm63xx_2_driver);
+	phy_driver_unregister_multiple(bcm63xx_drivers,
+			ARRAY_SIZE(bcm63xx_drivers));
 }
 
 module_init(bcm63xx_phy_init);
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index f220264..1b83f75 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -684,7 +684,7 @@ static int brcm_fet_config_intr(struct phy_device *phydev)
 	return err;
 }
 
-static struct phy_driver bcm5411_driver = {
+static struct phy_driver broadcom_drivers[] = { {
 	.phy_id		= PHY_ID_BCM5411,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM5411",
@@ -695,9 +695,7 @@ static struct phy_driver bcm5411_driver = {
 	.ack_interrupt	= bcm54xx_ack_interrupt,
 	.config_intr	= bcm54xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
-};
-
-static struct phy_driver bcm5421_driver = {
+}, {
 	.phy_id		= PHY_ID_BCM5421,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM5421",
@@ -708,9 +706,7 @@ static struct phy_driver bcm5421_driver = {
 	.ack_interrupt	= bcm54xx_ack_interrupt,
 	.config_intr	= bcm54xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
-};
-
-static struct phy_driver bcm5461_driver = {
+}, {
 	.phy_id		= PHY_ID_BCM5461,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM5461",
@@ -721,9 +717,7 @@ static struct phy_driver bcm5461_driver = {
 	.ack_interrupt	= bcm54xx_ack_interrupt,
 	.config_intr	= bcm54xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
-};
-
-static struct phy_driver bcm5464_driver = {
+}, {
 	.phy_id		= PHY_ID_BCM5464,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM5464",
@@ -734,9 +728,7 @@ static struct phy_driver bcm5464_driver = {
 	.ack_interrupt	= bcm54xx_ack_interrupt,
 	.config_intr	= bcm54xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
-};
-
-static struct phy_driver bcm5481_driver = {
+}, {
 	.phy_id		= PHY_ID_BCM5481,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM5481",
@@ -748,9 +740,7 @@ static struct phy_driver bcm5481_driver = {
 	.ack_interrupt	= bcm54xx_ack_interrupt,
 	.config_intr	= bcm54xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
-};
-
-static struct phy_driver bcm5482_driver = {
+}, {
 	.phy_id		= PHY_ID_BCM5482,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM5482",
@@ -762,9 +752,7 @@ static struct phy_driver bcm5482_driver = {
 	.ack_interrupt	= bcm54xx_ack_interrupt,
 	.config_intr	= bcm54xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
-};
-
-static struct phy_driver bcm50610_driver = {
+}, {
 	.phy_id		= PHY_ID_BCM50610,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM50610",
@@ -775,9 +763,7 @@ static struct phy_driver bcm50610_driver = {
 	.ack_interrupt	= bcm54xx_ack_interrupt,
 	.config_intr	= bcm54xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
-};
-
-static struct phy_driver bcm50610m_driver = {
+}, {
 	.phy_id		= PHY_ID_BCM50610M,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM50610M",
@@ -788,9 +774,7 @@ static struct phy_driver bcm50610m_driver = {
 	.ack_interrupt	= bcm54xx_ack_interrupt,
 	.config_intr	= bcm54xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
-};
-
-static struct phy_driver bcm57780_driver = {
+}, {
 	.phy_id		= PHY_ID_BCM57780,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM57780",
@@ -801,9 +785,7 @@ static struct phy_driver bcm57780_driver = {
 	.ack_interrupt	= bcm54xx_ack_interrupt,
 	.config_intr	= bcm54xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
-};
-
-static struct phy_driver bcmac131_driver = {
+}, {
 	.phy_id		= PHY_ID_BCMAC131,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCMAC131",
@@ -814,9 +796,7 @@ static struct phy_driver bcmac131_driver = {
 	.ack_interrupt	= brcm_fet_ack_interrupt,
 	.config_intr	= brcm_fet_config_intr,
 	.driver		= { .owner = THIS_MODULE },
-};
-
-static struct phy_driver bcm5241_driver = {
+}, {
 	.phy_id		= PHY_ID_BCM5241,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM5241",
@@ -827,84 +807,18 @@ static struct phy_driver bcm5241_driver = {
 	.ack_interrupt	= brcm_fet_ack_interrupt,
 	.config_intr	= brcm_fet_config_intr,
 	.driver		= { .owner = THIS_MODULE },
-};
+} };
 
 static int __init broadcom_init(void)
 {
-	int ret;
-
-	ret = phy_driver_register(&bcm5411_driver);
-	if (ret)
-		goto out_5411;
-	ret = phy_driver_register(&bcm5421_driver);
-	if (ret)
-		goto out_5421;
-	ret = phy_driver_register(&bcm5461_driver);
-	if (ret)
-		goto out_5461;
-	ret = phy_driver_register(&bcm5464_driver);
-	if (ret)
-		goto out_5464;
-	ret = phy_driver_register(&bcm5481_driver);
-	if (ret)
-		goto out_5481;
-	ret = phy_driver_register(&bcm5482_driver);
-	if (ret)
-		goto out_5482;
-	ret = phy_driver_register(&bcm50610_driver);
-	if (ret)
-		goto out_50610;
-	ret = phy_driver_register(&bcm50610m_driver);
-	if (ret)
-		goto out_50610m;
-	ret = phy_driver_register(&bcm57780_driver);
-	if (ret)
-		goto out_57780;
-	ret = phy_driver_register(&bcmac131_driver);
-	if (ret)
-		goto out_ac131;
-	ret = phy_driver_register(&bcm5241_driver);
-	if (ret)
-		goto out_5241;
-	return ret;
-
-out_5241:
-	phy_driver_unregister(&bcmac131_driver);
-out_ac131:
-	phy_driver_unregister(&bcm57780_driver);
-out_57780:
-	phy_driver_unregister(&bcm50610m_driver);
-out_50610m:
-	phy_driver_unregister(&bcm50610_driver);
-out_50610:
-	phy_driver_unregister(&bcm5482_driver);
-out_5482:
-	phy_driver_unregister(&bcm5481_driver);
-out_5481:
-	phy_driver_unregister(&bcm5464_driver);
-out_5464:
-	phy_driver_unregister(&bcm5461_driver);
-out_5461:
-	phy_driver_unregister(&bcm5421_driver);
-out_5421:
-	phy_driver_unregister(&bcm5411_driver);
-out_5411:
-	return ret;
+	return phy_driver_register_multiple(broadcom_drivers,
+			ARRAY_SIZE(broadcom_drivers));
 }
 
 static void __exit broadcom_exit(void)
 {
-	phy_driver_unregister(&bcm5241_driver);
-	phy_driver_unregister(&bcmac131_driver);
-	phy_driver_unregister(&bcm57780_driver);
-	phy_driver_unregister(&bcm50610m_driver);
-	phy_driver_unregister(&bcm50610_driver);
-	phy_driver_unregister(&bcm5482_driver);
-	phy_driver_unregister(&bcm5481_driver);
-	phy_driver_unregister(&bcm5464_driver);
-	phy_driver_unregister(&bcm5461_driver);
-	phy_driver_unregister(&bcm5421_driver);
-	phy_driver_unregister(&bcm5411_driver);
+	phy_driver_unregister_multiple(broadcom_drivers,
+			ARRAY_SIZE(broadcom_drivers));
 }
 
 module_init(broadcom_init);
diff --git a/drivers/net/phy/cicada.c b/drivers/net/phy/cicada.c
index c409ca2..9c805cd 100644
--- a/drivers/net/phy/cicada.c
+++ b/drivers/net/phy/cicada.c
@@ -101,8 +101,8 @@ static int cis820x_config_intr(struct phy_device *phydev)
 	return err;
 }
 
-/* Cicada 8201, a.k.a Vitesse VSC8201 */
-static struct phy_driver cis8201_driver = {
+static struct phy_driver cicada_drivers[] = { {
+	/* Cicada 8201, a.k.a Vitesse VSC8201 */
 	.phy_id		= 0x000fc410,
 	.name		= "Cicada Cis8201",
 	.phy_id_mask	= 0x000ffff0,
@@ -112,10 +112,7 @@ static struct phy_driver cis8201_driver = {
 	.ack_interrupt	= &cis820x_ack_interrupt,
 	.config_intr	= &cis820x_config_intr,
 	.driver 	= { .owner = THIS_MODULE,},
-};
-
-/* Cicada 8204 */
-static struct phy_driver cis8204_driver = {
+}, {
 	.phy_id		= 0x000fc440,
 	.name		= "Cicada Cis8204",
 	.phy_id_mask	= 0x000fffc0,
@@ -125,31 +122,18 @@ static struct phy_driver cis8204_driver = {
 	.ack_interrupt	= &cis820x_ack_interrupt,
 	.config_intr	= &cis820x_config_intr,
 	.driver 	= { .owner = THIS_MODULE,},
-};
+} };
 
 static int __init cicada_init(void)
 {
-	int ret;
-
-	ret = phy_driver_register(&cis8204_driver);
-	if (ret)
-		goto err1;
-
-	ret = phy_driver_register(&cis8201_driver);
-	if (ret)
-		goto err2;
-	return 0;
-
-err2:
-	phy_driver_unregister(&cis8204_driver);
-err1:
-	return ret;
+	return phy_driver_register_multiple(cicada_drivers,
+			ARRAY_SIZE(cicada_drivers));
 }
 
 static void __exit cicada_exit(void)
 {
-	phy_driver_unregister(&cis8204_driver);
-	phy_driver_unregister(&cis8201_driver);
+	phy_driver_unregister_multiple(cicada_drivers,
+			ARRAY_SIZE(cicada_drivers));
 }
 
 module_init(cicada_init);
diff --git a/drivers/net/phy/davicom.c b/drivers/net/phy/davicom.c
index 5249e1e..435be46 100644
--- a/drivers/net/phy/davicom.c
+++ b/drivers/net/phy/davicom.c
@@ -149,7 +149,7 @@ static int dm9161_ack_interrupt(struct phy_device *phydev)
 	return (err < 0) ? err : 0;
 }
 
-static struct phy_driver dm9161e_driver = {
+static struct phy_driver davicom_drivers[] = { {
 	.phy_id		= 0x0181b880,
 	.name		= "Davicom DM9161E",
 	.phy_id_mask	= 0x0ffffff0,
@@ -157,9 +157,7 @@ static struct phy_driver dm9161e_driver = {
 	.config_init	= dm9161_config_init,
 	.config_aneg	= dm9161_config_aneg,
 	.driver		= { .owner = THIS_MODULE,},
-};
-
-static struct phy_driver dm9161a_driver = {
+}, {
 	.phy_id		= 0x0181b8a0,
 	.name		= "Davicom DM9161A",
 	.phy_id_mask	= 0x0ffffff0,
@@ -167,9 +165,7 @@ static struct phy_driver dm9161a_driver = {
 	.config_init	= dm9161_config_init,
 	.config_aneg	= dm9161_config_aneg,
 	.driver		= { .owner = THIS_MODULE,},
-};
-
-static struct phy_driver dm9131_driver = {
+}, {
 	.phy_id		= 0x00181b80,
 	.name		= "Davicom DM9131",
 	.phy_id_mask	= 0x0ffffff0,
@@ -178,38 +174,18 @@ static struct phy_driver dm9131_driver = {
 	.ack_interrupt	= dm9161_ack_interrupt,
 	.config_intr	= dm9161_config_intr,
 	.driver		= { .owner = THIS_MODULE,},
-};
+} };
 
 static int __init davicom_init(void)
 {
-	int ret;
-
-	ret = phy_driver_register(&dm9161e_driver);
-	if (ret)
-		goto err1;
-
-	ret = phy_driver_register(&dm9161a_driver);
-	if (ret)
-		goto err2;
-
-	ret = phy_driver_register(&dm9131_driver);
-	if (ret)
-		goto err3;
-	return 0;
-
- err3:
-	phy_driver_unregister(&dm9161a_driver);
- err2:
-	phy_driver_unregister(&dm9161e_driver);
- err1:
-	return ret;
+	return phy_driver_register_multiple(davicom_drivers,
+			ARRAY_SIZE(davicom_drivers));
 }
 
 static void __exit davicom_exit(void)
 {
-	phy_driver_unregister(&dm9161e_driver);
-	phy_driver_unregister(&dm9161a_driver);
-	phy_driver_unregister(&dm9131_driver);
+	phy_driver_unregister_multiple(davicom_drivers,
+			ARRAY_SIZE(davicom_drivers));
 }
 
 module_init(davicom_init);
diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index 28a190e..b23c78f 100644
--- a/drivers/net/phy/icplus.c
+++ b/drivers/net/phy/icplus.c
@@ -131,7 +131,7 @@ static int ip175c_config_aneg(struct phy_device *phydev)
 	return 0;
 }
 
-static struct phy_driver ip175c_driver = {
+static struct phy_driver icplus_drivers[] = { {
 	.phy_id		= 0x02430d80,
 	.name		= "ICPlus IP175C",
 	.phy_id_mask	= 0x0ffffff0,
@@ -142,9 +142,7 @@ static struct phy_driver ip175c_driver = {
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 	.driver		= { .owner = THIS_MODULE,},
-};
-
-static struct phy_driver ip1001_driver = {
+}, {
 	.phy_id		= 0x02430d90,
 	.name		= "ICPlus IP1001",
 	.phy_id_mask	= 0x0ffffff0,
@@ -154,23 +152,18 @@ static struct phy_driver ip1001_driver = {
 	.suspend	= genphy_suspend,
 	.resume		= genphy_resume,
 	.driver		= { .owner = THIS_MODULE,},
-};
+} };
 
 static int __init icplus_init(void)
 {
-	int ret = 0;
-
-	ret = phy_driver_register(&ip1001_driver);
-	if (ret < 0)
-		return -ENODEV;
-
-	return phy_driver_register(&ip175c_driver);
+	return phy_driver_register_multiple(icplus_drivers,
+			ARRAY_SIZE(icplus_drivers));
 }
 
 static void __exit icplus_exit(void)
 {
-	phy_driver_unregister(&ip1001_driver);
-	phy_driver_unregister(&ip175c_driver);
+	phy_driver_unregister_multiple(icplus_drivers,
+			ARRAY_SIZE(icplus_drivers));
 }
 
 module_init(icplus_init);
diff --git a/drivers/net/phy/lxt.c b/drivers/net/phy/lxt.c
index 0ed7e51..902d2d1 100644
--- a/drivers/net/phy/lxt.c
+++ b/drivers/net/phy/lxt.c
@@ -149,7 +149,7 @@ static int lxt973_config_aneg(struct phy_device *phydev)
 	return phydev->priv ? 0 : genphy_config_aneg(phydev);
 }
 
-static struct phy_driver lxt970_driver = {
+static struct phy_driver lxt_drivers[] = { {
 	.phy_id		= 0x78100000,
 	.name		= "LXT970",
 	.phy_id_mask	= 0xfffffff0,
@@ -159,9 +159,7 @@ static struct phy_driver lxt970_driver = {
 	.ack_interrupt	= lxt970_ack_interrupt,
 	.config_intr	= lxt970_config_intr,
 	.driver 	= { .owner = THIS_MODULE,},
-};
-
-static struct phy_driver lxt971_driver = {
+}, {
 	.phy_id		= 0x001378e0,
 	.name		= "LXT971",
 	.phy_id_mask	= 0xfffffff0,
@@ -170,9 +168,7 @@ static struct phy_driver lxt971_driver = {
 	.ack_interrupt	= lxt971_ack_interrupt,
 	.config_intr	= lxt971_config_intr,
 	.driver 	= { .owner = THIS_MODULE,},
-};
-
-static struct phy_driver lxt973_driver = {
+}, {
 	.phy_id		= 0x00137a10,
 	.name		= "LXT973",
 	.phy_id_mask	= 0xfffffff0,
@@ -181,38 +177,18 @@ static struct phy_driver lxt973_driver = {
 	.probe		= lxt973_probe,
 	.config_aneg	= lxt973_config_aneg,
 	.driver 	= { .owner = THIS_MODULE,},
-};
+} };
 
 static int __init lxt_init(void)
 {
-	int ret;
-
-	ret = phy_driver_register(&lxt970_driver);
-	if (ret)
-		goto err1;
-
-	ret = phy_driver_register(&lxt971_driver);
-	if (ret)
-		goto err2;
-
-	ret = phy_driver_register(&lxt973_driver);
-	if (ret)
-		goto err3;
-	return 0;
-
- err3:
-	phy_driver_unregister(&lxt971_driver);
- err2:
-	phy_driver_unregister(&lxt970_driver);
- err1:
-	return ret;
+	return phy_driver_register_multiple(lxt_drivers,
+			ARRAY_SIZE(lxt_drivers));
 }
 
 static void __exit lxt_exit(void)
 {
-	phy_driver_unregister(&lxt970_driver);
-	phy_driver_unregister(&lxt971_driver);
-	phy_driver_unregister(&lxt973_driver);
+	phy_driver_unregister_multiple(lxt_drivers,
+			ARRAY_SIZE(lxt_drivers));
 }
 
 module_init(lxt_init);
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index e4beb96..9aaae96 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -832,28 +832,14 @@ static struct phy_driver marvell_drivers[] = {
 
 static int __init marvell_init(void)
 {
-	int ret;
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(marvell_drivers); i++) {
-		ret = phy_driver_register(&marvell_drivers[i]);
-
-		if (ret) {
-			while (i-- > 0)
-				phy_driver_unregister(&marvell_drivers[i]);
-			return ret;
-		}
-	}
-
-	return 0;
+	return phy_driver_register_multiple(marvell_drivers,
+			ARRAY_SIZE(marvell_drivers));
 }
 
 static void __exit marvell_exit(void)
 {
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(marvell_drivers); i++)
-		phy_driver_unregister(&marvell_drivers[i]);
+	phy_driver_unregister_multiple(marvell_drivers,
+			ARRAY_SIZE(marvell_drivers));
 }
 
 module_init(marvell_init);
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 1404b3c..1359a2e 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -114,7 +114,7 @@ static int ks8051_config_init(struct phy_device *phydev)
 	return 0;
 }
 
-static struct phy_driver ks8737_driver = {
+static struct phy_driver ksphy_drivers[] = { {
 	.phy_id		= PHY_ID_KS8737,
 	.phy_id_mask	= 0x00fffff0,
 	.name		= "Micrel KS8737",
@@ -124,9 +124,7 @@ static struct phy_driver ks8737_driver = {
 	.ack_interrupt	= kszphy_ack_interrupt,
 	.config_intr	= ks8737_config_intr,
 	.driver		= { .owner = THIS_MODULE,},
-};
-
-static struct phy_driver ks8041_driver = {
+}, {
 	.phy_id		= PHY_ID_KS8041,
 	.phy_id_mask	= 0x00fffff0,
 	.name		= "Micrel KS8041",
@@ -137,9 +135,7 @@ static struct phy_driver ks8041_driver = {
 	.ack_interrupt	= kszphy_ack_interrupt,
 	.config_intr	= kszphy_config_intr,
 	.driver		= { .owner = THIS_MODULE,},
-};
-
-static struct phy_driver ks8051_driver = {
+}, {
 	.phy_id		= PHY_ID_KS8051,
 	.phy_id_mask	= 0x00fffff0,
 	.name		= "Micrel KS8051",
@@ -150,9 +146,7 @@ static struct phy_driver ks8051_driver = {
 	.ack_interrupt	= kszphy_ack_interrupt,
 	.config_intr	= kszphy_config_intr,
 	.driver		= { .owner = THIS_MODULE,},
-};
-
-static struct phy_driver ks8001_driver = {
+}, {
 	.phy_id		= PHY_ID_KS8001,
 	.name		= "Micrel KS8001 or KS8721",
 	.phy_id_mask	= 0x00fffff0,
@@ -162,9 +156,7 @@ static struct phy_driver ks8001_driver = {
 	.ack_interrupt	= kszphy_ack_interrupt,
 	.config_intr	= kszphy_config_intr,
 	.driver		= { .owner = THIS_MODULE,},
-};
-
-static struct phy_driver ksz9021_driver = {
+}, {
 	.phy_id		= PHY_ID_KSZ9021,
 	.phy_id_mask	= 0x000fff10,
 	.name		= "Micrel KSZ9021 Gigabit PHY",
@@ -175,51 +167,18 @@ static struct phy_driver ksz9021_driver = {
 	.ack_interrupt	= kszphy_ack_interrupt,
 	.config_intr	= ksz9021_config_intr,
 	.driver		= { .owner = THIS_MODULE, },
-};
+} };
 
 static int __init ksphy_init(void)
 {
-	int ret;
-
-	ret = phy_driver_register(&ks8001_driver);
-	if (ret)
-		goto err1;
-
-	ret = phy_driver_register(&ksz9021_driver);
-	if (ret)
-		goto err2;
-
-	ret = phy_driver_register(&ks8737_driver);
-	if (ret)
-		goto err3;
-	ret = phy_driver_register(&ks8041_driver);
-	if (ret)
-		goto err4;
-	ret = phy_driver_register(&ks8051_driver);
-	if (ret)
-		goto err5;
-
-	return 0;
-
-err5:
-	phy_driver_unregister(&ks8041_driver);
-err4:
-	phy_driver_unregister(&ks8737_driver);
-err3:
-	phy_driver_unregister(&ksz9021_driver);
-err2:
-	phy_driver_unregister(&ks8001_driver);
-err1:
-	return ret;
+	return phy_driver_register_multiple(ksphy_drivers,
+			ARRAY_SIZE(ksphy_drivers));
 }
 
 static void __exit ksphy_exit(void)
 {
-	phy_driver_unregister(&ks8001_driver);
-	phy_driver_unregister(&ks8737_driver);
-	phy_driver_unregister(&ksz9021_driver);
-	phy_driver_unregister(&ks8041_driver);
-	phy_driver_unregister(&ks8051_driver);
+	phy_driver_unregister_multiple(ksphy_drivers,
+			ARRAY_SIZE(ksphy_drivers));
 }
 
 module_init(ksphy_init);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f1d8352..8990e87 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1012,6 +1012,38 @@ void phy_driver_unregister(struct phy_driver *drv)
 }
 EXPORT_SYMBOL(phy_driver_unregister);
 
+/**
+ * phy_driver_register_multiple - register an array of phy_drivers
+ * @drivers: array of phy_drivers to register
+ */
+int phy_driver_register_multiple(struct phy_driver *drivers,
+		unsigned long nr_drivers)
+{
+	unsigned long i;
+	for (i = 0; i < nr_drivers; i++) {
+		int retval = phy_driver_register(&drivers[i]);
+		if (retval) {
+			/*
+			 * A failure occurred, so unregister everything that
+			 * was already successfully registered.
+			 */
+			phy_driver_unregister_multiple__(drivers, i);
+			return retval;
+		}
+	}
+	return 0;
+}
+EXPORT_SYMBOL(phy_driver_register_multiple);
+
+void phy_driver_unregister_multiple(struct phy_driver *drivers,
+		unsigned long nr_drivers)
+{
+	unsigned long i;
+	for (i = nr_drivers; i > 0; i++)
+		phy_driver_unregister(drvs[i-1]);
+}
+EXPORT_SYMBOL(phy_driver_unregister_multiple);
+
 static struct phy_driver genphy_driver = {
 	.phy_id		= 0xffffffff,
 	.phy_id_mask	= 0xffffffff,
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 1a00deb..0172248 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -62,11 +62,7 @@ static struct phy_driver rtl821x_driver = {
 
 static int __init realtek_init(void)
 {
-	int ret;
-
-	ret = phy_driver_register(&rtl821x_driver);
-
-	return ret;
+	return phy_driver_register(&rtl821x_driver);
 }
 
 static void __exit realtek_exit(void)
diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index a8aa088..eb05b0f 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -80,7 +80,7 @@ static int lan911x_config_init(struct phy_device *phydev)
 	return smsc_phy_ack_interrupt(phydev);
 }
 
-static struct phy_driver lan83c185_driver = {
+static struct phy_driver smsc_drivers[] = { {
 	.phy_id		= 0x0007c0a0, /* OUI=0x00800f, Model#=0x0a */
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "SMSC LAN83C185",
@@ -100,9 +100,7 @@ static struct phy_driver lan83c185_driver = {
 	.resume		= genphy_resume,
 
 	.driver		= { .owner = THIS_MODULE, }
-};
-
-static struct phy_driver lan8187_driver = {
+}, {
 	.phy_id		= 0x0007c0b0, /* OUI=0x00800f, Model#=0x0b */
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "SMSC LAN8187",
@@ -122,9 +120,7 @@ static struct phy_driver lan8187_driver = {
 	.resume		= genphy_resume,
 
 	.driver		= { .owner = THIS_MODULE, }
-};
-
-static struct phy_driver lan8700_driver = {
+}, {
 	.phy_id		= 0x0007c0c0, /* OUI=0x00800f, Model#=0x0c */
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "SMSC LAN8700",
@@ -144,9 +140,7 @@ static struct phy_driver lan8700_driver = {
 	.resume		= genphy_resume,
 
 	.driver		= { .owner = THIS_MODULE, }
-};
-
-static struct phy_driver lan911x_int_driver = {
+}, {
 	.phy_id		= 0x0007c0d0, /* OUI=0x00800f, Model#=0x0d */
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "SMSC LAN911x Internal PHY",
@@ -166,9 +160,7 @@ static struct phy_driver lan911x_int_driver = {
 	.resume		= genphy_resume,
 
 	.driver		= { .owner = THIS_MODULE, }
-};
-
-static struct phy_driver lan8710_driver = {
+}, {
 	.phy_id		= 0x0007c0f0, /* OUI=0x00800f, Model#=0x0f */
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "SMSC LAN8710/LAN8720",
@@ -188,53 +180,18 @@ static struct phy_driver lan8710_driver = {
 	.resume		= genphy_resume,
 
 	.driver		= { .owner = THIS_MODULE, }
-};
+} };
 
 static int __init smsc_init(void)
 {
-	int ret;
-
-	ret = phy_driver_register (&lan83c185_driver);
-	if (ret)
-		goto err1;
-
-	ret = phy_driver_register (&lan8187_driver);
-	if (ret)
-		goto err2;
-
-	ret = phy_driver_register (&lan8700_driver);
-	if (ret)
-		goto err3;
-
-	ret = phy_driver_register (&lan911x_int_driver);
-	if (ret)
-		goto err4;
-
-	ret = phy_driver_register (&lan8710_driver);
-	if (ret)
-		goto err5;
-
-	return 0;
-
-err5:
-	phy_driver_unregister (&lan911x_int_driver);
-err4:
-	phy_driver_unregister (&lan8700_driver);
-err3:
-	phy_driver_unregister (&lan8187_driver);
-err2:
-	phy_driver_unregister (&lan83c185_driver);
-err1:
-	return ret;
+	return phy_driver_register_multiple(smsc_drivers,
+			ARRAY_SIZE(smsc_drivers));
 }
 
 static void __exit smsc_exit(void)
 {
-	phy_driver_unregister (&lan8710_driver);
-	phy_driver_unregister (&lan911x_int_driver);
-	phy_driver_unregister (&lan8700_driver);
-	phy_driver_unregister (&lan8187_driver);
-	phy_driver_unregister (&lan83c185_driver);
+	phy_driver_unregister_multiple(smsc_drivers,
+			ARRAY_SIZE(smsc_drivers));
 }
 
 MODULE_DESCRIPTION("SMSC PHY driver");
diff --git a/drivers/net/phy/ste10Xp.c b/drivers/net/phy/ste10Xp.c
index 45cde8f..855589c 100644
--- a/drivers/net/phy/ste10Xp.c
+++ b/drivers/net/phy/ste10Xp.c
@@ -81,7 +81,7 @@ static int ste10Xp_ack_interrupt(struct phy_device *phydev)
 	return 0;
 }
 
-static struct phy_driver ste101p_pdriver = {
+static struct phy_driver ste10Xp_drivers[] = { {
 	.phy_id = STE101P_PHY_ID,
 	.phy_id_mask = 0xfffffff0,
 	.name = "STe101p",
@@ -93,9 +93,7 @@ static struct phy_driver ste101p_pdriver = {
 	.suspend = genphy_suspend,
 	.resume = genphy_resume,
 	.driver = {.owner = THIS_MODULE,}
-};
-
-static struct phy_driver ste100p_pdriver = {
+}, {
 	.phy_id = STE100P_PHY_ID,
 	.phy_id_mask = 0xffffffff,
 	.name = "STe100p",
@@ -107,22 +105,18 @@ static struct phy_driver ste100p_pdriver = {
 	.suspend = genphy_suspend,
 	.resume = genphy_resume,
 	.driver = {.owner = THIS_MODULE,}
-};
+} };
 
 static int __init ste10Xp_init(void)
 {
-	int retval;
-
-	retval = phy_driver_register(&ste100p_pdriver);
-	if (retval < 0)
-		return retval;
-	return phy_driver_register(&ste101p_pdriver);
+	return phy_driver_register_multiple(ste10Xp_drivers,
+			ARRAY_SIZE(ste10Xp_drivers));
 }
 
 static void __exit ste10Xp_exit(void)
 {
-	phy_driver_unregister(&ste100p_pdriver);
-	phy_driver_unregister(&ste101p_pdriver);
+	phy_driver_unregister_multiple(ste10Xp_drivers,
+			ARRAY_SIZE(ste10Xp_drivers));
 }
 
 module_init(ste10Xp_init);
diff --git a/drivers/net/phy/vitesse.c b/drivers/net/phy/vitesse.c
index 20ea438..82f808e 100644
--- a/drivers/net/phy/vitesse.c
+++ b/drivers/net/phy/vitesse.c
@@ -128,19 +128,6 @@ static int vsc82xx_config_intr(struct phy_device *phydev)
 	return err;
 }
 
-/* Vitesse 824x */
-static struct phy_driver vsc8244_driver = {
-	.phy_id		= PHY_ID_VSC8244,
-	.name		= "Vitesse VSC8244",
-	.phy_id_mask	= 0x000fffc0,
-	.features	= PHY_GBIT_FEATURES,
-	.flags		= PHY_HAS_INTERRUPT,
-	.config_init	= &vsc824x_config_init,
-	.ack_interrupt	= &vsc824x_ack_interrupt,
-	.config_intr	= &vsc82xx_config_intr,
-	.driver 	= { .owner = THIS_MODULE,},
-};
-
 static int vsc8221_config_init(struct phy_device *phydev)
 {
 	int err;
@@ -153,8 +140,17 @@ static int vsc8221_config_init(struct phy_device *phydev)
 	   Options are 802.3Z SerDes or SGMII */
 }
 
-/* Vitesse 8221 */
-static struct phy_driver vsc8221_driver = {
+static struct phy_driver vitesse_drivers[] = { {
+	.phy_id		= PHY_ID_VSC8244,
+	.name		= "Vitesse VSC8244",
+	.phy_id_mask	= 0x000fffc0,
+	.features	= PHY_GBIT_FEATURES,
+	.flags		= PHY_HAS_INTERRUPT,
+	.config_init	= &vsc824x_config_init,
+	.ack_interrupt	= &vsc824x_ack_interrupt,
+	.config_intr	= &vsc82xx_config_intr,
+	.driver		= { .owner = THIS_MODULE, },
+}, {
 	.phy_id		= PHY_ID_VSC8221,
 	.phy_id_mask	= 0x000ffff0,
 	.name		= "Vitesse VSC8221",
@@ -163,26 +159,19 @@ static struct phy_driver vsc8221_driver = {
 	.config_init	= &vsc8221_config_init,
 	.ack_interrupt	= &vsc824x_ack_interrupt,
 	.config_intr	= &vsc82xx_config_intr,
-	.driver 	= { .owner = THIS_MODULE,},
-};
+	.driver		= { .owner = THIS_MODULE, },
+} };
 
 static int __init vsc82xx_init(void)
 {
-	int err;
-
-	err = phy_driver_register(&vsc8244_driver);
-	if (err < 0)
-		return err;
-	err = phy_driver_register(&vsc8221_driver);
-	if (err < 0)
-		phy_driver_unregister(&vsc8244_driver);
-	return err;
+	return phy_driver_register_multiple(vitesse_drivers,
+			ARRAY_SIZE(vitesse_drivers));
 }
 
 static void __exit vsc82xx_exit(void)
 {
-	phy_driver_unregister(&vsc8244_driver);
-	phy_driver_unregister(&vsc8221_driver);
+	phy_driver_unregister_multiple(vitesse_drivers,
+			ARRAY_SIZE(vitesse_drivers));
 }
 
 module_init(vsc82xx_init);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index a55a6c4..c6bbb38 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -505,7 +505,9 @@ int genphy_read_status(struct phy_device *phydev);
 int genphy_suspend(struct phy_device *phydev);
 int genphy_resume(struct phy_device *phydev);
 void phy_driver_unregister(struct phy_driver *drv);
+void phy_driver_unregister_multiple(struct phy_driver *drvs, unsigned long nr);
 int phy_driver_register(struct phy_driver *new_driver);
+int phy_driver_register_multiple(struct phy_driver *drvs, unsigned long nr);
 void phy_state_machine(struct work_struct *work);
 void phy_start_machine(struct phy_device *phydev,
 		void (*handler)(struct net_device *));
-- 
1.7.2.5

Re: [RFC PATCH 06/17] phy_driver: Add and use phy_driver_[un]register_multiple()

[Mike Frysinger]

[-- Attachment #1: Type: Text/Plain, Size: 390 bytes --]

On Thursday 20 October 2011 17:00:13 Kyle Moffett wrote:
> +	return phy_driver_register_multiple(bcm63xx_drivers,
> +			ARRAY_SIZE(bcm63xx_drivers));

if only the arm ARRAY_AND_SIZE() macro could gain traction in the wider tree:
	return phy_driver_register_multiple(ARRAY_AND_SIZE(bcm63xx_drivers));

ignoring that, this patch looks cool

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [RFC PATCH 06/17] phy_driver: Add and use phy_driver_[un]register_multiple()

[Grant Likely]

On Thu, Oct 20, 2011 at 05:15:54PM -0400, Mike Frysinger wrote:
> On Thursday 20 October 2011 17:00:13 Kyle Moffett wrote:
> > +	return phy_driver_register_multiple(bcm63xx_drivers,
> > +			ARRAY_SIZE(bcm63xx_drivers));
> 
> if only the arm ARRAY_AND_SIZE() macro could gain traction in the wider tree:
> 	return phy_driver_register_multiple(ARRAY_AND_SIZE(bcm63xx_drivers));

Propose a patch to move it from arch/arm to include/linux/kernel.h.
This is the first I've heard of this macro, but I would probably use
it.

> ignoring that, this patch looks cool
> 
> Acked-by: Mike Frysinger <vapier@gentoo.org>

Indeed, I like the diffstat.  :-)

Acked-by: Grant Likely <grant.likely@secretlab.ca>

Re: [RFC PATCH 06/17] phy_driver: Add and use phy_driver_[un]register_multiple()

[Mike Frysinger]

On Tue, Oct 25, 2011 at 01:00, Grant Likely wrote:
> On Thu, Oct 20, 2011 at 05:15:54PM -0400, Mike Frysinger wrote:
>> On Thursday 20 October 2011 17:00:13 Kyle Moffett wrote:
>> > +   return phy_driver_register_multiple(bcm63xx_drivers,
>> > +                   ARRAY_SIZE(bcm63xx_drivers));
>>
>> if only the arm ARRAY_AND_SIZE() macro could gain traction in the wider tree:
>>       return phy_driver_register_multiple(ARRAY_AND_SIZE(bcm63xx_drivers));
>
> Propose a patch to move it from arch/arm to include/linux/kernel.h.
> This is the first I've heard of this macro, but I would probably use
> it.

i thought last time rmk suggested it, things got shot down.  but maybe
i remember wrong ... it has been a while since i proposed something
that generated bike shedding, so i'll try again.
-mike

Re: [RFC PATCH 06/17] phy_driver: Add and use phy_driver_[un]register_multiple()

[Andy Fleming]

On Thu, Oct 20, 2011 at 4:00 PM, Kyle Moffett <Kyle.D.Moffett@boeing.com> wrote:
> Several of the PHY drivers are registering dozens of "phy_driver"
> structures at load-time, with different IDs and variations on methods.
> As a result, the error handling is exceptionally ugly.
>
> The "marvell" driver already uses an array of "struct phy_driver" and
> iterates over that to add the drivers, but even then its error handling
> contains a bug (it will not unregister array item 0 on failure).
>
> To resolve these problems, new functions phy_driver_register_multiple()
> and phy_driver_unregister_multiple() are added which take an array and a
> number of drivers and iterate over them internally.
>
> All of the PHY driver files which register more than one "phy_driver"
> are modified to use the new helpers.  This is a sizable net removal of
> about 260 lines of code.
>
> Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>

Let's try that again, sending to the list.

This is excellent!

Very much
Acked-by: Andy Fleming <afleming@freescale.com>

[RFC PATCH 07/17] phy: Unify PHY reset, initialization, and fixup handling

[Kyle Moffett]

Whenever the BMCR_RESET bit is set (in the MII_BMCR register), the
IEEE standards say we MUST poll until it is cleared, which may take up
to 0.5s.  Failing to do so may result in MDIO bus errors or ignored
register writes.  Furthermore, some chips require a little extra time
*after* the bit is cleared before they will work correctly.

This modifies the phy_init_hw() helper function to perform the necessary
full-reset followed by scans of board-specific and driver-specific fixups,
then ensures that phy_init_hw() is called everywhere necessary.

Since some of the fixups must be the first thing run after reset, all
ethernet drivers should call phy_init_hw() to set things up instead of
setting BMCR_RESET by hand.

NOTE: I'm pretty sure the locking is wrong here, it's on my TODO list.

Not-yet-Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---
 Documentation/networking/phy.txt |    5 +-
 drivers/net/phy/phy.c            |   20 ++++--
 drivers/net/phy/phy_device.c     |  132 +++++++++++++++++++++++++++-----------
 include/linux/phy.h              |    1 -
 4 files changed, 111 insertions(+), 47 deletions(-)

diff --git a/Documentation/networking/phy.txt b/Documentation/networking/phy.txt
index 62f890e..0db8c81 100644
--- a/Documentation/networking/phy.txt
+++ b/Documentation/networking/phy.txt
@@ -251,15 +251,16 @@ Writing a PHY driver
 
  Each driver consists of a number of function pointers:
 
+   probe: Allocate phy->priv, optionally refuse to bind.
+     PHY may not have been reset or had fixups run yet.
    config_init: configures PHY into a sane state after a reset.
      For instance, a Davicom PHY requires descrambling disabled.
-   probe: Does any setup needed by the driver
    suspend/resume: power management
    config_aneg: Changes the speed/duplex/negotiation settings
    read_status: Reads the current speed/duplex/negotiation settings
    ack_interrupt: Clear a pending interrupt
    config_intr: Enable or disable interrupts
-   remove: Does any driver take-down
+   remove: Free phy->priv and clean up driver state
 
  All of these functions are optional.  It is strongly preferred to use the
  generic phy driver's versions of these two functions if at all possible:
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index cc7f353..c378f91 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -354,14 +354,20 @@ int phy_mii_ioctl(struct phy_device *phydev,
 		}
 
 		mdiobus_write(phydev->bus, mii_data->phy_id,
-			      mii_data->reg_num, val);
+				mii_data->reg_num, val);
+
+		/*
+		 * If they wanted to reset the PHY, then this needs to also
+		 * wait for the reset to complete. or later MDIO cycles may
+		 * fail with errors.
+		 *
+		 * NOTE: This interface is for debug only, and therefore it
+		 * will NOT rerun board or driver initialization.
+		 */
+		if ((mii_data->reg_num == MII_BMCR) && (val & BMCR_RESET)
+				&& (mii_data->phy_id == phydev->addr))
+			phy_poll_reset(phydev);
 
-		if (mii_data->reg_num == MII_BMCR &&
-		    val & BMCR_RESET &&
-		    phydev->drv->config_init) {
-			phy_scan_fixups(phydev);
-			phydev->drv->config_init(phydev);
-		}
 		break;
 
 	case SIOCSHWTSTAMP:
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8990e87..fc0f315 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -125,30 +125,6 @@ static int phy_needs_fixup(struct phy_device *phydev, struct phy_fixup *fixup)
 	return 1;
 }
 
-/* Runs any matching fixups for this phydev */
-int phy_scan_fixups(struct phy_device *phydev)
-{
-	struct phy_fixup *fixup;
-
-	mutex_lock(&phy_fixup_lock);
-	list_for_each_entry(fixup, &phy_fixup_list, list) {
-		if (phy_needs_fixup(phydev, fixup)) {
-			int err;
-
-			err = fixup->run(phydev);
-
-			if (err < 0) {
-				mutex_unlock(&phy_fixup_lock);
-				return err;
-			}
-		}
-	}
-	mutex_unlock(&phy_fixup_lock);
-
-	return 0;
-}
-EXPORT_SYMBOL(phy_scan_fixups);
-
 static struct phy_device* phy_device_create(struct mii_bus *bus,
 					    int addr, int phy_id)
 {
@@ -274,12 +250,18 @@ int phy_device_register(struct phy_device *phydev)
 		return -EINVAL;
 	phydev->bus->phy_map[phydev->addr] = phydev;
 
-	/* Run all of the fixups for this PHY */
-	phy_scan_fixups(phydev);
+	/* Reset and reinitialize the PHY */
+	err = phy_init_hw(phydev);
+	if (err) {
+		dev_err(phydev->dev.parent, "phy %d failed to init: %d\n",
+				phydev->addr, err);
+		goto out;
+	}
 
 	err = device_register(&phydev->dev);
 	if (err) {
-		pr_err("phy %d failed to register\n", phydev->addr);
+		dev_err(phydev->dev.parent, "phy %d failed to register: %d\n",
+				phydev->addr, err);
 		goto out;
 	}
 
@@ -380,7 +362,7 @@ struct phy_device * phy_connect(struct net_device *dev, const char *bus_id,
 	 * PHY with the requested name */
 	d = bus_find_device_by_name(&mdio_bus_type, NULL, bus_id);
 	if (!d) {
-		pr_err("PHY %s not found\n", bus_id);
+		dev_err(&dev->dev, "PHY %s not found\n", bus_id);
 		return ERR_PTR(-ENODEV);
 	}
 	phydev = to_phy_device(d);
@@ -410,19 +392,93 @@ void phy_disconnect(struct phy_device *phydev)
 }
 EXPORT_SYMBOL(phy_disconnect);
 
-int phy_init_hw(struct phy_device *phydev)
+/**
+ * phy_poll_reset - Safely wait until a PHY reset has properly completed
+ * @phydev: The PHY device to poll
+ *
+ * Description: According to IEEE 802.3, Section 2, Subsection 22.2.4.1.1, as
+ *   published in 2008, a PHY reset may take up to 0.5 seconds.  The MII BMCR
+ *   register must be polled until the BMCR_RESET bit clears.
+ *
+ *   Furthermore, any attempts to write to PHY registers may have no effect
+ *   or even generate MDIO bus errors until this is complete.
+ *
+ *   Some PHYs (such as the Marvell 88E1111) don't entirely conform to the
+ *   standard and do not fully reset after the BMCR_RESET bit is set, and may
+ *   even *REQUIRE* a soft-reset to properly restart autonegotiation.  In an
+ *   effort to support such broken PHYs, this function is separate from the
+ *   standard phy_init_hw() which will zero all the other bits in the BMCR
+ *   and reapply all driver-specific and board-specific fixups.
+ */
+int phy_poll_reset(struct phy_device *phydev)
 {
+	/* Poll until the reset bit clears (50ms per retry == 0.6 sec) */
+	unsigned int retries = 12;
 	int ret;
+	do {
+		msleep(50);
+		ret = phy_read(phydev, MII_BMCR);
+		if (ret < 0)
+			return ret;
+	} while (ret & BMCR_RESET && --retries);
+	if (ret & BMCR_RESET)
+		return -ETIMEDOUT;
+
+	/*
+	 * Some chips (smsc911x) may still need up to another 1ms after the
+	 * BMCR_RESET bit is cleared before they are usable.
+	 */
+	msleep(1);
+	return 0;
+}
 
-	if (!phydev->drv || !phydev->drv->config_init)
-		return 0;
+/**
+ * phy_init_hw - Fully reinitialize a PHY and apply board and driver fixups
+ * @phydev: the PHY device to reset
+ *
+ * Description: Reset the specified PHY using the BMCR_RESET bit, clearing
+ *   all of the other bits in the BMCR.  On standards-compliant PHYs this
+ *   should also restore all other registers to power-on defaults.
+ *
+ *   In addition, this will automatically run any board-specific and
+ *   driver-specific fixups.
+ */
+int phy_init_hw(struct phy_device *phydev)
+{
+	struct phy_fixup *fixup;
 
-	ret = phy_scan_fixups(phydev);
+	/* Always perform a reset first */
+	int ret = phy_write(phydev, MII_BMCR, BMCR_RESET);
 	if (ret < 0)
 		return ret;
 
-	return phydev->drv->config_init(phydev);
+	/* Now wait for the reset to complete */
+	ret = phy_poll_reset(phydev);
+	if (ret < 0)
+		return ret;
+
+	/* Scan board-specific PHY fixups */
+	mutex_lock(&phy_fixup_lock);
+	list_for_each_entry(fixup, &phy_fixup_list, list) {
+		if (!phy_needs_fixup(phydev, fixup))
+			continue;
+
+		ret = fixup->run(phydev);
+		if (ret < 0)
+			break;
+	}
+	mutex_unlock(&phy_fixup_lock);
+	if (ret < 0)
+		return ret;
+
+	/* Now driver-specific initialization */
+	ret = 0;
+	if (phydev->drv && phydev->drv->config_init)
+		ret = phydev->drv->config_init(phydev);
+
+	return ret;
 }
+EXPORT_SYMBOL(phy_init_hw);
 
 /**
  * phy_attach_direct - attach a network device to a given PHY device pointer
@@ -471,9 +527,11 @@ static int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
 
 	phydev->state = PHY_READY;
 
-	/* Do initial configuration here, now that
-	 * we have certain key parameters
-	 * (dev_flags and interface) */
+	/*
+	 * The PHY was already reset and initialize when it was first probed,
+	 * but we just set the dev_flags and interface fields, so the driver
+	 * may need to re-initialize things again to take those into account.
+	 */
 	err = phy_init_hw(phydev);
 	if (err)
 		phy_detach(phydev);
@@ -503,7 +561,7 @@ struct phy_device *phy_attach(struct net_device *dev,
 	 * PHY with the requested name */
 	d = bus_find_device_by_name(bus, NULL, bus_id);
 	if (!d) {
-		pr_err("PHY %s not found\n", bus_id);
+		dev_err(&dev->dev, "PHY %s not found\n", bus_id);
 		return ERR_PTR(-ENODEV);
 	}
 	phydev = to_phy_device(d);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index c6bbb38..f07fc1c 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -526,7 +526,6 @@ int phy_register_fixup_for_id(const char *bus_id,
 		int (*run)(struct phy_device *));
 int phy_register_fixup_for_uid(u32 phy_uid, u32 phy_uid_mask,
 		int (*run)(struct phy_device *));
-int phy_scan_fixups(struct phy_device *phydev);
 
 int __init mdio_bus_init(void);
 void mdio_bus_exit(void);
-- 
1.7.2.5

[RFC PATCH 08/17] drivers/net/bfin_mac: Don't unnecessarily reset the PHY

[Kyle Moffett]

The PHY is already reset during driver probing, and this manual reset
afterwards will wipe out board-specific PHY fixups and driver-specific
phy->drv->config_init() register tweaks.

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---
 drivers/net/bfin_mac.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
index 6c019e1..1427ec3 100644
--- a/drivers/net/bfin_mac.c
+++ b/drivers/net/bfin_mac.c
@@ -1402,7 +1402,6 @@ static int bfin_mac_open(struct net_device *dev)
 		return ret;
 
 	phy_start(lp->phydev);
-	phy_write(lp->phydev, MII_BMCR, BMCR_RESET);
 	setup_system_regs(dev);
 	setup_mac_addr(dev->dev_addr);
 
-- 
1.7.2.5

Re: [RFC PATCH 08/17] drivers/net/bfin_mac: Don't unnecessarily reset the PHY

[Mike Frysinger]

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike

RE: [RFC PATCH 08/17] drivers/net/bfin_mac: Don't unnecessarily reset the PHY

[Zhang, Sonic]

Acked-by: Sonic Zhang <sonic.zhang@analog.com>


Sonic

>-----Original Message-----
>From: Kyle Moffett [mailto:Kyle.D.Moffett@boeing.com]
>Sent: Friday, October 21, 2011 5:00 AM
>To: linux-kernel@vger.kernel.org; netdev@vger.kernel.org
>Cc: Kyle Moffett; David S. Miller; Mike Frysinger; Zhang,
>Sonic; Tobias Klauser; uclinux-dist-devel@blackfin.uclinux.org
>Subject: [RFC PATCH 08/17] drivers/net/bfin_mac: Don't
>unnecessarily reset the PHY
>
>The PHY is already reset during driver probing, and this
>manual reset afterwards will wipe out board-specific PHY
>fixups and driver-specific
>phy->drv->config_init() register tweaks.
>
>Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
>---
> drivers/net/bfin_mac.c |    1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
>
>diff --git a/drivers/net/bfin_mac.c b/drivers/net/bfin_mac.c
>index 6c019e1..1427ec3 100644
>--- a/drivers/net/bfin_mac.c
>+++ b/drivers/net/bfin_mac.c
>@@ -1402,7 +1402,6 @@ static int bfin_mac_open(struct net_device *dev)
>               return ret;
>
>       phy_start(lp->phydev);
>-      phy_write(lp->phydev, MII_BMCR, BMCR_RESET);
>       setup_system_regs(dev);
>       setup_mac_addr(dev->dev_addr);
>
>--
>1.7.2.5
>
>
>

[RFC PATCH 09/17] mv643xx_eth: Use standardized phy_init_hw() for PHY reset

[Kyle Moffett]

The PHY does not need to be reset immediately before phy_attach(), as
that function will call phy_init_hw() properly on its own.  Furthermore,
the PHY should not be manually reset, as that will undo board-specific
PHY fixups and driver-specific phy->drv->config_init() register tweaks.

NOTE: Depends on earlier phy_init_hw() patch.

Not-yet-Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---
 drivers/net/mv643xx_eth.c |   22 +---------------------
 1 files changed, 1 insertions(+), 21 deletions(-)

diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
index 2596999..d3e223c 100644
--- a/drivers/net/mv643xx_eth.c
+++ b/drivers/net/mv643xx_eth.c
@@ -2188,23 +2188,6 @@ static inline void oom_timer_wrapper(unsigned long data)
 	napi_schedule(&mp->napi);
 }
 
-static void phy_reset(struct mv643xx_eth_private *mp)
-{
-	int data;
-
-	data = phy_read(mp->phy, MII_BMCR);
-	if (data < 0)
-		return;
-
-	data |= BMCR_RESET;
-	if (phy_write(mp->phy, MII_BMCR, data) < 0)
-		return;
-
-	do {
-		data = phy_read(mp->phy, MII_BMCR);
-	} while (data >= 0 && data & BMCR_RESET);
-}
-
 static void port_start(struct mv643xx_eth_private *mp)
 {
 	u32 pscr;
@@ -2217,7 +2200,7 @@ static void port_start(struct mv643xx_eth_private *mp)
 		struct ethtool_cmd cmd;
 
 		mv643xx_eth_get_settings(mp->dev, &cmd);
-		phy_reset(mp);
+		phy_init_hw(mp->phy);
 		mv643xx_eth_set_settings(mp->dev, &cmd);
 	}
 
@@ -2779,9 +2762,6 @@ static struct phy_device *phy_scan(struct mv643xx_eth_private *mp,
 static void phy_init(struct mv643xx_eth_private *mp, int speed, int duplex)
 {
 	struct phy_device *phy = mp->phy;
-
-	phy_reset(mp);
-
 	phy_attach(mp->dev, dev_name(&phy->dev), 0, PHY_INTERFACE_MODE_GMII);
 
 	if (speed == 0) {
-- 
1.7.2.5

[RFC PATCH 10/17] pxa186_eth: Use standardized phy_init_hw() for PHY reset

[Kyle Moffett]

The PHY does not need to be reset immediately before phy_attach(), as
that function will call phy_init_hw() properly on its own.  Furthermore,
the PHY should not be manually reset, as that will undo board-specific
PHY fixups and driver-specific phy->drv->config_init() register tweaks.

NOTE: Depends on earlier phy_init_hw() patch.

Not-yet-Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---
 drivers/net/pxa168_eth.c |   21 +--------------------
 1 files changed, 1 insertions(+), 20 deletions(-)

diff --git a/drivers/net/pxa168_eth.c b/drivers/net/pxa168_eth.c
index d17d062..ab3bca9 100644
--- a/drivers/net/pxa168_eth.c
+++ b/drivers/net/pxa168_eth.c
@@ -323,23 +323,6 @@ static void ethernet_phy_set_addr(struct pxa168_eth_private *pep, int phy_addr)
 	wrl(pep, PHY_ADDRESS, reg_data);
 }
 
-static void ethernet_phy_reset(struct pxa168_eth_private *pep)
-{
-	int data;
-
-	data = phy_read(pep->phy, MII_BMCR);
-	if (data < 0)
-		return;
-
-	data |= BMCR_RESET;
-	if (phy_write(pep->phy, MII_BMCR, data) < 0)
-		return;
-
-	do {
-		data = phy_read(pep->phy, MII_BMCR);
-	} while (data >= 0 && data & BMCR_RESET);
-}
-
 static void rxq_refill(struct net_device *dev)
 {
 	struct pxa168_eth_private *pep = netdev_priv(dev);
@@ -647,7 +630,7 @@ static void eth_port_start(struct net_device *dev)
 		struct ethtool_cmd cmd;
 
 		pxa168_get_settings(pep->dev, &cmd);
-		ethernet_phy_reset(pep);
+		phy_init_hw(pep->phy);
 		pxa168_set_settings(pep->dev, &cmd);
 	}
 
@@ -1393,8 +1376,6 @@ static struct phy_device *phy_scan(struct pxa168_eth_private *pep, int phy_addr)
 static void phy_init(struct pxa168_eth_private *pep, int speed, int duplex)
 {
 	struct phy_device *phy = pep->phy;
-	ethernet_phy_reset(pep);
-
 	phy_attach(pep->dev, dev_name(&phy->dev), 0, PHY_INTERFACE_MODE_MII);
 
 	if (speed == 0) {
-- 
1.7.2.5

[RFC PATCH 11/17] sh_eth: Don't unnecessarily reset the PHY

[Kyle Moffett]

The PHY is already reset during driver probing, and this manual reset
afterwards will wipe out board-specific PHY fixups and driver-specific
phy->drv->config_init() register tweaks.

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---
 drivers/net/sh_eth.c |   19 +------------------
 1 files changed, 1 insertions(+), 18 deletions(-)

diff --git a/drivers/net/sh_eth.c b/drivers/net/sh_eth.c
index 1c1666e..7ef4378 100644
--- a/drivers/net/sh_eth.c
+++ b/drivers/net/sh_eth.c
@@ -1235,23 +1235,6 @@ static int sh_eth_phy_init(struct net_device *ndev)
 	return 0;
 }
 
-/* PHY control start function */
-static int sh_eth_phy_start(struct net_device *ndev)
-{
-	struct sh_eth_private *mdp = netdev_priv(ndev);
-	int ret;
-
-	ret = sh_eth_phy_init(ndev);
-	if (ret)
-		return ret;
-
-	/* reset phy - this also wakes it from PDOWN */
-	phy_write(mdp->phydev, MII_BMCR, BMCR_RESET);
-	phy_start(mdp->phydev);
-
-	return 0;
-}
-
 static int sh_eth_get_settings(struct net_device *ndev,
 			struct ethtool_cmd *ecmd)
 {
@@ -1410,7 +1393,7 @@ static int sh_eth_open(struct net_device *ndev)
 		goto out_free_irq;
 
 	/* PHY control start*/
-	ret = sh_eth_phy_start(ndev);
+	ret = sh_eth_phy_init(ndev);
 	if (ret)
 		goto out_free_irq;
 
-- 
1.7.2.5

Re: [RFC PATCH 11/17] sh_eth: Don't unnecessarily reset the PHY

[Yoshihiro Shimoda]

Hi,

2011/10/21 6:00, Kyle Moffett wrote:
> The PHY is already reset during driver probing, and this manual reset
> afterwards will wipe out board-specific PHY fixups and driver-specific
> phy->drv->config_init() register tweaks.

Thank you for your patch. I think so.
However, the patch cannot work correctly on my environment.

> Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
> ---
>  drivers/net/sh_eth.c |   19 +------------------
>  1 files changed, 1 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/sh_eth.c b/drivers/net/sh_eth.c
> index 1c1666e..7ef4378 100644
> --- a/drivers/net/sh_eth.c
> +++ b/drivers/net/sh_eth.c
> @@ -1235,23 +1235,6 @@ static int sh_eth_phy_init(struct net_device *ndev)
>  	return 0;
>  }
>  
> -/* PHY control start function */
> -static int sh_eth_phy_start(struct net_device *ndev)
> -{
> -	struct sh_eth_private *mdp = netdev_priv(ndev);
> -	int ret;
> -
> -	ret = sh_eth_phy_init(ndev);
> -	if (ret)
> -		return ret;
> -
> -	/* reset phy - this also wakes it from PDOWN */
> -	phy_write(mdp->phydev, MII_BMCR, BMCR_RESET);
> -	phy_start(mdp->phydev);

I think that the driver needs the phy_start().
So, I removed the phy_write() only. Then, the driver could work correctly.

Therefore, I would to remove the following code only.
Do you think about this?

> -	/* reset phy - this also wakes it from PDOWN */
> -	phy_write(mdp->phydev, MII_BMCR, BMCR_RESET);

Thanks,
Yoshihiro Shimoda

Re: [RFC PATCH 11/17] sh_eth: Don't unnecessarily reset the PHY

[Moffett, Kyle D]

On Oct 25, 2011, at 07:11, Yoshihiro Shimoda wrote:
> 2011/10/21 6:00, Kyle Moffett wrote:
>> Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
>> ---
>> drivers/net/sh_eth.c |   19 +------------------
>> 1 files changed, 1 insertions(+), 18 deletions(-)
>> 
>> diff --git a/drivers/net/sh_eth.c b/drivers/net/sh_eth.c
>> index 1c1666e..7ef4378 100644
>> --- a/drivers/net/sh_eth.c
>> +++ b/drivers/net/sh_eth.c
>> @@ -1235,23 +1235,6 @@ static int sh_eth_phy_init(struct net_device *ndev)
>> 	return 0;
>> }
>> 
>> -/* PHY control start function */
>> -static int sh_eth_phy_start(struct net_device *ndev)
>> -{
>> -	struct sh_eth_private *mdp = netdev_priv(ndev);
>> -	int ret;
>> -
>> -	ret = sh_eth_phy_init(ndev);
>> -	if (ret)
>> -		return ret;
>> -
>> -	/* reset phy - this also wakes it from PDOWN */
>> -	phy_write(mdp->phydev, MII_BMCR, BMCR_RESET);
>> -	phy_start(mdp->phydev);
> 
> I think that the driver needs the phy_start().
> So, I removed the phy_write() only. Then, the driver could work correctly.
> 
> Therefore, I would to remove the following code only.
> Do you think about this?
> 
>> -	/* reset phy - this also wakes it from PDOWN */
>> -	phy_write(mdp->phydev, MII_BMCR, BMCR_RESET);

You're right, sorry about that.  I think I mis-merged this when I rebased
from a much earlier commit.  The simple removal of the BMCR_RESET write
should achieve what I was intending.

Cheers,
Kyle Moffett

--
Curious about my work on the Debian powerpcspe port?
I'm keeping a blog here: http://pureperl.blogspot.com/

[RFC PATCH 12/17] tc35815: Use standard phy_init_hw() instead of BMCR_RESET bit

[Kyle Moffett]

The PHY should not be manually reset with the BMCR, as that will undo
board-specific PHY fixups and driver-specific phy->drv->config_init().

Instead, the PHY should be reset using phy_init_hw().

NOTE: Depends on earlier phy_init_hw() patch.

Not-yet-Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---
 drivers/net/tc35815.c |   15 ++-------------
 1 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/drivers/net/tc35815.c b/drivers/net/tc35815.c
index 4a55a16..c3422f8 100644
--- a/drivers/net/tc35815.c
+++ b/drivers/net/tc35815.c
@@ -1175,19 +1175,8 @@ static void tc35815_restart(struct net_device *dev)
 {
 	struct tc35815_local *lp = netdev_priv(dev);
 
-	if (lp->phy_dev) {
-		int timeout;
-
-		phy_write(lp->phy_dev, MII_BMCR, BMCR_RESET);
-		timeout = 100;
-		while (--timeout) {
-			if (!(phy_read(lp->phy_dev, MII_BMCR) & BMCR_RESET))
-				break;
-			udelay(1);
-		}
-		if (!timeout)
-			printk(KERN_ERR "%s: BMCR reset failed.\n", dev->name);
-	}
+	if (lp->phy_dev)
+		phy_init_hw(lp->phy_dev);
 
 	spin_lock_bh(&lp->rx_lock);
 	spin_lock_irq(&lp->lock);
-- 
1.7.2.5

[RFC PATCH 13/17] mpc836x: Move board-specific bcm5481 fixup out of the PHY driver

[Kyle Moffett]

By comparing the BCM5481 registers modified in the ->config_aneg()
method with the datasheet I have for the BCM5482, it appears that the
register writes are adjusting signal timing to work around a known
trace-length issue on the PCB.

Such hardware workarounds don't belong in the generic driver, and should
instead be placed in a board-specific PHY fixup.

NOTE: Needs testing by somebody with the hardware.

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---
 arch/powerpc/platforms/83xx/mpc836x_rdk.c |   35 ++++++++++++++++++++++++++++
 drivers/net/phy/broadcom.c                |   36 -----------------------------
 2 files changed, 35 insertions(+), 36 deletions(-)

diff --git a/arch/powerpc/platforms/83xx/mpc836x_rdk.c b/arch/powerpc/platforms/83xx/mpc836x_rdk.c
index b0090aa..b7cc607 100644
--- a/arch/powerpc/platforms/83xx/mpc836x_rdk.c
+++ b/arch/powerpc/platforms/83xx/mpc836x_rdk.c
@@ -14,6 +14,7 @@
 
 #include <linux/kernel.h>
 #include <linux/pci.h>
+#include <linux/phy.h>
 #include <linux/of_platform.h>
 #include <linux/io.h>
 #include <asm/prom.h>
@@ -38,6 +39,36 @@ static int __init mpc836x_rdk_declare_of_platform_devices(void)
 }
 machine_device_initcall(mpc836x_rdk, mpc836x_rdk_declare_of_platform_devices);
 
+static int mpc836x_rdk_bcm5481_fixup(struct phy_device *phydev)
+{
+	int reg;
+
+	if (phydev->interface != PHY_INTERFACE_MODE_RGMII_RXID)
+		return;
+
+	/*
+	 * There is no BCM5481 specification available, so down
+	 * here is everything we know about "register 0x18". This
+	 * at least helps BCM5481 to successfuly receive packets
+	 * on MPC8360E-RDK board. Peter Barada <peterb@logicpd.com>
+	 * says: "This sets delay between the RXD and RXC signals
+	 * instead of using trace lengths to achieve timing".
+	 */
+
+	/* Set RDX clk delay. */
+	reg = 0x7 | (0x7 << 12);
+	phy_write(phydev, 0x18, reg);
+	reg = phy_read(phydev, 0x18);
+	if (reg < 0)
+		return reg;
+
+	/* Set RDX-RXC skew. */
+	reg |= (1 << 8);
+	/* Write bits 14:0. */
+	reg |= (1 << 15);
+	return phy_write(phydev, 0x18, reg);
+}
+
 static void __init mpc836x_rdk_setup_arch(void)
 {
 #ifdef CONFIG_PCI
@@ -54,6 +85,10 @@ static void __init mpc836x_rdk_setup_arch(void)
 #ifdef CONFIG_QUICC_ENGINE
 	qe_reset();
 #endif
+#ifdef CONFIG_BROADCOM_PHY
+	phy_register_fixup_for_uid(0x0143bca0, 0xfffffff0,
+			&mpc836x_rdk_bcm5481_phy_fixup);
+#endif
 }
 
 static void __init mpc836x_rdk_init_IRQ(void)
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 1b83f75..8c03ebc 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -539,41 +539,6 @@ static int bcm54xx_config_intr(struct phy_device *phydev)
 	return err;
 }
 
-static int bcm5481_config_aneg(struct phy_device *phydev)
-{
-	int ret;
-
-	/* Aneg firsly. */
-	ret = genphy_config_aneg(phydev);
-
-	/* Then we can set up the delay. */
-	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
-		u16 reg;
-
-		/*
-		 * There is no BCM5481 specification available, so down
-		 * here is everything we know about "register 0x18". This
-		 * at least helps BCM5481 to successfuly receive packets
-		 * on MPC8360E-RDK board. Peter Barada <peterb@logicpd.com>
-		 * says: "This sets delay between the RXD and RXC signals
-		 * instead of using trace lengths to achieve timing".
-		 */
-
-		/* Set RDX clk delay. */
-		reg = 0x7 | (0x7 << 12);
-		phy_write(phydev, 0x18, reg);
-
-		reg = phy_read(phydev, 0x18);
-		/* Set RDX-RXC skew. */
-		reg |= (1 << 8);
-		/* Write bits 14:0. */
-		reg |= (1 << 15);
-		phy_write(phydev, 0x18, reg);
-	}
-
-	return ret;
-}
-
 static int brcm_phy_setbits(struct phy_device *phydev, int reg, int set)
 {
 	int val;
@@ -736,7 +701,6 @@ static struct phy_driver broadcom_drivers[] = { {
 			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= bcm54xx_config_init,
-	.config_aneg	= bcm5481_config_aneg,
 	.ack_interrupt	= bcm54xx_ack_interrupt,
 	.config_intr	= bcm54xx_config_intr,
 	.driver		= { .owner = THIS_MODULE },
-- 
1.7.2.5

[RFC PATCH 14/17] lxt973: Clean up fixed-mode fiber PHY handling

[Kyle Moffett]

The LXT973 driver does not need to detect fiber/copper PHY modes in the
phy ->probe() method, it can instead check the register directly from
the ->config_aneg() method.

Furthermore, the driver should not manually poke the registers, but
instead adjust the PHY state variables and allow genphy_config_aneg() to
do the rest of the work.

NOTE: Needs testing by somebody with the hardware.

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---
 drivers/net/phy/lxt.c |   33 +++++++++++++--------------------
 1 files changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/net/phy/lxt.c b/drivers/net/phy/lxt.c
index 902d2d1..186dc94 100644
--- a/drivers/net/phy/lxt.c
+++ b/drivers/net/phy/lxt.c
@@ -122,31 +122,25 @@ static int lxt971_config_intr(struct phy_device *phydev)
 	return err;
 }
 
-static int lxt973_probe(struct phy_device *phydev)
+static int lxt973_config_aneg(struct phy_device *phydev)
 {
 	int val = phy_read(phydev, MII_LXT973_PCR);
+	if (val < 0)
+		return val;
 
+	/*
+	 * If the PHY is in fiber-only mode then ignore the ethtool settings
+	 * and force the required 100Base-FX mode.
+	 */
 	if (val & PCR_FIBER_SELECT) {
-		/*
-		 * If fiber is selected, then the only correct setting
-		 * is 100Mbps, full duplex, and auto negotiation off.
-		 */
-		val = phy_read(phydev, MII_BMCR);
-		val |= (BMCR_SPEED100 | BMCR_FULLDPLX);
-		val &= ~BMCR_ANENABLE;
-		phy_write(phydev, MII_BMCR, val);
-		/* Remember that the port is in fiber mode. */
-		phydev->priv = lxt973_probe;
-	} else {
-		phydev->priv = NULL;
+		phydev->supported = ADVERTISED_FIBRE|ADVERTISED_100baseT_Full;
+		phydev->advertising = phydev->supported;
+		phydev->autoneg = AUTONEG_DISABLE;
+		phydev->speed = SPEED_100;
+		phydev->duplex = DUPLEX_FULL;
 	}
-	return 0;
-}
 
-static int lxt973_config_aneg(struct phy_device *phydev)
-{
-	/* Do nothing if port is in fiber mode. */
-	return phydev->priv ? 0 : genphy_config_aneg(phydev);
+	return genphy_config_aneg(phydev);
 }
 
 static struct phy_driver lxt_drivers[] = { {
@@ -174,7 +168,6 @@ static struct phy_driver lxt_drivers[] = { {
 	.phy_id_mask	= 0xfffffff0,
 	.features	= PHY_BASIC_FEATURES,
 	.flags		= 0,
-	.probe		= lxt973_probe,
 	.config_aneg	= lxt973_config_aneg,
 	.driver 	= { .owner = THIS_MODULE,},
 } };
-- 
1.7.2.5

[RFC PATCH 15/17] phy_device: Add "port" and "transciever" fields

[Kyle Moffett]

Some PHYs have multiple software-selectable inputs and outputs,
including RGMII, SGMII, SerDes, etc.  New fields are added to the
"struct phy_device" for "port" and "transciever" to allow "ethtool" to
switch outputs at runtime.  The defaults for the new fields are
identical to the hardcoded values used previously.

This should make no functional changes to the PHY layer behavior, but
it will allow later PHY/ethernet drivers to override those fields.

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---
 drivers/net/phy/phy.c        |    4 ++--
 drivers/net/phy/phy_device.c |    2 ++
 include/linux/phy.h          |    4 ++++
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index c378f91..5f72055 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -290,9 +290,9 @@ int phy_ethtool_gset(struct phy_device *phydev, struct ethtool_cmd *cmd)
 
 	ethtool_cmd_speed_set(cmd, phydev->speed);
 	cmd->duplex = phydev->duplex;
-	cmd->port = PORT_MII;
+	cmd->port = phydev->port;
 	cmd->phy_address = phydev->addr;
-	cmd->transceiver = XCVR_EXTERNAL;
+	cmd->transceiver = phydev->transciever;
 	cmd->autoneg = phydev->autoneg;
 
 	return 0;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index fc0f315..d01b272 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -141,6 +141,8 @@ static struct phy_device* phy_device_create(struct mii_bus *bus,
 
 	dev->speed = 0;
 	dev->duplex = -1;
+	dev->port = PORT_MII;
+	dev->transceiver = XCVR_EXTERNAL;
 	dev->pause = dev->asym_pause = 0;
 	dev->link = 1;
 	dev->interface = PHY_INTERFACE_MODE_GMII;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index f07fc1c..0cb300d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -308,6 +308,10 @@ struct phy_device {
 	u32 supported;
 	u32 advertising;
 
+	/* The current port/xcvr info (Copper, Fibre, MII, Direct-Attach) */
+	u8 port;
+	u8 transceiver;
+
 	int autoneg;
 
 	int link_timeout;
-- 
1.7.2.5

Re: [RFC PATCH 15/17] phy_device: Add "port" and "transciever" fields

[Ben Hutchings]

On Thu, 2011-10-20 at 17:00 -0400, Kyle Moffett wrote:
> Some PHYs have multiple software-selectable inputs and outputs,
> including RGMII, SGMII, SerDes, etc.  New fields are added to the
> "struct phy_device" for "port" and "transciever" to allow "ethtool" to
> switch outputs at runtime.  The defaults for the new fields are
> identical to the hardcoded values used previously.
> 
> This should make no functional changes to the PHY layer behavior, but
> it will allow later PHY/ethernet drivers to override those fields.
> 
> Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
> ---
>  drivers/net/phy/phy.c        |    4 ++--
>  drivers/net/phy/phy_device.c |    2 ++
>  include/linux/phy.h          |    4 ++++
>  3 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index c378f91..5f72055 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -290,9 +290,9 @@ int phy_ethtool_gset(struct phy_device *phydev, struct ethtool_cmd *cmd)
>  
>  	ethtool_cmd_speed_set(cmd, phydev->speed);
>  	cmd->duplex = phydev->duplex;
> -	cmd->port = PORT_MII;
> +	cmd->port = phydev->port;
>  	cmd->phy_address = phydev->addr;
> -	cmd->transceiver = XCVR_EXTERNAL;
> +	cmd->transceiver = phydev->transciever;

'transceiver' is spelt thus.

[...]
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -308,6 +308,10 @@ struct phy_device {
>  	u32 supported;
>  	u32 advertising;
>  
> +	/* The current port/xcvr info (Copper, Fibre, MII, Direct-Attach) */
> +	u8 port;
> +	u8 transceiver;
> +

And yet you got it right here.

Ben.

>  	int autoneg;
>  
>  	int link_timeout;

-- 
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.

Re: [RFC PATCH 15/17] phy_device: Add "port" and "transciever" fields

[Kyle Moffett]

On Thu, Oct 20, 2011 at 19:27, Ben Hutchings <bhutchings@solarflare.com> wrote:
> On Thu, 2011-10-20 at 17:00 -0400, Kyle Moffett wrote:
>> Some PHYs have multiple software-selectable inputs and outputs,
>> including RGMII, SGMII, SerDes, etc.  New fields are added to the
>> "struct phy_device" for "port" and "transciever" to allow "ethtool" to
>> switch outputs at runtime.  The defaults for the new fields are
>> identical to the hardcoded values used previously.
>>
>> This should make no functional changes to the PHY layer behavior, but
>> it will allow later PHY/ethernet drivers to override those fields.
>>
>> Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
>> ---
>>  drivers/net/phy/phy.c        |    4 ++--
>>  drivers/net/phy/phy_device.c |    2 ++
>>  include/linux/phy.h          |    4 ++++
>>  3 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index c378f91..5f72055 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -290,9 +290,9 @@ int phy_ethtool_gset(struct phy_device *phydev, struct ethtool_cmd *cmd)
>>
>>       ethtool_cmd_speed_set(cmd, phydev->speed);
>>       cmd->duplex = phydev->duplex;
>> -     cmd->port = PORT_MII;
>> +     cmd->port = phydev->port;
>>       cmd->phy_address = phydev->addr;
>> -     cmd->transceiver = XCVR_EXTERNAL;
>> +     cmd->transceiver = phydev->transciever;
>
> 'transceiver' is spelt thus.

Oh shoot!

That's what I get for trying to do a last-minute patch edit before
sending the email; thanks for pointing it out!

Cheers,
Kyle Moffett

-- 
Curious about my work on the Debian powerpcspe port?
I'm keeping a blog here: http://pureperl.blogspot.com/

[RFC PATCH 16/17] phy_device: Add phy_setmask helper function

[Kyle Moffett]

In the PHY layer there are frequently occasions when code needs to
modify only some subset of the bits in a particular 16-bit register.

To clean up the flow control and improve readability a phy_setmask()
helper function is added.

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---
 include/linux/phy.h |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 0cb300d..3713c8d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -474,6 +474,26 @@ static inline int phy_write(struct phy_device *phydev, u32 regnum, u16 val)
 	return mdiobus_write(phydev->bus, phydev->addr, regnum, val);
 }
 
+/**
+ * phy_setmask - Convenience function for changing PHY register bits
+ * @phydev: the phy_device struct
+ * @regnum: register number to write
+ * @val: value to write to the @mask bits of @regnum
+ * @mask: the mask of bits to change
+ *
+ * NOTE: MUST NOT be called from interrupt context,
+ * because the bus read/write functions may wait for an interrupt
+ * to conclude the operation.
+ */
+static inline int phy_setmask(struct phy_device *phydev, u16 regnum,
+		u16 val, u16 mask)
+{
+	int ret = phy_read(phydev, regnum);
+	if (ret < 0)
+		return ret;
+	return phy_write(phydev, regnum, (ret & ~mask) | (val & mask));
+}
+
 int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id);
 struct phy_device* get_phy_device(struct mii_bus *bus, int addr);
 int phy_device_register(struct phy_device *phy);
-- 
1.7.2.5

[RFC PATCH 17/17] phy_device: Rename phy_start_aneg() to phy_start_link()

[Kyle Moffett]

The name of the "phy_start_aneg()" function is very confusing, because
it also handles forced-mode (AUTONEG_DISABLE) links.

Rename the function to phy_start_link() and fix up all users.

Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>
---
 Documentation/networking/phy.txt       |    2 +-
 drivers/net/arm/ixp4xx_eth.c           |    2 +-
 drivers/net/dnet.c                     |    2 +-
 drivers/net/greth.c                    |    2 +-
 drivers/net/lantiq_etop.c              |    2 +-
 drivers/net/mv643xx_eth.c              |    2 +-
 drivers/net/octeon/octeon_mgmt.c       |    2 +-
 drivers/net/phy/phy.c                  |   12 ++++++------
 drivers/net/pxa168_eth.c               |    2 +-
 drivers/net/sh_eth.c                   |    2 +-
 drivers/net/smsc911x.c                 |    2 +-
 drivers/net/smsc9420.c                 |    2 +-
 drivers/net/stmmac/stmmac_ethtool.c    |    2 +-
 drivers/net/tg3.c                      |    8 ++++----
 drivers/net/ucc_geth_ethtool.c         |    2 +-
 drivers/staging/octeon/ethernet-mdio.c |    4 ++--
 include/linux/phy.h                    |    2 +-
 net/dsa/slave.c                        |    2 +-
 18 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/Documentation/networking/phy.txt b/Documentation/networking/phy.txt
index 0db8c81..6f862e5 100644
--- a/Documentation/networking/phy.txt
+++ b/Documentation/networking/phy.txt
@@ -190,7 +190,7 @@ Doing it all yourself
    driver if none was found during bus initialization.  Passes in
    any phy-specific flags as needed.
 
- int phy_start_aneg(struct phy_device *phydev);
+ int phy_start_link(struct phy_device *phydev);
    
    Using variables inside the phydev structure, either configures advertising
    and resets autonegotiation, or disables autonegotiation, and configures
diff --git a/drivers/net/arm/ixp4xx_eth.c b/drivers/net/arm/ixp4xx_eth.c
index de51e84..8864be5 100644
--- a/drivers/net/arm/ixp4xx_eth.c
+++ b/drivers/net/arm/ixp4xx_eth.c
@@ -998,7 +998,7 @@ static int ixp4xx_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 static int ixp4xx_nway_reset(struct net_device *dev)
 {
 	struct port *port = netdev_priv(dev);
-	return phy_start_aneg(port->phydev);
+	return phy_start_link(port->phydev);
 }
 
 static const struct ethtool_ops ixp4xx_ethtool_ops = {
diff --git a/drivers/net/dnet.c b/drivers/net/dnet.c
index c1063d1..edb4635 100644
--- a/drivers/net/dnet.c
+++ b/drivers/net/dnet.c
@@ -669,7 +669,7 @@ static int dnet_open(struct net_device *dev)
 	napi_enable(&bp->napi);
 	dnet_init_hw(bp);
 
-	phy_start_aneg(bp->phy_dev);
+	phy_start_link(bp->phy_dev);
 
 	/* schedule a link state check */
 	phy_start(bp->phy_dev);
diff --git a/drivers/net/greth.c b/drivers/net/greth.c
index e7f268f..0ad3f14 100644
--- a/drivers/net/greth.c
+++ b/drivers/net/greth.c
@@ -1363,7 +1363,7 @@ static int greth_mdio_init(struct greth_private *greth)
 
 	/* If Ethernet debug link is used make autoneg happen right away */
 	if (greth->edcl && greth_edcl == 1) {
-		phy_start_aneg(greth->phy);
+		phy_start_link(greth->phy);
 		timeout = jiffies + 6*HZ;
 		while (!phy_aneg_done(greth->phy) && time_before(jiffies, timeout)) {
 		}
diff --git a/drivers/net/lantiq_etop.c b/drivers/net/lantiq_etop.c
index 45f252b..c8795aa 100644
--- a/drivers/net/lantiq_etop.c
+++ b/drivers/net/lantiq_etop.c
@@ -326,7 +326,7 @@ ltq_etop_nway_reset(struct net_device *dev)
 {
 	struct ltq_etop_priv *priv = netdev_priv(dev);
 
-	return phy_start_aneg(priv->phydev);
+	return phy_start_link(priv->phydev);
 }
 
 static const struct ethtool_ops ltq_etop_ethtool_ops = {
diff --git a/drivers/net/mv643xx_eth.c b/drivers/net/mv643xx_eth.c
index d3e223c..0ce514c 100644
--- a/drivers/net/mv643xx_eth.c
+++ b/drivers/net/mv643xx_eth.c
@@ -2775,7 +2775,7 @@ static void phy_init(struct mv643xx_eth_private *mp, int speed, int duplex)
 		phy->speed = speed;
 		phy->duplex = duplex;
 	}
-	phy_start_aneg(phy);
+	phy_start_link(phy);
 }
 
 static void init_pscr(struct mv643xx_eth_private *mp, int speed, int duplex)
diff --git a/drivers/net/octeon/octeon_mgmt.c b/drivers/net/octeon/octeon_mgmt.c
index 429e08c..fcdf28a 100644
--- a/drivers/net/octeon/octeon_mgmt.c
+++ b/drivers/net/octeon/octeon_mgmt.c
@@ -686,7 +686,7 @@ static int octeon_mgmt_init_phy(struct net_device *netdev)
 		return -1;
 	}
 
-	phy_start_aneg(p->phydev);
+	phy_start_link(p->phydev);
 
 	return 0;
 }
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 5f72055..fc486fe 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -232,7 +232,7 @@ static void phy_sanitize_settings(struct phy_device *phydev)
  * A few notes about parameter checking:
  * - We don't set port or transceiver, so we don't care what they
  *   were set to.
- * - phy_start_aneg() will make sure forced settings are sane, and
+ * - phy_start_link() will make sure forced settings are sane, and
  *   choose the next best ones from the ones selected, so we don't
  *   care if ethtool tries to give us bad values.
  */
@@ -276,7 +276,7 @@ int phy_ethtool_sset(struct phy_device *phydev, struct ethtool_cmd *cmd)
 	phydev->duplex = cmd->duplex;
 
 	/* Restart the PHY */
-	phy_start_aneg(phydev);
+	phy_start_link(phydev);
 
 	return 0;
 }
@@ -384,7 +384,7 @@ int phy_mii_ioctl(struct phy_device *phydev,
 EXPORT_SYMBOL(phy_mii_ioctl);
 
 /**
- * phy_start_aneg - start auto-negotiation for this PHY device
+ * phy_start_link - start auto-negotiation for this PHY device
  * @phydev: the phy_device struct
  *
  * Description: Sanitizes the settings (if we're not autonegotiating
@@ -392,7 +392,7 @@ EXPORT_SYMBOL(phy_mii_ioctl);
  *   If the PHYCONTROL Layer is operating, we change the state to
  *   reflect the beginning of Auto-negotiation or forcing.
  */
-int phy_start_aneg(struct phy_device *phydev)
+int phy_start_link(struct phy_device *phydev)
 {
 	int err;
 
@@ -423,7 +423,7 @@ out_unlock:
 	mutex_unlock(&phydev->lock);
 	return err;
 }
-EXPORT_SYMBOL(phy_start_aneg);
+EXPORT_SYMBOL(phy_start_link);
 
 
 static void phy_change(struct work_struct *work);
@@ -970,7 +970,7 @@ void phy_state_machine(struct work_struct *work)
 	mutex_unlock(&phydev->lock);
 
 	if (needs_aneg)
-		err = phy_start_aneg(phydev);
+		err = phy_start_link(phydev);
 
 	if (err < 0)
 		phy_error(phydev);
diff --git a/drivers/net/pxa168_eth.c b/drivers/net/pxa168_eth.c
index ab3bca9..8f3d751 100644
--- a/drivers/net/pxa168_eth.c
+++ b/drivers/net/pxa168_eth.c
@@ -1390,7 +1390,7 @@ static void phy_init(struct pxa168_eth_private *pep, int speed, int duplex)
 		phy->speed = speed;
 		phy->duplex = duplex;
 	}
-	phy_start_aneg(phy);
+	phy_start_link(phy);
 }
 
 static int ethernet_phy_setup(struct net_device *dev)
diff --git a/drivers/net/sh_eth.c b/drivers/net/sh_eth.c
index 7ef4378..0927b1b 100644
--- a/drivers/net/sh_eth.c
+++ b/drivers/net/sh_eth.c
@@ -1291,7 +1291,7 @@ static int sh_eth_nway_reset(struct net_device *ndev)
 	int ret;
 
 	spin_lock_irqsave(&mdp->lock, flags);
-	ret = phy_start_aneg(mdp->phydev);
+	ret = phy_start_link(mdp->phydev);
 	spin_unlock_irqrestore(&mdp->lock, flags);
 
 	return ret;
diff --git a/drivers/net/smsc911x.c b/drivers/net/smsc911x.c
index b9016a3..0b5bb15 100644
--- a/drivers/net/smsc911x.c
+++ b/drivers/net/smsc911x.c
@@ -1712,7 +1712,7 @@ static int smsc911x_ethtool_nwayreset(struct net_device *dev)
 {
 	struct smsc911x_data *pdata = netdev_priv(dev);
 
-	return phy_start_aneg(pdata->phy_dev);
+	return phy_start_link(pdata->phy_dev);
 }
 
 static u32 smsc911x_ethtool_getmsglevel(struct net_device *dev)
diff --git a/drivers/net/smsc9420.c b/drivers/net/smsc9420.c
index 459726f..fb9e160 100644
--- a/drivers/net/smsc9420.c
+++ b/drivers/net/smsc9420.c
@@ -302,7 +302,7 @@ static int smsc9420_ethtool_nway_reset(struct net_device *netdev)
 	if (!pd->phy_dev)
 		return -ENODEV;
 
-	return phy_start_aneg(pd->phy_dev);
+	return phy_start_link(pd->phy_dev);
 }
 
 static int smsc9420_ethtool_getregslen(struct net_device *dev)
diff --git a/drivers/net/stmmac/stmmac_ethtool.c b/drivers/net/stmmac/stmmac_ethtool.c
index 7ed8fb6..68e107b 100644
--- a/drivers/net/stmmac/stmmac_ethtool.c
+++ b/drivers/net/stmmac/stmmac_ethtool.c
@@ -240,7 +240,7 @@ stmmac_set_pauseparam(struct net_device *netdev,
 
 	if (phy->autoneg) {
 		if (netif_running(netdev))
-			ret = phy_start_aneg(phy);
+			ret = phy_start_link(phy);
 	} else
 		priv->hw->mac->flow_ctrl(priv->ioaddr, phy->duplex,
 					 priv->flow_ctrl, priv->pause);
diff --git a/drivers/net/tg3.c b/drivers/net/tg3.c
index 4a1374d..a317d11 100644
--- a/drivers/net/tg3.c
+++ b/drivers/net/tg3.c
@@ -1661,7 +1661,7 @@ static void tg3_phy_start(struct tg3 *tp)
 
 	phy_start(phydev);
 
-	phy_start_aneg(phydev);
+	phy_start_link(phydev);
 }
 
 static void tg3_phy_stop(struct tg3 *tp)
@@ -2848,7 +2848,7 @@ static int tg3_power_down_prepare(struct tg3 *tp)
 
 			phydev->advertising = advertising;
 
-			phy_start_aneg(phydev);
+			phy_start_link(phydev);
 
 			phyid = phydev->drv->phy_id & phydev->drv->phy_id_mask;
 			if (phyid != PHY_ID_BCMAC131) {
@@ -10340,7 +10340,7 @@ static int tg3_nway_reset(struct net_device *dev)
 	if (tg3_flag(tp, USE_PHYLIB)) {
 		if (!(tp->phy_flags & TG3_PHYFLG_IS_CONNECTED))
 			return -EAGAIN;
-		r = phy_start_aneg(tp->mdio_bus->phy_map[TG3_PHY_MII_ADDR]);
+		r = phy_start_link(tp->mdio_bus->phy_map[TG3_PHY_MII_ADDR]);
 	} else {
 		u32 bmcr;
 
@@ -10500,7 +10500,7 @@ static int tg3_set_pauseparam(struct net_device *dev, struct ethtool_pauseparam
 					 * tg3_adjust_link() do the final
 					 * flow control setup.
 					 */
-					return phy_start_aneg(phydev);
+					return phy_start_link(phydev);
 				}
 			}
 
diff --git a/drivers/net/ucc_geth_ethtool.c b/drivers/net/ucc_geth_ethtool.c
index a97257f..68a8743 100644
--- a/drivers/net/ucc_geth_ethtool.c
+++ b/drivers/net/ucc_geth_ethtool.c
@@ -342,7 +342,7 @@ static int uec_nway_reset(struct net_device *netdev)
 {
 	struct ucc_geth_private *ugeth = netdev_priv(netdev);
 
-	return phy_start_aneg(ugeth->phydev);
+	return phy_start_link(ugeth->phydev);
 }
 
 /* Report driver information */
diff --git a/drivers/staging/octeon/ethernet-mdio.c b/drivers/staging/octeon/ethernet-mdio.c
index f18e3e1..dc152dd 100644
--- a/drivers/staging/octeon/ethernet-mdio.c
+++ b/drivers/staging/octeon/ethernet-mdio.c
@@ -81,7 +81,7 @@ static int cvm_oct_nway_reset(struct net_device *dev)
 		return -EPERM;
 
 	if (priv->phydev)
-		return phy_start_aneg(priv->phydev);
+		return phy_start_link(priv->phydev);
 
 	return -EINVAL;
 }
@@ -176,7 +176,7 @@ int cvm_oct_phy_setup_device(struct net_device *dev)
 			return -1;
 		}
 		priv->last_link = 0;
-		phy_start_aneg(priv->phydev);
+		phy_start_link(priv->phydev);
 	}
 	return 0;
 }
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 3713c8d..45f6ee8 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -511,7 +511,7 @@ void phy_disconnect(struct phy_device *phydev);
 void phy_detach(struct phy_device *phydev);
 void phy_start(struct phy_device *phydev);
 void phy_stop(struct phy_device *phydev);
-int phy_start_aneg(struct phy_device *phydev);
+int phy_start_link(struct phy_device *phydev);
 
 int phy_stop_interrupts(struct phy_device *phydev);
 
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 0a47b6c..90ff96d 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -400,7 +400,7 @@ dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 		p->phy->speed = 0;
 		p->phy->duplex = 0;
 		p->phy->advertising = p->phy->supported | ADVERTISED_Autoneg;
-		phy_start_aneg(p->phy);
+		phy_start_link(p->phy);
 	}
 
 	return slave_dev;
-- 
1.7.2.5

Re: [RFC PATCH 17/17] phy_device: Rename phy_start_aneg() to phy_start_link()

[Andy Fleming]

2011/10/20 Kyle Moffett <Kyle.D.Moffett@boeing.com>:
> The name of the "phy_start_aneg()" function is very confusing, because
> it also handles forced-mode (AUTONEG_DISABLE) links.
>
> Rename the function to phy_start_link() and fix up all users.
>
> Signed-off-by: Kyle Moffett <Kyle.D.Moffett@boeing.com>

Sounds good to me.

Acked-by: Andy Fleming <afleming@freescale.com>