From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751944AbdHQDI7 (ORCPT ); Wed, 16 Aug 2017 23:08:59 -0400 Received: from szxga05-in.huawei.com ([45.249.212.191]:3576 "EHLO szxga05-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751569AbdHQDI6 (ORCPT ); Wed, 16 Aug 2017 23:08:58 -0400 Subject: Re: [PATCH v4 2/6] perf: hisi: Add support for HiSilicon SoC uncore PMU driver To: Mark Rutland References: <1500984642-204676-1-git-send-email-zhangshaokun@hisilicon.com> <1500984642-204676-3-git-send-email-zhangshaokun@hisilicon.com> <20170815101627.GC6090@leverpostej> CC: , , , , From: Zhangshaokun Message-ID: <4f2b8398-7628-29ac-188b-35eacbff512b@hisilicon.com> Date: Thu, 17 Aug 2017 11:08:32 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <20170815101627.GC6090@leverpostej> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.74.221.148] X-CFilter-Loop: Reflected X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020203.599508C1.005D,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 5d0f1120fbdd5287c21c5d1169aabedc Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Mark, On 2017/8/15 18:16, Mark Rutland wrote: > Hi, > > On Tue, Jul 25, 2017 at 08:10:38PM +0800, Shaokun Zhang wrote: >> +/* Read Super CPU cluster and CPU cluster ID from MPIDR_EL1 */ >> +void hisi_read_sccl_and_ccl_id(u32 *sccl_id, u32 *ccl_id) >> +{ >> + u64 mpidr; >> + >> + mpidr = read_cpuid_mpidr(); >> + if (mpidr & MPIDR_MT_BITMASK) { >> + if (sccl_id) >> + *sccl_id = MPIDR_AFFINITY_LEVEL(mpidr, 3); >> + if (ccl_id) >> + *ccl_id = MPIDR_AFFINITY_LEVEL(mpidr, 2); >> + } else { >> + if (sccl_id) >> + *sccl_id = MPIDR_AFFINITY_LEVEL(mpidr, 2); >> + if (ccl_id) >> + *ccl_id = MPIDR_AFFINITY_LEVEL(mpidr, 1); >> + } >> +} > > How exactly are SCCLs organised w.r.t. MPIDRS? > For single-thread core, sccl_id is in MPIDR[aff2] and ccl_id is MPIDR[aff1]; For MT core, sccl_id is in MPIDR[aff3] and ccl_id in MPIDR[aff2]. I shall add comments here. > Is this guaranteed to be correct for future SoCs? > Sorry that it is uncertain. > It would be nicer if this were described explicitly by FW rather than > guessed at based on the MPIDR. > Whilst I agree, we assume this isn't going to happen now and the logic can be updated to support this if it we have more complex topology in the future. >> +static bool hisi_validate_event_group(struct perf_event *event) >> +{ >> + struct perf_event *sibling, *leader = event->group_leader; >> + struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu); >> + /* Include count for the event */ >> + int counters = 1; >> + >> + /* >> + * We must NOT create groups containing mixed PMUs, although >> + * software events are acceptable >> + */ >> + if (leader->pmu != event->pmu && !is_software_event(leader)) >> + return false; >> + >> + /* Increment counter for the leader */ >> + counters++; > > If this event is the leader, you account for it twice. > > I guess you get away with that assuming you have at least two counters, > but it's less than ideal. > We update this as per https://marc.info/?l=linux-arm-kernel&m=149096885106554&w=2 Any thoughts to avoid this issue? >> + >> + list_for_each_entry(sibling, &event->group_leader->sibling_list, >> + group_entry) { >> + if (is_software_event(sibling)) >> + continue; >> + if (sibling->pmu != event->pmu) >> + return false; >> + /* Increment counter for each sibling */ >> + counters++; >> + } >> + >> + /* The group can not count events more than the counters in the HW */ >> + return counters <= hisi_pmu->num_counters; >> +} > > [...] > >> +/* >> + * Set the counter to count the event that we're interested in, >> + * and enable counter and interrupt. >> + */ >> +static void hisi_uncore_pmu_enable_event(struct perf_event *event) >> +{ >> + struct hisi_pmu *hisi_pmu = to_hisi_pmu(event->pmu); >> + struct hw_perf_event *hwc = &event->hw; >> + >> + /* >> + * Write event code in event select registers(for DDRC PMU, >> + * event has been mapped to fixed-purpose counter, there is >> + * no need to write event type). >> + */ >> + if (hisi_pmu->ops->write_evtype) >> + hisi_pmu->ops->write_evtype(hisi_pmu, hwc->idx, >> + HISI_GET_EVENTID(event)); > > It looks like this is the only op which might be NULL. It would be > cleaner for the DDRC PMU code to provide an empty callback. > Ok. > [...] > >> +struct hisi_pmu *hisi_pmu_alloc(struct device *dev, u32 num_cntrs) >> +{ >> + struct hisi_pmu *hisi_pmu; >> + struct hisi_pmu_hwevents *pmu_events; >> + >> + hisi_pmu = devm_kzalloc(dev, sizeof(*hisi_pmu), GFP_KERNEL); >> + if (!hisi_pmu) >> + return ERR_PTR(-ENOMEM); >> + >> + pmu_events = &hisi_pmu->pmu_events; >> + pmu_events->hw_events = devm_kcalloc(dev, >> + num_cntrs, >> + sizeof(*pmu_events->hw_events), >> + GFP_KERNEL); >> + if (!pmu_events->hw_events) >> + return ERR_PTR(-ENOMEM); >> + >> + pmu_events->used_mask = devm_kcalloc(dev, >> + BITS_TO_LONGS(num_cntrs), >> + sizeof(*pmu_events->used_mask), >> + GFP_KERNEL); > > How big can num_counters be? > At the moment, the max num_counters is 0x10 for HHA PMU. > Assuming it's not too big, it would be nicer to embed these within the > hisi_pmu_hwevents. > Ok, shall refactor hisi_pmu_hwevents, will use the max num_counters and remove num_cntrs for hisi_pmu_alloc function. > [...] > >> + >> +/* Generic pmu struct for different pmu types */ >> +struct hisi_pmu { >> + const char *name; >> + struct pmu pmu; > > struct pmu has a name field. Why do we need another? > It is redundant and shall remove it. >> + union { >> + u32 ddrc_chn_id; >> + u32 l3c_tag_id; >> + u32 hha_uid; >> + }; > > This would be simpler as a `u32 id` rather than a union. > Ok. >> + int num_counters; >> + int num_events; > > Subsequent patches intialise num_events, but it is never used. Was it > supposed to be checked at event_init time? Or is it unnnecessary? > Yes, it is unnecessary and shall remove it. Thanks. Shaokun > Thanks, > Mark. > _______________________________________________ > linuxarm mailing list > linuxarm@huawei.com > http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm > > . >