| Message ID | 20240718032834.53876-2-changhuang.liang@starfivetech.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add StarFive Camera Subsystem hibernation support | expand |
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
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; + +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);
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(-)