[PATCH v3 0/6] Add HiSilicon SoC uncore Performance Monitoring Unit driver

[PATCH v3 0/6] Add HiSilicon SoC uncore Performance Monitoring Unit driver

[Shaokun Zhang]

This patchset adds support for HiSilicon SoC uncore PMUs driver. It
includes L3C, Hydra Home Agent (HHA) and DDRC.

Changes in v3:
* rebase to 4.13-rc1
* add dev_err if ioremap fails for PMUs
 
Changes in v2:
* fix kbuild test robot error
* make hisi_uncore_ops static

Shaokun Zhang (6):
  Documentation: perf: hisi: Documentation for HiSilicon SoC PMU driver
  perf: hisi: Add support for HiSilicon SoC uncore PMU driver
  perf: hisi: Add support for HiSilicon SoC L3C PMU driver
  perf: hisi: Add support for HiSilicon SoC HHA PMU driver
  perf: hisi: Add support for HiSilicon SoC DDRC PMU driver
  arm64: MAINTAINERS: hisi: Add HiSilicon SoC PMU support

 Documentation/perf/hisi-pmu.txt               |  51 +++
 MAINTAINERS                                   |   7 +
 drivers/perf/Kconfig                          |   7 +
 drivers/perf/Makefile                         |   1 +
 drivers/perf/hisilicon/Makefile               |   1 +
 drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c | 437 ++++++++++++++++++++
 drivers/perf/hisilicon/hisi_uncore_hha_pmu.c  | 453 +++++++++++++++++++++
 drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c  | 556 ++++++++++++++++++++++++++
 drivers/perf/hisilicon/hisi_uncore_pmu.c      | 411 +++++++++++++++++++
 drivers/perf/hisilicon/hisi_uncore_pmu.h      | 116 ++++++
 include/linux/cpuhotplug.h                    |   1 +
 11 files changed, 2041 insertions(+)
 create mode 100644 Documentation/perf/hisi-pmu.txt
 create mode 100644 drivers/perf/hisilicon/Makefile
 create mode 100644 drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
 create mode 100644 drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
 create mode 100644 drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
 create mode 100644 drivers/perf/hisilicon/hisi_uncore_pmu.c
 create mode 100644 drivers/perf/hisilicon/hisi_uncore_pmu.h

-- 
1.9.1

[PATCH v3 1/6] Documentation: perf: hisi: Documentation for HiSilicon SoC PMU driver

[Shaokun Zhang]

This patch adds documentation for the uncore PMUs on HiSilicon SoC.

Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
Signed-off-by: Anurup M <anurup.m@huawei.com>
---
 Documentation/perf/hisi-pmu.txt | 51 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/perf/hisi-pmu.txt

diff --git a/Documentation/perf/hisi-pmu.txt b/Documentation/perf/hisi-pmu.txt
new file mode 100644
index 0000000..5fa0b1a
--- /dev/null
+++ b/Documentation/perf/hisi-pmu.txt
@@ -0,0 +1,51 @@
+HiSilicon SoC uncore Performance Monitoring Unit (PMU)
+======================================================
+The HiSilicon SoC chip comprehends various independent system device PMUs
+such as L3 cache (L3C), Hydra Home Agent (HHA) and DDRC. These PMUs are
+independent and have hardware logic to gather statistics and performance
+information.
+
+HiSilicon SoC encapsulates multiple CPU and IO dies. Each CPU cluster (CC
+L) is made up of 4 cpu cores sharing one L3 cache; Each CPU die is called
+Super CPU cluster (SCCL) and is made up of 6 CCLs. Each SCCL has two HHAs
+(0 - 1) and four DDRCs (0 - 3), respectively.
+
+HiSilicon SoC uncore PMU driver
+---------------------------------------
+Each device PMU has separate registers for event counting, control and
+interrupt, and the PMU driver shall register perf PMU drivers like L3C,
+HHA and DDRC etc. The available events and configuration options shall
+be described in the sysfs, see /sys/devices/hisi_*.
+The "perf list" command shall list the available events from sysfs.
+
+Each L3C, HHA and DDRC in one SCCL are registered as an separate PMU with perf.
+The PMU name will appear in event listing as hisi_module <index-id>_<sccl-id>.
+where "index-id" is the index of module and "sccl-id" is the identifier of
+the SCCL.
+e.g. hisi_l3c0_1/rd_hit_cpipe is READ_HIT_CPIPE event of L3C index #0 and SCCL
+ID #1.
+e.g. hisi_hha0_1/rx_operations is RX_OPERATIONS event of HHA index #0 and SCCL
+ID #1.
+
+The driver also provides a "cpumask" sysfs attribute, which shows the CPU core
+ID used to count the uncore PMU event.
+
+Example usage of perf:
+$# perf list
+hisi_l3c0_3/rd_hit_cpipe/ [kernel PMU event]
+------------------------------------------
+hisi_l3c0_3/wr_hit_cpipe/ [kernel PMU event]
+------------------------------------------
+hisi_l3c0_1/rd_hit_cpipe/ [kernel PMU event]
+------------------------------------------
+hisi_l3c0_1/wr_hit_cpipe/ [kernel PMU event]
+------------------------------------------
+
+$# perf stat -a -e hisi_l3c0_1/rd_hit_cpipe/ sleep 5
+$# perf stat -a -e hisi_l3c0_1/config=0x02/ sleep 5
+
+The current driver does not support sampling. So "perf record" is unsupported.
+Also attach to a task is unsupported as the events are all uncore.
+
+Note: Please contact the maintainer for a complete list of events supported for
+the PMU devices in the SoC and its information if needed.
-- 
1.9.1

[PATCH v3 2/6] perf: hisi: Add support for HiSilicon SoC uncore PMU driver

[Shaokun Zhang]

This patch adds support HiSilicon SoC uncore PMU driver framework and
interfaces.

Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
Signed-off-by: Anurup M <anurup.m@huawei.com>
---
 drivers/perf/Kconfig                     |   7 +
 drivers/perf/Makefile                    |   1 +
 drivers/perf/hisilicon/Makefile          |   1 +
 drivers/perf/hisilicon/hisi_uncore_pmu.c | 411 +++++++++++++++++++++++++++++++
 drivers/perf/hisilicon/hisi_uncore_pmu.h | 116 +++++++++
 5 files changed, 536 insertions(+)
 create mode 100644 drivers/perf/hisilicon/Makefile
 create mode 100644 drivers/perf/hisilicon/hisi_uncore_pmu.c
 create mode 100644 drivers/perf/hisilicon/hisi_uncore_pmu.h

diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
index e5197ff..78fc4bc 100644
--- a/drivers/perf/Kconfig
+++ b/drivers/perf/Kconfig
@@ -17,6 +17,13 @@ config ARM_PMU_ACPI
 	depends on ARM_PMU && ACPI
 	def_bool y
 
+config HISI_PMU
+	bool "HiSilicon SoC PMU"
+	depends on ARM64 && ACPI
+	help
+	  Support for HiSilicon SoC uncore performance monitoring
+	  unit (PMU), such as: L3C, HHA and DDRC.
+
 config QCOM_L2_PMU
 	bool "Qualcomm Technologies L2-cache PMU"
 	depends on ARCH_QCOM && ARM64 && ACPI
diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
index 6420bd4..41d3342 100644
--- a/drivers/perf/Makefile
+++ b/drivers/perf/Makefile
@@ -1,5 +1,6 @@
 obj-$(CONFIG_ARM_PMU) += arm_pmu.o arm_pmu_platform.o
 obj-$(CONFIG_ARM_PMU_ACPI) += arm_pmu_acpi.o
+obj-$(CONFIG_HISI_PMU) += hisilicon/
 obj-$(CONFIG_QCOM_L2_PMU)	+= qcom_l2_pmu.o
 obj-$(CONFIG_QCOM_L3_PMU) += qcom_l3_pmu.o
 obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
diff --git a/drivers/perf/hisilicon/Makefile b/drivers/perf/hisilicon/Makefile
new file mode 100644
index 0000000..2783bb3
--- /dev/null
+++ b/drivers/perf/hisilicon/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
new file mode 100644
index 0000000..4eb04e1
--- /dev/null
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
@@ -0,0 +1,411 @@
+/*
+ * HiSilicon SoC Hardware event counters support
+ *
+ * Copyright (C) 2017 Hisilicon Limited
+ * Author: Anurup M <anurup.m@huawei.com>
+ *         Shaokun Zhang <zhangshaokun@hisilicon.com>
+ *
+ * This code is based on the uncore PMUs like arm-cci and arm-ccn.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/bitmap.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/perf_event.h>
+#include "hisi_uncore_pmu.h"
+
+/*
+ * PMU format attributes
+ */
+ssize_t hisi_format_sysfs_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	struct dev_ext_attribute *eattr;
+
+	eattr = container_of(attr, struct dev_ext_attribute, attr);
+
+	return sprintf(buf, "%s\n", (char *)eattr->var);
+}
+
+/*
+ * PMU event attributes
+ */
+ssize_t hisi_event_sysfs_show(struct device *dev,
+			      struct device_attribute *attr, char *page)
+{
+	struct dev_ext_attribute *eattr;
+
+	eattr = container_of(attr, struct dev_ext_attribute, attr);
+
+	return sprintf(page, "config=0x%lx\n", (unsigned long)eattr->var);
+}
+
+/*
+ * sysfs cpumask attributes
+ */
+ssize_t hisi_cpumask_sysfs_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct pmu *pmu = dev_get_drvdata(dev);
+	struct hisi_pmu *hisi_pmu = to_hisi_pmu(pmu);
+
+	return cpumap_print_to_pagebuf(true, buf, &hisi_pmu->cpus);
+}
+
+/* Read Super CPU cluster and CPU cluster ID from MPIDR_EL1 */
+void hisi_read_scl_and_ccl_id(u32 *scl_id, u32 *ccl_id)
+{
+	u64 mpidr;
+
+	mpidr = read_cpuid_mpidr();
+	if (mpidr & MPIDR_MT_BITMASK) {
+		if (scl_id)
+			*scl_id = MPIDR_AFFINITY_LEVEL(mpidr, 3);
+		if (ccl_id)
+			*ccl_id = MPIDR_AFFINITY_LEVEL(mpidr, 2);
+	} else {
+		if (scl_id)
+			*scl_id = MPIDR_AFFINITY_LEVEL(mpidr, 2);
+		if (ccl_id)
+			*ccl_id = MPIDR_AFFINITY_LEVEL(mpidr, 1);
+	}
+}
+
+static bool hisi_validate_event_group(struct perf_event *event)
+{
+	struct perf_event *sibling, *leader = event->group_leader;
+	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
+	/* Include count for the event */
+	int counters = 1;
+
+	/*
+	 * We must NOT create groups containing mixed PMUs, although
+	 * software events are acceptable
+	 */
+	if (leader->pmu != event->pmu && !is_software_event(leader))
+		return false;
+
+	/* Increment counter for the leader */
+	counters++;
+
+	list_for_each_entry(sibling, &event->group_leader->sibling_list,
+			    group_entry) {
+		if (is_software_event(sibling))
+			continue;
+		if (sibling->pmu != event->pmu)
+			return false;
+		/* Increment counter for each sibling */
+		counters++;
+	}
+
+	/* The group can not count events more than the counters in the HW */
+	return counters <= hisi_pmu->num_counters;
+}
+
+int hisi_uncore_pmu_counter_valid(struct hisi_pmu *hisi_pmu, int idx)
+{
+	return (idx >= 0 && idx < hisi_pmu->num_counters);
+}
+
+int hisi_uncore_pmu_get_event_idx(struct perf_event *event)
+{
+	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
+	unsigned long *used_mask = hisi_pmu->pmu_events.used_mask;
+	u32 num_counters = hisi_pmu->num_counters;
+	int idx;
+
+	idx = find_first_zero_bit(used_mask, num_counters);
+	if (idx == num_counters)
+		return -EAGAIN;
+
+	set_bit(idx, used_mask);
+
+	return idx;
+}
+
+void hisi_uncore_pmu_clear_event_idx(struct hisi_pmu *hisi_pmu, int idx)
+{
+	if (!hisi_uncore_pmu_counter_valid(hisi_pmu, idx)) {
+		dev_err(hisi_pmu->dev, "Unsupported event index:%d!\n", idx);
+		return;
+	}
+
+	clear_bit(idx, hisi_pmu->pmu_events.used_mask);
+}
+
+int hisi_uncore_pmu_event_init(struct perf_event *event)
+{
+	struct hisi_pmu *hisi_pmu;
+	struct hw_perf_event *hwc = &event->hw;
+	int cpu;
+
+	if (event->attr.type != event->pmu->type)
+		return -ENOENT;
+
+	/*
+	 * We do not support sampling as the counters are all
+	 * shared by all CPU cores in a CPU die(SCCL). Also we
+	 * do not support attach to a task(per-process mode)
+	 */
+	if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK)
+		return -EOPNOTSUPP;
+
+	/* counters do not have these bits */
+	if (event->attr.exclude_user	||
+	    event->attr.exclude_kernel	||
+	    event->attr.exclude_host	||
+	    event->attr.exclude_guest	||
+	    event->attr.exclude_hv	||
+	    event->attr.exclude_idle)
+		return -EINVAL;
+
+	/*
+	 *  The uncore counters not specific to any CPU, so cannot
+	 *  support per-task
+	 */
+	if (event->cpu < 0)
+		return -EINVAL;
+
+	/*
+	 * Validate if the events in group does not exceed the
+	 * available counters in hardware.
+	 */
+	if (!hisi_validate_event_group(event))
+		return -EINVAL;
+
+	/*
+	 * We don't assign an index until we actually place the event onto
+	 * hardware. Use -1 to signify that we haven't decided where to put it
+	 * yet.
+	 */
+	hwc->idx		= -1;
+	hwc->config_base	= event->attr.config;
+
+	/* Select an available CPU to monitor events in this PMU */
+	hisi_pmu = to_hisi_pmu(event->pmu);
+	cpu = cpumask_first(&hisi_pmu->cpus);
+	if (cpu >= nr_cpu_ids)
+		return -EINVAL;
+
+	/* Enforce to use the same CPU for all events in this PMU */
+	event->cpu = cpu;
+
+	return 0;
+}
+
+/*
+ * Set the counter to count the event that we're interested in,
+ * and enable counter and interrupt.
+ */
+static void hisi_uncore_pmu_enable_event(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
+
+	/*
+	 * Write event code in event select registers(for DDRC PMU,
+	 * event has been mapped to fixed-purpose counter, there is
+	 * no need to write event type).
+	 */
+	if (hisi_pmu->ops->write_evtype)
+		hisi_pmu->ops->write_evtype(hisi_pmu, hwc->idx,
+					    GET_EVENTID(event));
+
+	/* Enable the hardware event counting and interrupt */
+	hisi_pmu->ops->enable_counter(hisi_pmu, hwc);
+	hisi_pmu->ops->enable_counter_int(hisi_pmu, hwc);
+}
+
+/*
+ * Disable counting and interrupt.
+ */
+static void hisi_uncore_pmu_disable_event(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
+
+	hisi_pmu->ops->disable_counter(hisi_pmu, hwc);
+	hisi_pmu->ops->disable_counter_int(hisi_pmu, hwc);
+}
+
+void hisi_uncore_pmu_set_event_period(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
+
+	/*
+	 * The HiSilicon PMU counters support 32 bits or 48 bits, depending on
+	 * the PMU. We reduce it to 2^(counter_bits - 1) to account for the
+	 * extreme interrupt latency. So we could hopefully handle the overflow
+	 * interrupt before another 2^(counter_bits - 1) events occur and the
+	 * counter overtakes its previous value.
+	 */
+	u64 val = BIT_ULL(hisi_pmu->counter_bits - 1);
+
+	local64_set(&hwc->prev_count, val);
+	/* Write start value to the hardware event counter */
+	hisi_pmu->ops->write_counter(hisi_pmu, hwc, val);
+}
+
+u64 hisi_uncore_pmu_event_update(struct perf_event *event)
+{
+	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+	u64 delta, prev_raw_count, new_raw_count;
+
+	do {
+		/* Read the count from the counter register */
+		new_raw_count = hisi_pmu->ops->read_counter(hisi_pmu, hwc);
+		prev_raw_count = local64_read(&hwc->prev_count);
+	} while (local64_cmpxchg(&hwc->prev_count, prev_raw_count,
+				 new_raw_count) != prev_raw_count);
+	/*
+	 * compute the delta
+	 */
+	delta = (new_raw_count - prev_raw_count) &
+		HISI_MAX_PERIOD(hisi_pmu->counter_bits);
+	local64_add(delta, &event->count);
+
+	return new_raw_count;
+}
+
+void hisi_uncore_pmu_start(struct perf_event *event, int flags)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
+
+	if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED)))
+		return;
+
+	WARN_ON_ONCE(!(hwc->state & PERF_HES_UPTODATE));
+	hwc->state = 0;
+	hisi_uncore_pmu_set_event_period(event);
+
+	if (flags & PERF_EF_RELOAD) {
+		u64 prev_raw_count =  local64_read(&hwc->prev_count);
+
+		hisi_pmu->ops->write_counter(hisi_pmu, hwc, prev_raw_count);
+	}
+
+	hisi_uncore_pmu_enable_event(event);
+	perf_event_update_userpage(event);
+}
+
+void hisi_uncore_pmu_stop(struct perf_event *event, int flags)
+{
+	struct hw_perf_event *hwc = &event->hw;
+
+	hisi_uncore_pmu_disable_event(event);
+	WARN_ON_ONCE(hwc->state & PERF_HES_STOPPED);
+	hwc->state |= PERF_HES_STOPPED;
+
+	if (hwc->state & PERF_HES_UPTODATE)
+		return;
+
+	/* Read hardware counter and update the Perf counter statistics */
+	hisi_uncore_pmu_event_update(event);
+	hwc->state |= PERF_HES_UPTODATE;
+}
+
+int hisi_uncore_pmu_add(struct perf_event *event, int flags)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
+	int idx;
+
+	hwc->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
+
+	/* Get an available counter index for counting */
+	idx = hisi_pmu->ops->get_event_idx(event);
+	if (idx < 0)
+		return -EAGAIN;
+
+	event->hw.idx = idx;
+	hisi_pmu->pmu_events.hw_events[idx] = event;
+
+	if (flags & PERF_EF_START)
+		hisi_uncore_pmu_start(event, PERF_EF_RELOAD);
+
+	return 0;
+}
+
+void hisi_uncore_pmu_del(struct perf_event *event, int flags)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
+
+	hisi_uncore_pmu_stop(event, PERF_EF_UPDATE);
+	hisi_uncore_pmu_clear_event_idx(hisi_pmu, hwc->idx);
+	perf_event_update_userpage(event);
+	hisi_pmu->pmu_events.hw_events[hwc->idx] = NULL;
+}
+
+struct hisi_pmu *hisi_pmu_alloc(struct device *dev, u32 num_cntrs)
+{
+	struct hisi_pmu *hisi_pmu;
+	struct hisi_pmu_hwevents *pmu_events;
+
+	hisi_pmu = devm_kzalloc(dev, sizeof(*hisi_pmu), GFP_KERNEL);
+	if (!hisi_pmu)
+		return ERR_PTR(-ENOMEM);
+
+	pmu_events = &hisi_pmu->pmu_events;
+	pmu_events->hw_events = devm_kcalloc(dev,
+					     num_cntrs,
+					     sizeof(*pmu_events->hw_events),
+					     GFP_KERNEL);
+	if (!pmu_events->hw_events)
+		return ERR_PTR(-ENOMEM);
+
+	pmu_events->used_mask = devm_kcalloc(dev,
+					     BITS_TO_LONGS(num_cntrs),
+					     sizeof(*pmu_events->used_mask),
+					     GFP_KERNEL);
+	if (!pmu_events->used_mask)
+		return ERR_PTR(-ENOMEM);
+
+	return 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);
+}
+
+void hisi_uncore_pmu_enable(struct pmu *pmu)
+{
+	struct hisi_pmu *hisi_pmu = to_hisi_pmu(pmu);
+	int enabled = bitmap_weight(hisi_pmu->pmu_events.used_mask,
+				    hisi_pmu->num_counters);
+
+	if (!enabled)
+		return;
+
+	hisi_pmu->ops->start_counters(hisi_pmu);
+}
+
+void hisi_uncore_pmu_disable(struct pmu *pmu)
+{
+	struct hisi_pmu *hisi_pmu = to_hisi_pmu(pmu);
+
+	hisi_pmu->ops->stop_counters(hisi_pmu);
+}
+
+int hisi_uncore_pmu_setup(struct hisi_pmu *hisi_pmu, const char *pmu_name)
+{
+	/* Register the events with perf */
+	return perf_pmu_register(&hisi_pmu->pmu, pmu_name, -1);
+}
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h
new file mode 100644
index 0000000..eb2abb0
--- /dev/null
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
@@ -0,0 +1,116 @@
+/*
+ * HiSilicon SoC Hardware event counters support
+ *
+ * Copyright (C) 2017 Hisilicon Limited
+ * Author: Anurup M <anurup.m@huawei.com>
+ *         Shaokun Zhang <zhangshaokun@hisilicon.com>
+ *
+ * This code is based on the uncore PMUs like arm-cci and arm-ccn.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef __HISI_UNCORE_PMU_H__
+#define __HISI_UNCORE_PMU_H__
+
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/ktime.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <asm/local64.h>
+
+#undef pr_fmt
+#define pr_fmt(fmt)     "hisi_pmu: " fmt
+
+#define GET_EVENTID(ev)   (ev->hw.config_base & 0xff)
+#define to_hisi_pmu(p)	(container_of(p, struct hisi_pmu, pmu))
+#define HISI_MAX_PERIOD(nr) (BIT_ULL(nr) - 1)
+
+#define HISI_PMU_ATTR(_name, _func, _config)				\
+	(&((struct dev_ext_attribute[]) {				\
+		{ __ATTR(_name, 0444, _func, NULL), (void *)_config }   \
+	})[0].attr.attr)
+
+#define HISI_PMU_FORMAT_ATTR(_name, _config)		\
+	HISI_PMU_ATTR(_name, hisi_format_sysfs_show, (void *)_config)
+#define HISI_PMU_EVENT_ATTR(_name, _config)		\
+	HISI_PMU_ATTR(_name, hisi_event_sysfs_show, (unsigned long)_config)
+
+struct hisi_pmu;
+
+struct hisi_uncore_ops {
+	void (*write_evtype)(struct hisi_pmu *, int, u32);
+	int (*get_event_idx)(struct perf_event *);
+	u64 (*read_counter)(struct hisi_pmu *, struct hw_perf_event *);
+	void (*write_counter)(struct hisi_pmu *, struct hw_perf_event *, u64);
+	void (*enable_counter)(struct hisi_pmu *, struct hw_perf_event *);
+	void (*disable_counter)(struct hisi_pmu *, struct hw_perf_event *);
+	void (*enable_counter_int)(struct hisi_pmu *, struct hw_perf_event *);
+	void (*disable_counter_int)(struct hisi_pmu *, struct hw_perf_event *);
+	void (*start_counters)(struct hisi_pmu *);
+	void (*stop_counters)(struct hisi_pmu *);
+};
+
+struct hisi_pmu_hwevents {
+	struct perf_event **hw_events;
+	unsigned long *used_mask;
+};
+
+/* Generic pmu struct for different pmu types */
+struct hisi_pmu {
+	const char *name;
+	struct pmu pmu;
+	const struct hisi_uncore_ops *ops;
+	struct hisi_pmu_hwevents pmu_events;
+	cpumask_t cpus;
+	struct device *dev;
+	struct hlist_node node;
+	u32 scl_id;
+	u32 ccl_id;
+	/* Hardware information for different pmu types */
+	void __iomem *base;
+	union {
+		u32 ddrc_chn_id;
+		u32 l3c_tag_id;
+		u32 hha_uid;
+	};
+	int num_counters;
+	int num_events;
+	int counter_bits;
+};
+
+int hisi_uncore_pmu_counter_valid(struct hisi_pmu *hisi_pmu, int idx);
+int hisi_uncore_pmu_get_event_idx(struct perf_event *event);
+void hisi_uncore_pmu_clear_event_idx(struct hisi_pmu *hisi_pmu, int idx);
+void hisi_uncore_pmu_read(struct perf_event *event);
+int hisi_uncore_pmu_add(struct perf_event *event, int flags);
+void hisi_uncore_pmu_del(struct perf_event *event, int flags);
+void hisi_uncore_pmu_start(struct perf_event *event, int flags);
+void hisi_uncore_pmu_stop(struct perf_event *event, int flags);
+void hisi_uncore_pmu_set_event_period(struct perf_event *event);
+u64 hisi_uncore_pmu_event_update(struct perf_event *event);
+int hisi_uncore_pmu_event_init(struct perf_event *event);
+int hisi_uncore_pmu_setup(struct hisi_pmu *hisi_pmu, const char *pmu_name);
+void hisi_uncore_pmu_enable(struct pmu *pmu);
+void hisi_uncore_pmu_disable(struct pmu *pmu);
+struct hisi_pmu *hisi_pmu_alloc(struct device *dev, u32 num_cntrs);
+ssize_t hisi_event_sysfs_show(struct device *dev,
+			      struct device_attribute *attr, char *buf);
+ssize_t hisi_format_sysfs_show(struct device *dev,
+			       struct device_attribute *attr, char *buf);
+ssize_t hisi_cpumask_sysfs_show(struct device *dev,
+				struct device_attribute *attr, char *buf);
+void hisi_read_scl_and_ccl_id(u32 *scl_id, u32 *ccl_id);
+#endif /* __HISI_UNCORE_PMU_H__ */
-- 
1.9.1

