linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RESEND v4 0/6] misc: Add Add Synopsys DesignWare xData IP driver
@ 2021-02-03 22:12 Gustavo Pimentel
  2021-02-03 22:12 ` [RESEND v4 1/6] misc: " Gustavo Pimentel
                   ` (5 more replies)
  0 siblings, 6 replies; 19+ messages in thread
From: Gustavo Pimentel @ 2021-02-03 22:12 UTC (permalink / raw)
  To: linux-doc, linux-pci, linux-kernel, Derek Kiernan, Dragan Cvetic,
	Arnd Bergmann, Andrew Morton, Greg Kroah-Hartman,
	Jonathan Corbet
  Cc: 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.

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

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: linux-pci@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Gustavo Pimentel (6):
  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
  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                         |  11 +
 drivers/misc/Makefile                        |   1 +
 drivers/misc/dw-xdata-pcie.c                 | 378 +++++++++++++++++++++++++++
 6 files changed, 483 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


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

* [RESEND v4 1/6] misc: Add Synopsys DesignWare xData IP driver
  2021-02-03 22:12 [RESEND v4 0/6] misc: Add Add Synopsys DesignWare xData IP driver Gustavo Pimentel
@ 2021-02-03 22:12 ` Gustavo Pimentel
  2021-02-03 22:33   ` Greg Kroah-Hartman
  2021-02-07  1:36   ` Krzysztof Wilczyński
  2021-02-03 22:12 ` [RESEND v4 2/6] misc: Add Synopsys DesignWare xData IP driver to Makefile Gustavo Pimentel
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 19+ messages in thread
From: Gustavo Pimentel @ 2021-02-03 22:12 UTC (permalink / raw)
  To: linux-doc, linux-pci, linux-kernel, Derek Kiernan, Dragan Cvetic,
	Arnd Bergmann, Andrew Morton, Greg Kroah-Hartman,
	Jonathan Corbet, 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 | 378 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 378 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..3e1bc35
--- /dev/null
+++ b/drivers/misc/dw-xdata-pcie.c
@@ -0,0 +1,378 @@
+// 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/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)
+
+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;
+};
+
+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 = readl(&(__dw_regs(dw)->burst_cnt));
+
+	if (burst & BIT(31)) {
+		burst &= ~(u32)BIT(31);
+		writel(burst, &(__dw_regs(dw)->burst_cnt));
+	}
+}
+
+static void dw_xdata_start(struct dw_xdata *dw, bool write)
+{
+	u32 control, status;
+
+	/* Stop first if xfer in progress */
+	dw_xdata_stop(dw);
+
+	/* Clear status register */
+	writel(0x0, &(__dw_regs(dw)->status));
+
+	/* Burst count register set for continuous until stopped */
+	writel(0x80001001, &(__dw_regs(dw)->burst_cnt));
+
+	/* Pattern register */
+	writel(0x0, &(__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));
+
+	usleep_range(100, 150);
+
+	status = readl(&(__dw_regs(dw)->status));
+	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;
+
+	/* First measurement */
+	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));
+
+	/* Delay 100ms */
+	mdelay(100);
+
+	/* Second measurement */
+	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));
+
+	/* Calculations */
+	diff = jiffies_to_nsecs(time[1] - time[0]);
+	*rate = dw_xdata_perf_diff(&data[1], &data[0], diff);
+
+	pci_dbg(dw->pdev, "xData: time=%llu us, %s=%llu MB/s\n",
+		diff, write ? "write" : "read", *rate);
+}
+
+static ssize_t write_show(struct device *dev, struct device_attribute *attr,
+			  char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct dw_xdata *dw = pci_get_drvdata(pdev);
+	u64 rate;
+
+	mutex_lock(&dw->mutex);
+	dw_xdata_perf(dw, &rate, true);
+	mutex_unlock(&dw->mutex);
+
+	return sysfs_emit(buf, "%llu MB/s\n", rate);
+}
+
+static ssize_t write_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t size)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct dw_xdata *dw = pci_get_drvdata(pdev);
+
+	pci_dbg(pdev, "xData: requested write transfer\n");
+
+	mutex_lock(&dw->mutex);
+	dw_xdata_start(dw, true);
+	mutex_unlock(&dw->mutex);
+
+	return size;
+}
+
+static DEVICE_ATTR_RW(write);
+
+static ssize_t read_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct dw_xdata *dw = pci_get_drvdata(pdev);
+	u64 rate;
+
+	mutex_lock(&dw->mutex);
+	dw_xdata_perf(dw, &rate, false);
+	mutex_unlock(&dw->mutex);
+
+	return sysfs_emit(buf, "%llu MB/s\n", rate);
+}
+
+static ssize_t read_store(struct device *dev, struct device_attribute *attr,
+			  const char *buf, size_t size)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct dw_xdata *dw = pci_get_drvdata(pdev);
+
+	pci_dbg(pdev, "xData: requested read transfer\n");
+
+	mutex_lock(&dw->mutex);
+	dw_xdata_start(dw, false);
+	mutex_unlock(&dw->mutex);
+
+	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 pci_dev *pdev = to_pci_dev(dev);
+	struct dw_xdata *dw = pci_get_drvdata(pdev);
+
+	pci_dbg(pdev, "xData: requested stop any transfer\n");
+
+	mutex_lock(&dw->mutex);
+	dw_xdata_stop(dw);
+	mutex_unlock(&dw->mutex);
+
+	return size;
+}
+
+static DEVICE_ATTR_WO(stop);
+
+static struct attribute *default_attrs[] = {
+	&dev_attr_write.attr,
+	&dev_attr_read.attr,
+	&dev_attr_stop.attr,
+	NULL,
+};
+
+static const struct attribute_group xdata_attr_group = {
+	.attrs = default_attrs,
+	.name = DW_XDATA_DRIVER_NAME,
+};
+
+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;
+
+	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);
+
+	/* Sysfs */
+	err = sysfs_create_group(&pdev->dev.kobj, &xdata_attr_group);
+	if (err)
+		return err;
+
+	err = sysfs_create_link(kernel_kobj, &pdev->dev.kobj,
+				DW_XDATA_DRIVER_NAME);
+	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) {
+		mutex_lock(&dw->mutex);
+		dw_xdata_stop(dw);
+		mutex_unlock(&dw->mutex);
+	}
+
+	sysfs_remove_link(kernel_kobj, DW_XDATA_DRIVER_NAME);
+	sysfs_remove_group(&pdev->dev.kobj, &xdata_attr_group);
+}
+
+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


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

