[PATCH v4 0/2] qcom: add l2 cache perf events driver

[PATCH v4 0/2] qcom: add l2 cache perf events driver

[Neil Leeder]

This adds a new dynamic PMU to the perf events framework to program
and control the L2 cache PMUs in Qualcomm Centriq SOCs.

The driver exports formatting and event information to sysfs so it
can be used by the perf user space tools with the syntax:
perf stat -a -e l2cache_0/event=0x42/

Qualcomm Technologies PMUs have events arranged in a matrix of rows and columns.
Only one event can be enabled from each column at once. This is enforced
by the filter_match callback.

v4:
Replace notifier with hotplug statemachine
Allocate PMU struct dynamically

v3:
Remove exports from l2-accessors
Change l2-accessors Kconfig to make it not user-selectable
Reorder and remove unnecessary includes

v2:

Add the l2-accessors patch to this patchset, previously posted separately.
Remove sampling and per-task functionality for this uncore PMU.
Use cpumask to replace code which filtered events to one cpu per slice.
Replace manual event filtering with filter_match callback.
Use a separate used_mask for event groups.
Add hotplug notifier for CPU and irq migration.
Remove extraneous synchronisation instructions.
Other miscellaneous cleanup.

Neil Leeder (2):
  soc: qcom: provide mechanism for drivers to access L2 registers
  soc: qcom: add l2 cache perf events driver

 drivers/soc/qcom/Kconfig               |  16 +
 drivers/soc/qcom/Makefile              |   2 +
 drivers/soc/qcom/l2-accessors.c        |  63 +++
 drivers/soc/qcom/perf_event_l2.c       | 855 +++++++++++++++++++++++++++++++++
 include/linux/cpuhotplug.h             |   1 +
 include/linux/soc/qcom/l2-accessors.h  |  20 +
 include/linux/soc/qcom/perf_event_l2.h |  79 +++
 7 files changed, 1036 insertions(+)
 create mode 100644 drivers/soc/qcom/l2-accessors.c
 create mode 100644 drivers/soc/qcom/perf_event_l2.c
 create mode 100644 include/linux/soc/qcom/l2-accessors.h
 create mode 100644 include/linux/soc/qcom/perf_event_l2.h

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[PATCH v4 1/2] soc: qcom: provide mechanism for drivers to access L2 registers

[Neil Leeder]

L2 registers are accessed using a select register and data
register pair. To prevent multiple concurrent writes to the
select register by independent drivers, the write to the
select register and the associated access of the data register
are protected with a lock. All drivers accessing the L2
registers use the set and get functions provided by
l2-accessors to ensure correct reads and writes to L2 registers.

Signed-off-by: Neil Leeder <nleeder@codeaurora.org>
---
 drivers/soc/qcom/Kconfig              |  6 ++++
 drivers/soc/qcom/Makefile             |  1 +
 drivers/soc/qcom/l2-accessors.c       | 63 +++++++++++++++++++++++++++++++++++
 include/linux/soc/qcom/l2-accessors.h | 20 +++++++++++
 4 files changed, 90 insertions(+)
 create mode 100644 drivers/soc/qcom/l2-accessors.c
 create mode 100644 include/linux/soc/qcom/l2-accessors.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 461b387..ddd6b71 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -10,6 +10,12 @@ config QCOM_GSBI
           functions for connecting the underlying serial UART, SPI, and I2C
           devices to the output pins.
 
+config QCOM_L2_ACCESSORS
+	bool
+	help
+	  Provides support for accessing registers in the L2 cache
+	  for Qualcomm Technologies ARM64 chips.
+
 config QCOM_PM
 	bool "Qualcomm Power Management"
 	depends on ARCH_QCOM && !ARM64
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index fdd664e..6ef29b9 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -1,4 +1,5 @@
 obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
+obj-$(CONFIG_QCOM_L2_ACCESSORS) += l2-accessors.o
 obj-$(CONFIG_QCOM_PM)	+=	spm.o
 obj-$(CONFIG_QCOM_SMD) +=	smd.o
 obj-$(CONFIG_QCOM_SMD_RPM)	+= smd-rpm.o
diff --git a/drivers/soc/qcom/l2-accessors.c b/drivers/soc/qcom/l2-accessors.c
new file mode 100644
index 0000000..2625d33
--- /dev/null
+++ b/drivers/soc/qcom/l2-accessors.c
@@ -0,0 +1,63 @@
+/*
+ * Copyright (c) 2014-2016 The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only 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.
+ */
+
+#include <linux/spinlock.h>
+#include <linux/soc/qcom/l2-accessors.h>
+#include <asm/cputype.h>
+#include <asm/sysreg.h>
+
+#define	L2CPUSRSELR_EL1	S3_3_c15_c0_6
+#define	L2CPUSRDR_EL1	S3_3_c15_c0_7
+
+static DEFINE_RAW_SPINLOCK(l2_access_lock);
+
+/**
+ * set_l2_indirect_reg: write value to an L2 register
+ * @reg: Address of L2 register.
+ * @value: Value to be written to register.
+ *
+ * Use architecturally required barriers for ordering between system register
+ * accesses
+ */
+void set_l2_indirect_reg(u64 reg, u64 val)
+{
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&l2_access_lock, flags);
+	write_sysreg(reg, L2CPUSRSELR_EL1);
+	isb();
+	write_sysreg(val, L2CPUSRDR_EL1);
+	isb();
+	raw_spin_unlock_irqrestore(&l2_access_lock, flags);
+}
+
+/**
+ * get_l2_indirect_reg: read an L2 register value
+ * @reg: Address of L2 register.
+ *
+ * Use architecturally required barriers for ordering between system register
+ * accesses
+ */
+u64 get_l2_indirect_reg(u64 reg)
+{
+	u64 val;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&l2_access_lock, flags);
+	write_sysreg(reg, L2CPUSRSELR_EL1);
+	isb();
+	val = read_sysreg(L2CPUSRDR_EL1);
+	raw_spin_unlock_irqrestore(&l2_access_lock, flags);
+
+	return val;
+}
diff --git a/include/linux/soc/qcom/l2-accessors.h b/include/linux/soc/qcom/l2-accessors.h
new file mode 100644
index 0000000..e51b72a
--- /dev/null
+++ b/include/linux/soc/qcom/l2-accessors.h
@@ -0,0 +1,20 @@
+/*
+ * Copyright (c) 2011-2016 The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only 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.
+ */
+
+#ifndef __QCOM_L2_ACCESSORS_H
+#define __QCOM_L2_ACCESSORS_H
+
+void set_l2_indirect_reg(u64 reg_addr, u64 val);
+u64 get_l2_indirect_reg(u64 reg_addr);
+
+#endif
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[PATCH v4 2/2] soc: qcom: add l2 cache perf events driver

[Neil Leeder]

Adds perf events support for L2 cache PMU.

The L2 cache PMU driver is named 'l2cache_0' and can be used
with perf events to profile L2 events such as cache hits
and misses.

Signed-off-by: Neil Leeder <nleeder@codeaurora.org>
---
 drivers/soc/qcom/Kconfig               |  10 +
 drivers/soc/qcom/Makefile              |   1 +
 drivers/soc/qcom/perf_event_l2.c       | 855 +++++++++++++++++++++++++++++++++
 include/linux/cpuhotplug.h             |   1 +
 include/linux/soc/qcom/perf_event_l2.h |  79 +++
 5 files changed, 946 insertions(+)
 create mode 100644 drivers/soc/qcom/perf_event_l2.c
 create mode 100644 include/linux/soc/qcom/perf_event_l2.h

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index ddd6b71..7e71d8d 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -16,6 +16,16 @@ config QCOM_L2_ACCESSORS
 	  Provides support for accessing registers in the L2 cache
 	  for Qualcomm Technologies ARM64 chips.
 
+config QCOM_PERF_EVENTS_L2
+	bool "Qualcomm Technologies L2-cache perf events"
+	depends on ARCH_QCOM && ARM64 && HW_PERF_EVENTS && ACPI
+	select QCOM_L2_ACCESSORS
+	  help
+	  Provides support for the L2 cache performance monitor unit (PMU)
+	  in Qualcomm Technologies processors.
+	  Adds the L2 cache PMU into the perf events subsystem for
+	  monitoring L2 cache events.
+
 config QCOM_PM
 	bool "Qualcomm Power Management"
 	depends on ARCH_QCOM && !ARM64
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 6ef29b9..c8e89ca9 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -1,5 +1,6 @@
 obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
 obj-$(CONFIG_QCOM_L2_ACCESSORS) += l2-accessors.o
+obj-$(CONFIG_QCOM_PERF_EVENTS_L2)	+= perf_event_l2.o
 obj-$(CONFIG_QCOM_PM)	+=	spm.o
 obj-$(CONFIG_QCOM_SMD) +=	smd.o
 obj-$(CONFIG_QCOM_SMD_RPM)	+= smd-rpm.o
