[PATCH 0/6] net/macb: add HW features to macb driver

[PATCH 0/6] net/macb: add HW features to macb driver

[Cyrille Pitchen]

Hi,

this series of patches adds new hardware features to macb driver. These
features can be enabled/disabled at runtime using ethtool. Depending on
hardware and design configuration, some are enabled by default whereas other
are disabled.

For instance, checksum offload features are enabled by default for gem designed
for packet buffer mode but disabled for fifo mode design or for old macb.

Besides, the scatter-gather feature is enabled and tested on macb but disabled
on sama5d3x gem. When testing this feature on sama5d3x gem, TX lockups occured
frequently.

These patches were made for net-next.git repository.

Cyrille Pitchen (5):
  net/macb: add scatter-gather hw feature
  net/macb: add TX checksum offload feature
  net/macb: add RX checksum offload feature
  ARM: at91: change compatibility string for sama5d3x gem
  net/macb: enable scatter-gather feature and set DMA burst length for
    sama5d4 gem

Nicolas Ferre (1):
  net/macb: configure for FIFO mode and non-gigabit

 arch/arm/boot/dts/sama5d3_gmac.dtsi |   2 +-
 drivers/net/ethernet/cadence/macb.c | 409 ++++++++++++++++++++++++++++++------
 drivers/net/ethernet/cadence/macb.h |  36 +++-
 3 files changed, 375 insertions(+), 72 deletions(-)

-- 
1.8.2.2

[PATCH 1/6] net/macb: configure for FIFO mode and non-gigabit

[Cyrille Pitchen]

From: Nicolas Ferre <nicolas.ferre@atmel.com>

This addition will also allow to configure DMA burst length.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Acked-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
---
 drivers/net/ethernet/cadence/macb.c | 72 +++++++++++++++++++++++++++----------
 drivers/net/ethernet/cadence/macb.h | 19 ++++++++--
 2 files changed, 71 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index e9daa07..0896d88 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -264,7 +264,8 @@ static void macb_handle_link_change(struct net_device *dev)
 				reg |= MACB_BIT(FD);
 			if (phydev->speed == SPEED_100)
 				reg |= MACB_BIT(SPD);
-			if (phydev->speed == SPEED_1000)
+			if (phydev->speed == SPEED_1000
+			 && bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
 				reg |= GEM_BIT(GBE);
 
 			macb_or_gem_writel(bp, NCFGR, reg);
@@ -337,7 +338,7 @@ static int macb_mii_probe(struct net_device *dev)
 	}
 
 	/* mask with MAC supported features */
-	if (macb_is_gem(bp))
+	if (macb_is_gem(bp) && bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)
 		phydev->supported &= PHY_GBIT_FEATURES;
 	else
 		phydev->supported &= PHY_BASIC_FEATURES;
