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

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

[Benjamin Herrenschmidt]

This is version 2 of 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.

Version 2 addresses some review comments to patches 5 and 10
(see version history in the respective emails).

[PATCH v2 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 v2 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 v2 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 v2 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 v2 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>
--

v2. Use phy_print_status()
    Only bail out on link down *after* updating priv->cur_speed
---
 drivers/net/ethernet/faraday/ftgmac100.c | 52 +++++++++++++++++++++++++++-----
 1 file changed, 44 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index cc2271b..cc6e971 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,31 @@ 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;
 
-	priv->old_speed = phydev->speed;
+	/* Print status if we have a link or we had one and just lost it,
+	 * don't print otherwise.
+	 */
+	if (new_speed || priv->cur_speed)
+		phy_print_status(phydev);
+
+	priv->cur_speed = new_speed;
+	priv->cur_duplex = phydev->duplex;
+
+	/* Link is down, do nothing else */
+	if (!new_speed)
+		return;
 
 	ier = ioread32(priv->base + FTGMAC100_OFFSET_IER);
 
@@ -869,7 +891,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 +1111,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 +1135,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

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

[Andrew Lunn]

On Wed, Apr 05, 2017 at 12:28:45PM +1000, Benjamin Herrenschmidt wrote:
> 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>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH v2 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 cc6e971..0d0576f 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)
@@ -1099,12 +1115,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);
@@ -1168,8 +1192,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;
 }
 
@@ -1190,6 +1215,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 v2 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 0d0576f..bb444d2 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1158,6 +1158,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);
 
@@ -1188,6 +1191,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);
@@ -1207,6 +1211,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)
@@ -1379,9 +1384,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 v2 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 bb444d2..fdb8638 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1129,12 +1129,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
@@ -1161,6 +1155,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);
 
@@ -1191,12 +1192,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 v2 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 fdb8638..36f2905 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1109,10 +1109,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  */
@@ -1122,13 +1147,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
@@ -1162,26 +1180,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;
@@ -1189,16 +1202,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 v2 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>
--

v2. Fix lock ordering
    Add mdio_bus mutex
---
 drivers/net/ethernet/faraday/ftgmac100.c | 87 +++++++++++++++++++++++++++-----
 1 file changed, 74 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 36f2905..61f02bf 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)
@@ -897,20 +897,11 @@ static void ftgmac100_adjust_link(struct net_device *netdev)
 	if (!new_speed)
 		return;
 
-	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)
@@ -1135,6 +1126,61 @@ 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");
+
+	/* Lock the world */
+	rtnl_lock();
+	if (netdev->phydev)
+		mutex_lock(&netdev->phydev->lock);
+	if (priv->mii_bus)
+		mutex_lock(&priv->mii_bus->mdio_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:
+	if (priv->mii_bus)
+		mutex_unlock(&priv->mii_bus->mdio_lock);
+	if (netdev->phydev)
+		mutex_unlock(&netdev->phydev->lock);
+	rtnl_unlock();
+}
+
 static int ftgmac100_open(struct net_device *netdev)
 {
 	struct ftgmac100 *priv = netdev_priv(netdev);
@@ -1220,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);
 
@@ -1395,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);
 
@@ -1498,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

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

[Andrew Lunn]

On Wed, Apr 05, 2017 at 12:28:50PM +1000, Benjamin Herrenschmidt wrote:
> 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>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH v2 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 61f02bf..3adfb92 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 v2 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 3adfb92..4fa138b 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -1047,14 +1047,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 v2 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 4fa138b..88dab5f 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,
@@ -1046,10 +1046,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;
 }
@@ -1057,68 +1096,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);
 	}
 
@@ -1146,7 +1168,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 v2 00/13] ftgmac100: Rework batch 1 - Link & Interrupts

[Florian Fainelli]

Salut Benjamin,

Le 04/04/17 à 19:28, Benjamin Herrenschmidt a écrit :
> This is version 2 of 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.
> 
> Version 2 addresses some review comments to patches 5 and 10
> (see version history in the respective emails).
> 

This looks pretty good to me, two minor things:

- most drivers keep track of the old status/duplex/pause/link variables
instead of the current one which is already available within struct
phy_device, any particular reason for not doing like the other drivers?

- the need to reset the HW during link changes is just ... well too bad

With that:

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

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

[Benjamin Herrenschmidt]

