From: Vinod Koul <vkoul@kernel.org>
To: Green Wan <green.wan@sifive.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Linus Walleij <linus.walleij@linaro.org>,
Palmer Dabbelt <palmer@sifive.com>,
Nicolas Ferre <nicolas.ferre@microchip.com>,
linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
dmaengine@vger.kernel.org,
Paul Walmsley <paul.walmsley@sifive.com>,
Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
linux-hackers@sifive.com, Dan Williams <dan.j.williams@intel.com>,
"Paul E. McKenney" <paulmck@linux.ibm.com>,
"David S. Miller" <davem@davemloft.net>
Subject: Re: [PATCH v3 3/3] dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00
Date: Tue, 24 Sep 2019 14:20:11 -0700 [thread overview]
Message-ID: <20190924212011.GG3824@vkoul-mobl> (raw)
In-Reply-To: <20190920090205.19552-1-green.wan@sifive.com>
Hi Green,
On 20-09-19, 17:01, Green Wan wrote:
Please make sure threading is *not* broken in your patch series. Atm
they are all over place in my mailbox!
> Link: https://www.kernel.org/doc/html/v4.17/driver-api/dmaengine/
> Link: https://static.dev.sifive.com/FU540-C000-v1.0.pdf
Link tag is used for discussion for the patch, please drop first one and
add second one as a documentation for hardware
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d0caa09a479e..c5f0662c9106 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -14594,6 +14594,7 @@ F: drivers/media/mmc/siano/
> SIFIVE PDMA DRIVER
> M: Green Wan <green.wan@sifive.com>
> S: Maintained
> +F: drivers/dma/sf-pdma/
> F: Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
What is this generated against, only one line?
> +static void sf_pdma_fill_desc(struct sf_pdma_chan *chan,
> + u64 dst,
> + u64 src,
> + u64 size)
Please align these to precceeding line open brace!
> +{
> + struct pdma_regs *regs = &chan->regs;
> +
> + writel(PDMA_FULL_SPEED, regs->xfer_type);
> + writeq(size, regs->xfer_size);
> + writeq(dst, regs->dst_addr);
> + writeq(src, regs->src_addr);
> +}
> +
> +void sf_pdma_disclaim_chan(struct sf_pdma_chan *chan)
> +{
> + struct pdma_regs *regs = &chan->regs;
> +
> + writel(PDMA_CLEAR_CTRL, regs->ctrl);
> +}
> +
> +struct dma_async_tx_descriptor *
> + sf_pdma_prep_dma_memcpy(struct dma_chan *dchan,
> + dma_addr_t dest,
> + dma_addr_t src,
> + size_t len,
> + unsigned long flags)
> +{
> + struct sf_pdma_chan *chan = to_sf_pdma_chan(dchan);
> + struct sf_pdma_desc *desc;
> +
> + if (!chan || !len || !dest || !src) {
> + pr_debug("%s: Please check dma len, dest, src!\n", __func__);
> + return NULL;
> + }
> +
> + desc = sf_pdma_alloc_desc(chan);
> + if (!desc)
> + return NULL;
> +
> + desc->in_use = true;
> + desc->dirn = DMA_MEM_TO_MEM;
> + desc->async_tx = vchan_tx_prep(&chan->vchan, &desc->vdesc, flags);
No error checking?
> +
> + spin_lock_irqsave(&chan->lock, flags);
> + chan->desc = desc;
> + sf_pdma_fill_desc(desc->chan, dest, src, len);
> + spin_unlock_irqrestore(&chan->lock, flags);
> +
> + return desc->async_tx;
> +}
> +
> +static void sf_pdma_unprep_slave_dma(struct sf_pdma_chan *chan)
> +{
> + if (chan->dma_dir != DMA_NONE)
> + dma_unmap_resource(chan->vchan.chan.device->dev,
This is slave dma right, why are you unmapping? Also where is the
mapping call?
> + chan->dma_dev_addr,
> + chan->dma_dev_size,
> + chan->dma_dir, 0);
> + chan->dma_dir = DMA_NONE;
> +}
> +
> +static int sf_pdma_slave_config(struct dma_chan *dchan,
> + struct dma_slave_config *cfg)
> +{
> + struct sf_pdma_chan *chan = to_sf_pdma_chan(dchan);
> +
> + memcpy(&chan->cfg, cfg, sizeof(*cfg));
> + sf_pdma_unprep_slave_dma(chan);
Why unprep?
> +static enum dma_status
> +sf_pdma_tx_status(struct dma_chan *dchan,
> + dma_cookie_t cookie,
> + struct dma_tx_state *txstate)
> +{
> + struct sf_pdma_chan *chan = to_sf_pdma_chan(dchan);
> + enum dma_status status;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&chan->lock, flags);
> + if (chan->xfer_err) {
> + chan->status = DMA_ERROR;
> + spin_unlock_irqrestore(&chan->lock, flags);
> + return chan->status;
> + }
> +
> + spin_unlock_irqrestore(&chan->lock, flags);
> +
> + status = dma_cookie_status(dchan, cookie, txstate);
> +
> + if (status == DMA_COMPLETE)
> + return status;
> +
> + if (!txstate)
> + return chan->status;
why not return status? Is that expected to be different than status?
> +static int sf_pdma_remove(struct platform_device *pdev)
> +{
> + struct sf_pdma *pdma = platform_get_drvdata(pdev);
> +
> + dma_async_device_unregister(&pdma->dma_dev);
whay about irqs and tasklets, they are still enabled and can trigger!
--
~Vinod
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2019-09-24 21:21 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-20 9:01 [PATCH v3 3/3] dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00 Green Wan
2019-09-24 21:20 ` Vinod Koul [this message]
[not found] ` <CAJivOr4qZ7s20cME5=Fdw6G2-2JQGjO2ZT-ar2oHk3aad0R1gg@mail.gmail.com>
2019-09-26 17:26 ` 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=20190924212011.GG3824@vkoul-mobl \
--to=vkoul@kernel.org \
--cc=dan.j.williams@intel.com \
--cc=davem@davemloft.net \
--cc=dmaengine@vger.kernel.org \
--cc=green.wan@sifive.com \
--cc=gregkh@linuxfoundation.org \
--cc=linus.walleij@linaro.org \
--cc=linux-hackers@sifive.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=mchehab+samsung@kernel.org \
--cc=nicolas.ferre@microchip.com \
--cc=palmer@sifive.com \
--cc=paul.walmsley@sifive.com \
--cc=paulmck@linux.ibm.com \
/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).