* [RESEND v4 2/6] misc: Add Synopsys DesignWare xData IP driver to Makefile
  2021-02-03 22:12 [RESEND v4 0/6] misc: Add Add Synopsys DesignWare xData IP driver Gustavo Pimentel
  2021-02-03 22:12 ` [RESEND v4 1/6] misc: " Gustavo Pimentel
@ 2021-02-03 22:12 ` Gustavo Pimentel
  2021-02-03 22:12 ` [RESEND v4 3/6] misc: Add Synopsys DesignWare xData IP driver to Kconfig Gustavo Pimentel
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 19+ messages in thread
From: Gustavo Pimentel @ 2021-02-03 22:12 UTC (permalink / raw)
  To: linux-doc, linux-pci, linux-kernel, Derek Kiernan, Dragan Cvetic,
	Arnd Bergmann, Andrew Morton, Greg Kroah-Hartman,
	Jonathan Corbet
  Cc: 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 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


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

* [RESEND v4 3/6] misc: Add Synopsys DesignWare xData IP driver to Kconfig
  2021-02-03 22:12 [RESEND v4 0/6] misc: Add Add Synopsys DesignWare xData IP driver Gustavo Pimentel
  2021-02-03 22:12 ` [RESEND v4 1/6] misc: " Gustavo Pimentel
  2021-02-03 22:12 ` [RESEND v4 2/6] misc: Add Synopsys DesignWare xData IP driver to Makefile Gustavo Pimentel
@ 2021-02-03 22:12 ` Gustavo Pimentel
  2021-02-03 22:26   ` Greg Kroah-Hartman
  2021-02-03 22:27   ` Greg Kroah-Hartman
  2021-02-03 22:12 ` [RESEND v4 4/6] Documentation: misc-devices: Add Documentation for dw-xdata-pcie driver Gustavo Pimentel
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 19+ messages in thread
From: Gustavo Pimentel @ 2021-02-03 22:12 UTC (permalink / raw)
  To: linux-doc, linux-pci, linux-kernel, Derek Kiernan, Dragan Cvetic,
	Arnd Bergmann, Andrew Morton, Greg Kroah-Hartman,
	Jonathan Corbet
  Cc: 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 | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index fafa8b0..6d5783f 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -423,6 +423,17 @@ config SRAM
 config SRAM_EXEC
 	bool
 
+config DW_XDATA_PCIE
+	depends on PCI
+	tristate "Synopsys DesignWare xData PCIe driver"
+	default	n
+	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


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

* [RESEND v4 4/6] Documentation: misc-devices: Add Documentation for dw-xdata-pcie driver
  2021-02-03 22:12 [RESEND v4 0/6] misc: Add Add Synopsys DesignWare xData IP driver Gustavo Pimentel
                   ` (2 preceding siblings ...)
  2021-02-03 22:12 ` [RESEND v4 3/6] misc: Add Synopsys DesignWare xData IP driver to Kconfig Gustavo Pimentel
@ 2021-02-03 22:12 ` Gustavo Pimentel
  2021-02-06 23:31   ` Krzysztof Wilczyński
  2021-02-03 22:12 ` [RESEND v4 5/6] MAINTAINERS: Add Synopsys xData IP driver maintainer Gustavo Pimentel
  2021-02-03 22:12 ` [RESEND v4 6/6] docs: ABI: Add sysfs documentation interface of dw-xdata-pcie driver Gustavo Pimentel
  5 siblings, 1 reply; 19+ messages in thread
From: Gustavo Pimentel @ 2021-02-03 22:12 UTC (permalink / raw)
  To: linux-doc, linux-pci, linux-kernel, Derek Kiernan, Dragan Cvetic,
	Arnd Bergmann, Andrew Morton, Greg Kroah-Hartman,
	Jonathan Corbet, 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..3af9fad
--- /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/kernel/dw-xdata-pcie/write
+
+Get write TLPs traffic link throughput
+- Command:
+        cat /sys/kernel/dw-xdata-pcie/write
+- Output example:
+	204 MB/s
+
+Request read TLPs traffic generation - Endpoint to Root Complex direction:
+- Command:
+	echo 1 > /sys/kernel/dw-xdata-pcie/read
+
+Get read TLPs traffic link throughput
+- Command:
+        cat /sys/kernel/dw-xdata-pcie/read
+- Output example:
+	199 MB/s
+
+Request to stop any current TLP transfer:
+- Command:
+	echo 1 > /sys/kernel/dw-xdata-pcie/stop
-- 
2.7.4


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

* [RESEND v4 5/6] MAINTAINERS: Add Synopsys xData IP driver maintainer
  2021-02-03 22:12 [RESEND v4 0/6] misc: Add Add Synopsys DesignWare xData IP driver Gustavo Pimentel
                   ` (3 preceding siblings ...)
  2021-02-03 22:12 ` [RESEND v4 4/6] Documentation: misc-devices: Add Documentation for dw-xdata-pcie driver Gustavo Pimentel
@ 2021-02-03 22:12 ` Gustavo Pimentel
  2021-02-03 22:12 ` [RESEND v4 6/6] docs: ABI: Add sysfs documentation interface of dw-xdata-pcie driver Gustavo Pimentel
  5 siblings, 0 replies; 19+ messages in thread
From: Gustavo Pimentel @ 2021-02-03 22:12 UTC (permalink / raw)
  To: linux-doc, linux-pci, linux-kernel, Derek Kiernan, Dragan Cvetic,
	Arnd Bergmann, Andrew Morton, Greg Kroah-Hartman,
	Jonathan Corbet
  Cc: 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


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

* [RESEND v4 6/6] docs: ABI: Add sysfs documentation interface of dw-xdata-pcie driver
  2021-02-03 22:12 [RESEND v4 0/6] misc: Add Add Synopsys DesignWare xData IP driver Gustavo Pimentel
                   ` (4 preceding siblings ...)
  2021-02-03 22:12 ` [RESEND v4 5/6] MAINTAINERS: Add Synopsys xData IP driver maintainer Gustavo Pimentel
@ 2021-02-03 22:12 ` Gustavo Pimentel
  5 siblings, 0 replies; 19+ messages in thread