[PATCH v3 3/6] perf: hisi: Add support for HiSilicon SoC L3C PMU driver

[Shaokun Zhang]

This patch adds support for L3C PMU driver in HiSilicon SoC chip, Each
L3C has own control, counter and interrupt registers and is an separate
PMU. For each L3C PMU, it has 8-programable counters and supports 0x60
events, event code is 8-bits and every counter is free-running.
Interrupt is supported to handle counter (48-bits) overflow.

Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
Signed-off-by: Anurup M <anurup.m@huawei.com>
---
 drivers/perf/hisilicon/Makefile              |   2 +-
 drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c | 556 +++++++++++++++++++++++++++
 include/linux/cpuhotplug.h                   |   1 +
 3 files changed, 558 insertions(+), 1 deletion(-)
 create mode 100644 drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c

diff --git a/drivers/perf/hisilicon/Makefile b/drivers/perf/hisilicon/Makefile
index 2783bb3..4a3d3e6 100644
--- a/drivers/perf/hisilicon/Makefile
+++ b/drivers/perf/hisilicon/Makefile
@@ -1 +1 @@
-obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o
+obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o hisi_uncore_l3c_pmu.o
diff --git a/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
new file mode 100644
index 0000000..d430508
--- /dev/null
+++ b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
@@ -0,0 +1,556 @@
+/*
+ * HiSilicon SoC L3C uncore Hardware event counters support
+ *
+ * Copyright (C) 2017 Hisilicon Limited
+ * Author: Anurup M <anurup.m@huawei.com>
+ *         Shaokun Zhang <zhangshaokun@hisilicon.com>
+ *
+ * This code is based on the uncore PMUs like arm-cci and arm-ccn.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/acpi.h>
+#include <linux/bitmap.h>
+#include <linux/cpuhotplug.h>
+#include <linux/module.h>
+#include <linux/perf_event.h>
+#include <linux/topology.h>
+#include "hisi_uncore_pmu.h"
+
+/* L3C register definition */
+#define L3C_PERF_CTRL		0x0408
+#define L3C_INT_MASK		0x0800
+#define L3C_INT_STATUS		0x0808
+#define L3C_INT_CLEAR		0x080c
+#define L3C_EVENT_CTRL	        0x1c00
+#define L3C_EVENT_TYPE0		0x1d00
+#define L3C_CNTR0_LOWER		0x1e00
+
+/* L3C has 8-counters and supports 0x60 events */
+#define L3C_NR_COUNTERS		0x8
+#define L3C_NR_EVENTS		0x60
+
+#define L3C_PERF_CTRL_EN	0x20000
+#define L3C_EVTYPE_NONE		0xff
+
+/*
+ * Select the counter register offset using the counter index
+ * every counter is 48-bits and [48:63] is reserved.
+ */
+static u32 get_counter_reg_off(int cntr_idx)
+{
+	return (L3C_CNTR0_LOWER + (cntr_idx * 8));
+}
+
+static u64 hisi_l3c_pmu_read_counter(struct hisi_pmu *l3c_pmu,
+				     struct hw_perf_event *hwc)
+{
+	u32 idx = hwc->idx;
+	u32 reg;
+
+	if (!hisi_uncore_pmu_counter_valid(l3c_pmu, idx)) {
+		dev_err(l3c_pmu->dev, "Unsupported event index:%d!\n", idx);
+		return 0;
+	}
+
+	reg = get_counter_reg_off(idx);
+
+	/* Read 64-bits and the upper 16 bits are Read-As-Zero */
+	return readq(l3c_pmu->base + reg);
+}
+
+static void hisi_l3c_pmu_write_counter(struct hisi_pmu *l3c_pmu,
+				       struct hw_perf_event *hwc, u64 val)
+{
+	u32 idx = hwc->idx;
+	u32 reg;
+
+	if (!hisi_uncore_pmu_counter_valid(l3c_pmu, idx)) {
+		dev_err(l3c_pmu->dev, "Unsupported event index:%d!\n", idx);
+		return;
+	}
+
+	reg = get_counter_reg_off(idx);
+	/* Write 64-bits and the upper 16 bits are Writes-Ignored */
+	writeq(val, l3c_pmu->base + reg);
+}
+
+static void hisi_l3c_pmu_write_evtype(struct hisi_pmu *l3c_pmu, int idx,
+				      u32 type)
+{
+	u32 reg, reg_idx, shift, val;
+
+	/*
+	 * Select the appropriate event select register(L3C_EVENT_TYPE0/1).
+	 * There are 2 event select registers for the 8 hardware counters.
+	 * Event code is 8-bits and for the former 4 hardware counters,
+	 * L3C_EVENT_TYPE0 is chosen. For the latter 4 hardware counters,
+	 * L3C_EVENT_TYPE1 is chosen.
+	 */
+	reg = L3C_EVENT_TYPE0 + (idx / 4) * 4;
+	reg_idx = idx % 4;
+	shift = 8 * reg_idx;
+
+	/* Write event code to L3C_EVENT_TYPEx Register */
+	val = readl(l3c_pmu->base + reg);
+	val &= ~(L3C_EVTYPE_NONE << shift);
+	val |= (type << shift);
+	writel(val, l3c_pmu->base + reg);
+}
+
+static void hisi_l3c_pmu_start_counters(struct hisi_pmu *l3c_pmu)
+{
+	u32 val;
+
+	/*
+	 * Set perf_enable bit in L3C_PERF_CTRL register to start counting
+	 * for all enabled counters.
+	 */
+	val = readl(l3c_pmu->base + L3C_PERF_CTRL);
+	val |= L3C_PERF_CTRL_EN;
+	writel(val, l3c_pmu->base + L3C_PERF_CTRL);
+}
+
+static void hisi_l3c_pmu_stop_counters(struct hisi_pmu *l3c_pmu)
+{
+	u32 val;
+
+	/*
+	 * Clear perf_enable bit in L3C_PERF_CTRL register to stop counting
+	 * for all enabled counters.
+	 */
+	val = readl(l3c_pmu->base + L3C_PERF_CTRL);
+	val &= ~(L3C_PERF_CTRL_EN);
+	writel(val, l3c_pmu->base + L3C_PERF_CTRL);
+}
+
+static void hisi_l3c_pmu_enable_counter(struct hisi_pmu *l3c_pmu,
+					struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	/* Enable counter index in L3C_EVENT_CTRL register */
+	val = readl(l3c_pmu->base + L3C_EVENT_CTRL);
+	val |= (1 << hwc->idx);
+	writel(val, l3c_pmu->base + L3C_EVENT_CTRL);
+}
+
+static void hisi_l3c_pmu_disable_counter(struct hisi_pmu *l3c_pmu,
+					 struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	/* Clear counter index in L3C_EVENT_CTRL register */
+	val = readl(l3c_pmu->base + L3C_EVENT_CTRL);
+	val &= ~(1 << hwc->idx);
+	writel(val, l3c_pmu->base + L3C_EVENT_CTRL);
+}
+
+static void hisi_l3c_pmu_enable_counter_int(struct hisi_pmu *l3c_pmu,
+					    struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	val = readl(l3c_pmu->base + L3C_INT_MASK);
+	/* Write 0 to enable interrupt */
+	val &= ~(1 << hwc->idx);
+	writel(val, l3c_pmu->base + L3C_INT_MASK);
+}
+
+static void hisi_l3c_pmu_disable_counter_int(struct hisi_pmu *l3c_pmu,
+					     struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	val = readl(l3c_pmu->base + L3C_INT_MASK);
+	/* Write 1 to mask interrupt */
+	val |= (1 << hwc->idx);
+	writel(val, l3c_pmu->base + L3C_INT_MASK);
+}
+
+static irqreturn_t hisi_l3c_pmu_isr(int irq, void *dev_id)
+{
+	struct hisi_pmu *l3c_pmu = dev_id;
+	struct perf_event *event;
+	unsigned long overflown;
+	u32 status;
+	int idx;
+
+	/* Read L3C_INT_STATUS register */
+	status = readl(l3c_pmu->base + L3C_INT_STATUS);
+	if (!status)
+		return IRQ_NONE;
+	overflown = status;
+
+	/*
+	 * Find the counter index which overflowed if the bit was set
+	 * and handle it.
+	 */
+	for_each_set_bit(idx, &overflown, L3C_NR_COUNTERS) {
+		/* Write 1 to clear the IRQ status flag */
+		writel((1 << idx), l3c_pmu->base + L3C_INT_CLEAR);
+
+		/* Get the corresponding event struct */
+		event = l3c_pmu->pmu_events.hw_events[idx];
+		if (!event)
+			continue;
+
+		hisi_uncore_pmu_event_update(event);
+		hisi_uncore_pmu_set_event_period(event);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int hisi_l3c_pmu_init_irq(struct hisi_pmu *l3c_pmu,
+				 struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int irq, ret;
+
+	/* Read and init IRQ */
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "irq init: fail L3C overflow interrupt\n");
+		return irq;
+	}
+
+	ret = devm_request_irq(dev, irq, hisi_l3c_pmu_isr,
+			       IRQF_NOBALANCING | IRQF_NO_THREAD,
+			       dev_name(dev), l3c_pmu);
+	if (ret < 0) {
+		dev_err(dev, "Fail to request IRQ:%d ret:%d\n", irq, ret);
+		return ret;
+	}
+
+	/* Overflow interrupt also should use the same CPU */
+	WARN_ON(irq_set_affinity(irq, &l3c_pmu->cpus));
+
+	return 0;
+}
+
+/* Check if the CPU belongs to the SCCL and CCL of PMU */
+static bool hisi_l3c_is_cpu_in_ccl(struct hisi_pmu *l3c_pmu)
+{
+	u32 ccl_id, scl_id;
+
+	/* Read Super CPU cluster ID from CPU MPIDR_EL1 */
+	hisi_read_scl_and_ccl_id(&scl_id, &ccl_id);
+
+	/* Check if the CPU match with the SCCL and CCL */
+	if (scl_id != l3c_pmu->scl_id)
+		return false;
+	if (ccl_id != l3c_pmu->ccl_id)
+		return false;
+
+	/* Return true if matched */
+	return true;
+}
+
+static int hisi_l3c_pmu_online_cpu(unsigned int cpu, struct hlist_node *node)
+{
+	struct hisi_pmu *l3c_pmu;
+
+	l3c_pmu = hlist_entry_safe(node, struct hisi_pmu, node);
+
+	/* Proceed only when CPU belongs to the same SCCL and CCL */
+	if (!hisi_l3c_is_cpu_in_ccl(l3c_pmu))
+		return 0;
+
+	/* If another CPU is already managing the CPU cluster, simply return */
+	if (!cpumask_empty(&l3c_pmu->cpus))
+		return 0;
+
+	/* Use this CPU for event counting in the CCL */
+	cpumask_set_cpu(cpu, &l3c_pmu->cpus);
+
+	return 0;
+}
+
+static int hisi_l3c_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
+{
+	struct hisi_pmu *l3c_pmu;
+	cpumask_t ccl_online_cpus;
+	unsigned int target;
+
+	l3c_pmu = hlist_entry_safe(node, struct hisi_pmu, node);
+
+	/* Proceed only when CPU belongs to the same SCCL and CCL */
+	if (!hisi_l3c_is_cpu_in_ccl(l3c_pmu))
+		return 0;
+
+	/* Proceed if this CPU is used for event counting */
+	if (!cpumask_test_cpu(cpu, &l3c_pmu->cpus))
+		return 0;
+
+	/* Give up ownership of CCL */
+	cpumask_test_and_clear_cpu(cpu, &l3c_pmu->cpus);
+
+	/* Any other CPU for this CCL which is still online */
+	cpumask_and(&ccl_online_cpus, cpu_coregroup_mask(cpu),
+		    cpu_online_mask);
+	target = cpumask_any_but(&ccl_online_cpus, cpu);
+	if (target >= nr_cpu_ids)
+		return 0;
+
+	perf_pmu_migrate_context(&l3c_pmu->pmu, cpu, target);
+	/* Use this CPU for event counting in the CCL */
+	cpumask_set_cpu(target, &l3c_pmu->cpus);
+
+	return 0;
+}
+
+static const struct acpi_device_id hisi_l3c_pmu_acpi_match[] = {
+	{ "HISI0213", },
+	{},
+};
+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)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	unsigned long long id;
+	acpi_status status;
+
+	status = acpi_evaluate_integer(ACPI_HANDLE(dev), "_UID", NULL, &id);
+	if (ACPI_FAILURE(status))
+		return false;
+	l3c_pmu->l3c_tag_id = id;
+
+	/* Get the L3C SCCL ID */
+	if (device_property_read_u32(dev, "hisilicon,scl-id",
+				     &l3c_pmu->scl_id)) {
+		dev_err(dev, "Can not read l3c scl-id!\n");
+		return -EINVAL;
+	}
+
+	/* Get the L3C CPU cluster(CCL) ID */
+	if (device_property_read_u32(dev, "hisilicon,ccl-id",
+				     &l3c_pmu->ccl_id)) {
+		dev_err(dev, "Can not read l3c ccl-id!\n");
+		return -EINVAL;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	l3c_pmu->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(l3c_pmu->base)) {
+		dev_err(dev, "ioremap failed for l3c_pmu resource\n");
+		return PTR_ERR(l3c_pmu->base);
+	}
+
+	return 0;
+}
+
+static struct attribute *hisi_l3c_pmu_format_attr[] = {
+	HISI_PMU_FORMAT_ATTR(event, "config:0-7"),
+	NULL,
+};
+
+static const struct attribute_group hisi_l3c_pmu_format_group = {
+	.name = "format",
+	.attrs = hisi_l3c_pmu_format_attr,
+};
+
+static struct attribute *hisi_l3c_pmu_events_attr[] = {
+	HISI_PMU_EVENT_ATTR(rd_cpipe,		0x00),
+	HISI_PMU_EVENT_ATTR(wr_cpipe,		0x01),
+	HISI_PMU_EVENT_ATTR(rd_hit_cpipe,	0x02),
+	HISI_PMU_EVENT_ATTR(wr_hit_cpipe,	0x03),
+	HISI_PMU_EVENT_ATTR(victim_num,		0x04),
+	HISI_PMU_EVENT_ATTR(rd_spipe,		0x20),
+	HISI_PMU_EVENT_ATTR(wr_spipe,		0x21),
+	HISI_PMU_EVENT_ATTR(rd_hit_spipe,	0x22),
+	HISI_PMU_EVENT_ATTR(wr_hit_spipe,	0x23),
+	HISI_PMU_EVENT_ATTR(back_invalid,	0x29),
+	HISI_PMU_EVENT_ATTR(retry_cpu,		0x40),
+	HISI_PMU_EVENT_ATTR(retry_ring,		0x41),
+	HISI_PMU_EVENT_ATTR(prefetch_drop,	0x42),
+	NULL,
+};
+
+static const struct attribute_group hisi_l3c_pmu_events_group = {
+	.name = "events",
+	.attrs = hisi_l3c_pmu_events_attr,
+};
+
+static struct attribute *hisi_l3c_pmu_attrs[] = {
+	NULL,
+};
+
+static const struct attribute_group hisi_l3c_pmu_attr_group = {
+	.attrs = hisi_l3c_pmu_attrs,
+};
+
+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 const struct attribute_group *hisi_l3c_pmu_attr_groups[] = {
+	&hisi_l3c_pmu_attr_group,
+	&hisi_l3c_pmu_format_group,
+	&hisi_l3c_pmu_events_group,
+	&hisi_l3c_pmu_cpumask_attr_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,
+	.start_counters		= hisi_l3c_pmu_start_counters,
+	.stop_counters		= hisi_l3c_pmu_stop_counters,
+	.enable_counter		= hisi_l3c_pmu_enable_counter,
+	.disable_counter	= hisi_l3c_pmu_disable_counter,
+	.enable_counter_int	= hisi_l3c_pmu_enable_counter_int,
+	.disable_counter_int	= hisi_l3c_pmu_disable_counter_int,
+	.write_counter		= hisi_l3c_pmu_write_counter,
+	.read_counter		= hisi_l3c_pmu_read_counter,
+};
+
+static int hisi_l3c_pmu_dev_probe(struct platform_device *pdev,
+				  struct hisi_pmu *l3c_pmu)
+{
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	ret = hisi_l3c_pmu_init_data(pdev, l3c_pmu);
+	if (ret)
+		return ret;
+
+	ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
+				       &l3c_pmu->node);
+	if (ret) {
+		dev_err(dev, "Error %d registering hotplug", ret);
+		return ret;
+	}
+
+	ret = hisi_l3c_pmu_init_irq(l3c_pmu, pdev);
+	if (ret)
+		goto fail;
+
+	l3c_pmu->name = devm_kasprintf(dev, GFP_KERNEL, "hisi_l3c%u_%u",
+				       l3c_pmu->l3c_tag_id, l3c_pmu->scl_id);
+	l3c_pmu->num_events = L3C_NR_EVENTS;
+	l3c_pmu->num_counters = L3C_NR_COUNTERS;
+	l3c_pmu->counter_bits = 48;
+	l3c_pmu->ops = &hisi_uncore_l3c_ops;
+	l3c_pmu->dev = dev;
+
+	return 0;
+
+fail:
+	cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
+				    &l3c_pmu->node);
+	return ret;
+}
+
+static int hisi_l3c_pmu_probe(struct platform_device *pdev)
+{
+	struct hisi_pmu *l3c_pmu;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	l3c_pmu = hisi_pmu_alloc(dev, L3C_NR_COUNTERS);
+	if (!l3c_pmu)
+		return -ENOMEM;
+
+	ret = hisi_l3c_pmu_dev_probe(pdev, l3c_pmu);
+	if (ret)
+		return ret;
+
+	l3c_pmu->pmu = (struct pmu) {
+		.name		= l3c_pmu->name,
+		.task_ctx_nr	= perf_invalid_context,
+		.event_init	= hisi_uncore_pmu_event_init,
+		.pmu_enable	= hisi_uncore_pmu_enable,
+		.pmu_disable	= hisi_uncore_pmu_disable,
+		.add		= hisi_uncore_pmu_add,
+		.del		= hisi_uncore_pmu_del,
+		.start		= hisi_uncore_pmu_start,
+		.stop		= hisi_uncore_pmu_stop,
+		.read		= hisi_uncore_pmu_read,
+		.attr_groups	= hisi_l3c_pmu_attr_groups,
+	};
+
+	ret = hisi_uncore_pmu_setup(l3c_pmu, l3c_pmu->name);
+	if (ret) {
+		dev_err(l3c_pmu->dev, "hisi_uncore_pmu_setup failed!\n");
+		cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
+					    &l3c_pmu->node);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, l3c_pmu);
+
+	return 0;
+}
+
+static int hisi_l3c_pmu_remove(struct platform_device *pdev)
+{
+	struct hisi_pmu *l3c_pmu = platform_get_drvdata(pdev);
+
+	perf_pmu_unregister(&l3c_pmu->pmu);
+	cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
+				    &l3c_pmu->node);
+
+	return 0;
+}
+
+static struct platform_driver hisi_l3c_pmu_driver = {
+	.driver = {
+		.name = "hisi_l3c_pmu",
+		.acpi_match_table = ACPI_PTR(hisi_l3c_pmu_acpi_match),
+	},
+	.probe = hisi_l3c_pmu_probe,
+	.remove = hisi_l3c_pmu_remove,
+};
+
+static int __init hisi_l3c_pmu_module_init(void)
+{
+	int ret;
+
+	ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
+				      "AP_PERF_ARM_HISI_L3_ONLINE",
+				      hisi_l3c_pmu_online_cpu,
+				      hisi_l3c_pmu_offline_cpu);
+	if (ret) {
+		pr_err("l3c_pmu_module_init: Error setup hotplug, ret=%d", ret);
+		return ret;
+	}
+
+	ret = platform_driver_register(&hisi_l3c_pmu_driver);
+	if (ret)
+		cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE);
+
+	return ret;
+}
+module_init(hisi_l3c_pmu_module_init);
+
+static void __exit hisi_l3c_pmu_module_exit(void)
+{
+	cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE);
+	platform_driver_unregister(&hisi_l3c_pmu_driver);
+}
+module_exit(hisi_l3c_pmu_module_exit);
+
+MODULE_DESCRIPTION("HiSilicon SoC L3C uncore PMU driver");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Anurup M, Shaokun Zhang");
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index b56573b..2eab157 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -136,6 +136,7 @@ enum cpuhp_state {
 	CPUHP_AP_PERF_S390_SF_ONLINE,
 	CPUHP_AP_PERF_ARM_CCI_ONLINE,
 	CPUHP_AP_PERF_ARM_CCN_ONLINE,
+	CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
 	CPUHP_AP_PERF_ARM_L2X0_ONLINE,
 	CPUHP_AP_PERF_ARM_QCOM_L2_ONLINE,
 	CPUHP_AP_PERF_ARM_QCOM_L3_ONLINE,
-- 
1.9.1

[PATCH v3 4/6] perf: hisi: Add support for HiSilicon SoC HHA PMU driver

[Shaokun Zhang]

L3 cache coherence is maintained by Hydra Home Agent (HHA) in HiSilicon
SoC. This patch adds support for HHA PMU driver, Each HHA has own
control, counter and interrupt registers and is an separate PMU. For
each HHA PMU, it has 16-programable counters and supports 0x50 events,
event code is 8-bits and every counter is free-running. Interrupt is
supported to handle counter (48-bits) overflow.

Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
Signed-off-by: Anurup M <anurup.m@huawei.com>
---
 drivers/perf/hisilicon/Makefile              |   2 +-
 drivers/perf/hisilicon/hisi_uncore_hha_pmu.c | 453 +++++++++++++++++++++++++++
 2 files changed, 454 insertions(+), 1 deletion(-)
 create mode 100644 drivers/perf/hisilicon/hisi_uncore_hha_pmu.c

diff --git a/drivers/perf/hisilicon/Makefile b/drivers/perf/hisilicon/Makefile
index 4a3d3e6..a72afe8 100644
--- a/drivers/perf/hisilicon/Makefile
+++ b/drivers/perf/hisilicon/Makefile
@@ -1 +1 @@
-obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o hisi_uncore_l3c_pmu.o
+obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o hisi_uncore_l3c_pmu.o hisi_uncore_hha_pmu.o
diff --git a/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
new file mode 100644
index 0000000..f56dced
--- /dev/null
+++ b/drivers/perf/hisilicon/hisi_uncore_hha_pmu.c
@@ -0,0 +1,453 @@
+/*
+ * HiSilicon SoC HHA uncore Hardware event counters support
+ *
+ * Copyright (C) 2017 Hisilicon Limited
+ * Author: Shaokun Zhang <zhangshaokun@hisilicon.com>
+ *         Anurup M <anurup.m@huawei.com>
+ *
+ * This code is based on the uncore PMUs like arm-cci and arm-ccn.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/acpi.h>
+#include <linux/bitmap.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/perf_event.h>
+#include "hisi_uncore_pmu.h"
+
+/* HHA register definition */
+#define HHA_INT_MASK		0x0804
+#define HHA_INT_STATUS		0x0808
+#define HHA_INT_CLEAR		0x080C
+#define HHA_PERF_CTRL		0x1E00
+#define HHA_EVENT_CTRL		0x1E04
+#define HHA_EVENT_TYPE0		0x1E80
+#define HHA_CNT0_LOWER		0x1F00
+
+/* HHA has 16-counters and supports 0x50 events */
+#define HHA_NR_COUNTERS		0x10
+#define HHA_NR_EVENTS		0x50
+
+#define HHA_PERF_CTRL_EN	0x1
+#define HHA_EVTYPE_NONE		0xff
+
+#define HHA_EVTYPE_REG(idx) (HHA_EVENT_TYPE0 + (idx <= 3 ? 0 : 4 * \
+			    (idx / 4)))
+
+/*
+ * Select the counter register offset using the counter index
+ * every counter is 48-bits and [48:63] is reserved.
+ */
+static u32 get_counter_reg_off(int cntr_idx)
+{
+	return (HHA_CNT0_LOWER + (cntr_idx * 8));
+}
+
+static u64 hisi_hha_pmu_read_counter(struct hisi_pmu *hha_pmu,
+				     struct hw_perf_event *hwc)
+{
+	u32 idx = hwc->idx;
+	u32 reg;
+
+	if (!hisi_uncore_pmu_counter_valid(hha_pmu, idx)) {
+		dev_err(hha_pmu->dev, "Unsupported event index:%d!\n", idx);
+		return 0;
+	}
+
+	reg = get_counter_reg_off(idx);
+
+	/* Read 64 bits and like L3C, top 16 bits are RAZ */
+	return readq(hha_pmu->base + reg);
+}
+
+static void hisi_hha_pmu_write_counter(struct hisi_pmu *hha_pmu,
+				       struct hw_perf_event *hwc, u64 val)
+{
+	u32 idx = hwc->idx;
+	u32 reg;
+
+	if (!hisi_uncore_pmu_counter_valid(hha_pmu, idx)) {
+		dev_err(hha_pmu->dev, "Unsupported event index:%d!\n", idx);
+		return;
+	}
+
+	reg = get_counter_reg_off(idx);
+	/* Write 64 bits and like L3C, top 16 bits are WI */
+	writeq(val, hha_pmu->base + reg);
+}
+
+static void hisi_hha_pmu_write_evtype(struct hisi_pmu *hha_pmu, int idx,
+				      u32 type)
+{
+	u32 reg, reg_idx, shift, val;
+
+	/*
+	 * Select the appropriate Event select register(HHA_EVENT_TYPEx).
+	 * There are 4 event select registers for the 16 hardware counters.
+	 * Event code is 8-bits and for the first 4 hardware counters,
+	 * HHA_EVENT_TYPE0 is chosen. For the next 4 hardware counters,
+	 * HHA_EVENT_TYPE1 is chosen and so on.
+	 */
+	reg = HHA_EVTYPE_REG(idx);
+	reg_idx = idx % 4;
+	shift = 8 * reg_idx;
+
+	/* Write event code to HHA_EVENT_TYPEx register */
+	val = readl(hha_pmu->base + reg);
+	val &= ~(HHA_EVTYPE_NONE << shift);
+	val |= (type << shift);
+	writel(val, hha_pmu->base + reg);
+}
+
+static void hisi_hha_pmu_start_counters(struct hisi_pmu *hha_pmu)
+{
+	u32 val;
+
+	/*
+	 * Set perf_enable bit in HHA_PERF_CTRL to start event
+	 * counting for all enabled counters.
+	 */
+	val = readl(hha_pmu->base + HHA_PERF_CTRL);
+	val |= HHA_PERF_CTRL_EN;
+	writel(val, hha_pmu->base + HHA_PERF_CTRL);
+}
+
+static void hisi_hha_pmu_stop_counters(struct hisi_pmu *hha_pmu)
+{
+	u32 val;
+
+	/*
+	 * Clear perf_enable bit in HHA_PERF_CTRL to stop event
+	 * counting for all enabled counters.
+	 */
+	val = readl(hha_pmu->base + HHA_PERF_CTRL);
+	val &= ~(HHA_PERF_CTRL_EN);
+	writel(val, hha_pmu->base + HHA_PERF_CTRL);
+}
+
+static void hisi_hha_pmu_enable_counter(struct hisi_pmu *hha_pmu,
+					struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	/* Enable counter index in HHA_EVENT_CTRL register */
+	val = readl(hha_pmu->base + HHA_EVENT_CTRL);
+	val |= (1 << hwc->idx);
+	writel(val, hha_pmu->base + HHA_EVENT_CTRL);
+}
+
+static void hisi_hha_pmu_disable_counter(struct hisi_pmu *hha_pmu,
+					 struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	/* Clear counter index in HHA_EVENT_CTRL register */
+	val = readl(hha_pmu->base + HHA_EVENT_CTRL);
+	val &= ~(1 << hwc->idx);
+	writel(val, hha_pmu->base + HHA_EVENT_CTRL);
+}
+
+static void hisi_hha_pmu_enable_counter_int(struct hisi_pmu *hha_pmu,
+					    struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	/* Write 0 to enable interrupt */
+	val = readl(hha_pmu->base + HHA_INT_MASK);
+	val &= ~(1 << hwc->idx);
+	writel(val, hha_pmu->base + HHA_INT_MASK);
+}
+
+static void hisi_hha_pmu_disable_counter_int(struct hisi_pmu *hha_pmu,
+					     struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	/* Write 1 to mask interrupt */
+	val = readl(hha_pmu->base + HHA_INT_MASK);
+	val |= (1 << hwc->idx);
+	writel(val, hha_pmu->base + HHA_INT_MASK);
+}
+
+static irqreturn_t hisi_hha_pmu_isr(int irq, void *dev_id)
+{
+	struct hisi_pmu *hha_pmu = dev_id;
+	struct perf_event *event;
+	unsigned long overflown;
+	u32 status;
+	int idx;
+
+	/* Read HHA_INT_STATUS register */
+	status = readl(hha_pmu->base + HHA_INT_STATUS);
+	if (!status)
+		return IRQ_NONE;
+	overflown = status;
+
+	/*
+	 * Find the counter index which overflowed if the bit was set
+	 * and handle it
+	 */
+	for_each_set_bit(idx, &overflown, HHA_NR_COUNTERS) {
+		/* Write 1 to clear the IRQ status flag */
+		writel((1 << idx), hha_pmu->base + HHA_INT_CLEAR);
+
+		/* Get the corresponding event struct */
+		event = hha_pmu->pmu_events.hw_events[idx];
+		if (!event)
+			continue;
+
+		hisi_uncore_pmu_event_update(event);
+		hisi_uncore_pmu_set_event_period(event);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int hisi_hha_pmu_init_irq(struct hisi_pmu *hha_pmu,
+				 struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int irq, ret;
+
+	/* Read and init IRQ */
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "irq init: fail map HHA overflow interrupt\n");
+		return -ENOENT;
+	}
+
+	ret = devm_request_irq(dev, irq, hisi_hha_pmu_isr,
+			      IRQF_NOBALANCING | IRQF_NO_THREAD,
+			      dev_name(dev), hha_pmu);
+	if (ret < 0) {
+		dev_err(dev, "Fail to request IRQ:%d ret:%d\n", irq, ret);
+		return ret;
+	}
+
+	/* Overflow interrupt also should use the same CPU */
+	WARN_ON(irq_set_affinity(irq, &hha_pmu->cpus));
+
+	return 0;
+}
+
+static const struct acpi_device_id hisi_hha_pmu_acpi_match[] = {
+	{ "HISI0243", },
+	{},
+};
+MODULE_DEVICE_TABLE(acpi, hisi_hha_pmu_acpi_match);
+
+static int hisi_hha_pmu_init_data(struct platform_device *pdev,
+				  struct hisi_pmu *hha_pmu)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	unsigned long long uid;
+	acpi_status status;
+
+	status = acpi_evaluate_integer(ACPI_HANDLE(dev), "_UID", NULL, &uid);
+	if (ACPI_FAILURE(status))
+		return false;
+	hha_pmu->hha_uid = uid;
+
+	/* Get the HHA SCCL ID */
+	if (device_property_read_u32(dev, "hisilicon,scl-id",
+				     &hha_pmu->scl_id)) {
+		dev_err(dev, "Can not read hha scl-id!\n");
+		return -EINVAL;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	hha_pmu->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(hha_pmu->base)) {
+		dev_err(dev, "ioremap failed for hha_pmu resource\n");
+		return PTR_ERR(hha_pmu->base);
+	}
+
+	return 0;
+}
+
+static struct attribute *hisi_hha_pmu_format_attr[] = {
+	HISI_PMU_FORMAT_ATTR(event, "config:0-7"),
+	NULL,
+};
+
+static const struct attribute_group hisi_hha_pmu_format_group = {
+	.name = "format",
+	.attrs = hisi_hha_pmu_format_attr,
+};
+
+static struct attribute *hisi_hha_pmu_events_attr[] = {
+	HISI_PMU_EVENT_ATTR(rx_ops_num,		0x00),
+	HISI_PMU_EVENT_ATTR(rx_outer,		0x01),
+	HISI_PMU_EVENT_ATTR(rx_sccl,		0x02),
+	HISI_PMU_EVENT_ATTR(rx_ccix,		0x03),
+	HISI_PMU_EVENT_ATTR(rx_wbi,		0x04),
+	HISI_PMU_EVENT_ATTR(rx_wbip,		0x05),
+	HISI_PMU_EVENT_ATTR(rx_wtistash,	0x11),
+	HISI_PMU_EVENT_ATTR(rd_ddr_64b,		0x1c),
+	HISI_PMU_EVENT_ATTR(wr_dr_64b,		0x1d),
+	HISI_PMU_EVENT_ATTR(rd_ddr_128b,	0x1e),
+	HISI_PMU_EVENT_ATTR(wr_ddr_128b,	0x1f),
+	HISI_PMU_EVENT_ATTR(spill_num,		0x20),
+	HISI_PMU_EVENT_ATTR(spill_success,	0x21),
+	HISI_PMU_EVENT_ATTR(bi_num,		0x23),
+	HISI_PMU_EVENT_ATTR(mediated_num,	0x32),
+	HISI_PMU_EVENT_ATTR(tx_snp_num,		0x33),
+	HISI_PMU_EVENT_ATTR(tx_snp_outer,	0x34),
+	HISI_PMU_EVENT_ATTR(tx_snp_ccix,	0x35),
+	HISI_PMU_EVENT_ATTR(rx_snprspdata,	0x38),
+	HISI_PMU_EVENT_ATTR(rx_snprsp_outer,	0x3c),
+	HISI_PMU_EVENT_ATTR(sdir-lookup,	0x40),
+	HISI_PMU_EVENT_ATTR(edir-lookup,	0x41),
+	HISI_PMU_EVENT_ATTR(sdir-hit,		0x42),
+	HISI_PMU_EVENT_ATTR(edir-hit,		0x43),
+	HISI_PMU_EVENT_ATTR(sdir-home-migrate,	0x4c),
+	HISI_PMU_EVENT_ATTR(edir-home-migrate,  0x4d),
+	NULL,
+};
+
+static const struct attribute_group hisi_hha_pmu_events_group = {
+	.name = "events",
+	.attrs = hisi_hha_pmu_events_attr,
+};
+
+static struct attribute *hisi_hha_pmu_attrs[] = {
+	NULL,
+};
+
+static const struct attribute_group hisi_hha_pmu_attr_group = {
+	.attrs = hisi_hha_pmu_attrs,
+};
+
+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 const struct attribute_group *hisi_hha_pmu_attr_groups[] = {
+	&hisi_hha_pmu_attr_group,
+	&hisi_hha_pmu_format_group,
+	&hisi_hha_pmu_events_group,
+	&hisi_hha_pmu_cpumask_attr_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,
+	.start_counters		= hisi_hha_pmu_start_counters,
+	.stop_counters		= hisi_hha_pmu_stop_counters,
+	.enable_counter		= hisi_hha_pmu_enable_counter,
+	.disable_counter	= hisi_hha_pmu_disable_counter,
+	.enable_counter_int	= hisi_hha_pmu_enable_counter_int,
+	.disable_counter_int	= hisi_hha_pmu_disable_counter_int,
+	.write_counter		= hisi_hha_pmu_write_counter,
+	.read_counter		= hisi_hha_pmu_read_counter,
+};
+
+static int hisi_hha_pmu_dev_probe(struct platform_device *pdev,
+				  struct hisi_pmu *hha_pmu)
+{
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	ret = hisi_hha_pmu_init_data(pdev, hha_pmu);
+	if (ret)
+		return ret;
+
+	/* Pick one core to use for cpumask attributes */
+	cpumask_set_cpu(smp_processor_id(), &hha_pmu->cpus);
+
+	ret = hisi_hha_pmu_init_irq(hha_pmu, pdev);
+	if (ret)
+		return ret;
+
+	hha_pmu->name = devm_kasprintf(dev, GFP_KERNEL, "hisi_hha%u_%u",
+				       hha_pmu->hha_uid, hha_pmu->scl_id);
+	hha_pmu->num_events = HHA_NR_EVENTS;
+	hha_pmu->num_counters = HHA_NR_COUNTERS;
+	hha_pmu->counter_bits = 48;
+	hha_pmu->ops = &hisi_uncore_hha_ops;
+	hha_pmu->dev = dev;
+
+	return 0;
+}
+
+static int hisi_hha_pmu_probe(struct platform_device *pdev)
+{
+	struct hisi_pmu *hha_pmu;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	hha_pmu = hisi_pmu_alloc(dev, HHA_NR_COUNTERS);
+	if (!hha_pmu)
+		return -ENOMEM;
+
+	ret = hisi_hha_pmu_dev_probe(pdev, hha_pmu);
+	if (ret)
+		return ret;
+
+	hha_pmu->pmu = (struct pmu) {
+		.name		= hha_pmu->name,
+		.task_ctx_nr	= perf_invalid_context,
+		.event_init	= hisi_uncore_pmu_event_init,
+		.pmu_enable	= hisi_uncore_pmu_enable,
+		.pmu_disable	= hisi_uncore_pmu_disable,
+		.add		= hisi_uncore_pmu_add,
+		.del		= hisi_uncore_pmu_del,
+		.start		= hisi_uncore_pmu_start,
+		.stop		= hisi_uncore_pmu_stop,
+		.read		= hisi_uncore_pmu_read,
+		.attr_groups	= hisi_hha_pmu_attr_groups,
+	};
+
+	ret = hisi_uncore_pmu_setup(hha_pmu, hha_pmu->name);
+	if (ret) {
+		dev_err(hha_pmu->dev, "hisi_uncore_pmu_setup failed!\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, hha_pmu);
+
+	return 0;
+}
+
+static int hisi_hha_pmu_remove(struct platform_device *pdev)
+{
+	struct hisi_pmu *hha_pmu = platform_get_drvdata(pdev);
+
+	perf_pmu_unregister(&hha_pmu->pmu);
+
+	return 0;
+}
+
+static struct platform_driver hha_pmu_driver = {
+	.driver = {
+		.name = "hisi_hha_pmu",
+		.acpi_match_table = ACPI_PTR(hisi_hha_pmu_acpi_match),
+	},
+	.probe = hisi_hha_pmu_probe,
+	.remove = hisi_hha_pmu_remove,
+};
+module_platform_driver(hha_pmu_driver);
+
+MODULE_DESCRIPTION("HiSilicon SoC HHA uncore PMU driver");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Shaokun Zhang, Anurup M");
-- 
1.9.1

[PATCH v3 5/6] perf: hisi: Add support for HiSilicon SoC DDRC PMU driver

[Shaokun Zhang]

This patch adds support for DDRC PMU driver in HiSilicon SoC chip, Each
DDRC has own control, counter and interrupt registers and is an separate
PMU. For each DDRC PMU, it has 8-fixed-purpose counters which have been
mapped to 8-events by hardware, it assumes that counter index is equal
to event code (0 - 7) in DDRC PMU driver. Interrupt is supported to
handle counter (32-bits) overflow.

Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
Signed-off-by: Anurup M <anurup.m@huawei.com>
---
 drivers/perf/hisilicon/Makefile               |   2 +-
 drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c | 437 ++++++++++++++++++++++++++
 2 files changed, 438 insertions(+), 1 deletion(-)
 create mode 100644 drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c

diff --git a/drivers/perf/hisilicon/Makefile b/drivers/perf/hisilicon/Makefile
index a72afe8..2621d51 100644
--- a/drivers/perf/hisilicon/Makefile
+++ b/drivers/perf/hisilicon/Makefile
@@ -1 +1 @@
-obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o hisi_uncore_l3c_pmu.o hisi_uncore_hha_pmu.o
+obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o hisi_uncore_l3c_pmu.o hisi_uncore_hha_pmu.o hisi_uncore_ddrc_pmu.o
diff --git a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
new file mode 100644
index 0000000..f362835
--- /dev/null
+++ b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
@@ -0,0 +1,437 @@
+/*
+ * HiSilicon SoC DDRC uncore Hardware event counters support
+ *
+ * Copyright (C) 2017 Hisilicon Limited
+ * Author: Shaokun Zhang <zhangshaokun@hisilicon.com>
+ *         Anurup M <anurup.m@huawei.com>
+ *
+ * This code is based on the uncore PMUs like arm-cci and arm-ccn.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/acpi.h>
+#include <linux/bitmap.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/perf_event.h>
+#include "hisi_uncore_pmu.h"
+
+/* DDRC register definition */
+#define DDRC_PERF_CTRL		0x010
+#define DDRC_FLUX_WR		0x380
+#define DDRC_FLUX_RD		0x384
+#define DDRC_FLUX_WCMD          0x388
+#define DDRC_FLUX_RCMD          0x38c
+#define DDRC_PRE_CMD            0x3c0
+#define DDRC_ACT_CMD            0x3c4
+#define DDRC_BNK_CHG            0x3c8
+#define DDRC_RNK_CHG            0x3cc
+#define DDRC_EVENT_CTRL         0x6C0
+#define DDRC_INT_MASK		0x6c8
+#define DDRC_INT_STATUS		0x6cc
+#define DDRC_INT_CLEAR		0x6d0
+
+/* DDRC supports 8-events and counter is fixed-purpose */
+#define DDRC_NR_COUNTERS	0x8
+#define DDRC_NR_EVENTS		DDRC_NR_COUNTERS
+
+#define DDRC_PERF_CTRL_EN	0x2
+
+/*
+ * For DDRC PMU, there are eight-events and every event has been mapped
+ * to fixed-purpose counters which register offset is not consistent.
+ * Therefore there is no write event type and we assume that event
+ * code (0 to 7) is equal to counter index in PMU driver.
+ */
+#define GET_DDRC_EVENTID(hwc)	(hwc->config_base & 0x7)
+
+static const u32 ddrc_reg_off[] = {
+	DDRC_FLUX_WR, DDRC_FLUX_RD, DDRC_FLUX_WCMD, DDRC_FLUX_RCMD,
+	DDRC_PRE_CMD, DDRC_ACT_CMD, DDRC_BNK_CHG, DDRC_RNK_CHG
+};
+
+/*
+ * Select the counter register offset using the counter index.
+ * In DDRC there are no programmable counter, the count
+ * is readed form the statistics counter register itself.
+ */
+static u32 get_counter_reg_off(int cntr_idx)
+{
+	return ddrc_reg_off[cntr_idx];
+}
+
+static u64 hisi_ddrc_pmu_read_counter(struct hisi_pmu *ddrc_pmu,
+				      struct hw_perf_event *hwc)
+{
+	/* Use event code as counter index */
+	u32 idx = GET_DDRC_EVENTID(hwc);
+	u32 reg;
+
+	if (!hisi_uncore_pmu_counter_valid(ddrc_pmu, idx)) {
+		dev_err(ddrc_pmu->dev, "Unsupported event index:%d!\n", idx);
+		return 0;
+	}
+
+	reg = get_counter_reg_off(idx);
+
+	return readl(ddrc_pmu->base + reg);
+}
+
+static void hisi_ddrc_pmu_write_counter(struct hisi_pmu *ddrc_pmu,
+					struct hw_perf_event *hwc, u64 val)
+{
+	u32 idx = GET_DDRC_EVENTID(hwc);
+	u32 reg;
+
+	if (!hisi_uncore_pmu_counter_valid(ddrc_pmu, idx)) {
+		dev_err(ddrc_pmu->dev, "Unsupported event index:%d!\n", idx);
+		return;
+	}
+
+	reg = get_counter_reg_off(idx);
+	writel((u32)val, ddrc_pmu->base + reg);
+}
+
+static void hisi_ddrc_pmu_start_counters(struct hisi_pmu *ddrc_pmu)
+{
+	u32 val;
+
+	/* Set perf_enable in DDRC_PERF_CTRL to start event counting */
+	val = readl(ddrc_pmu->base + DDRC_PERF_CTRL);
+	val |= DDRC_PERF_CTRL_EN;
+	writel(val, ddrc_pmu->base + DDRC_PERF_CTRL);
+}
+
+static void hisi_ddrc_pmu_stop_counters(struct hisi_pmu *ddrc_pmu)
+{
+	u32 val;
+
+	/* Clear perf_enable in DDRC_PERF_CTRL to stop event counting */
+	val = readl(ddrc_pmu->base + DDRC_PERF_CTRL);
+	val &= ~DDRC_PERF_CTRL_EN;
+	writel(val, ddrc_pmu->base + DDRC_PERF_CTRL);
+}
+
+static void hisi_ddrc_pmu_enable_counter(struct hisi_pmu *ddrc_pmu,
+					 struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	/* Set counter index(event code) in DDRC_EVENT_CTRL register */
+	val = readl(ddrc_pmu->base + DDRC_EVENT_CTRL);
+	val |= (1 << GET_DDRC_EVENTID(hwc));
+	writel(val, ddrc_pmu->base + DDRC_EVENT_CTRL);
+}
+
+static void hisi_ddrc_pmu_disable_counter(struct hisi_pmu *ddrc_pmu,
+					  struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	/* Clear counter index(event code) in DDRC_EVENT_CTRL register */
+	val = readl(ddrc_pmu->base + DDRC_EVENT_CTRL);
+	val &= ~(1 << GET_DDRC_EVENTID(hwc));
+	writel(val, ddrc_pmu->base + DDRC_EVENT_CTRL);
+}
+
+static int hisi_ddrc_pmu_get_event_idx(struct perf_event *event)
+{
+	struct hisi_pmu *ddrc_pmu = to_hisi_pmu(event->pmu);
+	unsigned long *used_mask = ddrc_pmu->pmu_events.used_mask;
+	struct hw_perf_event *hwc = &event->hw;
+	/* For DDRC PMU, we use event code as counter index */
+	int idx = GET_DDRC_EVENTID(hwc);
+
+	if (test_bit(idx, used_mask))
+		return -EAGAIN;
+
+	set_bit(idx, used_mask);
+
+	return idx;
+}
+
+static void hisi_ddrc_pmu_enable_counter_int(struct hisi_pmu *ddrc_pmu,
+					     struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	/* Write 0 to enable interrupt */
+	val = readl(ddrc_pmu->base + DDRC_INT_MASK);
+	val &= ~(1 << GET_DDRC_EVENTID(hwc));
+	writel(val, ddrc_pmu->base + DDRC_INT_MASK);
+}
+
+static void hisi_ddrc_pmu_disable_counter_int(struct hisi_pmu *ddrc_pmu,
+					      struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	/* Write 1 to mask interrupt */
+	val = readl(ddrc_pmu->base + DDRC_INT_MASK);
+	val |= (1 << GET_DDRC_EVENTID(hwc));
+	writel(val, ddrc_pmu->base + DDRC_INT_MASK);
+}
+
+static irqreturn_t hisi_ddrc_pmu_isr(int irq, void *dev_id)
+{
+	struct hisi_pmu *ddrc_pmu = dev_id;
+	struct perf_event *event;
+	unsigned long overflown;
+	u32 status;
+	int idx;
+
+	/* Read the DDRC_INT_STATUS register */
+	status = readl(ddrc_pmu->base + DDRC_INT_STATUS);
+	if (!status)
+		return IRQ_NONE;
+	overflown = status;
+
+	/*
+	 * Find the counter index which overflowed if the bit was set
+	 * and handle it
+	 */
+	for_each_set_bit(idx, &overflown, DDRC_NR_COUNTERS) {
+		/* Write 1 to clear the IRQ status flag */
+		writel((1 << idx), ddrc_pmu->base + DDRC_INT_CLEAR);
+
+		/* Get the corresponding event struct */
+		event = ddrc_pmu->pmu_events.hw_events[idx];
+		if (!event)
+			continue;
+
+		hisi_uncore_pmu_event_update(event);
+		hisi_uncore_pmu_set_event_period(event);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int hisi_ddrc_pmu_init_irq(struct hisi_pmu *ddrc_pmu,
+				  struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int irq, ret;
+
+	/* Read and init IRQ */
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "irq init: fail map DDRC overflow interrupt\n");
+		return irq;
+	}
+
+	ret = devm_request_irq(dev, irq, hisi_ddrc_pmu_isr,
+			       IRQF_NOBALANCING | IRQF_NO_THREAD,
+			       dev_name(dev), ddrc_pmu);
+	if (ret < 0) {
+		dev_err(dev, "Fail to request IRQ:%d ret:%d\n", irq, ret);
+		return ret;
+	}
+
+	/* Overflow interrupt also should use the same CPU */
+	WARN_ON(irq_set_affinity(irq, &ddrc_pmu->cpus));
+
+	return 0;
+}
+
+static const struct acpi_device_id hisi_ddrc_pmu_acpi_match[] = {
+	{ "HISI0233", },
+	{},
+};
+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)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+
+	/* Get the DDRC Channel ID */
+	if (device_property_read_u32(dev, "hisilicon,ch-id",
+				     &ddrc_pmu->ddrc_chn_id)) {
+		dev_err(dev, "Can not read ddrc ch-id!\n");
+		return -EINVAL;
+	}
+
+	/* Get the DDRC SCCL ID */
+	if (device_property_read_u32(dev, "hisilicon,scl-id",
+				     &ddrc_pmu->scl_id)) {
+		dev_err(dev, "Can not read ddrc scl-id!\n");
+		return -EINVAL;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ddrc_pmu->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(ddrc_pmu->base)) {
+		dev_err(dev, "ioremap failed for ddrc_pmu resource\n");
+		return PTR_ERR(ddrc_pmu->base);
+	}
+
+	return 0;
+}
+
+static struct attribute *hisi_ddrc_pmu_format_attr[] = {
+	HISI_PMU_FORMAT_ATTR(event, "config:0-4"),
+	NULL,
+};
+
+static const struct attribute_group hisi_ddrc_pmu_format_group = {
+	.name = "format",
+	.attrs = hisi_ddrc_pmu_format_attr,
+};
+
+static struct attribute *hisi_ddrc_pmu_events_attr[] = {
+	HISI_PMU_EVENT_ATTR(flux_wr,		0x00),
+	HISI_PMU_EVENT_ATTR(flux_rd,		0x01),
+	HISI_PMU_EVENT_ATTR(flux_wcmd,		0x02),
+	HISI_PMU_EVENT_ATTR(flux_rcmd,		0x03),
+	HISI_PMU_EVENT_ATTR(pre_cmd,		0x04),
+	HISI_PMU_EVENT_ATTR(act_cmd,		0x05),
+	HISI_PMU_EVENT_ATTR(rnk_chg,		0x06),
+	HISI_PMU_EVENT_ATTR(rw_chg,		0x07),
+	NULL,
+};
+
+static const struct attribute_group hisi_ddrc_pmu_events_group = {
+	.name = "events",
+	.attrs = hisi_ddrc_pmu_events_attr,
+};
+
+static struct attribute *hisi_ddrc_pmu_attrs[] = {
+	NULL,
+};
+
+static const struct attribute_group hisi_ddrc_pmu_attr_group = {
+	.attrs = hisi_ddrc_pmu_attrs,
+};
+
+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 const struct attribute_group *hisi_ddrc_pmu_attr_groups[] = {
+	&hisi_ddrc_pmu_attr_group,
+	&hisi_ddrc_pmu_format_group,
+	&hisi_ddrc_pmu_events_group,
+	&hisi_ddrc_pmu_cpumask_attr_group,
+	NULL,
+};
+
+static const struct hisi_uncore_ops hisi_uncore_ddrc_ops = {
+	.get_event_idx		= hisi_ddrc_pmu_get_event_idx,
+	.start_counters		= hisi_ddrc_pmu_start_counters,
+	.stop_counters		= hisi_ddrc_pmu_stop_counters,
+	.enable_counter		= hisi_ddrc_pmu_enable_counter,
+	.disable_counter	= hisi_ddrc_pmu_disable_counter,
+	.enable_counter_int	= hisi_ddrc_pmu_enable_counter_int,
+	.disable_counter_int	= hisi_ddrc_pmu_disable_counter_int,
+	.write_counter		= hisi_ddrc_pmu_write_counter,
+	.read_counter		= hisi_ddrc_pmu_read_counter,
+};
+
+static int hisi_ddrc_pmu_dev_probe(struct platform_device *pdev,
+				   struct hisi_pmu *ddrc_pmu)
+{
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	ret = hisi_ddrc_pmu_init_data(pdev, ddrc_pmu);
+	if (ret)
+		return ret;
+
+	/* Pick one core to use for cpumask attributes */
+	cpumask_set_cpu(smp_processor_id(), &ddrc_pmu->cpus);
+
+	ret = hisi_ddrc_pmu_init_irq(ddrc_pmu, pdev);
+	if (ret)
+		return ret;
+
+	ddrc_pmu->name = devm_kasprintf(dev, GFP_KERNEL, "hisi_ddrc%u_%u",
+					ddrc_pmu->ddrc_chn_id,
+					ddrc_pmu->scl_id);
+	ddrc_pmu->num_events = DDRC_NR_EVENTS;
+	ddrc_pmu->num_counters = DDRC_NR_COUNTERS;
+	ddrc_pmu->counter_bits = 32;
+	ddrc_pmu->ops = &hisi_uncore_ddrc_ops;
+	ddrc_pmu->dev = dev;
+
+	return 0;
+}
+
+static int hisi_ddrc_pmu_probe(struct platform_device *pdev)
+{
+	struct hisi_pmu *ddrc_pmu;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	ddrc_pmu = hisi_pmu_alloc(dev, DDRC_NR_COUNTERS);
+	if (!ddrc_pmu)
+		return -ENOMEM;
+
+	ret = hisi_ddrc_pmu_dev_probe(pdev, ddrc_pmu);
+	if (ret)
+		return ret;
+
+	ddrc_pmu->pmu = (struct pmu) {
+		.name		= ddrc_pmu->name,
+		.task_ctx_nr	= perf_invalid_context,
+		.event_init	= hisi_uncore_pmu_event_init,
+		.pmu_enable	= hisi_uncore_pmu_enable,
+		.pmu_disable	= hisi_uncore_pmu_disable,
+		.add		= hisi_uncore_pmu_add,
+		.del		= hisi_uncore_pmu_del,
+		.start		= hisi_uncore_pmu_start,
+		.stop		= hisi_uncore_pmu_stop,
+		.read		= hisi_uncore_pmu_read,
+		.attr_groups	= hisi_ddrc_pmu_attr_groups,
+	};
+
+	ret = hisi_uncore_pmu_setup(ddrc_pmu, ddrc_pmu->name);
+	if (ret) {
+		dev_err(ddrc_pmu->dev, "hisi_uncore_pmu_setup failed!\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, ddrc_pmu);
+
+	return 0;
+}
+
+static int hisi_ddrc_pmu_remove(struct platform_device *pdev)
+{
+	struct hisi_pmu *ddrc_pmu = platform_get_drvdata(pdev);
+
+	perf_pmu_unregister(&ddrc_pmu->pmu);
+
+	return 0;
+}
+
+static struct platform_driver hisi_ddrc_pmu_driver = {
+	.driver = {
+		.name = "hisi_ddrc_pmu",
+		.acpi_match_table = ACPI_PTR(hisi_ddrc_pmu_acpi_match),
+	},
+	.probe = hisi_ddrc_pmu_probe,
+	.remove = hisi_ddrc_pmu_remove,
+};
+module_platform_driver(hisi_ddrc_pmu_driver);
+
+MODULE_DESCRIPTION("HiSilicon SoC DDRC uncore PMU driver");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Shaokun Zhang, Anurup M");
-- 
1.9.1

[PATCH v3 6/6] arm64: MAINTAINERS: hisi: Add HiSilicon SoC PMU support

[Shaokun Zhang]

Add support HiSilicon SoC uncore PMU driver.

Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 205d397..649b144 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6197,6 +6197,13 @@ S:	Maintained
 F:	drivers/net/ethernet/hisilicon/
 F:	Documentation/devicetree/bindings/net/hisilicon*.txt
 
+HISILICON PMU DRIVER
+M:	Shaokun Zhang <zhangshaokun@hisilicon.com>
+W:	http://www.hisilicon.com
+S:	Supported
+F:	drivers/perf/hisilicon
+F:	Documentation/perf/hisi-pmu.txt
+
 HISILICON ROCE DRIVER
 M:	Lijun Ou <oulijun@huawei.com>
 M:	Wei Hu(Xavier) <xavier.huwei@huawei.com>
-- 
1.9.1

Re: [PATCH v3 1/6] Documentation: perf: hisi: Documentation for HiSilicon SoC PMU driver

[Jonathan Cameron]

On Tue, 18 Jul 2017 15:59:54 +0800
Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:

> This patch adds documentation for the uncore PMUs on HiSilicon SoC.
> 
> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> Signed-off-by: Anurup M <anurup.m@huawei.com>  
Hi Shaokun,

Sorry for the late reply on this (only recently joined Huawei)

This is a fairly generic review of the code rather than going into
the actual userspace ABI choices as this is an area I'm only just
starting to become familiar with.

Thanks,

Jonathan
> ---
>  Documentation/perf/hisi-pmu.txt | 51 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>  create mode 100644 Documentation/perf/hisi-pmu.txt
> 
> diff --git a/Documentation/perf/hisi-pmu.txt b/Documentation/perf/hisi-pmu.txt
> new file mode 100644
> index 0000000..5fa0b1a
> --- /dev/null
> +++ b/Documentation/perf/hisi-pmu.txt
> @@ -0,0 +1,51 @@
> +HiSilicon SoC uncore Performance Monitoring Unit (PMU)
> +======================================================
> +The HiSilicon SoC chip comprehends various independent system device PMUs
> +such as L3 cache (L3C), Hydra Home Agent (HHA) and DDRC. These PMUs are
> +independent and have hardware logic to gather statistics and performance
> +information.
> +
> +HiSilicon SoC encapsulates multiple CPU and IO dies. Each CPU cluster (CC
> +L) is made up of 4 cpu cores sharing one L3 cache; Each CPU die is called  
nitpick, I'd not have a line break mid acronym. 

> +Super CPU cluster (SCCL) and is made up of 6 CCLs. Each SCCL has two HHAs
> +(0 - 1) and four DDRCs (0 - 3), respectively.
> +
> +HiSilicon SoC uncore PMU driver
> +---------------------------------------
> +Each device PMU has separate registers for event counting, control and
> +interrupt, and the PMU driver shall register perf PMU drivers like L3C,
> +HHA and DDRC etc. The available events and configuration options shall
> +be described in the sysfs, see /sys/devices/hisi_*.  
Is there not a subsystem directory that would make more sense to
refer to than the full device list?

> +The "perf list" command shall list the available events from sysfs.
> +
> +Each L3C, HHA and DDRC in one SCCL are registered as an separate PMU with perf.
> +The PMU name will appear in event listing as hisi_module <index-id>_<sccl-id>.
> +where "index-id" is the index of module and "sccl-id" is the identifier of
> +the SCCL.
> +e.g. hisi_l3c0_1/rd_hit_cpipe is READ_HIT_CPIPE event of L3C index #0 and SCCL
> +ID #1.
> +e.g. hisi_hha0_1/rx_operations is RX_OPERATIONS event of HHA index #0 and SCCL
> +ID #1.
> +
> +The driver also provides a "cpumask" sysfs attribute, which shows the CPU core
> +ID used to count the uncore PMU event.
> +
> +Example usage of perf:
> +$# perf list
> +hisi_l3c0_3/rd_hit_cpipe/ [kernel PMU event]
> +------------------------------------------
> +hisi_l3c0_3/wr_hit_cpipe/ [kernel PMU event]
> +------------------------------------------
> +hisi_l3c0_1/rd_hit_cpipe/ [kernel PMU event]
> +------------------------------------------
> +hisi_l3c0_1/wr_hit_cpipe/ [kernel PMU event]
> +------------------------------------------
> +
> +$# perf stat -a -e hisi_l3c0_1/rd_hit_cpipe/ sleep 5
> +$# perf stat -a -e hisi_l3c0_1/config=0x02/ sleep 5
> +
> +The current driver does not support sampling. So "perf record" is unsupported.
> +Also attach to a task is unsupported as the events are all uncore.
> +
> +Note: Please contact the maintainer for a complete list of events supported for
> +the PMU devices in the SoC and its information if needed.  


_______________________________________________
linuxarm mailing list

Re: [PATCH v3 2/6] perf: hisi: Add support for HiSilicon SoC uncore PMU driver

[Jonathan Cameron]

On Tue, 18 Jul 2017 15:59:55 +0800
Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:

> This patch adds support HiSilicon SoC uncore PMU driver framework and
> interfaces.
> 
> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> Signed-off-by: Anurup M <anurup.m@huawei.com>  
A couple of minor things inline.

Jonathan
> ---
>  drivers/perf/Kconfig                     |   7 +
>  drivers/perf/Makefile                    |   1 +
>  drivers/perf/hisilicon/Makefile          |   1 +
>  drivers/perf/hisilicon/hisi_uncore_pmu.c | 411 +++++++++++++++++++++++++++++++
>  drivers/perf/hisilicon/hisi_uncore_pmu.h | 116 +++++++++
>  5 files changed, 536 insertions(+)
>  create mode 100644 drivers/perf/hisilicon/Makefile
>  create mode 100644 drivers/perf/hisilicon/hisi_uncore_pmu.c
>  create mode 100644 drivers/perf/hisilicon/hisi_uncore_pmu.h
> 
> diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> index e5197ff..78fc4bc 100644
> --- a/drivers/perf/Kconfig
> +++ b/drivers/perf/Kconfig
> @@ -17,6 +17,13 @@ config ARM_PMU_ACPI
>  	depends on ARM_PMU && ACPI
>  	def_bool y
>  
> +config HISI_PMU
> +	bool "HiSilicon SoC PMU"
> +	depends on ARM64 && ACPI
> +	help
> +	  Support for HiSilicon SoC uncore performance monitoring
> +	  unit (PMU), such as: L3C, HHA and DDRC.
> +
>  config QCOM_L2_PMU
>  	bool "Qualcomm Technologies L2-cache PMU"
>  	depends on ARCH_QCOM && ARM64 && ACPI
> diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
> index 6420bd4..41d3342 100644
> --- a/drivers/perf/Makefile
> +++ b/drivers/perf/Makefile
> @@ -1,5 +1,6 @@
>  obj-$(CONFIG_ARM_PMU) += arm_pmu.o arm_pmu_platform.o
>  obj-$(CONFIG_ARM_PMU_ACPI) += arm_pmu_acpi.o
> +obj-$(CONFIG_HISI_PMU) += hisilicon/
>  obj-$(CONFIG_QCOM_L2_PMU)	+= qcom_l2_pmu.o
>  obj-$(CONFIG_QCOM_L3_PMU) += qcom_l3_pmu.o
>  obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
> diff --git a/drivers/perf/hisilicon/Makefile b/drivers/perf/hisilicon/Makefile
> new file mode 100644
> index 0000000..2783bb3
> --- /dev/null
> +++ b/drivers/perf/hisilicon/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o
> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
> new file mode 100644
> index 0000000..4eb04e1
> --- /dev/null
> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
> @@ -0,0 +1,411 @@
> +/*
> + * HiSilicon SoC Hardware event counters support
> + *
> + * Copyright (C) 2017 Hisilicon Limited
> + * Author: Anurup M <anurup.m@huawei.com>
> + *         Shaokun Zhang <zhangshaokun@hisilicon.com>
> + *
> + * This code is based on the uncore PMUs like arm-cci and arm-ccn.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/bitmap.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/perf_event.h>
> +#include "hisi_uncore_pmu.h"
> +
> +/*
> + * PMU format attributes
> + */
> +ssize_t hisi_format_sysfs_show(struct device *dev,
> +			       struct device_attribute *attr, char *buf)
> +{
> +	struct dev_ext_attribute *eattr;
> +
> +	eattr = container_of(attr, struct dev_ext_attribute, attr);
> +
> +	return sprintf(buf, "%s\n", (char *)eattr->var);
> +}
> +
> +/*
> + * PMU event attributes
> + */
> +ssize_t hisi_event_sysfs_show(struct device *dev,
> +			      struct device_attribute *attr, char *page)
> +{
> +	struct dev_ext_attribute *eattr;
> +
> +	eattr = container_of(attr, struct dev_ext_attribute, attr);
> +
> +	return sprintf(page, "config=0x%lx\n", (unsigned long)eattr->var);
> +}
> +
> +/*
> + * sysfs cpumask attributes
> + */
> +ssize_t hisi_cpumask_sysfs_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct pmu *pmu = dev_get_drvdata(dev);
> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(pmu);
> +
> +	return cpumap_print_to_pagebuf(true, buf, &hisi_pmu->cpus);
> +}
> +
> +/* Read Super CPU cluster and CPU cluster ID from MPIDR_EL1 */
> +void hisi_read_scl_and_ccl_id(u32 *scl_id, u32 *ccl_id)
> +{
> +	u64 mpidr;
> +
> +	mpidr = read_cpuid_mpidr();
> +	if (mpidr & MPIDR_MT_BITMASK) {
> +		if (scl_id)
> +			*scl_id = MPIDR_AFFINITY_LEVEL(mpidr, 3);
> +		if (ccl_id)
> +			*ccl_id = MPIDR_AFFINITY_LEVEL(mpidr, 2);
> +	} else {
> +		if (scl_id)
> +			*scl_id = MPIDR_AFFINITY_LEVEL(mpidr, 2);
> +		if (ccl_id)
> +			*ccl_id = MPIDR_AFFINITY_LEVEL(mpidr, 1);
> +	}
> +}
> +
> +static bool hisi_validate_event_group(struct perf_event *event)
> +{
> +	struct perf_event *sibling, *leader = event->group_leader;
> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
> +	/* Include count for the event */
> +	int counters = 1;
> +
> +	/*
> +	 * We must NOT create groups containing mixed PMUs, although
> +	 * software events are acceptable
> +	 */
> +	if (leader->pmu != event->pmu && !is_software_event(leader))
> +		return false;
> +
> +	/* Increment counter for the leader */
> +	counters++;
> +
> +	list_for_each_entry(sibling, &event->group_leader->sibling_list,
> +			    group_entry) {
> +		if (is_software_event(sibling))
> +			continue;
> +		if (sibling->pmu != event->pmu)
> +			return false;
> +		/* Increment counter for each sibling */
> +		counters++;
> +	}
> +
> +	/* The group can not count events more than the counters in the HW */
> +	return counters <= hisi_pmu->num_counters;
> +}
> +
> +int hisi_uncore_pmu_counter_valid(struct hisi_pmu *hisi_pmu, int idx)
> +{
> +	return (idx >= 0 && idx < hisi_pmu->num_counters);
> +}
> +
> +int hisi_uncore_pmu_get_event_idx(struct perf_event *event)
> +{
> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
> +	unsigned long *used_mask = hisi_pmu->pmu_events.used_mask;
> +	u32 num_counters = hisi_pmu->num_counters;
> +	int idx;
> +
> +	idx = find_first_zero_bit(used_mask, num_counters);
> +	if (idx == num_counters)
> +		return -EAGAIN;
> +
> +	set_bit(idx, used_mask);
> +
> +	return idx;
> +}
> +
> +void hisi_uncore_pmu_clear_event_idx(struct hisi_pmu *hisi_pmu, int idx)
> +{
> +	if (!hisi_uncore_pmu_counter_valid(hisi_pmu, idx)) {
> +		dev_err(hisi_pmu->dev, "Unsupported event index:%d!\n", idx);
> +		return;
> +	}
> +
> +	clear_bit(idx, hisi_pmu->pmu_events.used_mask);
> +}
> +
> +int hisi_uncore_pmu_event_init(struct perf_event *event)
> +{
> +	struct hisi_pmu *hisi_pmu;
> +	struct hw_perf_event *hwc = &event->hw;
> +	int cpu;
> +
> +	if (event->attr.type != event->pmu->type)
> +		return -ENOENT;
> +
> +	/*
> +	 * We do not support sampling as the counters are all
> +	 * shared by all CPU cores in a CPU die(SCCL). Also we
> +	 * do not support attach to a task(per-process mode)
> +	 */
> +	if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK)
> +		return -EOPNOTSUPP;
> +
> +	/* counters do not have these bits */
> +	if (event->attr.exclude_user	||
> +	    event->attr.exclude_kernel	||
> +	    event->attr.exclude_host	||
> +	    event->attr.exclude_guest	||
> +	    event->attr.exclude_hv	||
> +	    event->attr.exclude_idle)
> +		return -EINVAL;
> +
> +	/*
> +	 *  The uncore counters not specific to any CPU, so cannot
> +	 *  support per-task
> +	 */
> +	if (event->cpu < 0)
> +		return -EINVAL;
> +
> +	/*
> +	 * Validate if the events in group does not exceed the
> +	 * available counters in hardware.
> +	 */
> +	if (!hisi_validate_event_group(event))
> +		return -EINVAL;
> +
> +	/*
> +	 * We don't assign an index until we actually place the event onto
> +	 * hardware. Use -1 to signify that we haven't decided where to put it
> +	 * yet.
> +	 */
> +	hwc->idx		= -1;
> +	hwc->config_base	= event->attr.config;
> +
> +	/* Select an available CPU to monitor events in this PMU */
> +	hisi_pmu = to_hisi_pmu(event->pmu);
> +	cpu = cpumask_first(&hisi_pmu->cpus);
> +	if (cpu >= nr_cpu_ids)
> +		return -EINVAL;
> +
> +	/* Enforce to use the same CPU for all events in this PMU */
> +	event->cpu = cpu;
> +
> +	return 0;
> +}
> +
> +/*
> + * Set the counter to count the event that we're interested in,
> + * and enable counter and interrupt.
> + */
> +static void hisi_uncore_pmu_enable_event(struct perf_event *event)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
> +
> +	/*
> +	 * Write event code in event select registers(for DDRC PMU,
> +	 * event has been mapped to fixed-purpose counter, there is
> +	 * no need to write event type).
> +	 */
> +	if (hisi_pmu->ops->write_evtype)
> +		hisi_pmu->ops->write_evtype(hisi_pmu, hwc->idx,
> +					    GET_EVENTID(event));
> +
> +	/* Enable the hardware event counting and interrupt */
> +	hisi_pmu->ops->enable_counter(hisi_pmu, hwc);
> +	hisi_pmu->ops->enable_counter_int(hisi_pmu, hwc);
> +}
> +
> +/*
> + * Disable counting and interrupt.
> + */
> +static void hisi_uncore_pmu_disable_event(struct perf_event *event)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
> +
> +	hisi_pmu->ops->disable_counter(hisi_pmu, hwc);
> +	hisi_pmu->ops->disable_counter_int(hisi_pmu, hwc);
> +}
> +
> +void hisi_uncore_pmu_set_event_period(struct perf_event *event)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
> +
> +	/*
> +	 * The HiSilicon PMU counters support 32 bits or 48 bits, depending on
> +	 * the PMU. We reduce it to 2^(counter_bits - 1) to account for the
> +	 * extreme interrupt latency. So we could hopefully handle the overflow
> +	 * interrupt before another 2^(counter_bits - 1) events occur and the
> +	 * counter overtakes its previous value.
> +	 */
> +	u64 val = BIT_ULL(hisi_pmu->counter_bits - 1);
> +
> +	local64_set(&hwc->prev_count, val);
> +	/* Write start value to the hardware event counter */
> +	hisi_pmu->ops->write_counter(hisi_pmu, hwc, val);
> +}
> +
> +u64 hisi_uncore_pmu_event_update(struct perf_event *event)
> +{
> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	u64 delta, prev_raw_count, new_raw_count;
> +
> +	do {
> +		/* Read the count from the counter register */
> +		new_raw_count = hisi_pmu->ops->read_counter(hisi_pmu, hwc);
> +		prev_raw_count = local64_read(&hwc->prev_count);
> +	} while (local64_cmpxchg(&hwc->prev_count, prev_raw_count,
> +				 new_raw_count) != prev_raw_count);
> +	/*
> +	 * compute the delta
> +	 */
> +	delta = (new_raw_count - prev_raw_count) &
> +		HISI_MAX_PERIOD(hisi_pmu->counter_bits);
> +	local64_add(delta, &event->count);
> +
> +	return new_raw_count;
> +}
> +
> +void hisi_uncore_pmu_start(struct perf_event *event, int flags)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
> +
> +	if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED)))
> +		return;
> +
> +	WARN_ON_ONCE(!(hwc->state & PERF_HES_UPTODATE));
> +	hwc->state = 0;
> +	hisi_uncore_pmu_set_event_period(event);
> +
> +	if (flags & PERF_EF_RELOAD) {
> +		u64 prev_raw_count =  local64_read(&hwc->prev_count);
> +
> +		hisi_pmu->ops->write_counter(hisi_pmu, hwc, prev_raw_count);
> +	}
> +
> +	hisi_uncore_pmu_enable_event(event);
> +	perf_event_update_userpage(event);
> +}
> +
> +void hisi_uncore_pmu_stop(struct perf_event *event, int flags)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +
> +	hisi_uncore_pmu_disable_event(event);
> +	WARN_ON_ONCE(hwc->state & PERF_HES_STOPPED);
> +	hwc->state |= PERF_HES_STOPPED;
> +
> +	if (hwc->state & PERF_HES_UPTODATE)
> +		return;
> +
> +	/* Read hardware counter and update the Perf counter statistics */
> +	hisi_uncore_pmu_event_update(event);
> +	hwc->state |= PERF_HES_UPTODATE;
> +}
> +
> +int hisi_uncore_pmu_add(struct perf_event *event, int flags)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
> +	int idx;
> +
> +	hwc->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
> +
> +	/* Get an available counter index for counting */
> +	idx = hisi_pmu->ops->get_event_idx(event);
> +	if (idx < 0)
> +		return -EAGAIN;
> +
> +	event->hw.idx = idx;
> +	hisi_pmu->pmu_events.hw_events[idx] = event;
> +
> +	if (flags & PERF_EF_START)
> +		hisi_uncore_pmu_start(event, PERF_EF_RELOAD);
> +
> +	return 0;
> +}
> +
> +void hisi_uncore_pmu_del(struct perf_event *event, int flags)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
> +
> +	hisi_uncore_pmu_stop(event, PERF_EF_UPDATE);
> +	hisi_uncore_pmu_clear_event_idx(hisi_pmu, hwc->idx);
> +	perf_event_update_userpage(event);
> +	hisi_pmu->pmu_events.hw_events[hwc->idx] = NULL;
> +}
> +
> +struct hisi_pmu *hisi_pmu_alloc(struct device *dev, u32 num_cntrs)
> +{
> +	struct hisi_pmu *hisi_pmu;
> +	struct hisi_pmu_hwevents *pmu_events;
> +
> +	hisi_pmu = devm_kzalloc(dev, sizeof(*hisi_pmu), GFP_KERNEL);
> +	if (!hisi_pmu)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pmu_events = &hisi_pmu->pmu_events;
> +	pmu_events->hw_events = devm_kcalloc(dev,
> +					     num_cntrs,
> +					     sizeof(*pmu_events->hw_events),
> +					     GFP_KERNEL);
> +	if (!pmu_events->hw_events)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pmu_events->used_mask = devm_kcalloc(dev,
> +					     BITS_TO_LONGS(num_cntrs),
> +					     sizeof(*pmu_events->used_mask),
> +					     GFP_KERNEL);
> +	if (!pmu_events->used_mask)
> +		return ERR_PTR(-ENOMEM);
> +
> +	return 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);
> +}
> +
> +void hisi_uncore_pmu_enable(struct pmu *pmu)
> +{
> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(pmu);
> +	int enabled = bitmap_weight(hisi_pmu->pmu_events.used_mask,
> +				    hisi_pmu->num_counters);
> +
> +	if (!enabled)
> +		return;
> +
> +	hisi_pmu->ops->start_counters(hisi_pmu);
> +}
> +
> +void hisi_uncore_pmu_disable(struct pmu *pmu)
> +{
> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(pmu);
> +
> +	hisi_pmu->ops->stop_counters(hisi_pmu);
> +}
> +
> +int hisi_uncore_pmu_setup(struct hisi_pmu *hisi_pmu, const char *pmu_name)
> +{
> +	/* Register the events with perf */
> +	return perf_pmu_register(&hisi_pmu->pmu, pmu_name, -1);
> +}  
This seems oddly unbalanced given you then do the
perf_pmu_unregister() calls directly in the various drivers.

