[PATCH 0/4] perf: ampere: Add support for Ampere SoC PMUs

[PATCH 0/4] perf: ampere: Add support for Ampere SoC PMUs

[Ilkka Koskinen]

Changes since v3:
    * use_64b_counter_reg => has_atomic_dword (patch 1/4)
    * Removed the unnecessary hook for group validation (patch 3/4)
    * Added group config validation to ampere_cspmu_validate_event() (patch 4/4) 
    * Rebased the patchset

Changes since v2:
    * Changed to use supports_64bits_atomics() and replaced the split writes
      with lo_hi_writeq()
    * Added implementation specific group validation to patch 3
    * Dropped shared interrupt patch
    * Removed unnecessary filter_enable parameter from ampere module
    * Added group validation to ampere module

Changes since v1:
    * Rather than creating a completely new driver, implemented as a submodule
      of Arm CoreSight PMU driver
    * Fixed shared filter handling

Ilkka Koskinen (4):
  perf: arm_cspmu: Split 64-bit write to 32-bit writes
  perf: arm_cspmu: Support implementation specific filters
  perf: arm_cspmu: Support implementation specific validation
  perf: arm_cspmu: ampere_cspmu: Add support for Ampere SoC PMU

 .../admin-guide/perf/ampere_cspmu.rst         |  29 +++
 drivers/perf/arm_cspmu/Makefile               |   2 +-
 drivers/perf/arm_cspmu/ampere_cspmu.c         | 232 ++++++++++++++++++
 drivers/perf/arm_cspmu/ampere_cspmu.h         |  17 ++
 drivers/perf/arm_cspmu/arm_cspmu.c            |  28 ++-
 drivers/perf/arm_cspmu/arm_cspmu.h            |   6 +
 6 files changed, 309 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/admin-guide/perf/ampere_cspmu.rst
 create mode 100644 drivers/perf/arm_cspmu/ampere_cspmu.c
 create mode 100644 drivers/perf/arm_cspmu/ampere_cspmu.h

-- 
2.40.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 0/4] perf: ampere: Add support for Ampere SoC PMUs

[Ilkka Koskinen]

Changes since v3:
    * use_64b_counter_reg => has_atomic_dword (patch 1/4)
    * Removed the unnecessary hook for group validation (patch 3/4)
    * Added group config validation to ampere_cspmu_validate_event() (patch 4/4) 
    * Rebased the patchset

Changes since v2:
    * Changed to use supports_64bits_atomics() and replaced the split writes
      with lo_hi_writeq()
    * Added implementation specific group validation to patch 3
    * Dropped shared interrupt patch
    * Removed unnecessary filter_enable parameter from ampere module
    * Added group validation to ampere module

Changes since v1:
    * Rather than creating a completely new driver, implemented as a submodule
      of Arm CoreSight PMU driver
    * Fixed shared filter handling

Ilkka Koskinen (4):
  perf: arm_cspmu: Split 64-bit write to 32-bit writes
  perf: arm_cspmu: Support implementation specific filters
  perf: arm_cspmu: Support implementation specific validation
  perf: arm_cspmu: ampere_cspmu: Add support for Ampere SoC PMU

 .../admin-guide/perf/ampere_cspmu.rst         |  29 +++
 drivers/perf/arm_cspmu/Makefile               |   2 +-
 drivers/perf/arm_cspmu/ampere_cspmu.c         | 232 ++++++++++++++++++
 drivers/perf/arm_cspmu/ampere_cspmu.h         |  17 ++
 drivers/perf/arm_cspmu/arm_cspmu.c            |  28 ++-
 drivers/perf/arm_cspmu/arm_cspmu.h            |   6 +
 6 files changed, 309 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/admin-guide/perf/ampere_cspmu.rst
 create mode 100644 drivers/perf/arm_cspmu/ampere_cspmu.c
 create mode 100644 drivers/perf/arm_cspmu/ampere_cspmu.h

-- 
2.40.1

[PATCH 1/4] perf: arm_cspmu: Split 64-bit write to 32-bit writes

[Ilkka Koskinen]

Split the 64-bit register accesses if 64-bit access is not supported
by the PMU.

Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
---
 drivers/perf/arm_cspmu/arm_cspmu.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index e2b7827c4563..0f517152cb4e 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c
@@ -696,7 +696,10 @@ static void arm_cspmu_write_counter(struct perf_event *event, u64 val)
 	if (use_64b_counter_reg(cspmu)) {
 		offset = counter_offset(sizeof(u64), event->hw.idx);
 
-		writeq(val, cspmu->base1 + offset);
+		if (cspmu->has_atomic_dword)
+			writeq(val, cspmu->base1 + offset);
+		else
+			lo_hi_writeq(val, cspmu->base1 + offset);
 	} else {
 		offset = counter_offset(sizeof(u32), event->hw.idx);
 
-- 
2.40.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/4] perf: arm_cspmu: Split 64-bit write to 32-bit writes

[Ilkka Koskinen]

Split the 64-bit register accesses if 64-bit access is not supported
by the PMU.

Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
---
 drivers/perf/arm_cspmu/arm_cspmu.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index e2b7827c4563..0f517152cb4e 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c
@@ -696,7 +696,10 @@ static void arm_cspmu_write_counter(struct perf_event *event, u64 val)
 	if (use_64b_counter_reg(cspmu)) {
 		offset = counter_offset(sizeof(u64), event->hw.idx);
 
-		writeq(val, cspmu->base1 + offset);
+		if (cspmu->has_atomic_dword)
+			writeq(val, cspmu->base1 + offset);
+		else
+			lo_hi_writeq(val, cspmu->base1 + offset);
 	} else {
 		offset = counter_offset(sizeof(u32), event->hw.idx);
 
-- 
2.40.1

RE: [PATCH 1/4] perf: arm_cspmu: Split 64-bit write to 32-bit writes

[Besar Wicaksono]

> -----Original Message-----
> From: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> Sent: Thursday, June 22, 2023 8:12 AM
> To: Will Deacon <will@kernel.org>; Robin Murphy <robin.murphy@arm.com>;
> Besar Wicaksono <bwicaksono@nvidia.com>; Suzuki K Poulose
> <suzuki.poulose@arm.com>; Mark Rutland <mark.rutland@arm.com>
> Cc: Ilkka Koskinen <ilkka@os.amperecomputing.com>; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 1/4] perf: arm_cspmu: Split 64-bit write to 32-bit writes
> 
> External email: Use caution opening links or attachments
> 
> 
> Split the 64-bit register accesses if 64-bit access is not supported
> by the PMU.
> 
> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>

Reviewed-by: Besar Wicaksono <bwicaksono@nvidia.com>

Thanks,
Besar

> ---
>  drivers/perf/arm_cspmu/arm_cspmu.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c
> b/drivers/perf/arm_cspmu/arm_cspmu.c
> index e2b7827c4563..0f517152cb4e 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -696,7 +696,10 @@ static void arm_cspmu_write_counter(struct
> perf_event *event, u64 val)
>         if (use_64b_counter_reg(cspmu)) {
>                 offset = counter_offset(sizeof(u64), event->hw.idx);
> 
> -               writeq(val, cspmu->base1 + offset);
> +               if (cspmu->has_atomic_dword)
> +                       writeq(val, cspmu->base1 + offset);
> +               else
> +                       lo_hi_writeq(val, cspmu->base1 + offset);
>         } else {
>                 offset = counter_offset(sizeof(u32), event->hw.idx);
> 
> --
> 2.40.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 1/4] perf: arm_cspmu: Split 64-bit write to 32-bit writes

[Besar Wicaksono]

> -----Original Message-----
> From: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> Sent: Thursday, June 22, 2023 8:12 AM
> To: Will Deacon <will@kernel.org>; Robin Murphy <robin.murphy@arm.com>;
> Besar Wicaksono <bwicaksono@nvidia.com>; Suzuki K Poulose
> <suzuki.poulose@arm.com>; Mark Rutland <mark.rutland@arm.com>
> Cc: Ilkka Koskinen <ilkka@os.amperecomputing.com>; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [PATCH 1/4] perf: arm_cspmu: Split 64-bit write to 32-bit writes
> 
> External email: Use caution opening links or attachments
> 
> 
> Split the 64-bit register accesses if 64-bit access is not supported
> by the PMU.
> 
> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>

Reviewed-by: Besar Wicaksono <bwicaksono@nvidia.com>

Thanks,
Besar

> ---
>  drivers/perf/arm_cspmu/arm_cspmu.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c
> b/drivers/perf/arm_cspmu/arm_cspmu.c
> index e2b7827c4563..0f517152cb4e 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -696,7 +696,10 @@ static void arm_cspmu_write_counter(struct
> perf_event *event, u64 val)
>         if (use_64b_counter_reg(cspmu)) {
>                 offset = counter_offset(sizeof(u64), event->hw.idx);
> 
> -               writeq(val, cspmu->base1 + offset);
> +               if (cspmu->has_atomic_dword)
> +                       writeq(val, cspmu->base1 + offset);
> +               else
> +                       lo_hi_writeq(val, cspmu->base1 + offset);
>         } else {
>                 offset = counter_offset(sizeof(u32), event->hw.idx);
> 
> --
> 2.40.1

[PATCH 2/4] perf: arm_cspmu: Support implementation specific filters

[Ilkka Koskinen]

Generic filters aren't used in all the platforms. Instead, the platforms
may use different means to filter events. Add support for implementation
specific filters.

Reviewed-by: Besar Wicaksono <bwicaksono@nvidia.com>
Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
---
 drivers/perf/arm_cspmu/arm_cspmu.c | 8 ++++++--
 drivers/perf/arm_cspmu/arm_cspmu.h | 3 +++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index 0f517152cb4e..fafd734c3218 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c
@@ -117,6 +117,9 @@
 
 static unsigned long arm_cspmu_cpuhp_state;
 
+static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
+				    struct hw_perf_event *hwc, u32 filter);
+
 static struct acpi_apmt_node *arm_cspmu_apmt_node(struct device *dev)
 {
 	return *(struct acpi_apmt_node **)dev_get_platdata(dev);
@@ -426,6 +429,7 @@ static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
 	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;
 }
@@ -792,7 +796,7 @@ static inline void arm_cspmu_set_event(struct arm_cspmu *cspmu,
 	writel(hwc->config, cspmu->base0 + offset);
 }
 
-static inline void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
+static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
 					   struct hw_perf_event *hwc,
 					   u32 filter)
 {
@@ -826,7 +830,7 @@ static void arm_cspmu_start(struct perf_event *event, int pmu_flags)
 		arm_cspmu_set_cc_filter(cspmu, filter);
 	} else {
 		arm_cspmu_set_event(cspmu, hwc);
-		arm_cspmu_set_ev_filter(cspmu, hwc, filter);
+		cspmu->impl.ops.set_ev_filter(cspmu, hwc, filter);
 	}
 
 	hwc->state = 0;
diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h b/drivers/perf/arm_cspmu/arm_cspmu.h
index 83df53d1c132..d6d88c246a23 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.h
+++ b/drivers/perf/arm_cspmu/arm_cspmu.h
@@ -101,6 +101,9 @@ struct arm_cspmu_impl_ops {
 	u32 (*event_type)(const struct perf_event *event);
 	/* Decode filter value from configs */
 	u32 (*event_filter)(const struct perf_event *event);
+	/* Set event filter */
+	void (*set_ev_filter)(struct arm_cspmu *cspmu,
+			      struct hw_perf_event *hwc, u32 filter);
 	/* Hide/show unsupported events */
 	umode_t (*event_attr_is_visible)(struct kobject *kobj,
 					 struct attribute *attr, int unused);
-- 
2.40.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/4] perf: arm_cspmu: Support implementation specific filters

[Ilkka Koskinen]

Generic filters aren't used in all the platforms. Instead, the platforms
may use different means to filter events. Add support for implementation
specific filters.

Reviewed-by: Besar Wicaksono <bwicaksono@nvidia.com>
Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
---
 drivers/perf/arm_cspmu/arm_cspmu.c | 8 ++++++--
 drivers/perf/arm_cspmu/arm_cspmu.h | 3 +++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index 0f517152cb4e..fafd734c3218 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c
@@ -117,6 +117,9 @@
 
 static unsigned long arm_cspmu_cpuhp_state;
 
+static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
+				    struct hw_perf_event *hwc, u32 filter);
+
 static struct acpi_apmt_node *arm_cspmu_apmt_node(struct device *dev)
 {
 	return *(struct acpi_apmt_node **)dev_get_platdata(dev);
@@ -426,6 +429,7 @@ static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
 	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;
 }
@@ -792,7 +796,7 @@ static inline void arm_cspmu_set_event(struct arm_cspmu *cspmu,
 	writel(hwc->config, cspmu->base0 + offset);
 }
 
-static inline void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
+static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
 					   struct hw_perf_event *hwc,
 					   u32 filter)
 {
@@ -826,7 +830,7 @@ static void arm_cspmu_start(struct perf_event *event, int pmu_flags)
 		arm_cspmu_set_cc_filter(cspmu, filter);
 	} else {
 		arm_cspmu_set_event(cspmu, hwc);
-		arm_cspmu_set_ev_filter(cspmu, hwc, filter);
+		cspmu->impl.ops.set_ev_filter(cspmu, hwc, filter);
 	}
 
 	hwc->state = 0;
diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h b/drivers/perf/arm_cspmu/arm_cspmu.h
index 83df53d1c132..d6d88c246a23 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.h
+++ b/drivers/perf/arm_cspmu/arm_cspmu.h
@@ -101,6 +101,9 @@ struct arm_cspmu_impl_ops {
 	u32 (*event_type)(const struct perf_event *event);
 	/* Decode filter value from configs */
 	u32 (*event_filter)(const struct perf_event *event);
+	/* Set event filter */
+	void (*set_ev_filter)(struct arm_cspmu *cspmu,
+			      struct hw_perf_event *hwc, u32 filter);
 	/* Hide/show unsupported events */
 	umode_t (*event_attr_is_visible)(struct kobject *kobj,
 					 struct attribute *attr, int unused);
-- 
2.40.1

Re: [PATCH 2/4] perf: arm_cspmu: Support implementation specific filters

[Jonathan Cameron]

On Wed, 21 Jun 2023 18:11:39 -0700
Ilkka Koskinen <ilkka@os.amperecomputing.com> wrote:

> Generic filters aren't used in all the platforms. Instead, the platforms
> may use different means to filter events. Add support for implementation
> specific filters.

If the specification allows explicitly for non standard ways of controlling filters
it would be good to add a specification reference to this.

Otherwise one question inline.
> 
> Reviewed-by: Besar Wicaksono <bwicaksono@nvidia.com>
> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> ---
>  drivers/perf/arm_cspmu/arm_cspmu.c | 8 ++++++--
>  drivers/perf/arm_cspmu/arm_cspmu.h | 3 +++
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index 0f517152cb4e..fafd734c3218 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -117,6 +117,9 @@
>  
>  static unsigned long arm_cspmu_cpuhp_state;
>  
> +static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
> +				    struct hw_perf_event *hwc, u32 filter);
> +
>  static struct acpi_apmt_node *arm_cspmu_apmt_node(struct device *dev)
>  {
>  	return *(struct acpi_apmt_node **)dev_get_platdata(dev);
> @@ -426,6 +429,7 @@ static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
>  	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;
>  }
> @@ -792,7 +796,7 @@ static inline void arm_cspmu_set_event(struct arm_cspmu *cspmu,
>  	writel(hwc->config, cspmu->base0 + offset);
>  }
>  
> -static inline void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
> +static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
>  					   struct hw_perf_event *hwc,
>  					   u32 filter)
>  {
> @@ -826,7 +830,7 @@ static void arm_cspmu_start(struct perf_event *event, int pmu_flags)
>  		arm_cspmu_set_cc_filter(cspmu, filter);
>  	} else {
>  		arm_cspmu_set_event(cspmu, hwc);
> -		arm_cspmu_set_ev_filter(cspmu, hwc, filter);
> +		cspmu->impl.ops.set_ev_filter(cspmu, hwc, filter);

Optional callback so don't you need either provide a default, or check
it isn't null?

>  	}
>  
>  	hwc->state = 0;
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h b/drivers/perf/arm_cspmu/arm_cspmu.h
> index 83df53d1c132..d6d88c246a23 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.h
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.h
> @@ -101,6 +101,9 @@ struct arm_cspmu_impl_ops {
>  	u32 (*event_type)(const struct perf_event *event);
>  	/* Decode filter value from configs */
>  	u32 (*event_filter)(const struct perf_event *event);
> +	/* Set event filter */
> +	void (*set_ev_filter)(struct arm_cspmu *cspmu,
> +			      struct hw_perf_event *hwc, u32 filter);
>  	/* Hide/show unsupported events */
>  	umode_t (*event_attr_is_visible)(struct kobject *kobj,
>  					 struct attribute *attr, int unused);


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/4] perf: arm_cspmu: Support implementation specific filters

[Jonathan Cameron]

On Wed, 21 Jun 2023 18:11:39 -0700
Ilkka Koskinen <ilkka@os.amperecomputing.com> wrote:

> Generic filters aren't used in all the platforms. Instead, the platforms
> may use different means to filter events. Add support for implementation
> specific filters.

If the specification allows explicitly for non standard ways of controlling filters
it would be good to add a specification reference to this.

Otherwise one question inline.
> 
> Reviewed-by: Besar Wicaksono <bwicaksono@nvidia.com>
> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> ---
>  drivers/perf/arm_cspmu/arm_cspmu.c | 8 ++++++--
>  drivers/perf/arm_cspmu/arm_cspmu.h | 3 +++
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index 0f517152cb4e..fafd734c3218 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -117,6 +117,9 @@
>  
>  static unsigned long arm_cspmu_cpuhp_state;
>  
> +static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
> +				    struct hw_perf_event *hwc, u32 filter);
> +
>  static struct acpi_apmt_node *arm_cspmu_apmt_node(struct device *dev)
>  {
>  	return *(struct acpi_apmt_node **)dev_get_platdata(dev);
> @@ -426,6 +429,7 @@ static int arm_cspmu_init_impl_ops(struct arm_cspmu *cspmu)
>  	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;
>  }
> @@ -792,7 +796,7 @@ static inline void arm_cspmu_set_event(struct arm_cspmu *cspmu,
>  	writel(hwc->config, cspmu->base0 + offset);
>  }
>  
> -static inline void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
> +static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
>  					   struct hw_perf_event *hwc,
>  					   u32 filter)
>  {
> @@ -826,7 +830,7 @@ static void arm_cspmu_start(struct perf_event *event, int pmu_flags)
>  		arm_cspmu_set_cc_filter(cspmu, filter);
>  	} else {
>  		arm_cspmu_set_event(cspmu, hwc);
> -		arm_cspmu_set_ev_filter(cspmu, hwc, filter);
> +		cspmu->impl.ops.set_ev_filter(cspmu, hwc, filter);

Optional callback so don't you need either provide a default, or check
it isn't null?

>  	}
>  
>  	hwc->state = 0;
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h b/drivers/perf/arm_cspmu/arm_cspmu.h
> index 83df53d1c132..d6d88c246a23 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.h
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.h
> @@ -101,6 +101,9 @@ struct arm_cspmu_impl_ops {
>  	u32 (*event_type)(const struct perf_event *event);
>  	/* Decode filter value from configs */
>  	u32 (*event_filter)(const struct perf_event *event);
> +	/* Set event filter */
> +	void (*set_ev_filter)(struct arm_cspmu *cspmu,
> +			      struct hw_perf_event *hwc, u32 filter);
>  	/* Hide/show unsupported events */
>  	umode_t (*event_attr_is_visible)(struct kobject *kobj,
>  					 struct attribute *attr, int unused);

RE: [PATCH 2/4] perf: arm_cspmu: Support implementation specific filters

[Besar Wicaksono]

> -----Original Message-----
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Sent: Thursday, June 22, 2023 3:34 PM
> To: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> Cc: Will Deacon <will@kernel.org>; Robin Murphy <robin.murphy@arm.com>;
> Besar Wicaksono <bwicaksono@nvidia.com>; Suzuki K Poulose
> <suzuki.poulose@arm.com>; Mark Rutland <mark.rutland@arm.com>; linux-
> arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/4] perf: arm_cspmu: Support implementation specific
> filters
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, 21 Jun 2023 18:11:39 -0700
> Ilkka Koskinen <ilkka@os.amperecomputing.com> wrote:
> 
> > Generic filters aren't used in all the platforms. Instead, the platforms
> > may use different means to filter events. Add support for implementation
> > specific filters.
> 
> If the specification allows explicitly for non standard ways of controlling filters
> it would be good to add a specification reference to this.
> 

Want to point out that the spec considers PMEVTYPER and PMEVFILTR* registers as optional,
please refer to section 2.1 in the spec. The spec also defines PMIMPDEF registers (starting
from offset 0xD80), which is intended for implementation defined extension. My interpretation
to this is implementer can have other methods to configure event selection and filtering, although
maybe not clear of how much freedom is given to the implementer. 

> Otherwise one question inline.
> >
> > Reviewed-by: Besar Wicaksono <bwicaksono@nvidia.com>
> > Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> > ---
> >  drivers/perf/arm_cspmu/arm_cspmu.c | 8 ++++++--
> >  drivers/perf/arm_cspmu/arm_cspmu.h | 3 +++
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c
> b/drivers/perf/arm_cspmu/arm_cspmu.c
> > index 0f517152cb4e..fafd734c3218 100644
> > --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> > +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> > @@ -117,6 +117,9 @@
> >
> >  static unsigned long arm_cspmu_cpuhp_state;
> >
> > +static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
> > +                                 struct hw_perf_event *hwc, u32 filter);
> > +
> >  static struct acpi_apmt_node *arm_cspmu_apmt_node(struct device *dev)
> >  {
> >       return *(struct acpi_apmt_node **)dev_get_platdata(dev);
> > @@ -426,6 +429,7 @@ static int arm_cspmu_init_impl_ops(struct
> arm_cspmu *cspmu)
> >       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;
> >  }
> > @@ -792,7 +796,7 @@ static inline void arm_cspmu_set_event(struct
> arm_cspmu *cspmu,
> >       writel(hwc->config, cspmu->base0 + offset);
> >  }
> >
> > -static inline void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
> > +static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
> >                                          struct hw_perf_event *hwc,
> >                                          u32 filter)
> >  {
> > @@ -826,7 +830,7 @@ static void arm_cspmu_start(struct perf_event
> *event, int pmu_flags)
> >               arm_cspmu_set_cc_filter(cspmu, filter);
> >       } else {
> >               arm_cspmu_set_event(cspmu, hwc);
> > -             arm_cspmu_set_ev_filter(cspmu, hwc, filter);
> > +             cspmu->impl.ops.set_ev_filter(cspmu, hwc, filter);
> 
> Optional callback so don't you need either provide a default, or check
> it isn't null?
> 

Right, the CHECK_DEFAULT_IMPL_OPS(impl_ops, set_ev_filter) above will fallback
to default if ops.set_ev_filter is null.

Regards,
Besar

> >       }
> >
> >       hwc->state = 0;
> > diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h
> b/drivers/perf/arm_cspmu/arm_cspmu.h
> > index 83df53d1c132..d6d88c246a23 100644
> > --- a/drivers/perf/arm_cspmu/arm_cspmu.h
> > +++ b/drivers/perf/arm_cspmu/arm_cspmu.h
> > @@ -101,6 +101,9 @@ struct arm_cspmu_impl_ops {
> >       u32 (*event_type)(const struct perf_event *event);
> >       /* Decode filter value from configs */
> >       u32 (*event_filter)(const struct perf_event *event);
> > +     /* Set event filter */
> > +     void (*set_ev_filter)(struct arm_cspmu *cspmu,
> > +                           struct hw_perf_event *hwc, u32 filter);
> >       /* Hide/show unsupported events */
> >       umode_t (*event_attr_is_visible)(struct kobject *kobj,
> >                                        struct attribute *attr, int unused);


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 2/4] perf: arm_cspmu: Support implementation specific filters

[Besar Wicaksono]

> -----Original Message-----
> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
> Sent: Thursday, June 22, 2023 3:34 PM
> To: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> Cc: Will Deacon <will@kernel.org>; Robin Murphy <robin.murphy@arm.com>;
> Besar Wicaksono <bwicaksono@nvidia.com>; Suzuki K Poulose
> <suzuki.poulose@arm.com>; Mark Rutland <mark.rutland@arm.com>; linux-
> arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/4] perf: arm_cspmu: Support implementation specific
> filters
> 
> External email: Use caution opening links or attachments
> 
> 
> On Wed, 21 Jun 2023 18:11:39 -0700
> Ilkka Koskinen <ilkka@os.amperecomputing.com> wrote:
> 
> > Generic filters aren't used in all the platforms. Instead, the platforms
> > may use different means to filter events. Add support for implementation
> > specific filters.
> 
> If the specification allows explicitly for non standard ways of controlling filters
> it would be good to add a specification reference to this.
> 

Want to point out that the spec considers PMEVTYPER and PMEVFILTR* registers as optional,
please refer to section 2.1 in the spec. The spec also defines PMIMPDEF registers (starting
from offset 0xD80), which is intended for implementation defined extension. My interpretation
to this is implementer can have other methods to configure event selection and filtering, although
maybe not clear of how much freedom is given to the implementer. 

