Re: [PATCH v3 3/8] drm/imagination: Use pwrseq for TH1520 GPU power management

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

On 6/3/25 15:28, Krzysztof Kozlowski wrote:
> On Fri, May 30, 2025 at 12:23:50AM GMT, Michal Wilczynski wrote:
>> Update the Imagination PVR DRM driver to leverage the pwrseq framework
>> for managing the power sequence of the GPU on the T-HEAD TH1520 SoC.
>>
>> In pvr_device_init(), the driver now attempts to get a handle to the
>> "gpu-power" sequencer target using devm_pwrseq_get(). If successful,
>> the responsibility for powering on and off the GPU's core clocks and
>> resets is delegated to the power sequencer. Consequently, the GPU
>> driver conditionally skips acquiring the GPU reset line if the pwrseq
>> handle is obtained, as the sequencer's match function will acquire it.
>> Clock handles are still acquired by the GPU driver for other purposes
>> like devfreq.
>>
>> The runtime PM callbacks, pvr_power_device_resume() and
>> pvr_power_device_suspend(), are modified to call pwrseq_power_on() and
>> pwrseq_power_off() respectively when the sequencer is present.  If no
>> sequencer is found, the driver falls back to its existing manual clock
>> and reset management. A helper function,
>> pvr_power_off_sequence_manual(), is introduced to encapsulate the manual
>> power-down logic.
>>
>> Signed-off-by: Michal Wilczynski <m.wilczynski@samsung.com>
>> ---
>>  drivers/gpu/drm/imagination/Kconfig      |  1 +
>>  drivers/gpu/drm/imagination/pvr_device.c | 33 +++++++++++--
>>  drivers/gpu/drm/imagination/pvr_device.h |  6 +++
>>  drivers/gpu/drm/imagination/pvr_power.c  | 82 +++++++++++++++++++++-----------
>>  4 files changed, 89 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/imagination/Kconfig b/drivers/gpu/drm/imagination/Kconfig
>> index 3bfa2ac212dccb73c53bdc2bc259bcba636e7cfc..737ace77c4f1247c687cc1fde2f139fc2e118c50 100644
>> --- a/drivers/gpu/drm/imagination/Kconfig
>> +++ b/drivers/gpu/drm/imagination/Kconfig
>> @@ -11,6 +11,7 @@ config DRM_POWERVR
>>  	select DRM_SCHED
>>  	select DRM_GPUVM
>>  	select FW_LOADER
>> +  select POWER_SEQUENCING
> 
> Messed indent.
> 
>>  	help
>>  	  Choose this option if you have a system that has an Imagination
>>  	  Technologies PowerVR (Series 6 or later) or IMG GPU.
>> diff --git a/drivers/gpu/drm/imagination/pvr_device.c b/drivers/gpu/drm/imagination/pvr_device.c
>> index 8b9ba4983c4cb5bc40342fcafc4259078bc70547..19d48bbc828cf2b8dbead602e90ff88780152124 100644
>> --- a/drivers/gpu/drm/imagination/pvr_device.c
>> +++ b/drivers/gpu/drm/imagination/pvr_device.c
>> @@ -25,6 +25,7 @@
>>  #include <linux/interrupt.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/pm_runtime.h>
>> +#include <linux/pwrseq/consumer.h>
>>  #include <linux/reset.h>
>>  #include <linux/slab.h>
>>  #include <linux/stddef.h>
>> @@ -631,10 +632,34 @@ pvr_device_init(struct pvr_device *pvr_dev)
>>  	if (err)
>>  		return err;
>>  
>> -	/* Get the reset line for the GPU */
>> -	err = pvr_device_reset_init(pvr_dev);
>> -	if (err)
>> -		return err;
>> +	/*
>> +	 * Try to get a power sequencer. If successful, it will handle clocks
>> +	 * and resets. Otherwise, we fall back to managing them ourselves.
>> +	 */
>> +	pvr_dev->pwrseq = devm_pwrseq_get(dev, "gpu-power");
>> +	if (IS_ERR(pvr_dev->pwrseq)) {
>> +		int pwrseq_err = PTR_ERR(pvr_dev->pwrseq);
>> +
>> +		/*
>> +		 * If the error is -EPROBE_DEFER, it's because the
>> +		 * optional sequencer provider is not present
>> +		 * and it's safe to fall back on manual power-up.
> 
> It is safe but why it is desirable? The rule is rather to defer the
> probe, assuming this is probe path.

Yeah this is probe path.

The GPU node will depend on the AON node, which will be the sole
provider for the 'gpu-power' sequencer (based on the discussion in patch
1).

Therefore, if the AON/pwrseq driver has already completed its probe, and
devm_pwrseq_get() in the GPU driver subsequently returns -EPROBE_DEFER
(because pwrseq_get found 'no match' on the bus for 'gpu-power'), the
interpretation is that the AON driver did not register this optional
sequencer. Since AON is the only anticipated source, it implies the
sequencer won't become available later from its designated provider.
This specific scenario is when we intend to fall back to manual power
management, which pvr driver is capable of, it just doesn't work for the
TH1520 SoC, as it has a custom requirements implemented by the pwrseq
driver.

> 
> Best regards,
> Krzysztof
> 
> 

Best regards,
-- 
Michal Wilczynski <m.wilczynski@samsung.com>