So I'd push the contents of this function down into the drivers
themselves.

> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h
> new file mode 100644
> index 0000000..eb2abb0
> --- /dev/null
> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
> @@ -0,0 +1,116 @@
> +/*
> + * HiSilicon SoC Hardware event counters support
> + *
> + * Copyright (C) 2017 Hisilicon Limited
> + * Author: Anurup M <anurup.m@huawei.com>
> + *         Shaokun Zhang <zhangshaokun@hisilicon.com>
> + *
> + * This code is based on the uncore PMUs like arm-cci and arm-ccn.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#ifndef __HISI_UNCORE_PMU_H__
> +#define __HISI_UNCORE_PMU_H__
> +
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/ktime.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <asm/local64.h>  
This is a wide range of headers to have included from
an include. At least some of these should be pushed down into the c files.

> +
> +#undef pr_fmt
> +#define pr_fmt(fmt)     "hisi_pmu: " fmt
> +
> +#define GET_EVENTID(ev)   (ev->hw.config_base & 0xff)  
I'd add a prefix to this as chance a clash in future for
something with that name seems very high.

It is also only used in hisi_uncore_pmu.c so I'd push it down into
that file.

> +#define to_hisi_pmu(p)	(container_of(p, struct hisi_pmu, pmu))
> +#define HISI_MAX_PERIOD(nr) (BIT_ULL(nr) - 1)  
Only used in one file, so I'd push it down into the c file.
> +
> +#define HISI_PMU_ATTR(_name, _func, _config)				\
> +	(&((struct dev_ext_attribute[]) {				\
> +		{ __ATTR(_name, 0444, _func, NULL), (void *)_config }   \
> +	})[0].attr.attr)
> +
> +#define HISI_PMU_FORMAT_ATTR(_name, _config)		\
> +	HISI_PMU_ATTR(_name, hisi_format_sysfs_show, (void *)_config)
> +#define HISI_PMU_EVENT_ATTR(_name, _config)		\
> +	HISI_PMU_ATTR(_name, hisi_event_sysfs_show, (unsigned long)_config)
> +
> +struct hisi_pmu;
> +
> +struct hisi_uncore_ops {
> +	void (*write_evtype)(struct hisi_pmu *, int, u32);
> +	int (*get_event_idx)(struct perf_event *);
> +	u64 (*read_counter)(struct hisi_pmu *, struct hw_perf_event *);
> +	void (*write_counter)(struct hisi_pmu *, struct hw_perf_event *, u64);
> +	void (*enable_counter)(struct hisi_pmu *, struct hw_perf_event *);
> +	void (*disable_counter)(struct hisi_pmu *, struct hw_perf_event *);
> +	void (*enable_counter_int)(struct hisi_pmu *, struct hw_perf_event *);
> +	void (*disable_counter_int)(struct hisi_pmu *, struct hw_perf_event *);
> +	void (*start_counters)(struct hisi_pmu *);
> +	void (*stop_counters)(struct hisi_pmu *);
> +};
> +
> +struct hisi_pmu_hwevents {
> +	struct perf_event **hw_events;
> +	unsigned long *used_mask;
> +};
> +
> +/* Generic pmu struct for different pmu types */
> +struct hisi_pmu {
> +	const char *name;
> +	struct pmu pmu;
> +	const struct hisi_uncore_ops *ops;
> +	struct hisi_pmu_hwevents pmu_events;
> +	cpumask_t cpus;
> +	struct device *dev;
> +	struct hlist_node node;
> +	u32 scl_id;
> +	u32 ccl_id;
> +	/* Hardware information for different pmu types */
> +	void __iomem *base;
> +	union {
> +		u32 ddrc_chn_id;
> +		u32 l3c_tag_id;
> +		u32 hha_uid;
> +	};
> +	int num_counters;
> +	int num_events;
> +	int counter_bits;
> +};
> +
> +int hisi_uncore_pmu_counter_valid(struct hisi_pmu *hisi_pmu, int idx);
> +int hisi_uncore_pmu_get_event_idx(struct perf_event *event);
> +void hisi_uncore_pmu_clear_event_idx(struct hisi_pmu *hisi_pmu, int idx);  
The above is only used in hisi_uncore_pmu.c so doesn't need to be here
and can be static.

