| Message ID | 1496753369-5356-1-git-send-email-jonathanh@nvidia.com |
|---|---|
| State | Deferred |
| Headers | show |
On Tue, Jun 06, 2017 at 01:49:29PM +0100, Jon Hunter wrote: > Commit edd3bdbe9db1 ("dmaengine: tegra-apb: Correct runtime-pm usage") > added pm_runtime_get/put() calls to the tegra-apb DMA system suspend > callbacks. Runtime PM is disabled during system suspend and so these > APIs cannot be used. Fix the suspend handling for the tegra-apb DMA by > moving the save and restore of the DMA register context into the > runtime PM suspend and resume callbacks, and then use the > pm_runtime_force_suspend/resume() APIs to invoke the runtime PM > callbacks during system suspend. > > Fixes: edd3bdbe9db1 ("dmaengine: tegra-apb: Correct runtime-pm usage") > > Signed-off-by: Jon Hunter <jonathanh@nvidia.com> > --- > > Changes since V1: > - Drop the custom suspend/resume callbacks and use > pm_runtime_force_suspend/resume directly in SET_SYSTEM_SLEEP_PM_OPS. > > drivers/dma/tegra20-apb-dma.c | 50 +++++++++---------------------------------- > 1 file changed, 10 insertions(+), 40 deletions(-) > > diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c > index 3722b9d8d9fe..b9d75a54c896 100644 > --- a/drivers/dma/tegra20-apb-dma.c > +++ b/drivers/dma/tegra20-apb-dma.c > @@ -1494,35 +1494,7 @@ static int tegra_dma_remove(struct platform_device *pdev) > static int tegra_dma_runtime_suspend(struct device *dev) > { > struct tegra_dma *tdma = dev_get_drvdata(dev); > - > - clk_disable_unprepare(tdma->dma_clk); > - return 0; > -} > - > -static int tegra_dma_runtime_resume(struct device *dev) > -{ > - struct tegra_dma *tdma = dev_get_drvdata(dev); > - int ret; > - > - ret = clk_prepare_enable(tdma->dma_clk); > - if (ret < 0) { > - dev_err(dev, "clk_enable failed: %d\n", ret); > - return ret; > - } > - return 0; > -} > - > -#ifdef CONFIG_PM_SLEEP > -static int tegra_dma_pm_suspend(struct device *dev) > -{ > - struct tegra_dma *tdma = dev_get_drvdata(dev); > int i; > - int ret; > - > - /* Enable clock before accessing register */ > - ret = pm_runtime_get_sync(dev); > - if (ret < 0) > - return ret; > > tdma->reg_gen = tdma_read(tdma, TEGRA_APBDMA_GENERAL); > for (i = 0; i < tdma->chip_data->nr_channels; i++) { > @@ -1543,21 +1515,21 @@ static int tegra_dma_pm_suspend(struct device *dev) > TEGRA_APBDMA_CHAN_WCOUNT); > } > > - /* Disable clock */ > - pm_runtime_put(dev); > + clk_disable_unprepare(tdma->dma_clk); > + > return 0; > } > > -static int tegra_dma_pm_resume(struct device *dev) > +static int tegra_dma_runtime_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 = pm_runtime_get_sync(dev); > - if (ret < 0) > + ret = clk_prepare_enable(tdma->dma_clk); > + if (ret < 0) { > + dev_err(dev, "clk_enable failed: %d\n", ret); > return ret; > + } > > tdma_write(tdma, TEGRA_APBDMA_GENERAL, tdma->reg_gen); > tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0); > @@ -1582,16 +1554,14 @@ static int tegra_dma_pm_resume(struct device *dev) > (ch_reg->csr & ~TEGRA_APBDMA_CSR_ENB)); > } > > - /* Disable clock */ > - pm_runtime_put(dev); > return 0; > } > -#endif > > static const struct dev_pm_ops tegra_dma_dev_pm_ops = { > 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) > + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > + pm_runtime_force_resume) Is that even necessary? I thought runtime PM was going to be triggered for system sleep anyway, but it looks like there are other examples of this usage, so maybe I'm mistaken. Thierry
On 06/06/17 15:48, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Tue, Jun 06, 2017 at 01:49:29PM +0100, Jon Hunter wrote: >> Commit edd3bdbe9db1 ("dmaengine: tegra-apb: Correct runtime-pm usage") >> added pm_runtime_get/put() calls to the tegra-apb DMA system suspend >> callbacks. Runtime PM is disabled during system suspend and so these >> APIs cannot be used. Fix the suspend handling for the tegra-apb DMA by >> moving the save and restore of the DMA register context into the >> runtime PM suspend and resume callbacks, and then use the >> pm_runtime_force_suspend/resume() APIs to invoke the runtime PM >> callbacks during system suspend. >> >> Fixes: edd3bdbe9db1 ("dmaengine: tegra-apb: Correct runtime-pm usage") >> >> Signed-off-by: Jon Hunter <jonathanh@nvidia.com> >> --- >> >> Changes since V1: >> - Drop the custom suspend/resume callbacks and use >> pm_runtime_force_suspend/resume directly in SET_SYSTEM_SLEEP_PM_OPS. >> >> drivers/dma/tegra20-apb-dma.c | 50 +++++++++---------------------------------- >> 1 file changed, 10 insertions(+), 40 deletions(-) >> >> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c >> index 3722b9d8d9fe..b9d75a54c896 100644 >> --- a/drivers/dma/tegra20-apb-dma.c >> +++ b/drivers/dma/tegra20-apb-dma.c >> @@ -1494,35 +1494,7 @@ static int tegra_dma_remove(struct platform_device *pdev) >> static int tegra_dma_runtime_suspend(struct device *dev) >> { >> struct tegra_dma *tdma = dev_get_drvdata(dev); >> - >> - clk_disable_unprepare(tdma->dma_clk); >> - return 0; >> -} >> - >> -static int tegra_dma_runtime_resume(struct device *dev) >> -{ >> - struct tegra_dma *tdma = dev_get_drvdata(dev); >> - int ret; >> - >> - ret = clk_prepare_enable(tdma->dma_clk); >> - if (ret < 0) { >> - dev_err(dev, "clk_enable failed: %d\n", ret); >> - return ret; >> - } >> - return 0; >> -} >> - >> -#ifdef CONFIG_PM_SLEEP >> -static int tegra_dma_pm_suspend(struct device *dev) >> -{ >> - struct tegra_dma *tdma = dev_get_drvdata(dev); >> int i; >> - int ret; >> - >> - /* Enable clock before accessing register */ >> - ret = pm_runtime_get_sync(dev); >> - if (ret < 0) >> - return ret; >> >> tdma->reg_gen = tdma_read(tdma, TEGRA_APBDMA_GENERAL); >> for (i = 0; i < tdma->chip_data->nr_channels; i++) { >> @@ -1543,21 +1515,21 @@ static int tegra_dma_pm_suspend(struct device *dev) >> TEGRA_APBDMA_CHAN_WCOUNT); >> } >> >> - /* Disable clock */ >> - pm_runtime_put(dev); >> + clk_disable_unprepare(tdma->dma_clk); >> + >> return 0; >> } >> >> -static int tegra_dma_pm_resume(struct device *dev) >> +static int tegra_dma_runtime_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 = pm_runtime_get_sync(dev); >> - if (ret < 0) >> + ret = clk_prepare_enable(tdma->dma_clk); >> + if (ret < 0) { >> + dev_err(dev, "clk_enable failed: %d\n", ret); >> return ret; >> + } >> >> tdma_write(tdma, TEGRA_APBDMA_GENERAL, tdma->reg_gen); >> tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0); >> @@ -1582,16 +1554,14 @@ static int tegra_dma_pm_resume(struct device *dev) >> (ch_reg->csr & ~TEGRA_APBDMA_CSR_ENB)); >> } >> >> - /* Disable clock */ >> - pm_runtime_put(dev); >> return 0; >> } >> -#endif >> >> static const struct dev_pm_ops tegra_dma_dev_pm_ops = { >> 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) >> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, >> + pm_runtime_force_resume) > > Is that even necessary? I thought runtime PM was going to be triggered > for system sleep anyway, but it looks like there are other examples of > this usage, so maybe I'm mistaken. Yes this is necessary. No RPM is not automatically trigger by system suspend AFAICT. Cheers Jon
On Tue, Jun 27, 2017 at 12:44:58PM +0100, Jon Hunter wrote: > >> static const struct dev_pm_ops tegra_dma_dev_pm_ops = { > >> 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) > >> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > >> + pm_runtime_force_resume) > > > > Is that even necessary? I thought runtime PM was going to be triggered > > for system sleep anyway, but it looks like there are other examples of > > this usage, so maybe I'm mistaken. > > Yes this is necessary. No RPM is not automatically trigger by system > suspend AFAICT. Yes I was earlier under the same impression but later did realize that the behaviour seems to be arch specific and we don't have guarantee on this
On Tue, Jun 06, 2017 at 01:49:29PM +0100, Jon Hunter wrote: > Commit edd3bdbe9db1 ("dmaengine: tegra-apb: Correct runtime-pm usage") > added pm_runtime_get/put() calls to the tegra-apb DMA system suspend > callbacks. Runtime PM is disabled during system suspend and so these > APIs cannot be used. Fix the suspend handling for the tegra-apb DMA by > moving the save and restore of the DMA register context into the > runtime PM suspend and resume callbacks, and then use the > pm_runtime_force_suspend/resume() APIs to invoke the runtime PM > callbacks during system suspend. Applied, thanks
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c index 3722b9d8d9fe..b9d75a54c896 100644 --- a/drivers/dma/tegra20-apb-dma.c +++ b/drivers/dma/tegra20-apb-dma.c @@ -1494,35 +1494,7 @@ static int tegra_dma_remove(struct platform_device *pdev) static int tegra_dma_runtime_suspend(struct device *dev) { struct tegra_dma *tdma = dev_get_drvdata(dev); - - clk_disable_unprepare(tdma->dma_clk); - return 0; -} - -static int tegra_dma_runtime_resume(struct device *dev) -{ - struct tegra_dma *tdma = dev_get_drvdata(dev); - int ret; - - ret = clk_prepare_enable(tdma->dma_clk); - if (ret < 0) { - dev_err(dev, "clk_enable failed: %d\n", ret); - return ret; - } - return 0; -} - -#ifdef CONFIG_PM_SLEEP -static int tegra_dma_pm_suspend(struct device *dev) -{ - struct tegra_dma *tdma = dev_get_drvdata(dev); int i; - int ret; - - /* Enable clock before accessing register */ - ret = pm_runtime_get_sync(dev); - if (ret < 0) - return ret; tdma->reg_gen = tdma_read(tdma, TEGRA_APBDMA_GENERAL); for (i = 0; i < tdma->chip_data->nr_channels; i++) { @@ -1543,21 +1515,21 @@ static int tegra_dma_pm_suspend(struct device *dev) TEGRA_APBDMA_CHAN_WCOUNT); } - /* Disable clock */ - pm_runtime_put(dev); + clk_disable_unprepare(tdma->dma_clk); + return 0; } -static int tegra_dma_pm_resume(struct device *dev) +static int tegra_dma_runtime_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 = pm_runtime_get_sync(dev); - if (ret < 0) + ret = clk_prepare_enable(tdma->dma_clk); + if (ret < 0) { + dev_err(dev, "clk_enable failed: %d\n", ret); return ret; + } tdma_write(tdma, TEGRA_APBDMA_GENERAL, tdma->reg_gen); tdma_write(tdma, TEGRA_APBDMA_CONTROL, 0); @@ -1582,16 +1554,14 @@ static int tegra_dma_pm_resume(struct device *dev) (ch_reg->csr & ~TEGRA_APBDMA_CSR_ENB)); } - /* Disable clock */ - pm_runtime_put(dev); return 0; } -#endif static const struct dev_pm_ops tegra_dma_dev_pm_ops = { 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) + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, + pm_runtime_force_resume) }; static const struct of_device_id tegra_dma_of_match[] = {
Commit edd3bdbe9db1 ("dmaengine: tegra-apb: Correct runtime-pm usage") added pm_runtime_get/put() calls to the tegra-apb DMA system suspend callbacks. Runtime PM is disabled during system suspend and so these APIs cannot be used. Fix the suspend handling for the tegra-apb DMA by moving the save and restore of the DMA register context into the runtime PM suspend and resume callbacks, and then use the pm_runtime_force_suspend/resume() APIs to invoke the runtime PM callbacks during system suspend. Fixes: edd3bdbe9db1 ("dmaengine: tegra-apb: Correct runtime-pm usage") Signed-off-by: Jon Hunter <jonathanh@nvidia.com> --- Changes since V1: - Drop the custom suspend/resume callbacks and use pm_runtime_force_suspend/resume directly in SET_SYSTEM_SLEEP_PM_OPS. drivers/dma/tegra20-apb-dma.c | 50 +++++++++---------------------------------- 1 file changed, 10 insertions(+), 40 deletions(-)