diff --git a/drivers/soc/qcom/perf_event_l2.c b/drivers/soc/qcom/perf_event_l2.c
new file mode 100644
index 0000000..5c13e87
--- /dev/null
+++ b/drivers/soc/qcom/perf_event_l2.c
@@ -0,0 +1,855 @@
+/* Copyright (c) 2015,2016 The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only 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.
+ */
+#define pr_fmt(fmt) "l2 perfevents: " fmt
+
+#include <linux/acpi.h>
+#include <linux/interrupt.h>
+#include <linux/perf_event.h>
+#include <linux/platform_device.h>
+#include <linux/soc/qcom/l2-accessors.h>
+#include <linux/soc/qcom/perf_event_l2.h>
+
+/*
+ * Aggregate PMU. Implements the core pmu functions and manages
+ * the hardware PMUs.
+ */
+struct l2cache_pmu {
+	struct list_head entry;
+	u32 num_pmus;
+	struct pmu pmu;
+	int num_counters;
+	cpumask_t cpumask;
+	struct platform_device *pdev;
+};
+
+/*
+ * The cache is made-up of one or more slices, each slice has its own PMU.
+ * This structure represents one of the hardware PMUs.
+ */
+struct hml2_pmu {
+	struct perf_event *events[MAX_L2_CTRS];
+	struct l2cache_pmu *l2cache_pmu;
+	unsigned long used_mask[BITS_TO_LONGS(MAX_L2_CTRS)];
+	unsigned long group_used_mask[BITS_TO_LONGS(L2_EVT_GROUP_MAX + 1)];
+	int group_to_counter[L2_EVT_GROUP_MAX + 1];
+	int irq;
+	/* The CPU that is used for collecting events on this slice */
+	int on_cpu;
+	/* All the CPUs associated with this slice */
+	cpumask_t slice_cpus;
+	atomic64_t prev_count[MAX_L2_CTRS];
+	spinlock_t pmu_lock;
+};
+
+#define to_l2cache_pmu(p) (container_of(p, struct l2cache_pmu, pmu))
+
+static DEFINE_MUTEX(l2cache_pmu_mutex);
+static LIST_HEAD(l2cache_pmu_list);
+static DEFINE_PER_CPU(struct hml2_pmu *, cpu_to_pmu);
+static u32 l2_cycle_ctr_idx;
+static u32 l2_reset_mask;
+
+static inline u32 idx_to_reg_bit(u32 idx)
+{
+	u32 bit;
+
+	if (idx == l2_cycle_ctr_idx)
+		bit = BIT(L2CYCLE_CTR_BIT);
+	else
+		bit = BIT(idx);
+	return bit;
+}
+
+static inline struct hml2_pmu *get_hml2_pmu(int cpu)
+{
+	return per_cpu(cpu_to_pmu, cpu);
+}
+
+static void hml2_pmu__reset_on_slice(void *x)
+{
+	/* Reset all ctrs */
+	set_l2_indirect_reg(L2PMCR, L2PMCR_RESET_ALL);
+	set_l2_indirect_reg(L2PMCNTENCLR, l2_reset_mask);
+	set_l2_indirect_reg(L2PMINTENCLR, l2_reset_mask);
+	set_l2_indirect_reg(L2PMOVSCLR, l2_reset_mask);
+}
+
+static inline void hml2_pmu__reset(struct hml2_pmu *slice)
+{
+	int cpu;
+
+	if (cpumask_test_cpu(smp_processor_id(), &slice->slice_cpus)) {
+		hml2_pmu__reset_on_slice(NULL);
+		return;
+	}
+
+	/* Call each cpu in the cluster until one works */
+	for_each_cpu(cpu, &slice->slice_cpus) {
+		if (!smp_call_function_single(cpu, hml2_pmu__reset_on_slice,
+					      NULL, 1))
+			return;
+	}
+
+	dev_err(&slice->l2cache_pmu->pdev->dev,
+		"Failed to reset on cluster with cpu %d\n",
+		cpumask_first(&slice->slice_cpus));
+}
+
+static inline void hml2_pmu__enable(void)
+{
+	set_l2_indirect_reg(L2PMCR, L2PMCR_GLOBAL_ENABLE);
+}
+
+static inline void hml2_pmu__disable(void)
+{
+	set_l2_indirect_reg(L2PMCR, L2PMCR_GLOBAL_DISABLE);
+}
+
+static inline void hml2_pmu__counter_set_value(u32 idx, u64 value)
+{
+	u32 counter_reg;
+
+	if (idx == l2_cycle_ctr_idx) {
+		set_l2_indirect_reg(L2PMCCNTR, value);
+	} else {
+		counter_reg = (idx * IA_L2_REG_OFFSET) + IA_L2PMXEVCNTR_BASE;
+		set_l2_indirect_reg(counter_reg, (u32)(value & GENMASK(31, 0)));
+	}
+}
+
+static inline u64 hml2_pmu__counter_get_value(u32 idx)
+{
+	u64 value;
+	u32 counter_reg;
+
+	if (idx == l2_cycle_ctr_idx) {
+		value = get_l2_indirect_reg(L2PMCCNTR);
+	} else {
+		counter_reg = (idx * IA_L2_REG_OFFSET) + IA_L2PMXEVCNTR_BASE;
+		value = get_l2_indirect_reg(counter_reg);
+	}
+
+	return value;
+}
+
+static inline void hml2_pmu__counter_enable(u32 idx)
+{
+	u32 reg;
+
+	reg = get_l2_indirect_reg(L2PMCNTENSET);
+	reg |= idx_to_reg_bit(idx);
+	set_l2_indirect_reg(L2PMCNTENSET, reg);
+}
+
+static inline void hml2_pmu__counter_disable(u32 idx)
+{
+	set_l2_indirect_reg(L2PMCNTENCLR, idx_to_reg_bit(idx));
+}
+
+static inline void hml2_pmu__counter_enable_interrupt(u32 idx)
+{
+	u32 reg;
+
+	reg = get_l2_indirect_reg(L2PMINTENSET);
+	reg |= idx_to_reg_bit(idx);
+	set_l2_indirect_reg(L2PMINTENSET, reg);
+}
+
+static inline void hml2_pmu__counter_disable_interrupt(u32 idx)
+{
+	set_l2_indirect_reg(L2PMINTENCLR, idx_to_reg_bit(idx));
+}
+
+static inline void hml2_pmu__set_evcntcr(u32 ctr, u32 val)
+{
+	u32 evtcr_reg = (ctr * IA_L2_REG_OFFSET) + IA_L2PMXEVCNTCR_BASE;
+
+	set_l2_indirect_reg(evtcr_reg, val);
+}
+
+static inline void hml2_pmu__set_evtyper(u32 ctr, u32 val)
+{
+	u32 evtype_reg = (ctr * IA_L2_REG_OFFSET) + IA_L2PMXEVTYPER_BASE;
+
+	set_l2_indirect_reg(evtype_reg, val);
+}
+
+static void hml2_pmu__set_resr(struct hml2_pmu *slice,
+			       u32 event_group, u32 event_cc)
+{
+	u64 field;
+	u64 resr_val;
+	u32 shift;
+	unsigned long iflags;
+
+	shift = L2PMRESR_GROUP_BITS * event_group;
+	field = ((u64)(event_cc & L2PMRESR_GROUP_MASK) << shift) | L2PMRESR_EN;
+
+	spin_lock_irqsave(&slice->pmu_lock, iflags);
+
+	resr_val = get_l2_indirect_reg(L2PMRESR);
+	resr_val &= ~(L2PMRESR_GROUP_MASK << shift);
+	resr_val |= field;
+	set_l2_indirect_reg(L2PMRESR, resr_val);
+
+	spin_unlock_irqrestore(&slice->pmu_lock, iflags);
+}
+
+static inline void hml2_pmu__set_evfilter_sys_mode(u32 ctr)
+{
+	set_l2_indirect_reg((ctr * IA_L2_REG_OFFSET) + IA_L2PMXEVFILTER_BASE,
+			    L2PMXEVFILTER_SUFILTER_ALL |
+			    L2PMXEVFILTER_ORGFILTER_IDINDEP |
+			    L2PMXEVFILTER_ORGFILTER_ALL);
+}
+
+static inline u32 hml2_pmu__getreset_ovsr(void)
+{
+	u32 result = get_l2_indirect_reg(L2PMOVSSET);
+
+	set_l2_indirect_reg(L2PMOVSCLR, result);
+	return result;
+}
+
+static inline bool hml2_pmu__has_overflowed(u32 ovsr)
+{
+	return !!(ovsr & l2_reset_mask);
+}
+
+static inline bool hml2_pmu__counter_has_overflowed(u32 ovsr, u32 idx)
+{
+	return !!(ovsr & idx_to_reg_bit(idx));
+}
+
+static void l2_cache__event_update_from_slice(struct perf_event *event,
+					      struct hml2_pmu *slice)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	u64 delta64, prev, now;
+	u32 delta;
+	u32 idx = hwc->idx;
+
+	do {
+		prev = atomic64_read(&slice->prev_count[idx]);
+		now = hml2_pmu__counter_get_value(idx);
+	} while (atomic64_cmpxchg(&slice->prev_count[idx], prev, now) != prev);
+
+	if (idx == l2_cycle_ctr_idx) {
+		/*
+		 * The cycle counter is 64-bit so needs separate handling
+		 * of 64-bit delta.
+		 */
+		delta64 = now - prev;
+		local64_add(delta64, &event->count);
+	} else {
+		/*
+		 * 32-bit counters need the unsigned 32-bit math to handle
+		 * overflow and now < prev
+		 */
+		delta = now - prev;
+		local64_add(delta, &event->count);
+	}
+}
+
+static void l2_cache__slice_set_period(struct hml2_pmu *slice,
+				       struct hw_perf_event *hwc)
+{
+	u64 value = L2_MAX_PERIOD - (L2_CNT_PERIOD - 1);
+	u32 idx = hwc->idx;
+	u64 prev = atomic64_read(&slice->prev_count[idx]);
+
+	if (prev < value) {
+		value += prev;
+		atomic64_set(&slice->prev_count[idx], value);
+	} else {
+		value = prev;
+	}
+
+	hml2_pmu__counter_set_value(idx, value);
+}
+
+static int l2_cache__get_event_idx(struct hml2_pmu *slice,
+				   struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	int idx;
+
+	if (hwc->config_base == L2CYCLE_CTR_RAW_CODE) {
+		if (test_and_set_bit(l2_cycle_ctr_idx, slice->used_mask))
+			return -EAGAIN;
+
+		return l2_cycle_ctr_idx;
+	}
+
+	for (idx = 0; idx < slice->l2cache_pmu->num_counters - 1; idx++) {
+		if (!test_and_set_bit(idx, slice->used_mask)) {
+			set_bit(L2_EVT_GROUP(hwc->config_base),
+				slice->group_used_mask);
+			return idx;
+		}
+	}
+
+	/* The counters are all in use. */
+	return -EAGAIN;
+}
+
+static void l2_cache__clear_event_idx(struct hml2_pmu *slice,
+				      struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+
+	clear_bit(L2_EVT_GROUP(hwc->config_base), slice->group_used_mask);
+}
+
+static irqreturn_t l2_cache__handle_irq(int irq_num, void *data)
+{
+	struct hml2_pmu *slice = data;
+	u32 ovsr;
+	int idx;
+
+	ovsr = hml2_pmu__getreset_ovsr();
+	if (!hml2_pmu__has_overflowed(ovsr))
+		return IRQ_NONE;
+
+	for (idx = 0; idx < slice->l2cache_pmu->num_counters; idx++) {
+		struct perf_event *event = slice->events[idx];
+		struct hw_perf_event *hwc;
+
+		if (!event)
+			continue;
+
+		if (!hml2_pmu__counter_has_overflowed(ovsr, idx))
+			continue;
+
+		l2_cache__event_update_from_slice(event, slice);
+		hwc = &event->hw;
+
+		l2_cache__slice_set_period(slice, hwc);
+	}
+
+	/*
+	 * Handle the pending perf events.
+	 *
+	 * Note: this call *must* be run with interrupts disabled. For
+	 * platforms that can have the PMU interrupts raised as an NMI, this
+	 * will not work.
+	 */
+	irq_work_run();
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * Implementation of abstract pmu functionality required by
+ * the core perf events code.
+ */
+
+static void l2_cache__pmu_enable(struct pmu *pmu)
+{
+	hml2_pmu__enable();
+}
+
+static void l2_cache__pmu_disable(struct pmu *pmu)
+{
+	hml2_pmu__disable();
+}
+
+static int l2_cache__event_init(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	struct hml2_pmu *slice;
+	struct perf_event *sibling;
+	struct l2cache_pmu *l2cache_pmu = to_l2cache_pmu(event->pmu);
+
+	if (event->attr.type != l2cache_pmu->pmu.type)
+		return -ENOENT;
+
+	if (hwc->sample_period) {
+		dev_warn(&l2cache_pmu->pdev->dev, "Sampling not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (event->cpu < 0) {
+		dev_warn(&l2cache_pmu->pdev->dev, "Per-task mode not supported\n");
+		return -EOPNOTSUPP;
+	}
+
+	/* We cannot filter accurately so we just don't allow it. */
+	if (event->attr.exclude_user || event->attr.exclude_kernel ||
+	    event->attr.exclude_hv || event->attr.exclude_idle) {
+		dev_warn(&l2cache_pmu->pdev->dev, "Can't exclude execution levels\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (((L2_EVT_GROUP(event->attr.config) > L2_EVT_GROUP_MAX) ||
+	    (L2_EVT_PREFIX(event->attr.config) != 0) ||
+	    (L2_EVT_REG(event->attr.config) != 0)) &&
+	    (event->attr.config != L2CYCLE_CTR_RAW_CODE)) {
+		dev_warn(&l2cache_pmu->pdev->dev, "Invalid config %llx\n",
+			 event->attr.config);
+		return -EINVAL;
+	}
+
+	/* Don't allow groups with mixed PMUs, except for s/w events */
+	if (event->group_leader->pmu != event->pmu &&
+	    !is_software_event(event->group_leader)) {
+		dev_warn(&l2cache_pmu->pdev->dev,
+			 "Can't create mixed PMU group\n");
+		return -EINVAL;
+	}
+
+	list_for_each_entry(sibling, &event->group_leader->sibling_list,
+			    group_entry)
+		if (sibling->pmu != event->pmu &&
+		    !is_software_event(sibling)) {
+			dev_warn(&l2cache_pmu->pdev->dev,
+				 "Can't create mixed PMU group\n");
+			return -EINVAL;
+		}
+
+	hwc->idx = -1;
+	hwc->config_base = event->attr.config;
+
+	/*
+	 * Ensure all events are on the same cpu so all events are in the
+	 * same cpu context, to avoid races on pmu_enable etc.
+	 */
+	slice = get_hml2_pmu(event->cpu);
+	event->cpu = slice->on_cpu;
+
+	return 0;
+}
+
+static void l2_cache__event_update(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+
+	if (hwc->idx < 0)
+		return;
+
+	l2_cache__event_update_from_slice(event, get_hml2_pmu(event->cpu));
+}
+
+static void l2_cache__event_start(struct perf_event *event, int flags)
+{
+	struct hml2_pmu *slice;
+	struct hw_perf_event *hwc = &event->hw;
+	int idx = hwc->idx;
+	u32 config;
+	u32 evt_prefix, event_cc, event_group;
+
+	if (idx < 0)
+		return;
+
+	hwc->state = 0;
+
+	slice = get_hml2_pmu(event->cpu);
+	l2_cache__slice_set_period(slice, hwc);
+
+	if (hwc->config_base == L2CYCLE_CTR_RAW_CODE)
+		goto out;
+
+	config = hwc->config_base;
+	evt_prefix  = L2_EVT_PREFIX(config);
+	event_cc    = L2_EVT_CODE(config);
+	event_group = L2_EVT_GROUP(config);
+
+	hml2_pmu__set_evcntcr(idx, 0x0);
+	hml2_pmu__set_evtyper(idx, event_group);
+	hml2_pmu__set_resr(slice, event_group, event_cc);
+	hml2_pmu__set_evfilter_sys_mode(idx);
+out:
+	hml2_pmu__counter_enable_interrupt(idx);
+	hml2_pmu__counter_enable(idx);
+}
+
+static void l2_cache__event_stop(struct perf_event *event, int flags)
+{
+	struct hml2_pmu *slice;
+	struct hw_perf_event *hwc = &event->hw;
+	int idx = hwc->idx;
+
+	if (idx < 0)
+		return;
+
+	if (!(hwc->state & PERF_HES_STOPPED)) {
+		slice = get_hml2_pmu(event->cpu);
+		hml2_pmu__counter_disable_interrupt(idx);
+		hml2_pmu__counter_disable(idx);
+
+		if (flags & PERF_EF_UPDATE)
+			l2_cache__event_update(event);
+		hwc->state |= PERF_HES_STOPPED | PERF_HES_UPTODATE;
+	}
+}
+
+static int l2_cache__event_add(struct perf_event *event, int flags)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	int idx;
+	int err = 0;
+	struct hml2_pmu *slice;
+
+	slice = get_hml2_pmu(event->cpu);
+
+	idx = l2_cache__get_event_idx(slice, event);
+	if (idx < 0) {
+		err = idx;
+		goto out;
+	}
+
+	hwc->idx = idx;
+	hwc->state = PERF_HES_STOPPED | PERF_HES_UPTODATE;
+	slice->events[idx] = event;
+	slice->group_to_counter[L2_EVT_GROUP(hwc->config_base)] = idx;
+	atomic64_set(&slice->prev_count[idx], 0ULL);
+
+	if (flags & PERF_EF_START)
+		l2_cache__event_start(event, flags);
+
+	/* Propagate changes to the userspace mapping. */
+	perf_event_update_userpage(event);
+
+out:
+	return err;
+}
+
+static void l2_cache__event_del(struct perf_event *event, int flags)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	struct hml2_pmu *slice;
+	int idx = hwc->idx;
+
+	if (idx < 0)
+		return;
+
+	slice = get_hml2_pmu(event->cpu);
+	l2_cache__event_stop(event, flags | PERF_EF_UPDATE);
+	slice->events[idx] = NULL;
+	clear_bit(idx, slice->used_mask);
+	l2_cache__clear_event_idx(slice, event);
+
+	perf_event_update_userpage(event);
+}
+
+static void l2_cache__event_read(struct perf_event *event)
+{
+	l2_cache__event_update(event);
+}
+
+static int l2_cache_filter_match(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	struct hml2_pmu *slice = get_hml2_pmu(event->cpu);
+	unsigned int group = L2_EVT_GROUP(hwc->config_base);
+
+	/* check for column exclusion: group already in use by another event */
+	if (test_bit(group, slice->group_used_mask) &&
+	    slice->events[slice->group_to_counter[group]] != event)
+		return 0;
+
+	return 1;
+}
+
+static ssize_t l2_cache_pmu_cpumask_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct l2cache_pmu *l2cache_pmu = to_l2cache_pmu(dev_get_drvdata(dev));
+
+	return cpumap_print_to_pagebuf(true, buf, &l2cache_pmu->cpumask);
+}
+
+static struct device_attribute l2_cache_pmu_cpumask_attr =
+		__ATTR(cpumask, S_IRUGO, l2_cache_pmu_cpumask_show, NULL);
+
+static struct attribute *l2_cache_pmu_cpumask_attrs[] = {
+	&l2_cache_pmu_cpumask_attr.attr,
+	NULL,
+};
+
+static struct attribute_group l2_cache_pmu_cpumask_group = {
+	.attrs = l2_cache_pmu_cpumask_attrs,
+};
+
+/* NRCCG format for perf RAW codes. */
+PMU_FORMAT_ATTR(l2_prefix, "config:16-19");
+PMU_FORMAT_ATTR(l2_reg,    "config:12-15");
+PMU_FORMAT_ATTR(l2_code,   "config:4-11");
+PMU_FORMAT_ATTR(l2_grp,    "config:0-3");
+static struct attribute *l2_cache_pmu_formats[] = {
+	&format_attr_l2_prefix.attr,
+	&format_attr_l2_reg.attr,
+	&format_attr_l2_code.attr,
+	&format_attr_l2_grp.attr,
+	NULL,
+};
+
+static struct attribute_group l2_cache_pmu_format_group = {
+	.name = "format",
+	.attrs = l2_cache_pmu_formats,
+};
+
+static const struct attribute_group *l2_cache_pmu_attr_grps[] = {
+	&l2_cache_pmu_format_group,
+	&l2_cache_pmu_cpumask_group,
+	NULL,
+};
+
+/*
+ * Generic device handlers
+ */
+
+static const struct acpi_device_id l2_cache_pmu_acpi_match[] = {
+	{ "QCOM8130", },
+	{ }
+};
+
+static int get_num_counters(void)
+{
+	int val;
+
+	val = get_l2_indirect_reg(L2PMCR);
+
+	/*
+	 * Read number of counters from L2PMCR and add 1
+	 * for the cycle counter.
+	 */
+	return ((val >> L2PMCR_NUM_EV_SHIFT) & L2PMCR_NUM_EV_MASK) + 1;
+}
+
+static int l2cache_pmu_online_cpu(unsigned int cpu)
+{
+	struct hml2_pmu *slice;
+	struct l2cache_pmu *l2cache_pmu;
+	cpumask_t slice_online_cpus;
+
+	mutex_lock(&l2cache_pmu_mutex);
+	list_for_each_entry(l2cache_pmu, &l2cache_pmu_list, entry) {
+		slice = get_hml2_pmu(cpu);
+		cpumask_and(&slice_online_cpus, &slice->slice_cpus,
+			    cpu_online_mask);
+		if (cpumask_weight(&slice_online_cpus) == 1) {
+			/* all CPUs on this slice were down, use this one */
+			slice->on_cpu = cpu;
+			cpumask_set_cpu(cpu, &l2cache_pmu->cpumask);
+			WARN_ON(irq_set_affinity(slice->irq, cpumask_of(cpu)));
+		}
+	}
+	mutex_unlock(&l2cache_pmu_mutex);
+
+	return 0;
+}
+
+static int l2cache_pmu_offline_cpu(unsigned int cpu)
+{
+	struct hml2_pmu *slice;
+	struct l2cache_pmu *l2cache_pmu;
+	cpumask_t slice_online_cpus;
+	unsigned int target;
+
+	mutex_lock(&l2cache_pmu_mutex);
+	list_for_each_entry(l2cache_pmu, &l2cache_pmu_list, entry) {
+		if (!cpumask_test_and_clear_cpu(cpu, &l2cache_pmu->cpumask))
+			break;
+		slice = get_hml2_pmu(cpu);
+		cpumask_and(&slice_online_cpus, &slice->slice_cpus,
+			    cpu_online_mask);
+		/* Any other CPU for this slice which is still online */
+		target = cpumask_any_but(&slice_online_cpus, cpu);
+		if (target >= nr_cpu_ids)
+			break;
+		perf_pmu_migrate_context(&l2cache_pmu->pmu, cpu, target);
+		slice->on_cpu = target;
+		cpumask_set_cpu(target, &l2cache_pmu->cpumask);
+		WARN_ON(irq_set_affinity(slice->irq, cpumask_of(target)));
+	}
+	mutex_unlock(&l2cache_pmu_mutex);
+
+	return 0;
+}
+
+static int l2_cache_pmu_probe_slice(struct device *dev, void *data)
+{
+	struct platform_device *pdev = to_platform_device(dev->parent);
+	struct platform_device *sdev = to_platform_device(dev);
+	struct l2cache_pmu *l2cache_pmu = data;
+	struct hml2_pmu *slice;
+	struct acpi_device *device;
+	int irq;
+	int err;
+	int logical_cpu;
+	unsigned long fw_slice_id;
+
+	if (acpi_bus_get_device(ACPI_HANDLE(dev), &device))
+		return -ENODEV;
+
+	if (kstrtol(device->pnp.unique_id, 10, &fw_slice_id) < 0) {
+		dev_err(&pdev->dev, "unable to read ACPI uid\n");
+		return -ENODEV;
+	}
+
+	irq = platform_get_irq(sdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev,
+			"Failed to get valid irq for slice %ld\n", fw_slice_id);
+		return irq;
+	}
+
+	slice = devm_kzalloc(&pdev->dev, sizeof(*slice), GFP_KERNEL);
+	if (!slice)
+		return -ENOMEM;
+
+	slice->l2cache_pmu = l2cache_pmu;
+	for_each_present_cpu(logical_cpu) {
+		if (topology_physical_package_id(logical_cpu) == fw_slice_id) {
+			cpumask_set_cpu(logical_cpu, &slice->slice_cpus);
+			per_cpu(cpu_to_pmu, logical_cpu) = slice;
+		}
+	}
+	slice->irq = irq;
+
+	if (cpumask_empty(&slice->slice_cpus)) {
+		dev_err(&pdev->dev, "No CPUs found for L2 cache instance %ld\n",
+			fw_slice_id);
+		return -ENODEV;
+	}
+
+	/* Pick one CPU to be the preferred one to use in the slice */
+	slice->on_cpu = cpumask_first(&slice->slice_cpus);
+
+	if (irq_set_affinity(irq, cpumask_of(slice->on_cpu))) {
+		dev_err(&pdev->dev,
+			"Unable to set irq affinity (irq=%d, cpu=%d)\n",
+			irq, slice->on_cpu);
+		return -ENODEV;
+	}
+
+	err = devm_request_irq(
+		&pdev->dev, irq, l2_cache__handle_irq,
+		IRQF_NOBALANCING, "l2-cache-pmu", slice);
+	if (err) {
+		dev_err(&pdev->dev,
+			"Unable to request IRQ%d for L2 PMU counters\n",
+			irq);
+		return err;
+	}
+
+	dev_info(&pdev->dev,
+		 "Registered L2 cache PMU instance %ld with %d CPUs\n",
+		 fw_slice_id, cpumask_weight(&slice->slice_cpus));
+
+	slice->pmu_lock = __SPIN_LOCK_UNLOCKED(slice->pmu_lock);
+	cpumask_set_cpu(slice->on_cpu, &l2cache_pmu->cpumask);
+
+	hml2_pmu__reset(slice);
+	l2cache_pmu->num_pmus++;
+
+	return 0;
+}
+
+static int l2_cache_pmu_probe(struct platform_device *pdev)
+{
+	int err;
+	struct l2cache_pmu *l2cache_pmu;
+
+	l2cache_pmu =
+		devm_kzalloc(&pdev->dev, sizeof(*l2cache_pmu), GFP_KERNEL);
+	if (!l2cache_pmu)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, l2cache_pmu);
+	l2cache_pmu->pmu = (struct pmu) {
+		/* suffix is instance id for future use with multiple sockets */
+		.name		= "l2cache_0",
+		.task_ctx_nr    = perf_invalid_context,
+		.pmu_enable	= l2_cache__pmu_enable,
+		.pmu_disable	= l2_cache__pmu_disable,
+		.event_init	= l2_cache__event_init,
+		.add		= l2_cache__event_add,
+		.del		= l2_cache__event_del,
+		.start		= l2_cache__event_start,
+		.stop		= l2_cache__event_stop,
+		.read		= l2_cache__event_read,
+		.attr_groups	= l2_cache_pmu_attr_grps,
+		.filter_match   = l2_cache_filter_match,
+	};
+
+	l2cache_pmu->num_counters = get_num_counters();
+	l2cache_pmu->pdev = pdev;
+	l2_cycle_ctr_idx = l2cache_pmu->num_counters - 1;
+	l2_reset_mask = GENMASK(l2cache_pmu->num_counters - 2, 0) |
+		L2PM_CC_ENABLE;
+
+	cpumask_clear(&l2cache_pmu->cpumask);
+
+	/* Read slice info and initialize each slice */
+	err = device_for_each_child(&pdev->dev, l2cache_pmu,
+				    l2_cache_pmu_probe_slice);
+	if (err < 0)
+		return err;
+
+	if (l2cache_pmu->num_pmus == 0) {
+		dev_err(&pdev->dev, "No hardware L2 cache PMUs found\n");
+		return -ENODEV;
+	}
+
+	err = perf_pmu_register(&l2cache_pmu->pmu, l2cache_pmu->pmu.name, -1);
+	if (err < 0) {
+		dev_err(&pdev->dev, "Error %d registering L2 cache PMU\n", err);
+		return err;
+	}
+
+	dev_info(&pdev->dev, "Registered L2 cache PMU using %d HW PMUs\n",
+		 l2cache_pmu->num_pmus);
+	mutex_lock(&l2cache_pmu_mutex);
+	list_add(&l2cache_pmu->entry, &l2cache_pmu_list);
+	mutex_unlock(&l2cache_pmu_mutex);
+
+	return err;
+}
+
+static int l2_cache_pmu_remove(struct platform_device *pdev)
+{
+	struct l2cache_pmu *l2cache_pmu =
+		to_l2cache_pmu(platform_get_drvdata(pdev));
+
+	mutex_lock(&l2cache_pmu_mutex);
+	list_del(&l2cache_pmu->entry);
+	mutex_unlock(&l2cache_pmu_mutex);
+	perf_pmu_unregister(&l2cache_pmu->pmu);
+	return 0;
+}
+
+static struct platform_driver l2_cache_pmu_driver = {
+	.driver = {
+		.name = "qcom-l2cache-pmu",
+		.owner = THIS_MODULE,
+		.acpi_match_table = ACPI_PTR(l2_cache_pmu_acpi_match),
+	},
+	.probe = l2_cache_pmu_probe,
+	.remove = l2_cache_pmu_remove,
+};
+
+static int __init register_l2_cache_pmu_driver(void)
+{
+	int err;
+
+	err = cpuhp_setup_state_nocalls(CPUHP_AP_PERF_ARM_QCOM_L2_ONLINE,
+					"AP_PERF_ARM_QCOM_L2_ONLINE",
+					l2cache_pmu_online_cpu,
+					l2cache_pmu_offline_cpu);
+	if (err)
+		return err;
+	return platform_driver_register(&l2_cache_pmu_driver);
+}
+device_initcall(register_l2_cache_pmu_driver);
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 242bf53..69597b9 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -86,6 +86,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_QCOM_L2_ONLINE,
 	CPUHP_AP_WORKQUEUE_ONLINE,
 	CPUHP_AP_RCUTREE_ONLINE,
 	CPUHP_AP_NOTIFY_ONLINE,
