All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
@ 2015-11-10 16:14 Mans Rullgard
  2015-11-10 17:55 ` Eric Dumazet
                   ` (4 more replies)
  0 siblings, 5 replies; 52+ messages in thread
From: Mans Rullgard @ 2015-11-10 16:14 UTC (permalink / raw)
  To: linux-kernel, netdev; +Cc: slash.tmp

This adds a driver for the Aurora VLSI NB8800 Ethernet controller.
It is an almost complete rewrite of a driver originally found in
a Sigma Designs 2.6.22 tree.

Signed-off-by: Mans Rullgard <mans@mansr.com>
---
Changes:
- Refactored mdio access functions
- Refactored register access helpers
- Improved error handling in rx buffer allocation
- Optimised some fifo parameters
- Overhauled tx dma. Multiple packets are now chained in a single dma
  operation if xmit_more is set, improving performance.
- Improved rx irq handling. It's not possible to disable interrupts
  entirely for napi poll, but they can be slowed down a little.
- Use readx_poll_timeout in various places
- Improved error detection
- Improved statistics
- Report hardware statistics counters through ethtool
- Improved tangox-specific setup
- Support for flow control using pause frames
- Explanatory comments added
- Various minor stylistic changes
---
 drivers/net/ethernet/Kconfig         |    1 +
 drivers/net/ethernet/Makefile        |    1 +
 drivers/net/ethernet/aurora/Kconfig  |   20 +
 drivers/net/ethernet/aurora/Makefile |    1 +
 drivers/net/ethernet/aurora/nb8800.c | 1530 ++++++++++++++++++++++++++++++++++
 drivers/net/ethernet/aurora/nb8800.h |  314 +++++++
 6 files changed, 1867 insertions(+)
 create mode 100644 drivers/net/ethernet/aurora/Kconfig
 create mode 100644 drivers/net/ethernet/aurora/Makefile
 create mode 100644 drivers/net/ethernet/aurora/nb8800.c
 create mode 100644 drivers/net/ethernet/aurora/nb8800.h

diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
index 05aa759..8310163 100644
--- a/drivers/net/ethernet/Kconfig
+++ b/drivers/net/ethernet/Kconfig
@@ -29,6 +29,7 @@ source "drivers/net/ethernet/apm/Kconfig"
 source "drivers/net/ethernet/apple/Kconfig"
 source "drivers/net/ethernet/arc/Kconfig"
 source "drivers/net/ethernet/atheros/Kconfig"
+source "drivers/net/ethernet/aurora/Kconfig"
 source "drivers/net/ethernet/cadence/Kconfig"
 source "drivers/net/ethernet/adi/Kconfig"
 source "drivers/net/ethernet/broadcom/Kconfig"
diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
index ddfc808..b435fb0 100644
--- a/drivers/net/ethernet/Makefile
+++ b/drivers/net/ethernet/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_NET_XGENE) += apm/
 obj-$(CONFIG_NET_VENDOR_APPLE) += apple/
 obj-$(CONFIG_NET_VENDOR_ARC) += arc/
 obj-$(CONFIG_NET_VENDOR_ATHEROS) += atheros/
+obj-$(CONFIG_NET_VENDOR_AURORA) += aurora/
 obj-$(CONFIG_NET_CADENCE) += cadence/
 obj-$(CONFIG_NET_BFIN) += adi/
 obj-$(CONFIG_NET_VENDOR_BROADCOM) += broadcom/
