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

Vinod Koul
2020-05-04T05:55:45Z

On 28-04-20, 16:13, Sanjay R Mehta wrote:
> From: Sanjay R Mehta <sanju.mehta@amd.com>
>
> This driver add support for AMD PTDMA controller. This device
> 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.
>
> The driver handles multiple devices, which are logged on a linked
> list; all devices are treated equivalently.
>
> 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 | 399 ++++++++++++++++++++++++++++++++++++++++++
> drivers/dma/ptdma/ptdma-pci.c | 253 ++++++++++++++++++++++++++
> drivers/dma/ptdma/ptdma.h | 324 ++++++++++++++++++++++++++++++++++
> 8 files changed, 1006 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 e64e5db..8bf94b4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -921,6 +921,12 @@ S: Supported
> F: arch/arm64/boot/dts/amd/amd-seattle-xgbe*.dtsi
> F: drivers/net/ethernet/amd/xgbe/
>
> +AMD PTDMA DRIVER
> +M: Sanjay R Mehta <sanju.mehta@amd.com>
> +L: dmaengine@vger.kernel.org
> +S: Maintained
> +F: drivers/dma/ptdma/
> +
> ANALOG DEVICES INC AD5686 DRIVER
> M: Stefan Popa <stefan.popa@analog.com>
> L: linux-pm@vger.kernel.org
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 0924836..44b8d73 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -738,6 +738,8 @@ source "drivers/dma/ti/Kconfig"
>
> source "drivers/dma/fsl-dpaa2-qdma/Kconfig"
>
> +source "drivers/dma/ptdma/Kconfig"
> +
> # clients
> comment "DMA Clients"
> depends on DMA_ENGINE
> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> index e60f813..2785756 100644
> --- a/drivers/dma/Makefile
> +++ b/drivers/dma/Makefile
> @@ -83,6 +83,7 @@ obj-$(CONFIG_XGENE_DMA) += xgene-dma.o
> obj-$(CONFIG_ZX_DMA) += zx_dma.o
> obj-$(CONFIG_ST_FDMA) += st_fdma.o
> obj-$(CONFIG_FSL_DPAA2_QDMA) += fsl-dpaa2-qdma/
> +obj-$(CONFIG_AMD_PTDMA) += ptdma/
>
> obj-y += mediatek/
> obj-y += qcom/
> diff --git a/drivers/dma/ptdma/Kconfig b/drivers/dma/ptdma/Kconfig
> new file mode 100644
> index 0000000..f93f9c2
> --- /dev/null
> +++ b/drivers/dma/ptdma/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config AMD_PTDMA
> + tristate "AMD PassThru DMA Engine"
> + depends on X86_64 && PCI
> + help
> + Enable support for the AMD PTDMA controller. This controller
> + provides DMA capabilities & performs high bandwidth memory to
> + memory and IO copy operation and performs DMA transfer through
> + queue based descriptor management. This DMA controller is intended
> + to use with AMD Non-Transparent Bridge devices and not for general
> + purpose slave DMA.
> diff --git a/drivers/dma/ptdma/Makefile b/drivers/dma/ptdma/Makefile
> new file mode 100644
> index 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..0a69fd4
> --- /dev/null
> +++ b/drivers/dma/ptdma/ptdma-dev.c
> @@ -0,0 +1,399 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * AMD Passthru DMA device driver
> + * -- Based on the CCP driver
> + *
> + * Copyright (C) 2016,2020 Advanced Micro Devices, Inc.
> + *
> + * Author: Sanjay R Mehta <sanju.mehta@amd.com>
> + * Author: Gary R Hook <gary.hook@amd.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/pci.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/interrupt.h>
> +
> +#include "ptdma.h"
> +
> +static int cmd_queue_length = 32;
> +module_param(cmd_queue_length, uint, 0644);
> +MODULE_PARM_DESC(cmd_queue_length,
> + " length of the command queue, a power of 2 (2 <= val <= 128)");

Any reason for this as module param? who will configure this and how?

> + * List of PTDMAs, PTDMA count, read-write access lock, and access functions
> + *
> + * Lock structure: get pt_unit_lock for reading whenever we need to
> + * examine the PTDMA list. While holding it for reading we can acquire
> + * the RR lock to update the round-robin next-PTDMA pointer. The unit lock
> + * must be acquired before the RR lock.
> + *
> + * If the unit-lock is acquired for writing, we have total control over
> + * the list, so there's no value in getting the RR lock.
> + */
> +static DEFINE_RWLOCK(pt_unit_lock);
> +static LIST_HEAD(pt_units);
> +
> +static struct pt_device *pt_rr;

why do we need these globals and not in driver context?

> +static void pt_add_device(struct pt_device *pt)
> +{
> + unsigned long flags;
> +
> + write_lock_irqsave(&pt_unit_lock, flags);
> + list_add_tail(&pt->entry, &pt_units);
> + if (!pt_rr)
> + /*
> + * We already have the list lock (we're first) so this
> + * pointer can't change on us. Set its initial value.
> + */
> + pt_rr = pt;
> + write_unlock_irqrestore(&pt_unit_lock, flags);
> +}

Can you please explain what do you mean by having a list of devices and
why are we adding/removing dynamically?

--
~Vinod