[PATCH 0/6] dmaengine: tegra-apb: Various updates

[PATCH 0/6] dmaengine: tegra-apb: Various updates

[Jon Hunter]

A few updates for the tegra-apb driver. Build tested for ARM and ARM64 and
the driver has been tested on tegra124 with the HSUART driver that uses
DMA.

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: Disable interrupts on removal

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

-- 
2.1.4

[PATCH 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>
---
 drivers/dma/tegra20-apb-dma.c | 50 ++++++++++++++++---------------------------
 1 file changed, 18 insertions(+), 32 deletions(-)

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index c8f79dcaaee8..fe4a006adeb0 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1182,14 +1182,11 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
 {
 	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
 	struct tegra_dma *tdma = tdc->tdma;
-	int ret;
 
 	dma_cookie_init(&tdc->dma_chan);
 	tdc->config_init = false;
-	ret = clk_prepare_enable(tdma->dma_clk);
-	if (ret < 0)
-		dev_err(tdc2dev(tdc), "clk_prepare_enable failed: %d\n", ret);
-	return ret;
+
+	return pm_runtime_get_sync(tdma->dev);
 }
 
 static void tegra_dma_free_chan_resources(struct dma_chan *dc)
@@ -1232,7 +1229,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 +1353,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;
+	if (ret) {
+		pm_runtime_disable(&pdev->dev);
+		return ret;
 	}
 
 	/* Reset DMA controller */
@@ -1382,7 +1373,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 +1476,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 +1529,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 +1549,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 +1580,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 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 fe4a006adeb0..cf546671f83d 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1504,8 +1504,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;
@@ -1513,8 +1512,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 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 cf546671f83d..06063d370272 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1544,6 +1544,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 */
@@ -1569,6 +1572,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 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 06063d370272..35723703d4b9 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1539,6 +1539,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);
@@ -1572,6 +1578,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

[PATCH 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 35723703d4b9..2bfab8d28b53 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 6/6] dmaengine: tegra-apb: Disable interrupts on removal

[Jon Hunter]

On driver removal, before killing any tasklets, ensure that the channel
interrupts are disabled so that the tasklet will not try to run during
or after the removal of the driver.

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

diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
index 2bfab8d28b53..0dd6e7deaa8e 100644
--- a/drivers/dma/tegra20-apb-dma.c
+++ b/drivers/dma/tegra20-apb-dma.c
@@ -1492,6 +1492,7 @@ static int tegra_dma_remove(struct platform_device *pdev)
 
 	for (i = 0; i < tdma->chip_data->nr_channels; ++i) {
 		tdc = &tdma->channels[i];
+		disable_irq(tdc->irq);
 		tasklet_kill(&tdc->tasklet);
 	}
 
-- 
2.1.4

Re: [PATCH 6/6] dmaengine: tegra-apb: Disable interrupts on removal

[Lars-Peter Clausen]

On 10/16/2015 10:25 AM, Jon Hunter wrote:
> On driver removal, before killing any tasklets, ensure that the channel
> interrupts are disabled so that the tasklet will not try to run during
> or after the removal of the driver.
> 
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
>  drivers/dma/tegra20-apb-dma.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index 2bfab8d28b53..0dd6e7deaa8e 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -1492,6 +1492,7 @@ static int tegra_dma_remove(struct platform_device *pdev)
>  
>  	for (i = 0; i < tdma->chip_data->nr_channels; ++i) {
>  		tdc = &tdma->channels[i];
> +		disable_irq(tdc->irq);

How about just calling free_irq()? That's how you'd typically handle this.

>  		tasklet_kill(&tdc->tasklet);
>  	}
>  
> 

Re: [PATCH 6/6] dmaengine: tegra-apb: Disable interrupts on removal

[Jon Hunter]