> Otherwise one question inline.
> >
> > Reviewed-by: Besar Wicaksono <bwicaksono@nvidia.com>
> > Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> > ---
> >  drivers/perf/arm_cspmu/arm_cspmu.c | 8 ++++++--
> >  drivers/perf/arm_cspmu/arm_cspmu.h | 3 +++
> >  2 files changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c
> b/drivers/perf/arm_cspmu/arm_cspmu.c
> > index 0f517152cb4e..fafd734c3218 100644
> > --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> > +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> > @@ -117,6 +117,9 @@
> >
> >  static unsigned long arm_cspmu_cpuhp_state;
> >
> > +static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
> > +                                 struct hw_perf_event *hwc, u32 filter);
> > +
> >  static struct acpi_apmt_node *arm_cspmu_apmt_node(struct device *dev)
> >  {
> >       return *(struct acpi_apmt_node **)dev_get_platdata(dev);
> > @@ -426,6 +429,7 @@ static int arm_cspmu_init_impl_ops(struct
> arm_cspmu *cspmu)
> >       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;
> >  }
> > @@ -792,7 +796,7 @@ static inline void arm_cspmu_set_event(struct
> arm_cspmu *cspmu,
> >       writel(hwc->config, cspmu->base0 + offset);
> >  }
> >
> > -static inline void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
> > +static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
> >                                          struct hw_perf_event *hwc,
> >                                          u32 filter)
> >  {
> > @@ -826,7 +830,7 @@ static void arm_cspmu_start(struct perf_event
> *event, int pmu_flags)
> >               arm_cspmu_set_cc_filter(cspmu, filter);
> >       } else {
> >               arm_cspmu_set_event(cspmu, hwc);
> > -             arm_cspmu_set_ev_filter(cspmu, hwc, filter);
> > +             cspmu->impl.ops.set_ev_filter(cspmu, hwc, filter);
> 
> Optional callback so don't you need either provide a default, or check
> it isn't null?
> 

Right, the CHECK_DEFAULT_IMPL_OPS(impl_ops, set_ev_filter) above will fallback
to default if ops.set_ev_filter is null.

Regards,
Besar

> >       }
> >
> >       hwc->state = 0;
> > diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h
> b/drivers/perf/arm_cspmu/arm_cspmu.h
> > index 83df53d1c132..d6d88c246a23 100644
> > --- a/drivers/perf/arm_cspmu/arm_cspmu.h
> > +++ b/drivers/perf/arm_cspmu/arm_cspmu.h
> > @@ -101,6 +101,9 @@ struct arm_cspmu_impl_ops {
> >       u32 (*event_type)(const struct perf_event *event);
> >       /* Decode filter value from configs */
> >       u32 (*event_filter)(const struct perf_event *event);
> > +     /* Set event filter */
> > +     void (*set_ev_filter)(struct arm_cspmu *cspmu,
> > +                           struct hw_perf_event *hwc, u32 filter);
> >       /* Hide/show unsupported events */
> >       umode_t (*event_attr_is_visible)(struct kobject *kobj,
> >                                        struct attribute *attr, int unused);

RE: [PATCH 2/4] perf: arm_cspmu: Support implementation specific filters

[Ilkka Koskinen]

On Thu, 22 Jun 2023, Besar Wicaksono wrote:

>> -----Original Message-----
>> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
>> Sent: Thursday, June 22, 2023 3:34 PM
>> To: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>> Cc: Will Deacon <will@kernel.org>; Robin Murphy <robin.murphy@arm.com>;
>> Besar Wicaksono <bwicaksono@nvidia.com>; Suzuki K Poulose
>> <suzuki.poulose@arm.com>; Mark Rutland <mark.rutland@arm.com>; linux-
>> arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH 2/4] perf: arm_cspmu: Support implementation specific
>> filters
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On Wed, 21 Jun 2023 18:11:39 -0700
>> Ilkka Koskinen <ilkka@os.amperecomputing.com> wrote:
>>
>>> Generic filters aren't used in all the platforms. Instead, the platforms
>>> may use different means to filter events. Add support for implementation
>>> specific filters.
>>
>> If the specification allows explicitly for non standard ways of controlling filters
>> it would be good to add a specification reference to this.
>>
>
> Want to point out that the spec considers PMEVTYPER and PMEVFILTR* registers as optional,
> please refer to section 2.1 in the spec. The spec also defines PMIMPDEF registers (starting
> from offset 0xD80), which is intended for implementation defined extension. My interpretation
> to this is implementer can have other methods to configure event selection and filtering, although
> maybe not clear of how much freedom is given to the implementer.

That's a good idea. I do that.

Cheers, Ilkka

>
>> Otherwise one question inline.
>>>
>>> Reviewed-by: Besar Wicaksono <bwicaksono@nvidia.com>
>>> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>>> ---
>>>  drivers/perf/arm_cspmu/arm_cspmu.c | 8 ++++++--
>>>  drivers/perf/arm_cspmu/arm_cspmu.h | 3 +++
>>>  2 files changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c
>> b/drivers/perf/arm_cspmu/arm_cspmu.c
>>> index 0f517152cb4e..fafd734c3218 100644
>>> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
>>> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
>>> @@ -117,6 +117,9 @@
>>>
>>>  static unsigned long arm_cspmu_cpuhp_state;
>>>
>>> +static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
>>> +                                 struct hw_perf_event *hwc, u32 filter);
>>> +
>>>  static struct acpi_apmt_node *arm_cspmu_apmt_node(struct device *dev)
>>>  {
>>>       return *(struct acpi_apmt_node **)dev_get_platdata(dev);
>>> @@ -426,6 +429,7 @@ static int arm_cspmu_init_impl_ops(struct
>> arm_cspmu *cspmu)
>>>       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;
>>>  }
>>> @@ -792,7 +796,7 @@ static inline void arm_cspmu_set_event(struct
>> arm_cspmu *cspmu,
>>>       writel(hwc->config, cspmu->base0 + offset);
>>>  }
>>>
>>> -static inline void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
>>> +static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
>>>                                          struct hw_perf_event *hwc,
>>>                                          u32 filter)
>>>  {
>>> @@ -826,7 +830,7 @@ static void arm_cspmu_start(struct perf_event
>> *event, int pmu_flags)
>>>               arm_cspmu_set_cc_filter(cspmu, filter);
>>>       } else {
>>>               arm_cspmu_set_event(cspmu, hwc);
>>> -             arm_cspmu_set_ev_filter(cspmu, hwc, filter);
>>> +             cspmu->impl.ops.set_ev_filter(cspmu, hwc, filter);
>>
>> Optional callback so don't you need either provide a default, or check
>> it isn't null?
>>
>
> Right, the CHECK_DEFAULT_IMPL_OPS(impl_ops, set_ev_filter) above will fallback
> to default if ops.set_ev_filter is null.
>
> Regards,
> Besar
>
>>>       }
>>>
>>>       hwc->state = 0;
>>> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h
>> b/drivers/perf/arm_cspmu/arm_cspmu.h
>>> index 83df53d1c132..d6d88c246a23 100644
>>> --- a/drivers/perf/arm_cspmu/arm_cspmu.h
>>> +++ b/drivers/perf/arm_cspmu/arm_cspmu.h
>>> @@ -101,6 +101,9 @@ struct arm_cspmu_impl_ops {
>>>       u32 (*event_type)(const struct perf_event *event);
>>>       /* Decode filter value from configs */
>>>       u32 (*event_filter)(const struct perf_event *event);
>>> +     /* Set event filter */
>>> +     void (*set_ev_filter)(struct arm_cspmu *cspmu,
>>> +                           struct hw_perf_event *hwc, u32 filter);
>>>       /* Hide/show unsupported events */
>>>       umode_t (*event_attr_is_visible)(struct kobject *kobj,
>>>                                        struct attribute *attr, int unused);
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 2/4] perf: arm_cspmu: Support implementation specific filters

[Ilkka Koskinen]

On Thu, 22 Jun 2023, Besar Wicaksono wrote:

>> -----Original Message-----
>> From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
>> Sent: Thursday, June 22, 2023 3:34 PM
>> To: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>> Cc: Will Deacon <will@kernel.org>; Robin Murphy <robin.murphy@arm.com>;
>> Besar Wicaksono <bwicaksono@nvidia.com>; Suzuki K Poulose
>> <suzuki.poulose@arm.com>; Mark Rutland <mark.rutland@arm.com>; linux-
>> arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH 2/4] perf: arm_cspmu: Support implementation specific
>> filters
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On Wed, 21 Jun 2023 18:11:39 -0700
>> Ilkka Koskinen <ilkka@os.amperecomputing.com> wrote:
>>
>>> Generic filters aren't used in all the platforms. Instead, the platforms
>>> may use different means to filter events. Add support for implementation
>>> specific filters.
>>
>> If the specification allows explicitly for non standard ways of controlling filters
>> it would be good to add a specification reference to this.
>>
>
> Want to point out that the spec considers PMEVTYPER and PMEVFILTR* registers as optional,
> please refer to section 2.1 in the spec. The spec also defines PMIMPDEF registers (starting
> from offset 0xD80), which is intended for implementation defined extension. My interpretation
> to this is implementer can have other methods to configure event selection and filtering, although
> maybe not clear of how much freedom is given to the implementer.

That's a good idea. I do that.

Cheers, Ilkka

>
>> Otherwise one question inline.
>>>
>>> Reviewed-by: Besar Wicaksono <bwicaksono@nvidia.com>
>>> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
>>> ---
>>>  drivers/perf/arm_cspmu/arm_cspmu.c | 8 ++++++--
>>>  drivers/perf/arm_cspmu/arm_cspmu.h | 3 +++
>>>  2 files changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c
>> b/drivers/perf/arm_cspmu/arm_cspmu.c
>>> index 0f517152cb4e..fafd734c3218 100644
>>> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
>>> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
>>> @@ -117,6 +117,9 @@
>>>
>>>  static unsigned long arm_cspmu_cpuhp_state;
>>>
>>> +static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
>>> +                                 struct hw_perf_event *hwc, u32 filter);
>>> +
>>>  static struct acpi_apmt_node *arm_cspmu_apmt_node(struct device *dev)
>>>  {
>>>       return *(struct acpi_apmt_node **)dev_get_platdata(dev);
>>> @@ -426,6 +429,7 @@ static int arm_cspmu_init_impl_ops(struct
>> arm_cspmu *cspmu)
>>>       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;
>>>  }
>>> @@ -792,7 +796,7 @@ static inline void arm_cspmu_set_event(struct
>> arm_cspmu *cspmu,
>>>       writel(hwc->config, cspmu->base0 + offset);
>>>  }
>>>
>>> -static inline void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
>>> +static void arm_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
>>>                                          struct hw_perf_event *hwc,
>>>                                          u32 filter)
>>>  {
>>> @@ -826,7 +830,7 @@ static void arm_cspmu_start(struct perf_event
>> *event, int pmu_flags)
>>>               arm_cspmu_set_cc_filter(cspmu, filter);
>>>       } else {
>>>               arm_cspmu_set_event(cspmu, hwc);
>>> -             arm_cspmu_set_ev_filter(cspmu, hwc, filter);
>>> +             cspmu->impl.ops.set_ev_filter(cspmu, hwc, filter);
>>
>> Optional callback so don't you need either provide a default, or check
>> it isn't null?
>>
>
> Right, the CHECK_DEFAULT_IMPL_OPS(impl_ops, set_ev_filter) above will fallback
> to default if ops.set_ev_filter is null.
>
> Regards,
> Besar
>
>>>       }
>>>
>>>       hwc->state = 0;
>>> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h
>> b/drivers/perf/arm_cspmu/arm_cspmu.h
>>> index 83df53d1c132..d6d88c246a23 100644
>>> --- a/drivers/perf/arm_cspmu/arm_cspmu.h
>>> +++ b/drivers/perf/arm_cspmu/arm_cspmu.h
>>> @@ -101,6 +101,9 @@ struct arm_cspmu_impl_ops {
>>>       u32 (*event_type)(const struct perf_event *event);
>>>       /* Decode filter value from configs */
>>>       u32 (*event_filter)(const struct perf_event *event);
>>> +     /* Set event filter */
>>> +     void (*set_ev_filter)(struct arm_cspmu *cspmu,
>>> +                           struct hw_perf_event *hwc, u32 filter);
>>>       /* Hide/show unsupported events */
>>>       umode_t (*event_attr_is_visible)(struct kobject *kobj,
>>>                                        struct attribute *attr, int unused);
>
>

[PATCH 3/4] perf: arm_cspmu: Support implementation specific validation

[Ilkka Koskinen]

Some platforms may use e.g. different filtering mechanism and, thus,
may need different way to validate the events and group.

Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
---
 drivers/perf/arm_cspmu/arm_cspmu.c | 8 +++++++-
 drivers/perf/arm_cspmu/arm_cspmu.h | 3 +++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index fafd734c3218..471d6d7ac81a 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c
@@ -553,7 +553,7 @@ static void arm_cspmu_disable(struct pmu *pmu)
 static int arm_cspmu_get_event_idx(struct arm_cspmu_hw_events *hw_events,
 				struct perf_event *event)
 {
-	int idx;
+	int idx, ret;
 	struct arm_cspmu *cspmu = to_arm_cspmu(event->pmu);
 
 	if (supports_cycle_counter(cspmu)) {
@@ -587,6 +587,12 @@ static int arm_cspmu_get_event_idx(struct arm_cspmu_hw_events *hw_events,
 	if (idx >= cspmu->num_logical_ctrs)
 		return -EAGAIN;
 
+	if (cspmu->impl.ops.validate_event) {
+		ret = cspmu->impl.ops.validate_event(cspmu, event);
+		if (ret)
+			return ret;
+	}
+
 	set_bit(idx, hw_events->used_ctrs);
 
 	return idx;
diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h b/drivers/perf/arm_cspmu/arm_cspmu.h
index d6d88c246a23..5070d34c2285 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.h
+++ b/drivers/perf/arm_cspmu/arm_cspmu.h
@@ -104,6 +104,9 @@ struct arm_cspmu_impl_ops {
 	/* Set event filter */
 	void (*set_ev_filter)(struct arm_cspmu *cspmu,
 			      struct hw_perf_event *hwc, u32 filter);
+	/* Implementation specific event validation */
+	int (*validate_event)(struct arm_cspmu *cspmu,
+			      struct perf_event *event);
 	/* Hide/show unsupported events */
 	umode_t (*event_attr_is_visible)(struct kobject *kobj,
 					 struct attribute *attr, int unused);
-- 
2.40.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/4] perf: arm_cspmu: Support implementation specific validation

[Ilkka Koskinen]

Some platforms may use e.g. different filtering mechanism and, thus,
may need different way to validate the events and group.

Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
---
 drivers/perf/arm_cspmu/arm_cspmu.c | 8 +++++++-
 drivers/perf/arm_cspmu/arm_cspmu.h | 3 +++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index fafd734c3218..471d6d7ac81a 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c
@@ -553,7 +553,7 @@ static void arm_cspmu_disable(struct pmu *pmu)
 static int arm_cspmu_get_event_idx(struct arm_cspmu_hw_events *hw_events,
 				struct perf_event *event)
 {
-	int idx;
+	int idx, ret;
 	struct arm_cspmu *cspmu = to_arm_cspmu(event->pmu);
 
 	if (supports_cycle_counter(cspmu)) {
@@ -587,6 +587,12 @@ static int arm_cspmu_get_event_idx(struct arm_cspmu_hw_events *hw_events,
 	if (idx >= cspmu->num_logical_ctrs)
 		return -EAGAIN;
 
+	if (cspmu->impl.ops.validate_event) {
+		ret = cspmu->impl.ops.validate_event(cspmu, event);
+		if (ret)
+			return ret;
+	}
+
 	set_bit(idx, hw_events->used_ctrs);
 
 	return idx;
diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h b/drivers/perf/arm_cspmu/arm_cspmu.h
index d6d88c246a23..5070d34c2285 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.h
+++ b/drivers/perf/arm_cspmu/arm_cspmu.h
@@ -104,6 +104,9 @@ struct arm_cspmu_impl_ops {
 	/* Set event filter */
 	void (*set_ev_filter)(struct arm_cspmu *cspmu,
 			      struct hw_perf_event *hwc, u32 filter);
+	/* Implementation specific event validation */
+	int (*validate_event)(struct arm_cspmu *cspmu,
+			      struct perf_event *event);
 	/* Hide/show unsupported events */
 	umode_t (*event_attr_is_visible)(struct kobject *kobj,
 					 struct attribute *attr, int unused);
-- 
2.40.1

Re: [PATCH 3/4] perf: arm_cspmu: Support implementation specific validation

[Jonathan Cameron]

On Wed, 21 Jun 2023 18:11:40 -0700
Ilkka Koskinen <ilkka@os.amperecomputing.com> wrote:

> Some platforms may use e.g. different filtering mechanism and, thus,
> may need different way to validate the events and group.
> 
> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
FWIW given it's so simple...
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/perf/arm_cspmu/arm_cspmu.c | 8 +++++++-
>  drivers/perf/arm_cspmu/arm_cspmu.h | 3 +++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index fafd734c3218..471d6d7ac81a 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -553,7 +553,7 @@ static void arm_cspmu_disable(struct pmu *pmu)
>  static int arm_cspmu_get_event_idx(struct arm_cspmu_hw_events *hw_events,
>  				struct perf_event *event)
>  {
> -	int idx;
> +	int idx, ret;
>  	struct arm_cspmu *cspmu = to_arm_cspmu(event->pmu);
>  
>  	if (supports_cycle_counter(cspmu)) {
> @@ -587,6 +587,12 @@ static int arm_cspmu_get_event_idx(struct arm_cspmu_hw_events *hw_events,
>  	if (idx >= cspmu->num_logical_ctrs)
>  		return -EAGAIN;
>  
> +	if (cspmu->impl.ops.validate_event) {
> +		ret = cspmu->impl.ops.validate_event(cspmu, event);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	set_bit(idx, hw_events->used_ctrs);
>  
>  	return idx;
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h b/drivers/perf/arm_cspmu/arm_cspmu.h
> index d6d88c246a23..5070d34c2285 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.h
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.h
> @@ -104,6 +104,9 @@ struct arm_cspmu_impl_ops {
>  	/* Set event filter */
>  	void (*set_ev_filter)(struct arm_cspmu *cspmu,
>  			      struct hw_perf_event *hwc, u32 filter);
> +	/* Implementation specific event validation */
> +	int (*validate_event)(struct arm_cspmu *cspmu,
> +			      struct perf_event *event);
>  	/* Hide/show unsupported events */
>  	umode_t (*event_attr_is_visible)(struct kobject *kobj,
>  					 struct attribute *attr, int unused);


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/4] perf: arm_cspmu: Support implementation specific validation

[Jonathan Cameron]

On Wed, 21 Jun 2023 18:11:40 -0700
Ilkka Koskinen <ilkka@os.amperecomputing.com> wrote:

> Some platforms may use e.g. different filtering mechanism and, thus,
> may need different way to validate the events and group.
> 
> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
FWIW given it's so simple...
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/perf/arm_cspmu/arm_cspmu.c | 8 +++++++-
>  drivers/perf/arm_cspmu/arm_cspmu.h | 3 +++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index fafd734c3218..471d6d7ac81a 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -553,7 +553,7 @@ static void arm_cspmu_disable(struct pmu *pmu)
>  static int arm_cspmu_get_event_idx(struct arm_cspmu_hw_events *hw_events,
>  				struct perf_event *event)
>  {
> -	int idx;
> +	int idx, ret;
>  	struct arm_cspmu *cspmu = to_arm_cspmu(event->pmu);
>  
>  	if (supports_cycle_counter(cspmu)) {
> @@ -587,6 +587,12 @@ static int arm_cspmu_get_event_idx(struct arm_cspmu_hw_events *hw_events,
>  	if (idx >= cspmu->num_logical_ctrs)
>  		return -EAGAIN;
>  
> +	if (cspmu->impl.ops.validate_event) {
> +		ret = cspmu->impl.ops.validate_event(cspmu, event);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	set_bit(idx, hw_events->used_ctrs);
>  
>  	return idx;
> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.h b/drivers/perf/arm_cspmu/arm_cspmu.h
> index d6d88c246a23..5070d34c2285 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.h
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.h
> @@ -104,6 +104,9 @@ struct arm_cspmu_impl_ops {
>  	/* Set event filter */
>  	void (*set_ev_filter)(struct arm_cspmu *cspmu,
>  			      struct hw_perf_event *hwc, u32 filter);
> +	/* Implementation specific event validation */
> +	int (*validate_event)(struct arm_cspmu *cspmu,
> +			      struct perf_event *event);
>  	/* Hide/show unsupported events */
>  	umode_t (*event_attr_is_visible)(struct kobject *kobj,
>  					 struct attribute *attr, int unused);

[PATCH 4/4] perf: arm_cspmu: ampere_cspmu: Add support for Ampere SoC PMU

[Ilkka Koskinen]

Ampere SoC PMU follows CoreSight PMU architecture. It uses implementation
specific registers to filter events rather than PMEVFILTnR registers.

Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
---
 .../admin-guide/perf/ampere_cspmu.rst         |  29 +++
 drivers/perf/arm_cspmu/Makefile               |   2 +-
 drivers/perf/arm_cspmu/ampere_cspmu.c         | 232 ++++++++++++++++++
 drivers/perf/arm_cspmu/ampere_cspmu.h         |  17 ++
 drivers/perf/arm_cspmu/arm_cspmu.c            |   7 +
 5 files changed, 286 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/admin-guide/perf/ampere_cspmu.rst
 create mode 100644 drivers/perf/arm_cspmu/ampere_cspmu.c
 create mode 100644 drivers/perf/arm_cspmu/ampere_cspmu.h

diff --git a/Documentation/admin-guide/perf/ampere_cspmu.rst b/Documentation/admin-guide/perf/ampere_cspmu.rst
new file mode 100644
index 000000000000..bf86bffeef63
--- /dev/null
+++ b/Documentation/admin-guide/perf/ampere_cspmu.rst
@@ -0,0 +1,29 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+============================================
+Ampere SoC Performance Monitoring Unit (PMU)
+============================================
+
+Ampere SoC PMU is a generic PMU IP that follows Arm CoreSight PMU architecture.
+Therefore, the driver is implemented as a submodule of arm_cspmu driver. At the
+first phase it's used for counting MCU events on AmpereOne.
+
+
+MCU PMU events
+--------------
+
+The PMU driver supports setting filters for "rank", "bank", and "threshold".
+Note, that the filters are per PMU instance rather than per event.
+
+
+Example for perf tool use::
+
+  / # perf list ampere
+
+    ampere_mcu_pmu_0/act_sent/                         [Kernel PMU event]
+    <...>
+    ampere_mcu_pmu_1/rd_sent/                          [Kernel PMU event]
+    <...>
+
+  / # perf stat -a -e ampere_mcu_pmu_0/act_sent,filter_enable=3,bank=5,rank=3,threshold=2/,ampere_mcu_pmu_1/rd_sent/ \
+        sleep 1
diff --git a/drivers/perf/arm_cspmu/Makefile b/drivers/perf/arm_cspmu/Makefile
index fedb17df982d..b80a8bd8da54 100644
--- a/drivers/perf/arm_cspmu/Makefile
+++ b/drivers/perf/arm_cspmu/Makefile
@@ -3,4 +3,4 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU) += arm_cspmu_module.o
-arm_cspmu_module-y := arm_cspmu.o nvidia_cspmu.o
+arm_cspmu_module-y := arm_cspmu.o nvidia_cspmu.o ampere_cspmu.o
diff --git a/drivers/perf/arm_cspmu/ampere_cspmu.c b/drivers/perf/arm_cspmu/ampere_cspmu.c
new file mode 100644
index 000000000000..62dc893c94c1
--- /dev/null
+++ b/drivers/perf/arm_cspmu/ampere_cspmu.c
@@ -0,0 +1,232 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Ampere SoC PMU (Performance Monitor Unit)
+ *
+ * Copyright (c) 2023, Ampere Computing LLC
+ */
+
+#include "ampere_cspmu.h"
+
+#define PMAUXR0		0xD80
+#define PMAUXR1		0xD84
+#define PMAUXR2		0xD88
+#define PMAUXR3		0xD8C
+
+#define to_ampere_cspmu_ctx(cspmu)	((struct ampere_cspmu_ctx *)(cspmu->impl.ctx))
+
+struct ampere_cspmu_ctx {
+	const char *name;
+	struct attribute **event_attr;
+	struct attribute **format_attr;
+};
+
+#define SOC_PMU_EVENT_ATTR_EXTRACTOR(_name, _config, _start, _end)        \
+	static inline u32 get_##_name(const struct perf_event *event)     \
+	{                                                                 \
+		return FIELD_GET(GENMASK_ULL(_end, _start),               \
+				 event->attr._config);                    \
+	}                                                                 \
+
+SOC_PMU_EVENT_ATTR_EXTRACTOR(event, config, 0, 8);
+SOC_PMU_EVENT_ATTR_EXTRACTOR(threshold, config1, 0, 7);
+SOC_PMU_EVENT_ATTR_EXTRACTOR(rank, config1, 8, 23);
+SOC_PMU_EVENT_ATTR_EXTRACTOR(bank, config2, 0, 31);
+
+static struct attribute *ampereone_mcu_pmu_event_attrs[] = {
+	ARM_CSPMU_EVENT_ATTR(cycle_count,		0x00),
+	ARM_CSPMU_EVENT_ATTR(act_sent,			0x01),
+	ARM_CSPMU_EVENT_ATTR(pre_sent,			0x02),
+	ARM_CSPMU_EVENT_ATTR(rd_sent,			0x03),
+	ARM_CSPMU_EVENT_ATTR(rda_sent,			0x04),
+	ARM_CSPMU_EVENT_ATTR(wr_sent,			0x05),
+	ARM_CSPMU_EVENT_ATTR(wra_sent,			0x06),
+	ARM_CSPMU_EVENT_ATTR(pd_entry_vld,		0x07),
+	ARM_CSPMU_EVENT_ATTR(sref_entry_vld,		0x08),
+	ARM_CSPMU_EVENT_ATTR(prea_sent,			0x09),
+	ARM_CSPMU_EVENT_ATTR(pre_sb_sent,		0x0a),
+	ARM_CSPMU_EVENT_ATTR(ref_sent,			0x0b),
+	ARM_CSPMU_EVENT_ATTR(rfm_sent,			0x0c),
+	ARM_CSPMU_EVENT_ATTR(ref_sb_sent,		0x0d),
+	ARM_CSPMU_EVENT_ATTR(rfm_sb_sent,		0x0e),
+	ARM_CSPMU_EVENT_ATTR(rd_rda_sent,		0x0f),
+	ARM_CSPMU_EVENT_ATTR(wr_wra_sent,		0x10),
+	ARM_CSPMU_EVENT_ATTR(raw_hazard,		0x11),
+	ARM_CSPMU_EVENT_ATTR(war_hazard,		0x12),
+	ARM_CSPMU_EVENT_ATTR(waw_hazard,		0x13),
+	ARM_CSPMU_EVENT_ATTR(rar_hazard,		0x14),
+	ARM_CSPMU_EVENT_ATTR(raw_war_waw_hazard,	0x15),
+	ARM_CSPMU_EVENT_ATTR(hprd_lprd_wr_req_vld,	0x16),
+	ARM_CSPMU_EVENT_ATTR(lprd_req_vld,		0x17),
+	ARM_CSPMU_EVENT_ATTR(hprd_req_vld,		0x18),
+	ARM_CSPMU_EVENT_ATTR(hprd_lprd_req_vld,		0x19),
+	ARM_CSPMU_EVENT_ATTR(prefetch_tgt,		0x1a),
+	ARM_CSPMU_EVENT_ATTR(wr_req_vld,		0x1b),
+	ARM_CSPMU_EVENT_ATTR(partial_wr_req_vld,	0x1c),
+	ARM_CSPMU_EVENT_ATTR(rd_retry,			0x1d),
+	ARM_CSPMU_EVENT_ATTR(wr_retry,			0x1e),
+	ARM_CSPMU_EVENT_ATTR(retry_gnt,			0x1f),
+	ARM_CSPMU_EVENT_ATTR(rank_change,		0x20),
+	ARM_CSPMU_EVENT_ATTR(dir_change,		0x21),
+	ARM_CSPMU_EVENT_ATTR(rank_dir_change,		0x22),
+	ARM_CSPMU_EVENT_ATTR(rank_active,		0x23),
+	ARM_CSPMU_EVENT_ATTR(rank_idle,			0x24),
+	ARM_CSPMU_EVENT_ATTR(rank_pd,			0x25),
+	ARM_CSPMU_EVENT_ATTR(rank_sref,			0x26),
+	ARM_CSPMU_EVENT_ATTR(queue_fill_gt_thresh,	0x27),
+	ARM_CSPMU_EVENT_ATTR(queue_rds_gt_thresh,	0x28),
+	ARM_CSPMU_EVENT_ATTR(queue_wrs_gt_thresh,	0x29),
+	ARM_CSPMU_EVENT_ATTR(phy_updt_complt,		0x2a),
+	ARM_CSPMU_EVENT_ATTR(tz_fail,			0x2b),
+	ARM_CSPMU_EVENT_ATTR(dram_errc,			0x2c),
+	ARM_CSPMU_EVENT_ATTR(dram_errd,			0x2d),
+	ARM_CSPMU_EVENT_ATTR(read_data_return,		0x32),
+	ARM_CSPMU_EVENT_ATTR(chi_wr_data_delta,		0x33),
+	ARM_CSPMU_EVENT_ATTR(zq_start,			0x34),
+	ARM_CSPMU_EVENT_ATTR(zq_latch,			0x35),
+	ARM_CSPMU_EVENT_ATTR(wr_fifo_full,		0x36),
+	ARM_CSPMU_EVENT_ATTR(info_fifo_full,		0x37),
+	ARM_CSPMU_EVENT_ATTR(cmd_fifo_full,		0x38),
+	ARM_CSPMU_EVENT_ATTR(dfi_nop,			0x39),
+	ARM_CSPMU_EVENT_ATTR(dfi_cmd,			0x3a),
+	ARM_CSPMU_EVENT_ATTR(rd_run_len,		0x3b),
+	ARM_CSPMU_EVENT_ATTR(wr_run_len,		0x3c),
+
+	ARM_CSPMU_EVENT_ATTR(cycles, ARM_CSPMU_EVT_CYCLES_DEFAULT),
+	NULL,
+};
+
+static struct attribute *ampereone_mcu_format_attrs[] = {
+	ARM_CSPMU_FORMAT_EVENT_ATTR,
+	ARM_CSPMU_FORMAT_ATTR(threshold, "config1:0-7"),
+	ARM_CSPMU_FORMAT_ATTR(rank, "config1:8-23"),
+	ARM_CSPMU_FORMAT_ATTR(bank, "config2:0-31"),
+	NULL,
+};
+
+static struct attribute **
+ampere_cspmu_get_event_attrs(const struct arm_cspmu *cspmu)
+{
+	const struct ampere_cspmu_ctx *ctx = to_ampere_cspmu_ctx(cspmu);
+
+	return ctx->event_attr;
+}
+
+static struct attribute **
+ampere_cspmu_get_format_attrs(const struct arm_cspmu *cspmu)
+{
+	const struct ampere_cspmu_ctx *ctx = to_ampere_cspmu_ctx(cspmu);
+
+	return ctx->format_attr;
+}
+
+static const char *
+ampere_cspmu_get_name(const struct arm_cspmu *cspmu)
+{
+	const struct ampere_cspmu_ctx *ctx = to_ampere_cspmu_ctx(cspmu);
+
+	return ctx->name;
+}
+
+static u32 ampere_cspmu_event_filter(const struct perf_event *event)
+{
+	return 0;
+}
+
+static void ampere_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
+				       struct hw_perf_event *hwc,
+				       u32 filter)
+{
+	struct perf_event *event;
+	unsigned int idx;
+	u32 threshold, rank, bank;
+
+	/*
+	 * At this point, all the events have the same filter settings.
+	 * Therefore, take the first event and use its configuration.
+	 */
+	idx = find_first_bit(cspmu->hw_events.used_ctrs,
+			     cspmu->cycle_counter_logical_idx);
+
+	event = cspmu->hw_events.events[idx];
+
+	threshold	= get_threshold(event);
+	rank		= get_rank(event);
+	bank		= get_bank(event);
+
+	writel(threshold, cspmu->base0 + PMAUXR0);
+	writel(rank, cspmu->base0 + PMAUXR1);
+	writel(bank, cspmu->base0 + PMAUXR2);
+}
+
+static int ampere_cspmu_validate_configs(struct perf_event *event,
+					 struct perf_event *event2)
+{
+	if (get_threshold(event) != get_threshold(event2) ||
+	    get_rank(event) != get_rank(event2) ||
+	    get_bank(event) != get_bank(event2))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int ampere_cspmu_validate_event(struct arm_cspmu *cspmu,
+				       struct perf_event *new)
+{
+	struct perf_event *curr, *leader = new->group_leader;
+	unsigned int idx;
+	int ret;
+
+	ret = ampere_cspmu_validate_configs(new, leader);
+	if (ret)
+		return ret;
+
+	/* We compare the global filter settings to existing events */
+	idx = find_first_bit(cspmu->hw_events.used_ctrs,
+			     cspmu->cycle_counter_logical_idx);
+
+	/* This is the first event */
+	if (idx == cspmu->cycle_counter_logical_idx)
+		return 0;
+
+	curr = cspmu->hw_events.events[idx];
+
+	return ampere_cspmu_validate_configs(curr, new);
+}
+
+static char *ampere_cspmu_format_name(const struct arm_cspmu *cspmu,
+				      const char *name_pattern)
+{
+	struct device *dev = cspmu->dev;
+	static atomic_t pmu_generic_idx = {0};
+
+	return devm_kasprintf(dev, GFP_KERNEL, name_pattern,
+			      atomic_fetch_inc(&pmu_generic_idx));
+}
+
+int ampere_cspmu_init_ops(struct arm_cspmu *cspmu)
+{
+	struct device *dev = cspmu->dev;
+	struct ampere_cspmu_ctx *ctx;
+	struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops;
+
+	ctx = devm_kzalloc(dev, sizeof(struct ampere_cspmu_ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->event_attr		= ampereone_mcu_pmu_event_attrs;
+	ctx->format_attr	= ampereone_mcu_format_attrs;
+	ctx->name		= ampere_cspmu_format_name(cspmu,
+							   "ampere_mcu_pmu_%u");
+	cspmu->impl.ctx = ctx;
+
+	impl_ops->event_filter		= ampere_cspmu_event_filter;
+	impl_ops->set_ev_filter		= ampere_cspmu_set_ev_filter;
+	impl_ops->validate_event	= ampere_cspmu_validate_event;
+	impl_ops->get_name		= ampere_cspmu_get_name;
+	impl_ops->get_event_attrs	= ampere_cspmu_get_event_attrs;
+	impl_ops->get_format_attrs	= ampere_cspmu_get_format_attrs;
+
+	return 0;
+}
+
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/perf/arm_cspmu/ampere_cspmu.h b/drivers/perf/arm_cspmu/ampere_cspmu.h
new file mode 100644
index 000000000000..9b3e1628d1d6
--- /dev/null
+++ b/drivers/perf/arm_cspmu/ampere_cspmu.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Ampere SoC PMU (Performance Monitor Unit)
+ *
+ * Copyright (c) 2023, Ampere Computing LLC
+ */
+
+#ifndef __AMPERE_CSPMU_H__
+#define __AMPERE_CSPMU_H__
+
+#include "arm_cspmu.h"
+
+/* Allocate AMPERE descriptor. */
+int ampere_cspmu_init_ops(struct arm_cspmu *cspmu);
+
+#endif /* __AMPERE_CSPMU_H__ */
diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index 471d6d7ac81a..587515eea0b4 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c
@@ -29,6 +29,7 @@
 #include <linux/perf_event.h>
 #include <linux/platform_device.h>
 
+#include "ampere_cspmu.h"
 #include "arm_cspmu.h"
 #include "nvidia_cspmu.h"
 
@@ -114,6 +115,7 @@
 
 /* JEDEC-assigned JEP106 identification code */
 #define ARM_CSPMU_IMPL_ID_NVIDIA		0x36B
+#define ARM_CSPMU_IMPL_ID_AMPERE		0xA16
 
 static unsigned long arm_cspmu_cpuhp_state;
 
@@ -388,6 +390,11 @@ static const struct impl_match impl_match[] = {
 	  .mask = ARM_CSPMU_PMIIDR_IMPLEMENTER,
 	  .impl_init_ops = nv_cspmu_init_ops
 	},
+	{
+	  .pmiidr = ARM_CSPMU_IMPL_ID_AMPERE,
+	  .mask = ARM_CSPMU_PMIIDR_IMPLEMENTER,
+	  .impl_init_ops = ampere_cspmu_init_ops
+	},
 	{}
 };
 
-- 
2.40.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/4] perf: arm_cspmu: ampere_cspmu: Add support for Ampere SoC PMU

[Ilkka Koskinen]

Ampere SoC PMU follows CoreSight PMU architecture. It uses implementation
specific registers to filter events rather than PMEVFILTnR registers.

Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
---
 .../admin-guide/perf/ampere_cspmu.rst         |  29 +++
 drivers/perf/arm_cspmu/Makefile               |   2 +-
 drivers/perf/arm_cspmu/ampere_cspmu.c         | 232 ++++++++++++++++++
 drivers/perf/arm_cspmu/ampere_cspmu.h         |  17 ++
 drivers/perf/arm_cspmu/arm_cspmu.c            |   7 +
 5 files changed, 286 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/admin-guide/perf/ampere_cspmu.rst
 create mode 100644 drivers/perf/arm_cspmu/ampere_cspmu.c
 create mode 100644 drivers/perf/arm_cspmu/ampere_cspmu.h

diff --git a/Documentation/admin-guide/perf/ampere_cspmu.rst b/Documentation/admin-guide/perf/ampere_cspmu.rst
new file mode 100644
index 000000000000..bf86bffeef63
--- /dev/null
+++ b/Documentation/admin-guide/perf/ampere_cspmu.rst
@@ -0,0 +1,29 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+============================================
+Ampere SoC Performance Monitoring Unit (PMU)
+============================================
+
+Ampere SoC PMU is a generic PMU IP that follows Arm CoreSight PMU architecture.
+Therefore, the driver is implemented as a submodule of arm_cspmu driver. At the
+first phase it's used for counting MCU events on AmpereOne.
+
+
+MCU PMU events
+--------------
+
+The PMU driver supports setting filters for "rank", "bank", and "threshold".
+Note, that the filters are per PMU instance rather than per event.
+
+
+Example for perf tool use::
+
+  / # perf list ampere
+
+    ampere_mcu_pmu_0/act_sent/                         [Kernel PMU event]
+    <...>
+    ampere_mcu_pmu_1/rd_sent/                          [Kernel PMU event]
+    <...>
+
+  / # perf stat -a -e ampere_mcu_pmu_0/act_sent,filter_enable=3,bank=5,rank=3,threshold=2/,ampere_mcu_pmu_1/rd_sent/ \
+        sleep 1
diff --git a/drivers/perf/arm_cspmu/Makefile b/drivers/perf/arm_cspmu/Makefile
index fedb17df982d..b80a8bd8da54 100644
--- a/drivers/perf/arm_cspmu/Makefile
+++ b/drivers/perf/arm_cspmu/Makefile
@@ -3,4 +3,4 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU) += arm_cspmu_module.o
-arm_cspmu_module-y := arm_cspmu.o nvidia_cspmu.o
+arm_cspmu_module-y := arm_cspmu.o nvidia_cspmu.o ampere_cspmu.o
diff --git a/drivers/perf/arm_cspmu/ampere_cspmu.c b/drivers/perf/arm_cspmu/ampere_cspmu.c
new file mode 100644
index 000000000000..62dc893c94c1
--- /dev/null
+++ b/drivers/perf/arm_cspmu/ampere_cspmu.c
@@ -0,0 +1,232 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Ampere SoC PMU (Performance Monitor Unit)
+ *
+ * Copyright (c) 2023, Ampere Computing LLC
+ */
+
+#include "ampere_cspmu.h"
+
+#define PMAUXR0		0xD80
+#define PMAUXR1		0xD84
+#define PMAUXR2		0xD88
+#define PMAUXR3		0xD8C
+
+#define to_ampere_cspmu_ctx(cspmu)	((struct ampere_cspmu_ctx *)(cspmu->impl.ctx))
+
+struct ampere_cspmu_ctx {
+	const char *name;
+	struct attribute **event_attr;
+	struct attribute **format_attr;
+};
+
+#define SOC_PMU_EVENT_ATTR_EXTRACTOR(_name, _config, _start, _end)        \
+	static inline u32 get_##_name(const struct perf_event *event)     \
+	{                                                                 \
+		return FIELD_GET(GENMASK_ULL(_end, _start),               \
+				 event->attr._config);                    \
+	}                                                                 \
+
+SOC_PMU_EVENT_ATTR_EXTRACTOR(event, config, 0, 8);
+SOC_PMU_EVENT_ATTR_EXTRACTOR(threshold, config1, 0, 7);
+SOC_PMU_EVENT_ATTR_EXTRACTOR(rank, config1, 8, 23);
+SOC_PMU_EVENT_ATTR_EXTRACTOR(bank, config2, 0, 31);
+
+static struct attribute *ampereone_mcu_pmu_event_attrs[] = {
+	ARM_CSPMU_EVENT_ATTR(cycle_count,		0x00),
+	ARM_CSPMU_EVENT_ATTR(act_sent,			0x01),
+	ARM_CSPMU_EVENT_ATTR(pre_sent,			0x02),
+	ARM_CSPMU_EVENT_ATTR(rd_sent,			0x03),
+	ARM_CSPMU_EVENT_ATTR(rda_sent,			0x04),
+	ARM_CSPMU_EVENT_ATTR(wr_sent,			0x05),
+	ARM_CSPMU_EVENT_ATTR(wra_sent,			0x06),
+	ARM_CSPMU_EVENT_ATTR(pd_entry_vld,		0x07),
+	ARM_CSPMU_EVENT_ATTR(sref_entry_vld,		0x08),
+	ARM_CSPMU_EVENT_ATTR(prea_sent,			0x09),
+	ARM_CSPMU_EVENT_ATTR(pre_sb_sent,		0x0a),
+	ARM_CSPMU_EVENT_ATTR(ref_sent,			0x0b),
+	ARM_CSPMU_EVENT_ATTR(rfm_sent,			0x0c),
+	ARM_CSPMU_EVENT_ATTR(ref_sb_sent,		0x0d),
+	ARM_CSPMU_EVENT_ATTR(rfm_sb_sent,		0x0e),
+	ARM_CSPMU_EVENT_ATTR(rd_rda_sent,		0x0f),
+	ARM_CSPMU_EVENT_ATTR(wr_wra_sent,		0x10),
+	ARM_CSPMU_EVENT_ATTR(raw_hazard,		0x11),
+	ARM_CSPMU_EVENT_ATTR(war_hazard,		0x12),
+	ARM_CSPMU_EVENT_ATTR(waw_hazard,		0x13),
+	ARM_CSPMU_EVENT_ATTR(rar_hazard,		0x14),
+	ARM_CSPMU_EVENT_ATTR(raw_war_waw_hazard,	0x15),
+	ARM_CSPMU_EVENT_ATTR(hprd_lprd_wr_req_vld,	0x16),
+	ARM_CSPMU_EVENT_ATTR(lprd_req_vld,		0x17),
+	ARM_CSPMU_EVENT_ATTR(hprd_req_vld,		0x18),
+	ARM_CSPMU_EVENT_ATTR(hprd_lprd_req_vld,		0x19),
+	ARM_CSPMU_EVENT_ATTR(prefetch_tgt,		0x1a),
+	ARM_CSPMU_EVENT_ATTR(wr_req_vld,		0x1b),
+	ARM_CSPMU_EVENT_ATTR(partial_wr_req_vld,	0x1c),
+	ARM_CSPMU_EVENT_ATTR(rd_retry,			0x1d),
+	ARM_CSPMU_EVENT_ATTR(wr_retry,			0x1e),
+	ARM_CSPMU_EVENT_ATTR(retry_gnt,			0x1f),
+	ARM_CSPMU_EVENT_ATTR(rank_change,		0x20),
+	ARM_CSPMU_EVENT_ATTR(dir_change,		0x21),
+	ARM_CSPMU_EVENT_ATTR(rank_dir_change,		0x22),
+	ARM_CSPMU_EVENT_ATTR(rank_active,		0x23),
+	ARM_CSPMU_EVENT_ATTR(rank_idle,			0x24),
+	ARM_CSPMU_EVENT_ATTR(rank_pd,			0x25),
+	ARM_CSPMU_EVENT_ATTR(rank_sref,			0x26),
+	ARM_CSPMU_EVENT_ATTR(queue_fill_gt_thresh,	0x27),
+	ARM_CSPMU_EVENT_ATTR(queue_rds_gt_thresh,	0x28),
+	ARM_CSPMU_EVENT_ATTR(queue_wrs_gt_thresh,	0x29),
+	ARM_CSPMU_EVENT_ATTR(phy_updt_complt,		0x2a),
+	ARM_CSPMU_EVENT_ATTR(tz_fail,			0x2b),
+	ARM_CSPMU_EVENT_ATTR(dram_errc,			0x2c),
+	ARM_CSPMU_EVENT_ATTR(dram_errd,			0x2d),
+	ARM_CSPMU_EVENT_ATTR(read_data_return,		0x32),
+	ARM_CSPMU_EVENT_ATTR(chi_wr_data_delta,		0x33),
+	ARM_CSPMU_EVENT_ATTR(zq_start,			0x34),
+	ARM_CSPMU_EVENT_ATTR(zq_latch,			0x35),
+	ARM_CSPMU_EVENT_ATTR(wr_fifo_full,		0x36),
+	ARM_CSPMU_EVENT_ATTR(info_fifo_full,		0x37),
+	ARM_CSPMU_EVENT_ATTR(cmd_fifo_full,		0x38),
+	ARM_CSPMU_EVENT_ATTR(dfi_nop,			0x39),
+	ARM_CSPMU_EVENT_ATTR(dfi_cmd,			0x3a),
+	ARM_CSPMU_EVENT_ATTR(rd_run_len,		0x3b),
+	ARM_CSPMU_EVENT_ATTR(wr_run_len,		0x3c),
+
+	ARM_CSPMU_EVENT_ATTR(cycles, ARM_CSPMU_EVT_CYCLES_DEFAULT),
+	NULL,
+};
+
+static struct attribute *ampereone_mcu_format_attrs[] = {
+	ARM_CSPMU_FORMAT_EVENT_ATTR,
+	ARM_CSPMU_FORMAT_ATTR(threshold, "config1:0-7"),
+	ARM_CSPMU_FORMAT_ATTR(rank, "config1:8-23"),
+	ARM_CSPMU_FORMAT_ATTR(bank, "config2:0-31"),
+	NULL,
+};
+
+static struct attribute **
+ampere_cspmu_get_event_attrs(const struct arm_cspmu *cspmu)
+{
+	const struct ampere_cspmu_ctx *ctx = to_ampere_cspmu_ctx(cspmu);
+
+	return ctx->event_attr;
+}
+
+static struct attribute **
+ampere_cspmu_get_format_attrs(const struct arm_cspmu *cspmu)
+{
+	const struct ampere_cspmu_ctx *ctx = to_ampere_cspmu_ctx(cspmu);
+
+	return ctx->format_attr;
+}
+
+static const char *
+ampere_cspmu_get_name(const struct arm_cspmu *cspmu)
+{
+	const struct ampere_cspmu_ctx *ctx = to_ampere_cspmu_ctx(cspmu);
+
+	return ctx->name;
+}
+
+static u32 ampere_cspmu_event_filter(const struct perf_event *event)
+{
+	return 0;
+}
+
+static void ampere_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
+				       struct hw_perf_event *hwc,
+				       u32 filter)
+{
+	struct perf_event *event;
+	unsigned int idx;
+	u32 threshold, rank, bank;
+
+	/*
+	 * At this point, all the events have the same filter settings.
+	 * Therefore, take the first event and use its configuration.
+	 */
+	idx = find_first_bit(cspmu->hw_events.used_ctrs,
+			     cspmu->cycle_counter_logical_idx);
+
+	event = cspmu->hw_events.events[idx];
+
+	threshold	= get_threshold(event);
+	rank		= get_rank(event);
+	bank		= get_bank(event);
+
+	writel(threshold, cspmu->base0 + PMAUXR0);
+	writel(rank, cspmu->base0 + PMAUXR1);
+	writel(bank, cspmu->base0 + PMAUXR2);
+}
+
+static int ampere_cspmu_validate_configs(struct perf_event *event,
+					 struct perf_event *event2)
+{
+	if (get_threshold(event) != get_threshold(event2) ||
+	    get_rank(event) != get_rank(event2) ||
+	    get_bank(event) != get_bank(event2))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int ampere_cspmu_validate_event(struct arm_cspmu *cspmu,
+				       struct perf_event *new)
+{
+	struct perf_event *curr, *leader = new->group_leader;
+	unsigned int idx;
+	int ret;
+
+	ret = ampere_cspmu_validate_configs(new, leader);
+	if (ret)
+		return ret;
+
+	/* We compare the global filter settings to existing events */
+	idx = find_first_bit(cspmu->hw_events.used_ctrs,
+			     cspmu->cycle_counter_logical_idx);
+
+	/* This is the first event */
+	if (idx == cspmu->cycle_counter_logical_idx)
+		return 0;
+
+	curr = cspmu->hw_events.events[idx];
+
+	return ampere_cspmu_validate_configs(curr, new);
+}
+
+static char *ampere_cspmu_format_name(const struct arm_cspmu *cspmu,
+				      const char *name_pattern)
+{
+	struct device *dev = cspmu->dev;
+	static atomic_t pmu_generic_idx = {0};
+
+	return devm_kasprintf(dev, GFP_KERNEL, name_pattern,
+			      atomic_fetch_inc(&pmu_generic_idx));
+}
+
+int ampere_cspmu_init_ops(struct arm_cspmu *cspmu)
+{
+	struct device *dev = cspmu->dev;
+	struct ampere_cspmu_ctx *ctx;
+	struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops;
+
+	ctx = devm_kzalloc(dev, sizeof(struct ampere_cspmu_ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->event_attr		= ampereone_mcu_pmu_event_attrs;
+	ctx->format_attr	= ampereone_mcu_format_attrs;
+	ctx->name		= ampere_cspmu_format_name(cspmu,
+							   "ampere_mcu_pmu_%u");
+	cspmu->impl.ctx = ctx;
+
+	impl_ops->event_filter		= ampere_cspmu_event_filter;
+	impl_ops->set_ev_filter		= ampere_cspmu_set_ev_filter;
+	impl_ops->validate_event	= ampere_cspmu_validate_event;
+	impl_ops->get_name		= ampere_cspmu_get_name;
+	impl_ops->get_event_attrs	= ampere_cspmu_get_event_attrs;
+	impl_ops->get_format_attrs	= ampere_cspmu_get_format_attrs;
+
+	return 0;
+}
+
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/perf/arm_cspmu/ampere_cspmu.h b/drivers/perf/arm_cspmu/ampere_cspmu.h
new file mode 100644
index 000000000000..9b3e1628d1d6
--- /dev/null
+++ b/drivers/perf/arm_cspmu/ampere_cspmu.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * Ampere SoC PMU (Performance Monitor Unit)
+ *
+ * Copyright (c) 2023, Ampere Computing LLC
+ */
+
+#ifndef __AMPERE_CSPMU_H__
+#define __AMPERE_CSPMU_H__
+
+#include "arm_cspmu.h"
+
+/* Allocate AMPERE descriptor. */
+int ampere_cspmu_init_ops(struct arm_cspmu *cspmu);
+
+#endif /* __AMPERE_CSPMU_H__ */
diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index 471d6d7ac81a..587515eea0b4 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c
@@ -29,6 +29,7 @@
 #include <linux/perf_event.h>
 #include <linux/platform_device.h>
 
+#include "ampere_cspmu.h"
 #include "arm_cspmu.h"
 #include "nvidia_cspmu.h"
 
@@ -114,6 +115,7 @@
 
 /* JEDEC-assigned JEP106 identification code */
 #define ARM_CSPMU_IMPL_ID_NVIDIA		0x36B
+#define ARM_CSPMU_IMPL_ID_AMPERE		0xA16
 
 static unsigned long arm_cspmu_cpuhp_state;
 
@@ -388,6 +390,11 @@ static const struct impl_match impl_match[] = {
 	  .mask = ARM_CSPMU_PMIIDR_IMPLEMENTER,
 	  .impl_init_ops = nv_cspmu_init_ops
 	},
+	{
+	  .pmiidr = ARM_CSPMU_IMPL_ID_AMPERE,
+	  .mask = ARM_CSPMU_PMIIDR_IMPLEMENTER,
+	  .impl_init_ops = ampere_cspmu_init_ops
+	},
 	{}
 };
 
-- 
2.40.1

Re: [PATCH 4/4] perf: arm_cspmu: ampere_cspmu: Add support for Ampere SoC PMU

[Jonathan Cameron]

On Wed, 21 Jun 2023 18:11:41 -0700
Ilkka Koskinen <ilkka@os.amperecomputing.com> wrote:

> Ampere SoC PMU follows CoreSight PMU architecture. It uses implementation
> specific registers to filter events rather than PMEVFILTnR registers.
> 
> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
Hi Ilkka,

Drive by review so not super detailed (I was curious) but a few questions/comments inline.

Jonathan

> ---
>  .../admin-guide/perf/ampere_cspmu.rst         |  29 +++
>  drivers/perf/arm_cspmu/Makefile               |   2 +-
>  drivers/perf/arm_cspmu/ampere_cspmu.c         | 232 ++++++++++++++++++
>  drivers/perf/arm_cspmu/ampere_cspmu.h         |  17 ++
>  drivers/perf/arm_cspmu/arm_cspmu.c            |   7 +
>  5 files changed, 286 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/admin-guide/perf/ampere_cspmu.rst
>  create mode 100644 drivers/perf/arm_cspmu/ampere_cspmu.c
>  create mode 100644 drivers/perf/arm_cspmu/ampere_cspmu.h
> 
> diff --git a/Documentation/admin-guide/perf/ampere_cspmu.rst b/Documentation/admin-guide/perf/ampere_cspmu.rst
> new file mode 100644
> index 000000000000..bf86bffeef63
> --- /dev/null
> +++ b/Documentation/admin-guide/perf/ampere_cspmu.rst
> @@ -0,0 +1,29 @@

> +
> +Example for perf tool use::
> +
> +  / # perf list ampere
> +
> +    ampere_mcu_pmu_0/act_sent/                         [Kernel PMU event]
> +    <...>
> +    ampere_mcu_pmu_1/rd_sent/                          [Kernel PMU event]
> +    <...>
> +
> +  / # perf stat -a -e ampere_mcu_pmu_0/act_sent,filter_enable=3,bank=5,rank=3,threshold=2/,ampere_mcu_pmu_1/rd_sent/ \
> +        sleep 1

Why filter_enable=3?



> +static u32 ampere_cspmu_event_filter(const struct perf_event *event)
> +{

Whilst lots of other comments on this - perhaps add another one here to
why this is a noop.

> +	return 0;
> +}
> +
> +static void ampere_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
> +				       struct hw_perf_event *hwc,
> +				       u32 filter)
> +{
> +	struct perf_event *event;
> +	unsigned int idx;
> +	u32 threshold, rank, bank;
> +
> +	/*
> +	 * At this point, all the events have the same filter settings.
> +	 * Therefore, take the first event and use its configuration.
> +	 */
> +	idx = find_first_bit(cspmu->hw_events.used_ctrs,
> +			     cspmu->cycle_counter_logical_idx);
> +
> +	event = cspmu->hw_events.events[idx];
> +
> +	threshold	= get_threshold(event);
> +	rank		= get_rank(event);
> +	bank		= get_bank(event);
> +
> +	writel(threshold, cspmu->base0 + PMAUXR0);
> +	writel(rank, cspmu->base0 + PMAUXR1);
> +	writel(bank, cspmu->base0 + PMAUXR2);
> +}
> +
> +static int ampere_cspmu_validate_configs(struct perf_event *event,
> +					 struct perf_event *event2)
> +{
> +	if (get_threshold(event) != get_threshold(event2) ||
> +	    get_rank(event) != get_rank(event2) ||
> +	    get_bank(event) != get_bank(event2))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int ampere_cspmu_validate_event(struct arm_cspmu *cspmu,
> +				       struct perf_event *new)
> +{
> +	struct perf_event *curr, *leader = new->group_leader;
> +	unsigned int idx;
> +	int ret;
> +
> +	ret = ampere_cspmu_validate_configs(new, leader);
> +	if (ret)
> +		return ret;
> +
> +	/* We compare the global filter settings to existing events */
> +	idx = find_first_bit(cspmu->hw_events.used_ctrs,
> +			     cspmu->cycle_counter_logical_idx);
> +
> +	/* This is the first event */

Maybe add why that matters to the comment?

> +	if (idx == cspmu->cycle_counter_logical_idx)
> +		return 0;
> +
> +	curr = cspmu->hw_events.events[idx];
> +
> +	return ampere_cspmu_validate_configs(curr, new);
> +}
> +
> +static char *ampere_cspmu_format_name(const struct arm_cspmu *cspmu,
> +				      const char *name_pattern)
> +{
> +	struct device *dev = cspmu->dev;
> +	static atomic_t pmu_generic_idx = {0};

Why not an ida?

If the pmu drivers ever become easy to unbind then you won't get ID
reusage like this an eventually you will run into overflow problems.

> +
> +	return devm_kasprintf(dev, GFP_KERNEL, name_pattern,
> +			      atomic_fetch_inc(&pmu_generic_idx));
> +}
> +
> +int ampere_cspmu_init_ops(struct arm_cspmu *cspmu)
> +{
> +	struct device *dev = cspmu->dev;
> +	struct ampere_cspmu_ctx *ctx;
> +	struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops;
> +
> +	ctx = devm_kzalloc(dev, sizeof(struct ampere_cspmu_ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	ctx->event_attr		= ampereone_mcu_pmu_event_attrs;
> +	ctx->format_attr	= ampereone_mcu_format_attrs;
> +	ctx->name		= ampere_cspmu_format_name(cspmu,
> +							   "ampere_mcu_pmu_%u");

Long line and need to break avoided if you don't bother trying to align the = signs...
Personally I don't like this style as it causes a lot of churn as drivers
evolve, but meh, it's up to you.

Given the result is confusing if the allocation fails (name not what is expected)
I would also check that allocation and error out if it fails.  Obviously it won't
under realistic circumstances, but a bit of paranoia never hurt anyone.

> +	cspmu->impl.ctx = ctx;
> +
> +	impl_ops->event_filter		= ampere_cspmu_event_filter;
> +	impl_ops->set_ev_filter		= ampere_cspmu_set_ev_filter;
> +	impl_ops->validate_event	= ampere_cspmu_validate_event;
> +	impl_ops->get_name		= ampere_cspmu_get_name;
> +	impl_ops->get_event_attrs	= ampere_cspmu_get_event_attrs;
> +	impl_ops->get_format_attrs	= ampere_cspmu_get_format_attrs;
> +
> +	return 0;
> +}
> +
> +MODULE_LICENSE("GPL v2");

...

> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index 471d6d7ac81a..587515eea0b4 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -29,6 +29,7 @@
>  #include <linux/perf_event.h>
>  #include <linux/platform_device.h>
>  
> +#include "ampere_cspmu.h"

I'd be tempted to keep the generic header in a separate block then
follow with the vendor ones.  Not particularly important though.

>  #include "arm_cspmu.h"
>  #include "nvidia_cspmu.h"
>  
> @@ -114,6 +115,7 @@
>  
>  /* JEDEC-assigned JEP106 identification code */
>  #define ARM_CSPMU_IMPL_ID_NVIDIA		0x36B
> +#define ARM_CSPMU_IMPL_ID_AMPERE		0xA16
>  
>  static unsigned long arm_cspmu_cpuhp_state;
>  
> @@ -388,6 +390,11 @@ static const struct impl_match impl_match[] = {
>  	  .mask = ARM_CSPMU_PMIIDR_IMPLEMENTER,
>  	  .impl_init_ops = nv_cspmu_init_ops
>  	},
> +	{
> +	  .pmiidr = ARM_CSPMU_IMPL_ID_AMPERE,
> +	  .mask = ARM_CSPMU_PMIIDR_IMPLEMENTER,
> +	  .impl_init_ops = ampere_cspmu_init_ops
> +	},
>  	{}
>  };
>  


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/4] perf: arm_cspmu: ampere_cspmu: Add support for Ampere SoC PMU

[Jonathan Cameron]

On Wed, 21 Jun 2023 18:11:41 -0700
Ilkka Koskinen <ilkka@os.amperecomputing.com> wrote:

> Ampere SoC PMU follows CoreSight PMU architecture. It uses implementation
> specific registers to filter events rather than PMEVFILTnR registers.
> 
> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
Hi Ilkka,

Drive by review so not super detailed (I was curious) but a few questions/comments inline.

Jonathan

> ---
>  .../admin-guide/perf/ampere_cspmu.rst         |  29 +++
>  drivers/perf/arm_cspmu/Makefile               |   2 +-
>  drivers/perf/arm_cspmu/ampere_cspmu.c         | 232 ++++++++++++++++++
>  drivers/perf/arm_cspmu/ampere_cspmu.h         |  17 ++
>  drivers/perf/arm_cspmu/arm_cspmu.c            |   7 +
>  5 files changed, 286 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/admin-guide/perf/ampere_cspmu.rst
>  create mode 100644 drivers/perf/arm_cspmu/ampere_cspmu.c
>  create mode 100644 drivers/perf/arm_cspmu/ampere_cspmu.h
> 
> diff --git a/Documentation/admin-guide/perf/ampere_cspmu.rst b/Documentation/admin-guide/perf/ampere_cspmu.rst
> new file mode 100644
> index 000000000000..bf86bffeef63
> --- /dev/null
> +++ b/Documentation/admin-guide/perf/ampere_cspmu.rst
> @@ -0,0 +1,29 @@

> +
> +Example for perf tool use::
> +
> +  / # perf list ampere
> +
> +    ampere_mcu_pmu_0/act_sent/                         [Kernel PMU event]
> +    <...>
> +    ampere_mcu_pmu_1/rd_sent/                          [Kernel PMU event]
> +    <...>
> +
> +  / # perf stat -a -e ampere_mcu_pmu_0/act_sent,filter_enable=3,bank=5,rank=3,threshold=2/,ampere_mcu_pmu_1/rd_sent/ \
> +        sleep 1

Why filter_enable=3?



> +static u32 ampere_cspmu_event_filter(const struct perf_event *event)
> +{

Whilst lots of other comments on this - perhaps add another one here to
why this is a noop.

> +	return 0;
> +}
> +
> +static void ampere_cspmu_set_ev_filter(struct arm_cspmu *cspmu,
> +				       struct hw_perf_event *hwc,
> +				       u32 filter)
> +{
> +	struct perf_event *event;
> +	unsigned int idx;
> +	u32 threshold, rank, bank;
> +
> +	/*
> +	 * At this point, all the events have the same filter settings.
> +	 * Therefore, take the first event and use its configuration.
> +	 */
> +	idx = find_first_bit(cspmu->hw_events.used_ctrs,
> +			     cspmu->cycle_counter_logical_idx);
> +
> +	event = cspmu->hw_events.events[idx];
> +
> +	threshold	= get_threshold(event);
> +	rank		= get_rank(event);
> +	bank		= get_bank(event);
> +
> +	writel(threshold, cspmu->base0 + PMAUXR0);
> +	writel(rank, cspmu->base0 + PMAUXR1);
> +	writel(bank, cspmu->base0 + PMAUXR2);
> +}
> +
> +static int ampere_cspmu_validate_configs(struct perf_event *event,
> +					 struct perf_event *event2)
> +{
> +	if (get_threshold(event) != get_threshold(event2) ||
> +	    get_rank(event) != get_rank(event2) ||
> +	    get_bank(event) != get_bank(event2))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int ampere_cspmu_validate_event(struct arm_cspmu *cspmu,
> +				       struct perf_event *new)
> +{
> +	struct perf_event *curr, *leader = new->group_leader;
> +	unsigned int idx;
> +	int ret;
> +
> +	ret = ampere_cspmu_validate_configs(new, leader);
> +	if (ret)
> +		return ret;
> +
> +	/* We compare the global filter settings to existing events */
> +	idx = find_first_bit(cspmu->hw_events.used_ctrs,
> +			     cspmu->cycle_counter_logical_idx);
> +
> +	/* This is the first event */

Maybe add why that matters to the comment?

> +	if (idx == cspmu->cycle_counter_logical_idx)
> +		return 0;
> +
> +	curr = cspmu->hw_events.events[idx];
> +
> +	return ampere_cspmu_validate_configs(curr, new);
> +}
> +
> +static char *ampere_cspmu_format_name(const struct arm_cspmu *cspmu,
> +				      const char *name_pattern)
> +{
> +	struct device *dev = cspmu->dev;
> +	static atomic_t pmu_generic_idx = {0};

Why not an ida?

If the pmu drivers ever become easy to unbind then you won't get ID
reusage like this an eventually you will run into overflow problems.

> +
> +	return devm_kasprintf(dev, GFP_KERNEL, name_pattern,
> +			      atomic_fetch_inc(&pmu_generic_idx));
> +}
> +
> +int ampere_cspmu_init_ops(struct arm_cspmu *cspmu)
> +{
> +	struct device *dev = cspmu->dev;
> +	struct ampere_cspmu_ctx *ctx;
> +	struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops;
> +
> +	ctx = devm_kzalloc(dev, sizeof(struct ampere_cspmu_ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	ctx->event_attr		= ampereone_mcu_pmu_event_attrs;
> +	ctx->format_attr	= ampereone_mcu_format_attrs;
> +	ctx->name		= ampere_cspmu_format_name(cspmu,
> +							   "ampere_mcu_pmu_%u");

Long line and need to break avoided if you don't bother trying to align the = signs...
Personally I don't like this style as it causes a lot of churn as drivers
evolve, but meh, it's up to you.

Given the result is confusing if the allocation fails (name not what is expected)
I would also check that allocation and error out if it fails.  Obviously it won't
under realistic circumstances, but a bit of paranoia never hurt anyone.

> +	cspmu->impl.ctx = ctx;
> +
> +	impl_ops->event_filter		= ampere_cspmu_event_filter;
> +	impl_ops->set_ev_filter		= ampere_cspmu_set_ev_filter;
> +	impl_ops->validate_event	= ampere_cspmu_validate_event;
> +	impl_ops->get_name		= ampere_cspmu_get_name;
> +	impl_ops->get_event_attrs	= ampere_cspmu_get_event_attrs;
> +	impl_ops->get_format_attrs	= ampere_cspmu_get_format_attrs;
> +
> +	return 0;
> +}
> +
> +MODULE_LICENSE("GPL v2");

...

> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
> index 471d6d7ac81a..587515eea0b4 100644
> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
> @@ -29,6 +29,7 @@
>  #include <linux/perf_event.h>
>  #include <linux/platform_device.h>
>  
> +#include "ampere_cspmu.h"

I'd be tempted to keep the generic header in a separate block then
follow with the vendor ones.  Not particularly important though.

>  #include "arm_cspmu.h"
>  #include "nvidia_cspmu.h"
>  
> @@ -114,6 +115,7 @@
>  
>  /* JEDEC-assigned JEP106 identification code */
>  #define ARM_CSPMU_IMPL_ID_NVIDIA		0x36B
> +#define ARM_CSPMU_IMPL_ID_AMPERE		0xA16
>  
>  static unsigned long arm_cspmu_cpuhp_state;
>  
> @@ -388,6 +390,11 @@ static const struct impl_match impl_match[] = {
>  	  .mask = ARM_CSPMU_PMIIDR_IMPLEMENTER,
>  	  .impl_init_ops = nv_cspmu_init_ops
>  	},
> +	{
> +	  .pmiidr = ARM_CSPMU_IMPL_ID_AMPERE,
> +	  .mask = ARM_CSPMU_PMIIDR_IMPLEMENTER,
> +	  .impl_init_ops = ampere_cspmu_init_ops
> +	},
>  	{}
>  };
>  

Re: [PATCH 4/4] perf: arm_cspmu: ampere_cspmu: Add support for Ampere SoC PMU

[Ilkka Koskinen]

Hi Jonathan

On Thu, 22 Jun 2023, Jonathan Cameron wrote:
> On Wed, 21 Jun 2023 18:11:41 -0700
> Ilkka Koskinen <ilkka@os.amperecomputing.com> wrote:
>
>> Ampere SoC PMU follows CoreSight PMU architecture. It uses implementation
>> specific registers to filter events rather than PMEVFILTnR registers.
>>
>> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> Hi Ilkka,
>
> Drive by review so not super detailed (I was curious) but a few questions/comments inline.
>
> Jonathan
>
>> ---
>>  .../admin-guide/perf/ampere_cspmu.rst         |  29 +++
>>  drivers/perf/arm_cspmu/Makefile               |   2 +-
>>  drivers/perf/arm_cspmu/ampere_cspmu.c         | 232 ++++++++++++++++++
>>  drivers/perf/arm_cspmu/ampere_cspmu.h         |  17 ++
>>  drivers/perf/arm_cspmu/arm_cspmu.c            |   7 +
>>  5 files changed, 286 insertions(+), 1 deletion(-)
>>  create mode 100644 Documentation/admin-guide/perf/ampere_cspmu.rst
>>  create mode 100644 drivers/perf/arm_cspmu/ampere_cspmu.c
>>  create mode 100644 drivers/perf/arm_cspmu/ampere_cspmu.h
>>
>> diff --git a/Documentation/admin-guide/perf/ampere_cspmu.rst b/Documentation/admin-guide/perf/ampere_cspmu.rst
>> new file mode 100644
>> index 000000000000..bf86bffeef63
>> --- /dev/null
>> +++ b/Documentation/admin-guide/perf/ampere_cspmu.rst
>> @@ -0,0 +1,29 @@
>
>> +
>> +Example for perf tool use::
>> +
>> +  / # perf list ampere
>> +
>> +    ampere_mcu_pmu_0/act_sent/                         [Kernel PMU event]
>> +    <...>
>> +    ampere_mcu_pmu_1/rd_sent/                          [Kernel PMU event]
>> +    <...>
>> +
>> +  / # perf stat -a -e ampere_mcu_pmu_0/act_sent,filter_enable=3,bank=5,rank=3,threshold=2/,ampere_mcu_pmu_1/rd_sent/ \
>> +        sleep 1
>
> Why filter_enable=3?

Thanks for catching. That's from the first cspmu version and got 
accidentally back there. The rest of the patch looks like what it was 
supposed to be though.


>> +static u32 ampere_cspmu_event_filter(const struct perf_event *event)
>> +{
>
> Whilst lots of other comments on this - perhaps add another one here to
> why this is a noop.

Sure, makes sense.

>
>> +	return 0;
>> +}

...

>> +static int ampere_cspmu_validate_event(struct arm_cspmu *cspmu,
>> +				       struct perf_event *new)
>> +{
>> +	struct perf_event *curr, *leader = new->group_leader;
>> +	unsigned int idx;
>> +	int ret;
>> +
>> +	ret = ampere_cspmu_validate_configs(new, leader);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* We compare the global filter settings to existing events */
>> +	idx = find_first_bit(cspmu->hw_events.used_ctrs,
>> +			     cspmu->cycle_counter_logical_idx);
>> +
>> +	/* This is the first event */
>
> Maybe add why that matters to the comment?

Sure, I'll do that.

>
>> +	if (idx == cspmu->cycle_counter_logical_idx)
>> +		return 0;
>> +
>> +	curr = cspmu->hw_events.events[idx];
>> +
>> +	return ampere_cspmu_validate_configs(curr, new);
>> +}
>> +
>> +static char *ampere_cspmu_format_name(const struct arm_cspmu *cspmu,
>> +				      const char *name_pattern)
>> +{
>> +	struct device *dev = cspmu->dev;
>> +	static atomic_t pmu_generic_idx = {0};
>
> Why not an ida?
>
> If the pmu drivers ever become easy to unbind then you won't get ID
> reusage like this an eventually you will run into overflow problems.

Didn't appear in my mind when I wrote the submodule. I'll change it.

Releasing devices would eventually require a release hook too to support 
reusing the IDs but we can add that later.

>
>> +
>> +	return devm_kasprintf(dev, GFP_KERNEL, name_pattern,
>> +			      atomic_fetch_inc(&pmu_generic_idx));
>> +}
>> +
>> +int ampere_cspmu_init_ops(struct arm_cspmu *cspmu)
>> +{
>> +	struct device *dev = cspmu->dev;
>> +	struct ampere_cspmu_ctx *ctx;
>> +	struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops;
>> +
>> +	ctx = devm_kzalloc(dev, sizeof(struct ampere_cspmu_ctx), GFP_KERNEL);
>> +	if (!ctx)
>> +		return -ENOMEM;
>> +
>> +	ctx->event_attr		= ampereone_mcu_pmu_event_attrs;
>> +	ctx->format_attr	= ampereone_mcu_format_attrs;
>> +	ctx->name		= ampere_cspmu_format_name(cspmu,
>> +							   "ampere_mcu_pmu_%u");
>
> Long line and need to break avoided if you don't bother trying to align the = signs...
> Personally I don't like this style as it causes a lot of churn as drivers
> evolve, but meh, it's up to you.

I don't really have preference on either way. I remove the aligment and 
combine those two lines.

>
> Given the result is confusing if the allocation fails (name not what is expected)
> I would also check that allocation and error out if it fails.  Obviously it won't
> under realistic circumstances, but a bit of paranoia never hurt anyone.

I agree, I fix it.

>
>> +	cspmu->impl.ctx = ctx;
>> +
>> +	impl_ops->event_filter		= ampere_cspmu_event_filter;
>> +	impl_ops->set_ev_filter		= ampere_cspmu_set_ev_filter;
>> +	impl_ops->validate_event	= ampere_cspmu_validate_event;
>> +	impl_ops->get_name		= ampere_cspmu_get_name;
>> +	impl_ops->get_event_attrs	= ampere_cspmu_get_event_attrs;
>> +	impl_ops->get_format_attrs	= ampere_cspmu_get_format_attrs;
>> +
>> +	return 0;
>> +}
>> +
>> +MODULE_LICENSE("GPL v2");
>
> ...
>
>> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
>> index 471d6d7ac81a..587515eea0b4 100644
>> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
>> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
>> @@ -29,6 +29,7 @@
>>  #include <linux/perf_event.h>
>>  #include <linux/platform_device.h>
>>
>> +#include "ampere_cspmu.h"
>
> I'd be tempted to keep the generic header in a separate block then
> follow with the vendor ones.  Not particularly important though.

I like your idea

>
>>  #include "arm_cspmu.h"
>>  #include "nvidia_cspmu.h"
>>
>> @@ -114,6 +115,7 @@
>>
>>  /* JEDEC-assigned JEP106 identification code */
>>  #define ARM_CSPMU_IMPL_ID_NVIDIA		0x36B
>> +#define ARM_CSPMU_IMPL_ID_AMPERE		0xA16
>>
>>  static unsigned long arm_cspmu_cpuhp_state;
>>
>> @@ -388,6 +390,11 @@ static const struct impl_match impl_match[] = {
>>  	  .mask = ARM_CSPMU_PMIIDR_IMPLEMENTER,
>>  	  .impl_init_ops = nv_cspmu_init_ops
>>  	},
>> +	{
>> +	  .pmiidr = ARM_CSPMU_IMPL_ID_AMPERE,
>> +	  .mask = ARM_CSPMU_PMIIDR_IMPLEMENTER,
>> +	  .impl_init_ops = ampere_cspmu_init_ops
>> +	},
>>  	{}
>>  };
>>
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/4] perf: arm_cspmu: ampere_cspmu: Add support for Ampere SoC PMU

[Ilkka Koskinen]

Hi Jonathan

On Thu, 22 Jun 2023, Jonathan Cameron wrote:
> On Wed, 21 Jun 2023 18:11:41 -0700
> Ilkka Koskinen <ilkka@os.amperecomputing.com> wrote:
>
>> Ampere SoC PMU follows CoreSight PMU architecture. It uses implementation
>> specific registers to filter events rather than PMEVFILTnR registers.
>>
>> Signed-off-by: Ilkka Koskinen <ilkka@os.amperecomputing.com>
> Hi Ilkka,
>
> Drive by review so not super detailed (I was curious) but a few questions/comments inline.
>
> Jonathan
>
>> ---
>>  .../admin-guide/perf/ampere_cspmu.rst         |  29 +++
>>  drivers/perf/arm_cspmu/Makefile               |   2 +-
>>  drivers/perf/arm_cspmu/ampere_cspmu.c         | 232 ++++++++++++++++++
>>  drivers/perf/arm_cspmu/ampere_cspmu.h         |  17 ++
>>  drivers/perf/arm_cspmu/arm_cspmu.c            |   7 +
>>  5 files changed, 286 insertions(+), 1 deletion(-)
>>  create mode 100644 Documentation/admin-guide/perf/ampere_cspmu.rst
>>  create mode 100644 drivers/perf/arm_cspmu/ampere_cspmu.c
>>  create mode 100644 drivers/perf/arm_cspmu/ampere_cspmu.h
>>
>> diff --git a/Documentation/admin-guide/perf/ampere_cspmu.rst b/Documentation/admin-guide/perf/ampere_cspmu.rst
>> new file mode 100644
>> index 000000000000..bf86bffeef63
>> --- /dev/null
>> +++ b/Documentation/admin-guide/perf/ampere_cspmu.rst
>> @@ -0,0 +1,29 @@
>
>> +
>> +Example for perf tool use::
>> +
>> +  / # perf list ampere
>> +
>> +    ampere_mcu_pmu_0/act_sent/                         [Kernel PMU event]
>> +    <...>
>> +    ampere_mcu_pmu_1/rd_sent/                          [Kernel PMU event]
>> +    <...>
>> +
>> +  / # perf stat -a -e ampere_mcu_pmu_0/act_sent,filter_enable=3,bank=5,rank=3,threshold=2/,ampere_mcu_pmu_1/rd_sent/ \
>> +        sleep 1
>
> Why filter_enable=3?

Thanks for catching. That's from the first cspmu version and got 
accidentally back there. The rest of the patch looks like what it was 
supposed to be though.


>> +static u32 ampere_cspmu_event_filter(const struct perf_event *event)
>> +{
>
> Whilst lots of other comments on this - perhaps add another one here to
> why this is a noop.

Sure, makes sense.

>
>> +	return 0;
>> +}

...

>> +static int ampere_cspmu_validate_event(struct arm_cspmu *cspmu,
>> +				       struct perf_event *new)
>> +{
>> +	struct perf_event *curr, *leader = new->group_leader;
>> +	unsigned int idx;
>> +	int ret;
>> +
>> +	ret = ampere_cspmu_validate_configs(new, leader);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* We compare the global filter settings to existing events */
>> +	idx = find_first_bit(cspmu->hw_events.used_ctrs,
>> +			     cspmu->cycle_counter_logical_idx);
>> +
>> +	/* This is the first event */
>
> Maybe add why that matters to the comment?

Sure, I'll do that.

>
>> +	if (idx == cspmu->cycle_counter_logical_idx)
>> +		return 0;
>> +
>> +	curr = cspmu->hw_events.events[idx];
>> +
>> +	return ampere_cspmu_validate_configs(curr, new);
>> +}
>> +
>> +static char *ampere_cspmu_format_name(const struct arm_cspmu *cspmu,
>> +				      const char *name_pattern)
>> +{
>> +	struct device *dev = cspmu->dev;
>> +	static atomic_t pmu_generic_idx = {0};
>
> Why not an ida?
>
> If the pmu drivers ever become easy to unbind then you won't get ID
> reusage like this an eventually you will run into overflow problems.

Didn't appear in my mind when I wrote the submodule. I'll change it.

Releasing devices would eventually require a release hook too to support 
reusing the IDs but we can add that later.

>
>> +
>> +	return devm_kasprintf(dev, GFP_KERNEL, name_pattern,
>> +			      atomic_fetch_inc(&pmu_generic_idx));
>> +}
>> +
>> +int ampere_cspmu_init_ops(struct arm_cspmu *cspmu)
>> +{
>> +	struct device *dev = cspmu->dev;
>> +	struct ampere_cspmu_ctx *ctx;
>> +	struct arm_cspmu_impl_ops *impl_ops = &cspmu->impl.ops;
>> +
>> +	ctx = devm_kzalloc(dev, sizeof(struct ampere_cspmu_ctx), GFP_KERNEL);
>> +	if (!ctx)
>> +		return -ENOMEM;
>> +
>> +	ctx->event_attr		= ampereone_mcu_pmu_event_attrs;
>> +	ctx->format_attr	= ampereone_mcu_format_attrs;
>> +	ctx->name		= ampere_cspmu_format_name(cspmu,
>> +							   "ampere_mcu_pmu_%u");
>
> Long line and need to break avoided if you don't bother trying to align the = signs...
> Personally I don't like this style as it causes a lot of churn as drivers
> evolve, but meh, it's up to you.

I don't really have preference on either way. I remove the aligment and 
combine those two lines.

>
> Given the result is confusing if the allocation fails (name not what is expected)
> I would also check that allocation and error out if it fails.  Obviously it won't
> under realistic circumstances, but a bit of paranoia never hurt anyone.

I agree, I fix it.

>
>> +	cspmu->impl.ctx = ctx;
>> +
>> +	impl_ops->event_filter		= ampere_cspmu_event_filter;
>> +	impl_ops->set_ev_filter		= ampere_cspmu_set_ev_filter;
>> +	impl_ops->validate_event	= ampere_cspmu_validate_event;
>> +	impl_ops->get_name		= ampere_cspmu_get_name;
>> +	impl_ops->get_event_attrs	= ampere_cspmu_get_event_attrs;
>> +	impl_ops->get_format_attrs	= ampere_cspmu_get_format_attrs;
>> +
>> +	return 0;
>> +}
>> +
>> +MODULE_LICENSE("GPL v2");
>
> ...
>
>> diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
>> index 471d6d7ac81a..587515eea0b4 100644
>> --- a/drivers/perf/arm_cspmu/arm_cspmu.c
>> +++ b/drivers/perf/arm_cspmu/arm_cspmu.c
>> @@ -29,6 +29,7 @@
>>  #include <linux/perf_event.h>
>>  #include <linux/platform_device.h>
>>
>> +#include "ampere_cspmu.h"
>
> I'd be tempted to keep the generic header in a separate block then
> follow with the vendor ones.  Not particularly important though.

I like your idea

>
>>  #include "arm_cspmu.h"
>>  #include "nvidia_cspmu.h"
>>
>> @@ -114,6 +115,7 @@
>>
>>  /* JEDEC-assigned JEP106 identification code */
>>  #define ARM_CSPMU_IMPL_ID_NVIDIA		0x36B
>> +#define ARM_CSPMU_IMPL_ID_AMPERE		0xA16
>>
>>  static unsigned long arm_cspmu_cpuhp_state;
>>
>> @@ -388,6 +390,11 @@ static const struct impl_match impl_match[] = {
>>  	  .mask = ARM_CSPMU_PMIIDR_IMPLEMENTER,
>>  	  .impl_init_ops = nv_cspmu_init_ops
>>  	},
>> +	{
>> +	  .pmiidr = ARM_CSPMU_IMPL_ID_AMPERE,
>> +	  .mask = ARM_CSPMU_PMIIDR_IMPLEMENTER,
>> +	  .impl_init_ops = ampere_cspmu_init_ops
>> +	},
>>  	{}
>>  };
>>
>
>