[PATCH 0/8] Refactor the common parts to the HiSilicon Uncore PMU core and cleanups

[PATCH 0/8] Refactor the common parts to the HiSilicon Uncore PMU core and cleanups

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

This series mainly contains the refactor of the HiSilicon Uncore PMU by
extracting the common parts from each drivers into the core:
- the retrieving of the PMU topology ID
- the common sysfs attributes like cpumask and identifier

In order to achieve this, we need to do below preparation:
- improve the detection of associated CPUs for PMUs locates on a SICL
- maintain the topology information in a dedicated data structure

Besides also include below changes/cleanups in this patchset
- add one new sysfs attributes for better describing the topology information
  of the associated CPUs of the PMU.
- define a symbol namespace for HiSilicon Uncore PMUs to avoid pollute the
  common ones
- two minor nonfunctional cleanups of DDRC PMU

Junhao He (2):
  drivers/perf: hisi: Fix incorrect variable name "hha_pmu" in DDRC PMU
    driver
  drivers/perf: hisi: Delete redundant blank line of DDRC PMU

Yicong Yang (6):
  drivers/perf: hisi: Define a symbol namespace for HiSilicon Uncore
    PMUs
  drivers/perf: hisi: Improve the detection of associated CPUs
  drivers/perf: hisi: Extract topology information to a separate
    structure
  drivers/perf: hisi: Add a common function to retrieve topology from
    firmware
  drivers/perf: hisi: Refactor the attributes creation
  drivers/perf: hisi: Export associated CPUs of each PMU through sysfs

 Documentation/admin-guide/perf/hisi-pmu.rst   |   5 +-
 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c  |  55 ++----
 drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c |  82 +++-----
 drivers/perf/hisilicon/hisi_uncore_hha_pmu.c  |  71 +++----
 drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c  |  65 ++-----
 drivers/perf/hisilicon/hisi_uncore_pa_pmu.c   |  54 ++----
 drivers/perf/hisilicon/hisi_uncore_pmu.c      | 182 ++++++++++++++----
 drivers/perf/hisilicon/hisi_uncore_pmu.h      |  60 +++++-
 drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c |  54 ++----
 drivers/perf/hisilicon/hisi_uncore_uc_pmu.c   |  56 ++----
 10 files changed, 323 insertions(+), 361 deletions(-)

-- 
2.24.0

[PATCH 1/8] drivers/perf: hisi: Define a symbol namespace for HiSilicon Uncore PMUs

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

The HiSilicon Uncore PMU framework implements some common functions
and exported them to the drivers. Use a specific HISI_PMU namespace
for the exported symbols to avoid pollute the generic ones.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/perf/hisilicon/hisi_uncore_pmu.c | 36 ++++++++++++------------
 drivers/perf/hisilicon/hisi_uncore_pmu.h |  2 ++
 2 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
index 918cdc31de57..416f72a813fc 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
@@ -34,7 +34,7 @@ ssize_t hisi_event_sysfs_show(struct device *dev,
 
 	return sysfs_emit(page, "config=0x%lx\n", (unsigned long)eattr->var);
 }
-EXPORT_SYMBOL_GPL(hisi_event_sysfs_show);
+EXPORT_SYMBOL_NS_GPL(hisi_event_sysfs_show, HISI_PMU);
 
 /*
  * sysfs cpumask attributes. For uncore PMU, we only have a single CPU to show
@@ -46,7 +46,7 @@ ssize_t hisi_cpumask_sysfs_show(struct device *dev,
 
 	return sysfs_emit(buf, "%d\n", hisi_pmu->on_cpu);
 }
-EXPORT_SYMBOL_GPL(hisi_cpumask_sysfs_show);
+EXPORT_SYMBOL_NS_GPL(hisi_cpumask_sysfs_show, HISI_PMU);
 
 static bool hisi_validate_event_group(struct perf_event *event)
 {
@@ -96,7 +96,7 @@ int hisi_uncore_pmu_get_event_idx(struct perf_event *event)
 
 	return idx;
 }
-EXPORT_SYMBOL_GPL(hisi_uncore_pmu_get_event_idx);
+EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_get_event_idx, HISI_PMU);
 
 ssize_t hisi_uncore_pmu_identifier_attr_show(struct device *dev,
 					     struct device_attribute *attr,
@@ -106,7 +106,7 @@ ssize_t hisi_uncore_pmu_identifier_attr_show(struct device *dev,
 
 	return sysfs_emit(page, "0x%08x\n", hisi_pmu->identifier);
 }
-EXPORT_SYMBOL_GPL(hisi_uncore_pmu_identifier_attr_show);
+EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_identifier_attr_show, HISI_PMU);
 
 static void hisi_uncore_pmu_clear_event_idx(struct hisi_pmu *hisi_pmu, int idx)
 {
@@ -165,7 +165,7 @@ int hisi_uncore_pmu_init_irq(struct hisi_pmu *hisi_pmu,
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(hisi_uncore_pmu_init_irq);
+EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_init_irq, HISI_PMU);
 
 int hisi_uncore_pmu_event_init(struct perf_event *event)
 {
@@ -219,7 +219,7 @@ int hisi_uncore_pmu_event_init(struct perf_event *event)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(hisi_uncore_pmu_event_init);
+EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_event_init, HISI_PMU);
 
 /*
  * Set the counter to count the event that we're interested in,
@@ -273,7 +273,7 @@ void hisi_uncore_pmu_set_event_period(struct perf_event *event)
 	/* Write start value to the hardware event counter */
 	hisi_pmu->ops->write_counter(hisi_pmu, hwc, val);
 }
-EXPORT_SYMBOL_GPL(hisi_uncore_pmu_set_event_period);
+EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_set_event_period, HISI_PMU);
 
 void hisi_uncore_pmu_event_update(struct perf_event *event)
 {
@@ -294,7 +294,7 @@ void hisi_uncore_pmu_event_update(struct perf_event *event)
 		HISI_MAX_PERIOD(hisi_pmu->counter_bits);
 	local64_add(delta, &event->count);
 }
-EXPORT_SYMBOL_GPL(hisi_uncore_pmu_event_update);
+EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_event_update, HISI_PMU);
 
 void hisi_uncore_pmu_start(struct perf_event *event, int flags)
 {
@@ -317,7 +317,7 @@ void hisi_uncore_pmu_start(struct perf_event *event, int flags)
 	hisi_uncore_pmu_enable_event(event);
 	perf_event_update_userpage(event);
 }
-EXPORT_SYMBOL_GPL(hisi_uncore_pmu_start);
+EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_start, HISI_PMU);
 
 void hisi_uncore_pmu_stop(struct perf_event *event, int flags)
 {
@@ -334,7 +334,7 @@ void hisi_uncore_pmu_stop(struct perf_event *event, int flags)
 	hisi_uncore_pmu_event_update(event);
 	hwc->state |= PERF_HES_UPTODATE;
 }
-EXPORT_SYMBOL_GPL(hisi_uncore_pmu_stop);
+EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_stop, HISI_PMU);
 
 int hisi_uncore_pmu_add(struct perf_event *event, int flags)
 {
@@ -357,7 +357,7 @@ int hisi_uncore_pmu_add(struct perf_event *event, int flags)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(hisi_uncore_pmu_add);
+EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_add, HISI_PMU);
 
 void hisi_uncore_pmu_del(struct perf_event *event, int flags)
 {
@@ -369,14 +369,14 @@ void hisi_uncore_pmu_del(struct perf_event *event, int flags)
 	perf_event_update_userpage(event);
 	hisi_pmu->pmu_events.hw_events[hwc->idx] = NULL;
 }
-EXPORT_SYMBOL_GPL(hisi_uncore_pmu_del);
+EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_del, HISI_PMU);
 
 void hisi_uncore_pmu_read(struct perf_event *event)
 {
 	/* Read hardware counter and update the perf counter statistics */
 	hisi_uncore_pmu_event_update(event);
 }
-EXPORT_SYMBOL_GPL(hisi_uncore_pmu_read);
+EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_read, HISI_PMU);
 
 void hisi_uncore_pmu_enable(struct pmu *pmu)
 {
@@ -389,7 +389,7 @@ void hisi_uncore_pmu_enable(struct pmu *pmu)
 
 	hisi_pmu->ops->start_counters(hisi_pmu);
 }
-EXPORT_SYMBOL_GPL(hisi_uncore_pmu_enable);
+EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_enable, HISI_PMU);
 
 void hisi_uncore_pmu_disable(struct pmu *pmu)
 {
@@ -397,7 +397,7 @@ void hisi_uncore_pmu_disable(struct pmu *pmu)
 
 	hisi_pmu->ops->stop_counters(hisi_pmu);
 }
-EXPORT_SYMBOL_GPL(hisi_uncore_pmu_disable);
+EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_disable, HISI_PMU);
 
 
 /*
@@ -484,7 +484,7 @@ int hisi_uncore_pmu_online_cpu(unsigned int cpu, struct hlist_node *node)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(hisi_uncore_pmu_online_cpu);
+EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_online_cpu, HISI_PMU);
 
 int hisi_uncore_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
 {
@@ -515,7 +515,7 @@ int hisi_uncore_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(hisi_uncore_pmu_offline_cpu);
+EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_offline_cpu, HISI_PMU);
 
 void hisi_pmu_init(struct hisi_pmu *hisi_pmu, struct module *module)
 {
@@ -535,7 +535,7 @@ void hisi_pmu_init(struct hisi_pmu *hisi_pmu, struct module *module)
 	pmu->attr_groups        = hisi_pmu->pmu_events.attr_groups;
 	pmu->capabilities       = PERF_PMU_CAP_NO_EXCLUDE;
 }
-EXPORT_SYMBOL_GPL(hisi_pmu_init);
+EXPORT_SYMBOL_NS_GPL(hisi_pmu_init, HISI_PMU);
 
 MODULE_DESCRIPTION("HiSilicon SoC uncore Performance Monitor driver framework");
 MODULE_LICENSE("GPL v2");
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h
index 25b2d43b72bf..008f18bbef7e 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pmu.h
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
@@ -23,6 +23,8 @@
 #undef pr_fmt
 #define pr_fmt(fmt)     "hisi_pmu: " fmt
 
+MODULE_IMPORT_NS(HISI_PMU);
+
 #define HISI_PMU_V2		0x30
 #define HISI_MAX_COUNTERS 0x10
 #define to_hisi_pmu(p)	(container_of(p, struct hisi_pmu, pmu))
-- 
2.24.0

Re: [PATCH 1/8] drivers/perf: hisi: Define a symbol namespace for HiSilicon Uncore PMUs

[Jonathan Cameron]

> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h
> index 25b2d43b72bf..008f18bbef7e 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.h
> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
> @@ -23,6 +23,8 @@
>  #undef pr_fmt
>  #define pr_fmt(fmt)     "hisi_pmu: " fmt
>  
> +MODULE_IMPORT_NS(HISI_PMU);
> +
Hi Yicong,

Put the module import in each module because then it is easy to see it
right next to the other MODULE_xxxx lines.

https://docs.kernel.org/core-api/symbol-namespaces.html#how-to-use-symbols-exported-in-namespaces

Otherwise patch looks good.

Jonathan


>  #define HISI_PMU_V2		0x30
>  #define HISI_MAX_COUNTERS 0x10
>  #define to_hisi_pmu(p)	(container_of(p, struct hisi_pmu, pmu))

Re: [PATCH 1/8] drivers/perf: hisi: Define a symbol namespace for HiSilicon Uncore PMUs

[Yicong Yang]

On 2024/10/18 20:43, Jonathan Cameron wrote:
> 
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h
>> index 25b2d43b72bf..008f18bbef7e 100644
>> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.h
>> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
>> @@ -23,6 +23,8 @@
>>  #undef pr_fmt
>>  #define pr_fmt(fmt)     "hisi_pmu: " fmt
>>  
>> +MODULE_IMPORT_NS(HISI_PMU);
>> +
> Hi Yicong,
> 
> Put the module import in each module because then it is easy to see it
> right next to the other MODULE_xxxx lines.
> 
> https://docs.kernel.org/core-api/symbol-namespaces.html#how-to-use-symbols-exported-in-namespaces
> 
> Otherwise patch looks good.
> 

Thanks for the hint, will make it in each module.

I think I was referred to https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/pwm.h#n11

Thanks

[PATCH 2/8] drivers/perf: hisi: Improve the detection of associated CPUs

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

Currently we detect the associated CPUs in the cpuhp online callback.
If the CPU's sccl_id or the ccl_id matches the PMU's, they're
associated. There's an exception that some PMUs locate on the SICL
and will match no CPUs. The events of these PMUs can be opened on
any online CPUs. To handle this we just check whether the PMU's
sccl_id is -1, if so we know it locates on SICL and make any CPU
associated to it.

This can be tweaked so in this patch just do the below changes:
- If the PMU doesn't match any CPU then associated it to online CPUs
- Choose the target CPU according to the NUMA affinity for opening
  events

The function is implemented by hisi_pmu_init_associated_cpus() and
invoked in hisi_pmu_init().

Also we maintained the associated_cpus with all the online CPUs. This
is redundant since we'll always schedule the events on the online CPUs.
Get rid of this and make associated_cpus contain offline CPUs as well.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/perf/hisilicon/hisi_uncore_pmu.c | 56 +++++++++++++++++++-----
 drivers/perf/hisilicon/hisi_uncore_pmu.h |  5 +++
 2 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
index 416f72a813fc..5b9373d25f9d 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
@@ -399,6 +399,27 @@ void hisi_uncore_pmu_disable(struct pmu *pmu)
 }
 EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_disable, HISI_PMU);
 
+static void hisi_pmu_init_associated_cpus(struct hisi_pmu *hisi_pmu)
+{
+	/*
+	 * If the associated_cpus has already been initialized, for example
+	 * determined by comparing the sccl_id and ccl_id with the CPU's
+	 * mpidr_el1, then do nothing here. Otherwise the PMU has no affinity
+	 * and could be opened on any online CPU.
+	 */
+	if (!cpumask_empty(&hisi_pmu->associated_cpus))
+		return;
+
+	cpumask_copy(&hisi_pmu->associated_cpus, cpu_online_mask);
+	hisi_pmu->dummy_associated_cpus = true;
+
+	/*
+	 * Otherwise the associated CPUs haven't been online yet. Pick a
+	 * nearest CPU according to the PMU's numa node instead.
+	 */
+	hisi_pmu->on_cpu = cpumask_local_spread(0, dev_to_node(hisi_pmu->dev));
+	WARN_ON(irq_set_affinity(hisi_pmu->irq, cpumask_of(hisi_pmu->on_cpu)));
+}
 
 /*
  * The Super CPU Cluster (SCCL) and CPU Cluster (CCL) IDs can be
@@ -446,10 +467,6 @@ static bool hisi_pmu_cpu_is_associated_pmu(struct hisi_pmu *hisi_pmu)
 {
 	int sccl_id, ccl_id;
 
-	/* If SCCL_ID is -1, the PMU is in a SICL and has no CPU affinity */
-	if (hisi_pmu->sccl_id == -1)
-		return true;
-
 	if (hisi_pmu->ccl_id == -1) {
 		/* If CCL_ID is -1, the PMU only shares the same SCCL */
 		hisi_read_sccl_and_ccl_id(&sccl_id, NULL);
@@ -467,13 +484,29 @@ int hisi_uncore_pmu_online_cpu(unsigned int cpu, struct hlist_node *node)
 	struct hisi_pmu *hisi_pmu = hlist_entry_safe(node, struct hisi_pmu,
 						     node);
 
-	if (!hisi_pmu_cpu_is_associated_pmu(hisi_pmu))
-		return 0;
+	/*
+	 * If the CPU is not in the associated_cpus, it maybe a new CPU we didn't
+	 * access. Test whether it's associated or not.
+	 */
+	if (!cpumask_test_cpu(cpu, &hisi_pmu->associated_cpus)) {
+		if (!hisi_pmu_cpu_is_associated_pmu(hisi_pmu))
+			return 0;
+
+		/*
+		 * We found an associated CPU so we don't need to use the dummy
+		 * associated CPUs. Update it.
+		 */
+		if (hisi_pmu->dummy_associated_cpus) {
+			cpumask_clear(&hisi_pmu->associated_cpus);
+			hisi_pmu->dummy_associated_cpus = false;
+		}
 
-	cpumask_set_cpu(cpu, &hisi_pmu->associated_cpus);
+		cpumask_set_cpu(cpu, &hisi_pmu->associated_cpus);
+	}
 
-	/* If another CPU is already managing this PMU, simply return. */
-	if (hisi_pmu->on_cpu != -1)
+	/* If another associated CPU is already managing this PMU, simply return. */
+	if (hisi_pmu->on_cpu != -1 &&
+	    cpumask_test_cpu(hisi_pmu->on_cpu, &hisi_pmu->associated_cpus))
 		return 0;
 
 	/* Use this CPU in cpumask for event counting */
@@ -492,9 +525,6 @@ int hisi_uncore_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
 						     node);
 	unsigned int target;
 
-	if (!cpumask_test_and_clear_cpu(cpu, &hisi_pmu->associated_cpus))
-		return 0;
-
 	/* Nothing to do if this CPU doesn't own the PMU */
 	if (hisi_pmu->on_cpu != cpu)
 		return 0;
