[PATCH v5 1/1] Driver for Beckhoff CX5020 EtherCAT master module.

[PATCH v5 1/1] Driver for Beckhoff CX5020 EtherCAT master module.

[Darek Marcinkiewicz]

This driver adds support for EtherCAT master module located on CCAT
FPGA found on Beckhoff CX series industrial PCs. The driver exposes
EtherCAT master as an ethernet interface.

EtherCAT is a filedbus protocol defined on top of ethernet and Beckhoff
CX5020 PCs come with built-in EtherCAT master module located on a FPGA,
which in turn is connected to a PCI bus.

Signed-off-by: Dariusz Marcinkiewicz <reksio@newterm.pl>
---
Changes from v4: 
  * incorporates Francois Romieu comments
  * fixes typo spotted by Jesper Brouer

Changes from v3:
  * some clarificatoins around buffer allocations

Changes from v2:
  * removed all checkpatch warnings
  * driver makes use of device rx fifo

Changes from v1:
  * added endianess annotation to descriptors' structures
  * killed checkpath warnings about string literals being split into multiple
    lines

 drivers/net/ethernet/Kconfig  |   11 +
 drivers/net/ethernet/Makefile |    1 +
 drivers/net/ethernet/ec_bhf.c |  740 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 752 insertions(+)
 create mode 100644 drivers/net/ethernet/ec_bhf.c

diff --git a/drivers/net/ethernet/Kconfig b/drivers/net/ethernet/Kconfig
index 39b26fe..a93ea15 100644
--- a/drivers/net/ethernet/Kconfig
+++ b/drivers/net/ethernet/Kconfig
@@ -35,6 +35,17 @@ source "drivers/net/ethernet/calxeda/Kconfig"
 source "drivers/net/ethernet/chelsio/Kconfig"
 source "drivers/net/ethernet/cirrus/Kconfig"
 source "drivers/net/ethernet/cisco/Kconfig"
+
+config CX_ECAT
+	tristate "Beckhoff CX5020 EtherCAT master support"
+	---help---
+	  Driver for EtherCAT master module located on CCAT FPGA
+	  that can be found on Beckhoff CX5020, and possibly other of CX
+	  Beckhoff CX series industrial PCs.
+
+	  To compile this driver as a module, choose M here. The module
+	  will be called ec_bhf.
+
 source "drivers/net/ethernet/davicom/Kconfig"
 
 config DNET
diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
index 545d0b3..35190e3 100644
--- a/drivers/net/ethernet/Makefile
+++ b/drivers/net/ethernet/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_NET_CALXEDA_XGMAC) += calxeda/
 obj-$(CONFIG_NET_VENDOR_CHELSIO) += chelsio/
 obj-$(CONFIG_NET_VENDOR_CIRRUS) += cirrus/
 obj-$(CONFIG_NET_VENDOR_CISCO) += cisco/
+obj-$(CONFIG_CX_ECAT) += ec_bhf.o
 obj-$(CONFIG_DM9000) += davicom/
 obj-$(CONFIG_DNET) += dnet.o
 obj-$(CONFIG_NET_VENDOR_DEC) += dec/
