[PATCH 0/4] DMA: tegra-apb: Clean-up

[PATCH 0/4] DMA: tegra-apb: Clean-up

[Jon Hunter]

Some clean-up changes for the tegra-apb DMA driver.

These have been compile and boot tested for ARM and ARM64. Summary of the
ARM results are below. ARM64 has been tested locally.

Test summary
------------

Build: zImage:
    Pass: ( 2/ 2): multi_v7_defconfig, tegra_defconfig

Boot to userspace: multi_v7_defconfig:
    Pass: ( 4/ 4): tegra114-dalmore-a04, tegra124-jetson-tk1,
                   tegra20-trimslice, tegra30-beaver

Boot to userspace: tegra_defconfig:
    Pass: ( 4/ 4): tegra114-dalmore-a04, tegra124-jetson-tk1,
                   tegra20-trimslice, tegra30-beaver

Jon Hunter (4):
  DMA: tegra-apb: Remove unused variables
  DMA: tegra-apb: Avoid unnecessary channel base address calculation
  DMA: tegra-apb: Remove unnecessary return statements and variables
  DMA: tegra-apb: Simplify locking for device using global pause

 drivers/dma/tegra20-apb-dma.c | 63 ++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 25 deletions(-)

-- 
2.1.4

[PATCH 1/4] DMA: tegra-apb: Remove unused variables

[Jon Hunter]

The callback and callback_param members of the tegra_dma_sg_req structure
are never used. The dma-engine structure, dma_async_tx_descriptor, defines
the same members and these are the ones used by the driver. Therefore,
remove the unused versions from the tegra_dma_sg_req structure.

The half_done member of tegra_dma_channel structure is configured but
never used and so remove it.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/dma/tegra20-apb-dma.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index eaf585e8286b..cf8cff7827f0 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -155,7 +155,6 @@ struct tegra_dma_sg_req {
 	int				req_len;
 	bool				configured;
 	bool				last_sg;
-	bool				half_done;
 	struct list_head		node;
 	struct tegra_dma_desc		*dma_desc;
 };
