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

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

[Gustavo Pimentel]

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.

Cc: Derek Kiernan <derek.kiernan@xilinx.com>
Cc: Dragan Cvetic <dragan.cvetic@xilinx.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: linux-pci@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Gustavo Pimentel (5):
  misc: Add Synopsys DesignWare xData IP driver
  misc: Add Synopsys DesignWare xData IP driver to Makefile
  misc: Add Synopsys DesignWare xData IP driver to Kconfig
  Documentation: misc-devices: Add Documentation for dw-xdata-pcie
    driver
  MAINTAINERS: Add Synopsys xData IP driver maintainer

 Documentation/misc-devices/dw-xdata-pcie.rst |  43 +++
 MAINTAINERS                                  |   7 +
 drivers/misc/Kconfig                         |  10 +
 drivers/misc/Makefile                        |   1 +
 drivers/misc/dw-xdata-pcie.c                 | 395 +++++++++++++++++++++++++++
 5 files changed, 456 insertions(+)
 create mode 100644 Documentation/misc-devices/dw-xdata-pcie.rst
 create mode 100644 drivers/misc/dw-xdata-pcie.c

-- 
2.7.4

[PATCH 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 | 395 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 395 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..b529dae
--- /dev/null
+++ b/drivers/misc/dw-xdata-pcie.c
@@ -0,0 +1,395 @@
+// 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/pci-epf.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/pci.h>
+
+#define DW_XDATA_EP_MEM_OFFSET		0x8000000
+
+static DEFINE_MUTEX(xdata_lock);
+
+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 */
+};
+
+static int dw_xdata_command_set(const char *val, const struct kernel_param *kp);
+static const struct kernel_param_ops xdata_command_ops = {
+	.set = dw_xdata_command_set,
+};
+
+static char command;
+module_param_cb(command, &xdata_command_ops, &command, 0644);
+MODULE_PARM_DESC(command, "xData command");
+
+static struct pci_dev *priv;
+
+union dw_xdata_control_reg {
+	u32 reg;
+	struct {
+		u32 doorbell    : 1;			/* 00 */
+		u32 is_write    : 1;			/* 01 */
+		u32 length      : 12;			/* 02..13 */
+		u32 is_64       : 1;			/* 14 */
+		u32 ep		: 1;			/* 15 */
+		u32 pattern_inc : 1;			/* 16 */
+		u32 ie		: 1;			/* 17 */
+		u32 no_addr_inc : 1;			/* 18 */
+		u32 trigger     : 1;			/* 19 */
+		u32 _reserved0  : 12;			/* 20..31 */
+	};
+} __packed;
+
+union dw_xdata_status_reg {
+	u32 reg;
+	struct {
+		u32 done	: 1;			/* 00 */
+		u32 _reserved0  : 15;			/* 01..15 */
+		u32 version     : 16;			/* 16..31 */
+	};
+} __packed;
+
+union dw_xdata_xperf_control_reg {
+	u32 reg;
+	struct {
+		u32 _reserved0  : 4;			/* 00..03 */
+		u32 reset       : 1;			/* 04 */
+		u32 enable      : 1;			/* 05 */
+		u32 _reserved1  : 26;			/* 06..31 */
+	};
+} __packed;
+
+union _addr {
+	u64 reg;
+	struct {
+		u32 lsb;
+		u32 msb;
+	};
+};
+
+struct dw_xdata_regs {
+	union _addr addr;				/* 0x000..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 */
+	union _addr wr_cnt;				/* 0x100..0x104 */
+	union _addr rd_cnt;				/* 0x108..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 xfer len */
+	size_t max_rd_len;				/* max xfer len */
+};
+
+static inline struct dw_xdata_regs __iomem *__dw_regs(struct dw_xdata *dw)
+{
+	return dw->rg_region.vaddr;
+}
+
+#define SET(dw, name, value) \
+	writel(value, &(__dw_regs(dw)->name))
+
+#define GET(dw, name) \
+	readl(&(__dw_regs(dw)->name))
+
+#ifdef CONFIG_64BIT
+#define SET_64(dw, name, value) \
+	writel(value, &(__dw_regs(dw)->name))
+
+#define GET_64(dw, name) \
+	readq(&(__dw_regs(dw)->name))
+#endif /* CONFIG_64BIT */
+
+static void dw_xdata_stop(struct pci_dev *pdev)
+{
+	struct dw_xdata *dw = pci_get_drvdata(pdev);
+	u32 tmp = GET(dw, burst_cnt);
+
+	if (tmp & 0x80000000) {
+		tmp &= 0x7fffffff;
+		SET(dw, burst_cnt, tmp);
+	}
+}
+
+static void dw_xdata_start(struct pci_dev *pdev, bool write)
+{
+	struct dw_xdata *dw = pci_get_drvdata(pdev);
+	union dw_xdata_control_reg control;
+	union dw_xdata_status_reg status;
+
+	/* Stop first if xfer in progress */
+	dw_xdata_stop(pdev);
+
+	/* Clear status register */
+	SET(dw, status, 0x0);
+
+	/* Burst count register set for continuous until stopped */
+	SET(dw, burst_cnt, 0x80001001);
+
+	/* Pattern register */
+	SET(dw, pattern, 0x0);
+
+	/* Control register */
+	control.reg = 0x0;
+	control.doorbell = true;
+	control.is_write = write;
+	if (write)
+		control.length = dw->max_wr_len;
+	else
+		control.length = dw->max_rd_len;
+	control.pattern_inc = true;
+	control.no_addr_inc = true;
+	SET(dw, control, control.reg);
+
+	usleep_range(100, 150);
+
+	status.reg = GET(dw, status);
+	if (!status.done)
+		pci_info(pdev, "xData: started %s direction\n",
+			 write ? "write" : "read");
+}
+
+static u64 dw_xdata_perf_meas(struct pci_dev *pdev, u64 *wr, u64 *rd)
+{
+	struct dw_xdata *dw = pci_get_drvdata(pdev);
+
+	#ifdef CONFIG_64BIT
+		*wr = GET_64(dw, wr_cnt.reg);
+
+		*rd = GET_64(dw, rd_cnt.reg);
+	#else /* CONFIG_64BIT */
+		*wr = GET(dw, wr_cnt.msb);
+		*wr <<= 32;
+		*wr |= GET(dw, wr_cnt.lsb);
+
+		*rd = GET(dw, rd_cnt.msb);
+		*rd <<= 32;
+		*rd |= GET(dw, rd_cnt.lsb);
+	#endif /* CONFIG_64BIT */
+
+	return jiffies;
+}
+
+static u64 dw_xdata_perf_diff(u64 *m1, u64 *m2, u64 time)
+{
+	u64 rate;
+
+	rate = (*m1 - *m2);
+	rate *= (1000 * 1000 * 1000);
+	rate >>= 20;
+	rate = DIV_ROUND_CLOSEST_ULL(rate, time);
+
+	return rate;
+}
+
+static void dw_xdata_perf(struct pci_dev *pdev)
+{
+	struct dw_xdata *dw = pci_get_drvdata(pdev);
+	union dw_xdata_xperf_control_reg control;
+	u64 wr[2], rd[2], time[2], rate[2], diff;
+
+	/* First measurement */
+	control.reg = 0x0;
+	control.enable = false;
+	SET(dw, perf_control, control.reg);
+	time[0] = dw_xdata_perf_meas(pdev, &wr[0], &rd[0]);
+	control.enable = true;
+	SET(dw, perf_control, control.reg);
+
+	/* Delay 100ms */
+	mdelay(100);
+
+	/* Second measurement */
+	control.reg = 0x0;
+	control.enable = false;
+	SET(dw, perf_control, control.reg);
+	time[1] = dw_xdata_perf_meas(pdev, &wr[1], &rd[1]);
+	control.enable = true;
+	SET(dw, perf_control, control.reg);
+
+	/* Calculations */
+	diff = jiffies_to_nsecs(time[1] - time[0]);
+
+	/* Write performance */
+	rate[0] = dw_xdata_perf_diff(&wr[1], &wr[0], diff);
+
+	/* Read performance */
+	rate[1] = dw_xdata_perf_diff(&rd[1], &rd[0], diff);
+
+	pci_info(pdev,
+		 "xData: time=%llu us, write=%llu MB/s, read=%llu MB/s\n",
+		 diff, rate[0], rate[1]);
+}
+
+static int dw_xdata_command_set(const char *val, const struct kernel_param *kp)
+{
+	int ret = -EBUSY;
+
+	mutex_lock(&xdata_lock);
+	if (!priv)
+		goto err;
+
+	ret = param_set_charp(val, kp);
+	if (ret)
+		goto err;
+
+	switch (*val) {
+	case 'w':
+	case 'W':
+		pci_info(priv, "xData: requested write transfer\n");
+		dw_xdata_start(priv, true);
+		break;
+	case 'r':
+	case 'R':
+		pci_info(priv, "xData: requested read transfer\n");
+		dw_xdata_start(priv, false);
+		break;
+	case 'p':
+	case 'P':
+		pci_info(priv, "xData: requested performance analysis\n");
+		dw_xdata_perf(priv);
+		break;
+	default:
+		pci_info(priv, "xData: requested stop any transfer\n");
+		dw_xdata_stop(priv);
+		break;
+	}
+
+err:
+	mutex_unlock(&xdata_lock);
+
+	return ret;
+}
+
+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 device *dev = &pdev->dev;
+	struct dw_xdata *dw;
+	u64 addr;
+	int err;
+
+	priv = NULL;
+
+	/* 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(dev, sizeof(*dw), GFP_KERNEL);
+	if (!dw)
+		return -ENOMEM;
+
+	/* Data structure initialization */
+	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;
+
+	SET(dw, RAM_addr, 0x0);
+	SET(dw, RAM_port, 0x0);
+
+	addr = dw->rg_region.paddr + DW_XDATA_EP_MEM_OFFSET;
+#ifdef CONFIG_64BIT
+	SET_64(dw, addr.reg, addr);
+#else /* CONFIG_64BIT */
+	SET(dw, addr.lsb, lower_32_bits(addr));
+	SET(dw, addr.msb, upper_32_bits(addr));
+#endif /* CONFIG_64BIT */
+	pci_info(pdev, "xData: target address = 0x%.16llx\n", addr);
+
+	pci_info(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);
+	priv = pdev;
+
+	return 0;
+}
+
+static void dw_xdata_pcie_remove(struct pci_dev *pdev)
+{
+	if (priv) {
+		mutex_lock(&xdata_lock);
+
+		dw_xdata_stop(priv);
+		priv = NULL;
+
+		mutex_unlock(&xdata_lock);
+	}
+}
+
+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-pcie",
+	.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 2/5] misc: Add Synopsys DesignWare xData IP driver to Makefile

