* [PATCH 0/2] Add Support for HiSilicon CPA PMU
@ 2022-02-14 11:42 Qi Liu
2022-02-14 11:42 ` [PATCH 1/2] drivers/perf: hisi: Add Support for " Qi Liu
2022-02-14 11:42 ` [PATCH 2/2] perf jevents: Add support for HiSilicon CPA PMU aliasing Qi Liu
0 siblings, 2 replies; 10+ messages in thread
From: Qi Liu @ 2022-02-14 11:42 UTC (permalink / raw)
To: will, mark.rutland; +Cc: linux-arm-kernel, linux-kernel, linuxarm, zhangshaokun
Add Support for HiSilicon CPA PMU, including PMU driver and json
file in perf tool.
Qi Liu (2):
drivers/perf: hisi: Add Support for CPA PMU
perf jevents: Add support for HiSilicon CPA PMU aliasing
drivers/perf/hisilicon/Makefile | 2 +-
drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c | 403 ++++++++++++++++++
include/linux/cpuhotplug.h | 1 +
.../arm64/hisilicon/hip09/sys/uncore-cpa.json | 81 ++++
tools/perf/pmu-events/jevents.c | 1 +
5 files changed, 487 insertions(+), 1 deletion(-)
create mode 100644 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
create mode 100644 tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json
--
2.24.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] drivers/perf: hisi: Add Support for CPA PMU
2022-02-14 11:42 [PATCH 0/2] Add Support for HiSilicon CPA PMU Qi Liu
@ 2022-02-14 11:42 ` Qi Liu
2022-02-15 10:47 ` John Garry
2022-02-14 11:42 ` [PATCH 2/2] perf jevents: Add support for HiSilicon CPA PMU aliasing Qi Liu
1 sibling, 1 reply; 10+ messages in thread
From: Qi Liu @ 2022-02-14 11:42 UTC (permalink / raw)
To: will, mark.rutland; +Cc: linux-arm-kernel, linux-kernel, linuxarm, zhangshaokun
On HiSilicon Hip09 platform, there is a CPA(Coherency Protocol Agent) on
each SICL (Super I/O Cluster) which implements packet format translation,
route parsing and traffic statistic.
CPA PMU has separate PMU registers which the driver can program freely and
interrupt is supported to handle counter overflow. Let's support its driver
under the framework of HiSilicon uncore PMU driver.
Signed-off-by: Qi Liu <liuqi115@huawei.com>
---
drivers/perf/hisilicon/Makefile | 2 +-
drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c | 403 +++++++++++++++++++
include/linux/cpuhotplug.h | 1 +
3 files changed, 405 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..a28796e93ae1
--- /dev/null
+++ b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
@@ -0,0 +1,403 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * HiSilicon SoC CPA(Coherency Protocol Agent) uncore Hardware event counters support
+ *
+ * Copyright (C) 2022 HiSilicon Limited
+ * Author: Qi Liu <liuqi115@huawei.com>
+ *
+ * This code is based on the uncore PMUs like arm-cci and arm-ccn.
+ */
+
+#include <linux/acpi.h>
+#include <linux/bug.h>
+#include <linux/cpuhotplug.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/list.h>
+#include <linux/smp.h>
+
+#include "hisi_uncore_pmu.h"
+
+/* CPA register definition */
+#define CPA_PERF_CTRL 0x1C00
+#define CPA_EVENT_CTRL 0x1C04
+#define CPA_INT_MASK 0x1C70
+#define CPA_INT_STATUS 0x1C78
+#define CPA_INT_CLEAR 0x1C7C
+#define CPA_EVENT_TYPE0 0x1C80
+#define CPA_VERSION 0x1CF0
+#define CPA_CNT0_LOWER 0x1D00
+#define CPA_CFG_REG 0x0534
+
+/* CPA operation command */
+#define CPA_PERF_CTRL_EN BIT_ULL(0)
+#define CPA_EVTYPE_MASK 0xFFUL
+#define CPA_PM_CTRL BIT_ULL(9)
+
+/* CPA has 8-counters */
+#define CPA_NR_COUNTERS 0x8
+#define CPA_COUNTER_BITS 64
+#define CPA_NR_EVENTS 0xFF
+#define CPA_REG_OFFSET 0x8
+
+static u32 hisi_cpa_pmu_get_counter_offset(int idx)
+{
+ return (CPA_CNT0_LOWER + idx * CPA_REG_OFFSET);
+}
+
+static u64 hisi_cpa_pmu_read_counter(struct hisi_pmu *cpa_pmu,
+ struct hw_perf_event *hwc)
+{
+ return readq(cpa_pmu->base + hisi_cpa_pmu_get_counter_offset(hwc->idx));
+}
+
+static void hisi_cpa_pmu_write_counter(struct hisi_pmu *cpa_pmu,
+ struct hw_perf_event *hwc, u64 val)
+{
+ writeq(val, cpa_pmu->base + hisi_cpa_pmu_get_counter_offset(hwc->idx));
+}
+
+static void hisi_cpa_pmu_write_evtype(struct hisi_pmu *cpa_pmu, int idx, u32 type)
+{
+ u32 reg, reg_idx, shift, val;
+
+ /*
+ * Select the appropriate event select register(CPA_EVENT_TYPE0/1).
+ * There are 2 event select registers for the 8 hardware counters.
+ * Event code is 8-bits and for the former 4 hardware counters,
+ * CPA_EVENT_TYPE0 is chosen. For the latter 4 hardware counters,
+ * CPA_EVENT_TYPE1 is chosen.
+ */
+ reg = CPA_EVENT_TYPE0 + (idx / 4) * 4;
+ reg_idx = idx % 4;
+ shift = CPA_REG_OFFSET * reg_idx;
+
+ /* Write event code to CPA_EVENT_TYPEx Register */
+ val = readl(cpa_pmu->base + reg);
+ val &= ~(CPA_EVTYPE_MASK << shift);
+ val |= (type << shift);
+ writel(val, cpa_pmu->base + reg);
+}
+
+static void hisi_cpa_pmu_start_counters(struct hisi_pmu *cpa_pmu)
+{
+ u32 val;
+
+ val = readl(cpa_pmu->base + CPA_PERF_CTRL);
+ val |= CPA_PERF_CTRL_EN;
+ writel(val, cpa_pmu->base + CPA_PERF_CTRL);
+}
+
+static void hisi_cpa_pmu_stop_counters(struct hisi_pmu *cpa_pmu)
+{
+ u32 val;
+
+ val = readl(cpa_pmu->base + CPA_PERF_CTRL);
+ val &= ~(CPA_PERF_CTRL_EN);
+ writel(val, cpa_pmu->base + CPA_PERF_CTRL);
+}
+
+static void hisi_cpa_pmu_disable_pm(struct hisi_pmu *cpa_pmu)
+{
+ u32 val;
+
+ val = readl(cpa_pmu->base + CPA_CFG_REG);
+ val |= CPA_PM_CTRL;
+ writel(val, cpa_pmu->base + CPA_CFG_REG);
+}
+
+static void hisi_cpa_pmu_enable_pm(struct hisi_pmu *cpa_pmu)
+{
+ u32 val;
+
+ val = readl(cpa_pmu->base + CPA_CFG_REG);
+ val &= ~CPA_PM_CTRL;
+ writel(val, cpa_pmu->base + CPA_CFG_REG);
+}
+
+static void hisi_cpa_pmu_enable_counter(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
+{
+ u32 val;
+
+ /* Enable counter index in CPA_EVENT_CTRL register */
+ val = readl(cpa_pmu->base + CPA_EVENT_CTRL);
+ val |= 1 << hwc->idx;
+ writel(val, cpa_pmu->base + CPA_EVENT_CTRL);
+
+ /* Power management should be disabled before counting. */
+ hisi_cpa_pmu_disable_pm(cpa_pmu);
+}
+
+static void hisi_cpa_pmu_disable_counter(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
+{
+ u32 val;
+
+ hisi_cpa_pmu_enable_pm(cpa_pmu);
+
+ /* Clear counter index in CPA_EVENT_CTRL register */
+ val = readl(cpa_pmu->base + CPA_EVENT_CTRL);
+ val &= ~(1UL << hwc->idx);
+ writel(val, cpa_pmu->base + CPA_EVENT_CTRL);
+}
+
+static void hisi_cpa_pmu_enable_counter_int(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
+{
+ u32 val;
+
+ /* Write 0 to enable interrupt */
+ val = readl(cpa_pmu->base + CPA_INT_MASK);
+ val &= ~(1UL << hwc->idx);
+ writel(val, cpa_pmu->base + CPA_INT_MASK);
+}
+
+static void hisi_cpa_pmu_disable_counter_int(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
+{
+ u32 val;
+
+ /* Write 1 to mask interrupt */
+ val = readl(cpa_pmu->base + CPA_INT_MASK);
+ val |= 1 << hwc->idx;
+ writel(val, cpa_pmu->base + CPA_INT_MASK);
+}
+
+static u32 hisi_cpa_pmu_get_int_status(struct hisi_pmu *cpa_pmu)
+{
+ return readl(cpa_pmu->base + CPA_INT_STATUS);
+}
+
+static void hisi_cpa_pmu_clear_int_status(struct hisi_pmu *cpa_pmu, int idx)
+{
+ writel(1 << idx, cpa_pmu->base + CPA_INT_CLEAR);
+}
+
+static const struct acpi_device_id hisi_cpa_pmu_acpi_match[] = {
+ { "HISI0281", },
+ {}
+};
+MODULE_DEVICE_TABLE(acpi, hisi_cpa_pmu_acpi_match);
+
+static int hisi_cpa_pmu_init_data(struct platform_device *pdev,
+ struct hisi_pmu *cpa_pmu)
+{
+ if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
+ &cpa_pmu->sccl_id)) {
+ dev_err(&pdev->dev, "Can not read cpa_pmu sccl-id\n");
+ return -EINVAL;
+ }
+
+ cpa_pmu->ccl_id = -1;
+
+ if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
+ &cpa_pmu->index_id)) {
+ dev_err(&pdev->dev, "Cannot read idx-id\n");
+ return -EINVAL;
+ }
+
+ cpa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(cpa_pmu->base)) {
+ dev_err(&pdev->dev, "ioremap failed for cpa_pmu resource\n");
+ 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(p1_wr_dat, 0x61),
+ HISI_PMU_EVENT_ATTR(p1_rd_dat, 0x62),
+ HISI_PMU_EVENT_ATTR(p0_wr_dat, 0xE1),
+ HISI_PMU_EVENT_ATTR(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;
+
+ platform_set_drvdata(pdev, cpa_pmu);
+
+ ret = hisi_cpa_pmu_dev_probe(pdev, cpa_pmu);
+ if (ret)
+ return ret;
+
+ ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
+ &cpa_pmu->node);
+ if (ret) {
+ dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
+ return ret;
+ }
+
+ name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%d_cpa%u", cpa_pmu->sccl_id - 1,
+ cpa_pmu->index_id);
+
+ cpa_pmu->pmu = (struct pmu) {
+ .name = name,
+ .module = THIS_MODULE,
+ .task_ctx_nr = perf_invalid_context,
+ .event_init = hisi_uncore_pmu_event_init,
+ .pmu_enable = hisi_uncore_pmu_enable,
+ .pmu_disable = hisi_uncore_pmu_disable,
+ .add = hisi_uncore_pmu_add,
+ .del = hisi_uncore_pmu_del,
+ .start = hisi_uncore_pmu_start,
+ .stop = hisi_uncore_pmu_stop,
+ .read = hisi_uncore_pmu_read,
+ .attr_groups = cpa_pmu->pmu_events.attr_groups,
+ .capabilities = PERF_PMU_CAP_NO_EXCLUDE,
+ };
+
+ ret = perf_pmu_register(&cpa_pmu->pmu, name, -1);
+ if (ret) {
+ dev_err(cpa_pmu->dev, "CPA PMU register failed\n");
+ cpuhp_state_remove_instance_nocalls(
+ CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE, &cpa_pmu->node);
+ }
+
+ return ret;
+}
+
+static int hisi_cpa_pmu_remove(struct platform_device *pdev)
+{
+ struct hisi_pmu *cpa_pmu = platform_get_drvdata(pdev);
+
+ perf_pmu_unregister(&cpa_pmu->pmu);
+ cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
+ &cpa_pmu->node);
+ return 0;
+}
+
+static struct platform_driver hisi_cpa_pmu_driver = {
+ .driver = {
+ .name = "hisi_cpa_pmu",
+ .acpi_match_table = ACPI_PTR(hisi_cpa_pmu_acpi_match),
+ .suppress_bind_attrs = true,
+ },
+ .probe = hisi_cpa_pmu_probe,
+ .remove = hisi_cpa_pmu_remove,
+};
+
+static int __init hisi_cpa_pmu_module_init(void)
+{
+ int ret;
+
+ ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
+ "AP_PERF_ARM_HISI_CPA_ONLINE",
+ hisi_uncore_pmu_online_cpu,
+ hisi_uncore_pmu_offline_cpu);
+ if (ret) {
+ pr_err("CPA PMU: setup hotplug: %d\n", ret);
+ return ret;
+ }
+
+ ret = platform_driver_register(&hisi_cpa_pmu_driver);
+ if (ret)
+ cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE);
+
+ return ret;
+}
+module_init(hisi_cpa_pmu_module_init);
+
+static void __exit hisi_cpa_pmu_module_exit(void)
+{
+ platform_driver_unregister(&hisi_cpa_pmu_driver);
+ cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE);
+}
+module_exit(hisi_cpa_pmu_module_exit);
+
+MODULE_DESCRIPTION("HiSilicon SoC CPA uncore PMU driver");
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Qi Liu <liuqi115@huawei.com>");
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 411a428ace4d..d55063719904 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -220,6 +220,7 @@ enum cpuhp_state {
CPUHP_AP_PERF_S390_SF_ONLINE,
CPUHP_AP_PERF_ARM_CCI_ONLINE,
CPUHP_AP_PERF_ARM_CCN_ONLINE,
+ CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
CPUHP_AP_PERF_ARM_HISI_DDRC_ONLINE,
CPUHP_AP_PERF_ARM_HISI_HHA_ONLINE,
CPUHP_AP_PERF_ARM_HISI_L3_ONLINE,
--
2.24.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] perf jevents: Add support for HiSilicon CPA PMU aliasing
2022-02-14 11:42 [PATCH 0/2] Add Support for HiSilicon CPA PMU Qi Liu
2022-02-14 11:42 ` [PATCH 1/2] drivers/perf: hisi: Add Support for " Qi Liu
@ 2022-02-14 11:42 ` Qi Liu
2022-02-14 12:04 ` John Garry
2022-02-15 1:23 ` liuqi (BA)
1 sibling, 2 replies; 10+ messages in thread
From: Qi Liu @ 2022-02-14 11:42 UTC (permalink / raw)
To: will, mark.rutland; +Cc: linux-arm-kernel, linux-kernel, linuxarm, zhangshaokun
Add support for HiSilicon CPA PMU aliasing.
The kernel driver is in drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
Signed-off-by: Qi Liu <liuqi115@huawei.com>
---
.../arm64/hisilicon/hip09/sys/uncore-cpa.json | 81 +++++++++++++++++++
tools/perf/pmu-events/jevents.c | 1 +
2 files changed, 82 insertions(+)
create mode 100644 tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json
diff --git a/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json b/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json
new file mode 100644
index 000000000000..35c90da7f54b
--- /dev/null
+++ b/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json
@@ -0,0 +1,81 @@
+[
+ {
+ "ConfigCode": "0x00",
+ "EventName": "cpa_cycles",
+ "BriefDescription": "count of CPA cycles",
+ "Compat": "0x00000030",
+ "Unit": "hisi_sicl,cpa"
+ },
+ {
+ "ConfigCode": "0x61",
+ "EventName": "p1_wr_dat",
+ "BriefDescription": "Number of write ops transmitted by the P1 port",
+ "Compat": "0x00000030",
+ "Unit": "hisi_sicl,cpa"
+ },
+ {
+ "ConfigCode": "0x62",
+ "EventName": "p1_rd_dat",
+ "BriefDescription": "Number of read ops transmitted by the P1 port",
+ "Compat": "0x00000030",
+ "Unit": "hisi_sicl,cpa"
+ },
+ {
+ "ConfigCode": "0x3",
+ "EventName": "p1_rd_dat_64b",
+ "BriefDescription": "Number of read ops transmitted by the P1 port which size is 64 bytes",
+ "Compat": "0x00000030",
+ "Unit": "hisi_sicl,cpa"
+ },
+ {
+ "ConfigCode": "0x4",
+ "EventName": "p1_rd_dat_32b",
+ "BriefDescription": "Number of read ops transmitted by the P1 port which size is 32 bytes",
+ "Compat": "0x00000030",
+ "Unit": "hisi_sicl,cpa"
+ },
+ {
+ "ConfigCode": "0xE1",
+ "EventName": "p0_wr_dat",
+ "BriefDescription": "Number of write ops transmitted by the P0 port",
+ "Compat": "0x00000030",
+ "Unit": "hisi_sicl,cpa"
+ },
+ {
+ "ConfigCode": "0xE2",
+ "EventName": "p0_rd_dat",
+ "BriefDescription": "Number of read ops transmitted by the P0 port",
+ "Compat": "0x00000030",
+ "Unit": "hisi_sicl,cpa"
+ },
+ {
+ "ConfigCode": "0x83",
+ "EventName": "p0_rd_dat_64b",
+ "BriefDescription": "Number of read ops transmitted by the P0 port which size is 64 bytes",
+ "Compat": "0x00000030",
+ "Unit": "hisi_sicl,cpa"
+ },
+ {
+ "ConfigCode": "0x84",
+ "EventName": "p0_rd_dat_32b",
+ "BriefDescription": "Number of read ops transmitted by the P0 port which size is 32 bytes",
+ "Compat": "0x00000030",
+ "Unit": "hisi_sicl,cpa"
+ },
+ {
+ "MetricExpr": "(p1_wr_dat * 64 + p1_rd_dat_64b * 64 + p1_rd_dat_32b * 32) / cpa_cycles",
+ "BriefDescription": "Average bandwidth of CPA Port 1",
+ "MetricGroup": "CPA",
+ "MetricName": "p1_avg_bw",
+ "Compat": "0x00000030",
+ "Unit": "hisi_sicl,cpa"
+ },
+ {
+ "MetricExpr": "(p0_wr_dat * 64 + p0_rd_dat_64b * 64 + p0_rd_dat_32b * 32) / cpa_cycles",
+ "BriefDescription": "Average bandwidth of CPA Port 0",
+ "MetricGroup": "CPA",
+ "MetricName": "p0_avg_bw",
+ "Compat": "0x00000030",
+ "Unit": "hisi_sicl,cpa"
+ }
+]
diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
index 1a57c3f81dd4..159d9eab6e79 100644
--- a/tools/perf/pmu-events/jevents.c
+++ b/tools/perf/pmu-events/jevents.c
@@ -277,6 +277,7 @@ static struct map {
{ "CPU-M-CF", "cpum_cf" },
{ "CPU-M-SF", "cpum_sf" },
{ "UPI LL", "uncore_upi" },
+ { "hisi_sicl,cpa", "hisi_sicl,cpa"},
{ "hisi_sccl,ddrc", "hisi_sccl,ddrc" },
{ "hisi_sccl,hha", "hisi_sccl,hha" },
{ "hisi_sccl,l3c", "hisi_sccl,l3c" },
--
2.24.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] perf jevents: Add support for HiSilicon CPA PMU aliasing
2022-02-14 11:42 ` [PATCH 2/2] perf jevents: Add support for HiSilicon CPA PMU aliasing Qi Liu
@ 2022-02-14 12:04 ` John Garry
2022-02-15 1:27 ` liuqi (BA)
2022-02-15 1:23 ` liuqi (BA)
1 sibling, 1 reply; 10+ messages in thread
From: John Garry @ 2022-02-14 12:04 UTC (permalink / raw)
To: Qi Liu, will, mark.rutland
Cc: linux-arm-kernel, linux-kernel, Linuxarm, Zhangshaokun
On 14/02/2022 11:42, Qi Liu wrote:
> Add support for HiSilicon CPA PMU aliasing.
>
> The kernel driver is in drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
>
> Signed-off-by: Qi Liu<liuqi115@huawei.com>
You realy should cc the perf tool list and maintainers there also.
Reviewed-by: John Garry <john.garry@huawei.com>
> + },
> + {
> + "MetricExpr": "(p0_wr_dat * 64 + p0_rd_dat_64b * 64 + p0_rd_dat_32b * 32) / cpa_cycles",
> + "BriefDescription": "Average bandwidth of CPA Port 0",
> + "MetricGroup": "CPA",
nit: maybe this should have a more distrinct name
> + "MetricName": "p0_avg_bw",
> + "Compat": "0x00000030",
JFYI, I have a rewrite of the perf tool system pmu metric support that
will not require us to add the compat here.
> + "Unit": "hisi_sicl,cpa"
Thanks,
John
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] perf jevents: Add support for HiSilicon CPA PMU aliasing
2022-02-14 11:42 ` [PATCH 2/2] perf jevents: Add support for HiSilicon CPA PMU aliasing Qi Liu
2022-02-14 12:04 ` John Garry
@ 2022-02-15 1:23 ` liuqi (BA)
1 sibling, 0 replies; 10+ messages in thread
From: liuqi (BA) @ 2022-02-15 1:23 UTC (permalink / raw)
To: Qi Liu, will, mark.rutland
Cc: linux-arm-kernel, linux-kernel, zhangshaokun, Linuxarm, mingo,
acme, linux-perf-users, peterz
+CC perf tool list and maintainers.
Qi
On 2022/2/14 19:42, Qi Liu wrote:
> Add support for HiSilicon CPA PMU aliasing.
>
> The kernel driver is in drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
>
> Signed-off-by: Qi Liu <liuqi115@huawei.com>
> ---
> .../arm64/hisilicon/hip09/sys/uncore-cpa.json | 81 +++++++++++++++++++
> tools/perf/pmu-events/jevents.c | 1 +
> 2 files changed, 82 insertions(+)
> create mode 100644 tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json
>
> diff --git a/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json b/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json
> new file mode 100644
> index 000000000000..35c90da7f54b
> --- /dev/null
> +++ b/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json
> @@ -0,0 +1,81 @@
> +[
> + {
> + "ConfigCode": "0x00",
> + "EventName": "cpa_cycles",
> + "BriefDescription": "count of CPA cycles",
> + "Compat": "0x00000030",
> + "Unit": "hisi_sicl,cpa"
> + },
> + {
> + "ConfigCode": "0x61",
> + "EventName": "p1_wr_dat",
> + "BriefDescription": "Number of write ops transmitted by the P1 port",
> + "Compat": "0x00000030",
> + "Unit": "hisi_sicl,cpa"
> + },
> + {
> + "ConfigCode": "0x62",
> + "EventName": "p1_rd_dat",
> + "BriefDescription": "Number of read ops transmitted by the P1 port",
> + "Compat": "0x00000030",
> + "Unit": "hisi_sicl,cpa"
> + },
> + {
> + "ConfigCode": "0x3",
> + "EventName": "p1_rd_dat_64b",
> + "BriefDescription": "Number of read ops transmitted by the P1 port which size is 64 bytes",
> + "Compat": "0x00000030",
> + "Unit": "hisi_sicl,cpa"
> + },
> + {
> + "ConfigCode": "0x4",
> + "EventName": "p1_rd_dat_32b",
> + "BriefDescription": "Number of read ops transmitted by the P1 port which size is 32 bytes",
> + "Compat": "0x00000030",
> + "Unit": "hisi_sicl,cpa"
> + },
> + {
> + "ConfigCode": "0xE1",
> + "EventName": "p0_wr_dat",
> + "BriefDescription": "Number of write ops transmitted by the P0 port",
> + "Compat": "0x00000030",
> + "Unit": "hisi_sicl,cpa"
> + },
> + {
> + "ConfigCode": "0xE2",
> + "EventName": "p0_rd_dat",
> + "BriefDescription": "Number of read ops transmitted by the P0 port",
> + "Compat": "0x00000030",
> + "Unit": "hisi_sicl,cpa"
> + },
> + {
> + "ConfigCode": "0x83",
> + "EventName": "p0_rd_dat_64b",
> + "BriefDescription": "Number of read ops transmitted by the P0 port which size is 64 bytes",
> + "Compat": "0x00000030",
> + "Unit": "hisi_sicl,cpa"
> + },
> + {
> + "ConfigCode": "0x84",
> + "EventName": "p0_rd_dat_32b",
> + "BriefDescription": "Number of read ops transmitted by the P0 port which size is 32 bytes",
> + "Compat": "0x00000030",
> + "Unit": "hisi_sicl,cpa"
> + },
> + {
> + "MetricExpr": "(p1_wr_dat * 64 + p1_rd_dat_64b * 64 + p1_rd_dat_32b * 32) / cpa_cycles",
> + "BriefDescription": "Average bandwidth of CPA Port 1",
> + "MetricGroup": "CPA",
> + "MetricName": "p1_avg_bw",
> + "Compat": "0x00000030",
> + "Unit": "hisi_sicl,cpa"
> + },
> + {
> + "MetricExpr": "(p0_wr_dat * 64 + p0_rd_dat_64b * 64 + p0_rd_dat_32b * 32) / cpa_cycles",
> + "BriefDescription": "Average bandwidth of CPA Port 0",
> + "MetricGroup": "CPA",
> + "MetricName": "p0_avg_bw",
> + "Compat": "0x00000030",
> + "Unit": "hisi_sicl,cpa"
> + }
> +]
> diff --git a/tools/perf/pmu-events/jevents.c b/tools/perf/pmu-events/jevents.c
> index 1a57c3f81dd4..159d9eab6e79 100644
> --- a/tools/perf/pmu-events/jevents.c
> +++ b/tools/perf/pmu-events/jevents.c
> @@ -277,6 +277,7 @@ static struct map {
> { "CPU-M-CF", "cpum_cf" },
> { "CPU-M-SF", "cpum_sf" },
> { "UPI LL", "uncore_upi" },
> + { "hisi_sicl,cpa", "hisi_sicl,cpa"},
> { "hisi_sccl,ddrc", "hisi_sccl,ddrc" },
> { "hisi_sccl,hha", "hisi_sccl,hha" },
> { "hisi_sccl,l3c", "hisi_sccl,l3c" },
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] perf jevents: Add support for HiSilicon CPA PMU aliasing
2022-02-14 12:04 ` John Garry
@ 2022-02-15 1:27 ` liuqi (BA)
0 siblings, 0 replies; 10+ messages in thread
From: liuqi (BA) @ 2022-02-15 1:27 UTC (permalink / raw)
To: John Garry, Qi Liu, will, mark.rutland
Cc: linux-arm-kernel, linux-kernel, Zhangshaokun, Linuxarm, linux-perf-users
On 2022/2/14 20:04, John Garry wrote:
> On 14/02/2022 11:42, Qi Liu wrote:
>> Add support for HiSilicon CPA PMU aliasing.
>>
>> The kernel driver is in drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
>>
>> Signed-off-by: Qi Liu<liuqi115@huawei.com>
>
> You realy should cc the perf tool list and maintainers there also.
>
> Reviewed-by: John Garry <john.garry@huawei.com>
>
>> + },
>> + {
>> + "MetricExpr": "(p0_wr_dat * 64 + p0_rd_dat_64b * 64 +
>> p0_rd_dat_32b * 32) / cpa_cycles",
>> + "BriefDescription": "Average bandwidth of CPA Port 0",
>> + "MetricGroup": "CPA",
>
Hi john,
> nit: maybe this should have a more distrinct name
got it, will change metric's name next version.
>
>> + "MetricName": "p0_avg_bw",
>> + "Compat": "0x00000030",
>
> JFYI, I have a rewrite of the perf tool system pmu metric support that
> will not require us to add the compat here.
>
great! I'm very glad to test it using CPA PMU.
Thanks,
Qi
>> + "Unit": "hisi_sicl,cpa"
>
> Thanks,
> John
> .
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drivers/perf: hisi: Add Support for CPA PMU
2022-02-14 11:42 ` [PATCH 1/2] drivers/perf: hisi: Add Support for " Qi Liu
@ 2022-02-15 10:47 ` John Garry
2022-02-16 3:16 ` liuqi (BA)
0 siblings, 1 reply; 10+ messages in thread
From: John Garry @ 2022-02-15 10:47 UTC (permalink / raw)
To: Qi Liu, will, mark.rutland
Cc: linux-arm-kernel, linux-kernel, Linuxarm, Zhangshaokun
On 14/02/2022 11:42, Qi Liu wrote:
> On HiSilicon Hip09 platform, there is a CPA(Coherency Protocol Agent) on
> each SICL (Super I/O Cluster) which implements packet format translation,
> route parsing and traffic statistic.
>
> CPA PMU has separate PMU registers which the driver can program freely and
> interrupt is supported to handle counter overflow. Let's support its driver
> under the framework of HiSilicon uncore PMU driver.
>
Any documentation required?
> Signed-off-by: Qi Liu <liuqi115@huawei.com>
> ---
> drivers/perf/hisilicon/Makefile | 2 +-
> drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c | 403 +++++++++++++++++++
> include/linux/cpuhotplug.h | 1 +
> 3 files changed, 405 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..a28796e93ae1
> --- /dev/null
> +++ b/drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
> @@ -0,0 +1,403 @@
> +// 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
nit: some people prefer lower case for hex numbers, but not a big deal.
> +
> +/* CPA operation command */
> +#define CPA_PERF_CTRL_EN BIT_ULL(0)
> +#define CPA_EVTYPE_MASK 0xFFUL
> +#define CPA_PM_CTRL BIT_ULL(9)
> +
> +#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;
rounddown(,4) could be used, I think. Or something like it.
> + 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);
no need for ()
> + writel(val, cpa_pmu->base + reg);
> +}
> +
> +static void hisi_cpa_pmu_start_counters(struct hisi_pmu *cpa_pmu)
> +{
> + u32 val;
> +
> + val = readl(cpa_pmu->base + CPA_PERF_CTRL);
> + val |= CPA_PERF_CTRL_EN;
> + writel(val, cpa_pmu->base + CPA_PERF_CTRL);
> +}
> +
> +static void hisi_cpa_pmu_stop_counters(struct hisi_pmu *cpa_pmu)
> +{
> + u32 val;
> +
> + val = readl(cpa_pmu->base + CPA_PERF_CTRL);
> + val &= ~(CPA_PERF_CTRL_EN);
> + writel(val, cpa_pmu->base + CPA_PERF_CTRL);
> +}
> +
> +static void hisi_cpa_pmu_disable_pm(struct hisi_pmu *cpa_pmu)
> +{
> + u32 val;
> +
> + val = readl(cpa_pmu->base + CPA_CFG_REG);
> + val |= CPA_PM_CTRL;
> + writel(val, cpa_pmu->base + CPA_CFG_REG);
> +}
> +
> +static void hisi_cpa_pmu_enable_pm(struct hisi_pmu *cpa_pmu)
> +{
> + u32 val;
> +
> + val = readl(cpa_pmu->base + CPA_CFG_REG);
> + val &= ~CPA_PM_CTRL;
> + writel(val, cpa_pmu->base + CPA_CFG_REG);
> +}
> +
> +static void hisi_cpa_pmu_enable_counter(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
> +{
> + u32 val;
> +
> + /* Enable counter index in CPA_EVENT_CTRL register */
> + val = readl(cpa_pmu->base + CPA_EVENT_CTRL);
> + val |= 1 << hwc->idx;
> + writel(val, cpa_pmu->base + CPA_EVENT_CTRL);
> +
> + /* Power management should be disabled before counting. */
> + hisi_cpa_pmu_disable_pm(cpa_pmu);
> +}
> +
> +static void hisi_cpa_pmu_disable_counter(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
> +{
> + u32 val;
> +
> + hisi_cpa_pmu_enable_pm(cpa_pmu);
> +
> + /* Clear counter index in CPA_EVENT_CTRL register */
> + val = readl(cpa_pmu->base + CPA_EVENT_CTRL);
> + val &= ~(1UL << hwc->idx);
> + writel(val, cpa_pmu->base + CPA_EVENT_CTRL);
> +}
> +
> +static void hisi_cpa_pmu_enable_counter_int(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
> +{
> + u32 val;
> +
> + /* Write 0 to enable interrupt */
> + val = readl(cpa_pmu->base + CPA_INT_MASK);
> + val &= ~(1UL << hwc->idx);
> + writel(val, cpa_pmu->base + CPA_INT_MASK);
> +}
> +
> +static void hisi_cpa_pmu_disable_counter_int(struct hisi_pmu *cpa_pmu, struct hw_perf_event *hwc)
> +{
> + u32 val;
> +
> + /* Write 1 to mask interrupt */
> + val = readl(cpa_pmu->base + CPA_INT_MASK);
> + val |= 1 << hwc->idx;
> + writel(val, cpa_pmu->base + CPA_INT_MASK);
> +}
> +
> +static u32 hisi_cpa_pmu_get_int_status(struct hisi_pmu *cpa_pmu)
> +{
> + return readl(cpa_pmu->base + CPA_INT_STATUS);
> +}
> +
> +static void hisi_cpa_pmu_clear_int_status(struct hisi_pmu *cpa_pmu, int idx)
> +{
> + writel(1 << idx, cpa_pmu->base + CPA_INT_CLEAR);
> +}
> +
> +static const struct acpi_device_id hisi_cpa_pmu_acpi_match[] = {
> + { "HISI0281", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(acpi, hisi_cpa_pmu_acpi_match);
> +
> +static int hisi_cpa_pmu_init_data(struct platform_device *pdev,
> + struct hisi_pmu *cpa_pmu)
> +{
> + if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
> + &cpa_pmu->sccl_id)) {
> + dev_err(&pdev->dev, "Can not read cpa_pmu sccl-id\n");
> + return -EINVAL;
> + }
> +
> + cpa_pmu->ccl_id = -1;
> +
> + if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
> + &cpa_pmu->index_id)) {
> + dev_err(&pdev->dev, "Cannot read idx-id\n");
> + return -EINVAL;
> + }
> +
> + cpa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(cpa_pmu->base)) {
> + dev_err(&pdev->dev, "ioremap failed for cpa_pmu resource\n");
I think that devm_platform_ioremap_resource() gives error messages already
> + 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(p1_wr_dat, 0x61),
> + HISI_PMU_EVENT_ATTR(p1_rd_dat, 0x62),
> + HISI_PMU_EVENT_ATTR(p0_wr_dat, 0xE1),
> + HISI_PMU_EVENT_ATTR(p0_rd_dat, 0xE2),
would it be better to prefix them all with cpa_? That might be
future-proofing any event alias clashes with other PMU types
> + 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,
nit: no need to ',' on sentinel
> +};
> +
> +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);
nit: I don't see the need for this separate function. One issue is that
we need to study the code there to see if we're missing some tidy-up if
the hisi_uncore_pmu_init_irq call fails, below.
> + 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;
> +
> + platform_set_drvdata(pdev, cpa_pmu);
nit: normally we do this at the end, right?
> +
> + ret = hisi_cpa_pmu_dev_probe(pdev, cpa_pmu);
> + if (ret)
> + return ret;
> +
> + ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
> + &cpa_pmu->node);
> + if (ret) {
> + dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
> + return ret;
> + }
> +
> + name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "hisi_sicl%d_cpa%u", cpa_pmu->sccl_id - 1,
why subtract 1? Looks dangerous if sccl_id == 0
> + cpa_pmu->index_id);
> +
> + cpa_pmu->pmu = (struct pmu) {
> + .name = name,
> + .module = THIS_MODULE,
> + .task_ctx_nr = perf_invalid_context,
> + .event_init = hisi_uncore_pmu_event_init,
> + .pmu_enable = hisi_uncore_pmu_enable,
> + .pmu_disable = hisi_uncore_pmu_disable,
> + .add = hisi_uncore_pmu_add,
> + .del = hisi_uncore_pmu_del,
> + .start = hisi_uncore_pmu_start,
> + .stop = hisi_uncore_pmu_stop,
> + .read = hisi_uncore_pmu_read,
> + .attr_groups = cpa_pmu->pmu_events.attr_groups,
> + .capabilities = PERF_PMU_CAP_NO_EXCLUDE,
> + };
> +
> + ret = perf_pmu_register(&cpa_pmu->pmu, name, -1);
> + if (ret) {
> + dev_err(cpa_pmu->dev, "CPA PMU register failed\n");
> + cpuhp_state_remove_instance_nocalls(
> + CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE, &cpa_pmu->node);
> + }
> +
> + return ret;
> +}
> +
> +static int hisi_cpa_pmu_remove(struct platform_device *pdev)
> +{
> + struct hisi_pmu *cpa_pmu = platform_get_drvdata(pdev);
> +
> + perf_pmu_unregister(&cpa_pmu->pmu);
> + cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
> + &cpa_pmu->node);
> + return 0;
> +}
> +
> +static struct platform_driver hisi_cpa_pmu_driver = {
> + .driver = {
> + .name = "hisi_cpa_pmu",
> + .acpi_match_table = ACPI_PTR(hisi_cpa_pmu_acpi_match),
> + .suppress_bind_attrs = true,
> + },
> + .probe = hisi_cpa_pmu_probe,
> + .remove = hisi_cpa_pmu_remove,
> +};
> +
> +static int __init hisi_cpa_pmu_module_init(void)
> +{
> + int ret;
> +
> + ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
> + "AP_PERF_ARM_HISI_CPA_ONLINE",
> + hisi_uncore_pmu_online_cpu,
> + hisi_uncore_pmu_offline_cpu);
> + if (ret) {
> + pr_err("CPA PMU: setup hotplug: %d\n", ret);
> + return ret;
> + }
> +
> + ret = platform_driver_register(&hisi_cpa_pmu_driver);
> + if (ret)
> + cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE);
I am not being totally serious, but this pattern of registering a CPU
hotplug handler and then a driver is so common that we could nearly add
a wrapper for it.
> +
> + 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,
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drivers/perf: hisi: Add Support for CPA PMU
2022-02-15 10:47 ` John Garry
@ 2022-02-16 3:16 ` liuqi (BA)
2022-02-17 18:11 ` John Garry
0 siblings, 1 reply; 10+ messages in thread
From: liuqi (BA) @ 2022-02-16 3:16 UTC (permalink / raw)
To: John Garry, will, mark.rutland
Cc: linux-arm-kernel, linux-kernel, Linuxarm, Zhangshaokun
Hi John,
Thanks for the comments. some replies inline.
On 2022/2/15 18:47, John Garry wrote:
> On 14/02/2022 11:42, Qi Liu wrote:
>> On HiSilicon Hip09 platform, there is a CPA(Coherency Protocol Agent) on
>> each SICL (Super I/O Cluster) which implements packet format translation,
>> route parsing and traffic statistic.
>>
>> CPA PMU has separate PMU registers which the driver can program freely
>> and
>> interrupt is supported to handle counter overflow. Let's support its
>> driver
>> under the framework of HiSilicon uncore PMU driver.
>>
>
> Any documentation required?
I think CPA PMU is used as PMUs described in
Documentation/admin-guide/perf/hisi-pmu.rst, so doesn't need additional
documents.
>
>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
>> ---
>> drivers/perf/hisilicon/Makefile | 2 +-
>> drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c | 403 +++++++++++++++++++
>> include/linux/cpuhotplug.h | 1 +
>> 3 files changed, 405 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/perf/hisilicon/hisi_uncore_cpa_pmu.c
>>
[...]
>> +
>> +/* 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
>
> nit: some people prefer lower case for hex numbers, but not a big deal.
>
thanks, I'll fix this, to keep the same with other uncore PMUs.
>> +
>> +/* CPA operation command */
>> +#define CPA_PERF_CTRL_EN BIT_ULL(0)
>> +#define CPA_EVTYPE_MASK 0xFFUL
>> +#define CPA_PM_CTRL BIT_ULL(9)
[...]
>> +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;
>
> rounddown(,4) could be used, I think. Or something like it.
got it, thanks.
>
>> + 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);
>
> no need for ()
>
got it, thanks.
>> + writel(val, cpa_pmu->base + reg);
>> +}
>> +
[...]
>> +static int hisi_cpa_pmu_init_data(struct platform_device *pdev,
>> + struct hisi_pmu *cpa_pmu)
>> +{
>> + if (device_property_read_u32(&pdev->dev, "hisilicon,scl-id",
>> + &cpa_pmu->sccl_id)) {
>> + dev_err(&pdev->dev, "Can not read cpa_pmu sccl-id\n");
>> + return -EINVAL;
>> + }
>> +
>> + cpa_pmu->ccl_id = -1;
>> +
>> + if (device_property_read_u32(&pdev->dev, "hisilicon,idx-id",
>> + &cpa_pmu->index_id)) {
>> + dev_err(&pdev->dev, "Cannot read idx-id\n");
>> + return -EINVAL;
>> + }
>> +
>> + cpa_pmu->base = devm_platform_ioremap_resource(pdev, 0);
>> + if (IS_ERR(cpa_pmu->base)) {
>> + dev_err(&pdev->dev, "ioremap failed for cpa_pmu resource\n");
>
> I think that devm_platform_ioremap_resource() gives error messages already
>
ah, yes, I'll delete this, thanks.
>> + return PTR_ERR(cpa_pmu->base);
>> + }
>> +
>> + cpa_pmu->identifier = readl(cpa_pmu->base + CPA_VERSION);
>> +
>> + return 0;
>> +}
>> +
[...]
>> +
>> +static struct attribute *hisi_cpa_pmu_events_attr[] = {
>> + HISI_PMU_EVENT_ATTR(cpa_cycles, 0x00),
>> + HISI_PMU_EVENT_ATTR(p1_wr_dat, 0x61),
>> + HISI_PMU_EVENT_ATTR(p1_rd_dat, 0x62),
>> + HISI_PMU_EVENT_ATTR(p0_wr_dat, 0xE1),
>> + HISI_PMU_EVENT_ATTR(p0_rd_dat, 0xE2),
>
> would it be better to prefix them all with cpa_? That might be
> future-proofing any event alias clashes with other PMU types
>
yes, will fix this, thanks.
>> + NULL
>> +};
>> +
[...]
>> +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,
>
> nit: no need to ',' on sentinel
>
will fix this, thanks.
>> +};
>> +
>> +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);
>
> nit: I don't see the need for this separate function. One issue is that
> we need to study the code there to see if we're missing some tidy-up if
> the hisi_uncore_pmu_init_irq call fails, below.
hisi_cpa_init_data() is used to get data(including base address,
sci-id..) from ACPI, and it doesn't need tidy-up, so I think it will be
clearer as a separate function, thanks.
>
>> + 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;
>> +
>> + platform_set_drvdata(pdev, cpa_pmu);
>
> nit: normally we do this at the end, right?
>
yes, I'll fix this, thanks.
>> +
>> + ret = hisi_cpa_pmu_dev_probe(pdev, cpa_pmu);
>> + if (ret)
>> + return ret;
>> +
>> + ret = cpuhp_state_add_instance(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
>> + &cpa_pmu->node);
>> + if (ret) {
>> + dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
>> + return ret;
>> + }
>> +
>> + name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
>> "hisi_sicl%d_cpa%u", cpa_pmu->sccl_id - 1,
>
> why subtract 1? Looks dangerous if sccl_id == 0
As CPA PMU is on SICL, and id of SICL is 1 smaller than id of adjacent
SCCL. SCCL id in our Hip09 platform is set as 1, 3.... so, a reasonable
sccl-id will never be 0.
Parameters sccl_id is read from ACPI, and is checked in
hisi_pmu_cpu_is_associated_pmu(), so we could make sure that sccl_id
here is reasonable and is not 0.
>
>> + cpa_pmu->index_id);
>> +
>> + cpa_pmu->pmu = (struct pmu) {
>> + .name = name,
>> + .module = THIS_MODULE,
>> + .task_ctx_nr = perf_invalid_context,
>> + .event_init = hisi_uncore_pmu_event_init,
>> + .pmu_enable = hisi_uncore_pmu_enable,
>> + .pmu_disable = hisi_uncore_pmu_disable,
>> + .add = hisi_uncore_pmu_add,
>> + .del = hisi_uncore_pmu_del,
>> + .start = hisi_uncore_pmu_start,
>> + .stop = hisi_uncore_pmu_stop,
>> + .read = hisi_uncore_pmu_read,
>> + .attr_groups = cpa_pmu->pmu_events.attr_groups,
>> + .capabilities = PERF_PMU_CAP_NO_EXCLUDE,
>> + };
>> +
>> + ret = perf_pmu_register(&cpa_pmu->pmu, name, -1);
>> + if (ret) {
>> + dev_err(cpa_pmu->dev, "CPA PMU register failed\n");
>> + cpuhp_state_remove_instance_nocalls(
>> + CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE, &cpa_pmu->node);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int hisi_cpa_pmu_remove(struct platform_device *pdev)
>> +{
>> + struct hisi_pmu *cpa_pmu = platform_get_drvdata(pdev);
>> +
>> + perf_pmu_unregister(&cpa_pmu->pmu);
>> +
>> cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
>> + &cpa_pmu->node);
>> + return 0;
>> +}
>> +
>> +static struct platform_driver hisi_cpa_pmu_driver = {
>> + .driver = {
>> + .name = "hisi_cpa_pmu",
>> + .acpi_match_table = ACPI_PTR(hisi_cpa_pmu_acpi_match),
>> + .suppress_bind_attrs = true,
>> + },
>> + .probe = hisi_cpa_pmu_probe,
>> + .remove = hisi_cpa_pmu_remove,
>> +};
>> +
>> +static int __init hisi_cpa_pmu_module_init(void)
>> +{
>> + int ret;
>> +
>> + ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
>> + "AP_PERF_ARM_HISI_CPA_ONLINE",
>> + hisi_uncore_pmu_online_cpu,
>> + hisi_uncore_pmu_offline_cpu);
>> + if (ret) {
>> + pr_err("CPA PMU: setup hotplug: %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = platform_driver_register(&hisi_cpa_pmu_driver);
>> + if (ret)
>> + cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE);
>
> I am not being totally serious, but this pattern of registering a CPU
> hotplug handler and then a driver is so common that we could nearly add
> a wrapper for it.
>
Hi John, do you mean somthing like this?
in hisi_uncore_pmu.c:
int hisi_uncore_pmu_module_init(enum cpuhp_state state, const char
*name, struct platform_driver *drv)
{
int ret;
ret = cpuhp_setup_state_multi(state, name, hisi_uncore_pmu_online_cpu,
hisi_uncore_pmu_offline_cpu);
if (ret) {
pr_err("%s: setup hotplug: %d\n", drv->driver.name, ret);
return ret;
}
ret = platform_driver_register(drv);
if (ret)
cpuhp_remove_multi_state(state);
return ret;
}
in hisi_uncore_cpa_pmu.c:
static int __init hisi_cpa_pmu_module_init(void)
{
int ret;
ret = hisi_uncore_pmu_module_init(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
"AP_PERF_ARM_HISI_CPA_ONLINE",
&hisi_cpa_pmu_driver);
return ret;
}
module_init(hisi_cpa_pmu_module_init);
Thanks,
Qi
>> +
>> + 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,
>
> .
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drivers/perf: hisi: Add Support for CPA PMU
2022-02-16 3:16 ` liuqi (BA)
@ 2022-02-17 18:11 ` John Garry
2022-02-18 8:07 ` liuqi (BA)
0 siblings, 1 reply; 10+ messages in thread
From: John Garry @ 2022-02-17 18:11 UTC (permalink / raw)
To: liuqi (BA), will, mark.rutland
Cc: linux-arm-kernel, linux-kernel, Linuxarm, Zhangshaokun
On 16/02/2022 03:16, liuqi (BA) wrote:
>>>
>>> + name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
>>> "hisi_sicl%d_cpa%u", cpa_pmu->sccl_id - 1,
>>
>> why subtract 1? Looks dangerous if sccl_id == 0
> As CPA PMU is on SICL, and id of SICL is 1 smaller than id of adjacent
> SCCL. SCCL id in our Hip09 platform is set as 1, 3.... so, a reasonable
> sccl-id will never be 0.
I think that you need to view the HW IP independent of the SoC, and not
get into the practice of relying on these things. Anyway, doesn't the
sccl_id come from ACPI DSD (so we can set as we want)?
>
> Parameters sccl_id is read from ACPI, and is checked in
> hisi_pmu_cpu_is_associated_pmu(), so we could make sure that sccl_id
> here is reasonable and is not 0.
>
>
>>
>>> + cpa_pmu->index_id);
>>> +
>>> + cpa_pmu->pmu = (struct pmu) {
>>> + .name = name,
>>> + .module = THIS_MODULE,
>>> + .task_ctx_nr = perf_invalid_context,
>>> + .event_init = hisi_uncore_pmu_event_init,
>>> + .pmu_enable = hisi_uncore_pmu_enable,
>>> + .pmu_disable = hisi_uncore_pmu_disable,
>>> + .add = hisi_uncore_pmu_add,
>>> + .del = hisi_uncore_pmu_del,
>>> + .start = hisi_uncore_pmu_start,
>>> + .stop = hisi_uncore_pmu_stop,
>>> + .read = hisi_uncore_pmu_read,
>>> + .attr_groups = cpa_pmu->pmu_events.attr_groups,
>>> + .capabilities = PERF_PMU_CAP_NO_EXCLUDE,
>>> + };
>>> +
>>> + ret = perf_pmu_register(&cpa_pmu->pmu, name, -1);
>>> + if (ret) {
>>> + dev_err(cpa_pmu->dev, "CPA PMU register failed\n");
>>> + cpuhp_state_remove_instance_nocalls(
>>> + CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE, &cpa_pmu->node);
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int hisi_cpa_pmu_remove(struct platform_device *pdev)
>>> +{
>>> + struct hisi_pmu *cpa_pmu = platform_get_drvdata(pdev);
>>> +
>>> + perf_pmu_unregister(&cpa_pmu->pmu);
>>> + cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
>>> + &cpa_pmu->node);
>>> + return 0;
>>> +}
>>> +
>>> +static struct platform_driver hisi_cpa_pmu_driver = {
>>> + .driver = {
>>> + .name = "hisi_cpa_pmu",
>>> + .acpi_match_table = ACPI_PTR(hisi_cpa_pmu_acpi_match),
>>> + .suppress_bind_attrs = true,
>>> + },
>>> + .probe = hisi_cpa_pmu_probe,
>>> + .remove = hisi_cpa_pmu_remove,
>>> +};
>>> +
>>> +static int __init hisi_cpa_pmu_module_init(void)
>>> +{
>>> + int ret;
>>> +
>>> + ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
>>> + "AP_PERF_ARM_HISI_CPA_ONLINE",
>>> + hisi_uncore_pmu_online_cpu,
>>> + hisi_uncore_pmu_offline_cpu);
>>> + if (ret) {
>>> + pr_err("CPA PMU: setup hotplug: %d\n", ret);
>>> + return ret;
>>> + }
>>> +
>>> + ret = platform_driver_register(&hisi_cpa_pmu_driver);
>>> + if (ret)
>>> + cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE);
>>
>> I am not being totally serious, but this pattern of registering a CPU
>> hotplug handler and then a driver is so common that we could nearly
>> add a wrapper for it.
>>
>
> Hi John, do you mean somthing like this?
>
> in hisi_uncore_pmu.c:
>
> int hisi_uncore_pmu_module_init(enum cpuhp_state state, const char
> *name, struct platform_driver *drv)
> {
> int ret;
>
> ret = cpuhp_setup_state_multi(state, name, hisi_uncore_pmu_online_cpu,
> hisi_uncore_pmu_offline_cpu);
> if (ret) {
> pr_err("%s: setup hotplug: %d\n", drv->driver.name, ret);
> return ret;
> }
>
> ret = platform_driver_register(drv);
> if (ret)
> cpuhp_remove_multi_state(state);
>
> return ret;
> }
>
> in hisi_uncore_cpa_pmu.c:
>
> static int __init hisi_cpa_pmu_module_init(void)
> {
> int ret;
>
> ret = hisi_uncore_pmu_module_init(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
> "AP_PERF_ARM_HISI_CPA_ONLINE",
> &hisi_cpa_pmu_driver);
>
> return ret;
> }
> module_init(hisi_cpa_pmu_module_init);
Sure, but I'm talking about going further and having this as
drivers/perf or even platform_device.h helper:
diff --git a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
index 62299ab5a9be..22f635260a5f 100644
--- a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
@@ -562,26 +562,11 @@ static struct platform_driver hisi_ddrc_pmu_driver = {
.remove = hisi_ddrc_pmu_remove,
};
-static int __init hisi_ddrc_pmu_module_init(void)
-{
- int ret;
-
- ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_HISI_DDRC_ONLINE,
- "AP_PERF_ARM_HISI_DDRC_ONLINE",
- hisi_uncore_pmu_online_cpu,
- hisi_uncore_pmu_offline_cpu);
- if (ret) {
- pr_err("DDRC PMU: setup hotplug, ret = %d\n", ret);
- return ret;
- }
-
- ret = platform_driver_register(&hisi_ddrc_pmu_driver);
- if (ret)
- cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_DDRC_ONLINE);
+module_platform_driver_cpu_hotplug(hisi_ddrc_pmu_driver,
+ AP_PERF_ARM_HISI_DDRC_ONLINE,
+ hisi_uncore_pmu_online_cpu,
+ hisi_uncore_pmu_online_cpu);
- return ret;
-}
-module_init(hisi_ddrc_pmu_module_init);
static void __exit hisi_ddrc_pmu_module_exit(void)
{
diff --git a/include/linux/platform_device.h
b/include/linux/platform_device.h
index 7c96f169d274..d0816dfc0637 100644
--- a/include/linux/platform_device.h
+++ b/include/linux/platform_device.h
@@ -261,6 +261,28 @@ static inline void platform_set_drvdata(struct
platform_device *pdev,
#define builtin_platform_driver(__platform_driver) \
builtin_driver(__platform_driver, platform_driver_register)
+#define module_platform_driver_cpu_hotplug(__platform_driver,
cpuhp_state, online, offline) \
+static int __init __module_platform_driver_cpu_hotplug##_init(void) \
+{ \
+ int ret; \
+ ret = cpuhp_setup_state_multi(CPUHP_##cpuhp_state, \
+ "##cpuhp_state", \
+ online, \
+ offline); \
+ if (ret) { \
+ pr_err("setup hotplug, ret = %d\n", ret); \
+ return ret; \
+ } \
+ \
+ ret = platform_driver_register(&__platform_driver); \
+ if (ret) \
+ cpuhp_remove_multi_state(CPUHP_##cpuhp_state); \
+ \
+ return ret; \
+} \
+module_init(__module_platform_driver_cpu_hotplug##_init);
+
+
/* module_platform_driver_probe() - Helper macro for drivers that don't do
* anything special in module init/exit. This eliminates a lot of
* boilerplate. Each module may only use this macro once, and
--
2.26.2
but only a half serious suggestion :)
Thanks,
John
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] drivers/perf: hisi: Add Support for CPA PMU
2022-02-17 18:11 ` John Garry
@ 2022-02-18 8:07 ` liuqi (BA)
0 siblings, 0 replies; 10+ messages in thread
From: liuqi (BA) @ 2022-02-18 8:07 UTC (permalink / raw)
To: John Garry, will, mark.rutland
Cc: linux-arm-kernel, linux-kernel, Linuxarm, Zhangshaokun
HiJohn,
On 2022/2/18 2:11, John Garry wrote:
> On 16/02/2022 03:16, liuqi (BA) wrote:
>>>>
>>>> + name = devm_kasprintf(&pdev->dev, GFP_KERNEL,
>>>> "hisi_sicl%d_cpa%u", cpa_pmu->sccl_id - 1,
>>>
>>> why subtract 1? Looks dangerous if sccl_id == 0
>> As CPA PMU is on SICL, and id of SICL is 1 smaller than id of adjacent
>> SCCL. SCCL id in our Hip09 platform is set as 1, 3.... so, a
>> reasonable sccl-id will never be 0.
>
> I think that you need to view the HW IP independent of the SoC, and not
> get into the practice of relying on these things. Anyway, doesn't the
> sccl_id come from ACPI DSD (so we can set as we want)?
>
Yes, sccl_id in driver is read from ACPI DSD, and is checked in
hisi_pmu_cpu_is_associated_pmu().
>>
>> Parameters sccl_id is read from ACPI, and is checked in
>> hisi_pmu_cpu_is_associated_pmu(), so we could make sure that sccl_id
>> here is reasonable and is not 0.
>>
>>
>>>
>>>> + cpa_pmu->index_id);
>>>> +
>>>> + cpa_pmu->pmu = (struct pmu) {
>>>> + .name = name,
>>>> + .module = THIS_MODULE,
>>>> + .task_ctx_nr = perf_invalid_context,
>>>> + .event_init = hisi_uncore_pmu_event_init,
>>>> + .pmu_enable = hisi_uncore_pmu_enable,
>>>> + .pmu_disable = hisi_uncore_pmu_disable,
>>>> + .add = hisi_uncore_pmu_add,
>>>> + .del = hisi_uncore_pmu_del,
>>>> + .start = hisi_uncore_pmu_start,
>>>> + .stop = hisi_uncore_pmu_stop,
>>>> + .read = hisi_uncore_pmu_read,
>>>> + .attr_groups = cpa_pmu->pmu_events.attr_groups,
>>>> + .capabilities = PERF_PMU_CAP_NO_EXCLUDE,
>>>> + };
>>>> +
>>>> + ret = perf_pmu_register(&cpa_pmu->pmu, name, -1);
>>>> + if (ret) {
>>>> + dev_err(cpa_pmu->dev, "CPA PMU register failed\n");
>>>> + cpuhp_state_remove_instance_nocalls(
>>>> + CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE, &cpa_pmu->node);
>>>> + }
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int hisi_cpa_pmu_remove(struct platform_device *pdev)
>>>> +{
>>>> + struct hisi_pmu *cpa_pmu = platform_get_drvdata(pdev);
>>>> +
>>>> + perf_pmu_unregister(&cpa_pmu->pmu);
>>>> +
>>>> cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
>>>> + &cpa_pmu->node);
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static struct platform_driver hisi_cpa_pmu_driver = {
>>>> + .driver = {
>>>> + .name = "hisi_cpa_pmu",
>>>> + .acpi_match_table = ACPI_PTR(hisi_cpa_pmu_acpi_match),
>>>> + .suppress_bind_attrs = true,
>>>> + },
>>>> + .probe = hisi_cpa_pmu_probe,
>>>> + .remove = hisi_cpa_pmu_remove,
>>>> +};
>>>> +
>>>> +static int __init hisi_cpa_pmu_module_init(void)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
>>>> + "AP_PERF_ARM_HISI_CPA_ONLINE",
>>>> + hisi_uncore_pmu_online_cpu,
>>>> + hisi_uncore_pmu_offline_cpu);
>>>> + if (ret) {
>>>> + pr_err("CPA PMU: setup hotplug: %d\n", ret);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + ret = platform_driver_register(&hisi_cpa_pmu_driver);
>>>> + if (ret)
>>>> + cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE);
>>>
>>> I am not being totally serious, but this pattern of registering a CPU
>>> hotplug handler and then a driver is so common that we could nearly
>>> add a wrapper for it.
>>>
>>
>> Hi John, do you mean somthing like this?
>>
>> in hisi_uncore_pmu.c:
>>
>> int hisi_uncore_pmu_module_init(enum cpuhp_state state, const char
>> *name, struct platform_driver *drv)
>> {
>> int ret;
>>
>> ret = cpuhp_setup_state_multi(state, name,
>> hisi_uncore_pmu_online_cpu,
>> hisi_uncore_pmu_offline_cpu);
>> if (ret) {
>> pr_err("%s: setup hotplug: %d\n", drv->driver.name, ret);
>> return ret;
>> }
>>
>> ret = platform_driver_register(drv);
>> if (ret)
>> cpuhp_remove_multi_state(state);
>>
>> return ret;
>> }
>>
>> in hisi_uncore_cpa_pmu.c:
>>
>> static int __init hisi_cpa_pmu_module_init(void)
>> {
>> int ret;
>>
>> ret = hisi_uncore_pmu_module_init(CPUHP_AP_PERF_ARM_HISI_CPA_ONLINE,
>> "AP_PERF_ARM_HISI_CPA_ONLINE",
>> &hisi_cpa_pmu_driver);
>>
>> return ret;
>> }
>> module_init(hisi_cpa_pmu_module_init);
>
> Sure, but I'm talking about going further and having this as
> drivers/perf or even platform_device.h helper:
>
> diff --git a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
> b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
> index 62299ab5a9be..22f635260a5f 100644
> --- a/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
> +++ b/drivers/perf/hisilicon/hisi_uncore_ddrc_pmu.c
> @@ -562,26 +562,11 @@ static struct platform_driver hisi_ddrc_pmu_driver
> = {
> .remove = hisi_ddrc_pmu_remove,
> };
>
> -static int __init hisi_ddrc_pmu_module_init(void)
> -{
> - int ret;
> -
> - ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_HISI_DDRC_ONLINE,
> - "AP_PERF_ARM_HISI_DDRC_ONLINE",
> - hisi_uncore_pmu_online_cpu,
> - hisi_uncore_pmu_offline_cpu);
> - if (ret) {
> - pr_err("DDRC PMU: setup hotplug, ret = %d\n", ret);
> - return ret;
> - }
> -
> - ret = platform_driver_register(&hisi_ddrc_pmu_driver);
> - if (ret)
> - cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_HISI_DDRC_ONLINE);
> +module_platform_driver_cpu_hotplug(hisi_ddrc_pmu_driver,
> + AP_PERF_ARM_HISI_DDRC_ONLINE,
> + hisi_uncore_pmu_online_cpu,
> + hisi_uncore_pmu_online_cpu);
>
> - return ret;
> -}
> -module_init(hisi_ddrc_pmu_module_init);
>
> static void __exit hisi_ddrc_pmu_module_exit(void)
> {
> diff --git a/include/linux/platform_device.h
> b/include/linux/platform_device.h
> index 7c96f169d274..d0816dfc0637 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -261,6 +261,28 @@ static inline void platform_set_drvdata(struct
> platform_device *pdev,
> #define builtin_platform_driver(__platform_driver) \
> builtin_driver(__platform_driver, platform_driver_register)
>
> +#define module_platform_driver_cpu_hotplug(__platform_driver,
> cpuhp_state, online, offline) \
> +static int __init
> __module_platform_driver_cpu_hotplug##_init(void) \
> +{ \
> + int ret; \
> + ret = cpuhp_setup_state_multi(CPUHP_##cpuhp_state, \
> + "##cpuhp_state", \
> + online, \
> + offline); \
> + if (ret) { \
> + pr_err("setup hotplug, ret = %d\n", ret); \
> + return ret; \
> + } \
> + \
> + ret = platform_driver_register(&__platform_driver); \
> + if (ret) \
> + cpuhp_remove_multi_state(CPUHP_##cpuhp_state); \
> + \
> + return ret; \
> +} \
> +module_init(__module_platform_driver_cpu_hotplug##_init);
> +
> +
got it, I think we could make another patch to do these cleanup.
Thanks,
Qi
> /* module_platform_driver_probe() - Helper macro for drivers that
> don't do
> * anything special in module init/exit. This eliminates a lot of
> * boilerplate. Each module may only use this macro once, and
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-02-18 8:07 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-14 11:42 [PATCH 0/2] Add Support for HiSilicon CPA PMU Qi Liu
2022-02-14 11:42 ` [PATCH 1/2] drivers/perf: hisi: Add Support for " Qi Liu
2022-02-15 10:47 ` John Garry
2022-02-16 3:16 ` liuqi (BA)
2022-02-17 18:11 ` John Garry
2022-02-18 8:07 ` liuqi (BA)
2022-02-14 11:42 ` [PATCH 2/2] perf jevents: Add support for HiSilicon CPA PMU aliasing Qi Liu
2022-02-14 12:04 ` John Garry
2022-02-15 1:27 ` liuqi (BA)
2022-02-15 1:23 ` liuqi (BA)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).