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

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

[Jon Hunter]

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

Changes for V3 noted in the applicable patches.

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

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

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

Build: Image:
    Pass: ( 1/ 1): defconfig

Boot to userspace: defconfig:
    Pass: ( 1/ 1): qemu-vexpress64

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 (6):
  dmaengine: tegra-apb: Correct runtime-pm usage
  dmaengine: tegra-apb: Use dev_get_drvdata()
  dmaengine: tegra-apb: Save and restore word count
  dmaengine: tegra-apb: Only save channel state for those in use
  dmaengine: tegra-apb: Update driver to use GFP_NOWAIT
  dmaengine: tegra-apb: Free interrupts before killing tasklets

 drivers/dma/tegra20-apb-dma.c | 73 ++++++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 33 deletions(-)

-- 
2.1.4

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

[Jon Hunter]

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

Changes for V3 noted in the applicable patches.

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

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

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

Build: Image:
    Pass: ( 1/ 1): defconfig

Boot to userspace: defconfig:
    Pass: ( 1/ 1): qemu-vexpress64

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 (6):
  dmaengine: tegra-apb: Correct runtime-pm usage
  dmaengine: tegra-apb: Use dev_get_drvdata()
  dmaengine: tegra-apb: Save and restore word count
  dmaengine: tegra-apb: Only save channel state for those in use
  dmaengine: tegra-apb: Update driver to use GFP_NOWAIT
  dmaengine: tegra-apb: Free interrupts before killing tasklets

 drivers/dma/tegra20-apb-dma.c | 73 ++++++++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 33 deletions(-)

-- 
2.1.4

[PATCH V3 1/6] dmaengine: tegra-apb: Correct runtime-pm usage

[Jon Hunter]

The tegra-apb DMA driver enables runtime-pm but never calls
pm_runtime_get/put and hence the runtime-pm callbacks are never invoked.
The driver manages the clocks by directly calling clk_prepare_enable()
and clk_unprepare_disable().

Fix this by replacing the clk_prepare_enable() and clk_disable_unprepare()
with pm_runtime_get_sync() and pm_runtime_put(), respectively. Note that
the consequence of this is that if runtime-pm is disabled, then the clocks
will remain on the entire time the driver is loaded. However, if
runtime-pm is disabled, then power is not most likely not a concern.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
V3 changes:
- Removed unnecessary update to local variables in suspend/resume
V2 changes:
- Fixed return value for allocating channel
- Fixed test for return value from pm_runtime_get_sync()

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

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index c8f79dcaaee8..f68bccf55a24 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1186,10 +1186,12 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
 
 	dma_cookie_init(&tdc->dma_chan);
 	tdc->config_init = false;
-	ret = clk_prepare_enable(tdma->dma_clk);
+
+	ret = pm_runtime_get_sync(tdma->dev);
 	if (ret < 0)
-		dev_err(tdc2dev(tdc), "clk_prepare_enable failed: %d\n", ret);
-	return ret;
+		return ret;
+
+	return 0;
 }
 
 static void tegra_dma_free_chan_resources(struct dma_chan *dc)
@@ -1232,7 +1234,7 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
 		list_del(&sg_req->node);
 		kfree(sg_req);
 	}
-	clk_disable_unprepare(tdma->dma_clk);
+	pm_runtime_put(tdma->dev);
 
 	tdc->slave_id = 0;
 }
@@ -1356,20 +1358,14 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	spin_lock_init(&tdma->global_lock);
 
 	pm_runtime_enable(&pdev->dev);
-	if (!pm_runtime_enabled(&pdev->dev)) {
+	if (!pm_runtime_enabled(&pdev->dev))
 		ret = tegra_dma_runtime_resume(&pdev->dev);
-		if (ret) {
-			dev_err(&pdev->dev, "dma_runtime_resume failed %d\n",
-				ret);
-			goto err_pm_disable;
-		}
-	}
+	else
+		ret = pm_runtime_get_sync(&pdev->dev);
 
-	/* Enable clock before accessing registers */
-	ret = clk_prepare_enable(tdma->dma_clk);
 	if (ret < 0) {
-		dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
-		goto err_pm_disable;
+		pm_runtime_disable(&pdev->dev);
+		return ret;
 	}
 
 	/* Reset DMA controller */
@@ -1382,7 +1378,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0);
 	tdma_write(tdma, TEGRA_APBDMA_IRQ_MASK_SET, 0xFFFFFFFFul);
 
-	clk_disable_unprepare(tdma->dma_clk);
+	pm_runtime_put(&pdev->dev);
 
 	INIT_LIST_HEAD(&tdma->dma_dev.channels);
 	for (i = 0; i < cdata->nr_channels; i++) {
@@ -1485,7 +1481,6 @@ err_irq:
 		tasklet_kill(&tdc->tasklet);
 	}
 
-err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
 	if (!pm_runtime_status_suspended(&pdev->dev))
 		tegra_dma_runtime_suspend(&pdev->dev);
@@ -1543,7 +1538,7 @@ static int tegra_dma_pm_suspend(struct device *dev)
 	int ret;
 
 	/* Enable clock before accessing register */
-	ret = tegra_dma_runtime_resume(dev);
+	ret = pm_runtime_get_sync(dev);
 	if (ret < 0)
 		return ret;
 
@@ -1560,7 +1555,7 @@ static int tegra_dma_pm_suspend(struct device *dev)
 	}
 
 	/* Disable clock */
-	tegra_dma_runtime_suspend(dev);
+	pm_runtime_put(dev);
 	return 0;
 }
 
@@ -1571,7 +1566,7 @@ static int tegra_dma_pm_resume(struct device *dev)
 	int ret;
 
 	/* Enable clock before accessing register */
-	ret = tegra_dma_runtime_resume(dev);
+	ret = pm_runtime_get_sync(dev);
 	if (ret < 0)
 		return ret;
 
@@ -1592,16 +1587,14 @@ static int tegra_dma_pm_resume(struct device *dev)
 	}
 
 	/* Disable clock */
