Re: [PATCH v2 4/4] drm/imagination: Skip clocks if platform PM manages resources

[Michal Wilczynski <m.wilczynski@samsung.com>]

On 4/15/25 11:15, Matt Coster wrote:
> On 15/04/2025 09:55, Maxime Ripard wrote:
>> On Mon, Apr 14, 2025 at 08:52:58PM +0200, Michal Wilczynski wrote:
>>> Update the Imagination PVR driver to skip clock management during
>>> initialization if the platform PM has indicated that it manages platform
>>> resources.
>>>
>>> This is necessary for platforms like the T-HEAD TH1520, where the GPU's
>>> clocks and resets are managed via a PM domain, and should not be
>>> manipulated directly by the GPU driver.
>>>
>>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>>> ---
>>>  drivers/gpu/drm/imagination/pvr_device.c | 14 ++++++++++----
>>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
>>> index 1704c0268589bdeb65fa6535f9ec63182b0a3e94..f40468b99cf14da418aeecde086f009695ff877c 100644
>>> --- a/drivers/gpu/drm/imagination/pvr_device.c
>>> +++ b/drivers/gpu/drm/imagination/pvr_device.c
>>> @@ -504,10 +504,16 @@ pvr_device_init(struct pvr_device *pvr_dev)
>>>  	if (err)
>>>  		return err;
>>>  
>>> -	/* Enable and initialize clocks required for the device to operate. */
>>> -	err = pvr_device_clk_init(pvr_dev);
>>> -	if (err)
>>> -		return err;
>>> +	/*
>>> +	 * Only initialize clocks if they are not managed by the platform's
>>> +	 * PM domain.
>>> +	 */
>>> +	if (!device_platform_resources_pm_managed(dev)) {
>>> +		/* Enable and initialize clocks required for the device to operate. */
>>> +		err = pvr_device_clk_init(pvr_dev);
>>> +		if (err)
>>> +			return err;
>>> +	}
>>
>> So, how does that work for devfreq? I can understand the rationale for
>> resets and the sys clock, but the core clock at least should really be
>> handled by the driver.

Hi Maxime, Matt,

Thanks for the feedback.

This commit is trying to prevent the pvr RUNTIME_PM_OPS from controlling the
clocks or resets, as there is a custom start/stop sequence needed for
the TH1520 SoC coded in patch 3 of this series.

static const struct dev_pm_ops pvr_pm_ops = {
	RUNTIME_PM_OPS(pvr_power_device_suspend, pvr_power_device_resume, pvr_power_device_idle)
};

So, if the core clock needs to be used for other purposes like devfreq,
we could move the device_platform_resources_pm_managed() check to the
pvr_power_* functions instead. This would prevent the clocks and resets
from being managed in runtime PM in the consumer driver, while still
allowing the GPU driver to access and control clocks like the core clock
as needed for other purposes.

That way, clocks could be safely shared between the PM domain driver and the
device driver, with generic PM driver controlling the start/stop
sequence for reset and clocks. We would probably need to find a
better name for the flag then, to more clearly reflect that it's about
delegating clock/reset PM runtime control, rather than full resource
ownership.

> 
> I agree, this feels a bit "all or nothing" to me. There's only one clock
> on this platform that has issues, we can still control the other two
> just fine.
> 
> I thought fixed clocks were the standard mechanism for exposing
> non-controllable clocks to device drivers?

That's correct — and it's not really about the MEM clock at this point.
The main goal is to ensure the custom power-up sequence for the TH1520
SoC is followed. That sequence is implemented in
th1520_gpu_domain_start() in patch 3 of this series.

Regards,
Michał

> 
> Cheers,
> Matt
> 
>>
>> Maxime
> 
>