Re: [PATCH] [v3] spidernet : add improved phy support

2007-02-12 Thread Linas Vepstas

Tested the patch, it works for me.  Thus I'll attach a pre-emptive 
Acked-by: Linas Vepstas <[EMAIL PROTECTED]>

However, some quibbbles, which I think would be nice to see fixed:

On Mon, Feb 12, 2007 at 09:35:34PM +0100, Jens Osterkamp wrote:
> 
> Index: linux-2.6.20/drivers/net/sungem_phy.c
> ===
> --- linux-2.6.20.orig/drivers/net/sungem_phy.c
> +++ linux-2.6.20/drivers/net/sungem_phy.c
>  
> +#define BCM5421_MODE_MASK(1 << 5)

Customary practice is to have these in the heder file ... 

> + mode = (phy_reg & BCM5421_MODE_MASK) >> 5;
> +
> + if ( mode == BCM54XX_COPPER)

All this shifting makes the code hard to read and 
hard to verify for correctness.  Part of the problem 
seems to be that you are trying to re-cycle the 
BCM5421_COPPER and BCM5461_COPPER which are in 
different locations.

It would have been clearer to simply have

#define BCM5421_MODE_MASK  (1 << 5)
#define BCM5421_COPPER 0

if (phy_reg & BCM5421_MODE_MASK == BCM5421_COPPER)

> + if ( (phy_reg & 0x0080) >> 7)

There is no need for the shift. The if statement is 
just as true (or false) with or without the shift.

> +#define BCM5461_FIBER_LINK   (1 << 2)
> +#define BCM5461_MODE_MASK(3 << 1)
> +
> + mode = (phy_reg & BCM5461_MODE_MASK ) >> 1;

More confusing shifting 

> +enum {
> + BCM54XX_COPPER,
> + BCM54XX_FIBER,
> + BCM54XX_GBIC,
> + BCM54XX_SGMII,

Things that are being used for bitmak tests should probably
not be declared in an enum. For me, at least, there's a 
cognitive dissonance in doing things this way.

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


Re: [PATCH] [v3] spidernet : add improved phy support

2007-02-12 Thread Benjamin Herrenschmidt

> +static int bcm5461_enable_fiber(struct mii_phy* phy, int autoneg)
> +{
> + /* select fiber mode, enable 1000 base-X registers */
> + phy_write(phy, MII_NCONFIG, 0xfc0b);
> +
> + if (autoneg) {
> + /* enable fiber with no autonegotiation */
> + phy_write(phy, MII_ADVERTISE, 0x01e0);
> + phy_write(phy, MII_BMCR, 0x1140);
> + } else {
> + /* enable fiber with autonegotiation */
> + phy_write(phy, MII_BMCR, 0x0140);
> + }
> +
> + phy->autoneg = autoneg;
> +
> + return 0;
> +}

Something is backward... either the code or the comments...

Also, I dislike the autoneg bits without using any constants. Why not
use the normal setup_aneg() ? I think what is needed is a set_link_mode
or something like that that takes (fiber/copper) as an argument (or
better, use the proper names as documented by the chip).

Doesn't matter to much right now, if the code works. Have you had a
chance to check you don't break G5s with 5421 sungem ? If yes, then the
patch is good to go for now but we should revisit and/or try to merge
that with the generic PHY layer at one point.

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


[PATCH] [v3] spidernet : add improved phy support

2007-02-12 Thread Jens Osterkamp
This version moves the medium variable to the card specific structure and
changes the GMII_* to BCM54XX_* #defines.

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.

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/drivers/net/sungem_phy.c
===
--- linux-2.6.20.orig/drivers/net/sungem_phy.c
+++ linux-2.6.20/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 *ph