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

[Shaokun Zhang]

Hi Mark，

在 2021/2/3 21:43, 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.

Sure, it will be added in next version.

> 
>> +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.

For these filters, the default value is disabled and if the user needs
this feature, the driver will configure and enable this corresponding
control bit.

> 
>> +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.

Ok, will fix this in v3.

Thanks,
Shaokun

> 
> Thanks,
> Mark.
> .
>