> +void hisi_uncore_pmu_read(struct perf_event *event);
> +int hisi_uncore_pmu_add(struct perf_event *event, int flags);
> +void hisi_uncore_pmu_del(struct perf_event *event, int flags);
> +void hisi_uncore_pmu_start(struct perf_event *event, int flags);
> +void hisi_uncore_pmu_stop(struct perf_event *event, int flags);
> +void hisi_uncore_pmu_set_event_period(struct perf_event *event);
> +u64 hisi_uncore_pmu_event_update(struct perf_event *event);
> +int hisi_uncore_pmu_event_init(struct perf_event *event);
> +int hisi_uncore_pmu_setup(struct hisi_pmu *hisi_pmu, const char *pmu_name);
> +void hisi_uncore_pmu_enable(struct pmu *pmu);
> +void hisi_uncore_pmu_disable(struct pmu *pmu);
> +struct hisi_pmu *hisi_pmu_alloc(struct device *dev, u32 num_cntrs);
> +ssize_t hisi_event_sysfs_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf);
> +ssize_t hisi_format_sysfs_show(struct device *dev,
> +			       struct device_attribute *attr, char *buf);
> +ssize_t hisi_cpumask_sysfs_show(struct device *dev,
> +				struct device_attribute *attr, char *buf);
> +void hisi_read_scl_and_ccl_id(u32 *scl_id, u32 *ccl_id);
> +#endif /* __HISI_UNCORE_PMU_H__ */  