From: Gustavo Pimentel @ 2021-02-03 22:12 UTC (permalink / raw)
  To: linux-doc, linux-pci, linux-kernel, Derek Kiernan, Dragan Cvetic,
	Arnd Bergmann, Andrew Morton, Greg Kroah-Hartman,
	Jonathan Corbet
  Cc: 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..a7bb44b
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-xdata
@@ -0,0 +1,46 @@
+What:		/sys/kernel/dw-xdata-pcie/write
+Date:		February 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/kernel/dw-xdata-pcie/write
+
+		The user can read the current PCIe link throughput generated
+		through this generator.
+		Usage e.g.
+		 cat /sys/kernel/dw-xdata-pcie/write
+		 204 MB/s
+
+		The file is read and write.
+
+What:		/sys/kernel/dw-xdata-pcie/read
+Date:		February 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/kernel/dw-xdata-pcie/read
+
+		The user can read the current PCIe link throughput generated
+		through this generator.
+		Usage e.g.
+		 cat /sys/kernel/dw-xdata-pcie/read
+		 199 MB/s
+
+		The file is read and write.
+
+What:		/sys/kernel/dw-xdata-pcie/stop
+Date:		February 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/kernel/dw-xdata-pcie/stop
+
+		The file is write only.
-- 
2.7.4


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

* Re: [RESEND v4 3/6] misc: Add Synopsys DesignWare xData IP driver to Kconfig
  2021-02-03 22:12 ` [RESEND v4 3/6] misc: Add Synopsys DesignWare xData IP driver to Kconfig Gustavo Pimentel
@ 2021-02-03 22:26   ` Greg Kroah-Hartman
  2021-02-03 22:27   ` Greg Kroah-Hartman
  1 sibling, 0 replies; 19+ messages in thread
From: Greg Kroah-Hartman @ 2021-02-03 22:26 UTC (permalink / raw)
  To: Gustavo Pimentel
  Cc: linux-doc, linux-pci, linux-kernel, Derek Kiernan, Dragan Cvetic,
	Arnd Bergmann, Andrew Morton, Jonathan Corbet

On Wed, Feb 03, 2021 at 11:12:48PM +0100, Gustavo Pimentel wrote:
> 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 | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index fafa8b0..6d5783f 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -423,6 +423,17 @@ config SRAM
>  config SRAM_EXEC
>  	bool
>  
> +config DW_XDATA_PCIE
> +	depends on PCI
> +	tristate "Synopsys DesignWare xData PCIe driver"
> +	default	n

That's already the default, no need for this line at all.

> +	help
> +	  This driver allows controlling Synopsys DesignWare PCIe traffic
> +	  generator IP also known as xData, present in Synopsys Designware
> +	  PCIe Endpoint prototype.

Module name?

thanks,

greg k-h

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

* Re: [RESEND v4 3/6] misc: Add Synopsys DesignWare xData IP driver to Kconfig
  2021-02-03 22:12 ` [RESEND v4 3/6] misc: Add Synopsys DesignWare xData IP driver to Kconfig Gustavo Pimentel
  2021-02-03 22:26   ` Greg Kroah-Hartman
@ 2021-02-03 22:27   ` Greg Kroah-Hartman
  1 sibling, 0 replies; 19+ messages in thread
From: Greg Kroah-Hartman @ 2021-02-03 22:27 UTC (permalink / raw)
  To: Gustavo Pimentel
  Cc: linux-doc, linux-pci, linux-kernel, Derek Kiernan, Dragan Cvetic,
	Arnd Bergmann, Andrew Morton, Jonathan Corbet

On Wed, Feb 03, 2021 at 11:12:48PM +0100, Gustavo Pimentel wrote:
> 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 | 11 +++++++++++
>  1 file changed, 11 insertions(+)

Why isn't this, and patch 2, part of patch 1?  Why split this out?

thanks,

greg k-h

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

* Re: [RESEND v4 1/6] misc: Add Synopsys DesignWare xData IP driver
  2021-02-03 22:12 ` [RESEND v4 1/6] misc: " Gustavo Pimentel
@ 2021-02-03 22:33   ` Greg Kroah-Hartman
  2021-02-07  1:36   ` Krzysztof Wilczyński
  1 sibling, 0 replies; 19+ messages in thread
From: Greg Kroah-Hartman @ 2021-02-03 22:33 UTC (permalink / raw)
  To: Gustavo Pimentel
  Cc: linux-doc, linux-pci, linux-kernel, Derek Kiernan, Dragan Cvetic,
	Arnd Bergmann, Andrew Morton, Jonathan Corbet

On Wed, Feb 03, 2021 at 11:12:46PM +0100, Gustavo Pimentel wrote:
> +	/* Sysfs */
> +	err = sysfs_create_group(&pdev->dev.kobj, &xdata_attr_group);
> +	if (err)
> +		return err;
> +
> +	err = sysfs_create_link(kernel_kobj, &pdev->dev.kobj,
> +				DW_XDATA_DRIVER_NAME);
> +	if (err)
> +		return err;

Huge hint, if you EVER call sysfs_* in a driver, you are doing something
wrong.

You just raced userspace and lost, use the default attribute group for
your driver so that the driver core can automatically create the needed
sysfs files.

And drop the symlink, that's just crazy, never do that, I don't think
it's doing what you think it is doing, not to mention you did not
document it...

thanks,

greg k-h

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

* Re: [RESEND v4 4/6] Documentation: misc-devices: Add Documentation for dw-xdata-pcie driver
  2021-02-03 22:12 ` [RESEND v4 4/6] Documentation: misc-devices: Add Documentation for dw-xdata-pcie driver Gustavo Pimentel
@ 2021-02-06 23:31   ` Krzysztof Wilczyński
  2021-02-08  9:24     ` Gustavo Pimentel
  0 siblings, 1 reply; 19+ messages in thread
From: Krzysztof Wilczyński @ 2021-02-06 23:31 UTC (permalink / raw)
  To: Gustavo Pimentel
  Cc: linux-doc, linux-pci, linux-kernel, Derek Kiernan, Dragan Cvetic,
	Arnd Bergmann, Andrew Morton, Greg Kroah-Hartman,
	Jonathan Corbet

Hi Gustavo,

[...]
> +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.

The driver does not seem to offer any parameters (aside of using sysfs
for runtime settings), and it also seem to only print what it's doing
when debug level is enabled - unless I am missing something?

[...]
> +Request to stop any current TLP transfer:
> +- Command:
> +	echo 1 > /sys/kernel/dw-xdata-pcie/stop
[...]

When I do the following:

  # echo 1 > /sys/kernel/dw-xdata-pcie/write
  # echo 1 > /sys/kernel/dw-xdata-pcie/stop
  # cat /sys/kernel/dw-xdata-pcie/write

Would output from cat above simply show "0 MB/s" then?  I wonder how
someone using this new driver could tell whether "write" or "read"
traffic generation has been enabled aside of reading the sysfs files,
would adding "/sys/kernel/dw-xdata-pcie/active" be an overkill here?

What do you think?

Krzysztof

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

* Re: [RESEND v4 1/6] misc: Add Synopsys DesignWare xData IP driver
  2021-02-03 22:12 ` [RESEND v4 1/6] misc: " Gustavo Pimentel
  2021-02-03 22:33   ` Greg Kroah-Hartman
@ 2021-02-07  1:36   ` Krzysztof Wilczyński
  2021-02-08 11:21     ` Gustavo Pimentel
  1 sibling, 1 reply; 19+ messages in thread
From: Krzysztof Wilczyński @ 2021-02-07  1:36 UTC (permalink / raw)
  To: Gustavo Pimentel
  Cc: linux-doc, linux-pci, linux-kernel, Derek Kiernan, Dragan Cvetic,
	Arnd Bergmann, Andrew Morton, Greg Kroah-Hartman,
	Jonathan Corbet

Hi Gustavo,

Thank you for all the work here!

A few suggestions.

[...]
> +static void dw_xdata_stop(struct dw_xdata *dw)
> +{
> +	u32 burst = readl(&(__dw_xdara_regs(dw)->burst_cnt));
> +
> +	if (burst & BIT(31)) {
> +		burst &= ~(u32)BIT(31);
> +		writel(burst, &(__dw_regs(dw)->burst_cnt));
> +	}
> +}

Would it be possible to add a define for this "BIT(31)", similarly to
the "XPERF_CONTROL_ENABLE", for example:

  #define XPERF_CONTROL_ENABLE		BIT(5)
  #define XPERF_CONTROL_DISABLE		BIT(31)

What do you think?

> +static void dw_xdata_start(struct dw_xdata *dw, bool write)
> +{
> +	u32 control, status;
> +
> +	/* Stop first if xfer in progress */
> +	dw_xdata_stop(dw);
> +
> +	/* Clear status register */
> +	writel(0x0, &(__dw_regs(dw)->status));
> +
> +	/* Burst count register set for continuous until stopped */
> +	writel(0x80001001, &(__dw_regs(dw)->burst_cnt));

Would you mind adding a define (possibly with a comment, if it makes
sense, of course) rather than open coding it here.

> +	/* Pattern register */
> +	writel(0x0, &(__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));
> +
> +	usleep_range(100, 150);
[...]

Why sleep here?

Do you just add some delay that changes were reflected, or is it
a requirement of sorts?  What do you think about documenting why the
sleep where? Would it make sense?

[...]
> +static void dw_xdata_perf(struct dw_xdata *dw, u64 *rate, bool write)
> +{
> +	u64 data[2], time[2], diff;
> +
> +	/* First measurement */
> +	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));
> +
> +	/* Delay 100ms */
> +	mdelay(100);
[...]