diff --git a/include/linux/soc/qcom/perf_event_l2.h b/include/linux/soc/qcom/perf_event_l2.h
new file mode 100644
index 0000000..754970a
--- /dev/null
+++ b/include/linux/soc/qcom/perf_event_l2.h
@@ -0,0 +1,79 @@
+/*
+ * Copyright (c) 2015, 2016 The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only 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.
+ */
+
+#ifndef __QCOM_PERF_EVENT_L2_H
+#define __QCOM_PERF_EVENT_L2_H
+
+#define MAX_L2_CTRS             17
+
+#define L2PMCR_NUM_EV_SHIFT     11
+#define L2PMCR_NUM_EV_MASK      0x1F
+
+#define L2PMCR                  0x400
+#define L2PMCNTENCLR            0x403
+#define L2PMCNTENSET            0x404
+#define L2PMINTENCLR            0x405
+#define L2PMINTENSET            0x406
+#define L2PMOVSCLR              0x407
+#define L2PMOVSSET              0x408
+#define L2PMCCNTCR              0x409
+#define L2PMCCNTR               0x40A
+#define L2PMCCNTSR              0x40C
+#define L2PMRESR                0x410
+#define IA_L2PMXEVCNTCR_BASE    0x420
+#define IA_L2PMXEVCNTR_BASE     0x421
+#define IA_L2PMXEVFILTER_BASE   0x423
+#define IA_L2PMXEVTYPER_BASE    0x424
+
+#define IA_L2_REG_OFFSET        0x10
+
+#define L2PMXEVFILTER_SUFILTER_ALL      0x000E0000
+#define L2PMXEVFILTER_ORGFILTER_IDINDEP 0x00000004
+#define L2PMXEVFILTER_ORGFILTER_ALL     0x00000003
+
+#define L2PM_CC_ENABLE          0x80000000
+
+#define L2EVTYPER_REG_SHIFT     3
+
+#define L2PMRESR_GROUP_BITS     8
+#define L2PMRESR_GROUP_MASK     GENMASK(7, 0)
+
+#define L2CYCLE_CTR_BIT         31
+#define L2CYCLE_CTR_RAW_CODE    0xFE
+
+#define L2PMCR_RESET_ALL        0x6
+#define L2PMCR_GLOBAL_ENABLE    0x1
+#define L2PMCR_GLOBAL_DISABLE   0x0
+
+#define L2PMRESR_EN             ((u64)1 << 63)
+
+#define L2_EVT_MASK             0xFFFFF
+#define L2_EVT_PFX_MASK         0xF0000
+#define L2_EVT_REG_MASK         0x0F000
+#define L2_EVT_CODE_MASK        0x00FF0
+#define L2_EVT_GRP_MASK         0x0000F
+#define L2_EVT_PFX_SHIFT        16
+#define L2_EVT_REG_SHIFT        12
+#define L2_EVT_CODE_SHIFT        4
+#define L2_EVT_GRP_SHIFT         0
+#define L2_EVT_PREFIX(event) (((event) & L2_EVT_PFX_MASK) >> L2_EVT_PFX_SHIFT)
+#define L2_EVT_REG(event)    (((event) & L2_EVT_REG_MASK) >> L2_EVT_REG_SHIFT)
+#define L2_EVT_CODE(event)   (((event) & L2_EVT_CODE_MASK) >> L2_EVT_CODE_SHIFT)
+#define L2_EVT_GROUP(event)  (((event) & L2_EVT_GRP_MASK) >> L2_EVT_GRP_SHIFT)
+
+#define L2_EVT_GROUP_MAX         7
+
+#define L2_MAX_PERIOD           U32_MAX
+#define L2_CNT_PERIOD           (U32_MAX - GENMASK(26, 0))
+
+#endif
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v4 2/2] soc: qcom: add l2 cache perf events driver

