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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <edf101e3-c638-45fa-8f5d-48247b9e0c9d@gmail.com>
Date: Mon, 31 Mar 2025 12:49:11 +0200
From: Philippe Simons <simons.philippe@...il.com>
To: Steven Price <steven.price@....com>,
 Andre Przywara <andre.przywara@....com>,
 Boris Brezillon <boris.brezillon@...labora.com>,
 Rob Herring <robh@...nel.org>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Philipp Zabel <p.zabel@...gutronix.de>, dri-devel@...ts.freedesktop.org,
 linux-kernel@...r.kernel.org, linux-sunxi@...ts.linux.dev,
 Jernej Škrabec <jernej.skrabec@...il.com>
Subject: Re: [PATCH 1/2] drm/panfrost: Add PM runtime flags


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@...il.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 ?

>
> Reviewed-by: Steven Price <steven.price@....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@...il.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 {