The mdelay() is self-explanatory, so a comment that explains why to take
two measurements that are 100 ms apart and how rate is calculated might
be more useful (unless it would be an overkill here).

If this is an arbitrary delay without any special meaning, then probably
no comment is needed here.

What do you think?

[...]
> +	/* Calculations */

What sort of calculations precisely? :)

[...]
> +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);

This comment might not be needed.

[...]
> +	/* Mapping PCI BAR regions */
> +	err = pcim_iomap_regions(pdev, BIT(pdata->rg_bar), pci_name(pdev));

This comment might also not be needed.

[...]
> +	/* Allocate memory */

And so this comment.

[...]
> +	/* Data structure initialization */
[...]
> +	/* Saving data structure reference */
[...]
> +	/* Sysfs */
[...]

And possibly few of these are also not needed.

Krzysztof

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

* RE: [RESEND v4 4/6] Documentation: misc-devices: Add Documentation for dw-xdata-pcie driver
  2021-02-06 23:31   ` Krzysztof Wilczyński
@ 2021-02-08  9:24     ` Gustavo Pimentel
  2021-02-08 23:11       ` Krzysztof Wilczyński
  0 siblings, 1 reply; 19+ messages in thread
From: Gustavo Pimentel @ 2021-02-08  9:24 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: linux-doc, linux-pci, linux-kernel, Derek Kiernan, Dragan Cvetic,
	Arnd Bergmann, Andrew Morton, Greg Kroah-Hartman,
	Jonathan Corbet

Hi Krzysztof,

On Sat, Feb 6, 2021 at 23:31:14, Krzysztof Wilczyński <kw@linux.com> 
wrote:

> Hi Gustavo,
> 
> [...]
> > +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.
> 
> The driver does not seem to offer any parameters (aside of using sysfs
> for runtime settings), and it also seem to only print what it's doing
> when debug level is enabled - unless I am missing something?
> 
> [...]
> > +Request to stop any current TLP transfer:
> > +- Command:
> > +	echo 1 > /sys/kernel/dw-xdata-pcie/stop
> [...]
> 
> When I do the following:
> 
>   # echo 1 > /sys/kernel/dw-xdata-pcie/write
>   # echo 1 > /sys/kernel/dw-xdata-pcie/stop
>   # cat /sys/kernel/dw-xdata-pcie/write
> 
> Would output from cat above simply show "0 MB/s" then?  I wonder how
> someone using this new driver could tell whether "write" or "read"
> traffic generation has been enabled aside of reading the sysfs files,
> would adding "/sys/kernel/dw-xdata-pcie/active" be an overkill here?
> 
> What do you think?

Yes, it would display 0 MB/s. This driver is to be used mainly by the 
Synopsys DesignWare HW prototyping team. I don't think the general public 
will be interested or can use this driver, because requires a special HW 
block available only for this prototype.

I tried to reduce to the minimal the interfaces, to avoid possible 
confusion. For instance, even the /sys/kernel/dw-xdata-pcie/stop 
interface could be avoided, because on the driver unloading or changing 
the between write or read it calls the stop procedure.

Due to the nature of the HW block, it only can allow the write or the 
read at some given moment, therefore based on the last command enable 
write or read, we know which mode is this driving working.
This driver will be used by the testing team on their automation scripts, 
thus they will know exactly the sequence input.

Anyway, thanks for your feedback.

> 
> Krzysztof



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

* RE: [RESEND v4 1/6] misc: Add Synopsys DesignWare xData IP driver
  2021-02-07  1:36   ` Krzysztof Wilczyński
@ 2021-02-08 11:21     ` Gustavo Pimentel
  2021-02-08 22:53       ` Krzysztof Wilczyński
  0 siblings, 1 reply; 19+ messages in thread
From: Gustavo Pimentel @ 2021-02-08 11:21 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: linux-doc, linux-pci, linux-kernel, Derek Kiernan, Dragan Cvetic,
	Arnd Bergmann, Andrew Morton, Greg Kroah-Hartman,
	Jonathan Corbet

On Sun, Feb 7, 2021 at 1:36:15, Krzysztof Wilczyński <kw@linux.com> 
wrote:

> Hi Gustavo,
> 
> Thank you for all the work here!
> 
> A few suggestions.
> 
> [...]
> > +static void dw_xdata_stop(struct dw_xdata *dw)
> > +{
> > +	u32 burst = readl(&(__dw_xdara_regs(dw)->burst_cnt));
> > +
> > +	if (burst & BIT(31)) {
> > +		burst &= ~(u32)BIT(31);
> > +		writel(burst, &(__dw_regs(dw)->burst_cnt));
> > +	}
> > +}
> 
> Would it be possible to add a define for this "BIT(31)", similarly to
> the "XPERF_CONTROL_ENABLE", for example:
> 
>   #define XPERF_CONTROL_ENABLE		BIT(5)
>   #define XPERF_CONTROL_DISABLE		BIT(31)
> 
> What do you think?

I will a define for them.

