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

[PATCH V2 0/6] 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.

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 | 83 ++++++++++++++++++++++++-------------------
 1 file changed, 46 insertions(+), 37 deletions(-)

-- 
2.1.4

[PATCH V2 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
V2 changes:
- Fixed return value for allocating channel
- Fixed test for return value from pm_runtime_get_sync()

 drivers/dma/tegra20-apb-dma.c | 49 ++++++++++++++++++-------------------------
 1 file changed, 20 insertions(+), 29 deletions(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index c8f79dcaaee8..6007111fac1d 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);
@@ -1539,11 +1534,10 @@ static int tegra_dma_runtime_resume(struct device *dev)
 static int tegra_dma_pm_suspend(struct device *dev)
 {
 	struct tegra_dma *tdma = dev_get_drvdata(dev);
-	int i;
-	int ret;
+	int i, ret;
 
 	/* Enable clock before accessing register */
-	ret = tegra_dma_runtime_resume(dev);
+	ret = pm_runtime_get_sync(dev);
 	if (ret < 0)
 		return ret;
 
@@ -1560,18 +1554,17 @@ static int tegra_dma_pm_suspend(struct device *dev)
 	}
 
 	/* Disable clock */
-	tegra_dma_runtime_suspend(dev);
+	pm_runtime_put(dev);
 	return 0;
 }
 
 static int tegra_dma_pm_resume(struct device *dev)
 {
 	struct tegra_dma *tdma = dev_get_drvdata(dev);
-	int i;
-	int ret;
+	int i, 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 +1585,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

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

[Andy Shevchenko]

On Mon, Nov 9, 2015 at 3:23 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.

Nitpick

> @@ -1539,11 +1534,10 @@ static int tegra_dma_runtime_resume(struct device *dev)
>  static int tegra_dma_pm_suspend(struct device *dev)
>  {
>         struct tegra_dma *tdma = dev_get_drvdata(dev);
> -       int i;
> -       int ret;
> +       int i, ret;

>  static int tegra_dma_pm_resume(struct device *dev)
>  {
>         struct tegra_dma *tdma = dev_get_drvdata(dev);
> -       int i;
> -       int ret;
> +       int i, ret;

Do you really need that?

-- 
With Best Regards,
Andy Shevchenko

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

[Jon Hunter]

On 09/11/15 13:57, Andy Shevchenko wrote:
> On Mon, Nov 9, 2015 at 3:23 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.
> 
> Nitpick
> 
>> @@ -1539,11 +1534,10 @@ static int tegra_dma_runtime_resume(struct device *dev)
>>  static int tegra_dma_pm_suspend(struct device *dev)
>>  {
>>         struct tegra_dma *tdma = dev_get_drvdata(dev);
>> -       int i;
>> -       int ret;
>> +       int i, ret;
> 
>>  static int tegra_dma_pm_resume(struct device *dev)
>>  {
>>         struct tegra_dma *tdma = dev_get_drvdata(dev);
>> -       int i;
>> -       int ret;
>> +       int i, ret;
> 
> Do you really need that?

Rhetorical question? ;-)

I can drop that.

Jon

[PATCH V2 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 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 6007111fac1d..802d66a25de6 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 V2 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 802d66a25de6..94f59c3fcdca 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1549,6 +1549,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 */
@@ -1574,6 +1577,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 V2 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>
---
 drivers/dma/tegra20-apb-dma.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 94f59c3fcdca..4fc0851069cd 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1544,6 +1544,12 @@ 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);
@@ -1577,6 +1583,12 @@ 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

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

[Andy Shevchenko]

On Mon, Nov 9, 2015 at 3:23 PM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> 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.

Nitpick

> +               /*
> +                * Only save the state of DMA channels that are in use.
> +                */

One line?

> +               /*
> +                * Only restore the state of DMA channels that are in use.
> +                */

Same.

-- 
With Best Regards,
Andy Shevchenko

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

[Jon Hunter]

On 09/11/15 13:58, Andy Shevchenko wrote:
> On Mon, Nov 9, 2015 at 3:23 PM, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>> 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.
> 
> Nitpick
> 
>> +               /*
>> +                * Only save the state of DMA channels that are in use.
>> +                */
> 
> One line?
> 
>> +               /*
>> +                * Only restore the state of DMA channels that are in use.
>> +                */
> 
> Same.

Ok.

Jon

[PATCH V2 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 4fc0851069cd..004edcdb2d2e 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 V2 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>
---
V2 changes:
- Updated patch to use free_irq instead of disable_irq

 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 004edcdb2d2e..d3892a106c1e 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