linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/10] net/macb: driver enhancement concerning GEM support, ring logic and cleanup
@ 2012-09-05  8:19 Nicolas Ferre
  2012-09-05  8:19 ` [PATCH 01/10] net/macb: Add support for Gigabit Ethernet mode Nicolas Ferre
                   ` (9 more replies)
  0 siblings, 10 replies; 23+ messages in thread
From: Nicolas Ferre @ 2012-09-05  8:19 UTC (permalink / raw)
  To: netdev
  Cc: linux-arm-kernel, davem, havard, nicolas.ferre, plagnioj, jamie,
	linux-kernel, patrice.vilchez

This is an enhancement work that began several years ago. I try to catchup with
some performance improvement that has been implemented then by Havard.
The ring index logic and the TX error path modification are the biggest changes
but some cleanup/debugging have been added along the way.
The GEM revision will benefit from the Gigabit support.

The series has been tested on several Atmel AT91 SoC with the two MACB/GEM
flavors.

Havard Skinnemoen (5):
  net/macb: memory barriers cleanup
  net/macb: change debugging messages
  net/macb: Fix a race in macb_start_xmit()
  net/macb: clean up ring buffer logic
  net/macb: Offset first RX buffer by two bytes

Nicolas Ferre (4):
  net/macb: better manage tx errors
  net/macb: tx status is more than 8 bits now
  net/macb: macb_get_drvinfo: add GEM/MACB suffix to differentiate
    revision
  net/macb: ethtool interface: add register dump feature

Patrice Vilchez (1):
  net/macb: Add support for Gigabit Ethernet mode

 drivers/net/ethernet/cadence/macb.c |  408 ++++++++++++++++++++++++-----------
 drivers/net/ethernet/cadence/macb.h |   29 ++-
 2 files changed, 304 insertions(+), 133 deletions(-)

-- 
1.7.10


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 01/10] net/macb: Add support for Gigabit Ethernet mode
  2012-09-05  8:19 [PATCH 00/10] net/macb: driver enhancement concerning GEM support, ring logic and cleanup Nicolas Ferre
@ 2012-09-05  8:19 ` Nicolas Ferre
  2012-09-05  8:19 ` [PATCH 02/10] net/macb: memory barriers cleanup Nicolas Ferre
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 23+ messages in thread
From: Nicolas Ferre @ 2012-09-05  8:19 UTC (permalink / raw)
  To: netdev
  Cc: linux-arm-kernel, davem, havard, nicolas.ferre, plagnioj, jamie,
	linux-kernel, patrice.vilchez

From: Patrice Vilchez <patrice.vilchez@atmel.com>

Add Gigabit Ethernet mode to GEM cadence IP and enable RGMII connection.

Signed-off-by: Patrice Vilchez <patrice.vilchez@atmel.com>
Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/net/ethernet/cadence/macb.c |   15 ++++++++++++---
 drivers/net/ethernet/cadence/macb.h |    4 ++++
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 033064b..9a10f69 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -152,13 +152,17 @@ static void macb_handle_link_change(struct net_device *dev)
 
 			reg = macb_readl(bp, NCFGR);
 			reg &= ~(MACB_BIT(SPD) | MACB_BIT(FD));
+			if (macb_is_gem(bp))
+				reg &= ~GEM_BIT(GBE);
 
 			if (phydev->duplex)
 				reg |= MACB_BIT(FD);
 			if (phydev->speed == SPEED_100)
 				reg |= MACB_BIT(SPD);
+			if (phydev->speed == SPEED_1000)
+				reg |= GEM_BIT(GBE);
 
-			macb_writel(bp, NCFGR, reg);
+			macb_or_gem_writel(bp, NCFGR, reg);
 
 			bp->speed = phydev->speed;
 			bp->duplex = phydev->duplex;
@@ -216,7 +220,10 @@ static int macb_mii_probe(struct net_device *dev)
 	}
 
 	/* mask with MAC supported features */
-	phydev->supported &= PHY_BASIC_FEATURES;
+	if (macb_is_gem(bp))
+		phydev->supported &= PHY_GBIT_FEATURES;
+	else
+		phydev->supported &= PHY_BASIC_FEATURES;
 
 	phydev->advertising = phydev->supported;
 
@@ -1384,7 +1391,9 @@ static int __init macb_probe(struct platform_device *pdev)
 		bp->phy_interface = err;
 	}
 
-	if (bp->phy_interface == PHY_INTERFACE_MODE_RMII)
+	if (bp->phy_interface == PHY_INTERFACE_MODE_RGMII)
+		macb_or_gem_writel(bp, USRIO, GEM_BIT(RGMII));
+	else if (bp->phy_interface == PHY_INTERFACE_MODE_RMII)
 #if defined(CONFIG_ARCH_AT91)
 		macb_or_gem_writel(bp, USRIO, (MACB_BIT(RMII) |
 					       MACB_BIT(CLKEN)));
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 335e288..f69ceef 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -145,6 +145,8 @@
 #define MACB_IRXFCS_SIZE			1
 
 /* GEM specific NCFGR bitfields. */
+#define GEM_GBE_OFFSET				10
+#define GEM_GBE_SIZE				1
 #define GEM_CLK_OFFSET				18
 #define GEM_CLK_SIZE				3
 #define GEM_DBW_OFFSET				21
@@ -246,6 +248,8 @@
 /* Bitfields in USRIO (AT91) */
 #define MACB_RMII_OFFSET			0
 #define MACB_RMII_SIZE				1
+#define GEM_RGMII_OFFSET			0	/* GEM gigabit mode */
+#define GEM_RGMII_SIZE				1
 #define MACB_CLKEN_OFFSET			1
 #define MACB_CLKEN_SIZE				1
 