-	tegra_dma_runtime_suspend(dev);
+	pm_runtime_put(dev);
 	return 0;
 }
 #endif
 
 static const struct dev_pm_ops tegra_dma_dev_pm_ops = {
-#ifdef CONFIG_PM
-	.runtime_suspend = tegra_dma_runtime_suspend,
-	.runtime_resume = tegra_dma_runtime_resume,
-#endif
+	SET_RUNTIME_PM_OPS(tegra_dma_runtime_suspend, tegra_dma_runtime_resume,
+			   NULL)
 	SET_SYSTEM_SLEEP_PM_OPS(tegra_dma_pm_suspend, tegra_dma_pm_resume)
 };
 
-- 
2.1.4

[PATCH V3 1/6] dmaengine: tegra-apb: Correct runtime-pm usage

[Jon Hunter]

The tegra-apb DMA driver enables runtime-pm but never calls
pm_runtime_get/put and hence the runtime-pm callbacks are never invoked.
The driver manages the clocks by directly calling clk_prepare_enable()
and clk_unprepare_disable().

Fix this by replacing the clk_prepare_enable() and clk_disable_unprepare()
with pm_runtime_get_sync() and pm_runtime_put(), respectively. Note that
the consequence of this is that if runtime-pm is disabled, then the clocks
will remain on the entire time the driver is loaded. However, if
runtime-pm is disabled, then power is not most likely not a concern.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
V3 changes:
- Removed unnecessary update to local variables in suspend/resume
V2 changes:
- Fixed return value for allocating channel
- Fixed test for return value from pm_runtime_get_sync()

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

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index c8f79dcaaee8..f68bccf55a24 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1186,10 +1186,12 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
 
 	dma_cookie_init(&tdc->dma_chan);
 	tdc->config_init = false;
-	ret = clk_prepare_enable(tdma->dma_clk);
+
+	ret = pm_runtime_get_sync(tdma->dev);
 	if (ret < 0)
-		dev_err(tdc2dev(tdc), "clk_prepare_enable failed: %d\n", ret);
-	return ret;
+		return ret;
+
+	return 0;
 }
 
 static void tegra_dma_free_chan_resources(struct dma_chan *dc)
@@ -1232,7 +1234,7 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
 		list_del(&sg_req->node);
 		kfree(sg_req);
 	}
-	clk_disable_unprepare(tdma->dma_clk);
+	pm_runtime_put(tdma->dev);
 
 	tdc->slave_id = 0;
 }
@@ -1356,20 +1358,14 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	spin_lock_init(&tdma->global_lock);
 
 	pm_runtime_enable(&pdev->dev);
-	if (!pm_runtime_enabled(&pdev->dev)) {
+	if (!pm_runtime_enabled(&pdev->dev))
 		ret = tegra_dma_runtime_resume(&pdev->dev);
-		if (ret) {
-			dev_err(&pdev->dev, "dma_runtime_resume failed %d\n",
-				ret);
-			goto err_pm_disable;
-		}
-	}
+	else
+		ret = pm_runtime_get_sync(&pdev->dev);
 
-	/* Enable clock before accessing registers */
-	ret = clk_prepare_enable(tdma->dma_clk);
 	if (ret < 0) {
-		dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
-		goto err_pm_disable;
+		pm_runtime_disable(&pdev->dev);
+		return ret;
 	}
 
 	/* Reset DMA controller */
@@ -1382,7 +1378,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
 	tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0);
 	tdma_write(tdma, TEGRA_APBDMA_IRQ_MASK_SET, 0xFFFFFFFFul);
 
-	clk_disable_unprepare(tdma->dma_clk);
+	pm_runtime_put(&pdev->dev);
 
 	INIT_LIST_HEAD(&tdma->dma_dev.channels);
 	for (i = 0; i < cdata->nr_channels; i++) {
@@ -1485,7 +1481,6 @@ err_irq:
 		tasklet_kill(&tdc->tasklet);
 	}
 
-err_pm_disable:
 	pm_runtime_disable(&pdev->dev);
 	if (!pm_runtime_status_suspended(&pdev->dev))
 		tegra_dma_runtime_suspend(&pdev->dev);
@@ -1543,7 +1538,7 @@ static int tegra_dma_pm_suspend(struct device *dev)
 	int ret;
 
 	/* Enable clock before accessing register */
-	ret = tegra_dma_runtime_resume(dev);
+	ret = pm_runtime_get_sync(dev);
 	if (ret < 0)
 		return ret;
 
@@ -1560,7 +1555,7 @@ static int tegra_dma_pm_suspend(struct device *dev)
 	}
 
 	/* Disable clock */
-	tegra_dma_runtime_suspend(dev);
+	pm_runtime_put(dev);
 	return 0;
 }
 
@@ -1571,7 +1566,7 @@ static int tegra_dma_pm_resume(struct device *dev)
 	int ret;
 
 	/* Enable clock before accessing register */
-	ret = tegra_dma_runtime_resume(dev);
+	ret = pm_runtime_get_sync(dev);
 	if (ret < 0)
 		return ret;
 
@@ -1592,16 +1587,14 @@ static int tegra_dma_pm_resume(struct device *dev)
 	}
 
 	/* Disable clock */
-	tegra_dma_runtime_suspend(dev);
+	pm_runtime_put(dev);
 	return 0;
 }
 #endif
 
 static const struct dev_pm_ops tegra_dma_dev_pm_ops = {
-#ifdef CONFIG_PM
-	.runtime_suspend = tegra_dma_runtime_suspend,
-	.runtime_resume = tegra_dma_runtime_resume,
-#endif
+	SET_RUNTIME_PM_OPS(tegra_dma_runtime_suspend, tegra_dma_runtime_resume,
+			   NULL)
 	SET_SYSTEM_SLEEP_PM_OPS(tegra_dma_pm_suspend, tegra_dma_pm_resume)
 };
 
-- 
2.1.4

[PATCH V3 2/6] dmaengine: tegra-apb: Use dev_get_drvdata()