> 
> > +static void dw_xdata_start(struct dw_xdata *dw, bool write)
> > +{
> > +	u32 control, status;
> > +
> > +	/* Stop first if xfer in progress */
> > +	dw_xdata_stop(dw);
> > +
> > +	/* Clear status register */
> > +	writel(0x0, &(__dw_regs(dw)->status));
> > +
> > +	/* Burst count register set for continuous until stopped */
> > +	writel(0x80001001, &(__dw_regs(dw)->burst_cnt));
> 
> Would you mind adding a define (possibly with a comment, if it makes
> sense, of course) rather than open coding it here.

Ditto.

> 
> > +	/* Pattern register */
> > +	writel(0x0, &(__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));
> > +
> > +	usleep_range(100, 150);
> [...]
> 
> Why sleep here?
> 
> Do you just add some delay that changes were reflected, or is it
> a requirement of sorts?  What do you think about documenting why the
> sleep where? Would it make sense?

It requires to wait for some to allow the HW block to start the traffic 
generation.
I will some comments explaining that.

> 
> [...]
> > +static void dw_xdata_perf(struct dw_xdata *dw, u64 *rate, bool write)
> > +{
> > +	u64 data[2], time[2], diff;
> > +
> > +	/* First measurement */
> > +	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));
> > +
> > +	/* Delay 100ms */
> > +	mdelay(100);
> [...]
> 
> The mdelay() is self-explanatory, so a comment that explains why to take
> two measurements that are 100 ms apart and how rate is calculated might
> be more useful (unless it would be an overkill here).
> 
> If this is an arbitrary delay without any special meaning, then probably
> no comment is needed here.
> 
> What do you think?
> 
> [...]
> > +	/* Calculations */
> 
> What sort of calculations precisely? :)

Speed calculation. I will add some explanation about it on the code.

> 
> [...]
> > +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);
> 
> This comment might not be needed.

I normally put these comments as a code bookmark.

> 
> [...]
> > +	/* Mapping PCI BAR regions */
> > +	err = pcim_iomap_regions(pdev, BIT(pdata->rg_bar), pci_name(pdev));
> 
> This comment might also not be needed.
> 
> [...]
> > +	/* Allocate memory */
> 
> And so this comment.
> 
> [...]
> > +	/* Data structure initialization */
> [...]
> > +	/* Saving data structure reference */
> [...]
> > +	/* Sysfs */
> [...]
> 
> And possibly few of these are also not needed.
> 
> Krzysztof

Thanks for your review. I will wait for a couple of days, before sending 
a new version of this patch series based on your feedback.

-Gustavo

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

* Re: [RESEND v4 1/6] misc: Add Synopsys DesignWare xData IP driver
  2021-02-08 11:21     ` Gustavo Pimentel
@ 2021-02-08 22:53       ` Krzysztof Wilczyński
  2021-02-09 15:28         ` Gustavo Pimentel
  0 siblings, 1 reply; 19+ messages in thread
From: Krzysztof Wilczyński @ 2021-02-08 22:53 UTC (permalink / raw)
  To: Gustavo Pimentel
  Cc: linux-doc, linux-pci, linux-kernel, Derek Kiernan, Dragan Cvetic,
	Arnd Bergmann, Andrew Morton, Greg Kroah-Hartman,
	Jonathan Corbet, Bjorn Helgaas

[+cc Bjorn]

Hi Gustavo,

[...]
> Thanks for your review. I will wait for a couple of days, before sending 
> a new version of this patch series based on your feedback.

Thank you!

There might be one more change, and improvement, to be done as per
Bjorn's feedback, see:

  https://lore.kernel.org/linux-pci/20210208193516.GA406304@bjorn-Precision-5520/

The code in question would be (exceprt from the patch):

[...]
+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;
[...]
+	dw->rg_region.vaddr = pcim_iomap_table(pdev)[pdata->rg_bar];
+	if (!dw->rg_region.vaddr)
+		return -ENOMEM;
[...]

Perhaps something like the following would would?

void __iomem * const *iomap_table;

iomap_table = pcim_iomap_table(pdev);
if (!iomap_table)
        return -ENOMEM;

dw->rg_region.vaddr = iomap_table[pdata->rg_bar];
if (!dw->rg_region.vaddr)
	return -ENOMEM;

With sensible error messages added, of course.  What do you think?

Krzysztof

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

* Re: [RESEND v4 4/6] Documentation: misc-devices: Add Documentation for dw-xdata-pcie driver
  2021-02-08  9:24     ` Gustavo Pimentel
@ 2021-02-08 23:11       ` Krzysztof Wilczyński
  0 siblings, 0 replies; 19+ messages in thread
From: Krzysztof Wilczyński @ 2021-02-08 23:11 UTC (permalink / raw)
  To: Gustavo Pimentel
  Cc: linux-doc, linux-pci, linux-kernel, Derek Kiernan, Dragan Cvetic,
	Arnd Bergmann, Andrew Morton, Greg Kroah-Hartman,
	Jonathan Corbet

Hi Gustavo,

[...]
> > When I do the following:
> > 
> >   # echo 1 > /sys/kernel/dw-xdata-pcie/write
> >   # echo 1 > /sys/kernel/dw-xdata-pcie/stop
> >   # cat /sys/kernel/dw-xdata-pcie/write
> > 
> > Would output from cat above simply show "0 MB/s" then?  I wonder how
> > someone using this new driver could tell whether "write" or "read"
> > traffic generation has been enabled aside of reading the sysfs files,
> > would adding "/sys/kernel/dw-xdata-pcie/active" be an overkill here?
> > 
> > What do you think?
> 
> Yes, it would display 0 MB/s. This driver is to be used mainly by the 
> Synopsys DesignWare HW prototyping team. I don't think the general public 
> will be interested or can use this driver, because requires a special HW 
> block available only for this prototype.

Got it.

> I tried to reduce to the minimal the interfaces, to avoid possible 
> confusion. For instance, even the /sys/kernel/dw-xdata-pcie/stop 
> interface could be avoided, because on the driver unloading or changing 
> the between write or read it calls the stop procedure.

Oh, OK.
 
> Due to the nature of the HW block, it only can allow the write or the 
> read at some given moment, therefore based on the last command enable 
> write or read, we know which mode is this driving working.
> This driver will be used by the testing team on their automation scripts, 
> thus they will know exactly the sequence input.
> 
> Anyway, thanks for your feedback.

Thank you for taking the time to add more details for me.  Much
appreciated.

Krzysztof

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

* RE: [RESEND v4 1/6] misc: Add Synopsys DesignWare xData IP driver
  2021-02-08 22:53       ` Krzysztof Wilczyński
