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

PATCH NAK Steven J. Hill 2 participants 4 messages Jul 19, 2007 – Jul 20, 2007

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Steven J. Hill

author

2007-07-19 04:56 UTC #0 lore raw

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

Get rid of global PHY spinlock.

Signed-off-by Steven J. Hill <[email protected]>

---

drivers/net/mv643xx_eth.c +34 ~~-30~~

diff -ur linux-2.6.22.1/drivers/net/mv643xx_eth.c linux-2.6.22.1-rci/drivers/net/mv643xx_eth.c
@@ -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);
 }
 
 /*

drivers/net/mv643xx_eth.h +3 -3

diff -ur linux-2.6.22.1/drivers/net/mv643xx_eth.h linux-2.6.22.1-rci/drivers/net/mv643xx_eth.h
@@ -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);

Dale Farnsworth

NAK

2007-07-19 14:22 UTC #1 lore raw

Quoted 1 line · Steven J. Hill

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

Steven J. Hill

author

2007-07-20 02:10 UTC #2 lore raw

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

Quoted 8 lines · Dale Farnsworth

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

Dale Farnsworth

2007-07-20 03:40 UTC #3 lore raw

Quoted 7 lines · Steven J. Hill

Quoted 6 lines · Dale Farnsworth
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.

Quoted 7 lines

Quoted 2 lines
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