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
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 <[email protected]>
Signed-off-by: Ilkka Koskinen <[email protected]>
---
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
On Wed, 21 Jun 2023 18:11:39 -0700
Ilkka Koskinen <[email protected]> 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 <[email protected]>
> Signed-off-by: Ilkka Koskinen <[email protected]>
> ---
> 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);
> -----Original Message-----
> From: Jonathan Cameron <[email protected]>
> Sent: Thursday, June 22, 2023 3:34 PM
> To: Ilkka Koskinen <[email protected]>
> Cc: Will Deacon <[email protected]>; Robin Murphy <[email protected]>;
> Besar Wicaksono <[email protected]>; Suzuki K Poulose
> <[email protected]>; Mark Rutland <[email protected]>; linux-
> [email protected]; [email protected]
> 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 <[email protected]> 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 <[email protected]>
> > Signed-off-by: Ilkka Koskinen <[email protected]>
> > ---
> > 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);
On Thu, 22 Jun 2023, Besar Wicaksono wrote:
>> -----Original Message-----
>> From: Jonathan Cameron <[email protected]>
>> Sent: Thursday, June 22, 2023 3:34 PM
>> To: Ilkka Koskinen <[email protected]>
>> Cc: Will Deacon <[email protected]>; Robin Murphy <[email protected]>;
>> Besar Wicaksono <[email protected]>; Suzuki K Poulose
>> <[email protected]>; Mark Rutland <[email protected]>; linux-
>> [email protected]; [email protected]
>> 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 <[email protected]> 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 <[email protected]>
>>> Signed-off-by: Ilkka Koskinen <[email protected]>
>>> ---
>>> 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);
>
>