[PATCH v1 00/10] drm/panthor: Add support for Mali-G1 GPUs

[PATCH v1 00/10] drm/panthor: Add support for Mali-G1 GPUs

[Karunika Choo]

This patch series extends the Panthor driver with basic support for
Mali-G1 GPUs.

The v14 architecture introduces several hardware and register-level
changes compared to prior GPUs. This series adds the necessary
architecture-specific support infrastructure, power control and reset
handling for Mali-G1 GPUs.

Patch Breakdown:
[Patch 1-4]:  Refactor panthor_hw to introduce architecture-specific
              hooks and abstractions to support the v14 architecture.
              These patches introduces architecture-specific HW binding
              for feature bits and function pointers.
[Patch 5-7]:  Adds basic L2 power on/off and soft reset support for the
              PWR_CONTROL block introduced in v14.
[Patch 8]:    Update MCU halt and warm boot operations to reflect the
              GLB_REQ.STATE changes in v14. This ensures that the MCU is
              properly halted and the correct operations are performed
              on warm boot depending on the FW version.
[Patch 9]:    Align endpoint_req with changes introduced in v14, where
              the register is widened to 64-bit and shifed down by
              4-bytes. This patch adds the necessary infrastructure to
              discern the correct endpoint_req register to use.
[Patch 10]:   Enables Mali-G1 support on Panthor by adding HW bindings
              for v14 architecture.

Kind regards,
Karunika Choo

Karunika Choo (10):
  drm/panthor: Factor out GPU_ID register read into separate function
  drm/panthor: Add arch-specific panthor_hw binding
  drm/panthor: Introduce framework for architecture-specific features
  drm/panthor: Add architecture-specific function operations
  drm/panthor: Introduce panthor_pwr API and power control framework
  drm/panthor: Implement L2 power on/off via PWR_CONTROL
  drm/panthor: Implement soft and fast reset via PWR_CONTROL
  drm/panthor: Support GLB_REQ.STATE field for Mali-G1 GPUs
  drm/panthor: Support 64-bit endpoint_req register for Mali-G1
  drm/panthor: Add support for Mali-G1 GPUs

 drivers/gpu/drm/panthor/Makefile         |   1 +
 drivers/gpu/drm/panthor/panthor_device.c |  18 +-
 drivers/gpu/drm/panthor/panthor_device.h |   8 +
 drivers/gpu/drm/panthor/panthor_fw.c     | 123 ++++-
 drivers/gpu/drm/panthor/panthor_fw.h     |  27 +-
 drivers/gpu/drm/panthor/panthor_gpu.c    |  13 +-
 drivers/gpu/drm/panthor/panthor_gpu.h    |   1 +
 drivers/gpu/drm/panthor/panthor_hw.c     | 107 ++++-
 drivers/gpu/drm/panthor/panthor_hw.h     |  50 ++
 drivers/gpu/drm/panthor/panthor_pwr.c    | 581 +++++++++++++++++++++++
 drivers/gpu/drm/panthor/panthor_pwr.h    |  25 +
 drivers/gpu/drm/panthor/panthor_regs.h   |  79 +++
 drivers/gpu/drm/panthor/panthor_sched.c  |  22 +-
 13 files changed, 1019 insertions(+), 36 deletions(-)
 create mode 100644 drivers/gpu/drm/panthor/panthor_pwr.c
 create mode 100644 drivers/gpu/drm/panthor/panthor_pwr.h

-- 
2.49.0

[PATCH v1 01/10] drm/panthor: Factor out GPU_ID register read into separate function

[Karunika Choo]

Split the GPU_ID register read into its own helper function. The GPU_ID
value will be used to enable architecture-specific behaviours, which may
also affect how other registers (such as those used for gpu_info) are
read.

This change separates the read operation so that subsequent code can
depend on the intermediate result of processing the GPU_ID.

Signed-off-by: Karunika Choo <karunika.choo@arm.com>
---
 drivers/gpu/drm/panthor/panthor_hw.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panthor/panthor_hw.c b/drivers/gpu/drm/panthor/panthor_hw.c
index 4f2858114e5e..326a9db0b5c2 100644
--- a/drivers/gpu/drm/panthor/panthor_hw.c
+++ b/drivers/gpu/drm/panthor/panthor_hw.c
@@ -62,7 +62,6 @@ static void panthor_gpu_info_init(struct panthor_device *ptdev)
 {
 	unsigned int i;
 
-	ptdev->gpu_info.gpu_id = gpu_read(ptdev, GPU_ID);
 	ptdev->gpu_info.csf_id = gpu_read(ptdev, GPU_CSF_ID);
 	ptdev->gpu_info.gpu_rev = gpu_read(ptdev, GPU_REVID);
 	ptdev->gpu_info.core_features = gpu_read(ptdev, GPU_CORE_FEATURES);
@@ -117,8 +116,23 @@ static void panthor_hw_info_init(struct panthor_device *ptdev)
 		 ptdev->gpu_info.tiler_present);
 }
 
+static int panthor_hw_gpu_id_init(struct panthor_device *ptdev)
+{
+	ptdev->gpu_info.gpu_id = gpu_read(ptdev, GPU_ID);
+	if (!ptdev->gpu_info.gpu_id)
+		return -ENXIO;
+
+	return 0;
+}
+
 int panthor_hw_init(struct panthor_device *ptdev)
 {
+	int ret = 0;
+
+	ret = panthor_hw_gpu_id_init(ptdev);
+	if (ret)
+		return ret;
+
 	panthor_hw_info_init(ptdev);
 
 	return 0;
-- 
2.49.0

Re: [PATCH v1 01/10] drm/panthor: Factor out GPU_ID register read into separate function

[Steven Price]

On 14/10/2025 10:43, Karunika Choo wrote:
> Split the GPU_ID register read into its own helper function. The GPU_ID
> value will be used to enable architecture-specific behaviours, which may
> also affect how other registers (such as those used for gpu_info) are
> read.
> 
> This change separates the read operation so that subsequent code can
> depend on the intermediate result of processing the GPU_ID.
> 
> Signed-off-by: Karunika Choo <karunika.choo@arm.com>

I'd be very tempted to squash this into the next commit and simple read
the GPU ID at the beginning of panthor_hw_bind_device(). Is there any
reason not to?

Steve

> ---
>  drivers/gpu/drm/panthor/panthor_hw.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_hw.c b/drivers/gpu/drm/panthor/panthor_hw.c
> index 4f2858114e5e..326a9db0b5c2 100644
> --- a/drivers/gpu/drm/panthor/panthor_hw.c
> +++ b/drivers/gpu/drm/panthor/panthor_hw.c
> @@ -62,7 +62,6 @@ static void panthor_gpu_info_init(struct panthor_device *ptdev)
>  {
>  	unsigned int i;
>  
> -	ptdev->gpu_info.gpu_id = gpu_read(ptdev, GPU_ID);
>  	ptdev->gpu_info.csf_id = gpu_read(ptdev, GPU_CSF_ID);
>  	ptdev->gpu_info.gpu_rev = gpu_read(ptdev, GPU_REVID);
>  	ptdev->gpu_info.core_features = gpu_read(ptdev, GPU_CORE_FEATURES);
> @@ -117,8 +116,23 @@ static void panthor_hw_info_init(struct panthor_device *ptdev)
>  		 ptdev->gpu_info.tiler_present);
>  }
>  
> +static int panthor_hw_gpu_id_init(struct panthor_device *ptdev)
> +{
> +	ptdev->gpu_info.gpu_id = gpu_read(ptdev, GPU_ID);
> +	if (!ptdev->gpu_info.gpu_id)
> +		return -ENXIO;
> +
> +	return 0;
> +}
> +
>  int panthor_hw_init(struct panthor_device *ptdev)
>  {
> +	int ret = 0;
> +
> +	ret = panthor_hw_gpu_id_init(ptdev);
> +	if (ret)
> +		return ret;
> +
>  	panthor_hw_info_init(ptdev);
>  
>  	return 0;

[PATCH v1 02/10] drm/panthor: Add arch-specific panthor_hw binding

[Karunika Choo]

This patch adds the framework for binding to a specific panthor_hw
structure based on the architecture major value parsed from the GPU_ID
register. This is in preparation of enabling architecture-specific
behaviours based on GPU_ID.

This framework allows a single panthor_hw structure to be shared across
multiple architectures should there be minimal changes between them via
the arch_min and arch_max field of the panthor_hw_entry structure,
instead of duplicating the structure across multiple architectures.

Signed-off-by: Karunika Choo <karunika.choo@arm.com>
---
 drivers/gpu/drm/panthor/panthor_device.h |  4 ++
 drivers/gpu/drm/panthor/panthor_hw.c     | 49 ++++++++++++++++++++++++
 drivers/gpu/drm/panthor/panthor_hw.h     |  6 +++
 3 files changed, 59 insertions(+)

diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
index a764111359d2..1457c1255f1f 100644
--- a/drivers/gpu/drm/panthor/panthor_device.h
+++ b/drivers/gpu/drm/panthor/panthor_device.h
@@ -26,6 +26,7 @@ struct panthor_device;
 struct panthor_gpu;
 struct panthor_group_pool;
 struct panthor_heap_pool;
+struct panthor_hw;
 struct panthor_job;
 struct panthor_mmu;
 struct panthor_fw;
@@ -122,6 +123,9 @@ struct panthor_device {
 	/** @csif_info: Command stream interface information. */
 	struct drm_panthor_csif_info csif_info;
 
+	/** @hw: GPU-specific data. */
+	struct panthor_hw *hw;
+
 	/** @gpu: GPU management data. */
 	struct panthor_gpu *gpu;
 
diff --git a/drivers/gpu/drm/panthor/panthor_hw.c b/drivers/gpu/drm/panthor/panthor_hw.c
index 326a9db0b5c2..b6e7401327c3 100644
--- a/drivers/gpu/drm/panthor/panthor_hw.c
+++ b/drivers/gpu/drm/panthor/panthor_hw.c
@@ -8,6 +8,28 @@
 #define GPU_PROD_ID_MAKE(arch_major, prod_major) \
 	(((arch_major) << 24) | (prod_major))
 
+/** struct panthor_hw_entry - HW arch major to panthor_hw binding entry */
+struct panthor_hw_entry {
+	/** @arch_min: Minimum supported architecture major value (inclusive) */
+	u8 arch_min;
+
+	/** @arch_max: Maximum supported architecture major value (inclusive) */
+	u8 arch_max;
+
+	/** @hwdev: Pointer to panthor_hw structure */
+	struct panthor_hw *hwdev;
+};
+
+static struct panthor_hw panthor_hw_arch_v10 = {};
+
+static struct panthor_hw_entry panthor_hw_match[] = {
+	{
+		.arch_min = 10,
+		.arch_max = 13,
+		.hwdev = &panthor_hw_arch_v10,
+	},
+};
+
 static char *get_gpu_model_name(struct panthor_device *ptdev)
 {
 	const u32 gpu_id = ptdev->gpu_info.gpu_id;
@@ -116,6 +138,29 @@ static void panthor_hw_info_init(struct panthor_device *ptdev)
 		 ptdev->gpu_info.tiler_present);
 }
 
+static int panthor_hw_bind_device(struct panthor_device *ptdev)
+{
+	struct panthor_hw *hdev = NULL;
+	const u32 arch_major = GPU_ARCH_MAJOR(ptdev->gpu_info.gpu_id);
+	int i = 0;
+
+	for (i = 0; i < ARRAY_SIZE(panthor_hw_match); i++) {
+		struct panthor_hw_entry *entry = &panthor_hw_match[i];
+
+		if (arch_major >= entry->arch_min && arch_major <= entry->arch_max) {
+			hdev = entry->hwdev;
+			break;
+		}
+	}
+
+	if (!hdev)
+		return -EOPNOTSUPP;
+
+	ptdev->hw = hdev;
+
+	return 0;
+}
+
 static int panthor_hw_gpu_id_init(struct panthor_device *ptdev)
 {
 	ptdev->gpu_info.gpu_id = gpu_read(ptdev, GPU_ID);
@@ -133,6 +178,10 @@ int panthor_hw_init(struct panthor_device *ptdev)
 	if (ret)
 		return ret;
 
+	ret = panthor_hw_bind_device(ptdev);
+	if (ret)
+		return ret;
+
 	panthor_hw_info_init(ptdev);
 
 	return 0;
diff --git a/drivers/gpu/drm/panthor/panthor_hw.h b/drivers/gpu/drm/panthor/panthor_hw.h
index 0af6acc6aa6a..39752de3e7ad 100644
--- a/drivers/gpu/drm/panthor/panthor_hw.h
+++ b/drivers/gpu/drm/panthor/panthor_hw.h
@@ -6,6 +6,12 @@
 
 struct panthor_device;
 
+/**
+ * struct panthor_hw - GPU specific register mapping and functions
+ */
+struct panthor_hw {
+};
+
 int panthor_hw_init(struct panthor_device *ptdev);
 
 #endif /* __PANTHOR_HW_H__ */
-- 
2.49.0

Re: [PATCH v1 02/10] drm/panthor: Add arch-specific panthor_hw binding

[Steven Price]

On 14/10/2025 10:43, Karunika Choo wrote:
> This patch adds the framework for binding to a specific panthor_hw
> structure based on the architecture major value parsed from the GPU_ID
> register. This is in preparation of enabling architecture-specific
> behaviours based on GPU_ID.
> 
> This framework allows a single panthor_hw structure to be shared across
> multiple architectures should there be minimal changes between them via
> the arch_min and arch_max field of the panthor_hw_entry structure,
> instead of duplicating the structure across multiple architectures.
> 
> Signed-off-by: Karunika Choo <karunika.choo@arm.com>

Looks fine (although see my comment in the previous patch about
potentially squashing into this one).

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panthor/panthor_device.h |  4 ++
>  drivers/gpu/drm/panthor/panthor_hw.c     | 49 ++++++++++++++++++++++++
>  drivers/gpu/drm/panthor/panthor_hw.h     |  6 +++
>  3 files changed, 59 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index a764111359d2..1457c1255f1f 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -26,6 +26,7 @@ struct panthor_device;
>  struct panthor_gpu;
>  struct panthor_group_pool;
>  struct panthor_heap_pool;
> +struct panthor_hw;
>  struct panthor_job;
>  struct panthor_mmu;
>  struct panthor_fw;
> @@ -122,6 +123,9 @@ struct panthor_device {
>  	/** @csif_info: Command stream interface information. */
>  	struct drm_panthor_csif_info csif_info;
>  
> +	/** @hw: GPU-specific data. */
> +	struct panthor_hw *hw;
> +
>  	/** @gpu: GPU management data. */
>  	struct panthor_gpu *gpu;
>  
> diff --git a/drivers/gpu/drm/panthor/panthor_hw.c b/drivers/gpu/drm/panthor/panthor_hw.c
> index 326a9db0b5c2..b6e7401327c3 100644
> --- a/drivers/gpu/drm/panthor/panthor_hw.c
> +++ b/drivers/gpu/drm/panthor/panthor_hw.c
> @@ -8,6 +8,28 @@
>  #define GPU_PROD_ID_MAKE(arch_major, prod_major) \
>  	(((arch_major) << 24) | (prod_major))
>  
> +/** struct panthor_hw_entry - HW arch major to panthor_hw binding entry */
> +struct panthor_hw_entry {
> +	/** @arch_min: Minimum supported architecture major value (inclusive) */
> +	u8 arch_min;
> +
> +	/** @arch_max: Maximum supported architecture major value (inclusive) */
> +	u8 arch_max;
> +
> +	/** @hwdev: Pointer to panthor_hw structure */
> +	struct panthor_hw *hwdev;
> +};
> +
> +static struct panthor_hw panthor_hw_arch_v10 = {};
> +
> +static struct panthor_hw_entry panthor_hw_match[] = {
> +	{
> +		.arch_min = 10,
> +		.arch_max = 13,
> +		.hwdev = &panthor_hw_arch_v10,
> +	},
> +};
> +
>  static char *get_gpu_model_name(struct panthor_device *ptdev)
>  {
>  	const u32 gpu_id = ptdev->gpu_info.gpu_id;
> @@ -116,6 +138,29 @@ static void panthor_hw_info_init(struct panthor_device *ptdev)
>  		 ptdev->gpu_info.tiler_present);
>  }
>  
> +static int panthor_hw_bind_device(struct panthor_device *ptdev)
> +{
> +	struct panthor_hw *hdev = NULL;
> +	const u32 arch_major = GPU_ARCH_MAJOR(ptdev->gpu_info.gpu_id);
> +	int i = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(panthor_hw_match); i++) {
> +		struct panthor_hw_entry *entry = &panthor_hw_match[i];
> +
> +		if (arch_major >= entry->arch_min && arch_major <= entry->arch_max) {
> +			hdev = entry->hwdev;
> +			break;
> +		}
> +	}
> +
> +	if (!hdev)
> +		return -EOPNOTSUPP;
> +
> +	ptdev->hw = hdev;
> +
> +	return 0;
> +}
> +
>  static int panthor_hw_gpu_id_init(struct panthor_device *ptdev)
>  {
>  	ptdev->gpu_info.gpu_id = gpu_read(ptdev, GPU_ID);
> @@ -133,6 +178,10 @@ int panthor_hw_init(struct panthor_device *ptdev)
>  	if (ret)
>  		return ret;
>  
> +	ret = panthor_hw_bind_device(ptdev);
> +	if (ret)
> +		return ret;
> +
>  	panthor_hw_info_init(ptdev);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/panthor/panthor_hw.h b/drivers/gpu/drm/panthor/panthor_hw.h
> index 0af6acc6aa6a..39752de3e7ad 100644
> --- a/drivers/gpu/drm/panthor/panthor_hw.h
> +++ b/drivers/gpu/drm/panthor/panthor_hw.h
> @@ -6,6 +6,12 @@
>  
>  struct panthor_device;
>  
> +/**
> + * struct panthor_hw - GPU specific register mapping and functions
> + */
> +struct panthor_hw {
> +};
> +
>  int panthor_hw_init(struct panthor_device *ptdev);
>  
>  #endif /* __PANTHOR_HW_H__ */

[PATCH v1 03/10] drm/panthor: Introduce framework for architecture-specific features

[Karunika Choo]

Add a framework to support architecture-specific features. This allows
other parts of the driver to adjust their behaviour based on the feature
bits enabled for a given architecture.

Signed-off-by: Karunika Choo <karunika.choo@arm.com>
---
 drivers/gpu/drm/panthor/panthor_hw.c |  5 +++++
 drivers/gpu/drm/panthor/panthor_hw.h | 18 ++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/panthor/panthor_hw.c b/drivers/gpu/drm/panthor/panthor_hw.c
index b6e7401327c3..34536526384d 100644
--- a/drivers/gpu/drm/panthor/panthor_hw.c
+++ b/drivers/gpu/drm/panthor/panthor_hw.c
@@ -186,3 +186,8 @@ int panthor_hw_init(struct panthor_device *ptdev)
 
 	return 0;
 }
+
+bool panthor_hw_has_feature(struct panthor_device *ptdev, enum panthor_hw_feature feature)
+{
+	return test_bit(feature, ptdev->hw->features);
+}
diff --git a/drivers/gpu/drm/panthor/panthor_hw.h b/drivers/gpu/drm/panthor/panthor_hw.h
index 39752de3e7ad..7a191e76aeec 100644
--- a/drivers/gpu/drm/panthor/panthor_hw.h
+++ b/drivers/gpu/drm/panthor/panthor_hw.h
@@ -4,14 +4,32 @@
 #ifndef __PANTHOR_HW_H__
 #define __PANTHOR_HW_H__
 
+#include <linux/types.h>
+
 struct panthor_device;
 
+/**
+ * enum panthor_hw_feature - Bit position of each HW feature
+ *
+ * Used to define GPU specific features based on the GPU architecture ID.
+ * New feature flags will be added with support for newer GPU architectures.
+ */
+enum panthor_hw_feature {
+	/** @PANTHOR_HW_FEATURES_END: Must be last. */
+	PANTHOR_HW_FEATURES_END
+};
+
+
 /**
  * struct panthor_hw - GPU specific register mapping and functions
  */
 struct panthor_hw {
+	/** @features: Bitmap containing panthor_hw_feature */
+	DECLARE_BITMAP(features, PANTHOR_HW_FEATURES_END);
 };
 
 int panthor_hw_init(struct panthor_device *ptdev);
 
+bool panthor_hw_has_feature(struct panthor_device *ptdev, enum panthor_hw_feature feature);
+
 #endif /* __PANTHOR_HW_H__ */
-- 
2.49.0

Re: [PATCH v1 03/10] drm/panthor: Introduce framework for architecture-specific features

[Steven Price]

On 14/10/2025 10:43, Karunika Choo wrote:
> Add a framework to support architecture-specific features. This allows
> other parts of the driver to adjust their behaviour based on the feature
> bits enabled for a given architecture.
> 
> Signed-off-by: Karunika Choo <karunika.choo@arm.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  drivers/gpu/drm/panthor/panthor_hw.c |  5 +++++
>  drivers/gpu/drm/panthor/panthor_hw.h | 18 ++++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_hw.c b/drivers/gpu/drm/panthor/panthor_hw.c
> index b6e7401327c3..34536526384d 100644
> --- a/drivers/gpu/drm/panthor/panthor_hw.c
> +++ b/drivers/gpu/drm/panthor/panthor_hw.c
> @@ -186,3 +186,8 @@ int panthor_hw_init(struct panthor_device *ptdev)
>  
>  	return 0;
>  }
> +
> +bool panthor_hw_has_feature(struct panthor_device *ptdev, enum panthor_hw_feature feature)
> +{
> +	return test_bit(feature, ptdev->hw->features);
> +}
> diff --git a/drivers/gpu/drm/panthor/panthor_hw.h b/drivers/gpu/drm/panthor/panthor_hw.h
> index 39752de3e7ad..7a191e76aeec 100644
> --- a/drivers/gpu/drm/panthor/panthor_hw.h
> +++ b/drivers/gpu/drm/panthor/panthor_hw.h
> @@ -4,14 +4,32 @@
>  #ifndef __PANTHOR_HW_H__
>  #define __PANTHOR_HW_H__
>  
> +#include <linux/types.h>
> +
>  struct panthor_device;
>  
> +/**
> + * enum panthor_hw_feature - Bit position of each HW feature
> + *
> + * Used to define GPU specific features based on the GPU architecture ID.
> + * New feature flags will be added with support for newer GPU architectures.
> + */
> +enum panthor_hw_feature {
> +	/** @PANTHOR_HW_FEATURES_END: Must be last. */
> +	PANTHOR_HW_FEATURES_END
> +};
> +
> +
>  /**
>   * struct panthor_hw - GPU specific register mapping and functions
>   */
>  struct panthor_hw {
> +	/** @features: Bitmap containing panthor_hw_feature */
> +	DECLARE_BITMAP(features, PANTHOR_HW_FEATURES_END);
>  };
>  
>  int panthor_hw_init(struct panthor_device *ptdev);
>  
> +bool panthor_hw_has_feature(struct panthor_device *ptdev, enum panthor_hw_feature feature);
> +
>  #endif /* __PANTHOR_HW_H__ */

Re: [PATCH v1 03/10] drm/panthor: Introduce framework for architecture-specific features

[Boris Brezillon]

On Tue, 14 Oct 2025 10:43:30 +0100
Karunika Choo <karunika.choo@arm.com> wrote:

> Add a framework to support architecture-specific features. This allows
> other parts of the driver to adjust their behaviour based on the feature
> bits enabled for a given architecture.

I'm not convinced we need this just yet. AFAICT, the only feature flag
being added in this patchset is PANTHOR_HW_FEATURE_PWR_CONTROL, and
most of this is abstracted away with function pointers already. The
only part that tests this FEATURE_PWR_CONTROL flag is the
initialization, which could very much be abstracted away with a
function pointer (NULL meaning no PWR block present). There might be
other use cases you're planning to use this for, so I'd like to hear
about them to make my final opinion on that.

> 
> Signed-off-by: Karunika Choo <karunika.choo@arm.com>
> ---
>  drivers/gpu/drm/panthor/panthor_hw.c |  5 +++++
>  drivers/gpu/drm/panthor/panthor_hw.h | 18 ++++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_hw.c b/drivers/gpu/drm/panthor/panthor_hw.c
> index b6e7401327c3..34536526384d 100644
> --- a/drivers/gpu/drm/panthor/panthor_hw.c
> +++ b/drivers/gpu/drm/panthor/panthor_hw.c
> @@ -186,3 +186,8 @@ int panthor_hw_init(struct panthor_device *ptdev)
>  
>  	return 0;
>  }
> +
> +bool panthor_hw_has_feature(struct panthor_device *ptdev, enum panthor_hw_feature feature)
> +{
> +	return test_bit(feature, ptdev->hw->features);
> +}
> diff --git a/drivers/gpu/drm/panthor/panthor_hw.h b/drivers/gpu/drm/panthor/panthor_hw.h
> index 39752de3e7ad..7a191e76aeec 100644
> --- a/drivers/gpu/drm/panthor/panthor_hw.h
> +++ b/drivers/gpu/drm/panthor/panthor_hw.h
> @@ -4,14 +4,32 @@
>  #ifndef __PANTHOR_HW_H__
>  #define __PANTHOR_HW_H__
>  
> +#include <linux/types.h>
> +
>  struct panthor_device;
>  
> +/**
> + * enum panthor_hw_feature - Bit position of each HW feature
> + *
> + * Used to define GPU specific features based on the GPU architecture ID.
> + * New feature flags will be added with support for newer GPU architectures.
> + */
> +enum panthor_hw_feature {
> +	/** @PANTHOR_HW_FEATURES_END: Must be last. */
> +	PANTHOR_HW_FEATURES_END
> +};
> +
> +
>  /**
>   * struct panthor_hw - GPU specific register mapping and functions
>   */
>  struct panthor_hw {
> +	/** @features: Bitmap containing panthor_hw_feature */
> +	DECLARE_BITMAP(features, PANTHOR_HW_FEATURES_END);
>  };
>  
>  int panthor_hw_init(struct panthor_device *ptdev);
>  
> +bool panthor_hw_has_feature(struct panthor_device *ptdev, enum panthor_hw_feature feature);
> +
>  #endif /* __PANTHOR_HW_H__ */

Re: [PATCH v1 03/10] drm/panthor: Introduce framework for architecture-specific features

[Karunika Choo]

On 24/10/2025 07:43, Boris Brezillon wrote:
> On Tue, 14 Oct 2025 10:43:30 +0100
> Karunika Choo <karunika.choo@arm.com> wrote:
> 
>> Add a framework to support architecture-specific features. This allows
>> other parts of the driver to adjust their behaviour based on the feature
>> bits enabled for a given architecture.
> 
> I'm not convinced we need this just yet. AFAICT, the only feature flag
> being added in this patchset is PANTHOR_HW_FEATURE_PWR_CONTROL, and
> most of this is abstracted away with function pointers already. The
> only part that tests this FEATURE_PWR_CONTROL flag is the
> initialization, which could very much be abstracted away with a
> function pointer (NULL meaning no PWR block present). There might be
> other use cases you're planning to use this for, so I'd like to hear
> about them to make my final opinion on that.
> 

I see your point — the intent here is mainly to have the feature flag
reflect hardware-level changes. In this series, for example, it
corresponds to the addition of the new PWR_CONTROL block.

Another use case would be arch v11, where a new PRFCNT_FEATURES register
was introduced. In that case, we might want to adjust the
counters_per_block [1] value depending on that register’s value.

I would also expect this mechanism to remain useful for future hardware
revisions, as it provides a clean way to describe architectural
differences without scattering version-specific checks throughout the
code, while still being lighter-weight than function pointers.

[1] https://lore.kernel.org/dri-devel/cover.1753449448.git.lukas.zapolskas@arm.com/

Kind regards,
Karunika Choo

>>
>> Signed-off-by: Karunika Choo <karunika.choo@arm.com>
>> ---
>>  drivers/gpu/drm/panthor/panthor_hw.c |  5 +++++
>>  drivers/gpu/drm/panthor/panthor_hw.h | 18 ++++++++++++++++++
>>  2 files changed, 23 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/panthor/panthor_hw.c b/drivers/gpu/drm/panthor/panthor_hw.c
>> index b6e7401327c3..34536526384d 100644
>> --- a/drivers/gpu/drm/panthor/panthor_hw.c
>> +++ b/drivers/gpu/drm/panthor/panthor_hw.c
>> @@ -186,3 +186,8 @@ int panthor_hw_init(struct panthor_device *ptdev)
>>  
>>  	return 0;
>>  }
>> +
>> +bool panthor_hw_has_feature(struct panthor_device *ptdev, enum panthor_hw_feature feature)
>> +{
>> +	return test_bit(feature, ptdev->hw->features);
>> +}
>> diff --git a/drivers/gpu/drm/panthor/panthor_hw.h b/drivers/gpu/drm/panthor/panthor_hw.h
>> index 39752de3e7ad..7a191e76aeec 100644
>> --- a/drivers/gpu/drm/panthor/panthor_hw.h
>> +++ b/drivers/gpu/drm/panthor/panthor_hw.h
>> @@ -4,14 +4,32 @@
>>  #ifndef __PANTHOR_HW_H__
>>  #define __PANTHOR_HW_H__
>>  
>> +#include <linux/types.h>
>> +
>>  struct panthor_device;
>>  
>> +/**
>> + * enum panthor_hw_feature - Bit position of each HW feature
>> + *
>> + * Used to define GPU specific features based on the GPU architecture ID.
>> + * New feature flags will be added with support for newer GPU architectures.
>> + */
>> +enum panthor_hw_feature {
>> +	/** @PANTHOR_HW_FEATURES_END: Must be last. */
>> +	PANTHOR_HW_FEATURES_END
>> +};
>> +
>> +
>>  /**
>>   * struct panthor_hw - GPU specific register mapping and functions
>>   */
>>  struct panthor_hw {
>> +	/** @features: Bitmap containing panthor_hw_feature */
>> +	DECLARE_BITMAP(features, PANTHOR_HW_FEATURES_END);
>>  };
>>  
>>  int panthor_hw_init(struct panthor_device *ptdev);
>>  
>> +bool panthor_hw_has_feature(struct panthor_device *ptdev, enum panthor_hw_feature feature);
>> +
>>  #endif /* __PANTHOR_HW_H__ */
> 

Re: [PATCH v1 03/10] drm/panthor: Introduce framework for architecture-specific features

[Boris Brezillon]

On Fri, 24 Oct 2025 10:26:16 +0100
Karunika Choo <karunika.choo@arm.com> wrote:

