[U-Boot-Users] [PATCH] PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

[U-Boot-Users] [PATCH] PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

[Larry Johnson]

This patch adds a new switch: "CONFIG_PHY_DYNAMIC_ANEG".  When this symbol
is defined, the PHY will advertise it's capabilities for autonegotiation
based on the capabilities shown in the PHY's status registers, including
1000BASE-X.  When "CONFIG_PHY_DYNAMIC_ANEG" is not defined, the PHY will
advertise hard-coded capabilities, as before.

Signed-off-by: Larry Johnson <lrj@acm.org>
---
diff --git a/common/miiphyutil.c b/common/miiphyutil.c
index c69501f..39e3c20 100644
--- a/common/miiphyutil.c
+++ b/common/miiphyutil.c
@@ -36,7 +36,6 @@
 #include <net.h>

 /* local debug macro */
-#define MII_DEBUG
 #undef MII_DEBUG

 #undef debug
@@ -49,10 +48,10 @@
 struct mii_dev {
 	struct list_head link;
 	char *name;
-	int (* read)(char *devname, unsigned char addr,
-			unsigned char reg, unsigned short *value);
-	int (* write)(char *devname, unsigned char addr,
-			unsigned char reg, unsigned short value);
+	int (*read) (char *devname, unsigned char addr,
+		     unsigned char reg, unsigned short *value);
+	int (*write) (char *devname, unsigned char addr,
+		      unsigned char reg, unsigned short value);
 };

 static struct list_head mii_devs;
@@ -62,21 +61,21 @@ static struct mii_dev *current_mii;
  *
  * Initialize global data. Need to be called before any other miiphy routine.
  */
-void miiphy_init()
+void miiphy_init ()
 {
-		INIT_LIST_HEAD(&mii_devs);
-		current_mii = NULL;
+	INIT_LIST_HEAD (&mii_devs);
+	current_mii = NULL;
 }

 /*****************************************************************************
  *
  * Register read and write MII access routines for the device <name>.
  */
