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, build tested for x86.
Signed-off-by: Alexey Brodkin <[email protected]>
Reviewed-by: Vineet Gupta <[email protected]>
Reviewed-by: Mischa Jonker <[email protected]>
Cc: Vineet Gupta <[email protected]>
Cc: Mischa Jonker <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Grant Likely <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Paul Gortmaker <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: [email protected]
Cc: [email protected]
---
Hi all,
this is v2 respin of "arc_emac" driver.
Please find summary of changes below.
* Cosmetics as suggested by David Miller - mostly identation fixes in
accordance to the CodingStyle
* Get rid of extra header "arc_emac_main.h". Its contents moved right
into "arc_emac_main.c"
* Merge PHY description in "Ethernet" entry in DT so it could be
accessed by "of_mdiobus_register".
* Add dependences from OF_NET and OF_IRQ in Kconfig to make sure all
needed functions are avaialable.
* Add DT binding description.
* Use "void __iomem *" pointers for MMIO mappings instead of simple
"void *"
* Use "devm_ioremap_resource" instead of pair:
"devm_request_mem_region" & "devm_ioremap_nocache"
---
Documentation/devicetree/bindings/net/arc_emac.txt | 32 +
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_main.c | 956 ++++++++++++++++++++
drivers/net/ethernet/arc/arc_emac_mdio.c | 170 ++++
drivers/net/ethernet/arc/arc_emac_mdio.h | 22 +
drivers/net/ethernet/arc/arc_emac_regs.h | 72 ++
9 files changed, 1291 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_main.c
create mode 100644 drivers/net/ethernet/arc/arc_emac_mdio.c
create mode 100644 drivers/net/ethernet/arc/arc_emac_mdio.h
create mode 100644 drivers/net/ethernet/arc/arc_emac_regs.h
diff --git a/Documentation/devicetree/bindings/net/arc_emac.txt b/Documentation/devicetree/bindings/net/arc_emac.txt
new file mode 100644
index 0000000..4205581
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/arc_emac.txt
@@ -0,0 +1,32 @@
+* 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.
+- 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>;
+ 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_main.c b/drivers/net/ethernet/arc/arc_emac_main.c
new file mode 100644
index 0000000..f098a27
--- /dev/null
+++ b/drivers/net/ethernet/arc/arc_emac_main.c
@@ -0,0 +1,956 @@
+/*
+ * Copyright (C) 2004, 2007-2010, 2011-2013 Synopsys, Inc. (http://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 (Rev 5)
+ *
+ * Alexey Brodkin: June 2013
+ * -Upsteaming
+ * -Refactoring
+ * = Use device-tree/OF for configuration
+ * = Use libphy for phy management
+ * = Remove non-NAPI code sections
+ * = Remove ARC-specific BD management implementations
+ * = Add ethtool functionality
+ *
+ * Vineet Gupta: June 2011
+ * -Issues when working with 64b cache line size
+ * = BD rings point to aligned location in an internal buffer
+ * = added support for cache coherent BD Ring memory
+ *
+ * Vineet Gupta: May 2010
+ * -Reduced foot-print of the main ISR (handling for error cases moved out
+ * into a separate non-inline function).
+ * -Driver Tx path optimized for small packets (which fit into 1 BD = 2K).
+ * Any specifics for chaining are in a separate block of code.
+ *
+ * Vineet Gupta: Nov 2009
+ * -Unified NAPI and Non-NAPI Code.
+ * -API changes since 2.6.26 for making NAPI independent of netdevice
+ * -Cutting a few checks in main Rx poll routine
+ * -Tweaked NAPI implementation:
+ * In poll mode, Original driver would always start sweeping BD chain
+ * from BD-0 upto poll budget (40). And if it got over-budget it would
+ * drop reminder of packets.
+ * Instead now we remember last BD polled and in next
+ * cycle, we resume from next BD onwards. That way in case of over-budget
+ * no packet needs to be dropped.
+ *
+ * Vineet Gupta: Nov 2009
+ * -Rewrote the driver register access macros so that multiple accesses
+ * in same function use "anchor" reg to save the base addr causing
+ * shorter instructions
+ *
+ * Amit Bhor, Sameer Dhavale: 2004
+ */
+
+#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_regs.h"
+#include "arc_emac_mdio.h"
+
+#define DRV_NAME "arc_emac"
+#define DRV_VERSION "2.0"
+
+/* Transmission timeout */
+#define TX_TIMEOUT (400*HZ/1000)
+
+#define NAPI_WEIGHT 40 /* Workload for NAPI */
+
+#define TX_FIFO_SIZE 0x800 /* 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;
+ void *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 arc_emac_priv {
+ struct net_device_stats stats;
+ unsigned int clock_frequency;
+
+ /* 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 sk_buff *rx_skbuff[RX_BD_NUM];
+ struct sk_buff *tx_skbuff[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;
+
+ /* Storage for "arc_emac_adjust_link" */
+ int old_link;
+ int old_speed;
+ int old_duplex;
+
+ /* Devices */
+ struct device_node *phy_node;
+ struct phy_device *phy_dev;
+ struct net_device *net_dev;
+
+ /* MDIO private structure */
+ struct arc_mdio_priv *mdio_priv;
+
+ /* EMAC registers base address */
+ void __iomem *reg_base_addr;
+};
+
+/**
+ * arc_emac_adjust_link - Adjust the PHY link speed/duplex.
+ * @net_dev: Pointer to the net_device structure.
+ *
+ * This function is called to change the speed and duplex setting after
+ * auto negotiation is done by the PHY.
+ */
+static void arc_emac_adjust_link(struct net_device *net_dev)
+{
+ struct arc_emac_priv *priv = netdev_priv(net_dev);
+ struct phy_device *phydev = priv->phy_dev;
+ unsigned int reg, status_change = 0;
+
+ BUG_ON(!priv->phy_dev);
+
+ if (phydev->link && (priv->old_speed != phydev->speed)) {
+ /* speed changed */
+ switch (phydev->speed) {
+ case SPEED_10:
+ case SPEED_100:
+ break;
+ default:
+ netdev_warn(net_dev, "Speed (%d) is not 10/100?\n",
+ phydev->speed);
+ break;
+ }
+ priv->old_speed = phydev->speed;
+ status_change = 1;
+ }
+
+ if (phydev->link && (priv->old_duplex != phydev->duplex)) {
+ /* duplex mode changed */
+ reg = EMAC_REG_GET(priv->reg_base_addr, R_CTRL);
+
+ if (DUPLEX_FULL == phydev->duplex)
+ reg |= ENFL_MASK;
+ else
+ reg &= ~ENFL_MASK;
+
+ EMAC_REG_SET(priv->reg_base_addr, R_CTRL, reg);
+ priv->old_duplex = phydev->duplex;
+ status_change = 1;
+ }
+
+ if (phydev->link != priv->old_link) {
+ /* link state changed */
+ if (!phydev->link) {
+ /* link went down */
+ priv->old_speed = 0;
+ priv->old_duplex = -1;
+ }
+ priv->old_link = phydev->link;
+ status_change = 1;
+ }
+
+ if (status_change)
+ phy_print_status(phydev);
+}
+
+/**
+ * arc_emac_get_settings - Get PHY settings.
+ * @net_dev: 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 *net_dev,
+ struct ethtool_cmd *cmd)
+{
+ struct arc_emac_priv *priv = netdev_priv(net_dev);
+
+ 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.
+ * @net_dev: 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 *net_dev,
+ struct ethtool_cmd *cmd)
+{
+ struct arc_emac_priv *priv = netdev_priv(net_dev);
+
+ 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.
+ * @net_dev: 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 *net_dev,
+ 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,
+};
+
+static int arc_emac_poll(struct napi_struct *napi, int budget)
+{
+ struct net_device *net_dev = napi->dev;
+ struct arc_emac_priv *priv = netdev_priv(net_dev);
+ struct sk_buff *skb, *skbnew;
+ unsigned int i, loop, len, info, work_done = 0;
+
+ /* 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++) {
+ i = (i + 1) & (RX_BD_NUM - 1);
+
+ info = priv->rxbd[i].info;
+
+ /* BD contains a packet for CPU to grab */
+ if (likely((info & OWN_MASK) == FOR_CPU)) {
+
+ /* Make a note that we saw a packet at this BD.
+ * So next time, driver starts from this + 1
+ */
+ priv->last_rx_bd = i;
+
+ /* Packet fits in one BD (Non Fragmented) */
+ if (likely((info & (FRST_MASK | LAST_MASK)) ==
+ (FRST_MASK | LAST_MASK))) {
+
+ len = info & LEN_MASK;
+ priv->stats.rx_packets++;
+ priv->stats.rx_bytes += len;
+ skb = priv->rx_skbuff[i];
+
+ /* Get a new SKB from stack */
+ skbnew = netdev_alloc_skb(net_dev,
+ net_dev->mtu +
+ EMAC_BUFFER_PAD);
+
+ if (!skbnew) {
+ netdev_err(net_dev, "Out of memory, "
+ "dropping packet\n");
+
+ /* Return buffer to EMAC */
+ priv->rxbd[i].info = FOR_EMAC |
+ (net_dev->mtu + EMAC_BUFFER_PAD);
+ priv->stats.rx_dropped++;
+ continue;
+ }
+
+ /* Actually preparing the BD for next cycle
+ * IP header align, eth is 14 bytes
+ */
+ skb_reserve(skbnew, 2);
+ priv->rx_skbuff[i] = skbnew;
+
+ priv->rxbd[i].data = skbnew->data;
+ priv->rxbd[i].info = FOR_EMAC |
+ (net_dev->mtu + EMAC_BUFFER_PAD);
+
+ /* Prepare arrived pkt for delivery to stack */
+ dma_map_single(&net_dev->dev, (void *)skb->data,
+ len, DMA_FROM_DEVICE);
+
+ skb->dev = net_dev;
+
+ /* Make room for data */
+ skb_put(skb, len - 4);
+ skb->protocol = eth_type_trans(skb, net_dev);
+
+ /* Correct NAPI semantics:
+ * If work quota exceeded return don't de-queue
+ * any further packets
+ */
+ work_done++;
+ netif_receive_skb(skb);
+ if (work_done >= budget)
+ break;
+ } else {
+ /* Buffer chaining results in a significant
+ * amount of additional bus overhead and thus
+ * a higher CLK frequency is required or larger
+ * FIFOs are required.
+ * Because of that fact we don't support
+ * chaining of receive packets. We preallocate
+ * buffers of MTU size so incoming packets
+ * won't be chained
+ */
+ netdev_warn(net_dev,
+ "Rx chained, packet is larger than "
+ "device MTU (%d bytes)\n",
+ net_dev->mtu);
+
+ /* Return buffer to EMAC */
+ priv->rxbd[i].info = FOR_EMAC |
+ (net_dev->mtu + EMAC_BUFFER_PAD);
+ priv->stats.rx_length_errors++;
+ }
+ } /* BD for CPU */
+ } /* BD chain loop */
+
+ if (work_done < budget) {
+ napi_complete(napi);
+ EMAC_REG_OR(priv->reg_base_addr, R_ENABLE, RXINT_MASK);
+ }
+
+ return work_done;
+}
+
+/**
+ * arc_emac_intr - Global interrupt handler for EMAC.
+ * @irq: irq number.
+ * @net_dev: net_device pointer.
+ *
+ * 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 *net_dev = (struct net_device *)dev_instance;
+ struct arc_emac_priv *priv = netdev_priv(net_dev);
+ unsigned int status;
+
+ /* Read STATUS register from EMAC */
+ status = EMAC_REG_GET(priv->reg_base_addr, R_STATUS);
+
+ /* Mask all bits except "MDIO complete" */
+ status &= ~MDIO_MASK;
+
+ /* To reset any bit in STATUS register we need to write "1" in
+ * corresponding bit. That's why we write only masked bits back.
+ */
+ EMAC_REG_SET(priv->reg_base_addr, R_STATUS, status);
+
+ if (likely(status & (RXINT_MASK | TXINT_MASK))) {
+ if (status & RXINT_MASK) {
+ if (likely(napi_schedule_prep(&priv->napi))) {
+ EMAC_REG_CLR(priv->reg_base_addr, R_ENABLE,
+ RXINT_MASK);
+ __napi_schedule(&priv->napi);
+ }
+ }
+ if (status & TXINT_MASK) {
+ unsigned int i, info;
+ struct sk_buff *skb;
+
+ for (i = 0; i < TX_BD_NUM; i++) {
+ info = priv->txbd[priv->txbd_dirty].info;
+
+ if (info & (DROP | DEFR | LTCL | UFLO))
+ netdev_warn(net_dev,
+ "add Tx errors to stats\n");
+
+ if ((info & FOR_EMAC) ||
+ !priv->txbd[priv->txbd_dirty].data)
+ break;
+
+ if (info & LAST_MASK) {
+ skb = priv->tx_skbuff[priv->txbd_dirty];
+ priv->stats.tx_packets++;
+ priv->stats.tx_bytes += skb->len;
+
+ /* return the sk_buff to system */
+ dev_kfree_skb_irq(skb);
+ }
+ priv->txbd[priv->txbd_dirty].data = 0x0;
+ priv->txbd[priv->txbd_dirty].info = 0x0;
+ priv->txbd_dirty = (priv->txbd_dirty + 1) %
+ TX_BD_NUM;
+ }
+ }
+ } else {
+ if (status & ERR_MASK) {
+ /* MSER/RXCR/RXFR/RXFL interrupt fires on corresponding
+ * 8-bit error counter overrun.
+ * Because of this fact we add 256 items each time
+ * overrun interrupt happens.
+ */
+
+ if (status & TXCH_MASK) {
+ priv->stats.tx_errors++;
+ priv->stats.tx_aborted_errors++;
+ netdev_err(priv->net_dev,
+ "Tx chaining err! txbd_dirty = %u\n",
+ priv->txbd_dirty);
+ } else if (status & MSER_MASK) {
+ priv->stats.rx_missed_errors += 255;
+ priv->stats.rx_errors += 255;
+ } else if (status & RXCR_MASK) {
+ priv->stats.rx_crc_errors += 255;
+ priv->stats.rx_errors += 255;
+ } else if (status & RXFR_MASK) {
+ priv->stats.rx_frame_errors += 255;
+ priv->stats.rx_errors += 255;
+ } else if (status & RXFL_MASK) {
+ priv->stats.rx_over_errors += 255;
+ priv->stats.rx_errors += 255;
+ } else {
+ netdev_err(priv->net_dev,
+ "unknown err. Status reg is 0x%x\n",
+ status);
+ }
+ }
+ }
+ return IRQ_HANDLED;
+}
+
+/**
+ * arc_emac_open - Open the network device.
+ * @net_dev: 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.
+ */
+int arc_emac_open(struct net_device *net_dev)
+{
+ struct arc_emac_priv *priv = netdev_priv(net_dev);
+ struct arc_emac_bd_t *bd;
+ struct sk_buff *skb;
+ int i;
+
+ if (!priv->phy_node) {
+ netdev_err(net_dev, "arc_emac_open: phy device is absent\n");
+ return -ENODEV;
+ }
+
+ priv->phy_dev = of_phy_connect(net_dev, priv->phy_node,
+ arc_emac_adjust_link, 0,
+ PHY_INTERFACE_MODE_MII);
+
+ if (!priv->phy_dev) {
+ netdev_err(net_dev, "of_phy_connect() failed\n");
+ return -ENODEV;
+ }
+
+ netdev_info(net_dev, "connected to %s phy with id 0x%x\n",
+ priv->phy_dev->drv->name, priv->phy_dev->phy_id);
+
+ priv->phy_dev->autoneg = AUTONEG_ENABLE;
+ priv->phy_dev->speed = 0;
+ priv->phy_dev->duplex = 0;
+
+ /* We support only up-to 100mbps speeds */
+ priv->phy_dev->advertising = priv->phy_dev->supported;
+ priv->phy_dev->advertising &= PHY_BASIC_FEATURES;
+ priv->phy_dev->advertising |= ADVERTISED_Autoneg;
+
+ /* Restart auto negotiation */
+ phy_start_aneg(priv->phy_dev);
+
+ netif_start_queue(net_dev);
+
+ /* Allocate and set buffers for Rx BD's */
+ bd = priv->rxbd;
+ for (i = 0; i < RX_BD_NUM; i++) {
+ skb = netdev_alloc_skb(net_dev, net_dev->mtu + EMAC_BUFFER_PAD);
+
+ /* IP header Alignment (14 byte Ethernet header) */
+ skb_reserve(skb, 2);
+ priv->rx_skbuff[i] = skb;
+ bd->data = skb->data;
+
+ /* EMAC owns Rx descriptors */
+ bd->info = FOR_EMAC | (net_dev->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 */
+ EMAC_REG_SET(priv->reg_base_addr, R_LAFL, 0);
+ EMAC_REG_SET(priv->reg_base_addr, R_LAFH, 0);
+
+ /* Set BD ring pointers for device side */
+ EMAC_REG_SET(priv->reg_base_addr, R_RX_RING,
+ (unsigned int)priv->rxbd_dma_hdl);
+
+ EMAC_REG_SET(priv->reg_base_addr, R_TX_RING,
+ (unsigned int)priv->rxbd_dma_hdl + RX_RING_SZ);
+
+ /* Set Poll rate so that it polls every 1 ms */
+ EMAC_REG_SET(priv->reg_base_addr, R_POLLRATE,
+ priv->clock_frequency / 1000000);
+
+ /* Enable interrupts. Note: interrupts wont actually come till we set
+ * CONTROL below.
+ */
+ EMAC_REG_SET(priv->reg_base_addr, R_ENABLE,
+ TXINT_MASK | /* Transmit interrupt */
+ RXINT_MASK | /* Receive interrupt */
+ ERR_MASK | /* Error interrupt */
+ TXCH_MASK | /* Transmit chaining error interrupt */
+ MSER_MASK); /* Missed packet error */
+
+ /* Set CONTROL */
+ EMAC_REG_SET(priv->reg_base_addr, R_CTRL,
+ (RX_BD_NUM << 24) | /* RX buffer desc table len */
+ (TX_BD_NUM << 16) | /* TX buffer desc table len */
+ TXRN_MASK | /* TX enable */
+ RXRN_MASK); /* RX enable */
+
+ EMAC_REG_OR(priv->reg_base_addr, R_CTRL, EN_MASK);
+
+ netif_wake_queue(net_dev);
+ napi_enable(&priv->napi);
+
+ return 0;
+}
+
+/**
+ * arc_emac_stop - Close the network device.
+ * @net_dev: 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.
+ */
+int arc_emac_stop(struct net_device *net_dev)
+{
+ struct arc_emac_priv *priv = netdev_priv(net_dev);
+
+ napi_disable(&priv->napi);
+ netif_stop_queue(net_dev);
+
+ /* Disable interrupts */
+ EMAC_REG_CLR(priv->reg_base_addr, R_ENABLE,
+ TXINT_MASK | /* Tx interrupt */
+ RXINT_MASK | /* Rx interrupt */
+ ERR_MASK | /* Error interrupt */
+ TXCH_MASK); /* Transmit chaining error interrupt */
+
+ if (priv->phy_dev)
+ phy_disconnect(priv->phy_dev);
+
+ priv->phy_dev = NULL;
+
+ /* Disable EMAC */
+ EMAC_REG_CLR(priv->reg_base_addr, R_CTRL, EN_MASK);
+
+ return 0;
+}
+
+/**
+ * arc_emac_stats - Get system network statistics.
+ * @net_dev: Pointer to net_device structure.
+ *
+ * Returns the address of the device statistics structure.
+ * Statistics are updated in interrupt handler.
+ */
+struct net_device_stats *arc_emac_stats(struct net_device *net_dev)
+{
+ unsigned long miss, rxerr, rxfram, rxcrc, rxoflow;
+ struct arc_emac_priv *priv = netdev_priv(net_dev);
+
+ rxerr = EMAC_REG_GET(priv->reg_base_addr, R_RXERR);
+ miss = EMAC_REG_GET(priv->reg_base_addr, R_MISS);
+
+ rxcrc = (rxerr & 0xff);
+ rxfram = (rxerr >> 8 & 0xff);
+ rxoflow = (rxerr >> 16 & 0xff);
+
+ priv->stats.rx_errors += miss;
+ priv->stats.rx_errors += rxcrc + rxfram + rxoflow;
+
+ priv->stats.rx_over_errors += rxoflow;
+ priv->stats.rx_frame_errors += rxfram;
+ priv->stats.rx_crc_errors += rxcrc;
+ priv->stats.rx_missed_errors += miss;
+
+ return &priv->stats;
+}
+
+/**
+ * arc_emac_tx - Starts the data transmission.
+ * @skb: sk_buff pointer that contains data to be Transmitted.
+ * @net_dev: 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.
+ */
+int arc_emac_tx(struct sk_buff *skb, struct net_device *net_dev)
+{
+ int len, bitmask;
+ unsigned int info;
+ char *pkt;
+ struct arc_emac_priv *priv = netdev_priv(net_dev);
+
+ len = skb->len < ETH_ZLEN ? ETH_ZLEN : skb->len;
+ pkt = skb->data;
+ net_dev->trans_start = jiffies;
+
+ dma_map_single(&net_dev->dev, (void *)pkt, len, DMA_TO_DEVICE);
+
+ /* Set first bit */
+ bitmask = FRST_MASK;
+
+tx_next_chunk:
+
+ info = priv->txbd[priv->txbd_curr].info;
+ if (likely((info & OWN_MASK) == FOR_CPU)) {
+ priv->txbd[priv->txbd_curr].data = pkt;
+
+ /* This case handles 2 scenarios:
+ * 1. pkt fits into 1 BD (2k)
+ * 2. Last chunk of pkt (in multiples of 2k)
+ */
+ if (likely(len <= TX_FIFO_SIZE)) {
+ priv->tx_skbuff[priv->txbd_curr] = skb;
+
+ /* Set data length, bit mask and give ownership to
+ * EMAC This should be the last thing we do. EMAC
+ * might immediately start sending
+ */
+ priv->txbd[priv->txbd_curr].info = FOR_EMAC | bitmask |
+ LAST_MASK | len;
+
+ /* Set TXPOLL bit to force a poll */
+ EMAC_REG_SET(priv->reg_base_addr, R_STATUS, TXPL_MASK);
+
+ priv->txbd_curr = (priv->txbd_curr + 1) % TX_BD_NUM;
+ return NETDEV_TX_OK;
+ } else {
+ /* if pkt > 2k, this case handles all non last chunks */
+ priv->txbd[priv->txbd_curr].info = FOR_EMAC | bitmask |
+ (len & (TX_FIFO_SIZE - 1));
+
+ /* clear the FIRST CHUNK bit */
+ bitmask = 0;
+
+ len -= TX_FIFO_SIZE;
+ }
+
+ priv->txbd_curr = (priv->txbd_curr + 1) % TX_BD_NUM;
+ goto tx_next_chunk;
+
+ } else {
+ return NETDEV_TX_BUSY;
+ }
+}
+
+/**
+ * arc_emac_set_address - Set the MAC address for this device.
+ * @net_dev: 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.
+ */
+int arc_emac_set_address(struct net_device *net_dev, void *p)
+{
+ struct sockaddr *addr = p;
+ struct arc_emac_priv *priv = netdev_priv(net_dev);
+
+ if (netif_running(net_dev))
+ return -EBUSY;
+
+ memcpy(net_dev->dev_addr, addr->sa_data, net_dev->addr_len);
+
+ EMAC_REG_SET(priv->reg_base_addr, R_ADDRL,
+ *(unsigned int *)net_dev->dev_addr);
+
+ EMAC_REG_SET(priv->reg_base_addr, R_ADDRH,
+ *(unsigned int *)&net_dev->dev_addr[4] & 0x0000ffff);
+
+ 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 *net_dev;
+ struct arc_emac_priv *priv;
+ int err;
+ unsigned int id, clock_frequency;
+ struct resource res_regs, res_irq;
+ const char *mac_addr = NULL;
+
+ /* no device tree device */
+ if (!pdev->dev.of_node)
+ return -ENODEV;
+
+ net_dev = alloc_etherdev(sizeof(struct arc_emac_priv));
+ if (!net_dev) {
+ err = -ENOMEM;
+ goto out;
+ }
+
+ SET_NETDEV_DEV(net_dev, &pdev->dev);
+
+ /* Populate our net_device structure */
+ net_dev->netdev_ops = &arc_emac_netdev_ops;
+ net_dev->ethtool_ops = &arc_emac_ethtool_ops;
+ net_dev->watchdog_timeo = TX_TIMEOUT;
+ /* FIXME :: no multicast support yet */
+ net_dev->flags &= ~IFF_MULTICAST;
+
+ priv = netdev_priv(net_dev);
+ priv->net_dev = net_dev;
+
+ /* 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)) {
+ dev_err(&pdev->dev, "failed to ioremap MAC registers\n");
+ 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 = EMAC_REG_GET(priv->reg_base_addr, 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",
+ &clock_frequency)) {
+ dev_err(&pdev->dev, "failed to retrieve <clock-frequency> "
+ "from device tree\n");
+ err = -EINVAL;
+ goto out;
+ }
+ priv->clock_frequency = clock_frequency;
+
+ /* 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;
+ }
+ net_dev->irq = res_irq.start;
+ dev_info(&pdev->dev, "IRQ is %d\n", net_dev->irq);
+
+ /* Register interrupt handler for device */
+ err = devm_request_irq(&pdev->dev, net_dev->irq, arc_emac_intr, 0,
+ net_dev->name, net_dev);
+ 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(net_dev);
+ else
+ memcpy(net_dev->dev_addr, mac_addr, ETH_ALEN);
+
+ dev_info(&pdev->dev, "MAC address is now %pM\n", net_dev->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 = (struct arc_emac_bd_t *)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);
+
+ priv->mdio_priv = devm_kzalloc(&pdev->dev, sizeof(struct arc_mdio_priv),
+ GFP_KERNEL);
+
+ if (!priv->mdio_priv) {
+ dev_err(&pdev->dev, "cannot allocate memory for MDIO "
+ "private structure\n");
+ err = -ENOMEM;
+ goto out;
+ }
+
+ priv->mdio_priv->reg_base_addr = priv->reg_base_addr;
+ priv->mdio_priv->dev = &pdev->dev;
+
+ err = arc_mdio_probe(pdev->dev.of_node, priv->mdio_priv);
+ if (err) {
+ dev_err(&pdev->dev, "failed to probe MII bus\n");
+ goto out;
+ }
+
+ netif_napi_add(net_dev, &priv->napi, arc_emac_poll, NAPI_WEIGHT);
+
+ err = register_netdev(net_dev);
+ if (err) {
+ netif_napi_del(&priv->napi);
+ dev_err(&pdev->dev, "failed to register network device\n");
+ goto out;
+ }
+
+ return 0;
+
+out:
+ free_netdev(net_dev);
+ return err;
+}
+
+static int arc_emac_remove(struct platform_device *pdev)
+{
+ struct net_device *net_dev = platform_get_drvdata(pdev);
+ struct arc_emac_priv *priv = netdev_priv(net_dev);
+
+ arc_mdio_remove(priv->mdio_priv);
+ unregister_netdev(net_dev);
+ netif_napi_del(&priv->napi);
+ free_netdev(net_dev);
+
+ 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 <[email protected]>");
+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..7d13dd5
--- /dev/null
+++ b/drivers/net/ethernet/arc/arc_emac_mdio.c
@@ -0,0 +1,170 @@
+/*
+ * Copyright (C) 2004, 2007-2010, 2011-2013 Synopsys, Inc. (http://www.synopsys.com)
+ *
+ * MDIO implementation for ARC EMAC
+ */
+
+#include <linux/delay.h>
+#include <linux/of_mdio.h>
+
+#include "arc_emac_regs.h"
+#include "arc_emac_mdio.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 MDIO private data structure.
+ *
+ * returns: 0 on success, -ETIMEDOUT on a timeout.
+ */
+static int arc_mdio_complete_wait(struct arc_mdio_priv *priv)
+{
+ unsigned int status;
+ unsigned int count = (ARC_MDIO_COMPLETE_POLL_COUNT * 40);
+
+ while (1) {
+ /* Read STATUS register from EMAC */
+ status = EMAC_REG_GET(priv->reg_base_addr, R_STATUS);
+
+ /* Mask "MDIO complete" bit */
+ status &= MDIO_MASK;
+
+ if (status) {
+ /* Reset "MDIO complete" flag */
+ EMAC_REG_SET(priv->reg_base_addr, R_STATUS, status);
+ break;
+ }
+
+ /* Make sure we never get into infinite loop */
+ if (count-- == 0) {
+ WARN_ON(1);
+ return -ETIMEDOUT;
+ }
+ msleep(25);
+ }
+ return 0;
+}
+
+/**
+ * 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)
+{
+ int error;
+ unsigned int value;
+ struct arc_mdio_priv *priv = bus->priv;
+
+ EMAC_REG_SET(priv->reg_base_addr, R_MDIO,
+ 0x60020000 | (phy_addr << 23) | (reg_num << 18));
+
+ error = arc_mdio_complete_wait(priv);
+ if (error < 0)
+ return error;
+
+ value = EMAC_REG_GET(priv->reg_base_addr, 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_mdio_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);
+
+ EMAC_REG_SET(priv->reg_base_addr, R_MDIO,
+ 0x50020000 | (phy_addr << 23) | (reg_num << 18) |
+ (value & 0xffff));
+
+ return arc_mdio_complete_wait(priv);
+}
+
+/**
+ * arc_mdio_probe - MDIO probe function.
+ * @dev_node: Pointer to device node.
+ * @priv: Pointer to ARC MDIO 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_mdio_priv *priv)
+{
+ struct mii_bus *bus;
+ int error;
+
+ bus = mdiobus_alloc();
+ if (!bus) {
+ error = -ENOMEM;
+ goto cleanup;
+ }
+
+ priv->bus = bus;
+ bus->priv = priv;
+ 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);
+
+ bus->parent = priv->dev;
+
+ error = of_mdiobus_register(bus, dev_node);
+ if (error) {
+ dev_err(priv->dev, "cannot register %s as MDIO bus\n",
+ bus->name);
+ goto cleanup;
+ }
+
+ return 0;
+
+cleanup:
+ if (bus)
+ mdiobus_free(bus);
+
+ return error;
+}
+
+/**
+ * arc_mdio_remove - MDIO remove function.
+ * @priv: Pointer to ARC MDIO private data structure.
+ *
+ * Unregisters the MDIO and frees any associate memory for MII bus.
+ */
+int arc_mdio_remove(struct arc_mdio_priv *priv)
+{
+ mdiobus_unregister(priv->bus);
+ mdiobus_free(priv->bus);
+ priv->bus = NULL;
+
+ return 0;
+}
diff --git a/drivers/net/ethernet/arc/arc_emac_mdio.h b/drivers/net/ethernet/arc/arc_emac_mdio.h
new file mode 100644
index 0000000..954241e
--- /dev/null
+++ b/drivers/net/ethernet/arc/arc_emac_mdio.h
@@ -0,0 +1,22 @@
+/*
+ * Copyright (C) 2004, 2007-2010, 2011-2013 Synopsys, Inc. (http://www.synopsys.com)
+ *
+ * Definitions for MDIO of ARC EMAC device driver
+ */
+
+#ifndef ARC_MDIO_H
+#define ARC_MDIO_H
+
+#include <linux/device.h>
+#include <linux/phy.h>
+
+struct arc_mdio_priv {
+ struct mii_bus *bus;
+ struct device *dev;
+ void __iomem *reg_base_addr; /* MAC registers base address */
+};
+
+int arc_mdio_probe(struct device_node *np, struct arc_mdio_priv *priv);
+int arc_mdio_remove(struct arc_mdio_priv *priv);
+
+#endif /* ARC_MDIO_H */
diff --git a/drivers/net/ethernet/arc/arc_emac_regs.h b/drivers/net/ethernet/arc/arc_emac_regs.h
new file mode 100644
index 0000000..e4c8d73
--- /dev/null
+++ b/drivers/net/ethernet/arc/arc_emac_regs.h
@@ -0,0 +1,72 @@
+/*
+ * Copyright (C) 2004, 2007-2010, 2011-2013 Synopsys, Inc. (http://www.synopsys.com)
+ *
+ * Registers and bits definitions of ARC EMAC
+ */
+
+#ifndef ARC_EMAC_REGS_H
+#define ARC_EMAC_REGS_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) /* TXPOLL */
+
+/* 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,
+};
+
+#define EMAC_REG_SET(reg_base_addr, reg, val) \
+ iowrite32((val), reg_base_addr + reg * sizeof(int))
+
+#define EMAC_REG_GET(reg_base_addr, reg) \
+ ioread32(reg_base_addr + reg * sizeof(int))
+
+#define EMAC_REG_OR(b, r, v) EMAC_REG_SET(b, r, EMAC_REG_GET(b, r) | (v))
+#define EMAC_REG_CLR(b, r, v) EMAC_REG_SET(b, r, EMAC_REG_GET(b, r) & ~(v))
+
+#endif /* ARC_EMAC_REGS_H */
--
1.7.10.4
On Friday 07 June 2013, Alexey Brodkin 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.
>
> This is based off of current Linus tree, build tested for x86.
>
> Signed-off-by: Alexey Brodkin <[email protected]>
> Reviewed-by: Vineet Gupta <[email protected]>
> Reviewed-by: Mischa Jonker <[email protected]>
>
Acked-by: Arnd Bergmann <[email protected]>
On Fri, 2013-06-07 at 19:07 +0400, Alexey Brodkin 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.
trivial comments only:
> diff --git a/drivers/net/ethernet/arc/arc_emac_main.c b/drivers/net/ethernet/arc/arc_emac_main.c
[]
> @@ -0,0 +1,956 @@
> +/*
> + * Copyright (C) 2004, 2007-2010, 2011-2013 Synopsys, Inc. (http://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 (Rev 5)
> + *
> + * Alexey Brodkin: June 2013
> + * -Upsteaming
[]
> + * Vineet Gupta: June 2011
> + * -Issues when working with 64b cache line size
[]
> + * Vineet Gupta: May 2010
> + * -Reduced foot-print of the main ISR (handling for error cases moved out
[]
> + * Vineet Gupta: Nov 2009
> + * -Unified NAPI and Non-NAPI Code.
[]
> + * Vineet Gupta: Nov 2009
[]
> + * Amit Bhor, Sameer Dhavale: 2004
Does the internal changelog add anything useful?
> +static int arc_emac_poll(struct napi_struct *napi, int budget)
> +{
> + struct net_device *net_dev = napi->dev;
> + struct arc_emac_priv *priv = netdev_priv(net_dev);
> + struct sk_buff *skb, *skbnew;
> + unsigned int i, loop, len, info, work_done = 0;
> +
> + /* 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++) {
> + i = (i + 1) & (RX_BD_NUM - 1);
> +
> + info = priv->rxbd[i].info;
> +
> + /* BD contains a packet for CPU to grab */
> + if (likely((info & OWN_MASK) == FOR_CPU)) {
You could reduce indentation a level for all
the lines that follow by using
if (unlikely(!(...)
continue;
> + /* Get a new SKB from stack */
> + skbnew = netdev_alloc_skb(net_dev,
> + net_dev->mtu +
> + EMAC_BUFFER_PAD);
> +
> + if (!skbnew) {
> + netdev_err(net_dev, "Out of memory, "
> + "dropping packet\n");
OOM messages aren't particularly useful,
and coalesce format please...
[]
> + netdev_warn(net_dev,
> + "Rx chained, packet is larger than "
> + "device MTU (%d bytes)\n",
> + net_dev->mtu);
Same here.
> +static irqreturn_t arc_emac_intr(int irq, void *dev_instance)
> +{
> + struct net_device *net_dev = (struct net_device *)dev_instance;
Don't need to cast void *
netdev is a more common variable name
> + if (status & TXINT_MASK) {
> + unsigned int i, info;
> + struct sk_buff *skb;
[]
> + if (status & TXCH_MASK) {
> + priv->stats.tx_errors++;
> + priv->stats.tx_aborted_errors++;
> + netdev_err(priv->net_dev,
> + "Tx chaining err! txbd_dirty = %u\n",
> + priv->txbd_dirty);
You already have a local net_dev.
why not use that?
> + } else {
> + netdev_err(priv->net_dev,
> + "unknown err. Status reg is 0x%x\n",
> + status);
here too.
> +int arc_emac_open(struct net_device *net_dev)
> +{
> + struct arc_emac_priv *priv = netdev_priv(net_dev);
> + struct arc_emac_bd_t *bd;
> + struct sk_buff *skb;
> + int i;
> +
> + if (!priv->phy_node) {
> + netdev_err(net_dev, "arc_emac_open: phy device is absent\n");
If you really need the function name,
it's better to use "%s: ", __func__
> +int arc_emac_tx(struct sk_buff *skb, struct net_device *net_dev)
> +{
[]
> +tx_next_chunk:
> +
> + info = priv->txbd[priv->txbd_curr].info;
> + if (likely((info & OWN_MASK) == FOR_CPU)) {
if (unlikely(!(etc...)
return NETDEV_TX_BUSY;
and save an indent level
> +/**
> +int arc_emac_set_address(struct net_device *net_dev, void *p)
[]
> + EMAC_REG_SET(priv->reg_base_addr, R_ADDRH,
> + *(unsigned int *)&net_dev->dev_addr[4] & 0x0000ffff);
That doesn't seem endian friendly.
> +static int arc_emac_probe(struct platform_device *pdev)
> +{
[]
> + /* 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");
Coalesce formats please
> + 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");
[]
> + /* 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");
[]
Alexey Brodkin <[email protected]> :
[...]
> 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..f098a27
> --- /dev/null
> +++ b/drivers/net/ethernet/arc/arc_emac_main.c
> @@ -0,0 +1,956 @@
> +/*
> + * Copyright (C) 2004, 2007-2010, 2011-2013 Synopsys, Inc. (http://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 (Rev 5)
> + *
> + * Alexey Brodkin: June 2013
> + * -Upsteaming
> + * -Refactoring
> + * = Use device-tree/OF for configuration
> + * = Use libphy for phy management
> + * = Remove non-NAPI code sections
> + * = Remove ARC-specific BD management implementations
> + * = Add ethtool functionality
> + *
> + * Vineet Gupta: June 2011
> + * -Issues when working with 64b cache line size
> + * = BD rings point to aligned location in an internal buffer
> + * = added support for cache coherent BD Ring memory
> + *
> + * Vineet Gupta: May 2010
> + * -Reduced foot-print of the main ISR (handling for error cases moved out
> + * into a separate non-inline function).
> + * -Driver Tx path optimized for small packets (which fit into 1 BD = 2K).
> + * Any specifics for chaining are in a separate block of code.
> + *
> + * Vineet Gupta: Nov 2009
> + * -Unified NAPI and Non-NAPI Code.
> + * -API changes since 2.6.26 for making NAPI independent of netdevice
> + * -Cutting a few checks in main Rx poll routine
> + * -Tweaked NAPI implementation:
> + * In poll mode, Original driver would always start sweeping BD chain
> + * from BD-0 upto poll budget (40). And if it got over-budget it would
> + * drop reminder of packets.
> + * Instead now we remember last BD polled and in next
> + * cycle, we resume from next BD onwards. That way in case of over-budget
> + * no packet needs to be dropped.
> + *
> + * Vineet Gupta: Nov 2009
> + * -Rewrote the driver register access macros so that multiple accesses
> + * in same function use "anchor" reg to save the base addr causing
> + * shorter instructions
The kernel has been using git for some time: even if you don't remove this
stuff, you shouldn't add more of it.
> + *
> + * Amit Bhor, Sameer Dhavale: 2004
> + */
> +
> +#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_regs.h"
> +#include "arc_emac_mdio.h"
> +
> +#define DRV_NAME "arc_emac"
> +#define DRV_VERSION "2.0"
> +
> +/* Transmission timeout */
> +#define TX_TIMEOUT (400*HZ/1000)
> +
> +#define NAPI_WEIGHT 40 /* Workload for NAPI */
ARC_EMAC_NAPI_WEIGHT ?
> +
> +#define TX_FIFO_SIZE 0x800 /* 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;
> + void *data;
Why no char * ?
It's skb->data after all.
[...]
> +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,
.get_settings = arc_emac_get_settings,
.set_settings = arc_emac_set_settings,
.get_drvinfo = arc_emac_get_drvinfo,
.get_link = ethtool_op_get_link,
> +};
> +
> +static int arc_emac_poll(struct napi_struct *napi, int budget)
> +{
> + struct net_device *net_dev = napi->dev;
> + struct arc_emac_priv *priv = netdev_priv(net_dev);
> + struct sk_buff *skb, *skbnew;
> + unsigned int i, loop, len, info, work_done = 0;
You may reduce the scope of several variables.
> +
> + /* 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++) {
> + i = (i + 1) & (RX_BD_NUM - 1);
> +
> + info = priv->rxbd[i].info;
> +
> + /* BD contains a packet for CPU to grab */
> + if (likely((info & OWN_MASK) == FOR_CPU)) {
if unlikely continue / break and win an extra tab level
> +
> + /* Make a note that we saw a packet at this BD.
> + * So next time, driver starts from this + 1
> + */
> + priv->last_rx_bd = i;
> +
> + /* Packet fits in one BD (Non Fragmented) */
> + if (likely((info & (FRST_MASK | LAST_MASK)) ==
> + (FRST_MASK | LAST_MASK))) {
You may #define (FRST_MASK | LAST_MASK)
> +
> + len = info & LEN_MASK;
> + priv->stats.rx_packets++;
> + priv->stats.rx_bytes += len;
> + skb = priv->rx_skbuff[i];
> +
> + /* Get a new SKB from stack */
> + skbnew = netdev_alloc_skb(net_dev,
> + net_dev->mtu +
> + EMAC_BUFFER_PAD);
> +
> + if (!skbnew) {
> + netdev_err(net_dev, "Out of memory, "
> + "dropping packet\n");
Rate limit or do nothing.
> +
> + /* Return buffer to EMAC */
> + priv->rxbd[i].info = FOR_EMAC |
> + (net_dev->mtu + EMAC_BUFFER_PAD);
> + priv->stats.rx_dropped++;
> + continue;
> + }
> +
> + /* Actually preparing the BD for next cycle
> + * IP header align, eth is 14 bytes
> + */
> + skb_reserve(skbnew, 2);
netdev_alloc_skb_ip_align
> + priv->rx_skbuff[i] = skbnew;
> +
> + priv->rxbd[i].data = skbnew->data;
> + priv->rxbd[i].info = FOR_EMAC |
> + (net_dev->mtu + EMAC_BUFFER_PAD);
> +
> + /* Prepare arrived pkt for delivery to stack */
> + dma_map_single(&net_dev->dev, (void *)skb->data,
> + len, DMA_FROM_DEVICE);
dma_map_single may fail.
Speaking of it, where are dma_unmap and dma_sync ?
[...]
> +/**
> + * arc_emac_intr - Global interrupt handler for EMAC.
> + * @irq: irq number.
> + * @net_dev: net_device pointer.
> + *
> + * 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 *net_dev = (struct net_device *)dev_instance;
Useless cast from void *
> + struct arc_emac_priv *priv = netdev_priv(net_dev);
> + unsigned int status;
> +
> + /* Read STATUS register from EMAC */
> + status = EMAC_REG_GET(priv->reg_base_addr, R_STATUS);
> +
> + /* Mask all bits except "MDIO complete" */
> + status &= ~MDIO_MASK;
> +
> + /* To reset any bit in STATUS register we need to write "1" in
> + * corresponding bit. That's why we write only masked bits back.
> + */
> + EMAC_REG_SET(priv->reg_base_addr, R_STATUS, status);
> +
> + if (likely(status & (RXINT_MASK | TXINT_MASK))) {
> + if (status & RXINT_MASK) {
> + if (likely(napi_schedule_prep(&priv->napi))) {
> + EMAC_REG_CLR(priv->reg_base_addr, R_ENABLE,
> + RXINT_MASK);
> + __napi_schedule(&priv->napi);
> + }
> + }
> + if (status & TXINT_MASK) {
> + unsigned int i, info;
> + struct sk_buff *skb;
> +
> + for (i = 0; i < TX_BD_NUM; i++) {
> + info = priv->txbd[priv->txbd_dirty].info;
> +
> + if (info & (DROP | DEFR | LTCL | UFLO))
> + netdev_warn(net_dev,
> + "add Tx errors to stats\n");
> +
> + if ((info & FOR_EMAC) ||
> + !priv->txbd[priv->txbd_dirty].data)
if ((info & FOR_EMAC) ||
!priv->txbd[priv->txbd_dirty].data)
> + break;
> +
> + if (info & LAST_MASK) {
> + skb = priv->tx_skbuff[priv->txbd_dirty];
> + priv->stats.tx_packets++;
> + priv->stats.tx_bytes += skb->len;
> +
> + /* return the sk_buff to system */
> + dev_kfree_skb_irq(skb);
> + }
> + priv->txbd[priv->txbd_dirty].data = 0x0;
> + priv->txbd[priv->txbd_dirty].info = 0x0;
> + priv->txbd_dirty = (priv->txbd_dirty + 1) %
> + TX_BD_NUM;
> + }
> + }
> + } else {
> + if (status & ERR_MASK) {
> + /* MSER/RXCR/RXFR/RXFL interrupt fires on corresponding
> + * 8-bit error counter overrun.
> + * Because of this fact we add 256 items each time
> + * overrun interrupt happens.
> + */
Please use a local &priv->stats variable.
> +
> + if (status & TXCH_MASK) {
> + priv->stats.tx_errors++;
> + priv->stats.tx_aborted_errors++;
> + netdev_err(priv->net_dev,
> + "Tx chaining err! txbd_dirty = %u\n",
> + priv->txbd_dirty);
> + } else if (status & MSER_MASK) {
> + priv->stats.rx_missed_errors += 255;
> + priv->stats.rx_errors += 255;
> + } else if (status & RXCR_MASK) {
> + priv->stats.rx_crc_errors += 255;
> + priv->stats.rx_errors += 255;
> + } else if (status & RXFR_MASK) {
> + priv->stats.rx_frame_errors += 255;
> + priv->stats.rx_errors += 255;
> + } else if (status & RXFL_MASK) {
> + priv->stats.rx_over_errors += 255;
> + priv->stats.rx_errors += 255;
> + } else {
> + netdev_err(priv->net_dev,
> + "unknown err. Status reg is 0x%x\n",
> + status);
> + }
> + }
> + }
> + return IRQ_HANDLED;
> +}
> +
> +/**
> + * arc_emac_open - Open the network device.
> + * @net_dev: 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.
> + */
> +int arc_emac_open(struct net_device *net_dev)
static
> +{
> + struct arc_emac_priv *priv = netdev_priv(net_dev);
> + struct arc_emac_bd_t *bd;
> + struct sk_buff *skb;
> + int i;
> +
> + if (!priv->phy_node) {
> + netdev_err(net_dev, "arc_emac_open: phy device is absent\n");
> + return -ENODEV;
> + }
> +
> + priv->phy_dev = of_phy_connect(net_dev, priv->phy_node,
> + arc_emac_adjust_link, 0,
> + PHY_INTERFACE_MODE_MII);
> +
> + if (!priv->phy_dev) {
> + netdev_err(net_dev, "of_phy_connect() failed\n");
> + return -ENODEV;
> + }
Is there a reason why it could not be done in probe and thus save
some !priv->phy_dev checks ?
> +
> + netdev_info(net_dev, "connected to %s phy with id 0x%x\n",
> + priv->phy_dev->drv->name, priv->phy_dev->phy_id);
> +
> + priv->phy_dev->autoneg = AUTONEG_ENABLE;
> + priv->phy_dev->speed = 0;
> + priv->phy_dev->duplex = 0;
> +
> + /* We support only up-to 100mbps speeds */
> + priv->phy_dev->advertising = priv->phy_dev->supported;
> + priv->phy_dev->advertising &= PHY_BASIC_FEATURES;
> + priv->phy_dev->advertising |= ADVERTISED_Autoneg;
I'd go for a local priv->phy_dev.
> +
> + /* Restart auto negotiation */
> + phy_start_aneg(priv->phy_dev);
> +
> + netif_start_queue(net_dev);
No need to rush. Please finish software init.
> +
> + /* Allocate and set buffers for Rx BD's */
> + bd = priv->rxbd;
> + for (i = 0; i < RX_BD_NUM; i++) {
> + skb = netdev_alloc_skb(net_dev, net_dev->mtu + EMAC_BUFFER_PAD);
Missing NULL check.
> +
> + /* IP header Alignment (14 byte Ethernet header) */
> + skb_reserve(skb, 2);
netdev_alloc_skb_ip_align
[...]
> +int arc_emac_stop(struct net_device *net_dev)
static.
> +{
> + struct arc_emac_priv *priv = netdev_priv(net_dev);
> +
> + napi_disable(&priv->napi);
> + netif_stop_queue(net_dev);
> +
> + /* Disable interrupts */
> + EMAC_REG_CLR(priv->reg_base_addr, R_ENABLE,
> + TXINT_MASK | /* Tx interrupt */
> + RXINT_MASK | /* Rx interrupt */
> + ERR_MASK | /* Error interrupt */
> + TXCH_MASK); /* Transmit chaining error interrupt */
Useless comments.
> +
> + if (priv->phy_dev)
> + phy_disconnect(priv->phy_dev);
arc_emac_open succeeded: priv->phy_dev can't be NULL.
> +
> + priv->phy_dev = NULL;
> +
> + /* Disable EMAC */
> + EMAC_REG_CLR(priv->reg_base_addr, R_CTRL, EN_MASK);
> +
> + return 0;
> +}
> +
> +/**
> + * arc_emac_stats - Get system network statistics.
> + * @net_dev: Pointer to net_device structure.
> + *
> + * Returns the address of the device statistics structure.
> + * Statistics are updated in interrupt handler.
> + */
> +struct net_device_stats *arc_emac_stats(struct net_device *net_dev)
> +{
> + unsigned long miss, rxerr, rxfram, rxcrc, rxoflow;
> + struct arc_emac_priv *priv = netdev_priv(net_dev);
struct net_device_stats *stats = &priv->stats;
[...]
> +int arc_emac_tx(struct sk_buff *skb, struct net_device *net_dev)
static
> +{
> + int len, bitmask;
> + unsigned int info;
> + char *pkt;
> + struct arc_emac_priv *priv = netdev_priv(net_dev);
> +
> + len = skb->len < ETH_ZLEN ? ETH_ZLEN : skb->len;
> + pkt = skb->data;
> + net_dev->trans_start = jiffies;
> +
> + dma_map_single(&net_dev->dev, (void *)pkt, len, DMA_TO_DEVICE);
dma_map_single may fail.
[...]
> +int arc_emac_set_address(struct net_device *net_dev, void *p)
static
[...]
> +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,
Please use tabs before "=".
[...]
> +static int arc_emac_probe(struct platform_device *pdev)
[...]
> + priv->rxbd = (struct arc_emac_bd_t *)dmam_alloc_coherent(&pdev->dev,
> + RX_RING_SZ + TX_RING_SZ,
> + &priv->rxbd_dma_hdl, GFP_KERNEL);
Cast from void *.
[...]
> +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,
> + },
Excess tab.
[...]
> 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..7d13dd5
> --- /dev/null
> +++ b/drivers/net/ethernet/arc/arc_emac_mdio.c
[...]
> +static int arc_mdio_complete_wait(struct arc_mdio_priv *priv)
> +{
> + unsigned int status;
Excess scope.
> + unsigned int count = (ARC_MDIO_COMPLETE_POLL_COUNT * 40);
> +
> + while (1) {
> + /* Read STATUS register from EMAC */
> + status = EMAC_REG_GET(priv->reg_base_addr, R_STATUS);
Useless comment.
> +
> + /* Mask "MDIO complete" bit */
> + status &= MDIO_MASK;
> +
> + if (status) {
> + /* Reset "MDIO complete" flag */
> + EMAC_REG_SET(priv->reg_base_addr, R_STATUS, status);
> + break;
return 0;
> + }
> +
> + /* Make sure we never get into infinite loop */
> + if (count-- == 0) {
KISS: use a for loop ?
> + WARN_ON(1);
> + return -ETIMEDOUT;
> + }
> + msleep(25);
> + }
> + return 0;
> +}
[...]
> +static int arc_mdio_read(struct mii_bus *bus, int phy_addr, int reg_num)
> +{
> + int error;
> + unsigned int value;
> + struct arc_mdio_priv *priv = bus->priv;
Revert the xmas tree.
[...]
> +int arc_mdio_probe(struct device_node *dev_node, struct arc_mdio_priv *priv)
> +{
> + struct mii_bus *bus;
> + int error;
> +
> + bus = mdiobus_alloc();
> + if (!bus) {
> + error = -ENOMEM;
> + goto cleanup;
Nothing needs to be cleaned up.
[...]
> diff --git a/drivers/net/ethernet/arc/arc_emac_mdio.h b/drivers/net/ethernet/arc/arc_emac_mdio.h
> new file mode 100644
> index 0000000..954241e
> --- /dev/null
> +++ b/drivers/net/ethernet/arc/arc_emac_mdio.h
> @@ -0,0 +1,22 @@
> +/*
> + * Copyright (C) 2004, 2007-2010, 2011-2013 Synopsys, Inc. (http://www.synopsys.com)
> + *
> + * Definitions for MDIO of ARC EMAC device driver
> + */
> +
> +#ifndef ARC_MDIO_H
> +#define ARC_MDIO_H
> +
> +#include <linux/device.h>
> +#include <linux/phy.h>
> +
> +struct arc_mdio_priv {
> + struct mii_bus *bus;
> + struct device *dev;
> + void __iomem *reg_base_addr; /* MAC registers base address */
> +};
Overengineered ?
[...]
> diff --git a/drivers/net/ethernet/arc/arc_emac_regs.h b/drivers/net/ethernet/arc/arc_emac_regs.h
> new file mode 100644
> index 0000000..e4c8d73
> --- /dev/null
> +++ b/drivers/net/ethernet/arc/arc_emac_regs.h
[...]
> +#define EMAC_REG_SET(reg_base_addr, reg, val) \
> + iowrite32((val), reg_base_addr + reg * sizeof(int))
> +
> +#define EMAC_REG_GET(reg_base_addr, reg) \
> + ioread32(reg_base_addr + reg * sizeof(int))
May use real non-caps functions.
> +
> +#define EMAC_REG_OR(b, r, v) EMAC_REG_SET(b, r, EMAC_REG_GET(b, r) | (v))
> +#define EMAC_REG_CLR(b, r, v) EMAC_REG_SET(b, r, EMAC_REG_GET(b, r) & ~(v))
May use real non-caps functions.
Go ahead.
--
Ueimor
On 06/08/2013 04:49 PM, Alexey Brodkin wrote:
> >> + if (!priv->phy_node) {
> >> + dev_err(&pdev->dev, "failed to retrieve phy description "
> >> + "from device tree\n");
> >
> > Coalesce formats please
>
> Could you please clarify how should I format lines in question?
> I'm a bit lost here.
Don't break the string - even if line becomes > 80 chars. This is to help with
grep and such.
-Vineet
On 06/07/2013 10:13 PM, Joe Perches wrote:
> On Fri, 2013-06-07 at 19:07 +0400, Alexey Brodkin 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.
>
> trivial comments only:
>
>> diff --git a/drivers/net/ethernet/arc/arc_emac_main.c
b/drivers/net/ethernet/arc/arc_emac_main.c
>> + * Alexey Brodkin: June 2013
>> + * -Upsteaming
> []
>> + * Vineet Gupta: June 2011
>> + * -Issues when working with 64b cache line size
> []
>> + * Vineet Gupta: May 2010
>> + * -Reduced foot-print of the main ISR (handling for error cases
moved out
> []
>> + * Vineet Gupta: Nov 2009
>> + * -Unified NAPI and Non-NAPI Code.
> []
>> + * Vineet Gupta: Nov 2009
> []
>> + * Amit Bhor, Sameer Dhavale: 2004
>
> Does the internal changelog add anything useful?
Well, I think at this point it's more of honoring people who contributed
into this driver through past years.
I saw this kind of changelogs in other drivers like "arc_uart" that was
recently upstreamed:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/tty/serial/arc_uart.c
So if I still want to name contributor should I just list their names?
>> +static int arc_emac_poll(struct napi_struct *napi, int budget)
>> +{
>> + struct net_device *net_dev = napi->dev;
>> + struct arc_emac_priv *priv = netdev_priv(net_dev);
>> + struct sk_buff *skb, *skbnew;
>> + unsigned int i, loop, len, info, work_done = 0;
>> +
>> + /* 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++) {
>> + i = (i + 1) & (RX_BD_NUM - 1);
>> +
>> + info = priv->rxbd[i].info;
>> +
>> + /* BD contains a packet for CPU to grab */
>> + if (likely((info & OWN_MASK) == FOR_CPU)) {
>
> You could reduce indentation a level for all
> the lines that follow by using
> if (unlikely(!(...)
> continue;
To be honest I thought about doing it, but personally I don't like
inverted logic - I'd say it makes code logic not that straight-forward
for understanding. Still it might be only my feeling and as I've got at
last to advises to act so - I'll definitely follow your proposal.
>> + /* Get a new SKB from stack */
>> + skbnew = netdev_alloc_skb(net_dev,
>> + net_dev->mtu +
>> + EMAC_BUFFER_PAD);
>> +
>> + if (!skbnew) {
>> + netdev_err(net_dev, "Out of memory, "
>> + "dropping packet\n");
>
> OOM messages aren't particularly useful,
> and coalesce format please...
I noticed that some drivers don't print error messages for this kind of
errors. My intention was to make possible problem troubleshooting a bit
easier. But if it is not supposed to print "standard" errors in console
but simply return proper error code from the function - I'm fine with it
- makes code smaller)
>> +static irqreturn_t arc_emac_intr(int irq, void *dev_instance)
>> +{
>> + struct net_device *net_dev = (struct net_device *)dev_instance;
>
> Don't need to cast void *
> netdev is a more common variable name
Good point - thanks.
>> + if (status & TXINT_MASK) {
>> + unsigned int i, info;
>> + struct sk_buff *skb;
> []
>> + if (status & TXCH_MASK) {
>> + priv->stats.tx_errors++;
>> + priv->stats.tx_aborted_errors++;
>> + netdev_err(priv->net_dev,
>> + "Tx chaining err! txbd_dirty = %u\n",
>> + priv->txbd_dirty);
>
> You already have a local net_dev.
> why not use that?
Good point - overengineered stuff)
>> +int arc_emac_open(struct net_device *net_dev)
>> +{
>> + struct arc_emac_priv *priv = netdev_priv(net_dev);
>> + struct arc_emac_bd_t *bd;
>> + struct sk_buff *skb;
>> + int i;
>> +
>> + if (!priv->phy_node) {
>> + netdev_err(net_dev, "arc_emac_open: phy device is absent\n");
>
> If you really need the function name,
> it's better to use "%s: ", __func__
Reminder from previous "printk". Will clean-up.
>> +int arc_emac_tx(struct sk_buff *skb, struct net_device *net_dev)
>> +{
> []
>> +tx_next_chunk:
>> +
>> + info = priv->txbd[priv->txbd_curr].info;
>> + if (likely((info & OWN_MASK) == FOR_CPU)) {
>
> if (unlikely(!(etc...)
> return NETDEV_TX_BUSY;
>
> and save an indent level
Same comment as above - will invert logic.
>> +/**
>
>> +int arc_emac_set_address(struct net_device *net_dev, void *p)
> []
>> + EMAC_REG_SET(priv->reg_base_addr, R_ADDRH,
>> + *(unsigned int *)&net_dev->dev_addr[4] & 0x0000ffff);
>
> That doesn't seem endian friendly.
Right. Need to revisit this one.
>> +static int arc_emac_probe(struct platform_device *pdev)
>> +{
> []
>> + /* 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");
>
> Coalesce formats please
Could you please clarify how should I format lines in question?
I'm a bit lost here.
>> + 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");
> []
>> + /* 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");
> []
-Alexey
On 06/08/2013 12:33 AM, Francois Romieu wrote:
> Alexey Brodkin <[email protected]> :
[]
>> + * Vineet Gupta: Nov 2009
>> + * -Rewrote the driver register access macros so that multiple
accesses
>> + * in same function use "anchor" reg to save the base addr causing
>> + * shorter instructions
>
> The kernel has been using git for some time: even if you don't remove
this
> stuff, you shouldn't add more of it.
As replied to Joe I just want to name people contributed in this driver.
What is a appropriate way to do it?
>> +#define NAPI_WEIGHT 40 /* Workload for NAPI */
>
> ARC_EMAC_NAPI_WEIGHT ?
If it makes sense I may rename this one. I didn't do it earlier just
because this symbol is not exposed to anything outside this driver so
short and might be common name shouldn't hurt.
>> +struct arc_emac_bd_t {
>> + unsigned int info;
>> + void *data;
>
> Why no char * ?
>
> It's skb->data after all.
Makes sense - will correct.
> [...]
>> +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,
>
> .get_settings = arc_emac_get_settings,
> .set_settings = arc_emac_set_settings,
> .get_drvinfo = arc_emac_get_drvinfo,
> .get_link = ethtool_op_get_link,
Good point. Thanks.
>> +};
>> +
>> +static int arc_emac_poll(struct napi_struct *napi, int budget)
>> +{
>> + struct net_device *net_dev = napi->dev;
>> + struct arc_emac_priv *priv = netdev_priv(net_dev);
>> + struct sk_buff *skb, *skbnew;
>> + unsigned int i, loop, len, info, work_done = 0;
>
> You may reduce the scope of several variables.
Correct.
>> +
>> + /* Make a note that we saw a packet at this BD.
>> + * So next time, driver starts from this + 1
>> + */
>> + priv->last_rx_bd = i;
>> +
>> + /* Packet fits in one BD (Non Fragmented) */
>> + if (likely((info & (FRST_MASK | LAST_MASK)) ==
>> + (FRST_MASK | LAST_MASK))) {
>
> You may #define (FRST_MASK | LAST_MASK)
This combo is used in only 2 places so is it worth to introduce another
define? With these (FRST_MASK | LAST_MASK) I suppose reader will
understand that these are 2 separate bits. But still it might be just my
vision and if #define (FRST_MASK | LAST_MASK) looks better then I'll add
it immediately.
>> +
>> + len = info & LEN_MASK;
>> + priv->stats.rx_packets++;
>> + priv->stats.rx_bytes += len;
>> + skb = priv->rx_skbuff[i];
>> +
>> + /* Get a new SKB from stack */
>> + skbnew = netdev_alloc_skb(net_dev,
>> + net_dev->mtu +
>> + EMAC_BUFFER_PAD);
>> +
>> + if (!skbnew) {
>> + netdev_err(net_dev, "Out of memory, "
>> + "dropping packet\n");
>
> Rate limit or do nothing.
Not clear what do you mean. Could you please clarify?
>> +
>> + /* Return buffer to EMAC */
>> + priv->rxbd[i].info = FOR_EMAC |
>> + (net_dev->mtu + EMAC_BUFFER_PAD);
>> + priv->stats.rx_dropped++;
>> + continue;
>> + }
>> +
>> + /* Actually preparing the BD for next cycle
>> + * IP header align, eth is 14 bytes
>> + */
>> + skb_reserve(skbnew, 2);
>
> netdev_alloc_skb_ip_align
Thanks, will use this one instead.
>> + priv->rx_skbuff[i] = skbnew;
>> +
>> + priv->rxbd[i].data = skbnew->data;
>> + priv->rxbd[i].info = FOR_EMAC |
>> + (net_dev->mtu + EMAC_BUFFER_PAD);
>> +
>> + /* Prepare arrived pkt for delivery to stack */
>> + dma_map_single(&net_dev->dev, (void *)skb->data,
>> + len, DMA_FROM_DEVICE);
>
> dma_map_single may fail.
Correct. Will add test of returned value.
> Speaking of it, where are dma_unmap and dma_sync ?
Oops, this is something I need to look at a bit now)
> [...]
>> +/**
>> + * arc_emac_intr - Global interrupt handler for EMAC.
>> + * @irq: irq number.
>> + * @net_dev: net_device pointer.
>> + *
>> + * 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 *net_dev = (struct net_device *)dev_instance;
>
> Useless cast from void *
Agree.
>> + struct arc_emac_priv *priv = netdev_priv(net_dev);
>> + unsigned int status;
>> +
>> + /* Read STATUS register from EMAC */
>> + status = EMAC_REG_GET(priv->reg_base_addr, R_STATUS);
>> +
>> + /* Mask all bits except "MDIO complete" */
>> + status &= ~MDIO_MASK;
>> +
>> + /* To reset any bit in STATUS register we need to write "1" in
>> + * corresponding bit. That's why we write only masked bits back.
>> + */
>> + EMAC_REG_SET(priv->reg_base_addr, R_STATUS, status);
>> +
>> + if (likely(status & (RXINT_MASK | TXINT_MASK))) {
>> + if (status & RXINT_MASK) {
>> + if (likely(napi_schedule_prep(&priv->napi))) {
>> + EMAC_REG_CLR(priv->reg_base_addr, R_ENABLE,
>> + RXINT_MASK);
>> + __napi_schedule(&priv->napi);
>> + }
>> + }
>> + if (status & TXINT_MASK) {
>> + unsigned int i, info;
>> + struct sk_buff *skb;
>> +
>> + for (i = 0; i < TX_BD_NUM; i++) {
>> + info = priv->txbd[priv->txbd_dirty].info;
>> +
>> + if (info & (DROP | DEFR | LTCL | UFLO))
>> + netdev_warn(net_dev,
>> + "add Tx errors to stats\n");
>> +
>> + if ((info & FOR_EMAC) ||
>> + !priv->txbd[priv->txbd_dirty].data)
>
> if ((info & FOR_EMAC) ||
> !priv->txbd[priv->txbd_dirty].data)
>
Incorrect indentation, I see this now.
>> + break;
>> +
>> + if (info & LAST_MASK) {
>> + skb = priv->tx_skbuff[priv->txbd_dirty];
>> + priv->stats.tx_packets++;
>> + priv->stats.tx_bytes += skb->len;
>> +
>> + /* return the sk_buff to system */
>> + dev_kfree_skb_irq(skb);
>> + }
>> + priv->txbd[priv->txbd_dirty].data = 0x0;
>> + priv->txbd[priv->txbd_dirty].info = 0x0;
>> + priv->txbd_dirty = (priv->txbd_dirty + 1) %
>> + TX_BD_NUM;
>> + }
>> + }
>> + } else {
>> + if (status & ERR_MASK) {
>> + /* MSER/RXCR/RXFR/RXFL interrupt fires on corresponding
>> + * 8-bit error counter overrun.
>> + * Because of this fact we add 256 items each time
>> + * overrun interrupt happens.
>> + */
>
> Please use a local &priv->stats variable.
Ok, makes sense.
>> +
>> + if (status & TXCH_MASK) {
>> + priv->stats.tx_errors++;
>> + priv->stats.tx_aborted_errors++;
>> + netdev_err(priv->net_dev,
>> + "Tx chaining err! txbd_dirty = %u\n",
>> + priv->txbd_dirty);
>> + } else if (status & MSER_MASK) {
>> + priv->stats.rx_missed_errors += 255;
>> + priv->stats.rx_errors += 255;
>> + } else if (status & RXCR_MASK) {
>> + priv->stats.rx_crc_errors += 255;
>> + priv->stats.rx_errors += 255;
>> + } else if (status & RXFR_MASK) {
>> + priv->stats.rx_frame_errors += 255;
>> + priv->stats.rx_errors += 255;
>> + } else if (status & RXFL_MASK) {
>> + priv->stats.rx_over_errors += 255;
>> + priv->stats.rx_errors += 255;
>> + } else {
>> + netdev_err(priv->net_dev,
>> + "unknown err. Status reg is 0x%x\n",
>> + status);
>> + }
>> + }
>> + }
>> + return IRQ_HANDLED;
>> +}
>> +
>> +/**
>> + * arc_emac_open - Open the network device.
>> + * @net_dev: 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.
>> + */
>> +int arc_emac_open(struct net_device *net_dev)
>
> static
>
Why didn't I put static for every other function? Will fix it now.
>> +{
>> + struct arc_emac_priv *priv = netdev_priv(net_dev);
>> + struct arc_emac_bd_t *bd;
>> + struct sk_buff *skb;
>> + int i;
>> +
>> + if (!priv->phy_node) {
>> + netdev_err(net_dev, "arc_emac_open: phy device is absent\n");
>> + return -ENODEV;
>> + }
>> +
>> + priv->phy_dev = of_phy_connect(net_dev, priv->phy_node,
>> + arc_emac_adjust_link, 0,
>> + PHY_INTERFACE_MODE_MII);
>> +
>> + if (!priv->phy_dev) {
>> + netdev_err(net_dev, "of_phy_connect() failed\n");
>> + return -ENODEV;
>> + }
>
> Is there a reason why it could not be done in probe and thus save
> some !priv->phy_dev checks ?
I think that I saw in couple of drivers phy connection is done in
"open". That's why I put mine here too. In general I think it's possible
to move this functionality in "probe" easily.
>> +
>> + netdev_info(net_dev, "connected to %s phy with id 0x%x\n",
>> + priv->phy_dev->drv->name, priv->phy_dev->phy_id);
>> +
>> + priv->phy_dev->autoneg = AUTONEG_ENABLE;
>> + priv->phy_dev->speed = 0;
>> + priv->phy_dev->duplex = 0;
>> +
>> + /* We support only up-to 100mbps speeds */
>> + priv->phy_dev->advertising = priv->phy_dev->supported;
>> + priv->phy_dev->advertising &= PHY_BASIC_FEATURES;
>> + priv->phy_dev->advertising |= ADVERTISED_Autoneg;
>
> I'd go for a local priv->phy_dev.
Makes sense.
>> +
>> + /* Restart auto negotiation */
>> + phy_start_aneg(priv->phy_dev);
>> +
>> + netif_start_queue(net_dev);
>
> No need to rush. Please finish software init.
Ok.
>> +
>> + /* Allocate and set buffers for Rx BD's */
>> + bd = priv->rxbd;
>> + for (i = 0; i < RX_BD_NUM; i++) {
>> + skb = netdev_alloc_skb(net_dev, net_dev->mtu + EMAC_BUFFER_PAD);
>
> Missing NULL check.
Correct. Will fix.
>> +{
>> + struct arc_emac_priv *priv = netdev_priv(net_dev);
>> +
>> + napi_disable(&priv->napi);
>> + netif_stop_queue(net_dev);
>> +
>> + /* Disable interrupts */
>> + EMAC_REG_CLR(priv->reg_base_addr, R_ENABLE,
>> + TXINT_MASK | /* Tx interrupt */
>> + RXINT_MASK | /* Rx interrupt */
>> + ERR_MASK | /* Error interrupt */
>> + TXCH_MASK); /* Transmit chaining error interrupt */
>
> Useless comments.
Why so? Is it clear that TXCH means "Transmit chaining error interrupt"?
Or those defines should just have comments where they are defined and
later just use them with no comments?
>
>> +
>> + if (priv->phy_dev)
>> + phy_disconnect(priv->phy_dev);
>
> arc_emac_open succeeded: priv->phy_dev can't be NULL.
Right. Useless check.
>> +{
>> + int len, bitmask;
>> + unsigned int info;
>> + char *pkt;
>> + struct arc_emac_priv *priv = netdev_priv(net_dev);
>> +
>> + len = skb->len < ETH_ZLEN ? ETH_ZLEN : skb->len;
>> + pkt = skb->data;
>> + net_dev->trans_start = jiffies;
>> +
>> + dma_map_single(&net_dev->dev, (void *)pkt, len, DMA_TO_DEVICE);
>
> dma_map_single may fail.
Check is needed I see.
>> +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,
>
> Please use tabs before "=".
Ok.
>> +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,
>> + },
>
> Excess tab.
Ok.
> [...]
>> 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..7d13dd5
>> --- /dev/null
>> +++ b/drivers/net/ethernet/arc/arc_emac_mdio.c
> [...]
>> +static int arc_mdio_complete_wait(struct arc_mdio_priv *priv)
>> +{
>> + unsigned int status;
>
> Excess scope.
Ok.
>> + unsigned int count = (ARC_MDIO_COMPLETE_POLL_COUNT * 40);
>> +
>> + while (1) {
>> + /* Read STATUS register from EMAC */
>> + status = EMAC_REG_GET(priv->reg_base_addr, R_STATUS);
>
> Useless comment.
Might be so.
>> +
>> + /* Mask "MDIO complete" bit */
>> + status &= MDIO_MASK;
>> +
>> + if (status) {
>> + /* Reset "MDIO complete" flag */
>> + EMAC_REG_SET(priv->reg_base_addr, R_STATUS, status);
>> + break;
>
> return 0;
I prefer 1 exit point. That's why I put here "break".
>> + }
>> +
>> + /* Make sure we never get into infinite loop */
>> + if (count-- == 0) {
>
> KISS: use a for loop ?
>
Good point. Indeed "for" fits better here.
>> + WARN_ON(1);
>> + return -ETIMEDOUT;
>> + }
>> + msleep(25);
>> + }
>> + return 0;
>> +}
> [...]
>> +static int arc_mdio_read(struct mii_bus *bus, int phy_addr, int
reg_num)
>> +{
>> + int error;
>> + unsigned int value;
>> + struct arc_mdio_priv *priv = bus->priv;
>
> Revert the xmas tree.
Not clear what does it mean? Could you please calrify?
> [...]
>> +int arc_mdio_probe(struct device_node *dev_node, struct
arc_mdio_priv *priv)
>> +{
>> + struct mii_bus *bus;
>> + int error;
>> +
>> + bus = mdiobus_alloc();
>> + if (!bus) {
>> + error = -ENOMEM;
>> + goto cleanup;
>
> Nothing needs to be cleaned up.
Seems like that's true )
> [...]
>> diff --git a/drivers/net/ethernet/arc/arc_emac_mdio.h
b/drivers/net/ethernet/arc/arc_emac_mdio.h
>> new file mode 100644
>> index 0000000..954241e
>> --- /dev/null
>> +++ b/drivers/net/ethernet/arc/arc_emac_mdio.h
>> @@ -0,0 +1,22 @@
>> +/*
>> + * Copyright (C) 2004, 2007-2010, 2011-2013 Synopsys, Inc.
(http://www.synopsys.com)
>> + *
>> + * Definitions for MDIO of ARC EMAC device driver
>> + */
>> +
>> +#ifndef ARC_MDIO_H
>> +#define ARC_MDIO_H
>> +
>> +#include <linux/device.h>
>> +#include <linux/phy.h>
>> +
>> +struct arc_mdio_priv {
>> + struct mii_bus *bus;
>> + struct device *dev;
>> + void __iomem *reg_base_addr; /* MAC registers base address */
>> +};
>
> Overengineered ?
Why so? Not clear, sorry.
> [...]
>> diff --git a/drivers/net/ethernet/arc/arc_emac_regs.h
b/drivers/net/ethernet/arc/arc_emac_regs.h
>> new file mode 100644
>> index 0000000..e4c8d73
>> --- /dev/null
>> +++ b/drivers/net/ethernet/arc/arc_emac_regs.h
> [...]
>> +#define EMAC_REG_SET(reg_base_addr, reg, val) \
>> + iowrite32((val), reg_base_addr + reg * sizeof(int))
>> +
>> +#define EMAC_REG_GET(reg_base_addr, reg) \
>> + ioread32(reg_base_addr + reg * sizeof(int))
>
> May use real non-caps functions.
Do you mean to use "io{read|write}32" directly without macro?
>> +
>> +#define EMAC_REG_OR(b, r, v) EMAC_REG_SET(b, r, EMAC_REG_GET(b, r)
| (v))
>> +#define EMAC_REG_CLR(b, r, v) EMAC_REG_SET(b, r, EMAC_REG_GET(b, r)
& ~(v))
>
> May use real non-caps functions.
>
> Go ahead.
>
Thanks a lot for this deep analisys.
-Alexey
On Fri, Jun 7, 2013 at 6:07 PM, Alexey Brodkin
<[email protected]> 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.
>
> This is based off of current Linus tree, build tested for x86.
> +++ b/drivers/net/ethernet/arc/arc_emac_main.c
> @@ -0,0 +1,956 @@
> +/*
> + * Copyright (C) 2004, 2007-2010, 2011-2013 Synopsys, Inc. (http://www.synopsys.com)
2004, 2007-2013 ?
> + * Driver for the ARC EMAC 10100 (Rev 5)
If you are going to upstream this, why do you need revision?
> + * Alexey Brodkin: June 2013
> + * -Upsteaming
It will be obvious from existence of your commit in the git tree.
> + * -Refactoring
> + * = Use device-tree/OF for configuration
> + * = Use libphy for phy management
> + * = Remove non-NAPI code sections
> + * = Remove ARC-specific BD management implementations
> + * = Add ethtool functionality
> + *
> + * Vineet Gupta: June 2011
> + * -Issues when working with 64b cache line size
> + * = BD rings point to aligned location in an internal buffer
> + * = added support for cache coherent BD Ring memory
> + *
> + * Vineet Gupta: May 2010
> + * -Reduced foot-print of the main ISR (handling for error cases moved out
> + * into a separate non-inline function).
> + * -Driver Tx path optimized for small packets (which fit into 1 BD = 2K).
> + * Any specifics for chaining are in a separate block of code.
> + *
> + * Vineet Gupta: Nov 2009
> + * -Unified NAPI and Non-NAPI Code.
> + * -API changes since 2.6.26 for making NAPI independent of netdevice
> + * -Cutting a few checks in main Rx poll routine
> + * -Tweaked NAPI implementation:
> + * In poll mode, Original driver would always start sweeping BD chain
> + * from BD-0 upto poll budget (40). And if it got over-budget it would
> + * drop reminder of packets.
> + * Instead now we remember last BD polled and in next
> + * cycle, we resume from next BD onwards. That way in case of over-budget
> + * no packet needs to be dropped.
> + *
> + * Vineet Gupta: Nov 2009
> + * -Rewrote the driver register access macros so that multiple accesses
> + * in same function use "anchor" reg to save the base addr causing
> + * shorter instructions
> + *
> + * Amit Bhor, Sameer Dhavale: 2004
I don't know if it's a good idea to keep changelog inside source file,
we have git commit messages for that. You may move this to the first
commit message if you want to keep history, and in future git will do
the job for you.
> + */
> +
> +#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_regs.h"
> +#include "arc_emac_mdio.h"
Since you are creating driver under its own folder why to keep the
prefixes in the filenames? I think regs.h and mdio.h would be enough.
> +/* Transmission timeout */
> +#define TX_TIMEOUT (400*HZ/1000)
(400 * HZ / 1000)
> +struct arc_emac_priv {
> + /* Pointers to BD rings - Device side */
What about documenting structure fields in the kerneldoc format on the
top of struct definition?
> + dma_addr_t rxbd_dma_hdl;
> + dma_addr_t txbd_dma_hdl;
Perhaps you may consider to reduce field names somethow. I don't know
why rxbd_dma and txbd_dma is not enough.
> + struct sk_buff *rx_skbuff[RX_BD_NUM];
> + struct sk_buff *tx_skbuff[TX_BD_NUM];
Ditto.
> + void __iomem *reg_base_addr;
Ditto.
For example, base or regs.
> +/**
> + * arc_emac_adjust_link - Adjust the PHY link speed/duplex.
> + * @net_dev: Pointer to the net_device structure.
> + *
> + * This function is called to change the speed and duplex setting after
> + * auto negotiation is done by the PHY.
> + */
> +static void arc_emac_adjust_link(struct net_device *net_dev)
> +{
> + struct arc_emac_priv *priv = netdev_priv(net_dev);
> + struct phy_device *phydev = priv->phy_dev;
> + unsigned int reg, status_change = 0;
> +
> + BUG_ON(!priv->phy_dev);
> +
> + if (phydev->link && (priv->old_speed != phydev->speed)) {
> + /* speed changed */
> + switch (phydev->speed) {
> + case SPEED_10:
> + case SPEED_100:
> + break;
> + default:
> + netdev_warn(net_dev, "Speed (%d) is not 10/100?\n",
> + phydev->speed);
> + break;
> + }
> + priv->old_speed = phydev->speed;
> + status_change = 1;
> + }
> +
> + if (phydev->link && (priv->old_duplex != phydev->duplex)) {
> + /* duplex mode changed */
> + reg = EMAC_REG_GET(priv->reg_base_addr, R_CTRL);
> +
> + if (DUPLEX_FULL == phydev->duplex)
> + reg |= ENFL_MASK;
> + else
> + reg &= ~ENFL_MASK;
> +
> + EMAC_REG_SET(priv->reg_base_addr, R_CTRL, reg);
> + priv->old_duplex = phydev->duplex;
> + status_change = 1;
> + }
> +
> + if (phydev->link != priv->old_link) {
> + /* link state changed */
> + if (!phydev->link) {
> + /* link went down */
> + priv->old_speed = 0;
> + priv->old_duplex = -1;
If you move this part to the top of function you may simplify conditions
if (priv->old_link && !phydev->link) {
/* link went down */
priv->old_speed = 0;
priv->old_duplex = -1;
priv->old_link = NULL;
phy_print_status(phydev);
return;
}
if (priv->old_speed != phydev->speed) {
...
> + }
> + priv->old_link = phydev->link;
> + status_change = 1;
> + }
> +
> + if (status_change)
> + phy_print_status(phydev);
> +}
> +static int arc_emac_poll(struct napi_struct *napi, int budget)
> +{
> + for (loop = 0; loop < RX_BD_NUM; loop++) {
> + i = (i + 1) & (RX_BD_NUM - 1);
i = (i + 1) % RX_BD_NUM;
> +static irqreturn_t arc_emac_intr(int irq, void *dev_instance)
> +{
> + if (likely(status & (RXINT_MASK | TXINT_MASK))) {
It dublicates following checks.
Instead of using else, you may check for opposite condition.
> + if (status & RXINT_MASK) {
> + }
> + if (status & TXINT_MASK) {
> + }
> + } else {
> + if (status & ERR_MASK) {
> + /* MSER/RXCR/RXFR/RXFL interrupt fires on corresponding
> + * 8-bit error counter overrun.
> + * Because of this fact we add 256 items each time
> + * overrun interrupt happens.
> + */
> +
> + if (status & TXCH_MASK) {
> + priv->stats.tx_errors++;
> + priv->stats.tx_aborted_errors++;
> + netdev_err(priv->net_dev,
> + "Tx chaining err! txbd_dirty = %u\n",
> + priv->txbd_dirty);
> + } else if (status & MSER_MASK) {
> + priv->stats.rx_missed_errors += 255;
> + priv->stats.rx_errors += 255;
> + } else if (status & RXCR_MASK) {
> + priv->stats.rx_crc_errors += 255;
> + priv->stats.rx_errors += 255;
> + } else if (status & RXFR_MASK) {
> + priv->stats.rx_frame_errors += 255;
> + priv->stats.rx_errors += 255;
Why 255 ?
> + } else if (status & RXFL_MASK) {
> + priv->stats.rx_over_errors += 255;
> + priv->stats.rx_errors += 255;
> +struct net_device_stats *arc_emac_stats(struct net_device *net_dev)
> +{
> + unsigned long miss, rxerr, rxfram, rxcrc, rxoflow;
> + struct arc_emac_priv *priv = netdev_priv(net_dev);
> +
> + rxerr = EMAC_REG_GET(priv->reg_base_addr, R_RXERR);
> + miss = EMAC_REG_GET(priv->reg_base_addr, R_MISS);
> +
> + rxcrc = (rxerr & 0xff);
> + rxfram = (rxerr >> 8 & 0xff);
> + rxoflow = (rxerr >> 16 & 0xff);
For what purpose you embraced those?
> +int arc_emac_tx(struct sk_buff *skb, struct net_device *net_dev)
> +{
> + int len, bitmask;
> + unsigned int info;
> + char *pkt;
> + struct arc_emac_priv *priv = netdev_priv(net_dev);
> +
> + len = skb->len < ETH_ZLEN ? ETH_ZLEN : skb->len;
len = max_t(..., ETH_ZLEN, skb->len);
> +tx_next_chunk:
> +
> + info = priv->txbd[priv->txbd_curr].info;
> + if (likely((info & OWN_MASK) == FOR_CPU)) {
> + } else {
> + return NETDEV_TX_BUSY;
What about to do this check first and return, and do the body unconditionally?
> + }
> +}
> +
> +static int arc_emac_probe(struct platform_device *pdev)
> +{
> + struct net_device *net_dev;
> + struct arc_emac_priv *priv;
> + int err;
> + unsigned int id, clock_frequency;
> + struct resource res_regs, res_irq;
> + const char *mac_addr = NULL;
It seems useless assignment.
> + priv->reg_base_addr = devm_ioremap_resource(&pdev->dev, &res_regs);
> + if (IS_ERR(priv->reg_base_addr)) {
> + dev_err(&pdev->dev, "failed to ioremap MAC registers\n");
No need to print this here.
> +++ b/drivers/net/ethernet/arc/arc_emac_mdio.c
> @@ -0,0 +1,170 @@
> +/*
> + * Copyright (C) 2004, 2007-2010, 2011-2013 Synopsys, Inc. (http://www.synopsys.com)
2007-2013?
> +static int arc_mdio_complete_wait(struct arc_mdio_priv *priv)
> +{
> + unsigned int status;
> + unsigned int count = (ARC_MDIO_COMPLETE_POLL_COUNT * 40);
> + /* Make sure we never get into infinite loop */
> + if (count-- == 0) {
> + WARN_ON(1);
Is it really requires WARN_ON?
> + return -ETIMEDOUT;
> + }
> + msleep(25);
Overall delay is up to 1000 ms, correct?
> +static int arc_mdio_write(struct mii_bus *bus, int phy_addr,
> + int reg_num, u16 value)
> +{
> + EMAC_REG_SET(priv->reg_base_addr, R_MDIO,
> + 0x50020000 | (phy_addr << 23) | (reg_num << 18) |
> + (value & 0xffff));
Useless 0xffff for u16 value.
> +++ b/drivers/net/ethernet/arc/arc_emac_mdio.h
> @@ -0,0 +1,22 @@
> +/*
> + * Copyright (C) 2004, 2007-2010, 2011-2013 Synopsys, Inc. (http://www.synopsys.com)
2007-2013
> +struct arc_mdio_priv {
> + struct mii_bus *bus;
> + struct device *dev;
> + void __iomem *reg_base_addr; /* MAC registers base address */
kerneldoc format?
> +++ b/drivers/net/ethernet/arc/arc_emac_regs.h
> @@ -0,0 +1,72 @@
> +/*
> + * Copyright (C) 2004, 2007-2010, 2011-2013 Synopsys, Inc. (http://www.synopsys.com)
2007-2013 ?
--
With Best Regards,
Andy Shevchenko
Alexey Brodkin <[email protected]> :
> On 06/08/2013 12:33 AM, Francois Romieu wrote:
> > Alexey Brodkin <[email protected]> :
[...]
> As replied to Joe I just want to name people contributed in this driver.
> What is a appropriate way to do it?
A polite way could be to see with contributors if it's ok for them to
appear in the (c) section.
[...]
> > You may #define (FRST_MASK | LAST_MASK)
>
> This combo is used in only 2 places so is it worth to introduce another
> define? With these (FRST_MASK | LAST_MASK) I suppose reader will
> understand that these are 2 separate bits. But still it might be just my
> vision and if #define (FRST_MASK | LAST_MASK) looks better then I'll add
> it immediately.
Your call. The #define only needs to appear in or near the function.
[...]
> >> + if (!skbnew) {
> >> + netdev_err(net_dev, "Out of memory, "
> >> + "dropping packet\n");
> >
> > Rate limit or do nothing.
>
> Not clear what do you mean. Could you please clarify ?
net_ratelimit() will prevent a storm of log messages. Actually I would
avoid the message completely.
[...]
> >> +{
> >> + struct arc_emac_priv *priv = netdev_priv(net_dev);
> >> +
> >> + napi_disable(&priv->napi);
> >> + netif_stop_queue(net_dev);
> >> +
> >> + /* Disable interrupts */
> >> + EMAC_REG_CLR(priv->reg_base_addr, R_ENABLE,
> >> + TXINT_MASK | /* Tx interrupt */
> >> + RXINT_MASK | /* Rx interrupt */
> >> + ERR_MASK | /* Error interrupt */
> >> + TXCH_MASK); /* Transmit chaining error interrupt */
> >
> > Useless comments.
>
> Is it clear that TXCH means "Transmit chaining error interrupt"?
ERR_TXCHAIN_MASK ?
> Or those defines should just have comments where they are defined and
> later just use them with no comments?
s/should have/may have/
Otherwise, yes.
[...]
> >> +static int arc_mdio_read(struct mii_bus *bus, int phy_addr, int
> reg_num)
> >> +{
> >> + int error;
> >> + unsigned int value;
> >> + struct arc_mdio_priv *priv = bus->priv;
> >
> > Revert the xmas tree.
>
> Not clear what does it mean? Could you please calrify?
struct arc_mdio_priv *priv = bus->priv;
unsigned int value;
int error;
[...]
> >> +struct arc_mdio_priv {
> >> + struct mii_bus *bus;
> >> + struct device *dev;
> >> + void __iomem *reg_base_addr; /* MAC registers base address */
> >> +};
> >
> > Overengineered ?
>
> Why so? Not clear, sorry.
Most of this struct is shared with the device private one. I am not
sure that they need to be separated.
[...]
> >> +#define EMAC_REG_SET(reg_base_addr, reg, val) \
> >> + iowrite32((val), reg_base_addr + reg * sizeof(int))
> >> +
> >> +#define EMAC_REG_GET(reg_base_addr, reg) \
> >> + ioread32(reg_base_addr + reg * sizeof(int))
> >
> > May use real non-caps functions.
>
> Do you mean to use "io{read|write}32" directly without macro?
Add your own arc_{r/w} functions and use the device private struct
pointer as one of their parameters (whence no void * parameter).
[...]
> Thanks a lot for this deep analisys.
Not that deep:
- arc_emac_tx should disable tx queue as soon as the tx ring is exhausted
- you may consider moving work from the irq handler to napi poll.
--
Ueimor