Re: [PATCH v3 3/6] perf: hisi: Add support for HiSilicon SoC L3C PMU driver

[Jonathan Cameron]

On Tue, 18 Jul 2017 15:59:56 +0800
Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:

> This patch adds support for L3C PMU driver in HiSilicon SoC chip, Each
> L3C has own control, counter and interrupt registers and is an separate
> PMU. For each L3C PMU, it has 8-programable counters and supports 0x60
> events, event code is 8-bits and every counter is free-running.
> Interrupt is supported to handle counter (48-bits) overflow.
> 
> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> Signed-off-by: Anurup M <anurup.m@huawei.com>  
Hi Shaokun,

A few minor points inline.

Thanks,

Jonathan
> ---
>  drivers/perf/hisilicon/Makefile              |   2 +-
>  drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c | 556 +++++++++++++++++++++++++++
>  include/linux/cpuhotplug.h                   |   1 +
>  3 files changed, 558 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
> 
> diff --git a/drivers/perf/hisilicon/Makefile b/drivers/perf/hisilicon/Makefile
> index 2783bb3..4a3d3e6 100644
> --- a/drivers/perf/hisilicon/Makefile
> +++ b/drivers/perf/hisilicon/Makefile
> @@ -1 +1 @@
> -obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o
> +obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o hisi_uncore_l3c_pmu.o
> diff --git a/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
> new file mode 100644
> index 0000000..d430508
> --- /dev/null
> +++ b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
> @@ -0,0 +1,556 @@
> +/*
> + * HiSilicon SoC L3C uncore Hardware event counters support
> + *
> + * Copyright (C) 2017 Hisilicon Limited
> + * Author: Anurup M <anurup.m@huawei.com>
> + *         Shaokun Zhang <zhangshaokun@hisilicon.com>
> + *
> + * This code is based on the uncore PMUs like arm-cci and arm-ccn.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
Missed this on previous but putting full gpl text in is not normally done any
more. 
> + */
> +#include <linux/acpi.h>
> +#include <linux/bitmap.h>  
Not immediately seeing any bitmaps in use in here.
(I may well have missed something though!)