[Mark Rutland]

Hi Neil,

On Tue, Aug 30, 2016 at 01:01:33PM -0400, Neil Leeder wrote:
> Adds perf events support for L2 cache PMU.
> 
> The L2 cache PMU driver is named 'l2cache_0' and can be used
> with perf events to profile L2 events such as cache hits
> and misses.
> 
> Signed-off-by: Neil Leeder <nleeder@codeaurora.org>
> ---
>  drivers/soc/qcom/Kconfig               |  10 +
>  drivers/soc/qcom/Makefile              |   1 +
>  drivers/soc/qcom/perf_event_l2.c       | 855 +++++++++++++++++++++++++++++++++
>  include/linux/cpuhotplug.h             |   1 +
>  include/linux/soc/qcom/perf_event_l2.h |  79 +++

Unless you have another user of perf_event_l2.h, please fold that into
perf_event_l2.c. Nothing is gained by having that separate.

If you do have another user of that, please describe that. Given the
values are HW constants I can't imagine what else would be using that.

>  5 files changed, 946 insertions(+)
>  create mode 100644 drivers/soc/qcom/perf_event_l2.c
>  create mode 100644 include/linux/soc/qcom/perf_event_l2.h
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index ddd6b71..7e71d8d 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -16,6 +16,16 @@ config QCOM_L2_ACCESSORS
>  	  Provides support for accessing registers in the L2 cache
>  	  for Qualcomm Technologies ARM64 chips.
>  
> +config QCOM_PERF_EVENTS_L2
> +	bool "Qualcomm Technologies L2-cache perf events"
> +	depends on ARCH_QCOM && ARM64 && HW_PERF_EVENTS && ACPI
> +	select QCOM_L2_ACCESSORS

