Re: [PATCH v7 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA

2021-03-21 Thread Sanjay R Mehta



On 3/22/2021 11:34 AM, Vinod Koul wrote:
> [CAUTION: External Email]
> 
> On 18-03-21, 16:16, Sanjay R Mehta wrote:
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
 +#include 
>>>
>>> why do you need sched.h here?
>>>
 +
 +#include "ptdma.h"
 +
 +/* Ever-increasing value to produce unique unit numbers */
 +static atomic_t pt_ordinal;
>>>
>>> What is the need of that?
>>>
>>
> 
> [please wrap your emails within 80 chars]
> 
Sure Vinod.

>> The "pt_ordinal" is incremented for each DMA instances and its number
>> is used only to assign device name for each instances.  This same
>> device name is passed as a string parameter in many places in code
>> like while using request_irq(), dma_pool_create() and in debugfs.
> 
> Why do you need that, why not use device name which is unique..?
> 
Can we take this as part of bug fixes series in future?

>> Also, I have implemented all of the comments for this patch except
>> this. if this is fine, will send the next version for review.
> 
> Am not sure I remember all the comments I gave, it has been _quite_ a
> while since the feedback was provided. In order to have effective review
> it would be great to revert back on a reasonable timeline and discuss...
> 
Apologies from my side. I understand that I have taken more time. But I assure 
it doesn't happen again.
I have already sent out v8, can you please have a look at and provide your 
valuable feedback.


> Thanks
> --
> ~Vinod
> 


Re: [PATCH v7 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA

2021-03-21 Thread Vinod Koul
On 18-03-21, 16:16, Sanjay R Mehta wrote:
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> > 
> > why do you need sched.h here?
> > 
> >> +
> >> +#include "ptdma.h"
> >> +
> >> +/* Ever-increasing value to produce unique unit numbers */
> >> +static atomic_t pt_ordinal;
> > 
> > What is the need of that?
> > 
> 

[please wrap your emails within 80 chars]

> The "pt_ordinal" is incremented for each DMA instances and its number
> is used only to assign device name for each instances.  This same
> device name is passed as a string parameter in many places in code
> like while using request_irq(), dma_pool_create() and in debugfs.

Why do you need that, why not use device name which is unique..?

> Also, I have implemented all of the comments for this patch except
> this. if this is fine, will send the next version for review.

Am not sure I remember all the comments I gave, it has been _quite_ a
while since the feedback was provided. In order to have effective review
it would be great to revert back on a reasonable timeline and discuss...

Thanks
-- 
~Vinod


Re: [PATCH v7 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA

2021-03-18 Thread Sanjay R Mehta
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
> 
> why do you need sched.h here?
> 
>> +
>> +#include "ptdma.h"
>> +
>> +/* Ever-increasing value to produce unique unit numbers */
>> +static atomic_t pt_ordinal;
> 
> What is the need of that?
> 

The "pt_ordinal" is incremented for each DMA instances and its number is used 
only to assign device name for each instances.
This same device name is passed as a string parameter in many places in code 
like while using request_irq(), dma_pool_create() and in debugfs.

Also, I have implemented all of the comments for this patch except this. if 
this is fine, will send the next version for review.


Thanks,
Sanjay Mehta


>> +static int pt_get_irqs(struct pt_device *pt)
>> +{
>> + struct device *dev = pt->dev;
>> + int ret;
>> +
>> + ret = pt_get_msix_irqs(pt);
>> + if (!ret)
>> + return 0;
>> +
>> + /* Couldn't get MSI-X vectors, try MSI */
>> + dev_dbg(dev, "could not enable MSI-X (%d), trying MSI\n", ret);
>> + ret = pt_get_msi_irq(pt);




Re: [PATCH v7 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA

2020-11-18 Thread Vinod Koul
On 16-10-20, 02:39, Sanjay R Mehta wrote:
> From: Sanjay R Mehta 
> 
> Adding support for AMD PTDMA controller. It performs high-bandwidth

Add support or Adds support.. would be apt!

> memory to memory and IO copy operation. Device commands are managed
> via a circular queue of 'descriptors', each of which specifies source
> and destination addresses for copying a single buffer of data.
> 
> Signed-off-by: Sanjay R Mehta 
> ---
>  MAINTAINERS   |   6 +
>  drivers/dma/Kconfig   |   2 +
>  drivers/dma/Makefile  |   1 +
>  drivers/dma/ptdma/Kconfig |  11 ++
>  drivers/dma/ptdma/Makefile|  10 ++
>  drivers/dma/ptdma/ptdma-dev.c | 296 +++
>  drivers/dma/ptdma/ptdma-pci.c | 252 +
>  drivers/dma/ptdma/ptdma.h | 316 
> ++
>  8 files changed, 894 insertions(+)
>  create mode 100644 drivers/dma/ptdma/Kconfig
>  create mode 100644 drivers/dma/ptdma/Makefile
>  create mode 100644 drivers/dma/ptdma/ptdma-dev.c
>  create mode 100644 drivers/dma/ptdma/ptdma-pci.c
>  create mode 100644 drivers/dma/ptdma/ptdma.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 33b27e6..f6ae0d1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -943,6 +943,12 @@ S:   Supported
>  F:   arch/arm64/boot/dts/amd/amd-seattle-xgbe*.dtsi
>  F:   drivers/net/ethernet/amd/xgbe/
>  
> +AMD PTDMA DRIVER
> +M:   Sanjay R Mehta 
> +L:   dmaeng...@vger.kernel.org
> +S:   Maintained
> +F:   drivers/dma/ptdma/
> +
>  ANALOG DEVICES INC AD5686 DRIVER
>  M:   Michael Hennerich 
>  L:   linux...@vger.kernel.org
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 518a143..a21c983 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -748,6 +748,8 @@ source "drivers/dma/ti/Kconfig"
>  
>  source "drivers/dma/fsl-dpaa2-qdma/Kconfig"
>  
> +source "drivers/dma/ptdma/Kconfig"
> +
>  # clients
>  comment "DMA Clients"
>   depends on DMA_ENGINE
> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> index e60f813..2785756 100644
> --- a/drivers/dma/Makefile
> +++ b/drivers/dma/Makefile
> @@ -83,6 +83,7 @@ obj-$(CONFIG_XGENE_DMA) += xgene-dma.o
>  obj-$(CONFIG_ZX_DMA) += zx_dma.o
>  obj-$(CONFIG_ST_FDMA) += st_fdma.o
>  obj-$(CONFIG_FSL_DPAA2_QDMA) += fsl-dpaa2-qdma/
> +obj-$(CONFIG_AMD_PTDMA) += ptdma/

Please keep this sorted :(

>  obj-y += mediatek/
>  obj-y += qcom/
> diff --git a/drivers/dma/ptdma/Kconfig b/drivers/dma/ptdma/Kconfig
> new file mode 100644
> index 000..f93f9c2
> --- /dev/null
> +++ b/drivers/dma/ptdma/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config AMD_PTDMA
> + tristate  "AMD PassThru DMA Engine"
> + depends on X86_64 && PCI
> + help
> +   Enable support for the AMD PTDMA controller.  This controller
> +   provides DMA capabilities & performs high bandwidth memory to
> +   memory and IO copy operation and performs DMA transfer through
> +   queue based descriptor management. This DMA controller is intended
> +   to use with AMD Non-Transparent Bridge devices and not for general
> +   purpose slave DMA.

s/slave/peripheral

> +static int pt_core_execute_cmd(struct ptdma_desc *desc,
> +struct pt_cmd_queue *cmd_q)
> +{
> + __le32 *mp;
> + u32 *dp;
> + u32 tail;
> + int i;
> +
> + if (desc->dw0.soc) {
> + desc->dw0.ioc = 1;
> + desc->dw0.soc = 0;
> + }
> + mutex_lock(&cmd_q->q_mutex);
> +
> + mp = (__le32 *)&cmd_q->qbase[cmd_q->qidx];

why not make the qidx __le32 instead of int ?

> + dp = (u32 *)desc;
> + for (i = 0; i < 8; i++)

why 8..?

> + mp[i] = cpu_to_le32(dp[i]); /* handle endianness */

Where is this used..? Why not drop this..

> +static irqreturn_t pt_core_irq_handler(int irq, void *data)
> +{
> + struct pt_device *pt = (struct pt_device *)data;

why do you need cast away from void *

> +int pt_core_init(struct pt_device *pt)
> +{
> + char dma_pool_name[MAX_DMAPOOL_NAME_LEN];
> + struct pt_cmd_queue *cmd_q = &pt->cmd_q;
> + u32 dma_addr_lo, dma_addr_hi;
> + struct device *dev = pt->dev;
> + struct dma_pool *dma_pool;
> + int ret;
> +
> + /* Allocate a dma pool for the queue */
> + snprintf(dma_pool_name, sizeof(dma_pool_name), "%s_q", pt->name);
> +
> + dma_pool = dma_pool_create(dma_pool_name, dev,
> +PT_DMAPOOL_MAX_SIZE,
> +PT_DMAPOOL_ALIGN, 0);
> + if (!dma_pool) {
> + dev_err(dev, "unable to allocate dma pool\n");
> + ret = -ENOMEM;
> + return ret;

return -ENOMEM?

> + }
> +
> + /* ptdma core initialisation */
> + iowrite32(CMD_CONFIG_VHB_EN, pt->io_regs + CMD_CONFIG_OFFSET);
> + iowrite32(CMD_QUEUE_PRIO, pt->io_regs + CMD_QUEUE_PRIO_OFFSET);
> + iowrite32(CMD_TIMEOUT_DISABLE, pt->io_regs + CMD_TIMEOUT_

[PATCH v7 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA

2020-10-16 Thread Sanjay R Mehta
From: Sanjay R Mehta 

Adding support for AMD PTDMA controller. It performs high-bandwidth
memory to memory and IO copy operation. Device commands are managed
via a circular queue of 'descriptors', each of which specifies source
and destination addresses for copying a single buffer of data.

Signed-off-by: Sanjay R Mehta 
---
 MAINTAINERS   |   6 +
 drivers/dma/Kconfig   |   2 +
 drivers/dma/Makefile  |   1 +
 drivers/dma/ptdma/Kconfig |  11 ++
 drivers/dma/ptdma/Makefile|  10 ++
 drivers/dma/ptdma/ptdma-dev.c | 296 +++
 drivers/dma/ptdma/ptdma-pci.c | 252 +
 drivers/dma/ptdma/ptdma.h | 316 ++
 8 files changed, 894 insertions(+)
 create mode 100644 drivers/dma/ptdma/Kconfig
 create mode 100644 drivers/dma/ptdma/Makefile
 create mode 100644 drivers/dma/ptdma/ptdma-dev.c
 create mode 100644 drivers/dma/ptdma/ptdma-pci.c
 create mode 100644 drivers/dma/ptdma/ptdma.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 33b27e6..f6ae0d1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -943,6 +943,12 @@ S: Supported
 F: arch/arm64/boot/dts/amd/amd-seattle-xgbe*.dtsi
 F: drivers/net/ethernet/amd/xgbe/
 
+AMD PTDMA DRIVER
+M: Sanjay R Mehta 
+L: dmaeng...@vger.kernel.org
+S: Maintained
+F: drivers/dma/ptdma/
+
 ANALOG DEVICES INC AD5686 DRIVER
 M: Michael Hennerich 
 L: linux...@vger.kernel.org
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 518a143..a21c983 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -748,6 +748,8 @@ source "drivers/dma/ti/Kconfig"
 
 source "drivers/dma/fsl-dpaa2-qdma/Kconfig"
 
+source "drivers/dma/ptdma/Kconfig"
+
 # clients
 comment "DMA Clients"
depends on DMA_ENGINE
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index e60f813..2785756 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -83,6 +83,7 @@ obj-$(CONFIG_XGENE_DMA) += xgene-dma.o
 obj-$(CONFIG_ZX_DMA) += zx_dma.o
 obj-$(CONFIG_ST_FDMA) += st_fdma.o
 obj-$(CONFIG_FSL_DPAA2_QDMA) += fsl-dpaa2-qdma/
+obj-$(CONFIG_AMD_PTDMA) += ptdma/
 
 obj-y += mediatek/
 obj-y += qcom/
diff --git a/drivers/dma/ptdma/Kconfig b/drivers/dma/ptdma/Kconfig
new file mode 100644
index 000..f93f9c2
--- /dev/null
+++ b/drivers/dma/ptdma/Kconfig
@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config AMD_PTDMA
+   tristate  "AMD PassThru DMA Engine"
+   depends on X86_64 && PCI
+   help
+ Enable support for the AMD PTDMA controller.  This controller
+ provides DMA capabilities & performs high bandwidth memory to
+ memory and IO copy operation and performs DMA transfer through
+ queue based descriptor management. This DMA controller is intended
+ to use with AMD Non-Transparent Bridge devices and not for general
+ purpose slave DMA.
diff --git a/drivers/dma/ptdma/Makefile b/drivers/dma/ptdma/Makefile
new file mode 100644
index 000..320fa82
--- /dev/null
+++ b/drivers/dma/ptdma/Makefile
@@ -0,0 +1,10 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# AMD Passthru DMA driver
+#
+
+obj-$(CONFIG_AMD_PTDMA) += ptdma.o
+
+ptdma-objs := ptdma-dev.o
+
+ptdma-$(CONFIG_PCI) += ptdma-pci.o
diff --git a/drivers/dma/ptdma/ptdma-dev.c b/drivers/dma/ptdma/ptdma-dev.c
new file mode 100644
index 000..d1e9892
--- /dev/null
+++ b/drivers/dma/ptdma/ptdma-dev.c
@@ -0,0 +1,296 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * AMD Passthru DMA device driver
+ * -- Based on the CCP driver
+ *
+ * Copyright (C) 2016,2020 Advanced Micro Devices, Inc.
+ *
+ * Author: Sanjay R Mehta 
+ * Author: Gary R Hook 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "ptdma.h"
+
+/* Human-readable error strings */
+static char *pt_error_codes[] = {
+   "",
+   "ERR 01: ILLEGAL_ENGINE",
+   "ERR 03: ILLEGAL_FUNCTION_TYPE",
+   "ERR 04: ILLEGAL_FUNCTION_MODE",
+   "ERR 06: ILLEGAL_FUNCTION_SIZE",
+   "ERR 08: ILLEGAL_FUNCTION_RSVD",
+   "ERR 09: ILLEGAL_BUFFER_LENGTH",
+   "ERR 10: VLSB_FAULT",
+   "ERR 11: ILLEGAL_MEM_ADDR",
+   "ERR 12: ILLEGAL_MEM_SEL",
+   "ERR 13: ILLEGAL_CONTEXT_ID",
+   "ERR 15: 0xF Reserved",
+   "ERR 18: CMD_TIMEOUT",
+   "ERR 19: IDMA0_AXI_SLVERR",
+   "ERR 20: IDMA0_AXI_DECERR",
+   "ERR 21: 0x15 Reserved",
+   "ERR 22: IDMA1_AXI_SLAVE_FAULT",
+   "ERR 23: IDMA1_AIXI_DECERR",
+   "ERR 24: 0x18 Reserved",
+   "ERR 27: 0x1B Reserved",
+   "ERR 38: ODMA0_AXI_SLVERR",
+   "ERR 39: ODMA0_AXI_DECERR",
+   "ERR 40: 0x28 Reserved",
+   "ERR 41: ODMA1_AXI_SLVERR",
+   "ERR 42: ODMA1_AXI_DECERR",
+   "ERR 43: LSB_PARITY_ERR",
+};
+
+static void pt_log_error(struct pt_device *d, int e)
+{
+   dev_err(d->dev, "PTDMA error: %s (0x%x)\n", pt_error_codes[e], e);
+}
+
+void pt_start_queue(struct pt_cmd_queue *cmd_q)
+{
+   /* Turn