[PATCH 1/4] net: mvneta: driver for Marvell Armada 370/XP network unit

Hello Thomas,

The overall driver looks very nice, my biggest concern with this driver being 
that it does not implement phylib and therefore reimplements a bit of existing 
code. I am not commentin on how to represent this MDIO/PHY devices using 
Device Tree since this has been addressed already.

Once you register a MDIO bus for your interface, please make sure that you 
give it a unique name in the system (<pdev->name>-<pdev->id>).

Other comments inline.

On Tuesday 04 September 2012 15:06:41 Thomas Petazzoni wrote:
[snip]

+
+/* Increment txq get counter */
+static void mvneta_inc_get(struct mvneta_tx_queue *txq)
+{
+	txq->txq_get_index++;
+	if (txq->txq_get_index == txq->size)
+		txq->txq_get_index = 0;
+}
+
+/* Increment txq put counter */
+static void mvneta_inc_put(struct mvneta_tx_queue *txq)
+{
+	txq->txq_put_index++;
+	if (txq->txq_put_index == txq->size)
+		txq->txq_put_index = 0;
+}

I would make it clear that these helpers operate on the txq, and suffix it with 
_txq.

+
+
+/* Clear all MIB counters */
+static void mvneta_mib_counters_clear(struct mvneta_port *pp)
+{
+	int i;
+	u32 dummy;
+
+	/* Perform dummy reads from MIB counters */
+	for (i = 0; i < MVNETA_MIB_LATE_COLLISION; i += 4)
+		dummy = mvreg_read(pp, (MVNETA_MIB_COUNTERS_BASE + i));
+}
+
+/* Read speed, duplex, and flow control from port status register */
+static int mvneta_link_status(struct mvneta_port *pp,
+			      struct mvneta_lnk_status *status)
+{
+	u32 val;
+
+	val = mvreg_read(pp, MVNETA_GMAC_STATUS);
+
+	if (val & MVNETA_GMAC_SPEED_1000_MASK)
+		status->speed = MVNETA_SPEED_1000;
+	else if (val & MVNETA_GMAC_SPEED_100_MASK)
+		status->speed = MVNETA_SPEED_100;
+	else
+		status->speed = MVNETA_SPEED_10;
+
+	if (val & MVNETA_GMAC_LINK_UP_MASK)
+		status->linkup = 1;
+	else
+		status->linkup = 0;
+
+	if (val & MVNETA_GMAC_FULL_DUPLEX_MASK)
+		status->duplex = MVNETA_DUPLEX_FULL;
+	else
+		status->duplex = MVNETA_DUPLEX_HALF;
+
+	if (val & MVNETA_GMAC_TX_FLOW_CTRL_ACTIVE_MASK)
+		status->tx_fc = MVNETA_FC_ACTIVE;
+	else if (val & MVNETA_GMAC_TX_FLOW_CTRL_ENABLE_MASK)
+		status->tx_fc = MVNETA_FC_ENABLE;
+	else
+		status->tx_fc = MVNETA_FC_DISABLE;
+
+	if (val & MVNETA_GMAC_RX_FLOW_CTRL_ACTIVE_MASK)
+		status->rx_fc = MVNETA_FC_ACTIVE;
+	else if (val & MVNETA_GMAC_RX_FLOW_CTRL_ENABLE_MASK)
+		status->rx_fc = MVNETA_FC_ENABLE;
+	else
+		status->rx_fc = MVNETA_FC_DISABLE;
+

I would rather see you use a struct phy_device and update its properties 
instead of keeping a local copy of it. This would allow you to have consistent 
reporting through ethtool, I have more comments on this later on as well.

+static void mvneta_rxq_non_occup_desc_add(struct mvneta_port *pp,
+					  struct mvneta_rx_queue *rxq,
+					  int rx_desc)
+{
+	u32 val;
+
+	/* Only 255 descriptors can be added at once */
+	while (rx_desc > 0xff) {
+		val = (0xff << MVNETA_RXQ_ADD_NON_OCCUPIED_OFFS);
+		mvreg_write(pp, MVNETA_RXQ_STATUS_UPDATE_REG(rxq->id), val);
+		rx_desc = rx_desc - 0xff;
+	}

You could probably use a define here for 255/0xff.

[snip]

+	m_delay = 0;

This does not look like an useful name, count would be better

+	do {
+		if (m_delay >= MVNETA_RX_DISABLE_TIMEOUT_MSEC) {
+			netdev_info(pp->dev,
+				"TIMEOUT for RX stopped ! rx_queue_cmd: 0x08%x\n",
+				val);

Please use a different logging level such as netdev_err() or netdev_warn() for 
instance.

+			break;
+		}
+		mdelay(1);
+		m_delay++;

What about using msleep() instead here?

+
+		val = mvreg_read(pp, MVNETA_RXQ_CMD);
+	} while (val & 0xff);
+
+	/* Stop Tx port activity. Check port Tx activity. Issue stop
+	   command for active channels only  */
+	val = (mvreg_read(pp, MVNETA_TXQ_CMD)) & MVNETA_TXQ_ENABLE_MASK;
+
+	if (val != 0)
+		mvreg_write(pp, MVNETA_TXQ_CMD,
+			(val << MVNETA_TXQ_DISABLE_OFFS));
+
+	/* Wait for all Tx activity to terminate. */
+	m_delay = 0;
+	do {
+		if (m_delay >= MVNETA_TX_DISABLE_TIMEOUT_MSEC) {
+			netdev_info(pp->dev,
+				"TIMEOUT for TX stopped tx_queue_cmd - 0x%08x\n",
+				val);
+			break;
+		}
+		mdelay(1);
+		m_delay++;
+
+		/* Check TX Command reg that all Txqs are stopped */
+		val = mvreg_read(pp, MVNETA_TXQ_CMD);

Ditto

+
+	} while (val & 0xff);
+	tx_fifo_empty_mask |= MVNETA_TX_FIFO_EMPTY_MASK;
+	tx_in_prog_mask    |= MVNETA_TX_IN_PRGRS_MASK;
+
+	/* Double check to verify that TX FIFO is empty */
+	m_delay = 0;
+	while (1) {
+		do {
+			if (m_delay >= MVNETA_TX_FIFO_EMPTY_TIMEOUT) {
+				netdev_info(pp->dev,
+					    "TX FIFO empty timeout status=0x08%x, 

empty=%x, in_prog=%x",

+					    val, tx_fifo_empty_mask,
+					    tx_in_prog_mask);
+				break;
+			}
+			mdelay(1);
+			m_delay++;

Ditto

+
+			val = mvreg_read(pp, MVNETA_PORT_STATUS);
+		} while (((val & tx_fifo_empty_mask) != tx_fifo_empty_mask)
+			 || ((val & tx_in_prog_mask) != 0));
+
+		if (m_delay >= MVNETA_TX_FIFO_EMPTY_TIMEOUT)
+			break;
+
+		val = mvreg_read(pp, MVNETA_PORT_STATUS);
+		if (((val & tx_fifo_empty_mask) == tx_fifo_empty_mask) &&
+		    ((val & tx_in_prog_mask) == 0))
+			break;
+		else
+			netdev_info(pp->dev, "TX FIFO Empty double check failed. %d 

msec status=0x%x, empty=0x%x, in_prog=0x%x\n",

+				    m_delay, val, tx_fifo_empty_mask,
+				    tx_in_prog_mask);
+	}
+
+	udelay(200);
+}

[snip]

+
+/* This method sets defaults to the NETA port:
+ *	Clears interrupt Cause and Mask registers.
+ *	Clears all MAC tables.
+ *	Sets defaults to all registers.
+ *	Resets RX and TX descriptor rings.
+ *	Resets PHY.
+ * This method can be called after mvneta_port_down() to return the port
+ *	settings to defaults.
+ */

Please use standard kernel-doc style comments.

[snip]

+/* Read the Link Up bit (LinkUp) in port MAC control register */
+static int mvneta_link_is_up(struct mvneta_port *pp)
+{
+	u32 val;
+	val = mvreg_read(pp, MVNETA_GMAC_STATUS);
+	if (val & MVNETA_GMAC_LINK_UP_MASK)
+		return 1;

	return mvreg_read(pp, MVNETA_GMAC_STATUS) & MVNETA_GMAC_LINK_UP_MASK;
and make it static inline.

+
+	return 0;
+}
+
+/* Get phy address */
+static int mvneta_phy_addr_get(struct mvneta_port *pp)
+{
+	unsigned int val;
+
+	val = mvreg_read(pp, MVNETA_PHY_ADDR);
+	val &= 0x1f;

Use PHY_MAX_ADDR - 1 instead here.

+	return val;
+}
+

[snip]

+/* Display status (link, duplex, speed) of the port */
+void mvneta_link_status_print(struct mvneta_port *pp)
+{
+	struct mvneta_lnk_status link;
+	char *speedstr, *duplexstr;
+
+	mvneta_link_status(pp, &link);
+
+	if (link.linkup) {
+		if (link.speed == MVNETA_SPEED_1000)
+			speedstr = "1 Gbps";
+		else if (link.speed == MVNETA_SPEED_100)
+			speedstr = "100 Mbps";
+		else
+			speedstr = "10 Mbps";
+
+		if (link.duplex == MVNETA_DUPLEX_FULL)
+			duplexstr = "full";
+		else
+			duplexstr = "half";
+
+		netdev_info(pp->dev,
+			    "link up, %s duplex, speed %s\n",
+			    duplexstr, speedstr);
+	} else
+		netdev_info(pp->dev, "link down\n");
+}

You should rather define a phylib adjust_link callback to do this. Otherwise 
please reduce the indentation by handling the case when the link is down first.

+
+/* Display more error info */
+static void mvneta_rx_error(struct mvneta_port *pp,
+			    struct mvneta_rx_desc *rx_desc)
+{
+	u32 status = rx_desc->status;
+
+	if (pp->dev)
+		pp->dev->stats.rx_errors++;

Please do this outside of this function and just let it print the error.

+
+	if ((status & MVNETA_RXD_FIRST_LAST_DESC_MASK)
+	    != MVNETA_RXD_FIRST_LAST_DESC_MASK) {
+		netdev_err(pp->dev,
+			   "bad rx status %08x (buffer oversize), size=%d\n",
+			   rx_desc->status, rx_desc->data_size);
+		return;
+	}

[snip]

+
+/* Refill processing */
+static int mvneta_rx_refill(struct mvneta_port *pp,
+			    struct mvneta_rx_desc *rx_desc)
+
+{
+	unsigned long phys_addr;
+	struct sk_buff *skb;
+
+	skb = netdev_alloc_skb(pp->dev, pp->pkt_size);
+	if (!skb) {
+		mvneta_add_cleanup_timer(pp);
+		return 1;
+	}
+
+	phys_addr = dma_map_single(pp->dev->dev.parent, skb->head,
+				   MVNETA_RX_BUF_SIZE(pp->pkt_size),
+				   DMA_FROM_DEVICE);

Check that your phys_addr cookie has been successfully mapped using 
dma_mapping_error().

[snip]

+/* Main tx processing */
+static int mvneta_tx(struct sk_buff *skb, struct net_device *dev)
+{
+	struct mvneta_port *pp = netdev_priv(dev);
+
+	int frags = 0;
+	int res = NETDEV_TX_OK;
+	u32 tx_cmd;
+	struct mvneta_tx_queue *txq = NULL;
+	struct mvneta_tx_desc *tx_desc;
+
+	if (!test_bit(MVNETA_F_STARTED_BIT, &pp->flags))
+		goto out;

Is not this equivalent to !netif_running(dev)? At least print some message so 
we know that this is not supposed to happen.

+
+	txq = &pp->txqs[mvneta_txq_def];
+
+	frags = skb_shinfo(skb)->nr_frags + 1;
+
+	tx_desc = mvneta_tx_desc_get(pp, txq, frags);
+	if (tx_desc == NULL) {
+		frags = 0;
+		dev->stats.tx_dropped++;
+		res = NETDEV_TX_BUSY;
+		goto out;
+	}

+
+	tx_cmd = mvneta_skb_tx_csum(pp, skb);
+
+	tx_desc->data_size = skb_headlen(skb);
+
+	tx_desc->buf_phys_addr = dma_map_single(dev->dev.parent, skb->data,
+						tx_desc->data_size,
+						DMA_TO_DEVICE);

Please check this dma_map_single() return value too.

[snip]

+static int mvneta_addr_crc(unsigned char *addr)
+{
+	int crc = 0;
+	int i;
+
+	for (i = 0; i < 6; i++) {

ETH_ALEN instead of 6 to make it clear it operates on addresses.

+		int j;
+
+		crc = (crc ^ addr[i]) << 8;
+		for (j = 7; j >= 0; j--) {
+			if (crc & (0x100 << j))
+				crc ^= 0x107 << j;
+		}
+	}
+
+	return crc;
+}

[snip]

+
+/* Interrupt handling - the callback for request_irq() */
+static irqreturn_t mvneta_isr(int irq, void *dev_id)
+{
+	struct mvneta_port *pp = (struct mvneta_port *)dev_id;
+
+	/* Mask all interrupts */
+	mvreg_write(pp, MVNETA_INTR_NEW_MASK, 0);
+
+	/* Verify that the device not already on the polling list */
+	if (napi_schedule_prep(&pp->napi))
+		__napi_schedule(&pp->napi);

Does not the hardware generate interrupts for tx completion, PHY interrupts 
etc ...?

+
+	return IRQ_HANDLED;
+}
+
+/* Handle link event */
+static void mvneta_link_event(struct mvneta_port *pp)
+{
+	struct net_device *dev = pp->dev;
+
+	/* Check Link status on ethernet port */
+
+	if (mvneta_link_is_up(pp)) {
+		mvneta_port_up(pp);
+		set_bit(MVNETA_F_LINK_UP_BIT, &pp->flags);
+
+		if (dev) {
+			netif_carrier_on(dev);
+			netif_tx_wake_all_queues(dev);
+		}
+	} else {
+		if (dev) {
+			netif_carrier_off(dev);
+			netif_tx_stop_all_queues(dev);
+		}
+		mvneta_port_down(pp);
+		clear_bit(MVNETA_F_LINK_UP_BIT, &pp->flags);
+	}
+
+	mvneta_link_status_print(pp);

Again, this is taken care of by phylib nicely, and does not require you to 
have this F_LINK_UP_BIT.

[snip]

+
+/* Handle rxq fill: allocates rxq skbs; called when initializing a port */
+static int mvneta_rxq_fill(struct mvneta_port *pp, struct mvneta_rx_queue 

*rxq,

+			   int num)
+{
+	int i;
+	struct sk_buff *skb;
+	struct mvneta_rx_desc *rx_desc;
+	unsigned long phys_addr;
+	struct net_device *dev = pp->dev;
+
+	for (i = 0; i < num; i++) {
+		skb = dev_alloc_skb(pp->pkt_size);
+		if (!skb) {
+			netdev_err(pp->dev, "%s:rxq %d, %d of %d buffs filled\n",
+				   __func__, rxq->id, i, num);
+			break;
+		}
+
+		rx_desc = rxq->descs + i;
+		memset(rx_desc, 0, sizeof(struct mvneta_rx_desc));
+		phys_addr = dma_map_single(dev->dev.parent, skb->head,
+					   MVNETA_RX_BUF_SIZE(pp->pkt_size),
+					   DMA_FROM_DEVICE);

Here again, check phys_addr.

+		mvneta_rx_desc_fill(rx_desc, phys_addr, (u32)skb);
+	}
+
+	/* add this num of RX descriptors as non occupied (ready to get pkts) */
+	mvneta_rxq_non_occup_desc_add(pp, rxq, i);
+
+	return i;
+}
+

[snip]

+
+/* Create a specified RX queue */
+static int mvneta_rxq_init(struct mvneta_port *pp,
+			   struct mvneta_rx_queue *rxq)
+
+{
+	rxq->size = pp->rx_ring_size;
+
+	/* Allocate DMA descriptors array */
+	rxq->descs_orig = dma_alloc_coherent(pp->dev->dev.parent,
+					     MVNETA_RX_TOTAL_DESCS_SIZE(rxq),
+					     &rxq->descs_phys_orig,
+					     GFP_KERNEL);
+	if (rxq->descs_orig == NULL) {

Use dma_mapping_error() instead.

+		netdev_err(pp->dev, "rxQ=%d: Can't allocate %d bytes for %d RX 

descr\n",

+			   rxq->id, MVNETA_RX_TOTAL_DESCS_SIZE(rxq), rxq->size);
+		return -ENOMEM;
+	}
+
+	/* Make sure descriptor address is cache line size aligned  */
+	rxq->descs = PTR_ALIGN(rxq->descs_orig, MVNETA_CPU_D_CACHE_LINE_SIZE);
+	rxq->descs_phys = ALIGN(rxq->descs_phys_orig,
+				MVNETA_CPU_D_CACHE_LINE_SIZE);
+
+	rxq->last_desc = rxq->size - 1;

Don't you need some kind of barrier here? I do not know exactly how coherent 
your peripherals and memory are, just wondering.

+
+	/* Set Rx descriptors queue starting address */
+	mvreg_write(pp, MVNETA_RXQ_BASE_ADDR_REG(rxq->id), rxq->descs_phys);
+	mvreg_write(pp, MVNETA_RXQ_SIZE_REG(rxq->id), rxq->size);
+
+	/* Set Offset */
+	mvneta_rxq_offset_set(pp, rxq, NET_SKB_PAD);
+
+	/* Set coalescing pkts and time */
+	mvneta_rx_pkts_coal_set(pp, rxq, rxq->pkts_coal);
+	mvneta_rx_time_coal_set(pp, rxq, rxq->time_coal);
+
+	/* Fill RXQ with buffers from RX pool */
+	mvneta_rxq_buf_size_set(pp, rxq, MVNETA_RX_BUF_SIZE(pp->pkt_size));
+	mvneta_rxq_bm_disable(pp, rxq);
+	mvneta_rxq_fill(pp, rxq, rxq->size);
+
+	return 0;
+}
+

[snip]

+static int mvneta_txq_init(struct mvneta_port *pp,
+			   struct mvneta_tx_queue *txq)
+{
+	txq->size = pp->tx_ring_size;
+
+	/* Allocate DMA descriptors array */
+	txq->descs_orig = dma_alloc_coherent(pp->dev->dev.parent,
+					     MVNETA_TX_TOTAL_DESCS_SIZE(txq),
+					     &txq->descs_phys_orig,
+					     GFP_KERNEL);
+	if (txq->descs_orig == NULL) {

Use dma_mapping_error().

+		netdev_err(pp->dev, "txQ=%d: Can't allocate %d bytes for %d TX 

descr\n",

+			   txq->id, MVNETA_TX_TOTAL_DESCS_SIZE(txq), txq->size);
+		return -ENOMEM;
+	}
+

[snip]

+/* Fill rx buffers, start Rx/Tx activity, set coalesing,
+*  clear and unmask interrupt bits
+*/
+static int mvneta_start_internals(struct mvneta_port *pp, int mtu)
+{
+	int err = 0;
+
+	pp->pkt_size = MVNETA_RX_PKT_SIZE(mtu);
+	if (test_bit(MVNETA_F_STARTED_BIT, &pp->flags))
+		return -EINVAL;

You probably mean -EBUSY here instead?

+
+	if (mvneta_max_rx_size_set(pp, MVNETA_RX_PKT_SIZE(mtu))) {
+		netdev_err(pp->dev,
+			   "%s: can't set maxRxSize=%d mtu=%d\n",
+			   __func__, MVNETA_RX_PKT_SIZE(mtu), mtu);
+		return -EINVAL;
+	}
+
+	err = mvneta_setup_rxqs(pp);
+	if (unlikely(err))
+		return err;
+
+	err = mvneta_setup_txqs(pp);
+	if (unlikely(err)) {
+		mvneta_cleanup_rxqs(pp);
+		return err;
+	}
+
+	mvneta_txq_max_tx_size_set(pp, MVNETA_RX_PKT_SIZE(mtu));
+
+	/* start the Rx/Tx activity */
+	mvneta_port_enable(pp);
+
+	set_bit(MVNETA_F_LINK_UP_BIT, &pp->flags);
+	set_bit(MVNETA_F_STARTED_BIT, &pp->flags);
+
+	return 0;
+}
+
+/* Stop port Rx/Tx activity, free skb's from Rx/Tx rings */
+static int mvneta_stop_internals(struct mvneta_port *pp)
+{
+	clear_bit(MVNETA_F_STARTED_BIT, &pp->flags);
+
+	/* Stop the port activity */
+	mvneta_port_disable(pp);
+
+	/* Clear all ethernet port interrupts */
+	mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0);
+	mvreg_write(pp, MVNETA_INTR_OLD_CAUSE, 0);
+
+	/* Mask all interrupts */
+	mvneta_interrupts_mask(pp);
+	smp_call_function_many(cpu_online_mask, mvneta_interrupts_mask,
+			       pp, 1);
+
+	/* Reset TX port here. */
+	mvneta_tx_reset(pp);
+
+	mvneta_cleanup_rxqs(pp);
+	mvneta_cleanup_txqs(pp);
+
+	return 0;
+
+}
+
+/* Start the port, connect to port interrupt line, unmask interrupts  */
+static int mvneta_start(struct net_device *dev)
+{
+	struct mvneta_port *pp = netdev_priv(dev);
+
+	/* In default link is down */
+	netif_carrier_off(dev);
+	netif_tx_stop_all_queues(dev);
+
+	/* Fill rx buffers, start Rx/Tx activity, set coalescing */
+	if (mvneta_start_internals(pp, dev->mtu) != 0) {
+		netdev_err(dev, "start internals failed\n");
+		return -ENODEV;
+	}
+
+	/* Enable polling on the port, must be used after netif_poll_disable */
+	napi_enable(&pp->napi);
+
+	if (pp->flags & MVNETA_F_LINK_UP) {
+		netif_carrier_on(dev);
+		netif_tx_wake_all_queues(dev);
+	} else {
+		netdev_info(dev, "%s: NOT MVNETA_F_LINK_UP\n", __func__);
+	}

Remove this message as well.

+
+	/* Connect to port interrupt line */
+	if (request_irq(dev->irq, mvneta_isr, (IRQF_DISABLED), "mv_eth", pp)) {
+		netdev_err(dev, "cannot request irq %d\n", dev->irq);
+		napi_disable(&pp->napi);
+		goto error;
+	}

You should probably request the interrupt prior to calling napi_enable()

+
+	/* Unmask interrupts */
+	mvneta_interrupts_unmask(pp);
+	smp_call_function_many(cpu_online_mask,
+			       mvneta_interrupts_unmask,
+			       pp, 1);
+
+	netdev_info(dev, "started\n");

Remove this please.

+	return 0;
+
+error:
+	netdev_err(dev, "start failed\n");
+	mvneta_cleanup_rxqs(pp);
+	mvneta_cleanup_txqs(pp);
+
+	return -ENODEV;
+}
+

+	if (dev->irq != 0)
+		free_irq(dev->irq, pp);

This looks superfluous, you refuse to bring up the interface if the interrupt 
requesting fails.

+
+	netdev_info(dev, "stopped\n");
+
+	return 0;
+}
+
+
+/* tx timeout callback - display a message and stop/start the network 

device */

+static void mvneta_tx_timeout(struct net_device *dev)
+{
+	netdev_info(dev, "tx timeout\n");
+	if (netif_running(dev)) {
+		mvneta_stop(dev);
+		mvneta_start(dev);
+	}

You should never end-up with the case where the interface is not running and 
you face a transmit timeout.

[snip]

+/* Change the device mtu */
+static int mvneta_change_mtu(struct net_device *dev, int mtu)
+{
+	int old_mtu = dev->mtu;
+
+	mtu = mvneta_check_mtu_valid(dev, mtu);
+	if (mtu < 0)
+		return -EINVAL;
+
+	dev->mtu = mtu;
+
+	if (!netif_running(dev)) {
+		netdev_info(dev, "change mtu %d (buffer-size %d) to %d (buffer-size 

%d)\n",

+			old_mtu, MVNETA_RX_PKT_SIZE(old_mtu),
+			dev->mtu, MVNETA_RX_PKT_SIZE(dev->mtu));
+		return 0;

Remove this message.

+	}
+
+	if (mvneta_stop(dev)) {
+		netdev_err(dev, "stop interface failed\n");
+		goto error;
+	}
+
+	if (mvneta_start(dev)) {
+		netdev_err(dev, "start interface failed\n");
+		goto error;
+	}

Propagate the returned error codes back to the caller.

+
+	netdev_info(dev, "change mtu %d (buffer-size %d) to %d (buffer-size %d)\n",
+		old_mtu, MVNETA_RX_PKT_SIZE(old_mtu),
+		dev->mtu, MVNETA_RX_PKT_SIZE(dev->mtu));

Remove this message too.

+
+	return 0;
+
+error:
+	netdev_info(dev, "change mtu failed\n");
+	return -EINVAL;
+}
+
+/* Handle setting mac address (low level) */
+static int mvneta_set_mac_addr_internals(struct net_device *dev, void 

*addr)

+{
+	struct mvneta_port *pp = netdev_priv(dev);
+	u8 *mac = addr + 2;
+	int i;
+
+	/* Remove previous address table entry */
+	if (mvneta_mac_addr_set(pp, dev->dev_addr, -1) != 0) {
+		netdev_err(dev, "mvneta_mac_addr_set failed\n");
+		return -EINVAL;
+	}
+
+	/* Set new addr in hw */
+	if (mvneta_mac_addr_set(pp, mac, mvneta_rxq_def) != 0) {
+		netdev_err(dev, "mvneta_mac_addr_set failed\n");
+		return -EINVAL;
+	}
+
+	/* Set addr in the device */
+	for (i = 0; i < MVNETA_MAC_ADDR_SIZE; i++)
+		dev->dev_addr[i] = mac[i];

ETH_ALEN.

+
+	netdev_info(dev, "mac address changed\n");

Remove this please.

+
+	return 0;
+}
+
+/* Handle setting mac address */
+static int mvneta_set_mac_addr(struct net_device *dev, void *addr)
+{
+	if (!netif_running(dev)) {
+		if (mvneta_set_mac_addr_internals(dev, addr) == -1)
+			goto error;
+		return 0;
+	}

Usually you just check if the interface is running, and if it is return 
something like -EBUSY.

+
+	if (mvneta_stop(dev)) {
+		netdev_err(dev, "stop interface failed\n");
+		goto error;
+	}
+
+	if (mvneta_set_mac_addr_internals(dev, addr) == -1)
+		goto error;
+
+	if (mvneta_start(dev)) {
+		netdev_err(dev, "start interface failed\n");
+		goto error;
+	}

Propagate error codes here too please.

+
+	return 0;
+
+error:
+	netdev_err(dev, "set mac addr failed\n");
+	return -EINVAL;
+}
+
+/*
+ * Called when a network interface is made active.
+ * Returns 0 on success, -EINVAL or =ENODEV on failure
+ * mvneta_open() is called when a network interface is made
+ * active by the system (IFF_UP). We set the mac address and
+ * invoke mvneta_start() to start the device.
+ */
+static int mvneta_open(struct net_device *dev)
+{
+	struct mvneta_port *pp = netdev_priv(dev);
+	int queue = mvneta_rxq_def;
+
+	if (mvneta_mac_addr_set(pp, dev->dev_addr, queue) != 0) {
+		netdev_err(dev, "mvneta_mac_addr_set failed\n");
+		return -EINVAL;
+	}
+
+	if (mvneta_start(dev)) {
+		netdev_err(dev, "start interface failed\n");
+		return -ENODEV;
+	}
+
+	return 0;

Propagate the error code here too.

[snip]

+static void mvneta_ethtool_get_drvinfo(struct net_device *dev,
+				    struct ethtool_drvinfo *drvinfo)
+{
+	strlcpy(drvinfo->driver, mvneta_driver_name,
+		sizeof(drvinfo->driver));
+	strlcpy(drvinfo->version, mvneta_driver_version,
+		sizeof(drvinfo->version));

You can probably also provide informations about the firmware version, bus_info 
at least.

[snip]

+/* Device initialization routine */
+static int __devinit mvneta_probe(struct platform_device *pdev)
+{
+	int err = -EINVAL;
+	struct mvneta_port *pp;
+	struct net_device *dev;
+	u32 phy_addr, clk;
+	int phy_mode;
+	const char *mac_addr;
+	const struct mbus_dram_target_info *dram_target_info;
+	struct device_node *dn = pdev->dev.of_node;
+
+	dev = alloc_etherdev_mq(sizeof(struct mvneta_port), 8);
+	if (!dev)
+		return -ENOMEM;
+
+	dev->irq = irq_of_parse_and_map(dn, 0);
+	if (dev->irq == 0) {
+		err = -EINVAL;
+		goto err_irq;
+	}
+
+	if (of_property_read_u32(dn, "phy-addr", &phy_addr) != 0) {
+		dev_err(&pdev->dev, "could not read phy_addr\n");
+		err = -ENODEV;
+		goto err_node;
+	}
+
+	phy_mode = of_get_phy_mode(dn);
+	if (phy_mode < 0) {
+		dev_err(&pdev->dev, "wrong phy-mode\n");
+		err = -EINVAL;
+		goto err_node;
+	}
+
+	if (of_property_read_u32(dn, "clock-frequency", &clk) != 0) {
+		dev_err(&pdev->dev, "could not read clock-frequency\n");
+		err = -EINVAL;
+		goto err_node;
+	}
+
+	mac_addr = of_get_mac_address(dn);
+
+	if (!mac_addr || !is_valid_ether_addr(mac_addr))
+		eth_hw_addr_random(dev);
+	else
+		memcpy(dev->dev_addr, mac_addr, 6);
+
+	dev->tx_queue_len = MVNETA_MAX_TXD;
+	dev->watchdog_timeo = 5 * HZ;
+	dev->netdev_ops = &mvneta_netdev_ops;
+
+	SET_ETHTOOL_OPS(dev, &mvneta_eth_tool_ops);
+
+	pp = netdev_priv(dev);
+
+	pp->tx_done_timer.function = mvneta_tx_done_timer_callback;
+	init_timer(&pp->tx_done_timer);
+	clear_bit(MVNETA_F_TX_DONE_TIMER_BIT, &pp->flags);
+	pp->cleanup_timer.function = mvneta_cleanup_timer_callback;
+	init_timer(&pp->cleanup_timer);
+	clear_bit(MVNETA_F_CLEANUP_TIMER_BIT, &pp->flags);
+
+	pp->weight = MVNETA_RX_POLL_WEIGHT;
+	pp->clk = clk;

Rename this clk_freq so make it less ambiguous, because this is not a proper 
struct clk pointer.

+
+	pp->base = of_iomap(dn, 0);
+	if (pp->base == NULL) {
+		err = -ENOMEM;
+		goto err_node;
+	}
+
+	pp->tx_done_timer.data = (unsigned long)dev;
+	pp->cleanup_timer.data = (unsigned long)dev;
+
+	pp->tx_ring_size = MVNETA_MAX_TXD;
+	pp->rx_ring_size = MVNETA_MAX_RXD;
+
+	pp->dev = dev;
+
+	if (mvneta_init(pp, phy_addr)) {
+		dev_err(&pdev->dev, "can't init eth hal\n");
+		err = -ENODEV;
+		goto err_base;
+	}
+	mvneta_port_power_up(pp, phy_mode);
+
+	dram_target_info = mv_mbus_dram_info();
+	if (dram_target_info)
+		mvneta_conf_mbus_windows(pp, dram_target_info);
+
+	netif_napi_add(dev, &pp->napi, mvneta_poll, pp->weight);
+
+	SET_NETDEV_DEV(dev, &pdev->dev);
+
+	if (register_netdev(dev)) {
+		dev_err(&pdev->dev, "failed to register\n");
+		err = ENOMEM;
+		goto err_base;
+	}
+
+	dev->features = NETIF_F_SG;
+	dev->hw_features =  NETIF_F_SG;
+	dev->priv_flags |= IFF_UNICAST_FLT;
+
+	if (dev->mtu <= MVNETA_TX_CSUM_MAX_SIZE) {
+		dev->features |= NETIF_F_IP_CSUM;
+		dev->hw_features |= NETIF_F_IP_CSUM;
+	}

At this point, the condition is always true, so just set these features and 
update them when the MTU changes.

+
+	dev_info(&pdev->dev, "%s, mac: %pM pp->base=%p\n", dev->name,
+		 dev->dev_addr, pp->base);
+
+	platform_set_drvdata(pdev, pp->dev);
+
+	return 0;
+err_base:
+	iounmap(pp->base);
+err_node:
+	irq_dispose_mapping(dev->irq);
+err_irq:
+	free_netdev(dev);
+	return err;
+}
+
+/* Device removal routine */
+static int __devexit mvneta_remove(struct platform_device *pdev)
+{
+	struct net_device  *dev = platform_get_drvdata(pdev);
+	struct mvneta_port *pp = netdev_priv(dev);
+
+	dev_info(&pdev->dev, "Removing Marvell Ethernet Driver\n");

I would remove this message.