As with prior postings, I think it makes sense to fold the accessors
from patch 1 in also. They can be factored out if and when a subsequent
patch series adds another user.

> +	  help
> +	  Provides support for the L2 cache performance monitor unit (PMU)
> +	  in Qualcomm Technologies processors.
> +	  Adds the L2 cache PMU into the perf events subsystem for
> +	  monitoring L2 cache events.
> +
>  config QCOM_PM
>  	bool "Qualcomm Power Management"
>  	depends on ARCH_QCOM && !ARM64
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 6ef29b9..c8e89ca9 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -1,5 +1,6 @@
>  obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
>  obj-$(CONFIG_QCOM_L2_ACCESSORS) += l2-accessors.o
> +obj-$(CONFIG_QCOM_PERF_EVENTS_L2)	+= perf_event_l2.o
>  obj-$(CONFIG_QCOM_PM)	+=	spm.o
>  obj-$(CONFIG_QCOM_SMD) +=	smd.o
>  obj-$(CONFIG_QCOM_SMD_RPM)	+= smd-rpm.o
> diff --git a/drivers/soc/qcom/perf_event_l2.c b/drivers/soc/qcom/perf_event_l2.c
> new file mode 100644
> index 0000000..5c13e87
> --- /dev/null
> +++ b/drivers/soc/qcom/perf_event_l2.c
> @@ -0,0 +1,855 @@
> +/* Copyright (c) 2015,2016 The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only 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.
> + */
> +#define pr_fmt(fmt) "l2 perfevents: " fmt

There are no printk uses in the file, so this can go.

Otherwise, it'd be worth using "qcom-l2", or better, drop the above and
use dev_printk and friends.

> +
> +#include <linux/acpi.h>
> +#include <linux/interrupt.h>
> +#include <linux/perf_event.h>
> +#include <linux/platform_device.h>
> +#include <linux/soc/qcom/l2-accessors.h>
> +#include <linux/soc/qcom/perf_event_l2.h>
> +
> +/*
> + * Aggregate PMU. Implements the core pmu functions and manages
> + * the hardware PMUs.
> + */
> +struct l2cache_pmu {
> +	struct list_head entry;
> +	u32 num_pmus;
> +	struct pmu pmu;
> +	int num_counters;
> +	cpumask_t cpumask;
> +	struct platform_device *pdev;
> +};
> +
> +/*
> + * The cache is made-up of one or more slices, each slice has its own PMU.
> + * This structure represents one of the hardware PMUs.
> + */

I realise that this is ground we've covered before, but I'm rather fuzzy
on the slice details, which makes this very difficult to review, and
will make it very difficult to come back to in future.

It would be helpful if you could expand the block comment above to
elaborate a little further on the HW design, and the concepts you are
trying to capture above.

Specifically:
- How do slices relate to CPUs? 1-1, M-N, or somewhere between?
- What are groups? What are columns? How do they relate to counters?

> +struct hml2_pmu {
> +	struct perf_event *events[MAX_L2_CTRS];
> +	struct l2cache_pmu *l2cache_pmu;
> +	unsigned long used_mask[BITS_TO_LONGS(MAX_L2_CTRS)];
> +	unsigned long group_used_mask[BITS_TO_LONGS(L2_EVT_GROUP_MAX + 1)];

Please use DECLARE_BITMAP. Also, the "used_mask" name is ambiguous, so I
would recommend "used_counters", and likewise "used_groups" instead of
group_used_mask. e.g.

	DECLARE_BITMAP(used_counters, MAX_L2_CTRS);
	DECLARE_BITMAP(used_groups, L2_EVT_GROUP_MAX + 1);

> +	int group_to_counter[L2_EVT_GROUP_MAX + 1];
> +	int irq;
> +	/* The CPU that is used for collecting events on this slice */
> +	int on_cpu;
> +	/* All the CPUs associated with this slice */
> +	cpumask_t slice_cpus;
> +	atomic64_t prev_count[MAX_L2_CTRS];

Use the hw_perf_event field for this. See struct perf_event::hw and
struct hw_perf_event.

> +	spinlock_t pmu_lock;
> +};
> +
> +#define to_l2cache_pmu(p) (container_of(p, struct l2cache_pmu, pmu))
> +
> +static DEFINE_MUTEX(l2cache_pmu_mutex);


A mutex (which can sleep) is not safe for the hotplug state machine
stuff. See recent patches to the arm_pmu code.

That's further subsumed by the multi-instance stuff, so this (and the
list) will not be necessary shortly.

> +static LIST_HEAD(l2cache_pmu_list);
> +static DEFINE_PER_CPU(struct hml2_pmu *, cpu_to_pmu);

Typically, per-cpu variables are named after what the variable holds,
ignoring the fact that it's per-cpu. The naming here makes it sound like
each per-cpu instances holds a CPU -> PMU mapping, which is not the
case.

I would recommend:

static DEFINE_PER_CPU(struct hml2_pmu *, pmu_slice);

> +static u32 l2_cycle_ctr_idx;
> +static u32 l2_reset_mask;
> +
> +static inline u32 idx_to_reg_bit(u32 idx)
> +{
> +	u32 bit;
> +
> +	if (idx == l2_cycle_ctr_idx)
> +		bit = BIT(L2CYCLE_CTR_BIT);
> +	else
> +		bit = BIT(idx);
> +	return bit;
> +}

This can be:

static inline u32 idx_to_reg_bit(u32 idx)
{
	if (idx == l2_cycle_ctr_idx);
		return BIT(L2CYCLE_CTR_BIT);
	
	return BIT(idx);
}

> +
> +static inline struct hml2_pmu *get_hml2_pmu(int cpu)
> +{
> +	return per_cpu(cpu_to_pmu, cpu);
> +}
> +
> +static void hml2_pmu__reset_on_slice(void *x)
> +{
> +	/* Reset all ctrs */
> +	set_l2_indirect_reg(L2PMCR, L2PMCR_RESET_ALL);
> +	set_l2_indirect_reg(L2PMCNTENCLR, l2_reset_mask);
> +	set_l2_indirect_reg(L2PMINTENCLR, l2_reset_mask);
> +	set_l2_indirect_reg(L2PMOVSCLR, l2_reset_mask);
> +}
> +
> +static inline void hml2_pmu__reset(struct hml2_pmu *slice)
> +{
> +	int cpu;
> +
> +	if (cpumask_test_cpu(smp_processor_id(), &slice->slice_cpus)) {
> +		hml2_pmu__reset_on_slice(NULL);
> +		return;
> +	}
> +
> +	/* Call each cpu in the cluster until one works */
> +	for_each_cpu(cpu, &slice->slice_cpus) {
> +		if (!smp_call_function_single(cpu, hml2_pmu__reset_on_slice,
> +					      NULL, 1))
> +			return;
> +	}

Homebrew smp_call_function_any()? I think the above can be replaces
with:

	cpumask_t *mask = &slice->slice_cpus;

	if (!smp_call_function_any(mask, hml2_pmu__reset_on_slice, NULL))
		return;

> +
> +	dev_err(&slice->l2cache_pmu->pdev->dev,
> +		"Failed to reset on cluster with cpu %d\n",
> +		cpumask_first(&slice->slice_cpus));
> +}
> +
> +static inline void hml2_pmu__enable(void)
> +{
> +	set_l2_indirect_reg(L2PMCR, L2PMCR_GLOBAL_ENABLE);
> +}
> +
> +static inline void hml2_pmu__disable(void)
> +{
> +	set_l2_indirect_reg(L2PMCR, L2PMCR_GLOBAL_DISABLE);
> +}

