[PATCH 00/13] ftgmac100: Rework batch 1 - Link & Interrupts

[PATCH 00/13] ftgmac100: Rework batch 1 - Link & Interrupts

[Benjamin Herrenschmidt]

This is the first batch of updates to the ftgmac100 driver.

Essentially:

 - A few misc cleanups 
 - Fixing link speed & duplex handling (including dealing with
   an Aspeed requirement to double reset the controller when
   the speed changes) 
 - And addition of a reset task workqueue which will be used
   for delaying the re-initialization of the controller
 - Fixing a number of issues with how interrupts and NAPI 
   are dealt with.

Subsequent batches will rework and improve the rx path, the
tx path, and add a bunch of features and fixes.

[PATCH 01/13] ftgmac100: Use netdev->irq instead of private copy

[Benjamin Herrenschmidt]

There's a placeholder already for the irq, use it

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 928b0df..bf7b1c0 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -55,7 +55,6 @@ struct ftgmac100_descs {
 struct ftgmac100 {
 	struct resource *res;
 	void __iomem *base;
-	int irq;
 
 	struct ftgmac100_descs *descs;
 	dma_addr_t descs_dma_addr;
@@ -1119,9 +1118,9 @@ static int ftgmac100_open(struct net_device *netdev)
 		goto err_alloc;
 	}
 
-	err = request_irq(priv->irq, ftgmac100_interrupt, 0, netdev->name, netdev);
+	err = request_irq(netdev->irq, ftgmac100_interrupt, 0, netdev->name, netdev);
 	if (err) {
-		netdev_err(netdev, "failed to request irq %d\n", priv->irq);
+		netdev_err(netdev, "failed to request irq %d\n", netdev->irq);
 		goto err_irq;
 	}
 
@@ -1168,7 +1167,7 @@ static int ftgmac100_open(struct net_device *netdev)
 	netif_stop_queue(netdev);
 	iowrite32(0, priv->base + FTGMAC100_OFFSET_IER);
 err_hw:
-	free_irq(priv->irq, netdev);
+	free_irq(netdev->irq, netdev);
 err_irq:
 	ftgmac100_free_buffers(priv);
 err_alloc:
@@ -1194,7 +1193,7 @@ static int ftgmac100_stop(struct net_device *netdev)
 		ncsi_stop_dev(priv->ndev);
 
 	ftgmac100_stop_hw(priv);
-	free_irq(priv->irq, netdev);
+	free_irq(netdev->irq, netdev);
 	ftgmac100_free_buffers(priv);
 
 	return 0;
@@ -1381,7 +1380,7 @@ static int ftgmac100_probe(struct platform_device *pdev)
 		goto err_ioremap;
 	}
 
-	priv->irq = irq;
+	netdev->irq = irq;
 
 	/* MAC address from chip or random one */
 	ftgmac100_setup_mac(priv);
@@ -1438,7 +1437,7 @@ static int ftgmac100_probe(struct platform_device *pdev)
 		goto err_register_netdev;
 	}
 
-	netdev_info(netdev, "irq %d, mapped at %p\n", priv->irq, priv->base);
+	netdev_info(netdev, "irq %d, mapped at %p\n", netdev->irq, priv->base);
 
 	return 0;
 
-- 
2.9.3

[PATCH 02/13] ftgmac100: Remove "banner" comments

[Benjamin Herrenschmidt]

The divisions they represent are not particularily meaningful
and things are going to be moving around with upcoming changes
making these comments more a burden than anything else.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 42 --------------------------------
 1 file changed, 42 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index bf7b1c0..6501aa7 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -44,9 +44,6 @@
 #define MAX_PKT_SIZE		1518
 #define RX_BUF_SIZE		PAGE_SIZE	/* must be smaller than 0x3fff */
 
