Re: [PATCH v7 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA
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
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
>> +#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
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
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