[PATCH v7 0/5] misc: Add Add Synopsys DesignWare xData IP driver

[PATCH v7 0/5] misc: Add Add Synopsys DesignWare xData IP driver

[Gustavo Pimentel]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 2442 bytes --]

This patch series adds a new driver called xData-pcie for the Synopsys
DesignWare PCIe prototype.

The driver configures and enables the Synopsys DesignWare PCIe traffic
generator IP inside of prototype Endpoint which will generate upstream
and downstream PCIe traffic. This allows to quickly test the PCIe link
throughput speed and check is the prototype solution has some limitation
or not.

Changes:
 V2: Rework driver according to Greg Kroah-Hartman' feedback
 V3: Fixed issues detected while running on 64 bits platforms
     Rebased patches on top of v5.11-rc1 version
 V4: Rework driver according to Greg Kroah-Hartman' feedback
     Add the ABI doc related to the sysfs implemented on this driver
 V5: Rework driver accordingly to Leon Romanovsky' feedback
     Rework driver accordingly to Krzysztof Wilczyński' feedback
 V6: Rework driver according to Greg Kroah-Hartman' feedback
     Rework driver accordingly to Krzysztof Wilczyński' feedback
     Rework driver accordingly to Leon Romanovsky' feedback
 V7: Rework driver according to Greg Kroah-Hartman' feedback

Cc: linux-doc@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Derek Kiernan <derek.kiernan@xilinx.com>
Cc: Dragan Cvetic <dragan.cvetic@xilinx.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Krzysztof Wilczyński <kw@linux.com>

Gustavo Pimentel (5):
  misc: Add Synopsys DesignWare xData IP driver
  misc: Add Synopsys DesignWare xData IP driver to Makefile and Kconfig
  Documentation: misc-devices: Add Documentation for dw-xdata-pcie
    driver
  MAINTAINERS: Add Synopsys xData IP driver maintainer
  docs: ABI: Add sysfs documentation interface of dw-xdata-pcie driver

 Documentation/ABI/testing/sysfs-driver-xdata |  46 +++
 Documentation/misc-devices/dw-xdata-pcie.rst |  40 +++
 MAINTAINERS                                  |   7 +
 drivers/misc/Kconfig                         |  10 +
 drivers/misc/Makefile                        |   1 +
 drivers/misc/dw-xdata-pcie.c                 | 401 +++++++++++++++++++++++++++
 6 files changed, 505 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-xdata
 create mode 100644 Documentation/misc-devices/dw-xdata-pcie.rst
 create mode 100644 drivers/misc/dw-xdata-pcie.c

-- 
2.7.4

[PATCH v7 1/5] misc: Add Synopsys DesignWare xData IP driver

[Gustavo Pimentel]

Add Synopsys DesignWare xData IP driver. This driver enables/disables
the PCI traffic generator module pertain to the Synopsys DesignWare
prototype.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 drivers/misc/dw-xdata-pcie.c | 401 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 401 insertions(+)
 create mode 100644 drivers/misc/dw-xdata-pcie.c

