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

From: Sascha Hauer <s.hauer@pengutronix.de>
To: Han Xu <han.xu@nxp.com>
Cc: vigneshr@ti.com, richard@nod.at, esben@geanix.com,
	linux-kernel@vger.kernel.org, vkoul@kernel.org,
	boris.brezillon@collabora.com, linux-mtd@lists.infradead.org,
	linux-imx@nxp.com, festevam@gmail.com, miquel.raynal@bootlin.com,
	dmaengine@vger.kernel.org, shawnguo@kernel.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 3/6] dmaengine: mxs: add the power management functions
Date: Wed, 15 Jan 2020 09:02:57 +0100	[thread overview]
Message-ID: <20200115080257.dtd4vss4uhopbvn2@pengutronix.de> (raw)
In-Reply-To: <1579038243-28550-4-git-send-email-han.xu@nxp.com>

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

-- 
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/