dmaengine.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Vinod Koul <vkoul@kernel.org>
To: Keguang Zhang <keguang.zhang@gmail.com>
Cc: dmaengine@vger.kernel.org, linux-mips@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH V4 RESEND] dmaengine: Loongson1: Add Loongson1 dmaengine driver
Date: Mon, 7 Jun 2021 16:37:07 +0530	[thread overview]
Message-ID: <YL392y4a6iRf1UyQ@vkoul-mobl> (raw)
In-Reply-To: <20210520230225.11911-1-keguang.zhang@gmail.com>

On 21-05-21, 07:02, Keguang Zhang wrote:

> +config LOONGSON1_DMA
> +	tristate "Loongson1 DMA support"
> +	depends on MACH_LOONGSON32

Why does it have to do that? The dma driver is generic..

> +static int ls1x_dma_alloc_chan_resources(struct dma_chan *dchan)
> +{
> +	struct ls1x_dma_chan *chan = to_ls1x_dma_chan(dchan);
> +
> +	chan->desc_pool = dma_pool_create(dma_chan_name(dchan),
> +					  dchan->device->dev,
> +					  sizeof(struct ls1x_dma_lli),
> +					  __alignof__(struct ls1x_dma_lli), 0);
> +	if (!chan->desc_pool) {
> +		dev_err(chan2dev(dchan),
> +			"failed to alloc DMA descriptor pool!\n");

This can be dropped, allocators will warn you for the allocation
failures

> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static void ls1x_dma_free_desc(struct virt_dma_desc *vdesc)
> +{
> +	struct ls1x_dma_desc *desc = to_ls1x_dma_desc(vdesc);
> +
> +	if (desc->nr_descs) {
> +		unsigned int i = desc->nr_descs;
> +		struct ls1x_dma_hwdesc *hwdesc;
> +
> +		do {
> +			hwdesc = &desc->hwdesc[--i];
> +			dma_pool_free(desc->chan->desc_pool, hwdesc->lli,
> +				      hwdesc->phys);
> +		} while (i);
> +	}
> +
> +	kfree(desc);
> +}
> +
> +static struct ls1x_dma_desc *ls1x_dma_alloc_desc(struct ls1x_dma_chan *chan,
> +						 int sg_len)

single line now :)

> +{
> +	struct ls1x_dma_desc *desc;
> +	struct dma_chan *dchan = &chan->vchan.chan;
> +
> +	desc = kzalloc(struct_size(desc, hwdesc, sg_len), GFP_NOWAIT);
> +	if (!desc)
> +		dev_err(chan2dev(dchan), "failed to alloc DMA descriptor!\n");

this can be dropped too..

> +static struct dma_async_tx_descriptor *
> +ls1x_dma_prep_slave_sg(struct dma_chan *dchan, struct scatterlist *sgl,
> +		       unsigned int sg_len,
> +		       enum dma_transfer_direction direction,
> +		       unsigned long flags, void *context)
> +{
> +	struct ls1x_dma_chan *chan = to_ls1x_dma_chan(dchan);
> +	struct dma_slave_config *cfg = &chan->cfg;
> +	struct ls1x_dma_desc *desc;
> +	struct scatterlist *sg;
> +	unsigned int dev_addr, bus_width, cmd, i;
> +
> +	if (!is_slave_direction(direction)) {
> +		dev_err(chan2dev(dchan), "invalid DMA direction!\n");
> +		return NULL;
> +	}
> +
> +	dev_dbg(chan2dev(dchan), "sg_len=%d, dir=%s, flags=0x%lx\n", sg_len,
> +		direction == DMA_MEM_TO_DEV ? "to device" : "from device",
> +		flags);
> +
> +	switch (direction) {
> +	case DMA_MEM_TO_DEV:
> +		dev_addr = cfg->dst_addr;
> +		bus_width = cfg->dst_addr_width;
> +		cmd = LS1X_DMA_RAM2DEV | LS1X_DMA_INT;
> +		break;
> +	case DMA_DEV_TO_MEM:
> +		dev_addr = cfg->src_addr;
> +		bus_width = cfg->src_addr_width;
> +		cmd = LS1X_DMA_INT;
> +		break;
> +	default:
> +		dev_err(chan2dev(dchan),
> +			"unsupported DMA transfer mode! %d\n", direction);
> +		return NULL;

will this be ever executed?

> +static int ls1x_dma_slave_config(struct dma_chan *dchan,
> +				 struct dma_slave_config *config)
> +{
> +	struct ls1x_dma_chan *chan = to_ls1x_dma_chan(dchan);
> +
> +	if (!dchan)
> +		return -EINVAL;

should this not be checked before you dereference this to get chan

> +static void ls1x_dma_trigger(struct ls1x_dma_chan *chan)
> +{
> +	struct dma_chan *dchan = &chan->vchan.chan;
> +	struct ls1x_dma_desc *desc;
> +	struct virt_dma_desc *vdesc;
> +	unsigned int val;
> +
> +	vdesc = vchan_next_desc(&chan->vchan);
> +	if (!vdesc) {
> +		dev_warn(chan2dev(dchan), "No pending descriptor\n");

Hmm, I would not log that... this is called from
ls1x_dma_issue_pending() and which can be called from client driver but
previous completion would push and this can find empty queue so it can
happen quite frequently

> +static irqreturn_t ls1x_dma_irq_handler(int irq, void *data)
> +{
> +	struct ls1x_dma_chan *chan = data;
> +	struct dma_chan *dchan = &chan->vchan.chan;
> +
> +	dev_dbg(chan2dev(dchan), "DMA IRQ %d on channel %d\n", irq, chan->id);
> +	if (!chan->desc) {
> +		dev_warn(chan2dev(dchan),
> +			 "DMA IRQ with no active descriptor on channel %d\n",
> +			 chan->id);

single line pls

> +		return IRQ_NONE;
> +	}
> +
> +	spin_lock(&chan->vchan.lock);
> +
> +	if (chan->desc->type == DMA_CYCLIC) {
> +		vchan_cyclic_callback(&chan->desc->vdesc);
> +	} else {
> +		list_del(&chan->desc->vdesc.node);
> +		vchan_cookie_complete(&chan->desc->vdesc);
> +		chan->desc = NULL;
> +	}

not submitting next txn, defeats the purpose of dma if we dont push txns
as fast as possible..

> +static struct platform_driver ls1x_dma_driver = {
> +	.probe	= ls1x_dma_probe,
> +	.remove	= ls1x_dma_remove,
> +	.driver	= {
> +		.name	= "ls1x-dma",
> +	},

No device tree?

-- 
~Vinod

  reply	other threads:[~2021-06-07 11:07 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-20 23:02 [PATCH V4 RESEND] dmaengine: Loongson1: Add Loongson1 dmaengine driver Keguang Zhang
2021-06-07 11:07 ` Vinod Koul [this message]
2021-07-04 14:45   ` Kelvin Cheung
2021-07-05  3:49     ` Vinod Koul

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YL392y4a6iRf1UyQ@vkoul-mobl \
    --to=vkoul@kernel.org \
    --cc=dmaengine@vger.kernel.org \
    --cc=keguang.zhang@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mips@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).