Patchwork gianfar: Omit TBI auto-negotiation based on device tree

login
register
mail settings
Submitter Nate Case
Date Oct. 28, 2008, 10:53 p.m.
Message ID <1225234382-23050-1-git-send-email-ncase@xes-inc.com>
Download mbox | patch
Permalink /patch/6171/
State Superseded, archived
Headers show

Comments

Nate Case - Oct. 28, 2008, 10:53 p.m.
Some SGMII PHYs don't auto-negotiate correctly with the TBI+SerDes
interface on the mpc85xx processors.  Check for the "sgmii-aneg-disable"
device tree flag and skip enabling auto-negotiation on the TBI
side if present.  Full duplex 1000 Mbit/s will be assumed for the
SGMII link to the PHY (note that this does not affect the link speed
on the external side of the external PHY).

Signed-off-by: Nate Case <ncase@xes-inc.com>
---
I'm submitting this to linuxppc-dev and netdev, though I'm not sure
which tree it should go through.  It touches network driver code
and some Freescale arch code, all of which is maintained by Kumar.

 Documentation/powerpc/dts-bindings/fsl/tsec.txt |    3 +++
 arch/powerpc/sysdev/fsl_soc.c                   |    4 ++++
 drivers/net/gianfar.c                           |   21 +++++++++++++++++++--
 drivers/net/gianfar.h                           |    3 ++-
 include/linux/fsl_devices.h                     |    1 +
 5 files changed, 29 insertions(+), 3 deletions(-)
Trent Piepho - Oct. 31, 2008, 1:07 a.m.
On Tue, 28 Oct 2008, Nate Case wrote:
> Some SGMII PHYs don't auto-negotiate correctly with the TBI+SerDes
> interface on the mpc85xx processors.  Check for the "sgmii-aneg-disable"
> device tree flag and skip enabling auto-negotiation on the TBI
> side if present.  Full duplex 1000 Mbit/s will be assumed for the
> SGMII link to the PHY (note that this does not affect the link speed
> on the external side of the external PHY).

Note that there is a race in the tbi/serdes setup code.  The writes to the
TBI/SerDes with gfar_local_mdio_write() use the same MDIO bus registers as
phylib uses to talk to the real phy or phys.  There is no locking for
gfar_local_mdio vs phylib so they can (and will) clobber each other.

It doesn't usually happen, due to luck and general phylib slowness.  But I've
got some patches in 2.6.28 that speed up phylib and might makes this happen
more often...

But more relevant to your serdes problem, I also have a patch that prevents
restarting serdes auto-negotiation if the serdes link is already up.  My SGMII
PHY will auto-negotiate, but it takes about 3 seconds.  Avoiding an
unnecessary 3 second auto-negotiation when the gianfar device is opened lets
me cut my power-on to DHCP completion time in half.

I wonder if this would also fix your problem, without needing to add the extra
workaround?
Nate Case - Nov. 3, 2008, 6:55 p.m.
On Thu, 2008-10-30 at 18:07 -0700, Trent Piepho wrote:
> But more relevant to your serdes problem, I also have a patch that
> prevents
> restarting serdes auto-negotiation if the serdes link is already up.
> My SGMII
> PHY will auto-negotiate, but it takes about 3 seconds.  Avoiding an
> unnecessary 3 second auto-negotiation when the gianfar device is
> opened lets
> me cut my power-on to DHCP completion time in half.
> 
> I wonder if this would also fix your problem, without needing to add
> the extra
> workaround?

I just verified that your patch solves my problem without the need for
my workaround.  So at this point, it looks like we can drop this patch
("Omit TBI auto-negotiation based on device tree").