[Jon Hunter]

In the tegra_dma_runtime_suspend/resume functions, the pdev structure
is not needed, and so just call dev_get_drvdata() to get the device
data structure.

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

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index f68bccf55a24..355dbc354818 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1509,8 +1509,7 @@ static int tegra_dma_remove(struct platform_device *pdev)
 
 static int tegra_dma_runtime_suspend(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct tegra_dma *tdma = platform_get_drvdata(pdev);
+	struct tegra_dma *tdma = dev_get_drvdata(dev);
 
 	clk_disable_unprepare(tdma->dma_clk);
 	return 0;
@@ -1518,8 +1517,7 @@ static int tegra_dma_runtime_suspend(struct device *dev)
 
 static int tegra_dma_runtime_resume(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct tegra_dma *tdma = platform_get_drvdata(pdev);
+	struct tegra_dma *tdma = dev_get_drvdata(dev);
 	int ret;
 
 	ret = clk_prepare_enable(tdma->dma_clk);
-- 
2.1.4

[PATCH V3 2/6] dmaengine: tegra-apb: Use dev_get_drvdata()

[Jon Hunter]

In the tegra_dma_runtime_suspend/resume functions, the pdev structure
is not needed, and so just call dev_get_drvdata() to get the device
data structure.

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

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index f68bccf55a24..355dbc354818 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1509,8 +1509,7 @@ static int tegra_dma_remove(struct platform_device *pdev)
 
 static int tegra_dma_runtime_suspend(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct tegra_dma *tdma = platform_get_drvdata(pdev);
+	struct tegra_dma *tdma = dev_get_drvdata(dev);
 
 	clk_disable_unprepare(tdma->dma_clk);
 	return 0;
@@ -1518,8 +1517,7 @@ static int tegra_dma_runtime_suspend(struct device *dev)
 
 static int tegra_dma_runtime_resume(struct device *dev)
 {
-	struct platform_device *pdev = to_platform_device(dev);
-	struct tegra_dma *tdma = platform_get_drvdata(pdev);
+	struct tegra_dma *tdma = dev_get_drvdata(dev);
 	int ret;
 
 	ret = clk_prepare_enable(tdma->dma_clk);
-- 
2.1.4

[PATCH V3 3/6] dmaengine: tegra-apb: Save and restore word count

[Jon Hunter]

Newer tegra devices have a separate word count register per channel that
contains the number of words to be transferred. This register is not
saved or restored by the suspend/resume helpers for these newer devices
and so ensure that it is.

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

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 355dbc354818..18a561376c63 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1550,6 +1550,9 @@ static int tegra_dma_pm_suspend(struct device *dev)
 		ch_reg->apb_ptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_APBPTR);
 		ch_reg->ahb_seq = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBSEQ);
 		ch_reg->apb_seq = tdc_read(tdc, TEGRA_APBDMA_CHAN_APBSEQ);
+		if (tdma->chip_data->support_separate_wcount_reg)
+			ch_reg->wcount = tdc_read(tdc,
+						  TEGRA_APBDMA_CHAN_WCOUNT);
 	}
 
 	/* Disable clock */
@@ -1576,6 +1579,9 @@ static int tegra_dma_pm_resume(struct device *dev)
 		struct tegra_dma_channel *tdc = &tdma->channels[i];
 		struct tegra_dma_channel_regs *ch_reg = &tdc->channel_reg;
 
+		if (tdma->chip_data->support_separate_wcount_reg)
+			tdc_write(tdc, TEGRA_APBDMA_CHAN_WCOUNT,
+				  ch_reg->wcount);
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_APBSEQ, ch_reg->apb_seq);
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_APBPTR, ch_reg->apb_ptr);
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_AHBSEQ, ch_reg->ahb_seq);
-- 
2.1.4

[PATCH V3 3/6] dmaengine: tegra-apb: Save and restore word count

[Jon Hunter]

Newer tegra devices have a separate word count register per channel that
contains the number of words to be transferred. This register is not
saved or restored by the suspend/resume helpers for these newer devices
and so ensure that it is.

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

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 355dbc354818..18a561376c63 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1550,6 +1550,9 @@ static int tegra_dma_pm_suspend(struct device *dev)
 		ch_reg->apb_ptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_APBPTR);
 		ch_reg->ahb_seq = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBSEQ);
 		ch_reg->apb_seq = tdc_read(tdc, TEGRA_APBDMA_CHAN_APBSEQ);
+		if (tdma->chip_data->support_separate_wcount_reg)
+			ch_reg->wcount = tdc_read(tdc,
+						  TEGRA_APBDMA_CHAN_WCOUNT);
 	}
 
 	/* Disable clock */
@@ -1576,6 +1579,9 @@ static int tegra_dma_pm_resume(struct device *dev)
 		struct tegra_dma_channel *tdc = &tdma->channels[i];
 		struct tegra_dma_channel_regs *ch_reg = &tdc->channel_reg;
 
+		if (tdma->chip_data->support_separate_wcount_reg)
+			tdc_write(tdc, TEGRA_APBDMA_CHAN_WCOUNT,
+				  ch_reg->wcount);
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_APBSEQ, ch_reg->apb_seq);
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_APBPTR, ch_reg->apb_ptr);
 		tdc_write(tdc, TEGRA_APBDMA_CHAN_AHBSEQ, ch_reg->ahb_seq);
-- 
2.1.4

[PATCH V3 4/6] dmaengine: tegra-apb: Only save channel state for those in use

[Jon Hunter]

Currently the tegra-apb DMA driver suspend/resume helpers, save and
restore the registers for all channels regardless of whether they are
in use or not. Change this so that only channels that have been
allocated and configured are saved and restored.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
V3 changes:
- Updated comment to be on a single line

 drivers/dma/tegra20-apb-dma.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 18a561376c63..d4cabd6931e5 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1545,6 +1545,10 @@ static int tegra_dma_pm_suspend(struct device *dev)
 		struct tegra_dma_channel *tdc = &tdma->channels[i];
 		struct tegra_dma_channel_regs *ch_reg = &tdc->channel_reg;
 
