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

Hi Mark，

在 2021/2/3 21:43, Mark Rutland 写道:

On Wed, Feb 03, 2021 at 03:51:07PM +0800, Shaokun Zhang wrote:

quoted

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.

quoted

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

quoted

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

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel