On Wed, Dec 11, 2013 at 01:49:29PM -0700, Stephen Warren wrote:
> On 12/11/2013 01:49 AM, Chaitanya Bandi wrote:
> > Used runtime_pm APIs for clock enabling/disabling.
> > Made changes such that clock is not enabled during
> > idle. Also moved the usage of clk prepare/unprepare
> > such that they are not called in isr context.
>
> Hmm. This is going to cause conflicts with the patch I'm taking through
> the Tegra tree which converts the driver to support the standard DMA DT
> bindings. Perhaps this can wait until 3.15, or perhaps we can merge the
> Tegra branch back into the DMA tree to resolve the conflict.
Ok pls do resubmit once your stuff is sorted out!
>
> > diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
>
> > - * Copyright (c) 2012, NVIDIA CORPORATION. All rights reserved.
> > + * Copyright (c) 2012-13, NVIDIA CORPORATION. All rights reserved.
>
> s/13/2013/
>
> > @@ -580,6 +580,11 @@ static void handle_once_dma_done(struct tegra_dma_channel *tdc,
> > list_add_tail(&sgreq->node, &tdc->free_sg_req);
> >
> > /* Do not start DMA if it is going to be terminate */
> > + if (list_empty(&tdc->pending_sg_req) && (!to_terminate)) {
> > + clk_disable(tdc->tdma->dma_clk);
> > + pm_runtime_put(tdc->tdma->dev);
> > + }
> > +
> > if (to_terminate || list_empty(&tdc->pending_sg_req))
> > return;
>
> Don't you want to insert the new code before the comment? Otherwise,
> you're separating the existing comment and code.
>
> Here and many other places, both pm_runtime_get/put *and*
> clk_enable/disable are called. Why doesn't the code *just* call
> pm_runtime_get/put, and let the implementation of those APIs perform the
> clock_enable/disable? That'd be a lot more typical.
And that's what I was thinking too. Why dont we rely on runtime calls for doing
the ref counting and then in runtime suspend & resume handlers disable and
enable clock
>
> Most of the new calls to pm_runtime_*()/clk_{en,dis}able() are missing
> error checking.
>
> > @@ -682,12 +687,21 @@ static void tegra_dma_issue_pending(struct dma_chan *dc)
>
> > + pm_runtime_get(tdc->tdma->dev);
>
> I think you need pm_runtime_get_sync() here to make sure the clock gets
> turned on immediately? Perhaps that why...
>
> > + ret = clk_enable(tdc->tdma->dma_clk);
>
> ... there's also direct manipulation of the clock everywhere.
>
> Also, shouldn't the DMA core be calling pm_runtime_get(), rather than
> each individual DMA driver?
Yes i have been keen on that, not getting priortized yet!
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