+		/* Only save the state of DMA channels that are in use */
+		if (!tdc->config_init)
+			continue;
+
 		ch_reg->csr = tdc_read(tdc, TEGRA_APBDMA_CHAN_CSR);
 		ch_reg->ahb_ptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR);
 		ch_reg->apb_ptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_APBPTR);
@@ -1579,6 +1583,10 @@ static int tegra_dma_pm_resume(struct device *dev)
 		struct tegra_dma_channel *tdc = &tdma->channels[i];
 		struct tegra_dma_channel_regs *ch_reg = &tdc->channel_reg;
 
+		/* Only restore the state of DMA channels that are in use */
+		if (!tdc->config_init)
+			continue;
+
 		if (tdma->chip_data->support_separate_wcount_reg)
 			tdc_write(tdc, TEGRA_APBDMA_CHAN_WCOUNT,
 				  ch_reg->wcount);
-- 
2.1.4

[PATCH V3 4/6] dmaengine: tegra-apb: Only save channel state for those in use

[Jon Hunter]

Currently the tegra-apb DMA driver suspend/resume helpers, save and
restore the registers for all channels regardless of whether they are
in use or not. Change this so that only channels that have been
allocated and configured are saved and restored.

Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
---
V3 changes:
- Updated comment to be on a single line

 drivers/dma/tegra20-apb-dma.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 18a561376c63..d4cabd6931e5 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1545,6 +1545,10 @@ static int tegra_dma_pm_suspend(struct device *dev)
 		struct tegra_dma_channel *tdc = &tdma->channels[i];
 		struct tegra_dma_channel_regs *ch_reg = &tdc->channel_reg;
 
+		/* Only save the state of DMA channels that are in use */
+		if (!tdc->config_init)
+			continue;
+
 		ch_reg->csr = tdc_read(tdc, TEGRA_APBDMA_CHAN_CSR);
 		ch_reg->ahb_ptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_AHBPTR);
 		ch_reg->apb_ptr = tdc_read(tdc, TEGRA_APBDMA_CHAN_APBPTR);
@@ -1579,6 +1583,10 @@ static int tegra_dma_pm_resume(struct device *dev)
 		struct tegra_dma_channel *tdc = &tdma->channels[i];
 		struct tegra_dma_channel_regs *ch_reg = &tdc->channel_reg;
 
+		/* Only restore the state of DMA channels that are in use */
+		if (!tdc->config_init)
+			continue;
+
 		if (tdma->chip_data->support_separate_wcount_reg)
 			tdc_write(tdc, TEGRA_APBDMA_CHAN_WCOUNT,
 				  ch_reg->wcount);
-- 
2.1.4

[PATCH V3 5/6] dmaengine: tegra-apb: Update driver to use GFP_NOWAIT

[Jon Hunter]