Are these per-slice, or shared across slices?

I had assumed everything prefixed with hml2_pmu was per-slice, but the
GLOBAL naming, and the use below in l2_cache__pmu_{enable,disable}
implies that's not the case here.

> +
> +static inline void hml2_pmu__counter_set_value(u32 idx, u64 value)
> +{
> +	u32 counter_reg;
> +
> +	if (idx == l2_cycle_ctr_idx) {
> +		set_l2_indirect_reg(L2PMCCNTR, value);
> +	} else {
> +		counter_reg = (idx * IA_L2_REG_OFFSET) + IA_L2PMXEVCNTR_BASE;
> +		set_l2_indirect_reg(counter_reg, (u32)(value & GENMASK(31, 0)));

You shouldn't need the u32 cast here.

> +	}
> +}
> +
> +static inline u64 hml2_pmu__counter_get_value(u32 idx)
> +{
> +	u64 value;
> +	u32 counter_reg;
> +
> +	if (idx == l2_cycle_ctr_idx) {
> +		value = get_l2_indirect_reg(L2PMCCNTR);
> +	} else {
> +		counter_reg = (idx * IA_L2_REG_OFFSET) + IA_L2PMXEVCNTR_BASE;
> +		value = get_l2_indirect_reg(counter_reg);
> +	}
> +
> +	return value;
> +}
> +
> +static inline void hml2_pmu__counter_enable(u32 idx)
> +{
> +	u32 reg;
> +
> +	reg = get_l2_indirect_reg(L2PMCNTENSET);
> +	reg |= idx_to_reg_bit(idx);
> +	set_l2_indirect_reg(L2PMCNTENSET, reg);
> +

Just to check, given you have separate SET and CLR registers, do you
actually need a RMW sequence here? Does wirintg a zero to a field in the
SET register actually clear it?

> +}
> +static inline void hml2_pmu__counter_disable(u32 idx)
> +{
> +	set_l2_indirect_reg(L2PMCNTENCLR, idx_to_reg_bit(idx));
> +}
> +
> +static inline void hml2_pmu__counter_enable_interrupt(u32 idx)
> +{
> +	u32 reg;
> +
> +	reg = get_l2_indirect_reg(L2PMINTENSET);
> +	reg |= idx_to_reg_bit(idx);
> +	set_l2_indirect_reg(L2PMINTENSET, reg);
> +}

Likewise?

> +static inline void hml2_pmu__counter_disable_interrupt(u32 idx)
> +{
> +	set_l2_indirect_reg(L2PMINTENCLR, idx_to_reg_bit(idx));
> +}
> +
> +static inline void hml2_pmu__set_evcntcr(u32 ctr, u32 val)
> +{
> +	u32 evtcr_reg = (ctr * IA_L2_REG_OFFSET) + IA_L2PMXEVCNTCR_BASE;
> +
> +	set_l2_indirect_reg(evtcr_reg, val);
> +}
> +
> +static inline void hml2_pmu__set_evtyper(u32 ctr, u32 val)
> +{
> +	u32 evtype_reg = (ctr * IA_L2_REG_OFFSET) + IA_L2PMXEVTYPER_BASE;
> +
> +	set_l2_indirect_reg(evtype_reg, val);
> +}
> +
> +static void hml2_pmu__set_resr(struct hml2_pmu *slice,
> +			       u32 event_group, u32 event_cc)
> +{
> +	u64 field;
> +	u64 resr_val;
> +	u32 shift;
> +	unsigned long iflags;

Nit: s/iflags/flags/ for consistency

[mark@leverpostej:~/src/linux]% git grep irqsave | grep ',\s*flags)' | wc -l
15074
[mark@leverpostej:~/src/linux]% git grep irqsave | grep ',\s*iflags)' | wc -l
106

> +
> +	shift = L2PMRESR_GROUP_BITS * event_group;
> +	field = ((u64)(event_cc & L2PMRESR_GROUP_MASK) << shift) | L2PMRESR_EN;
> +
> +	spin_lock_irqsave(&slice->pmu_lock, iflags);
> +
> +	resr_val = get_l2_indirect_reg(L2PMRESR);
> +	resr_val &= ~(L2PMRESR_GROUP_MASK << shift);
> +	resr_val |= field;
> +	set_l2_indirect_reg(L2PMRESR, resr_val);
> +
> +	spin_unlock_irqrestore(&slice->pmu_lock, iflags);
> +}
> +
> +static inline void hml2_pmu__set_evfilter_sys_mode(u32 ctr)
> +{
> +	set_l2_indirect_reg((ctr * IA_L2_REG_OFFSET) + IA_L2PMXEVFILTER_BASE,
> +			    L2PMXEVFILTER_SUFILTER_ALL |
> +			    L2PMXEVFILTER_ORGFILTER_IDINDEP |
> +			    L2PMXEVFILTER_ORGFILTER_ALL);
> +}

This is painful to read. Please use temporary variables for the two
parameters.

What are these filters?

> +
> +static inline u32 hml2_pmu__getreset_ovsr(void)
> +{
> +	u32 result = get_l2_indirect_reg(L2PMOVSSET);
> +
> +	set_l2_indirect_reg(L2PMOVSCLR, result);
> +	return result;
> +}
> +
> +static inline bool hml2_pmu__has_overflowed(u32 ovsr)
> +{
> +	return !!(ovsr & l2_reset_mask);
> +}

It took me a moment to figure out what was happening here.

l2_reset_mask is the mask of counters actually present, right?

Please rename it to something that doesn't imply it's specific to reset.

If a counter isn't implemented, is its bit zero, one, or unknown?

> +static inline bool hml2_pmu__counter_has_overflowed(u32 ovsr, u32 idx)
> +{
> +	return !!(ovsr & idx_to_reg_bit(idx));
> +}
> +
> +static void l2_cache__event_update_from_slice(struct perf_event *event,
> +					      struct hml2_pmu *slice)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +	u64 delta64, prev, now;
> +	u32 delta;
> +	u32 idx = hwc->idx;
> +
> +	do {
> +		prev = atomic64_read(&slice->prev_count[idx]);
> +		now = hml2_pmu__counter_get_value(idx);
> +	} while (atomic64_cmpxchg(&slice->prev_count[idx], prev, now) != prev);
> +

As covered above, use hwc->prev_count.

> +	if (idx == l2_cycle_ctr_idx) {
> +		/*
> +		 * The cycle counter is 64-bit so needs separate handling
> +		 * of 64-bit delta.
> +		 */
> +		delta64 = now - prev;
> +		local64_add(delta64, &event->count);
> +	} else {
> +		/*
> +		 * 32-bit counters need the unsigned 32-bit math to handle
> +		 * overflow and now < prev
> +		 */
> +		delta = now - prev;
> +		local64_add(delta, &event->count);
> +	}
> +}
> +
> +static void l2_cache__slice_set_period(struct hml2_pmu *slice,
> +				       struct hw_perf_event *hwc)
> +{
> +	u64 value = L2_MAX_PERIOD - (L2_CNT_PERIOD - 1);
> +	u32 idx = hwc->idx;
> +	u64 prev = atomic64_read(&slice->prev_count[idx]);

Use hwc->prev_count.

> +
> +	if (prev < value) {
> +		value += prev;
> +		atomic64_set(&slice->prev_count[idx], value);
> +	} else {
> +		value = prev;
> +	}

I don't follow this. Surely you need to update the prev_count in either
case? Otherwise subsequent updates are going to give you erroneous
numbers.

> +
> +	hml2_pmu__counter_set_value(idx, value);
> +}
> +
> +static int l2_cache__get_event_idx(struct hml2_pmu *slice,
> +				   struct perf_event *event)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +	int idx;
> +
> +	if (hwc->config_base == L2CYCLE_CTR_RAW_CODE) {
> +		if (test_and_set_bit(l2_cycle_ctr_idx, slice->used_mask))
> +			return -EAGAIN;
> +
> +		return l2_cycle_ctr_idx;
> +	}
> +
> +	for (idx = 0; idx < slice->l2cache_pmu->num_counters - 1; idx++) {
> +		if (!test_and_set_bit(idx, slice->used_mask)) {
> +			set_bit(L2_EVT_GROUP(hwc->config_base),
> +				slice->group_used_mask);

Can't the group already be taken by another event?

> +			return idx;
> +		}
> +	}
> +
> +	/* The counters are all in use. */
> +	return -EAGAIN;
> +}
> +
> +static void l2_cache__clear_event_idx(struct hml2_pmu *slice,
> +				      struct perf_event *event)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +
> +	clear_bit(L2_EVT_GROUP(hwc->config_base), slice->group_used_mask);
> +}

What about used_mask? Shouldn't this mirror l2_cache__get_event_idx?

> +
> +static irqreturn_t l2_cache__handle_irq(int irq_num, void *data)
> +{
> +	struct hml2_pmu *slice = data;
> +	u32 ovsr;
> +	int idx;
> +
> +	ovsr = hml2_pmu__getreset_ovsr();
> +	if (!hml2_pmu__has_overflowed(ovsr))
> +		return IRQ_NONE;
> +
> +	for (idx = 0; idx < slice->l2cache_pmu->num_counters; idx++) {

Can't you use for_each_set_bit over the used_mask?

> +		struct perf_event *event = slice->events[idx];
> +		struct hw_perf_event *hwc;
> +
> +		if (!event)
> +			continue;

Then so long as we're careful with the mask manipulation we can remove
this check.

> +
> +		if (!hml2_pmu__counter_has_overflowed(ovsr, idx))
> +			continue;
> +
> +		l2_cache__event_update_from_slice(event, slice);
> +		hwc = &event->hw;
> +
> +		l2_cache__slice_set_period(slice, hwc);
> +	}
> +
> +	/*
> +	 * Handle the pending perf events.
> +	 *
> +	 * Note: this call *must* be run with interrupts disabled. For
> +	 * platforms that can have the PMU interrupts raised as an NMI, this
> +	 * will not work.
> +	 */

Please remove the last sentence, as we do not have an NMI.

> +	irq_work_run();

Do we ever have any work to run? I thought that only happened in the
case of an overflow event when sampling, which isn't possible here...

> +
> +	return IRQ_HANDLED;
> +}
> +
> +/*
> + * Implementation of abstract pmu functionality required by
> + * the core perf events code.
> + */
> +
> +static void l2_cache__pmu_enable(struct pmu *pmu)
> +{
> +	hml2_pmu__enable();
> +}
> +
> +static void l2_cache__pmu_disable(struct pmu *pmu)
> +{
> +	hml2_pmu__disable();
> +}
> +
> +static int l2_cache__event_init(struct perf_event *event)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +	struct hml2_pmu *slice;
> +	struct perf_event *sibling;
> +	struct l2cache_pmu *l2cache_pmu = to_l2cache_pmu(event->pmu);

