Re: spidernet: add improved phy support in sungem_phy.c

2007-02-01 Thread Jens Osterkamp 

Francois,

thank you for your comments. I'll send a revised patch soon.

 +#define BCM5421_MODE_MASK1  5
 
 Please add parenthesis.

Done.

  is fine despite the lack of parenthesis above but it is error-prone.

Corrected.

 
 +
 + if ( mode == GMII_COPPER) {
^^^
 + return genmii_poll_link(phy);
 + }
 
 No curly-braces for single line statements please.

I corrected this on all my additions.

 Ternary operator ?

I dont like ternary operators. Yes, I know, they are cool, but
they make the code much less readable IMHO.

 +#define BCM5461_FIBER_LINK   1  2
 +#define BCM5461_MODE_MASK3  1
 
 Please add parenthesis.

Done.

 Join the dark side and use a ternary operator.

See above.

 +static int bcm5461_enable_fiber(struct mii_phy* phy, int autoneg)
 +{
 + /*
 + phy_write(phy, MII_NCONFIG, 0xfc0c);
 + phy_write(phy, MII_BMCR, 0x4140);
 + */
 
 Remove ?

Done.

Jens
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: spidernet: add improved phy support in sungem_phy.c

2007-02-01 Thread Jens Osterkamp 
On Tuesday 30 January 2007 11:30 pm, Linas Vepstas wrote:

 Shifting to the right by 5 bits has no effect on the result
 of this conditional. Either the bit is set, or its not.
 There is no need to shift.
 
  +   if ( (phy_reg  0x0020)  7 ) {

You are right, I corrected this.

 The result here will always be zero, since the bit,
 if set, will be shifted off the end. Bits on the lef
 are padded with zero.  Ergo, this is a bug.

You are right, wrong bit. I corrected this.

Jens
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: spidernet: add improved phy support in sungem_phy.c

2007-01-30 Thread Linas Vepstas 
On Sat, Jan 27, 2007 at 12:38:09AM +0100, Francois Romieu wrote:
 Jens Osterkamp [EMAIL PROTECTED] :
  
 Index: linux-2.6.20-rc5/drivers/net/sungem_phy.c
 ===
 --- linux-2.6.20-rc5.orig/drivers/net/sungem_phy.c
 +++ linux-2.6.20-rc5/drivers/net/sungem_phy.c
 @@ -311,6 +311,107 @@ static int bcm5411_init(struct mii_phy* 
 [...]
 + if ( (phy_reg  0x0020)  5 ) {

Shifting to the right by 5 bits has no effect on the result
of this conditional. Either the bit is set, or its not.
There is no need to shift.

 + if ( (phy_reg  0x0020)  7 ) {

The result here will always be zero, since the bit,
if set, will be shifted off the end. Bits on the lef
are padded with zero.  Ergo, this is a bug.

--linas
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

spidernet: add improved phy support in sungem_phy.c

2007-01-26 Thread Jens Osterkamp 
This patch adds improved version of enable_fiber for both the 5421 and
the 5461 phy. It is now possible to specify with these wether you want
autonegotiation or not. This is needed for bladecenter switches where
some expect autonegotiation and some dont seem to like this at all.
Depending on this flag it sets phy-autoneg accordingly for the fiber mode.

More importantly it implements proper read_link and poll_link functions
for both phys which can handle both copper and fiber mode by determining
the medium first and then branching to the required functions. For fiber
they all work fine, for copper they are not tested but return the result
of the genmii_* function anyway which is supposed to work.

A medium variable in the phy struct is introduced to save the medium
with which the phy is currently used.

The patch moves the genmii_* functions around to avoid foreward declarations.

Signed-off-by: Jens Osterkamp [EMAIL PROTECTED]
Signed-off-by: Arnd Bergmann [EMAIL PROTECTED]

Index: linux-2.6.20-rc5/drivers/net/sungem_phy.c
===
--- linux-2.6.20-rc5.orig/drivers/net/sungem_phy.c
+++ linux-2.6.20-rc5/drivers/net/sungem_phy.c
@@ -311,6 +311,107 @@ static int bcm5411_init(struct mii_phy* 
  return 0;
 }
 
+static int genmii_setup_aneg(struct mii_phy *phy, u32 advertise)
+{
+ u16 ctl, adv;
+
+ phy-autoneg = 1;
+ phy-speed = SPEED_10;
+ phy-duplex = DUPLEX_HALF;
+ phy-pause = 0;
+ phy-advertising = advertise;
+
+ /* Setup standard advertise */
+ adv = phy_read(phy, MII_ADVERTISE);
+ adv = ~(ADVERTISE_ALL | ADVERTISE_100BASE4);
+ if (advertise  ADVERTISED_10baseT_Half)
+  adv |= ADVERTISE_10HALF;
+ if (advertise  ADVERTISED_10baseT_Full)
+  adv |= ADVERTISE_10FULL;
+ if (advertise  ADVERTISED_100baseT_Half)
+  adv |= ADVERTISE_100HALF;
+ if (advertise  ADVERTISED_100baseT_Full)
+  adv |= ADVERTISE_100FULL;
+ phy_write(phy, MII_ADVERTISE, adv);
+
+ /* Start/Restart aneg */
+ ctl = phy_read(phy, MII_BMCR);
+ ctl |= (BMCR_ANENABLE | BMCR_ANRESTART);
+ phy_write(phy, MII_BMCR, ctl);
+
+ return 0;
+}
+
+static int genmii_setup_forced(struct mii_phy *phy, int speed, int fd)
+{
+ u16 ctl;
+
+ phy-autoneg = 0;
+ phy-speed = speed;
+ phy-duplex = fd;
+ phy-pause = 0;
+
+ ctl = phy_read(phy, MII_BMCR);
+ ctl = ~(BMCR_FULLDPLX|BMCR_SPEED100|BMCR_ANENABLE);
+
+ /* First reset the PHY */
+ phy_write(phy, MII_BMCR, ctl | BMCR_RESET);
+
+ /* Select speed  duplex */
+ switch(speed) {
+ case SPEED_10:
+  break;
+ case SPEED_100:
+  ctl |= BMCR_SPEED100;
+  break;
+ case SPEED_1000:
+ default:
+  return -EINVAL;
+ }
+ if (fd == DUPLEX_FULL)
+  ctl |= BMCR_FULLDPLX;
+ phy_write(phy, MII_BMCR, ctl);
+
+ return 0;
+}
+
+static int genmii_poll_link(struct mii_phy *phy)
+{
+ u16 status;
+
+ (void)phy_read(phy, MII_BMSR);
+ status = phy_read(phy, MII_BMSR);
+ if ((status  BMSR_LSTATUS) == 0)
+  return 0;
+ if (phy-autoneg  !(status  BMSR_ANEGCOMPLETE))
+  return 0;
+ return 1;
+}
+
+static int genmii_read_link(struct mii_phy *phy)
+{
+ u16 lpa;
+
+ if (phy-autoneg) {
+  lpa = phy_read(phy, MII_LPA);
+
+  if (lpa  (LPA_10FULL | LPA_100FULL))
+   phy-duplex = DUPLEX_FULL;
+  else
+   phy-duplex = DUPLEX_HALF;
+  if (lpa  (LPA_100FULL | LPA_100HALF))
+   phy-speed = SPEED_100;
+  else
+   phy-speed = SPEED_10;
+  phy-pause = 0;
+ }
+ /* On non-aneg, we assume what we put in BMCR is the speed,
+  * though magic-aneg shouldn't prevent this case from occurring
+  */
+
+  return 0;
+}
+
 static int generic_suspend(struct mii_phy* phy)
 {
  phy_write(phy, MII_BMCR, BMCR_PDOWN);
@@ -365,30 +466,6 @@ static int bcm5421_init(struct mii_phy* 
  return 0;
 }
 
-static int bcm5421_enable_fiber(struct mii_phy* phy)
-{
- /* enable fiber mode */
- phy_write(phy, MII_NCONFIG, 0x9020);
- /* LEDs active in both modes, autosense prio = fiber */
- phy_write(phy, MII_NCONFIG, 0x945f);
-
- /* switch off fibre autoneg */
- phy_write(phy, MII_NCONFIG, 0xfc01);
- phy_write(phy, 0x0b, 0x0004);
-
- return 0;
-}
-
-static int bcm5461_enable_fiber(struct mii_phy* phy)
-{
- phy_write(phy, MII_NCONFIG, 0xfc0c);
- phy_write(phy, MII_BMCR, 0x4140);
- phy_write(phy, MII_NCONFIG, 0xfc0b);
- phy_write(phy, MII_BMCR, 0x0140);
-
- return 0;
-}
-
 static int bcm54xx_setup_aneg(struct mii_phy *phy, u32 advertise)
 {
  u16 ctl, adv;
@@ -516,6 +593,166 @@ static int marvell88e_init(struct mi
  return 0;
 }
 
+#define BCM5421_MODE_MASK 1  5
+
+static int bcm5421_poll_link(struct mii_phy* phy)
+{
+ u32 phy_reg;
+ int mode;
+
+ /* find out in what mode we are */
+phy_write(phy, MII_NCONFIG, 0x1000);
+ phy_reg = phy_read(phy, MII_NCONFIG);
+
+ mode = (phy_reg  BCM5421_MODE_MASK )  5;
+
+ if ( mode == GMII_COPPER) {
+  return genmii_poll_link(phy);
+ }
+
+ /* try to find out wether we have a link */
+phy_write(phy, MII_NCONFIG, 0x2000);
+ phy_reg = phy_read(phy, MII_NCONFIG);
+
+ if ( (phy_reg  0x0020)  5 ) {
+  return 0;
+ } else {
+  return 1;
+ }
+
+}
+
+static int bcm5421_read_link(struct mii_phy*

Re: spidernet: add improved phy support in sungem_phy.c

2007-01-26 Thread Benjamin Herrenschmidt 
On Fri, 2007-01-26 at 14:07 +0100, Jens Osterkamp wrote:
 This patch adds improved version of enable_fiber for both the 5421 and
 the 5461 phy. It is now possible to specify with these wether you want
 autonegotiation or not. This is needed for bladecenter switches where
 some expect autonegotiation and some dont seem to like this at all.
 Depending on this flag it sets phy-autoneg accordingly for the fiber mode.
 
 More importantly it implements proper read_link and poll_link functions
 for both phys which can handle both copper and fiber mode by determining
 the medium first and then branching to the required functions. For fiber
 they all work fine, for copper they are not tested but return the result
 of the genmii_* function anyway which is supposed to work.
 
 A medium variable in the phy struct is introduced to save the medium
 with which the phy is currently used.
 
 The patch moves the genmii_* functions around to avoid foreward declarations.
 
 Signed-off-by: Jens Osterkamp [EMAIL PROTECTED]
 Signed-off-by: Arnd Bergmann [EMAIL PROTECTED]

Patch is whitespace damaged...

Ben.


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

Re: spidernet: add improved phy support in sungem_phy.c

2007-01-26 Thread Jens Osterkamp 
 Patch is whitespace damaged...

sending it again as attachment...

Jens
Subject: spidernet: add improved phy support

This patch adds improved version of enable_fiber for both the 5421 and
the 5461 phy. It is now possible to specify with these wether you want
autonegotiation or not. This is needed for bladecenter switches where
some expect autonegotiation and some dont seem to like this at all.
Depending on this flag it sets phy-autoneg accordingly for the fiber mode.

More importantly it implements proper read_link and poll_link functions
for both phys which can handle both copper and fiber mode by determining
the medium first and then branching to the required functions. For fiber
they all work fine, for copper they are not tested but return the result
of the genmii_* function anyway which is supposed to work.

A medium variable in the phy struct is introduced to save the medium
with which the phy is currently used.

The patch moves the genmii_* functions around to avoid foreward declarations.

Signed-off-by: Jens Osterkamp [EMAIL PROTECTED]
Signed-off-by: Arnd Bergmann [EMAIL PROTECTED]

Index: linux-2.6.20-rc5/drivers/net/sungem_phy.c
===
--- linux-2.6.20-rc5.orig/drivers/net/sungem_phy.c
+++ linux-2.6.20-rc5/drivers/net/sungem_phy.c
@@ -311,6 +311,107 @@ static int bcm5411_init(struct mii_phy* 
 	return 0;
 }
 
+static int genmii_setup_aneg(struct mii_phy *phy, u32 advertise)
+{
+	u16 ctl, adv;
+
+	phy-autoneg = 1;
+	phy-speed = SPEED_10;
+	phy-duplex = DUPLEX_HALF;
+	phy-pause = 0;
+	phy-advertising = advertise;
+
+	/* Setup standard advertise */
+	adv = phy_read(phy, MII_ADVERTISE);
+	adv = ~(ADVERTISE_ALL | ADVERTISE_100BASE4);
+	if (advertise  ADVERTISED_10baseT_Half)
+		adv |= ADVERTISE_10HALF;
+	if (advertise  ADVERTISED_10baseT_Full)
+		adv |= ADVERTISE_10FULL;
+	if (advertise  ADVERTISED_100baseT_Half)
+		adv |= ADVERTISE_100HALF;
+	if (advertise  ADVERTISED_100baseT_Full)
+		adv |= ADVERTISE_100FULL;
+	phy_write(phy, MII_ADVERTISE, adv);
+
+	/* Start/Restart aneg */
+	ctl = phy_read(phy, MII_BMCR);
+	ctl |= (BMCR_ANENABLE | BMCR_ANRESTART);
+	phy_write(phy, MII_BMCR, ctl);
+
+	return 0;
+}
+
+static int genmii_setup_forced(struct mii_phy *phy, int speed, int fd)
+{
+	u16 ctl;
+
+	phy-autoneg = 0;
+	phy-speed = speed;
+	phy-duplex = fd;
+	phy-pause = 0;
+
+	ctl = phy_read(phy, MII_BMCR);
+	ctl = ~(BMCR_FULLDPLX|BMCR_SPEED100|BMCR_ANENABLE);
+
+	/* First reset the PHY */
+	phy_write(phy, MII_BMCR, ctl | BMCR_RESET);
+
+	/* Select speed  duplex */
+	switch(speed) {
+	case SPEED_10:
+		break;
+	case SPEED_100:
+		ctl |= BMCR_SPEED100;
+		break;
+	case SPEED_1000:
+	default:
+		return -EINVAL;
+	}
+	if (fd == DUPLEX_FULL)
+		ctl |= BMCR_FULLDPLX;
+	phy_write(phy, MII_BMCR, ctl);
+
+	return 0;
+}
+
+static int genmii_poll_link(struct mii_phy *phy)
+{
+	u16 status;
+
+	(void)phy_read(phy, MII_BMSR);
+	status = phy_read(phy, MII_BMSR);
+	if ((status  BMSR_LSTATUS) == 0)
+		return 0;
+	if (phy-autoneg  !(status  BMSR_ANEGCOMPLETE))
+		return 0;
+	return 1;
+}
+
+static int genmii_read_link(struct mii_phy *phy)
+{
+	u16 lpa;
+
+	if (phy-autoneg) {
+		lpa = phy_read(phy, MII_LPA);
+
+		if (lpa  (LPA_10FULL | LPA_100FULL))
+			phy-duplex = DUPLEX_FULL;
+		else
+			phy-duplex = DUPLEX_HALF;
+		if (lpa  (LPA_100FULL | LPA_100HALF))
+			phy-speed = SPEED_100;
+		else
+			phy-speed = SPEED_10;
+		phy-pause = 0;
+	}
+	/* On non-aneg, we assume what we put in BMCR is the speed,
+	 * though magic-aneg shouldn't prevent this case from occurring
+	 */
+
+	 return 0;
+}
+
 static int generic_suspend(struct mii_phy* phy)
 {
 	phy_write(phy, MII_BMCR, BMCR_PDOWN);
@@ -365,30 +466,6 @@ static int bcm5421_init(struct mii_phy* 
 	return 0;
 }
 
-static int bcm5421_enable_fiber(struct mii_phy* phy)
-{
-	/* enable fiber mode */
-	phy_write(phy, MII_NCONFIG, 0x9020);
-	/* LEDs active in both modes, autosense prio = fiber */
-	phy_write(phy, MII_NCONFIG, 0x945f);
-
-	/* switch off fibre autoneg */
-	phy_write(phy, MII_NCONFIG, 0xfc01);
-	phy_write(phy, 0x0b, 0x0004);
-
-	return 0;
-}
-
-static int bcm5461_enable_fiber(struct mii_phy* phy)
-{
-	phy_write(phy, MII_NCONFIG, 0xfc0c);
-	phy_write(phy, MII_BMCR, 0x4140);
-	phy_write(phy, MII_NCONFIG, 0xfc0b);
-	phy_write(phy, MII_BMCR, 0x0140);
-
-	return 0;
-}
-
 static int bcm54xx_setup_aneg(struct mii_phy *phy, u32 advertise)
 {
 	u16 ctl, adv;
@@ -516,6 +593,166 @@ static int marvell88e_init(struct mi
 	return 0;
 }
 
+#define BCM5421_MODE_MASK	1  5
+
+static int bcm5421_poll_link(struct mii_phy* phy)
+{
+	u32 phy_reg;
+	int mode;
+
+	/* find out in what mode we are */
+phy_write(phy, MII_NCONFIG, 0x1000);
+	phy_reg = phy_read(phy, MII_NCONFIG);
+
+	mode = (phy_reg  BCM5421_MODE_MASK )  5;
+
+	if ( mode == GMII_COPPER) {
+		return genmii_poll_link(phy);
+	}
+
+	/* try to find out wether we have a link */
+phy_write(phy, MII_NCONFIG, 0x2000);
+	phy_reg = phy_read(phy, MII_NCONFIG);
+
+	if (

Re: spidernet: add improved phy support in sungem_phy.c

2007-01-26 Thread Francois Romieu 
Jens Osterkamp [EMAIL PROTECTED] :
  Patch is whitespace damaged...
 
 sending it again as attachment...

Inlined patches are preferred.


Index: linux-2.6.20-rc5/drivers/net/sungem_phy.c
===
--- linux-2.6.20-rc5.orig/drivers/net/sungem_phy.c
+++ linux-2.6.20-rc5/drivers/net/sungem_phy.c
@@ -311,6 +311,107 @@ static int bcm5411_init(struct mii_phy* 
[...]
+static int genmii_setup_forced(struct mii_phy *phy, int speed, int fd)
+{
+   u16 ctl;
+
+   phy-autoneg = 0;
+   phy-speed = speed;
+   phy-duplex = fd;
+   phy-pause = 0;
+
+   ctl = phy_read(phy, MII_BMCR);
+   ctl = ~(BMCR_FULLDPLX|BMCR_SPEED100|BMCR_ANENABLE);
 ^^^   ^^^
+
+   /* First reset the PHY */
+   phy_write(phy, MII_BMCR, ctl | BMCR_RESET);
+
+   /* Select speed  duplex */
+   switch(speed) {
+   case SPEED_10:
+   break;
+   case SPEED_100:
+   ctl |= BMCR_SPEED100;
+   break;
+   case SPEED_1000:
+   default:
+   return -EINVAL;
+   }
+   if (fd == DUPLEX_FULL)
+   ctl |= BMCR_FULLDPLX;
+   phy_write(phy, MII_BMCR, ctl);
+
+   return 0;
+}
+
+static int genmii_poll_link(struct mii_phy *phy)
+{
+   u16 status;
+
+   (void)phy_read(phy, MII_BMSR);
+   status = phy_read(phy, MII_BMSR);
+   if ((status  BMSR_LSTATUS) == 0)
+   return 0;
+   if (phy-autoneg  !(status  BMSR_ANEGCOMPLETE))
+   return 0;
+   return 1;
+}
+
+static int genmii_read_link(struct mii_phy *phy)
+{
+   u16 lpa;
+
+   if (phy-autoneg) {
+   lpa = phy_read(phy, MII_LPA);
+
+   if (lpa  (LPA_10FULL | LPA_100FULL))
+   phy-duplex = DUPLEX_FULL;
+   else
+   phy-duplex = DUPLEX_HALF;
+   if (lpa  (LPA_100FULL | LPA_100HALF))
+   phy-speed = SPEED_100;
+   else
+   phy-speed = SPEED_10;
+   phy-pause = 0;
+   }
+   /* On non-aneg, we assume what we put in BMCR is the speed,
+* though magic-aneg shouldn't prevent this case from occurring
+*/
+
+return 0;

You could do a quick:
if (!phy-autoneg) 
return 0;

but it would not be enough for ternary ops to fit on a single line :o/

+
 static int generic_suspend(struct mii_phy* phy)
 {
phy_write(phy, MII_BMCR, BMCR_PDOWN);
[...]
@@ -516,6 +593,166 @@ static int marvell88e_init(struct mi
return 0;
 }
 
+#define BCM5421_MODE_MASK  1  5

Please add parenthesis.

+
+static int bcm5421_poll_link(struct mii_phy* phy)
+{
+   u32 phy_reg;
+   int mode;
+
+   /* find out in what mode we are */
+phy_write(phy, MII_NCONFIG, 0x1000);
^^
+   phy_reg = phy_read(phy, MII_NCONFIG);
+
+   mode = (phy_reg  BCM5421_MODE_MASK )  5;
  ^^^

 is fine despite the lack of parenthesis above but it is error-prone.

+
+   if ( mode == GMII_COPPER) {
   ^^^
+   return genmii_poll_link(phy);
+   }

No curly-braces for single line statements please.

+
+   /* try to find out wether we have a link */
+phy_write(phy, MII_NCONFIG, 0x2000);
^^
+   phy_reg = phy_read(phy, MII_NCONFIG);
+
+   if ( (phy_reg  0x0020)  5 ) {
   ^^^ ^^^
+   return 0;
+   } else {
+   return 1;
+   }

Ternary operator ?

+
+}
+
+static int bcm5421_read_link(struct mii_phy* phy)
+{
+   u32 phy_reg;
+   int mode;
+
+   /* find out in what mode we are */
+phy_write(phy, MII_NCONFIG, 0x1000);
^^
+   phy_reg = phy_read(phy, MII_NCONFIG);
+
+   mode = (phy_reg  BCM5421_MODE_MASK )  5;
  ^^^
+
+   if ( mode == GMII_COPPER) {
   ^^^
+   return bcm54xx_read_link(phy);
+   }
+
+   phy-speed = SPEED_1000;
+
+   /* find out wether we are running half- or full duplex */
+phy_write(phy, MII_NCONFIG, 0x2000);
^^
+   phy_reg = phy_read(phy, MII_NCONFIG);
+
+   if ( (phy_reg  0x0020)  7 ) {
   ^^^ ^^^
+   phy-duplex |=  DUPLEX_HALF;
+   } else {
+   phy-duplex |=  DUPLEX_FULL;
+   }

Ternary operator ?

+
+   return 0;
+}
+
[...]
+#define BCM5461_FIBER_LINK 1  2
+#define BCM5461_MODE_MASK  3  1

Please add parenthesis.

+
+static int bcm5461_poll_link(struct mii_phy* phy)
+{
+   u32 phy_reg;
+   int mode;
+
+   /* find out in what mode we are */
+phy_write(phy, MII_NCONFIG, 0x7c00);
^^
+   phy_reg = phy_read(phy, MII_NCONFIG);
+
+   mode = (phy_reg  BCM5461_MODE_MASK )  1;
  ^^^
+
+   if ( mode == GMII_COPPER) {