On 31/12/2020 06:19, 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.

Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: John Garry <redacted>
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Co-developed-by: Qi Liu <redacted>
Signed-off-by: Qi Liu <redacted>
Signed-off-by: Shaokun Zhang <redacted>

nit: please stop using inline when not needed

Apart from minor comments, below:
Reviewed-by: John Garry <redacted>

note: we do internal review, but tags not given - maybe we should in 
future...

nit: generally lower case used for hex, but not so important

+#define PA_EVENT_CTRL			0x1C04
+#define PA_TT_CTRL			0x1C08
+#define PA_TGTID_CTRL			0x1C14
+#define PA_SRCID_CTRL			0x1C18
+#define PA_INT_MASK			0x1C70
+#define PA_INT_STATUS			0x1C78
+#define PA_INT_CLEAR			0x1C7C
+#define PA_EVENT_TYPE0			0x1C80
+#define PA_PMU_VERSION			0x1CF0
+#define PA_EVENT_CNT0_L			0x1D00
+

...

+
+static int hisi_pa_pmu_init_data(struct platform_device *pdev,
+				   struct hisi_pmu *pa_pmu)
+{
+	/*
+	 * Use the SCCL_ID and the index ID to identify the PA PMU,
+	 * while SCCL_ID is the nearst SCCL_ID from this SICL.
+	 */
+	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
+				     &pa_pmu->sccl_id)) {

hmmm... I do wonder if we should make it clearer in the driver that this 
is the SICL. I know I mentioned this before--sorry

+		dev_err(&pdev->dev, "Cannot read sccl-id!\n");
+		return -EINVAL;
+	}
+
+	if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
+				     &pa_pmu->index_id)) {
+		dev_err(&pdev->dev, "Cannot read idx-id!\n");
+		return -EINVAL;
+	}
+
+	pa_pmu->ccl_id = -1;
+
+	pa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(pa_pmu->base)) {
+		dev_err(&pdev->dev, "ioremap failed for pa_pmu resource.\n");
+		return PTR_ERR(pa_pmu->base);
+	}
+
+	pa_pmu->identifier = readl(pa_pmu->base + PA_PMU_VERSION);
+
+	return 0;
+}
+

...

+static int hisi_pa_pmu_probe(struct platform_device *pdev)
+{
+	struct hisi_pmu *pa_pmu;
+	char *name;
+	int ret;
+
+	pa_pmu = devm_kzalloc(&pdev->dev, sizeof(*pa_pmu), GFP_KERNEL);
+	if (!pa_pmu)
+		return -ENOMEM;
+
+	ret = hisi_pa_pmu_dev_probe(pdev, pa_pmu);
+	if (ret)
+		return ret;
+	/*
+	 * PA is attached in SICL and the CPU core is chosen to manage this
+	 * PMU which is the nearest SCCL,

do we really need to do this? Can any CPU in that chip do it?

while its SCCL_ID is greater than
+	 * one with the SICL_ID.
+	 */
+	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%u_pa%u",
+			      pa_pmu->sccl_id - 1, pa_pmu->index_id);
+	if (!name) {
+		dev_err(&pdev->dev, "failed to allocate name for PMU\n");
+		return -ENOMEM;
+	}
+
+	ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_PA_ONLINE,
+				       &pa_pmu->node);
+	if (ret) {
+		dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
+		return ret;
+	}
+
+	pa_pmu->pmu = (struct pmu) {
+		.module		= THIS_MODULE,
+		.task_ctx_nr	= perf_invalid_context,
+		.event_init	= hisi_uncore_pmu_event_init,
+		.pmu_enable	= hisi_uncore_pmu_enable,
+		.pmu_disable	= hisi_uncore_pmu_disable,
+		.add		= hisi_uncore_pmu_add,
+		.del		= hisi_uncore_pmu_del,
+		.start		= hisi_uncore_pmu_start,
+		.stop		= hisi_uncore_pmu_stop,
+		.read		= hisi_uncore_pmu_read,
+		.attr_groups    = pa_pmu->pmu_events.attr_groups,
+		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
+	};
+
+	ret = perf_pmu_register(&pa_pmu->pmu, name, -1);
+	if (ret) {
+		dev_err(pa_pmu->dev, "PMU register failed, ret = %d\n", ret);
+		cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_HISI_PA_ONLINE,
+					    &pa_pmu->node);
+		irq_set_affinity_hint(pa_pmu->irq, NULL);
+	}
+
+	platform_set_drvdata(pdev, pa_pmu);

as before

+	return ret;
+}
+
+static int hisi_pa_pmu_remove(struct platform_device *pdev)
+{
+	struct hisi_pmu *pa_pmu = platform_get_drvdata(pdev);
+
+	perf_pmu_unregister(&pa_pmu->pmu);
+	cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_HISI_PA_ONLINE,
+					    &pa_pmu->node);
+	irq_set_affinity_hint(pa_pmu->irq, NULL);
+
+	return 0;
+}
+
+static struct platform_driver hisi_pa_pmu_driver = {
+	.driver = {
+		.name = "hisi_pa_pmu",
+		.acpi_match_table = hisi_pa_pmu_acpi_match,

some small comments on earlier patch reviews apply here

+	},
+	.probe = hisi_pa_pmu_probe,
+	.remove = hisi_pa_pmu_remove,
+};
+
+static int __init hisi_pa_pmu_module_init(void)
+{
+	int ret;
+
+	ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_HISI_PA_ONLINE,
+				      "AP_PERF_ARM_HISI_PA_ONLINE",
+				      hisi_uncore_pmu_online_cpu,
+				      hisi_uncore_pmu_offline_cpu);
+	if (ret) {
+		pr_err("PA PMU: cpuhp state setup failed, ret = %d\n", ret);
+		return ret;
+	}
+
+	ret = platform_driver_register(&hisi_pa_pmu_driver);
+	if (ret)
+		cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_PA_ONLINE);
+
+	return ret;
+}
+module_init(hisi_pa_pmu_module_init);
+
+static void __exit hisi_pa_pmu_module_exit(void)
+{
+	platform_driver_unregister(&hisi_pa_pmu_driver);
+	cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_PA_ONLINE);
+}
+module_exit(hisi_pa_pmu_module_exit);
+
+MODULE_DESCRIPTION("HiSilicon PA uncore PMU driver");

nit maybe have "PA (Protocol Adapter)"

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