[PATCH] Merge GT/MV642xx Support into MV643xx Driver [7/8]

[PATCH] Merge GT/MV642xx Support into MV643xx Driver [7/8]

[Steven J. Hill]

[-- Attachment #1.1: Type: text/plain, Size: 101 bytes --]

Get rid of global PHY spinlock.

Signed-off-by: Steven J. Hill <sjhill1@rockwellcollins.com>
---

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 07-mv643xx-remove-phy-spinlock.patch --]
[-- Type: text/x-patch; name="07-mv643xx-remove-phy-spinlock.patch", Size: 7997 bytes --]

diff -ur linux-2.6.22.1/drivers/net/mv643xx_eth.c linux-2.6.22.1-rci/drivers/net/mv643xx_eth.c
--- linux-2.6.22.1/drivers/net/mv643xx_eth.c	2007-07-18 22:51:54.000000000 -0500
+++ linux-2.6.22.1-rci/drivers/net/mv643xx_eth.c	2007-07-18 22:39:37.000000000 -0500
@@ -74,7 +74,7 @@
 #endif
 static int ethernet_phy_get(unsigned int eth_port_num);
 static void ethernet_phy_set(unsigned int eth_port_num, int phy_addr);
-static int ethernet_phy_detect(unsigned int eth_port_num);
+static int ethernet_phy_detect(struct mv643xx_private *mp);
 static int mv643xx_mdio_read(struct net_device *dev, int phy_id, int location);
 static void mv643xx_mdio_write(struct net_device *dev, int phy_id, int location, int val);
 static int mv643xx_eth_do_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd);
@@ -85,8 +85,6 @@
 
 static void __iomem *mv643xx_eth_shared_base;
 
-/* used to protect MV643XX_ETH_SMI_REG, which is shared across ports */
-static DEFINE_SPINLOCK(mv643xx_eth_phy_lock);
 #ifdef CONFIG_GT64260
 extern struct mv64x60_handle bh;
 static u32 eth_hash_table_size[3] = { 1, 1, 1 };
@@ -936,11 +927,12 @@
 static int mv643xx_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
 	struct mv643xx_private *mp = netdev_priv(dev);
+	unsigned long flags;
 	int err;
 
-	spin_lock_irq(&mp->lock);
+	spin_lock_irqsave(&mp->lock, flags);
 	err = mii_ethtool_sset(&mp->mii, cmd);
-	spin_unlock_irq(&mp->lock);
+	spin_unlock_irqrestore(&mp->lock, flags);
 
 	return err;
 }
