From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751433AbdBJGHC (ORCPT ); Fri, 10 Feb 2017 01:07:02 -0500 Received: from mga09.intel.com ([134.134.136.24]:29723 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751023AbdBJGG6 (ORCPT ); Fri, 10 Feb 2017 01:06:58 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.35,139,1484035200"; d="scan'208";a="932249554" Date: Fri, 10 Feb 2017 11:36:06 +0530 From: Vinod Koul To: Eugeniy Paltsev Cc: dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-snps-arc@lists.infradead.org, Dan Williams , Mark Rutland , Rob Herring , Andy Shevchenko , Alexey Brodkin Subject: Re: [PATCH 2/2] dmaengine: Add DW AXI DMAC driver Message-ID: <20170210060606.GO19244@localhost> References: <1485358457-22957-1-git-send-email-Eugeniy.Paltsev@synopsys.com> <1485358457-22957-3-git-send-email-Eugeniy.Paltsev@synopsys.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1485358457-22957-3-git-send-email-Eugeniy.Paltsev@synopsys.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 Wed, Jan 25, 2017 at 06:34:17PM +0300, Eugeniy Paltsev wrote: > This patch adds support for the DW AXI DMAC controller. > > DW AXI DMAC is a part of upcoming development board from Synopsys. How different is AXI from DW Synopsys? Is the spec publicly available? > +config AXI_DW_DMAC > + tristate "Synopsys DesignWare AXI DMA support" > + depends on OF && !64BIT why not 64 bit, can you also add compile test > +#define AXI_DMA_BUSWIDTHS \ > + (DMA_SLAVE_BUSWIDTH_UNDEFINED | \ DMA_SLAVE_BUSWIDTH_UNDEFINED?? > +static irqreturn_t dw_axi_dma_intretupt(int irq, void *dev_id) > +{ > + struct axi_dma_chip *chip = dev_id; > + > + /* Disable DMAC inerrupts. We'll enable them in the tasklet */ > + axi_dma_irq_disable(chip); > + > + tasklet_schedule(&chip->dw->tasklet); This is very inefficient, we would want to submit next txn here and not in tasklet > +static void axi_chan_block_xfer_complete(struct axi_dma_chan *chan) > +{ > + struct virt_dma_desc *vd; > + unsigned long flags; > + > + spin_lock_irqsave(&chan->vc.lock, flags); > + if (unlikely(axi_chan_is_hw_enable(chan))) { > + dev_err(chan2dev(chan), "BUG: %s catched DWAXIDMAC_IRQ_DMA_TRF, but channel not idle!\n", > + axi_chan_name(chan)); > + axi_chan_disable(chan); > + } > + > + /* The completed descriptor currently is in the head of vc list */ > + vd = vchan_next_desc(&chan->vc); > + /* Remove the completed descriptor from issued list before completing */ > + list_del(&vd->node); > + vchan_cookie_complete(vd); this should be called from irq handler > +static int dw_remove(struct platform_device *pdev) > +{ > + struct axi_dma_chip *chip = platform_get_drvdata(pdev); > + struct dw_axi_dma *dw = chip->dw; > + struct axi_dma_chan *chan, *_chan; > + u32 i; > + > + axi_dma_irq_disable(chip); > + for (i = 0; i < dw->hdata->nr_channels; i++) { > + axi_chan_disable(&chip->dw->chan[i]); > + axi_chan_irq_disable(&chip->dw->chan[i], DWAXIDMAC_IRQ_ALL); > + } > + axi_dma_disable(chip); > + > + tasklet_kill(&dw->tasklet); > + > + list_for_each_entry_safe(chan, _chan, &dw->dma.channels, > + vc.chan.device_node) { > + list_del(&chan->vc.chan.device_node); > + tasklet_kill(&chan->vc.task); > + } very nice :) But please freeup irq as well > +module_platform_driver(dw_driver); > + > +static int __init dw_init(void) > +{ > + return platform_driver_register(&dw_driver); > +} > +subsys_initcall(dw_init); why subsys_initcall?? > +/* Common registers offset */ > +#define DMAC_ID 0x000 // R DMAC ID > +#define DMAC_COMPVER 0x008 // R DMAC Component Version > +#define DMAC_CFG 0x010 // R/W DMAC Configuration > +#define DMAC_CHEN 0x018 // R/W DMAC Channel Enable > +#define DMAC_CHEN_L 0x018 // R/W DMAC Channel Enable 00-31 > +#define DMAC_CHEN_H 0x01C // R/W DMAC Channel Enable 32-63 > +#define DMAC_INTSTATUS 0x030 // R DMAC Interrupt Status > +#define DMAC_COMMON_INTCLEAR 0x038 // W DMAC Interrupt Clear > +#define DMAC_COMMON_INTSTATUS_ENA 0x040 // R DMAC Interrupt Status Enable > +#define DMAC_COMMON_INTSIGNAL_ENA 0x048 // R/W DMAC Interrupt Signal Enable > +#define DMAC_COMMON_INTSTATUS 0x050 // R DMAC Interrupt Status > +#define DMAC_RESET 0x058 // R DMAC Reset Register1 DMAC is a generic term, AX_DMAC and no C99 style comment, checkpatch would have cribbed Use BITS and GENMASK here > + > +/* DMA channel registers offset */ > +#define CH_SAR 0x000 // R/W Chan Source Address > +#define CH_DAR 0x008 // R/W Chan Destination Address > +#define CH_BLOCK_TS 0x010 // R/W Chan Block Transfer Size > +#define CH_CTL 0x018 // R/W Chan Control > +#define CH_CTL_L 0x018 // R/W Chan Control 00-31 > +#define CH_CTL_H 0x01C // R/W Chan Control 32-63 > +#define CH_CFG 0x020 // R/W Chan Configuration > +#define CH_CFG_L 0x020 // R/W Chan Configuration 00-31 > +#define CH_CFG_H 0x024 // R/W Chan Configuration 32-63 > +#define CH_LLP 0x028 // R/W Chan Linked List Pointer > +#define CH_STATUS 0x030 // R Chan Status > +#define CH_SWHSSRC 0x038 // R/W Chan SW Handshake Source > +#define CH_SWHSDST 0x040 // R/W Chan SW Handshake Destination > +#define CH_BLK_TFR_RESUMEREQ 0x048 // W Chan Block Transfer Resume Req > +#define CH_AXI_ID 0x050 // R/W Chan AXI ID > +#define CH_AXI_QOS 0x058 // R/W Chan AXI QOS > +#define CH_SSTAT 0x060 // R Chan Source Status > +#define CH_DSTAT 0x068 // R Chan Destination Status > +#define CH_SSTATAR 0x070 // R/W Chan Source Status Fetch Addr > +#define CH_DSTATAR 0x078 // R/W Chan Destination Status Fetch Addr > +#define CH_INTSTATUS_ENA 0x080 // R/W Chan Interrupt Status Enable > +#define CH_INTSTATUS 0x088 // R/W Chan Interrupt Status > +#define CH_INTSIGNAL_ENA 0x090 // R/W Chan Interrupt Signal Enable > +#define CH_INTCLEAR 0x098 // W Chan Interrupt Clear Same here -- ~Vinod From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vinod Koul Subject: Re: [PATCH 2/2] dmaengine: Add DW AXI DMAC driver Date: Fri, 10 Feb 2017 11:36:06 +0530 Message-ID: <20170210060606.GO19244@localhost> References: <1485358457-22957-1-git-send-email-Eugeniy.Paltsev@synopsys.com> <1485358457-22957-3-git-send-email-Eugeniy.Paltsev@synopsys.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1485358457-22957-3-git-send-email-Eugeniy.Paltsev@synopsys.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-snps-arc" Errors-To: linux-snps-arc-bounces+gla-linux-snps-arc=m.gmane.org@lists.infradead.org To: Eugeniy Paltsev Cc: Mark Rutland , devicetree@vger.kernel.org, Andy Shevchenko , Alexey Brodkin , linux-kernel@vger.kernel.org, Rob Herring , dmaengine@vger.kernel.org, Dan Williams , linux-snps-arc@lists.infradead.org List-Id: devicetree@vger.kernel.org On Wed, Jan 25, 2017 at 06:34:17PM +0300, Eugeniy Paltsev wrote: > This patch adds support for the DW AXI DMAC controller. > > DW AXI DMAC is a part of upcoming development board from Synopsys. How different is AXI from DW Synopsys? Is the spec publicly available? > +config AXI_DW_DMAC > + tristate "Synopsys DesignWare AXI DMA support" > + depends on OF && !64BIT why not 64 bit, can you also add compile test > +#define AXI_DMA_BUSWIDTHS \ > + (DMA_SLAVE_BUSWIDTH_UNDEFINED | \ DMA_SLAVE_BUSWIDTH_UNDEFINED?? > +static irqreturn_t dw_axi_dma_intretupt(int irq, void *dev_id) > +{ > + struct axi_dma_chip *chip = dev_id; > + > + /* Disable DMAC inerrupts. We'll enable them in the tasklet */ > + axi_dma_irq_disable(chip); > + > + tasklet_schedule(&chip->dw->tasklet); This is very inefficient, we would want to submit next txn here and not in tasklet > +static void axi_chan_block_xfer_complete(struct axi_dma_chan *chan) > +{ > + struct virt_dma_desc *vd; > + unsigned long flags; > + > + spin_lock_irqsave(&chan->vc.lock, flags); > + if (unlikely(axi_chan_is_hw_enable(chan))) { > + dev_err(chan2dev(chan), "BUG: %s catched DWAXIDMAC_IRQ_DMA_TRF, but channel not idle!\n", > + axi_chan_name(chan)); > + axi_chan_disable(chan); > + } > + > + /* The completed descriptor currently is in the head of vc list */ > + vd = vchan_next_desc(&chan->vc); > + /* Remove the completed descriptor from issued list before completing */ > + list_del(&vd->node); > + vchan_cookie_complete(vd); this should be called from irq handler > +static int dw_remove(struct platform_device *pdev) > +{ > + struct axi_dma_chip *chip = platform_get_drvdata(pdev); > + struct dw_axi_dma *dw = chip->dw; > + struct axi_dma_chan *chan, *_chan; > + u32 i; > + > + axi_dma_irq_disable(chip); > + for (i = 0; i < dw->hdata->nr_channels; i++) { > + axi_chan_disable(&chip->dw->chan[i]); > + axi_chan_irq_disable(&chip->dw->chan[i], DWAXIDMAC_IRQ_ALL); > + } > + axi_dma_disable(chip); > + > + tasklet_kill(&dw->tasklet); > + > + list_for_each_entry_safe(chan, _chan, &dw->dma.channels, > + vc.chan.device_node) { > + list_del(&chan->vc.chan.device_node); > + tasklet_kill(&chan->vc.task); > + } very nice :) But please freeup irq as well > +module_platform_driver(dw_driver); > + > +static int __init dw_init(void) > +{ > + return platform_driver_register(&dw_driver); > +} > +subsys_initcall(dw_init); why subsys_initcall?? > +/* Common registers offset */ > +#define DMAC_ID 0x000 // R DMAC ID > +#define DMAC_COMPVER 0x008 // R DMAC Component Version > +#define DMAC_CFG 0x010 // R/W DMAC Configuration > +#define DMAC_CHEN 0x018 // R/W DMAC Channel Enable > +#define DMAC_CHEN_L 0x018 // R/W DMAC Channel Enable 00-31 > +#define DMAC_CHEN_H 0x01C // R/W DMAC Channel Enable 32-63 > +#define DMAC_INTSTATUS 0x030 // R DMAC Interrupt Status > +#define DMAC_COMMON_INTCLEAR 0x038 // W DMAC Interrupt Clear > +#define DMAC_COMMON_INTSTATUS_ENA 0x040 // R DMAC Interrupt Status Enable > +#define DMAC_COMMON_INTSIGNAL_ENA 0x048 // R/W DMAC Interrupt Signal Enable > +#define DMAC_COMMON_INTSTATUS 0x050 // R DMAC Interrupt Status > +#define DMAC_RESET 0x058 // R DMAC Reset Register1 DMAC is a generic term, AX_DMAC and no C99 style comment, checkpatch would have cribbed Use BITS and GENMASK here > + > +/* DMA channel registers offset */ > +#define CH_SAR 0x000 // R/W Chan Source Address > +#define CH_DAR 0x008 // R/W Chan Destination Address > +#define CH_BLOCK_TS 0x010 // R/W Chan Block Transfer Size > +#define CH_CTL 0x018 // R/W Chan Control > +#define CH_CTL_L 0x018 // R/W Chan Control 00-31 > +#define CH_CTL_H 0x01C // R/W Chan Control 32-63 > +#define CH_CFG 0x020 // R/W Chan Configuration > +#define CH_CFG_L 0x020 // R/W Chan Configuration 00-31 > +#define CH_CFG_H 0x024 // R/W Chan Configuration 32-63 > +#define CH_LLP 0x028 // R/W Chan Linked List Pointer > +#define CH_STATUS 0x030 // R Chan Status > +#define CH_SWHSSRC 0x038 // R/W Chan SW Handshake Source > +#define CH_SWHSDST 0x040 // R/W Chan SW Handshake Destination > +#define CH_BLK_TFR_RESUMEREQ 0x048 // W Chan Block Transfer Resume Req > +#define CH_AXI_ID 0x050 // R/W Chan AXI ID > +#define CH_AXI_QOS 0x058 // R/W Chan AXI QOS > +#define CH_SSTAT 0x060 // R Chan Source Status > +#define CH_DSTAT 0x068 // R Chan Destination Status > +#define CH_SSTATAR 0x070 // R/W Chan Source Status Fetch Addr > +#define CH_DSTATAR 0x078 // R/W Chan Destination Status Fetch Addr > +#define CH_INTSTATUS_ENA 0x080 // R/W Chan Interrupt Status Enable > +#define CH_INTSTATUS 0x088 // R/W Chan Interrupt Status > +#define CH_INTSIGNAL_ENA 0x090 // R/W Chan Interrupt Signal Enable > +#define CH_INTCLEAR 0x098 // W Chan Interrupt Clear Same here -- ~Vinod From mboxrd@z Thu Jan 1 00:00:00 1970 From: vinod.koul@intel.com (Vinod Koul) Date: Fri, 10 Feb 2017 11:36:06 +0530 Subject: [PATCH 2/2] dmaengine: Add DW AXI DMAC driver In-Reply-To: <1485358457-22957-3-git-send-email-Eugeniy.Paltsev@synopsys.com> References: <1485358457-22957-1-git-send-email-Eugeniy.Paltsev@synopsys.com> <1485358457-22957-3-git-send-email-Eugeniy.Paltsev@synopsys.com> List-ID: Message-ID: <20170210060606.GO19244@localhost> To: linux-snps-arc@lists.infradead.org On Wed, Jan 25, 2017@06:34:17PM +0300, Eugeniy Paltsev wrote: > This patch adds support for the DW AXI DMAC controller. > > DW AXI DMAC is a part of upcoming development board from Synopsys. How different is AXI from DW Synopsys? Is the spec publicly available? > +config AXI_DW_DMAC > + tristate "Synopsys DesignWare AXI DMA support" > + depends on OF && !64BIT why not 64 bit, can you also add compile test > +#define AXI_DMA_BUSWIDTHS \ > + (DMA_SLAVE_BUSWIDTH_UNDEFINED | \ DMA_SLAVE_BUSWIDTH_UNDEFINED?? > +static irqreturn_t dw_axi_dma_intretupt(int irq, void *dev_id) > +{ > + struct axi_dma_chip *chip = dev_id; > + > + /* Disable DMAC inerrupts. We'll enable them in the tasklet */ > + axi_dma_irq_disable(chip); > + > + tasklet_schedule(&chip->dw->tasklet); This is very inefficient, we would want to submit next txn here and not in tasklet > +static void axi_chan_block_xfer_complete(struct axi_dma_chan *chan) > +{ > + struct virt_dma_desc *vd; > + unsigned long flags; > + > + spin_lock_irqsave(&chan->vc.lock, flags); > + if (unlikely(axi_chan_is_hw_enable(chan))) { > + dev_err(chan2dev(chan), "BUG: %s catched DWAXIDMAC_IRQ_DMA_TRF, but channel not idle!\n", > + axi_chan_name(chan)); > + axi_chan_disable(chan); > + } > + > + /* The completed descriptor currently is in the head of vc list */ > + vd = vchan_next_desc(&chan->vc); > + /* Remove the completed descriptor from issued list before completing */ > + list_del(&vd->node); > + vchan_cookie_complete(vd); this should be called from irq handler > +static int dw_remove(struct platform_device *pdev) > +{ > + struct axi_dma_chip *chip = platform_get_drvdata(pdev); > + struct dw_axi_dma *dw = chip->dw; > + struct axi_dma_chan *chan, *_chan; > + u32 i; > + > + axi_dma_irq_disable(chip); > + for (i = 0; i < dw->hdata->nr_channels; i++) { > + axi_chan_disable(&chip->dw->chan[i]); > + axi_chan_irq_disable(&chip->dw->chan[i], DWAXIDMAC_IRQ_ALL); > + } > + axi_dma_disable(chip); > + > + tasklet_kill(&dw->tasklet); > + > + list_for_each_entry_safe(chan, _chan, &dw->dma.channels, > + vc.chan.device_node) { > + list_del(&chan->vc.chan.device_node); > + tasklet_kill(&chan->vc.task); > + } very nice :) But please freeup irq as well > +module_platform_driver(dw_driver); > + > +static int __init dw_init(void) > +{ > + return platform_driver_register(&dw_driver); > +} > +subsys_initcall(dw_init); why subsys_initcall?? > +/* Common registers offset */ > +#define DMAC_ID 0x000 // R DMAC ID > +#define DMAC_COMPVER 0x008 // R DMAC Component Version > +#define DMAC_CFG 0x010 // R/W DMAC Configuration > +#define DMAC_CHEN 0x018 // R/W DMAC Channel Enable > +#define DMAC_CHEN_L 0x018 // R/W DMAC Channel Enable 00-31 > +#define DMAC_CHEN_H 0x01C // R/W DMAC Channel Enable 32-63 > +#define DMAC_INTSTATUS 0x030 // R DMAC Interrupt Status > +#define DMAC_COMMON_INTCLEAR 0x038 // W DMAC Interrupt Clear > +#define DMAC_COMMON_INTSTATUS_ENA 0x040 // R DMAC Interrupt Status Enable > +#define DMAC_COMMON_INTSIGNAL_ENA 0x048 // R/W DMAC Interrupt Signal Enable > +#define DMAC_COMMON_INTSTATUS 0x050 // R DMAC Interrupt Status > +#define DMAC_RESET 0x058 // R DMAC Reset Register1 DMAC is a generic term, AX_DMAC and no C99 style comment, checkpatch would have cribbed Use BITS and GENMASK here > + > +/* DMA channel registers offset */ > +#define CH_SAR 0x000 // R/W Chan Source Address > +#define CH_DAR 0x008 // R/W Chan Destination Address > +#define CH_BLOCK_TS 0x010 // R/W Chan Block Transfer Size > +#define CH_CTL 0x018 // R/W Chan Control > +#define CH_CTL_L 0x018 // R/W Chan Control 00-31 > +#define CH_CTL_H 0x01C // R/W Chan Control 32-63 > +#define CH_CFG 0x020 // R/W Chan Configuration > +#define CH_CFG_L 0x020 // R/W Chan Configuration 00-31 > +#define CH_CFG_H 0x024 // R/W Chan Configuration 32-63 > +#define CH_LLP 0x028 // R/W Chan Linked List Pointer > +#define CH_STATUS 0x030 // R Chan Status > +#define CH_SWHSSRC 0x038 // R/W Chan SW Handshake Source > +#define CH_SWHSDST 0x040 // R/W Chan SW Handshake Destination > +#define CH_BLK_TFR_RESUMEREQ 0x048 // W Chan Block Transfer Resume Req > +#define CH_AXI_ID 0x050 // R/W Chan AXI ID > +#define CH_AXI_QOS 0x058 // R/W Chan AXI QOS > +#define CH_SSTAT 0x060 // R Chan Source Status > +#define CH_DSTAT 0x068 // R Chan Destination Status > +#define CH_SSTATAR 0x070 // R/W Chan Source Status Fetch Addr > +#define CH_DSTATAR 0x078 // R/W Chan Destination Status Fetch Addr > +#define CH_INTSTATUS_ENA 0x080 // R/W Chan Interrupt Status Enable > +#define CH_INTSTATUS 0x088 // R/W Chan Interrupt Status > +#define CH_INTSIGNAL_ENA 0x090 // R/W Chan Interrupt Signal Enable > +#define CH_INTCLEAR 0x098 // W Chan Interrupt Clear Same here -- ~Vinod