[PATCH v2 0/2] Add Support for HiSilicon CPA PMU

[PATCH v2 0/2] Add Support for HiSilicon CPA PMU

[Qi Liu]

Add Support for HiSilicon CPA PMU, including PMU driver and json
file in perf tool.

Change since v1:
- Address the comments from John.
- Link: https://lore.kernel.org/linux-arm-kernel/20220214114228.40859-1-liuqi115@huawei.com/

Qi Liu (2):
  drivers/perf: hisi: Add Support for CPA PMU
  perf jevents: Add support for HiSilicon CPA PMU aliasing

 drivers/perf/hisilicon/Makefile               |   2 +-
 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c  | 401 ++++++++++++++++++
 include/linux/cpuhotplug.h                    |   1 +
 .../arm64/hisilicon/hip09/sys/uncore-cpa.json |  81 ++++
 tools/perf/pmu-events/jevents.c               |   1 +
 5 files changed, 485 insertions(+), 1 deletion(-)
 create mode 100644 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
 create mode 100644 tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json

-- 
2.24.0

[PATCH v2 0/2] Add Support for HiSilicon CPA PMU

[Qi Liu]

Add Support for HiSilicon CPA PMU, including PMU driver and json
file in perf tool.

Change since v1:
- Address the comments from John.
- Link: https://lore.kernel.org/linux-arm-kernel/20220214114228.40859-1-liuqi115@huawei.com/

Qi Liu (2):
  drivers/perf: hisi: Add Support for CPA PMU
  perf jevents: Add support for HiSilicon CPA PMU aliasing

 drivers/perf/hisilicon/Makefile               |   2 +-
 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c  | 401 ++++++++++++++++++
 include/linux/cpuhotplug.h                    |   1 +
 .../arm64/hisilicon/hip09/sys/uncore-cpa.json |  81 ++++
 tools/perf/pmu-events/jevents.c               |   1 +
 5 files changed, 485 insertions(+), 1 deletion(-)
 create mode 100644 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
 create mode 100644 tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json

-- 
2.24.0


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

[PATCH v2 1/2] drivers/perf: hisi: Add Support for CPA PMU

[Qi Liu]

On HiSilicon Hip09 platform, there is a CPA(Coherency Protocol Agent) on
each SICL (Super I/O Cluster) which implements packet format translation,
route parsing and traffic statistic.

CPA PMU has separate PMU registers which the driver can program freely and
interrupt is supported to handle counter overflow. Let's support its driver
under the framework of HiSilicon uncore PMU driver.

Signed-off-by: Qi Liu <liuqi115@huawei.com>
---
 drivers/perf/hisilicon/Makefile              |   2 +-
 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c | 401 +++++++++++++++++++
 include/linux/cpuhotplug.h                   |   1 +
 3 files changed, 403 insertions(+), 1 deletion(-)
 create mode 100644 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c

diff --git a/drivers/perf/hisilicon/Makefile b/drivers/perf/hisilicon/Makefile
index 506ed39e3266..6be83517acaa 100644
--- a/drivers/perf/hisilicon/Makefile
+++ b/drivers/perf/hisilicon/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o hisi_uncore_l3c_pmu.o \
 			  hisi_uncore_hha_pmu.o hisi_uncore_ddrc_pmu.o hisi_uncore_sllc_pmu.o \
-			  hisi_uncore_pa_pmu.o
+			  hisi_uncore_pa_pmu.o hisi_uncore_cpa_pmu.o
 
 obj-$(CONFIG_HISI_PCIE_PMU) += hisi_pcie_pmu.o
diff --git a/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
new file mode 100644
index 000000000000..00573719f148
--- /dev/null
+++ b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
@@ -0,0 +1,401 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * HiSilicon SoC CPA(Coherency Protocol Agent) uncore Hardware event counters support
+ *
+ * Copyright (C) 2022 HiSilicon Limited
+ * Author: Qi Liu <liuqi115@huawei.com>
+ *
+ * This code is based on the uncore PMUs like arm-cci and arm-ccn.
+ */
+
+#include <linux/acpi.h>
+#include <linux/bug.h>
+#include <linux/cpuhotplug.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/list.h>
+#include <linux/smp.h>
+
+#include "hisi_uncore_pmu.h"
+
+/* CPA register definition */
+#define CPA_PERF_CTRL		0x1c00
+#define CPA_EVENT_CTRL		0x1c04
+#define CPA_INT_MASK		0x1c70
+#define CPA_INT_STATUS		0x1c78
+#define CPA_INT_CLEAR		0x1c7c
+#define CPA_EVENT_TYPE0		0x1c80
+#define CPA_VERSION		0x1cf0
+#define CPA_CNT0_LOWER		0x1d00
+#define CPA_CFG_REG		0x0534
+
+/* CPA operation command */
+#define CPA_PERF_CTRL_EN	BIT_ULL(0)
+#define CPA_EVTYPE_MASK		0xffUL
+#define CPA_PM_CTRL		BIT_ULL(9)
+
+/* CPA has 8-counters */
+#define CPA_NR_COUNTERS		0x8
+#define CPA_COUNTER_BITS	64
+#define CPA_NR_EVENTS		0xff
+#define CPA_REG_OFFSET		0x8
+
+static u32 hisi_cpa_pmu_get_counter_offset(int idx)
+{
+	return (CPA_CNT0_LOWER + idx * CPA_REG_OFFSET);
+}
+
+static u64 hisi_cpa_pmu_read_counter(struct hisi_pmu *cpa_pmu,
+				     struct hw_perf_event *hwc)
+{
+	return readq(cpa_pmu->base + hisi_cpa_pmu_get_counter_offset(hwc->idx));
+}
+
+static void hisi_cpa_pmu_write_counter(struct hisi_pmu *cpa_pmu,
+				       struct hw_perf_event *hwc, u64 val)
+{
+	writeq(val, cpa_pmu->base + hisi_cpa_pmu_get_counter_offset(hwc->idx));
+}
+
+static void hisi_cpa_pmu_write_evtype(struct hisi_pmu *cpa_pmu, int idx, u32 type)
+{
+	u32 reg, reg_idx, shift, val;
+
+	/*
+	 * Select the appropriate event select register(CPA_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,
+	 * CPA_EVENT_TYPE0 is chosen. For the latter 4 hardware counters,
+	 * CPA_EVENT_TYPE1 is chosen.
+	 */
+	reg = CPA_EVENT_TYPE0 + (idx / 4) * 4;
+	reg_idx = idx % 4;
+	shift = CPA_REG_OFFSET * reg_idx;
+
+	/* Write event code to CPA_EVENT_TYPEx Register */
+	val = readl(cpa_pmu->base + reg);
+	val &= ~(CPA_EVTYPE_MASK << shift);
+	val |= type << shift;
+	writel(val, cpa_pmu->base + reg);
+}
+
+static void hisi_cpa_pmu_start_counters(struct hisi_pmu *cpa_pmu)
+{
+	u32 val;
+
+	val = readl(cpa_pmu->base + CPA_PERF_CTRL);
+	val |= CPA_PERF_CTRL_EN;
+	writel(val, cpa_pmu->base + CPA_PERF_CTRL);
+}
+
+static void hisi_cpa_pmu_stop_counters(struct hisi_pmu *cpa_pmu)
+{
+	u32 val;
+
+	val = readl(cpa_pmu->base + CPA_PERF_CTRL);
+	val &= ~(CPA_PERF_CTRL_EN);
+	writel(val, cpa_pmu->base + CPA_PERF_CTRL);
+}
+
+static void hisi_cpa_pmu_disable_pm(struct hisi_pmu *cpa_pmu)
+{
+	u32 val;
+
+	val = readl(cpa_pmu->base + CPA_CFG_REG);
+	val |= CPA_PM_CTRL;
+	writel(val, cpa_pmu->base + CPA_CFG_REG);
+}
+
+static void hisi_cpa_pmu_enable_pm(struct hisi_pmu *cpa_pmu)
+{
+	u32 val;
+
+	val = readl(cpa_pmu->base + CPA_CFG_REG);
+	val &= ~CPA_PM_CTRL;
+	writel(val, cpa_pmu->base + CPA_CFG_REG);
+}
+
+static void hisi_cpa_pmu_enable_counter(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	/* Enable counter index in CPA_EVENT_CTRL register */
+	val = readl(cpa_pmu->base + CPA_EVENT_CTRL);
+	val |= 1 << hwc->idx;
+	writel(val, cpa_pmu->base + CPA_EVENT_CTRL);
+
+	/* Power management should be disabled before counting. */
+	hisi_cpa_pmu_disable_pm(cpa_pmu);
+}
+
+static void hisi_cpa_pmu_disable_counter(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	hisi_cpa_pmu_enable_pm(cpa_pmu);
+
+	/* Clear counter index in CPA_EVENT_CTRL register */
+	val = readl(cpa_pmu->base + CPA_EVENT_CTRL);
+	val &= ~(1UL << hwc->idx);
+	writel(val, cpa_pmu->base + CPA_EVENT_CTRL);
+}
+
+static void hisi_cpa_pmu_enable_counter_int(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	/* Write 0 to enable interrupt */
+	val = readl(cpa_pmu->base + CPA_INT_MASK);
+	val &= ~(1UL << hwc->idx);
+	writel(val, cpa_pmu->base + CPA_INT_MASK);
+}
+
+static void hisi_cpa_pmu_disable_counter_int(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	/* Write 1 to mask interrupt */
+	val = readl(cpa_pmu->base + CPA_INT_MASK);
+	val |= 1 << hwc->idx;
+	writel(val, cpa_pmu->base + CPA_INT_MASK);
+}
+
+static u32 hisi_cpa_pmu_get_int_status(struct hisi_pmu *cpa_pmu)
+{
+	return readl(cpa_pmu->base + CPA_INT_STATUS);
+}
+
+static void hisi_cpa_pmu_clear_int_status(struct hisi_pmu *cpa_pmu, int idx)
+{
+	writel(1 << idx, cpa_pmu->base + CPA_INT_CLEAR);
+}
+
+static const struct acpi_device_id hisi_cpa_pmu_acpi_match[] = {
+	{ "HISI0281", },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, hisi_cpa_pmu_acpi_match);
+
+static int hisi_cpa_pmu_init_data(struct platform_device *pdev,
+				  struct hisi_pmu *cpa_pmu)
+{
+	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
+				     &cpa_pmu->sccl_id)) {
+		dev_err(&pdev->dev, "Can not read cpa_pmu sccl-id\n");
+		return -EINVAL;
+	}
+
+	cpa_pmu->ccl_id = -1;
+
+	if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
+				     &cpa_pmu->index_id)) {
+		dev_err(&pdev->dev, "Cannot read idx-id\n");
+		return -EINVAL;
+	}
+
+	cpa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(cpa_pmu->base))
+		return PTR_ERR(cpa_pmu->base);
+
+	cpa_pmu->identifier = readl(cpa_pmu->base + CPA_VERSION);
+
+	return 0;
+}
+
+static struct attribute *hisi_cpa_pmu_format_attr[] = {
+	HISI_PMU_FORMAT_ATTR(event, "config:0-15"),
+	NULL
+};
+
+static const struct attribute_group hisi_cpa_pmu_format_group = {
+	.name = "format",
+	.attrs = hisi_cpa_pmu_format_attr,
+};
+
+static struct attribute *hisi_cpa_pmu_events_attr[] = {
+	HISI_PMU_EVENT_ATTR(cpa_cycles,		0x00),
+	HISI_PMU_EVENT_ATTR(cpa_p1_wr_dat,	0x61),
+	HISI_PMU_EVENT_ATTR(cpa_p1_rd_dat,	0x62),
+	HISI_PMU_EVENT_ATTR(cpa_p0_wr_dat,	0xE1),
+	HISI_PMU_EVENT_ATTR(cpa_p0_rd_dat,	0xE2),
+	NULL
+};
+
+static const struct attribute_group hisi_cpa_pmu_events_group = {
+	.name = "events",
+	.attrs = hisi_cpa_pmu_events_attr,
+};
+
+static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
+
+static struct attribute *hisi_cpa_pmu_cpumask_attrs[] = {
+	&dev_attr_cpumask.attr,
+	NULL
+};
+
+static const struct attribute_group hisi_cpa_pmu_cpumask_attr_group = {
+	.attrs = hisi_cpa_pmu_cpumask_attrs,
+};
+
+static struct device_attribute hisi_cpa_pmu_identifier_attr =
+	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
+
+static struct attribute *hisi_cpa_pmu_identifier_attrs[] = {
+	&hisi_cpa_pmu_identifier_attr.attr,
+	NULL
+};
+
+static const struct attribute_group hisi_cpa_pmu_identifier_group = {
+	.attrs = hisi_cpa_pmu_identifier_attrs,
+};
+
+static const struct attribute_group *hisi_cpa_pmu_attr_groups[] = {
+	&hisi_cpa_pmu_format_group,
+	&hisi_cpa_pmu_events_group,
+	&hisi_cpa_pmu_cpumask_attr_group,
+	&hisi_cpa_pmu_identifier_group,
+	NULL
+};
+
+static const struct hisi_uncore_ops hisi_uncore_cpa_pmu_ops = {
+	.write_evtype           = hisi_cpa_pmu_write_evtype,
+	.get_event_idx		= hisi_uncore_pmu_get_event_idx,
+	.start_counters		= hisi_cpa_pmu_start_counters,
+	.stop_counters		= hisi_cpa_pmu_stop_counters,
+	.enable_counter		= hisi_cpa_pmu_enable_counter,
+	.disable_counter	= hisi_cpa_pmu_disable_counter,
+	.enable_counter_int	= hisi_cpa_pmu_enable_counter_int,
+	.disable_counter_int	= hisi_cpa_pmu_disable_counter_int,
+	.write_counter		= hisi_cpa_pmu_write_counter,
+	.read_counter		= hisi_cpa_pmu_read_counter,
+	.get_int_status		= hisi_cpa_pmu_get_int_status,
+	.clear_int_status	= hisi_cpa_pmu_clear_int_status,
+};
+
+static int hisi_cpa_pmu_dev_probe(struct platform_device *pdev,
+				  struct hisi_pmu *cpa_pmu)
+{
+	int ret;
+
+	ret = hisi_cpa_pmu_init_data(pdev, cpa_pmu);
+	if (ret)
+		return ret;
+
+	ret = hisi_uncore_pmu_init_irq(cpa_pmu, pdev);
+	if (ret)
+		return ret;
+
+	cpa_pmu->counter_bits = CPA_COUNTER_BITS;
+	cpa_pmu->check_event = CPA_NR_EVENTS;
+	cpa_pmu->pmu_events.attr_groups = hisi_cpa_pmu_attr_groups;
+	cpa_pmu->ops = &hisi_uncore_cpa_pmu_ops;
+	cpa_pmu->num_counters = CPA_NR_COUNTERS;
+	cpa_pmu->dev = &pdev->dev;
+	cpa_pmu->on_cpu = -1;
+
+	return 0;
+}
+
+static int hisi_cpa_pmu_probe(struct platform_device *pdev)
+{
+	struct hisi_pmu *cpa_pmu;
+	char *name;
+	int ret;
+
+	cpa_pmu = devm_kzalloc(&pdev->dev, sizeof(*cpa_pmu), GFP_KERNEL);
+	if (!cpa_pmu)
+		return -ENOMEM;
+
+	ret = hisi_cpa_pmu_dev_probe(pdev, cpa_pmu);
+	if (ret)
+		return ret;
+
+	ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
+				       &cpa_pmu->node);
+	if (ret) {
+		dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
+		return ret;
+	}
+
+	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%d_cpa%u", cpa_pmu->sccl_id - 1,
+			      cpa_pmu->index_id);
+
+	cpa_pmu->pmu = (struct pmu) {
+		.name		= name,
+		.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	= cpa_pmu->pmu_events.attr_groups,
+		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
+	};
+
+	ret = perf_pmu_register(&cpa_pmu->pmu, name, -1);
+	if (ret) {
+		dev_err(cpa_pmu->dev, "CPA PMU register failed\n");
+		cpuhp_state_remove_instance_nocalls(
+			CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE, &cpa_pmu->node);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, cpa_pmu);
+	return ret;
+}
+
+static int hisi_cpa_pmu_remove(struct platform_device *pdev)
+{
+	struct hisi_pmu *cpa_pmu = platform_get_drvdata(pdev);
+
+	perf_pmu_unregister(&cpa_pmu->pmu);
+	cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
+					    &cpa_pmu->node);
+	return 0;
+}
+
+static struct platform_driver hisi_cpa_pmu_driver = {
+	.driver = {
+		.name = "hisi_cpa_pmu",
+		.acpi_match_table = ACPI_PTR(hisi_cpa_pmu_acpi_match),
+		.suppress_bind_attrs = true,
+	},
+	.probe = hisi_cpa_pmu_probe,
+	.remove = hisi_cpa_pmu_remove,
+};
+
+static int __init hisi_cpa_pmu_module_init(void)
+{
+	int ret;
+
+	ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
+				      "AP_PERF_ARM_HISI_CPA_ONLINE",
+				      hisi_uncore_pmu_online_cpu,
+				      hisi_uncore_pmu_offline_cpu);
+	if (ret) {
+		pr_err("CPA PMU: setup hotplug: %d\n", ret);
+		return ret;
+	}
+
+	ret = platform_driver_register(&hisi_cpa_pmu_driver);
+	if (ret)
+		cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE);
+
+	return ret;
+}
+module_init(hisi_cpa_pmu_module_init);
+
+static void __exit hisi_cpa_pmu_module_exit(void)
+{
+	platform_driver_unregister(&hisi_cpa_pmu_driver);
+	cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE);
+}
+module_exit(hisi_cpa_pmu_module_exit);
+
+MODULE_DESCRIPTION("HiSilicon SoC CPA uncore PMU driver");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Qi Liu <liuqi115@huawei.com>");
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 411a428ace4d..d55063719904 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -220,6 +220,7 @@ enum cpuhp_state {
 	CPUHP_AP_PERF_S390_SF_ONLINE,
 	CPUHP_AP_PERF_ARM_CCI_ONLINE,
 	CPUHP_AP_PERF_ARM_CCN_ONLINE,
+	CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
 	CPUHP_AP_PERF_ARM_HISI_DDRC_ONLINE,
 	CPUHP_AP_PERF_ARM_HISI_HHA_ONLINE,
 	CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
-- 
2.24.0

[PATCH v2 1/2] drivers/perf: hisi: Add Support for CPA PMU

[Qi Liu]

On HiSilicon Hip09 platform, there is a CPA(Coherency Protocol Agent) on
each SICL (Super I/O Cluster) which implements packet format translation,
route parsing and traffic statistic.

CPA PMU has separate PMU registers which the driver can program freely and
interrupt is supported to handle counter overflow. Let's support its driver
under the framework of HiSilicon uncore PMU driver.

Signed-off-by: Qi Liu <liuqi115@huawei.com>
---
 drivers/perf/hisilicon/Makefile              |   2 +-
 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c | 401 +++++++++++++++++++
 include/linux/cpuhotplug.h                   |   1 +
 3 files changed, 403 insertions(+), 1 deletion(-)
 create mode 100644 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c

diff --git a/drivers/perf/hisilicon/Makefile b/drivers/perf/hisilicon/Makefile
index 506ed39e3266..6be83517acaa 100644
--- a/drivers/perf/hisilicon/Makefile
+++ b/drivers/perf/hisilicon/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o hisi_uncore_l3c_pmu.o \
 			  hisi_uncore_hha_pmu.o hisi_uncore_ddrc_pmu.o hisi_uncore_sllc_pmu.o \
-			  hisi_uncore_pa_pmu.o
+			  hisi_uncore_pa_pmu.o hisi_uncore_cpa_pmu.o
 
 obj-$(CONFIG_HISI_PCIE_PMU) += hisi_pcie_pmu.o
diff --git a/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
new file mode 100644
index 000000000000..00573719f148
--- /dev/null
+++ b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
@@ -0,0 +1,401 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * HiSilicon SoC CPA(Coherency Protocol Agent) uncore Hardware event counters support
+ *
+ * Copyright (C) 2022 HiSilicon Limited
+ * Author: Qi Liu <liuqi115@huawei.com>
+ *
+ * This code is based on the uncore PMUs like arm-cci and arm-ccn.
+ */
+
+#include <linux/acpi.h>
+#include <linux/bug.h>
+#include <linux/cpuhotplug.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/list.h>
+#include <linux/smp.h>
+
+#include "hisi_uncore_pmu.h"
+
+/* CPA register definition */
+#define CPA_PERF_CTRL		0x1c00
+#define CPA_EVENT_CTRL		0x1c04
+#define CPA_INT_MASK		0x1c70
+#define CPA_INT_STATUS		0x1c78
+#define CPA_INT_CLEAR		0x1c7c
+#define CPA_EVENT_TYPE0		0x1c80
+#define CPA_VERSION		0x1cf0
+#define CPA_CNT0_LOWER		0x1d00
+#define CPA_CFG_REG		0x0534
+
+/* CPA operation command */
+#define CPA_PERF_CTRL_EN	BIT_ULL(0)
+#define CPA_EVTYPE_MASK		0xffUL
+#define CPA_PM_CTRL		BIT_ULL(9)
+
+/* CPA has 8-counters */
+#define CPA_NR_COUNTERS		0x8
+#define CPA_COUNTER_BITS	64
+#define CPA_NR_EVENTS		0xff
+#define CPA_REG_OFFSET		0x8
+
+static u32 hisi_cpa_pmu_get_counter_offset(int idx)
+{
+	return (CPA_CNT0_LOWER + idx * CPA_REG_OFFSET);
+}
+
+static u64 hisi_cpa_pmu_read_counter(struct hisi_pmu *cpa_pmu,
+				     struct hw_perf_event *hwc)
+{
+	return readq(cpa_pmu->base + hisi_cpa_pmu_get_counter_offset(hwc->idx));
+}
+
+static void hisi_cpa_pmu_write_counter(struct hisi_pmu *cpa_pmu,
+				       struct hw_perf_event *hwc, u64 val)
+{
+	writeq(val, cpa_pmu->base + hisi_cpa_pmu_get_counter_offset(hwc->idx));
+}
+
+static void hisi_cpa_pmu_write_evtype(struct hisi_pmu *cpa_pmu, int idx, u32 type)
+{
+	u32 reg, reg_idx, shift, val;
+
+	/*
+	 * Select the appropriate event select register(CPA_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,
+	 * CPA_EVENT_TYPE0 is chosen. For the latter 4 hardware counters,
+	 * CPA_EVENT_TYPE1 is chosen.
+	 */
+	reg = CPA_EVENT_TYPE0 + (idx / 4) * 4;
+	reg_idx = idx % 4;
+	shift = CPA_REG_OFFSET * reg_idx;
+
+	/* Write event code to CPA_EVENT_TYPEx Register */
+	val = readl(cpa_pmu->base + reg);
+	val &= ~(CPA_EVTYPE_MASK << shift);
+	val |= type << shift;
+	writel(val, cpa_pmu->base + reg);
+}
+
+static void hisi_cpa_pmu_start_counters(struct hisi_pmu *cpa_pmu)
+{
+	u32 val;
+
+	val = readl(cpa_pmu->base + CPA_PERF_CTRL);
+	val |= CPA_PERF_CTRL_EN;
+	writel(val, cpa_pmu->base + CPA_PERF_CTRL);
+}
+
+static void hisi_cpa_pmu_stop_counters(struct hisi_pmu *cpa_pmu)
+{
+	u32 val;
+
+	val = readl(cpa_pmu->base + CPA_PERF_CTRL);
+	val &= ~(CPA_PERF_CTRL_EN);
+	writel(val, cpa_pmu->base + CPA_PERF_CTRL);
+}
+
+static void hisi_cpa_pmu_disable_pm(struct hisi_pmu *cpa_pmu)
+{
+	u32 val;
+
+	val = readl(cpa_pmu->base + CPA_CFG_REG);
+	val |= CPA_PM_CTRL;
+	writel(val, cpa_pmu->base + CPA_CFG_REG);
+}
+
+static void hisi_cpa_pmu_enable_pm(struct hisi_pmu *cpa_pmu)
+{
+	u32 val;
+
+	val = readl(cpa_pmu->base + CPA_CFG_REG);
+	val &= ~CPA_PM_CTRL;
+	writel(val, cpa_pmu->base + CPA_CFG_REG);
+}
+
+static void hisi_cpa_pmu_enable_counter(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	/* Enable counter index in CPA_EVENT_CTRL register */
+	val = readl(cpa_pmu->base + CPA_EVENT_CTRL);
+	val |= 1 << hwc->idx;
+	writel(val, cpa_pmu->base + CPA_EVENT_CTRL);
+
+	/* Power management should be disabled before counting. */
+	hisi_cpa_pmu_disable_pm(cpa_pmu);
+}
+
+static void hisi_cpa_pmu_disable_counter(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	hisi_cpa_pmu_enable_pm(cpa_pmu);
+
+	/* Clear counter index in CPA_EVENT_CTRL register */
+	val = readl(cpa_pmu->base + CPA_EVENT_CTRL);
+	val &= ~(1UL << hwc->idx);
+	writel(val, cpa_pmu->base + CPA_EVENT_CTRL);
+}
+
+static void hisi_cpa_pmu_enable_counter_int(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	/* Write 0 to enable interrupt */
+	val = readl(cpa_pmu->base + CPA_INT_MASK);
+	val &= ~(1UL << hwc->idx);
+	writel(val, cpa_pmu->base + CPA_INT_MASK);
+}
+
+static void hisi_cpa_pmu_disable_counter_int(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	/* Write 1 to mask interrupt */
+	val = readl(cpa_pmu->base + CPA_INT_MASK);
+	val |= 1 << hwc->idx;
+	writel(val, cpa_pmu->base + CPA_INT_MASK);
+}
+
+static u32 hisi_cpa_pmu_get_int_status(struct hisi_pmu *cpa_pmu)
+{
+	return readl(cpa_pmu->base + CPA_INT_STATUS);
+}
+
+static void hisi_cpa_pmu_clear_int_status(struct hisi_pmu *cpa_pmu, int idx)
+{
+	writel(1 << idx, cpa_pmu->base + CPA_INT_CLEAR);
+}
+
+static const struct acpi_device_id hisi_cpa_pmu_acpi_match[] = {
+	{ "HISI0281", },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, hisi_cpa_pmu_acpi_match);
+
+static int hisi_cpa_pmu_init_data(struct platform_device *pdev,
+				  struct hisi_pmu *cpa_pmu)
+{
+	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
+				     &cpa_pmu->sccl_id)) {
+		dev_err(&pdev->dev, "Can not read cpa_pmu sccl-id\n");
+		return -EINVAL;
+	}
+
+	cpa_pmu->ccl_id = -1;
+
+	if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
+				     &cpa_pmu->index_id)) {
+		dev_err(&pdev->dev, "Cannot read idx-id\n");
+		return -EINVAL;
+	}
+
+	cpa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(cpa_pmu->base))
+		return PTR_ERR(cpa_pmu->base);
+
+	cpa_pmu->identifier = readl(cpa_pmu->base + CPA_VERSION);
+
+	return 0;
+}
+
+static struct attribute *hisi_cpa_pmu_format_attr[] = {
+	HISI_PMU_FORMAT_ATTR(event, "config:0-15"),
+	NULL
+};
+
+static const struct attribute_group hisi_cpa_pmu_format_group = {
+	.name = "format",
+	.attrs = hisi_cpa_pmu_format_attr,
+};
+
+static struct attribute *hisi_cpa_pmu_events_attr[] = {
+	HISI_PMU_EVENT_ATTR(cpa_cycles,		0x00),
+	HISI_PMU_EVENT_ATTR(cpa_p1_wr_dat,	0x61),
+	HISI_PMU_EVENT_ATTR(cpa_p1_rd_dat,	0x62),
+	HISI_PMU_EVENT_ATTR(cpa_p0_wr_dat,	0xE1),
+	HISI_PMU_EVENT_ATTR(cpa_p0_rd_dat,	0xE2),
+	NULL
+};
+
+static const struct attribute_group hisi_cpa_pmu_events_group = {
+	.name = "events",
+	.attrs = hisi_cpa_pmu_events_attr,
+};
+
+static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
+
+static struct attribute *hisi_cpa_pmu_cpumask_attrs[] = {
+	&dev_attr_cpumask.attr,
+	NULL
+};
+
+static const struct attribute_group hisi_cpa_pmu_cpumask_attr_group = {
+	.attrs = hisi_cpa_pmu_cpumask_attrs,
+};
+
+static struct device_attribute hisi_cpa_pmu_identifier_attr =
+	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
+
+static struct attribute *hisi_cpa_pmu_identifier_attrs[] = {
+	&hisi_cpa_pmu_identifier_attr.attr,
+	NULL
+};
+
+static const struct attribute_group hisi_cpa_pmu_identifier_group = {
+	.attrs = hisi_cpa_pmu_identifier_attrs,
+};
+
+static const struct attribute_group *hisi_cpa_pmu_attr_groups[] = {
+	&hisi_cpa_pmu_format_group,
+	&hisi_cpa_pmu_events_group,
+	&hisi_cpa_pmu_cpumask_attr_group,
+	&hisi_cpa_pmu_identifier_group,
+	NULL
+};
+
+static const struct hisi_uncore_ops hisi_uncore_cpa_pmu_ops = {
+	.write_evtype           = hisi_cpa_pmu_write_evtype,
+	.get_event_idx		= hisi_uncore_pmu_get_event_idx,
+	.start_counters		= hisi_cpa_pmu_start_counters,
+	.stop_counters		= hisi_cpa_pmu_stop_counters,
+	.enable_counter		= hisi_cpa_pmu_enable_counter,
+	.disable_counter	= hisi_cpa_pmu_disable_counter,
+	.enable_counter_int	= hisi_cpa_pmu_enable_counter_int,
+	.disable_counter_int	= hisi_cpa_pmu_disable_counter_int,
+	.write_counter		= hisi_cpa_pmu_write_counter,
+	.read_counter		= hisi_cpa_pmu_read_counter,
+	.get_int_status		= hisi_cpa_pmu_get_int_status,
+	.clear_int_status	= hisi_cpa_pmu_clear_int_status,
+};
+
+static int hisi_cpa_pmu_dev_probe(struct platform_device *pdev,
+				  struct hisi_pmu *cpa_pmu)
+{
+	int ret;
+
+	ret = hisi_cpa_pmu_init_data(pdev, cpa_pmu);
+	if (ret)
+		return ret;
+
+	ret = hisi_uncore_pmu_init_irq(cpa_pmu, pdev);
+	if (ret)
+		return ret;
+
+	cpa_pmu->counter_bits = CPA_COUNTER_BITS;
+	cpa_pmu->check_event = CPA_NR_EVENTS;
+	cpa_pmu->pmu_events.attr_groups = hisi_cpa_pmu_attr_groups;
+	cpa_pmu->ops = &hisi_uncore_cpa_pmu_ops;
+	cpa_pmu->num_counters = CPA_NR_COUNTERS;
+	cpa_pmu->dev = &pdev->dev;
+	cpa_pmu->on_cpu = -1;
+
+	return 0;
+}
+
+static int hisi_cpa_pmu_probe(struct platform_device *pdev)
+{
+	struct hisi_pmu *cpa_pmu;
+	char *name;
+	int ret;
+
+	cpa_pmu = devm_kzalloc(&pdev->dev, sizeof(*cpa_pmu), GFP_KERNEL);
+	if (!cpa_pmu)
+		return -ENOMEM;
+
+	ret = hisi_cpa_pmu_dev_probe(pdev, cpa_pmu);
+	if (ret)
+		return ret;
+
+	ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
+				       &cpa_pmu->node);
+	if (ret) {
+		dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
+		return ret;
+	}
+
+	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%d_cpa%u", cpa_pmu->sccl_id - 1,
+			      cpa_pmu->index_id);
+
+	cpa_pmu->pmu = (struct pmu) {
+		.name		= name,
+		.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	= cpa_pmu->pmu_events.attr_groups,
+		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
+	};
+
+	ret = perf_pmu_register(&cpa_pmu->pmu, name, -1);
+	if (ret) {
+		dev_err(cpa_pmu->dev, "CPA PMU register failed\n");
+		cpuhp_state_remove_instance_nocalls(
+			CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE, &cpa_pmu->node);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, cpa_pmu);
+	return ret;
+}
+
+static int hisi_cpa_pmu_remove(struct platform_device *pdev)
+{
+	struct hisi_pmu *cpa_pmu = platform_get_drvdata(pdev);
+
+	perf_pmu_unregister(&cpa_pmu->pmu);
+	cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
+					    &cpa_pmu->node);
+	return 0;
+}
+
+static struct platform_driver hisi_cpa_pmu_driver = {
+	.driver = {
+		.name = "hisi_cpa_pmu",
+		.acpi_match_table = ACPI_PTR(hisi_cpa_pmu_acpi_match),
+		.suppress_bind_attrs = true,
+	},
+	.probe = hisi_cpa_pmu_probe,
+	.remove = hisi_cpa_pmu_remove,
+};
+
+static int __init hisi_cpa_pmu_module_init(void)
+{
+	int ret;
+
+	ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
+				      "AP_PERF_ARM_HISI_CPA_ONLINE",
+				      hisi_uncore_pmu_online_cpu,
+				      hisi_uncore_pmu_offline_cpu);
+	if (ret) {
+		pr_err("CPA PMU: setup hotplug: %d\n", ret);
+		return ret;
+	}
+
+	ret = platform_driver_register(&hisi_cpa_pmu_driver);
+	if (ret)
+		cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE);
+
+	return ret;
+}
+module_init(hisi_cpa_pmu_module_init);
+
+static void __exit hisi_cpa_pmu_module_exit(void)
+{
+	platform_driver_unregister(&hisi_cpa_pmu_driver);
+	cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE);
+}
+module_exit(hisi_cpa_pmu_module_exit);
+
+MODULE_DESCRIPTION("HiSilicon SoC CPA uncore PMU driver");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Qi Liu <liuqi115@huawei.com>");
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 411a428ace4d..d55063719904 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -220,6 +220,7 @@ enum cpuhp_state {
 	CPUHP_AP_PERF_S390_SF_ONLINE,
 	CPUHP_AP_PERF_ARM_CCI_ONLINE,
 	CPUHP_AP_PERF_ARM_CCN_ONLINE,
+	CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
 	CPUHP_AP_PERF_ARM_HISI_DDRC_ONLINE,
 	CPUHP_AP_PERF_ARM_HISI_HHA_ONLINE,
 	CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
-- 
2.24.0


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

[PATCH v2 2/2] perf jevents: Add support for HiSilicon CPA PMU aliasing

[Qi Liu]

Add support for HiSilicon CPA PMU aliasing.

The kernel driver is in drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c

Reviewed-by: John Garry <john.garry@huawei.com>
Signed-off-by: Qi Liu <liuqi115@huawei.com>
---
 .../arm64/hisilicon/hip09/sys/uncore-cpa.json | 81 +++++++++++++++++++
 tools/perf/pmu-events/jevents.c               |  1 +
 2 files changed, 82 insertions(+)
 create mode 100644 tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json

diff --git a/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json b/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json
new file mode 100644
index 000000000000..7bcddec8a84f
--- /dev/null
+++ b/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json
@@ -0,0 +1,81 @@
+[
+	{
+		"ConfigCode": "0x00",
+		"EventName": "cpa_cycles",
+		"BriefDescription": "count of CPA cycles",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	},
+	{
+		"ConfigCode": "0x61",
+		"EventName": "cpa_p1_wr_dat",
+		"BriefDescription": "Number of write ops transmitted by the P1 port",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	},
+	{
+		"ConfigCode": "0x62",
+		"EventName": "cpa_p1_rd_dat",
+		"BriefDescription": "Number of read ops transmitted by the P1 port",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	},
+	{
+		"ConfigCode": "0x3",
+		"EventName": "cpa_p1_rd_dat_64b",
+		"BriefDescription": "Number of read ops transmitted by the P1 port which size is 64 bytes",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	},
+	{
+		"ConfigCode": "0x4",
+		"EventName": "cpa_p1_rd_dat_32b",
+		"BriefDescription": "Number of read ops transmitted by the P1 port which size is 32 bytes",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	},
+	{
+		"ConfigCode": "0xE1",
+		"EventName": "cpa_p0_wr_dat",
+		"BriefDescription": "Number of write ops transmitted by the P0 port",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	},
+	{
+		"ConfigCode": "0xE2",
+		"EventName": "cpa_p0_rd_dat",
+		"BriefDescription": "Number of read ops transmitted by the P0 port",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	},
+	{
+		"ConfigCode": "0x83",
+		"EventName": "cpa_p0_rd_dat_64b",
+		"BriefDescription": "Number of read ops transmitted by the P0 port which size is 64 bytes",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	},
+	{
+		"ConfigCode": "0x84",
+		"EventName": "cpa_p0_rd_dat_32b",
+		"BriefDescription": "Number of read ops transmitted by the P0 port which size is 32 bytes",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	},
+	{
+		"MetricExpr": "(cpa_p1_wr_dat * 64 + cpa_p1_rd_dat_64b * 64 + cpa_p1_rd_dat_32b * 32) / cpa_cycles",
+		"BriefDescription": "Average bandwidth of CPA Port 1",
+		"MetricGroup": "CPA",
+		"MetricName": "cpa_p1_avg_bw",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	},
+	{
+		"MetricExpr": "(cpa_p0_wr_dat * 64 + cpa_p0_rd_dat_64b * 64 + cpa_p0_rd_dat_32b * 32) / cpa_cycles",
+		"BriefDescription": "Average bandwidth of CPA Port 0",
+		"MetricGroup": "CPA",
+		"MetricName": "cpa_p0_avg_bw",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	}
+]
diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index 1a57c3f81dd4..159d9eab6e79 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -277,6 +277,7 @@ static struct map {
 	{ "CPU-M-CF", "cpum_cf" },
 	{ "CPU-M-SF", "cpum_sf" },
 	{ "UPI LL", "uncore_upi" },
+	{ "hisi_sicl,cpa", "hisi_sicl,cpa"},
 	{ "hisi_sccl,ddrc", "hisi_sccl,ddrc" },
 	{ "hisi_sccl,hha", "hisi_sccl,hha" },
 	{ "hisi_sccl,l3c", "hisi_sccl,l3c" },
-- 
2.24.0

[PATCH v2 2/2] perf jevents: Add support for HiSilicon CPA PMU aliasing

[Qi Liu]

Add support for HiSilicon CPA PMU aliasing.

The kernel driver is in drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c

Reviewed-by: John Garry <john.garry@huawei.com>
Signed-off-by: Qi Liu <liuqi115@huawei.com>
---
 .../arm64/hisilicon/hip09/sys/uncore-cpa.json | 81 +++++++++++++++++++
 tools/perf/pmu-events/jevents.c               |  1 +
 2 files changed, 82 insertions(+)
 create mode 100644 tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json

diff --git a/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json b/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json
new file mode 100644
index 000000000000..7bcddec8a84f
--- /dev/null
+++ b/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json
@@ -0,0 +1,81 @@
+[
+	{
+		"ConfigCode": "0x00",
+		"EventName": "cpa_cycles",
+		"BriefDescription": "count of CPA cycles",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	},
+	{
+		"ConfigCode": "0x61",
+		"EventName": "cpa_p1_wr_dat",
+		"BriefDescription": "Number of write ops transmitted by the P1 port",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	},
+	{
+		"ConfigCode": "0x62",
+		"EventName": "cpa_p1_rd_dat",
+		"BriefDescription": "Number of read ops transmitted by the P1 port",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	},
+	{
+		"ConfigCode": "0x3",
+		"EventName": "cpa_p1_rd_dat_64b",
+		"BriefDescription": "Number of read ops transmitted by the P1 port which size is 64 bytes",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	},
+	{
+		"ConfigCode": "0x4",
+		"EventName": "cpa_p1_rd_dat_32b",
+		"BriefDescription": "Number of read ops transmitted by the P1 port which size is 32 bytes",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	},
+	{
+		"ConfigCode": "0xE1",
+		"EventName": "cpa_p0_wr_dat",
+		"BriefDescription": "Number of write ops transmitted by the P0 port",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	},
+	{
+		"ConfigCode": "0xE2",
+		"EventName": "cpa_p0_rd_dat",
+		"BriefDescription": "Number of read ops transmitted by the P0 port",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	},
+	{
+		"ConfigCode": "0x83",
+		"EventName": "cpa_p0_rd_dat_64b",
+		"BriefDescription": "Number of read ops transmitted by the P0 port which size is 64 bytes",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	},
+	{
+		"ConfigCode": "0x84",
+		"EventName": "cpa_p0_rd_dat_32b",
+		"BriefDescription": "Number of read ops transmitted by the P0 port which size is 32 bytes",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	},
+	{
+		"MetricExpr": "(cpa_p1_wr_dat * 64 + cpa_p1_rd_dat_64b * 64 + cpa_p1_rd_dat_32b * 32) / cpa_cycles",
+		"BriefDescription": "Average bandwidth of CPA Port 1",
+		"MetricGroup": "CPA",
+		"MetricName": "cpa_p1_avg_bw",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	},
+	{
+		"MetricExpr": "(cpa_p0_wr_dat * 64 + cpa_p0_rd_dat_64b * 64 + cpa_p0_rd_dat_32b * 32) / cpa_cycles",
+		"BriefDescription": "Average bandwidth of CPA Port 0",
+		"MetricGroup": "CPA",
+		"MetricName": "cpa_p0_avg_bw",
+		"Compat": "0x00000030",
+		"Unit": "hisi_sicl,cpa"
+	}
+]
diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index 1a57c3f81dd4..159d9eab6e79 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -277,6 +277,7 @@ static struct map {
 	{ "CPU-M-CF", "cpum_cf" },
 	{ "CPU-M-SF", "cpum_sf" },
 	{ "UPI LL", "uncore_upi" },
+	{ "hisi_sicl,cpa", "hisi_sicl,cpa"},
 	{ "hisi_sccl,ddrc", "hisi_sccl,ddrc" },
 	{ "hisi_sccl,hha", "hisi_sccl,hha" },
 	{ "hisi_sccl,l3c", "hisi_sccl,l3c" },
-- 
2.24.0


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

Re: [PATCH v2 2/2] perf jevents: Add support for HiSilicon CPA PMU aliasing

[Arnaldo Carvalho de Melo]

Em Thu, Feb 24, 2022 at 07:11:29PM +0800, Qi Liu escreveu:
> Add support for HiSilicon CPA PMU aliasing.
> 
> The kernel driver is in drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c

Thanks, applied the tools/perf part, the drivers/perf/ should go via the
kernel maintainer for this area. From what I see its not yet merged.

- Arnaldo

 
> Reviewed-by: John Garry <john.garry@huawei.com>
> Signed-off-by: Qi Liu <liuqi115@huawei.com>
> ---
>  .../arm64/hisilicon/hip09/sys/uncore-cpa.json | 81 +++++++++++++++++++
>  tools/perf/pmu-events/jevents.c               |  1 +
>  2 files changed, 82 insertions(+)
>  create mode 100644 tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json
> 
> diff --git a/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json b/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json
> new file mode 100644
> index 000000000000..7bcddec8a84f
> --- /dev/null
> +++ b/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json
> @@ -0,0 +1,81 @@
> +[
> +	{
> +		"ConfigCode": "0x00",
> +		"EventName": "cpa_cycles",
> +		"BriefDescription": "count of CPA cycles",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	},
> +	{
> +		"ConfigCode": "0x61",
> +		"EventName": "cpa_p1_wr_dat",
> +		"BriefDescription": "Number of write ops transmitted by the P1 port",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	},
> +	{
> +		"ConfigCode": "0x62",
> +		"EventName": "cpa_p1_rd_dat",
> +		"BriefDescription": "Number of read ops transmitted by the P1 port",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	},
> +	{
> +		"ConfigCode": "0x3",
> +		"EventName": "cpa_p1_rd_dat_64b",
> +		"BriefDescription": "Number of read ops transmitted by the P1 port which size is 64 bytes",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	},
> +	{
> +		"ConfigCode": "0x4",
> +		"EventName": "cpa_p1_rd_dat_32b",
> +		"BriefDescription": "Number of read ops transmitted by the P1 port which size is 32 bytes",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	},
> +	{
> +		"ConfigCode": "0xE1",
> +		"EventName": "cpa_p0_wr_dat",
> +		"BriefDescription": "Number of write ops transmitted by the P0 port",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	},
> +	{
> +		"ConfigCode": "0xE2",
> +		"EventName": "cpa_p0_rd_dat",
> +		"BriefDescription": "Number of read ops transmitted by the P0 port",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	},
> +	{
> +		"ConfigCode": "0x83",
> +		"EventName": "cpa_p0_rd_dat_64b",
> +		"BriefDescription": "Number of read ops transmitted by the P0 port which size is 64 bytes",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	},
> +	{
> +		"ConfigCode": "0x84",
> +		"EventName": "cpa_p0_rd_dat_32b",
> +		"BriefDescription": "Number of read ops transmitted by the P0 port which size is 32 bytes",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	},
> +	{
> +		"MetricExpr": "(cpa_p1_wr_dat * 64 + cpa_p1_rd_dat_64b * 64 + cpa_p1_rd_dat_32b * 32) / cpa_cycles",
> +		"BriefDescription": "Average bandwidth of CPA Port 1",
> +		"MetricGroup": "CPA",
> +		"MetricName": "cpa_p1_avg_bw",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	},
> +	{
> +		"MetricExpr": "(cpa_p0_wr_dat * 64 + cpa_p0_rd_dat_64b * 64 + cpa_p0_rd_dat_32b * 32) / cpa_cycles",
> +		"BriefDescription": "Average bandwidth of CPA Port 0",
> +		"MetricGroup": "CPA",
> +		"MetricName": "cpa_p0_avg_bw",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	}
> +]
> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
> index 1a57c3f81dd4..159d9eab6e79 100644
> --- a/tools/perf/pmu-events/jevents.c
> +++ b/tools/perf/pmu-events/jevents.c
> @@ -277,6 +277,7 @@ static struct map {
>  	{ "CPU-M-CF", "cpum_cf" },
>  	{ "CPU-M-SF", "cpum_sf" },
>  	{ "UPI LL", "uncore_upi" },
> +	{ "hisi_sicl,cpa", "hisi_sicl,cpa"},
>  	{ "hisi_sccl,ddrc", "hisi_sccl,ddrc" },
>  	{ "hisi_sccl,hha", "hisi_sccl,hha" },
>  	{ "hisi_sccl,l3c", "hisi_sccl,l3c" },
> -- 
> 2.24.0

-- 

- Arnaldo

Re: [PATCH v2 2/2] perf jevents: Add support for HiSilicon CPA PMU aliasing

[Arnaldo Carvalho de Melo]

Em Thu, Feb 24, 2022 at 07:11:29PM +0800, Qi Liu escreveu:
> Add support for HiSilicon CPA PMU aliasing.
> 
> The kernel driver is in drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c

Thanks, applied the tools/perf part, the drivers/perf/ should go via the
kernel maintainer for this area. From what I see its not yet merged.

- Arnaldo

 
> Reviewed-by: John Garry <john.garry@huawei.com>
> Signed-off-by: Qi Liu <liuqi115@huawei.com>
> ---
>  .../arm64/hisilicon/hip09/sys/uncore-cpa.json | 81 +++++++++++++++++++
>  tools/perf/pmu-events/jevents.c               |  1 +
>  2 files changed, 82 insertions(+)
>  create mode 100644 tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json
> 
> diff --git a/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json b/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json
> new file mode 100644
> index 000000000000..7bcddec8a84f
> --- /dev/null
> +++ b/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json
> @@ -0,0 +1,81 @@
> +[
> +	{
> +		"ConfigCode": "0x00",
> +		"EventName": "cpa_cycles",
> +		"BriefDescription": "count of CPA cycles",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	},
> +	{
> +		"ConfigCode": "0x61",
> +		"EventName": "cpa_p1_wr_dat",
> +		"BriefDescription": "Number of write ops transmitted by the P1 port",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	},
> +	{
> +		"ConfigCode": "0x62",
> +		"EventName": "cpa_p1_rd_dat",
> +		"BriefDescription": "Number of read ops transmitted by the P1 port",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	},
> +	{
> +		"ConfigCode": "0x3",
> +		"EventName": "cpa_p1_rd_dat_64b",
> +		"BriefDescription": "Number of read ops transmitted by the P1 port which size is 64 bytes",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	},
> +	{
> +		"ConfigCode": "0x4",
> +		"EventName": "cpa_p1_rd_dat_32b",
> +		"BriefDescription": "Number of read ops transmitted by the P1 port which size is 32 bytes",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	},
> +	{
> +		"ConfigCode": "0xE1",
> +		"EventName": "cpa_p0_wr_dat",
> +		"BriefDescription": "Number of write ops transmitted by the P0 port",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	},
> +	{
> +		"ConfigCode": "0xE2",
> +		"EventName": "cpa_p0_rd_dat",
> +		"BriefDescription": "Number of read ops transmitted by the P0 port",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	},
> +	{
> +		"ConfigCode": "0x83",
> +		"EventName": "cpa_p0_rd_dat_64b",
> +		"BriefDescription": "Number of read ops transmitted by the P0 port which size is 64 bytes",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	},
> +	{
> +		"ConfigCode": "0x84",
> +		"EventName": "cpa_p0_rd_dat_32b",
> +		"BriefDescription": "Number of read ops transmitted by the P0 port which size is 32 bytes",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	},
> +	{
> +		"MetricExpr": "(cpa_p1_wr_dat * 64 + cpa_p1_rd_dat_64b * 64 + cpa_p1_rd_dat_32b * 32) / cpa_cycles",
> +		"BriefDescription": "Average bandwidth of CPA Port 1",
> +		"MetricGroup": "CPA",
> +		"MetricName": "cpa_p1_avg_bw",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	},
> +	{
> +		"MetricExpr": "(cpa_p0_wr_dat * 64 + cpa_p0_rd_dat_64b * 64 + cpa_p0_rd_dat_32b * 32) / cpa_cycles",
> +		"BriefDescription": "Average bandwidth of CPA Port 0",
> +		"MetricGroup": "CPA",
> +		"MetricName": "cpa_p0_avg_bw",
> +		"Compat": "0x00000030",
> +		"Unit": "hisi_sicl,cpa"
> +	}
> +]
> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
> index 1a57c3f81dd4..159d9eab6e79 100644
> --- a/tools/perf/pmu-events/jevents.c
> +++ b/tools/perf/pmu-events/jevents.c
> @@ -277,6 +277,7 @@ static struct map {
>  	{ "CPU-M-CF", "cpum_cf" },
>  	{ "CPU-M-SF", "cpum_sf" },
>  	{ "UPI LL", "uncore_upi" },
> +	{ "hisi_sicl,cpa", "hisi_sicl,cpa"},
>  	{ "hisi_sccl,ddrc", "hisi_sccl,ddrc" },
>  	{ "hisi_sccl,hha", "hisi_sccl,hha" },
>  	{ "hisi_sccl,l3c", "hisi_sccl,l3c" },
> -- 
> 2.24.0

-- 

- Arnaldo

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

Re: [PATCH v2 1/2] drivers/perf: hisi: Add Support for CPA PMU

[John Garry]

On 24/02/2022 11:11, Qi Liu wrote:
> On HiSilicon Hip09 platform, there is a CPA(Coherency Protocol Agent) on
> each SICL (Super I/O Cluster) which implements packet format translation,
> route parsing and traffic statistic.
> 
> CPA PMU has separate PMU registers which the driver can program freely

I am not sure what you mean by program freely. Most IO mem drivers have 
registers which we can program.

> and
> interrupt is supported to handle counter overflow. Let's support its driver
> under the framework of HiSilicon uncore PMU driver.
> 
> Signed-off-by: Qi Liu <liuqi115@huawei.com>
> ---
>   drivers/perf/hisilicon/Makefile              |   2 +-
>   drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c | 401 +++++++++++++++++++
>   include/linux/cpuhotplug.h                   |   1 +
>   3 files changed, 403 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
> 
> diff --git a/drivers/perf/hisilicon/Makefile b/drivers/perf/hisilicon/Makefile
> index 506ed39e3266..6be83517acaa 100644
> --- a/drivers/perf/hisilicon/Makefile
> +++ b/drivers/perf/hisilicon/Makefile
> @@ -1,6 +1,6 @@
>   # SPDX-License-Identifier: GPL-2.0-only
>   obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o hisi_uncore_l3c_pmu.o \
>   			  hisi_uncore_hha_pmu.o hisi_uncore_ddrc_pmu.o hisi_uncore_sllc_pmu.o \
> -			  hisi_uncore_pa_pmu.o
> +			  hisi_uncore_pa_pmu.o hisi_uncore_cpa_pmu.o
>   
>   obj-$(CONFIG_HISI_PCIE_PMU) += hisi_pcie_pmu.o
> diff --git a/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
> new file mode 100644
> index 000000000000..00573719f148
> --- /dev/null
> +++ b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
> @@ -0,0 +1,401 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * HiSilicon SoC CPA(Coherency Protocol Agent) uncore Hardware event counters support

/s/Hardware/hardware/

> + *
> + * Copyright (C) 2022 HiSilicon Limited
> + * Author: Qi Liu <liuqi115@huawei.com>
> + *
> + * This code is based on the uncore PMUs like arm-cci and arm-ccn.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/bug.h>
> +#include <linux/cpuhotplug.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/list.h>
> +#include <linux/smp.h>
> +
> +#include "hisi_uncore_pmu.h"
> +
> +/* CPA register definition */
> +#define CPA_PERF_CTRL		0x1c00
> +#define CPA_EVENT_CTRL		0x1c04
> +#define CPA_INT_MASK		0x1c70
> +#define CPA_INT_STATUS		0x1c78
> +#define CPA_INT_CLEAR		0x1c7c
> +#define CPA_EVENT_TYPE0		0x1c80
> +#define CPA_VERSION		0x1cf0
> +#define CPA_CNT0_LOWER		0x1d00
> +#define CPA_CFG_REG		0x0534
> +
> +/* CPA operation command */
> +#define CPA_PERF_CTRL_EN	BIT_ULL(0)
> +#define CPA_EVTYPE_MASK		0xffUL
> +#define CPA_PM_CTRL		BIT_ULL(9)
> +
> +/* CPA has 8-counters */
> +#define CPA_NR_COUNTERS		0x8
> +#define CPA_COUNTER_BITS	64
> +#define CPA_NR_EVENTS		0xff
> +#define CPA_REG_OFFSET		0x8
> +
> +static u32 hisi_cpa_pmu_get_counter_offset(int idx)
> +{
> +	return (CPA_CNT0_LOWER + idx * CPA_REG_OFFSET);
> +}
> +
> +static u64 hisi_cpa_pmu_read_counter(struct hisi_pmu *cpa_pmu,
> +				     struct hw_perf_event *hwc)
> +{
> +	return readq(cpa_pmu->base + hisi_cpa_pmu_get_counter_offset(hwc->idx));
> +}
> +
> +static void hisi_cpa_pmu_write_counter(struct hisi_pmu *cpa_pmu,
> +				       struct hw_perf_event *hwc, u64 val)
> +{
> +	writeq(val, cpa_pmu->base + hisi_cpa_pmu_get_counter_offset(hwc->idx));
> +}
> +
> +static void hisi_cpa_pmu_write_evtype(struct hisi_pmu *cpa_pmu, int idx, u32 type)
> +{
> +	u32 reg, reg_idx, shift, val;
> +
> +	/*
> +	 * Select the appropriate event select register(CPA_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,
> +	 * CPA_EVENT_TYPE0 is chosen. For the latter 4 hardware counters,
> +	 * CPA_EVENT_TYPE1 is chosen.
> +	 */
> +	reg = CPA_EVENT_TYPE0 + (idx / 4) * 4;
> +	reg_idx = idx % 4;
> +	shift = CPA_REG_OFFSET * reg_idx;
> +
> +	/* Write event code to CPA_EVENT_TYPEx Register */
> +	val = readl(cpa_pmu->base + reg);
> +	val &= ~(CPA_EVTYPE_MASK << shift);
> +	val |= type << shift;
> +	writel(val, cpa_pmu->base + reg);
> +}
> +
> +static void hisi_cpa_pmu_start_counters(struct hisi_pmu *cpa_pmu)
> +{
> +	u32 val;
> +
> +	val = readl(cpa_pmu->base + CPA_PERF_CTRL);
> +	val |= CPA_PERF_CTRL_EN;
> +	writel(val, cpa_pmu->base + CPA_PERF_CTRL);
> +}
> +
> +static void hisi_cpa_pmu_stop_counters(struct hisi_pmu *cpa_pmu)
> +{
> +	u32 val;
> +
> +	val = readl(cpa_pmu->base + CPA_PERF_CTRL);
> +	val &= ~(CPA_PERF_CTRL_EN);
> +	writel(val, cpa_pmu->base + CPA_PERF_CTRL);
> +}
> +
> +static void hisi_cpa_pmu_disable_pm(struct hisi_pmu *cpa_pmu)

this seems unique for this new driver - why do we need to disable PM?

> +{
> +	u32 val;
> +
> +	val = readl(cpa_pmu->base + CPA_CFG_REG);
> +	val |= CPA_PM_CTRL;
> +	writel(val, cpa_pmu->base + CPA_CFG_REG);
> +}
> +
> +static void hisi_cpa_pmu_enable_pm(struct hisi_pmu *cpa_pmu)
> +{
> +	u32 val;
> +
> +	val = readl(cpa_pmu->base + CPA_CFG_REG);
> +	val &= ~CPA_PM_CTRL;

nit: you use () in hisi_cpa_pmu_stop_counters(), but not here, so please 
be consistent

> +	writel(val, cpa_pmu->base + CPA_CFG_REG);
> +}
> +
> +static void hisi_cpa_pmu_enable_counter(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
> +{
> +	u32 val;
> +
> +	/* Enable counter index in CPA_EVENT_CTRL register */
> +	val = readl(cpa_pmu->base + CPA_EVENT_CTRL);
> +	val |= 1 << hwc->idx;
> +	writel(val, cpa_pmu->base + CPA_EVENT_CTRL);
> +
> +	/* Power management should be disabled before counting. */
> +	hisi_cpa_pmu_disable_pm(cpa_pmu);
> +}
> +
> +static void hisi_cpa_pmu_disable_counter(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
> +{
> +	u32 val;
> +
> +	hisi_cpa_pmu_enable_pm(cpa_pmu);
> +
> +	/* Clear counter index in CPA_EVENT_CTRL register */
> +	val = readl(cpa_pmu->base + CPA_EVENT_CTRL);
> +	val &= ~(1UL << hwc->idx);
> +	writel(val, cpa_pmu->base + CPA_EVENT_CTRL);
> +}
> +
> +static void hisi_cpa_pmu_enable_counter_int(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
> +{
> +	u32 val;
> +
> +	/* Write 0 to enable interrupt */
> +	val = readl(cpa_pmu->base + CPA_INT_MASK);
> +	val &= ~(1UL << hwc->idx);
> +	writel(val, cpa_pmu->base + CPA_INT_MASK);
> +}
> +
> +static void hisi_cpa_pmu_disable_counter_int(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
> +{
> +	u32 val;
> +
> +	/* Write 1 to mask interrupt */
> +	val = readl(cpa_pmu->base + CPA_INT_MASK);
> +	val |= 1 << hwc->idx;
> +	writel(val, cpa_pmu->base + CPA_INT_MASK);
> +}
> +
> +static u32 hisi_cpa_pmu_get_int_status(struct hisi_pmu *cpa_pmu)
> +{
> +	return readl(cpa_pmu->base + CPA_INT_STATUS);
> +}
> +
> +static void hisi_cpa_pmu_clear_int_status(struct hisi_pmu *cpa_pmu, int idx)
> +{
> +	writel(1 << idx, cpa_pmu->base + CPA_INT_CLEAR);
> +}
> +
> +static const struct acpi_device_id hisi_cpa_pmu_acpi_match[] = {
> +	{ "HISI0281", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, hisi_cpa_pmu_acpi_match);
> +
> +static int hisi_cpa_pmu_init_data(struct platform_device *pdev,
> +				  struct hisi_pmu *cpa_pmu)
> +{
> +	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
> +				     &cpa_pmu->sccl_id)) {
> +		dev_err(&pdev->dev, "Can not read cpa_pmu sccl-id\n");

strange that the FW uses "scl-id" but driver uses sccl_id (I am talking 
about sccl vs scl difference)

> +		return -EINVAL;
> +	}
> +
> +	cpa_pmu->ccl_id = -1;
> +
> +	if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
> +				     &cpa_pmu->index_id)) {
> +		dev_err(&pdev->dev, "Cannot read idx-id\n");
> +		return -EINVAL;
> +	}
> +
> +	cpa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(cpa_pmu->base))
> +		return PTR_ERR(cpa_pmu->base);
> +
> +	cpa_pmu->identifier = readl(cpa_pmu->base + CPA_VERSION);
> +
> +	return 0;
> +}
> +
> +static struct attribute *hisi_cpa_pmu_format_attr[] = {
> +	HISI_PMU_FORMAT_ATTR(event, "config:0-15"),
> +	NULL
> +};
> +
> +static const struct attribute_group hisi_cpa_pmu_format_group = {
> +	.name = "format",
> +	.attrs = hisi_cpa_pmu_format_attr,
> +};
> +
> +static struct attribute *hisi_cpa_pmu_events_attr[] = {
> +	HISI_PMU_EVENT_ATTR(cpa_cycles,		0x00),
> +	HISI_PMU_EVENT_ATTR(cpa_p1_wr_dat,	0x61),
> +	HISI_PMU_EVENT_ATTR(cpa_p1_rd_dat,	0x62),
> +	HISI_PMU_EVENT_ATTR(cpa_p0_wr_dat,	0xE1),
> +	HISI_PMU_EVENT_ATTR(cpa_p0_rd_dat,	0xE2),

nit: you were using lower case for hex numbers elsewhere

> +	NULL
> +};
> +
> +static const struct attribute_group hisi_cpa_pmu_events_group = {
> +	.name = "events",
> +	.attrs = hisi_cpa_pmu_events_attr,
> +};
> +
> +static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
> +
> +static struct attribute *hisi_cpa_pmu_cpumask_attrs[] = {
> +	&dev_attr_cpumask.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group hisi_cpa_pmu_cpumask_attr_group = {
> +	.attrs = hisi_cpa_pmu_cpumask_attrs,
> +};
> +
> +static struct device_attribute hisi_cpa_pmu_identifier_attr =
> +	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
> +
> +static struct attribute *hisi_cpa_pmu_identifier_attrs[] = {
> +	&hisi_cpa_pmu_identifier_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group hisi_cpa_pmu_identifier_group = {
> +	.attrs = hisi_cpa_pmu_identifier_attrs,
> +};
> +
> +static const struct attribute_group *hisi_cpa_pmu_attr_groups[] = {
> +	&hisi_cpa_pmu_format_group,
> +	&hisi_cpa_pmu_events_group,
> +	&hisi_cpa_pmu_cpumask_attr_group,
> +	&hisi_cpa_pmu_identifier_group,
> +	NULL
> +};
> +
> +static const struct hisi_uncore_ops hisi_uncore_cpa_pmu_ops = {
> +	.write_evtype           = hisi_cpa_pmu_write_evtype,
> +	.get_event_idx		= hisi_uncore_pmu_get_event_idx,
> +	.start_counters		= hisi_cpa_pmu_start_counters,
> +	.stop_counters		= hisi_cpa_pmu_stop_counters,
> +	.enable_counter		= hisi_cpa_pmu_enable_counter,
> +	.disable_counter	= hisi_cpa_pmu_disable_counter,
> +	.enable_counter_int	= hisi_cpa_pmu_enable_counter_int,
> +	.disable_counter_int	= hisi_cpa_pmu_disable_counter_int,
> +	.write_counter		= hisi_cpa_pmu_write_counter,
> +	.read_counter		= hisi_cpa_pmu_read_counter,
> +	.get_int_status		= hisi_cpa_pmu_get_int_status,
> +	.clear_int_status	= hisi_cpa_pmu_clear_int_status,
> +};
> +
> +static int hisi_cpa_pmu_dev_probe(struct platform_device *pdev,
> +				  struct hisi_pmu *cpa_pmu)
> +{
> +	int ret;
> +
> +	ret = hisi_cpa_pmu_init_data(pdev, cpa_pmu);
> +	if (ret)
> +		return ret;
> +
> +	ret = hisi_uncore_pmu_init_irq(cpa_pmu, pdev);
> +	if (ret)
> +		return ret;
> +
> +	cpa_pmu->counter_bits = CPA_COUNTER_BITS;
> +	cpa_pmu->check_event = CPA_NR_EVENTS;
> +	cpa_pmu->pmu_events.attr_groups = hisi_cpa_pmu_attr_groups;
> +	cpa_pmu->ops = &hisi_uncore_cpa_pmu_ops;
> +	cpa_pmu->num_counters = CPA_NR_COUNTERS;
> +	cpa_pmu->dev = &pdev->dev;
> +	cpa_pmu->on_cpu = -1;
> +
> +	return 0;
> +}
> +
> +static int hisi_cpa_pmu_probe(struct platform_device *pdev)
> +{
> +	struct hisi_pmu *cpa_pmu;
> +	char *name;
> +	int ret;
> +
> +	cpa_pmu = devm_kzalloc(&pdev->dev, sizeof(*cpa_pmu), GFP_KERNEL);
> +	if (!cpa_pmu)
> +		return -ENOMEM;
> +
> +	ret = hisi_cpa_pmu_dev_probe(pdev, cpa_pmu);
> +	if (ret)
> +		return ret;
> +
> +	ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
> +				       &cpa_pmu->node);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
> +		return ret;
> +	}
> +
> +	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%d_cpa%u", cpa_pmu->sccl_id - 1,

sorry, but I still don't like this "- 1". From checking the chat on v1, 
my impression was that you agreed with me on this one.

> +			      cpa_pmu->index_id);
> +
> +	cpa_pmu->pmu = (struct pmu) {
> +		.name		= name,
> +		.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	= cpa_pmu->pmu_events.attr_groups,
> +		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
> +	};
> +
> +	ret = perf_pmu_register(&cpa_pmu->pmu, name, -1);
> +	if (ret) {
> +		dev_err(cpa_pmu->dev, "CPA PMU register failed\n");

I don't think that you need to mention "CPA PMU", as the driver name is 
included in dev_err() prints

> +		cpuhp_state_remove_instance_nocalls(
> +			CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE, &cpa_pmu->node);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, cpa_pmu);
> +	return ret;
> +}
> +


Thanks,
John

Re: [PATCH v2 1/2] drivers/perf: hisi: Add Support for CPA PMU

[John Garry]

On 24/02/2022 11:11, Qi Liu wrote:
> On HiSilicon Hip09 platform, there is a CPA(Coherency Protocol Agent) on
> each SICL (Super I/O Cluster) which implements packet format translation,
> route parsing and traffic statistic.
> 
> CPA PMU has separate PMU registers which the driver can program freely

I am not sure what you mean by program freely. Most IO mem drivers have 
registers which we can program.

> and
> interrupt is supported to handle counter overflow. Let's support its driver
> under the framework of HiSilicon uncore PMU driver.
> 
> Signed-off-by: Qi Liu <liuqi115@huawei.com>
> ---
>   drivers/perf/hisilicon/Makefile              |   2 +-
>   drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c | 401 +++++++++++++++++++
>   include/linux/cpuhotplug.h                   |   1 +
>   3 files changed, 403 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
> 
> diff --git a/drivers/perf/hisilicon/Makefile b/drivers/perf/hisilicon/Makefile
> index 506ed39e3266..6be83517acaa 100644
> --- a/drivers/perf/hisilicon/Makefile
> +++ b/drivers/perf/hisilicon/Makefile
> @@ -1,6 +1,6 @@
>   # SPDX-License-Identifier: GPL-2.0-only
>   obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o hisi_uncore_l3c_pmu.o \
>   			  hisi_uncore_hha_pmu.o hisi_uncore_ddrc_pmu.o hisi_uncore_sllc_pmu.o \
> -			  hisi_uncore_pa_pmu.o
> +			  hisi_uncore_pa_pmu.o hisi_uncore_cpa_pmu.o
>   
>   obj-$(CONFIG_HISI_PCIE_PMU) += hisi_pcie_pmu.o
> diff --git a/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
> new file mode 100644
> index 000000000000..00573719f148
> --- /dev/null
> +++ b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
> @@ -0,0 +1,401 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * HiSilicon SoC CPA(Coherency Protocol Agent) uncore Hardware event counters support

/s/Hardware/hardware/

> + *
> + * Copyright (C) 2022 HiSilicon Limited
> + * Author: Qi Liu <liuqi115@huawei.com>
> + *
> + * This code is based on the uncore PMUs like arm-cci and arm-ccn.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/bug.h>
> +#include <linux/cpuhotplug.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/list.h>
> +#include <linux/smp.h>
> +
> +#include "hisi_uncore_pmu.h"
> +
> +/* CPA register definition */
> +#define CPA_PERF_CTRL		0x1c00
> +#define CPA_EVENT_CTRL		0x1c04
> +#define CPA_INT_MASK		0x1c70
> +#define CPA_INT_STATUS		0x1c78
> +#define CPA_INT_CLEAR		0x1c7c
> +#define CPA_EVENT_TYPE0		0x1c80
> +#define CPA_VERSION		0x1cf0
> +#define CPA_CNT0_LOWER		0x1d00
> +#define CPA_CFG_REG		0x0534
> +
> +/* CPA operation command */
> +#define CPA_PERF_CTRL_EN	BIT_ULL(0)
> +#define CPA_EVTYPE_MASK		0xffUL
> +#define CPA_PM_CTRL		BIT_ULL(9)
> +
> +/* CPA has 8-counters */
> +#define CPA_NR_COUNTERS		0x8
> +#define CPA_COUNTER_BITS	64
> +#define CPA_NR_EVENTS		0xff
> +#define CPA_REG_OFFSET		0x8
> +
> +static u32 hisi_cpa_pmu_get_counter_offset(int idx)
> +{
> +	return (CPA_CNT0_LOWER + idx * CPA_REG_OFFSET);
> +}
> +
> +static u64 hisi_cpa_pmu_read_counter(struct hisi_pmu *cpa_pmu,
> +				     struct hw_perf_event *hwc)
> +{
> +	return readq(cpa_pmu->base + hisi_cpa_pmu_get_counter_offset(hwc->idx));
> +}
> +
> +static void hisi_cpa_pmu_write_counter(struct hisi_pmu *cpa_pmu,
> +				       struct hw_perf_event *hwc, u64 val)
> +{
> +	writeq(val, cpa_pmu->base + hisi_cpa_pmu_get_counter_offset(hwc->idx));
> +}
> +
> +static void hisi_cpa_pmu_write_evtype(struct hisi_pmu *cpa_pmu, int idx, u32 type)
> +{
> +	u32 reg, reg_idx, shift, val;
> +
> +	/*
> +	 * Select the appropriate event select register(CPA_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,
> +	 * CPA_EVENT_TYPE0 is chosen. For the latter 4 hardware counters,
> +	 * CPA_EVENT_TYPE1 is chosen.
> +	 */
> +	reg = CPA_EVENT_TYPE0 + (idx / 4) * 4;
> +	reg_idx = idx % 4;
> +	shift = CPA_REG_OFFSET * reg_idx;
> +
> +	/* Write event code to CPA_EVENT_TYPEx Register */
> +	val = readl(cpa_pmu->base + reg);
> +	val &= ~(CPA_EVTYPE_MASK << shift);
> +	val |= type << shift;
> +	writel(val, cpa_pmu->base + reg);
> +}
> +
> +static void hisi_cpa_pmu_start_counters(struct hisi_pmu *cpa_pmu)
> +{
> +	u32 val;
> +
> +	val = readl(cpa_pmu->base + CPA_PERF_CTRL);
> +	val |= CPA_PERF_CTRL_EN;
> +	writel(val, cpa_pmu->base + CPA_PERF_CTRL);
> +}
> +
> +static void hisi_cpa_pmu_stop_counters(struct hisi_pmu *cpa_pmu)
> +{
> +	u32 val;
> +
> +	val = readl(cpa_pmu->base + CPA_PERF_CTRL);
> +	val &= ~(CPA_PERF_CTRL_EN);
> +	writel(val, cpa_pmu->base + CPA_PERF_CTRL);
> +}
> +
> +static void hisi_cpa_pmu_disable_pm(struct hisi_pmu *cpa_pmu)

this seems unique for this new driver - why do we need to disable PM?

> +{
> +	u32 val;
> +
> +	val = readl(cpa_pmu->base + CPA_CFG_REG);
> +	val |= CPA_PM_CTRL;
> +	writel(val, cpa_pmu->base + CPA_CFG_REG);
> +}
> +
> +static void hisi_cpa_pmu_enable_pm(struct hisi_pmu *cpa_pmu)
> +{
> +	u32 val;
> +
> +	val = readl(cpa_pmu->base + CPA_CFG_REG);
> +	val &= ~CPA_PM_CTRL;

nit: you use () in hisi_cpa_pmu_stop_counters(), but not here, so please 
be consistent

> +	writel(val, cpa_pmu->base + CPA_CFG_REG);
> +}
> +
> +static void hisi_cpa_pmu_enable_counter(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
> +{
> +	u32 val;
> +
> +	/* Enable counter index in CPA_EVENT_CTRL register */
> +	val = readl(cpa_pmu->base + CPA_EVENT_CTRL);
> +	val |= 1 << hwc->idx;
> +	writel(val, cpa_pmu->base + CPA_EVENT_CTRL);
> +
> +	/* Power management should be disabled before counting. */
> +	hisi_cpa_pmu_disable_pm(cpa_pmu);
> +}
> +
> +static void hisi_cpa_pmu_disable_counter(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
> +{
> +	u32 val;
> +
> +	hisi_cpa_pmu_enable_pm(cpa_pmu);
> +
> +	/* Clear counter index in CPA_EVENT_CTRL register */
> +	val = readl(cpa_pmu->base + CPA_EVENT_CTRL);
> +	val &= ~(1UL << hwc->idx);
> +	writel(val, cpa_pmu->base + CPA_EVENT_CTRL);
> +}
> +
> +static void hisi_cpa_pmu_enable_counter_int(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
> +{
> +	u32 val;
> +
> +	/* Write 0 to enable interrupt */
> +	val = readl(cpa_pmu->base + CPA_INT_MASK);
> +	val &= ~(1UL << hwc->idx);
> +	writel(val, cpa_pmu->base + CPA_INT_MASK);
> +}
> +
> +static void hisi_cpa_pmu_disable_counter_int(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
> +{
> +	u32 val;
> +
> +	/* Write 1 to mask interrupt */
> +	val = readl(cpa_pmu->base + CPA_INT_MASK);
> +	val |= 1 << hwc->idx;
> +	writel(val, cpa_pmu->base + CPA_INT_MASK);
> +}
> +
> +static u32 hisi_cpa_pmu_get_int_status(struct hisi_pmu *cpa_pmu)
> +{
> +	return readl(cpa_pmu->base + CPA_INT_STATUS);
> +}
> +
> +static void hisi_cpa_pmu_clear_int_status(struct hisi_pmu *cpa_pmu, int idx)
> +{
> +	writel(1 << idx, cpa_pmu->base + CPA_INT_CLEAR);
> +}
> +
> +static const struct acpi_device_id hisi_cpa_pmu_acpi_match[] = {
> +	{ "HISI0281", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, hisi_cpa_pmu_acpi_match);
> +
> +static int hisi_cpa_pmu_init_data(struct platform_device *pdev,
> +				  struct hisi_pmu *cpa_pmu)
> +{
> +	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
> +				     &cpa_pmu->sccl_id)) {
> +		dev_err(&pdev->dev, "Can not read cpa_pmu sccl-id\n");

strange that the FW uses "scl-id" but driver uses sccl_id (I am talking 
about sccl vs scl difference)

> +		return -EINVAL;
> +	}
> +
> +	cpa_pmu->ccl_id = -1;
> +
> +	if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
> +				     &cpa_pmu->index_id)) {
> +		dev_err(&pdev->dev, "Cannot read idx-id\n");
> +		return -EINVAL;
> +	}
> +
> +	cpa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(cpa_pmu->base))
> +		return PTR_ERR(cpa_pmu->base);
> +
> +	cpa_pmu->identifier = readl(cpa_pmu->base + CPA_VERSION);
> +
> +	return 0;
> +}
> +
> +static struct attribute *hisi_cpa_pmu_format_attr[] = {
> +	HISI_PMU_FORMAT_ATTR(event, "config:0-15"),
> +	NULL
> +};
> +
> +static const struct attribute_group hisi_cpa_pmu_format_group = {
> +	.name = "format",
> +	.attrs = hisi_cpa_pmu_format_attr,
> +};
> +
> +static struct attribute *hisi_cpa_pmu_events_attr[] = {
> +	HISI_PMU_EVENT_ATTR(cpa_cycles,		0x00),
> +	HISI_PMU_EVENT_ATTR(cpa_p1_wr_dat,	0x61),
> +	HISI_PMU_EVENT_ATTR(cpa_p1_rd_dat,	0x62),
> +	HISI_PMU_EVENT_ATTR(cpa_p0_wr_dat,	0xE1),
> +	HISI_PMU_EVENT_ATTR(cpa_p0_rd_dat,	0xE2),

nit: you were using lower case for hex numbers elsewhere

> +	NULL
> +};
> +
> +static const struct attribute_group hisi_cpa_pmu_events_group = {
> +	.name = "events",
> +	.attrs = hisi_cpa_pmu_events_attr,
> +};
> +
> +static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
> +
> +static struct attribute *hisi_cpa_pmu_cpumask_attrs[] = {
> +	&dev_attr_cpumask.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group hisi_cpa_pmu_cpumask_attr_group = {
> +	.attrs = hisi_cpa_pmu_cpumask_attrs,
> +};
> +
> +static struct device_attribute hisi_cpa_pmu_identifier_attr =
> +	__ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, NULL);
> +
> +static struct attribute *hisi_cpa_pmu_identifier_attrs[] = {
> +	&hisi_cpa_pmu_identifier_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group hisi_cpa_pmu_identifier_group = {
> +	.attrs = hisi_cpa_pmu_identifier_attrs,
> +};
> +
> +static const struct attribute_group *hisi_cpa_pmu_attr_groups[] = {
> +	&hisi_cpa_pmu_format_group,
> +	&hisi_cpa_pmu_events_group,
> +	&hisi_cpa_pmu_cpumask_attr_group,
> +	&hisi_cpa_pmu_identifier_group,
> +	NULL
> +};
> +
> +static const struct hisi_uncore_ops hisi_uncore_cpa_pmu_ops = {
> +	.write_evtype           = hisi_cpa_pmu_write_evtype,
> +	.get_event_idx		= hisi_uncore_pmu_get_event_idx,
> +	.start_counters		= hisi_cpa_pmu_start_counters,
> +	.stop_counters		= hisi_cpa_pmu_stop_counters,
> +	.enable_counter		= hisi_cpa_pmu_enable_counter,
> +	.disable_counter	= hisi_cpa_pmu_disable_counter,
> +	.enable_counter_int	= hisi_cpa_pmu_enable_counter_int,
> +	.disable_counter_int	= hisi_cpa_pmu_disable_counter_int,
> +	.write_counter		= hisi_cpa_pmu_write_counter,
> +	.read_counter		= hisi_cpa_pmu_read_counter,
> +	.get_int_status		= hisi_cpa_pmu_get_int_status,
> +	.clear_int_status	= hisi_cpa_pmu_clear_int_status,
> +};
> +
> +static int hisi_cpa_pmu_dev_probe(struct platform_device *pdev,
> +				  struct hisi_pmu *cpa_pmu)
> +{
> +	int ret;
> +
> +	ret = hisi_cpa_pmu_init_data(pdev, cpa_pmu);
> +	if (ret)
> +		return ret;
> +
> +	ret = hisi_uncore_pmu_init_irq(cpa_pmu, pdev);
> +	if (ret)
> +		return ret;
> +
> +	cpa_pmu->counter_bits = CPA_COUNTER_BITS;
> +	cpa_pmu->check_event = CPA_NR_EVENTS;
> +	cpa_pmu->pmu_events.attr_groups = hisi_cpa_pmu_attr_groups;
> +	cpa_pmu->ops = &hisi_uncore_cpa_pmu_ops;
> +	cpa_pmu->num_counters = CPA_NR_COUNTERS;
> +	cpa_pmu->dev = &pdev->dev;
> +	cpa_pmu->on_cpu = -1;
> +
> +	return 0;
> +}
> +
> +static int hisi_cpa_pmu_probe(struct platform_device *pdev)
> +{
> +	struct hisi_pmu *cpa_pmu;
> +	char *name;
> +	int ret;
> +
> +	cpa_pmu = devm_kzalloc(&pdev->dev, sizeof(*cpa_pmu), GFP_KERNEL);
> +	if (!cpa_pmu)
> +		return -ENOMEM;
> +
> +	ret = hisi_cpa_pmu_dev_probe(pdev, cpa_pmu);
> +	if (ret)
> +		return ret;
> +
> +	ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
> +				       &cpa_pmu->node);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
> +		return ret;
> +	}
> +
> +	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%d_cpa%u", cpa_pmu->sccl_id - 1,

sorry, but I still don't like this "- 1". From checking the chat on v1, 
my impression was that you agreed with me on this one.

> +			      cpa_pmu->index_id);
> +
> +	cpa_pmu->pmu = (struct pmu) {
> +		.name		= name,
> +		.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	= cpa_pmu->pmu_events.attr_groups,
> +		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
> +	};
> +
> +	ret = perf_pmu_register(&cpa_pmu->pmu, name, -1);
> +	if (ret) {
> +		dev_err(cpa_pmu->dev, "CPA PMU register failed\n");

I don't think that you need to mention "CPA PMU", as the driver name is 
included in dev_err() prints

> +		cpuhp_state_remove_instance_nocalls(
> +			CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE, &cpa_pmu->node);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, cpa_pmu);
> +	return ret;
> +}
> +


Thanks,
John

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

Re: [PATCH v2 1/2] drivers/perf: hisi: Add Support for CPA PMU

[liuqi (BA)]

Hi John,

Thanks for the comments. some replies inline.

On 2022/2/28 17:56, John Garry wrote:
> On 24/02/2022 11:11, Qi Liu wrote:
>> On HiSilicon Hip09 platform, there is a CPA(Coherency Protocol Agent) on
>> each SICL (Super I/O Cluster) which implements packet format translation,
>> route parsing and traffic statistic.
>>
>> CPA PMU has separate PMU registers which the driver can program freely
> 
> I am not sure what you mean by program freely. Most IO mem drivers have 
> registers which we can program.
> 
I'll drop these ambiguous description, thanks.

>> and
>> interrupt is supported to handle counter overflow. Let's support its 
>> driver
>> under the framework of HiSilicon uncore PMU driver.
>>
>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
>> ---
>>   drivers/perf/hisilicon/Makefile              |   2 +-
>>   drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c | 401 +++++++++++++++++++
>>   include/linux/cpuhotplug.h                   |   1 +
>>   3 files changed, 403 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
>>
>> diff --git a/drivers/perf/hisilicon/Makefile 
>> b/drivers/perf/hisilicon/Makefile
>> index 506ed39e3266..6be83517acaa 100644
>> --- a/drivers/perf/hisilicon/Makefile
>> +++ b/drivers/perf/hisilicon/Makefile
>> @@ -1,6 +1,6 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>>   obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o hisi_uncore_l3c_pmu.o \
>>                 hisi_uncore_hha_pmu.o hisi_uncore_ddrc_pmu.o 
>> hisi_uncore_sllc_pmu.o \
>> -              hisi_uncore_pa_pmu.o
>> +              hisi_uncore_pa_pmu.o hisi_uncore_cpa_pmu.o
>>   obj-$(CONFIG_HISI_PCIE_PMU) += hisi_pcie_pmu.o
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c 
>> b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
>> new file mode 100644
>> index 000000000000..00573719f148
>> --- /dev/null
>> +++ b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
>> @@ -0,0 +1,401 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * HiSilicon SoC CPA(Coherency Protocol Agent) uncore Hardware event 
>> counters support
> 
> /s/Hardware/hardware/
> 
ok, will fix this.
>> + *
>> + * Copyright (C) 2022 HiSilicon Limited
>> + * Author: Qi Liu <liuqi115@huawei.com>
>> + *
>> + * This code is based on the uncore PMUs like arm-cci and arm-ccn.
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/bug.h>
>> +#include <linux/cpuhotplug.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/list.h>
>> +#include <linux/smp.h>
>> +
>> +#include "hisi_uncore_pmu.h"
>> +
>> +/* CPA register definition */
>> +#define CPA_PERF_CTRL        0x1c00
>> +#define CPA_EVENT_CTRL        0x1c04
>> +#define CPA_INT_MASK        0x1c70
>> +#define CPA_INT_STATUS        0x1c78
>> +#define CPA_INT_CLEAR        0x1c7c
>> +#define CPA_EVENT_TYPE0        0x1c80
>> +#define CPA_VERSION        0x1cf0
>> +#define CPA_CNT0_LOWER        0x1d00
>> +#define CPA_CFG_REG        0x0534
>> +
>> +/* CPA operation command */
>> +#define CPA_PERF_CTRL_EN    BIT_ULL(0)
>> +#define CPA_EVTYPE_MASK        0xffUL
>> +#define CPA_PM_CTRL        BIT_ULL(9)
>> +
>> +/* CPA has 8-counters */
>> +#define CPA_NR_COUNTERS        0x8
>> +#define CPA_COUNTER_BITS    64
>> +#define CPA_NR_EVENTS        0xff
>> +#define CPA_REG_OFFSET        0x8
>> +
>> +static u32 hisi_cpa_pmu_get_counter_offset(int idx)
>> +{
>> +    return (CPA_CNT0_LOWER + idx * CPA_REG_OFFSET);
>> +}
>> +
>> +static u64 hisi_cpa_pmu_read_counter(struct hisi_pmu *cpa_pmu,
>> +                     struct hw_perf_event *hwc)
>> +{
>> +    return readq(cpa_pmu->base + 
>> hisi_cpa_pmu_get_counter_offset(hwc->idx));
>> +}
>> +
>> +static void hisi_cpa_pmu_write_counter(struct hisi_pmu *cpa_pmu,
>> +                       struct hw_perf_event *hwc, u64 val)
>> +{
>> +    writeq(val, cpa_pmu->base + 
>> hisi_cpa_pmu_get_counter_offset(hwc->idx));
>> +}
>> +
>> +static void hisi_cpa_pmu_write_evtype(struct hisi_pmu *cpa_pmu, int 
>> idx, u32 type)
>> +{
>> +    u32 reg, reg_idx, shift, val;
>> +
>> +    /*
>> +     * Select the appropriate event select register(CPA_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,
>> +     * CPA_EVENT_TYPE0 is chosen. For the latter 4 hardware counters,
>> +     * CPA_EVENT_TYPE1 is chosen.
>> +     */
>> +    reg = CPA_EVENT_TYPE0 + (idx / 4) * 4;
>> +    reg_idx = idx % 4;
>> +    shift = CPA_REG_OFFSET * reg_idx;
>> +
>> +    /* Write event code to CPA_EVENT_TYPEx Register */
>> +    val = readl(cpa_pmu->base + reg);
>> +    val &= ~(CPA_EVTYPE_MASK << shift);
>> +    val |= type << shift;
>> +    writel(val, cpa_pmu->base + reg);
>> +}
>> +
>> +static void hisi_cpa_pmu_start_counters(struct hisi_pmu *cpa_pmu)
>> +{
>> +    u32 val;
>> +
>> +    val = readl(cpa_pmu->base + CPA_PERF_CTRL);
>> +    val |= CPA_PERF_CTRL_EN;
>> +    writel(val, cpa_pmu->base + CPA_PERF_CTRL);
>> +}
>> +
>> +static void hisi_cpa_pmu_stop_counters(struct hisi_pmu *cpa_pmu)
>> +{
>> +    u32 val;
>> +
>> +    val = readl(cpa_pmu->base + CPA_PERF_CTRL);
>> +    val &= ~(CPA_PERF_CTRL_EN);
>> +    writel(val, cpa_pmu->base + CPA_PERF_CTRL);
>> +}
>> +
>> +static void hisi_cpa_pmu_disable_pm(struct hisi_pmu *cpa_pmu)
> 
> this seems unique for this new driver - why do we need to disable PM?
> 
CPA PMU doesn't work under power mangement state, so we need to disable 
PM before using CPA PMU.

>> +{
>> +    u32 val;
>> +
>> +    val = readl(cpa_pmu->base + CPA_CFG_REG);
>> +    val |= CPA_PM_CTRL;
>> +    writel(val, cpa_pmu->base + CPA_CFG_REG);
>> +}
>> +
>> +static void hisi_cpa_pmu_enable_pm(struct hisi_pmu *cpa_pmu)
>> +{
>> +    u32 val;
>> +
>> +    val = readl(cpa_pmu->base + CPA_CFG_REG);
>> +    val &= ~CPA_PM_CTRL;
> 
> nit: you use () in hisi_cpa_pmu_stop_counters(), but not here, so please 
> be consistent
> 

got it, I'll pay attention to this. thanks.
>> +    writel(val, cpa_pmu->base + CPA_CFG_REG);
>> +}
>> +
>> +static void hisi_cpa_pmu_enable_counter(struct hisi_pmu *cpa_pmu, 
>> struct hw_perf_event *hwc)
>> +{
>> +    u32 val;
>> +
>> +    /* Enable counter index in CPA_EVENT_CTRL register */
>> +    val = readl(cpa_pmu->base + CPA_EVENT_CTRL);
>> +    val |= 1 << hwc->idx;
>> +    writel(val, cpa_pmu->base + CPA_EVENT_CTRL);
>> +
>> +    /* Power management should be disabled before counting. */
>> +    hisi_cpa_pmu_disable_pm(cpa_pmu);
>> +}
>> +
>> +static void hisi_cpa_pmu_disable_counter(struct hisi_pmu *cpa_pmu, 
>> struct hw_perf_event *hwc)
>> +{
>> +    u32 val;
>> +
>> +    hisi_cpa_pmu_enable_pm(cpa_pmu);
>> +
>> +    /* Clear counter index in CPA_EVENT_CTRL register */
>> +    val = readl(cpa_pmu->base + CPA_EVENT_CTRL);
>> +    val &= ~(1UL << hwc->idx);
>> +    writel(val, cpa_pmu->base + CPA_EVENT_CTRL);
>> +}
>> +
>> +static void hisi_cpa_pmu_enable_counter_int(struct hisi_pmu *cpa_pmu, 
>> struct hw_perf_event *hwc)
>> +{
>> +    u32 val;
>> +
>> +    /* Write 0 to enable interrupt */
>> +    val = readl(cpa_pmu->base + CPA_INT_MASK);
>> +    val &= ~(1UL << hwc->idx);
>> +    writel(val, cpa_pmu->base + CPA_INT_MASK);
>> +}
>> +
>> +static void hisi_cpa_pmu_disable_counter_int(struct hisi_pmu 
>> *cpa_pmu, struct hw_perf_event *hwc)
>> +{
>> +    u32 val;
>> +
>> +    /* Write 1 to mask interrupt */
>> +    val = readl(cpa_pmu->base + CPA_INT_MASK);
>> +    val |= 1 << hwc->idx;
>> +    writel(val, cpa_pmu->base + CPA_INT_MASK);
>> +}
>> +
>> +static u32 hisi_cpa_pmu_get_int_status(struct hisi_pmu *cpa_pmu)
>> +{
>> +    return readl(cpa_pmu->base + CPA_INT_STATUS);
>> +}
>> +
>> +static void hisi_cpa_pmu_clear_int_status(struct hisi_pmu *cpa_pmu, 
>> int idx)
>> +{
>> +    writel(1 << idx, cpa_pmu->base + CPA_INT_CLEAR);
>> +}
>> +
>> +static const struct acpi_device_id hisi_cpa_pmu_acpi_match[] = {
>> +    { "HISI0281", },
>> +    {}
>> +};
>> +MODULE_DEVICE_TABLE(acpi, hisi_cpa_pmu_acpi_match);
>> +
>> +static int hisi_cpa_pmu_init_data(struct platform_device *pdev,
>> +                  struct hisi_pmu *cpa_pmu)
>> +{
>> +    if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
>> +                     &cpa_pmu->sccl_id)) {
>> +        dev_err(&pdev->dev, "Can not read cpa_pmu sccl-id\n");
> 
> strange that the FW uses "scl-id" but driver uses sccl_id (I am talking 
> about sccl vs scl difference)
> 
yes... these two names means the same thing, maybe could use a cleanup 
patch to keep them consistent.

>> +        return -EINVAL;
>> +    }
>> +
>> +    cpa_pmu->ccl_id = -1;
>> +
>> +    if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
>> +                     &cpa_pmu->index_id)) {
>> +        dev_err(&pdev->dev, "Cannot read idx-id\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    cpa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
>> +    if (IS_ERR(cpa_pmu->base))
>> +        return PTR_ERR(cpa_pmu->base);
>> +
>> +    cpa_pmu->identifier = readl(cpa_pmu->base + CPA_VERSION);
>> +
>> +    return 0;
>> +}
>> +
>> +static struct attribute *hisi_cpa_pmu_format_attr[] = {
>> +    HISI_PMU_FORMAT_ATTR(event, "config:0-15"),
>> +    NULL
>> +};
>> +
>> +static const struct attribute_group hisi_cpa_pmu_format_group = {
>> +    .name = "format",
>> +    .attrs = hisi_cpa_pmu_format_attr,
>> +};
>> +
>> +static struct attribute *hisi_cpa_pmu_events_attr[] = {
>> +    HISI_PMU_EVENT_ATTR(cpa_cycles,        0x00),
>> +    HISI_PMU_EVENT_ATTR(cpa_p1_wr_dat,    0x61),
>> +    HISI_PMU_EVENT_ATTR(cpa_p1_rd_dat,    0x62),
>> +    HISI_PMU_EVENT_ATTR(cpa_p0_wr_dat,    0xE1),
>> +    HISI_PMU_EVENT_ATTR(cpa_p0_rd_dat,    0xE2),
> 
> nit: you were using lower case for hex numbers elsewhere
> 
>> +    NULL
>> +};
>> +
>> +static const struct attribute_group hisi_cpa_pmu_events_group = {
>> +    .name = "events",
>> +    .attrs = hisi_cpa_pmu_events_attr,
>> +};
>> +
>> +static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
>> +
>> +static struct attribute *hisi_cpa_pmu_cpumask_attrs[] = {
>> +    &dev_attr_cpumask.attr,
>> +    NULL
>> +};
>> +
>> +static const struct attribute_group hisi_cpa_pmu_cpumask_attr_group = {
>> +    .attrs = hisi_cpa_pmu_cpumask_attrs,
>> +};
>> +
>> +static struct device_attribute hisi_cpa_pmu_identifier_attr =
>> +    __ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, 
>> NULL);
>> +
>> +static struct attribute *hisi_cpa_pmu_identifier_attrs[] = {
>> +    &hisi_cpa_pmu_identifier_attr.attr,
>> +    NULL
>> +};
>> +
>> +static const struct attribute_group hisi_cpa_pmu_identifier_group = {
>> +    .attrs = hisi_cpa_pmu_identifier_attrs,
>> +};
>> +
>> +static const struct attribute_group *hisi_cpa_pmu_attr_groups[] = {
>> +    &hisi_cpa_pmu_format_group,
>> +    &hisi_cpa_pmu_events_group,
>> +    &hisi_cpa_pmu_cpumask_attr_group,
>> +    &hisi_cpa_pmu_identifier_group,
>> +    NULL
>> +};
>> +
>> +static const struct hisi_uncore_ops hisi_uncore_cpa_pmu_ops = {
>> +    .write_evtype           = hisi_cpa_pmu_write_evtype,
>> +    .get_event_idx        = hisi_uncore_pmu_get_event_idx,
>> +    .start_counters        = hisi_cpa_pmu_start_counters,
>> +    .stop_counters        = hisi_cpa_pmu_stop_counters,
>> +    .enable_counter        = hisi_cpa_pmu_enable_counter,
>> +    .disable_counter    = hisi_cpa_pmu_disable_counter,
>> +    .enable_counter_int    = hisi_cpa_pmu_enable_counter_int,
>> +    .disable_counter_int    = hisi_cpa_pmu_disable_counter_int,
>> +    .write_counter        = hisi_cpa_pmu_write_counter,
>> +    .read_counter        = hisi_cpa_pmu_read_counter,
>> +    .get_int_status        = hisi_cpa_pmu_get_int_status,
>> +    .clear_int_status    = hisi_cpa_pmu_clear_int_status,
>> +};
>> +
>> +static int hisi_cpa_pmu_dev_probe(struct platform_device *pdev,
>> +                  struct hisi_pmu *cpa_pmu)
>> +{
>> +    int ret;
>> +
>> +    ret = hisi_cpa_pmu_init_data(pdev, cpa_pmu);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = hisi_uncore_pmu_init_irq(cpa_pmu, pdev);
>> +    if (ret)
>> +        return ret;
>> +
>> +    cpa_pmu->counter_bits = CPA_COUNTER_BITS;
>> +    cpa_pmu->check_event = CPA_NR_EVENTS;
>> +    cpa_pmu->pmu_events.attr_groups = hisi_cpa_pmu_attr_groups;
>> +    cpa_pmu->ops = &hisi_uncore_cpa_pmu_ops;
>> +    cpa_pmu->num_counters = CPA_NR_COUNTERS;
>> +    cpa_pmu->dev = &pdev->dev;
>> +    cpa_pmu->on_cpu = -1;
>> +
>> +    return 0;
>> +}
>> +
>> +static int hisi_cpa_pmu_probe(struct platform_device *pdev)
>> +{
>> +    struct hisi_pmu *cpa_pmu;
>> +    char *name;
>> +    int ret;
>> +
>> +    cpa_pmu = devm_kzalloc(&pdev->dev, sizeof(*cpa_pmu), GFP_KERNEL);
>> +    if (!cpa_pmu)
>> +        return -ENOMEM;
>> +
>> +    ret = hisi_cpa_pmu_dev_probe(pdev, cpa_pmu);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
>> +                       &cpa_pmu->node);
>> +    if (ret) {
>> +        dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    name = devm_kasprintf(&pdev->dev, GFP_KERNEL, 
>> "hisi_sicl%d_cpa%u", cpa_pmu->sccl_id - 1,
> 
> sorry, but I still don't like this "- 1". From checking the chat on v1, 
> my impression was that you agreed with me on this one.
> 

sorry, maybe I didn't express it clearly.
CPA PMU is on IO die, but BIOS give CPA PMU driver the sccl-id of 
adjacent CPU die (as we need this sccl-id to pass 
hisi_pmu_cpu_is_associated_pmu() and find the associated cpu).
so, when naming CPA PMU, we use the sccl-id supported by BIOS to get the 
sicl-id of IO die which CPA PMU located in, that is: cpa_pmu->sccl_id - 1.

>> +                  cpa_pmu->index_id);
>> +
>> +    cpa_pmu->pmu = (struct pmu) {
>> +        .name        = name,
>> +        .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    = cpa_pmu->pmu_events.attr_groups,
>> +        .capabilities    = PERF_PMU_CAP_NO_EXCLUDE,
>> +    };
>> +
>> +    ret = perf_pmu_register(&cpa_pmu->pmu, name, -1);
>> +    if (ret) {
>> +        dev_err(cpa_pmu->dev, "CPA PMU register failed\n");
> 
> I don't think that you need to mention "CPA PMU", as the driver name is 
> included in dev_err() prints

got it, will fix this.

Thanks,
Qi
> 
>> +        cpuhp_state_remove_instance_nocalls(
>> +            CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE, &cpa_pmu->node);
>> +        return ret;
>> +    }
>> +
>> +    platform_set_drvdata(pdev, cpa_pmu);
>> +    return ret;
>> +}
>> +
> 
> 
> Thanks,
> John
> .

