[PATCH v5 2/2] dma: Add Xilinx AXI Central Direct Memory Access Engine driver support

[Appana Durga Kedareswara Rao]

Hi Vinod,

> -----Original Message-----
> From: dmaengine-owner at vger.kernel.org [mailto:dmaengine-
> owner at vger.kernel.org] On Behalf Of Vinod Koul
> Sent: Monday, June 22, 2015 3:45 PM
> To: Appana Durga Kedareswara Rao
> Cc: dan.j.williams at intel.com; Michal Simek; Soren Brinkmann; Appana Durga
> Kedareswara Rao; Anirudha Sarangi; Punnaiah Choudary Kalluri;
> dmaengine at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux-
> kernel at vger.kernel.org; Srikanth Thokala
> Subject: Re: [PATCH v5 2/2] dma: Add Xilinx AXI Central Direct Memory
> Access Engine driver support
>
> On Tue, Jun 09, 2015 at 02:18:10PM +0530, Kedareswara rao Appana wrote:
>
> pls conform to the subsystem naming convention which git log on subsystem
> should have told you that it is dmaengine: xxx......

Ok Sure will take care of it in the next version of the patch.

>
> > This is the driver for the AXI Central Direct Memory Access (AXI
> > CDMA) core, which is a soft Xilinx IP core that provides
> > high-bandwidth Direct Memory Access (DMA) between a memory-
> mapped
> > source address and a memory-mapped destination address.
> >
> > This module works on Zynq (ARM Based SoC) and Microblaze platforms.
> >
> > Signed-off-by: Srikanth Thokala <sthokal at xilinx.com>
> > Signed-off-by: Kedareswara rao Appana <appanad at xilinx.com>
> > ---
> > This patch is rebased on the commit
> > Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net
> why a random, unrelated commit? and no mention of tree..

Ok will fix in the next iteration of the patch.

>
> > +static struct dma_async_tx_descriptor *
> > +xilinx_cdma_prep_memcpy(struct dma_chan *dchan, dma_addr_t
> dma_dst,
> > +                   dma_addr_t dma_src, size_t len, unsigned long flags)
> {
> > +   struct xilinx_cdma_chan *chan = to_xilinx_chan(dchan);
> > +   struct xilinx_cdma_desc_hw *hw;
> > +   struct xilinx_cdma_tx_descriptor *desc;
> > +   struct xilinx_cdma_tx_segment *segment, *prev;
> > +
> > +   if (!len || len > XILINX_CDMA_MAX_TRANS_LEN)
> > +           return NULL;
> > +
> > +   desc = xilinx_cdma_alloc_tx_descriptor(chan);
> > +   if (!desc)
> > +           return NULL;
> > +
> > +   dma_async_tx_descriptor_init(&desc->async_tx, &chan->common);
> > +   desc->async_tx.tx_submit = xilinx_cdma_tx_submit;
> > +   async_tx_ack(&desc->async_tx);
> and why are you doing this ?

Ok will remove.

>
> > +int xilinx_cdma_channel_set_config(struct dma_chan *dchan,
> > +                                   struct xilinx_cdma_config *cfg)
> > +{
> > +   struct xilinx_cdma_chan *chan = to_xilinx_chan(dchan);
> > +   u32 reg = cdma_read(chan, XILINX_CDMA_CONTROL_OFFSET);
> > +
> > +   if (!xilinx_cdma_is_idle(chan))
> > +           return -EBUSY;
> Why cant it be like dma_slave_config and take effect on next descriptor
> prepare?

Will fix this part.

>
> > +
> > +   if (cfg->reset)
> > +           return xilinx_cdma_chan_reset(chan);
> Why do you want to reset this externally, that sounds bad to me

If someone (client driver) want to reset the controller externally. It will be useful right?


>
> > +
> > +   if (cfg->coalesc <= XILINX_CDMA_COALESCE_MAX) {
> > +           reg &= ~XILINX_CDMA_XR_COALESCE_MASK;
> > +           reg |= cfg->coalesc << XILINX_CDMA_COALESCE_SHIFT;
> > +   }
> Can you explain what coalesc means here?

Coalesc means interrupt threshold
This value is used for setting the interrupt threshold. When IOC (interrupt on complete) interrupt events occur, an internal counter
Counts down from the Interrupt Threshold setting. When the count reaches zero, an interrupt out is generated by the DMA engine.
This will be useful in case of SG transfer.

>
> > +
> > +   if (cfg->delay <= XILINX_CDMA_DELAY_MAX) {
> > +           reg &= ~XILINX_CDMA_XR_DELAY_MASK;
> > +           reg |= cfg->delay << XILINX_CDMA_DELAY_SHIFT;
> > +   }
> > +
> > +   cdma_write(chan, XILINX_CDMA_CONTROL_OFFSET, reg);
> Shouldn't you save it apply for a descriptor?

Currently I am not making efficient use of it will use take care of it.

> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL(xilinx_cdma_channel_set_config);
> not EXPORT_SYMBOL_GPL?

Ok will modify.

>
>
> > +static int xilinx_cdma_chan_probe(struct xilinx_cdma_device *xdev,
> > +                                   struct device_node *node)
> > +{
> > +   struct xilinx_cdma_chan *chan;
> > +   bool has_dre;
> > +   u32 value, width;
> > +   int err;
> > +
> > +   /* Alloc channel */
> > +   chan = devm_kzalloc(xdev->dev, sizeof(*chan), GFP_NOWAIT);
> Why do you use GFP_NOWAIT in probe?

Ok will fix in the next version of patch.

Thanks for the review.

Regards,
Kedar.

>
> --
> ~Vinod
>
> --
> To unsubscribe from this list: send the line "unsubscribe dmaengine" in


This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.