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@xxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx>
> + */
> +
> +#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
