[PATCH] gianfar: Omit TBI auto-negotiation based on device tree

[PATCH] gianfar: Omit TBI auto-negotiation based on device tree

[Nate Case]

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

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 */
-- 
1.6.0.2

Re: [PATCH] gianfar: Omit TBI auto-negotiation based on device tree

[Trent Piepho]

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?

[PATCH 1/2] gianfar: Fix race in TBI/SerDes configuration

[Trent Piepho]

The init_phy() function attaches to the PHY, then configures the
SerDes<->TBI link (in SGMII mode).  The TBI is on the MDIO bus with the PHY
(sort of) and is accessed via the gianfar's MDIO registers, using the
functions gfar_local_mdio_read/write(), which don't do any locking.

The previously attached PHY will start a work-queue on a timer, and
probably an irq handler as well, which will talk to the PHY and thus use
the MDIO bus.  This uses phy_read/write(), which have locking, but not
against the gfar_local_mdio versions.

The result is that PHY code will try to use the MDIO bus at the same time
as the SerDes setup code, corrupting the transfers.

Setting up the SerDes before attaching to the PHY will insure that there is
no race between the SerDes code and *our* PHY, but doesn't fix everything.
Typically the PHYs for all gianfar devices are on the same MDIO bus, which
is associated with the first gianfar device.  This means that the first
gianfar's SerDes code could corrupt the MDIO transfers for a different
gianfar's PHY.

The lock used by phy_read/write() is contained in the mii_bus structure,
which is pointed to by the PHY.  This is difficult to access from the
gianfar drivers, as there is no link between a gianfar device and the
mii_bus which shares the same MDIO registers.  As far as the device layer
and drivers are concerned they are two unrelated devices (which happen to
share registers).

Generally all gianfar devices' PHYs will be on the bus associated with the
first gianfar.  But this might not be the case, so simply locking the
gianfar's PHY's mii bus might not lock the mii bus that the SerDes setup
code is going to use.

We solve this by having the code that creates the gianfar platform device
look in the device tree for an mdio device that shares the gianfar's
registers.  If one is found the ID of its platform device is saved in the
gianfar's platform data.

A new function in the gianfar mii code, gfar_get_miibus(), can use the bus
ID to search through the platform devices for a gianfar_mdio device with
the right ID.  The platform device's driver data is the mii_bus structure,
which the SerDes setup code can use to lock the current bus.

Signed-off-by: Trent Piepho <tpiepho@freescale.com>
CC: Andy Fleming <afleming@freescale.com>
---
 arch/powerpc/sysdev/fsl_soc.c |   26 ++++++++++++++++++++++++++
 drivers/net/gianfar.c         |    7 +++++++
 drivers/net/gianfar_mii.c     |   21 +++++++++++++++++++++
 drivers/net/gianfar_mii.h     |    3 +++
 include/linux/fsl_devices.h   |    3 ++-
 5 files changed, 59 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 01b884b..26ecb96 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -223,6 +223,8 @@ static int gfar_mdio_of_init_one(struct device_node *np)
 	if (ret)
 		return ret;
 
+	/* The gianfar device will try to use the same ID created below to find
+	 * this bus, to coordinate register access (since they share).  */
 	mdio_dev = platform_device_register_simple("fsl-gianfar_mdio",
 			res.start&0xfffff, &res, 1);
 	if (IS_ERR(mdio_dev))
@@ -394,6 +396,30 @@ static int __init gfar_of_init(void)
 			of_node_put(mdio);
 		}
 
+		/* Get MDIO bus controlled by this eTSEC, if any.  Normally only
+		 * eTSEC 1 will control an MDIO bus, not necessarily the same
+		 * bus that its PHY is on ('mdio' above), so we can't just use
+		 * that.  What we do is look for a gianfar mdio device that has
+		 * overlapping registers with this device.  That's really the
+		 * whole point, to find the device sharing our registers to
+		 * coordinate access with it.
+		 */
+		for_each_compatible_node(mdio, NULL, "fsl,gianfar-mdio") {
+			if (of_address_to_resource(mdio, 0, &res))
+				continue;
+
+			if (res.start >= r[0].start && res.end <= r[0].end) {
+				/* Get the ID the mdio bus platform device was
+				 * registered with.  gfar_data.bus_id is
+				 * different because it's for finding a PHY,
+				 * while this is for finding a MII bus.
+				 */
+				gfar_data.mdio_bus = res.start&0xfffff;
+				of_node_put(mdio);
+				break;
+			}
+		}
+
 		ret =
 		    platform_device_add_data(gfar_dev, &gfar_data,
 					     sizeof(struct
diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 64b2011..249541a 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -586,6 +586,10 @@ 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);
+	struct mii_bus *bus = gfar_get_miibus(priv);
+
+	if (bus)
+		mutex_lock(&bus->mdio_lock);
 
 	/* Single clk mode, mii mode off(for serdes communication) */
 	gfar_local_mdio_write(regs, tbipa, MII_TBICON, TBICON_CLK_SELECT);
@@ -596,6 +600,9 @@ static void gfar_configure_serdes(struct net_device *dev)
 
 	gfar_local_mdio_write(regs, tbipa, MII_BMCR, BMCR_ANENABLE |
 			BMCR_ANRESTART | BMCR_FULLDPLX | BMCR_SPEED1000);
+
+	if (bus)
+		mutex_unlock(&bus->mdio_lock);
 }
 
 static void init_registers(struct net_device *dev)