> On 24/10/2025 07:43, Boris Brezillon wrote:
> > On Tue, 14 Oct 2025 10:43:30 +0100
> > Karunika Choo <karunika.choo@arm.com> wrote:
> >   
> >> Add a framework to support architecture-specific features. This allows
> >> other parts of the driver to adjust their behaviour based on the feature
> >> bits enabled for a given architecture.  
> > 
> > I'm not convinced we need this just yet. AFAICT, the only feature flag
> > being added in this patchset is PANTHOR_HW_FEATURE_PWR_CONTROL, and
> > most of this is abstracted away with function pointers already. The
> > only part that tests this FEATURE_PWR_CONTROL flag is the
> > initialization, which could very much be abstracted away with a
> > function pointer (NULL meaning no PWR block present). There might be
> > other use cases you're planning to use this for, so I'd like to hear
> > about them to make my final opinion on that.
> >   
> 
> I see your point — the intent here is mainly to have the feature flag
> reflect hardware-level changes. In this series, for example, it
> corresponds to the addition of the new PWR_CONTROL block.

Yes, but those are not really optional features. Those are functional
changes that are usually done on major version changes. But let's say
it was something done on a minor version change, it's still something
that I think would be better off abstracted using a vtable of some
sort, and have this vtable forked everytime a version changes requires
something new.

> 
> Another use case would be arch v11, where a new PRFCNT_FEATURES register
> was introduced. In that case, we might want to adjust the
> counters_per_block [1] value depending on that register’s value.

Again, it looks like a property that can be determined at init time. For
v10 it'd be hardcoded to X, and on v11+, you'd extract that from
PERFCNT_FEATURES. I'm really not a huge fan of this feature flag
pattern because it's very easy to forget to add/propagate one flag when
adding support for new HW/flags. So I'd much rather rely on ">= X.Y"
version checks in the init path, and for anything more involved or
happening in some hot path, function based pointer specialization.

> 
> I would also expect this mechanism to remain useful for future hardware
> revisions, as it provides a clean way to describe architectural
> differences without scattering version-specific checks throughout the
> code, while still being lighter-weight than function pointers.

Well, that's questionable. What I usually see in practice is the
following pattern spreading over the code base:

	if (SUPPORTS(OBSCURE_FEATURE_NAME)) {
		// do stuff that are not obviously related to the
		// feature flag name
	}

whereas, if we're having a model where the specialization is done high
enough, you'd just end up with functions calling more specialized
helpers:

void do_something_for_v12()
{
	hw_block_a_do_y()
	hw_block_b_do_x()
	...
}

[PATCH v1 04/10] drm/panthor: Add architecture-specific function operations

[Karunika Choo]

Introduce architecture-specific function pointers to support
architecture-dependent behaviours. This patch adds the following
function pointers and updates their usage accordingly:

- soft_reset
- l2_power_on
- l2_power_off

Signed-off-by: Karunika Choo <karunika.choo@arm.com>
---
 drivers/gpu/drm/panthor/panthor_device.c |  4 ++--
 drivers/gpu/drm/panthor/panthor_fw.c     |  5 +++--
 drivers/gpu/drm/panthor/panthor_gpu.c    | 13 ++++++++++---
 drivers/gpu/drm/panthor/panthor_gpu.h    |  1 +
 drivers/gpu/drm/panthor/panthor_hw.c     |  9 ++++++++-
 drivers/gpu/drm/panthor/panthor_hw.h     | 23 +++++++++++++++++++++++
 6 files changed, 47 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
index 81df49880bd8..847dea458682 100644
--- a/drivers/gpu/drm/panthor/panthor_device.c
+++ b/drivers/gpu/drm/panthor/panthor_device.c
@@ -141,8 +141,8 @@ static void panthor_device_reset_work(struct work_struct *work)
 	panthor_sched_pre_reset(ptdev);
 	panthor_fw_pre_reset(ptdev, true);
 	panthor_mmu_pre_reset(ptdev);
-	panthor_gpu_soft_reset(ptdev);
-	panthor_gpu_l2_power_on(ptdev);
+	panthor_hw_soft_reset(ptdev);
+	panthor_hw_l2_power_on(ptdev);
 	panthor_mmu_post_reset(ptdev);
 	ret = panthor_fw_post_reset(ptdev);
 	atomic_set(&ptdev->reset.pending, 0);
diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
index 9bf06e55eaee..e6c39c70d348 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.c
+++ b/drivers/gpu/drm/panthor/panthor_fw.c
@@ -21,6 +21,7 @@
 #include "panthor_fw.h"
 #include "panthor_gem.h"
 #include "panthor_gpu.h"
+#include "panthor_hw.h"
 #include "panthor_mmu.h"
 #include "panthor_regs.h"
 #include "panthor_sched.h"
@@ -1184,7 +1185,7 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
 	ptdev->fw->vm = NULL;
 
 	if (!IS_ENABLED(CONFIG_PM) || pm_runtime_active(ptdev->base.dev))
-		panthor_gpu_power_off(ptdev, L2, ptdev->gpu_info.l2_present, 20000);
+		panthor_hw_l2_power_off(ptdev);
 }
 
 /**
@@ -1363,7 +1364,7 @@ int panthor_fw_init(struct panthor_device *ptdev)
 		return ret;
 	}
 
-	ret = panthor_gpu_l2_power_on(ptdev);
+	ret = panthor_hw_l2_power_on(ptdev);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/panthor/panthor_gpu.c b/drivers/gpu/drm/panthor/panthor_gpu.c
index db69449a5be0..431ac866affd 100644
--- a/drivers/gpu/drm/panthor/panthor_gpu.c
+++ b/drivers/gpu/drm/panthor/panthor_gpu.c
@@ -18,6 +18,7 @@
 
 #include "panthor_device.h"
 #include "panthor_gpu.h"
+#include "panthor_hw.h"
 #include "panthor_regs.h"
 
 /**
@@ -218,6 +219,12 @@ int panthor_gpu_block_power_on(struct panthor_device *ptdev,
 	return 0;
 }
 
+int panthor_gpu_l2_power_off(struct panthor_device *ptdev)
+{
+	return panthor_gpu_power_off(ptdev, L2, ptdev->gpu_info.l2_present,
+				     20000);
+}
+
 /**
  * panthor_gpu_l2_power_on() - Power-on the L2-cache
  * @ptdev: Device.
@@ -344,9 +351,9 @@ void panthor_gpu_suspend(struct panthor_device *ptdev)
 {
 	/* On a fast reset, simply power down the L2. */
 	if (!ptdev->reset.fast)
-		panthor_gpu_soft_reset(ptdev);
+		panthor_hw_soft_reset(ptdev);
 	else
-		panthor_gpu_power_off(ptdev, L2, 1, 20000);
+		panthor_hw_l2_power_off(ptdev);
 
 	panthor_gpu_irq_suspend(&ptdev->gpu->irq);
 }
@@ -361,6 +368,6 @@ void panthor_gpu_suspend(struct panthor_device *ptdev)
 void panthor_gpu_resume(struct panthor_device *ptdev)
 {
 	panthor_gpu_irq_resume(&ptdev->gpu->irq, GPU_INTERRUPTS_MASK);
-	panthor_gpu_l2_power_on(ptdev);
+	panthor_hw_l2_power_on(ptdev);
 }
 
diff --git a/drivers/gpu/drm/panthor/panthor_gpu.h b/drivers/gpu/drm/panthor/panthor_gpu.h
index 7c17a8c06858..bc67546f3f6e 100644
--- a/drivers/gpu/drm/panthor/panthor_gpu.h
+++ b/drivers/gpu/drm/panthor/panthor_gpu.h
@@ -46,6 +46,7 @@ int panthor_gpu_block_power_off(struct panthor_device *ptdev,
 				   type ## _PWRTRANS, \
 				   mask, timeout_us)
 
+int panthor_gpu_l2_power_off(struct panthor_device *ptdev);
 int panthor_gpu_l2_power_on(struct panthor_device *ptdev);
 int panthor_gpu_flush_caches(struct panthor_device *ptdev,
 			     u32 l2, u32 lsc, u32 other);
diff --git a/drivers/gpu/drm/panthor/panthor_hw.c b/drivers/gpu/drm/panthor/panthor_hw.c
index 34536526384d..77fd2c56e69f 100644
--- a/drivers/gpu/drm/panthor/panthor_hw.c
+++ b/drivers/gpu/drm/panthor/panthor_hw.c
@@ -2,6 +2,7 @@
 /* Copyright 2025 ARM Limited. All rights reserved. */
 
 #include "panthor_device.h"
+#include "panthor_gpu.h"
 #include "panthor_hw.h"
 #include "panthor_regs.h"
 
@@ -20,7 +21,13 @@ struct panthor_hw_entry {
 	struct panthor_hw *hwdev;
 };
 
-static struct panthor_hw panthor_hw_arch_v10 = {};
+static struct panthor_hw panthor_hw_arch_v10 = {
+	.ops = {
+		.soft_reset = panthor_gpu_soft_reset,
+		.l2_power_off = panthor_gpu_l2_power_off,
+		.l2_power_on = panthor_gpu_l2_power_on,
+	},
+};
 
 static struct panthor_hw_entry panthor_hw_match[] = {
 	{
diff --git a/drivers/gpu/drm/panthor/panthor_hw.h b/drivers/gpu/drm/panthor/panthor_hw.h
index 7a191e76aeec..5a4e4aad9099 100644
--- a/drivers/gpu/drm/panthor/panthor_hw.h
+++ b/drivers/gpu/drm/panthor/panthor_hw.h
@@ -20,12 +20,35 @@ enum panthor_hw_feature {
 };
 
 
+/**
+ * struct panthor_hw_ops - HW operations that are specific to a GPU
+ */
+struct panthor_hw_ops {
+	/** @soft_reset: Soft reset function pointer */
+	int (*soft_reset)(struct panthor_device *ptdev);
+#define panthor_hw_soft_reset(__ptdev) \
+	((__ptdev)->hw->ops.soft_reset ? (__ptdev)->hw->ops.soft_reset(__ptdev) : 0)
+
+	/** @l2_power_off: L2 power off function pointer */
+	int (*l2_power_off)(struct panthor_device *ptdev);
+#define panthor_hw_l2_power_off(__ptdev) \
+	((__ptdev)->hw->ops.l2_power_off ? (__ptdev)->hw->ops.l2_power_off(__ptdev) : 0)
+
+	/** @l2_power_on: L2 power on function pointer */
+	int (*l2_power_on)(struct panthor_device *ptdev);
+#define panthor_hw_l2_power_on(__ptdev) \
+	((__ptdev)->hw->ops.l2_power_on ? (__ptdev)->hw->ops.l2_power_on(__ptdev) : 0)
+};
+
 /**
  * struct panthor_hw - GPU specific register mapping and functions
  */
 struct panthor_hw {
 	/** @features: Bitmap containing panthor_hw_feature */
 	DECLARE_BITMAP(features, PANTHOR_HW_FEATURES_END);
+
+	/** @ops: Panthor HW specific operations */
+	struct panthor_hw_ops ops;
 };
 
 int panthor_hw_init(struct panthor_device *ptdev);
-- 
2.49.0

Re: [PATCH v1 04/10] drm/panthor: Add architecture-specific function operations

[Steven Price]

On 14/10/2025 10:43, Karunika Choo wrote:
> Introduce architecture-specific function pointers to support
> architecture-dependent behaviours. This patch adds the following
> function pointers and updates their usage accordingly:
> 
> - soft_reset
> - l2_power_on
> - l2_power_off
> 
> Signed-off-by: Karunika Choo <karunika.choo@arm.com>
> ---
>  drivers/gpu/drm/panthor/panthor_device.c |  4 ++--
>  drivers/gpu/drm/panthor/panthor_fw.c     |  5 +++--
>  drivers/gpu/drm/panthor/panthor_gpu.c    | 13 ++++++++++---
>  drivers/gpu/drm/panthor/panthor_gpu.h    |  1 +
>  drivers/gpu/drm/panthor/panthor_hw.c     |  9 ++++++++-
>  drivers/gpu/drm/panthor/panthor_hw.h     | 23 +++++++++++++++++++++++
>  6 files changed, 47 insertions(+), 8 deletions(-)
> 
<snip>
> diff --git a/drivers/gpu/drm/panthor/panthor_hw.h b/drivers/gpu/drm/panthor/panthor_hw.h
> index 7a191e76aeec..5a4e4aad9099 100644
> --- a/drivers/gpu/drm/panthor/panthor_hw.h
> +++ b/drivers/gpu/drm/panthor/panthor_hw.h
> @@ -20,12 +20,35 @@ enum panthor_hw_feature {
>  };
>  
>  
> +/**
> + * struct panthor_hw_ops - HW operations that are specific to a GPU
> + */
> +struct panthor_hw_ops {
> +	/** @soft_reset: Soft reset function pointer */
> +	int (*soft_reset)(struct panthor_device *ptdev);
> +#define panthor_hw_soft_reset(__ptdev) \
> +	((__ptdev)->hw->ops.soft_reset ? (__ptdev)->hw->ops.soft_reset(__ptdev) : 0)
> +
> +	/** @l2_power_off: L2 power off function pointer */
> +	int (*l2_power_off)(struct panthor_device *ptdev);
> +#define panthor_hw_l2_power_off(__ptdev) \
> +	((__ptdev)->hw->ops.l2_power_off ? (__ptdev)->hw->ops.l2_power_off(__ptdev) : 0)
> +
> +	/** @l2_power_on: L2 power on function pointer */
> +	int (*l2_power_on)(struct panthor_device *ptdev);
> +#define panthor_hw_l2_power_on(__ptdev) \
> +	((__ptdev)->hw->ops.l2_power_on ? (__ptdev)->hw->ops.l2_power_on(__ptdev) : 0)
> +};

Minor comments:

 * You are defining these to have a return value, but you haven't
updated any of the call-sites to deal with a failure (the return value
is ignored). This is actually an existing problem, but AFAICT the new
_pwr_ versions have more error codes which are simply getting thrown away.

 * Is there a good reason why we need to support these functions being
NULL? It seems unlikely to be useful, and TBH I'd prefer to just assign
a dummy (empty) function in those cases.

 * A static inline function would be neater and would avoid any
potential issues from the multiple evaluation of __ptdev.

Thanks,
Steve

> +
>  /**
>   * struct panthor_hw - GPU specific register mapping and functions
>   */
>  struct panthor_hw {
>  	/** @features: Bitmap containing panthor_hw_feature */
>  	DECLARE_BITMAP(features, PANTHOR_HW_FEATURES_END);
> +
> +	/** @ops: Panthor HW specific operations */
> +	struct panthor_hw_ops ops;
>  };
>  
>  int panthor_hw_init(struct panthor_device *ptdev);

Re: [PATCH v1 04/10] drm/panthor: Add architecture-specific function operations

[Karunika Choo]

On 20/10/2025 10:10, Steven Price wrote:
> On 14/10/2025 10:43, Karunika Choo wrote:
>> Introduce architecture-specific function pointers to support
>> architecture-dependent behaviours. This patch adds the following
>> function pointers and updates their usage accordingly:
>>
>> - soft_reset
>> - l2_power_on
>> - l2_power_off
>>
>> Signed-off-by: Karunika Choo <karunika.choo@arm.com>
>> ---
>>  drivers/gpu/drm/panthor/panthor_device.c |  4 ++--
>>  drivers/gpu/drm/panthor/panthor_fw.c     |  5 +++--
>>  drivers/gpu/drm/panthor/panthor_gpu.c    | 13 ++++++++++---
>>  drivers/gpu/drm/panthor/panthor_gpu.h    |  1 +
>>  drivers/gpu/drm/panthor/panthor_hw.c     |  9 ++++++++-
>>  drivers/gpu/drm/panthor/panthor_hw.h     | 23 +++++++++++++++++++++++
>>  6 files changed, 47 insertions(+), 8 deletions(-)
>>
> <snip>
>> diff --git a/drivers/gpu/drm/panthor/panthor_hw.h b/drivers/gpu/drm/panthor/panthor_hw.h
>> index 7a191e76aeec..5a4e4aad9099 100644
>> --- a/drivers/gpu/drm/panthor/panthor_hw.h
>> +++ b/drivers/gpu/drm/panthor/panthor_hw.h
>> @@ -20,12 +20,35 @@ enum panthor_hw_feature {
>>  };
>>  
>>  
>> +/**
>> + * struct panthor_hw_ops - HW operations that are specific to a GPU
>> + */
>> +struct panthor_hw_ops {
>> +	/** @soft_reset: Soft reset function pointer */
>> +	int (*soft_reset)(struct panthor_device *ptdev);
>> +#define panthor_hw_soft_reset(__ptdev) \
>> +	((__ptdev)->hw->ops.soft_reset ? (__ptdev)->hw->ops.soft_reset(__ptdev) : 0)
>> +
>> +	/** @l2_power_off: L2 power off function pointer */
>> +	int (*l2_power_off)(struct panthor_device *ptdev);
>> +#define panthor_hw_l2_power_off(__ptdev) \
>> +	((__ptdev)->hw->ops.l2_power_off ? (__ptdev)->hw->ops.l2_power_off(__ptdev) : 0)
>> +
>> +	/** @l2_power_on: L2 power on function pointer */
>> +	int (*l2_power_on)(struct panthor_device *ptdev);
>> +#define panthor_hw_l2_power_on(__ptdev) \
>> +	((__ptdev)->hw->ops.l2_power_on ? (__ptdev)->hw->ops.l2_power_on(__ptdev) : 0)
>> +};
> 
> Minor comments:
> 
>  * You are defining these to have a return value, but you haven't
> updated any of the call-sites to deal with a failure (the return value
> is ignored). This is actually an existing problem, but AFAICT the new
> _pwr_ versions have more error codes which are simply getting thrown away.
> 

Hi Steve,

While I agree that there is an existing problem, I'd argue that most of
these are called from void functions where there really isn't much
benefit in handling the return value apart from printing a "whoops"
(which the called functions themselves mostly already do) and
continuing. In fact, in the one place it isn't called from a void
function, we do handle the return value.

Still, I do think that it is an issue, biggest of which probably is the
soft reset work. Perhaps we can revisit this topic when we want to have
another go at the soft reset handling in the future?

>  * Is there a good reason why we need to support these functions being
> NULL? It seems unlikely to be useful, and TBH I'd prefer to just assign
> a dummy (empty) function in those cases.
> 
>  * A static inline function would be neater and would avoid any
> potential issues from the multiple evaluation of __ptdev.
> 
> Thanks,
> Steve
> 

Thanks for pointing this out + the suggestion, I will change this in v2.

Kind regards,
Karunika

>> +
>>  /**
>>   * struct panthor_hw - GPU specific register mapping and functions
>>   */
>>  struct panthor_hw {
>>  	/** @features: Bitmap containing panthor_hw_feature */
>>  	DECLARE_BITMAP(features, PANTHOR_HW_FEATURES_END);
>> +
>> +	/** @ops: Panthor HW specific operations */
>> +	struct panthor_hw_ops ops;
>>  };
>>  
>>  int panthor_hw_init(struct panthor_device *ptdev);
> 

Re: [PATCH v1 04/10] drm/panthor: Add architecture-specific function operations

[Steven Price]

On 23/10/2025 21:59, Karunika Choo wrote:
> On 20/10/2025 10:10, Steven Price wrote:
>> On 14/10/2025 10:43, Karunika Choo wrote:
>>> Introduce architecture-specific function pointers to support
>>> architecture-dependent behaviours. This patch adds the following
>>> function pointers and updates their usage accordingly:
>>>
>>> - soft_reset
>>> - l2_power_on
>>> - l2_power_off
>>>
>>> Signed-off-by: Karunika Choo <karunika.choo@arm.com>
>>> ---
>>>  drivers/gpu/drm/panthor/panthor_device.c |  4 ++--
>>>  drivers/gpu/drm/panthor/panthor_fw.c     |  5 +++--
>>>  drivers/gpu/drm/panthor/panthor_gpu.c    | 13 ++++++++++---
>>>  drivers/gpu/drm/panthor/panthor_gpu.h    |  1 +
>>>  drivers/gpu/drm/panthor/panthor_hw.c     |  9 ++++++++-
>>>  drivers/gpu/drm/panthor/panthor_hw.h     | 23 +++++++++++++++++++++++
>>>  6 files changed, 47 insertions(+), 8 deletions(-)
>>>
>> <snip>
>>> diff --git a/drivers/gpu/drm/panthor/panthor_hw.h b/drivers/gpu/drm/panthor/panthor_hw.h
>>> index 7a191e76aeec..5a4e4aad9099 100644
>>> --- a/drivers/gpu/drm/panthor/panthor_hw.h
>>> +++ b/drivers/gpu/drm/panthor/panthor_hw.h
>>> @@ -20,12 +20,35 @@ enum panthor_hw_feature {
>>>  };
>>>  
>>>  
>>> +/**
>>> + * struct panthor_hw_ops - HW operations that are specific to a GPU
>>> + */
>>> +struct panthor_hw_ops {
>>> +	/** @soft_reset: Soft reset function pointer */
>>> +	int (*soft_reset)(struct panthor_device *ptdev);
>>> +#define panthor_hw_soft_reset(__ptdev) \
>>> +	((__ptdev)->hw->ops.soft_reset ? (__ptdev)->hw->ops.soft_reset(__ptdev) : 0)
>>> +
>>> +	/** @l2_power_off: L2 power off function pointer */
>>> +	int (*l2_power_off)(struct panthor_device *ptdev);
>>> +#define panthor_hw_l2_power_off(__ptdev) \
>>> +	((__ptdev)->hw->ops.l2_power_off ? (__ptdev)->hw->ops.l2_power_off(__ptdev) : 0)
>>> +
>>> +	/** @l2_power_on: L2 power on function pointer */
>>> +	int (*l2_power_on)(struct panthor_device *ptdev);
>>> +#define panthor_hw_l2_power_on(__ptdev) \
>>> +	((__ptdev)->hw->ops.l2_power_on ? (__ptdev)->hw->ops.l2_power_on(__ptdev) : 0)
>>> +};
>>
>> Minor comments:
>>
>>  * You are defining these to have a return value, but you haven't
>> updated any of the call-sites to deal with a failure (the return value
>> is ignored). This is actually an existing problem, but AFAICT the new
>> _pwr_ versions have more error codes which are simply getting thrown away.
>>
> 
> Hi Steve,
> 
> While I agree that there is an existing problem, I'd argue that most of
> these are called from void functions where there really isn't much
> benefit in handling the return value apart from printing a "whoops"
> (which the called functions themselves mostly already do) and
> continuing. In fact, in the one place it isn't called from a void
> function, we do handle the return value.
> 
> Still, I do think that it is an issue, biggest of which probably is the
> soft reset work. Perhaps we can revisit this topic when we want to have
> another go at the soft reset handling in the future?

I agree it's not making things worse. But if we're adding another
backend it's worth stopping to think about the API. Does it make sense
to return an error code if the call sites cannot handle the error?
Should any of these function be void return? Specifically the power off
call as I'm not sure what the caller can usefully do if that fails.

I'm happy for the soft reset to be left for now, it would be good to
handle errors properly in that path but it's an orthogonal problem to
this series.

Thanks,
Steve

>>  * Is there a good reason why we need to support these functions being
>> NULL? It seems unlikely to be useful, and TBH I'd prefer to just assign
>> a dummy (empty) function in those cases.
>>
>>  * A static inline function would be neater and would avoid any
>> potential issues from the multiple evaluation of __ptdev.
>>
>> Thanks,
>> Steve
>>
> 
> Thanks for pointing this out + the suggestion, I will change this in v2.
> 
> Kind regards,
> Karunika
> 
>>> +
>>>  /**
>>>   * struct panthor_hw - GPU specific register mapping and functions
>>>   */
>>>  struct panthor_hw {
>>>  	/** @features: Bitmap containing panthor_hw_feature */
>>>  	DECLARE_BITMAP(features, PANTHOR_HW_FEATURES_END);
>>> +
>>> +	/** @ops: Panthor HW specific operations */
>>> +	struct panthor_hw_ops ops;
>>>  };
>>>  
>>>  int panthor_hw_init(struct panthor_device *ptdev);
>>
> 

[PATCH v1 05/10] drm/panthor: Introduce panthor_pwr API and power control framework

[Karunika Choo]

Add the new panthor_pwr module, which provides basic power control
management for Mali-G1 GPUs. The initial implementation includes
infrastructure for initializing the PWR_CONTROL block, requesting and
handling its IRQ, and defining the PANTHOR_HW_FEATURE_PWR_CONTROL
feature flag.

The patch also integrates panthor_pwr with the device lifecycle (init,
suspend, resume, and unplug) through the new API functions. It also
registers the IRQ handler under the 'gpu' IRQ as the PWR_CONTROL block
is located within the GPU_CONTROL block.

Signed-off-by: Karunika Choo <karunika.choo@arm.com>
---
 drivers/gpu/drm/panthor/Makefile         |   1 +
 drivers/gpu/drm/panthor/panthor_device.c |  14 ++-
 drivers/gpu/drm/panthor/panthor_device.h |   4 +
 drivers/gpu/drm/panthor/panthor_hw.h     |   3 +
 drivers/gpu/drm/panthor/panthor_pwr.c    | 135 +++++++++++++++++++++++
 drivers/gpu/drm/panthor/panthor_pwr.h    |  23 ++++
 drivers/gpu/drm/panthor/panthor_regs.h   |  79 +++++++++++++
 7 files changed, 258 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/panthor/panthor_pwr.c
 create mode 100644 drivers/gpu/drm/panthor/panthor_pwr.h

diff --git a/drivers/gpu/drm/panthor/Makefile b/drivers/gpu/drm/panthor/Makefile
index 02db21748c12..753a32c446df 100644
--- a/drivers/gpu/drm/panthor/Makefile
+++ b/drivers/gpu/drm/panthor/Makefile
@@ -10,6 +10,7 @@ panthor-y := \
 	panthor_heap.o \
 	panthor_hw.o \
 	panthor_mmu.o \
+	panthor_pwr.o \
 	panthor_sched.o
 
 obj-$(CONFIG_DRM_PANTHOR) += panthor.o
diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
index 847dea458682..d3e16da0b24e 100644
--- a/drivers/gpu/drm/panthor/panthor_device.c
+++ b/drivers/gpu/drm/panthor/panthor_device.c
@@ -20,6 +20,7 @@
 #include "panthor_gpu.h"
 #include "panthor_hw.h"
 #include "panthor_mmu.h"
+#include "panthor_pwr.h"
 #include "panthor_regs.h"
 #include "panthor_sched.h"
 
@@ -102,6 +103,7 @@ void panthor_device_unplug(struct panthor_device *ptdev)
 	panthor_fw_unplug(ptdev);
 	panthor_mmu_unplug(ptdev);
 	panthor_gpu_unplug(ptdev);
+	panthor_pwr_unplug(ptdev);
 
 	pm_runtime_dont_use_autosuspend(ptdev->base.dev);
 	pm_runtime_put_sync_suspend(ptdev->base.dev);
@@ -249,10 +251,14 @@ int panthor_device_init(struct panthor_device *ptdev)
 	if (ret)
 		goto err_rpm_put;
 
-	ret = panthor_gpu_init(ptdev);
+	ret = panthor_pwr_init(ptdev);
 	if (ret)
 		goto err_rpm_put;
 
+	ret = panthor_gpu_init(ptdev);
+	if (ret)
+		goto err_unplug_pwr;
+
 	ret = panthor_gpu_coherency_init(ptdev);
 	if (ret)
 		goto err_unplug_gpu;
@@ -293,6 +299,9 @@ int panthor_device_init(struct panthor_device *ptdev)
 err_unplug_gpu:
 	panthor_gpu_unplug(ptdev);
 
+err_unplug_pwr:
+	panthor_pwr_unplug(ptdev);
+
 err_rpm_put:
 	pm_runtime_put_sync_suspend(ptdev->base.dev);
 	return ret;
@@ -446,6 +455,7 @@ static int panthor_device_resume_hw_components(struct panthor_device *ptdev)
 {
 	int ret;
 
+	panthor_pwr_resume(ptdev);
 	panthor_gpu_resume(ptdev);
 	panthor_mmu_resume(ptdev);
 
@@ -455,6 +465,7 @@ static int panthor_device_resume_hw_components(struct panthor_device *ptdev)
 
 	panthor_mmu_suspend(ptdev);
 	panthor_gpu_suspend(ptdev);
+	panthor_pwr_suspend(ptdev);
 	return ret;
 }
 
@@ -568,6 +579,7 @@ int panthor_device_suspend(struct device *dev)
 		panthor_fw_suspend(ptdev);
 		panthor_mmu_suspend(ptdev);
 		panthor_gpu_suspend(ptdev);
+		panthor_pwr_suspend(ptdev);
 		drm_dev_exit(cookie);
 	}
 
diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
index 1457c1255f1f..05818318e0ba 100644
--- a/drivers/gpu/drm/panthor/panthor_device.h
+++ b/drivers/gpu/drm/panthor/panthor_device.h
@@ -31,6 +31,7 @@ struct panthor_job;
 struct panthor_mmu;
 struct panthor_fw;
 struct panthor_perfcnt;
+struct panthor_pwr;
 struct panthor_vm;
 struct panthor_vm_pool;
 
@@ -126,6 +127,9 @@ struct panthor_device {
 	/** @hw: GPU-specific data. */
 	struct panthor_hw *hw;
 
+	/** @pwr: Power control management data. */
+	struct panthor_pwr *pwr;
+
 	/** @gpu: GPU management data. */
 	struct panthor_gpu *gpu;
 
diff --git a/drivers/gpu/drm/panthor/panthor_hw.h b/drivers/gpu/drm/panthor/panthor_hw.h
index 5a4e4aad9099..caba522cd680 100644
--- a/drivers/gpu/drm/panthor/panthor_hw.h
+++ b/drivers/gpu/drm/panthor/panthor_hw.h
@@ -15,6 +15,9 @@ struct panthor_device;
  * New feature flags will be added with support for newer GPU architectures.
  */
 enum panthor_hw_feature {
+	/** @PANTHOR_HW_FEATURE_PWR_CONTROL: HW supports the PWR_CONTROL interface. */
+	PANTHOR_HW_FEATURE_PWR_CONTROL,
+
 	/** @PANTHOR_HW_FEATURES_END: Must be last. */
 	PANTHOR_HW_FEATURES_END
 };
diff --git a/drivers/gpu/drm/panthor/panthor_pwr.c b/drivers/gpu/drm/panthor/panthor_pwr.c
new file mode 100644
index 000000000000..d07ad5b7953a
--- /dev/null
+++ b/drivers/gpu/drm/panthor/panthor_pwr.c
@@ -0,0 +1,135 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+/* Copyright 2025 ARM Limited. All rights reserved. */
+
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/wait.h>
+
+#include <drm/drm_managed.h>
+
+#include "panthor_device.h"
+#include "panthor_hw.h"
+#include "panthor_pwr.h"
+#include "panthor_regs.h"
+
+#define PWR_INTERRUPTS_MASK \
+	(PWR_IRQ_POWER_CHANGED_SINGLE | \
+	 PWR_IRQ_POWER_CHANGED_ALL | \
+	 PWR_IRQ_DELEGATION_CHANGED | \
+	 PWR_IRQ_RESET_COMPLETED | \
+	 PWR_IRQ_RETRACT_COMPLETED | \
+	 PWR_IRQ_INSPECT_COMPLETED | \
+	 PWR_IRQ_COMMAND_NOT_ALLOWED | \
+	 PWR_IRQ_COMMAND_INVALID)
+
+/**
+ * struct panthor_pwr - PWR_CONTROL block management data.
+ */
+struct panthor_pwr {
+	/** @irq: PWR irq. */
+	struct panthor_irq irq;
+
+	/** @reqs_lock: Lock protecting access to pending_reqs. */
+	spinlock_t reqs_lock;
+
+	/** @pending_reqs: Pending PWR requests. */
+	u32 pending_reqs;
+
+	/** @reqs_acked: PWR request wait queue. */
+	wait_queue_head_t reqs_acked;
+};
+
+static void panthor_pwr_irq_handler(struct panthor_device *ptdev, u32 status)
+{
+	spin_lock(&ptdev->pwr->reqs_lock);
+	gpu_write(ptdev, PWR_INT_CLEAR, status);
+
+	if (unlikely(status & PWR_IRQ_COMMAND_NOT_ALLOWED))
+		drm_err(&ptdev->base, "PWR_IRQ: COMMAND_NOT_ALLOWED");
+
+	if (unlikely(status & PWR_IRQ_COMMAND_INVALID))
+		drm_err(&ptdev->base, "PWR_IRQ: COMMAND_INVALID");
+
+	if (status & ptdev->pwr->pending_reqs) {
+		ptdev->pwr->pending_reqs &= ~status;
+		wake_up_all(&ptdev->pwr->reqs_acked);
+	}
+	spin_unlock(&ptdev->pwr->reqs_lock);
+}
+PANTHOR_IRQ_HANDLER(pwr, PWR, panthor_pwr_irq_handler);
+
+void panthor_pwr_unplug(struct panthor_device *ptdev)
+{
+	unsigned long flags;
+
+	if (!ptdev->pwr)
+		return;
+
+	/* Make sure the IRQ handler is not running after that point. */
+	panthor_pwr_irq_suspend(&ptdev->pwr->irq);
+
+	/* Wake-up all waiters. */
+	spin_lock_irqsave(&ptdev->pwr->reqs_lock, flags);
+	ptdev->pwr->pending_reqs = 0;
+	wake_up_all(&ptdev->pwr->reqs_acked);
+	spin_unlock_irqrestore(&ptdev->pwr->reqs_lock, flags);
+}
+
+int panthor_pwr_init(struct panthor_device *ptdev)
+{
+	struct panthor_pwr *pwr;
+	int err, irq;
+
+	if (!panthor_hw_has_feature(ptdev, PANTHOR_HW_FEATURE_PWR_CONTROL))
+		return 0;
+
+	pwr = drmm_kzalloc(&ptdev->base, sizeof(*pwr), GFP_KERNEL);
+	if (!pwr)
+		return -ENOMEM;
+
+	spin_lock_init(&pwr->reqs_lock);
+	init_waitqueue_head(&pwr->reqs_acked);
+	ptdev->pwr = pwr;
+
+	irq = platform_get_irq_byname(to_platform_device(ptdev->base.dev), "gpu");
+	if (irq < 0)
+		return irq;
+
+	err = panthor_request_pwr_irq(ptdev, &pwr->irq, irq, PWR_INTERRUPTS_MASK);
+	if (err)
+		return err;
+
+	return 0;
+}
+
+int panthor_pwr_reset_soft(struct panthor_device *ptdev)
+{
+	return 0;
+}
+
+int panthor_pwr_l2_power_off(struct panthor_device *ptdev)
+{
+	return 0;
+}
+
+int panthor_pwr_l2_power_on(struct panthor_device *ptdev)
+{
+	return 0;
+}
+
+void panthor_pwr_suspend(struct panthor_device *ptdev)
+{
+	if (!ptdev->pwr)
+		return;
+
+	panthor_pwr_irq_suspend(&ptdev->pwr->irq);
+}
+
+void panthor_pwr_resume(struct panthor_device *ptdev)
+{
+	if (!ptdev->pwr)
+		return;
+
+	panthor_pwr_irq_resume(&ptdev->pwr->irq, PWR_INTERRUPTS_MASK);
+}
diff --git a/drivers/gpu/drm/panthor/panthor_pwr.h b/drivers/gpu/drm/panthor/panthor_pwr.h
new file mode 100644
index 000000000000..a4042c125448
--- /dev/null
+++ b/drivers/gpu/drm/panthor/panthor_pwr.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 or MIT */
+/* Copyright 2025 ARM Limited. All rights reserved. */
+
+#ifndef __PANTHOR_PWR_H__
+#define __PANTHOR_PWR_H__
+
+struct panthor_device;
+
+void panthor_pwr_unplug(struct panthor_device *ptdev);
+
+int panthor_pwr_init(struct panthor_device *ptdev);
+
+int panthor_pwr_reset_soft(struct panthor_device *ptdev);
+
+int panthor_pwr_l2_power_on(struct panthor_device *ptdev);
+
+int panthor_pwr_l2_power_off(struct panthor_device *ptdev);
+
+void panthor_pwr_suspend(struct panthor_device *ptdev);
+
+void panthor_pwr_resume(struct panthor_device *ptdev);
+
+#endif /* __PANTHOR_PWR_H__ */
diff --git a/drivers/gpu/drm/panthor/panthor_regs.h b/drivers/gpu/drm/panthor/panthor_regs.h
index 8bee76d01bf8..84db97c11e68 100644
--- a/drivers/gpu/drm/panthor/panthor_regs.h
+++ b/drivers/gpu/drm/panthor/panthor_regs.h
@@ -72,6 +72,7 @@
 
 #define GPU_FEATURES					0x60
 #define   GPU_FEATURES_RAY_INTERSECTION			BIT(2)
+#define   GPU_FEATURES_RAY_TRAVERSAL			BIT(5)
 
 #define GPU_TIMESTAMP_OFFSET				0x88
 #define GPU_CYCLE_COUNT					0x90
@@ -205,4 +206,82 @@
 #define CSF_DOORBELL(i)					(0x80000 + ((i) * 0x10000))
 #define CSF_GLB_DOORBELL_ID				0
 
+/* PWR Control registers */
+
+#define PWR_CONTROL_BASE				0x800
+#define PWR_CTRL_REG(x)					(PWR_CONTROL_BASE + (x))
+
+#define PWR_INT_RAWSTAT					PWR_CTRL_REG(0x0)
+#define PWR_INT_CLEAR					PWR_CTRL_REG(0x4)
+#define PWR_INT_MASK					PWR_CTRL_REG(0x8)
+#define PWR_INT_STAT					PWR_CTRL_REG(0xc)
+#define   PWR_IRQ_POWER_CHANGED_SINGLE			BIT(0)
+#define   PWR_IRQ_POWER_CHANGED_ALL			BIT(1)
+#define   PWR_IRQ_DELEGATION_CHANGED			BIT(2)
+#define   PWR_IRQ_RESET_COMPLETED			BIT(3)
+#define   PWR_IRQ_RETRACT_COMPLETED			BIT(4)
+#define   PWR_IRQ_INSPECT_COMPLETED			BIT(5)
+#define   PWR_IRQ_COMMAND_NOT_ALLOWED			BIT(30)
+#define   PWR_IRQ_COMMAND_INVALID			BIT(31)
+
+#define PWR_STATUS					PWR_CTRL_REG(0x20)
+#define   PWR_STATUS_ALLOW_L2				BIT(0)
+#define   PWR_STATUS_ALLOW_TILER			BIT(1)
+#define   PWR_STATUS_ALLOW_SHADER			BIT(8)
+#define   PWR_STATUS_ALLOW_BASE				BIT(14)
+#define   PWR_STATUS_ALLOW_STACK			BIT(15)
+#define   PWR_STATUS_DOMAIN_ALLOWED(x)			(1 << (x))
+#define   PWR_STATUS_DELEGATED_L2			BIT(16)
+#define   PWR_STATUS_DELEGATED_TILER			BIT(17)
+#define   PWR_STATUS_DELEGATED_SHADER			BIT(24)
+#define   PWR_STATUS_DELEGATED_BASE			BIT(30)
+#define   PWR_STATUS_DELEGATED_STACK			BIT(31)
+#define   PWR_STATUS_DELEGATED_SHIFT			16
+#define   PWR_STATUS_DOMAIN_DELEGATED(x)		(1 << ((x) + PWR_STATUS_DELEGATED_SHIFT))
+#define   PWR_STATUS_ALLOW_SOFT_RESET			BIT(33)
+#define   PWR_STATUS_ALLOW_FAST_RESET			BIT(34)
+#define   PWR_STATUS_POWER_PENDING			BIT(41)
+#define   PWR_STATUS_RESET_PENDING			BIT(42)
+#define   PWR_STATUS_RETRACT_PENDING			BIT(43)
+#define   PWR_STATUS_INSPECT_PENDING			BIT(44)
+
+#define PWR_COMMAND					PWR_CTRL_REG(0x28)
+#define   PWR_COMMAND_POWER_UP				0x10
+#define   PWR_COMMAND_POWER_DOWN			0x11
+#define   PWR_COMMAND_DELEGATE				0x20
+#define   PWR_COMMAND_RETRACT				0x21
+#define   PWR_COMMAND_RESET_SOFT			0x31
+#define   PWR_COMMAND_RESET_FAST			0x32
+#define   PWR_COMMAND_INSPECT				0xF0
+#define   PWR_COMMAND_DOMAIN_L2				0
+#define   PWR_COMMAND_DOMAIN_TILER			1
+#define   PWR_COMMAND_DOMAIN_SHADER			8
+#define   PWR_COMMAND_DOMAIN_BASE			14
+#define   PWR_COMMAND_DOMAIN_STACK			15
+#define   PWR_COMMAND_SUBDOMAIN_RTU			BIT(0)
+#define   PWR_COMMAND_DEF(cmd, domain, subdomain)	\
+	(((subdomain) << 16) | ((domain) << 8) | (cmd))
+
+#define PWR_CMDARG					PWR_CTRL_REG(0x30)
+
+#define PWR_L2_PRESENT					PWR_CTRL_REG(0x100)
+#define PWR_L2_READY					PWR_CTRL_REG(0x108)
+#define PWR_L2_PWRTRANS					PWR_CTRL_REG(0x110)
+#define PWR_L2_PWRACTIVE				PWR_CTRL_REG(0x118)
+#define PWR_TILER_PRESENT				PWR_CTRL_REG(0x140)
+#define PWR_TILER_READY					PWR_CTRL_REG(0x148)
+#define PWR_TILER_PWRTRANS				PWR_CTRL_REG(0x150)
+#define PWR_TILER_PWRACTIVE				PWR_CTRL_REG(0x158)
+#define PWR_SHADER_PRESENT				PWR_CTRL_REG(0x200)
+#define PWR_SHADER_READY				PWR_CTRL_REG(0x208)
+#define PWR_SHADER_PWRTRANS				PWR_CTRL_REG(0x210)
+#define PWR_SHADER_PWRACTIVE				PWR_CTRL_REG(0x218)
+#define PWR_BASE_PRESENT				PWR_CTRL_REG(0x380)
+#define PWR_BASE_READY					PWR_CTRL_REG(0x388)
+#define PWR_BASE_PWRTRANS				PWR_CTRL_REG(0x390)
+#define PWR_BASE_PWRACTIVE				PWR_CTRL_REG(0x398)
+#define PWR_STACK_PRESENT				PWR_CTRL_REG(0x3c0)
+#define PWR_STACK_READY					PWR_CTRL_REG(0x3c8)
+#define PWR_STACK_PWRTRANS				PWR_CTRL_REG(0x3d0)
+
 #endif
-- 
2.49.0

Re: [PATCH v1 05/10] drm/panthor: Introduce panthor_pwr API and power control framework

[Steven Price]

On 14/10/2025 10:43, Karunika Choo wrote:
> Add the new panthor_pwr module, which provides basic power control
> management for Mali-G1 GPUs. The initial implementation includes
> infrastructure for initializing the PWR_CONTROL block, requesting and
> handling its IRQ, and defining the PANTHOR_HW_FEATURE_PWR_CONTROL
> feature flag.
> 
> The patch also integrates panthor_pwr with the device lifecycle (init,
> suspend, resume, and unplug) through the new API functions. It also
> registers the IRQ handler under the 'gpu' IRQ as the PWR_CONTROL block
> is located within the GPU_CONTROL block.
> 
> Signed-off-by: Karunika Choo <karunika.choo@arm.com>

Mostlt looks good - two minor comments below.

> ---
>  drivers/gpu/drm/panthor/Makefile         |   1 +
>  drivers/gpu/drm/panthor/panthor_device.c |  14 ++-
>  drivers/gpu/drm/panthor/panthor_device.h |   4 +
>  drivers/gpu/drm/panthor/panthor_hw.h     |   3 +
>  drivers/gpu/drm/panthor/panthor_pwr.c    | 135 +++++++++++++++++++++++
>  drivers/gpu/drm/panthor/panthor_pwr.h    |  23 ++++
>  drivers/gpu/drm/panthor/panthor_regs.h   |  79 +++++++++++++
>  7 files changed, 258 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/panthor/panthor_pwr.c
>  create mode 100644 drivers/gpu/drm/panthor/panthor_pwr.h
> 
> diff --git a/drivers/gpu/drm/panthor/Makefile b/drivers/gpu/drm/panthor/Makefile
> index 02db21748c12..753a32c446df 100644
> --- a/drivers/gpu/drm/panthor/Makefile
> +++ b/drivers/gpu/drm/panthor/Makefile
> @@ -10,6 +10,7 @@ panthor-y := \
>  	panthor_heap.o \
>  	panthor_hw.o \
>  	panthor_mmu.o \
> +	panthor_pwr.o \
>  	panthor_sched.o
>  
>  obj-$(CONFIG_DRM_PANTHOR) += panthor.o
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 847dea458682..d3e16da0b24e 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -20,6 +20,7 @@
>  #include "panthor_gpu.h"
>  #include "panthor_hw.h"
>  #include "panthor_mmu.h"
> +#include "panthor_pwr.h"
>  #include "panthor_regs.h"
>  #include "panthor_sched.h"
>  
> @@ -102,6 +103,7 @@ void panthor_device_unplug(struct panthor_device *ptdev)
>  	panthor_fw_unplug(ptdev);
>  	panthor_mmu_unplug(ptdev);
>  	panthor_gpu_unplug(ptdev);
> +	panthor_pwr_unplug(ptdev);
>  
>  	pm_runtime_dont_use_autosuspend(ptdev->base.dev);
>  	pm_runtime_put_sync_suspend(ptdev->base.dev);
> @@ -249,10 +251,14 @@ int panthor_device_init(struct panthor_device *ptdev)
>  	if (ret)
>  		goto err_rpm_put;
>  
> -	ret = panthor_gpu_init(ptdev);
> +	ret = panthor_pwr_init(ptdev);
>  	if (ret)
>  		goto err_rpm_put;
>  
> +	ret = panthor_gpu_init(ptdev);
> +	if (ret)
> +		goto err_unplug_pwr;
> +
>  	ret = panthor_gpu_coherency_init(ptdev);
>  	if (ret)
>  		goto err_unplug_gpu;
> @@ -293,6 +299,9 @@ int panthor_device_init(struct panthor_device *ptdev)
>  err_unplug_gpu:
>  	panthor_gpu_unplug(ptdev);
>  
> +err_unplug_pwr:
> +	panthor_pwr_unplug(ptdev);
> +
>  err_rpm_put:
>  	pm_runtime_put_sync_suspend(ptdev->base.dev);
>  	return ret;
> @@ -446,6 +455,7 @@ static int panthor_device_resume_hw_components(struct panthor_device *ptdev)
>  {
>  	int ret;
>  
> +	panthor_pwr_resume(ptdev);
>  	panthor_gpu_resume(ptdev);
>  	panthor_mmu_resume(ptdev);
>  
> @@ -455,6 +465,7 @@ static int panthor_device_resume_hw_components(struct panthor_device *ptdev)
>  
>  	panthor_mmu_suspend(ptdev);
>  	panthor_gpu_suspend(ptdev);
> +	panthor_pwr_suspend(ptdev);
>  	return ret;
>  }
>  
> @@ -568,6 +579,7 @@ int panthor_device_suspend(struct device *dev)
>  		panthor_fw_suspend(ptdev);
>  		panthor_mmu_suspend(ptdev);
>  		panthor_gpu_suspend(ptdev);
> +		panthor_pwr_suspend(ptdev);
>  		drm_dev_exit(cookie);
>  	}
>  
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index 1457c1255f1f..05818318e0ba 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -31,6 +31,7 @@ struct panthor_job;
>  struct panthor_mmu;
>  struct panthor_fw;
>  struct panthor_perfcnt;
> +struct panthor_pwr;
>  struct panthor_vm;
>  struct panthor_vm_pool;
>  
> @@ -126,6 +127,9 @@ struct panthor_device {
>  	/** @hw: GPU-specific data. */
>  	struct panthor_hw *hw;
>  
> +	/** @pwr: Power control management data. */
> +	struct panthor_pwr *pwr;
> +
>  	/** @gpu: GPU management data. */
>  	struct panthor_gpu *gpu;
>  
> diff --git a/drivers/gpu/drm/panthor/panthor_hw.h b/drivers/gpu/drm/panthor/panthor_hw.h
> index 5a4e4aad9099..caba522cd680 100644
> --- a/drivers/gpu/drm/panthor/panthor_hw.h
> +++ b/drivers/gpu/drm/panthor/panthor_hw.h
> @@ -15,6 +15,9 @@ struct panthor_device;
>   * New feature flags will be added with support for newer GPU architectures.
>   */
>  enum panthor_hw_feature {
> +	/** @PANTHOR_HW_FEATURE_PWR_CONTROL: HW supports the PWR_CONTROL interface. */
> +	PANTHOR_HW_FEATURE_PWR_CONTROL,
> +
>  	/** @PANTHOR_HW_FEATURES_END: Must be last. */
>  	PANTHOR_HW_FEATURES_END
>  };
> diff --git a/drivers/gpu/drm/panthor/panthor_pwr.c b/drivers/gpu/drm/panthor/panthor_pwr.c
> new file mode 100644
> index 000000000000..d07ad5b7953a
> --- /dev/null
> +++ b/drivers/gpu/drm/panthor/panthor_pwr.c
> @@ -0,0 +1,135 @@
> +// SPDX-License-Identifier: GPL-2.0 or MIT
> +/* Copyright 2025 ARM Limited. All rights reserved. */
> +
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/wait.h>
> +
> +#include <drm/drm_managed.h>
> +
> +#include "panthor_device.h"
> +#include "panthor_hw.h"
> +#include "panthor_pwr.h"
> +#include "panthor_regs.h"
> +
> +#define PWR_INTERRUPTS_MASK \
> +	(PWR_IRQ_POWER_CHANGED_SINGLE | \
> +	 PWR_IRQ_POWER_CHANGED_ALL | \
> +	 PWR_IRQ_DELEGATION_CHANGED | \
> +	 PWR_IRQ_RESET_COMPLETED | \
> +	 PWR_IRQ_RETRACT_COMPLETED | \
> +	 PWR_IRQ_INSPECT_COMPLETED | \
> +	 PWR_IRQ_COMMAND_NOT_ALLOWED | \
> +	 PWR_IRQ_COMMAND_INVALID)
> +
> +/**
> + * struct panthor_pwr - PWR_CONTROL block management data.
> + */
> +struct panthor_pwr {
> +	/** @irq: PWR irq. */
> +	struct panthor_irq irq;
> +
> +	/** @reqs_lock: Lock protecting access to pending_reqs. */
> +	spinlock_t reqs_lock;
> +
> +	/** @pending_reqs: Pending PWR requests. */
> +	u32 pending_reqs;
> +
> +	/** @reqs_acked: PWR request wait queue. */
> +	wait_queue_head_t reqs_acked;
> +};
> +
> +static void panthor_pwr_irq_handler(struct panthor_device *ptdev, u32 status)
> +{
> +	spin_lock(&ptdev->pwr->reqs_lock);
> +	gpu_write(ptdev, PWR_INT_CLEAR, status);
> +
> +	if (unlikely(status & PWR_IRQ_COMMAND_NOT_ALLOWED))
> +		drm_err(&ptdev->base, "PWR_IRQ: COMMAND_NOT_ALLOWED");
> +
> +	if (unlikely(status & PWR_IRQ_COMMAND_INVALID))
> +		drm_err(&ptdev->base, "PWR_IRQ: COMMAND_INVALID");
> +
> +	if (status & ptdev->pwr->pending_reqs) {
> +		ptdev->pwr->pending_reqs &= ~status;
> +		wake_up_all(&ptdev->pwr->reqs_acked);
> +	}
> +	spin_unlock(&ptdev->pwr->reqs_lock);
> +}
> +PANTHOR_IRQ_HANDLER(pwr, PWR, panthor_pwr_irq_handler);
> +
> +void panthor_pwr_unplug(struct panthor_device *ptdev)
> +{
> +	unsigned long flags;
> +
> +	if (!ptdev->pwr)
> +		return;
> +
> +	/* Make sure the IRQ handler is not running after that point. */
> +	panthor_pwr_irq_suspend(&ptdev->pwr->irq);
> +
> +	/* Wake-up all waiters. */
> +	spin_lock_irqsave(&ptdev->pwr->reqs_lock, flags);
> +	ptdev->pwr->pending_reqs = 0;
> +	wake_up_all(&ptdev->pwr->reqs_acked);
> +	spin_unlock_irqrestore(&ptdev->pwr->reqs_lock, flags);
> +}
> +
> +int panthor_pwr_init(struct panthor_device *ptdev)
> +{
> +	struct panthor_pwr *pwr;
> +	int err, irq;
> +
> +	if (!panthor_hw_has_feature(ptdev, PANTHOR_HW_FEATURE_PWR_CONTROL))
> +		return 0;
> +
> +	pwr = drmm_kzalloc(&ptdev->base, sizeof(*pwr), GFP_KERNEL);
> +	if (!pwr)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&pwr->reqs_lock);
> +	init_waitqueue_head(&pwr->reqs_acked);
> +	ptdev->pwr = pwr;
> +
> +	irq = platform_get_irq_byname(to_platform_device(ptdev->base.dev), "gpu");
> +	if (irq < 0)
> +		return irq;
> +
> +	err = panthor_request_pwr_irq(ptdev, &pwr->irq, irq, PWR_INTERRUPTS_MASK);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +int panthor_pwr_reset_soft(struct panthor_device *ptdev)
> +{
> +	return 0;
> +}
> +
> +int panthor_pwr_l2_power_off(struct panthor_device *ptdev)
> +{
> +	return 0;
> +}
> +
> +int panthor_pwr_l2_power_on(struct panthor_device *ptdev)
> +{
> +	return 0;
> +}

I don't see any need to add these dummy functions - just add the full
implementation in the next patch.

> +
> +void panthor_pwr_suspend(struct panthor_device *ptdev)
> +{
> +	if (!ptdev->pwr)
> +		return;
> +
> +	panthor_pwr_irq_suspend(&ptdev->pwr->irq);
> +}
> +
> +void panthor_pwr_resume(struct panthor_device *ptdev)
> +{
> +	if (!ptdev->pwr)
> +		return;
> +
> +	panthor_pwr_irq_resume(&ptdev->pwr->irq, PWR_INTERRUPTS_MASK);
> +}
> diff --git a/drivers/gpu/drm/panthor/panthor_pwr.h b/drivers/gpu/drm/panthor/panthor_pwr.h
> new file mode 100644
> index 000000000000..a4042c125448
> --- /dev/null
> +++ b/drivers/gpu/drm/panthor/panthor_pwr.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0 or MIT */
> +/* Copyright 2025 ARM Limited. All rights reserved. */
> +
> +#ifndef __PANTHOR_PWR_H__
> +#define __PANTHOR_PWR_H__
> +
> +struct panthor_device;
> +
> +void panthor_pwr_unplug(struct panthor_device *ptdev);
> +
> +int panthor_pwr_init(struct panthor_device *ptdev);
> +
> +int panthor_pwr_reset_soft(struct panthor_device *ptdev);
> +
> +int panthor_pwr_l2_power_on(struct panthor_device *ptdev);
> +
> +int panthor_pwr_l2_power_off(struct panthor_device *ptdev);
> +
> +void panthor_pwr_suspend(struct panthor_device *ptdev);
> +
> +void panthor_pwr_resume(struct panthor_device *ptdev);
> +
> +#endif /* __PANTHOR_PWR_H__ */
> diff --git a/drivers/gpu/drm/panthor/panthor_regs.h b/drivers/gpu/drm/panthor/panthor_regs.h
> index 8bee76d01bf8..84db97c11e68 100644
> --- a/drivers/gpu/drm/panthor/panthor_regs.h
> +++ b/drivers/gpu/drm/panthor/panthor_regs.h
> @@ -72,6 +72,7 @@
>  
>  #define GPU_FEATURES					0x60
>  #define   GPU_FEATURES_RAY_INTERSECTION			BIT(2)
> +#define   GPU_FEATURES_RAY_TRAVERSAL			BIT(5)

This line shouldn't be in this patch.

Thanks,
Steve

>  
>  #define GPU_TIMESTAMP_OFFSET				0x88
>  #define GPU_CYCLE_COUNT					0x90
> @@ -205,4 +206,82 @@
>  #define CSF_DOORBELL(i)					(0x80000 + ((i) * 0x10000))
>  #define CSF_GLB_DOORBELL_ID				0
>  
> +/* PWR Control registers */
> +
> +#define PWR_CONTROL_BASE				0x800
> +#define PWR_CTRL_REG(x)					(PWR_CONTROL_BASE + (x))
> +
> +#define PWR_INT_RAWSTAT					PWR_CTRL_REG(0x0)
> +#define PWR_INT_CLEAR					PWR_CTRL_REG(0x4)
> +#define PWR_INT_MASK					PWR_CTRL_REG(0x8)
> +#define PWR_INT_STAT					PWR_CTRL_REG(0xc)
> +#define   PWR_IRQ_POWER_CHANGED_SINGLE			BIT(0)
> +#define   PWR_IRQ_POWER_CHANGED_ALL			BIT(1)
> +#define   PWR_IRQ_DELEGATION_CHANGED			BIT(2)
> +#define   PWR_IRQ_RESET_COMPLETED			BIT(3)
> +#define   PWR_IRQ_RETRACT_COMPLETED			BIT(4)
> +#define   PWR_IRQ_INSPECT_COMPLETED			BIT(5)
> +#define   PWR_IRQ_COMMAND_NOT_ALLOWED			BIT(30)
> +#define   PWR_IRQ_COMMAND_INVALID			BIT(31)
> +
> +#define PWR_STATUS					PWR_CTRL_REG(0x20)
> +#define   PWR_STATUS_ALLOW_L2				BIT(0)
> +#define   PWR_STATUS_ALLOW_TILER			BIT(1)
> +#define   PWR_STATUS_ALLOW_SHADER			BIT(8)
> +#define   PWR_STATUS_ALLOW_BASE				BIT(14)
> +#define   PWR_STATUS_ALLOW_STACK			BIT(15)
> +#define   PWR_STATUS_DOMAIN_ALLOWED(x)			(1 << (x))
> +#define   PWR_STATUS_DELEGATED_L2			BIT(16)
> +#define   PWR_STATUS_DELEGATED_TILER			BIT(17)
> +#define   PWR_STATUS_DELEGATED_SHADER			BIT(24)
> +#define   PWR_STATUS_DELEGATED_BASE			BIT(30)
> +#define   PWR_STATUS_DELEGATED_STACK			BIT(31)
> +#define   PWR_STATUS_DELEGATED_SHIFT			16
> +#define   PWR_STATUS_DOMAIN_DELEGATED(x)		(1 << ((x) + PWR_STATUS_DELEGATED_SHIFT))
> +#define   PWR_STATUS_ALLOW_SOFT_RESET			BIT(33)
> +#define   PWR_STATUS_ALLOW_FAST_RESET			BIT(34)
> +#define   PWR_STATUS_POWER_PENDING			BIT(41)
> +#define   PWR_STATUS_RESET_PENDING			BIT(42)
> +#define   PWR_STATUS_RETRACT_PENDING			BIT(43)
> +#define   PWR_STATUS_INSPECT_PENDING			BIT(44)
> +
> +#define PWR_COMMAND					PWR_CTRL_REG(0x28)
> +#define   PWR_COMMAND_POWER_UP				0x10
> +#define   PWR_COMMAND_POWER_DOWN			0x11
> +#define   PWR_COMMAND_DELEGATE				0x20
> +#define   PWR_COMMAND_RETRACT				0x21
> +#define   PWR_COMMAND_RESET_SOFT			0x31
> +#define   PWR_COMMAND_RESET_FAST			0x32
> +#define   PWR_COMMAND_INSPECT				0xF0
> +#define   PWR_COMMAND_DOMAIN_L2				0
> +#define   PWR_COMMAND_DOMAIN_TILER			1
> +#define   PWR_COMMAND_DOMAIN_SHADER			8
> +#define   PWR_COMMAND_DOMAIN_BASE			14
> +#define   PWR_COMMAND_DOMAIN_STACK			15
> +#define   PWR_COMMAND_SUBDOMAIN_RTU			BIT(0)
> +#define   PWR_COMMAND_DEF(cmd, domain, subdomain)	\
> +	(((subdomain) << 16) | ((domain) << 8) | (cmd))
> +
> +#define PWR_CMDARG					PWR_CTRL_REG(0x30)
> +
> +#define PWR_L2_PRESENT					PWR_CTRL_REG(0x100)
> +#define PWR_L2_READY					PWR_CTRL_REG(0x108)
> +#define PWR_L2_PWRTRANS					PWR_CTRL_REG(0x110)
> +#define PWR_L2_PWRACTIVE				PWR_CTRL_REG(0x118)
> +#define PWR_TILER_PRESENT				PWR_CTRL_REG(0x140)
> +#define PWR_TILER_READY					PWR_CTRL_REG(0x148)
> +#define PWR_TILER_PWRTRANS				PWR_CTRL_REG(0x150)
> +#define PWR_TILER_PWRACTIVE				PWR_CTRL_REG(0x158)
> +#define PWR_SHADER_PRESENT				PWR_CTRL_REG(0x200)
> +#define PWR_SHADER_READY				PWR_CTRL_REG(0x208)
> +#define PWR_SHADER_PWRTRANS				PWR_CTRL_REG(0x210)
> +#define PWR_SHADER_PWRACTIVE				PWR_CTRL_REG(0x218)
> +#define PWR_BASE_PRESENT				PWR_CTRL_REG(0x380)
> +#define PWR_BASE_READY					PWR_CTRL_REG(0x388)
> +#define PWR_BASE_PWRTRANS				PWR_CTRL_REG(0x390)
> +#define PWR_BASE_PWRACTIVE				PWR_CTRL_REG(0x398)
> +#define PWR_STACK_PRESENT				PWR_CTRL_REG(0x3c0)
> +#define PWR_STACK_READY					PWR_CTRL_REG(0x3c8)
> +#define PWR_STACK_PWRTRANS				PWR_CTRL_REG(0x3d0)
> +
>  #endif

[PATCH v1 06/10] drm/panthor: Implement L2 power on/off via PWR_CONTROL

[Karunika Choo]

This patch adds common helpers to issue power commands, poll
transitions, and validate domain state, then wires them into the L2
on/off paths.

The L2 power-on sequence now delegates control of the SHADER and TILER
domains to the MCU when allowed, while the L2 itself is never delegated.
On power-off, dependent domains beneath the L2 are checked, and if
necessary, retracted and powered down to maintain proper domain
ordering.

Signed-off-by: Karunika Choo <karunika.choo@arm.com>
---
 drivers/gpu/drm/panthor/panthor_pwr.c | 390 +++++++++++++++++++++++++-
 1 file changed, 388 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_pwr.c b/drivers/gpu/drm/panthor/panthor_pwr.c
index d07ad5b7953a..594181aba847 100644
--- a/drivers/gpu/drm/panthor/panthor_pwr.c
+++ b/drivers/gpu/drm/panthor/panthor_pwr.c
@@ -23,6 +23,14 @@
 	 PWR_IRQ_COMMAND_NOT_ALLOWED | \
 	 PWR_IRQ_COMMAND_INVALID)
 
+#define PWR_ALL_CORES_MASK		GENMASK(63, 0)
+
+#define PWR_DOMAIN_MAX_BITS		16
+
+#define PWR_TRANSITION_TIMEOUT_US	2000000
+
+#define PWR_RETRACT_TIMEOUT_US		2000
+
 /**
  * struct panthor_pwr - PWR_CONTROL block management data.
  */
@@ -59,6 +67,302 @@ static void panthor_pwr_irq_handler(struct panthor_device *ptdev, u32 status)
 }
 PANTHOR_IRQ_HANDLER(pwr, PWR, panthor_pwr_irq_handler);
 