@@ -1342,7 +1343,7 @@ static u32 macb_dbw(struct macb *bp)
 /*
  * Configure the receive DMA engine
  * - use the correct receive buffer size
- * - set the possibility to use INCR16 bursts
+ * - set best burst length for DMA operations
  *   (if not supported by FIFO, it will fallback to default)
  * - set both rx/tx packet buffers to full memory size
  * These are configurable parameters for GEM.
@@ -1354,24 +1355,15 @@ static void macb_configure_dma(struct macb *bp)
 	if (macb_is_gem(bp)) {
 		dmacfg = gem_readl(bp, DMACFG) & ~GEM_BF(RXBS, -1L);
 		dmacfg |= GEM_BF(RXBS, bp->rx_buffer_size / RX_BUFFER_MULTIPLE);
-		dmacfg |= GEM_BF(FBLDO, 16);
+		if (bp->dma_burst_length)
+			dmacfg = GEM_BFINS(FBLDO, bp->dma_burst_length, dmacfg);
 		dmacfg |= GEM_BIT(TXPBMS) | GEM_BF(RXBMS, -1L);
 		dmacfg &= ~GEM_BIT(ENDIA);
+		netdev_dbg(bp->dev, "Cadence configure DMA with 0x%08x\n", dmacfg);
 		gem_writel(bp, DMACFG, dmacfg);
 	}
 }
 
-/*
- * Configure peripheral capacities according to integration options used
- */
-static void macb_configure_caps(struct macb *bp)
-{
-	if (macb_is_gem(bp)) {
-		if (GEM_BFEXT(IRQCOR, gem_readl(bp, DCFG1)) == 0)
-			bp->caps |= MACB_CAPS_ISR_CLEAR_ON_WRITE;
-	}
-}
-
 static void macb_init_hw(struct macb *bp)
 {
 	u32 config;
@@ -1394,7 +1386,6 @@ static void macb_init_hw(struct macb *bp)
 	bp->duplex = DUPLEX_HALF;
 
 	macb_configure_dma(bp);
-	macb_configure_caps(bp);
 
 	/* Initialize TX and RX buffers */
 	macb_writel(bp, RBQP, bp->rx_ring_dma);
@@ -1783,17 +1774,59 @@ static const struct net_device_ops macb_netdev_ops = {
 };
 
 #if defined(CONFIG_OF)
+static struct macb_config pc302gem_config = {
+	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE,
+	.dma_burst_length = 16,
+};
+
 static const struct of_device_id macb_dt_ids[] = {
 	{ .compatible = "cdns,at32ap7000-macb" },
 	{ .compatible = "cdns,at91sam9260-macb" },
 	{ .compatible = "cdns,macb" },
-	{ .compatible = "cdns,pc302-gem" },
-	{ .compatible = "cdns,gem" },
+	{ .compatible = "cdns,pc302-gem", .data = &pc302gem_config },
+	{ .compatible = "cdns,gem", .data = &pc302gem_config },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, macb_dt_ids);
 #endif
 
+/*
+ * Configure peripheral capacities according to device tree
+ * and integration options used
+ */
+static void macb_configure_caps(struct macb *bp)
+{
+	u32 dcfg;
+	const struct of_device_id *match;
+
+	if (bp->pdev->dev.of_node) {
+		match = of_match_node(macb_dt_ids, bp->pdev->dev.of_node);
+		if (match && match->data) {
+			bp->caps = ((struct macb_config *)match->data)->caps;
+			/*
+			 * As we have access to the matching node, configure
+			 * DMA burst length as well
+			 */
+			bp->dma_burst_length =
+				((struct macb_config *)match->data)->dma_burst_length;
+		}
+	}
+
+	if (MACB_BFEXT(IDNUM, macb_readl(bp, MID)) == 0x2)
+		bp->caps |= MACB_CAPS_MACB_IS_GEM;
+
+	if (macb_is_gem(bp)) {
+		dcfg = gem_readl(bp, DCFG1);
+		if (GEM_BFEXT(IRQCOR, dcfg) == 0)
+			bp->caps |= MACB_CAPS_ISR_CLEAR_ON_WRITE;
+		dcfg = gem_readl(bp, DCFG2);
+		if ((dcfg & (GEM_BIT(RX_PKT_BUFF) | GEM_BIT(TX_PKT_BUFF))) == 0)
+			bp->caps |= MACB_CAPS_FIFO_MODE;
+	}
+
+	netdev_dbg(bp->dev, "Cadence caps 0x%08x\n", bp->caps);
+}
+
 static int __init macb_probe(struct platform_device *pdev)
 {
 	struct macb_platform_data *pdata;
@@ -1897,6 +1930,9 @@ static int __init macb_probe(struct platform_device *pdev)
 
 	dev->base_addr = regs->start;
 
+	/* setup capacities */
+	macb_configure_caps(bp);
+
 	/* setup appropriated routines according to adapter type */
 	if (macb_is_gem(bp)) {
 		bp->macbgem_ops.mog_alloc_rx_buffers = gem_alloc_rx_buffers;
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 51c0244..7ce751b 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -305,6 +305,12 @@
 #define GEM_DBWDEF_OFFSET			25
 #define GEM_DBWDEF_SIZE				3
 
+/* Bitfields in DCFG2. */
+#define GEM_RX_PKT_BUFF_OFFSET			20
+#define GEM_RX_PKT_BUFF_SIZE			1
+#define GEM_TX_PKT_BUFF_OFFSET			21
+#define GEM_TX_PKT_BUFF_SIZE			1
+
 /* Constants for CLK */
 #define MACB_CLK_DIV8				0
 #define MACB_CLK_DIV16				1
@@ -326,7 +332,10 @@
 #define MACB_MAN_CODE				2
 
 /* Capability mask bits */
-#define MACB_CAPS_ISR_CLEAR_ON_WRITE		0x1
+#define MACB_CAPS_ISR_CLEAR_ON_WRITE		0x00000001
+#define MACB_CAPS_FIFO_MODE			0x10000000
+#define MACB_CAPS_GIGABIT_MODE_AVAILABLE	0x20000000
+#define MACB_CAPS_MACB_IS_GEM			0x80000000
 
 /* Bit manipulation macros */
 #define MACB_BIT(name)					\
@@ -554,6 +563,11 @@ struct macb_or_gem_ops {
 	int	(*mog_rx)(struct macb *bp, int budget);
 };
 
+struct macb_config {
+	u32			caps;
+	unsigned int		dma_burst_length;
+};
+
 struct macb {
 	void __iomem		*regs;
 
@@ -595,6 +609,7 @@ struct macb {
 	unsigned int 		duplex;
 
 	u32			caps;
+	unsigned int		dma_burst_length;
 
 	phy_interface_t		phy_interface;
 
@@ -615,7 +630,7 @@ void macb_get_hwaddr(struct macb *bp);
 
 static inline bool macb_is_gem(struct macb *bp)
 {
-	return MACB_BFEXT(IDNUM, macb_readl(bp, MID)) == 0x2;
+	return !!(bp->caps & MACB_CAPS_MACB_IS_GEM);
 }
 
 #endif /* _MACB_H */
-- 
1.8.2.2

[PATCH 2/6] net/macb: add scatter-gather hw feature

[Cyrille Pitchen]

The scatter-gather feature will allow to enable the Generic Segmentation Offload.
Generic Segmentation Offload can be enabled/disabled using ethtool -K DEVNAME gso on|off.

e.g:
ethtool -K eth0 gso off

When enabled, the driver may be provided with socket buffers splitted into many fragments.
These fragments need to be queued into the TX ring in reverse order, starting from to the
last one down to the first one, to avoid a race condition with the MAC.
Especially the 'TX_USED' bit in word 1 of the transmit buffer descriptor of the
first fragment should be cleared at the very final step of the queueing algorithm.
This will tell the hardware that fragments are ready to be sent.

Also since the MAC only update the status word of the first buffer descriptor of the
ethernet frame, the queueing algorithm can no longer expect a 'TX_USED' bit to be set by
the MAC into the buffer descriptor following the one for last fragment of the skb.
This is why the driver sets the 'TX_USED' bit before queueing any fragment, so the end of
queue position is well defined for the MAC.

Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
---
 drivers/net/ethernet/cadence/macb.c | 276 +++++++++++++++++++++++++++++-------
 drivers/net/ethernet/cadence/macb.h |  15 +-
 2 files changed, 239 insertions(+), 52 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 0896d88..06e9934 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -52,6 +52,9 @@
 					| MACB_BIT(TXERR))
 #define MACB_TX_INT_FLAGS	(MACB_TX_ERR_FLAGS | MACB_BIT(TCOMP))
 
+#define MACB_MAX_TX_LEN		((unsigned int)((1 << MACB_TX_FRMLEN_SIZE) - 1))
+#define GEM_MAX_TX_LEN		((unsigned int)((1 << GEM_TX_FRMLEN_SIZE) - 1))
+
 /*
  * Graceful stop timeouts in us. We should allow up to
  * 1 frame time (10 Mbits/s, full-duplex, ignoring collisions)
@@ -468,6 +471,25 @@ static int macb_halt_tx(struct macb *bp)
 	return -ETIMEDOUT;
 }
 
+static void macb_tx_unmap(struct macb *bp, struct macb_tx_skb *tx_skb)
+{
+	if (tx_skb->mapping) {
+		if (tx_skb->mapped_as_page) {
+			dma_unmap_page(&bp->pdev->dev, tx_skb->mapping,
+				       tx_skb->size, DMA_TO_DEVICE);
+		} else {
+			dma_unmap_single(&bp->pdev->dev, tx_skb->mapping,
+					 tx_skb->size, DMA_TO_DEVICE);
+		}
+		tx_skb->mapping = 0;
+	}
+
+	if (tx_skb->skb) {
+		dev_kfree_skb_any(tx_skb->skb);
+		tx_skb->skb = NULL;
+	}
+}
+
 static void macb_tx_error_task(struct work_struct *work)
 {
 	struct macb	*bp = container_of(work, struct macb, tx_error_task);
@@ -505,10 +527,23 @@ static void macb_tx_error_task(struct work_struct *work)
 		skb = tx_skb->skb;
 
 		if (ctrl & MACB_BIT(TX_USED)) {
-			netdev_vdbg(bp->dev, "txerr skb %u (data %p) TX complete\n",
-				    macb_tx_ring_wrap(tail), skb->data);
-			bp->stats.tx_packets++;
-			bp->stats.tx_bytes += skb->len;
+			/* skb is set for the last buffer of the frame */
+			while (!skb) {
+				macb_tx_unmap(bp, tx_skb);
+				tail++;
+				tx_skb = macb_tx_skb(bp, tail);
+				skb = tx_skb->skb;
+			}
+
+			/* ctrl still refers to the first buffer descriptor
+			 * since it's the only one written back by the hardware
+			 */
+			if (!(ctrl & MACB_BIT(TX_BUF_EXHAUSTED))) {
+				netdev_vdbg(bp->dev, "txerr skb %u (data %p) TX complete\n",
+					    macb_tx_ring_wrap(tail), skb->data);
+				bp->stats.tx_packets++;
+				bp->stats.tx_bytes += skb->len;
+			}
 		} else {
 			/*
 			 * "Buffers exhausted mid-frame" errors may only happen
@@ -522,10 +557,7 @@ static void macb_tx_error_task(struct work_struct *work)
 			desc->ctrl = 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(skb);
+		macb_tx_unmap(bp, tx_skb);
 	}
 
 	/* Make descriptor updates visible to hardware */
@@ -573,20 +605,35 @@ static void macb_tx_interrupt(struct macb *bp)
 
 		ctrl = desc->ctrl;
 
+		/* TX_USED bit is only set by hardware on the very first buffer
+		 * descriptor of the transmitted frame.
+		 */
 		if (!(ctrl & MACB_BIT(TX_USED)))
 			break;
 
-		tx_skb = macb_tx_skb(bp, tail);
-		skb = tx_skb->skb;
+		/* Process all buffers of the current transmitted frame */
+		for (;; tail++) {
+			tx_skb = macb_tx_skb(bp, tail);
+			skb = tx_skb->skb;
+
+			/* First, update TX stats if needed */
+			if (skb) {
+				netdev_vdbg(bp->dev, "skb %u (data %p) TX complete\n",
+					macb_tx_ring_wrap(tail), skb->data);
+				bp->stats.tx_packets++;
+				bp->stats.tx_bytes += skb->len;
+			}
 
-		netdev_vdbg(bp->dev, "skb %u (data %p) TX complete\n",
-			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;
-		tx_skb->skb = NULL;
-		dev_kfree_skb_irq(skb);
+			/* Now we can safely release resources */
+			macb_tx_unmap(bp, tx_skb);
+
+			/* skb is set only for the last buffer of the frame.
+			 * WARNING: at this point skb has been freed by
+			 * macb_tx_unmap().
+			 */
+			if (skb)
+				break;
+		}
 	}
 
 	bp->tx_tail = tail;
@@ -1002,15 +1049,142 @@ static void macb_poll_controller(struct net_device *dev)
 }
 #endif
 
-static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
+static inline unsigned int macb_count_tx_descriptors(struct macb *bp,
+						     unsigned int len)
+{
+	return (len + bp->max_tx_length - 1) / bp->max_tx_length;
+}
+
+static unsigned int macb_tx_map(struct macb *bp,
+				struct sk_buff *skb)
 {
-	struct macb *bp = netdev_priv(dev);
 	dma_addr_t mapping;
-	unsigned int len, entry;
+	unsigned int len, entry, i, tx_head = bp->tx_head;
+	struct macb_tx_skb *tx_skb = NULL;
 	struct macb_dma_desc *desc;
-	struct macb_tx_skb *tx_skb;
+	unsigned int offset, size, count = 0;
+	unsigned int f, nr_frags = skb_shinfo(skb)->nr_frags;
+	unsigned int eof = 1;
 	u32 ctrl;
+
+	/* First, map non-paged data */
+	len = skb_headlen(skb);
+	offset = 0;
+	while (len) {
+		size = min(len, bp->max_tx_length);
+		entry = macb_tx_ring_wrap(tx_head);
+		tx_skb = &bp->tx_skb[entry];
+
+		mapping = dma_map_single(&bp->pdev->dev,
+					 skb->data + offset,
+					 size, DMA_TO_DEVICE);
+		if (dma_mapping_error(&bp->pdev->dev, mapping))
+			goto dma_error;
+
+		/* Save info to properly release resources */
+		tx_skb->skb = NULL;
+		tx_skb->mapping = mapping;
+		tx_skb->size = size;
+		tx_skb->mapped_as_page = false;
+
+		len -= size;
+		offset += size;
+		count++;
+		tx_head++;
+	}
+
+	/* Then, map paged data from fragments */
+	for (f = 0; f < nr_frags; f++) {
+		const skb_frag_t *frag = &skb_shinfo(skb)->frags[f];
+
+		len = skb_frag_size(frag);
+		offset = 0;
+		while (len) {
+			size = min(len, bp->max_tx_length);
+			entry = macb_tx_ring_wrap(tx_head);
+			tx_skb = &bp->tx_skb[entry];
+
+			mapping = skb_frag_dma_map(&bp->pdev->dev, frag,
+						   offset, size, DMA_TO_DEVICE);
+			if (dma_mapping_error(&bp->pdev->dev, mapping))
+				goto dma_error;
+
+			/* Save info to properly release resources */
+			tx_skb->skb = NULL;
+			tx_skb->mapping = mapping;
+			tx_skb->size = size;
+			tx_skb->mapped_as_page = true;
+
+			len -= size;
+			offset += size;
+			count++;
+			tx_head++;
+		}
+	}
+
+	/* Should never happen */
+	if (unlikely(tx_skb == NULL)) {
+		netdev_err(bp->dev, "BUG! empty skb!\n");
+		return 0;
+	}
+
+	/* This is the last buffer of the frame: save socket buffer */
+	tx_skb->skb = skb;
+
+	/* Update TX ring: update buffer descriptors in reverse order
+	 * to avoid race condition
+	 */
+
+	/* Set 'TX_USED' bit in buffer descriptor at tx_head position
+	 * to set the end of TX queue
+	 */
+	i = tx_head;
+	entry = macb_tx_ring_wrap(i);
+	ctrl = MACB_BIT(TX_USED);
+	desc = &bp->tx_ring[entry];
+	desc->ctrl = ctrl;
+
+	do {
+		i--;
+		entry = macb_tx_ring_wrap(i);
+		tx_skb = &bp->tx_skb[entry];
+		desc = &bp->tx_ring[entry];
+
+		ctrl = (u32)tx_skb->size;
+		if (eof) {
+			ctrl |= MACB_BIT(TX_LAST);
+			eof = 0;
+		}
+		if (unlikely(entry == (TX_RING_SIZE - 1)))
+			ctrl |= MACB_BIT(TX_WRAP);
+
+		/* Set TX buffer descriptor */
+		desc->addr = tx_skb->mapping;
+		wmb();
+		desc->ctrl = ctrl;
+	} while (i != bp->tx_head);
+
+	bp->tx_head = tx_head;
+
+	return count;
+
+dma_error:
+	netdev_err(bp->dev, "TX DMA map failed\n");
+
+	for (i = bp->tx_head; i != tx_head; i++) {
+		tx_skb = macb_tx_skb(bp, i);
+
+		macb_tx_unmap(bp, tx_skb);
+	}
+
+	return 0;
+}
+
+static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+	struct macb *bp = netdev_priv(dev);
 	unsigned long flags;
+	unsigned int count, nr_frags, frag_size, f;
 
 #if defined(DEBUG) && defined(VERBOSE_DEBUG)
 	netdev_vdbg(bp->dev,
@@ -1021,44 +1195,35 @@ static int macb_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		       skb->data, 16, true);
 #endif
 
-	len = skb->len;
+	/*
+	 * Count how many TX buffer descriptors are needed to send this
+	 * socket buffer: skb fragments of jumbo frames may need to be
+	 * splitted into many buffer descriptors.
+	 */
+	count = macb_count_tx_descriptors(bp, skb_headlen(skb));
+	nr_frags = skb_shinfo(skb)->nr_frags;
+	for (f = 0; f < nr_frags; f++) {
+		frag_size = skb_frag_size(&skb_shinfo(skb)->frags[f]);
+		count += macb_count_tx_descriptors(bp, frag_size);
+	}
+
 	spin_lock_irqsave(&bp->lock, flags);
 
 	/* This is a hard error, log it. */
-	if (CIRC_SPACE(bp->tx_head, bp->tx_tail, TX_RING_SIZE) < 1) {
+	if (CIRC_SPACE(bp->tx_head, bp->tx_tail, TX_RING_SIZE) < count) {
 		netif_stop_queue(dev);
 		spin_unlock_irqrestore(&bp->lock, flags);
-		netdev_err(bp->dev, "BUG! Tx Ring full when queue awake!\n");
 		netdev_dbg(bp->dev, "tx_head = %u, tx_tail = %u\n",
 			   bp->tx_head, bp->tx_tail);
 		return NETDEV_TX_BUSY;
 	}
 
-	entry = macb_tx_ring_wrap(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);
-	if (dma_mapping_error(&bp->pdev->dev, mapping)) {
+	/* Map socket buffer for DMA transfer */
+	if (!macb_tx_map(bp, skb)) {
 		dev_kfree_skb_any(skb);
 		goto unlock;
 	}
 
-	bp->tx_head++;
-	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);
-
-	ctrl = MACB_BF(TX_FRMLEN, len);
-	ctrl |= MACB_BIT(TX_LAST);
-	if (entry == (TX_RING_SIZE - 1))
-		ctrl |= MACB_BIT(TX_WRAP);
-
-	desc = &bp->tx_ring[entry];
-	desc->addr = mapping;
-	desc->ctrl = ctrl;
-
 	/* Make newly initialized descriptor visible to hardware */
 	wmb();
 
@@ -1775,7 +1940,12 @@ static const struct net_device_ops macb_netdev_ops = {
 
 #if defined(CONFIG_OF)
 static struct macb_config pc302gem_config = {
-	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE,
+	.caps = MACB_CAPS_SG_DISABLED | MACB_CAPS_GIGABIT_MODE_AVAILABLE,
+	.dma_burst_length = 16,
+};
+
+static struct macb_config sama5d3_config = {
+	.caps = MACB_CAPS_SG_DISABLED | MACB_CAPS_GIGABIT_MODE_AVAILABLE,
 	.dma_burst_length = 16,
 };
 
@@ -1785,6 +1955,7 @@ static const struct of_device_id macb_dt_ids[] = {
 	{ .compatible = "cdns,macb" },
 	{ .compatible = "cdns,pc302-gem", .data = &pc302gem_config },
 	{ .compatible = "cdns,gem", .data = &pc302gem_config },
+	{ .compatible = "atmel,sama5d3-gem", .data = &sama5d3_config },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, macb_dt_ids);
@@ -1861,9 +2032,6 @@ static int __init macb_probe(struct platform_device *pdev)
 
 	SET_NETDEV_DEV(dev, &pdev->dev);
 
-	/* TODO: Actually, we have some interesting features... */
-	dev->features |= 0;
-
 	bp = netdev_priv(dev);
 	bp->pdev = pdev;
 	bp->dev = dev;
@@ -1935,17 +2103,25 @@ static int __init macb_probe(struct platform_device *pdev)
 
 	/* setup appropriated routines according to adapter type */
 	if (macb_is_gem(bp)) {
+		bp->max_tx_length = GEM_MAX_TX_LEN;
 		bp->macbgem_ops.mog_alloc_rx_buffers = gem_alloc_rx_buffers;
 		bp->macbgem_ops.mog_free_rx_buffers = gem_free_rx_buffers;
 		bp->macbgem_ops.mog_init_rings = gem_init_rings;
 		bp->macbgem_ops.mog_rx = gem_rx;
 	} else {
+		bp->max_tx_length = MACB_MAX_TX_LEN;
 		bp->macbgem_ops.mog_alloc_rx_buffers = macb_alloc_rx_buffers;
 		bp->macbgem_ops.mog_free_rx_buffers = macb_free_rx_buffers;
 		bp->macbgem_ops.mog_init_rings = macb_init_rings;
 		bp->macbgem_ops.mog_rx = macb_rx;
 	}
 
+	/* Set features */
+	dev->hw_features = NETIF_F_SG;
+	if (bp->caps & MACB_CAPS_SG_DISABLED)
+		dev->hw_features &= ~NETIF_F_SG;
+	dev->features = dev->hw_features;
+
 	/* Set MII management clock divider */
 	config = macb_mdc_clk_div(bp);
 	config |= macb_dbw(bp);
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 7ce751b..7bf8285 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -335,6 +335,7 @@
 #define MACB_CAPS_ISR_CLEAR_ON_WRITE		0x00000001
 #define MACB_CAPS_FIFO_MODE			0x10000000
 #define MACB_CAPS_GIGABIT_MODE_AVAILABLE	0x20000000
+#define MACB_CAPS_SG_DISABLED			0x40000000
 #define MACB_CAPS_MACB_IS_GEM			0x80000000
 
 /* Bit manipulation macros */
@@ -468,14 +469,23 @@ struct macb_dma_desc {
 #define MACB_TX_USED_OFFSET			31
 #define MACB_TX_USED_SIZE			1
 
+#define GEM_TX_FRMLEN_OFFSET			0
+#define GEM_TX_FRMLEN_SIZE			14
+
 /**
  * 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
+ * @skb: skb currently being transmitted, only set for the last buffer
+ *       of the frame
+ * @mapping: DMA address of the skb's fragment buffer
+ * @size: size of the DMA mapped buffer
+ * @mapped_as_page: true when buffer was mapped with skb_frag_dma_map(),
+ *                  false when buffer was mapped with dma_map_single()
  */
 struct macb_tx_skb {
 	struct sk_buff		*skb;
 	dma_addr_t		mapping;
+	size_t			size;
+	bool			mapped_as_page;
 };
 
 /*
@@ -617,6 +627,7 @@ struct macb {
 	struct sk_buff *skb;			/* holds skb until xmit interrupt completes */
 	dma_addr_t skb_physaddr;		/* phys addr from pci_map_single */
 	int skb_length;				/* saved skb length for pci_unmap_single */
+	unsigned int		max_tx_length;
 };
 
 extern const struct ethtool_ops macb_ethtool_ops;
-- 
1.8.2.2

[PATCH 3/6] net/macb: add TX checksum offload feature

[Cyrille Pitchen]

Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
---
 drivers/net/ethernet/cadence/macb.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 06e9934..9bdcd1b 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -1524,6 +1524,10 @@ static void macb_configure_dma(struct macb *bp)
 			dmacfg = GEM_BFINS(FBLDO, bp->dma_burst_length, dmacfg);
 		dmacfg |= GEM_BIT(TXPBMS) | GEM_BF(RXBMS, -1L);
 		dmacfg &= ~GEM_BIT(ENDIA);
+		if (bp->dev->features & NETIF_F_HW_CSUM)
+			dmacfg |= GEM_BIT(TXCOEN);
+		else
+			dmacfg &= ~GEM_BIT(TXCOEN);
 		netdev_dbg(bp->dev, "Cadence configure DMA with 0x%08x\n", dmacfg);
 		gem_writel(bp, DMACFG, dmacfg);
 	}
@@ -1923,6 +1927,27 @@ int macb_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 }
 EXPORT_SYMBOL_GPL(macb_ioctl);
 
+static int macb_set_features(struct net_device *netdev,
+			     netdev_features_t features)
+{
+	struct macb *bp = netdev_priv(netdev);
+	netdev_features_t changed = features ^ netdev->features;
+
+	/* TX checksum offload */
+	if ((changed & NETIF_F_HW_CSUM) && macb_is_gem(bp)) {
+		u32 dmacfg;
+
+		dmacfg = gem_readl(bp, DMACFG);
+		if (features & NETIF_F_HW_CSUM)
+			dmacfg |= GEM_BIT(TXCOEN);
+		else
+			dmacfg &= ~GEM_BIT(TXCOEN);
+		gem_writel(bp, DMACFG, dmacfg);
+	}
+
+	return 0;
+}
+
 static const struct net_device_ops macb_netdev_ops = {
 	.ndo_open		= macb_open,
 	.ndo_stop		= macb_close,
@@ -1936,6 +1961,7 @@ static const struct net_device_ops macb_netdev_ops = {
 #ifdef CONFIG_NET_POLL_CONTROLLER
 	.ndo_poll_controller	= macb_poll_controller,
 #endif
+	.ndo_set_features	= macb_set_features,
 };
 
 #if defined(CONFIG_OF)
@@ -2118,6 +2144,10 @@ static int __init macb_probe(struct platform_device *pdev)
 
 	/* Set features */
 	dev->hw_features = NETIF_F_SG;
+	if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE)) {
+		/* Checksum offload is only available on packet buffer design */
+		dev->hw_features |= NETIF_F_HW_CSUM;
+	}
 	if (bp->caps & MACB_CAPS_SG_DISABLED)
 		dev->hw_features &= ~NETIF_F_SG;
 	dev->features = dev->hw_features;
-- 
1.8.2.2

[PATCH 4/6] net/macb: add RX checksum offload feature

[Cyrille Pitchen]

Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
---
 drivers/net/ethernet/cadence/macb.c | 29 ++++++++++++++++++++++++++++-
 drivers/net/ethernet/cadence/macb.h |  2 ++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 9bdcd1b..6acd6e2 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -766,6 +766,8 @@ static int gem_rx(struct macb *bp, int budget)
 
 		skb->protocol = eth_type_trans(skb, bp->dev);
 		skb_checksum_none_assert(skb);
+		if (bp->dev->features & NETIF_F_RXCSUM)
+			skb->ip_summed = CHECKSUM_UNNECESSARY;
 
 		bp->stats.rx_packets++;
 		bp->stats.rx_bytes += skb->len;
@@ -1506,6 +1508,18 @@ static u32 macb_dbw(struct macb *bp)
 }
 
 /*
+ * Configure the RX checksum offload
+ */
+static u32 macb_rxcsum(struct macb *bp)
+{
+	if (macb_is_gem(bp) &&
+	    (bp->dev->features & NETIF_F_RXCSUM))
+		return GEM_BIT(RXCOEN);
+
+	return 0;
+}
+
+/*
  * Configure the receive DMA engine
  * - use the correct receive buffer size
  * - set best burst length for DMA operations
@@ -1550,6 +1564,7 @@ static void macb_init_hw(struct macb *bp)
 	if (!(bp->dev->flags & IFF_BROADCAST))
 		config |= MACB_BIT(NBC);	/* No BroadCast */
 	config |= macb_dbw(bp);
+	config |= macb_rxcsum(bp);
 	macb_writel(bp, NCFGR, config);
 	bp->speed = SPEED_10;
 	bp->duplex = DUPLEX_HALF;
@@ -1945,6 +1960,18 @@ static int macb_set_features(struct net_device *netdev,
 		gem_writel(bp, DMACFG, dmacfg);
 	}
 
+	/* RX checksum offload */
+	if ((changed & NETIF_F_RXCSUM) && macb_is_gem(bp)) {
+		u32 netcfg;
+
+		netcfg = gem_readl(bp, NCFGR);
+		if (features & NETIF_F_RXCSUM)
+			netcfg |= GEM_BIT(RXCOEN);
+		else
+			netcfg &= ~GEM_BIT(RXCOEN);
+		gem_writel(bp, NCFGR, netcfg);
+	}
+
 	return 0;
 }
 
