Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 24 Jan 2020 11:30:58 +0530
From:   Vinod Koul <vkoul@kernel.org>
To:     Sanjay R Mehta <Sanju.Mehta@amd.com>
Cc:     gregkh@linuxfoundation.org, dan.j.williams@intel.com,
        Thomas.Lendacky@amd.com, Shyam-sundar.S-k@amd.com,
        Nehal-bakulchandra.Shah@amd.com, robh@kernel.org,
        mchehab+samsung@kernel.org, davem@davemloft.net,
        Jonathan.Cameron@huawei.com, linux-kernel@vger.kernel.org,
        dmaengine@vger.kernel.org, Gary R Hook <gary.hook@amd.com>
Subject: Re: [PATCH v3 1/3] dmaengine: ptdma: Initial driver for the AMD
 PassThru DMA engine
Message-ID: <20200124060058.GC2841@vkoul-mobl>
References: <1579597494-60348-1-git-send-email-Sanju.Mehta@amd.com>
 <1579597494-60348-2-git-send-email-Sanju.Mehta@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1579597494-60348-2-git-send-email-Sanju.Mehta@amd.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 21-01-20, 03:04, Sanjay R Mehta wrote:
> From: Sanjay R Mehta <sanju.mehta@amd.com>
> 
> This device performs high-bandwidth memory-to-memory
> transfer operations.

Why is it called PassThru DMA engine?

>  obj-y += mediatek/
>  obj-y += qcom/
> diff --git a/drivers/dma/ptdma/Kconfig b/drivers/dma/ptdma/Kconfig
> new file mode 100644
> index 0000000..4ec259e
> --- /dev/null
> +++ b/drivers/dma/ptdma/Kconfig
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config AMD_PTDMA
> +	tristate  "AMD PassThru DMA Engine"
> +	depends on X86_64 && PCI

not using DMA_VIRTUAL_CHANNELS?

> +	help
> +	  Provides the support for AMD PassThru DMA Engine.

more help text please

> +++ b/drivers/dma/ptdma/ptdma-dev.c
> @@ -0,0 +1,387 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * AMD Passthru DMA device driver
> + * -- Based on the CCP driver

What is ccp driver?

> + *
> + * Copyright (C) 2016,2019 Advanced Micro Devices, Inc.

2020..

> +#ifdef CONFIG_PM
> +static int pt_pci_suspend(struct pci_dev *pdev, pm_message_t state)
> +{
> +	return -ENOSYS;
> +}
> +
> +static int pt_pci_resume(struct pci_dev *pdev)
> +{
> +	return -ENOSYS;
> +}
> +#endif

please remove the dummy code, you can add these when you have support
for it

> +/* Bit masks */
> +#define	CMD_DESC_DW0_VAL		0x500012
> +#define	CMD_CONFIG_REQID		0x0
> +#define	CMD_CONFIG_VHB_EN		0x00000001
> +#define	CMD_QUEUE_PRIO			0x00000006
> +#define	CMD_TIMEOUT_DISABLE		0x00000000
> +#define	CMD_CLK_DYN_GATING_EN	0x1
> +#define	CMD_CLK_DYN_GATING_DIS	0x0
> +#define	CMD_CLK_HW_GATE_MODE	0x1
> +#define	CMD_CLK_SW_GATE_MODE	0x0
> +#define	CMD_CLK_GATE_ON_DELAY	0x1000
> +#define	CMD_CLK_GATE_CTL	0x0
> +#define	CMD_CLK_GATE_OFF_DELAY	0x1000

BIT and GENMASK for these...

All this adds handling of pt controller, I am not seeing dmaengine bits,
so please word the changelog accordingly and mention this adds based
bits and not dmaengine support (yet)

-- 
~Vinod