lkml.org 
[lkml]   [2015]   [Nov]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH V3 1/6] dmaengine: tegra-apb: Correct runtime-pm usage
From
Date

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


\
 
 \ /
  Last update: 2015-11-16 11:01    [W:0.049 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site