Re: [PATCH v2 1/2] drivers/perf: hisi: Add Support for CPA PMU

[liuqi (BA)]

Hi John,

Thanks for the comments. some replies inline.

On 2022/2/28 17:56, John Garry wrote:
> On 24/02/2022 11:11, Qi Liu wrote:
>> On HiSilicon Hip09 platform, there is a CPA(Coherency Protocol Agent) on
>> each SICL (Super I/O Cluster) which implements packet format translation,
>> route parsing and traffic statistic.
>>
>> CPA PMU has separate PMU registers which the driver can program freely
> 
> I am not sure what you mean by program freely. Most IO mem drivers have 
> registers which we can program.
> 
I'll drop these ambiguous description, thanks.

>> and
>> interrupt is supported to handle counter overflow. Let's support its 
>> driver
>> under the framework of HiSilicon uncore PMU driver.
>>
>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
>> ---
>>   drivers/perf/hisilicon/Makefile              |   2 +-
>>   drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c | 401 +++++++++++++++++++
>>   include/linux/cpuhotplug.h                   |   1 +
>>   3 files changed, 403 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
>>
>> diff --git a/drivers/perf/hisilicon/Makefile 
>> b/drivers/perf/hisilicon/Makefile
>> index 506ed39e3266..6be83517acaa 100644
>> --- a/drivers/perf/hisilicon/Makefile
>> +++ b/drivers/perf/hisilicon/Makefile
>> @@ -1,6 +1,6 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>>   obj-$(CONFIG_HISI_PMU) += hisi_uncore_pmu.o hisi_uncore_l3c_pmu.o \
>>                 hisi_uncore_hha_pmu.o hisi_uncore_ddrc_pmu.o 
>> hisi_uncore_sllc_pmu.o \
>> -              hisi_uncore_pa_pmu.o
>> +              hisi_uncore_pa_pmu.o hisi_uncore_cpa_pmu.o
>>   obj-$(CONFIG_HISI_PCIE_PMU) += hisi_pcie_pmu.o
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c 
>> b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
>> new file mode 100644
>> index 000000000000..00573719f148
>> --- /dev/null
>> +++ b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
>> @@ -0,0 +1,401 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * HiSilicon SoC CPA(Coherency Protocol Agent) uncore Hardware event 
>> counters support
> 
> /s/Hardware/hardware/
> 
ok, will fix this.
>> + *
>> + * Copyright (C) 2022 HiSilicon Limited
>> + * Author: Qi Liu <liuqi115@huawei.com>
>> + *
>> + * This code is based on the uncore PMUs like arm-cci and arm-ccn.
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/bug.h>
>> +#include <linux/cpuhotplug.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/list.h>
>> +#include <linux/smp.h>
>> +
>> +#include "hisi_uncore_pmu.h"
>> +
>> +/* CPA register definition */
>> +#define CPA_PERF_CTRL        0x1c00
>> +#define CPA_EVENT_CTRL        0x1c04
>> +#define CPA_INT_MASK        0x1c70
>> +#define CPA_INT_STATUS        0x1c78
>> +#define CPA_INT_CLEAR        0x1c7c
>> +#define CPA_EVENT_TYPE0        0x1c80
>> +#define CPA_VERSION        0x1cf0
>> +#define CPA_CNT0_LOWER        0x1d00
>> +#define CPA_CFG_REG        0x0534
>> +
>> +/* CPA operation command */
>> +#define CPA_PERF_CTRL_EN    BIT_ULL(0)
>> +#define CPA_EVTYPE_MASK        0xffUL
>> +#define CPA_PM_CTRL        BIT_ULL(9)
>> +
>> +/* CPA has 8-counters */
>> +#define CPA_NR_COUNTERS        0x8
>> +#define CPA_COUNTER_BITS    64
>> +#define CPA_NR_EVENTS        0xff
>> +#define CPA_REG_OFFSET        0x8
>> +
>> +static u32 hisi_cpa_pmu_get_counter_offset(int idx)
>> +{
>> +    return (CPA_CNT0_LOWER + idx * CPA_REG_OFFSET);
>> +}
>> +
>> +static u64 hisi_cpa_pmu_read_counter(struct hisi_pmu *cpa_pmu,
>> +                     struct hw_perf_event *hwc)
>> +{
>> +    return readq(cpa_pmu->base + 
>> hisi_cpa_pmu_get_counter_offset(hwc->idx));
>> +}
>> +
>> +static void hisi_cpa_pmu_write_counter(struct hisi_pmu *cpa_pmu,
>> +                       struct hw_perf_event *hwc, u64 val)
>> +{
>> +    writeq(val, cpa_pmu->base + 
>> hisi_cpa_pmu_get_counter_offset(hwc->idx));
>> +}
>> +
>> +static void hisi_cpa_pmu_write_evtype(struct hisi_pmu *cpa_pmu, int 
>> idx, u32 type)
>> +{
>> +    u32 reg, reg_idx, shift, val;
>> +
>> +    /*
>> +     * Select the appropriate event select register(CPA_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,
>> +     * CPA_EVENT_TYPE0 is chosen. For the latter 4 hardware counters,
>> +     * CPA_EVENT_TYPE1 is chosen.
>> +     */
>> +    reg = CPA_EVENT_TYPE0 + (idx / 4) * 4;
>> +    reg_idx = idx % 4;
>> +    shift = CPA_REG_OFFSET * reg_idx;
>> +
>> +    /* Write event code to CPA_EVENT_TYPEx Register */
>> +    val = readl(cpa_pmu->base + reg);
>> +    val &= ~(CPA_EVTYPE_MASK << shift);
>> +    val |= type << shift;
>> +    writel(val, cpa_pmu->base + reg);
>> +}
>> +
>> +static void hisi_cpa_pmu_start_counters(struct hisi_pmu *cpa_pmu)
>> +{
>> +    u32 val;
>> +
>> +    val = readl(cpa_pmu->base + CPA_PERF_CTRL);
>> +    val |= CPA_PERF_CTRL_EN;
>> +    writel(val, cpa_pmu->base + CPA_PERF_CTRL);
>> +}
>> +
>> +static void hisi_cpa_pmu_stop_counters(struct hisi_pmu *cpa_pmu)
>> +{
>> +    u32 val;
>> +
>> +    val = readl(cpa_pmu->base + CPA_PERF_CTRL);
>> +    val &= ~(CPA_PERF_CTRL_EN);
>> +    writel(val, cpa_pmu->base + CPA_PERF_CTRL);
>> +}
>> +
>> +static void hisi_cpa_pmu_disable_pm(struct hisi_pmu *cpa_pmu)
> 
> this seems unique for this new driver - why do we need to disable PM?
> 
CPA PMU doesn't work under power mangement state, so we need to disable 
PM before using CPA PMU.

