All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3] ethernet/arc/arc_emac - Add new driver
@ 2013-06-13 14:37 ` Alexey Brodkin
  0 siblings, 0 replies; 21+ messages in thread
From: Alexey Brodkin @ 2013-06-13 14:37 UTC (permalink / raw)
  To: netdev
  Cc: Alexey Brodkin, Andy Shevchenko, Francois Romieu, Joe Perches,
	Vineet Gupta, Mischa Jonker, Arnd Bergmann, Grant Likely,
	Rob Herring, Paul Gortmaker, David S. Miller, linux-kernel,
	devicetree-discuss

Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
instantiated in some legacy ARC (Synopsys) FPGA Boards such as
ARCAngel4/ML50x.

This is based off of current Linus tree.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
Reviewed-by: Vineet Gupta <vgupta@synopsys.com>
Reviewed-by: Mischa Jonker <mjonker@synopsys.com>

Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Francois Romieu <romieu@fr.zoreil.com>
Cc: Joe Perches <joe@perches.com>
Cc: Vineet Gupta <vgupta@synopsys.com>
Cc: Mischa Jonker <mjonker@synopsys.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-kernel@vger.kernel.org
Cc: devicetree-discuss@lists.ozlabs.org

---
Hi all,

this is 3rd re-spin of my patch.

It introduces pretty significant changes in comparison to v2.
Those changes were inspiresd by very useful comments of:
 * Andy Shevchenko
 * Francois Romieu
 * Joe Perches

I hope now driver looks simpler, cleaner and better complies to the most
recent "standards" for network drivers in Linux kernel.

Below is a list of main changes in this respin.

1. Functional changes:
1.1. Heavy re-work of "arc_emac_poll", "arc_emac_tx" and
"arc_emac_intr". Now they look simpler and implement most of bullets
below.
1.2. Properly support DMA-API: dma_{map|unmap}_single, error checking
etc.
1.3. Rework "arc_emac_set_address" to be endian-friendly.
1.4. Use "netdev_alloc_skb_ip_align" instead of "netdev_alloc_skb" +
"skb_reserve".
1.5. Move PHY connection from "arc_emac_open" to "arc_emac_probe".
1.6. Merge EMAC and MDIO private structures into single one.
1.7. Add use of "netif_{stop|resume}_queue".
1.8. Add "max-speed" DT item for EMAC for proper PHY setup. Some systems
have limited memory bandwidth so we need to limit incoming data-rate.
1.9. Convert register accessors from macros into in-lined functions.

2. Cosmetics:
2.1. Remove change-log.
2.2. With inverted logic saved 2 indentation levels.
2.3. Merge multi-line text comments into 1 line.
2.4. Use "ndev" instead of "net_dev".
2.5. All functions in "arc_emac_main.c" are "static" now.
---
 Documentation/devicetree/bindings/net/arc_emac.txt |   38 +
 drivers/net/ethernet/Kconfig                       |    1 +
 drivers/net/ethernet/Makefile                      |    1 +
 drivers/net/ethernet/arc/Kconfig                   |   31 +
 drivers/net/ethernet/arc/Makefile                  |    6 +
 drivers/net/ethernet/arc/arc_emac.h                |  201 +++++
 drivers/net/ethernet/arc/arc_emac_main.c           |  813 ++++++++++++++++++++
 drivers/net/ethernet/arc/arc_emac_mdio.c           |  152 ++++
 8 files changed, 1243 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/arc_emac.txt
 create mode 100644 drivers/net/ethernet/arc/Kconfig
 create mode 100644 drivers/net/ethernet/arc/Makefile
 create mode 100644 drivers/net/ethernet/arc/arc_emac.h
 create mode 100644 drivers/net/ethernet/arc/arc_emac_main.c
 create mode 100644 drivers/net/ethernet/arc/arc_emac_mdio.c

diff --git a/Documentation/devicetree/bindings/net/arc_emac.txt b/Documentation/devicetree/bindings/net/arc_emac.txt
new file mode 100644
index 0000000..bcbc3f0
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/arc_emac.txt
@@ -0,0 +1,38 @@
+* Synopsys ARC EMAC 10/100 Ethernet driver (EMAC)
+
+Required properties:
+- compatible: Should be "snps,arc-emac"
+- reg: Address and length of the register set for the device
+- interrupts: Should contain the EMAC interrupts
+- clock-frequency: CPU frequency. It is needed to calculate and set polling
+period of EMAC.
+- max-speed: Maximum supported data-rate in Mbit/s. In some HW configurations
+bandwidth of external memory controller might be a limiting factor. That's why
+it's required to specify which data-rate is supported on current SoC or FPGA.
+For example if only 10 Mbit/s is supported (10BASE-T) set "10". If 100 Mbit/s is
+supported (100BASE-TX) set "100".
+- phy: PHY device attached to the EMAC via MDIO bus
+
+Child nodes of the driver are the individual PHY devices connected to the
+MDIO bus. They must have a "reg" property given the PHY address on the MDIO bus.
+
+Optional properties:
+- mac-address: 6 bytes, mac address
+
+Examples:
+
+	ethernet@c0fc2000 {
+		compatible = "snps,arc-emac";
+		reg = <0xc0fc2000 0x3c>;
+		interrupts = <6>;
+		mac-address = [ 00 11 22 33 44 55 ];
+		clock-frequency = <80000000>;
+		max-speed = <100>;
+		phy = <&phy0>;
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+		phy0: ethernet-phy@0 {
+			reg = <1>;
+		};
+	};
diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
index 4bedae2..98eec48 100644
--- a/drivers/net/ethernet/Kconfig
+++ b/drivers/net/ethernet/Kconfig
@@ -23,6 +23,7 @@ source "drivers/net/ethernet/aeroflex/Kconfig"
 source "drivers/net/ethernet/alteon/Kconfig"
 source "drivers/net/ethernet/amd/Kconfig"
 source "drivers/net/ethernet/apple/Kconfig"
+source "drivers/net/ethernet/arc/Kconfig"
 source "drivers/net/ethernet/atheros/Kconfig"
 source "drivers/net/ethernet/cadence/Kconfig"
 source "drivers/net/ethernet/adi/Kconfig"
diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
index 183e3f4..b764c73 100644
--- a/drivers/net/ethernet/Makefile
+++ b/drivers/net/ethernet/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_GRETH) += aeroflex/
 obj-$(CONFIG_NET_VENDOR_ALTEON) += alteon/
 obj-$(CONFIG_NET_VENDOR_AMD) += amd/
 obj-$(CONFIG_NET_VENDOR_APPLE) += apple/
+obj-$(CONFIG_NET_VENDOR_ARC) += arc/
 obj-$(CONFIG_NET_VENDOR_ATHEROS) += atheros/
 obj-$(CONFIG_NET_CADENCE) += cadence/
 obj-$(CONFIG_NET_BFIN) += adi/
diff --git a/drivers/net/ethernet/arc/Kconfig b/drivers/net/ethernet/arc/Kconfig
new file mode 100644
index 0000000..514c57f
--- /dev/null
+++ b/drivers/net/ethernet/arc/Kconfig
@@ -0,0 +1,31 @@
+#
+# ARC EMAC network device configuration
+#
+
+config NET_VENDOR_ARC
+	bool "ARC devices"
+	default y
+	---help---
+	  If you have a network (Ethernet) card belonging to this class, say Y
+	  and read the Ethernet-HOWTO, available from
+	  <http://www.tldp.org/docs.html#howto>.
+
+	  Note that the answer to this question doesn't directly affect the
+	  kernel: saying N will just cause the configurator to skip all
+	  the questions about ARC cards. If you say Y, you will be asked for
+	  your specific card in the following questions.
+
+if NET_VENDOR_ARC
+
+config ARC_EMAC
+	tristate "ARC EMAC support"
+	select MII
+	select PHYLIB
+	depends on OF_IRQ
+	depends on OF_NET
+	---help---
+	  On some legacy ARC (Synopsys) FPGA boards such as ARCAngel4/ML50x
+	  non-standard on-chip ethernet device ARC EMAC 10/100 is used.
+	  Say Y here if you have such a board.  If unsure, say N.
+
+endif # NET_VENDOR_ARC
diff --git a/drivers/net/ethernet/arc/Makefile b/drivers/net/ethernet/arc/Makefile
new file mode 100644
index 0000000..f5b73e5
--- /dev/null
+++ b/drivers/net/ethernet/arc/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for the ARC network device drivers.
+#
+
+arc_emac-objs := arc_emac_main.o arc_emac_mdio.o
+obj-$(CONFIG_ARC_EMAC) += arc_emac.o
diff --git a/drivers/net/ethernet/arc/arc_emac.h b/drivers/net/ethernet/arc/arc_emac.h
new file mode 100644
index 0000000..6691db2
--- /dev/null
+++ b/drivers/net/ethernet/arc/arc_emac.h
@@ -0,0 +1,201 @@
+/*
+ * Copyright (C) 2004-2013 Synopsys, Inc. (www.synopsys.com)
+ *
+ * Registers and bits definitions of ARC EMAC
+ */
+
+#ifndef ARC_EMAC_H
+#define ARC_EMAC_H
+
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/netdevice.h>
+#include <linux/phy.h>
+
+/* STATUS and ENABLE Register bit masks */
+#define TXINT_MASK	(1<<0)	/* Transmit interrupt */
+#define RXINT_MASK	(1<<1)	/* Receive interrupt */
+#define ERR_MASK	(1<<2)	/* Error interrupt */
+#define TXCH_MASK	(1<<3)	/* Transmit chaining error interrupt */
+#define MSER_MASK	(1<<4)	/* Missed packet counter error */
+#define RXCR_MASK	(1<<8)	/* RXCRCERR counter rolled over  */
+#define RXFR_MASK	(1<<9)	/* RXFRAMEERR counter rolled over */
+#define RXFL_MASK	(1<<10)	/* RXOFLOWERR counter rolled over */
+#define MDIO_MASK	(1<<12)	/* MDIO complete interrupt */
+#define TXPL_MASK	(1<<31)	/* Force polling of BD by EMAC */
+
+/* CONTROL Register bit masks */
+#define EN_MASK		(1<<0)	/* VMAC enable */
+#define TXRN_MASK	(1<<3)	/* TX enable */
+#define RXRN_MASK	(1<<4)	/* RX enable */
+#define DSBC_MASK	(1<<8)	/* Disable receive broadcast */
+#define ENFL_MASK	(1<<10)	/* Enable Full-duplex */
+#define PROM_MASK	(1<<11)	/* Promiscuous mode */
+
+/* Buffer descriptor INFO bit masks */
+#define OWN_MASK	(1<<31)	/* 0-CPU owns buffer, 1-EMAC owns buffer */
+#define FRST_MASK	(1<<16)	/* First buffer in chain */
+#define LAST_MASK	(1<<17)	/* Last buffer in chain */
+#define LEN_MASK	0x000007FF	/* last 11 bits */
+#define CRLS		(1<<21)
+#define DEFR		(1<<22)
+#define DROP		(1<<23)
+#define RTRY		(1<<24)
+#define LTCL		(1<<28)
+#define UFLO		(1<<29)
+
+#define FOR_EMAC	OWN_MASK
+#define FOR_CPU		0
+
+/* ARC EMAC register set combines entries for MAC and MDIO */
+enum {
+	R_ID = 0,
+	R_STATUS,
+	R_ENABLE,
+	R_CTRL,
+	R_POLLRATE,
+	R_RXERR,
+	R_MISS,
+	R_TX_RING,
+	R_RX_RING,
+	R_ADDRL,
+	R_ADDRH,
+	R_LAFL,
+	R_LAFH,
+	R_MDIO,
+};
+
+/* Transmission timeout */
+#define TX_TIMEOUT (400*HZ/1000)
+
+#define ARC_EMAC_NAPI_WEIGHT 40	/* Workload for NAPI */
+
+#define TX_FIFO_SIZE 2048	/* EMAC Tx FIFO size */
+
+/* Assuming MTU=1500 (IP pkt: hdr+payload), we round off to next cache-line
+ * boundary: 1536 % {32,64,128} == 0
+ * This provides enough space for Ethernet header (14) and also ensures that
+ * buf-size passed to EMAC > max pkt rx (1514). Latter is due to a EMAC quirk
+ * wherein it can generate a chained pkt (with all data in first part, and
+ * an empty 2nd part - albeit with last bit set) when Rx-pkt-sz is exactly
+ * equal to buf-size: hence the need to keep buf-size slightly bigger than
+ * largest pkt.
+ */
+#define	EMAC_BUFFER_PAD 36
+
+struct arc_emac_bd_t {
+	unsigned int info;
+	char *data;
+};
+
+/* Number of Rx/Tx BD's */
+#define RX_BD_NUM	128
+#define TX_BD_NUM	128
+
+#define RX_RING_SZ  (RX_BD_NUM * sizeof(struct arc_emac_bd_t))
+#define TX_RING_SZ  (TX_BD_NUM * sizeof(struct arc_emac_bd_t))
+
+struct buffer_state {
+	struct sk_buff *skb;
+	DEFINE_DMA_UNMAP_ADDR(addr);
+	DEFINE_DMA_UNMAP_LEN(len);
+};
+
+struct arc_emac_priv {
+	struct net_device_stats stats;
+	unsigned int clock_frequency;
+	unsigned int max_speed;
+
+	/* Pointers to BD rings - CPU side */
+	struct arc_emac_bd_t *rxbd;
+	struct arc_emac_bd_t *txbd;
+
+	/* Pointers to BD rings - Device side */
+	dma_addr_t rxbd_dma_hdl;
+	dma_addr_t txbd_dma_hdl;
+
+	/* The actual socket buffers */
+	struct buffer_state rx_buff[RX_BD_NUM];
+	struct buffer_state tx_buff[TX_BD_NUM];
+	unsigned int txbd_curr;
+	unsigned int txbd_dirty;
+
+	/* Remember where driver last saw a packet, so next iteration it
+	 * starts from here and not 0
+	 */
+	unsigned int last_rx_bd;
+
+	struct napi_struct napi;
+
+	/* Phy saved state */
+	int duplex;
+	int speed;
+
+	/* Devices */
+	struct device *dev;
+	struct device_node *phy_node;
+	struct phy_device *phy_dev;
+	struct net_device *ndev;
+	struct mii_bus *bus;
+
+	/* EMAC registers base address */
+	void __iomem *reg_base_addr;
+};
+
+/**
+ * arc_reg_set - Sets EMAC register with provided value.
+ * @priv:	Pointer to ARC EMAC private data structure.
+ * @reg:	Register offset from base address
+ * @value:	Value to set in register
+ */
+static inline void arc_reg_set(struct arc_emac_priv *priv, int reg, int value)
+{
+	iowrite32(value, priv->reg_base_addr + reg * sizeof(int));
+}
+
+/**
+ * arc_reg_get - Gets value of specified EMAC register.
+ * @priv:	Pointer to ARC EMAC private data structure.
+ * @reg:	Register offset from base address
+ *
+ * returns:	Value of requested register
+ */
+static inline unsigned int arc_reg_get(struct arc_emac_priv *priv, int reg)
+{
+	return ioread32(priv->reg_base_addr + reg * sizeof(int));
+}
+
+/**
+ * arc_reg_or - Applies mask to specified EMAC register - ("reg" | "mask")
+ * @priv:	Pointer to ARC EMAC private data structure.
+ * @reg:	Register offset from base address
+ * @mask:	Mask to apply to specified register
+ *
+ * This function reads initial register value, then applies provided mask
+ * to it and then writes register back.
+ */
+static inline void arc_reg_or(struct arc_emac_priv *priv, int reg, int mask)
+{
+	unsigned int value = arc_reg_get(priv, reg);
+	arc_reg_set(priv, reg, value | mask);
+}
+
+/**
+ * arc_reg_clr - Applies mask to specified EMAC register - ("reg" & ~"mask")
+ * @priv:	Pointer to ARC EMAC private data structure.
+ * @reg:	Register offset from base address
+ * @mask:	Mask to apply to specified register
+ *
+ * This function reads initial register value, then applies provided mask
+ * to it and then writes register back.
+ */
+static inline void arc_reg_clr(struct arc_emac_priv *priv, int reg, int mask)
+{
+	unsigned int value = arc_reg_get(priv, reg);
+	arc_reg_set(priv, reg, value & ~mask);
+}
+
+int arc_mdio_probe(struct device_node *np, struct arc_emac_priv *priv);
+int arc_mdio_remove(struct arc_emac_priv *priv);
+
+#endif /* ARC_EMAC_H */
diff --git a/drivers/net/ethernet/arc/arc_emac_main.c b/drivers/net/ethernet/arc/arc_emac_main.c
new file mode 100644
index 0000000..7ae759f
--- /dev/null
+++ b/drivers/net/ethernet/arc/arc_emac_main.c
@@ -0,0 +1,813 @@
+/*
+ * Copyright (C) 2004-2013 Synopsys, Inc. (www.synopsys.com)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Driver for the ARC EMAC 10100 (hardware revision 5)
+ *
+ * Contributors:
+ *		Amit Bhor
+ *		Sameer Dhavale
+ *		Vineet Gupta
+ */
+
+#include <linux/etherdevice.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_mdio.h>
+#include <linux/of_net.h>
+#include <linux/of_platform.h>
+
+#include "arc_emac.h"
+
+#define DRV_NAME	"arc_emac"
+#define DRV_VERSION	"1.0"
+
+/**
+ * arc_emac_adjust_link - Adjust the PHY link duplex.
+ * @ndev:	Pointer to the net_device structure.
+ *
+ * This function is called to change the duplex setting after auto negotiation
+ * is done by the PHY.
+ */
+static void arc_emac_adjust_link(struct net_device *ndev)
+{
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+	struct phy_device *phy_dev = priv->phy_dev;
+	unsigned int reg, state_changed = 0;
+
+	if (!phy_dev->link)
+		return;
+
+	if (priv->duplex != phy_dev->duplex) {
+		reg = arc_reg_get(priv, R_CTRL);
+
+		if (DUPLEX_FULL == phy_dev->duplex)
+			reg |= ENFL_MASK;
+		else
+			reg &= ~ENFL_MASK;
+
+		arc_reg_set(priv, R_CTRL, reg);
+		priv->duplex = phy_dev->duplex;
+		state_changed = 1;
+	}
+
+	if (priv->speed != phy_dev->speed) {
+		priv->speed = phy_dev->speed;
+		state_changed = 1;
+	}
+
+	if (state_changed)
+		phy_print_status(phy_dev);
+}
+
+/**
+ * arc_emac_get_settings - Get PHY settings.
+ * @ndev:	Pointer to net_device structure.
+ * @cmd:	Pointer to ethtool_cmd structure.
+ *
+ * This implements ethtool command for getting PHY settings. If PHY could
+ * not be found, the function returns -ENODEV. This function calls the
+ * relevant PHY ethtool API to get the PHY settings.
+ * Issue "ethtool ethX" under linux prompt to execute this function.
+ */
+static int arc_emac_get_settings(struct net_device *ndev,
+				 struct ethtool_cmd *cmd)
+{
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+
+	if (priv->phy_dev)
+		return phy_ethtool_gset(priv->phy_dev, cmd);
+
+	return -EINVAL;
+}
+
+/**
+ * arc_emac_set_settings - Set PHY settings as passed in the argument.
+ * @ndev:	Pointer to net_device structure.
+ * @cmd:	Pointer to ethtool_cmd structure.
+ *
+ * This implements ethtool command for setting various PHY settings. If PHY
+ * could not be found, the function returns -ENODEV. This function calls the
+ * relevant PHY ethtool API to set the PHY.
+ * Issue e.g. "ethtool -s ethX speed 1000" under linux prompt to execute this
+ * function.
+ */
+static int arc_emac_set_settings(struct net_device *ndev,
+				 struct ethtool_cmd *cmd)
+{
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+
+	if (!capable(CAP_NET_ADMIN))
+		return -EPERM;
+
+	if (priv->phy_dev)
+		return phy_ethtool_sset(priv->phy_dev, cmd);
+
+	return -EINVAL;
+}
+
+/**
+ * arc_emac_get_drvinfo - Get EMAC driver information.
+ * @ndev:	Pointer to net_device structure.
+ * @info:	Pointer to ethtool_drvinfo structure.
+ *
+ * This implements ethtool command for getting the driver information.
+ * Issue "ethtool -i ethX" under linux prompt to execute this function.
+ */
+static void arc_emac_get_drvinfo(struct net_device *ndev,
+				 struct ethtool_drvinfo *info)
+{
+	strlcpy(info->driver, DRV_NAME, sizeof(info->driver));
+	strlcpy(info->version, DRV_VERSION, sizeof(info->version));
+}
+
+static const struct ethtool_ops arc_emac_ethtool_ops = {
+	.get_settings	= arc_emac_get_settings,
+	.set_settings	= arc_emac_set_settings,
+	.get_drvinfo	= arc_emac_get_drvinfo,
+	.get_link	= ethtool_op_get_link,
+};
+
+/**
+ * arc_emac_poll - processing of incoming packets with NAPI.
+ * @napi:	Pointer to napi_struct structure.
+ * @budget:	How many BDs to process on 1 call.
+ *
+ * returns:	Number of processed BDs
+ *
+ * Iterate through Rx BDs and deliver received packages to upper layer.
+ */
+static int arc_emac_poll(struct napi_struct *napi, int budget)
+{
+	struct net_device *ndev = napi->dev;
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+	unsigned int i, loop, work_done = 0;
+	struct sk_buff *skb;
+
+	/* Loop thru the BD chain, but not always from 0.
+	 * Start from right after where we last saw a packet.
+	 */
+	i = priv->last_rx_bd;
+
+	for (loop = 0; loop < RX_BD_NUM; loop++) {
+		struct net_device_stats *stats = &priv->stats;
+		struct buffer_state *rx_buff;
+		struct arc_emac_bd_t *rxbd;
+		dma_addr_t addr;
+		unsigned int info, pktlen;
+		unsigned int buflen = ndev->mtu + EMAC_BUFFER_PAD;
+
+		i = (i + 1) % RX_BD_NUM;
+		rx_buff = &priv->rx_buff[i];
+		rxbd = &priv->rxbd[i];
+		info = rxbd->info;
+
+		/* Process current BD */
+
+		if (unlikely((info & OWN_MASK) == FOR_EMAC)) {
+			/* BD is still owned by EMAC */
+			continue;
+		}
+
+		/* Make a note that we saw a packet at this BD.
+		 * So next time, driver starts from this + 1
+		 */
+		priv->last_rx_bd = i;
+
+#define FRST_OR_LAST_MASK	(FRST_MASK | LAST_MASK)
+
+		if (unlikely((info & FRST_OR_LAST_MASK) != FRST_OR_LAST_MASK)) {
+			/* Buffer chaining results in a significant
+			 * amount of additional bus overhead and thus
+			 * a higher CLK frequency or larger FIFOs are required.
+			 * Because of that fact we don't support
+			 * chaining of receive packets. We pre-allocate
+			 * buffers of MTU size so incoming packets
+			 * won't be chained.
+			 */
+			if (net_ratelimit())
+				netdev_err(ndev,
+					   "incomplete packed received\n");
+
+			/* Return ownership to EMAC */
+			rxbd->info = FOR_EMAC | buflen;
+			stats->rx_errors++;
+			stats->rx_length_errors++;
+			continue;
+		}
+
+		pktlen = info & LEN_MASK;
+		stats->rx_packets++;
+		stats->rx_bytes += pktlen;
+		skb = rx_buff->skb;
+		skb_put(skb, pktlen);
+		skb->dev = ndev;
+		skb->protocol = eth_type_trans(skb, ndev);
+		netif_receive_skb(skb);
+
+		dma_unmap_single(&ndev->dev,
+				 dma_unmap_addr(&rx_buff, addr),
+				 dma_unmap_len(&rx_buff, len),
+				 DMA_FROM_DEVICE);
+
+		/* Prepare the BD for next cycle */
+		rx_buff->skb = netdev_alloc_skb_ip_align(ndev, buflen);
+		if (unlikely(!rx_buff->skb)) {
+			if (net_ratelimit())
+				netdev_err(ndev, "cannot allocate skb\n");
+			stats->rx_errors++;
+			continue;
+		}
+
+		addr = dma_map_single(&ndev->dev, (void *)rx_buff->skb->data,
+				      buflen, DMA_FROM_DEVICE);
+		if (dma_mapping_error(&ndev->dev, addr)) {
+			if (net_ratelimit())
+				if (net_ratelimit())
+					netdev_err(ndev, "cannot dma map\n");
+			dev_kfree_skb(rx_buff->skb);
+			stats->rx_errors++;
+			continue;
+		}
+		dma_unmap_addr_set(&rx_buff, mapping, addr);
+		dma_unmap_len_set(&rx_buff, len, buflen);
+
+		rxbd->data = rx_buff->skb->data;
+
+		/* Return ownership to EMAC */
+		rxbd->info = FOR_EMAC | buflen;
+
+		work_done++;
+		if (work_done >= budget)
+			break;
+	}
+
+	if (work_done < budget) {
+		napi_complete(napi);
+		arc_reg_or(priv, R_ENABLE, RXINT_MASK);
+	}
+
+	return work_done;
+}
+
+/**
+ * arc_emac_intr - Global interrupt handler for EMAC.
+ * @irq:		irq number.
+ * @dev_instance:	device instance.
+ *
+ * returns: IRQ_HANDLED for all cases.
+ *
+ * ARC EMAC has only 1 interrupt line, and depending on bits raised in
+ * STATUS register we may tell what is a reason for interrupt to fire.
+ */
+static irqreturn_t arc_emac_intr(int irq, void *dev_instance)
+{
+	struct net_device *ndev = dev_instance;
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+	struct net_device_stats *stats = &priv->stats;
+	unsigned int status;
+
+	status = arc_reg_get(priv, R_STATUS);
+	status &= ~MDIO_MASK;
+
+	/* Reset all flags except "MDIO complete"*/
+	arc_reg_set(priv, R_STATUS, status);
+
+	if (status & RXINT_MASK) {
+		if (likely(napi_schedule_prep(&priv->napi))) {
+			arc_reg_clr(priv, R_ENABLE, RXINT_MASK);
+			__napi_schedule(&priv->napi);
+		}
+	}
+
+	if (status & TXINT_MASK) {
+		unsigned int i;
+
+		for (i = 0; i < TX_BD_NUM; i++) {
+			unsigned int *txbd_dirty = &priv->txbd_dirty;
+			struct arc_emac_bd_t *txbd = &priv->txbd[*txbd_dirty];
+			struct buffer_state *tx_buff = &priv->tx_buff[*txbd_dirty];
+			struct sk_buff *skb = tx_buff->skb;
+			unsigned int info = txbd->info;
+
+			/* EMAC owns this buffer, cannot touch it now */
+			if (info & FOR_EMAC)
+				continue;
+
+			/* Buffer is not in use currently, skip it */
+			if (!txbd->data)
+				continue;
+
+			if (unlikely(info & (DROP | DEFR | LTCL | UFLO))) {
+				stats->tx_errors++;
+				stats->tx_dropped++;
+
+				if (info & DEFR)
+					stats->tx_carrier_errors++;
+
+				if (info & LTCL)
+					stats->collisions++;
+
+				if (info & UFLO)
+					stats->tx_fifo_errors++;
+			} else if (likely(info & (FRST_MASK | LAST_MASK))) {
+				stats->tx_packets++;
+				stats->tx_bytes += skb->len;
+			}
+
+			dma_unmap_single(&ndev->dev,
+					 dma_unmap_addr(&tx_buff, addr),
+					 dma_unmap_len(&tx_buff, len),
+					 DMA_TO_DEVICE);
+
+			/* return the sk_buff to system */
+			dev_kfree_skb_irq(skb);
+
+			txbd->data = 0x0;
+			txbd->info = 0x0;
+
+			*txbd_dirty = (*txbd_dirty + 1) % TX_BD_NUM;
+
+			if (netif_queue_stopped(ndev))
+				netif_wake_queue(ndev);
+		}
+	}
+
+	if (status & ERR_MASK) {
+		/* MSER/RXCR/RXFR/RXFL interrupt fires on corresponding
+		 * 8-bit error counter overrun.
+		 * Because of this fact we add 256 (0x100) items each time
+		 * overrun interrupt happens.
+		 */
+
+		if (status & MSER_MASK) {
+			stats->rx_missed_errors += 0x100;
+			stats->rx_errors += 0x100;
+		}
+
+		if (status & RXCR_MASK) {
+			stats->rx_crc_errors += 0x100;
+			stats->rx_errors += 0x100;
+		}
+
+		if (status & RXFR_MASK) {
+			stats->rx_frame_errors += 0x100;
+			stats->rx_errors += 0x100;
+		}
+
+		if (status & RXFL_MASK) {
+			stats->rx_over_errors += 0x100;
+			stats->rx_errors += 0x100;
+		}
+	}
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * arc_emac_open - Open the network device.
+ * @ndev:	Pointer to the network device.
+ *
+ * returns: 0, on success or non-zero error value on failure.
+ *
+ * This function sets the MAC address, requests and enables an IRQ
+ * for the EMAC device and starts the Tx queue.
+ * It also connects to the phy device.
+ */
+static int arc_emac_open(struct net_device *ndev)
+{
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+	struct phy_device *phy_dev = priv->phy_dev;
+	struct arc_emac_bd_t *bd;
+	struct sk_buff *skb;
+	int i;
+
+	phy_dev->autoneg = AUTONEG_ENABLE;
+	phy_dev->speed = 0;
+	phy_dev->duplex = 0;
+	phy_dev->advertising = phy_dev->supported;
+
+	if (priv->max_speed > 100) {
+		phy_dev->advertising &= PHY_GBIT_FEATURES;
+	} else if (priv->max_speed <= 100) {
+		phy_dev->advertising &= PHY_BASIC_FEATURES;
+		if (priv->max_speed <= 10) {
+			phy_dev->advertising &= ~SUPPORTED_100baseT_Half;
+			phy_dev->advertising &= ~SUPPORTED_100baseT_Full;
+		}
+	}
+
+	/* Allocate and set buffers for Rx BD's */
+	bd = priv->rxbd;
+	for (i = 0; i < RX_BD_NUM; i++) {
+		skb = netdev_alloc_skb_ip_align(ndev, ndev->mtu +
+						EMAC_BUFFER_PAD);
+
+		priv->rx_buff[i].skb = skb;
+		bd->data = skb->data;
+
+		/* Set ownership to EMAC */
+		bd->info = FOR_EMAC | (ndev->mtu + EMAC_BUFFER_PAD);
+		bd++;
+	}
+
+	/* setup last seen to MAX, so driver starts from 0 */
+	priv->last_rx_bd = RX_BD_NUM - 1;
+
+	/* Allocate Tx BD's similar to Rx BD's. All Tx BD's owned by CPU */
+	bd = priv->txbd;
+	for (i = 0; i < TX_BD_NUM; i++) {
+		bd->data = 0;
+		bd->info = 0;
+		bd++;
+	}
+
+	/* Initialize logical address filter */
+	arc_reg_set(priv, R_LAFL, 0);
+	arc_reg_set(priv, R_LAFH, 0);
+
+	/* Set BD ring pointers for device side */
+	arc_reg_set(priv, R_RX_RING,
+		     (unsigned int)priv->rxbd_dma_hdl);
+
+	arc_reg_set(priv, R_TX_RING,
+		     (unsigned int)priv->rxbd_dma_hdl + RX_RING_SZ);
+
+	/* Set Poll rate so that it polls every 1 ms */
+	arc_reg_set(priv, R_POLLRATE,
+		     priv->clock_frequency / 1000000);
+
+	/* Enable interrupts */
+	arc_reg_set(priv, R_ENABLE,
+		     TXINT_MASK | RXINT_MASK | ERR_MASK);
+
+	/* Set CONTROL */
+	arc_reg_set(priv, R_CTRL,
+		     (RX_BD_NUM << 24) |	/* RX BD table length */
+		     (TX_BD_NUM << 16) |	/* TX BD table length */
+		     TXRN_MASK | RXRN_MASK);
+
+	/* Enable EMAC */
+	arc_reg_or(priv, R_CTRL, EN_MASK);
+
+	phy_start_aneg(priv->phy_dev);
+
+	netif_start_queue(ndev);
+	napi_enable(&priv->napi);
+
+	return 0;
+}
+
+/**
+ * arc_emac_stop - Close the network device.
+ * @ndev:	Pointer to the network device.
+ *
+ * This function stops the Tx queue, disables interrupts and frees the IRQ for
+ * the EMAC device.
+ * It also disconnects the PHY device associated with the EMAC device.
+ */
+static int arc_emac_stop(struct net_device *ndev)
+{
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+
+	napi_disable(&priv->napi);
+	netif_stop_queue(ndev);
+
+	/* Disable interrupts */
+	arc_reg_clr(priv, R_ENABLE, TXINT_MASK | RXINT_MASK | ERR_MASK);
+
+	/* Disable EMAC */
+	arc_reg_clr(priv, R_CTRL, EN_MASK);
+
+	return 0;
+}
+
+/**
+ * arc_emac_stats - Get system network statistics.
+ * @ndev:	Pointer to net_device structure.
+ *
+ * Returns the address of the device statistics structure.
+ * Statistics are updated in interrupt handler.
+ */
+static struct net_device_stats *arc_emac_stats(struct net_device *ndev)
+{
+	unsigned long miss, rxerr, rxfram, rxcrc, rxoflow;
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+	struct net_device_stats *stats = &priv->stats;
+
+	rxerr = arc_reg_get(priv, R_RXERR);
+	miss = arc_reg_get(priv, R_MISS);
+
+	rxcrc = rxerr & 0xff;
+	rxfram = rxerr >> 8 & 0xff;
+	rxoflow = rxerr >> 16 & 0xff;
+
+	stats->rx_errors += miss;
+	stats->rx_errors += rxcrc + rxfram + rxoflow;
+
+	stats->rx_over_errors += rxoflow;
+	stats->rx_frame_errors += rxfram;
+	stats->rx_crc_errors += rxcrc;
+	stats->rx_missed_errors += miss;
+
+	return stats;
+}
+
+/**
+ * arc_emac_tx - Starts the data transmission.
+ * @skb:	sk_buff pointer that contains data to be Transmitted.
+ * @ndev:	Pointer to net_device structure.
+ *
+ * returns: NETDEV_TX_OK, on success
+ *		NETDEV_TX_BUSY, if any of the descriptors are not free.
+ *
+ * This function is invoked from upper layers to initiate transmission.
+ */
+static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+	unsigned int info, len, *txbd_curr = &priv->txbd_curr;
+	dma_addr_t addr;
+	char *pkt = skb->data;
+
+	len = max_t(unsigned int, ETH_ZLEN, skb->len);
+	info = priv->txbd[*txbd_curr].info;
+
+	/* EMAC still holds this buffer in its possession.
+	 * CPU must not modify this buffer descriptor
+	 */
+	if (unlikely((info & OWN_MASK) == FOR_EMAC)) {
+		netif_stop_queue(ndev);
+		return NETDEV_TX_BUSY;
+	}
+
+	addr = dma_map_single(&ndev->dev, (void *)pkt, len, DMA_TO_DEVICE);
+	if (unlikely(dma_mapping_error(&ndev->dev, addr))) {
+		dev_kfree_skb(skb);
+		return NETDEV_TX_OK;
+	}
+	dma_unmap_addr_set(&priv->tx_buff[*txbd_curr], mapping, addr);
+	dma_unmap_len_set(&priv->tx_buff[*txbd_curr], len, len);
+
+	priv->tx_buff[*txbd_curr].skb = skb;
+	priv->txbd[*txbd_curr].data = pkt;
+	priv->txbd[*txbd_curr].info = FOR_EMAC | FRST_MASK | LAST_MASK | len;
+
+	*txbd_curr = (*txbd_curr + 1) % TX_BD_NUM;
+
+	arc_reg_set(priv, R_STATUS, TXPL_MASK);
+
+	skb_tx_timestamp(skb);
+
+	return NETDEV_TX_OK;
+}
+
+/**
+ * arc_emac_set_address - Set the MAC address for this device.
+ * @ndev:	Pointer to net_device structure.
+ * @p:		6 byte Address to be written as MAC address.
+ *
+ * This function copies the HW address from the sockaddr structure to the
+ * net_device structure and updates the address in HW.
+ *
+ * returns:	-EBUSY if the net device is busy or 0 if the address is set
+ *		successfully.
+ */
+static int arc_emac_set_address(struct net_device *ndev, void *p)
+{
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+	struct sockaddr *addr = p;
+	unsigned int addr_low, addr_hi;
+
+	if (netif_running(ndev))
+		return -EBUSY;
+
+	if (!is_valid_ether_addr(addr->sa_data))
+		return -EADDRNOTAVAIL;
+
+	memcpy(ndev->dev_addr, addr->sa_data, ndev->addr_len);
+
+	addr_low = le32_to_cpu(*(__le32 *) &ndev->dev_addr[0]);
+	addr_hi = le16_to_cpu(*(__le16 *) &ndev->dev_addr[4]);
+
+	arc_reg_set(priv, R_ADDRL, addr_low);
+	arc_reg_set(priv, R_ADDRH, addr_hi);
+
+	return 0;
+}
+
+static const struct net_device_ops arc_emac_netdev_ops = {
+	.ndo_open		= arc_emac_open,
+	.ndo_stop		= arc_emac_stop,
+	.ndo_start_xmit		= arc_emac_tx,
+	.ndo_set_mac_address	= arc_emac_set_address,
+	.ndo_get_stats		= arc_emac_stats,
+};
+
+static int arc_emac_probe(struct platform_device *pdev)
+{
+	struct net_device *ndev;
+	struct arc_emac_priv *priv;
+	int err;
+	unsigned int id;
+	struct resource res_regs, res_irq;
+	const char *mac_addr;
+
+	/* no device tree device */
+	if (!pdev->dev.of_node)
+		return -ENODEV;
+
+	ndev = alloc_etherdev(sizeof(struct arc_emac_priv));
+	if (!ndev) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	SET_NETDEV_DEV(ndev, &pdev->dev);
+
+	/* Populate our net_device structure */
+	ndev->netdev_ops = &arc_emac_netdev_ops;
+	ndev->ethtool_ops = &arc_emac_ethtool_ops;
+	ndev->watchdog_timeo = TX_TIMEOUT;
+	/* FIXME :: no multicast support yet */
+	ndev->flags &= ~IFF_MULTICAST;
+
+	priv = netdev_priv(ndev);
+	priv->dev = &pdev->dev;
+	priv->ndev = ndev;
+
+	/* Get PHY from device tree */
+	priv->phy_node = of_parse_phandle(pdev->dev.of_node, "phy", 0);
+	if (!priv->phy_node) {
+		dev_err(&pdev->dev, "failed to retrieve phy description from device tree\n");
+		err = -ENODEV;
+		goto out;
+	}
+
+	/* Get EMAC registers base address from device tree */
+	err = of_address_to_resource(pdev->dev.of_node, 0, &res_regs);
+	if (err) {
+		dev_err(&pdev->dev, "failed to retrieve registers base from device tree\n");
+		err = -ENODEV;
+		goto out;
+	}
+
+	priv->reg_base_addr = devm_ioremap_resource(&pdev->dev, &res_regs);
+	if (IS_ERR(priv->reg_base_addr)) {
+		err = PTR_ERR(priv->reg_base_addr);
+		goto out;
+	}
+	dev_dbg(&pdev->dev, "Registers base address is 0x%p\n",
+		priv->reg_base_addr);
+
+	id = arc_reg_get(priv, R_ID);
+	/* Check for EMAC revision 5 or 7, magic number */
+	if (!(id == 0x0005fd02 || id == 0x0007fd02)) {
+		dev_err(&pdev->dev, "ARC EMAC not detected, id=0x%x\n", id);
+		err = -ENODEV;
+		goto out;
+	}
+	dev_info(&pdev->dev, "ARC EMAC detected with id: 0x%x\n", id);
+
+	/* Get CPU clock frequency from device tree */
+	if (of_property_read_u32(pdev->dev.of_node, "clock-frequency",
+				 &priv->clock_frequency)) {
+		dev_err(&pdev->dev, "failed to retrieve <clock-frequency> from device tree\n");
+		err = -EINVAL;
+		goto out;
+	}
+
+	/* Get max speed of operation from device tree */
+	if (of_property_read_u32(pdev->dev.of_node, "max-speed",
+				 &priv->max_speed)) {
+		dev_err(&pdev->dev, "failed to retrieve <max-speed> from device tree\n");
+		err = -EINVAL;
+		goto out;
+	}
+
+	/* Get IRQ from device tree */
+	err = of_irq_to_resource(pdev->dev.of_node, 0, &res_irq);
+	if (!err) {
+		dev_err(&pdev->dev, "failed to retrieve <irq> value from device tree\n");
+		err = -ENODEV;
+		goto out;
+	}
+	ndev->irq = res_irq.start;
+	dev_info(&pdev->dev, "IRQ is %d\n", ndev->irq);
+
+	/* Register interrupt handler for device */
+	err = devm_request_irq(&pdev->dev, ndev->irq, arc_emac_intr, 0,
+			       ndev->name, ndev);
+	if (err) {
+		dev_err(&pdev->dev, "could not allocate IRQ\n");
+		goto out;
+	}
+
+	/* Get MAC address from device tree */
+	mac_addr = of_get_mac_address(pdev->dev.of_node);
+
+	if (!mac_addr || !is_valid_ether_addr(mac_addr))
+		eth_hw_addr_random(ndev);
+	else
+		memcpy(ndev->dev_addr, mac_addr, ETH_ALEN);
+
+	dev_info(&pdev->dev, "MAC address is now %pM\n", ndev->dev_addr);
+
+	/* Allocate cache coherent memory for BD Rings - to avoid need to do
+	 * explicit uncached ".di" accesses, which can potentially screw-up
+	 */
+	priv->rxbd = dmam_alloc_coherent(&pdev->dev, RX_RING_SZ + TX_RING_SZ,
+					 &priv->rxbd_dma_hdl, GFP_KERNEL);
+
+	if (!priv->rxbd) {
+		dev_err(&pdev->dev, "failed to allocate data buffers\n");
+		err = -ENOMEM;
+		goto out;
+	}
+
+	priv->txbd = priv->rxbd + RX_BD_NUM;
+
+	/* To keep things simple - we just do 1 big allocation, instead of 2
+	 * separate ones for Rx and Tx Rings responsively
+	 */
+	priv->txbd_dma_hdl = priv->rxbd_dma_hdl + RX_RING_SZ;
+	dev_dbg(&pdev->dev, "EMAC Device addr: Rx Ring [0x%x], Tx Ring[%x]\n",
+		(unsigned int)priv->rxbd_dma_hdl,
+		(unsigned int)priv->txbd_dma_hdl);
+
+	err = arc_mdio_probe(pdev->dev.of_node, priv);
+	if (err) {
+		dev_err(&pdev->dev, "failed to probe MII bus\n");
+		goto out;
+	}
+
+
+	priv->phy_dev = of_phy_connect(ndev, priv->phy_node,
+				       arc_emac_adjust_link, 0,
+				       PHY_INTERFACE_MODE_MII);
+	if (!priv->phy_dev) {
+		netdev_err(ndev, "of_phy_connect() failed\n");
+		return -ENODEV;
+	}
+
+	dev_info(&pdev->dev, "connected to %s phy with id 0x%x\n",
+		 priv->phy_dev->drv->name, priv->phy_dev->phy_id);
+
+	netif_napi_add(ndev, &priv->napi, arc_emac_poll, ARC_EMAC_NAPI_WEIGHT);
+
+	err = register_netdev(ndev);
+	if (err) {
+		netif_napi_del(&priv->napi);
+		dev_err(&pdev->dev, "failed to register network device\n");
+		goto out;
+	}
+
+	return 0;
+
+out:
+	free_netdev(ndev);
+	return err;
+}
+
+static int arc_emac_remove(struct platform_device *pdev)
+{
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+
+	phy_disconnect(priv->phy_dev);
+	priv->phy_dev = NULL;
+	arc_mdio_remove(priv);
+	unregister_netdev(ndev);
+	netif_napi_del(&priv->napi);
+	free_netdev(ndev);
+
+	return 0;
+}
+
+static const struct of_device_id arc_emac_dt_ids[] = {
+	{ .compatible = "snps,arc-emac" },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, arc_emac_dt_ids);
+
+static struct platform_driver arc_emac_driver = {
+	.probe = arc_emac_probe,
+	.remove = arc_emac_remove,
+	.driver = {
+		.name = DRV_NAME,
+		.owner = THIS_MODULE,
+		.of_match_table  = arc_emac_dt_ids,
+		},
+};
+
+module_platform_driver(arc_emac_driver);
+
+MODULE_AUTHOR("Alexey Brodkin <abrodkin@synopsys.com>");
+MODULE_DESCRIPTION("ARC EMAC driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/net/ethernet/arc/arc_emac_mdio.c b/drivers/net/ethernet/arc/arc_emac_mdio.c
new file mode 100644
index 0000000..3ae48d4
--- /dev/null
+++ b/drivers/net/ethernet/arc/arc_emac_mdio.c
@@ -0,0 +1,152 @@
+/*
+ * Copyright (C) 2004-2013 Synopsys, Inc. (www.synopsys.com)
+ *
+ * MDIO implementation for ARC EMAC
+ */
+
+#include <linux/delay.h>
+#include <linux/of_mdio.h>
+
+#include "arc_emac.h"
+
+/* Number of seconds we wait for "MDIO complete" flag to appear */
+#define ARC_MDIO_COMPLETE_POLL_COUNT	1
+
+/**
+ * arc_mdio_complete_wait - Waits until MDIO transaction is completed.
+ * @priv:	Pointer to ARC EMAC private data structure.
+ *
+ * returns:	0 on success, -ETIMEDOUT on a timeout.
+ */
+static int arc_mdio_complete_wait(struct arc_emac_priv *priv)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARC_MDIO_COMPLETE_POLL_COUNT * 40; i++) {
+		unsigned int status = arc_reg_get(priv, R_STATUS);
+
+		status &= MDIO_MASK;
+
+		if (status) {
+			/* Reset "MDIO complete" flag */
+			arc_reg_set(priv, R_STATUS, status);
+			return 0;
+		}
+
+		msleep(25);
+	}
+
+	return -ETIMEDOUT;
+}
+
+/**
+ * arc_mdio_read - MDIO interface read function.
+ * @bus:	Pointer to MII bus structure.
+ * @phy_addr:	Address of the PHY device.
+ * @reg_num:	PHY register to read.
+ *
+ * returns:	The register contents on success, -ETIMEDOUT on a timeout.
+ *
+ * Reads the contents of the requested register from the requested PHY
+ * address.
+ */
+static int arc_mdio_read(struct mii_bus *bus, int phy_addr, int reg_num)
+{
+	struct arc_emac_priv *priv = bus->priv;
+	unsigned int value;
+	int error;
+
+	arc_reg_set(priv, R_MDIO,
+		    0x60020000 | (phy_addr << 23) | (reg_num << 18));
+
+	error = arc_mdio_complete_wait(priv);
+	if (error < 0)
+		return error;
+
+	value = arc_reg_get(priv, R_MDIO) & 0xffff;
+
+	dev_dbg(priv->dev, "arc_mdio_read(phy_addr=%i, reg_num=%x) = %x\n",
+		phy_addr, reg_num, value);
+
+	return value;
+}
+
+/**
+ * arc_mdio_write - MDIO interface write function.
+ * @bus:	Pointer to MII bus structure.
+ * @phy_addr:	Address of the PHY device.
+ * @reg_num:	PHY register to write to.
+ * @value:	Value to be written into the register.
+ *
+ * returns:	0 on success, -ETIMEDOUT on a timeout.
+ *
+ * Writes the value to the requested register.
+ */
+static int arc_mdio_write(struct mii_bus *bus, int phy_addr,
+			  int reg_num, u16 value)
+{
+	struct arc_emac_priv *priv = bus->priv;
+
+	dev_dbg(priv->dev,
+		"arc_mdio_write(phy_addr=%i, reg_num=%x, value=%x)\n",
+		phy_addr, reg_num, value);
+
+	arc_reg_set(priv, R_MDIO,
+		     0x50020000 | (phy_addr << 23) | (reg_num << 18) | value);
+
+	return arc_mdio_complete_wait(priv);
+}
+
+/**
+ * arc_mdio_probe - MDIO probe function.
+ * @dev_node:	Pointer to device node.
+ * @priv:	Pointer to ARC EMAC private data structure.
+ *
+ * returns:	0 on success, -ENOMEM when mdiobus_alloc
+ * (to allocate memory for MII bus structure) fails.
+ *
+ * Sets up and registers the MDIO interface.
+ */
+int arc_mdio_probe(struct device_node *dev_node, struct arc_emac_priv *priv)
+{
+	struct mii_bus *bus;
+	int error;
+
+	bus = mdiobus_alloc();
+	if (!bus)
+		return -ENOMEM;
+
+	priv->bus = bus;
+	bus->priv = priv;
+	bus->parent = priv->dev;
+	bus->name = "Synopsys MII Bus",
+	bus->read = &arc_mdio_read;
+	bus->write = &arc_mdio_write;
+
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%.8x",
+		 (unsigned int)priv->reg_base_addr);
+
+	error = of_mdiobus_register(bus, dev_node);
+	if (error) {
+		dev_err(priv->dev, "cannot register MDIO bus %s\n", bus->name);
+		mdiobus_free(bus);
+		return error;
+	}
+
+	return 0;
+}
+
+/**
+ * arc_mdio_remove - MDIO remove function.
+ * @priv:	Pointer to ARC EMAC private data structure.
+ *
+ * Unregisters the MDIO and frees any associate memory for MII bus.
+ */
+int arc_mdio_remove(struct arc_emac_priv *priv)
+{
+	mdiobus_unregister(priv->bus);
+	mdiobus_free(priv->bus);
+	priv->bus = NULL;
+
+	return 0;
+}
-- 
1.7.10.4


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

* [PATCH v3] ethernet/arc/arc_emac - Add new driver
@ 2013-06-13 14:37 ` Alexey Brodkin
  0 siblings, 0 replies; 21+ messages in thread