@@ -2146,7 +2173,7 @@ static int __init macb_probe(struct platform_device *pdev)
 	dev->hw_features = NETIF_F_SG;
 	if (macb_is_gem(bp) && !(bp->caps & MACB_CAPS_FIFO_MODE)) {
 		/* Checksum offload is only available on packet buffer design */
-		dev->hw_features |= NETIF_F_HW_CSUM;
+		dev->hw_features |= NETIF_F_HW_CSUM | NETIF_F_RXCSUM;
 	}
 	if (bp->caps & MACB_CAPS_SG_DISABLED)
 		dev->hw_features &= ~NETIF_F_SG;
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 7bf8285..3678b83 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -164,6 +164,8 @@
 #define GEM_CLK_SIZE				3
 #define GEM_DBW_OFFSET				21
 #define GEM_DBW_SIZE				2
+#define GEM_RXCOEN_OFFSET			24
+#define GEM_RXCOEN_SIZE				1
 
 /* Constants for data bus width. */
 #define GEM_DBW32				0
-- 
1.8.2.2

[PATCH 5/6] ARM: at91: change compatibility string for sama5d3x gem

[Cyrille Pitchen]

this new compatibility string prevents macb/gem driver from using the
scatter-gather and gso features on sama5d3x boards.

Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
---
 arch/arm/boot/dts/sama5d3_gmac.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/sama5d3_gmac.dtsi b/arch/arm/boot/dts/sama5d3_gmac.dtsi
