| Message ID | 20200112173006.29863-11-digetx@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | NVIDIA Tegra APB DMA driver fixes and improvements | expand |
On 12/01/2020 17:30, Dmitry Osipenko wrote: > It's a bit impractical to enable hardware's clock at the time of DMA > channel's allocation because most of DMA client drivers allocate DMA > channel at the time of the driver's probing and thus DMA clock is kept > always-enabled in practice, defeating the whole purpose of runtime PM. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > drivers/dma/tegra20-apb-dma.c | 43 ++++++++++++++++++++++------------- > 1 file changed, 27 insertions(+), 16 deletions(-) > > diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c > index cc4a9ca20780..b9d8e57eaf54 100644 > --- a/drivers/dma/tegra20-apb-dma.c > +++ b/drivers/dma/tegra20-apb-dma.c > @@ -436,6 +436,8 @@ static void tegra_dma_stop(struct tegra_dma_channel *tdc) > tdc_write(tdc, TEGRA_APBDMA_CHAN_STATUS, status); > } > tdc->busy = false; > + > + pm_runtime_put(tdc->tdma->dev); Is this the right place to call put? Seems that in terminate_all resume is called after stop which will access the registers. > } > > static void tegra_dma_start(struct tegra_dma_channel *tdc, > @@ -500,18 +502,25 @@ static void tegra_dma_configure_for_next(struct tegra_dma_channel *tdc, > tegra_dma_resume(tdc); > } > > -static void tdc_start_head_req(struct tegra_dma_channel *tdc) > +static bool tdc_start_head_req(struct tegra_dma_channel *tdc) > { > struct tegra_dma_sg_req *sg_req; > + int err; > > if (list_empty(&tdc->pending_sg_req)) > - return; > + return false; > + > + err = pm_runtime_get_sync(tdc->tdma->dev); > + if (WARN_ON_ONCE(err < 0)) > + return false; > > sg_req = list_first_entry(&tdc->pending_sg_req, typeof(*sg_req), node); > tegra_dma_start(tdc, sg_req); > sg_req->configured = true; > sg_req->words_xferred = 0; > tdc->busy = true; > + > + return true; > } > > static void tdc_configure_next_head_desc(struct tegra_dma_channel *tdc) > @@ -615,6 +624,8 @@ static void handle_once_dma_done(struct tegra_dma_channel *tdc, > } > list_add_tail(&sgreq->node, &tdc->free_sg_req); > > + pm_runtime_put(tdc->tdma->dev); > + > /* Do not start DMA if it is going to be terminate */ > if (to_terminate || list_empty(&tdc->pending_sg_req)) > return; > @@ -730,9 +741,7 @@ static void tegra_dma_issue_pending(struct dma_chan *dc) > dev_err(tdc2dev(tdc), "No DMA request\n"); > goto end; > } > - if (!tdc->busy) { > - tdc_start_head_req(tdc); > - > + if (!tdc->busy && tdc_start_head_req(tdc)) { > /* Continuous single mode: Configure next req */ > if (tdc->cyclic) { > /* > @@ -1280,22 +1289,15 @@ tegra_dma_prep_dma_cyclic(struct dma_chan *dc, dma_addr_t buf_addr, > 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); > > - ret = pm_runtime_get_sync(tdma->dev); > - if (ret < 0) > - return ret; > - > return 0; > } > > static void tegra_dma_free_chan_resources(struct dma_chan *dc) > { > struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc); > - struct tegra_dma *tdma = tdc->tdma; > struct tegra_dma_desc *dma_desc; > struct tegra_dma_sg_req *sg_req; > struct list_head dma_desc_list; > @@ -1328,7 +1330,6 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc) > list_del(&sg_req->node); > kfree(sg_req); > } > - pm_runtime_put(tdma->dev); > > tdc->slave_id = TEGRA_APBDMA_SLAVE_ID_INVALID; > } > @@ -1428,11 +1429,16 @@ static int tegra_dma_probe(struct platform_device *pdev) > > spin_lock_init(&tdma->global_lock); > > + ret = clk_prepare(tdma->dma_clk); > + if (ret) > + return ret; > + > + pm_runtime_irq_safe(&pdev->dev); > pm_runtime_enable(&pdev->dev); > if (!pm_runtime_enabled(&pdev->dev)) { > ret = tegra_dma_runtime_resume(&pdev->dev); > if (ret) > - return ret; > + goto err_clk_unprepare; > } else { > ret = pm_runtime_get_sync(&pdev->dev); There is a get here but I don't see a put in probe. Jon
15.01.2020 13:08, Jon Hunter пишет: > > > On 12/01/2020 17:30, Dmitry Osipenko wrote: >> It's a bit impractical to enable hardware's clock at the time of DMA >> channel's allocation because most of DMA client drivers allocate DMA >> channel at the time of the driver's probing and thus DMA clock is kept >> always-enabled in practice, defeating the whole purpose of runtime PM. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> drivers/dma/tegra20-apb-dma.c | 43 ++++++++++++++++++++++------------- >> 1 file changed, 27 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c >> index cc4a9ca20780..b9d8e57eaf54 100644 >> --- a/drivers/dma/tegra20-apb-dma.c >> +++ b/drivers/dma/tegra20-apb-dma.c >> @@ -436,6 +436,8 @@ static void tegra_dma_stop(struct tegra_dma_channel *tdc) >> tdc_write(tdc, TEGRA_APBDMA_CHAN_STATUS, status); >> } >> tdc->busy = false; >> + >> + pm_runtime_put(tdc->tdma->dev); > > Is this the right place to call put? Seems that in terminate_all resume > is called after stop which will access the registers. Indeed, there is a problem here. Looks like resume/pause should take get/put the RPM. I'll correct it in v5, thanks. >> } >> >> static void tegra_dma_start(struct tegra_dma_channel *tdc, >> @@ -500,18 +502,25 @@ static void tegra_dma_configure_for_next(struct tegra_dma_channel *tdc, >> tegra_dma_resume(tdc); >> } >> >> -static void tdc_start_head_req(struct tegra_dma_channel *tdc) >> +static bool tdc_start_head_req(struct tegra_dma_channel *tdc) >> { >> struct tegra_dma_sg_req *sg_req; >> + int err; >> >> if (list_empty(&tdc->pending_sg_req)) >> - return; >> + return false; >> + >> + err = pm_runtime_get_sync(tdc->tdma->dev); >> + if (WARN_ON_ONCE(err < 0)) >> + return false; >> >> sg_req = list_first_entry(&tdc->pending_sg_req, typeof(*sg_req), node); >> tegra_dma_start(tdc, sg_req); >> sg_req->configured = true; >> sg_req->words_xferred = 0; >> tdc->busy = true; >> + >> + return true; >> } >> >> static void tdc_configure_next_head_desc(struct tegra_dma_channel *tdc) >> @@ -615,6 +624,8 @@ static void handle_once_dma_done(struct tegra_dma_channel *tdc, >> } >> list_add_tail(&sgreq->node, &tdc->free_sg_req); >> >> + pm_runtime_put(tdc->tdma->dev); >> + >> /* Do not start DMA if it is going to be terminate */ >> if (to_terminate || list_empty(&tdc->pending_sg_req)) >> return; >> @@ -730,9 +741,7 @@ static void tegra_dma_issue_pending(struct dma_chan *dc) >> dev_err(tdc2dev(tdc), "No DMA request\n"); >> goto end; >> } >> - if (!tdc->busy) { >> - tdc_start_head_req(tdc); >> - >> + if (!tdc->busy && tdc_start_head_req(tdc)) { >> /* Continuous single mode: Configure next req */ >> if (tdc->cyclic) { >> /* >> @@ -1280,22 +1289,15 @@ tegra_dma_prep_dma_cyclic(struct dma_chan *dc, dma_addr_t buf_addr, >> 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); >> >> - ret = pm_runtime_get_sync(tdma->dev); >> - if (ret < 0) >> - return ret; >> - >> return 0; >> } >> >> static void tegra_dma_free_chan_resources(struct dma_chan *dc) >> { >> struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc); >> - struct tegra_dma *tdma = tdc->tdma; >> struct tegra_dma_desc *dma_desc; >> struct tegra_dma_sg_req *sg_req; >> struct list_head dma_desc_list; >> @@ -1328,7 +1330,6 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc) >> list_del(&sg_req->node); >> kfree(sg_req); >> } >> - pm_runtime_put(tdma->dev); >> >> tdc->slave_id = TEGRA_APBDMA_SLAVE_ID_INVALID; >> } >> @@ -1428,11 +1429,16 @@ static int tegra_dma_probe(struct platform_device *pdev) >> >> spin_lock_init(&tdma->global_lock); >> >> + ret = clk_prepare(tdma->dma_clk); >> + if (ret) >> + return ret; >> + >> + pm_runtime_irq_safe(&pdev->dev); >> pm_runtime_enable(&pdev->dev); >> if (!pm_runtime_enabled(&pdev->dev)) { >> ret = tegra_dma_runtime_resume(&pdev->dev); >> if (ret) >> - return ret; >> + goto err_clk_unprepare; >> } else { >> ret = pm_runtime_get_sync(&pdev->dev); > > There is a get here but I don't see a put in probe. Please see: https://elixir.bootlin.com/linux/v5.5-rc6/source/drivers/dma/tegra20-apb-dma.c#L1445
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c index cc4a9ca20780..b9d8e57eaf54 100644 --- a/drivers/dma/tegra20-apb-dma.c +++ b/drivers/dma/tegra20-apb-dma.c @@ -436,6 +436,8 @@ static void tegra_dma_stop(struct tegra_dma_channel *tdc) tdc_write(tdc, TEGRA_APBDMA_CHAN_STATUS, status); } tdc->busy = false; + + pm_runtime_put(tdc->tdma->dev); } static void tegra_dma_start(struct tegra_dma_channel *tdc, @@ -500,18 +502,25 @@ static void tegra_dma_configure_for_next(struct tegra_dma_channel *tdc, tegra_dma_resume(tdc); } -static void tdc_start_head_req(struct tegra_dma_channel *tdc) +static bool tdc_start_head_req(struct tegra_dma_channel *tdc) { struct tegra_dma_sg_req *sg_req; + int err; if (list_empty(&tdc->pending_sg_req)) - return; + return false; + + err = pm_runtime_get_sync(tdc->tdma->dev); + if (WARN_ON_ONCE(err < 0)) + return false; sg_req = list_first_entry(&tdc->pending_sg_req, typeof(*sg_req), node); tegra_dma_start(tdc, sg_req); sg_req->configured = true; sg_req->words_xferred = 0; tdc->busy = true; + + return true; } static void tdc_configure_next_head_desc(struct tegra_dma_channel *tdc) @@ -615,6 +624,8 @@ static void handle_once_dma_done(struct tegra_dma_channel *tdc, } list_add_tail(&sgreq->node, &tdc->free_sg_req); + pm_runtime_put(tdc->tdma->dev); + /* Do not start DMA if it is going to be terminate */ if (to_terminate || list_empty(&tdc->pending_sg_req)) return; @@ -730,9 +741,7 @@ static void tegra_dma_issue_pending(struct dma_chan *dc) dev_err(tdc2dev(tdc), "No DMA request\n"); goto end; } - if (!tdc->busy) { - tdc_start_head_req(tdc); - + if (!tdc->busy && tdc_start_head_req(tdc)) { /* Continuous single mode: Configure next req */ if (tdc->cyclic) { /* @@ -1280,22 +1289,15 @@ tegra_dma_prep_dma_cyclic(struct dma_chan *dc, dma_addr_t buf_addr, 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); - ret = pm_runtime_get_sync(tdma->dev); - if (ret < 0) - return ret; - return 0; } static void tegra_dma_free_chan_resources(struct dma_chan *dc) { struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc); - struct tegra_dma *tdma = tdc->tdma; struct tegra_dma_desc *dma_desc; struct tegra_dma_sg_req *sg_req; struct list_head dma_desc_list; @@ -1328,7 +1330,6 @@ static void tegra_dma_free_chan_resources(struct dma_chan *dc) list_del(&sg_req->node); kfree(sg_req); } - pm_runtime_put(tdma->dev); tdc->slave_id = TEGRA_APBDMA_SLAVE_ID_INVALID; } @@ -1428,11 +1429,16 @@ static int tegra_dma_probe(struct platform_device *pdev) spin_lock_init(&tdma->global_lock); + ret = clk_prepare(tdma->dma_clk); + if (ret) + return ret; + + pm_runtime_irq_safe(&pdev->dev); pm_runtime_enable(&pdev->dev); if (!pm_runtime_enabled(&pdev->dev)) { ret = tegra_dma_runtime_resume(&pdev->dev); if (ret) - return ret; + goto err_clk_unprepare; } else { ret = pm_runtime_get_sync(&pdev->dev); if (ret < 0) @@ -1552,6 +1558,9 @@ static int tegra_dma_probe(struct platform_device *pdev) else pm_runtime_disable(&pdev->dev); +err_clk_unprepare: + clk_unprepare(tdma->dma_clk); + return ret; } @@ -1566,6 +1575,8 @@ static int tegra_dma_remove(struct platform_device *pdev) else pm_runtime_disable(&pdev->dev); + clk_unprepare(tdma->dma_clk); + return 0; } @@ -1593,7 +1604,7 @@ static int tegra_dma_runtime_suspend(struct device *dev) TEGRA_APBDMA_CHAN_WCOUNT); } - clk_disable_unprepare(tdma->dma_clk); + clk_disable(tdma->dma_clk); return 0; } @@ -1604,7 +1615,7 @@ static int tegra_dma_runtime_resume(struct device *dev) unsigned int i; int ret; - ret = clk_prepare_enable(tdma->dma_clk); + ret = clk_enable(tdma->dma_clk); if (ret < 0) { dev_err(dev, "clk_enable failed: %d\n", ret); return ret;
It's a bit impractical to enable hardware's clock at the time of DMA channel's allocation because most of DMA client drivers allocate DMA channel at the time of the driver's probing and thus DMA clock is kept always-enabled in practice, defeating the whole purpose of runtime PM. Signed-off-by: Dmitry Osipenko <digetx@gmail.com> --- drivers/dma/tegra20-apb-dma.c | 43 ++++++++++++++++++++++------------- 1 file changed, 27 insertions(+), 16 deletions(-)