[v2,1/5] media: cadence: csi2rx: Support runtime PM

  Use runtime power management hooks to save power when CSI-RX is not in
use.

Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
---
 drivers/media/platform/cadence/cdns-csi2rx.c | 121 ++++++++++++-------
 1 file changed, 80 insertions(+), 41 deletions(-)
  

Hello Changhuang

On Wed, Jul 17, 2024 at 08:28:30PM GMT, Changhuang Liang wrote:
> Use runtime power management hooks to save power when CSI-RX is not in
> use.
>

The driver does not depend on PM afaict and the IP can be integrated in
different SoCs and not all of them might select PM.

Either make it depend on PM in Kconfig or manually enable the device during
probe (see the many examples of drivers manually enabling the device
in probe() then calling pm_runtime_set_active(), pm_runtime_enable()
and pm_request_idle()).

Also, you might want to consider autosuspend delay. Autosuspend delay
avoids power cycling the interface by delaying suspend by a certain
amout of time, in case it resumed immediately.

> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> ---
>  drivers/media/platform/cadence/cdns-csi2rx.c | 121 ++++++++++++-------
>  1 file changed, 80 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> index 6f7d27a48eff..981819adbb3a 100644
> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -211,11 +211,6 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>  	u32 reg;
>  	int ret;
>
> -	ret = clk_prepare_enable(csi2rx->p_clk);
> -	if (ret)
> -		return ret;
> -
> -	reset_control_deassert(csi2rx->p_rst);
>  	csi2rx_reset(csi2rx);
>
>  	reg = csi2rx->num_lanes << 8;
> @@ -253,7 +248,7 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>  		if (ret) {
>  			dev_err(csi2rx->dev,
>  				"Failed to configure external DPHY: %d\n", ret);
> -			goto err_disable_pclk;
> +			return ret;
>  		}
>  	}
>
> @@ -268,11 +263,6 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>  	 * hence the reference counting.
>  	 */
>  	for (i = 0; i < csi2rx->max_streams; i++) {
> -		ret = clk_prepare_enable(csi2rx->pixel_clk[i]);
> -		if (ret)
> -			goto err_disable_pixclk;
> -
> -		reset_control_deassert(csi2rx->pixel_rst[i]);
>
>  		writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,
>  		       csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
> @@ -288,34 +278,18 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>  		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
>  	}
>
> -	ret = clk_prepare_enable(csi2rx->sys_clk);
> -	if (ret)
> -		goto err_disable_pixclk;
> -
> -	reset_control_deassert(csi2rx->sys_rst);
>
>  	ret = v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, true);
>  	if (ret)
> -		goto err_disable_sysclk;
> -
> -	clk_disable_unprepare(csi2rx->p_clk);
> +		goto err_phy_power_off;
>
>  	return 0;
>
> -err_disable_sysclk:
> -	clk_disable_unprepare(csi2rx->sys_clk);
> -err_disable_pixclk:
> -	for (; i > 0; i--) {
> -		reset_control_assert(csi2rx->pixel_rst[i - 1]);
> -		clk_disable_unprepare(csi2rx->pixel_clk[i - 1]);
> -	}
> -
> +err_phy_power_off:
>  	if (csi2rx->dphy) {
>  		writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG);
>  		phy_power_off(csi2rx->dphy);
>  	}
> -err_disable_pclk:
> -	clk_disable_unprepare(csi2rx->p_clk);
>
>  	return ret;
>  }
> @@ -326,10 +300,6 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
>  	u32 val;
>  	int ret;
>
> -	clk_prepare_enable(csi2rx->p_clk);
> -	reset_control_assert(csi2rx->sys_rst);
> -	clk_disable_unprepare(csi2rx->sys_clk);
> -
>  	for (i = 0; i < csi2rx->max_streams; i++) {
>  		writel(CSI2RX_STREAM_CTRL_STOP,
>  		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> @@ -342,14 +312,8 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
>  		if (ret)
>  			dev_warn(csi2rx->dev,
>  				 "Failed to stop streaming on pad%u\n", i);
> -
> -		reset_control_assert(csi2rx->pixel_rst[i]);
> -		clk_disable_unprepare(csi2rx->pixel_clk[i]);
>  	}
>
> -	reset_control_assert(csi2rx->p_rst);
> -	clk_disable_unprepare(csi2rx->p_clk);
> -
>  	if (v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false))
>  		dev_warn(csi2rx->dev, "Couldn't disable our subdev\n");
>
> @@ -374,9 +338,15 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
>  		 * enable the whole controller.
>  		 */
>  		if (!csi2rx->count) {
> +			ret = pm_runtime_resume_and_get(csi2rx->dev);
> +			if (ret < 0)
> +				goto out;
> +
>  			ret = csi2rx_start(csi2rx);
> -			if (ret)
> +			if (ret) {
> +				pm_runtime_put(csi2rx->dev);
>  				goto out;
> +			}
>  		}
>
>  		csi2rx->count++;
> @@ -386,8 +356,10 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
>  		/*
>  		 * Let the last user turn off the lights.
>  		 */
> -		if (!csi2rx->count)
> +		if (!csi2rx->count) {
>  			csi2rx_stop(csi2rx);
> +			pm_runtime_put(csi2rx->dev);
> +		}
>  	}
>
>  out:
> @@ -707,6 +679,7 @@ static int csi2rx_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_cleanup;
>
> +	pm_runtime_enable(csi2rx->dev);
>  	ret = v4l2_async_register_subdev(&csi2rx->subdev);
>  	if (ret < 0)
>  		goto err_free_state;
> @@ -721,6 +694,7 @@ static int csi2rx_probe(struct platform_device *pdev)
>
>  err_free_state:
>  	v4l2_subdev_cleanup(&csi2rx->subdev);
> +	pm_runtime_disable(csi2rx->dev);
>  err_cleanup:
>  	v4l2_async_nf_unregister(&csi2rx->notifier);
>  	v4l2_async_nf_cleanup(&csi2rx->notifier);
> @@ -739,9 +713,73 @@ static void csi2rx_remove(struct platform_device *pdev)
>  	v4l2_async_unregister_subdev(&csi2rx->subdev);
>  	v4l2_subdev_cleanup(&csi2rx->subdev);
>  	media_entity_cleanup(&csi2rx->subdev.entity);
> +	pm_runtime_disable(csi2rx->dev);
>  	kfree(csi2rx);
>  }
>
> +static int csi2rx_runtime_suspend(struct device *dev)
> +{
> +	struct csi2rx_priv *csi2rx = dev_get_drvdata(dev);
> +	unsigned int i;
> +
> +	reset_control_assert(csi2rx->sys_rst);
> +	clk_disable_unprepare(csi2rx->sys_clk);
> +
> +	for (i = 0; i < csi2rx->max_streams; i++) {
> +		reset_control_assert(csi2rx->pixel_rst[i]);
> +		clk_disable_unprepare(csi2rx->pixel_clk[i]);
> +	}
> +
> +	reset_control_assert(csi2rx->p_rst);
> +	clk_disable_unprepare(csi2rx->p_clk);
> +
> +	return 0;
> +}
> +
> +static int csi2rx_runtime_resume(struct device *dev)
> +{
> +	struct csi2rx_priv *csi2rx = dev_get_drvdata(dev);
> +	unsigned int i;
> +	int ret;
> +
> +	ret = clk_prepare_enable(csi2rx->p_clk);
> +	if (ret)
> +		return ret;
> +
> +	reset_control_deassert(csi2rx->p_rst);
> +
> +	for (i = 0; i < csi2rx->max_streams; i++) {
> +		ret = clk_prepare_enable(csi2rx->pixel_clk[i]);
> +		if (ret)
> +			goto err_disable_pixclk;
> +
> +		reset_control_deassert(csi2rx->pixel_rst[i]);
> +	}
> +
> +	ret = clk_prepare_enable(csi2rx->sys_clk);
> +	if (ret)
> +		goto err_disable_pixclk;
> +
> +	reset_control_deassert(csi2rx->sys_rst);
> +
> +	return ret;

You can return 0 here

Thanks
   j

> +
> +err_disable_pixclk:
> +	for (; i > 0; i--) {
> +		reset_control_assert(csi2rx->pixel_rst[i - 1]);
> +		clk_disable_unprepare(csi2rx->pixel_clk[i - 1]);
> +	}
> +
> +	reset_control_assert(csi2rx->p_rst);
> +	clk_disable_unprepare(csi2rx->p_clk);
> +
> +	return ret;
> +}
> +
> +static const struct dev_pm_ops csi2rx_pm_ops = {
> +	SET_RUNTIME_PM_OPS(csi2rx_runtime_suspend, csi2rx_runtime_resume, NULL)
> +};
> +
>  static const struct of_device_id csi2rx_of_table[] = {
>  	{ .compatible = "starfive,jh7110-csi2rx" },
>  	{ .compatible = "cdns,csi2rx" },
> @@ -756,6 +794,7 @@ static struct platform_driver csi2rx_driver = {
>  	.driver	= {
>  		.name		= "cdns-csi2rx",
>  		.of_match_table	= csi2rx_of_table,
> +		.pm		= &csi2rx_pm_ops,
>  	},
>  };
>  module_platform_driver(csi2rx_driver);
> --
> 2.25.1
>
>
  

Hi Changhuang,

kernel test robot noticed the following build warnings:

[auto build test WARNING on media-tree/master]
[also build test WARNING on linuxtv-media-stage/master staging/staging-testing staging/staging-next staging/staging-linus linus/master v6.10 next-20240718]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Changhuang-Liang/media-cadence-csi2rx-Support-runtime-PM/20240718-131216
base:   git://linuxtv.org/media_tree.git master
patch link:    https://lore.kernel.org/r/20240718032834.53876-2-changhuang.liang%40starfivetech.com
patch subject: [PATCH v2 1/5] media: cadence: csi2rx: Support runtime PM
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20240718/202407182235.kxDoVX8T-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240718/202407182235.kxDoVX8T-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407182235.kxDoVX8T-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/media/platform/cadence/cdns-csi2rx.c:739:12: warning: 'csi2rx_runtime_resume' defined but not used [-Wunused-function]
     739 | static int csi2rx_runtime_resume(struct device *dev)
         |            ^~~~~~~~~~~~~~~~~~~~~
>> drivers/media/platform/cadence/cdns-csi2rx.c:720:12: warning: 'csi2rx_runtime_suspend' defined but not used [-Wunused-function]
     720 | static int csi2rx_runtime_suspend(struct device *dev)
         |            ^~~~~~~~~~~~~~~~~~~~~~


vim +/csi2rx_runtime_resume +739 drivers/media/platform/cadence/cdns-csi2rx.c

   719	
 > 720	static int csi2rx_runtime_suspend(struct device *dev)
   721	{
   722		struct csi2rx_priv *csi2rx = dev_get_drvdata(dev);
   723		unsigned int i;
   724	
   725		reset_control_assert(csi2rx->sys_rst);
   726		clk_disable_unprepare(csi2rx->sys_clk);
   727	
   728		for (i = 0; i < csi2rx->max_streams; i++) {
   729			reset_control_assert(csi2rx->pixel_rst[i]);
   730			clk_disable_unprepare(csi2rx->pixel_clk[i]);
   731		}
   732	
   733		reset_control_assert(csi2rx->p_rst);
   734		clk_disable_unprepare(csi2rx->p_clk);
   735	
   736		return 0;
   737	}
   738	
 > 739	static int csi2rx_runtime_resume(struct device *dev)
   740	{
   741		struct csi2rx_priv *csi2rx = dev_get_drvdata(dev);
   742		unsigned int i;
   743		int ret;
   744	
   745		ret = clk_prepare_enable(csi2rx->p_clk);
   746		if (ret)
   747			return ret;
   748	
   749		reset_control_deassert(csi2rx->p_rst);
   750	
   751		for (i = 0; i < csi2rx->max_streams; i++) {
   752			ret = clk_prepare_enable(csi2rx->pixel_clk[i]);
   753			if (ret)
   754				goto err_disable_pixclk;
   755	
   756			reset_control_deassert(csi2rx->pixel_rst[i]);
   757		}
   758	
   759		ret = clk_prepare_enable(csi2rx->sys_clk);
   760		if (ret)
   761			goto err_disable_pixclk;
   762	
   763		reset_control_deassert(csi2rx->sys_rst);
   764	
   765		return ret;
   766	
   767	err_disable_pixclk:
   768		for (; i > 0; i--) {
   769			reset_control_assert(csi2rx->pixel_rst[i - 1]);
   770			clk_disable_unprepare(csi2rx->pixel_clk[i - 1]);
   771		}
   772	
   773		reset_control_assert(csi2rx->p_rst);
   774		clk_disable_unprepare(csi2rx->p_clk);
   775	
   776		return ret;
   777	}
   778