index a6cb050..de5ed59 100644
--- a/arch/arm/boot/dts/sama5d3_gmac.dtsi
+++ b/arch/arm/boot/dts/sama5d3_gmac.dtsi
@@ -74,7 +74,7 @@
 			};
 
 			macb0: ethernet@f0028000 {
-				compatible = "cdns,pc302-gem", "cdns,gem";
+				compatible = "atmel,sama5d3-gem";
 				reg = <0xf0028000 0x100>;
 				interrupts = <34 IRQ_TYPE_LEVEL_HIGH 3>;
 				pinctrl-names = "default";
-- 
1.8.2.2

[PATCH 6/6] net/macb: enable scatter-gather feature and set DMA burst length for sama5d4 gem

[Cyrille Pitchen]

Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
---
 drivers/net/ethernet/cadence/macb.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 6acd6e2..08105dc 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -2002,6 +2002,11 @@ static struct macb_config sama5d3_config = {
 	.dma_burst_length = 16,
 };
 
+static struct macb_config sama5d4_config = {
+	.caps = 0,
+	.dma_burst_length = 4,
+};
+
 static const struct of_device_id macb_dt_ids[] = {
 	{ .compatible = "cdns,at32ap7000-macb" },
 	{ .compatible = "cdns,at91sam9260-macb" },
@@ -2009,6 +2014,7 @@ static const struct of_device_id macb_dt_ids[] = {
 	{ .compatible = "cdns,pc302-gem", .data = &pc302gem_config },
 	{ .compatible = "cdns,gem", .data = &pc302gem_config },
 	{ .compatible = "atmel,sama5d3-gem", .data = &sama5d3_config },
+	{ .compatible = "atmel,sama5d4-gem", .data = &sama5d4_config },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, macb_dt_ids);
-- 
1.8.2.2

Re: [PATCH 4/6] net/macb: add RX checksum offload feature

[Eric Dumazet]

On Fri, 2014-07-18 at 16:21 +0200, Cyrille Pitchen wrote:
> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> ---
>  drivers/net/ethernet/cadence/macb.c | 29 ++++++++++++++++++++++++++++-
>  drivers/net/ethernet/cadence/macb.h |  2 ++
>  2 files changed, 30 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index 9bdcd1b..6acd6e2 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -766,6 +766,8 @@ static int gem_rx(struct macb *bp, int budget)
>  
>  		skb->protocol = eth_type_trans(skb, bp->dev);
>  		skb_checksum_none_assert(skb);
> +		if (bp->dev->features & NETIF_F_RXCSUM)
> +			skb->ip_summed = CHECKSUM_UNNECESSARY;
>  


Really ?

If this is what you meant, this deserves a big and fat comment.

Re: [PATCH 1/6] net/macb: configure for FIFO mode and non-gigabit

[David Miller]

From: Cyrille Pitchen <cyrille.pitchen@atmel.com>
Date: Fri, 18 Jul 2014 16:21:13 +0200

> From: Nicolas Ferre <nicolas.ferre@atmel.com>
> 
> This addition will also allow to configure DMA burst length.
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> Acked-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> ---
>  drivers/net/ethernet/cadence/macb.c | 72 +++++++++++++++++++++++++++----------
>  drivers/net/ethernet/cadence/macb.h | 19 ++++++++--
>  2 files changed, 71 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index e9daa07..0896d88 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -264,7 +264,8 @@ static void macb_handle_link_change(struct net_device *dev)
>  				reg |= MACB_BIT(FD);
>  			if (phydev->speed == SPEED_100)
>  				reg |= MACB_BIT(SPD);
> -			if (phydev->speed == SPEED_1000)
> +			if (phydev->speed == SPEED_1000
> +			 && bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)

Conditional operators end lines, they do not begin lines.

Also you must start the second and subsequent lines at the very
first column after the openning parenthesis of the first line.

Re: [PATCH 2/6] net/macb: add scatter-gather hw feature

[David Miller]

From: Cyrille Pitchen <cyrille.pitchen@atmel.com>
Date: Fri, 18 Jul 2014 16:21:14 +0200

> +		if (tx_skb->mapped_as_page) {
> +			dma_unmap_page(&bp->pdev->dev, tx_skb->mapping,
> +				       tx_skb->size, DMA_TO_DEVICE);
> +		} else {
> +			dma_unmap_single(&bp->pdev->dev, tx_skb->mapping,
> +					 tx_skb->size, DMA_TO_DEVICE);
> +		}

Please do not use curly braces for basic block composed of a single statement.

Re: [PATCH 4/6] net/macb: add RX checksum offload feature

[Cyrille Pitchen]

Le 18/07/2014 17:36, Eric Dumazet a écrit :
> On Fri, 2014-07-18 at 16:21 +0200, Cyrille Pitchen wrote:
>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
>> ---
>>  drivers/net/ethernet/cadence/macb.c | 29 ++++++++++++++++++++++++++++-
>>  drivers/net/ethernet/cadence/macb.h |  2 ++
>>  2 files changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
>> index 9bdcd1b..6acd6e2 100644
>> --- a/drivers/net/ethernet/cadence/macb.c
>> +++ b/drivers/net/ethernet/cadence/macb.c
>> @@ -766,6 +766,8 @@ static int gem_rx(struct macb *bp, int budget)
>>  
>>  		skb->protocol = eth_type_trans(skb, bp->dev);
>>  		skb_checksum_none_assert(skb);
>> +		if (bp->dev->features & NETIF_F_RXCSUM)
>> +			skb->ip_summed = CHECKSUM_UNNECESSARY;
>>  
> 
> 
> Really ?
> 
> If this is what you meant, this deserves a big and fat comment.
> 
> 
> 
Hi Eric,

Isn't it the proper way to do? According to Cadence documentation about RX checksum offload:
"If any of the checksums (IP, TCP or UDP) are verified incorrect by the GEM, the packet is discarded."

If I understand, when RX checksum offload is enabled setting bit 24 of the Network Configuration Register, the driver only receives RX frames with correct checksum. Then it tells the kernel that there's no need to verify the checksum again in software. This is done setting skb->ip_summed to CHECKSUM_UNNECESSARY. Intel e1000 driver does the same in e1000_rx_checksum() function.

Also bit 24 of the Network Configuration Register is updated by macb_open() and macb_set_features() so it always matches the state of NETIF_F_RXCSUM flag in dev->features, once the network interface is up. That's why I'd rather read from dev->features than read from register.

Did I make a mistake? Is it the kind of comment you expect to be added?

Regards,

Cyrille

Re: [PATCH 4/6] net/macb: add RX checksum offload feature

[Florian Fainelli]

2014-07-22 9:27 GMT-07:00 Cyrille Pitchen <cyrille.pitchen@atmel.com>:
> Le 18/07/2014 17:36, Eric Dumazet a écrit :
>> On Fri, 2014-07-18 at 16:21 +0200, Cyrille Pitchen wrote:
>>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
>>> ---
>>>  drivers/net/ethernet/cadence/macb.c | 29 ++++++++++++++++++++++++++++-
>>>  drivers/net/ethernet/cadence/macb.h |  2 ++
>>>  2 files changed, 30 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
>>> index 9bdcd1b..6acd6e2 100644
>>> --- a/drivers/net/ethernet/cadence/macb.c
>>> +++ b/drivers/net/ethernet/cadence/macb.c
>>> @@ -766,6 +766,8 @@ static int gem_rx(struct macb *bp, int budget)
>>>
>>>              skb->protocol = eth_type_trans(skb, bp->dev);
>>>              skb_checksum_none_assert(skb);
>>> +            if (bp->dev->features & NETIF_F_RXCSUM)
>>> +                    skb->ip_summed = CHECKSUM_UNNECESSARY;
>>>
>>
>>
>> Really ?
>>
>> If this is what you meant, this deserves a big and fat comment.
>>
>>
>>
> Hi Eric,
>
> Isn't it the proper way to do? According to Cadence documentation about RX checksum offload:
> "If any of the checksums (IP, TCP or UDP) are verified incorrect by the GEM, the packet is discarded."
>
> If I understand, when RX checksum offload is enabled setting bit 24 of the Network Configuration Register, the driver only receives RX frames with correct checksum. Then it tells the kernel that there's no need to verify the checksum again in software. This is done setting skb->ip_summed to CHECKSUM_UNNECESSARY. Intel e1000 driver does the same in e1000_rx_checksum() function.

This is not the same thing as what the e1000 driver is doing, the
e1000 driver is checking a per-packet status mask which tells it
whether a given packet had its protocol level checksum computed by the
HW.

I think there might be some slight confusion here between the Ethernet
frame Frame Control Checksum which is at the Ethernet level, and will
(with most adapters not supporting RX_FCS_ERR) cause the Ethernet
controller to drop frames at the MAC level, and the higher level
protocol checksums such as the TCP or UDP checksum.

>
> Also bit 24 of the Network Configuration Register is updated by macb_open() and macb_set_features() so it always matches the state of NETIF_F_RXCSUM flag in dev->features, once the network interface is up. That's why I'd rather read from dev->features than read from register.

I don't think the Cadence GEM Ethernet controller is any different
than other hardware out there, there must be a per-packet status bit
which tells whether the TCP/UDP or other protocol checksum was
successfully validated by the hardware. One of the reason for that, is
that depending on the type of traffic your receive (e.g: ARP), there
might be no protocol-level checksum at all in the packet, and the
hardware should know about that.

>
> Did I make a mistake? Is it the kind of comment you expect to be added?
>
> Regards,
>
> Cyrille
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



-- 
Florian

Re: [PATCH 4/6] net/macb: add RX checksum offload feature

[Eric Dumazet]

On Tue, 2014-07-22 at 18:27 +0200, Cyrille Pitchen wrote:
> Le 18/07/2014 17:36, Eric Dumazet a écrit :
> > On Fri, 2014-07-18 at 16:21 +0200, Cyrille Pitchen wrote:
> >> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> >> ---
> >>  drivers/net/ethernet/cadence/macb.c | 29 ++++++++++++++++++++++++++++-
> >>  drivers/net/ethernet/cadence/macb.h |  2 ++
> >>  2 files changed, 30 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> >> index 9bdcd1b..6acd6e2 100644
> >> --- a/drivers/net/ethernet/cadence/macb.c
> >> +++ b/drivers/net/ethernet/cadence/macb.c
> >> @@ -766,6 +766,8 @@ static int gem_rx(struct macb *bp, int budget)
> >>  
> >>  		skb->protocol = eth_type_trans(skb, bp->dev);
> >>  		skb_checksum_none_assert(skb);
> >> +		if (bp->dev->features & NETIF_F_RXCSUM)
> >> +			skb->ip_summed = CHECKSUM_UNNECESSARY;
> >>  
> > 
> > 
> > Really ?
> > 
> > If this is what you meant, this deserves a big and fat comment.
> > 
> > 
> > 
> Hi Eric,
> 
> Isn't it the proper way to do? According to Cadence documentation
> about RX checksum offload:
> "If any of the checksums (IP, TCP or UDP) are verified incorrect by
> the GEM, the packet is discarded."
> 
> If I understand, when RX checksum offload is enabled setting bit 24 of
> the Network Configuration Register, the driver only receives RX frames
> with correct checksum. Then it tells the kernel that there's no need
> to verify the checksum again in software. This is done setting
> skb->ip_summed to CHECKSUM_UNNECESSARY. Intel e1000 driver does the
> same in e1000_rx_checksum() function.
> 
> Also bit 24 of the Network Configuration Register is updated by
> macb_open() and macb_set_features() so it always matches the state of
> NETIF_F_RXCSUM flag in dev->features, once the network interface is
> up. That's why I'd rather read from dev->features than read from
> register.
> 
> Did I make a mistake? Is it the kind of comment you expect to be
> added?
> 
> Regards,

Usually, frames are not discarded, and a bit is present in rx descriptor
to tell us if (TCP) checksum was validated by the NIC

We want tcpdump to get a copy of corrupted TCP frames, even if TCP stack
drops them later.

If this NIC drops incorrect frames, I am afraid we cant use this RX
offload at all.

Re: [PATCH 2/6] net/macb: add scatter-gather hw feature

[Cyrille Pitchen]

Le 21/07/2014 06:13, David Miller a écrit :
> From: Cyrille Pitchen <cyrille.pitchen@atmel.com>
> Date: Fri, 18 Jul 2014 16:21:14 +0200
> 
>> +		if (tx_skb->mapped_as_page) {
>> +			dma_unmap_page(&bp->pdev->dev, tx_skb->mapping,
>> +				       tx_skb->size, DMA_TO_DEVICE);
>> +		} else {
>> +			dma_unmap_single(&bp->pdev->dev, tx_skb->mapping,
>> +					 tx_skb->size, DMA_TO_DEVICE);
>> +		}
> 
> Please do not use curly braces for basic block composed of a single statement.
> 
Hi David,

thanks for your review, I'll fix it in the next series of patch.

Regards,

Cyrille

Re: [PATCH 4/6] net/macb: add RX checksum offload feature

[Cyrille Pitchen]

Le 23/07/2014 00:58, Eric Dumazet a écrit :
> On Tue, 2014-07-22 at 18:27 +0200, Cyrille Pitchen wrote:
>> Le 18/07/2014 17:36, Eric Dumazet a écrit :
>>> On Fri, 2014-07-18 at 16:21 +0200, Cyrille Pitchen wrote:
>>>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
>>>> ---
>>>>  drivers/net/ethernet/cadence/macb.c | 29 ++++++++++++++++++++++++++++-
>>>>  drivers/net/ethernet/cadence/macb.h |  2 ++
>>>>  2 files changed, 30 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
>>>> index 9bdcd1b..6acd6e2 100644
>>>> --- a/drivers/net/ethernet/cadence/macb.c
>>>> +++ b/drivers/net/ethernet/cadence/macb.c
>>>> @@ -766,6 +766,8 @@ static int gem_rx(struct macb *bp, int budget)
>>>>  
>>>>  		skb->protocol = eth_type_trans(skb, bp->dev);
>>>>  		skb_checksum_none_assert(skb);
>>>> +		if (bp->dev->features & NETIF_F_RXCSUM)
>>>> +			skb->ip_summed = CHECKSUM_UNNECESSARY;
>>>>  
>>>
>>>
>>> Really ?
>>>
>>> If this is what you meant, this deserves a big and fat comment.
>>>
>>>
>>>
>> Hi Eric,
>>
>> Isn't it the proper way to do? According to Cadence documentation
>> about RX checksum offload:
>> "If any of the checksums (IP, TCP or UDP) are verified incorrect by
>> the GEM, the packet is discarded."
>>
>> If I understand, when RX checksum offload is enabled setting bit 24 of
>> the Network Configuration Register, the driver only receives RX frames
>> with correct checksum. Then it tells the kernel that there's no need
>> to verify the checksum again in software. This is done setting
>> skb->ip_summed to CHECKSUM_UNNECESSARY. Intel e1000 driver does the
>> same in e1000_rx_checksum() function.
>>
>> Also bit 24 of the Network Configuration Register is updated by
>> macb_open() and macb_set_features() so it always matches the state of
>> NETIF_F_RXCSUM flag in dev->features, once the network interface is
>> up. That's why I'd rather read from dev->features than read from
>> register.
>>
>> Did I make a mistake? Is it the kind of comment you expect to be
>> added?
>>
>> Regards,
> 
> Usually, frames are not discarded, and a bit is present in rx descriptor
> to tell us if (TCP) checksum was validated by the NIC
> 
> We want tcpdump to get a copy of corrupted TCP frames, even if TCP stack
> drops them later.
> 
> If this NIC drops incorrect frames, I am afraid we cant use this RX
> offload at all.
> 
> 
> 
OK thanks for your comments. In the next series of patches I'll allow RX 
checksum offload only when promiscuous mode is disabled. Also I'll check the
status returned by the GEM through the buffer descriptor.

Regards,

Cyrille