On Tue, 2017-04-04 at 21:21 -0700, Florian Fainelli wrote:
> 
> This looks pretty good to me, two minor things:
> 
> - most drivers keep track of the old status/duplex/pause/link variables
> instead of the current one which is already available within struct
> phy_device, any particular reason for not doing like the other drivers?

We don't necessarily have a phydev attached when using NC-SI, so it was
easier to have the core code path not have to go fishing for those
settings in different places based on whether we're using NC-SI or not.

> - the need to reset the HW during link changes is just ... well too bad

Yup but there's little choice. The HW wants it. I don't see any real
point in optimizing that path mind you. Losing a few packets around
a link change isn't going to hurt and it keeps the code a lot simpler
by having a single "re-init" path.

> With that:
> 
> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Thanks !

I'll post batch 2 in the next couple of days which tackles the RX path.

Cheers,
Ben.

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

[Florian Fainelli]

On 04/04/2017 10:53 PM, Benjamin Herrenschmidt wrote:
> On Tue, 2017-04-04 at 21:21 -0700, Florian Fainelli wrote:
>>
>> This looks pretty good to me, two minor things:
>>
>> - most drivers keep track of the old status/duplex/pause/link variables
>> instead of the current one which is already available within struct
>> phy_device, any particular reason for not doing like the other drivers?
> 
> We don't necessarily have a phydev attached when using NC-SI, so it was
> easier to have the core code path not have to go fishing for those
> settings in different places based on whether we're using NC-SI or not.

Oh right, I missed that part. Is there a reason why NC-SI does not have
a PHY device attached? If not, could you somehow model the link using a
fixed PHY (which appears to Linux as a normal phy_device) just to keep
things simple.

> 
>> - the need to reset the HW during link changes is just ... well too bad
> 
> Yup but there's little choice. The HW wants it. I don't see any real
> point in optimizing that path mind you. Losing a few packets around
> a link change isn't going to hurt and it keeps the code a lot simpler
> by having a single "re-init" path.

I was just merely trying to say nicely: what a nicely broken piece of HW
(there were other adjectives coming to mind), and I do understand the pain.

> 
>> With that:
>>
>>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> 
> Thanks !
> 
> I'll post batch 2 in the next couple of days which tackles the RX path.

Cool, looking forward to that!
-- 
Florian

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

[Benjamin Herrenschmidt]

On Tue, 2017-04-04 at 23:02 -0700, Florian Fainelli wrote:

> We don't necessarily have a phydev attached when using NC-SI, so it was
> > easier to have the core code path not have to go fishing for those
> > settings in different places based on whether we're using NC-SI or not.
> 
> Oh right, I missed that part. Is there a reason why NC-SI does not have
> a PHY device attached? If not, could you somehow model the link using a
> fixed PHY (which appears to Linux as a normal phy_device) just to keep
> things simple.

Hrm ... maybe another day if you don't mind ;-)

First NC-SI isn't really a PHY .... it's a cross-over RMII connection
to another NIC.

Now we could make it a phydev using a "fixed" PHY I suppose, that just
"represents" the other end. That would be a way to do it. It would need
to have the link permanently up however (see below).

That said I do want to tackle making it some kind of pseudo-PHY that
actually reflects the state of the remote end (especially the link
state, ie. up/down).

However there are a couple of issues to tackle if we do that. Well
mostly one annoying one:

NC-SI needs to talk to the remote NIC via specific ethernet frames.

With the current link watch code however, if we reflect the remote link
to the local NIC link via netif_carrier_on/off, we end up deactivating
the device on link off and thus preventing the NC-SI stack from talking
to the peer NIC at all.

I thought a while ago we could add some dev flag to prevent the link
watch from doing that, but never got to look into it myself and
apparently neither did Gavin.

So yes, those are worthwhile improvements and I can probably tackle
them once I've unpiled a dozen other train wrecks from my plate ;)
However I'd like to not block this series further since it's not
actually making things any worse than they are.

> > > - the need to reset the HW during link changes is just ... well too bad
> > 
> > Yup but there's little choice. The HW wants it. I don't see any real
> > point in optimizing that path mind you. Losing a few packets around
> > a link change isn't going to hurt and it keeps the code a lot simpler
> > by having a single "re-init" path.
> 
> I was just merely trying to say nicely: what a nicely broken piece of HW
> (there were other adjectives coming to mind), and I do understand the pain.

:-) At least I got a register spec (and little more) :-)

It looks like those Aspeed BMCs are the only game in town for BMC chips
these days and they use that "interesting" IP block from Faraday so
this is probably here to stay, at least for a while.

Another "interesting" attribute of that piece of c^Hhw is its handling
of receive descriptors.

It doesn't "count" how many are free. It has to constantly "read" the
head descriptor in the RX ring to check the own bit. So you have to
setup a HW timer for the chip to go "poll" on your memory. It's pretty
insane. At least for TX there's an MMIO you can poke to tell it to go
fetch more. There's sort-of one for RX but it doesn't seem to do what
you would expect, or I did something wrong when playing with it.

It's not like it would have been hard to have a counter, which is
incremented by writing a value to a register so Linux can "provide"
descriptors by writing the number freed in there.

So the chip never really knows how many free descriptors it has which
also means it cannot do flow control based on that, only on the FIFO
threshold. With a 2K only FIFO that's .... interesting.

Anyway, it sort-of works. Without my patches I maxed out at about
80Mbit/s iperf on a gigabit link with the AST2500 eval board (ARM11
800Mhz base). With my patches I get to about 400Mbit/s.

Cheers.
Ben.

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

[Florian Fainelli]

On 04/04/2017 11:31 PM, Benjamin Herrenschmidt wrote:
> On Tue, 2017-04-04 at 23:02 -0700, Florian Fainelli wrote:
> 
>> We don't necessarily have a phydev attached when using NC-SI, so it was
>>> easier to have the core code path not have to go fishing for those
>>> settings in different places based on whether we're using NC-SI or not.
>>
>> Oh right, I missed that part. Is there a reason why NC-SI does not have
>> a PHY device attached? If not, could you somehow model the link using a
>> fixed PHY (which appears to Linux as a normal phy_device) just to keep
>> things simple.
> 
> Hrm ... maybe another day if you don't mind ;-)
> 
> First NC-SI isn't really a PHY .... it's a cross-over RMII connection
> to another NIC.
> 
> Now we could make it a phydev using a "fixed" PHY I suppose, that just
> "represents" the other end. That would be a way to do it. It would need
> to have the link permanently up however (see below).
> 
> That said I do want to tackle making it some kind of pseudo-PHY that
> actually reflects the state of the remote end (especially the link
> state, ie. up/down).
> 
> However there are a couple of issues to tackle if we do that. Well
> mostly one annoying one:
> 
> NC-SI needs to talk to the remote NIC via specific ethernet frames.
> 
> With the current link watch code however, if we reflect the remote link
> to the local NIC link via netif_carrier_on/off, we end up deactivating
> the device on link off and thus preventing the NC-SI stack from talking
> to the peer NIC at all.
> 
> I thought a while ago we could add some dev flag to prevent the link
> watch from doing that, but never got to look into it myself and
> apparently neither did Gavin.

It sounds like a similar situation to e.g: 802.1x, you want to let some
frames make it through and you need a working link for that, but you
can't quite flag the device as UP entirely. Maybe you can find a way to
re-use IFF_LOWER_UP to that purpose and only set that flag based on the
NC-SI frames indicating link state?
-- 
Florian

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

[Benjamin Herrenschmidt]

On Thu, 2017-04-06 at 12:46 -0700, Florian Fainelli wrote:
> > I thought a while ago we could add some dev flag to prevent the link
> > watch from doing that, but never got to look into it myself and
> > apparently neither did Gavin.
> 
> It sounds like a similar situation to e.g: 802.1x, you want to let some
> frames make it through and you need a working link for that, but you
> can't quite flag the device as UP entirely. Maybe you can find a way to
> re-use IFF_LOWER_UP to that purpose and only set that flag based on the
> NC-SI frames indicating link state?

Possibly yes, I wasn't familiar with the difference between 
"carrier" and IFF_LOWER_UP so far. I'll have a look or have Gavin do
so.

It would be nice to reflect that properly so things like fallover or
bonding do the right thing. On the other hand I yet have to see a
system where the BMC has more than one NC-SI link to a NIC (what they
usually have is that NC-SI is connected to multiple NICs which is a
different matter and should be handled within the NC-SI stack).

Anyway, I'll look into it further down the track along with other
improvements I'd like to do to the NC-SI stack such as properly setting
up multicast and vlan filters in the peer NIC and various cleanups.

Cheers,
Ben.

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

[David Miller]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Wed,  5 Apr 2017 12:28:40 +1000

> This is version 2 of 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.
> 
> Version 2 addresses some review comments to patches 5 and 10
> (see version history in the respective emails).

Series applied.