Re: [PATCH v2 2/3] drivers/perf: hisi: Add support for HiSilicon UC PMU driver

Hi Jonathan,


On 2023/5/31 23:59, Jonathan Cameron wrote:

On Wed, 31 May 2023 18:46:24 +0800
Junhao He [off-list ref] wrote:

Hi Junhao,

A few small comments inline.

quoted

On HiSilicon Hip09 platform, there is a UC (unified cache) module
on each chip SCCL (Super CPU Cluster). UC is a cache that provides
coherence between NUMA and UMA domains. It is located between L2
and Memory System. While PA uncore PMU model is the same as other
Hip09 PMU modules and many PMU events are supported.

I don't follow what this sentence means.  Normally you'd have
While A, B is different..

Ok, will fix it.

quoted

Let's support
the PMU driver using the HiSilicon uncore PMU framework.

* rd_req_en : rd_req_en is the abbreviation of read request tracetag enable
and allows user to count only read operations.
details are listed in the hisi-pmu document.

Details are .. Also no need for the ine break
   and allows user to count only read operations. Details are listed
   in the hisi-pmu document at ....

Thanks, I'm going to replace this

quoted

* srcid_en & srcid: allows user to filter statistics that come from

Allows
for consistency with the uring_channel description that follows.

Yes, I will do that.

quoted

specific CPU/ICL by configuration source ID.

* uring_channel: Allows users to filter statistical information based on
the specified tx request uring channel.
uring_channel only supported events: [0x47 ~ 0x59].

Signed-off-by: Junhao He <redacted>

quoted

diff --git a/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c b/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c
new file mode 100644
index 000000000000..d27f28584fd7
--- /dev/null
+++ b/drivers/perf/hisilicon/hisi_uncore_uc_pmu.c

@@ -0,0 +1,577 @@

...

quoted

+static int hisi_uc_pmu_init_data(struct platform_device *pdev,
+				 struct hisi_pmu *uc_pmu)
+{
+	/*
+	 * Use SCCL (Super CPU Cluster) ID and CCL (CPU Cluster) ID to
+	 * identify the topology information of UC PMU devices in the chip.
+	 */
From patch description, I'd assume there is only one of these

per sccl so why do we care about the cluster level or the sub-id?
Perhaps that description is missleading?

They have some CCLs per SCCL and then 4 uc pmu per CCL.
The patch description is misleading and I will fix this in the next release.
Thanks.

quoted

+	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
+				     &uc_pmu->sccl_id)) {
+		dev_err(&pdev->dev, "Can not read uc sccl-id!\n");
+		return -EINVAL;
+	}
+
+	if (device_property_read_u32(&pdev->dev, "hisilicon,ccl-id",
+				     &uc_pmu->ccl_id)) {
+		dev_err(&pdev->dev, "Can not read uc ccl-id!\n");
+		return -EINVAL;
+	}
+
+	if (device_property_read_u32(&pdev->dev, "hisilicon,sub-id",
+				     &uc_pmu->sub_id)) {
+		dev_err(&pdev->dev, "Can not read sub-id!\n");
+		return -EINVAL;
+	}
+
+	uc_pmu->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(uc_pmu->base)) {
+		dev_err(&pdev->dev, "ioremap failed for uc_pmu resource\n");
+		return PTR_ERR(uc_pmu->base);
+	}
+
+	uc_pmu->identifier = readl(uc_pmu->base + HISI_UC_VERSION_REG);
+
+	return 0;
+}

quoted

+static struct platform_driver hisi_uc_pmu_driver = {
+	.driver = {
+		.name = "hisi_uc_pmu",
+		.acpi_match_table = hisi_uc_pmu_acpi_match,
+		/*
+		 * We have not worked out a safe bind/unbind process,
+		 * so this is not supported yet.

If you can reference more info on this that would be great.
Perhaps a thread talking about why?

forcefully unbinding during sampling will lead to a
kernel panic, because the perf upper-layer framework
call a NULL pointer in this situation.

Best regards,
Junhao.

quoted

+		 */
+		.suppress_bind_attrs = true,
+	},
+	.probe = hisi_uc_pmu_probe,
+};

.

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