-void miiphy_register(char *name,
-		int (* read)(char *devname, unsigned char addr,
-			unsigned char reg, unsigned short *value),
-		int (* write)(char *devname, unsigned char addr,
-			unsigned char reg, unsigned short value))
+void miiphy_register (char *name,
+		      int (*read) (char *devname, unsigned char addr,
+				   unsigned char reg, unsigned short *value),
+		      int (*write) (char *devname, unsigned char addr,
+				    unsigned char reg, unsigned short value))
 {
 	struct list_head *entry;
 	struct mii_dev *new_dev;
@@ -84,63 +83,64 @@ void miiphy_register(char *name,
 	unsigned int name_len;

 	/* check if we have unique name */
-	list_for_each(entry, &mii_devs) {
-		miidev = list_entry(entry, struct mii_dev, link);
-		if (strcmp(miidev->name, name) == 0) {
-			printf("miiphy_register: non unique device name '%s'\n",
-					name);
+	list_for_each (entry, &mii_devs) {
+		miidev = list_entry (entry, struct mii_dev, link);
+		if (strcmp (miidev->name, name) == 0) {
+			printf ("miiphy_register: non unique device name "
+				"'%s'\n", name);
 			return;
 		}
 	}

 	/* allocate memory */
-	name_len = strlen(name);
-	new_dev = (struct mii_dev *)malloc(sizeof(struct mii_dev) + name_len + 1);
+	name_len = strlen (name);
+	new_dev =
+	    (struct mii_dev *)malloc (sizeof (struct mii_dev) + name_len + 1);

-	if(new_dev == NULL) {
-		printf("miiphy_register: cannot allocate memory for '%s'\n",
-				name);
+	if (new_dev == NULL) {
+		printf ("miiphy_register: cannot allocate memory for '%s'\n",
+			name);
 		return;
 	}
-	memset(new_dev, 0, sizeof(struct mii_dev) + name_len);
+	memset (new_dev, 0, sizeof (struct mii_dev) + name_len);

 	/* initalize mii_dev struct fields */
-	INIT_LIST_HEAD(&new_dev->link);
+	INIT_LIST_HEAD (&new_dev->link);
 	new_dev->read = read;
 	new_dev->write = write;
 	new_dev->name = (char *)(new_dev + 1);
-	strncpy(new_dev->name, name, name_len);
+	strncpy (new_dev->name, name, name_len);
 	new_dev->name[name_len] = '\0';

-	debug("miiphy_register: added '%s', read=0x%08lx, write=0x%08lx\n",
-			new_dev->name, new_dev->read, new_dev->write);
+	debug ("miiphy_register: added '%s', read=0x%08lx, write=0x%08lx\n",
+	       new_dev->name, new_dev->read, new_dev->write);

 	/* add it to the list */
-	list_add_tail(&new_dev->link, &mii_devs);
+	list_add_tail (&new_dev->link, &mii_devs);

 	if (!current_mii)
 		current_mii = new_dev;
 }

-int miiphy_set_current_dev(char *devname)
+int miiphy_set_current_dev (char *devname)
 {
 	struct list_head *entry;
 	struct mii_dev *dev;

-	list_for_each(entry, &mii_devs) {
-		dev = list_entry(entry, struct mii_dev, link);
+	list_for_each (entry, &mii_devs) {
+		dev = list_entry (entry, struct mii_dev, link);

-		if (strcmp(devname, dev->name) == 0) {
+		if (strcmp (devname, dev->name) == 0) {
 			current_mii = dev;
 			return 0;
 		}
 	}

-	printf("No such device: %s\n", devname);
+	printf ("No such device: %s\n", devname);
 	return 1;
 }

-char *miiphy_get_current_dev()
+char *miiphy_get_current_dev ()
 {
 	if (current_mii)
 		return current_mii->name;
@@ -156,8 +156,8 @@ char *miiphy_get_current_dev()
  * Returns:
  *   0 on success
  */
-int miiphy_read(char *devname, unsigned char addr, unsigned char reg,
-		unsigned short *value)
+int miiphy_read (char *devname, unsigned char addr, unsigned char reg,
+		 unsigned short *value)
 {
 	struct list_head *entry;
 	struct mii_dev *dev;
@@ -165,22 +165,22 @@ int miiphy_read(char *devname, unsigned char addr, unsigned char reg,
 	int read_ret = 0;

 	if (!devname) {
-		printf("NULL device name!\n");
+		printf ("NULL device name!\n");
 		return 1;
 	}

-	list_for_each(entry, &mii_devs) {
-		dev = list_entry(entry, struct mii_dev, link);
+	list_for_each (entry, &mii_devs) {
+		dev = list_entry (entry, struct mii_dev, link);

-		if (strcmp(devname, dev->name) == 0) {
+		if (strcmp (devname, dev->name) == 0) {
 			found_dev = 1;
-			read_ret = dev->read(devname, addr, reg, value);
+			read_ret = dev->read (devname, addr, reg, value);
 			break;
 		}
 	}

 	if (found_dev == 0)
-		printf("No such device: %s\n", devname);
+		printf ("No such device: %s\n", devname);

 	return ((found_dev) ? read_ret : 1);
 }
@@ -193,8 +193,8 @@ int miiphy_read(char *devname, unsigned char addr, unsigned char reg,
  * Returns:
  *   0 on success
  */
-int miiphy_write(char *devname, unsigned char addr, unsigned char reg,
-		unsigned short value)
+int miiphy_write (char *devname, unsigned char addr, unsigned char reg,
+		  unsigned short value)
 {
 	struct list_head *entry;
 	struct mii_dev *dev;
@@ -202,22 +202,22 @@ int miiphy_write(char *devname, unsigned char addr, unsigned char reg,
 	int write_ret = 0;

 	if (!devname) {
-		printf("NULL device name!\n");
+		printf ("NULL device name!\n");
 		return 1;
 	}

-	list_for_each(entry, &mii_devs) {
-		dev = list_entry(entry, struct mii_dev, link);
+	list_for_each (entry, &mii_devs) {
+		dev = list_entry (entry, struct mii_dev, link);

-		if (strcmp(devname, dev->name) == 0) {
+		if (strcmp (devname, dev->name) == 0) {
 			found_dev = 1;
-			write_ret = dev->write(devname, addr, reg, value);
+			write_ret = dev->write (devname, addr, reg, value);
 			break;
 		}
 	}

 	if (found_dev == 0)
-		printf("No such device: %s\n", devname);
+		printf ("No such device: %s\n", devname);

 	return ((found_dev) ? write_ret : 1);
 }
@@ -226,23 +226,22 @@ int miiphy_write(char *devname, unsigned char addr, unsigned char reg,
  *
  * Print out list of registered MII capable devices.
  */
-void miiphy_listdev(void)
+void miiphy_listdev (void)
 {
 	struct list_head *entry;
 	struct mii_dev *dev;

-	puts("MII devices: ");
-	list_for_each(entry, &mii_devs) {
-		dev = list_entry(entry, struct mii_dev, link);
-		printf("'%s' ", dev->name);
+	puts ("MII devices: ");
+	list_for_each (entry, &mii_devs) {
+		dev = list_entry (entry, struct mii_dev, link);
+		printf ("'%s' ", dev->name);
 	}
-	puts("\n");
+	puts ("\n");

 	if (current_mii)
-		printf("Current device: '%s'\n", current_mii->name);
+		printf ("Current device: '%s'\n", current_mii->name);
 }

-
 /*****************************************************************************
  *
  * Read the OUI, manufacture's model number, and revision number.
@@ -254,9 +253,7 @@ void miiphy_listdev(void)
  * Returns:
  *   0 on success
  */
-int miiphy_info (char *devname,
-		 unsigned char addr,
-		 unsigned int *oui,
+int miiphy_info (char *devname, unsigned char addr, unsigned int *oui,
 		 unsigned char *model, unsigned char *rev)
 {
 	unsigned int reg = 0;
@@ -288,13 +285,12 @@ int miiphy_info (char *devname,
 #ifdef DEBUG
 	printf ("PHY_PHYIDR[1,2] @ 0x%x = 0x%08x\n", addr, reg);
 #endif
-	*oui   =                 ( reg >> 10);
-	*model = (unsigned char) ((reg >>  4) & 0x0000003F);
-	*rev   = (unsigned char) ( reg        & 0x0000000F);
+	*oui = (reg >> 10);
+	*model = (unsigned char)((reg >> 4) & 0x0000003F);
+	*rev = (unsigned char)(reg & 0x0000000F);
 	return (0);
 }

-
 /*****************************************************************************
  *
  * Reset the PHY.
@@ -345,104 +341,150 @@ int miiphy_reset (char *devname, unsigned char addr)
 	return (0);
 }

-
 /*****************************************************************************
  *
- * Determine the ethernet speed (10/100).
+ * Determine the ethernet speed (10/100/1000).  Return 10 on error.
  */
 int miiphy_speed (char *devname, unsigned char addr)
 {
-	unsigned short reg;
+	u16 bmcr;

 #if defined(CONFIG_PHY_GIGE)
-	if (miiphy_read (devname, addr, PHY_1000BTSR, &reg)) {
-		printf ("PHY 1000BT Status read failed\n");
-	} else {
-		if (reg != 0xFFFF) {
-			if ((reg & (PHY_1000BTSR_1000FD | PHY_1000BTSR_1000HD)) !=0) {
-				return (_1000BASET);
-			}
+	u16 btsr;
+
+#if defined(CONFIG_PHY_DYNAMIC_ANEG)
+	u16 bmsr;
+
+	/* Check for 1000BASE-X. */
+	if (miiphy_read (devname, addr, PHY_BMSR, &bmsr)) {
+		printf ("PHY status");
+		goto miiphy_read_failed;
+	}
+	if (bmsr & PHY_BMSR_EXT_STAT) {
+		u16 exsr;
+
+		if (miiphy_read (devname, addr, PHY_EXSR, &exsr)) {
+			printf ("PHY extended status");
+			goto miiphy_read_failed;
+		}
+		if (exsr & (PHY_EXSR_1000XF | PHY_EXSR_1000XH)) {
+			/* 1000BASE-X */
+			return _1000BASET;
 		}
 	}
+#endif /*  defined(CONFIG_PHY_DYNAMIC_ANEG) */
+
+	/* Check for 1000BASE-T. */
+	if (miiphy_read (devname, addr, PHY_1000BTSR, &btsr)) {
+		printf ("PHY 1000BT status");
+		goto miiphy_read_failed;
+	}
+	if (btsr != 0xFFFF &&
+	    (btsr & (PHY_1000BTSR_1000FD | PHY_1000BTSR_1000HD))) {
+		return _1000BASET;
+	}
 #endif /* CONFIG_PHY_GIGE */

 	/* Check Basic Management Control Register first. */
-	if (miiphy_read (devname, addr, PHY_BMCR, &reg)) {
-		puts ("PHY speed read failed, assuming 10bT\n");
-		return (_10BASET);
+	if (miiphy_read (devname, addr, PHY_BMCR, &bmcr)) {
+		printf ("PHY speed");
+		goto miiphy_read_failed;
 	}
 	/* Check if auto-negotiation is on. */
-	if ((reg & PHY_BMCR_AUTON) != 0) {
+	if (bmcr & PHY_BMCR_AUTON) {
 		/* Get auto-negotiation results. */
-		if (miiphy_read (devname, addr, PHY_ANLPAR, &reg)) {
-			puts ("PHY AN speed read failed, assuming 10bT\n");
-			return (_10BASET);
-		}
-		if ((reg & PHY_ANLPAR_100) != 0) {
-			return (_100BASET);
-		} else {
-			return (_10BASET);
+		u16 anlpar;
+
+		if (miiphy_read (devname, addr, PHY_ANLPAR, &anlpar)) {
+			printf ("PHY AN speed");
+			goto miiphy_read_failed;
 		}
+		return (anlpar & PHY_ANLPAR_100) ? _100BASET : _10BASET;
 	}
 	/* Get speed from basic control settings. */
-	else if (reg & PHY_BMCR_100MB) {
-		return (_100BASET);
-	} else {
-		return (_10BASET);
-	}
+	return (bmcr & PHY_BMCR_100MB) ? _100BASET : _10BASET;

+      miiphy_read_failed:
+	printf (" read failed, assuming 10BASE-T\n");
+	return _10BASET;
 }

-
 /*****************************************************************************
  *
- * Determine full/half duplex.
+ * Determine full/half duplex.  Return half on error.
  */
 int miiphy_duplex (char *devname, unsigned char addr)
 {
-	unsigned short reg;
+	u16 bmcr;

 #if defined(CONFIG_PHY_GIGE)
-	if (miiphy_read (devname, addr, PHY_1000BTSR, &reg)) {
-		printf ("PHY 1000BT Status read failed\n");
-	} else {
-		if ( (reg != 0xFFFF) &&
-		     (reg & (PHY_1000BTSR_1000FD | PHY_1000BTSR_1000HD)) ) {
-			if ((reg & PHY_1000BTSR_1000FD) !=0) {
-				return (FULL);
-			} else {
-				return (HALF);
+	u16 btsr;
+
+#if defined(CONFIG_PHY_DYNAMIC_ANEG)
+	u16 bmsr;
+
+	/* Check for 1000BASE-X. */
+	if (miiphy_read (devname, addr, PHY_BMSR, &bmsr)) {
+		printf ("PHY status");
+		goto miiphy_read_failed;
+	}
+	if (bmsr & PHY_BMSR_EXT_STAT) {
+		u16 exsr;
+
+		if (miiphy_read (devname, addr, PHY_EXSR, &exsr)) {
+			printf ("PHY extended status");
+			goto miiphy_read_failed;
+		}
+		if (exsr & (PHY_EXSR_1000XF | PHY_EXSR_1000XH)) {
+			/* 1000BASE-X */
+			u16 anlpar;
+
+			if (miiphy_read (devname, addr, PHY_ANLPAR, &anlpar)) {
+				printf ("1000BASE-X PHY AN duplex");
+				goto miiphy_read_failed;
 			}
+			return (anlpar & PHY_X_ANLPAR_FD) ? FULL : HALF;
+		}
+	}
+#endif /* defined(CONFIG_PHY_DYNAMIC_ANEG) */
+
+	/* Check for 1000BASE-T. */
+	if (miiphy_read (devname, addr, PHY_1000BTSR, &btsr)) {
+		printf ("PHY 1000BT status");
+		goto miiphy_read_failed;
+	}
+	if (btsr != 0xFFFF) {
+		if (btsr & PHY_1000BTSR_1000FD) {
+			return FULL;
+		} else if (btsr & PHY_1000BTSR_1000HD) {
+			return HALF;
 		}
 	}
 #endif /* CONFIG_PHY_GIGE */

 	/* Check Basic Management Control Register first. */
-	if (miiphy_read (devname, addr, PHY_BMCR, &reg)) {
-		puts ("PHY duplex read failed, assuming half duplex\n");
-		return (HALF);
+	if (miiphy_read (devname, addr, PHY_BMCR, &bmcr)) {
+		puts ("PHY duplex");
+		goto miiphy_read_failed;
 	}
 	/* Check if auto-negotiation is on. */
-	if ((reg & PHY_BMCR_AUTON) != 0) {
+	if ((bmcr & PHY_BMCR_AUTON) != 0) {
 		/* Get auto-negotiation results. */
-		if (miiphy_read (devname, addr, PHY_ANLPAR, &reg)) {
-			puts ("PHY AN duplex read failed, assuming half duplex\n");
-			return (HALF);
-		}
+		u16 anlpar;

-		if ((reg & (PHY_ANLPAR_10FD | PHY_ANLPAR_TXFD)) != 0) {
-			return (FULL);
-		} else {
-			return (HALF);
+		if (miiphy_read (devname, addr, PHY_ANLPAR, &anlpar)) {
+			puts ("PHY AN duplex");
+			goto miiphy_read_failed;
 		}
+		return (anlpar & (PHY_ANLPAR_10FD | PHY_ANLPAR_TXFD)) ?
+		    FULL : HALF;
 	}
 	/* Get speed from basic control settings. */
-	else if (reg & PHY_BMCR_DPLX) {
-		return (FULL);
-	} else {
-		return (HALF);
-	}
+	return (bmcr & PHY_BMCR_DPLX) ? FULL : HALF;

+      miiphy_read_failed:
+	printf (" read failed, assuming half duplex\n");
+	return HALF;
 }

 #ifdef CFG_FAULT_ECHO_LINK_DOWN
@@ -455,7 +497,7 @@ int miiphy_link (char *devname, unsigned char addr)
 	unsigned short reg;

 	/* dummy read; needed to latch some phys */
-	(void)miiphy_read(devname, addr, PHY_BMSR, &reg);
+	(void)miiphy_read (devname, addr, PHY_BMSR, &reg);
 	if (miiphy_read (devname, addr, PHY_BMSR, &reg)) {
 		puts ("PHY_BMSR read failed, assuming no link\n");
 		return (0);
@@ -469,5 +511,4 @@ int miiphy_link (char *devname, unsigned char addr)
 	}
 }
 #endif
-
 #endif /* CONFIG_MII */
diff --git a/cpu/ppc4xx/miiphy.c b/cpu/ppc4xx/miiphy.c
index 6b98025..55d777f 100644
--- a/cpu/ppc4xx/miiphy.c
+++ b/cpu/ppc4xx/miiphy.c
@@ -39,6 +39,9 @@
   |  19-Jul-00	 Ported to esd cpci405					     sr
   |  23-Dec-03	 Ported from miiphy.c to 440GX Travis Sawyer		     TBS
   |		 <travis.sawyer@sandburst.com>
+  |  20-Oct-07	 Added support for autonegotiation base on reported	     lrj
+  |		 capabilities, including 1000BASE-X.  Larry Johnson
+  |		 <lrj@acm.org>
   |
   +-----------------------------------------------------------------------------*/

@@ -60,7 +63,6 @@ void miiphy_dump (char *devname, unsigned char addr)
 	unsigned long i;
 	unsigned short data;

-
 	for (i = 0; i < 0x1A; i++) {
 		if (miiphy_read (devname, addr, i, &data)) {
 			printf ("read error for reg %lx\n", i);
@@ -75,15 +77,87 @@ void miiphy_dump (char *devname, unsigned char addr)
 	}			/* end for loop */
 }				/* end dump */

-
 /***********************************************************/
 /* (Re)start autonegotiation				   */
 /***********************************************************/
 int phy_setup_aneg (char *devname, unsigned char addr)
 {
-	unsigned short ctl, adv;
+	u16 bmcr;
+
+#if defined(CONFIG_PHY_DYNAMIC_ANEG)
+	/*
+	 * Set up advertisement based on capablilities reported by the PHY.
+	 * This should work for both copper and fiber.
+	 */
+	u16 bmsr;
+#if defined(CONFIG_PHY_GIGE)
+	u16 exsr = 0x0000;
+#endif
+
+	miiphy_read (devname, addr, PHY_BMSR, &bmsr);
+
+#if defined(CONFIG_PHY_GIGE)
+	if (bmsr & PHY_BMSR_EXT_STAT) {
+		miiphy_read (devname, addr, PHY_EXSR, &exsr);
+	}
+
+	if (exsr & (PHY_EXSR_1000XF | PHY_EXSR_1000XH)) {
+		/* 1000BASE-X */
+		u16 anar = 0x0000;
+
+		if (exsr & PHY_EXSR_1000XF) {
+			anar |= PHY_X_ANLPAR_FD;
+		}
+		if (exsr & PHY_EXSR_1000XH) {
+			anar |= PHY_X_ANLPAR_HD;
+		}
+		miiphy_write (devname, addr, PHY_ANAR, anar);
+	} else
+#endif
+	{
+		u16 anar, btcr;
+
+		miiphy_read (devname, addr, PHY_ANAR, &anar);
+		anar &= ~(0x5000 | PHY_ANLPAR_T4 | PHY_ANLPAR_TXFD |
+			  PHY_ANLPAR_TX | PHY_ANLPAR_10FD | PHY_ANLPAR_10);
+
+		miiphy_read (devname, addr, PHY_1000BTCR, &btcr);
+		btcr &= ~(0x00FF | PHY_1000BTCR_1000FD | PHY_1000BTCR_1000HD);
+
+		if (bmsr & PHY_BMSR_100T4) {
+			anar |= PHY_ANLPAR_T4;
+		}
+		if (bmsr & PHY_BMSR_100TXF) {
+			anar |= PHY_ANLPAR_TXFD;
+		}
+		if (bmsr & PHY_BMSR_100TXH) {
+			anar |= PHY_ANLPAR_TX;
+		}
+		if (bmsr & PHY_BMSR_10TF) {
+			anar |= PHY_ANLPAR_10FD;
+		}
+		if (bmsr & PHY_BMSR_10TH) {
+			anar |= PHY_ANLPAR_10;
+		}
+		miiphy_write (devname, addr, PHY_ANAR, anar);
+
+#if defined(CONFIG_PHY_GIGE)
+		if (exsr & PHY_EXSR_1000TF) {
+			btcr |= PHY_1000BTCR_1000FD;
+		}
+		if (exsr & PHY_EXSR_1000TH) {
+			btcr |= PHY_1000BTCR_1000HD;
+		}
+		miiphy_write (devname, addr, PHY_1000BTCR, btcr);
+#endif
+	}
+
+#else /* defined(CONFIG_PHY_DYNAMIC_ANEG) */
+	/*
+	 * Set up standard advertisement
+	 */
+	u16 adv;

-	/* Setup standard advertise */
 	miiphy_read (devname, addr, PHY_ANAR, &adv);
 	adv |= (PHY_ANLPAR_ACK | PHY_ANLPAR_RF | PHY_ANLPAR_T4 |
 		PHY_ANLPAR_TXFD | PHY_ANLPAR_TX | PHY_ANLPAR_10FD |
@@ -94,15 +168,16 @@ int phy_setup_aneg (char *devname, unsigned char addr)
 	adv |= (0x0300);
 	miiphy_write (devname, addr, PHY_1000BTCR, adv);

+#endif /* defined(CONFIG_PHY_DYNAMIC_ANEG) */
+
 	/* Start/Restart aneg */
-	miiphy_read (devname, addr, PHY_BMCR, &ctl);
-	ctl |= (PHY_BMCR_AUTON | PHY_BMCR_RST_NEG);
-	miiphy_write (devname, addr, PHY_BMCR, ctl);
+	miiphy_read (devname, addr, PHY_BMCR, &bmcr);
+	bmcr |= (PHY_BMCR_AUTON | PHY_BMCR_RST_NEG);
+	miiphy_write (devname, addr, PHY_BMCR, bmcr);

 	return 0;
 }

-
 /***********************************************************/
 /* read a phy reg and return the value with a rc	   */
 /***********************************************************/
@@ -145,21 +220,20 @@ unsigned int miiphy_getemac_offset (void)
 #endif
 }

-
-int emac4xx_miiphy_read (char *devname, unsigned char addr,
-		unsigned char reg, unsigned short *value)
+int emac4xx_miiphy_read (char *devname, unsigned char addr, unsigned char reg,
+			 unsigned short *value)
 {
 	unsigned long sta_reg;	/* STA scratch area */
 	unsigned long i;
 	unsigned long emac_reg;

-
 	emac_reg = miiphy_getemac_offset ();
 	/* see if it is ready for 1000 nsec */
 	i = 0;

 	/* see if it is ready for  sec */
-	while ((in32 (EMAC_STACR + emac_reg) & EMAC_STACR_OC) == EMAC_STACR_OC_MASK) {
+	while ((in32 (EMAC_STACR + emac_reg) & EMAC_STACR_OC) ==
+	       EMAC_STACR_OC_MASK) {
 		udelay (7);
 		if (i > 5) {
 #ifdef ET_DEBUG
@@ -175,10 +249,10 @@ int emac4xx_miiphy_read (char *devname, unsigned char addr,
 	/* set clock (50Mhz) and read flags */
 #if defined(CONFIG_440GX) || defined(CONFIG_440SPE) || \
     defined(CONFIG_440EPX) || defined(CONFIG_440GRX)
-#if defined(CONFIG_IBM_EMAC4_V4)      /* EMAC4 V4 changed bit setting */
-		sta_reg = (sta_reg & ~EMAC_STACR_OP_MASK) | EMAC_STACR_READ;
+#if defined(CONFIG_IBM_EMAC4_V4)	/* EMAC4 V4 changed bit setting */
+	sta_reg = (sta_reg & ~EMAC_STACR_OP_MASK) | EMAC_STACR_READ;
 #else
-		sta_reg |= EMAC_STACR_READ;
+	sta_reg |= EMAC_STACR_READ;
 #endif
 #else
 	sta_reg = (sta_reg | EMAC_STACR_READ) & ~EMAC_STACR_CLK_100MHZ;
@@ -198,7 +272,7 @@ int emac4xx_miiphy_read (char *devname, unsigned char addr,

 	sta_reg = in32 (EMAC_STACR + emac_reg);
 #ifdef ET_DEBUG
-		printf ("a21: read : EMAC_STACR=0x%0x\n", sta_reg);	/* test-only */
+	printf ("a21: read : EMAC_STACR=0x%0x\n", sta_reg);	/* test-only */
 #endif
 	i = 0;
 	while ((sta_reg & EMAC_STACR_OC) == EMAC_STACR_OC_MASK) {
@@ -216,19 +290,17 @@ int emac4xx_miiphy_read (char *devname, unsigned char addr,
 		return -1;
 	}

-	*value = *(short *) (&sta_reg);
+	*value = *(short *)(&sta_reg);
 	return 0;

-
 }				/* phy_read */

-
 /***********************************************************/
 /* write a phy reg and return the value with a rc	    */
 /***********************************************************/

-int emac4xx_miiphy_write (char *devname, unsigned char addr,
-		unsigned char reg, unsigned short value)
+int emac4xx_miiphy_write (char *devname, unsigned char addr, unsigned char reg,
+			  unsigned short value)
 {
 	unsigned long sta_reg;	/* STA scratch area */
 	unsigned long i;
@@ -238,7 +310,8 @@ int emac4xx_miiphy_write (char *devname, unsigned char addr,
 	/* see if it is ready for 1000 nsec */
 	i = 0;

-	while ((in32 (EMAC_STACR + emac_reg) & EMAC_STACR_OC) == EMAC_STACR_OC_MASK) {
+	while ((in32 (EMAC_STACR + emac_reg) & EMAC_STACR_OC) ==
+	       EMAC_STACR_OC_MASK) {
 		if (i > 5)
 			return -1;
 		udelay (7);
@@ -249,10 +322,10 @@ int emac4xx_miiphy_write (char *devname, unsigned char addr,
 	/* set clock (50Mhz) and read flags */
 #if defined(CONFIG_440GX) || defined(CONFIG_440SPE) || \
     defined(CONFIG_440EPX) || defined(CONFIG_440GRX)
-#if defined(CONFIG_IBM_EMAC4_V4)      /* EMAC4 V4 changed bit setting */
-		sta_reg = (sta_reg & ~EMAC_STACR_OP_MASK) | EMAC_STACR_WRITE;
+#if defined(CONFIG_IBM_EMAC4_V4)	/* EMAC4 V4 changed bit setting */
+	sta_reg = (sta_reg & ~EMAC_STACR_OP_MASK) | EMAC_STACR_WRITE;
 #else
-		sta_reg |= EMAC_STACR_WRITE;
+	sta_reg |= EMAC_STACR_WRITE;
 #endif
 #else
 	sta_reg = (sta_reg | EMAC_STACR_WRITE) & ~EMAC_STACR_CLK_100MHZ;
@@ -263,8 +336,8 @@ int emac4xx_miiphy_write (char *devname, unsigned char addr,
     !defined(CONFIG_440EPX) && !defined(CONFIG_440GRX)
 	sta_reg = sta_reg | CONFIG_PHY_CLK_FREQ;	/* Set clock frequency (PLB freq. dependend) */
 #endif
-	sta_reg = sta_reg | ((unsigned long) addr << 5);/* Phy address */
-	sta_reg = sta_reg | EMAC_STACR_OC_MASK;		/* new IBM emac v4 */
+	sta_reg = sta_reg | ((unsigned long)addr << 5);	/* Phy address */
+	sta_reg = sta_reg | EMAC_STACR_OC_MASK;	/* new IBM emac v4 */
 	memcpy (&sta_reg, &value, 2);	/* put in data */

 	out32 (EMAC_STACR + emac_reg, sta_reg);
@@ -273,7 +346,7 @@ int emac4xx_miiphy_write (char *devname, unsigned char addr,
 	i = 0;
 	sta_reg = in32 (EMAC_STACR + emac_reg);
 #ifdef ET_DEBUG
-		printf ("a31: read : EMAC_STACR=0x%0x\n", sta_reg);	/* test-only */
+	printf ("a31: read : EMAC_STACR=0x%0x\n", sta_reg);	/* test-only */
 #endif
 	while ((sta_reg & EMAC_STACR_OC) == EMAC_STACR_OC_MASK) {
 		udelay (7);
diff --git a/include/miiphy.h b/include/miiphy.h
index 71716b0..0332e4b 100644
--- a/include/miiphy.h
+++ b/include/miiphy.h
@@ -33,6 +33,7 @@
 |  04-May-99   Created							MKW
 |  07-Jul-99   Added full duplex support				MKW
 |  08-Sep-01   Tweaks							gvb
+|  28-Oct-07   Added 1000BASE-X support.  Larry Johnson <lrj@acm.org>   lrj
 |
 +----------------------------------------------------------------------------*/
 #ifndef _miiphy_h_
@@ -40,26 +41,26 @@

 #include <net.h>

-int  miiphy_read(char *devname, unsigned char addr, unsigned char reg,
+int miiphy_read(char *devname, unsigned char addr, unsigned char reg,
 		unsigned short *value);
-int  miiphy_write(char *devname, unsigned char addr, unsigned char reg,
-		unsigned short value);
-int  miiphy_info(char *devname, unsigned char addr, unsigned int  *oui,
+int miiphy_write(char *devname, unsigned char addr, unsigned char reg,
+		 unsigned short value);
+int miiphy_info(char *devname, unsigned char addr, unsigned int *oui,
 		unsigned char *model, unsigned char *rev);
-int  miiphy_reset(char *devname, unsigned char addr);
-int  miiphy_speed(char *devname, unsigned char addr);
-int  miiphy_duplex(char *devname, unsigned char addr);
+int miiphy_reset(char *devname, unsigned char addr);
+int miiphy_speed(char *devname, unsigned char addr);
+int miiphy_duplex(char *devname, unsigned char addr);
 #ifdef CFG_FAULT_ECHO_LINK_DOWN
-int  miiphy_link(char *devname, unsigned char addr);
+int miiphy_link(char *devname, unsigned char addr);
 #endif

 void miiphy_init(void);

 void miiphy_register(char *devname,
-	int (* read)(char *devname, unsigned char addr,
-		unsigned char reg, unsigned short *value),
-	int (* write)(char *devname, unsigned char addr,
-		unsigned char reg, unsigned short value));
+		     int (*read) (char *devname, unsigned char addr,
+				  unsigned char reg, unsigned short *value),
+		     int (*write) (char *devname, unsigned char addr,
+				   unsigned char reg, unsigned short value));

 int miiphy_set_current_dev(char *devname);
 char *miiphy_get_current_dev(void);
@@ -68,14 +69,14 @@ void miiphy_listdev(void);

 #define BB_MII_DEVNAME	"bbmii"

-int bb_miiphy_read (char *devname, unsigned char addr,
-		unsigned char reg, unsigned short *value);
-int bb_miiphy_write (char *devname, unsigned char addr,
-		unsigned char reg, unsigned short value);
+int bb_miiphy_read(char *devname, unsigned char addr,
+		   unsigned char reg, unsigned short *value);
+int bb_miiphy_write(char *devname, unsigned char addr,
+		    unsigned char reg, unsigned short value);

 /* phy seed setup */
 #define AUTO			99
-#define _1000BASET              1000
+#define _1000BASET		1000
 #define _100BASET		100
 #define _10BASET		10
 #define HALF			22
@@ -90,9 +91,10 @@ int bb_miiphy_write (char *devname, unsigned char addr,
 #define PHY_ANLPAR		0x05
 #define PHY_ANER		0x06
 #define PHY_ANNPTR		0x07
-#define PHY_ANLPNP              0x08
-#define PHY_1000BTCR            0x09
-#define PHY_1000BTSR            0x0A
+#define PHY_ANLPNP		0x08
+#define PHY_1000BTCR		0x09
+#define PHY_1000BTSR		0x0A
+#define PHY_EXSR		0x0F
 #define PHY_PHYSTS		0x10
 #define PHY_MIPSCR		0x11
 #define PHY_MIPGSR		0x12
@@ -126,6 +128,7 @@ int bb_miiphy_write (char *devname, unsigned char addr,
 #define PHY_BMSR_100TXH		0x2000
 #define PHY_BMSR_10TF		0x1000
 #define PHY_BMSR_10TH		0x0800
+#define PHY_BMSR_EXT_STAT	0x0100
 #define PHY_BMSR_PRE_SUP	0x0040
 #define PHY_BMSR_AUTN_COMP	0x0020
 #define PHY_BMSR_RF		0x0010
@@ -138,18 +141,31 @@ int bb_miiphy_write (char *devname, unsigned char addr,
 #define PHY_ANLPAR_NP		0x8000
 #define PHY_ANLPAR_ACK		0x4000
 #define PHY_ANLPAR_RF		0x2000
+#define PHY_ANLPAR_ASYMP	0x0800
+#define PHY_ANLPAR_PAUSE	0x0400
 #define PHY_ANLPAR_T4		0x0200
 #define PHY_ANLPAR_TXFD		0x0100
 #define PHY_ANLPAR_TX		0x0080
 #define PHY_ANLPAR_10FD		0x0040
 #define PHY_ANLPAR_10		0x0020
-#define PHY_ANLPAR_100		0x0380	    /* we can run at 100 */
+#define PHY_ANLPAR_100		0x0380	/* we can run at 100 */
+/* phy ANLPAR 1000BASE-X */
+#define PHY_X_ANLPAR_NP		0x8000
+#define PHY_X_ANLPAR_ACK	0x4000
+#define PHY_X_ANLPAR_RF_MASK	0x3000
+#define PHY_X_ANLPAR_PAUSE_MASK	0x0180
+#define PHY_X_ANLPAR_HD		0x0040
+#define PHY_X_ANLPAR_FD		0x0020

 #define PHY_ANLPAR_PSB_MASK     0x001f
 #define PHY_ANLPAR_PSB_802_3    0x0001
 #define PHY_ANLPAR_PSB_802_9    0x0002

-/* PHY_1000BTSR */
+/* phy 1000BTCR */
+#define PHY_1000BTCR_1000FD     0x0200
+#define PHY_1000BTCR_1000HD     0x0100
+
+/* phy 1000BTSR */
 #define PHY_1000BTSR_MSCF       0x8000
 #define PHY_1000BTSR_MSCR       0x4000
 #define PHY_1000BTSR_LRS        0x2000
@@ -157,4 +173,10 @@ int bb_miiphy_write (char *devname, unsigned char addr,
 #define PHY_1000BTSR_1000FD     0x0800
 #define PHY_1000BTSR_1000HD     0x0400

+/* phy EXSR */
+#define PHY_EXSR_1000XF		0x8000
+#define PHY_EXSR_1000XH		0x4000
+#define PHY_EXSR_1000TF		0x2000
+#define PHY_EXSR_1000TH		0x1000
+
 #endif

[U-Boot-Users] [PATCH] PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

[Ben Warren]

Larry Johnson wrote:
> This patch adds a new switch: "CONFIG_PHY_DYNAMIC_ANEG".  When this symbol
> is defined, the PHY will advertise it's capabilities for autonegotiation
> based on the capabilities shown in the PHY's status registers, including
> 1000BASE-X.  When "CONFIG_PHY_DYNAMIC_ANEG" is not defined, the PHY will
> advertise hard-coded capabilities, as before.
>   
I won't address the content yet, just the cosmetic changes that make up 
the bulk of this patch. I think you've done a good thing by running 
Lindent (or whatever) on all the files you've touched, but it has the 
effect of obscuring the meat of your work. No need to re-do it this 
time, but IMHO, purely cosmetic changes should be separate patches and 
labeled as such.

Of course, that's just MHO, and others may feel differently. Thoughts?

regards,
Ben

[U-Boot-Users] [PATCH] PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

[Larry Johnson]

Ben Warren wrote:
> Larry Johnson wrote:
>> This patch adds a new switch: "CONFIG_PHY_DYNAMIC_ANEG".  When this
>> symbol
>> is defined, the PHY will advertise it's capabilities for autonegotiation
>> based on the capabilities shown in the PHY's status registers, including
>> 1000BASE-X.  When "CONFIG_PHY_DYNAMIC_ANEG" is not defined, the PHY will
>> advertise hard-coded capabilities, as before.
>>   
> I won't address the content yet, just the cosmetic changes that make up
> the bulk of this patch. I think you've done a good thing by running
> Lindent (or whatever) on all the files you've touched, but it has the
> effect of obscuring the meat of your work. No need to re-do it this
> time, but IMHO, purely cosmetic changes should be separate patches and
> labeled as such.
> 
> Of course, that's just MHO, and others may feel differently. Thoughts?
> 
> regards,
> Ben


Thanks for bringing this up.  I noticed the same probelm, but couldn't
think of how to handle it.  I want to run Lindent on my changes to fix
any issues with them, especially as the U-Boot/Linux style is so
different from what I'm used to.  I thought about running Lindent and
copying just my changes back to the original, but that seems error prone
(and too much work).

Would it be better to run Lindent on the original files, post those as
changes, and then post a second patch with the new material?

BTW, I've noticed that the "Lindent -pcs" format is used inconsistently
within files.  Since I'm also making changes in Linux, it's hard to
remember whether to type "foo(bar);" or "foo (bar);", maybe others have
this difficulty, too.  I've tried to use the form that is prevalent in
each individual file, though some are pretty much split down the middle.

How do others feel?

Regards, Larry

[U-Boot-Users] [PATCH] PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

[Jon Loeliger]

On Mon, 2007-10-29 at 13:41, Larry Johnson wrote:

> Would it be better to run Lindent on the original files, post those as
> changes, and then post a second patch with the new material?

In my opinion, yes, please.

> BTW, I've noticed that the "Lindent -pcs" format is used inconsistently
> within files.  Since I'm also making changes in Linux, it's hard to
> remember whether to type "foo(bar);" or "foo (bar);", 

In my opinion, "foo (bar)" is bad style, and I prefer
the Linux form, "foo(bar)".

jdl

[U-Boot-Users] [PATCH] PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

[Stefan Roese]

On Monday 29 October 2007, Jon Loeliger wrote:
> On Mon, 2007-10-29 at 13:41, Larry Johnson wrote:
> > Would it be better to run Lindent on the original files, post those as
> > changes, and then post a second patch with the new material?
>
> In my opinion, yes, please.

Yes, please do the coding style cleanup first, and the "real" patch later in a 
separate commit. Otherwise it will very hard to make out your changes in the 
commit-diff.

> > BTW, I've noticed that the "Lindent -pcs" format is used inconsistently
> > within files.  Since I'm also making changes in Linux, it's hard to
> > remember whether to type "foo(bar);" or "foo (bar);",
>
> In my opinion, "foo (bar)" is bad style, and I prefer
> the Linux form, "foo(bar)".

This is my way of coding too. Mainly because I'm used to doing it this way and 
Linux uses it too. But I know that Wolfgang prefers the "other" style. My 
vote would be, when you really run Lindent on this file (or other), switch to 
the Linux style completely.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot-Users] [PATCH] PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

[Larry Johnson]

Stefan Roese wrote:
> On Monday 29 October 2007, Jon Loeliger wrote:
>> On Mon, 2007-10-29 at 13:41, Larry Johnson wrote:
>>> Would it be better to run Lindent on the original files, post those as
>>> changes, and then post a second patch with the new material?
>> In my opinion, yes, please.
> 
> Yes, please do the coding style cleanup first, and the "real" patch later in a 
> separate commit. Otherwise it will very hard to make out your changes in the 
> commit-diff.
> 
>>> BTW, I've noticed that the "Lindent -pcs" format is used inconsistently
>>> within files.  Since I'm also making changes in Linux, it's hard to
>>> remember whether to type "foo(bar);" or "foo (bar);",
>> In my opinion, "foo (bar)" is bad style, and I prefer
>> the Linux form, "foo(bar)".
> 
> This is my way of coding too. Mainly because I'm used to doing it this way and 
> Linux uses it too. But I know that Wolfgang prefers the "other" style. My 
> vote would be, when you really run Lindent on this file (or other), switch to 
> the Linux style completely.
> 
> Best regards,
> Stefan
> 
> =====================================================================
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
> =====================================================================
> 

Thanks everyone for the suggests.  I will split the cosmetic from the
functional patches and resubmit for further comments.

Best regards, Larry

[U-Boot-Users] [PATCH] PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

[Ben Warren]

Jon Loeliger wrote:
> On Mon, 2007-10-29 at 13:41, Larry Johnson wrote:
>
>   
>> Would it be better to run Lindent on the original files, post those as
>> changes, and then post a second patch with the new material?
>>     
>
> In my opinion, yes, please.
>
>   
>> BTW, I've noticed that the "Lindent -pcs" format is used inconsistently
>> within files.  Since I'm also making changes in Linux, it's hard to
>> remember whether to type "foo(bar);" or "foo (bar);", 
>>     
>
> In my opinion, "foo (bar)" is bad style, and I prefer
> the Linux form, "foo(bar)".
>
> jdl
>
>   
Yeah, in Larry's patch, Lindent mostly created the 'bad-style' format. I 
thought the idea behind Lindent was that you didn't need to figure out 
all the appropriate switches for 'indent'. I don't know what '-pcs' 
does... Of course, my Lindent is old (c 2.6.19) and maybe it's been 
changed since then.

regards,
Ben

[U-Boot-Users] [PATCH] PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

[Wolfgang Denk]

In message <1193685290.1462.19.camel@ld0161-tx32> you wrote:
> 
> In my opinion, "foo (bar)" is bad style, and I prefer
> the Linux form, "foo(bar)".

That's your opinion, which I usually  value,  but  here  it  will  be
ignored ;-)

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
What is mind?  No matter.  What is matter?  Never mind.
                                      -- Thomas Hewitt Key, 1799-1875

[U-Boot-Users] [PATCH] PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

[Wolfgang Denk]

In message <47262967.8070307@arlinx.com> you wrote:
>
> Would it be better to run Lindent on the original files, post those as
> changes, and then post a second patch with the new material?

If it's your code, or other original code, then yes, this is the way
to go.

But if it's code that was "borrowed" from somewhere else  (like  from
the  Linux  kernel, busybox, etc.) and then only slightly modified to
make it work under U-Boot, then please *DO*  *NOT*  reformat  it,  as
this basicly makes it impossible to update the file from new versions
of the original code.

> BTW, I've noticed that the "Lindent -pcs" format is used inconsistently
> within files.  Since I'm also making changes in Linux, it's hard to
> remember whether to type "foo(bar);" or "foo (bar);", maybe others have
> this difficulty, too.  I've tried to use the form that is prevalent in
> each individual file, though some are pretty much split down the middle.
> 
> How do others feel?

It should be "foo (bar)".

And all this should be sufficiently documented at
http://www.denx.de/wiki/UBoot/CodingStyle

If you feel that details are missing or not  clear  enought  in  this
text please help to extend / fix it.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I distrust all systematisers, and avoid them. The will  to  a  system
shows a lack of honesty.
- Friedrich Wilhelm Nietzsche _G?tzen-D?mmerung [The Twilight of  the
Idols]_ ``Maxims and Missiles'' no. 26

[U-Boot-Users] [PATCH] PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

[Larry Johnson]

Wolfgang Denk wrote:
> In message <47262967.8070307@arlinx.com> you wrote:
>> Would it be better to run Lindent on the original files, post those as
>> changes, and then post a second patch with the new material?
> 
> If it's your code, or other original code, then yes, this is the way
> to go.
> 
> But if it's code that was "borrowed" from somewhere else  (like  from
> the  Linux  kernel, busybox, etc.) and then only slightly modified to
> make it work under U-Boot, then please *DO*  *NOT*  reformat  it,  as
> this basicly makes it impossible to update the file from new versions
> of the original code.

Because, in general, it's hard to know the source of the code, it sounds
like the safest thing is for my to use Lindent only to identify problems
the patch, and fix them manually.

>> BTW, I've noticed that the "Lindent -pcs" format is used inconsistently
>> within files.  Since I'm also making changes in Linux, it's hard to
>> remember whether to type "foo(bar);" or "foo (bar);", maybe others have
>> this difficulty, too.  I've tried to use the form that is prevalent in
>> each individual file, though some are pretty much split down the middle.
>>
>> How do others feel?
> 
> It should be "foo (bar)".
> 
> And all this should be sufficiently documented at
> http://www.denx.de/wiki/UBoot/CodingStyle
> 
> If you feel that details are missing or not  clear  enought  in  this
> text please help to extend / fix it.

The text is clear, I just couldn't decide if this was "a custom more
honour'd in the breach than the observance" :-).

> Best regards,
> 
> Wolfgang Denk

Thanks for your help, I'll re-redo the patch.

Best regards, Larry

[U-Boot-Users] [PATCH] PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

[Stefan Roese]

On Monday 29 October 2007, Larry Johnson wrote:
> This patch adds a new switch: "CONFIG_PHY_DYNAMIC_ANEG".  When this symbol
> is defined, the PHY will advertise it's capabilities for autonegotiation
> based on the capabilities shown in the PHY's status registers, including
> 1000BASE-X.  When "CONFIG_PHY_DYNAMIC_ANEG" is not defined, the PHY will
> advertise hard-coded capabilities, as before.
>
> Signed-off-by: Larry Johnson <lrj@acm.org>

<snip>

> diff --git a/cpu/ppc4xx/miiphy.c b/cpu/ppc4xx/miiphy.c
> index 6b98025..55d777f 100644
> --- a/cpu/ppc4xx/miiphy.c
> +++ b/cpu/ppc4xx/miiphy.c
> @@ -39,6 +39,9 @@
>
>    |  19-Jul-00	 Ported to esd cpci405					     sr
>    |  23-Dec-03	 Ported from miiphy.c to 440GX Travis Sawyer		     TBS
>    |		 <travis.sawyer@sandburst.com>
>
> +  |  20-Oct-07	 Added support for autonegotiation base on reported	    
> lrj +  |		 capabilities, including 1000BASE-X.  Larry Johnson
> +  |		 <lrj@acm.org>

Please don't add commit logs to the source files. I know it has been done 
before, but we use git to maintain such change messages. If you like you can 
remove the older change messages too in your next patch version.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================

[U-Boot-Users] [PATCH] PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

[Ben Warren]

Larry Johnson wrote:
> This patch adds a new switch: "CONFIG_PHY_DYNAMIC_ANEG".  When this symbol
> is defined, the PHY will advertise it's capabilities for autonegotiation
> based on the capabilities shown in the PHY's status registers, including
> 1000BASE-X.  When "CONFIG_PHY_DYNAMIC_ANEG" is not defined, the PHY will
> advertise hard-coded capabilities, as before.
>
>   
<snip>
> -
>  /*****************************************************************************
>   *
> - * Determine the ethernet speed (10/100).
> + * Determine the ethernet speed (10/100/1000).  Return 10 on error.
>   */
>  int miiphy_speed (char *devname, unsigned char addr)
>  {
> -	unsigned short reg;
> +	u16 bmcr;
>
>  #if defined(CONFIG_PHY_GIGE)
> -	if (miiphy_read (devname, addr, PHY_1000BTSR, &reg)) {
> -		printf ("PHY 1000BT Status read failed\n");
> -	} else {
> -		if (reg != 0xFFFF) {
> -			if ((reg & (PHY_1000BTSR_1000FD | PHY_1000BTSR_1000HD)) !=0) {
> -				return (_1000BASET);
> -			}
> +	u16 btsr;
> +
> +#if defined(CONFIG_PHY_DYNAMIC_ANEG)
> +	u16 bmsr;
> +
> +	/* Check for 1000BASE-X. */
> +	if (miiphy_read (devname, addr, PHY_BMSR, &bmsr)) {
> +		printf ("PHY status");
> +		goto miiphy_read_failed;
> +	}
> +	if (bmsr & PHY_BMSR_EXT_STAT) {
> +		u16 exsr;
>   
Please don't define variables in code blocks. The original code used a 
single variable called 'reg' that worked well enough. The bitmasks 
(PHY_BMSR_* and so on) effectively tell the reader what register he/she 
is looking at.

I'm going to stop here and wait for you to separate content from 
cosmetic, and hopefully fix up what I've mentioned here.

regards,
Ben

[U-Boot-Users] [PATCH] PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

[Larry Johnson]

Ben Warren wrote:
> Larry Johnson wrote:
>> This patch adds a new switch: "CONFIG_PHY_DYNAMIC_ANEG".  When this
>> symbol
>> is defined, the PHY will advertise it's capabilities for autonegotiation
>> based on the capabilities shown in the PHY's status registers, including
>> 1000BASE-X.  When "CONFIG_PHY_DYNAMIC_ANEG" is not defined, the PHY will
>> advertise hard-coded capabilities, as before.
>>
>>   
> <snip>
>> -
>>  /*****************************************************************************
>>
>>   *
>> - * Determine the ethernet speed (10/100).
>> + * Determine the ethernet speed (10/100/1000).  Return 10 on error.
>>   */
>>  int miiphy_speed (char *devname, unsigned char addr)
>>  {
>> -    unsigned short reg;
>> +    u16 bmcr;
>>
>>  #if defined(CONFIG_PHY_GIGE)
>> -    if (miiphy_read (devname, addr, PHY_1000BTSR, &reg)) {
>> -        printf ("PHY 1000BT Status read failed\n");
>> -    } else {
>> -        if (reg != 0xFFFF) {
>> -            if ((reg & (PHY_1000BTSR_1000FD | PHY_1000BTSR_1000HD))
>> !=0) {
>> -                return (_1000BASET);
>> -            }
>> +    u16 btsr;
>> +
>> +#if defined(CONFIG_PHY_DYNAMIC_ANEG)
>> +    u16 bmsr;
>> +
>> +    /* Check for 1000BASE-X. */
>> +    if (miiphy_read (devname, addr, PHY_BMSR, &bmsr)) {
>> +        printf ("PHY status");
>> +        goto miiphy_read_failed;
>> +    }
>> +    if (bmsr & PHY_BMSR_EXT_STAT) {
>> +        u16 exsr;
>>   
> Please don't define variables in code blocks. The original code used a
> single variable called 'reg' that worked well enough. The bitmasks
> (PHY_BMSR_* and so on) effectively tell the reader what register he/she
> is looking at.
> 
> I'm going to stop here and wait for you to separate content from
> cosmetic, and hopefully fix up what I've mentioned here.
> 
> regards,
> Ben
> 

Hi Ben,

You bring up two issues here, and I'm wondering how you and others feel
about them.  The first is whether all variables should be declared at
the start of the function, or within the smallest block in which they
are used.  I prefer to declare variables close to where they are used,
but don't mind doing it either way.

I do think that using separate dedicated variables for to hold the
different registers is a better coding practice.  The compiler can do a
better job keeping track of variable lifetimes than I can, and someone
modifying the code later won't have to check to see if it's safe to
modify 'reg' at some point or other.  Also, the register and bitmask
definitions make it obvious that the code is correct, as in

    "if (miiphy_read (devname, addr, PHY_BMSR, &bmsr)) {"

and

    "if (bmsr & PHY_BMSR_EXT_STAT) {".

Those are the advantage I see, what do you see as the disadvantages?

Best regards, Larry