On 23/01/2019 12:14, Andrew Murray wrote:
[...]
+static inline void smmu_pmu_counter_set_value(struct smmu_pmu
*smmu_pmu,
+ u32 idx, u64 value)
+{
+ if (smmu_pmu->counter_mask & BIT(32))
+ writeq(value, smmu_pmu->reloc_base + SMMU_PMCG_EVCNTR(idx,
8));
+ else
+ writel(value, smmu_pmu->reloc_base + SMMU_PMCG_EVCNTR(idx,
4));
The arm64 IO macros use __force u32 and so it's probably OK to provide a 64
bit
value to writel. But you could use something like lower_32_bits for clarity.
Yes, macro uses __force u32. I will change it to make it more clear though.
To be pedantic, the first cast which the value actually undergoes is to
(__u32) ;)
Ultimately, __raw_writel() takes a u32, so in that sense it's never a
problem to pass a u64, since unsigned truncation is well-defined in the
C standard. The casting involved in the I/O accessors is mostly about
going to an endian-specific type and back again.
[...]
+static void smmu_pmu_event_start(struct perf_event *event, int flags)
+{
+ struct smmu_pmu *smmu_pmu = to_smmu_pmu(event->pmu);
+ struct hw_perf_event *hwc = &event->hw;
+ int idx = hwc->idx;
+ u32 filter_span, filter_sid;
+ u32 evtyper;
+
+ hwc->state = 0;
+
+ smmu_pmu_set_period(smmu_pmu, hwc);
+
+ smmu_pmu_get_event_filter(event, &filter_span, &filter_sid);
+
+ evtyper = get_event(event) |
+ filter_span << SMMU_PMCG_SID_SPAN_SHIFT;
+
+ smmu_pmu_set_evtyper(smmu_pmu, idx, evtyper);
+ smmu_pmu_set_smr(smmu_pmu, idx, filter_sid);
+ smmu_pmu_interrupt_enable(smmu_pmu, idx);
+ smmu_pmu_counter_enable(smmu_pmu, idx);
+}
+
+static void smmu_pmu_event_stop(struct perf_event *event, int flags)
+{
+ struct smmu_pmu *smmu_pmu = to_smmu_pmu(event->pmu);
+ struct hw_perf_event *hwc = &event->hw;
+ int idx = hwc->idx;
+
+ if (hwc->state & PERF_HES_STOPPED)
+ return;
+
+ smmu_pmu_counter_disable(smmu_pmu, idx);
Is it intentional not to call smmu_pmu_interrupt_disable here?
Yes, it is. Earlier patch had the interrupt toggling and Robin pointed out that
it is not really needed as counters are anyway stopped and explicitly disabling
may not solve the spurious interrupt case as well.
Ah apologies for not seeing that in earlier reviews.
Hmm, I didn't exactly mean "keep enabling it every time an event is
restarted and never disable it anywhere", though. I was thinking more
along the lines of enabling in event_add() and disabling in event_del()
(i.e. effectively tying it to allocation and deallocation of the counter).
Robin.