[Gustavo Pimentel]

Add Synopsys DesignWare xData IP driver to Makefile.

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/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index c7bd01a..88a83f0 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -50,6 +50,7 @@ obj-y				+= mic/
 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 3/5] misc: Add Synopsys DesignWare xData IP driver to Kconfig

[Gustavo Pimentel]

Add Synopsys DesignWare xData IP driver to 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 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index ce136d6..851b460 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
-- 
2.7.4

[PATCH 4/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 | 43 ++++++++++++++++++++++++++++
 1 file changed, 43 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..4d4616e
--- /dev/null
+++ b/Documentation/misc-devices/dw-xdata-pcie.rst
@@ -0,0 +1,43 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===========================================================================w
+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 w > /sys/module/dw_xdata_pcie/parameters/command
+- Output example:
+	dw-xdata-pcie 0000:01.0: xData: requested write transfer
+	dw-xdata-pcie 0000:01.0: xData: started write direction
+
+Request read TLPs traffic generation - Endpoint to Root Complex direction:
+- Command:
+	echo r > /sys/module/dw_xdata_pcie/parameters/command
+- Output example:
+	dw-xdata-pcie 0000:01.0: xData: requested read transfer
+	dw-xdata-pcie 0000:01.0: xData: started read direction
+
+Request to stop any current TLP transfer:
+- Command:
+	echo s > /sys/module/dw_xdata_pcie/parameters/command
+- Output example:
+	dw-xdata-pcie 0000:01.0: xData: requested stop any transfer
+
+Request PCIe link performance analysis:
+- Command:
+	echo p > /sys/module/dw_xdata_pcie/parameters/command
+   - Output example:
+	dw-xdata-pcie 0000:01.0: xData: requested performance analysis
+	dw-xdata-pcie 0000:01.0: xData: time=112000000us, read=0 MB/s, write=0 MB/s
-- 
2.7.4

[PATCH 5/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>
---
 Documentation/misc-devices/dw-xdata-pcie.rst | 4 ++--
 MAINTAINERS                                  | 7 +++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/misc-devices/dw-xdata-pcie.rst b/Documentation/misc-devices/dw-xdata-pcie.rst
index 4d4616e..3d2981d 100644
--- a/Documentation/misc-devices/dw-xdata-pcie.rst
+++ b/Documentation/misc-devices/dw-xdata-pcie.rst
@@ -1,6 +1,6 @@
 .. SPDX-License-Identifier: GPL-2.0
 
-===========================================================================w
+===========================================================================
 Driver for Synopsys DesignWare PCIe traffic generator (also known as xData)
 ===========================================================================
 
@@ -40,4 +40,4 @@ Request PCIe link performance analysis:
 	echo p > /sys/module/dw_xdata_pcie/parameters/command
    - Output example:
 	dw-xdata-pcie 0000:01.0: xData: requested performance analysis
-	dw-xdata-pcie 0000:01.0: xData: time=112000000us, read=0 MB/s, write=0 MB/s
+	dw-xdata-pcie 0000:01.0: xData: time=100000000 us, write=0 MB/s, read=0 MB/s
diff --git a/MAINTAINERS b/MAINTAINERS
index 8671573..8856f6b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4983,6 +4983,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

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

[Greg Kroah-Hartman]

On Thu, Oct 29, 2020 at 08:13:36PM +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 | 395 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 395 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..b529dae
> --- /dev/null
> +++ b/drivers/misc/dw-xdata-pcie.c
> @@ -0,0 +1,395 @@
> +// 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/pci-epf.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/delay.h>
> +#include <linux/pci.h>
> +
> +#define DW_XDATA_EP_MEM_OFFSET		0x8000000
> +
> +static DEFINE_MUTEX(xdata_lock);
> +
> +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 */
> +};
> +
> +static int dw_xdata_command_set(const char *val, const struct kernel_param *kp);
> +static const struct kernel_param_ops xdata_command_ops = {
> +	.set = dw_xdata_command_set,
> +};
> +
> +static char command;
> +module_param_cb(command, &xdata_command_ops, &command, 0644);
> +MODULE_PARM_DESC(command, "xData command");

Please do not add new module parameters.  This is not the 1990's, we
have better ways of getting data into a driver.

> +
> +static struct pci_dev *priv;

You are only going to support one PCI device in the system at once?
That's not needed, again, this isn't the 1990's, please use
device-specific data and you will be fine, no "global" variables needed.

> +
> +union dw_xdata_control_reg {
> +	u32 reg;
> +	struct {
> +		u32 doorbell    : 1;			/* 00 */
> +		u32 is_write    : 1;			/* 01 */
> +		u32 length      : 12;			/* 02..13 */
> +		u32 is_64       : 1;			/* 14 */
> +		u32 ep		: 1;			/* 15 */
> +		u32 pattern_inc : 1;			/* 16 */
> +		u32 ie		: 1;			/* 17 */
> +		u32 no_addr_inc : 1;			/* 18 */
> +		u32 trigger     : 1;			/* 19 */
> +		u32 _reserved0  : 12;			/* 20..31 */
> +	};
> +} __packed;

What is the endian-ness of these structures?  That needs to be defined
somewhere, right?

> +
> +union dw_xdata_status_reg {
> +	u32 reg;
> +	struct {
> +		u32 done	: 1;			/* 00 */
> +		u32 _reserved0  : 15;			/* 01..15 */
> +		u32 version     : 16;			/* 16..31 */
> +	};
> +} __packed;
> +
> +union dw_xdata_xperf_control_reg {
> +	u32 reg;
> +	struct {
> +		u32 _reserved0  : 4;			/* 00..03 */
> +		u32 reset       : 1;			/* 04 */
> +		u32 enable      : 1;			/* 05 */
> +		u32 _reserved1  : 26;			/* 06..31 */
> +	};
> +} __packed;
> +
> +union _addr {
> +	u64 reg;
> +	struct {
> +		u32 lsb;
> +		u32 msb;
> +	};
> +};
> +
> +struct dw_xdata_regs {
> +	union _addr addr;				/* 0x000..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 */
> +	union _addr wr_cnt;				/* 0x100..0x104 */
> +	union _addr rd_cnt;				/* 0x108..0x10c */
> +} __packed;

Why packed?  Does this cross the user/kernel boundry?  If so, please use
the correct data types for the (__u32 not u32).


> +
> +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 xfer len */
> +	size_t max_rd_len;				/* max xfer len */
> +};
> +
> +static inline struct dw_xdata_regs __iomem *__dw_regs(struct dw_xdata *dw)
> +{
> +	return dw->rg_region.vaddr;
> +}
> +
> +#define SET(dw, name, value) \
> +	writel(value, &(__dw_regs(dw)->name))
> +
> +#define GET(dw, name) \
> +	readl(&(__dw_regs(dw)->name))