diff --git a/drivers/net/ethernet/ec_bhf.c b/drivers/net/ethernet/ec_bhf.c
new file mode 100644
index 0000000..8e78b8d
--- /dev/null
+++ b/drivers/net/ethernet/ec_bhf.c
@@ -0,0 +1,740 @@
+ /*
+ * drivers/net/ethernet/beckhoff/ec_bhf.c
+ *
+ * Copyright (C) 2014 Darek Marcinkiewicz <reksio@newterm.pl>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+/* This is a driver for EtherCAT master module present on CCAT FPGA.
+ * Those can be found on Bechhoff CX50xx industrial PCs.
+ */
+
+#if 0
+#define DEBUG
+#endif
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/pci.h>
+#include <linux/init.h>
+
+#include <linux/netdevice.h>
+#include <linux/etherdevice.h>
+#include <linux/ip.h>
+#include <linux/skbuff.h>
+#include <linux/hrtimer.h>
+#include <linux/interrupt.h>
+#include <linux/stat.h>
+
+#define TIMER_INTERVAL_NSEC	20000
+
+#define INFO_BLOCK_SIZE		0x10
+#define INFO_BLOCK_TYPE		0x0
+#define INFO_BLOCK_REV		0x2
+#define INFO_BLOCK_BLK_CNT	0x4
+#define INFO_BLOCK_TX_CHAN	0x4
+#define INFO_BLOCK_RX_CHAN	0x5
+#define INFO_BLOCK_OFFSET	0x8
+
+#define EC_MII_OFFSET		0x4
+#define EC_FIFO_OFFSET		0x8
+#define EC_MAC_OFFSET		0xc
+
+#define MAC_FRAME_ERR_CNT	0x0
+#define MAC_RX_ERR_CNT		0x1
+#define MAC_CRC_ERR_CNT		0x2
+#define MAC_LNK_LST_ERR_CNT	0x3
+#define MAC_TX_FRAME_CNT	0x10
+#define MAC_RX_FRAME_CNT	0x14
+#define MAC_TX_FIFO_LVL		0x20
+#define MAC_DROPPED_FRMS	0x28
+#define MAC_CONNECTED_CCAT_FLAG	0x78
+
+#define MII_MAC_ADDR		0x8
+#define MII_MAC_FILT_FLAG	0xe
+#define MII_LINK_STATUS		0xf
+
+#define FIFO_TX_REG		0x0
+#define FIFO_TX_RESET		0x8
+#define FIFO_RX_REG		0x10
+#define FIFO_RX_ADDR_VALID	(1u << 31)
+#define FIFO_RX_RESET		0x18
+
+#define DMA_CHAN_OFFSET		0x1000
+#define DMA_CHAN_SIZE		0x8
+
+static struct pci_device_id ids[] = {
+	{ PCI_DEVICE(0x15ec, 0x5000), },
+	{ 0, }
+};
+MODULE_DEVICE_TABLE(pci, ids);
+
+struct rx_header {
+#define RXHDR_NEXT_ADDR_MASK	0xffffffu
+#define RXHDR_NEXT_VALID	(1u << 31)
+	__le32 next;
+#define RXHDR_NEXT_RECV_FLAG	0x1
+	__le32 recv;
+#define RXHDR_LEN_MASK		0xfffu
+	__le16 len;
+	__le16 port;
+	__le32 reserved;
+	u8 timestamp[8];
+} __packed;
+
+#define PKT_PAYLOAD_SIZE	0x7e8
+struct rx_desc {
+	struct rx_header header;
+	u8 data[PKT_PAYLOAD_SIZE];
+	struct list_head node;
+} __packed;
+
+struct tx_header {
+	__le16 len;
+#define TX_HDR_PORT_0		0x1
+#define TX_HDR_PORT_1		0x2
+	u8 port;
+	u8 ts_enable;
+#define TX_HDR_SENT		0x1
+	__le32 sent;
+	u8 timestamp[8];
+} __packed;
+
+struct tx_desc {
+	struct tx_header header;
+	u8 data[PKT_PAYLOAD_SIZE];
+	struct list_head node;
+} __packed;
+
+#define FIFO_SIZE		64
+
+static long polling_frequency = TIMER_INTERVAL_NSEC;
+
+struct bhf_dma {
+	u8 *buf;
+	size_t len;
+	dma_addr_t buf_phys;
+
+	u8 *alloc;
+	size_t alloc_len;
+	dma_addr_t alloc_phys;
+};
+
+struct ec_bhf_priv {
+	struct net_device *net_dev;
+
+	struct pci_dev *dev;
+
+	void * __iomem io;
+	void * __iomem dma_io;
+
+	struct hrtimer hrtimer;
+
+	int tx_dma_chan;
+	int rx_dma_chan;
+	void * __iomem ec_io;
+	void * __iomem fifo_io;
+	void * __iomem mii_io;
+	void * __iomem mac_io;
+
+	struct bhf_dma rx_buf;
+	struct rx_desc *rx_desc;
+
+	struct list_head tx_desc_free;
+	struct list_head tx_desc_queue;
+	struct bhf_dma tx_buf;
+
+	u64 stat_rx_bytes;
+	u64 stat_tx_bytes;
+
+	spinlock_t lock;
+};
+
+#define PRIV_TO_DEV(priv) (&(priv)->dev->dev)
+
+#define ETHERCAT_MASTER_ID	0x14
+
+static void ec_bhf_print_status(struct ec_bhf_priv *priv)
+{
+	struct device *dev = PRIV_TO_DEV(priv);
+
+	dev_dbg(dev, "Frame error counter: %d\n",
+		 ioread8(priv->mac_io + MAC_FRAME_ERR_CNT));
+	dev_dbg(dev, "RX error counter: %d\n",
+		 ioread8(priv->mac_io + MAC_RX_ERR_CNT));
+	dev_dbg(dev, "CRC error counter: %d\n",
+		 ioread8(priv->mac_io + MAC_CRC_ERR_CNT));
+	dev_dbg(dev, "TX frame counter: %d\n",
+		 ioread32(priv->mac_io + MAC_TX_FRAME_CNT));
+	dev_dbg(dev, "RX frame counter: %d\n",
+		 ioread32(priv->mac_io + MAC_RX_FRAME_CNT));
+	dev_dbg(dev, "TX fifo level: %d\n",
+		 ioread8(priv->mac_io + MAC_TX_FIFO_LVL));
+	dev_dbg(dev, "Dropped frames: %d\n",
+		 ioread8(priv->mac_io + MAC_DROPPED_FRMS));
+	dev_dbg(dev, "Connected with CCAT slot: %d\n",
+		 ioread8(priv->mac_io + MAC_CONNECTED_CCAT_FLAG));
+	dev_dbg(dev, "Link status: %d\n",
+		 ioread8(priv->mii_io + MII_LINK_STATUS));
+}
+
+static void ec_bhf_reset(struct ec_bhf_priv *priv)
+{
+	iowrite8(0, priv->mac_io + MAC_FRAME_ERR_CNT);
+	iowrite8(0, priv->mac_io + MAC_RX_ERR_CNT);
+	iowrite8(0, priv->mac_io + MAC_CRC_ERR_CNT);
+	iowrite8(0, priv->mac_io + MAC_LNK_LST_ERR_CNT);
+	iowrite32(0, priv->mac_io + MAC_TX_FRAME_CNT);
+	iowrite32(0, priv->mac_io + MAC_RX_FRAME_CNT);
+	iowrite8(0, priv->mac_io + MAC_DROPPED_FRMS);
+
+	iowrite8(0, priv->fifo_io + FIFO_TX_RESET);
+	iowrite8(0, priv->fifo_io + FIFO_RX_RESET);
+
+	iowrite8(0, priv->mac_io + MAC_TX_FIFO_LVL);
+}
+
+static void ec_bhf_send_packet(struct ec_bhf_priv *priv, struct tx_desc *desc)
+{
+	u32 addr = (u8 *)desc - priv->tx_buf.buf;
+	u32 len = le16_to_cpu(desc->header.len) + sizeof(desc->header);
+
+	iowrite32((ALIGN(len, 8) << 24) | addr, priv->fifo_io + FIFO_TX_REG);
+
+	dev_dbg(PRIV_TO_DEV(priv), "Done sending packet\n");
+}
+
+static void ec_bhf_process_tx(struct ec_bhf_priv *priv)
+{
+	struct tx_desc *desc;
+	unsigned long flags;
+	int sent;
+
+	spin_lock_irqsave(&priv->lock, flags);
+
+	if (list_empty(&priv->tx_desc_queue))
+		goto out;
+
+	desc = list_first_entry(&priv->tx_desc_queue,
+				struct tx_desc, node);
+	sent = le32_to_cpu(desc->header.sent) & TX_HDR_SENT;
+	if (!sent)
+		goto out;
+
+	list_del(&desc->node);
+	list_add_tail(&desc->node, &priv->tx_desc_free);
+
+	if (netif_queue_stopped(priv->net_dev)) {
+		dev_info(PRIV_TO_DEV(priv), "Waking netif queue\n");
+		netif_wake_queue(priv->net_dev);
+	}
+
+	if (!list_empty(&priv->tx_desc_queue))
+		ec_bhf_send_packet(priv, list_first_entry(&priv->tx_desc_queue,
+							  struct tx_desc,
+							  node));
+out:
+	spin_unlock_irqrestore(&priv->lock, flags);
+}
+
+static int ec_bhf_pkt_received(struct rx_desc *desc)
+{
+	return le32_to_cpu(desc->header.recv) & RXHDR_NEXT_RECV_FLAG;
+}
+
+static void ec_bhf_add_rx_desc(struct ec_bhf_priv *priv, struct rx_desc *desc)
+{
+	iowrite32(FIFO_RX_ADDR_VALID | ((u8 *)(desc) - priv->rx_buf.buf),
+		  priv->fifo_io + FIFO_RX_REG);
+}
+
+static void ec_bhf_process_rx(struct ec_bhf_priv *priv)
+{
+	struct device *dev = PRIV_TO_DEV(priv);
+	struct rx_desc *desc = priv->rx_desc;
+
+	while (ec_bhf_pkt_received(desc)) {
+		int pkt_size = (le16_to_cpu(desc->header.len) &
+			       RXHDR_LEN_MASK) - sizeof(struct rx_header) - 4;
+		u8 *data = desc->data;
+		struct sk_buff *skb;
+
+		skb = netdev_alloc_skb_ip_align(priv->net_dev, pkt_size);
+		dev_dbg(dev, "Received packet, size: %d\n", pkt_size);
+
+		if (skb) {
+			memcpy(skb_put(skb, pkt_size), data, pkt_size);
+			skb->protocol = eth_type_trans(skb, priv->net_dev);
+			dev_dbg(dev, "Protocol type: %x\n", skb->protocol);
+
+			netif_rx(skb);
+		} else {
+			dev_err_ratelimited(dev,
+				"Couldn't allocate a skb_buff for a packet of size %u\n",
+				pkt_size);
+		}
+
+		priv->stat_rx_bytes += pkt_size;
+
+		desc->header.recv = 0;
+
+		ec_bhf_add_rx_desc(priv, desc);
+
+		desc = list_entry(desc->node.next, struct rx_desc, node);
+	}
+
+	priv->rx_desc = desc;
+}
+
+static enum hrtimer_restart ec_bhf_timer_fun(struct hrtimer *timer)
+{
+	struct ec_bhf_priv *priv = container_of(timer,
+					struct ec_bhf_priv,
+					hrtimer);
+	ec_bhf_process_rx(priv);
+	ec_bhf_process_tx(priv);
+
+	if (!netif_running(priv->net_dev))
+		return HRTIMER_NORESTART;
+
+	hrtimer_forward_now(timer, ktime_set(0, polling_frequency));
+	return HRTIMER_RESTART;
+}
+
+static int ec_bhf_setup_offsets(struct ec_bhf_priv *priv)
+{
+	struct device *dev = PRIV_TO_DEV(priv);
+	unsigned block_count, i;
+	void * __iomem ec_info;
+
+	dev_info(dev, "Info block:\n");
+	dev_info(dev, "Type of function: %x\n", (unsigned)ioread16(priv->io));
+	dev_info(dev, "Revision of function: %x\n",
+		(unsigned)ioread16(priv->io + INFO_BLOCK_REV));
+
+	block_count = ioread8(priv->io + INFO_BLOCK_BLK_CNT);
+	dev_info(dev, "Number of function blocks: %x\n", block_count);
+
+	for (i = 0; i < block_count; i++) {
+		u16 type = ioread16(priv->io + i * INFO_BLOCK_SIZE +
+				    INFO_BLOCK_TYPE);
+		if (type == ETHERCAT_MASTER_ID)
+			break;
+	}
+	if (i == block_count) {
+		dev_err(dev, "EtherCAT master with DMA block not found\n");
+		return -ENODEV;
+	}
+	dev_info(dev, "EtherCAT master with DMA block found at pos: %d\n", i);
+
+	ec_info = priv->io + i * INFO_BLOCK_SIZE;
+	dev_info(dev, "EtherCAT master revision: %d\n",
+		 ioread16(ec_info + INFO_BLOCK_REV));
+
+	priv->tx_dma_chan = ioread8(ec_info + INFO_BLOCK_TX_CHAN);
+	dev_info(dev, "EtherCAT master tx dma channel: %d\n",
+		 priv->tx_dma_chan);
+
+	priv->rx_dma_chan = ioread8(ec_info + INFO_BLOCK_RX_CHAN);
+	dev_info(dev, "EtherCAT master rx dma channel: %d\n",
+		 priv->rx_dma_chan);
+
+	priv->ec_io = priv->io + ioread32(ec_info + INFO_BLOCK_OFFSET);
+	priv->mii_io = priv->ec_io + ioread32(priv->ec_io + EC_MII_OFFSET);
+	priv->fifo_io = priv->ec_io + ioread32(priv->ec_io + EC_FIFO_OFFSET);
+	priv->mac_io = priv->ec_io + ioread32(priv->ec_io + EC_MAC_OFFSET);
+
+	dev_info(dev,
+		"EtherCAT block addres: %p, fifo address: %p, mii address: %p, mac address: %p",
+		priv->ec_io, priv->fifo_io, priv->mii_io, priv->mac_io);
+
+	return 0;
+}
+
+static netdev_tx_t ec_bhf_start_xmit(struct sk_buff *skb,
+				     struct net_device *net_dev)
+{
+	struct ec_bhf_priv *priv = netdev_priv(net_dev);
+	struct tx_desc *desc;
+	unsigned long flags;
+	unsigned len;
+
+	dev_dbg(PRIV_TO_DEV(priv), "Starting xmit\n");
+
+	spin_lock_irqsave(&priv->lock, flags);
+
+	BUG_ON(list_empty(&priv->tx_desc_free));
+
+	desc = list_first_entry(&priv->tx_desc_free, struct tx_desc, node);
+	list_del(&desc->node);
+
+	skb_copy_and_csum_dev(skb, desc->data);
+	len = skb->len;
+
+	memset(&desc->header, 0, sizeof(desc->header));
+	desc->header.len = cpu_to_le16(len);
+	desc->header.port = TX_HDR_PORT_0;
+
+	if (list_empty(&priv->tx_desc_queue))
+		ec_bhf_send_packet(priv, desc);
+
+	list_add_tail(&desc->node, &priv->tx_desc_queue);
+
+	if (list_empty(&priv->tx_desc_free)) {
+		dev_info(PRIV_TO_DEV(priv), "Stopping netif queue\n");
+		ec_bhf_print_status(priv);
+
+		netif_stop_queue(net_dev);
+	}
+
+	priv->stat_tx_bytes += len;
+
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	dev_kfree_skb(skb);
+
+	return NETDEV_TX_OK;
+}
+
+static int ec_bhf_alloc_dma_mem(struct ec_bhf_priv *priv,
+				int channel,
+				struct bhf_dma *buf)
+{
+	int offset = channel * DMA_CHAN_SIZE + DMA_CHAN_OFFSET;
+	struct device *dev = PRIV_TO_DEV(priv);
+	u32 mask;
+
+	iowrite32(0xffffffff, priv->dma_io + offset);
+
+	mask = ioread32(priv->dma_io + offset);
+	mask &= 0xfffffffc;
+	dev_info(dev, "Read mask %x for channel %d\n", mask, channel);
+	/* We want to allocate a chunk of memory that is:
+	 * - aligned to the mask we just read
+	 * - is of size 2^mask bytes
+	 * In order to ensure that we will allocate buffer of
+	 * 2 * 2^mask bytes.
+	 */
+	buf->len = ~mask + 1;
+	buf->alloc_len = 2 * buf->len;
+
+	dev_info(dev, "Allocating %d bytes for channel %d",
+		(int)buf->alloc_len, channel);
+	buf->alloc = dma_alloc_coherent(dev, buf->alloc_len,
+					&buf->alloc_phys, GFP_KERNEL);
+	if (buf->alloc == NULL) {
+		dev_info(dev, "Failed to allocate buffer\n");
+		return -ENOMEM;
+	}
+
+	buf->buf_phys = (buf->alloc_phys + buf->len) & mask;
+	buf->buf = buf->alloc + (buf->buf_phys - buf->alloc_phys);
+
+	iowrite32(0, priv->dma_io + offset + 4);
+	iowrite32(buf->buf_phys, priv->dma_io + offset);
+	dev_dbg(dev, "Buffer: %x and read from dev: %x",
+		(unsigned)buf->buf_phys, ioread32(priv->dma_io + offset));
+
+	return 0;
+}
+
+static void ec_bhf_setup_tx_descs(struct ec_bhf_priv *priv)
+{
+	struct tx_desc *descs = (struct tx_desc *) priv->tx_buf.buf;
+	int dcount, i;
+
+	INIT_LIST_HEAD(&priv->tx_desc_free);
+	INIT_LIST_HEAD(&priv->tx_desc_queue);
+
+	dcount = priv->tx_buf.len / sizeof(struct rx_desc);
+	for (i = 0; i < dcount; i++) {
+		struct tx_desc *desc = descs + i;
+		list_add_tail(&desc->node, &priv->tx_desc_free);
+	}
+}
+
+static void ec_bhf_setup_rx_descs(struct ec_bhf_priv *priv)
+{
+	int dcount = min_t(int, FIFO_SIZE, priv->rx_buf.len /
+					   (sizeof(struct rx_desc)));
+	struct rx_desc *desc = (struct rx_desc *) priv->rx_buf.buf;
+	int i;
+
+	for (i = 0; i < dcount; i++) {
+		u32 next;
+
+		if (i != dcount - 1)
+			next = (u8 *)(desc + 1) - priv->rx_buf.buf;
+		else
+			next = 0;
+		next |= RXHDR_NEXT_VALID;
+		desc->header.next = cpu_to_le32(next);
+		desc->header.recv = 0;
+		ec_bhf_add_rx_desc(priv, desc);
+
+		if (i == 0) {
+			INIT_LIST_HEAD(&desc->node);
+			priv->rx_desc = desc;
+		} else {
+			list_add_tail(&desc->node, &priv->rx_desc->node);
+		}
+
+		desc += 1;
+	}
+}
+
+static int ec_bhf_open(struct net_device *net_dev)
+{
+	struct ec_bhf_priv *priv = netdev_priv(net_dev);
+	struct device *dev = PRIV_TO_DEV(priv);
+	int err = 0;
+
+	dev_info(dev, "Opening device\n");
+
+	ec_bhf_reset(priv);
+
+	err = ec_bhf_alloc_dma_mem(priv, priv->rx_dma_chan, &priv->rx_buf);
+	if (err) {
+		dev_err(dev, "Failed to allocate rx buffer");
+		err = -ENOMEM;
+		goto out;
+	}
+	ec_bhf_setup_rx_descs(priv);
+
+	dev_info(dev, "RX buffer allocated, address: %x\n",
+		 (unsigned)priv->rx_buf.buf_phys);
+
+	err = ec_bhf_alloc_dma_mem(priv, priv->tx_dma_chan, &priv->tx_buf);
+	if (err) {
+		dev_err(dev, "Failed to allocate tx buffer");
+		goto error_rx_free;
+	}
+	dev_info(dev, "TX buffer allocated, addres: %x\n",
+		 (unsigned)priv->tx_buf.buf_phys);
+
+	iowrite8(0, priv->mii_io + MII_MAC_FILT_FLAG);
+
+	ec_bhf_setup_tx_descs(priv);
+
+	netif_start_queue(net_dev);
+
+	hrtimer_init(&priv->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	priv->hrtimer.function = ec_bhf_timer_fun;
+	hrtimer_start(&priv->hrtimer,
+		      ktime_set(0, polling_frequency),
+		      HRTIMER_MODE_REL);
+
+	dev_info(PRIV_TO_DEV(priv), "Device open\n");
+
+	ec_bhf_print_status(priv);
+
+	return 0;
+
+error_rx_free:
+	dma_free_coherent(dev, priv->rx_buf.alloc_len, priv->rx_buf.alloc,
+			  priv->rx_buf.alloc_len);
+out:
+	return err;
+}
+
+static int ec_bhf_stop(struct net_device *net_dev)
+{
+	struct ec_bhf_priv *priv = netdev_priv(net_dev);
+	struct device *dev = PRIV_TO_DEV(priv);
+
+	hrtimer_cancel(&priv->hrtimer);
+
+	ec_bhf_reset(priv);
+
+	netif_tx_disable(net_dev);
+
+	dma_free_coherent(dev, priv->tx_buf.alloc_len,
+			  priv->tx_buf.alloc, priv->tx_buf.alloc_phys);
+	dma_free_coherent(dev, priv->rx_buf.alloc_len,
+			  priv->rx_buf.alloc, priv->rx_buf.alloc_phys);
+
+	return 0;
+}
+
+static struct rtnl_link_stats64 *
+ec_bhf_get_stats(struct net_device *net_dev,
+		 struct rtnl_link_stats64 *stats)
+{
+	struct ec_bhf_priv *priv = netdev_priv(net_dev);
+
+	memset(stats, 0, sizeof(*stats));
+
+	stats->rx_errors = ioread8(priv->mac_io + MAC_RX_ERR_CNT) +
+				ioread8(priv->mac_io + MAC_CRC_ERR_CNT) +
+				ioread8(priv->mac_io + MAC_FRAME_ERR_CNT);
+	stats->rx_packets = ioread32(priv->mac_io + MAC_RX_FRAME_CNT);
+	stats->tx_packets = ioread32(priv->mac_io + MAC_TX_FRAME_CNT);
+	stats->rx_dropped = ioread8(priv->mac_io + MAC_DROPPED_FRMS);
+
+	stats->tx_bytes = priv->stat_tx_bytes;
+	stats->rx_bytes = priv->stat_rx_bytes;
+
+	return stats;
+}
+
+static const struct net_device_ops ec_bhf_netdev_ops = {
+	.ndo_start_xmit		= ec_bhf_start_xmit,
+	.ndo_open		= ec_bhf_open,
+	.ndo_stop		= ec_bhf_stop,
+	.ndo_get_stats64	= ec_bhf_get_stats,
+	.ndo_change_mtu		= eth_change_mtu,
+	.ndo_validate_addr	= eth_validate_addr,
+	.ndo_set_mac_address	= eth_mac_addr
+};
+
+static int ec_bhf_probe(struct pci_dev *dev, const struct pci_device_id *id)
+{
+	int err = 0;
+	void * __iomem io;
+	void * __iomem dma_io;
+	struct net_device *net_dev;
+	struct ec_bhf_priv *priv;
+
+	err = pci_enable_device(dev);
+	if (err)
+		return err;
+
+	pci_set_master(dev);
+
+	err = pci_set_dma_mask(dev, DMA_BIT_MASK(32));
+	if (err) {
+		dev_err(&dev->dev,
+			"Required dma mask not supported, failed to initialize device\n");
+		err = -EIO;
+		goto err_disable_dev;
+	}
+
+	err = pci_set_consistent_dma_mask(dev, DMA_BIT_MASK(32));
+	if (err) {
+		dev_err(&dev->dev,
+			"Required dma mask not supported, failed to initialize device\n");
+		goto err_disable_dev;
+	}
+
+	err = pci_request_regions(dev, "ec_bhf");
+	if (err) {
+		dev_err(&dev->dev,
+			"Failed to request pci memory regions\n");
+		goto err_disable_dev;
+	}
+
+	io = pci_iomap(dev, 0, 0);
+	if (!io) {
+		dev_err(&dev->dev, "Failed to map pci card memory bar 0");
+		err = -EIO;
+		goto err_release_regions;
+	}
+
+	dma_io = pci_iomap(dev, 2, 0);
+	if (!dma_io) {
+		dev_err(&dev->dev, "Failed to map pci card memory bar 2");
+		err = -EIO;
+		goto err_unmap;
+	}
+
+	net_dev = alloc_etherdev(sizeof(struct ec_bhf_priv));
+	if (net_dev == 0) {
+		err = -ENOMEM;
+		goto err_unmap_dma_io;
+	}
+
+	pci_set_drvdata(dev, net_dev);
+	SET_NETDEV_DEV(net_dev, &dev->dev);
+
+	net_dev->features = 0;
+	net_dev->flags |= IFF_NOARP;
+
+	net_dev->netdev_ops = &ec_bhf_netdev_ops;
+
+	priv = netdev_priv(net_dev);
+	priv->net_dev = net_dev;
+	priv->io = io;
+	priv->dma_io = dma_io;
+	priv->dev = dev;
+
+	spin_lock_init(&priv->lock);
+
+	err = ec_bhf_setup_offsets(priv);
+	if (err < 0)
+		goto err_free_net_dev;
+
+	memcpy_fromio(net_dev->dev_addr, priv->mii_io + MII_MAC_ADDR, 6);
+
+	dev_info(&dev->dev, "CX5020 Ethercat master address: %pM\n",
+		 net_dev->dev_addr);
+
+	err = register_netdev(net_dev);
+	if (err < 0)
+		goto err_free_net_dev;
+
+	return 0;
+
+err_free_net_dev:
+	free_netdev(net_dev);
+err_unmap_dma_io:
+	pci_iounmap(dev, dma_io);
+err_unmap:
+	pci_iounmap(dev, io);
+err_release_regions:
+	pci_release_regions(dev);
+err_disable_dev:
+	pci_clear_master(dev);
+	pci_disable_device(dev);
+
+	return err;
+}
+
+static void ec_bhf_remove(struct pci_dev *dev)
+{
+	struct net_device *net_dev = pci_get_drvdata(dev);
+	struct ec_bhf_priv *priv = netdev_priv(net_dev);
+
+	unregister_netdev(net_dev);
+	free_netdev(net_dev);
+
+	pci_iounmap(dev, priv->dma_io);
+	pci_iounmap(dev, priv->io);
+	pci_release_regions(dev);
+	pci_clear_master(dev);
+	pci_disable_device(dev);
+}
+
+static struct pci_driver pci_driver = {
+	.name		= "ec_bhf",
+	.id_table	= ids,
+	.probe		= ec_bhf_probe,
+	.remove		= ec_bhf_remove,
+};
+
+static int __init ec_bhf_init(void)
+{
+	return pci_register_driver(&pci_driver);
+}
+
+static void __exit ec_bhf_exit(void)
+{
+	pci_unregister_driver(&pci_driver);
+}
+
+module_init(ec_bhf_init);
+module_exit(ec_bhf_exit);
+
+module_param(polling_frequency, long, S_IRUGO);
+MODULE_PARM_DESC(polling_frequency, "Polling timer frequency in ns");
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Dariusz Marcinkiewicz <reksio@newterm.pl>");
-- 
1.7.10.4

Re: [PATCH v5 1/1] Driver for Beckhoff CX5020 EtherCAT master module.

[Francois Romieu]

[-- Attachment #1: Type: text/plain, Size: 1681 bytes --]

Darek Marcinkiewicz <reksio@newterm.pl> :
[...]
> This driver adds support for EtherCAT master module located on CCAT
> FPGA found on Beckhoff CX series industrial PCs. The driver exposes
> EtherCAT master as an ethernet interface.
> 
> EtherCAT is a filedbus protocol defined on top of ethernet and Beckhoff

s/filedbus/fieldbus/ :o)

> CX5020 PCs come with built-in EtherCAT master module located on a FPGA,
> which in turn is connected to a PCI bus.
> 
> Signed-off-by: Dariusz Marcinkiewicz <reksio@newterm.pl>

I have attached four patches. Patch #3 / 4 contains a bugfix. Remaining
patches are small nits.

The points below are not showstoppers. Your answer will be welcome though.

If the tx_desc_free list can turn empty, I don't see much point for not
using an array as descriptor ring. It's quite common for linux ethernet
drivers.

Be it a list or an array, sizing it as the max count of descriptors that
could fit in the memory window through which packet descriptors _and_
ethernet data are copied seems overkill. It may be an upper bound though.
If you are not sure about a sane default value, ethtool API allows to
change the descriptor ring size.

Given the amount of debug messages, ethtool registers dump support could
be of some value.

I guess that the ASIC and the host CPU need some external polling to
communicate (no IRQ, really ?), whence the hrtimer. It should naturally
fit with NAPI. hrtimer could thus be disabled as soon as some ASIC event
is detected. It should save some hrtimer work when load increases and
the usual Tx lock avoidance patterns would kick in (current locking is
gross).

Did Beckhoff document their FPGA PCI interface ?

-- 
Ueimor

[-- Attachment #2: 0001-ec_bhf-inverted-xmas-tree.patch --]
[-- Type: text/plain, Size: 1446 bytes --]

>From d2a5bfdc7854e9b3cffcab9249299354fa5037d0 Mon Sep 17 00:00:00 2001
Message-Id: <d2a5bfdc7854e9b3cffcab9249299354fa5037d0.1399110121.git.romieu@fr.zoreil.com>
From: Francois Romieu <romieu@fr.zoreil.com>
Date: Sat, 3 May 2014 11:15:26 +0200
Subject: [PATCH 1/4] ec_bhf: inverted xmas tree.
X-Organisation: Land of Sunshine Inc.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/ethernet/ec_bhf.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ec_bhf.c b/drivers/net/ethernet/ec_bhf.c
index 8e78b8d..9e1636b 100644
--- a/drivers/net/ethernet/ec_bhf.c
+++ b/drivers/net/ethernet/ec_bhf.c
@@ -205,8 +205,8 @@ static void ec_bhf_reset(struct ec_bhf_priv *priv)
 
 static void ec_bhf_send_packet(struct ec_bhf_priv *priv, struct tx_desc *desc)
 {
-	u32 addr = (u8 *)desc - priv->tx_buf.buf;
 	u32 len = le16_to_cpu(desc->header.len) + sizeof(desc->header);
+	u32 addr = (u8 *)desc - priv->tx_buf.buf;
 
 	iowrite32((ALIGN(len, 8) << 24) | addr, priv->fifo_io + FIFO_TX_REG);
 
@@ -598,11 +598,11 @@ static const struct net_device_ops ec_bhf_netdev_ops = {
 
 static int ec_bhf_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
-	int err = 0;
-	void * __iomem io;
-	void * __iomem dma_io;
 	struct net_device *net_dev;
 	struct ec_bhf_priv *priv;
+	void * __iomem dma_io;
+	void * __iomem io;
+	int err = 0;
 
 	err = pci_enable_device(dev);
 	if (err)
-- 
1.9.0


[-- Attachment #3: 0002-ec_bhf-line-breaks-and-indent.patch --]
[-- Type: text/plain, Size: 4203 bytes --]

>From b64a35b7f4190a8126bc7b393370c711285f8c30 Mon Sep 17 00:00:00 2001
Message-Id: <b64a35b7f4190a8126bc7b393370c711285f8c30.1399110121.git.romieu@fr.zoreil.com>
In-Reply-To: <d2a5bfdc7854e9b3cffcab9249299354fa5037d0.1399110121.git.romieu@fr.zoreil.com>
References: <d2a5bfdc7854e9b3cffcab9249299354fa5037d0.1399110121.git.romieu@fr.zoreil.com>
From: Francois Romieu <romieu@fr.zoreil.com>
Date: Sat, 3 May 2014 11:31:55 +0200
Subject: [PATCH 2/4] ec_bhf: line breaks and indent.
X-Organisation: Land of Sunshine Inc.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/ethernet/ec_bhf.c | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/ec_bhf.c b/drivers/net/ethernet/ec_bhf.c
index 9e1636b..d33b691 100644
--- a/drivers/net/ethernet/ec_bhf.c
+++ b/drivers/net/ethernet/ec_bhf.c
@@ -168,23 +168,23 @@ static void ec_bhf_print_status(struct ec_bhf_priv *priv)
 	struct device *dev = PRIV_TO_DEV(priv);
 
 	dev_dbg(dev, "Frame error counter: %d\n",
-		 ioread8(priv->mac_io + MAC_FRAME_ERR_CNT));
+		ioread8(priv->mac_io + MAC_FRAME_ERR_CNT));
 	dev_dbg(dev, "RX error counter: %d\n",
-		 ioread8(priv->mac_io + MAC_RX_ERR_CNT));
+		ioread8(priv->mac_io + MAC_RX_ERR_CNT));
 	dev_dbg(dev, "CRC error counter: %d\n",
-		 ioread8(priv->mac_io + MAC_CRC_ERR_CNT));
+		ioread8(priv->mac_io + MAC_CRC_ERR_CNT));
 	dev_dbg(dev, "TX frame counter: %d\n",
-		 ioread32(priv->mac_io + MAC_TX_FRAME_CNT));
+		ioread32(priv->mac_io + MAC_TX_FRAME_CNT));
 	dev_dbg(dev, "RX frame counter: %d\n",
-		 ioread32(priv->mac_io + MAC_RX_FRAME_CNT));
+		ioread32(priv->mac_io + MAC_RX_FRAME_CNT));
 	dev_dbg(dev, "TX fifo level: %d\n",
-		 ioread8(priv->mac_io + MAC_TX_FIFO_LVL));
+		ioread8(priv->mac_io + MAC_TX_FIFO_LVL));
 	dev_dbg(dev, "Dropped frames: %d\n",
-		 ioread8(priv->mac_io + MAC_DROPPED_FRMS));
+		ioread8(priv->mac_io + MAC_DROPPED_FRMS));
 	dev_dbg(dev, "Connected with CCAT slot: %d\n",
-		 ioread8(priv->mac_io + MAC_CONNECTED_CCAT_FLAG));
+		ioread8(priv->mac_io + MAC_CONNECTED_CCAT_FLAG));
 	dev_dbg(dev, "Link status: %d\n",
-		 ioread8(priv->mii_io + MII_LINK_STATUS));
+		ioread8(priv->mii_io + MII_LINK_STATUS));
 }
 
 static void ec_bhf_reset(struct ec_bhf_priv *priv)