From: Alexey Brodkin @ 2013-06-13 14:37 UTC (permalink / raw)
  To: netdev
  Cc: Alexey Brodkin, Andy Shevchenko, Francois Romieu, Joe Perches,
	Vineet Gupta, Mischa Jonker, Arnd Bergmann, Grant Likely,
	Rob Herring, Paul Gortmaker, David S. Miller, linux-kernel,
	devicetree-discuss

Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
instantiated in some legacy ARC (Synopsys) FPGA Boards such as
ARCAngel4/ML50x.

This is based off of current Linus tree.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
Reviewed-by: Vineet Gupta <vgupta@synopsys.com>
Reviewed-by: Mischa Jonker <mjonker@synopsys.com>

Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Francois Romieu <romieu@fr.zoreil.com>
Cc: Joe Perches <joe@perches.com>
Cc: Vineet Gupta <vgupta@synopsys.com>
Cc: Mischa Jonker <mjonker@synopsys.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Grant Likely <grant.likely@linaro.org>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: Paul Gortmaker <paul.gortmaker@windriver.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-kernel@vger.kernel.org
Cc: devicetree-discuss@lists.ozlabs.org

---
Hi all,

this is 3rd re-spin of my patch.

It introduces pretty significant changes in comparison to v2.
Those changes were inspiresd by very useful comments of:
 * Andy Shevchenko
 * Francois Romieu
 * Joe Perches

