linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/2] Add CCPI2 PMU support
@ 2019-07-23  9:16 Ganapatrao Kulkarni
  2019-07-23  9:16 ` [PATCH v3 1/2] Documentation: perf: Update documentation for ThunderX2 PMU uncore driver Ganapatrao Kulkarni
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Ganapatrao Kulkarni @ 2019-07-23  9:16 UTC (permalink / raw)
  To: linux-doc, linux-kernel, linux-arm-kernel
  Cc: mark.rutland, corbet, Jan Glauber,
	Jayachandran Chandrasekharan Nair, gklkml16, Robert Richter,
	will

Add Cavium Coherent Processor Interconnect (CCPI2) PMU
support in ThunderX2 Uncore driver.

v3: Rebased to 5.3-rc1

v2: Updated with review comments [1]

[1] https://lkml.org/lkml/2019/6/14/965

v1: initial patch

Ganapatrao Kulkarni (2):
  Documentation: perf: Update documentation for ThunderX2 PMU uncore
    driver
  drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver.

 .../admin-guide/perf/thunderx2-pmu.rst        |  20 +-
 drivers/perf/thunderx2_pmu.c                  | 248 +++++++++++++++---
 2 files changed, 225 insertions(+), 43 deletions(-)

-- 
2.17.1


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3 1/2] Documentation: perf: Update documentation for ThunderX2 PMU uncore driver
  2019-07-23  9:16 [PATCH v3 0/2] Add CCPI2 PMU support Ganapatrao Kulkarni
@ 2019-07-23  9:16 ` Ganapatrao Kulkarni
  2019-07-23  9:16 ` [PATCH v3 2/2] drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver Ganapatrao Kulkarni
  2019-07-29 10:54 ` [PATCH v3 0/2] Add CCPI2 PMU support Ganapatrao Kulkarni
  2 siblings, 0 replies; 10+ messages in thread
From: Ganapatrao Kulkarni @ 2019-07-23  9:16 UTC (permalink / raw)
  To: linux-doc, linux-kernel, linux-arm-kernel
  Cc: mark.rutland, corbet, Jan Glauber,
	Jayachandran Chandrasekharan Nair, gklkml16, Robert Richter,
	will

From: Ganapatrao Kulkarni <ganapatrao.kulkarni@marvell.com>

Add documentation for Cavium Coherent Processor Interconnect (CCPI2) PMU.

Signed-off-by: Ganapatrao Kulkarni <gkulkarni@marvell.com>
---
 .../admin-guide/perf/thunderx2-pmu.rst        | 20 ++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/Documentation/admin-guide/perf/thunderx2-pmu.rst b/Documentation/admin-guide/perf/thunderx2-pmu.rst
index 08e33675853a..01f158238ae1 100644
--- a/Documentation/admin-guide/perf/thunderx2-pmu.rst
+++ b/Documentation/admin-guide/perf/thunderx2-pmu.rst
@@ -3,24 +3,26 @@ Cavium ThunderX2 SoC Performance Monitoring Unit (PMU UNCORE)
 =============================================================
 
 The ThunderX2 SoC PMU consists of independent, system-wide, per-socket
-PMUs such as the Level 3 Cache (L3C) and DDR4 Memory Controller (DMC).
+PMUs such as the Level 3 Cache (L3C), DDR4 Memory Controller (DMC) and
+Cavium Coherent Processor Interconnect (CCPI2).
 
 The DMC has 8 interleaved channels and the L3C has 16 interleaved tiles.
 Events are counted for the default channel (i.e. channel 0) and prorated
 to the total number of channels/tiles.
 
-The DMC and L3C support up to 4 counters. Counters are independently
-programmable and can be started and stopped individually. Each counter
-can be set to a different event. Counters are 32-bit and do not support
-an overflow interrupt; they are read every 2 seconds.
+The DMC and L3C support up to 4 counters, while the CCPI2 supports up to 8
+counters. Counters are independently programmable to different events and
+can be started and stopped individually. None of the counters support an
+overflow interrupt. DMC and L3C counters are 32-bit and read every 2 seconds.
+The CCPI2 counters are 64-bit and assumed not to overflow in normal operation.
 
 PMU UNCORE (perf) driver:
 
 The thunderx2_pmu driver registers per-socket perf PMUs for the DMC and
-L3C devices.  Each PMU can be used to count up to 4 events
-simultaneously. The PMUs provide a description of their available events
-and configuration options under sysfs, see
-/sys/devices/uncore_<l3c_S/dmc_S/>; S is the socket id.
+L3C devices.  Each PMU can be used to count up to 4 (DMC/L3C) or up to 8
+(CCPI2) events simultaneously. The PMUs provide a description of their
+available events and configuration options under sysfs, see
+/sys/devices/uncore_<l3c_S/dmc_S/ccpi2_S/>; S is the socket id.
 
 The driver does not support sampling, therefore "perf record" will not
 work. Per-task perf sessions are also not supported.
-- 
2.17.1


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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v3 2/2] drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver.
  2019-07-23  9:16 [PATCH v3 0/2] Add CCPI2 PMU support Ganapatrao Kulkarni
  2019-07-23  9:16 ` [PATCH v3 1/2] Documentation: perf: Update documentation for ThunderX2 PMU uncore driver Ganapatrao Kulkarni
@ 2019-07-23  9:16 ` Ganapatrao Kulkarni
  2019-08-12 12:01   ` Mark Rutland
  2019-07-29 10:54 ` [PATCH v3 0/2] Add CCPI2 PMU support Ganapatrao Kulkarni
  2 siblings, 1 reply; 10+ messages in thread
From: Ganapatrao Kulkarni @ 2019-07-23  9:16 UTC (permalink / raw)
  To: linux-doc, linux-kernel, linux-arm-kernel
  Cc: mark.rutland, corbet, Jan Glauber,
	Jayachandran Chandrasekharan Nair, gklkml16, Robert Richter,
	will

CCPI2 is a low-latency high-bandwidth serial interface for connecting
ThunderX2 processors. This patch adds support to capture CCPI2 perf events.

Signed-off-by: Ganapatrao Kulkarni <gkulkarni@marvell.com>
---
 drivers/perf/thunderx2_pmu.c | 248 ++++++++++++++++++++++++++++++-----
 1 file changed, 214 insertions(+), 34 deletions(-)

diff --git a/drivers/perf/thunderx2_pmu.c b/drivers/perf/thunderx2_pmu.c
index 43d76c85da56..a4e1273eafa3 100644
--- a/drivers/perf/thunderx2_pmu.c
+++ b/drivers/perf/thunderx2_pmu.c
@@ -17,22 +17,31 @@
  */
 
 #define TX2_PMU_MAX_COUNTERS		4
+
 #define TX2_PMU_DMC_CHANNELS		8
 #define TX2_PMU_L3_TILES		16
 
 #define TX2_PMU_HRTIMER_INTERVAL	(2 * NSEC_PER_SEC)
-#define GET_EVENTID(ev)			((ev->hw.config) & 0x1f)
-#define GET_COUNTERID(ev)		((ev->hw.idx) & 0x3)
+#define GET_EVENTID(ev, mask)		((ev->hw.config) & mask)
+#define GET_COUNTERID(ev, mask)		((ev->hw.idx) & mask)
  /* 1 byte per counter(4 counters).
   * Event id is encoded in bits [5:1] of a byte,
   */
 #define DMC_EVENT_CFG(idx, val)		((val) << (((idx) * 8) + 1))
 
+/* bits[3:0] to select counters, are indexed from 8 to 15. */
+#define CCPI2_COUNTER_OFFSET		8
+
 #define L3C_COUNTER_CTL			0xA8
 #define L3C_COUNTER_DATA		0xAC
 #define DMC_COUNTER_CTL			0x234
 #define DMC_COUNTER_DATA		0x240
 
+#define CCPI2_PERF_CTL			0x108
+#define CCPI2_COUNTER_CTL		0x10C
+#define CCPI2_COUNTER_SEL		0x12c
+#define CCPI2_COUNTER_DATA_L		0x130
+
 /* L3C event IDs */
 #define L3_EVENT_READ_REQ		0xD
 #define L3_EVENT_WRITEBACK_REQ		0xE
@@ -51,15 +60,23 @@
 #define DMC_EVENT_READ_TXNS		0xF
 #define DMC_EVENT_MAX			0x10
 
+#define CCPI2_EVENT_REQ_PKT_SENT	0x3D
+#define CCPI2_EVENT_SNOOP_PKT_SENT	0x65
+#define CCPI2_EVENT_DATA_PKT_SENT	0x105
+#define CCPI2_EVENT_GIC_PKT_SENT	0x12D
+#define CCPI2_EVENT_MAX			0x200
+
 enum tx2_uncore_type {
 	PMU_TYPE_L3C,
 	PMU_TYPE_DMC,
+	PMU_TYPE_CCPI2,
 	PMU_TYPE_INVALID,
 };
 
+
 /*
- * pmu on each socket has 2 uncore devices(dmc and l3c),
- * each device has 4 counters.
+ * pmu on each socket has 3 uncore devices(dmc, l3ci and ccpi2),
+ * dmc and l3c has 4 counters and ccpi2 8.
  */
 struct tx2_uncore_pmu {
 	struct hlist_node hpnode;
@@ -69,12 +86,14 @@ struct tx2_uncore_pmu {
 	int node;
 	int cpu;
 	u32 max_counters;
+	u32 counters_mask;
 	u32 prorate_factor;
 	u32 max_events;
+	u32 events_mask;
 	u64 hrtimer_interval;
 	void __iomem *base;
 	DECLARE_BITMAP(active_counters, TX2_PMU_MAX_COUNTERS);
-	struct perf_event *events[TX2_PMU_MAX_COUNTERS];
+	struct perf_event **events;
 	struct device *dev;
 	struct hrtimer hrtimer;
 	const struct attribute_group **attr_groups;
@@ -92,7 +111,21 @@ static inline struct tx2_uncore_pmu *pmu_to_tx2_pmu(struct pmu *pmu)
 	return container_of(pmu, struct tx2_uncore_pmu, pmu);
 }
 
-PMU_FORMAT_ATTR(event,	"config:0-4");
+#define TX2_PMU_FORMAT_ATTR(_var, _name, _format)			\
+static ssize_t								\
+__tx2_pmu_##_var##_show(struct device *dev,				\
+			       struct device_attribute *attr,		\
+			       char *page)				\
+{									\
+	BUILD_BUG_ON(sizeof(_format) >= PAGE_SIZE);			\
+	return sprintf(page, _format "\n");				\
+}									\
+									\
+static struct device_attribute format_attr_##_var =			\
+	__ATTR(_name, 0444, __tx2_pmu_##_var##_show, NULL)
+
+TX2_PMU_FORMAT_ATTR(event, event, "config:0-4");
+TX2_PMU_FORMAT_ATTR(event_ccpi2, event, "config:0-9");
 
 static struct attribute *l3c_pmu_format_attrs[] = {
 	&format_attr_event.attr,
@@ -104,6 +137,11 @@ static struct attribute *dmc_pmu_format_attrs[] = {
 	NULL,
 };
 
+static struct attribute *ccpi2_pmu_format_attrs[] = {
+	&format_attr_event_ccpi2.attr,
+	NULL,
+};
+
 static const struct attribute_group l3c_pmu_format_attr_group = {
 	.name = "format",
 	.attrs = l3c_pmu_format_attrs,
@@ -114,6 +152,11 @@ static const struct attribute_group dmc_pmu_format_attr_group = {
 	.attrs = dmc_pmu_format_attrs,
 };
 
+static const struct attribute_group ccpi2_pmu_format_attr_group = {
+	.name = "format",
+	.attrs = ccpi2_pmu_format_attrs,
+};
+
 /*
  * sysfs event attributes
  */
@@ -164,6 +207,19 @@ static struct attribute *dmc_pmu_events_attrs[] = {
 	NULL,
 };
 
+TX2_EVENT_ATTR(req_pktsent, CCPI2_EVENT_REQ_PKT_SENT);
+TX2_EVENT_ATTR(snoop_pktsent, CCPI2_EVENT_SNOOP_PKT_SENT);
+TX2_EVENT_ATTR(data_pktsent, CCPI2_EVENT_DATA_PKT_SENT);
+TX2_EVENT_ATTR(gic_pktsent, CCPI2_EVENT_GIC_PKT_SENT);
+
+static struct attribute *ccpi2_pmu_events_attrs[] = {
+	&tx2_pmu_event_attr_req_pktsent.attr.attr,
+	&tx2_pmu_event_attr_snoop_pktsent.attr.attr,
+	&tx2_pmu_event_attr_data_pktsent.attr.attr,
+	&tx2_pmu_event_attr_gic_pktsent.attr.attr,
+	NULL,
+};
+
 static const struct attribute_group l3c_pmu_events_attr_group = {
 	.name = "events",
 	.attrs = l3c_pmu_events_attrs,
@@ -174,6 +230,11 @@ static const struct attribute_group dmc_pmu_events_attr_group = {
 	.attrs = dmc_pmu_events_attrs,
 };
 
+static const struct attribute_group ccpi2_pmu_events_attr_group = {
+	.name = "events",
+	.attrs = ccpi2_pmu_events_attrs,
+};
+
 /*
  * sysfs cpumask attributes
  */
@@ -213,11 +274,23 @@ static const struct attribute_group *dmc_pmu_attr_groups[] = {
 	NULL
 };
 
+static const struct attribute_group *ccpi2_pmu_attr_groups[] = {
+	&ccpi2_pmu_format_attr_group,
+	&pmu_cpumask_attr_group,
+	&ccpi2_pmu_events_attr_group,
+	NULL
+};
+
 static inline u32 reg_readl(unsigned long addr)
 {
 	return readl((void __iomem *)addr);
 }
 
+static inline u32 reg_readq(unsigned long addr)
+{
+	return readq((void __iomem *)addr);
+}
+
 static inline void reg_writel(u32 val, unsigned long addr)
 {
 	writel(val, (void __iomem *)addr);
@@ -245,33 +318,58 @@ static void init_cntr_base_l3c(struct perf_event *event,
 		struct tx2_uncore_pmu *tx2_pmu)
 {
 	struct hw_perf_event *hwc = &event->hw;
+	u32 cmask;
+
+	tx2_pmu = pmu_to_tx2_pmu(event->pmu);
+	cmask = tx2_pmu->counters_mask;
 
 	/* counter ctrl/data reg offset at 8 */
 	hwc->config_base = (unsigned long)tx2_pmu->base
-		+ L3C_COUNTER_CTL + (8 * GET_COUNTERID(event));
+		+ L3C_COUNTER_CTL + (8 * GET_COUNTERID(event, cmask));
 	hwc->event_base =  (unsigned long)tx2_pmu->base
-		+ L3C_COUNTER_DATA + (8 * GET_COUNTERID(event));
+		+ L3C_COUNTER_DATA + (8 * GET_COUNTERID(event, cmask));
 }
 
 static void init_cntr_base_dmc(struct perf_event *event,
 		struct tx2_uncore_pmu *tx2_pmu)
 {
 	struct hw_perf_event *hwc = &event->hw;
+	u32 cmask;
+
+	tx2_pmu = pmu_to_tx2_pmu(event->pmu);
+	cmask = tx2_pmu->counters_mask;
 
 	hwc->config_base = (unsigned long)tx2_pmu->base
 		+ DMC_COUNTER_CTL;
 	/* counter data reg offset at 0xc */
 	hwc->event_base = (unsigned long)tx2_pmu->base
-		+ DMC_COUNTER_DATA + (0xc * GET_COUNTERID(event));
+		+ DMC_COUNTER_DATA + (0xc * GET_COUNTERID(event, cmask));
+}
+
+static void init_cntr_base_ccpi2(struct perf_event *event,
+		struct tx2_uncore_pmu *tx2_pmu)
+{
+	struct hw_perf_event *hwc = &event->hw;
+	u32 cmask;
+
+	cmask = tx2_pmu->counters_mask;
+
+	hwc->config_base = (unsigned long)tx2_pmu->base
+		+ CCPI2_COUNTER_CTL + (4 * GET_COUNTERID(event, cmask));
+	hwc->event_base =  (unsigned long)tx2_pmu->base;
 }
 
 static void uncore_start_event_l3c(struct perf_event *event, int flags)
 {
-	u32 val;
+	u32 val, emask;
 	struct hw_perf_event *hwc = &event->hw;
+	struct tx2_uncore_pmu *tx2_pmu;
+
+	tx2_pmu = pmu_to_tx2_pmu(event->pmu);
+	emask = tx2_pmu->events_mask;
 
 	/* event id encoded in bits [07:03] */
-	val = GET_EVENTID(event) << 3;
+	val = GET_EVENTID(event, emask) << 3;
 	reg_writel(val, hwc->config_base);
 	local64_set(&hwc->prev_count, 0);
 	reg_writel(0, hwc->event_base);
@@ -284,10 +382,17 @@ static inline void uncore_stop_event_l3c(struct perf_event *event)
 
 static void uncore_start_event_dmc(struct perf_event *event, int flags)
 {
-	u32 val;
+	u32 val, cmask, emask;
 	struct hw_perf_event *hwc = &event->hw;
-	int idx = GET_COUNTERID(event);
-	int event_id = GET_EVENTID(event);
+	struct tx2_uncore_pmu *tx2_pmu;
+	int idx, event_id;
+
+	tx2_pmu = pmu_to_tx2_pmu(event->pmu);
+	cmask = tx2_pmu->counters_mask;
+	emask = tx2_pmu->events_mask;
+
+	idx = GET_COUNTERID(event, cmask);
+	event_id = GET_EVENTID(event, emask);
 
 	/* enable and start counters.
 	 * 8 bits for each counter, bits[05:01] of a counter to set event type.
@@ -302,9 +407,14 @@ static void uncore_start_event_dmc(struct perf_event *event, int flags)
 
 static void uncore_stop_event_dmc(struct perf_event *event)
 {
-	u32 val;
+	u32 val, cmask;
 	struct hw_perf_event *hwc = &event->hw;
-	int idx = GET_COUNTERID(event);
+	struct tx2_uncore_pmu *tx2_pmu;
+	int idx;
+
+	tx2_pmu = pmu_to_tx2_pmu(event->pmu);
+	cmask = tx2_pmu->counters_mask;
+	idx = GET_COUNTERID(event, cmask);
 
 	/* clear event type(bits[05:01]) to stop counter */
 	val = reg_readl(hwc->config_base);
@@ -312,6 +422,31 @@ static void uncore_stop_event_dmc(struct perf_event *event)
 	reg_writel(val, hwc->config_base);
 }
 
+static void uncore_start_event_ccpi2(struct perf_event *event, int flags)
+{
+	u32 emask;
+	struct hw_perf_event *hwc = &event->hw;
+	struct tx2_uncore_pmu *tx2_pmu;
+
+	tx2_pmu = pmu_to_tx2_pmu(event->pmu);
+	emask = tx2_pmu->events_mask;
+
+	/* Bit [09:00] to set event id, set level and type to 1 */
+	reg_writel((3 << 10) |
+			GET_EVENTID(event, emask), hwc->config_base);
+	/* reset[4], enable[0] and start[1] counters */
+	reg_writel(0x13, hwc->event_base + CCPI2_PERF_CTL);
+	local64_set(&event->hw.prev_count, 0ULL);
+}
+
+static void uncore_stop_event_ccpi2(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+
+	/* disable and stop counter */
+	reg_writel(0, hwc->event_base + CCPI2_PERF_CTL);
+}
+
 static void tx2_uncore_event_update(struct perf_event *event)
 {
 	s64 prev, delta, new = 0;
@@ -319,20 +454,30 @@ static void tx2_uncore_event_update(struct perf_event *event)
 	struct tx2_uncore_pmu *tx2_pmu;
 	enum tx2_uncore_type type;
 	u32 prorate_factor;
+	u32 cmask, emask;
 
 	tx2_pmu = pmu_to_tx2_pmu(event->pmu);
 	type = tx2_pmu->type;
+	cmask = tx2_pmu->counters_mask;
+	emask = tx2_pmu->events_mask;
 	prorate_factor = tx2_pmu->prorate_factor;
-
-	new = reg_readl(hwc->event_base);
-	prev = local64_xchg(&hwc->prev_count, new);
-
-	/* handles rollover of 32 bit counter */
-	delta = (u32)(((1UL << 32) - prev) + new);
+	if (type == PMU_TYPE_CCPI2) {
+		reg_writel(CCPI2_COUNTER_OFFSET +
+				GET_COUNTERID(event, cmask),
+				hwc->event_base + CCPI2_COUNTER_SEL);
+		new = reg_readq(hwc->event_base + CCPI2_COUNTER_DATA_L);
+		prev = local64_xchg(&hwc->prev_count, new);
+		delta = new - prev;
+	} else {
+		new = reg_readl(hwc->event_base);
+		prev = local64_xchg(&hwc->prev_count, new);
+		/* handles rollover of 32 bit counter */
+		delta = (u32)(((1UL << 32) - prev) + new);
+	}
 
 	/* DMC event data_transfers granularity is 16 Bytes, convert it to 64 */
 	if (type == PMU_TYPE_DMC &&
-			GET_EVENTID(event) == DMC_EVENT_DATA_TRANSFERS)
+			GET_EVENTID(event, emask) == DMC_EVENT_DATA_TRANSFERS)
 		delta = delta/4;
 
 	/* L3C and DMC has 16 and 8 interleave channels respectively.
@@ -351,6 +496,7 @@ static enum tx2_uncore_type get_tx2_pmu_type(struct acpi_device *adev)
 	} devices[] = {
 		{"CAV901D", PMU_TYPE_L3C},
 		{"CAV901F", PMU_TYPE_DMC},
+		{"CAV901E", PMU_TYPE_CCPI2},
 		{"", PMU_TYPE_INVALID}
 	};
 
@@ -380,7 +526,8 @@ static bool tx2_uncore_validate_event(struct pmu *pmu,
  * Make sure the group of events can be scheduled at once
  * on the PMU.
  */
-static bool tx2_uncore_validate_event_group(struct perf_event *event)
+static bool tx2_uncore_validate_event_group(struct perf_event *event,
+		int max_counters)
 {
 	struct perf_event *sibling, *leader = event->group_leader;
 	int counters = 0;
@@ -403,7 +550,7 @@ static bool tx2_uncore_validate_event_group(struct perf_event *event)
 	 * If the group requires more counters than the HW has,
 	 * it cannot ever be scheduled.
 	 */
-	return counters <= TX2_PMU_MAX_COUNTERS;
+	return counters <= max_counters;
 }
 
 
@@ -439,7 +586,7 @@ static int tx2_uncore_event_init(struct perf_event *event)
 	hwc->config = event->attr.config;
 
 	/* Validate the group */
-	if (!tx2_uncore_validate_event_group(event))
+	if (!tx2_uncore_validate_event_group(event, tx2_pmu->max_counters))
 		return -EINVAL;
 
 	return 0;
@@ -456,8 +603,9 @@ static void tx2_uncore_event_start(struct perf_event *event, int flags)
 	tx2_pmu->start_event(event, flags);
 	perf_event_update_userpage(event);
 
-	/* Start timer for first event */
-	if (bitmap_weight(tx2_pmu->active_counters,
+	/* Start timer for first non ccpi2 event */
+	if (tx2_pmu->type != PMU_TYPE_CCPI2 &&
+			bitmap_weight(tx2_pmu->active_counters,
 				tx2_pmu->max_counters) == 1) {
 		hrtimer_start(&tx2_pmu->hrtimer,
 			ns_to_ktime(tx2_pmu->hrtimer_interval),
@@ -495,7 +643,8 @@ static int tx2_uncore_event_add(struct perf_event *event, int flags)
 	if (hwc->idx < 0)
 		return -EAGAIN;
 
-	tx2_pmu->events[hwc->idx] = event;
+	if (tx2_pmu->events)
+		tx2_pmu->events[hwc->idx] = event;
 	/* set counter control and data registers base address */
 	tx2_pmu->init_cntr_base(event, tx2_pmu);
 
@@ -510,14 +659,17 @@ static void tx2_uncore_event_del(struct perf_event *event, int flags)
 {
 	struct tx2_uncore_pmu *tx2_pmu = pmu_to_tx2_pmu(event->pmu);
 	struct hw_perf_event *hwc = &event->hw;
+	u32 cmask;
 
+	cmask = tx2_pmu->counters_mask;
 	tx2_uncore_event_stop(event, PERF_EF_UPDATE);
 
 	/* clear the assigned counter */
-	free_counter(tx2_pmu, GET_COUNTERID(event));
+	free_counter(tx2_pmu, GET_COUNTERID(event, cmask));
 
 	perf_event_update_userpage(event);
-	tx2_pmu->events[hwc->idx] = NULL;
+	if (tx2_pmu->events)
+		tx2_pmu->events[hwc->idx] = NULL;
 	hwc->idx = -1;
 }
 
@@ -580,8 +732,12 @@ static int tx2_uncore_pmu_add_dev(struct tx2_uncore_pmu *tx2_pmu)
 			cpu_online_mask);
 
 	tx2_pmu->cpu = cpu;
-	hrtimer_init(&tx2_pmu->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
-	tx2_pmu->hrtimer.function = tx2_hrtimer_callback;
+	/* CCPI2 counters are 64 bit counters, no overflow  */
+	if (tx2_pmu->type != PMU_TYPE_CCPI2) {
+		hrtimer_init(&tx2_pmu->hrtimer,
+				CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+		tx2_pmu->hrtimer.function = tx2_hrtimer_callback;
+	}
 
 	ret = tx2_uncore_pmu_register(tx2_pmu);
 	if (ret) {
@@ -654,8 +810,10 @@ static struct tx2_uncore_pmu *tx2_uncore_pmu_init_dev(struct device *dev,
 	switch (tx2_pmu->type) {
 	case PMU_TYPE_L3C:
 		tx2_pmu->max_counters = TX2_PMU_MAX_COUNTERS;
+		tx2_pmu->counters_mask = 0x3;
 		tx2_pmu->prorate_factor = TX2_PMU_L3_TILES;
 		tx2_pmu->max_events = L3_EVENT_MAX;
+		tx2_pmu->events_mask = 0x1f;
 		tx2_pmu->hrtimer_interval = TX2_PMU_HRTIMER_INTERVAL;
 		tx2_pmu->attr_groups = l3c_pmu_attr_groups;
 		tx2_pmu->name = devm_kasprintf(dev, GFP_KERNEL,
@@ -663,11 +821,15 @@ static struct tx2_uncore_pmu *tx2_uncore_pmu_init_dev(struct device *dev,
 		tx2_pmu->init_cntr_base = init_cntr_base_l3c;
 		tx2_pmu->start_event = uncore_start_event_l3c;
 		tx2_pmu->stop_event = uncore_stop_event_l3c;
+		tx2_pmu->events = devm_kzalloc(dev, tx2_pmu->max_counters *
+				sizeof(*tx2_pmu->events), GFP_KERNEL);
 		break;
 	case PMU_TYPE_DMC:
 		tx2_pmu->max_counters = TX2_PMU_MAX_COUNTERS;
+		tx2_pmu->counters_mask = 0x3;
 		tx2_pmu->prorate_factor = TX2_PMU_DMC_CHANNELS;
 		tx2_pmu->max_events = DMC_EVENT_MAX;
+		tx2_pmu->events_mask = 0x1f;
 		tx2_pmu->hrtimer_interval = TX2_PMU_HRTIMER_INTERVAL;
 		tx2_pmu->attr_groups = dmc_pmu_attr_groups;
 		tx2_pmu->name = devm_kasprintf(dev, GFP_KERNEL,
@@ -675,6 +837,23 @@ static struct tx2_uncore_pmu *tx2_uncore_pmu_init_dev(struct device *dev,
 		tx2_pmu->init_cntr_base = init_cntr_base_dmc;
 		tx2_pmu->start_event = uncore_start_event_dmc;
 		tx2_pmu->stop_event = uncore_stop_event_dmc;
+		tx2_pmu->events = devm_kzalloc(dev, tx2_pmu->max_counters *
+				sizeof(*tx2_pmu->events), GFP_KERNEL);
+		break;
+	case PMU_TYPE_CCPI2:
+		/* CCPI2 has 8 counters */
+		tx2_pmu->max_counters = 8;
+		tx2_pmu->counters_mask = 0x7;
+		tx2_pmu->prorate_factor = 1;
+		tx2_pmu->max_events = CCPI2_EVENT_MAX;
+		tx2_pmu->events_mask = 0x1ff;
+		tx2_pmu->attr_groups = ccpi2_pmu_attr_groups;
+		tx2_pmu->name = devm_kasprintf(dev, GFP_KERNEL,
+				"uncore_ccpi2_%d", tx2_pmu->node);
+		tx2_pmu->init_cntr_base = init_cntr_base_ccpi2;
+		tx2_pmu->start_event = uncore_start_event_ccpi2;
+		tx2_pmu->stop_event = uncore_stop_event_ccpi2;
+		tx2_pmu->events = NULL;
 		break;
 	case PMU_TYPE_INVALID:
 		devm_kfree(dev, tx2_pmu);
@@ -744,7 +923,8 @@ static int tx2_uncore_pmu_offline_cpu(unsigned int cpu,
 	if (cpu != tx2_pmu->cpu)
 		return 0;
 
-	hrtimer_cancel(&tx2_pmu->hrtimer);
+	if (tx2_pmu->type != PMU_TYPE_CCPI2)
+		hrtimer_cancel(&tx2_pmu->hrtimer);
 	cpumask_copy(&cpu_online_mask_temp, cpu_online_mask);
 	cpumask_clear_cpu(cpu, &cpu_online_mask_temp);
 	new_cpu = cpumask_any_and(
-- 
2.17.1


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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 0/2] Add CCPI2 PMU support
  2019-07-23  9:16 [PATCH v3 0/2] Add CCPI2 PMU support Ganapatrao Kulkarni
  2019-07-23  9:16 ` [PATCH v3 1/2] Documentation: perf: Update documentation for ThunderX2 PMU uncore driver Ganapatrao Kulkarni
  2019-07-23  9:16 ` [PATCH v3 2/2] drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver Ganapatrao Kulkarni
@ 2019-07-29 10:54 ` Ganapatrao Kulkarni
  2019-08-05  3:47   ` Ganapatrao Kulkarni
  2 siblings, 1 reply; 10+ messages in thread
From: Ganapatrao Kulkarni @ 2019-07-29 10:54 UTC (permalink / raw)
  To: Ganapatrao Kulkarni, will
  Cc: mark.rutland, corbet, Jan Glauber, linux-doc, linux-kernel,
	Robert Richter, Jayachandran Chandrasekharan Nair,
	linux-arm-kernel

Hi Will,

Any comments to this patchset?

On Tue, Jul 23, 2019 at 2:46 PM Ganapatrao Kulkarni
<gkulkarni@marvell.com> wrote:
>
> Add Cavium Coherent Processor Interconnect (CCPI2) PMU
> support in ThunderX2 Uncore driver.
>
> v3: Rebased to 5.3-rc1
>
> v2: Updated with review comments [1]
>
> [1] https://lkml.org/lkml/2019/6/14/965
>
> v1: initial patch
>
> Ganapatrao Kulkarni (2):
>   Documentation: perf: Update documentation for ThunderX2 PMU uncore
>     driver
>   drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver.
>
>  .../admin-guide/perf/thunderx2-pmu.rst        |  20 +-
>  drivers/perf/thunderx2_pmu.c                  | 248 +++++++++++++++---
>  2 files changed, 225 insertions(+), 43 deletions(-)
>
> --
> 2.17.1
>

Thanks,
Ganapat

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 0/2] Add CCPI2 PMU support
  2019-07-29 10:54 ` [PATCH v3 0/2] Add CCPI2 PMU support Ganapatrao Kulkarni
@ 2019-08-05  3:47   ` Ganapatrao Kulkarni
  0 siblings, 0 replies; 10+ messages in thread
From: Ganapatrao Kulkarni @ 2019-08-05  3:47 UTC (permalink / raw)
  To: Ganapatrao Kulkarni, will
  Cc: mark.rutland, corbet, Jan Glauber, linux-doc, linux-kernel,
	Robert Richter, Jayachandran Chandrasekharan Nair,
	linux-arm-kernel

On Mon, Jul 29, 2019 at 4:24 PM Ganapatrao Kulkarni <gklkml16@gmail.com> wrote:
>
> Hi Will,
>
> Any comments to this patchset?

If no further comments, can it be queued please?
>
> On Tue, Jul 23, 2019 at 2:46 PM Ganapatrao Kulkarni
> <gkulkarni@marvell.com> wrote:
> >
> > Add Cavium Coherent Processor Interconnect (CCPI2) PMU
> > support in ThunderX2 Uncore driver.
> >
> > v3: Rebased to 5.3-rc1
> >
> > v2: Updated with review comments [1]
> >
> > [1] https://lkml.org/lkml/2019/6/14/965
> >
> > v1: initial patch
> >
> > Ganapatrao Kulkarni (2):
> >   Documentation: perf: Update documentation for ThunderX2 PMU uncore
> >     driver
> >   drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver.
> >
> >  .../admin-guide/perf/thunderx2-pmu.rst        |  20 +-
> >  drivers/perf/thunderx2_pmu.c                  | 248 +++++++++++++++---
> >  2 files changed, 225 insertions(+), 43 deletions(-)
> >
> > --
> > 2.17.1
> >
>
> Thanks,
> Ganapat

Thanks,
Ganapat

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/2] drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver.
  2019-07-23  9:16 ` [PATCH v3 2/2] drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver Ganapatrao Kulkarni
@ 2019-08-12 12:01   ` Mark Rutland
  2019-08-13 10:55     ` Ganapatrao Kulkarni
  0 siblings, 1 reply; 10+ messages in thread
From: Mark Rutland @ 2019-08-12 12:01 UTC (permalink / raw)
  To: Ganapatrao Kulkarni
  Cc: corbet, Jan Glauber, linux-doc, linux-kernel, Robert Richter,
	gklkml16, Jayachandran Chandrasekharan Nair, will,
	linux-arm-kernel

On Tue, Jul 23, 2019 at 09:16:28AM +0000, Ganapatrao Kulkarni wrote:
> CCPI2 is a low-latency high-bandwidth serial interface for connecting
> ThunderX2 processors. This patch adds support to capture CCPI2 perf events.

It would be worth pointing out in the commit message how the CCPI2
counters differ from the others. I realise you have that in the body of
patch 1, but it's critical information when reviewing this patch...

> 
> Signed-off-by: Ganapatrao Kulkarni <gkulkarni@marvell.com>
> ---
>  drivers/perf/thunderx2_pmu.c | 248 ++++++++++++++++++++++++++++++-----
>  1 file changed, 214 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/perf/thunderx2_pmu.c b/drivers/perf/thunderx2_pmu.c
> index 43d76c85da56..a4e1273eafa3 100644
> --- a/drivers/perf/thunderx2_pmu.c
> +++ b/drivers/perf/thunderx2_pmu.c
> @@ -17,22 +17,31 @@
>   */
>  
>  #define TX2_PMU_MAX_COUNTERS		4

Shouldn't this be 8 now?

[...]

>  /*
> - * pmu on each socket has 2 uncore devices(dmc and l3c),
> - * each device has 4 counters.
> + * pmu on each socket has 3 uncore devices(dmc, l3ci and ccpi2),
> + * dmc and l3c has 4 counters and ccpi2 8.
>   */

How about:

/*
 * Each socket has 3 uncore device associated with a PMU. The DMC and
 * L3C have 4 32-bit counters, and the CCPI2 has 8 64-bit counters.
 */

>  struct tx2_uncore_pmu {
>  	struct hlist_node hpnode;
> @@ -69,12 +86,14 @@ struct tx2_uncore_pmu {
>  	int node;
>  	int cpu;
>  	u32 max_counters;
> +	u32 counters_mask;
>  	u32 prorate_factor;
>  	u32 max_events;
> +	u32 events_mask;
>  	u64 hrtimer_interval;
>  	void __iomem *base;
>  	DECLARE_BITMAP(active_counters, TX2_PMU_MAX_COUNTERS);

This bitmap isn't big enough for the 4 new counters.

> -	struct perf_event *events[TX2_PMU_MAX_COUNTERS];
> +	struct perf_event **events;

As above, can't we bump TX2_PMU_MAX_COUNTERS to 8 rather than making
this a dynamic allocation?

[...]

>  static inline u32 reg_readl(unsigned long addr)
>  {
>  	return readl((void __iomem *)addr);
>  }
>  
> +static inline u32 reg_readq(unsigned long addr)
> +{
> +	return readq((void __iomem *)addr);
> +}

Presumably reg_readq() should return a u64.

[...]

> +static void uncore_start_event_ccpi2(struct perf_event *event, int flags)
> +{
> +	u32 emask;
> +	struct hw_perf_event *hwc = &event->hw;
> +	struct tx2_uncore_pmu *tx2_pmu;
> +
> +	tx2_pmu = pmu_to_tx2_pmu(event->pmu);
> +	emask = tx2_pmu->events_mask;
> +
> +	/* Bit [09:00] to set event id, set level and type to 1 */
> +	reg_writel((3 << 10) |

Do you mean that bits [11:10] are level and type?

What exactly are 'level' and 'type'?

Can we give those bits mnemonics?

> +			GET_EVENTID(event, emask), hwc->config_base);
> +	/* reset[4], enable[0] and start[1] counters */

Rather than using magic numbers everywhere, please give these mnemonics,
e.g.

#define CCPI2_PERF_CTL_ENABLE	BIT(0)
#define CCPI2_PERF_CTL_START	BIT(1)
#define CCPI2_PERF_CTL_RESET	BIT(4)

> +	reg_writel(0x13, hwc->event_base + CCPI2_PERF_CTL);

... and then you can OR them in here:

	ctl = CCPI2_PERF_CTL_ENABLE |
	      CCPI2_PERF_CTL_START |
	      CCPI2_PERF_CTL_RESET;
	reg_writel(ctl, hwc->event_base + CCPI2_PERF_CTL);

[...]

> @@ -456,8 +603,9 @@ static void tx2_uncore_event_start(struct perf_event *event, int flags)
>  	tx2_pmu->start_event(event, flags);
>  	perf_event_update_userpage(event);
>  
> -	/* Start timer for first event */
> -	if (bitmap_weight(tx2_pmu->active_counters,
> +	/* Start timer for first non ccpi2 event */
> +	if (tx2_pmu->type != PMU_TYPE_CCPI2 &&
> +			bitmap_weight(tx2_pmu->active_counters,
>  				tx2_pmu->max_counters) == 1) {
>  		hrtimer_start(&tx2_pmu->hrtimer,
>  			ns_to_ktime(tx2_pmu->hrtimer_interval),

This would be easier to read as two statements:

	/* No hrtimer needed with 64-bit counters */
	if (tx2_pmu->type == PMU_TYPE_CCPI2)
		return;
	
	/* Start timer for first event */
	if (bitmap_weight(tx2_pmu->active_counters,
	    tx2_pmu->max_counters) != 1) {
	    	...
	}

> @@ -495,7 +643,8 @@ static int tx2_uncore_event_add(struct perf_event *event, int flags)
>  	if (hwc->idx < 0)
>  		return -EAGAIN;
>  
> -	tx2_pmu->events[hwc->idx] = event;
> +	if (tx2_pmu->events)
> +		tx2_pmu->events[hwc->idx] = event;

So this is NULL for CCPI2?

I guess we don't strictly need the if we don't have a hrtimer to update
event counts, but this makes the code more complicated than it needs to
be.

[...]

> @@ -580,8 +732,12 @@ static int tx2_uncore_pmu_add_dev(struct tx2_uncore_pmu *tx2_pmu)
>  			cpu_online_mask);
>  
>  	tx2_pmu->cpu = cpu;
> -	hrtimer_init(&tx2_pmu->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> -	tx2_pmu->hrtimer.function = tx2_hrtimer_callback;
> +	/* CCPI2 counters are 64 bit counters, no overflow  */
> +	if (tx2_pmu->type != PMU_TYPE_CCPI2) {
> +		hrtimer_init(&tx2_pmu->hrtimer,
> +				CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +		tx2_pmu->hrtimer.function = tx2_hrtimer_callback;
> +	}

Hmmm... this means that tx2_pmu->hrtimer.function is NULL for the CCPI2
PMU. I think it would be best to check that when (re)programming the
counters rather than the PMU type. For example, in
tx2_uncore_event_start() we could have:

	if (!tx2_pmu->hrtimer.function)
		return;
	if (bitmap_weight(tx2_pmu->active_counters,
	    tx2_pmu->max_counters) != 1) {
	    	...
	}

Thanks,
Mark.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/2] drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver.
  2019-08-12 12:01   ` Mark Rutland
@ 2019-08-13 10:55     ` Ganapatrao Kulkarni
  2019-08-13 11:03       ` Mark Rutland
  0 siblings, 1 reply; 10+ messages in thread
From: Ganapatrao Kulkarni @ 2019-08-13 10:55 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Ganapatrao Kulkarni, corbet, Jan Glauber, linux-doc,
	linux-kernel, Robert Richter, Jayachandran Chandrasekharan Nair,
	will, linux-arm-kernel

Hi Mark,

On Mon, Aug 12, 2019 at 5:31 PM Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Tue, Jul 23, 2019 at 09:16:28AM +0000, Ganapatrao Kulkarni wrote:
> > CCPI2 is a low-latency high-bandwidth serial interface for connecting
> > ThunderX2 processors. This patch adds support to capture CCPI2 perf events.
>
> It would be worth pointing out in the commit message how the CCPI2
> counters differ from the others. I realise you have that in the body of
> patch 1, but it's critical information when reviewing this patch...

Ok, I will add in next version.
>
> >
> > Signed-off-by: Ganapatrao Kulkarni <gkulkarni@marvell.com>
> > ---
> >  drivers/perf/thunderx2_pmu.c | 248 ++++++++++++++++++++++++++++++-----
> >  1 file changed, 214 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/perf/thunderx2_pmu.c b/drivers/perf/thunderx2_pmu.c
> > index 43d76c85da56..a4e1273eafa3 100644
> > --- a/drivers/perf/thunderx2_pmu.c
> > +++ b/drivers/perf/thunderx2_pmu.c
> > @@ -17,22 +17,31 @@
> >   */
> >
> >  #define TX2_PMU_MAX_COUNTERS         4
>
> Shouldn't this be 8 now?

It is kept unchanged to 4(as suggested by Will), which is same for
both L3 and DMC.
For CCPI2 this macro is not used.

>
> [...]
>
> >  /*
> > - * pmu on each socket has 2 uncore devices(dmc and l3c),
> > - * each device has 4 counters.
> > + * pmu on each socket has 3 uncore devices(dmc, l3ci and ccpi2),
> > + * dmc and l3c has 4 counters and ccpi2 8.
> >   */
>
> How about:
>
> /*
>  * Each socket has 3 uncore device associated with a PMU. The DMC and
>  * L3C have 4 32-bit counters, and the CCPI2 has 8 64-bit counters.
>  */

Thanks.
>
> >  struct tx2_uncore_pmu {
> >       struct hlist_node hpnode;
> > @@ -69,12 +86,14 @@ struct tx2_uncore_pmu {
> >       int node;
> >       int cpu;
> >       u32 max_counters;
> > +     u32 counters_mask;
> >       u32 prorate_factor;
> >       u32 max_events;
> > +     u32 events_mask;
> >       u64 hrtimer_interval;
> >       void __iomem *base;
> >       DECLARE_BITMAP(active_counters, TX2_PMU_MAX_COUNTERS);
>
> This bitmap isn't big enough for the 4 new counters.
>
> > -     struct perf_event *events[TX2_PMU_MAX_COUNTERS];
> > +     struct perf_event **events;
>
> As above, can't we bump TX2_PMU_MAX_COUNTERS to 8 rather than making
> this a dynamic allocation?

events is only relevant for L3 and DMC since they use timer callbacks.
This is done as per previous review comments.

>
> [...]
>
> >  static inline u32 reg_readl(unsigned long addr)
> >  {
> >       return readl((void __iomem *)addr);
> >  }
> >
> > +static inline u32 reg_readq(unsigned long addr)
> > +{
> > +     return readq((void __iomem *)addr);
> > +}
>
> Presumably reg_readq() should return a u64.

Yes,  My bad.

>
> [...]
>
> > +static void uncore_start_event_ccpi2(struct perf_event *event, int flags)
> > +{
> > +     u32 emask;
> > +     struct hw_perf_event *hwc = &event->hw;
> > +     struct tx2_uncore_pmu *tx2_pmu;
> > +
> > +     tx2_pmu = pmu_to_tx2_pmu(event->pmu);
> > +     emask = tx2_pmu->events_mask;
> > +
> > +     /* Bit [09:00] to set event id, set level and type to 1 */
> > +     reg_writel((3 << 10) |
>
> Do you mean that bits [11:10] are level and type?

Yes, i will change the comment.
>
> What exactly are 'level' and 'type'?

They are for other settings which are not relevant for software/kernel.
>
> Can we give those bits mnemonics?
>
> > +                     GET_EVENTID(event, emask), hwc->config_base);
> > +     /* reset[4], enable[0] and start[1] counters */
>
> Rather than using magic numbers everywhere, please give these mnemonics,
> e.g.
>
> #define CCPI2_PERF_CTL_ENABLE   BIT(0)
> #define CCPI2_PERF_CTL_START    BIT(1)
> #define CCPI2_PERF_CTL_RESET    BIT(4)

not used everywhere, only in this function.
I can add these macros.

>
> > +     reg_writel(0x13, hwc->event_base + CCPI2_PERF_CTL);
>
> ... and then you can OR them in here:

OK
>
>         ctl = CCPI2_PERF_CTL_ENABLE |
>               CCPI2_PERF_CTL_START |
>               CCPI2_PERF_CTL_RESET;
>         reg_writel(ctl, hwc->event_base + CCPI2_PERF_CTL);
>
> [...]
>
> > @@ -456,8 +603,9 @@ static void tx2_uncore_event_start(struct perf_event *event, int flags)
> >       tx2_pmu->start_event(event, flags);
> >       perf_event_update_userpage(event);
> >
> > -     /* Start timer for first event */
> > -     if (bitmap_weight(tx2_pmu->active_counters,
> > +     /* Start timer for first non ccpi2 event */
> > +     if (tx2_pmu->type != PMU_TYPE_CCPI2 &&
> > +                     bitmap_weight(tx2_pmu->active_counters,
> >                               tx2_pmu->max_counters) == 1) {
> >               hrtimer_start(&tx2_pmu->hrtimer,
> >                       ns_to_ktime(tx2_pmu->hrtimer_interval),
>
> This would be easier to read as two statements:
>
>         /* No hrtimer needed with 64-bit counters */
>         if (tx2_pmu->type == PMU_TYPE_CCPI2)
>                 return;
>
>         /* Start timer for first event */
>         if (bitmap_weight(tx2_pmu->active_counters,
>             tx2_pmu->max_counters) != 1) {
>                 ...
>         }
>

OK, makes sense.

> > @@ -495,7 +643,8 @@ static int tx2_uncore_event_add(struct perf_event *event, int flags)
> >       if (hwc->idx < 0)
> >               return -EAGAIN;
> >
> > -     tx2_pmu->events[hwc->idx] = event;
> > +     if (tx2_pmu->events)
> > +             tx2_pmu->events[hwc->idx] = event;
>
> So this is NULL for CCPI2?

Yes.
>
> I guess we don't strictly need the if we don't have a hrtimer to update
> event counts, but this makes the code more complicated than it needs to
> be.

Yes I am using tx2_pmu->events to differentiate the type, it is NULL for CCPI2.
I can extend same to tx2_uncore_event_start().
>
> [...]
>
> > @@ -580,8 +732,12 @@ static int tx2_uncore_pmu_add_dev(struct tx2_uncore_pmu *tx2_pmu)
> >                       cpu_online_mask);
> >
> >       tx2_pmu->cpu = cpu;
> > -     hrtimer_init(&tx2_pmu->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > -     tx2_pmu->hrtimer.function = tx2_hrtimer_callback;
> > +     /* CCPI2 counters are 64 bit counters, no overflow  */
> > +     if (tx2_pmu->type != PMU_TYPE_CCPI2) {
> > +             hrtimer_init(&tx2_pmu->hrtimer,
> > +                             CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > +             tx2_pmu->hrtimer.function = tx2_hrtimer_callback;
> > +     }
>
> Hmmm... this means that tx2_pmu->hrtimer.function is NULL for the CCPI2
> PMU. I think it would be best to check that when (re)programming the
> counters rather than the PMU type. For example, in
> tx2_uncore_event_start() we could have:
>
>         if (!tx2_pmu->hrtimer.function)
>                 return;
>         if (bitmap_weight(tx2_pmu->active_counters,
>             tx2_pmu->max_counters) != 1) {
>                 ...
>         }
>

Yes it is NULL for CCPI2.
Ok, I can use tx2_pmu->events instead(like other places).

> Thanks,
> Mark.

Thanks,
Ganapat

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/2] drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver.
  2019-08-13 10:55     ` Ganapatrao Kulkarni
@ 2019-08-13 11:03       ` Mark Rutland
  2019-08-21 16:53         ` Will Deacon
  0 siblings, 1 reply; 10+ messages in thread
From: Mark Rutland @ 2019-08-13 11:03 UTC (permalink / raw)
  To: Ganapatrao Kulkarni
  Cc: Ganapatrao Kulkarni, corbet, Jan Glauber, linux-doc,
	linux-kernel, Robert Richter, Jayachandran Chandrasekharan Nair,
	will, linux-arm-kernel

On Tue, Aug 13, 2019 at 04:25:15PM +0530, Ganapatrao Kulkarni wrote:
> Hi Mark,
> 
> On Mon, Aug 12, 2019 at 5:31 PM Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > On Tue, Jul 23, 2019 at 09:16:28AM +0000, Ganapatrao Kulkarni wrote:
> > > CCPI2 is a low-latency high-bandwidth serial interface for connecting
> > > ThunderX2 processors. This patch adds support to capture CCPI2 perf events.
> >
> > It would be worth pointing out in the commit message how the CCPI2
> > counters differ from the others. I realise you have that in the body of
> > patch 1, but it's critical information when reviewing this patch...
> 
> Ok, I will add in next version.
> >
> > >
> > > Signed-off-by: Ganapatrao Kulkarni <gkulkarni@marvell.com>
> > > ---
> > >  drivers/perf/thunderx2_pmu.c | 248 ++++++++++++++++++++++++++++++-----
> > >  1 file changed, 214 insertions(+), 34 deletions(-)
> > >
> > > diff --git a/drivers/perf/thunderx2_pmu.c b/drivers/perf/thunderx2_pmu.c
> > > index 43d76c85da56..a4e1273eafa3 100644
> > > --- a/drivers/perf/thunderx2_pmu.c
> > > +++ b/drivers/perf/thunderx2_pmu.c
> > > @@ -17,22 +17,31 @@
> > >   */
> > >
> > >  #define TX2_PMU_MAX_COUNTERS         4
> >
> > Shouldn't this be 8 now?
> 
> It is kept unchanged to 4(as suggested by Will), which is same for
> both L3 and DMC.
> For CCPI2 this macro is not used.

Hmmm....

I disagree with that suggestion given that this also affects the
active_counters bitmap size (and thus it is not correctly sized as of
this patch), and it doesn't really save us much.

I think it would be better to bump this to 8 and always update the
events array, even though it will be unused for CCPI2. That's less
surprising, needs fewer special-cases, and we can use the hrtimer
function pointer alone to determine if we need to do any hrtimer work.

Thanks,
Mark.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/2] drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver.
  2019-08-13 11:03       ` Mark Rutland
@ 2019-08-21 16:53         ` Will Deacon
  2019-08-22  4:00           ` Ganapatrao Kulkarni
  0 siblings, 1 reply; 10+ messages in thread
From: Will Deacon @ 2019-08-21 16:53 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Ganapatrao Kulkarni, corbet, Jan Glauber, linux-doc,
	linux-kernel, Robert Richter, Ganapatrao Kulkarni,
	Jayachandran Chandrasekharan Nair, linux-arm-kernel

On Tue, Aug 13, 2019 at 12:03:45PM +0100, Mark Rutland wrote:
> On Tue, Aug 13, 2019 at 04:25:15PM +0530, Ganapatrao Kulkarni wrote:
> > On Mon, Aug 12, 2019 at 5:31 PM Mark Rutland <mark.rutland@arm.com> wrote:
> > >
> > > On Tue, Jul 23, 2019 at 09:16:28AM +0000, Ganapatrao Kulkarni wrote:
> > > > CCPI2 is a low-latency high-bandwidth serial interface for connecting
> > > > ThunderX2 processors. This patch adds support to capture CCPI2 perf events.
> > >
> > > It would be worth pointing out in the commit message how the CCPI2
> > > counters differ from the others. I realise you have that in the body of
> > > patch 1, but it's critical information when reviewing this patch...
> > 
> > Ok, I will add in next version.
> > >
> > > >
> > > > Signed-off-by: Ganapatrao Kulkarni <gkulkarni@marvell.com>
> > > > ---
> > > >  drivers/perf/thunderx2_pmu.c | 248 ++++++++++++++++++++++++++++++-----
> > > >  1 file changed, 214 insertions(+), 34 deletions(-)
> > > >
> > > > diff --git a/drivers/perf/thunderx2_pmu.c b/drivers/perf/thunderx2_pmu.c
> > > > index 43d76c85da56..a4e1273eafa3 100644
> > > > --- a/drivers/perf/thunderx2_pmu.c
> > > > +++ b/drivers/perf/thunderx2_pmu.c
> > > > @@ -17,22 +17,31 @@
> > > >   */
> > > >
> > > >  #define TX2_PMU_MAX_COUNTERS         4
> > >
> > > Shouldn't this be 8 now?
> > 
> > It is kept unchanged to 4(as suggested by Will), which is same for
> > both L3 and DMC.
> > For CCPI2 this macro is not used.
> 
> Hmmm....
> 
> I disagree with that suggestion given that this also affects the
> active_counters bitmap size (and thus it is not correctly sized as of
> this patch), and it doesn't really save us much.
> 
> I think it would be better to bump this to 8 and always update the
> events array, even though it will be unused for CCPI2. That's less
> surprising, needs fewer special-cases, and we can use the hrtimer
> function pointer alone to determine if we need to do any hrtimer work.

tbf, my complaint was actually about some macros applying to the whole
PMU whilst others refer only to DMC/L3C and this not being apparent from
the naming:

https://lkml.org/lkml/2019/6/27/250

so I'm fine having TX2_PMU_DMC_L3C_MAX_COUNTERS and
TX2_PMU_CCPI2_MAX_COUNTERS, but that sort of naming needs to be consistent
unless the macro/definition really applies to both. That fed the suggestion
that GET_EVENTID could be generic and switch on the event type internally
instead of at the caller.

Will

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v3 2/2] drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver.
  2019-08-21 16:53         ` Will Deacon
@ 2019-08-22  4:00           ` Ganapatrao Kulkarni
  0 siblings, 0 replies; 10+ messages in thread
From: Ganapatrao Kulkarni @ 2019-08-22  4:00 UTC (permalink / raw)
  To: Will Deacon
  Cc: Mark Rutland, Ganapatrao Kulkarni, corbet, Jan Glauber,
	linux-doc, linux-kernel, Robert Richter,
	Jayachandran Chandrasekharan Nair, linux-arm-kernel

On Wed, Aug 21, 2019 at 10:23 PM Will Deacon <will@kernel.org> wrote:
>
> On Tue, Aug 13, 2019 at 12:03:45PM +0100, Mark Rutland wrote:
> > On Tue, Aug 13, 2019 at 04:25:15PM +0530, Ganapatrao Kulkarni wrote:
> > > On Mon, Aug 12, 2019 at 5:31 PM Mark Rutland <mark.rutland@arm.com> wrote:
> > > >
> > > > On Tue, Jul 23, 2019 at 09:16:28AM +0000, Ganapatrao Kulkarni wrote:
> > > > > CCPI2 is a low-latency high-bandwidth serial interface for connecting
> > > > > ThunderX2 processors. This patch adds support to capture CCPI2 perf events.
> > > >
> > > > It would be worth pointing out in the commit message how the CCPI2
> > > > counters differ from the others. I realise you have that in the body of
> > > > patch 1, but it's critical information when reviewing this patch...
> > >
> > > Ok, I will add in next version.
> > > >
> > > > >
> > > > > Signed-off-by: Ganapatrao Kulkarni <gkulkarni@marvell.com>
> > > > > ---
> > > > >  drivers/perf/thunderx2_pmu.c | 248 ++++++++++++++++++++++++++++++-----
> > > > >  1 file changed, 214 insertions(+), 34 deletions(-)
> > > > >
> > > > > diff --git a/drivers/perf/thunderx2_pmu.c b/drivers/perf/thunderx2_pmu.c
> > > > > index 43d76c85da56..a4e1273eafa3 100644
> > > > > --- a/drivers/perf/thunderx2_pmu.c
> > > > > +++ b/drivers/perf/thunderx2_pmu.c
> > > > > @@ -17,22 +17,31 @@
> > > > >   */
> > > > >
> > > > >  #define TX2_PMU_MAX_COUNTERS         4
> > > >
> > > > Shouldn't this be 8 now?
> > >
> > > It is kept unchanged to 4(as suggested by Will), which is same for
> > > both L3 and DMC.
> > > For CCPI2 this macro is not used.
> >
> > Hmmm....
> >
> > I disagree with that suggestion given that this also affects the
> > active_counters bitmap size (and thus it is not correctly sized as of
> > this patch), and it doesn't really save us much.
> >
> > I think it would be better to bump this to 8 and always update the
> > events array, even though it will be unused for CCPI2. That's less
> > surprising, needs fewer special-cases, and we can use the hrtimer
> > function pointer alone to determine if we need to do any hrtimer work.
>
> tbf, my complaint was actually about some macros applying to the whole
> PMU whilst others refer only to DMC/L3C and this not being apparent from
> the naming:
>
> https://lkml.org/lkml/2019/6/27/250
>
> so I'm fine having TX2_PMU_DMC_L3C_MAX_COUNTERS and
> TX2_PMU_CCPI2_MAX_COUNTERS, but that sort of naming needs to be consistent
> unless the macro/definition really applies to both. That fed the suggestion
> that GET_EVENTID could be generic and switch on the event type internally
> instead of at the caller.

Thanks Will for the clarification.
I will send new version with changes as suggested.

>
> Will

Thanks,
Ganapat

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-08-22  4:01 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-23  9:16 [PATCH v3 0/2] Add CCPI2 PMU support Ganapatrao Kulkarni
2019-07-23  9:16 ` [PATCH v3 1/2] Documentation: perf: Update documentation for ThunderX2 PMU uncore driver Ganapatrao Kulkarni
2019-07-23  9:16 ` [PATCH v3 2/2] drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver Ganapatrao Kulkarni
2019-08-12 12:01   ` Mark Rutland
2019-08-13 10:55     ` Ganapatrao Kulkarni
2019-08-13 11:03       ` Mark Rutland
2019-08-21 16:53         ` Will Deacon
2019-08-22  4:00           ` Ganapatrao Kulkarni
2019-07-29 10:54 ` [PATCH v3 0/2] Add CCPI2 PMU support Ganapatrao Kulkarni
2019-08-05  3:47   ` Ganapatrao Kulkarni

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).