@@ -203,8 +202,6 @@ struct tegra_dma_channel {
 	/* 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 */
 	unsigned int slave_id;
@@ -1136,7 +1133,6 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
 		sg_req->ch_regs.apb_seq = apb_seq;
 		sg_req->ch_regs.ahb_seq = ahb_seq;
 		sg_req->configured = false;
-		sg_req->half_done = false;
 		sg_req->last_sg = false;
 		sg_req->dma_desc = dma_desc;
 		sg_req->req_len = len;
-- 
2.1.4

[PATCH 2/4] DMA: tegra-apb: Avoid unnecessary channel base address calculation

[Jon Hunter]

Everytime a DMA channel register is accessed, the channel base address
is calculated by adding the DMA base address and the channel register
offset. Avoid this calculation and simply calculate the channel base
address once at probe time for each DMA channel.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/dma/tegra20-apb-dma.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index cf8cff7827f0..11edcca7619b 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -187,7 +187,7 @@ struct tegra_dma_channel {
 	bool			config_init;
 	int			id;
 	int			irq;
-	unsigned long		chan_base_offset;
+	void __iomem		*chan_addr;
 	spinlock_t		lock;
 	bool			busy;
 	struct tegra_dma	*tdma;
@@ -239,12 +239,12 @@ static inline u32 tdma_read(struct tegra_dma *tdma, u32 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);
+	writel(val, tdc->chan_addr + reg);
 }
 
 static inline u32 tdc_read(struct tegra_dma_channel *tdc, u32 reg)
 {
-	return readl(tdc->tdma->base_addr + tdc->chan_base_offset + reg);
+	return readl(tdc->chan_addr + reg);
 }
 
 static inline struct tegra_dma_channel *to_tegra_dma_chan(struct dma_chan *dc)
@@ -1373,8 +1373,9 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	for (i = 0; i < cdata->nr_channels; i++) {
 		struct tegra_dma_channel *tdc = &tdma->channels[i];
 
-		tdc->chan_base_offset = TEGRA_APBDMA_CHANNEL_BASE_ADD_OFFSET +
-					i * cdata->channel_reg_size;
+		tdc->chan_addr = tdma->base_addr +
+				 TEGRA_APBDMA_CHANNEL_BASE_ADD_OFFSET +
+				 (i * cdata->channel_reg_size);
 
 		res = platform_get_resource(pdev, IORESOURCE_IRQ, i);
 		if (!res) {
-- 
2.1.4

[PATCH 3/4] DMA: tegra-apb: Remove unnecessary return statements and variables

[Jon Hunter]

Some void functions have unnecessary return statements at the end
(reported by sparse) and so remove these. Also remove the return variables
from functions tegra_dma_prep_slave_sg() and tegra_dma_prep_slave_cyclic()
because the value is not used.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/dma/tegra20-apb-dma.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 11edcca7619b..53a8075dcec8 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -598,7 +598,6 @@ static void handle_once_dma_done(struct tegra_dma_channel *tdc,
 		return;
 
 	tdc_start_head_req(tdc);
-	return;
 }
 
 static void handle_cont_sngl_cycle_dma_done(struct tegra_dma_channel *tdc,
@@ -625,7 +624,6 @@ static void handle_cont_sngl_cycle_dma_done(struct tegra_dma_channel *tdc,
 		if (!st)
 			dma_desc->dma_status = DMA_ERROR;
 	}
-	return;
 }
 
 static void tegra_dma_tasklet(unsigned long data)
@@ -717,7 +715,6 @@ static void tegra_dma_issue_pending(struct dma_chan *dc)
 	}
 end:
 	spin_unlock_irqrestore(&tdc->lock, flags);
-	return;
 }
 
 static int tegra_dma_terminate_all(struct dma_chan *dc)
@@ -929,7 +926,6 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
 	struct tegra_dma_sg_req  *sg_req = NULL;
 	u32 burst_size;
 	enum dma_slave_buswidth slave_bw;
-	int ret;
 
 	if (!tdc->config_init) {
 		dev_err(tdc2dev(tdc), "dma channel is not configured\n");
@@ -940,9 +936,8 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_slave_sg(
 		return NULL;
 	}
 
-	ret = get_transfer_param(tdc, direction, &apb_ptr, &apb_seq, &csr,
-				&burst_size, &slave_bw);
-	if (ret < 0)
+	if (get_transfer_param(tdc, direction, &apb_ptr, &apb_seq, &csr,
+				&burst_size, &slave_bw) < 0)
 		return NULL;
 
 	INIT_LIST_HEAD(&req_list);
@@ -1045,7 +1040,6 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
 	dma_addr_t mem = buf_addr;
 	u32 burst_size;
 	enum dma_slave_buswidth slave_bw;
-	int ret;
 
 	if (!buf_len || !period_len) {
 		dev_err(tdc2dev(tdc), "Invalid buffer/period len\n");
@@ -1084,12 +1078,10 @@ static struct dma_async_tx_descriptor *tegra_dma_prep_dma_cyclic(
 		return NULL;
 	}
 
-	ret = get_transfer_param(tdc, direction, &apb_ptr, &apb_seq, &csr,
-				&burst_size, &slave_bw);
-	if (ret < 0)
+	if (get_transfer_param(tdc, direction, &apb_ptr, &apb_seq, &csr,
+				&burst_size, &slave_bw) < 0)
 		return NULL;
 
-
 	ahb_seq = TEGRA_APBDMA_AHBSEQ_INTR_ENB;
 	ahb_seq |= TEGRA_APBDMA_AHBSEQ_WRAP_NONE <<
 					TEGRA_APBDMA_AHBSEQ_WRAP_SHIFT;
-- 
2.1.4

[PATCH 4/4] DMA: tegra-apb: Simplify locking for device using global pause

[Jon Hunter]

Sparse reports the following with regard to locking in the
tegra_dma_global_pause() and tegra_dma_global_resume() functions:

drivers/dma/tegra20-apb-dma.c:362:9: warning: context imbalance in
	'tegra_dma_global_pause' - wrong count at exit
drivers/dma/tegra20-apb-dma.c:366:13: warning: context imbalance in
	'tegra_dma_global_resume' - unexpected unlock

The warning is caused because tegra_dma_global_pause() acquires a lock
but does not release it. However, the lock is released by
tegra_dma_global_resume(). These pause/resume functions are called in
pairs and so it does appear to work.

This global pause is used on early tegra devices that do not have an
individual pause for each channel. The lock appears to be used to ensure
that multiple channels do not attempt to assert/de-assert the global pause
at the same time which could cause the DMA controller to be in the wrong
paused state. Rather than locking around the entire code between the pause
and resume, employ a simple counter to keep track of the global pause
requests. By using a counter, it is only necessary to hold the lock when
pausing and unpausing the DMA controller and hence, fixes the sparse
warning.

Please note that for devices that support individual channel pausing, the
DMA controller lock is not held between pausing and unpausing the channel.
Hence, this change will make the devices that use the global pause behave
in the same way, with regard to locking, as those that don't.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
 drivers/dma/tegra20-apb-dma.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 53a8075dcec8..c8f79dcaaee8 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -219,6 +219,13 @@ struct tegra_dma {
 	void __iomem			*base_addr;
 	const struct tegra_dma_chip_data *chip_data;
 
+	/*
+	 * Counter for managing global pausing of the DMA controller.
+	 * Only applicable for devices that don't support individual
+	 * channel pausing.
+	 */
+	u32				global_pause_count;
+
 	/* Some register need to be cache before suspend */
 	u32				reg_gen;
 
@@ -358,16 +365,32 @@ static void tegra_dma_global_pause(struct tegra_dma_channel *tdc,
 	struct tegra_dma *tdma = tdc->tdma;
 
 	spin_lock(&tdma->global_lock);
-	tdma_write(tdma, TEGRA_APBDMA_GENERAL, 0);
-	if (wait_for_burst_complete)
-		udelay(TEGRA_APBDMA_BURST_COMPLETE_TIME);
+
+	if (tdc->tdma->global_pause_count == 0) {
+		tdma_write(tdma, TEGRA_APBDMA_GENERAL, 0);
+		if (wait_for_burst_complete)
+			udelay(TEGRA_APBDMA_BURST_COMPLETE_TIME);
+	}
+
+	tdc->tdma->global_pause_count++;
+
+	spin_unlock(&tdma->global_lock);
 }
 
 static void tegra_dma_global_resume(struct tegra_dma_channel *tdc)
 {
 	struct tegra_dma *tdma = tdc->tdma;
 
-	tdma_write(tdma, TEGRA_APBDMA_GENERAL, TEGRA_APBDMA_GENERAL_ENABLE);
+	spin_lock(&tdma->global_lock);
+
+	if (WARN_ON(tdc->tdma->global_pause_count == 0))
+		goto out;
+
+	if (--tdc->tdma->global_pause_count == 0)
+		tdma_write(tdma, TEGRA_APBDMA_GENERAL,
+			   TEGRA_APBDMA_GENERAL_ENABLE);
+
+out:
 	spin_unlock(&tdma->global_lock);
 }
 
@@ -1407,6 +1430,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	dma_cap_set(DMA_PRIVATE, tdma->dma_dev.cap_mask);
 	dma_cap_set(DMA_CYCLIC, tdma->dma_dev.cap_mask);
 
+	tdma->global_pause_count = 0;
 	tdma->dma_dev.dev = &pdev->dev;
 	tdma->dma_dev.device_alloc_chan_resources =
 					tegra_dma_alloc_chan_resources;
-- 
2.1.4

Re: [PATCH 0/4] DMA: tegra-apb: Clean-up

[Jon Hunter]

Any feedback on this series?

Cheers Jon

On 06/08/15 14:32, Jon Hunter wrote:
> Some clean-up changes for the tegra-apb DMA driver.
> 
> These have been compile and boot tested for ARM and ARM64. Summary of the
> ARM results are below. ARM64 has been tested locally.
> 
> Test summary
> ------------
> 
> Build: zImage:
>     Pass: ( 2/ 2): multi_v7_defconfig, tegra_defconfig
> 
> Boot to userspace: multi_v7_defconfig:
>     Pass: ( 4/ 4): tegra114-dalmore-a04, tegra124-jetson-tk1,
>                    tegra20-trimslice, tegra30-beaver
> 
> Boot to userspace: tegra_defconfig:
>     Pass: ( 4/ 4): tegra114-dalmore-a04, tegra124-jetson-tk1,
>                    tegra20-trimslice, tegra30-beaver
> 
> Jon Hunter (4):
>   DMA: tegra-apb: Remove unused variables
>   DMA: tegra-apb: Avoid unnecessary channel base address calculation
>   DMA: tegra-apb: Remove unnecessary return statements and variables
>   DMA: tegra-apb: Simplify locking for device using global pause
> 
>  drivers/dma/tegra20-apb-dma.c | 63 ++++++++++++++++++++++++++-----------------
>  1 file changed, 38 insertions(+), 25 deletions(-)
> 

Re: [PATCH 0/4] DMA: tegra-apb: Clean-up

[Vinod Koul]

On Thu, Aug 06, 2015 at 02:32:29PM +0100, Jon Hunter wrote:
> Some clean-up changes for the tegra-apb DMA driver.

Applied after fixing subsystem name

-- 
~Vinod