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...

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.
 */

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);

[...]

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) {
	    	...
	}

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.

[...]

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.