+static u64 panthor_pwr_read_status(struct panthor_device *ptdev)
+{
+	return gpu_read64(ptdev, PWR_STATUS);
+}
+
+static void panthor_pwr_write_command(struct panthor_device *ptdev, u32 command, u64 args)
+{
+	if (args)
+		gpu_write64(ptdev, PWR_CMDARG, args);
+
+	gpu_write(ptdev, PWR_COMMAND, command);
+}
+
+static const char *get_domain_name(u8 domain)
+{
+	switch (domain) {
+	case PWR_COMMAND_DOMAIN_L2:
+		return "L2";
+	case PWR_COMMAND_DOMAIN_TILER:
+		return "Tiler";
+	case PWR_COMMAND_DOMAIN_SHADER:
+		return "Shader";
+	case PWR_COMMAND_DOMAIN_BASE:
+		return "Base";
+	case PWR_COMMAND_DOMAIN_STACK:
+		return "Stack";
+	}
+	return "Unknown";
+}
+
+static u32 get_domain_base(u8 domain)
+{
+	switch (domain) {
+	case PWR_COMMAND_DOMAIN_L2:
+		return PWR_L2_PRESENT;
+	case PWR_COMMAND_DOMAIN_TILER:
+		return PWR_TILER_PRESENT;
+	case PWR_COMMAND_DOMAIN_SHADER:
+		return PWR_SHADER_PRESENT;
+	case PWR_COMMAND_DOMAIN_BASE:
+		return PWR_BASE_PRESENT;
+	case PWR_COMMAND_DOMAIN_STACK:
+		return PWR_STACK_PRESENT;
+	}
+	return 0;
+}
+
+static u32 get_domain_ready_reg(u32 domain)
+{
+	return get_domain_base(domain) + (PWR_L2_READY - PWR_L2_PRESENT);
+}
+
+static u32 get_domain_pwrtrans_reg(u32 domain)
+{
+	return get_domain_base(domain) + (PWR_L2_PWRTRANS - PWR_L2_PRESENT);
+}
+
+static bool is_valid_domain(u32 domain)
+{
+	return get_domain_base(domain) != 0;
+}
+
+static bool has_rtu(struct panthor_device *ptdev)
+{
+	return ptdev->gpu_info.gpu_features & GPU_FEATURES_RAY_TRAVERSAL;
+}
+
+static u8 get_domain_subdomain(struct panthor_device *ptdev, u32 domain)
+{
+	if ((domain == PWR_COMMAND_DOMAIN_SHADER) && has_rtu(ptdev))
+		return PWR_COMMAND_SUBDOMAIN_RTU;
+
+	return 0;
+}
+
+static int panthor_pwr_domain_wait_transition(struct panthor_device *ptdev, u32 domain,
+					      u32 timeout_us)
+{
+	u32 pwrtrans_reg = get_domain_pwrtrans_reg(domain);
+	u64 val;
+	int ret = 0;
+
+	ret = gpu_read64_poll_timeout(ptdev, pwrtrans_reg, val,
+				      !(PWR_ALL_CORES_MASK & val), 100,
+				      timeout_us);
+	if (ret) {
+		drm_err(&ptdev->base, "%s domain power in transition, pwrtrans(0x%llx)",
+			get_domain_name(domain), val);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void panthor_pwr_info_show(struct panthor_device *ptdev)
+{
+	drm_info(&ptdev->base, "GPU_FEATURES:    0x%016llx", ptdev->gpu_info.gpu_features);
+	drm_info(&ptdev->base, "PWR_STATUS:      0x%016llx", gpu_read64(ptdev, PWR_STATUS));
+	drm_info(&ptdev->base, "L2_PRESENT:      0x%016llx", gpu_read64(ptdev, PWR_L2_PRESENT));
+	drm_info(&ptdev->base, "L2_PWRTRANS:     0x%016llx", gpu_read64(ptdev, PWR_L2_PWRTRANS));
+	drm_info(&ptdev->base, "L2_READY:        0x%016llx", gpu_read64(ptdev, PWR_L2_READY));
+	drm_info(&ptdev->base, "TILER_PRESENT:   0x%016llx", gpu_read64(ptdev, PWR_TILER_PRESENT));
+	drm_info(&ptdev->base, "TILER_PWRTRANS:  0x%016llx", gpu_read64(ptdev, PWR_TILER_PWRTRANS));
+	drm_info(&ptdev->base, "TILER_READY:     0x%016llx", gpu_read64(ptdev, PWR_TILER_READY));
+	drm_info(&ptdev->base, "SHADER_PRESENT:  0x%016llx", gpu_read64(ptdev, PWR_SHADER_PRESENT));
+	drm_info(&ptdev->base, "SHADER_PWRTRANS: 0x%016llx",
+		 gpu_read64(ptdev, PWR_SHADER_PWRTRANS));
+	drm_info(&ptdev->base, "SHADER_READY:    0x%016llx", gpu_read64(ptdev, PWR_SHADER_READY));
+}
+
+static int panthor_pwr_domain_transition(struct panthor_device *ptdev, u32 cmd, u32 domain,
+					 u64 mask, u32 timeout_us)
+{
+	u32 ready_reg = get_domain_ready_reg(domain);
+	u32 pwr_cmd = PWR_COMMAND_DEF(cmd, domain, get_domain_subdomain(ptdev, domain));
+	u64 expected_val = cmd == PWR_COMMAND_POWER_DOWN ? 0 : mask;
+	u64 val;
+	int ret = 0;
+
+	if (!is_valid_domain(domain))
+		return -EINVAL;
+
+	if (cmd != PWR_COMMAND_POWER_DOWN && cmd != PWR_COMMAND_POWER_UP) {
+		drm_err(&ptdev->base, "Invalid power domain transition command (0x%x)", cmd);
+		return -EINVAL;
+	}
+
+	ret = panthor_pwr_domain_wait_transition(ptdev, domain, timeout_us);
+	if (ret)
+		return ret;
+
+	/* domain already in target state, return early */
+	if ((gpu_read64(ptdev, ready_reg) & mask) == expected_val)
+		return 0;
+
+	panthor_pwr_write_command(ptdev, pwr_cmd, mask);
+
+	ret = gpu_read64_poll_timeout(ptdev, ready_reg, val, (mask & val) == expected_val, 100,
+				      timeout_us);
+	if (ret) {
+		drm_err(&ptdev->base, "timeout waiting on %s power %s, cmd(0x%x), arg(0x%llx)",
+			get_domain_name(domain), cmd == PWR_COMMAND_POWER_DOWN ? "down" : "up",
+			pwr_cmd, mask);
+		panthor_pwr_info_show(ptdev);
+		return ret;
+	}
+
+	return 0;
+}
+
+#define panthor_pwr_domain_power_off(__ptdev, __domain, __mask, __timeout_us)            \
+	panthor_pwr_domain_transition(__ptdev, PWR_COMMAND_POWER_DOWN, __domain, __mask, \
+				      __timeout_us)
+
+#define panthor_pwr_domain_power_on(__ptdev, __domain, __mask, __timeout_us) \
+	panthor_pwr_domain_transition(__ptdev, PWR_COMMAND_POWER_UP, __domain, __mask, __timeout_us)
+
+/**
+ * retract_domain() - Retract control of a domain from MCU
+ * @ptdev: Device.
+ * @domain: Domain to retract the control
+ *
+ * Retracting L2 domain is not expected since it won't be delegated.
+ *
+ * Return: 0 on success or retracted already.
+ *         -EPERM if domain is L2.
+ *         A negative error code otherwise.
+ */
+static int retract_domain(struct panthor_device *ptdev, u32 domain)
+{
+	const u32 pwr_cmd = PWR_COMMAND_DEF(PWR_COMMAND_RETRACT, domain, 0);
+	const u32 pwr_status_reg = panthor_pwr_read_status(ptdev);
+	const u32 delegated_mask = PWR_STATUS_DOMAIN_DELEGATED(domain);
+	const u32 allow_mask = PWR_STATUS_DOMAIN_ALLOWED(domain);
+	u64 val;
+	int ret;
+
+	if (WARN_ON(domain == PWR_COMMAND_DOMAIN_L2))
+		return -EINVAL;
+
+	if (WARN_ON(pwr_status_reg & PWR_STATUS_DOMAIN_DELEGATED(PWR_COMMAND_DOMAIN_L2)))
+		return -EPERM;
+
+	if (!(pwr_status_reg & delegated_mask)) {
+		drm_dbg(&ptdev->base, "%s domain already retracted",
+			get_domain_name(domain));
+		return 0;
+	}
+
+	ret = gpu_read64_poll_timeout(ptdev, PWR_STATUS, val,
+				      !(PWR_STATUS_RETRACT_PENDING & val), 0,
+				      PWR_RETRACT_TIMEOUT_US);
+	if (ret) {
+		drm_err(&ptdev->base, "%s domain retract pending",
+			get_domain_name(domain));
+		return ret;
+	}
+
+	panthor_pwr_write_command(ptdev, pwr_cmd, 0);
+
+	/*
+	 * On successful retraction
+	 * allow-flag will be set with delegated-flag being cleared.
+	 */
+	ret = gpu_read64_poll_timeout(ptdev, PWR_STATUS, val,
+				      ((delegated_mask | allow_mask) & val) == allow_mask,
+				      10, PWR_TRANSITION_TIMEOUT_US);
+	if (ret) {
+		drm_err(&ptdev->base, "Retracting %s domain timeout, cmd(0x%x)",
+			get_domain_name(domain), pwr_cmd);
+		return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * delegate_domain() - Delegate control of a domain to MCU
+ * @ptdev: Device.
+ * @domain: Domain to delegate the control
+ *
+ * Delegating L2 domain is prohibited.
+ *
+ * Return:
+ * *       0 on success or delegated already.
+ * *       -EPERM if domain is L2.
+ * *       A negative error code otherwise.
+ */
+static int delegate_domain(struct panthor_device *ptdev, u32 domain)
+{
+	const u32 pwr_cmd = PWR_COMMAND_DEF(PWR_COMMAND_DELEGATE, domain, 0);
+	const u32 pwr_status_reg = panthor_pwr_read_status(ptdev);
+	const u32 pwrtrans_reg = get_domain_pwrtrans_reg(domain);
+	const u32 allow_mask = PWR_STATUS_DOMAIN_ALLOWED(domain);
+	const u32 delegated_mask = PWR_STATUS_DOMAIN_DELEGATED(domain);
+	u64 val;
+	int ret;
+
+	if (WARN_ON_ONCE(domain == PWR_COMMAND_DOMAIN_L2))
+		return -EINVAL;
+
+	if (pwr_status_reg & delegated_mask) {
+		drm_dbg(&ptdev->base, "%s domain already delegated",
+			get_domain_name(domain));
+		return 0;
+	}
+
+	/* Check if the command is allowed before delegating. */
+	if (unlikely(!(pwr_status_reg & allow_mask))) {
+		drm_warn(&ptdev->base, "Delegating %s domain not allowed",
+			 get_domain_name(domain));
+		return -EPERM;
+	}
+
+	if (unlikely(gpu_read64(ptdev, pwrtrans_reg))) {
+		drm_warn(&ptdev->base, "%s domain power in transition",
+			 get_domain_name(domain));
+		return -EPERM;
+	}
+
+	panthor_pwr_write_command(ptdev, pwr_cmd, 0);
+
+	/*
+	 * On successful delegation
+	 * allow-flag will be cleared with delegated-flag being set.
+	 */
+	ret = gpu_read64_poll_timeout(ptdev, PWR_STATUS, val,
+				      ((delegated_mask | allow_mask) & val) == delegated_mask,
+				      10, PWR_TRANSITION_TIMEOUT_US);
+	if (ret) {
+		drm_err(&ptdev->base, "Delegating %s domain timeout, cmd(0x%x)",
+			get_domain_name(domain), pwr_cmd);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int panthor_pwr_delegate_domains(struct panthor_device *ptdev)
+{
+	int ret;
+
+	if (!ptdev->pwr)
+		return 0;
+
+	ret = delegate_domain(ptdev, PWR_COMMAND_DOMAIN_SHADER);
+	if (ret)
+		return ret;
+
+	ret = delegate_domain(ptdev, PWR_COMMAND_DOMAIN_TILER);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 void panthor_pwr_unplug(struct panthor_device *ptdev)
 {
 	unsigned long flags;
@@ -110,12 +414,94 @@ int panthor_pwr_reset_soft(struct panthor_device *ptdev)
 
 int panthor_pwr_l2_power_off(struct panthor_device *ptdev)
 {
-	return 0;
+	const u32 l2_allow_mask = PWR_STATUS_DOMAIN_ALLOWED(PWR_COMMAND_DOMAIN_L2);
+	const u32 l2_pwrtrans_reg = get_domain_pwrtrans_reg(PWR_COMMAND_DOMAIN_L2);
+	unsigned long child_domain_mask = {
+		BIT(PWR_COMMAND_DOMAIN_SHADER) |
+		BIT(PWR_COMMAND_DOMAIN_TILER)
+	};
+	u32 pwr_status, child_domain;
+	int ret = 0;
+
+	if (unlikely(!ptdev->pwr))
+		return -ENODEV;
+
+	pwr_status = panthor_pwr_read_status(ptdev);
+
+	/* Abort if L2 power off constraints are not satisfied */
+	if (!(pwr_status & l2_allow_mask)) {
+		drm_warn(&ptdev->base, "Power off L2 domain not allowed");
+		return 0;
+	}
+
+	if (gpu_read64(ptdev, l2_pwrtrans_reg)) {
+		drm_warn(&ptdev->base, "L2 Power in transition");
+		return 0;
+	}
+
+	/* It is expected that when halting the MCU, it would power down its
+	 * delegated domains. However, an unresponsive or hung MCU may not do
+	 * so, which is why we need to check and retract the domains back into
+	 * host control to be powered down before powering down the L2.
+	 */
+	for_each_set_bit(child_domain, &child_domain_mask, PWR_DOMAIN_MAX_BITS) {
+		u64 domain_ready = gpu_read64(ptdev, get_domain_ready_reg(child_domain));
+
+		if (domain_ready && (pwr_status & PWR_STATUS_DOMAIN_DELEGATED(child_domain))) {
+			drm_warn(&ptdev->base,
+				 "L2 power off: Delegated %s domain not powered down by MCU",
+				 get_domain_name(child_domain));
+			ret = retract_domain(ptdev, child_domain);
+			if (ret) {
+				drm_err(&ptdev->base, "Failed to retract %s domain",
+					get_domain_name(child_domain));
+				panthor_pwr_info_show(ptdev);
+				return ret;
+			}
+		}
+
+		ret = panthor_pwr_domain_power_off(ptdev, child_domain, domain_ready,
+						   PWR_TRANSITION_TIMEOUT_US);
+		if (ret)
+			return ret;
+	}
+
+	return panthor_pwr_domain_power_off(ptdev, PWR_COMMAND_DOMAIN_L2,
+					    ptdev->gpu_info.l2_present,
+					    PWR_TRANSITION_TIMEOUT_US);
 }
 
 int panthor_pwr_l2_power_on(struct panthor_device *ptdev)
 {
-	return 0;
+	const u32 pwr_status = panthor_pwr_read_status(ptdev);
+	const u32 l2_allow_mask = PWR_STATUS_DOMAIN_ALLOWED(PWR_COMMAND_DOMAIN_L2);
+	const u32 l2_pwrtrans_reg = get_domain_pwrtrans_reg(PWR_COMMAND_DOMAIN_L2);
+	int ret;
+
+	if (!ptdev->pwr)
+		return 0;
+
+	if ((pwr_status & l2_allow_mask) == 0) {
+		drm_warn(&ptdev->base, "Power on L2 domain not allowed");
+		return -EPERM;
+	}
+
+	if (gpu_read64(ptdev, l2_pwrtrans_reg)) {
+		drm_warn(&ptdev->base, "L2 Power is in transition");
+		return -EINVAL;
+	}
+
+	ret = panthor_pwr_domain_power_on(ptdev, PWR_COMMAND_DOMAIN_L2,
+					  ptdev->gpu_info.l2_present,
+					  PWR_TRANSITION_TIMEOUT_US);
+	if (ret)
+		return ret;
+
+	/* Delegate control of the shader and tiler power domains to the MCU as
+	 * it can better manage which shader/tiler cores need to be powered up
+	 * or can be powered down based on currently running jobs.
+	 */
+	return panthor_pwr_delegate_domains(ptdev);
 }
 
 void panthor_pwr_suspend(struct panthor_device *ptdev)
-- 
2.49.0

Re: [PATCH v1 06/10] drm/panthor: Implement L2 power on/off via PWR_CONTROL

[kernel test robot]

Hi Karunika,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip linus/master v6.18-rc1 next-20251014]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Karunika-Choo/drm-panthor-Factor-out-GPU_ID-register-read-into-separate-function/20251014-174729
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20251014094337.1009601-7-karunika.choo%40arm.com
patch subject: [PATCH v1 06/10] drm/panthor: Implement L2 power on/off via PWR_CONTROL
config: i386-buildonly-randconfig-002-20251015 (https://download.01.org/0day-ci/archive/20251015/202510151252.aKq1qUWF-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251015/202510151252.aKq1qUWF-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510151252.aKq1qUWF-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/gpu/drm/panthor/panthor_pwr.c:6:
   drivers/gpu/drm/panthor/panthor_pwr.c: In function 'panthor_pwr_domain_wait_transition':
>> include/linux/bits.h:49:27: warning: right shift count >= width of type [-Wshift-count-overflow]
      49 |               type_max(t) >> (BITS_PER_TYPE(t) - 1 - (h)))))
         |                           ^~
   include/linux/iopoll.h:145:9: note: in expansion of macro 'poll_timeout_us'
     145 |         poll_timeout_us((val) = op(args), cond, sleep_us, timeout_us, sleep_before_read)
         |         ^~~~~~~~~~~~~~~
   drivers/gpu/drm/panthor/panthor_device.h:533:9: note: in expansion of macro 'read_poll_timeout'
     533 |         read_poll_timeout(gpu_read64, val, cond, delay_us, timeout_us, false,   \
         |         ^~~~~~~~~~~~~~~~~
   drivers/gpu/drm/panthor/panthor_pwr.c:152:15: note: in expansion of macro 'gpu_read64_poll_timeout'
     152 |         ret = gpu_read64_poll_timeout(ptdev, pwrtrans_reg, val,
         |               ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/bits.h:51:33: note: in expansion of macro 'GENMASK_TYPE'
      51 | #define GENMASK(h, l)           GENMASK_TYPE(unsigned long, h, l)
         |                                 ^~~~~~~~~~~~
   drivers/gpu/drm/panthor/panthor_pwr.c:26:41: note: in expansion of macro 'GENMASK'
      26 | #define PWR_ALL_CORES_MASK              GENMASK(63, 0)
         |                                         ^~~~~~~
   drivers/gpu/drm/panthor/panthor_pwr.c:153:41: note: in expansion of macro 'PWR_ALL_CORES_MASK'
     153 |                                       !(PWR_ALL_CORES_MASK & val), 100,
         |                                         ^~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/panthor/panthor_pwr.c: In function 'retract_domain':
>> include/vdso/bits.h:7:40: warning: left shift count >= width of type [-Wshift-count-overflow]
       7 | #define BIT(nr)                 (UL(1) << (nr))
         |                                        ^~
   include/linux/iopoll.h:49:21: note: in definition of macro 'poll_timeout_us'
      49 |                 if (cond) { \
         |                     ^~~~
   drivers/gpu/drm/panthor/panthor_device.h:533:9: note: in expansion of macro 'read_poll_timeout'
     533 |         read_poll_timeout(gpu_read64, val, cond, delay_us, timeout_us, false,   \
         |         ^~~~~~~~~~~~~~~~~
   drivers/gpu/drm/panthor/panthor_pwr.c:259:15: note: in expansion of macro 'gpu_read64_poll_timeout'
     259 |         ret = gpu_read64_poll_timeout(ptdev, PWR_STATUS, val,
         |               ^~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/panthor/panthor_regs.h:249:57: note: in expansion of macro 'BIT'
     249 | #define   PWR_STATUS_RETRACT_PENDING                    BIT(43)
         |                                                         ^~~
   drivers/gpu/drm/panthor/panthor_pwr.c:260:41: note: in expansion of macro 'PWR_STATUS_RETRACT_PENDING'
     260 |                                       !(PWR_STATUS_RETRACT_PENDING & val), 0,
         |                                         ^~~~~~~~~~~~~~~~~~~~~~~~~~


vim +49 include/linux/bits.h

31299a5e021124 Vincent Mailhol 2025-03-26  35  
19408200c09485 Vincent Mailhol 2025-03-26  36  /*
19408200c09485 Vincent Mailhol 2025-03-26  37   * Generate a mask for the specified type @t. Additional checks are made to
19408200c09485 Vincent Mailhol 2025-03-26  38   * guarantee the value returned fits in that type, relying on
19408200c09485 Vincent Mailhol 2025-03-26  39   * -Wshift-count-overflow compiler check to detect incompatible arguments.
19408200c09485 Vincent Mailhol 2025-03-26  40   * For example, all these create build errors or warnings:
19408200c09485 Vincent Mailhol 2025-03-26  41   *
19408200c09485 Vincent Mailhol 2025-03-26  42   * - GENMASK(15, 20): wrong argument order
19408200c09485 Vincent Mailhol 2025-03-26  43   * - GENMASK(72, 15): doesn't fit unsigned long
19408200c09485 Vincent Mailhol 2025-03-26  44   * - GENMASK_U32(33, 15): doesn't fit in a u32
19408200c09485 Vincent Mailhol 2025-03-26  45   */
19408200c09485 Vincent Mailhol 2025-03-26  46  #define GENMASK_TYPE(t, h, l)					\
19408200c09485 Vincent Mailhol 2025-03-26  47  	((t)(GENMASK_INPUT_CHECK(h, l) +			\
19408200c09485 Vincent Mailhol 2025-03-26  48  	     (type_max(t) << (l) &				\
19408200c09485 Vincent Mailhol 2025-03-26 @49  	      type_max(t) >> (BITS_PER_TYPE(t) - 1 - (h)))))
19408200c09485 Vincent Mailhol 2025-03-26  50  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 06/10] drm/panthor: Implement L2 power on/off via PWR_CONTROL

[kernel test robot]

Hi Karunika,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip linus/master v6.18-rc1 next-20251015]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Karunika-Choo/drm-panthor-Factor-out-GPU_ID-register-read-into-separate-function/20251014-174729
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20251014094337.1009601-7-karunika.choo%40arm.com
patch subject: [PATCH v1 06/10] drm/panthor: Implement L2 power on/off via PWR_CONTROL
config: arm-randconfig-r132-20251016 (https://download.01.org/0day-ci/archive/20251016/202510161626.f3OG4u62-lkp@intel.com/config)
compiler: clang version 22.0.0git (https://github.com/llvm/llvm-project 39f292ffa13d7ca0d1edff27ac8fd55024bb4d19)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251016/202510161626.f3OG4u62-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202510161626.f3OG4u62-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/panthor/panthor_pwr.c:153:13: warning: shift count >= width of type [-Wshift-count-overflow]
     152 |         ret = gpu_read64_poll_timeout(ptdev, pwrtrans_reg, val,
         |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     153 |                                       !(PWR_ALL_CORES_MASK & val), 100,
         |                                       ~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     154 |                                       timeout_us);
         |                                       ~~~~~~~~~~~
   drivers/gpu/drm/panthor/panthor_pwr.c:26:29: note: expanded from macro 'PWR_ALL_CORES_MASK'
      26 | #define PWR_ALL_CORES_MASK              GENMASK(63, 0)
         |                                         ^
   include/linux/bits.h:51:24: note: expanded from macro 'GENMASK'
      51 | #define GENMASK(h, l)           GENMASK_TYPE(unsigned long, h, l)
         |                                 ^
   include/linux/bits.h:49:20: note: expanded from macro 'GENMASK_TYPE'
      49 |               type_max(t) >> (BITS_PER_TYPE(t) - 1 - (h)))))
         |                           ^
   drivers/gpu/drm/panthor/panthor_device.h:533:37: note: expanded from macro 'gpu_read64_poll_timeout'
     533 |         read_poll_timeout(gpu_read64, val, cond, delay_us, timeout_us, false,   \
         |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     534 |                           dev, reg)
         |                           ~~~~~~~~~
   include/linux/iopoll.h:145:36: note: expanded from macro 'read_poll_timeout'
     145 |         poll_timeout_us((val) = op(args), cond, sleep_us, timeout_us, sleep_before_read)
         |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/iopoll.h:49:7: note: expanded from macro 'poll_timeout_us'
      49 |                 if (cond) { \
         |                     ^~~~
   drivers/gpu/drm/panthor/panthor_pwr.c:260:13: warning: shift count >= width of type [-Wshift-count-overflow]
     259 |         ret = gpu_read64_poll_timeout(ptdev, PWR_STATUS, val,
         |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     260 |                                       !(PWR_STATUS_RETRACT_PENDING & val), 0,
         |                                       ~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     261 |                                       PWR_RETRACT_TIMEOUT_US);
         |                                       ~~~~~~~~~~~~~~~~~~~~~~~
   drivers/gpu/drm/panthor/panthor_regs.h:249:40: note: expanded from macro 'PWR_STATUS_RETRACT_PENDING'
     249 | #define   PWR_STATUS_RETRACT_PENDING                    BIT(43)
         |                                                         ^
   include/vdso/bits.h:7:26: note: expanded from macro 'BIT'
       7 | #define BIT(nr)                 (UL(1) << (nr))
         |                                        ^
   drivers/gpu/drm/panthor/panthor_device.h:533:37: note: expanded from macro 'gpu_read64_poll_timeout'
     533 |         read_poll_timeout(gpu_read64, val, cond, delay_us, timeout_us, false,   \
         |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     534 |                           dev, reg)
         |                           ~~~~~~~~~
   include/linux/iopoll.h:145:36: note: expanded from macro 'read_poll_timeout'
     145 |         poll_timeout_us((val) = op(args), cond, sleep_us, timeout_us, sleep_before_read)
         |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/iopoll.h:49:7: note: expanded from macro 'poll_timeout_us'
      49 |                 if (cond) { \
         |                     ^~~~
   2 warnings generated.


vim +153 drivers/gpu/drm/panthor/panthor_pwr.c

   144	
   145	static int panthor_pwr_domain_wait_transition(struct panthor_device *ptdev, u32 domain,
   146						      u32 timeout_us)
   147	{
   148		u32 pwrtrans_reg = get_domain_pwrtrans_reg(domain);
   149		u64 val;
   150		int ret = 0;
   151	
   152		ret = gpu_read64_poll_timeout(ptdev, pwrtrans_reg, val,
 > 153					      !(PWR_ALL_CORES_MASK & val), 100,
   154					      timeout_us);
   155		if (ret) {
   156			drm_err(&ptdev->base, "%s domain power in transition, pwrtrans(0x%llx)",
   157				get_domain_name(domain), val);
   158			return ret;
   159		}
   160	
   161		return 0;
   162	}
   163	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 06/10] drm/panthor: Implement L2 power on/off via PWR_CONTROL

[Steven Price]

On 14/10/2025 10:43, Karunika Choo wrote:
> This patch adds common helpers to issue power commands, poll
> transitions, and validate domain state, then wires them into the L2
> on/off paths.
> 
> The L2 power-on sequence now delegates control of the SHADER and TILER
> domains to the MCU when allowed, while the L2 itself is never delegated.
> On power-off, dependent domains beneath the L2 are checked, and if
> necessary, retracted and powered down to maintain proper domain
> ordering.
> 
> Signed-off-by: Karunika Choo <karunika.choo@arm.com>
> ---
>  drivers/gpu/drm/panthor/panthor_pwr.c | 390 +++++++++++++++++++++++++-
>  1 file changed, 388 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_pwr.c b/drivers/gpu/drm/panthor/panthor_pwr.c
> index d07ad5b7953a..594181aba847 100644
> --- a/drivers/gpu/drm/panthor/panthor_pwr.c
> +++ b/drivers/gpu/drm/panthor/panthor_pwr.c
> @@ -23,6 +23,14 @@
>  	 PWR_IRQ_COMMAND_NOT_ALLOWED | \
>  	 PWR_IRQ_COMMAND_INVALID)
>  
> +#define PWR_ALL_CORES_MASK		GENMASK(63, 0)

This isn't 32bit safe (I think that's what the kernel test robot is
complaining about). Use GENMASK_U64().

> +
> +#define PWR_DOMAIN_MAX_BITS		16
> +
> +#define PWR_TRANSITION_TIMEOUT_US	2000000
> +
> +#define PWR_RETRACT_TIMEOUT_US		2000
> +
>  /**
>   * struct panthor_pwr - PWR_CONTROL block management data.
>   */
> @@ -59,6 +67,302 @@ static void panthor_pwr_irq_handler(struct panthor_device *ptdev, u32 status)
>  }
>  PANTHOR_IRQ_HANDLER(pwr, PWR, panthor_pwr_irq_handler);
>  
> +static u64 panthor_pwr_read_status(struct panthor_device *ptdev)
> +{
> +	return gpu_read64(ptdev, PWR_STATUS);
> +}
> +
> +static void panthor_pwr_write_command(struct panthor_device *ptdev, u32 command, u64 args)
> +{
> +	if (args)
> +		gpu_write64(ptdev, PWR_CMDARG, args);
> +
> +	gpu_write(ptdev, PWR_COMMAND, command);
> +}
> +
> +static const char *get_domain_name(u8 domain)
> +{
> +	switch (domain) {
> +	case PWR_COMMAND_DOMAIN_L2:
> +		return "L2";
> +	case PWR_COMMAND_DOMAIN_TILER:
> +		return "Tiler";
> +	case PWR_COMMAND_DOMAIN_SHADER:
> +		return "Shader";
> +	case PWR_COMMAND_DOMAIN_BASE:
> +		return "Base";
> +	case PWR_COMMAND_DOMAIN_STACK:
> +		return "Stack";
> +	}
> +	return "Unknown";
> +}
> +
> +static u32 get_domain_base(u8 domain)
> +{
> +	switch (domain) {
> +	case PWR_COMMAND_DOMAIN_L2:
> +		return PWR_L2_PRESENT;
> +	case PWR_COMMAND_DOMAIN_TILER:
> +		return PWR_TILER_PRESENT;
> +	case PWR_COMMAND_DOMAIN_SHADER:
> +		return PWR_SHADER_PRESENT;
> +	case PWR_COMMAND_DOMAIN_BASE:
> +		return PWR_BASE_PRESENT;
> +	case PWR_COMMAND_DOMAIN_STACK:
> +		return PWR_STACK_PRESENT;
> +	}
> +	return 0;
> +}
> +
> +static u32 get_domain_ready_reg(u32 domain)
> +{
> +	return get_domain_base(domain) + (PWR_L2_READY - PWR_L2_PRESENT);
> +}
> +
> +static u32 get_domain_pwrtrans_reg(u32 domain)
> +{
> +	return get_domain_base(domain) + (PWR_L2_PWRTRANS - PWR_L2_PRESENT);
> +}
> +
> +static bool is_valid_domain(u32 domain)
> +{
> +	return get_domain_base(domain) != 0;
> +}
> +
> +static bool has_rtu(struct panthor_device *ptdev)
> +{
> +	return ptdev->gpu_info.gpu_features & GPU_FEATURES_RAY_TRAVERSAL;

Ok, I see why you added GPU_FEATURES_RAY_TRAVERSAL in a previous patch.
It might be ok then to just explain in the commit message why that
definition was included. Or you could move it to this patch where the
usage is.

> +}
> +
> +static u8 get_domain_subdomain(struct panthor_device *ptdev, u32 domain)
> +{
> +	if ((domain == PWR_COMMAND_DOMAIN_SHADER) && has_rtu(ptdev))
> +		return PWR_COMMAND_SUBDOMAIN_RTU;
> +
> +	return 0;
> +}
> +
> +static int panthor_pwr_domain_wait_transition(struct panthor_device *ptdev, u32 domain,
> +					      u32 timeout_us)
> +{
> +	u32 pwrtrans_reg = get_domain_pwrtrans_reg(domain);
> +	u64 val;
> +	int ret = 0;
> +
> +	ret = gpu_read64_poll_timeout(ptdev, pwrtrans_reg, val,
> +				      !(PWR_ALL_CORES_MASK & val), 100,
> +				      timeout_us);
> +	if (ret) {
> +		drm_err(&ptdev->base, "%s domain power in transition, pwrtrans(0x%llx)",
> +			get_domain_name(domain), val);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void panthor_pwr_info_show(struct panthor_device *ptdev)
> +{
> +	drm_info(&ptdev->base, "GPU_FEATURES:    0x%016llx", ptdev->gpu_info.gpu_features);
> +	drm_info(&ptdev->base, "PWR_STATUS:      0x%016llx", gpu_read64(ptdev, PWR_STATUS));
> +	drm_info(&ptdev->base, "L2_PRESENT:      0x%016llx", gpu_read64(ptdev, PWR_L2_PRESENT));
> +	drm_info(&ptdev->base, "L2_PWRTRANS:     0x%016llx", gpu_read64(ptdev, PWR_L2_PWRTRANS));
> +	drm_info(&ptdev->base, "L2_READY:        0x%016llx", gpu_read64(ptdev, PWR_L2_READY));
> +	drm_info(&ptdev->base, "TILER_PRESENT:   0x%016llx", gpu_read64(ptdev, PWR_TILER_PRESENT));
> +	drm_info(&ptdev->base, "TILER_PWRTRANS:  0x%016llx", gpu_read64(ptdev, PWR_TILER_PWRTRANS));
> +	drm_info(&ptdev->base, "TILER_READY:     0x%016llx", gpu_read64(ptdev, PWR_TILER_READY));
> +	drm_info(&ptdev->base, "SHADER_PRESENT:  0x%016llx", gpu_read64(ptdev, PWR_SHADER_PRESENT));
> +	drm_info(&ptdev->base, "SHADER_PWRTRANS: 0x%016llx",
> +		 gpu_read64(ptdev, PWR_SHADER_PWRTRANS));
> +	drm_info(&ptdev->base, "SHADER_READY:    0x%016llx", gpu_read64(ptdev, PWR_SHADER_READY));

Minor comments:

 * Put SHADER_PWRTRANS on one line - wrapping that differently just
makes it harder to read.

 * Using the cached value for GPU_FEATURES is likely to lead to
confusion. You're not using the cached _PRESENT values. This is
debugging for when things have gone wrong, so if the FEATURES value has
changed (e.g. the bus to the GPU is misbehaving) that would be a good
thing to immediately see.

 * It might be worth a comment saying these are printed for debugging -
I was about to complain this was too verbose until I spotted that the
only call sites are when something has gone wrong.

> +}
> +
> +static int panthor_pwr_domain_transition(struct panthor_device *ptdev, u32 cmd, u32 domain,
> +					 u64 mask, u32 timeout_us)
> +{
> +	u32 ready_reg = get_domain_ready_reg(domain);
> +	u32 pwr_cmd = PWR_COMMAND_DEF(cmd, domain, get_domain_subdomain(ptdev, domain));
> +	u64 expected_val = cmd == PWR_COMMAND_POWER_DOWN ? 0 : mask;

Yuk. Can we have a readable 'if' instead please? ;)

> +	u64 val;
> +	int ret = 0;
> +
> +	if (!is_valid_domain(domain))
> +		return -EINVAL;

Do we ever expect this to happen? I'd suggest a WARN_ON() here as this
looks like a programming mistake if it happens.

> +
> +	if (cmd != PWR_COMMAND_POWER_DOWN && cmd != PWR_COMMAND_POWER_UP) {
> +		drm_err(&ptdev->base, "Invalid power domain transition command (0x%x)", cmd);
> +		return -EINVAL;
> +	}

Indeed combining with this 'if' we can have something like:

	switch (cmd) {
	case PWR_COMMAND_POWER_DOWN:
		expected_val = 0;
		break;
	case PWR_COMMAND_POWER_UP:
		expected_val = mask;
		break;
	default:
		drm_err(...);
		return -EINVAL;
	}

which IMHO is much easier to read.

> +
> +	ret = panthor_pwr_domain_wait_transition(ptdev, domain, timeout_us);
> +	if (ret)
> +		return ret;
> +
> +	/* domain already in target state, return early */
> +	if ((gpu_read64(ptdev, ready_reg) & mask) == expected_val)
> +		return 0;
> +
> +	panthor_pwr_write_command(ptdev, pwr_cmd, mask);
> +
> +	ret = gpu_read64_poll_timeout(ptdev, ready_reg, val, (mask & val) == expected_val, 100,
> +				      timeout_us);
> +	if (ret) {
> +		drm_err(&ptdev->base, "timeout waiting on %s power %s, cmd(0x%x), arg(0x%llx)",
> +			get_domain_name(domain), cmd == PWR_COMMAND_POWER_DOWN ? "down" : "up",

I'd avoid this sort of ternary expression. If someone updates this
function with a third action they're going to forget to update this. So
either drop the pretty "up/down" text, assign the direction string in a
switch above, or provide a helper function which decodes the command.

Or if you really want to stick with something like this then
str_down_up(cmd == PWR_COMMAND_POWER_DOWN) which at least helps the
linker with deduping.

> +			pwr_cmd, mask);
> +		panthor_pwr_info_show(ptdev);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +#define panthor_pwr_domain_power_off(__ptdev, __domain, __mask, __timeout_us)            \
> +	panthor_pwr_domain_transition(__ptdev, PWR_COMMAND_POWER_DOWN, __domain, __mask, \
> +				      __timeout_us)
> +
> +#define panthor_pwr_domain_power_on(__ptdev, __domain, __mask, __timeout_us) \
> +	panthor_pwr_domain_transition(__ptdev, PWR_COMMAND_POWER_UP, __domain, __mask, __timeout_us)
> +
> +/**
> + * retract_domain() - Retract control of a domain from MCU
> + * @ptdev: Device.
> + * @domain: Domain to retract the control
> + *
> + * Retracting L2 domain is not expected since it won't be delegated.
> + *
> + * Return: 0 on success or retracted already.
> + *         -EPERM if domain is L2.
> + *         A negative error code otherwise.
> + */
> +static int retract_domain(struct panthor_device *ptdev, u32 domain)
> +{
> +	const u32 pwr_cmd = PWR_COMMAND_DEF(PWR_COMMAND_RETRACT, domain, 0);
> +	const u32 pwr_status_reg = panthor_pwr_read_status(ptdev);
> +	const u32 delegated_mask = PWR_STATUS_DOMAIN_DELEGATED(domain);
> +	const u32 allow_mask = PWR_STATUS_DOMAIN_ALLOWED(domain);
> +	u64 val;
> +	int ret;
> +
> +	if (WARN_ON(domain == PWR_COMMAND_DOMAIN_L2))
> +		return -EINVAL;

The kerneldoc says -EPERM is the domain is L2.

> +
> +	if (WARN_ON(pwr_status_reg & PWR_STATUS_DOMAIN_DELEGATED(PWR_COMMAND_DOMAIN_L2)))
> +		return -EPERM;

But this is testing for the L2 being delegated? I'm not sure how this
could happen, but maybe this should be WARN_ON_ONCE() as it doesn't seem
like something that we're going to manage to resolve.

> +
> +	if (!(pwr_status_reg & delegated_mask)) {
> +		drm_dbg(&ptdev->base, "%s domain already retracted",
> +			get_domain_name(domain));
> +		return 0;
> +	}
> +
> +	ret = gpu_read64_poll_timeout(ptdev, PWR_STATUS, val,
> +				      !(PWR_STATUS_RETRACT_PENDING & val), 0,
> +				      PWR_RETRACT_TIMEOUT_US);
> +	if (ret) {
> +		drm_err(&ptdev->base, "%s domain retract pending",
> +			get_domain_name(domain));
> +		return ret;
> +	}
> +
> +	panthor_pwr_write_command(ptdev, pwr_cmd, 0);
> +
> +	/*
> +	 * On successful retraction
> +	 * allow-flag will be set with delegated-flag being cleared.
> +	 */
> +	ret = gpu_read64_poll_timeout(ptdev, PWR_STATUS, val,
> +				      ((delegated_mask | allow_mask) & val) == allow_mask,
> +				      10, PWR_TRANSITION_TIMEOUT_US);
> +	if (ret) {
> +		drm_err(&ptdev->base, "Retracting %s domain timeout, cmd(0x%x)",
> +			get_domain_name(domain), pwr_cmd);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * delegate_domain() - Delegate control of a domain to MCU
> + * @ptdev: Device.
> + * @domain: Domain to delegate the control
> + *
> + * Delegating L2 domain is prohibited.
> + *
> + * Return:
> + * *       0 on success or delegated already.
> + * *       -EPERM if domain is L2.
> + * *       A negative error code otherwise.
> + */
> +static int delegate_domain(struct panthor_device *ptdev, u32 domain)
> +{
> +	const u32 pwr_cmd = PWR_COMMAND_DEF(PWR_COMMAND_DELEGATE, domain, 0);
> +	const u32 pwr_status_reg = panthor_pwr_read_status(ptdev);
> +	const u32 pwrtrans_reg = get_domain_pwrtrans_reg(domain);
> +	const u32 allow_mask = PWR_STATUS_DOMAIN_ALLOWED(domain);
> +	const u32 delegated_mask = PWR_STATUS_DOMAIN_DELEGATED(domain);
> +	u64 val;
> +	int ret;
> +
> +	if (WARN_ON_ONCE(domain == PWR_COMMAND_DOMAIN_L2))
> +		return -EINVAL;

Why WARN_ON_ONCE? This is a programming error. Return code doesn't match
kerneldoc.

> +
> +	if (pwr_status_reg & delegated_mask) {
> +		drm_dbg(&ptdev->base, "%s domain already delegated",
> +			get_domain_name(domain));
> +		return 0;
> +	}
> +
> +	/* Check if the command is allowed before delegating. */
> +	if (unlikely(!(pwr_status_reg & allow_mask))) {
> +		drm_warn(&ptdev->base, "Delegating %s domain not allowed",
> +			 get_domain_name(domain));
> +		return -EPERM;
> +	}

We usually only use unlikely() on hot paths.

> +
> +	if (unlikely(gpu_read64(ptdev, pwrtrans_reg))) {
> +		drm_warn(&ptdev->base, "%s domain power in transition",
> +			 get_domain_name(domain));
> +		return -EPERM;

-EPERM seems wrong, -EBUSY perhaps?

> +	}
> +
> +	panthor_pwr_write_command(ptdev, pwr_cmd, 0);
> +
> +	/*
> +	 * On successful delegation
> +	 * allow-flag will be cleared with delegated-flag being set.
> +	 */
> +	ret = gpu_read64_poll_timeout(ptdev, PWR_STATUS, val,
> +				      ((delegated_mask | allow_mask) & val) == delegated_mask,
> +				      10, PWR_TRANSITION_TIMEOUT_US);
> +	if (ret) {
> +		drm_err(&ptdev->base, "Delegating %s domain timeout, cmd(0x%x)",
> +			get_domain_name(domain), pwr_cmd);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int panthor_pwr_delegate_domains(struct panthor_device *ptdev)
> +{
> +	int ret;
> +
> +	if (!ptdev->pwr)
> +		return 0;
> +
> +	ret = delegate_domain(ptdev, PWR_COMMAND_DOMAIN_SHADER);
> +	if (ret)
> +		return ret;
> +
> +	ret = delegate_domain(ptdev, PWR_COMMAND_DOMAIN_TILER);
> +	if (ret)
> +		return ret;

Given nothing actually handles these errors currently, do we have any
problems if the shader cores are delegated, but the tiler cores fail? Do
we need to unwind?

Generally the error handling seems a bit shaky here.

> +
> +	return 0;
> +}
> +
>  void panthor_pwr_unplug(struct panthor_device *ptdev)
>  {
>  	unsigned long flags;
> @@ -110,12 +414,94 @@ int panthor_pwr_reset_soft(struct panthor_device *ptdev)
>  
>  int panthor_pwr_l2_power_off(struct panthor_device *ptdev)
>  {
> -	return 0;
> +	const u32 l2_allow_mask = PWR_STATUS_DOMAIN_ALLOWED(PWR_COMMAND_DOMAIN_L2);
> +	const u32 l2_pwrtrans_reg = get_domain_pwrtrans_reg(PWR_COMMAND_DOMAIN_L2);
> +	unsigned long child_domain_mask = {
> +		BIT(PWR_COMMAND_DOMAIN_SHADER) |
> +		BIT(PWR_COMMAND_DOMAIN_TILER)
> +	};

What's with the braces?

> +	u32 pwr_status, child_domain;
> +	int ret = 0;
> +
> +	if (unlikely(!ptdev->pwr))
> +		return -ENODEV;
> +
> +	pwr_status = panthor_pwr_read_status(ptdev);
> +
> +	/* Abort if L2 power off constraints are not satisfied */
> +	if (!(pwr_status & l2_allow_mask)) {
> +		drm_warn(&ptdev->base, "Power off L2 domain not allowed");
> +		return 0;
> +	}
> +
> +	if (gpu_read64(ptdev, l2_pwrtrans_reg)) {
> +		drm_warn(&ptdev->base, "L2 Power in transition");
> +		return 0;

Isn't this an error? How are we handling this? Shouldn't we attempt to
wait for the transition to complete?

> +	}
> +
> +	/* It is expected that when halting the MCU, it would power down its
> +	 * delegated domains. However, an unresponsive or hung MCU may not do
> +	 * so, which is why we need to check and retract the domains back into
> +	 * host control to be powered down before powering down the L2.
> +	 */
> +	for_each_set_bit(child_domain, &child_domain_mask, PWR_DOMAIN_MAX_BITS) {

This feels overkill for two bits... Can we not just put this in a helper
function and call it for both domains?

> +		u64 domain_ready = gpu_read64(ptdev, get_domain_ready_reg(child_domain));
> +
> +		if (domain_ready && (pwr_status & PWR_STATUS_DOMAIN_DELEGATED(child_domain))) {
> +			drm_warn(&ptdev->base,
> +				 "L2 power off: Delegated %s domain not powered down by MCU",
> +				 get_domain_name(child_domain));
> +			ret = retract_domain(ptdev, child_domain);
> +			if (ret) {
> +				drm_err(&ptdev->base, "Failed to retract %s domain",
> +					get_domain_name(child_domain));
> +				panthor_pwr_info_show(ptdev);
> +				return ret;
> +			}
> +		}
> +
> +		ret = panthor_pwr_domain_power_off(ptdev, child_domain, domain_ready,
> +						   PWR_TRANSITION_TIMEOUT_US);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return panthor_pwr_domain_power_off(ptdev, PWR_COMMAND_DOMAIN_L2,
> +					    ptdev->gpu_info.l2_present,
> +					    PWR_TRANSITION_TIMEOUT_US);

Does this implicitly 'retract' the shader/tiler power domains? If so I
think it's worth a comment. Otherwise it looks like we don't actually
know the status of whether the shader/tiler power domains are retracted
or not.

Thanks,
Steve

>  }
>  
>  int panthor_pwr_l2_power_on(struct panthor_device *ptdev)
>  {
> -	return 0;
> +	const u32 pwr_status = panthor_pwr_read_status(ptdev);
> +	const u32 l2_allow_mask = PWR_STATUS_DOMAIN_ALLOWED(PWR_COMMAND_DOMAIN_L2);
> +	const u32 l2_pwrtrans_reg = get_domain_pwrtrans_reg(PWR_COMMAND_DOMAIN_L2);
> +	int ret;
> +
> +	if (!ptdev->pwr)
> +		return 0;
> +
> +	if ((pwr_status & l2_allow_mask) == 0) {
> +		drm_warn(&ptdev->base, "Power on L2 domain not allowed");
> +		return -EPERM;
> +	}
> +
> +	if (gpu_read64(ptdev, l2_pwrtrans_reg)) {
> +		drm_warn(&ptdev->base, "L2 Power is in transition");
> +		return -EINVAL;
> +	}
> +
> +	ret = panthor_pwr_domain_power_on(ptdev, PWR_COMMAND_DOMAIN_L2,
> +					  ptdev->gpu_info.l2_present,
> +					  PWR_TRANSITION_TIMEOUT_US);
> +	if (ret)
> +		return ret;
> +
> +	/* Delegate control of the shader and tiler power domains to the MCU as
> +	 * it can better manage which shader/tiler cores need to be powered up
> +	 * or can be powered down based on currently running jobs.
> +	 */
> +	return panthor_pwr_delegate_domains(ptdev);
>  }
>  
>  void panthor_pwr_suspend(struct panthor_device *ptdev)

Re: [PATCH v1 06/10] drm/panthor: Implement L2 power on/off via PWR_CONTROL

[Karunika Choo]

On 20/10/2025 11:50, Steven Price wrote:
> On 14/10/2025 10:43, Karunika Choo wrote:
>> This patch adds common helpers to issue power commands, poll
>> transitions, and validate domain state, then wires them into the L2
>> on/off paths.
>>
>> The L2 power-on sequence now delegates control of the SHADER and TILER
>> domains to the MCU when allowed, while the L2 itself is never delegated.
>> On power-off, dependent domains beneath the L2 are checked, and if
>> necessary, retracted and powered down to maintain proper domain
>> ordering.
>>
>> Signed-off-by: Karunika Choo <karunika.choo@arm.com>
>> ---
>>  drivers/gpu/drm/panthor/panthor_pwr.c | 390 +++++++++++++++++++++++++-
>>  1 file changed, 388 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panthor/panthor_pwr.c b/drivers/gpu/drm/panthor/panthor_pwr.c
>> index d07ad5b7953a..594181aba847 100644
>> --- a/drivers/gpu/drm/panthor/panthor_pwr.c
>> +++ b/drivers/gpu/drm/panthor/panthor_pwr.c
>> @@ -23,6 +23,14 @@
>>  	 PWR_IRQ_COMMAND_NOT_ALLOWED | \
>>  	 PWR_IRQ_COMMAND_INVALID)
>>  
>> +#define PWR_ALL_CORES_MASK		GENMASK(63, 0)
> 
> This isn't 32bit safe (I think that's what the kernel test robot is
> complaining about). Use GENMASK_U64().
> 
>> +
>> +#define PWR_DOMAIN_MAX_BITS		16
>> +
>> +#define PWR_TRANSITION_TIMEOUT_US	2000000
>> +
>> +#define PWR_RETRACT_TIMEOUT_US		2000
>> +
>>  /**
>>   * struct panthor_pwr - PWR_CONTROL block management data.
>>   */
>> @@ -59,6 +67,302 @@ static void panthor_pwr_irq_handler(struct panthor_device *ptdev, u32 status)
>>  }
>>  PANTHOR_IRQ_HANDLER(pwr, PWR, panthor_pwr_irq_handler);
>>  
>> +static u64 panthor_pwr_read_status(struct panthor_device *ptdev)
>> +{
>> +	return gpu_read64(ptdev, PWR_STATUS);
>> +}
>> +
>> +static void panthor_pwr_write_command(struct panthor_device *ptdev, u32 command, u64 args)
>> +{
>> +	if (args)
>> +		gpu_write64(ptdev, PWR_CMDARG, args);
>> +
>> +	gpu_write(ptdev, PWR_COMMAND, command);
>> +}
>> +
>> +static const char *get_domain_name(u8 domain)
>> +{
>> +	switch (domain) {
>> +	case PWR_COMMAND_DOMAIN_L2:
>> +		return "L2";
>> +	case PWR_COMMAND_DOMAIN_TILER:
>> +		return "Tiler";
>> +	case PWR_COMMAND_DOMAIN_SHADER:
>> +		return "Shader";
>> +	case PWR_COMMAND_DOMAIN_BASE:
>> +		return "Base";
>> +	case PWR_COMMAND_DOMAIN_STACK:
>> +		return "Stack";
>> +	}
>> +	return "Unknown";
>> +}
>> +
>> +static u32 get_domain_base(u8 domain)
>> +{
>> +	switch (domain) {
>> +	case PWR_COMMAND_DOMAIN_L2:
>> +		return PWR_L2_PRESENT;
>> +	case PWR_COMMAND_DOMAIN_TILER:
>> +		return PWR_TILER_PRESENT;
>> +	case PWR_COMMAND_DOMAIN_SHADER:
>> +		return PWR_SHADER_PRESENT;
>> +	case PWR_COMMAND_DOMAIN_BASE:
>> +		return PWR_BASE_PRESENT;
>> +	case PWR_COMMAND_DOMAIN_STACK:
>> +		return PWR_STACK_PRESENT;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static u32 get_domain_ready_reg(u32 domain)
>> +{
>> +	return get_domain_base(domain) + (PWR_L2_READY - PWR_L2_PRESENT);
>> +}
>> +
>> +static u32 get_domain_pwrtrans_reg(u32 domain)
>> +{
>> +	return get_domain_base(domain) + (PWR_L2_PWRTRANS - PWR_L2_PRESENT);
>> +}
>> +
>> +static bool is_valid_domain(u32 domain)
>> +{
>> +	return get_domain_base(domain) != 0;
>> +}
>> +
>> +static bool has_rtu(struct panthor_device *ptdev)
>> +{
>> +	return ptdev->gpu_info.gpu_features & GPU_FEATURES_RAY_TRAVERSAL;
> 
> Ok, I see why you added GPU_FEATURES_RAY_TRAVERSAL in a previous patch.
> It might be ok then to just explain in the commit message why that
> definition was included. Or you could move it to this patch where the
> usage is.
> 
>> +}
>> +
>> +static u8 get_domain_subdomain(struct panthor_device *ptdev, u32 domain)
>> +{
>> +	if ((domain == PWR_COMMAND_DOMAIN_SHADER) && has_rtu(ptdev))
>> +		return PWR_COMMAND_SUBDOMAIN_RTU;
>> +
>> +	return 0;
>> +}
>> +
>> +static int panthor_pwr_domain_wait_transition(struct panthor_device *ptdev, u32 domain,
>> +					      u32 timeout_us)
>> +{
>> +	u32 pwrtrans_reg = get_domain_pwrtrans_reg(domain);
>> +	u64 val;
>> +	int ret = 0;
>> +
>> +	ret = gpu_read64_poll_timeout(ptdev, pwrtrans_reg, val,
>> +				      !(PWR_ALL_CORES_MASK & val), 100,
>> +				      timeout_us);
>> +	if (ret) {
>> +		drm_err(&ptdev->base, "%s domain power in transition, pwrtrans(0x%llx)",
>> +			get_domain_name(domain), val);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void panthor_pwr_info_show(struct panthor_device *ptdev)
>> +{
>> +	drm_info(&ptdev->base, "GPU_FEATURES:    0x%016llx", ptdev->gpu_info.gpu_features);
>> +	drm_info(&ptdev->base, "PWR_STATUS:      0x%016llx", gpu_read64(ptdev, PWR_STATUS));
>> +	drm_info(&ptdev->base, "L2_PRESENT:      0x%016llx", gpu_read64(ptdev, PWR_L2_PRESENT));
>> +	drm_info(&ptdev->base, "L2_PWRTRANS:     0x%016llx", gpu_read64(ptdev, PWR_L2_PWRTRANS));
>> +	drm_info(&ptdev->base, "L2_READY:        0x%016llx", gpu_read64(ptdev, PWR_L2_READY));
>> +	drm_info(&ptdev->base, "TILER_PRESENT:   0x%016llx", gpu_read64(ptdev, PWR_TILER_PRESENT));
>> +	drm_info(&ptdev->base, "TILER_PWRTRANS:  0x%016llx", gpu_read64(ptdev, PWR_TILER_PWRTRANS));
>> +	drm_info(&ptdev->base, "TILER_READY:     0x%016llx", gpu_read64(ptdev, PWR_TILER_READY));
>> +	drm_info(&ptdev->base, "SHADER_PRESENT:  0x%016llx", gpu_read64(ptdev, PWR_SHADER_PRESENT));
>> +	drm_info(&ptdev->base, "SHADER_PWRTRANS: 0x%016llx",
>> +		 gpu_read64(ptdev, PWR_SHADER_PWRTRANS));
>> +	drm_info(&ptdev->base, "SHADER_READY:    0x%016llx", gpu_read64(ptdev, PWR_SHADER_READY));
> 
> Minor comments:
> 
>  * Put SHADER_PWRTRANS on one line - wrapping that differently just
> makes it harder to read.
> 
>  * Using the cached value for GPU_FEATURES is likely to lead to
> confusion. You're not using the cached _PRESENT values. This is
> debugging for when things have gone wrong, so if the FEATURES value has
> changed (e.g. the bus to the GPU is misbehaving) that would be a good
> thing to immediately see.
> 
>  * It might be worth a comment saying these are printed for debugging -
> I was about to complain this was too verbose until I spotted that the
> only call sites are when something has gone wrong.
> 

Thanks, I will address these and rename the function to
panthor_pwr_debug_info_show() to reflect its nature and use case.

>> +}
>> +
>> +static int panthor_pwr_domain_transition(struct panthor_device *ptdev, u32 cmd, u32 domain,
>> +					 u64 mask, u32 timeout_us)
>> +{
>> +	u32 ready_reg = get_domain_ready_reg(domain);
>> +	u32 pwr_cmd = PWR_COMMAND_DEF(cmd, domain, get_domain_subdomain(ptdev, domain));
>> +	u64 expected_val = cmd == PWR_COMMAND_POWER_DOWN ? 0 : mask;
> 
> Yuk. Can we have a readable 'if' instead please? ;)
> 
>> +	u64 val;
>> +	int ret = 0;
>> +
>> +	if (!is_valid_domain(domain))
>> +		return -EINVAL;
> 
> Do we ever expect this to happen? I'd suggest a WARN_ON() here as this
> looks like a programming mistake if it happens.
> 
>> +
>> +	if (cmd != PWR_COMMAND_POWER_DOWN && cmd != PWR_COMMAND_POWER_UP) {
>> +		drm_err(&ptdev->base, "Invalid power domain transition command (0x%x)", cmd);
>> +		return -EINVAL;
>> +	}
> 
> Indeed combining with this 'if' we can have something like:
> 
> 	switch (cmd) {
> 	case PWR_COMMAND_POWER_DOWN:
> 		expected_val = 0;
> 		break;
> 	case PWR_COMMAND_POWER_UP:
> 		expected_val = mask;
> 		break;
> 	default:
> 		drm_err(...);
> 		return -EINVAL;
> 	}
> 
> which IMHO is much easier to read.
> 

I will clean this up in v2. Thanks.

>> +
>> +	ret = panthor_pwr_domain_wait_transition(ptdev, domain, timeout_us);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* domain already in target state, return early */
>> +	if ((gpu_read64(ptdev, ready_reg) & mask) == expected_val)
>> +		return 0;
>> +
>> +	panthor_pwr_write_command(ptdev, pwr_cmd, mask);
>> +
>> +	ret = gpu_read64_poll_timeout(ptdev, ready_reg, val, (mask & val) == expected_val, 100,
>> +				      timeout_us);
>> +	if (ret) {
>> +		drm_err(&ptdev->base, "timeout waiting on %s power %s, cmd(0x%x), arg(0x%llx)",
>> +			get_domain_name(domain), cmd == PWR_COMMAND_POWER_DOWN ? "down" : "up",
> 
> I'd avoid this sort of ternary expression. If someone updates this
> function with a third action they're going to forget to update this. So
> either drop the pretty "up/down" text, assign the direction string in a
> switch above, or provide a helper function which decodes the command.
> 
> Or if you really want to stick with something like this then
> str_down_up(cmd == PWR_COMMAND_POWER_DOWN) which at least helps the
> linker with deduping.
> 

Will remove the "up" | "down" entirely as we can decode that from the
printed command.

>> +			pwr_cmd, mask);
>> +		panthor_pwr_info_show(ptdev);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +#define panthor_pwr_domain_power_off(__ptdev, __domain, __mask, __timeout_us)            \
>> +	panthor_pwr_domain_transition(__ptdev, PWR_COMMAND_POWER_DOWN, __domain, __mask, \
>> +				      __timeout_us)
>> +
>> +#define panthor_pwr_domain_power_on(__ptdev, __domain, __mask, __timeout_us) \
>> +	panthor_pwr_domain_transition(__ptdev, PWR_COMMAND_POWER_UP, __domain, __mask, __timeout_us)
>> +
>> +/**
>> + * retract_domain() - Retract control of a domain from MCU
>> + * @ptdev: Device.
>> + * @domain: Domain to retract the control
>> + *
>> + * Retracting L2 domain is not expected since it won't be delegated.
>> + *
>> + * Return: 0 on success or retracted already.
>> + *         -EPERM if domain is L2.
>> + *         A negative error code otherwise.
>> + */
>> +static int retract_domain(struct panthor_device *ptdev, u32 domain)
>> +{
>> +	const u32 pwr_cmd = PWR_COMMAND_DEF(PWR_COMMAND_RETRACT, domain, 0);
>> +	const u32 pwr_status_reg = panthor_pwr_read_status(ptdev);
>> +	const u32 delegated_mask = PWR_STATUS_DOMAIN_DELEGATED(domain);
>> +	const u32 allow_mask = PWR_STATUS_DOMAIN_ALLOWED(domain);
>> +	u64 val;
>> +	int ret;
>> +
>> +	if (WARN_ON(domain == PWR_COMMAND_DOMAIN_L2))
>> +		return -EINVAL;
> 
> The kerneldoc says -EPERM is the domain is L2.
> 
>> +
>> +	if (WARN_ON(pwr_status_reg & PWR_STATUS_DOMAIN_DELEGATED(PWR_COMMAND_DOMAIN_L2)))
>> +		return -EPERM;
> 
> But this is testing for the L2 being delegated? I'm not sure how this
> could happen, but maybe this should be WARN_ON_ONCE() as it doesn't seem
> like something that we're going to manage to resolve.
> 
>> +
>> +	if (!(pwr_status_reg & delegated_mask)) {
>> +		drm_dbg(&ptdev->base, "%s domain already retracted",
>> +			get_domain_name(domain));
>> +		return 0;
>> +	}
>> +
>> +	ret = gpu_read64_poll_timeout(ptdev, PWR_STATUS, val,
>> +				      !(PWR_STATUS_RETRACT_PENDING & val), 0,
>> +				      PWR_RETRACT_TIMEOUT_US);
>> +	if (ret) {
>> +		drm_err(&ptdev->base, "%s domain retract pending",
>> +			get_domain_name(domain));
>> +		return ret;
>> +	}
>> +
>> +	panthor_pwr_write_command(ptdev, pwr_cmd, 0);
>> +
>> +	/*
>> +	 * On successful retraction
>> +	 * allow-flag will be set with delegated-flag being cleared.
>> +	 */
>> +	ret = gpu_read64_poll_timeout(ptdev, PWR_STATUS, val,
>> +				      ((delegated_mask | allow_mask) & val) == allow_mask,
>> +				      10, PWR_TRANSITION_TIMEOUT_US);
>> +	if (ret) {
>> +		drm_err(&ptdev->base, "Retracting %s domain timeout, cmd(0x%x)",
>> +			get_domain_name(domain), pwr_cmd);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>> + * delegate_domain() - Delegate control of a domain to MCU
>> + * @ptdev: Device.
>> + * @domain: Domain to delegate the control
>> + *
>> + * Delegating L2 domain is prohibited.
>> + *
>> + * Return:
>> + * *       0 on success or delegated already.
>> + * *       -EPERM if domain is L2.
>> + * *       A negative error code otherwise.
>> + */
>> +static int delegate_domain(struct panthor_device *ptdev, u32 domain)
>> +{
>> +	const u32 pwr_cmd = PWR_COMMAND_DEF(PWR_COMMAND_DELEGATE, domain, 0);
>> +	const u32 pwr_status_reg = panthor_pwr_read_status(ptdev);
>> +	const u32 pwrtrans_reg = get_domain_pwrtrans_reg(domain);
>> +	const u32 allow_mask = PWR_STATUS_DOMAIN_ALLOWED(domain);
>> +	const u32 delegated_mask = PWR_STATUS_DOMAIN_DELEGATED(domain);
>> +	u64 val;
>> +	int ret;
>> +
>> +	if (WARN_ON_ONCE(domain == PWR_COMMAND_DOMAIN_L2))
>> +		return -EINVAL;
> 
> Why WARN_ON_ONCE? This is a programming error. Return code doesn't match
> kerneldoc.
> 
>> +
>> +	if (pwr_status_reg & delegated_mask) {
>> +		drm_dbg(&ptdev->base, "%s domain already delegated",
>> +			get_domain_name(domain));
>> +		return 0;
>> +	}
>> +
>> +	/* Check if the command is allowed before delegating. */
>> +	if (unlikely(!(pwr_status_reg & allow_mask))) {
>> +		drm_warn(&ptdev->base, "Delegating %s domain not allowed",
>> +			 get_domain_name(domain));
>> +		return -EPERM;
>> +	}
> 
> We usually only use unlikely() on hot paths.
> 
>> +
>> +	if (unlikely(gpu_read64(ptdev, pwrtrans_reg))) {
>> +		drm_warn(&ptdev->base, "%s domain power in transition",
>> +			 get_domain_name(domain));
>> +		return -EPERM;
> 
> -EPERM seems wrong, -EBUSY perhaps?
> 
>> +	}
>> +
>> +	panthor_pwr_write_command(ptdev, pwr_cmd, 0);
>> +
>> +	/*
>> +	 * On successful delegation
>> +	 * allow-flag will be cleared with delegated-flag being set.
>> +	 */
>> +	ret = gpu_read64_poll_timeout(ptdev, PWR_STATUS, val,
>> +				      ((delegated_mask | allow_mask) & val) == delegated_mask,
>> +				      10, PWR_TRANSITION_TIMEOUT_US);
>> +	if (ret) {
>> +		drm_err(&ptdev->base, "Delegating %s domain timeout, cmd(0x%x)",
>> +			get_domain_name(domain), pwr_cmd);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int panthor_pwr_delegate_domains(struct panthor_device *ptdev)
>> +{
>> +	int ret;
>> +
>> +	if (!ptdev->pwr)
>> +		return 0;
>> +
>> +	ret = delegate_domain(ptdev, PWR_COMMAND_DOMAIN_SHADER);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = delegate_domain(ptdev, PWR_COMMAND_DOMAIN_TILER);
>> +	if (ret)
>> +		return ret;
> 
> Given nothing actually handles these errors currently, do we have any
> problems if the shader cores are delegated, but the tiler cores fail? Do
> we need to unwind?
> 
> Generally the error handling seems a bit shaky here.
> 

Will update this to unwind on the second error.

>> +
>> +	return 0;
>> +}
>> +
>>  void panthor_pwr_unplug(struct panthor_device *ptdev)
>>  {
>>  	unsigned long flags;
>> @@ -110,12 +414,94 @@ int panthor_pwr_reset_soft(struct panthor_device *ptdev)
>>  
>>  int panthor_pwr_l2_power_off(struct panthor_device *ptdev)
>>  {
>> -	return 0;
>> +	const u32 l2_allow_mask = PWR_STATUS_DOMAIN_ALLOWED(PWR_COMMAND_DOMAIN_L2);
>> +	const u32 l2_pwrtrans_reg = get_domain_pwrtrans_reg(PWR_COMMAND_DOMAIN_L2);
>> +	unsigned long child_domain_mask = {
>> +		BIT(PWR_COMMAND_DOMAIN_SHADER) |
>> +		BIT(PWR_COMMAND_DOMAIN_TILER)
>> +	};
> 
> What's with the braces?
> 
>> +	u32 pwr_status, child_domain;
>> +	int ret = 0;
>> +
>> +	if (unlikely(!ptdev->pwr))
>> +		return -ENODEV;
>> +
>> +	pwr_status = panthor_pwr_read_status(ptdev);
>> +
>> +	/* Abort if L2 power off constraints are not satisfied */
>> +	if (!(pwr_status & l2_allow_mask)) {
>> +		drm_warn(&ptdev->base, "Power off L2 domain not allowed");
>> +		return 0;
>> +	}
>> +
>> +	if (gpu_read64(ptdev, l2_pwrtrans_reg)) {
>> +		drm_warn(&ptdev->base, "L2 Power in transition");
>> +		return 0;
> 
> Isn't this an error? How are we handling this? Shouldn't we attempt to
> wait for the transition to complete?
> 
>> +	}
>> +
>> +	/* It is expected that when halting the MCU, it would power down its
>> +	 * delegated domains. However, an unresponsive or hung MCU may not do
>> +	 * so, which is why we need to check and retract the domains back into
>> +	 * host control to be powered down before powering down the L2.
>> +	 */
>> +	for_each_set_bit(child_domain, &child_domain_mask, PWR_DOMAIN_MAX_BITS) {
> 
> This feels overkill for two bits... Can we not just put this in a helper
> function and call it for both domains?
> 

Will do so in v2.

>> +		u64 domain_ready = gpu_read64(ptdev, get_domain_ready_reg(child_domain));
>> +
>> +		if (domain_ready && (pwr_status & PWR_STATUS_DOMAIN_DELEGATED(child_domain))) {
>> +			drm_warn(&ptdev->base,
>> +				 "L2 power off: Delegated %s domain not powered down by MCU",
>> +				 get_domain_name(child_domain));
>> +			ret = retract_domain(ptdev, child_domain);
>> +			if (ret) {
>> +				drm_err(&ptdev->base, "Failed to retract %s domain",
>> +					get_domain_name(child_domain));
>> +				panthor_pwr_info_show(ptdev);
>> +				return ret;
>> +			}
>> +		}
>> +
>> +		ret = panthor_pwr_domain_power_off(ptdev, child_domain, domain_ready,
>> +						   PWR_TRANSITION_TIMEOUT_US);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return panthor_pwr_domain_power_off(ptdev, PWR_COMMAND_DOMAIN_L2,
>> +					    ptdev->gpu_info.l2_present,
>> +					    PWR_TRANSITION_TIMEOUT_US);
> 
> Does this implicitly 'retract' the shader/tiler power domains? If so I
> think it's worth a comment. Otherwise it looks like we don't actually
> know the status of whether the shader/tiler power domains are retracted
> or not.
> 

panthor_pwr_l2_power_off() will only retract the shader/tiler domains if
they have not been powered down by the MCU. In cases where the MCU did
power down these child domains, delegate_domain() will exit early as
they would already be delegated. I understand the ambiguity here,
hopefully it is somewhat acceptable.

Kind regards,
Karunika Choo

> Thanks,
> Steve
> 
>>  }
>>  
>>  int panthor_pwr_l2_power_on(struct panthor_device *ptdev)
>>  {
>> -	return 0;
>> +	const u32 pwr_status = panthor_pwr_read_status(ptdev);
>> +	const u32 l2_allow_mask = PWR_STATUS_DOMAIN_ALLOWED(PWR_COMMAND_DOMAIN_L2);
>> +	const u32 l2_pwrtrans_reg = get_domain_pwrtrans_reg(PWR_COMMAND_DOMAIN_L2);
>> +	int ret;
>> +
>> +	if (!ptdev->pwr)
>> +		return 0;
>> +
>> +	if ((pwr_status & l2_allow_mask) == 0) {
>> +		drm_warn(&ptdev->base, "Power on L2 domain not allowed");
>> +		return -EPERM;
>> +	}
>> +
>> +	if (gpu_read64(ptdev, l2_pwrtrans_reg)) {
>> +		drm_warn(&ptdev->base, "L2 Power is in transition");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = panthor_pwr_domain_power_on(ptdev, PWR_COMMAND_DOMAIN_L2,
>> +					  ptdev->gpu_info.l2_present,
>> +					  PWR_TRANSITION_TIMEOUT_US);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Delegate control of the shader and tiler power domains to the MCU as
>> +	 * it can better manage which shader/tiler cores need to be powered up
>> +	 * or can be powered down based on currently running jobs.
>> +	 */
>> +	return panthor_pwr_delegate_domains(ptdev);
>>  }
>>  
>>  void panthor_pwr_suspend(struct panthor_device *ptdev)
> 

Re: [PATCH v1 06/10] drm/panthor: Implement L2 power on/off via PWR_CONTROL

[Steven Price]

On 23/10/2025 23:16, Karunika Choo wrote:
> On 20/10/2025 11:50, Steven Price wrote:
>> On 14/10/2025 10:43, Karunika Choo wrote:
>>> This patch adds common helpers to issue power commands, poll
>>> transitions, and validate domain state, then wires them into the L2
>>> on/off paths.
>>>
>>> The L2 power-on sequence now delegates control of the SHADER and TILER
>>> domains to the MCU when allowed, while the L2 itself is never delegated.
>>> On power-off, dependent domains beneath the L2 are checked, and if
>>> necessary, retracted and powered down to maintain proper domain
>>> ordering.
>>>
>>> Signed-off-by: Karunika Choo <karunika.choo@arm.com>
>>> ---
[...]
>>> +		u64 domain_ready = gpu_read64(ptdev, get_domain_ready_reg(child_domain));
>>> +
>>> +		if (domain_ready && (pwr_status & PWR_STATUS_DOMAIN_DELEGATED(child_domain))) {
>>> +			drm_warn(&ptdev->base,
>>> +				 "L2 power off: Delegated %s domain not powered down by MCU",
>>> +				 get_domain_name(child_domain));
>>> +			ret = retract_domain(ptdev, child_domain);
>>> +			if (ret) {
>>> +				drm_err(&ptdev->base, "Failed to retract %s domain",
>>> +					get_domain_name(child_domain));
>>> +				panthor_pwr_info_show(ptdev);
>>> +				return ret;
>>> +			}
>>> +		}
>>> +
>>> +		ret = panthor_pwr_domain_power_off(ptdev, child_domain, domain_ready,
>>> +						   PWR_TRANSITION_TIMEOUT_US);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	return panthor_pwr_domain_power_off(ptdev, PWR_COMMAND_DOMAIN_L2,
>>> +					    ptdev->gpu_info.l2_present,
>>> +					    PWR_TRANSITION_TIMEOUT_US);
>>
>> Does this implicitly 'retract' the shader/tiler power domains? If so I
>> think it's worth a comment. Otherwise it looks like we don't actually
>> know the status of whether the shader/tiler power domains are retracted
>> or not.
>>
> 
> panthor_pwr_l2_power_off() will only retract the shader/tiler domains if
> they have not been powered down by the MCU. In cases where the MCU did
> power down these child domains, delegate_domain() will exit early as
> they would already be delegated. I understand the ambiguity here,
> hopefully it is somewhat acceptable.

So my question was really how does the driver know whether the domains
are delegated or not when this function returns?

I couldn't quite get my head around whether turning the L2 power domain
off would implicitly 'retract' the shader/tiler power domains -
obviously it forces them off which means the MCU doesn't have control.
So retracting would make sense, but I couldn't see anything in the spec.

It would be good to have a comment explaining what the expected state is
after this function (panthor_pwr_l2_power_off) returns. Is it unknown
whether the shader/tiler are retracted, or is there something in the
hardware which does this automatically so we know but don't have to
manually retract? Presumably if we end up fully powering down the GPU
that must effectively retract all domains (the GPU hardware is reset so
it goes back to reset conditions).

Sorry, it's a bit of a basic question but the spec is somewhat unhelpful
on this point! (Or at least I haven't found a relevant statement).

Thanks,
Steve

Re: [PATCH v1 06/10] drm/panthor: Implement L2 power on/off via PWR_CONTROL

[Karunika Choo]

On 24/10/2025 10:43, Steven Price wrote:
> On 23/10/2025 23:16, Karunika Choo wrote:
>> On 20/10/2025 11:50, Steven Price wrote:
>>> On 14/10/2025 10:43, Karunika Choo wrote:
>>>> This patch adds common helpers to issue power commands, poll
>>>> transitions, and validate domain state, then wires them into the L2
>>>> on/off paths.
>>>>
>>>> The L2 power-on sequence now delegates control of the SHADER and TILER
>>>> domains to the MCU when allowed, while the L2 itself is never delegated.
>>>> On power-off, dependent domains beneath the L2 are checked, and if
>>>> necessary, retracted and powered down to maintain proper domain
>>>> ordering.
>>>>
>>>> Signed-off-by: Karunika Choo <karunika.choo@arm.com>
>>>> ---
> [...]
>>>> +		u64 domain_ready = gpu_read64(ptdev, get_domain_ready_reg(child_domain));
>>>> +
>>>> +		if (domain_ready && (pwr_status & PWR_STATUS_DOMAIN_DELEGATED(child_domain))) {
>>>> +			drm_warn(&ptdev->base,
>>>> +				 "L2 power off: Delegated %s domain not powered down by MCU",
>>>> +				 get_domain_name(child_domain));
>>>> +			ret = retract_domain(ptdev, child_domain);
>>>> +			if (ret) {
>>>> +				drm_err(&ptdev->base, "Failed to retract %s domain",
>>>> +					get_domain_name(child_domain));
>>>> +				panthor_pwr_info_show(ptdev);
>>>> +				return ret;
>>>> +			}
>>>> +		}
>>>> +
>>>> +		ret = panthor_pwr_domain_power_off(ptdev, child_domain, domain_ready,
>>>> +						   PWR_TRANSITION_TIMEOUT_US);
>>>> +		if (ret)
>>>> +			return ret;
>>>> +	}
>>>> +
>>>> +	return panthor_pwr_domain_power_off(ptdev, PWR_COMMAND_DOMAIN_L2,
>>>> +					    ptdev->gpu_info.l2_present,
>>>> +					    PWR_TRANSITION_TIMEOUT_US);
>>>
>>> Does this implicitly 'retract' the shader/tiler power domains? If so I
>>> think it's worth a comment. Otherwise it looks like we don't actually
>>> know the status of whether the shader/tiler power domains are retracted
>>> or not.
>>>
>>
>> panthor_pwr_l2_power_off() will only retract the shader/tiler domains if
>> they have not been powered down by the MCU. In cases where the MCU did
>> power down these child domains, delegate_domain() will exit early as
>> they would already be delegated. I understand the ambiguity here,
>> hopefully it is somewhat acceptable.
> 
> So my question was really how does the driver know whether the domains
> are delegated or not when this function returns?
> 
> I couldn't quite get my head around whether turning the L2 power domain
> off would implicitly 'retract' the shader/tiler power domains -
> obviously it forces them off which means the MCU doesn't have control.
> So retracting would make sense, but I couldn't see anything in the spec.
> 
> It would be good to have a comment explaining what the expected state is
> after this function (panthor_pwr_l2_power_off) returns. Is it unknown
> whether the shader/tiler are retracted, or is there something in the
> hardware which does this automatically so we know but don't have to
> manually retract? Presumably if we end up fully powering down the GPU
> that must effectively retract all domains (the GPU hardware is reset so
> it goes back to reset conditions).
> 
> Sorry, it's a bit of a basic question but the spec is somewhat unhelpful
> on this point! (Or at least I haven't found a relevant statement).
> 

Powering off the L2 does not automatically retract its child domains.
The above case is for handling the edge case where the MCU is hung and
is not able to power off the delegated domains, therefore the host needs
to take over and power them down before turning off the L2. Additionally,
like you have alluded to, powering off the GPU will inevitably reset all
of these states (retracting the child domains), necessitating a
re-delegation on L2 power on.

Therefore, the typical operation loop will be as follows:
 1. L2 power on
 2. Delegate Tiler/Shader
 <suspend>
 3. Halt MCU (should power down Tiler/Shader)
 4. L2 power off (no retract of Tiler/Shader)
 <resume>
 5. L2 power on (next resume)
 6. Delegate Tiler/Shader (skipped as already delegated)

If the MCU is hung:
 1. L2 power on
 2. Delegate Tiler/Shader
 <suspend>
 3. Halt MCU fails
 4. L2 power off (Retract and power off Shader/Tiler)
 <resume>
 5. L2 power on
 6. Delegate Tiler/Shader

If the GPU is turned off between suspend and resume:
 1. L2 power on
 2. Delegate Tiler/Shader
 <suspend>
 3. Halt MCU (should power down Tiler/Shader)
 4. L2 power off (no retract of Tiler/Shader)
 <GPU turned off>
 <resume>
 6. L2 power on
 7. Delegate Tiler/Shader

With the current implementation, we cannot expect it to be always
retracted on return of the function, but it does provide the
additional benefit that on resume we don't need to go through the
whole delegate cycle after powering up the L2, allowing us to
save some time there.

On the other hand, if we want to explicitly enforce that we retract on 
suspend, then we have to accept the additional cost to delegate the
domains on resume.

Kind regards,
Karunika Choo

> Thanks,
> Steve
> 

Re: [PATCH v1 06/10] drm/panthor: Implement L2 power on/off via PWR_CONTROL

[Steven Price]

On 24/10/2025 12:51, Karunika Choo wrote:
> On 24/10/2025 10:43, Steven Price wrote:
>> On 23/10/2025 23:16, Karunika Choo wrote:
>>> On 20/10/2025 11:50, Steven Price wrote:
>>>> On 14/10/2025 10:43, Karunika Choo wrote:
>>>>> This patch adds common helpers to issue power commands, poll
>>>>> transitions, and validate domain state, then wires them into the L2
>>>>> on/off paths.
>>>>>
>>>>> The L2 power-on sequence now delegates control of the SHADER and TILER
>>>>> domains to the MCU when allowed, while the L2 itself is never delegated.
>>>>> On power-off, dependent domains beneath the L2 are checked, and if
>>>>> necessary, retracted and powered down to maintain proper domain
>>>>> ordering.
>>>>>
>>>>> Signed-off-by: Karunika Choo <karunika.choo@arm.com>
>>>>> ---
>> [...]
>>>>> +		u64 domain_ready = gpu_read64(ptdev, get_domain_ready_reg(child_domain));
>>>>> +
>>>>> +		if (domain_ready && (pwr_status & PWR_STATUS_DOMAIN_DELEGATED(child_domain))) {
>>>>> +			drm_warn(&ptdev->base,
>>>>> +				 "L2 power off: Delegated %s domain not powered down by MCU",
>>>>> +				 get_domain_name(child_domain));
>>>>> +			ret = retract_domain(ptdev, child_domain);
>>>>> +			if (ret) {
>>>>> +				drm_err(&ptdev->base, "Failed to retract %s domain",
>>>>> +					get_domain_name(child_domain));
>>>>> +				panthor_pwr_info_show(ptdev);
>>>>> +				return ret;
>>>>> +			}
>>>>> +		}
>>>>> +
>>>>> +		ret = panthor_pwr_domain_power_off(ptdev, child_domain, domain_ready,
>>>>> +						   PWR_TRANSITION_TIMEOUT_US);
>>>>> +		if (ret)
>>>>> +			return ret;
>>>>> +	}
>>>>> +
>>>>> +	return panthor_pwr_domain_power_off(ptdev, PWR_COMMAND_DOMAIN_L2,
>>>>> +					    ptdev->gpu_info.l2_present,
>>>>> +					    PWR_TRANSITION_TIMEOUT_US);
>>>>
>>>> Does this implicitly 'retract' the shader/tiler power domains? If so I
>>>> think it's worth a comment. Otherwise it looks like we don't actually
>>>> know the status of whether the shader/tiler power domains are retracted
>>>> or not.
>>>>
>>>
>>> panthor_pwr_l2_power_off() will only retract the shader/tiler domains if
>>> they have not been powered down by the MCU. In cases where the MCU did
>>> power down these child domains, delegate_domain() will exit early as
>>> they would already be delegated. I understand the ambiguity here,
>>> hopefully it is somewhat acceptable.
>>
>> So my question was really how does the driver know whether the domains
>> are delegated or not when this function returns?
>>
>> I couldn't quite get my head around whether turning the L2 power domain
>> off would implicitly 'retract' the shader/tiler power domains -
>> obviously it forces them off which means the MCU doesn't have control.
>> So retracting would make sense, but I couldn't see anything in the spec.
>>
>> It would be good to have a comment explaining what the expected state is
>> after this function (panthor_pwr_l2_power_off) returns. Is it unknown
>> whether the shader/tiler are retracted, or is there something in the
>> hardware which does this automatically so we know but don't have to
>> manually retract? Presumably if we end up fully powering down the GPU
>> that must effectively retract all domains (the GPU hardware is reset so
>> it goes back to reset conditions).
>>
>> Sorry, it's a bit of a basic question but the spec is somewhat unhelpful
>> on this point! (Or at least I haven't found a relevant statement).
>>
> 
> Powering off the L2 does not automatically retract its child domains.
> The above case is for handling the edge case where the MCU is hung and
> is not able to power off the delegated domains, therefore the host needs
> to take over and power them down before turning off the L2. Additionally,
> like you have alluded to, powering off the GPU will inevitably reset all
> of these states (retracting the child domains), necessitating a
> re-delegation on L2 power on.
> 
> Therefore, the typical operation loop will be as follows:
>  1. L2 power on
>  2. Delegate Tiler/Shader
>  <suspend>
>  3. Halt MCU (should power down Tiler/Shader)
>  4. L2 power off (no retract of Tiler/Shader)
>  <resume>
>  5. L2 power on (next resume)
>  6. Delegate Tiler/Shader (skipped as already delegated)
> 
> If the MCU is hung:
>  1. L2 power on
>  2. Delegate Tiler/Shader
>  <suspend>
>  3. Halt MCU fails
>  4. L2 power off (Retract and power off Shader/Tiler)
>  <resume>
>  5. L2 power on
>  6. Delegate Tiler/Shader
> 
> If the GPU is turned off between suspend and resume:
>  1. L2 power on
>  2. Delegate Tiler/Shader
>  <suspend>
>  3. Halt MCU (should power down Tiler/Shader)
>  4. L2 power off (no retract of Tiler/Shader)
>  <GPU turned off>
>  <resume>
>  6. L2 power on
>  7. Delegate Tiler/Shader

Thanks for the explanation!

> 
> With the current implementation, we cannot expect it to be always
> retracted on return of the function, but it does provide the
> additional benefit that on resume we don't need to go through the
> whole delegate cycle after powering up the L2, allowing us to
> save some time there.
> 
> On the other hand, if we want to explicitly enforce that we retract on 
> suspend, then we have to accept the additional cost to delegate the
> domains on resume.

No, there's no need to change it. But I think it's worth a comment that
in the usual case (the MCU isn't hung) the shader/tiler are left
delegated and the attempt to delegate them again will detect this and
skip it.

I think what mostly confused me is that delegate_domain() has the following:

> +	if (pwr_status_reg & delegated_mask) {
> +		drm_dbg(&ptdev->base, "%s domain already delegated",
> +			get_domain_name(domain));
> +		return 0;
> +	}

Although it's "drm_dbg" that message makes it seem like this is an
unexpected situation. Whereas actually we would normally expect that to
happen during a resume (as long as the GPU remains powered). With that
in my head I then started to think that there might be something in the
hardware causing an automatic "retract".

Thanks,
Steve

[PATCH v1 07/10] drm/panthor: Implement soft and fast reset via PWR_CONTROL

[Karunika Choo]

Add helpers to issue reset commands through the PWR_CONTROL interface
and wait for reset completion using IRQ signaling. This enables support
for both RESET_SOFT and RESET_FAST operations with timeout handling and
status verification.

Signed-off-by: Karunika Choo <karunika.choo@arm.com>
---
 drivers/gpu/drm/panthor/panthor_pwr.c | 62 ++++++++++++++++++++++++++-
 drivers/gpu/drm/panthor/panthor_pwr.h |  2 +
 2 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panthor/panthor_pwr.c b/drivers/gpu/drm/panthor/panthor_pwr.c
index 594181aba847..ecb278824d06 100644
--- a/drivers/gpu/drm/panthor/panthor_pwr.c
+++ b/drivers/gpu/drm/panthor/panthor_pwr.c
@@ -3,6 +3,7 @@
 
 #include <linux/platform_device.h>
 #include <linux/interrupt.h>
+#include <linux/cleanup.h>
 #include <linux/iopoll.h>
 #include <linux/wait.h>
 
@@ -31,6 +32,8 @@
 
 #define PWR_RETRACT_TIMEOUT_US		2000
 
+#define PWR_RESET_TIMEOUT_MS		500
+
 /**
  * struct panthor_pwr - PWR_CONTROL block management data.
  */
@@ -80,6 +83,42 @@ static void panthor_pwr_write_command(struct panthor_device *ptdev, u32 command,
 	gpu_write(ptdev, PWR_COMMAND, command);
 }
 
+static bool reset_irq_raised(struct panthor_device *ptdev)
+{
+	return gpu_read(ptdev, PWR_INT_RAWSTAT) & PWR_IRQ_RESET_COMPLETED;
+}
+
+static bool reset_completed(struct panthor_device *ptdev)
+{
+	return (ptdev->pwr->pending_reqs & PWR_IRQ_RESET_COMPLETED);
+}
+
+static int panthor_pwr_reset(struct panthor_device *ptdev, u32 reset_cmd)
+{
+	scoped_guard(spinlock_irqsave, &ptdev->pwr->reqs_lock) {
+		if (!drm_WARN_ON(&ptdev->base, !reset_completed(ptdev))) {
+			ptdev->pwr->pending_reqs |= PWR_IRQ_RESET_COMPLETED;
+			gpu_write(ptdev, PWR_INT_CLEAR, PWR_IRQ_RESET_COMPLETED);
+			panthor_pwr_write_command(ptdev, reset_cmd, 0);
+		}
+	}
+
+	if (!wait_event_timeout(ptdev->pwr->reqs_acked, reset_completed(ptdev),
+				msecs_to_jiffies(PWR_RESET_TIMEOUT_MS))) {
+		guard(spinlock_irqsave)(&ptdev->pwr->reqs_lock);
+
+		if (!reset_completed(ptdev) && !reset_irq_raised(ptdev)) {
+			drm_err(&ptdev->base, "RESET_%s timed out",
+				reset_cmd == PWR_COMMAND_RESET_SOFT ? "SOFT" : "FAST");
+			return -ETIMEDOUT;
+		}
+
+		ptdev->pwr->pending_reqs &= ~PWR_IRQ_RESET_COMPLETED;
+	}
+
+	return 0;
+}
+
 static const char *get_domain_name(u8 domain)
 {
 	switch (domain) {
@@ -407,9 +446,30 @@ int panthor_pwr_init(struct panthor_device *ptdev)
 	return 0;
 }
 
+int panthor_pwr_reset_fast(struct panthor_device *ptdev)
+{
+	if (!ptdev->pwr)
+		return 0;
+
+	if (!(panthor_pwr_read_status(ptdev) & PWR_STATUS_ALLOW_FAST_RESET)) {
+		drm_err(&ptdev->base, "RESET_SOFT not allowed");
+		return -EOPNOTSUPP;
+	}
+
+	return panthor_pwr_reset(ptdev, PWR_COMMAND_RESET_FAST);
+}
+
 int panthor_pwr_reset_soft(struct panthor_device *ptdev)
 {
-	return 0;
+	if (!ptdev->pwr)
+		return 0;
+
+	if (!(panthor_pwr_read_status(ptdev) & PWR_STATUS_ALLOW_SOFT_RESET)) {
+		drm_err(&ptdev->base, "RESET_SOFT not allowed");
+		return -EOPNOTSUPP;
+	}
+
+	return panthor_pwr_reset(ptdev, PWR_COMMAND_RESET_SOFT);
 }
 
 int panthor_pwr_l2_power_off(struct panthor_device *ptdev)
diff --git a/drivers/gpu/drm/panthor/panthor_pwr.h b/drivers/gpu/drm/panthor/panthor_pwr.h
index a4042c125448..2301c26dab86 100644
--- a/drivers/gpu/drm/panthor/panthor_pwr.h
+++ b/drivers/gpu/drm/panthor/panthor_pwr.h
@@ -10,6 +10,8 @@ void panthor_pwr_unplug(struct panthor_device *ptdev);
 
 int panthor_pwr_init(struct panthor_device *ptdev);
 
+int panthor_pwr_reset_fast(struct panthor_device *ptdev);
+
 int panthor_pwr_reset_soft(struct panthor_device *ptdev);
 
 int panthor_pwr_l2_power_on(struct panthor_device *ptdev);
-- 
2.49.0

Re: [PATCH v1 07/10] drm/panthor: Implement soft and fast reset via PWR_CONTROL

[Steven Price]

On 14/10/2025 10:43, Karunika Choo wrote:
> Add helpers to issue reset commands through the PWR_CONTROL interface
> and wait for reset completion using IRQ signaling. This enables support
> for both RESET_SOFT and RESET_FAST operations with timeout handling and
> status verification.
> 
> Signed-off-by: Karunika Choo <karunika.choo@arm.com>
> ---
>  drivers/gpu/drm/panthor/panthor_pwr.c | 62 ++++++++++++++++++++++++++-
>  drivers/gpu/drm/panthor/panthor_pwr.h |  2 +
>  2 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_pwr.c b/drivers/gpu/drm/panthor/panthor_pwr.c
> index 594181aba847..ecb278824d06 100644
> --- a/drivers/gpu/drm/panthor/panthor_pwr.c
> +++ b/drivers/gpu/drm/panthor/panthor_pwr.c
> @@ -3,6 +3,7 @@
>  
>  #include <linux/platform_device.h>
>  #include <linux/interrupt.h>
> +#include <linux/cleanup.h>
>  #include <linux/iopoll.h>
>  #include <linux/wait.h>
>  
> @@ -31,6 +32,8 @@
>  
>  #define PWR_RETRACT_TIMEOUT_US		2000
>  
> +#define PWR_RESET_TIMEOUT_MS		500
> +
>  /**
>   * struct panthor_pwr - PWR_CONTROL block management data.
>   */
> @@ -80,6 +83,42 @@ static void panthor_pwr_write_command(struct panthor_device *ptdev, u32 command,
>  	gpu_write(ptdev, PWR_COMMAND, command);
>  }
>  
> +static bool reset_irq_raised(struct panthor_device *ptdev)
> +{
> +	return gpu_read(ptdev, PWR_INT_RAWSTAT) & PWR_IRQ_RESET_COMPLETED;
> +}
> +
> +static bool reset_completed(struct panthor_device *ptdev)
> +{
> +	return (ptdev->pwr->pending_reqs & PWR_IRQ_RESET_COMPLETED);
> +}
> +
> +static int panthor_pwr_reset(struct panthor_device *ptdev, u32 reset_cmd)
> +{
> +	scoped_guard(spinlock_irqsave, &ptdev->pwr->reqs_lock) {
> +		if (!drm_WARN_ON(&ptdev->base, !reset_completed(ptdev))) {
> +			ptdev->pwr->pending_reqs |= PWR_IRQ_RESET_COMPLETED;
> +			gpu_write(ptdev, PWR_INT_CLEAR, PWR_IRQ_RESET_COMPLETED);
> +			panthor_pwr_write_command(ptdev, reset_cmd, 0);
> +		}

This would be easier to read as:

if (reset_completed(ptdev)) {
	....
} else {
	drm_WARN(&ptdev->base, 1, "Hey, we're already resetting?");
}

[Message modified to taste ;) ]

I'm also wondering if things would be easier to read if you switched
from reset_completed() to reset_pending(). Certainly here it's the
'pending' test you are trying to do.

> +	}
> +
> +	if (!wait_event_timeout(ptdev->pwr->reqs_acked, reset_completed(ptdev),
> +				msecs_to_jiffies(PWR_RESET_TIMEOUT_MS))) {
> +		guard(spinlock_irqsave)(&ptdev->pwr->reqs_lock);
> +
> +		if (!reset_completed(ptdev) && !reset_irq_raised(ptdev)) {
> +			drm_err(&ptdev->base, "RESET_%s timed out",
> +				reset_cmd == PWR_COMMAND_RESET_SOFT ? "SOFT" : "FAST");
> +			return -ETIMEDOUT;
> +		}
> +
> +		ptdev->pwr->pending_reqs &= ~PWR_IRQ_RESET_COMPLETED;
> +	}
> +
> +	return 0;
> +}
> +
>  static const char *get_domain_name(u8 domain)
>  {
>  	switch (domain) {
> @@ -407,9 +446,30 @@ int panthor_pwr_init(struct panthor_device *ptdev)
>  	return 0;
>  }
>  
> +int panthor_pwr_reset_fast(struct panthor_device *ptdev)
> +{
> +	if (!ptdev->pwr)
> +		return 0;
> +
> +	if (!(panthor_pwr_read_status(ptdev) & PWR_STATUS_ALLOW_FAST_RESET)) {
> +		drm_err(&ptdev->base, "RESET_SOFT not allowed");

Copy/paste mistake on the error message.

> +		return -EOPNOTSUPP;
> +	}
> +
> +	return panthor_pwr_reset(ptdev, PWR_COMMAND_RESET_FAST);
> +}

I can't actually find a caller of this function within the series.

> +
>  int panthor_pwr_reset_soft(struct panthor_device *ptdev)
>  {
> -	return 0;
> +	if (!ptdev->pwr)
> +		return 0;

When would this happen? Is this not a programming error?

Thanks,
Steve

> +
> +	if (!(panthor_pwr_read_status(ptdev) & PWR_STATUS_ALLOW_SOFT_RESET)) {
> +		drm_err(&ptdev->base, "RESET_SOFT not allowed");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return panthor_pwr_reset(ptdev, PWR_COMMAND_RESET_SOFT);
>  }
>  
>  int panthor_pwr_l2_power_off(struct panthor_device *ptdev)
> diff --git a/drivers/gpu/drm/panthor/panthor_pwr.h b/drivers/gpu/drm/panthor/panthor_pwr.h
> index a4042c125448..2301c26dab86 100644
> --- a/drivers/gpu/drm/panthor/panthor_pwr.h
> +++ b/drivers/gpu/drm/panthor/panthor_pwr.h
> @@ -10,6 +10,8 @@ void panthor_pwr_unplug(struct panthor_device *ptdev);
>  
>  int panthor_pwr_init(struct panthor_device *ptdev);
>  
> +int panthor_pwr_reset_fast(struct panthor_device *ptdev);
> +
>  int panthor_pwr_reset_soft(struct panthor_device *ptdev);
>  
>  int panthor_pwr_l2_power_on(struct panthor_device *ptdev);

Re: [PATCH v1 07/10] drm/panthor: Implement soft and fast reset via PWR_CONTROL

[Karunika Choo]

On 20/10/2025 12:24, Steven Price wrote:
> On 14/10/2025 10:43, Karunika Choo wrote:
>> Add helpers to issue reset commands through the PWR_CONTROL interface
>> and wait for reset completion using IRQ signaling. This enables support
>> for both RESET_SOFT and RESET_FAST operations with timeout handling and
>> status verification.
>>
>> Signed-off-by: Karunika Choo <karunika.choo@arm.com>
>> ---
>>  drivers/gpu/drm/panthor/panthor_pwr.c | 62 ++++++++++++++++++++++++++-
>>  drivers/gpu/drm/panthor/panthor_pwr.h |  2 +
>>  2 files changed, 63 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/panthor/panthor_pwr.c b/drivers/gpu/drm/panthor/panthor_pwr.c
>> index 594181aba847..ecb278824d06 100644
>> --- a/drivers/gpu/drm/panthor/panthor_pwr.c
>> +++ b/drivers/gpu/drm/panthor/panthor_pwr.c
>> @@ -3,6 +3,7 @@
>>  
>>  #include <linux/platform_device.h>
>>  #include <linux/interrupt.h>
>> +#include <linux/cleanup.h>
>>  #include <linux/iopoll.h>
>>  #include <linux/wait.h>
>>  
>> @@ -31,6 +32,8 @@
>>  
>>  #define PWR_RETRACT_TIMEOUT_US		2000
>>  
>> +#define PWR_RESET_TIMEOUT_MS		500
>> +
>>  /**
>>   * struct panthor_pwr - PWR_CONTROL block management data.
>>   */
>> @@ -80,6 +83,42 @@ static void panthor_pwr_write_command(struct panthor_device *ptdev, u32 command,
>>  	gpu_write(ptdev, PWR_COMMAND, command);
>>  }
>>  
>> +static bool reset_irq_raised(struct panthor_device *ptdev)
>> +{
>> +	return gpu_read(ptdev, PWR_INT_RAWSTAT) & PWR_IRQ_RESET_COMPLETED;
>> +}
>> +
>> +static bool reset_completed(struct panthor_device *ptdev)
>> +{
>> +	return (ptdev->pwr->pending_reqs & PWR_IRQ_RESET_COMPLETED);
>> +}
>> +
>> +static int panthor_pwr_reset(struct panthor_device *ptdev, u32 reset_cmd)
>> +{
>> +	scoped_guard(spinlock_irqsave, &ptdev->pwr->reqs_lock) {
>> +		if (!drm_WARN_ON(&ptdev->base, !reset_completed(ptdev))) {
>> +			ptdev->pwr->pending_reqs |= PWR_IRQ_RESET_COMPLETED;
>> +			gpu_write(ptdev, PWR_INT_CLEAR, PWR_IRQ_RESET_COMPLETED);
>> +			panthor_pwr_write_command(ptdev, reset_cmd, 0);
>> +		}
> 
> This would be easier to read as:
> 
> if (reset_completed(ptdev)) {
> 	....
> } else {
> 	drm_WARN(&ptdev->base, 1, "Hey, we're already resetting?");
> }
> 
> [Message modified to taste ;) ]
> 
> I'm also wondering if things would be easier to read if you switched
> from reset_completed() to reset_pending(). Certainly here it's the
> 'pending' test you are trying to do.
> 

Oops. I might have made a mistake with the logic. Let me fix this in v2.
Thanks for spotting it

>> +	}
>> +
>> +	if (!wait_event_timeout(ptdev->pwr->reqs_acked, reset_completed(ptdev),
>> +				msecs_to_jiffies(PWR_RESET_TIMEOUT_MS))) {
>> +		guard(spinlock_irqsave)(&ptdev->pwr->reqs_lock);
>> +
>> +		if (!reset_completed(ptdev) && !reset_irq_raised(ptdev)) {
>> +			drm_err(&ptdev->base, "RESET_%s timed out",
>> +				reset_cmd == PWR_COMMAND_RESET_SOFT ? "SOFT" : "FAST");
>> +			return -ETIMEDOUT;
>> +		}
>> +
>> +		ptdev->pwr->pending_reqs &= ~PWR_IRQ_RESET_COMPLETED;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static const char *get_domain_name(u8 domain)
>>  {
>>  	switch (domain) {
>> @@ -407,9 +446,30 @@ int panthor_pwr_init(struct panthor_device *ptdev)
>>  	return 0;
>>  }
>>  
>> +int panthor_pwr_reset_fast(struct panthor_device *ptdev)
>> +{
>> +	if (!ptdev->pwr)
>> +		return 0;
>> +
>> +	if (!(panthor_pwr_read_status(ptdev) & PWR_STATUS_ALLOW_FAST_RESET)) {
>> +		drm_err(&ptdev->base, "RESET_SOFT not allowed");
> 
> Copy/paste mistake on the error message.
> 
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	return panthor_pwr_reset(ptdev, PWR_COMMAND_RESET_FAST);
>> +}
> 
> I can't actually find a caller of this function within the series.
> 

I will remove the fast reset option entirely in v2 as there is currently
no use for this function. We can reimplement this in the future, should
it be something that is desired.

>> +
>>  int panthor_pwr_reset_soft(struct panthor_device *ptdev)
>>  {
>> -	return 0;
>> +	if (!ptdev->pwr)
>> +		return 0;
> 
> When would this happen? Is this not a programming error?
> 

I will remove this. Thanks.

Kind regards,
Karunika Choo

> Thanks,
> Steve
> 
>> +
>> +	if (!(panthor_pwr_read_status(ptdev) & PWR_STATUS_ALLOW_SOFT_RESET)) {
>> +		drm_err(&ptdev->base, "RESET_SOFT not allowed");
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	return panthor_pwr_reset(ptdev, PWR_COMMAND_RESET_SOFT);
>>  }
>>  
>>  int panthor_pwr_l2_power_off(struct panthor_device *ptdev)
>> diff --git a/drivers/gpu/drm/panthor/panthor_pwr.h b/drivers/gpu/drm/panthor/panthor_pwr.h
>> index a4042c125448..2301c26dab86 100644
>> --- a/drivers/gpu/drm/panthor/panthor_pwr.h
>> +++ b/drivers/gpu/drm/panthor/panthor_pwr.h
>> @@ -10,6 +10,8 @@ void panthor_pwr_unplug(struct panthor_device *ptdev);
>>  
>>  int panthor_pwr_init(struct panthor_device *ptdev);
>>  
>> +int panthor_pwr_reset_fast(struct panthor_device *ptdev);
>> +
>>  int panthor_pwr_reset_soft(struct panthor_device *ptdev);
>>  
>>  int panthor_pwr_l2_power_on(struct panthor_device *ptdev);
> 

[PATCH v1 08/10] drm/panthor: Support GLB_REQ.STATE field for Mali-G1 GPUs

[Karunika Choo]

Add support for the GLB_REQ.STATE field introduced in CSF v4.1+, which
replaces the HALT bit to provide finer control over the MCU state. This
change implements basic handling for transitioning the MCU between
ACTIVE and HALT states on Mali-G1 GPUs.

The update introduces new helpers to issue the state change requests,
poll for MCU halt completion, and restore the MCU to an active state
after halting.

Signed-off-by: Karunika Choo <karunika.choo@arm.com>
---
 drivers/gpu/drm/panthor/panthor_fw.c | 82 ++++++++++++++++++++++------
 drivers/gpu/drm/panthor/panthor_fw.h |  7 +++
 2 files changed, 73 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
index e6c39c70d348..48bbae8931cb 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.c
+++ b/drivers/gpu/drm/panthor/panthor_fw.c
@@ -33,6 +33,7 @@
 #define PROGRESS_TIMEOUT_SCALE_SHIFT		10
 #define IDLE_HYSTERESIS_US			800
 #define PWROFF_HYSTERESIS_US			10000
+#define MCU_HALT_TIMEOUT_US			(10ULL * 1000 * 1000)
 
 /**
  * struct panthor_fw_binary_hdr - Firmware binary header.
@@ -996,6 +997,9 @@ static void panthor_fw_init_global_iface(struct panthor_device *ptdev)
 					 GLB_IDLE_EN |
 					 GLB_IDLE;
 
+	if (panthor_fw_csf_version(ptdev) >= CSF_IFACE_VERSION(4, 1, 0))
+		glb_iface->input->ack_irq_mask |= GLB_STATE_MASK;
+
 	panthor_fw_update_reqs(glb_iface, req, GLB_IDLE_EN, GLB_IDLE_EN);
 	panthor_fw_toggle_reqs(glb_iface, req, ack,
 			       GLB_CFG_ALLOC_EN |
@@ -1069,6 +1073,54 @@ static void panthor_fw_stop(struct panthor_device *ptdev)
 		drm_err(&ptdev->base, "Failed to stop MCU");
 }
 
+static bool panthor_fw_mcu_halted(struct panthor_device *ptdev)
+{
+	struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
+	bool halted;
+
+	halted = gpu_read(ptdev, MCU_STATUS) == MCU_STATUS_HALT;
+
+	if (panthor_fw_csf_version(ptdev) >= CSF_IFACE_VERSION(4, 1, 0))
+		halted &= (GLB_STATE_GET(glb_iface->output->ack) == GLB_STATE_HALT);
+
+	return halted;
+}
+
+static void panthor_fw_halt_mcu(struct panthor_device *ptdev)
+{
+	struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
+
+	if (panthor_fw_csf_version(ptdev) >= CSF_IFACE_VERSION(4, 1, 0))
+		panthor_fw_update_reqs(glb_iface, req, GLB_STATE(GLB_STATE_HALT), GLB_STATE_MASK);
+	else
+		panthor_fw_update_reqs(glb_iface, req, GLB_HALT, GLB_HALT);
+
+	gpu_write(ptdev, CSF_DOORBELL(CSF_GLB_DOORBELL_ID), 1);
+}
+
+static bool panthor_fw_wait_mcu_halted(struct panthor_device *ptdev)
+{
+	bool halted = false;
+
+	if (read_poll_timeout_atomic(panthor_fw_mcu_halted, halted, halted, 10,
+				     MCU_HALT_TIMEOUT_US, 0, ptdev)) {
+		drm_warn(&ptdev->base, "Timed out waiting for MCU to halt");
+		return false;
+	}
+
+	return true;
+}
+
+static void panthor_fw_mcu_set_active(struct panthor_device *ptdev)
+{
+	struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
+
+	if (panthor_fw_csf_version(ptdev) >= CSF_IFACE_VERSION(4, 1, 0))
+		panthor_fw_update_reqs(glb_iface, req, GLB_STATE(GLB_STATE_ACTIVE), GLB_STATE_MASK);
+	else
+		panthor_fw_update_reqs(glb_iface, req, 0, GLB_HALT);
+}
+
 /**
  * panthor_fw_pre_reset() - Call before a reset.
  * @ptdev: Device.
@@ -1085,19 +1137,13 @@ void panthor_fw_pre_reset(struct panthor_device *ptdev, bool on_hang)
 	ptdev->reset.fast = false;
 
 	if (!on_hang) {
-		struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
-		u32 status;
-
-		panthor_fw_update_reqs(glb_iface, req, GLB_HALT, GLB_HALT);
-		gpu_write(ptdev, CSF_DOORBELL(CSF_GLB_DOORBELL_ID), 1);
-		if (!gpu_read_poll_timeout(ptdev, MCU_STATUS, status,
-					   status == MCU_STATUS_HALT, 10,
-					   100000)) {
-			ptdev->reset.fast = true;
-		} else {
+		panthor_fw_halt_mcu(ptdev);
+		if (!panthor_fw_wait_mcu_halted(ptdev))
 			drm_warn(&ptdev->base, "Failed to cleanly suspend MCU");
-		}
+		else
+			ptdev->reset.fast = true;
 	}
+	panthor_fw_stop(ptdev);
 
 	panthor_job_irq_suspend(&ptdev->fw->irq);
 }
@@ -1125,14 +1171,14 @@ int panthor_fw_post_reset(struct panthor_device *ptdev)
 		 */
 		panthor_reload_fw_sections(ptdev, true);
 	} else {
-		/* The FW detects 0 -> 1 transitions. Make sure we reset
-		 * the HALT bit before the FW is rebooted.
+		/*
+		 * If the FW was previously successfully halted in the pre-reset
+		 * operation, we need to transition it to active again before
+		 * the FW is rebooted.
 		 * This is not needed on a slow reset because FW sections are
 		 * re-initialized.
 		 */
-		struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
-
-		panthor_fw_update_reqs(glb_iface, req, 0, GLB_HALT);
+		panthor_fw_mcu_set_active(ptdev);
 	}
 
 	ret = panthor_fw_start(ptdev);
@@ -1170,6 +1216,10 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
 		if (ptdev->fw->irq.irq)
 			panthor_job_irq_suspend(&ptdev->fw->irq);
 
+		panthor_fw_halt_mcu(ptdev);
+		if (!panthor_fw_wait_mcu_halted(ptdev))
+			drm_warn(&ptdev->base, "Failed to halt MCU on unplug");
+
 		panthor_fw_stop(ptdev);
 	}
 
diff --git a/drivers/gpu/drm/panthor/panthor_fw.h b/drivers/gpu/drm/panthor/panthor_fw.h
index 6598d96c6d2a..a19ed48b2d0b 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.h
+++ b/drivers/gpu/drm/panthor/panthor_fw.h
@@ -214,6 +214,13 @@ struct panthor_fw_global_input_iface {
 #define GLB_FWCFG_UPDATE			BIT(9)
 #define GLB_IDLE_EN				BIT(10)
 #define GLB_SLEEP				BIT(12)
+#define GLB_STATE_MASK				GENMASK(14, 12)
+#define   GLB_STATE_ACTIVE			0
+#define   GLB_STATE_HALT			1
+#define   GLB_STATE_SLEEP			2
+#define   GLB_STATE_SUSPEND			3
+#define   GLB_STATE(x)				(((x) << 12) & GLB_STATE_MASK)
+#define   GLB_STATE_GET(x)			(((x) & GLB_STATE_MASK) >> 12)
 #define GLB_INACTIVE_COMPUTE			BIT(20)
 #define GLB_INACTIVE_FRAGMENT			BIT(21)
 #define GLB_INACTIVE_TILER			BIT(22)
-- 
2.49.0

Re: [PATCH v1 08/10] drm/panthor: Support GLB_REQ.STATE field for Mali-G1 GPUs

[Steven Price]

On 14/10/2025 10:43, Karunika Choo wrote:
> Add support for the GLB_REQ.STATE field introduced in CSF v4.1+, which
> replaces the HALT bit to provide finer control over the MCU state. This
> change implements basic handling for transitioning the MCU between
> ACTIVE and HALT states on Mali-G1 GPUs.
> 
> The update introduces new helpers to issue the state change requests,
> poll for MCU halt completion, and restore the MCU to an active state
> after halting.
> 
> Signed-off-by: Karunika Choo <karunika.choo@arm.com>
> ---
>  drivers/gpu/drm/panthor/panthor_fw.c | 82 ++++++++++++++++++++++------
>  drivers/gpu/drm/panthor/panthor_fw.h |  7 +++
>  2 files changed, 73 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index e6c39c70d348..48bbae8931cb 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> @@ -33,6 +33,7 @@
>  #define PROGRESS_TIMEOUT_SCALE_SHIFT		10
>  #define IDLE_HYSTERESIS_US			800
>  #define PWROFF_HYSTERESIS_US			10000
> +#define MCU_HALT_TIMEOUT_US			(10ULL * 1000 * 1000)

NIT: (10ULL * USEC_PER_SEC)

Also isn't 10 seconds somewhat excessive?

>  
>  /**
>   * struct panthor_fw_binary_hdr - Firmware binary header.
> @@ -996,6 +997,9 @@ static void panthor_fw_init_global_iface(struct panthor_device *ptdev)
>  					 GLB_IDLE_EN |
>  					 GLB_IDLE;
>  
> +	if (panthor_fw_csf_version(ptdev) >= CSF_IFACE_VERSION(4, 1, 0))

This function/macro panthor_fw_csf_version() doesn't exist. You replace
it with glb_iface->control->version in the next patch. So this series
isn't bisectable.

I'm also thinking some sort of helper to check whether features are
implemented in the CSF interface would probably help (rather than
repeating these magic version numbers through the code). Or even
separate functions and use indirection like you do the GPU power control?

Thanks,
Steve

> +		glb_iface->input->ack_irq_mask |= GLB_STATE_MASK;
> +
>  	panthor_fw_update_reqs(glb_iface, req, GLB_IDLE_EN, GLB_IDLE_EN);
>  	panthor_fw_toggle_reqs(glb_iface, req, ack,
>  			       GLB_CFG_ALLOC_EN |
> @@ -1069,6 +1073,54 @@ static void panthor_fw_stop(struct panthor_device *ptdev)
>  		drm_err(&ptdev->base, "Failed to stop MCU");
>  }
>  
> +static bool panthor_fw_mcu_halted(struct panthor_device *ptdev)
> +{
> +	struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
> +	bool halted;
> +
> +	halted = gpu_read(ptdev, MCU_STATUS) == MCU_STATUS_HALT;
> +
> +	if (panthor_fw_csf_version(ptdev) >= CSF_IFACE_VERSION(4, 1, 0))
> +		halted &= (GLB_STATE_GET(glb_iface->output->ack) == GLB_STATE_HALT);
> +
> +	return halted;
> +}
> +
> +static void panthor_fw_halt_mcu(struct panthor_device *ptdev)
> +{
> +	struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
> +
> +	if (panthor_fw_csf_version(ptdev) >= CSF_IFACE_VERSION(4, 1, 0))
> +		panthor_fw_update_reqs(glb_iface, req, GLB_STATE(GLB_STATE_HALT), GLB_STATE_MASK);
> +	else
> +		panthor_fw_update_reqs(glb_iface, req, GLB_HALT, GLB_HALT);
> +
> +	gpu_write(ptdev, CSF_DOORBELL(CSF_GLB_DOORBELL_ID), 1);
> +}
> +
> +static bool panthor_fw_wait_mcu_halted(struct panthor_device *ptdev)
> +{
> +	bool halted = false;
> +
> +	if (read_poll_timeout_atomic(panthor_fw_mcu_halted, halted, halted, 10,
> +				     MCU_HALT_TIMEOUT_US, 0, ptdev)) {
> +		drm_warn(&ptdev->base, "Timed out waiting for MCU to halt");
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +static void panthor_fw_mcu_set_active(struct panthor_device *ptdev)
> +{
> +	struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
> +
> +	if (panthor_fw_csf_version(ptdev) >= CSF_IFACE_VERSION(4, 1, 0))
> +		panthor_fw_update_reqs(glb_iface, req, GLB_STATE(GLB_STATE_ACTIVE), GLB_STATE_MASK);
> +	else
> +		panthor_fw_update_reqs(glb_iface, req, 0, GLB_HALT);
> +}
> +
>  /**
>   * panthor_fw_pre_reset() - Call before a reset.
>   * @ptdev: Device.
> @@ -1085,19 +1137,13 @@ void panthor_fw_pre_reset(struct panthor_device *ptdev, bool on_hang)
>  	ptdev->reset.fast = false;
>  
>  	if (!on_hang) {
> -		struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
> -		u32 status;
> -
> -		panthor_fw_update_reqs(glb_iface, req, GLB_HALT, GLB_HALT);
> -		gpu_write(ptdev, CSF_DOORBELL(CSF_GLB_DOORBELL_ID), 1);
> -		if (!gpu_read_poll_timeout(ptdev, MCU_STATUS, status,
> -					   status == MCU_STATUS_HALT, 10,
> -					   100000)) {
> -			ptdev->reset.fast = true;
> -		} else {
> +		panthor_fw_halt_mcu(ptdev);
> +		if (!panthor_fw_wait_mcu_halted(ptdev))
>  			drm_warn(&ptdev->base, "Failed to cleanly suspend MCU");
> -		}
> +		else
> +			ptdev->reset.fast = true;
>  	}
> +	panthor_fw_stop(ptdev);
>  
>  	panthor_job_irq_suspend(&ptdev->fw->irq);
>  }
> @@ -1125,14 +1171,14 @@ int panthor_fw_post_reset(struct panthor_device *ptdev)
>  		 */
>  		panthor_reload_fw_sections(ptdev, true);
>  	} else {
> -		/* The FW detects 0 -> 1 transitions. Make sure we reset
> -		 * the HALT bit before the FW is rebooted.
> +		/*
> +		 * If the FW was previously successfully halted in the pre-reset
> +		 * operation, we need to transition it to active again before
> +		 * the FW is rebooted.
>  		 * This is not needed on a slow reset because FW sections are
>  		 * re-initialized.
>  		 */
> -		struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
> -
> -		panthor_fw_update_reqs(glb_iface, req, 0, GLB_HALT);
> +		panthor_fw_mcu_set_active(ptdev);
>  	}
>  
>  	ret = panthor_fw_start(ptdev);
> @@ -1170,6 +1216,10 @@ void panthor_fw_unplug(struct panthor_device *ptdev)
>  		if (ptdev->fw->irq.irq)
>  			panthor_job_irq_suspend(&ptdev->fw->irq);
>  
> +		panthor_fw_halt_mcu(ptdev);
> +		if (!panthor_fw_wait_mcu_halted(ptdev))
> +			drm_warn(&ptdev->base, "Failed to halt MCU on unplug");
> +
>  		panthor_fw_stop(ptdev);
>  	}
>  
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.h b/drivers/gpu/drm/panthor/panthor_fw.h
> index 6598d96c6d2a..a19ed48b2d0b 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.h
> +++ b/drivers/gpu/drm/panthor/panthor_fw.h
> @@ -214,6 +214,13 @@ struct panthor_fw_global_input_iface {
>  #define GLB_FWCFG_UPDATE			BIT(9)
>  #define GLB_IDLE_EN				BIT(10)
>  #define GLB_SLEEP				BIT(12)
> +#define GLB_STATE_MASK				GENMASK(14, 12)
> +#define   GLB_STATE_ACTIVE			0
> +#define   GLB_STATE_HALT			1
> +#define   GLB_STATE_SLEEP			2
> +#define   GLB_STATE_SUSPEND			3
> +#define   GLB_STATE(x)				(((x) << 12) & GLB_STATE_MASK)
> +#define   GLB_STATE_GET(x)			(((x) & GLB_STATE_MASK) >> 12)
>  #define GLB_INACTIVE_COMPUTE			BIT(20)
>  #define GLB_INACTIVE_FRAGMENT			BIT(21)
>  #define GLB_INACTIVE_TILER			BIT(22)

[PATCH v1 09/10] drm/panthor: Support 64-bit endpoint_req register for Mali-G1

[Karunika Choo]

Add support for the 64-bit endpoint_req register introduced in CSF v4.0+
GPUs. Unlike a simple register widening, the 64-bit variant occupies the
next 64 bits after the original 32-bit field, requiring
version-dependent access.

This change introduces helper functions to read, write, and update the
endpoint_req register based on the CSF interface version, ensuring
correct handling on both pre-v4.0 and v4.0+ hardware.

Signed-off-by: Karunika Choo <karunika.choo@arm.com>
---
 drivers/gpu/drm/panthor/panthor_fw.c    | 43 ++++++++++++++++++++++---
 drivers/gpu/drm/panthor/panthor_fw.h    | 20 +++++++++++-
 drivers/gpu/drm/panthor/panthor_sched.c | 22 +++++++------
 3 files changed, 70 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
index 48bbae8931cb..c1b2fba311d8 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.c
+++ b/drivers/gpu/drm/panthor/panthor_fw.c
@@ -318,6 +318,41 @@ panthor_fw_get_cs_iface(struct panthor_device *ptdev, u32 csg_slot, u32 cs_slot)
 	return &ptdev->fw->iface.streams[csg_slot][cs_slot];
 }
 
+u64 panthor_fw_csg_endpoint_req_get(struct panthor_device *ptdev, u32 csg_id)
+{
+	struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
+	struct panthor_fw_csg_iface *csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
+
+	if (glb_iface->control->version >= CSF_IFACE_VERSION(4, 0, 0))
+		return csg_iface->input->endpoint_req2;
+	else
+		return csg_iface->input->endpoint_req;
+}
+
+void panthor_fw_csg_endpoint_req_set(struct panthor_device *ptdev, u32 csg_id, u64 value)
+{
+	struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
+	struct panthor_fw_csg_iface *csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
+
+	if (glb_iface->control->version >= CSF_IFACE_VERSION(4, 0, 0))
+		csg_iface->input->endpoint_req2 = value;
+	else
+		csg_iface->input->endpoint_req = lower_32_bits(value);
+}
+
+void panthor_fw_csg_endpoint_req_update(struct panthor_device *ptdev, u32 csg_id, u64 value,
+					u64 mask)
+{
+	struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
+	struct panthor_fw_csg_iface *csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
+
+	if (glb_iface->control->version >= CSF_IFACE_VERSION(4, 0, 0))
+		panthor_fw_update_reqs64(csg_iface, endpoint_req2, value, mask);
+	else
+		panthor_fw_update_reqs(csg_iface, endpoint_req, lower_32_bits(value),
+				       lower_32_bits(mask));
+}
+
 /**
  * panthor_fw_conv_timeout() - Convert a timeout into a cycle-count
  * @ptdev: Device.
@@ -997,7 +1032,7 @@ static void panthor_fw_init_global_iface(struct panthor_device *ptdev)
 					 GLB_IDLE_EN |
 					 GLB_IDLE;
 
-	if (panthor_fw_csf_version(ptdev) >= CSF_IFACE_VERSION(4, 1, 0))
+	if (glb_iface->control->version >= CSF_IFACE_VERSION(4, 1, 0))
 		glb_iface->input->ack_irq_mask |= GLB_STATE_MASK;
 
 	panthor_fw_update_reqs(glb_iface, req, GLB_IDLE_EN, GLB_IDLE_EN);
@@ -1080,7 +1115,7 @@ static bool panthor_fw_mcu_halted(struct panthor_device *ptdev)
 
 	halted = gpu_read(ptdev, MCU_STATUS) == MCU_STATUS_HALT;
 
-	if (panthor_fw_csf_version(ptdev) >= CSF_IFACE_VERSION(4, 1, 0))
+	if (glb_iface->control->version >= CSF_IFACE_VERSION(4, 1, 0))
 		halted &= (GLB_STATE_GET(glb_iface->output->ack) == GLB_STATE_HALT);
 
 	return halted;
@@ -1090,7 +1125,7 @@ static void panthor_fw_halt_mcu(struct panthor_device *ptdev)
 {
 	struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
 
-	if (panthor_fw_csf_version(ptdev) >= CSF_IFACE_VERSION(4, 1, 0))
+	if (glb_iface->control->version >= CSF_IFACE_VERSION(4, 1, 0))
 		panthor_fw_update_reqs(glb_iface, req, GLB_STATE(GLB_STATE_HALT), GLB_STATE_MASK);
 	else
 		panthor_fw_update_reqs(glb_iface, req, GLB_HALT, GLB_HALT);
@@ -1115,7 +1150,7 @@ static void panthor_fw_mcu_set_active(struct panthor_device *ptdev)
 {
 	struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
 
-	if (panthor_fw_csf_version(ptdev) >= CSF_IFACE_VERSION(4, 1, 0))
+	if (glb_iface->control->version >= CSF_IFACE_VERSION(4, 1, 0))
 		panthor_fw_update_reqs(glb_iface, req, GLB_STATE(GLB_STATE_ACTIVE), GLB_STATE_MASK);
 	else
 		panthor_fw_update_reqs(glb_iface, req, 0, GLB_HALT);
diff --git a/drivers/gpu/drm/panthor/panthor_fw.h b/drivers/gpu/drm/panthor/panthor_fw.h
index a19ed48b2d0b..25ebf0d31d0d 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.h
+++ b/drivers/gpu/drm/panthor/panthor_fw.h
@@ -168,9 +168,10 @@ struct panthor_fw_csg_input_iface {
 #define CSG_EP_REQ_EXCL_COMPUTE			BIT(20)
 #define CSG_EP_REQ_EXCL_FRAGMENT		BIT(21)
 #define CSG_EP_REQ_PRIORITY(x)			(((x) << 28) & GENMASK(31, 28))
+#define CSG_EP_REQ_PRIORITY_GET(x)		(((x) & GENMASK(31, 28)) >> 28)
 #define CSG_EP_REQ_PRIORITY_MASK		GENMASK(31, 28)
 	u32 endpoint_req;
-	u32 reserved2[2];
+	u64 endpoint_req2;
 	u64 suspend_buf;
 	u64 protm_suspend_buf;
 	u32 config;
@@ -464,6 +465,16 @@ struct panthor_fw_global_iface {
 		spin_unlock(&(__iface)->lock); \
 	} while (0)
 
+#define panthor_fw_update_reqs64(__iface, __in_reg, __val, __mask) \
+	do { \
+		u64 __cur_val, __new_val; \
+		spin_lock(&(__iface)->lock); \
+		__cur_val = READ_ONCE((__iface)->input->__in_reg); \
+		__new_val = (__cur_val & ~(__mask)) | ((__val) & (__mask)); \
+		WRITE_ONCE((__iface)->input->__in_reg, __new_val); \
+		spin_unlock(&(__iface)->lock); \
+	} while (0)
+
 struct panthor_fw_global_iface *
 panthor_fw_get_glb_iface(struct panthor_device *ptdev);
 
@@ -473,6 +484,13 @@ panthor_fw_get_csg_iface(struct panthor_device *ptdev, u32 csg_slot);
 struct panthor_fw_cs_iface *
 panthor_fw_get_cs_iface(struct panthor_device *ptdev, u32 csg_slot, u32 cs_slot);
 
+u64 panthor_fw_csg_endpoint_req_get(struct panthor_device *ptdev, u32 csg_id);
+
+void panthor_fw_csg_endpoint_req_set(struct panthor_device *ptdev, u32 csg_id, u64 value);
+
+void panthor_fw_csg_endpoint_req_update(struct panthor_device *ptdev, u32 csg_id, u64 value,
+					u64 mask);
+
 int panthor_fw_csg_wait_acks(struct panthor_device *ptdev, u32 csg_id, u32 req_mask,
 			     u32 *acked, u32 timeout_ms);
 
diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index 0cc9055f4ee5..25663de62b8e 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -1138,12 +1138,11 @@ static void
 csg_slot_sync_priority_locked(struct panthor_device *ptdev, u32 csg_id)
 {
 	struct panthor_csg_slot *csg_slot = &ptdev->scheduler->csg_slots[csg_id];
-	struct panthor_fw_csg_iface *csg_iface;
 
 	lockdep_assert_held(&ptdev->scheduler->lock);
 
-	csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
-	csg_slot->priority = (csg_iface->input->endpoint_req & CSG_EP_REQ_PRIORITY_MASK) >> 28;
+	csg_slot->priority =
+		CSG_EP_REQ_PRIORITY_GET(panthor_fw_csg_endpoint_req_get(ptdev, csg_id));
 }
 
 /**
@@ -1303,6 +1302,7 @@ csg_slot_prog_locked(struct panthor_device *ptdev, u32 csg_id, u32 priority)
 	struct panthor_csg_slot *csg_slot;
 	struct panthor_group *group;
 	u32 queue_mask = 0, i;
+	u32 endpoint_req = 0;
 
 	lockdep_assert_held(&ptdev->scheduler->lock);
 
@@ -1329,10 +1329,12 @@ csg_slot_prog_locked(struct panthor_device *ptdev, u32 csg_id, u32 priority)
 	csg_iface->input->allow_compute = group->compute_core_mask;
 	csg_iface->input->allow_fragment = group->fragment_core_mask;
 	csg_iface->input->allow_other = group->tiler_core_mask;
-	csg_iface->input->endpoint_req = CSG_EP_REQ_COMPUTE(group->max_compute_cores) |
-					 CSG_EP_REQ_FRAGMENT(group->max_fragment_cores) |
-					 CSG_EP_REQ_TILER(group->max_tiler_cores) |
-					 CSG_EP_REQ_PRIORITY(priority);
+	endpoint_req = CSG_EP_REQ_COMPUTE(group->max_compute_cores) |
+		       CSG_EP_REQ_FRAGMENT(group->max_fragment_cores) |
+		       CSG_EP_REQ_TILER(group->max_tiler_cores) |
+		       CSG_EP_REQ_PRIORITY(priority);
+	panthor_fw_csg_endpoint_req_set(ptdev, csg_id, endpoint_req);
+
 	csg_iface->input->config = panthor_vm_as(group->vm);
 
 	if (group->suspend_buf)
@@ -2230,9 +2232,9 @@ tick_ctx_apply(struct panthor_scheduler *sched, struct panthor_sched_tick_ctx *c
 				continue;
 			}
 
-			panthor_fw_update_reqs(csg_iface, endpoint_req,
-					       CSG_EP_REQ_PRIORITY(new_csg_prio),
-					       CSG_EP_REQ_PRIORITY_MASK);
+			panthor_fw_csg_endpoint_req_update(ptdev, csg_id,
+							   CSG_EP_REQ_PRIORITY(new_csg_prio),
+							   CSG_EP_REQ_PRIORITY_MASK);
 			csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, csg_id,
 						csg_iface->output->ack ^ CSG_ENDPOINT_CONFIG,
 						CSG_ENDPOINT_CONFIG);
-- 
2.49.0

Re: [PATCH v1 09/10] drm/panthor: Support 64-bit endpoint_req register for Mali-G1

[Steven Price]

On 14/10/2025 10:43, Karunika Choo wrote:
> Add support for the 64-bit endpoint_req register introduced in CSF v4.0+
> GPUs. Unlike a simple register widening, the 64-bit variant occupies the
> next 64 bits after the original 32-bit field, requiring
> version-dependent access.
> 
> This change introduces helper functions to read, write, and update the
> endpoint_req register based on the CSF interface version, ensuring
> correct handling on both pre-v4.0 and v4.0+ hardware.
> 
> Signed-off-by: Karunika Choo <karunika.choo@arm.com>
> ---
>  drivers/gpu/drm/panthor/panthor_fw.c    | 43 ++++++++++++++++++++++---
>  drivers/gpu/drm/panthor/panthor_fw.h    | 20 +++++++++++-
>  drivers/gpu/drm/panthor/panthor_sched.c | 22 +++++++------
>  3 files changed, 70 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index 48bbae8931cb..c1b2fba311d8 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> @@ -318,6 +318,41 @@ panthor_fw_get_cs_iface(struct panthor_device *ptdev, u32 csg_slot, u32 cs_slot)
>  	return &ptdev->fw->iface.streams[csg_slot][cs_slot];
>  }
>  
> +u64 panthor_fw_csg_endpoint_req_get(struct panthor_device *ptdev, u32 csg_id)
> +{
> +	struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
> +	struct panthor_fw_csg_iface *csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
> +
> +	if (glb_iface->control->version >= CSF_IFACE_VERSION(4, 0, 0))
> +		return csg_iface->input->endpoint_req2;
> +	else
> +		return csg_iface->input->endpoint_req;
> +}

I'm not sure it's an improvement taking csg_id rather than csg_iface.
csg_slot_sync_priority_locked() does get more simple, but the other
functions already have the csg_iface pointer so this is just repeated work.

We also really want a helper for the version check...

> +
> +void panthor_fw_csg_endpoint_req_set(struct panthor_device *ptdev, u32 csg_id, u64 value)
> +{
> +	struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
> +	struct panthor_fw_csg_iface *csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
> +
> +	if (glb_iface->control->version >= CSF_IFACE_VERSION(4, 0, 0))
> +		csg_iface->input->endpoint_req2 = value;
> +	else
> +		csg_iface->input->endpoint_req = lower_32_bits(value);
> +}
> +
> +void panthor_fw_csg_endpoint_req_update(struct panthor_device *ptdev, u32 csg_id, u64 value,
> +					u64 mask)
> +{
> +	struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
> +	struct panthor_fw_csg_iface *csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
> +
> +	if (glb_iface->control->version >= CSF_IFACE_VERSION(4, 0, 0))
> +		panthor_fw_update_reqs64(csg_iface, endpoint_req2, value, mask);
> +	else
> +		panthor_fw_update_reqs(csg_iface, endpoint_req, lower_32_bits(value),
> +				       lower_32_bits(mask));
> +}
> +
>  /**
>   * panthor_fw_conv_timeout() - Convert a timeout into a cycle-count
>   * @ptdev: Device.
> @@ -997,7 +1032,7 @@ static void panthor_fw_init_global_iface(struct panthor_device *ptdev)
>  					 GLB_IDLE_EN |
>  					 GLB_IDLE;
>  
> -	if (panthor_fw_csf_version(ptdev) >= CSF_IFACE_VERSION(4, 1, 0))
> +	if (glb_iface->control->version >= CSF_IFACE_VERSION(4, 1, 0))

These should be in the previous patch.

>  		glb_iface->input->ack_irq_mask |= GLB_STATE_MASK;
>  
>  	panthor_fw_update_reqs(glb_iface, req, GLB_IDLE_EN, GLB_IDLE_EN);
> @@ -1080,7 +1115,7 @@ static bool panthor_fw_mcu_halted(struct panthor_device *ptdev)
>  
>  	halted = gpu_read(ptdev, MCU_STATUS) == MCU_STATUS_HALT;
>  
> -	if (panthor_fw_csf_version(ptdev) >= CSF_IFACE_VERSION(4, 1, 0))
> +	if (glb_iface->control->version >= CSF_IFACE_VERSION(4, 1, 0))
>  		halted &= (GLB_STATE_GET(glb_iface->output->ack) == GLB_STATE_HALT);
>  
>  	return halted;
> @@ -1090,7 +1125,7 @@ static void panthor_fw_halt_mcu(struct panthor_device *ptdev)
>  {
>  	struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
>  
> -	if (panthor_fw_csf_version(ptdev) >= CSF_IFACE_VERSION(4, 1, 0))
> +	if (glb_iface->control->version >= CSF_IFACE_VERSION(4, 1, 0))
>  		panthor_fw_update_reqs(glb_iface, req, GLB_STATE(GLB_STATE_HALT), GLB_STATE_MASK);
>  	else
>  		panthor_fw_update_reqs(glb_iface, req, GLB_HALT, GLB_HALT);
> @@ -1115,7 +1150,7 @@ static void panthor_fw_mcu_set_active(struct panthor_device *ptdev)
>  {
>  	struct panthor_fw_global_iface *glb_iface = panthor_fw_get_glb_iface(ptdev);
>  
> -	if (panthor_fw_csf_version(ptdev) >= CSF_IFACE_VERSION(4, 1, 0))
> +	if (glb_iface->control->version >= CSF_IFACE_VERSION(4, 1, 0))
>  		panthor_fw_update_reqs(glb_iface, req, GLB_STATE(GLB_STATE_ACTIVE), GLB_STATE_MASK);
>  	else
>  		panthor_fw_update_reqs(glb_iface, req, 0, GLB_HALT);
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.h b/drivers/gpu/drm/panthor/panthor_fw.h
> index a19ed48b2d0b..25ebf0d31d0d 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.h
> +++ b/drivers/gpu/drm/panthor/panthor_fw.h
> @@ -168,9 +168,10 @@ struct panthor_fw_csg_input_iface {
>  #define CSG_EP_REQ_EXCL_COMPUTE			BIT(20)
>  #define CSG_EP_REQ_EXCL_FRAGMENT		BIT(21)
>  #define CSG_EP_REQ_PRIORITY(x)			(((x) << 28) & GENMASK(31, 28))
> +#define CSG_EP_REQ_PRIORITY_GET(x)		(((x) & GENMASK(31, 28)) >> 28)
>  #define CSG_EP_REQ_PRIORITY_MASK		GENMASK(31, 28)

NIT: if we reorder then we can avoid repeating GENMASK(31, 28).

>  	u32 endpoint_req;
> -	u32 reserved2[2];
> +	u64 endpoint_req2;
>  	u64 suspend_buf;
>  	u64 protm_suspend_buf;
>  	u32 config;
> @@ -464,6 +465,16 @@ struct panthor_fw_global_iface {
>  		spin_unlock(&(__iface)->lock); \
>  	} while (0)
>  
> +#define panthor_fw_update_reqs64(__iface, __in_reg, __val, __mask) \
> +	do { \
> +		u64 __cur_val, __new_val; \
> +		spin_lock(&(__iface)->lock); \
> +		__cur_val = READ_ONCE((__iface)->input->__in_reg); \
> +		__new_val = (__cur_val & ~(__mask)) | ((__val) & (__mask)); \
> +		WRITE_ONCE((__iface)->input->__in_reg, __new_val); \
> +		spin_unlock(&(__iface)->lock); \
> +	} while (0)
> +
>  struct panthor_fw_global_iface *
>  panthor_fw_get_glb_iface(struct panthor_device *ptdev);
>  
> @@ -473,6 +484,13 @@ panthor_fw_get_csg_iface(struct panthor_device *ptdev, u32 csg_slot);
>  struct panthor_fw_cs_iface *
>  panthor_fw_get_cs_iface(struct panthor_device *ptdev, u32 csg_slot, u32 cs_slot);
>  
> +u64 panthor_fw_csg_endpoint_req_get(struct panthor_device *ptdev, u32 csg_id);
> +
> +void panthor_fw_csg_endpoint_req_set(struct panthor_device *ptdev, u32 csg_id, u64 value);
> +
> +void panthor_fw_csg_endpoint_req_update(struct panthor_device *ptdev, u32 csg_id, u64 value,
> +					u64 mask);
> +
>  int panthor_fw_csg_wait_acks(struct panthor_device *ptdev, u32 csg_id, u32 req_mask,
>  			     u32 *acked, u32 timeout_ms);
>  
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 0cc9055f4ee5..25663de62b8e 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -1138,12 +1138,11 @@ static void
>  csg_slot_sync_priority_locked(struct panthor_device *ptdev, u32 csg_id)
>  {
>  	struct panthor_csg_slot *csg_slot = &ptdev->scheduler->csg_slots[csg_id];
> -	struct panthor_fw_csg_iface *csg_iface;
>  
>  	lockdep_assert_held(&ptdev->scheduler->lock);
>  
> -	csg_iface = panthor_fw_get_csg_iface(ptdev, csg_id);
> -	csg_slot->priority = (csg_iface->input->endpoint_req & CSG_EP_REQ_PRIORITY_MASK) >> 28;
> +	csg_slot->priority =
> +		CSG_EP_REQ_PRIORITY_GET(panthor_fw_csg_endpoint_req_get(ptdev, csg_id));

NIT: This is a useful refactor, but it's best to avoid refactoring at
the same time as making other changes as it's then harder to review (and
regressions are harder to pin down).

I also think a local variable would help with the line length:

  u64 endpoint_req = panthor_fw_csg_endpoint_req_get(ptdev, csg_id);

  csg_slot->priority = CSG_EP_REQ_PRIORITY_GET(endpoint_req);

>  }
>  
>  /**
> @@ -1303,6 +1302,7 @@ csg_slot_prog_locked(struct panthor_device *ptdev, u32 csg_id, u32 priority)
>  	struct panthor_csg_slot *csg_slot;
>  	struct panthor_group *group;
>  	u32 queue_mask = 0, i;
> +	u32 endpoint_req = 0;

No need to initialise (and doing so hides compiler warnings if this is
used when it shouldn't be).

It won't actually make any difference, but this should also be u64 to
match the new wider register.

Thanks,
Steve

>  
>  	lockdep_assert_held(&ptdev->scheduler->lock);
>  
> @@ -1329,10 +1329,12 @@ csg_slot_prog_locked(struct panthor_device *ptdev, u32 csg_id, u32 priority)
>  	csg_iface->input->allow_compute = group->compute_core_mask;
>  	csg_iface->input->allow_fragment = group->fragment_core_mask;
>  	csg_iface->input->allow_other = group->tiler_core_mask;
> -	csg_iface->input->endpoint_req = CSG_EP_REQ_COMPUTE(group->max_compute_cores) |
> -					 CSG_EP_REQ_FRAGMENT(group->max_fragment_cores) |
> -					 CSG_EP_REQ_TILER(group->max_tiler_cores) |
> -					 CSG_EP_REQ_PRIORITY(priority);
> +	endpoint_req = CSG_EP_REQ_COMPUTE(group->max_compute_cores) |
> +		       CSG_EP_REQ_FRAGMENT(group->max_fragment_cores) |
> +		       CSG_EP_REQ_TILER(group->max_tiler_cores) |
> +		       CSG_EP_REQ_PRIORITY(priority);
> +	panthor_fw_csg_endpoint_req_set(ptdev, csg_id, endpoint_req);
> +
>  	csg_iface->input->config = panthor_vm_as(group->vm);
>  
>  	if (group->suspend_buf)
> @@ -2230,9 +2232,9 @@ tick_ctx_apply(struct panthor_scheduler *sched, struct panthor_sched_tick_ctx *c
>  				continue;
>  			}
>  
> -			panthor_fw_update_reqs(csg_iface, endpoint_req,
> -					       CSG_EP_REQ_PRIORITY(new_csg_prio),
> -					       CSG_EP_REQ_PRIORITY_MASK);
> +			panthor_fw_csg_endpoint_req_update(ptdev, csg_id,
> +							   CSG_EP_REQ_PRIORITY(new_csg_prio),
> +							   CSG_EP_REQ_PRIORITY_MASK);
>  			csgs_upd_ctx_queue_reqs(ptdev, &upd_ctx, csg_id,
>  						csg_iface->output->ack ^ CSG_ENDPOINT_CONFIG,
>  						CSG_ENDPOINT_CONFIG);

[PATCH v1 10/10] drm/panthor: Add support for Mali-G1 GPUs

[Karunika Choo]

Add support for Mali-G1 GPUs (CSF architecture v14), introducing a new
panthor_hw_arch_v14 entry with reset and L2 power management operations
via the PWR_CONTROL block.

Mali-G1 introduces a dedicated PWR_CONTROL block for managing resets and
power domains. panthor_gpu_info_init() is updated to use this block for
L2, tiler, and shader domain present register reads.

Signed-off-by: Karunika Choo <karunika.choo@arm.com>
---
 drivers/gpu/drm/panthor/panthor_fw.c |  1 +
 drivers/gpu/drm/panthor/panthor_hw.c | 30 ++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
index c1b2fba311d8..dd5ffbea8cd8 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.c
+++ b/drivers/gpu/drm/panthor/panthor_fw.c
@@ -1493,3 +1493,4 @@ MODULE_FIRMWARE("arm/mali/arch10.12/mali_csffw.bin");
 MODULE_FIRMWARE("arm/mali/arch11.8/mali_csffw.bin");
 MODULE_FIRMWARE("arm/mali/arch12.8/mali_csffw.bin");
 MODULE_FIRMWARE("arm/mali/arch13.8/mali_csffw.bin");
+MODULE_FIRMWARE("arm/mali/arch14.8/mali_csffw.bin");
diff --git a/drivers/gpu/drm/panthor/panthor_hw.c b/drivers/gpu/drm/panthor/panthor_hw.c
index 77fd2c56e69f..7a47414d246e 100644
--- a/drivers/gpu/drm/panthor/panthor_hw.c
+++ b/drivers/gpu/drm/panthor/panthor_hw.c
@@ -4,6 +4,7 @@
 #include "panthor_device.h"
 #include "panthor_gpu.h"
 #include "panthor_hw.h"
+#include "panthor_pwr.h"
 #include "panthor_regs.h"
 
 #define GPU_PROD_ID_MAKE(arch_major, prod_major) \
@@ -29,12 +30,28 @@ static struct panthor_hw panthor_hw_arch_v10 = {
 	},
 };
 
+static struct panthor_hw panthor_hw_arch_v14 = {
+	.features = {
+		BIT(PANTHOR_HW_FEATURE_PWR_CONTROL)
+	},
+	.ops = {
+		.soft_reset = panthor_pwr_reset_soft,
+		.l2_power_off = panthor_pwr_l2_power_off,
+		.l2_power_on = panthor_pwr_l2_power_on,
+	},
+};
+
 static struct panthor_hw_entry panthor_hw_match[] = {
 	{
 		.arch_min = 10,
 		.arch_max = 13,
 		.hwdev = &panthor_hw_arch_v10,
 	},
+	{
+		.arch_min = 14,
+		.arch_max = 14,
+		.hwdev = &panthor_hw_arch_v14,
+	},
 };
 
 static char *get_gpu_model_name(struct panthor_device *ptdev)
@@ -82,6 +99,12 @@ static char *get_gpu_model_name(struct panthor_device *ptdev)
 		fallthrough;
 	case GPU_PROD_ID_MAKE(13, 1):
 		return "Mali-G625";
+	case GPU_PROD_ID_MAKE(14, 0):
+		return "Mali-G1-Ultra";
+	case GPU_PROD_ID_MAKE(14, 1):
+		return "Mali-G1-Premium";
+	case GPU_PROD_ID_MAKE(14, 3):
+		return "Mali-G1-Pro";
 	}
 
 	return "(Unknown Mali GPU)";
@@ -114,6 +137,13 @@ static void panthor_gpu_info_init(struct panthor_device *ptdev)
 
 	/* Introduced in arch 11.x */
 	ptdev->gpu_info.gpu_features = gpu_read64(ptdev, GPU_FEATURES);
+
+	/* Introduced in arch 14.x */
+	if (panthor_hw_has_feature(ptdev, PANTHOR_HW_FEATURE_PWR_CONTROL)) {
+		ptdev->gpu_info.l2_present = gpu_read64(ptdev, PWR_L2_PRESENT);
+		ptdev->gpu_info.tiler_present = gpu_read64(ptdev, PWR_TILER_PRESENT);
+		ptdev->gpu_info.shader_present = gpu_read64(ptdev, PWR_SHADER_PRESENT);
+	}
 }
 
 static void panthor_hw_info_init(struct panthor_device *ptdev)
-- 
2.49.0

Re: [PATCH v1 10/10] drm/panthor: Add support for Mali-G1 GPUs

[Steven Price]

On 14/10/2025 10:43, Karunika Choo wrote:
> Add support for Mali-G1 GPUs (CSF architecture v14), introducing a new
> panthor_hw_arch_v14 entry with reset and L2 power management operations
> via the PWR_CONTROL block.
> 
> Mali-G1 introduces a dedicated PWR_CONTROL block for managing resets and
> power domains. panthor_gpu_info_init() is updated to use this block for
> L2, tiler, and shader domain present register reads.
> 
> Signed-off-by: Karunika Choo <karunika.choo@arm.com>
> ---
>  drivers/gpu/drm/panthor/panthor_fw.c |  1 +
>  drivers/gpu/drm/panthor/panthor_hw.c | 30 ++++++++++++++++++++++++++++
>  2 files changed, 31 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
> index c1b2fba311d8..dd5ffbea8cd8 100644
> --- a/drivers/gpu/drm/panthor/panthor_fw.c
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
> @@ -1493,3 +1493,4 @@ MODULE_FIRMWARE("arm/mali/arch10.12/mali_csffw.bin");
>  MODULE_FIRMWARE("arm/mali/arch11.8/mali_csffw.bin");
>  MODULE_FIRMWARE("arm/mali/arch12.8/mali_csffw.bin");
>  MODULE_FIRMWARE("arm/mali/arch13.8/mali_csffw.bin");
> +MODULE_FIRMWARE("arm/mali/arch14.8/mali_csffw.bin");
> diff --git a/drivers/gpu/drm/panthor/panthor_hw.c b/drivers/gpu/drm/panthor/panthor_hw.c
> index 77fd2c56e69f..7a47414d246e 100644
> --- a/drivers/gpu/drm/panthor/panthor_hw.c
> +++ b/drivers/gpu/drm/panthor/panthor_hw.c
> @@ -4,6 +4,7 @@
>  #include "panthor_device.h"
>  #include "panthor_gpu.h"
>  #include "panthor_hw.h"
> +#include "panthor_pwr.h"
>  #include "panthor_regs.h"
>  
>  #define GPU_PROD_ID_MAKE(arch_major, prod_major) \
> @@ -29,12 +30,28 @@ static struct panthor_hw panthor_hw_arch_v10 = {
>  	},
>  };
>  
> +static struct panthor_hw panthor_hw_arch_v14 = {
> +	.features = {
> +		BIT(PANTHOR_HW_FEATURE_PWR_CONTROL)
> +	},
> +	.ops = {
> +		.soft_reset = panthor_pwr_reset_soft,
> +		.l2_power_off = panthor_pwr_l2_power_off,
> +		.l2_power_on = panthor_pwr_l2_power_on,
> +	},
> +};
> +
>  static struct panthor_hw_entry panthor_hw_match[] = {
>  	{
>  		.arch_min = 10,
>  		.arch_max = 13,
>  		.hwdev = &panthor_hw_arch_v10,
>  	},
> +	{
> +		.arch_min = 14,
> +		.arch_max = 14,
> +		.hwdev = &panthor_hw_arch_v14,
> +	},
>  };
>  
>  static char *get_gpu_model_name(struct panthor_device *ptdev)
> @@ -82,6 +99,12 @@ static char *get_gpu_model_name(struct panthor_device *ptdev)
>  		fallthrough;
>  	case GPU_PROD_ID_MAKE(13, 1):
>  		return "Mali-G625";
> +	case GPU_PROD_ID_MAKE(14, 0):
> +		return "Mali-G1-Ultra";
> +	case GPU_PROD_ID_MAKE(14, 1):
> +		return "Mali-G1-Premium";
> +	case GPU_PROD_ID_MAKE(14, 3):
> +		return "Mali-G1-Pro";
>  	}
>  
>  	return "(Unknown Mali GPU)";
> @@ -114,6 +137,13 @@ static void panthor_gpu_info_init(struct panthor_device *ptdev)
>  
>  	/* Introduced in arch 11.x */
>  	ptdev->gpu_info.gpu_features = gpu_read64(ptdev, GPU_FEATURES);
> +
> +	/* Introduced in arch 14.x */
> +	if (panthor_hw_has_feature(ptdev, PANTHOR_HW_FEATURE_PWR_CONTROL)) {
> +		ptdev->gpu_info.l2_present = gpu_read64(ptdev, PWR_L2_PRESENT);
> +		ptdev->gpu_info.tiler_present = gpu_read64(ptdev, PWR_TILER_PRESENT);
> +		ptdev->gpu_info.shader_present = gpu_read64(ptdev, PWR_SHADER_PRESENT);
> +	}

This should be instead of reading the GPU_SHADER_PRESENT etc registers.
At the moment we're reading a bunch of registers which are reserved on
the new GPUs, only to replace the values here.

Thanks,
Steve

>  }
>  
>  static void panthor_hw_info_init(struct panthor_device *ptdev)