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

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

[Qi Liu]

Add Support for HiSilicon CPA PMU driver. Json file in perf tool
is applied in 
https://lore.kernel.org/linux-arm-kernel/YhkhSZp+a715Ldzr@kernel.org/.

Change since v2:
- Associate CPA PMUs with CPUs online, rather than CPUs in the nearest SCCL.
- Move CPA power management to probe and remove function.
- Address the comments from John.
- Link: https://lore.kernel.org/linux-arm-kernel/20220224111129.41416-1-liuqi115@huawei.com/

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: Associate PMUs in SICL with CPUs online
  drivers/perf: hisi: Add Support for CPA PMU

 drivers/perf/hisilicon/Makefile              |   2 +-
 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c | 400 +++++++++++++++++++
 drivers/perf/hisilicon/hisi_uncore_pa_pmu.c  |  18 +-
 drivers/perf/hisilicon/hisi_uncore_pmu.c     |   4 +
 drivers/perf/hisilicon/hisi_uncore_pmu.h     |   1 +
 include/linux/cpuhotplug.h                   |   1 +
 6 files changed, 414 insertions(+), 12 deletions(-)
 create mode 100644 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c

-- 
2.24.0

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

[Qi Liu]

Add Support for HiSilicon CPA PMU driver. Json file in perf tool
is applied in 
https://lore.kernel.org/linux-arm-kernel/YhkhSZp+a715Ldzr@kernel.org/.

Change since v2:
- Associate CPA PMUs with CPUs online, rather than CPUs in the nearest SCCL.
- Move CPA power management to probe and remove function.
- Address the comments from John.
- Link: https://lore.kernel.org/linux-arm-kernel/20220224111129.41416-1-liuqi115@huawei.com/

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: Associate PMUs in SICL with CPUs online
  drivers/perf: hisi: Add Support for CPA PMU

 drivers/perf/hisilicon/Makefile              |   2 +-
 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c | 400 +++++++++++++++++++
 drivers/perf/hisilicon/hisi_uncore_pa_pmu.c  |  18 +-
 drivers/perf/hisilicon/hisi_uncore_pmu.c     |   4 +
 drivers/perf/hisilicon/hisi_uncore_pmu.h     |   1 +
 include/linux/cpuhotplug.h                   |   1 +
 6 files changed, 414 insertions(+), 12 deletions(-)
 create mode 100644 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c

-- 
2.24.0


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

[PATCH v3 1/2] drivers/perf: hisi: Associate PMUs in SICL with CPUs online

[Qi Liu]

If a PMU is in SICL, we associate it with all CPUs online, rather
than CPUs in the nearest SCCL.

Signed-off-by: Qi Liu <liuqi115@huawei.com>
---
 drivers/perf/hisilicon/hisi_uncore_pa_pmu.c | 18 +++++++-----------
 drivers/perf/hisilicon/hisi_uncore_pmu.c    |  4 ++++
 drivers/perf/hisilicon/hisi_uncore_pmu.h    |  1 +
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
index bad99d149172..54c604c0d404 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
@@ -258,13 +258,12 @@ static int hisi_pa_pmu_init_data(struct platform_device *pdev,
 				   struct hisi_pmu *pa_pmu)
 {
 	/*
-	 * Use the SCCL_ID and the index ID to identify the PA PMU,
-	 * while SCCL_ID is the nearst SCCL_ID from this SICL and
-	 * CPU core is chosen from this SCCL to manage this PMU.
+	 * As PA PMU is in SICL, use the SICL_ID and the index ID
+	 * to identify the PA PMU.
 	 */
 	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
-				     &pa_pmu->sccl_id)) {
-		dev_err(&pdev->dev, "Cannot read sccl-id!\n");
+				     &pa_pmu->sicl_id)) {
+		dev_err(&pdev->dev, "Cannot read sicl-id!\n");
 		return -EINVAL;
 	}
 
@@ -275,6 +274,7 @@ static int hisi_pa_pmu_init_data(struct platform_device *pdev,
 	}
 
 	pa_pmu->ccl_id = -1;
