Message-ID: <1336558444.1540.243.camel@vkoul-udesk3>
Date:	Wed, 09 May 2012 15:44:04 +0530
From:	Vinod Koul <vinod.koul@...ux.intel.com>
To:	Laxman Dewangan <ldewangan@...dia.com>
Cc:	dan.j.williams@...el.com, grant.likely@...retlab.ca,
	rob.herring@...xeda.com, linux-kernel@...r.kernel.org,
	devicetree-discuss@...ts.ozlabs.org, linux-tegra@...r.kernel.org
Subject: Re: [PATCH V2 2/2] dmaengine: tegra: add dma driver

On Thu, 2012-05-03 at 13:11 +0530, Laxman Dewangan wrote:
> Adding dmaengine based NVIDIA's Tegra APB dma driver.
> This driver support the slave mode of data transfer from
> peripheral to memory and vice versa.
> The driver supports for the cyclic and non-cyclic mode
> of data transfer.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@...dia.com>
> ---
> Changes from V1: 
> - Incorportaed review comments from Vinod.
> - get rid of the tegra_dma header.
> - Having clock control through runtime pm.
> - Remove regmap support.
> 
>  drivers/dma/Kconfig     |   13 +
>  drivers/dma/Makefile    |    1 +
>  drivers/dma/tegra_dma.c | 1714 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 1728 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/dma/tegra_dma.c
> 
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index ef378b5..26d9a23 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -148,6 +148,19 @@ config TXX9_DMAC
>  	  Support the TXx9 SoC internal DMA controller.  This can be
>  	  integrated in chips such as the Toshiba TX4927/38/39.
>  
> +config TEGRA_DMA
> +	bool "NVIDIA Tegra DMA support"
> +	depends on ARCH_TEGRA
> +	select DMA_ENGINE
> +	help
> +	  Support for the NVIDIA Tegra DMA controller driver. The DMA
> +	  controller is having multiple DMA channel which can be configured
> +	  for different peripherals like audio, UART, SPI, I2C etc which is
> +	  in APB bus.
> +	  This DMA controller transfers data from memory to peripheral fifo
> +	  address or vice versa. It does not support memory to memory data
> +	  transfer.
> +
>  config SH_DMAE
>  	tristate "Renesas SuperH DMAC support"
>  	depends on (SUPERH && SH_DMA) || (ARM && ARCH_SHMOBILE)
> diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
> index 86b795b..3aaa63a 100644
> --- a/drivers/dma/Makefile
> +++ b/drivers/dma/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_MXS_DMA) += mxs-dma.o
>  obj-$(CONFIG_TIMB_DMA) += timb_dma.o
>  obj-$(CONFIG_SIRF_DMA) += sirf-dma.o
>  obj-$(CONFIG_STE_DMA40) += ste_dma40.o ste_dma40_ll.o
> +obj-$(CONFIG_TEGRA_DMA) += tegra_dma.o
>  obj-$(CONFIG_PL330_DMA) += pl330.o
>  obj-$(CONFIG_PCH_DMA) += pch_dma.o
>  obj-$(CONFIG_AMBA_PL08X) += amba-pl08x.o
> diff --git a/drivers/dma/tegra_dma.c b/drivers/dma/tegra_dma.c
> new file mode 100644
> index 0000000..3c599ca
> --- /dev/null
> +++ b/drivers/dma/tegra_dma.c
> @@ -0,0 +1,1714 @@
> +/*
> + * DMA driver for Nvidia's Tegra APB DMA controller.
> + *
> + * Copyright (c) 2012, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/slab.h>
> +
> +#include <mach/clk.h>
> +#include "dmaengine.h"
> +
> +#define APB_DMA_GEN			0x0
> +#define GEN_ENABLE			BIT(31)
> +
> +#define APB_DMA_CNTRL			0x010
> +#define APB_DMA_IRQ_MASK		0x01c
> +#define APB_DMA_IRQ_MASK_SET		0x020
> +
> +/* CSR register */
> +#define APB_DMA_CHAN_CSR		0x00
> +#define CSR_ENB				BIT(31)
> +#define CSR_IE_EOC			BIT(30)
> +#define CSR_HOLD			BIT(29)
> +#define CSR_DIR				BIT(28)
> +#define CSR_ONCE			BIT(27)
> +#define CSR_FLOW			BIT(21)
> +#define CSR_REQ_SEL_SHIFT		16
> +#define CSR_WCOUNT_MASK			0xFFFC
> +
> +/* STATUS register */
> +#define APB_DMA_CHAN_STA		0x004
> +#define STA_BUSY			BIT(31)
> +#define STA_ISE_EOC			BIT(30)
> +#define STA_HALT			BIT(29)
> +#define STA_PING_PONG			BIT(28)
> +#define STA_COUNT_SHIFT			2
> +#define STA_COUNT_MASK			0xFFFC
> +
> +/* AHB memory address */
> +#define APB_DMA_CHAN_AHB_PTR		0x010
> +
> +/* AHB sequence register */
> +#define APB_DMA_CHAN_AHB_SEQ		0x14
> +#define AHB_SEQ_INTR_ENB		BIT(31)
> +#define AHB_SEQ_BUS_WIDTH_SHIFT		28
> +#define AHB_SEQ_BUS_WIDTH_8		(0 << AHB_SEQ_BUS_WIDTH_SHIFT)
> +#define AHB_SEQ_BUS_WIDTH_16		(1 << AHB_SEQ_BUS_WIDTH_SHIFT)
> +#define AHB_SEQ_BUS_WIDTH_32		(2 << AHB_SEQ_BUS_WIDTH_SHIFT)
> +#define AHB_SEQ_BUS_WIDTH_64		(3 << AHB_SEQ_BUS_WIDTH_SHIFT)
> +#define AHB_SEQ_BUS_WIDTH_128		(4 << AHB_SEQ_BUS_WIDTH_SHIFT)
> +#define AHB_SEQ_DATA_SWAP		BIT(27)
> +#define AHB_SEQ_BURST_1			(4 << 24)
> +#define AHB_SEQ_BURST_4			(5 << 24)
> +#define AHB_SEQ_BURST_8			(6 << 24)
> +#define AHB_SEQ_DBL_BUF			BIT(19)
> +#define AHB_SEQ_WRAP_SHIFT		16
> +#define AHB_SEQ_WRAP_NONE		0
> +
> +/* APB address */
> +#define APB_DMA_CHAN_APB_PTR		0x018
> +
> +/* APB sequence register */
> +#define APB_DMA_CHAN_APB_SEQ		0x01c
> +#define APB_SEQ_BUS_WIDTH_SHIFT		28
> +#define APB_SEQ_BUS_WIDTH_8		(0 << APB_SEQ_BUS_WIDTH_SHIFT)
> +#define APB_SEQ_BUS_WIDTH_16		(1 << APB_SEQ_BUS_WIDTH_SHIFT)
> +#define APB_SEQ_BUS_WIDTH_32		(2 << APB_SEQ_BUS_WIDTH_SHIFT)
> +#define APB_SEQ_BUS_WIDTH_64		(3 << APB_SEQ_BUS_WIDTH_SHIFT)
> +#define APB_SEQ_BUS_WIDTH_128		(4 << APB_SEQ_BUS_WIDTH_SHIFT)
> +#define APB_SEQ_DATA_SWAP		BIT(27)
> +#define APB_SEQ_WRAP_SHIFT		16
> +#define APB_SEQ_WRAP_WORD_1		(1 << APB_SEQ_WRAP_SHIFT)
> +
> +/*
> + * If any burst is in flight and DMA paused then this is the time to complete
> + * on-flight burst and update DMA status register.
> + */
> +#define DMA_BURST_COMPLETE_TIME		20
> +
> +/* Channel base address offset from APBDMA base address */
> +#define DMA_CHANNEL_BASE_ADDRESS_OFFSET	0x1000
> +
> +/* DMA channel register space size */
> +#define DMA_CHANNEL_REGISTER_SIZE	0x20
> +
> +/*
> + * Initial number of descriptors to allocate for each channel during
> + * allocation. More descriptors will be allocated dynamically if
> + * client needs it.
> + */
> +#define DMA_NR_DESCS_PER_CHANNEL	4
> +#define DMA_NR_REQ_PER_DESC		8
all of these should be namespaced.
APB and AHB are fairly generic names. 
> +
> +struct tegra_dma;
> +
> +/*
> + * tegra_dma_chip_data Tegra chip specific DMA data
> + * @nr_channels: Number of channels available in the controller.
> + * @max_dma_count: Maximum DMA transfer count supported by DMA controller.
> + */
> +struct tegra_dma_chip_data {
> +	int nr_channels;
> +	int max_dma_count;
> +};
> +
> +/*
> + * dma_transfer_mode: Different DMA transfer mode.
> + * DMA_MODE_ONCE: DMA transfer the configured buffer once and at the end of
> + *		transfer, DMA  stops automatically and generates interrupt
> + *		if enabled. SW need to reprogram DMA for next transfer.
> + * DMA_MODE_CYCLE: DMA keeps transferring the same buffer again and again
> + *		until DMA stopped explicitly by SW or another buffer
> + *		configured. After transfer completes, DMA again starts
> + *		transfer from beginning of buffer without SW intervention.
> + *		If any new address/size is configured during buffer transfer
> + *		then DMA start transfer with new configuration when the
> + *		current transfer has completed otherwise it will keep
> + *		transferring with old configuration. It also generates
> + *		the interrupt each time the buffer transfer completes.
> + * DMA_MODE_CYCLE_HALF_NOTIFY: This mode is identical to DMA_MODE_CYCLE,
> + *		except that an additional interrupt is generated half-way
> + *		through the buffer. This is kind of ping-pong buffer mechanism.
> + *		If SW wants to change the address/size of the buffer then
> + *		it must only change when DMA is transferring the second
> + *		half of buffer. In DMA configuration, it only need to
> + *		configure starting of first buffer and size of the half buffer.
> + */
> +enum dma_transfer_mode {
> +	DMA_MODE_NONE,
> +	DMA_MODE_ONCE,
> +	DMA_MODE_CYCLE,
> +	DMA_MODE_CYCLE_HALF_NOTIFY,
> +};
I dont understand why you would need to keep track of these?
You get a request for DMA. You have properties of transfer. You prepare
you descriptors and then submit.
Why would you need to create above modes and remember them?
> +
> +/* Dma channel registers */
> +struct tegra_dma_channel_regs {
> +	unsigned long	csr;
> +	unsigned long	ahb_ptr;
> +	unsigned long	apb_ptr;
> +	unsigned long	ahb_seq;
> +	unsigned long	apb_seq;
> +};
> +
> +/*
> + * tegra_dma_sg_req: Dma request details to configure hardware. This
> + * contains the details for one transfer to configure DMA hw.
> + * The client's request for data transfer can be broken into multiple
> + * sub-transfer as per requester details and hw support.
> + * This sub transfer get added in the list of transfer and point to Tegra
> + * DMA descriptor which manages the transfer details.
> + */
> +struct tegra_dma_sg_req {
> +	struct tegra_dma_channel_regs	ch_regs;
> +	int				req_len;
> +	bool				configured;
> +	bool				last_sg;
> +	bool				half_done;
> +	struct list_head		node;
> +	struct tegra_dma_desc		*dma_desc;
> +};
> +
> +/*
> + * tegra_dma_desc: Tegra DMA descriptors which manages the client requests.
> + * This descriptor keep track of transfer status, callbacks and request
> + * counts etc.
> + */
> +struct tegra_dma_desc {
> +	struct dma_async_tx_descriptor	txd;
> +	int				bytes_requested;
> +	int				bytes_transferred;
> +	enum dma_status			dma_status;
> +	struct list_head		node;
> +	struct list_head		tx_list;
> +	struct list_head		cb_node;
> +	bool				ack_reqd;
> +	bool				cb_due;
what are these two for, seems redundant to me
> +	dma_cookie_t			cookie;
> +};
> +
> +struct tegra_dma_channel;
> +
> +typedef void (*dma_isr_handler)(struct tegra_dma_channel *tdc,
> +				bool to_terminate);
> +
> +/* tegra_dma_channel: Channel specific information */
> +struct tegra_dma_channel {
> +	struct dma_chan		dma_chan;
> +	bool			config_init;
> +	int			id;
> +	int			irq;
> +	unsigned long		chan_base_offset;
> +	spinlock_t		lock;
> +	bool			busy;
> +	enum dma_transfer_mode	dma_mode;
> +	int			descs_allocated;
> +	struct tegra_dma	*tdma;
> +
> +	/* Different lists for managing the requests */
> +	struct list_head	free_sg_req;
> +	struct list_head	pending_sg_req;
> +	struct list_head	free_dma_desc;
> +	struct list_head	wait_ack_dma_desc;
> +	struct list_head	cb_desc;
> +
> +	/* isr handler and tasklet for bottom half of isr handling */
> +	dma_isr_handler		isr_handler;
> +	struct tasklet_struct	tasklet;
> +	dma_async_tx_callback	callback;
> +	void			*callback_param;
> +
> +	/* Channel-slave specific configuration */
> +	struct dma_slave_config dma_sconfig;
> +};
> +
> +/* tegra_dma: Tegra DMA specific information */
> +struct tegra_dma {
> +	struct dma_device		dma_dev;
> +	struct device			*dev;
> +	struct clk			*dma_clk;
> +	spinlock_t			global_lock;
> +	void __iomem			*base_addr;
> +	struct tegra_dma_chip_data	*chip_data;
> +
> +	/* Some register need to be cache before suspend */
> +	u32				reg_gen;
> +
> +	/* Last member of the structure */
> +	struct tegra_dma_channel channels[0];
> +};
> +
> +static inline void tdma_write(struct tegra_dma *tdma, u32 reg, u32 val)
> +{
> +	writel(val, tdma->base_addr + reg);
> +}
> +
> +static inline u32 tdma_read(struct tegra_dma *tdma, u32 reg)
> +{
> +	return readl(tdma->base_addr + reg);
> +}
> +
> +static inline void tdc_write(struct tegra_dma_channel *tdc,
> +		u32 reg, u32 val)
> +{
> +	writel(val, tdc->tdma->base_addr + tdc->chan_base_offset + reg);
> +}
> +
> +static inline u32 tdc_read(struct tegra_dma_channel *tdc, u32 reg)
> +{
> +	return readl(tdc->tdma->base_addr + tdc->chan_base_offset + reg);
> +}
> +
> +static inline struct tegra_dma_channel *to_tegra_dma_chan(struct dma_chan *dc)
> +{
> +	return container_of(dc, struct tegra_dma_channel, dma_chan);
> +}
> +
> +static inline struct tegra_dma_desc *txd_to_tegra_dma_desc(
> +		struct dma_async_tx_descriptor *td)
> +{
> +	return container_of(td, struct tegra_dma_desc, txd);
> +}
> +
> +static inline struct device *tdc2dev(struct tegra_dma_channel *tdc)
> +{
> +	return &tdc->dma_chan.dev->device;
> +}
> +
> +static dma_cookie_t tegra_dma_tx_submit(struct dma_async_tx_descriptor *tx);
> +static int tegra_dma_runtime_suspend(struct device *dev);
> +static int tegra_dma_runtime_resume(struct device *dev);
> +
> +static int allocate_tegra_desc(struct tegra_dma_channel *tdc,
> +		int ndma_desc, int nsg_req)
pls follow single convention for naming in driver eithe xxx_tegra_yyy or
tegra_xxx_yyy... BUT not both!
> +{
> +	int i;
> +	struct tegra_dma_desc *dma_desc;
> +	struct tegra_dma_sg_req *sg_req;
> +	struct dma_chan *dc = &tdc->dma_chan;
> +	struct list_head dma_desc_list;
> +	struct list_head sg_req_list;
> +	unsigned long flags;
> +
> +	INIT_LIST_HEAD(&dma_desc_list);
> +	INIT_LIST_HEAD(&sg_req_list);
> +
> +	/* Initialize DMA descriptors */
> +	for (i = 0; i < ndma_desc; ++i) {
> +		dma_desc = kzalloc(sizeof(struct tegra_dma_desc), GFP_ATOMIC);
kernel convention is kzalloc(sizeof(*dma_desc),....
> +		if (!dma_desc) {
> +			dev_err(tdc2dev(tdc),
> +				"%s(): Memory allocation fails\n", __func__);
> +			goto fail_alloc;
> +		}
> +
> +		dma_async_tx_descriptor_init(&dma_desc->txd, dc);
> +		dma_desc->txd.tx_submit = tegra_dma_tx_submit;
> +		dma_desc->txd.flags = DMA_CTRL_ACK;
> +		list_add_tail(&dma_desc->node, &dma_desc_list);
> +	}
> +
> +	/* Initialize req descriptors */
> +	for (i = 0; i < nsg_req; ++i) {
> +		sg_req = kzalloc(sizeof(struct tegra_dma_sg_req), GFP_ATOMIC);
> +		if (!sg_req) {
> +			dev_err(tdc2dev(tdc),
> +				"%s(): Memory allocation fails\n", __func__);
> +			goto fail_alloc;
> +		}
> +		list_add_tail(&sg_req->node, &sg_req_list);
> +	}
> +
> +	spin_lock_irqsave(&tdc->lock, flags);
> +	if (ndma_desc) {
> +		tdc->descs_allocated += ndma_desc;
> +		list_splice(&dma_desc_list, &tdc->free_dma_desc);
> +	}
> +
> +	if (nsg_req)
> +		list_splice(&sg_req_list, &tdc->free_sg_req);
> +	spin_unlock_irqrestore(&tdc->lock, flags);
> +	return tdc->descs_allocated;
> +
> +fail_alloc:
> +	while (!list_empty(&dma_desc_list)) {
> +		dma_desc = list_first_entry(&dma_desc_list,
> +					typeof(*dma_desc), node);
> +		list_del(&dma_desc->node);
> +	}
> +
> +	while (!list_empty(&sg_req_list)) {
> +		sg_req = list_first_entry(&sg_req_list, typeof(*sg_req), node);
> +		list_del(&sg_req->node);
> +	}
> +	return 0;
> +}
> +
> +/* Get DMA desc from free list, if not there then allocate it */
> +static struct tegra_dma_desc *tegra_dma_desc_get(struct tegra_dma_channel *tdc)
> +{
> +	struct tegra_dma_desc *dma_desc = NULL;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&tdc->lock, flags);
> +
> +	/* Check from free list desc */
> +	if (!list_empty(&tdc->free_dma_desc)) {
> +		dma_desc = list_first_entry(&tdc->free_dma_desc,
> +					typeof(*dma_desc), node);
> +		list_del(&dma_desc->node);
> +		goto end;
> +	}
sorry I dont like this jumping and returning for two lines of code.
Makes much sense to return from here.
> +
> +	/*
> +	 * Check list with desc which are waiting for ack, may be it
> +	 * got acked from client.
> +	 */
> +	if (!list_empty(&tdc->wait_ack_dma_desc)) {
> +		list_for_each_entry(dma_desc, &tdc->wait_ack_dma_desc, node) {
> +			if (async_tx_test_ack(&dma_desc->txd)) {
> +				list_del(&dma_desc->node);
> +				goto end;
> +			}
> +		}
> +	}
> +
> +	/* There is no free desc, allocate it */
> +	spin_unlock_irqrestore(&tdc->lock, flags);
> +	dev_dbg(tdc2dev(tdc),
> +		"Allocating more descriptors for channel %d\n", tdc->id);
> +	allocate_tegra_desc(tdc, DMA_NR_DESCS_PER_CHANNEL,
> +				DMA_NR_DESCS_PER_CHANNEL * DMA_NR_REQ_PER_DESC);
> +	spin_lock_irqsave(&tdc->lock, flags);
> +	if (list_empty(&tdc->free_dma_desc))
> +		goto end;
> +
> +	dma_desc = list_first_entry(&tdc->free_dma_desc,
> +					typeof(*dma_desc), node);
> +	list_del(&dma_desc->node);
> +end:
> +	spin_unlock_irqrestore(&tdc->lock, flags);
> +	return dma_desc;
> +}
> +
> +static void tegra_dma_desc_put(struct tegra_dma_channel *tdc,
> +		struct tegra_dma_desc *dma_desc)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&tdc->lock, flags);
> +	if (!list_empty(&dma_desc->tx_list))
> +		list_splice_init(&dma_desc->tx_list, &tdc->free_sg_req);
> +	list_add_tail(&dma_desc->node, &tdc->free_dma_desc);
> +	spin_unlock_irqrestore(&tdc->lock, flags);
> +}
> +
> +static void tegra_dma_desc_done_locked(struct tegra_dma_channel *tdc,
> +		struct tegra_dma_desc *dma_desc)
> +{
> +	if (dma_desc->ack_reqd)
> +		list_add_tail(&dma_desc->node, &tdc->wait_ack_dma_desc);
what does ack_reqd mean?
Do you ahve unlocked version of this function, name suggests so...
> +	else
> +		list_add_tail(&dma_desc->node, &tdc->free_dma_desc);
> +}
> +
> +static struct tegra_dma_sg_req *tegra_dma_sg_req_get(
> +		struct tegra_dma_channel *tdc)
in this and other functions, you have used goto to unlock and return.
Rather than that why don't you simplify code by calling these while
holding lock
> +{
> +	struct tegra_dma_sg_req *sg_req = NULL;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&tdc->lock, flags);
> +	if (list_empty(&tdc->free_sg_req)) {
> +		spin_unlock_irqrestore(&tdc->lock, flags);
> +		dev_dbg(tdc2dev(tdc),
> +			"Reallocating sg_req for channel %d\n", tdc->id);
> +		allocate_tegra_desc(tdc, 0,
> +				DMA_NR_DESCS_PER_CHANNEL * DMA_NR_REQ_PER_DESC);
> +		spin_lock_irqsave(&tdc->lock, flags);
> +		if (list_empty(&tdc->free_sg_req)) {
> +			dev_dbg(tdc2dev(tdc),
> +			"Not found free sg_req for channel %d\n", tdc->id);
> +			goto end;
> +		}
> +	}
> +
> +	sg_req = list_first_entry(&tdc->free_sg_req, typeof(*sg_req), node);
> +	list_del(&sg_req->node);
> +end:
> +	spin_unlock_irqrestore(&tdc->lock, flags);
> +	return sg_req;
> +}
[snip]