I hope now driver looks simpler, cleaner and better complies to the most
recent "standards" for network drivers in Linux kernel.

Below is a list of main changes in this respin.

1. Functional changes:
1.1. Heavy re-work of "arc_emac_poll", "arc_emac_tx" and
"arc_emac_intr". Now they look simpler and implement most of bullets
below.
1.2. Properly support DMA-API: dma_{map|unmap}_single, error checking
etc.
1.3. Rework "arc_emac_set_address" to be endian-friendly.
1.4. Use "netdev_alloc_skb_ip_align" instead of "netdev_alloc_skb" +
"skb_reserve".
1.5. Move PHY connection from "arc_emac_open" to "arc_emac_probe".
1.6. Merge EMAC and MDIO private structures into single one.
1.7. Add use of "netif_{stop|resume}_queue".
1.8. Add "max-speed" DT item for EMAC for proper PHY setup. Some systems
have limited memory bandwidth so we need to limit incoming data-rate.
1.9. Convert register accessors from macros into in-lined functions.

2. Cosmetics:
2.1. Remove change-log.
2.2. With inverted logic saved 2 indentation levels.
2.3. Merge multi-line text comments into 1 line.
2.4. Use "ndev" instead of "net_dev".
2.5. All functions in "arc_emac_main.c" are "static" now.
---
 Documentation/devicetree/bindings/net/arc_emac.txt |   38 +
 drivers/net/ethernet/Kconfig                       |    1 +
 drivers/net/ethernet/Makefile                      |    1 +
 drivers/net/ethernet/arc/Kconfig                   |   31 +
 drivers/net/ethernet/arc/Makefile                  |    6 +
 drivers/net/ethernet/arc/arc_emac.h                |  201 +++++
 drivers/net/ethernet/arc/arc_emac_main.c           |  813 ++++++++++++++++++++
 drivers/net/ethernet/arc/arc_emac_mdio.c           |  152 ++++
 8 files changed, 1243 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/arc_emac.txt
 create mode 100644 drivers/net/ethernet/arc/Kconfig
 create mode 100644 drivers/net/ethernet/arc/Makefile
 create mode 100644 drivers/net/ethernet/arc/arc_emac.h
 create mode 100644 drivers/net/ethernet/arc/arc_emac_main.c
 create mode 100644 drivers/net/ethernet/arc/arc_emac_mdio.c

diff --git a/Documentation/devicetree/bindings/net/arc_emac.txt b/Documentation/devicetree/bindings/net/arc_emac.txt
new file mode 100644
index 0000000..bcbc3f0
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/arc_emac.txt
@@ -0,0 +1,38 @@
+* Synopsys ARC EMAC 10/100 Ethernet driver (EMAC)
+
+Required properties:
+- compatible: Should be "snps,arc-emac"
+- reg: Address and length of the register set for the device
+- interrupts: Should contain the EMAC interrupts
+- clock-frequency: CPU frequency. It is needed to calculate and set polling
+period of EMAC.
+- max-speed: Maximum supported data-rate in Mbit/s. In some HW configurations
+bandwidth of external memory controller might be a limiting factor. That's why
+it's required to specify which data-rate is supported on current SoC or FPGA.
+For example if only 10 Mbit/s is supported (10BASE-T) set "10". If 100 Mbit/s is
+supported (100BASE-TX) set "100".
+- phy: PHY device attached to the EMAC via MDIO bus
+
+Child nodes of the driver are the individual PHY devices connected to the
+MDIO bus. They must have a "reg" property given the PHY address on the MDIO bus.
+
+Optional properties:
+- mac-address: 6 bytes, mac address
+
+Examples:
+
+	ethernet@c0fc2000 {
+		compatible = "snps,arc-emac";
+		reg = <0xc0fc2000 0x3c>;
+		interrupts = <6>;
+		mac-address = [ 00 11 22 33 44 55 ];
+		clock-frequency = <80000000>;
+		max-speed = <100>;
+		phy = <&phy0>;
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+		phy0: ethernet-phy@0 {
+			reg = <1>;
+		};
+	};
diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
index 4bedae2..98eec48 100644
--- a/drivers/net/ethernet/Kconfig
+++ b/drivers/net/ethernet/Kconfig
@@ -23,6 +23,7 @@ source "drivers/net/ethernet/aeroflex/Kconfig"
 source "drivers/net/ethernet/alteon/Kconfig"
 source "drivers/net/ethernet/amd/Kconfig"
 source "drivers/net/ethernet/apple/Kconfig"
+source "drivers/net/ethernet/arc/Kconfig"
 source "drivers/net/ethernet/atheros/Kconfig"
 source "drivers/net/ethernet/cadence/Kconfig"
 source "drivers/net/ethernet/adi/Kconfig"
diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
index 183e3f4..b764c73 100644
--- a/drivers/net/ethernet/Makefile
+++ b/drivers/net/ethernet/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_GRETH) += aeroflex/
 obj-$(CONFIG_NET_VENDOR_ALTEON) += alteon/
 obj-$(CONFIG_NET_VENDOR_AMD) += amd/
 obj-$(CONFIG_NET_VENDOR_APPLE) += apple/
+obj-$(CONFIG_NET_VENDOR_ARC) += arc/
 obj-$(CONFIG_NET_VENDOR_ATHEROS) += atheros/
 obj-$(CONFIG_NET_CADENCE) += cadence/
 obj-$(CONFIG_NET_BFIN) += adi/
diff --git a/drivers/net/ethernet/arc/Kconfig b/drivers/net/ethernet/arc/Kconfig
new file mode 100644
index 0000000..514c57f
--- /dev/null
+++ b/drivers/net/ethernet/arc/Kconfig
@@ -0,0 +1,31 @@
+#
+# ARC EMAC network device configuration
+#
+
+config NET_VENDOR_ARC
+	bool "ARC devices"
+	default y
+	---help---
+	  If you have a network (Ethernet) card belonging to this class, say Y
+	  and read the Ethernet-HOWTO, available from
+	  <http://www.tldp.org/docs.html#howto>.
+
+	  Note that the answer to this question doesn't directly affect the
+	  kernel: saying N will just cause the configurator to skip all
+	  the questions about ARC cards. If you say Y, you will be asked for
+	  your specific card in the following questions.
+
+if NET_VENDOR_ARC
+
+config ARC_EMAC
+	tristate "ARC EMAC support"
+	select MII
+	select PHYLIB
+	depends on OF_IRQ
+	depends on OF_NET
+	---help---
+	  On some legacy ARC (Synopsys) FPGA boards such as ARCAngel4/ML50x
+	  non-standard on-chip ethernet device ARC EMAC 10/100 is used.
+	  Say Y here if you have such a board.  If unsure, say N.
+
+endif # NET_VENDOR_ARC
diff --git a/drivers/net/ethernet/arc/Makefile b/drivers/net/ethernet/arc/Makefile
new file mode 100644
index 0000000..f5b73e5
--- /dev/null
+++ b/drivers/net/ethernet/arc/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for the ARC network device drivers.
+#
+
+arc_emac-objs := arc_emac_main.o arc_emac_mdio.o
+obj-$(CONFIG_ARC_EMAC) += arc_emac.o
diff --git a/drivers/net/ethernet/arc/arc_emac.h b/drivers/net/ethernet/arc/arc_emac.h
new file mode 100644
index 0000000..6691db2
--- /dev/null
+++ b/drivers/net/ethernet/arc/arc_emac.h
@@ -0,0 +1,201 @@
+/*
+ * Copyright (C) 2004-2013 Synopsys, Inc. (www.synopsys.com)
+ *
+ * Registers and bits definitions of ARC EMAC
+ */
+
+#ifndef ARC_EMAC_H
+#define ARC_EMAC_H
+
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/netdevice.h>
+#include <linux/phy.h>
+
+/* STATUS and ENABLE Register bit masks */
+#define TXINT_MASK	(1<<0)	/* Transmit interrupt */
+#define RXINT_MASK	(1<<1)	/* Receive interrupt */
+#define ERR_MASK	(1<<2)	/* Error interrupt */
+#define TXCH_MASK	(1<<3)	/* Transmit chaining error interrupt */
+#define MSER_MASK	(1<<4)	/* Missed packet counter error */
+#define RXCR_MASK	(1<<8)	/* RXCRCERR counter rolled over  */
+#define RXFR_MASK	(1<<9)	/* RXFRAMEERR counter rolled over */
+#define RXFL_MASK	(1<<10)	/* RXOFLOWERR counter rolled over */
+#define MDIO_MASK	(1<<12)	/* MDIO complete interrupt */
+#define TXPL_MASK	(1<<31)	/* Force polling of BD by EMAC */
+
+/* CONTROL Register bit masks */
+#define EN_MASK		(1<<0)	/* VMAC enable */
+#define TXRN_MASK	(1<<3)	/* TX enable */
+#define RXRN_MASK	(1<<4)	/* RX enable */
+#define DSBC_MASK	(1<<8)	/* Disable receive broadcast */
+#define ENFL_MASK	(1<<10)	/* Enable Full-duplex */
+#define PROM_MASK	(1<<11)	/* Promiscuous mode */
+
+/* Buffer descriptor INFO bit masks */
+#define OWN_MASK	(1<<31)	/* 0-CPU owns buffer, 1-EMAC owns buffer */
+#define FRST_MASK	(1<<16)	/* First buffer in chain */
+#define LAST_MASK	(1<<17)	/* Last buffer in chain */
+#define LEN_MASK	0x000007FF	/* last 11 bits */
+#define CRLS		(1<<21)
+#define DEFR		(1<<22)
+#define DROP		(1<<23)
+#define RTRY		(1<<24)
+#define LTCL		(1<<28)
+#define UFLO		(1<<29)
+
+#define FOR_EMAC	OWN_MASK
+#define FOR_CPU		0
+
+/* ARC EMAC register set combines entries for MAC and MDIO */
+enum {
+	R_ID = 0,
+	R_STATUS,
+	R_ENABLE,
+	R_CTRL,
+	R_POLLRATE,
+	R_RXERR,
+	R_MISS,
+	R_TX_RING,
+	R_RX_RING,
+	R_ADDRL,
+	R_ADDRH,
+	R_LAFL,
+	R_LAFH,
+	R_MDIO,
+};
+
+/* Transmission timeout */
+#define TX_TIMEOUT (400*HZ/1000)
+
+#define ARC_EMAC_NAPI_WEIGHT 40	/* Workload for NAPI */
+
+#define TX_FIFO_SIZE 2048	/* EMAC Tx FIFO size */
+
+/* Assuming MTU=1500 (IP pkt: hdr+payload), we round off to next cache-line
+ * boundary: 1536 % {32,64,128} == 0
+ * This provides enough space for Ethernet header (14) and also ensures that
+ * buf-size passed to EMAC > max pkt rx (1514). Latter is due to a EMAC quirk
+ * wherein it can generate a chained pkt (with all data in first part, and
+ * an empty 2nd part - albeit with last bit set) when Rx-pkt-sz is exactly
+ * equal to buf-size: hence the need to keep buf-size slightly bigger than
+ * largest pkt.
+ */
+#define	EMAC_BUFFER_PAD 36
+
+struct arc_emac_bd_t {
+	unsigned int info;
+	char *data;
+};
+
+/* Number of Rx/Tx BD's */
+#define RX_BD_NUM	128
+#define TX_BD_NUM	128
+
+#define RX_RING_SZ  (RX_BD_NUM * sizeof(struct arc_emac_bd_t))
+#define TX_RING_SZ  (TX_BD_NUM * sizeof(struct arc_emac_bd_t))
+
+struct buffer_state {
+	struct sk_buff *skb;
+	DEFINE_DMA_UNMAP_ADDR(addr);
+	DEFINE_DMA_UNMAP_LEN(len);
+};
+
+struct arc_emac_priv {
+	struct net_device_stats stats;
+	unsigned int clock_frequency;
+	unsigned int max_speed;
+
+	/* Pointers to BD rings - CPU side */
+	struct arc_emac_bd_t *rxbd;
+	struct arc_emac_bd_t *txbd;
+
+	/* Pointers to BD rings - Device side */
+	dma_addr_t rxbd_dma_hdl;
+	dma_addr_t txbd_dma_hdl;
+
+	/* The actual socket buffers */
+	struct buffer_state rx_buff[RX_BD_NUM];
+	struct buffer_state tx_buff[TX_BD_NUM];
+	unsigned int txbd_curr;
+	unsigned int txbd_dirty;
+
+	/* Remember where driver last saw a packet, so next iteration it
+	 * starts from here and not 0
+	 */
+	unsigned int last_rx_bd;
+
+	struct napi_struct napi;
+
+	/* Phy saved state */
+	int duplex;
+	int speed;
+
+	/* Devices */
+	struct device *dev;
+	struct device_node *phy_node;
+	struct phy_device *phy_dev;
+	struct net_device *ndev;
+	struct mii_bus *bus;
+
+	/* EMAC registers base address */
+	void __iomem *reg_base_addr;
+};
+
+/**
+ * arc_reg_set - Sets EMAC register with provided value.
+ * @priv:	Pointer to ARC EMAC private data structure.
+ * @reg:	Register offset from base address
+ * @value:	Value to set in register
+ */
+static inline void arc_reg_set(struct arc_emac_priv *priv, int reg, int value)
+{
+	iowrite32(value, priv->reg_base_addr + reg * sizeof(int));
+}
+
+/**
+ * arc_reg_get - Gets value of specified EMAC register.
+ * @priv:	Pointer to ARC EMAC private data structure.
+ * @reg:	Register offset from base address
+ *
+ * returns:	Value of requested register
+ */
+static inline unsigned int arc_reg_get(struct arc_emac_priv *priv, int reg)
+{
+	return ioread32(priv->reg_base_addr + reg * sizeof(int));
+}
+
+/**
+ * arc_reg_or - Applies mask to specified EMAC register - ("reg" | "mask")
+ * @priv:	Pointer to ARC EMAC private data structure.
+ * @reg:	Register offset from base address
+ * @mask:	Mask to apply to specified register
+ *
+ * This function reads initial register value, then applies provided mask
+ * to it and then writes register back.
+ */
+static inline void arc_reg_or(struct arc_emac_priv *priv, int reg, int mask)
+{
+	unsigned int value = arc_reg_get(priv, reg);
+	arc_reg_set(priv, reg, value | mask);
+}
+
+/**
+ * arc_reg_clr - Applies mask to specified EMAC register - ("reg" & ~"mask")
+ * @priv:	Pointer to ARC EMAC private data structure.
+ * @reg:	Register offset from base address
+ * @mask:	Mask to apply to specified register
+ *
+ * This function reads initial register value, then applies provided mask
+ * to it and then writes register back.
+ */
+static inline void arc_reg_clr(struct arc_emac_priv *priv, int reg, int mask)
+{
+	unsigned int value = arc_reg_get(priv, reg);
+	arc_reg_set(priv, reg, value & ~mask);
+}
+
+int arc_mdio_probe(struct device_node *np, struct arc_emac_priv *priv);
+int arc_mdio_remove(struct arc_emac_priv *priv);
+
+#endif /* ARC_EMAC_H */
diff --git a/drivers/net/ethernet/arc/arc_emac_main.c b/drivers/net/ethernet/arc/arc_emac_main.c
new file mode 100644
index 0000000..7ae759f
--- /dev/null
+++ b/drivers/net/ethernet/arc/arc_emac_main.c
@@ -0,0 +1,813 @@
+/*
+ * Copyright (C) 2004-2013 Synopsys, Inc. (www.synopsys.com)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Driver for the ARC EMAC 10100 (hardware revision 5)
+ *
+ * Contributors:
+ *		Amit Bhor
+ *		Sameer Dhavale
+ *		Vineet Gupta
+ */
+
+#include <linux/etherdevice.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_mdio.h>
+#include <linux/of_net.h>
+#include <linux/of_platform.h>
+
+#include "arc_emac.h"
+
+#define DRV_NAME	"arc_emac"
+#define DRV_VERSION	"1.0"
+
+/**
+ * arc_emac_adjust_link - Adjust the PHY link duplex.
+ * @ndev:	Pointer to the net_device structure.
+ *
+ * This function is called to change the duplex setting after auto negotiation
+ * is done by the PHY.
+ */
+static void arc_emac_adjust_link(struct net_device *ndev)
+{
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+	struct phy_device *phy_dev = priv->phy_dev;
+	unsigned int reg, state_changed = 0;
+
+	if (!phy_dev->link)
+		return;
+
+	if (priv->duplex != phy_dev->duplex) {
+		reg = arc_reg_get(priv, R_CTRL);
+
+		if (DUPLEX_FULL == phy_dev->duplex)
+			reg |= ENFL_MASK;
+		else
+			reg &= ~ENFL_MASK;
+
+		arc_reg_set(priv, R_CTRL, reg);
+		priv->duplex = phy_dev->duplex;
+		state_changed = 1;
+	}
+
+	if (priv->speed != phy_dev->speed) {
+		priv->speed = phy_dev->speed;
+		state_changed = 1;
+	}
+
+	if (state_changed)
+		phy_print_status(phy_dev);
+}
+
+/**
+ * arc_emac_get_settings - Get PHY settings.
+ * @ndev:	Pointer to net_device structure.
+ * @cmd:	Pointer to ethtool_cmd structure.
+ *
+ * This implements ethtool command for getting PHY settings. If PHY could
+ * not be found, the function returns -ENODEV. This function calls the
+ * relevant PHY ethtool API to get the PHY settings.
+ * Issue "ethtool ethX" under linux prompt to execute this function.
+ */
+static int arc_emac_get_settings(struct net_device *ndev,
+				 struct ethtool_cmd *cmd)
+{
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+
+	if (priv->phy_dev)
+		return phy_ethtool_gset(priv->phy_dev, cmd);
+
+	return -EINVAL;
+}
+
+/**
+ * arc_emac_set_settings - Set PHY settings as passed in the argument.
+ * @ndev:	Pointer to net_device structure.
+ * @cmd:	Pointer to ethtool_cmd structure.
+ *
+ * This implements ethtool command for setting various PHY settings. If PHY
+ * could not be found, the function returns -ENODEV. This function calls the
+ * relevant PHY ethtool API to set the PHY.
+ * Issue e.g. "ethtool -s ethX speed 1000" under linux prompt to execute this
+ * function.
+ */
+static int arc_emac_set_settings(struct net_device *ndev,
+				 struct ethtool_cmd *cmd)
+{
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+
+	if (!capable(CAP_NET_ADMIN))
+		return -EPERM;
+
+	if (priv->phy_dev)
+		return phy_ethtool_sset(priv->phy_dev, cmd);
+
+	return -EINVAL;
+}
+
+/**
+ * arc_emac_get_drvinfo - Get EMAC driver information.
+ * @ndev:	Pointer to net_device structure.
+ * @info:	Pointer to ethtool_drvinfo structure.
+ *
+ * This implements ethtool command for getting the driver information.
+ * Issue "ethtool -i ethX" under linux prompt to execute this function.
+ */
+static void arc_emac_get_drvinfo(struct net_device *ndev,
+				 struct ethtool_drvinfo *info)
+{
+	strlcpy(info->driver, DRV_NAME, sizeof(info->driver));
+	strlcpy(info->version, DRV_VERSION, sizeof(info->version));
+}
+
+static const struct ethtool_ops arc_emac_ethtool_ops = {
+	.get_settings	= arc_emac_get_settings,
+	.set_settings	= arc_emac_set_settings,
+	.get_drvinfo	= arc_emac_get_drvinfo,
+	.get_link	= ethtool_op_get_link,
+};
+
+/**
+ * arc_emac_poll - processing of incoming packets with NAPI.
+ * @napi:	Pointer to napi_struct structure.
+ * @budget:	How many BDs to process on 1 call.
+ *
+ * returns:	Number of processed BDs
+ *
+ * Iterate through Rx BDs and deliver received packages to upper layer.
+ */
+static int arc_emac_poll(struct napi_struct *napi, int budget)
+{
+	struct net_device *ndev = napi->dev;
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+	unsigned int i, loop, work_done = 0;
+	struct sk_buff *skb;
+
+	/* Loop thru the BD chain, but not always from 0.
+	 * Start from right after where we last saw a packet.
+	 */
+	i = priv->last_rx_bd;
+
+	for (loop = 0; loop < RX_BD_NUM; loop++) {
+		struct net_device_stats *stats = &priv->stats;
+		struct buffer_state *rx_buff;
+		struct arc_emac_bd_t *rxbd;
+		dma_addr_t addr;
+		unsigned int info, pktlen;
+		unsigned int buflen = ndev->mtu + EMAC_BUFFER_PAD;
+
+		i = (i + 1) % RX_BD_NUM;
+		rx_buff = &priv->rx_buff[i];
+		rxbd = &priv->rxbd[i];
+		info = rxbd->info;
+
+		/* Process current BD */
+
+		if (unlikely((info & OWN_MASK) == FOR_EMAC)) {
+			/* BD is still owned by EMAC */
+			continue;
+		}
+
+		/* Make a note that we saw a packet at this BD.
+		 * So next time, driver starts from this + 1
+		 */
+		priv->last_rx_bd = i;
+
+#define FRST_OR_LAST_MASK	(FRST_MASK | LAST_MASK)
+
+		if (unlikely((info & FRST_OR_LAST_MASK) != FRST_OR_LAST_MASK)) {
+			/* Buffer chaining results in a significant
+			 * amount of additional bus overhead and thus
+			 * a higher CLK frequency or larger FIFOs are required.
+			 * Because of that fact we don't support
+			 * chaining of receive packets. We pre-allocate
+			 * buffers of MTU size so incoming packets
+			 * won't be chained.
+			 */
+			if (net_ratelimit())
+				netdev_err(ndev,
+					   "incomplete packed received\n");
+
+			/* Return ownership to EMAC */
+			rxbd->info = FOR_EMAC | buflen;
+			stats->rx_errors++;
+			stats->rx_length_errors++;
+			continue;
+		}
+
+		pktlen = info & LEN_MASK;
+		stats->rx_packets++;
+		stats->rx_bytes += pktlen;
+		skb = rx_buff->skb;
+		skb_put(skb, pktlen);
+		skb->dev = ndev;
+		skb->protocol = eth_type_trans(skb, ndev);
+		netif_receive_skb(skb);
+
+		dma_unmap_single(&ndev->dev,
+				 dma_unmap_addr(&rx_buff, addr),
+				 dma_unmap_len(&rx_buff, len),
+				 DMA_FROM_DEVICE);
+
+		/* Prepare the BD for next cycle */
+		rx_buff->skb = netdev_alloc_skb_ip_align(ndev, buflen);
+		if (unlikely(!rx_buff->skb)) {
+			if (net_ratelimit())
+				netdev_err(ndev, "cannot allocate skb\n");
+			stats->rx_errors++;
+			continue;
+		}
+
+		addr = dma_map_single(&ndev->dev, (void *)rx_buff->skb->data,
+				      buflen, DMA_FROM_DEVICE);
+		if (dma_mapping_error(&ndev->dev, addr)) {
+			if (net_ratelimit())
+				if (net_ratelimit())
+					netdev_err(ndev, "cannot dma map\n");
+			dev_kfree_skb(rx_buff->skb);
+			stats->rx_errors++;
+			continue;
+		}
+		dma_unmap_addr_set(&rx_buff, mapping, addr);
+		dma_unmap_len_set(&rx_buff, len, buflen);
+
+		rxbd->data = rx_buff->skb->data;
+
+		/* Return ownership to EMAC */
+		rxbd->info = FOR_EMAC | buflen;
+
+		work_done++;
+		if (work_done >= budget)
+			break;
+	}
+
+	if (work_done < budget) {
+		napi_complete(napi);
+		arc_reg_or(priv, R_ENABLE, RXINT_MASK);
+	}
+
+	return work_done;
+}
+
+/**
+ * arc_emac_intr - Global interrupt handler for EMAC.
+ * @irq:		irq number.
+ * @dev_instance:	device instance.
+ *
+ * returns: IRQ_HANDLED for all cases.
+ *
+ * ARC EMAC has only 1 interrupt line, and depending on bits raised in
+ * STATUS register we may tell what is a reason for interrupt to fire.
+ */
+static irqreturn_t arc_emac_intr(int irq, void *dev_instance)
+{
+	struct net_device *ndev = dev_instance;
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+	struct net_device_stats *stats = &priv->stats;
+	unsigned int status;
+
+	status = arc_reg_get(priv, R_STATUS);
+	status &= ~MDIO_MASK;
+
+	/* Reset all flags except "MDIO complete"*/
+	arc_reg_set(priv, R_STATUS, status);
+
+	if (status & RXINT_MASK) {
+		if (likely(napi_schedule_prep(&priv->napi))) {
+			arc_reg_clr(priv, R_ENABLE, RXINT_MASK);
+			__napi_schedule(&priv->napi);
+		}
+	}
+
+	if (status & TXINT_MASK) {
+		unsigned int i;
+
+		for (i = 0; i < TX_BD_NUM; i++) {
+			unsigned int *txbd_dirty = &priv->txbd_dirty;
+			struct arc_emac_bd_t *txbd = &priv->txbd[*txbd_dirty];
+			struct buffer_state *tx_buff = &priv->tx_buff[*txbd_dirty];
+			struct sk_buff *skb = tx_buff->skb;
+			unsigned int info = txbd->info;
+
+			/* EMAC owns this buffer, cannot touch it now */
+			if (info & FOR_EMAC)
+				continue;
+
+			/* Buffer is not in use currently, skip it */
+			if (!txbd->data)
+				continue;
+
+			if (unlikely(info & (DROP | DEFR | LTCL | UFLO))) {
+				stats->tx_errors++;
+				stats->tx_dropped++;
+
+				if (info & DEFR)
+					stats->tx_carrier_errors++;
+
+				if (info & LTCL)
+					stats->collisions++;
+
+				if (info & UFLO)
+					stats->tx_fifo_errors++;
+			} else if (likely(info & (FRST_MASK | LAST_MASK))) {
+				stats->tx_packets++;
+				stats->tx_bytes += skb->len;
+			}
+
+			dma_unmap_single(&ndev->dev,
+					 dma_unmap_addr(&tx_buff, addr),
+					 dma_unmap_len(&tx_buff, len),
+					 DMA_TO_DEVICE);
+
+			/* return the sk_buff to system */
+			dev_kfree_skb_irq(skb);
+
+			txbd->data = 0x0;
+			txbd->info = 0x0;
+
+			*txbd_dirty = (*txbd_dirty + 1) % TX_BD_NUM;
+
+			if (netif_queue_stopped(ndev))
+				netif_wake_queue(ndev);
+		}
+	}
+
+	if (status & ERR_MASK) {
+		/* MSER/RXCR/RXFR/RXFL interrupt fires on corresponding
+		 * 8-bit error counter overrun.
+		 * Because of this fact we add 256 (0x100) items each time
+		 * overrun interrupt happens.
+		 */
+
+		if (status & MSER_MASK) {
+			stats->rx_missed_errors += 0x100;
+			stats->rx_errors += 0x100;
+		}
+
+		if (status & RXCR_MASK) {
+			stats->rx_crc_errors += 0x100;
+			stats->rx_errors += 0x100;
+		}
+
+		if (status & RXFR_MASK) {
+			stats->rx_frame_errors += 0x100;
+			stats->rx_errors += 0x100;
+		}
+
+		if (status & RXFL_MASK) {
+			stats->rx_over_errors += 0x100;
+			stats->rx_errors += 0x100;
+		}
+	}
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * arc_emac_open - Open the network device.
+ * @ndev:	Pointer to the network device.
+ *
+ * returns: 0, on success or non-zero error value on failure.
+ *
+ * This function sets the MAC address, requests and enables an IRQ
+ * for the EMAC device and starts the Tx queue.
+ * It also connects to the phy device.
+ */
+static int arc_emac_open(struct net_device *ndev)
+{
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+	struct phy_device *phy_dev = priv->phy_dev;
+	struct arc_emac_bd_t *bd;
+	struct sk_buff *skb;
+	int i;
+
+	phy_dev->autoneg = AUTONEG_ENABLE;
+	phy_dev->speed = 0;
+	phy_dev->duplex = 0;
+	phy_dev->advertising = phy_dev->supported;
+
+	if (priv->max_speed > 100) {
+		phy_dev->advertising &= PHY_GBIT_FEATURES;
+	} else if (priv->max_speed <= 100) {
+		phy_dev->advertising &= PHY_BASIC_FEATURES;
+		if (priv->max_speed <= 10) {
+			phy_dev->advertising &= ~SUPPORTED_100baseT_Half;
+			phy_dev->advertising &= ~SUPPORTED_100baseT_Full;
+		}
+	}
+
+	/* Allocate and set buffers for Rx BD's */
+	bd = priv->rxbd;
+	for (i = 0; i < RX_BD_NUM; i++) {
+		skb = netdev_alloc_skb_ip_align(ndev, ndev->mtu +
+						EMAC_BUFFER_PAD);
+
+		priv->rx_buff[i].skb = skb;
+		bd->data = skb->data;
+
+		/* Set ownership to EMAC */
+		bd->info = FOR_EMAC | (ndev->mtu + EMAC_BUFFER_PAD);
+		bd++;
+	}
+
+	/* setup last seen to MAX, so driver starts from 0 */
+	priv->last_rx_bd = RX_BD_NUM - 1;
+
+	/* Allocate Tx BD's similar to Rx BD's. All Tx BD's owned by CPU */
+	bd = priv->txbd;
+	for (i = 0; i < TX_BD_NUM; i++) {
+		bd->data = 0;
+		bd->info = 0;
+		bd++;
+	}
+
+	/* Initialize logical address filter */
+	arc_reg_set(priv, R_LAFL, 0);
+	arc_reg_set(priv, R_LAFH, 0);
+
+	/* Set BD ring pointers for device side */
+	arc_reg_set(priv, R_RX_RING,
+		     (unsigned int)priv->rxbd_dma_hdl);
+
+	arc_reg_set(priv, R_TX_RING,
+		     (unsigned int)priv->rxbd_dma_hdl + RX_RING_SZ);
+
+	/* Set Poll rate so that it polls every 1 ms */
+	arc_reg_set(priv, R_POLLRATE,
+		     priv->clock_frequency / 1000000);
+
+	/* Enable interrupts */
+	arc_reg_set(priv, R_ENABLE,
+		     TXINT_MASK | RXINT_MASK | ERR_MASK);
+
+	/* Set CONTROL */
+	arc_reg_set(priv, R_CTRL,
+		     (RX_BD_NUM << 24) |	/* RX BD table length */
+		     (TX_BD_NUM << 16) |	/* TX BD table length */
+		     TXRN_MASK | RXRN_MASK);
+
+	/* Enable EMAC */
+	arc_reg_or(priv, R_CTRL, EN_MASK);
+
+	phy_start_aneg(priv->phy_dev);
+
+	netif_start_queue(ndev);
+	napi_enable(&priv->napi);
+
+	return 0;
+}
+
+/**
+ * arc_emac_stop - Close the network device.
+ * @ndev:	Pointer to the network device.
+ *
+ * This function stops the Tx queue, disables interrupts and frees the IRQ for
+ * the EMAC device.
+ * It also disconnects the PHY device associated with the EMAC device.
+ */
+static int arc_emac_stop(struct net_device *ndev)
+{
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+
+	napi_disable(&priv->napi);
+	netif_stop_queue(ndev);
+
+	/* Disable interrupts */
+	arc_reg_clr(priv, R_ENABLE, TXINT_MASK | RXINT_MASK | ERR_MASK);
+
+	/* Disable EMAC */
+	arc_reg_clr(priv, R_CTRL, EN_MASK);
+
+	return 0;
+}
+
+/**
+ * arc_emac_stats - Get system network statistics.
+ * @ndev:	Pointer to net_device structure.
+ *
+ * Returns the address of the device statistics structure.
+ * Statistics are updated in interrupt handler.
+ */
+static struct net_device_stats *arc_emac_stats(struct net_device *ndev)
+{
+	unsigned long miss, rxerr, rxfram, rxcrc, rxoflow;
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+	struct net_device_stats *stats = &priv->stats;
+
+	rxerr = arc_reg_get(priv, R_RXERR);
+	miss = arc_reg_get(priv, R_MISS);
+
+	rxcrc = rxerr & 0xff;
+	rxfram = rxerr >> 8 & 0xff;
+	rxoflow = rxerr >> 16 & 0xff;
+
+	stats->rx_errors += miss;
+	stats->rx_errors += rxcrc + rxfram + rxoflow;
+
+	stats->rx_over_errors += rxoflow;
+	stats->rx_frame_errors += rxfram;
+	stats->rx_crc_errors += rxcrc;
+	stats->rx_missed_errors += miss;
+
+	return stats;
+}
+
+/**
+ * arc_emac_tx - Starts the data transmission.
+ * @skb:	sk_buff pointer that contains data to be Transmitted.
+ * @ndev:	Pointer to net_device structure.
+ *
+ * returns: NETDEV_TX_OK, on success
+ *		NETDEV_TX_BUSY, if any of the descriptors are not free.
+ *
+ * This function is invoked from upper layers to initiate transmission.
+ */
+static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+	unsigned int info, len, *txbd_curr = &priv->txbd_curr;
+	dma_addr_t addr;
+	char *pkt = skb->data;
+
+	len = max_t(unsigned int, ETH_ZLEN, skb->len);
+	info = priv->txbd[*txbd_curr].info;
+
+	/* EMAC still holds this buffer in its possession.
+	 * CPU must not modify this buffer descriptor
+	 */
+	if (unlikely((info & OWN_MASK) == FOR_EMAC)) {
+		netif_stop_queue(ndev);
+		return NETDEV_TX_BUSY;
+	}
+
+	addr = dma_map_single(&ndev->dev, (void *)pkt, len, DMA_TO_DEVICE);
+	if (unlikely(dma_mapping_error(&ndev->dev, addr))) {
+		dev_kfree_skb(skb);
+		return NETDEV_TX_OK;
+	}
+	dma_unmap_addr_set(&priv->tx_buff[*txbd_curr], mapping, addr);
+	dma_unmap_len_set(&priv->tx_buff[*txbd_curr], len, len);
+
+	priv->tx_buff[*txbd_curr].skb = skb;
+	priv->txbd[*txbd_curr].data = pkt;
+	priv->txbd[*txbd_curr].info = FOR_EMAC | FRST_MASK | LAST_MASK | len;
+
+	*txbd_curr = (*txbd_curr + 1) % TX_BD_NUM;
+
+	arc_reg_set(priv, R_STATUS, TXPL_MASK);
+
+	skb_tx_timestamp(skb);
+
+	return NETDEV_TX_OK;
+}
+
+/**
+ * arc_emac_set_address - Set the MAC address for this device.
+ * @ndev:	Pointer to net_device structure.
+ * @p:		6 byte Address to be written as MAC address.
+ *
+ * This function copies the HW address from the sockaddr structure to the
+ * net_device structure and updates the address in HW.
+ *
+ * returns:	-EBUSY if the net device is busy or 0 if the address is set
+ *		successfully.
+ */
+static int arc_emac_set_address(struct net_device *ndev, void *p)
+{
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+	struct sockaddr *addr = p;
+	unsigned int addr_low, addr_hi;
+
+	if (netif_running(ndev))
+		return -EBUSY;
+
+	if (!is_valid_ether_addr(addr->sa_data))
+		return -EADDRNOTAVAIL;
+
+	memcpy(ndev->dev_addr, addr->sa_data, ndev->addr_len);
+
+	addr_low = le32_to_cpu(*(__le32 *) &ndev->dev_addr[0]);
+	addr_hi = le16_to_cpu(*(__le16 *) &ndev->dev_addr[4]);
+
+	arc_reg_set(priv, R_ADDRL, addr_low);
+	arc_reg_set(priv, R_ADDRH, addr_hi);
+
+	return 0;
+}
+
+static const struct net_device_ops arc_emac_netdev_ops = {
+	.ndo_open		= arc_emac_open,
+	.ndo_stop		= arc_emac_stop,
+	.ndo_start_xmit		= arc_emac_tx,
+	.ndo_set_mac_address	= arc_emac_set_address,
+	.ndo_get_stats		= arc_emac_stats,
+};
+
+static int arc_emac_probe(struct platform_device *pdev)
+{
+	struct net_device *ndev;
+	struct arc_emac_priv *priv;
+	int err;
+	unsigned int id;
+	struct resource res_regs, res_irq;
+	const char *mac_addr;
+
+	/* no device tree device */
+	if (!pdev->dev.of_node)
+		return -ENODEV;
+
+	ndev = alloc_etherdev(sizeof(struct arc_emac_priv));
+	if (!ndev) {
+		err = -ENOMEM;
+		goto out;
+	}
+
+	SET_NETDEV_DEV(ndev, &pdev->dev);
+
+	/* Populate our net_device structure */
+	ndev->netdev_ops = &arc_emac_netdev_ops;
+	ndev->ethtool_ops = &arc_emac_ethtool_ops;
+	ndev->watchdog_timeo = TX_TIMEOUT;
+	/* FIXME :: no multicast support yet */
+	ndev->flags &= ~IFF_MULTICAST;
+
+	priv = netdev_priv(ndev);
+	priv->dev = &pdev->dev;
+	priv->ndev = ndev;
+
+	/* Get PHY from device tree */
+	priv->phy_node = of_parse_phandle(pdev->dev.of_node, "phy", 0);
+	if (!priv->phy_node) {
+		dev_err(&pdev->dev, "failed to retrieve phy description from device tree\n");
+		err = -ENODEV;
+		goto out;
+	}
+
+	/* Get EMAC registers base address from device tree */
+	err = of_address_to_resource(pdev->dev.of_node, 0, &res_regs);
+	if (err) {
+		dev_err(&pdev->dev, "failed to retrieve registers base from device tree\n");
+		err = -ENODEV;
+		goto out;
+	}
+
+	priv->reg_base_addr = devm_ioremap_resource(&pdev->dev, &res_regs);
+	if (IS_ERR(priv->reg_base_addr)) {
+		err = PTR_ERR(priv->reg_base_addr);
+		goto out;
+	}
+	dev_dbg(&pdev->dev, "Registers base address is 0x%p\n",
+		priv->reg_base_addr);
+
+	id = arc_reg_get(priv, R_ID);
+	/* Check for EMAC revision 5 or 7, magic number */
+	if (!(id == 0x0005fd02 || id == 0x0007fd02)) {
+		dev_err(&pdev->dev, "ARC EMAC not detected, id=0x%x\n", id);
+		err = -ENODEV;
+		goto out;
+	}
+	dev_info(&pdev->dev, "ARC EMAC detected with id: 0x%x\n", id);
+
+	/* Get CPU clock frequency from device tree */
+	if (of_property_read_u32(pdev->dev.of_node, "clock-frequency",
+				 &priv->clock_frequency)) {
+		dev_err(&pdev->dev, "failed to retrieve <clock-frequency> from device tree\n");
+		err = -EINVAL;
+		goto out;
+	}
+
+	/* Get max speed of operation from device tree */
+	if (of_property_read_u32(pdev->dev.of_node, "max-speed",
+				 &priv->max_speed)) {
+		dev_err(&pdev->dev, "failed to retrieve <max-speed> from device tree\n");
+		err = -EINVAL;
+		goto out;
+	}
+
+	/* Get IRQ from device tree */
+	err = of_irq_to_resource(pdev->dev.of_node, 0, &res_irq);
+	if (!err) {
+		dev_err(&pdev->dev, "failed to retrieve <irq> value from device tree\n");
+		err = -ENODEV;
+		goto out;
+	}
+	ndev->irq = res_irq.start;
+	dev_info(&pdev->dev, "IRQ is %d\n", ndev->irq);
+
+	/* Register interrupt handler for device */
+	err = devm_request_irq(&pdev->dev, ndev->irq, arc_emac_intr, 0,
+			       ndev->name, ndev);
+	if (err) {
+		dev_err(&pdev->dev, "could not allocate IRQ\n");
+		goto out;
+	}
+
+	/* Get MAC address from device tree */
+	mac_addr = of_get_mac_address(pdev->dev.of_node);
+
+	if (!mac_addr || !is_valid_ether_addr(mac_addr))
+		eth_hw_addr_random(ndev);
+	else
+		memcpy(ndev->dev_addr, mac_addr, ETH_ALEN);
+
+	dev_info(&pdev->dev, "MAC address is now %pM\n", ndev->dev_addr);
+
+	/* Allocate cache coherent memory for BD Rings - to avoid need to do
+	 * explicit uncached ".di" accesses, which can potentially screw-up
+	 */
+	priv->rxbd = dmam_alloc_coherent(&pdev->dev, RX_RING_SZ + TX_RING_SZ,
+					 &priv->rxbd_dma_hdl, GFP_KERNEL);
+
+	if (!priv->rxbd) {
+		dev_err(&pdev->dev, "failed to allocate data buffers\n");
+		err = -ENOMEM;
+		goto out;
+	}
+
+	priv->txbd = priv->rxbd + RX_BD_NUM;
+
+	/* To keep things simple - we just do 1 big allocation, instead of 2
+	 * separate ones for Rx and Tx Rings responsively
+	 */
+	priv->txbd_dma_hdl = priv->rxbd_dma_hdl + RX_RING_SZ;
+	dev_dbg(&pdev->dev, "EMAC Device addr: Rx Ring [0x%x], Tx Ring[%x]\n",
+		(unsigned int)priv->rxbd_dma_hdl,
+		(unsigned int)priv->txbd_dma_hdl);
+
+	err = arc_mdio_probe(pdev->dev.of_node, priv);
+	if (err) {
+		dev_err(&pdev->dev, "failed to probe MII bus\n");
+		goto out;
+	}
+
+
+	priv->phy_dev = of_phy_connect(ndev, priv->phy_node,
+				       arc_emac_adjust_link, 0,
+				       PHY_INTERFACE_MODE_MII);
+	if (!priv->phy_dev) {
+		netdev_err(ndev, "of_phy_connect() failed\n");
+		return -ENODEV;
+	}
+
+	dev_info(&pdev->dev, "connected to %s phy with id 0x%x\n",
+		 priv->phy_dev->drv->name, priv->phy_dev->phy_id);
+
+	netif_napi_add(ndev, &priv->napi, arc_emac_poll, ARC_EMAC_NAPI_WEIGHT);
+
+	err = register_netdev(ndev);
+	if (err) {
+		netif_napi_del(&priv->napi);
+		dev_err(&pdev->dev, "failed to register network device\n");
+		goto out;
+	}
+
+	return 0;
+
+out:
+	free_netdev(ndev);
+	return err;
+}
+
+static int arc_emac_remove(struct platform_device *pdev)
+{
+	struct net_device *ndev = platform_get_drvdata(pdev);
+	struct arc_emac_priv *priv = netdev_priv(ndev);
+
+	phy_disconnect(priv->phy_dev);
+	priv->phy_dev = NULL;
+	arc_mdio_remove(priv);
+	unregister_netdev(ndev);
+	netif_napi_del(&priv->napi);
+	free_netdev(ndev);
+
+	return 0;
+}
+
+static const struct of_device_id arc_emac_dt_ids[] = {
+	{ .compatible = "snps,arc-emac" },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, arc_emac_dt_ids);
+
+static struct platform_driver arc_emac_driver = {
+	.probe = arc_emac_probe,
+	.remove = arc_emac_remove,
+	.driver = {
+		.name = DRV_NAME,
+		.owner = THIS_MODULE,
+		.of_match_table  = arc_emac_dt_ids,
+		},
+};
+
+module_platform_driver(arc_emac_driver);
+
+MODULE_AUTHOR("Alexey Brodkin <abrodkin@synopsys.com>");
+MODULE_DESCRIPTION("ARC EMAC driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/net/ethernet/arc/arc_emac_mdio.c b/drivers/net/ethernet/arc/arc_emac_mdio.c
new file mode 100644
index 0000000..3ae48d4
--- /dev/null
+++ b/drivers/net/ethernet/arc/arc_emac_mdio.c
@@ -0,0 +1,152 @@
+/*
+ * Copyright (C) 2004-2013 Synopsys, Inc. (www.synopsys.com)
+ *
+ * MDIO implementation for ARC EMAC
+ */
+
+#include <linux/delay.h>
+#include <linux/of_mdio.h>
+
+#include "arc_emac.h"
+
+/* Number of seconds we wait for "MDIO complete" flag to appear */
+#define ARC_MDIO_COMPLETE_POLL_COUNT	1
+
+/**
+ * arc_mdio_complete_wait - Waits until MDIO transaction is completed.
+ * @priv:	Pointer to ARC EMAC private data structure.
+ *
+ * returns:	0 on success, -ETIMEDOUT on a timeout.
+ */
+static int arc_mdio_complete_wait(struct arc_emac_priv *priv)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARC_MDIO_COMPLETE_POLL_COUNT * 40; i++) {
+		unsigned int status = arc_reg_get(priv, R_STATUS);
+
+		status &= MDIO_MASK;
+
+		if (status) {
+			/* Reset "MDIO complete" flag */
+			arc_reg_set(priv, R_STATUS, status);
+			return 0;
+		}
+
+		msleep(25);
+	}
+
+	return -ETIMEDOUT;
+}
+
+/**
+ * arc_mdio_read - MDIO interface read function.
+ * @bus:	Pointer to MII bus structure.
+ * @phy_addr:	Address of the PHY device.
+ * @reg_num:	PHY register to read.
+ *
+ * returns:	The register contents on success, -ETIMEDOUT on a timeout.
+ *
+ * Reads the contents of the requested register from the requested PHY
+ * address.
+ */
+static int arc_mdio_read(struct mii_bus *bus, int phy_addr, int reg_num)
+{
+	struct arc_emac_priv *priv = bus->priv;
+	unsigned int value;
+	int error;
+
+	arc_reg_set(priv, R_MDIO,
+		    0x60020000 | (phy_addr << 23) | (reg_num << 18));
+
+	error = arc_mdio_complete_wait(priv);
+	if (error < 0)
+		return error;
+
+	value = arc_reg_get(priv, R_MDIO) & 0xffff;
+
+	dev_dbg(priv->dev, "arc_mdio_read(phy_addr=%i, reg_num=%x) = %x\n",
+		phy_addr, reg_num, value);
+
+	return value;
+}
+
+/**
+ * arc_mdio_write - MDIO interface write function.
+ * @bus:	Pointer to MII bus structure.
+ * @phy_addr:	Address of the PHY device.
+ * @reg_num:	PHY register to write to.
+ * @value:	Value to be written into the register.
+ *
+ * returns:	0 on success, -ETIMEDOUT on a timeout.
+ *
+ * Writes the value to the requested register.
+ */
+static int arc_mdio_write(struct mii_bus *bus, int phy_addr,
+			  int reg_num, u16 value)
+{
+	struct arc_emac_priv *priv = bus->priv;
+
+	dev_dbg(priv->dev,
+		"arc_mdio_write(phy_addr=%i, reg_num=%x, value=%x)\n",
+		phy_addr, reg_num, value);
+
+	arc_reg_set(priv, R_MDIO,
+		     0x50020000 | (phy_addr << 23) | (reg_num << 18) | value);
+
+	return arc_mdio_complete_wait(priv);
+}
+
+/**
+ * arc_mdio_probe - MDIO probe function.
+ * @dev_node:	Pointer to device node.
+ * @priv:	Pointer to ARC EMAC private data structure.
+ *
+ * returns:	0 on success, -ENOMEM when mdiobus_alloc
+ * (to allocate memory for MII bus structure) fails.
+ *
+ * Sets up and registers the MDIO interface.
+ */
+int arc_mdio_probe(struct device_node *dev_node, struct arc_emac_priv *priv)
+{
+	struct mii_bus *bus;
+	int error;
+
+	bus = mdiobus_alloc();
+	if (!bus)
+		return -ENOMEM;
+
+	priv->bus = bus;
+	bus->priv = priv;
+	bus->parent = priv->dev;
+	bus->name = "Synopsys MII Bus",
+	bus->read = &arc_mdio_read;
+	bus->write = &arc_mdio_write;
+
+	snprintf(bus->id, MII_BUS_ID_SIZE, "%.8x",
+		 (unsigned int)priv->reg_base_addr);
+
+	error = of_mdiobus_register(bus, dev_node);
+	if (error) {
+		dev_err(priv->dev, "cannot register MDIO bus %s\n", bus->name);
+		mdiobus_free(bus);
+		return error;
+	}
+
+	return 0;
+}
+
+/**
+ * arc_mdio_remove - MDIO remove function.
+ * @priv:	Pointer to ARC EMAC private data structure.
+ *
+ * Unregisters the MDIO and frees any associate memory for MII bus.
+ */
+int arc_mdio_remove(struct arc_emac_priv *priv)
+{
+	mdiobus_unregister(priv->bus);
+	mdiobus_free(priv->bus);
+	priv->bus = NULL;
+
+	return 0;
+}
-- 
1.7.10.4

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

* Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver
  2013-06-13 14:37 ` Alexey Brodkin
  (?)
@ 2013-06-13 18:25 ` Andy Shevchenko
  2013-06-13 18:33     ` Joe Perches
                     ` (2 more replies)
  -1 siblings, 3 replies; 21+ messages in thread
From: Andy Shevchenko @ 2013-06-13 18:25 UTC (permalink / raw)
  To: Alexey Brodkin
  Cc: netdev, Francois Romieu, Joe Perches, Vineet Gupta,
	Mischa Jonker, Arnd Bergmann, Grant Likely, Rob Herring,
	Paul Gortmaker, David S. Miller, linux-kernel,
	Devicetree Discuss

On Thu, Jun 13, 2013 at 5:37 PM, Alexey Brodkin
<Alexey.Brodkin@synopsys.com> wrote:
> Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
> instantiated in some legacy ARC (Synopsys) FPGA Boards such as
> ARCAngel4/ML50x.

Much better. But still few comments below.

> +++ b/drivers/net/ethernet/arc/arc_emac.h

What the point to keep arc_emac prefix for files? You have separate
folder for them. This particular one can be main.h.

> +struct arc_emac_priv {
> +       struct net_device_stats stats;
> +       unsigned int clock_frequency;
> +       unsigned int max_speed;
> +
> +       /* Pointers to BD rings - CPU side */
> +       struct arc_emac_bd_t *rxbd;
> +       struct arc_emac_bd_t *txbd;

Usually "_t" means "type" that is used in definitions without 'struct' word.
I don't think this is the case here.

> +       /* Pointers to BD rings - Device side */
> +       dma_addr_t rxbd_dma_hdl;
> +       dma_addr_t txbd_dma_hdl;
> +
> +       /* The actual socket buffers */
> +       struct buffer_state rx_buff[RX_BD_NUM];
> +       struct buffer_state tx_buff[TX_BD_NUM];
> +       unsigned int txbd_curr;
> +       unsigned int txbd_dirty;
> +
> +       /* Remember where driver last saw a packet, so next iteration it
> +        * starts from here and not 0
> +        */
> +       unsigned int last_rx_bd;
> +
> +       struct napi_struct napi;
> +
> +       /* Phy saved state */
> +       int duplex;
> +       int speed;
> +
> +       /* Devices */
> +       struct device *dev;
> +       struct device_node *phy_node;
> +       struct phy_device *phy_dev;
> +       struct net_device *ndev;
> +       struct mii_bus *bus;
> +
> +       /* EMAC registers base address */
> +       void __iomem *reg_base_addr;
> +};

It seems you missed my comments against the names of the members. Can
you address them or comment why not?

> +++ b/drivers/net/ethernet/arc/arc_emac_main.c

