diff mbox series

[1/5] perf/arm_cspmu: Simplify initialisation

Message ID 9842b71263a0a5e7ebb9aebb5f9ac15e1e24ff65.1701793996.git.robin.murphy@arm.com (mailing list archive)
State New, archived
Headers show
Series perf/arm_cspmu: Add devicetree support | expand

Commit Message

Robin Murphy Dec. 5, 2023, 4:51 p.m. UTC 
It's far simpler for implementations to literally override whichever
default ops they want to, by initialising to the default ops first. This
saves all the bother of checking what the impl_init_ops call has or
hasn't touched. Make the same clear distinction for the PMIIDR override
as well, in case we gain more sources for overriding that in future.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/perf/arm_cspmu/arm_cspmu.c    | 55 +++++++++++----------------
 drivers/perf/arm_cspmu/nvidia_cspmu.c |  6 ---
 2 files changed, 22 insertions(+), 39 deletions(-)

Comments

Ilkka Koskinen Dec. 7, 2023, 2:16 a.m. UTC | #1 
On Tue, 5 Dec 2023, Robin Murphy wrote:
> It's far simpler for implementations to literally override whichever
> default ops they want to, by initialising to the default ops first. This
> saves all the bother of checking what the impl_init_ops call has or
> hasn't touched. Make the same clear distinction for the PMIIDR override
> as well, in case we gain more sources for overriding that in future.
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Reviewed-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>

Cheers, Ilkka


