[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5FC3163CFD30C246ABAA99954A238FA83929A9FE@lhreml524-mbs.china.huawei.com>
Date: Fri, 25 Jan 2019 09:22:23 +0000
From: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@...wei.com>
To: Robin Murphy <robin.murphy@....com>,
Andrew Murray <andrew.murray@....com>
CC: "lorenzo.pieralisi@....com" <lorenzo.pieralisi@....com>,
"mark.rutland@....com" <mark.rutland@....com>,
"vkilari@...eaurora.org" <vkilari@...eaurora.org>,
"neil.m.leeder@...il.com" <neil.m.leeder@...il.com>,
"jean-philippe.brucker@....com" <jean-philippe.brucker@....com>,
"pabba@...eaurora.org" <pabba@...eaurora.org>,
John Garry <john.garry@...wei.com>,
"will.deacon@....com" <will.deacon@....com>,
"rruigrok@...eaurora.org" <rruigrok@...eaurora.org>,
Linuxarm <linuxarm@...wei.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
"Guohanjun (Hanjun Guo)" <guohanjun@...wei.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>
Subject: RE: [PATCH v5 2/4] perf: add arm64 smmuv3 pmu driver
> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@....com]
> Sent: 24 January 2019 18:25
> To: Andrew Murray <andrew.murray@....com>; Shameerali Kolothum Thodi
> <shameerali.kolothum.thodi@...wei.com>
> Cc: lorenzo.pieralisi@....com; mark.rutland@....com;
> vkilari@...eaurora.org; neil.m.leeder@...il.com;
> jean-philippe.brucker@....com; pabba@...eaurora.org; John Garry
> <john.garry@...wei.com>; will.deacon@....com; rruigrok@...eaurora.org;
> Linuxarm <linuxarm@...wei.com>; linux-kernel@...r.kernel.org;
> linux-acpi@...r.kernel.org; Guohanjun (Hanjun Guo)
> <guohanjun@...wei.com>; linux-arm-kernel@...ts.infradead.org
> 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
Powered by blists - more mailing lists