On 16/10/15 09:53, Lars-Peter Clausen wrote:
> On 10/16/2015 10:25 AM, Jon Hunter wrote:
>> On driver removal, before killing any tasklets, ensure that the channel
>> interrupts are disabled so that the tasklet will not try to run during
>> or after the removal of the driver.
>>
>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>> ---
>>  drivers/dma/tegra20-apb-dma.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>> index 2bfab8d28b53..0dd6e7deaa8e 100644
>> --- a/drivers/dma/tegra20-apb-dma.c
>> +++ b/drivers/dma/tegra20-apb-dma.c
>> @@ -1492,6 +1492,7 @@ static int tegra_dma_remove(struct platform_device *pdev)
>>  
>>  	for (i = 0; i < tdma->chip_data->nr_channels; ++i) {
>>  		tdc = &tdma->channels[i];
>> +		disable_irq(tdc->irq);
> 
> How about just calling free_irq()? That's how you'd typically handle this.

Yes, however, the interrupt is requested by devm_request_irq(). I guess
I could call devm_free_irq() here?

Cheers
Jon

Re: [PATCH 6/6] dmaengine: tegra-apb: Disable interrupts on removal

[Lars-Peter Clausen]

On 10/16/2015 11:29 AM, Jon Hunter wrote:
> 
> On 16/10/15 09:53, Lars-Peter Clausen wrote:
>> On 10/16/2015 10:25 AM, Jon Hunter wrote:
>>> On driver removal, before killing any tasklets, ensure that the channel
>>> interrupts are disabled so that the tasklet will not try to run during
>>> or after the removal of the driver.
>>>
>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>> ---
>>>  drivers/dma/tegra20-apb-dma.c | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>>> index 2bfab8d28b53..0dd6e7deaa8e 100644
>>> --- a/drivers/dma/tegra20-apb-dma.c
>>> +++ b/drivers/dma/tegra20-apb-dma.c
>>> @@ -1492,6 +1492,7 @@ static int tegra_dma_remove(struct platform_device *pdev)
>>>  
>>>  	for (i = 0; i < tdma->chip_data->nr_channels; ++i) {
>>>  		tdc = &tdma->channels[i];
>>> +		disable_irq(tdc->irq);
>>
>> How about just calling free_irq()? That's how you'd typically handle this.
> 
> Yes, however, the interrupt is requested by devm_request_irq(). I guess
> I could call devm_free_irq() here?

Just use request_irq() instead of devm_request_irq(). You have the same
issue on the error path in the probe function anyway and also need to add
the free_irq() before the tasklet_kill() there as well.

Re: [PATCH 6/6] dmaengine: tegra-apb: Disable interrupts on removal

[Jon Hunter]

On 16/10/15 11:40, Lars-Peter Clausen wrote:
> On 10/16/2015 11:29 AM, Jon Hunter wrote:
>>
>> On 16/10/15 09:53, Lars-Peter Clausen wrote:
>>> On 10/16/2015 10:25 AM, Jon Hunter wrote:
>>>> On driver removal, before killing any tasklets, ensure that the channel
>>>> interrupts are disabled so that the tasklet will not try to run during
>>>> or after the removal of the driver.
>>>>
>>>> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
>>>> ---
>>>>  drivers/dma/tegra20-apb-dma.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>>>> index 2bfab8d28b53..0dd6e7deaa8e 100644
>>>> --- a/drivers/dma/tegra20-apb-dma.c
>>>> +++ b/drivers/dma/tegra20-apb-dma.c
>>>> @@ -1492,6 +1492,7 @@ static int tegra_dma_remove(struct platform_device *pdev)
>>>>  
>>>>  	for (i = 0; i < tdma->chip_data->nr_channels; ++i) {
>>>>  		tdc = &tdma->channels[i];
>>>> +		disable_irq(tdc->irq);
>>>
>>> How about just calling free_irq()? That's how you'd typically handle this.
>>
>> Yes, however, the interrupt is requested by devm_request_irq(). I guess
>> I could call devm_free_irq() here?
> 
> Just use request_irq() instead of devm_request_irq(). You have the same
> issue on the error path in the probe function anyway and also need to add
> the free_irq() before the tasklet_kill() there as well.

I was wondering about that but the tasklets should never be scheduled if
the probe does not succeed, so I think it is ok.

Jon

Re: [PATCH 6/6] dmaengine: tegra-apb: Disable interrupts on removal

[Vinod Koul]

On Fri, Oct 16, 2015 at 11:57:02AM +0100, Jon Hunter wrote:
> >>> How about just calling free_irq()? That's how you'd typically handle this.
> >>
> >> Yes, however, the interrupt is requested by devm_request_irq(). I guess
> >> I could call devm_free_irq() here?
> > 
> > Just use request_irq() instead of devm_request_irq(). You have the same
> > issue on the error path in the probe function anyway and also need to add
> > the free_irq() before the tasklet_kill() there as well.
> 
> I was wondering about that but the tasklets should never be scheduled if
> the probe does not succeed, so I think it is ok.

This is actually very racy, if probe fails but due to devm_ calls your irq
is alive till it freed by core

And a faulty device triggering irq can complicate matters, so for irq IMHO
we don't get much benefit with devm_ variant

-- 
~Vinod

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

[Vinod Koul]

On Fri, Oct 16, 2015 at 09:25:52AM +0100, Jon Hunter wrote:
> @@ -1182,14 +1182,11 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
>  {
>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>  	struct tegra_dma *tdma = tdc->tdma;
> -	int ret;
>  
>  	dma_cookie_init(&tdc->dma_chan);
>  	tdc->config_init = false;
> -	ret = clk_prepare_enable(tdma->dma_clk);
> -	if (ret < 0)
> -		dev_err(tdc2dev(tdc), "clk_prepare_enable failed: %d\n", ret);
> -	return ret;
> +
> +	return pm_runtime_get_sync(tdma->dev);

Alloc channel is supposed to return number of descriptors allocated and if
pm_runtime_get_sync() returns postive values we get wrong return!

>  	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);

do we need pm_runtime_get() here, should we use pm_request_resume() ?

>  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 you are runtime suspended then core will runtime resume you before
invoking suspend, so why do we need this

-- 
~Vinod

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

[Jon Hunter]

On 28/10/15 07:03, Vinod Koul wrote:
> On Fri, Oct 16, 2015 at 09:25:52AM +0100, Jon Hunter wrote:
>> @@ -1182,14 +1182,11 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
>>  {
>>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>>  	struct tegra_dma *tdma = tdc->tdma;
>> -	int ret;
>>  
>>  	dma_cookie_init(&tdc->dma_chan);
>>  	tdc->config_init = false;
>> -	ret = clk_prepare_enable(tdma->dma_clk);
>> -	if (ret < 0)
>> -		dev_err(tdc2dev(tdc), "clk_prepare_enable failed: %d\n", ret);
>> -	return ret;
>> +
>> +	return pm_runtime_get_sync(tdma->dev);
> 
> Alloc channel is supposed to return number of descriptors allocated and if
> pm_runtime_get_sync() returns postive values we get wrong return!

Yes I will fix. I assume that returning 0 is allowed if no descriptors
are allocated here. So much for correcting rpm usage ;-)

>>  	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);
> 
> do we need pm_runtime_get() here, should we use pm_request_resume() ?

We definitely want pm_runtime_get_sync() because pm_request_resume() is
an ASYNC resume and so does not guarantee the device is accessible after
the call returns. The pm_runtime_get variant is nice too because it
keeps track of the number of gets and puts that have occurred.

>>  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 you are runtime suspended then core will runtime resume you before
> invoking suspend, so why do we need this

Is this change now in the mainline? Do you have commit ID for that?

I recall the last time we discussed this that Rafael said that they were
going to do that, but he said as a rule of thumb if you need to resume
it, resume it [0].

Cheers
Jon

[0] https://lkml.org/lkml/2015/8/24/845

Re: [PATCH 6/6] dmaengine: tegra-apb: Disable interrupts on removal

[Jon Hunter]

On 28/10/15 06:53, Vinod Koul wrote:
> On Fri, Oct 16, 2015 at 11:57:02AM +0100, Jon Hunter wrote:
>>>>> How about just calling free_irq()? That's how you'd typically handle this.
>>>>
>>>> Yes, however, the interrupt is requested by devm_request_irq(). I guess
>>>> I could call devm_free_irq() here?
>>>
>>> Just use request_irq() instead of devm_request_irq(). You have the same
>>> issue on the error path in the probe function anyway and also need to add
>>> the free_irq() before the tasklet_kill() there as well.
>>
>> I was wondering about that but the tasklets should never be scheduled if
>> the probe does not succeed, so I think it is ok.
> 
> This is actually very racy, if probe fails but due to devm_ calls your irq
> is alive till it freed by core
> 
> And a faulty device triggering irq can complicate matters, so for irq IMHO
> we don't get much benefit with devm_ variant

That's fine, I will drop the devm_ usage here then.

Jon

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

[Vinod Koul]

On Wed, Oct 28, 2015 at 01:32:12PM +0000, Jon Hunter wrote:
> 
> On 28/10/15 07:03, Vinod Koul wrote:
> > On Fri, Oct 16, 2015 at 09:25:52AM +0100, Jon Hunter wrote:
> >> @@ -1182,14 +1182,11 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
> >>  {
> >>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
> >>  	struct tegra_dma *tdma = tdc->tdma;
> >> -	int ret;
> >>  
> >>  	dma_cookie_init(&tdc->dma_chan);
> >>  	tdc->config_init = false;
> >> -	ret = clk_prepare_enable(tdma->dma_clk);
> >> -	if (ret < 0)
> >> -		dev_err(tdc2dev(tdc), "clk_prepare_enable failed: %d\n", ret);
> >> -	return ret;
> >> +
> >> +	return pm_runtime_get_sync(tdma->dev);
> > 
> > Alloc channel is supposed to return number of descriptors allocated and if
> > pm_runtime_get_sync() returns postive values we get wrong return!
> 
> Yes I will fix. I assume that returning 0 is allowed if no descriptors
> are allocated here. So much for correcting rpm usage ;-)

Yes 0 is allowed...

> >>  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 you are runtime suspended then core will runtime resume you before
> > invoking suspend, so why do we need this
> 
> Is this change now in the mainline? Do you have commit ID for that?
> 
> I recall the last time we discussed this that Rafael said that they were
> going to do that, but he said as a rule of thumb if you need to resume
> it, resume it [0].

IIRC this has been always the behaviour, at least I see this when I test the
devices

-- 
~Vinod

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

[Jon Hunter]

On 29/10/15 01:57, Vinod Koul wrote:
> On Wed, Oct 28, 2015 at 01:32:12PM +0000, Jon Hunter wrote:
>>
>> On 28/10/15 07:03, Vinod Koul wrote:
>>> On Fri, Oct 16, 2015 at 09:25:52AM +0100, Jon Hunter wrote:
>>>> @@ -1182,14 +1182,11 @@ static int tegra_dma_alloc_chan_resources(struct dma_chan *dc)
>>>>  {
>>>>  	struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
>>>>  	struct tegra_dma *tdma = tdc->tdma;
>>>> -	int ret;
>>>>  
>>>>  	dma_cookie_init(&tdc->dma_chan);
>>>>  	tdc->config_init = false;
>>>> -	ret = clk_prepare_enable(tdma->dma_clk);
>>>> -	if (ret < 0)
>>>> -		dev_err(tdc2dev(tdc), "clk_prepare_enable failed: %d\n", ret);
>>>> -	return ret;
>>>> +
>>>> +	return pm_runtime_get_sync(tdma->dev);
>>>
>>> Alloc channel is supposed to return number of descriptors allocated and if
>>> pm_runtime_get_sync() returns postive values we get wrong return!
>>
>> Yes I will fix. I assume that returning 0 is allowed if no descriptors
>> are allocated here. So much for correcting rpm usage ;-)
> 
> Yes 0 is allowed...
> 
>>>>  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 you are runtime suspended then core will runtime resume you before
>>> invoking suspend, so why do we need this
>>
>> Is this change now in the mainline? Do you have commit ID for that?
>>
>> I recall the last time we discussed this that Rafael said that they were
>> going to do that, but he said as a rule of thumb if you need to resume
>> it, resume it [0].
> 
> IIRC this has been always the behaviour, at least I see this when I test the
> devices

I have been doing some testing today and if the DMA is runtime
suspended, then I don't see it runtime resumed before suspend is called.

Can you elborate on "at least I see this when I test the devices"? What
are you looking at? Are you using kernel function tracers in some way?

Cheers
Jon

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

[Kevin Hilman]

Jon Hunter <jonathanh@nvidia.com> writes:

> On 29/10/15 01:57, Vinod Koul wrote:
>> On Wed, Oct 28, 2015 at 01:32:12PM +0000, Jon Hunter wrote:
>>>
>>> On 28/10/15 07:03, Vinod Koul wrote:
>>>> On Fri, Oct 16, 2015 at 09:25:52AM +0100, Jon Hunter wrote:

[...]

>>>>>  	/* Enable clock before accessing register */
>>>>> -	ret = tegra_dma_runtime_resume(dev);
>>>>> +	ret = pm_runtime_get_sync(dev);
>>>>
>>>> If you are runtime suspended then core will runtime resume you before
>>>> invoking suspend, so why do we need this
>>>
>>> Is this change now in the mainline? Do you have commit ID for that?
>>>
>>> I recall the last time we discussed this that Rafael said that they were
>>> going to do that, but he said as a rule of thumb if you need to resume
>>> it, resume it [0].
>> 
>> IIRC this has been always the behaviour, at least I see this when I test the
>> devices
>
> I have been doing some testing today and if the DMA is runtime
> suspended, then I don't see it runtime resumed before suspend is called.
>
> Can you elborate on "at least I see this when I test the devices"? What
> are you looking at? Are you using kernel function tracers in some way?

The PM core does a _get_noresume()[1] which tries to prevent runtime
suspends *during* a system suspend.  However, the PM core should not be
doing an actual runtime resume of the device, so if the device is
already runtime suspended, it will not be runtime resumed by the core,
so if the driver needs it to be runtime resumed, it needs to do it
itself.

Kevin


[1] c.f. drivers/base/power/main.c::device_prepare()

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

[Vinod Koul]

On Tue, Nov 03, 2015 at 01:25:09PM -0800, Kevin Hilman wrote:
> >>>>>  	/* Enable clock before accessing register */
> >>>>> -	ret = tegra_dma_runtime_resume(dev);
> >>>>> +	ret = pm_runtime_get_sync(dev);
> >>>>
> >>>> If you are runtime suspended then core will runtime resume you before
> >>>> invoking suspend, so why do we need this
> >>>
> >>> Is this change now in the mainline? Do you have commit ID for that?
> >>>
> >>> I recall the last time we discussed this that Rafael said that they were
> >>> going to do that, but he said as a rule of thumb if you need to resume
> >>> it, resume it [0].
> >> 
> >> IIRC this has been always the behaviour, at least I see this when I test the
> >> devices
> >
> > I have been doing some testing today and if the DMA is runtime
> > suspended, then I don't see it runtime resumed before suspend is called.
> >
> > Can you elborate on "at least I see this when I test the devices"? What
> > are you looking at? Are you using kernel function tracers in some way?
> 
> The PM core does a _get_noresume()[1] which tries to prevent runtime
> suspends *during* a system suspend.  However, the PM core should not be
> doing an actual runtime resume of the device, so if the device is
> already runtime suspended, it will not be runtime resumed by the core,
> so if the driver needs it to be runtime resumed, it needs to do it
> itself.

+ Rafael

This is contrariry to what I see, If my driver is runtime suspended and on
suspend, it gets runtime resumed and then suspended

-- 
~Vinod

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

[Kevin Hilman]

Vinod Koul <vinod.koul@intel.com> writes:

> On Tue, Nov 03, 2015 at 01:25:09PM -0800, Kevin Hilman wrote:
>> >>>>>  	/* Enable clock before accessing register */
>> >>>>> -	ret = tegra_dma_runtime_resume(dev);
>> >>>>> +	ret = pm_runtime_get_sync(dev);
>> >>>>
>> >>>> If you are runtime suspended then core will runtime resume you before
>> >>>> invoking suspend, so why do we need this
>> >>>
>> >>> Is this change now in the mainline? Do you have commit ID for that?
>> >>>
>> >>> I recall the last time we discussed this that Rafael said that they were
>> >>> going to do that, but he said as a rule of thumb if you need to resume
>> >>> it, resume it [0].
>> >> 
>> >> IIRC this has been always the behaviour, at least I see this when I test the
>> >> devices
>> >
>> > I have been doing some testing today and if the DMA is runtime
>> > suspended, then I don't see it runtime resumed before suspend is called.
>> >
>> > Can you elborate on "at least I see this when I test the devices"? What
>> > are you looking at? Are you using kernel function tracers in some way?
>> 
>> The PM core does a _get_noresume()[1] which tries to prevent runtime
>> suspends *during* a system suspend.  However, the PM core should not be
>> doing an actual runtime resume of the device, so if the device is
>> already runtime suspended, it will not be runtime resumed by the core,
>> so if the driver needs it to be runtime resumed, it needs to do it
>> itself.
>
> + Rafael
>
> This is contrariry to what I see, If my driver is runtime suspended and on
> suspend, it gets runtime resumed and then suspended

Since I was late to the thread, can you explain what kind of driver and
on what bus type you're seeing this behavior?

It could be that your bus-type is doing something, but I don't think it
should be the PM core.

Kevin

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

[Rafael J. Wysocki]

On Wednesday, November 04, 2015 08:59:43 AM Kevin Hilman wrote:
> Vinod Koul <vinod.koul@intel.com> writes:
> 
> > On Tue, Nov 03, 2015 at 01:25:09PM -0800, Kevin Hilman wrote:
> >> >>>>>  	/* Enable clock before accessing register */
> >> >>>>> -	ret = tegra_dma_runtime_resume(dev);
> >> >>>>> +	ret = pm_runtime_get_sync(dev);
> >> >>>>
> >> >>>> If you are runtime suspended then core will runtime resume you before
> >> >>>> invoking suspend, so why do we need this
> >> >>>
> >> >>> Is this change now in the mainline? Do you have commit ID for that?
> >> >>>
> >> >>> I recall the last time we discussed this that Rafael said that they were
> >> >>> going to do that, but he said as a rule of thumb if you need to resume
> >> >>> it, resume it [0].
> >> >> 
> >> >> IIRC this has been always the behaviour, at least I see this when I test the
> >> >> devices
> >> >
> >> > I have been doing some testing today and if the DMA is runtime
> >> > suspended, then I don't see it runtime resumed before suspend is called.
> >> >
> >> > Can you elborate on "at least I see this when I test the devices"? What
> >> > are you looking at? Are you using kernel function tracers in some way?
> >> 
> >> The PM core does a _get_noresume()[1] which tries to prevent runtime
> >> suspends *during* a system suspend.  However, the PM core should not be
> >> doing an actual runtime resume of the device, so if the device is
> >> already runtime suspended, it will not be runtime resumed by the core,
> >> so if the driver needs it to be runtime resumed, it needs to do it
> >> itself.
> >
> > + Rafael
> >
> > This is contrariry to what I see, If my driver is runtime suspended and on
> > suspend, it gets runtime resumed and then suspended
> 
> Since I was late to the thread, can you explain what kind of driver and
> on what bus type you're seeing this behavior?
> 
> It could be that your bus-type is doing something, but I don't think it
> should be the PM core.

Right.

Bus types do that, the core doesn't.  The ACPI PM domain does that too
for some devices.

So Vinod, more details, please.

Thanks,
Rafael

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

[Vinod Koul]

On Thu, Nov 05, 2015 at 03:15:18AM +0100, Rafael J. Wysocki wrote:

> > > + Rafael
> > >
> > > This is contrariry to what I see, If my driver is runtime suspended and on
> > > suspend, it gets runtime resumed and then suspended
> > 
> > Since I was late to the thread, can you explain what kind of driver and
> > on what bus type you're seeing this behavior?
> > 
> > It could be that your bus-type is doing something, but I don't think it
> > should be the PM core.
> 
> Right.
> 
> Bus types do that, the core doesn't.  The ACPI PM domain does that too
> for some devices.
> 
> So Vinod, more details, please.

Okay relooking at core I do think that runtime resume should not be invoked
while suspending, as core seems to call pm_runtime_get_noresume() but I am
still missing something here..

I do see this behaviour (runtime resume on suspend) on Intel audio drivers
which are PCI devices, is PCI or ACPI doing some magic here.

I have seen this as consistent behavior and actually an irritant, as we used
to download firmware in resume patch, but then we end up thrashing the
controller while going to suspend!

-- 
~Vinod