> ---
> drivers/perf/arm_cspmu/arm_cspmu.c    | 55 +++++++++++----------------
> drivers/perf/arm_cspmu/nvidia_cspmu.c |  6 ---
> 2 files changed, 22 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index 2cc35dded007..a3347b1287e6 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -100,13 +100,6 @@
> #define ARM_CSPMU_ACTIVE_CPU_MASK		0x0
> #define ARM_CSPMU_ASSOCIATED_CPU_MASK		0x1
>
> -/* Check and use default if implementer doesn't provide attribute callback */
> -#define CHECK_DEFAULT_IMPL_OPS(ops, callback)			\
> -	do {							\
> -		if (!ops->callback)				\
> -			ops->callback = arm_cspmu_ ## callback;	\
> -	} while (0)
> -
> /*
>  * Maximum poll count for reading counter value using high-low-high sequence.
>  */
> @@ -408,21 +401,32 @@ static struct arm_cspmu_impl_match *arm_cspmu_impl_match_get(u32 pmiidr)
> 	return NULL;
> }
>
> +#define DEFAULT_IMPL_OP(name)	.name = arm_cspmu_##name
> +
> static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
> {
> 	int ret = 0;
> -	struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops;
> 	struct acpi_apmt_node *apmt_node = arm_cspmu_apmt_node(cspmu->dev);
> 	struct arm_cspmu_impl_match *match;
>
> -	/*
> -	 * Get PMU implementer and product id from APMT node.
> -	 * If APMT node doesn't have implementer/product id, try get it
> -	 * from PMIIDR.
> -	 */
> -	cspmu->impl.pmiidr =
> -		(apmt_node->impl_id) ? apmt_node->impl_id :
> -				       readl(cspmu->base0 + PMIIDR);
> +	/* Start with a default PMU implementation */
> +	cspmu->impl.module = THIS_MODULE;
> +	cspmu->impl.pmiidr = readl(cspmu->base0 + PMIIDR);
> +	cspmu->impl.ops = (struct arm_cspmu_impl_ops) {
> +		DEFAULT_IMPL_OP(get_event_attrs),
> +		DEFAULT_IMPL_OP(get_format_attrs),
> +		DEFAULT_IMPL_OP(get_identifier),
> +		DEFAULT_IMPL_OP(get_name),
> +		DEFAULT_IMPL_OP(is_cycle_counter_event),
> +		DEFAULT_IMPL_OP(event_type),
> +		DEFAULT_IMPL_OP(event_filter),
> +		DEFAULT_IMPL_OP(set_ev_filter),
> +		DEFAULT_IMPL_OP(event_attr_is_visible),
> +	};
> +
> +	/* Firmware may override implementer/product ID from PMIIDR */
> +	if (apmt_node->impl_id)
> +		cspmu->impl.pmiidr = apmt_node->impl_id;
>
> 	/* Find implementer specific attribute ops. */
> 	match = arm_cspmu_impl_match_get(cspmu->impl.pmiidr);
> @@ -450,24 +454,9 @@ static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
> 		}
>
> 		mutex_unlock(&arm_cspmu_lock);
> +	}
>
> -		if (ret)
> -			return ret;
> -	} else
> -		cspmu->impl.module = THIS_MODULE;
> -
> -	/* Use default callbacks if implementer doesn't provide one. */
> -	CHECK_DEFAULT_IMPL_OPS(impl_ops, get_event_attrs);
> -	CHECK_DEFAULT_IMPL_OPS(impl_ops, get_format_attrs);
> -	CHECK_DEFAULT_IMPL_OPS(impl_ops, get_identifier);
> -	CHECK_DEFAULT_IMPL_OPS(impl_ops, get_name);
> -	CHECK_DEFAULT_IMPL_OPS(impl_ops, is_cycle_counter_event);
> -	CHECK_DEFAULT_IMPL_OPS(impl_ops, event_type);
> -	CHECK_DEFAULT_IMPL_OPS(impl_ops, event_filter);
> -	CHECK_DEFAULT_IMPL_OPS(impl_ops, event_attr_is_visible);
> -	CHECK_DEFAULT_IMPL_OPS(impl_ops, set_ev_filter);
> -
> -	return 0;
> +	return ret;
> }
>
> static struct attribute_group *
> diff --git a/drivers/perf/arm_cspmu/nvidia_cspmu.c b/drivers/perf/arm_cspmu/nvidia_cspmu.c
> index 0382b702f092..5b84b701ad62 100644
> --- a/drivers/perf/arm_cspmu/nvidia_cspmu.c
> +++ b/drivers/perf/arm_cspmu/nvidia_cspmu.c
> @@ -388,12 +388,6 @@ static int nv_cspmu_init_ops(struct arm_cspmu *cspmu)
> 	impl_ops->get_format_attrs		= nv_cspmu_get_format_attrs;
> 	impl_ops->get_name			= nv_cspmu_get_name;
>
> -	/* Set others to NULL to use default callback. */
> -	impl_ops->event_type			= NULL;
> -	impl_ops->event_attr_is_visible		= NULL;
> -	impl_ops->get_identifier		= NULL;
> -	impl_ops->is_cycle_counter_event	= NULL;
> -
> 	return 0;
> }
>
> -- 
> 2.39.2.101.g768bb238c484.dirty
>
>
diff mbox series

Patch

diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index 2cc35dded007..a3347b1287e6 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c
@@ -100,13 +100,6 @@ 
 #define ARM_CSPMU_ACTIVE_CPU_MASK		0x0
 #define ARM_CSPMU_ASSOCIATED_CPU_MASK		0x1
 
-/* Check and use default if implementer doesn't provide attribute callback */
-#define CHECK_DEFAULT_IMPL_OPS(ops, callback)			\
-	do {							\
-		if (!ops->callback)				\
-			ops->callback = arm_cspmu_ ## callback;	\
-	} while (0)
-
 /*
  * Maximum poll count for reading counter value using high-low-high sequence.
  */
@@ -408,21 +401,32 @@  static struct arm_cspmu_impl_match *arm_cspmu_impl_match_get(u32 pmiidr)
 	return NULL;
 }
 