Just write out readl() and writel() in the driver, it makes more sense
to anyone trying to read the code.

> +
> +#ifdef CONFIG_64BIT
> +#define SET_64(dw, name, value) \
> +	writel(value, &(__dw_regs(dw)->name))
> +
> +#define GET_64(dw, name) \
> +	readq(&(__dw_regs(dw)->name))
> +#endif /* CONFIG_64BIT */

No need for this #ifdef, right?


> +
> +static void dw_xdata_stop(struct pci_dev *pdev)
> +{
> +	struct dw_xdata *dw = pci_get_drvdata(pdev);
> +	u32 tmp = GET(dw, burst_cnt);
> +
> +	if (tmp & 0x80000000) {
> +		tmp &= 0x7fffffff;
> +		SET(dw, burst_cnt, tmp);
> +	}
> +}
> +
> +static void dw_xdata_start(struct pci_dev *pdev, bool write)
> +{
> +	struct dw_xdata *dw = pci_get_drvdata(pdev);
> +	union dw_xdata_control_reg control;
> +	union dw_xdata_status_reg status;
> +
> +	/* Stop first if xfer in progress */
> +	dw_xdata_stop(pdev);
> +
> +	/* Clear status register */
> +	SET(dw, status, 0x0);
> +
> +	/* Burst count register set for continuous until stopped */
> +	SET(dw, burst_cnt, 0x80001001);
> +
> +	/* Pattern register */
> +	SET(dw, pattern, 0x0);
> +
> +	/* Control register */
> +	control.reg = 0x0;
> +	control.doorbell = true;
> +	control.is_write = write;
> +	if (write)
> +		control.length = dw->max_wr_len;
> +	else
> +		control.length = dw->max_rd_len;
> +	control.pattern_inc = true;
> +	control.no_addr_inc = true;
> +	SET(dw, control, control.reg);
> +
> +	usleep_range(100, 150);
> +
> +	status.reg = GET(dw, status);
> +	if (!status.done)
> +		pci_info(pdev, "xData: started %s direction\n",
> +			 write ? "write" : "read");

Don't be noisy if all is well.  You have a lot of "debugging" messages
in this driver, please drop them all down to the debug level, or just
remove them.


> +}
> +
> +static u64 dw_xdata_perf_meas(struct pci_dev *pdev, u64 *wr, u64 *rd)
> +{
> +	struct dw_xdata *dw = pci_get_drvdata(pdev);
> +
> +	#ifdef CONFIG_64BIT
> +		*wr = GET_64(dw, wr_cnt.reg);
> +
> +		*rd = GET_64(dw, rd_cnt.reg);
> +	#else /* CONFIG_64BIT */
> +		*wr = GET(dw, wr_cnt.msb);
> +		*wr <<= 32;
> +		*wr |= GET(dw, wr_cnt.lsb);
> +
> +		*rd = GET(dw, rd_cnt.msb);
> +		*rd <<= 32;
> +		*rd |= GET(dw, rd_cnt.lsb);
> +	#endif /* CONFIG_64BIT */
> +
> +	return jiffies;

Why are you returning jiffies???


> +}
> +
> +static u64 dw_xdata_perf_diff(u64 *m1, u64 *m2, u64 time)
> +{
> +	u64 rate;
> +
> +	rate = (*m1 - *m2);
> +	rate *= (1000 * 1000 * 1000);
> +	rate >>= 20;
> +	rate = DIV_ROUND_CLOSEST_ULL(rate, time);
> +
> +	return rate;
> +}
> +
> +static void dw_xdata_perf(struct pci_dev *pdev)
> +{
> +	struct dw_xdata *dw = pci_get_drvdata(pdev);
> +	union dw_xdata_xperf_control_reg control;
> +	u64 wr[2], rd[2], time[2], rate[2], diff;
> +
> +	/* First measurement */
> +	control.reg = 0x0;
> +	control.enable = false;
> +	SET(dw, perf_control, control.reg);
> +	time[0] = dw_xdata_perf_meas(pdev, &wr[0], &rd[0]);
> +	control.enable = true;
> +	SET(dw, perf_control, control.reg);
> +
> +	/* Delay 100ms */
> +	mdelay(100);
> +
> +	/* Second measurement */
> +	control.reg = 0x0;
> +	control.enable = false;
> +	SET(dw, perf_control, control.reg);
> +	time[1] = dw_xdata_perf_meas(pdev, &wr[1], &rd[1]);
> +	control.enable = true;
> +	SET(dw, perf_control, control.reg);
> +
> +	/* Calculations */
> +	diff = jiffies_to_nsecs(time[1] - time[0]);
> +
> +	/* Write performance */
> +	rate[0] = dw_xdata_perf_diff(&wr[1], &wr[0], diff);
> +
> +	/* Read performance */
> +	rate[1] = dw_xdata_perf_diff(&rd[1], &rd[0], diff);
> +
> +	pci_info(pdev,
> +		 "xData: time=%llu us, write=%llu MB/s, read=%llu MB/s\n",
> +		 diff, rate[0], rate[1]);

Again, be quiet.

> +}
> +
> +static int dw_xdata_command_set(const char *val, const struct kernel_param *kp)
> +{
> +	int ret = -EBUSY;
> +
> +	mutex_lock(&xdata_lock);
> +	if (!priv)
> +		goto err;
> +
> +	ret = param_set_charp(val, kp);
> +	if (ret)
> +		goto err;
> +
> +	switch (*val) {
> +	case 'w':
> +	case 'W':
> +		pci_info(priv, "xData: requested write transfer\n");
> +		dw_xdata_start(priv, true);
> +		break;
> +	case 'r':
> +	case 'R':
> +		pci_info(priv, "xData: requested read transfer\n");
> +		dw_xdata_start(priv, false);
> +		break;
> +	case 'p':
> +	case 'P':
> +		pci_info(priv, "xData: requested performance analysis\n");
> +		dw_xdata_perf(priv);
> +		break;
> +	default:
> +		pci_info(priv, "xData: requested stop any transfer\n");
> +		dw_xdata_stop(priv);
> +		break;
> +	}
> +
> +err:
> +	mutex_unlock(&xdata_lock);
> +
> +	return ret;
> +}
> +
> +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 device *dev = &pdev->dev;
> +	struct dw_xdata *dw;
> +	u64 addr;
> +	int err;
> +
> +	priv = NULL;
> +
> +	/* 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(dev, sizeof(*dw), GFP_KERNEL);
> +	if (!dw)
> +		return -ENOMEM;
> +
> +	/* Data structure initialization */
> +	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;
> +
> +	SET(dw, RAM_addr, 0x0);
> +	SET(dw, RAM_port, 0x0);
> +
> +	addr = dw->rg_region.paddr + DW_XDATA_EP_MEM_OFFSET;
> +#ifdef CONFIG_64BIT
> +	SET_64(dw, addr.reg, addr);
> +#else /* CONFIG_64BIT */
> +	SET(dw, addr.lsb, lower_32_bits(addr));
> +	SET(dw, addr.msb, upper_32_bits(addr));
> +#endif /* CONFIG_64BIT */
> +	pci_info(pdev, "xData: target address = 0x%.16llx\n", addr);
> +
> +	pci_info(pdev, "xData: wr_len=%zu, rd_len=%zu\n",
> +		 dw->max_wr_len * 4, dw->max_rd_len * 4);