The tegra20-apb-dma driver currently uses the flag GFP_ATOMIC when
allocating memory for structures used in conjunction with the DMA
descriptors. It is preferred that dmaengine drivers use GFP_NOWAIT
instead and so the emergency memory pool will not be used by these
drivers.

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

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index d4cabd6931e5..754c1f54d4fe 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -296,7 +296,7 @@ static struct tegra_dma_desc *tegra_dma_desc_get(
 	spin_unlock_irqrestore(&tdc->lock, flags);
 
 	/* Allocate DMA desc */
-	dma_desc = kzalloc(sizeof(*dma_desc), GFP_ATOMIC);
+	dma_desc = kzalloc(sizeof(*dma_desc), GFP_NOWAIT);
 	if (!dma_desc) {
 		dev_err(tdc2dev(tdc), "dma_desc alloc failed\n");
 		return NULL;
@@ -336,7 +336,7 @@ static struct tegra_dma_sg_req *tegra_dma_sg_req_get(
 	}
 	spin_unlock_irqrestore(&tdc->lock, flags);
 
-	sg_req = kzalloc(sizeof(struct tegra_dma_sg_req), GFP_ATOMIC);
+	sg_req = kzalloc(sizeof(struct tegra_dma_sg_req), GFP_NOWAIT);
 	if (!sg_req)
 		dev_err(tdc2dev(tdc), "sg_req alloc failed\n");
 	return sg_req;
-- 
2.1.4

[PATCH V3 5/6] dmaengine: tegra-apb: Update driver to use GFP_NOWAIT

[Jon Hunter]

The tegra20-apb-dma driver currently uses the flag GFP_ATOMIC when
allocating memory for structures used in conjunction with the DMA
descriptors. It is preferred that dmaengine drivers use GFP_NOWAIT
instead and so the emergency memory pool will not be used by these
drivers.

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

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index d4cabd6931e5..754c1f54d4fe 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -296,7 +296,7 @@ static struct tegra_dma_desc *tegra_dma_desc_get(
 	spin_unlock_irqrestore(&tdc->lock, flags);
 
 	/* Allocate DMA desc */
-	dma_desc = kzalloc(sizeof(*dma_desc), GFP_ATOMIC);
+	dma_desc = kzalloc(sizeof(*dma_desc), GFP_NOWAIT);
 	if (!dma_desc) {
 		dev_err(tdc2dev(tdc), "dma_desc alloc failed\n");
 		return NULL;
@@ -336,7 +336,7 @@ static struct tegra_dma_sg_req *tegra_dma_sg_req_get(
 	}
 	spin_unlock_irqrestore(&tdc->lock, flags);
 
-	sg_req = kzalloc(sizeof(struct tegra_dma_sg_req), GFP_ATOMIC);
+	sg_req = kzalloc(sizeof(struct tegra_dma_sg_req), GFP_NOWAIT);
 	if (!sg_req)
 		dev_err(tdc2dev(tdc), "sg_req alloc failed\n");
 	return sg_req;
-- 
2.1.4

[PATCH V3 6/6] dmaengine: tegra-apb: Free interrupts before killing tasklets

[Jon Hunter]

On probe failure or driver removal, before killing any tasklets, ensure
that the channel interrupt is freed to ensure that another channel
interrupt cannot occur and schedule the tasklet again.

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

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 754c1f54d4fe..935da8192f59 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1396,8 +1396,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
 		}
 		tdc->irq = res->start;
 		snprintf(tdc->name, sizeof(tdc->name), "apbdma.%d", i);
-		ret = devm_request_irq(&pdev->dev, tdc->irq,
-				tegra_dma_isr, 0, tdc->name, tdc);
+		ret = request_irq(tdc->irq, tegra_dma_isr, 0, tdc->name, tdc);
 		if (ret) {
 			dev_err(&pdev->dev,
 				"request_irq failed with err %d channel %d\n",
@@ -1478,6 +1477,8 @@ err_unregister_dma_dev:
 err_irq:
 	while (--i >= 0) {
 		struct tegra_dma_channel *tdc = &tdma->channels[i];
+
+		free_irq(tdc->irq, tdc);
 		tasklet_kill(&tdc->tasklet);
 	}
 
@@ -1497,6 +1498,7 @@ static int tegra_dma_remove(struct platform_device *pdev)
 
 	for (i = 0; i < tdma->chip_data->nr_channels; ++i) {
 		tdc = &tdma->channels[i];
+		free_irq(tdc->irq, tdc);
 		tasklet_kill(&tdc->tasklet);
 	}
 
-- 
2.1.4

[PATCH V3 6/6] dmaengine: tegra-apb: Free interrupts before killing tasklets

[Jon Hunter]

On probe failure or driver removal, before killing any tasklets, ensure
that the channel interrupt is freed to ensure that another channel
interrupt cannot occur and schedule the tasklet again.

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

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 754c1f54d4fe..935da8192f59 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1396,8 +1396,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
 		}
 		tdc->irq = res->start;
 		snprintf(tdc->name, sizeof(tdc->name), "apbdma.%d", i);
-		ret = devm_request_irq(&pdev->dev, tdc->irq,
-				tegra_dma_isr, 0, tdc->name, tdc);
+		ret = request_irq(tdc->irq, tegra_dma_isr, 0, tdc->name, tdc);
 		if (ret) {
 			dev_err(&pdev->dev,
 				"request_irq failed with err %d channel %d\n",
@@ -1478,6 +1477,8 @@ err_unregister_dma_dev:
 err_irq:
 	while (--i >= 0) {
 		struct tegra_dma_channel *tdc = &tdma->channels[i];
+
+		free_irq(tdc->irq, tdc);
 		tasklet_kill(&tdc->tasklet);
 	}
 
@@ -1497,6 +1498,7 @@ static int tegra_dma_remove(struct platform_device *pdev)
 
 	for (i = 0; i < tdma->chip_data->nr_channels; ++i) {
 		tdc = &tdma->channels[i];
+		free_irq(tdc->irq, tdc);
 		tasklet_kill(&tdc->tasklet);
 	}
 
-- 
2.1.4

Re: [PATCH V3 1/6] dmaengine: tegra-apb: Correct runtime-pm usage

[Andy Shevchenko]

On Fri, Nov 13, 2015 at 6:39 PM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> The tegra-apb DMA driver enables runtime-pm but never calls
> pm_runtime_get/put and hence the runtime-pm callbacks are never invoked.
> The driver manages the clocks by directly calling clk_prepare_enable()
> and clk_unprepare_disable().
>
> Fix this by replacing the clk_prepare_enable() and clk_disable_unprepare()
> with pm_runtime_get_sync() and pm_runtime_put(), respectively. Note that
> the consequence of this is that if runtime-pm is disabled, then the clocks
> will remain on the entire time the driver is loaded. However, if
> runtime-pm is disabled, then power is not most likely not a concern.
>

Have you tested these changes somehow?

See my comments below.

> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> V3 changes:
> - Removed unnecessary update to local variables in suspend/resume
> V2 changes:
> - Fixed return value for allocating channel
> - Fixed test for return value from pm_runtime_get_sync()
>
> drivers/dma/tegra20-apb-dma.c | 43 ++++++++++++++++++-------------------------
>  1 file changed, 18 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index c8f79dcaaee8..f68bccf55a24 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -1186,10 +1186,12 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
>
>         dma_cookie_init(&tdc->dma_chan);
>         tdc->config_init = false;
> -       ret = clk_prepare_enable(tdma->dma_clk);
> +
> +       ret = pm_runtime_get_sync(tdma->dev);
>         if (ret < 0)
> -               dev_err(tdc2dev(tdc), "clk_prepare_enable failed: %d\n", ret);
> -       return ret;
> +               return ret;
> +

> +       return 0;

This is a non-reachable code.

>  }
>
>  static void tegra_dma_free_chan_resources(struct dma_chan *dc)
> @@ -1232,7 +1234,7 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
>                 list_del(&sg_req->node);
>                 kfree(sg_req);
>         }
> -       clk_disable_unprepare(tdma->dma_clk);
> +       pm_runtime_put(tdma->dev);
>
>         tdc->slave_id = 0;
>  }
> @@ -1356,20 +1358,14 @@ static int tegra_dma_probe(struct platform_device *pdev)
>         spin_lock_init(&tdma->global_lock);
>
>         pm_runtime_enable(&pdev->dev);
> -       if (!pm_runtime_enabled(&pdev->dev)) {
> +       if (!pm_runtime_enabled(&pdev->dev))
>                 ret = tegra_dma_runtime_resume(&pdev->dev);

I didn't check, but does this bring device to powered on state?

> -               if (ret) {
> -                       dev_err(&pdev->dev, "dma_runtime_resume failed %d\n",
> -                               ret);
> -                       goto err_pm_disable;
> -               }
> -       }
> +       else
> +               ret = pm_runtime_get_sync(&pdev->dev);
>
> -       /* Enable clock before accessing registers */
> -       ret = clk_prepare_enable(tdma->dma_clk);
>         if (ret < 0) {
> -               dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
> -               goto err_pm_disable;
> +               pm_runtime_disable(&pdev->dev);
> +               return ret;
>         }
>
>         /* Reset DMA controller */
> @@ -1382,7 +1378,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
>         tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0);
>         tdma_write(tdma, TEGRA_APBDMA_IRQ_MASK_SET, 0xFFFFFFFFul);
>
> -       clk_disable_unprepare(tdma->dma_clk);
> +       pm_runtime_put(&pdev->dev);
>
>         INIT_LIST_HEAD(&tdma->dma_dev.channels);
>         for (i = 0; i < cdata->nr_channels; i++) {
> @@ -1485,7 +1481,6 @@ err_irq:
>                 tasklet_kill(&tdc->tasklet);
>         }
>
> -err_pm_disable:
>         pm_runtime_disable(&pdev->dev);
>         if (!pm_runtime_status_suspended(&pdev->dev))
>                 tegra_dma_runtime_suspend(&pdev->dev);
> @@ -1543,7 +1538,7 @@ static int tegra_dma_pm_suspend(struct device *dev)
>         int ret;
>
>         /* Enable clock before accessing register */
> -       ret = tegra_dma_runtime_resume(dev);
> +       ret = pm_runtime_get_sync(dev);
>         if (ret < 0)
>                 return ret;
>
> @@ -1560,7 +1555,7 @@ static int tegra_dma_pm_suspend(struct device *dev)
>         }
>
>         /* Disable clock */
> -       tegra_dma_runtime_suspend(dev);
> +       pm_runtime_put(dev);
>         return 0;
>  }
>
> @@ -1571,7 +1566,7 @@ static int tegra_dma_pm_resume(struct device *dev)
>         int ret;
>
>         /* Enable clock before accessing register */
> -       ret = tegra_dma_runtime_resume(dev);
> +       ret = pm_runtime_get_sync(dev);
>         if (ret < 0)
>                 return ret;
>
> @@ -1592,16 +1587,14 @@ static int tegra_dma_pm_resume(struct device *dev)
>         }
>
>         /* Disable clock */
> -       tegra_dma_runtime_suspend(dev);
> +       pm_runtime_put(dev);
>         return 0;
>  }
>  #endif
>
>  static const struct dev_pm_ops tegra_dma_dev_pm_ops = {
> -#ifdef CONFIG_PM
> -       .runtime_suspend = tegra_dma_runtime_suspend,
> -       .runtime_resume = tegra_dma_runtime_resume,
> -#endif
> +       SET_RUNTIME_PM_OPS(tegra_dma_runtime_suspend, tegra_dma_runtime_resume,
> +                          NULL)
>         SET_SYSTEM_SLEEP_PM_OPS(tegra_dma_pm_suspend, tegra_dma_pm_resume)
>  };
>
> --
> 2.1.4
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V3 1/6] dmaengine: tegra-apb: Correct runtime-pm usage

[Andy Shevchenko]

On Fri, Nov 13, 2015 at 6:39 PM, Jon Hunter <jonathanh@nvidia.com> wrote:
> The tegra-apb DMA driver enables runtime-pm but never calls
> pm_runtime_get/put and hence the runtime-pm callbacks are never invoked.
> The driver manages the clocks by directly calling clk_prepare_enable()
> and clk_unprepare_disable().
>
> Fix this by replacing the clk_prepare_enable() and clk_disable_unprepare()
> with pm_runtime_get_sync() and pm_runtime_put(), respectively. Note that
> the consequence of this is that if runtime-pm is disabled, then the clocks
> will remain on the entire time the driver is loaded. However, if
> runtime-pm is disabled, then power is not most likely not a concern.
>

Have you tested these changes somehow?

See my comments below.

> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
> V3 changes:
> - Removed unnecessary update to local variables in suspend/resume
> V2 changes:
> - Fixed return value for allocating channel
> - Fixed test for return value from pm_runtime_get_sync()
>
> drivers/dma/tegra20-apb-dma.c | 43 ++++++++++++++++++-------------------------
>  1 file changed, 18 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index c8f79dcaaee8..f68bccf55a24 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -1186,10 +1186,12 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
>
>         dma_cookie_init(&tdc->dma_chan);
>         tdc->config_init = false;
> -       ret = clk_prepare_enable(tdma->dma_clk);
> +
> +       ret = pm_runtime_get_sync(tdma->dev);
>         if (ret < 0)
> -               dev_err(tdc2dev(tdc), "clk_prepare_enable failed: %d\n", ret);
> -       return ret;
> +               return ret;
> +

> +       return 0;

This is a non-reachable code.