> +#include <linux/cpuhotplug.h>
> +#include <linux/module.h>
> +#include <linux/perf_event.h>
> +#include <linux/topology.h>
> +#include "hisi_uncore_pmu.h"
<snip>
> +
> +/* Check if the CPU belongs to the SCCL and CCL of PMU */
> +static bool hisi_l3c_is_cpu_in_ccl(struct hisi_pmu *l3c_pmu)
> +{
> +	u32 ccl_id, scl_id;
> +
> +	/* Read Super CPU cluster ID from CPU MPIDR_EL1 */
> +	hisi_read_scl_and_ccl_id(&scl_id, &ccl_id);
> +
> +	/* Check if the CPU match with the SCCL and CCL */
> +	if (scl_id != l3c_pmu->scl_id)
> +		return false;
> +	if (ccl_id != l3c_pmu->ccl_id)
> +		return false;
> +
> +	/* Return true if matched */  
I think the name of the function makes this clear enough to
not need a comment here, but up to you.
> +	return true;
> +}
> +
> +static int hisi_l3c_pmu_online_cpu(unsigned int cpu, struct hlist_node *node)
> +{
> +	struct hisi_pmu *l3c_pmu;
> +
> +	l3c_pmu = hlist_entry_safe(node, struct hisi_pmu, node);
> +
> +	/* Proceed only when CPU belongs to the same SCCL and CCL */
> +	if (!hisi_l3c_is_cpu_in_ccl(l3c_pmu))
> +		return 0;
> +
> +	/* If another CPU is already managing the CPU cluster, simply return */
> +	if (!cpumask_empty(&l3c_pmu->cpus))
> +		return 0;
> +
> +	/* Use this CPU for event counting in the CCL */
> +	cpumask_set_cpu(cpu, &l3c_pmu->cpus);
> +
> +	return 0;
> +}
> +
> +static int hisi_l3c_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
> +{
> +	struct hisi_pmu *l3c_pmu;
> +	cpumask_t ccl_online_cpus;
> +	unsigned int target;
> +
> +	l3c_pmu = hlist_entry_safe(node, struct hisi_pmu, node);
> +
> +	/* Proceed only when CPU belongs to the same SCCL and CCL */
> +	if (!hisi_l3c_is_cpu_in_ccl(l3c_pmu))
> +		return 0;
> +
> +	/* Proceed if this CPU is used for event counting */
> +	if (!cpumask_test_cpu(cpu, &l3c_pmu->cpus))
> +		return 0;
> +
> +	/* Give up ownership of CCL */
> +	cpumask_test_and_clear_cpu(cpu, &l3c_pmu->cpus);
> +
> +	/* Any other CPU for this CCL which is still online */
> +	cpumask_and(&ccl_online_cpus, cpu_coregroup_mask(cpu),
> +		    cpu_online_mask);
> +	target = cpumask_any_but(&ccl_online_cpus, cpu);
> +	if (target >= nr_cpu_ids)
> +		return 0;
> +
> +	perf_pmu_migrate_context(&l3c_pmu->pmu, cpu, target);
> +	/* Use this CPU for event counting in the CCL */
> +	cpumask_set_cpu(target, &l3c_pmu->cpus);
> +
> +	return 0;
> +}
> +
> +static const struct acpi_device_id hisi_l3c_pmu_acpi_match[] = {
> +	{ "HISI0213", },
> +	{},
> +};
> +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)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	unsigned long long id;
> +	acpi_status status;
> +
> +	status = acpi_evaluate_integer(ACPI_HANDLE(dev), "_UID", NULL, &id);
> +	if (ACPI_FAILURE(status))
> +		return false;  
Nitpick: A blank line here would slightly aid readability.
> +	l3c_pmu->l3c_tag_id = id;
> +
> +	/* Get the L3C SCCL ID */
> +	if (device_property_read_u32(dev, "hisilicon,scl-id",
> +				     &l3c_pmu->scl_id)) {
> +		dev_err(dev, "Can not read l3c scl-id!\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Get the L3C CPU cluster(CCL) ID */
> +	if (device_property_read_u32(dev, "hisilicon,ccl-id",
> +				     &l3c_pmu->ccl_id)) {
> +		dev_err(dev, "Can not read l3c ccl-id!\n");
> +		return -EINVAL;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	l3c_pmu->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(l3c_pmu->base)) {
> +		dev_err(dev, "ioremap failed for l3c_pmu resource\n");
> +		return PTR_ERR(l3c_pmu->base);
> +	}
> +
> +	return 0;
> +}
> +
> +static struct attribute *hisi_l3c_pmu_format_attr[] = {
> +	HISI_PMU_FORMAT_ATTR(event, "config:0-7"),
> +	NULL,
> +};
> +
> +static const struct attribute_group hisi_l3c_pmu_format_group = {
> +	.name = "format",
> +	.attrs = hisi_l3c_pmu_format_attr,
> +};
> +
> +static struct attribute *hisi_l3c_pmu_events_attr[] = {
> +	HISI_PMU_EVENT_ATTR(rd_cpipe,		0x00),
> +	HISI_PMU_EVENT_ATTR(wr_cpipe,		0x01),
> +	HISI_PMU_EVENT_ATTR(rd_hit_cpipe,	0x02),
> +	HISI_PMU_EVENT_ATTR(wr_hit_cpipe,	0x03),
> +	HISI_PMU_EVENT_ATTR(victim_num,		0x04),
> +	HISI_PMU_EVENT_ATTR(rd_spipe,		0x20),
> +	HISI_PMU_EVENT_ATTR(wr_spipe,		0x21),
> +	HISI_PMU_EVENT_ATTR(rd_hit_spipe,	0x22),
> +	HISI_PMU_EVENT_ATTR(wr_hit_spipe,	0x23),
> +	HISI_PMU_EVENT_ATTR(back_invalid,	0x29),
> +	HISI_PMU_EVENT_ATTR(retry_cpu,		0x40),
> +	HISI_PMU_EVENT_ATTR(retry_ring,		0x41),
> +	HISI_PMU_EVENT_ATTR(prefetch_drop,	0x42),
> +	NULL,
> +};
> +
> +static const struct attribute_group hisi_l3c_pmu_events_group = {
> +	.name = "events",
> +	.attrs = hisi_l3c_pmu_events_attr,
> +};
> +
> +static struct attribute *hisi_l3c_pmu_attrs[] = {
> +	NULL,
> +};  
Why have the empty group? 
> +
> +static const struct attribute_group hisi_l3c_pmu_attr_group = {
> +	.attrs = hisi_l3c_pmu_attrs,
> +};
> +
> +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 const struct attribute_group *hisi_l3c_pmu_attr_groups[] = {
> +	&hisi_l3c_pmu_attr_group,
> +	&hisi_l3c_pmu_format_group,
> +	&hisi_l3c_pmu_events_group,
> +	&hisi_l3c_pmu_cpumask_attr_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,
> +	.start_counters		= hisi_l3c_pmu_start_counters,
> +	.stop_counters		= hisi_l3c_pmu_stop_counters,
> +	.enable_counter		= hisi_l3c_pmu_enable_counter,
> +	.disable_counter	= hisi_l3c_pmu_disable_counter,
> +	.enable_counter_int	= hisi_l3c_pmu_enable_counter_int,
> +	.disable_counter_int	= hisi_l3c_pmu_disable_counter_int,
> +	.write_counter		= hisi_l3c_pmu_write_counter,
> +	.read_counter		= hisi_l3c_pmu_read_counter,
> +};
> +
> +static int hisi_l3c_pmu_dev_probe(struct platform_device *pdev,
> +				  struct hisi_pmu *l3c_pmu)
> +{
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	ret = hisi_l3c_pmu_init_data(pdev, l3c_pmu);
> +	if (ret)
> +		return ret;
> +
> +	ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
> +				       &l3c_pmu->node);
> +	if (ret) {
> +		dev_err(dev, "Error %d registering hotplug", ret);
> +		return ret;
> +	}
> +
> +	ret = hisi_l3c_pmu_init_irq(l3c_pmu, pdev);
> +	if (ret)
> +		goto fail;
> +
> +	l3c_pmu->name = devm_kasprintf(dev, GFP_KERNEL, "hisi_l3c%u_%u",
> +				       l3c_pmu->l3c_tag_id, l3c_pmu->scl_id);
> +	l3c_pmu->num_events = L3C_NR_EVENTS;
> +	l3c_pmu->num_counters = L3C_NR_COUNTERS;
> +	l3c_pmu->counter_bits = 48;
> +	l3c_pmu->ops = &hisi_uncore_l3c_ops;
> +	l3c_pmu->dev = dev;
> +
> +	return 0;
> +
> +fail:
> +	cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
> +				    &l3c_pmu->node);
> +	return ret;
> +}
> +
> +static int hisi_l3c_pmu_probe(struct platform_device *pdev)
> +{
> +	struct hisi_pmu *l3c_pmu;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	l3c_pmu = hisi_pmu_alloc(dev, L3C_NR_COUNTERS);
> +	if (!l3c_pmu)
> +		return -ENOMEM;
> +
> +	ret = hisi_l3c_pmu_dev_probe(pdev, l3c_pmu);
> +	if (ret)
> +		return ret;
> +
> +	l3c_pmu->pmu = (struct pmu) {
> +		.name		= l3c_pmu->name,
> +		.task_ctx_nr	= perf_invalid_context,
> +		.event_init	= hisi_uncore_pmu_event_init,
> +		.pmu_enable	= hisi_uncore_pmu_enable,
> +		.pmu_disable	= hisi_uncore_pmu_disable,
> +		.add		= hisi_uncore_pmu_add,
> +		.del		= hisi_uncore_pmu_del,
> +		.start		= hisi_uncore_pmu_start,
> +		.stop		= hisi_uncore_pmu_stop,
> +		.read		= hisi_uncore_pmu_read,
> +		.attr_groups	= hisi_l3c_pmu_attr_groups,  
Obviously not relevant to this patch as you work with what you
have, but this is a structure that looks rather ripe for
splitting into a const ops structure and a data structure
containing the bits that tend to be dynamic (like the name.
Perhaps it's not worth the hassle though.
> +	};
> +
> +	ret = hisi_uncore_pmu_setup(l3c_pmu, l3c_pmu->name);  
As mentioned for patch 2, I'd remove this wrapper.  That will make it
clearer that the remove is reversing everything it needs to.

> +	if (ret) {
> +		dev_err(l3c_pmu->dev, "hisi_uncore_pmu_setup failed!\n");
> +		cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
> +					    &l3c_pmu->node);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, l3c_pmu);
> +
> +	return 0;
> +}
> +
> +static int hisi_l3c_pmu_remove(struct platform_device *pdev)
> +{
> +	struct hisi_pmu *l3c_pmu = platform_get_drvdata(pdev);
> +
> +	perf_pmu_unregister(&l3c_pmu->pmu);
> +	cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
> +				    &l3c_pmu->node);  
The related add_instance call is wrapped up in
hisi_l3c_pmu_dev_probe. 
I would suggest wrapping this last call up in a hisi_l3c_pmu_dev_remove
function so that the balance between probe and remove is obvious.

Basic aim of the game is to make the code as easy to review as possible
and this would make it slightly more readable.
> +
> +	return 0;
> +}
> +
> +static struct platform_driver hisi_l3c_pmu_driver = {
> +	.driver = {
> +		.name = "hisi_l3c_pmu",
> +		.acpi_match_table = ACPI_PTR(hisi_l3c_pmu_acpi_match),
> +	},
> +	.probe = hisi_l3c_pmu_probe,
> +	.remove = hisi_l3c_pmu_remove,
> +};
> +
> +static int __init hisi_l3c_pmu_module_init(void)
> +{
> +	int ret;
> +
> +	ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
> +				      "AP_PERF_ARM_HISI_L3_ONLINE",
> +				      hisi_l3c_pmu_online_cpu,
> +				      hisi_l3c_pmu_offline_cpu);
> +	if (ret) {
> +		pr_err("l3c_pmu_module_init: Error setup hotplug, ret=%d", ret);
> +		return ret;
> +	}
> +
> +	ret = platform_driver_register(&hisi_l3c_pmu_driver);
> +	if (ret)
> +		cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE);
> +
> +	return ret;
> +}
> +module_init(hisi_l3c_pmu_module_init);
> +
> +static void __exit hisi_l3c_pmu_module_exit(void)
> +{
> +	cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE);
> +	platform_driver_unregister(&hisi_l3c_pmu_driver);  

Would normally expect an exit function to reverse the order of what
goes on in the init function.  This is true even if it doesn't
matter as it makes it 'obviously correct' and the reviewer doesn't
have to think about it! So...

platform_driver_unregister(&hisi_l3c_pmu_driver);
cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE);

If not there should be a comment explaining why not.

> +}
> +module_exit(hisi_l3c_pmu_module_exit);
> +
> +MODULE_DESCRIPTION("HiSilicon SoC L3C uncore PMU driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Anurup M, Shaokun Zhang");
> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index b56573b..2eab157 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -136,6 +136,7 @@ enum cpuhp_state {
>  	CPUHP_AP_PERF_S390_SF_ONLINE,
>  	CPUHP_AP_PERF_ARM_CCI_ONLINE,
>  	CPUHP_AP_PERF_ARM_CCN_ONLINE,
> +	CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
>  	CPUHP_AP_PERF_ARM_L2X0_ONLINE,
>  	CPUHP_AP_PERF_ARM_QCOM_L2_ONLINE,
>  	CPUHP_AP_PERF_ARM_QCOM_L3_ONLINE,  

Re: [PATCH v3 1/6] Documentation: perf: hisi: Documentation for HiSilicon SoC PMU driver

[Zhangshaokun]

Hi Jonathan,

Thanks for your comments firstly.

On 2017/7/19 17:17, Jonathan Cameron wrote:
> On Tue, 18 Jul 2017 15:59:54 +0800
> Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
> 
>> This patch adds documentation for the uncore PMUs on HiSilicon SoC.
>>
>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
>> Signed-off-by: Anurup M <anurup.m@huawei.com>  
> Hi Shaokun,
> 
> Sorry for the late reply on this (only recently joined Huawei)
> 
> This is a fairly generic review of the code rather than going into
> the actual userspace ABI choices as this is an area I'm only just
> starting to become familiar with.
> 
> Thanks,
> 
> Jonathan
>> ---
>>  Documentation/perf/hisi-pmu.txt | 51 +++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 51 insertions(+)
>>  create mode 100644 Documentation/perf/hisi-pmu.txt
>>
>> diff --git a/Documentation/perf/hisi-pmu.txt b/Documentation/perf/hisi-pmu.txt
>> new file mode 100644
>> index 0000000..5fa0b1a
>> --- /dev/null
>> +++ b/Documentation/perf/hisi-pmu.txt
>> @@ -0,0 +1,51 @@
>> +HiSilicon SoC uncore Performance Monitoring Unit (PMU)
>> +======================================================
>> +The HiSilicon SoC chip comprehends various independent system device PMUs
>> +such as L3 cache (L3C), Hydra Home Agent (HHA) and DDRC. These PMUs are
>> +independent and have hardware logic to gather statistics and performance
>> +information.
>> +
>> +HiSilicon SoC encapsulates multiple CPU and IO dies. Each CPU cluster (CC
>> +L) is made up of 4 cpu cores sharing one L3 cache; Each CPU die is called  
> nitpick, I'd not have a line break mid acronym. 
> 

Ok, shall keep CCL in the same line.

>> +Super CPU cluster (SCCL) and is made up of 6 CCLs. Each SCCL has two HHAs
>> +(0 - 1) and four DDRCs (0 - 3), respectively.
>> +
>> +HiSilicon SoC uncore PMU driver
>> +---------------------------------------
>> +Each device PMU has separate registers for event counting, control and
>> +interrupt, and the PMU driver shall register perf PMU drivers like L3C,
>> +HHA and DDRC etc. The available events and configuration options shall
>> +be described in the sysfs, see /sys/devices/hisi_*.  
> Is there not a subsystem directory that would make more sense to
> refer to than the full device list?
> 

For uncore devices, it is more reasonable to list in /sys/devices/***.

Thanks.
Shaokun

>> +The "perf list" command shall list the available events from sysfs.
>> +
>> +Each L3C, HHA and DDRC in one SCCL are registered as an separate PMU with perf.
>> +The PMU name will appear in event listing as hisi_module <index-id>_<sccl-id>.
>> +where "index-id" is the index of module and "sccl-id" is the identifier of
>> +the SCCL.
>> +e.g. hisi_l3c0_1/rd_hit_cpipe is READ_HIT_CPIPE event of L3C index #0 and SCCL
>> +ID #1.
>> +e.g. hisi_hha0_1/rx_operations is RX_OPERATIONS event of HHA index #0 and SCCL
>> +ID #1.
>> +
>> +The driver also provides a "cpumask" sysfs attribute, which shows the CPU core
>> +ID used to count the uncore PMU event.
>> +
>> +Example usage of perf:
>> +$# perf list
>> +hisi_l3c0_3/rd_hit_cpipe/ [kernel PMU event]
>> +------------------------------------------
>> +hisi_l3c0_3/wr_hit_cpipe/ [kernel PMU event]
>> +------------------------------------------
>> +hisi_l3c0_1/rd_hit_cpipe/ [kernel PMU event]
>> +------------------------------------------
>> +hisi_l3c0_1/wr_hit_cpipe/ [kernel PMU event]
>> +------------------------------------------
>> +
>> +$# perf stat -a -e hisi_l3c0_1/rd_hit_cpipe/ sleep 5
>> +$# perf stat -a -e hisi_l3c0_1/config=0x02/ sleep 5
>> +
>> +The current driver does not support sampling. So "perf record" is unsupported.
>> +Also attach to a task is unsupported as the events are all uncore.
>> +
>> +Note: Please contact the maintainer for a complete list of events supported for
>> +the PMU devices in the SoC and its information if needed.  
> 
> 
> _______________________________________________
> linuxarm mailing list
> 
> 
> .
> 

Re: [PATCH v3 2/6] perf: hisi: Add support for HiSilicon SoC uncore PMU driver

[Zhangshaokun]

Hi Jonathan

On 2017/7/19 17:19, Jonathan Cameron wrote:
> On Tue, 18 Jul 2017 15:59:55 +0800
> Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
> 
>> This patch adds support HiSilicon SoC uncore PMU driver framework and
>> interfaces.
>>
>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
>> Signed-off-by: Anurup M <anurup.m@huawei.com>  
> A couple of minor things inline.
> 
> Jonathan
>> ---
>>  drivers/perf/Kconfig                     |   7 +
>>  drivers/perf/Makefile                    |   1 +
>>  drivers/perf/hisilicon/Makefile          |   1 +
>>  drivers/perf/hisilicon/hisi_uncore_pmu.c | 411 +++++++++++++++++++++++++++++++
>>  drivers/perf/hisilicon/hisi_uncore_pmu.h | 116 +++++++++
>>  5 files changed, 536 insertions(+)
>>  create mode 100644 drivers/perf/hisilicon/Makefile
>>  create mode 100644 drivers/perf/hisilicon/hisi_uncore_pmu.c
>>  create mode 100644 drivers/perf/hisilicon/hisi_uncore_pmu.h
>>
>> diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
>> index e5197ff..78fc4bc 100644
>> --- a/drivers/perf/Kconfig
>> +++ b/drivers/perf/Kconfig
>> @@ -17,6 +17,13 @@ config ARM_PMU_ACPI
>>  	depends on ARM_PMU && ACPI
>>  	def_bool y
>>  
>> +config HISI_PMU
>> +	bool "HiSilicon SoC PMU"
>> +	depends on ARM64 && ACPI
>> +	help
>> +	  Support for HiSilicon SoC uncore performance monitoring
>> +	  unit (PMU), such as: L3C, HHA and DDRC.
>> +
>>  config QCOM_L2_PMU
>>  	bool "Qualcomm Technologies L2-cache PMU"
>>  	depends on ARCH_QCOM && ARM64 && ACPI
>> diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
>> index 6420bd4..41d3342 100644
>> --- a/drivers/perf/Makefile
>> +++ b/drivers/perf/Makefile
>> @@ -1,5 +1,6 @@
>>  obj-$(CONFIG_ARM_PMU) += arm_pmu.o arm_pmu_platform.o
>>  obj-$(CONFIG_ARM_PMU_ACPI) += arm_pmu_acpi.o
>> +obj-$(CONFIG_HISI_PMU) += hisilicon/
>>  obj-$(CONFIG_QCOM_L2_PMU)	+= qcom_l2_pmu.o
>>  obj-$(CONFIG_QCOM_L3_PMU) += qcom_l3_pmu.o
>>  obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
>> diff --git a/drivers/perf/hisilicon/Makefile b/drivers/perf/hisilicon/Makefile
>> new file mode 100644
>> index 0000000..2783bb3
>> --- /dev/null
>> +++ b/drivers/perf/hisilicon/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
>> new file mode 100644
>> index 0000000..4eb04e1
>> --- /dev/null
>> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
>> @@ -0,0 +1,411 @@
>> +/*
>> + * HiSilicon SoC Hardware event counters support
>> + *
>> + * Copyright (C) 2017 Hisilicon Limited
>> + * Author: Anurup M <anurup.m@huawei.com>
>> + *         Shaokun Zhang <zhangshaokun@hisilicon.com>
>> + *
>> + * This code is based on the uncore PMUs like arm-cci and arm-ccn.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +#include <linux/bitmap.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/perf_event.h>
>> +#include "hisi_uncore_pmu.h"
>> +
>> +/*
>> + * PMU format attributes
>> + */
>> +ssize_t hisi_format_sysfs_show(struct device *dev,
>> +			       struct device_attribute *attr, char *buf)
>> +{
>> +	struct dev_ext_attribute *eattr;
>> +
>> +	eattr = container_of(attr, struct dev_ext_attribute, attr);
>> +
>> +	return sprintf(buf, "%s\n", (char *)eattr->var);
>> +}
>> +
>> +/*
>> + * PMU event attributes
>> + */
>> +ssize_t hisi_event_sysfs_show(struct device *dev,
>> +			      struct device_attribute *attr, char *page)
>> +{
>> +	struct dev_ext_attribute *eattr;
>> +
>> +	eattr = container_of(attr, struct dev_ext_attribute, attr);
>> +
>> +	return sprintf(page, "config=0x%lx\n", (unsigned long)eattr->var);
>> +}
>> +
>> +/*
>> + * sysfs cpumask attributes
>> + */
>> +ssize_t hisi_cpumask_sysfs_show(struct device *dev,
>> +				struct device_attribute *attr, char *buf)
>> +{
>> +	struct pmu *pmu = dev_get_drvdata(dev);
>> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(pmu);
>> +
>> +	return cpumap_print_to_pagebuf(true, buf, &hisi_pmu->cpus);
>> +}
>> +
>> +/* Read Super CPU cluster and CPU cluster ID from MPIDR_EL1 */
>> +void hisi_read_scl_and_ccl_id(u32 *scl_id, u32 *ccl_id)
>> +{
>> +	u64 mpidr;
>> +
>> +	mpidr = read_cpuid_mpidr();
>> +	if (mpidr & MPIDR_MT_BITMASK) {
>> +		if (scl_id)
>> +			*scl_id = MPIDR_AFFINITY_LEVEL(mpidr, 3);
>> +		if (ccl_id)
>> +			*ccl_id = MPIDR_AFFINITY_LEVEL(mpidr, 2);
>> +	} else {
>> +		if (scl_id)
>> +			*scl_id = MPIDR_AFFINITY_LEVEL(mpidr, 2);
>> +		if (ccl_id)
>> +			*ccl_id = MPIDR_AFFINITY_LEVEL(mpidr, 1);
>> +	}
>> +}
>> +
>> +static bool hisi_validate_event_group(struct perf_event *event)
>> +{
>> +	struct perf_event *sibling, *leader = event->group_leader;
>> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
>> +	/* Include count for the event */
>> +	int counters = 1;
>> +
>> +	/*
>> +	 * We must NOT create groups containing mixed PMUs, although
>> +	 * software events are acceptable
>> +	 */
>> +	if (leader->pmu != event->pmu && !is_software_event(leader))
>> +		return false;
>> +
>> +	/* Increment counter for the leader */
>> +	counters++;
>> +
>> +	list_for_each_entry(sibling, &event->group_leader->sibling_list,
>> +			    group_entry) {
>> +		if (is_software_event(sibling))
>> +			continue;
>> +		if (sibling->pmu != event->pmu)
>> +			return false;
>> +		/* Increment counter for each sibling */
>> +		counters++;
>> +	}
>> +
>> +	/* The group can not count events more than the counters in the HW */
>> +	return counters <= hisi_pmu->num_counters;
>> +}
>> +
>> +int hisi_uncore_pmu_counter_valid(struct hisi_pmu *hisi_pmu, int idx)
>> +{
>> +	return (idx >= 0 && idx < hisi_pmu->num_counters);
>> +}
>> +
>> +int hisi_uncore_pmu_get_event_idx(struct perf_event *event)
>> +{
>> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
>> +	unsigned long *used_mask = hisi_pmu->pmu_events.used_mask;
>> +	u32 num_counters = hisi_pmu->num_counters;
>> +	int idx;
>> +
>> +	idx = find_first_zero_bit(used_mask, num_counters);
>> +	if (idx == num_counters)
>> +		return -EAGAIN;
>> +
>> +	set_bit(idx, used_mask);
>> +
>> +	return idx;
>> +}
>> +
>> +void hisi_uncore_pmu_clear_event_idx(struct hisi_pmu *hisi_pmu, int idx)
>> +{
>> +	if (!hisi_uncore_pmu_counter_valid(hisi_pmu, idx)) {
>> +		dev_err(hisi_pmu->dev, "Unsupported event index:%d!\n", idx);
>> +		return;
>> +	}
>> +
>> +	clear_bit(idx, hisi_pmu->pmu_events.used_mask);
>> +}
>> +
>> +int hisi_uncore_pmu_event_init(struct perf_event *event)
>> +{
>> +	struct hisi_pmu *hisi_pmu;
>> +	struct hw_perf_event *hwc = &event->hw;
>> +	int cpu;
>> +
>> +	if (event->attr.type != event->pmu->type)
>> +		return -ENOENT;
>> +
>> +	/*
>> +	 * We do not support sampling as the counters are all
>> +	 * shared by all CPU cores in a CPU die(SCCL). Also we
>> +	 * do not support attach to a task(per-process mode)
>> +	 */
>> +	if (is_sampling_event(event) || event->attach_state & PERF_ATTACH_TASK)
>> +		return -EOPNOTSUPP;
>> +
>> +	/* counters do not have these bits */
>> +	if (event->attr.exclude_user	||
>> +	    event->attr.exclude_kernel	||
>> +	    event->attr.exclude_host	||
>> +	    event->attr.exclude_guest	||
>> +	    event->attr.exclude_hv	||
>> +	    event->attr.exclude_idle)
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 *  The uncore counters not specific to any CPU, so cannot
>> +	 *  support per-task
>> +	 */
>> +	if (event->cpu < 0)
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * Validate if the events in group does not exceed the
>> +	 * available counters in hardware.
>> +	 */
>> +	if (!hisi_validate_event_group(event))
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * We don't assign an index until we actually place the event onto
>> +	 * hardware. Use -1 to signify that we haven't decided where to put it
>> +	 * yet.
>> +	 */
>> +	hwc->idx		= -1;
>> +	hwc->config_base	= event->attr.config;
>> +
>> +	/* Select an available CPU to monitor events in this PMU */
>> +	hisi_pmu = to_hisi_pmu(event->pmu);
>> +	cpu = cpumask_first(&hisi_pmu->cpus);
>> +	if (cpu >= nr_cpu_ids)
>> +		return -EINVAL;
>> +
>> +	/* Enforce to use the same CPU for all events in this PMU */
>> +	event->cpu = cpu;
>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * Set the counter to count the event that we're interested in,
>> + * and enable counter and interrupt.
>> + */
>> +static void hisi_uncore_pmu_enable_event(struct perf_event *event)
>> +{
>> +	struct hw_perf_event *hwc = &event->hw;
>> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
>> +
>> +	/*
>> +	 * Write event code in event select registers(for DDRC PMU,
>> +	 * event has been mapped to fixed-purpose counter, there is
>> +	 * no need to write event type).
>> +	 */
>> +	if (hisi_pmu->ops->write_evtype)
>> +		hisi_pmu->ops->write_evtype(hisi_pmu, hwc->idx,
>> +					    GET_EVENTID(event));
>> +
>> +	/* Enable the hardware event counting and interrupt */
>> +	hisi_pmu->ops->enable_counter(hisi_pmu, hwc);
>> +	hisi_pmu->ops->enable_counter_int(hisi_pmu, hwc);
>> +}
>> +
>> +/*
>> + * Disable counting and interrupt.
>> + */
>> +static void hisi_uncore_pmu_disable_event(struct perf_event *event)
>> +{
>> +	struct hw_perf_event *hwc = &event->hw;
>> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
>> +
>> +	hisi_pmu->ops->disable_counter(hisi_pmu, hwc);
>> +	hisi_pmu->ops->disable_counter_int(hisi_pmu, hwc);
>> +}
>> +
>> +void hisi_uncore_pmu_set_event_period(struct perf_event *event)
>> +{
>> +	struct hw_perf_event *hwc = &event->hw;
>> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
>> +
>> +	/*
>> +	 * The HiSilicon PMU counters support 32 bits or 48 bits, depending on
>> +	 * the PMU. We reduce it to 2^(counter_bits - 1) to account for the
>> +	 * extreme interrupt latency. So we could hopefully handle the overflow
>> +	 * interrupt before another 2^(counter_bits - 1) events occur and the
>> +	 * counter overtakes its previous value.
>> +	 */
>> +	u64 val = BIT_ULL(hisi_pmu->counter_bits - 1);
>> +
>> +	local64_set(&hwc->prev_count, val);
>> +	/* Write start value to the hardware event counter */
>> +	hisi_pmu->ops->write_counter(hisi_pmu, hwc, val);
>> +}
>> +
>> +u64 hisi_uncore_pmu_event_update(struct perf_event *event)
>> +{
>> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
>> +	struct hw_perf_event *hwc = &event->hw;
>> +	u64 delta, prev_raw_count, new_raw_count;
>> +
>> +	do {
>> +		/* Read the count from the counter register */
>> +		new_raw_count = hisi_pmu->ops->read_counter(hisi_pmu, hwc);
>> +		prev_raw_count = local64_read(&hwc->prev_count);
>> +	} while (local64_cmpxchg(&hwc->prev_count, prev_raw_count,
>> +				 new_raw_count) != prev_raw_count);
>> +	/*
>> +	 * compute the delta
>> +	 */
>> +	delta = (new_raw_count - prev_raw_count) &
>> +		HISI_MAX_PERIOD(hisi_pmu->counter_bits);
>> +	local64_add(delta, &event->count);
>> +
>> +	return new_raw_count;
>> +}
>> +
>> +void hisi_uncore_pmu_start(struct perf_event *event, int flags)
>> +{
>> +	struct hw_perf_event *hwc = &event->hw;
>> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
>> +
>> +	if (WARN_ON_ONCE(!(hwc->state & PERF_HES_STOPPED)))
>> +		return;
>> +
>> +	WARN_ON_ONCE(!(hwc->state & PERF_HES_UPTODATE));
>> +	hwc->state = 0;
>> +	hisi_uncore_pmu_set_event_period(event);
>> +
>> +	if (flags & PERF_EF_RELOAD) {
>> +		u64 prev_raw_count =  local64_read(&hwc->prev_count);
>> +
>> +		hisi_pmu->ops->write_counter(hisi_pmu, hwc, prev_raw_count);
>> +	}
>> +
>> +	hisi_uncore_pmu_enable_event(event);
>> +	perf_event_update_userpage(event);
>> +}
>> +
>> +void hisi_uncore_pmu_stop(struct perf_event *event, int flags)
>> +{
>> +	struct hw_perf_event *hwc = &event->hw;
>> +
>> +	hisi_uncore_pmu_disable_event(event);
>> +	WARN_ON_ONCE(hwc->state & PERF_HES_STOPPED);
>> +	hwc->state |= PERF_HES_STOPPED;
>> +
>> +	if (hwc->state & PERF_HES_UPTODATE)
>> +		return;
>> +
>> +	/* Read hardware counter and update the Perf counter statistics */
>> +	hisi_uncore_pmu_event_update(event);
>> +	hwc->state |= PERF_HES_UPTODATE;
>> +}
>> +
>> +int hisi_uncore_pmu_add(struct perf_event *event, int flags)
>> +{
>> +	struct hw_perf_event *hwc = &event->hw;
>> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
>> +	int idx;
>> +
>> +	hwc->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
>> +
>> +	/* Get an available counter index for counting */
>> +	idx = hisi_pmu->ops->get_event_idx(event);
>> +	if (idx < 0)
>> +		return -EAGAIN;
>> +
>> +	event->hw.idx = idx;
>> +	hisi_pmu->pmu_events.hw_events[idx] = event;
>> +
>> +	if (flags & PERF_EF_START)
>> +		hisi_uncore_pmu_start(event, PERF_EF_RELOAD);
>> +
>> +	return 0;
>> +}
>> +
>> +void hisi_uncore_pmu_del(struct perf_event *event, int flags)
>> +{
>> +	struct hw_perf_event *hwc = &event->hw;
>> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu);
>> +
>> +	hisi_uncore_pmu_stop(event, PERF_EF_UPDATE);
>> +	hisi_uncore_pmu_clear_event_idx(hisi_pmu, hwc->idx);
>> +	perf_event_update_userpage(event);
>> +	hisi_pmu->pmu_events.hw_events[hwc->idx] = NULL;
>> +}
>> +
>> +struct hisi_pmu *hisi_pmu_alloc(struct device *dev, u32 num_cntrs)
>> +{
>> +	struct hisi_pmu *hisi_pmu;
>> +	struct hisi_pmu_hwevents *pmu_events;
>> +
>> +	hisi_pmu = devm_kzalloc(dev, sizeof(*hisi_pmu), GFP_KERNEL);
>> +	if (!hisi_pmu)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	pmu_events = &hisi_pmu->pmu_events;
>> +	pmu_events->hw_events = devm_kcalloc(dev,
>> +					     num_cntrs,
>> +					     sizeof(*pmu_events->hw_events),
>> +					     GFP_KERNEL);
>> +	if (!pmu_events->hw_events)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	pmu_events->used_mask = devm_kcalloc(dev,
>> +					     BITS_TO_LONGS(num_cntrs),
>> +					     sizeof(*pmu_events->used_mask),
>> +					     GFP_KERNEL);
>> +	if (!pmu_events->used_mask)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	return 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);
>> +}
>> +
>> +void hisi_uncore_pmu_enable(struct pmu *pmu)
>> +{
>> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(pmu);
>> +	int enabled = bitmap_weight(hisi_pmu->pmu_events.used_mask,
>> +				    hisi_pmu->num_counters);
>> +
>> +	if (!enabled)
>> +		return;
>> +
>> +	hisi_pmu->ops->start_counters(hisi_pmu);
>> +}
>> +
>> +void hisi_uncore_pmu_disable(struct pmu *pmu)
>> +{
>> +	struct hisi_pmu *hisi_pmu = to_hisi_pmu(pmu);
>> +
>> +	hisi_pmu->ops->stop_counters(hisi_pmu);
>> +}
>> +
>> +int hisi_uncore_pmu_setup(struct hisi_pmu *hisi_pmu, const char *pmu_name)
>> +{
>> +	/* Register the events with perf */
>> +	return perf_pmu_register(&hisi_pmu->pmu, pmu_name, -1);
>> +}  
> This seems oddly unbalanced given you then do the
> perf_pmu_unregister() calls directly in the various drivers.
> 
> So I'd push the contents of this function down into the drivers
> themselves.
> 