Drivers need to be quiet...

thanks,

greg k-h

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

[Gustavo Pimentel]

Hi Greg,

On Mon, Nov 9, 2020 at 17:31:8, Greg Kroah-Hartman 
<gregkh@linuxfoundation.org> wrote:

> On Thu, Oct 29, 2020 at 08:13:36PM +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 | 395 +++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 395 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..b529dae
> > --- /dev/null
> > +++ b/drivers/misc/dw-xdata-pcie.c
> > @@ -0,0 +1,395 @@
> > +// 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/pci-epf.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/device.h>
> > +#include <linux/delay.h>
> > +#include <linux/pci.h>
> > +
> > +#define DW_XDATA_EP_MEM_OFFSET		0x8000000
> > +
> > +static DEFINE_MUTEX(xdata_lock);
> > +
> > +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 */
> > +};
> > +
> > +static int dw_xdata_command_set(const char *val, const struct kernel_param *kp);
> > +static const struct kernel_param_ops xdata_command_ops = {
> > +	.set = dw_xdata_command_set,
> > +};
> > +
> > +static char command;
> > +module_param_cb(command, &xdata_command_ops, &command, 0644);
> > +MODULE_PARM_DESC(command, "xData command");
> 
> Please do not add new module parameters.  This is not the 1990's, we
> have better ways of getting data into a driver.

