From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754532AbbGPMeL (ORCPT ); Thu, 16 Jul 2015 08:34:11 -0400 Received: from mga11.intel.com ([192.55.52.93]:34319 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751704AbbGPMeI (ORCPT ); Thu, 16 Jul 2015 08:34:08 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,488,1432623600"; d="scan'208";a="765615946" Date: Thu, 16 Jul 2015 18:05:48 +0530 From: Vinod Koul To: Punnaiah Choudary Kalluri Cc: robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org, michal.simek@xilinx.com, soren.brinkmann@xilinx.com, dan.j.williams@intel.com, dmaengine@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, kalluripunnaiahchoudary@gmail.com, kpc528@gmail.com, Punnaiah Choudary Kalluri Subject: Re: [PATCH v3 2/2] dma: Add Xilinx zynqmp dma engine driver support Message-ID: <20150716123548.GO5086@localhost> References: <1434422083-8653-1-git-send-email-punnaia@xilinx.com> <1434422083-8653-2-git-send-email-punnaia@xilinx.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1434422083-8653-2-git-send-email-punnaia@xilinx.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jun 16, 2015 at 08:04:43AM +0530, Punnaiah Choudary Kalluri wrote: > +/* Register Offsets */ > +#define ISR 0x100 > +#define IMR 0x104 > +#define IER 0x108 > +#define IDS 0x10C > +#define CTRL0 0x110 > +#define CTRL1 0x114 > +#define STATUS 0x11C > +#define DATA_ATTR 0x120 > +#define DSCR_ATTR 0x124 > +#define SRC_DSCR_WRD0 0x128 > +#define SRC_DSCR_WRD1 0x12C > +#define SRC_DSCR_WRD2 0x130 > +#define SRC_DSCR_WRD3 0x134 > +#define DST_DSCR_WRD0 0x138 > +#define DST_DSCR_WRD1 0x13C > +#define DST_DSCR_WRD2 0x140 > +#define DST_DSCR_WRD3 0x144 > +#define SRC_START_LSB 0x158 > +#define SRC_START_MSB 0x15C > +#define DST_START_LSB 0x160 > +#define DST_START_MSB 0x164 > +#define TOTAL_BYTE 0x188 > +#define RATE_CTRL 0x18C > +#define IRQ_SRC_ACCT 0x190 > +#define IRQ_DST_ACCT 0x194 > +#define CTRL2 0x200 Can you namespace these and other defines > +struct zynqmp_dma_chan { > + struct zynqmp_dma_device *xdev; xdev seems an odd name, i suspect copy paste error! > + void __iomem *regs; > + spinlock_t lock; > + struct list_head pending_list; > + struct list_head free_list; > + struct list_head active_list; > + struct zynqmp_dma_desc_sw *sw_desc_pool; > + struct list_head done_list; > + struct dma_chan common; > + void *desc_pool_v; > + dma_addr_t desc_pool_p; why two pools and one void *? > +/** > + * zynqmp_dma_alloc_chan_resources - Allocate channel resources > + * @dchan: DMA channel > + * > + * Return: '0' on success and failure value on error wrong, its number of descriptors which maybe 0 > + */ > +static int zynqmp_dma_alloc_chan_resources(struct dma_chan *dchan) > +{ > + struct zynqmp_dma_chan *chan = to_chan(dchan); > + struct zynqmp_dma_desc_sw *desc; > + int i; > + > + chan->sw_desc_pool = kzalloc(sizeof(*desc) * ZYNQMP_DMA_NUM_DESCS, > + GFP_KERNEL); and you are allocating the number so pls return that Btw is ZYNQMP_DMA_NUM_DESCS sw or HW limit > +static void zynqmp_dma_start_transfer(struct zynqmp_dma_chan *chan) > +{ > + struct zynqmp_dma_desc_sw *desc; > + > + if (!zynqmp_dma_chan_is_idle(chan)) > + return; > + > + desc = list_first_entry_or_null(&chan->pending_list, > + struct zynqmp_dma_desc_sw, node); > + if (!desc) > + return; > + > + if (chan->has_sg) > + list_splice_tail_init(&chan->pending_list, &chan->active_list); > + else > + list_move_tail(&desc->node, &chan->active_list); > + > + if (chan->has_sg) > + zynqmp_dma_update_desc_to_ctrlr(chan, desc); > + else > + zynqmp_dma_config_simple_desc(chan, desc->src, desc->dst, > + desc->len); > + zynqmp_dma_start(chan); > +} Lots of the list management will get simplified if you use the vchan to do so > + > + > +/** > + * zynqmp_dma_chan_desc_cleanup - Cleanup the completed descriptors > + * @chan: ZynqMP DMA channel > + */ > +static void zynqmp_dma_chan_desc_cleanup(struct zynqmp_dma_chan *chan) > +{ > + struct zynqmp_dma_desc_sw *desc, *next; > + > + list_for_each_entry_safe(desc, next, &chan->done_list, node) { > + dma_async_tx_callback callback; > + void *callback_param; > + > + list_del(&desc->node); > + > + callback = desc->async_tx.callback; > + callback_param = desc->async_tx.callback_param; > + if (callback) > + callback(callback_param); > + and you are calling the callback with lock held and user can do further submissions so this is wrong! > +static enum dma_status zynqmp_dma_tx_status(struct dma_chan *dchan, > + dma_cookie_t cookie, > + struct dma_tx_state *txstate) > +{ > + return dma_cookie_status(dchan, cookie, txstate); no residue calculation? > +static struct dma_async_tx_descriptor *zynqmp_dma_prep_memcpy( > + struct dma_chan *dchan, dma_addr_t dma_dst, > + dma_addr_t dma_src, size_t len, ulong flags) > +{ > + struct zynqmp_dma_chan *chan; > + struct zynqmp_dma_desc_sw *new, *first = NULL; > + void *desc = NULL, *prev = NULL; > + size_t copy; > + u32 desc_cnt; > + > + chan = to_chan(dchan); > + > + if ((len > ZYNQMP_DMA_MAX_TRANS_LEN) && !chan->has_sg) why sg? > +static int zynqmp_dma_remove(struct platform_device *pdev) > +{ > + struct zynqmp_dma_device *xdev = platform_get_drvdata(pdev); > + > + of_dma_controller_free(pdev->dev.of_node); > + dma_async_device_unregister(&xdev->common); > + > + zynqmp_dma_chan_remove(xdev->chan); Please free up irq here explictly and also ensure tasklet is not running -- ~Vinod