Agree, shall remove this function call perf_pmu_unregister() directly in drivers.

>> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h
>> new file mode 100644
>> index 0000000..eb2abb0
>> --- /dev/null
>> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
>> @@ -0,0 +1,116 @@
>> +/*
>> + * HiSilicon SoC Hardware event counters support
>> + *
>> + * Copyright (C) 2017 Hisilicon Limited
>> + * Author: Anurup M <anurup.m@huawei.com>
>> + *         Shaokun Zhang <zhangshaokun@hisilicon.com>
>> + *
>> + * This code is based on the uncore PMUs like arm-cci and arm-ccn.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +#ifndef __HISI_UNCORE_PMU_H__
>> +#define __HISI_UNCORE_PMU_H__
>> +
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/ktime.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/types.h>
>> +#include <asm/local64.h>  
> This is a wide range of headers to have included from
> an include. At least some of these should be pushed down into the c files.
> 

Ok, shall organize the header file.

>> +
>> +#undef pr_fmt
>> +#define pr_fmt(fmt)     "hisi_pmu: " fmt
>> +
>> +#define GET_EVENTID(ev)   (ev->hw.config_base & 0xff)  
> I'd add a prefix to this as chance a clash in future for
> something with that name seems very high.
> 
> It is also only used in hisi_uncore_pmu.c so I'd push it down into
> that file.
> 

Ok, shall fix it.

>> +#define to_hisi_pmu(p)	(container_of(p, struct hisi_pmu, pmu))
>> +#define HISI_MAX_PERIOD(nr) (BIT_ULL(nr) - 1)  
> Only used in one file, so I'd push it down into the c file.

OK.

>> +
>> +#define HISI_PMU_ATTR(_name, _func, _config)				\
>> +	(&((struct dev_ext_attribute[]) {				\
>> +		{ __ATTR(_name, 0444, _func, NULL), (void *)_config }   \
>> +	})[0].attr.attr)
>> +
>> +#define HISI_PMU_FORMAT_ATTR(_name, _config)		\
>> +	HISI_PMU_ATTR(_name, hisi_format_sysfs_show, (void *)_config)
>> +#define HISI_PMU_EVENT_ATTR(_name, _config)		\
>> +	HISI_PMU_ATTR(_name, hisi_event_sysfs_show, (unsigned long)_config)
>> +
>> +struct hisi_pmu;
>> +
>> +struct hisi_uncore_ops {
>> +	void (*write_evtype)(struct hisi_pmu *, int, u32);
>> +	int (*get_event_idx)(struct perf_event *);
>> +	u64 (*read_counter)(struct hisi_pmu *, struct hw_perf_event *);
>> +	void (*write_counter)(struct hisi_pmu *, struct hw_perf_event *, u64);
>> +	void (*enable_counter)(struct hisi_pmu *, struct hw_perf_event *);
>> +	void (*disable_counter)(struct hisi_pmu *, struct hw_perf_event *);
>> +	void (*enable_counter_int)(struct hisi_pmu *, struct hw_perf_event *);
>> +	void (*disable_counter_int)(struct hisi_pmu *, struct hw_perf_event *);
>> +	void (*start_counters)(struct hisi_pmu *);
>> +	void (*stop_counters)(struct hisi_pmu *);
>> +};
>> +
>> +struct hisi_pmu_hwevents {
>> +	struct perf_event **hw_events;
>> +	unsigned long *used_mask;
>> +};
>> +
>> +/* Generic pmu struct for different pmu types */
>> +struct hisi_pmu {
>> +	const char *name;
>> +	struct pmu pmu;
>> +	const struct hisi_uncore_ops *ops;
>> +	struct hisi_pmu_hwevents pmu_events;
>> +	cpumask_t cpus;
>> +	struct device *dev;
>> +	struct hlist_node node;
>> +	u32 scl_id;
>> +	u32 ccl_id;
>> +	/* Hardware information for different pmu types */
>> +	void __iomem *base;
>> +	union {
>> +		u32 ddrc_chn_id;
>> +		u32 l3c_tag_id;
>> +		u32 hha_uid;
>> +	};
>> +	int num_counters;
>> +	int num_events;
>> +	int counter_bits;
>> +};
>> +
>> +int hisi_uncore_pmu_counter_valid(struct hisi_pmu *hisi_pmu, int idx);
>> +int hisi_uncore_pmu_get_event_idx(struct perf_event *event);
>> +void hisi_uncore_pmu_clear_event_idx(struct hisi_pmu *hisi_pmu, int idx);  
> The above is only used in hisi_uncore_pmu.c so doesn't need to be here
> and can be static.
> 

These functions would be called in L3C/HHA/DDR PMU driver. We want to give a
uncore perf framework in hisi_uncore_pmu.c for hisilicon uncore PMUs.

Thanks.
Shaokun

>> +void hisi_uncore_pmu_read(struct perf_event *event);
>> +int hisi_uncore_pmu_add(struct perf_event *event, int flags);
>> +void hisi_uncore_pmu_del(struct perf_event *event, int flags);
>> +void hisi_uncore_pmu_start(struct perf_event *event, int flags);
>> +void hisi_uncore_pmu_stop(struct perf_event *event, int flags);
>> +void hisi_uncore_pmu_set_event_period(struct perf_event *event);
>> +u64 hisi_uncore_pmu_event_update(struct perf_event *event);
>> +int hisi_uncore_pmu_event_init(struct perf_event *event);
>> +int hisi_uncore_pmu_setup(struct hisi_pmu *hisi_pmu, const char *pmu_name);
>> +void hisi_uncore_pmu_enable(struct pmu *pmu);
>> +void hisi_uncore_pmu_disable(struct pmu *pmu);
>> +struct hisi_pmu *hisi_pmu_alloc(struct device *dev, u32 num_cntrs);
>> +ssize_t hisi_event_sysfs_show(struct device *dev,
>> +			      struct device_attribute *attr, char *buf);
>> +ssize_t hisi_format_sysfs_show(struct device *dev,
>> +			       struct device_attribute *attr, char *buf);
>> +ssize_t hisi_cpumask_sysfs_show(struct device *dev,
>> +				struct device_attribute *attr, char *buf);
>> +void hisi_read_scl_and_ccl_id(u32 *scl_id, u32 *ccl_id);
>> +#endif /* __HISI_UNCORE_PMU_H__ */  
> 
> 
> .
> 

Re: [PATCH v3 1/6] Documentation: perf: hisi: Documentation for HiSilicon SoC PMU driver

[Will Deacon]

On Thu, Jul 20, 2017 at 08:54:36PM +0800, Zhangshaokun wrote:
> On 2017/7/19 17:17, Jonathan Cameron wrote:
> >> +Super CPU cluster (SCCL) and is made up of 6 CCLs. Each SCCL has two HHAs
> >> +(0 - 1) and four DDRCs (0 - 3), respectively.
> >> +
> >> +HiSilicon SoC uncore PMU driver
> >> +---------------------------------------
> >> +Each device PMU has separate registers for event counting, control and
> >> +interrupt, and the PMU driver shall register perf PMU drivers like L3C,
> >> +HHA and DDRC etc. The available events and configuration options shall
> >> +be described in the sysfs, see /sys/devices/hisi_*.  
> > Is there not a subsystem directory that would make more sense to
> > refer to than the full device list?
> > 
> 
> For uncore devices, it is more reasonable to list in /sys/devices/***.

The usual place for these things is /sys/bus/event_source/devices/<pmu name>.

The events are described as files under the events directory (one file per
event, which describes the encoding) and you can describe other things such
as formatting and/or capabilities under other directories too.

Take a look at some other PMU drivers to get an idea of how it works.

Will

Re: [PATCH v3 1/6] Documentation: perf: hisi: Documentation for HiSilicon SoC PMU driver

[Will Deacon]

On Thu, Jul 20, 2017 at 02:08:47PM +0100, Will Deacon wrote:
> On Thu, Jul 20, 2017 at 08:54:36PM +0800, Zhangshaokun wrote:
> > On 2017/7/19 17:17, Jonathan Cameron wrote:
> > >> +Super CPU cluster (SCCL) and is made up of 6 CCLs. Each SCCL has two HHAs
> > >> +(0 - 1) and four DDRCs (0 - 3), respectively.
> > >> +
> > >> +HiSilicon SoC uncore PMU driver
> > >> +---------------------------------------
> > >> +Each device PMU has separate registers for event counting, control and
> > >> +interrupt, and the PMU driver shall register perf PMU drivers like L3C,
> > >> +HHA and DDRC etc. The available events and configuration options shall
> > >> +be described in the sysfs, see /sys/devices/hisi_*.  
> > > Is there not a subsystem directory that would make more sense to
> > > refer to than the full device list?
> > > 
> > 
> > For uncore devices, it is more reasonable to list in /sys/devices/***.
> 
> The usual place for these things is /sys/bus/event_source/devices/<pmu name>.

... which are symlinks to directories under /sys/devices/! I didn't realise
that, so I suspect you're ok. Still worth getting the format of things
correct though, so perf can read/parse them with minimal effort.

Will

Re: [PATCH v3 2/6] perf: hisi: Add support for HiSilicon SoC uncore PMU driver

[Jonathan Cameron]

On Thu, 20 Jul 2017 21:03:19 +0800
Zhangshaokun <zhangshaokun@hisilicon.com> wrote:

> Hi Jonathan
> 
> On 2017/7/19 17:19, Jonathan Cameron wrote:
> > On Tue, 18 Jul 2017 15:59:55 +0800
> > Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
> >   
> >> This patch adds support HiSilicon SoC uncore PMU driver framework and
> >> interfaces.
> >>
> >> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> >> Signed-off-by: Anurup M <anurup.m@huawei.com>    
> > A couple of minor things inline.
> > 
<snip>
> >> +/* Generic pmu struct for different pmu types */
> >> +struct hisi_pmu {
> >> +	const char *name;
> >> +	struct pmu pmu;
> >> +	const struct hisi_uncore_ops *ops;
> >> +	struct hisi_pmu_hwevents pmu_events;
> >> +	cpumask_t cpus;
> >> +	struct device *dev;
> >> +	struct hlist_node node;
> >> +	u32 scl_id;
> >> +	u32 ccl_id;
> >> +	/* Hardware information for different pmu types */
> >> +	void __iomem *base;
> >> +	union {
> >> +		u32 ddrc_chn_id;
> >> +		u32 l3c_tag_id;
> >> +		u32 hha_uid;
> >> +	};
> >> +	int num_counters;
> >> +	int num_events;
> >> +	int counter_bits;
> >> +};
> >> +
> >> +int hisi_uncore_pmu_counter_valid(struct hisi_pmu *hisi_pmu, int idx);
> >> +int hisi_uncore_pmu_get_event_idx(struct perf_event *event);
> >> +void hisi_uncore_pmu_clear_event_idx(struct hisi_pmu *hisi_pmu, int idx);    
> > The above is only used in hisi_uncore_pmu.c so doesn't need to be here
> > and can be static.
> >   
> 
> These functions would be called in L3C/HHA/DDR PMU driver. We want to give a
> uncore perf framework in hisi_uncore_pmu.c for hisilicon uncore PMUs.
> 
For all but the one function above that is true.  I couldn't find this one being
used anywhere in those drivers.