Ok, I'll move this towards into the future, lol I'll use debugfs instead.

> 
> > +
> > +static struct pci_dev *priv;
> 
> You are only going to support one PCI device in the system at once?
> That's not needed, again, this isn't the 1990's, please use
> device-specific data and you will be fine, no "global" variables needed.
> 
> > +
> > +union dw_xdata_control_reg {
> > +	u32 reg;
> > +	struct {
> > +		u32 doorbell    : 1;			/* 00 */
> > +		u32 is_write    : 1;			/* 01 */
> > +		u32 length      : 12;			/* 02..13 */
> > +		u32 is_64       : 1;			/* 14 */
> > +		u32 ep		: 1;			/* 15 */
> > +		u32 pattern_inc : 1;			/* 16 */
> > +		u32 ie		: 1;			/* 17 */
> > +		u32 no_addr_inc : 1;			/* 18 */
> > +		u32 trigger     : 1;			/* 19 */
> > +		u32 _reserved0  : 12;			/* 20..31 */
> > +	};
> > +} __packed;
> 
> What is the endian-ness of these structures?  That needs to be defined
> somewhere, right?

What you suggest? Use __le32 instead of u32? Or some comment referring 
the expected endianness?

> 
> > +
> > +union dw_xdata_status_reg {
> > +	u32 reg;
> > +	struct {
> > +		u32 done	: 1;			/* 00 */
> > +		u32 _reserved0  : 15;			/* 01..15 */
> > +		u32 version     : 16;			/* 16..31 */
> > +	};
> > +} __packed;
> > +
> > +union dw_xdata_xperf_control_reg {
> > +	u32 reg;
> > +	struct {
> > +		u32 _reserved0  : 4;			/* 00..03 */
> > +		u32 reset       : 1;			/* 04 */
> > +		u32 enable      : 1;			/* 05 */
> > +		u32 _reserved1  : 26;			/* 06..31 */
> > +	};
> > +} __packed;
> > +
> > +union _addr {
> > +	u64 reg;
> > +	struct {
> > +		u32 lsb;
> > +		u32 msb;
> > +	};
> > +};
> > +
> > +struct dw_xdata_regs {
> > +	union _addr addr;				/* 0x000..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 */
> > +	union _addr wr_cnt;				/* 0x100..0x104 */
> > +	union _addr rd_cnt;				/* 0x108..0x10c */
> > +} __packed;
> 
> Why packed?  Does this cross the user/kernel boundry?  If so, please use
> the correct data types for the (__u32 not u32).

The idea behind this was to be a *mask* of the HW registers. By using the 
packed attribute would ensure that the struct would be matching with what 
is defined on the HW.
Since the used unions definitions are already packed, maybe this packed 
attribute on this structure is not needed. What this what you meant?

> 
> 
> > +
> > +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 xfer len */
> > +	size_t max_rd_len;				/* max xfer len */
> > +};
> > +
> > +static inline struct dw_xdata_regs __iomem *__dw_regs(struct dw_xdata *dw)
> > +{
> > +	return dw->rg_region.vaddr;
> > +}
> > +
> > +#define SET(dw, name, value) \
> > +	writel(value, &(__dw_regs(dw)->name))
> > +
> > +#define GET(dw, name) \
> > +	readl(&(__dw_regs(dw)->name))
> 
> Just write out readl() and writel() in the driver, it makes more sense
> to anyone trying to read the code.

Ok, that's curious. I made this exactly to turn the code more readable. 
I'm okay to put like you said, no problem.