I tested when booting the kernel in U-Boot both via both TFTP and flash
(I was worried that your patch may only fix things for the TFTP boot
case, but that wasn't the case fortunately).

Thanks for the patch!
Kumar Gala - Nov. 3, 2008, 8:38 p.m.
On Nov 3, 2008, at 12:55 PM, Nate Case wrote:

> On Thu, 2008-10-30 at 18:07 -0700, Trent Piepho wrote:
>> But more relevant to your serdes problem, I also have a patch that
>> prevents
>> restarting serdes auto-negotiation if the serdes link is already up.
>> My SGMII
>> PHY will auto-negotiate, but it takes about 3 seconds.  Avoiding an
>> unnecessary 3 second auto-negotiation when the gianfar device is
>> opened lets
>> me cut my power-on to DHCP completion time in half.
>>
>> I wonder if this would also fix your problem, without needing to add
>> the extra
>> workaround?
>
> I just verified that your patch solves my problem without the need for
> my workaround.  So at this point, it looks like we can drop this patch
> ("Omit TBI auto-negotiation based on device tree").
>
> I tested when booting the kernel in U-Boot both via both TFTP and  
> flash
> (I was worried that your patch may only fix things for the TFTP boot
> case, but that wasn't the case fortunately).
>
> Thanks for the patch!

Ok, marked Nate's patch as superseded in patchworks.

- k

Patch

diff --git a/Documentation/powerpc/dts-bindings/fsl/tsec.txt b/Documentation/powerpc/dts-bindings/fsl/tsec.txt
index cf55fa4..ad0633c 100644
--- a/Documentation/powerpc/dts-bindings/fsl/tsec.txt
+++ b/Documentation/powerpc/dts-bindings/fsl/tsec.txt
@@ -48,6 +48,9 @@  Properties:
     hardware.
   - fsl,magic-packet : If present, indicates that the hardware supports
     waking up via magic packet.
+  - sgmii-aneg-disable : If present, indicates that the device's SGMII
+    auto-negotiation should be disabled.  This may be necessary on boards
+    with PHYs that are unable to auto-negotiate with the MAC.
 
 Example:
 	ethernet@24000 {
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 01b884b..5321aa4 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -355,6 +355,10 @@  static int __init gfar_of_init(void)
 		if (of_get_property(np, "fsl,magic-packet", NULL))
 			gfar_data.device_flags |= FSL_GIANFAR_DEV_HAS_MAGIC_PACKET;
 
+		if (of_get_property(np, "sgmii-aneg-disable", NULL))
+			gfar_data.board_flags |=
+				FSL_GIANFAR_BRD_SGMII_ANEG_DIS;
+
 		ph = of_get_property(np, "phy-handle", NULL);
 		if (ph == NULL) {
 			u32 *fixed_link;
diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 64b2011..0a1c22f 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -335,6 +335,15 @@  static int gfar_probe(struct platform_device *pdev)
 	if (dev->features & NETIF_F_IP_CSUM)
 		dev->hard_header_len += GMAC_FCB_LEN;
 
+	/*
+	 * Some SGMII PHYs don't auto-negotiate correctly with the
+	 * TBI+SerDes interface.
+	 */
+	if (priv->einfo->board_flags & FSL_GIANFAR_BRD_SGMII_ANEG_DIS)
+		priv->tbi_aneg_enable = 0;
+	else
+		priv->tbi_aneg_enable = 1;
+
 	priv->rx_buffer_size = DEFAULT_RX_BUFFER_SIZE;
 	priv->tx_ring_size = DEFAULT_TX_RING_SIZE;
 	priv->rx_ring_size = DEFAULT_RX_RING_SIZE;
@@ -586,6 +595,7 @@  static void gfar_configure_serdes(struct net_device *dev)
 	struct gfar_mii __iomem *regs =
 			(void __iomem *)&priv->regs->gfar_mii_regs;
 	int tbipa = gfar_read(&priv->regs->tbipa);
+	u16 bmcr_val;
 
 	/* Single clk mode, mii mode off(for serdes communication) */
 	gfar_local_mdio_write(regs, tbipa, MII_TBICON, TBICON_CLK_SELECT);
@@ -594,8 +604,15 @@  static void gfar_configure_serdes(struct net_device *dev)
 			ADVERTISE_1000XFULL | ADVERTISE_1000XPAUSE |
 			ADVERTISE_1000XPSE_ASYM);
 
-	gfar_local_mdio_write(regs, tbipa, MII_BMCR, BMCR_ANENABLE |
-			BMCR_ANRESTART | BMCR_FULLDPLX | BMCR_SPEED1000);
+	if (priv->tbi_aneg_enable)
+		/* ANEG enable, restart ANEG, full duplex mode, speed[1] set */
+		bmcr_val = BMCR_ANENABLE | BMCR_ANRESTART | BMCR_FULLDPLX |
+			   BMCR_SPEED1000;
+	else
+		/* ANEG disabled, full duplex, speed[1] set */
+		bmcr_val = BMCR_FULLDPLX | BMCR_SPEED1000;
+
+	gfar_local_mdio_write(regs, tbipa, MII_BMCR, bmcr_val);
 }
 
 static void init_registers(struct net_device *dev)
diff --git a/drivers/net/gianfar.h b/drivers/net/gianfar.h
index f46e9b6..aa485da 100644
--- a/drivers/net/gianfar.h
+++ b/drivers/net/gianfar.h
@@ -737,7 +737,8 @@  struct gfar_private {
 		rx_csum_enable:1,
 		extended_hash:1,
 		bd_stash_en:1,
-		wol_en:1; /* Wake-on-LAN enabled */
+		wol_en:1, /* Wake-on-LAN enabled */
+		tbi_aneg_enable:1;
 	unsigned short padding;
 
 	unsigned int interruptTransmit;
diff --git a/include/linux/fsl_devices.h b/include/linux/fsl_devices.h
index 4e625e0..f516dbc 100644
--- a/include/linux/fsl_devices.h
+++ b/include/linux/fsl_devices.h
@@ -74,6 +74,7 @@  struct gianfar_mdio_data {
 /* Flags in gianfar_platform_data */
 #define FSL_GIANFAR_BRD_HAS_PHY_INTR	0x00000001 /* set or use a timer */
 #define FSL_GIANFAR_BRD_IS_REDUCED	0x00000002 /* Set if RGMII, RMII */
+#define FSL_GIANFAR_BRD_SGMII_ANEG_DIS	0x00000004 /* SGMII PHY w/o ANEG */
 
 struct fsl_i2c_platform_data {
 	/* device specific information */