>> +{
>> +    u32 val;
>> +
>> +    val = readl(cpa_pmu->base + CPA_CFG_REG);
>> +    val |= CPA_PM_CTRL;
>> +    writel(val, cpa_pmu->base + CPA_CFG_REG);
>> +}
>> +
>> +static void hisi_cpa_pmu_enable_pm(struct hisi_pmu *cpa_pmu)
>> +{
>> +    u32 val;
>> +
>> +    val = readl(cpa_pmu->base + CPA_CFG_REG);
>> +    val &= ~CPA_PM_CTRL;
> 
> nit: you use () in hisi_cpa_pmu_stop_counters(), but not here, so please 
> be consistent
> 

got it, I'll pay attention to this. thanks.
>> +    writel(val, cpa_pmu->base + CPA_CFG_REG);
>> +}
>> +
>> +static void hisi_cpa_pmu_enable_counter(struct hisi_pmu *cpa_pmu, 
>> struct hw_perf_event *hwc)
>> +{
>> +    u32 val;
>> +
>> +    /* Enable counter index in CPA_EVENT_CTRL register */
>> +    val = readl(cpa_pmu->base + CPA_EVENT_CTRL);
>> +    val |= 1 << hwc->idx;
>> +    writel(val, cpa_pmu->base + CPA_EVENT_CTRL);
>> +
>> +    /* Power management should be disabled before counting. */
>> +    hisi_cpa_pmu_disable_pm(cpa_pmu);
>> +}
>> +
>> +static void hisi_cpa_pmu_disable_counter(struct hisi_pmu *cpa_pmu, 
>> struct hw_perf_event *hwc)
>> +{
>> +    u32 val;
>> +
>> +    hisi_cpa_pmu_enable_pm(cpa_pmu);
>> +
>> +    /* Clear counter index in CPA_EVENT_CTRL register */
>> +    val = readl(cpa_pmu->base + CPA_EVENT_CTRL);
>> +    val &= ~(1UL << hwc->idx);
>> +    writel(val, cpa_pmu->base + CPA_EVENT_CTRL);
>> +}
>> +
>> +static void hisi_cpa_pmu_enable_counter_int(struct hisi_pmu *cpa_pmu, 
>> struct hw_perf_event *hwc)
>> +{
>> +    u32 val;
>> +
>> +    /* Write 0 to enable interrupt */
>> +    val = readl(cpa_pmu->base + CPA_INT_MASK);
>> +    val &= ~(1UL << hwc->idx);
>> +    writel(val, cpa_pmu->base + CPA_INT_MASK);
>> +}
>> +
>> +static void hisi_cpa_pmu_disable_counter_int(struct hisi_pmu 
>> *cpa_pmu, struct hw_perf_event *hwc)
>> +{
>> +    u32 val;
>> +
>> +    /* Write 1 to mask interrupt */
>> +    val = readl(cpa_pmu->base + CPA_INT_MASK);
>> +    val |= 1 << hwc->idx;
>> +    writel(val, cpa_pmu->base + CPA_INT_MASK);
>> +}
>> +
>> +static u32 hisi_cpa_pmu_get_int_status(struct hisi_pmu *cpa_pmu)
>> +{
>> +    return readl(cpa_pmu->base + CPA_INT_STATUS);
>> +}
>> +
>> +static void hisi_cpa_pmu_clear_int_status(struct hisi_pmu *cpa_pmu, 
>> int idx)
>> +{
>> +    writel(1 << idx, cpa_pmu->base + CPA_INT_CLEAR);
>> +}
>> +
>> +static const struct acpi_device_id hisi_cpa_pmu_acpi_match[] = {
>> +    { "HISI0281", },
>> +    {}
>> +};
>> +MODULE_DEVICE_TABLE(acpi, hisi_cpa_pmu_acpi_match);
>> +
>> +static int hisi_cpa_pmu_init_data(struct platform_device *pdev,
>> +                  struct hisi_pmu *cpa_pmu)
>> +{
>> +    if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
>> +                     &cpa_pmu->sccl_id)) {
>> +        dev_err(&pdev->dev, "Can not read cpa_pmu sccl-id\n");
> 
> strange that the FW uses "scl-id" but driver uses sccl_id (I am talking 
> about sccl vs scl difference)
> 
yes... these two names means the same thing, maybe could use a cleanup 
patch to keep them consistent.

>> +        return -EINVAL;
>> +    }
>> +
>> +    cpa_pmu->ccl_id = -1;
>> +
>> +    if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
>> +                     &cpa_pmu->index_id)) {
>> +        dev_err(&pdev->dev, "Cannot read idx-id\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    cpa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
>> +    if (IS_ERR(cpa_pmu->base))
>> +        return PTR_ERR(cpa_pmu->base);
>> +
>> +    cpa_pmu->identifier = readl(cpa_pmu->base + CPA_VERSION);
>> +
>> +    return 0;
>> +}
>> +
>> +static struct attribute *hisi_cpa_pmu_format_attr[] = {
>> +    HISI_PMU_FORMAT_ATTR(event, "config:0-15"),
>> +    NULL
>> +};
>> +
>> +static const struct attribute_group hisi_cpa_pmu_format_group = {
>> +    .name = "format",
>> +    .attrs = hisi_cpa_pmu_format_attr,
>> +};
>> +
>> +static struct attribute *hisi_cpa_pmu_events_attr[] = {
>> +    HISI_PMU_EVENT_ATTR(cpa_cycles,        0x00),
>> +    HISI_PMU_EVENT_ATTR(cpa_p1_wr_dat,    0x61),
>> +    HISI_PMU_EVENT_ATTR(cpa_p1_rd_dat,    0x62),
>> +    HISI_PMU_EVENT_ATTR(cpa_p0_wr_dat,    0xE1),
>> +    HISI_PMU_EVENT_ATTR(cpa_p0_rd_dat,    0xE2),
> 
> nit: you were using lower case for hex numbers elsewhere
> 
>> +    NULL
>> +};
>> +
>> +static const struct attribute_group hisi_cpa_pmu_events_group = {
>> +    .name = "events",
>> +    .attrs = hisi_cpa_pmu_events_attr,
>> +};
>> +
>> +static DEVICE_ATTR(cpumask, 0444, hisi_cpumask_sysfs_show, NULL);
>> +
>> +static struct attribute *hisi_cpa_pmu_cpumask_attrs[] = {
>> +    &dev_attr_cpumask.attr,
>> +    NULL
>> +};
>> +
>> +static const struct attribute_group hisi_cpa_pmu_cpumask_attr_group = {
>> +    .attrs = hisi_cpa_pmu_cpumask_attrs,
>> +};
>> +
>> +static struct device_attribute hisi_cpa_pmu_identifier_attr =
>> +    __ATTR(identifier, 0444, hisi_uncore_pmu_identifier_attr_show, 
>> NULL);
>> +
>> +static struct attribute *hisi_cpa_pmu_identifier_attrs[] = {
>> +    &hisi_cpa_pmu_identifier_attr.attr,
>> +    NULL
>> +};
>> +
>> +static const struct attribute_group hisi_cpa_pmu_identifier_group = {
>> +    .attrs = hisi_cpa_pmu_identifier_attrs,
>> +};
>> +
>> +static const struct attribute_group *hisi_cpa_pmu_attr_groups[] = {
>> +    &hisi_cpa_pmu_format_group,
>> +    &hisi_cpa_pmu_events_group,
>> +    &hisi_cpa_pmu_cpumask_attr_group,
>> +    &hisi_cpa_pmu_identifier_group,
>> +    NULL
>> +};
>> +
>> +static const struct hisi_uncore_ops hisi_uncore_cpa_pmu_ops = {
>> +    .write_evtype           = hisi_cpa_pmu_write_evtype,
>> +    .get_event_idx        = hisi_uncore_pmu_get_event_idx,
>> +    .start_counters        = hisi_cpa_pmu_start_counters,
>> +    .stop_counters        = hisi_cpa_pmu_stop_counters,
>> +    .enable_counter        = hisi_cpa_pmu_enable_counter,
>> +    .disable_counter    = hisi_cpa_pmu_disable_counter,
>> +    .enable_counter_int    = hisi_cpa_pmu_enable_counter_int,
>> +    .disable_counter_int    = hisi_cpa_pmu_disable_counter_int,
>> +    .write_counter        = hisi_cpa_pmu_write_counter,
>> +    .read_counter        = hisi_cpa_pmu_read_counter,
>> +    .get_int_status        = hisi_cpa_pmu_get_int_status,
>> +    .clear_int_status    = hisi_cpa_pmu_clear_int_status,
>> +};
>> +
>> +static int hisi_cpa_pmu_dev_probe(struct platform_device *pdev,
>> +                  struct hisi_pmu *cpa_pmu)
>> +{
>> +    int ret;
>> +
>> +    ret = hisi_cpa_pmu_init_data(pdev, cpa_pmu);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = hisi_uncore_pmu_init_irq(cpa_pmu, pdev);
>> +    if (ret)
>> +        return ret;
>> +
>> +    cpa_pmu->counter_bits = CPA_COUNTER_BITS;
>> +    cpa_pmu->check_event = CPA_NR_EVENTS;
>> +    cpa_pmu->pmu_events.attr_groups = hisi_cpa_pmu_attr_groups;
>> +    cpa_pmu->ops = &hisi_uncore_cpa_pmu_ops;
>> +    cpa_pmu->num_counters = CPA_NR_COUNTERS;
>> +    cpa_pmu->dev = &pdev->dev;
>> +    cpa_pmu->on_cpu = -1;
>> +
>> +    return 0;
>> +}
>> +
>> +static int hisi_cpa_pmu_probe(struct platform_device *pdev)
>> +{
>> +    struct hisi_pmu *cpa_pmu;
>> +    char *name;
>> +    int ret;
>> +
>> +    cpa_pmu = devm_kzalloc(&pdev->dev, sizeof(*cpa_pmu), GFP_KERNEL);
>> +    if (!cpa_pmu)
>> +        return -ENOMEM;
>> +
>> +    ret = hisi_cpa_pmu_dev_probe(pdev, cpa_pmu);
>> +    if (ret)
>> +        return ret;
>> +
>> +    ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
>> +                       &cpa_pmu->node);
>> +    if (ret) {
>> +        dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    name = devm_kasprintf(&pdev->dev, GFP_KERNEL, 
>> "hisi_sicl%d_cpa%u", cpa_pmu->sccl_id - 1,
> 
> sorry, but I still don't like this "- 1". From checking the chat on v1, 
> my impression was that you agreed with me on this one.
> 

sorry, maybe I didn't express it clearly.
CPA PMU is on IO die, but BIOS give CPA PMU driver the sccl-id of 
adjacent CPU die (as we need this sccl-id to pass 
hisi_pmu_cpu_is_associated_pmu() and find the associated cpu).
so, when naming CPA PMU, we use the sccl-id supported by BIOS to get the 
sicl-id of IO die which CPA PMU located in, that is: cpa_pmu->sccl_id - 1.

>> +                  cpa_pmu->index_id);
>> +
>> +    cpa_pmu->pmu = (struct pmu) {
>> +        .name        = name,
>> +        .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    = cpa_pmu->pmu_events.attr_groups,
>> +        .capabilities    = PERF_PMU_CAP_NO_EXCLUDE,
>> +    };
>> +
>> +    ret = perf_pmu_register(&cpa_pmu->pmu, name, -1);
>> +    if (ret) {
>> +        dev_err(cpa_pmu->dev, "CPA PMU register failed\n");
> 
> I don't think that you need to mention "CPA PMU", as the driver name is 
> included in dev_err() prints

got it, will fix this.

Thanks,
Qi
> 
>> +        cpuhp_state_remove_instance_nocalls(
>> +            CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE, &cpa_pmu->node);
>> +        return ret;
>> +    }
>> +
>> +    platform_set_drvdata(pdev, cpa_pmu);
>> +    return ret;
>> +}
>> +
> 
> 
> Thanks,
> John
> .

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

Re: [PATCH v2 1/2] drivers/perf: hisi: Add Support for CPA PMU

[John Garry]

>>> +
>>> +static void hisi_cpa_pmu_stop_counters(struct hisi_pmu *cpa_pmu)
>>> +{
>>> +    u32 val;
>>> +
>>> +    val = readl(cpa_pmu->base + CPA_PERF_CTRL);
>>> +    val &= ~(CPA_PERF_CTRL_EN);
>>> +    writel(val, cpa_pmu->base + CPA_PERF_CTRL);
>>> +}
>>> +
>>> +static void hisi_cpa_pmu_disable_pm(struct hisi_pmu *cpa_pmu)
>>
>> this seems unique for this new driver - why do we need to disable PM?
>>
> CPA PMU doesn't work under power mangement state, so we need to disable 
> PM before using CPA PMU.

OK, so you need to disable something related to PM to use the HW. But 
why do it when you enable the counter? I mean, can we just do it once 
when we probe the HW? Why is it even enabled at all?

> 
>>> +{
>>> +    u32 val;
>>> +
>>> +    val = readl(cpa_pmu->base + CPA_CFG_REG);
>>> +    val |= CPA_PM_CTRL;
>>> +    writel(val, cpa_pmu->base + CPA_CFG_REG);
>>> +}
>>> +
>>> +static void hisi_cpa_pmu_enable_pm(struct hisi_pmu *cpa_pmu)
>>> +{
>>> +    u32 val;
>>> +
>>> +    val = readl(cpa_pmu->base + CPA_CFG_REG);
>>> +    val &= ~CPA_PM_CTRL;
>>
>> nit: you use () in hisi_cpa_pmu_stop_counters(), but not here, so 
>> please be consistent
>>
> ]

...

>>> +static int hisi_cpa_pmu_init_data(struct platform_device *pdev,
>>> +                  struct hisi_pmu *cpa_pmu)
>>> +{
>>> +    if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
>>> +                     &cpa_pmu->sccl_id)) {
>>> +        dev_err(&pdev->dev, "Can not read cpa_pmu sccl-id\n");
>>
>> strange that the FW uses "scl-id" but driver uses sccl_id (I am 
>> talking about sccl vs scl difference)
>>
> yes... these two names means the same thing, maybe could use a cleanup 
> patch to keep them consistent.
> 
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    cpa_pmu->ccl_id = -1;
>>> +
>>> +    if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
>>> +                     &cpa_pmu->index_id)) {
>>> +        dev_err(&pdev->dev, "Cannot read idx-id\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    cpa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
>>> +    if (IS_ERR(cpa_pmu->base))
>>> +        return PTR_ERR(cpa_pmu->base);
>>> +
>>> +    cpa_pmu->identifier = readl(cpa_pmu->base + CPA_VERSION);
>>> +
>>> +    return 0;
>>> +}
>>> +

...

>>> +static int hisi_cpa_pmu_probe(struct platform_device *pdev)
>>> +{
>>> +    struct hisi_pmu *cpa_pmu;
>>> +    char *name;
>>> +    int ret;
>>> +
>>> +    cpa_pmu = devm_kzalloc(&pdev->dev, sizeof(*cpa_pmu), GFP_KERNEL);
>>> +    if (!cpa_pmu)
>>> +        return -ENOMEM;
>>> +
>>> +    ret = hisi_cpa_pmu_dev_probe(pdev, cpa_pmu);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
>>> +                       &cpa_pmu->node);
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
>>> +        return ret;
>>> +    }
>>> +
>>> +    name = devm_kasprintf(&pdev->dev, GFP_KERNEL, 
>>> "hisi_sicl%d_cpa%u", cpa_pmu->sccl_id - 1,
>>
>> sorry, but I still don't like this "- 1". From checking the chat on 
>> v1, my impression was that you agreed with me on this one.
>>
> 
> sorry, maybe I didn't express it clearly.
> CPA PMU is on IO die,

Then it is quite strange to provide the sccl id and not some sicl id 
(since it is in the IO die)

  but BIOS give CPA PMU driver the sccl-id of
> adjacent CPU die (as we need this sccl-id to pass 
> hisi_pmu_cpu_is_associated_pmu() and find the associated cpu).


Is the PMU only ever used by one SCCL? Or can more than one SCCL access? 
If so, is there a special affinity to one particular SCCL?

> so, when naming CPA PMU, we use the sccl-id supported by BIOS to get the 
> sicl-id of IO die which CPA PMU located in, that is: cpa_pmu->sccl_id - 1.
> 
>>> +                  cpa_pmu->index_id);