diff --git a/drivers/misc/dw-xdata-pcie.c b/drivers/misc/dw-xdata-pcie.c
new file mode 100644
index 00000000..43fdd35
--- /dev/null
+++ b/drivers/misc/dw-xdata-pcie.c
@@ -0,0 +1,401 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020 Synopsys, Inc. and/or its affiliates.
+ * Synopsys DesignWare xData driver
+ *
+ * Author: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
+ */
+
+#include <linux/miscdevice.h>
+#include <linux/bitfield.h>
+#include <linux/pci-epf.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/bitops.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+#include <linux/pci.h>
+
+#define DW_XDATA_DRIVER_NAME		"dw-xdata-pcie"
+
+#define DW_XDATA_EP_MEM_OFFSET		0x8000000
+
+struct dw_xdata_pcie_data {
+	/* xData registers location */
+	enum pci_barno			rg_bar;
+	off_t				rg_off;
+	size_t				rg_sz;
+};
+
+static const struct dw_xdata_pcie_data snps_edda_data = {
+	/* xData registers location */
+	.rg_bar				= BAR_0,
+	.rg_off				= 0x00000000,   /*   0 Kbytes */
+	.rg_sz				= 0x0000012c,   /* 300  bytes */
+};
+
+#define STATUS_DONE			BIT(0)
+
+#define CONTROL_DOORBELL		BIT(0)
+#define CONTROL_IS_WRITE		BIT(1)
+#define CONTROL_LENGTH(a)		FIELD_PREP(GENMASK(13, 2), a)
+#define CONTROL_PATTERN_INC		BIT(16)
+#define CONTROL_NO_ADDR_INC		BIT(18)
+
+#define XPERF_CONTROL_ENABLE		BIT(5)
+
+#define BURST_REPEAT			BIT(31)
+#define BURST_VALUE			0x1001
+
+#define PATTERN_VALUE			0x0
+
+struct dw_xdata_regs {
+	u32 addr_lsb;					/* 0x000 */
+	u32 addr_msb;					/* 0x004 */
+	u32 burst_cnt;					/* 0x008 */
+	u32 control;					/* 0x00c */
+	u32 pattern;					/* 0x010 */
+	u32 status;					/* 0x014 */
+	u32 RAM_addr;					/* 0x018 */
+	u32 RAM_port;					/* 0x01c */
+	u32 _reserved0[14];				/* 0x020..0x054 */
+	u32 perf_control;				/* 0x058 */
+	u32 _reserved1[41];				/* 0x05c..0x0fc */
+	u32 wr_cnt_lsb;					/* 0x100 */
+	u32 wr_cnt_msb;					/* 0x104 */
+	u32 rd_cnt_lsb;					/* 0x108 */
+	u32 rd_cnt_msb;					/* 0x10c */
+} __packed;
+
+struct dw_xdata_region {
+	phys_addr_t paddr;				/* physical address */
+	void __iomem *vaddr;				/* virtual address */
+	size_t sz;					/* size */
+};
+
+struct dw_xdata {
+	struct dw_xdata_region rg_region;		/* registers */
+	size_t max_wr_len;				/* max wr xfer len */
+	size_t max_rd_len;				/* max rd xfer len */
+	struct mutex mutex;
+	struct pci_dev *pdev;
+	struct device *dev;
+	struct miscdevice misc_dev;
+};
+
+static inline struct dw_xdata_regs __iomem *__dw_regs(struct dw_xdata *dw)
+{
+	return dw->rg_region.vaddr;
+}
+
+static void dw_xdata_stop(struct dw_xdata *dw)
+{
+	u32 burst;
+
+	mutex_lock(&dw->mutex);
+
+	burst = readl(&(__dw_regs(dw)->burst_cnt));
+
+	if (burst & BURST_REPEAT) {
+		burst &= ~(u32)BURST_REPEAT;
+		writel(burst, &(__dw_regs(dw)->burst_cnt));
+	}
+
+	mutex_unlock(&dw->mutex);
+}
+
+static void dw_xdata_start(struct dw_xdata *dw, bool write)
+{
+	u32 control, status;
+
+	/* Stop first if xfer in progress */
+	dw_xdata_stop(dw);
+
+	mutex_lock(&dw->mutex);
+
+	/* Clear status register */
+	writel(0x0, &(__dw_regs(dw)->status));
+
+	/* Burst count register set for continuous until stopped */
+	writel(BURST_REPEAT | BURST_VALUE, &(__dw_regs(dw)->burst_cnt));
+
+	/* Pattern register */
+	writel(PATTERN_VALUE, &(__dw_regs(dw)->pattern));
+
+	/* Control register */
+	control = CONTROL_DOORBELL | CONTROL_PATTERN_INC | CONTROL_NO_ADDR_INC;
+	if (write) {
+		control |= CONTROL_IS_WRITE;
+		control |= CONTROL_LENGTH(dw->max_wr_len);
+	} else {
+		control |= CONTROL_LENGTH(dw->max_rd_len);
+	}
+	writel(control, &(__dw_regs(dw)->control));
+
+	/*
+	 * The xData HW block needs about 100 ms to initiate the traffic
+	 * generation according this HW block datasheet.
+	 */
+	usleep_range(100, 150);
+
+	status = readl(&(__dw_regs(dw)->status));
+
+	mutex_unlock(&dw->mutex);
+
+	if (!(status & STATUS_DONE))
+		pci_dbg(dw->pdev, "xData: started %s direction\n",
+			write ? "write" : "read");
+}
+
+static void dw_xdata_perf_meas(struct dw_xdata *dw, u64 *data, bool write)
+{
+	if (write) {
+		*data = readl(&(__dw_regs(dw)->wr_cnt_msb));
+		*data <<= 32;
+		*data |= readl(&(__dw_regs(dw)->wr_cnt_lsb));
+	} else {
+		*data = readl(&(__dw_regs(dw)->rd_cnt_msb));
+		*data <<= 32;
+		*data |= readl(&(__dw_regs(dw)->rd_cnt_lsb));
+	}
+}
+
+static u64 dw_xdata_perf_diff(u64 *m1, u64 *m2, u64 time)
+{
+	u64 rate = (*m1 - *m2);
+
+	rate *= (1000 * 1000 * 1000);
+	rate >>= 20;
+	rate = DIV_ROUND_CLOSEST_ULL(rate, time);
+
+	return rate;
+}
+
+static void dw_xdata_perf(struct dw_xdata *dw, u64 *rate, bool write)
+{
+	u64 data[2], time[2], diff;
+
+	mutex_lock(&dw->mutex);
+
+	/* First acquisition of current count frames */
+	writel(0x0, &(__dw_regs(dw)->perf_control));
+	dw_xdata_perf_meas(dw, &data[0], write);
+	time[0] = jiffies;
+	writel((u32)XPERF_CONTROL_ENABLE, &(__dw_regs(dw)->perf_control));
+
+	/*
+	 * Wait 100ms between the 1st count frame acquisition and the 2nd
+	 * count frame acquisition, in order to calculate the speed later
+	 */
+	mdelay(100);
+
+	/* Second acquisition of current count frames */
+	writel(0x0, &(__dw_regs(dw)->perf_control));
+	dw_xdata_perf_meas(dw, &data[1], write);
+	time[1] = jiffies;
+	writel((u32)XPERF_CONTROL_ENABLE, &(__dw_regs(dw)->perf_control));
+
+	/*
+	 * Speed calculation
+	 *
+	 * rate = (2nd count frames - 1st count frames) / (time elapsed)
+	 */
+	diff = jiffies_to_nsecs(time[1] - time[0]);
+	*rate = dw_xdata_perf_diff(&data[1], &data[0], diff);
+
+	mutex_unlock(&dw->mutex);
+
+	pci_dbg(dw->pdev, "xData: time=%llu us, %s=%llu MB/s\n",
+		diff, write ? "write" : "read", *rate);
+}
+
+static struct dw_xdata *misc_dev_to_dw(struct miscdevice *misc_dev)
+{
+	return container_of(misc_dev, struct dw_xdata, misc_dev);
+}
+
+static ssize_t write_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct miscdevice *misc_dev = dev_get_drvdata(dev);
+	struct dw_xdata *dw = misc_dev_to_dw(misc_dev);
+	u64 rate;
+
+	dw_xdata_perf(dw, &rate, true);
+
+	return sysfs_emit(buf, "%llu\n", rate);
+}
+
+static ssize_t write_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t size)
+{
+	struct miscdevice *misc_dev = dev_get_drvdata(dev);
+	struct dw_xdata *dw = misc_dev_to_dw(misc_dev);
+
+	pci_dbg(dw->pdev, "xData: requested write transfer\n");
+
+	dw_xdata_start(dw, true);
+
+	return size;
+}
+
+static DEVICE_ATTR_RW(write);
+
+static ssize_t read_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct miscdevice *misc_dev = dev_get_drvdata(dev);
+	struct dw_xdata *dw = misc_dev_to_dw(misc_dev);
+	u64 rate;
+
+	dw_xdata_perf(dw, &rate, false);
+
+	return sysfs_emit(buf, "%llu\n", rate);
+}
+
+static ssize_t read_store(struct device *dev, struct device_attribute *attr,
+			  const char *buf, size_t size)
+{
+	struct miscdevice *misc_dev = dev_get_drvdata(dev);
+	struct dw_xdata *dw = misc_dev_to_dw(misc_dev);
+
+	pci_dbg(dw->pdev, "xData: requested read transfer\n");
+
+	dw_xdata_start(dw, false);
+
+	return size;
+}
+
+static DEVICE_ATTR_RW(read);
+
+static ssize_t stop_store(struct device *dev, struct device_attribute *attr,
+			  const char *buf, size_t size)
+{
+	struct miscdevice *misc_dev = dev_get_drvdata(dev);
+	struct dw_xdata *dw = misc_dev_to_dw(misc_dev);
+
+	pci_dbg(dw->pdev, "xData: requested stop any transfer\n");
+
+	dw_xdata_stop(dw);
+
+	return size;
+}
+
+static DEVICE_ATTR_WO(stop);
+
+static struct attribute *xdata_attrs[] = {
+	&dev_attr_write.attr,
+	&dev_attr_read.attr,
+	&dev_attr_stop.attr,
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(xdata);
+
+static int dw_xdata_pcie_probe(struct pci_dev *pdev,
+			       const struct pci_device_id *pid)
+{
+	const struct dw_xdata_pcie_data *pdata = (void *)pid->driver_data;
+	struct dw_xdata *dw;
+	u64 addr;
+	int err;
+
+	/* Enable PCI device */
+	err = pcim_enable_device(pdev);
+	if (err) {
+		pci_err(pdev, "enabling device failed\n");
+		return err;
+	}
+
+	/* Mapping PCI BAR regions */
+	err = pcim_iomap_regions(pdev, BIT(pdata->rg_bar), pci_name(pdev));
+	if (err) {
+		pci_err(pdev, "xData BAR I/O remapping failed\n");
+		return err;
+	}
+
+	pci_set_master(pdev);
+
+	/* Allocate memory */
+	dw = devm_kzalloc(&pdev->dev, sizeof(*dw), GFP_KERNEL);
+	if (!dw)
+		return -ENOMEM;
+
+	/* Data structure initialization */
+	mutex_init(&dw->mutex);
+
+	dw->rg_region.vaddr = pcim_iomap_table(pdev)[pdata->rg_bar];
+	if (!dw->rg_region.vaddr)
+		return -ENOMEM;
+
+	dw->rg_region.vaddr += pdata->rg_off;
+	dw->rg_region.paddr = pdev->resource[pdata->rg_bar].start;
+	dw->rg_region.paddr += pdata->rg_off;
+	dw->rg_region.sz = pdata->rg_sz;
+
+	dw->max_wr_len = pcie_get_mps(pdev);
+	dw->max_wr_len >>= 2;
+
+	dw->max_rd_len = pcie_get_readrq(pdev);
+	dw->max_rd_len >>= 2;
+
+	dw->pdev = pdev;
+	dw->dev = &pdev->dev;
+
+	dw->misc_dev.minor = MISC_DYNAMIC_MINOR;
+	dw->misc_dev.name = kstrdup(DW_XDATA_DRIVER_NAME, GFP_KERNEL);
+	dw->misc_dev.parent = &pdev->dev;
+	dw->misc_dev.groups = xdata_groups;
+
+	writel(0x0, &(__dw_regs(dw)->RAM_addr));
+	writel(0x0, &(__dw_regs(dw)->RAM_port));
+
+	addr = dw->rg_region.paddr + DW_XDATA_EP_MEM_OFFSET;
+	writel(lower_32_bits(addr), &(__dw_regs(dw)->addr_lsb));
+	writel(upper_32_bits(addr), &(__dw_regs(dw)->addr_msb));
+	pci_dbg(pdev, "xData: target address = 0x%.16llx\n", addr);
+
+	pci_dbg(pdev, "xData: wr_len = %zu, rd_len = %zu\n",
+		dw->max_wr_len * 4, dw->max_rd_len * 4);
+
+	/* Saving data structure reference */
+	pci_set_drvdata(pdev, dw);
+
+	/* Register misc device */
+	err = misc_register(&dw->misc_dev);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+static void dw_xdata_pcie_remove(struct pci_dev *pdev)
+{
+	struct dw_xdata *dw = pci_get_drvdata(pdev);
+
+	if (dw) {
+		dw_xdata_stop(dw);
+		misc_deregister(&dw->misc_dev);
+	}
+}
+
+static const struct pci_device_id dw_xdata_pcie_id_table[] = {
+	{ PCI_DEVICE_DATA(SYNOPSYS, EDDA, &snps_edda_data) },
+	{ }
+};
+MODULE_DEVICE_TABLE(pci, dw_xdata_pcie_id_table);
+
+static struct pci_driver dw_xdata_pcie_driver = {
+	.name		= DW_XDATA_DRIVER_NAME,
+	.id_table	= dw_xdata_pcie_id_table,
+	.probe		= dw_xdata_pcie_probe,
+	.remove		= dw_xdata_pcie_remove,
+};
+
+module_pci_driver(dw_xdata_pcie_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Synopsys DesignWare xData PCIe driver");
+MODULE_AUTHOR("Gustavo Pimentel <gustavo.pimentel@synopsys.com>");
+
-- 
2.7.4

[PATCH v7 2/5] misc: Add Synopsys DesignWare xData IP driver to Makefile and Kconfig

[Gustavo Pimentel]

Add Synopsys DesignWare xData IP driver to Makefile and Kconfig.

This driver enables/disables the PCIe traffic generator module
pertain to the Synopsys DesignWare prototype.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 drivers/misc/Kconfig  | 10 ++++++++++
 drivers/misc/Makefile |  1 +
 2 files changed, 11 insertions(+)

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index fafa8b0..e42b171 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -423,6 +423,16 @@ config SRAM
 config SRAM_EXEC
 	bool
 
+config DW_XDATA_PCIE
+	depends on PCI
+	tristate "Synopsys DesignWare xData PCIe driver"
+	help
+	  This driver allows controlling Synopsys DesignWare PCIe traffic
+	  generator IP also known as xData, present in Synopsys DesignWare
+	  PCIe Endpoint prototype.
+
+	  If unsure, say N.
+
 config PCI_ENDPOINT_TEST
 	depends on PCI
 	select CRC32
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index d23231e..bf22021 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -49,6 +49,7 @@ obj-$(CONFIG_SRAM_EXEC)		+= sram-exec.o
 obj-$(CONFIG_GENWQE)		+= genwqe/
 obj-$(CONFIG_ECHO)		+= echo/
 obj-$(CONFIG_CXL_BASE)		+= cxl/
+obj-$(CONFIG_DW_XDATA_PCIE)	+= dw-xdata-pcie.o
 obj-$(CONFIG_PCI_ENDPOINT_TEST)	+= pci_endpoint_test.o
 obj-$(CONFIG_OCXL)		+= ocxl/
 obj-y				+= cardreader/
-- 
2.7.4

[PATCH v7 3/5] Documentation: misc-devices: Add Documentation for dw-xdata-pcie driver

[Gustavo Pimentel]

Add Documentation for dw-xdata-pcie driver.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 Documentation/misc-devices/dw-xdata-pcie.rst | 40 ++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/misc-devices/dw-xdata-pcie.rst

diff --git a/Documentation/misc-devices/dw-xdata-pcie.rst b/Documentation/misc-devices/dw-xdata-pcie.rst
new file mode 100644
index 00000000..fd75c93
--- /dev/null
+++ b/Documentation/misc-devices/dw-xdata-pcie.rst
@@ -0,0 +1,40 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===========================================================================
+Driver for Synopsys DesignWare PCIe traffic generator (also known as xData)
+===========================================================================
+
+This driver should be used as a host-side (Root Complex) driver and Synopsys
+DesignWare prototype that includes this IP.
+
+The "dw-xdata-pcie" driver can be used to enable/disable PCIe traffic
+generator in either direction (mutual exclusion) besides allowing the
+PCIe link performance analysis.
+
+The interaction with this driver is done through the module parameter and
+can be changed in runtime. The driver outputs the requested command state
+information to /var/log/kern.log or dmesg.
+
+Request write TLPs traffic generation - Root Complex to Endpoint direction
+- Command:
+	echo 1 > /sys/class/misc/dw-xdata-pcie/write
+
+Get write TLPs traffic link throughput in MB/s
+- Command:
+        cat /sys/class/misc/dw-xdata-pcie/write
+- Output example:
+	204
+
+Request read TLPs traffic generation - Endpoint to Root Complex direction:
+- Command:
+	echo 1 > /sys/class/misc/dw-xdata-pcie/read
+
+Get read TLPs traffic link throughput in MB/s
+- Command:
+        cat /sys/class/misc/dw-xdata-pcie/read
+- Output example:
+	199
+
+Request to stop any current TLP transfer:
+- Command:
+	echo 1 > /sys/class/misc/dw-xdata-pcie/stop
-- 
2.7.4

[PATCH v7 4/5] MAINTAINERS: Add Synopsys xData IP driver maintainer

[Gustavo Pimentel]

Add Synopsys xData IP driver maintainer.

This driver aims to support Synopsys xData IP and is normally distributed
along with Synopsys PCIe EndPoint IP as a PCIe traffic generator (depends
of the use and licensing agreement).

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 546aa66..f9d681b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5061,6 +5061,13 @@ S:	Maintained
 F:	drivers/dma/dw-edma/
 F:	include/linux/dma/edma.h
 
+DESIGNWARE XDATA IP DRIVER
+M:	Gustavo Pimentel <gustavo.pimentel@synopsys.com>
+L:	linux-pci@vger.kernel.org
+S:	Maintained
+F:	Documentation/misc-devices/dw-xdata-pcie.rst
+F:	drivers/misc/dw-xdata-pcie.c
+
 DESIGNWARE USB2 DRD IP DRIVER
 M:	Minas Harutyunyan <hminas@synopsys.com>
 L:	linux-usb@vger.kernel.org
-- 
2.7.4

[PATCH v7 5/5] docs: ABI: Add sysfs documentation interface of dw-xdata-pcie driver

[Gustavo Pimentel]

This patch describes the sysfs interface implemented on the dw-xdata-pcie
driver.

Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
---
 Documentation/ABI/testing/sysfs-driver-xdata | 46 ++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-driver-xdata

diff --git a/Documentation/ABI/testing/sysfs-driver-xdata b/Documentation/ABI/testing/sysfs-driver-xdata
new file mode 100644
index 00000000..cb3ab7e
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-xdata
@@ -0,0 +1,46 @@
+What:		/sys/class/misc/drivers/dw-xdata-pcie/write
+Date:		April 2021
+KernelVersion:	5.12
+Contact:	Gustavo Pimentel <gustavo.pimentel@synopsys.com>
+Description:	Allows the user to enable the PCIe traffic generator which
+		will create write TLPs frames - from the Root Complex to the
+		Endpoint direction.
+		Usage e.g.
+		 echo 1 > /sys/class/misc/dw-xdata-pcie/write
+
+		The user can read the current PCIe link throughput generated
+		through this generator in MB/s.
+		Usage e.g.
+		 cat /sys/class/misc/dw-xdata-pcie/write
+		 204
+
+		The file is read and write.
+
+What:		/sys/class/misc/dw-xdata-pcie/read
+Date:		April 2021
+KernelVersion:	5.12
+Contact:	Gustavo Pimentel <gustavo.pimentel@synopsys.com>
+Description:	Allows the user to enable the PCIe traffic generator which
+		will create read TLPs frames - from the Endpoint to the Root
+		Complex direction.
+		Usage e.g.
+		 echo 1 > /sys/class/misc/dw-xdata-pcie/read
+
+		The user can read the current PCIe link throughput generated
+		through this generator in MB/s.
+		Usage e.g.
+		 cat /sys/class/misc/dw-xdata-pcie/read
+		 199
+
+		The file is read and write.
+
+What:		/sys/class/misc/dw-xdata-pcie/stop
+Date:		April 2021
+KernelVersion:	5.12
+Contact:	Gustavo Pimentel <gustavo.pimentel@synopsys.com>
+Description:	Allows the user to disable the PCIe traffic generator in all
+		directions.
+		Usage e.g.
+		 echo 1 > /sys/class/misc/dw-xdata-pcie/stop
+
+		The file is write only.
-- 
2.7.4

Re: [PATCH v7 0/5] misc: Add Add Synopsys DesignWare xData IP driver

[Greg Kroah-Hartman]

On Sat, Mar 27, 2021 at 04:06:50AM +0100, Gustavo Pimentel wrote:
> This patch series adds a new driver called xData-pcie for the Synopsys
> DesignWare PCIe prototype.
> 
> The driver configures and enables the Synopsys DesignWare PCIe traffic
> generator IP inside of prototype Endpoint which will generate upstream
> and downstream PCIe traffic. This allows to quickly test the PCIe link
> throughput speed and check is the prototype solution has some limitation
> or not.
> 
> Changes:
>  V2: Rework driver according to Greg Kroah-Hartman' feedback
>  V3: Fixed issues detected while running on 64 bits platforms
>      Rebased patches on top of v5.11-rc1 version
>  V4: Rework driver according to Greg Kroah-Hartman' feedback
>      Add the ABI doc related to the sysfs implemented on this driver
>  V5: Rework driver accordingly to Leon Romanovsky' feedback
>      Rework driver accordingly to Krzysztof Wilczyński' feedback
>  V6: Rework driver according to Greg Kroah-Hartman' feedback
>      Rework driver accordingly to Krzysztof Wilczyński' feedback
>      Rework driver accordingly to Leon Romanovsky' feedback
>  V7: Rework driver according to Greg Kroah-Hartman' feedback

This really doesn't help as I know I don't remember what my feedback
was, so I can't know what to look at to see if it was changed properly
:(

Please be more specific next time.

thanks,

greg k-h

Re: [PATCH v7 1/5] misc: Add Synopsys DesignWare xData IP driver

[Greg Kroah-Hartman]

On Sat, Mar 27, 2021 at 04:06:51AM +0100, Gustavo Pimentel wrote:
> Add Synopsys DesignWare xData IP driver. This driver enables/disables
> the PCI traffic generator module pertain to the Synopsys DesignWare
> prototype.
> 
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
>  drivers/misc/dw-xdata-pcie.c | 401 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 401 insertions(+)
>  create mode 100644 drivers/misc/dw-xdata-pcie.c
> 
> diff --git a/drivers/misc/dw-xdata-pcie.c b/drivers/misc/dw-xdata-pcie.c
> new file mode 100644
> index 00000000..43fdd35
> --- /dev/null
> +++ b/drivers/misc/dw-xdata-pcie.c
> @@ -0,0 +1,401 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020 Synopsys, Inc. and/or its affiliates.
> + * Synopsys DesignWare xData driver
> + *
> + * Author: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> + */
> +
> +#include <linux/miscdevice.h>
> +#include <linux/bitfield.h>
> +#include <linux/pci-epf.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/bitops.h>
> +#include <linux/mutex.h>
> +#include <linux/delay.h>
> +#include <linux/pci.h>
> +
> +#define DW_XDATA_DRIVER_NAME		"dw-xdata-pcie"
> +
> +#define DW_XDATA_EP_MEM_OFFSET		0x8000000
> +
> +struct dw_xdata_pcie_data {
> +	/* xData registers location */
> +	enum pci_barno			rg_bar;
> +	off_t				rg_off;
> +	size_t				rg_sz;
> +};
> +
> +static const struct dw_xdata_pcie_data snps_edda_data = {
> +	/* xData registers location */
> +	.rg_bar				= BAR_0,
> +	.rg_off				= 0x00000000,   /*   0 Kbytes */
> +	.rg_sz				= 0x0000012c,   /* 300  bytes */
> +};
> +
> +#define STATUS_DONE			BIT(0)
> +
> +#define CONTROL_DOORBELL		BIT(0)
> +#define CONTROL_IS_WRITE		BIT(1)
> +#define CONTROL_LENGTH(a)		FIELD_PREP(GENMASK(13, 2), a)
> +#define CONTROL_PATTERN_INC		BIT(16)
> +#define CONTROL_NO_ADDR_INC		BIT(18)
> +
> +#define XPERF_CONTROL_ENABLE		BIT(5)
> +
> +#define BURST_REPEAT			BIT(31)
> +#define BURST_VALUE			0x1001
> +
> +#define PATTERN_VALUE			0x0
> +
> +struct dw_xdata_regs {
> +	u32 addr_lsb;					/* 0x000 */
> +	u32 addr_msb;					/* 0x004 */
> +	u32 burst_cnt;					/* 0x008 */
> +	u32 control;					/* 0x00c */
> +	u32 pattern;					/* 0x010 */
> +	u32 status;					/* 0x014 */
> +	u32 RAM_addr;					/* 0x018 */
> +	u32 RAM_port;					/* 0x01c */
> +	u32 _reserved0[14];				/* 0x020..0x054 */
> +	u32 perf_control;				/* 0x058 */
> +	u32 _reserved1[41];				/* 0x05c..0x0fc */
> +	u32 wr_cnt_lsb;					/* 0x100 */
> +	u32 wr_cnt_msb;					/* 0x104 */
> +	u32 rd_cnt_lsb;					/* 0x108 */
> +	u32 rd_cnt_msb;					/* 0x10c */
> +} __packed;
> +
> +struct dw_xdata_region {
> +	phys_addr_t paddr;				/* physical address */
> +	void __iomem *vaddr;				/* virtual address */
> +	size_t sz;					/* size */
> +};
> +
> +struct dw_xdata {
> +	struct dw_xdata_region rg_region;		/* registers */
> +	size_t max_wr_len;				/* max wr xfer len */
> +	size_t max_rd_len;				/* max rd xfer len */
> +	struct mutex mutex;
> +	struct pci_dev *pdev;
> +	struct device *dev;

You do not need this 'struct device' pointer at all, please don't store
it as you are not handling any reference counting correctly.

> +	struct miscdevice misc_dev;
> +};
> +
> +static inline struct dw_xdata_regs __iomem *__dw_regs(struct dw_xdata *dw)
> +{
> +	return dw->rg_region.vaddr;
> +}
> +
> +static void dw_xdata_stop(struct dw_xdata *dw)
> +{
> +	u32 burst;
> +
> +	mutex_lock(&dw->mutex);
> +
> +	burst = readl(&(__dw_regs(dw)->burst_cnt));
> +
> +	if (burst & BURST_REPEAT) {
> +		burst &= ~(u32)BURST_REPEAT;
> +		writel(burst, &(__dw_regs(dw)->burst_cnt));
> +	}
> +
> +	mutex_unlock(&dw->mutex);
> +}
> +
> +static void dw_xdata_start(struct dw_xdata *dw, bool write)
> +{
> +	u32 control, status;
> +
> +	/* Stop first if xfer in progress */
> +	dw_xdata_stop(dw);
> +
> +	mutex_lock(&dw->mutex);
> +
> +	/* Clear status register */
> +	writel(0x0, &(__dw_regs(dw)->status));
> +
> +	/* Burst count register set for continuous until stopped */
> +	writel(BURST_REPEAT | BURST_VALUE, &(__dw_regs(dw)->burst_cnt));
> +
> +	/* Pattern register */
> +	writel(PATTERN_VALUE, &(__dw_regs(dw)->pattern));
> +
> +	/* Control register */
> +	control = CONTROL_DOORBELL | CONTROL_PATTERN_INC | CONTROL_NO_ADDR_INC;
> +	if (write) {
> +		control |= CONTROL_IS_WRITE;
> +		control |= CONTROL_LENGTH(dw->max_wr_len);
> +	} else {
> +		control |= CONTROL_LENGTH(dw->max_rd_len);
> +	}
> +	writel(control, &(__dw_regs(dw)->control));
> +
> +	/*
> +	 * The xData HW block needs about 100 ms to initiate the traffic
> +	 * generation according this HW block datasheet.
> +	 */
> +	usleep_range(100, 150);
> +
> +	status = readl(&(__dw_regs(dw)->status));
> +
> +	mutex_unlock(&dw->mutex);
> +
> +	if (!(status & STATUS_DONE))
> +		pci_dbg(dw->pdev, "xData: started %s direction\n",
> +			write ? "write" : "read");
> +}
> +
> +static void dw_xdata_perf_meas(struct dw_xdata *dw, u64 *data, bool write)
> +{
> +	if (write) {
> +		*data = readl(&(__dw_regs(dw)->wr_cnt_msb));
> +		*data <<= 32;
> +		*data |= readl(&(__dw_regs(dw)->wr_cnt_lsb));
> +	} else {
> +		*data = readl(&(__dw_regs(dw)->rd_cnt_msb));
> +		*data <<= 32;
> +		*data |= readl(&(__dw_regs(dw)->rd_cnt_lsb));
> +	}
> +}
> +
> +static u64 dw_xdata_perf_diff(u64 *m1, u64 *m2, u64 time)
> +{
> +	u64 rate = (*m1 - *m2);
> +
> +	rate *= (1000 * 1000 * 1000);
> +	rate >>= 20;
> +	rate = DIV_ROUND_CLOSEST_ULL(rate, time);
> +
> +	return rate;
> +}
> +
> +static void dw_xdata_perf(struct dw_xdata *dw, u64 *rate, bool write)
> +{
> +	u64 data[2], time[2], diff;
> +
> +	mutex_lock(&dw->mutex);
> +
> +	/* First acquisition of current count frames */
> +	writel(0x0, &(__dw_regs(dw)->perf_control));
> +	dw_xdata_perf_meas(dw, &data[0], write);
> +	time[0] = jiffies;
> +	writel((u32)XPERF_CONTROL_ENABLE, &(__dw_regs(dw)->perf_control));
> +
> +	/*
> +	 * Wait 100ms between the 1st count frame acquisition and the 2nd
> +	 * count frame acquisition, in order to calculate the speed later
> +	 */
> +	mdelay(100);
> +
> +	/* Second acquisition of current count frames */
> +	writel(0x0, &(__dw_regs(dw)->perf_control));
> +	dw_xdata_perf_meas(dw, &data[1], write);
> +	time[1] = jiffies;
> +	writel((u32)XPERF_CONTROL_ENABLE, &(__dw_regs(dw)->perf_control));
> +
> +	/*
> +	 * Speed calculation
> +	 *
> +	 * rate = (2nd count frames - 1st count frames) / (time elapsed)
> +	 */
> +	diff = jiffies_to_nsecs(time[1] - time[0]);
> +	*rate = dw_xdata_perf_diff(&data[1], &data[0], diff);
> +
> +	mutex_unlock(&dw->mutex);
> +
> +	pci_dbg(dw->pdev, "xData: time=%llu us, %s=%llu MB/s\n",
> +		diff, write ? "write" : "read", *rate);
> +}
> +
> +static struct dw_xdata *misc_dev_to_dw(struct miscdevice *misc_dev)
> +{
> +	return container_of(misc_dev, struct dw_xdata, misc_dev);
> +}
> +
> +static ssize_t write_show(struct device *dev, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct miscdevice *misc_dev = dev_get_drvdata(dev);
> +	struct dw_xdata *dw = misc_dev_to_dw(misc_dev);
> +	u64 rate;
> +
> +	dw_xdata_perf(dw, &rate, true);
> +
> +	return sysfs_emit(buf, "%llu\n", rate);
> +}
> +
> +static ssize_t write_store(struct device *dev, struct device_attribute *attr,
> +			   const char *buf, size_t size)
> +{
> +	struct miscdevice *misc_dev = dev_get_drvdata(dev);
> +	struct dw_xdata *dw = misc_dev_to_dw(misc_dev);
> +
> +	pci_dbg(dw->pdev, "xData: requested write transfer\n");
> +
> +	dw_xdata_start(dw, true);
> +
> +	return size;
> +}
> +
> +static DEVICE_ATTR_RW(write);
> +
> +static ssize_t read_show(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct miscdevice *misc_dev = dev_get_drvdata(dev);
> +	struct dw_xdata *dw = misc_dev_to_dw(misc_dev);
> +	u64 rate;
> +
> +	dw_xdata_perf(dw, &rate, false);
> +
> +	return sysfs_emit(buf, "%llu\n", rate);
> +}
> +
> +static ssize_t read_store(struct device *dev, struct device_attribute *attr,
> +			  const char *buf, size_t size)
> +{
> +	struct miscdevice *misc_dev = dev_get_drvdata(dev);
> +	struct dw_xdata *dw = misc_dev_to_dw(misc_dev);
> +
> +	pci_dbg(dw->pdev, "xData: requested read transfer\n");

dev_dbg() for your misc device, not for your pci device, as that will
show the proper device that is causing this to happen for.

> +
> +	dw_xdata_start(dw, false);

You do not even look at the data written?  That feels buggy :(

> +
> +	return size;
> +}
> +
> +static DEVICE_ATTR_RW(read);
> +
> +static ssize_t stop_store(struct device *dev, struct device_attribute *attr,
> +			  const char *buf, size_t size)
> +{
> +	struct miscdevice *misc_dev = dev_get_drvdata(dev);
> +	struct dw_xdata *dw = misc_dev_to_dw(misc_dev);
> +
> +	pci_dbg(dw->pdev, "xData: requested stop any transfer\n");

Same as above.

> +
> +	dw_xdata_stop(dw);

Again, you do not even look at the data?

> +
> +	return size;
> +}
> +
> +static DEVICE_ATTR_WO(stop);
> +
> +static struct attribute *xdata_attrs[] = {
> +	&dev_attr_write.attr,
> +	&dev_attr_read.attr,
> +	&dev_attr_stop.attr,
> +	NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(xdata);
> +
> +static int dw_xdata_pcie_probe(struct pci_dev *pdev,
> +			       const struct pci_device_id *pid)
> +{
> +	const struct dw_xdata_pcie_data *pdata = (void *)pid->driver_data;
> +	struct dw_xdata *dw;
> +	u64 addr;
> +	int err;
> +
> +	/* Enable PCI device */
> +	err = pcim_enable_device(pdev);
> +	if (err) {
> +		pci_err(pdev, "enabling device failed\n");
> +		return err;
> +	}
> +
> +	/* Mapping PCI BAR regions */
> +	err = pcim_iomap_regions(pdev, BIT(pdata->rg_bar), pci_name(pdev));
> +	if (err) {
> +		pci_err(pdev, "xData BAR I/O remapping failed\n");
> +		return err;
> +	}
> +
> +	pci_set_master(pdev);
> +
> +	/* Allocate memory */
> +	dw = devm_kzalloc(&pdev->dev, sizeof(*dw), GFP_KERNEL);
> +	if (!dw)
> +		return -ENOMEM;
> +
> +	/* Data structure initialization */
> +	mutex_init(&dw->mutex);
> +
> +	dw->rg_region.vaddr = pcim_iomap_table(pdev)[pdata->rg_bar];
> +	if (!dw->rg_region.vaddr)
> +		return -ENOMEM;
> +
> +	dw->rg_region.vaddr += pdata->rg_off;
> +	dw->rg_region.paddr = pdev->resource[pdata->rg_bar].start;
> +	dw->rg_region.paddr += pdata->rg_off;
> +	dw->rg_region.sz = pdata->rg_sz;
> +
> +	dw->max_wr_len = pcie_get_mps(pdev);
> +	dw->max_wr_len >>= 2;
> +
> +	dw->max_rd_len = pcie_get_readrq(pdev);
> +	dw->max_rd_len >>= 2;
> +
> +	dw->pdev = pdev;

No reference counting?

> +	dw->dev = &pdev->dev;

As mentioned above, this is not needed at all.

> +
> +	dw->misc_dev.minor = MISC_DYNAMIC_MINOR;
> +	dw->misc_dev.name = kstrdup(DW_XDATA_DRIVER_NAME, GFP_KERNEL);

Where do you free this memory?

> +	dw->misc_dev.parent = &pdev->dev;
> +	dw->misc_dev.groups = xdata_groups;
> +
> +	writel(0x0, &(__dw_regs(dw)->RAM_addr));
> +	writel(0x0, &(__dw_regs(dw)->RAM_port));
> +
> +	addr = dw->rg_region.paddr + DW_XDATA_EP_MEM_OFFSET;
> +	writel(lower_32_bits(addr), &(__dw_regs(dw)->addr_lsb));
> +	writel(upper_32_bits(addr), &(__dw_regs(dw)->addr_msb));
> +	pci_dbg(pdev, "xData: target address = 0x%.16llx\n", addr);
> +
> +	pci_dbg(pdev, "xData: wr_len = %zu, rd_len = %zu\n",
> +		dw->max_wr_len * 4, dw->max_rd_len * 4);
> +
> +	/* Saving data structure reference */
> +	pci_set_drvdata(pdev, dw);
> +
> +	/* Register misc device */
> +	err = misc_register(&dw->misc_dev);
> +	if (err)
> +		return err;
> +
> +	return 0;

How about:
	return misc_register(...);


> +}
> +
> +static void dw_xdata_pcie_remove(struct pci_dev *pdev)
> +{
> +	struct dw_xdata *dw = pci_get_drvdata(pdev);
> +
> +	if (dw) {

How can this ever not be true?  You never set this to NULL so this check
is pointless.

> +		dw_xdata_stop(dw);
> +		misc_deregister(&dw->misc_dev);
> +	}
> +}
> +
> +static const struct pci_device_id dw_xdata_pcie_id_table[] = {
> +	{ PCI_DEVICE_DATA(SYNOPSYS, EDDA, &snps_edda_data) },

Why do you need a pointer to snps_edda_data here?

thanks,

greg k-h

Re: [PATCH v7 2/5] misc: Add Synopsys DesignWare xData IP driver to Makefile and Kconfig

[Greg Kroah-Hartman]

On Sat, Mar 27, 2021 at 04:06:52AM +0100, Gustavo Pimentel wrote:
> Add Synopsys DesignWare xData IP driver to Makefile and Kconfig.
> 
> This driver enables/disables the PCIe traffic generator module
> pertain to the Synopsys DesignWare prototype.
> 
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
>  drivers/misc/Kconfig  | 10 ++++++++++
>  drivers/misc/Makefile |  1 +
>  2 files changed, 11 insertions(+)

This should be part of the patch that adds the driver so that you can
properly bisect things to when problems really happen.

thnaks,

greg k-h

Re: [PATCH v7 5/5] docs: ABI: Add sysfs documentation interface of dw-xdata-pcie driver

[Greg Kroah-Hartman]

On Sat, Mar 27, 2021 at 04:06:55AM +0100, Gustavo Pimentel wrote:
> This patch describes the sysfs interface implemented on the dw-xdata-pcie
> driver.
> 
> Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> ---
>  Documentation/ABI/testing/sysfs-driver-xdata | 46 ++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-driver-xdata
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-xdata b/Documentation/ABI/testing/sysfs-driver-xdata
> new file mode 100644
> index 00000000..cb3ab7e
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-xdata
> @@ -0,0 +1,46 @@
> +What:		/sys/class/misc/drivers/dw-xdata-pcie/write
> +Date:		April 2021
> +KernelVersion:	5.12
> +Contact:	Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> +Description:	Allows the user to enable the PCIe traffic generator which
> +		will create write TLPs frames - from the Root Complex to the
> +		Endpoint direction.
> +		Usage e.g.
> +		 echo 1 > /sys/class/misc/dw-xdata-pcie/write

That did not look like what the code was looking for at all :(


> +
> +		The user can read the current PCIe link throughput generated
> +		through this generator in MB/s.
> +		Usage e.g.
> +		 cat /sys/class/misc/dw-xdata-pcie/write
> +		 204
> +
> +		The file is read and write.
> +
> +What:		/sys/class/misc/dw-xdata-pcie/read
> +Date:		April 2021
> +KernelVersion:	5.12
> +Contact:	Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> +Description:	Allows the user to enable the PCIe traffic generator which
> +		will create read TLPs frames - from the Endpoint to the Root
> +		Complex direction.
> +		Usage e.g.
> +		 echo 1 > /sys/class/misc/dw-xdata-pcie/read

Again, did not match the code :(


> +
> +		The user can read the current PCIe link throughput generated
> +		through this generator in MB/s.
> +		Usage e.g.
> +		 cat /sys/class/misc/dw-xdata-pcie/read
> +		 199
> +
> +		The file is read and write.
> +
> +What:		/sys/class/misc/dw-xdata-pcie/stop
> +Date:		April 2021
> +KernelVersion:	5.12
> +Contact:	Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> +Description:	Allows the user to disable the PCIe traffic generator in all
> +		directions.
> +		Usage e.g.
> +		 echo 1 > /sys/class/misc/dw-xdata-pcie/stop

Same here :(

Who tested this?

RE: [PATCH v7 1/5] misc: Add Synopsys DesignWare xData IP driver

[Gustavo Pimentel]

On Sun, Mar 28, 2021 at 13:49:13, Greg Kroah-Hartman 
<gregkh@linuxfoundation.org> wrote:

> On Sat, Mar 27, 2021 at 04:06:51AM +0100, Gustavo Pimentel wrote:
> > Add Synopsys DesignWare xData IP driver. This driver enables/disables
> > the PCI traffic generator module pertain to the Synopsys DesignWare
> > prototype.
> > 
> > Signed-off-by: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> > ---
> >  drivers/misc/dw-xdata-pcie.c | 401 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 401 insertions(+)
> >  create mode 100644 drivers/misc/dw-xdata-pcie.c
> > 
> > diff --git a/drivers/misc/dw-xdata-pcie.c b/drivers/misc/dw-xdata-pcie.c
> > new file mode 100644
> > index 00000000..43fdd35
> > --- /dev/null
> > +++ b/drivers/misc/dw-xdata-pcie.c
> > @@ -0,0 +1,401 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2020 Synopsys, Inc. and/or its affiliates.
> > + * Synopsys DesignWare xData driver
> > + *
> > + * Author: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
> > + */
> > +
> > +#include <linux/miscdevice.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/pci-epf.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/device.h>
> > +#include <linux/bitops.h>
> > +#include <linux/mutex.h>
> > +#include <linux/delay.h>
> > +#include <linux/pci.h>
> > +
> > +#define DW_XDATA_DRIVER_NAME		"dw-xdata-pcie"
> > +
> > +#define DW_XDATA_EP_MEM_OFFSET		0x8000000
> > +
> > +struct dw_xdata_pcie_data {
> > +	/* xData registers location */
> > +	enum pci_barno			rg_bar;
> > +	off_t				rg_off;
> > +	size_t				rg_sz;
> > +};
> > +
> > +static const struct dw_xdata_pcie_data snps_edda_data = {
> > +	/* xData registers location */
> > +	.rg_bar				= BAR_0,
> > +	.rg_off				= 0x00000000,   /*   0 Kbytes */
> > +	.rg_sz				= 0x0000012c,   /* 300  bytes */
> > +};
> > +
> > +#define STATUS_DONE			BIT(0)
> > +
> > +#define CONTROL_DOORBELL		BIT(0)
> > +#define CONTROL_IS_WRITE		BIT(1)
> > +#define CONTROL_LENGTH(a)		FIELD_PREP(GENMASK(13, 2), a)
> > +#define CONTROL_PATTERN_INC		BIT(16)
> > +#define CONTROL_NO_ADDR_INC		BIT(18)
> > +
> > +#define XPERF_CONTROL_ENABLE		BIT(5)
> > +
> > +#define BURST_REPEAT			BIT(31)
> > +#define BURST_VALUE			0x1001
> > +
> > +#define PATTERN_VALUE			0x0
> > +
> > +struct dw_xdata_regs {
> > +	u32 addr_lsb;					/* 0x000 */
> > +	u32 addr_msb;					/* 0x004 */
> > +	u32 burst_cnt;					/* 0x008 */
> > +	u32 control;					/* 0x00c */
> > +	u32 pattern;					/* 0x010 */
> > +	u32 status;					/* 0x014 */
> > +	u32 RAM_addr;					/* 0x018 */
> > +	u32 RAM_port;					/* 0x01c */
> > +	u32 _reserved0[14];				/* 0x020..0x054 */
> > +	u32 perf_control;				/* 0x058 */
> > +	u32 _reserved1[41];				/* 0x05c..0x0fc */
> > +	u32 wr_cnt_lsb;					/* 0x100 */
> > +	u32 wr_cnt_msb;					/* 0x104 */
> > +	u32 rd_cnt_lsb;					/* 0x108 */
> > +	u32 rd_cnt_msb;					/* 0x10c */
> > +} __packed;
> > +
> > +struct dw_xdata_region {
> > +	phys_addr_t paddr;				/* physical address */
> > +	void __iomem *vaddr;				/* virtual address */
> > +	size_t sz;					/* size */
> > +};
> > +
> > +struct dw_xdata {
> > +	struct dw_xdata_region rg_region;		/* registers */
> > +	size_t max_wr_len;				/* max wr xfer len */
> > +	size_t max_rd_len;				/* max rd xfer len */
> > +	struct mutex mutex;
> > +	struct pci_dev *pdev;
> > +	struct device *dev;
> 
> You do not need this 'struct device' pointer at all, please don't store
> it as you are not handling any reference counting correctly.

Agreed.

> 
> > +	struct miscdevice misc_dev;
> > +};
> > +
> > +static inline struct dw_xdata_regs __iomem *__dw_regs(struct dw_xdata *dw)
> > +{
> > +	return dw->rg_region.vaddr;
> > +}
> > +
> > +static void dw_xdata_stop(struct dw_xdata *dw)
> > +{
> > +	u32 burst;
> > +
> > +	mutex_lock(&dw->mutex);
> > +
> > +	burst = readl(&(__dw_regs(dw)->burst_cnt));
> > +
> > +	if (burst & BURST_REPEAT) {
> > +		burst &= ~(u32)BURST_REPEAT;
> > +		writel(burst, &(__dw_regs(dw)->burst_cnt));
> > +	}
> > +
> > +	mutex_unlock(&dw->mutex);
> > +}
> > +
> > +static void dw_xdata_start(struct dw_xdata *dw, bool write)
> > +{
> > +	u32 control, status;
> > +
> > +	/* Stop first if xfer in progress */
> > +	dw_xdata_stop(dw);
> > +
> > +	mutex_lock(&dw->mutex);
> > +
> > +	/* Clear status register */
> > +	writel(0x0, &(__dw_regs(dw)->status));
> > +
> > +	/* Burst count register set for continuous until stopped */
> > +	writel(BURST_REPEAT | BURST_VALUE, &(__dw_regs(dw)->burst_cnt));
> > +
> > +	/* Pattern register */
> > +	writel(PATTERN_VALUE, &(__dw_regs(dw)->pattern));
> > +
> > +	/* Control register */
> > +	control = CONTROL_DOORBELL | CONTROL_PATTERN_INC | CONTROL_NO_ADDR_INC;
> > +	if (write) {
> > +		control |= CONTROL_IS_WRITE;
> > +		control |= CONTROL_LENGTH(dw->max_wr_len);
> > +	} else {
> > +		control |= CONTROL_LENGTH(dw->max_rd_len);
> > +	}
> > +	writel(control, &(__dw_regs(dw)->control));
> > +
> > +	/*
> > +	 * The xData HW block needs about 100 ms to initiate the traffic
> > +	 * generation according this HW block datasheet.
> > +	 */
> > +	usleep_range(100, 150);
> > +
> > +	status = readl(&(__dw_regs(dw)->status));
> > +
> > +	mutex_unlock(&dw->mutex);
> > +
> > +	if (!(status & STATUS_DONE))
> > +		pci_dbg(dw->pdev, "xData: started %s direction\n",
> > +			write ? "write" : "read");
> > +}
> > +
> > +static void dw_xdata_perf_meas(struct dw_xdata *dw, u64 *data, bool write)
> > +{
> > +	if (write) {
> > +		*data = readl(&(__dw_regs(dw)->wr_cnt_msb));
> > +		*data <<= 32;
> > +		*data |= readl(&(__dw_regs(dw)->wr_cnt_lsb));
> > +	} else {
> > +		*data = readl(&(__dw_regs(dw)->rd_cnt_msb));
> > +		*data <<= 32;
> > +		*data |= readl(&(__dw_regs(dw)->rd_cnt_lsb));
> > +	}
> > +}
> > +
> > +static u64 dw_xdata_perf_diff(u64 *m1, u64 *m2, u64 time)
> > +{
> > +	u64 rate = (*m1 - *m2);
> > +
> > +	rate *= (1000 * 1000 * 1000);
> > +	rate >>= 20;
> > +	rate = DIV_ROUND_CLOSEST_ULL(rate, time);
> > +
> > +	return rate;
> > +}
> > +
> > +static void dw_xdata_perf(struct dw_xdata *dw, u64 *rate, bool write)
> > +{
> > +	u64 data[2], time[2], diff;
> > +
> > +	mutex_lock(&dw->mutex);
> > +
> > +	/* First acquisition of current count frames */
> > +	writel(0x0, &(__dw_regs(dw)->perf_control));
> > +	dw_xdata_perf_meas(dw, &data[0], write);
> > +	time[0] = jiffies;
> > +	writel((u32)XPERF_CONTROL_ENABLE, &(__dw_regs(dw)->perf_control));
> > +
> > +	/*
> > +	 * Wait 100ms between the 1st count frame acquisition and the 2nd
> > +	 * count frame acquisition, in order to calculate the speed later
> > +	 */
> > +	mdelay(100);
> > +
> > +	/* Second acquisition of current count frames */
> > +	writel(0x0, &(__dw_regs(dw)->perf_control));
> > +	dw_xdata_perf_meas(dw, &data[1], write);
> > +	time[1] = jiffies;
> > +	writel((u32)XPERF_CONTROL_ENABLE, &(__dw_regs(dw)->perf_control));
> > +
> > +	/*
> > +	 * Speed calculation
> > +	 *
> > +	 * rate = (2nd count frames - 1st count frames) / (time elapsed)
> > +	 */
> > +	diff = jiffies_to_nsecs(time[1] - time[0]);
> > +	*rate = dw_xdata_perf_diff(&data[1], &data[0], diff);
> > +
> > +	mutex_unlock(&dw->mutex);
> > +
> > +	pci_dbg(dw->pdev, "xData: time=%llu us, %s=%llu MB/s\n",
> > +		diff, write ? "write" : "read", *rate);
> > +}
> > +
> > +static struct dw_xdata *misc_dev_to_dw(struct miscdevice *misc_dev)
> > +{
> > +	return container_of(misc_dev, struct dw_xdata, misc_dev);
> > +}
> > +
> > +static ssize_t write_show(struct device *dev, struct device_attribute *attr,
> > +			  char *buf)
> > +{
> > +	struct miscdevice *misc_dev = dev_get_drvdata(dev);
> > +	struct dw_xdata *dw = misc_dev_to_dw(misc_dev);
> > +	u64 rate;
> > +
> > +	dw_xdata_perf(dw, &rate, true);
> > +
> > +	return sysfs_emit(buf, "%llu\n", rate);
> > +}
> > +
> > +static ssize_t write_store(struct device *dev, struct device_attribute *attr,
> > +			   const char *buf, size_t size)
> > +{
> > +	struct miscdevice *misc_dev = dev_get_drvdata(dev);
> > +	struct dw_xdata *dw = misc_dev_to_dw(misc_dev);
> > +
> > +	pci_dbg(dw->pdev, "xData: requested write transfer\n");
> > +
> > +	dw_xdata_start(dw, true);
> > +
> > +	return size;
> > +}
> > +
> > +static DEVICE_ATTR_RW(write);
> > +
> > +static ssize_t read_show(struct device *dev, struct device_attribute *attr,
> > +			 char *buf)
> > +{
> > +	struct miscdevice *misc_dev = dev_get_drvdata(dev);
> > +	struct dw_xdata *dw = misc_dev_to_dw(misc_dev);
> > +	u64 rate;
> > +
> > +	dw_xdata_perf(dw, &rate, false);
> > +
> > +	return sysfs_emit(buf, "%llu\n", rate);
> > +}
> > +
> > +static ssize_t read_store(struct device *dev, struct device_attribute *attr,
> > +			  const char *buf, size_t size)
> > +{
> > +	struct miscdevice *misc_dev = dev_get_drvdata(dev);
> > +	struct dw_xdata *dw = misc_dev_to_dw(misc_dev);
> > +
> > +	pci_dbg(dw->pdev, "xData: requested read transfer\n");
> 
> dev_dbg() for your misc device, not for your pci device, as that will
> show the proper device that is causing this to happen for.

Ok, I will do a general replacement.

> 
> > +
> > +	dw_xdata_start(dw, false);
> 
> You do not even look at the data written?  That feels buggy :(

By data written, you mean the content of buf?

For this particular use case, I don't think that I would need that.
The goal was just to provide a way to trigger/initiate the PCI traffic 
generator on the read direction, which doesn't require to have any 
particular value, that's why it doesn't check the input value.
On ABI documentation I've given the example "echo 1 > 
/sys/class/misc/dw-xdata-pcie/read", but it could be "echo abc > 
/sys/class/misc/dw-xdata-pcie/read".

The same applies to "write" and "stop" on the store methods.

Perhaps it might exist a better way to do this kind of operations, any 
suggestions?

Of course, I could merge the "write", "read" and "stop" in just one 
device attribute as "command", but I think it will be more complex to 
understand and work with it.

> 
> > +
> > +	return size;
> > +}
> > +
> > +static DEVICE_ATTR_RW(read);
> > +
> > +static ssize_t stop_store(struct device *dev, struct device_attribute *attr,
> > +			  const char *buf, size_t size)
> > +{
> > +	struct miscdevice *misc_dev = dev_get_drvdata(dev);
> > +	struct dw_xdata *dw = misc_dev_to_dw(misc_dev);
> > +
> > +	pci_dbg(dw->pdev, "xData: requested stop any transfer\n");
> 
> Same as above.
> 
> > +
> > +	dw_xdata_stop(dw);
> 
> Again, you do not even look at the data?
> 
> > +
> > +	return size;
> > +}
> > +
> > +static DEVICE_ATTR_WO(stop);
> > +
> > +static struct attribute *xdata_attrs[] = {
> > +	&dev_attr_write.attr,
> > +	&dev_attr_read.attr,
> > +	&dev_attr_stop.attr,
> > +	NULL,
> > +};
> > +
> > +ATTRIBUTE_GROUPS(xdata);
> > +
> > +static int dw_xdata_pcie_probe(struct pci_dev *pdev,
> > +			       const struct pci_device_id *pid)
> > +{
> > +	const struct dw_xdata_pcie_data *pdata = (void *)pid->driver_data;
> > +	struct dw_xdata *dw;
> > +	u64 addr;
> > +	int err;
> > +
> > +	/* Enable PCI device */
> > +	err = pcim_enable_device(pdev);
> > +	if (err) {
> > +		pci_err(pdev, "enabling device failed\n");
> > +		return err;
> > +	}
> > +
> > +	/* Mapping PCI BAR regions */
> > +	err = pcim_iomap_regions(pdev, BIT(pdata->rg_bar), pci_name(pdev));
> > +	if (err) {
> > +		pci_err(pdev, "xData BAR I/O remapping failed\n");
> > +		return err;
> > +	}
> > +
> > +	pci_set_master(pdev);
> > +
> > +	/* Allocate memory */
> > +	dw = devm_kzalloc(&pdev->dev, sizeof(*dw), GFP_KERNEL);
> > +	if (!dw)
> > +		return -ENOMEM;
> > +
> > +	/* Data structure initialization */
> > +	mutex_init(&dw->mutex);
> > +
> > +	dw->rg_region.vaddr = pcim_iomap_table(pdev)[pdata->rg_bar];
> > +	if (!dw->rg_region.vaddr)
> > +		return -ENOMEM;
> > +
> > +	dw->rg_region.vaddr += pdata->rg_off;
> > +	dw->rg_region.paddr = pdev->resource[pdata->rg_bar].start;
> > +	dw->rg_region.paddr += pdata->rg_off;
> > +	dw->rg_region.sz = pdata->rg_sz;
> > +
> > +	dw->max_wr_len = pcie_get_mps(pdev);
> > +	dw->max_wr_len >>= 2;
> > +
> > +	dw->max_rd_len = pcie_get_readrq(pdev);
> > +	dw->max_rd_len >>= 2;
> > +
> > +	dw->pdev = pdev;
> 
> No reference counting?

Since this driver was developed for internal testing purposes and 
normally it will be used just with one prototype endpoint, I didn't think 
on that, but I'll include that on v8.

> 
> > +	dw->dev = &pdev->dev;
> 
> As mentioned above, this is not needed at all.

Ok.

> 
> > +
> > +	dw->misc_dev.minor = MISC_DYNAMIC_MINOR;
> > +	dw->misc_dev.name = kstrdup(DW_XDATA_DRIVER_NAME, GFP_KERNEL);
> 
> Where do you free this memory?

It's not being done, I noticed it after sending the patch series. On v8 
this will be fixed.

> 
> > +	dw->misc_dev.parent = &pdev->dev;
> > +	dw->misc_dev.groups = xdata_groups;
> > +
> > +	writel(0x0, &(__dw_regs(dw)->RAM_addr));
> > +	writel(0x0, &(__dw_regs(dw)->RAM_port));
> > +
> > +	addr = dw->rg_region.paddr + DW_XDATA_EP_MEM_OFFSET;
> > +	writel(lower_32_bits(addr), &(__dw_regs(dw)->addr_lsb));
> > +	writel(upper_32_bits(addr), &(__dw_regs(dw)->addr_msb));
> > +	pci_dbg(pdev, "xData: target address = 0x%.16llx\n", addr);
> > +
> > +	pci_dbg(pdev, "xData: wr_len = %zu, rd_len = %zu\n",
> > +		dw->max_wr_len * 4, dw->max_rd_len * 4);
> > +
> > +	/* Saving data structure reference */
> > +	pci_set_drvdata(pdev, dw);
> > +
> > +	/* Register misc device */
> > +	err = misc_register(&dw->misc_dev);
> > +	if (err)
> > +		return err;
> > +
> > +	return 0;
> 
> How about:
> 	return misc_register(...);

I've reworked this part to include the kfree of the variable 
dw->misc_dev.name

> 
> 
> > +}
> > +
> > +static void dw_xdata_pcie_remove(struct pci_dev *pdev)
> > +{
> > +	struct dw_xdata *dw = pci_get_drvdata(pdev);
> > +
> > +	if (dw) {
> 
> How can this ever not be true?  You never set this to NULL so this check
> is pointless.

It will be removed.

> 
> > +		dw_xdata_stop(dw);
> > +		misc_deregister(&dw->misc_dev);
> > +	}
> > +}
> > +
> > +static const struct pci_device_id dw_xdata_pcie_id_table[] = {
> > +	{ PCI_DEVICE_DATA(SYNOPSYS, EDDA, &snps_edda_data) },
> 
> Why do you need a pointer to snps_edda_data here?

The structure snps_edda_data indicates the location of this IP block (BAR 
and offset) for this particular endpoint.
It's very likely in the future to be more variants that for HW design 
reasons might require this IP block to be on a different location.

-Gustavo
> 
> thanks,
> 
> greg k-h

Re: [PATCH v7 1/5] misc: Add Synopsys DesignWare xData IP driver

[Greg Kroah-Hartman]

On Sun, Mar 28, 2021 at 09:06:47PM +0000, Gustavo Pimentel wrote:
> > > +static const struct pci_device_id dw_xdata_pcie_id_table[] = {
> > > +	{ PCI_DEVICE_DATA(SYNOPSYS, EDDA, &snps_edda_data) },
> > 
> > Why do you need a pointer to snps_edda_data here?
> 
> The structure snps_edda_data indicates the location of this IP block (BAR 
> and offset) for this particular endpoint.
> It's very likely in the future to be more variants that for HW design 
> reasons might require this IP block to be on a different location.

Then make the change when that happens sometime in the future.  Don't
add unneeded complexity today, that just makes the code harder to review
by us now, and for you to maintain today.

thanks,

greg k-h