Defer this until after the type check. You don't know it's an
l2cache_pmu yet.

> +
> +	if (event->attr.type != l2cache_pmu->pmu.type)
> +		return -ENOENT;
> +
> +	if (hwc->sample_period) {
> +		dev_warn(&l2cache_pmu->pdev->dev, "Sampling not supported\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (event->cpu < 0) {
> +		dev_warn(&l2cache_pmu->pdev->dev, "Per-task mode not supported\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	/* We cannot filter accurately so we just don't allow it. */
> +	if (event->attr.exclude_user || event->attr.exclude_kernel ||
> +	    event->attr.exclude_hv || event->attr.exclude_idle) {
> +		dev_warn(&l2cache_pmu->pdev->dev, "Can't exclude execution levels\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (((L2_EVT_GROUP(event->attr.config) > L2_EVT_GROUP_MAX) ||
> +	    (L2_EVT_PREFIX(event->attr.config) != 0) ||
> +	    (L2_EVT_REG(event->attr.config) != 0)) &&
> +	    (event->attr.config != L2CYCLE_CTR_RAW_CODE)) {
> +		dev_warn(&l2cache_pmu->pdev->dev, "Invalid config %llx\n",
> +			 event->attr.config);
> +		return -EINVAL;
> +	}
> +
> +	/* Don't allow groups with mixed PMUs, except for s/w events */
> +	if (event->group_leader->pmu != event->pmu &&
> +	    !is_software_event(event->group_leader)) {
> +		dev_warn(&l2cache_pmu->pdev->dev,
> +			 "Can't create mixed PMU group\n");
> +		return -EINVAL;
> +	}
> +
> +	list_for_each_entry(sibling, &event->group_leader->sibling_list,
> +			    group_entry)
> +		if (sibling->pmu != event->pmu &&
> +		    !is_software_event(sibling)) {
> +			dev_warn(&l2cache_pmu->pdev->dev,
> +				 "Can't create mixed PMU group\n");
> +			return -EINVAL;
> +		}
> +
> +	hwc->idx = -1;
> +	hwc->config_base = event->attr.config;
> +
> +	/*
> +	 * Ensure all events are on the same cpu so all events are in the
> +	 * same cpu context, to avoid races on pmu_enable etc.
> +	 */
> +	slice = get_hml2_pmu(event->cpu);
> +	event->cpu = slice->on_cpu;

This could put an event on a different CPU to its group siblings, which
is broken.

> +
> +	return 0;
> +}
> +
> +static void l2_cache__event_update(struct perf_event *event)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +
> +	if (hwc->idx < 0)
> +		return;

When does this happen?

> +
> +	l2_cache__event_update_from_slice(event, get_hml2_pmu(event->cpu));
> +}
> +
> +static void l2_cache__event_start(struct perf_event *event, int flags)
> +{
> +	struct hml2_pmu *slice;
> +	struct hw_perf_event *hwc = &event->hw;
> +	int idx = hwc->idx;
> +	u32 config;
> +	u32 evt_prefix, event_cc, event_group;
> +
> +	if (idx < 0)
> +		return;

When does this happen?

> +
> +	hwc->state = 0;
> +
> +	slice = get_hml2_pmu(event->cpu);
> +	l2_cache__slice_set_period(slice, hwc);
> +
> +	if (hwc->config_base == L2CYCLE_CTR_RAW_CODE)
> +		goto out;

Why?

You can't reset or program the cycle counter value?

> +
> +	config = hwc->config_base;
> +	evt_prefix  = L2_EVT_PREFIX(config);
> +	event_cc    = L2_EVT_CODE(config);
> +	event_group = L2_EVT_GROUP(config);
> +
> +	hml2_pmu__set_evcntcr(idx, 0x0);
> +	hml2_pmu__set_evtyper(idx, event_group);
> +	hml2_pmu__set_resr(slice, event_group, event_cc);
> +	hml2_pmu__set_evfilter_sys_mode(idx);
> +out:
> +	hml2_pmu__counter_enable_interrupt(idx);
> +	hml2_pmu__counter_enable(idx);
> +}
> +
> +static void l2_cache__event_stop(struct perf_event *event, int flags)
> +{
> +	struct hml2_pmu *slice;
> +	struct hw_perf_event *hwc = &event->hw;
> +	int idx = hwc->idx;
> +
> +	if (idx < 0)
> +		return;

When does this happen?

> +
> +	if (!(hwc->state & PERF_HES_STOPPED)) {
> +		slice = get_hml2_pmu(event->cpu);
> +		hml2_pmu__counter_disable_interrupt(idx);
> +		hml2_pmu__counter_disable(idx);
> +
> +		if (flags & PERF_EF_UPDATE)
> +			l2_cache__event_update(event);
> +		hwc->state |= PERF_HES_STOPPED | PERF_HES_UPTODATE;
> +	}
> +}

[...]

> +static void l2_cache__event_del(struct perf_event *event, int flags)
> +{
> +	struct hw_perf_event *hwc = &event->hw;
> +	struct hml2_pmu *slice;
> +	int idx = hwc->idx;
> +
> +	if (idx < 0)
> +		return;

When does this happen?

> +
> +	slice = get_hml2_pmu(event->cpu);
> +	l2_cache__event_stop(event, flags | PERF_EF_UPDATE);
> +	slice->events[idx] = NULL;
> +	clear_bit(idx, slice->used_mask);
> +	l2_cache__clear_event_idx(slice, event);
> +
> +	perf_event_update_userpage(event);
> +}

[...]

> +static int l2cache_pmu_online_cpu(unsigned int cpu)
> +{
> +	struct hml2_pmu *slice;
> +	struct l2cache_pmu *l2cache_pmu;
> +	cpumask_t slice_online_cpus;
> +
> +	mutex_lock(&l2cache_pmu_mutex);

As I mentioned above, using a mutex here is not safe, as it can sleep,
and will be called in a context where that is not permitted.

> +	list_for_each_entry(l2cache_pmu, &l2cache_pmu_list, entry) {
> +		slice = get_hml2_pmu(cpu);
> +		cpumask_and(&slice_online_cpus, &slice->slice_cpus,
> +			    cpu_online_mask);
> +		if (cpumask_weight(&slice_online_cpus) == 1) {
> +			/* all CPUs on this slice were down, use this one */
> +			slice->on_cpu = cpu;
> +			cpumask_set_cpu(cpu, &l2cache_pmu->cpumask);
> +			WARN_ON(irq_set_affinity(slice->irq, cpumask_of(cpu)));
> +		}

Please split this up with some newlines.

> +	}
> +	mutex_unlock(&l2cache_pmu_mutex);
> +
> +	return 0;
> +}
> +
> +static int l2cache_pmu_offline_cpu(unsigned int cpu)
> +{
> +	struct hml2_pmu *slice;
> +	struct l2cache_pmu *l2cache_pmu;
> +	cpumask_t slice_online_cpus;
> +	unsigned int target;
> +
> +	mutex_lock(&l2cache_pmu_mutex);
> +	list_for_each_entry(l2cache_pmu, &l2cache_pmu_list, entry) {
> +		if (!cpumask_test_and_clear_cpu(cpu, &l2cache_pmu->cpumask))
> +			break;

Shouldn't that be a continue, so we check the other PMUs in the list?

> +		slice = get_hml2_pmu(cpu);
> +		cpumask_and(&slice_online_cpus, &slice->slice_cpus,
> +			    cpu_online_mask);
> +		/* Any other CPU for this slice which is still online */
> +		target = cpumask_any_but(&slice_online_cpus, cpu);
> +		if (target >= nr_cpu_ids)
> +			break;
> +		perf_pmu_migrate_context(&l2cache_pmu->pmu, cpu, target);
> +		slice->on_cpu = target;
> +		cpumask_set_cpu(target, &l2cache_pmu->cpumask);
> +		WARN_ON(irq_set_affinity(slice->irq, cpumask_of(target)));

Please try to split this up with newlines to make it more legible.

> +	}
> +	mutex_unlock(&l2cache_pmu_mutex);
> +
> +	return 0;
> +}
> +
> +static int l2_cache_pmu_probe_slice(struct device *dev, void *data)
> +{
> +	struct platform_device *pdev = to_platform_device(dev->parent);
> +	struct platform_device *sdev = to_platform_device(dev);
> +	struct l2cache_pmu *l2cache_pmu = data;
> +	struct hml2_pmu *slice;
> +	struct acpi_device *device;
> +	int irq;
> +	int err;
> +	int logical_cpu;
> +	unsigned long fw_slice_id;
> +
> +	if (acpi_bus_get_device(ACPI_HANDLE(dev), &device))
> +		return -ENODEV;
> +
> +	if (kstrtol(device->pnp.unique_id, 10, &fw_slice_id) < 0) {
> +		dev_err(&pdev->dev, "unable to read ACPI uid\n");
> +		return -ENODEV;
> +	}
> +
> +	irq = platform_get_irq(sdev, 0);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev,
> +			"Failed to get valid irq for slice %ld\n", fw_slice_id);
> +		return irq;
> +	}
> +
> +	slice = devm_kzalloc(&pdev->dev, sizeof(*slice), GFP_KERNEL);
> +	if (!slice)
> +		return -ENOMEM;
> +
> +	slice->l2cache_pmu = l2cache_pmu;
> +	for_each_present_cpu(logical_cpu) {
> +		if (topology_physical_package_id(logical_cpu) == fw_slice_id) {
> +			cpumask_set_cpu(logical_cpu, &slice->slice_cpus);
> +			per_cpu(cpu_to_pmu, logical_cpu) = slice;
> +		}
> +	}
> +	slice->irq = irq;
> +
> +	if (cpumask_empty(&slice->slice_cpus)) {
> +		dev_err(&pdev->dev, "No CPUs found for L2 cache instance %ld\n",
> +			fw_slice_id);
> +		return -ENODEV;
> +	}
> +
> +	/* Pick one CPU to be the preferred one to use in the slice */
> +	slice->on_cpu = cpumask_first(&slice->slice_cpus);
> +
> +	if (irq_set_affinity(irq, cpumask_of(slice->on_cpu))) {
> +		dev_err(&pdev->dev,
> +			"Unable to set irq affinity (irq=%d, cpu=%d)\n",
> +			irq, slice->on_cpu);
> +		return -ENODEV;
> +	}
> +
> +	err = devm_request_irq(
> +		&pdev->dev, irq, l2_cache__handle_irq,
> +		IRQF_NOBALANCING, "l2-cache-pmu", slice);

