staging: octeon: refactor rgmii 10 mbps preamble error checking

Refactor RGMII 10 Mbps preamble error checking. The current implementation
does not work correctly in phydev mode since only the link status changes
trigger the callback, and if we stay on 10 Mbps operation the periodic
checks for error counters are never done.

Provide a periodic worker also during the phydev operation, and notify
the link status changes through the phydev instead of the inband
status change interrupt. This also has the benefit that we don't need
to use legacy CVMX MDIO calls to check the PHY state, and we can avoid
races that trigger bogus "Using 10Mbps with software preamble removal"
logs when interfaces are being bringed up. It also avoids some corner-case
crashes when the in-band interrupt triggers while the interface is
being taken down.

Tested on EdgeRouter Lite & D-Link DSR-1000N.

Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

drivers/staging/octeon/ethernet-mdio.c

@@ -118,13 +118,20 @@ void cvm_oct_adjust_link(struct net_device *dev)
 	struct octeon_ethernet *priv = netdev_priv(dev);
 	cvmx_helper_link_info_t link_info;
 
+	link_info.u64		= 0;
+	link_info.s.link_up	= priv->phydev->link ? 1 : 0;
+	link_info.s.full_duplex = priv->phydev->duplex ? 1 : 0;
+	link_info.s.speed	= priv->phydev->speed;
+	priv->link_info		= link_info.u64;
+
+	/*
+	 * The polling task need to know about link status changes.
+	 */
+	if (priv->poll)
+		priv->poll(dev);
+
 	if (priv->last_link != priv->phydev->link) {
 		priv->last_link = priv->phydev->link;
-		link_info.u64 = 0;
-		link_info.s.link_up = priv->last_link ? 1 : 0;
-		link_info.s.full_duplex = priv->phydev->duplex ? 1 : 0;
-		link_info.s.speed = priv->phydev->speed;
-
 		cvmx_helper_link_set(priv->port, link_info);
 		cvm_oct_note_carrier(priv, link_info);
 	}

drivers/staging/octeon/ethernet-rgmii.c

@@ -30,8 +30,6 @@
 
 static DEFINE_SPINLOCK(global_register_lock);
 
-static int number_rgmii_ports;
-
 static void cvm_oct_set_hw_preamble(struct octeon_ethernet *priv, bool enable)
 {
 	union cvmx_gmxx_rxx_frm_ctl gmxx_rxx_frm_ctl;
@@ -63,247 +61,106 @@ static void cvm_oct_set_hw_preamble(struct octeon_ethernet *priv, bool enable)
 		       gmxx_rxx_int_reg.u64);
 }
 