+#define DEFAULT_IMPL_OP(name)	.name = arm_cspmu_##name
+
 static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
 {
 	int ret = 0;
-	struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops;
 	struct acpi_apmt_node *apmt_node = arm_cspmu_apmt_node(cspmu->dev);
 	struct arm_cspmu_impl_match *match;
 
-	/*
-	 * Get PMU implementer and product id from APMT node.
-	 * If APMT node doesn't have implementer/product id, try get it
-	 * from PMIIDR.
-	 */
-	cspmu->impl.pmiidr =
-		(apmt_node->impl_id) ? apmt_node->impl_id :
-				       readl(cspmu->base0 + PMIIDR);
+	/* Start with a default PMU implementation */
+	cspmu->impl.module = THIS_MODULE;
+	cspmu->impl.pmiidr = readl(cspmu->base0 + PMIIDR);
+	cspmu->impl.ops = (struct arm_cspmu_impl_ops) {
+		DEFAULT_IMPL_OP(get_event_attrs),
+		DEFAULT_IMPL_OP(get_format_attrs),
+		DEFAULT_IMPL_OP(get_identifier),
+		DEFAULT_IMPL_OP(get_name),
+		DEFAULT_IMPL_OP(is_cycle_counter_event),
+		DEFAULT_IMPL_OP(event_type),
+		DEFAULT_IMPL_OP(event_filter),
+		DEFAULT_IMPL_OP(set_ev_filter),
+		DEFAULT_IMPL_OP(event_attr_is_visible),
+	};
+
+	/* Firmware may override implementer/product ID from PMIIDR */
+	if (apmt_node->impl_id)
+		cspmu->impl.pmiidr = apmt_node->impl_id;
 
 	/* Find implementer specific attribute ops. */
 	match = arm_cspmu_impl_match_get(cspmu->impl.pmiidr);
@@ -450,24 +454,9 @@  static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
 		}
 
 		mutex_unlock(&arm_cspmu_lock);
+	}
 
-		if (ret)
-			return ret;
-	} else
-		cspmu->impl.module = THIS_MODULE;
-
-	/* Use default callbacks if implementer doesn't provide one. */
-	CHECK_DEFAULT_IMPL_OPS(impl_ops, get_event_attrs);
-	CHECK_DEFAULT_IMPL_OPS(impl_ops, get_format_attrs);
-	CHECK_DEFAULT_IMPL_OPS(impl_ops, get_identifier);
-	CHECK_DEFAULT_IMPL_OPS(impl_ops, get_name);
-	CHECK_DEFAULT_IMPL_OPS(impl_ops, is_cycle_counter_event);
-	CHECK_DEFAULT_IMPL_OPS(impl_ops, event_type);
-	CHECK_DEFAULT_IMPL_OPS(impl_ops, event_filter);
-	CHECK_DEFAULT_IMPL_OPS(impl_ops, event_attr_is_visible);
-	CHECK_DEFAULT_IMPL_OPS(impl_ops, set_ev_filter);
-
-	return 0;
+	return ret;
 }
 
 static struct attribute_group *
diff --git a/drivers/perf/arm_cspmu/nvidia_cspmu.c b/drivers/perf/arm_cspmu/nvidia_cspmu.c
index 0382b702f092..5b84b701ad62 100644
--- a/drivers/perf/arm_cspmu/nvidia_cspmu.c
+++ b/drivers/perf/arm_cspmu/nvidia_cspmu.c
@@ -388,12 +388,6 @@  static int nv_cspmu_init_ops(struct arm_cspmu *cspmu)
 	impl_ops->get_format_attrs		= nv_cspmu_get_format_attrs;
 	impl_ops->get_name			= nv_cspmu_get_name;
 
-	/* Set others to NULL to use default callback. */
-	impl_ops->event_type			= NULL;
-	impl_ops->event_attr_is_visible		= NULL;
-	impl_ops->get_identifier		= NULL;
-	impl_ops->is_cycle_counter_event	= NULL;
-
 	return 0;
 }