@@ -948,11 +940,12 @@
 static int mv643xx_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 {
 	struct mv643xx_private *mp = netdev_priv(dev);
+	unsigned long flags;
 	int err;
 
-	spin_lock_irq(&mp->lock);
+	spin_lock_irqsave(&mp->lock, flags);
 	err = mii_ethtool_gset(&mp->mii, cmd);
-	spin_unlock_irq(&mp->lock);
+	spin_unlock_irqrestore(&mp->lock, flags);
 
 	/* The PHY may support 1000baseT_Half, but the mv643xx does not */
 	cmd->supported &= ~SUPPORTED_1000baseT_Half;
@@ -1594,12 +1599,12 @@
 static int mv643xx_eth_probe(struct platform_device *pdev)
 {
 	struct mv643xx_eth_platform_data *pd;
-	int port_num;
+	unsigned int port_num;
 	struct mv643xx_private *mp;
 	struct net_device *dev;
 	u8 *p;
 	struct resource *res;
-	int err;
+	int err = 0;
 	struct ethtool_cmd cmd;
 	int duplex = DUPLEX_HALF;
 	int speed = 0;			/* default to auto-negotiation */
@@ -1702,7 +1707,7 @@
 	mp->mii.phy_id_mask = 0x3f;
 	mp->mii.reg_num_mask = 0x1f;
 
-	err = ethernet_phy_detect(port_num);
+	err = ethernet_phy_detect(mp);
 	if (err) {
 		pr_debug("MV643xx ethernet port %d: "
 					"No PHY detected at addr %d\n",
@@ -1710,7 +1715,7 @@
 		goto out;
 	}
 
-	ethernet_phy_reset(port_num);
+	ethernet_phy_reset(mp);
 	mp->mii.supports_gmii = mii_check_gmii_support(&mp->mii);
 	mv643xx_init_ethtool_cmd(dev, mp->mii.phy_id, speed, duplex, &cmd);
 	mv643xx_eth_update_pscr(dev, &cmd);
@@ -2158,7 +2163,7 @@
 
 	/* save phy settings across reset */
 	mv643xx_get_settings(dev, &ethtool_cmd);
-	ethernet_phy_reset(mp->port_num);
+	ethernet_phy_reset(mp);
 	mv643xx_set_settings(dev, &ethtool_cmd);
 }
 
@@ -2761,22 +2766,22 @@
  *	-ENODEV on failure
  *
  */
-static int ethernet_phy_detect(unsigned int port_num)
+static int ethernet_phy_detect(struct mv643xx_private *mp)
 {
 	unsigned int phy_reg_data0;
 	int auto_neg;
 
-	eth_port_read_smi_reg(port_num, 0, &phy_reg_data0);
+	eth_port_read_smi_reg(mp, 0, &phy_reg_data0);
 	auto_neg = phy_reg_data0 & 0x1000;
 	phy_reg_data0 ^= 0x1000;	/* invert auto_neg */
-	eth_port_write_smi_reg(port_num, 0, phy_reg_data0);
+	eth_port_write_smi_reg(mp, 0, phy_reg_data0);
 
-	eth_port_read_smi_reg(port_num, 0, &phy_reg_data0);
+	eth_port_read_smi_reg(mp, 0, &phy_reg_data0);
 	if ((phy_reg_data0 & 0x1000) == auto_neg)
 		return -ENODEV;				/* change didn't take */
 
 	phy_reg_data0 ^= 0x1000;
-	eth_port_write_smi_reg(port_num, 0, phy_reg_data0);
+	eth_port_write_smi_reg(mp, 0, phy_reg_data0);
 	return 0;
 }
 
@@ -2849,19 +2854,19 @@
  *	None.
  *
  */
-static void ethernet_phy_reset(unsigned int eth_port_num)
+static void ethernet_phy_reset(struct mv643xx_private *mp)
 {
 	unsigned int phy_reg_data;
 
 	/* Reset the PHY */
-	eth_port_read_smi_reg(eth_port_num, 0, &phy_reg_data);
+	eth_port_read_smi_reg(mp, 0, &phy_reg_data);
 	phy_reg_data |= 0x8000;	/* Set bit 15 to reset the PHY */
-	eth_port_write_smi_reg(eth_port_num, 0, phy_reg_data);
+	eth_port_write_smi_reg(mp, 0, phy_reg_data);
 
 	/* wait for PHY to come out of reset */
 	do {
 		udelay(1);
-		eth_port_read_smi_reg(eth_port_num, 0, &phy_reg_data);
+		eth_port_read_smi_reg(mp, 0, &phy_reg_data);
 	} while (phy_reg_data & 0x8000);
 }
 
@@ -3033,15 +3038,18 @@
  *	true otherwise.
  *
  */
-static void eth_port_read_smi_reg(unsigned int port_num,
+static void eth_port_read_smi_reg(struct mv643xx_private *mp,
 				unsigned int phy_reg, unsigned int *value)
 {
-	int phy_addr = ethernet_phy_get(port_num);
+	int port_num = mp->port_num;
+	int phy_addr;
 	unsigned long flags;
 	int i;
 
+	phy_addr = ethernet_phy_get(port_num);
+
 	/* the SMI register is a shared resource */
-	spin_lock_irqsave(&mv643xx_eth_phy_lock, flags);
+	spin_lock_irqsave(&mp->lock, flags);
 
 	/* wait for the SMI register to become available */
 	for (i = 0; mv_read(MV643XX_ETH_SMI_REG) & ETH_SMI_BUSY; i++) {
@@ -3066,7 +3074,7 @@
 
 	*value = mv_read(MV643XX_ETH_SMI_REG) & 0xffff;
 out:
-	spin_unlock_irqrestore(&mv643xx_eth_phy_lock, flags);
+	spin_unlock_irqrestore(&mp->lock, flags);
 }
 
 /*
@@ -3089,9 +3097,10 @@
  *	true otherwise.
  *
  */
-static void eth_port_write_smi_reg(unsigned int eth_port_num,
+static void eth_port_write_smi_reg(struct mv643xx_private *mp,
 				   unsigned int phy_reg, unsigned int value)
 {
+	int eth_port_num = mp->port_num;
 	int phy_addr;
 	int i;
 	unsigned long flags;
@@ -3099,7 +3108,7 @@
 	phy_addr = ethernet_phy_get(eth_port_num);
 
 	/* the SMI register is a shared resource */
-	spin_lock_irqsave(&mv643xx_eth_phy_lock, flags);
+	spin_lock_irqsave(&mp->lock, flags);
 
 	/* wait for the SMI register to become available */
 	for (i = 0; mv_read(MV643XX_ETH_SMI_REG) & ETH_SMI_BUSY; i++) {
@@ -3114,7 +3123,7 @@
 	mv_write(MV643XX_ETH_SMI_REG, (phy_addr << 16) | (phy_reg << 21) |
 				ETH_SMI_OPCODE_WRITE | (value & 0xffff));
 out:
-	spin_unlock_irqrestore(&mv643xx_eth_phy_lock, flags);
+	spin_unlock_irqrestore(&mp->lock, flags);
 }
 
 /*
@@ -3125,14 +3134,14 @@
 	int val;
 	struct mv643xx_private *mp = netdev_priv(dev);
 
-	eth_port_read_smi_reg(mp->port_num, location, &val);
+	eth_port_read_smi_reg(mp, location, &val);
 	return val;
 }
 
 static void mv643xx_mdio_write(struct net_device *dev, int phy_id, int location, int val)
 {
 	struct mv643xx_private *mp = netdev_priv(dev);
-	eth_port_write_smi_reg(mp->port_num, location, val);
+	eth_port_write_smi_reg(mp, location, val);
 }
 
 /*
diff -ur linux-2.6.22.1/drivers/net/mv643xx_eth.h linux-2.6.22.1-rci/drivers/net/mv643xx_eth.h
--- linux-2.6.22.1/drivers/net/mv643xx_eth.h	2007-07-18 22:47:02.000000000 -0500
+++ linux-2.6.22.1-rci/drivers/net/mv643xx_eth.h	2007-07-11 09:28:16.000000000 -0500
@@ -510,12 +510,12 @@
 static void eth_port_start(struct net_device *dev);
 
 /* PHY and MIB routines */
-static void ethernet_phy_reset(unsigned int eth_port_num);
+static void ethernet_phy_reset(struct mv643xx_private *mp);
 
-static void eth_port_write_smi_reg(unsigned int eth_port_num,
+static void eth_port_write_smi_reg(struct mv643xx_private *mp,
 				   unsigned int phy_reg, unsigned int value);
 
-static void eth_port_read_smi_reg(unsigned int eth_port_num,
+static void eth_port_read_smi_reg(struct mv643xx_private *mp,
 				  unsigned int phy_reg, unsigned int *value);
 
 static void eth_clear_mib_counters(unsigned int eth_port_num);

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 254 bytes --]

Re: [PATCH] Merge GT/MV642xx Support into MV643xx Driver [7/8]

[Dale Farnsworth]

On Thu, Jul 19, 2007 at 04:56:51AM +0000, Steven J. Hill wrote:
> Get rid of global PHY spinlock.

You have replaced the use of the global PHY spinlock with a per-port spinlock.
However, the SMI register is shared by all ports.  The global lock is
needed to prevent simultaneous updates of the register by drivers for
multiple ports.

NAK

-Dale

Re: [PATCH] Merge GT/MV642xx Support into MV643xx Driver [7/8]

[Steven J. Hill]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Dale Farnsworth wrote:
> 
> You have replaced the use of the global PHY spinlock with a per-port spinlock.
> However, the SMI register is shared by all ports.  The global lock is
> needed to prevent simultaneous updates of the register by drivers for
> multiple ports.
> 
> NAK
>
Are you sure? Notice that a majority of the spinlocks were changed to disable
IRQs. Secondly, the lowest level mv_read/mv_write functions have to acquire
the big mv64x60_lock before they can read or write registers. I see the PHY
spinlock as being an additional and unnecessary lock to contend with. Am I
make an improper assumption?

- -Steve
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.2 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGoBmSgyK5H2Ic36cRAj8JAKCfmg/T2FgOdYZ5YfnXJsiyn3RkaQCfadSk
GS8ICyW0+qNRHr5QqnY0PUQ=
=nvSB
-----END PGP SIGNATURE-----

Re: [PATCH] Merge GT/MV642xx Support into MV643xx Driver [7/8]

[Dale Farnsworth]

On Thu, Jul 19, 2007 at 09:10:26PM -0500, Steven J. Hill wrote:
> Dale Farnsworth wrote:
> > 
> > You have replaced the use of the global PHY spinlock with a per-port spinlock.
> > However, the SMI register is shared by all ports.  The global lock is
> > needed to prevent simultaneous updates of the register by drivers for
> > multiple ports.
> > 
> > NAK
> >
> Are you sure? Notice that a majority of the spinlocks were changed to disable
> IRQs. Secondly, the lowest level mv_read/mv_write functions have to acquire
> the big mv64x60_lock before they can read or write registers. I see the PHY
> spinlock as being an additional and unnecessary lock to contend with. Am I
> make an improper assumption?

I'm sure.  (Of course, I could be wrong.)  On an SMP (or fully
preemptive) system, disabling IRQs doesn't provide sufficient
protection.  Nor does a per-port spinlock, since multiple ports
share the single register.  It seems to me that a driver-scope
lock is required.

-Dale