[PATCH] remoteproc: mediatek: Unprepare SCP clock during system suspend

[PATCH] remoteproc: mediatek: Unprepare SCP clock during system suspend

[Tzung-Bi Shih]

Prior to commit d935187cfb27 ("remoteproc: mediatek: Break lock
dependency to prepare_lock"), `scp->clk` was prepared and enabled only
when it needs to communicate with the SCP.  The commit d935187cfb27
moved the prepare operation to remoteproc's prepare(), keeping the clock
prepared as long as the SCP is running.

The power consumption due to the prolonged clock preparation can be
negligible when the system is running, as SCP is designed to be a very
power efficient processor.

However, the clock remains prepared even when the system enters system
suspend.  This prevents the underlying clock controller (and potentially
the parent PLLs) from shutting down, which increases power consumption
and may block the system from entering deep sleep states.

Add suspend and resume callbacks.  Unprepare the clock in suspend() if
it was active and re-prepare it in resume() to ensure the clock is
properly disabled during system suspend, while maintaining the "always
prepared" semantics while the system is active.

Fixes: d935187cfb27 ("remoteproc: mediatek: Break lock dependency to prepare_lock")
Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
---
 drivers/remoteproc/mtk_scp.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
index 4651311aeb07..6200c687e232 100644
--- a/drivers/remoteproc/mtk_scp.c
+++ b/drivers/remoteproc/mtk_scp.c
@@ -1592,12 +1592,40 @@ static const struct of_device_id mtk_scp_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, mtk_scp_of_match);
 
+#ifdef CONFIG_PM_SLEEP
+static int scp_suspend(struct device *dev)
+{
+	struct mtk_scp *scp = dev_get_drvdata(dev);
+	struct rproc *rproc = scp->rproc;
+
+	/* Only unprepare if the SCP is running and holding the clock */
+	if (rproc->state == RPROC_RUNNING)
+		clk_unprepare(scp->clk);
+	return 0;
+}
+
+static int scp_resume(struct device *dev)
+{
+	struct mtk_scp *scp = dev_get_drvdata(dev);
+	struct rproc *rproc = scp->rproc;
+
+	if (rproc->state == RPROC_RUNNING)
+		return clk_prepare(scp->clk);
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops scp_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(scp_suspend, scp_resume)
+};
+
 static struct platform_driver mtk_scp_driver = {
 	.probe = scp_probe,
 	.remove = scp_remove,
 	.driver = {
 		.name = "mtk-scp",
 		.of_match_table = mtk_scp_of_match,
+		.pm = &scp_pm_ops,
 	},
 };
 
-- 
2.53.0.rc2.204.g2597b5adb4-goog

Re: [PATCH] remoteproc: mediatek: Unprepare SCP clock during system suspend

[AngeloGioacchino Del Regno]