+	pa_pmu->sccl_id = -1;
 
 	pa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pa_pmu->base)) {
@@ -399,13 +399,9 @@ static int hisi_pa_pmu_probe(struct platform_device *pdev)
 	ret = hisi_pa_pmu_dev_probe(pdev, pa_pmu);
 	if (ret)
 		return ret;
-	/*
-	 * PA is attached in SICL and the CPU core is chosen to manage this
-	 * PMU which is the nearest SCCL, while its SCCL_ID is greater than
-	 * one with the SICL_ID.
-	 */
+
 	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%u_pa%u",
-			      pa_pmu->sccl_id - 1, pa_pmu->index_id);
+			      pa_pmu->sicl_id, pa_pmu->index_id);
 	if (!name)
 		return -ENOMEM;
 
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
index a738aeab5c04..31930166c34b 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
@@ -458,6 +458,10 @@ static bool hisi_pmu_cpu_is_associated_pmu(struct hisi_pmu *hisi_pmu)
 {
 	int sccl_id, ccl_id;
 
+	/* If SCCL_ID is -1, the PMU is in SICL and can be associated this PMU with any CPU */
+	if (hisi_pmu->sccl_id == -1)
+		return true;
+
 	if (hisi_pmu->ccl_id == -1) {
 		/* If CCL_ID is -1, the PMU only shares the same SCCL */
 		hisi_read_sccl_and_ccl_id(&sccl_id, NULL);
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h
index 7f5841d6f592..96eeddad55ff 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pmu.h
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
@@ -81,6 +81,7 @@ struct hisi_pmu {
 	struct device *dev;
 	struct hlist_node node;
 	int sccl_id;
+	int sicl_id;
 	int ccl_id;
 	void __iomem *base;
 	/* the ID of the PMU modules */
-- 
2.24.0

[PATCH v3 1/2] drivers/perf: hisi: Associate PMUs in SICL with CPUs online

[Qi Liu]

If a PMU is in SICL, we associate it with all CPUs online, rather
than CPUs in the nearest SCCL.

Signed-off-by: Qi Liu <liuqi115@huawei.com>
---
 drivers/perf/hisilicon/hisi_uncore_pa_pmu.c | 18 +++++++-----------
 drivers/perf/hisilicon/hisi_uncore_pmu.c    |  4 ++++
 drivers/perf/hisilicon/hisi_uncore_pmu.h    |  1 +
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
index bad99d149172..54c604c0d404 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
@@ -258,13 +258,12 @@ static int hisi_pa_pmu_init_data(struct platform_device *pdev,
 				   struct hisi_pmu *pa_pmu)
 {
 	/*
-	 * Use the SCCL_ID and the index ID to identify the PA PMU,
-	 * while SCCL_ID is the nearst SCCL_ID from this SICL and
-	 * CPU core is chosen from this SCCL to manage this PMU.
+	 * As PA PMU is in SICL, use the SICL_ID and the index ID
+	 * to identify the PA PMU.
 	 */
 	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
-				     &pa_pmu->sccl_id)) {
-		dev_err(&pdev->dev, "Cannot read sccl-id!\n");
+				     &pa_pmu->sicl_id)) {
+		dev_err(&pdev->dev, "Cannot read sicl-id!\n");
 		return -EINVAL;
 	}
 
@@ -275,6 +274,7 @@ static int hisi_pa_pmu_init_data(struct platform_device *pdev,
 	}
 
 	pa_pmu->ccl_id = -1;
+	pa_pmu->sccl_id = -1;
 
 	pa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pa_pmu->base)) {
@@ -399,13 +399,9 @@ static int hisi_pa_pmu_probe(struct platform_device *pdev)
 	ret = hisi_pa_pmu_dev_probe(pdev, pa_pmu);
 	if (ret)
 		return ret;
-	/*
-	 * PA is attached in SICL and the CPU core is chosen to manage this
-	 * PMU which is the nearest SCCL, while its SCCL_ID is greater than
-	 * one with the SICL_ID.
-	 */
+
 	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%u_pa%u",
-			      pa_pmu->sccl_id - 1, pa_pmu->index_id);
+			      pa_pmu->sicl_id, pa_pmu->index_id);
 	if (!name)
 		return -ENOMEM;
 
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
index a738aeab5c04..31930166c34b 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
@@ -458,6 +458,10 @@ static bool hisi_pmu_cpu_is_associated_pmu(struct hisi_pmu *hisi_pmu)
 {
 	int sccl_id, ccl_id;
 
+	/* If SCCL_ID is -1, the PMU is in SICL and can be associated this PMU with any CPU */
+	if (hisi_pmu->sccl_id == -1)
+		return true;
+
 	if (hisi_pmu->ccl_id == -1) {
 		/* If CCL_ID is -1, the PMU only shares the same SCCL */
 		hisi_read_sccl_and_ccl_id(&sccl_id, NULL);
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h
index 7f5841d6f592..96eeddad55ff 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pmu.h
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
@@ -81,6 +81,7 @@ struct hisi_pmu {
 	struct device *dev;
 	struct hlist_node node;
 	int sccl_id;
+	int sicl_id;
 	int ccl_id;
 	void __iomem *base;
 	/* the ID of the PMU modules */
-- 
2.24.0


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

[PATCH v3 2/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 8 PMU counters and interrupt is supported to handle counter
overflow. Let's support its driver under the framework of HiSilicon PMU
driver.

Signed-off-by: Qi Liu <liuqi115@huawei.com>
---
 drivers/perf/hisilicon/Makefile              |   2 +-
 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c | 400 +++++++++++++++++++
 include/linux/cpuhotplug.h                   |   1 +
 3 files changed, 402 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..3f1d9c3f99fe
--- /dev/null
+++ b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
@@ -0,0 +1,400 @@
+// 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);
+}
+
+static void hisi_cpa_pmu_disable_counter(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	/* 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->sicl_id)) {
+		dev_err(&pdev->dev, "Can not read cpa_pmu sicl-id\n");
+		return -EINVAL;
+	}
+
+	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->ccl_id = -1;
+	cpa_pmu->sccl_id = -1;
+	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->sicl_id,
+			      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, "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);
+
+	/* Power Management should be disabled before using CPA PMU. */
+	hisi_cpa_pmu_disable_pm(cpa_pmu);
+	return ret;
+}
+
+static int hisi_cpa_pmu_remove(struct platform_device *pdev)
+{
+	struct hisi_pmu *cpa_pmu = platform_get_drvdata(pdev);
+
+	hisi_cpa_pmu_enable_pm(cpa_pmu);
+	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 v3 2/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 8 PMU counters and interrupt is supported to handle counter
overflow. Let's support its driver under the framework of HiSilicon PMU
driver.

Signed-off-by: Qi Liu <liuqi115@huawei.com>
---
 drivers/perf/hisilicon/Makefile              |   2 +-
 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c | 400 +++++++++++++++++++
 include/linux/cpuhotplug.h                   |   1 +
 3 files changed, 402 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..3f1d9c3f99fe
--- /dev/null
+++ b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
@@ -0,0 +1,400 @@
+// 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);
+}
+
+static void hisi_cpa_pmu_disable_counter(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
+{
+	u32 val;
+
+	/* 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->sicl_id)) {
+		dev_err(&pdev->dev, "Can not read cpa_pmu sicl-id\n");
+		return -EINVAL;
+	}
+
+	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->ccl_id = -1;
+	cpa_pmu->sccl_id = -1;
+	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->sicl_id,
+			      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, "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);
+
+	/* Power Management should be disabled before using CPA PMU. */
+	hisi_cpa_pmu_disable_pm(cpa_pmu);
+	return ret;
+}
+
+static int hisi_cpa_pmu_remove(struct platform_device *pdev)
+{
+	struct hisi_pmu *cpa_pmu = platform_get_drvdata(pdev);
+
+	hisi_cpa_pmu_enable_pm(cpa_pmu);
+	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

Re: [PATCH v3 1/2] drivers/perf: hisi: Associate PMUs in SICL with CPUs online

[John Garry]

On 21/03/2022 07:01, Qi Liu wrote:
> If a PMU is in SICL, we associate it with all CPUs online, rather

/s/in SICL/in a SICL/ [and all other places, including the code change], 
or maybe it is better mention "IO super cluster" as well for people who 
would not know or remember.

> than CPUs in the nearest SCCL.

You are not really saying why. I would mention that we do it as it is 
not appropiate to associate with a CPU die, and you can also mention 
problems that associating with a SICL creates.

Please also mention how changing FW definition is ok at this stage.

> 
> Signed-off-by: Qi Liu <liuqi115@huawei.com>
> ---
>   drivers/perf/hisilicon/hisi_uncore_pa_pmu.c | 18 +++++++-----------
>   drivers/perf/hisilicon/hisi_uncore_pmu.c    |  4 ++++
>   drivers/perf/hisilicon/hisi_uncore_pmu.h    |  1 +
>   3 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
> index bad99d149172..54c604c0d404 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
> @@ -258,13 +258,12 @@ static int hisi_pa_pmu_init_data(struct platform_device *pdev,
>   				   struct hisi_pmu *pa_pmu)
>   {
>   	/*
> -	 * Use the SCCL_ID and the index ID to identify the PA PMU,
> -	 * while SCCL_ID is the nearst SCCL_ID from this SICL and
> -	 * CPU core is chosen from this SCCL to manage this PMU.
> +	 * As PA PMU is in SICL, use the SICL_ID and the index ID

"a SICL"

> +	 * to identify the PA PMU.
>   	 */
>   	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
> -				     &pa_pmu->sccl_id)) {
> -		dev_err(&pdev->dev, "Cannot read sccl-id!\n");
> +				     &pa_pmu->sicl_id)) {
> +		dev_err(&pdev->dev, "Cannot read sicl-id!\n");
>   		return -EINVAL;
>   	}
>   
> @@ -275,6 +274,7 @@ static int hisi_pa_pmu_init_data(struct platform_device *pdev,
>   	}
>   
>   	pa_pmu->ccl_id = -1;
> +	pa_pmu->sccl_id = -1;
>   
>   	pa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
>   	if (IS_ERR(pa_pmu->base)) {
> @@ -399,13 +399,9 @@ static int hisi_pa_pmu_probe(struct platform_device *pdev)
>   	ret = hisi_pa_pmu_dev_probe(pdev, pa_pmu);
>   	if (ret)
>   		return ret;
> -	/*
> -	 * PA is attached in SICL and the CPU core is chosen to manage this
> -	 * PMU which is the nearest SCCL, while its SCCL_ID is greater than
> -	 * one with the SICL_ID.
> -	 */
> +
>   	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%u_pa%u",
> -			      pa_pmu->sccl_id - 1, pa_pmu->index_id);
> +			      pa_pmu->sicl_id, pa_pmu->index_id);
>   	if (!name)
>   		return -ENOMEM;
>   
> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
> index a738aeab5c04..31930166c34b 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
> @@ -458,6 +458,10 @@ static bool hisi_pmu_cpu_is_associated_pmu(struct hisi_pmu *hisi_pmu)
>   {
>   	int sccl_id, ccl_id;
>   
> +	/* If SCCL_ID is -1, the PMU is in SICL and can be associated this PMU with any CPU */

too long, just have "The PMU is in a SICL and has no CPU affinity"

> +	if (hisi_pmu->sccl_id == -1)
> +		return true;
> +
>   	if (hisi_pmu->ccl_id == -1) {
>   		/* If CCL_ID is -1, the PMU only shares the same SCCL */
>   		hisi_read_sccl_and_ccl_id(&sccl_id, NULL);
> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h
> index 7f5841d6f592..96eeddad55ff 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.h
> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
> @@ -81,6 +81,7 @@ struct hisi_pmu {
>   	struct device *dev;
>   	struct hlist_node node;
>   	int sccl_id;
> +	int sicl_id;
>   	int ccl_id;
>   	void __iomem *base;
>   	/* the ID of the PMU modules */


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

Re: [PATCH v3 1/2] drivers/perf: hisi: Associate PMUs in SICL with CPUs online

[John Garry]

On 21/03/2022 07:01, Qi Liu wrote:
> If a PMU is in SICL, we associate it with all CPUs online, rather

/s/in SICL/in a SICL/ [and all other places, including the code change], 
or maybe it is better mention "IO super cluster" as well for people who 
would not know or remember.

> than CPUs in the nearest SCCL.

You are not really saying why. I would mention that we do it as it is 
not appropiate to associate with a CPU die, and you can also mention 
problems that associating with a SICL creates.

Please also mention how changing FW definition is ok at this stage.

> 
> Signed-off-by: Qi Liu <liuqi115@huawei.com>
> ---
>   drivers/perf/hisilicon/hisi_uncore_pa_pmu.c | 18 +++++++-----------
>   drivers/perf/hisilicon/hisi_uncore_pmu.c    |  4 ++++
>   drivers/perf/hisilicon/hisi_uncore_pmu.h    |  1 +
>   3 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
> index bad99d149172..54c604c0d404 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
> @@ -258,13 +258,12 @@ static int hisi_pa_pmu_init_data(struct platform_device *pdev,
>   				   struct hisi_pmu *pa_pmu)
>   {
>   	/*
> -	 * Use the SCCL_ID and the index ID to identify the PA PMU,
> -	 * while SCCL_ID is the nearst SCCL_ID from this SICL and
> -	 * CPU core is chosen from this SCCL to manage this PMU.
> +	 * As PA PMU is in SICL, use the SICL_ID and the index ID

"a SICL"

> +	 * to identify the PA PMU.
>   	 */
>   	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
> -				     &pa_pmu->sccl_id)) {
> -		dev_err(&pdev->dev, "Cannot read sccl-id!\n");
> +				     &pa_pmu->sicl_id)) {
> +		dev_err(&pdev->dev, "Cannot read sicl-id!\n");
>   		return -EINVAL;
>   	}
>   
> @@ -275,6 +274,7 @@ static int hisi_pa_pmu_init_data(struct platform_device *pdev,
>   	}
>   
>   	pa_pmu->ccl_id = -1;
> +	pa_pmu->sccl_id = -1;
>   
>   	pa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
>   	if (IS_ERR(pa_pmu->base)) {
> @@ -399,13 +399,9 @@ static int hisi_pa_pmu_probe(struct platform_device *pdev)
>   	ret = hisi_pa_pmu_dev_probe(pdev, pa_pmu);
>   	if (ret)
>   		return ret;
> -	/*
> -	 * PA is attached in SICL and the CPU core is chosen to manage this
> -	 * PMU which is the nearest SCCL, while its SCCL_ID is greater than
> -	 * one with the SICL_ID.
> -	 */
> +
>   	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%u_pa%u",
> -			      pa_pmu->sccl_id - 1, pa_pmu->index_id);
> +			      pa_pmu->sicl_id, pa_pmu->index_id);
>   	if (!name)
>   		return -ENOMEM;
>   
> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
> index a738aeab5c04..31930166c34b 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
> @@ -458,6 +458,10 @@ static bool hisi_pmu_cpu_is_associated_pmu(struct hisi_pmu *hisi_pmu)
>   {
>   	int sccl_id, ccl_id;
>   
> +	/* If SCCL_ID is -1, the PMU is in SICL and can be associated this PMU with any CPU */

too long, just have "The PMU is in a SICL and has no CPU affinity"

> +	if (hisi_pmu->sccl_id == -1)
> +		return true;
> +
>   	if (hisi_pmu->ccl_id == -1) {
>   		/* If CCL_ID is -1, the PMU only shares the same SCCL */
>   		hisi_read_sccl_and_ccl_id(&sccl_id, NULL);
> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h
> index 7f5841d6f592..96eeddad55ff 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.h
> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
> @@ -81,6 +81,7 @@ struct hisi_pmu {
>   	struct device *dev;
>   	struct hlist_node node;
>   	int sccl_id;
> +	int sicl_id;
>   	int ccl_id;
>   	void __iomem *base;
>   	/* the ID of the PMU modules */

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

[John Garry]

On 21/03/2022 07:01, Qi Liu wrote:
> On HiSilicon Hip09 platform, there is a CPA(Coherency Protocol Agent) on

leave whitespace before '('

> each SICL (Super I/O Cluster) which implements packet format translation,
> route parsing and traffic statistic.

statistics

> 
> CPA PMU has 8 PMU counters and interrupt is supported to handle counter
> overflow. Let's support its driver under the framework of HiSilicon PMU
> driver.
> 

Generally this looks ok, just some small comments to consider.

Thanks,
John

> Signed-off-by: Qi Liu <liuqi115@huawei.com>
> ---
>   drivers/perf/hisilicon/Makefile              |   2 +-
>   drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c | 400 +++++++++++++++++++
>   include/linux/cpuhotplug.h                   |   1 +
>   3 files changed, 402 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..3f1d9c3f99fe
> --- /dev/null
> +++ b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
> @@ -0,0 +1,400 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * HiSilicon SoC CPA(Coherency Protocol Agent) uncore hardware event counters support

I am not sure why we even mention uncore for something which is not in a 
SCCL

> + *
> + * 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);
> +}
> +
> +static void hisi_cpa_pmu_disable_counter(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
> +{
> +	u32 val;
> +
> +	/* 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->sicl_id)) {
> +		dev_err(&pdev->dev, "Can not read cpa_pmu sicl-id\n");

I don't think that you need to mention "cpa_pmu"

> +		return -EINVAL;
> +	}
> +
> +	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->ccl_id = -1;
> +	cpa_pmu->sccl_id = -1;
> +	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->sicl_id,
> +			      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, "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);
> +
> +	/* Power Management should be disabled before using CPA PMU. */
> +	hisi_cpa_pmu_disable_pm(cpa_pmu);

maybe you can move this earlier in the probe, before registering

> +	return ret;
> +}
> +
> +static int hisi_cpa_pmu_remove(struct platform_device *pdev)
> +{
> +	struct hisi_pmu *cpa_pmu = platform_get_drvdata(pdev);
> +
> +	hisi_cpa_pmu_enable_pm(cpa_pmu);

why do we do this?

> +	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);

you don't mention failed

And mostly we use lowercase for the driver name and maybe consider using 
pr_fmt

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


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

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

[John Garry]

On 21/03/2022 07:01, Qi Liu wrote:
> On HiSilicon Hip09 platform, there is a CPA(Coherency Protocol Agent) on

leave whitespace before '('

> each SICL (Super I/O Cluster) which implements packet format translation,
> route parsing and traffic statistic.

statistics

> 
> CPA PMU has 8 PMU counters and interrupt is supported to handle counter
> overflow. Let's support its driver under the framework of HiSilicon PMU
> driver.
> 

Generally this looks ok, just some small comments to consider.

Thanks,
John

> Signed-off-by: Qi Liu <liuqi115@huawei.com>
> ---
>   drivers/perf/hisilicon/Makefile              |   2 +-
>   drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c | 400 +++++++++++++++++++
>   include/linux/cpuhotplug.h                   |   1 +
>   3 files changed, 402 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..3f1d9c3f99fe
> --- /dev/null
> +++ b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
> @@ -0,0 +1,400 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * HiSilicon SoC CPA(Coherency Protocol Agent) uncore hardware event counters support

I am not sure why we even mention uncore for something which is not in a 
SCCL

> + *
> + * 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);
> +}
> +
> +static void hisi_cpa_pmu_disable_counter(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
> +{
> +	u32 val;
> +
> +	/* 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->sicl_id)) {
> +		dev_err(&pdev->dev, "Can not read cpa_pmu sicl-id\n");

I don't think that you need to mention "cpa_pmu"

> +		return -EINVAL;
> +	}
> +
> +	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->ccl_id = -1;
> +	cpa_pmu->sccl_id = -1;
> +	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->sicl_id,
> +			      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, "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);
> +
> +	/* Power Management should be disabled before using CPA PMU. */
> +	hisi_cpa_pmu_disable_pm(cpa_pmu);

maybe you can move this earlier in the probe, before registering

> +	return ret;
> +}
> +
> +static int hisi_cpa_pmu_remove(struct platform_device *pdev)
> +{
> +	struct hisi_pmu *cpa_pmu = platform_get_drvdata(pdev);
> +
> +	hisi_cpa_pmu_enable_pm(cpa_pmu);

why do we do this?

> +	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);

you don't mention failed

And mostly we use lowercase for the driver name and maybe consider using 
pr_fmt

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

Re: [PATCH v3 1/2] drivers/perf: hisi: Associate PMUs in SICL with CPUs online

[liuqi (BA)]

Hi John,

Thanks for the comments. some replies inline.

On 2022/4/5 16:18, John Garry wrote:
> On 21/03/2022 07:01, Qi Liu wrote:
>> If a PMU is in SICL, we associate it with all CPUs online, rather
> 
> /s/in SICL/in a SICL/ [and all other places, including the code change], 
> or maybe it is better mention "IO super cluster" as well for people who 
> would not know or remember.
> 
got it, will change these description, thanks.

>> than CPUs in the nearest SCCL.
> 
> You are not really saying why. I would mention that we do it as it is 
> not appropiate to associate with a CPU die, and you can also mention 
> problems that associating with a SICL creates.
> 
> Please also mention how changing FW definition is ok at this stage.

ok, I'll metion this next time.
> 
>>
>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
>> ---
>>   drivers/perf/hisilicon/hisi_uncore_pa_pmu.c | 18 +++++++-----------
>>   drivers/perf/hisilicon/hisi_uncore_pmu.c    |  4 ++++
>>   drivers/perf/hisilicon/hisi_uncore_pmu.h    |  1 +
>>   3 files changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c 
>> b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
>> index bad99d149172..54c604c0d404 100644
>> --- a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
>> +++ b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
>> @@ -258,13 +258,12 @@ static int hisi_pa_pmu_init_data(struct 
>> platform_device *pdev,
>>                      struct hisi_pmu *pa_pmu)
>>   {
>>       /*
>> -     * Use the SCCL_ID and the index ID to identify the PA PMU,
>> -     * while SCCL_ID is the nearst SCCL_ID from this SICL and
>> -     * CPU core is chosen from this SCCL to manage this PMU.
>> +     * As PA PMU is in SICL, use the SICL_ID and the index ID
> 
> "a SICL"

got it.
> 
>> +     * to identify the PA PMU.
>>        */
>>       if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
>> -                     &pa_pmu->sccl_id)) {
>> -        dev_err(&pdev->dev, "Cannot read sccl-id!\n");
>> +                     &pa_pmu->sicl_id)) {
>> +        dev_err(&pdev->dev, "Cannot read sicl-id!\n");
>>           return -EINVAL;
>>       }
>> @@ -275,6 +274,7 @@ static int hisi_pa_pmu_init_data(struct 
>> platform_device *pdev,
>>       }
>>       pa_pmu->ccl_id = -1;
>> +    pa_pmu->sccl_id = -1;
>>       pa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
>>       if (IS_ERR(pa_pmu->base)) {
>> @@ -399,13 +399,9 @@ static int hisi_pa_pmu_probe(struct 
>> platform_device *pdev)
>>       ret = hisi_pa_pmu_dev_probe(pdev, pa_pmu);
>>       if (ret)
>>           return ret;
>> -    /*
>> -     * PA is attached in SICL and the CPU core is chosen to manage this
>> -     * PMU which is the nearest SCCL, while its SCCL_ID is greater than
>> -     * one with the SICL_ID.
>> -     */
>> +
>>       name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%u_pa%u",
>> -                  pa_pmu->sccl_id - 1, pa_pmu->index_id);
>> +                  pa_pmu->sicl_id, pa_pmu->index_id);
>>       if (!name)
>>           return -ENOMEM;
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c 
>> b/drivers/perf/hisilicon/hisi_uncore_pmu.c
>> index a738aeab5c04..31930166c34b 100644
>> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
>> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
>> @@ -458,6 +458,10 @@ static bool hisi_pmu_cpu_is_associated_pmu(struct 
>> hisi_pmu *hisi_pmu)
>>   {
>>       int sccl_id, ccl_id;
>> +    /* If SCCL_ID is -1, the PMU is in SICL and can be associated 
>> this PMU with any CPU */
> 
> too long, just have "The PMU is in a SICL and has no CPU affinity"

ok, will fix this.

Thanks,
Qi
> 
>> +    if (hisi_pmu->sccl_id == -1)
>> +        return true;
>> +
>>       if (hisi_pmu->ccl_id == -1) {
>>           /* If CCL_ID is -1, the PMU only shares the same SCCL */
>>           hisi_read_sccl_and_ccl_id(&sccl_id, NULL);
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h 
>> b/drivers/perf/hisilicon/hisi_uncore_pmu.h
>> index 7f5841d6f592..96eeddad55ff 100644
>> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.h
>> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
>> @@ -81,6 +81,7 @@ struct hisi_pmu {
>>       struct device *dev;
>>       struct hlist_node node;
>>       int sccl_id;
>> +    int sicl_id;
>>       int ccl_id;
>>       void __iomem *base;
>>       /* the ID of the PMU modules */
> 
> .

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

Re: [PATCH v3 1/2] drivers/perf: hisi: Associate PMUs in SICL with CPUs online

[liuqi (BA)]

Hi John,

Thanks for the comments. some replies inline.

On 2022/4/5 16:18, John Garry wrote:
> On 21/03/2022 07:01, Qi Liu wrote:
>> If a PMU is in SICL, we associate it with all CPUs online, rather
> 
> /s/in SICL/in a SICL/ [and all other places, including the code change], 
> or maybe it is better mention "IO super cluster" as well for people who 
> would not know or remember.
> 
got it, will change these description, thanks.

>> than CPUs in the nearest SCCL.
> 
> You are not really saying why. I would mention that we do it as it is 
> not appropiate to associate with a CPU die, and you can also mention 
> problems that associating with a SICL creates.
> 
> Please also mention how changing FW definition is ok at this stage.

ok, I'll metion this next time.
> 
>>
>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
>> ---
>>   drivers/perf/hisilicon/hisi_uncore_pa_pmu.c | 18 +++++++-----------
>>   drivers/perf/hisilicon/hisi_uncore_pmu.c    |  4 ++++
>>   drivers/perf/hisilicon/hisi_uncore_pmu.h    |  1 +
>>   3 files changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c 
>> b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
>> index bad99d149172..54c604c0d404 100644
>> --- a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
>> +++ b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
>> @@ -258,13 +258,12 @@ static int hisi_pa_pmu_init_data(struct 
>> platform_device *pdev,
>>                      struct hisi_pmu *pa_pmu)
>>   {
>>       /*
>> -     * Use the SCCL_ID and the index ID to identify the PA PMU,
>> -     * while SCCL_ID is the nearst SCCL_ID from this SICL and
>> -     * CPU core is chosen from this SCCL to manage this PMU.
>> +     * As PA PMU is in SICL, use the SICL_ID and the index ID
> 
> "a SICL"

got it.
> 
>> +     * to identify the PA PMU.
>>        */
>>       if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
>> -                     &pa_pmu->sccl_id)) {
>> -        dev_err(&pdev->dev, "Cannot read sccl-id!\n");
>> +                     &pa_pmu->sicl_id)) {
>> +        dev_err(&pdev->dev, "Cannot read sicl-id!\n");
>>           return -EINVAL;
>>       }
>> @@ -275,6 +274,7 @@ static int hisi_pa_pmu_init_data(struct 
>> platform_device *pdev,
>>       }
>>       pa_pmu->ccl_id = -1;
>> +    pa_pmu->sccl_id = -1;
>>       pa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
>>       if (IS_ERR(pa_pmu->base)) {
>> @@ -399,13 +399,9 @@ static int hisi_pa_pmu_probe(struct 
>> platform_device *pdev)
>>       ret = hisi_pa_pmu_dev_probe(pdev, pa_pmu);
>>       if (ret)
>>           return ret;
>> -    /*
>> -     * PA is attached in SICL and the CPU core is chosen to manage this
>> -     * PMU which is the nearest SCCL, while its SCCL_ID is greater than
>> -     * one with the SICL_ID.
>> -     */
>> +
>>       name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%u_pa%u",
>> -                  pa_pmu->sccl_id - 1, pa_pmu->index_id);
>> +                  pa_pmu->sicl_id, pa_pmu->index_id);
>>       if (!name)
>>           return -ENOMEM;
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c 
>> b/drivers/perf/hisilicon/hisi_uncore_pmu.c
>> index a738aeab5c04..31930166c34b 100644
>> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
>> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
>> @@ -458,6 +458,10 @@ static bool hisi_pmu_cpu_is_associated_pmu(struct 
>> hisi_pmu *hisi_pmu)
>>   {
>>       int sccl_id, ccl_id;
>> +    /* If SCCL_ID is -1, the PMU is in SICL and can be associated 
>> this PMU with any CPU */
> 
> too long, just have "The PMU is in a SICL and has no CPU affinity"

ok, will fix this.

Thanks,
Qi
> 
>> +    if (hisi_pmu->sccl_id == -1)
>> +        return true;
>> +
>>       if (hisi_pmu->ccl_id == -1) {
>>           /* If CCL_ID is -1, the PMU only shares the same SCCL */
>>           hisi_read_sccl_and_ccl_id(&sccl_id, NULL);
>> diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h 
>> b/drivers/perf/hisilicon/hisi_uncore_pmu.h
>> index 7f5841d6f592..96eeddad55ff 100644
>> --- a/drivers/perf/hisilicon/hisi_uncore_pmu.h
>> +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
>> @@ -81,6 +81,7 @@ struct hisi_pmu {
>>       struct device *dev;
>>       struct hlist_node node;
>>       int sccl_id;
>> +    int sicl_id;
>>       int ccl_id;
>>       void __iomem *base;
>>       /* the ID of the PMU modules */
> 
> .

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

[liuqi (BA)]

Hi John,

Thanks for the comments. some replies inline.

On 2022/4/5 16:28, John Garry wrote:
> On 21/03/2022 07:01, Qi Liu wrote:
>> On HiSilicon Hip09 platform, there is a CPA(Coherency Protocol Agent) on
> 
> leave whitespace before '('
> 
ok, will fix this.
>> each SICL (Super I/O Cluster) which implements packet format translation,
>> route parsing and traffic statistic.
> 
> statistics
will fix this, thanks.
> 
>>
>> CPA PMU has 8 PMU counters and interrupt is supported to handle counter
>> overflow. Let's support its driver under the framework of HiSilicon PMU
>> driver.
>>
> 
> Generally this looks ok, just some small comments to consider.
> 
> Thanks,
> John
> 
>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
>> ---
>>   drivers/perf/hisilicon/Makefile              |   2 +-
>>   drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c | 400 +++++++++++++++++++
>>   include/linux/cpuhotplug.h                   |   1 +
>>   3 files changed, 402 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..3f1d9c3f99fe
>> --- /dev/null
>> +++ b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
>> @@ -0,0 +1,400 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * HiSilicon SoC CPA(Coherency Protocol Agent) uncore hardware event 
>> counters support
> 
> I am not sure why we even mention uncore for something which is not in a 
> SCCL
> 

we metion uncore as it use hisi_uncore_pmu framework, I'll drop this 
next time.
>> + *
>> + * 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.
>> + */
>> +
[...]
>> +
>> +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->sicl_id)) {
>> +        dev_err(&pdev->dev, "Can not read cpa_pmu sicl-id\n");
> 
> I don't think that you need to mention "cpa_pmu"

will drop it.

> 
>> +        return -EINVAL;
>> +    }
>> +
>> +    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->ccl_id = -1;
>> +    cpa_pmu->sccl_id = -1;
>> +    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,

>> +
>> +    ret = perf_pmu_register(&cpa_pmu->pmu, name, -1);
>> +    if (ret) {
>> +        dev_err(cpa_pmu->dev, "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);
>> +
>> +    /* Power Management should be disabled before using CPA PMU. */
>> +    hisi_cpa_pmu_disable_pm(cpa_pmu);
> 
> maybe you can move this earlier in the probe, before registering
> 
got it, will do this.

>> +    return ret;
>> +}
>> +
>> +static int hisi_cpa_pmu_remove(struct platform_device *pdev)
>> +{
>> +    struct hisi_pmu *cpa_pmu = platform_get_drvdata(pdev);
>> +
>> +    hisi_cpa_pmu_enable_pm(cpa_pmu);
> 
> why do we do this?

restore CPA registers after remove CPA PMU driver, as we disable pmu in 
probe function.

> 
>> +    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);
> 
> you don't mention failed
> 
> And mostly we use lowercase for the driver name and maybe consider using 
> pr_fmt
> 
got it, will fix this next time.

Thanks,
Qi
>> +        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,
> 
> .

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

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

[liuqi (BA)]

Hi John,

Thanks for the comments. some replies inline.

On 2022/4/5 16:28, John Garry wrote:
> On 21/03/2022 07:01, Qi Liu wrote:
>> On HiSilicon Hip09 platform, there is a CPA(Coherency Protocol Agent) on
> 
> leave whitespace before '('
> 
ok, will fix this.
>> each SICL (Super I/O Cluster) which implements packet format translation,
>> route parsing and traffic statistic.
> 
> statistics
will fix this, thanks.
> 
>>
>> CPA PMU has 8 PMU counters and interrupt is supported to handle counter
>> overflow. Let's support its driver under the framework of HiSilicon PMU
>> driver.
>>
> 
> Generally this looks ok, just some small comments to consider.
> 
> Thanks,
> John
> 
>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
>> ---
>>   drivers/perf/hisilicon/Makefile              |   2 +-
>>   drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c | 400 +++++++++++++++++++
>>   include/linux/cpuhotplug.h                   |   1 +
>>   3 files changed, 402 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..3f1d9c3f99fe
>> --- /dev/null
>> +++ b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
>> @@ -0,0 +1,400 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * HiSilicon SoC CPA(Coherency Protocol Agent) uncore hardware event 
>> counters support
> 
> I am not sure why we even mention uncore for something which is not in a 
> SCCL
> 

we metion uncore as it use hisi_uncore_pmu framework, I'll drop this 
next time.
>> + *
>> + * 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.
>> + */
>> +
[...]
>> +
>> +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->sicl_id)) {
>> +        dev_err(&pdev->dev, "Can not read cpa_pmu sicl-id\n");
> 
> I don't think that you need to mention "cpa_pmu"

will drop it.

> 
>> +        return -EINVAL;
>> +    }
>> +
>> +    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->ccl_id = -1;
>> +    cpa_pmu->sccl_id = -1;
>> +    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,

>> +
>> +    ret = perf_pmu_register(&cpa_pmu->pmu, name, -1);
>> +    if (ret) {
>> +        dev_err(cpa_pmu->dev, "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);
>> +
>> +    /* Power Management should be disabled before using CPA PMU. */
>> +    hisi_cpa_pmu_disable_pm(cpa_pmu);
> 
> maybe you can move this earlier in the probe, before registering
> 
got it, will do this.

>> +    return ret;
>> +}
>> +
>> +static int hisi_cpa_pmu_remove(struct platform_device *pdev)
>> +{
>> +    struct hisi_pmu *cpa_pmu = platform_get_drvdata(pdev);
>> +
>> +    hisi_cpa_pmu_enable_pm(cpa_pmu);
> 
> why do we do this?

restore CPA registers after remove CPA PMU driver, as we disable pmu in 
probe function.

> 
>> +    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);
> 
> you don't mention failed
> 
> And mostly we use lowercase for the driver name and maybe consider using 
> pr_fmt
> 
got it, will fix this next time.

Thanks,
Qi
>> +        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,
> 
> .

[PATCH v3 1/2] drivers/perf: hisi: Associate PMUs in SICL with CPUs online

[Qi Liu]

If a PMU is in a SICL (Super IO cluster), it is not appropriate to
associate this PMU with a CPU die. So we associate it with all CPUs
online.

As the firmware of Hip09 platform hasn't been published yet, change
of PMU driver will not influence backwards compatibility between
driver and firmware.

Signed-off-by: Qi Liu <liuqi115@huawei.com>
---
 drivers/perf/hisilicon/hisi_uncore_pa_pmu.c | 18 +++++++-----------
 drivers/perf/hisilicon/hisi_uncore_pmu.c    |  4 ++++
 drivers/perf/hisilicon/hisi_uncore_pmu.h    |  1 +
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
index bad99d149172..a0ee84d97c41 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
@@ -258,13 +258,12 @@ static int hisi_pa_pmu_init_data(struct platform_device *pdev,
 				   struct hisi_pmu *pa_pmu)
 {
 	/*
-	 * Use the SCCL_ID and the index ID to identify the PA PMU,
-	 * while SCCL_ID is the nearst SCCL_ID from this SICL and
-	 * CPU core is chosen from this SCCL to manage this PMU.
+	 * As PA PMU is in a SICL, use the SICL_ID and the index ID
+	 * to identify the PA PMU.
 	 */
 	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
-				     &pa_pmu->sccl_id)) {
-		dev_err(&pdev->dev, "Cannot read sccl-id!\n");
+				     &pa_pmu->sicl_id)) {
+		dev_err(&pdev->dev, "Cannot read sicl-id!\n");
 		return -EINVAL;
 	}
 
@@ -275,6 +274,7 @@ static int hisi_pa_pmu_init_data(struct platform_device *pdev,
 	}
 
 	pa_pmu->ccl_id = -1;
+	pa_pmu->sccl_id = -1;
 
 	pa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pa_pmu->base)) {
@@ -399,13 +399,9 @@ static int hisi_pa_pmu_probe(struct platform_device *pdev)
 	ret = hisi_pa_pmu_dev_probe(pdev, pa_pmu);
 	if (ret)
 		return ret;
-	/*
-	 * PA is attached in SICL and the CPU core is chosen to manage this
-	 * PMU which is the nearest SCCL, while its SCCL_ID is greater than
-	 * one with the SICL_ID.
-	 */
+
 	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%u_pa%u",
-			      pa_pmu->sccl_id - 1, pa_pmu->index_id);
+			      pa_pmu->sicl_id, pa_pmu->index_id);
 	if (!name)
 		return -ENOMEM;
 
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
index a738aeab5c04..463061fa8fc8 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
@@ -458,6 +458,10 @@ static bool hisi_pmu_cpu_is_associated_pmu(struct hisi_pmu *hisi_pmu)
 {
 	int sccl_id, ccl_id;
 
+	/* If SCCL_ID is -1, the PMU is in a SICL and has no CPU affinity */
+	if (hisi_pmu->sccl_id == -1)
+		return true;
+
 	if (hisi_pmu->ccl_id == -1) {
 		/* If CCL_ID is -1, the PMU only shares the same SCCL */
 		hisi_read_sccl_and_ccl_id(&sccl_id, NULL);
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h
index 7f5841d6f592..96eeddad55ff 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pmu.h
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
@@ -81,6 +81,7 @@ struct hisi_pmu {
 	struct device *dev;
 	struct hlist_node node;
 	int sccl_id;
+	int sicl_id;
 	int ccl_id;
 	void __iomem *base;
 	/* the ID of the PMU modules */
-- 
2.24.0

[PATCH v3 1/2] drivers/perf: hisi: Associate PMUs in SICL with CPUs online

[Qi Liu]

If a PMU is in a SICL (Super IO cluster), it is not appropriate to
associate this PMU with a CPU die. So we associate it with all CPUs
online.

As the firmware of Hip09 platform hasn't been published yet, change
of PMU driver will not influence backwards compatibility between
driver and firmware.

Signed-off-by: Qi Liu <liuqi115@huawei.com>
---
 drivers/perf/hisilicon/hisi_uncore_pa_pmu.c | 18 +++++++-----------
 drivers/perf/hisilicon/hisi_uncore_pmu.c    |  4 ++++
 drivers/perf/hisilicon/hisi_uncore_pmu.h    |  1 +
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
index bad99d149172..a0ee84d97c41 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_pa_pmu.c
@@ -258,13 +258,12 @@ static int hisi_pa_pmu_init_data(struct platform_device *pdev,
 				   struct hisi_pmu *pa_pmu)
 {
 	/*
-	 * Use the SCCL_ID and the index ID to identify the PA PMU,
-	 * while SCCL_ID is the nearst SCCL_ID from this SICL and
-	 * CPU core is chosen from this SCCL to manage this PMU.
+	 * As PA PMU is in a SICL, use the SICL_ID and the index ID
+	 * to identify the PA PMU.
 	 */
 	if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
-				     &pa_pmu->sccl_id)) {
-		dev_err(&pdev->dev, "Cannot read sccl-id!\n");
+				     &pa_pmu->sicl_id)) {
+		dev_err(&pdev->dev, "Cannot read sicl-id!\n");
 		return -EINVAL;
 	}
 
@@ -275,6 +274,7 @@ static int hisi_pa_pmu_init_data(struct platform_device *pdev,
 	}
 
 	pa_pmu->ccl_id = -1;
+	pa_pmu->sccl_id = -1;
 
 	pa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(pa_pmu->base)) {
@@ -399,13 +399,9 @@ static int hisi_pa_pmu_probe(struct platform_device *pdev)
 	ret = hisi_pa_pmu_dev_probe(pdev, pa_pmu);
 	if (ret)
 		return ret;
-	/*
-	 * PA is attached in SICL and the CPU core is chosen to manage this
-	 * PMU which is the nearest SCCL, while its SCCL_ID is greater than
-	 * one with the SICL_ID.
-	 */
+
 	name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%u_pa%u",
-			      pa_pmu->sccl_id - 1, pa_pmu->index_id);
+			      pa_pmu->sicl_id, pa_pmu->index_id);
 	if (!name)
 		return -ENOMEM;
 
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
index a738aeab5c04..463061fa8fc8 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
@@ -458,6 +458,10 @@ static bool hisi_pmu_cpu_is_associated_pmu(struct hisi_pmu *hisi_pmu)
 {
 	int sccl_id, ccl_id;
 
+	/* If SCCL_ID is -1, the PMU is in a SICL and has no CPU affinity */
+	if (hisi_pmu->sccl_id == -1)
+		return true;
+
 	if (hisi_pmu->ccl_id == -1) {
 		/* If CCL_ID is -1, the PMU only shares the same SCCL */
 		hisi_read_sccl_and_ccl_id(&sccl_id, NULL);
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h
index 7f5841d6f592..96eeddad55ff 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pmu.h
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h
@@ -81,6 +81,7 @@ struct hisi_pmu {
 	struct device *dev;
 	struct hlist_node node;
 	int sccl_id;
+	int sicl_id;
 	int ccl_id;
 	void __iomem *base;
 	/* the ID of the PMU modules */
-- 
2.24.0


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