[PATCH 0/2] drm/imagination: add reset handling

[PATCH 0/2] drm/imagination: add reset handling

[Parthiban Nallathambi]

reset control is included in platforms from Allwinner, specifically
A133, A733. Add reset control from devicetree and these changes are
tested with A133 based helper board, which will be upstremed after
Allwinner power domain controller.

Firmware is used from [1].

[1]: https://gitlab.freedesktop.org/imagination/linux-firmware/-/issues/5

Signed-off-by: Parthiban Nallathambi <parthiban@linumiz.com>
---
Parthiban Nallathambi (2):
      dt-bindings: gpu: add reset control property
      drm/imagination: add reset control support

 .../devicetree/bindings/gpu/img,powervr-rogue.yaml       |  3 +++
 drivers/gpu/drm/imagination/pvr_device.h                 |  8 ++++++++
 drivers/gpu/drm/imagination/pvr_drv.c                    |  5 +++++
 drivers/gpu/drm/imagination/pvr_power.c                  | 16 +++++++++++++++-
 4 files changed, 31 insertions(+), 1 deletion(-)
---
base-commit: adc218676eef25575469234709c2d87185ca223a
change-id: 20241125-pvr-reset-0a27fbae5ebc

Best regards,
-- 
Parthiban Nallathambi <parthiban@linumiz.com>

[PATCH 1/2] dt-bindings: gpu: add reset control property

[Parthiban Nallathambi]

GE8300 in Allwinner A133 have reset control from the ccu.
Add the resets property as optional one to control it.

Signed-off-by: Parthiban Nallathambi <parthiban@linumiz.com>
---
 Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
index 256e252f8087..bb607d4b1e07 100644
--- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
+++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
@@ -37,6 +37,9 @@ properties:
   power-domains:
     maxItems: 1
 
+  resets:
+    maxItems: 1
+
 required:
   - compatible
   - reg

-- 
2.39.2

Re: [PATCH 1/2] dt-bindings: gpu: add reset control property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1000 bytes --]

On Mon, Nov 25, 2024 at 10:07:03PM +0530, Parthiban Nallathambi wrote:
> GE8300 in Allwinner A133 have reset control from the ccu.
> Add the resets property as optional one to control it.

There's no specific compatible here for an a133, but the binding
requires one. Where is your dts patch?

> 
> Signed-off-by: Parthiban Nallathambi <parthiban@linumiz.com>
> ---
>  Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> index 256e252f8087..bb607d4b1e07 100644
> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
> @@ -37,6 +37,9 @@ properties:
>    power-domains:
>      maxItems: 1
>  
> +  resets:
> +    maxItems: 1
> +
>  required:
>    - compatible
>    - reg
> 
> -- 
> 2.39.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/2] dt-bindings: gpu: add reset control property

[Parthiban]

On 11/25/24 11:37 PM, Conor Dooley wrote:
> On Mon, Nov 25, 2024 at 10:07:03PM +0530, Parthiban Nallathambi wrote:
>> GE8300 in Allwinner A133 have reset control from the ccu.
>> Add the resets property as optional one to control it.
> 
> There's no specific compatible here for an a133, but the binding
> requires one. Where is your dts patch?
A133 GPU is still work in progress in both Kernel and Mesa3D. Also power
domain support needs an additional driver.

But reset control is independent of those changes. Should reset control
needs to be clubbed GPU dts changes?

Thanks,
Parthiban
> 
>>
>> Signed-off-by: Parthiban Nallathambi <parthiban@linumiz.com>
>> ---
>>  Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>> index 256e252f8087..bb607d4b1e07 100644
>> --- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>> +++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
>> @@ -37,6 +37,9 @@ properties:
>>    power-domains:
>>      maxItems: 1
>>  
>> +  resets:
>> +    maxItems: 1
>> +
>>  required:
>>    - compatible
>>    - reg
>>
>> -- 
>> 2.39.2
>>

Re: [PATCH 1/2] dt-bindings: gpu: add reset control property

[Krzysztof Kozlowski]

