| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <rpbe5ebzu7e536qrruseffklmtvfkp5bbenjtx7enipm6y5gbr@wsjvomihwmv6>
Date: Thu, 18 Jul 2024 16:24:37 +0200
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: Changhuang Liang <changhuang.liang@...rfivetech.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
Maxime Ripard <mripard@...nel.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Hans Verkuil <hverkuil-cisco@...all.nl>, Jacopo Mondi <jacopo.mondi@...asonboard.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>, Tomi Valkeinen <tomi.valkeinen+renesas@...asonboard.com>,
Jack Zhu <jack.zhu@...rfivetech.com>, Keith Zhao <keith.zhao@...rfivetech.com>,
Jayshri Pawar <jpawar@...ence.com>, Jai Luthra <j-luthra@...com>, linux-media@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-staging@...ts.linux.dev
Subject: Re: [PATCH v2 1/5] media: cadence: csi2rx: Support runtime PM
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@...rfivetech.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
>
>
Powered by blists - more mailing lists