Re: [PATCH] drm/panfrost: Unprepare clocks on system suspend

["Adrián Larumbe" <adrian.larumbe@collabora.com>]

Hi Michał,

On 03.10.2026 21:48, Michał Kopeć wrote:
> With GPU_PM_CLK_DIS, system suspend only disables the GPU clocks, which
> leaves them prepared. Some providers power a clock down only in
> unprepare: MediaTek PLLs are switched off in mtk_pll_unprepare(). On
> MT8188 this keeps MFGPLL running through suspend, and TF-A refuses the
> 26 MHz-off state because of it:
> 
>   suspend: bus26m total blocked = 0x40040000
> 
> The SoC then only reaches a shallower sleep state, never signals sleep
> to the EC, and the EC reports a sleep hang and wakes the system.
> 
> Use clk_disable_unprepare() on suspend and clk_prepare_enable() on
> resume. Both run in process context, so preparing is allowed.
> 
> Fixes: 56e76c017918 ("drm/panfrost: Implement ability to turn on/off GPU clocks in suspend")
> Signed-off-by: Michał Kopeć <michal@nozomi.space>
> ---
>  drivers/gpu/drm/panfrost/panfrost_device.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 485349faf2511..3f1a8109a79a4 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -507,12 +507,12 @@ static int panfrost_device_resume(struct device *dev)
>  	}
>  
>  	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS)) {
> -		ret = clk_enable(pfdev->clock);
> +		ret = clk_prepare_enable(pfdev->clock);

My understanding of the clock API is that clk_{prepare|unprepare} are often most costly than
disabling/enabling clocks, so forcing an 'unprepare' at system suspension time for SoCs that
don't have this requirement sounds wasteful. I would say for SoCs where this is a necessity,
add a new feature to 'enum panfrost_gpu_pm' like GPU_PM_CLK_UNP and OR it into its relevant
struct panfrost_compatible. Then, do clk_{prepare|unprepare} in SYSTEM_SLEEP_PM_OPS only
when the feature is set, leaving clk_{enable|disable} as is.

On top of that, would you mind rebasing this onto the latest drm-misc-next? There's been quite
a few panfrost changes wrt to RPM and clock management, so the patch no longer applies.

Cheers,
Adrian

>  		if (ret)
>  			goto err_clk;
>  
>  		if (pfdev->bus_clock) {
> -			ret = clk_enable(pfdev->bus_clock);
> +			ret = clk_prepare_enable(pfdev->bus_clock);
>  			if (ret)
>  				goto err_bus_clk;
>  		}
> @@ -526,10 +526,10 @@ static int panfrost_device_resume(struct device *dev)
>  
>  err_resume:
>  	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS) && pfdev->bus_clock)
> -		clk_disable(pfdev->bus_clock);
> +		clk_disable_unprepare(pfdev->bus_clock);
>  err_bus_clk:
>  	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS))
> -		clk_disable(pfdev->clock);
> +		clk_disable_unprepare(pfdev->clock);
>  err_clk:
>  	if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF))
>  		dev_pm_opp_set_opp(dev, NULL);
> @@ -545,11 +545,15 @@ static int panfrost_device_suspend(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * Unprepare as well: some clock providers, e.g. MediaTek PLLs, only
> +	 * power down in unprepare, and a running GPU PLL blocks system sleep.
> +	 */
>  	if (pfdev->comp->pm_features & BIT(GPU_PM_CLK_DIS)) {
>  		if (pfdev->bus_clock)
> -			clk_disable(pfdev->bus_clock);
> +			clk_disable_unprepare(pfdev->bus_clock);
>  
> -		clk_disable(pfdev->clock);
> +		clk_disable_unprepare(pfdev->clock);
>  	}
>  
>  	if (pfdev->comp->pm_features & BIT(GPU_PM_VREG_OFF))
> 
> base-commit: 70456f05d4b6396b22048c4b8cd3cb98ecf9f9e3
> -- 
> 2.55.0