Re: [PATCH v8 8/8] perf: ARM DynamIQ Shared Unit PMU support

[Suzuki K Poulose]

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