@ 2021-02-09 15:28         ` Gustavo Pimentel
  2021-02-09 17:24           ` Krzysztof Wilczyński
  2021-02-09 18:52           ` Bjorn Helgaas
  0 siblings, 2 replies; 19+ messages in thread
From: Gustavo Pimentel @ 2021-02-09 15:28 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: linux-doc, linux-pci, linux-kernel, Derek Kiernan, Dragan Cvetic,
	Arnd Bergmann, Andrew Morton, Greg Kroah-Hartman,
	Jonathan Corbet, Bjorn Helgaas

On Mon, Feb 8, 2021 at 22:53:54, Krzysztof Wilczyński <kw@linux.com> 
wrote:

> [+cc Bjorn]
> 
> Hi Gustavo,
> 
> [...]
> > Thanks for your review. I will wait for a couple of days, before sending 
> > a new version of this patch series based on your feedback.
> 
> Thank you!
> 
> There might be one more change, and improvement, to be done as per
> Bjorn's feedback, see:
> 
>   https://urldefense.com/v3/__https://lore.kernel.org/linux-pci/20210208193516.GA406304@bjorn-Precision-5520/__;!!A4F2R9G_pg!Oxp56pU_UN6M2BhfNRSdYqsFUncqVklBj_1IdLQD_w_V6dKRPDO_FjPUystMa5D39SRj8uo$ 
> 
> The code in question would be (exceprt from the patch):
> 
> [...]
> +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;
> [...]
> +	dw->rg_region.vaddr = pcim_iomap_table(pdev)[pdata->rg_bar];
> +	if (!dw->rg_region.vaddr)
> +		return -ENOMEM;
> [...]
> 
> Perhaps something like the following would would?
> 
> void __iomem * const *iomap_table;
> 
> iomap_table = pcim_iomap_table(pdev);
> if (!iomap_table)
>         return -ENOMEM;
> 
> dw->rg_region.vaddr = iomap_table[pdata->rg_bar];
> if (!dw->rg_region.vaddr)
> 	return -ENOMEM;
> 
> With sensible error messages added, of course.  What do you think?

I think all the improvements are welcome. I will do that.
My only doubt is if Bjorn recommends removing the 
iomap_table[pdata->rg_bar] check, after adding the verification on the 
pcim_iomap_table, because all other drivers doesn't do that.

Thanks!

-Gustavo
> 
> Krzysztof



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

* Re: [RESEND v4 1/6] misc: Add Synopsys DesignWare xData IP driver
  2021-02-09 15:28         ` Gustavo Pimentel
@ 2021-02-09 17:24           ` Krzysztof Wilczyński
  2021-02-09 18:52           ` Bjorn Helgaas
  1 sibling, 0 replies; 19+ messages in thread
From: Krzysztof Wilczyński @ 2021-02-09 17:24 UTC (permalink / raw)
  To: Gustavo Pimentel
  Cc: linux-doc, linux-pci, linux-kernel, Derek Kiernan, Dragan Cvetic,
	Arnd Bergmann, Andrew Morton, Greg Kroah-Hartman,
	Jonathan Corbet, Bjorn Helgaas

Hi Gustavo,

[...]
> > The code in question would be (exceprt from the patch):
> > 
> > [...]
> > +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;
> > [...]
> > +	dw->rg_region.vaddr = pcim_iomap_table(pdev)[pdata->rg_bar];
> > +	if (!dw->rg_region.vaddr)
> > +		return -ENOMEM;
> > [...]
> > 
> > Perhaps something like the following would would?
> > 
> > void __iomem * const *iomap_table;
> > 
> > iomap_table = pcim_iomap_table(pdev);
> > if (!iomap_table)
> >         return -ENOMEM;
> > 
> > dw->rg_region.vaddr = iomap_table[pdata->rg_bar];
> > if (!dw->rg_region.vaddr)
> > 	return -ENOMEM;
> > 
> > With sensible error messages added, of course.  What do you think?
> 
> I think all the improvements are welcome. I will do that.
> My only doubt is if Bjorn recommends removing the 
> iomap_table[pdata->rg_bar] check, after adding the verification on the 
> pcim_iomap_table, because all other drivers doesn't do that.

Good point.  I only found two drivers that do this extra check:

  https://elixir.bootlin.com/linux/v5.11-rc7/source/drivers/crypto/ccp/sp-pci.c#L203
  https://elixir.bootlin.com/linux/v5.11-rc7/source/drivers/net/ethernet/amd/xgbe/xgbe-pci.c#L252

Bjorn, do you think that there is some likelihood that the table might
be missing a mapped address for a given BAR?

I don't think that is the case, but should it be the case, then perhaps
adding a small wrapper that would take a BAR and do all the verification
internally might be a good idea.

Krzysztof

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

* Re: [RESEND v4 1/6] misc: Add Synopsys DesignWare xData IP driver
  2021-02-09 15:28         ` Gustavo Pimentel
  2021-02-09 17:24           ` Krzysztof Wilczyński