-- 
1.7.10


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 02/10] net/macb: memory barriers cleanup
  2012-09-05  8:19 [PATCH 00/10] net/macb: driver enhancement concerning GEM support, ring logic and cleanup Nicolas Ferre
  2012-09-05  8:19 ` [PATCH 01/10] net/macb: Add support for Gigabit Ethernet mode Nicolas Ferre
@ 2012-09-05  8:19 ` Nicolas Ferre
  2012-09-05  8:19 ` [PATCH 03/10] net/macb: change debugging messages Nicolas Ferre
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 23+ messages in thread
From: Nicolas Ferre @ 2012-09-05  8:19 UTC (permalink / raw)
  To: netdev
  Cc: linux-arm-kernel, davem, havard, nicolas.ferre, plagnioj, jamie,
	linux-kernel, patrice.vilchez

From: Havard Skinnemoen <havard@skinnemoen.net>

Remove a couple of unneeded barriers and document the remaining ones.

Signed-off-by: Havard Skinnemoen <havard@skinnemoen.net>
[nicolas.ferre@atmel.com: split patch in topics]
Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/net/ethernet/cadence/macb.c |   18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 9a10f69..26ca01e 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -372,7 +372,9 @@ static void macb_tx(struct macb *bp)
 
 		BUG_ON(skb == NULL);
 
+		/* Make hw descriptor updates visible to CPU */
 		rmb();
+
 		bufstat = bp->tx_ring[tail].ctrl;
 
 		if (!(bufstat & MACB_BIT(TX_USED)))
@@ -415,7 +417,10 @@ static int macb_rx_frame(struct macb *bp, unsigned int first_frag,
 			if (frag == last_frag)
 				break;
 		}
+
+		/* Make descriptor updates visible to hardware */
 		wmb();
+
 		return 1;
 	}
 
@@ -436,12 +441,14 @@ static int macb_rx_frame(struct macb *bp, unsigned int first_frag,
 					       frag_len);
 		offset += RX_BUFFER_SIZE;
 		bp->rx_ring[frag].addr &= ~MACB_BIT(RX_USED);
-		wmb();
 
 		if (frag == last_frag)
 			break;
 	}
 
+	/* Make descriptor updates visible to hardware */
+	wmb();
+
 	skb->protocol = eth_type_trans(skb, bp->dev);
 
 	bp->stats.rx_packets++;
@@ -461,6 +468,8 @@ static void discard_partial_frame(struct macb *bp, unsigned int begin,
 
 	for (frag = begin; frag != end; frag = NEXT_RX(frag))
 		bp->rx_ring[frag].addr &= ~MACB_BIT(RX_USED);
+
+	/* Make descriptor updates visible to hardware */
 	wmb();
 
 	/*
@@ -479,7 +488,9 @@ static int macb_rx(struct macb *bp, int budget)
 	for (; budget > 0; tail = NEXT_RX(tail)) {
 		u32 addr, ctrl;
 
+		/* Make hw descriptor updates visible to CPU */
 		rmb();
+
 		addr = bp->rx_ring[tail].addr;
 		ctrl = bp->rx_ring[tail].ctrl;
 
@@ -674,6 +685,8 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	bp->tx_ring[entry].addr = mapping;
 	bp->tx_ring[entry].ctrl = ctrl;
+
+	/* Make newly initialized descriptor visible to hardware */
 	wmb();
 
 	entry = NEXT_TX(entry);
@@ -782,9 +795,6 @@ static void macb_init_rings(struct macb *bp)
 
 static void macb_reset_hw(struct macb *bp)
 {
-	/* Make sure we have the write buffer for ourselves */
-	wmb();
-
 	/*
 	 * Disable RX and TX (XXX: Should we halt the transmission
 	 * more gracefully?)
-- 
1.7.10


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 03/10] net/macb: change debugging messages
  2012-09-05  8:19 [PATCH 00/10] net/macb: driver enhancement concerning GEM support, ring logic and cleanup Nicolas Ferre
  2012-09-05  8:19 ` [PATCH 01/10] net/macb: Add support for Gigabit Ethernet mode Nicolas Ferre
  2012-09-05  8:19 ` [PATCH 02/10] net/macb: memory barriers cleanup Nicolas Ferre
@ 2012-09-05  8:19 ` Nicolas Ferre
  2012-09-05  8:19 ` [PATCH 04/10] net/macb: Fix a race in macb_start_xmit() Nicolas Ferre
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 23+ messages in thread
From: Nicolas Ferre @ 2012-09-05  8:19 UTC (permalink / raw)
  To: netdev
  Cc: linux-arm-kernel, davem, havard, nicolas.ferre, plagnioj, jamie,
	linux-kernel, patrice.vilchez

From: Havard Skinnemoen <havard@skinnemoen.net>

Convert some noisy netdev_dbg() statements to netdev_vdbg(). Defining
DEBUG will no longer fill up the logs; VERBOSE_DEBUG still does.
Add one more verbose debug for ISR status.

Signed-off-by: Havard Skinnemoen <havard@skinnemoen.net>
[nicolas.ferre@atmel.com: split patch in topics, add ISR status]
Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/net/ethernet/cadence/macb.c |   22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 26ca01e..2228dfc 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -313,7 +313,7 @@ static void macb_tx(struct macb *bp)
 	status = macb_readl(bp, TSR);
 	macb_writel(bp, TSR, status);
 
-	netdev_dbg(bp->dev, "macb_tx status = %02lx\n", (unsigned long)status);
+	netdev_vdbg(bp->dev, "macb_tx status = %02lx\n", (unsigned long)status);
 
 	if (status & (MACB_BIT(UND) | MACB_BIT(TSR_RLE))) {
 		int i;
@@ -380,7 +380,7 @@ static void macb_tx(struct macb *bp)
 		if (!(bufstat & MACB_BIT(TX_USED)))
 			break;
 
-		netdev_dbg(bp->dev, "skb %u (data %p) TX complete\n",
+		netdev_vdbg(bp->dev, "skb %u (data %p) TX complete\n",
 			   tail, skb->data);
 		dma_unmap_single(&bp->pdev->dev, rp->mapping, skb->len,
 				 DMA_TO_DEVICE);
@@ -406,7 +406,7 @@ static int macb_rx_frame(struct macb *bp, unsigned int first_frag,
 
 	len = MACB_BFEXT(RX_FRMLEN, bp->rx_ring[last_frag].ctrl);
 
-	netdev_dbg(bp->dev, "macb_rx_frame frags %u - %u (len %u)\n",
+	netdev_vdbg(bp->dev, "macb_rx_frame frags %u - %u (len %u)\n",
 		   first_frag, last_frag, len);
 
 	skb = netdev_alloc_skb(bp->dev, len + RX_OFFSET);
@@ -453,7 +453,7 @@ static int macb_rx_frame(struct macb *bp, unsigned int first_frag,
 
 	bp->stats.rx_packets++;
 	bp->stats.rx_bytes += len;
-	netdev_dbg(bp->dev, "received skb of length %u, csum: %08x\n",
+	netdev_vdbg(bp->dev, "received skb of length %u, csum: %08x\n",
 		   skb->len, skb->csum);
 	netif_receive_skb(skb);
 
@@ -535,7 +535,7 @@ static int macb_poll(struct napi_struct *napi, int budget)
 
 	work_done = 0;
 
-	netdev_dbg(bp->dev, "poll: status = %08lx, budget = %d\n",
+	netdev_vdbg(bp->dev, "poll: status = %08lx, budget = %d\n",
 		   (unsigned long)status, budget);
 
 	work_done = macb_rx(bp, budget);
@@ -574,6 +574,8 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
 			break;
 		}
 
+		netdev_vdbg(bp->dev, "isr = 0x%08lx\n", (unsigned long)status);
+
 		if (status & MACB_RX_INT_FLAGS) {
 			/*
 			 * There's no point taking any more interrupts
@@ -585,7 +587,7 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
 			macb_writel(bp, IDR, MACB_RX_INT_FLAGS);
 
 			if (napi_schedule_prep(&bp->napi)) {
-				netdev_dbg(bp->dev, "scheduling RX softirq\n");
+				netdev_vdbg(bp->dev, "scheduling RX softirq\n");
 				__napi_schedule(&bp->napi);
 			}
 		}
@@ -647,8 +649,8 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	u32 ctrl;
 	unsigned long flags;
 
-#ifdef DEBUG
-	netdev_dbg(bp->dev,
+#if defined(DEBUG) && defined(VERBOSE_DEBUG)
+	netdev_vdbg(bp->dev,
 		   "start_xmit: len %u head %p data %p tail %p end %p\n",
 		   skb->len, skb->head, skb->data,
 		   skb_tail_pointer(skb), skb_end_pointer(skb));
@@ -670,12 +672,12 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	entry = bp->tx_head;
-	netdev_dbg(bp->dev, "Allocated ring entry %u\n", entry);
+	netdev_vdbg(bp->dev, "Allocated ring entry %u\n", entry);
 	mapping = dma_map_single(&bp->pdev->dev, skb->data,
 				 len, DMA_TO_DEVICE);
 	bp->tx_skb[entry].skb = skb;
 	bp->tx_skb[entry].mapping = mapping;
-	netdev_dbg(bp->dev, "Mapped skb data %p to DMA addr %08lx\n",
+	netdev_vdbg(bp->dev, "Mapped skb data %p to DMA addr %08lx\n",
 		   skb->data, (unsigned long)mapping);
 
 	ctrl = MACB_BF(TX_FRMLEN, len);
-- 
1.7.10


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 04/10] net/macb: Fix a race in macb_start_xmit()
  2012-09-05  8:19 [PATCH 00/10] net/macb: driver enhancement concerning GEM support, ring logic and cleanup Nicolas Ferre
                   ` (2 preceding siblings ...)
  2012-09-05  8:19 ` [PATCH 03/10] net/macb: change debugging messages Nicolas Ferre
@ 2012-09-05  8:19 ` Nicolas Ferre
  2012-09-05 21:30   ` David Miller
  2012-09-05  9:00 ` [PATCH 05/10] net/macb: clean up ring buffer logic Nicolas Ferre
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 23+ messages in thread
From: Nicolas Ferre @ 2012-09-05  8:19 UTC (permalink / raw)
  To: netdev
  Cc: linux-arm-kernel, davem, havard, nicolas.ferre, plagnioj, jamie,
	linux-kernel, patrice.vilchez

From: Havard Skinnemoen <havard@skinnemoen.net>

Fix a race in macb_start_xmit() where we unconditionally set the TSTART bit.
If an underrun just happened (we do this with interrupts disabled, so it might
not have been handled yet), the controller starts transmitting from the first
entry in the ring, which is usually wrong.
Restart the controller after error handling.

Signed-off-by: Havard Skinnemoen <havard@skinnemoen.net>
[nicolas.ferre@atmel.com: split patch in topics]
Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/net/ethernet/cadence/macb.c |   20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 2228dfc..f4b8adf 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -390,6 +390,13 @@ static void macb_tx(struct macb *bp)
 		dev_kfree_skb_irq(skb);
 	}
 
+	/*
+	 * Someone may have submitted a new frame while this interrupt
+	 * was pending, or we may just have handled an error.
+	 */
+	if (head != tail && !(status & MACB_BIT(TGO)))
+		macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
+
 	bp->tx_tail = tail;
 	if (netif_queue_stopped(bp->dev) &&
 	    TX_BUFFS_AVAIL(bp) > MACB_TX_WAKEUP_THRESH)
@@ -696,7 +703,18 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	skb_tx_timestamp(skb);
 
-	macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
+	/*
+	 * Only start the controller if the queue was empty; otherwise
+	 * we may race against the hardware resetting the ring pointer
+	 * due to a transmit error.
+	 *
+	 * If the controller is idle but the queue isn't empty, there
+	 * must be a pending interrupt that will trigger as soon as we
+	 * re-enable interrupts, and the interrupt handler will make
+	 * sure the controler is started.
+	 */
+	if (NEXT_TX(bp->tx_tail) == bp->tx_head)
+		macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
 
 	if (TX_BUFFS_AVAIL(bp) < 1)
 		netif_stop_queue(dev);
-- 
1.7.10


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 05/10] net/macb: clean up ring buffer logic
  2012-09-05  8:19 [PATCH 00/10] net/macb: driver enhancement concerning GEM support, ring logic and cleanup Nicolas Ferre
                   ` (3 preceding siblings ...)
  2012-09-05  8:19 ` [PATCH 04/10] net/macb: Fix a race in macb_start_xmit() Nicolas Ferre
@ 2012-09-05  9:00 ` Nicolas Ferre
  2012-09-05  9:00 ` [PATCH 06/10] net/macb: better manage tx errors Nicolas Ferre
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 23+ messages in thread
From: Nicolas Ferre @ 2012-09-05  9:00 UTC (permalink / raw)
  To: netdev
  Cc: linux-arm-kernel, davem, havard, nicolas.ferre, plagnioj, jamie,
	linux-kernel, patrice.vilchez

From: Havard Skinnemoen <havard@skinnemoen.net>

Instead of masking head and tail every time we increment them, just let them
wrap through UINT_MAX and mask them when subscripting. Add simple accessor
functions to do the subscripting properly to minimize the chances of messing
this up.

This makes the code slightly smaller, and hopefully faster as well.  Also,
doing the ring buffer management this way will simplify things a lot when
making the ring sizes configurable in the future.

Signed-off-by: Havard Skinnemoen <havard@skinnemoen.net>
[nicolas.ferre@atmel.com: split patch in topics, adapt to newer kernel]
Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/net/ethernet/cadence/macb.c |  170 ++++++++++++++++++++++-------------
 drivers/net/ethernet/cadence/macb.h |   22 +++--
 2 files changed, 123 insertions(+), 69 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index f4b8adf..3d3a077 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -31,24 +31,13 @@
 
 #define RX_BUFFER_SIZE		128
 #define RX_RING_SIZE		512
-#define RX_RING_BYTES		(sizeof(struct dma_desc) * RX_RING_SIZE)
+#define RX_RING_BYTES		(sizeof(struct macb_dma_desc) * RX_RING_SIZE)
 
 /* Make the IP header word-aligned (the ethernet header is 14 bytes) */
 #define RX_OFFSET		2
 
 #define TX_RING_SIZE		128
-#define DEF_TX_RING_PENDING	(TX_RING_SIZE - 1)
-#define TX_RING_BYTES		(sizeof(struct dma_desc) * TX_RING_SIZE)
-
-#define TX_RING_GAP(bp)						\
-	(TX_RING_SIZE - (bp)->tx_pending)
-#define TX_BUFFS_AVAIL(bp)					\
-	(((bp)->tx_tail <= (bp)->tx_head) ?			\
-	 (bp)->tx_tail + (bp)->tx_pending - (bp)->tx_head :	\
-	 (bp)->tx_tail - (bp)->tx_head - TX_RING_GAP(bp))
-#define NEXT_TX(n)		(((n) + 1) & (TX_RING_SIZE - 1))
-
-#define NEXT_RX(n)		(((n) + 1) & (RX_RING_SIZE - 1))
+#define TX_RING_BYTES		(sizeof(struct macb_dma_desc) * TX_RING_SIZE)
 
 /* minimum number of free TX descriptors before waking up TX process */
 #define MACB_TX_WAKEUP_THRESH	(TX_RING_SIZE / 4)