>  }
>
>  static void tegra_dma_free_chan_resources(struct dma_chan *dc)
> @@ -1232,7 +1234,7 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
>                 list_del(&sg_req->node);
>                 kfree(sg_req);
>         }
> -       clk_disable_unprepare(tdma->dma_clk);
> +       pm_runtime_put(tdma->dev);
>
>         tdc->slave_id = 0;
>  }
> @@ -1356,20 +1358,14 @@ static int tegra_dma_probe(struct platform_device *pdev)
>         spin_lock_init(&tdma->global_lock);
>
>         pm_runtime_enable(&pdev->dev);
> -       if (!pm_runtime_enabled(&pdev->dev)) {
> +       if (!pm_runtime_enabled(&pdev->dev))
>                 ret = tegra_dma_runtime_resume(&pdev->dev);

I didn't check, but does this bring device to powered on state?

> -               if (ret) {
> -                       dev_err(&pdev->dev, "dma_runtime_resume failed %d\n",
> -                               ret);
> -                       goto err_pm_disable;
> -               }
> -       }
> +       else
> +               ret = pm_runtime_get_sync(&pdev->dev);
>
> -       /* Enable clock before accessing registers */
> -       ret = clk_prepare_enable(tdma->dma_clk);
>         if (ret < 0) {
> -               dev_err(&pdev->dev, "clk_prepare_enable failed: %d\n", ret);
> -               goto err_pm_disable;
> +               pm_runtime_disable(&pdev->dev);
> +               return ret;
>         }
>
>         /* Reset DMA controller */
> @@ -1382,7 +1378,7 @@ static int tegra_dma_probe(struct platform_device *pdev)
>         tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0);
>         tdma_write(tdma, TEGRA_APBDMA_IRQ_MASK_SET, 0xFFFFFFFFul);
>
> -       clk_disable_unprepare(tdma->dma_clk);
> +       pm_runtime_put(&pdev->dev);
>
>         INIT_LIST_HEAD(&tdma->dma_dev.channels);
>         for (i = 0; i < cdata->nr_channels; i++) {
> @@ -1485,7 +1481,6 @@ err_irq:
>                 tasklet_kill(&tdc->tasklet);
>         }
>
> -err_pm_disable:
>         pm_runtime_disable(&pdev->dev);
>         if (!pm_runtime_status_suspended(&pdev->dev))
>                 tegra_dma_runtime_suspend(&pdev->dev);
> @@ -1543,7 +1538,7 @@ static int tegra_dma_pm_suspend(struct device *dev)
>         int ret;
>
>         /* Enable clock before accessing register */
> -       ret = tegra_dma_runtime_resume(dev);
> +       ret = pm_runtime_get_sync(dev);
>         if (ret < 0)
>                 return ret;
>
> @@ -1560,7 +1555,7 @@ static int tegra_dma_pm_suspend(struct device *dev)
>         }
>
>         /* Disable clock */
> -       tegra_dma_runtime_suspend(dev);
> +       pm_runtime_put(dev);
>         return 0;
>  }
>
> @@ -1571,7 +1566,7 @@ static int tegra_dma_pm_resume(struct device *dev)
>         int ret;
>
>         /* Enable clock before accessing register */
> -       ret = tegra_dma_runtime_resume(dev);
> +       ret = pm_runtime_get_sync(dev);
>         if (ret < 0)
>                 return ret;
>
> @@ -1592,16 +1587,14 @@ static int tegra_dma_pm_resume(struct device *dev)
>         }
>
>         /* Disable clock */
> -       tegra_dma_runtime_suspend(dev);
> +       pm_runtime_put(dev);
>         return 0;
>  }
>  #endif
>
>  static const struct dev_pm_ops tegra_dma_dev_pm_ops = {
> -#ifdef CONFIG_PM
> -       .runtime_suspend = tegra_dma_runtime_suspend,
> -       .runtime_resume = tegra_dma_runtime_resume,
> -#endif
> +       SET_RUNTIME_PM_OPS(tegra_dma_runtime_suspend, tegra_dma_runtime_resume,
> +                          NULL)
>         SET_SYSTEM_SLEEP_PM_OPS(tegra_dma_pm_suspend, tegra_dma_pm_resume)
>  };
>
> --
> 2.1.4
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V3 1/6] dmaengine: tegra-apb: Correct runtime-pm usage

[Jon Hunter]

On 14/11/15 13:27, Andy Shevchenko wrote:
> On Fri, Nov 13, 2015 at 6:39 PM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> The tegra-apb DMA driver enables runtime-pm but never calls
>> pm_runtime_get/put and hence the runtime-pm callbacks are never invoked.
>> The driver manages the clocks by directly calling clk_prepare_enable()
>> and clk_unprepare_disable().
>>
>> Fix this by replacing the clk_prepare_enable() and clk_disable_unprepare()
>> with pm_runtime_get_sync() and pm_runtime_put(), respectively. Note that
>> the consequence of this is that if runtime-pm is disabled, then the clocks
>> will remain on the entire time the driver is loaded. However, if
>> runtime-pm is disabled, then power is not most likely not a concern.
>>
> 
> Have you tested these changes somehow?

Yes.

> See my comments below.
> 
>> Signed-off-by: Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>> V3 changes:
>> - Removed unnecessary update to local variables in suspend/resume
>> V2 changes:
>> - Fixed return value for allocating channel
>> - Fixed test for return value from pm_runtime_get_sync()
>>
>> drivers/dma/tegra20-apb-dma.c | 43 ++++++++++++++++++-------------------------
>>  1 file changed, 18 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>> index c8f79dcaaee8..f68bccf55a24 100644
>> --- a/drivers/dma/tegra20-apb-dma.c
>> +++ b/drivers/dma/tegra20-apb-dma.c
>> @@ -1186,10 +1186,12 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
>>
>>         dma_cookie_init(&tdc->dma_chan);
>>         tdc->config_init = false;
>> -       ret = clk_prepare_enable(tdma->dma_clk);
>> +
>> +       ret = pm_runtime_get_sync(tdma->dev);
>>         if (ret < 0)
>> -               dev_err(tdc2dev(tdc), "clk_prepare_enable failed: %d\n", ret);
>> -       return ret;
>> +               return ret;
>> +
> 
>> +       return 0;
> 
> This is a non-reachable code.

No it is not.

