Re: [PATCH v2] dmaengine: hisilicon: Add Kunpeng DMA engine support

From: Vinod Koul <vkoul@kernel.org>
To: Zhou Wang <wangzhou1@hisilicon.com>
Cc: Dan Williams <dan.j.williams@intel.com>,
	dmaengine@vger.kernel.org, linuxarm@huawei.com,
	Zhenfa Qiu <qiuzhenfa@hisilicon.com>
Subject: Re: [PATCH v2] dmaengine: hisilicon: Add Kunpeng DMA engine support
Date: Wed, 11 Dec 2019 16:22:34 +0530	[thread overview]
Message-ID: <20191211105234.GG2536@vkoul-mobl> (raw)
In-Reply-To: <1575943997-164744-1-git-send-email-wangzhou1@hisilicon.com>

On 10-12-19, 10:13, Zhou Wang wrote:
> This patch adds a driver for HiSilicon Kunpeng DMA engine.

Can you please describe this controller here, how many channels,
controller capabilities etc

> +static void hisi_dma_enable_dma(struct hisi_dma_dev *hdma_dev, u32 index,
> +				bool enable)

Coding Style expects the second line to be aligned to preceding line
brace opne, --strict option with checkpatch should warn you!

> +{
> +	void __iomem *addr = hdma_dev->base + HISI_DMA_CTRL0(index);
> +	u32 tmp;
> +
> +	tmp = readl_relaxed(addr);
> +	tmp = enable ? tmp | HISI_DMA_CTRL0_QUEUE_EN :
> +		       tmp & ~HISI_DMA_CTRL0_QUEUE_EN;
> +	writel_relaxed(tmp, addr);
> +}

why not create a modifyl() macro and then use that here and other places
rather than doiun read, modify, write sequence

> +static void hisi_dma_reset_hw_chan(struct hisi_dma_chan *chan)
> +{
> +	struct hisi_dma_dev *hdma_dev = chan->hdma_dev;
> +	u32 index = chan->qp_num, tmp;
> +	int ret;
> +
> +	hisi_dma_pause_dma(hdma_dev, index, true);
> +	hisi_dma_enable_dma(hdma_dev, index, false);
> +	hisi_dma_mask_irq(hdma_dev, index);
> +
> +	ret = readl_relaxed_poll_timeout(hdma_dev->base +
> +		HISI_DMA_Q_FSM_STS(index), tmp,
> +		FIELD_GET(HISI_DMA_FSM_STS_MASK, tmp) != RUN, 10, 1000);
> +	if (ret)
> +		dev_err(&hdma_dev->pdev->dev, "disable channel timeout!\n");

you want to continue on this timeout?

> +static int hisi_dma_terminate_all(struct dma_chan *c)
> +{
> +	struct hisi_dma_chan *chan = to_hisi_dma_chan(c);
> +	unsigned long flags;
> +	LIST_HEAD(head);
> +
> +	spin_lock_irqsave(&chan->vc.lock, flags);
> +
> +	hisi_dma_pause_dma(chan->hdma_dev, chan->qp_num, true);
> +	if (chan->desc) {
> +		vchan_terminate_vdesc(&chan->desc->vd);
> +		chan->desc = NULL;
> +	}
> +
> +	vchan_get_all_descriptors(&chan->vc, &head);
> +
> +	spin_unlock_irqrestore(&chan->vc.lock, flags);
> +
> +	vchan_dma_desc_free_list(&chan->vc, &head);
> +	hisi_dma_pause_dma(chan->hdma_dev, chan->qp_num, false);

pause on terminate? Not DISABLE?

> +static struct pci_driver hisi_dma_pci_driver = {
> +	.name		= "hisi_dma",
> +	.id_table	= hisi_dma_pci_tbl,
> +	.probe		= hisi_dma_probe,

no .remove and kconfig has a tristate option!

> +MODULE_AUTHOR("Zhou Wang <wangzhou1@hisilicon.com>");
> +MODULE_AUTHOR("Zhenfa Qiu <qiuzhenfa@hisilicon.com>");
> +MODULE_DESCRIPTION("HiSilicon Kunpeng DMA controller driver");
> +MODULE_LICENSE("GPL");

GPL v2..

-- 
~Vinod