On 26/11/2024 04:46, Parthiban wrote:
> On 11/25/24 11:37 PM, Conor Dooley wrote:
>> On Mon, Nov 25, 2024 at 10:07:03PM +0530, Parthiban Nallathambi wrote:
>>> GE8300 in Allwinner A133 have reset control from the ccu.
>>> Add the resets property as optional one to control it.
>>
>> There's no specific compatible here for an a133, but the binding
>> requires one. Where is your dts patch?
> A133 GPU is still work in progress in both Kernel and Mesa3D. Also power
> domain support needs an additional driver.
> 
> But reset control is independent of those changes. Should reset control
> needs to be clubbed GPU dts changes?
How is it independent? Are you adding it for the new platforms? If yes,
then it is part of new platforms. Don't add properties which are not used.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: gpu: add reset control property

[Parthiban]

On 11/26/24 3:02 PM, Krzysztof Kozlowski wrote:
> On 26/11/2024 04:46, Parthiban wrote:
>> On 11/25/24 11:37 PM, Conor Dooley wrote:
>>> On Mon, Nov 25, 2024 at 10:07:03PM +0530, Parthiban Nallathambi wrote:
>>>> GE8300 in Allwinner A133 have reset control from the ccu.
>>>> Add the resets property as optional one to control it.
>>>
>>> There's no specific compatible here for an a133, but the binding
>>> requires one. Where is your dts patch?
>> A133 GPU is still work in progress in both Kernel and Mesa3D. Also power
>> domain support needs an additional driver.
>>
>> But reset control is independent of those changes. Should reset control
>> needs to be clubbed GPU dts changes?
> How is it independent? Are you adding it for the new platforms? If yes,
> then it is part of new platforms. Don't add properties which are not used.
Thanks for the review. Will address the points together when adding support
for GE8300 GPU.

Thanks,
Parthiban

> 
> Best regards,
> Krzysztof

Re: [PATCH 1/2] dt-bindings: gpu: add reset control property

[Krzysztof Kozlowski]

On 25/11/2024 17:37, Parthiban Nallathambi wrote:
> GE8300 in Allwinner A133 have reset control from the ccu.
> Add the resets property as optional one to control it.
> 
> Signed-off-by: Parthiban Nallathambi <parthiban@linumiz.com>
> ---

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

Missing prefix for the device. You are not adding reset to all GPU devices.

Best regards,
Krzysztof

[PATCH 2/2] drm/imagination: add reset control support

[Parthiban Nallathambi]

On some platforms like Allwinner A133 with GE8300 includes
reset control from reset control unit. Add reset control
optionally from the devicetree.

Signed-off-by: Parthiban Nallathambi <parthiban@linumiz.com>
---
 drivers/gpu/drm/imagination/pvr_device.h |  8 ++++++++
 drivers/gpu/drm/imagination/pvr_drv.c    |  5 +++++
 drivers/gpu/drm/imagination/pvr_power.c  | 16 +++++++++++++++-
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/imagination/pvr_device.h b/drivers/gpu/drm/imagination/pvr_device.h
index 6d0dfacb677b..21ec7dd64415 100644
--- a/drivers/gpu/drm/imagination/pvr_device.h
+++ b/drivers/gpu/drm/imagination/pvr_device.h
@@ -23,6 +23,7 @@
 #include <linux/kernel.h>
 #include <linux/math.h>
 #include <linux/mutex.h>
+#include <linux/reset.h>
 #include <linux/spinlock_types.h>
 #include <linux/timer.h>
 #include <linux/types.h>
