> -----Original Message----- > From: Robin Murphy [mailto:robin.murphy@xxxxxxx] > Sent: 24 January 2019 18:25 > To: Andrew Murray <andrew.murray@xxxxxxx>; Shameerali Kolothum Thodi > <shameerali.kolothum.thodi@xxxxxxxxxx> > Cc: lorenzo.pieralisi@xxxxxxx; mark.rutland@xxxxxxx; > vkilari@xxxxxxxxxxxxxx; neil.m.leeder@xxxxxxxxx; > jean-philippe.brucker@xxxxxxx; pabba@xxxxxxxxxxxxxx; John Garry > <john.garry@xxxxxxxxxx>; will.deacon@xxxxxxx; rruigrok@xxxxxxxxxxxxxx; > Linuxarm <linuxarm@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; > linux-acpi@xxxxxxxxxxxxxxx; Guohanjun (Hanjun Guo) > <guohanjun@xxxxxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH v5 2/4] perf: add arm64 smmuv3 pmu driver > > 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). > Right. I missed the point that it was not disabled at all!. I will rearrange it to _add/_del path. Thanks for all the comments. I am planning to send out a v6 soon. Between, did you get a chance to take a look at patch #4 (HiSilicon erratum one) ? Appreciate if you could take a look and let me know before v6. Thanks, Shameer