Il 04/02/26 09:54, Tzung-Bi Shih ha scritto:
> Prior to commit d935187cfb27 ("remoteproc: mediatek: Break lock
> dependency to prepare_lock"), `scp->clk` was prepared and enabled only
> when it needs to communicate with the SCP.  The commit d935187cfb27
> moved the prepare operation to remoteproc's prepare(), keeping the clock
> prepared as long as the SCP is running.
> 
> The power consumption due to the prolonged clock preparation can be
> negligible when the system is running, as SCP is designed to be a very
> power efficient processor.
> 
> However, the clock remains prepared even when the system enters system
> suspend.  This prevents the underlying clock controller (and potentially
> the parent PLLs) from shutting down, which increases power consumption
> and may block the system from entering deep sleep states.
> 
> Add suspend and resume callbacks.  Unprepare the clock in suspend() if
> it was active and re-prepare it in resume() to ensure the clock is
> properly disabled during system suspend, while maintaining the "always
> prepared" semantics while the system is active.
> 
> Fixes: d935187cfb27 ("remoteproc: mediatek: Break lock dependency to prepare_lock")
> Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>

Would be great if you could mention that in this driver, at the moment of writing,
there is no .attach() callback, hence rproc->state can never be RPROC_ATTACHED (as
in case that would also leave the clock prepared).

In any case, even though I don't exactly *love* how this is actually done, I don't
really have any better option.

But....

> ---
>   drivers/remoteproc/mtk_scp.c | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/remoteproc/mtk_scp.c b/drivers/remoteproc/mtk_scp.c
> index 4651311aeb07..6200c687e232 100644
> --- a/drivers/remoteproc/mtk_scp.c
> +++ b/drivers/remoteproc/mtk_scp.c
> @@ -1592,12 +1592,40 @@ static const struct of_device_id mtk_scp_of_match[] = {
>   };
>   MODULE_DEVICE_TABLE(of, mtk_scp_of_match);
>   
> +#ifdef CONFIG_PM_SLEEP

...you don't need this ifdef as when CONFIG_PM_SLEEP is not set the macro
SET_SYSTEM_SLEEP_PM_OPS is stubbed out (check pm.h).

After removing the ifdef, there'll be something to change, specifically:

> +static int scp_suspend(struct device *dev)

static int __maybe_unused scp_suspend(struct device *dev)
{

> +{
> +	struct mtk_scp *scp = dev_get_drvdata(dev);
> +	struct rproc *rproc = scp->rproc;
> +
> +	/* Only unprepare if the SCP is running and holding the clock */
> +	if (rproc->state == RPROC_RUNNING)
> +		clk_unprepare(scp->clk);
> +	return 0;
> +}
> +
> +static int scp_resume(struct device *dev)

static int __maybe_unused scp_resume(struct device *dev)
{

After which,

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Cheers,
Angelo

> +{
> +	struct mtk_scp *scp = dev_get_drvdata(dev);
> +	struct rproc *rproc = scp->rproc;
> +
> +	if (rproc->state == RPROC_RUNNING)
> +		return clk_prepare(scp->clk);
> +	return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops scp_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(scp_suspend, scp_resume)
> +};
> +
>   static struct platform_driver mtk_scp_driver = {
>   	.probe = scp_probe,
>   	.remove = scp_remove,
>   	.driver = {
>   		.name = "mtk-scp",
>   		.of_match_table = mtk_scp_of_match,
> +		.pm = &scp_pm_ops,
>   	},
>   };
>   

Re: [PATCH] remoteproc: mediatek: Unprepare SCP clock during system suspend

[Tzung-Bi Shih]

On Thu, Feb 05, 2026 at 01:30:42PM +0100, AngeloGioacchino Del Regno wrote:
> Il 04/02/26 09:54, Tzung-Bi Shih ha scritto:
> > Prior to commit d935187cfb27 ("remoteproc: mediatek: Break lock
> > dependency to prepare_lock"), `scp->clk` was prepared and enabled only
> > when it needs to communicate with the SCP.  The commit d935187cfb27
> > moved the prepare operation to remoteproc's prepare(), keeping the clock
> > prepared as long as the SCP is running.
> > 
> > The power consumption due to the prolonged clock preparation can be
> > negligible when the system is running, as SCP is designed to be a very
> > power efficient processor.
> > 
> > However, the clock remains prepared even when the system enters system
> > suspend.  This prevents the underlying clock controller (and potentially
> > the parent PLLs) from shutting down, which increases power consumption
> > and may block the system from entering deep sleep states.
> > 
> > Add suspend and resume callbacks.  Unprepare the clock in suspend() if
> > it was active and re-prepare it in resume() to ensure the clock is
> > properly disabled during system suspend, while maintaining the "always
> > prepared" semantics while the system is active.
> > 
> > Fixes: d935187cfb27 ("remoteproc: mediatek: Break lock dependency to prepare_lock")
> > Signed-off-by: Tzung-Bi Shih <tzungbi@kernel.org>
> 
> Would be great if you could mention that in this driver, at the moment of writing,
> there is no .attach() callback, hence rproc->state can never be RPROC_ATTACHED (as
> in case that would also leave the clock prepared).

Thanks for your review.  Add it in the commit message and code comment in
v2[1].

[1] https://lore.kernel.org/all/20260206033034.3031781-1-tzungbi@kernel.org/

> > +#ifdef CONFIG_PM_SLEEP
> 
> ...you don't need this ifdef as when CONFIG_PM_SLEEP is not set the macro
> SET_SYSTEM_SLEEP_PM_OPS is stubbed out (check pm.h).
> 
> After removing the ifdef, there'll be something to change, specifically:
> 
> > +static int scp_suspend(struct device *dev)
> 
> static int __maybe_unused scp_suspend(struct device *dev)
> {
> 
> > +{
> > +	struct mtk_scp *scp = dev_get_drvdata(dev);
> > +	struct rproc *rproc = scp->rproc;
> > +
> > +	/* Only unprepare if the SCP is running and holding the clock */
> > +	if (rproc->state == RPROC_RUNNING)
> > +		clk_unprepare(scp->clk);
> > +	return 0;
> > +}
> > +
> > +static int scp_resume(struct device *dev)
> 
> static int __maybe_unused scp_resume(struct device *dev)
> {

Fix them in v2 too.