> 
> > +
> > +#ifdef CONFIG_64BIT
> > +#define SET_64(dw, name, value) \
> > +	writel(value, &(__dw_regs(dw)->name))
> > +
> > +#define GET_64(dw, name) \
> > +	readq(&(__dw_regs(dw)->name))
> > +#endif /* CONFIG_64BIT */
> 
> No need for this #ifdef, right?
> 
> 
> > +
> > +static void dw_xdata_stop(struct pci_dev *pdev)
> > +{
> > +	struct dw_xdata *dw = pci_get_drvdata(pdev);
> > +	u32 tmp = GET(dw, burst_cnt);
> > +
> > +	if (tmp & 0x80000000) {
> > +		tmp &= 0x7fffffff;
> > +		SET(dw, burst_cnt, tmp);
> > +	}
> > +}
> > +
> > +static void dw_xdata_start(struct pci_dev *pdev, bool write)
> > +{
> > +	struct dw_xdata *dw = pci_get_drvdata(pdev);
> > +	union dw_xdata_control_reg control;
> > +	union dw_xdata_status_reg status;
> > +
> > +	/* Stop first if xfer in progress */
> > +	dw_xdata_stop(pdev);
> > +
> > +	/* Clear status register */
> > +	SET(dw, status, 0x0);
> > +
> > +	/* Burst count register set for continuous until stopped */
> > +	SET(dw, burst_cnt, 0x80001001);
> > +
> > +	/* Pattern register */
> > +	SET(dw, pattern, 0x0);
> > +
> > +	/* Control register */
> > +	control.reg = 0x0;
> > +	control.doorbell = true;
> > +	control.is_write = write;
> > +	if (write)
> > +		control.length = dw->max_wr_len;
> > +	else
> > +		control.length = dw->max_rd_len;
> > +	control.pattern_inc = true;
> > +	control.no_addr_inc = true;
> > +	SET(dw, control, control.reg);
> > +
> > +	usleep_range(100, 150);
> > +
> > +	status.reg = GET(dw, status);
> > +	if (!status.done)
> > +		pci_info(pdev, "xData: started %s direction\n",
> > +			 write ? "write" : "read");
> 
> Don't be noisy if all is well.  You have a lot of "debugging" messages
> in this driver, please drop them all down to the debug level, or just
> remove them.

I understand and I agree with that, but this driver will be only used to 
assist a HW bring up. It's focus will be only on FPGA prototype 
solutions, restricted to only some cases. This help messages will be 
important for a HW design or a solutions tester to find a problem root 
cause.
In this case can this messages be an exception to the rule?

> 
> 
> > +}
> > +
> > +static u64 dw_xdata_perf_meas(struct pci_dev *pdev, u64 *wr, u64 *rd)
> > +{
> > +	struct dw_xdata *dw = pci_get_drvdata(pdev);
> > +
> > +	#ifdef CONFIG_64BIT
> > +		*wr = GET_64(dw, wr_cnt.reg);
> > +
> > +		*rd = GET_64(dw, rd_cnt.reg);
> > +	#else /* CONFIG_64BIT */
> > +		*wr = GET(dw, wr_cnt.msb);
> > +		*wr <<= 32;
> > +		*wr |= GET(dw, wr_cnt.lsb);
> > +
> > +		*rd = GET(dw, rd_cnt.msb);
> > +		*rd <<= 32;
> > +		*rd |= GET(dw, rd_cnt.lsb);
> > +	#endif /* CONFIG_64BIT */
> > +
> > +	return jiffies;
> 
> Why are you returning jiffies???

The goal of this function was to acquire the number of packets on a 
determine instant, returning jiffies was a pretty way of having all the 
info grouped. But I'll move this outside of the function.
> 
> 
> > +}
> > +
> > +static u64 dw_xdata_perf_diff(u64 *m1, u64 *m2, u64 time)
> > +{
> > +	u64 rate;
> > +
> > +	rate = (*m1 - *m2);
> > +	rate *= (1000 * 1000 * 1000);
> > +	rate >>= 20;
> > +	rate = DIV_ROUND_CLOSEST_ULL(rate, time);
> > +
> > +	return rate;
> > +}
> > +
> > +static void dw_xdata_perf(struct pci_dev *pdev)
> > +{
> > +	struct dw_xdata *dw = pci_get_drvdata(pdev);
> > +	union dw_xdata_xperf_control_reg control;
> > +	u64 wr[2], rd[2], time[2], rate[2], diff;
> > +
> > +	/* First measurement */
> > +	control.reg = 0x0;
> > +	control.enable = false;
> > +	SET(dw, perf_control, control.reg);
> > +	time[0] = dw_xdata_perf_meas(pdev, &wr[0], &rd[0]);
> > +	control.enable = true;
> > +	SET(dw, perf_control, control.reg);
> > +
> > +	/* Delay 100ms */
> > +	mdelay(100);
> > +
> > +	/* Second measurement */
> > +	control.reg = 0x0;
> > +	control.enable = false;
> > +	SET(dw, perf_control, control.reg);
> > +	time[1] = dw_xdata_perf_meas(pdev, &wr[1], &rd[1]);
> > +	control.enable = true;
> > +	SET(dw, perf_control, control.reg);
> > +
> > +	/* Calculations */
> > +	diff = jiffies_to_nsecs(time[1] - time[0]);
> > +
> > +	/* Write performance */
> > +	rate[0] = dw_xdata_perf_diff(&wr[1], &wr[0], diff);
> > +
> > +	/* Read performance */
> > +	rate[1] = dw_xdata_perf_diff(&rd[1], &rd[0], diff);
> > +
> > +	pci_info(pdev,
> > +		 "xData: time=%llu us, write=%llu MB/s, read=%llu MB/s\n",
> > +		 diff, rate[0], rate[1]);
> 
> Again, be quiet.
> 
> > +}
> > +
> > +static int dw_xdata_command_set(const char *val, const struct kernel_param *kp)
> > +{
> > +	int ret = -EBUSY;
> > +
> > +	mutex_lock(&xdata_lock);
> > +	if (!priv)
> > +		goto err;
> > +
> > +	ret = param_set_charp(val, kp);
> > +	if (ret)
> > +		goto err;
> > +
> > +	switch (*val) {
> > +	case 'w':
> > +	case 'W':
> > +		pci_info(priv, "xData: requested write transfer\n");
> > +		dw_xdata_start(priv, true);
> > +		break;
> > +	case 'r':
> > +	case 'R':
> > +		pci_info(priv, "xData: requested read transfer\n");
> > +		dw_xdata_start(priv, false);
> > +		break;
> > +	case 'p':
> > +	case 'P':
> > +		pci_info(priv, "xData: requested performance analysis\n");
> > +		dw_xdata_perf(priv);
> > +		break;
> > +	default:
> > +		pci_info(priv, "xData: requested stop any transfer\n");
> > +		dw_xdata_stop(priv);
> > +		break;
> > +	}
> > +
> > +err:
> > +	mutex_unlock(&xdata_lock);
> > +
> > +	return ret;
> > +}
> > +
> > +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 device *dev = &pdev->dev;
> > +	struct dw_xdata *dw;
> > +	u64 addr;
> > +	int err;
> > +
> > +	priv = NULL;
> > +
> > +	/* 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(dev, sizeof(*dw), GFP_KERNEL);
> > +	if (!dw)
> > +		return -ENOMEM;
> > +
> > +	/* Data structure initialization */
> > +	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;
> > +
> > +	SET(dw, RAM_addr, 0x0);
> > +	SET(dw, RAM_port, 0x0);
> > +
> > +	addr = dw->rg_region.paddr + DW_XDATA_EP_MEM_OFFSET;
> > +#ifdef CONFIG_64BIT
> > +	SET_64(dw, addr.reg, addr);
> > +#else /* CONFIG_64BIT */
> > +	SET(dw, addr.lsb, lower_32_bits(addr));
> > +	SET(dw, addr.msb, upper_32_bits(addr));
> > +#endif /* CONFIG_64BIT */
> > +	pci_info(pdev, "xData: target address = 0x%.16llx\n", addr);
> > +
> > +	pci_info(pdev, "xData: wr_len=%zu, rd_len=%zu\n",
> > +		 dw->max_wr_len * 4, dw->max_rd_len * 4);
> 
> Drivers need to be quiet...
> 
> thanks,
> 
> greg k-h

-Gustavo

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

[Greg Kroah-Hartman]

On Tue, Nov 10, 2020 at 03:17:54PM +0000, Gustavo Pimentel wrote:
> Hi Greg,
> 
> On Mon, Nov 9, 2020 at 17:31:8, Greg Kroah-Hartman 
> <gregkh@linuxfoundation.org> wrote:
> 
> > On Thu, Oct 29, 2020 at 08:13:36PM +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 | 395 +++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 395 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..b529dae
> > > --- /dev/null
> > > +++ b/drivers/misc/dw-xdata-pcie.c
> > > @@ -0,0 +1,395 @@
> > > +// 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/pci-epf.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/device.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/pci.h>
> > > +
> > > +#define DW_XDATA_EP_MEM_OFFSET		0x8000000
> > > +
> > > +static DEFINE_MUTEX(xdata_lock);
> > > +
> > > +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 */
> > > +};
> > > +
> > > +static int dw_xdata_command_set(const char *val, const struct kernel_param *kp);
> > > +static const struct kernel_param_ops xdata_command_ops = {
> > > +	.set = dw_xdata_command_set,
> > > +};
> > > +
> > > +static char command;
> > > +module_param_cb(command, &xdata_command_ops, &command, 0644);
> > > +MODULE_PARM_DESC(command, "xData command");
> > 
> > Please do not add new module parameters.  This is not the 1990's, we
> > have better ways of getting data into a driver.
> 
> Ok, I'll move this towards into the future, lol I'll use debugfs instead.
> 
> > 
> > > +
> > > +static struct pci_dev *priv;
> > 
> > You are only going to support one PCI device in the system at once?
> > That's not needed, again, this isn't the 1990's, please use
> > device-specific data and you will be fine, no "global" variables needed.
> > 
> > > +
> > > +union dw_xdata_control_reg {
> > > +	u32 reg;
> > > +	struct {
> > > +		u32 doorbell    : 1;			/* 00 */
> > > +		u32 is_write    : 1;			/* 01 */
> > > +		u32 length      : 12;			/* 02..13 */
> > > +		u32 is_64       : 1;			/* 14 */
> > > +		u32 ep		: 1;			/* 15 */
> > > +		u32 pattern_inc : 1;			/* 16 */
> > > +		u32 ie		: 1;			/* 17 */
> > > +		u32 no_addr_inc : 1;			/* 18 */
> > > +		u32 trigger     : 1;			/* 19 */
> > > +		u32 _reserved0  : 12;			/* 20..31 */
> > > +	};
> > > +} __packed;
> > 
> > What is the endian-ness of these structures?  That needs to be defined
> > somewhere, right?
> 
> What you suggest? Use __le32 instead of u32? Or some comment referring 
> the expected endianness?

Use bitmasks, that way it works on any endian system.

> 
> > 
> > > +
> > > +union dw_xdata_status_reg {
> > > +	u32 reg;
> > > +	struct {
> > > +		u32 done	: 1;			/* 00 */
> > > +		u32 _reserved0  : 15;			/* 01..15 */
> > > +		u32 version     : 16;			/* 16..31 */
> > > +	};
> > > +} __packed;
> > > +
> > > +union dw_xdata_xperf_control_reg {
> > > +	u32 reg;
> > > +	struct {
> > > +		u32 _reserved0  : 4;			/* 00..03 */
> > > +		u32 reset       : 1;			/* 04 */
> > > +		u32 enable      : 1;			/* 05 */
> > > +		u32 _reserved1  : 26;			/* 06..31 */
> > > +	};
> > > +} __packed;
> > > +
> > > +union _addr {
> > > +	u64 reg;
> > > +	struct {
> > > +		u32 lsb;
> > > +		u32 msb;
> > > +	};
> > > +};
> > > +
> > > +struct dw_xdata_regs {
> > > +	union _addr addr;				/* 0x000..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 */
> > > +	union _addr wr_cnt;				/* 0x100..0x104 */
> > > +	union _addr rd_cnt;				/* 0x108..0x10c */
> > > +} __packed;
> > 
> > Why packed?  Does this cross the user/kernel boundry?  If so, please use
> > the correct data types for the (__u32 not u32).
> 
> The idea behind this was to be a *mask* of the HW registers. By using the 
> packed attribute would ensure that the struct would be matching with what 
> is defined on the HW.
> Since the used unions definitions are already packed, maybe this packed 
> attribute on this structure is not needed. What this what you meant?

No, that's fine, just that if this does cross the user/kernel boundry,
like you say, then you need to specify the endian of the variables, and
use the correct types, which you are not doing anywhere here.

> > > +	status.reg = GET(dw, status);
> > > +	if (!status.done)
> > > +		pci_info(pdev, "xData: started %s direction\n",
> > > +			 write ? "write" : "read");
> > 
> > Don't be noisy if all is well.  You have a lot of "debugging" messages
> > in this driver, please drop them all down to the debug level, or just
> > remove them.
> 
> I understand and I agree with that, but this driver will be only used to 
> assist a HW bring up. It's focus will be only on FPGA prototype 
> solutions, restricted to only some cases. This help messages will be 
> important for a HW design or a solutions tester to find a problem root 
> cause.
> In this case can this messages be an exception to the rule?

Why not use ftrace if you are supposed to be tracing the data flow
through the driver?  Or tracepoints?  Don't rely on printk for this.

thanks,

greg k-h

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

[Gustavo Pimentel]

On Tue, Nov 10, 2020 at 17:30:5, Greg Kroah-Hartman 
<gregkh@linuxfoundation.org> wrote:

