* [PATCH v9 0/3] Add support for AMD PTDMA controller driver
@ 2021-06-02 17:22 Sanjay R Mehta
2021-06-02 17:22 ` [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA Sanjay R Mehta
` (2 more replies)
0 siblings, 3 replies; 26+ messages in thread
From: Sanjay R Mehta @ 2021-06-02 17:22 UTC (permalink / raw)
To: vkoul
Cc: gregkh, dan.j.williams, Thomas.Lendacky, Shyam-sundar.S-k,
Nehal-bakulchandra.Shah, robh, mchehab+samsung, davem,
linux-kernel, dmaengine, Sanjay R Mehta
From: Sanjay R Mehta <sanju.mehta@amd.com>
This patch series add support for AMD PTDMA controller which
performs high bandwidth memory-to-memory and IO copy operation,
performs DMA transfer through queue based descriptor management.
AMD Processor has multiple ptdma device instances with each controller
having single queue. The driver also adds support for for multiple PTDMA
instances, each device will get an unique identifier and uniquely
named resources.
v9:
- Modified the help message in Kconfig as per Randy's comment.
- reverted the split of code for "pt_handle_active_desc" as there
was driver hang being observerd after few iterations.
v8:
- splitted the code into different functions, one to find active desc
and second to complete and invoke callback.
- used FIELD_PREP & FIELD_GET instead of union struct bitfields.
- modified all style fixes as per the comments.
v7:
- Fixed module warnings reported ( by kernel test robot <lkp@intel.com> ).
v6:
- Removed debug artifacts and made the suggested cosmetic changes.
- implemented and used to_pt_chan and to_pt_desc inline functions.
- Removed src and dst address check as framework does this.
- Removed devm_kcalloc() usage and used devm_kzalloc() api.
- Using framework debugfs directory to store dma info.
v5:
- modified code to submit next tranction in ISR itself and removed the tasklet.
- implemented .device_synchronize API.
- converted debugfs code by using DEFINE_SHOW_ATTRIBUTE()
- using dbg_dev_root for debugfs root directory.
- removed dma_status from pt_dma_chan
- removed module parameter cmd_queue_lenght.
- removed global device list for multiple devics.
- removed code related to dynamic adding/deleting to device list
- removed pt_add_device and pt_del_device functions
v4:
- modified DMA channel and descriptor management using virt-dma layer
instead of list based management.
- return only status of the cookie from pt_tx_status
- copyright year changed from 2019 to 2020
- removed dummy code for suspend & resume
- used bitmask and genmask
v3:
- Fixed the sparse warnings.
v2:
- Added controller description in cover letter
- Removed "default m" from Kconfig
- Replaced low_address() and high_address() functions with kernel
API's lower_32_bits & upper_32_bits().
- Removed the BH handler function pt_core_irq_bh() and instead
handling transaction in irq handler itself.
- Moved presetting of command queue registers into new function
"init_cmdq_regs()"
- Removed the kernel thread dependency to submit transaction.
- Increased the hardware command queue size to 32 and adding it
as a module parameter.
- Removed backlog command queue handling mechanism.
- Removed software command queue handling and instead submitting
transaction command directly to
hardware command queue.
- Added tasklet structure variable in "struct pt_device".
This is used to invoke pt_do_cmd_complete() upon receiving interrupt
for command completion.
- pt_core_perform_passthru() function parameters are modified and it is
now used to submit command directly to hardware from dmaengine framew
- Removed below structures, enums, macros and functions, as these value
constants. Making command submission simple,
- Removed "union pt_function" and several macros like PT_VERSION,
PT_BYTESWAP, PT_CMD_* etc..
- enum pt_passthru_bitwise, enum pt_passthru_byteswap, enum pt_memty
struct pt_dma_info, struct pt_data, struct pt_mem, struct pt_passt
struct pt_op,
Links of the review comments for v7:
1. https://lkml.org/lkml/2020/11/18/351
2. https://lkml.org/lkml/2020/11/18/384
Links of the review comments for v5:
1. https://lkml.org/lkml/2020/7/3/154
2. https://lkml.org/lkml/2020/8/25/431
3. https://lkml.org/lkml/2020/7/3/177
4. https://lkml.org/lkml/2020/7/3/186
Links of the review comments for v5:
1. https://lkml.org/lkml/2020/5/4/42
2. https://lkml.org/lkml/2020/5/4/45
3. https://lkml.org/lkml/2020/5/4/38
4. https://lkml.org/lkml/2020/5/26/70
Links of the review comments for v4:
1. https://lkml.org/lkml/2020/1/24/12
2. https://lkml.org/lkml/2020/1/24/17
Links of the review comments for v2:
1https://lkml.org/lkml/2019/12/27/630
2. https://lkml.org/lkml/2020/1/3/23
3. https://lkml.org/lkml/2020/1/3/314
4. https://lkml.org/lkml/2020/1/10/100
Links of the review comments for v1:
1. https://lkml.org/lkml/2019/9/24/490
2. https://lkml.org/lkml/2019/9/24/399
3. https://lkml.org/lkml/2019/9/24/862
4. https://lkml.org/lkml/2019/9/24/122
Sanjay R Mehta (3):
dmaengine: ptdma: Initial driver for the AMD PTDMA
dmaengine: ptdma: register PTDMA controller as a DMA resource
dmaengine: ptdma: Add debugfs entries for PTDMA
MAINTAINERS | 6 +
drivers/dma/Kconfig | 2 +
drivers/dma/Makefile | 1 +
drivers/dma/ptdma/Kconfig | 13 +
drivers/dma/ptdma/Makefile | 10 +
drivers/dma/ptdma/ptdma-debugfs.c | 115 +++++++++
drivers/dma/ptdma/ptdma-dev.c | 327 +++++++++++++++++++++++++
drivers/dma/ptdma/ptdma-dmaengine.c | 460 ++++++++++++++++++++++++++++++++++++
drivers/dma/ptdma/ptdma-pci.c | 251 ++++++++++++++++++++
drivers/dma/ptdma/ptdma.h | 342 +++++++++++++++++++++++++++
10 files changed, 1527 insertions(+)
create mode 100644 drivers/dma/ptdma/Kconfig
create mode 100644 drivers/dma/ptdma/Makefile
create mode 100644 drivers/dma/ptdma/ptdma-debugfs.c
create mode 100644 drivers/dma/ptdma/ptdma-dev.c
create mode 100644 drivers/dma/ptdma/ptdma-dmaengine.c
create mode 100644 drivers/dma/ptdma/ptdma-pci.c
create mode 100644 drivers/dma/ptdma/ptdma.h
--
2.7.4
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA
2021-06-02 17:22 [PATCH v9 0/3] Add support for AMD PTDMA controller driver Sanjay R Mehta
@ 2021-06-02 17:22 ` Sanjay R Mehta
2021-06-08 17:39 ` Vinod Koul
2021-06-02 17:22 ` [PATCH v9 2/3] dmaengine: ptdma: register PTDMA controller as a DMA resource Sanjay R Mehta
2021-06-02 17:22 ` [PATCH v9 3/3] dmaengine: ptdma: Add debugfs entries for PTDMA Sanjay R Mehta
2 siblings, 1 reply; 26+ messages in thread
From: Sanjay R Mehta @ 2021-06-02 17:22 UTC (permalink / raw)
To: vkoul
Cc: gregkh, dan.j.williams, Thomas.Lendacky, Shyam-sundar.S-k,
Nehal-bakulchandra.Shah, robh, mchehab+samsung, davem,
linux-kernel, dmaengine, Sanjay R Mehta
From: Sanjay R Mehta <sanju.mehta@amd.com>
Add 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 <sanju.mehta@amd.com>
---
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 | 290 +++++++++++++++++++++++++++++++++++++++
drivers/dma/ptdma/ptdma-pci.c | 251 ++++++++++++++++++++++++++++++++++
drivers/dma/ptdma/ptdma.h | 305 ++++++++++++++++++++++++++++++++++++++++++
8 files changed, 876 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 bd7aff0c..2c897fa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -957,6 +957,12 @@ S: Supported
T: git git://people.freedesktop.org/~agd5f/linux
F: drivers/gpu/drm/amd/pm/powerplay/
++AMD PTDMA DRIVER
++M: Sanjay R Mehta <sanju.mehta@amd.com>
++L: dmaengine@vger.kernel.org
++S: Maintained
++F: drivers/dma/ptdma/
+
AMD SEATTLE DEVICE TREE SUPPORT
M: Brijesh Singh <brijeshkumar.singh@amd.com>
M: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 6ab9d9a..7900f1a 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -714,6 +714,8 @@ source "drivers/dma/bestcomm/Kconfig"
source "drivers/dma/mediatek/Kconfig"
+source "drivers/dma/ptdma/Kconfig"
+
source "drivers/dma/qcom/Kconfig"
source "drivers/dma/dw/Kconfig"
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index aa69094..bf9ffc2 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_DMATEST) += dmatest.o
obj-$(CONFIG_ALTERA_MSGDMA) += altera-msgdma.o
obj-$(CONFIG_AMBA_PL08X) += amba-pl08x.o
obj-$(CONFIG_AMCC_PPC440SPE_ADMA) += ppc4xx/
+obj-$(CONFIG_AMD_PTDMA) += ptdma/
obj-$(CONFIG_AT_HDMAC) += at_hdmac.o
obj-$(CONFIG_AT_XDMAC) += at_xdmac.o
obj-$(CONFIG_AXI_DMAC) += dma-axi-dmac.o
diff --git a/drivers/dma/ptdma/Kconfig b/drivers/dma/ptdma/Kconfig
new file mode 100644
index 0000000..e6b8ca1
--- /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 to perform high bandwidth memory to
+ memory and IO copy operations. It performs DMA transfer through
+ queue-based descriptor management. This DMA controller is intended
+ to be used with AMD Non-Transparent Bridge devices and not for
+ general purpose peripheral DMA.
diff --git a/drivers/dma/ptdma/Makefile b/drivers/dma/ptdma/Makefile
new file mode 100644
index 0000000..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 0000000..4617550
--- /dev/null
+++ b/drivers/dma/ptdma/ptdma-dev.c
@@ -0,0 +1,290 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * AMD Passthru DMA device driver
+ * -- Based on the CCP driver
+ *
+ * Copyright (C) 2016,2021 Advanced Micro Devices, Inc.
+ *
+ * Author: Sanjay R Mehta <sanju.mehta@amd.com>
+ * Author: Gary R Hook <gary.hook@amd.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/dma-mapping.h>
+#include <linux/debugfs.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+
+#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 on the run bit */
+ iowrite32(cmd_q->qcontrol | CMD_Q_RUN, cmd_q->reg_control);
+}
+
+void pt_stop_queue(struct pt_cmd_queue *cmd_q)
+{
+ /* Turn off the run bit */
+ iowrite32(cmd_q->qcontrol & ~CMD_Q_RUN, cmd_q->reg_control);
+}
+
+static int pt_core_execute_cmd(struct ptdma_desc *desc, struct pt_cmd_queue *cmd_q)
+{
+ bool soc = FIELD_GET(DWORD0_SOC, desc->dw0);
+ u8 *q_desc = (u8 *)&cmd_q->qbase[cmd_q->qidx];
+ u8 *dp = (u8 *)desc;
+ u32 tail;
+
+ if (soc) {
+ desc->dw0 |= FIELD_PREP(DWORD0_IOC, desc->dw0);
+ desc->dw0 &= ~DWORD0_SOC;
+ }
+ mutex_lock(&cmd_q->q_mutex);
+
+ /* Copy 32-byte command descriptor to hw queue. */
+ memcpy(q_desc, dp, 32);
+ cmd_q->qidx = (cmd_q->qidx + 1) % CMD_Q_LEN;
+
+ /* The data used by this command must be flushed to memory */
+ wmb();
+
+ /* Write the new tail address back to the queue register */
+ tail = lower_32_bits(cmd_q->qdma_tail + cmd_q->qidx * Q_DESC_SIZE);
+ iowrite32(tail, cmd_q->reg_tail_lo);
+
+ /* Turn the queue back on using our cached control register */
+ pt_start_queue(cmd_q);
+ mutex_unlock(&cmd_q->q_mutex);
+
+ return 0;
+}
+
+int pt_core_perform_passthru(struct pt_cmd_queue *cmd_q,
+ struct pt_passthru_engine *pt_engine)
+{
+ struct ptdma_desc desc;
+
+ cmd_q->cmd_error = 0;
+ memset(&desc, 0, sizeof(desc));
+ desc.dw0 = CMD_DESC_DW0_VAL;
+ desc.length = pt_engine->src_len;
+ desc.src_lo = lower_32_bits(pt_engine->src_dma);
+ desc.dw3.src_hi = upper_32_bits(pt_engine->src_dma);
+ desc.dst_lo = lower_32_bits(pt_engine->dst_dma);
+ desc.dw5.dst_hi = upper_32_bits(pt_engine->dst_dma);
+
+ return pt_core_execute_cmd(&desc, cmd_q);
+}
+
+static inline void pt_core_disable_queue_interrupts(struct pt_device *pt)
+{
+ iowrite32(0, pt->cmd_q.reg_int_enable);
+}
+
+static inline void pt_core_enable_queue_interrupts(struct pt_device *pt)
+{
+ iowrite32(SUPPORTED_INTERRUPTS, pt->cmd_q.reg_int_enable);
+}
+
+static irqreturn_t pt_core_irq_handler(int irq, void *data)
+{
+ struct pt_device *pt = data;
+ struct pt_cmd_queue *cmd_q = &pt->cmd_q;
+ u32 status;
+
+ pt_core_disable_queue_interrupts(pt);
+
+ status = ioread32(cmd_q->reg_interrupt_status);
+ if (status) {
+ cmd_q->int_status = status;
+ cmd_q->q_status = ioread32(cmd_q->reg_status);
+ cmd_q->q_int_status = ioread32(cmd_q->reg_int_status);
+
+ /* On error, only save the first error value */
+ if ((status & INT_ERROR) && !cmd_q->cmd_error)
+ cmd_q->cmd_error = CMD_Q_ERROR(cmd_q->q_status);
+
+ /* Acknowledge the interrupt */
+ iowrite32(status, cmd_q->reg_interrupt_status);
+ }
+
+ pt_core_enable_queue_interrupts(pt);
+
+ return IRQ_HANDLED;
+}
+
+static void pt_init_cmdq_regs(struct pt_cmd_queue *cmd_q)
+{
+ void __iomem *io_regs = cmd_q->reg_control;
+
+ cmd_q->reg_tail_lo = io_regs + CMD_Q_TAIL_LO_BASE;
+ cmd_q->reg_head_lo = io_regs + CMD_Q_HEAD_LO_BASE;
+ cmd_q->reg_status = io_regs + CMD_Q_STATUS_BASE;
+ cmd_q->reg_int_enable = io_regs + CMD_Q_INT_ENABLE_BASE;
+ cmd_q->reg_int_status = io_regs + CMD_Q_INT_STATUS_BASE;
+ cmd_q->reg_dma_status = io_regs + CMD_Q_DMA_STATUS_BASE;
+ cmd_q->reg_dma_read_status = io_regs + CMD_Q_DMA_READ_STATUS_BASE;
+ cmd_q->reg_dma_write_status = io_regs + CMD_Q_DMA_WRITE_STATUS_BASE;
+ cmd_q->reg_interrupt_status = io_regs + CMD_Q_INTERRUPT_STATUS_BASE;
+}
+
+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");
+ 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_OFFSET);
+ iowrite32(CMD_CLK_GATE_CONFIG, pt->io_regs + CMD_CLK_GATE_CTL_OFFSET);
+ iowrite32(CMD_CONFIG_REQID, pt->io_regs + CMD_REQID_CONFIG_OFFSET);
+
+ cmd_q->pt = pt;
+ cmd_q->dma_pool = dma_pool;
+ mutex_init(&cmd_q->q_mutex);
+
+ /* Page alignment satisfies our needs for N <= 128 */
+ cmd_q->qsize = Q_SIZE(Q_DESC_SIZE);
+ cmd_q->qbase = dma_alloc_coherent(dev, cmd_q->qsize,
+ &cmd_q->qbase_dma,
+ GFP_KERNEL);
+ if (!cmd_q->qbase) {
+ dev_err(dev, "unable to allocate command queue\n");
+ ret = -ENOMEM;
+ goto e_dma_alloc;
+ }
+
+ cmd_q->qidx = 0;
+
+ /* Preset some register values */
+ cmd_q->reg_control = pt->io_regs + CMD_Q_STATUS_INCR;
+ pt_init_cmdq_regs(cmd_q);
+
+ /* Turn off the queues and disable interrupts until ready */
+ pt_core_disable_queue_interrupts(pt);
+
+ cmd_q->qcontrol = 0; /* Start with nothing */
+ iowrite32(cmd_q->qcontrol, cmd_q->reg_control);
+
+ ioread32(cmd_q->reg_int_status);
+ ioread32(cmd_q->reg_status);
+
+ /* Clear the interrupt status */
+ iowrite32(SUPPORTED_INTERRUPTS, cmd_q->reg_interrupt_status);
+
+ /* Request an irq */
+ ret = request_irq(pt->pt_irq, pt_core_irq_handler, 0, pt->name, pt);
+ if (ret)
+ goto e_pool;
+
+ /* Update the device registers with queue information. */
+ cmd_q->qcontrol &= ~CMD_Q_SIZE;
+ cmd_q->qcontrol |= FIELD_PREP(CMD_Q_SIZE, QUEUE_SIZE_VAL);
+
+ cmd_q->qdma_tail = cmd_q->qbase_dma;
+ dma_addr_lo = lower_32_bits(cmd_q->qdma_tail);
+ iowrite32((u32)dma_addr_lo, cmd_q->reg_tail_lo);
+ iowrite32((u32)dma_addr_lo, cmd_q->reg_head_lo);
+
+ dma_addr_hi = upper_32_bits(cmd_q->qdma_tail);
+ cmd_q->qcontrol |= (dma_addr_hi << 16);
+ iowrite32(cmd_q->qcontrol, cmd_q->reg_control);
+
+ pt_core_enable_queue_interrupts(pt);
+
+ return 0;
+
+e_dma_alloc:
+ dma_free_coherent(dev, cmd_q->qsize, cmd_q->qbase, cmd_q->qbase_dma);
+
+e_pool:
+ dev_err(dev, "unable to allocate an IRQ\n");
+ dma_pool_destroy(pt->cmd_q.dma_pool);
+
+ return ret;
+}
+
+void pt_core_destroy(struct pt_device *pt)
+{
+ struct device *dev = pt->dev;
+ struct pt_cmd_queue *cmd_q = &pt->cmd_q;
+ struct pt_cmd *cmd;
+
+ /* Disable and clear interrupts */
+ pt_core_disable_queue_interrupts(pt);
+
+ /* Turn off the run bit */
+ pt_stop_queue(cmd_q);
+
+ /* Clear the interrupt status */
+ iowrite32(SUPPORTED_INTERRUPTS, cmd_q->reg_interrupt_status);
+ ioread32(cmd_q->reg_int_status);
+ ioread32(cmd_q->reg_status);
+
+ free_irq(pt->pt_irq, pt);
+
+ dma_free_coherent(dev, cmd_q->qsize, cmd_q->qbase,
+ cmd_q->qbase_dma);
+
+ /* Flush the cmd queue */
+ while (!list_empty(&pt->cmd)) {
+ /* Invoke the callback directly with an error code */
+ cmd = list_first_entry(&pt->cmd, struct pt_cmd, entry);
+ list_del(&cmd->entry);
+ cmd->pt_cmd_callback(cmd->data, -ENODEV);
+ }
+}
diff --git a/drivers/dma/ptdma/ptdma-pci.c b/drivers/dma/ptdma/ptdma-pci.c
new file mode 100644
index 0000000..0945c27
--- /dev/null
+++ b/drivers/dma/ptdma/ptdma-pci.c
@@ -0,0 +1,251 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * AMD Passthru DMA device driver
+ * -- Based on the CCP driver
+ *
+ * Copyright (C) 2016,2021 Advanced Micro Devices, Inc.
+ *
+ * Author: Sanjay R Mehta <sanju.mehta@amd.com>
+ * Author: Tom Lendacky <thomas.lendacky@amd.com>
+ * Author: Gary R Hook <gary.hook@amd.com>
+ */
+
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/kthread.h>
+#include <linux/module.h>
+#include <linux/pci_ids.h>
+#include <linux/pci.h>
+#include <linux/spinlock.h>
+
+#include "ptdma.h"
+
+/* Ever-increasing value to produce unique unit numbers */
+static atomic_t pt_ordinal;
+
+struct pt_msix {
+ int msix_count;
+ struct msix_entry msix_entry;
+};
+
+/*
+ * pt_alloc_struct - allocate and initialize the pt_device struct
+ *
+ * @dev: device struct of the PTDMA
+ */
+static struct pt_device *pt_alloc_struct(struct device *dev)
+{
+ struct pt_device *pt;
+
+ pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
+
+ if (!pt)
+ return NULL;
+ pt->dev = dev;
+ pt->ord = atomic_inc_return(&pt_ordinal);
+
+ INIT_LIST_HEAD(&pt->cmd);
+
+ snprintf(pt->name, MAX_PT_NAME_LEN, "pt-%u", pt->ord);
+
+ return pt;
+}
+
+static int pt_get_msix_irqs(struct pt_device *pt)
+{
+ struct pt_msix *pt_msix = pt->pt_msix;
+ struct device *dev = pt->dev;
+ struct pci_dev *pdev = to_pci_dev(dev);
+ int ret;
+
+ pt_msix->msix_entry.entry = 0;
+
+ ret = pci_enable_msix_range(pdev, &pt_msix->msix_entry, 1, 1);
+ if (ret < 0)
+ return ret;
+
+ pt_msix->msix_count = ret;
+
+ pt->pt_irq = pt_msix->msix_entry.vector;
+
+ return 0;
+}
+
+static int pt_get_msi_irq(struct pt_device *pt)
+{
+ struct device *dev = pt->dev;
+ struct pci_dev *pdev = to_pci_dev(dev);
+ int ret;
+
+ ret = pci_enable_msi(pdev);
+ if (ret)
+ return ret;
+
+ pt->pt_irq = pdev->irq;
+
+ return 0;
+}
+
+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_err(dev, "could not enable MSI-X (%d), trying MSI\n", ret);
+ ret = pt_get_msi_irq(pt);
+ if (!ret)
+ return 0;
+
+ /* Couldn't get MSI interrupt */
+ dev_err(dev, "could not enable MSI (%d)\n", ret);
+
+ return ret;
+}
+
+static void pt_free_irqs(struct pt_device *pt)
+{
+ struct pt_msix *pt_msix = pt->pt_msix;
+ struct device *dev = pt->dev;
+ struct pci_dev *pdev = to_pci_dev(dev);
+
+ if (pt_msix->msix_count)
+ pci_disable_msix(pdev);
+ else if (pt->pt_irq)
+ pci_disable_msi(pdev);
+
+ pt->pt_irq = 0;
+}
+
+static int pt_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+ struct pt_device *pt;
+ struct pt_msix *pt_msix;
+ struct device *dev = &pdev->dev;
+ void __iomem * const *iomap_table;
+ int bar_mask;
+ int ret = -ENOMEM;
+
+ pt = pt_alloc_struct(dev);
+ if (!pt)
+ goto e_err;
+
+ pt_msix = devm_kzalloc(dev, sizeof(*pt_msix), GFP_KERNEL);
+ if (!pt_msix)
+ goto e_err;
+
+ pt->pt_msix = pt_msix;
+ pt->dev_vdata = (struct pt_dev_vdata *)id->driver_data;
+ if (!pt->dev_vdata) {
+ ret = -ENODEV;
+ dev_err(dev, "missing driver data\n");
+ goto e_err;
+ }
+
+ ret = pcim_enable_device(pdev);
+ if (ret) {
+ dev_err(dev, "pcim_enable_device failed (%d)\n", ret);
+ goto e_err;
+ }
+
+ bar_mask = pci_select_bars(pdev, IORESOURCE_MEM);
+ ret = pcim_iomap_regions(pdev, bar_mask, "ptdma");
+ if (ret) {
+ dev_err(dev, "pcim_iomap_regions failed (%d)\n", ret);
+ goto e_err;
+ }
+
+ iomap_table = pcim_iomap_table(pdev);
+ if (!iomap_table) {
+ dev_err(dev, "pcim_iomap_table failed\n");
+ ret = -ENOMEM;
+ goto e_err;
+ }
+
+ pt->io_regs = iomap_table[pt->dev_vdata->bar];
+ if (!pt->io_regs) {
+ dev_err(dev, "ioremap failed\n");
+ ret = -ENOMEM;
+ goto e_err;
+ }
+
+ ret = pt_get_irqs(pt);
+ if (ret)
+ goto e_err;
+
+ pci_set_master(pdev);
+
+ ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(48));
+ if (ret) {
+ ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
+ if (ret) {
+ dev_err(dev, "dma_set_mask_and_coherent failed (%d)\n",
+ ret);
+ goto e_err;
+ }
+ }
+
+ dev_set_drvdata(dev, pt);
+
+ if (pt->dev_vdata)
+ ret = pt_core_init(pt);
+
+ if (ret)
+ goto e_err;
+
+ dev_dbg(dev, "PTDMA enabled\n");
+
+ return 0;
+
+e_err:
+ dev_err(dev, "initialization failed\n");
+
+ return ret;
+}
+
+static void pt_pci_remove(struct pci_dev *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct pt_device *pt = dev_get_drvdata(dev);
+
+ if (!pt)
+ return;
+
+ if (pt->dev_vdata)
+ pt_core_destroy(pt);
+
+ pt_free_irqs(pt);
+}
+
+static const struct pt_dev_vdata dev_vdata[] = {
+ {
+ .bar = 2,
+ },
+};
+
+static const struct pci_device_id pt_pci_table[] = {
+ { PCI_VDEVICE(AMD, 0x1498), (kernel_ulong_t)&dev_vdata[0] },
+ /* Last entry must be zero */
+ { 0, }
+};
+MODULE_DEVICE_TABLE(pci, pt_pci_table);
+
+static struct pci_driver pt_pci_driver = {
+ .name = "ptdma",
+ .id_table = pt_pci_table,
+ .probe = pt_pci_probe,
+ .remove = pt_pci_remove,
+};
+
+module_pci_driver(pt_pci_driver);
+
+MODULE_AUTHOR("Sanjay R Mehta <sanju.mehta@amd.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("AMD PassThru DMA driver");
diff --git a/drivers/dma/ptdma/ptdma.h b/drivers/dma/ptdma/ptdma.h
new file mode 100644
index 0000000..a89a74ac
--- /dev/null
+++ b/drivers/dma/ptdma/ptdma.h
@@ -0,0 +1,305 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * AMD Passthru DMA device driver
+ * -- Based on the CCP driver
+ *
+ * Copyright (C) 2016,2021 Advanced Micro Devices, Inc.
+ *
+ * Author: Sanjay R Mehta <sanju.mehta@amd.com>
+ * Author: Tom Lendacky <thomas.lendacky@amd.com>
+ * Author: Gary R Hook <gary.hook@amd.com>
+ */
+
+#ifndef __PT_DEV_H__
+#define __PT_DEV_H__
+
+#include <linux/device.h>
+#include <linux/pci.h>
+#include <linux/spinlock.h>
+#include <linux/mutex.h>
+#include <linux/list.h>
+#include <linux/wait.h>
+#include <linux/dmapool.h>
+
+#define MAX_PT_NAME_LEN 16
+#define MAX_DMAPOOL_NAME_LEN 32
+
+#define MAX_HW_QUEUES 1
+#define MAX_CMD_QLEN 100
+
+#define PT_ENGINE_PASSTHRU 5
+#define PT_OFFSET 0x0
+
+#define PT_VSIZE 16
+#define PT_VMASK ((unsigned int)((1 << PT_VSIZE) - 1))
+
+/* Register Mappings */
+#define IRQ_MASK_REG 0x040
+#define IRQ_STATUS_REG 0x200
+
+#define CMD_Q_ERROR(__qs) ((__qs) & 0x0000003f)
+
+#define CMD_QUEUE_PRIO_OFFSET 0x00
+#define CMD_REQID_CONFIG_OFFSET 0x04
+#define CMD_TIMEOUT_OFFSET 0x08
+#define CMD_PT_VERSION 0x10
+
+#define CMD_Q_CONTROL_BASE 0x0000
+#define CMD_Q_TAIL_LO_BASE 0x0004
+#define CMD_Q_HEAD_LO_BASE 0x0008
+#define CMD_Q_INT_ENABLE_BASE 0x000C
+#define CMD_Q_INTERRUPT_STATUS_BASE 0x0010
+
+#define CMD_Q_STATUS_BASE 0x0100
+#define CMD_Q_INT_STATUS_BASE 0x0104
+#define CMD_Q_DMA_STATUS_BASE 0x0108
+#define CMD_Q_DMA_READ_STATUS_BASE 0x010C
+#define CMD_Q_DMA_WRITE_STATUS_BASE 0x0110
+#define CMD_Q_ABORT_BASE 0x0114
+#define CMD_Q_AX_CACHE_BASE 0x0118
+
+#define CMD_CONFIG_OFFSET 0x1120
+#define CMD_CLK_GATE_CTL_OFFSET 0x6004
+
+#define CMD_DESC_DW0_VAL 0x500012
+
+/* Address offset for virtual queue registers */
+#define CMD_Q_STATUS_INCR 0x1000
+
+/* Bit masks */
+#define CMD_CONFIG_REQID 0
+#define CMD_TIMEOUT_DISABLE 0
+#define CMD_CLK_DYN_GATING_DIS 0
+#define CMD_CLK_SW_GATE_MODE 0
+#define CMD_CLK_GATE_CTL 0
+#define CMD_QUEUE_PRIO GENMASK(2, 1)
+#define CMD_CONFIG_VHB_EN BIT(0)
+#define CMD_CLK_DYN_GATING_EN BIT(0)
+#define CMD_CLK_HW_GATE_MODE BIT(0)
+#define CMD_CLK_GATE_ON_DELAY BIT(12)
+#define CMD_CLK_GATE_OFF_DELAY BIT(12)
+
+#define CMD_CLK_GATE_CONFIG (CMD_CLK_GATE_CTL | \
+ CMD_CLK_HW_GATE_MODE | \
+ CMD_CLK_GATE_ON_DELAY | \
+ CMD_CLK_DYN_GATING_EN | \
+ CMD_CLK_GATE_OFF_DELAY)
+
+#define CMD_Q_LEN 32
+#define CMD_Q_RUN BIT(0)
+#define CMD_Q_HALT BIT(1)
+#define CMD_Q_MEM_LOCATION BIT(2)
+#define CMD_Q_SIZE_MASK GENMASK(4, 0)
+#define CMD_Q_SIZE GENMASK(7, 3)
+#define CMD_Q_SHIFT GENMASK(1, 0)
+#define QUEUE_SIZE_VAL ((ffs(CMD_Q_LEN) - 2) & \
+ CMD_Q_SIZE_MASK)
+#define Q_PTR_MASK (2 << (QUEUE_SIZE_VAL + 5) - 1)
+#define Q_DESC_SIZE sizeof(struct ptdma_desc)
+#define Q_SIZE(n) (CMD_Q_LEN * (n))
+
+#define INT_COMPLETION BIT(0)
+#define INT_ERROR BIT(1)
+#define INT_QUEUE_STOPPED BIT(2)
+#define INT_EMPTY_QUEUE BIT(3)
+#define SUPPORTED_INTERRUPTS (INT_COMPLETION | INT_ERROR)
+
+/****** Local Storage Block ******/
+#define LSB_START 0
+#define LSB_END 127
+#define LSB_COUNT (LSB_END - LSB_START + 1)
+
+#define PT_DMAPOOL_MAX_SIZE 64
+#define PT_DMAPOOL_ALIGN BIT(5)
+
+#define PT_PASSTHRU_BLOCKSIZE 512
+
+struct pt_device;
+
+struct pt_tasklet_data {
+ struct completion completion;
+ struct pt_cmd *cmd;
+};
+
+/*
+ * struct pt_passthru_engine - pass-through operation
+ * without performing DMA mapping
+ * @mask: mask to be applied to data
+ * @mask_len: length in bytes of mask
+ * @src_dma: data to be used for this operation
+ * @dst_dma: data produced by this operation
+ * @src_len: length in bytes of data used for this operation
+ *
+ * Variables required to be set when calling pt_enqueue_cmd():
+ * - bit_mod, byte_swap, src, dst, src_len
+ * - mask, mask_len if bit_mod is not PT_PASSTHRU_BITWISE_NOOP
+ */
+struct pt_passthru_engine {
+ dma_addr_t mask;
+ u32 mask_len; /* In bytes */
+
+ dma_addr_t src_dma, dst_dma;
+ u64 src_len; /* In bytes */
+};
+
+/*
+ * struct pt_cmd - PTDMA operation request
+ * @entry: list element
+ * @work: work element used for callbacks
+ * @pt: PT device to be run on
+ * @ret: operation return code
+ * @flags: cmd processing flags
+ * @engine: PTDMA operation to perform (passthru)
+ * @engine_error: PT engine return code
+ * @passthru: engine specific structures, refer to specific engine struct below
+ * @callback: operation completion callback function
+ * @data: parameter value to be supplied to the callback function
+ *
+ * Variables required to be set when calling pt_enqueue_cmd():
+ * - engine, callback
+ * - See the operation structures below for what is required for each
+ * operation.
+ */
+struct pt_cmd {
+ struct list_head entry;
+ struct work_struct work;
+ struct pt_device *pt;
+ int ret;
+ u32 engine;
+ u32 engine_error;
+ struct pt_passthru_engine passthru;
+ /* Completion callback support */
+ void (*pt_cmd_callback)(void *data, int err);
+ void *data;
+};
+
+struct pt_cmd_queue {
+ struct pt_device *pt;
+
+ /* Queue dma pool */
+ struct dma_pool *dma_pool;
+
+ /* Queue base address (not neccessarily aligned)*/
+ struct ptdma_desc *qbase;
+
+ /* Aligned queue start address (per requirement) */
+ struct mutex q_mutex ____cacheline_aligned;
+ unsigned int qidx;
+
+ unsigned int qsize;
+ dma_addr_t qbase_dma;
+ dma_addr_t qdma_tail;
+
+ unsigned int active;
+ unsigned int suspended;
+
+ /* Register addresses for queue */
+ void __iomem *reg_control;
+ void __iomem *reg_tail_lo;
+ void __iomem *reg_head_lo;
+ void __iomem *reg_int_enable;
+ void __iomem *reg_interrupt_status;
+ void __iomem *reg_status;
+ void __iomem *reg_int_status;
+ void __iomem *reg_dma_status;
+ void __iomem *reg_dma_read_status;
+ void __iomem *reg_dma_write_status;
+ u32 qcontrol; /* Cached control register */
+
+ /* Status values from job */
+ u32 int_status;
+ u32 q_status;
+ u32 q_int_status;
+ u32 cmd_error;
+} ____cacheline_aligned;
+
+struct pt_device {
+ struct list_head entry;
+
+ unsigned int ord;
+ char name[MAX_PT_NAME_LEN];
+
+ struct device *dev;
+
+ /* Bus specific device information */
+ struct pt_msix *pt_msix;
+
+ struct pt_dev_vdata *dev_vdata;
+
+ unsigned int pt_irq;
+
+ /* I/O area used for device communication */
+ void __iomem *io_regs;
+
+ spinlock_t cmd_lock ____cacheline_aligned;
+ unsigned int cmd_count;
+ struct list_head cmd;
+
+ /*
+ * The command queue. This represent the queue available on the
+ * PTDMA that are available for processing cmds
+ */
+ struct pt_cmd_queue cmd_q;
+
+ wait_queue_head_t lsb_queue;
+
+ struct pt_tasklet_data tdata;
+};
+
+/*
+ * descriptor for PTDMA commands
+ * 8 32-bit words:
+ * word 0: function; engine; control bits
+ * word 1: length of source data
+ * word 2: low 32 bits of source pointer
+ * word 3: upper 16 bits of source pointer; source memory type
+ * word 4: low 32 bits of destination pointer
+ * word 5: upper 16 bits of destination pointer; destination memory type
+ * word 6: reserved 32 bits
+ * word 7: reserved 32 bits
+ */
+
+#define DWORD0_SOC BIT(0)
+#define DWORD0_IOC BIT(1)
+
+struct dword3 {
+ unsigned int src_hi:16;
+ unsigned int src_mem:2;
+ unsigned int lsb_cxt_id:8;
+ unsigned int rsvd1:5;
+ unsigned int fixed:1;
+};
+
+struct dword5 {
+ unsigned int dst_hi:16;
+ unsigned int dst_mem:2;
+ unsigned int rsvd1:13;
+ unsigned int fixed:1;
+};
+
+struct ptdma_desc {
+ u32 dw0;
+ u32 length;
+ u32 src_lo;
+ struct dword3 dw3;
+ u32 dst_lo;
+ struct dword5 dw5;
+ __le32 rsvd1;
+ __le32 rsvd2;
+};
+
+/* Structure to hold PT device data */
+struct pt_dev_vdata {
+ const unsigned int bar;
+};
+
+int pt_core_init(struct pt_device *pt);
+void pt_core_destroy(struct pt_device *pt);
+
+int pt_core_perform_passthru(struct pt_cmd_queue *cmd_q,
+ struct pt_passthru_engine *pt_engine);
+
+void pt_start_queue(struct pt_cmd_queue *cmd_q);
+void pt_stop_queue(struct pt_cmd_queue *cmd_q);
+
+#endif
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v9 2/3] dmaengine: ptdma: register PTDMA controller as a DMA resource
2021-06-02 17:22 [PATCH v9 0/3] Add support for AMD PTDMA controller driver Sanjay R Mehta
2021-06-02 17:22 ` [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA Sanjay R Mehta
@ 2021-06-02 17:22 ` Sanjay R Mehta
2021-06-08 18:56 ` Vinod Koul
2021-06-02 17:22 ` [PATCH v9 3/3] dmaengine: ptdma: Add debugfs entries for PTDMA Sanjay R Mehta
2 siblings, 1 reply; 26+ messages in thread
From: Sanjay R Mehta @ 2021-06-02 17:22 UTC (permalink / raw)
To: vkoul
Cc: gregkh, dan.j.williams, Thomas.Lendacky, Shyam-sundar.S-k,
Nehal-bakulchandra.Shah, robh, mchehab+samsung, davem,
linux-kernel, dmaengine, Sanjay R Mehta
From: Sanjay R Mehta <sanju.mehta@amd.com>
Register ptdma queue to Linux dmaengine framework as general-purpose
DMA channels.
Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
---
drivers/dma/ptdma/Kconfig | 2 +
drivers/dma/ptdma/Makefile | 2 +-
drivers/dma/ptdma/ptdma-dev.c | 32 +++
drivers/dma/ptdma/ptdma-dmaengine.c | 460 ++++++++++++++++++++++++++++++++++++
drivers/dma/ptdma/ptdma.h | 31 +++
5 files changed, 526 insertions(+), 1 deletion(-)
create mode 100644 drivers/dma/ptdma/ptdma-dmaengine.c
diff --git a/drivers/dma/ptdma/Kconfig b/drivers/dma/ptdma/Kconfig
index e6b8ca1..b430edd 100644
--- a/drivers/dma/ptdma/Kconfig
+++ b/drivers/dma/ptdma/Kconfig
@@ -2,6 +2,8 @@
config AMD_PTDMA
tristate "AMD PassThru DMA Engine"
depends on X86_64 && PCI
+ select DMA_ENGINE
+ select DMA_VIRTUAL_CHANNELS
help
Enable support for the AMD PTDMA controller. This controller
provides DMA capabilities to perform high bandwidth memory to
diff --git a/drivers/dma/ptdma/Makefile b/drivers/dma/ptdma/Makefile
index 320fa82..a528cb0 100644
--- a/drivers/dma/ptdma/Makefile
+++ b/drivers/dma/ptdma/Makefile
@@ -5,6 +5,6 @@
obj-$(CONFIG_AMD_PTDMA) += ptdma.o
-ptdma-objs := ptdma-dev.o
+ptdma-objs := ptdma-dev.o ptdma-dmaengine.o
ptdma-$(CONFIG_PCI) += ptdma-pci.o
diff --git a/drivers/dma/ptdma/ptdma-dev.c b/drivers/dma/ptdma/ptdma-dev.c
index 4617550..7122933 100644
--- a/drivers/dma/ptdma/ptdma-dev.c
+++ b/drivers/dma/ptdma/ptdma-dev.c
@@ -124,6 +124,26 @@ static inline void pt_core_enable_queue_interrupts(struct pt_device *pt)
iowrite32(SUPPORTED_INTERRUPTS, pt->cmd_q.reg_int_enable);
}
+static void pt_do_cmd_complete(unsigned long data)
+{
+ struct pt_tasklet_data *tdata = (struct pt_tasklet_data *)data;
+ struct pt_cmd *cmd = tdata->cmd;
+ struct pt_cmd_queue *cmd_q = &cmd->pt->cmd_q;
+ u32 tail;
+
+ tail = lower_32_bits(cmd_q->qdma_tail + cmd_q->qidx * Q_DESC_SIZE);
+ if (cmd_q->cmd_error) {
+ /*
+ * Log the error and flush the queue by
+ * moving the head pointer
+ */
+ pt_log_error(cmd_q->pt, cmd_q->cmd_error);
+ iowrite32(tail, cmd_q->reg_head_lo);
+ }
+
+ cmd->pt_cmd_callback(cmd->data, cmd->ret);
+}
+
static irqreturn_t pt_core_irq_handler(int irq, void *data)
{
struct pt_device *pt = data;
@@ -147,6 +167,7 @@ static irqreturn_t pt_core_irq_handler(int irq, void *data)
}
pt_core_enable_queue_interrupts(pt);
+ pt_do_cmd_complete((ulong)&pt->tdata);
return IRQ_HANDLED;
}
@@ -246,8 +267,16 @@ int pt_core_init(struct pt_device *pt)
pt_core_enable_queue_interrupts(pt);
+ /* Register the DMA engine support */
+ ret = pt_dmaengine_register(pt);
+ if (ret)
+ goto e_dmaengine;
+
return 0;
+e_dmaengine:
+ free_irq(pt->pt_irq, pt);
+
e_dma_alloc:
dma_free_coherent(dev, cmd_q->qsize, cmd_q->qbase, cmd_q->qbase_dma);
@@ -264,6 +293,9 @@ void pt_core_destroy(struct pt_device *pt)
struct pt_cmd_queue *cmd_q = &pt->cmd_q;
struct pt_cmd *cmd;
+ /* Unregister the DMA engine */
+ pt_dmaengine_unregister(pt);
+
/* Disable and clear interrupts */
pt_core_disable_queue_interrupts(pt);
diff --git a/drivers/dma/ptdma/ptdma-dmaengine.c b/drivers/dma/ptdma/ptdma-dmaengine.c
new file mode 100644
index 0000000..c67a1ab
--- /dev/null
+++ b/drivers/dma/ptdma/ptdma-dmaengine.c
@@ -0,0 +1,460 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * AMD Passthrough DMA device driver
+ * -- Based on the CCP driver
+ *
+ * Copyright (C) 2016,2021 Advanced Micro Devices, Inc.
+ *
+ * Author: Sanjay R Mehta <sanju.mehta@amd.com>
+ * Author: Gary R Hook <gary.hook@amd.com>
+ */
+
+#include "ptdma.h"
+#include "../dmaengine.h"
+#include "../virt-dma.h"
+
+static inline struct pt_dma_chan *to_pt_chan(struct dma_chan *dma_chan)
+{
+ return container_of(dma_chan, struct pt_dma_chan, vc.chan);
+}
+
+static inline struct pt_dma_desc *to_pt_desc(struct virt_dma_desc *vd)
+{
+ return container_of(vd, struct pt_dma_desc, vd);
+}
+
+static void pt_free_cmd_resources(struct pt_device *pt,
+ struct list_head *list)
+{
+ struct pt_dma_cmd *cmd, *ctmp;
+
+ list_for_each_entry_safe(cmd, ctmp, list, entry) {
+ list_del(&cmd->entry);
+ kmem_cache_free(pt->dma_cmd_cache, cmd);
+ }
+}
+
+static void pt_free_chan_resources(struct dma_chan *dma_chan)
+{
+ struct pt_dma_chan *chan = to_pt_chan(dma_chan);
+
+ vchan_free_chan_resources(&chan->vc);
+}
+
+static void pt_synchronize(struct dma_chan *dma_chan)
+{
+ struct pt_dma_chan *chan = to_pt_chan(dma_chan);
+
+ vchan_synchronize(&chan->vc);
+}
+
+static void pt_do_cleanup(struct virt_dma_desc *vd)
+{
+ struct pt_dma_desc *desc = to_pt_desc(vd);
+ struct pt_device *pt = desc->pt;
+
+ pt_free_cmd_resources(pt, &desc->cmdlist);
+ kmem_cache_free(pt->dma_desc_cache, desc);
+}
+
+static int pt_issue_next_cmd(struct pt_dma_desc *desc)
+{
+ struct pt_passthru_engine *pt_engine;
+ struct pt_dma_cmd *cmd;
+ struct pt_device *pt;
+ struct pt_cmd *pt_cmd;
+ struct pt_cmd_queue *cmd_q;
+
+ cmd = list_first_entry(&desc->cmdlist, struct pt_dma_cmd, entry);
+ desc->issued_to_hw = 1;
+
+ pt_cmd = &cmd->pt_cmd;
+ pt = pt_cmd->pt;
+ cmd_q = &pt->cmd_q;
+ pt_engine = &pt_cmd->passthru;
+
+ pt->tdata.cmd = pt_cmd;
+
+ /* Execute the command */
+ pt_cmd->ret = pt_core_perform_passthru(cmd_q, pt_engine);
+
+ return 0;
+}
+
+static void pt_free_active_cmd(struct pt_dma_desc *desc)
+{
+ struct pt_dma_cmd *cmd = NULL;
+
+ if (desc->issued_to_hw)
+ cmd = list_first_entry_or_null(&desc->cmdlist, struct pt_dma_cmd,
+ entry);
+ if (!cmd)
+ return;
+
+ list_del(&cmd->entry);
+ kmem_cache_free(desc->pt->dma_cmd_cache, cmd);
+}
+
+static struct pt_dma_desc *pt_next_dma_desc(struct pt_dma_chan *chan)
+{
+ /* Get the next DMA descriptor on the active list */
+ struct virt_dma_desc *vd = vchan_next_desc(&chan->vc);
+
+ return vd ? to_pt_desc(vd) : NULL;
+}
+
+static struct pt_dma_desc *__pt_next_dma_desc(struct pt_dma_chan *chan)
+{
+ /* Get the next DMA descriptor on the active list */
+ struct virt_dma_desc *vd = vchan_next_desc(&chan->vc);
+
+ if (list_empty(&chan->vc.desc_submitted))
+ return NULL;
+
+ vd = list_empty(&chan->vc.desc_issued) ?
+ list_first_entry(&chan->vc.desc_submitted,
+ struct virt_dma_desc, node) : NULL;
+
+ vchan_issue_pending(&chan->vc);
+
+ return vd ? to_pt_desc(vd) : NULL;
+}
+
+static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
+ struct pt_dma_desc *desc)
+{
+ struct dma_async_tx_descriptor *tx_desc;
+ struct virt_dma_desc *vd;
+ unsigned long flags;
+
+ /* Loop over descriptors until one is found with commands */
+ do {
+ if (desc) {
+ /* Remove the DMA command from the list and free it */
+ pt_free_active_cmd(desc);
+ if (!desc->issued_to_hw) {
+ /* No errors, keep going */
+ if (desc->status != DMA_ERROR)
+ return desc;
+ /* Error, free remaining commands and move on */
+ pt_free_cmd_resources(desc->pt,
+ &desc->cmdlist);
+ }
+
+ tx_desc = &desc->vd.tx;
+ vd = &desc->vd;
+ } else {
+ tx_desc = NULL;
+ }
+
+ spin_lock_irqsave(&chan->vc.lock, flags);
+
+ if (desc) {
+ if (desc->status != DMA_ERROR)
+ desc->status = DMA_COMPLETE;
+
+ dma_cookie_complete(tx_desc);
+ dma_descriptor_unmap(tx_desc);
+ list_del(&desc->vd.node);
+ }
+
+ desc = pt_next_dma_desc(chan);
+
+ spin_unlock_irqrestore(&chan->vc.lock, flags);
+
+ if (tx_desc) {
+ dmaengine_desc_get_callback_invoke(tx_desc, NULL);
+ dma_run_dependencies(tx_desc);
+ vchan_vdesc_fini(vd);
+ }
+ } while (desc);
+
+ return NULL;
+}
+
+static void pt_cmd_callback(void *data, int err)
+{
+ struct pt_dma_desc *desc = data;
+ struct dma_chan *dma_chan;
+ struct pt_dma_chan *chan;
+ int ret;
+
+ if (err == -EINPROGRESS)
+ return;
+
+ dma_chan = desc->vd.tx.chan;
+ chan = to_pt_chan(dma_chan);
+
+ if (err)
+ desc->status = DMA_ERROR;
+
+ while (true) {
+ /* Check for DMA descriptor completion */
+ desc = pt_handle_active_desc(chan, desc);
+
+ /* Don't submit cmd if no descriptor or DMA is paused */
+ if (!desc)
+ break;
+
+ ret = pt_issue_next_cmd(desc);
+ if (!ret)
+ break;
+
+ desc->status = DMA_ERROR;
+ }
+}
+
+static struct pt_dma_cmd *pt_alloc_dma_cmd(struct pt_dma_chan *chan)
+{
+ struct pt_dma_cmd *cmd;
+
+ cmd = kmem_cache_zalloc(chan->pt->dma_cmd_cache, GFP_NOWAIT);
+
+ return cmd;
+}
+
+static struct pt_dma_desc *pt_alloc_dma_desc(struct pt_dma_chan *chan,
+ unsigned long flags)
+{
+ struct pt_dma_desc *desc;
+
+ desc = kmem_cache_zalloc(chan->pt->dma_desc_cache, GFP_NOWAIT);
+ if (!desc)
+ return NULL;
+
+ vchan_tx_prep(&chan->vc, &desc->vd, flags);
+
+ desc->pt = chan->pt;
+ desc->issued_to_hw = 0;
+ INIT_LIST_HEAD(&desc->cmdlist);
+ desc->status = DMA_IN_PROGRESS;
+
+ return desc;
+}
+
+static struct pt_dma_desc *pt_create_desc(struct dma_chan *dma_chan,
+ dma_addr_t dst,
+ dma_addr_t src,
+ unsigned int len,
+ unsigned long flags)
+{
+ struct pt_dma_chan *chan = to_pt_chan(dma_chan);
+ struct pt_passthru_engine *pt_engine;
+ struct pt_device *pt = chan->pt;
+ struct pt_dma_desc *desc;
+ struct pt_dma_cmd *cmd;
+ struct pt_cmd *pt_cmd;
+
+ desc = pt_alloc_dma_desc(chan, flags);
+ if (!desc)
+ return NULL;
+
+ cmd = pt_alloc_dma_cmd(chan);
+ if (!cmd)
+ goto err;
+
+ pt_cmd = &cmd->pt_cmd;
+ pt_cmd->pt = chan->pt;
+ pt_engine = &pt_cmd->passthru;
+ pt_cmd->engine = PT_ENGINE_PASSTHRU;
+ pt_engine->src_dma = src;
+ pt_engine->dst_dma = dst;
+ pt_engine->src_len = len;
+ pt_cmd->pt_cmd_callback = pt_cmd_callback;
+ pt_cmd->data = desc;
+
+ list_add_tail(&cmd->entry, &desc->cmdlist);
+
+ desc->len = len;
+
+ if (list_empty(&desc->cmdlist))
+ goto err;
+
+ return desc;
+
+err:
+ pt_free_cmd_resources(pt, &desc->cmdlist);
+ kmem_cache_free(pt->dma_desc_cache, desc);
+
+ return NULL;
+}
+
+static struct dma_async_tx_descriptor *
+pt_prep_dma_memcpy(struct dma_chan *dma_chan, dma_addr_t dst,
+ dma_addr_t src, size_t len, unsigned long flags)
+{
+ struct pt_dma_desc *desc;
+
+ desc = pt_create_desc(dma_chan, dst, src, len, flags);
+ if (!desc)
+ return NULL;
+
+ return &desc->vd.tx;
+}
+
+static struct dma_async_tx_descriptor *
+pt_prep_dma_interrupt(struct dma_chan *dma_chan, unsigned long flags)
+{
+ struct pt_dma_chan *chan = to_pt_chan(dma_chan);
+ struct pt_dma_desc *desc;
+
+ desc = pt_alloc_dma_desc(chan, flags);
+ if (!desc)
+ return NULL;
+
+ return &desc->vd.tx;
+}
+
+static void pt_issue_pending(struct dma_chan *dma_chan)
+{
+ struct pt_dma_chan *chan = to_pt_chan(dma_chan);
+ struct pt_dma_desc *desc;
+ unsigned long flags;
+
+ spin_lock_irqsave(&chan->vc.lock, flags);
+
+ desc = __pt_next_dma_desc(chan);
+
+ spin_unlock_irqrestore(&chan->vc.lock, flags);
+
+ /* If there was nothing active, start processing */
+ if (desc)
+ pt_cmd_callback(desc, 0);
+}
+
+static int pt_pause(struct dma_chan *dma_chan)
+{
+ struct pt_dma_chan *chan = to_pt_chan(dma_chan);
+ unsigned long flags;
+
+ spin_lock_irqsave(&chan->vc.lock, flags);
+ pt_stop_queue(&chan->pt->cmd_q);
+ spin_unlock_irqrestore(&chan->vc.lock, flags);
+
+ return 0;
+}
+
+static int pt_resume(struct dma_chan *dma_chan)
+{
+ struct pt_dma_chan *chan = to_pt_chan(dma_chan);
+ struct pt_dma_desc *desc = NULL;
+ unsigned long flags;
+
+ spin_lock_irqsave(&chan->vc.lock, flags);
+ pt_start_queue(&chan->pt->cmd_q);
+ desc = __pt_next_dma_desc(chan);
+ spin_unlock_irqrestore(&chan->vc.lock, flags);
+
+ /* If there was something active, re-start */
+ if (desc)
+ pt_cmd_callback(desc, 0);
+
+ return 0;
+}
+
+static int pt_terminate_all(struct dma_chan *dma_chan)
+{
+ struct pt_dma_chan *chan = to_pt_chan(dma_chan);
+
+ vchan_free_chan_resources(&chan->vc);
+
+ return 0;
+}
+
+int pt_dmaengine_register(struct pt_device *pt)
+{
+ struct pt_dma_chan *chan;
+ struct dma_device *dma_dev = &pt->dma_dev;
+ char *cmd_cache_name;
+ char *desc_cache_name;
+ int ret;
+
+ pt->pt_dma_chan = devm_kzalloc(pt->dev, sizeof(*pt->pt_dma_chan),
+ GFP_KERNEL);
+ if (!pt->pt_dma_chan)
+ return -ENOMEM;
+
+ cmd_cache_name = devm_kasprintf(pt->dev, GFP_KERNEL,
+ "%s-dmaengine-cmd-cache",
+ pt->name);
+ if (!cmd_cache_name)
+ return -ENOMEM;
+
+ pt->dma_cmd_cache = kmem_cache_create(cmd_cache_name,
+ sizeof(struct pt_dma_cmd),
+ sizeof(void *),
+ SLAB_HWCACHE_ALIGN, NULL);
+ if (!pt->dma_cmd_cache)
+ return -ENOMEM;
+
+ desc_cache_name = devm_kasprintf(pt->dev, GFP_KERNEL,
+ "%s-dmaengine-desc-cache",
+ pt->name);
+ if (!desc_cache_name) {
+ ret = -ENOMEM;
+ goto err_cache;
+ }
+
+ pt->dma_desc_cache = kmem_cache_create(desc_cache_name,
+ sizeof(struct pt_dma_desc),
+ sizeof(void *),
+ SLAB_HWCACHE_ALIGN, NULL);
+ if (!pt->dma_desc_cache) {
+ ret = -ENOMEM;
+ goto err_cache;
+ }
+
+ dma_dev->dev = pt->dev;
+ dma_dev->src_addr_widths = DMA_SLAVE_BUSWIDTH_64_BYTES;
+ dma_dev->dst_addr_widths = DMA_SLAVE_BUSWIDTH_64_BYTES;
+ dma_dev->directions = DMA_MEM_TO_MEM;
+ dma_dev->residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
+ dma_cap_set(DMA_MEMCPY, dma_dev->cap_mask);
+ dma_cap_set(DMA_INTERRUPT, dma_dev->cap_mask);
+ dma_cap_set(DMA_PRIVATE, dma_dev->cap_mask);
+
+ INIT_LIST_HEAD(&dma_dev->channels);
+
+ chan = pt->pt_dma_chan;
+ chan->pt = pt;
+
+ /* Set base and prep routines */
+ dma_dev->device_free_chan_resources = pt_free_chan_resources;
+ dma_dev->device_prep_dma_memcpy = pt_prep_dma_memcpy;
+ dma_dev->device_prep_dma_interrupt = pt_prep_dma_interrupt;
+ dma_dev->device_issue_pending = pt_issue_pending;
+ dma_dev->device_tx_status = dma_cookie_status;
+ dma_dev->device_pause = pt_pause;
+ dma_dev->device_resume = pt_resume;
+ dma_dev->device_terminate_all = pt_terminate_all;
+ dma_dev->device_synchronize = pt_synchronize;
+
+ chan->vc.desc_free = pt_do_cleanup;
+ vchan_init(&chan->vc, dma_dev);
+
+ dma_set_mask_and_coherent(pt->dev, DMA_BIT_MASK(64));
+
+ ret = dma_async_device_register(dma_dev);
+ if (ret)
+ goto err_reg;
+
+ return 0;
+
+err_reg:
+ kmem_cache_destroy(pt->dma_desc_cache);
+
+err_cache:
+ kmem_cache_destroy(pt->dma_cmd_cache);
+
+ return ret;
+}
+
+void pt_dmaengine_unregister(struct pt_device *pt)
+{
+ struct dma_device *dma_dev = &pt->dma_dev;
+
+ dma_async_device_unregister(dma_dev);
+
+ kmem_cache_destroy(pt->dma_desc_cache);
+ kmem_cache_destroy(pt->dma_cmd_cache);
+}
diff --git a/drivers/dma/ptdma/ptdma.h b/drivers/dma/ptdma/ptdma.h
index a89a74ac..bc6676d 100644
--- a/drivers/dma/ptdma/ptdma.h
+++ b/drivers/dma/ptdma/ptdma.h
@@ -14,6 +14,7 @@
#define __PT_DEV_H__
#include <linux/device.h>
+#include <linux/dmaengine.h>
#include <linux/pci.h>
#include <linux/spinlock.h>
#include <linux/mutex.h>
@@ -21,6 +22,8 @@
#include <linux/wait.h>
#include <linux/dmapool.h>
+#include "../virt-dma.h"
+
#define MAX_PT_NAME_LEN 16
#define MAX_DMAPOOL_NAME_LEN 32
@@ -173,6 +176,25 @@ struct pt_cmd {
void *data;
};
+struct pt_dma_cmd {
+ struct list_head entry;
+ struct pt_cmd pt_cmd;
+};
+
+struct pt_dma_desc {
+ struct virt_dma_desc vd;
+ struct pt_device *pt;
+ struct list_head cmdlist;
+ enum dma_status status;
+ size_t len;
+ bool issued_to_hw;
+};
+
+struct pt_dma_chan {
+ struct virt_dma_chan vc;
+ struct pt_device *pt;
+};
+
struct pt_cmd_queue {
struct pt_device *pt;
@@ -241,6 +263,12 @@ struct pt_device {
*/
struct pt_cmd_queue cmd_q;
+ /* Support for the DMA Engine capabilities */
+ struct dma_device dma_dev;
+ struct pt_dma_chan *pt_dma_chan;
+ struct kmem_cache *dma_cmd_cache;
+ struct kmem_cache *dma_desc_cache;
+
wait_queue_head_t lsb_queue;
struct pt_tasklet_data tdata;
@@ -293,6 +321,9 @@ struct pt_dev_vdata {
const unsigned int bar;
};
+int pt_dmaengine_register(struct pt_device *pt);
+void pt_dmaengine_unregister(struct pt_device *pt);
+
int pt_core_init(struct pt_device *pt);
void pt_core_destroy(struct pt_device *pt);
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v9 3/3] dmaengine: ptdma: Add debugfs entries for PTDMA
2021-06-02 17:22 [PATCH v9 0/3] Add support for AMD PTDMA controller driver Sanjay R Mehta
2021-06-02 17:22 ` [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA Sanjay R Mehta
2021-06-02 17:22 ` [PATCH v9 2/3] dmaengine: ptdma: register PTDMA controller as a DMA resource Sanjay R Mehta
@ 2021-06-02 17:22 ` Sanjay R Mehta
2021-06-09 14:10 ` Vinod Koul
2 siblings, 1 reply; 26+ messages in thread
From: Sanjay R Mehta @ 2021-06-02 17:22 UTC (permalink / raw)
To: vkoul
Cc: gregkh, dan.j.williams, Thomas.Lendacky, Shyam-sundar.S-k,
Nehal-bakulchandra.Shah, robh, mchehab+samsung, davem,
linux-kernel, dmaengine, Sanjay R Mehta
From: Sanjay R Mehta <sanju.mehta@amd.com>
Expose data about the configuration and operation of the
PTDMA through debugfs entries: device name, capabilities,
configuration, statistics.
Signed-off-by: Sanjay R Mehta <sanju.mehta@amd.com>
---
drivers/dma/ptdma/Makefile | 2 +-
drivers/dma/ptdma/ptdma-debugfs.c | 115 ++++++++++++++++++++++++++++++++++++++
drivers/dma/ptdma/ptdma-dev.c | 5 ++
drivers/dma/ptdma/ptdma.h | 6 ++
4 files changed, 127 insertions(+), 1 deletion(-)
create mode 100644 drivers/dma/ptdma/ptdma-debugfs.c
diff --git a/drivers/dma/ptdma/Makefile b/drivers/dma/ptdma/Makefile
index a528cb0..ce54102 100644
--- a/drivers/dma/ptdma/Makefile
+++ b/drivers/dma/ptdma/Makefile
@@ -5,6 +5,6 @@
obj-$(CONFIG_AMD_PTDMA) += ptdma.o
-ptdma-objs := ptdma-dev.o ptdma-dmaengine.o
+ptdma-objs := ptdma-dev.o ptdma-dmaengine.o ptdma-debugfs.o
ptdma-$(CONFIG_PCI) += ptdma-pci.o
diff --git a/drivers/dma/ptdma/ptdma-debugfs.c b/drivers/dma/ptdma/ptdma-debugfs.c
new file mode 100644
index 0000000..1f69159
--- /dev/null
+++ b/drivers/dma/ptdma/ptdma-debugfs.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * AMD Passthrough DMA device driver
+ * -- Based on the CCP driver
+ *
+ * Copyright (C) 2016,2021 Advanced Micro Devices, Inc.
+ *
+ * Author: Sanjay R Mehta <sanju.mehta@amd.com>
+ * Author: Gary R Hook <gary.hook@amd.com>
+ */
+
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+
+#include "ptdma.h"
+
+/* DebugFS helpers */
+#define MAX_NAME_LEN 20
+#define RI_VERSION_NUM 0x0000003F
+
+#define RI_NUM_VQM 0x00078000
+#define RI_NVQM_SHIFT 15
+
+static DEFINE_MUTEX(pt_debugfs_lock);
+
+static int pt_debugfs_info_show(struct seq_file *s, void *p)
+{
+ struct pt_device *pt = s->private;
+ unsigned int regval;
+
+ if (!pt)
+ return 0;
+
+ seq_printf(s, "Device name: %s\n", pt->name);
+ seq_printf(s, " # Queues: %d\n", 1);
+ seq_printf(s, " # Cmds: %d\n", pt->cmd_count);
+
+ regval = ioread32(pt->io_regs + CMD_PT_VERSION);
+
+ seq_printf(s, " Version: %d\n", regval & RI_VERSION_NUM);
+ seq_puts(s, " Engines:");
+ seq_puts(s, "\n");
+ seq_printf(s, " Queues: %d\n", (regval & RI_NUM_VQM) >> RI_NVQM_SHIFT);
+
+ return 0;
+}
+
+/*
+ * Return a formatted buffer containing the current
+ * statistics of queue for PTDMA
+ */
+static int pt_debugfs_stats_show(struct seq_file *s, void *p)
+{
+ struct pt_device *pt = s->private;
+
+ seq_printf(s, "Total Interrupts Handled: %ld\n", pt->total_interrupts);
+
+ return 0;
+}
+
+static int pt_debugfs_queue_show(struct seq_file *s, void *p)
+{
+ struct pt_cmd_queue *cmd_q = s->private;
+ unsigned int regval;
+
+ if (!cmd_q)
+ return 0;
+
+ seq_printf(s, " Pass-Thru: %ld\n", cmd_q->total_pt_ops);
+
+ regval = ioread32(cmd_q->reg_int_enable);
+
+ seq_puts(s, " Enabled Interrupts:");
+ if (regval & INT_EMPTY_QUEUE)
+ seq_puts(s, " EMPTY");
+ if (regval & INT_QUEUE_STOPPED)
+ seq_puts(s, " STOPPED");
+ if (regval & INT_ERROR)
+ seq_puts(s, " ERROR");
+ if (regval & INT_COMPLETION)
+ seq_puts(s, " COMPLETION");
+ seq_puts(s, "\n");
+
+ return 0;
+}
+
+DEFINE_SHOW_ATTRIBUTE(pt_debugfs_info);
+DEFINE_SHOW_ATTRIBUTE(pt_debugfs_queue);
+DEFINE_SHOW_ATTRIBUTE(pt_debugfs_stats);
+
+void ptdma_debugfs_setup(struct pt_device *pt)
+{
+ struct pt_cmd_queue *cmd_q;
+ char name[MAX_NAME_LEN + 1];
+ struct dentry *debugfs_q_instance;
+
+ if (!debugfs_initialized())
+ return;
+
+ debugfs_create_file("info", 0400, pt->dma_dev.dbg_dev_root, pt,
+ &pt_debugfs_info_fops);
+
+ debugfs_create_file("stats", 0600, pt->dma_dev.dbg_dev_root, pt,
+ &pt_debugfs_stats_fops);
+
+ cmd_q = &pt->cmd_q;
+
+ snprintf(name, MAX_NAME_LEN - 1, "q");
+
+ debugfs_q_instance =
+ debugfs_create_dir(name, pt->dma_dev.dbg_dev_root);
+
+ debugfs_create_file("stats", 0600, debugfs_q_instance, cmd_q,
+ &pt_debugfs_queue_fops);
+}
diff --git a/drivers/dma/ptdma/ptdma-dev.c b/drivers/dma/ptdma/ptdma-dev.c
index 7122933..ba37b81 100644
--- a/drivers/dma/ptdma/ptdma-dev.c
+++ b/drivers/dma/ptdma/ptdma-dev.c
@@ -103,6 +103,7 @@ int pt_core_perform_passthru(struct pt_cmd_queue *cmd_q,
struct ptdma_desc desc;
cmd_q->cmd_error = 0;
+ cmd_q->total_pt_ops++;
memset(&desc, 0, sizeof(desc));
desc.dw0 = CMD_DESC_DW0_VAL;
desc.length = pt_engine->src_len;
@@ -151,6 +152,7 @@ static irqreturn_t pt_core_irq_handler(int irq, void *data)
u32 status;
pt_core_disable_queue_interrupts(pt);
+ pt->total_interrupts++;
status = ioread32(cmd_q->reg_interrupt_status);
if (status) {
@@ -272,6 +274,9 @@ int pt_core_init(struct pt_device *pt)
if (ret)
goto e_dmaengine;
+ /* Set up debugfs entries */
+ ptdma_debugfs_setup(pt);
+
return 0;
e_dmaengine:
diff --git a/drivers/dma/ptdma/ptdma.h b/drivers/dma/ptdma/ptdma.h
index bc6676d..a02f47a 100644
--- a/drivers/dma/ptdma/ptdma.h
+++ b/drivers/dma/ptdma/ptdma.h
@@ -233,6 +233,8 @@ struct pt_cmd_queue {
u32 q_status;
u32 q_int_status;
u32 cmd_error;
+ /* Queue Statistics */
+ unsigned long total_pt_ops;
} ____cacheline_aligned;
struct pt_device {
@@ -271,6 +273,9 @@ struct pt_device {
wait_queue_head_t lsb_queue;
+ /* Device Statistics */
+ unsigned long total_interrupts;
+
struct pt_tasklet_data tdata;
};
@@ -324,6 +329,7 @@ struct pt_dev_vdata {
int pt_dmaengine_register(struct pt_device *pt);
void pt_dmaengine_unregister(struct pt_device *pt);
+void ptdma_debugfs_setup(struct pt_device *pt);
int pt_core_init(struct pt_device *pt);
void pt_core_destroy(struct pt_device *pt);
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA
2021-06-02 17:22 ` [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA Sanjay R Mehta
@ 2021-06-08 17:39 ` Vinod Koul
2021-06-15 11:20 ` Sanjay R Mehta
0 siblings, 1 reply; 26+ messages in thread
From: Vinod Koul @ 2021-06-08 17:39 UTC (permalink / raw)
To: Sanjay R Mehta
Cc: gregkh, dan.j.williams, Thomas.Lendacky, Shyam-sundar.S-k,
Nehal-bakulchandra.Shah, robh, mchehab+samsung, davem,
linux-kernel, dmaengine
On 02-06-21, 12:22, Sanjay R Mehta wrote:
> +static int pt_core_execute_cmd(struct ptdma_desc *desc, struct pt_cmd_queue *cmd_q)
> +{
> + bool soc = FIELD_GET(DWORD0_SOC, desc->dw0);
> + u8 *q_desc = (u8 *)&cmd_q->qbase[cmd_q->qidx];
> + u8 *dp = (u8 *)desc;
this case seems unnecessary?
> +int pt_core_perform_passthru(struct pt_cmd_queue *cmd_q,
> + struct pt_passthru_engine *pt_engine)
Pls align this to preceding open brace, checkpatch with --strict would
warn you about this
> +static irqreturn_t pt_core_irq_handler(int irq, void *data)
> +{
> + struct pt_device *pt = data;
> + struct pt_cmd_queue *cmd_q = &pt->cmd_q;
> + u32 status;
> +
> + pt_core_disable_queue_interrupts(pt);
> +
> + status = ioread32(cmd_q->reg_interrupt_status);
> + if (status) {
> + cmd_q->int_status = status;
> + cmd_q->q_status = ioread32(cmd_q->reg_status);
> + cmd_q->q_int_status = ioread32(cmd_q->reg_int_status);
> +
> + /* On error, only save the first error value */
> + if ((status & INT_ERROR) && !cmd_q->cmd_error)
> + cmd_q->cmd_error = CMD_Q_ERROR(cmd_q->q_status);
> +
> + /* Acknowledge the interrupt */
> + iowrite32(status, cmd_q->reg_interrupt_status);
> + }
> +
> + pt_core_enable_queue_interrupts(pt);
> +
> + return IRQ_HANDLED;
should you always return IRQ_HANDLED, that sounds apt for the if loop
but not for the non loop case
> +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");
This is superfluous, allocator would warn on failure
> +static struct pt_device *pt_alloc_struct(struct device *dev)
> +{
> + struct pt_device *pt;
> +
> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
> +
> + if (!pt)
> + return NULL;
> + pt->dev = dev;
> + pt->ord = atomic_inc_return(&pt_ordinal);
What is the use of this number?
> +static int pt_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> + struct pt_device *pt;
> + struct pt_msix *pt_msix;
> + struct device *dev = &pdev->dev;
> + void __iomem * const *iomap_table;
> + int bar_mask;
> + int ret = -ENOMEM;
> +
> + pt = pt_alloc_struct(dev);
> + if (!pt)
> + goto e_err;
> +
> + pt_msix = devm_kzalloc(dev, sizeof(*pt_msix), GFP_KERNEL);
> + if (!pt_msix)
> + goto e_err;
> +
> + pt->pt_msix = pt_msix;
> + pt->dev_vdata = (struct pt_dev_vdata *)id->driver_data;
> + if (!pt->dev_vdata) {
> + ret = -ENODEV;
> + dev_err(dev, "missing driver data\n");
> + goto e_err;
> + }
> +
> + ret = pcim_enable_device(pdev);
> + if (ret) {
> + dev_err(dev, "pcim_enable_device failed (%d)\n", ret);
> + goto e_err;
> + }
> +
> + bar_mask = pci_select_bars(pdev, IORESOURCE_MEM);
> + ret = pcim_iomap_regions(pdev, bar_mask, "ptdma");
> + if (ret) {
> + dev_err(dev, "pcim_iomap_regions failed (%d)\n", ret);
> + goto e_err;
> + }
> +
> + iomap_table = pcim_iomap_table(pdev);
> + if (!iomap_table) {
> + dev_err(dev, "pcim_iomap_table failed\n");
> + ret = -ENOMEM;
> + goto e_err;
> + }
> +
> + pt->io_regs = iomap_table[pt->dev_vdata->bar];
> + if (!pt->io_regs) {
> + dev_err(dev, "ioremap failed\n");
> + ret = -ENOMEM;
> + goto e_err;
> + }
> +
> + ret = pt_get_irqs(pt);
> + if (ret)
> + goto e_err;
> +
> + pci_set_master(pdev);
> +
> + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(48));
> + if (ret) {
> + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
> + if (ret) {
> + dev_err(dev, "dma_set_mask_and_coherent failed (%d)\n",
> + ret);
> + goto e_err;
> + }
> + }
> +
> + dev_set_drvdata(dev, pt);
> +
> + if (pt->dev_vdata)
> + ret = pt_core_init(pt);
> +
> + if (ret)
> + goto e_err;
> +
> + dev_dbg(dev, "PTDMA enabled\n");
pls remove these...
> +#include <linux/device.h>
> +#include <linux/pci.h>
> +#include <linux/spinlock.h>
> +#include <linux/mutex.h>
> +#include <linux/list.h>
> +#include <linux/wait.h>
> +#include <linux/dmapool.h>
> +
> +#define MAX_PT_NAME_LEN 16
> +#define MAX_DMAPOOL_NAME_LEN 32
> +
> +#define MAX_HW_QUEUES 1
> +#define MAX_CMD_QLEN 100
> +
> +#define PT_ENGINE_PASSTHRU 5
> +#define PT_OFFSET 0x0
> +
> +#define PT_VSIZE 16
> +#define PT_VMASK ((unsigned int)((1 << PT_VSIZE) - 1))
why cast?
--
~Vinod
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 2/3] dmaengine: ptdma: register PTDMA controller as a DMA resource
2021-06-02 17:22 ` [PATCH v9 2/3] dmaengine: ptdma: register PTDMA controller as a DMA resource Sanjay R Mehta
@ 2021-06-08 18:56 ` Vinod Koul
2021-06-15 11:34 ` Sanjay R Mehta
2021-06-20 3:52 ` Sanjay R Mehta
0 siblings, 2 replies; 26+ messages in thread
From: Vinod Koul @ 2021-06-08 18:56 UTC (permalink / raw)
To: Sanjay R Mehta
Cc: gregkh, dan.j.williams, Thomas.Lendacky, Shyam-sundar.S-k,
Nehal-bakulchandra.Shah, robh, mchehab+samsung, davem,
linux-kernel, dmaengine
On 02-06-21, 12:22, Sanjay R Mehta wrote:
> +static void pt_do_cmd_complete(unsigned long data)
> +{
> + struct pt_tasklet_data *tdata = (struct pt_tasklet_data *)data;
> + struct pt_cmd *cmd = tdata->cmd;
> + struct pt_cmd_queue *cmd_q = &cmd->pt->cmd_q;
> + u32 tail;
> +
> + tail = lower_32_bits(cmd_q->qdma_tail + cmd_q->qidx * Q_DESC_SIZE);
why extract tail in non error case?
> +static int pt_issue_next_cmd(struct pt_dma_desc *desc)
> +{
> + struct pt_passthru_engine *pt_engine;
> + struct pt_dma_cmd *cmd;
> + struct pt_device *pt;
> + struct pt_cmd *pt_cmd;
> + struct pt_cmd_queue *cmd_q;
> +
> + cmd = list_first_entry(&desc->cmdlist, struct pt_dma_cmd, entry);
> + desc->issued_to_hw = 1;
Why do you need this, the descriptor should be in vc.desc_issued list
> +static void pt_free_active_cmd(struct pt_dma_desc *desc)
> +{
> + struct pt_dma_cmd *cmd = NULL;
> +
> + if (desc->issued_to_hw)
> + cmd = list_first_entry_or_null(&desc->cmdlist, struct pt_dma_cmd,
> + entry);
single line pls and use lists provided..
> +static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
> + struct pt_dma_desc *desc)
> +{
> + struct dma_async_tx_descriptor *tx_desc;
> + struct virt_dma_desc *vd;
> + unsigned long flags;
> +
> + /* Loop over descriptors until one is found with commands */
> + do {
> + if (desc) {
> + /* Remove the DMA command from the list and free it */
> + pt_free_active_cmd(desc);
> + if (!desc->issued_to_hw) {
> + /* No errors, keep going */
> + if (desc->status != DMA_ERROR)
> + return desc;
> + /* Error, free remaining commands and move on */
> + pt_free_cmd_resources(desc->pt,
> + &desc->cmdlist);
single line again
> +static struct pt_dma_desc *pt_alloc_dma_desc(struct pt_dma_chan *chan,
> + unsigned long flags)
> +{
> + struct pt_dma_desc *desc;
> +
> + desc = kmem_cache_zalloc(chan->pt->dma_desc_cache, GFP_NOWAIT);
> + if (!desc)
> + return NULL;
> +
> + vchan_tx_prep(&chan->vc, &desc->vd, flags);
> +
> + desc->pt = chan->pt;
> + desc->issued_to_hw = 0;
> + INIT_LIST_HEAD(&desc->cmdlist);
why do you need your own list, the lists in vc should suffice?
> +static int pt_resume(struct dma_chan *dma_chan)
> +{
> + struct pt_dma_chan *chan = to_pt_chan(dma_chan);
> + struct pt_dma_desc *desc = NULL;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&chan->vc.lock, flags);
> + pt_start_queue(&chan->pt->cmd_q);
> + desc = __pt_next_dma_desc(chan);
> + spin_unlock_irqrestore(&chan->vc.lock, flags);
> +
> + /* If there was something active, re-start */
> + if (desc)
> + pt_cmd_callback(desc, 0);
this doesn't sound correct. In pause yoy stop the queue, so start of the
queue should be done here... Why grab a descriptor?
> +static int pt_terminate_all(struct dma_chan *dma_chan)
> +{
> + struct pt_dma_chan *chan = to_pt_chan(dma_chan);
> +
> + vchan_free_chan_resources(&chan->vc);
what about the descriptors, are you not going to clear the lists and
free them..
> +int pt_dmaengine_register(struct pt_device *pt)
> +{
> + struct pt_dma_chan *chan;
> + struct dma_device *dma_dev = &pt->dma_dev;
> + char *cmd_cache_name;
> + char *desc_cache_name;
> + int ret;
> +
> + pt->pt_dma_chan = devm_kzalloc(pt->dev, sizeof(*pt->pt_dma_chan),
> + GFP_KERNEL);
> + if (!pt->pt_dma_chan)
> + return -ENOMEM;
> +
> + cmd_cache_name = devm_kasprintf(pt->dev, GFP_KERNEL,
> + "%s-dmaengine-cmd-cache",
> + pt->name);
> + if (!cmd_cache_name)
> + return -ENOMEM;
> +
> + pt->dma_cmd_cache = kmem_cache_create(cmd_cache_name,
> + sizeof(struct pt_dma_cmd),
> + sizeof(void *),
> + SLAB_HWCACHE_ALIGN, NULL);
> + if (!pt->dma_cmd_cache)
> + return -ENOMEM;
> +
> + desc_cache_name = devm_kasprintf(pt->dev, GFP_KERNEL,
> + "%s-dmaengine-desc-cache",
> + pt->name);
> + if (!desc_cache_name) {
> + ret = -ENOMEM;
> + goto err_cache;
> + }
> +
> + pt->dma_desc_cache = kmem_cache_create(desc_cache_name,
> + sizeof(struct pt_dma_desc),
> + sizeof(void *),
sizeof void ptr?
> + SLAB_HWCACHE_ALIGN, NULL);
> + if (!pt->dma_desc_cache) {
> + ret = -ENOMEM;
> + goto err_cache;
> + }
> +
> + dma_dev->dev = pt->dev;
> + dma_dev->src_addr_widths = DMA_SLAVE_BUSWIDTH_64_BYTES;
> + dma_dev->dst_addr_widths = DMA_SLAVE_BUSWIDTH_64_BYTES;
> + dma_dev->directions = DMA_MEM_TO_MEM;
> + dma_dev->residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
> + dma_cap_set(DMA_MEMCPY, dma_dev->cap_mask);
> + dma_cap_set(DMA_INTERRUPT, dma_dev->cap_mask);
> + dma_cap_set(DMA_PRIVATE, dma_dev->cap_mask);
Why DMA_PRIVATE ? this is a dma mempcy controller ...
--
~Vinod
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 3/3] dmaengine: ptdma: Add debugfs entries for PTDMA
2021-06-02 17:22 ` [PATCH v9 3/3] dmaengine: ptdma: Add debugfs entries for PTDMA Sanjay R Mehta
@ 2021-06-09 14:10 ` Vinod Koul
2021-06-15 11:18 ` Sanjay R Mehta
0 siblings, 1 reply; 26+ messages in thread
From: Vinod Koul @ 2021-06-09 14:10 UTC (permalink / raw)
To: Sanjay R Mehta
Cc: gregkh, dan.j.williams, Thomas.Lendacky, Shyam-sundar.S-k,
Nehal-bakulchandra.Shah, robh, mchehab+samsung, davem,
linux-kernel, dmaengine
On 02-06-21, 12:22, Sanjay R Mehta wrote:
> +/* DebugFS helpers */
> +#define MAX_NAME_LEN 20
> +#define RI_VERSION_NUM 0x0000003F
> +
> +#define RI_NUM_VQM 0x00078000
> +#define RI_NVQM_SHIFT 15
> +
> +static DEFINE_MUTEX(pt_debugfs_lock);
unused?
> +
> +static int pt_debugfs_info_show(struct seq_file *s, void *p)
> +{
> + struct pt_device *pt = s->private;
> + unsigned int regval;
> +
> + if (!pt)
> + return 0;
better return an error code?
> +
> + seq_printf(s, "Device name: %s\n", pt->name);
> + seq_printf(s, " # Queues: %d\n", 1);
> + seq_printf(s, " # Cmds: %d\n", pt->cmd_count);
> +
> + regval = ioread32(pt->io_regs + CMD_PT_VERSION);
how do you ensure your device is not sleeping or you can access iomem
safely?
> +void ptdma_debugfs_setup(struct pt_device *pt)
> +{
> + struct pt_cmd_queue *cmd_q;
> + char name[MAX_NAME_LEN + 1];
> + struct dentry *debugfs_q_instance;
> +
> + if (!debugfs_initialized())
> + return;
> +
> + debugfs_create_file("info", 0400, pt->dma_dev.dbg_dev_root, pt,
> + &pt_debugfs_info_fops);
> +
> + debugfs_create_file("stats", 0600, pt->dma_dev.dbg_dev_root, pt,
why 600 here?
--
~Vinod
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 3/3] dmaengine: ptdma: Add debugfs entries for PTDMA
2021-06-09 14:10 ` Vinod Koul
@ 2021-06-15 11:18 ` Sanjay R Mehta
0 siblings, 0 replies; 26+ messages in thread
From: Sanjay R Mehta @ 2021-06-15 11:18 UTC (permalink / raw)
To: Vinod Koul, Sanjay R Mehta
Cc: gregkh, dan.j.williams, Thomas.Lendacky, Shyam-sundar.S-k,
Nehal-bakulchandra.Shah, robh, mchehab+samsung, davem,
linux-kernel, dmaengine
On 6/9/2021 7:40 PM, Vinod Koul wrote:
> [CAUTION: External Email]
>
> On 02-06-21, 12:22, Sanjay R Mehta wrote:
>
>> +/* DebugFS helpers */
>> +#define MAX_NAME_LEN 20
>> +#define RI_VERSION_NUM 0x0000003F
>> +
>> +#define RI_NUM_VQM 0x00078000
>> +#define RI_NVQM_SHIFT 15
>> +
>> +static DEFINE_MUTEX(pt_debugfs_lock);
>
> unused?
>
>> +
>> +static int pt_debugfs_info_show(struct seq_file *s, void *p)
>> +{
>> + struct pt_device *pt = s->private;
>> + unsigned int regval;
>> +
>> + if (!pt)
>> + return 0;
>
> better return an error code?
>
>> +
>> + seq_printf(s, "Device name: %s\n", pt->name);
>> + seq_printf(s, " # Queues: %d\n", 1);
>> + seq_printf(s, " # Cmds: %d\n", pt->cmd_count);
>> +
>> + regval = ioread32(pt->io_regs + CMD_PT_VERSION);
>
> how do you ensure your device is not sleeping or you can access iomem
> safely?
>
This device will never go to sleep state as this DMA device is part of AMD server SOC.
Hence PM support is not implemented.
- Sanjay
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA
2021-06-08 17:39 ` Vinod Koul
@ 2021-06-15 11:20 ` Sanjay R Mehta
2021-06-16 4:15 ` Vinod Koul
0 siblings, 1 reply; 26+ messages in thread
From: Sanjay R Mehta @ 2021-06-15 11:20 UTC (permalink / raw)
To: Vinod Koul, Sanjay R Mehta
Cc: gregkh, dan.j.williams, Thomas.Lendacky, Shyam-sundar.S-k,
Nehal-bakulchandra.Shah, robh, mchehab+samsung, davem,
linux-kernel, dmaengine
On 6/8/2021 11:09 PM, Vinod Koul wrote:
> [CAUTION: External Email]
>
> On 02-06-21, 12:22, Sanjay R Mehta wrote:
>
>> +static int pt_core_execute_cmd(struct ptdma_desc *desc, struct pt_cmd_queue *cmd_q)
>> +{
>> + bool soc = FIELD_GET(DWORD0_SOC, desc->dw0);
>> + u8 *q_desc = (u8 *)&cmd_q->qbase[cmd_q->qidx];
>> + u8 *dp = (u8 *)desc;
>
> this case seems unnecessary?
>
>> +int pt_core_perform_passthru(struct pt_cmd_queue *cmd_q,
>> + struct pt_passthru_engine *pt_engine)
>
> Pls align this to preceding open brace, checkpatch with --strict would
> warn you about this
>
>> +static irqreturn_t pt_core_irq_handler(int irq, void *data)
>> +{
>> + struct pt_device *pt = data;
>> + struct pt_cmd_queue *cmd_q = &pt->cmd_q;
>> + u32 status;
>> +
>> + pt_core_disable_queue_interrupts(pt);
>> +
>> + status = ioread32(cmd_q->reg_interrupt_status);
>> + if (status) {
>> + cmd_q->int_status = status;
>> + cmd_q->q_status = ioread32(cmd_q->reg_status);
>> + cmd_q->q_int_status = ioread32(cmd_q->reg_int_status);
>> +
>> + /* On error, only save the first error value */
>> + if ((status & INT_ERROR) && !cmd_q->cmd_error)
>> + cmd_q->cmd_error = CMD_Q_ERROR(cmd_q->q_status);
>> +
>> + /* Acknowledge the interrupt */
>> + iowrite32(status, cmd_q->reg_interrupt_status);
>> + }
>> +
>> + pt_core_enable_queue_interrupts(pt);
>> +
>> + return IRQ_HANDLED;
>
> should you always return IRQ_HANDLED, that sounds apt for the if loop
> but not for the non loop case
>
>> +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");
>
> This is superfluous, allocator would warn on failure
>
>> +static struct pt_device *pt_alloc_struct(struct device *dev)
>> +{
>> + struct pt_device *pt;
>> +
>> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
>> +
>> + if (!pt)
>> + return NULL;
>> + pt->dev = dev;
>> + pt->ord = atomic_inc_return(&pt_ordinal);
>
> What is the use of this number?
>
There are eight similar instances of this DMA engine on AMD SOC.
It is to differentiate each of these instances.
- Sanjay
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 2/3] dmaengine: ptdma: register PTDMA controller as a DMA resource
2021-06-08 18:56 ` Vinod Koul
@ 2021-06-15 11:34 ` Sanjay R Mehta
2021-06-16 4:18 ` Vinod Koul
2021-06-20 3:52 ` Sanjay R Mehta
1 sibling, 1 reply; 26+ messages in thread
From: Sanjay R Mehta @ 2021-06-15 11:34 UTC (permalink / raw)
To: Vinod Koul, Sanjay R Mehta
Cc: gregkh, dan.j.williams, Thomas.Lendacky, Shyam-sundar.S-k,
Nehal-bakulchandra.Shah, robh, mchehab+samsung, davem,
linux-kernel, dmaengine
On 6/9/2021 12:26 AM, Vinod Koul wrote:
[snipped]
>> +static struct pt_dma_desc *pt_alloc_dma_desc(struct pt_dma_chan *chan,
>> + unsigned long flags)
>> +{
>> + struct pt_dma_desc *desc;
>> +
>> + desc = kmem_cache_zalloc(chan->pt->dma_desc_cache, GFP_NOWAIT);
>> + if (!desc)
>> + return NULL;
>> +
>> + vchan_tx_prep(&chan->vc, &desc->vd, flags);
>> +
>> + desc->pt = chan->pt;
>> + desc->issued_to_hw = 0;
>> + INIT_LIST_HEAD(&desc->cmdlist);
>
> why do you need your own list, the lists in vc should suffice?
>
Do you think this should be a major blocker for pulling this series in 5.14?
Would you be okay to accept this change in the subsequent driver updates?
>> +static int pt_resume(struct dma_chan *dma_chan)
>> +{
>> + struct pt_dma_chan *chan = to_pt_chan(dma_chan);
>> + struct pt_dma_desc *desc = NULL;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&chan->vc.lock, flags);
>> + pt_start_queue(&chan->pt->cmd_q);
>> + desc = __pt_next_dma_desc(chan);
>> + spin_unlock_irqrestore(&chan->vc.lock, flags);
>> +
>> + /* If there was something active, re-start */
>> + if (desc)
>> + pt_cmd_callback(desc, 0);
>
> this doesn't sound correct. In pause yoy stop the queue, so start of the
> queue should be done here... Why grab a descriptor?
>
>> +static int pt_terminate_all(struct dma_chan *dma_chan)
>> +{
>> + struct pt_dma_chan *chan = to_pt_chan(dma_chan);
>> +
>> + vchan_free_chan_resources(&chan->vc);
>
> what about the descriptors, are you not going to clear the lists and
> free them..
>
>> +int pt_dmaengine_register(struct pt_device *pt)
>> +{
>> + struct pt_dma_chan *chan;
>> + struct dma_device *dma_dev = &pt->dma_dev;
>> + char *cmd_cache_name;
>> + char *desc_cache_name;
>> + int ret;
>> +
>> + pt->pt_dma_chan = devm_kzalloc(pt->dev, sizeof(*pt->pt_dma_chan),
>> + GFP_KERNEL);
>> + if (!pt->pt_dma_chan)
>> + return -ENOMEM;
>> +
>> + cmd_cache_name = devm_kasprintf(pt->dev, GFP_KERNEL,
>> + "%s-dmaengine-cmd-cache",
>> + pt->name);
>> + if (!cmd_cache_name)
>> + return -ENOMEM;
>> +
>> + pt->dma_cmd_cache = kmem_cache_create(cmd_cache_name,
>> + sizeof(struct pt_dma_cmd),
>> + sizeof(void *),
>> + SLAB_HWCACHE_ALIGN, NULL);
>> + if (!pt->dma_cmd_cache)
>> + return -ENOMEM;
>> +
>> + desc_cache_name = devm_kasprintf(pt->dev, GFP_KERNEL,
>> + "%s-dmaengine-desc-cache",
>> + pt->name);
>> + if (!desc_cache_name) {
>> + ret = -ENOMEM;
>> + goto err_cache;
>> + }
>> +
>> + pt->dma_desc_cache = kmem_cache_create(desc_cache_name,
>> + sizeof(struct pt_dma_desc),
>> + sizeof(void *),
>
> sizeof void ptr?
>
>> + SLAB_HWCACHE_ALIGN, NULL);
>> + if (!pt->dma_desc_cache) {
>> + ret = -ENOMEM;
>> + goto err_cache;
>> + }
>> +
>> + dma_dev->dev = pt->dev;
>> + dma_dev->src_addr_widths = DMA_SLAVE_BUSWIDTH_64_BYTES;
>> + dma_dev->dst_addr_widths = DMA_SLAVE_BUSWIDTH_64_BYTES;
>> + dma_dev->directions = DMA_MEM_TO_MEM;
>> + dma_dev->residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
>> + dma_cap_set(DMA_MEMCPY, dma_dev->cap_mask);
>> + dma_cap_set(DMA_INTERRUPT, dma_dev->cap_mask);
>> + dma_cap_set(DMA_PRIVATE, dma_dev->cap_mask);
>
> Why DMA_PRIVATE ? this is a dma mempcy controller ...
This DMA controller is intended to be used with AMD Non-Transparent
Bridge devices and not for general purpose peripheral DMA. Hence marking
it as DMA_PRIVATE.
- Sanjay
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA
2021-06-15 11:20 ` Sanjay R Mehta
@ 2021-06-16 4:15 ` Vinod Koul
2021-06-16 4:54 ` Sanjay R Mehta
0 siblings, 1 reply; 26+ messages in thread
From: Vinod Koul @ 2021-06-16 4:15 UTC (permalink / raw)
To: Sanjay R Mehta
Cc: Sanjay R Mehta, gregkh, dan.j.williams, Thomas.Lendacky,
Shyam-sundar.S-k, Nehal-bakulchandra.Shah, robh, mchehab+samsung,
davem, linux-kernel, dmaengine
On 15-06-21, 16:50, Sanjay R Mehta wrote:
> >> +static struct pt_device *pt_alloc_struct(struct device *dev)
> >> +{
> >> + struct pt_device *pt;
> >> +
> >> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
> >> +
> >> + if (!pt)
> >> + return NULL;
> >> + pt->dev = dev;
> >> + pt->ord = atomic_inc_return(&pt_ordinal);
> >
> > What is the use of this number?
> >
>
> There are eight similar instances of this DMA engine on AMD SOC.
> It is to differentiate each of these instances.
Are they individual device objects?
--
~Vinod
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 2/3] dmaengine: ptdma: register PTDMA controller as a DMA resource
2021-06-15 11:34 ` Sanjay R Mehta
@ 2021-06-16 4:18 ` Vinod Koul
2021-06-16 5:23 ` Sanjay R Mehta
0 siblings, 1 reply; 26+ messages in thread
From: Vinod Koul @ 2021-06-16 4:18 UTC (permalink / raw)
To: Sanjay R Mehta
Cc: Sanjay R Mehta, gregkh, dan.j.williams, Thomas.Lendacky,
Shyam-sundar.S-k, Nehal-bakulchandra.Shah, robh, mchehab+samsung,
davem, linux-kernel, dmaengine
On 15-06-21, 17:04, Sanjay R Mehta wrote:
>
>
> On 6/9/2021 12:26 AM, Vinod Koul wrote:
>
> [snipped]
>
> >> +static struct pt_dma_desc *pt_alloc_dma_desc(struct pt_dma_chan *chan,
> >> + unsigned long flags)
> >> +{
> >> + struct pt_dma_desc *desc;
> >> +
> >> + desc = kmem_cache_zalloc(chan->pt->dma_desc_cache, GFP_NOWAIT);
> >> + if (!desc)
> >> + return NULL;
> >> +
> >> + vchan_tx_prep(&chan->vc, &desc->vd, flags);
> >> +
> >> + desc->pt = chan->pt;
> >> + desc->issued_to_hw = 0;
> >> + INIT_LIST_HEAD(&desc->cmdlist);
> >
> > why do you need your own list, the lists in vc should suffice?
> >
>
> Do you think this should be a major blocker for pulling this series in 5.14?
> Would you be okay to accept this change in the subsequent driver updates?
Sorry that is not how upstream works, I would like things to be better
before we merge this
>
> >> +static int pt_resume(struct dma_chan *dma_chan)
> >> +{
> >> + struct pt_dma_chan *chan = to_pt_chan(dma_chan);
> >> + struct pt_dma_desc *desc = NULL;
> >> + unsigned long flags;
> >> +
> >> + spin_lock_irqsave(&chan->vc.lock, flags);
> >> + pt_start_queue(&chan->pt->cmd_q);
> >> + desc = __pt_next_dma_desc(chan);
> >> + spin_unlock_irqrestore(&chan->vc.lock, flags);
> >> +
> >> + /* If there was something active, re-start */
> >> + if (desc)
> >> + pt_cmd_callback(desc, 0);
> >
> > this doesn't sound correct. In pause yoy stop the queue, so start of the
> > queue should be done here... Why grab a descriptor?
> >
> >> +static int pt_terminate_all(struct dma_chan *dma_chan)
> >> +{
> >> + struct pt_dma_chan *chan = to_pt_chan(dma_chan);
> >> +
> >> + vchan_free_chan_resources(&chan->vc);
> >
> > what about the descriptors, are you not going to clear the lists and
> > free them..
> >
> >> +int pt_dmaengine_register(struct pt_device *pt)
> >> +{
> >> + struct pt_dma_chan *chan;
> >> + struct dma_device *dma_dev = &pt->dma_dev;
> >> + char *cmd_cache_name;
> >> + char *desc_cache_name;
> >> + int ret;
> >> +
> >> + pt->pt_dma_chan = devm_kzalloc(pt->dev, sizeof(*pt->pt_dma_chan),
> >> + GFP_KERNEL);
> >> + if (!pt->pt_dma_chan)
> >> + return -ENOMEM;
> >> +
> >> + cmd_cache_name = devm_kasprintf(pt->dev, GFP_KERNEL,
> >> + "%s-dmaengine-cmd-cache",
> >> + pt->name);
> >> + if (!cmd_cache_name)
> >> + return -ENOMEM;
> >> +
> >> + pt->dma_cmd_cache = kmem_cache_create(cmd_cache_name,
> >> + sizeof(struct pt_dma_cmd),
> >> + sizeof(void *),
> >> + SLAB_HWCACHE_ALIGN, NULL);
> >> + if (!pt->dma_cmd_cache)
> >> + return -ENOMEM;
> >> +
> >> + desc_cache_name = devm_kasprintf(pt->dev, GFP_KERNEL,
> >> + "%s-dmaengine-desc-cache",
> >> + pt->name);
> >> + if (!desc_cache_name) {
> >> + ret = -ENOMEM;
> >> + goto err_cache;
> >> + }
> >> +
> >> + pt->dma_desc_cache = kmem_cache_create(desc_cache_name,
> >> + sizeof(struct pt_dma_desc),
> >> + sizeof(void *),
> >
> > sizeof void ptr?
This and many more comments are left not replied, do you agree to them,
do you disagree, hard to tell from silence..
> >
> >> + SLAB_HWCACHE_ALIGN, NULL);
> >> + if (!pt->dma_desc_cache) {
> >> + ret = -ENOMEM;
> >> + goto err_cache;
> >> + }
> >> +
> >> + dma_dev->dev = pt->dev;
> >> + dma_dev->src_addr_widths = DMA_SLAVE_BUSWIDTH_64_BYTES;
> >> + dma_dev->dst_addr_widths = DMA_SLAVE_BUSWIDTH_64_BYTES;
> >> + dma_dev->directions = DMA_MEM_TO_MEM;
> >> + dma_dev->residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
> >> + dma_cap_set(DMA_MEMCPY, dma_dev->cap_mask);
> >> + dma_cap_set(DMA_INTERRUPT, dma_dev->cap_mask);
> >> + dma_cap_set(DMA_PRIVATE, dma_dev->cap_mask);
> >
> > Why DMA_PRIVATE ? this is a dma mempcy controller ...
>
> This DMA controller is intended to be used with AMD Non-Transparent
> Bridge devices and not for general purpose peripheral DMA. Hence marking
> it as DMA_PRIVATE.
Okay, maybe add a comment so that people would know
--
~Vinod
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA
2021-06-16 4:15 ` Vinod Koul
@ 2021-06-16 4:54 ` Sanjay R Mehta
2021-06-16 6:16 ` Greg KH
0 siblings, 1 reply; 26+ messages in thread
From: Sanjay R Mehta @ 2021-06-16 4:54 UTC (permalink / raw)
To: Vinod Koul
Cc: Sanjay R Mehta, gregkh, dan.j.williams, Thomas.Lendacky,
Shyam-sundar.S-k, Nehal-bakulchandra.Shah, robh, mchehab+samsung,
davem, linux-kernel, dmaengine
On 6/16/2021 9:45 AM, Vinod Koul wrote:
> [CAUTION: External Email]
>
> On 15-06-21, 16:50, Sanjay R Mehta wrote:
>
>>>> +static struct pt_device *pt_alloc_struct(struct device *dev)
>>>> +{
>>>> + struct pt_device *pt;
>>>> +
>>>> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
>>>> +
>>>> + if (!pt)
>>>> + return NULL;
>>>> + pt->dev = dev;
>>>> + pt->ord = atomic_inc_return(&pt_ordinal);
>>>
>>> What is the use of this number?
>>>
>>
>> There are eight similar instances of this DMA engine on AMD SOC.
>> It is to differentiate each of these instances.
>
> Are they individual device objects?
>
Yes, they are individual device objects.
> --
> ~Vinod
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 2/3] dmaengine: ptdma: register PTDMA controller as a DMA resource
2021-06-16 4:18 ` Vinod Koul
@ 2021-06-16 5:23 ` Sanjay R Mehta
0 siblings, 0 replies; 26+ messages in thread
From: Sanjay R Mehta @ 2021-06-16 5:23 UTC (permalink / raw)
To: Vinod Koul
Cc: Sanjay R Mehta, gregkh, dan.j.williams, Thomas.Lendacky,
Shyam-sundar.S-k, Nehal-bakulchandra.Shah, robh, mchehab+samsung,
davem, linux-kernel, dmaengine
On 6/16/2021 9:48 AM, Vinod Koul wrote:
> [CAUTION: External Email]
>
> On 15-06-21, 17:04, Sanjay R Mehta wrote:
>>
>>
>> On 6/9/2021 12:26 AM, Vinod Koul wrote:
>>
>> [snipped]
>>
>>>> +static struct pt_dma_desc *pt_alloc_dma_desc(struct pt_dma_chan *chan,
>>>> + unsigned long flags)
>>>> +{
>>>> + struct pt_dma_desc *desc;
>>>> +
>>>> + desc = kmem_cache_zalloc(chan->pt->dma_desc_cache, GFP_NOWAIT);
>>>> + if (!desc)
>>>> + return NULL;
>>>> +
>>>> + vchan_tx_prep(&chan->vc, &desc->vd, flags);
>>>> +
>>>> + desc->pt = chan->pt;
>>>> + desc->issued_to_hw = 0;
>>>> + INIT_LIST_HEAD(&desc->cmdlist);
>>>
>>> why do you need your own list, the lists in vc should suffice?
>>>
>>
>> Do you think this should be a major blocker for pulling this series in 5.14?
>> Would you be okay to accept this change in the subsequent driver updates?
>
> Sorry that is not how upstream works, I would like things to be better
> before we merge this
>
Sure Vinod, I will fix this and send the change in next version.
>>
>>>> +static int pt_resume(struct dma_chan *dma_chan)
>>>> +{
>>>> + struct pt_dma_chan *chan = to_pt_chan(dma_chan);
>>>> + struct pt_dma_desc *desc = NULL;
>>>> + unsigned long flags;
>>>> +
>>>> + spin_lock_irqsave(&chan->vc.lock, flags);
>>>> + pt_start_queue(&chan->pt->cmd_q);
>>>> + desc = __pt_next_dma_desc(chan);
>>>> + spin_unlock_irqrestore(&chan->vc.lock, flags);
>>>> +
>>>> + /* If there was something active, re-start */
>>>> + if (desc)
>>>> + pt_cmd_callback(desc, 0);
>>>
>>> this doesn't sound correct. In pause yoy stop the queue, so start of the
>>> queue should be done here... Why grab a descriptor?
>>>
>>>> +static int pt_terminate_all(struct dma_chan *dma_chan)
>>>> +{
>>>> + struct pt_dma_chan *chan = to_pt_chan(dma_chan);
>>>> +
>>>> + vchan_free_chan_resources(&chan->vc);
>>>
>>> what about the descriptors, are you not going to clear the lists and
>>> free them..
>>>
>>>> +int pt_dmaengine_register(struct pt_device *pt)
>>>> +{
>>>> + struct pt_dma_chan *chan;
>>>> + struct dma_device *dma_dev = &pt->dma_dev;
>>>> + char *cmd_cache_name;
>>>> + char *desc_cache_name;
>>>> + int ret;
>>>> +
>>>> + pt->pt_dma_chan = devm_kzalloc(pt->dev, sizeof(*pt->pt_dma_chan),
>>>> + GFP_KERNEL);
>>>> + if (!pt->pt_dma_chan)
>>>> + return -ENOMEM;
>>>> +
>>>> + cmd_cache_name = devm_kasprintf(pt->dev, GFP_KERNEL,
>>>> + "%s-dmaengine-cmd-cache",
>>>> + pt->name);
>>>> + if (!cmd_cache_name)
>>>> + return -ENOMEM;
>>>> +
>>>> + pt->dma_cmd_cache = kmem_cache_create(cmd_cache_name,
>>>> + sizeof(struct pt_dma_cmd),
>>>> + sizeof(void *),
>>>> + SLAB_HWCACHE_ALIGN, NULL);
>>>> + if (!pt->dma_cmd_cache)
>>>> + return -ENOMEM;
>>>> +
>>>> + desc_cache_name = devm_kasprintf(pt->dev, GFP_KERNEL,
>>>> + "%s-dmaengine-desc-cache",
>>>> + pt->name);
>>>> + if (!desc_cache_name) {
>>>> + ret = -ENOMEM;
>>>> + goto err_cache;
>>>> + }
>>>> +
>>>> + pt->dma_desc_cache = kmem_cache_create(desc_cache_name,
>>>> + sizeof(struct pt_dma_desc),
>>>> + sizeof(void *),
>>>
>>> sizeof void ptr?
>
> This and many more comments are left not replied, do you agree to them,
> do you disagree, hard to tell from silence..
>
Yes, I agree with all other comments and will send the changes in next
version of the patch series.
>>>
>>>> + SLAB_HWCACHE_ALIGN, NULL);
>>>> + if (!pt->dma_desc_cache) {
>>>> + ret = -ENOMEM;
>>>> + goto err_cache;
>>>> + }
>>>> +
>>>> + dma_dev->dev = pt->dev;
>>>> + dma_dev->src_addr_widths = DMA_SLAVE_BUSWIDTH_64_BYTES;
>>>> + dma_dev->dst_addr_widths = DMA_SLAVE_BUSWIDTH_64_BYTES;
>>>> + dma_dev->directions = DMA_MEM_TO_MEM;
>>>> + dma_dev->residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
>>>> + dma_cap_set(DMA_MEMCPY, dma_dev->cap_mask);
>>>> + dma_cap_set(DMA_INTERRUPT, dma_dev->cap_mask);
>>>> + dma_cap_set(DMA_PRIVATE, dma_dev->cap_mask);
>>>
>>> Why DMA_PRIVATE ? this is a dma mempcy controller ...
>>
>> This DMA controller is intended to be used with AMD Non-Transparent
>> Bridge devices and not for general purpose peripheral DMA. Hence marking
>> it as DMA_PRIVATE.
>
> Okay, maybe add a comment so that people would know
>
Sure. I will add comment here.
> --
> ~Vinod
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA
2021-06-16 4:54 ` Sanjay R Mehta
@ 2021-06-16 6:16 ` Greg KH
2021-06-16 6:57 ` Sanjay R Mehta
0 siblings, 1 reply; 26+ messages in thread
From: Greg KH @ 2021-06-16 6:16 UTC (permalink / raw)
To: Sanjay R Mehta
Cc: Vinod Koul, Sanjay R Mehta, dan.j.williams, Thomas.Lendacky,
Shyam-sundar.S-k, Nehal-bakulchandra.Shah, robh, mchehab+samsung,
davem, linux-kernel, dmaengine
On Wed, Jun 16, 2021 at 10:24:52AM +0530, Sanjay R Mehta wrote:
>
>
> On 6/16/2021 9:45 AM, Vinod Koul wrote:
> > [CAUTION: External Email]
> >
> > On 15-06-21, 16:50, Sanjay R Mehta wrote:
> >
> >>>> +static struct pt_device *pt_alloc_struct(struct device *dev)
> >>>> +{
> >>>> + struct pt_device *pt;
> >>>> +
> >>>> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
> >>>> +
> >>>> + if (!pt)
> >>>> + return NULL;
> >>>> + pt->dev = dev;
> >>>> + pt->ord = atomic_inc_return(&pt_ordinal);
> >>>
> >>> What is the use of this number?
> >>>
> >>
> >> There are eight similar instances of this DMA engine on AMD SOC.
> >> It is to differentiate each of these instances.
> >
> > Are they individual device objects?
> >
>
> Yes, they are individual device objects.
Then what is "ord" for? Why are you using an atomic variable for this?
What does this field do? Why doesn't the normal way of naming a device
come into play here instead?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA
2021-06-16 6:16 ` Greg KH
@ 2021-06-16 6:57 ` Sanjay R Mehta
2021-06-16 7:17 ` Greg KH
2021-06-16 7:52 ` Vinod Koul
0 siblings, 2 replies; 26+ messages in thread
From: Sanjay R Mehta @ 2021-06-16 6:57 UTC (permalink / raw)
To: Greg KH
Cc: Vinod Koul, Sanjay R Mehta, dan.j.williams, Thomas.Lendacky,
Shyam-sundar.S-k, Nehal-bakulchandra.Shah, robh, mchehab+samsung,
davem, linux-kernel, dmaengine
On 6/16/2021 11:46 AM, Greg KH wrote:
> [CAUTION: External Email]
>
> On Wed, Jun 16, 2021 at 10:24:52AM +0530, Sanjay R Mehta wrote:
>>
>>
>> On 6/16/2021 9:45 AM, Vinod Koul wrote:
>>> [CAUTION: External Email]
>>>
>>> On 15-06-21, 16:50, Sanjay R Mehta wrote:
>>>
>>>>>> +static struct pt_device *pt_alloc_struct(struct device *dev)
>>>>>> +{
>>>>>> + struct pt_device *pt;
>>>>>> +
>>>>>> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
>>>>>> +
>>>>>> + if (!pt)
>>>>>> + return NULL;
>>>>>> + pt->dev = dev;
>>>>>> + pt->ord = atomic_inc_return(&pt_ordinal);
>>>>>
>>>>> What is the use of this number?
>>>>>
>>>>
>>>> There are eight similar instances of this DMA engine on AMD SOC.
>>>> It is to differentiate each of these instances.
>>>
>>> Are they individual device objects?
>>>
>>
>> Yes, they are individual device objects.
>
> Then what is "ord" for? Why are you using an atomic variable for this?
> What does this field do? Why doesn't the normal way of naming a device
> come into play here instead?
>
Hi Greg,
The value of "ord" is incremented for each device instance and then it
is used to store different name for each device as shown in below snippet.
pt->ord = atomic_inc_return(&pt_ordinal);
snprintf(pt->name, MAX_PT_NAME_LEN, "pt-%u", pt->ord);
- Sanjay
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA
2021-06-16 6:57 ` Sanjay R Mehta
@ 2021-06-16 7:17 ` Greg KH
2021-06-16 7:52 ` Vinod Koul
1 sibling, 0 replies; 26+ messages in thread
From: Greg KH @ 2021-06-16 7:17 UTC (permalink / raw)
To: Sanjay R Mehta
Cc: Vinod Koul, Sanjay R Mehta, dan.j.williams, Thomas.Lendacky,
Shyam-sundar.S-k, Nehal-bakulchandra.Shah, robh, mchehab+samsung,
davem, linux-kernel, dmaengine
On Wed, Jun 16, 2021 at 12:27:32PM +0530, Sanjay R Mehta wrote:
>
>
> On 6/16/2021 11:46 AM, Greg KH wrote:
> > [CAUTION: External Email]
> >
> > On Wed, Jun 16, 2021 at 10:24:52AM +0530, Sanjay R Mehta wrote:
> >>
> >>
> >> On 6/16/2021 9:45 AM, Vinod Koul wrote:
> >>> [CAUTION: External Email]
> >>>
> >>> On 15-06-21, 16:50, Sanjay R Mehta wrote:
> >>>
> >>>>>> +static struct pt_device *pt_alloc_struct(struct device *dev)
> >>>>>> +{
> >>>>>> + struct pt_device *pt;
> >>>>>> +
> >>>>>> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
> >>>>>> +
> >>>>>> + if (!pt)
> >>>>>> + return NULL;
> >>>>>> + pt->dev = dev;
> >>>>>> + pt->ord = atomic_inc_return(&pt_ordinal);
> >>>>>
> >>>>> What is the use of this number?
> >>>>>
> >>>>
> >>>> There are eight similar instances of this DMA engine on AMD SOC.
> >>>> It is to differentiate each of these instances.
> >>>
> >>> Are they individual device objects?
> >>>
> >>
> >> Yes, they are individual device objects.
> >
> > Then what is "ord" for? Why are you using an atomic variable for this?
> > What does this field do? Why doesn't the normal way of naming a device
> > come into play here instead?
> >
>
> Hi Greg,
>
> The value of "ord" is incremented for each device instance and then it
> is used to store different name for each device as shown in below snippet.
>
> pt->ord = atomic_inc_return(&pt_ordinal);
> snprintf(pt->name, MAX_PT_NAME_LEN, "pt-%u", pt->ord);
Why not use an idr structure like this like all other drivers do? That
way when devices are removed the numbers are properly reused as well.
And why do you need to save the value?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA
2021-06-16 6:57 ` Sanjay R Mehta
2021-06-16 7:17 ` Greg KH
@ 2021-06-16 7:52 ` Vinod Koul
2021-06-16 7:59 ` Greg KH
2021-06-16 12:00 ` Sanjay R Mehta
1 sibling, 2 replies; 26+ messages in thread
From: Vinod Koul @ 2021-06-16 7:52 UTC (permalink / raw)
To: Sanjay R Mehta
Cc: Greg KH, Sanjay R Mehta, dan.j.williams, Thomas.Lendacky,
Shyam-sundar.S-k, Nehal-bakulchandra.Shah, robh, mchehab+samsung,
davem, linux-kernel, dmaengine
On 16-06-21, 12:27, Sanjay R Mehta wrote:
>
>
> On 6/16/2021 11:46 AM, Greg KH wrote:
> > [CAUTION: External Email]
> >
> > On Wed, Jun 16, 2021 at 10:24:52AM +0530, Sanjay R Mehta wrote:
> >>
> >>
> >> On 6/16/2021 9:45 AM, Vinod Koul wrote:
> >>> [CAUTION: External Email]
> >>>
> >>> On 15-06-21, 16:50, Sanjay R Mehta wrote:
> >>>
> >>>>>> +static struct pt_device *pt_alloc_struct(struct device *dev)
> >>>>>> +{
> >>>>>> + struct pt_device *pt;
> >>>>>> +
> >>>>>> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
> >>>>>> +
> >>>>>> + if (!pt)
> >>>>>> + return NULL;
> >>>>>> + pt->dev = dev;
> >>>>>> + pt->ord = atomic_inc_return(&pt_ordinal);
> >>>>>
> >>>>> What is the use of this number?
> >>>>>
> >>>>
> >>>> There are eight similar instances of this DMA engine on AMD SOC.
> >>>> It is to differentiate each of these instances.
> >>>
> >>> Are they individual device objects?
> >>>
> >>
> >> Yes, they are individual device objects.
> >
> > Then what is "ord" for? Why are you using an atomic variable for this?
> > What does this field do? Why doesn't the normal way of naming a device
> > come into play here instead?
> >
>
> Hi Greg,
>
> The value of "ord" is incremented for each device instance and then it
> is used to store different name for each device as shown in below snippet.
>
> pt->ord = atomic_inc_return(&pt_ordinal);
> snprintf(pt->name, MAX_PT_NAME_LEN, "pt-%u", pt->ord);
Okay why not use device->name ?
Trying to unroll further, who creates pt_device? who creates the dev
object under this..?
Thanks
--
~Vinod
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA
2021-06-16 7:52 ` Vinod Koul
@ 2021-06-16 7:59 ` Greg KH
2021-06-16 9:46 ` Sanjay R Mehta
2021-06-16 12:00 ` Sanjay R Mehta
1 sibling, 1 reply; 26+ messages in thread
From: Greg KH @ 2021-06-16 7:59 UTC (permalink / raw)
To: Vinod Koul
Cc: Sanjay R Mehta, Sanjay R Mehta, dan.j.williams, Thomas.Lendacky,
Shyam-sundar.S-k, Nehal-bakulchandra.Shah, robh, mchehab+samsung,
davem, linux-kernel, dmaengine
On Wed, Jun 16, 2021 at 01:22:54PM +0530, Vinod Koul wrote:
> On 16-06-21, 12:27, Sanjay R Mehta wrote:
> >
> >
> > On 6/16/2021 11:46 AM, Greg KH wrote:
> > > [CAUTION: External Email]
> > >
> > > On Wed, Jun 16, 2021 at 10:24:52AM +0530, Sanjay R Mehta wrote:
> > >>
> > >>
> > >> On 6/16/2021 9:45 AM, Vinod Koul wrote:
> > >>> [CAUTION: External Email]
> > >>>
> > >>> On 15-06-21, 16:50, Sanjay R Mehta wrote:
> > >>>
> > >>>>>> +static struct pt_device *pt_alloc_struct(struct device *dev)
In looking at this, why are you dealing with a "raw" struct device?
Shouldn't this be a parent pointer? Why not pass in the real type that
this can be made a child of?
> > >>>>>> +{
> > >>>>>> + struct pt_device *pt;
> > >>>>>> +
> > >>>>>> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
> > >>>>>> +
> > >>>>>> + if (!pt)
> > >>>>>> + return NULL;
> > >>>>>> + pt->dev = dev;
> > >>>>>> + pt->ord = atomic_inc_return(&pt_ordinal);
> > >>>>>
> > >>>>> What is the use of this number?
> > >>>>>
> > >>>>
> > >>>> There are eight similar instances of this DMA engine on AMD SOC.
> > >>>> It is to differentiate each of these instances.
> > >>>
> > >>> Are they individual device objects?
> > >>>
> > >>
> > >> Yes, they are individual device objects.
> > >
> > > Then what is "ord" for? Why are you using an atomic variable for this?
> > > What does this field do? Why doesn't the normal way of naming a device
> > > come into play here instead?
> > >
> >
> > Hi Greg,
> >
> > The value of "ord" is incremented for each device instance and then it
> > is used to store different name for each device as shown in below snippet.
> >
> > pt->ord = atomic_inc_return(&pt_ordinal);
> > snprintf(pt->name, MAX_PT_NAME_LEN, "pt-%u", pt->ord);
>
> Okay why not use device->name ?
Ah, I missed this. Yes, do not have 2 names for the same structure,
that is wasteful and confusing.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA
2021-06-16 7:59 ` Greg KH
@ 2021-06-16 9:46 ` Sanjay R Mehta
2021-06-16 9:56 ` Vinod Koul
0 siblings, 1 reply; 26+ messages in thread
From: Sanjay R Mehta @ 2021-06-16 9:46 UTC (permalink / raw)
To: Greg KH, Vinod Koul
Cc: Sanjay R Mehta, dan.j.williams, Thomas.Lendacky,
Shyam-sundar.S-k, Nehal-bakulchandra.Shah, robh, mchehab+samsung,
davem, linux-kernel, dmaengine
On 6/16/2021 1:29 PM, Greg KH wrote:
> [CAUTION: External Email]
>
> On Wed, Jun 16, 2021 at 01:22:54PM +0530, Vinod Koul wrote:
>> On 16-06-21, 12:27, Sanjay R Mehta wrote:
>>>
>>>
>>> On 6/16/2021 11:46 AM, Greg KH wrote:
>>>> [CAUTION: External Email]
>>>>
>>>> On Wed, Jun 16, 2021 at 10:24:52AM +0530, Sanjay R Mehta wrote:
>>>>>
>>>>>
>>>>> On 6/16/2021 9:45 AM, Vinod Koul wrote:
>>>>>> [CAUTION: External Email]
>>>>>>
>>>>>> On 15-06-21, 16:50, Sanjay R Mehta wrote:
>>>>>>
>>>>>>>>> +static struct pt_device *pt_alloc_struct(struct device *dev)
>
> In looking at this, why are you dealing with a "raw" struct device?
> Shouldn't this be a parent pointer? Why not pass in the real type that
> this can be made a child of?
>
>
>>>>>>>>> +{
>>>>>>>>> + struct pt_device *pt;
>>>>>>>>> +
>>>>>>>>> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
>>>>>>>>> +
>>>>>>>>> + if (!pt)
>>>>>>>>> + return NULL;
>>>>>>>>> + pt->dev = dev;
>>>>>>>>> + pt->ord = atomic_inc_return(&pt_ordinal);
>>>>>>>>
>>>>>>>> What is the use of this number?
>>>>>>>>
>>>>>>>
>>>>>>> There are eight similar instances of this DMA engine on AMD SOC.
>>>>>>> It is to differentiate each of these instances.
>>>>>>
>>>>>> Are they individual device objects?
>>>>>>
>>>>>
>>>>> Yes, they are individual device objects.
>>>>
>>>> Then what is "ord" for? Why are you using an atomic variable for this?
>>>> What does this field do? Why doesn't the normal way of naming a device
>>>> come into play here instead?
>>>>
>>>
>>> Hi Greg,
>>>
>>> The value of "ord" is incremented for each device instance and then it
>>> is used to store different name for each device as shown in below snippet.
>>>
>>> pt->ord = atomic_inc_return(&pt_ordinal);
>>> snprintf(pt->name, MAX_PT_NAME_LEN, "pt-%u", pt->ord);
>>
>> Okay why not use device->name ?
>
> Ah, I missed this. Yes, do not have 2 names for the same structure,
> that is wasteful and confusing.
>
Thanks, Greg & Vinod. I just verified with "dev_name(dev)" and this is
serving the purpose :).
I will send this change in the next version.
- Sanjay
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA
2021-06-16 9:46 ` Sanjay R Mehta
@ 2021-06-16 9:56 ` Vinod Koul
0 siblings, 0 replies; 26+ messages in thread
From: Vinod Koul @ 2021-06-16 9:56 UTC (permalink / raw)
To: Sanjay R Mehta
Cc: Greg KH, Sanjay R Mehta, dan.j.williams, Thomas.Lendacky,
Shyam-sundar.S-k, Nehal-bakulchandra.Shah, robh, mchehab+samsung,
davem, linux-kernel, dmaengine
On 16-06-21, 15:16, Sanjay R Mehta wrote:
>
>
> On 6/16/2021 1:29 PM, Greg KH wrote:
> > [CAUTION: External Email]
> >
> > On Wed, Jun 16, 2021 at 01:22:54PM +0530, Vinod Koul wrote:
> >> On 16-06-21, 12:27, Sanjay R Mehta wrote:
> >>>
> >>>
> >>> On 6/16/2021 11:46 AM, Greg KH wrote:
> >>>> [CAUTION: External Email]
> >>>>
> >>>> On Wed, Jun 16, 2021 at 10:24:52AM +0530, Sanjay R Mehta wrote:
> >>>>>
> >>>>>
> >>>>> On 6/16/2021 9:45 AM, Vinod Koul wrote:
> >>>>>> [CAUTION: External Email]
> >>>>>>
> >>>>>> On 15-06-21, 16:50, Sanjay R Mehta wrote:
> >>>>>>
> >>>>>>>>> +static struct pt_device *pt_alloc_struct(struct device *dev)
> >
> > In looking at this, why are you dealing with a "raw" struct device?
> > Shouldn't this be a parent pointer? Why not pass in the real type that
> > this can be made a child of?
> >
> >
> >>>>>>>>> +{
> >>>>>>>>> + struct pt_device *pt;
> >>>>>>>>> +
> >>>>>>>>> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
> >>>>>>>>> +
> >>>>>>>>> + if (!pt)
> >>>>>>>>> + return NULL;
> >>>>>>>>> + pt->dev = dev;
> >>>>>>>>> + pt->ord = atomic_inc_return(&pt_ordinal);
> >>>>>>>>
> >>>>>>>> What is the use of this number?
> >>>>>>>>
> >>>>>>>
> >>>>>>> There are eight similar instances of this DMA engine on AMD SOC.
> >>>>>>> It is to differentiate each of these instances.
> >>>>>>
> >>>>>> Are they individual device objects?
> >>>>>>
> >>>>>
> >>>>> Yes, they are individual device objects.
> >>>>
> >>>> Then what is "ord" for? Why are you using an atomic variable for this?
> >>>> What does this field do? Why doesn't the normal way of naming a device
> >>>> come into play here instead?
> >>>>
> >>>
> >>> Hi Greg,
> >>>
> >>> The value of "ord" is incremented for each device instance and then it
> >>> is used to store different name for each device as shown in below snippet.
> >>>
> >>> pt->ord = atomic_inc_return(&pt_ordinal);
> >>> snprintf(pt->name, MAX_PT_NAME_LEN, "pt-%u", pt->ord);
> >>
> >> Okay why not use device->name ?
> >
> > Ah, I missed this. Yes, do not have 2 names for the same structure,
> > that is wasteful and confusing.
> >
>
> Thanks, Greg & Vinod. I just verified with "dev_name(dev)" and this is
> serving the purpose :).
>
> I will send this change in the next version.
Great, but there are few more questions I had, like who creates the
device etc, can you please respond to those questions as well, so that
we understand properly how this device works
Thanks
--
~Vinod
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA
2021-06-16 7:52 ` Vinod Koul
2021-06-16 7:59 ` Greg KH
@ 2021-06-16 12:00 ` Sanjay R Mehta
2021-06-16 12:23 ` Greg KH
1 sibling, 1 reply; 26+ messages in thread
From: Sanjay R Mehta @ 2021-06-16 12:00 UTC (permalink / raw)
To: Vinod Koul
Cc: Greg KH, Sanjay R Mehta, dan.j.williams, Thomas.Lendacky,
Shyam-sundar.S-k, Nehal-bakulchandra.Shah, robh, mchehab+samsung,
davem, linux-kernel, dmaengine
On 6/16/2021 1:22 PM, Vinod Koul wrote:
> [CAUTION: External Email]
>
> On 16-06-21, 12:27, Sanjay R Mehta wrote:
>>
>>
>> On 6/16/2021 11:46 AM, Greg KH wrote:
>>> [CAUTION: External Email]
>>>
>>> On Wed, Jun 16, 2021 at 10:24:52AM +0530, Sanjay R Mehta wrote:
>>>>
>>>>
>>>> On 6/16/2021 9:45 AM, Vinod Koul wrote:
>>>>> [CAUTION: External Email]
>>>>>
>>>>> On 15-06-21, 16:50, Sanjay R Mehta wrote:
>>>>>
>>>>>>>> +static struct pt_device *pt_alloc_struct(struct device *dev)
>>>>>>>> +{
>>>>>>>> + struct pt_device *pt;
>>>>>>>> +
>>>>>>>> + pt = devm_kzalloc(dev, sizeof(*pt), GFP_KERNEL);
>>>>>>>> +
>>>>>>>> + if (!pt)
>>>>>>>> + return NULL;
>>>>>>>> + pt->dev = dev;
>>>>>>>> + pt->ord = atomic_inc_return(&pt_ordinal);
>>>>>>>
>>>>>>> What is the use of this number?
>>>>>>>
>>>>>>
>>>>>> There are eight similar instances of this DMA engine on AMD SOC.
>>>>>> It is to differentiate each of these instances.
>>>>>
>>>>> Are they individual device objects?
>>>>>
>>>>
>>>> Yes, they are individual device objects.
>>>
>>> Then what is "ord" for? Why are you using an atomic variable for this?
>>> What does this field do? Why doesn't the normal way of naming a device
>>> come into play here instead?
>>>
>>
>> Hi Greg,
>>
>> The value of "ord" is incremented for each device instance and then it
>> is used to store different name for each device as shown in below snippet.
>>
>> pt->ord = atomic_inc_return(&pt_ordinal);
>> snprintf(pt->name, MAX_PT_NAME_LEN, "pt-%u", pt->ord);
>
> Okay why not use device->name ?
>
> Trying to unroll further, who creates pt_device? who creates the dev
> object under this..?
>
Hi Vinod,
The pt_device is allocated and initialized in the PCI probe function and
then we just get the "dev" from the "pci_dev" object and save it in
"pt->dev" as shown in below snippet.
static int pt_pci_probe(struct pci_dev *pdev, const struct
pci_device_id *id)
{
struct pt_device *pt;
struct pt_msix *pt_msix;
struct device *dev = &pdev->dev;
Thanks,
- Sanjay
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA
2021-06-16 12:00 ` Sanjay R Mehta
@ 2021-06-16 12:23 ` Greg KH
2021-06-16 12:53 ` Sanjay R Mehta
0 siblings, 1 reply; 26+ messages in thread
From: Greg KH @ 2021-06-16 12:23 UTC (permalink / raw)
To: Sanjay R Mehta
Cc: Vinod Koul, Sanjay R Mehta, dan.j.williams, Thomas.Lendacky,
Shyam-sundar.S-k, Nehal-bakulchandra.Shah, robh, mchehab+samsung,
davem, linux-kernel, dmaengine
On Wed, Jun 16, 2021 at 05:30:49PM +0530, Sanjay R Mehta wrote:
> The pt_device is allocated and initialized in the PCI probe function and
> then we just get the "dev" from the "pci_dev" object and save it in
> "pt->dev" as shown in below snippet.
>
>
> static int pt_pci_probe(struct pci_dev *pdev, const struct
> pci_device_id *id)
> {
> struct pt_device *pt;
> struct pt_msix *pt_msix;
> struct device *dev = &pdev->dev;
So "dev" is a parent here, or something else?
If it is the parent, please call it such otherwise it is confusing.
If you are creating child devices, what bus do they belong to?
Can you fix up this series and resend it so that we can review it again?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA
2021-06-16 12:23 ` Greg KH
@ 2021-06-16 12:53 ` Sanjay R Mehta
2021-06-16 12:57 ` Greg KH
0 siblings, 1 reply; 26+ messages in thread
From: Sanjay R Mehta @ 2021-06-16 12:53 UTC (permalink / raw)
To: Greg KH
Cc: Vinod Koul, Sanjay R Mehta, dan.j.williams, Thomas.Lendacky,
Shyam-sundar.S-k, Nehal-bakulchandra.Shah, robh, mchehab+samsung,
davem, linux-kernel, dmaengine
On 6/16/2021 5:53 PM, Greg KH wrote:
> [CAUTION: External Email]
>
> On Wed, Jun 16, 2021 at 05:30:49PM +0530, Sanjay R Mehta wrote:
>> The pt_device is allocated and initialized in the PCI probe function and
>> then we just get the "dev" from the "pci_dev" object and save it in
>> "pt->dev" as shown in below snippet.
>>
>>
>> static int pt_pci_probe(struct pci_dev *pdev, const struct
>> pci_device_id *id)
>> {
>> struct pt_device *pt;
>> struct pt_msix *pt_msix;
>> struct device *dev = &pdev->dev;
>
> So "dev" is a parent here, or something else?
>
> If it is the parent, please call it such otherwise it is confusing.
>
> If you are creating child devices, what bus do they belong to?
>
> Can you fix up this series and resend it so that we can review it again?
>
Hi Greg,
Yes, "dev" is the parent here and there are no child devices created.
My apologies for not calling it rightly.
Sure, I will fix up this series addressing all the comments and will
send the next version.
Thanks,
-Sanjay
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA
2021-06-16 12:53 ` Sanjay R Mehta
@ 2021-06-16 12:57 ` Greg KH
0 siblings, 0 replies; 26+ messages in thread
From: Greg KH @ 2021-06-16 12:57 UTC (permalink / raw)
To: Sanjay R Mehta
Cc: Vinod Koul, Sanjay R Mehta, dan.j.williams, Thomas.Lendacky,
Shyam-sundar.S-k, Nehal-bakulchandra.Shah, robh, mchehab+samsung,
davem, linux-kernel, dmaengine
On Wed, Jun 16, 2021 at 06:23:10PM +0530, Sanjay R Mehta wrote:
>
>
> On 6/16/2021 5:53 PM, Greg KH wrote:
> > [CAUTION: External Email]
> >
> > On Wed, Jun 16, 2021 at 05:30:49PM +0530, Sanjay R Mehta wrote:
> >> The pt_device is allocated and initialized in the PCI probe function and
> >> then we just get the "dev" from the "pci_dev" object and save it in
> >> "pt->dev" as shown in below snippet.
> >>
> >>
> >> static int pt_pci_probe(struct pci_dev *pdev, const struct
> >> pci_device_id *id)
> >> {
> >> struct pt_device *pt;
> >> struct pt_msix *pt_msix;
> >> struct device *dev = &pdev->dev;
> >
> > So "dev" is a parent here, or something else?
> >
> > If it is the parent, please call it such otherwise it is confusing.
> >
> > If you are creating child devices, what bus do they belong to?
> >
> > Can you fix up this series and resend it so that we can review it again?
> >
>
> Hi Greg,
>
> Yes, "dev" is the parent here and there are no child devices created.
But you should be creating a child device, as that will be the name you
need. Or again, I am probably confused, I'll wait for the next round of
patches...
thanks,
greg k-h
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v9 2/3] dmaengine: ptdma: register PTDMA controller as a DMA resource
2021-06-08 18:56 ` Vinod Koul
2021-06-15 11:34 ` Sanjay R Mehta
@ 2021-06-20 3:52 ` Sanjay R Mehta
1 sibling, 0 replies; 26+ messages in thread
From: Sanjay R Mehta @ 2021-06-20 3:52 UTC (permalink / raw)
To: Vinod Koul, Sanjay R Mehta
Cc: gregkh, dan.j.williams, Thomas.Lendacky, Shyam-sundar.S-k,
Nehal-bakulchandra.Shah, robh, mchehab+samsung, davem,
linux-kernel, dmaengine
Hi Vinod,
I have addressed all the feedback provided by you. I will be send the
update in the next version of this series.
On 6/9/2021 12:26 AM, Vinod Koul wrote:
> [CAUTION: External Email]
>
> On 02-06-21, 12:22, Sanjay R Mehta wrote:
>
>> +static void pt_do_cmd_complete(unsigned long data)
>> +{
>> + struct pt_tasklet_data *tdata = (struct pt_tasklet_data *)data;
>> + struct pt_cmd *cmd = tdata->cmd;
>> + struct pt_cmd_queue *cmd_q = &cmd->pt->cmd_q;
>> + u32 tail;
>> +
>> + tail = lower_32_bits(cmd_q->qdma_tail + cmd_q->qidx * Q_DESC_SIZE);
>
> why extract tail in non error case?
>
You are right. it is unnecessary in a non-error case and I will correct
it in the next version.
>> +static int pt_issue_next_cmd(struct pt_dma_desc *desc)
>> +{
>> + struct pt_passthru_engine *pt_engine;
>> + struct pt_dma_cmd *cmd;
>> + struct pt_device *pt;
>> + struct pt_cmd *pt_cmd;
>> + struct pt_cmd_queue *cmd_q;
>> +
>> + cmd = list_first_entry(&desc->cmdlist, struct pt_dma_cmd, entry);
>> + desc->issued_to_hw = 1;
>
> Why do you need this, the descriptor should be in vc.desc_issued list
>
Agree. I have removed the dependency of cmdlist everywhere from the code
and used the vc.desc_issued instead.
>> +static void pt_free_active_cmd(struct pt_dma_desc *desc)
>> +{
>> + struct pt_dma_cmd *cmd = NULL;
>> +
>> + if (desc->issued_to_hw)
>> + cmd = list_first_entry_or_null(&desc->cmdlist, struct pt_dma_cmd,
>> + entry);
>
> single line pls and use lists provided..
>
>
>> +static struct pt_dma_desc *pt_handle_active_desc(struct pt_dma_chan *chan,
>> + struct pt_dma_desc *desc)
>> +{
>> + struct dma_async_tx_descriptor *tx_desc;
>> + struct virt_dma_desc *vd;
>> + unsigned long flags;
>> +
>> + /* Loop over descriptors until one is found with commands */
>> + do {
>> + if (desc) {
>> + /* Remove the DMA command from the list and free it */
>> + pt_free_active_cmd(desc);
>> + if (!desc->issued_to_hw) {
>> + /* No errors, keep going */
>> + if (desc->status != DMA_ERROR)
>> + return desc;
>> + /* Error, free remaining commands and move on */
>> + pt_free_cmd_resources(desc->pt,
>> + &desc->cmdlist);
>
> single line again
>
Sure. I will addressand send in the next version.
>> +static struct pt_dma_desc *pt_alloc_dma_desc(struct pt_dma_chan *chan,
>> + unsigned long flags)
>> +{
>> + struct pt_dma_desc *desc;
>> +
>> + desc = kmem_cache_zalloc(chan->pt->dma_desc_cache, GFP_NOWAIT);
>> + if (!desc)
>> + return NULL;
>> +
>> + vchan_tx_prep(&chan->vc, &desc->vd, flags);
>> +
>> + desc->pt = chan->pt;
>> + desc->issued_to_hw = 0;
>> + INIT_LIST_HEAD(&desc->cmdlist);
>
> why do you need your own list, the lists in vc should suffice?
>
Agree. I have removed the dependency of this list everywhere from the code.
>> +static int pt_resume(struct dma_chan *dma_chan)
>> +{
>> + struct pt_dma_chan *chan = to_pt_chan(dma_chan);
>> + struct pt_dma_desc *desc = NULL;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&chan->vc.lock, flags);
>> + pt_start_queue(&chan->pt->cmd_q);
>> + desc = __pt_next_dma_desc(chan);
>> + spin_unlock_irqrestore(&chan->vc.lock, flags);
>> +
>> + /* If there was something active, re-start */
>> + if (desc)
>> + pt_cmd_callback(desc, 0);
>
> this doesn't sound correct. In pause yoy stop the queue, so start of the
> queue should be done here... Why grab a descriptor?
>
The start of queue is already done here.
If there was any active descriptor when pause occured, that is needs to
be started again during resume. Hence getting that descriptor and
restarting the transaction.
>> +static int pt_terminate_all(struct dma_chan *dma_chan)
>> +{
>> + struct pt_dma_chan *chan = to_pt_chan(dma_chan);
>> +
>> + vchan_free_chan_resources(&chan->vc);
>
> what about the descriptors, are you not going to clear the lists and
> free them..
>
Agree. I will address this by clearing the lists & free them and will
send the update in the next version.
>> +int pt_dmaengine_register(struct pt_device *pt)
>> +{
>> + struct pt_dma_chan *chan;
>> + struct dma_device *dma_dev = &pt->dma_dev;
>> + char *cmd_cache_name;
>> + char *desc_cache_name;
>> + int ret;
>> +
>> + pt->pt_dma_chan = devm_kzalloc(pt->dev, sizeof(*pt->pt_dma_chan),
>> + GFP_KERNEL);
>> + if (!pt->pt_dma_chan)
>> + return -ENOMEM;
>> +
>> + cmd_cache_name = devm_kasprintf(pt->dev, GFP_KERNEL,
>> + "%s-dmaengine-cmd-cache",
>> + pt->name);
>> + if (!cmd_cache_name)
>> + return -ENOMEM;
>> +
>> + pt->dma_cmd_cache = kmem_cache_create(cmd_cache_name,
>> + sizeof(struct pt_dma_cmd),
>> + sizeof(void *),
>> + SLAB_HWCACHE_ALIGN, NULL);
>> + if (!pt->dma_cmd_cache)
>> + return -ENOMEM;
>> +
>> + desc_cache_name = devm_kasprintf(pt->dev, GFP_KERNEL,
>> + "%s-dmaengine-desc-cache",
>> + pt->name);
>> + if (!desc_cache_name) {
>> + ret = -ENOMEM;
>> + goto err_cache;
>> + }
>> +
>> + pt->dma_desc_cache = kmem_cache_create(desc_cache_name,
>> + sizeof(struct pt_dma_desc),
>> + sizeof(void *),
>
> sizeof void ptr?
>
Sure. I will address and send in the next version.
>> + SLAB_HWCACHE_ALIGN, NULL);
>> + if (!pt->dma_desc_cache) {
>> + ret = -ENOMEM;
>> + goto err_cache;
>> + }
>> +
>> + dma_dev->dev = pt->dev;
>> + dma_dev->src_addr_widths = DMA_SLAVE_BUSWIDTH_64_BYTES;
>> + dma_dev->dst_addr_widths = DMA_SLAVE_BUSWIDTH_64_BYTES;
>> + dma_dev->directions = DMA_MEM_TO_MEM;
>> + dma_dev->residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
>> + dma_cap_set(DMA_MEMCPY, dma_dev->cap_mask);
>> + dma_cap_set(DMA_INTERRUPT, dma_dev->cap_mask);
>> + dma_cap_set(DMA_PRIVATE, dma_dev->cap_mask);
>
> Why DMA_PRIVATE ? this is a dma mempcy controller ...
> --
> ~Vinod
>
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2021-06-20 3:52 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-02 17:22 [PATCH v9 0/3] Add support for AMD PTDMA controller driver Sanjay R Mehta
2021-06-02 17:22 ` [PATCH v9 1/3] dmaengine: ptdma: Initial driver for the AMD PTDMA Sanjay R Mehta
2021-06-08 17:39 ` Vinod Koul
2021-06-15 11:20 ` Sanjay R Mehta
2021-06-16 4:15 ` Vinod Koul
2021-06-16 4:54 ` Sanjay R Mehta
2021-06-16 6:16 ` Greg KH
2021-06-16 6:57 ` Sanjay R Mehta
2021-06-16 7:17 ` Greg KH
2021-06-16 7:52 ` Vinod Koul
2021-06-16 7:59 ` Greg KH
2021-06-16 9:46 ` Sanjay R Mehta
2021-06-16 9:56 ` Vinod Koul
2021-06-16 12:00 ` Sanjay R Mehta
2021-06-16 12:23 ` Greg KH
2021-06-16 12:53 ` Sanjay R Mehta
2021-06-16 12:57 ` Greg KH
2021-06-02 17:22 ` [PATCH v9 2/3] dmaengine: ptdma: register PTDMA controller as a DMA resource Sanjay R Mehta
2021-06-08 18:56 ` Vinod Koul
2021-06-15 11:34 ` Sanjay R Mehta
2021-06-16 4:18 ` Vinod Koul
2021-06-16 5:23 ` Sanjay R Mehta
2021-06-20 3:52 ` Sanjay R Mehta
2021-06-02 17:22 ` [PATCH v9 3/3] dmaengine: ptdma: Add debugfs entries for PTDMA Sanjay R Mehta
2021-06-09 14:10 ` Vinod Koul
2021-06-15 11:18 ` Sanjay R Mehta
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.