@@ -224,8 +224,7 @@ static void ec_bhf_process_tx(struct ec_bhf_priv *priv)
 	if (list_empty(&priv->tx_desc_queue))
 		goto out;
 
-	desc = list_first_entry(&priv->tx_desc_queue,
-				struct tx_desc, node);
+	desc = list_first_entry(&priv->tx_desc_queue, struct tx_desc, node);
 	sent = le32_to_cpu(desc->header.sent) & TX_HDR_SENT;
 	if (!sent)
 		goto out;
@@ -297,9 +296,8 @@ static void ec_bhf_process_rx(struct ec_bhf_priv *priv)
 
 static enum hrtimer_restart ec_bhf_timer_fun(struct hrtimer *timer)
 {
-	struct ec_bhf_priv *priv = container_of(timer,
-					struct ec_bhf_priv,
-					hrtimer);
+	struct ec_bhf_priv *priv = container_of(timer, struct ec_bhf_priv,
+						hrtimer);
 	ec_bhf_process_rx(priv);
 	ec_bhf_process_tx(priv);
 
@@ -429,8 +427,8 @@ static int ec_bhf_alloc_dma_mem(struct ec_bhf_priv *priv,
 
 	dev_info(dev, "Allocating %d bytes for channel %d",
 		(int)buf->alloc_len, channel);
-	buf->alloc = dma_alloc_coherent(dev, buf->alloc_len,
-					&buf->alloc_phys, GFP_KERNEL);
+	buf->alloc = dma_alloc_coherent(dev, buf->alloc_len, &buf->alloc_phys,
+					GFP_KERNEL);
 	if (buf->alloc == NULL) {
 		dev_info(dev, "Failed to allocate buffer\n");
 		return -ENOMEM;
@@ -529,8 +527,7 @@ static int ec_bhf_open(struct net_device *net_dev)
 
 	hrtimer_init(&priv->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	priv->hrtimer.function = ec_bhf_timer_fun;
-	hrtimer_start(&priv->hrtimer,
-		      ktime_set(0, polling_frequency),
+	hrtimer_start(&priv->hrtimer, ktime_set(0, polling_frequency),
 		      HRTIMER_MODE_REL);
 
 	dev_info(PRIV_TO_DEV(priv), "Device open\n");
@@ -627,8 +624,7 @@ static int ec_bhf_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	err = pci_request_regions(dev, "ec_bhf");
 	if (err) {
-		dev_err(&dev->dev,
-			"Failed to request pci memory regions\n");
+		dev_err(&dev->dev, "Failed to request pci memory regions\n");
 		goto err_disable_dev;
 	}
 
-- 
1.9.0


[-- Attachment #4: 0003-ec_bhf-rx_desc-vs-tx_desc-confusion.patch --]
[-- Type: text/plain, Size: 1249 bytes --]

>From a687c964badc9d01e0d2ae269bc5d6996e508dd1 Mon Sep 17 00:00:00 2001
Message-Id: <a687c964badc9d01e0d2ae269bc5d6996e508dd1.1399110121.git.romieu@fr.zoreil.com>
In-Reply-To: <d2a5bfdc7854e9b3cffcab9249299354fa5037d0.1399110121.git.romieu@fr.zoreil.com>
References: <d2a5bfdc7854e9b3cffcab9249299354fa5037d0.1399110121.git.romieu@fr.zoreil.com>
From: Francois Romieu <romieu@fr.zoreil.com>
Date: Sat, 3 May 2014 11:36:12 +0200
Subject: [PATCH 3/4] ec_bhf: rx_desc vs tx_desc confusion.
X-Organisation: Land of Sunshine Inc.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/ethernet/ec_bhf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ec_bhf.c b/drivers/net/ethernet/ec_bhf.c
index d33b691..c697b48 100644
--- a/drivers/net/ethernet/ec_bhf.c
+++ b/drivers/net/ethernet/ec_bhf.c
@@ -453,7 +453,7 @@ static void ec_bhf_setup_tx_descs(struct ec_bhf_priv *priv)
 	INIT_LIST_HEAD(&priv->tx_desc_free);
 	INIT_LIST_HEAD(&priv->tx_desc_queue);
 
-	dcount = priv->tx_buf.len / sizeof(struct rx_desc);
+	dcount = priv->tx_buf.len / sizeof(struct tx_desc);
 	for (i = 0; i < dcount; i++) {
 		struct tx_desc *desc = descs + i;
 		list_add_tail(&desc->node, &priv->tx_desc_free);
-- 
1.9.0


[-- Attachment #5: 0004-ec_bhf-use-once-variable-removal.patch --]
[-- Type: text/plain, Size: 1547 bytes --]

>From d47e13a4a22a5e5ebc32d88397acd702f1ae95d4 Mon Sep 17 00:00:00 2001
Message-Id: <d47e13a4a22a5e5ebc32d88397acd702f1ae95d4.1399110121.git.romieu@fr.zoreil.com>
In-Reply-To: <d2a5bfdc7854e9b3cffcab9249299354fa5037d0.1399110121.git.romieu@fr.zoreil.com>
References: <d2a5bfdc7854e9b3cffcab9249299354fa5037d0.1399110121.git.romieu@fr.zoreil.com>
From: Francois Romieu <romieu@fr.zoreil.com>
Date: Sat, 3 May 2014 11:40:37 +0200
Subject: [PATCH 4/4] ec_bhf: use once variable removal.
X-Organisation: Land of Sunshine Inc.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/ethernet/ec_bhf.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/ec_bhf.c b/drivers/net/ethernet/ec_bhf.c
index c697b48..10dbcc4 100644
--- a/drivers/net/ethernet/ec_bhf.c
+++ b/drivers/net/ethernet/ec_bhf.c
@@ -448,16 +448,13 @@ static int ec_bhf_alloc_dma_mem(struct ec_bhf_priv *priv,
 static void ec_bhf_setup_tx_descs(struct ec_bhf_priv *priv)
 {
 	struct tx_desc *descs = (struct tx_desc *) priv->tx_buf.buf;
-	int dcount, i;
+	int i;
 
 	INIT_LIST_HEAD(&priv->tx_desc_free);
 	INIT_LIST_HEAD(&priv->tx_desc_queue);
 
-	dcount = priv->tx_buf.len / sizeof(struct tx_desc);
-	for (i = 0; i < dcount; i++) {
-		struct tx_desc *desc = descs + i;
-		list_add_tail(&desc->node, &priv->tx_desc_free);
-	}
+	for (i = 0; i < priv->tx_buf.len / sizeof(struct tx_desc); i++)
+		list_add_tail(&descs[i].node, &priv->tx_desc_free);
 }
 
 static void ec_bhf_setup_rx_descs(struct ec_bhf_priv *priv)
-- 
1.9.0

Re: [PATCH v5 1/1] Driver for Beckhoff CX5020 EtherCAT master module.

[Darek Marcinkiewicz]

[-- Attachment #1: Type: text/plain, Size: 3196 bytes --]

On Sat, May 03, 2014 at 01:40:29PM +0200, Francois Romieu wrote:
> Darek Marcinkiewicz <reksio@newterm.pl> :
> [...]
> > This driver adds support for EtherCAT master module located on CCAT
> > FPGA found on Beckhoff CX series industrial PCs. The driver exposes
> > EtherCAT master as an ethernet interface.
> > 
> > EtherCAT is a filedbus protocol defined on top of ethernet and Beckhoff
> 
> s/filedbus/fieldbus/ :o)
> 
Ops :) Fixed in next revision of the patch.
> > CX5020 PCs come with built-in EtherCAT master module located on a FPGA,
> > which in turn is connected to a PCI bus.
> > 
> > Signed-off-by: Dariusz Marcinkiewicz <reksio@newterm.pl>
> 
> I have attached four patches. Patch #3 / 4 contains a bugfix. Remaining
> patches are small nits.
> 
Thank you. I am attaching 2 of thse to this repsonse - the other two 
are no longer relevant due to the changes I made into the driver.
One of the attached patches is slightly modfied by simply having one hunk
removed (that hunk was applying to the code that was removed in next rev 
of the driver). Not sure how to proceed with those patches, shall I simply
sent out these patches to this list as a separate messages?

> The points below are not showstoppers. Your answer will be welcome though.
> 
> If the tx_desc_free list can turn empty, I don't see much point for not
> using an array as descriptor ring. It's quite common for linux ethernet
> drivers.
> 
I have switched the code to use array of descriptors. Yep, lists do
not make too much sense here.

> Be it a list or an array, sizing it as the max count of descriptors that
> could fit in the memory window through which packet descriptors _and_
> ethernet data are copied seems overkill. It may be an upper bound though.
> If you are not sure about a sane default value, ethtool API allows to
> change the descriptor ring size.
> 
I have changed the code to use much modest value - it is set to be of the
size of the fifo now. I think that this value is much better, but of course
having this configurable would be even better.

> Given the amount of debug messages, ethtool registers dump support could
> be of some value.
> 
> I guess that the ASIC and the host CPU need some external polling to
> communicate (no IRQ, really ?), whence the hrtimer. It should naturally
> fit with NAPI. hrtimer could thus be disabled as soon as some ASIC event
> is detected. It should save some hrtimer work when load increases and
> the usual Tx lock avoidance patterns would kick in (current locking is
> gross).
> 
No, there is really no interrupt, hence the timer. Also on this device I wouldn't 
expect any bursts of data. What happens here, during regular operation of the 
device, is a periodic exchange of (few) ethernet packets between host cpu and
terminals attached to the EtherCAT bus. As for the locking on the tx path,
I have removed that completely on receiving path. I simply didn't know 
that it is such a big no-no here :)

> Did Beckhoff document their FPGA PCI interface ?
> 

Yes. It took a lot of nagging but I got some documentation from them, together
with a sample code showing how to use dma interface and generally interact with
the device.


Thank you!
-- 
DM

[-- Attachment #2: 0001-ec_bhf-inverted-xmas-tree.patch --]
[-- Type: text/x-diff, Size: 1323 bytes --]

>From 4254a10cb8407634c94882f0f64c8e4a7f84b852 Mon Sep 17 00:00:00 2001
From: Francois Romieu <romieu@fr.zoreil.com>
Date: Sat, 3 May 2014 11:15:26 +0200
Subject: [PATCH 1/2] ec_bhf: inverted xmas tree.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/ethernet/ec_bhf.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ec_bhf.c b/drivers/net/ethernet/ec_bhf.c
index feb40cd..e6e303c 100644
--- a/drivers/net/ethernet/ec_bhf.c
+++ b/drivers/net/ethernet/ec_bhf.c
@@ -204,8 +204,8 @@ static void ec_bhf_reset(struct ec_bhf_priv *priv)
 
 static void ec_bhf_send_packet(struct ec_bhf_priv *priv, struct tx_desc *desc)
 {
-	u32 addr = (u8 *)desc - priv->tx_buf.buf;
 	u32 len = le16_to_cpu(desc->header.len) + sizeof(desc->header);
+	u32 addr = (u8 *)desc - priv->tx_buf.buf;
 
 	iowrite32((ALIGN(len, 8) << 24) | addr, priv->fifo_io + FIFO_TX_REG);
 
@@ -561,11 +561,11 @@ static const struct net_device_ops ec_bhf_netdev_ops = {
 
 static int ec_bhf_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
-	int err = 0;
-	void * __iomem io;
-	void * __iomem dma_io;
 	struct net_device *net_dev;
 	struct ec_bhf_priv *priv;
+	void * __iomem dma_io;
+	void * __iomem io;
+	int err = 0;
 
 	err = pci_enable_device(dev);
 	if (err)
-- 
1.7.10.4


[-- Attachment #3: 0002-ec_bhf-line-breaks-and-indent.patch --]
[-- Type: text/x-diff, Size: 3534 bytes --]

>From d20cec3758521de9c5c9c7a35c0a96a3671bc2c2 Mon Sep 17 00:00:00 2001
From: Francois Romieu <romieu@fr.zoreil.com>
Date: Sat, 3 May 2014 11:31:55 +0200
Subject: [PATCH 2/2] ec_bhf: line breaks and indent.

Signed-off-by: Francois Romieu <romieu@fr.zoreil.com>
---
 drivers/net/ethernet/ec_bhf.c |   33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/ec_bhf.c b/drivers/net/ethernet/ec_bhf.c
index e6e303c..46618b9 100644
--- a/drivers/net/ethernet/ec_bhf.c
+++ b/drivers/net/ethernet/ec_bhf.c
@@ -167,23 +167,23 @@ static void ec_bhf_print_status(struct ec_bhf_priv *priv)
 	struct device *dev = PRIV_TO_DEV(priv);
 
 	dev_dbg(dev, "Frame error counter: %d\n",
-		 ioread8(priv->mac_io + MAC_FRAME_ERR_CNT));
+		ioread8(priv->mac_io + MAC_FRAME_ERR_CNT));
 	dev_dbg(dev, "RX error counter: %d\n",
-		 ioread8(priv->mac_io + MAC_RX_ERR_CNT));
+		ioread8(priv->mac_io + MAC_RX_ERR_CNT));
 	dev_dbg(dev, "CRC error counter: %d\n",
-		 ioread8(priv->mac_io + MAC_CRC_ERR_CNT));
+		ioread8(priv->mac_io + MAC_CRC_ERR_CNT));
 	dev_dbg(dev, "TX frame counter: %d\n",
-		 ioread32(priv->mac_io + MAC_TX_FRAME_CNT));
+		ioread32(priv->mac_io + MAC_TX_FRAME_CNT));
 	dev_dbg(dev, "RX frame counter: %d\n",
-		 ioread32(priv->mac_io + MAC_RX_FRAME_CNT));
+		ioread32(priv->mac_io + MAC_RX_FRAME_CNT));
 	dev_dbg(dev, "TX fifo level: %d\n",
-		 ioread8(priv->mac_io + MAC_TX_FIFO_LVL));
+		ioread8(priv->mac_io + MAC_TX_FIFO_LVL));
 	dev_dbg(dev, "Dropped frames: %d\n",
-		 ioread8(priv->mac_io + MAC_DROPPED_FRMS));
+		ioread8(priv->mac_io + MAC_DROPPED_FRMS));
 	dev_dbg(dev, "Connected with CCAT slot: %d\n",
-		 ioread8(priv->mac_io + MAC_CONNECTED_CCAT_FLAG));
+		ioread8(priv->mac_io + MAC_CONNECTED_CCAT_FLAG));
 	dev_dbg(dev, "Link status: %d\n",
-		 ioread8(priv->mii_io + MII_LINK_STATUS));
+		ioread8(priv->mii_io + MII_LINK_STATUS));
 }
 
 static void ec_bhf_reset(struct ec_bhf_priv *priv)
@@ -277,9 +277,8 @@ static void ec_bhf_process_rx(struct ec_bhf_priv *priv)
 
 static enum hrtimer_restart ec_bhf_timer_fun(struct hrtimer *timer)
 {
-	struct ec_bhf_priv *priv = container_of(timer,
-					struct ec_bhf_priv,
-					hrtimer);
+	struct ec_bhf_priv *priv = container_of(timer, struct ec_bhf_priv,
+						hrtimer);
 	ec_bhf_process_rx(priv);
 	ec_bhf_process_tx(priv);
 
@@ -401,8 +400,8 @@ static int ec_bhf_alloc_dma_mem(struct ec_bhf_priv *priv,
 
 	dev_info(dev, "Allocating %d bytes for channel %d",
 		(int)buf->alloc_len, channel);
-	buf->alloc = dma_alloc_coherent(dev, buf->alloc_len,
-					&buf->alloc_phys, GFP_KERNEL);
+	buf->alloc = dma_alloc_coherent(dev, buf->alloc_len, &buf->alloc_phys,
+					GFP_KERNEL);
 	if (buf->alloc == NULL) {
 		dev_info(dev, "Failed to allocate buffer\n");
 		return -ENOMEM;
@@ -492,8 +491,7 @@ static int ec_bhf_open(struct net_device *net_dev)
 
 	hrtimer_init(&priv->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 	priv->hrtimer.function = ec_bhf_timer_fun;
-	hrtimer_start(&priv->hrtimer,
-		      ktime_set(0, polling_frequency),
+	hrtimer_start(&priv->hrtimer, ktime_set(0, polling_frequency),
 		      HRTIMER_MODE_REL);
 
 	dev_info(PRIV_TO_DEV(priv), "Device open\n");
@@ -590,8 +588,7 @@ static int ec_bhf_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	err = pci_request_regions(dev, "ec_bhf");
 	if (err) {
-		dev_err(&dev->dev,
-			"Failed to request pci memory regions\n");
+		dev_err(&dev->dev, "Failed to request pci memory regions\n");
 		goto err_disable_dev;
 	}
 
-- 
1.7.10.4

Re: [PATCH v5 1/1] Driver for Beckhoff CX5020 EtherCAT master module.

[Darek Marcinkiewicz]

On Sun, May 04, 2014 at 01:04:13PM +0200, Darek Marcinkiewicz wrote:
> > 
...
> attached to the EtherCAT bus. As for the locking on the tx path,
> I have removed that completely on receiving path. I simply didn't know 
...
Erm. I meant - I have removed it on the tx path. It was gone on rx
path already a few patch revs ago :)

-- 
DM

Re: [PATCH v5 1/1] Driver for Beckhoff CX5020 EtherCAT master module.

[Francois Romieu]

Darek Marcinkiewicz <reksio@newterm.pl> :
> On Sat, May 03, 2014 at 01:40:29PM +0200, Francois Romieu wrote:
[...]
> Thank you. I am attaching 2 of thse to this repsonse - the other two 
> are no longer relevant due to the changes I made into the driver.
> One of the attached patches is slightly modfied by simply having one hunk
> removed (that hunk was applying to the code that was removed in next rev 
> of the driver). Not sure how to proceed with those patches, shall I simply
> sent out these patches to this list as a separate messages?

You should submit a complete series if you want them separated - git
format-patch does wonders here - or include these directly in your own
patch as I don't really care for the credit.

[...]
> I have changed the code to use much modest value - it is set to be of the
> size of the fifo now. I think that this value is much better, but of course
> having this configurable would be even better.

(see ethtool_ops.[gs]et_ringparam)

[...]
> No, there is really no interrupt, hence the timer. Also on this device I wouldn't 
> expect any bursts of data. What happens here, during regular operation of the 
> device, is a periodic exchange of (few) ethernet packets between host cpu and
> terminals attached to the EtherCAT bus. As for the locking on the tx path,
> I have removed that completely on receiving path. I simply didn't know 
> that it is such a big no-no here :)

Ok.

Regarding tx_dnext updates, you may add a short notice in ec_bhf_start_xmit
and ec_bhf_process_tx explaining that the periodic poller will somehow end
working with the right value, whence no (smp_)barrier at all.

-- 
Ueimor

Re: [PATCH v5 1/1] Driver for Beckhoff CX5020 EtherCAT master module.

[Darek Marcinkiewicz]

On Sun, May 04, 2014 at 08:43:51PM +0200, Francois Romieu wrote:
> Darek Marcinkiewicz <reksio@newterm.pl> :
> > On Sat, May 03, 2014 at 01:40:29PM +0200, Francois Romieu wrote:
> [...]
> > Thank you. I am attaching 2 of thse to this repsonse - the other two 
> > are no longer relevant due to the changes I made into the driver.
> > One of the attached patches is slightly modfied by simply having one hunk
> > removed (that hunk was applying to the code that was removed in next rev 
> > of the driver). Not sure how to proceed with those patches, shall I simply
> > sent out these patches to this list as a separate messages?
> 
> You should submit a complete series if you want them separated - git
> format-patch does wonders here - or include these directly in your own
> patch as I don't really care for the credit.
> 
Ok. I've coalesced them into single patch. Thanks.

> [...]
> > I have changed the code to use much modest value - it is set to be of the
> > size of the fifo now. I think that this value is much better, but of course
> > having this configurable would be even better.
> 
> (see ethtool_ops.[gs]et_ringparam)
> 
After giving it a little though I came to the consclusion that in reality it won't
bring much value. So, if it is a showstopper, I would leave it at that.


> [...]
> > No, there is really no interrupt, hence the timer. Also on this device I wouldn't 
> > expect any bursts of data. What happens here, during regular operation of the 
> > device, is a periodic exchange of (few) ethernet packets between host cpu and
> > terminals attached to the EtherCAT bus. As for the locking on the tx path,
> > I have removed that completely on receiving path. I simply didn't know 
> > that it is such a big no-no here :)
> 
> Ok.
> 
> Regarding tx_dnext updates, you may add a short notice in ec_bhf_start_xmit
> and ec_bhf_process_tx explaining that the periodic poller will somehow end
> working with the right value, whence no (smp_)barrier at all.
> 
Hmm, good point. I am not really sure that it is not a race. So, I've added
memory barriers for the case when the tx ring becomes full.

About to send out new patch revison.

-- 
DM

Re: [PATCH v5 1/1] Driver for Beckhoff CX5020 EtherCAT master module.

[Francois Romieu]

Darek Marcinkiewicz <reksio@newterm.pl> :
> On Sun, May 04, 2014 at 08:43:51PM +0200, Francois Romieu wrote:
[...]
> > Regarding tx_dnext updates, you may add a short notice in ec_bhf_start_xmit
> > and ec_bhf_process_tx explaining that the periodic poller will somehow end
> > working with the right value, whence no (smp_)barrier at all.
> > 
> Hmm, good point. I am not really sure that it is not a race. So, I've added
> memory barriers for the case when the tx ring becomes full.

Without memory barrier, the hrtimer poller may be wrong but 1) it will
always be pessimistic and 2) it won't last. It would be sloppy though.

Did you have some time to test the latest version ?

-- 
Ueimor

Re: [PATCH v5 1/1] Driver for Beckhoff CX5020 EtherCAT master module.

[Darek Marcinkiewicz]

On Sun, May 04, 2014 at 11:19:28PM +0200, Francois Romieu wrote:
> Darek Marcinkiewicz <reksio@newterm.pl> :
> > On Sun, May 04, 2014 at 08:43:51PM +0200, Francois Romieu wrote:
> [...]
> > > Regarding tx_dnext updates, you may add a short notice in ec_bhf_start_xmit
> > > and ec_bhf_process_tx explaining that the periodic poller will somehow end
> > > working with the right value, whence no (smp_)barrier at all.
> > > 
> > Hmm, good point. I am not really sure that it is not a race. So, I've added
> > memory barriers for the case when the tx ring becomes full.
> 
> Without memory barrier, the hrtimer poller may be wrong but 1) it will
> always be pessimistic and 2) it won't last. It would be sloppy though.
> 
> Did you have some time to test the latest version ?
> 
Yes, I have this code continuously running and it looks good. This is a regular
application that I have running now, so not all interesting edge cases might have been
exercised, though.

-- 
DM