@ 2021-02-09 18:52           ` Bjorn Helgaas
  1 sibling, 0 replies; 19+ messages in thread
From: Bjorn Helgaas @ 2021-02-09 18:52 UTC (permalink / raw)
  To: Gustavo Pimentel
  Cc: Krzysztof Wilczyński, linux-doc, linux-pci, linux-kernel,
	Derek Kiernan, Dragan Cvetic, Arnd Bergmann, Andrew Morton,
	Greg Kroah-Hartman, Jonathan Corbet

On Tue, Feb 09, 2021 at 03:28:16PM +0000, Gustavo Pimentel wrote:
> On Mon, Feb 8, 2021 at 22:53:54, Krzysztof Wilczyński <kw@linux.com> 
> wrote:
> > [...]
> > > Thanks for your review. I will wait for a couple of days, before sending 
> > > a new version of this patch series based on your feedback.
> > 
> > Thank you!
> > 
> > There might be one more change, and improvement, to be done as per
> > Bjorn's feedback, see:
> > 
> >   https://urldefense.com/v3/__https://lore.kernel.org/linux-pci/20210208193516.GA406304@bjorn-Precision-5520/__;!!A4F2R9G_pg!Oxp56pU_UN6M2BhfNRSdYqsFUncqVklBj_1IdLQD_w_V6dKRPDO_FjPUystMa5D39SRj8uo$ 
> > 
> > The code in question would be (exceprt from the patch):
> > 
> > [...]
> > +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;
> > [...]
> > +	dw->rg_region.vaddr = pcim_iomap_table(pdev)[pdata->rg_bar];
> > +	if (!dw->rg_region.vaddr)
> > +		return -ENOMEM;
> > [...]
> > 
> > Perhaps something like the following would would?
> > 
> > void __iomem * const *iomap_table;
> > 
> > iomap_table = pcim_iomap_table(pdev);
> > if (!iomap_table)
> >         return -ENOMEM;
> > 
> > dw->rg_region.vaddr = iomap_table[pdata->rg_bar];
> > if (!dw->rg_region.vaddr)
> > 	return -ENOMEM;
> > 
> > With sensible error messages added, of course.  What do you think?
> 
> I think all the improvements are welcome. I will do that.
> My only doubt is if Bjorn recommends removing the 
> iomap_table[pdata->rg_bar] check, after adding the verification on the 
> pcim_iomap_table, because all other drivers doesn't do that.

I misunderstood the usage of pcim_iomap_table() -- it looks like one
must call pcim_iomap_regions() *first*, and test its result, and
*that* is where we should catch any pcim_iomap_table() failures, e.g.,

  rc = pcim_iomap_regions()   # or pcim_iomap_regions_request_all()
  if (rc)
    return rc;                # pcim_iomap_table() or other failure

  vaddr = pcim_iomap_table()[BAR];
  if (!vaddr)
    return -ENOMEM;           # BAR doesn't exist

You *do* correctly call pcim_iomap_regions() first, which calls
pcim_imap_table() internally, so if pcim_iomap_table() were to return
NULL, you should catch it there.

Then we assume that the subsequent "pcim_iomap_table()[BAR]" call will
succeed and NOT return NULL, so it should be safe to index into the
table.  And if the table[BAR] entry is NULL, it means the BAR doesn't
exist or isn't mapped.

That sort of makes sense, but the API design doesn't quite seem
obviously correct to me.  The table was created by
pcim_iomap_regions(), and pcim_iomap_table() is basically retrieving
that artifact.

I wonder if it could be improved by making pcim_iomap_table() strictly
internal to devres.c and having the pcim_iomap functions return the
table directly.  Then the code would look something like this:

  table = pcim_iomap_regions();
  if (IS_ERR(table))
    return PTR_ERR(table);    # pcim_iomap_table() or other failure

  vaddr = table[BAR];         # "table" is guaranteed to be non-NULL
  if (!vaddr)
    return -ENOMEM;

Obviously this is not something you should do for *this* series.
I think you should follow the example of other drivers, which means
keeping your patch exactly as you posted it.  I'm just interested in
opinions on this as a possible future API improvement.

Bjorn

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

end of thread, other threads:[~2021-02-09 19:05 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-03 22:12 [RESEND v4 0/6] misc: Add Add Synopsys DesignWare xData IP driver Gustavo Pimentel
2021-02-03 22:12 ` [RESEND v4 1/6] misc: " Gustavo Pimentel
2021-02-03 22:33   ` Greg Kroah-Hartman
2021-02-07  1:36   ` Krzysztof Wilczyński
2021-02-08 11:21     ` Gustavo Pimentel
2021-02-08 22:53       ` Krzysztof Wilczyński
2021-02-09 15:28         ` Gustavo Pimentel
2021-02-09 17:24           ` Krzysztof Wilczyński
2021-02-09 18:52           ` Bjorn Helgaas
2021-02-03 22:12 ` [RESEND v4 2/6] misc: Add Synopsys DesignWare xData IP driver to Makefile Gustavo Pimentel
2021-02-03 22:12 ` [RESEND v4 3/6] misc: Add Synopsys DesignWare xData IP driver to Kconfig Gustavo Pimentel
2021-02-03 22:26   ` Greg Kroah-Hartman
2021-02-03 22:27   ` Greg Kroah-Hartman
2021-02-03 22:12 ` [RESEND v4 4/6] Documentation: misc-devices: Add Documentation for dw-xdata-pcie driver Gustavo Pimentel
2021-02-06 23:31   ` Krzysztof Wilczyński
2021-02-08  9:24     ` Gustavo Pimentel
2021-02-08 23:11       ` Krzysztof Wilczyński
2021-02-03 22:12 ` [RESEND v4 5/6] MAINTAINERS: Add Synopsys xData IP driver maintainer Gustavo Pimentel
2021-02-03 22:12 ` [RESEND v4 6/6] docs: ABI: Add sysfs documentation interface of dw-xdata-pcie driver Gustavo Pimentel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).