Skip to contentSkip to conversation

PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

PATCH New Larry Johnson 5 participants 13 messages Oct 29, 2007

lore
Share & tools
lore

Larry Johnson

author
#0 lore raw

PATCH 1/1 PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

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 <[email protected]>

---

common/miiphyutil.c +165 -124
index c69501f..39e3c20 100644
@@ -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 */
cpu/ppc4xx/miiphy.c +102 -29

234 lines not on this page · raw message

include/miiphy.h +44 -22

127 lines not on this page · raw message

#1 lore raw

PATCH 1/1 PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

Quoted 6 lines · 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.

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

author
#2 lore raw

PATCH 1/1 PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

Quoted 15 lines · Ben Warren
Quoted 7 lines · 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.

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.

Quoted 4 lines

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

#3 lore raw

PATCH 1/1 PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

Quoted 2 lines · Larry Johnson

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.

Quoted 3 lines

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

#4 lore raw

PATCH 1/1 PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

Quoted 5 lines · Jon Loeliger
Quoted 2 lines · Larry Johnson

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.

Quoted 6 lines
Quoted 3 lines

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

author
#5 lore raw

PATCH 1/1 PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

Quoted 21 lines · Stefan Roese
Quoted 4 lines · Jon Loeliger
Quoted 2 lines · Larry Johnson

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.

Quoted 5 lines
Quoted 3 lines

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.

Quoted 9 lines

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

#6 lore raw

PATCH 1/1 PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

In reply to Jon Loeliger
Quoted 15 lines · Jon Loeliger
Quoted 3 lines · Larry Johnson

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.

Quoted 4 lines

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);",

Quoted 6 lines

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

#7 lore raw

PATCH 1/1 PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

In reply to Jon Loeliger
Quoted 3 lines · 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

Signature · 5 lines
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
#8 lore raw

PATCH 1/1 PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

In reply to Larry Johnson
Quoted 3 lines · In message <[email protected]> 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.

Quoted 7 lines

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

Signature · 7 lines
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
author
#9 lore raw

PATCH 1/1 PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

Quoted 4 lines · Wolfgang Denk
Quoted 2 lines · In message <[email protected]> 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?

Quoted 8 lines

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.

Quoted 8 lines
Quoted 7 lines

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?

Quoted 7 lines

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" :-).

Quoted 3 lines

Best regards,

Wolfgang Denk

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

Best regards, Larry

#10 lore raw

PATCH 1/1 PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

In reply to Larry Johnson
Quoted 7 lines · 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 <[email protected]>

<snip>

Quoted 11 lines
cpu/ppc4xx/miiphy.c +1 -0
index 6b98025..55d777f 100644
@@ -39,6 +39,9 @@
 
   |  19-Jul-00	 Ported to esd cpci405					     sr
   |  23-Dec-03	 Ported from miiphy.c to 440GX Travis Sawyer		     TBS
   |		 <[email protected]>
 
+  |  20-Oct-07	 Added support for autonegotiation base on reported
Quoted 2 lines

lrj + | capabilities, including 1000BASE-X. Larry Johnson
+ | <[email protected]>

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

#11 lore raw

PATCH 1/1 PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

In reply to Larry Johnson
Quoted 7 lines · 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.

<snip>

Quoted 22 lines
common/miiphyutil.c +4 -10
-
 /*****************************************************************************
  *
- * 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;
+

common/miiphyutil.c int miiphy_reset (char *devname, unsigned char addr) · line 364

Quoted 10 lines
common/miiphyutil.c +10 -0
+#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

author
#12 lore raw

PATCH 1/1 PPC4xx: Add Ethernet 1000BASE-X support for PPC4xx

Quoted 49 lines · Ben Warren
Quoted 8 lines · 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.

<snip>

Quoted 24 lines
common/miiphyutil.c +2 -8
-
 /*****************************************************************************
 
  *
- * 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) {

common/miiphyutil.c +2 -2
-                return (_1000BASET);
-            }
+    u16 btsr;
+

common/miiphyutil.c int miiphy_reset (char *devname, unsigned char addr) · line 364

Quoted 10 lines
common/miiphyutil.c +10 -0
+#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.

Quoted 6 lines

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