[PATCH 0/2] drm/panfrost:

[PATCH 0/2] drm/panfrost:

[Philippe Simons]

Allwinner H616 has a dedicated power domain for its Mali G31.

Currently after probe, the GPU is put in runtime suspend which 
disable the power domain.
On first usage of GPU, the power domain enable hangs the system.

This series adds the necessary calls to enable the clocks and
deasserting the reset line after the power domain enabling and
asserting the reset line and disabling the clocks prior to the
power domain disabling.

This allows to use the Mali GPU on all Allwinner H616
boards and devices.

Philippe Simons (2):
  drm/panfrost: Add PM runtime flags
  drm/panfrost: add h616 compatible string

 drivers/gpu/drm/panfrost/panfrost_device.c | 37 ++++++++++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_device.h |  4 +++
 drivers/gpu/drm/panfrost/panfrost_drv.c    |  8 +++++
 3 files changed, 49 insertions(+)


base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
prerequisite-patch-id: eb8a11e2b24bb282970d8b8528834dea7ee392cc
-- 
2.48.1

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

[Philippe Simons]

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.

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 {
-- 
2.48.1

[PATCH 2/2] drm/panfrost: add h616 compatible string

[Philippe Simons]

Tie the Allwinner compatible string to the two features bits that will
toggle the clocks and the reset line whenever the power domain is changing
state.

Signed-off-by: Philippe Simons <simons.philippe@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 0f3935556ac7..f13743fe6bad 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -776,6 +776,13 @@ static const struct panfrost_compatible default_data = {
 	.pm_domain_names = NULL,
 };
 
+static const struct panfrost_compatible allwinner_h616_data = {
+	.num_supplies = ARRAY_SIZE(default_supplies) - 1,
+	.supply_names = default_supplies,
+	.num_pm_domains = 1,
+	.pm_features = BIT(GPU_PM_RT_CLK_DIS) | BIT(GPU_PM_RT_RST_ASRT),
+};
+
 static const struct panfrost_compatible amlogic_data = {
 	.num_supplies = ARRAY_SIZE(default_supplies) - 1,
 	.supply_names = default_supplies,
@@ -859,6 +866,7 @@ static const struct of_device_id dt_match[] = {
 	{ .compatible = "mediatek,mt8186-mali", .data = &mediatek_mt8186_data },
 	{ .compatible = "mediatek,mt8188-mali", .data = &mediatek_mt8188_data },
 	{ .compatible = "mediatek,mt8192-mali", .data = &mediatek_mt8192_data },
+	{ .compatible = "allwinner,sun50i-h616-mali", .data = &allwinner_h616_data },
 	{}
 };
 MODULE_DEVICE_TABLE(of, dt_match);
-- 
2.48.1

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

[Andre Przywara]

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?

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?

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 {

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

[Steven Price]

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:

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 {
> 

Re: [PATCH 2/2] drm/panfrost: add h616 compatible string

[Steven Price]

On 12/03/2025 23:23, Philippe Simons wrote:
> Tie the Allwinner compatible string to the two features bits that will
> toggle the clocks and the reset line whenever the power domain is changing
> state.

This looks fine, but we need the new compatible string to be documented
in the bindings:

Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml

I'm not sure what the situation is for the device tree for this
platform, but it would be good to get that all sorted before we merge
the compatible into panfrost.

Thanks,
Steve

> Signed-off-by: Philippe Simons <simons.philippe@gmail.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 0f3935556ac7..f13743fe6bad 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -776,6 +776,13 @@ static const struct panfrost_compatible default_data = {
>  	.pm_domain_names = NULL,
>  };
>  
> +static const struct panfrost_compatible allwinner_h616_data = {
> +	.num_supplies = ARRAY_SIZE(default_supplies) - 1,
> +	.supply_names = default_supplies,
> +	.num_pm_domains = 1,
> +	.pm_features = BIT(GPU_PM_RT_CLK_DIS) | BIT(GPU_PM_RT_RST_ASRT),
> +};
> +
>  static const struct panfrost_compatible amlogic_data = {
>  	.num_supplies = ARRAY_SIZE(default_supplies) - 1,
>  	.supply_names = default_supplies,
> @@ -859,6 +866,7 @@ static const struct of_device_id dt_match[] = {
>  	{ .compatible = "mediatek,mt8186-mali", .data = &mediatek_mt8186_data },
>  	{ .compatible = "mediatek,mt8188-mali", .data = &mediatek_mt8188_data },
>  	{ .compatible = "mediatek,mt8192-mali", .data = &mediatek_mt8192_data },
> +	{ .compatible = "allwinner,sun50i-h616-mali", .data = &allwinner_h616_data },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, dt_match);

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

[Philippe Simons]

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 ?

>
> 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 {

Re: [PATCH 2/2] drm/panfrost: add h616 compatible string

[Andre Przywara]

On Mon, 31 Mar 2025 11:32:58 +0100
Steven Price <steven.price@arm.com> wrote:

Hi Steven,

thanks for having a look!

> On 12/03/2025 23:23, Philippe Simons wrote:
> > Tie the Allwinner compatible string to the two features bits that will
> > toggle the clocks and the reset line whenever the power domain is changing
> > state.  
> 
> This looks fine, but we need the new compatible string to be documented
> in the bindings:
> 
> Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> 
> I'm not sure what the situation is for the device tree for this
> platform, but it would be good to get that all sorted before we merge
> the compatible into panfrost.

The binding addition was sent earlier, as part of my power-domain driver
series:
https://lore.kernel.org/linux-sunxi/20250221005802.11001-1-andre.przywara@arm.com/T/#m083df99cf34ddfd06a6a4b8fbb49636a51b05112

Rob took that one already, and it landed in Linus' tree last week, so
that would be covered.

Cheers,
Andre

> 
> Thanks,
> Steve
> 
> > Signed-off-by: Philippe Simons <simons.philippe@gmail.com>
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_drv.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > index 0f3935556ac7..f13743fe6bad 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > @@ -776,6 +776,13 @@ static const struct panfrost_compatible default_data = {
> >  	.pm_domain_names = NULL,
> >  };
> >  
> > +static const struct panfrost_compatible allwinner_h616_data = {
> > +	.num_supplies = ARRAY_SIZE(default_supplies) - 1,
> > +	.supply_names = default_supplies,
> > +	.num_pm_domains = 1,
> > +	.pm_features = BIT(GPU_PM_RT_CLK_DIS) | BIT(GPU_PM_RT_RST_ASRT),
> > +};
> > +
> >  static const struct panfrost_compatible amlogic_data = {
> >  	.num_supplies = ARRAY_SIZE(default_supplies) - 1,
> >  	.supply_names = default_supplies,
> > @@ -859,6 +866,7 @@ static const struct of_device_id dt_match[] = {
> >  	{ .compatible = "mediatek,mt8186-mali", .data = &mediatek_mt8186_data },
> >  	{ .compatible = "mediatek,mt8188-mali", .data = &mediatek_mt8188_data },
> >  	{ .compatible = "mediatek,mt8192-mali", .data = &mediatek_mt8192_data },
> > +	{ .compatible = "allwinner,sun50i-h616-mali", .data = &allwinner_h616_data },
> >  	{}
> >  };
> >  MODULE_DEVICE_TABLE(of, dt_match);  
> 

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

[Steven Price]

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 {

Re: [PATCH 2/2] drm/panfrost: add h616 compatible string

[Steven Price]

On 31/03/2025 11:57, Andre Przywara wrote:
> On Mon, 31 Mar 2025 11:32:58 +0100
> Steven Price <steven.price@arm.com> wrote:
> 
> Hi Steven,
> 
> thanks for having a look!
> 
>> On 12/03/2025 23:23, Philippe Simons wrote:
>>> Tie the Allwinner compatible string to the two features bits that will
>>> toggle the clocks and the reset line whenever the power domain is changing
>>> state.  
>>
>> This looks fine, but we need the new compatible string to be documented
>> in the bindings:
>>
>> Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
>>
>> I'm not sure what the situation is for the device tree for this
>> platform, but it would be good to get that all sorted before we merge
>> the compatible into panfrost.
> 
> The binding addition was sent earlier, as part of my power-domain driver
> series:
> https://lore.kernel.org/linux-sunxi/20250221005802.11001-1-andre.przywara@arm.com/T/#m083df99cf34ddfd06a6a4b8fbb49636a51b05112
> 
> Rob took that one already, and it landed in Linus' tree last week, so
> that would be covered.

Ah, cool. I realised now I was looking in drm-misc-next not linux-next
(too many Linux checkouts!) which is why I didn't find it.

Thanks,
Steve

> Cheers,
> Andre
> 
>>
>> Thanks,
>> Steve
>>
>>> Signed-off-by: Philippe Simons <simons.philippe@gmail.com>
>>> ---
>>>  drivers/gpu/drm/panfrost/panfrost_drv.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
>>> index 0f3935556ac7..f13743fe6bad 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
>>> @@ -776,6 +776,13 @@ static const struct panfrost_compatible default_data = {
>>>  	.pm_domain_names = NULL,
>>>  };
>>>  
>>> +static const struct panfrost_compatible allwinner_h616_data = {
>>> +	.num_supplies = ARRAY_SIZE(default_supplies) - 1,
>>> +	.supply_names = default_supplies,
>>> +	.num_pm_domains = 1,
>>> +	.pm_features = BIT(GPU_PM_RT_CLK_DIS) | BIT(GPU_PM_RT_RST_ASRT),
>>> +};
>>> +
>>>  static const struct panfrost_compatible amlogic_data = {
>>>  	.num_supplies = ARRAY_SIZE(default_supplies) - 1,
>>>  	.supply_names = default_supplies,
>>> @@ -859,6 +866,7 @@ static const struct of_device_id dt_match[] = {
>>>  	{ .compatible = "mediatek,mt8186-mali", .data = &mediatek_mt8186_data },
>>>  	{ .compatible = "mediatek,mt8188-mali", .data = &mediatek_mt8188_data },
>>>  	{ .compatible = "mediatek,mt8192-mali", .data = &mediatek_mt8192_data },
>>> +	{ .compatible = "allwinner,sun50i-h616-mali", .data = &allwinner_h616_data },
>>>  	{}
>>>  };
>>>  MODULE_DEVICE_TABLE(of, dt_match);  
>>
> 

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

[Andre Przywara]

On Mon, 31 Mar 2025 11:32:58 +0100
Steven Price <steven.price@arm.com> 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:
> 
> 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? ;)

Seems like Philippe volunteered ;-) (on IRC). Actually we tried this
already some weeks ago, but this alone didn't help. I think it's that this
power domain in panfrost_device_init() doesn't trigger for some reason,
but only in suspend()/resume(), so he came up with this patch here.

Thanks!
Andre

> 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 {  
> >   
>