@@ -521,6 +551,8 @@ void hisi_pmu_init(struct hisi_pmu *hisi_pmu, struct module *module)
 {
 	struct pmu *pmu = &hisi_pmu->pmu;
 
+	hisi_pmu_init_associated_cpus(hisi_pmu);
+
 	pmu->module             = module;
 	pmu->parent             = hisi_pmu->dev;
 	pmu->task_ctx_nr        = perf_invalid_context;
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h
index 008f18bbef7e..30f824173bcf 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pmu.h
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
@@ -91,6 +91,11 @@ struct hisi_pmu {
 	struct hisi_pmu_hwevents pmu_events;
 	/* associated_cpus: All CPUs associated with the PMU */
 	cpumask_t associated_cpus;
+	/*
+	 * If the real associated CPUs not onlined by the time initializing,
+	 * we'll initialize with online CPUs and indicate it.
+	 */
+	bool dummy_associated_cpus;
 	/* CPU used for counting */
 	int on_cpu;
 	int irq;
-- 
2.24.0

Re: [PATCH 2/8] drivers/perf: hisi: Improve the detection of associated CPUs

[Jonathan Cameron]

On Fri, 18 Oct 2024 17:57:39 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> Currently we detect the associated CPUs in the cpuhp online callback.
Should really avoid we in patch descriptions.

Currently associated CPUs are detected in the cphup online callback.
etc

> If the CPU's sccl_id or the ccl_id matches the PMU's, they're
> associated. There's an exception that some PMUs locate on the SICL
> and will match no CPUs. The events of these PMUs can be opened on
> any online CPUs. To handle this we just check whether the PMU's
> sccl_id is -1, if so we know it locates on SICL and make any CPU
> associated to it.
> 
> This can be tweaked so in this patch just do the below changes:
> - If the PMU doesn't match any CPU then associated it to online CPUs
> - Choose the target CPU according to the NUMA affinity for opening
>   events
> 
> The function is implemented by hisi_pmu_init_associated_cpus() and
> invoked in hisi_pmu_init().
> 
> Also we maintained the associated_cpus with all the online CPUs. This
> is redundant since we'll always schedule the events on the online CPUs.
> Get rid of this and make associated_cpus contain offline CPUs as well.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

One trivial comment inline otherwise LGTM

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

>  /*
>   * The Super CPU Cluster (SCCL) and CPU Cluster (CCL) IDs can be
> @@ -446,10 +467,6 @@ static bool hisi_pmu_cpu_is_associated_pmu(struct hisi_pmu *hisi_pmu)
>  {
>  	int sccl_id, ccl_id;
>  
> -	/* If SCCL_ID is -1, the PMU is in a SICL and has no CPU affinity */
> -	if (hisi_pmu->sccl_id == -1)
> -		return true;
> -
>  	if (hisi_pmu->ccl_id == -1) {
>  		/* If CCL_ID is -1, the PMU only shares the same SCCL */
>  		hisi_read_sccl_and_ccl_id(&sccl_id, NULL);
> @@ -467,13 +484,29 @@ int hisi_uncore_pmu_online_cpu(unsigned int cpu, struct hlist_node *node)
>  	struct hisi_pmu *hisi_pmu = hlist_entry_safe(node, struct hisi_pmu,
>  						     node);
>  
> -	if (!hisi_pmu_cpu_is_associated_pmu(hisi_pmu))
> -		return 0;
> +	/*
> +	 * If the CPU is not in the associated_cpus, it maybe a new CPU we didn't
> +	 * access. Test whether it's associated or not.
it maybe a new CPU.  Test whether...

I'm not sure what the "didn't access" adds.

> +	 */
> +	if (!cpumask_test_cpu(cpu, &hisi_pmu->associated_cpus)) {
> +		if (!hisi_pmu_cpu_is_associated_pmu(hisi_pmu))
> +			return 0;
> +
> +		/*
> +		 * We found an associated CPU so we don't need to use the dummy
> +		 * associated CPUs. Update it.
> +		 */
> +		if (hisi_pmu->dummy_associated_cpus) {
> +			cpumask_clear(&hisi_pmu->associated_cpus);
> +			hisi_pmu->dummy_associated_cpus = false;
> +		}
>  
> -	cpumask_set_cpu(cpu, &hisi_pmu->associated_cpus);
> +		cpumask_set_cpu(cpu, &hisi_pmu->associated_cpus);
> +	}

Re: [PATCH 2/8] drivers/perf: hisi: Improve the detection of associated CPUs

[Yicong Yang]

On 2024/10/18 20:48, Jonathan Cameron wrote:
> On Fri, 18 Oct 2024 17:57:39 +0800
> Yicong Yang <yangyicong@huawei.com> wrote:
> 
>> From: Yicong Yang <yangyicong@hisilicon.com>
>>
>> Currently we detect the associated CPUs in the cpuhp online callback.
> Should really avoid we in patch descriptions.
> 
> Currently associated CPUs are detected in the cphup online callback.
> etc
> 
>> If the CPU's sccl_id or the ccl_id matches the PMU's, they're
>> associated. There's an exception that some PMUs locate on the SICL
>> and will match no CPUs. The events of these PMUs can be opened on
>> any online CPUs. To handle this we just check whether the PMU's
>> sccl_id is -1, if so we know it locates on SICL and make any CPU
>> associated to it.
>>
>> This can be tweaked so in this patch just do the below changes:
>> - If the PMU doesn't match any CPU then associated it to online CPUs
>> - Choose the target CPU according to the NUMA affinity for opening
>>   events
>>
>> The function is implemented by hisi_pmu_init_associated_cpus() and
>> invoked in hisi_pmu_init().
>>
>> Also we maintained the associated_cpus with all the online CPUs. This
>> is redundant since we'll always schedule the events on the online CPUs.
>> Get rid of this and make associated_cpus contain offline CPUs as well.
>>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> 
> One trivial comment inline otherwise LGTM
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
>>  /*
>>   * The Super CPU Cluster (SCCL) and CPU Cluster (CCL) IDs can be
>> @@ -446,10 +467,6 @@ static bool hisi_pmu_cpu_is_associated_pmu(struct hisi_pmu *hisi_pmu)
>>  {
>>  	int sccl_id, ccl_id;
>>  
>> -	/* If SCCL_ID is -1, the PMU is in a SICL and has no CPU affinity */
>> -	if (hisi_pmu->sccl_id == -1)
>> -		return true;
>> -
>>  	if (hisi_pmu->ccl_id == -1) {
>>  		/* If CCL_ID is -1, the PMU only shares the same SCCL */
>>  		hisi_read_sccl_and_ccl_id(&sccl_id, NULL);
>> @@ -467,13 +484,29 @@ int hisi_uncore_pmu_online_cpu(unsigned int cpu, struct hlist_node *node)
>>  	struct hisi_pmu *hisi_pmu = hlist_entry_safe(node, struct hisi_pmu,
>>  						     node);
>>  
>> -	if (!hisi_pmu_cpu_is_associated_pmu(hisi_pmu))
>> -		return 0;
>> +	/*
>> +	 * If the CPU is not in the associated_cpus, it maybe a new CPU we didn't
>> +	 * access. Test whether it's associated or not.
> it maybe a new CPU.  Test whether...
> 
> I'm not sure what the "didn't access" adds.

I mean the newly onlined CPU hasn't been "tested" by us. Will refine the comments.

Thanks.

> 
>> +	 */
>> +	if (!cpumask_test_cpu(cpu, &hisi_pmu->associated_cpus)) {
>> +		if (!hisi_pmu_cpu_is_associated_pmu(hisi_pmu))
>> +			return 0;
>> +
>> +		/*
>> +		 * We found an associated CPU so we don't need to use the dummy
>> +		 * associated CPUs. Update it.
>> +		 */
>> +		if (hisi_pmu->dummy_associated_cpus) {
>> +			cpumask_clear(&hisi_pmu->associated_cpus);
>> +			hisi_pmu->dummy_associated_cpus = false;
>> +		}
>>  
>> -	cpumask_set_cpu(cpu, &hisi_pmu->associated_cpus);
>> +		cpumask_set_cpu(cpu, &hisi_pmu->associated_cpus);
>> +	}
> .
> 

[PATCH 3/8] drivers/perf: hisi: Extract topology information to a separate structure

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

HiSilicon Uncore PMUs are identified by the IDs of the topology element
on which the PMUs are located. Add a new separate struct hisi_pmu_toplogy
to encapsulate this information. Add additional documentation on the
meaning of each ID.

- make sccl_id and sicl_id into a union since they're exclusive. It can
  also be accessed by scl_id if the SICL/SCCL is unawared.
- make index_id and sub_id to int since we'll make them to -1 if the
  PMU doesn't have this topology or fail to retrieve them

This patch should have no functional changes.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c  | 12 +++---
 drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c | 16 ++++----
 drivers/perf/hisilicon/hisi_uncore_hha_pmu.c  | 10 ++---
 drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c  |  6 +--
 drivers/perf/hisilicon/hisi_uncore_pa_pmu.c   | 12 +++---
 drivers/perf/hisilicon/hisi_uncore_pmu.c      |  7 ++--
 drivers/perf/hisilicon/hisi_uncore_pmu.h      | 38 +++++++++++++++----
 drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c |  8 ++--
 drivers/perf/hisilicon/hisi_uncore_uc_pmu.c   |  9 +++--
 9 files changed, 72 insertions(+), 46 deletions(-)

diff --git a/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
index 0e923f94fa5b..b0fc973f3278 100644
--- a/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
@@ -181,19 +181,19 @@ static int hisi_cpa_pmu_init_data(struct platform_device *pdev,
 				  struct hisi_pmu *cpa_pmu)
 {
 	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
-				     &cpa_pmu->sicl_id)) {
+				     &cpa_pmu->topo.sicl_id)) {
 		dev_err(&pdev->dev, "Can not read sicl-id\n");
 		return -EINVAL;
 	}
 
 	if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
-				     &cpa_pmu->index_id)) {
+				     &cpa_pmu->topo.index_id)) {
 		dev_err(&pdev->dev, "Cannot read idx-id\n");
 		return -EINVAL;
 	}
 
-	cpa_pmu->ccl_id = -1;
-	cpa_pmu->sccl_id = -1;
+	cpa_pmu->topo.ccl_id = -1;
+	cpa_pmu->topo.sccl_id = -1;
 	cpa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(cpa_pmu->base))
 		return PTR_ERR(cpa_pmu->base);
@@ -311,8 +311,8 @@ static int hisi_cpa_pmu_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%d_cpa%u",
-			      cpa_pmu->sicl_id, cpa_pmu->index_id);
+	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%d_cpa%d",
+			      cpa_pmu->topo.sicl_id, cpa_pmu->topo.index_id);
 	if (!name)
 		return -ENOMEM;
 
diff --git a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
index b804e3738113..4fdea7f89989 100644
--- a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
@@ -302,18 +302,18 @@ static int hisi_ddrc_pmu_init_data(struct platform_device *pdev,
 	 * DDRC PMU, while SCCL_ID is in MPIDR[aff2].
 	 */
 	if (device_property_read_u32(&pdev->dev, "hisilicon,ch-id",
-				     &ddrc_pmu->index_id)) {
+				     &ddrc_pmu->topo.index_id)) {
 		dev_err(&pdev->dev, "Can not read ddrc channel-id!\n");
 		return -EINVAL;
 	}
 
 	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
-				     &ddrc_pmu->sccl_id)) {
+				     &ddrc_pmu->topo.sccl_id)) {
 		dev_err(&pdev->dev, "Can not read ddrc sccl-id!\n");
 		return -EINVAL;
 	}
 	/* DDRC PMUs only share the same SCCL */
-	ddrc_pmu->ccl_id = -1;
+	ddrc_pmu->topo.ccl_id = -1;
 
 	ddrc_pmu->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(ddrc_pmu->base)) {
@@ -324,7 +324,7 @@ static int hisi_ddrc_pmu_init_data(struct platform_device *pdev,
 	ddrc_pmu->identifier = readl(ddrc_pmu->base + DDRC_VERSION);
 	if (ddrc_pmu->identifier >= HISI_PMU_V2) {
 		if (device_property_read_u32(&pdev->dev, "hisilicon,sub-id",
-					     &ddrc_pmu->sub_id)) {
+					     &ddrc_pmu->topo.sub_id)) {
 			dev_err(&pdev->dev, "Can not read sub-id!\n");
 			return -EINVAL;
 		}
@@ -502,12 +502,12 @@ static int hisi_ddrc_pmu_probe(struct platform_device *pdev)
 	if (ddrc_pmu->identifier >= HISI_PMU_V2)
 		name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
 				      "hisi_sccl%u_ddrc%u_%u",
-				      ddrc_pmu->sccl_id, ddrc_pmu->index_id,
-				      ddrc_pmu->sub_id);
+				      ddrc_pmu->topo.sccl_id, ddrc_pmu->topo.index_id,
+				      ddrc_pmu->topo.sub_id);
 	else
 		name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
-				      "hisi_sccl%u_ddrc%u", ddrc_pmu->sccl_id,
-				      ddrc_pmu->index_id);
+				      "hisi_sccl%u_ddrc%u", ddrc_pmu->topo.sccl_id,
+				      ddrc_pmu->topo.index_id);
 
 	if (!name)
 		return -ENOMEM;
diff --git a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
index 21e69b1cdd4d..ac0cddac7065 100644
--- a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
@@ -300,7 +300,7 @@ static int hisi_hha_pmu_init_data(struct platform_device *pdev,
 	 * SCCL_ID is in MPIDR[aff2].
 	 */
 	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
-				     &hha_pmu->sccl_id)) {
+				     &hha_pmu->topo.sccl_id)) {
 		dev_err(&pdev->dev, "Can not read hha sccl-id!\n");
 		return -EINVAL;
 	}
@@ -310,7 +310,7 @@ static int hisi_hha_pmu_init_data(struct platform_device *pdev,
 	 * both "hisilicon, idx-id" as preference, if available.
 	 */
 	if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
-				     &hha_pmu->index_id)) {
+				     &hha_pmu->topo.index_id)) {
 		status = acpi_evaluate_integer(ACPI_HANDLE(&pdev->dev),
 					       "_UID", NULL, &id);
 		if (ACPI_FAILURE(status)) {
@@ -318,10 +318,10 @@ static int hisi_hha_pmu_init_data(struct platform_device *pdev,
 			return -EINVAL;
 		}
 
-		hha_pmu->index_id = id;
+		hha_pmu->topo.index_id = id;
 	}
 	/* HHA PMUs only share the same SCCL */
-	hha_pmu->ccl_id = -1;
+	hha_pmu->topo.ccl_id = -1;
 
 	hha_pmu->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(hha_pmu->base)) {
@@ -511,7 +511,7 @@ static int hisi_hha_pmu_probe(struct platform_device *pdev)
 		return ret;
 
 	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sccl%u_hha%u",
-			      hha_pmu->sccl_id, hha_pmu->index_id);
+			      hha_pmu->topo.sccl_id, hha_pmu->topo.index_id);
 	if (!name)
 		return -ENOMEM;
 
diff --git a/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
index 51ba76871097..28e8166b9c2f 100644
--- a/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
@@ -360,13 +360,13 @@ static int hisi_l3c_pmu_init_data(struct platform_device *pdev,
 	 * SCCL_ID is in MPIDR[aff2] and CCL_ID is in MPIDR[aff1].
 	 */
 	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
-				     &l3c_pmu->sccl_id)) {
+				     &l3c_pmu->topo.sccl_id)) {
 		dev_err(&pdev->dev, "Can not read l3c sccl-id!\n");
 		return -EINVAL;
 	}
 
 	if (device_property_read_u32(&pdev->dev, "hisilicon,ccl-id",
-				     &l3c_pmu->ccl_id)) {
+				     &l3c_pmu->topo.ccl_id)) {
 		dev_err(&pdev->dev, "Can not read l3c ccl-id!\n");
 		return -EINVAL;
 	}
@@ -545,7 +545,7 @@ static int hisi_l3c_pmu_probe(struct platform_device *pdev)
 		return ret;
 
 	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sccl%u_l3c%u",
-			      l3c_pmu->sccl_id, l3c_pmu->ccl_id);
+			      l3c_pmu->topo.sccl_id, l3c_pmu->topo.ccl_id);
 	if (!name)
 		return -ENOMEM;
 
diff --git a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
index 3cdb35c741f9..52dfe9835e40 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
@@ -274,19 +274,19 @@ static int hisi_pa_pmu_init_data(struct platform_device *pdev,
 	 * to identify the PA PMU.
 	 */
 	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
-				     &pa_pmu->sicl_id)) {
+				     &pa_pmu->topo.sicl_id)) {
 		dev_err(&pdev->dev, "Cannot read sicl-id!\n");
 		return -EINVAL;
 	}
 
 	if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
-				     &pa_pmu->index_id)) {
+				     &pa_pmu->topo.index_id)) {
 		dev_err(&pdev->dev, "Cannot read idx-id!\n");
 		return -EINVAL;
 	}
 
-	pa_pmu->ccl_id = -1;
-	pa_pmu->sccl_id = -1;
+	pa_pmu->topo.ccl_id = -1;
+	pa_pmu->topo.sccl_id = -1;
 
 	pa_pmu->dev_info = device_get_match_data(&pdev->dev);
 	if (!pa_pmu->dev_info)
@@ -489,8 +489,8 @@ static int hisi_pa_pmu_probe(struct platform_device *pdev)
 		return ret;
 
 	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%d_%s%u",
-			      pa_pmu->sicl_id, pa_pmu->dev_info->name,
-			      pa_pmu->index_id);
+			      pa_pmu->topo.sicl_id, pa_pmu->dev_info->name,
+			      pa_pmu->topo.index_id);
 	if (!name)
 		return -ENOMEM;
 
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
index 5b9373d25f9d..edb4f3179991 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
@@ -465,18 +465,19 @@ static void hisi_read_sccl_and_ccl_id(int *scclp, int *cclp)
  */
 static bool hisi_pmu_cpu_is_associated_pmu(struct hisi_pmu *hisi_pmu)
 {
+	struct hisi_pmu_topology *topo = &hisi_pmu->topo;
 	int sccl_id, ccl_id;
 
-	if (hisi_pmu->ccl_id == -1) {
+	if (topo->ccl_id == -1) {
 		/* If CCL_ID is -1, the PMU only shares the same SCCL */
 		hisi_read_sccl_and_ccl_id(&sccl_id, NULL);
 
-		return sccl_id == hisi_pmu->sccl_id;
+		return sccl_id == topo->sccl_id;
 	}
 
 	hisi_read_sccl_and_ccl_id(&sccl_id, &ccl_id);
 
-	return sccl_id == hisi_pmu->sccl_id && ccl_id == hisi_pmu->ccl_id;
+	return sccl_id == topo->sccl_id && ccl_id == topo->ccl_id;
 }
 
 int hisi_uncore_pmu_online_cpu(unsigned int cpu, struct hlist_node *node)
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h
index 30f824173bcf..97917f37507b 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pmu.h
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
@@ -83,12 +83,43 @@ struct hisi_pmu_hwevents {
 	const struct attribute_group **attr_groups;
 };
 
+/**
+ * struct hisi_pmu_topology - Describe the topology hierarchy on which the PMU
+ *                            is located.
+ * @sccl_id: ID of the SCCL on which the PMU locate is located.
+ * @sicl_id: ID of the SICL on which the PMU locate is located.
+ * @scl_id:  ID used by the core which is unaware of the SCCL/SICL.
+ * @ccl_id: ID of the CCL (CPU cluster) on which the PMU is located.
+ * @index_id: the ID of the PMU module if there're several PMUs at a
+ *            particularly location in the topology.
+ * @sub_id: submodule ID of the PMU. For example we use this for DDRC PMU v2
+ *          since each DDRC has more than one DMC
+ *
+ * The ID will be -1 if the PMU isn't located on a certain topology.
+ */
+struct hisi_pmu_topology {
+	/*
+	 * SCCL (Super CPU CLuster) and SICL (Super I/O Cluster) are parallel
+	 * so a PMU cannot locate on a SCCL and a SICL. If the SCCL/SICL is
+	 * not awared use scl_id instead.
+	 */
+	union {
+		int sccl_id;
+		int sicl_id;
+		int scl_id;
+	};
+	int ccl_id;
+	int index_id;
+	int sub_id;
+};
+
 /* Generic pmu struct for different pmu types */
 struct hisi_pmu {
 	struct pmu pmu;
 	const struct hisi_uncore_ops *ops;
 	const struct hisi_pmu_dev_info *dev_info;
 	struct hisi_pmu_hwevents pmu_events;
+	struct hisi_pmu_topology topo;
 	/* associated_cpus: All CPUs associated with the PMU */
 	cpumask_t associated_cpus;
 	/*
@@ -101,14 +132,7 @@ struct hisi_pmu {
 	int irq;
 	struct device *dev;
 	struct hlist_node node;
-	int sccl_id;
-	int sicl_id;
-	int ccl_id;
 	void __iomem *base;
-	/* the ID of the PMU modules */
-	u32 index_id;
-	/* For DDRC PMU v2: each DDRC has more than one DMC */
-	u32 sub_id;
 	int num_counters;
 	int counter_bits;
 	/* check event code range */
diff --git a/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c
index 765bbd61db26..478e9c420d7c 100644
--- a/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c
@@ -293,19 +293,19 @@ static int hisi_sllc_pmu_init_data(struct platform_device *pdev,
 	 * while SCCL_ID is from MPIDR_EL1 by CPU.
 	 */
 	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
-				     &sllc_pmu->sccl_id)) {
+				     &sllc_pmu->topo.sccl_id)) {
 		dev_err(&pdev->dev, "Cannot read sccl-id!\n");
 		return -EINVAL;
 	}
 
 	if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
-				     &sllc_pmu->index_id)) {
+				     &sllc_pmu->topo.index_id)) {
 		dev_err(&pdev->dev, "Cannot read idx-id!\n");
 		return -EINVAL;
 	}
 
 	/* SLLC PMUs only share the same SCCL */
-	sllc_pmu->ccl_id = -1;
+	sllc_pmu->topo.ccl_id = -1;
 
 	sllc_pmu->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(sllc_pmu->base)) {
@@ -434,7 +434,7 @@ static int hisi_sllc_pmu_probe(struct platform_device *pdev)
 		return ret;
 
 	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sccl%u_sllc%u",
-			      sllc_pmu->sccl_id, sllc_pmu->index_id);
+			      sllc_pmu->topo.sccl_id, sllc_pmu->topo.index_id);
 	if (!name)
 		return -ENOMEM;
 
diff --git a/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
index 481dcc9e8fbf..ac1c0ebfc67b 100644
--- a/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
@@ -372,19 +372,19 @@ static int hisi_uc_pmu_init_data(struct platform_device *pdev,
 	 * They have some CCLs per SCCL and then 4 UC PMU per CCL.
 	 */
 	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
-				     &uc_pmu->sccl_id)) {
+				     &uc_pmu->topo.sccl_id)) {
 		dev_err(&pdev->dev, "Can not read uc sccl-id!\n");
 		return -EINVAL;
 	}
 
 	if (device_property_read_u32(&pdev->dev, "hisilicon,ccl-id",
-				     &uc_pmu->ccl_id)) {
+				     &uc_pmu->topo.ccl_id)) {
 		dev_err(&pdev->dev, "Can not read uc ccl-id!\n");
 		return -EINVAL;
 	}
 
 	if (device_property_read_u32(&pdev->dev, "hisilicon,sub-id",
-				     &uc_pmu->sub_id)) {
+				     &uc_pmu->topo.sub_id)) {
 		dev_err(&pdev->dev, "Can not read sub-id!\n");
 		return -EINVAL;
 	}
@@ -539,7 +539,8 @@ static int hisi_uc_pmu_probe(struct platform_device *pdev)
 		return ret;
 
 	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sccl%d_uc%d_%u",
-			      uc_pmu->sccl_id, uc_pmu->ccl_id, uc_pmu->sub_id);
+			      uc_pmu->topo.sccl_id, uc_pmu->topo.ccl_id,
+			      uc_pmu->topo.sub_id);
 	if (!name)
 		return -ENOMEM;
 
-- 
2.24.0

Re: [PATCH 3/8] drivers/perf: hisi: Extract topology information to a separate structure

[Jonathan Cameron]

On Fri, 18 Oct 2024 17:57:40 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> HiSilicon Uncore PMUs are identified by the IDs of the topology element
> on which the PMUs are located. Add a new separate struct hisi_pmu_toplogy
> to encapsulate this information. Add additional documentation on the
> meaning of each ID.
> 
> - make sccl_id and sicl_id into a union since they're exclusive. It can
>   also be accessed by scl_id if the SICL/SCCL is unawared.

As below. Maybe "distinction is not relevant." as not sure what unwared means here.

> - make index_id and sub_id to int since we'll make them to -1 if the
>   PMU doesn't have this topology or fail to retrieve them
- make index_id and sub_id signed so -1 may be used to indicate
  the PMU doesn't have this topology element or it could not be retrieved.

> 
> This patch should have no functional changes.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
With those small description updates (or similar)

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> +/**
> + * struct hisi_pmu_topology - Describe the topology hierarchy on which the PMU
> + *                            is located.
> + * @sccl_id: ID of the SCCL on which the PMU locate is located.
> + * @sicl_id: ID of the SICL on which the PMU locate is located.
> + * @scl_id:  ID used by the core which is unaware of the SCCL/SICL.
> + * @ccl_id: ID of the CCL (CPU cluster) on which the PMU is located.
> + * @index_id: the ID of the PMU module if there're several PMUs at a
> + *            particularly location in the topology.
> + * @sub_id: submodule ID of the PMU. For example we use this for DDRC PMU v2
> + *          since each DDRC has more than one DMC
> + *
> + * The ID will be -1 if the PMU isn't located on a certain topology.
> + */
> +struct hisi_pmu_topology {
> +	/*
> +	 * SCCL (Super CPU CLuster) and SICL (Super I/O Cluster) are parallel
> +	 * so a PMU cannot locate on a SCCL and a SICL. If the SCCL/SICL is
> +	 * not awared use scl_id instead.
distinction is not relevant?  I' not quite sure what not awared means.

> +	 */
> +	union {
> +		int sccl_id;
> +		int sicl_id;
> +		int scl_id;
> +	};
> +	int ccl_id;
> +	int index_id;
> +	int sub_id;
> +};
> +

Re: [PATCH 3/8] drivers/perf: hisi: Extract topology information to a separate structure

[Yicong Yang]

On 2024/10/18 20:54, Jonathan Cameron wrote:
> On Fri, 18 Oct 2024 17:57:40 +0800
> Yicong Yang <yangyicong@huawei.com> wrote:
> 
>> From: Yicong Yang <yangyicong@hisilicon.com>
>>
>> HiSilicon Uncore PMUs are identified by the IDs of the topology element
>> on which the PMUs are located. Add a new separate struct hisi_pmu_toplogy
>> to encapsulate this information. Add additional documentation on the
>> meaning of each ID.
>>
>> - make sccl_id and sicl_id into a union since they're exclusive. It can
>>   also be accessed by scl_id if the SICL/SCCL is unawared.
> 
> As below. Maybe "distinction is not relevant." as not sure what unwared means here.
> 
>> - make index_id and sub_id to int since we'll make them to -1 if the
>>   PMU doesn't have this topology or fail to retrieve them
> - make index_id and sub_id signed so -1 may be used to indicate
>   the PMU doesn't have this topology element or it could not be retrieved.
> 
>>
>> This patch should have no functional changes.
>>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> With those small description updates (or similar)
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
>> ---
>> +/**
>> + * struct hisi_pmu_topology - Describe the topology hierarchy on which the PMU
>> + *                            is located.
>> + * @sccl_id: ID of the SCCL on which the PMU locate is located.
>> + * @sicl_id: ID of the SICL on which the PMU locate is located.
>> + * @scl_id:  ID used by the core which is unaware of the SCCL/SICL.
>> + * @ccl_id: ID of the CCL (CPU cluster) on which the PMU is located.
>> + * @index_id: the ID of the PMU module if there're several PMUs at a
>> + *            particularly location in the topology.
>> + * @sub_id: submodule ID of the PMU. For example we use this for DDRC PMU v2
>> + *          since each DDRC has more than one DMC
>> + *
>> + * The ID will be -1 if the PMU isn't located on a certain topology.
>> + */
>> +struct hisi_pmu_topology {
>> +	/*
>> +	 * SCCL (Super CPU CLuster) and SICL (Super I/O Cluster) are parallel
>> +	 * so a PMU cannot locate on a SCCL and a SICL. If the SCCL/SICL is
>> +	 * not awared use scl_id instead.
> distinction is not relevant?  I' not quite sure what not awared means.
> 

The SICL or SCCL is only meaningful to the certain driver like L3C, since they
know whether this PMU locates on a SICL or SCCL. For the framework it's not
capable of inferring such information and don't care about it (not awared here).

Will make this piece of comment more clearer.

Thanks.

>> +	 */
>> +	union {
>> +		int sccl_id;
>> +		int sicl_id;
>> +		int scl_id;
>> +	};
>> +	int ccl_id;
>> +	int index_id;
>> +	int sub_id;
>> +};
>> +
> 
> .
> 

[PATCH 4/8] drivers/perf: hisi: Add a common function to retrieve topology from firmware

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

Currently each type of uncore PMU driver uses almost the same routine
and the same firmware interface (or properties) to retrieve the topology
information, then reset the unused IDs to -1. Extract the common parts
to the framework in hisi_uncore_pmu_init_topology().

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c  | 10 +++----
 drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c | 10 +++----
 drivers/perf/hisilicon/hisi_uncore_hha_pmu.c  | 10 +++----
 drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c  |  8 ++---
 drivers/perf/hisilicon/hisi_uncore_pa_pmu.c   | 11 +++----
 drivers/perf/hisilicon/hisi_uncore_pmu.c      | 30 +++++++++++++++++++
 drivers/perf/hisilicon/hisi_uncore_pmu.h      |  1 +
 drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c | 11 +++----
 drivers/perf/hisilicon/hisi_uncore_uc_pmu.c   | 12 ++++----
 9 files changed, 60 insertions(+), 43 deletions(-)

diff --git a/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
index b0fc973f3278..bd1c1799f935 100644
--- a/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
@@ -180,20 +180,18 @@ MODULE_DEVICE_TABLE(acpi, hisi_cpa_pmu_acpi_match);
 static int hisi_cpa_pmu_init_data(struct platform_device *pdev,
 				  struct hisi_pmu *cpa_pmu)
 {
-	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
-				     &cpa_pmu->topo.sicl_id)) {
+	hisi_uncore_pmu_init_topology(cpa_pmu, &pdev->dev);
+
+	if (cpa_pmu->topo.sicl_id < 0) {
 		dev_err(&pdev->dev, "Can not read sicl-id\n");
 		return -EINVAL;
 	}
 
-	if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
-				     &cpa_pmu->topo.index_id)) {
+	if (cpa_pmu->topo.index_id < 0) {
 		dev_err(&pdev->dev, "Cannot read idx-id\n");
 		return -EINVAL;
 	}
 
-	cpa_pmu->topo.ccl_id = -1;
-	cpa_pmu->topo.sccl_id = -1;
 	cpa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(cpa_pmu->base))
 		return PTR_ERR(cpa_pmu->base);
diff --git a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
index 4fdea7f89989..415a95e24993 100644
--- a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
@@ -297,6 +297,8 @@ MODULE_DEVICE_TABLE(acpi, hisi_ddrc_pmu_acpi_match);
 static int hisi_ddrc_pmu_init_data(struct platform_device *pdev,
 				   struct hisi_pmu *ddrc_pmu)
 {
+	hisi_uncore_pmu_init_topology(ddrc_pmu, &pdev->dev);
+
 	/*
 	 * Use the SCCL_ID and DDRC channel ID to identify the
 	 * DDRC PMU, while SCCL_ID is in MPIDR[aff2].
@@ -307,13 +309,10 @@ static int hisi_ddrc_pmu_init_data(struct platform_device *pdev,
 		return -EINVAL;
 	}
 
-	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
-				     &ddrc_pmu->topo.sccl_id)) {
+	if (ddrc_pmu->topo.sccl_id < 0) {
 		dev_err(&pdev->dev, "Can not read ddrc sccl-id!\n");
 		return -EINVAL;
 	}
-	/* DDRC PMUs only share the same SCCL */
-	ddrc_pmu->topo.ccl_id = -1;
 
 	ddrc_pmu->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(ddrc_pmu->base)) {
@@ -323,8 +322,7 @@ static int hisi_ddrc_pmu_init_data(struct platform_device *pdev,
 
 	ddrc_pmu->identifier = readl(ddrc_pmu->base + DDRC_VERSION);
 	if (ddrc_pmu->identifier >= HISI_PMU_V2) {
-		if (device_property_read_u32(&pdev->dev, "hisilicon,sub-id",
-					     &ddrc_pmu->topo.sub_id)) {
+		if (ddrc_pmu->topo.sub_id < 0) {
 			dev_err(&pdev->dev, "Can not read sub-id!\n");
 			return -EINVAL;
 		}
diff --git a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
index ac0cddac7065..43554e4f8a36 100644
--- a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
@@ -295,12 +295,13 @@ static int hisi_hha_pmu_init_data(struct platform_device *pdev,
 	unsigned long long id;
 	acpi_status status;
 
+	hisi_uncore_pmu_init_topology(hha_pmu, &pdev->dev);
+
 	/*
 	 * Use SCCL_ID and UID to identify the HHA PMU, while
 	 * SCCL_ID is in MPIDR[aff2].
 	 */
-	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
-				     &hha_pmu->topo.sccl_id)) {
+	if (hha_pmu->topo.sccl_id < 0) {
 		dev_err(&pdev->dev, "Can not read hha sccl-id!\n");
 		return -EINVAL;
 	}
@@ -309,8 +310,7 @@ static int hisi_hha_pmu_init_data(struct platform_device *pdev,
 	 * Early versions of BIOS support _UID by mistake, so we support
 	 * both "hisilicon, idx-id" as preference, if available.
 	 */
-	if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
-				     &hha_pmu->topo.index_id)) {
+	if (hha_pmu->topo.index_id < 0) {
 		status = acpi_evaluate_integer(ACPI_HANDLE(&pdev->dev),
 					       "_UID", NULL, &id);
 		if (ACPI_FAILURE(status)) {
@@ -320,8 +320,6 @@ static int hisi_hha_pmu_init_data(struct platform_device *pdev,
 
 		hha_pmu->topo.index_id = id;
 	}
-	/* HHA PMUs only share the same SCCL */
-	hha_pmu->topo.ccl_id = -1;
 
 	hha_pmu->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(hha_pmu->base)) {
diff --git a/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
index 28e8166b9c2f..b113620d27f9 100644
--- a/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
@@ -355,18 +355,18 @@ MODULE_DEVICE_TABLE(acpi, hisi_l3c_pmu_acpi_match);
 static int hisi_l3c_pmu_init_data(struct platform_device *pdev,
 				  struct hisi_pmu *l3c_pmu)
 {
+	hisi_uncore_pmu_init_topology(l3c_pmu, &pdev->dev);
+
 	/*
 	 * Use the SCCL_ID and CCL_ID to identify the L3C PMU, while
 	 * SCCL_ID is in MPIDR[aff2] and CCL_ID is in MPIDR[aff1].
 	 */
-	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
-				     &l3c_pmu->topo.sccl_id)) {
+	if (l3c_pmu->topo.sccl_id < 0) {
 		dev_err(&pdev->dev, "Can not read l3c sccl-id!\n");
 		return -EINVAL;
 	}
 
-	if (device_property_read_u32(&pdev->dev, "hisilicon,ccl-id",
-				     &l3c_pmu->topo.ccl_id)) {
+	if (l3c_pmu->topo.ccl_id < 0) {
 		dev_err(&pdev->dev, "Can not read l3c ccl-id!\n");
 		return -EINVAL;
 	}
diff --git a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
index 52dfe9835e40..dbe4d7b97b2b 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
@@ -269,25 +269,22 @@ static void hisi_pa_pmu_clear_int_status(struct hisi_pmu *pa_pmu, int idx)
 static int hisi_pa_pmu_init_data(struct platform_device *pdev,
 				   struct hisi_pmu *pa_pmu)
 {
+	hisi_uncore_pmu_init_topology(pa_pmu, &pdev->dev);
+
 	/*
 	 * As PA PMU is in a SICL, use the SICL_ID and the index ID
 	 * to identify the PA PMU.
 	 */
-	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
-				     &pa_pmu->topo.sicl_id)) {
+	if (pa_pmu->topo.sicl_id < 0) {
 		dev_err(&pdev->dev, "Cannot read sicl-id!\n");
 		return -EINVAL;
 	}
 
-	if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
-				     &pa_pmu->topo.index_id)) {
+	if (pa_pmu->topo.index_id < 0) {
 		dev_err(&pdev->dev, "Cannot read idx-id!\n");
 		return -EINVAL;
 	}
 
-	pa_pmu->topo.ccl_id = -1;
-	pa_pmu->topo.sccl_id = -1;
-
 	pa_pmu->dev_info = device_get_match_data(&pdev->dev);
 	if (!pa_pmu->dev_info)
 		return -ENODEV;
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
index edb4f3179991..e1756e639784 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
@@ -14,6 +14,7 @@
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/interrupt.h>
+#include <linux/property.h>
 
 #include <asm/cputype.h>
 #include <asm/local64.h>
@@ -548,6 +549,35 @@ int hisi_uncore_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
 }
 EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_offline_cpu, HISI_PMU);
 
+/*
+ * Retrieve the topology information from the firmware for the hisi_pmu device.
+ * The topology ID will be -1 if we cannot initialize it, it may either due to
+ * the PMU doesn't locate on this certain topology or the firmware needs to be
+ * fixed.
+ */
+void hisi_uncore_pmu_init_topology(struct hisi_pmu *hisi_pmu, struct device *dev)
+{
+	struct hisi_pmu_topology *topo = &hisi_pmu->topo;
+
+	topo->sccl_id = -1;
+	topo->ccl_id = -1;
+	topo->index_id = -1;
+	topo->sub_id = -1;
+
+	if (device_property_read_u32(dev, "hisilicon,scl-id", &topo->sccl_id))
+		dev_dbg(dev, "no scl-id present\n");
+
+	if (device_property_read_u32(dev, "hisilicon,ccl-id", &topo->ccl_id))
+		dev_dbg(dev, "no ccl-id present\n");
+
+	if (device_property_read_u32(dev, "hisilicon,idx-id", &topo->index_id))
+		dev_dbg(dev, "no idx-id present\n");
+
+	if (device_property_read_u32(dev, "hisilicon,sub-id", &topo->sub_id))
+		dev_dbg(dev, "no sub-id present\n");
+}
+EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_init_topology, HISI_PMU);
+
 void hisi_pmu_init(struct hisi_pmu *hisi_pmu, struct module *module)
 {
 	struct pmu *pmu = &hisi_pmu->pmu;
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h
index 97917f37507b..239c45d847b3 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pmu.h
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
@@ -163,6 +163,7 @@ ssize_t hisi_uncore_pmu_identifier_attr_show(struct device *dev,
 					     char *page);
 int hisi_uncore_pmu_init_irq(struct hisi_pmu *hisi_pmu,
 			     struct platform_device *pdev);
+void hisi_uncore_pmu_init_topology(struct hisi_pmu *hisi_pmu, struct device *dev);
 
 void hisi_pmu_init(struct hisi_pmu *hisi_pmu, struct module *module);
 #endif /* __HISI_UNCORE_PMU_H__ */
diff --git a/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c
index 478e9c420d7c..43cbdf0fb7c7 100644
--- a/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c
@@ -288,25 +288,22 @@ MODULE_DEVICE_TABLE(acpi, hisi_sllc_pmu_acpi_match);
 static int hisi_sllc_pmu_init_data(struct platform_device *pdev,
 				   struct hisi_pmu *sllc_pmu)
 {
+	hisi_uncore_pmu_init_topology(sllc_pmu, &pdev->dev);
+
 	/*
 	 * Use the SCCL_ID and the index ID to identify the SLLC PMU,
 	 * while SCCL_ID is from MPIDR_EL1 by CPU.
 	 */
-	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
-				     &sllc_pmu->topo.sccl_id)) {
+	if (sllc_pmu->topo.sccl_id < 0) {
 		dev_err(&pdev->dev, "Cannot read sccl-id!\n");
 		return -EINVAL;
 	}
 
-	if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
-				     &sllc_pmu->topo.index_id)) {
+	if (sllc_pmu->topo.index_id < 0) {
 		dev_err(&pdev->dev, "Cannot read idx-id!\n");
 		return -EINVAL;
 	}
 
-	/* SLLC PMUs only share the same SCCL */
-	sllc_pmu->topo.ccl_id = -1;
-
 	sllc_pmu->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(sllc_pmu->base)) {
 		dev_err(&pdev->dev, "ioremap failed for sllc_pmu resource.\n");
diff --git a/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
index ac1c0ebfc67b..2040f6a8871e 100644
--- a/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
@@ -11,7 +11,6 @@
 #include <linux/irq.h>
 #include <linux/list.h>
 #include <linux/mod_devicetable.h>
-#include <linux/property.h>
 
 #include "hisi_uncore_pmu.h"
 
@@ -366,25 +365,24 @@ static void hisi_uc_pmu_clear_int_status(struct hisi_pmu *uc_pmu, int idx)
 static int hisi_uc_pmu_init_data(struct platform_device *pdev,
 				 struct hisi_pmu *uc_pmu)
 {
+	hisi_uncore_pmu_init_topology(uc_pmu, &pdev->dev);
+
 	/*
 	 * Use SCCL (Super CPU Cluster) ID and CCL (CPU Cluster) ID to
 	 * identify the topology information of UC PMU devices in the chip.
 	 * They have some CCLs per SCCL and then 4 UC PMU per CCL.
 	 */
-	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
-				     &uc_pmu->topo.sccl_id)) {
+	if (uc_pmu->topo.sccl_id < 0) {
 		dev_err(&pdev->dev, "Can not read uc sccl-id!\n");
 		return -EINVAL;
 	}
 
-	if (device_property_read_u32(&pdev->dev, "hisilicon,ccl-id",
-				     &uc_pmu->topo.ccl_id)) {
+	if (uc_pmu->topo.ccl_id < 0) {
 		dev_err(&pdev->dev, "Can not read uc ccl-id!\n");
 		return -EINVAL;
 	}
 
-	if (device_property_read_u32(&pdev->dev, "hisilicon,sub-id",
-				     &uc_pmu->topo.sub_id)) {
+	if (uc_pmu->topo.sub_id < 0) {
 		dev_err(&pdev->dev, "Can not read sub-id!\n");
 		return -EINVAL;
 	}
-- 
2.24.0

Re: [PATCH 4/8] drivers/perf: hisi: Add a common function to retrieve topology from firmware

[Jonathan Cameron]

On Fri, 18 Oct 2024 17:57:41 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> Currently each type of uncore PMU driver uses almost the same routine
> and the same firmware interface (or properties) to retrieve the topology
> information, then reset the unused IDs to -1. Extract the common parts
> to the framework in hisi_uncore_pmu_init_topology().
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Patch looks good to me

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

but it did leave me wondering why there was no removal of property.h
from some of the drivers now all the property stuff is central.

Seems they don't include, it but do include acpi.h which they
should not.  So a nice follow up would be to drop the acpi.h
includes in favour of mod_devicetable.h which is needed
for the id tables.

Jonathan

[PATCH 5/8] drivers/perf: hisi: Refactor the attributes creation

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

Each type of HiSilicon Uncore PMU has the following sysfs attributes:

- format: bitmask in perf_event_attr::config[012] of corresponding
  attribute
- event: events name and corresponding event code
- cpumask: range of CPUs the events can be opened on
- identifier: the version of this PMU

Different types of PMU have different implementations of the "format"
and "event" but all share the same implementation of the "cpumask"
and "identifier". Thus we can move cpumask and identifier to the
hisi_uncore_pmu framework and drivers can use the generic
implementation.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c  | 41 +++----------
 drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c | 55 +++++-------------
 drivers/perf/hisilicon/hisi_uncore_hha_pmu.c  | 57 ++++++-------------
 drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c  | 55 +++++-------------
 drivers/perf/hisilicon/hisi_uncore_pa_pmu.c   | 39 +++----------
 drivers/perf/hisilicon/hisi_uncore_pmu.c      | 45 +++++++++++----
 drivers/perf/hisilicon/hisi_uncore_pmu.h      | 14 ++++-
 drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c | 41 +++----------
 drivers/perf/hisilicon/hisi_uncore_uc_pmu.c   | 41 +++----------
 9 files changed, 128 insertions(+), 260 deletions(-)

diff --git a/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
index bd1c1799f935..4dd52eba9f27 100644
--- a/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
@@ -225,37 +225,6 @@ static const struct attribute_group hisi_cpa_pmu_events_group = {
 	.attrs = hisi_cpa_pmu_events_attr,
 };
 
-static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
-
-static struct attribute *hisi_cpa_pmu_cpumask_attrs[] = {
-	&dev_attr_cpumask.attr,
-	NULL
-};
-
-static const struct attribute_group hisi_cpa_pmu_cpumask_attr_group = {
-	.attrs = hisi_cpa_pmu_cpumask_attrs,
-};
-
-static struct device_attribute hisi_cpa_pmu_identifier_attr =
-	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
-
-static struct attribute *hisi_cpa_pmu_identifier_attrs[] = {
-	&hisi_cpa_pmu_identifier_attr.attr,
-	NULL
-};
-
-static const struct attribute_group hisi_cpa_pmu_identifier_group = {
-	.attrs = hisi_cpa_pmu_identifier_attrs,
-};
-
-static const struct attribute_group *hisi_cpa_pmu_attr_groups[] = {
-	&hisi_cpa_pmu_format_group,
-	&hisi_cpa_pmu_events_group,
-	&hisi_cpa_pmu_cpumask_attr_group,
-	&hisi_cpa_pmu_identifier_group,
-	NULL
-};
-
 static const struct hisi_uncore_ops hisi_uncore_cpa_pmu_ops = {
 	.write_evtype           = hisi_cpa_pmu_write_evtype,
 	.get_event_idx		= hisi_uncore_pmu_get_event_idx,
@@ -274,6 +243,7 @@ static const struct hisi_uncore_ops hisi_uncore_cpa_pmu_ops = {
 static int hisi_cpa_pmu_dev_probe(struct platform_device *pdev,
 				  struct hisi_pmu *cpa_pmu)
 {
+	struct hisi_pmu_hwevents *pmu_events = &cpa_pmu->pmu_events;
 	int ret;
 
 	ret = hisi_cpa_pmu_init_data(pdev, cpa_pmu);
@@ -286,7 +256,14 @@ static int hisi_cpa_pmu_dev_probe(struct platform_device *pdev,
 
 	cpa_pmu->counter_bits = CPA_COUNTER_BITS;
 	cpa_pmu->check_event = CPA_NR_EVENTS;
-	cpa_pmu->pmu_events.attr_groups = hisi_cpa_pmu_attr_groups;
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
+						&hisi_cpa_pmu_format_group;
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
+						&hisi_cpa_pmu_events_group;
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_CPUMASK] =
+						&hisi_pmu_cpumask_attr_group;
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_IDENTIFIER] =
+						&hisi_pmu_identifier_group;
 	cpa_pmu->ops = &hisi_uncore_cpa_pmu_ops;
 	cpa_pmu->num_counters = CPA_NR_COUNTERS;
 	cpa_pmu->dev = &pdev->dev;
diff --git a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
index 415a95e24993..6d805ca4562f 100644
--- a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
@@ -380,45 +380,6 @@ static const struct attribute_group hisi_ddrc_pmu_v2_events_group = {
 	.attrs = hisi_ddrc_pmu_v2_events_attr,
 };
 
-static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
-
-static struct attribute *hisi_ddrc_pmu_cpumask_attrs[] = {
-	&dev_attr_cpumask.attr,
-	NULL,
-};
-
-static const struct attribute_group hisi_ddrc_pmu_cpumask_attr_group = {
-	.attrs = hisi_ddrc_pmu_cpumask_attrs,
-};
-
-static struct device_attribute hisi_ddrc_pmu_identifier_attr =
-	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
-
-static struct attribute *hisi_ddrc_pmu_identifier_attrs[] = {
-	&hisi_ddrc_pmu_identifier_attr.attr,
-	NULL
-};
-
-static const struct attribute_group hisi_ddrc_pmu_identifier_group = {
-	.attrs = hisi_ddrc_pmu_identifier_attrs,
-};
-
-static const struct attribute_group *hisi_ddrc_pmu_v1_attr_groups[] = {
-	&hisi_ddrc_pmu_v1_format_group,
-	&hisi_ddrc_pmu_v1_events_group,
-	&hisi_ddrc_pmu_cpumask_attr_group,
-	&hisi_ddrc_pmu_identifier_group,
-	NULL,
-};
-
-static const struct attribute_group *hisi_ddrc_pmu_v2_attr_groups[] = {
-	&hisi_ddrc_pmu_v2_format_group,
-	&hisi_ddrc_pmu_v2_events_group,
-	&hisi_ddrc_pmu_cpumask_attr_group,
-	&hisi_ddrc_pmu_identifier_group,
-	NULL
-};
-
 static const struct hisi_uncore_ops hisi_uncore_ddrc_v1_ops = {
 	.write_evtype           = hisi_ddrc_pmu_write_evtype,
 	.get_event_idx		= hisi_ddrc_pmu_v1_get_event_idx,
@@ -452,6 +413,7 @@ static const struct hisi_uncore_ops hisi_uncore_ddrc_v2_ops = {
 static int hisi_ddrc_pmu_dev_probe(struct platform_device *pdev,
 				   struct hisi_pmu *ddrc_pmu)
 {
+	struct hisi_pmu_hwevents *pmu_events = &ddrc_pmu->pmu_events;
 	int ret;
 
 	ret = hisi_ddrc_pmu_init_data(pdev, ddrc_pmu);
@@ -465,15 +427,26 @@ static int hisi_ddrc_pmu_dev_probe(struct platform_device *pdev,
 	if (ddrc_pmu->identifier >= HISI_PMU_V2) {
 		ddrc_pmu->counter_bits = 48;
 		ddrc_pmu->check_event = DDRC_V2_NR_EVENTS;
-		ddrc_pmu->pmu_events.attr_groups = hisi_ddrc_pmu_v2_attr_groups;
+		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
+						&hisi_ddrc_pmu_v2_format_group;
+		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
+						&hisi_ddrc_pmu_v2_events_group;
 		ddrc_pmu->ops = &hisi_uncore_ddrc_v2_ops;
 	} else {
 		ddrc_pmu->counter_bits = 32;
 		ddrc_pmu->check_event = DDRC_V1_NR_EVENTS;
-		ddrc_pmu->pmu_events.attr_groups = hisi_ddrc_pmu_v1_attr_groups;
+		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
+						&hisi_ddrc_pmu_v1_format_group;
+		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
+						&hisi_ddrc_pmu_v1_events_group;
 		ddrc_pmu->ops = &hisi_uncore_ddrc_v1_ops;
 	}
 
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_CPUMASK] =
+						&hisi_pmu_cpumask_attr_group;
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_IDENTIFIER] =
+						&hisi_pmu_identifier_group;
+
 	ddrc_pmu->num_counters = DDRC_NR_COUNTERS;
 	ddrc_pmu->dev = &pdev->dev;
 	ddrc_pmu->on_cpu = -1;
diff --git a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
index 43554e4f8a36..07cab6cf4897 100644
--- a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
@@ -405,45 +405,6 @@ static const struct attribute_group hisi_hha_pmu_v2_events_group = {
 	.attrs = hisi_hha_pmu_v2_events_attr,
 };
 
-static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
-
-static struct attribute *hisi_hha_pmu_cpumask_attrs[] = {
-	&dev_attr_cpumask.attr,
-	NULL,
-};
-
-static const struct attribute_group hisi_hha_pmu_cpumask_attr_group = {
-	.attrs = hisi_hha_pmu_cpumask_attrs,
-};
-
-static struct device_attribute hisi_hha_pmu_identifier_attr =
-	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
-
-static struct attribute *hisi_hha_pmu_identifier_attrs[] = {
-	&hisi_hha_pmu_identifier_attr.attr,
-	NULL
-};
-
-static const struct attribute_group hisi_hha_pmu_identifier_group = {
-	.attrs = hisi_hha_pmu_identifier_attrs,
-};
-
-static const struct attribute_group *hisi_hha_pmu_v1_attr_groups[] = {
-	&hisi_hha_pmu_v1_format_group,
-	&hisi_hha_pmu_v1_events_group,
-	&hisi_hha_pmu_cpumask_attr_group,
-	&hisi_hha_pmu_identifier_group,
-	NULL,
-};
-
-static const struct attribute_group *hisi_hha_pmu_v2_attr_groups[] = {
-	&hisi_hha_pmu_v2_format_group,
-	&hisi_hha_pmu_v2_events_group,
-	&hisi_hha_pmu_cpumask_attr_group,
-	&hisi_hha_pmu_identifier_group,
-	NULL
-};
-
 static const struct hisi_uncore_ops hisi_uncore_hha_ops = {
 	.write_evtype		= hisi_hha_pmu_write_evtype,
 	.get_event_idx		= hisi_uncore_pmu_get_event_idx,
@@ -464,6 +425,7 @@ static const struct hisi_uncore_ops hisi_uncore_hha_ops = {
 static int hisi_hha_pmu_dev_probe(struct platform_device *pdev,
 				  struct hisi_pmu *hha_pmu)
 {
+	struct hisi_pmu_hwevents *pmu_events = &hha_pmu->pmu_events;
 	int ret;
 
 	ret = hisi_hha_pmu_init_data(pdev, hha_pmu);
@@ -477,14 +439,27 @@ static int hisi_hha_pmu_dev_probe(struct platform_device *pdev,
 	if (hha_pmu->identifier >= HISI_PMU_V2) {
 		hha_pmu->counter_bits = 64;
 		hha_pmu->check_event = HHA_V2_NR_EVENT;
-		hha_pmu->pmu_events.attr_groups = hisi_hha_pmu_v2_attr_groups;
+		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
+						&hisi_hha_pmu_v2_format_group;
+		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
+						&hisi_hha_pmu_v2_events_group;
 		hha_pmu->num_counters = HHA_V2_NR_COUNTERS;
 	} else {
 		hha_pmu->counter_bits = 48;
 		hha_pmu->check_event = HHA_V1_NR_EVENT;
-		hha_pmu->pmu_events.attr_groups = hisi_hha_pmu_v1_attr_groups;
+		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
+						&hisi_hha_pmu_v1_format_group;
+		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
+						&hisi_hha_pmu_v1_events_group;
 		hha_pmu->num_counters = HHA_V1_NR_COUNTERS;
 	}
+
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_CPUMASK] =
+						&hisi_pmu_cpumask_attr_group;
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_IDENTIFIER] =
+						&hisi_pmu_identifier_group;
+
+
 	hha_pmu->ops = &hisi_uncore_hha_ops;
 	hha_pmu->dev = &pdev->dev;
 	hha_pmu->on_cpu = -1;
diff --git a/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
index b113620d27f9..6f540ab1f451 100644
--- a/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
@@ -441,45 +441,6 @@ static const struct attribute_group hisi_l3c_pmu_v2_events_group = {
 	.attrs = hisi_l3c_pmu_v2_events_attr,
 };
 
-static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
-
-static struct attribute *hisi_l3c_pmu_cpumask_attrs[] = {
-	&dev_attr_cpumask.attr,
-	NULL,
-};
-
-static const struct attribute_group hisi_l3c_pmu_cpumask_attr_group = {
-	.attrs = hisi_l3c_pmu_cpumask_attrs,
-};
-
-static struct device_attribute hisi_l3c_pmu_identifier_attr =
-	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
-
-static struct attribute *hisi_l3c_pmu_identifier_attrs[] = {
-	&hisi_l3c_pmu_identifier_attr.attr,
-	NULL
-};
-
-static const struct attribute_group hisi_l3c_pmu_identifier_group = {
-	.attrs = hisi_l3c_pmu_identifier_attrs,
-};
-
-static const struct attribute_group *hisi_l3c_pmu_v1_attr_groups[] = {
-	&hisi_l3c_pmu_v1_format_group,
-	&hisi_l3c_pmu_v1_events_group,
-	&hisi_l3c_pmu_cpumask_attr_group,
-	&hisi_l3c_pmu_identifier_group,
-	NULL,
-};
-
-static const struct attribute_group *hisi_l3c_pmu_v2_attr_groups[] = {
-	&hisi_l3c_pmu_v2_format_group,
-	&hisi_l3c_pmu_v2_events_group,
-	&hisi_l3c_pmu_cpumask_attr_group,
-	&hisi_l3c_pmu_identifier_group,
-	NULL
-};
-
 static const struct hisi_uncore_ops hisi_uncore_l3c_ops = {
 	.write_evtype		= hisi_l3c_pmu_write_evtype,
 	.get_event_idx		= hisi_uncore_pmu_get_event_idx,
@@ -500,6 +461,7 @@ static const struct hisi_uncore_ops hisi_uncore_l3c_ops = {
 static int hisi_l3c_pmu_dev_probe(struct platform_device *pdev,
 				  struct hisi_pmu *l3c_pmu)
 {
+	struct hisi_pmu_hwevents *pmu_events = &l3c_pmu->pmu_events;
 	int ret;
 
 	ret = hisi_l3c_pmu_init_data(pdev, l3c_pmu);
@@ -513,13 +475,24 @@ static int hisi_l3c_pmu_dev_probe(struct platform_device *pdev,
 	if (l3c_pmu->identifier >= HISI_PMU_V2) {
 		l3c_pmu->counter_bits = 64;
 		l3c_pmu->check_event = L3C_V2_NR_EVENTS;
-		l3c_pmu->pmu_events.attr_groups = hisi_l3c_pmu_v2_attr_groups;
+		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
+						 &hisi_l3c_pmu_v2_format_group;
+		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
+						 &hisi_l3c_pmu_v2_events_group;
 	} else {
 		l3c_pmu->counter_bits = 48;
 		l3c_pmu->check_event = L3C_V1_NR_EVENTS;
-		l3c_pmu->pmu_events.attr_groups = hisi_l3c_pmu_v1_attr_groups;
+		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
+						&hisi_l3c_pmu_v1_format_group;
+		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
+						&hisi_l3c_pmu_v1_events_group;
 	}
 
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_CPUMASK] =
+						&hisi_pmu_cpumask_attr_group;
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_IDENTIFIER] =
+						&hisi_pmu_identifier_group;
+
 	l3c_pmu->num_counters = L3C_NR_COUNTERS;
 	l3c_pmu->ops = &hisi_uncore_l3c_ops;
 	l3c_pmu->dev = &pdev->dev;
diff --git a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
index dbe4d7b97b2b..69931e73a3cd 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
@@ -353,29 +353,6 @@ static const struct attribute_group hisi_h60pa_pmu_events_group = {
 	.attrs = hisi_h60pa_pmu_events_attr,
 };
 
-static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
-
-static struct attribute *hisi_pa_pmu_cpumask_attrs[] = {
-	&dev_attr_cpumask.attr,
-	NULL
-};
-
-static const struct attribute_group hisi_pa_pmu_cpumask_attr_group = {
-	.attrs = hisi_pa_pmu_cpumask_attrs,
-};
-
-static struct device_attribute hisi_pa_pmu_identifier_attr =
-	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
-
-static struct attribute *hisi_pa_pmu_identifier_attrs[] = {
-	&hisi_pa_pmu_identifier_attr.attr,
-	NULL
-};
-
-static const struct attribute_group hisi_pa_pmu_identifier_group = {
-	.attrs = hisi_pa_pmu_identifier_attrs,
-};
-
 static struct hisi_pa_pmu_int_regs hisi_pa_pmu_regs = {
 	.mask_offset = PA_INT_MASK,
 	.clear_offset = PA_INT_CLEAR,
@@ -385,8 +362,6 @@ static struct hisi_pa_pmu_int_regs hisi_pa_pmu_regs = {
 static const struct attribute_group *hisi_pa_pmu_v2_attr_groups[] = {
 	&hisi_pa_pmu_v2_format_group,
 	&hisi_pa_pmu_v2_events_group,
-	&hisi_pa_pmu_cpumask_attr_group,
-	&hisi_pa_pmu_identifier_group,
 	NULL
 };
 
@@ -399,8 +374,6 @@ static const struct hisi_pmu_dev_info hisi_h32pa_v2 = {
 static const struct attribute_group *hisi_pa_pmu_v3_attr_groups[] = {
 	&hisi_pa_pmu_v2_format_group,
 	&hisi_pa_pmu_v3_events_group,
-	&hisi_pa_pmu_cpumask_attr_group,
-	&hisi_pa_pmu_identifier_group,
 	NULL
 };
 
@@ -419,8 +392,6 @@ static struct hisi_pa_pmu_int_regs hisi_h60pa_pmu_regs = {
 static const struct attribute_group *hisi_h60pa_pmu_attr_groups[] = {
 	&hisi_pa_pmu_v2_format_group,
 	&hisi_h60pa_pmu_events_group,
-	&hisi_pa_pmu_cpumask_attr_group,
-	&hisi_pa_pmu_identifier_group,
 	NULL
 };
 
@@ -450,6 +421,7 @@ static const struct hisi_uncore_ops hisi_uncore_pa_ops = {
 static int hisi_pa_pmu_dev_probe(struct platform_device *pdev,
 				 struct hisi_pmu *pa_pmu)
 {
+	struct hisi_pmu_hwevents *pmu_events = &pa_pmu->pmu_events;
 	int ret;
 
 	ret = hisi_pa_pmu_init_data(pdev, pa_pmu);
@@ -460,7 +432,14 @@ static int hisi_pa_pmu_dev_probe(struct platform_device *pdev,
 	if (ret)
 		return ret;
 
-	pa_pmu->pmu_events.attr_groups = pa_pmu->dev_info->attr_groups;
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
+		pa_pmu->dev_info->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT];
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
+		pa_pmu->dev_info->attr_groups[HISI_PMU_ATTR_GROUP_EVENT];
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_CPUMASK] =
+						&hisi_pmu_cpumask_attr_group;
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_IDENTIFIER] =
+						&hisi_pmu_identifier_group;
 	pa_pmu->num_counters = PA_NR_COUNTERS;
 	pa_pmu->ops = &hisi_uncore_pa_ops;
 	pa_pmu->check_event = 0xB0;
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
index e1756e639784..648d0e5e08ed 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
@@ -49,6 +49,41 @@ ssize_t hisi_cpumask_sysfs_show(struct device *dev,
 }
 EXPORT_SYMBOL_NS_GPL(hisi_cpumask_sysfs_show, HISI_PMU);
 
+static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
+
+static struct attribute *hisi_pmu_cpumask_attrs[] = {
+	&dev_attr_cpumask.attr,
+	NULL
+};
+
+const struct attribute_group hisi_pmu_cpumask_attr_group = {
+	.attrs = hisi_pmu_cpumask_attrs,
+};
+EXPORT_SYMBOL_NS_GPL(hisi_pmu_cpumask_attr_group, HISI_PMU);
+
+ssize_t hisi_uncore_pmu_identifier_attr_show(struct device *dev,
+					     struct device_attribute *attr,
+					     char *page)
+{
+	struct hisi_pmu *hisi_pmu = to_hisi_pmu(dev_get_drvdata(dev));
+
+	return sysfs_emit(page, "0x%08x\n", hisi_pmu->identifier);
+}
+EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_identifier_attr_show, HISI_PMU);
+
+static struct device_attribute hisi_pmu_identifier_attr =
+	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
+
+static struct attribute *hisi_pmu_identifier_attrs[] = {
+	&hisi_pmu_identifier_attr.attr,
+	NULL
+};
+
+const struct attribute_group hisi_pmu_identifier_group = {
+	.attrs = hisi_pmu_identifier_attrs,
+};
+EXPORT_SYMBOL_NS_GPL(hisi_pmu_identifier_group, HISI_PMU);
+
 static bool hisi_validate_event_group(struct perf_event *event)
 {
 	struct perf_event *sibling, *leader = event->group_leader;
@@ -99,16 +134,6 @@ int hisi_uncore_pmu_get_event_idx(struct perf_event *event)
 }
 EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_get_event_idx, HISI_PMU);
 
-ssize_t hisi_uncore_pmu_identifier_attr_show(struct device *dev,
-					     struct device_attribute *attr,
-					     char *page)
-{
-	struct hisi_pmu *hisi_pmu = to_hisi_pmu(dev_get_drvdata(dev));
-
-	return sysfs_emit(page, "0x%08x\n", hisi_pmu->identifier);
-}
-EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_identifier_attr_show, HISI_PMU);
-
 static void hisi_uncore_pmu_clear_event_idx(struct hisi_pmu *hisi_pmu, int idx)
 {
 	clear_bit(idx, hisi_pmu->pmu_events.used_mask);
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h
index 239c45d847b3..ac2be8c337b7 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pmu.h
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
@@ -77,10 +77,22 @@ struct hisi_pmu_dev_info {
 	void *private;
 };
 
+enum hisi_pmu_attr_groups {
+	HISI_PMU_ATTR_GROUP_FORMAT,
+	HISI_PMU_ATTR_GROUP_EVENT,
+	HISI_PMU_ATTR_GROUP_CPUMASK,
+	HISI_PMU_ATTR_GROUP_IDENTIFIER,
+	HISI_PMU_ATTR_GROUP_NR
+};
+
+/* Generic implementation of cpumask/identifier group */
+extern const struct attribute_group hisi_pmu_cpumask_attr_group;
+extern const struct attribute_group hisi_pmu_identifier_group;
+
 struct hisi_pmu_hwevents {
 	struct perf_event *hw_events[HISI_MAX_COUNTERS];
 	DECLARE_BITMAP(used_mask, HISI_MAX_COUNTERS);
-	const struct attribute_group **attr_groups;
+	const struct attribute_group *attr_groups[HISI_PMU_ATTR_GROUP_NR + 1];
 };
 
 /**
diff --git a/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c
index 43cbdf0fb7c7..676a7078f2ef 100644
--- a/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c
@@ -344,37 +344,6 @@ static const struct attribute_group hisi_sllc_pmu_v2_events_group = {
 	.attrs = hisi_sllc_pmu_v2_events_attr,
 };
 
-static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
-
-static struct attribute *hisi_sllc_pmu_cpumask_attrs[] = {
-	&dev_attr_cpumask.attr,
-	NULL
-};
-
-static const struct attribute_group hisi_sllc_pmu_cpumask_attr_group = {
-	.attrs = hisi_sllc_pmu_cpumask_attrs,
-};
-
-static struct device_attribute hisi_sllc_pmu_identifier_attr =
-	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
-
-static struct attribute *hisi_sllc_pmu_identifier_attrs[] = {
-	&hisi_sllc_pmu_identifier_attr.attr,
-	NULL
-};
-
-static const struct attribute_group hisi_sllc_pmu_identifier_group = {
-	.attrs = hisi_sllc_pmu_identifier_attrs,
-};
-
-static const struct attribute_group *hisi_sllc_pmu_v2_attr_groups[] = {
-	&hisi_sllc_pmu_v2_format_group,
-	&hisi_sllc_pmu_v2_events_group,
-	&hisi_sllc_pmu_cpumask_attr_group,
-	&hisi_sllc_pmu_identifier_group,
-	NULL
-};
-
 static const struct hisi_uncore_ops hisi_uncore_sllc_ops = {
 	.write_evtype		= hisi_sllc_pmu_write_evtype,
 	.get_event_idx		= hisi_uncore_pmu_get_event_idx,
@@ -395,6 +364,7 @@ static const struct hisi_uncore_ops hisi_uncore_sllc_ops = {
 static int hisi_sllc_pmu_dev_probe(struct platform_device *pdev,
 				   struct hisi_pmu *sllc_pmu)
 {
+	struct hisi_pmu_hwevents *pmu_events = &sllc_pmu->pmu_events;
 	int ret;
 
 	ret = hisi_sllc_pmu_init_data(pdev, sllc_pmu);
@@ -405,7 +375,14 @@ static int hisi_sllc_pmu_dev_probe(struct platform_device *pdev,
 	if (ret)
 		return ret;
 
-	sllc_pmu->pmu_events.attr_groups = hisi_sllc_pmu_v2_attr_groups;
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
+						&hisi_sllc_pmu_v2_format_group;
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
+						&hisi_sllc_pmu_v2_events_group;
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_CPUMASK] =
+						&hisi_pmu_cpumask_attr_group;
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_IDENTIFIER] =
+						&hisi_pmu_identifier_group;
 	sllc_pmu->ops = &hisi_uncore_sllc_ops;
 	sllc_pmu->check_event = SLLC_NR_EVENTS;
 	sllc_pmu->counter_bits = 64;
diff --git a/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
index 2040f6a8871e..c2ae852f6b19 100644
--- a/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
@@ -437,37 +437,6 @@ static const struct attribute_group hisi_uc_pmu_events_group = {
 	.attrs = hisi_uc_pmu_events_attr,
 };
 
-static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
-
-static struct attribute *hisi_uc_pmu_cpumask_attrs[] = {
-	&dev_attr_cpumask.attr,
-	NULL,
-};
-
-static const struct attribute_group hisi_uc_pmu_cpumask_attr_group = {
-	.attrs = hisi_uc_pmu_cpumask_attrs,
-};
-
-static struct device_attribute hisi_uc_pmu_identifier_attr =
-	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
-
-static struct attribute *hisi_uc_pmu_identifier_attrs[] = {
-	&hisi_uc_pmu_identifier_attr.attr,
-	NULL
-};
-
-static const struct attribute_group hisi_uc_pmu_identifier_group = {
-	.attrs = hisi_uc_pmu_identifier_attrs,
-};
-
-static const struct attribute_group *hisi_uc_pmu_attr_groups[] = {
-	&hisi_uc_pmu_format_group,
-	&hisi_uc_pmu_events_group,
-	&hisi_uc_pmu_cpumask_attr_group,
-	&hisi_uc_pmu_identifier_group,
-	NULL
-};
-
 static const struct hisi_uncore_ops hisi_uncore_uc_pmu_ops = {
 	.check_filter		= hisi_uc_pmu_check_filter,
 	.write_evtype		= hisi_uc_pmu_write_evtype,
@@ -489,6 +458,7 @@ static const struct hisi_uncore_ops hisi_uncore_uc_pmu_ops = {
 static int hisi_uc_pmu_dev_probe(struct platform_device *pdev,
 				 struct hisi_pmu *uc_pmu)
 {
+	struct hisi_pmu_hwevents *pmu_events = &uc_pmu->pmu_events;
 	int ret;
 
 	ret = hisi_uc_pmu_init_data(pdev, uc_pmu);
@@ -499,7 +469,14 @@ static int hisi_uc_pmu_dev_probe(struct platform_device *pdev,
 	if (ret)
 		return ret;
 
-	uc_pmu->pmu_events.attr_groups = hisi_uc_pmu_attr_groups;
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
+						&hisi_uc_pmu_format_group;
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
+						&hisi_uc_pmu_events_group;
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_CPUMASK] =
+						&hisi_pmu_cpumask_attr_group;
+	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_IDENTIFIER] =
+						&hisi_pmu_identifier_group;
 	uc_pmu->check_event = HISI_UC_EVTYPE_MASK;
 	uc_pmu->ops = &hisi_uncore_uc_pmu_ops;
 	uc_pmu->counter_bits = HISI_UC_CNTR_REG_BITS;
-- 
2.24.0

Re: [PATCH 5/8] drivers/perf: hisi: Refactor the attributes creation

[Jonathan Cameron]

On Fri, 18 Oct 2024 17:57:42 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> Each type of HiSilicon Uncore PMU has the following sysfs attributes:
> 
> - format: bitmask in perf_event_attr::config[012] of corresponding
>   attribute
> - event: events name and corresponding event code
> - cpumask: range of CPUs the events can be opened on
> - identifier: the version of this PMU
> 
> Different types of PMU have different implementations of the "format"
> and "event" but all share the same implementation of the "cpumask"
> and "identifier". Thus we can move cpumask and identifier to the
> hisi_uncore_pmu framework and drivers can use the generic
> implementation.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

Is there a reason to move to code setting all 4 elements vs
just using the extern struct attribute_group in the static const arrays?

e.g.
static const struct attribute_group *hisi_uc_pmu_attr_groups[] = {
	&hisi_uc_pmu_format_group,
	&hisi_uc_pmu_events_group,
	&hisi_pmu_cpumask_attr_group,
	&hisi_pmu_identifier_group,
	NULL
};
mixing attributes defined in each module with those coming from the core module?

For the cases where it varies between versions of the PMU, just have a bunch
of such arrays to pick from.

I might be missing some subtle issue, but a really quick hack shows it builds
fine.

Jonathan


> ---
>  drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c  | 41 +++----------
>  drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c | 55 +++++-------------
>  drivers/perf/hisilicon/hisi_uncore_hha_pmu.c  | 57 ++++++-------------
>  drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c  | 55 +++++-------------
>  drivers/perf/hisilicon/hisi_uncore_pa_pmu.c   | 39 +++----------
>  drivers/perf/hisilicon/hisi_uncore_pmu.c      | 45 +++++++++++----
>  drivers/perf/hisilicon/hisi_uncore_pmu.h      | 14 ++++-
>  drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c | 41 +++----------
>  drivers/perf/hisilicon/hisi_uncore_uc_pmu.c   | 41 +++----------
>  9 files changed, 128 insertions(+), 260 deletions(-)
> 
> diff --git a/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
> index bd1c1799f935..4dd52eba9f27 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
> @@ -225,37 +225,6 @@ static const struct attribute_group hisi_cpa_pmu_events_group = {
>  	.attrs = hisi_cpa_pmu_events_attr,
>  };
>  
> -static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
> -
> -static struct attribute *hisi_cpa_pmu_cpumask_attrs[] = {
> -	&dev_attr_cpumask.attr,
> -	NULL
> -};
> -
> -static const struct attribute_group hisi_cpa_pmu_cpumask_attr_group = {
> -	.attrs = hisi_cpa_pmu_cpumask_attrs,
> -};
> -
> -static struct device_attribute hisi_cpa_pmu_identifier_attr =
> -	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
> -
> -static struct attribute *hisi_cpa_pmu_identifier_attrs[] = {
> -	&hisi_cpa_pmu_identifier_attr.attr,
> -	NULL
> -};
> -
> -static const struct attribute_group hisi_cpa_pmu_identifier_group = {
> -	.attrs = hisi_cpa_pmu_identifier_attrs,
> -};
> -
> -static const struct attribute_group *hisi_cpa_pmu_attr_groups[] = {
> -	&hisi_cpa_pmu_format_group,
> -	&hisi_cpa_pmu_events_group,
> -	&hisi_cpa_pmu_cpumask_attr_group,
> -	&hisi_cpa_pmu_identifier_group,
> -	NULL
> -};
> -
>  static const struct hisi_uncore_ops hisi_uncore_cpa_pmu_ops = {
>  	.write_evtype           = hisi_cpa_pmu_write_evtype,
>  	.get_event_idx		= hisi_uncore_pmu_get_event_idx,
> @@ -274,6 +243,7 @@ static const struct hisi_uncore_ops hisi_uncore_cpa_pmu_ops = {
>  static int hisi_cpa_pmu_dev_probe(struct platform_device *pdev,
>  				  struct hisi_pmu *cpa_pmu)
>  {
> +	struct hisi_pmu_hwevents *pmu_events = &cpa_pmu->pmu_events;
>  	int ret;
>  
>  	ret = hisi_cpa_pmu_init_data(pdev, cpa_pmu);
> @@ -286,7 +256,14 @@ static int hisi_cpa_pmu_dev_probe(struct platform_device *pdev,
>  
>  	cpa_pmu->counter_bits = CPA_COUNTER_BITS;
>  	cpa_pmu->check_event = CPA_NR_EVENTS;
> -	cpa_pmu->pmu_events.attr_groups = hisi_cpa_pmu_attr_groups;
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
> +						&hisi_cpa_pmu_format_group;
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
> +						&hisi_cpa_pmu_events_group;
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_CPUMASK] =
> +						&hisi_pmu_cpumask_attr_group;
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_IDENTIFIER] =
> +						&hisi_pmu_identifier_group;
>  	cpa_pmu->ops = &hisi_uncore_cpa_pmu_ops;
>  	cpa_pmu->num_counters = CPA_NR_COUNTERS;
>  	cpa_pmu->dev = &pdev->dev;
> diff --git a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
> index 415a95e24993..6d805ca4562f 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
> @@ -380,45 +380,6 @@ static const struct attribute_group hisi_ddrc_pmu_v2_events_group = {
>  	.attrs = hisi_ddrc_pmu_v2_events_attr,
>  };
>  
> -static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
> -
> -static struct attribute *hisi_ddrc_pmu_cpumask_attrs[] = {
> -	&dev_attr_cpumask.attr,
> -	NULL,
> -};
> -
> -static const struct attribute_group hisi_ddrc_pmu_cpumask_attr_group = {
> -	.attrs = hisi_ddrc_pmu_cpumask_attrs,
> -};
> -
> -static struct device_attribute hisi_ddrc_pmu_identifier_attr =
> -	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
> -
> -static struct attribute *hisi_ddrc_pmu_identifier_attrs[] = {
> -	&hisi_ddrc_pmu_identifier_attr.attr,
> -	NULL
> -};
> -
> -static const struct attribute_group hisi_ddrc_pmu_identifier_group = {
> -	.attrs = hisi_ddrc_pmu_identifier_attrs,
> -};
> -
> -static const struct attribute_group *hisi_ddrc_pmu_v1_attr_groups[] = {
> -	&hisi_ddrc_pmu_v1_format_group,
> -	&hisi_ddrc_pmu_v1_events_group,
> -	&hisi_ddrc_pmu_cpumask_attr_group,
> -	&hisi_ddrc_pmu_identifier_group,
> -	NULL,
> -};
> -
> -static const struct attribute_group *hisi_ddrc_pmu_v2_attr_groups[] = {
> -	&hisi_ddrc_pmu_v2_format_group,
> -	&hisi_ddrc_pmu_v2_events_group,
> -	&hisi_ddrc_pmu_cpumask_attr_group,
> -	&hisi_ddrc_pmu_identifier_group,
> -	NULL
> -};
> -
>  static const struct hisi_uncore_ops hisi_uncore_ddrc_v1_ops = {
>  	.write_evtype           = hisi_ddrc_pmu_write_evtype,
>  	.get_event_idx		= hisi_ddrc_pmu_v1_get_event_idx,
> @@ -452,6 +413,7 @@ static const struct hisi_uncore_ops hisi_uncore_ddrc_v2_ops = {
>  static int hisi_ddrc_pmu_dev_probe(struct platform_device *pdev,
>  				   struct hisi_pmu *ddrc_pmu)
>  {
> +	struct hisi_pmu_hwevents *pmu_events = &ddrc_pmu->pmu_events;
>  	int ret;
>  
>  	ret = hisi_ddrc_pmu_init_data(pdev, ddrc_pmu);
> @@ -465,15 +427,26 @@ static int hisi_ddrc_pmu_dev_probe(struct platform_device *pdev,
>  	if (ddrc_pmu->identifier >= HISI_PMU_V2) {
>  		ddrc_pmu->counter_bits = 48;
>  		ddrc_pmu->check_event = DDRC_V2_NR_EVENTS;
> -		ddrc_pmu->pmu_events.attr_groups = hisi_ddrc_pmu_v2_attr_groups;
> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
> +						&hisi_ddrc_pmu_v2_format_group;
> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
> +						&hisi_ddrc_pmu_v2_events_group;
>  		ddrc_pmu->ops = &hisi_uncore_ddrc_v2_ops;
>  	} else {
>  		ddrc_pmu->counter_bits = 32;
>  		ddrc_pmu->check_event = DDRC_V1_NR_EVENTS;
> -		ddrc_pmu->pmu_events.attr_groups = hisi_ddrc_pmu_v1_attr_groups;
> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
> +						&hisi_ddrc_pmu_v1_format_group;
> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
> +						&hisi_ddrc_pmu_v1_events_group;
>  		ddrc_pmu->ops = &hisi_uncore_ddrc_v1_ops;
>  	}
>  
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_CPUMASK] =
> +						&hisi_pmu_cpumask_attr_group;
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_IDENTIFIER] =
> +						&hisi_pmu_identifier_group;
> +
>  	ddrc_pmu->num_counters = DDRC_NR_COUNTERS;
>  	ddrc_pmu->dev = &pdev->dev;
>  	ddrc_pmu->on_cpu = -1;
> diff --git a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> index 43554e4f8a36..07cab6cf4897 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
> @@ -405,45 +405,6 @@ static const struct attribute_group hisi_hha_pmu_v2_events_group = {
>  	.attrs = hisi_hha_pmu_v2_events_attr,
>  };
>  
> -static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
> -
> -static struct attribute *hisi_hha_pmu_cpumask_attrs[] = {
> -	&dev_attr_cpumask.attr,
> -	NULL,
> -};
> -
> -static const struct attribute_group hisi_hha_pmu_cpumask_attr_group = {
> -	.attrs = hisi_hha_pmu_cpumask_attrs,
> -};
> -
> -static struct device_attribute hisi_hha_pmu_identifier_attr =
> -	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
> -
> -static struct attribute *hisi_hha_pmu_identifier_attrs[] = {
> -	&hisi_hha_pmu_identifier_attr.attr,
> -	NULL
> -};
> -
> -static const struct attribute_group hisi_hha_pmu_identifier_group = {
> -	.attrs = hisi_hha_pmu_identifier_attrs,
> -};
> -
> -static const struct attribute_group *hisi_hha_pmu_v1_attr_groups[] = {
> -	&hisi_hha_pmu_v1_format_group,
> -	&hisi_hha_pmu_v1_events_group,
> -	&hisi_hha_pmu_cpumask_attr_group,
> -	&hisi_hha_pmu_identifier_group,
> -	NULL,
> -};
> -
> -static const struct attribute_group *hisi_hha_pmu_v2_attr_groups[] = {
> -	&hisi_hha_pmu_v2_format_group,
> -	&hisi_hha_pmu_v2_events_group,
> -	&hisi_hha_pmu_cpumask_attr_group,
> -	&hisi_hha_pmu_identifier_group,
> -	NULL
> -};
> -
>  static const struct hisi_uncore_ops hisi_uncore_hha_ops = {
>  	.write_evtype		= hisi_hha_pmu_write_evtype,
>  	.get_event_idx		= hisi_uncore_pmu_get_event_idx,
> @@ -464,6 +425,7 @@ static const struct hisi_uncore_ops hisi_uncore_hha_ops = {
>  static int hisi_hha_pmu_dev_probe(struct platform_device *pdev,
>  				  struct hisi_pmu *hha_pmu)
>  {
> +	struct hisi_pmu_hwevents *pmu_events = &hha_pmu->pmu_events;
>  	int ret;
>  
>  	ret = hisi_hha_pmu_init_data(pdev, hha_pmu);
> @@ -477,14 +439,27 @@ static int hisi_hha_pmu_dev_probe(struct platform_device *pdev,
>  	if (hha_pmu->identifier >= HISI_PMU_V2) {
>  		hha_pmu->counter_bits = 64;
>  		hha_pmu->check_event = HHA_V2_NR_EVENT;
> -		hha_pmu->pmu_events.attr_groups = hisi_hha_pmu_v2_attr_groups;
> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
> +						&hisi_hha_pmu_v2_format_group;
> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
> +						&hisi_hha_pmu_v2_events_group;
>  		hha_pmu->num_counters = HHA_V2_NR_COUNTERS;
>  	} else {
>  		hha_pmu->counter_bits = 48;
>  		hha_pmu->check_event = HHA_V1_NR_EVENT;
> -		hha_pmu->pmu_events.attr_groups = hisi_hha_pmu_v1_attr_groups;
> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
> +						&hisi_hha_pmu_v1_format_group;
> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
> +						&hisi_hha_pmu_v1_events_group;
>  		hha_pmu->num_counters = HHA_V1_NR_COUNTERS;
>  	}
> +
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_CPUMASK] =
> +						&hisi_pmu_cpumask_attr_group;
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_IDENTIFIER] =
> +						&hisi_pmu_identifier_group;
> +
> +
>  	hha_pmu->ops = &hisi_uncore_hha_ops;
>  	hha_pmu->dev = &pdev->dev;
>  	hha_pmu->on_cpu = -1;
> diff --git a/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
> index b113620d27f9..6f540ab1f451 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
> @@ -441,45 +441,6 @@ static const struct attribute_group hisi_l3c_pmu_v2_events_group = {
>  	.attrs = hisi_l3c_pmu_v2_events_attr,
>  };
>  
> -static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
> -
> -static struct attribute *hisi_l3c_pmu_cpumask_attrs[] = {
> -	&dev_attr_cpumask.attr,
> -	NULL,
> -};
> -
> -static const struct attribute_group hisi_l3c_pmu_cpumask_attr_group = {
> -	.attrs = hisi_l3c_pmu_cpumask_attrs,
> -};
> -
> -static struct device_attribute hisi_l3c_pmu_identifier_attr =
> -	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
> -
> -static struct attribute *hisi_l3c_pmu_identifier_attrs[] = {
> -	&hisi_l3c_pmu_identifier_attr.attr,
> -	NULL
> -};
> -
> -static const struct attribute_group hisi_l3c_pmu_identifier_group = {
> -	.attrs = hisi_l3c_pmu_identifier_attrs,
> -};
> -
> -static const struct attribute_group *hisi_l3c_pmu_v1_attr_groups[] = {
> -	&hisi_l3c_pmu_v1_format_group,
> -	&hisi_l3c_pmu_v1_events_group,
> -	&hisi_l3c_pmu_cpumask_attr_group,
> -	&hisi_l3c_pmu_identifier_group,
> -	NULL,
> -};
> -
> -static const struct attribute_group *hisi_l3c_pmu_v2_attr_groups[] = {
> -	&hisi_l3c_pmu_v2_format_group,
> -	&hisi_l3c_pmu_v2_events_group,
> -	&hisi_l3c_pmu_cpumask_attr_group,
> -	&hisi_l3c_pmu_identifier_group,
> -	NULL
> -};
> -
>  static const struct hisi_uncore_ops hisi_uncore_l3c_ops = {
>  	.write_evtype		= hisi_l3c_pmu_write_evtype,
>  	.get_event_idx		= hisi_uncore_pmu_get_event_idx,
> @@ -500,6 +461,7 @@ static const struct hisi_uncore_ops hisi_uncore_l3c_ops = {
>  static int hisi_l3c_pmu_dev_probe(struct platform_device *pdev,
>  				  struct hisi_pmu *l3c_pmu)
>  {
> +	struct hisi_pmu_hwevents *pmu_events = &l3c_pmu->pmu_events;
>  	int ret;
>  
>  	ret = hisi_l3c_pmu_init_data(pdev, l3c_pmu);
> @@ -513,13 +475,24 @@ static int hisi_l3c_pmu_dev_probe(struct platform_device *pdev,
>  	if (l3c_pmu->identifier >= HISI_PMU_V2) {
>  		l3c_pmu->counter_bits = 64;
>  		l3c_pmu->check_event = L3C_V2_NR_EVENTS;
> -		l3c_pmu->pmu_events.attr_groups = hisi_l3c_pmu_v2_attr_groups;
> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
> +						 &hisi_l3c_pmu_v2_format_group;
> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
> +						 &hisi_l3c_pmu_v2_events_group;
>  	} else {
>  		l3c_pmu->counter_bits = 48;
>  		l3c_pmu->check_event = L3C_V1_NR_EVENTS;
> -		l3c_pmu->pmu_events.attr_groups = hisi_l3c_pmu_v1_attr_groups;
> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
> +						&hisi_l3c_pmu_v1_format_group;
> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
> +						&hisi_l3c_pmu_v1_events_group;
>  	}
>  
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_CPUMASK] =
> +						&hisi_pmu_cpumask_attr_group;
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_IDENTIFIER] =
> +						&hisi_pmu_identifier_group;
> +
>  	l3c_pmu->num_counters = L3C_NR_COUNTERS;
>  	l3c_pmu->ops = &hisi_uncore_l3c_ops;
>  	l3c_pmu->dev = &pdev->dev;
> diff --git a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
> index dbe4d7b97b2b..69931e73a3cd 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
> @@ -353,29 +353,6 @@ static const struct attribute_group hisi_h60pa_pmu_events_group = {
>  	.attrs = hisi_h60pa_pmu_events_attr,
>  };
>  
> -static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
> -
> -static struct attribute *hisi_pa_pmu_cpumask_attrs[] = {
> -	&dev_attr_cpumask.attr,
> -	NULL
> -};
> -
> -static const struct attribute_group hisi_pa_pmu_cpumask_attr_group = {
> -	.attrs = hisi_pa_pmu_cpumask_attrs,
> -};
> -
> -static struct device_attribute hisi_pa_pmu_identifier_attr =
> -	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
> -
> -static struct attribute *hisi_pa_pmu_identifier_attrs[] = {
> -	&hisi_pa_pmu_identifier_attr.attr,
> -	NULL
> -};
> -
> -static const struct attribute_group hisi_pa_pmu_identifier_group = {
> -	.attrs = hisi_pa_pmu_identifier_attrs,
> -};
> -
>  static struct hisi_pa_pmu_int_regs hisi_pa_pmu_regs = {
>  	.mask_offset = PA_INT_MASK,
>  	.clear_offset = PA_INT_CLEAR,
> @@ -385,8 +362,6 @@ static struct hisi_pa_pmu_int_regs hisi_pa_pmu_regs = {
>  static const struct attribute_group *hisi_pa_pmu_v2_attr_groups[] = {
>  	&hisi_pa_pmu_v2_format_group,
>  	&hisi_pa_pmu_v2_events_group,
> -	&hisi_pa_pmu_cpumask_attr_group,
> -	&hisi_pa_pmu_identifier_group,
>  	NULL
>  };
>  
> @@ -399,8 +374,6 @@ static const struct hisi_pmu_dev_info hisi_h32pa_v2 = {
>  static const struct attribute_group *hisi_pa_pmu_v3_attr_groups[] = {
>  	&hisi_pa_pmu_v2_format_group,
>  	&hisi_pa_pmu_v3_events_group,
> -	&hisi_pa_pmu_cpumask_attr_group,
> -	&hisi_pa_pmu_identifier_group,
>  	NULL
>  };
>  
> @@ -419,8 +392,6 @@ static struct hisi_pa_pmu_int_regs hisi_h60pa_pmu_regs = {
>  static const struct attribute_group *hisi_h60pa_pmu_attr_groups[] = {
>  	&hisi_pa_pmu_v2_format_group,
>  	&hisi_h60pa_pmu_events_group,
> -	&hisi_pa_pmu_cpumask_attr_group,
> -	&hisi_pa_pmu_identifier_group,
>  	NULL
>  };
>  
> @@ -450,6 +421,7 @@ static const struct hisi_uncore_ops hisi_uncore_pa_ops = {
>  static int hisi_pa_pmu_dev_probe(struct platform_device *pdev,
>  				 struct hisi_pmu *pa_pmu)
>  {
> +	struct hisi_pmu_hwevents *pmu_events = &pa_pmu->pmu_events;
>  	int ret;
>  
>  	ret = hisi_pa_pmu_init_data(pdev, pa_pmu);
> @@ -460,7 +432,14 @@ static int hisi_pa_pmu_dev_probe(struct platform_device *pdev,
>  	if (ret)
>  		return ret;
>  
> -	pa_pmu->pmu_events.attr_groups = pa_pmu->dev_info->attr_groups;
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
> +		pa_pmu->dev_info->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT];
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
> +		pa_pmu->dev_info->attr_groups[HISI_PMU_ATTR_GROUP_EVENT];
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_CPUMASK] =
> +						&hisi_pmu_cpumask_attr_group;
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_IDENTIFIER] =
> +						&hisi_pmu_identifier_group;
>  	pa_pmu->num_counters = PA_NR_COUNTERS;
>  	pa_pmu->ops = &hisi_uncore_pa_ops;
>  	pa_pmu->check_event = 0xB0;
> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
> index e1756e639784..648d0e5e08ed 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
> @@ -49,6 +49,41 @@ ssize_t hisi_cpumask_sysfs_show(struct device *dev,
>  }
>  EXPORT_SYMBOL_NS_GPL(hisi_cpumask_sysfs_show, HISI_PMU);
>  
> +static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
> +
> +static struct attribute *hisi_pmu_cpumask_attrs[] = {
> +	&dev_attr_cpumask.attr,
> +	NULL
> +};
> +
> +const struct attribute_group hisi_pmu_cpumask_attr_group = {
> +	.attrs = hisi_pmu_cpumask_attrs,
> +};
> +EXPORT_SYMBOL_NS_GPL(hisi_pmu_cpumask_attr_group, HISI_PMU);
> +
> +ssize_t hisi_uncore_pmu_identifier_attr_show(struct device *dev,
> +					     struct device_attribute *attr,
> +					     char *page)
> +{
> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(dev_get_drvdata(dev));
> +
> +	return sysfs_emit(page, "0x%08x\n", hisi_pmu->identifier);
> +}
> +EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_identifier_attr_show, HISI_PMU);
> +
> +static struct device_attribute hisi_pmu_identifier_attr =
> +	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
> +
> +static struct attribute *hisi_pmu_identifier_attrs[] = {
> +	&hisi_pmu_identifier_attr.attr,
> +	NULL
> +};
> +
> +const struct attribute_group hisi_pmu_identifier_group = {
> +	.attrs = hisi_pmu_identifier_attrs,
> +};
> +EXPORT_SYMBOL_NS_GPL(hisi_pmu_identifier_group, HISI_PMU);
> +
>  static bool hisi_validate_event_group(struct perf_event *event)
>  {
>  	struct perf_event *sibling, *leader = event->group_leader;
> @@ -99,16 +134,6 @@ int hisi_uncore_pmu_get_event_idx(struct perf_event *event)
>  }
>  EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_get_event_idx, HISI_PMU);
>  
> -ssize_t hisi_uncore_pmu_identifier_attr_show(struct device *dev,
> -					     struct device_attribute *attr,
> -					     char *page)
> -{
> -	struct hisi_pmu *hisi_pmu = to_hisi_pmu(dev_get_drvdata(dev));
> -
> -	return sysfs_emit(page, "0x%08x\n", hisi_pmu->identifier);
> -}
> -EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_identifier_attr_show, HISI_PMU);
> -
>  static void hisi_uncore_pmu_clear_event_idx(struct hisi_pmu *hisi_pmu, int idx)
>  {
>  	clear_bit(idx, hisi_pmu->pmu_events.used_mask);
> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h
> index 239c45d847b3..ac2be8c337b7 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.h
> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
> @@ -77,10 +77,22 @@ struct hisi_pmu_dev_info {
>  	void *private;
>  };
>  
> +enum hisi_pmu_attr_groups {
> +	HISI_PMU_ATTR_GROUP_FORMAT,
> +	HISI_PMU_ATTR_GROUP_EVENT,
> +	HISI_PMU_ATTR_GROUP_CPUMASK,
> +	HISI_PMU_ATTR_GROUP_IDENTIFIER,
> +	HISI_PMU_ATTR_GROUP_NR
> +};
> +
> +/* Generic implementation of cpumask/identifier group */
> +extern const struct attribute_group hisi_pmu_cpumask_attr_group;
> +extern const struct attribute_group hisi_pmu_identifier_group;
> +
>  struct hisi_pmu_hwevents {
>  	struct perf_event *hw_events[HISI_MAX_COUNTERS];
>  	DECLARE_BITMAP(used_mask, HISI_MAX_COUNTERS);
> -	const struct attribute_group **attr_groups;
> +	const struct attribute_group *attr_groups[HISI_PMU_ATTR_GROUP_NR + 1];
>  };
>  
>  /**
> diff --git a/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c
> index 43cbdf0fb7c7..676a7078f2ef 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c
> @@ -344,37 +344,6 @@ static const struct attribute_group hisi_sllc_pmu_v2_events_group = {
>  	.attrs = hisi_sllc_pmu_v2_events_attr,
>  };
>  
> -static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
> -
> -static struct attribute *hisi_sllc_pmu_cpumask_attrs[] = {
> -	&dev_attr_cpumask.attr,
> -	NULL
> -};
> -
> -static const struct attribute_group hisi_sllc_pmu_cpumask_attr_group = {
> -	.attrs = hisi_sllc_pmu_cpumask_attrs,
> -};
> -
> -static struct device_attribute hisi_sllc_pmu_identifier_attr =
> -	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
> -
> -static struct attribute *hisi_sllc_pmu_identifier_attrs[] = {
> -	&hisi_sllc_pmu_identifier_attr.attr,
> -	NULL
> -};
> -
> -static const struct attribute_group hisi_sllc_pmu_identifier_group = {
> -	.attrs = hisi_sllc_pmu_identifier_attrs,
> -};
> -
> -static const struct attribute_group *hisi_sllc_pmu_v2_attr_groups[] = {
> -	&hisi_sllc_pmu_v2_format_group,
> -	&hisi_sllc_pmu_v2_events_group,
> -	&hisi_sllc_pmu_cpumask_attr_group,
> -	&hisi_sllc_pmu_identifier_group,
> -	NULL
> -};
> -
>  static const struct hisi_uncore_ops hisi_uncore_sllc_ops = {
>  	.write_evtype		= hisi_sllc_pmu_write_evtype,
>  	.get_event_idx		= hisi_uncore_pmu_get_event_idx,
> @@ -395,6 +364,7 @@ static const struct hisi_uncore_ops hisi_uncore_sllc_ops = {
>  static int hisi_sllc_pmu_dev_probe(struct platform_device *pdev,
>  				   struct hisi_pmu *sllc_pmu)
>  {
> +	struct hisi_pmu_hwevents *pmu_events = &sllc_pmu->pmu_events;
>  	int ret;
>  
>  	ret = hisi_sllc_pmu_init_data(pdev, sllc_pmu);
> @@ -405,7 +375,14 @@ static int hisi_sllc_pmu_dev_probe(struct platform_device *pdev,
>  	if (ret)
>  		return ret;
>  
> -	sllc_pmu->pmu_events.attr_groups = hisi_sllc_pmu_v2_attr_groups;
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
> +						&hisi_sllc_pmu_v2_format_group;
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
> +						&hisi_sllc_pmu_v2_events_group;
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_CPUMASK] =
> +						&hisi_pmu_cpumask_attr_group;
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_IDENTIFIER] =
> +						&hisi_pmu_identifier_group;
>  	sllc_pmu->ops = &hisi_uncore_sllc_ops;
>  	sllc_pmu->check_event = SLLC_NR_EVENTS;
>  	sllc_pmu->counter_bits = 64;
> diff --git a/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
> index 2040f6a8871e..c2ae852f6b19 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
> @@ -437,37 +437,6 @@ static const struct attribute_group hisi_uc_pmu_events_group = {
>  	.attrs = hisi_uc_pmu_events_attr,
>  };
>  
> -static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
> -
> -static struct attribute *hisi_uc_pmu_cpumask_attrs[] = {
> -	&dev_attr_cpumask.attr,
> -	NULL,
> -};
> -
> -static const struct attribute_group hisi_uc_pmu_cpumask_attr_group = {
> -	.attrs = hisi_uc_pmu_cpumask_attrs,
> -};
> -
> -static struct device_attribute hisi_uc_pmu_identifier_attr =
> -	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
> -
> -static struct attribute *hisi_uc_pmu_identifier_attrs[] = {
> -	&hisi_uc_pmu_identifier_attr.attr,
> -	NULL
> -};
> -
> -static const struct attribute_group hisi_uc_pmu_identifier_group = {
> -	.attrs = hisi_uc_pmu_identifier_attrs,
> -};
> -
> -static const struct attribute_group *hisi_uc_pmu_attr_groups[] = {
> -	&hisi_uc_pmu_format_group,
> -	&hisi_uc_pmu_events_group,
> -	&hisi_uc_pmu_cpumask_attr_group,
> -	&hisi_uc_pmu_identifier_group,
> -	NULL
> -};
> -
>  static const struct hisi_uncore_ops hisi_uncore_uc_pmu_ops = {
>  	.check_filter		= hisi_uc_pmu_check_filter,
>  	.write_evtype		= hisi_uc_pmu_write_evtype,
> @@ -489,6 +458,7 @@ static const struct hisi_uncore_ops hisi_uncore_uc_pmu_ops = {
>  static int hisi_uc_pmu_dev_probe(struct platform_device *pdev,
>  				 struct hisi_pmu *uc_pmu)
>  {
> +	struct hisi_pmu_hwevents *pmu_events = &uc_pmu->pmu_events;
>  	int ret;
>  
>  	ret = hisi_uc_pmu_init_data(pdev, uc_pmu);
> @@ -499,7 +469,14 @@ static int hisi_uc_pmu_dev_probe(struct platform_device *pdev,
>  	if (ret)
>  		return ret;
>  
> -	uc_pmu->pmu_events.attr_groups = hisi_uc_pmu_attr_groups;
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
> +						&hisi_uc_pmu_format_group;
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
> +						&hisi_uc_pmu_events_group;
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_CPUMASK] =
> +						&hisi_pmu_cpumask_attr_group;
> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_IDENTIFIER] =
> +						&hisi_pmu_identifier_group;
>  	uc_pmu->check_event = HISI_UC_EVTYPE_MASK;
>  	uc_pmu->ops = &hisi_uncore_uc_pmu_ops;
>  	uc_pmu->counter_bits = HISI_UC_CNTR_REG_BITS;

Re: [PATCH 5/8] drivers/perf: hisi: Refactor the attributes creation

[Yicong Yang]

On 2024/10/18 21:47, Jonathan Cameron wrote:
> On Fri, 18 Oct 2024 17:57:42 +0800
> Yicong Yang <yangyicong@huawei.com> wrote:
> 
>> From: Yicong Yang <yangyicong@hisilicon.com>
>>
>> Each type of HiSilicon Uncore PMU has the following sysfs attributes:
>>
>> - format: bitmask in perf_event_attr::config[012] of corresponding
>>   attribute
>> - event: events name and corresponding event code
>> - cpumask: range of CPUs the events can be opened on
>> - identifier: the version of this PMU
>>
>> Different types of PMU have different implementations of the "format"
>> and "event" but all share the same implementation of the "cpumask"
>> and "identifier". Thus we can move cpumask and identifier to the
>> hisi_uncore_pmu framework and drivers can use the generic
>> implementation.
>>
>> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
> 
> Is there a reason to move to code setting all 4 elements vs
> just using the extern struct attribute_group in the static const arrays?
> 
> e.g.
> static const struct attribute_group *hisi_uc_pmu_attr_groups[] = {
> 	&hisi_uc_pmu_format_group,
> 	&hisi_uc_pmu_events_group,
> 	&hisi_pmu_cpumask_attr_group,
> 	&hisi_pmu_identifier_group,
> 	NULL
> };
> mixing attributes defined in each module with those coming from the core module?
> 
> For the cases where it varies between versions of the PMU, just have a bunch
> of such arrays to pick from.

ah I got the suggestion here. then "enum hisi_pmu_attr_groups" maybe unnecessary
and could be dropped. Only the generic groups like "cpumask" and "identifier"
are centralized and others keep the same. Will try.

Thanks.

> 
> I might be missing some subtle issue, but a really quick hack shows it builds
> fine.
> 
> Jonathan
> 
> 
>> ---
>>  drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c  | 41 +++----------
>>  drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c | 55 +++++-------------
>>  drivers/perf/hisilicon/hisi_uncore_hha_pmu.c  | 57 ++++++-------------
>>  drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c  | 55 +++++-------------
>>  drivers/perf/hisilicon/hisi_uncore_pa_pmu.c   | 39 +++----------
>>  drivers/perf/hisilicon/hisi_uncore_pmu.c      | 45 +++++++++++----
>>  drivers/perf/hisilicon/hisi_uncore_pmu.h      | 14 ++++-
>>  drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c | 41 +++----------
>>  drivers/perf/hisilicon/hisi_uncore_uc_pmu.c   | 41 +++----------
>>  9 files changed, 128 insertions(+), 260 deletions(-)
>>
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
>> index bd1c1799f935..4dd52eba9f27 100644
>> --- a/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
>> +++ b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
>> @@ -225,37 +225,6 @@ static const struct attribute_group hisi_cpa_pmu_events_group = {
>>  	.attrs = hisi_cpa_pmu_events_attr,
>>  };
>>  
>> -static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
>> -
>> -static struct attribute *hisi_cpa_pmu_cpumask_attrs[] = {
>> -	&dev_attr_cpumask.attr,
>> -	NULL
>> -};
>> -
>> -static const struct attribute_group hisi_cpa_pmu_cpumask_attr_group = {
>> -	.attrs = hisi_cpa_pmu_cpumask_attrs,
>> -};
>> -
>> -static struct device_attribute hisi_cpa_pmu_identifier_attr =
>> -	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
>> -
>> -static struct attribute *hisi_cpa_pmu_identifier_attrs[] = {
>> -	&hisi_cpa_pmu_identifier_attr.attr,
>> -	NULL
>> -};
>> -
>> -static const struct attribute_group hisi_cpa_pmu_identifier_group = {
>> -	.attrs = hisi_cpa_pmu_identifier_attrs,
>> -};
>> -
>> -static const struct attribute_group *hisi_cpa_pmu_attr_groups[] = {
>> -	&hisi_cpa_pmu_format_group,
>> -	&hisi_cpa_pmu_events_group,
>> -	&hisi_cpa_pmu_cpumask_attr_group,
>> -	&hisi_cpa_pmu_identifier_group,
>> -	NULL
>> -};
>> -
>>  static const struct hisi_uncore_ops hisi_uncore_cpa_pmu_ops = {
>>  	.write_evtype           = hisi_cpa_pmu_write_evtype,
>>  	.get_event_idx		= hisi_uncore_pmu_get_event_idx,
>> @@ -274,6 +243,7 @@ static const struct hisi_uncore_ops hisi_uncore_cpa_pmu_ops = {
>>  static int hisi_cpa_pmu_dev_probe(struct platform_device *pdev,
>>  				  struct hisi_pmu *cpa_pmu)
>>  {
>> +	struct hisi_pmu_hwevents *pmu_events = &cpa_pmu->pmu_events;
>>  	int ret;
>>  
>>  	ret = hisi_cpa_pmu_init_data(pdev, cpa_pmu);
>> @@ -286,7 +256,14 @@ static int hisi_cpa_pmu_dev_probe(struct platform_device *pdev,
>>  
>>  	cpa_pmu->counter_bits = CPA_COUNTER_BITS;
>>  	cpa_pmu->check_event = CPA_NR_EVENTS;
>> -	cpa_pmu->pmu_events.attr_groups = hisi_cpa_pmu_attr_groups;
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
>> +						&hisi_cpa_pmu_format_group;
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
>> +						&hisi_cpa_pmu_events_group;
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_CPUMASK] =
>> +						&hisi_pmu_cpumask_attr_group;
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_IDENTIFIER] =
>> +						&hisi_pmu_identifier_group;
>>  	cpa_pmu->ops = &hisi_uncore_cpa_pmu_ops;
>>  	cpa_pmu->num_counters = CPA_NR_COUNTERS;
>>  	cpa_pmu->dev = &pdev->dev;
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
>> index 415a95e24993..6d805ca4562f 100644
>> --- a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
>> +++ b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
>> @@ -380,45 +380,6 @@ static const struct attribute_group hisi_ddrc_pmu_v2_events_group = {
>>  	.attrs = hisi_ddrc_pmu_v2_events_attr,
>>  };
>>  
>> -static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
>> -
>> -static struct attribute *hisi_ddrc_pmu_cpumask_attrs[] = {
>> -	&dev_attr_cpumask.attr,
>> -	NULL,
>> -};
>> -
>> -static const struct attribute_group hisi_ddrc_pmu_cpumask_attr_group = {
>> -	.attrs = hisi_ddrc_pmu_cpumask_attrs,
>> -};
>> -
>> -static struct device_attribute hisi_ddrc_pmu_identifier_attr =
>> -	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
>> -
>> -static struct attribute *hisi_ddrc_pmu_identifier_attrs[] = {
>> -	&hisi_ddrc_pmu_identifier_attr.attr,
>> -	NULL
>> -};
>> -
>> -static const struct attribute_group hisi_ddrc_pmu_identifier_group = {
>> -	.attrs = hisi_ddrc_pmu_identifier_attrs,
>> -};
>> -
>> -static const struct attribute_group *hisi_ddrc_pmu_v1_attr_groups[] = {
>> -	&hisi_ddrc_pmu_v1_format_group,
>> -	&hisi_ddrc_pmu_v1_events_group,
>> -	&hisi_ddrc_pmu_cpumask_attr_group,
>> -	&hisi_ddrc_pmu_identifier_group,
>> -	NULL,
>> -};
>> -
>> -static const struct attribute_group *hisi_ddrc_pmu_v2_attr_groups[] = {
>> -	&hisi_ddrc_pmu_v2_format_group,
>> -	&hisi_ddrc_pmu_v2_events_group,
>> -	&hisi_ddrc_pmu_cpumask_attr_group,
>> -	&hisi_ddrc_pmu_identifier_group,
>> -	NULL
>> -};
>> -
>>  static const struct hisi_uncore_ops hisi_uncore_ddrc_v1_ops = {
>>  	.write_evtype           = hisi_ddrc_pmu_write_evtype,
>>  	.get_event_idx		= hisi_ddrc_pmu_v1_get_event_idx,
>> @@ -452,6 +413,7 @@ static const struct hisi_uncore_ops hisi_uncore_ddrc_v2_ops = {
>>  static int hisi_ddrc_pmu_dev_probe(struct platform_device *pdev,
>>  				   struct hisi_pmu *ddrc_pmu)
>>  {
>> +	struct hisi_pmu_hwevents *pmu_events = &ddrc_pmu->pmu_events;
>>  	int ret;
>>  
>>  	ret = hisi_ddrc_pmu_init_data(pdev, ddrc_pmu);
>> @@ -465,15 +427,26 @@ static int hisi_ddrc_pmu_dev_probe(struct platform_device *pdev,
>>  	if (ddrc_pmu->identifier >= HISI_PMU_V2) {
>>  		ddrc_pmu->counter_bits = 48;
>>  		ddrc_pmu->check_event = DDRC_V2_NR_EVENTS;
>> -		ddrc_pmu->pmu_events.attr_groups = hisi_ddrc_pmu_v2_attr_groups;
>> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
>> +						&hisi_ddrc_pmu_v2_format_group;
>> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
>> +						&hisi_ddrc_pmu_v2_events_group;
>>  		ddrc_pmu->ops = &hisi_uncore_ddrc_v2_ops;
>>  	} else {
>>  		ddrc_pmu->counter_bits = 32;
>>  		ddrc_pmu->check_event = DDRC_V1_NR_EVENTS;
>> -		ddrc_pmu->pmu_events.attr_groups = hisi_ddrc_pmu_v1_attr_groups;
>> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
>> +						&hisi_ddrc_pmu_v1_format_group;
>> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
>> +						&hisi_ddrc_pmu_v1_events_group;
>>  		ddrc_pmu->ops = &hisi_uncore_ddrc_v1_ops;
>>  	}
>>  
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_CPUMASK] =
>> +						&hisi_pmu_cpumask_attr_group;
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_IDENTIFIER] =
>> +						&hisi_pmu_identifier_group;
>> +
>>  	ddrc_pmu->num_counters = DDRC_NR_COUNTERS;
>>  	ddrc_pmu->dev = &pdev->dev;
>>  	ddrc_pmu->on_cpu = -1;
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
>> index 43554e4f8a36..07cab6cf4897 100644
>> --- a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
>> +++ b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
>> @@ -405,45 +405,6 @@ static const struct attribute_group hisi_hha_pmu_v2_events_group = {
>>  	.attrs = hisi_hha_pmu_v2_events_attr,
>>  };
>>  
>> -static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
>> -
>> -static struct attribute *hisi_hha_pmu_cpumask_attrs[] = {
>> -	&dev_attr_cpumask.attr,
>> -	NULL,
>> -};
>> -
>> -static const struct attribute_group hisi_hha_pmu_cpumask_attr_group = {
>> -	.attrs = hisi_hha_pmu_cpumask_attrs,
>> -};
>> -
>> -static struct device_attribute hisi_hha_pmu_identifier_attr =
>> -	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
>> -
>> -static struct attribute *hisi_hha_pmu_identifier_attrs[] = {
>> -	&hisi_hha_pmu_identifier_attr.attr,
>> -	NULL
>> -};
>> -
>> -static const struct attribute_group hisi_hha_pmu_identifier_group = {
>> -	.attrs = hisi_hha_pmu_identifier_attrs,
>> -};
>> -
>> -static const struct attribute_group *hisi_hha_pmu_v1_attr_groups[] = {
>> -	&hisi_hha_pmu_v1_format_group,
>> -	&hisi_hha_pmu_v1_events_group,
>> -	&hisi_hha_pmu_cpumask_attr_group,
>> -	&hisi_hha_pmu_identifier_group,
>> -	NULL,
>> -};
>> -
>> -static const struct attribute_group *hisi_hha_pmu_v2_attr_groups[] = {
>> -	&hisi_hha_pmu_v2_format_group,
>> -	&hisi_hha_pmu_v2_events_group,
>> -	&hisi_hha_pmu_cpumask_attr_group,
>> -	&hisi_hha_pmu_identifier_group,
>> -	NULL
>> -};
>> -
>>  static const struct hisi_uncore_ops hisi_uncore_hha_ops = {
>>  	.write_evtype		= hisi_hha_pmu_write_evtype,
>>  	.get_event_idx		= hisi_uncore_pmu_get_event_idx,
>> @@ -464,6 +425,7 @@ static const struct hisi_uncore_ops hisi_uncore_hha_ops = {
>>  static int hisi_hha_pmu_dev_probe(struct platform_device *pdev,
>>  				  struct hisi_pmu *hha_pmu)
>>  {
>> +	struct hisi_pmu_hwevents *pmu_events = &hha_pmu->pmu_events;
>>  	int ret;
>>  
>>  	ret = hisi_hha_pmu_init_data(pdev, hha_pmu);
>> @@ -477,14 +439,27 @@ static int hisi_hha_pmu_dev_probe(struct platform_device *pdev,
>>  	if (hha_pmu->identifier >= HISI_PMU_V2) {
>>  		hha_pmu->counter_bits = 64;
>>  		hha_pmu->check_event = HHA_V2_NR_EVENT;
>> -		hha_pmu->pmu_events.attr_groups = hisi_hha_pmu_v2_attr_groups;
>> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
>> +						&hisi_hha_pmu_v2_format_group;
>> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
>> +						&hisi_hha_pmu_v2_events_group;
>>  		hha_pmu->num_counters = HHA_V2_NR_COUNTERS;
>>  	} else {
>>  		hha_pmu->counter_bits = 48;
>>  		hha_pmu->check_event = HHA_V1_NR_EVENT;
>> -		hha_pmu->pmu_events.attr_groups = hisi_hha_pmu_v1_attr_groups;
>> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
>> +						&hisi_hha_pmu_v1_format_group;
>> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
>> +						&hisi_hha_pmu_v1_events_group;
>>  		hha_pmu->num_counters = HHA_V1_NR_COUNTERS;
>>  	}
>> +
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_CPUMASK] =
>> +						&hisi_pmu_cpumask_attr_group;
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_IDENTIFIER] =
>> +						&hisi_pmu_identifier_group;
>> +
>> +
>>  	hha_pmu->ops = &hisi_uncore_hha_ops;
>>  	hha_pmu->dev = &pdev->dev;
>>  	hha_pmu->on_cpu = -1;
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
>> index b113620d27f9..6f540ab1f451 100644
>> --- a/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
>> +++ b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
>> @@ -441,45 +441,6 @@ static const struct attribute_group hisi_l3c_pmu_v2_events_group = {
>>  	.attrs = hisi_l3c_pmu_v2_events_attr,
>>  };
>>  
>> -static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
>> -
>> -static struct attribute *hisi_l3c_pmu_cpumask_attrs[] = {
>> -	&dev_attr_cpumask.attr,
>> -	NULL,
>> -};
>> -
>> -static const struct attribute_group hisi_l3c_pmu_cpumask_attr_group = {
>> -	.attrs = hisi_l3c_pmu_cpumask_attrs,
>> -};
>> -
>> -static struct device_attribute hisi_l3c_pmu_identifier_attr =
>> -	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
>> -
>> -static struct attribute *hisi_l3c_pmu_identifier_attrs[] = {
>> -	&hisi_l3c_pmu_identifier_attr.attr,
>> -	NULL
>> -};
>> -
>> -static const struct attribute_group hisi_l3c_pmu_identifier_group = {
>> -	.attrs = hisi_l3c_pmu_identifier_attrs,
>> -};
>> -
>> -static const struct attribute_group *hisi_l3c_pmu_v1_attr_groups[] = {
>> -	&hisi_l3c_pmu_v1_format_group,
>> -	&hisi_l3c_pmu_v1_events_group,
>> -	&hisi_l3c_pmu_cpumask_attr_group,
>> -	&hisi_l3c_pmu_identifier_group,
>> -	NULL,
>> -};
>> -
>> -static const struct attribute_group *hisi_l3c_pmu_v2_attr_groups[] = {
>> -	&hisi_l3c_pmu_v2_format_group,
>> -	&hisi_l3c_pmu_v2_events_group,
>> -	&hisi_l3c_pmu_cpumask_attr_group,
>> -	&hisi_l3c_pmu_identifier_group,
>> -	NULL
>> -};
>> -
>>  static const struct hisi_uncore_ops hisi_uncore_l3c_ops = {
>>  	.write_evtype		= hisi_l3c_pmu_write_evtype,
>>  	.get_event_idx		= hisi_uncore_pmu_get_event_idx,
>> @@ -500,6 +461,7 @@ static const struct hisi_uncore_ops hisi_uncore_l3c_ops = {
>>  static int hisi_l3c_pmu_dev_probe(struct platform_device *pdev,
>>  				  struct hisi_pmu *l3c_pmu)
>>  {
>> +	struct hisi_pmu_hwevents *pmu_events = &l3c_pmu->pmu_events;
>>  	int ret;
>>  
>>  	ret = hisi_l3c_pmu_init_data(pdev, l3c_pmu);
>> @@ -513,13 +475,24 @@ static int hisi_l3c_pmu_dev_probe(struct platform_device *pdev,
>>  	if (l3c_pmu->identifier >= HISI_PMU_V2) {
>>  		l3c_pmu->counter_bits = 64;
>>  		l3c_pmu->check_event = L3C_V2_NR_EVENTS;
>> -		l3c_pmu->pmu_events.attr_groups = hisi_l3c_pmu_v2_attr_groups;
>> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
>> +						 &hisi_l3c_pmu_v2_format_group;
>> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
>> +						 &hisi_l3c_pmu_v2_events_group;
>>  	} else {
>>  		l3c_pmu->counter_bits = 48;
>>  		l3c_pmu->check_event = L3C_V1_NR_EVENTS;
>> -		l3c_pmu->pmu_events.attr_groups = hisi_l3c_pmu_v1_attr_groups;
>> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
>> +						&hisi_l3c_pmu_v1_format_group;
>> +		pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
>> +						&hisi_l3c_pmu_v1_events_group;
>>  	}
>>  
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_CPUMASK] =
>> +						&hisi_pmu_cpumask_attr_group;
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_IDENTIFIER] =
>> +						&hisi_pmu_identifier_group;
>> +
>>  	l3c_pmu->num_counters = L3C_NR_COUNTERS;
>>  	l3c_pmu->ops = &hisi_uncore_l3c_ops;
>>  	l3c_pmu->dev = &pdev->dev;
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
>> index dbe4d7b97b2b..69931e73a3cd 100644
>> --- a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
>> +++ b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
>> @@ -353,29 +353,6 @@ static const struct attribute_group hisi_h60pa_pmu_events_group = {
>>  	.attrs = hisi_h60pa_pmu_events_attr,
>>  };
>>  
>> -static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
>> -
>> -static struct attribute *hisi_pa_pmu_cpumask_attrs[] = {
>> -	&dev_attr_cpumask.attr,
>> -	NULL
>> -};
>> -
>> -static const struct attribute_group hisi_pa_pmu_cpumask_attr_group = {
>> -	.attrs = hisi_pa_pmu_cpumask_attrs,
>> -};
>> -
>> -static struct device_attribute hisi_pa_pmu_identifier_attr =
>> -	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
>> -
>> -static struct attribute *hisi_pa_pmu_identifier_attrs[] = {
>> -	&hisi_pa_pmu_identifier_attr.attr,
>> -	NULL
>> -};
>> -
>> -static const struct attribute_group hisi_pa_pmu_identifier_group = {
>> -	.attrs = hisi_pa_pmu_identifier_attrs,
>> -};
>> -
>>  static struct hisi_pa_pmu_int_regs hisi_pa_pmu_regs = {
>>  	.mask_offset = PA_INT_MASK,
>>  	.clear_offset = PA_INT_CLEAR,
>> @@ -385,8 +362,6 @@ static struct hisi_pa_pmu_int_regs hisi_pa_pmu_regs = {
>>  static const struct attribute_group *hisi_pa_pmu_v2_attr_groups[] = {
>>  	&hisi_pa_pmu_v2_format_group,
>>  	&hisi_pa_pmu_v2_events_group,
>> -	&hisi_pa_pmu_cpumask_attr_group,
>> -	&hisi_pa_pmu_identifier_group,
>>  	NULL
>>  };
>>  
>> @@ -399,8 +374,6 @@ static const struct hisi_pmu_dev_info hisi_h32pa_v2 = {
>>  static const struct attribute_group *hisi_pa_pmu_v3_attr_groups[] = {
>>  	&hisi_pa_pmu_v2_format_group,
>>  	&hisi_pa_pmu_v3_events_group,
>> -	&hisi_pa_pmu_cpumask_attr_group,
>> -	&hisi_pa_pmu_identifier_group,
>>  	NULL
>>  };
>>  
>> @@ -419,8 +392,6 @@ static struct hisi_pa_pmu_int_regs hisi_h60pa_pmu_regs = {
>>  static const struct attribute_group *hisi_h60pa_pmu_attr_groups[] = {
>>  	&hisi_pa_pmu_v2_format_group,
>>  	&hisi_h60pa_pmu_events_group,
>> -	&hisi_pa_pmu_cpumask_attr_group,
>> -	&hisi_pa_pmu_identifier_group,
>>  	NULL
>>  };
>>  
>> @@ -450,6 +421,7 @@ static const struct hisi_uncore_ops hisi_uncore_pa_ops = {
>>  static int hisi_pa_pmu_dev_probe(struct platform_device *pdev,
>>  				 struct hisi_pmu *pa_pmu)
>>  {
>> +	struct hisi_pmu_hwevents *pmu_events = &pa_pmu->pmu_events;
>>  	int ret;
>>  
>>  	ret = hisi_pa_pmu_init_data(pdev, pa_pmu);
>> @@ -460,7 +432,14 @@ static int hisi_pa_pmu_dev_probe(struct platform_device *pdev,
>>  	if (ret)
>>  		return ret;
>>  
>> -	pa_pmu->pmu_events.attr_groups = pa_pmu->dev_info->attr_groups;
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
>> +		pa_pmu->dev_info->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT];
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
>> +		pa_pmu->dev_info->attr_groups[HISI_PMU_ATTR_GROUP_EVENT];
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_CPUMASK] =
>> +						&hisi_pmu_cpumask_attr_group;
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_IDENTIFIER] =
>> +						&hisi_pmu_identifier_group;
>>  	pa_pmu->num_counters = PA_NR_COUNTERS;
>>  	pa_pmu->ops = &hisi_uncore_pa_ops;
>>  	pa_pmu->check_event = 0xB0;
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
>> index e1756e639784..648d0e5e08ed 100644
>> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
>> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
>> @@ -49,6 +49,41 @@ ssize_t hisi_cpumask_sysfs_show(struct device *dev,
>>  }
>>  EXPORT_SYMBOL_NS_GPL(hisi_cpumask_sysfs_show, HISI_PMU);
>>  
>> +static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
>> +
>> +static struct attribute *hisi_pmu_cpumask_attrs[] = {
>> +	&dev_attr_cpumask.attr,
>> +	NULL
>> +};
>> +
>> +const struct attribute_group hisi_pmu_cpumask_attr_group = {
>> +	.attrs = hisi_pmu_cpumask_attrs,
>> +};
>> +EXPORT_SYMBOL_NS_GPL(hisi_pmu_cpumask_attr_group, HISI_PMU);
>> +
>> +ssize_t hisi_uncore_pmu_identifier_attr_show(struct device *dev,
>> +					     struct device_attribute *attr,
>> +					     char *page)
>> +{
>> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(dev_get_drvdata(dev));
>> +
>> +	return sysfs_emit(page, "0x%08x\n", hisi_pmu->identifier);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_identifier_attr_show, HISI_PMU);
>> +
>> +static struct device_attribute hisi_pmu_identifier_attr =
>> +	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
>> +
>> +static struct attribute *hisi_pmu_identifier_attrs[] = {
>> +	&hisi_pmu_identifier_attr.attr,
>> +	NULL
>> +};
>> +
>> +const struct attribute_group hisi_pmu_identifier_group = {
>> +	.attrs = hisi_pmu_identifier_attrs,
>> +};
>> +EXPORT_SYMBOL_NS_GPL(hisi_pmu_identifier_group, HISI_PMU);
>> +
>>  static bool hisi_validate_event_group(struct perf_event *event)
>>  {
>>  	struct perf_event *sibling, *leader = event->group_leader;
>> @@ -99,16 +134,6 @@ int hisi_uncore_pmu_get_event_idx(struct perf_event *event)
>>  }
>>  EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_get_event_idx, HISI_PMU);
>>  
>> -ssize_t hisi_uncore_pmu_identifier_attr_show(struct device *dev,
>> -					     struct device_attribute *attr,
>> -					     char *page)
>> -{
>> -	struct hisi_pmu *hisi_pmu = to_hisi_pmu(dev_get_drvdata(dev));
>> -
>> -	return sysfs_emit(page, "0x%08x\n", hisi_pmu->identifier);
>> -}
>> -EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_identifier_attr_show, HISI_PMU);
>> -
>>  static void hisi_uncore_pmu_clear_event_idx(struct hisi_pmu *hisi_pmu, int idx)
>>  {
>>  	clear_bit(idx, hisi_pmu->pmu_events.used_mask);
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h
>> index 239c45d847b3..ac2be8c337b7 100644
>> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.h
>> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
>> @@ -77,10 +77,22 @@ struct hisi_pmu_dev_info {
>>  	void *private;
>>  };
>>  
>> +enum hisi_pmu_attr_groups {
>> +	HISI_PMU_ATTR_GROUP_FORMAT,
>> +	HISI_PMU_ATTR_GROUP_EVENT,
>> +	HISI_PMU_ATTR_GROUP_CPUMASK,
>> +	HISI_PMU_ATTR_GROUP_IDENTIFIER,
>> +	HISI_PMU_ATTR_GROUP_NR
>> +};
>> +
>> +/* Generic implementation of cpumask/identifier group */
>> +extern const struct attribute_group hisi_pmu_cpumask_attr_group;
>> +extern const struct attribute_group hisi_pmu_identifier_group;
>> +
>>  struct hisi_pmu_hwevents {
>>  	struct perf_event *hw_events[HISI_MAX_COUNTERS];
>>  	DECLARE_BITMAP(used_mask, HISI_MAX_COUNTERS);
>> -	const struct attribute_group **attr_groups;
>> +	const struct attribute_group *attr_groups[HISI_PMU_ATTR_GROUP_NR + 1];
>>  };
>>  
>>  /**
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c
>> index 43cbdf0fb7c7..676a7078f2ef 100644
>> --- a/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c
>> +++ b/drivers/perf/hisilicon/hisi_uncore_sllc_pmu.c
>> @@ -344,37 +344,6 @@ static const struct attribute_group hisi_sllc_pmu_v2_events_group = {
>>  	.attrs = hisi_sllc_pmu_v2_events_attr,
>>  };
>>  
>> -static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
>> -
>> -static struct attribute *hisi_sllc_pmu_cpumask_attrs[] = {
>> -	&dev_attr_cpumask.attr,
>> -	NULL
>> -};
>> -
>> -static const struct attribute_group hisi_sllc_pmu_cpumask_attr_group = {
>> -	.attrs = hisi_sllc_pmu_cpumask_attrs,
>> -};
>> -
>> -static struct device_attribute hisi_sllc_pmu_identifier_attr =
>> -	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
>> -
>> -static struct attribute *hisi_sllc_pmu_identifier_attrs[] = {
>> -	&hisi_sllc_pmu_identifier_attr.attr,
>> -	NULL
>> -};
>> -
>> -static const struct attribute_group hisi_sllc_pmu_identifier_group = {
>> -	.attrs = hisi_sllc_pmu_identifier_attrs,
>> -};
>> -
>> -static const struct attribute_group *hisi_sllc_pmu_v2_attr_groups[] = {
>> -	&hisi_sllc_pmu_v2_format_group,
>> -	&hisi_sllc_pmu_v2_events_group,
>> -	&hisi_sllc_pmu_cpumask_attr_group,
>> -	&hisi_sllc_pmu_identifier_group,
>> -	NULL
>> -};
>> -
>>  static const struct hisi_uncore_ops hisi_uncore_sllc_ops = {
>>  	.write_evtype		= hisi_sllc_pmu_write_evtype,
>>  	.get_event_idx		= hisi_uncore_pmu_get_event_idx,
>> @@ -395,6 +364,7 @@ static const struct hisi_uncore_ops hisi_uncore_sllc_ops = {
>>  static int hisi_sllc_pmu_dev_probe(struct platform_device *pdev,
>>  				   struct hisi_pmu *sllc_pmu)
>>  {
>> +	struct hisi_pmu_hwevents *pmu_events = &sllc_pmu->pmu_events;
>>  	int ret;
>>  
>>  	ret = hisi_sllc_pmu_init_data(pdev, sllc_pmu);
>> @@ -405,7 +375,14 @@ static int hisi_sllc_pmu_dev_probe(struct platform_device *pdev,
>>  	if (ret)
>>  		return ret;
>>  
>> -	sllc_pmu->pmu_events.attr_groups = hisi_sllc_pmu_v2_attr_groups;
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
>> +						&hisi_sllc_pmu_v2_format_group;
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
>> +						&hisi_sllc_pmu_v2_events_group;
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_CPUMASK] =
>> +						&hisi_pmu_cpumask_attr_group;
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_IDENTIFIER] =
>> +						&hisi_pmu_identifier_group;
>>  	sllc_pmu->ops = &hisi_uncore_sllc_ops;
>>  	sllc_pmu->check_event = SLLC_NR_EVENTS;
>>  	sllc_pmu->counter_bits = 64;
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
>> index 2040f6a8871e..c2ae852f6b19 100644
>> --- a/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
>> +++ b/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
>> @@ -437,37 +437,6 @@ static const struct attribute_group hisi_uc_pmu_events_group = {
>>  	.attrs = hisi_uc_pmu_events_attr,
>>  };
>>  
>> -static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
>> -
>> -static struct attribute *hisi_uc_pmu_cpumask_attrs[] = {
>> -	&dev_attr_cpumask.attr,
>> -	NULL,
>> -};
>> -
>> -static const struct attribute_group hisi_uc_pmu_cpumask_attr_group = {
>> -	.attrs = hisi_uc_pmu_cpumask_attrs,
>> -};
>> -
>> -static struct device_attribute hisi_uc_pmu_identifier_attr =
>> -	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
>> -
>> -static struct attribute *hisi_uc_pmu_identifier_attrs[] = {
>> -	&hisi_uc_pmu_identifier_attr.attr,
>> -	NULL
>> -};
>> -
>> -static const struct attribute_group hisi_uc_pmu_identifier_group = {
>> -	.attrs = hisi_uc_pmu_identifier_attrs,
>> -};
>> -
>> -static const struct attribute_group *hisi_uc_pmu_attr_groups[] = {
>> -	&hisi_uc_pmu_format_group,
>> -	&hisi_uc_pmu_events_group,
>> -	&hisi_uc_pmu_cpumask_attr_group,
>> -	&hisi_uc_pmu_identifier_group,
>> -	NULL
>> -};
>> -
>>  static const struct hisi_uncore_ops hisi_uncore_uc_pmu_ops = {
>>  	.check_filter		= hisi_uc_pmu_check_filter,
>>  	.write_evtype		= hisi_uc_pmu_write_evtype,
>> @@ -489,6 +458,7 @@ static const struct hisi_uncore_ops hisi_uncore_uc_pmu_ops = {
>>  static int hisi_uc_pmu_dev_probe(struct platform_device *pdev,
>>  				 struct hisi_pmu *uc_pmu)
>>  {
>> +	struct hisi_pmu_hwevents *pmu_events = &uc_pmu->pmu_events;
>>  	int ret;
>>  
>>  	ret = hisi_uc_pmu_init_data(pdev, uc_pmu);
>> @@ -499,7 +469,14 @@ static int hisi_uc_pmu_dev_probe(struct platform_device *pdev,
>>  	if (ret)
>>  		return ret;
>>  
>> -	uc_pmu->pmu_events.attr_groups = hisi_uc_pmu_attr_groups;
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_FORMAT] =
>> +						&hisi_uc_pmu_format_group;
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_EVENT] =
>> +						&hisi_uc_pmu_events_group;
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_CPUMASK] =
>> +						&hisi_pmu_cpumask_attr_group;
>> +	pmu_events->attr_groups[HISI_PMU_ATTR_GROUP_IDENTIFIER] =
>> +						&hisi_pmu_identifier_group;
>>  	uc_pmu->check_event = HISI_UC_EVTYPE_MASK;
>>  	uc_pmu->ops = &hisi_uncore_uc_pmu_ops;
>>  	uc_pmu->counter_bits = HISI_UC_CNTR_REG_BITS;
> 
> .
> 

[PATCH 6/8] drivers/perf: hisi: Export associated CPUs of each PMU through sysfs

[Yicong Yang]

From: Yicong Yang <yangyicong@hisilicon.com>

Although the event of the uncore PMU can only be opened on a single
CPU, some PMU does have the affinity on a range of CPUs. For example
the L3C PMU is associated to the CPUs sharing the L3T it monitors.
Users may infer this affinity by the PMU name which may have SCCL ID
and CCL ID encoded (for L3C etc), but it's not that straightforward.
So export this information by adding an "associated_cpus" sysfs
attribute then user can get this directly.

Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 Documentation/admin-guide/perf/hisi-pmu.rst |  5 ++++-
 drivers/perf/hisilicon/hisi_uncore_pmu.c    | 10 ++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/Documentation/admin-guide/perf/hisi-pmu.rst b/Documentation/admin-guide/perf/hisi-pmu.rst
index 5cc248d18c63..2b3d5a90c34c 100644
--- a/Documentation/admin-guide/perf/hisi-pmu.rst
+++ b/Documentation/admin-guide/perf/hisi-pmu.rst
@@ -35,7 +35,10 @@ e.g. hisi_sccl1_hha0/rx_operations is RX_OPERATIONS event of HHA index #0 in
 SCCL ID #1.
 
 The driver also provides a "cpumask" sysfs attribute, which shows the CPU core
-ID used to count the uncore PMU event.
+ID used to count the uncore PMU event. An "associated_cpus" sysfs attribute is
+also provided to show the CPUs associated with this PMU. The "cpumask" indicates
+the CPUs to open the events, usually as a hit for userspaces tools like perf.
+It only contains one associated CPU from the "associated_cpus".
 
 Example usage of perf::
 
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
index 648d0e5e08ed..b9437522ec73 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
@@ -51,8 +51,18 @@ EXPORT_SYMBOL_NS_GPL(hisi_cpumask_sysfs_show, HISI_PMU);
 
 static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
 
+static ssize_t hisi_associated_cpus_sysfs_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct hisi_pmu *hisi_pmu = to_hisi_pmu(dev_get_drvdata(dev));
+
+	return cpumap_print_to_pagebuf(true, buf, &hisi_pmu->associated_cpus);
+}
+static DEVICE_ATTR(associated_cpus, 0444, hisi_associated_cpus_sysfs_show, NULL);
+
 static struct attribute *hisi_pmu_cpumask_attrs[] = {
 	&dev_attr_cpumask.attr,
+	&dev_attr_associated_cpus.attr,
 	NULL
 };
 
-- 
2.24.0

Re: [PATCH 6/8] drivers/perf: hisi: Export associated CPUs of each PMU through sysfs

[Jonathan Cameron]

On Fri, 18 Oct 2024 17:57:43 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Yicong Yang <yangyicong@hisilicon.com>
> 
> Although the event of the uncore PMU can only be opened on a single
> CPU, some PMU does have the affinity on a range of CPUs. For example
> the L3C PMU is associated to the CPUs sharing the L3T it monitors.
> Users may infer this affinity by the PMU name which may have SCCL ID
> and CCL ID encoded (for L3C etc), but it's not that straightforward.
> So export this information by adding an "associated_cpus" sysfs
> attribute then user can get this directly.
> 
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>

Seems sensible to expose this.
Reviewed-by: Jonathan Cameron <Joanthan.Cameron@huawei.com>

[PATCH 7/8] drivers/perf: hisi: Fix incorrect variable name "hha_pmu" in DDRC PMU driver

[Yicong Yang]

From: Junhao He <hejunhao3@huawei.com>

In the callback function write_evtype(), the variable name of struct
hisi_pmu should be "ddrc_pmu" instead of "hha_pmu".

Signed-off-by: Junhao He <hejunhao3@huawei.com>
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
index 6d805ca4562f..62983a9fc9a1 100644
--- a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
@@ -111,14 +111,14 @@ static void hisi_ddrc_pmu_v2_write_counter(struct hisi_pmu *ddrc_pmu,
  * so there is no need to write event type, while it is programmable counter in
  * PMU v2.
  */
-static void hisi_ddrc_pmu_write_evtype(struct hisi_pmu *hha_pmu, int idx,
+static void hisi_ddrc_pmu_write_evtype(struct hisi_pmu *ddrc_pmu, int idx,
 				       u32 type)
 {
 	u32 offset;
 
-	if (hha_pmu->identifier >= HISI_PMU_V2) {
+	if (ddrc_pmu->identifier >= HISI_PMU_V2) {
 		offset = DDRC_V2_EVENT_TYPE + 4 * idx;
-		writel(type, hha_pmu->base + offset);
+		writel(type, ddrc_pmu->base + offset);
 	}
 }
 
-- 
2.24.0

Re: [PATCH 7/8] drivers/perf: hisi: Fix incorrect variable name "hha_pmu" in DDRC PMU driver

[Jonathan Cameron]

On Fri, 18 Oct 2024 17:57:44 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Junhao He <hejunhao3@huawei.com>
> 
> In the callback function write_evtype(), the variable name of struct
> hisi_pmu should be "ddrc_pmu" instead of "hha_pmu".
> 
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

[PATCH 8/8] drivers/perf: hisi: Delete redundant blank line of DDRC PMU

[Yicong Yang]

From: Junhao He <hejunhao3@huawei.com>

Do not add blank line at the end of a code block defined by braces.

Signed-off-by: Junhao He <hejunhao3@huawei.com>
Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
---
 drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
index 62983a9fc9a1..a087812cc7ec 100644
--- a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
@@ -546,7 +546,6 @@ static void __exit hisi_ddrc_pmu_module_exit(void)
 {
 	platform_driver_unregister(&hisi_ddrc_pmu_driver);
 	cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_DDRC_ONLINE);
-
 }
 module_exit(hisi_ddrc_pmu_module_exit);
 
-- 
2.24.0

Re: [PATCH 8/8] drivers/perf: hisi: Delete redundant blank line of DDRC PMU

[Jonathan Cameron]

On Fri, 18 Oct 2024 17:57:45 +0800
Yicong Yang <yangyicong@huawei.com> wrote:

> From: Junhao He <hejunhao3@huawei.com>
> 
> Do not add blank line at the end of a code block defined by braces.
> 
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
> index 62983a9fc9a1..a087812cc7ec 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
> @@ -546,7 +546,6 @@ static void __exit hisi_ddrc_pmu_module_exit(void)
>  {
>  	platform_driver_unregister(&hisi_ddrc_pmu_driver);
>  	cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_DDRC_ONLINE);
> -
>  }
>  module_exit(hisi_ddrc_pmu_module_exit);
>