-/******************************************************************************
- * private data
- *****************************************************************************/
 struct ftgmac100_descs {
 	struct ftgmac100_rxdes rxdes[RX_QUEUE_ENTRIES];
 	struct ftgmac100_txdes txdes[TX_QUEUE_ENTRIES];
@@ -86,9 +83,6 @@ struct ftgmac100 {
 static int ftgmac100_alloc_rx_page(struct ftgmac100 *priv,
 				   struct ftgmac100_rxdes *rxdes, gfp_t gfp);
 
-/******************************************************************************
- * internal functions (hardware register access)
- *****************************************************************************/
 static void ftgmac100_set_rx_ring_base(struct ftgmac100 *priv, dma_addr_t addr)
 {
 	iowrite32(addr, priv->base + FTGMAC100_OFFSET_RXR_BADR);
@@ -243,9 +237,6 @@ static void ftgmac100_stop_hw(struct ftgmac100 *priv)
 	iowrite32(0, priv->base + FTGMAC100_OFFSET_MACCR);
 }
 
-/******************************************************************************
- * internal functions (receive descriptor)
- *****************************************************************************/
 static bool ftgmac100_rxdes_first_segment(struct ftgmac100_rxdes *rxdes)
 {
 	return rxdes->rxdes0 & cpu_to_le32(FTGMAC100_RXDES0_FRS);
@@ -370,9 +361,6 @@ static struct page *ftgmac100_rxdes_get_page(struct ftgmac100 *priv,
 	return *ftgmac100_rxdes_page_slot(priv, rxdes);
 }
 
-/******************************************************************************
- * internal functions (receive)
- *****************************************************************************/
 static int ftgmac100_next_rx_pointer(int pointer)
 {
 	return (pointer + 1) & (RX_QUEUE_ENTRIES - 1);
@@ -557,9 +545,6 @@ static bool ftgmac100_rx_packet(struct ftgmac100 *priv, int *processed)
 	return true;
 }
 
-/******************************************************************************
- * internal functions (transmit descriptor)
- *****************************************************************************/
 static void ftgmac100_txdes_reset(const struct ftgmac100 *priv,
 				  struct ftgmac100_txdes *txdes)
 {
@@ -653,9 +638,6 @@ static struct sk_buff *ftgmac100_txdes_get_skb(struct ftgmac100_txdes *txdes)
 	return (struct sk_buff *)txdes->txdes2;
 }
 
-/******************************************************************************
- * internal functions (transmit)
- *****************************************************************************/
 static int ftgmac100_next_tx_pointer(int pointer)
 {
 	return (pointer + 1) & (TX_QUEUE_ENTRIES - 1);
@@ -771,9 +753,6 @@ static int ftgmac100_xmit(struct ftgmac100 *priv, struct sk_buff *skb,
 	return NETDEV_TX_OK;
 }
 
-/******************************************************************************
- * internal functions (buffer)
- *****************************************************************************/
 static int ftgmac100_alloc_rx_page(struct ftgmac100 *priv,
 				   struct ftgmac100_rxdes *rxdes, gfp_t gfp)
 {
@@ -865,9 +844,6 @@ static int ftgmac100_alloc_buffers(struct ftgmac100 *priv)
 	return -ENOMEM;
 }
 
-/******************************************************************************
- * internal functions (mdio)
- *****************************************************************************/
 static void ftgmac100_adjust_link(struct net_device *netdev)
 {
 	struct ftgmac100 *priv = netdev_priv(netdev);
@@ -917,9 +893,6 @@ static int ftgmac100_mii_probe(struct ftgmac100 *priv)
 	return 0;
 }
 
-/******************************************************************************
- * struct mii_bus functions
- *****************************************************************************/
 static int ftgmac100_mdiobus_read(struct mii_bus *bus, int phy_addr, int regnum)
 {
 	struct net_device *netdev = bus->priv;
@@ -991,9 +964,6 @@ static int ftgmac100_mdiobus_write(struct mii_bus *bus, int phy_addr,
 	return -EIO;
 }
 
-/******************************************************************************
- * struct ethtool_ops functions
- *****************************************************************************/
 static void ftgmac100_get_drvinfo(struct net_device *netdev,
 				  struct ethtool_drvinfo *info)
 {
@@ -1009,9 +979,6 @@ static const struct ethtool_ops ftgmac100_ethtool_ops = {
 	.set_link_ksettings	= phy_ethtool_set_link_ksettings,
 };
 
-/******************************************************************************
- * interrupt handler
- *****************************************************************************/
 static irqreturn_t ftgmac100_interrupt(int irq, void *dev_id)
 {
 	struct net_device *netdev = dev_id;
@@ -1029,9 +996,6 @@ static irqreturn_t ftgmac100_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
-/******************************************************************************
- * struct napi_struct functions
- *****************************************************************************/
 static int ftgmac100_poll(struct napi_struct *napi, int budget)
 {
 	struct ftgmac100 *priv = container_of(napi, struct ftgmac100, napi);
@@ -1103,9 +1067,6 @@ static int ftgmac100_poll(struct napi_struct *napi, int budget)
 	return rx;
 }
 
-/******************************************************************************
- * struct net_device_ops functions
- *****************************************************************************/
 static int ftgmac100_open(struct net_device *netdev)
 {
 	struct ftgmac100 *priv = netdev_priv(netdev);
@@ -1318,9 +1279,6 @@ static void ftgmac100_ncsi_handler(struct ncsi_dev *nd)
 		    nd->link_up ? "up" : "down");
 }
 
-/******************************************************************************
- * struct platform_driver functions
- *****************************************************************************/
 static int ftgmac100_probe(struct platform_device *pdev)
 {
 	struct resource *res;
-- 
2.9.3

[PATCH 03/13] ftgmac100: Reorder struct fields and comment

[Benjamin Herrenschmidt]

Reorder the fields in struct ftgmac in slightly more logical
groups. Will make more sense as I add/remove some.

No code change.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 6501aa7..02e0534 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -50,34 +50,39 @@ struct ftgmac100_descs {
 };
 
 struct ftgmac100 {
+	/* Registers */
 	struct resource *res;
 	void __iomem *base;
 
 	struct ftgmac100_descs *descs;
 	dma_addr_t descs_dma_addr;
 
+	/* Rx ring */
 	struct page *rx_pages[RX_QUEUE_ENTRIES];
-
 	unsigned int rx_pointer;
+	u32 rxdes0_edorr_mask;
+
+	/* Tx ring */
 	unsigned int tx_clean_pointer;
 	unsigned int tx_pointer;
 	unsigned int tx_pending;
-
+	u32 txdes0_edotr_mask;
 	spinlock_t tx_lock;
 
+	/* Component structures */
 	struct net_device *netdev;
 	struct device *dev;
 	struct ncsi_dev *ndev;
 	struct napi_struct napi;
-
 	struct mii_bus *mii_bus;
+
+	/* Link management */
 	int old_speed;
-	int int_mask_all;
 	bool use_ncsi;
-	bool enabled;
 
-	u32 rxdes0_edorr_mask;
-	u32 txdes0_edotr_mask;
+	/* Misc */
+	int int_mask_all;
+	bool enabled;
 };
 
 static int ftgmac100_alloc_rx_page(struct ftgmac100 *priv,
-- 
2.9.3

[PATCH 04/13] ftgmac100: Remove "enabled" flags

[Benjamin Herrenschmidt]

It's not used in any meaningful way

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 02e0534..cc2271b 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -82,7 +82,6 @@ struct ftgmac100 {
 
 	/* Misc */
 	int int_mask_all;
-	bool enabled;
 };
 
 static int ftgmac100_alloc_rx_page(struct ftgmac100 *priv,
@@ -1124,8 +1123,6 @@ static int ftgmac100_open(struct net_device *netdev)
 			goto err_ncsi;
 	}
 
-	priv->enabled = true;
-
 	return 0;
 
 err_ncsi:
@@ -1144,11 +1141,7 @@ static int ftgmac100_stop(struct net_device *netdev)
 {
 	struct ftgmac100 *priv = netdev_priv(netdev);
 
-	if (!priv->enabled)
-		return 0;
-
 	/* disable all interrupts */
-	priv->enabled = false;
 	iowrite32(0, priv->base + FTGMAC100_OFFSET_IER);
 
 	netif_stop_queue(netdev);
-- 
2.9.3

[PATCH 05/13] ftgmac100: Cleanup speed/duplex tracking and fix duplex config

[Benjamin Herrenschmidt]

Keep track of both the current speed and duplex settings
instead of only speed and properly apply the duplex setting
to the HW.

This reworks the adjust_link() function to also avoid trying
to reconfigure the HW when there is no link and to display
the link state to the user.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 54 +++++++++++++++++++++++++++-----
 1 file changed, 46 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index cc2271b..219131c 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -77,7 +77,8 @@ struct ftgmac100 {
 	struct mii_bus *mii_bus;
 
 	/* Link management */
-	int old_speed;
+	int cur_speed;
+	int cur_duplex;
 	bool use_ncsi;
 
 	/* Misc */
@@ -210,16 +211,15 @@ static void ftgmac100_init_hw(struct ftgmac100 *priv)
 				 FTGMAC100_MACCR_RXDMA_EN	| \
 				 FTGMAC100_MACCR_TXMAC_EN	| \
 				 FTGMAC100_MACCR_RXMAC_EN	| \
-				 FTGMAC100_MACCR_FULLDUP	| \
 				 FTGMAC100_MACCR_CRC_APD	| \
 				 FTGMAC100_MACCR_RX_RUNT	| \
 				 FTGMAC100_MACCR_RX_BROADPKT)
 
-static void ftgmac100_start_hw(struct ftgmac100 *priv, int speed)
+static void ftgmac100_start_hw(struct ftgmac100 *priv)
 {
 	int maccr = MACCR_ENABLE_ALL;
 
-	switch (speed) {
+	switch (priv->cur_speed) {
 	default:
 	case 10:
 		break;
@@ -233,6 +233,9 @@ static void ftgmac100_start_hw(struct ftgmac100 *priv, int speed)
 		break;
 	}
 
+	if (priv->cur_duplex == DUPLEX_FULL)
+		maccr |= FTGMAC100_MACCR_FULLDUP;
+
 	iowrite32(maccr, priv->base + FTGMAC100_OFFSET_MACCR);
 }
 
@@ -852,12 +855,33 @@ static void ftgmac100_adjust_link(struct net_device *netdev)
 {
 	struct ftgmac100 *priv = netdev_priv(netdev);
 	struct phy_device *phydev = netdev->phydev;
+	int new_speed;
 	int ier;
 
-	if (phydev->speed == priv->old_speed)
+	/* We store "no link" as speed 0 */
+	if (!phydev->link)
+		new_speed = 0;
+	else
+		new_speed = phydev->speed;
+
+	if (phydev->speed == priv->cur_speed &&
+	    phydev->duplex == priv->cur_duplex)
+		return;
+
+	if (new_speed) {
+		netdev_info(netdev, "Link up at %d Mbit/s %s duplex\n",
+			    new_speed,
+			    phydev->duplex == DUPLEX_FULL ? "full" : "half");
+	} else if (priv->cur_speed) {
+		/* No link, just return. Leave the HW alone so it can
+		 * continue draining the tx ring.
+		 */
+		netdev_info(netdev, "Link down\n");
 		return;
+	}
 
-	priv->old_speed = phydev->speed;
+	priv->cur_speed = new_speed;
+	priv->cur_duplex = phydev->duplex;
 
 	ier = ioread32(priv->base + FTGMAC100_OFFSET_IER);
 
@@ -869,7 +893,7 @@ static void ftgmac100_adjust_link(struct net_device *netdev)
 
 	netif_start_queue(netdev);
 	ftgmac100_init_hw(priv);
-	ftgmac100_start_hw(priv, phydev->speed);
+	ftgmac100_start_hw(priv);
 
 	/* re-enable interrupts */
 	iowrite32(ier, priv->base + FTGMAC100_OFFSET_IER);
@@ -1089,6 +1113,20 @@ static int ftgmac100_open(struct net_device *netdev)
 		goto err_irq;
 	}
 
+	/* When using NC-SI we force the speed to 100Mbit/s full duplex,
+	 *
+	 * Otherwise we leave it set to 0 (no link), the link
+	 * message from the PHY layer will handle setting it up to
+	 * something else if needed.
+	 */
+	if (priv->use_ncsi) {
+		priv->cur_duplex = DUPLEX_FULL;
+		priv->cur_speed = SPEED_100;
+	} else {
+		priv->cur_duplex = 0;
+		priv->cur_speed = 0;
+	}
+
 	priv->rx_pointer = 0;
 	priv->tx_clean_pointer = 0;
 	priv->tx_pointer = 0;
@@ -1099,7 +1137,7 @@ static int ftgmac100_open(struct net_device *netdev)
 		goto err_hw;
 
 	ftgmac100_init_hw(priv);
-	ftgmac100_start_hw(priv, priv->use_ncsi ? 100 : 10);
+	ftgmac100_start_hw(priv);
 
 	/* Clear stale interrupts */
 	status = ioread32(priv->base + FTGMAC100_OFFSET_ISR);
-- 
2.9.3

[PATCH 06/13] ftgmac100: Split ring alloc, init and rx buffer alloc

[Benjamin Herrenschmidt]

Currently, a single function is used to allocate the rings
themselves, initialize them, populate the rx ring, and
allocate the rx buffers. The same happens on free.

This splits them into separate functions. This will be
useful when properly implementing re-initialization on
link changes and error handling when the rings will be
repopulated but not freed.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 68 ++++++++++++++++++++++----------
 1 file changed, 47 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 219131c..8792c7c 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -792,6 +792,7 @@ static void ftgmac100_free_buffers(struct ftgmac100 *priv)
 {
 	int i;
 
+	/* Free all RX buffers */
 	for (i = 0; i < RX_QUEUE_ENTRIES; i++) {
 		struct ftgmac100_rxdes *rxdes = &priv->descs->rxdes[i];
 		struct page *page = ftgmac100_rxdes_get_page(priv, rxdes);
@@ -804,6 +805,7 @@ static void ftgmac100_free_buffers(struct ftgmac100 *priv)
 		__free_page(page);
 	}
 
+	/* Free all TX buffers */
 	for (i = 0; i < TX_QUEUE_ENTRIES; i++) {
 		struct ftgmac100_txdes *txdes = &priv->descs->txdes[i];
 		struct sk_buff *skb = ftgmac100_txdes_get_skb(txdes);
@@ -815,40 +817,54 @@ static void ftgmac100_free_buffers(struct ftgmac100 *priv)
 		dma_unmap_single(priv->dev, map, skb_headlen(skb), DMA_TO_DEVICE);
 		kfree_skb(skb);
 	}
-
-	dma_free_coherent(priv->dev, sizeof(struct ftgmac100_descs),
-			  priv->descs, priv->descs_dma_addr);
 }
 
-static int ftgmac100_alloc_buffers(struct ftgmac100 *priv)
+static void ftgmac100_free_rings(struct ftgmac100 *priv)
 {
-	int i;
+	/* Free descriptors */
+	if (priv->descs)
+		dma_free_coherent(priv->dev, sizeof(struct ftgmac100_descs),
+				  priv->descs, priv->descs_dma_addr);
+}
 
+static int ftgmac100_alloc_rings(struct ftgmac100 *priv)
+{
+	/* Allocate descriptors */
 	priv->descs = dma_zalloc_coherent(priv->dev,
 					  sizeof(struct ftgmac100_descs),
 					  &priv->descs_dma_addr, GFP_KERNEL);
 	if (!priv->descs)
 		return -ENOMEM;
 
-	/* initialize RX ring */
-	ftgmac100_rxdes_set_end_of_ring(priv,
-					&priv->descs->rxdes[RX_QUEUE_ENTRIES - 1]);
+	return 0;
+}
+
+static void ftgmac100_init_rings(struct ftgmac100 *priv)
+{
+	int i;
+
+	/* Initialize RX ring */
+	for (i = 0; i < RX_QUEUE_ENTRIES; i++)
+		priv->descs->rxdes[i].rxdes0 = 0;
+	ftgmac100_rxdes_set_end_of_ring(priv, &priv->descs->rxdes[i - 1]);
+
+	/* Initialize TX ring */
+	for (i = 0; i < TX_QUEUE_ENTRIES; i++)
+		priv->descs->txdes[i].txdes0 = 0;
+	ftgmac100_txdes_set_end_of_ring(priv, &priv->descs->txdes[i -1]);
+}
+
+static int ftgmac100_alloc_rx_buffers(struct ftgmac100 *priv)
+{
+	int i;
 
 	for (i = 0; i < RX_QUEUE_ENTRIES; i++) {
 		struct ftgmac100_rxdes *rxdes = &priv->descs->rxdes[i];
 
 		if (ftgmac100_alloc_rx_page(priv, rxdes, GFP_KERNEL))
-			goto err;
+			return -ENOMEM;
 	}
-
-	/* initialize TX ring */
-	ftgmac100_txdes_set_end_of_ring(priv,
-					&priv->descs->txdes[TX_QUEUE_ENTRIES - 1]);
 	return 0;
-
-err:
-	ftgmac100_free_buffers(priv);
-	return -ENOMEM;
 }
 
 static void ftgmac100_adjust_link(struct net_device *netdev)
@@ -1101,12 +1117,20 @@ static int ftgmac100_open(struct net_device *netdev)
 	unsigned int status;
 	int err;
 
-	err = ftgmac100_alloc_buffers(priv);
+	/* Allocate ring buffers  */
+	err = ftgmac100_alloc_rings(priv);
 	if (err) {
-		netdev_err(netdev, "failed to allocate buffers\n");
-		goto err_alloc;
+		netdev_err(netdev, "Failed to allocate descriptors\n");
+		return err;
 	}
 
+	/* Initialize the rings */
+	ftgmac100_init_rings(priv);
+
+	/* Allocate receive buffers */
+	if (ftgmac100_alloc_rx_buffers(priv))
+		goto err_alloc;
+
 	err = request_irq(netdev->irq, ftgmac100_interrupt, 0, netdev->name, netdev);
 	if (err) {
 		netdev_err(netdev, "failed to request irq %d\n", netdev->irq);
@@ -1170,8 +1194,9 @@ static int ftgmac100_open(struct net_device *netdev)
 err_hw:
 	free_irq(netdev->irq, netdev);
 err_irq:
-	ftgmac100_free_buffers(priv);
 err_alloc:
+	ftgmac100_free_buffers(priv);
+	ftgmac100_free_rings(priv);
 	return err;
 }
 
@@ -1192,6 +1217,7 @@ static int ftgmac100_stop(struct net_device *netdev)
 	ftgmac100_stop_hw(priv);
 	free_irq(netdev->irq, netdev);
 	ftgmac100_free_buffers(priv);
+	ftgmac100_free_rings(priv);
 
 	return 0;
 }
-- 
2.9.3

[PATCH 07/13] ftgmac100: Move napi_add/del to open/close

[Benjamin Herrenschmidt]

Rather than probe/remove

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 8792c7c..ac22f71 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1160,6 +1160,9 @@ static int ftgmac100_open(struct net_device *netdev)
 	if (err)
 		goto err_hw;
 
+	/* Initialize NAPI */
+	netif_napi_add(netdev, &priv->napi, ftgmac100_poll, 64);
+
 	ftgmac100_init_hw(priv);
 	ftgmac100_start_hw(priv);
 
@@ -1190,6 +1193,7 @@ static int ftgmac100_open(struct net_device *netdev)
 err_ncsi:
 	napi_disable(&priv->napi);
 	netif_stop_queue(netdev);
+	netif_napi_del(&priv->napi);
 	iowrite32(0, priv->base + FTGMAC100_OFFSET_IER);
 err_hw:
 	free_irq(netdev->irq, netdev);
@@ -1209,6 +1213,7 @@ static int ftgmac100_stop(struct net_device *netdev)
 
 	netif_stop_queue(netdev);
 	napi_disable(&priv->napi);
+	netif_napi_del(&priv->napi);
 	if (netdev->phydev)
 		phy_stop(netdev->phydev);
 	else if (priv->use_ncsi)
@@ -1381,9 +1386,6 @@ static int ftgmac100_probe(struct platform_device *pdev)
 
 	spin_lock_init(&priv->tx_lock);
 
-	/* initialize NAPI */
-	netif_napi_add(netdev, &priv->napi, ftgmac100_poll, 64);
-
 	/* map io memory */
 	priv->res = request_mem_region(res->start, resource_size(res),
 				       dev_name(&pdev->dev));
-- 
2.9.3

[PATCH 08/13] ftgmac100: Request the interrupt only after HW is reset

[Benjamin Herrenschmidt]

The interrupt isn't shared, so this will keep it masked
until we have the HW in a known sane state.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index ac22f71..0f7fd20 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1131,12 +1131,6 @@ static int ftgmac100_open(struct net_device *netdev)
 	if (ftgmac100_alloc_rx_buffers(priv))
 		goto err_alloc;
 
-	err = request_irq(netdev->irq, ftgmac100_interrupt, 0, netdev->name, netdev);
-	if (err) {
-		netdev_err(netdev, "failed to request irq %d\n", netdev->irq);
-		goto err_irq;
-	}
-
 	/* When using NC-SI we force the speed to 100Mbit/s full duplex,
 	 *
 	 * Otherwise we leave it set to 0 (no link), the link
@@ -1163,6 +1157,13 @@ static int ftgmac100_open(struct net_device *netdev)
 	/* Initialize NAPI */
 	netif_napi_add(netdev, &priv->napi, ftgmac100_poll, 64);
 
+	/* Grab our interrupt */
+	err = request_irq(netdev->irq, ftgmac100_interrupt, 0, netdev->name, netdev);
+	if (err) {
+		netdev_err(netdev, "failed to request irq %d\n", netdev->irq);
+		goto err_irq;
+	}
+
 	ftgmac100_init_hw(priv);
 	ftgmac100_start_hw(priv);
 
@@ -1193,12 +1194,12 @@ static int ftgmac100_open(struct net_device *netdev)
 err_ncsi:
 	napi_disable(&priv->napi);
 	netif_stop_queue(netdev);
-	netif_napi_del(&priv->napi);
-	iowrite32(0, priv->base + FTGMAC100_OFFSET_IER);
-err_hw:
 	free_irq(netdev->irq, netdev);
 err_irq:
+	netif_napi_del(&priv->napi);
+err_hw:
 err_alloc:
+	iowrite32(0, priv->base + FTGMAC100_OFFSET_IER);
 	ftgmac100_free_buffers(priv);
 	ftgmac100_free_rings(priv);
 	return err;
-- 
2.9.3

[PATCH 09/13] ftgmac100: Move the bulk of inits to a separate function

[Benjamin Herrenschmidt]

The link monitoring and error handling code will have to
redo the ring inits and HW setup so move the code out of
ftgmac100_open() into a dedicated function.

This forces a bit of re-ordering of ftgmac100_open() but
nothing dramatic.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 71 +++++++++++++++++++-------------
 1 file changed, 42 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 0f7fd20..96c6bc2 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1111,10 +1111,35 @@ static int ftgmac100_poll(struct napi_struct *napi, int budget)
 	return rx;
 }
 
+static int ftgmac100_init_all(struct ftgmac100 *priv, bool ignore_alloc_err)
+{
+	int err = 0;
+
+	/* Re-init descriptors (adjust queue sizes) */
+	ftgmac100_init_rings(priv);
+
+	/* Realloc rx descriptors */
+	err = ftgmac100_alloc_rx_buffers(priv);
+	if (err && !ignore_alloc_err)
+		return err;
+
+	/* Reinit and restart HW */
+	ftgmac100_init_hw(priv);
+	ftgmac100_start_hw(priv);
+
+	/* Re-enable the device */
+	napi_enable(&priv->napi);
+	netif_start_queue(priv->netdev);
+
+	/* Enable all interrupts */
+	iowrite32(priv->int_mask_all, priv->base + FTGMAC100_OFFSET_IER);
+
+	return err;
+}
+
 static int ftgmac100_open(struct net_device *netdev)
 {
 	struct ftgmac100 *priv = netdev_priv(netdev);
-	unsigned int status;
 	int err;
 
 	/* Allocate ring buffers  */
@@ -1124,13 +1149,6 @@ static int ftgmac100_open(struct net_device *netdev)
 		return err;
 	}
 
-	/* Initialize the rings */
-	ftgmac100_init_rings(priv);
-
-	/* Allocate receive buffers */
-	if (ftgmac100_alloc_rx_buffers(priv))
-		goto err_alloc;
-
 	/* When using NC-SI we force the speed to 100Mbit/s full duplex,
 	 *
 	 * Otherwise we leave it set to 0 (no link), the link
@@ -1164,26 +1182,21 @@ static int ftgmac100_open(struct net_device *netdev)
 		goto err_irq;
 	}
 
-	ftgmac100_init_hw(priv);
-	ftgmac100_start_hw(priv);
-
-	/* Clear stale interrupts */
-	status = ioread32(priv->base + FTGMAC100_OFFSET_ISR);
-	iowrite32(status, priv->base + FTGMAC100_OFFSET_ISR);
+	/* Start things up */
+	err = ftgmac100_init_all(priv, false);
+	if (err) {
+		netdev_err(netdev, "Failed to allocate packet buffers\n");
+		goto err_alloc;
+	}
 
-	if (netdev->phydev)
+	if (netdev->phydev) {
+		/* If we have a PHY, start polling */
 		phy_start(netdev->phydev);
-	else if (priv->use_ncsi)
+	} else if (priv->use_ncsi) {
+		/* If using NC-SI, set our carrier on and start the stack */
 		netif_carrier_on(netdev);
 
-	napi_enable(&priv->napi);
-	netif_start_queue(netdev);
-
-	/* enable all interrupts */
-	iowrite32(priv->int_mask_all, priv->base + FTGMAC100_OFFSET_IER);
-
-	/* Start the NCSI device */
-	if (priv->use_ncsi) {
+		/* Start the NCSI device */
 		err = ncsi_start_dev(priv->ndev);
 		if (err)
 			goto err_ncsi;
@@ -1191,16 +1204,16 @@ static int ftgmac100_open(struct net_device *netdev)
 
 	return 0;
 
-err_ncsi:
+ err_ncsi:
 	napi_disable(&priv->napi);
 	netif_stop_queue(netdev);
+ err_alloc:
+	ftgmac100_free_buffers(priv);
 	free_irq(netdev->irq, netdev);
-err_irq:
+ err_irq:
 	netif_napi_del(&priv->napi);
-err_hw:
-err_alloc:
+ err_hw:
 	iowrite32(0, priv->base + FTGMAC100_OFFSET_IER);
-	ftgmac100_free_buffers(priv);
 	ftgmac100_free_rings(priv);
 	return err;
 }
-- 
2.9.3

[PATCH 10/13] ftgmac100: Add a reset task and use it for link changes

[Benjamin Herrenschmidt]

Link speed changes require a full HW reset. This isn't done
properly at the moment. It will involve delays and thus isn't
suitable to do from the link poll callback.

So let's create a reset_task that we can queue up when the
link changes. It will be useful for various cases of error
handling as well.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 85 +++++++++++++++++++++++++++-----
 1 file changed, 72 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 96c6bc2..d616c0a 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -74,6 +74,7 @@ struct ftgmac100 {
 	struct device *dev;
 	struct ncsi_dev *ndev;
 	struct napi_struct napi;
+	struct work_struct reset_task;
 	struct mii_bus *mii_bus;
 
 	/* Link management */
@@ -872,7 +873,6 @@ static void ftgmac100_adjust_link(struct net_device *netdev)
 	struct ftgmac100 *priv = netdev_priv(netdev);
 	struct phy_device *phydev = netdev->phydev;
 	int new_speed;
-	int ier;
 
 	/* We store "no link" as speed 0 */
 	if (!phydev->link)
@@ -899,20 +899,11 @@ static void ftgmac100_adjust_link(struct net_device *netdev)
 	priv->cur_speed = new_speed;
 	priv->cur_duplex = phydev->duplex;
 
-	ier = ioread32(priv->base + FTGMAC100_OFFSET_IER);
-
-	/* disable all interrupts */
+	/* Disable all interrupts */
 	iowrite32(0, priv->base + FTGMAC100_OFFSET_IER);
 
-	netif_stop_queue(netdev);
-	ftgmac100_stop_hw(priv);
-
-	netif_start_queue(netdev);
-	ftgmac100_init_hw(priv);
-	ftgmac100_start_hw(priv);
-
-	/* re-enable interrupts */
-	iowrite32(ier, priv->base + FTGMAC100_OFFSET_IER);
+	/* Reset the adapter asynchronously */
+	schedule_work(&priv->reset_task);
 }
 
 static int ftgmac100_mii_probe(struct ftgmac100 *priv)
@@ -1137,6 +1128,59 @@ static int ftgmac100_init_all(struct ftgmac100 *priv, bool ignore_alloc_err)
 	return err;
 }
 
+static void ftgmac100_reset_task(struct work_struct *work)
+{
+	struct ftgmac100 *priv = container_of(work, struct ftgmac100,
+					      reset_task);
+	struct net_device *netdev = priv->netdev;
+	int err;
+
+	netdev_dbg(netdev, "Resetting NIC...\n");
+
+	/* Block PHY polling */
+	if (netdev->phydev)
+		mutex_lock(&netdev->phydev->lock);
+
+	rtnl_lock();
+
+	/* Check if the interface is still up */
+	if (!netif_running(netdev))
+		goto bail;
+
+	/* Stop the network stack */
+	netif_trans_update(netdev);
+	napi_disable(&priv->napi);
+	netif_tx_disable(netdev);
+
+	/* Stop and reset the MAC */
+	ftgmac100_stop_hw(priv);
+	err = ftgmac100_reset_hw(priv);
+	if (err) {
+		/* Not much we can do ... it might come back... */
+		netdev_err(netdev, "attempting to continue...\n");
+	}
+
+	/* Free all rx and tx buffers */
+	ftgmac100_free_buffers(priv);
+
+	/* The ring pointers have been reset in HW, reflect this here */
+	priv->rx_pointer = 0;
+	priv->tx_clean_pointer = 0;
+	priv->tx_pointer = 0;
+	priv->tx_pending = 0;
+
+	/* Setup everything again and restart chip */
+	ftgmac100_init_all(priv, true);
+
+	netdev_dbg(netdev, "Reset done !\n");
+ bail:
+	rtnl_unlock();
+
+	/* Unblock PHY polling */
+	if (netdev->phydev)
+		mutex_unlock(&netdev->phydev->lock);
+}
+
 static int ftgmac100_open(struct net_device *netdev)
 {
 	struct ftgmac100 *priv = netdev_priv(netdev);
@@ -1222,6 +1266,14 @@ static int ftgmac100_stop(struct net_device *netdev)
 {
 	struct ftgmac100 *priv = netdev_priv(netdev);
 
+	/* Note about the reset task: We are called with the rtnl lock
+	 * held, so we are synchronized against the core of the reset
+	 * task. We must not try to synchronously cancel it otherwise
+	 * we can deadlock. But since it will test for netif_running()
+	 * which has already been cleared by the net core, we don't
+	 * anything special to do.
+	 */
+
 	/* disable all interrupts */
 	iowrite32(0, priv->base + FTGMAC100_OFFSET_IER);
 
@@ -1397,6 +1449,7 @@ static int ftgmac100_probe(struct platform_device *pdev)
 	priv = netdev_priv(netdev);
 	priv->netdev = netdev;
 	priv->dev = &pdev->dev;
+	INIT_WORK(&priv->reset_task, ftgmac100_reset_task);
 
 	spin_lock_init(&priv->tx_lock);
 
@@ -1500,6 +1553,12 @@ static int ftgmac100_remove(struct platform_device *pdev)
 	priv = netdev_priv(netdev);
 
 	unregister_netdev(netdev);
+
+	/* There's a small chance the reset task will have been re-queued,
+	 * during stop, make sure it's gone before we free the structure.
+	 */
+	cancel_work_sync(&priv->reset_task);
+
 	ftgmac100_destroy_mdio(netdev);
 
 	iounmap(priv->base);
-- 
2.9.3

[PATCH 11/13] ftgmac100: Rework MAC reset and init

[Benjamin Herrenschmidt]

The HW requires a full MAC reset when changing the speed.

Additionally the Aspeed documentation spells out that the
MAC needs to be reset twice with a 10us interval.

We thus move the speed setting and top level reset code
into a new ftgmac100_reset_and_config_mac() function which
handles both. Move the ring pointers initialization there
too in order to reflect the HW change.

Also reduce the timeout for the MAC reset as it shouldn't
take more than 300 clock cycles according to the doc.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 98 +++++++++++++++++++-------------
 1 file changed, 59 insertions(+), 39 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index d616c0a..9647b7c 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -112,27 +112,64 @@ static void ftgmac100_txdma_normal_prio_start_polling(struct ftgmac100 *priv)
 	iowrite32(1, priv->base + FTGMAC100_OFFSET_NPTXPD);
 }
 
-static int ftgmac100_reset_hw(struct ftgmac100 *priv)
+static int ftgmac100_reset_mac(struct ftgmac100 *priv, u32 maccr)
 {
 	struct net_device *netdev = priv->netdev;
 	int i;
 
 	/* NOTE: reset clears all registers */
-	iowrite32(FTGMAC100_MACCR_SW_RST, priv->base + FTGMAC100_OFFSET_MACCR);
-	for (i = 0; i < 5; i++) {
+	iowrite32(maccr, priv->base + FTGMAC100_OFFSET_MACCR);
+	iowrite32(maccr | FTGMAC100_MACCR_SW_RST,
+		  priv->base + FTGMAC100_OFFSET_MACCR);
+	for (i = 0; i < 50; i++) {
 		unsigned int maccr;
 
 		maccr = ioread32(priv->base + FTGMAC100_OFFSET_MACCR);
 		if (!(maccr & FTGMAC100_MACCR_SW_RST))
 			return 0;
 
-		udelay(1000);
+		udelay(1);
 	}
 
-	netdev_err(netdev, "software reset failed\n");
+	netdev_err(netdev, "Hardware reset failed\n");
 	return -EIO;
 }
 
+static int ftgmac100_reset_and_config_mac(struct ftgmac100 *priv)
+{
+	u32 maccr = 0;
+
+	switch (priv->cur_speed) {
+	case SPEED_10:
+	case 0: /* no link */
+		break;
+
+	case SPEED_100:
+		maccr |= FTGMAC100_MACCR_FAST_MODE;
+		break;
+
+	case SPEED_1000:
+		maccr |= FTGMAC100_MACCR_GIGA_MODE;
+		break;
+	default:
+		netdev_err(priv->netdev, "Unknown speed %d !\n",
+			   priv->cur_speed);
+		break;
+	}
+
+	/* (Re)initialize the queue pointers */
+	priv->rx_pointer = 0;
+	priv->tx_clean_pointer = 0;
+	priv->tx_pointer = 0;
+	priv->tx_pending = 0;
+
+	/* The doc says reset twice with 10us interval */
+	if (ftgmac100_reset_mac(priv, maccr))
+		return -EIO;
+	usleep_range(10, 1000);
+	return ftgmac100_reset_mac(priv, maccr);
+}
+
 static void ftgmac100_set_mac(struct ftgmac100 *priv, const unsigned char *mac)
 {
 	unsigned int maddr = mac[0] << 8 | mac[1];
@@ -208,35 +245,28 @@ static void ftgmac100_init_hw(struct ftgmac100 *priv)
 	ftgmac100_set_mac(priv, priv->netdev->dev_addr);
 }
 
-#define MACCR_ENABLE_ALL	(FTGMAC100_MACCR_TXDMA_EN	| \
-				 FTGMAC100_MACCR_RXDMA_EN	| \
-				 FTGMAC100_MACCR_TXMAC_EN	| \
-				 FTGMAC100_MACCR_RXMAC_EN	| \
-				 FTGMAC100_MACCR_CRC_APD	| \
-				 FTGMAC100_MACCR_RX_RUNT	| \
-				 FTGMAC100_MACCR_RX_BROADPKT)
-
 static void ftgmac100_start_hw(struct ftgmac100 *priv)
 {
-	int maccr = MACCR_ENABLE_ALL;
+	u32 maccr = ioread32(priv->base + FTGMAC100_OFFSET_MACCR);
 
-	switch (priv->cur_speed) {
-	default:
-	case 10:
-		break;
+	/* Keep the original GMAC and FAST bits */
+	maccr &= (FTGMAC100_MACCR_FAST_MODE | FTGMAC100_MACCR_GIGA_MODE);
 
-	case 100:
-		maccr |= FTGMAC100_MACCR_FAST_MODE;
-		break;
-
-	case 1000:
-		maccr |= FTGMAC100_MACCR_GIGA_MODE;
-		break;
-	}
+	/* Add all the main enable bits */
+	maccr |= FTGMAC100_MACCR_TXDMA_EN	|
+		 FTGMAC100_MACCR_RXDMA_EN	|
+		 FTGMAC100_MACCR_TXMAC_EN	|
+		 FTGMAC100_MACCR_RXMAC_EN	|
+		 FTGMAC100_MACCR_CRC_APD	|
+		 FTGMAC100_MACCR_PHY_LINK_LEVEL	|
+		 FTGMAC100_MACCR_RX_RUNT	|
+		 FTGMAC100_MACCR_RX_BROADPKT;
 
+	/* Add other bits as needed */
 	if (priv->cur_duplex == DUPLEX_FULL)
 		maccr |= FTGMAC100_MACCR_FULLDUP;
 
+	/* Hit the HW */
 	iowrite32(maccr, priv->base + FTGMAC100_OFFSET_MACCR);
 }
 
@@ -1154,7 +1184,7 @@ static void ftgmac100_reset_task(struct work_struct *work)
 
 	/* Stop and reset the MAC */
 	ftgmac100_stop_hw(priv);
-	err = ftgmac100_reset_hw(priv);
+	err = ftgmac100_reset_and_config_mac(priv);
 	if (err) {
 		/* Not much we can do ... it might come back... */
 		netdev_err(netdev, "attempting to continue...\n");
@@ -1163,12 +1193,6 @@ static void ftgmac100_reset_task(struct work_struct *work)
 	/* Free all rx and tx buffers */
 	ftgmac100_free_buffers(priv);
 
-	/* The ring pointers have been reset in HW, reflect this here */
-	priv->rx_pointer = 0;
-	priv->tx_clean_pointer = 0;
-	priv->tx_pointer = 0;
-	priv->tx_pending = 0;
-
 	/* Setup everything again and restart chip */
 	ftgmac100_init_all(priv, true);
 
@@ -1207,12 +1231,8 @@ static int ftgmac100_open(struct net_device *netdev)
 		priv->cur_speed = 0;
 	}
 
-	priv->rx_pointer = 0;
-	priv->tx_clean_pointer = 0;
-	priv->tx_pointer = 0;
-	priv->tx_pending = 0;
-
-	err = ftgmac100_reset_hw(priv);
+	/* Reset the hardware */
+	err = ftgmac100_reset_and_config_mac(priv);
 	if (err)
 		goto err_hw;
 
-- 
2.9.3

[PATCH 12/13] ftgmac100: Remove useless tests in interrupt handler

[Benjamin Herrenschmidt]

The interrupt is neither enabled nor registered when the interface
isn't running (regardless of whether we use nc-si or not) so the
test isn't useful.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 9647b7c..dc85d78 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1049,14 +1049,9 @@ static irqreturn_t ftgmac100_interrupt(int irq, void *dev_id)
 	struct net_device *netdev = dev_id;
 	struct ftgmac100 *priv = netdev_priv(netdev);
 
-	/* When running in NCSI mode, the interface should be ready for
-	 * receiving or transmitting NCSI packets before it's opened.
-	 */
-	if (likely(priv->use_ncsi || netif_running(netdev))) {
-		/* Disable interrupts for polling */
-		iowrite32(0, priv->base + FTGMAC100_OFFSET_IER);
-		napi_schedule(&priv->napi);
-	}
+	/* Disable interrupts for polling */
+	iowrite32(0, priv->base + FTGMAC100_OFFSET_IER);
+	napi_schedule(&priv->napi);
 
 	return IRQ_HANDLED;
 }
-- 
2.9.3

[PATCH 13/13] ftgmac100: Rework NAPI & interrupts handling

[Benjamin Herrenschmidt]

First, don't look at the interrupt status in the poll loop
to decide what to poll. It's wrong. If we have run out of
budget, we may still have RX packets to unqueue but no more
RX interrupt pending.

So instead move the code looking at the interrupt status
into the interrupt handler where it belongs. That avoids a slow
MMIO read in the NAPI fast path. We keep the abnormal interrupts
enabled while NAPI is scheduled.

While at it, actually do something useful in the "error" cases:

On AHB bus error, trigger the new reset task, that's about all
we can do. On RX packet fifo or descriptor overflows, we need
to restart the MAC after having freed things up. So set a flag
that NAPI will see and use to perform that restart after
harvesting the RX ring.

Finally, we shouldn't complete NAPI if there are still outgoing
packets that will need harvesting. Waiting for more interrupts
is less efficient than letting NAPI run a while longer while
the queue drains.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/net/ethernet/faraday/ftgmac100.c | 137 +++++++++++++++++--------------
 drivers/net/ethernet/faraday/ftgmac100.h |  14 ++++
 2 files changed, 90 insertions(+), 61 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index dc85d78..cbcbc92 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -83,7 +83,7 @@ struct ftgmac100 {
 	bool use_ncsi;
 
 	/* Misc */
-	int int_mask_all;
+	bool need_mac_restart;
 };
 
 static int ftgmac100_alloc_rx_page(struct ftgmac100 *priv,
@@ -1048,10 +1048,49 @@ static irqreturn_t ftgmac100_interrupt(int irq, void *dev_id)
 {
 	struct net_device *netdev = dev_id;
 	struct ftgmac100 *priv = netdev_priv(netdev);
+	unsigned int status, new_mask = FTGMAC100_INT_BAD;
 
-	/* Disable interrupts for polling */
-	iowrite32(0, priv->base + FTGMAC100_OFFSET_IER);
-	napi_schedule(&priv->napi);
+	/* Fetch and clear interrupt bits, process abnormal ones */
+	status = ioread32(priv->base + FTGMAC100_OFFSET_ISR);
+	iowrite32(status, priv->base + FTGMAC100_OFFSET_ISR);
+	if (unlikely(status & FTGMAC100_INT_BAD)) {
+
+		/* RX buffer unavailable */
+		if (status & FTGMAC100_INT_NO_RXBUF)
+			netdev->stats.rx_over_errors++;
+
+		/* received packet lost due to RX FIFO full */
+		if (status & FTGMAC100_INT_RPKT_LOST)
+			netdev->stats.rx_fifo_errors++;
+
+		/* sent packet lost due to excessive TX collision */
+		if (status & FTGMAC100_INT_XPKT_LOST)
+			netdev->stats.tx_fifo_errors++;
+
+		/* AHB error -> Reset the chip */
+		if (status & FTGMAC100_INT_AHB_ERR) {
+			if (net_ratelimit())
+				netdev_warn(netdev,
+					   "AHB bus error ! Resetting chip.\n");
+			iowrite32(0, priv->base + FTGMAC100_OFFSET_IER);
+			schedule_work(&priv->reset_task);
+			return IRQ_HANDLED;
+		}
+
+		/* We may need to restart the MAC after such errors, delay
+		 * this until after we have freed some Rx buffers though
+		 */
+		priv->need_mac_restart = true;
+
+		/* Disable those errors until we restart */
+		new_mask &= ~status;
+	}
+
+	/* Only enable "bad" interrupts while NAPI is on */
+	iowrite32(new_mask, priv->base + FTGMAC100_OFFSET_IER);
+
+	/* Schedule NAPI bh */
+	napi_schedule_irqoff(&priv->napi);
 
 	return IRQ_HANDLED;
 }
@@ -1059,68 +1098,51 @@ static irqreturn_t ftgmac100_interrupt(int irq, void *dev_id)
 static int ftgmac100_poll(struct napi_struct *napi, int budget)
 {
 	struct ftgmac100 *priv = container_of(napi, struct ftgmac100, napi);
-	struct net_device *netdev = priv->netdev;
-	unsigned int status;
-	bool completed = true;
+	bool more, completed = true;
 	int rx = 0;
 
-	status = ioread32(priv->base + FTGMAC100_OFFSET_ISR);
-	iowrite32(status, priv->base + FTGMAC100_OFFSET_ISR);
-
-	if (status & (FTGMAC100_INT_RPKT_BUF | FTGMAC100_INT_NO_RXBUF)) {
-		/*
-		 * FTGMAC100_INT_RPKT_BUF:
-		 *	RX DMA has received packets into RX buffer successfully
-		 *
-		 * FTGMAC100_INT_NO_RXBUF:
-		 *	RX buffer unavailable
-		 */
-		bool retry;
+	ftgmac100_tx_complete(priv);
 
-		do {
-			retry = ftgmac100_rx_packet(priv, &rx);
-		} while (retry && rx < budget);
+	do {
+		more = ftgmac100_rx_packet(priv, &rx);
+	} while (more && rx < budget);
 
-		if (retry && rx == budget)
-			completed = false;
-	}
+	if (more && rx == budget)
+		completed = false;
 
-	if (status & (FTGMAC100_INT_XPKT_ETH | FTGMAC100_INT_XPKT_LOST)) {
-		/*
-		 * FTGMAC100_INT_XPKT_ETH:
-		 *	packet transmitted to ethernet successfully
-		 *
-		 * FTGMAC100_INT_XPKT_LOST:
-		 *	packet transmitted to ethernet lost due to late
-		 *	collision or excessive collision
-		 */
-		ftgmac100_tx_complete(priv);
-	}
-
-	if (status & priv->int_mask_all & (FTGMAC100_INT_NO_RXBUF |
-			FTGMAC100_INT_RPKT_LOST | FTGMAC100_INT_AHB_ERR)) {
-		if (net_ratelimit())
-			netdev_info(netdev, "[ISR] = 0x%x: %s%s%s\n", status,
-				    status & FTGMAC100_INT_NO_RXBUF ? "NO_RXBUF " : "",
-				    status & FTGMAC100_INT_RPKT_LOST ? "RPKT_LOST " : "",
-				    status & FTGMAC100_INT_AHB_ERR ? "AHB_ERR " : "");
 
-		if (status & FTGMAC100_INT_NO_RXBUF) {
-			/* RX buffer unavailable */
-			netdev->stats.rx_over_errors++;
-		}
+	/* The interrupt is telling us to kick the MAC back to life
+	 * after an RX overflow
+	 */
+	if (unlikely(priv->need_mac_restart)) {
+		ftgmac100_start_hw(priv);
 
-		if (status & FTGMAC100_INT_RPKT_LOST) {
-			/* received packet lost due to RX FIFO full */
-			netdev->stats.rx_fifo_errors++;
-		}
+		/* Re-enable "bad" interrupts */
+		iowrite32(FTGMAC100_INT_BAD,
+			  priv->base + FTGMAC100_OFFSET_IER);
 	}
 
+	/* Keep NAPI going if we have still packets to reclaim */
+	if (priv->tx_pending)
+		return budget;
+
 	if (completed) {
+		/* We are about to re-enable all interrupts. However
+		 * the HW has been latching RX/TX packet interrupts while
+		 * they were masked. So we clear them first, then we need
+		 * to re-check if there's something to process
+		 */
+		iowrite32(FTGMAC100_INT_RXTX,
+			  priv->base + FTGMAC100_OFFSET_ISR);
+		if (ftgmac100_rxdes_packet_ready
+		    (ftgmac100_current_rxdes(priv)) || priv->tx_pending)
+			return budget;
+
+		/* deschedule NAPI */
 		napi_complete(napi);
 
 		/* enable all interrupts */
-		iowrite32(priv->int_mask_all,
+		iowrite32(FTGMAC100_INT_ALL,
 			  priv->base + FTGMAC100_OFFSET_IER);
 	}
 
@@ -1148,7 +1170,7 @@ static int ftgmac100_init_all(struct ftgmac100 *priv, bool ignore_alloc_err)
 	netif_start_queue(priv->netdev);
 
 	/* Enable all interrupts */
-	iowrite32(priv->int_mask_all, priv->base + FTGMAC100_OFFSET_IER);
+	iowrite32(FTGMAC100_INT_ALL, priv->base + FTGMAC100_OFFSET_IER);
 
 	return err;
 }
@@ -1489,13 +1511,6 @@ static int ftgmac100_probe(struct platform_device *pdev)
 	/* MAC address from chip or random one */
 	ftgmac100_setup_mac(priv);
 
-	priv->int_mask_all = (FTGMAC100_INT_RPKT_LOST |
-			      FTGMAC100_INT_XPKT_ETH |
-			      FTGMAC100_INT_XPKT_LOST |
-			      FTGMAC100_INT_AHB_ERR |
-			      FTGMAC100_INT_RPKT_BUF |
-			      FTGMAC100_INT_NO_RXBUF);
-
 	if (of_machine_is_compatible("aspeed,ast2400") ||
 	    of_machine_is_compatible("aspeed,ast2500")) {
 		priv->rxdes0_edorr_mask = BIT(30);
diff --git a/drivers/net/ethernet/faraday/ftgmac100.h b/drivers/net/ethernet/faraday/ftgmac100.h
index a7ce0ac..c4d5cc1 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.h
+++ b/drivers/net/ethernet/faraday/ftgmac100.h
@@ -86,6 +86,20 @@
 #define FTGMAC100_INT_PHYSTS_CHG	(1 << 9)
 #define FTGMAC100_INT_NO_HPTXBUF	(1 << 10)
 
+/* Interrupts we care about in NAPI mode */
+#define FTGMAC100_INT_BAD  (FTGMAC100_INT_RPKT_LOST | \
+			    FTGMAC100_INT_XPKT_LOST | \
+			    FTGMAC100_INT_AHB_ERR   | \
+			    FTGMAC100_INT_NO_RXBUF)
+
+/* Normal RX/TX interrupts, enabled when NAPI off */
+#define FTGMAC100_INT_RXTX (FTGMAC100_INT_XPKT_ETH  | \
+			    FTGMAC100_INT_RPKT_BUF)
+
+/* All the interrupts we care about */
+#define FTGMAC100_INT_ALL (FTGMAC100_INT_RPKT_BUF  |  \
+			   FTGMAC100_INT_BAD)
+
 /*
  * Interrupt timer control register
  */
-- 
2.9.3

Re: [PATCH 05/13] ftgmac100: Cleanup speed/duplex tracking and fix duplex config

[Andrew Lunn]

> +	if (new_speed) {
> +		netdev_info(netdev, "Link up at %d Mbit/s %s duplex\n",
> +			    new_speed,
> +			    phydev->duplex == DUPLEX_FULL ? "full" : "half");
> +	} else if (priv->cur_speed) {
> +		/* No link, just return. Leave the HW alone so it can
> +		 * continue draining the tx ring.
> +		 */
> +		netdev_info(netdev, "Link down\n");
>  		return;

Hi Ben

Please consider using phy_print_status().

       Andrew

Re: [PATCH 10/13] ftgmac100: Add a reset task and use it for link changes

[Andrew Lunn]

> +static void ftgmac100_reset_task(struct work_struct *work)
> +{
> +	struct ftgmac100 *priv = container_of(work, struct ftgmac100,
> +					      reset_task);
> +	struct net_device *netdev = priv->netdev;
> +	int err;
> +
> +	netdev_dbg(netdev, "Resetting NIC...\n");
> +
> +	/* Block PHY polling */
> +	if (netdev->phydev)
> +		mutex_lock(&netdev->phydev->lock);
> +
> +	rtnl_lock();

Hi Ben

Have you run lockdep tests on this? I think it is normal to take the
rtnl lock first, then the phydev lock. Try some ethtool operations and
see if you get a splat.

Also, do you need to be worried about mdio operations? Do you need to
take that lock as well, since there might be other PHYs, or an
Ethernet switch on the bus?

    Andrew

Re: [PATCH 10/13] ftgmac100: Add a reset task and use it for link changes

[Benjamin Herrenschmidt]

On Sun, 2017-04-02 at 13:35 +1000, Benjamin Herrenschmidt wrote:
> +       /* Block PHY polling */
> +       if (netdev->phydev)
> +               mutex_lock(&netdev->phydev->lock);
> +
> +       rtnl_lock();

Self-given brown paper bag, those two need to be taken the other way
around. I'll send an updated patch today.

Cheers,
Ben.

Re: [PATCH 05/13] ftgmac100: Cleanup speed/duplex tracking and fix duplex config

[Benjamin Herrenschmidt]

On Sun, 2017-04-02 at 20:28 +0200, Andrew Lunn wrote:
> > +	if (new_speed) {
> > +		netdev_info(netdev, "Link up at %d Mbit/s %s
> > duplex\n",
> > +			    new_speed,
> > +			    phydev->duplex == DUPLEX_FULL ? "full"
> > : "half");
> > +	} else if (priv->cur_speed) {
> > +		/* No link, just return. Leave the HW alone so it
> > can
> > +		 * continue draining the tx ring.
> > +		 */
> > +		netdev_info(netdev, "Link down\n");
> >  		return;
> 
> Hi Ben
> 
> Please consider using phy_print_status().

Thanks. I didn't know about that one. I'll update this.

Cheers,
Ben.

Re: [PATCH 10/13] ftgmac100: Add a reset task and use it for link changes

[Benjamin Herrenschmidt]

On Sun, 2017-04-02 at 20:42 +0200, Andrew Lunn wrote:
> Have you run lockdep tests on this? I think it is normal to take the
> rtnl lock first, then the phydev lock. Try some ethtool operations
> and see if you get a splat.

You are right. I actually realized that was wrong last night ;-) It
will splat with the order in open/close.

Just a stupid thinko (I added the phy lock in a second pass). I'll
fix that.

> Also, do you need to be worried about mdio operations? Do you need to
> take that lock as well, since there might be other PHYs, or an
> Ethernet switch on the bus?

There could be I suppose, none of the setups I've seen with those
SoC has done it but nothing prevents it, I'll have a look.

Thanks.
Ben.

Re: [PATCH 10/13] ftgmac100: Add a reset task and use it for link changes

[Benjamin Herrenschmidt]

On Sun, 2017-04-02 at 20:42 +0200, Andrew Lunn wrote:

> Have you run lockdep tests on this?

Good idea. For some reason I run it regularly on powerpc but it never
occurred to me to try it on that 800Mhz ARM11 :-)

Cheers,
Ben.

Re: [PATCH 05/13] ftgmac100: Cleanup speed/duplex tracking and fix duplex config

[Benjamin Herrenschmidt]

On Sun, 2017-04-02 at 13:35 +1000, Benjamin Herrenschmidt wrote:
> +       } else if (priv->cur_speed) {
> +               /* No link, just return. Leave the HW alone so it can
> +                * continue draining the tx ring.
> +                */
> +               netdev_info(netdev, "Link down\n");
>                 return;
> +       }
>  
> -       priv->old_speed = phydev->speed;
> +       priv->cur_speed = new_speed;
> +       priv->cur_duplex = phydev->duplex;

Finding my own bugs too ... the return above makes us fail
to update priv->cur_speed, thus the driver still thinks we
have a link. Not a huge deal but I'll fix too. (Caused by a
recent re-org in that code).

Cheers,
Ben.

Re: [PATCH 05/13] ftgmac100: Cleanup speed/duplex tracking and fix duplex config

[Benjamin Herrenschmidt]

On Sun, 2017-04-02 at 20:28 +0200, Andrew Lunn wrote:

> Please consider using phy_print_status().

So in a subsequent (not yet posted) patch, I add Pause support. I
noticed that phy_print_status() doesn't distinguish between full
and partial (asym) pause support.

Is that intentional ?

Otherwise, I'll submit a latch later to update it along with the
patch that adds pause support.

Cheers,
Ben.

Re: [PATCH 10/13] ftgmac100: Add a reset task and use it for link changes

[Benjamin Herrenschmidt]

On Mon, 2017-04-03 at 07:24 +1000, Benjamin Herrenschmidt wrote:
> On Sun, 2017-04-02 at 20:42 +0200, Andrew Lunn wrote:
> 
> > Have you run lockdep tests on this?
> 
> Good idea. For some reason I run it regularly on powerpc but it never
> occurred to me to try it on that 800Mhz ARM11 :-)

That was the only thing lockdep spotted. I added the MDIO mutex to
the list as well, I'll sent the respun series later in case there's
more reviews coming.

Thanks !

Cheers,
Ben.