On 14/11/15 13:27, Andy Shevchenko wrote:
> On Fri, Nov 13, 2015 at 6:39 PM, Jon Hunter <jonathanh@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