> On Tue, Nov 10, 2020 at 03:17:54PM +0000, Gustavo Pimentel wrote:
> > Hi Greg,
> > 
> > On Mon, Nov 9, 2020 at 17:31:8, Greg Kroah-Hartman 
> > <gregkh@linuxfoundation.org> wrote:
> > 
> > > On Thu, Oct 29, 2020 at 08:13:36PM +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 | 395 +++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 395 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..b529dae
> > > > --- /dev/null
> > > > +++ b/drivers/misc/dw-xdata-pcie.c
> > > > @@ -0,0 +1,395 @@
> > > > +// 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/pci-epf.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/device.h>
> > > > +#include <linux/delay.h>
> > > > +#include <linux/pci.h>
> > > > +
> > > > +#define DW_XDATA_EP_MEM_OFFSET		0x8000000
> > > > +
> > > > +static DEFINE_MUTEX(xdata_lock);
> > > > +
> > > > +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 */
> > > > +};
> > > > +
> > > > +static int dw_xdata_command_set(const char *val, const struct kernel_param *kp);
> > > > +static const struct kernel_param_ops xdata_command_ops = {
> > > > +	.set = dw_xdata_command_set,
> > > > +};
> > > > +
> > > > +static char command;
> > > > +module_param_cb(command, &xdata_command_ops, &command, 0644);
> > > > +MODULE_PARM_DESC(command, "xData command");
> > > 
> > > Please do not add new module parameters.  This is not the 1990's, we
> > > have better ways of getting data into a driver.
> > 
> > Ok, I'll move this towards into the future, lol I'll use debugfs instead.
> > 
> > > 
> > > > +
> > > > +static struct pci_dev *priv;
> > > 
> > > You are only going to support one PCI device in the system at once?
> > > That's not needed, again, this isn't the 1990's, please use
> > > device-specific data and you will be fine, no "global" variables needed.
> > > 
> > > > +
> > > > +union dw_xdata_control_reg {
> > > > +	u32 reg;
> > > > +	struct {
> > > > +		u32 doorbell    : 1;			/* 00 */
> > > > +		u32 is_write    : 1;			/* 01 */
> > > > +		u32 length      : 12;			/* 02..13 */
> > > > +		u32 is_64       : 1;			/* 14 */
> > > > +		u32 ep		: 1;			/* 15 */
> > > > +		u32 pattern_inc : 1;			/* 16 */
> > > > +		u32 ie		: 1;			/* 17 */
> > > > +		u32 no_addr_inc : 1;			/* 18 */
> > > > +		u32 trigger     : 1;			/* 19 */
> > > > +		u32 _reserved0  : 12;			/* 20..31 */
> > > > +	};
> > > > +} __packed;
> > > 
> > > What is the endian-ness of these structures?  That needs to be defined
> > > somewhere, right?
> > 
> > What you suggest? Use __le32 instead of u32? Or some comment referring 
> > the expected endianness?
> 
> Use bitmasks, that way it works on any endian system.

All platforms used in prototype validation are little endian and this 
structures were validated using those platforms. I can put my the backlog 
to do a general driver rework on the driver to use bitmasks, but for now, 
I rather not do it right now due to the equipment access 
limitation/restrictions caused by the pandemic. Can you accept this?

> 
> > 
> > > 
> > > > +
> > > > +union dw_xdata_status_reg {
> > > > +	u32 reg;
> > > > +	struct {
> > > > +		u32 done	: 1;			/* 00 */
> > > > +		u32 _reserved0  : 15;			/* 01..15 */
> > > > +		u32 version     : 16;			/* 16..31 */
> > > > +	};
> > > > +} __packed;
> > > > +
> > > > +union dw_xdata_xperf_control_reg {
> > > > +	u32 reg;
> > > > +	struct {
> > > > +		u32 _reserved0  : 4;			/* 00..03 */
> > > > +		u32 reset       : 1;			/* 04 */
> > > > +		u32 enable      : 1;			/* 05 */
> > > > +		u32 _reserved1  : 26;			/* 06..31 */
> > > > +	};
> > > > +} __packed;
> > > > +
> > > > +union _addr {
> > > > +	u64 reg;
> > > > +	struct {
> > > > +		u32 lsb;
> > > > +		u32 msb;
> > > > +	};
> > > > +};
> > > > +
> > > > +struct dw_xdata_regs {
> > > > +	union _addr addr;				/* 0x000..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 */
> > > > +	union _addr wr_cnt;				/* 0x100..0x104 */
> > > > +	union _addr rd_cnt;				/* 0x108..0x10c */
> > > > +} __packed;
> > > 
> > > Why packed?  Does this cross the user/kernel boundry?  If so, please use
> > > the correct data types for the (__u32 not u32).
> > 
> > The idea behind this was to be a *mask* of the HW registers. By using the 
> > packed attribute would ensure that the struct would be matching with what 
> > is defined on the HW.
> > Since the used unions definitions are already packed, maybe this packed 
> > attribute on this structure is not needed. What this what you meant?
> 
> No, that's fine, just that if this does cross the user/kernel boundry,
> like you say, then you need to specify the endian of the variables, and
> use the correct types, which you are not doing anywhere here.

Understood, but on this case it will not. These registers will be used 
only HW initialization and setting HW behaviors.

> 
> > > > +	status.reg = GET(dw, status);
> > > > +	if (!status.done)
> > > > +		pci_info(pdev, "xData: started %s direction\n",
> > > > +			 write ? "write" : "read");
> > > 
> > > Don't be noisy if all is well.  You have a lot of "debugging" messages
> > > in this driver, please drop them all down to the debug level, or just
> > > remove them.
> > 
> > I understand and I agree with that, but this driver will be only used to 
> > assist a HW bring up. It's focus will be only on FPGA prototype 
> > solutions, restricted to only some cases. This help messages will be 
> > important for a HW design or a solutions tester to find a problem root 
> > cause.
> > In this case can this messages be an exception to the rule?
> 
> Why not use ftrace if you are supposed to be tracing the data flow
> through the driver?  Or tracepoints?  Don't rely on printk for this.

This trace message is not for my debug, it's for the HW IP validation 
testers and designer.
For instance in this driver they want to know upon the driver loading 
which are certain values set on the HW and if they match what was defined 
on the FPGA design. Since this under prototyping scope sometimes the HW 
might block/hang by multiple reasons, having this debug messages, can 
help them to understand where was the last "heartbeat" was taken and 
point them to probable the root cause.
As I said, this driver will be use on the prototype validation (basically 
internal to the company), by persons that will have the bare minimal 
Linux knowledge, because their focus and base knowledge is the HW itself.

> 
> thanks,
> 
> greg k-h