Thanks,
John

Re: [PATCH v2 1/2] drivers/perf: hisi: Add Support for CPA PMU

[John Garry]

>>> +
>>> +static void hisi_cpa_pmu_stop_counters(struct hisi_pmu *cpa_pmu)
>>> +{
>>> +    u32 val;
>>> +
>>> +    val = readl(cpa_pmu->base + CPA_PERF_CTRL);
>>> +    val &= ~(CPA_PERF_CTRL_EN);
>>> +    writel(val, cpa_pmu->base + CPA_PERF_CTRL);
>>> +}
>>> +
>>> +static void hisi_cpa_pmu_disable_pm(struct hisi_pmu *cpa_pmu)
>>
>> this seems unique for this new driver - why do we need to disable PM?
>>
> CPA PMU doesn't work under power mangement state, so we need to disable 
> PM before using CPA PMU.

OK, so you need to disable something related to PM to use the HW. But 
why do it when you enable the counter? I mean, can we just do it once 
when we probe the HW? Why is it even enabled at all?

> 
>>> +{
>>> +    u32 val;
>>> +
>>> +    val = readl(cpa_pmu->base + CPA_CFG_REG);
>>> +    val |= CPA_PM_CTRL;
>>> +    writel(val, cpa_pmu->base + CPA_CFG_REG);
>>> +}
>>> +
>>> +static void hisi_cpa_pmu_enable_pm(struct hisi_pmu *cpa_pmu)
>>> +{
>>> +    u32 val;
>>> +
>>> +    val = readl(cpa_pmu->base + CPA_CFG_REG);
>>> +    val &= ~CPA_PM_CTRL;
>>
>> nit: you use () in hisi_cpa_pmu_stop_counters(), but not here, so 
>> please be consistent
>>
> ]