diff --git a/drivers/net/ethernet/aurora/Kconfig b/drivers/net/ethernet/aurora/Kconfig
new file mode 100644
index 0000000..a3c7106
--- /dev/null
+++ b/drivers/net/ethernet/aurora/Kconfig
@@ -0,0 +1,20 @@
+config NET_VENDOR_AURORA
+	bool "Aurora VLSI devices"
+	help
+	  If you have a network (Ethernet) device belonging to this class,
+	  say Y.
+
+	  Note that the answer to this question doesn't directly affect the
+	  kernel: saying N will just cause the configurator to skip all
+	  questions about Aurora devices. If you say Y, you will be asked
+	  for your specific device in the following questions.
+
+if NET_VENDOR_AURORA
+
+config AURORA_NB8800
+	tristate "Aurora AU-NB8800 support"
+	select PHYLIB
+	help
+	 Support for the AU-NB8800 gigabit Ethernet controller.
+
+endif
diff --git a/drivers/net/ethernet/aurora/Makefile b/drivers/net/ethernet/aurora/Makefile
new file mode 100644
index 0000000..6cb528a
--- /dev/null
+++ b/drivers/net/ethernet/aurora/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_AURORA_NB8800) += nb8800.o
diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
new file mode 100644
index 0000000..11cd389
--- /dev/null
+++ b/drivers/net/ethernet/aurora/nb8800.c
@@ -0,0 +1,1530 @@
+/*
+ * Copyright (C) 2015 Mans Rullgard <mans@mansr.com>
+ *
+ * Mostly rewritten, based on driver from Sigma Designs.  Original
+ * copyright notice below.
+ *
+ *
+ * Driver for tangox SMP864x/SMP865x/SMP867x/SMP868x builtin Ethernet Mac.
+ *
+ * Copyright (C) 2005 Maxime Bizon <mbizon@freebox.fr>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/etherdevice.h>
+#include <linux/delay.h>
+#include <linux/ethtool.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/of_device.h>
+#include <linux/of_mdio.h>
+#include <linux/of_net.h>
+#include <linux/dma-mapping.h>
+#include <linux/phy.h>
+#include <linux/cache.h>
+#include <linux/jiffies.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <asm/barrier.h>
+
+#include "nb8800.h"
+
+static int nb8800_dma_stop(struct net_device *dev);
+
+static inline u8 nb8800_readb(struct nb8800_priv *priv, int reg)
+{
+	return readb(priv->base + reg);
+}
+
+static inline u32 nb8800_readl(struct nb8800_priv *priv, int reg)
+{
+	return readl(priv->base + reg);
+}
+
+static inline void nb8800_writeb(struct nb8800_priv *priv, int reg, u8 val)
+{
+	writeb(val, priv->base + reg);
+}
+
+static inline void nb8800_writew(struct nb8800_priv *priv, int reg, u16 val)
+{
+	writew(val, priv->base + reg);
+}
+
+static inline void nb8800_writel(struct nb8800_priv *priv, int reg, u32 val)
+{
+	writel(val, priv->base + reg);
+}
+
+static inline void nb8800_maskb(struct nb8800_priv *priv, int reg,
+				u32 mask, u32 val)
+{
+	u32 old = nb8800_readb(priv, reg);
+	u32 new = (old & ~mask) | val;
+	if (new != old)
+		nb8800_writeb(priv, reg, new);
+}
+
+static inline void nb8800_maskl(struct nb8800_priv *priv, int reg,
+				u32 mask, u32 val)
+{
+	u32 old = nb8800_readl(priv, reg);
+	u32 new = (old & ~mask) | val;
+	if (new != old)
+		nb8800_writel(priv, reg, new);
+}
+
+static inline void nb8800_modb(struct nb8800_priv *priv, int reg, u8 bits,
+			       bool set)
+{
+	nb8800_maskb(priv, reg, bits, set ? bits : 0);
+}
+
+static inline void nb8800_setb(struct nb8800_priv *priv, int reg, u8 bits)
+{
+	nb8800_maskb(priv, reg, bits, bits);
+}
+
+static inline void nb8800_clearb(struct nb8800_priv *priv, int reg, u8 bits)
+{
+	nb8800_maskb(priv, reg, bits, 0);
+}
+
+static inline void nb8800_modl(struct nb8800_priv *priv, int reg, u32 bits,
+			       bool set)
+{
+	nb8800_maskl(priv, reg, bits, set ? bits : 0);
+}
+
+static inline void nb8800_setl(struct nb8800_priv *priv, int reg, u32 bits)
+{
+	nb8800_maskl(priv, reg, bits, bits);
+}
+
+static inline void nb8800_clearl(struct nb8800_priv *priv, int reg, u32 bits)
+{
+	nb8800_maskl(priv, reg, bits, 0);
+}
+
+static int nb8800_mdio_wait(struct mii_bus *bus)
+{
+	struct nb8800_priv *priv = bus->priv;
+	u32 val;
+
+	return readl_poll_timeout_atomic(priv->base + NB8800_MDIO_CMD,
+					 val, !(val & MDIO_CMD_GO), 1, 1000);
+}
+
+static int nb8800_mdio_cmd(struct mii_bus *bus, u32 cmd)
+{
+	struct nb8800_priv *priv = bus->priv;
+	int err;
+
+	err = nb8800_mdio_wait(bus);
+	if (err)
+		return err;
+
+	nb8800_writel(priv, NB8800_MDIO_CMD, cmd);
+	udelay(10);
+	nb8800_writel(priv, NB8800_MDIO_CMD, cmd | MDIO_CMD_GO);
+
+	return nb8800_mdio_wait(bus);
+}
+
+static int nb8800_mdio_read(struct mii_bus *bus, int phy_id, int reg)
+{
+	struct nb8800_priv *priv = bus->priv;
+	u32 val;
+	int err;
+
+	err = nb8800_mdio_cmd(bus, MIIAR_ADDR(phy_id) | MIIAR_REG(reg));
+	if (err)
+		return err;
+
+	val = nb8800_readl(priv, NB8800_MDIO_STS);
+	if (val & MDIO_STS_ERR)
+		return 0xffff;
+
+	return val & 0xffff;
+}
+
+static int nb8800_mdio_write(struct mii_bus *bus, int phy_id, int reg, u16 val)
+{
+	u32 cmd = MIIAR_DATA(val) | MIIAR_ADDR(phy_id) | MIIAR_REG(reg) |
+		MDIO_CMD_WR;
+
+	return nb8800_mdio_cmd(bus, cmd);
+}
+
+static void nb8800_mac_tx(struct net_device *dev, bool enable)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+
+	while (nb8800_readl(priv, NB8800_TXC_CR) & TCR_EN)
+		cpu_relax();
+
+	nb8800_modb(priv, NB8800_TX_CTL1, TX_EN, enable);
+}
+
+static void nb8800_mac_rx(struct net_device *dev, bool enable)
+{
+	nb8800_modb(netdev_priv(dev), NB8800_RX_CTL, RX_EN, enable);
+}
+
+static void nb8800_mac_af(struct net_device *dev, bool enable)
+{
+	nb8800_modb(netdev_priv(dev), NB8800_RX_CTL, RX_AF_EN, enable);
+}
+
+static void nb8800_start_rx(struct net_device *dev)
+{
+	nb8800_setl(netdev_priv(dev), NB8800_RXC_CR, RCR_EN);
+}
+
+static int nb8800_alloc_rx(struct net_device *dev, int i, bool napi)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+	struct nb8800_rx_desc *rxd = &priv->rx_descs[i];
+	struct nb8800_rx_buf *rxb = &priv->rx_bufs[i];
+	int size = L1_CACHE_ALIGN(RX_BUF_SIZE);
+	dma_addr_t dma_addr;
+	struct page *page;
+	unsigned long offset;
+	void *data;
+
+	data = napi ? napi_alloc_frag(size) : netdev_alloc_frag(size);
+	if (!data)
+		return -ENOMEM;
+
+	page = virt_to_head_page(data);
+	offset = data - page_address(page);
+
+	dma_addr = dma_map_page(&dev->dev, page, offset, RX_BUF_SIZE,
+				DMA_FROM_DEVICE);
+
+	if (dma_mapping_error(&dev->dev, dma_addr)) {
+		skb_free_frag(data);
+		return -ENOMEM;
+	}
+
+	rxb->page = page;
+	rxb->offset = offset;
+	rxd->desc.s_addr = dma_addr;
+
+	return 0;
+}
+
+static void nb8800_receive(struct net_device *dev, int i, int len)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+	struct nb8800_rx_desc *rxd = &priv->rx_descs[i];
+	struct page *page = priv->rx_bufs[i].page;
+	int offset = priv->rx_bufs[i].offset;
+	void *data = page_address(page) + offset;
+	dma_addr_t dma = rxd->desc.s_addr;
+	struct sk_buff *skb;
+	int size;
+	int err;
+
+	size = len <= RX_COPYBREAK ? len : RX_COPYHDR;
+
+	skb = napi_alloc_skb(&priv->napi, size);
+	if (!skb) {
+		netdev_err(dev, "rx skb allocation failed\n");
+		dev->stats.rx_dropped++;
+		return;
+	}
+
+	if (len <= RX_COPYBREAK) {
+		dma_sync_single_for_cpu(&dev->dev, dma, len, DMA_FROM_DEVICE);
+		memcpy(skb_put(skb, len), data, len);
+		dma_sync_single_for_device(&dev->dev, dma, len,
+					   DMA_FROM_DEVICE);
+	} else {
+		err = nb8800_alloc_rx(dev, i, true);
+		if (err) {
+			netdev_err(dev, "rx buffer allocation failed\n");
+			dev->stats.rx_dropped++;
+			return;
+		}
+
+		dma_unmap_page(&dev->dev, dma, RX_BUF_SIZE, DMA_FROM_DEVICE);
+		memcpy(skb_put(skb, RX_COPYHDR), data, RX_COPYHDR);
+		skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page,
+				offset + RX_COPYHDR, len - RX_COPYHDR,
+				RX_BUF_SIZE);
+	}
+
+	skb->protocol = eth_type_trans(skb, dev);
+	netif_receive_skb(skb);
+}
+
+static void nb8800_rx_error(struct net_device *dev, u32 report)
+{
+	if (report & RX_LENGTH_ERR)
+		dev->stats.rx_length_errors++;
+
+	if (report & RX_FCS_ERR)
+		dev->stats.rx_crc_errors++;
+
+	if (report & RX_FIFO_OVERRUN)
+		dev->stats.rx_fifo_errors++;
+
+	if (report & RX_ALIGNMENT_ERROR)
+		dev->stats.rx_frame_errors++;
+
+	dev->stats.rx_errors++;
+}
+
+static int nb8800_poll(struct napi_struct *napi, int budget)
+{
+	struct net_device *dev = napi->dev;
+	struct nb8800_priv *priv = netdev_priv(dev);
+	struct nb8800_rx_desc *rxd;
+	int work = 0;
+	int last = priv->rx_eoc;
+	int next;
+
+again:
+	while (work < budget) {
+		struct nb8800_rx_buf *rxb;
+		int len;
+
+		next = (last + 1) % RX_DESC_COUNT;
+
+		rxb = &priv->rx_bufs[next];
+		rxd = &priv->rx_descs[next];
+
+		if (!rxd->report)
+			break;
+
+		len = RX_BYTES_TRANSFERRED(rxd->report);
+
+		if (IS_RX_ERROR(rxd->report))
+			nb8800_rx_error(dev, rxd->report);
+		else
+			nb8800_receive(dev, next, len);
+
+		dev->stats.rx_packets++;
+		dev->stats.rx_bytes += len;
+
+		if (rxd->report & RX_MULTICAST_PKT)
+			dev->stats.multicast++;
+
+		rxd->report = 0;
+		last = next;
+		work++;
+	}
+
+	if (work) {
+		priv->rx_descs[last].desc.config |= DESC_EOC;
+		wmb();	/* ensure new EOC is written before clearing old */
+		priv->rx_descs[priv->rx_eoc].desc.config &= ~DESC_EOC;
+		priv->rx_eoc = last;
+		nb8800_start_rx(dev);
+	}
+
+	if (work < budget) {
+		nb8800_writel(priv, NB8800_RX_ITR, priv->rx_itr_irq);
+
+		/* If a packet arrived after we last checked but
+		 * before writing RX_ITR, the interrupt will be
+		 * delayed, so we retrieve it now. */
+		if (priv->rx_descs[next].report)
+			goto again;
+
+		napi_complete_done(napi, work);
+	}
+
+	return work;
+}
+
+static void nb8800_tx_dma_start(struct net_device *dev)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+	struct nb8800_tx_buf *txb;
+	u32 txc_cr;
+
+	txb = &priv->tx_bufs[priv->tx_queue];
+	if (!txb->ready)
+		return;
+
+	txc_cr = nb8800_readl(priv, NB8800_TXC_CR);
+	if (txc_cr & TCR_EN)
+		return;
+
+	nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
+	wmb();		/* ensure desc addr is written before starting DMA */
+	nb8800_writel(priv, NB8800_TXC_CR, txc_cr | TCR_EN);
+
+	priv->tx_queue = (priv->tx_queue + txb->chain_len) % TX_DESC_COUNT;
+}
+
+static void nb8800_tx_dma_start_irq(struct net_device *dev)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+
+	spin_lock(&priv->tx_lock);
+	nb8800_tx_dma_start(dev);
+	spin_unlock(&priv->tx_lock);
+}
+
+static int nb8800_xmit(struct sk_buff *skb, struct net_device *dev)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+	struct nb8800_tx_desc *txd;
+	struct nb8800_tx_buf *txb;
+	struct nb8800_dma_desc *dma;
+	dma_addr_t dma_addr;
+	unsigned int dma_len;
+	unsigned long flags;
+	int align;
+	int next;
+
+	if (atomic_read(&priv->tx_free) <= NB8800_DESC_LOW) {
+		netif_stop_queue(dev);
+		return NETDEV_TX_BUSY;
+	}
+
+	align = (8 - (uintptr_t)skb->data) & 7;
+
+	dma_len = skb->len - align;
+	dma_addr = dma_map_single(&dev->dev, skb->data + align,
+				  dma_len, DMA_TO_DEVICE);
+
+	if (dma_mapping_error(&dev->dev, dma_addr)) {
+		netdev_err(dev, "tx dma mapping error\n");
+		kfree_skb(skb);
+		dev->stats.tx_dropped++;
+		return NETDEV_TX_OK;
+	}
+
+	if (atomic_dec_return(&priv->tx_free) <= NB8800_DESC_LOW)
+		netif_stop_queue(dev);
+
+	next = priv->tx_next;
+	txb = &priv->tx_bufs[next];
+	txd = &priv->tx_descs[next];
+	dma = &txd->desc[0];
+
+	next = (next + 1) % TX_DESC_COUNT;
+
+	if (align) {
+		memcpy(txd->buf, skb->data, align);
+
+		dma->s_addr =
+			txb->dma_desc + offsetof(struct nb8800_tx_desc, buf);
+		dma->n_addr = txb->dma_desc + sizeof(txd->desc[0]);
+		dma->config = DESC_BTS(2) | DESC_DS | align;
+
+		dma++;
+	}
+
+	dma->s_addr = dma_addr;
+	dma->n_addr = priv->tx_bufs[next].dma_desc;
+	dma->config = DESC_BTS(2) | DESC_DS | DESC_EOF | dma_len;
+
+	if (!skb->xmit_more)
+		dma->config |= DESC_EOC;
+
+	txb->skb = skb;
+	txb->dma_addr = dma_addr;
+	txb->dma_len = dma_len;
+
+	if (!priv->tx_chain) {
+		txb->chain_len = 1;
+		priv->tx_chain = txb;
+	} else {
+		priv->tx_chain->chain_len++;
+	}
+
+	netdev_sent_queue(dev, skb->len);
+
+	smp_mb__before_spinlock();
+	spin_lock_irqsave(&priv->tx_lock, flags);
+
+	if (!skb->xmit_more) {
+		priv->tx_chain->ready = true;
+		priv->tx_chain = NULL;
+		nb8800_tx_dma_start(dev);
+	}
+
+	spin_unlock_irqrestore(&priv->tx_lock, flags);
+
+	priv->tx_next = next;
+
+	return NETDEV_TX_OK;
+}
+
+static void nb8800_tx_error(struct net_device *dev, u32 report)
+{
+	if (report & TX_LATE_COLLISION)
+		dev->stats.collisions++;
+
+	if (report & TX_PACKET_DROPPED)
+		dev->stats.tx_dropped++;
+
+	if (report & TX_FIFO_UNDERRUN)
+		dev->stats.tx_fifo_errors++;
+
+	dev->stats.tx_errors++;
+}
+
+static void nb8800_tx_done(struct net_device *dev)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+	int limit = priv->tx_next;
+	int done = priv->tx_done;
+	unsigned int packets = 0;
+	unsigned int len = 0;
+
+	while (done != limit) {
+		struct nb8800_tx_desc *txd = &priv->tx_descs[done];
+		struct nb8800_tx_buf *txb = &priv->tx_bufs[done];
+		struct sk_buff *skb;
+
+		if (!txd->report)
+			break;
+
+		skb = txb->skb;
+		len += skb->len;
+
+		dma_unmap_single(&dev->dev, txb->dma_addr, txb->dma_len,
+				 DMA_TO_DEVICE);
+
+		if (IS_TX_ERROR(txd->report)) {
+			nb8800_tx_error(dev, txd->report);
+			dev_kfree_skb_irq(skb);
+		} else {
+			dev_consume_skb_irq(skb);
+		}
+
+		dev->stats.tx_packets++;
+		dev->stats.tx_bytes += TX_BYTES_TRANSFERRED(txd->report);
+		dev->stats.collisions += TX_EARLY_COLLISIONS(txd->report);
+
+		txb->skb = NULL;
+		txb->ready = false;
+		txd->report = 0;
+
+		done = (done + 1) % TX_DESC_COUNT;
+		packets++;
+	}
+
+	if (packets) {
+		smp_mb__before_atomic();
+		atomic_add(packets, &priv->tx_free);
+		netdev_completed_queue(dev, packets, len);
+		netif_wake_queue(dev);
+		priv->tx_done = done;
+	}
+}
+
+static irqreturn_t nb8800_irq(int irq, void *dev_id)
+{
+	struct net_device *dev = dev_id;
+	struct nb8800_priv *priv = netdev_priv(dev);
+	u32 val;
+
+	/* tx interrupt */
+	val = nb8800_readl(priv, NB8800_TXC_SR);
+	if (val) {
+		nb8800_writel(priv, NB8800_TXC_SR, val);
+
+		if (val & TSR_DI)
+			nb8800_tx_dma_start_irq(dev);
+
+		if (val & TSR_TI)
+			nb8800_tx_done(dev);
+
+		if (unlikely(val & TSR_DE))
+			netdev_err(dev, "TX DMA error\n");
+
+		/* should never happen with automatic status retrieval */
+		if (unlikely(val & TSR_TO))
+			netdev_err(dev, "TX Status FIFO overflow\n");
+	}
+
+	/* rx interrupt */
+	val = nb8800_readl(priv, NB8800_RXC_SR);
+	if (val) {
+		nb8800_writel(priv, NB8800_RXC_SR, val);
+
+		if (likely(val & (RSR_RI | RSR_DI))) {
+			nb8800_writel(priv, NB8800_RX_ITR, priv->rx_itr_poll);
+			napi_schedule_irqoff(&priv->napi);
+		}
+
+		if (unlikely(val & RSR_DE))
+			netdev_err(dev, "RX DMA error\n");
+
+		/* should never happen with automatic status retrieval */
+		if (unlikely(val & RSR_RO))
+			netdev_err(dev, "RX Status FIFO overflow\n");
+	}
+
+	return IRQ_HANDLED;
+}
+
+static void nb8800_mac_config(struct net_device *dev)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+	bool gigabit = priv->speed == SPEED_1000;
+	u32 mac_mode_mask = RGMII_MODE | HALF_DUPLEX | GMAC_MODE;
+	u32 mac_mode = 0;
+	u32 slot_time;
+	u32 phy_clk;
+	u32 ict;
+
+	if (!priv->duplex)
+		mac_mode |= HALF_DUPLEX;
+
+	if (gigabit) {
+		if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
+			mac_mode |= RGMII_MODE;
+
+		mac_mode |= GMAC_MODE;
+		phy_clk = 125000000;
+
+		/* Should be 512 but register is only 8 bits */
+		slot_time = 255;
+	} else {
+		phy_clk = 25000000;
+		slot_time = 128;
+	}
+
+	ict = DIV_ROUND_UP(phy_clk, clk_get_rate(priv->clk));
+
+	nb8800_writeb(priv, NB8800_IC_THRESHOLD, ict);
+	nb8800_writeb(priv, NB8800_SLOT_TIME, slot_time);
+	nb8800_maskb(priv, NB8800_MAC_MODE, mac_mode_mask, mac_mode);
+}
+
+static void nb8800_pause_config(struct net_device *dev)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+	struct phy_device *phydev = priv->phydev;
+	u32 rxcr;
+
+	if (priv->pause_aneg) {
+		if (!phydev || !phydev->link)
+			return;
+
+		priv->pause_rx = phydev->pause;
+		priv->pause_tx = phydev->pause ^ phydev->asym_pause;
+	}
+
+	nb8800_modb(priv, NB8800_RX_CTL, RX_PAUSE_EN, priv->pause_rx);
+
+	rxcr = nb8800_readl(priv, NB8800_RXC_CR);
+	if (!!(rxcr & RCR_FL) == priv->pause_tx)
+		return;
+
+	if (netif_running(dev)) {
+		napi_disable(&priv->napi);
+		netif_tx_lock_bh(dev);
+		nb8800_dma_stop(dev);
+		nb8800_modl(priv, NB8800_RXC_CR, RCR_FL, priv->pause_tx);
+		nb8800_start_rx(dev);
+		netif_tx_unlock_bh(dev);
+		napi_enable(&priv->napi);
+	} else {
+		nb8800_modl(priv, NB8800_RXC_CR, RCR_FL, priv->pause_tx);
+	}
+}
+
+static void nb8800_link_reconfigure(struct net_device *dev)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+	struct phy_device *phydev = priv->phydev;
+	int change = 0;
+
+	if (phydev->link) {
+		if (phydev->speed != priv->speed) {
+			priv->speed = phydev->speed;
+			change = 1;
+		}
+
+		if (phydev->duplex != priv->duplex) {
+			priv->duplex = phydev->duplex;
+			change = 1;
+		}
+
+		if (change)
+			nb8800_mac_config(dev);
+
+		nb8800_pause_config(dev);
+	}
+
+	if (phydev->link != priv->link) {
+		priv->link = phydev->link;
+		change = 1;
+	}
+
+	if (change)
+		phy_print_status(priv->phydev);
+}
+
+static void nb8800_update_mac_addr(struct net_device *dev)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+	int i;
+
+	for (i = 0; i < ETH_ALEN; i++)
+		nb8800_writeb(priv, NB8800_SRC_ADDR(i), dev->dev_addr[i]);
+
+	for (i = 0; i < ETH_ALEN; i++)
+		nb8800_writeb(priv, NB8800_UC_ADDR(i), dev->dev_addr[i]);
+}
+
+static int nb8800_set_mac_address(struct net_device *dev, void *addr)
+{
+	struct sockaddr *sock = addr;
+
+	if (netif_running(dev))
+		return -EBUSY;
+
+	ether_addr_copy(dev->dev_addr, sock->sa_data);
+	nb8800_update_mac_addr(dev);
+
+	return 0;
+}
+
+static void nb8800_mc_init(struct net_device *dev, int val)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+
+	nb8800_writeb(priv, NB8800_MC_INIT, val);
+	readb_poll_timeout_atomic(priv->base + NB8800_MC_INIT, val, !val,
+				  1, 1000);
+}
+
+static void nb8800_set_rx_mode(struct net_device *dev)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+	struct netdev_hw_addr *ha;
+	int i;
+
+	if (dev->flags & (IFF_PROMISC | IFF_ALLMULTI)) {
+		nb8800_mac_af(dev, false);
+		return;
+	}
+
+	nb8800_mac_af(dev, true);
+	nb8800_mc_init(dev, 0);
+
+	netdev_for_each_mc_addr(ha, dev) {
+		for (i = 0; i < ETH_ALEN; i++)
+			nb8800_writeb(priv, NB8800_MC_ADDR(i), ha->addr[i]);
+
+		nb8800_mc_init(dev, 0xff);
+	}
+}
+
+#define RX_DESC_SIZE (RX_DESC_COUNT * sizeof(struct nb8800_rx_desc))
+#define TX_DESC_SIZE (TX_DESC_COUNT * sizeof(struct nb8800_tx_desc))
+
+static void nb8800_dma_free(struct net_device *dev)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+	int i;
+
+	if (priv->rx_bufs) {
+		for (i = 0; i < RX_DESC_COUNT; i++)
+			if (priv->rx_bufs[i].page)
+				put_page(priv->rx_bufs[i].page);
+
+		kfree(priv->rx_bufs);
+		priv->rx_bufs = NULL;
+	}
+
+	if (priv->tx_bufs) {
+		for (i = 0; i < TX_DESC_COUNT; i++)
+			kfree_skb(priv->tx_bufs[i].skb);
+
+		kfree(priv->tx_bufs);
+		priv->tx_bufs = NULL;
+	}
+
+	if (priv->rx_descs) {
+		dma_free_coherent(dev->dev.parent, RX_DESC_SIZE, priv->rx_descs,
+				  priv->rx_desc_dma);
+		priv->rx_descs = NULL;
+	}
+
+	if (priv->tx_descs) {
+		dma_free_coherent(dev->dev.parent, TX_DESC_SIZE, priv->tx_descs,
+				  priv->tx_desc_dma);
+		priv->tx_descs = NULL;
+	}
+}
+
+static void nb8800_dma_reset(struct net_device *dev)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+	struct nb8800_rx_desc *rxd;
+	struct nb8800_tx_desc *txd;
+	int i;
+
+	for (i = 0; i < RX_DESC_COUNT; i++) {
+		dma_addr_t rx_dma = priv->rx_desc_dma + i * sizeof(*rxd);
+
+		rxd = &priv->rx_descs[i];
+		rxd->desc.n_addr = rx_dma + sizeof(*rxd);
+		rxd->desc.r_addr =
+			rx_dma + offsetof(struct nb8800_rx_desc, report);
+		rxd->desc.config = priv->rx_dma_config;
+		rxd->report = 0;
+	}
+
+	rxd->desc.n_addr = priv->rx_desc_dma;
+	rxd->desc.config |= DESC_EOC;
+
+	priv->rx_eoc = RX_DESC_COUNT - 1;
+
+	for (i = 0; i < TX_DESC_COUNT; i++) {
+		struct nb8800_tx_buf *txb = &priv->tx_bufs[i];
+		dma_addr_t r_dma = txb->dma_desc +
+			offsetof(struct nb8800_tx_desc, report);
+
+		txd = &priv->tx_descs[i];
+		txd->desc[0].r_addr = r_dma;
+		txd->desc[1].r_addr = r_dma;
+		txd->report = 0;
+	}
+
+	priv->tx_next = 0;
+	priv->tx_queue = 0;
+	priv->tx_done = 0;
+	atomic_set(&priv->tx_free, TX_DESC_COUNT);
+
+	nb8800_writel(priv, NB8800_RX_DESC_ADDR, priv->rx_desc_dma);
+
+	wmb();		/* ensure all setup is written before starting */
+}
+
+static int nb8800_dma_init(struct net_device *dev)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+	int n_rx = RX_DESC_COUNT;
+	int n_tx = TX_DESC_COUNT;
+	int err;
+	int i;
+
+	priv->rx_descs = dma_alloc_coherent(dev->dev.parent, RX_DESC_SIZE,
+					    &priv->rx_desc_dma, GFP_KERNEL);
+	if (!priv->rx_descs)
+		goto err_out;
+
+	priv->rx_bufs = kcalloc(n_rx, sizeof(*priv->rx_bufs), GFP_KERNEL);
+	if (!priv->rx_bufs)
+		goto err_out;
+
+	for (i = 0; i < n_rx; i++) {
+		err = nb8800_alloc_rx(dev, i, false);
+		if (err)
+			goto err_out;
+	}
+
+	priv->tx_descs = dma_alloc_coherent(dev->dev.parent, TX_DESC_SIZE,
+					    &priv->tx_desc_dma, GFP_KERNEL);
+	if (!priv->tx_descs)
+		goto err_out;
+
+	priv->tx_bufs = kcalloc(n_tx, sizeof(*priv->tx_bufs), GFP_KERNEL);
+	if (!priv->tx_bufs)
+		goto err_out;
+
+	for (i = 0; i < n_tx; i++)
+		priv->tx_bufs[i].dma_desc =
+			priv->tx_desc_dma + i * sizeof(struct nb8800_tx_desc);
+
+	nb8800_dma_reset(dev);
+
+	return 0;
+
+err_out:
+	nb8800_dma_free(dev);
+
+	return -ENOMEM;
+}
+
+static int nb8800_dma_stop(struct net_device *dev)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+	struct nb8800_tx_buf *txb = &priv->tx_bufs[0];
+	struct nb8800_tx_desc *txd = &priv->tx_descs[0];
+	int retry = 5;
+	u32 txcr;
+	u32 rxcr;
+	int err;
+	int i;
+
+	/* wait for tx to finish */
+	err = readl_poll_timeout_atomic(priv->base + NB8800_TXC_CR, txcr,
+					!(txcr & TCR_EN) &&
+					priv->tx_done == priv->tx_next,
+					1000, 1000000);
+	if (err)
+		return err;
+
+	/* The rx DMA only stops if it reaches the end of chain.
+	 * To make this happen, we set the EOC flag on all rx
+	 * descriptors, put the device in loopback mode, and send
+	 * a few dummy frames.  The interrupt handler will ignore
+	 * these since NAPI is disabled and no real frames are in
+	 * the tx queue. */
+
+	for (i = 0; i < RX_DESC_COUNT; i++)
+		priv->rx_descs[i].desc.config |= DESC_EOC;
+
+	txd->desc[0].s_addr =
+		txb->dma_desc + offsetof(struct nb8800_tx_desc, buf);
+	txd->desc[0].config = DESC_BTS(2) | DESC_DS | DESC_EOF | DESC_EOC | 8;
+	memset(txd->buf, 0, sizeof(txd->buf));
+
+	nb8800_mac_af(dev, false);
+	nb8800_setb(priv, NB8800_MAC_MODE, LOOPBACK_EN);
+
+	do {
+		nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
+		wmb();
+		nb8800_writel(priv, NB8800_TXC_CR, txcr | TCR_EN);
+
+		err = readl_poll_timeout_atomic(priv->base + NB8800_RXC_CR,
+						rxcr, !(rxcr & RCR_EN),
+						1000, 100000);
+	} while (err && --retry);
+
+	nb8800_mac_af(dev, true);
+	nb8800_clearb(priv, NB8800_MAC_MODE, LOOPBACK_EN);
+	nb8800_dma_reset(dev);
+
+	return retry ? 0 : -ETIMEDOUT;
+}
+
+static void nb8800_pause_adv(struct net_device *dev)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+	u32 adv = 0;
+
+	if (!priv->phydev)
+		return;
+
+	if (priv->pause_rx)
+		adv |= ADVERTISED_Pause | ADVERTISED_Asym_Pause;
+	if (priv->pause_tx)
+		adv ^= ADVERTISED_Asym_Pause;
+
+	priv->phydev->supported |= adv;
+	priv->phydev->advertising |= adv;
+
+}
+
+static int nb8800_open(struct net_device *dev)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+	int err;
+
+	/* clear any pending interrupts */
+	nb8800_writel(priv, NB8800_RXC_SR, 0xf);
+	nb8800_writel(priv, NB8800_TXC_SR, 0xf);
+
+	err = nb8800_dma_init(dev);
+	if (err)
+		return err;
+
+	err = request_irq(dev->irq, nb8800_irq, 0, dev_name(&dev->dev), dev);
+	if (err)
+		goto err_free_dma;
+
+	nb8800_mac_rx(dev, true);
+	nb8800_mac_tx(dev, true);
+
+	priv->phydev = of_phy_connect(dev, priv->phy_node,
+				      nb8800_link_reconfigure, 0,
+				      priv->phy_mode);
+	if (!priv->phydev)
+		goto err_free_irq;
+
+	nb8800_pause_adv(dev);
+
+	netdev_reset_queue(dev);
+	napi_enable(&priv->napi);
+	netif_start_queue(dev);
+
+	nb8800_start_rx(dev);
+	phy_start(priv->phydev);
+
+	return 0;
+
+err_free_irq:
+	free_irq(dev->irq, dev);
+err_free_dma:
+	nb8800_dma_free(dev);
+
+	return err;
+}
+
+static int nb8800_stop(struct net_device *dev)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+
+	phy_stop(priv->phydev);
+
+	netif_stop_queue(dev);
+	napi_disable(&priv->napi);
+
+	nb8800_dma_stop(dev);
+	nb8800_mac_rx(dev, false);
+	nb8800_mac_tx(dev, false);
+
+	phy_disconnect(priv->phydev);
+	priv->phydev = NULL;
+
+	free_irq(dev->irq, dev);
+
+	nb8800_dma_free(dev);
+
+	return 0;
+}
+
+static int nb8800_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+
+	return phy_mii_ioctl(priv->phydev, rq, cmd);
+}
+
+static const struct net_device_ops nb8800_netdev_ops = {
+	.ndo_open		= nb8800_open,
+	.ndo_stop		= nb8800_stop,
+	.ndo_start_xmit		= nb8800_xmit,
+	.ndo_set_mac_address	= nb8800_set_mac_address,
+	.ndo_set_rx_mode	= nb8800_set_rx_mode,
+	.ndo_do_ioctl		= nb8800_ioctl,
+	.ndo_change_mtu		= eth_change_mtu,
+	.ndo_validate_addr	= eth_validate_addr,
+};
+
+static int nb8800_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+
+	if (!priv->phydev)
+		return -ENODEV;
+
+	return phy_ethtool_gset(priv->phydev, cmd);
+}
+
+static int nb8800_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+
+	if (!priv->phydev)
+		return -ENODEV;
+
+	return phy_ethtool_sset(priv->phydev, cmd);
+}
+
+static int nb8800_nway_reset(struct net_device *dev)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+
+	if (!priv->phydev)
+		return -ENODEV;
+
+	return genphy_restart_aneg(priv->phydev);
+}
+
+static void nb8800_get_pauseparam(struct net_device *dev,
+				  struct ethtool_pauseparam *pp)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+
+	pp->autoneg = priv->pause_aneg;
+	pp->rx_pause = priv->pause_rx;
+	pp->tx_pause = priv->pause_tx;
+}
+
+static int nb8800_set_pauseparam(struct net_device *dev,
+				 struct ethtool_pauseparam *pp)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+
+	priv->pause_aneg = pp->autoneg;
+	priv->pause_rx = pp->rx_pause;
+	priv->pause_tx = pp->tx_pause;
+
+	nb8800_pause_adv(dev);
+
+	if (!priv->pause_aneg)
+		nb8800_pause_config(dev);
+	else if (priv->phydev)
+		phy_start_aneg(priv->phydev);
+
+	return 0;
+}
+
+static const char nb8800_stats_names[][ETH_GSTRING_LEN] = {
+	"rx_bytes_ok",
+	"rx_frames_ok",
+	"rx_undersize_frames",
+	"rx_fragment_frames",
+	"rx_64_byte_frames",
+	"rx_127_byte_frames",
+	"rx_255_byte_frames",
+	"rx_511_byte_frames",
+	"rx_1023_byte_frames",
+	"rx_max_size_frames",
+	"rx_oversize_frames",
+	"rx_bad_fcs_frames",
+	"rx_broadcast_frames",
+	"rx_multicast_frames",
+	"rx_control_frames",
+	"rx_pause_frames",
+	"rx_unsup_control_frames",
+	"rx_align_error_frames",
+	"rx_overrun_frames",
+	"rx_jabber_frames",
+	"rx_bytes",
+	"rx_frames",
+
+	"tx_bytes_ok",
+	"tx_frames_ok",
+	"tx_64_byte_frames",
+	"tx_127_byte_frames",
+	"tx_255_byte_frames",
+	"tx_511_byte_frames",
+	"tx_1023_byte_frames",
+	"tx_max_size_frames",
+	"tx_oversize_frames",
+	"tx_broadcast_frames",
+	"tx_multicast_frames",
+	"tx_control_frames",
+	"tx_pause_frames",
+	"tx_underrun_frames",
+	"tx_single_collision_frames",
+	"tx_multi_collision_frames",
+	"tx_deferred_collision_frames",
+	"tx_late_collision_frames",
+	"tx_excessive_collision_frames",
+	"tx_bytes",
+	"tx_frames",
+	"tx_collisions",
+};
+
+#define NB8800_NUM_STATS ARRAY_SIZE(nb8800_stats_names)
+
+static int nb8800_get_sset_count(struct net_device *dev, int sset)
+{
+	if (sset == ETH_SS_STATS)
+		return NB8800_NUM_STATS;
+
+	return -EOPNOTSUPP;
+}
+
+static void nb8800_get_strings(struct net_device *dev, u32 sset, u8 *buf)
+{
+	if (sset == ETH_SS_STATS)
+		memcpy(buf, &nb8800_stats_names, sizeof(nb8800_stats_names));
+}
+
+static u32 nb8800_read_stat(struct net_device *dev, int index)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+
+	nb8800_writeb(priv, NB8800_STAT_INDEX, index);
+
+	return nb8800_readl(priv, NB8800_STAT_DATA);
+}
+
+static void nb8800_get_ethtool_stats(struct net_device *dev,
+				     struct ethtool_stats *estats, u64 *st)
+{
+	u32 rx, tx;
+	int i;
+
+	for (i = 0; i < NB8800_NUM_STATS / 2; i++) {
+		rx = nb8800_read_stat(dev, i);
+		tx = nb8800_read_stat(dev, i | 0x80);
+		st[i] = rx;
+		st[i + NB8800_NUM_STATS / 2] = tx;
+	}
+}
+
+static const struct ethtool_ops nb8800_ethtool_ops = {
+	.get_settings		= nb8800_get_settings,
+	.set_settings		= nb8800_set_settings,
+	.nway_reset		= nb8800_nway_reset,
+	.get_link		= ethtool_op_get_link,
+	.get_pauseparam		= nb8800_get_pauseparam,
+	.set_pauseparam		= nb8800_set_pauseparam,
+	.get_sset_count		= nb8800_get_sset_count,
+	.get_strings		= nb8800_get_strings,
+	.get_ethtool_stats	= nb8800_get_ethtool_stats,
+};
+
+static int nb8800_hw_init(struct net_device *dev)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+	u32 val;
+
+	val = TX_RETRY_EN | TX_PAD_EN | TX_APPEND_FCS;
+	nb8800_writeb(priv, NB8800_TX_CTL1, val);
+
+	/* Collision retry count */
+	nb8800_writeb(priv, NB8800_TX_CTL2, 5);
+
+	val = RX_PAD_STRIP | RX_AF_EN;
+	nb8800_writeb(priv, NB8800_RX_CTL, val);
+
+	/* Chosen by fair dice roll */
+	nb8800_writeb(priv, NB8800_RANDOM_SEED, 4);
+
+	/* TX cycles per deferral period */
+	nb8800_writeb(priv, NB8800_TX_SDP, 12);
+
+	/* The following three threshold values have been
+	 * experimentally determined for good results. */
+
+	/* RX/TX FIFO threshold for partial empty (64-bit entries) */
+	nb8800_writeb(priv, NB8800_PE_THRESHOLD, 0);
+
+	/* RX/TX FIFO threshold for partial full (64-bit entries) */
+	nb8800_writeb(priv, NB8800_PF_THRESHOLD, 255);
+
+	/* Buffer size for transmit (64-bit entries) */
+	nb8800_writeb(priv, NB8800_TX_BUFSIZE, 64);
+
+	/* Configure tx DMA */
+
+	val = nb8800_readl(priv, NB8800_TXC_CR);
+	val &= TCR_LE;		/* keep endian setting */
+	val |= TCR_DM;		/* DMA descriptor mode */
+	val |= TCR_RS;		/* automatically store tx status  */
+	val |= TCR_DIE;		/* interrupt on DMA chain completion */
+	val |= TCR_TFI(7);	/* interrupt after 7 frames transmitted */
+	val |= TCR_BTS(2);	/* 32-byte bus transaction size */
+	nb8800_writel(priv, NB8800_TXC_CR, val);
+
+	/* TX complete interrupt after 10 ms or 7 frames (see above) */
+	val = clk_get_rate(priv->clk) / 100;
+	nb8800_writel(priv, NB8800_TX_ITR, val);
+
+	/* Configure rx DMA */
+
+	val = nb8800_readl(priv, NB8800_RXC_CR);
+	val &= RCR_LE;		/* keep endian setting */
+	val |= RCR_DM;		/* DMA descriptor mode */
+	val |= RCR_RS;		/* automatically store rx status */
+	val |= RCR_DIE;		/* interrupt at end of DMA chain */
+	val |= RCR_RFI(7);	/* interrupt after 7 frames received */
+	val |= RCR_BTS(2);	/* 32-byte bus transaction size */
+	nb8800_writel(priv, NB8800_RXC_CR, val);
+
+	/* The rx interrupt can fire before the DMA has completed
+	 * unless a small delay is added.  50 us is hopefully enough. */
+	priv->rx_itr_irq = clk_get_rate(priv->clk) / 20000;
+
+	/* In NAPI poll mode we want to disable interrupts, but the
+	 * hardware does not permit this.  Delay 10 ms instead. */
+	priv->rx_itr_poll = clk_get_rate(priv->clk) / 100;
+
+	nb8800_writel(priv, NB8800_RX_ITR, priv->rx_itr_irq);
+
+	priv->rx_dma_config = RX_BUF_SIZE | DESC_BTS(2) | DESC_DS | DESC_EOF;
+
+	/* Flow control settings */
+
+	/* Pause time of 0.1 ms */
+	val = 100000 / 512;
+	nb8800_writeb(priv, NB8800_PQ1, val >> 8);
+	nb8800_writeb(priv, NB8800_PQ2, val & 0xff);
+
+	/* Auto-negotiate by default */
+	priv->pause_aneg = true;
+	priv->pause_rx = true;
+	priv->pause_tx = true;
+
+	nb8800_mc_init(dev, 0);
+
+	return 0;
+}
+
+static int nb8800_tangox_init(struct net_device *dev)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+	u32 pad_mode = PAD_MODE_MII;
+
+	switch (priv->phy_mode) {
+	case PHY_INTERFACE_MODE_MII:
+	case PHY_INTERFACE_MODE_GMII:
+		pad_mode = PAD_MODE_MII;
+		break;
+
+	case PHY_INTERFACE_MODE_RGMII:
+		pad_mode = PAD_MODE_RGMII;
+		break;
+
+	case PHY_INTERFACE_MODE_RGMII_TXID:
+		pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
+		break;
+
+	default:
+		dev_err(dev->dev.parent, "unsupported phy mode %s\n",
+			phy_modes(priv->phy_mode));
+		return -EINVAL;
+	}
+
+	nb8800_writeb(priv, NB8800_TANGOX_PAD_MODE, pad_mode);
+
+	return 0;
+}
+
+static int nb8800_tangox_reset(struct net_device *dev)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+	int clk_div;
+
+	nb8800_writeb(priv, NB8800_TANGOX_RESET, 0);
+	usleep_range(1000, 10000);
+	nb8800_writeb(priv, NB8800_TANGOX_RESET, 1);
+
+	wmb();		/* ensure reset is cleared before proceeding */
+
+	clk_div = DIV_ROUND_UP(clk_get_rate(priv->clk), 2 * MAX_MDC_CLOCK);
+	nb8800_writew(priv, NB8800_TANGOX_MDIO_CLKDIV, clk_div);
+
+	return 0;
+}
+
+static const struct nb8800_ops nb8800_tangox_ops = {
+	.init	= nb8800_tangox_init,
+	.reset	= nb8800_tangox_reset,
+};
+
+static int nb8800_tango4_init(struct net_device *dev)
+{
+	struct nb8800_priv *priv = netdev_priv(dev);
+	int err;
+
+	err = nb8800_tangox_init(dev);
+	if (err)
+		return err;
+
+	/* On tango4 interrupt on DMA completion per frame works and gives
+	 * better performance despite generating more rx interrupts. */
+
+	/* Disable unnecessary interrupt on rx completion */
+	nb8800_clearl(priv, NB8800_RXC_CR, RCR_RFI(7));
+
+	/* Request interrupt on descriptor DMA completion */
+	priv->rx_dma_config |= DESC_ID;
+
+	return 0;
+}
+
+static const struct nb8800_ops nb8800_tango4_ops = {
+	.init	= nb8800_tango4_init,
+	.reset	= nb8800_tangox_reset,
+};
+
+static const struct of_device_id nb8800_dt_ids[] = {
+	{
+		.compatible = "aurora,nb8800",
+	},
+	{
+		.compatible = "sigma,smp8642-ethernet",
+		.data = &nb8800_tangox_ops,
+	},
+	{
+		.compatible = "sigma,smp8734-ethernet",
+		.data = &nb8800_tango4_ops,
+	},
+	{ }
+};
+
+static int nb8800_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+	const struct nb8800_ops *ops = NULL;
+	struct nb8800_priv *priv;
+	struct resource *res;
+	struct net_device *dev;
+	struct mii_bus *bus;
+	const unsigned char *mac;
+	void __iomem *base;
+	int irq;
+	int ret;
+
+	match = of_match_device(nb8800_dt_ids, &pdev->dev);
+	if (match)
+		ops = match->data;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq <= 0) {
+		dev_err(&pdev->dev, "No IRQ\n");
+		return -EINVAL;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	dev_dbg(&pdev->dev, "AU-NB8800 Ethernet at %pa\n", &res->start);
+
+	dev = alloc_etherdev(sizeof(*priv));
+	if (!dev)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, dev);
+	SET_NETDEV_DEV(dev, &pdev->dev);
+
+	priv = netdev_priv(dev);
+	priv->base = base;
+
+	priv->phy_mode = of_get_phy_mode(pdev->dev.of_node);
+	if (priv->phy_mode < 0)
+		priv->phy_mode = PHY_INTERFACE_MODE_RGMII;
+
+	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		dev_err(&pdev->dev, "failed to get clock\n");
+		ret = PTR_ERR(priv->clk);
+		goto err_free_dev;
+	}
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		goto err_free_dev;
+
+	spin_lock_init(&priv->tx_lock);
+
+	if (ops && ops->reset) {
+		ret = ops->reset(dev);
+		if (ret)
+			goto err_free_dev;
+	}
+
+	bus = devm_mdiobus_alloc(&pdev->dev);
+	if (!bus) {
+		ret = -ENOMEM;
+		goto err_disable_clk;
+	}
+
+	bus->name = "nb8800-mii";
+	bus->read = nb8800_mdio_read;
+	bus->write = nb8800_mdio_write;
+	bus->parent = &pdev->dev;
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%lx.nb8800-mii",
+		 (unsigned long)res->start);
+	bus->priv = priv;
+
+	ret = of_mdiobus_register(bus, pdev->dev.of_node);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register MII bus\n");
+		goto err_disable_clk;
+	}
+
+	priv->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
+	if (!priv->phy_node) {
+		dev_err(&pdev->dev, "no PHY specified\n");
+		ret = -ENODEV;
+		goto err_free_bus;
+	}
+
+	priv->mii_bus = bus;
+
+	ret = nb8800_hw_init(dev);
+	if (ret)
+		goto err_free_bus;
+
+	if (ops && ops->init) {
+		ret = ops->init(dev);
+		if (ret)
+			goto err_free_bus;
+	}
+
+	dev->netdev_ops = &nb8800_netdev_ops;
+	dev->ethtool_ops = &nb8800_ethtool_ops;
+	dev->flags |= IFF_MULTICAST;
+	dev->irq = irq;
+
+	mac = of_get_mac_address(pdev->dev.of_node);
+	if (mac)
+		ether_addr_copy(dev->dev_addr, mac);
+
+	if (!is_valid_ether_addr(dev->dev_addr))
+		eth_hw_addr_random(dev);
+
+	nb8800_update_mac_addr(dev);
+
+	netif_carrier_off(dev);
+
+	ret = register_netdev(dev);
+	if (ret) {
+		netdev_err(dev, "failed to register netdev\n");
+		goto err_free_dma;
+	}
+
+	netif_napi_add(dev, &priv->napi, nb8800_poll, NAPI_POLL_WEIGHT);
+
+	netdev_info(dev, "MAC address %pM\n", dev->dev_addr);
+
+	return 0;
+
+err_free_dma:
+	nb8800_dma_free(dev);
+err_free_bus:
+	mdiobus_unregister(bus);
+err_disable_clk:
+	clk_disable_unprepare(priv->clk);
+err_free_dev:
+	free_netdev(dev);
+
+	return ret;
+}
+
+static int nb8800_remove(struct platform_device *pdev)
+{
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct nb8800_priv *priv = netdev_priv(ndev);
+
+	unregister_netdev(ndev);
+
+	mdiobus_unregister(priv->mii_bus);
+
+	clk_disable_unprepare(priv->clk);
+
+	nb8800_dma_free(ndev);
+	free_netdev(ndev);
+
+	return 0;
+}
+
+static struct platform_driver nb8800_driver = {
+	.driver = {
+		.name		= "nb8800",
+		.of_match_table	= nb8800_dt_ids,
+	},
+	.probe	= nb8800_probe,
+	.remove	= nb8800_remove,
+};
+
+module_platform_driver(nb8800_driver);
+
+MODULE_DESCRIPTION("Aurora AU-NB8800 Ethernet driver");
+MODULE_AUTHOR("Mans Rullgard <mans@mansr.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/net/ethernet/aurora/nb8800.h b/drivers/net/ethernet/aurora/nb8800.h
new file mode 100644
index 0000000..e108d01
--- /dev/null
+++ b/drivers/net/ethernet/aurora/nb8800.h
@@ -0,0 +1,314 @@
+#ifndef _NB8800_H_
+#define _NB8800_H_
+
+#include <linux/types.h>
+#include <linux/skbuff.h>
+#include <linux/phy.h>
+#include <linux/clk.h>
+#include <linux/bitops.h>
+
+#define RX_DESC_COUNT			256
+#define TX_DESC_COUNT			256
+
+#define NB8800_DESC_LOW			4
+
+#define RX_BUF_SIZE			1552
+
+#define RX_COPYBREAK			256
+#define RX_COPYHDR			128
+
+#define MAX_MDC_CLOCK			2500000
+
+/* Stargate Solutions SSN8800 core registers */
+#define NB8800_TX_CTL1			0x000
+#define TX_TPD				BIT(5)
+#define TX_APPEND_FCS			BIT(4)
+#define TX_PAD_EN			BIT(3)
+#define TX_RETRY_EN			BIT(2)
+#define TX_EN				BIT(0)
+
+#define NB8800_TX_CTL2			0x001
+
+#define NB8800_RX_CTL			0x004
+#define RX_BC_DISABLE			BIT(7)
+#define RX_RUNT				BIT(6)
+#define RX_AF_EN			BIT(5)
+#define RX_PAUSE_EN			BIT(3)
+#define RX_SEND_CRC			BIT(2)
+#define RX_PAD_STRIP			BIT(1)
+#define RX_EN				BIT(0)
+
+#define NB8800_RANDOM_SEED		0x008
+#define NB8800_TX_SDP			0x14
+#define NB8800_TX_TPDP1			0x18
+#define NB8800_TX_TPDP2			0x19
+#define NB8800_SLOT_TIME		0x1c
+
+#define NB8800_MDIO_CMD			0x020
+#define MIIAR_ADDR(x)			((x) << 21)
+#define MIIAR_REG(x)			((x) << 16)
+#define MIIAR_DATA(x)			((x) <<	 0)
+#define MDIO_CMD_GO			BIT(31)
+#define MDIO_CMD_WR			BIT(26)
+
+#define NB8800_MDIO_STS			0x024
+#define MDIO_STS_ERR			BIT(31)
+
+#define NB8800_MC_ADDR(i)		(0x028 + (i))
+#define NB8800_MC_INIT			0x02e
+#define NB8800_UC_ADDR(i)		(0x03c + (i))
+
+#define NB8800_MAC_MODE			0x044
+#define RGMII_MODE			BIT(7)
+#define HALF_DUPLEX			BIT(4)
+#define BURST_EN			BIT(3)
+#define LOOPBACK_EN			BIT(2)
+#define GMAC_MODE			BIT(0)
+
+#define NB8800_IC_THRESHOLD		0x050
+#define NB8800_PE_THRESHOLD		0x051
+#define NB8800_PF_THRESHOLD		0x052
+#define NB8800_TX_BUFSIZE		0x054
+#define NB8800_FIFO_CTL			0x056
+#define NB8800_PQ1			0x060
+#define NB8800_PQ2			0x061
+#define NB8800_SRC_ADDR(i)		(0x06a + (i))
+#define NB8800_STAT_DATA		0x078
+#define NB8800_STAT_INDEX		0x07c
+#define NB8800_STAT_CLEAR		0x07d
+
+#define NB8800_SLEEP_MODE		0x07e
+#define SLEEP_MODE			BIT(0)
+
+#define NB8800_WAKEUP			0x07f
+#define WAKEUP				BIT(0)
+
+/* Aurora NB8800 host interface registers */
+#define NB8800_TXC_CR			0x100
+#define TCR_LK				BIT(12)
+#define TCR_DS				BIT(11)
+#define TCR_BTS(x)			(((x) & 0x7) << 8)
+#define TCR_DIE				BIT(7)
+#define TCR_TFI(x)			(((x) & 0x7) << 4)
+#define TCR_LE				BIT(3)
+#define TCR_RS				BIT(2)
+#define TCR_DM				BIT(1)
+#define TCR_EN				BIT(0)
+
+#define NB8800_TXC_SR			0x104
+#define TSR_DE				BIT(3)
+#define TSR_DI				BIT(2)
+#define TSR_TO				BIT(1)
+#define TSR_TI				BIT(0)
+
+#define NB8800_TX_SAR			0x108
+#define NB8800_TX_DESC_ADDR		0x10c
+
+#define NB8800_TX_REPORT_ADDR		0x110
+#define TX_BYTES_TRANSFERRED(x)		(((x) >> 16) & 0xffff)
+#define TX_FIRST_DEFERRAL		BIT(7)
+#define TX_EARLY_COLLISIONS(x)		(((x) >> 3) & 0xf)
+#define TX_LATE_COLLISION		BIT(2)
+#define TX_PACKET_DROPPED		BIT(1)
+#define TX_FIFO_UNDERRUN		BIT(0)
+#define IS_TX_ERROR(r)			((r) & 0x07)
+
+#define NB8800_TX_FIFO_SR		0x114
+#define NB8800_TX_ITR			0x118
+
+#define NB8800_RXC_CR			0x200
+#define RCR_FL				BIT(13)
+#define RCR_LK				BIT(12)
+#define RCR_DS				BIT(11)
+#define RCR_BTS(x)			(((x) & 7) << 8)
+#define RCR_DIE				BIT(7)
+#define RCR_RFI(x)			(((x) & 7) << 4)
+#define RCR_LE				BIT(3)
+#define RCR_RS				BIT(2)
+#define RCR_DM				BIT(1)
+#define RCR_EN				BIT(0)
+
+#define NB8800_RXC_SR			0x204
+#define RSR_DE				BIT(3)
+#define RSR_DI				BIT(2)
+#define RSR_RO				BIT(1)
+#define RSR_RI				BIT(0)
+
+#define NB8800_RX_SAR			0x208
+#define NB8800_RX_DESC_ADDR		0x20c
+
+#define NB8800_RX_REPORT_ADDR		0x210
+#define RX_BYTES_TRANSFERRED(x)		(((x) >> 16) & 0xFFFF)
+#define RX_MULTICAST_PKT		BIT(9)
+#define RX_BROADCAST_PKT		BIT(8)
+#define RX_LENGTH_ERR			BIT(7)
+#define RX_FCS_ERR			BIT(6)
+#define RX_RUNT_PKT			BIT(5)
+#define RX_FIFO_OVERRUN			BIT(4)
+#define RX_LATE_COLLISION		BIT(3)
+#define RX_ALIGNMENT_ERROR		BIT(2)
+#define RX_ERROR_MASK			0xfc
+#define IS_RX_ERROR(r)			((r) & RX_ERROR_MASK)
+
+#define NB8800_RX_FIFO_SR		0x214
+#define NB8800_RX_ITR			0x218
+
+/* Sigma Designs SMP86xx additional registers */
+#define NB8800_TANGOX_PAD_MODE		0x400
+#define PAD_MODE_MASK			0x7
+#define PAD_MODE_MII			0x0
+#define PAD_MODE_RGMII			0x1
+#define PAD_MODE_GTX_CLK_INV		BIT(3)
+#define PAD_MODE_GTX_CLK_DELAY		BIT(4)
+
+#define NB8800_TANGOX_MDIO_CLKDIV	0x420
+#define NB8800_TANGOX_RESET		0x424
+
+/* Hardware DMA descriptor */
+struct nb8800_dma_desc {
+	u32				s_addr;	/* start address */
+	u32				n_addr;	/* next descriptor address */
+	u32				r_addr;	/* report address */
+	u32				config;
+} __aligned(8);
+
+#define DESC_ID				BIT(23)
+#define DESC_EOC			BIT(22)
+#define DESC_EOF			BIT(21)
+#define DESC_LK				BIT(20)
+#define DESC_DS				BIT(19)
+#define DESC_BTS(x)			(((x) & 0x7) << 16)
+
+/* DMA descriptor and associated data for rx.
+ * Allocated from coherent memory.
+ */
+struct nb8800_rx_desc {
+	/* DMA descriptor */
+	struct nb8800_dma_desc		desc;
+
+	/* Status report filled in by hardware */
+	u32				report;
+};
+
+/* Address of buffer on rx ring */
+struct nb8800_rx_buf {
+	struct page			*page;
+	unsigned long			offset;
+};
+
+/* DMA descriptors and associated data for tx.
+ * Allocated from coherent memory.
+ */
+struct nb8800_tx_desc {
+	/* DMA descriptor.  The second descriptor is used if packet
+	 * data is unaligned. */
+	struct nb8800_dma_desc		desc[2];
+
+	/* Status report filled in by hardware */
+	u32				report;
+
+	/* Bounce buffer for initial unaligned part of packet */
+	u8				buf[8] __aligned(8);
+};
+
+/* Packet in tx queue */
+struct nb8800_tx_buf {
+	/* Currently queued skb */
+	struct sk_buff			*skb;
+
+	/* DMA address of the first descriptor */
+	dma_addr_t			dma_desc;
+
+	/* DMA address of packet data */
+	dma_addr_t			dma_addr;
+
+	/* Length of DMA mapping, less than skb->len if alignment
+	 * buffer is used. */
+	unsigned int			dma_len;
+
+	/* Number of packets in chain starting here */
+	unsigned int			chain_len;
+
+	/* Packet chain ready to be submitted to hardware */
+	bool				ready;
+};
+
+struct nb8800_priv {
+	struct napi_struct		napi;
+
+	void __iomem			*base;
+
+	/* RX DMA descriptors */
+	struct nb8800_rx_desc		*rx_descs;
+
+	/* RX buffers referenced by DMA descriptors */
+	struct nb8800_rx_buf		*rx_bufs;
+
+	/* Current end of chain */
+	u32				rx_eoc;
+
+	/* Value for rx interrupt time register in NAPI interrupt mode */
+	u32				rx_itr_irq;
+
+	/* Value for rx interrupt time register in NAPI poll mode */
+	u32				rx_itr_poll;
+
+	/* Value for config field of rx DMA descriptors */
+	u32				rx_dma_config;
+
+	/* TX DMA descriptors */
+	struct nb8800_tx_desc		*tx_descs;
+
+	/* TX packet queue */
+	struct nb8800_tx_buf		*tx_bufs;
+
+	/* Number of free tx queue entries */
+	atomic_t			tx_free;
+
+	/* First free tx queue entry */
+	u32				tx_next;
+
+	/* Next buffer to transmit */
+	u32				tx_queue;
+
+	/* Start of current packet chain */
+	struct nb8800_tx_buf		*tx_chain;
+
+	/* Next buffer to reclaim */
+	u32				tx_done;
+
+	/* Lock for DMA activation */
+	spinlock_t			tx_lock;
+
+	struct mii_bus			*mii_bus;
+	struct device_node		*phy_node;
+	struct phy_device		*phydev;
+
+	/* PHY connection type from DT */
+	int				phy_mode;
+
+	/* Current link status */
+	int				speed;
+	int				duplex;
+	int				link;
+
+	/* Pause settings */
+	bool				pause_aneg;
+	bool				pause_rx;
+	bool				pause_tx;
+
+	/* DMA base address of rx descriptors, see rx_descs above */
+	dma_addr_t			rx_desc_dma;
+
+	/* DMA base address of tx descriptors, see tx_descs above */
+	dma_addr_t			tx_desc_dma;
+
+	struct clk			*clk;
+};
+
+struct nb8800_ops {
+	int				(*init)(struct net_device *dev);
+	int				(*reset)(struct net_device *dev);
+};
+
+#endif /* _NB8800_H_ */
-- 
2.6.3


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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-10 16:14 [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Mans Rullgard
@ 2015-11-10 17:55 ` Eric Dumazet
  2015-11-10 18:05   ` Måns Rullgård
  2015-11-10 17:58 ` Eric Dumazet
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 52+ messages in thread
From: Eric Dumazet @ 2015-11-10 17:55 UTC (permalink / raw)
  To: Mans Rullgard; +Cc: linux-kernel, netdev, slash.tmp

On Tue, 2015-11-10 at 16:14 +0000, Mans Rullgard wrote:
> This adds a driver for the Aurora VLSI NB8800 Ethernet controller.
> It is an almost complete rewrite of a driver originally found in
> a Sigma Designs 2.6.22 tree.

...

> +
> +static void nb8800_receive(struct net_device *dev, int i, int len)
> +{
> +	struct nb8800_priv *priv = netdev_priv(dev);
> +	struct nb8800_rx_desc *rxd = &priv->rx_descs[i];
> +	struct page *page = priv->rx_bufs[i].page;
> +	int offset = priv->rx_bufs[i].offset;
> +	void *data = page_address(page) + offset;
> +	dma_addr_t dma = rxd->desc.s_addr;
> +	struct sk_buff *skb;
> +	int size;
> +	int err;
> +
> +	size = len <= RX_COPYBREAK ? len : RX_COPYHDR;
> +
> +	skb = napi_alloc_skb(&priv->napi, size);
> +	if (!skb) {
> +		netdev_err(dev, "rx skb allocation failed\n");
> +		dev->stats.rx_dropped++;
> +		return;
> +	}
> +
> +	if (len <= RX_COPYBREAK) {
> +		dma_sync_single_for_cpu(&dev->dev, dma, len, DMA_FROM_DEVICE);
> +		memcpy(skb_put(skb, len), data, len);
> +		dma_sync_single_for_device(&dev->dev, dma, len,
> +					   DMA_FROM_DEVICE);
> +	} else {
> +		err = nb8800_alloc_rx(dev, i, true);
> +		if (err) {
> +			netdev_err(dev, "rx buffer allocation failed\n");
> +			dev->stats.rx_dropped++;
> +			return;
> +		}
> +
> +		dma_unmap_page(&dev->dev, dma, RX_BUF_SIZE, DMA_FROM_DEVICE);
> +		memcpy(skb_put(skb, RX_COPYHDR), data, RX_COPYHDR);
> +		skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page,
> +				offset + RX_COPYHDR, len - RX_COPYHDR,
> +				RX_BUF_SIZE);
> +	}
> +
> +	skb->protocol = eth_type_trans(skb, dev);
> +	netif_receive_skb(skb);
> +}
> +


Any reason you do not use napi_gro_receive(&priv->napi, skb) instead of
netif_receive_skb() ?

I see you correctly use late napi_complete_done(napi, work), but this
matters only if napi_gro_receive() was used.




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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-10 16:14 [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Mans Rullgard
  2015-11-10 17:55 ` Eric Dumazet
@ 2015-11-10 17:58 ` Eric Dumazet
  2015-11-10 18:07   ` Måns Rullgård
  2015-11-10 19:13 ` Mason
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 52+ messages in thread
From: Eric Dumazet @ 2015-11-10 17:58 UTC (permalink / raw)
  To: Mans Rullgard; +Cc: linux-kernel, netdev, slash.tmp

On Tue, 2015-11-10 at 16:14 +0000, Mans Rullgard wrote:
> This adds a driver for the Aurora VLSI NB8800 Ethernet controller.
> It is an almost complete rewrite of a driver originally found in
> a Sigma Designs 2.6.22 tree.

...

> +
> +static int nb8800_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct nb8800_priv *priv = netdev_priv(dev);
> +	struct nb8800_tx_desc *txd;
> +	struct nb8800_tx_buf *txb;
> +	struct nb8800_dma_desc *dma;
> +	dma_addr_t dma_addr;
> +	unsigned int dma_len;
> +	unsigned long flags;
> +	int align;
> +	int next;
> +
> +	if (atomic_read(&priv->tx_free) <= NB8800_DESC_LOW) {
> +		netif_stop_queue(dev);
> +		return NETDEV_TX_BUSY;
> +	}
> +
> +	align = (8 - (uintptr_t)skb->data) & 7;
> +
> +	dma_len = skb->len - align;
> +	dma_addr = dma_map_single(&dev->dev, skb->data + align,
> +				  dma_len, DMA_TO_DEVICE);
> +
> +	if (dma_mapping_error(&dev->dev, dma_addr)) {
> +		netdev_err(dev, "tx dma mapping error\n");
> +		kfree_skb(skb);
> +		dev->stats.tx_dropped++;
> +		return NETDEV_TX_OK;
> +	}
> +
> +	if (atomic_dec_return(&priv->tx_free) <= NB8800_DESC_LOW)
> +		netif_stop_queue(dev);


You probably also want to clear skb->xmit_more or risk a stall

xmit_more is tricky ;)

> +
> +	next = priv->tx_next;
> +	txb = &priv->tx_bufs[next];
> +	txd = &priv->tx_descs[next];
> +	dma = &txd->desc[0];
> +
> +	next = (next + 1) % TX_DESC_COUNT;
> +
> +	if (align) {
> +		memcpy(txd->buf, skb->data, align);
> +
> +		dma->s_addr =
> +			txb->dma_desc + offsetof(struct nb8800_tx_desc, buf);
> +		dma->n_addr = txb->dma_desc + sizeof(txd->desc[0]);
> +		dma->config = DESC_BTS(2) | DESC_DS | align;
> +
> +		dma++;
> +	}
> +
> +	dma->s_addr = dma_addr;
> +	dma->n_addr = priv->tx_bufs[next].dma_desc;
> +	dma->config = DESC_BTS(2) | DESC_DS | DESC_EOF | dma_len;
> +
> +	if (!skb->xmit_more)
> +		dma->config |= DESC_EOC;
> +
> +	txb->skb = skb;
> +	txb->dma_addr = dma_addr;
> +	txb->dma_len = dma_len;
> +
> +	if (!priv->tx_chain) {
> +		txb->chain_len = 1;
> +		priv->tx_chain = txb;
> +	} else {
> +		priv->tx_chain->chain_len++;
> +	}
> +
> +	netdev_sent_queue(dev, skb->len);
> +
> +	smp_mb__before_spinlock();
> +	spin_lock_irqsave(&priv->tx_lock, flags);
> +
> +	if (!skb->xmit_more) {
> +		priv->tx_chain->ready = true;
> +		priv->tx_chain = NULL;
> +		nb8800_tx_dma_start(dev);
> +	}
> +
> +	spin_unlock_irqrestore(&priv->tx_lock, flags);
> +
> +	priv->tx_next = next;
> +
> +	return NETDEV_TX_OK;
> +}
> +


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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-10 17:55 ` Eric Dumazet
@ 2015-11-10 18:05   ` Måns Rullgård
  2015-11-10 20:04     ` David Miller
  0 siblings, 1 reply; 52+ messages in thread
From: Måns Rullgård @ 2015-11-10 18:05 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: linux-kernel, netdev, slash.tmp

Eric Dumazet <eric.dumazet@gmail.com> writes:

> On Tue, 2015-11-10 at 16:14 +0000, Mans Rullgard wrote:
>> This adds a driver for the Aurora VLSI NB8800 Ethernet controller.
>> It is an almost complete rewrite of a driver originally found in
>> a Sigma Designs 2.6.22 tree.
>
> ...
>
>> +
>> +static void nb8800_receive(struct net_device *dev, int i, int len)
>> +{
>> +	struct nb8800_priv *priv = netdev_priv(dev);
>> +	struct nb8800_rx_desc *rxd = &priv->rx_descs[i];
>> +	struct page *page = priv->rx_bufs[i].page;
>> +	int offset = priv->rx_bufs[i].offset;
>> +	void *data = page_address(page) + offset;
>> +	dma_addr_t dma = rxd->desc.s_addr;
>> +	struct sk_buff *skb;
>> +	int size;
>> +	int err;
>> +
>> +	size = len <= RX_COPYBREAK ? len : RX_COPYHDR;
>> +
>> +	skb = napi_alloc_skb(&priv->napi, size);
>> +	if (!skb) {
>> +		netdev_err(dev, "rx skb allocation failed\n");
>> +		dev->stats.rx_dropped++;
>> +		return;
>> +	}
>> +
>> +	if (len <= RX_COPYBREAK) {
>> +		dma_sync_single_for_cpu(&dev->dev, dma, len, DMA_FROM_DEVICE);
>> +		memcpy(skb_put(skb, len), data, len);
>> +		dma_sync_single_for_device(&dev->dev, dma, len,
>> +					   DMA_FROM_DEVICE);
>> +	} else {
>> +		err = nb8800_alloc_rx(dev, i, true);
>> +		if (err) {
>> +			netdev_err(dev, "rx buffer allocation failed\n");
>> +			dev->stats.rx_dropped++;
>> +			return;
>> +		}
>> +
>> +		dma_unmap_page(&dev->dev, dma, RX_BUF_SIZE, DMA_FROM_DEVICE);
>> +		memcpy(skb_put(skb, RX_COPYHDR), data, RX_COPYHDR);
>> +		skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page,
>> +				offset + RX_COPYHDR, len - RX_COPYHDR,
>> +				RX_BUF_SIZE);
>> +	}
>> +
>> +	skb->protocol = eth_type_trans(skb, dev);
>> +	netif_receive_skb(skb);
>> +}
>> +
>
> Any reason you do not use napi_gro_receive(&priv->napi, skb) instead of
> netif_receive_skb() ?

Because I haven't been following the netdev list closely for the last
five years, and no documentation I read mentioned this function.  I can
certainly change it.

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-10 17:58 ` Eric Dumazet
@ 2015-11-10 18:07   ` Måns Rullgård
  0 siblings, 0 replies; 52+ messages in thread
From: Måns Rullgård @ 2015-11-10 18:07 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: linux-kernel, netdev, slash.tmp

Eric Dumazet <eric.dumazet@gmail.com> writes:

> On Tue, 2015-11-10 at 16:14 +0000, Mans Rullgard wrote:
>> This adds a driver for the Aurora VLSI NB8800 Ethernet controller.
>> It is an almost complete rewrite of a driver originally found in
>> a Sigma Designs 2.6.22 tree.
>
> ...
>
>> +
>> +static int nb8800_xmit(struct sk_buff *skb, struct net_device *dev)
>> +{
>> +	struct nb8800_priv *priv = netdev_priv(dev);
>> +	struct nb8800_tx_desc *txd;
>> +	struct nb8800_tx_buf *txb;
>> +	struct nb8800_dma_desc *dma;
>> +	dma_addr_t dma_addr;
>> +	unsigned int dma_len;
>> +	unsigned long flags;
>> +	int align;
>> +	int next;
>> +
>> +	if (atomic_read(&priv->tx_free) <= NB8800_DESC_LOW) {
>> +		netif_stop_queue(dev);
>> +		return NETDEV_TX_BUSY;
>> +	}
>> +
>> +	align = (8 - (uintptr_t)skb->data) & 7;
>> +
>> +	dma_len = skb->len - align;
>> +	dma_addr = dma_map_single(&dev->dev, skb->data + align,
>> +				  dma_len, DMA_TO_DEVICE);
>> +
>> +	if (dma_mapping_error(&dev->dev, dma_addr)) {
>> +		netdev_err(dev, "tx dma mapping error\n");
>> +		kfree_skb(skb);
>> +		dev->stats.tx_dropped++;
>> +		return NETDEV_TX_OK;
>> +	}
>> +
>> +	if (atomic_dec_return(&priv->tx_free) <= NB8800_DESC_LOW)
>> +		netif_stop_queue(dev);
>
> You probably also want to clear skb->xmit_more or risk a stall

Very true, will fix.

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-10 16:14 [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Mans Rullgard
  2015-11-10 17:55 ` Eric Dumazet
  2015-11-10 17:58 ` Eric Dumazet
@ 2015-11-10 19:13 ` Mason
  2015-11-10 19:25   ` Måns Rullgård
  2015-11-10 22:09 ` Andy Shevchenko
  2015-11-10 23:34 ` Francois Romieu
  4 siblings, 1 reply; 52+ messages in thread
From: Mason @ 2015-11-10 19:13 UTC (permalink / raw)
  To: Mans Rullgard; +Cc: linux-kernel, netdev

On 10/11/2015 17:14, Mans Rullgard wrote:

> This adds a driver for the Aurora VLSI NB8800 Ethernet controller.
> It is an almost complete rewrite of a driver originally found in
> a Sigma Designs 2.6.22 tree.
> 
> Signed-off-by: Mans Rullgard <mans@mansr.com>
> ---
> Changes:
> - Refactored mdio access functions
> - Refactored register access helpers
> - Improved error handling in rx buffer allocation
> - Optimised some fifo parameters
> - Overhauled tx dma. Multiple packets are now chained in a single dma
>   operation if xmit_more is set, improving performance.
> - Improved rx irq handling. It's not possible to disable interrupts
>   entirely for napi poll, but they can be slowed down a little.
> - Use readx_poll_timeout in various places
> - Improved error detection
> - Improved statistics
> - Report hardware statistics counters through ethtool
> - Improved tangox-specific setup
> - Support for flow control using pause frames
> - Explanatory comments added
> - Various minor stylistic changes
> ---
>  drivers/net/ethernet/Kconfig         |    1 +
>  drivers/net/ethernet/Makefile        |    1 +
>  drivers/net/ethernet/aurora/Kconfig  |   20 +
>  drivers/net/ethernet/aurora/Makefile |    1 +
>  drivers/net/ethernet/aurora/nb8800.c | 1530 ++++++++++++++++++++++++++++++++++
>  drivers/net/ethernet/aurora/nb8800.h |  314 +++++++
>  6 files changed, 1867 insertions(+)

The code has grown much since the previous patch, despite some
refactoring. Is this mostly due to ethtool_ops support?

 drivers/net/ethernet/aurora/nb8800.c | 1146 ++++++++++++++++++++++++++++++++++
 drivers/net/ethernet/aurora/nb8800.h |  230 +++++++

Regards.


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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-10 19:13 ` Mason
@ 2015-11-10 19:25   ` Måns Rullgård
  2015-11-12 13:33     ` Mason
  0 siblings, 1 reply; 52+ messages in thread
From: Måns Rullgård @ 2015-11-10 19:25 UTC (permalink / raw)
  To: Mason; +Cc: linux-kernel, netdev

Mason <slash.tmp@free.fr> writes:

> On 10/11/2015 17:14, Mans Rullgard wrote:
>
>> This adds a driver for the Aurora VLSI NB8800 Ethernet controller.
>> It is an almost complete rewrite of a driver originally found in
>> a Sigma Designs 2.6.22 tree.
>> 
>> Signed-off-by: Mans Rullgard <mans@mansr.com>
>> ---
>> Changes:
>> - Refactored mdio access functions
>> - Refactored register access helpers
>> - Improved error handling in rx buffer allocation
>> - Optimised some fifo parameters
>> - Overhauled tx dma. Multiple packets are now chained in a single dma
>>   operation if xmit_more is set, improving performance.
>> - Improved rx irq handling. It's not possible to disable interrupts
>>   entirely for napi poll, but they can be slowed down a little.
>> - Use readx_poll_timeout in various places
>> - Improved error detection
>> - Improved statistics
>> - Report hardware statistics counters through ethtool
>> - Improved tangox-specific setup
>> - Support for flow control using pause frames
>> - Explanatory comments added
>> - Various minor stylistic changes
>> ---
>>  drivers/net/ethernet/Kconfig         |    1 +
>>  drivers/net/ethernet/Makefile        |    1 +
>>  drivers/net/ethernet/aurora/Kconfig  |   20 +
>>  drivers/net/ethernet/aurora/Makefile |    1 +
>>  drivers/net/ethernet/aurora/nb8800.c | 1530 ++++++++++++++++++++++++++++++++++
>>  drivers/net/ethernet/aurora/nb8800.h |  314 +++++++
>>  6 files changed, 1867 insertions(+)
>
> The code has grown much since the previous patch, despite some
> refactoring. Is this mostly due to ethtool_ops support?
>
>  drivers/net/ethernet/aurora/nb8800.c | 1146 ++++++++++++++++++++++++++++++++++
>  drivers/net/ethernet/aurora/nb8800.h |  230 +++++++

Some of the increase is from new features, some from improvements, and
then there are a bunch of new comments.

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-10 18:05   ` Måns Rullgård
@ 2015-11-10 20:04     ` David Miller
  2015-11-10 20:53       ` Måns Rullgård
  0 siblings, 1 reply; 52+ messages in thread
From: David Miller @ 2015-11-10 20:04 UTC (permalink / raw)
  To: mans; +Cc: eric.dumazet, linux-kernel, netdev, slash.tmp

From: Måns Rullgård <mans@mansr.com>
Date: Tue, 10 Nov 2015 18:05:15 +0000

> Because I haven't been following the netdev list closely for the last
> five years, and no documentation I read mentioned this function.  I can
> certainly change it.

It is always advisable to mimick what other drivers do and use them as
a reference, rather than depend upon documentation which by definition
is always going to be out of sync with the source tree.

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-10 20:04     ` David Miller
@ 2015-11-10 20:53       ` Måns Rullgård
  2015-11-10 21:06         ` David Miller
  0 siblings, 1 reply; 52+ messages in thread
From: Måns Rullgård @ 2015-11-10 20:53 UTC (permalink / raw)
  To: David Miller; +Cc: eric.dumazet, linux-kernel, netdev, slash.tmp

David Miller <davem@davemloft.net> writes:

> From: Måns Rullgård <mans@mansr.com>
> Date: Tue, 10 Nov 2015 18:05:15 +0000
>
>> Because I haven't been following the netdev list closely for the last
>> five years, and no documentation I read mentioned this function.  I can
>> certainly change it.
>
> It is always advisable to mimick what other drivers do and use them as
> a reference, rather than depend upon documentation which by definition
> is always going to be out of sync with the source tree.

Sure.  The trick is to pick the right driver(s) to use as reference.
Quite a few of them don't use that function.

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-10 20:53       ` Måns Rullgård
@ 2015-11-10 21:06         ` David Miller
  2015-11-10 21:21           ` Måns Rullgård
  0 siblings, 1 reply; 52+ messages in thread
From: David Miller @ 2015-11-10 21:06 UTC (permalink / raw)
  To: mans; +Cc: eric.dumazet, linux-kernel, netdev, slash.tmp

From: Måns Rullgård <mans@mansr.com>
Date: Tue, 10 Nov 2015 20:53:19 +0000

> David Miller <davem@davemloft.net> writes:
> 
>> From: Måns Rullgård <mans@mansr.com>
>> Date: Tue, 10 Nov 2015 18:05:15 +0000
>>
>>> Because I haven't been following the netdev list closely for the last
>>> five years, and no documentation I read mentioned this function.  I can
>>> certainly change it.
>>
>> It is always advisable to mimick what other drivers do and use them as
>> a reference, rather than depend upon documentation which by definition
>> is always going to be out of sync with the source tree.
> 
> Sure.  The trick is to pick the right driver(s) to use as reference.
> Quite a few of them don't use that function.

If you really are stumped on this matter, start at least with the
ixgbe driver.  In fact pretty much every Intel ethernet driver is
a reasonable reference.  Others to check out are bnx2x and mlx5.


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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-10 21:06         ` David Miller
@ 2015-11-10 21:21           ` Måns Rullgård
  2015-11-10 21:51             ` Eric Dumazet
  0 siblings, 1 reply; 52+ messages in thread
From: Måns Rullgård @ 2015-11-10 21:21 UTC (permalink / raw)
  To: David Miller; +Cc: eric.dumazet, linux-kernel, netdev, slash.tmp

David Miller <davem@davemloft.net> writes:

> From: Måns Rullgård <mans@mansr.com>
> Date: Tue, 10 Nov 2015 20:53:19 +0000
>
>> David Miller <davem@davemloft.net> writes:
>> 
>>> From: Måns Rullgård <mans@mansr.com>
>>> Date: Tue, 10 Nov 2015 18:05:15 +0000
>>>
>>>> Because I haven't been following the netdev list closely for the last
>>>> five years, and no documentation I read mentioned this function.  I can
>>>> certainly change it.
>>>
>>> It is always advisable to mimick what other drivers do and use them as
>>> a reference, rather than depend upon documentation which by definition
>>> is always going to be out of sync with the source tree.
>> 
>> Sure.  The trick is to pick the right driver(s) to use as reference.
>> Quite a few of them don't use that function.
>
> If you really are stumped on this matter, start at least with the
> ixgbe driver.  In fact pretty much every Intel ethernet driver is
> a reasonable reference.  Others to check out are bnx2x and mlx5.

Even ixgbe uses napi_complete() while netdevice.h says one should
"consider using napi_complete_done() instead."  Did the author consider
it and decide not to, or has the driver simply not been updated?

As for the napi_gro_receive() function, calling that instead of
netif_receive_skb() is easy enough, or are there other things I should
be doing in addition?

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-10 21:21           ` Måns Rullgård
@ 2015-11-10 21:51             ` Eric Dumazet
  2015-11-11 13:41               ` Mason
  0 siblings, 1 reply; 52+ messages in thread
From: Eric Dumazet @ 2015-11-10 21:51 UTC (permalink / raw)
  To: Måns Rullgård; +Cc: David Miller, linux-kernel, netdev, slash.tmp

On Tue, 2015-11-10 at 21:21 +0000, Måns Rullgård wrote:

> Even ixgbe uses napi_complete() while netdevice.h says one should
> "consider using napi_complete_done() instead."  Did the author consider
> it and decide not to, or has the driver simply not been updated?

napi_complete_done() is quite new, very few drivers use it.

It still requires a tuning (/sys/class/net/ethX/gro_flush_timeout)

> 
> As for the napi_gro_receive() function, calling that instead of
> netif_receive_skb() is easy enough, or are there other things I should
> be doing in addition?

Nothing comes to mind, if you already have a NAPI context,
napi_gro_receive() is the way to go...




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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-10 16:14 [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Mans Rullgard
                   ` (2 preceding siblings ...)
  2015-11-10 19:13 ` Mason
@ 2015-11-10 22:09 ` Andy Shevchenko
  2015-11-10 22:34   ` Måns Rullgård
  2015-11-10 23:34 ` Francois Romieu
  4 siblings, 1 reply; 52+ messages in thread
From: Andy Shevchenko @ 2015-11-10 22:09 UTC (permalink / raw)
  To: Mans Rullgard; +Cc: linux-kernel, netdev, slash.tmp

On Tue, Nov 10, 2015 at 6:14 PM, Mans Rullgard <mans@mansr.com> wrote:
> This adds a driver for the Aurora VLSI NB8800 Ethernet controller.
> It is an almost complete rewrite of a driver originally found in
> a Sigma Designs 2.6.22 tree.

Few nitpicks below.

>
> Signed-off-by: Mans Rullgard <mans@mansr.com>
> ---
> Changes:
> - Refactored mdio access functions
> - Refactored register access helpers
> - Improved error handling in rx buffer allocation
> - Optimised some fifo parameters
> - Overhauled tx dma. Multiple packets are now chained in a single dma
>   operation if xmit_more is set, improving performance.
> - Improved rx irq handling. It's not possible to disable interrupts
>   entirely for napi poll, but they can be slowed down a little.
> - Use readx_poll_timeout in various places
> - Improved error detection
> - Improved statistics
> - Report hardware statistics counters through ethtool
> - Improved tangox-specific setup
> - Support for flow control using pause frames
> - Explanatory comments added
> - Various minor stylistic changes
> ---
>  drivers/net/ethernet/Kconfig         |    1 +
>  drivers/net/ethernet/Makefile        |    1 +
>  drivers/net/ethernet/aurora/Kconfig  |   20 +
>  drivers/net/ethernet/aurora/Makefile |    1 +
>  drivers/net/ethernet/aurora/nb8800.c | 1530 ++++++++++++++++++++++++++++++++++
>  drivers/net/ethernet/aurora/nb8800.h |  314 +++++++
>  6 files changed, 1867 insertions(+)
>  create mode 100644 drivers/net/ethernet/aurora/Kconfig
>  create mode 100644 drivers/net/ethernet/aurora/Makefile
>  create mode 100644 drivers/net/ethernet/aurora/nb8800.c
>  create mode 100644 drivers/net/ethernet/aurora/nb8800.h
>
> diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
> index 05aa759..8310163 100644
> --- a/drivers/net/ethernet/Kconfig
> +++ b/drivers/net/ethernet/Kconfig
> @@ -29,6 +29,7 @@ source "drivers/net/ethernet/apm/Kconfig"
>  source "drivers/net/ethernet/apple/Kconfig"
>  source "drivers/net/ethernet/arc/Kconfig"
>  source "drivers/net/ethernet/atheros/Kconfig"
> +source "drivers/net/ethernet/aurora/Kconfig"
>  source "drivers/net/ethernet/cadence/Kconfig"
>  source "drivers/net/ethernet/adi/Kconfig"
>  source "drivers/net/ethernet/broadcom/Kconfig"
> diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
> index ddfc808..b435fb0 100644
> --- a/drivers/net/ethernet/Makefile
> +++ b/drivers/net/ethernet/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_NET_XGENE) += apm/
>  obj-$(CONFIG_NET_VENDOR_APPLE) += apple/
>  obj-$(CONFIG_NET_VENDOR_ARC) += arc/
>  obj-$(CONFIG_NET_VENDOR_ATHEROS) += atheros/
> +obj-$(CONFIG_NET_VENDOR_AURORA) += aurora/
>  obj-$(CONFIG_NET_CADENCE) += cadence/
>  obj-$(CONFIG_NET_BFIN) += adi/
>  obj-$(CONFIG_NET_VENDOR_BROADCOM) += broadcom/
> diff --git a/drivers/net/ethernet/aurora/Kconfig b/drivers/net/ethernet/aurora/Kconfig
> new file mode 100644
> index 0000000..a3c7106
> --- /dev/null
> +++ b/drivers/net/ethernet/aurora/Kconfig
> @@ -0,0 +1,20 @@
> +config NET_VENDOR_AURORA
> +       bool "Aurora VLSI devices"
> +       help
> +         If you have a network (Ethernet) device belonging to this class,
> +         say Y.
> +
> +         Note that the answer to this question doesn't directly affect the
> +         kernel: saying N will just cause the configurator to skip all
> +         questions about Aurora devices. If you say Y, you will be asked
> +         for your specific device in the following questions.
> +
> +if NET_VENDOR_AURORA
> +
> +config AURORA_NB8800
> +       tristate "Aurora AU-NB8800 support"
> +       select PHYLIB
> +       help
> +        Support for the AU-NB8800 gigabit Ethernet controller.
> +
> +endif
> diff --git a/drivers/net/ethernet/aurora/Makefile b/drivers/net/ethernet/aurora/Makefile
> new file mode 100644
> index 0000000..6cb528a
> --- /dev/null
> +++ b/drivers/net/ethernet/aurora/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_AURORA_NB8800) += nb8800.o
> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
> new file mode 100644
> index 0000000..11cd389
> --- /dev/null
> +++ b/drivers/net/ethernet/aurora/nb8800.c
> @@ -0,0 +1,1530 @@
> +/*
> + * Copyright (C) 2015 Mans Rullgard <mans@mansr.com>
> + *
> + * Mostly rewritten, based on driver from Sigma Designs.  Original
> + * copyright notice below.
> + *
> + *
> + * Driver for tangox SMP864x/SMP865x/SMP867x/SMP868x builtin Ethernet Mac.
> + *
> + * Copyright (C) 2005 Maxime Bizon <mbizon@freebox.fr>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/etherdevice.h>
> +#include <linux/delay.h>
> +#include <linux/ethtool.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_device.h>
> +#include <linux/of_mdio.h>
> +#include <linux/of_net.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/phy.h>
> +#include <linux/cache.h>
> +#include <linux/jiffies.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <asm/barrier.h>
> +
> +#include "nb8800.h"
> +
> +static int nb8800_dma_stop(struct net_device *dev);
> +
> +static inline u8 nb8800_readb(struct nb8800_priv *priv, int reg)
> +{
> +       return readb(priv->base + reg);
> +}
> +
> +static inline u32 nb8800_readl(struct nb8800_priv *priv, int reg)
> +{
> +       return readl(priv->base + reg);
> +}
> +
> +static inline void nb8800_writeb(struct nb8800_priv *priv, int reg, u8 val)
> +{
> +       writeb(val, priv->base + reg);
> +}
> +
> +static inline void nb8800_writew(struct nb8800_priv *priv, int reg, u16 val)
> +{
> +       writew(val, priv->base + reg);
> +}
> +
> +static inline void nb8800_writel(struct nb8800_priv *priv, int reg, u32 val)
> +{
> +       writel(val, priv->base + reg);
> +}
> +
> +static inline void nb8800_maskb(struct nb8800_priv *priv, int reg,
> +                               u32 mask, u32 val)
> +{
> +       u32 old = nb8800_readb(priv, reg);
> +       u32 new = (old & ~mask) | val;

Shoudn't be "… | (val & mask);" ?

+ empty line?

> +       if (new != old)
> +               nb8800_writeb(priv, reg, new);
> +}
> +
> +static inline void nb8800_maskl(struct nb8800_priv *priv, int reg,
> +                               u32 mask, u32 val)
> +{
> +       u32 old = nb8800_readl(priv, reg);
> +       u32 new = (old & ~mask) | val;

Ditto.

> +       if (new != old)
> +               nb8800_writel(priv, reg, new);
> +}
> +
> +static inline void nb8800_modb(struct nb8800_priv *priv, int reg, u8 bits,
> +                              bool set)
> +{
> +       nb8800_maskb(priv, reg, bits, set ? bits : 0);
> +}
> +
> +static inline void nb8800_setb(struct nb8800_priv *priv, int reg, u8 bits)
> +{
> +       nb8800_maskb(priv, reg, bits, bits);
> +}
> +
> +static inline void nb8800_clearb(struct nb8800_priv *priv, int reg, u8 bits)
> +{
> +       nb8800_maskb(priv, reg, bits, 0);
> +}
> +
> +static inline void nb8800_modl(struct nb8800_priv *priv, int reg, u32 bits,
> +                              bool set)
> +{
> +       nb8800_maskl(priv, reg, bits, set ? bits : 0);
> +}
> +
> +static inline void nb8800_setl(struct nb8800_priv *priv, int reg, u32 bits)
> +{
> +       nb8800_maskl(priv, reg, bits, bits);
> +}
> +
> +static inline void nb8800_clearl(struct nb8800_priv *priv, int reg, u32 bits)
> +{
> +       nb8800_maskl(priv, reg, bits, 0);
> +}
> +
> +static int nb8800_mdio_wait(struct mii_bus *bus)
> +{
> +       struct nb8800_priv *priv = bus->priv;
> +       u32 val;
> +
> +       return readl_poll_timeout_atomic(priv->base + NB8800_MDIO_CMD,
> +                                        val, !(val & MDIO_CMD_GO), 1, 1000);
> +}
> +
> +static int nb8800_mdio_cmd(struct mii_bus *bus, u32 cmd)
> +{
> +       struct nb8800_priv *priv = bus->priv;
> +       int err;
> +
> +       err = nb8800_mdio_wait(bus);
> +       if (err)
> +               return err;
> +
> +       nb8800_writel(priv, NB8800_MDIO_CMD, cmd);
> +       udelay(10);
> +       nb8800_writel(priv, NB8800_MDIO_CMD, cmd | MDIO_CMD_GO);
> +
> +       return nb8800_mdio_wait(bus);
> +}
> +
> +static int nb8800_mdio_read(struct mii_bus *bus, int phy_id, int reg)
> +{
> +       struct nb8800_priv *priv = bus->priv;
> +       u32 val;
> +       int err;
> +
> +       err = nb8800_mdio_cmd(bus, MIIAR_ADDR(phy_id) | MIIAR_REG(reg));
> +       if (err)
> +               return err;
> +
> +       val = nb8800_readl(priv, NB8800_MDIO_STS);
> +       if (val & MDIO_STS_ERR)
> +               return 0xffff;
> +
> +       return val & 0xffff;
> +}
> +
> +static int nb8800_mdio_write(struct mii_bus *bus, int phy_id, int reg, u16 val)
> +{
> +       u32 cmd = MIIAR_DATA(val) | MIIAR_ADDR(phy_id) | MIIAR_REG(reg) |
> +               MDIO_CMD_WR;
> +
> +       return nb8800_mdio_cmd(bus, cmd);
> +}
> +
> +static void nb8800_mac_tx(struct net_device *dev, bool enable)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +
> +       while (nb8800_readl(priv, NB8800_TXC_CR) & TCR_EN)
> +               cpu_relax();
> +
> +       nb8800_modb(priv, NB8800_TX_CTL1, TX_EN, enable);
> +}
> +
> +static void nb8800_mac_rx(struct net_device *dev, bool enable)
> +{
> +       nb8800_modb(netdev_priv(dev), NB8800_RX_CTL, RX_EN, enable);
> +}
> +
> +static void nb8800_mac_af(struct net_device *dev, bool enable)
> +{
> +       nb8800_modb(netdev_priv(dev), NB8800_RX_CTL, RX_AF_EN, enable);
> +}
> +
> +static void nb8800_start_rx(struct net_device *dev)
> +{
> +       nb8800_setl(netdev_priv(dev), NB8800_RXC_CR, RCR_EN);
> +}
> +
> +static int nb8800_alloc_rx(struct net_device *dev, int i, bool napi)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       struct nb8800_rx_desc *rxd = &priv->rx_descs[i];
> +       struct nb8800_rx_buf *rxb = &priv->rx_bufs[i];
> +       int size = L1_CACHE_ALIGN(RX_BUF_SIZE);
> +       dma_addr_t dma_addr;
> +       struct page *page;
> +       unsigned long offset;
> +       void *data;
> +
> +       data = napi ? napi_alloc_frag(size) : netdev_alloc_frag(size);
> +       if (!data)
> +               return -ENOMEM;
> +
> +       page = virt_to_head_page(data);
> +       offset = data - page_address(page);
> +
> +       dma_addr = dma_map_page(&dev->dev, page, offset, RX_BUF_SIZE,
> +                               DMA_FROM_DEVICE);
> +
> +       if (dma_mapping_error(&dev->dev, dma_addr)) {
> +               skb_free_frag(data);
> +               return -ENOMEM;
> +       }
> +
> +       rxb->page = page;
> +       rxb->offset = offset;
> +       rxd->desc.s_addr = dma_addr;
> +
> +       return 0;
> +}
> +
> +static void nb8800_receive(struct net_device *dev, int i, int len)

unsigned int i ?
len as well?

> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       struct nb8800_rx_desc *rxd = &priv->rx_descs[i];
> +       struct page *page = priv->rx_bufs[i].page;
> +       int offset = priv->rx_bufs[i].offset;
> +       void *data = page_address(page) + offset;
> +       dma_addr_t dma = rxd->desc.s_addr;

dma -> addr ?

> +       struct sk_buff *skb;
> +       int size;
> +       int err;
> +
> +       size = len <= RX_COPYBREAK ? len : RX_COPYHDR;
> +
> +       skb = napi_alloc_skb(&priv->napi, size);
> +       if (!skb) {
> +               netdev_err(dev, "rx skb allocation failed\n");
> +               dev->stats.rx_dropped++;
> +               return;
> +       }
> +
> +       if (len <= RX_COPYBREAK) {
> +               dma_sync_single_for_cpu(&dev->dev, dma, len, DMA_FROM_DEVICE);
> +               memcpy(skb_put(skb, len), data, len);
> +               dma_sync_single_for_device(&dev->dev, dma, len,
> +                                          DMA_FROM_DEVICE);
> +       } else {
> +               err = nb8800_alloc_rx(dev, i, true);
> +               if (err) {
> +                       netdev_err(dev, "rx buffer allocation failed\n");
> +                       dev->stats.rx_dropped++;
> +                       return;
> +               }
> +
> +               dma_unmap_page(&dev->dev, dma, RX_BUF_SIZE, DMA_FROM_DEVICE);
> +               memcpy(skb_put(skb, RX_COPYHDR), data, RX_COPYHDR);
> +               skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page,
> +                               offset + RX_COPYHDR, len - RX_COPYHDR,
> +                               RX_BUF_SIZE);
> +       }
> +
> +       skb->protocol = eth_type_trans(skb, dev);
> +       netif_receive_skb(skb);
> +}
> +
> +static void nb8800_rx_error(struct net_device *dev, u32 report)
> +{
> +       if (report & RX_LENGTH_ERR)
> +               dev->stats.rx_length_errors++;
> +
> +       if (report & RX_FCS_ERR)
> +               dev->stats.rx_crc_errors++;
> +
> +       if (report & RX_FIFO_OVERRUN)
> +               dev->stats.rx_fifo_errors++;
> +
> +       if (report & RX_ALIGNMENT_ERROR)
> +               dev->stats.rx_frame_errors++;
> +
> +       dev->stats.rx_errors++;
> +}
> +
> +static int nb8800_poll(struct napi_struct *napi, int budget)
> +{
> +       struct net_device *dev = napi->dev;
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       struct nb8800_rx_desc *rxd;
> +       int work = 0;
> +       int last = priv->rx_eoc;
> +       int next;
> +
> +again:
> +       while (work < budget) {
> +               struct nb8800_rx_buf *rxb;
> +               int len;
> +
> +               next = (last + 1) % RX_DESC_COUNT;
> +
> +               rxb = &priv->rx_bufs[next];
> +               rxd = &priv->rx_descs[next];
> +
> +               if (!rxd->report)
> +                       break;
> +
> +               len = RX_BYTES_TRANSFERRED(rxd->report);
> +
> +               if (IS_RX_ERROR(rxd->report))
> +                       nb8800_rx_error(dev, rxd->report);
> +               else
> +                       nb8800_receive(dev, next, len);
> +
> +               dev->stats.rx_packets++;
> +               dev->stats.rx_bytes += len;
> +
> +               if (rxd->report & RX_MULTICAST_PKT)
> +                       dev->stats.multicast++;
> +
> +               rxd->report = 0;
> +               last = next;
> +               work++;
> +       }
> +
> +       if (work) {
> +               priv->rx_descs[last].desc.config |= DESC_EOC;
> +               wmb();  /* ensure new EOC is written before clearing old */
> +               priv->rx_descs[priv->rx_eoc].desc.config &= ~DESC_EOC;
> +               priv->rx_eoc = last;
> +               nb8800_start_rx(dev);
> +       }
> +
> +       if (work < budget) {
> +               nb8800_writel(priv, NB8800_RX_ITR, priv->rx_itr_irq);
> +
> +               /* If a packet arrived after we last checked but
> +                * before writing RX_ITR, the interrupt will be
> +                * delayed, so we retrieve it now. */

Block comments usually
/*
 * text
 */

Can be longer lines?

> +               if (priv->rx_descs[next].report)
> +                       goto again;
> +
> +               napi_complete_done(napi, work);
> +       }
> +
> +       return work;
> +}
> +
> +static void nb8800_tx_dma_start(struct net_device *dev)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       struct nb8800_tx_buf *txb;
> +       u32 txc_cr;
> +
> +       txb = &priv->tx_bufs[priv->tx_queue];
> +       if (!txb->ready)
> +               return;
> +
> +       txc_cr = nb8800_readl(priv, NB8800_TXC_CR);
> +       if (txc_cr & TCR_EN)
> +               return;
> +
> +       nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
> +       wmb();          /* ensure desc addr is written before starting DMA */

Hm… Have I missed corresponding rmb() ? If it's about MMIO, perhaps mmiowb() ?

> +       nb8800_writel(priv, NB8800_TXC_CR, txc_cr | TCR_EN);
> +
> +       priv->tx_queue = (priv->tx_queue + txb->chain_len) % TX_DESC_COUNT;
> +}
> +
> +static void nb8800_tx_dma_start_irq(struct net_device *dev)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +
> +       spin_lock(&priv->tx_lock);
> +       nb8800_tx_dma_start(dev);
> +       spin_unlock(&priv->tx_lock);
> +}
> +
> +static int nb8800_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       struct nb8800_tx_desc *txd;
> +       struct nb8800_tx_buf *txb;
> +       struct nb8800_dma_desc *dma;

dma -> desc ?

> +       dma_addr_t dma_addr;
> +       unsigned int dma_len;
> +       unsigned long flags;
> +       int align;
> +       int next;
> +
> +       if (atomic_read(&priv->tx_free) <= NB8800_DESC_LOW) {
> +               netif_stop_queue(dev);
> +               return NETDEV_TX_BUSY;
> +       }
> +
> +       align = (8 - (uintptr_t)skb->data) & 7;
> +
> +       dma_len = skb->len - align;
> +       dma_addr = dma_map_single(&dev->dev, skb->data + align,
> +                                 dma_len, DMA_TO_DEVICE);
> +
> +       if (dma_mapping_error(&dev->dev, dma_addr)) {
> +               netdev_err(dev, "tx dma mapping error\n");
> +               kfree_skb(skb);
> +               dev->stats.tx_dropped++;
> +               return NETDEV_TX_OK;
> +       }
> +
> +       if (atomic_dec_return(&priv->tx_free) <= NB8800_DESC_LOW)
> +               netif_stop_queue(dev);
> +
> +       next = priv->tx_next;
> +       txb = &priv->tx_bufs[next];
> +       txd = &priv->tx_descs[next];
> +       dma = &txd->desc[0];
> +
> +       next = (next + 1) % TX_DESC_COUNT;
> +
> +       if (align) {
> +               memcpy(txd->buf, skb->data, align);
> +
> +               dma->s_addr =
> +                       txb->dma_desc + offsetof(struct nb8800_tx_desc, buf);
> +               dma->n_addr = txb->dma_desc + sizeof(txd->desc[0]);
> +               dma->config = DESC_BTS(2) | DESC_DS | align;
> +
> +               dma++;
> +       }
> +
> +       dma->s_addr = dma_addr;
> +       dma->n_addr = priv->tx_bufs[next].dma_desc;
> +       dma->config = DESC_BTS(2) | DESC_DS | DESC_EOF | dma_len;
> +
> +       if (!skb->xmit_more)
> +               dma->config |= DESC_EOC;
> +
> +       txb->skb = skb;
> +       txb->dma_addr = dma_addr;
> +       txb->dma_len = dma_len;
> +
> +       if (!priv->tx_chain) {
> +               txb->chain_len = 1;
> +               priv->tx_chain = txb;
> +       } else {
> +               priv->tx_chain->chain_len++;
> +       }
> +
> +       netdev_sent_queue(dev, skb->len);
> +
> +       smp_mb__before_spinlock();
> +       spin_lock_irqsave(&priv->tx_lock, flags);
> +
> +       if (!skb->xmit_more) {
> +               priv->tx_chain->ready = true;
> +               priv->tx_chain = NULL;
> +               nb8800_tx_dma_start(dev);
> +       }
> +
> +       spin_unlock_irqrestore(&priv->tx_lock, flags);
> +
> +       priv->tx_next = next;
> +
> +       return NETDEV_TX_OK;
> +}
> +
> +static void nb8800_tx_error(struct net_device *dev, u32 report)
> +{
> +       if (report & TX_LATE_COLLISION)
> +               dev->stats.collisions++;
> +
> +       if (report & TX_PACKET_DROPPED)
> +               dev->stats.tx_dropped++;
> +
> +       if (report & TX_FIFO_UNDERRUN)
> +               dev->stats.tx_fifo_errors++;
> +
> +       dev->stats.tx_errors++;
> +}
> +
> +static void nb8800_tx_done(struct net_device *dev)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       int limit = priv->tx_next;
> +       int done = priv->tx_done;
> +       unsigned int packets = 0;
> +       unsigned int len = 0;
> +
> +       while (done != limit) {
> +               struct nb8800_tx_desc *txd = &priv->tx_descs[done];
> +               struct nb8800_tx_buf *txb = &priv->tx_bufs[done];
> +               struct sk_buff *skb;
> +
> +               if (!txd->report)
> +                       break;
> +
> +               skb = txb->skb;
> +               len += skb->len;
> +
> +               dma_unmap_single(&dev->dev, txb->dma_addr, txb->dma_len,
> +                                DMA_TO_DEVICE);
> +
> +               if (IS_TX_ERROR(txd->report)) {
> +                       nb8800_tx_error(dev, txd->report);
> +                       dev_kfree_skb_irq(skb);
> +               } else {
> +                       dev_consume_skb_irq(skb);
> +               }
> +
> +               dev->stats.tx_packets++;
> +               dev->stats.tx_bytes += TX_BYTES_TRANSFERRED(txd->report);
> +               dev->stats.collisions += TX_EARLY_COLLISIONS(txd->report);
> +
> +               txb->skb = NULL;
> +               txb->ready = false;
> +               txd->report = 0;
> +
> +               done = (done + 1) % TX_DESC_COUNT;
> +               packets++;
> +       }
> +
> +       if (packets) {
> +               smp_mb__before_atomic();
> +               atomic_add(packets, &priv->tx_free);
> +               netdev_completed_queue(dev, packets, len);
> +               netif_wake_queue(dev);
> +               priv->tx_done = done;
> +       }
> +}
> +
> +static irqreturn_t nb8800_irq(int irq, void *dev_id)
> +{
> +       struct net_device *dev = dev_id;
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       u32 val;
> +
> +       /* tx interrupt */
> +       val = nb8800_readl(priv, NB8800_TXC_SR);
> +       if (val) {
> +               nb8800_writel(priv, NB8800_TXC_SR, val);
> +
> +               if (val & TSR_DI)
> +                       nb8800_tx_dma_start_irq(dev);
> +
> +               if (val & TSR_TI)
> +                       nb8800_tx_done(dev);
> +
> +               if (unlikely(val & TSR_DE))
> +                       netdev_err(dev, "TX DMA error\n");
> +
> +               /* should never happen with automatic status retrieval */
> +               if (unlikely(val & TSR_TO))
> +                       netdev_err(dev, "TX Status FIFO overflow\n");
> +       }
> +
> +       /* rx interrupt */
> +       val = nb8800_readl(priv, NB8800_RXC_SR);
> +       if (val) {
> +               nb8800_writel(priv, NB8800_RXC_SR, val);
> +
> +               if (likely(val & (RSR_RI | RSR_DI))) {
> +                       nb8800_writel(priv, NB8800_RX_ITR, priv->rx_itr_poll);
> +                       napi_schedule_irqoff(&priv->napi);
> +               }
> +
> +               if (unlikely(val & RSR_DE))
> +                       netdev_err(dev, "RX DMA error\n");
> +
> +               /* should never happen with automatic status retrieval */
> +               if (unlikely(val & RSR_RO))
> +                       netdev_err(dev, "RX Status FIFO overflow\n");
> +       }
> +
> +       return IRQ_HANDLED;
> +}
> +
> +static void nb8800_mac_config(struct net_device *dev)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       bool gigabit = priv->speed == SPEED_1000;
> +       u32 mac_mode_mask = RGMII_MODE | HALF_DUPLEX | GMAC_MODE;
> +       u32 mac_mode = 0;
> +       u32 slot_time;
> +       u32 phy_clk;
> +       u32 ict;
> +
> +       if (!priv->duplex)
> +               mac_mode |= HALF_DUPLEX;
> +
> +       if (gigabit) {
> +               if (priv->phy_mode == PHY_INTERFACE_MODE_RGMII)
> +                       mac_mode |= RGMII_MODE;
> +
> +               mac_mode |= GMAC_MODE;
> +               phy_clk = 125000000;
> +
> +               /* Should be 512 but register is only 8 bits */
> +               slot_time = 255;
> +       } else {
> +               phy_clk = 25000000;
> +               slot_time = 128;
> +       }
> +
> +       ict = DIV_ROUND_UP(phy_clk, clk_get_rate(priv->clk));
> +
> +       nb8800_writeb(priv, NB8800_IC_THRESHOLD, ict);
> +       nb8800_writeb(priv, NB8800_SLOT_TIME, slot_time);
> +       nb8800_maskb(priv, NB8800_MAC_MODE, mac_mode_mask, mac_mode);
> +}
> +
> +static void nb8800_pause_config(struct net_device *dev)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       struct phy_device *phydev = priv->phydev;
> +       u32 rxcr;
> +
> +       if (priv->pause_aneg) {
> +               if (!phydev || !phydev->link)
> +                       return;
> +
> +               priv->pause_rx = phydev->pause;
> +               priv->pause_tx = phydev->pause ^ phydev->asym_pause;
> +       }
> +
> +       nb8800_modb(priv, NB8800_RX_CTL, RX_PAUSE_EN, priv->pause_rx);
> +
> +       rxcr = nb8800_readl(priv, NB8800_RXC_CR);
> +       if (!!(rxcr & RCR_FL) == priv->pause_tx)
> +               return;
> +
> +       if (netif_running(dev)) {
> +               napi_disable(&priv->napi);
> +               netif_tx_lock_bh(dev);
> +               nb8800_dma_stop(dev);
> +               nb8800_modl(priv, NB8800_RXC_CR, RCR_FL, priv->pause_tx);
> +               nb8800_start_rx(dev);
> +               netif_tx_unlock_bh(dev);
> +               napi_enable(&priv->napi);
> +       } else {
> +               nb8800_modl(priv, NB8800_RXC_CR, RCR_FL, priv->pause_tx);
> +       }
> +}
> +
> +static void nb8800_link_reconfigure(struct net_device *dev)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       struct phy_device *phydev = priv->phydev;
> +       int change = 0;
> +
> +       if (phydev->link) {
> +               if (phydev->speed != priv->speed) {
> +                       priv->speed = phydev->speed;
> +                       change = 1;
> +               }
> +
> +               if (phydev->duplex != priv->duplex) {
> +                       priv->duplex = phydev->duplex;
> +                       change = 1;
> +               }
> +
> +               if (change)
> +                       nb8800_mac_config(dev);
> +
> +               nb8800_pause_config(dev);
> +       }
> +
> +       if (phydev->link != priv->link) {
> +               priv->link = phydev->link;
> +               change = 1;
> +       }
> +
> +       if (change)
> +               phy_print_status(priv->phydev);
> +}
> +
> +static void nb8800_update_mac_addr(struct net_device *dev)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       int i;
> +
> +       for (i = 0; i < ETH_ALEN; i++)
> +               nb8800_writeb(priv, NB8800_SRC_ADDR(i), dev->dev_addr[i]);
> +
> +       for (i = 0; i < ETH_ALEN; i++)
> +               nb8800_writeb(priv, NB8800_UC_ADDR(i), dev->dev_addr[i]);
> +}
> +
> +static int nb8800_set_mac_address(struct net_device *dev, void *addr)
> +{
> +       struct sockaddr *sock = addr;
> +
> +       if (netif_running(dev))
> +               return -EBUSY;
> +
> +       ether_addr_copy(dev->dev_addr, sock->sa_data);
> +       nb8800_update_mac_addr(dev);
> +
> +       return 0;
> +}
> +
> +static void nb8800_mc_init(struct net_device *dev, int val)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +
> +       nb8800_writeb(priv, NB8800_MC_INIT, val);
> +       readb_poll_timeout_atomic(priv->base + NB8800_MC_INIT, val, !val,
> +                                 1, 1000);
> +}
> +
> +static void nb8800_set_rx_mode(struct net_device *dev)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       struct netdev_hw_addr *ha;
> +       int i;
> +
> +       if (dev->flags & (IFF_PROMISC | IFF_ALLMULTI)) {
> +               nb8800_mac_af(dev, false);
> +               return;
> +       }
> +
> +       nb8800_mac_af(dev, true);
> +       nb8800_mc_init(dev, 0);
> +
> +       netdev_for_each_mc_addr(ha, dev) {
> +               for (i = 0; i < ETH_ALEN; i++)
> +                       nb8800_writeb(priv, NB8800_MC_ADDR(i), ha->addr[i]);
> +
> +               nb8800_mc_init(dev, 0xff);
> +       }
> +}
> +
> +#define RX_DESC_SIZE (RX_DESC_COUNT * sizeof(struct nb8800_rx_desc))
> +#define TX_DESC_SIZE (TX_DESC_COUNT * sizeof(struct nb8800_tx_desc))
> +
> +static void nb8800_dma_free(struct net_device *dev)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       int i;

looks like unsigned int i;

> +
> +       if (priv->rx_bufs) {
> +               for (i = 0; i < RX_DESC_COUNT; i++)
> +                       if (priv->rx_bufs[i].page)
> +                               put_page(priv->rx_bufs[i].page);
> +
> +               kfree(priv->rx_bufs);
> +               priv->rx_bufs = NULL;
> +       }
> +
> +       if (priv->tx_bufs) {
> +               for (i = 0; i < TX_DESC_COUNT; i++)
> +                       kfree_skb(priv->tx_bufs[i].skb);
> +
> +               kfree(priv->tx_bufs);
> +               priv->tx_bufs = NULL;
> +       }
> +
> +       if (priv->rx_descs) {
> +               dma_free_coherent(dev->dev.parent, RX_DESC_SIZE, priv->rx_descs,
> +                                 priv->rx_desc_dma);
> +               priv->rx_descs = NULL;
> +       }
> +
> +       if (priv->tx_descs) {
> +               dma_free_coherent(dev->dev.parent, TX_DESC_SIZE, priv->tx_descs,
> +                                 priv->tx_desc_dma);
> +               priv->tx_descs = NULL;
> +       }
> +}
> +
> +static void nb8800_dma_reset(struct net_device *dev)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       struct nb8800_rx_desc *rxd;
> +       struct nb8800_tx_desc *txd;
> +       int i;

Ditto.

> +
> +       for (i = 0; i < RX_DESC_COUNT; i++) {
> +               dma_addr_t rx_dma = priv->rx_desc_dma + i * sizeof(*rxd);
> +
> +               rxd = &priv->rx_descs[i];
> +               rxd->desc.n_addr = rx_dma + sizeof(*rxd);
> +               rxd->desc.r_addr =
> +                       rx_dma + offsetof(struct nb8800_rx_desc, report);
> +               rxd->desc.config = priv->rx_dma_config;
> +               rxd->report = 0;
> +       }
> +
> +       rxd->desc.n_addr = priv->rx_desc_dma;
> +       rxd->desc.config |= DESC_EOC;
> +
> +       priv->rx_eoc = RX_DESC_COUNT - 1;
> +
> +       for (i = 0; i < TX_DESC_COUNT; i++) {
> +               struct nb8800_tx_buf *txb = &priv->tx_bufs[i];
> +               dma_addr_t r_dma = txb->dma_desc +
> +                       offsetof(struct nb8800_tx_desc, report);
> +
> +               txd = &priv->tx_descs[i];
> +               txd->desc[0].r_addr = r_dma;
> +               txd->desc[1].r_addr = r_dma;
> +               txd->report = 0;
> +       }
> +
> +       priv->tx_next = 0;
> +       priv->tx_queue = 0;
> +       priv->tx_done = 0;
> +       atomic_set(&priv->tx_free, TX_DESC_COUNT);
> +
> +       nb8800_writel(priv, NB8800_RX_DESC_ADDR, priv->rx_desc_dma);
> +
> +       wmb();          /* ensure all setup is written before starting */
> +}
> +
> +static int nb8800_dma_init(struct net_device *dev)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       int n_rx = RX_DESC_COUNT;
> +       int n_tx = TX_DESC_COUNT;
> +       int err;
> +       int i;

Ditto.

> +
> +       priv->rx_descs = dma_alloc_coherent(dev->dev.parent, RX_DESC_SIZE,
> +                                           &priv->rx_desc_dma, GFP_KERNEL);
> +       if (!priv->rx_descs)
> +               goto err_out;
> +
> +       priv->rx_bufs = kcalloc(n_rx, sizeof(*priv->rx_bufs), GFP_KERNEL);
> +       if (!priv->rx_bufs)
> +               goto err_out;
> +
> +       for (i = 0; i < n_rx; i++) {
> +               err = nb8800_alloc_rx(dev, i, false);
> +               if (err)
> +                       goto err_out;
> +       }
> +
> +       priv->tx_descs = dma_alloc_coherent(dev->dev.parent, TX_DESC_SIZE,
> +                                           &priv->tx_desc_dma, GFP_KERNEL);
> +       if (!priv->tx_descs)
> +               goto err_out;
> +
> +       priv->tx_bufs = kcalloc(n_tx, sizeof(*priv->tx_bufs), GFP_KERNEL);
> +       if (!priv->tx_bufs)
> +               goto err_out;
> +
> +       for (i = 0; i < n_tx; i++)
> +               priv->tx_bufs[i].dma_desc =
> +                       priv->tx_desc_dma + i * sizeof(struct nb8800_tx_desc);
> +
> +       nb8800_dma_reset(dev);
> +
> +       return 0;
> +
> +err_out:
> +       nb8800_dma_free(dev);
> +
> +       return -ENOMEM;
> +}
> +
> +static int nb8800_dma_stop(struct net_device *dev)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       struct nb8800_tx_buf *txb = &priv->tx_bufs[0];
> +       struct nb8800_tx_desc *txd = &priv->tx_descs[0];
> +       int retry = 5;
> +       u32 txcr;
> +       u32 rxcr;
> +       int err;
> +       int i;

Ditto.

> +
> +       /* wait for tx to finish */
> +       err = readl_poll_timeout_atomic(priv->base + NB8800_TXC_CR, txcr,
> +                                       !(txcr & TCR_EN) &&
> +                                       priv->tx_done == priv->tx_next,
> +                                       1000, 1000000);
> +       if (err)
> +               return err;
> +
> +       /* The rx DMA only stops if it reaches the end of chain.
> +        * To make this happen, we set the EOC flag on all rx
> +        * descriptors, put the device in loopback mode, and send
> +        * a few dummy frames.  The interrupt handler will ignore
> +        * these since NAPI is disabled and no real frames are in
> +        * the tx queue. */

Block comment
/*
 * text
 */

> +
> +       for (i = 0; i < RX_DESC_COUNT; i++)
> +               priv->rx_descs[i].desc.config |= DESC_EOC;
> +
> +       txd->desc[0].s_addr =
> +               txb->dma_desc + offsetof(struct nb8800_tx_desc, buf);
> +       txd->desc[0].config = DESC_BTS(2) | DESC_DS | DESC_EOF | DESC_EOC | 8;
> +       memset(txd->buf, 0, sizeof(txd->buf));
> +
> +       nb8800_mac_af(dev, false);
> +       nb8800_setb(priv, NB8800_MAC_MODE, LOOPBACK_EN);
> +
> +       do {
> +               nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
> +               wmb();
> +               nb8800_writel(priv, NB8800_TXC_CR, txcr | TCR_EN);
> +
> +               err = readl_poll_timeout_atomic(priv->base + NB8800_RXC_CR,
> +                                               rxcr, !(rxcr & RCR_EN),
> +                                               1000, 100000);
> +       } while (err && --retry);
> +
> +       nb8800_mac_af(dev, true);
> +       nb8800_clearb(priv, NB8800_MAC_MODE, LOOPBACK_EN);
> +       nb8800_dma_reset(dev);
> +
> +       return retry ? 0 : -ETIMEDOUT;
> +}
> +
> +static void nb8800_pause_adv(struct net_device *dev)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       u32 adv = 0;
> +
> +       if (!priv->phydev)
> +               return;
> +
> +       if (priv->pause_rx)
> +               adv |= ADVERTISED_Pause | ADVERTISED_Asym_Pause;
> +       if (priv->pause_tx)
> +               adv ^= ADVERTISED_Asym_Pause;
> +
> +       priv->phydev->supported |= adv;
> +       priv->phydev->advertising |= adv;
> +
> +}
> +
> +static int nb8800_open(struct net_device *dev)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       int err;
> +
> +       /* clear any pending interrupts */
> +       nb8800_writel(priv, NB8800_RXC_SR, 0xf);
> +       nb8800_writel(priv, NB8800_TXC_SR, 0xf);
> +
> +       err = nb8800_dma_init(dev);
> +       if (err)
> +               return err;
> +
> +       err = request_irq(dev->irq, nb8800_irq, 0, dev_name(&dev->dev), dev);
> +       if (err)
> +               goto err_free_dma;
> +
> +       nb8800_mac_rx(dev, true);
> +       nb8800_mac_tx(dev, true);
> +
> +       priv->phydev = of_phy_connect(dev, priv->phy_node,
> +                                     nb8800_link_reconfigure, 0,
> +                                     priv->phy_mode);
> +       if (!priv->phydev)
> +               goto err_free_irq;
> +
> +       nb8800_pause_adv(dev);
> +
> +       netdev_reset_queue(dev);
> +       napi_enable(&priv->napi);
> +       netif_start_queue(dev);
> +
> +       nb8800_start_rx(dev);
> +       phy_start(priv->phydev);
> +
> +       return 0;
> +
> +err_free_irq:
> +       free_irq(dev->irq, dev);
> +err_free_dma:
> +       nb8800_dma_free(dev);
> +
> +       return err;
> +}
> +
> +static int nb8800_stop(struct net_device *dev)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +
> +       phy_stop(priv->phydev);
> +
> +       netif_stop_queue(dev);
> +       napi_disable(&priv->napi);
> +
> +       nb8800_dma_stop(dev);
> +       nb8800_mac_rx(dev, false);
> +       nb8800_mac_tx(dev, false);
> +
> +       phy_disconnect(priv->phydev);
> +       priv->phydev = NULL;
> +
> +       free_irq(dev->irq, dev);
> +
> +       nb8800_dma_free(dev);
> +
> +       return 0;
> +}
> +
> +static int nb8800_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +
> +       return phy_mii_ioctl(priv->phydev, rq, cmd);
> +}
> +
> +static const struct net_device_ops nb8800_netdev_ops = {
> +       .ndo_open               = nb8800_open,
> +       .ndo_stop               = nb8800_stop,
> +       .ndo_start_xmit         = nb8800_xmit,
> +       .ndo_set_mac_address    = nb8800_set_mac_address,
> +       .ndo_set_rx_mode        = nb8800_set_rx_mode,
> +       .ndo_do_ioctl           = nb8800_ioctl,
> +       .ndo_change_mtu         = eth_change_mtu,
> +       .ndo_validate_addr      = eth_validate_addr,
> +};
> +
> +static int nb8800_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +
> +       if (!priv->phydev)
> +               return -ENODEV;
> +
> +       return phy_ethtool_gset(priv->phydev, cmd);
> +}
> +
> +static int nb8800_set_settings(struct net_device *dev, struct ethtool_cmd *cmd)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +
> +       if (!priv->phydev)
> +               return -ENODEV;
> +
> +       return phy_ethtool_sset(priv->phydev, cmd);
> +}
> +
> +static int nb8800_nway_reset(struct net_device *dev)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +
> +       if (!priv->phydev)
> +               return -ENODEV;
> +
> +       return genphy_restart_aneg(priv->phydev);
> +}
> +
> +static void nb8800_get_pauseparam(struct net_device *dev,
> +                                 struct ethtool_pauseparam *pp)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +
> +       pp->autoneg = priv->pause_aneg;
> +       pp->rx_pause = priv->pause_rx;
> +       pp->tx_pause = priv->pause_tx;
> +}
> +
> +static int nb8800_set_pauseparam(struct net_device *dev,
> +                                struct ethtool_pauseparam *pp)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +
> +       priv->pause_aneg = pp->autoneg;
> +       priv->pause_rx = pp->rx_pause;
> +       priv->pause_tx = pp->tx_pause;
> +
> +       nb8800_pause_adv(dev);
> +
> +       if (!priv->pause_aneg)
> +               nb8800_pause_config(dev);
> +       else if (priv->phydev)
> +               phy_start_aneg(priv->phydev);
> +
> +       return 0;
> +}
> +
> +static const char nb8800_stats_names[][ETH_GSTRING_LEN] = {
> +       "rx_bytes_ok",
> +       "rx_frames_ok",
> +       "rx_undersize_frames",
> +       "rx_fragment_frames",
> +       "rx_64_byte_frames",
> +       "rx_127_byte_frames",
> +       "rx_255_byte_frames",
> +       "rx_511_byte_frames",
> +       "rx_1023_byte_frames",
> +       "rx_max_size_frames",
> +       "rx_oversize_frames",
> +       "rx_bad_fcs_frames",
> +       "rx_broadcast_frames",
> +       "rx_multicast_frames",
> +       "rx_control_frames",
> +       "rx_pause_frames",
> +       "rx_unsup_control_frames",
> +       "rx_align_error_frames",
> +       "rx_overrun_frames",
> +       "rx_jabber_frames",
> +       "rx_bytes",
> +       "rx_frames",
> +
> +       "tx_bytes_ok",
> +       "tx_frames_ok",
> +       "tx_64_byte_frames",
> +       "tx_127_byte_frames",
> +       "tx_255_byte_frames",
> +       "tx_511_byte_frames",
> +       "tx_1023_byte_frames",
> +       "tx_max_size_frames",
> +       "tx_oversize_frames",
> +       "tx_broadcast_frames",
> +       "tx_multicast_frames",
> +       "tx_control_frames",
> +       "tx_pause_frames",
> +       "tx_underrun_frames",
> +       "tx_single_collision_frames",
> +       "tx_multi_collision_frames",
> +       "tx_deferred_collision_frames",
> +       "tx_late_collision_frames",
> +       "tx_excessive_collision_frames",
> +       "tx_bytes",
> +       "tx_frames",
> +       "tx_collisions",
> +};
> +
> +#define NB8800_NUM_STATS ARRAY_SIZE(nb8800_stats_names)
> +
> +static int nb8800_get_sset_count(struct net_device *dev, int sset)
> +{
> +       if (sset == ETH_SS_STATS)
> +               return NB8800_NUM_STATS;
> +
> +       return -EOPNOTSUPP;
> +}
> +
> +static void nb8800_get_strings(struct net_device *dev, u32 sset, u8 *buf)
> +{
> +       if (sset == ETH_SS_STATS)
> +               memcpy(buf, &nb8800_stats_names, sizeof(nb8800_stats_names));
> +}
> +
> +static u32 nb8800_read_stat(struct net_device *dev, int index)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +
> +       nb8800_writeb(priv, NB8800_STAT_INDEX, index);
> +
> +       return nb8800_readl(priv, NB8800_STAT_DATA);
> +}
> +
> +static void nb8800_get_ethtool_stats(struct net_device *dev,
> +                                    struct ethtool_stats *estats, u64 *st)
> +{
> +       u32 rx, tx;
> +       int i;

unsigned int i;

> +
> +       for (i = 0; i < NB8800_NUM_STATS / 2; i++) {
> +               rx = nb8800_read_stat(dev, i);
> +               tx = nb8800_read_stat(dev, i | 0x80);
> +               st[i] = rx;
> +               st[i + NB8800_NUM_STATS / 2] = tx;
> +       }
> +}
> +
> +static const struct ethtool_ops nb8800_ethtool_ops = {
> +       .get_settings           = nb8800_get_settings,
> +       .set_settings           = nb8800_set_settings,
> +       .nway_reset             = nb8800_nway_reset,
> +       .get_link               = ethtool_op_get_link,
> +       .get_pauseparam         = nb8800_get_pauseparam,
> +       .set_pauseparam         = nb8800_set_pauseparam,
> +       .get_sset_count         = nb8800_get_sset_count,
> +       .get_strings            = nb8800_get_strings,
> +       .get_ethtool_stats      = nb8800_get_ethtool_stats,
> +};
> +
> +static int nb8800_hw_init(struct net_device *dev)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       u32 val;
> +
> +       val = TX_RETRY_EN | TX_PAD_EN | TX_APPEND_FCS;
> +       nb8800_writeb(priv, NB8800_TX_CTL1, val);
> +
> +       /* Collision retry count */
> +       nb8800_writeb(priv, NB8800_TX_CTL2, 5);
> +
> +       val = RX_PAD_STRIP | RX_AF_EN;
> +       nb8800_writeb(priv, NB8800_RX_CTL, val);
> +
> +       /* Chosen by fair dice roll */
> +       nb8800_writeb(priv, NB8800_RANDOM_SEED, 4);
> +
> +       /* TX cycles per deferral period */
> +       nb8800_writeb(priv, NB8800_TX_SDP, 12);
> +
> +       /* The following three threshold values have been
> +        * experimentally determined for good results. */
> +
> +       /* RX/TX FIFO threshold for partial empty (64-bit entries) */
> +       nb8800_writeb(priv, NB8800_PE_THRESHOLD, 0);
> +
> +       /* RX/TX FIFO threshold for partial full (64-bit entries) */
> +       nb8800_writeb(priv, NB8800_PF_THRESHOLD, 255);
> +
> +       /* Buffer size for transmit (64-bit entries) */
> +       nb8800_writeb(priv, NB8800_TX_BUFSIZE, 64);
> +
> +       /* Configure tx DMA */
> +
> +       val = nb8800_readl(priv, NB8800_TXC_CR);
> +       val &= TCR_LE;          /* keep endian setting */
> +       val |= TCR_DM;          /* DMA descriptor mode */
> +       val |= TCR_RS;          /* automatically store tx status  */
> +       val |= TCR_DIE;         /* interrupt on DMA chain completion */
> +       val |= TCR_TFI(7);      /* interrupt after 7 frames transmitted */
> +       val |= TCR_BTS(2);      /* 32-byte bus transaction size */
> +       nb8800_writel(priv, NB8800_TXC_CR, val);
> +
> +       /* TX complete interrupt after 10 ms or 7 frames (see above) */
> +       val = clk_get_rate(priv->clk) / 100;
> +       nb8800_writel(priv, NB8800_TX_ITR, val);
> +
> +       /* Configure rx DMA */
> +
> +       val = nb8800_readl(priv, NB8800_RXC_CR);
> +       val &= RCR_LE;          /* keep endian setting */
> +       val |= RCR_DM;          /* DMA descriptor mode */
> +       val |= RCR_RS;          /* automatically store rx status */
> +       val |= RCR_DIE;         /* interrupt at end of DMA chain */
> +       val |= RCR_RFI(7);      /* interrupt after 7 frames received */
> +       val |= RCR_BTS(2);      /* 32-byte bus transaction size */
> +       nb8800_writel(priv, NB8800_RXC_CR, val);
> +
> +       /* The rx interrupt can fire before the DMA has completed
> +        * unless a small delay is added.  50 us is hopefully enough. */
> +       priv->rx_itr_irq = clk_get_rate(priv->clk) / 20000;
> +
> +       /* In NAPI poll mode we want to disable interrupts, but the
> +        * hardware does not permit this.  Delay 10 ms instead. */
> +       priv->rx_itr_poll = clk_get_rate(priv->clk) / 100;
> +
> +       nb8800_writel(priv, NB8800_RX_ITR, priv->rx_itr_irq);
> +
> +       priv->rx_dma_config = RX_BUF_SIZE | DESC_BTS(2) | DESC_DS | DESC_EOF;
> +
> +       /* Flow control settings */
> +
> +       /* Pause time of 0.1 ms */
> +       val = 100000 / 512;
> +       nb8800_writeb(priv, NB8800_PQ1, val >> 8);
> +       nb8800_writeb(priv, NB8800_PQ2, val & 0xff);
> +
> +       /* Auto-negotiate by default */
> +       priv->pause_aneg = true;
> +       priv->pause_rx = true;
> +       priv->pause_tx = true;
> +
> +       nb8800_mc_init(dev, 0);
> +
> +       return 0;
> +}
> +
> +static int nb8800_tangox_init(struct net_device *dev)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       u32 pad_mode = PAD_MODE_MII;
> +
> +       switch (priv->phy_mode) {
> +       case PHY_INTERFACE_MODE_MII:
> +       case PHY_INTERFACE_MODE_GMII:
> +               pad_mode = PAD_MODE_MII;
> +               break;
> +
> +       case PHY_INTERFACE_MODE_RGMII:
> +               pad_mode = PAD_MODE_RGMII;
> +               break;
> +
> +       case PHY_INTERFACE_MODE_RGMII_TXID:
> +               pad_mode = PAD_MODE_RGMII | PAD_MODE_GTX_CLK_DELAY;
> +               break;
> +
> +       default:
> +               dev_err(dev->dev.parent, "unsupported phy mode %s\n",
> +                       phy_modes(priv->phy_mode));
> +               return -EINVAL;
> +       }
> +
> +       nb8800_writeb(priv, NB8800_TANGOX_PAD_MODE, pad_mode);
> +
> +       return 0;
> +}
> +
> +static int nb8800_tangox_reset(struct net_device *dev)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       int clk_div;
> +
> +       nb8800_writeb(priv, NB8800_TANGOX_RESET, 0);
> +       usleep_range(1000, 10000);
> +       nb8800_writeb(priv, NB8800_TANGOX_RESET, 1);
> +
> +       wmb();          /* ensure reset is cleared before proceeding */
> +
> +       clk_div = DIV_ROUND_UP(clk_get_rate(priv->clk), 2 * MAX_MDC_CLOCK);
> +       nb8800_writew(priv, NB8800_TANGOX_MDIO_CLKDIV, clk_div);
> +
> +       return 0;
> +}
> +
> +static const struct nb8800_ops nb8800_tangox_ops = {
> +       .init   = nb8800_tangox_init,
> +       .reset  = nb8800_tangox_reset,
> +};
> +
> +static int nb8800_tango4_init(struct net_device *dev)
> +{
> +       struct nb8800_priv *priv = netdev_priv(dev);
> +       int err;
> +
> +       err = nb8800_tangox_init(dev);
> +       if (err)
> +               return err;
> +
> +       /* On tango4 interrupt on DMA completion per frame works and gives
> +        * better performance despite generating more rx interrupts. */
> +
> +       /* Disable unnecessary interrupt on rx completion */
> +       nb8800_clearl(priv, NB8800_RXC_CR, RCR_RFI(7));
> +
> +       /* Request interrupt on descriptor DMA completion */
> +       priv->rx_dma_config |= DESC_ID;
> +
> +       return 0;
> +}
> +
> +static const struct nb8800_ops nb8800_tango4_ops = {
> +       .init   = nb8800_tango4_init,
> +       .reset  = nb8800_tangox_reset,
> +};
> +
> +static const struct of_device_id nb8800_dt_ids[] = {
> +       {
> +               .compatible = "aurora,nb8800",
> +       },
> +       {
> +               .compatible = "sigma,smp8642-ethernet",
> +               .data = &nb8800_tangox_ops,
> +       },
> +       {
> +               .compatible = "sigma,smp8734-ethernet",
> +               .data = &nb8800_tango4_ops,
> +       },
> +       { }
> +};
> +
> +static int nb8800_probe(struct platform_device *pdev)
> +{
> +       const struct of_device_id *match;
> +       const struct nb8800_ops *ops = NULL;
> +       struct nb8800_priv *priv;
> +       struct resource *res;
> +       struct net_device *dev;
> +       struct mii_bus *bus;
> +       const unsigned char *mac;
> +       void __iomem *base;
> +       int irq;
> +       int ret;
> +
> +       match = of_match_device(nb8800_dt_ids, &pdev->dev);
> +       if (match)
> +               ops = match->data;
> +
> +       irq = platform_get_irq(pdev, 0);
> +       if (irq <= 0) {
> +               dev_err(&pdev->dev, "No IRQ\n");
> +               return -EINVAL;
> +       }
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(base))
> +               return PTR_ERR(base);
> +
> +       dev_dbg(&pdev->dev, "AU-NB8800 Ethernet at %pa\n", &res->start);
> +
> +       dev = alloc_etherdev(sizeof(*priv));
> +       if (!dev)
> +               return -ENOMEM;
> +
> +       platform_set_drvdata(pdev, dev);
> +       SET_NETDEV_DEV(dev, &pdev->dev);
> +
> +       priv = netdev_priv(dev);
> +       priv->base = base;
> +
> +       priv->phy_mode = of_get_phy_mode(pdev->dev.of_node);
> +       if (priv->phy_mode < 0)
> +               priv->phy_mode = PHY_INTERFACE_MODE_RGMII;
> +
> +       priv->clk = devm_clk_get(&pdev->dev, NULL);
> +       if (IS_ERR(priv->clk)) {
> +               dev_err(&pdev->dev, "failed to get clock\n");
> +               ret = PTR_ERR(priv->clk);
> +               goto err_free_dev;
> +       }
> +
> +       ret = clk_prepare_enable(priv->clk);
> +       if (ret)
> +               goto err_free_dev;
> +
> +       spin_lock_init(&priv->tx_lock);
> +
> +       if (ops && ops->reset) {
> +               ret = ops->reset(dev);
> +               if (ret)
> +                       goto err_free_dev;
> +       }
> +
> +       bus = devm_mdiobus_alloc(&pdev->dev);
> +       if (!bus) {
> +               ret = -ENOMEM;
> +               goto err_disable_clk;
> +       }
> +
> +       bus->name = "nb8800-mii";
> +       bus->read = nb8800_mdio_read;
> +       bus->write = nb8800_mdio_write;
> +       bus->parent = &pdev->dev;
> +       snprintf(bus->id, MII_BUS_ID_SIZE, "%lx.nb8800-mii",
> +                (unsigned long)res->start);
> +       bus->priv = priv;
> +
> +       ret = of_mdiobus_register(bus, pdev->dev.of_node);
> +       if (ret) {
> +               dev_err(&pdev->dev, "failed to register MII bus\n");
> +               goto err_disable_clk;
> +       }
> +
> +       priv->phy_node = of_parse_phandle(pdev->dev.of_node, "phy-handle", 0);
> +       if (!priv->phy_node) {
> +               dev_err(&pdev->dev, "no PHY specified\n");
> +               ret = -ENODEV;
> +               goto err_free_bus;
> +       }
> +
> +       priv->mii_bus = bus;
> +
> +       ret = nb8800_hw_init(dev);
> +       if (ret)
> +               goto err_free_bus;
> +
> +       if (ops && ops->init) {
> +               ret = ops->init(dev);
> +               if (ret)
> +                       goto err_free_bus;
> +       }
> +
> +       dev->netdev_ops = &nb8800_netdev_ops;
> +       dev->ethtool_ops = &nb8800_ethtool_ops;
> +       dev->flags |= IFF_MULTICAST;
> +       dev->irq = irq;
> +
> +       mac = of_get_mac_address(pdev->dev.of_node);
> +       if (mac)
> +               ether_addr_copy(dev->dev_addr, mac);
> +
> +       if (!is_valid_ether_addr(dev->dev_addr))
> +               eth_hw_addr_random(dev);
> +
> +       nb8800_update_mac_addr(dev);
> +
> +       netif_carrier_off(dev);
> +
> +       ret = register_netdev(dev);
> +       if (ret) {
> +               netdev_err(dev, "failed to register netdev\n");
> +               goto err_free_dma;
> +       }
> +
> +       netif_napi_add(dev, &priv->napi, nb8800_poll, NAPI_POLL_WEIGHT);
> +
> +       netdev_info(dev, "MAC address %pM\n", dev->dev_addr);
> +
> +       return 0;
> +
> +err_free_dma:
> +       nb8800_dma_free(dev);
> +err_free_bus:
> +       mdiobus_unregister(bus);
> +err_disable_clk:
> +       clk_disable_unprepare(priv->clk);
> +err_free_dev:
> +       free_netdev(dev);
> +
> +       return ret;
> +}
> +
> +static int nb8800_remove(struct platform_device *pdev)
> +{
> +       struct net_device *ndev = platform_get_drvdata(pdev);
> +       struct nb8800_priv *priv = netdev_priv(ndev);
> +
> +       unregister_netdev(ndev);
> +
> +       mdiobus_unregister(priv->mii_bus);
> +
> +       clk_disable_unprepare(priv->clk);
> +
> +       nb8800_dma_free(ndev);
> +       free_netdev(ndev);
> +
> +       return 0;
> +}
> +
> +static struct platform_driver nb8800_driver = {
> +       .driver = {
> +               .name           = "nb8800",
> +               .of_match_table = nb8800_dt_ids,
> +       },
> +       .probe  = nb8800_probe,
> +       .remove = nb8800_remove,
> +};
> +
> +module_platform_driver(nb8800_driver);
> +
> +MODULE_DESCRIPTION("Aurora AU-NB8800 Ethernet driver");
> +MODULE_AUTHOR("Mans Rullgard <mans@mansr.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/net/ethernet/aurora/nb8800.h b/drivers/net/ethernet/aurora/nb8800.h
> new file mode 100644
> index 0000000..e108d01
> --- /dev/null
> +++ b/drivers/net/ethernet/aurora/nb8800.h
> @@ -0,0 +1,314 @@
> +#ifndef _NB8800_H_
> +#define _NB8800_H_
> +
> +#include <linux/types.h>
> +#include <linux/skbuff.h>
> +#include <linux/phy.h>
> +#include <linux/clk.h>
> +#include <linux/bitops.h>
> +
> +#define RX_DESC_COUNT                  256
> +#define TX_DESC_COUNT                  256
> +
> +#define NB8800_DESC_LOW                        4
> +
> +#define RX_BUF_SIZE                    1552
> +
> +#define RX_COPYBREAK                   256
> +#define RX_COPYHDR                     128
> +
> +#define MAX_MDC_CLOCK                  2500000
> +
> +/* Stargate Solutions SSN8800 core registers */
> +#define NB8800_TX_CTL1                 0x000
> +#define TX_TPD                         BIT(5)
> +#define TX_APPEND_FCS                  BIT(4)
> +#define TX_PAD_EN                      BIT(3)
> +#define TX_RETRY_EN                    BIT(2)
> +#define TX_EN                          BIT(0)
> +
> +#define NB8800_TX_CTL2                 0x001
> +
> +#define NB8800_RX_CTL                  0x004
> +#define RX_BC_DISABLE                  BIT(7)
> +#define RX_RUNT                                BIT(6)
> +#define RX_AF_EN                       BIT(5)
> +#define RX_PAUSE_EN                    BIT(3)
> +#define RX_SEND_CRC                    BIT(2)
> +#define RX_PAD_STRIP                   BIT(1)
> +#define RX_EN                          BIT(0)
> +
> +#define NB8800_RANDOM_SEED             0x008
> +#define NB8800_TX_SDP                  0x14
> +#define NB8800_TX_TPDP1                        0x18
> +#define NB8800_TX_TPDP2                        0x19
> +#define NB8800_SLOT_TIME               0x1c
> +
> +#define NB8800_MDIO_CMD                        0x020

> +#define MIIAR_ADDR(x)                  ((x) << 21)
> +#define MIIAR_REG(x)                   ((x) << 16)
> +#define MIIAR_DATA(x)                  ((x) <<  0)
> +#define MDIO_CMD_GO                    BIT(31)
> +#define MDIO_CMD_WR                    BIT(26)

Perhaps sort them by bits used?

> +
> +#define NB8800_MDIO_STS                        0x024
> +#define MDIO_STS_ERR                   BIT(31)
> +
> +#define NB8800_MC_ADDR(i)              (0x028 + (i))
> +#define NB8800_MC_INIT                 0x02e
> +#define NB8800_UC_ADDR(i)              (0x03c + (i))
> +
> +#define NB8800_MAC_MODE                        0x044
> +#define RGMII_MODE                     BIT(7)
> +#define HALF_DUPLEX                    BIT(4)
> +#define BURST_EN                       BIT(3)
> +#define LOOPBACK_EN                    BIT(2)
> +#define GMAC_MODE                      BIT(0)
> +
> +#define NB8800_IC_THRESHOLD            0x050
> +#define NB8800_PE_THRESHOLD            0x051
> +#define NB8800_PF_THRESHOLD            0x052
> +#define NB8800_TX_BUFSIZE              0x054
> +#define NB8800_FIFO_CTL                        0x056
> +#define NB8800_PQ1                     0x060
> +#define NB8800_PQ2                     0x061
> +#define NB8800_SRC_ADDR(i)             (0x06a + (i))
> +#define NB8800_STAT_DATA               0x078
> +#define NB8800_STAT_INDEX              0x07c
> +#define NB8800_STAT_CLEAR              0x07d
> +
> +#define NB8800_SLEEP_MODE              0x07e
> +#define SLEEP_MODE                     BIT(0)
> +
> +#define NB8800_WAKEUP                  0x07f
> +#define WAKEUP                         BIT(0)
> +
> +/* Aurora NB8800 host interface registers */
> +#define NB8800_TXC_CR                  0x100
> +#define TCR_LK                         BIT(12)
> +#define TCR_DS                         BIT(11)
> +#define TCR_BTS(x)                     (((x) & 0x7) << 8)
> +#define TCR_DIE                                BIT(7)
> +#define TCR_TFI(x)                     (((x) & 0x7) << 4)
> +#define TCR_LE                         BIT(3)
> +#define TCR_RS                         BIT(2)
> +#define TCR_DM                         BIT(1)
> +#define TCR_EN                         BIT(0)
> +
> +#define NB8800_TXC_SR                  0x104
> +#define TSR_DE                         BIT(3)
> +#define TSR_DI                         BIT(2)
> +#define TSR_TO                         BIT(1)
> +#define TSR_TI                         BIT(0)
> +
> +#define NB8800_TX_SAR                  0x108
> +#define NB8800_TX_DESC_ADDR            0x10c
> +
> +#define NB8800_TX_REPORT_ADDR          0x110
> +#define TX_BYTES_TRANSFERRED(x)                (((x) >> 16) & 0xffff)
> +#define TX_FIRST_DEFERRAL              BIT(7)
> +#define TX_EARLY_COLLISIONS(x)         (((x) >> 3) & 0xf)
> +#define TX_LATE_COLLISION              BIT(2)
> +#define TX_PACKET_DROPPED              BIT(1)
> +#define TX_FIFO_UNDERRUN               BIT(0)
> +#define IS_TX_ERROR(r)                 ((r) & 0x07)
> +
> +#define NB8800_TX_FIFO_SR              0x114
> +#define NB8800_TX_ITR                  0x118
> +
> +#define NB8800_RXC_CR                  0x200
> +#define RCR_FL                         BIT(13)
> +#define RCR_LK                         BIT(12)
> +#define RCR_DS                         BIT(11)
> +#define RCR_BTS(x)                     (((x) & 7) << 8)
> +#define RCR_DIE                                BIT(7)
> +#define RCR_RFI(x)                     (((x) & 7) << 4)
> +#define RCR_LE                         BIT(3)
> +#define RCR_RS                         BIT(2)
> +#define RCR_DM                         BIT(1)
> +#define RCR_EN                         BIT(0)
> +
> +#define NB8800_RXC_SR                  0x204
> +#define RSR_DE                         BIT(3)
> +#define RSR_DI                         BIT(2)
> +#define RSR_RO                         BIT(1)
> +#define RSR_RI                         BIT(0)
> +
> +#define NB8800_RX_SAR                  0x208
> +#define NB8800_RX_DESC_ADDR            0x20c
> +
> +#define NB8800_RX_REPORT_ADDR          0x210
> +#define RX_BYTES_TRANSFERRED(x)                (((x) >> 16) & 0xFFFF)
> +#define RX_MULTICAST_PKT               BIT(9)
> +#define RX_BROADCAST_PKT               BIT(8)
> +#define RX_LENGTH_ERR                  BIT(7)
> +#define RX_FCS_ERR                     BIT(6)
> +#define RX_RUNT_PKT                    BIT(5)
> +#define RX_FIFO_OVERRUN                        BIT(4)
> +#define RX_LATE_COLLISION              BIT(3)
> +#define RX_ALIGNMENT_ERROR             BIT(2)
> +#define RX_ERROR_MASK                  0xfc
> +#define IS_RX_ERROR(r)                 ((r) & RX_ERROR_MASK)
> +
> +#define NB8800_RX_FIFO_SR              0x214
> +#define NB8800_RX_ITR                  0x218
> +
> +/* Sigma Designs SMP86xx additional registers */
> +#define NB8800_TANGOX_PAD_MODE         0x400
> +#define PAD_MODE_MASK                  0x7
> +#define PAD_MODE_MII                   0x0
> +#define PAD_MODE_RGMII                 0x1
> +#define PAD_MODE_GTX_CLK_INV           BIT(3)
> +#define PAD_MODE_GTX_CLK_DELAY         BIT(4)
> +
> +#define NB8800_TANGOX_MDIO_CLKDIV      0x420
> +#define NB8800_TANGOX_RESET            0x424
> +
> +/* Hardware DMA descriptor */
> +struct nb8800_dma_desc {
> +       u32                             s_addr; /* start address */
> +       u32                             n_addr; /* next descriptor address */
> +       u32                             r_addr; /* report address */
> +       u32                             config;
> +} __aligned(8);
> +
> +#define DESC_ID                                BIT(23)
> +#define DESC_EOC                       BIT(22)
> +#define DESC_EOF                       BIT(21)
> +#define DESC_LK                                BIT(20)
> +#define DESC_DS                                BIT(19)
> +#define DESC_BTS(x)                    (((x) & 0x7) << 16)
> +
> +/* DMA descriptor and associated data for rx.
> + * Allocated from coherent memory.
> + */
> +struct nb8800_rx_desc {
> +       /* DMA descriptor */
> +       struct nb8800_dma_desc          desc;
> +
> +       /* Status report filled in by hardware */
> +       u32                             report;
> +};
> +
> +/* Address of buffer on rx ring */
> +struct nb8800_rx_buf {
> +       struct page                     *page;
> +       unsigned long                   offset;
> +};
> +
> +/* DMA descriptors and associated data for tx.
> + * Allocated from coherent memory.
> + */

Convert to kernel doc?

> +struct nb8800_tx_desc {
> +       /* DMA descriptor.  The second descriptor is used if packet
> +        * data is unaligned. *


> +       struct nb8800_dma_desc          desc[2];
> +
> +       /* Status report filled in by hardware */
> +       u32                             report;
> +
> +       /* Bounce buffer for initial unaligned part of packet */

> +       u8                              buf[8] __aligned(8);
> +};
> +
> +/* Packet in tx queue */
> +struct nb8800_tx_buf {


> +       /* Currently queued skb */
> +       struct sk_buff                  *skb;
> +
> +       /* DMA address of the first descriptor */
> +       dma_addr_t                      dma_desc;
> +
> +       /* DMA address of packet data */
> +       dma_addr_t                      dma_addr;
> +
> +       /* Length of DMA mapping, less than skb->len if alignment
> +        * buffer is used. */
> +       unsigned int                    dma_len;
> +
> +       /* Number of packets in chain starting here */
> +       unsigned int                    chain_len;
> +
> +       /* Packet chain ready to be submitted to hardware */
> +       bool                            ready;
> +};
> +
> +struct nb8800_priv {

> +       struct napi_struct              napi;
> +
> +       void __iomem                    *base;
> +
> +       /* RX DMA descriptors */
> +       struct nb8800_rx_desc           *rx_descs;
> +
> +       /* RX buffers referenced by DMA descriptors */
> +       struct nb8800_rx_buf            *rx_bufs;
> +
> +       /* Current end of chain */
> +       u32                             rx_eoc;
> +
> +       /* Value for rx interrupt time register in NAPI interrupt mode */
> +       u32                             rx_itr_irq;
> +
> +       /* Value for rx interrupt time register in NAPI poll mode */
> +       u32                             rx_itr_poll;
> +
> +       /* Value for config field of rx DMA descriptors */
> +       u32                             rx_dma_config;
> +
> +       /* TX DMA descriptors */
> +       struct nb8800_tx_desc           *tx_descs;
> +
> +       /* TX packet queue */
> +       struct nb8800_tx_buf            *tx_bufs;
> +
> +       /* Number of free tx queue entries */
> +       atomic_t                        tx_free;
> +
> +       /* First free tx queue entry */
> +       u32                             tx_next;
> +
> +       /* Next buffer to transmit */
> +       u32                             tx_queue;
> +
> +       /* Start of current packet chain */
> +       struct nb8800_tx_buf            *tx_chain;
> +
> +       /* Next buffer to reclaim */
> +       u32                             tx_done;
> +
> +       /* Lock for DMA activation */
> +       spinlock_t                      tx_lock;
> +
> +       struct mii_bus                  *mii_bus;
> +       struct device_node              *phy_node;
> +       struct phy_device               *phydev;
> +
> +       /* PHY connection type from DT */
> +       int                             phy_mode;
> +
> +       /* Current link status */
> +       int                             speed;
> +       int                             duplex;
> +       int                             link;
> +
> +       /* Pause settings */
> +       bool                            pause_aneg;
> +       bool                            pause_rx;
> +       bool                            pause_tx;
> +
> +       /* DMA base address of rx descriptors, see rx_descs above */
> +       dma_addr_t                      rx_desc_dma;
> +
> +       /* DMA base address of tx descriptors, see tx_descs above */
> +       dma_addr_t                      tx_desc_dma;
> +
> +       struct clk                      *clk;
> +};
> +
> +struct nb8800_ops {
> +       int                             (*init)(struct net_device *dev);
> +       int                             (*reset)(struct net_device *dev);
> +};
> +
> +#endif /* _NB8800_H_ */
> --
> 2.6.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-10 22:09 ` Andy Shevchenko
@ 2015-11-10 22:34   ` Måns Rullgård
  2015-11-10 22:40     ` Andy Shevchenko
  0 siblings, 1 reply; 52+ messages in thread
From: Måns Rullgård @ 2015-11-10 22:34 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-kernel, netdev, slash.tmp

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

>> +static inline void nb8800_maskb(struct nb8800_priv *priv, int reg,
>> +                               u32 mask, u32 val)
>> +{
>> +       u32 old = nb8800_readb(priv, reg);
>> +       u32 new = (old & ~mask) | val;
>
> Shoudn't be "… | (val & mask);" ?

No, it's meant to replace the bits in mask with the corresponding bits
from val.

> + empty line?
>
>> +       if (new != old)
>> +               nb8800_writeb(priv, reg, new);
>> +}
>> +

[...]

>> +static void nb8800_receive(struct net_device *dev, int i, int len)
>
> unsigned int i ?
> len as well?

Does it matter?  The values are nowhere near overflowing signed int.


[...]

>> +               /* If a packet arrived after we last checked but
>> +                * before writing RX_ITR, the interrupt will be
>> +                * delayed, so we retrieve it now. */
>
> Block comments usually
> /*
>  * text
>  */

Documentation/CodingStyle says net/ and drivers/net/ are special, though
currently a mix of styles can be found.  Personally, I don't
particularly care.

> Can be longer lines?

Still won't fit on two lines.

>> +               if (priv->rx_descs[next].report)
>> +                       goto again;
>> +
>> +               napi_complete_done(napi, work);
>> +       }
>> +
>> +       return work;
>> +}
>> +
>> +static void nb8800_tx_dma_start(struct net_device *dev)
>> +{
>> +       struct nb8800_priv *priv = netdev_priv(dev);
>> +       struct nb8800_tx_buf *txb;
>> +       u32 txc_cr;
>> +
>> +       txb = &priv->tx_bufs[priv->tx_queue];
>> +       if (!txb->ready)
>> +               return;
>> +
>> +       txc_cr = nb8800_readl(priv, NB8800_TXC_CR);
>> +       if (txc_cr & TCR_EN)
>> +               return;
>> +
>> +       nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
>> +       wmb();          /* ensure desc addr is written before starting DMA */
>
> Hm… Have I missed corresponding rmb() ? If it's about MMIO, perhaps mmiowb() ?

Possibly.

>> +       nb8800_writel(priv, NB8800_TXC_CR, txc_cr | TCR_EN);
>> +
>> +       priv->tx_queue = (priv->tx_queue + txb->chain_len) % TX_DESC_COUNT;
>> +}

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-10 22:34   ` Måns Rullgård
@ 2015-11-10 22:40     ` Andy Shevchenko
  2015-11-10 23:07       ` Måns Rullgård
  0 siblings, 1 reply; 52+ messages in thread
From: Andy Shevchenko @ 2015-11-10 22:40 UTC (permalink / raw)
  To: Måns Rullgård; +Cc: linux-kernel, netdev, slash.tmp

On Wed, Nov 11, 2015 at 12:34 AM, Måns Rullgård <mans@mansr.com> wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
>
>>> +static inline void nb8800_maskb(struct nb8800_priv *priv, int reg,
>>> +                               u32 mask, u32 val)
>>> +{
>>> +       u32 old = nb8800_readb(priv, reg);
>>> +       u32 new = (old & ~mask) | val;
>>
>> Shoudn't be "… | (val & mask);" ?
>
> No, it's meant to replace the bits in mask with the corresponding bits
> from val.

But you unconditionally use entire val value which might bring bits
outside of mask.

>> Block comments usually
>> /*
>>  * text
>>  */
>
> Documentation/CodingStyle says net/ and drivers/net/ are special, though
> currently a mix of styles can be found.  Personally, I don't
> particularly care.

OK.

>>> +       nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
>>> +       wmb();          /* ensure desc addr is written before starting DMA */
>>
>> Hm… Have I missed corresponding rmb() ? If it's about MMIO, perhaps mmiowb() ?
>
> Possibly.

Standalone wmb() doesn't make sense.

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-10 22:40     ` Andy Shevchenko
@ 2015-11-10 23:07       ` Måns Rullgård
  2015-11-11  0:36         ` Andy Shevchenko
  0 siblings, 1 reply; 52+ messages in thread
From: Måns Rullgård @ 2015-11-10 23:07 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-kernel, netdev, slash.tmp

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> On Wed, Nov 11, 2015 at 12:34 AM, Måns Rullgård <mans@mansr.com> wrote:
>> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
>>
>>>> +static inline void nb8800_maskb(struct nb8800_priv *priv, int reg,
>>>> +                               u32 mask, u32 val)
>>>> +{
>>>> +       u32 old = nb8800_readb(priv, reg);
>>>> +       u32 new = (old & ~mask) | val;
>>>
>>> Shoudn't be "… | (val & mask);" ?
>>
>> No, it's meant to replace the bits in mask with the corresponding bits
>> from val.
>
> But you unconditionally use entire val value which might bring bits
> outside of mask.

Very well, I'll apply the mask to both then.

>>>> +       nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
>>>> +       wmb();          /* ensure desc addr is written before starting DMA */
>>>
>>> Hm… Have I missed corresponding rmb() ? If it's about MMIO, perhaps mmiowb() ?
>>
>> Possibly.
>
> Standalone wmb() doesn't make sense.

It does if you need to enforce ordering between normal and I/O memory.
In fact, since the descriptor is filled in using normal memory accesses,
my understanding is that mmiowb() would be insufficient here.  The
comment could be improved, however.

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-10 16:14 [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Mans Rullgard
                   ` (3 preceding siblings ...)
  2015-11-10 22:09 ` Andy Shevchenko
@ 2015-11-10 23:34 ` Francois Romieu
  2015-11-11  0:40   ` Måns Rullgård
  4 siblings, 1 reply; 52+ messages in thread
From: Francois Romieu @ 2015-11-10 23:34 UTC (permalink / raw)
  To: Mans Rullgard; +Cc: linux-kernel, netdev, slash.tmp

Mans Rullgard <mans@mansr.com> :
> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
> new file mode 100644
> index 0000000..11cd389
> --- /dev/null
> +++ b/drivers/net/ethernet/aurora/nb8800.c
[...]
> +static int nb8800_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
[...]
> +
> +	netdev_sent_queue(dev, skb->len);
> +
> +	smp_mb__before_spinlock();
> +	spin_lock_irqsave(&priv->tx_lock, flags);

At some point you may consider performing both Tx and Rx from napi context
and thus replacing priv->tx_lock with netif_tx_lock.

> +
> +	if (!skb->xmit_more) {
> +		priv->tx_chain->ready = true;
> +		priv->tx_chain = NULL;
> +		nb8800_tx_dma_start(dev);
> +	}
> +
> +	spin_unlock_irqrestore(&priv->tx_lock, flags);
> +
> +	priv->tx_next = next;

Are there strong reasons why nb8800_tx_done could not kick between
spin_unlock_irqrestore and the non-local update of priv->tx_next ?

[...]
> +static irqreturn_t nb8800_irq(int irq, void *dev_id)
> +{
> +	struct net_device *dev = dev_id;
> +	struct nb8800_priv *priv = netdev_priv(dev);
> +	u32 val;
> +
> +	/* tx interrupt */
> +	val = nb8800_readl(priv, NB8800_TXC_SR);
> +	if (val) {
[...]
> +	}
> +
> +	/* rx interrupt */
> +	val = nb8800_readl(priv, NB8800_RXC_SR);
> +	if (val) {
[...]
> +	}
> +
> +	return IRQ_HANDLED;

Returning IRQ_HANDLED is fine if one of those hold:
1. you're sure that at least one of the "if" branch is used
2. you'll be able to quickly figure out what's happening whenever 1. stops
   being true.

-- 
Ueimor

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-10 23:07       ` Måns Rullgård
@ 2015-11-11  0:36         ` Andy Shevchenko
  2015-11-11  0:44           ` Måns Rullgård
  0 siblings, 1 reply; 52+ messages in thread
From: Andy Shevchenko @ 2015-11-11  0:36 UTC (permalink / raw)
  To: Måns Rullgård; +Cc: linux-kernel, netdev, slash.tmp

On Wed, Nov 11, 2015 at 1:07 AM, Måns Rullgård <mans@mansr.com> wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> writes:

>>>>> +       nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
>>>>> +       wmb();          /* ensure desc addr is written before starting DMA */
>>>>
>>>> Hm… Have I missed corresponding rmb() ? If it's about MMIO, perhaps mmiowb() ?
>>>
>>> Possibly.
>>
>> Standalone wmb() doesn't make sense.
>
> It does if you need to enforce ordering between normal and I/O memory.
> In fact, since the descriptor is filled in using normal memory accesses,
> my understanding is that mmiowb() would be insufficient here.  The
> comment could be improved, however.

Can you then explain what exactly you are assured against in all cases
where you are using wmb()s? It seems I don't recognize this part in
some excerpts.


-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-10 23:34 ` Francois Romieu
@ 2015-11-11  0:40   ` Måns Rullgård
  2015-11-11  2:11     ` David Miller
  0 siblings, 1 reply; 52+ messages in thread
From: Måns Rullgård @ 2015-11-11  0:40 UTC (permalink / raw)
  To: Francois Romieu; +Cc: linux-kernel, netdev, slash.tmp

Francois Romieu <romieu@fr.zoreil.com> writes:

> Mans Rullgard <mans@mansr.com> :
>> diff --git a/drivers/net/ethernet/aurora/nb8800.c b/drivers/net/ethernet/aurora/nb8800.c
>> new file mode 100644
>> index 0000000..11cd389
>> --- /dev/null
>> +++ b/drivers/net/ethernet/aurora/nb8800.c
> [...]
>> +static int nb8800_xmit(struct sk_buff *skb, struct net_device *dev)
>> +{
> [...]
>> +
>> +	netdev_sent_queue(dev, skb->len);
>> +
>> +	smp_mb__before_spinlock();
>> +	spin_lock_irqsave(&priv->tx_lock, flags);
>
> At some point you may consider performing both Tx and Rx from napi context
> and thus replacing priv->tx_lock with netif_tx_lock.

That lock is to synchronise the DMA start between nb8800_xmit() and the
interrupt handler.  When the DMA complete interrupt arrives, the next
chain should be kicked off as quickly as possible, and I don't see why
that would benefit from being done in napi context.

>> +
>> +	if (!skb->xmit_more) {
>> +		priv->tx_chain->ready = true;
>> +		priv->tx_chain = NULL;
>> +		nb8800_tx_dma_start(dev);
>> +	}
>> +
>> +	spin_unlock_irqrestore(&priv->tx_lock, flags);
>> +
>> +	priv->tx_next = next;
>
> Are there strong reasons why nb8800_tx_done could not kick between
> spin_unlock_irqrestore and the non-local update of priv->tx_next ?

Good catch.  priv->tx_next wasn't accessed elsewhere in an earlier
version, and I forgot to fix that.  nb8800_tx_done() makes sure the DMA
has actually finished, so priv->tx_next should be updated before
starting the DMA rather than after.  The check against tx_next in
nb8800_tx_done() is only to put some limit on the loop and to avoid
confusion when nb8800_dma_stop() does it's dance.  There should be no
need for more synchronisation here than what the already present memory
barriers provide.

> [...]
>> +static irqreturn_t nb8800_irq(int irq, void *dev_id)
>> +{
>> +	struct net_device *dev = dev_id;
>> +	struct nb8800_priv *priv = netdev_priv(dev);
>> +	u32 val;
>> +
>> +	/* tx interrupt */
>> +	val = nb8800_readl(priv, NB8800_TXC_SR);
>> +	if (val) {
> [...]
>> +	}
>> +
>> +	/* rx interrupt */
>> +	val = nb8800_readl(priv, NB8800_RXC_SR);
>> +	if (val) {
> [...]
>> +	}
>> +
>> +	return IRQ_HANDLED;
>
> Returning IRQ_HANDLED is fine if one of those hold:
> 1. you're sure that at least one of the "if" branch is used
> 2. you'll be able to quickly figure out what's happening whenever 1. stops
>    being true.

You're right, better to check that the device really did have something
to say.

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11  0:36         ` Andy Shevchenko
@ 2015-11-11  0:44           ` Måns Rullgård
  0 siblings, 0 replies; 52+ messages in thread
From: Måns Rullgård @ 2015-11-11  0:44 UTC (permalink / raw)
  To: Andy Shevchenko; +Cc: linux-kernel, netdev, slash.tmp

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> On Wed, Nov 11, 2015 at 1:07 AM, Måns Rullgård <mans@mansr.com> wrote:
>> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
>
>>>>>> +       nb8800_writel(priv, NB8800_TX_DESC_ADDR, txb->dma_desc);
>>>>>> +       wmb();          /* ensure desc addr is written before starting DMA */
>>>>>
>>>>> Hm… Have I missed corresponding rmb() ? If it's about MMIO, perhaps mmiowb() ?
>>>>
>>>> Possibly.
>>>
>>> Standalone wmb() doesn't make sense.
>>
>> It does if you need to enforce ordering between normal and I/O memory.
>> In fact, since the descriptor is filled in using normal memory accesses,
>> my understanding is that mmiowb() would be insufficient here.  The
>> comment could be improved, however.
>
> Can you then explain what exactly you are assured against in all cases
> where you are using wmb()s? It seems I don't recognize this part in
> some excerpts.

Certainly.  I'll re-read memory-barriers.txt to make sure I'm doing the
right thing, then write a better comment.

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11  0:40   ` Måns Rullgård
@ 2015-11-11  2:11     ` David Miller
  2015-11-11 12:22       ` Måns Rullgård
  0 siblings, 1 reply; 52+ messages in thread
From: David Miller @ 2015-11-11  2:11 UTC (permalink / raw)
  To: mans; +Cc: romieu, linux-kernel, netdev, slash.tmp

From: Måns Rullgård <mans@mansr.com>
Date: Wed, 11 Nov 2015 00:40:09 +0000

> When the DMA complete interrupt arrives, the next chain should be
> kicked off as quickly as possible, and I don't see why that would
> benefit from being done in napi context.

NAPI isn't about low latency, it's about fairness and interrupt
mitigation.

You probably don't even realize that all of the TX SKB freeing you do
in the hardware interrupt handler end up being actually processed by a
scheduled software interrupt anyways.

So you are gaining almost nothing by not doing TX completion in NAPI
context, whereas by doing so you would be gaining a lot including
more simplified locking or even the ability to do no locking at all.

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11  2:11     ` David Miller
@ 2015-11-11 12:22       ` Måns Rullgård
  2015-11-11 13:04         ` Måns Rullgård
  2015-11-11 16:20         ` David Miller
  0 siblings, 2 replies; 52+ messages in thread
From: Måns Rullgård @ 2015-11-11 12:22 UTC (permalink / raw)
  To: David Miller; +Cc: romieu, linux-kernel, netdev, slash.tmp

David Miller <davem@davemloft.net> writes:

> From: Måns Rullgård <mans@mansr.com>
> Date: Wed, 11 Nov 2015 00:40:09 +0000
>
>> When the DMA complete interrupt arrives, the next chain should be
>> kicked off as quickly as possible, and I don't see why that would
>> benefit from being done in napi context.
>
> NAPI isn't about low latency, it's about fairness and interrupt
> mitigation.
>
> You probably don't even realize that all of the TX SKB freeing you do
> in the hardware interrupt handler end up being actually processed by a
> scheduled software interrupt anyways.
>
> So you are gaining almost nothing by not doing TX completion in NAPI
> context, whereas by doing so you would be gaining a lot including
> more simplified locking or even the ability to do no locking at all.

TX completion is separate from restarting the DMA, and moving that to
NAPI may well be a good idea.  Should I simply napi_schedule() if the
hardware indicates TX is complete and do the cleanup in the NAPI poll
function?

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 12:22       ` Måns Rullgård
@ 2015-11-11 13:04         ` Måns Rullgård
  2015-11-11 13:29           ` Eric Dumazet
  2015-11-11 16:24           ` David Miller
  2015-11-11 16:20         ` David Miller
  1 sibling, 2 replies; 52+ messages in thread
From: Måns Rullgård @ 2015-11-11 13:04 UTC (permalink / raw)
  To: David Miller; +Cc: romieu, linux-kernel, netdev, slash.tmp

Måns Rullgård <mans@mansr.com> writes:

> David Miller <davem@davemloft.net> writes:
>
>> From: Måns Rullgård <mans@mansr.com>
>> Date: Wed, 11 Nov 2015 00:40:09 +0000
>>
>>> When the DMA complete interrupt arrives, the next chain should be
>>> kicked off as quickly as possible, and I don't see why that would
>>> benefit from being done in napi context.
>>
>> NAPI isn't about low latency, it's about fairness and interrupt
>> mitigation.
>>
>> You probably don't even realize that all of the TX SKB freeing you do
>> in the hardware interrupt handler end up being actually processed by a
>> scheduled software interrupt anyways.
>>
>> So you are gaining almost nothing by not doing TX completion in NAPI
>> context, whereas by doing so you would be gaining a lot including
>> more simplified locking or even the ability to do no locking at all.
>
> TX completion is separate from restarting the DMA, and moving that to
> NAPI may well be a good idea.  Should I simply napi_schedule() if the
> hardware indicates TX is complete and do the cleanup in the NAPI poll
> function?

I tried that, and throughput (as measured by iperf3) dropped by 2%.
Maybe I did something wrong.

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 13:04         ` Måns Rullgård
@ 2015-11-11 13:29           ` Eric Dumazet
  2015-11-11 13:48             ` Måns Rullgård
  2015-11-11 16:24           ` David Miller
  1 sibling, 1 reply; 52+ messages in thread
From: Eric Dumazet @ 2015-11-11 13:29 UTC (permalink / raw)
  To: Måns Rullgård
  Cc: David Miller, romieu, linux-kernel, netdev, slash.tmp

On Wed, 2015-11-11 at 13:04 +0000, Måns Rullgård wrote:

> I tried that, and throughput (as measured by iperf3) dropped by 2%.
> Maybe I did something wrong.

What link speed have you used, what was the throughput you got,
and is the receiver using the same NIC ?





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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-10 21:51             ` Eric Dumazet
@ 2015-11-11 13:41               ` Mason
  2015-11-11 13:54                 ` Måns Rullgård
  2015-11-11 14:06                 ` Eric Dumazet
  0 siblings, 2 replies; 52+ messages in thread
From: Mason @ 2015-11-11 13:41 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: Mans Rullgard, David Miller, linux-kernel, netdev

On 10/11/2015 22:51, Eric Dumazet wrote:

> On Tue, 2015-11-10 at 21:21 +0000, Måns Rullgård wrote:
> 
>> Even ixgbe uses napi_complete() while netdevice.h says one should
>> "consider using napi_complete_done() instead."  Did the author consider
>> it and decide not to, or has the driver simply not been updated?
> 
> napi_complete_done() is quite new, very few drivers use it.

I was hoping to back-port this driver for my platform. Are the recent
features used in the driver available in 4.1? What about 3.14?

Regards.


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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 13:29           ` Eric Dumazet
@ 2015-11-11 13:48             ` Måns Rullgård
  2015-11-11 14:09               ` Eric Dumazet
  0 siblings, 1 reply; 52+ messages in thread
From: Måns Rullgård @ 2015-11-11 13:48 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: David Miller, romieu, linux-kernel, netdev, slash.tmp

Eric Dumazet <eric.dumazet@gmail.com> writes:

> On Wed, 2015-11-11 at 13:04 +0000, Måns Rullgård wrote:
>
>> I tried that, and throughput (as measured by iperf3) dropped by 2%.
>> Maybe I did something wrong.
>
> What link speed have you used, what was the throughput you got,
> and is the receiver using the same NIC ?

1Gbps link, 640 Mbps TCP transmit throughput to a PC with Intel NIC.
Why does it matter what NIC the receiver has?

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 13:41               ` Mason
@ 2015-11-11 13:54                 ` Måns Rullgård
  2015-11-11 14:10                   ` Eric Dumazet
  2015-11-11 14:06                 ` Eric Dumazet
  1 sibling, 1 reply; 52+ messages in thread
From: Måns Rullgård @ 2015-11-11 13:54 UTC (permalink / raw)
  To: Mason; +Cc: Eric Dumazet, David Miller, linux-kernel, netdev

Mason <slash.tmp@free.fr> writes:

> On 10/11/2015 22:51, Eric Dumazet wrote:
>
>> On Tue, 2015-11-10 at 21:21 +0000, Måns Rullgård wrote:
>> 
>>> Even ixgbe uses napi_complete() while netdevice.h says one should
>>> "consider using napi_complete_done() instead."  Did the author consider
>>> it and decide not to, or has the driver simply not been updated?
>> 
>> napi_complete_done() is quite new, very few drivers use it.
>
> I was hoping to back-port this driver for my platform. Are the recent
> features used in the driver available in 4.1? What about 3.14?

xmit_more was added in 3.18, napi_complete_done() in 3.19.

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 13:41               ` Mason
  2015-11-11 13:54                 ` Måns Rullgård
@ 2015-11-11 14:06                 ` Eric Dumazet
  1 sibling, 0 replies; 52+ messages in thread
From: Eric Dumazet @ 2015-11-11 14:06 UTC (permalink / raw)
  To: Mason; +Cc: Mans Rullgard, David Miller, linux-kernel, netdev

On Wed, 2015-11-11 at 14:41 +0100, Mason wrote:
> On 10/11/2015 22:51, Eric Dumazet wrote:
> 
> > On Tue, 2015-11-10 at 21:21 +0000, Måns Rullgård wrote:
> > 
> >> Even ixgbe uses napi_complete() while netdevice.h says one should
> >> "consider using napi_complete_done() instead."  Did the author consider
> >> it and decide not to, or has the driver simply not been updated?
> > 
> > napi_complete_done() is quite new, very few drivers use it.
> 
> I was hoping to back-port this driver for my platform. Are the recent
> features used in the driver available in 4.1? What about 3.14?

Sure, this is all available 

napi_complete_done() can be backported on old kernels as a
napi_complete() 



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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 13:48             ` Måns Rullgård
@ 2015-11-11 14:09               ` Eric Dumazet
  2015-11-11 14:15                 ` Måns Rullgård
  0 siblings, 1 reply; 52+ messages in thread
From: Eric Dumazet @ 2015-11-11 14:09 UTC (permalink / raw)
  To: Måns Rullgård
  Cc: David Miller, romieu, linux-kernel, netdev, slash.tmp

On Wed, 2015-11-11 at 13:48 +0000, Måns Rullgård wrote:
> Eric Dumazet <eric.dumazet@gmail.com> writes:
> 
> > On Wed, 2015-11-11 at 13:04 +0000, Måns Rullgård wrote:
> >
> >> I tried that, and throughput (as measured by iperf3) dropped by 2%.
> >> Maybe I did something wrong.
> >
> > What link speed have you used, what was the throughput you got,
> > and is the receiver using the same NIC ?
> 
> 1Gbps link, 640 Mbps TCP transmit throughput to a PC with Intel NIC.
> Why does it matter what NIC the receiver has?

Because at 1Gb line rate, you better get GRO properly implemented in the
receiver, so that TCP stack does not send one ACK every 2 MSS.

Send speed is also dependent on the number of ACK packets the sender has
to process.

This is why I suggested you use napi_gro_receive() in your driver.




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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 13:54                 ` Måns Rullgård
@ 2015-11-11 14:10                   ` Eric Dumazet
  0 siblings, 0 replies; 52+ messages in thread
From: Eric Dumazet @ 2015-11-11 14:10 UTC (permalink / raw)
  To: Måns Rullgård; +Cc: Mason, David Miller, linux-kernel, netdev

On Wed, 2015-11-11 at 13:54 +0000, Måns Rullgård wrote:
> Mason <slash.tmp@free.fr> writes:
> 
> > On 10/11/2015 22:51, Eric Dumazet wrote:
> >
> >> On Tue, 2015-11-10 at 21:21 +0000, Måns Rullgård wrote:
> >> 
> >>> Even ixgbe uses napi_complete() while netdevice.h says one should
> >>> "consider using napi_complete_done() instead."  Did the author consider
> >>> it and decide not to, or has the driver simply not been updated?
> >> 
> >> napi_complete_done() is quite new, very few drivers use it.
> >
> > I was hoping to back-port this driver for my platform. Are the recent
> > features used in the driver available in 4.1? What about 3.14?
> 
> xmit_more was added in 3.18, napi_complete_done() in 3.19.

xmit_more is a hint, you can simply consider skb->xmit_more being
replaced by 0 on old kernels if you want to backport.




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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 14:09               ` Eric Dumazet
@ 2015-11-11 14:15                 ` Måns Rullgård
  2015-11-11 14:35                   ` Måns Rullgård
  2015-11-11 14:42                   ` Eric Dumazet
  0 siblings, 2 replies; 52+ messages in thread
From: Måns Rullgård @ 2015-11-11 14:15 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: David Miller, romieu, linux-kernel, netdev, slash.tmp

Eric Dumazet <eric.dumazet@gmail.com> writes:

> On Wed, 2015-11-11 at 13:48 +0000, Måns Rullgård wrote:
>> Eric Dumazet <eric.dumazet@gmail.com> writes:
>> 
>> > On Wed, 2015-11-11 at 13:04 +0000, Måns Rullgård wrote:
>> >
>> >> I tried that, and throughput (as measured by iperf3) dropped by 2%.
>> >> Maybe I did something wrong.
>> >
>> > What link speed have you used, what was the throughput you got,
>> > and is the receiver using the same NIC ?
>> 
>> 1Gbps link, 640 Mbps TCP transmit throughput to a PC with Intel NIC.
>> Why does it matter what NIC the receiver has?
>
> Because at 1Gb line rate, you better get GRO properly implemented in the
> receiver, so that TCP stack does not send one ACK every 2 MSS.
>
> Send speed is also dependent on the number of ACK packets the sender has
> to process.
>
> This is why I suggested you use napi_gro_receive() in your driver.

FWIW, with UDP I get 650 Mbps.

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 14:15                 ` Måns Rullgård
@ 2015-11-11 14:35                   ` Måns Rullgård
  2015-11-11 14:44                     ` Eric Dumazet
  2015-11-11 14:42                   ` Eric Dumazet
  1 sibling, 1 reply; 52+ messages in thread
From: Måns Rullgård @ 2015-11-11 14:35 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: David Miller, romieu, linux-kernel, netdev, slash.tmp

Måns Rullgård <mans@mansr.com> writes:

> Eric Dumazet <eric.dumazet@gmail.com> writes:
>
>> On Wed, 2015-11-11 at 13:48 +0000, Måns Rullgård wrote:
>>> Eric Dumazet <eric.dumazet@gmail.com> writes:
>>> 
>>> > On Wed, 2015-11-11 at 13:04 +0000, Måns Rullgård wrote:
>>> >
>>> >> I tried that, and throughput (as measured by iperf3) dropped by 2%.
>>> >> Maybe I did something wrong.
>>> >
>>> > What link speed have you used, what was the throughput you got,
>>> > and is the receiver using the same NIC ?
>>> 
>>> 1Gbps link, 640 Mbps TCP transmit throughput to a PC with Intel NIC.
>>> Why does it matter what NIC the receiver has?
>>
>> Because at 1Gb line rate, you better get GRO properly implemented in the
>> receiver, so that TCP stack does not send one ACK every 2 MSS.
>>
>> Send speed is also dependent on the number of ACK packets the sender has
>> to process.
>>
>> This is why I suggested you use napi_gro_receive() in your driver.
>
> FWIW, with UDP I get 650 Mbps.

It seems the on-chip interconnect is limiting memory bandwidth available
to the Ethernet DMA.  If I increase the limits, I get 800 Mbps over TCP
and 850 Mbps over UDP with the driver posted here, the TCP case being
CPU bound in csum_partial_copy_from_user.

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 14:15                 ` Måns Rullgård
  2015-11-11 14:35                   ` Måns Rullgård
@ 2015-11-11 14:42                   ` Eric Dumazet
  1 sibling, 0 replies; 52+ messages in thread
From: Eric Dumazet @ 2015-11-11 14:42 UTC (permalink / raw)
  To: Måns Rullgård
  Cc: David Miller, romieu, linux-kernel, netdev, slash.tmp

On Wed, 2015-11-11 at 14:15 +0000, Måns Rullgård wrote:

> FWIW, with UDP I get 650 Mbps.

Have you tried pktgen ? It should give you raw numbers, without the
stack overhead, especially when using the clone operator.

(It also has xmit_more support)

If the NIC can not get more than 650 Mbps, not worth trying implementing
tso in your driver (using net/core/tso.c helpers)




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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 14:35                   ` Måns Rullgård
@ 2015-11-11 14:44                     ` Eric Dumazet
  0 siblings, 0 replies; 52+ messages in thread
From: Eric Dumazet @ 2015-11-11 14:44 UTC (permalink / raw)
  To: Måns Rullgård
  Cc: David Miller, romieu, linux-kernel, netdev, slash.tmp

On Wed, 2015-11-11 at 14:35 +0000, Måns Rullgård wrote:

> It seems the on-chip interconnect is limiting memory bandwidth available
> to the Ethernet DMA.  If I increase the limits, I get 800 Mbps over TCP
> and 850 Mbps over UDP with the driver posted here, the TCP case being
> CPU bound in csum_partial_copy_from_user.

Ah right, NIC has no TX checksum offload :(




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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 12:22       ` Måns Rullgård
  2015-11-11 13:04         ` Måns Rullgård
@ 2015-11-11 16:20         ` David Miller
  1 sibling, 0 replies; 52+ messages in thread
From: David Miller @ 2015-11-11 16:20 UTC (permalink / raw)
  To: mans; +Cc: romieu, linux-kernel, netdev, slash.tmp

From: Måns Rullgård <mans@mansr.com>
Date: Wed, 11 Nov 2015 12:22:28 +0000

> David Miller <davem@davemloft.net> writes:
> 
>> From: Måns Rullgård <mans@mansr.com>
>> Date: Wed, 11 Nov 2015 00:40:09 +0000
>>
>>> When the DMA complete interrupt arrives, the next chain should be
>>> kicked off as quickly as possible, and I don't see why that would
>>> benefit from being done in napi context.
>>
>> NAPI isn't about low latency, it's about fairness and interrupt
>> mitigation.
>>
>> You probably don't even realize that all of the TX SKB freeing you do
>> in the hardware interrupt handler end up being actually processed by a
>> scheduled software interrupt anyways.
>>
>> So you are gaining almost nothing by not doing TX completion in NAPI
>> context, whereas by doing so you would be gaining a lot including
>> more simplified locking or even the ability to do no locking at all.
> 
> TX completion is separate from restarting the DMA, and moving that to
> NAPI may well be a good idea.  Should I simply napi_schedule() if the
> hardware indicates TX is complete and do the cleanup in the NAPI poll
> function?

... just like every other high end driver... Yes.

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 13:04         ` Måns Rullgård
  2015-11-11 13:29           ` Eric Dumazet
@ 2015-11-11 16:24           ` David Miller
  2015-11-11 18:25             ` Måns Rullgård
  1 sibling, 1 reply; 52+ messages in thread
From: David Miller @ 2015-11-11 16:24 UTC (permalink / raw)
  To: mans; +Cc: romieu, linux-kernel, netdev, slash.tmp

From: Måns Rullgård <mans@mansr.com>
Date: Wed, 11 Nov 2015 13:04:07 +0000

> Måns Rullgård <mans@mansr.com> writes:
> 
>> David Miller <davem@davemloft.net> writes:
>>
>>> From: Måns Rullgård <mans@mansr.com>
>>> Date: Wed, 11 Nov 2015 00:40:09 +0000
>>>
>>>> When the DMA complete interrupt arrives, the next chain should be
>>>> kicked off as quickly as possible, and I don't see why that would
>>>> benefit from being done in napi context.
>>>
>>> NAPI isn't about low latency, it's about fairness and interrupt
>>> mitigation.
>>>
>>> You probably don't even realize that all of the TX SKB freeing you do
>>> in the hardware interrupt handler end up being actually processed by a
>>> scheduled software interrupt anyways.
>>>
>>> So you are gaining almost nothing by not doing TX completion in NAPI
>>> context, whereas by doing so you would be gaining a lot including
>>> more simplified locking or even the ability to do no locking at all.
>>
>> TX completion is separate from restarting the DMA, and moving that to
>> NAPI may well be a good idea.  Should I simply napi_schedule() if the
>> hardware indicates TX is complete and do the cleanup in the NAPI poll
>> function?
> 
> I tried that, and throughput (as measured by iperf3) dropped by 2%.
> Maybe I did something wrong.

Did you fix all the locking in that change?

Since all of your TX handling runs in software interrupt context, you
can stop using IRQ locking and use BH locking driver-wide instead.

And actually, no locking is really needed for TX processing.  With
proper memory barriers and properly crafter queue state tests, you
can run completely lockless.

Again, look at example drivers.  I know, for example, that
drivers/net/ethernet/broadcom/tg3.c runs TX lockless.  You'll
see that tg3_tx() takes no locks at all.

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 16:24           ` David Miller
@ 2015-11-11 18:25             ` Måns Rullgård
  2015-11-11 19:02               ` David Miller
  0 siblings, 1 reply; 52+ messages in thread
From: Måns Rullgård @ 2015-11-11 18:25 UTC (permalink / raw)
  To: David Miller; +Cc: romieu, linux-kernel, netdev, slash.tmp

David Miller <davem@davemloft.net> writes:

> From: Måns Rullgård <mans@mansr.com>
> Date: Wed, 11 Nov 2015 13:04:07 +0000
>
>> Måns Rullgård <mans@mansr.com> writes:
>> 
>>> David Miller <davem@davemloft.net> writes:
>>>
>>>> From: Måns Rullgård <mans@mansr.com>
>>>> Date: Wed, 11 Nov 2015 00:40:09 +0000
>>>>
>>>>> When the DMA complete interrupt arrives, the next chain should be
>>>>> kicked off as quickly as possible, and I don't see why that would
>>>>> benefit from being done in napi context.
>>>>
>>>> NAPI isn't about low latency, it's about fairness and interrupt
>>>> mitigation.
>>>>
>>>> You probably don't even realize that all of the TX SKB freeing you do
>>>> in the hardware interrupt handler end up being actually processed by a
>>>> scheduled software interrupt anyways.
>>>>
>>>> So you are gaining almost nothing by not doing TX completion in NAPI
>>>> context, whereas by doing so you would be gaining a lot including
>>>> more simplified locking or even the ability to do no locking at all.
>>>
>>> TX completion is separate from restarting the DMA, and moving that to
>>> NAPI may well be a good idea.  Should I simply napi_schedule() if the
>>> hardware indicates TX is complete and do the cleanup in the NAPI poll
>>> function?
>> 
>> I tried that, and throughput (as measured by iperf3) dropped by 2%.
>> Maybe I did something wrong.
>
> Did you fix all the locking in that change?
>
> Since all of your TX handling runs in software interrupt context, you
> can stop using IRQ locking and use BH locking driver-wide instead.
>
> And actually, no locking is really needed for TX processing.  With
> proper memory barriers and properly crafter queue state tests, you
> can run completely lockless.
>
> Again, look at example drivers.  I know, for example, that
> drivers/net/ethernet/broadcom/tg3.c runs TX lockless.  You'll
> see that tg3_tx() takes no locks at all.

The way the hardware works, once a DMA operation has been started,
adding more packets to the active chain can't be done reliably.  For
that reason, if start_xmit is called (with xmit_more zero) while a DMA
operation is in progress, the new packet(s) must be queued until the
hardware raises the DMA complete interrupt.  At that time, the next
pending DMA chain, if any, can be kicked off.  If the TX DMA channel is
idle when start_xmit is called, it can be started immediately.  Checking
the DMA status and starting it if idle has to be done atomically
somehow.

There is a separate indication for actual TX completion, and the
interrupt for that can be set to only fire every 7 frames or when a
timeout expires.  When this happens, the TX cleanup needs to run, and
that can obviously be done from NAPI without using any locks.

Bear in mind that this hardware is quite primitive compared to modern
high-performance Ethernet controllers from the likes of Intel and
Broadcom.  The documentation I have is dated 2003.

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 18:25             ` Måns Rullgård
@ 2015-11-11 19:02               ` David Miller
  2015-11-11 19:09                 ` Måns Rullgård
  0 siblings, 1 reply; 52+ messages in thread
From: David Miller @ 2015-11-11 19:02 UTC (permalink / raw)
  To: mans; +Cc: romieu, linux-kernel, netdev, slash.tmp

From: Måns Rullgård <mans@mansr.com>
Date: Wed, 11 Nov 2015 18:25:05 +0000

> If the TX DMA channel is idle when start_xmit is called, it can be
> started immediately.  Checking the DMA status and starting it if
> idle has to be done atomically somehow.

->ndo_start_xmit() is guaranteed to be invoked atomically, protected
by the TX queue spinlock.

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 19:02               ` David Miller
@ 2015-11-11 19:09                 ` Måns Rullgård
  2015-11-11 19:13                   ` David Miller
  0 siblings, 1 reply; 52+ messages in thread
From: Måns Rullgård @ 2015-11-11 19:09 UTC (permalink / raw)
  To: David Miller; +Cc: romieu, linux-kernel, netdev, slash.tmp

David Miller <davem@davemloft.net> writes:

> From: Måns Rullgård <mans@mansr.com>
> Date: Wed, 11 Nov 2015 18:25:05 +0000
>
>> If the TX DMA channel is idle when start_xmit is called, it can be
>> started immediately.  Checking the DMA status and starting it if
>> idle has to be done atomically somehow.
>
> ->ndo_start_xmit() is guaranteed to be invoked atomically, protected
> by the TX queue spinlock.

Yes, but the DMA needs to be restarted from some other context if it was
busy when start_xmit checked.

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 19:09                 ` Måns Rullgård
@ 2015-11-11 19:13                   ` David Miller
  2015-11-11 19:17                     ` Måns Rullgård
  0 siblings, 1 reply; 52+ messages in thread
From: David Miller @ 2015-11-11 19:13 UTC (permalink / raw)
  To: mans; +Cc: romieu, linux-kernel, netdev, slash.tmp

From: Måns Rullgård <mans@mansr.com>
Date: Wed, 11 Nov 2015 19:09:19 +0000

> David Miller <davem@davemloft.net> writes:
> 
>> From: Måns Rullgård <mans@mansr.com>
>> Date: Wed, 11 Nov 2015 18:25:05 +0000
>>
>>> If the TX DMA channel is idle when start_xmit is called, it can be
>>> started immediately.  Checking the DMA status and starting it if
>>> idle has to be done atomically somehow.
>>
>> ->ndo_start_xmit() is guaranteed to be invoked atomically, protected
>> by the TX queue spinlock.
> 
> Yes, but the DMA needs to be restarted from some other context if it was
> busy when start_xmit checked.

Then you can probably use the TXQ lock in the interrupt handler just for
that.

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 19:13                   ` David Miller
@ 2015-11-11 19:17                     ` Måns Rullgård
  2015-11-11 19:19                       ` David Miller
  0 siblings, 1 reply; 52+ messages in thread
From: Måns Rullgård @ 2015-11-11 19:17 UTC (permalink / raw)
  To: David Miller; +Cc: romieu, linux-kernel, netdev, slash.tmp

David Miller <davem@davemloft.net> writes:

> From: Måns Rullgård <mans@mansr.com>
> Date: Wed, 11 Nov 2015 19:09:19 +0000
>
>> David Miller <davem@davemloft.net> writes:
>> 
>>> From: Måns Rullgård <mans@mansr.com>
>>> Date: Wed, 11 Nov 2015 18:25:05 +0000
>>>
>>>> If the TX DMA channel is idle when start_xmit is called, it can be
>>>> started immediately.  Checking the DMA status and starting it if
>>>> idle has to be done atomically somehow.
>>>
>>> ->ndo_start_xmit() is guaranteed to be invoked atomically, protected
>>> by the TX queue spinlock.
>> 
>> Yes, but the DMA needs to be restarted from some other context if it was
>> busy when start_xmit checked.
>
> Then you can probably use the TXQ lock in the interrupt handler just for
> that.

That seems a bit heavy-handed when the critical section for this is only
a tiny part of the start_xmit function.

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 19:17                     ` Måns Rullgård
@ 2015-11-11 19:19                       ` David Miller
  2015-11-11 19:25                         ` Måns Rullgård
  0 siblings, 1 reply; 52+ messages in thread
From: David Miller @ 2015-11-11 19:19 UTC (permalink / raw)
  To: mans; +Cc: romieu, linux-kernel, netdev, slash.tmp

From: Måns Rullgård <mans@mansr.com>
Date: Wed, 11 Nov 2015 19:17:07 +0000

> David Miller <davem@davemloft.net> writes:
> 
>> From: Måns Rullgård <mans@mansr.com>
>> Date: Wed, 11 Nov 2015 19:09:19 +0000
>>
>>> David Miller <davem@davemloft.net> writes:
>>> 
>>>> From: Måns Rullgård <mans@mansr.com>
>>>> Date: Wed, 11 Nov 2015 18:25:05 +0000
>>>>
>>>>> If the TX DMA channel is idle when start_xmit is called, it can be
>>>>> started immediately.  Checking the DMA status and starting it if
>>>>> idle has to be done atomically somehow.
>>>>
>>>> ->ndo_start_xmit() is guaranteed to be invoked atomically, protected
>>>> by the TX queue spinlock.
>>> 
>>> Yes, but the DMA needs to be restarted from some other context if it was
>>> busy when start_xmit checked.
>>
>> Then you can probably use the TXQ lock in the interrupt handler just for
>> that.
> 
> That seems a bit heavy-handed when the critical section for this is only
> a tiny part of the start_xmit function.

Then what synchornization primitive other than spin locks are you going
to use for this?

My point is that there is a spinlock the core code is _already_ taking,
unconditionally, when ->ndo_start_xmit() executes.  And you can therefore
take advantage of that rather than using another lock of your own.

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 19:19                       ` David Miller
@ 2015-11-11 19:25                         ` Måns Rullgård
  2015-11-11 19:26                           ` David Miller
  0 siblings, 1 reply; 52+ messages in thread
From: Måns Rullgård @ 2015-11-11 19:25 UTC (permalink / raw)
  To: David Miller; +Cc: romieu, linux-kernel, netdev, slash.tmp

David Miller <davem@davemloft.net> writes:

> From: Måns Rullgård <mans@mansr.com>
> Date: Wed, 11 Nov 2015 19:17:07 +0000
>
>> David Miller <davem@davemloft.net> writes:
>> 
>>> From: Måns Rullgård <mans@mansr.com>
>>> Date: Wed, 11 Nov 2015 19:09:19 +0000
>>>
>>>> David Miller <davem@davemloft.net> writes:
>>>> 
>>>>> From: Måns Rullgård <mans@mansr.com>
>>>>> Date: Wed, 11 Nov 2015 18:25:05 +0000
>>>>>
>>>>>> If the TX DMA channel is idle when start_xmit is called, it can be
>>>>>> started immediately.  Checking the DMA status and starting it if
>>>>>> idle has to be done atomically somehow.
>>>>>
>>>>> ->ndo_start_xmit() is guaranteed to be invoked atomically, protected
>>>>> by the TX queue spinlock.
>>>> 
>>>> Yes, but the DMA needs to be restarted from some other context if it was
>>>> busy when start_xmit checked.
>>>
>>> Then you can probably use the TXQ lock in the interrupt handler just for
>>> that.
>> 
>> That seems a bit heavy-handed when the critical section for this is only
>> a tiny part of the start_xmit function.
>
> Then what synchornization primitive other than spin locks are you going
> to use for this?
>
> My point is that there is a spinlock the core code is _already_ taking,
> unconditionally, when ->ndo_start_xmit() executes.  And you can therefore
> take advantage of that rather than using another lock of your own.

I get that.  But that remains locked for the duration of ndo_start_xmit()
whereas the part that needs to be synchronised with the DMA completion
IRQ handler is tiny.  Having the IRQ handler spin for the duration of
ndo_start_xmit() seemed silly to me.

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 19:25                         ` Måns Rullgård
@ 2015-11-11 19:26                           ` David Miller
  2015-11-11 19:35                             ` Måns Rullgård
  0 siblings, 1 reply; 52+ messages in thread
From: David Miller @ 2015-11-11 19:26 UTC (permalink / raw)
  To: mans; +Cc: romieu, linux-kernel, netdev, slash.tmp

From: Måns Rullgård <mans@mansr.com>
Date: Wed, 11 Nov 2015 19:25:46 +0000

> David Miller <davem@davemloft.net> writes:
> 
>> From: Måns Rullgård <mans@mansr.com>
>> Date: Wed, 11 Nov 2015 19:17:07 +0000
>>
>>> David Miller <davem@davemloft.net> writes:
>>> 
>>>> From: Måns Rullgård <mans@mansr.com>
>>>> Date: Wed, 11 Nov 2015 19:09:19 +0000
>>>>
>>>>> David Miller <davem@davemloft.net> writes:
>>>>> 
>>>>>> From: Måns Rullgård <mans@mansr.com>
>>>>>> Date: Wed, 11 Nov 2015 18:25:05 +0000
>>>>>>
>>>>>>> If the TX DMA channel is idle when start_xmit is called, it can be
>>>>>>> started immediately.  Checking the DMA status and starting it if
>>>>>>> idle has to be done atomically somehow.
>>>>>>
>>>>>> ->ndo_start_xmit() is guaranteed to be invoked atomically, protected
>>>>>> by the TX queue spinlock.
>>>>> 
>>>>> Yes, but the DMA needs to be restarted from some other context if it was
>>>>> busy when start_xmit checked.
>>>>
>>>> Then you can probably use the TXQ lock in the interrupt handler just for
>>>> that.
>>> 
>>> That seems a bit heavy-handed when the critical section for this is only
>>> a tiny part of the start_xmit function.
>>
>> Then what synchornization primitive other than spin locks are you going
>> to use for this?
>>
>> My point is that there is a spinlock the core code is _already_ taking,
>> unconditionally, when ->ndo_start_xmit() executes.  And you can therefore
>> take advantage of that rather than using another lock of your own.
> 
> I get that.  But that remains locked for the duration of ndo_start_xmit()
> whereas the part that needs to be synchronised with the DMA completion
> IRQ handler is tiny.  Having the IRQ handler spin for the duration of
> ndo_start_xmit() seemed silly to me.

I don't think it's silly at all.

And unless you can measure it making a difference, don't knock the idea.

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 19:26                           ` David Miller
@ 2015-11-11 19:35                             ` Måns Rullgård
  2015-11-11 19:48                               ` David Miller
  0 siblings, 1 reply; 52+ messages in thread
From: Måns Rullgård @ 2015-11-11 19:35 UTC (permalink / raw)
  To: David Miller; +Cc: romieu, linux-kernel, netdev, slash.tmp

David Miller <davem@davemloft.net> writes:

> From: Måns Rullgård <mans@mansr.com>
> Date: Wed, 11 Nov 2015 19:25:46 +0000
>
>> David Miller <davem@davemloft.net> writes:
>> 
>>> From: Måns Rullgård <mans@mansr.com>
>>> Date: Wed, 11 Nov 2015 19:17:07 +0000
>>>
>>>> David Miller <davem@davemloft.net> writes:
>>>> 
>>>>> From: Måns Rullgård <mans@mansr.com>
>>>>> Date: Wed, 11 Nov 2015 19:09:19 +0000
>>>>>
>>>>>> David Miller <davem@davemloft.net> writes:
>>>>>> 
>>>>>>> From: Måns Rullgård <mans@mansr.com>
>>>>>>> Date: Wed, 11 Nov 2015 18:25:05 +0000
>>>>>>>
>>>>>>>> If the TX DMA channel is idle when start_xmit is called, it can be
>>>>>>>> started immediately.  Checking the DMA status and starting it if
>>>>>>>> idle has to be done atomically somehow.
>>>>>>>
>>>>>>> ->ndo_start_xmit() is guaranteed to be invoked atomically, protected
>>>>>>> by the TX queue spinlock.
>>>>>> 
>>>>>> Yes, but the DMA needs to be restarted from some other context if it was
>>>>>> busy when start_xmit checked.
>>>>>
>>>>> Then you can probably use the TXQ lock in the interrupt handler just for
>>>>> that.
>>>> 
>>>> That seems a bit heavy-handed when the critical section for this is only
>>>> a tiny part of the start_xmit function.
>>>
>>> Then what synchornization primitive other than spin locks are you going
>>> to use for this?
>>>
>>> My point is that there is a spinlock the core code is _already_ taking,
>>> unconditionally, when ->ndo_start_xmit() executes.  And you can therefore
>>> take advantage of that rather than using another lock of your own.
>> 
>> I get that.  But that remains locked for the duration of ndo_start_xmit()
>> whereas the part that needs to be synchronised with the DMA completion
>> IRQ handler is tiny.  Having the IRQ handler spin for the duration of
>> ndo_start_xmit() seemed silly to me.
>
> I don't think it's silly at all.

I'm sure I read somewhere that the time spent spinning on a lock should
be kept as small as possible.

> And unless you can measure it making a difference, don't knock the idea.

I tried using netif_tx_lock() in the IRQ handler instead, and it locked
up solid.  Clearly that was the wrong thing to do.

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 19:35                             ` Måns Rullgård
@ 2015-11-11 19:48                               ` David Miller
  2015-11-11 20:47                                 ` Måns Rullgård
  0 siblings, 1 reply; 52+ messages in thread
From: David Miller @ 2015-11-11 19:48 UTC (permalink / raw)
  To: mans; +Cc: romieu, linux-kernel, netdev, slash.tmp

From: Måns Rullgård <mans@mansr.com>
Date: Wed, 11 Nov 2015 19:35:05 +0000

>> I don't think it's silly at all.
> 
> I'm sure I read somewhere that the time spent spinning on a lock should
> be kept as small as possible.
> 
>> And unless you can measure it making a difference, don't knock the idea.
> 
> I tried using netif_tx_lock() in the IRQ handler instead, and it locked
> up solid.  Clearly that was the wrong thing to do.

Oh that's right, it's a BH lock not an IRQ one.

Yet another argument for doing everything in ->poll(), thus making all
operations outside of NAPI scheduling run in software interrupt
context, and therefore being able to make use of the TXQ lock for
this.

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-11 19:48                               ` David Miller
@ 2015-11-11 20:47                                 ` Måns Rullgård
  0 siblings, 0 replies; 52+ messages in thread
From: Måns Rullgård @ 2015-11-11 20:47 UTC (permalink / raw)
  To: David Miller; +Cc: romieu, linux-kernel, netdev, slash.tmp

David Miller <davem@davemloft.net> writes:

> From: Måns Rullgård <mans@mansr.com>
> Date: Wed, 11 Nov 2015 19:35:05 +0000
>
>>> I don't think it's silly at all.
>> 
>> I'm sure I read somewhere that the time spent spinning on a lock should
>> be kept as small as possible.
>> 
>>> And unless you can measure it making a difference, don't knock the idea.
>> 
>> I tried using netif_tx_lock() in the IRQ handler instead, and it locked
>> up solid.  Clearly that was the wrong thing to do.
>
> Oh that's right, it's a BH lock not an IRQ one.
>
> Yet another argument for doing everything in ->poll(), thus making all
> operations outside of NAPI scheduling run in software interrupt
> context, and therefore being able to make use of the TXQ lock for
> this.

Well, I tried calling the DMA restart function from NAPI poll under
netif_tx_lock().  Now it works only as long as there is incoming
traffic.

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-10 19:25   ` Måns Rullgård
@ 2015-11-12 13:33     ` Mason
  2015-11-12 14:04       ` Måns Rullgård
  0 siblings, 1 reply; 52+ messages in thread
From: Mason @ 2015-11-12 13:33 UTC (permalink / raw)
  To: Mans Rullgard; +Cc: linux-kernel, netdev

On 10/11/2015 20:25, Måns Rullgård wrote:

> Mason writes:
> 
>> On 10/11/2015 17:14, Mans Rullgard wrote:
>>
>>> This adds a driver for the Aurora VLSI NB8800 Ethernet controller.
>>> It is an almost complete rewrite of a driver originally found in
>>> a Sigma Designs 2.6.22 tree.
>>>
>>> Signed-off-by: Mans Rullgard <mans@mansr.com>
>>> ---
>>> Changes:
>>> - Refactored mdio access functions
>>> - Refactored register access helpers
>>> - Improved error handling in rx buffer allocation
>>> - Optimised some fifo parameters
>>> - Overhauled tx dma. Multiple packets are now chained in a single dma
>>>   operation if xmit_more is set, improving performance.
>>> - Improved rx irq handling. It's not possible to disable interrupts
>>>   entirely for napi poll, but they can be slowed down a little.
>>> - Use readx_poll_timeout in various places
>>> - Improved error detection
>>> - Improved statistics
>>> - Report hardware statistics counters through ethtool
>>> - Improved tangox-specific setup
>>> - Support for flow control using pause frames
>>> - Explanatory comments added
>>> - Various minor stylistic changes
>>> ---
>>>  drivers/net/ethernet/Kconfig         |    1 +
>>>  drivers/net/ethernet/Makefile        |    1 +
>>>  drivers/net/ethernet/aurora/Kconfig  |   20 +
>>>  drivers/net/ethernet/aurora/Makefile |    1 +
>>>  drivers/net/ethernet/aurora/nb8800.c | 1530 ++++++++++++++++++++++++++++++++++
>>>  drivers/net/ethernet/aurora/nb8800.h |  314 +++++++
>>>  6 files changed, 1867 insertions(+)
>>
>> The code has grown much since the previous patch, despite some
>> refactoring. Is this mostly due to ethtool_ops support?
>>
>>  drivers/net/ethernet/aurora/nb8800.c | 1146 ++++++++++++++++++++++++++++++++++
>>  drivers/net/ethernet/aurora/nb8800.h |  230 +++++++
> 
> Some of the increase is from new features, some from improvements, and
> then there are a bunch of new comments.

Sweet.

With this version, my kernel boots faster than before
(I had been using a 5 month-old version.)

Before:

[    0.613623] tangox-enet 26000.ethernet: SMP86xx internal Ethernet at 0x26000
[    0.623638] libphy: tangox-mii: probed
[    0.686527] tangox-enet 26000.ethernet: PHY: found Atheros 8035 ethernet at 0x4
[    0.697169] tangox-enet 26000.ethernet eth0: MAC address 00:16:e8:02:08:42
...
[    1.306360] Sending DHCP requests ..
[    4.699969] tangox-enet 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
[    8.899671] ., OK
[    8.926343] IP-Config: Got DHCP answer from 172.27.200.1, my address is 172.27.64.49
...
[    8.987327] Freeing unused kernel memory: 168K (c039e000 - c03c8000)

After:

[    0.623526] libphy: nb8800-mii: probed
[    0.628092] nb8800 26000.ethernet eth0: MAC address 00:16:e8:02:08:42
...
[    4.732948] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
[    4.752655] Sending DHCP requests ., OK
[    4.782644] IP-Config: Got DHCP answer from 172.27.200.1, my address is 172.27.64.49
...
[    4.849298] Freeing unused kernel memory: 164K (c039f000 - c03c8000)


The DHCP request is sent later, but the kernel doesn't twiddle its thumbs
for 4 seconds after the link comes up. Does this come from not probing the
PHY anymore?

BTW, you're not using the PHY IRQ, right? I think I remember you saying
it didn't work reliably?

Regards.


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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-12 13:33     ` Mason
@ 2015-11-12 14:04       ` Måns Rullgård
  2015-11-12 16:19         ` Mason
  0 siblings, 1 reply; 52+ messages in thread
From: Måns Rullgård @ 2015-11-12 14:04 UTC (permalink / raw)
  To: Mason; +Cc: linux-kernel, netdev

Mason <slash.tmp@free.fr> writes:

> On 10/11/2015 20:25, Måns Rullgård wrote:
>
>> Mason writes:
>> 
>>> On 10/11/2015 17:14, Mans Rullgard wrote:
>>>
>>>> This adds a driver for the Aurora VLSI NB8800 Ethernet controller.
>>>> It is an almost complete rewrite of a driver originally found in
>>>> a Sigma Designs 2.6.22 tree.
>>>>
>>>> Signed-off-by: Mans Rullgard <mans@mansr.com>
>>>> ---
>>>> Changes:
>>>> - Refactored mdio access functions
>>>> - Refactored register access helpers
>>>> - Improved error handling in rx buffer allocation
>>>> - Optimised some fifo parameters
>>>> - Overhauled tx dma. Multiple packets are now chained in a single dma
>>>>   operation if xmit_more is set, improving performance.
>>>> - Improved rx irq handling. It's not possible to disable interrupts
>>>>   entirely for napi poll, but they can be slowed down a little.
>>>> - Use readx_poll_timeout in various places
>>>> - Improved error detection
>>>> - Improved statistics
>>>> - Report hardware statistics counters through ethtool
>>>> - Improved tangox-specific setup
>>>> - Support for flow control using pause frames
>>>> - Explanatory comments added
>>>> - Various minor stylistic changes
>>>> ---
>>>>  drivers/net/ethernet/Kconfig         |    1 +
>>>>  drivers/net/ethernet/Makefile        |    1 +
>>>>  drivers/net/ethernet/aurora/Kconfig  |   20 +
>>>>  drivers/net/ethernet/aurora/Makefile |    1 +
>>>>  drivers/net/ethernet/aurora/nb8800.c | 1530 ++++++++++++++++++++++++++++++++++
>>>>  drivers/net/ethernet/aurora/nb8800.h |  314 +++++++
>>>>  6 files changed, 1867 insertions(+)
>>>
>>> The code has grown much since the previous patch, despite some
>>> refactoring. Is this mostly due to ethtool_ops support?
>>>
>>>  drivers/net/ethernet/aurora/nb8800.c | 1146 ++++++++++++++++++++++++++++++++++
>>>  drivers/net/ethernet/aurora/nb8800.h |  230 +++++++
>> 
>> Some of the increase is from new features, some from improvements, and
>> then there are a bunch of new comments.
>
> Sweet.
>
> With this version, my kernel boots faster than before
> (I had been using a 5 month-old version.)
>
> Before:
>
> [    0.613623] tangox-enet 26000.ethernet: SMP86xx internal Ethernet at 0x26000
> [    0.623638] libphy: tangox-mii: probed
> [    0.686527] tangox-enet 26000.ethernet: PHY: found Atheros 8035 ethernet at 0x4
> [    0.697169] tangox-enet 26000.ethernet eth0: MAC address 00:16:e8:02:08:42
> ...
> [    1.306360] Sending DHCP requests ..
> [    4.699969] tangox-enet 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
> [    8.899671] ., OK
> [    8.926343] IP-Config: Got DHCP answer from 172.27.200.1, my address is 172.27.64.49
> ...
> [    8.987327] Freeing unused kernel memory: 168K (c039e000 - c03c8000)
>
> After:
>
> [    0.623526] libphy: nb8800-mii: probed
> [    0.628092] nb8800 26000.ethernet eth0: MAC address 00:16:e8:02:08:42
> ...
> [    4.732948] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
> [    4.752655] Sending DHCP requests ., OK
> [    4.782644] IP-Config: Got DHCP answer from 172.27.200.1, my address is 172.27.64.49
> ...
> [    4.849298] Freeing unused kernel memory: 164K (c039f000 - c03c8000)
>
> The DHCP request is sent later, but the kernel doesn't twiddle its thumbs
> for 4 seconds after the link comes up. Does this come from not probing the
> PHY anymore?

No, that's from properly setting the link state initially down.

> BTW, you're not using the PHY IRQ, right? I think I remember you saying
> it didn't work reliably?

It doesn't seem to be wired up on any of my boards, or there's some
magic required to activate it that I'm unaware of.

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-12 14:04       ` Måns Rullgård
@ 2015-11-12 16:19         ` Mason
  2015-11-12 16:57           ` Måns Rullgård
  0 siblings, 1 reply; 52+ messages in thread
From: Mason @ 2015-11-12 16:19 UTC (permalink / raw)
  To: Mans Rullgard
  Cc: linux-kernel, netdev, Florian Fainelli, Daniel Mack,
	David S. Miller, Fabio Estevam

[ CCing a few knowledgeable people ]

Despite the subject, this is about an Atheros 8035 PHY :-)

On 12/11/2015 15:04, Måns Rullgård wrote:

> Mason wrote:
> 
>> BTW, you're not using the PHY IRQ, right? I think I remember you saying
>> it didn't work reliably?
> 
> It doesn't seem to be wired up on any of my boards, or there's some
> magic required to activate it that I'm unaware of.

Weird. The board schematics for the 1172 show Tango ETH0_MDINT# pin
properly connected to AR8035 INT pin (pin 20).

<Thinking out loud>

http://www.redeszone.net/app/uploads/2014/04/AR8035.pdf

INT pin 20
I/O, D, PD
Interrupt Signal to System; default OD-gate, needs an external
10Kohm pull-up, active low; can be configured to I/O by register,
active high.


4.1.17 Interrupt Enable
Offset: 0x12
Mode: Read/Write
Hardware Reset: 0


Strange... it looks like AT803X_INER and AT803X_INTR_ENABLE refer to
the same "Interrupt Enable" register?

In fact, AT803X_INER_INIT == 0xec00 makes sense for register 0x12:
link success/fail, speed/duplex changed, autoneg error

Looks like at803x_config_intr() is used for 8031, but not for 8035...

Relevant commit:
77a9939426f7a "phy/at8031: enable at8031 to work on interrupt mode"

If I add .config_intr and .ack_interrupt to the 8035 struct, then I get
(also added some traces)

[    0.883517] *** at803x_config_intr: ENABLE
[    1.576108] *** at803x_config_intr: DISABLE
[    1.580467] *** at803x_config_intr: ENABLE
[    1.584959] *** at803x_config_intr: DISABLE
[    1.589297] *** at803x_config_intr: ENABLE
[    4.321722] *** at803x_config_intr: DISABLE
[    4.326054] *** at803x_config_intr: ENABLE
[    4.330489] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
[    4.338335] *** at803x_config_intr: ENABLE

(Are all the ENABLE/DISABLE events expected?)

And if I unplug/replug the Ethernet cable,

[   71.903051] *** at803x_config_intr: DISABLE
[   71.907410] *** at803x_config_intr: ENABLE
[   71.912232] nb8800 26000.ethernet eth0: Link is Down
[   71.917309] *** at803x_config_intr: ENABLE
[   78.008972] *** at803x_config_intr: DISABLE
[   78.013375] *** at803x_config_intr: ENABLE
[   78.017797] nb8800 26000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
[   78.025702] *** at803x_config_intr: ENABLE

(Are all the ENABLE/DISABLE events expected there too?)

# cat /proc/interrupts 
            CPU0       CPU1       
 18:        107          0      irq0   1 Level     serial
 54:          5          0      irq0  37 Edge      phy_interrupt
 55:       4953          0      irq0  38 Level     eth0
211:       1147        254       GIC  29 Edge      twd


Questions:

Can't at803x_ack_interrupt() just return phy_read(phydev, AT803X_INSR);
Can at803x_config_intr() be used with the 8035
What about AT803X_INER/AT803X_INTR_ENABLE and AT803X_INSR/AT803X_INTR_STATUS

Regards.


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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-12 16:19         ` Mason
@ 2015-11-12 16:57           ` Måns Rullgård
  2015-11-12 17:20             ` Måns Rullgård
  0 siblings, 1 reply; 52+ messages in thread
From: Måns Rullgård @ 2015-11-12 16:57 UTC (permalink / raw)
  To: Mason
  Cc: linux-kernel, netdev, Florian Fainelli, Daniel Mack,
	David S. Miller, Fabio Estevam

Mason <slash.tmp@free.fr> writes:

> [ CCing a few knowledgeable people ]
>
> Despite the subject, this is about an Atheros 8035 PHY :-)
>
> On 12/11/2015 15:04, Måns Rullgård wrote:
>
>> Mason wrote:
>> 
>>> BTW, you're not using the PHY IRQ, right? I think I remember you saying
>>> it didn't work reliably?
>> 
>> It doesn't seem to be wired up on any of my boards, or there's some
>> magic required to activate it that I'm unaware of.
>
> Weird. The board schematics for the 1172 show Tango ETH0_MDINT# pin
> properly connected to AR8035 INT pin (pin 20).

I have a different board.

> <Thinking out loud>
>
> http://www.redeszone.net/app/uploads/2014/04/AR8035.pdf
>
> INT pin 20
> I/O, D, PD
> Interrupt Signal to System; default OD-gate, needs an external
> 10Kohm pull-up, active low; can be configured to I/O by register,
> active high.
>
> 4.1.17 Interrupt Enable
> Offset: 0x12
> Mode: Read/Write
> Hardware Reset: 0
>
> Strange... it looks like AT803X_INER and AT803X_INTR_ENABLE refer to
> the same "Interrupt Enable" register?

Seems like someone missed that it was already defined.

> In fact, AT803X_INER_INIT == 0xec00 makes sense for register 0x12:
> link success/fail, speed/duplex changed, autoneg error
>
> Looks like at803x_config_intr() is used for 8031, but not for 8035...
>
> Relevant commit:
> 77a9939426f7a "phy/at8031: enable at8031 to work on interrupt mode"
>
> If I add .config_intr and .ack_interrupt to the 8035 struct, then I get
> (also added some traces)

I tried that just now, and I get nothing.  What interrupt did you
specify in your device tree?

> Questions:
>
> Can't at803x_ack_interrupt() just return phy_read(phydev, AT803X_INSR);

No, that would return the actual value of the register.  The caller
doesn't care about the value, but should be notified if there was an
error.

> Can at803x_config_intr() be used with the 8035

Probably.  The person who sent the patch for 8031 probably happened to
have that model.

> What about AT803X_INER/AT803X_INTR_ENABLE and AT803X_INSR/AT803X_INTR_STATUS

Accidental duplicates.

-- 
Måns Rullgård
mans@mansr.com

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

* Re: [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
  2015-11-12 16:57           ` Måns Rullgård
@ 2015-11-12 17:20             ` Måns Rullgård
  0 siblings, 0 replies; 52+ messages in thread
From: Måns Rullgård @ 2015-11-12 17:20 UTC (permalink / raw)
  To: Mason
  Cc: linux-kernel, netdev, Florian Fainelli, Daniel Mack,
	David S. Miller, Fabio Estevam

Måns Rullgård <mans@mansr.com> writes:

> Mason <slash.tmp@free.fr> writes:
>
>> [ CCing a few knowledgeable people ]
>>
>> Despite the subject, this is about an Atheros 8035 PHY :-)
>>
>> On 12/11/2015 15:04, Måns Rullgård wrote:
>>
>>> Mason wrote:
>>> 
>>>> BTW, you're not using the PHY IRQ, right? I think I remember you saying
>>>> it didn't work reliably?
>>> 
>>> It doesn't seem to be wired up on any of my boards, or there's some
>>> magic required to activate it that I'm unaware of.
>>
>> Weird. The board schematics for the 1172 show Tango ETH0_MDINT# pin
>> properly connected to AR8035 INT pin (pin 20).
>
> I have a different board.
>
>> <Thinking out loud>
>>
>> http://www.redeszone.net/app/uploads/2014/04/AR8035.pdf
>>
>> INT pin 20
>> I/O, D, PD
>> Interrupt Signal to System; default OD-gate, needs an external
>> 10Kohm pull-up, active low; can be configured to I/O by register,
>> active high.
>>
>> 4.1.17 Interrupt Enable
>> Offset: 0x12
>> Mode: Read/Write
>> Hardware Reset: 0
>>
>> Strange... it looks like AT803X_INER and AT803X_INTR_ENABLE refer to
>> the same "Interrupt Enable" register?
>
> Seems like someone missed that it was already defined.
>
>> In fact, AT803X_INER_INIT == 0xec00 makes sense for register 0x12:
>> link success/fail, speed/duplex changed, autoneg error
>>
>> Looks like at803x_config_intr() is used for 8031, but not for 8035...
>>
>> Relevant commit:
>> 77a9939426f7a "phy/at8031: enable at8031 to work on interrupt mode"
>>
>> If I add .config_intr and .ack_interrupt to the 8035 struct, then I get
>> (also added some traces)
>
> I tried that just now, and I get nothing.  What interrupt did you
> specify in your device tree?

It works with the interrupt set to trigger on rising edge.

-- 
Måns Rullgård
mans@mansr.com

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

end of thread, other threads:[~2015-11-12 17:20 UTC | newest]

Thread overview: 52+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-11-10 16:14 [PATCH v5] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller Mans Rullgard
2015-11-10 17:55 ` Eric Dumazet
2015-11-10 18:05   ` Måns Rullgård
2015-11-10 20:04     ` David Miller
2015-11-10 20:53       ` Måns Rullgård
2015-11-10 21:06         ` David Miller
2015-11-10 21:21           ` Måns Rullgård
2015-11-10 21:51             ` Eric Dumazet
2015-11-11 13:41               ` Mason
2015-11-11 13:54                 ` Måns Rullgård
2015-11-11 14:10                   ` Eric Dumazet
2015-11-11 14:06                 ` Eric Dumazet
2015-11-10 17:58 ` Eric Dumazet
2015-11-10 18:07   ` Måns Rullgård
2015-11-10 19:13 ` Mason
2015-11-10 19:25   ` Måns Rullgård
2015-11-12 13:33     ` Mason
2015-11-12 14:04       ` Måns Rullgård
2015-11-12 16:19         ` Mason
2015-11-12 16:57           ` Måns Rullgård
2015-11-12 17:20             ` Måns Rullgård
2015-11-10 22:09 ` Andy Shevchenko
2015-11-10 22:34   ` Måns Rullgård
2015-11-10 22:40     ` Andy Shevchenko
2015-11-10 23:07       ` Måns Rullgård
2015-11-11  0:36         ` Andy Shevchenko
2015-11-11  0:44           ` Måns Rullgård
2015-11-10 23:34 ` Francois Romieu
2015-11-11  0:40   ` Måns Rullgård
2015-11-11  2:11     ` David Miller
2015-11-11 12:22       ` Måns Rullgård
2015-11-11 13:04         ` Måns Rullgård
2015-11-11 13:29           ` Eric Dumazet
2015-11-11 13:48             ` Måns Rullgård
2015-11-11 14:09               ` Eric Dumazet
2015-11-11 14:15                 ` Måns Rullgård
2015-11-11 14:35                   ` Måns Rullgård
2015-11-11 14:44                     ` Eric Dumazet
2015-11-11 14:42                   ` Eric Dumazet
2015-11-11 16:24           ` David Miller
2015-11-11 18:25             ` Måns Rullgård
2015-11-11 19:02               ` David Miller
2015-11-11 19:09                 ` Måns Rullgård
2015-11-11 19:13                   ` David Miller
2015-11-11 19:17                     ` Måns Rullgård
2015-11-11 19:19                       ` David Miller
2015-11-11 19:25                         ` Måns Rullgård
2015-11-11 19:26                           ` David Miller
2015-11-11 19:35                             ` Måns Rullgård
2015-11-11 19:48                               ` David Miller
2015-11-11 20:47                                 ` Måns Rullgård
2015-11-11 16:20         ` David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.