-static void cvm_oct_rgmii_poll(struct net_device *dev)
+static void cvm_oct_check_preamble_errors(struct net_device *dev)
 {
 	struct octeon_ethernet *priv = netdev_priv(dev);
-	unsigned long flags = 0;
 	cvmx_helper_link_info_t link_info;
-	int use_global_register_lock = (priv->phydev == NULL);
+	unsigned long flags;
+
+	link_info.u64 = priv->link_info;
 
-	BUG_ON(in_interrupt());
-	if (use_global_register_lock) {
+	/*
+	 * Take the global register lock since we are going to
+	 * touch registers that affect more than one port.
+	 */
+	spin_lock_irqsave(&global_register_lock, flags);
+
+	if (link_info.s.speed == 10 && priv->last_speed == 10) {
 		/*
-		 * Take the global register lock since we are going to
-		 * touch registers that affect more than one port.
+		 * Read the GMXX_RXX_INT_REG[PCTERR] bit and see if we are
+		 * getting preamble errors.
 		 */
-		spin_lock_irqsave(&global_register_lock, flags);
-	} else {
-		mutex_lock(&priv->phydev->mdio.bus->mdio_lock);
-	}
+		int interface = INTERFACE(priv->port);
+		int index = INDEX(priv->port);
+		union cvmx_gmxx_rxx_int_reg gmxx_rxx_int_reg;
 
-	link_info = cvmx_helper_link_get(priv->port);
-	if (link_info.u64 == priv->link_info) {
-		if (link_info.s.speed == 10) {
+		gmxx_rxx_int_reg.u64 = cvmx_read_csr(CVMX_GMXX_RXX_INT_REG
+							(index, interface));
+		if (gmxx_rxx_int_reg.s.pcterr) {
 			/*
-			 * Read the GMXX_RXX_INT_REG[PCTERR] bit and
-			 * see if we are getting preamble errors.
+			 * We are getting preamble errors at 10Mbps. Most
+			 * likely the PHY is giving us packets with misaligned
+			 * preambles. In order to get these packets we need to
+			 * disable preamble checking and do it in software.
 			 */
-			int interface = INTERFACE(priv->port);
-			int index = INDEX(priv->port);
-			union cvmx_gmxx_rxx_int_reg gmxx_rxx_int_reg;
-
-			gmxx_rxx_int_reg.u64 =
-			    cvmx_read_csr(CVMX_GMXX_RXX_INT_REG
-					  (index, interface));
-			if (gmxx_rxx_int_reg.s.pcterr) {
-				/*
-				 * We are getting preamble errors at
-				 * 10Mbps.  Most likely the PHY is
-				 * giving us packets with mis aligned
-				 * preambles. In order to get these
-				 * packets we need to disable preamble
-				 * checking and do it in software.
-				 */
-				cvm_oct_set_hw_preamble(priv, false);
-				printk_ratelimited("%s: Using 10Mbps with software preamble removal\n",
-						   dev->name);
-			}
+			cvm_oct_set_hw_preamble(priv, false);
+			printk_ratelimited("%s: Using 10Mbps with software preamble removal\n",
+					   dev->name);
 		}
-
-		if (use_global_register_lock)
-			spin_unlock_irqrestore(&global_register_lock, flags);
-		else
-			mutex_unlock(&priv->phydev->mdio.bus->mdio_lock);
-		return;
-	}
-
-	/* Since the 10Mbps preamble workaround is allowed we need to enable
-	 * preamble checking, FCS stripping, and clear error bits on
-	 * every speed change. If errors occur during 10Mbps operation
-	 * the above code will change this stuff
-	 */
-	cvm_oct_set_hw_preamble(priv, true);
-
-	if (priv->phydev == NULL) {
-		link_info = cvmx_helper_link_autoconf(priv->port);
-		priv->link_info = link_info.u64;
-	}
-
-	if (use_global_register_lock)
-		spin_unlock_irqrestore(&global_register_lock, flags);
-	else
-		mutex_unlock(&priv->phydev->mdio.bus->mdio_lock);
-
-	if (priv->phydev == NULL) {
-		/* Tell core. */
-		if (link_info.s.link_up) {
-			if (!netif_carrier_ok(dev))
-				netif_carrier_on(dev);
-		} else if (netif_carrier_ok(dev)) {
-			netif_carrier_off(dev);
-		}
-		cvm_oct_note_carrier(priv, link_info);
+	} else {
+		/*
+		 * Since the 10Mbps preamble workaround is allowed we need to
+		 * enable preamble checking, FCS stripping, and clear error
+		 * bits on every speed change. If errors occur during 10Mbps
+		 * operation the above code will change this stuff
+		 */
+		if (priv->last_speed != link_info.s.speed)
+			cvm_oct_set_hw_preamble(priv, true);
+		priv->last_speed = link_info.s.speed;
 	}
+	spin_unlock_irqrestore(&global_register_lock, flags);
 }
 
-static int cmv_oct_rgmii_gmx_interrupt(int interface)
+static void cvm_oct_rgmii_poll(struct net_device *dev)
 {
-	int index;
-	int count = 0;
-
-	/* Loop through every port of this interface */
-	for (index = 0;
-	     index < cvmx_helper_ports_on_interface(interface);
-	     index++) {
-		union cvmx_gmxx_rxx_int_reg gmx_rx_int_reg;
+	struct octeon_ethernet *priv = netdev_priv(dev);
+	cvmx_helper_link_info_t link_info;
+	bool status_change;
 
-		/* Read the GMX interrupt status bits */
-		gmx_rx_int_reg.u64 = cvmx_read_csr(CVMX_GMXX_RXX_INT_REG
-					  (index, interface));
-		gmx_rx_int_reg.u64 &= cvmx_read_csr(CVMX_GMXX_RXX_INT_EN
-					  (index, interface));
+	link_info = cvmx_helper_link_autoconf(priv->port);
+	status_change = priv->link_info != link_info.u64;
+	priv->link_info = link_info.u64;
 
-		/* Poll the port if inband status changed */
-		if (gmx_rx_int_reg.s.phy_dupx || gmx_rx_int_reg.s.phy_link ||
-		    gmx_rx_int_reg.s.phy_spd) {
-			struct net_device *dev =
-				    cvm_oct_device[cvmx_helper_get_ipd_port
-						   (interface, index)];
-			struct octeon_ethernet *priv = netdev_priv(dev);
+	cvm_oct_check_preamble_errors(dev);
 
-			if (dev && !atomic_read(&cvm_oct_poll_queue_stopping))
-				queue_work(cvm_oct_poll_queue,
-					   &priv->port_work);
+	if (likely(!status_change))
+		return;
 
-			gmx_rx_int_reg.u64 = 0;
-			gmx_rx_int_reg.s.phy_dupx = 1;
-			gmx_rx_int_reg.s.phy_link = 1;
-			gmx_rx_int_reg.s.phy_spd = 1;
-			cvmx_write_csr(CVMX_GMXX_RXX_INT_REG(index, interface),
-				       gmx_rx_int_reg.u64);
-			count++;
-		}
+	/* Tell core. */
+	if (link_info.s.link_up) {
+		if (!netif_carrier_ok(dev))
+			netif_carrier_on(dev);
+	} else if (netif_carrier_ok(dev)) {
+		netif_carrier_off(dev);
 	}
-	return count;
-}
-
-static irqreturn_t cvm_oct_rgmii_rml_interrupt(int cpl, void *dev_id)
-{
-	union cvmx_npi_rsl_int_blocks rsl_int_blocks;
-	int count = 0;
-
-	rsl_int_blocks.u64 = cvmx_read_csr(CVMX_NPI_RSL_INT_BLOCKS);
-
-	/* Check and see if this interrupt was caused by the GMX0 block */
-	if (rsl_int_blocks.s.gmx0)
-		count += cmv_oct_rgmii_gmx_interrupt(0);
-
-	/* Check and see if this interrupt was caused by the GMX1 block */
-	if (rsl_int_blocks.s.gmx1)
-		count += cmv_oct_rgmii_gmx_interrupt(1);
-
-	return count ? IRQ_HANDLED : IRQ_NONE;
+	cvm_oct_note_carrier(priv, link_info);
 }
 
 int cvm_oct_rgmii_open(struct net_device *dev)
-{
-	return cvm_oct_common_open(dev, cvm_oct_rgmii_poll);
-}
-
-static void cvm_oct_rgmii_immediate_poll(struct work_struct *work)
-{
-	struct octeon_ethernet *priv =
-		container_of(work, struct octeon_ethernet, port_work);
-	cvm_oct_rgmii_poll(cvm_oct_device[priv->port]);
-}
-
-int cvm_oct_rgmii_init(struct net_device *dev)
 {
 	struct octeon_ethernet *priv = netdev_priv(dev);
-	int r;
+	int ret;
 
-	cvm_oct_common_init(dev);
-	INIT_WORK(&priv->port_work, cvm_oct_rgmii_immediate_poll);
-	/*
-	 * Due to GMX errata in CN3XXX series chips, it is necessary
-	 * to take the link down immediately when the PHY changes
-	 * state. In order to do this we call the poll function every
-	 * time the RGMII inband status changes.  This may cause
-	 * problems if the PHY doesn't implement inband status
-	 * properly.
-	 */
-	if (number_rgmii_ports == 0) {
-		r = request_irq(OCTEON_IRQ_RML, cvm_oct_rgmii_rml_interrupt,
-				IRQF_SHARED, "RGMII", &number_rgmii_ports);
-		if (r != 0)
-			return r;
-	}
-	number_rgmii_ports++;
-
-	/*
-	 * Only true RGMII ports need to be polled. In GMII mode, port
-	 * 0 is really a RGMII port.
-	 */
-	if (((priv->imode == CVMX_HELPER_INTERFACE_MODE_GMII)
-	     && (priv->port == 0))
-	    || (priv->imode == CVMX_HELPER_INTERFACE_MODE_RGMII)) {
-		if (!octeon_is_simulation()) {
-			union cvmx_gmxx_rxx_int_en gmx_rx_int_en;
-			int interface = INTERFACE(priv->port);
-			int index = INDEX(priv->port);
+	ret = cvm_oct_common_open(dev, cvm_oct_rgmii_poll);
+	if (ret)
+		return ret;
 
-			/*
-			 * Enable interrupts on inband status changes
-			 * for this port.
-			 */
-			gmx_rx_int_en.u64 = 0;
-			gmx_rx_int_en.s.phy_dupx = 1;
-			gmx_rx_int_en.s.phy_link = 1;
-			gmx_rx_int_en.s.phy_spd = 1;
-			cvmx_write_csr(CVMX_GMXX_RXX_INT_EN(index, interface),
-				       gmx_rx_int_en.u64);
+	if (priv->phydev) {
+		/*
+		 * In phydev mode, we need still periodic polling for the
+		 * preamble error checking, and we also need to call this
+		 * function on every link state change.
+		 *
+		 * Only true RGMII ports need to be polled. In GMII mode, port
+		 * 0 is really a RGMII port.
+		 */
+		if ((priv->imode == CVMX_HELPER_INTERFACE_MODE_GMII &&
+		     priv->port  == 0) ||
+		    (priv->imode == CVMX_HELPER_INTERFACE_MODE_RGMII)) {
+			priv->poll = cvm_oct_check_preamble_errors;
+			cvm_oct_check_preamble_errors(dev);
 		}
 	}
 
 	return 0;
 }
-
-void cvm_oct_rgmii_uninit(struct net_device *dev)
-{
-	struct octeon_ethernet *priv = netdev_priv(dev);
-
-	cvm_oct_common_uninit(dev);
-
-	/*
-	 * Only true RGMII ports need to be polled. In GMII mode, port
-	 * 0 is really a RGMII port.
-	 */
-	if (((priv->imode == CVMX_HELPER_INTERFACE_MODE_GMII)
-	     && (priv->port == 0))
-	    || (priv->imode == CVMX_HELPER_INTERFACE_MODE_RGMII)) {
-		if (!octeon_is_simulation()) {
-			union cvmx_gmxx_rxx_int_en gmx_rx_int_en;
-			int interface = INTERFACE(priv->port);
-			int index = INDEX(priv->port);
-
-			/*
-			 * Disable interrupts on inband status changes
-			 * for this port.
-			 */
-			gmx_rx_int_en.u64 =
-			    cvmx_read_csr(CVMX_GMXX_RXX_INT_EN
-					  (index, interface));
-			gmx_rx_int_en.s.phy_dupx = 0;
-			gmx_rx_int_en.s.phy_link = 0;
-			gmx_rx_int_en.s.phy_spd = 0;
-			cvmx_write_csr(CVMX_GMXX_RXX_INT_EN(index, interface),
-				       gmx_rx_int_en.u64);
-		}
-	}
-
-	/* Remove the interrupt handler when the last port is removed. */
-	number_rgmii_ports--;
-	if (number_rgmii_ports == 0)
-		free_irq(OCTEON_IRQ_RML, &number_rgmii_ports);
-	cancel_work_sync(&priv->port_work);
-}

drivers/staging/octeon/ethernet.c

@@ -601,8 +601,8 @@ static const struct net_device_ops cvm_oct_spi_netdev_ops = {
 #endif
 };
 static const struct net_device_ops cvm_oct_rgmii_netdev_ops = {
-	.ndo_init		= cvm_oct_rgmii_init,
-	.ndo_uninit		= cvm_oct_rgmii_uninit,
+	.ndo_init		= cvm_oct_common_init,
+	.ndo_uninit		= cvm_oct_common_uninit,
 	.ndo_open		= cvm_oct_rgmii_open,
 	.ndo_stop		= cvm_oct_common_stop,
 	.ndo_start_xmit		= cvm_oct_xmit,

drivers/staging/octeon/octeon-ethernet.h

@@ -41,20 +41,18 @@ struct octeon_ethernet {
 	/* Device statistics */
 	struct net_device_stats stats;
 	struct phy_device *phydev;
+	unsigned int last_speed;
 	unsigned int last_link;
 	/* Last negotiated link state */
 	u64 link_info;
 	/* Called periodically to check link status */
 	void (*poll)(struct net_device *dev);
 	struct delayed_work	port_periodic_work;
-	struct work_struct	port_work;	/* may be unused. */
 	struct device_node	*of_node;
 };
 
 int cvm_oct_free_work(void *work_queue_entry);
 
-int cvm_oct_rgmii_init(struct net_device *dev);
-void cvm_oct_rgmii_uninit(struct net_device *dev);
 int cvm_oct_rgmii_open(struct net_device *dev);
 
 int cvm_oct_sgmii_init(struct net_device *dev);