Re: [PATCH v12 05/15] drm/panfrost: Consolidate device clock management and reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] clk: slow clk_prepare on runtime resume due to unprepare in suspend
--

commit 9d5414aa4af3f7dd6f34ed2bea83b5c9b25db769
Author: Adrián Larumbe <adrian.larumbe@collabora.com>
Subject: drm/panfrost: Consolidate device clock management and reset

This commit gathers clock enables and disables into a single function to
avoid repetition between driver init/fini and device resume/suspend. It
also combines prepare/unprepare with enable/disable, under the assumption
that clock preparation can simply increase the reference count of an already
prepared clock.

> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index b3a53504bd016..9f2b1967a3984 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
[ ... ]
> @@ -436,34 +450,17 @@ static int panfrost_device_runtime_resume(struct device *dev)
>  		if (ret)
>  			return ret;
>  
> -		ret = clk_enable(pfdev->clock);
> -		if (ret)
> -			goto err_clk;
> -
> -		ret = clk_enable(pfdev->bus_clock);
> -		if (ret)
> -			goto err_bus_clk;
> -
> -		ret = clk_enable(pfdev->bus_ace_clock);
> -		if (ret)
> -			goto err_bus_ace_clk;
> +		ret = panfrost_clks_enable(pfdev, true);
> +		if (ret) {
> +			reset_control_assert(pfdev->rstc);
> +			return ret;
> +		}
>  	}
>  
>  	panfrost_device_reset(pfdev, true);
[ ... ]
> @@ -480,9 +477,7 @@ static int panfrost_device_runtime_suspend(struct device *dev)
>  	panfrost_gpu_power_off(pfdev);
>  
>  	if (pfdev->comp->pm_features & BIT(GPU_PM_RT)) {
> -		clk_disable(pfdev->bus_ace_clock);
> -		clk_disable(pfdev->bus_clock);
> -		clk_disable(pfdev->clock);
> +		panfrost_clks_disable(pfdev, true);
>  		reset_control_assert(pfdev->rstc);
>  	}

[Severity: Medium]
Does this introduce a performance regression by fully unpreparing the clocks
on every idle state?

Previously, this code only called clk_disable() during runtime suspend,
leaving the clock prepared. Now panfrost_clks_disable() uses
clk_disable_unprepare(), reducing the prepare refcount to 0.

When the device wakes up, panfrost_device_runtime_resume() calls
panfrost_clks_enable() which uses clk_prepare_enable(). Because the clock
was unprepared here in suspend, the resume path will pay the penalty of
executing the full sleep-capable prepare sequence from a cold state instead
of just a fast atomic-safe enable.

The commit message notes that "the clk_prepare_* family of functions can
simply increase the refcnt of an already prepared clock". However, since the
clocks are fully unprepared in this runtime suspend path, they won't be
already prepared on resume.

Was it intended to fully unprepare the clocks on every runtime suspend?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929-claude-fixes-v12-0-62beb08de207@collabora.com?part=5