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.0On 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);
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);
>
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);
>>
>
© 2016 - 2026 Red Hat, Inc.