diff --git a/drivers/net/gianfar_mii.c b/drivers/net/gianfar_mii.c
index bf73eea..0e2595d 100644
--- a/drivers/net/gianfar_mii.c
+++ b/drivers/net/gianfar_mii.c
@@ -269,6 +269,27 @@ static struct device_driver gianfar_mdio_driver = {
 	.remove = gfar_mdio_remove,
 };
 
+static int match_mdio_bus(struct device *dev, void *data)
+{
+	const struct gfar_private *priv = data;
+	const struct platform_device *pdev = to_platform_device(dev);
+
+	return !strcmp(pdev->name, gianfar_mdio_driver.name) &&
+		pdev->id == priv->einfo->mdio_bus;
+}
+
+/* Given a gfar_priv structure, find the mii_bus controlled by this device (not
+ * necessarily the same as the bus the gfar's PHY is on), if one exists.
+ * Normally only the first gianfar controls a mii_bus.  */
+struct mii_bus *gfar_get_miibus(const struct gfar_private *priv)
+{
+	/*const*/ struct device *d;
+
+	d = bus_find_device(gianfar_mdio_driver.bus, NULL, (void *)priv,
+			    match_mdio_bus);
+	return d ? dev_get_drvdata(d) : NULL;
+}
+
 int __init gfar_mdio_init(void)
 {
 	return driver_register(&gianfar_mdio_driver);
diff --git a/drivers/net/gianfar_mii.h b/drivers/net/gianfar_mii.h
index 2af28b1..02dc970 100644
--- a/drivers/net/gianfar_mii.h
+++ b/drivers/net/gianfar_mii.h
@@ -18,6 +18,8 @@
 #ifndef __GIANFAR_MII_H
 #define __GIANFAR_MII_H
 
+struct gfar_private; /* forward ref */
+
 #define MIIMIND_BUSY            0x00000001
 #define MIIMIND_NOTVALID        0x00000004
 
@@ -44,6 +46,7 @@ int gfar_mdio_write(struct mii_bus *bus, int mii_id, int regnum, u16 value);
 int gfar_local_mdio_write(struct gfar_mii __iomem *regs, int mii_id,
 			  int regnum, u16 value);
 int gfar_local_mdio_read(struct gfar_mii __iomem *regs, int mii_id, int regnum);
+struct mii_bus *gfar_get_miibus(const struct gfar_private *priv);
 int __init gfar_mdio_init(void);
 void gfar_mdio_exit(void);
 #endif /* GIANFAR_PHY_H */
diff --git a/include/linux/fsl_devices.h b/include/linux/fsl_devices.h
index 4e625e0..708bab5 100644
--- a/include/linux/fsl_devices.h
+++ b/include/linux/fsl_devices.h
@@ -49,7 +49,8 @@ struct gianfar_platform_data {
 	u32	device_flags;
 	/* board specific information */
 	u32	board_flags;
-	char	bus_id[MII_BUS_ID_SIZE];
+	int	mdio_bus;			/* Bus controlled by us */
+	char	bus_id[MII_BUS_ID_SIZE];	/* Bus PHY is on */
 	u32	phy_id;
 	u8	mac_addr[6];
 	phy_interface_t interface;
-- 
1.5.4.1

Re: [PATCH 1/2] gianfar: Fix race in TBI/SerDes configuration

[Jeff Garzik]

Trent Piepho wrote:
> The init_phy() function attaches to the PHY, then configures the
> SerDes<->TBI link (in SGMII mode).  The TBI is on the MDIO bus with the PHY
> (sort of) and is accessed via the gianfar's MDIO registers, using the
> functions gfar_local_mdio_read/write(), which don't do any locking.
> 
> The previously attached PHY will start a work-queue on a timer, and
> probably an irq handler as well, which will talk to the PHY and thus use
> the MDIO bus.  This uses phy_read/write(), which have locking, but not
> against the gfar_local_mdio versions.
> 
> The result is that PHY code will try to use the MDIO bus at the same time
> as the SerDes setup code, corrupting the transfers.
> 
> Setting up the SerDes before attaching to the PHY will insure that there is
> no race between the SerDes code and *our* PHY, but doesn't fix everything.
> Typically the PHYs for all gianfar devices are on the same MDIO bus, which
> is associated with the first gianfar device.  This means that the first
> gianfar's SerDes code could corrupt the MDIO transfers for a different
> gianfar's PHY.
> 
> The lock used by phy_read/write() is contained in the mii_bus structure,
> which is pointed to by the PHY.  This is difficult to access from the
> gianfar drivers, as there is no link between a gianfar device and the
> mii_bus which shares the same MDIO registers.  As far as the device layer
> and drivers are concerned they are two unrelated devices (which happen to
> share registers).
> 
> Generally all gianfar devices' PHYs will be on the bus associated with the
> first gianfar.  But this might not be the case, so simply locking the
> gianfar's PHY's mii bus might not lock the mii bus that the SerDes setup
> code is going to use.
> 
> We solve this by having the code that creates the gianfar platform device
> look in the device tree for an mdio device that shares the gianfar's
> registers.  If one is found the ID of its platform device is saved in the
> gianfar's platform data.
> 
> A new function in the gianfar mii code, gfar_get_miibus(), can use the bus
> ID to search through the platform devices for a gianfar_mdio device with
> the right ID.  The platform device's driver data is the mii_bus structure,
> which the SerDes setup code can use to lock the current bus.
> 
> Signed-off-by: Trent Piepho <tpiepho@freescale.com>
> CC: Andy Fleming <afleming@freescale.com>
> ---
>  arch/powerpc/sysdev/fsl_soc.c |   26 ++++++++++++++++++++++++++
>  drivers/net/gianfar.c         |    7 +++++++
>  drivers/net/gianfar_mii.c     |   21 +++++++++++++++++++++
>  drivers/net/gianfar_mii.h     |    3 +++
>  include/linux/fsl_devices.h   |    3 ++-
>  5 files changed, 59 insertions(+), 1 deletions(-)

applied 1-2

[PATCH 2/2] gianfar: Don't reset TBI<->SerDes link if it's already up

[Trent Piepho]

The link may be up already via the chip's reset strapping, or though action
of U-Boot, or from the last time the interface was brought up.  Resetting
the link causes it to go down for several seconds.  This can significantly
increase the time from power-on to DHCP completion and a device being
accessible to the network.

Signed-off-by: Trent Piepho <tpiepho@freescale.com>
Acked-by: Andy Fleming <afleming@freescale.com>
---
 drivers/net/gianfar.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index 249541a..83a5cb6 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -591,6 +591,14 @@ static void gfar_configure_serdes(struct net_device *dev)
 	if (bus)
 		mutex_lock(&bus->mdio_lock);
 
+	/* If the link is already up, we must already be ok, and don't need to
+	 * configure and reset the TBI<->SerDes link.  Maybe U-Boot configured
+	 * everything for us?  Resetting it takes the link down and requires
+	 * several seconds for it to come back.
+	 */
+	if (gfar_local_mdio_read(regs, tbipa, MII_BMSR) & BMSR_LSTATUS)
+		goto done;
+
 	/* Single clk mode, mii mode off(for serdes communication) */
 	gfar_local_mdio_write(regs, tbipa, MII_TBICON, TBICON_CLK_SELECT);
 
@@ -601,6 +609,7 @@ static void gfar_configure_serdes(struct net_device *dev)
 	gfar_local_mdio_write(regs, tbipa, MII_BMCR, BMCR_ANENABLE |
 			BMCR_ANRESTART | BMCR_FULLDPLX | BMCR_SPEED1000);
 
+	done:
 	if (bus)
 		mutex_unlock(&bus->mdio_lock);
 }
-- 
1.5.4.1

Re: [PATCH] gianfar: Omit TBI auto-negotiation based on device tree

[Nate Case]

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!

-- 
Nate Case <ncase@xes-inc.com>

Re: [PATCH] gianfar: Omit TBI auto-negotiation based on device tree

[Kumar Gala]

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