...

>>> +static int hisi_cpa_pmu_init_data(struct platform_device *pdev,
>>> +                  struct hisi_pmu *cpa_pmu)
>>> +{
>>> +    if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
>>> +                     &cpa_pmu->sccl_id)) {
>>> +        dev_err(&pdev->dev, "Can not read cpa_pmu sccl-id\n");
>>
>> strange that the FW uses "scl-id" but driver uses sccl_id (I am 
>> talking about sccl vs scl difference)
>>
> yes... these two names means the same thing, maybe could use a cleanup 
> patch to keep them consistent.
> 
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    cpa_pmu->ccl_id = -1;
>>> +
>>> +    if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
>>> +                     &cpa_pmu->index_id)) {
>>> +        dev_err(&pdev->dev, "Cannot read idx-id\n");
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    cpa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
>>> +    if (IS_ERR(cpa_pmu->base))
>>> +        return PTR_ERR(cpa_pmu->base);
>>> +
>>> +    cpa_pmu->identifier = readl(cpa_pmu->base + CPA_VERSION);
>>> +
>>> +    return 0;
>>> +}
>>> +

...

>>> +static int hisi_cpa_pmu_probe(struct platform_device *pdev)
>>> +{
>>> +    struct hisi_pmu *cpa_pmu;
>>> +    char *name;
>>> +    int ret;
>>> +
>>> +    cpa_pmu = devm_kzalloc(&pdev->dev, sizeof(*cpa_pmu), GFP_KERNEL);
>>> +    if (!cpa_pmu)
>>> +        return -ENOMEM;
>>> +
>>> +    ret = hisi_cpa_pmu_dev_probe(pdev, cpa_pmu);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
>>> +                       &cpa_pmu->node);
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
>>> +        return ret;
>>> +    }
>>> +
>>> +    name = devm_kasprintf(&pdev->dev, GFP_KERNEL, 
>>> "hisi_sicl%d_cpa%u", cpa_pmu->sccl_id - 1,
>>
>> sorry, but I still don't like this "- 1". From checking the chat on 
>> v1, my impression was that you agreed with me on this one.
>>
> 
> sorry, maybe I didn't express it clearly.
> CPA PMU is on IO die,

Then it is quite strange to provide the sccl id and not some sicl id 
(since it is in the IO die)

  but BIOS give CPA PMU driver the sccl-id of
> adjacent CPU die (as we need this sccl-id to pass 
> hisi_pmu_cpu_is_associated_pmu() and find the associated cpu).


Is the PMU only ever used by one SCCL? Or can more than one SCCL access? 
If so, is there a special affinity to one particular SCCL?

> so, when naming CPA PMU, we use the sccl-id supported by BIOS to get the 
> sicl-id of IO die which CPA PMU located in, that is: cpa_pmu->sccl_id - 1.
> 
>>> +                  cpa_pmu->index_id);