> +
> +static void tegra_dma_abort_all(struct tegra_dma_channel *tdc)
> +{
> +	struct tegra_dma_sg_req *sgreq;
> +	struct tegra_dma_desc *dma_desc;
> +	while (!list_empty(&tdc->pending_sg_req)) {
> +		sgreq = list_first_entry(&tdc->pending_sg_req,
> +						typeof(*sgreq), node);
> +		list_del(&sgreq->node);
> +		list_add_tail(&sgreq->node, &tdc->free_sg_req);
> +		if (sgreq->last_sg) {
> +			dma_desc = sgreq->dma_desc;
> +			dma_desc->dma_status = DMA_ERROR;
> +			tegra_dma_desc_done_locked(tdc, dma_desc);
> +
> +			/* Add in cb list if it is not there. */
> +			if (!dma_desc->cb_due) {
> +				list_add_tail(&dma_desc->cb_node,
> +							&tdc->cb_desc);
> +				dma_desc->cb_due = true;
> +			}
> +			dma_cookie_complete(&dma_desc->txd);
does this make sense. You are marking an aborted descriptor as complete.
> +		}
> +	}
> +	tdc->dma_mode = DMA_MODE_NONE;
> +}
> +
> +static bool handle_continuous_head_request(struct tegra_dma_channel *tdc,
> +		struct tegra_dma_sg_req *last_sg_req, bool to_terminate)
> +{
> +	struct tegra_dma_sg_req *hsgreq = NULL;
> +
> +	if (list_empty(&tdc->pending_sg_req)) {
> +		dev_err(tdc2dev(tdc),
> +			"%s(): Dma is running without any req list\n",
> +			__func__);
this is just waste of real estate and very ugly. Move them to 1/2 lines.
> +		tegra_dma_stop(tdc);
> +		return false;
> +	}
> +
> +	/*
> +	 * Check that head req on list should be in flight.
> +	 * If it is not in flight then abort transfer as
> +	 * transfer looping can not continue.
> +	 */
> +	hsgreq = list_first_entry(&tdc->pending_sg_req, typeof(*hsgreq), node);
> +	if (!hsgreq->configured) {
> +		tegra_dma_stop(tdc);
> +		dev_err(tdc2dev(tdc),
> +			"Error in dma transfer loop, aborting dma\n");
> +		tegra_dma_abort_all(tdc);
> +		return false;
> +	}
> +
> +	/* Configure next request in single buffer mode */
> +	if (!to_terminate && (tdc->dma_mode == DMA_MODE_CYCLE))
> +		tdc_configure_next_head_desc(tdc);
> +	return true;
> +}
> +
> +static void tegra_dma_tasklet(unsigned long data)
> +{
> +	struct tegra_dma_channel *tdc = (struct tegra_dma_channel *)data;
> +	unsigned long flags;
> +	dma_async_tx_callback callback = NULL;
> +	void *callback_param = NULL;
> +	struct tegra_dma_desc *dma_desc;
> +	struct list_head cb_dma_desc_list;
> +
> +	INIT_LIST_HEAD(&cb_dma_desc_list);
> +	spin_lock_irqsave(&tdc->lock, flags);
> +	if (list_empty(&tdc->cb_desc)) {
> +		spin_unlock_irqrestore(&tdc->lock, flags);
> +		return;
> +	}
> +	list_splice_init(&tdc->cb_desc, &cb_dma_desc_list);
> +	spin_unlock_irqrestore(&tdc->lock, flags);
> +
> +	while (!list_empty(&cb_dma_desc_list)) {
> +		dma_desc  = list_first_entry(&cb_dma_desc_list,
> +				typeof(*dma_desc), cb_node);
> +		list_del(&dma_desc->cb_node);
> +
> +		callback = dma_desc->txd.callback;
> +		callback_param = dma_desc->txd.callback_param;
> +		dma_desc->cb_due = false;
> +		if (callback)
> +			callback(callback_param);
You should mark the descriptor as complete before calling callback.
Also tasklet is supposed to move the next pending descriptor to the
engine, I dont see that happening here?
> +	}
> +}
> +
> +static void tegra_dma_terminate_all(struct dma_chan *dc)
> +{
> +	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
> +	struct tegra_dma_sg_req *sgreq;
> +	struct tegra_dma_desc *dma_desc;
> +	unsigned long flags;
> +	unsigned long status;
> +	struct list_head new_list;
> +	dma_async_tx_callback callback = NULL;
> +	void *callback_param = NULL;
> +	struct list_head cb_dma_desc_list;
> +	bool was_busy;
> +
> +	INIT_LIST_HEAD(&new_list);
> +	INIT_LIST_HEAD(&cb_dma_desc_list);
> +
> +	spin_lock_irqsave(&tdc->lock, flags);
> +	if (list_empty(&tdc->pending_sg_req)) {
> +		spin_unlock_irqrestore(&tdc->lock, flags);
> +		return;
> +	}
> +
> +	if (!tdc->busy) {
> +		list_splice_init(&tdc->cb_desc, &cb_dma_desc_list);
> +		goto skip_dma_stop;
> +	}
> +
> +	/* Pause DMA before checking the queue status */
> +	tegra_dma_pause(tdc, true);
> +
> +	status = tdc_read(tdc, APB_DMA_CHAN_STA);
> +	if (status & STA_ISE_EOC) {
> +		dev_dbg(tdc2dev(tdc), "%s():handling isr\n", __func__);
> +		tdc->isr_handler(tdc, true);
> +		status = tdc_read(tdc, APB_DMA_CHAN_STA);
> +	}
> +	list_splice_init(&tdc->cb_desc, &cb_dma_desc_list);
> +
> +	was_busy = tdc->busy;
> +	tegra_dma_stop(tdc);
> +	if (!list_empty(&tdc->pending_sg_req) && was_busy) {
> +		sgreq = list_first_entry(&tdc->pending_sg_req,
> +					typeof(*sgreq), node);
> +		sgreq->dma_desc->bytes_transferred +=
> +				get_current_xferred_count(tdc, sgreq, status);
> +	}
> +	tegra_dma_resume(tdc);
> +
> +skip_dma_stop:
> +	tegra_dma_abort_all(tdc);
> +	/* Ignore callbacks pending list */
> +	INIT_LIST_HEAD(&tdc->cb_desc);
> +	spin_unlock_irqrestore(&tdc->lock, flags);
> +
> +	/* Call callbacks if was pending before aborting requests */
> +	while (!list_empty(&cb_dma_desc_list)) {
> +		dma_desc  = list_first_entry(&cb_dma_desc_list,
> +				typeof(*dma_desc), cb_node);
> +		list_del(&dma_desc->cb_node);
> +		callback = dma_desc->txd.callback;
> +		callback_param = dma_desc->txd.callback_param;
again, callback would be called from tasklet, so why would it need to be
called from here , and why would this be pending.
> +		if (callback)
> +			callback(callback_param);
> +	}
> +}
> +
> +
-- 
~Vinod

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists