Re: [PATCH 2/2] dmaengine: Add DW AXI DMAC driver

[Vinod Koul <vinod.koul@intel.com>]

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