> +static int arc_emac_get_settings(struct net_device *ndev,
> +                                struct ethtool_cmd *cmd)
> +{
> +       struct arc_emac_priv *priv = netdev_priv(ndev);
> +
> +       if (priv->phy_dev)
> +               return phy_ethtool_gset(priv->phy_dev, cmd);
> +
> +       return -EINVAL;

May be
if (!...)
 return -EINVAL;
return ...;

> +static int arc_emac_set_settings(struct net_device *ndev,
> +                                struct ethtool_cmd *cmd)
> +{
> +       struct arc_emac_priv *priv = netdev_priv(ndev);
> +
> +       if (!capable(CAP_NET_ADMIN))
> +               return -EPERM;
> +
> +       if (priv->phy_dev)
> +               return phy_ethtool_sset(priv->phy_dev, cmd);
> +
> +       return -EINVAL;

Ditto.

> +static int arc_emac_poll(struct napi_struct *napi, int budget)
> +{
> +       struct net_device *ndev = napi->dev;
> +       struct arc_emac_priv *priv = netdev_priv(ndev);
> +       unsigned int i, loop, work_done = 0;
> +       struct sk_buff *skb;

> +       for (loop = 0; loop < RX_BD_NUM; loop++) {
> +               struct net_device_stats *stats = &priv->stats;
> +               struct buffer_state *rx_buff;
> +               struct arc_emac_bd_t *rxbd;
> +               dma_addr_t addr;
> +               unsigned int info, pktlen;
> +               unsigned int buflen = ndev->mtu + EMAC_BUFFER_PAD;
> +
> +               i = (i + 1) % RX_BD_NUM;

I just realize you are using circular buffer here. What about to use
them with linux' native framework? Documentation/circular-buffers.txt

> +               if (unlikely((info & OWN_MASK) == FOR_EMAC)) {
> +                       /* BD is still owned by EMAC */
> +                       continue;
> +               }

Redundant braces.

> +               addr = dma_map_single(&ndev->dev, (void *)rx_buff->skb->data,
> +                                     buflen, DMA_FROM_DEVICE);
> +               if (dma_mapping_error(&ndev->dev, addr)) {
> +                       if (net_ratelimit())
> +                               if (net_ratelimit())

Is it not a typo?

> +                                       netdev_err(ndev, "cannot dma map\n");

> +               work_done++;
> +               if (work_done >= budget)
> +                       break;

Those three could easily go to the for () on the top of this function.

> +static irqreturn_t arc_emac_intr(int irq, void *dev_instance)
> +{

> +       if (status & TXINT_MASK) {
> +               unsigned int i;
> +
> +               for (i = 0; i < TX_BD_NUM; i++) {
> +                       unsigned int *txbd_dirty = &priv->txbd_dirty;
> +                       struct arc_emac_bd_t *txbd = &priv->txbd[*txbd_dirty];
> +                       struct buffer_state *tx_buff = &priv->tx_buff[*txbd_dirty];
> +                       struct sk_buff *skb = tx_buff->skb;
> +                       unsigned int info = txbd->info;

> +                       txbd->data = 0x0;
> +                       txbd->info = 0x0;

Plain 0?

> +                       *txbd_dirty = (*txbd_dirty + 1) % TX_BD_NUM;

Same idea about circular buffers.

> +static int arc_emac_open(struct net_device *ndev)
> +{

> +       /* Set Poll rate so that it polls every 1 ms */
> +       arc_reg_set(priv, R_POLLRATE,
> +                    priv->clock_frequency / 1000000);

I don't understand how you end up with 1ms here. 1000000 is just a
magic number, clock_frequency generally is an arbitrary value.

> +static struct net_device_stats *arc_emac_stats(struct net_device *ndev)
> +{
> +       unsigned long miss, rxerr, rxfram, rxcrc, rxoflow;
> +       struct arc_emac_priv *priv = netdev_priv(ndev);
> +       struct net_device_stats *stats = &priv->stats;
> +
> +       rxerr = arc_reg_get(priv, R_RXERR);
> +       miss = arc_reg_get(priv, R_MISS);
> +
> +       rxcrc = rxerr & 0xff;
> +       rxfram = rxerr >> 8 & 0xff;
> +       rxoflow = rxerr >> 16 & 0xff;

If you declare those three as u8, you can avoid those & 0xff.

> +       ndev = alloc_etherdev(sizeof(struct arc_emac_priv));
> +       if (!ndev) {
> +               err = -ENOMEM;
> +               goto out;
> +       }

Redundant goto. Just return here.

> +       }
> +
> +

Extra line.

--
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver
@ 2013-06-13 18:33     ` Joe Perches
  0 siblings, 0 replies; 21+ messages in thread
From: Joe Perches @ 2013-06-13 18:33 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Alexey Brodkin, netdev, Francois Romieu, Vineet Gupta,
	Mischa Jonker, Arnd Bergmann, Grant Likely, Rob Herring,
	Paul Gortmaker, David S. Miller, linux-kernel,
	Devicetree Discuss

On Thu, 2013-06-13 at 21:25 +0300, Andy Shevchenko wrote:
> On Thu, Jun 13, 2013 at 5:37 PM, Alexey Brodkin
> <Alexey.Brodkin@synopsys.com> wrote:
> > Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
> > instantiated in some legacy ARC (Synopsys) FPGA Boards such as
> > ARCAngel4/ML50x.
> 
> Much better. But still few comments below.

Sensible comments save for this truly trivial one

> > +++ b/drivers/net/ethernet/arc/arc_emac.h
[]
> > +               if (unlikely((info & OWN_MASK) == FOR_EMAC)) {
> > +                       /* BD is still owned by EMAC */
> > +                       continue;
> > +               }
> 
> Redundant braces.

Maybe not.  Braces can be a visual aid when there is
a comment in the test.



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

* Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver
@ 2013-06-13 18:33     ` Joe Perches
  0 siblings, 0 replies; 21+ messages in thread
From: Joe Perches @ 2013-06-13 18:33 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Devicetree Discuss, netdev, Vineet Gupta, Alexey Brodkin,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
	David S. Miller, Paul Gortmaker, Francois Romieu, Grant Likely,
	Mischa Jonker

On Thu, 2013-06-13 at 21:25 +0300, Andy Shevchenko wrote:
> On Thu, Jun 13, 2013 at 5:37 PM, Alexey Brodkin
> <Alexey.Brodkin-HKixBCOQz3hWk0Htik3J/w@public.gmane.org> wrote:
> > Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
> > instantiated in some legacy ARC (Synopsys) FPGA Boards such as
> > ARCAngel4/ML50x.
> 
> Much better. But still few comments below.

Sensible comments save for this truly trivial one

> > +++ b/drivers/net/ethernet/arc/arc_emac.h
[]
> > +               if (unlikely((info & OWN_MASK) == FOR_EMAC)) {
> > +                       /* BD is still owned by EMAC */
> > +                       continue;
> > +               }
> 
> Redundant braces.

Maybe not.  Braces can be a visual aid when there is
a comment in the test.

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

* Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver
  2013-06-13 18:33     ` Joe Perches
  (?)
@ 2013-06-13 19:16     ` Andy Shevchenko
  -1 siblings, 0 replies; 21+ messages in thread
From: Andy Shevchenko @ 2013-06-13 19:16 UTC (permalink / raw)
  To: Joe Perches
  Cc: Alexey Brodkin, netdev, Francois Romieu, Vineet Gupta,
	Mischa Jonker, Arnd Bergmann, Grant Likely, Rob Herring,
	Paul Gortmaker, David S. Miller, linux-kernel,
	Devicetree Discuss

On Thu, Jun 13, 2013 at 9:33 PM, Joe Perches <joe@perches.com> wrote:
> On Thu, 2013-06-13 at 21:25 +0300, Andy Shevchenko wrote:
>> On Thu, Jun 13, 2013 at 5:37 PM, Alexey Brodkin
>> <Alexey.Brodkin@synopsys.com> wrote:
>> > Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
>> > instantiated in some legacy ARC (Synopsys) FPGA Boards such as
>> > ARCAngel4/ML50x.

>> > +++ b/drivers/net/ethernet/arc/arc_emac.h
> []
>> > +               if (unlikely((info & OWN_MASK) == FOR_EMAC)) {
>> > +                       /* BD is still owned by EMAC */
>> > +                       continue;
>> > +               }
>>
>> Redundant braces.
>
> Maybe not.  Braces can be a visual aid when there is
> a comment in the test.

I'm okay with both approaches. If author considers the comment is
useful in that form, I agree with him.

--
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver
  2013-06-13 18:25 ` Andy Shevchenko
  2013-06-13 18:33     ` Joe Perches
@ 2013-06-13 19:42   ` Francois Romieu
  2013-06-13 20:25   ` Alexey Brodkin
  2 siblings, 0 replies; 21+ messages in thread
From: Francois Romieu @ 2013-06-13 19:42 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Alexey Brodkin, netdev, Joe Perches, Vineet Gupta, Mischa Jonker,
	Arnd Bergmann, Grant Likely, Rob Herring, Paul Gortmaker,
	David S. Miller, linux-kernel, Devicetree Discuss

Andy Shevchenko <andy.shevchenko@gmail.com> :
[...]
> I just realize you are using circular buffer here. What about to use
> them with linux' native framework? Documentation/circular-buffers.txt

It's almost unused in drivers/net. I won't mind if it stays this way.

-- 
Ueimor

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

* Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver
  2013-06-13 18:33     ` Joe Perches
  (?)
  (?)
@ 2013-06-13 19:54     ` David Miller
  -1 siblings, 0 replies; 21+ messages in thread
From: David Miller @ 2013-06-13 19:54 UTC (permalink / raw)
  To: joe
  Cc: andy.shevchenko, Alexey.Brodkin, netdev, romieu, Vineet.Gupta1,
	Mischa.Jonker, arnd, grant.likely, rob.herring, paul.gortmaker,
	linux-kernel, devicetree-discuss

From: Joe Perches <joe@perches.com>
Date: Thu, 13 Jun 2013 11:33:21 -0700

> On Thu, 2013-06-13 at 21:25 +0300, Andy Shevchenko wrote:
>> On Thu, Jun 13, 2013 at 5:37 PM, Alexey Brodkin
>> > +               if (unlikely((info & OWN_MASK) == FOR_EMAC)) {
>> > +                       /* BD is still owned by EMAC */
>> > +                       continue;
>> > +               }
>> 
>> Redundant braces.
> 
> Maybe not.  Braces can be a visual aid when there is
> a comment in the test.

Agreed.

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

* Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver
  2013-06-13 18:25 ` Andy Shevchenko
  2013-06-13 18:33     ` Joe Perches
  2013-06-13 19:42   ` Francois Romieu
@ 2013-06-13 20:25   ` Alexey Brodkin
  2013-06-13 20:50     ` Andy Shevchenko
  2 siblings, 1 reply; 21+ messages in thread
From: Alexey Brodkin @ 2013-06-13 20:25 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: netdev, Francois Romieu, Joe Perches, Vineet Gupta,
	Mischa Jonker, Arnd Bergmann, Grant Likely, Rob Herring,
	Paul Gortmaker, David S. Miller, linux-kernel,
	Devicetree Discuss

On 06/13/2013 10:25 PM, Andy Shevchenko wrote:
> On Thu, Jun 13, 2013 at 5:37 PM, Alexey Brodkin
> <Alexey.Brodkin@synopsys.com> wrote:
>> Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
>> instantiated in some legacy ARC (Synopsys) FPGA Boards such as
>> ARCAngel4/ML50x.
>
> Much better. But still few comments below.
>
>> +++ b/drivers/net/ethernet/arc/arc_emac.h
>
> What the point to keep arc_emac prefix for files? You have separate
> folder for them. This particular one can be main.h.

A reason is to still have an ability to tell what's a source a developer 
is dealing with. But in general your idea looks good. Will simplify names.

And what about function names? Do you think it worth to shorten them too 
since most of them aren't visible outside (static).

>> +struct arc_emac_priv {
>> +       struct net_device_stats stats;
>> +       unsigned int clock_frequency;
>> +       unsigned int max_speed;
>> +
>> +       /* Pointers to BD rings - CPU side */
>> +       struct arc_emac_bd_t *rxbd;
>> +       struct arc_emac_bd_t *txbd;
>
> Usually "_t" means "type" that is used in definitions without 'struct' word.
> I don't think this is the case here.

Right, it is a legacy. Back in the day it was "typedef" I later changed 
into "struct ...".

>> +       /* Pointers to BD rings - Device side */
>> +       dma_addr_t rxbd_dma_hdl;
>> +       dma_addr_t txbd_dma_hdl;
>> +
>> +       /* The actual socket buffers */
>> +       struct buffer_state rx_buff[RX_BD_NUM];
>> +       struct buffer_state tx_buff[TX_BD_NUM];
>> +       unsigned int txbd_curr;
>> +       unsigned int txbd_dirty;
>> +
>> +       /* Remember where driver last saw a packet, so next iteration it
>> +        * starts from here and not 0
>> +        */
>> +       unsigned int last_rx_bd;
>> +
>> +       struct napi_struct napi;
>> +
>> +       /* Phy saved state */
>> +       int duplex;
>> +       int speed;
>> +
>> +       /* Devices */
>> +       struct device *dev;
>> +       struct device_node *phy_node;
>> +       struct phy_device *phy_dev;
>> +       struct net_device *ndev;
>> +       struct mii_bus *bus;
>> +
>> +       /* EMAC registers base address */
>> +       void __iomem *reg_base_addr;
>> +};
>
> It seems you missed my comments against the names of the members. Can
> you address them or comment why not?

You mean to add description in kerneldoc format for all the fields in 
structures?

Well while in general it could be "a proper way" of documenting sources 
I found it not that convenient especially in case of really long structures.

Consider an example from here 
https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt:
=================
Example kernel-doc data structure comment.

/**
  * struct blah - the basic blah structure
  * @mem1:	describe the first member of struct blah
  * @mem2:	describe the second member of struct blah,
  *		perhaps with more lines and words.
  *
  * Longer description of this structure.
  */
=================

In my case "arc_emac_priv" structure has 21 members, so right before 
structure itself there will be another at least 21 line of comments.

Moreover: "The kernel-doc function comments describe each parameter to 
the function, in order, with the @name lines."

While I don't think that each and every member needs description. At 
least some pairs like Tx/Rx I believe may share the only comment saying 
"Pointers to BD rings - CPU side".

Also I barely can find an example of strict usage of kernel-doc format 
for data structures in drivers nearby.

For example take a look at STMMAC - drivers/net/ethernet/stmicro/stmmac/
Lots of structures defined, non with kernel-doc description.

Still you think the only way to go is to add kernel-doc description then 
I'll add it ASAP, might be it will be a good example for other developers.

>> +++ b/drivers/net/ethernet/arc/arc_emac_main.c
>
>> +static int arc_emac_get_settings(struct net_device *ndev,
>> +                                struct ethtool_cmd *cmd)
>> +{
>> +       struct arc_emac_priv *priv = netdev_priv(ndev);
>> +
>> +       if (priv->phy_dev)
>> +               return phy_ethtool_gset(priv->phy_dev, cmd);
>> +
>> +       return -EINVAL;
>
> May be
> if (!...)
>   return -EINVAL;
> return ...;

Agree, this will be better.

>> +static int arc_emac_set_settings(struct net_device *ndev,
>> +                                struct ethtool_cmd *cmd)
>> +{
>> +       struct arc_emac_priv *priv = netdev_priv(ndev);
>> +
>> +       if (!capable(CAP_NET_ADMIN))
>> +               return -EPERM;
>> +
>> +       if (priv->phy_dev)
>> +               return phy_ethtool_sset(priv->phy_dev, cmd);
>> +
>> +       return -EINVAL;
>
> Ditto.

Agree.

>> +static int arc_emac_poll(struct napi_struct *napi, int budget)
>> +{
>> +       struct net_device *ndev = napi->dev;
>> +       struct arc_emac_priv *priv = netdev_priv(ndev);
>> +       unsigned int i, loop, work_done = 0;
>> +       struct sk_buff *skb;
>
>> +       for (loop = 0; loop < RX_BD_NUM; loop++) {
>> +               struct net_device_stats *stats = &priv->stats;
>> +               struct buffer_state *rx_buff;
>> +               struct arc_emac_bd_t *rxbd;
>> +               dma_addr_t addr;
>> +               unsigned int info, pktlen;
>> +               unsigned int buflen = ndev->mtu + EMAC_BUFFER_PAD;
>> +
>> +               i = (i + 1) % RX_BD_NUM;
>
> I just realize you are using circular buffer here. What about to use
> them with linux' native framework? Documentation/circular-buffers.txt

Exactly as Francois commented already - I didn't find it to be widely 
used in network drivers so I decided to not use it at least for now.

>> +               if (unlikely((info & OWN_MASK) == FOR_EMAC)) {
>> +                       /* BD is still owned by EMAC */
>> +                       continue;
>> +               }
>
> Redundant braces.

The question is then where to put a comment?
Before "if"? But it only applicable to contents of "if", right?
If I put the comment right after "if" then "continue" will be a bit far 
from "if". And this may confuse a reader. That's why to make reading 
easier I put braces in place.

>> +               addr = dma_map_single(&ndev->dev, (void *)rx_buff->skb->data,
>> +                                     buflen, DMA_FROM_DEVICE);
>> +               if (dma_mapping_error(&ndev->dev, addr)) {
>> +                       if (net_ratelimit())
>> +                               if (net_ratelimit())
>
> Is it not a typo?

Definitely not a typo! Copy-paste! I should have found it myself...

>> +                                       netdev_err(ndev, "cannot dma map\n");
>
>> +               work_done++;
>> +               if (work_done >= budget)
>> +                       break;
>
> Those three could easily go to the for () on the top of this function.

Correct. It should be like this on top of the "arc_emac_poll":
====
		if (work_done >= budget)
			break;
		work_done++;
====

>> +static irqreturn_t arc_emac_intr(int irq, void *dev_instance)
>> +{
>
>> +       if (status & TXINT_MASK) {
>> +               unsigned int i;
>> +
>> +               for (i = 0; i < TX_BD_NUM; i++) {
>> +                       unsigned int *txbd_dirty = &priv->txbd_dirty;
>> +                       struct arc_emac_bd_t *txbd = &priv->txbd[*txbd_dirty];
>> +                       struct buffer_state *tx_buff = &priv->tx_buff[*txbd_dirty];
>> +                       struct sk_buff *skb = tx_buff->skb;
>> +                       unsigned int info = txbd->info;
>
>> +                       txbd->data = 0x0;
>> +                       txbd->info = 0x0;
>
> Plain 0?

Right, 0 might be nicer)

>> +                       *txbd_dirty = (*txbd_dirty + 1) % TX_BD_NUM;
>
> Same idea about circular buffers.
>
>> +static int arc_emac_open(struct net_device *ndev)
>> +{
>
>> +       /* Set Poll rate so that it polls every 1 ms */
>> +       arc_reg_set(priv, R_POLLRATE,
>> +                    priv->clock_frequency / 1000000);
>
> I don't understand how you end up with 1ms here. 1000000 is just a
> magic number, clock_frequency generally is an arbitrary value.

Here's an extract from documentation:
==========
The value programmed into this register is number of clocks between 
polls times 1024. A value of 1 will cause a poll every 1024 clocks, 
2=2048 clocks. 0 is not valid. A CPU clock frequency of 100Mhz would
typically want to program the POLLRATE register with a value of 100 
which will cause a poll to occur about once every millisecond (10ns * 
1024 * 100=1ms)
==========

Do you think this info should be put in comment somewhere near?
I'd say it's pretty specific and common reader doesn't need to go in 
such sort of details especially if he doesn't have access to Hw 
documentation.

Otherwise we'll need to add to many technical details in comments, right?

>> +static struct net_device_stats *arc_emac_stats(struct net_device *ndev)
>> +{
>> +       unsigned long miss, rxerr, rxfram, rxcrc, rxoflow;
>> +       struct arc_emac_priv *priv = netdev_priv(ndev);
>> +       struct net_device_stats *stats = &priv->stats;
>> +
>> +       rxerr = arc_reg_get(priv, R_RXERR);
>> +       miss = arc_reg_get(priv, R_MISS);
>> +
>> +       rxcrc = rxerr & 0xff;
>> +       rxfram = rxerr >> 8 & 0xff;
>> +       rxoflow = rxerr >> 16 & 0xff;
>
> If you declare those three as u8, you can avoid those & 0xff.

Makes sense.

>> +       ndev = alloc_etherdev(sizeof(struct arc_emac_priv));
>> +       if (!ndev) {
>> +               err = -ENOMEM;
>> +               goto out;
>> +       }
>
> Redundant goto. Just return here.

Correct.

>> +       }
>> +
>> +
>
> Extra line.

Thanks :)

> --
> With Best Regards,
> Andy Shevchenko
>

Regards,
Alexey

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

* Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver
  2013-06-13 20:25   ` Alexey Brodkin
@ 2013-06-13 20:50     ` Andy Shevchenko
  2013-06-13 21:48       ` Alexey Brodkin
  0 siblings, 1 reply; 21+ messages in thread
From: Andy Shevchenko @ 2013-06-13 20:50 UTC (permalink / raw)
  To: Alexey Brodkin
  Cc: netdev, Francois Romieu, Joe Perches, Vineet Gupta,
	Mischa Jonker, Arnd Bergmann, Grant Likely, Rob Herring,
	Paul Gortmaker, David S. Miller, linux-kernel,
	Devicetree Discuss

On Thu, Jun 13, 2013 at 11:25 PM, Alexey Brodkin
<Alexey.Brodkin@synopsys.com> wrote:
> On 06/13/2013 10:25 PM, Andy Shevchenko wrote:
>> On Thu, Jun 13, 2013 at 5:37 PM, Alexey Brodkin
>> <Alexey.Brodkin@synopsys.com> wrote:
>>> Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
>>> instantiated in some legacy ARC (Synopsys) FPGA Boards such as
>>> ARCAngel4/ML50x.

>>> +++ b/drivers/net/ethernet/arc/arc_emac.h

> And what about function names? Do you think it worth to shorten them too
> since most of them aren't visible outside (static).

It's better to keep them in their namespace. So, leave them as is.

>>> +struct arc_emac_priv {

>>> +};

>> It seems you missed my comments against the names of the members. Can
>> you address them or comment why not?
>
> You mean to add description in kerneldoc format for all the fields in
> structures?

Not only that one. About member names as well.

> Well while in general it could be "a proper way" of documenting sources
> I found it not that convenient especially in case of really long structures.

> In my case "arc_emac_priv" structure has 21 members, so right before
> structure itself there will be another at least 21 line of comments.

Not an argument, you understand.

> Moreover: "The kernel-doc function comments describe each parameter to
> the function, in order, with the @name lines."

> While I don't think that each and every member needs description.

Describe them in couple of words.

> At
> least some pairs like Tx/Rx I believe may share the only comment saying
> "Pointers to BD rings - CPU side".

What BD means? May be it worth to describe as well?

> Also I barely can find an example of strict usage of kernel-doc format
> for data structures in drivers nearby.
>
> For example take a look at STMMAC - drivers/net/ethernet/stmicro/stmmac/
> Lots of structures defined, non with kernel-doc description.

Again, not an excuse :-)

> Still you think the only way to go is to add kernel-doc description then
> I'll add it ASAP, might be it will be a good example for other developers.

Right.

>>> +++ b/drivers/net/ethernet/arc/arc_emac_main.c
>>

>>> +static int arc_emac_poll(struct napi_struct *napi, int budget)
>>> +{

>>> +       unsigned int i, loop, work_done = 0;

>>> +       for (loop = 0; loop < RX_BD_NUM; loop++) {

>>> +               work_done++;
>>> +               if (work_done >= budget)
>>> +                       break;
>>
>> Those three could easily go to the for () on the top of this function.
>
> Correct. It should be like this on top of the "arc_emac_poll":
> ====
>                 if (work_done >= budget)
>                         break;
>                 work_done++;
> ====

I meant something like
for (loop = 0; loop < RX_BD_NUM && work_done < budget; loop++, work_done++)

>>> +static int arc_emac_open(struct net_device *ndev)
>>> +{
>>
>>> +       /* Set Poll rate so that it polls every 1 ms */
>>> +       arc_reg_set(priv, R_POLLRATE,
>>> +                    priv->clock_frequency / 1000000);
>>
>> I don't understand how you end up with 1ms here. 1000000 is just a
>> magic number, clock_frequency generally is an arbitrary value.

I meant how do you guarantee this is 1ms? What if clock_frequency is not 100MHz?

--
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver
  2013-06-13 20:50     ` Andy Shevchenko
@ 2013-06-13 21:48       ` Alexey Brodkin
  0 siblings, 0 replies; 21+ messages in thread
From: Alexey Brodkin @ 2013-06-13 21:48 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: netdev, Francois Romieu, Joe Perches, Vineet Gupta,
	Mischa Jonker, Arnd Bergmann, Grant Likely, Rob Herring,
	Paul Gortmaker, David S. Miller, linux-kernel,
	Devicetree Discuss

On 06/14/2013 12:50 AM, Andy Shevchenko wrote:
> On Thu, Jun 13, 2013 at 11:25 PM, Alexey Brodkin
> <Alexey.Brodkin@synopsys.com> wrote:
>> On 06/13/2013 10:25 PM, Andy Shevchenko wrote:
>>> On Thu, Jun 13, 2013 at 5:37 PM, Alexey Brodkin
>>> <Alexey.Brodkin@synopsys.com> wrote:
>>>> Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
>>>> instantiated in some legacy ARC (Synopsys) FPGA Boards such as
>>>> ARCAngel4/ML50x.
>
>>>> +++ b/drivers/net/ethernet/arc/arc_emac.h
>
>> And what about function names? Do you think it worth to shorten them too
>> since most of them aren't visible outside (static).
>
> It's better to keep them in their namespace. So, leave them as is.

Ok, makes sense.

>>>> +struct arc_emac_priv {
>
>>>> +};
>
>>> It seems you missed my comments against the names of the members. Can
>>> you address them or comment why not?
>>
>> You mean to add description in kerneldoc format for all the fields in
>> structures?
>
> Not only that one. About member names as well.

Ok, I'll re-visit member names as advised.

>> Well while in general it could be "a proper way" of documenting sources
>> I found it not that convenient especially in case of really long structures.
>
>> In my case "arc_emac_priv" structure has 21 members, so right before
>> structure itself there will be another at least 21 line of comments.
>
> Not an argument, you understand.
>
>> Moreover: "The kernel-doc function comments describe each parameter to
>> the function, in order, with the @name lines."
>
>> While I don't think that each and every member needs description.
>
> Describe them in couple of words.
>
>> At
>> least some pairs like Tx/Rx I believe may share the only comment saying
>> "Pointers to BD rings - CPU side".
>
> What BD means? May be it worth to describe as well?
>
>> Also I barely can find an example of strict usage of kernel-doc format
>> for data structures in drivers nearby.
>>
>> For example take a look at STMMAC - drivers/net/ethernet/stmicro/stmmac/
>> Lots of structures defined, non with kernel-doc description.
>
> Again, not an excuse :-)

Ok, will add description)

>> Still you think the only way to go is to add kernel-doc description then
>> I'll add it ASAP, might be it will be a good example for other developers.
>
> Right.
>
>>>> +++ b/drivers/net/ethernet/arc/arc_emac_main.c
>>>
>
>>>> +static int arc_emac_poll(struct napi_struct *napi, int budget)
>>>> +{
>
>>>> +       unsigned int i, loop, work_done = 0;
>
>>>> +       for (loop = 0; loop < RX_BD_NUM; loop++) {
>
>>>> +               work_done++;
>>>> +               if (work_done >= budget)
>>>> +                       break;
>>>
>>> Those three could easily go to the for () on the top of this function.
>>
>> Correct. It should be like this on top of the "arc_emac_poll":
>> ====
>>                  if (work_done >= budget)
>>                          break;
>>                  work_done++;
>> ====
>
> I meant something like
> for (loop = 0; loop < RX_BD_NUM && work_done < budget; loop++, work_done++)

Well, it turned out that counter increment was put in the end of the 
loop on purpose.

But with help of this hint I understood that "arc_emac_poll" was 
implemented a bit incorrectly. Now I added an exit from the loop on the 
first BD owned by EMAC. This is possible because we know that buffers 
are processed in order.

>>>> +static int arc_emac_open(struct net_device *ndev)
>>>> +{
>>>
>>>> +       /* Set Poll rate so that it polls every 1 ms */
>>>> +       arc_reg_set(priv, R_POLLRATE,
>>>> +                    priv->clock_frequency / 1000000);
>>>
>>> I don't understand how you end up with 1ms here. 1000000 is just a
>>> magic number, clock_frequency generally is an arbitrary value.
>
> I meant how do you guarantee this is 1ms? What if clock_frequency is not 100MHz?

That's why I needed to pass CPU frequency to "arc_emac_probe" via DT.
As we see from description in HW documentation poll period measured in 
CPU cycles = "1024 cycles *  R_POLLRATE", or "1024 * 
priv->clock_frequency / 1000000", or "priv->clock_frequency / 1000".
Now since length of 1 cycle is "1 / priv->clock_frequency" we may 
calculate poll period measured in seconds. It will be 
"priv->clock_frequency / 1000 * (1 / priv->clock_frequency)" which is 
1/1000 of second i.e. 1 millisecond. Makes sense?

Regards,
Alexey

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

* Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver
  2013-06-13 14:37 ` Alexey Brodkin
  (?)
  (?)
@ 2013-06-13 22:19 ` Francois Romieu
  2013-06-14 14:14     ` Alexey Brodkin
  2013-09-04 12:14   ` Query about TX BD Reclaim in Napi poll path (was Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver) Vineet Gupta
  -1 siblings, 2 replies; 21+ messages in thread
From: Francois Romieu @ 2013-06-13 22:19 UTC (permalink / raw)
  To: Alexey Brodkin
  Cc: netdev, Andy Shevchenko, Joe Perches, Vineet Gupta,
	Mischa Jonker, Arnd Bergmann, Grant Likely, Rob Herring,
	Paul Gortmaker, David S. Miller, linux-kernel,
	devicetree-discuss

Alexey Brodkin <Alexey.Brodkin@synopsys.com> :
[...]
> diff --git a/drivers/net/ethernet/arc/arc_emac.h b/drivers/net/ethernet/arc/arc_emac.h
> new file mode 100644
> index 0000000..6691db2
> --- /dev/null
> +++ b/drivers/net/ethernet/arc/arc_emac.h
[...]
> +struct arc_emac_priv {
> +	struct net_device_stats stats;
> +	unsigned int clock_frequency;
> +	unsigned int max_speed;
> +
> +	/* Pointers to BD rings - CPU side */
> +	struct arc_emac_bd_t *rxbd;

There does not seem to be much need for rxbd->data.

> +	struct arc_emac_bd_t *txbd;

[...]
> +static int arc_emac_get_settings(struct net_device *ndev,
> +				 struct ethtool_cmd *cmd)
> +{
> +	struct arc_emac_priv *priv = netdev_priv(ndev);
> +
> +	if (priv->phy_dev)
> +		return phy_ethtool_gset(priv->phy_dev, cmd);

arc_emac_probe() succeeded : priv->phy_dev can't be NULL.

[...]
> +static int arc_emac_set_settings(struct net_device *ndev,
> +				 struct ethtool_cmd *cmd)
> +{
> +	struct arc_emac_priv *priv = netdev_priv(ndev);
> +
> +	if (!capable(CAP_NET_ADMIN))
> +		return -EPERM;
> +
> +	if (priv->phy_dev)
> +		return phy_ethtool_sset(priv->phy_dev, cmd);

priv->phy_dev can't be NULL either.

[...]
> +static int arc_emac_poll(struct napi_struct *napi, int budget)
> +{
> +	struct net_device *ndev = napi->dev;
> +	struct arc_emac_priv *priv = netdev_priv(ndev);
> +	unsigned int i, loop, work_done = 0;
> +	struct sk_buff *skb;
> +
> +	/* Loop thru the BD chain, but not always from 0.
> +	 * Start from right after where we last saw a packet.
> +	 */
> +	i = priv->last_rx_bd;
> +
> +	for (loop = 0; loop < RX_BD_NUM; loop++) {
> +		struct net_device_stats *stats = &priv->stats;
> +		struct buffer_state *rx_buff;
> +		struct arc_emac_bd_t *rxbd;
> +		dma_addr_t addr;
> +		unsigned int info, pktlen;
> +		unsigned int buflen = ndev->mtu + EMAC_BUFFER_PAD;
> +
> +		i = (i + 1) % RX_BD_NUM;
> +		rx_buff = &priv->rx_buff[i];
> +		rxbd = &priv->rxbd[i];
> +		info = rxbd->info;

You may / should update 'i' so that you can:

		struct buffer_state *rx_buff = priv->rx_buff + i;
		struct arc_emac_bd_t *rxbd = priv->rxbd + i;
		unsigned int pktlen, info = rxbd->info;

> +
> +		/* Process current BD */
> +
> +		if (unlikely((info & OWN_MASK) == FOR_EMAC)) {
> +			/* BD is still owned by EMAC */
> +			continue;
> +		}

Processing starts at priv->last_rx_bd. Could it not be a 'break' ?

[...]
> +		pktlen = info & LEN_MASK;
> +		stats->rx_packets++;
> +		stats->rx_bytes += pktlen;
> +		skb = rx_buff->skb;
> +		skb_put(skb, pktlen);
> +		skb->dev = ndev;
> +		skb->protocol = eth_type_trans(skb, ndev);
> +		netif_receive_skb(skb);
> +
> +		dma_unmap_single(&ndev->dev,
> +				 dma_unmap_addr(&rx_buff, addr),
> +				 dma_unmap_len(&rx_buff, len),
> +				 DMA_FROM_DEVICE);

		dma_unmap_single(&ndev->dev, dma_unmap_addr(&rx_buff, addr),
				 dma_unmap_len(&rx_buff, len), DMA_FROM_DEVICE);

Please unmap before netif_receive_skb.

> +
> +		/* Prepare the BD for next cycle */
> +		rx_buff->skb = netdev_alloc_skb_ip_align(ndev, buflen);
> +		if (unlikely(!rx_buff->skb)) {
> +			if (net_ratelimit())
> +				netdev_err(ndev, "cannot allocate skb\n");
> +			stats->rx_errors++;
> +			continue;
> +		}

The descriptor entry is left unchanged. Afaiu the driver will move to the
next descriptor and crash on dereferencing NULL (rx_buff->)skb next time
it wraps.

I suggest avoiding holes: don't netif_receive_skb if you can't alloc a new
skb.

[...]
> +static irqreturn_t arc_emac_intr(int irq, void *dev_instance)
> +{
> +	struct net_device *ndev = dev_instance;
> +	struct arc_emac_priv *priv = netdev_priv(ndev);
> +	struct net_device_stats *stats = &priv->stats;
> +	unsigned int status;
> +
> +	status = arc_reg_get(priv, R_STATUS);
> +	status &= ~MDIO_MASK;
> +
> +	/* Reset all flags except "MDIO complete"*/
> +	arc_reg_set(priv, R_STATUS, status);
> +
> +	if (status & RXINT_MASK) {
> +		if (likely(napi_schedule_prep(&priv->napi))) {
> +			arc_reg_clr(priv, R_ENABLE, RXINT_MASK);
> +			__napi_schedule(&priv->napi);
> +		}
> +	}
> +
> +	if (status & TXINT_MASK) {

You may consider moving everything into the napi poll handler.

[...]
> +static int arc_emac_open(struct net_device *ndev)
> +{
> +	struct arc_emac_priv *priv = netdev_priv(ndev);
> +	struct phy_device *phy_dev = priv->phy_dev;
> +	struct arc_emac_bd_t *bd;
> +	struct sk_buff *skb;
> +	int i;
> +
> +	phy_dev->autoneg = AUTONEG_ENABLE;
> +	phy_dev->speed = 0;
> +	phy_dev->duplex = 0;
> +	phy_dev->advertising = phy_dev->supported;
> +
> +	if (priv->max_speed > 100) {
> +		phy_dev->advertising &= PHY_GBIT_FEATURES;
> +	} else if (priv->max_speed <= 100) {
> +		phy_dev->advertising &= PHY_BASIC_FEATURES;
> +		if (priv->max_speed <= 10) {
> +			phy_dev->advertising &= ~SUPPORTED_100baseT_Half;
> +			phy_dev->advertising &= ~SUPPORTED_100baseT_Full;
> +		}
> +	}
> +
> +	/* Allocate and set buffers for Rx BD's */
> +	bd = priv->rxbd;
> +	for (i = 0; i < RX_BD_NUM; i++) {
> +		skb = netdev_alloc_skb_ip_align(ndev, ndev->mtu +
> +						EMAC_BUFFER_PAD);
> +

Missing NULL check.

[...]
> +	/* Set BD ring pointers for device side */
> +	arc_reg_set(priv, R_RX_RING,
> +		     (unsigned int)priv->rxbd_dma_hdl);

	arc_reg_set(priv, R_RX_RING, (unsigned int)priv->rxbd_dma_hdl);

> +
> +	arc_reg_set(priv, R_TX_RING,
> +		     (unsigned int)priv->rxbd_dma_hdl + RX_RING_SZ);

Forgot priv->txbd_dma_hdl ?

> +
> +	/* Set Poll rate so that it polls every 1 ms */
> +	arc_reg_set(priv, R_POLLRATE,
> +		     priv->clock_frequency / 1000000);

	arc_reg_set(priv, R_POLLRATE, priv->clock_frequency / 1000000);

> +
> +	/* Enable interrupts */
> +	arc_reg_set(priv, R_ENABLE,
> +		     TXINT_MASK | RXINT_MASK | ERR_MASK);

	arc_reg_set(priv, R_ENABLE, TXINT_MASK | RXINT_MASK | ERR_MASK);

> +
> +	/* Set CONTROL */
> +	arc_reg_set(priv, R_CTRL,
> +		     (RX_BD_NUM << 24) |	/* RX BD table length */
> +		     (TX_BD_NUM << 16) |	/* TX BD table length */
> +		     TXRN_MASK | RXRN_MASK);
> +
> +	/* Enable EMAC */
> +	arc_reg_or(priv, R_CTRL, EN_MASK);
> +
> +	phy_start_aneg(priv->phy_dev);
> +
> +	netif_start_queue(ndev);
> +	napi_enable(&priv->napi);

napi must be enabled before the first rx packet / irq kicks in.

[...]
> +static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev)
> +{
> +	struct arc_emac_priv *priv = netdev_priv(ndev);
> +	unsigned int info, len, *txbd_curr = &priv->txbd_curr;
> +	dma_addr_t addr;
> +	char *pkt = skb->data;
> +
> +	len = max_t(unsigned int, ETH_ZLEN, skb->len);

The device automatically pads, right ?

> +	info = priv->txbd[*txbd_curr].info;
> +
> +	/* EMAC still holds this buffer in its possession.
> +	 * CPU must not modify this buffer descriptor
> +	 */
> +	if (unlikely((info & OWN_MASK) == FOR_EMAC)) {
> +		netif_stop_queue(ndev);
> +		return NETDEV_TX_BUSY;

It is more than unlikely: you may check for it as a bug but you should
stop_queue when the tx ring is full (before returning below).

> +	}
> +
> +	addr = dma_map_single(&ndev->dev, (void *)pkt, len, DMA_TO_DEVICE);
> +	if (unlikely(dma_mapping_error(&ndev->dev, addr))) {
> +		dev_kfree_skb(skb);
> +		return NETDEV_TX_OK;

Please update tx_dropped stats.

> +	}
> +	dma_unmap_addr_set(&priv->tx_buff[*txbd_curr], mapping, addr);
> +	dma_unmap_len_set(&priv->tx_buff[*txbd_curr], len, len);
> +
> +	priv->tx_buff[*txbd_curr].skb = skb;
> +	priv->txbd[*txbd_curr].data = pkt;

Insert memory barrier.

> +	priv->txbd[*txbd_curr].info = FOR_EMAC | FRST_MASK | LAST_MASK | len;
> +
> +	*txbd_curr = (*txbd_curr + 1) % TX_BD_NUM;
> +
> +	arc_reg_set(priv, R_STATUS, TXPL_MASK);
> +
> +	skb_tx_timestamp(skb);
> +
> +	return NETDEV_TX_OK;
> +}

[...]
> +static int arc_emac_probe(struct platform_device *pdev)
> +{
[...]
> +	err = arc_mdio_probe(pdev->dev.of_node, priv);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to probe MII bus\n");
> +		goto out;
> +	}
> +
> +
> +	priv->phy_dev = of_phy_connect(ndev, priv->phy_node,
> +				       arc_emac_adjust_link, 0,
> +				       PHY_INTERFACE_MODE_MII);
> +	if (!priv->phy_dev) {
> +		netdev_err(ndev, "of_phy_connect() failed\n");
> +		return -ENODEV;

arc_mdio_remove leak.

-- 
Ueimor

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

* Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver
  2013-06-13 22:19 ` Francois Romieu
@ 2013-06-14 14:14     ` Alexey Brodkin
  2013-09-04 12:14   ` Query about TX BD Reclaim in Napi poll path (was Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver) Vineet Gupta
  1 sibling, 0 replies; 21+ messages in thread
From: Alexey Brodkin @ 2013-06-14 14:14 UTC (permalink / raw)
  To: Francois Romieu
  Cc: Alexey Brodkin, netdev, Andy Shevchenko, Joe Perches,
	Vineet Gupta, Mischa Jonker, Arnd Bergmann, Grant Likely,
	Rob Herring, Paul Gortmaker, David S. Miller, linux-kernel,
	devicetree-discuss

On 06/14/2013 02:20 AM, Francois Romieu wrote:
> Alexey Brodkin <Alexey.Brodkin@synopsys.com> :
> [...]
>> diff --git a/drivers/net/ethernet/arc/arc_emac.h b/drivers/net/ethernet/arc/arc_emac.h
>> new file mode 100644
>> index 0000000..6691db2
>> --- /dev/null
>> +++ b/drivers/net/ethernet/arc/arc_emac.h
> [...]
>> +struct arc_emac_priv {
>> +	struct net_device_stats stats;
>> +	unsigned int clock_frequency;
>> +	unsigned int max_speed;
>> +
>> +	/* Pointers to BD rings - CPU side */
>> +	struct arc_emac_bd_t *rxbd;
>
> There does not seem to be much need for rxbd->data.

Could you please clarify this comment? Not clear what do you mean.

>> +	struct arc_emac_bd_t *txbd;
>
> [...]
>> +static int arc_emac_get_settings(struct net_device *ndev,
>> +				 struct ethtool_cmd *cmd)
>> +{
>> +	struct arc_emac_priv *priv = netdev_priv(ndev);
>> +
>> +	if (priv->phy_dev)
>> +		return phy_ethtool_gset(priv->phy_dev, cmd);
>
> arc_emac_probe() succeeded : priv->phy_dev can't be NULL.
>
> [...]
>> +static int arc_emac_set_settings(struct net_device *ndev,
>> +				 struct ethtool_cmd *cmd)
>> +{
>> +	struct arc_emac_priv *priv = netdev_priv(ndev);
>> +
>> +	if (!capable(CAP_NET_ADMIN))
>> +		return -EPERM;
>> +
>> +	if (priv->phy_dev)
>> +		return phy_ethtool_sset(priv->phy_dev, cmd);
>
> priv->phy_dev can't be NULL either.

Nice catch! Fortunately I recognized it as well and already removed 
those useless checks.

> [...]
>> +static int arc_emac_poll(struct napi_struct *napi, int budget)
>> +{
>> +	struct net_device *ndev = napi->dev;
>> +	struct arc_emac_priv *priv = netdev_priv(ndev);
>> +	unsigned int i, loop, work_done = 0;
>> +	struct sk_buff *skb;
>> +
>> +	/* Loop thru the BD chain, but not always from 0.
>> +	 * Start from right after where we last saw a packet.
>> +	 */
>> +	i = priv->last_rx_bd;
>> +
>> +	for (loop = 0; loop < RX_BD_NUM; loop++) {
>> +		struct net_device_stats *stats = &priv->stats;
>> +		struct buffer_state *rx_buff;
>> +		struct arc_emac_bd_t *rxbd;
>> +		dma_addr_t addr;
>> +		unsigned int info, pktlen;
>> +		unsigned int buflen = ndev->mtu + EMAC_BUFFER_PAD;
>> +
>> +		i = (i + 1) % RX_BD_NUM;
>> +		rx_buff = &priv->rx_buff[i];
>> +		rxbd = &priv->rxbd[i];
>> +		info = rxbd->info;
>
> You may / should update 'i' so that you can:
>
> 		struct buffer_state *rx_buff = priv->rx_buff + i;
> 		struct arc_emac_bd_t *rxbd = priv->rxbd + i;
> 		unsigned int pktlen, info = rxbd->info;

Done)

>> +
>> +		/* Process current BD */
>> +
>> +		if (unlikely((info & OWN_MASK) == FOR_EMAC)) {
>> +			/* BD is still owned by EMAC */
>> +			continue;
>> +		}
>
> Processing starts at priv->last_rx_bd. Could it not be a 'break' ?

It has to be! Fixed.

> [...]
>> +		pktlen = info & LEN_MASK;
>> +		stats->rx_packets++;
>> +		stats->rx_bytes += pktlen;
>> +		skb = rx_buff->skb;
>> +		skb_put(skb, pktlen);
>> +		skb->dev = ndev;
>> +		skb->protocol = eth_type_trans(skb, ndev);
>> +		netif_receive_skb(skb);
>> +
>> +		dma_unmap_single(&ndev->dev,
>> +				 dma_unmap_addr(&rx_buff, addr),
>> +				 dma_unmap_len(&rx_buff, len),
>> +				 DMA_FROM_DEVICE);
>
> 		dma_unmap_single(&ndev->dev, dma_unmap_addr(&rx_buff, addr),
> 				 dma_unmap_len(&rx_buff, len), DMA_FROM_DEVICE);
>
> Please unmap before netif_receive_skb.

Done. Thanks.

>> +
>> +		/* Prepare the BD for next cycle */
>> +		rx_buff->skb = netdev_alloc_skb_ip_align(ndev, buflen);
>> +		if (unlikely(!rx_buff->skb)) {
>> +			if (net_ratelimit())
>> +				netdev_err(ndev, "cannot allocate skb\n");
>> +			stats->rx_errors++;
>> +			continue;
>> +		}
>
> The descriptor entry is left unchanged. Afaiu the driver will move to the
> next descriptor and crash on dereferencing NULL (rx_buff->)skb next time
> it wraps.
>
> I suggest avoiding holes: don't netif_receive_skb if you can't alloc a new
> skb.

Frankly I cannot understand how "don't netif_receive_skb" for one of the 
received packets helps to prevent crash on the next iteration?
And I don't see a way to return any error state from NAPI poll handler.
Could you please clarify your idea?

> [...]
>> +static irqreturn_t arc_emac_intr(int irq, void *dev_instance)
>> +{
>> +	struct net_device *ndev = dev_instance;
>> +	struct arc_emac_priv *priv = netdev_priv(ndev);
>> +	struct net_device_stats *stats = &priv->stats;
>> +	unsigned int status;
>> +
>> +	status = arc_reg_get(priv, R_STATUS);
>> +	status &= ~MDIO_MASK;
>> +
>> +	/* Reset all flags except "MDIO complete"*/
>> +	arc_reg_set(priv, R_STATUS, status);
>> +
>> +	if (status & RXINT_MASK) {
>> +		if (likely(napi_schedule_prep(&priv->napi))) {
>> +			arc_reg_clr(priv, R_ENABLE, RXINT_MASK);
>> +			__napi_schedule(&priv->napi);
>> +		}
>> +	}
>> +
>> +	if (status & TXINT_MASK) {
>
> You may consider moving everything into the napi poll handler.

Agree. Would be good - implementing.

> [...]
>> +static int arc_emac_open(struct net_device *ndev)
>> +{
>> +	struct arc_emac_priv *priv = netdev_priv(ndev);
>> +	struct phy_device *phy_dev = priv->phy_dev;
>> +	struct arc_emac_bd_t *bd;
>> +	struct sk_buff *skb;
>> +	int i;
>> +
>> +	phy_dev->autoneg = AUTONEG_ENABLE;
>> +	phy_dev->speed = 0;
>> +	phy_dev->duplex = 0;
>> +	phy_dev->advertising = phy_dev->supported;
>> +
>> +	if (priv->max_speed > 100) {
>> +		phy_dev->advertising &= PHY_GBIT_FEATURES;
>> +	} else if (priv->max_speed <= 100) {
>> +		phy_dev->advertising &= PHY_BASIC_FEATURES;
>> +		if (priv->max_speed <= 10) {
>> +			phy_dev->advertising &= ~SUPPORTED_100baseT_Half;
>> +			phy_dev->advertising &= ~SUPPORTED_100baseT_Full;
>> +		}
>> +	}
>> +
>> +	/* Allocate and set buffers for Rx BD's */
>> +	bd = priv->rxbd;
>> +	for (i = 0; i < RX_BD_NUM; i++) {
>> +		skb = netdev_alloc_skb_ip_align(ndev, ndev->mtu +
>> +						EMAC_BUFFER_PAD);
>> +
>
> Missing NULL check.

Thanks!

> [...]
>> +	/* Set BD ring pointers for device side */
>> +	arc_reg_set(priv, R_RX_RING,
>> +		     (unsigned int)priv->rxbd_dma_hdl);
>
> 	arc_reg_set(priv, R_RX_RING, (unsigned int)priv->rxbd_dma_hdl);
>
>> +
>> +	arc_reg_set(priv, R_TX_RING,
>> +		     (unsigned int)priv->rxbd_dma_hdl + RX_RING_SZ);
>
> Forgot priv->txbd_dma_hdl ?

Well, it was used implicitly, because priv->rxbd_dma_hdl + RX_RING_SZ = 
priv->txbd_dma_hdl. Replaced with explicit priv->txbd_dma_hdl.

>> +
>> +	/* Set Poll rate so that it polls every 1 ms */
>> +	arc_reg_set(priv, R_POLLRATE,
>> +		     priv->clock_frequency / 1000000);
>
> 	arc_reg_set(priv, R_POLLRATE, priv->clock_frequency / 1000000);
>
>> +
>> +	/* Enable interrupts */
>> +	arc_reg_set(priv, R_ENABLE,
>> +		     TXINT_MASK | RXINT_MASK | ERR_MASK);
>
> 	arc_reg_set(priv, R_ENABLE, TXINT_MASK | RXINT_MASK | ERR_MASK);

I see, thanks.

>> +
>> +	/* Set CONTROL */
>> +	arc_reg_set(priv, R_CTRL,
>> +		     (RX_BD_NUM << 24) |	/* RX BD table length */
>> +		     (TX_BD_NUM << 16) |	/* TX BD table length */
>> +		     TXRN_MASK | RXRN_MASK);
>> +
>> +	/* Enable EMAC */
>> +	arc_reg_or(priv, R_CTRL, EN_MASK);
>> +
>> +	phy_start_aneg(priv->phy_dev);
>> +
>> +	netif_start_queue(ndev);
>> +	napi_enable(&priv->napi);
>
> napi must be enabled before the first rx packet / irq kicks in.

Thanks.

> [...]
>> +static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev)
>> +{
>> +	struct arc_emac_priv *priv = netdev_priv(ndev);
>> +	unsigned int info, len, *txbd_curr = &priv->txbd_curr;
>> +	dma_addr_t addr;
>> +	char *pkt = skb->data;
>> +
>> +	len = max_t(unsigned int, ETH_ZLEN, skb->len);
>
> The device automatically pads, right ?

What do you mean here?

>> +	info = priv->txbd[*txbd_curr].info;
>> +
>> +	/* EMAC still holds this buffer in its possession.
>> +	 * CPU must not modify this buffer descriptor
>> +	 */
>> +	if (unlikely((info & OWN_MASK) == FOR_EMAC)) {
>> +		netif_stop_queue(ndev);
>> +		return NETDEV_TX_BUSY;
>
> It is more than unlikely: you may check for it as a bug but you should
> stop_queue when the tx ring is full (before returning below).

So I left this check, but add another one near bottom of Tx function to 
prevent mentioned situation.

>> +	}
>> +
>> +	addr = dma_map_single(&ndev->dev, (void *)pkt, len, DMA_TO_DEVICE);
>> +	if (unlikely(dma_mapping_error(&ndev->dev, addr))) {
>> +		dev_kfree_skb(skb);
>> +		return NETDEV_TX_OK;
>
> Please update tx_dropped stats.

Done.

>> +	}
>> +	dma_unmap_addr_set(&priv->tx_buff[*txbd_curr], mapping, addr);
>> +	dma_unmap_len_set(&priv->tx_buff[*txbd_curr], len, len);
>> +
>> +	priv->tx_buff[*txbd_curr].skb = skb;
>> +	priv->txbd[*txbd_curr].data = pkt;
>
> Insert memory barrier.

Very good and useful point, done with verbose comment.

>> +	priv->txbd[*txbd_curr].info = FOR_EMAC | FRST_MASK | LAST_MASK | len;
>> +
>> +	*txbd_curr = (*txbd_curr + 1) % TX_BD_NUM;
>> +
>> +	arc_reg_set(priv, R_STATUS, TXPL_MASK);
>> +
>> +	skb_tx_timestamp(skb);
>> +
>> +	return NETDEV_TX_OK;
>> +}
>
> [...]
>> +static int arc_emac_probe(struct platform_device *pdev)
>> +{
> [...]
>> +	err = arc_mdio_probe(pdev->dev.of_node, priv);
>> +	if (err) {
>> +		dev_err(&pdev->dev, "failed to probe MII bus\n");
>> +		goto out;
>> +	}
>> +
>> +
>> +	priv->phy_dev = of_phy_connect(ndev, priv->phy_node,
>> +				       arc_emac_adjust_link, 0,
>> +				       PHY_INTERFACE_MODE_MII);
>> +	if (!priv->phy_dev) {
>> +		netdev_err(ndev, "of_phy_connect() failed\n");
>> +		return -ENODEV;
>
> arc_mdio_remove leak.

Indeed) Fixed.

-Alexey



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

* Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver
@ 2013-06-14 14:14     ` Alexey Brodkin
  0 siblings, 0 replies; 21+ messages in thread
From: Alexey Brodkin @ 2013-06-14 14:14 UTC (permalink / raw)
  To: Francois Romieu
  Cc: Alexey Brodkin, netdev, Andy Shevchenko, Joe Perches,
	Vineet Gupta, Mischa Jonker, Arnd Bergmann, Grant Likely,
	Rob Herring, Paul Gortmaker, David S. Miller, linux-kernel,
	devicetree-discuss

On 06/14/2013 02:20 AM, Francois Romieu wrote:
> Alexey Brodkin <Alexey.Brodkin@synopsys.com> :
> [...]
>> diff --git a/drivers/net/ethernet/arc/arc_emac.h b/drivers/net/ethernet/arc/arc_emac.h
>> new file mode 100644
>> index 0000000..6691db2
>> --- /dev/null
>> +++ b/drivers/net/ethernet/arc/arc_emac.h
> [...]
>> +struct arc_emac_priv {
>> +	struct net_device_stats stats;
>> +	unsigned int clock_frequency;
>> +	unsigned int max_speed;
>> +
>> +	/* Pointers to BD rings - CPU side */
>> +	struct arc_emac_bd_t *rxbd;
>
> There does not seem to be much need for rxbd->data.

Could you please clarify this comment? Not clear what do you mean.

>> +	struct arc_emac_bd_t *txbd;
>
> [...]
>> +static int arc_emac_get_settings(struct net_device *ndev,
>> +				 struct ethtool_cmd *cmd)
>> +{
>> +	struct arc_emac_priv *priv = netdev_priv(ndev);
>> +
>> +	if (priv->phy_dev)
>> +		return phy_ethtool_gset(priv->phy_dev, cmd);
>
> arc_emac_probe() succeeded : priv->phy_dev can't be NULL.
>
> [...]
>> +static int arc_emac_set_settings(struct net_device *ndev,
>> +				 struct ethtool_cmd *cmd)
>> +{
>> +	struct arc_emac_priv *priv = netdev_priv(ndev);
>> +
>> +	if (!capable(CAP_NET_ADMIN))
>> +		return -EPERM;
>> +
>> +	if (priv->phy_dev)
>> +		return phy_ethtool_sset(priv->phy_dev, cmd);
>
> priv->phy_dev can't be NULL either.

Nice catch! Fortunately I recognized it as well and already removed 
those useless checks.

> [...]
>> +static int arc_emac_poll(struct napi_struct *napi, int budget)
>> +{
>> +	struct net_device *ndev = napi->dev;
>> +	struct arc_emac_priv *priv = netdev_priv(ndev);
>> +	unsigned int i, loop, work_done = 0;
>> +	struct sk_buff *skb;
>> +
>> +	/* Loop thru the BD chain, but not always from 0.
>> +	 * Start from right after where we last saw a packet.
>> +	 */
>> +	i = priv->last_rx_bd;
>> +
>> +	for (loop = 0; loop < RX_BD_NUM; loop++) {
>> +		struct net_device_stats *stats = &priv->stats;
>> +		struct buffer_state *rx_buff;
>> +		struct arc_emac_bd_t *rxbd;
>> +		dma_addr_t addr;
>> +		unsigned int info, pktlen;
>> +		unsigned int buflen = ndev->mtu + EMAC_BUFFER_PAD;
>> +
>> +		i = (i + 1) % RX_BD_NUM;
>> +		rx_buff = &priv->rx_buff[i];
>> +		rxbd = &priv->rxbd[i];
>> +		info = rxbd->info;
>
> You may / should update 'i' so that you can:
>
> 		struct buffer_state *rx_buff = priv->rx_buff + i;
> 		struct arc_emac_bd_t *rxbd = priv->rxbd + i;
> 		unsigned int pktlen, info = rxbd->info;

Done)

>> +
>> +		/* Process current BD */
>> +
>> +		if (unlikely((info & OWN_MASK) == FOR_EMAC)) {
>> +			/* BD is still owned by EMAC */
>> +			continue;
>> +		}
>
> Processing starts at priv->last_rx_bd. Could it not be a 'break' ?

It has to be! Fixed.

> [...]
>> +		pktlen = info & LEN_MASK;
>> +		stats->rx_packets++;
>> +		stats->rx_bytes += pktlen;
>> +		skb = rx_buff->skb;
>> +		skb_put(skb, pktlen);
>> +		skb->dev = ndev;
>> +		skb->protocol = eth_type_trans(skb, ndev);
>> +		netif_receive_skb(skb);
>> +
>> +		dma_unmap_single(&ndev->dev,
>> +				 dma_unmap_addr(&rx_buff, addr),
>> +				 dma_unmap_len(&rx_buff, len),
>> +				 DMA_FROM_DEVICE);
>
> 		dma_unmap_single(&ndev->dev, dma_unmap_addr(&rx_buff, addr),
> 				 dma_unmap_len(&rx_buff, len), DMA_FROM_DEVICE);
>
> Please unmap before netif_receive_skb.

Done. Thanks.

>> +
>> +		/* Prepare the BD for next cycle */
>> +		rx_buff->skb = netdev_alloc_skb_ip_align(ndev, buflen);
>> +		if (unlikely(!rx_buff->skb)) {
>> +			if (net_ratelimit())
>> +				netdev_err(ndev, "cannot allocate skb\n");
>> +			stats->rx_errors++;
>> +			continue;
>> +		}
>
> The descriptor entry is left unchanged. Afaiu the driver will move to the
> next descriptor and crash on dereferencing NULL (rx_buff->)skb next time
> it wraps.
>
> I suggest avoiding holes: don't netif_receive_skb if you can't alloc a new
> skb.

Frankly I cannot understand how "don't netif_receive_skb" for one of the 
received packets helps to prevent crash on the next iteration?
And I don't see a way to return any error state from NAPI poll handler.
Could you please clarify your idea?

> [...]
>> +static irqreturn_t arc_emac_intr(int irq, void *dev_instance)
>> +{
>> +	struct net_device *ndev = dev_instance;
>> +	struct arc_emac_priv *priv = netdev_priv(ndev);
>> +	struct net_device_stats *stats = &priv->stats;
>> +	unsigned int status;
>> +
>> +	status = arc_reg_get(priv, R_STATUS);
>> +	status &= ~MDIO_MASK;
>> +
>> +	/* Reset all flags except "MDIO complete"*/
>> +	arc_reg_set(priv, R_STATUS, status);
>> +
>> +	if (status & RXINT_MASK) {
>> +		if (likely(napi_schedule_prep(&priv->napi))) {
>> +			arc_reg_clr(priv, R_ENABLE, RXINT_MASK);
>> +			__napi_schedule(&priv->napi);
>> +		}
>> +	}
>> +
>> +	if (status & TXINT_MASK) {
>
> You may consider moving everything into the napi poll handler.

Agree. Would be good - implementing.

> [...]
>> +static int arc_emac_open(struct net_device *ndev)
>> +{
>> +	struct arc_emac_priv *priv = netdev_priv(ndev);
>> +	struct phy_device *phy_dev = priv->phy_dev;
>> +	struct arc_emac_bd_t *bd;
>> +	struct sk_buff *skb;
>> +	int i;
>> +
>> +	phy_dev->autoneg = AUTONEG_ENABLE;
>> +	phy_dev->speed = 0;
>> +	phy_dev->duplex = 0;
>> +	phy_dev->advertising = phy_dev->supported;
>> +
>> +	if (priv->max_speed > 100) {
>> +		phy_dev->advertising &= PHY_GBIT_FEATURES;
>> +	} else if (priv->max_speed <= 100) {
>> +		phy_dev->advertising &= PHY_BASIC_FEATURES;
>> +		if (priv->max_speed <= 10) {
>> +			phy_dev->advertising &= ~SUPPORTED_100baseT_Half;
>> +			phy_dev->advertising &= ~SUPPORTED_100baseT_Full;
>> +		}
>> +	}
>> +
>> +	/* Allocate and set buffers for Rx BD's */
>> +	bd = priv->rxbd;
>> +	for (i = 0; i < RX_BD_NUM; i++) {
>> +		skb = netdev_alloc_skb_ip_align(ndev, ndev->mtu +
>> +						EMAC_BUFFER_PAD);
>> +
>
> Missing NULL check.

Thanks!

> [...]
>> +	/* Set BD ring pointers for device side */
>> +	arc_reg_set(priv, R_RX_RING,
>> +		     (unsigned int)priv->rxbd_dma_hdl);
>
> 	arc_reg_set(priv, R_RX_RING, (unsigned int)priv->rxbd_dma_hdl);
>
>> +
>> +	arc_reg_set(priv, R_TX_RING,
>> +		     (unsigned int)priv->rxbd_dma_hdl + RX_RING_SZ);
>
> Forgot priv->txbd_dma_hdl ?

Well, it was used implicitly, because priv->rxbd_dma_hdl + RX_RING_SZ = 
priv->txbd_dma_hdl. Replaced with explicit priv->txbd_dma_hdl.

>> +
>> +	/* Set Poll rate so that it polls every 1 ms */
>> +	arc_reg_set(priv, R_POLLRATE,
>> +		     priv->clock_frequency / 1000000);
>
> 	arc_reg_set(priv, R_POLLRATE, priv->clock_frequency / 1000000);
>
>> +
>> +	/* Enable interrupts */
>> +	arc_reg_set(priv, R_ENABLE,
>> +		     TXINT_MASK | RXINT_MASK | ERR_MASK);
>
> 	arc_reg_set(priv, R_ENABLE, TXINT_MASK | RXINT_MASK | ERR_MASK);

I see, thanks.

>> +
>> +	/* Set CONTROL */
>> +	arc_reg_set(priv, R_CTRL,
>> +		     (RX_BD_NUM << 24) |	/* RX BD table length */
>> +		     (TX_BD_NUM << 16) |	/* TX BD table length */
>> +		     TXRN_MASK | RXRN_MASK);
>> +
>> +	/* Enable EMAC */
>> +	arc_reg_or(priv, R_CTRL, EN_MASK);
>> +
>> +	phy_start_aneg(priv->phy_dev);
>> +
>> +	netif_start_queue(ndev);
>> +	napi_enable(&priv->napi);
>
> napi must be enabled before the first rx packet / irq kicks in.

Thanks.

> [...]
>> +static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev)
>> +{
>> +	struct arc_emac_priv *priv = netdev_priv(ndev);
>> +	unsigned int info, len, *txbd_curr = &priv->txbd_curr;
>> +	dma_addr_t addr;
>> +	char *pkt = skb->data;
>> +
>> +	len = max_t(unsigned int, ETH_ZLEN, skb->len);
>
> The device automatically pads, right ?

What do you mean here?

>> +	info = priv->txbd[*txbd_curr].info;
>> +
>> +	/* EMAC still holds this buffer in its possession.
>> +	 * CPU must not modify this buffer descriptor
>> +	 */
>> +	if (unlikely((info & OWN_MASK) == FOR_EMAC)) {
>> +		netif_stop_queue(ndev);
>> +		return NETDEV_TX_BUSY;
>
> It is more than unlikely: you may check for it as a bug but you should
> stop_queue when the tx ring is full (before returning below).

So I left this check, but add another one near bottom of Tx function to 
prevent mentioned situation.

>> +	}
>> +
>> +	addr = dma_map_single(&ndev->dev, (void *)pkt, len, DMA_TO_DEVICE);
>> +	if (unlikely(dma_mapping_error(&ndev->dev, addr))) {
>> +		dev_kfree_skb(skb);
>> +		return NETDEV_TX_OK;
>
> Please update tx_dropped stats.

Done.

>> +	}
>> +	dma_unmap_addr_set(&priv->tx_buff[*txbd_curr], mapping, addr);
>> +	dma_unmap_len_set(&priv->tx_buff[*txbd_curr], len, len);
>> +
>> +	priv->tx_buff[*txbd_curr].skb = skb;
>> +	priv->txbd[*txbd_curr].data = pkt;
>
> Insert memory barrier.

Very good and useful point, done with verbose comment.

>> +	priv->txbd[*txbd_curr].info = FOR_EMAC | FRST_MASK | LAST_MASK | len;
>> +
>> +	*txbd_curr = (*txbd_curr + 1) % TX_BD_NUM;
>> +
>> +	arc_reg_set(priv, R_STATUS, TXPL_MASK);
>> +
>> +	skb_tx_timestamp(skb);
>> +
>> +	return NETDEV_TX_OK;
>> +}
>
> [...]
>> +static int arc_emac_probe(struct platform_device *pdev)
>> +{
> [...]
>> +	err = arc_mdio_probe(pdev->dev.of_node, priv);
>> +	if (err) {
>> +		dev_err(&pdev->dev, "failed to probe MII bus\n");
>> +		goto out;
>> +	}
>> +
>> +
>> +	priv->phy_dev = of_phy_connect(ndev, priv->phy_node,
>> +				       arc_emac_adjust_link, 0,
>> +				       PHY_INTERFACE_MODE_MII);
>> +	if (!priv->phy_dev) {
>> +		netdev_err(ndev, "of_phy_connect() failed\n");
>> +		return -ENODEV;
>
> arc_mdio_remove leak.

Indeed) Fixed.

-Alexey

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

* Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver
  2013-06-14 14:14     ` Alexey Brodkin
@ 2013-06-14 19:27       ` Francois Romieu
  -1 siblings, 0 replies; 21+ messages in thread
From: Francois Romieu @ 2013-06-14 19:27 UTC (permalink / raw)
  To: Alexey Brodkin
  Cc: netdev, Andy Shevchenko, Joe Perches, Vineet Gupta,
	Mischa Jonker, Arnd Bergmann, Grant Likely, Rob Herring,
	Paul Gortmaker, David S. Miller, linux-kernel,
	devicetree-discuss

Alexey Brodkin <Alexey.Brodkin@synopsys.com> :
> On 06/14/2013 02:20 AM, Francois Romieu wrote:
[...]
> >> +struct arc_emac_priv {
> >> +	struct net_device_stats stats;
> >> +	unsigned int clock_frequency;
> >> +	unsigned int max_speed;
> >> +
> >> +	/* Pointers to BD rings - CPU side */
> >> +	struct arc_emac_bd_t *rxbd;
> >
> > There does not seem to be much need for rxbd->data.
> 
> Could you please clarify this comment? Not clear what do you mean.

Rx and Tx use the same struct but they don't work the same.
They could/should use differents struct.

[...]
> > The descriptor entry is left unchanged. Afaiu the driver will move to the
> > next descriptor and crash on dereferencing NULL (rx_buff->)skb next time
> > it wraps.
> >
> > I suggest avoiding holes: don't netif_receive_skb if you can't alloc a new
> > skb.
> 
> Frankly I cannot understand how "don't netif_receive_skb" for one of the 
> received packets helps to prevent crash on the next iteration?
> And I don't see a way to return any error state from NAPI poll handler.
> Could you please clarify your idea?

The driver assigns the otherwise-netif_received skb to the current rx
descriptor as if it was a newly allocated one. The driver increases
stats.rx_dropped. The rx descriptor ring doesn't ever exhibit a hole.

[...]
> >> +static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev)
> >> +{
> >> +	struct arc_emac_priv *priv = netdev_priv(ndev);
> >> +	unsigned int info, len, *txbd_curr = &priv->txbd_curr;
> >> +	dma_addr_t addr;
> >> +	char *pkt = skb->data;
> >> +
> >> +	len = max_t(unsigned int, ETH_ZLEN, skb->len);
> >
> > The device automatically pads, right ?
> 
> What do you mean here?

Does the device fill a smaller than 64 bytes packet with zeroes or may
the driver leak information ? The driver should use skb_pad if the
latter applies.

-- 
Ueimor

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

* Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver
@ 2013-06-14 19:27       ` Francois Romieu
  0 siblings, 0 replies; 21+ messages in thread
From: Francois Romieu @ 2013-06-14 19:27 UTC (permalink / raw)
  To: Alexey Brodkin
  Cc: netdev, Andy Shevchenko, Joe Perches, Vineet Gupta,
	Mischa Jonker, Arnd Bergmann, Grant Likely, Rob Herring,
	Paul Gortmaker, David S. Miller, linux-kernel,
	devicetree-discuss

Alexey Brodkin <Alexey.Brodkin@synopsys.com> :
> On 06/14/2013 02:20 AM, Francois Romieu wrote:
[...]
> >> +struct arc_emac_priv {
> >> +	struct net_device_stats stats;
> >> +	unsigned int clock_frequency;
> >> +	unsigned int max_speed;
> >> +
> >> +	/* Pointers to BD rings - CPU side */
> >> +	struct arc_emac_bd_t *rxbd;
> >
> > There does not seem to be much need for rxbd->data.
> 
> Could you please clarify this comment? Not clear what do you mean.

Rx and Tx use the same struct but they don't work the same.
They could/should use differents struct.

[...]
> > The descriptor entry is left unchanged. Afaiu the driver will move to the
> > next descriptor and crash on dereferencing NULL (rx_buff->)skb next time
> > it wraps.
> >
> > I suggest avoiding holes: don't netif_receive_skb if you can't alloc a new
> > skb.
> 
> Frankly I cannot understand how "don't netif_receive_skb" for one of the 
> received packets helps to prevent crash on the next iteration?
> And I don't see a way to return any error state from NAPI poll handler.
> Could you please clarify your idea?

The driver assigns the otherwise-netif_received skb to the current rx
descriptor as if it was a newly allocated one. The driver increases
stats.rx_dropped. The rx descriptor ring doesn't ever exhibit a hole.

[...]
> >> +static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev)
> >> +{
> >> +	struct arc_emac_priv *priv = netdev_priv(ndev);
> >> +	unsigned int info, len, *txbd_curr = &priv->txbd_curr;
> >> +	dma_addr_t addr;
> >> +	char *pkt = skb->data;
> >> +
> >> +	len = max_t(unsigned int, ETH_ZLEN, skb->len);
> >
> > The device automatically pads, right ?
> 
> What do you mean here?

Does the device fill a smaller than 64 bytes packet with zeroes or may
the driver leak information ? The driver should use skb_pad if the
latter applies.

-- 
Ueimor

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

* Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver
  2013-06-14 19:27       ` Francois Romieu
@ 2013-06-15  8:51         ` Alexey Brodkin
  -1 siblings, 0 replies; 21+ messages in thread
From: Alexey Brodkin @ 2013-06-15  8:51 UTC (permalink / raw)
  To: Francois Romieu
  Cc: netdev, Andy Shevchenko, Joe Perches, Vineet Gupta,
	Mischa Jonker, Arnd Bergmann, Grant Likely, Rob Herring,
	Paul Gortmaker, David S. Miller, linux-kernel,
	devicetree-discuss

On 06/14/2013 11:27 PM, Francois Romieu wrote:
> Alexey Brodkin <Alexey.Brodkin@synopsys.com> :
>> On 06/14/2013 02:20 AM, Francois Romieu wrote:
> [...]
>>>> +struct arc_emac_priv {
>>>> +	struct net_device_stats stats;
>>>> +	unsigned int clock_frequency;
>>>> +	unsigned int max_speed;
>>>> +
>>>> +	/* Pointers to BD rings - CPU side */
>>>> +	struct arc_emac_bd_t *rxbd;
>>>
>>> There does not seem to be much need for rxbd->data.
>>
>> Could you please clarify this comment? Not clear what do you mean.
>
> Rx and Tx use the same struct but they don't work the same.
> They could/should use differents struct.

Structure "arc_emac_bd" represents a buffer descriptor that is used for 
intercommunication between EMAC and CPU for both Tx and Rx.

"info" field contains a number of flags (who's an owner of this BD now, 
if this buffer contains beginning and/or end of received packets, 
received packet length etc.) Those flags are used for both Tx and Rx.
At least some of them like "owner" and "length". In case of Tx "length" 
indicates size of memory buffer available for receiving next packet.

"data" field is a 32-bit pointer to memory buffer.
In case of Tx buffer contains data to send, in case of Rx buffer is used 
for receiving data from network.

If my comment doesn't make sense to you or I misunderstood your message 
could you please add more clarifications for your idea?

> [...]
>>> The descriptor entry is left unchanged. Afaiu the driver will move to the
>>> next descriptor and crash on dereferencing NULL (rx_buff->)skb next time
>>> it wraps.
>>>
>>> I suggest avoiding holes: don't netif_receive_skb if you can't alloc a new
>>> skb.
>>
>> Frankly I cannot understand how "don't netif_receive_skb" for one of the
>> received packets helps to prevent crash on the next iteration?
>> And I don't see a way to return any error state from NAPI poll handler.
>> Could you please clarify your idea?
>
> The driver assigns the otherwise-netif_received skb to the current rx
> descriptor as if it was a newly allocated one. The driver increases
> stats.rx_dropped. The rx descriptor ring doesn't ever exhibit a hole.

Makes sense, thanks!

> [...]
>>>> +static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev)
>>>> +{
>>>> +	struct arc_emac_priv *priv = netdev_priv(ndev);
>>>> +	unsigned int info, len, *txbd_curr = &priv->txbd_curr;
>>>> +	dma_addr_t addr;
>>>> +	char *pkt = skb->data;
>>>> +
>>>> +	len = max_t(unsigned int, ETH_ZLEN, skb->len);
>>>
>>> The device automatically pads, right ?
>>
>> What do you mean here?
>
> Does the device fill a smaller than 64 bytes packet with zeroes or may
> the driver leak information ? The driver should use skb_pad if the
> latter applies.

Done, thanks.

-Alexey


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

* Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver
@ 2013-06-15  8:51         ` Alexey Brodkin
  0 siblings, 0 replies; 21+ messages in thread
From: Alexey Brodkin @ 2013-06-15  8:51 UTC (permalink / raw)
  To: Francois Romieu
  Cc: netdev, Andy Shevchenko, Joe Perches, Vineet Gupta,
	Mischa Jonker, Arnd Bergmann, Grant Likely, Rob Herring,
	Paul Gortmaker, David S. Miller, linux-kernel,
	devicetree-discuss

On 06/14/2013 11:27 PM, Francois Romieu wrote:
> Alexey Brodkin <Alexey.Brodkin@synopsys.com> :
>> On 06/14/2013 02:20 AM, Francois Romieu wrote:
> [...]
>>>> +struct arc_emac_priv {
>>>> +	struct net_device_stats stats;
>>>> +	unsigned int clock_frequency;
>>>> +	unsigned int max_speed;
>>>> +
>>>> +	/* Pointers to BD rings - CPU side */
>>>> +	struct arc_emac_bd_t *rxbd;
>>>
>>> There does not seem to be much need for rxbd->data.
>>
>> Could you please clarify this comment? Not clear what do you mean.
>
> Rx and Tx use the same struct but they don't work the same.
> They could/should use differents struct.

Structure "arc_emac_bd" represents a buffer descriptor that is used for 
intercommunication between EMAC and CPU for both Tx and Rx.

"info" field contains a number of flags (who's an owner of this BD now, 
if this buffer contains beginning and/or end of received packets, 
received packet length etc.) Those flags are used for both Tx and Rx.
At least some of them like "owner" and "length". In case of Tx "length" 
indicates size of memory buffer available for receiving next packet.

"data" field is a 32-bit pointer to memory buffer.
In case of Tx buffer contains data to send, in case of Rx buffer is used 
for receiving data from network.

If my comment doesn't make sense to you or I misunderstood your message 
could you please add more clarifications for your idea?

> [...]
>>> The descriptor entry is left unchanged. Afaiu the driver will move to the
>>> next descriptor and crash on dereferencing NULL (rx_buff->)skb next time
>>> it wraps.
>>>
>>> I suggest avoiding holes: don't netif_receive_skb if you can't alloc a new
>>> skb.
>>
>> Frankly I cannot understand how "don't netif_receive_skb" for one of the
>> received packets helps to prevent crash on the next iteration?
>> And I don't see a way to return any error state from NAPI poll handler.
>> Could you please clarify your idea?
>
> The driver assigns the otherwise-netif_received skb to the current rx
> descriptor as if it was a newly allocated one. The driver increases
> stats.rx_dropped. The rx descriptor ring doesn't ever exhibit a hole.

Makes sense, thanks!

> [...]
>>>> +static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev)
>>>> +{
>>>> +	struct arc_emac_priv *priv = netdev_priv(ndev);
>>>> +	unsigned int info, len, *txbd_curr = &priv->txbd_curr;
>>>> +	dma_addr_t addr;
>>>> +	char *pkt = skb->data;
>>>> +
>>>> +	len = max_t(unsigned int, ETH_ZLEN, skb->len);
>>>
>>> The device automatically pads, right ?
>>
>> What do you mean here?
>
> Does the device fill a smaller than 64 bytes packet with zeroes or may
> the driver leak information ? The driver should use skb_pad if the
> latter applies.

Done, thanks.

-Alexey

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

* Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver
  2013-06-15  8:51         ` Alexey Brodkin
@ 2013-06-15 11:12           ` Francois Romieu
  -1 siblings, 0 replies; 21+ messages in thread
From: Francois Romieu @ 2013-06-15 11:12 UTC (permalink / raw)
  To: Alexey Brodkin
  Cc: netdev, Andy Shevchenko, Joe Perches, Vineet Gupta,
	Mischa Jonker, Arnd Bergmann, Grant Likely, Rob Herring,
	Paul Gortmaker, David S. Miller, linux-kernel,
	devicetree-discuss

Alexey Brodkin <Alexey.Brodkin@synopsys.com> :
[...]
> If my comment doesn't make sense to you or I misunderstood your message 
> could you please add more clarifications for your idea?

Sorry, I temporarily forgot it was the descriptor ring itself. :o/

It would make sense to qualify the descriptor members as __{le/be}32 then
use them with proper cpu_to_{le/be} wrappers _and_ mapping addresses
instead of direct skb->data assignment.

-- 
Ueimor

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

* Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver
@ 2013-06-15 11:12           ` Francois Romieu
  0 siblings, 0 replies; 21+ messages in thread
From: Francois Romieu @ 2013-06-15 11:12 UTC (permalink / raw)
  To: Alexey Brodkin
  Cc: netdev, Andy Shevchenko, Joe Perches, Vineet Gupta,
	Mischa Jonker, Arnd Bergmann, Grant Likely, Rob Herring,
	Paul Gortmaker, David S. Miller, linux-kernel,
	devicetree-discuss

Alexey Brodkin <Alexey.Brodkin@synopsys.com> :
[...]
> If my comment doesn't make sense to you or I misunderstood your message 
> could you please add more clarifications for your idea?

Sorry, I temporarily forgot it was the descriptor ring itself. :o/

It would make sense to qualify the descriptor members as __{le/be}32 then
use them with proper cpu_to_{le/be} wrappers _and_ mapping addresses
instead of direct skb->data assignment.

-- 
Ueimor

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

* Query about TX BD Reclaim in Napi poll path (was Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver)
  2013-06-13 22:19 ` Francois Romieu
  2013-06-14 14:14     ` Alexey Brodkin
@ 2013-09-04 12:14   ` Vineet Gupta
  1 sibling, 0 replies; 21+ messages in thread
From: Vineet Gupta @ 2013-09-04 12:14 UTC (permalink / raw)
  To: Francois Romieu
  Cc: Alexey Brodkin, netdev, Andy Shevchenko, David S. Miller, linux-kernel

Hi Francois,

Resurrecting an old thread.

On 06/14/2013 03:49 AM, Francois Romieu wrote:
>> +static irqreturn_t arc_emac_intr(int irq, void *dev_instance)
>> > +{
>> > +	struct net_device *ndev = dev_instance;
>> > +	struct arc_emac_priv *priv = netdev_priv(ndev);
>> > +	struct net_device_stats *stats = &priv->stats;
>> > +	unsigned int status;
>> > +
>> > +	status = arc_reg_get(priv, R_STATUS);
>> > +	status &= ~MDIO_MASK;
>> > +
>> > +	/* Reset all flags except "MDIO complete"*/
>> > +	arc_reg_set(priv, R_STATUS, status);
>> > +
>> > +	if (status & RXINT_MASK) {
>> > +		if (likely(napi_schedule_prep(&priv->napi))) {
>> > +			arc_reg_clr(priv, R_ENABLE, RXINT_MASK);
>> > +			__napi_schedule(&priv->napi);
>> > +		}
>> > +	}
>> > +
>> > +	if (status & TXINT_MASK) {
> You may consider moving everything into the napi poll handler.

I has to revisit this now-mainlined driver recently for fixing a bug. Per your
suggestion above, the TX BD reclaim was moved from interrupt context to NAPI
context. I was wondering if that is the right thing to do (I'm not a networking
expert but have worked on this driver heavily before it was mainlined by Alexey).

In case of large burst transfers by networking stack (say a large file copy over
NFS) will it not delay the TX BD reclaim possibly dropping more packets. Ofcourse
doing this requires enabling Tx interrupts which adds to overall cost from a
system perspective, but assuming the controller can coalesce the Tx interrupts,
will it not be better.

I did a quick hack to move the TX reclaim in intr path and it seems to be doing
slightly better than the current code - so the advantages are not sky high, but I
want to understand the implications nevertheless.

TIA,
-Vineet

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

end of thread, other threads:[~2013-09-04 12:14 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-13 14:37 [PATCH v3] ethernet/arc/arc_emac - Add new driver Alexey Brodkin
2013-06-13 14:37 ` Alexey Brodkin
2013-06-13 18:25 ` Andy Shevchenko
2013-06-13 18:33   ` Joe Perches
2013-06-13 18:33     ` Joe Perches
2013-06-13 19:16     ` Andy Shevchenko
2013-06-13 19:54     ` David Miller
2013-06-13 19:42   ` Francois Romieu
2013-06-13 20:25   ` Alexey Brodkin
2013-06-13 20:50     ` Andy Shevchenko
2013-06-13 21:48       ` Alexey Brodkin
2013-06-13 22:19 ` Francois Romieu
2013-06-14 14:14   ` Alexey Brodkin
2013-06-14 14:14     ` Alexey Brodkin
2013-06-14 19:27     ` Francois Romieu
2013-06-14 19:27       ` Francois Romieu
2013-06-15  8:51       ` Alexey Brodkin
2013-06-15  8:51         ` Alexey Brodkin
2013-06-15 11:12         ` Francois Romieu
2013-06-15 11:12           ` Francois Romieu
2013-09-04 12:14   ` Query about TX BD Reclaim in Napi poll path (was Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver) Vineet Gupta

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.