>>  }
>>
>>  static void tegra_dma_free_chan_resources(struct dma_chan *dc)
>> @@ -1232,7 +1234,7 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
>>                 list_del(&sg_req->node);
>>                 kfree(sg_req);
>>         }
>> -       clk_disable_unprepare(tdma->dma_clk);
>> +       pm_runtime_put(tdma->dev);
>>
>>         tdc->slave_id = 0;
>>  }
>> @@ -1356,20 +1358,14 @@ static int tegra_dma_probe(struct platform_device *pdev)
>>         spin_lock_init(&tdma->global_lock);
>>
>>         pm_runtime_enable(&pdev->dev);
>> -       if (!pm_runtime_enabled(&pdev->dev)) {
>> +       if (!pm_runtime_enabled(&pdev->dev))
>>                 ret = tegra_dma_runtime_resume(&pdev->dev);
> 
> I didn't check, but does this bring device to powered on state?

Yes.

Jon

Re: [PATCH V3 1/6] dmaengine: tegra-apb: Correct runtime-pm usage

[Jon Hunter]

On 14/11/15 13:27, Andy Shevchenko wrote:
> On Fri, Nov 13, 2015 at 6:39 PM, Jon Hunter <jonathanh@nvidia.com> wrote:
>> The tegra-apb DMA driver enables runtime-pm but never calls
>> pm_runtime_get/put and hence the runtime-pm callbacks are never invoked.
>> The driver manages the clocks by directly calling clk_prepare_enable()
>> and clk_unprepare_disable().
>>
>> Fix this by replacing the clk_prepare_enable() and clk_disable_unprepare()
>> with pm_runtime_get_sync() and pm_runtime_put(), respectively. Note that
>> the consequence of this is that if runtime-pm is disabled, then the clocks
>> will remain on the entire time the driver is loaded. However, if
>> runtime-pm is disabled, then power is not most likely not a concern.
>>
> 
> Have you tested these changes somehow?

Yes.

> See my comments below.
> 
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>> V3 changes:
>> - Removed unnecessary update to local variables in suspend/resume
>> V2 changes:
>> - Fixed return value for allocating channel
>> - Fixed test for return value from pm_runtime_get_sync()
>>
>> drivers/dma/tegra20-apb-dma.c | 43 ++++++++++++++++++-------------------------
>>  1 file changed, 18 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>> index c8f79dcaaee8..f68bccf55a24 100644
>> --- a/drivers/dma/tegra20-apb-dma.c
>> +++ b/drivers/dma/tegra20-apb-dma.c
>> @@ -1186,10 +1186,12 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
>>
>>         dma_cookie_init(&tdc->dma_chan);
>>         tdc->config_init = false;
>> -       ret = clk_prepare_enable(tdma->dma_clk);
>> +
>> +       ret = pm_runtime_get_sync(tdma->dev);
>>         if (ret < 0)
>> -               dev_err(tdc2dev(tdc), "clk_prepare_enable failed: %d\n", ret);
>> -       return ret;
>> +               return ret;
>> +
> 
>> +       return 0;
> 
> This is a non-reachable code.

No it is not.

>>  }
>>
>>  static void tegra_dma_free_chan_resources(struct dma_chan *dc)
>> @@ -1232,7 +1234,7 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc)
>>                 list_del(&sg_req->node);
>>                 kfree(sg_req);
>>         }
>> -       clk_disable_unprepare(tdma->dma_clk);
>> +       pm_runtime_put(tdma->dev);
>>
>>         tdc->slave_id = 0;
>>  }
>> @@ -1356,20 +1358,14 @@ static int tegra_dma_probe(struct platform_device *pdev)
>>         spin_lock_init(&tdma->global_lock);
>>
>>         pm_runtime_enable(&pdev->dev);
>> -       if (!pm_runtime_enabled(&pdev->dev)) {
>> +       if (!pm_runtime_enabled(&pdev->dev))
>>                 ret = tegra_dma_runtime_resume(&pdev->dev);
> 
> I didn't check, but does this bring device to powered on state?

Yes.

Jon

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

[Jon Hunter]

Hi Vinod,

Any more comments on this?

Cheers
Jon

On 13/11/15 16:39, Jon Hunter wrote:
> Some clean-up changes for the tegra-apb DMA driver.
> 
> Changes for V3 noted in the applicable patches.
> 
> These have been compile and boot tested for ARM and ARM64. Summary of the
> ARM results are below.
> 
> Test summary
> ------------
> 
> Build: zImage:
>     Pass: ( 2/ 2): multi_v7_defconfig, tegra_defconfig
> 
> Build: Image:
>     Pass: ( 1/ 1): defconfig
> 
> Boot to userspace: defconfig:
>     Pass: ( 1/ 1): qemu-vexpress64
> 
> 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 (6):
>   dmaengine: tegra-apb: Correct runtime-pm usage
>   dmaengine: tegra-apb: Use dev_get_drvdata()
>   dmaengine: tegra-apb: Save and restore word count
>   dmaengine: tegra-apb: Only save channel state for those in use
>   dmaengine: tegra-apb: Update driver to use GFP_NOWAIT
>   dmaengine: tegra-apb: Free interrupts before killing tasklets
> 
>  drivers/dma/tegra20-apb-dma.c | 73 ++++++++++++++++++++++++-------------------
>  1 file changed, 40 insertions(+), 33 deletions(-)
> 

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

[Jon Hunter]

Hi Vinod,

Any more comments on this?

Cheers
Jon

On 13/11/15 16:39, Jon Hunter wrote:
> Some clean-up changes for the tegra-apb DMA driver.
> 
> Changes for V3 noted in the applicable patches.
> 
> These have been compile and boot tested for ARM and ARM64. Summary of the
> ARM results are below.
> 
> Test summary
> ------------
> 
> Build: zImage:
>     Pass: ( 2/ 2): multi_v7_defconfig, tegra_defconfig
> 
> Build: Image:
>     Pass: ( 1/ 1): defconfig
> 
> Boot to userspace: defconfig:
>     Pass: ( 1/ 1): qemu-vexpress64
> 
> 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 (6):
>   dmaengine: tegra-apb: Correct runtime-pm usage
>   dmaengine: tegra-apb: Use dev_get_drvdata()
>   dmaengine: tegra-apb: Save and restore word count
>   dmaengine: tegra-apb: Only save channel state for those in use
>   dmaengine: tegra-apb: Update driver to use GFP_NOWAIT
>   dmaengine: tegra-apb: Free interrupts before killing tasklets
> 
>  drivers/dma/tegra20-apb-dma.c | 73 ++++++++++++++++++++++++-------------------
>  1 file changed, 40 insertions(+), 33 deletions(-)
> 

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

[Vinod Koul]

On Fri, Nov 13, 2015 at 04:39:37PM +0000, Jon Hunter wrote:
> Some clean-up changes for the tegra-apb DMA driver.
> 
> Changes for V3 noted in the applicable patches.
> 
> These have been compile and boot tested for ARM and ARM64. Summary of the
> ARM results are below.

Applied, thanks

-- 
~Vinod