Thanks,
John

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

Re: [PATCH v2 1/2] drivers/perf: hisi: Add Support for CPA PMU

[liuqi (BA)]

Hi John,
On 2022/3/8 1:50, John Garry wrote:
> 
>>>> +
>>>> +static void hisi_cpa_pmu_stop_counters(struct hisi_pmu *cpa_pmu)
>>>> +{
>>>> +    u32 val;
>>>> +
>>>> +    val = readl(cpa_pmu->base + CPA_PERF_CTRL);
>>>> +    val &= ~(CPA_PERF_CTRL_EN);
>>>> +    writel(val, cpa_pmu->base + CPA_PERF_CTRL);
>>>> +}
>>>> +
>>>> +static void hisi_cpa_pmu_disable_pm(struct hisi_pmu *cpa_pmu)
>>>
>>> this seems unique for this new driver - why do we need to disable PM?
>>>
>> CPA PMU doesn't work under power mangement state, so we need to 
>> disable PM before using CPA PMU.
> 
> OK, so you need to disable something related to PM to use the HW. But 
> why do it when you enable the counter? I mean, can we just do it once 
> when we probe the HW? Why is it even enabled at all?
> 
>>

PM is enabled by default. yes, we could disable it in probe function and 
restore in remove function, I'll change this in next version.

