From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752749AbdJTKRr (ORCPT ); Fri, 20 Oct 2017 06:17:47 -0400 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:38134 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751655AbdJTKRp (ORCPT ); Fri, 20 Oct 2017 06:17:45 -0400 Subject: Re: [PATCH v8 8/8] perf: ARM DynamIQ Shared Unit PMU support To: Mark Rutland Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, robh@kernel.org, will.deacon@arm.com, sudeep.holla@arm.com, frowand.list@gmail.com, devicetree@vger.kernel.org, Jonathan.Cameron@huawei.com, marc.zyngier@arm.com, peterz@infradead.org, mathieu.poirier@linaro.org, leo.yan@linaro.org References: <20171010103303.32436-1-suzuki.poulose@arm.com> <20171010103303.32436-9-suzuki.poulose@arm.com> <20171018092011.auey5hgpnjwkpsej@lakrids.cambridge.arm.com> From: Suzuki K Poulose Message-ID: Date: Fri, 20 Oct 2017 11:17:41 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20171018092011.auey5hgpnjwkpsej@lakrids.cambridge.arm.com> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 18/10/17 10:20, Mark Rutland wrote: > Hi Suzuki, > > This generally looks good. My comments below are mostly minor, modulo > the probing/hotplug bit at the end. > ... >> +static void dsu_pmu_probe_pmu(void *data) >> +{ >> + struct dsu_pmu *dsu_pmu = data; >> + u64 num_counters; >> + u32 cpmceid[2]; >> + >> + num_counters = (__dsu_pmu_read_pmcr() >> CLUSTERPMCR_N_SHIFT) & >> + CLUSTERPMCR_N_MASK; >> + /* We can only support upto 31 independent counters */ > > s/upto/up to/ > > Does the hardware spec allow for more than this? No, the "counter" mask registers are 32bit wide, with Bit 31 reserved for the Cycle counter. > >> + if (WARN_ON(num_counters > 31)) >> + num_counters = 31; >> + dsu_pmu->num_counters = num_counters; >> + if (!dsu_pmu->num_counters) >> + return; >> + cpmceid[0] = __dsu_pmu_read_pmceid(0); >> + cpmceid[1] = __dsu_pmu_read_pmceid(1); >> + bitmap_from_u32array(dsu_pmu->cpmceid_bitmap, >> + DSU_PMU_MAX_COMMON_EVENTS, >> + cpmceid, >> + ARRAY_SIZE(cpmceid)); >> +} > > [...] > >> + >> +static int dsu_pmu_device_probe(struct platform_device *pdev) >> +{ >> + int irq, rc, cpu; >> + struct dsu_pmu *dsu_pmu; >> + char *name; >> + static atomic_t pmu_idx = ATOMIC_INIT(-1); >> + >> + dsu_pmu = dsu_pmu_alloc(pdev); >> + if (IS_ERR(dsu_pmu)) >> + return PTR_ERR(dsu_pmu); >> + >> + rc = dsu_pmu_dt_get_cpus(pdev->dev.of_node, &dsu_pmu->associated_cpus); >> + if (rc) { >> + dev_warn(&pdev->dev, "Failed to parse the CPUs\n"); >> + return rc; >> + } >> + >> + rc = smp_call_function_any(&dsu_pmu->associated_cpus, >> + dsu_pmu_probe_pmu, >> + dsu_pmu, 1); > > Can we probe the relevant CPUs in the same was as the qcom l2 pmu, using > notifiers? > > That way we'd work better with maxcpus=. > > We have to do this cross-call in the arm_pmu driver because we need the > name at probe time, but that doesn't apply here. We should be able to > lazily initialize the set of events and number of counters. OK, I will make the necessary changes. Thanks a lot for the review. Cheers Suzuki