@@ -131,6 +132,13 @@ struct pvr_device {
 	 */
 	struct clk *mem_clk;
 
+	/**
+	 * @reset: Optional reset control
+	 *
+	 * This may be used on some platforms to reset the GPU module/IP.
+	 */
+	struct reset_control *reset;
+
 	/** @irq: IRQ number. */
 	int irq;
 
diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
index fb17196e05f4..d9b918410ea9 100644
--- a/drivers/gpu/drm/imagination/pvr_drv.c
+++ b/drivers/gpu/drm/imagination/pvr_drv.c
@@ -36,6 +36,7 @@
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset.h>
 #include <linux/xarray.h>
 
 /**
@@ -1427,6 +1428,10 @@ pvr_probe(struct platform_device *plat_dev)
 	pm_runtime_use_autosuspend(&plat_dev->dev);
 	pvr_watchdog_init(pvr_dev);
 
+	pvr_dev->reset = devm_reset_control_get_optional_exclusive(&plat_dev->dev, "ahb");
+	if (PTR_ERR(pvr_dev->reset) == -EPROBE_DEFER)
+		return PTR_ERR(pvr_dev->reset);
+
 	err = pvr_device_init(pvr_dev);
 	if (err)
 		goto err_watchdog_fini;
diff --git a/drivers/gpu/drm/imagination/pvr_power.c b/drivers/gpu/drm/imagination/pvr_power.c
index ba7816fd28ec..a24ed85f36c7 100644
--- a/drivers/gpu/drm/imagination/pvr_power.c
+++ b/drivers/gpu/drm/imagination/pvr_power.c
@@ -15,6 +15,7 @@
 #include <linux/mutex.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/reset.h>
 #include <linux/timer.h>
 #include <linux/types.h>
 #include <linux/workqueue.h>
@@ -252,6 +253,9 @@ pvr_power_device_suspend(struct device *dev)
 	clk_disable_unprepare(pvr_dev->sys_clk);
 	clk_disable_unprepare(pvr_dev->core_clk);
 
+	if (!IS_ERR(pvr_dev->reset))
+		reset_control_assert(pvr_dev->reset);
+
 err_drm_dev_exit:
 	drm_dev_exit(idx);
 
@@ -270,9 +274,15 @@ pvr_power_device_resume(struct device *dev)
 	if (!drm_dev_enter(drm_dev, &idx))
 		return -EIO;
 
+	if (!IS_ERR(pvr_dev->reset)) {
+		err = reset_control_reset(pvr_dev->reset);
+		if (err)
+			goto err_drm_dev_exit;
+	}
+
 	err = clk_prepare_enable(pvr_dev->core_clk);
 	if (err)
-		goto err_drm_dev_exit;
+		goto err_reset_exit;
 
 	err = clk_prepare_enable(pvr_dev->sys_clk);
 	if (err)
@@ -301,6 +311,10 @@ pvr_power_device_resume(struct device *dev)
 err_core_clk_disable:
 	clk_disable_unprepare(pvr_dev->core_clk);
 
+err_reset_exit:
+	if (!IS_ERR(pvr_dev->reset))
+		reset_control_assert(pvr_dev->reset);
+
 err_drm_dev_exit:
 	drm_dev_exit(idx);
 

-- 
2.39.2

Re: [PATCH 2/2] drm/imagination: add reset control support

[Philipp Zabel]

On Mo, 2024-11-25 at 22:07 +0530, Parthiban Nallathambi wrote:
> On some platforms like Allwinner A133 with GE8300 includes
> reset control from reset control unit. Add reset control
> optionally from the devicetree.
> 
> Signed-off-by: Parthiban Nallathambi <parthiban@linumiz.com>
> ---
>  drivers/gpu/drm/imagination/pvr_device.h |  8 ++++++++
>  drivers/gpu/drm/imagination/pvr_drv.c    |  5 +++++
>  drivers/gpu/drm/imagination/pvr_power.c  | 16 +++++++++++++++-
>  3 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_device.h b/drivers/gpu/drm/imagination/pvr_device.h
> index 6d0dfacb677b..21ec7dd64415 100644
> --- a/drivers/gpu/drm/imagination/pvr_device.h
> +++ b/drivers/gpu/drm/imagination/pvr_device.h
> @@ -23,6 +23,7 @@
>  #include <linux/kernel.h>
>  #include <linux/math.h>
>  #include <linux/mutex.h>
> +#include <linux/reset.h>
>  #include <linux/spinlock_types.h>
>  #include <linux/timer.h>
>  #include <linux/types.h>
> @@ -131,6 +132,13 @@ struct pvr_device {
>  	 */
>  	struct clk *mem_clk;
>  
> +	/**
> +	 * @reset: Optional reset control
> +	 *
> +	 * This may be used on some platforms to reset the GPU module/IP.
> +	 */
> +	struct reset_control *reset;
> +
>  	/** @irq: IRQ number. */
>  	int irq;
>  
> diff --git a/drivers/gpu/drm/imagination/pvr_drv.c b/drivers/gpu/drm/imagination/pvr_drv.c
> index fb17196e05f4..d9b918410ea9 100644
> --- a/drivers/gpu/drm/imagination/pvr_drv.c
> +++ b/drivers/gpu/drm/imagination/pvr_drv.c
> @@ -36,6 +36,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/reset.h>
>  #include <linux/xarray.h>
>  
>  /**
> @@ -1427,6 +1428,10 @@ pvr_probe(struct platform_device *plat_dev)
>  	pm_runtime_use_autosuspend(&plat_dev->dev);
>  	pvr_watchdog_init(pvr_dev);
>  
> +	pvr_dev->reset = devm_reset_control_get_optional_exclusive(&plat_dev->dev, "ahb");
> +	if (PTR_ERR(pvr_dev->reset) == -EPROBE_DEFER)
> +		return PTR_ERR(pvr_dev->reset);

	pvr_dev->reset = devm_reset_control_get_optional_exclusive(&plat_dev->dev, "ahb");
	if (IS_ERR(pvr_dev->reset))
		return PTR_ERR(pvr_dev->reset);

Please don't ignore errors. devm_reset_control_get_optional_exclusive()
returns NULL if the "ahb" reset control isn't specified in the device
tree, and the reset_control_assert/deassert() functions accept that as
a non-existing reset, see [1].

[1] https://docs.kernel.org/driver-api/reset.html#optional-resets

> +
>  	err = pvr_device_init(pvr_dev);
>  	if (err)
>  		goto err_watchdog_fini;
> diff --git a/drivers/gpu/drm/imagination/pvr_power.c b/drivers/gpu/drm/imagination/pvr_power.c
> index ba7816fd28ec..a24ed85f36c7 100644
> --- a/drivers/gpu/drm/imagination/pvr_power.c
> +++ b/drivers/gpu/drm/imagination/pvr_power.c
> @@ -15,6 +15,7 @@
>  #include <linux/mutex.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/reset.h>
>  #include <linux/timer.h>
>  #include <linux/types.h>
>  #include <linux/workqueue.h>
> @@ -252,6 +253,9 @@ pvr_power_device_suspend(struct device *dev)
>  	clk_disable_unprepare(pvr_dev->sys_clk);
>  	clk_disable_unprepare(pvr_dev->core_clk);
>  
> +	if (!IS_ERR(pvr_dev->reset))
> +		reset_control_assert(pvr_dev->reset);

	reset_control_assert(pvr_dev->reset);

This just returns 0 if pvr_dev->reset == NULL.

> +
>  err_drm_dev_exit:
>  	drm_dev_exit(idx);
>  
> @@ -270,9 +274,15 @@ pvr_power_device_resume(struct device *dev)
>  	if (!drm_dev_enter(drm_dev, &idx))
>  		return -EIO;
>  
> +	if (!IS_ERR(pvr_dev->reset)) {
> +		err = reset_control_reset(pvr_dev->reset);
> +		if (err)
> +			goto err_drm_dev_exit;
> +	}

	err = reset_control_reset(pvr_dev->reset);
	if (err)
		goto err_drm_dev_exit;

> +
>  	err = clk_prepare_enable(pvr_dev->core_clk);
>  	if (err)
> -		goto err_drm_dev_exit;
> +		goto err_reset_exit;
>  
>  	err = clk_prepare_enable(pvr_dev->sys_clk);
>  	if (err)
> @@ -301,6 +311,10 @@ pvr_power_device_resume(struct device *dev)
>  err_core_clk_disable:
>  	clk_disable_unprepare(pvr_dev->core_clk);
>  
> +err_reset_exit:
> +	if (!IS_ERR(pvr_dev->reset))
> +		reset_control_assert(pvr_dev->reset);

err_reset_exit:
	reset_control_assert(pvr_dev->reset);

> +
>  err_drm_dev_exit:
>  	drm_dev_exit(idx);
>  
> 

regards
Philipp

Re: [PATCH 2/2] drm/imagination: add reset control support

[Parthiban]

On 11/26/24 2:59 PM, Philipp Zabel wrote:
> On Mo, 2024-11-25 at 22:07 +0530, Parthiban Nallathambi wrote:
>> On some platforms like Allwinner A133 with GE8300 includes
>> reset control from reset control unit. Add reset control
>> optionally from the devicetree.
>>
>> Signed-off-by: Parthiban Nallathambi <parthiban@linumiz.com>
>> ---
>>  drivers/gpu/drm/imagination/pvr_device.h |  8 ++++++++
>>  drivers/gpu/drm/imagination/pvr_drv.c    |  5 +++++
>>  drivers/gpu/drm/imagination/pvr_power.c  | 16 +++++++++++++++-
>>  3 files changed, 28 insertions(+), 1 deletion(-)
Thanks for the reviews. As with the binding patch comments, reset control
needs to be added together with the consumer i.e in this case GE8300 GPU
in Allwinner A133 SoC.

I will address the comments when sending patch with support for GE8300.

Thanks,
Parthiban