Thanks,
Qi
>>>> +{
>>>> +    u32 val;
>>>> +
>>>> +    val = readl(cpa_pmu->base + CPA_CFG_REG);
>>>> +    val |= CPA_PM_CTRL;
>>>> +    writel(val, cpa_pmu->base + CPA_CFG_REG);
>>>> +}
>>>> +
>>>> +static void hisi_cpa_pmu_enable_pm(struct hisi_pmu *cpa_pmu)
>>>> +{
>>>> +    u32 val;
>>>> +
>>>> +    val = readl(cpa_pmu->base + CPA_CFG_REG);
>>>> +    val &= ~CPA_PM_CTRL;
>>>
>>> nit: you use () in hisi_cpa_pmu_stop_counters(), but not here, so 
>>> please be consistent
>>>
>> ]
> 
> ...
> 
>>>> +static int hisi_cpa_pmu_init_data(struct platform_device *pdev,
>>>> +                  struct hisi_pmu *cpa_pmu)
>>>> +{
>>>> +    if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
>>>> +                     &cpa_pmu->sccl_id)) {
>>>> +        dev_err(&pdev->dev, "Can not read cpa_pmu sccl-id\n");
>>>
>>> strange that the FW uses "scl-id" but driver uses sccl_id (I am 
>>> talking about sccl vs scl difference)
>>>
>> yes... these two names means the same thing, maybe could use a cleanup 
>> patch to keep them consistent.
>>
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    cpa_pmu->ccl_id = -1;
>>>> +
>>>> +    if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
>>>> +                     &cpa_pmu->index_id)) {
>>>> +        dev_err(&pdev->dev, "Cannot read idx-id\n");
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    cpa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
>>>> +    if (IS_ERR(cpa_pmu->base))
>>>> +        return PTR_ERR(cpa_pmu->base);
>>>> +
>>>> +    cpa_pmu->identifier = readl(cpa_pmu->base + CPA_VERSION);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
> 
> ...
> 
>>>> +static int hisi_cpa_pmu_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct hisi_pmu *cpa_pmu;
>>>> +    char *name;
>>>> +    int ret;
>>>> +
>>>> +    cpa_pmu = devm_kzalloc(&pdev->dev, sizeof(*cpa_pmu), GFP_KERNEL);
>>>> +    if (!cpa_pmu)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    ret = hisi_cpa_pmu_dev_probe(pdev, cpa_pmu);
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
>>>> +                       &cpa_pmu->node);
>>>> +    if (ret) {
>>>> +        dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    name = devm_kasprintf(&pdev->dev, GFP_KERNEL, 
>>>> "hisi_sicl%d_cpa%u", cpa_pmu->sccl_id - 1,
>>>
>>> sorry, but I still don't like this "- 1". From checking the chat on 
>>> v1, my impression was that you agreed with me on this one.
>>>
>>
>> sorry, maybe I didn't express it clearly.
>> CPA PMU is on IO die,
> 
> Then it is quite strange to provide the sccl id and not some sicl id 
> (since it is in the IO die)
> 
>   but BIOS give CPA PMU driver the sccl-id of
>> adjacent CPU die (as we need this sccl-id to pass 
>> hisi_pmu_cpu_is_associated_pmu() and find the associated cpu).
> 
> 
> Is the PMU only ever used by one SCCL? Or can more than one SCCL access? 
> If so, is there a special affinity to one particular SCCL?
> 
>> so, when naming CPA PMU, we use the sccl-id supported by BIOS to get 
>> the sicl-id of IO die which CPA PMU located in, that is: 
>> cpa_pmu->sccl_id - 1.
>>
>>>> +                  cpa_pmu->index_id);
> 
> Thanks,
> John
> .

Re: [PATCH v2 1/2] drivers/perf: hisi: Add Support for CPA PMU

[liuqi (BA)]

Hi John,
On 2022/3/8 1:50, John Garry wrote:
> 
>>>> +
>>>> +static void hisi_cpa_pmu_stop_counters(struct hisi_pmu *cpa_pmu)
>>>> +{
>>>> +    u32 val;
>>>> +
>>>> +    val = readl(cpa_pmu->base + CPA_PERF_CTRL);
>>>> +    val &= ~(CPA_PERF_CTRL_EN);
>>>> +    writel(val, cpa_pmu->base + CPA_PERF_CTRL);
>>>> +}
>>>> +
>>>> +static void hisi_cpa_pmu_disable_pm(struct hisi_pmu *cpa_pmu)
>>>
>>> this seems unique for this new driver - why do we need to disable PM?
>>>
>> CPA PMU doesn't work under power mangement state, so we need to 
>> disable PM before using CPA PMU.
> 
> OK, so you need to disable something related to PM to use the HW. But 
> why do it when you enable the counter? I mean, can we just do it once 
> when we probe the HW? Why is it even enabled at all?
> 
>>

PM is enabled by default. yes, we could disable it in probe function and 
restore in remove function, I'll change this in next version.

Thanks,
Qi
>>>> +{
>>>> +    u32 val;
>>>> +
>>>> +    val = readl(cpa_pmu->base + CPA_CFG_REG);
>>>> +    val |= CPA_PM_CTRL;
>>>> +    writel(val, cpa_pmu->base + CPA_CFG_REG);
>>>> +}
>>>> +
>>>> +static void hisi_cpa_pmu_enable_pm(struct hisi_pmu *cpa_pmu)
>>>> +{
>>>> +    u32 val;
>>>> +
>>>> +    val = readl(cpa_pmu->base + CPA_CFG_REG);
>>>> +    val &= ~CPA_PM_CTRL;
>>>
>>> nit: you use () in hisi_cpa_pmu_stop_counters(), but not here, so 
>>> please be consistent
>>>
>> ]
> 
> ...
> 
>>>> +static int hisi_cpa_pmu_init_data(struct platform_device *pdev,
>>>> +                  struct hisi_pmu *cpa_pmu)
>>>> +{
>>>> +    if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
>>>> +                     &cpa_pmu->sccl_id)) {
>>>> +        dev_err(&pdev->dev, "Can not read cpa_pmu sccl-id\n");
>>>
>>> strange that the FW uses "scl-id" but driver uses sccl_id (I am 
>>> talking about sccl vs scl difference)
>>>
>> yes... these two names means the same thing, maybe could use a cleanup 
>> patch to keep them consistent.
>>
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    cpa_pmu->ccl_id = -1;
>>>> +
>>>> +    if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
>>>> +                     &cpa_pmu->index_id)) {
>>>> +        dev_err(&pdev->dev, "Cannot read idx-id\n");
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    cpa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
>>>> +    if (IS_ERR(cpa_pmu->base))
>>>> +        return PTR_ERR(cpa_pmu->base);
>>>> +
>>>> +    cpa_pmu->identifier = readl(cpa_pmu->base + CPA_VERSION);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
> 
> ...
> 
>>>> +static int hisi_cpa_pmu_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct hisi_pmu *cpa_pmu;
>>>> +    char *name;
>>>> +    int ret;
>>>> +
>>>> +    cpa_pmu = devm_kzalloc(&pdev->dev, sizeof(*cpa_pmu), GFP_KERNEL);
>>>> +    if (!cpa_pmu)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    ret = hisi_cpa_pmu_dev_probe(pdev, cpa_pmu);
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
>>>> +                       &cpa_pmu->node);
>>>> +    if (ret) {
>>>> +        dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    name = devm_kasprintf(&pdev->dev, GFP_KERNEL, 
>>>> "hisi_sicl%d_cpa%u", cpa_pmu->sccl_id - 1,
>>>
>>> sorry, but I still don't like this "- 1". From checking the chat on 
>>> v1, my impression was that you agreed with me on this one.
>>>
>>
>> sorry, maybe I didn't express it clearly.
>> CPA PMU is on IO die,
> 
> Then it is quite strange to provide the sccl id and not some sicl id 
> (since it is in the IO die)
> 
>   but BIOS give CPA PMU driver the sccl-id of
>> adjacent CPU die (as we need this sccl-id to pass 
>> hisi_pmu_cpu_is_associated_pmu() and find the associated cpu).
> 
> 
> Is the PMU only ever used by one SCCL? Or can more than one SCCL access? 
> If so, is there a special affinity to one particular SCCL?
> 
>> so, when naming CPA PMU, we use the sccl-id supported by BIOS to get 
>> the sicl-id of IO die which CPA PMU located in, that is: 
>> cpa_pmu->sccl_id - 1.
>>
>>>> +                  cpa_pmu->index_id);
> 
> Thanks,
> John
> .

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