From: Vinod Koul <vkoul@kernel.org>
To: Alexander Gordeev <a.gordeev.box@gmail.com>
Cc: devel@driverdev.osuosl.org, Michael Chen <micchen@altera.com>,
dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/2] dmaengine: avalon: Intel Avalon-MM DMA Interface for PCIe
Date: Tue, 15 Oct 2019 16:03:21 +0530 [thread overview]
Message-ID: <20191015103321.GU2654@vkoul-mobl> (raw)
In-Reply-To: <3ed3c016b7fbe69e36023e7ee09c53acac8a064c.1570558807.git.a.gordeev.box@gmail.com>
On 09-10-19, 12:12, Alexander Gordeev wrote:
> +config AVALON_DMA_CTRL_BASE
> + hex "Avalon DMA controllers base"
> + default "0x00000000"
what kind of device is this? I dont think we want these and the ones
coming below as part of kernel kconfig!
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Avalon DMA engine
> + *
> + * Author: Alexander Gordeev <a.gordeev.box@gmail.com>
No copyright notices?
> +static int setup_dma_descs(struct dma_desc *dma_descs,
> + struct avalon_dma_desc *desc)
> +{
> + struct scatterlist *sg_stop;
> + unsigned int sg_set;
> + int ret;
> +
> + ret = setup_descs_sg(dma_descs, 0,
> + desc->direction,
> + desc->dev_addr,
> + desc->sg, desc->sg_len,
> + desc->sg_curr, desc->sg_offset,
> + &sg_stop, &sg_set);
> + BUG_ON(!ret);
Please remove this and others
> +static int start_dma_xfer(struct avalon_dma_hw *hw,
> + struct avalon_dma_desc *desc)
> +{
> + size_t ctrl_off;
> + struct __dma_desc_table *__table;
> + struct dma_desc_table *table;
> + u32 rc_src_hi, rc_src_lo;
> + u32 ep_dst_lo, ep_dst_hi;
> + int last_id, *__last_id;
> + int nr_descs;
> +
> + if (desc->direction == DMA_TO_DEVICE) {
> + __table = &hw->dma_desc_table_rd;
> +
> + ctrl_off = AVALON_DMA_RD_CTRL_OFFSET;
> +
> + ep_dst_hi = AVALON_DMA_RD_EP_DST_HI;
> + ep_dst_lo = AVALON_DMA_RD_EP_DST_LO;
> +
> + __last_id = &hw->h2d_last_id;
> + } else if (desc->direction == DMA_FROM_DEVICE) {
> + __table = &hw->dma_desc_table_wr;
> +
> + ctrl_off = AVALON_DMA_WR_CTRL_OFFSET;
> +
> + ep_dst_hi = AVALON_DMA_WR_EP_DST_HI;
> + ep_dst_lo = AVALON_DMA_WR_EP_DST_LO;
> +
> + __last_id = &hw->d2h_last_id;
> + } else {
> + BUG();
> + }
> +
> + table = __table->cpu_addr;
> + memset(&table->flags, 0, sizeof(table->flags));
> +
> + nr_descs = setup_dma_descs(table->descs, desc);
> + if (WARN_ON(nr_descs < 1))
> + return nr_descs;
> +
> + last_id = nr_descs - 1;
> + *__last_id = last_id;
> +
> + rc_src_hi = __table->dma_addr >> 32;
> + rc_src_lo = (u32)__table->dma_addr;
> +
> + start_xfer(hw->regs, ctrl_off,
> + rc_src_hi, rc_src_lo,
> + ep_dst_hi, ep_dst_lo,
> + last_id);
> +
> + return 0;
why have a return when you always return 0?
> +static irqreturn_t avalon_dma_interrupt(int irq, void *dev_id)
> +{
> + struct avalon_dma *adma = (struct avalon_dma *)dev_id;
> + struct avalon_dma_chan *chan = &adma->chan;
> + struct avalon_dma_hw *hw = &chan->hw;
> + spinlock_t *lock = &chan->vchan.lock;
> + u32 *rd_flags = hw->dma_desc_table_rd.cpu_addr->flags;
> + u32 *wr_flags = hw->dma_desc_table_wr.cpu_addr->flags;
> + struct avalon_dma_desc *desc;
> + struct virt_dma_desc *vdesc;
> + bool rd_done;
> + bool wr_done;
> +
> + spin_lock(lock);
> +
> + rd_done = (hw->h2d_last_id < 0);
> + wr_done = (hw->d2h_last_id < 0);
> +
> + if (rd_done && wr_done) {
> + spin_unlock(lock);
> + return IRQ_NONE;
> + }
> +
> + do {
> + if (!rd_done && rd_flags[hw->h2d_last_id])
> + rd_done = true;
> +
> + if (!wr_done && wr_flags[hw->d2h_last_id])
> + wr_done = true;
> + } while (!rd_done || !wr_done);
Can you explain this logic. I dont like busy loop in isr and who updates
rd_done and rd_flags etc..?
> +static struct dma_async_tx_descriptor *
> +avalon_dma_prep_slave_sg(struct dma_chan *dma_chan,
> + struct scatterlist *sg, unsigned int sg_len,
> + enum dma_transfer_direction direction,
> + unsigned long flags, void *context)
> +{
> + struct avalon_dma_chan *chan = to_avalon_dma_chan(dma_chan);
> + struct avalon_dma_desc *desc;
> + gfp_t gfp_flags = in_interrupt() ? GFP_NOWAIT : GFP_KERNEL;
We use GFP_NOWAIT in prep calls. They can be called from atomic context
> +static int __init avalon_pci_probe(struct pci_dev *pci_dev,
> + const struct pci_device_id *id)
> +{
> + void *adma;
> + void __iomem *regs;
> + int ret;
> +
> + ret = pci_enable_device(pci_dev);
> + if (ret)
> + goto enable_err;
> +
> + ret = pci_request_regions(pci_dev, DRIVER_NAME);
> + if (ret)
> + goto reg_err;
> +
> + regs = pci_ioremap_bar(pci_dev, PCI_BAR);
This is a pci device, so we should really be using the PCI way of
setting and querying the resources. Doing this thru kernel config
options is not correct!
> +static int __init avalon_drv_init(void)
> +{
> + return pci_register_driver(&dma_driver_ops);
> +}
> +
> +static void __exit avalon_drv_exit(void)
> +{
> + pci_unregister_driver(&dma_driver_ops);
> +}
> +
> +module_init(avalon_drv_init);
> +module_exit(avalon_drv_exit);
pls use module_pci_driver.
--
~Vinod
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
next prev parent reply other threads:[~2019-10-15 10:33 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-09 10:12 [PATCH v2 0/2] dmaengine: avalon: Support Avalon-MM DMA Interface for PCIe Alexander Gordeev
2019-10-09 10:12 ` [PATCH v2 1/2] dmaengine: avalon: Intel " Alexander Gordeev
2019-10-09 12:14 ` Dan Carpenter
2019-10-09 14:58 ` Alexander Gordeev
2019-10-09 18:53 ` Dan Carpenter
2019-10-10 8:51 ` Alexander Gordeev
2019-10-10 11:30 ` Dan Carpenter
2019-10-15 11:24 ` Alexander Gordeev
2019-10-15 11:41 ` Dan Carpenter
2019-10-15 12:27 ` Alexander Gordeev
2019-10-15 13:19 ` Dan Carpenter
2019-10-15 14:31 ` Alexander Gordeev
2019-10-15 14:47 ` Dan Carpenter
2019-10-09 13:07 ` Greg KH
2019-10-15 10:33 ` Vinod Koul [this message]
2019-10-15 13:11 ` Alexander Gordeev
2019-10-09 10:12 ` [PATCH RFC v2 2/2] dmaengine: avalon: Intel Avalon-MM DMA Interface for PCIe test Alexander Gordeev
2019-10-09 13:08 ` Greg KH
2019-10-09 13:46 ` Dan Carpenter
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=20191015103321.GU2654@vkoul-mobl \
--to=vkoul@kernel.org \
--cc=a.gordeev.box@gmail.com \
--cc=devel@driverdev.osuosl.org \
--cc=dmaengine@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=micchen@altera.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).