@@ -56,6 +45,51 @@
 #define MACB_RX_INT_FLAGS	(MACB_BIT(RCOMP) | MACB_BIT(RXUBR)	\
 				 | MACB_BIT(ISR_ROVR))
 
+/* Ring buffer accessors */
+static unsigned int macb_tx_ring_wrap(unsigned int index)
+{
+	return index & (TX_RING_SIZE - 1);
+}
+
+static unsigned int macb_tx_ring_avail(struct macb *bp)
+{
+	return TX_RING_SIZE - (bp->tx_head - bp->tx_tail);
+}
+
+static struct macb_dma_desc *macb_tx_desc(struct macb *bp, unsigned int index)
+{
+	return &bp->tx_ring[macb_tx_ring_wrap(index)];
+}
+
+static struct macb_tx_skb *macb_tx_skb(struct macb *bp, unsigned int index)
+{
+	return &bp->tx_skb[macb_tx_ring_wrap(index)];
+}
+
+static dma_addr_t macb_tx_dma(struct macb *bp, unsigned int index)
+{
+	dma_addr_t offset;
+
+	offset = macb_tx_ring_wrap(index) * sizeof(struct macb_dma_desc);
+
+	return bp->tx_ring_dma + offset;
+}
+
+static unsigned int macb_rx_ring_wrap(unsigned int index)
+{
+	return index & (RX_RING_SIZE - 1);
+}
+
+static struct macb_dma_desc *macb_rx_desc(struct macb *bp, unsigned int index)
+{
+	return &bp->rx_ring[macb_rx_ring_wrap(index)];
+}
+
+static void *macb_rx_buffer(struct macb *bp, unsigned int index)
+{
+	return bp->rx_buffers + RX_BUFFER_SIZE * macb_rx_ring_wrap(index);
+}
+
 static void __macb_set_hwaddr(struct macb *bp)
 {
 	u32 bottom;
@@ -335,17 +369,18 @@ static void macb_tx(struct macb *bp)
 		bp->tx_ring[TX_RING_SIZE - 1].ctrl |= MACB_BIT(TX_WRAP);
 
 		/* free transmit buffer in upper layer*/
-		for (tail = bp->tx_tail; tail != head; tail = NEXT_TX(tail)) {
-			struct ring_info *rp = &bp->tx_skb[tail];
-			struct sk_buff *skb = rp->skb;
-
-			BUG_ON(skb == NULL);
+		for (tail = bp->tx_tail; tail != head; tail++) {
+			struct macb_tx_skb	*tx_skb;
+			struct sk_buff		*skb;
 
 			rmb();
 
-			dma_unmap_single(&bp->pdev->dev, rp->mapping, skb->len,
-							 DMA_TO_DEVICE);
-			rp->skb = NULL;
+			tx_skb = macb_tx_skb(bp, tail);
+			skb = tx_skb->skb;
+
+			dma_unmap_single(&bp->pdev->dev, tx_skb->mapping,
+						skb->len, DMA_TO_DEVICE);
+			tx_skb->skb = NULL;
 			dev_kfree_skb_irq(skb);
 		}
 
@@ -365,28 +400,32 @@ static void macb_tx(struct macb *bp)
 		return;
 
 	head = bp->tx_head;
-	for (tail = bp->tx_tail; tail != head; tail = NEXT_TX(tail)) {
-		struct ring_info *rp = &bp->tx_skb[tail];
-		struct sk_buff *skb = rp->skb;
-		u32 bufstat;
+	for (tail = bp->tx_tail; tail != head; tail++) {
+		struct macb_tx_skb	*tx_skb;
+		struct sk_buff		*skb;
+		struct macb_dma_desc	*desc;
+		u32			ctrl;
 
-		BUG_ON(skb == NULL);
+		desc = macb_tx_desc(bp, tail);
 
 		/* Make hw descriptor updates visible to CPU */
 		rmb();
 
-		bufstat = bp->tx_ring[tail].ctrl;
+		ctrl = desc->ctrl;
 
-		if (!(bufstat & MACB_BIT(TX_USED)))
+		if (!(ctrl & MACB_BIT(TX_USED)))
 			break;
 
+		tx_skb = macb_tx_skb(bp, tail);
+		skb = tx_skb->skb;
+
 		netdev_vdbg(bp->dev, "skb %u (data %p) TX complete\n",
-			   tail, skb->data);
-		dma_unmap_single(&bp->pdev->dev, rp->mapping, skb->len,
+			macb_tx_ring_wrap(tail), skb->data);
+		dma_unmap_single(&bp->pdev->dev, tx_skb->mapping, skb->len,
 				 DMA_TO_DEVICE);
 		bp->stats.tx_packets++;
 		bp->stats.tx_bytes += skb->len;
-		rp->skb = NULL;
+		tx_skb->skb = NULL;
 		dev_kfree_skb_irq(skb);
 	}
 
@@ -398,8 +437,8 @@ static void macb_tx(struct macb *bp)
 		macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
 
 	bp->tx_tail = tail;
-	if (netif_queue_stopped(bp->dev) &&
-	    TX_BUFFS_AVAIL(bp) > MACB_TX_WAKEUP_THRESH)
+	if (netif_queue_stopped(bp->dev)
+			&& macb_tx_ring_avail(bp) > MACB_TX_WAKEUP_THRESH)
 		netif_wake_queue(bp->dev);
 }
 
@@ -410,17 +449,21 @@ static int macb_rx_frame(struct macb *bp, unsigned int first_frag,
 	unsigned int frag;
 	unsigned int offset = 0;
 	struct sk_buff *skb;
+	struct macb_dma_desc *desc;
 
-	len = MACB_BFEXT(RX_FRMLEN, bp->rx_ring[last_frag].ctrl);
+	desc = macb_rx_desc(bp, last_frag);
+	len = MACB_BFEXT(RX_FRMLEN, desc->ctrl);
 
 	netdev_vdbg(bp->dev, "macb_rx_frame frags %u - %u (len %u)\n",
-		   first_frag, last_frag, len);
+		macb_rx_ring_wrap(first_frag),
+		macb_rx_ring_wrap(last_frag), len);
 
 	skb = netdev_alloc_skb(bp->dev, len + RX_OFFSET);
 	if (!skb) {
 		bp->stats.rx_dropped++;
-		for (frag = first_frag; ; frag = NEXT_RX(frag)) {
-			bp->rx_ring[frag].addr &= ~MACB_BIT(RX_USED);
+		for (frag = first_frag; ; frag++) {
+			desc = macb_rx_desc(bp, frag);
+			desc->addr &= ~MACB_BIT(RX_USED);
 			if (frag == last_frag)
 				break;
 		}
@@ -435,7 +478,7 @@ static int macb_rx_frame(struct macb *bp, unsigned int first_frag,
 	skb_checksum_none_assert(skb);
 	skb_put(skb, len);
 
-	for (frag = first_frag; ; frag = NEXT_RX(frag)) {
+	for (frag = first_frag; ; frag++) {
 		unsigned int frag_len = RX_BUFFER_SIZE;
 
 		if (offset + frag_len > len) {
@@ -443,11 +486,10 @@ static int macb_rx_frame(struct macb *bp, unsigned int first_frag,
 			frag_len = len - offset;
 		}
 		skb_copy_to_linear_data_offset(skb, offset,
-					       (bp->rx_buffers +
-					        (RX_BUFFER_SIZE * frag)),
-					       frag_len);
+				macb_rx_buffer(bp, frag), frag_len);
 		offset += RX_BUFFER_SIZE;
-		bp->rx_ring[frag].addr &= ~MACB_BIT(RX_USED);
+		desc = macb_rx_desc(bp, frag);
+		desc->addr &= ~MACB_BIT(RX_USED);
 
 		if (frag == last_frag)
 			break;
@@ -473,8 +515,10 @@ static void discard_partial_frame(struct macb *bp, unsigned int begin,
 {
 	unsigned int frag;
 
-	for (frag = begin; frag != end; frag = NEXT_RX(frag))
-		bp->rx_ring[frag].addr &= ~MACB_BIT(RX_USED);
+	for (frag = begin; frag != end; frag++) {
+		struct macb_dma_desc *desc = macb_rx_desc(bp, frag);
+		desc->addr &= ~MACB_BIT(RX_USED);
+	}
 
 	/* Make descriptor updates visible to hardware */
 	wmb();
@@ -489,17 +533,18 @@ static void discard_partial_frame(struct macb *bp, unsigned int begin,
 static int macb_rx(struct macb *bp, int budget)
 {
 	int received = 0;
-	unsigned int tail = bp->rx_tail;
+	unsigned int tail;
 	int first_frag = -1;
 
-	for (; budget > 0; tail = NEXT_RX(tail)) {
+	for (tail = bp->rx_tail; budget > 0; tail++) {
+		struct macb_dma_desc *desc = macb_rx_desc(bp, tail);
 		u32 addr, ctrl;
 
 		/* Make hw descriptor updates visible to CPU */
 		rmb();
 
-		addr = bp->rx_ring[tail].addr;
-		ctrl = bp->rx_ring[tail].ctrl;
+		addr = desc->addr;
+		ctrl = desc->ctrl;
 
 		if (!(addr & MACB_BIT(RX_USED)))
 			break;
@@ -653,6 +698,8 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct macb *bp = netdev_priv(dev);
 	dma_addr_t mapping;
 	unsigned int len, entry;
+	struct macb_dma_desc *desc;
+	struct macb_tx_skb *tx_skb;
 	u32 ctrl;
 	unsigned long flags;
 
@@ -669,7 +716,7 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	spin_lock_irqsave(&bp->lock, flags);
 
 	/* This is a hard error, log it. */
-	if (TX_BUFFS_AVAIL(bp) < 1) {
+	if (macb_tx_ring_avail(bp) < 1) {
 		netif_stop_queue(dev);
 		spin_unlock_irqrestore(&bp->lock, flags);
 		netdev_err(bp->dev, "BUG! Tx Ring full when queue awake!\n");
@@ -678,12 +725,15 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		return NETDEV_TX_BUSY;
 	}
 
-	entry = bp->tx_head;
+	entry = macb_tx_ring_wrap(bp->tx_head);
+	bp->tx_head++;
 	netdev_vdbg(bp->dev, "Allocated ring entry %u\n", entry);
 	mapping = dma_map_single(&bp->pdev->dev, skb->data,
 				 len, DMA_TO_DEVICE);
-	bp->tx_skb[entry].skb = skb;
-	bp->tx_skb[entry].mapping = mapping;
+
+	tx_skb = &bp->tx_skb[entry];
+	tx_skb->skb = skb;
+	tx_skb->mapping = mapping;
 	netdev_vdbg(bp->dev, "Mapped skb data %p to DMA addr %08lx\n",
 		   skb->data, (unsigned long)mapping);
 
@@ -692,15 +742,13 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (entry == (TX_RING_SIZE - 1))
 		ctrl |= MACB_BIT(TX_WRAP);
 
-	bp->tx_ring[entry].addr = mapping;
-	bp->tx_ring[entry].ctrl = ctrl;
+	desc = &bp->tx_ring[entry];
+	desc->addr = mapping;
+	desc->ctrl = ctrl;
 
 	/* Make newly initialized descriptor visible to hardware */
 	wmb();
 
-	entry = NEXT_TX(entry);
-	bp->tx_head = entry;
-
 	skb_tx_timestamp(skb);
 
 	/*
@@ -713,10 +761,10 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	 * re-enable interrupts, and the interrupt handler will make
 	 * sure the controler is started.
 	 */
-	if (NEXT_TX(bp->tx_tail) == bp->tx_head)
+	if (bp->tx_tail == bp->tx_head - 1)
 		macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TSTART));
 
-	if (TX_BUFFS_AVAIL(bp) < 1)
+	if (macb_tx_ring_avail(bp) < 1)
 		netif_stop_queue(dev);
 
 	spin_unlock_irqrestore(&bp->lock, flags);
@@ -752,7 +800,7 @@ static int macb_alloc_consistent(struct macb *bp)
 {
 	int size;
 
-	size = TX_RING_SIZE * sizeof(struct ring_info);
+	size = TX_RING_SIZE * sizeof(struct macb_tx_skb);
 	bp->tx_skb = kmalloc(size, GFP_KERNEL);
 	if (!bp->tx_skb)
 		goto out_err;
@@ -1437,8 +1485,6 @@ static int __init macb_probe(struct platform_device *pdev)
 		macb_or_gem_writel(bp, USRIO, MACB_BIT(MII));
 #endif
 
-	bp->tx_pending = DEF_TX_RING_PENDING;
-
 	err = register_netdev(dev);
 	if (err) {
 		dev_err(&pdev->dev, "Cannot register net device, aborting.\n");
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index f69ceef..8a4ee2f 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -356,7 +356,12 @@
 		__v; \
 	})
 
-struct dma_desc {
+/**
+ * struct macb_dma_desc - Hardware DMA descriptor
+ * @addr: DMA address of data buffer
+ * @ctrl: Control and status bits
+ */
+struct macb_dma_desc {
 	u32	addr;
 	u32	ctrl;
 };
@@ -421,7 +426,12 @@ struct dma_desc {
 #define MACB_TX_USED_OFFSET			31
 #define MACB_TX_USED_SIZE			1
 
-struct ring_info {
+/**
+ * struct macb_tx_skb - data about an skb which is being transmitted
+ * @skb: skb currently being transmitted
+ * @mapping: DMA address of the skb's data buffer
+ */
+struct macb_tx_skb {
 	struct sk_buff		*skb;
 	dma_addr_t		mapping;
 };
@@ -506,12 +516,12 @@ struct macb {
 	void __iomem		*regs;
 
 	unsigned int		rx_tail;
-	struct dma_desc		*rx_ring;
+	struct macb_dma_desc	*rx_ring;
 	void			*rx_buffers;
 
 	unsigned int		tx_head, tx_tail;
-	struct dma_desc		*tx_ring;
-	struct ring_info	*tx_skb;
+	struct macb_dma_desc	*tx_ring;
+	struct macb_tx_skb	*tx_skb;
 
 	spinlock_t		lock;
 	struct platform_device	*pdev;
@@ -529,8 +539,6 @@ struct macb {
 	dma_addr_t		tx_ring_dma;
 	dma_addr_t		rx_buffers_dma;
 
-	unsigned int		rx_pending, tx_pending;
-
 	struct mii_bus		*mii_bus;
 	struct phy_device	*phy_dev;
 	unsigned int 		link;
-- 
1.7.10


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 06/10] net/macb: better manage tx errors
  2012-09-05  8:19 [PATCH 00/10] net/macb: driver enhancement concerning GEM support, ring logic and cleanup Nicolas Ferre
                   ` (4 preceding siblings ...)
  2012-09-05  9:00 ` [PATCH 05/10] net/macb: clean up ring buffer logic Nicolas Ferre
@ 2012-09-05  9:00 ` Nicolas Ferre
  2012-09-05  9:00 ` [PATCH 07/10] net/macb: tx status is more than 8 bits now Nicolas Ferre
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 23+ messages in thread
From: Nicolas Ferre @ 2012-09-05  9:00 UTC (permalink / raw)
  To: netdev
  Cc: linux-arm-kernel, davem, havard, nicolas.ferre, plagnioj, jamie,
	linux-kernel, patrice.vilchez

Handle all TX errors, not only underruns.
Reinitialize the TX ring after skipping all remaining frames, and
restart the controller when everything has been cleaned up properly.

Original idea from a patch by Havard Skinnemoen.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/net/ethernet/cadence/macb.c |  124 ++++++++++++++++++++---------------
 1 file changed, 71 insertions(+), 53 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 3d3a077..af71151 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -44,6 +44,10 @@
 
 #define MACB_RX_INT_FLAGS	(MACB_BIT(RCOMP) | MACB_BIT(RXUBR)	\
 				 | MACB_BIT(ISR_ROVR))
+#define MACB_TX_INT_FLAGS	(MACB_BIT(ISR_TUND)			\
+					| MACB_BIT(ISR_RLE)		\
+					| MACB_BIT(TXERR)		\
+					| MACB_BIT(TCOMP))
 
 /* Ring buffer accessors */
 static unsigned int macb_tx_ring_wrap(unsigned int index)
@@ -338,66 +342,56 @@ static void macb_update_stats(struct macb *bp)
 		*p += __raw_readl(reg);
 }
 
-static void macb_tx(struct macb *bp)
+static void macb_handle_tx_error(struct macb *bp, unsigned int err_tail, u32 ctrl)
 {
-	unsigned int tail;
-	unsigned int head;
-	u32 status;
-
-	status = macb_readl(bp, TSR);
-	macb_writel(bp, TSR, status);
+	struct macb_tx_skb	*tx_skb;
+	struct sk_buff		*skb;
+	unsigned int		head = bp->tx_head;
 
-	netdev_vdbg(bp->dev, "macb_tx status = %02lx\n", (unsigned long)status);
+	netdev_dbg(bp->dev, "TX error: ctrl 0x%08x, head %u, error tail %u\n",
+		   ctrl, head, err_tail);
 
-	if (status & (MACB_BIT(UND) | MACB_BIT(TSR_RLE))) {
-		int i;
-		netdev_err(bp->dev, "TX %s, resetting buffers\n",
-			   status & MACB_BIT(UND) ?
-			   "underrun" : "retry limit exceeded");
-
-		/* Transfer ongoing, disable transmitter, to avoid confusion */
-		if (status & MACB_BIT(TGO))
-			macb_writel(bp, NCR, macb_readl(bp, NCR) & ~MACB_BIT(TE));
-
-		head = bp->tx_head;
-
-		/*Mark all the buffer as used to avoid sending a lost buffer*/
-		for (i = 0; i < TX_RING_SIZE; i++)
-			bp->tx_ring[i].ctrl = MACB_BIT(TX_USED);
-
-		/* Add wrap bit */
-		bp->tx_ring[TX_RING_SIZE - 1].ctrl |= MACB_BIT(TX_WRAP);
+	/*
+	 * "Buffers exhausted mid-frame" errors may only happen if the
+	 * driver is buggy, so complain loudly about those. Statistics
+	 * are updated by hardware.
+	 */
+	if (ctrl & MACB_BIT(TX_BUF_EXHAUSTED))
+		netdev_err(bp->dev, "BUG: TX buffers exhausted mid-frame\n");
 
-		/* free transmit buffer in upper layer*/
-		for (tail = bp->tx_tail; tail != head; tail++) {
-			struct macb_tx_skb	*tx_skb;
-			struct sk_buff		*skb;
+	/*
+	 * Drop the frames that caused the error plus all remaining in queue.
+	 * Free transmit buffers in upper layer.
+	 */
+	for (; err_tail != head; err_tail++) {
+		struct macb_dma_desc	*desc;
 
-			rmb();
+		tx_skb = macb_tx_skb(bp, err_tail);
+		skb = tx_skb->skb;
+		dma_unmap_single(&bp->pdev->dev, tx_skb->mapping, skb->len,
+				 DMA_TO_DEVICE);
+		dev_kfree_skb_irq(skb);
+		tx_skb->skb = NULL;
 
-			tx_skb = macb_tx_skb(bp, tail);
-			skb = tx_skb->skb;
+		desc = macb_tx_desc(bp, err_tail);
+		desc->ctrl |= MACB_BIT(TX_USED);
+	}
 
-			dma_unmap_single(&bp->pdev->dev, tx_skb->mapping,
-						skb->len, DMA_TO_DEVICE);
-			tx_skb->skb = NULL;
-			dev_kfree_skb_irq(skb);
-		}
+	/* Make descriptor updates visible to hardware */
+	wmb();
+}
 
-		bp->tx_head = bp->tx_tail = 0;
+static void macb_tx_interrupt(struct macb *bp)
+{
+	unsigned int tail;
+	unsigned int head;
+	u32 status;
 
-		/* Enable the transmitter again */
-		if (status & MACB_BIT(TGO))
-			macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TE));
-	}
+	status = macb_readl(bp, TSR);
+	macb_writel(bp, TSR, status);
 
-	if (!(status & MACB_BIT(COMP)))
-		/*
-		 * This may happen when a buffer becomes complete
-		 * between reading the ISR and scanning the
-		 * descriptors.  Nothing to worry about.
-		 */
-		return;
+	netdev_vdbg(bp->dev, "macb_tx_interrupt status = %02lx\n",
+		(unsigned long)status);
 
 	head = bp->tx_head;
 	for (tail = bp->tx_tail; tail != head; tail++) {
@@ -413,6 +407,31 @@ static void macb_tx(struct macb *bp)
 
 		ctrl = desc->ctrl;
 
+		if (unlikely(ctrl & (MACB_BIT(TX_ERROR)
+					| MACB_BIT(TX_UNDERRUN)
+					| MACB_BIT(TX_BUF_EXHAUSTED)))) {
+			/*
+			 * In case of transfer ongoing, disable transmitter.
+			 * Should already be the case due to hardware,
+			 * but make sure to avoid confusion.
+			 */
+			if (status & MACB_BIT(TGO))
+				macb_writel(bp, NCR, macb_readl(bp, NCR) & ~MACB_BIT(TE));
+
+			/*
+			 * An error should always stop the queue from advancing.
+			 * reset entries in the ring and exit from the loop.
+			 */
+			macb_handle_tx_error(bp, tail, ctrl);
+			bp->tx_head = bp->tx_tail = head = tail = 0;
+
+			/* Enable the transmitter again, start TX will be done elsewhere */
+			if (status & MACB_BIT(TGO))
+				macb_writel(bp, NCR, macb_readl(bp, NCR) | MACB_BIT(TE));
+
+			break;
+		}
+
 		if (!(ctrl & MACB_BIT(TX_USED)))
 			break;
 
@@ -644,9 +663,8 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
 			}
 		}
 
-		if (status & (MACB_BIT(TCOMP) | MACB_BIT(ISR_TUND) |
-			    MACB_BIT(ISR_RLE)))
-			macb_tx(bp);
+		if (status & MACB_TX_INT_FLAGS)
+			macb_tx_interrupt(bp);
 
 		/*
 		 * Link change detection isn't possible with RMII, so we'll
-- 
1.7.10


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 07/10] net/macb: tx status is more than 8 bits now
  2012-09-05  8:19 [PATCH 00/10] net/macb: driver enhancement concerning GEM support, ring logic and cleanup Nicolas Ferre
                   ` (5 preceding siblings ...)
  2012-09-05  9:00 ` [PATCH 06/10] net/macb: better manage tx errors Nicolas Ferre
@ 2012-09-05  9:00 ` Nicolas Ferre
  2012-09-05  9:00 ` [PATCH 08/10] net/macb: macb_get_drvinfo: add GEM/MACB suffix to differentiate revision Nicolas Ferre
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 23+ messages in thread
From: Nicolas Ferre @ 2012-09-05  9:00 UTC (permalink / raw)
  To: netdev
  Cc: linux-arm-kernel, davem, havard, nicolas.ferre, plagnioj, jamie,
	linux-kernel, patrice.vilchez

On some revision of GEM, TSR status register is has more information.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/net/ethernet/cadence/macb.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index af71151..bd331fd 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -390,7 +390,7 @@ static void macb_tx_interrupt(struct macb *bp)
 	status = macb_readl(bp, TSR);
 	macb_writel(bp, TSR, status);
 
-	netdev_vdbg(bp->dev, "macb_tx_interrupt status = %02lx\n",
+	netdev_vdbg(bp->dev, "macb_tx_interrupt status = 0x%03lx\n",
 		(unsigned long)status);
 
 	head = bp->tx_head;
-- 
1.7.10


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 08/10] net/macb: macb_get_drvinfo: add GEM/MACB suffix to differentiate revision
  2012-09-05  8:19 [PATCH 00/10] net/macb: driver enhancement concerning GEM support, ring logic and cleanup Nicolas Ferre
                   ` (6 preceding siblings ...)
  2012-09-05  9:00 ` [PATCH 07/10] net/macb: tx status is more than 8 bits now Nicolas Ferre
@ 2012-09-05  9:00 ` Nicolas Ferre
  2012-09-05 21:31   ` David Miller
  2012-09-05 23:27   ` Ben Hutchings
  2012-09-05  9:00 ` [PATCH 09/10] net/macb: ethtool interface: add register dump feature Nicolas Ferre
  2012-09-05  9:04 ` [PATCH 10/10] net/macb: Offset first RX buffer by two bytes Nicolas Ferre
  9 siblings, 2 replies; 23+ messages in thread
From: Nicolas Ferre @ 2012-09-05  9:00 UTC (permalink / raw)
  To: netdev
  Cc: linux-arm-kernel, davem, havard, nicolas.ferre, plagnioj, jamie,
	linux-kernel, patrice.vilchez

Add an indication about which revision of the hardware we are running in
info->driver string.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/net/ethernet/cadence/macb.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index bd331fd..c7c39f1 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -1313,6 +1313,10 @@ static void macb_get_drvinfo(struct net_device *dev,
 	struct macb *bp = netdev_priv(dev);
 
 	strcpy(info->driver, bp->pdev->dev.driver->name);
+	if (macb_is_gem(bp))
+		strcat(info->driver, " GEM");
+	else
+		strcat(info->driver, " MACB");
 	strcpy(info->version, "$Revision: 1.14 $");
 	strcpy(info->bus_info, dev_name(&bp->pdev->dev));
 }
-- 
1.7.10


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 09/10] net/macb: ethtool interface: add register dump feature
  2012-09-05  8:19 [PATCH 00/10] net/macb: driver enhancement concerning GEM support, ring logic and cleanup Nicolas Ferre
                   ` (7 preceding siblings ...)
  2012-09-05  9:00 ` [PATCH 08/10] net/macb: macb_get_drvinfo: add GEM/MACB suffix to differentiate revision Nicolas Ferre
@ 2012-09-05  9:00 ` Nicolas Ferre
  2012-09-05 21:32   ` David Miller
  2012-09-05 23:36   ` Ben Hutchings
  2012-09-05  9:04 ` [PATCH 10/10] net/macb: Offset first RX buffer by two bytes Nicolas Ferre
  9 siblings, 2 replies; 23+ messages in thread
From: Nicolas Ferre @ 2012-09-05  9:00 UTC (permalink / raw)
  To: netdev
  Cc: linux-arm-kernel, davem, havard, nicolas.ferre, plagnioj, jamie,
	linux-kernel, patrice.vilchez

Add macb_get_regs() ethtool function and its helper function:
macb_get_regs_len().

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/net/ethernet/cadence/macb.c |   40 +++++++++++++++++++++++++++++++++++
 drivers/net/ethernet/cadence/macb.h |    3 +++
 2 files changed, 43 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index c7c39f1..f31c0a7 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -1321,10 +1321,50 @@ static void macb_get_drvinfo(struct net_device *dev,
 	strcpy(info->bus_info, dev_name(&bp->pdev->dev));
 }
 
+static int macb_get_regs_len(struct net_device *netdev)
+{
+	return MACB_GREGS_LEN * sizeof(u32);
+}
+
+static void macb_get_regs(struct net_device *dev, struct ethtool_regs *regs,
+			  void *p)
+{
+	struct macb *bp = netdev_priv(dev);
+	unsigned int tail, head;
+	u32 *regs_buff = p;
+
+        memset(p, 0, MACB_GREGS_LEN * sizeof(u32));
+	regs->version = MACB_BFEXT(IDNUM, macb_readl(bp, MID));
+
+	tail = macb_tx_ring_wrap(bp->tx_tail);
+	head = macb_tx_ring_wrap(bp->tx_head);
+
+	regs_buff[0]  = macb_readl(bp, NCR);
+	regs_buff[1]  = macb_or_gem_readl(bp, NCFGR);
+	regs_buff[2]  = macb_readl(bp, NSR);
+	regs_buff[3]  = macb_readl(bp, TSR);
+	regs_buff[4]  = macb_readl(bp, RBQP);
+	regs_buff[5]  = macb_readl(bp, TBQP);
+	regs_buff[6]  = macb_readl(bp, RSR);
+	regs_buff[7]  = macb_readl(bp, IMR);
+
+	regs_buff[8]  = tail;
+	regs_buff[9]  = head;
+	regs_buff[10] = macb_tx_dma(bp, tail);
+	regs_buff[11] = macb_tx_dma(bp, head);
+
+	if (macb_is_gem(bp)) {
+		regs_buff[12] = gem_readl(bp, USRIO);
+		regs_buff[13] = gem_readl(bp, DMACFG);
+	}
+}
+
 static const struct ethtool_ops macb_ethtool_ops = {
 	.get_settings		= macb_get_settings,
 	.set_settings		= macb_set_settings,
 	.get_drvinfo		= macb_get_drvinfo,
+	.get_regs_len		= macb_get_regs_len,
+	.get_regs		= macb_get_regs,
 	.get_link		= ethtool_op_get_link,
 	.get_ts_info		= ethtool_op_get_ts_info,
 };
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 8a4ee2f..d509e88 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -10,6 +10,9 @@
 #ifndef _MACB_H
 #define _MACB_H
 
+
+#define MACB_GREGS_LEN 32
+
 /* MACB register offsets */
 #define MACB_NCR				0x0000
 #define MACB_NCFGR				0x0004
-- 
1.7.10


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 10/10] net/macb: Offset first RX buffer by two bytes
  2012-09-05  8:19 [PATCH 00/10] net/macb: driver enhancement concerning GEM support, ring logic and cleanup Nicolas Ferre
                   ` (8 preceding siblings ...)
  2012-09-05  9:00 ` [PATCH 09/10] net/macb: ethtool interface: add register dump feature Nicolas Ferre
@ 2012-09-05  9:04 ` Nicolas Ferre
  9 siblings, 0 replies; 23+ messages in thread
From: Nicolas Ferre @ 2012-09-05  9:04 UTC (permalink / raw)
  To: netdev
  Cc: linux-arm-kernel, davem, havard, nicolas.ferre, plagnioj, jamie,
	linux-kernel, patrice.vilchez

From: Havard Skinnemoen <havard@skinnemoen.net>

Make the ethernet frame payload word-aligned, possibly making the
memcpy into the skb a bit faster. This will be even more important
after we eliminate the copy altogether.

Also eliminate the redundant RX_OFFSET constant -- it has the same
definition and purpose as NET_IP_ALIGN.

Signed-off-by: Havard Skinnemoen <havard@skinnemoen.net>
[nicolas.ferre@atmel.com: adapt to newer kernel]
Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 drivers/net/ethernet/cadence/macb.c |   23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index f31c0a7..f7716b6 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -33,9 +33,6 @@
 #define RX_RING_SIZE		512
 #define RX_RING_BYTES		(sizeof(struct macb_dma_desc) * RX_RING_SIZE)
 
-/* Make the IP header word-aligned (the ethernet header is 14 bytes) */
-#define RX_OFFSET		2
-
 #define TX_RING_SIZE		128
 #define TX_RING_BYTES		(sizeof(struct macb_dma_desc) * TX_RING_SIZE)
 
@@ -466,7 +463,7 @@ static int macb_rx_frame(struct macb *bp, unsigned int first_frag,
 {
 	unsigned int len;
 	unsigned int frag;
-	unsigned int offset = 0;
+	unsigned int offset;
 	struct sk_buff *skb;
 	struct macb_dma_desc *desc;
 
@@ -477,7 +474,16 @@ static int macb_rx_frame(struct macb *bp, unsigned int first_frag,
 		macb_rx_ring_wrap(first_frag),
 		macb_rx_ring_wrap(last_frag), len);
 
-	skb = netdev_alloc_skb(bp->dev, len + RX_OFFSET);
+	/*
+	 * The ethernet header starts NET_IP_ALIGN bytes into the
+	 * first buffer. Since the header is 14 bytes, this makes the
+	 * payload word-aligned.
+	 *
+	 * Instead of calling skb_reserve(NET_IP_ALIGN), we just copy
+	 * the two padding bytes into the skb so that we avoid hitting
+	 * the slowpath in memcpy(), and pull them off afterwards.
+	 */
+	skb = netdev_alloc_skb(bp->dev, len + NET_IP_ALIGN);
 	if (!skb) {
 		bp->stats.rx_dropped++;
 		for (frag = first_frag; ; frag++) {
@@ -493,7 +499,8 @@ static int macb_rx_frame(struct macb *bp, unsigned int first_frag,
 		return 1;
 	}
 
-	skb_reserve(skb, RX_OFFSET);
+	offset = 0;
+	len += NET_IP_ALIGN;
 	skb_checksum_none_assert(skb);
 	skb_put(skb, len);
 
@@ -517,10 +524,11 @@ static int macb_rx_frame(struct macb *bp, unsigned int first_frag,
 	/* Make descriptor updates visible to hardware */
 	wmb();
 
+	__skb_pull(skb, NET_IP_ALIGN);
 	skb->protocol = eth_type_trans(skb, bp->dev);
 
 	bp->stats.rx_packets++;
-	bp->stats.rx_bytes += len;
+	bp->stats.rx_bytes += skb->len;
 	netdev_vdbg(bp->dev, "received skb of length %u, csum: %08x\n",
 		   skb->len, skb->csum);
 	netif_receive_skb(skb);
@@ -985,6 +993,7 @@ static void macb_init_hw(struct macb *bp)
 	__macb_set_hwaddr(bp);
 
 	config = macb_mdc_clk_div(bp);
+	config |= MACB_BF(RBOF, NET_IP_ALIGN);	/* Make eth data aligned */
 	config |= MACB_BIT(PAE);		/* PAuse Enable */
 	config |= MACB_BIT(DRFCS);		/* Discard Rx FCS */
 	config |= MACB_BIT(BIG);		/* Receive oversized frames */
-- 
1.7.10


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH 04/10] net/macb: Fix a race in macb_start_xmit()
  2012-09-05  8:19 ` [PATCH 04/10] net/macb: Fix a race in macb_start_xmit() Nicolas Ferre
@ 2012-09-05 21:30   ` David Miller
  2012-09-06 14:52     ` Nicolas Ferre
  2012-09-06 15:49     ` Havard Skinnemoen
  0 siblings, 2 replies; 23+ messages in thread
From: David Miller @ 2012-09-05 21:30 UTC (permalink / raw)
  To: nicolas.ferre
  Cc: netdev, linux-arm-kernel, havard, plagnioj, jamie, linux-kernel,
	patrice.vilchez

From: Nicolas Ferre <nicolas.ferre@atmel.com>
Date: Wed, 5 Sep 2012 10:19:11 +0200

> From: Havard Skinnemoen <havard@skinnemoen.net>
> 
> Fix a race in macb_start_xmit() where we unconditionally set the TSTART bit.
> If an underrun just happened (we do this with interrupts disabled, so it might
> not have been handled yet), the controller starts transmitting from the first
> entry in the ring, which is usually wrong.
> Restart the controller after error handling.
> 
> Signed-off-by: Havard Skinnemoen <havard@skinnemoen.net>
> [nicolas.ferre@atmel.com: split patch in topics]
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>

Accumulating special case code and checks into the hot path of TX packet
processing is extremely unwise.

Instead, when you handle the TX error conditions and reset the chip you
should first ensure that there are no flows of control in the transmit
function of your driver by using the appropriate locking et al. facilities.

For example, you can quiesce the transmit path by handling the chip error
interrupt as follows:

1) Disable chip interrupt generation.

2) Schedule a workqueue so you can process the reset outside of hard
   interrupt context.

3) In the workqueue function, disable NAPI and perform a
   netif_tx_disable() to guarentee there are no threads of
   execution trying to queue up packets for TX into the driver.

4) Perform your chip reset and whatever else is necessary.

5) Re-enable NAPI and TX.

Then you don't need any special checks in your xmit method at all.



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 08/10] net/macb: macb_get_drvinfo: add GEM/MACB suffix to differentiate revision
  2012-09-05  9:00 ` [PATCH 08/10] net/macb: macb_get_drvinfo: add GEM/MACB suffix to differentiate revision Nicolas Ferre
@ 2012-09-05 21:31   ` David Miller
  2012-09-05 23:27   ` Ben Hutchings
  1 sibling, 0 replies; 23+ messages in thread
From: David Miller @ 2012-09-05 21:31 UTC (permalink / raw)
  To: nicolas.ferre
  Cc: netdev, linux-arm-kernel, havard, plagnioj, jamie, linux-kernel,
	patrice.vilchez

From: Nicolas Ferre <nicolas.ferre@atmel.com>
Date: Wed, 5 Sep 2012 11:00:53 +0200

>  	strcpy(info->driver, bp->pdev->dev.driver->name);
> +	if (macb_is_gem(bp))
> +		strcat(info->driver, " GEM");
> +	else
> +		strcat(info->driver, " MACB");

This is a driver string, which means the software driver, and has
absolutely nothing to do with the exact type of the underlying
physical hardware.

Therefore the these suffixes should be left out of the driver string.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 09/10] net/macb: ethtool interface: add register dump feature
  2012-09-05  9:00 ` [PATCH 09/10] net/macb: ethtool interface: add register dump feature Nicolas Ferre
@ 2012-09-05 21:32   ` David Miller
  2012-09-05 23:36   ` Ben Hutchings
  1 sibling, 0 replies; 23+ messages in thread
From: David Miller @ 2012-09-05 21:32 UTC (permalink / raw)
  To: nicolas.ferre
  Cc: netdev, linux-arm-kernel, havard, plagnioj, jamie, linux-kernel,
	patrice.vilchez

From: Nicolas Ferre <nicolas.ferre@atmel.com>
Date: Wed, 5 Sep 2012 11:00:54 +0200

> @@ -10,6 +10,9 @@
>  #ifndef _MACB_H
>  #define _MACB_H
>  
> +
> +#define MACB_GREGS_LEN 32

Please don't add such extraneous empty lines.  One empty line between
constructs is more than enough, and anything more is visually awkward.

Thanks.



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 08/10] net/macb: macb_get_drvinfo: add GEM/MACB suffix to differentiate revision
  2012-09-05  9:00 ` [PATCH 08/10] net/macb: macb_get_drvinfo: add GEM/MACB suffix to differentiate revision Nicolas Ferre
  2012-09-05 21:31   ` David Miller
@ 2012-09-05 23:27   ` Ben Hutchings
  2012-09-06 14:01     ` Nicolas Ferre
  1 sibling, 1 reply; 23+ messages in thread
From: Ben Hutchings @ 2012-09-05 23:27 UTC (permalink / raw)
  To: Nicolas Ferre
  Cc: netdev, linux-arm-kernel, davem, havard, plagnioj, jamie,
	linux-kernel, patrice.vilchez

On Wed, 2012-09-05 at 11:00 +0200, Nicolas Ferre wrote:
> Add an indication about which revision of the hardware we are running in
> info->driver string.
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
>  drivers/net/ethernet/cadence/macb.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index bd331fd..c7c39f1 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -1313,6 +1313,10 @@ static void macb_get_drvinfo(struct net_device *dev,
>  	struct macb *bp = netdev_priv(dev);
>  
>  	strcpy(info->driver, bp->pdev->dev.driver->name);
> +	if (macb_is_gem(bp))
> +		strcat(info->driver, " GEM");
> +	else
> +		strcat(info->driver, " MACB");
>  	strcpy(info->version, "$Revision: 1.14 $");

Related to hardware revisions (which don't belong here, as David said),
I rather doubt this CVS ID is very useful as a driver version.

If the driver doesn't have a meaningful version (aside from the kernel
version) then you can remove this function and let the ethtool core fill
in the other two fields automatically.

Ben.

>  	strcpy(info->bus_info, dev_name(&bp->pdev->dev));
>  }

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 09/10] net/macb: ethtool interface: add register dump feature
  2012-09-05  9:00 ` [PATCH 09/10] net/macb: ethtool interface: add register dump feature Nicolas Ferre
  2012-09-05 21:32   ` David Miller
@ 2012-09-05 23:36   ` Ben Hutchings
  2012-09-06 14:20     ` [PATCH v2 " Nicolas Ferre
  1 sibling, 1 reply; 23+ messages in thread
From: Ben Hutchings @ 2012-09-05 23:36 UTC (permalink / raw)
  To: Nicolas Ferre
  Cc: netdev, linux-arm-kernel, davem, havard, plagnioj, jamie,
	linux-kernel, patrice.vilchez

On Wed, 2012-09-05 at 11:00 +0200, Nicolas Ferre wrote:
> Add macb_get_regs() ethtool function and its helper function:
> macb_get_regs_len().
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
>  drivers/net/ethernet/cadence/macb.c |   40 +++++++++++++++++++++++++++++++++++
>  drivers/net/ethernet/cadence/macb.h |    3 +++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index c7c39f1..f31c0a7 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -1321,10 +1321,50 @@ static void macb_get_drvinfo(struct net_device *dev,
>  	strcpy(info->bus_info, dev_name(&bp->pdev->dev));
>  }
>  
> +static int macb_get_regs_len(struct net_device *netdev)
> +{
> +	return MACB_GREGS_LEN * sizeof(u32);
> +}
> +
> +static void macb_get_regs(struct net_device *dev, struct ethtool_regs *regs,
> +			  void *p)
> +{
> +	struct macb *bp = netdev_priv(dev);
> +	unsigned int tail, head;
> +	u32 *regs_buff = p;
> +
> +        memset(p, 0, MACB_GREGS_LEN * sizeof(u32));

The ethtool core does that for you.  (Drivers can't be trusted to do
it!)

> +	regs->version = MACB_BFEXT(IDNUM, macb_readl(bp, MID));

Note that the version field is supposed to be the version of the
register dump format.  Is this value sufficient for userland to easily
decide whether macb_is_gem()?  Are there spare bits that you can set if
you change the format later?

> +	tail = macb_tx_ring_wrap(bp->tx_tail);
> +	head = macb_tx_ring_wrap(bp->tx_head);
> +
> +	regs_buff[0]  = macb_readl(bp, NCR);
> +	regs_buff[1]  = macb_or_gem_readl(bp, NCFGR);
> +	regs_buff[2]  = macb_readl(bp, NSR);
> +	regs_buff[3]  = macb_readl(bp, TSR);
> +	regs_buff[4]  = macb_readl(bp, RBQP);
> +	regs_buff[5]  = macb_readl(bp, TBQP);
> +	regs_buff[6]  = macb_readl(bp, RSR);
> +	regs_buff[7]  = macb_readl(bp, IMR);
> +
> +	regs_buff[8]  = tail;
> +	regs_buff[9]  = head;
> +	regs_buff[10] = macb_tx_dma(bp, tail);
> +	regs_buff[11] = macb_tx_dma(bp, head);
> +
> +	if (macb_is_gem(bp)) {
> +		regs_buff[12] = gem_readl(bp, USRIO);
> +		regs_buff[13] = gem_readl(bp, DMACFG);
> +	}
> +}
> +
>  static const struct ethtool_ops macb_ethtool_ops = {
>  	.get_settings		= macb_get_settings,
>  	.set_settings		= macb_set_settings,
>  	.get_drvinfo		= macb_get_drvinfo,
> +	.get_regs_len		= macb_get_regs_len,
> +	.get_regs		= macb_get_regs,
>  	.get_link		= ethtool_op_get_link,
>  	.get_ts_info		= ethtool_op_get_ts_info,
>  };
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 8a4ee2f..d509e88 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -10,6 +10,9 @@
>  #ifndef _MACB_H
>  #define _MACB_H
>  
> +
> +#define MACB_GREGS_LEN 32

Why is this rather larger than the actual number of registers you
return?  Also, the name is not a great idea as 'regs_len' is normally a
number of bytes.

Ben.

>  /* MACB register offsets */
>  #define MACB_NCR				0x0000
>  #define MACB_NCFGR				0x0004

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 08/10] net/macb: macb_get_drvinfo: add GEM/MACB suffix to differentiate revision
  2012-09-05 23:27   ` Ben Hutchings
@ 2012-09-06 14:01     ` Nicolas Ferre
  2012-09-06 17:42       ` David Miller
  0 siblings, 1 reply; 23+ messages in thread
From: Nicolas Ferre @ 2012-09-06 14:01 UTC (permalink / raw)
  To: Ben Hutchings
  Cc: netdev, linux-arm-kernel, davem, havard, plagnioj, jamie,
	linux-kernel, patrice.vilchez

On 09/06/2012 01:27 AM, Ben Hutchings :
> On Wed, 2012-09-05 at 11:00 +0200, Nicolas Ferre wrote:
>> Add an indication about which revision of the hardware we are running in
>> info->driver string.
>>
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>> ---
>>  drivers/net/ethernet/cadence/macb.c |    4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
>> index bd331fd..c7c39f1 100644
>> --- a/drivers/net/ethernet/cadence/macb.c
>> +++ b/drivers/net/ethernet/cadence/macb.c
>> @@ -1313,6 +1313,10 @@ static void macb_get_drvinfo(struct net_device *dev,
>>  	struct macb *bp = netdev_priv(dev);
>>  
>>  	strcpy(info->driver, bp->pdev->dev.driver->name);
>> +	if (macb_is_gem(bp))
>> +		strcat(info->driver, " GEM");
>> +	else
>> +		strcat(info->driver, " MACB");
>>  	strcpy(info->version, "$Revision: 1.14 $");
> 
> Related to hardware revisions (which don't belong here, as David said),
> I rather doubt this CVS ID is very useful as a driver version.
> 
> If the driver doesn't have a meaningful version (aside from the kernel
> version) then you can remove this function and let the ethtool core fill
> in the other two fields automatically.

Absolutely, I will do this.

Thanks for the tip.

Best regards,
-- 
Nicolas Ferre

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 09/10] net/macb: ethtool interface: add register dump feature
  2012-09-05 23:36   ` Ben Hutchings
@ 2012-09-06 14:20     ` Nicolas Ferre
  2012-09-06 14:34       ` Ben Hutchings
  0 siblings, 1 reply; 23+ messages in thread
From: Nicolas Ferre @ 2012-09-06 14:20 UTC (permalink / raw)
  To: netdev, bhutchings, davem
  Cc: linux-arm-kernel, havard, plagnioj, patrice.vilchez,
	linux-kernel, Nicolas Ferre

Add macb_get_regs() ethtool function and its helper function:
macb_get_regs_len().
The version field is deduced from the IP revision which gives the
"MACB or GEM" information. An additional version field is reserved.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
v2: - modify MACB_GREGS_NBR name and adapt to number of registers
      actually displayed.
    - change version format to reflect register layout and
      add a version number to be future proof.

 drivers/net/ethernet/cadence/macb.c |   40 +++++++++++++++++++++++++++++++++++
 drivers/net/ethernet/cadence/macb.h |    3 +++
 2 files changed, 43 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index dc34ff1..cab42e7 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -1223,9 +1223,49 @@ static int macb_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
 	return phy_ethtool_sset(phydev, cmd);
 }
 
+static int macb_get_regs_len(struct net_device *netdev)
+{
+	return MACB_GREGS_NBR * sizeof(u32);
+}
+
+static void macb_get_regs(struct net_device *dev, struct ethtool_regs *regs,
+			  void *p)
+{
+	struct macb *bp = netdev_priv(dev);
+	unsigned int tail, head;
+	u32 *regs_buff = p;
+
+	regs->version = (macb_readl(bp, MID) & ((1 << MACB_REV_SIZE) - 1))
+			| MACB_GREGS_VERSION;
+
+	tail = macb_tx_ring_wrap(bp->tx_tail);
+	head = macb_tx_ring_wrap(bp->tx_head);
+
+	regs_buff[0]  = macb_readl(bp, NCR);
+	regs_buff[1]  = macb_or_gem_readl(bp, NCFGR);
+	regs_buff[2]  = macb_readl(bp, NSR);
+	regs_buff[3]  = macb_readl(bp, TSR);
+	regs_buff[4]  = macb_readl(bp, RBQP);
+	regs_buff[5]  = macb_readl(bp, TBQP);
+	regs_buff[6]  = macb_readl(bp, RSR);
+	regs_buff[7]  = macb_readl(bp, IMR);
+
+	regs_buff[8]  = tail;
+	regs_buff[9]  = head;
+	regs_buff[10] = macb_tx_dma(bp, tail);
+	regs_buff[11] = macb_tx_dma(bp, head);
+
+	if (macb_is_gem(bp)) {
+		regs_buff[12] = gem_readl(bp, USRIO);
+		regs_buff[13] = gem_readl(bp, DMACFG);
+	}
+}
+
 static const struct ethtool_ops macb_ethtool_ops = {
 	.get_settings		= macb_get_settings,
 	.set_settings		= macb_set_settings,
+	.get_regs_len		= macb_get_regs_len,
+	.get_regs		= macb_get_regs,
 	.get_link		= ethtool_op_get_link,
 };
 
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index f69ceef..bcadc3c 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -10,6 +10,9 @@
 #ifndef _MACB_H
 #define _MACB_H
 
+#define MACB_GREGS_NBR 16
+#define MACB_GREGS_VERSION 1
+
 /* MACB register offsets */
 #define MACB_NCR				0x0000
 #define MACB_NCFGR				0x0004
-- 
1.7.10


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 09/10] net/macb: ethtool interface: add register dump feature
  2012-09-06 14:20     ` [PATCH v2 " Nicolas Ferre
@ 2012-09-06 14:34       ` Ben Hutchings
  0 siblings, 0 replies; 23+ messages in thread
From: Ben Hutchings @ 2012-09-06 14:34 UTC (permalink / raw)
  To: Nicolas Ferre
  Cc: netdev, davem, linux-arm-kernel, havard, plagnioj,
	patrice.vilchez, linux-kernel

On Thu, 2012-09-06 at 16:20 +0200, Nicolas Ferre wrote:
> Add macb_get_regs() ethtool function and its helper function:
> macb_get_regs_len().
> The version field is deduced from the IP revision which gives the
> "MACB or GEM" information. An additional version field is reserved.
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Reviewed-by: Ben Hutchings <bhutchings@solarflare.com>
> ---
> v2: - modify MACB_GREGS_NBR name and adapt to number of registers
>       actually displayed.
>     - change version format to reflect register layout and
>       add a version number to be future proof.
> 
>  drivers/net/ethernet/cadence/macb.c |   40 +++++++++++++++++++++++++++++++++++
>  drivers/net/ethernet/cadence/macb.h |    3 +++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index dc34ff1..cab42e7 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -1223,9 +1223,49 @@ static int macb_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
>  	return phy_ethtool_sset(phydev, cmd);
>  }
>  
> +static int macb_get_regs_len(struct net_device *netdev)
> +{
> +	return MACB_GREGS_NBR * sizeof(u32);
> +}
> +
> +static void macb_get_regs(struct net_device *dev, struct ethtool_regs *regs,
> +			  void *p)
> +{
> +	struct macb *bp = netdev_priv(dev);
> +	unsigned int tail, head;
> +	u32 *regs_buff = p;
> +
> +	regs->version = (macb_readl(bp, MID) & ((1 << MACB_REV_SIZE) - 1))
> +			| MACB_GREGS_VERSION;
> +
> +	tail = macb_tx_ring_wrap(bp->tx_tail);
> +	head = macb_tx_ring_wrap(bp->tx_head);
> +
> +	regs_buff[0]  = macb_readl(bp, NCR);
> +	regs_buff[1]  = macb_or_gem_readl(bp, NCFGR);
> +	regs_buff[2]  = macb_readl(bp, NSR);
> +	regs_buff[3]  = macb_readl(bp, TSR);
> +	regs_buff[4]  = macb_readl(bp, RBQP);
> +	regs_buff[5]  = macb_readl(bp, TBQP);
> +	regs_buff[6]  = macb_readl(bp, RSR);
> +	regs_buff[7]  = macb_readl(bp, IMR);
> +
> +	regs_buff[8]  = tail;
> +	regs_buff[9]  = head;
> +	regs_buff[10] = macb_tx_dma(bp, tail);
> +	regs_buff[11] = macb_tx_dma(bp, head);
> +
> +	if (macb_is_gem(bp)) {
> +		regs_buff[12] = gem_readl(bp, USRIO);
> +		regs_buff[13] = gem_readl(bp, DMACFG);
> +	}
> +}
> +
>  static const struct ethtool_ops macb_ethtool_ops = {
>  	.get_settings		= macb_get_settings,
>  	.set_settings		= macb_set_settings,
> +	.get_regs_len		= macb_get_regs_len,
> +	.get_regs		= macb_get_regs,
>  	.get_link		= ethtool_op_get_link,
>  };
>  
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index f69ceef..bcadc3c 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -10,6 +10,9 @@
>  #ifndef _MACB_H
>  #define _MACB_H
>  
> +#define MACB_GREGS_NBR 16
> +#define MACB_GREGS_VERSION 1
> +
>  /* MACB register offsets */
>  #define MACB_NCR				0x0000
>  #define MACB_NCFGR				0x0004

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 04/10] net/macb: Fix a race in macb_start_xmit()
  2012-09-05 21:30   ` David Miller
@ 2012-09-06 14:52     ` Nicolas Ferre
  2012-09-06 15:49     ` Havard Skinnemoen
  1 sibling, 0 replies; 23+ messages in thread
From: Nicolas Ferre @ 2012-09-06 14:52 UTC (permalink / raw)
  To: David Miller
  Cc: netdev, linux-arm-kernel, havard, plagnioj, jamie, linux-kernel,
	patrice.vilchez

On 09/05/2012 11:30 PM, David Miller :
> From: Nicolas Ferre <nicolas.ferre@atmel.com>
> Date: Wed, 5 Sep 2012 10:19:11 +0200
> 
>> From: Havard Skinnemoen <havard@skinnemoen.net>
>>
>> Fix a race in macb_start_xmit() where we unconditionally set the TSTART bit.
>> If an underrun just happened (we do this with interrupts disabled, so it might
>> not have been handled yet), the controller starts transmitting from the first
>> entry in the ring, which is usually wrong.
>> Restart the controller after error handling.
>>
>> Signed-off-by: Havard Skinnemoen <havard@skinnemoen.net>
>> [nicolas.ferre@atmel.com: split patch in topics]
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> 
> Accumulating special case code and checks into the hot path of TX packet
> processing is extremely unwise.
> 
> Instead, when you handle the TX error conditions and reset the chip you
> should first ensure that there are no flows of control in the transmit
> function of your driver by using the appropriate locking et al. facilities.
> 
> For example, you can quiesce the transmit path by handling the chip error
> interrupt as follows:
> 
> 1) Disable chip interrupt generation.
> 
> 2) Schedule a workqueue so you can process the reset outside of hard
>    interrupt context.
> 
> 3) In the workqueue function, disable NAPI and perform a
>    netif_tx_disable() to guarentee there are no threads of
>    execution trying to queue up packets for TX into the driver.
> 
> 4) Perform your chip reset and whatever else is necessary.
> 
> 5) Re-enable NAPI and TX.
> 
> Then you don't need any special checks in your xmit method at all.

I see... I will rework the series and try to implement this as part of
the "[PATCH 06/10] net/macb: better manage tx errors"

So this patch will disappear in future v2 series and patch 06 will be
seriously modified. In fact I will also try to stack "cosmetic" patches
at the beginning of the series.

Thanks, best regards,
-- 
Nicolas Ferre

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 04/10] net/macb: Fix a race in macb_start_xmit()
  2012-09-05 21:30   ` David Miller
  2012-09-06 14:52     ` Nicolas Ferre
@ 2012-09-06 15:49     ` Havard Skinnemoen
  2012-09-07 16:34       ` Nicolas Ferre
  1 sibling, 1 reply; 23+ messages in thread
From: Havard Skinnemoen @ 2012-09-06 15:49 UTC (permalink / raw)
  To: David Miller
  Cc: nicolas.ferre, netdev, linux-arm-kernel, plagnioj, jamie,
	linux-kernel, patrice.vilchez

On Wed, Sep 5, 2012 at 11:30 PM, David Miller <davem@davemloft.net> wrote:
> From: Nicolas Ferre <nicolas.ferre@atmel.com>
> Date: Wed, 5 Sep 2012 10:19:11 +0200
>
>> From: Havard Skinnemoen <havard@skinnemoen.net>
>>
>> Fix a race in macb_start_xmit() where we unconditionally set the TSTART bit.
>> If an underrun just happened (we do this with interrupts disabled, so it might
>> not have been handled yet), the controller starts transmitting from the first
>> entry in the ring, which is usually wrong.
>> Restart the controller after error handling.
>>
>> Signed-off-by: Havard Skinnemoen <havard@skinnemoen.net>
>> [nicolas.ferre@atmel.com: split patch in topics]
>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>
> Accumulating special case code and checks into the hot path of TX packet
> processing is extremely unwise.
>
> Instead, when you handle the TX error conditions and reset the chip you
> should first ensure that there are no flows of control in the transmit
> function of your driver by using the appropriate locking et al. facilities.

IIRC, the hardware resets the ring pointers when an error happens, and
if we set TSTART right after that happens, the hardware will happily
transmit whatever is sitting in the beginning of the ring. This is
what I was trying to avoid.

The details are a bit hazy as it's been a while since I looked at
this, so it could be that simply letting it happen and using a bigger
hammer during reset processing might work just as well. Just want to
make sure y'all understand that we're talking about a race against
hardware, not against interrupt handlers, threads or anything that can
be solved by locking :)

Havard

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 08/10] net/macb: macb_get_drvinfo: add GEM/MACB suffix to differentiate revision
  2012-09-06 14:01     ` Nicolas Ferre
@ 2012-09-06 17:42       ` David Miller
  0 siblings, 0 replies; 23+ messages in thread
From: David Miller @ 2012-09-06 17:42 UTC (permalink / raw)
  To: nicolas.ferre
  Cc: bhutchings, netdev, linux-arm-kernel, havard, plagnioj, jamie,
	linux-kernel, patrice.vilchez

From: Nicolas Ferre <nicolas.ferre@atmel.com>
Date: Thu, 6 Sep 2012 16:01:34 +0200

> Absolutely, I will do this.

Please, when you receive feedback on your patches, you need to
resubmit the whole patch series for review not just the patches where
changes were asked for.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 04/10] net/macb: Fix a race in macb_start_xmit()
  2012-09-06 15:49     ` Havard Skinnemoen
@ 2012-09-07 16:34       ` Nicolas Ferre
  0 siblings, 0 replies; 23+ messages in thread
From: Nicolas Ferre @ 2012-09-07 16:34 UTC (permalink / raw)
  To: Havard Skinnemoen, David Miller, netdev
  Cc: linux-arm-kernel, plagnioj, jamie, linux-kernel, patrice.vilchez

On 09/06/2012 05:49 PM, Havard Skinnemoen :
> On Wed, Sep 5, 2012 at 11:30 PM, David Miller <davem@davemloft.net> wrote:
>> From: Nicolas Ferre <nicolas.ferre@atmel.com>
>> Date: Wed, 5 Sep 2012 10:19:11 +0200
>>
>>> From: Havard Skinnemoen <havard@skinnemoen.net>
>>>
>>> Fix a race in macb_start_xmit() where we unconditionally set the TSTART bit.
>>> If an underrun just happened (we do this with interrupts disabled, so it might
>>> not have been handled yet), the controller starts transmitting from the first
>>> entry in the ring, which is usually wrong.
>>> Restart the controller after error handling.
>>>
>>> Signed-off-by: Havard Skinnemoen <havard@skinnemoen.net>
>>> [nicolas.ferre@atmel.com: split patch in topics]
>>> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>>
>> Accumulating special case code and checks into the hot path of TX packet
>> processing is extremely unwise.
>>
>> Instead, when you handle the TX error conditions and reset the chip you
>> should first ensure that there are no flows of control in the transmit
>> function of your driver by using the appropriate locking et al. facilities.
> 
> IIRC, the hardware resets the ring pointers when an error happens, and
> if we set TSTART right after that happens, the hardware will happily
> transmit whatever is sitting in the beginning of the ring. This is
> what I was trying to avoid.
> 
> The details are a bit hazy as it's been a while since I looked at
> this, so it could be that simply letting it happen and using a bigger
> hammer during reset processing might work just as well. Just want to
> make sure y'all understand that we're talking about a race against
> hardware, not against interrupt handlers, threads or anything that can
> be solved by locking :)

Yes, you are right Havard.

I will see if we can let the transmitter go just before being
interrupted by the pending error.

It is true that there are several cases here:
- tx immediately stopped again by the USED bit of a non initialized
descriptor. We thus have to cleanup the error frame but also take care
about the newly queued packet...
- beginning of transmission of a not-related fragment that has just been
queued by the start_xmit; just before catching the pending error IRQ. We
may have to consider the consequences of this!

So, stay tuned ;-)

Best regards,
-- 
Nicolas Ferre

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2012-09-07 16:35 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-05  8:19 [PATCH 00/10] net/macb: driver enhancement concerning GEM support, ring logic and cleanup Nicolas Ferre
2012-09-05  8:19 ` [PATCH 01/10] net/macb: Add support for Gigabit Ethernet mode Nicolas Ferre
2012-09-05  8:19 ` [PATCH 02/10] net/macb: memory barriers cleanup Nicolas Ferre
2012-09-05  8:19 ` [PATCH 03/10] net/macb: change debugging messages Nicolas Ferre
2012-09-05  8:19 ` [PATCH 04/10] net/macb: Fix a race in macb_start_xmit() Nicolas Ferre
2012-09-05 21:30   ` David Miller
2012-09-06 14:52     ` Nicolas Ferre
2012-09-06 15:49     ` Havard Skinnemoen
2012-09-07 16:34       ` Nicolas Ferre
2012-09-05  9:00 ` [PATCH 05/10] net/macb: clean up ring buffer logic Nicolas Ferre
2012-09-05  9:00 ` [PATCH 06/10] net/macb: better manage tx errors Nicolas Ferre
2012-09-05  9:00 ` [PATCH 07/10] net/macb: tx status is more than 8 bits now Nicolas Ferre
2012-09-05  9:00 ` [PATCH 08/10] net/macb: macb_get_drvinfo: add GEM/MACB suffix to differentiate revision Nicolas Ferre
2012-09-05 21:31   ` David Miller
2012-09-05 23:27   ` Ben Hutchings
2012-09-06 14:01     ` Nicolas Ferre
2012-09-06 17:42       ` David Miller
2012-09-05  9:00 ` [PATCH 09/10] net/macb: ethtool interface: add register dump feature Nicolas Ferre
2012-09-05 21:32   ` David Miller
2012-09-05 23:36   ` Ben Hutchings
2012-09-06 14:20     ` [PATCH v2 " Nicolas Ferre
2012-09-06 14:34       ` Ben Hutchings
2012-09-05  9:04 ` [PATCH 10/10] net/macb: Offset first RX buffer by two bytes Nicolas Ferre

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).