Unusual formatting.

> +	if (err) {
> +		dev_err(&pdev->dev,
> +			"Unable to request IRQ%d for L2 PMU counters\n",
> +			irq);
> +		return err;
> +	}
> +
> +	dev_info(&pdev->dev,
> +		 "Registered L2 cache PMU instance %ld with %d CPUs\n",
> +		 fw_slice_id, cpumask_weight(&slice->slice_cpus));
> +
> +	slice->pmu_lock = __SPIN_LOCK_UNLOCKED(slice->pmu_lock);
> +	cpumask_set_cpu(slice->on_cpu, &l2cache_pmu->cpumask);
> +
> +	hml2_pmu__reset(slice);
> +	l2cache_pmu->num_pmus++;
> +
> +	return 0;
> +}

Thanks,
Mark.

Re: [PATCH v4 2/2] soc: qcom: add l2 cache perf events driver

[Mark Rutland]

On Thu, Sep 01, 2016 at 05:30:52PM +0100, Mark Rutland wrote:
> On Tue, Aug 30, 2016 at 01:01:33PM -0400, Neil Leeder wrote:
> > +static DEFINE_MUTEX(l2cache_pmu_mutex);
> 
> A mutex (which can sleep) is not safe for the hotplug state machine
> stuff. See recent patches to the arm_pmu code.
> 
> That's further subsumed by the multi-instance stuff, so this (and the
> list) will not be necessary shortly.

This has *just* landed in tip. See the smp/hotplug branch [1]. The
arm_pmu change [2] should serve as an example of what to do.

Thanks,
Mark.

[1] https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/log/?h=smp/hotplug
[2] https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=smp/hotplug&id=cc727977acb0fe05b7aa1f00cccb893530820895

Re: [PATCH v4 2/2] soc: qcom: add l2 cache perf events driver

[Neil Leeder]

Hi Mark,
Thank you for the thorough review. I will post an updated patchset which addresses
all of your comments. There is just one outstanding comment which I have a question about:

On 9/1/2016 12:30 PM, Mark Rutland wrote:
> On Tue, Aug 30, 2016 at 01:01:33PM -0400, Neil Leeder wrote:

>> +static int l2_cache__event_init(struct perf_event *event)
>> +{
>> +	struct hw_perf_event *hwc = &event->hw;
>> +	struct hml2_pmu *slice;
>> +	struct perf_event *sibling;
>> +	struct l2cache_pmu *l2cache_pmu = to_l2cache_pmu(event->pmu);
>> +
>> +	if (event->attr.type != l2cache_pmu->pmu.type)
>> +		return -ENOENT;
>> +
>> +	if (hwc->sample_period) {
>> +		dev_warn(&l2cache_pmu->pdev->dev, "Sampling not supported\n");
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	if (event->cpu < 0) {
>> +		dev_warn(&l2cache_pmu->pdev->dev, "Per-task mode not supported\n");
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	/* We cannot filter accurately so we just don't allow it. */
>> +	if (event->attr.exclude_user || event->attr.exclude_kernel ||
>> +	    event->attr.exclude_hv || event->attr.exclude_idle) {
>> +		dev_warn(&l2cache_pmu->pdev->dev, "Can't exclude execution levels\n");
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	if (((L2_EVT_GROUP(event->attr.config) > L2_EVT_GROUP_MAX) ||
>> +	    (L2_EVT_PREFIX(event->attr.config) != 0) ||
>> +	    (L2_EVT_REG(event->attr.config) != 0)) &&
>> +	    (event->attr.config != L2CYCLE_CTR_RAW_CODE)) {
>> +		dev_warn(&l2cache_pmu->pdev->dev, "Invalid config %llx\n",
>> +			 event->attr.config);
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Don't allow groups with mixed PMUs, except for s/w events */
>> +	if (event->group_leader->pmu != event->pmu &&
>> +	    !is_software_event(event->group_leader)) {
>> +		dev_warn(&l2cache_pmu->pdev->dev,
>> +			 "Can't create mixed PMU group\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	list_for_each_entry(sibling, &event->group_leader->sibling_list,
>> +			    group_entry)
>> +		if (sibling->pmu != event->pmu &&
>> +		    !is_software_event(sibling)) {
>> +			dev_warn(&l2cache_pmu->pdev->dev,
>> +				 "Can't create mixed PMU group\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +	hwc->idx = -1;
>> +	hwc->config_base = event->attr.config;
>> +
>> +	/*
>> +	 * Ensure all events are on the same cpu so all events are in the
>> +	 * same cpu context, to avoid races on pmu_enable etc.
>> +	 */
>> +	slice = get_hml2_pmu(event->cpu);
>> +	event->cpu = slice->on_cpu;
> 
> This could put an event on a different CPU to its group siblings, which
> is broken.

This is the same logic as in arm-ccn.c:arm_ccn_pmu_event_init(), where there
is a single CPU designated as the CPU to be used for all events. All
events for this slice are forced to slice->on_cpu which is the CPU set in the
cpumask for this slice.

I'm not sure how this can put an event on a different CPU to its group siblings?

Thanks,
Neil
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v4 2/2] soc: qcom: add l2 cache perf events driver

[Mark Rutland]

On Fri, Sep 16, 2016 at 11:33:39AM -0400, Neil Leeder wrote:
> Hi Mark,
> Thank you for the thorough review. I will post an updated patchset
> which addresses all of your comments. There is just one outstanding
> comment which I have a question about:

[...]

> On 9/1/2016 12:30 PM, Mark Rutland wrote:
> > On Tue, Aug 30, 2016 at 01:01:33PM -0400, Neil Leeder wrote:
> >> +	/* Don't allow groups with mixed PMUs, except for s/w events */
> >> +	if (event->group_leader->pmu != event->pmu &&
> >> +	    !is_software_event(event->group_leader)) {
> >> +		dev_warn(&l2cache_pmu->pdev->dev,
> >> +			 "Can't create mixed PMU group\n");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	list_for_each_entry(sibling, &event->group_leader->sibling_list,
> >> +			    group_entry)
> >> +		if (sibling->pmu != event->pmu &&
> >> +		    !is_software_event(sibling)) {
> >> +			dev_warn(&l2cache_pmu->pdev->dev,
> >> +				 "Can't create mixed PMU group\n");
> >> +			return -EINVAL;
> >> +		}
> >> +
> >> +	hwc->idx = -1;
> >> +	hwc->config_base = event->attr.config;
> >> +
> >> +	/*
> >> +	 * Ensure all events are on the same cpu so all events are in the
> >> +	 * same cpu context, to avoid races on pmu_enable etc.
> >> +	 */
> >> +	slice = get_hml2_pmu(event->cpu);
> >> +	event->cpu = slice->on_cpu;
> > 
> > This could put an event on a different CPU to its group siblings, which
> > is broken.
> 
> This is the same logic as in arm-ccn.c:arm_ccn_pmu_event_init(), where there
> is a single CPU designated as the CPU to be used for all events.
>
> All events for this slice are forced to slice->on_cpu which is the CPU
> set in the cpumask for this slice.

The CCN is a little different. For the CCN, a single CPU is designated
to handle *all* events.

For this driver, a CPU is designated per-slice, judging by the existence
of hml2_pmu::on_cpu (unless that's superfluous). We've only verified
that the events are all for this PMU, not the same slice, and thus each
event->cpu may differ.

> I'm not sure how this can put an event on a different CPU to its group
> siblings?

In practice today, we'll try to schedule the event on it's group
leader's CPU, but accounting and subsequent manipulation could go wrong.

Thanks,
Mark.

Re: [PATCH v4 2/2] soc: qcom: add l2 cache perf events driver

[Neil Leeder]

On 9/16/2016 12:40 PM, Mark Rutland wrote:
> On Fri, Sep 16, 2016 at 11:33:39AM -0400, Neil Leeder wrote:
[...]
>> On 9/1/2016 12:30 PM, Mark Rutland wrote:
>>> On Tue, Aug 30, 2016 at 01:01:33PM -0400, Neil Leeder wrote:
>>>> +	/* Don't allow groups with mixed PMUs, except for s/w events */
>>>> +	if (event->group_leader->pmu != event->pmu &&
>>>> +	    !is_software_event(event->group_leader)) {
>>>> +		dev_warn(&l2cache_pmu->pdev->dev,
>>>> +			 "Can't create mixed PMU group\n");
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	list_for_each_entry(sibling, &event->group_leader->sibling_list,
>>>> +			    group_entry)
>>>> +		if (sibling->pmu != event->pmu &&
>>>> +		    !is_software_event(sibling)) {
>>>> +			dev_warn(&l2cache_pmu->pdev->dev,
>>>> +				 "Can't create mixed PMU group\n");
>>>> +			return -EINVAL;
>>>> +		}
>>>> +
>>>> +	hwc->idx = -1;
>>>> +	hwc->config_base = event->attr.config;
>>>> +
>>>> +	/*
>>>> +	 * Ensure all events are on the same cpu so all events are in the
>>>> +	 * same cpu context, to avoid races on pmu_enable etc.
>>>> +	 */
>>>> +	slice = get_hml2_pmu(event->cpu);
>>>> +	event->cpu = slice->on_cpu;
>>>
>>> This could put an event on a different CPU to its group siblings, which
>>> is broken.
>>
>> This is the same logic as in arm-ccn.c:arm_ccn_pmu_event_init(), where there
>> is a single CPU designated as the CPU to be used for all events.
>>
>> All events for this slice are forced to slice->on_cpu which is the CPU
>> set in the cpumask for this slice.
> 
> The CCN is a little different. For the CCN, a single CPU is designated
> to handle *all* events.
> 
> For this driver, a CPU is designated per-slice, judging by the existence
> of hml2_pmu::on_cpu (unless that's superfluous). We've only verified
> that the events are all for this PMU, not the same slice, and thus each
> event->cpu may differ.
> 

I see. So I can add a check that the group_leader event must be on the
same slice, and thus on the same CPU.

>> I'm not sure how this can put an event on a different CPU to its group
>> siblings?
> 
> In practice today, we'll try to schedule the event on it's group
> leader's CPU, but accounting and subsequent manipulation could go wrong.
> 
> Thanks,
> Mark.
> 

Neil
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.