[PATCH v2 7/8] drivers/perf: hisi: Add support for HiSilicon PA PMU driver

[Mark Rutland]

On Wed, Feb 03, 2021 at 03:51:07PM +0800, Shaokun Zhang wrote:
> On HiSilicon Hip09 platform, there is a PA (Protocol Adapter) module on
> each chip SICL (Super I/O Cluster) which incorporates three Hydra interface
> and facilitates the cache coherency between the dies on the chip. While PA
> uncore PMU model is the same as other Hip09 PMU modules and many PMU events
> are supported. Let's support the PMU driver using the HiSilicon uncore PMU
> framework.

> +HISI_PMU_EVENT_ATTR_EXTRACTOR(tgtid_cmd, config1, 10, 0);
> +HISI_PMU_EVENT_ATTR_EXTRACTOR(tgtid_msk, config1, 21, 11);
> +HISI_PMU_EVENT_ATTR_EXTRACTOR(srcid_cmd, config1, 32, 22);
> +HISI_PMU_EVENT_ATTR_EXTRACTOR(srcid_msk, config1, 43, 33);
> +HISI_PMU_EVENT_ATTR_EXTRACTOR(tracetag_en, config1, 44, 44);

As with the other patches, a brief introduction for these in the commit
message would be helpful.

> +static void hisi_pa_pmu_enable_filter(struct perf_event *event)
> +{
> +	if (event->attr.config1 != 0x0) {
> +		hisi_pa_pmu_enable_tracetag(event);
> +		hisi_pa_pmu_config_srcid(event);
> +		hisi_pa_pmu_config_tgtid(event);
> +	}
> +}
> +
> +static void hisi_pa_pmu_disable_filter(struct perf_event *event)
> +{
> +	if (event->attr.config1 != 0x0) {
> +		hisi_pa_pmu_clear_tgtid(event);
> +		hisi_pa_pmu_clear_srcid(event);
> +		hisi_pa_pmu_clear_tracetag(event);
> +	}
> +}

Does this get reset when the driver probes? I couldn't spot where we
ensured this was in a sane initial state.

> +static void hisi_pa_pmu_write_evtype(struct hisi_pmu *pa_pmu, int idx,
> +				     u32 type)
> +{
> +	u32 reg, reg_idx, shift, val;
> +
> +	/*
> +	 * Select the appropriate event select register(PA_EVENT_TYPE0/1).
> +	 * There are 2 event select registers for the 8 hardware counters.
> +	 * Event code is 8-bits and for the former 4 hardware counters,
> +	 * PA_EVENT_TYPE0 is chosen. For the latter 4 hardware counters,
> +	 * PA_EVENT_TYPE1 is chosen.
> +	 */
> +	reg = PA_EVENT_TYPE0 + rounddown(idx, 4);

The use of rounddown() here is confusing, as it relies on the number of
elements per register happening to be equal to the size of the register
in bytes. That works here since each element is a byte, but as it's not
the common case it sticks out.

Please divide the index by the number of elements per register, then
multiply that by the size of the register.

Thanks,
Mark.