sumgem problems with initializiation order at a 1000SX SERDES card

sumgem problems with initializiation order at a 1000SX SERDES card

[Hermann Lauer]

Hello,
the current sungem driver sets up the phy layer when insmoding the
driver, and later resets the gem chip in gem_reinit_chip(), called
from gem_do_start() which is called when a "ifconfig up" is done.

Unfortunately with the sun PCI FIBER 1000SX-card (which uses a SERDES
connected to the gem PCS part) this resets the
gem included PCS part too, which then cancels the previously established 
link (see debug output below).

Any idea how that could be solved ?

Thanks,
  Hermann

PS: There is only one "writeb" call in the code to reset the PCS, but the 
value is read with a readl. Typo ?

----------------------------------------------------------------
Some PCS registers before and after gem_reset(gp) is executed in 
gem_reinit_chip().

eth2: PCS_MIISTAT:a 12c, pcs_lpa=4120, PCS_MIIADV: 1e0
eth2: PCS_MIISTAT:b 108, pcs_lpa=0, PCS_MIIADV: e0

Note that e0 is the default PCS_MIIADV register value according to the docs.
  
-- 
Netzwerkadministration/Zentrale Dienste, Interdiziplinaeres 
Zentrum fuer wissenschaftliches Rechnen der Universitaet Heidelberg
IWR; INF 368; 69120 Heidelberg; Tel: (06221)54-8236 Fax: -5224
Email: Hermann.Lauer@iwr.uni-heidelberg.de

Re: sumgem problems with initializiation order at a 1000SX SERDES card

[David Miller]

From: Hermann Lauer <Hermann.Lauer@iwr.uni-heidelberg.de>
Date: Tue, 25 Nov 2008 11:30:29 +0100

> PS: There is only one "writeb" call in the code to reset the PCS, but the 
> value is read with a readl. Typo ?

That is definitely a bug, which I've fixed as follows.

I don't know about the rest of your report at this point, sorry.

sungem: Fix PCS_MIICTRL register write in gem_init_phy().

Use writel not writeb.

Noticed by Hermann Lauer.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 drivers/net/sungem.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/sungem.c b/drivers/net/sungem.c
index 4291458..1349e41 100644
--- a/drivers/net/sungem.c
+++ b/drivers/net/sungem.c
@@ -1714,7 +1714,7 @@ static void gem_init_phy(struct gem *gp)
 		/* Reset PCS unit. */
 		val = readl(gp->regs + PCS_MIICTRL);
 		val |= PCS_MIICTRL_RST;
-		writeb(val, gp->regs + PCS_MIICTRL);
+		writel(val, gp->regs + PCS_MIICTRL);
 
 		limit = 32;
 		while (readl(gp->regs + PCS_MIICTRL) & PCS_MIICTRL_RST) {
-- 
1.5.6.5

Re: sumgem problems with initializiation order at a 1000SX SERDES card [experimental patch]

[Hermann Lauer]

[-- Attachment #1: Type: text/plain, Size: 674 bytes --]

Attached is an experimental patch which made my 1000SX sungem card (with phy_serdes) working.
As suspected, reinitialising the PCS part after the global reset in gem_reinit_chip()
gets autonegotiation working and the link is established also after an ifconfig up
command now. Access through the card did not show any errors so far.

Will try to add the missing if clauses to the patch as time allows me, comments are welcome.

Hermann

-- 
Netzwerkadministration/Zentrale Dienste, Interdiziplinaeres 
Zentrum fuer wissenschaftliches Rechnen der Universitaet Heidelberg
IWR; INF 368; 69120 Heidelberg; Tel: (06221)54-8236 Fax: -5224
Email: Hermann.Lauer@iwr.uni-heidelberg.de

[-- Attachment #2: sungem-serdes.patch --]
[-- Type: text/plain, Size: 2351 bytes --]

--- linux-2.6.24-2.6.24/drivers/net/sungem.c.2627	2008-11-28 00:40:58.000000000 +0100
+++ linux-2.6.24-2.6.24/drivers/net/sungem.c	2008-11-28 00:34:46.000000000 +0100
@@ -1324,7 +1324,7 @@
 	    	   gp->phy_type == phy_serdes) {
 		u32 pcs_lpa = readl(gp->regs + PCS_MIILP);
 
-		if (pcs_lpa & PCS_MIIADV_FD)
+		if ((pcs_lpa & PCS_MIIADV_FD) || gp->phy_type == phy_serdes)
 			full_duplex = 1;
 		speed = SPEED_1000;
 	}
@@ -1488,6 +1488,9 @@
 			val = readl(gp->regs + PCS_MIISTAT);
 
 		if ((val & PCS_MIISTAT_LS) != 0) {
+		  if (gp->lstate == link_up)
+			 goto restart;
+
 			gp->lstate = link_up;
 			netif_carrier_on(gp->dev);
 			(void)gem_set_link_modes(gp);
@@ -1714,7 +1717,7 @@
 		/* Reset PCS unit. */
 		val = readl(gp->regs + PCS_MIICTRL);
 		val |= PCS_MIICTRL_RST;
-		writeb(val, gp->regs + PCS_MIICTRL);
+		writel(val, gp->regs + PCS_MIICTRL);
 
 		limit = 32;
 		while (readl(gp->regs + PCS_MIICTRL) & PCS_MIICTRL_RST) {
@@ -2095,12 +2098,51 @@
 /* Must be invoked under gp->lock and gp->tx_lock. */
 static void gem_reinit_chip(struct gem *gp)
 {
+  int val;
 	/* Reset the chip */
 	gem_reset(gp);
 
 	/* Make sure ints are disabled */
 	gem_disable_ints(gp);
 
+	/* Make sure PCS is disabled while changing advertisement
+	 * configuration.
+	 */
+	val = readl(gp->regs + PCS_CFG);
+	val &= ~(PCS_CFG_ENABLE | PCS_CFG_TO);
+	writel(val, gp->regs + PCS_CFG);
+
+	/* Advertise all capabilities except assymetric
+	 * pause.
+	 */
+	val = readl(gp->regs + PCS_MIIADV);
+	val |= (PCS_MIIADV_FD | PCS_MIIADV_HD |
+		PCS_MIIADV_SP | PCS_MIIADV_AP);
+	writel(val, gp->regs + PCS_MIIADV);
+
+	/* Enable and restart auto-negotiation, disable wrapback/loopback,
+	 * and re-enable PCS.
+	 */
+	val = readl(gp->regs + PCS_MIICTRL);
+	val |= (PCS_MIICTRL_RAN | PCS_MIICTRL_ANE);
+	val &= ~PCS_MIICTRL_WB;
+	writel(val, gp->regs + PCS_MIICTRL);
+
+	val = readl(gp->regs + PCS_CFG);
+	val |= PCS_CFG_ENABLE;
+	writel(val, gp->regs + PCS_CFG);
+
+	/* Make sure serialink loopback is off.  The meaning
+	 * of this bit is logically inverted based upon whether
+	 * you are in Serialink or SERDES mode.
+	 */
+	val = readl(gp->regs + PCS_SCTRL);
+	if (gp->phy_type == phy_serialink)
+	  val &= ~PCS_SCTRL_LOOP;
+	else
+	  val |= PCS_SCTRL_LOOP;
+	writel(val, gp->regs + PCS_SCTRL);
+
 	/* Allocate & setup ring buffers */
 	gem_init_rings(gp);
 

Re: sumgem problems with initializiation order at a 1000SX SERDES card [experimental patch]

[David Miller]

From: Hermann Lauer <Hermann.Lauer@iwr.uni-heidelberg.de>
Date: Fri, 28 Nov 2008 01:00:59 +0100

> Attached is an experimental patch which made my 1000SX sungem card (with phy_serdes) working.
> As suspected, reinitialising the PCS part after the global reset in gem_reinit_chip()
> gets autonegotiation working and the link is established also after an ifconfig up
> command now. Access through the card did not show any errors so far.
> 
> Will try to add the missing if clauses to the patch as time allows me, comments are welcome.

Thanks a lot for doing this work and tracking down the problem.

I'll take a look at your patch as soon as I can.

Thanks again.

[patch] sumgem: improve ethtool output with fibre card using the internal pcs and a serdes

[Hermann Lauer]

[-- Attachment #1: Type: text/plain, Size: 1943 bytes --]

Attached is a patch which improves the output of ethtool (see below) to
some sensefull values with a sungem fibre card which uses the 
sungem interal pcs connected to a serdes chip. The seriallink case
in the driver is untouched.

Most values are hardcoded, because gigabit fibre autoneg is anyways
limited and the driver don't really support much at the moment
with that hardware.

Please consider for inclusion or comment.
Thanks,
  Hermann

patched# ethtool eth2
Settings for eth2:
        Supported ports: [ FIBRE ]
        Supported link modes:   1000baseT/Half 1000baseT/Full
        Supports auto-negotiation: Yes
        Advertised link modes:  10baseT/Half 10baseT/Full
                                100baseT/Half 100baseT/Full
        Advertised auto-negotiation: Yes
        Speed: 1000Mb/s
        Duplex: Half
        Port: FIBRE
        PHYAD: 0
        Transceiver: internal
        Auto-negotiation: off
        Supports Wake-on: d
        Wake-on: d
        Current message level: 0x00000007 (7)
        Link detected: yes

unpatched# ethtool eth2
Settings for eth2:
        Supported ports: [ ]
        Supported link modes:   10baseT/Half 10baseT/Full
                                100baseT/Half 100baseT/Full
        Supports auto-negotiation: Yes
        Advertised link modes:  10baseT/Half 10baseT/Full
                                100baseT/Half 100baseT/Full
        Advertised auto-negotiation: Yes
        Speed: Unknown! (0)
        Duplex: Half
        Port: Twisted Pair
        PHYAD: 0
        Transceiver: internal
        Auto-negotiation: off
        Supports Wake-on: d
        Wake-on: d
        Current message level: 0x00000007 (7)
        Link detected: yes

-- 
Netzwerkadministration/Zentrale Dienste, Interdiziplinaeres 
Zentrum fuer wissenschaftliches Rechnen der Universitaet Heidelberg
IWR; INF 368; 69120 Heidelberg; Tel: (06221)54-8236 Fax: -5224
Email: Hermann.Lauer@iwr.uni-heidelberg.de

[-- Attachment #2: sungem-get_settings.patch --]
[-- Type: text/plain, Size: 921 bytes --]

--- linux-2.6.27.7/drivers/net/sungem.c.orig	2008-11-27 18:20:26.000000000 +0100
+++ linux-2.6.27.7/drivers/net/sungem.c	2008-12-04 08:17:28.000000000 +0100
@@ -2680,6 +2680,21 @@ static int gem_get_settings(struct net_d
 		cmd->speed = 0;
 		cmd->duplex = cmd->port = cmd->phy_address =
 			cmd->transceiver = cmd->autoneg = 0;
+
+		/* serdes means usually a Fibre connector, with most fixed */
+		if (gp->phy_type == phy_serdes) {
+			cmd->port = PORT_FIBRE;
+			cmd->supported = (SUPPORTED_1000baseT_Half |
+				SUPPORTED_1000baseT_Full |
+				SUPPORTED_FIBRE | SUPPORTED_Autoneg |
+				SUPPORTED_Pause | SUPPORTED_Asym_Pause);
+			cmd->advertising = cmd->supported;
+			cmd->transceiver = XCVR_INTERNAL;
+			if (gp->lstate == link_up)
+				cmd->speed = SPEED_1000;
+			cmd->duplex = DUPLEX_FULL;
+			cmd->autoneg = 1;
+		}
 	}
 	cmd->maxtxpkt = cmd->maxrxpkt = 0;
 
Signed-off-by: Hermann.Lauer@iwr.uni-heidelberg.de

Re: sumgem problems with initializiation order at a 1000SX SERDES card [experimental patch]

[David Miller]

From: Hermann Lauer <Hermann.Lauer@iwr.uni-heidelberg.de>
Date: Fri, 28 Nov 2008 01:00:59 +0100

> Will try to add the missing if clauses to the patch as time allows
> me, comments are welcome.

Again, thanks for doing this work.

We don't want to run this code on MDIO phys, so at least a test
is needed to guard this new code.

And this new code is just a copy of the PHY init sequence on
PCS, so we should split that out into a helper function instead
of duplicating it.

Any time gem_reset() is invoked, the PCS is going to need to be
reset like this, so let's do the advertisement reinit there.

Does this version of the fix work for you?

diff --git a/drivers/net/sungem.c b/drivers/net/sungem.c
index 1349e41..bb7a478 100644
--- a/drivers/net/sungem.c
+++ b/drivers/net/sungem.c
@@ -1142,6 +1142,70 @@ static int gem_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	return NETDEV_TX_OK;
 }
 
+static void gem_pcs_reset(struct gem *gp)
+{
+	int limit;
+	u32 val;
+
+	/* Reset PCS unit. */
+	val = readl(gp->regs + PCS_MIICTRL);
+	val |= PCS_MIICTRL_RST;
+	writel(val, gp->regs + PCS_MIICTRL);
+
+	limit = 32;
+	while (readl(gp->regs + PCS_MIICTRL) & PCS_MIICTRL_RST) {
+		udelay(100);
+		if (limit-- <= 0)
+			break;
+	}
+	if (limit <= 0)
+		printk(KERN_WARNING "%s: PCS reset bit would not clear.\n",
+		       gp->dev->name);
+}
+
+static void gem_pcs_reinit_adv(struct gem *gp)
+{
+	u32 val;
+
+	/* Make sure PCS is disabled while changing advertisement
+	 * configuration.
+	 */
+	val = readl(gp->regs + PCS_CFG);
+	val &= ~(PCS_CFG_ENABLE | PCS_CFG_TO);
+	writel(val, gp->regs + PCS_CFG);
+
+	/* Advertise all capabilities except assymetric
+	 * pause.
+	 */
+	val = readl(gp->regs + PCS_MIIADV);
+	val |= (PCS_MIIADV_FD | PCS_MIIADV_HD |
+		PCS_MIIADV_SP | PCS_MIIADV_AP);
+	writel(val, gp->regs + PCS_MIIADV);
+
+	/* Enable and restart auto-negotiation, disable wrapback/loopback,
+	 * and re-enable PCS.
+	 */
+	val = readl(gp->regs + PCS_MIICTRL);
+	val |= (PCS_MIICTRL_RAN | PCS_MIICTRL_ANE);
+	val &= ~PCS_MIICTRL_WB;
+	writel(val, gp->regs + PCS_MIICTRL);
+
+	val = readl(gp->regs + PCS_CFG);
+	val |= PCS_CFG_ENABLE;
+	writel(val, gp->regs + PCS_CFG);
+
+	/* Make sure serialink loopback is off.  The meaning
+	 * of this bit is logically inverted based upon whether
+	 * you are in Serialink or SERDES mode.
+	 */
+	val = readl(gp->regs + PCS_SCTRL);
+	if (gp->phy_type == phy_serialink)
+		val &= ~PCS_SCTRL_LOOP;
+	else
+		val |= PCS_SCTRL_LOOP;
+	writel(val, gp->regs + PCS_SCTRL);
+}
+
 #define STOP_TRIES 32
 
 /* Must be invoked under gp->lock and gp->tx_lock. */
@@ -1168,6 +1232,9 @@ static void gem_reset(struct gem *gp)
 
 	if (limit <= 0)
 		printk(KERN_ERR "%s: SW reset is ghetto.\n", gp->dev->name);
+
+	if (gp->phy_type == phy_serialink || gp->phy_type == phy_serdes)
+		gem_pcs_reinit_adv(gp);
 }
 
 /* Must be invoked under gp->lock and gp->tx_lock. */
@@ -1324,7 +1391,7 @@ static int gem_set_link_modes(struct gem *gp)
 	    	   gp->phy_type == phy_serdes) {
 		u32 pcs_lpa = readl(gp->regs + PCS_MIILP);
 
-		if (pcs_lpa & PCS_MIIADV_FD)
+		if ((pcs_lpa & PCS_MIIADV_FD) || gp->phy_type == phy_serdes)
 			full_duplex = 1;
 		speed = SPEED_1000;
 	}
@@ -1488,6 +1555,9 @@ static void gem_link_timer(unsigned long data)
 			val = readl(gp->regs + PCS_MIISTAT);
 
 		if ((val & PCS_MIISTAT_LS) != 0) {
+			if (gp->lstate == link_up)
+				goto restart;
+
 			gp->lstate = link_up;
 			netif_carrier_on(gp->dev);
 			(void)gem_set_link_modes(gp);
@@ -1708,61 +1778,8 @@ static void gem_init_phy(struct gem *gp)
 		if (gp->phy_mii.def && gp->phy_mii.def->ops->init)
 			gp->phy_mii.def->ops->init(&gp->phy_mii);
 	} else {
-		u32 val;
-		int limit;
-
-		/* Reset PCS unit. */
-		val = readl(gp->regs + PCS_MIICTRL);
-		val |= PCS_MIICTRL_RST;
-		writel(val, gp->regs + PCS_MIICTRL);
-
-		limit = 32;
-		while (readl(gp->regs + PCS_MIICTRL) & PCS_MIICTRL_RST) {
-			udelay(100);
-			if (limit-- <= 0)
-				break;
-		}
-		if (limit <= 0)
-			printk(KERN_WARNING "%s: PCS reset bit would not clear.\n",
-			       gp->dev->name);
-
-		/* Make sure PCS is disabled while changing advertisement
-		 * configuration.
-		 */
-		val = readl(gp->regs + PCS_CFG);
-		val &= ~(PCS_CFG_ENABLE | PCS_CFG_TO);
-		writel(val, gp->regs + PCS_CFG);
-
-		/* Advertise all capabilities except assymetric
-		 * pause.
-		 */
-		val = readl(gp->regs + PCS_MIIADV);
-		val |= (PCS_MIIADV_FD | PCS_MIIADV_HD |
-			PCS_MIIADV_SP | PCS_MIIADV_AP);
-		writel(val, gp->regs + PCS_MIIADV);
-
-		/* Enable and restart auto-negotiation, disable wrapback/loopback,
-		 * and re-enable PCS.
-		 */
-		val = readl(gp->regs + PCS_MIICTRL);
-		val |= (PCS_MIICTRL_RAN | PCS_MIICTRL_ANE);
-		val &= ~PCS_MIICTRL_WB;
-		writel(val, gp->regs + PCS_MIICTRL);
-
-		val = readl(gp->regs + PCS_CFG);
-		val |= PCS_CFG_ENABLE;
-		writel(val, gp->regs + PCS_CFG);
-
-		/* Make sure serialink loopback is off.  The meaning
-		 * of this bit is logically inverted based upon whether
-		 * you are in Serialink or SERDES mode.
-		 */
-		val = readl(gp->regs + PCS_SCTRL);
-		if (gp->phy_type == phy_serialink)
-			val &= ~PCS_SCTRL_LOOP;
-		else
-			val |= PCS_SCTRL_LOOP;
-		writel(val, gp->regs + PCS_SCTRL);
+		gem_pcs_reset(gp);
+		gem_pcs_reinit_adv(gp);
 	}
 
 	/* Default aneg parameters */

Re: sumgem problems with initializiation order at a 1000SX SERDES card [experimental patch]

[Hermann Lauer]

On Thu, Dec 04, 2008 at 05:33:24PM -0800, David Miller wrote:

> And this new code is just a copy of the PHY init sequence on
> PCS, so we should split that out into a helper function instead
> of duplicating it.
> 
> Any time gem_reset() is invoked, the PCS is going to need to be
> reset like this, so let's do the advertisement reinit there.
> 
> Does this version of the fix work for you?

Yes, works the same. It has some delay before receiving packets works
after an ifconfig up or a reestablishing of the link, but thats
the same as with the experimental patch.
Probably the link state handling has to be improved, I'm not
shure my experimental patch was correct there.

But as the driver is working now with this patch on this hardware
and not without - please commit.

Thanks,
  Hermann

-- 
Netzwerkadministration/Zentrale Dienste, Interdiziplinaeres 
Zentrum fuer wissenschaftliches Rechnen der Universitaet Heidelberg
IWR; INF 368; 69120 Heidelberg; Tel: (06221)54-8236 Fax: -5224
Email: Hermann.Lauer@iwr.uni-heidelberg.de

Re: sumgem problems with initializiation order at a 1000SX SERDES card [experimental patch]

[David Miller]

From: Hermann Lauer <Hermann.Lauer@iwr.uni-heidelberg.de>
Date: Mon, 8 Dec 2008 16:59:36 +0100

> On Thu, Dec 04, 2008 at 05:33:24PM -0800, David Miller wrote:
> 
> > And this new code is just a copy of the PHY init sequence on
> > PCS, so we should split that out into a helper function instead
> > of duplicating it.
> > 
> > Any time gem_reset() is invoked, the PCS is going to need to be
> > reset like this, so let's do the advertisement reinit there.
> > 
> > Does this version of the fix work for you?
> 
> Yes, works the same. It has some delay before receiving packets works
> after an ifconfig up or a reestablishing of the link, but thats
> the same as with the experimental patch.
> Probably the link state handling has to be improved, I'm not
> shure my experimental patch was correct there.
> 
> But as the driver is working now with this patch on this hardware
> and not without - please commit.

I agree, and that's what I'll do.

Thanks for testing.

Re: [patch] sumgem: improve ethtool output with fibre card using the internal pcs and a serdes

[David Miller]

From: Hermann Lauer <Hermann.Lauer@iwr.uni-heidelberg.de>
Date: Thu, 4 Dec 2008 12:34:34 +0100

> Attached is a patch which improves the output of ethtool (see below) to
> some sensefull values with a sungem fibre card which uses the 
> sungem interal pcs connected to a serdes chip. The seriallink case
> in the driver is untouched.
> 
> Most values are hardcoded, because gigabit fibre autoneg is anyways
> limited and the driver don't really support much at the moment
> with that hardware.
> 
> Please consider for inclusion or comment.
> Thanks,
>   Hermann

Patch applied, thanks Hermann.