Re: [PATCH 1/2] drm/panfrost: Add PM runtime flags

[Steven Price <steven.price@arm.com>]

On 31/03/2025 11:49, Philippe Simons wrote:
> 
> On 3/31/25 12:32, Steven Price wrote:
>> On 27/03/2025 12:36, Andre Przywara wrote:
>>> On Thu, 13 Mar 2025 00:23:18 +0100
>>> Philippe Simons <simons.philippe@gmail.com> wrote:
>>>
>>> Hi Rob, Boris, Steven,
>>>
>>>> When the GPU is the only device attached to a single power domain,
>>>> core genpd disable and enable it when gpu enter and leave runtime
>>>> suspend.
>>>>
>>>> Some power-domain requires a sequence before disabled,
>>>> and the reverse when enabled.
>>>>
>>>> Add PM flags for CLK and RST, and implement in
>>>> panfrost_device_runtime_suspend/resume.
>>> So some Mali configuration and integration manual I am looking at says
>>> that this sequence should be always observed, as the powerdown sequence
>>> would include disabling the clocks first, then asserting the reset, then
>>> turning the power switches off (and the inverse sequence on powerup).
>>>
>>> So should we make this unconditional, not depending on implementation
>>> specific flags?
>> I think you're right, this probably should be unconditional. My only
>> reservation is that "it works" currently and we'd need to test this
>> doesn't cause regressions on existing platforms. So unless someone with
>> a reasonable board farm is able to do that testing I think this solution
>> is reasonable. So:
> 
> Should I merge both flags together then ? something like GPU_PM_RT ?

Yes, that would probably be a good idea and might simplify things a little.

Thanks,
Steve

>>
>> Reviewed-by: Steven Price <steven.price@arm.com>
>>
>>> And also I am wondering if panfrost_device_init() gets this wrong as
>>> well?
>>> As I see it enabling clock first, then reset, then pm_domain, where it
>>> should be exactly the opposite?
>> I agree, that looks very wrong - the power needs to be enabled before
>> reset is deasserted. I'm somewhat surprised we've got away with that.
>> Fancy writing a patch? ;)
>>
>> Steve
>>
>>> Cheers,
>>> Andre
>>>
>>>> Signed-off-by: Philippe Simons <simons.philippe@gmail.com>
>>>> ---
>>>>   drivers/gpu/drm/panfrost/panfrost_device.c | 37 ++++++++++++++++++
>>>> ++++
>>>>   drivers/gpu/drm/panfrost/panfrost_device.h |  4 +++
>>>>   2 files changed, 41 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/
>>>> gpu/drm/panfrost/panfrost_device.c
>>>> index a45e4addcc19..189ad2ad2b32 100644
>>>> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
>>>> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
>>>> @@ -406,11 +406,38 @@ void panfrost_device_reset(struct
>>>> panfrost_device *pfdev)
>>>>   static int panfrost_device_runtime_resume(struct device *dev)
>>>>   {
>>>>       struct panfrost_device *pfdev = dev_get_drvdata(dev);
>>>> +    int ret;
>>>> +
>>>> +    if (pfdev->comp->pm_features & BIT(GPU_PM_RT_RST_ASRT)) {
>>>> +        ret = reset_control_deassert(pfdev->rstc);
>>>> +        if (ret)
>>>> +            return ret;
>>>> +    }
>>>> +
>>>> +    if (pfdev->comp->pm_features & BIT(GPU_PM_RT_CLK_DIS)) {
>>>> +        ret = clk_enable(pfdev->clock);
>>>> +        if (ret)
>>>> +            goto err_clk;
>>>> +
>>>> +        if (pfdev->bus_clock) {
>>>> +            ret = clk_enable(pfdev->bus_clock);
>>>> +            if (ret)
>>>> +                goto err_bus_clk;
>>>> +        }
>>>> +    }
>>>>         panfrost_device_reset(pfdev);
>>>>       panfrost_devfreq_resume(pfdev);
>>>>         return 0;
>>>> +
>>>> +err_bus_clk:
>>>> +    if (pfdev->comp->pm_features & BIT(GPU_PM_RT_CLK_DIS))
>>>> +        clk_disable(pfdev->clock);
>>>> +err_clk:
>>>> +    if (pfdev->comp->pm_features & BIT(GPU_PM_RT_RST_ASRT))
>>>> +        reset_control_assert(pfdev->rstc);
>>>> +    return ret;
>>>>   }
>>>>     static int panfrost_device_runtime_suspend(struct device *dev)
>>>> @@ -426,6 +453,16 @@ static int
>>>> panfrost_device_runtime_suspend(struct device *dev)
>>>>       panfrost_gpu_suspend_irq(pfdev);
>>>>       panfrost_gpu_power_off(pfdev);
>>>>   +    if (pfdev->comp->pm_features & BIT(GPU_PM_RT_CLK_DIS)) {
>>>> +        if (pfdev->bus_clock)
>>>> +            clk_disable(pfdev->bus_clock);
>>>> +
>>>> +        clk_disable(pfdev->clock);
>>>> +    }
>>>> +
>>>> +    if (pfdev->comp->pm_features & BIT(GPU_PM_RT_RST_ASRT))
>>>> +        reset_control_assert(pfdev->rstc);
>>>> +
>>>>       return 0;
>>>>   }
>>>>   diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/
>>>> gpu/drm/panfrost/panfrost_device.h
>>>> index cffcb0ac7c11..f372d4819262 100644
>>>> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
>>>> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
>>>> @@ -36,10 +36,14 @@ enum panfrost_drv_comp_bits {
>>>>    * enum panfrost_gpu_pm - Supported kernel power management features
>>>>    * @GPU_PM_CLK_DIS:  Allow disabling clocks during system suspend
>>>>    * @GPU_PM_VREG_OFF: Allow turning off regulators during system
>>>> suspend
>>>> + * @GPU_PM_RT_CLK_DIS: Allow disabling clocks during system runtime
>>>> suspend
>>>> + * @GPU_PM_RST_ASRT: Allow asserting the reset control during
>>>> runtime suspend
>>>>    */
>>>>   enum panfrost_gpu_pm {
>>>>       GPU_PM_CLK_DIS,
>>>>       GPU_PM_VREG_OFF,
>>>> +    GPU_PM_RT_CLK_DIS,
>>>> +    GPU_PM_RT_RST_ASRT
>>>>   };
>>>>     struct panfrost_features {