> Thanks.
> Shaokun
> 
> >> +void hisi_uncore_pmu_read(struct perf_event *event);
> >> +int hisi_uncore_pmu_add(struct perf_event *event, int flags);
> >> +void hisi_uncore_pmu_del(struct perf_event *event, int flags);
> >> +void hisi_uncore_pmu_start(struct perf_event *event, int flags);
> >> +void hisi_uncore_pmu_stop(struct perf_event *event, int flags);
> >> +void hisi_uncore_pmu_set_event_period(struct perf_event *event);
> >> +u64 hisi_uncore_pmu_event_update(struct perf_event *event);
> >> +int hisi_uncore_pmu_event_init(struct perf_event *event);
> >> +int hisi_uncore_pmu_setup(struct hisi_pmu *hisi_pmu, const char *pmu_name);
> >> +void hisi_uncore_pmu_enable(struct pmu *pmu);
> >> +void hisi_uncore_pmu_disable(struct pmu *pmu);
> >> +struct hisi_pmu *hisi_pmu_alloc(struct device *dev, u32 num_cntrs);
> >> +ssize_t hisi_event_sysfs_show(struct device *dev,
> >> +			      struct device_attribute *attr, char *buf);
> >> +ssize_t hisi_format_sysfs_show(struct device *dev,
> >> +			       struct device_attribute *attr, char *buf);
> >> +ssize_t hisi_cpumask_sysfs_show(struct device *dev,
> >> +				struct device_attribute *attr, char *buf);
> >> +void hisi_read_scl_and_ccl_id(u32 *scl_id, u32 *ccl_id);
> >> +#endif /* __HISI_UNCORE_PMU_H__ */    
> > 
> > 
> > .
> >   
> 

Re: [PATCH v3 3/6] perf: hisi: Add support for HiSilicon SoC L3C PMU driver

[Zhangshaokun]

Hi Jonathan,

On 2017/7/19 17:28, Jonathan Cameron wrote:
> On Tue, 18 Jul 2017 15:59:56 +0800
> Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
> 
>> This patch adds support for L3C PMU driver in HiSilicon SoC chip, Each
>> L3C has own control, counter and interrupt registers and is an separate
>> PMU. For each L3C PMU, it has 8-programable counters and supports 0x60
>> events, event code is 8-bits and every counter is free-running.
>> Interrupt is supported to handle counter (48-bits) overflow.
>>
>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
>> Signed-off-by: Anurup M <anurup.m@huawei.com>  
> Hi Shaokun,
> 
> A few minor points inline.
> 
> Thanks,
> 
> Jonathan
>> ---
>>  drivers/perf/hisilicon/Makefile              |   2 +-
>>  drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c | 556 +++++++++++++++++++++++++++
>>  include/linux/cpuhotplug.h                   |   1 +
>>  3 files changed, 558 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
>>
>> diff --git a/drivers/perf/hisilicon/Makefile b/drivers/perf/hisilicon/Makefile
>> index 2783bb3..4a3d3e6 100644
>> --- a/drivers/perf/hisilicon/Makefile
>> +++ b/drivers/perf/hisilicon/Makefile
>> @@ -1 +1 @@
>> -obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o
>> +obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o hisi_uncore_l3c_pmu.o
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
>> new file mode 100644
>> index 0000000..d430508
>> --- /dev/null
>> +++ b/drivers/perf/hisilicon/hisi_uncore_l3c_pmu.c
>> @@ -0,0 +1,556 @@
>> +/*
>> + * HiSilicon SoC L3C uncore Hardware event counters support
>> + *
>> + * Copyright (C) 2017 Hisilicon Limited
>> + * Author: Anurup M <anurup.m@huawei.com>
>> + *         Shaokun Zhang <zhangshaokun@hisilicon.com>
>> + *
>> + * This code is based on the uncore PMUs like arm-cci and arm-ccn.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> Missed this on previous but putting full gpl text in is not normally done any
> more. 
>> + */
>> +#include <linux/acpi.h>
>> +#include <linux/bitmap.h>  
> Not immediately seeing any bitmaps in use in here.
> (I may well have missed something though!)
> 

Ok, shall remove it.

>> +#include <linux/cpuhotplug.h>
>> +#include <linux/module.h>
>> +#include <linux/perf_event.h>
>> +#include <linux/topology.h>
>> +#include "hisi_uncore_pmu.h"
> <snip>
>> +
>> +/* Check if the CPU belongs to the SCCL and CCL of PMU */
>> +static bool hisi_l3c_is_cpu_in_ccl(struct hisi_pmu *l3c_pmu)
>> +{
>> +	u32 ccl_id, scl_id;
>> +
>> +	/* Read Super CPU cluster ID from CPU MPIDR_EL1 */
>> +	hisi_read_scl_and_ccl_id(&scl_id, &ccl_id);
>> +
>> +	/* Check if the CPU match with the SCCL and CCL */
>> +	if (scl_id != l3c_pmu->scl_id)
>> +		return false;
>> +	if (ccl_id != l3c_pmu->ccl_id)
>> +		return false;
>> +
>> +	/* Return true if matched */  
> I think the name of the function makes this clear enough to
> not need a comment here, but up to you.

Shall remove some redundant comments

>> +	return true;
>> +}
>> +
>> +static int hisi_l3c_pmu_online_cpu(unsigned int cpu, struct hlist_node *node)
>> +{
>> +	struct hisi_pmu *l3c_pmu;
>> +
>> +	l3c_pmu = hlist_entry_safe(node, struct hisi_pmu, node);
>> +
>> +	/* Proceed only when CPU belongs to the same SCCL and CCL */
>> +	if (!hisi_l3c_is_cpu_in_ccl(l3c_pmu))
>> +		return 0;
>> +
>> +	/* If another CPU is already managing the CPU cluster, simply return */
>> +	if (!cpumask_empty(&l3c_pmu->cpus))
>> +		return 0;
>> +
>> +	/* Use this CPU for event counting in the CCL */
>> +	cpumask_set_cpu(cpu, &l3c_pmu->cpus);
>> +
>> +	return 0;
>> +}
>> +
>> +static int hisi_l3c_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
>> +{
>> +	struct hisi_pmu *l3c_pmu;
>> +	cpumask_t ccl_online_cpus;
>> +	unsigned int target;
>> +
>> +	l3c_pmu = hlist_entry_safe(node, struct hisi_pmu, node);
>> +
>> +	/* Proceed only when CPU belongs to the same SCCL and CCL */
>> +	if (!hisi_l3c_is_cpu_in_ccl(l3c_pmu))
>> +		return 0;
>> +
>> +	/* Proceed if this CPU is used for event counting */
>> +	if (!cpumask_test_cpu(cpu, &l3c_pmu->cpus))
>> +		return 0;
>> +
>> +	/* Give up ownership of CCL */
>> +	cpumask_test_and_clear_cpu(cpu, &l3c_pmu->cpus);
>> +
>> +	/* Any other CPU for this CCL which is still online */
>> +	cpumask_and(&ccl_online_cpus, cpu_coregroup_mask(cpu),
>> +		    cpu_online_mask);
>> +	target = cpumask_any_but(&ccl_online_cpus, cpu);
>> +	if (target >= nr_cpu_ids)
>> +		return 0;
>> +
>> +	perf_pmu_migrate_context(&l3c_pmu->pmu, cpu, target);
>> +	/* Use this CPU for event counting in the CCL */
>> +	cpumask_set_cpu(target, &l3c_pmu->cpus);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct acpi_device_id hisi_l3c_pmu_acpi_match[] = {
>> +	{ "HISI0213", },
>> +	{},
>> +};
>> +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)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct resource *res;
>> +	unsigned long long id;
>> +	acpi_status status;
>> +
>> +	status = acpi_evaluate_integer(ACPI_HANDLE(dev), "_UID", NULL, &id);
>> +	if (ACPI_FAILURE(status))
>> +		return false;  
> Nitpick: A blank line here would slightly aid readability.
>> +	l3c_pmu->l3c_tag_id = id;
>> +
>> +	/* Get the L3C SCCL ID */
>> +	if (device_property_read_u32(dev, "hisilicon,scl-id",
>> +				     &l3c_pmu->scl_id)) {
>> +		dev_err(dev, "Can not read l3c scl-id!\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Get the L3C CPU cluster(CCL) ID */
>> +	if (device_property_read_u32(dev, "hisilicon,ccl-id",
>> +				     &l3c_pmu->ccl_id)) {
>> +		dev_err(dev, "Can not read l3c ccl-id!\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	l3c_pmu->base = devm_ioremap_resource(dev, res);
>> +	if (IS_ERR(l3c_pmu->base)) {
>> +		dev_err(dev, "ioremap failed for l3c_pmu resource\n");
>> +		return PTR_ERR(l3c_pmu->base);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct attribute *hisi_l3c_pmu_format_attr[] = {
>> +	HISI_PMU_FORMAT_ATTR(event, "config:0-7"),
>> +	NULL,
>> +};
>> +
>> +static const struct attribute_group hisi_l3c_pmu_format_group = {
>> +	.name = "format",
>> +	.attrs = hisi_l3c_pmu_format_attr,
>> +};
>> +
>> +static struct attribute *hisi_l3c_pmu_events_attr[] = {
>> +	HISI_PMU_EVENT_ATTR(rd_cpipe,		0x00),
>> +	HISI_PMU_EVENT_ATTR(wr_cpipe,		0x01),
>> +	HISI_PMU_EVENT_ATTR(rd_hit_cpipe,	0x02),
>> +	HISI_PMU_EVENT_ATTR(wr_hit_cpipe,	0x03),
>> +	HISI_PMU_EVENT_ATTR(victim_num,		0x04),
>> +	HISI_PMU_EVENT_ATTR(rd_spipe,		0x20),
>> +	HISI_PMU_EVENT_ATTR(wr_spipe,		0x21),
>> +	HISI_PMU_EVENT_ATTR(rd_hit_spipe,	0x22),
>> +	HISI_PMU_EVENT_ATTR(wr_hit_spipe,	0x23),
>> +	HISI_PMU_EVENT_ATTR(back_invalid,	0x29),
>> +	HISI_PMU_EVENT_ATTR(retry_cpu,		0x40),
>> +	HISI_PMU_EVENT_ATTR(retry_ring,		0x41),
>> +	HISI_PMU_EVENT_ATTR(prefetch_drop,	0x42),
>> +	NULL,
>> +};
>> +
>> +static const struct attribute_group hisi_l3c_pmu_events_group = {
>> +	.name = "events",
>> +	.attrs = hisi_l3c_pmu_events_attr,
>> +};
>> +
>> +static struct attribute *hisi_l3c_pmu_attrs[] = {
>> +	NULL,
>> +};  
> Why have the empty group? 
>> +
>> +static const struct attribute_group hisi_l3c_pmu_attr_group = {
>> +	.attrs = hisi_l3c_pmu_attrs,
>> +};
>> +
>> +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 const struct attribute_group *hisi_l3c_pmu_attr_groups[] = {
>> +	&hisi_l3c_pmu_attr_group,
>> +	&hisi_l3c_pmu_format_group,
>> +	&hisi_l3c_pmu_events_group,
>> +	&hisi_l3c_pmu_cpumask_attr_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,
>> +	.start_counters		= hisi_l3c_pmu_start_counters,
>> +	.stop_counters		= hisi_l3c_pmu_stop_counters,
>> +	.enable_counter		= hisi_l3c_pmu_enable_counter,
>> +	.disable_counter	= hisi_l3c_pmu_disable_counter,
>> +	.enable_counter_int	= hisi_l3c_pmu_enable_counter_int,
>> +	.disable_counter_int	= hisi_l3c_pmu_disable_counter_int,
>> +	.write_counter		= hisi_l3c_pmu_write_counter,
>> +	.read_counter		= hisi_l3c_pmu_read_counter,
>> +};
>> +
>> +static int hisi_l3c_pmu_dev_probe(struct platform_device *pdev,
>> +				  struct hisi_pmu *l3c_pmu)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	int ret;
>> +
>> +	ret = hisi_l3c_pmu_init_data(pdev, l3c_pmu);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
>> +				       &l3c_pmu->node);
>> +	if (ret) {
>> +		dev_err(dev, "Error %d registering hotplug", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = hisi_l3c_pmu_init_irq(l3c_pmu, pdev);
>> +	if (ret)
>> +		goto fail;
>> +
>> +	l3c_pmu->name = devm_kasprintf(dev, GFP_KERNEL, "hisi_l3c%u_%u",
>> +				       l3c_pmu->l3c_tag_id, l3c_pmu->scl_id);
>> +	l3c_pmu->num_events = L3C_NR_EVENTS;
>> +	l3c_pmu->num_counters = L3C_NR_COUNTERS;
>> +	l3c_pmu->counter_bits = 48;
>> +	l3c_pmu->ops = &hisi_uncore_l3c_ops;
>> +	l3c_pmu->dev = dev;
>> +
>> +	return 0;
>> +
>> +fail:
>> +	cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
>> +				    &l3c_pmu->node);
>> +	return ret;
>> +}
>> +
>> +static int hisi_l3c_pmu_probe(struct platform_device *pdev)
>> +{
>> +	struct hisi_pmu *l3c_pmu;
>> +	struct device *dev = &pdev->dev;
>> +	int ret;
>> +
>> +	l3c_pmu = hisi_pmu_alloc(dev, L3C_NR_COUNTERS);
>> +	if (!l3c_pmu)
>> +		return -ENOMEM;
>> +
>> +	ret = hisi_l3c_pmu_dev_probe(pdev, l3c_pmu);
>> +	if (ret)
>> +		return ret;
>> +
>> +	l3c_pmu->pmu = (struct pmu) {
>> +		.name		= l3c_pmu->name,
>> +		.task_ctx_nr	= perf_invalid_context,
>> +		.event_init	= hisi_uncore_pmu_event_init,
>> +		.pmu_enable	= hisi_uncore_pmu_enable,
>> +		.pmu_disable	= hisi_uncore_pmu_disable,
>> +		.add		= hisi_uncore_pmu_add,
>> +		.del		= hisi_uncore_pmu_del,
>> +		.start		= hisi_uncore_pmu_start,
>> +		.stop		= hisi_uncore_pmu_stop,
>> +		.read		= hisi_uncore_pmu_read,
>> +		.attr_groups	= hisi_l3c_pmu_attr_groups,  
> Obviously not relevant to this patch as you work with what you
> have, but this is a structure that looks rather ripe for
> splitting into a const ops structure and a data structure
> containing the bits that tend to be dynamic (like the name.
> Perhaps it's not worth the hassle though.
>> +	};
>> +
>> +	ret = hisi_uncore_pmu_setup(l3c_pmu, l3c_pmu->name);  
> As mentioned for patch 2, I'd remove this wrapper.  That will make it
> clearer that the remove is reversing everything it needs to.
> 

Ok

>> +	if (ret) {
>> +		dev_err(l3c_pmu->dev, "hisi_uncore_pmu_setup failed!\n");
>> +		cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
>> +					    &l3c_pmu->node);
>> +		return ret;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, l3c_pmu);
>> +
>> +	return 0;
>> +}
>> +
>> +static int hisi_l3c_pmu_remove(struct platform_device *pdev)
>> +{
>> +	struct hisi_pmu *l3c_pmu = platform_get_drvdata(pdev);
>> +
>> +	perf_pmu_unregister(&l3c_pmu->pmu);
>> +	cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
>> +				    &l3c_pmu->node);  
> The related add_instance call is wrapped up in
> hisi_l3c_pmu_dev_probe. 
> I would suggest wrapping this last call up in a hisi_l3c_pmu_dev_remove
> function so that the balance between probe and remove is obvious.
> 
> Basic aim of the game is to make the code as easy to review as possible
> and this would make it slightly more readable.

Agree, shall fix it.

>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver hisi_l3c_pmu_driver = {
>> +	.driver = {
>> +		.name = "hisi_l3c_pmu",
>> +		.acpi_match_table = ACPI_PTR(hisi_l3c_pmu_acpi_match),
>> +	},
>> +	.probe = hisi_l3c_pmu_probe,
>> +	.remove = hisi_l3c_pmu_remove,
>> +};
>> +
>> +static int __init hisi_l3c_pmu_module_init(void)
>> +{
>> +	int ret;
>> +
>> +	ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
>> +				      "AP_PERF_ARM_HISI_L3_ONLINE",
>> +				      hisi_l3c_pmu_online_cpu,
>> +				      hisi_l3c_pmu_offline_cpu);
>> +	if (ret) {
>> +		pr_err("l3c_pmu_module_init: Error setup hotplug, ret=%d", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = platform_driver_register(&hisi_l3c_pmu_driver);
>> +	if (ret)
>> +		cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE);
>> +
>> +	return ret;
>> +}
>> +module_init(hisi_l3c_pmu_module_init);
>> +
>> +static void __exit hisi_l3c_pmu_module_exit(void)
>> +{
>> +	cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE);
>> +	platform_driver_unregister(&hisi_l3c_pmu_driver);  
> 
> Would normally expect an exit function to reverse the order of what
> goes on in the init function.  This is true even if it doesn't
> matter as it makes it 'obviously correct' and the reviewer doesn't
> have to think about it! So...
> 
> platform_driver_unregister(&hisi_l3c_pmu_driver);
> cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_L3_ONLINE);
> 
> If not there should be a comment explaining why not.
> 

Agree to reverse the order of the init function and shall modify it.

Thanks.
Shaokun

>> +}
>> +module_exit(hisi_l3c_pmu_module_exit);
>> +
>> +MODULE_DESCRIPTION("HiSilicon SoC L3C uncore PMU driver");
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR("Anurup M, Shaokun Zhang");
>> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
>> index b56573b..2eab157 100644
>> --- a/include/linux/cpuhotplug.h
>> +++ b/include/linux/cpuhotplug.h
>> @@ -136,6 +136,7 @@ enum cpuhp_state {
>>  	CPUHP_AP_PERF_S390_SF_ONLINE,
>>  	CPUHP_AP_PERF_ARM_CCI_ONLINE,
>>  	CPUHP_AP_PERF_ARM_CCN_ONLINE,
>> +	CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
>>  	CPUHP_AP_PERF_ARM_L2X0_ONLINE,
>>  	CPUHP_AP_PERF_ARM_QCOM_L2_ONLINE,
>>  	CPUHP_AP_PERF_ARM_QCOM_L3_ONLINE,  
> 
> 
> .
> 

Re: [PATCH v3 2/6] perf: hisi: Add support for HiSilicon SoC uncore PMU driver

[Zhangshaokun]

Hi Jonathan

On 2017/7/20 21:49, Jonathan Cameron wrote:
> On Thu, 20 Jul 2017 21:03:19 +0800
> Zhangshaokun <zhangshaokun@hisilicon.com> wrote:
> 
>> Hi Jonathan
>>
>> On 2017/7/19 17:19, Jonathan Cameron wrote:
>>> On Tue, 18 Jul 2017 15:59:55 +0800
>>> Shaokun Zhang <zhangshaokun@hisilicon.com> wrote:
>>>   
>>>> This patch adds support HiSilicon SoC uncore PMU driver framework and
>>>> interfaces.
>>>>
>>>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
>>>> Signed-off-by: Anurup M <anurup.m@huawei.com>    
>>> A couple of minor things inline.
>>>
> <snip>
>>>> +/* Generic pmu struct for different pmu types */
>>>> +struct hisi_pmu {
>>>> +	const char *name;
>>>> +	struct pmu pmu;
>>>> +	const struct hisi_uncore_ops *ops;
>>>> +	struct hisi_pmu_hwevents pmu_events;
>>>> +	cpumask_t cpus;
>>>> +	struct device *dev;
>>>> +	struct hlist_node node;
>>>> +	u32 scl_id;
>>>> +	u32 ccl_id;
>>>> +	/* Hardware information for different pmu types */
>>>> +	void __iomem *base;
>>>> +	union {
>>>> +		u32 ddrc_chn_id;
>>>> +		u32 l3c_tag_id;
>>>> +		u32 hha_uid;
>>>> +	};
>>>> +	int num_counters;
>>>> +	int num_events;
>>>> +	int counter_bits;
>>>> +};
>>>> +
>>>> +int hisi_uncore_pmu_counter_valid(struct hisi_pmu *hisi_pmu, int idx);
>>>> +int hisi_uncore_pmu_get_event_idx(struct perf_event *event);
>>>> +void hisi_uncore_pmu_clear_event_idx(struct hisi_pmu *hisi_pmu, int idx);    
>>> The above is only used in hisi_uncore_pmu.c so doesn't need to be here
>>> and can be static.
>>>   
>>
>> These functions would be called in L3C/HHA/DDR PMU driver. We want to give a
>> uncore perf framework in hisi_uncore_pmu.c for hisilicon uncore PMUs.
>>
> For all but the one function above that is true.  I couldn't find this one being
> used anywhere in those drivers.
> 

My apologies, it is what your said, thanks.

> 
>> Thanks.
>> Shaokun
>>
>>>> +void hisi_uncore_pmu_read(struct perf_event *event);
>>>> +int hisi_uncore_pmu_add(struct perf_event *event, int flags);
>>>> +void hisi_uncore_pmu_del(struct perf_event *event, int flags);
>>>> +void hisi_uncore_pmu_start(struct perf_event *event, int flags);
>>>> +void hisi_uncore_pmu_stop(struct perf_event *event, int flags);
>>>> +void hisi_uncore_pmu_set_event_period(struct perf_event *event);
>>>> +u64 hisi_uncore_pmu_event_update(struct perf_event *event);
>>>> +int hisi_uncore_pmu_event_init(struct perf_event *event);
>>>> +int hisi_uncore_pmu_setup(struct hisi_pmu *hisi_pmu, const char *pmu_name);
>>>> +void hisi_uncore_pmu_enable(struct pmu *pmu);
>>>> +void hisi_uncore_pmu_disable(struct pmu *pmu);
>>>> +struct hisi_pmu *hisi_pmu_alloc(struct device *dev, u32 num_cntrs);
>>>> +ssize_t hisi_event_sysfs_show(struct device *dev,
>>>> +			      struct device_attribute *attr, char *buf);
>>>> +ssize_t hisi_format_sysfs_show(struct device *dev,
>>>> +			       struct device_attribute *attr, char *buf);
>>>> +ssize_t hisi_cpumask_sysfs_show(struct device *dev,
>>>> +				struct device_attribute *attr, char *buf);
>>>> +void hisi_read_scl_and_ccl_id(u32 *scl_id, u32 *ccl_id);
>>>> +#endif /* __HISI_UNCORE_PMU_H__ */    
>>>
>>>
>>> .
>>>   
>>
> 
> 
> 
> .
>