diff mbox series

[3/6] dmaengine: mxs: add the power management functions

Message ID 1579038243-28550-4-git-send-email-han.xu@nxp.com
State Not Applicable
Headers show
Series gpmi/mxs-dma runtime pm patch set | expand

Commit Message

Han Xu Jan. 14, 2020, 9:44 p.m. UTC 
add the power management functions and leverage the runtime pm for
system suspend/resume

Signed-off-by: Han Xu <han.xu@nxp.com>
---
 drivers/dma/mxs-dma.c | 97 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 90 insertions(+), 7 deletions(-)

Comments

Sascha Hauer Jan. 15, 2020, 8:02 a.m. UTC | #1 
On Wed, Jan 15, 2020 at 05:44:00AM +0800, Han Xu wrote:
> add the power management functions and leverage the runtime pm for
> system suspend/resume
> 
> Signed-off-by: Han Xu <han.xu@nxp.com>
> ---
>  drivers/dma/mxs-dma.c | 97 +++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 90 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> index b458f06f9067..251492c5ea58 100644
> --- a/drivers/dma/mxs-dma.c
> +++ b/drivers/dma/mxs-dma.c
> @@ -25,6 +25,7 @@
>  #include <linux/of_dma.h>
>  #include <linux/list.h>
>  #include <linux/dma/mxs-dma.h>
> +#include <linux/pm_runtime.h>
>  
>  #include <asm/irq.h>
>  
> @@ -39,6 +40,8 @@
>  #define dma_is_apbh(mxs_dma)	((mxs_dma)->type == MXS_DMA_APBH)
>  #define apbh_is_old(mxs_dma)	((mxs_dma)->dev_id == IMX23_DMA)
>  
> +#define MXS_DMA_RPM_TIMEOUT 50 /* ms */
> +
>  #define HW_APBHX_CTRL0				0x000
>  #define BM_APBH_CTRL0_APB_BURST8_EN		(1 << 29)
>  #define BM_APBH_CTRL0_APB_BURST_EN		(1 << 28)
> @@ -416,6 +419,7 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
>  {
>  	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
>  	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> +	struct device *dev = &mxs_dma->pdev->dev;
>  	int ret;
>  
>  	mxs_chan->ccw = dma_alloc_coherent(mxs_dma->dma_device.dev,
> @@ -431,9 +435,11 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
>  	if (ret)
>  		goto err_irq;
>  
> -	ret = clk_prepare_enable(mxs_dma->clk);
> -	if (ret)
> +	ret = pm_runtime_get_sync(dev);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to enable clock\n");
>  		goto err_clk;

From looking at other DMA drivers I know we are in good company here,
but I think this is wrong. Doing pm_runtime_get_sync() in
alloc_chan_resources() and going to autosuspend in free_chan_resources()
effectively disables runtime_pm as clients normally acquire their
channels during driver probe and release them only in driver remove.

In the next patch you release the DMA channels in the GPMI nand drivers
runtime_suspend hook just to somehow trigger the runtime_suspend of the
DMA driver.

What you should do instead is to make sure the hook runtime_pm to the
DMA drivers activity phases, like for example the pl330 driver does.
Then you wouldn't have to care about manually putting the DMA driver into
suspend from the GPMI NAND driver.

Sascha
Han Xu Jan. 16, 2020, 4:36 p.m. UTC | #2 
On Wed, Jan 15, 2020 at 2:03 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Wed, Jan 15, 2020 at 05:44:00AM +0800, Han Xu wrote:
> > add the power management functions and leverage the runtime pm for
> > system suspend/resume
> >
> > Signed-off-by: Han Xu <han.xu@nxp.com>
> > ---
> >  drivers/dma/mxs-dma.c | 97 +++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 90 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> > index b458f06f9067..251492c5ea58 100644
> > --- a/drivers/dma/mxs-dma.c
> > +++ b/drivers/dma/mxs-dma.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/of_dma.h>
> >  #include <linux/list.h>
> >  #include <linux/dma/mxs-dma.h>
> > +#include <linux/pm_runtime.h>
> >
> >  #include <asm/irq.h>
> >
> > @@ -39,6 +40,8 @@
> >  #define dma_is_apbh(mxs_dma) ((mxs_dma)->type == MXS_DMA_APBH)
> >  #define apbh_is_old(mxs_dma) ((mxs_dma)->dev_id == IMX23_DMA)
> >
> > +#define MXS_DMA_RPM_TIMEOUT 50 /* ms */
> > +
> >  #define HW_APBHX_CTRL0                               0x000
> >  #define BM_APBH_CTRL0_APB_BURST8_EN          (1 << 29)
> >  #define BM_APBH_CTRL0_APB_BURST_EN           (1 << 28)
> > @@ -416,6 +419,7 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
> >  {
> >       struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> >       struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> > +     struct device *dev = &mxs_dma->pdev->dev;
> >       int ret;
> >
> >       mxs_chan->ccw = dma_alloc_coherent(mxs_dma->dma_device.dev,
> > @@ -431,9 +435,11 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
> >       if (ret)
> >               goto err_irq;
> >
> > -     ret = clk_prepare_enable(mxs_dma->clk);
> > -     if (ret)
> > +     ret = pm_runtime_get_sync(dev);
> > +     if (ret < 0) {
> > +             dev_err(dev, "Failed to enable clock\n");
> >               goto err_clk;
>
> From looking at other DMA drivers I know we are in good company here,
> but I think this is wrong. Doing pm_runtime_get_sync() in
> alloc_chan_resources() and going to autosuspend in free_chan_resources()
> effectively disables runtime_pm as clients normally acquire their
> channels during driver probe and release them only in driver remove.

Thanks for the comments.
That's why I moved acquire_dma_resource from the probe to
runtime_resume in the gpmi driver, this change won't disable the
runtime_pm function and the incremental counter always balanced.

>
> In the next patch you release the DMA channels in the GPMI nand drivers
> runtime_suspend hook just to somehow trigger the runtime_suspend of the
> DMA driver.
>
> What you should do instead is to make sure the hook runtime_pm to the
> DMA drivers activity phases, like for example the pl330 driver does.
> Then you wouldn't have to care about manually putting the DMA driver into
> suspend from the GPMI NAND driver.
>
> Sascha
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
Sascha Hauer Jan. 17, 2020, 8:13 a.m. UTC | #3 
On Thu, Jan 16, 2020 at 10:36:33AM -0600, Han Xu wrote:
> On Wed, Jan 15, 2020 at 2:03 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > On Wed, Jan 15, 2020 at 05:44:00AM +0800, Han Xu wrote:
> > > add the power management functions and leverage the runtime pm for
> > > system suspend/resume
> > >
> > > Signed-off-by: Han Xu <han.xu@nxp.com>
> > > ---
> > >  drivers/dma/mxs-dma.c | 97 +++++++++++++++++++++++++++++++++++++++----
> > >  1 file changed, 90 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> > > index b458f06f9067..251492c5ea58 100644
> > > --- a/drivers/dma/mxs-dma.c
> > > +++ b/drivers/dma/mxs-dma.c
> > > @@ -25,6 +25,7 @@
> > >  #include <linux/of_dma.h>
> > >  #include <linux/list.h>
> > >  #include <linux/dma/mxs-dma.h>
> > > +#include <linux/pm_runtime.h>
> > >
> > >  #include <asm/irq.h>
> > >
> > > @@ -39,6 +40,8 @@
> > >  #define dma_is_apbh(mxs_dma) ((mxs_dma)->type == MXS_DMA_APBH)
> > >  #define apbh_is_old(mxs_dma) ((mxs_dma)->dev_id == IMX23_DMA)
> > >
> > > +#define MXS_DMA_RPM_TIMEOUT 50 /* ms */
> > > +
> > >  #define HW_APBHX_CTRL0                               0x000
> > >  #define BM_APBH_CTRL0_APB_BURST8_EN          (1 << 29)
> > >  #define BM_APBH_CTRL0_APB_BURST_EN           (1 << 28)
> > > @@ -416,6 +419,7 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
> > >  {
> > >       struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
> > >       struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
> > > +     struct device *dev = &mxs_dma->pdev->dev;
> > >       int ret;
> > >
> > >       mxs_chan->ccw = dma_alloc_coherent(mxs_dma->dma_device.dev,
> > > @@ -431,9 +435,11 @@ static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
> > >       if (ret)
> > >               goto err_irq;
> > >
> > > -     ret = clk_prepare_enable(mxs_dma->clk);
> > > -     if (ret)
> > > +     ret = pm_runtime_get_sync(dev);
> > > +     if (ret < 0) {
> > > +             dev_err(dev, "Failed to enable clock\n");
> > >               goto err_clk;
> >
> > From looking at other DMA drivers I know we are in good company here,
> > but I think this is wrong. Doing pm_runtime_get_sync() in
> > alloc_chan_resources() and going to autosuspend in free_chan_resources()
> > effectively disables runtime_pm as clients normally acquire their
> > channels during driver probe and release them only in driver remove.
> 
> Thanks for the comments.
> That's why I moved acquire_dma_resource from the probe to
> runtime_resume in the gpmi driver, this change won't disable the
> runtime_pm function and the incremental counter always balanced.

Yes, that's what I've written a few lines further down:

> 
> >
> > In the next patch you release the DMA channels in the GPMI nand drivers
> > runtime_suspend hook just to somehow trigger the runtime_suspend of the
> > DMA driver.

And I consider doing this a crude hack. Here is what I suggested doing
instead:

> >
> > What you should do instead is to make sure the hook runtime_pm to the
> > DMA drivers activity phases, like for example the pl330 driver does.
> > Then you wouldn't have to care about manually putting the DMA driver into
> > suspend from the GPMI NAND driver.

Sascha
diff mbox series

Patch

diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
index b458f06f9067..251492c5ea58 100644
--- a/drivers/dma/mxs-dma.c
+++ b/drivers/dma/mxs-dma.c
@@ -25,6 +25,7 @@ 
 #include <linux/of_dma.h>
 #include <linux/list.h>
 #include <linux/dma/mxs-dma.h>
+#include <linux/pm_runtime.h>
 
 #include <asm/irq.h>
 
@@ -39,6 +40,8 @@ 
 #define dma_is_apbh(mxs_dma)	((mxs_dma)->type == MXS_DMA_APBH)
 #define apbh_is_old(mxs_dma)	((mxs_dma)->dev_id == IMX23_DMA)
 
+#define MXS_DMA_RPM_TIMEOUT 50 /* ms */
+
 #define HW_APBHX_CTRL0				0x000
 #define BM_APBH_CTRL0_APB_BURST8_EN		(1 << 29)
 #define BM_APBH_CTRL0_APB_BURST_EN		(1 << 28)
@@ -416,6 +419,7 @@  static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
 {
 	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
 	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
+	struct device *dev = &mxs_dma->pdev->dev;
 	int ret;
 
 	mxs_chan->ccw = dma_alloc_coherent(mxs_dma->dma_device.dev,
@@ -431,9 +435,11 @@  static int mxs_dma_alloc_chan_resources(struct dma_chan *chan)
 	if (ret)
 		goto err_irq;
 
-	ret = clk_prepare_enable(mxs_dma->clk);
-	if (ret)
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		dev_err(dev, "Failed to enable clock\n");
 		goto err_clk;
+	}
 
 	mxs_dma_reset_chan(chan);
 
@@ -458,6 +464,7 @@  static void mxs_dma_free_chan_resources(struct dma_chan *chan)
 {
 	struct mxs_dma_chan *mxs_chan = to_mxs_dma_chan(chan);
 	struct mxs_dma_engine *mxs_dma = mxs_chan->mxs_dma;
+	struct device *dev = &mxs_dma->pdev->dev;
 
 	mxs_dma_disable_chan(chan);
 
@@ -466,7 +473,9 @@  static void mxs_dma_free_chan_resources(struct dma_chan *chan)
 	dma_free_coherent(mxs_dma->dma_device.dev, CCW_BLOCK_SIZE,
 			mxs_chan->ccw, mxs_chan->ccw_phys);
 
-	clk_disable_unprepare(mxs_dma->clk);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
 }
 
 /*
@@ -689,14 +698,32 @@  static enum dma_status mxs_dma_tx_status(struct dma_chan *chan,
 	return mxs_chan->status;
 }
 
-static int __init mxs_dma_init(struct mxs_dma_engine *mxs_dma)
+static int mxs_dma_init_rpm(struct mxs_dma_engine *mxs_dma)
 {
+	struct device *dev = &mxs_dma->pdev->dev;
+
+	pm_runtime_enable(dev);
+	pm_runtime_set_autosuspend_delay(dev, MXS_DMA_RPM_TIMEOUT);
+	pm_runtime_use_autosuspend(dev);
+
+	return 0;
+}
+
+static int mxs_dma_init(struct mxs_dma_engine *mxs_dma)
+{
+	struct device *dev = &mxs_dma->pdev->dev;
 	int ret;
 
-	ret = clk_prepare_enable(mxs_dma->clk);
+	ret = mxs_dma_init_rpm(mxs_dma);
 	if (ret)
 		return ret;
 
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		dev_err(dev, "Failed to enable clock\n");
+		return ret;
+	}
+
 	ret = stmp_reset_block(mxs_dma->base);
 	if (ret)
 		goto err_out;
@@ -714,7 +741,8 @@  static int __init mxs_dma_init(struct mxs_dma_engine *mxs_dma)
 		mxs_dma->base + HW_APBHX_CTRL1 + STMP_OFFSET_REG_SET);
 
 err_out:
-	clk_disable_unprepare(mxs_dma->clk);
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
 	return ret;
 }
 
@@ -821,11 +849,13 @@  static int mxs_dma_probe(struct platform_device *pdev)
 			&mxs_dma->dma_device.channels);
 	}
 
+	platform_set_drvdata(pdev, mxs_dma);
+	mxs_dma->pdev = pdev;
+
 	ret = mxs_dma_init(mxs_dma);
 	if (ret)
 		return ret;
 
-	mxs_dma->pdev = pdev;
 	mxs_dma->dma_device.dev = &pdev->dev;
 
 	/* mxs_dma gets 65535 bytes maximum sg size */
@@ -879,9 +909,62 @@  static int mxs_dma_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int mxs_dma_pm_suspend(struct device *dev)
+{
+	int ret;
+
+	ret = pm_runtime_force_suspend(dev);
+
+	return ret;
+}
+
+static int mxs_dma_pm_resume(struct device *dev)
+{
+	struct mxs_dma_engine *mxs_dma = dev_get_drvdata(dev);
+	int ret;
+
+	ret = mxs_dma_init(mxs_dma);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+#endif
+
+int mxs_dma_runtime_suspend(struct device *dev)
+{
+	struct mxs_dma_engine *mxs_dma = dev_get_drvdata(dev);
+
+	clk_disable_unprepare(mxs_dma->clk);
+
+	return 0;
+}
+
+int mxs_dma_runtime_resume(struct device *dev)
+{
+	struct mxs_dma_engine *mxs_dma = dev_get_drvdata(dev);
+	int ret;
+
+	ret = clk_prepare_enable(mxs_dma->clk);
+	if (ret) {
+		dev_err(&mxs_dma->pdev->dev, "failed to enable the clock\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct dev_pm_ops mxs_dma_pm_ops = {
+	SET_RUNTIME_PM_OPS(mxs_dma_runtime_suspend,
+			   mxs_dma_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(mxs_dma_pm_suspend, mxs_dma_pm_resume)
+};
+
 static struct platform_driver mxs_dma_driver = {
 	.driver		= {
 		.name	= "mxs-dma",
+		.pm = &mxs_dma_pm_ops,
 		.of_match_table = mxs_dma_dt_ids,
 	},
 	.id_table	= mxs_dma_ids,