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

[Tzung-Bi Shih <tzungbi@kernel.org>]

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.