[PATCH 0/6] Marvell Odyssey uncore performance monitor support

[PATCH 0/6] Marvell Odyssey uncore performance monitor support

[Gowthami Thiagarajan]

This series of patches introduces support for uncore performance monitor
units (PMUs) on the Marvell Odyssey platform. The PMUs covered in this
series include the PEM PMU, LLC-TAD PMU and DDR PMU.

Patches 1, 3 and 5 adds support for PEM pmu, LLC-TAD pmu, DDR pmu
respectively.
Patches 2, 4 and 6 deal with their respective device tree bindings.

Gowthami Thiagarajan (6):
  perf/marvell: Marvell PEM performance monitor support
  dt-bindings: perf: marvell: Add YAML schemas for Marvell PEM pmu
  perf/marvell : Odyssey LLC-TAD performance monitor support
  dt-bindings: perf: marvell: Add YAML schemas for Marvell Odyssey
    LLC-TAD pmu
  perf/marvell: Odyssey DDR Performance monitor support
  dt-bindings: Add YAML schemas for Marvell Odyssey DDR PMU

 .../bindings/perf/marvell-cn10k-ddr.yaml      |   9 +-
 .../bindings/perf/marvell-odyssey-pem.yaml    |  38 ++
 .../bindings/perf/marvell-odyssey-tad.yaml    |  63 +++
 MAINTAINERS                                   |  15 +
 drivers/perf/Kconfig                          |  14 +
 drivers/perf/Makefile                         |   2 +
 drivers/perf/marvell_cn10k_ddr_pmu.c          | 404 +++++++++++++---
 drivers/perf/marvell_odyssey_tad_pmu.c        | 406 ++++++++++++++++
 drivers/perf/marvell_pem_pmu.c                | 433 ++++++++++++++++++
 include/linux/cpuhotplug.h                    |   1 +
 10 files changed, 1318 insertions(+), 67 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/perf/marvell-odyssey-pem.yaml
 create mode 100644 Documentation/devicetree/bindings/perf/marvell-odyssey-tad.yaml
 create mode 100644 drivers/perf/marvell_odyssey_tad_pmu.c
 create mode 100644 drivers/perf/marvell_pem_pmu.c

-- 
2.25.1

[PATCH 1/6] perf/marvell: Marvell PEM performance monitor support

[Gowthami Thiagarajan]

PCI Express Interface PMU includes various performance counters to monitor
the data that is transmitted over the PCIe link. The counters track various
inbound and outbound transactions which includes separate counters for
posted/non-posted/completion TLPs. Also, inbound and outbound memory read
requests along with their latencies can also be monitored. Address
Translation Services(ATS)events such as ATS Translation, ATS Page Request,
ATS Invalidation along with their corresponding latencies are also
supported.

The performance counters are 64 bits wide.

For instance,
perf stat -e ib_tlp_pr <workload>
tracks the inbound posted TLPs for the workload.

Signed-off-by: Linu Cherian <lcherian@marvell.com>
Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com>
---
 MAINTAINERS                    |   7 +
 drivers/perf/Kconfig           |   7 +
 drivers/perf/Makefile          |   1 +
 drivers/perf/marvell_pem_pmu.c | 433 +++++++++++++++++++++++++++++++++
 include/linux/cpuhotplug.h     |   1 +
 5 files changed, 449 insertions(+)
 create mode 100644 drivers/perf/marvell_pem_pmu.c

diff --git a/MAINTAINERS b/MAINTAINERS
index c6545eb54104..55a2a9b6f346 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12473,6 +12473,13 @@ S:	Supported
 F:	Documentation/networking/device_drivers/ethernet/marvell/octeontx2.rst
 F:	drivers/net/ethernet/marvell/octeontx2/af/
 
+MARVELL PEM PMU DRIVER
+M:	Linu Cherian <lcherian@marvell.com>
+M:	Gowthami Thiagarajan <gthiagarajan@marvell.com>
+S:	Supported
+F:	Documentation/devicetree/bindings/perf/marvell-odyssey-pem.yaml
+F:	drivers/perf/marvell_pem_pmu.c
+
 MARVELL PRESTERA ETHERNET SWITCH DRIVER
 M:	Taras Chornyi <taras.chornyi@plvision.eu>
 S:	Supported
diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
index 66c259000a44..1cd8d07ffefd 100644
--- a/drivers/perf/Kconfig
+++ b/drivers/perf/Kconfig
@@ -203,4 +203,11 @@ source "drivers/perf/arm_cspmu/Kconfig"
 
 source "drivers/perf/amlogic/Kconfig"
 
+config MARVELL_PEM_PMU
+	tristate "MARVELL PEM PMU Support"
+	depends on ARCH_THUNDER || (COMPILE_TEST && 64BIT)
+	help
+	  Enable support for PCIe Interface performance monitoring
+	  on Marvell platform.
+
 endmenu
diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
index 13e45da61100..bf9fe9cacad9 100644
--- a/drivers/perf/Makefile
+++ b/drivers/perf/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_ARM_SPE_PMU) += arm_spe_pmu.o
 obj-$(CONFIG_ARM_DMC620_PMU) += arm_dmc620_pmu.o
 obj-$(CONFIG_MARVELL_CN10K_TAD_PMU) += marvell_cn10k_tad_pmu.o
 obj-$(CONFIG_MARVELL_CN10K_DDR_PMU) += marvell_cn10k_ddr_pmu.o
+obj-$(CONFIG_MARVELL_PEM_PMU) += marvell_pem_pmu.o
 obj-$(CONFIG_APPLE_M1_CPU_PMU) += apple_m1_cpu_pmu.o
 obj-$(CONFIG_ALIBABA_UNCORE_DRW_PMU) += alibaba_uncore_drw_pmu.o
 obj-$(CONFIG_ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU) += arm_cspmu/
diff --git a/drivers/perf/marvell_pem_pmu.c b/drivers/perf/marvell_pem_pmu.c
new file mode 100644
index 000000000000..fb27112aa7d4
--- /dev/null
+++ b/drivers/perf/marvell_pem_pmu.c
@@ -0,0 +1,433 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Marvell PEM(PCIe RC) Performance Monitor Driver
+ *
+ * Copyright (C) 2023 Marvell.
+ */
+
+#include <linux/acpi.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/perf_event.h>
+
+/* Each of these events maps to a free running 64 bit counter
+ * with no event control, but can be reset.
+ *
+ */
+enum pem_events {
+	IB_TLP_NPR,
+	IB_TLP_PR,
+	IB_TLP_CPL,
+	IB_TLP_DWORDS_NPR,
+	IB_TLP_DWORDS_PR,
+	IB_TLP_DWORDS_CPL,
+	IB_INFLIGHT,
+	IB_READS,
+	IB_REQ_NO_RO_NCB,
+	IB_REQ_NO_RO_EBUS,
+	OB_TLP_NPR,
+	OB_TLP_PR,
+	OB_TLP_CPL,
+	OB_TLP_DWORDS_NPR,
+	OB_TLP_DWORDS_PR,
+	OB_TLP_DWORDS_CPL,
+	OB_INFLIGHT,
+	OB_READS,
+	OB_MERGES_NPR,
+	OB_MERGES_PR,
+	OB_MERGES_CPL,
+	ATS_TRANS,
+	ATS_TRANS_LATENCY,
+	ATS_PRI,
+	ATS_PRI_LATENCY,
+	ATS_INV,
+	ATS_INV_LATENCY,
+	PEM_EVENTIDS_MAX,
+};
+
+static u64 eventid_to_offset_table[] = {
+	0x0,
+	0x8,
+	0x10,
+	0x100,
+	0x108,
+	0x110,
+	0x200,
+	0x300,
+	0x400,
+	0x408,
+	0x500,
+	0x508,
+	0x510,
+	0x600,
+	0x608,
+	0x610,
+	0x700,
+	0x800,
+	0x900,
+	0x908,
+	0x910,
+	0x2D18,
+	0x2D20,
+	0x2D28,
+	0x2D30,
+	0x2D38,
+	0x2D40,
+};
+
+struct pem_pmu {
+	struct pmu pmu;
+	void __iomem *base;
+	unsigned int cpu;
+	struct	device *dev;
+	struct hlist_node node;
+};
+
+#define to_pem_pmu(p)	container_of(p, struct pem_pmu, pmu)
+
+static int eventid_to_offset(int eventid)
+{
+	return eventid_to_offset_table[eventid];
+}
+
+/* Events */
+static ssize_t pem_pmu_event_show(struct device *dev,
+				  struct device_attribute *attr,
+				  char *page)
+{
+	struct perf_pmu_events_attr *pmu_attr;
+
+	pmu_attr = container_of(attr, struct perf_pmu_events_attr, attr);
+	return sysfs_emit(page, "event=0x%02llx\n", pmu_attr->id);
+}
+
+#define PEM_EVENT_ATTR(_name, _id)					\
+	(&((struct perf_pmu_events_attr[]) {				\
+	{ .attr = __ATTR(_name, 0444, pem_pmu_event_show, NULL),	\
+		.id = _id, }						\
+	})[0].attr.attr)
+
+static struct attribute *pem_perf_events_attrs[] = {
+	PEM_EVENT_ATTR(ib_tlp_npr, IB_TLP_NPR),
+	PEM_EVENT_ATTR(ib_tlp_pr, IB_TLP_PR),
+	PEM_EVENT_ATTR(ib_tlp_cpl_partid, IB_TLP_CPL),
+	PEM_EVENT_ATTR(ib_tlp_dwords_npr, IB_TLP_DWORDS_NPR),
+	PEM_EVENT_ATTR(ib_tlp_dwords_pr, IB_TLP_DWORDS_PR),
+	PEM_EVENT_ATTR(ib_tlp_dwords_cpl_partid, IB_TLP_DWORDS_CPL),
+	PEM_EVENT_ATTR(ib_inflight, IB_INFLIGHT),
+	PEM_EVENT_ATTR(ib_reads, IB_READS),
+	PEM_EVENT_ATTR(ib_req_no_ro_ncb, IB_REQ_NO_RO_NCB),
+	PEM_EVENT_ATTR(ib_req_no_ro_ebus, IB_REQ_NO_RO_EBUS),
+	PEM_EVENT_ATTR(ob_tlp_npr_partid, OB_TLP_NPR),
+	PEM_EVENT_ATTR(ob_tlp_pr_partid, OB_TLP_PR),
+	PEM_EVENT_ATTR(ob_tlp_cpl_partid, OB_TLP_CPL),
+	PEM_EVENT_ATTR(ob_tlp_dwords_npr_partid, OB_TLP_DWORDS_NPR),
+	PEM_EVENT_ATTR(ob_tlp_dwords_pr_partid, OB_TLP_DWORDS_PR),
+	PEM_EVENT_ATTR(ob_tlp_dwords_cpl_partid, OB_TLP_DWORDS_CPL),
+	PEM_EVENT_ATTR(ob_inflight_partid, OB_INFLIGHT),
+	PEM_EVENT_ATTR(ob_reads_partid, OB_READS),
+	PEM_EVENT_ATTR(ob_merges_npr_partid, OB_MERGES_NPR),
+	PEM_EVENT_ATTR(ob_merges_pr_partid, OB_MERGES_PR),
+	PEM_EVENT_ATTR(ob_merges_cpl_partid, OB_MERGES_CPL),
+	PEM_EVENT_ATTR(ats_trans, ATS_TRANS),
+	PEM_EVENT_ATTR(ats_trans_latency, ATS_TRANS_LATENCY),
+	PEM_EVENT_ATTR(ats_pri, ATS_PRI),
+	PEM_EVENT_ATTR(ats_pri_latency, ATS_PRI_LATENCY),
+	PEM_EVENT_ATTR(ats_inv, ATS_INV),
+	PEM_EVENT_ATTR(ats_inv_latency, ATS_INV_LATENCY),
+	NULL
+};
+
+static struct attribute_group pem_perf_events_attr_group = {
+	.name = "events",
+	.attrs = pem_perf_events_attrs,
+};
+
+PMU_FORMAT_ATTR(event, "config:0-5");
+
+static struct attribute *pem_perf_format_attrs[] = {
+	&format_attr_event.attr,
+	NULL
+};
+
+static struct attribute_group pem_perf_format_attr_group = {
+	.name = "format",
+	.attrs = pem_perf_format_attrs,
+};
+
+/* cpumask */
+static ssize_t pem_perf_cpumask_show(struct device *dev,
+				     struct device_attribute *attr,
+				     char *buf)
+{
+	struct pem_pmu *pmu = dev_get_drvdata(dev);
+
+	return cpumap_print_to_pagebuf(true, buf, cpumask_of(pmu->cpu));
+}
+
+static struct device_attribute pem_perf_cpumask_attr =
+	__ATTR(cpumask, 0444, pem_perf_cpumask_show, NULL);
+
+static struct attribute *pem_perf_cpumask_attrs[] = {
+	&pem_perf_cpumask_attr.attr,
+	NULL
+};
+
+static struct attribute_group pem_perf_cpumask_attr_group = {
+	.attrs = pem_perf_cpumask_attrs,
+};
+
+static const struct attribute_group *pem_perf_attr_groups[] = {
+	&pem_perf_events_attr_group,
+	&pem_perf_cpumask_attr_group,
+	&pem_perf_format_attr_group,
+	NULL
+};
+
+static int pem_perf_event_init(struct perf_event *event)
+{
+	struct pem_pmu *pmu = to_pem_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+
+	if (event->attr.type != event->pmu->type)
+		return -ENOENT;
+
+	if (is_sampling_event(event)) {
+		dev_info(pmu->dev, "Sampling not supported!\n");
+		return -EOPNOTSUPP;
+	}
+
+	if (event->cpu < 0) {
+		dev_warn(pmu->dev, "Can't provide per-task data!\n");
+		return -EOPNOTSUPP;
+	}
+
+	/*  We must NOT create groups containing mixed PMUs */
+	if (event->group_leader->pmu != event->pmu &&
+	    !is_software_event(event->group_leader))
+		return -EINVAL;
+
+	/* Set ownership of event to one CPU, same event can not be observed
+	 * on multiple cpus at same time.
+	 */
+	event->cpu = pmu->cpu;
+	hwc->idx = -1;
+	return 0;
+}
+
+static void pem_perf_counter_reset(struct pem_pmu *pmu,
+				   struct perf_event *event, int eventid)
+{
+	writeq_relaxed(0x0, pmu->base + eventid_to_offset(eventid));
+}
+
+static u64 pem_perf_read_counter(struct pem_pmu *pmu,
+				 struct perf_event *event, int eventid)
+{
+	return readq_relaxed(pmu->base + eventid_to_offset(eventid));
+}
+
+static void pem_perf_event_update(struct perf_event *event)
+{
+	struct pem_pmu *pmu = to_pem_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+	u64 prev_count, new_count;
+
+	do {
+		prev_count = local64_read(&hwc->prev_count);
+		new_count = pem_perf_read_counter(pmu, event, hwc->idx);
+	} while (local64_xchg(&hwc->prev_count, new_count) != prev_count);
+
+	local64_add((new_count - prev_count), &event->count);
+}
+
+static void pem_perf_event_start(struct perf_event *event, int flags)
+{
+	struct pem_pmu *pmu = to_pem_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+	int eventid = hwc->idx;
+
+	local64_set(&hwc->prev_count, 0);
+
+	pem_perf_counter_reset(pmu, event, eventid);
+
+	hwc->state = 0;
+}
+
+static int pem_perf_event_add(struct perf_event *event, int flags)
+{
+	struct hw_perf_event *hwc = &event->hw;
+
+	hwc->idx = event->attr.config;
+	if (hwc->idx >= PEM_EVENTIDS_MAX)
+		return -EINVAL;
+	hwc->state |= PERF_HES_STOPPED;
+
+	if (flags & PERF_EF_START)
+		pem_perf_event_start(event, flags);
+
+	return 0;
+}
+
+static void pem_perf_event_stop(struct perf_event *event, int flags)
+{
+	struct hw_perf_event *hwc = &event->hw;
+
+	if (flags & PERF_EF_UPDATE)
+		pem_perf_event_update(event);
+
+	hwc->state |= PERF_HES_STOPPED;
+}
+
+static void pem_perf_event_del(struct perf_event *event, int flags)
+{
+	struct hw_perf_event *hwc = &event->hw;
+
+	pem_perf_event_stop(event, PERF_EF_UPDATE);
+	hwc->idx = -1;
+}
+
+static int pem_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
+{
+	struct pem_pmu *pmu = hlist_entry_safe(node, struct pem_pmu,
+					       node);
+	unsigned int target;
+
+	if (cpu != pmu->cpu)
+		return 0;
+
+	target = cpumask_any_but(cpu_online_mask, cpu);
+	if (target >= nr_cpu_ids)
+		return 0;
+
+	perf_pmu_migrate_context(&pmu->pmu, cpu, target);
+	pmu->cpu = target;
+	return 0;
+}
+
+static int pem_perf_probe(struct platform_device *pdev)
+{
+	struct pem_pmu *pem_pmu;
+	struct resource *res;
+	void __iomem *base;
+	char *name;
+	int ret;
+
+	pem_pmu = devm_kzalloc(&pdev->dev, sizeof(*pem_pmu), GFP_KERNEL);
+	if (!pem_pmu)
+		return -ENOMEM;
+
+	pem_pmu->dev = &pdev->dev;
+	platform_set_drvdata(pdev, pem_pmu);
+
+	base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	pem_pmu->base = base;
+
+	pem_pmu->pmu = (struct pmu) {
+		.module	      = THIS_MODULE,
+		.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
+		.task_ctx_nr = perf_invalid_context,
+		.attr_groups = pem_perf_attr_groups,
+		.event_init  = pem_perf_event_init,
+		.add	     = pem_perf_event_add,
+		.del	     = pem_perf_event_del,
+		.start	     = pem_perf_event_start,
+		.stop	     = pem_perf_event_stop,
+		.read	     = pem_perf_event_update,
+	};
+
+	/* Choose this cpu to collect perf data */
+	pem_pmu->cpu = raw_smp_processor_id();
+
+	name = devm_kasprintf(pem_pmu->dev, GFP_KERNEL, "mrvl_pcie_rc_pmu_%llx",
+			      res->start);
+	if (!name)
+		return -ENOMEM;
+
+	cpuhp_state_add_instance_nocalls(CPUHP_AP_PERF_ARM_MARVELL_PEM_ONLINE,
+					 &pem_pmu->node);
+
+	ret = perf_pmu_register(&pem_pmu->pmu, name, -1);
+	if (ret)
+		goto error;
+
+	pr_info("Marvell PEM(PCIe RC) PMU Driver for pem@%llx\n", res->start);
+	return 0;
+error:
+	cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_MARVELL_PEM_ONLINE,
+					    &pem_pmu->node);
+	return ret;
+}
+
+static int pem_perf_remove(struct platform_device *pdev)
+{
+	struct pem_pmu *pem_pmu = platform_get_drvdata(pdev);
+
+	cpuhp_state_remove_instance_nocalls(CPUHP_AP_PERF_ARM_MARVELL_PEM_ONLINE,
+					    &pem_pmu->node);
+
+	perf_pmu_unregister(&pem_pmu->pmu);
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id pem_pmu_of_match[] = {
+	{ .compatible = "marvell,pem-pmu", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, pem_pmu_of_match);
+#endif
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id pem_pmu_acpi_match[] = {
+	{"MRVL000E", 0},
+	{},
+};
+MODULE_DEVICE_TABLE(acpi, pem_pmu_acpi_match);
+#endif
+
+static struct platform_driver pem_pmu_driver = {
+	.driver	= {
+		.name   = "pem-pmu",
+		.of_match_table = of_match_ptr(pem_pmu_of_match),
+		.acpi_match_table = ACPI_PTR(pem_pmu_acpi_match),
+		.suppress_bind_attrs = true,
+	},
+	.probe		= pem_perf_probe,
+	.remove		= pem_perf_remove,
+};
+
+static int __init pem_pmu_init(void)
+{
+	int ret;
+
+	ret = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_MARVELL_PEM_ONLINE,
+				      "perf/marvell/pem:online", NULL,
+				       pem_pmu_offline_cpu);
+	if (ret)
+		return ret;
+
+	ret = platform_driver_register(&pem_pmu_driver);
+	if (ret)
+		cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_MARVELL_PEM_ONLINE);
+	return ret;
+}
+
+static void __exit pem_pmu_exit(void)
+{
+	platform_driver_unregister(&pem_pmu_driver);
+	cpuhp_remove_multi_state(CPUHP_AP_PERF_ARM_MARVELL_PEM_ONLINE);
+}
+
+module_init(pem_pmu_init);
+module_exit(pem_pmu_exit);
+
+MODULE_DESCRIPTION("Marvell PEM Perf driver");
+MODULE_AUTHOR("Linu Cherian <lcherian@marvell.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
index 0f1001dca0e0..f7710c03d24e 100644
--- a/include/linux/cpuhotplug.h
+++ b/include/linux/cpuhotplug.h
@@ -235,6 +235,7 @@ enum cpuhp_state {
 	CPUHP_AP_PERF_ARM_APM_XGENE_ONLINE,
 	CPUHP_AP_PERF_ARM_CAVIUM_TX2_UNCORE_ONLINE,
 	CPUHP_AP_PERF_ARM_MARVELL_CN10K_DDR_ONLINE,
+	CPUHP_AP_PERF_ARM_MARVELL_PEM_ONLINE,
 	CPUHP_AP_PERF_POWERPC_NEST_IMC_ONLINE,
 	CPUHP_AP_PERF_POWERPC_CORE_IMC_ONLINE,
 	CPUHP_AP_PERF_POWERPC_THREAD_IMC_ONLINE,
-- 
2.25.1

[PATCH 2/6] dt-bindings: perf: marvell: Add YAML schemas for Marvell PEM pmu

[Gowthami Thiagarajan]

Add device tree bindings for Marvell PEM performance monitor unit

Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com>
Signed-off-by: Linu Cherian <lcherian@marvell.com>
---
 .../bindings/perf/marvell-odyssey-pem.yaml    | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/perf/marvell-odyssey-pem.yaml

diff --git a/Documentation/devicetree/bindings/perf/marvell-odyssey-pem.yaml b/Documentation/devicetree/bindings/perf/marvell-odyssey-pem.yaml
new file mode 100644
index 000000000000..6af201fbccd8
--- /dev/null
+++ b/Documentation/devicetree/bindings/perf/marvell-odyssey-pem.yaml
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/perf/marvell-odyssey-pem.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Marvell Odyssey PCIe interface performance monitor
+
+maintainers:
+  - Linu Cherian <lcherian@marvell.com>
+  - Gowthami Thiagarajan <gthiagarajan@marvell.com>
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - marvell,pem-pmu
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    bus {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        pmu@8e0000005000 {
+            compatible = "marvell,pem-pmu";
+            reg = <0x8E00 0x00005000 0x0 0x3000>;
+        };
+    };
-- 
2.25.1

[PATCH 3/6] perf/marvell : Odyssey LLC-TAD performance monitor support

[Gowthami Thiagarajan]

Each TAD provides eight 64-bit counters for monitoring
cache behavior.The driver always configures the same counter for
all the TADs. The user would end up effectively reserving one of
eight counters in every TAD to look across all TADs.
The occurrences of events are aggregated and presented to the user
at the end of running the workload. The driver does not provide a
way for the user to partition TADs so that different TADs are used for
different applications.

The performance events reflect various internal or interface activities.
By combining the values from multiple performance counters, cache
performance can be measured in terms such as: cache miss rate, cache
allocations, interface retry rate, internal resource occupancy, etc.

Each supported counter's event and formatting information is exposed
to sysfs at /sys/devices/tad/. Use perf tool stat command to measure
the pmu events. For instance:

perf stat -e tad_hit_ltg,tad_hit_dtg <workload>

Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com>
---
 MAINTAINERS                            |   8 +
 drivers/perf/Kconfig                   |   7 +
 drivers/perf/Makefile                  |   1 +
 drivers/perf/marvell_odyssey_tad_pmu.c | 406 +++++++++++++++++++++++++
 4 files changed, 422 insertions(+)
 create mode 100644 drivers/perf/marvell_odyssey_tad_pmu.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 55a2a9b6f346..bbf3a97502db 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12512,6 +12512,14 @@ L:	netdev@vger.kernel.org
 S:	Supported
 F:	drivers/net/ethernet/marvell/octeon_ep
 
+MARVELL ODYSSEY TAD PMU DRIVER
+M:	Gowthami Thiagarajan <gthiagarajan@marvell.com>
+M:	Bharat Bhushan <bbhushan2@marvell.com>
+M:	Linu Cherian <lcherian@marvell.com>
+M:	George Cherian <gcherian@marvell.com>
+S:	Supported
+F:	drivers/perf/marvell_odyssey_tad_pmu.c
+
 MATROX FRAMEBUFFER DRIVER
 L:	linux-fbdev@vger.kernel.org
 S:	Orphan
diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
index 1cd8d07ffefd..2dc649768c1b 100644
--- a/drivers/perf/Kconfig
+++ b/drivers/perf/Kconfig
@@ -210,4 +210,11 @@ config MARVELL_PEM_PMU
 	  Enable support for PCIe Interface performance monitoring
 	  on Marvell platform.
 
+config MARVELL_ODYSSEY_TAD_PMU
+        tristate "MARVELL ODYSSEY LLC-TAD PMU"
+        depends on ARCH_THUNDER || (COMPILE_TEST && 64BIT)
+        help
+          Provides support for Last-Level cache Tag-and-data Units (LLC-TAD)
+          performance monitor on Odyssey platform
+
 endmenu
diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
index bf9fe9cacad9..5dc1438e7d3d 100644
--- a/drivers/perf/Makefile
+++ b/drivers/perf/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_ARM_DMC620_PMU) += arm_dmc620_pmu.o
 obj-$(CONFIG_MARVELL_CN10K_TAD_PMU) += marvell_cn10k_tad_pmu.o
 obj-$(CONFIG_MARVELL_CN10K_DDR_PMU) += marvell_cn10k_ddr_pmu.o
 obj-$(CONFIG_MARVELL_PEM_PMU) += marvell_pem_pmu.o
+obj-$(CONFIG_MARVELL_ODYSSEY_TAD_PMU) += marvell_odyssey_tad_pmu.o
 obj-$(CONFIG_APPLE_M1_CPU_PMU) += apple_m1_cpu_pmu.o
 obj-$(CONFIG_ALIBABA_UNCORE_DRW_PMU) += alibaba_uncore_drw_pmu.o
 obj-$(CONFIG_ARM_CORESIGHT_PMU_ARCH_SYSTEM_PMU) += arm_cspmu/
diff --git a/drivers/perf/marvell_odyssey_tad_pmu.c b/drivers/perf/marvell_odyssey_tad_pmu.c
new file mode 100644
index 000000000000..8f0204c88539
--- /dev/null
+++ b/drivers/perf/marvell_odyssey_tad_pmu.c
@@ -0,0 +1,406 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Marvell Odyssey LLC-TAD perf driver
+ *
+ * Copyright (C) 2023 Marvell.
+ */
+
+#define pr_fmt(fmt) "tad_pmu: " fmt
+
+#include <linux/acpi.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/cpuhotplug.h>
+#include <linux/perf_event.h>
+#include <linux/platform_device.h>
+
+#define TAD_PFC_OFFSET		0x0800
+#define TAD_PFC(counter)	(TAD_PFC_OFFSET | ((counter) << 3))
+#define TAD_PRF_OFFSET		0x0900
+#define TAD_PRF(counter)	(TAD_PRF_OFFSET | ((counter) << 3))
+#define TAD_PRF_CNTSEL_MASK	0xFF
+#define TAD_MAX_COUNTERS	8
+
+#define to_tad_pmu(p) (container_of(p, struct tad_pmu, pmu))
+
+struct tad_region {
+	void __iomem	*base;
+};
+
+struct tad_pmu {
+	struct pmu pmu;
+	struct tad_region *regions;
+	u32 region_cnt;
+	unsigned int cpu;
+	struct hlist_node node;
+	struct perf_event *events[TAD_MAX_COUNTERS];
+	DECLARE_BITMAP(counters_map, TAD_MAX_COUNTERS);
+};
+
+static int tad_pmu_cpuhp_state;
+
+static void tad_pmu_event_counter_read(struct perf_event *event)
+{
+	struct tad_pmu *tad_pmu = to_tad_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+	u32 counter_idx = hwc->idx;
+	u64 delta, prev, new;
+	int tad_region;
+
+	do {
+		prev = local64_read(&hwc->prev_count);
+		for (tad_region = 0, new = 0; tad_region < tad_pmu->region_cnt; tad_region++)
+			new += readq(tad_pmu->regions[tad_region].base +
+				     TAD_PFC(counter_idx));
+	} while (local64_cmpxchg(&hwc->prev_count, prev, new) != prev);
+
+	delta = (new - prev) & GENMASK_ULL(63, 0);
+	local64_add(delta, &event->count);
+}
+
+static void tad_pmu_event_counter_stop(struct perf_event *event, int flags)
+{
+	struct tad_pmu *tad_pmu = to_tad_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+	u32 counter_idx = hwc->idx;
+	int tad_region;
+
+	/* TAD()_PFC() stop counting on the write
+	 * which sets TAD()_PRF()[CNTSEL] == 0
+	 */
+	for (tad_region = 0; tad_region < tad_pmu->region_cnt; tad_region++)
+		writeq(0, tad_pmu->regions[tad_region].base + TAD_PRF(counter_idx));
+
+	tad_pmu_event_counter_read(event);
+	hwc->state |= PERF_HES_STOPPED | PERF_HES_UPTODATE;
+}
+
+static void tad_pmu_event_counter_start(struct perf_event *event, int flags)
+{
+	struct tad_pmu *tad_pmu = to_tad_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+	u32 event_idx = event->attr.config;
+	u32 counter_idx = hwc->idx;
+	u64 reg_val;
+	int tad_region;
+
+	hwc->state = 0;
+
+	/* Typically TAD_PFC() are zeroed to start counting */
+	for (tad_region = 0; tad_region < tad_pmu->region_cnt; tad_region++)
+		writeq(0, tad_pmu->regions[tad_region].base + TAD_PFC(counter_idx));
+
+	/* TAD()_PFC() start counting on the write
+	 * which sets TAD()_PRF()[CNTSEL] != 0
+	 */
+	for (tad_region = 0; tad_region < tad_pmu->region_cnt; tad_region++) {
+		reg_val = (event_idx & 0xFF);
+		writeq(reg_val,	tad_pmu->regions[tad_region].base +
+				TAD_PRF(counter_idx));
+	}
+}
+
+static void tad_pmu_event_counter_del(struct perf_event *event, int flags)
+{
+	struct tad_pmu *tad_pmu = to_tad_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+	int idx = hwc->idx;
+
+	tad_pmu_event_counter_stop(event, flags | PERF_EF_UPDATE);
+	tad_pmu->events[idx] = NULL;
+	clear_bit(idx, tad_pmu->counters_map);
+}
+
+static int tad_pmu_event_counter_add(struct perf_event *event, int flags)
+{
+	struct tad_pmu *tad_pmu = to_tad_pmu(event->pmu);
+	struct hw_perf_event *hwc = &event->hw;
+	int idx;
+
+	/* Get a free counter for this event */
+	idx = find_first_zero_bit(tad_pmu->counters_map, TAD_MAX_COUNTERS);
+	if (idx == TAD_MAX_COUNTERS)
+		return -EAGAIN;
+
+	set_bit(idx, tad_pmu->counters_map);
+
+	hwc->idx = idx;
+	hwc->state = PERF_HES_STOPPED;
+	tad_pmu->events[idx] = event;
+
+	if (flags & PERF_EF_START)
+		tad_pmu_event_counter_start(event, flags);
+
+	return 0;
+}
+
+static int tad_pmu_event_init(struct perf_event *event)
+{
+	struct tad_pmu *tad_pmu = to_tad_pmu(event->pmu);
+
+	if (event->attr.type != event->pmu->type)
+		return -ENOENT;
+
+	if (!event->attr.disabled)
+		return -EINVAL;
+
+	if (event->state != PERF_EVENT_STATE_OFF)
+		return -EINVAL;
+
+	event->cpu = tad_pmu->cpu;
+	event->hw.idx = -1;
+	event->hw.config_base = event->attr.config;
+
+	return 0;
+}
+
+static ssize_t tad_pmu_event_show(struct device *dev,
+				  struct device_attribute *attr, char *page)
+{
+	struct perf_pmu_events_attr *pmu_attr;
+
+	pmu_attr = container_of(attr, struct perf_pmu_events_attr, attr);
+	return sprintf(page, "event=0x%02llx\n", pmu_attr->id);
+}
+
+#define TAD_PMU_EVENT_ATTR(_name, _id)					\
+	(&((struct perf_pmu_events_attr[]) {				\
+		{ .attr = __ATTR(_name, 0444, tad_pmu_event_show, NULL),\
+		  .id = _id, }						\
+	})[0].attr.attr)
+
+static struct attribute *tad_pmu_event_attrs[] = {
+	TAD_PMU_EVENT_ATTR(tad_none, 0x0),
+	TAD_PMU_EVENT_ATTR(tad_req_msh_in_exlmn, 0x3),
+	TAD_PMU_EVENT_ATTR(tad_alloc_dtg, 0x1a),
+	TAD_PMU_EVENT_ATTR(tad_alloc_ltg, 0x1b),
+	TAD_PMU_EVENT_ATTR(tad_alloc_any, 0x1c),
+	TAD_PMU_EVENT_ATTR(tad_hit_dtg, 0x1d),
+	TAD_PMU_EVENT_ATTR(tad_hit_ltg, 0x1e),
+	TAD_PMU_EVENT_ATTR(tad_hit_any, 0x1f),
+	TAD_PMU_EVENT_ATTR(tad_tag_rd, 0x20),
+	TAD_PMU_EVENT_ATTR(tad_tot_cycle, 0xFF),
+	NULL
+};
+
+static const struct attribute_group tad_pmu_events_attr_group = {
+	.name = "events",
+	.attrs = tad_pmu_event_attrs,
+};
+
+PMU_FORMAT_ATTR(event, "config:0-7");
+
+static struct attribute *tad_pmu_format_attrs[] = {
+	&format_attr_event.attr,
+	NULL,
+};
+
+static struct attribute_group tad_pmu_format_attr_group = {
+	.name = "format",
+	.attrs = tad_pmu_format_attrs,
+};
+
+static ssize_t tad_pmu_cpumask_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct tad_pmu *tad_pmu = to_tad_pmu(dev_get_drvdata(dev));
+
+	return cpumap_print_to_pagebuf(true, buf, cpumask_of(tad_pmu->cpu));
+}
+
+static DEVICE_ATTR(cpumask, 0444, tad_pmu_cpumask_show, NULL);
+
+static struct attribute *tad_pmu_cpumask_attrs[] = {
+	&dev_attr_cpumask.attr,
+	NULL
+};
+
+static struct attribute_group tad_pmu_cpumask_attr_group = {
+	.attrs = tad_pmu_cpumask_attrs,
+};
+
+static const struct attribute_group *tad_pmu_attr_groups[] = {
+	&tad_pmu_events_attr_group,
+	&tad_pmu_format_attr_group,
+	&tad_pmu_cpumask_attr_group,
+	NULL
+};
+
+static int tad_pmu_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct tad_region *regions;
+	struct tad_pmu *tad_pmu;
+	struct resource *res;
+	u32 tad_pmu_page_size;
+	u32 tad_page_size;
+	u32 tad_cnt;
+	int i, ret;
+	char *name;
+
+	tad_pmu = devm_kzalloc(&pdev->dev, sizeof(*tad_pmu), GFP_KERNEL);
+	if (!tad_pmu)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, tad_pmu);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "Mem resource not found\n");
+		return -ENODEV;
+	}
+
+	ret = device_property_read_u32(dev, "marvell,tad-page-size", &tad_page_size);
+	if (ret) {
+		dev_err(&pdev->dev, "Can't find tad-page-size property\n");
+		return ret;
+	}
+
+	ret = device_property_read_u32(dev, "marvell,tad-pmu-page-size",
+				       &tad_pmu_page_size);
+	if (ret) {
+		dev_err(&pdev->dev, "Can't find tad-pmu-page-size property\n");
+		return ret;
+	}
+
+	ret = device_property_read_u32(dev, "marvell,tad-cnt", &tad_cnt);
+	if (ret) {
+		dev_err(&pdev->dev, "Can't find tad-cnt property\n");
+		return ret;
+	}
+
+	regions = kcalloc(tad_cnt, sizeof(*regions), GFP_KERNEL);
+	if (!regions)
+		return -ENOMEM;
+
+	/* ioremap the distributed TAD pmu regions */
+	for (i = 0; i < tad_cnt && res->start < res->end; i++) {
+		regions[i].base = devm_ioremap(&pdev->dev,
+					       res->start,
+					       tad_pmu_page_size);
+		if (IS_ERR(regions[i].base)) {
+			dev_err(&pdev->dev, "TAD%d ioremap fail\n", i);
+			return -ENOMEM;
+		}
+		res->start += tad_page_size;
+	}
+
+	tad_pmu->regions = regions;
+	tad_pmu->region_cnt = tad_cnt;
+
+	tad_pmu->pmu = (struct pmu) {
+		.module		= THIS_MODULE,
+		.attr_groups	= tad_pmu_attr_groups,
+		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
+		.task_ctx_nr	= perf_invalid_context,
+
+		.event_init	= tad_pmu_event_init,
+		.add		= tad_pmu_event_counter_add,
+		.del		= tad_pmu_event_counter_del,
+		.start		= tad_pmu_event_counter_start,
+		.stop		= tad_pmu_event_counter_stop,
+		.read		= tad_pmu_event_counter_read,
+	};
+
+	tad_pmu->cpu = raw_smp_processor_id();
+
+	/* Register pmu instance for cpu hotplug */
+	ret = cpuhp_state_add_instance_nocalls(tad_pmu_cpuhp_state,
+					       &tad_pmu->node);
+	if (ret) {
+		dev_err(&pdev->dev, "Error %d registering hotplug\n", ret);
+		return ret;
+	}
+
+	name = "tad";
+	ret = perf_pmu_register(&tad_pmu->pmu, name, -1);
+	if (ret)
+		cpuhp_state_remove_instance_nocalls(tad_pmu_cpuhp_state,
+						    &tad_pmu->node);
+
+	return ret;
+}
+
+static int tad_pmu_remove(struct platform_device *pdev)
+{
+	struct tad_pmu *pmu = platform_get_drvdata(pdev);
+
+	cpuhp_state_remove_instance_nocalls(tad_pmu_cpuhp_state,
+					    &pmu->node);
+	perf_pmu_unregister(&pmu->pmu);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id tad_pmu_of_match[] = {
+	{ .compatible = "marvell,odyssey-tad-pmu", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, tad_pmu_of_match);
+#endif
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id tad_pmu_acpi_match[] = {
+	{"MRVL000D", 0},
+	{},
+};
+MODULE_DEVICE_TABLE(acpi, tad_pmu_acpi_match);
+#endif
+
+static struct platform_driver odyssey_tad_pmu_driver = {
+	.driver         = {
+		.name   = "odyssey_tad_pmu",
+		.of_match_table = of_match_ptr(tad_pmu_of_match),
+		.acpi_match_table = ACPI_PTR(tad_pmu_acpi_match),
+		.suppress_bind_attrs = true,
+	},
+	.probe          = tad_pmu_probe,
+	.remove         = tad_pmu_remove,
+};
+
+static int odyssey_tad_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
+{
+	struct tad_pmu *pmu = hlist_entry_safe(node, struct tad_pmu, node);
+	unsigned int target;
+
+	if (cpu != pmu->cpu)
+		return 0;
+
+	target = cpumask_any_but(cpu_online_mask, cpu);
+	if (target >= nr_cpu_ids)
+		return 0;
+
+	perf_pmu_migrate_context(&pmu->pmu, cpu, target);
+	pmu->cpu = target;
+
+	return 0;
+}
+
+static int __init odyssey_tad_pmu_init(void)
+{
+	int ret;
+
+	ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN,
+				      "perf/odyssey/tadpmu:online",
+				      NULL,
+				      odyssey_tad_pmu_offline_cpu);
+	if (ret < 0)
+		return ret;
+	tad_pmu_cpuhp_state = ret;
+	return platform_driver_register(&odyssey_tad_pmu_driver);
+}
+
+static void __exit odyssey_tad_pmu_exit(void)
+{
+	platform_driver_unregister(&odyssey_tad_pmu_driver);
+	cpuhp_remove_multi_state(tad_pmu_cpuhp_state);
+}
+
+module_init(odyssey_tad_pmu_init);
+module_exit(odyssey_tad_pmu_exit);
+
+MODULE_DESCRIPTION("Marvell ODYSSEY LLC-TAD Perf driver");
+MODULE_AUTHOR("Gowthami Thiagarajan <gthiagarajan@marvell.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.25.1

[PATCH 4/6] dt-bindings: perf: marvell: Add YAML schemas for Marvell Odyssey LLC-TAD pmu

[Gowthami Thiagarajan]

Add device tree bindings for Marvell Odyssey LLC-TAD performance
monitor unit

Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com>
---
 .../bindings/perf/marvell-odyssey-tad.yaml    | 63 +++++++++++++++++++
 1 file changed, 63 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/perf/marvell-odyssey-tad.yaml

diff --git a/Documentation/devicetree/bindings/perf/marvell-odyssey-tad.yaml b/Documentation/devicetree/bindings/perf/marvell-odyssey-tad.yaml
new file mode 100644
index 000000000000..139567166f77
--- /dev/null
+++ b/Documentation/devicetree/bindings/perf/marvell-odyssey-tad.yaml
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/perf/marvell-odyssey-tad.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Marvell Odyssey LLC-TAD performance monitor
+
+maintainers:
+  - Gowthami Thiagarajan <gthiagarajan@marvell.com>
+
+description: |
+  The Tag-and-Data units (TADs) maintain coherence and contain CN10K
+  shared on-chip last level cache (LLC). The tad pmu measures the
+  performance of last-level cache. Each tad pmu supports up to eight
+  counters.
+
+  The DT setup comprises of number of tad blocks, the sizes of pmu
+  regions, tad blocks and overall base address of the HW.
+
+properties:
+  compatible:
+    const: marvell,odyssey-tad-pmu
+
+  reg:
+    maxItems: 1
+
+  marvell,tad-cnt:
+    description: specifies the number of tads on the soc
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  marvell,tad-page-size:
+    description: specifies the size of each tad page
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  marvell,tad-pmu-page-size:
+    description: specifies the size of page that the pmu uses
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+required:
+  - compatible
+  - reg
+  - marvell,tad-cnt
+  - marvell,tad-page-size
+  - marvell,tad-pmu-page-size
+
+additionalProperties: false
+
+examples:
+  - |
+
+    tad {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        tad_pmu@80000000 {
+            compatible = "marvell,odyssey-tad-pmu";
+            reg = <0x87E2 0x2B030000 0x0 0x1000>;
+            marvell,tad-cnt = <1>;
+            marvell,tad-page-size = <0x1000>;
+            marvell,tad-pmu-page-size = <0x1000>;
+        };
+    };
-- 
2.25.1

[PATCH 5/6] perf/marvell: Odyssey DDR Performance monitor support

[Gowthami Thiagarajan]

Odyssey DRAM Subsystem supports eight counters for monitoring performance
and software can program those counters to monitor any of the defined
performance events. Supported performance events include those counted
at the interface between the DDR controller and the PHY, interface between
the DDR Controller and the CHI interconnect, or within the DDR Controller.

Additionally DSS also supports two fixed performance event counters, one
for ddr reads and the other for ddr writes.

Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com>
---
 drivers/perf/marvell_cn10k_ddr_pmu.c | 404 ++++++++++++++++++++++-----
 1 file changed, 339 insertions(+), 65 deletions(-)

diff --git a/drivers/perf/marvell_cn10k_ddr_pmu.c b/drivers/perf/marvell_cn10k_ddr_pmu.c
index b94a5f6cc22b..d012e3d32e26 100644
--- a/drivers/perf/marvell_cn10k_ddr_pmu.c
+++ b/drivers/perf/marvell_cn10k_ddr_pmu.c
@@ -15,24 +15,29 @@
 #include <linux/acpi.h>
 
 /* Performance Counters Operating Mode Control Registers */
-#define DDRC_PERF_CNT_OP_MODE_CTRL	0x8020
-#define OP_MODE_CTRL_VAL_MANNUAL	0x1
+#define CN10K_DDRC_PERF_CNT_OP_MODE_CTRL	0x8020
+#define ODY_DDRC_PERF_CNT_OP_MODE_CTRL		0x20020
+#define OP_MODE_CTRL_VAL_MANUAL			0x1
 
 /* Performance Counters Start Operation Control Registers */
-#define DDRC_PERF_CNT_START_OP_CTRL	0x8028
-#define START_OP_CTRL_VAL_START		0x1ULL
-#define START_OP_CTRL_VAL_ACTIVE	0x2
+#define CN10K_DDRC_PERF_CNT_START_OP_CTRL	0x8028
+#define ODY_DDRC_PERF_CNT_START_OP_CTRL		0x200A0
+#define START_OP_CTRL_VAL_START			0x1ULL
+#define START_OP_CTRL_VAL_ACTIVE		0x2
 
 /* Performance Counters End Operation Control Registers */
-#define DDRC_PERF_CNT_END_OP_CTRL	0x8030
-#define END_OP_CTRL_VAL_END		0x1ULL
+#define CN10K_DDRC_PERF_CNT_END_OP_CTRL		0x8030
+#define ODY_DDRC_PERF_CNT_END_OP_CTRL		0x200E0
+#define END_OP_CTRL_VAL_END			0x1ULL
 
 /* Performance Counters End Status Registers */
-#define DDRC_PERF_CNT_END_STATUS		0x8038
+#define CN10K_DDRC_PERF_CNT_END_STATUS		0x8038
+#define ODY_DDRC_PERF_CNT_END_STATUS		0x20120
 #define END_STATUS_VAL_END_TIMER_MODE_END	0x1
 
 /* Performance Counters Configuration Registers */
-#define DDRC_PERF_CFG_BASE		0x8040
+#define CN10K_DDRC_PERF_CFG_BASE		0x8040
+#define ODY_DDRC_PERF_CFG_BASE			0x20160
 
 /* 8 Generic event counter + 2 fixed event counters */
 #define DDRC_PERF_NUM_GEN_COUNTERS	8
@@ -43,18 +48,31 @@
 					 DDRC_PERF_NUM_FIX_COUNTERS)
 
 /* Generic event counter registers */
-#define DDRC_PERF_CFG(n)		(DDRC_PERF_CFG_BASE + 8 * (n))
+#define DDRC_PERF_CFG(base, n)		((base) + 8 * (n))
 #define EVENT_ENABLE			BIT_ULL(63)
 
 /* Two dedicated event counters for DDR reads and writes */
 #define EVENT_DDR_READS			101
 #define EVENT_DDR_WRITES		100
 
+#define DDRC_PERF_REG(base, n)		((base) + 8 * (n))
 /*
  * programmable events IDs in programmable event counters.
  * DO NOT change these event-id numbers, they are used to
  * program event bitmap in h/w.
+ *
+ */
+/*
+ * Additional programmable events defined in
+ * Odyssey.
  */
+#define EVENT_DFI_CMD_IS_RETRY			61
+#define EVENT_RD_UC_ECC_ERROR			60
+#define EVENT_RD_CRC_ERROR			59
+#define EVENT_CAPAR_ERROR			58
+#define EVENT_WR_CRC_ERROR			57
+#define EVENT_DFI_PARITY_POISON			56
+
 #define EVENT_OP_IS_ZQLATCH			55
 #define EVENT_OP_IS_ZQSTART			54
 #define EVENT_OP_IS_TCR_MRR			53
@@ -64,8 +82,8 @@
 #define EVENT_VISIBLE_WIN_LIMIT_REACHED_RD	49
 #define EVENT_BSM_STARVATION			48
 #define EVENT_BSM_ALLOC				47
-#define EVENT_LPR_REQ_WITH_NOCREDIT		46
-#define EVENT_HPR_REQ_WITH_NOCREDIT		45
+#define EVENT_RETRY_FIFO_FULL_OR_LPR_REQ_NOCRED	46
+#define EVENT_DFI_OR_HPR_REQ_NOCRED		45
 #define EVENT_OP_IS_ZQCS			44
 #define EVENT_OP_IS_ZQCL			43
 #define EVENT_OP_IS_LOAD_MODE			42
@@ -103,28 +121,37 @@
 #define EVENT_HIF_RD_OR_WR			1
 
 /* Event counter value registers */
-#define DDRC_PERF_CNT_VALUE_BASE		0x8080
-#define DDRC_PERF_CNT_VALUE(n)	(DDRC_PERF_CNT_VALUE_BASE + 8 * (n))
+#define CN10K_DDRC_PERF_CNT_VALUE_BASE		0x8080
+#define ODY_DDRC_PERF_CNT_VALUE_BASE		0x201C0
 
 /* Fixed event counter enable/disable register */
-#define DDRC_PERF_CNT_FREERUN_EN	0x80C0
-#define DDRC_PERF_FREERUN_WRITE_EN	0x1
-#define DDRC_PERF_FREERUN_READ_EN	0x2
+#define CN10K_DDRC_PERF_CNT_FREERUN_EN		0x80C0
+#define DDRC_PERF_FREERUN_WRITE_EN		0x1
+#define DDRC_PERF_FREERUN_READ_EN		0x2
 
 /* Fixed event counter control register */
-#define DDRC_PERF_CNT_FREERUN_CTRL	0x80C8
-#define DDRC_FREERUN_WRITE_CNT_CLR	0x1
-#define DDRC_FREERUN_READ_CNT_CLR	0x2
+#define CN10K_DDRC_PERF_CNT_FREERUN_CTRL	0x80C8
+#define ODY_DDRC_PERF_CNT_FREERUN_CTRL		0x20240
+#define DDRC_FREERUN_WRITE_CNT_CLR		0x1
+#define DDRC_FREERUN_READ_CNT_CLR		0x2
+
+/* Fixed event counter clear register, defined only for Odyssey */
+#define ODY_DDRC_PERF_CNT_FREERUN_CLR		0x20248
 
 /* Fixed event counter value register */
-#define DDRC_PERF_CNT_VALUE_WR_OP	0x80D0
-#define DDRC_PERF_CNT_VALUE_RD_OP	0x80D8
-#define DDRC_PERF_CNT_VALUE_OVERFLOW	BIT_ULL(48)
-#define DDRC_PERF_CNT_MAX_VALUE		GENMASK_ULL(48, 0)
+#define CN10K_DDRC_PERF_CNT_VALUE_WR_OP		0x80D0
+#define CN10K_DDRC_PERF_CNT_VALUE_RD_OP		0x80D8
+#define ODY_DDRC_PERF_CNT_VALUE_WR_OP		0x20250
+#define ODY_DDRC_PERF_CNT_VALUE_RD_OP		0x20258
+
+#define VERSION_V1				1
+#define VERSION_V2				2
 
 struct cn10k_ddr_pmu {
 	struct pmu pmu;
 	void __iomem *base;
+	struct ddr_pmu_platform_data *p_data;
+	int version;
 	unsigned int cpu;
 	struct	device *dev;
 	int active_events;
@@ -135,6 +162,54 @@ struct cn10k_ddr_pmu {
 
 #define to_cn10k_ddr_pmu(p)	container_of(p, struct cn10k_ddr_pmu, pmu)
 
+struct ddr_pmu_platform_data {
+	u64 counter_overflow_val;
+	u64 counter_max_val;
+	u64 ddrc_perf_cnt_base;
+	u64 ddrc_perf_cfg_base;
+	u64 ddrc_perf_cnt_op_mode_ctrl;
+	u64 ddrc_perf_cnt_start_op_ctrl;
+	u64 ddrc_perf_cnt_end_op_ctrl;
+	u64 ddrc_perf_cnt_end_status;
+	u64 ddrc_perf_cnt_freerun_en;
+	u64 ddrc_perf_cnt_freerun_ctrl;
+	u64 ddrc_perf_cnt_freerun_clr;
+	u64 ddrc_perf_cnt_value_wr_op;
+	u64 ddrc_perf_cnt_value_rd_op;
+};
+
+static const struct ddr_pmu_platform_data cn10k_ddr_pmu_pdata = {
+	.counter_overflow_val =  BIT_ULL(48),
+	.counter_max_val = GENMASK_ULL(48, 0),
+	.ddrc_perf_cnt_base = CN10K_DDRC_PERF_CNT_VALUE_BASE,
+	.ddrc_perf_cfg_base = CN10K_DDRC_PERF_CFG_BASE,
+	.ddrc_perf_cnt_op_mode_ctrl = CN10K_DDRC_PERF_CNT_OP_MODE_CTRL,
+	.ddrc_perf_cnt_start_op_ctrl = CN10K_DDRC_PERF_CNT_START_OP_CTRL,
+	.ddrc_perf_cnt_end_op_ctrl = CN10K_DDRC_PERF_CNT_END_OP_CTRL,
+	.ddrc_perf_cnt_end_status = CN10K_DDRC_PERF_CNT_END_STATUS,
+	.ddrc_perf_cnt_freerun_en = CN10K_DDRC_PERF_CNT_FREERUN_EN,
+	.ddrc_perf_cnt_freerun_ctrl = CN10K_DDRC_PERF_CNT_FREERUN_CTRL,
+	.ddrc_perf_cnt_freerun_clr = 0,
+	.ddrc_perf_cnt_value_wr_op = CN10K_DDRC_PERF_CNT_VALUE_WR_OP,
+	.ddrc_perf_cnt_value_rd_op = CN10K_DDRC_PERF_CNT_VALUE_RD_OP,
+};
+
+static const struct ddr_pmu_platform_data odyssey_ddr_pmu_pdata = {
+	.counter_overflow_val = 0,
+	.counter_max_val = GENMASK_ULL(63, 0),
+	.ddrc_perf_cnt_base = ODY_DDRC_PERF_CNT_VALUE_BASE,
+	.ddrc_perf_cfg_base = ODY_DDRC_PERF_CFG_BASE,
+	.ddrc_perf_cnt_op_mode_ctrl = ODY_DDRC_PERF_CNT_OP_MODE_CTRL,
+	.ddrc_perf_cnt_start_op_ctrl = ODY_DDRC_PERF_CNT_START_OP_CTRL,
+	.ddrc_perf_cnt_end_op_ctrl = ODY_DDRC_PERF_CNT_END_OP_CTRL,
+	.ddrc_perf_cnt_end_status = ODY_DDRC_PERF_CNT_END_STATUS,
+	.ddrc_perf_cnt_freerun_en = 0,
+	.ddrc_perf_cnt_freerun_ctrl = ODY_DDRC_PERF_CNT_FREERUN_CTRL,
+	.ddrc_perf_cnt_freerun_clr = ODY_DDRC_PERF_CNT_FREERUN_CLR,
+	.ddrc_perf_cnt_value_wr_op = ODY_DDRC_PERF_CNT_VALUE_WR_OP,
+	.ddrc_perf_cnt_value_rd_op = ODY_DDRC_PERF_CNT_VALUE_RD_OP,
+};
+
 static ssize_t cn10k_ddr_pmu_event_show(struct device *dev,
 					struct device_attribute *attr,
 					char *page)
@@ -190,9 +265,9 @@ static struct attribute *cn10k_ddr_perf_events_attrs[] = {
 	CN10K_DDR_PMU_EVENT_ATTR(ddr_zqcl, EVENT_OP_IS_ZQCL),
 	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_wr_access, EVENT_OP_IS_ZQCS),
 	CN10K_DDR_PMU_EVENT_ATTR(ddr_hpr_req_with_nocredit,
-					EVENT_HPR_REQ_WITH_NOCREDIT),
+				 EVENT_DFI_OR_HPR_REQ_NOCRED),
 	CN10K_DDR_PMU_EVENT_ATTR(ddr_lpr_req_with_nocredit,
-					EVENT_LPR_REQ_WITH_NOCREDIT),
+				 EVENT_RETRY_FIFO_FULL_OR_LPR_REQ_NOCRED),
 	CN10K_DDR_PMU_EVENT_ATTR(ddr_bsm_alloc, EVENT_BSM_ALLOC),
 	CN10K_DDR_PMU_EVENT_ATTR(ddr_bsm_starvation, EVENT_BSM_STARVATION),
 	CN10K_DDR_PMU_EVENT_ATTR(ddr_win_limit_reached_rd,
@@ -215,6 +290,78 @@ static struct attribute_group cn10k_ddr_perf_events_attr_group = {
 	.attrs = cn10k_ddr_perf_events_attrs,
 };
 
+static struct attribute *odyssey_ddr_perf_events_attrs[] = {
+	/* Programmable */
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_rd_or_wr_access, EVENT_HIF_RD_OR_WR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_wr_access, EVENT_HIF_WR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_rd_access, EVENT_HIF_RD),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_rmw_access, EVENT_HIF_RMW),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_hif_pri_rdaccess, EVENT_HIF_HI_PRI_RD),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_rd_bypass_access, EVENT_READ_BYPASS),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_act_bypass_access, EVENT_ACT_BYPASS),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_dfi_wr_data_access, EVENT_DFI_WR_DATA_CYCLES),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_dfi_rd_data_access, EVENT_DFI_RD_DATA_CYCLES),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_hpri_sched_rd_crit_access,
+				 EVENT_HPR_XACT_WHEN_CRITICAL),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_lpri_sched_rd_crit_access,
+				 EVENT_LPR_XACT_WHEN_CRITICAL),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_wr_trxn_crit_access,
+				 EVENT_WR_XACT_WHEN_CRITICAL),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_active_access, EVENT_OP_IS_ACTIVATE),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_rd_or_wr_access, EVENT_OP_IS_RD_OR_WR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_rd_active_access, EVENT_OP_IS_RD_ACTIVATE),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_read, EVENT_OP_IS_RD),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_write, EVENT_OP_IS_WR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_mwr, EVENT_OP_IS_MWR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_precharge, EVENT_OP_IS_PRECHARGE),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_precharge_for_rdwr, EVENT_PRECHARGE_FOR_RDWR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_precharge_for_other,
+				 EVENT_PRECHARGE_FOR_OTHER),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_rdwr_transitions, EVENT_RDWR_TRANSITIONS),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_write_combine, EVENT_WRITE_COMBINE),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_war_hazard, EVENT_WAR_HAZARD),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_raw_hazard, EVENT_RAW_HAZARD),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_waw_hazard, EVENT_WAW_HAZARD),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_enter_selfref, EVENT_OP_IS_ENTER_SELFREF),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_enter_powerdown, EVENT_OP_IS_ENTER_POWERDOWN),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_enter_mpsm, EVENT_OP_IS_ENTER_MPSM),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_refresh, EVENT_OP_IS_REFRESH),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_crit_ref, EVENT_OP_IS_CRIT_REF),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_spec_ref, EVENT_OP_IS_SPEC_REF),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_load_mode, EVENT_OP_IS_LOAD_MODE),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_zqcl, EVENT_OP_IS_ZQCL),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_cam_wr_access, EVENT_OP_IS_ZQCS),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_dfi_cycles, EVENT_DFI_OR_HPR_REQ_NOCRED),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_retry_fifo_full,
+				 EVENT_RETRY_FIFO_FULL_OR_LPR_REQ_NOCRED),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_bsm_alloc, EVENT_BSM_ALLOC),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_bsm_starvation, EVENT_BSM_STARVATION),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_win_limit_reached_rd,
+				 EVENT_VISIBLE_WIN_LIMIT_REACHED_RD),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_win_limit_reached_wr,
+				 EVENT_VISIBLE_WIN_LIMIT_REACHED_WR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_dqsosc_mpc, EVENT_OP_IS_DQSOSC_MPC),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_dqsosc_mrr, EVENT_OP_IS_DQSOSC_MRR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_tcr_mrr, EVENT_OP_IS_TCR_MRR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_zqstart, EVENT_OP_IS_ZQSTART),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_zqlatch, EVENT_OP_IS_ZQLATCH),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_dfi_parity_poison, EVENT_DFI_PARITY_POISON),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_wr_crc_error, EVENT_WR_CRC_ERROR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_capar_error, EVENT_CAPAR_ERROR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_rd_crc_error, EVENT_RD_CRC_ERROR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_rd_uc_ecc_error, EVENT_RD_UC_ECC_ERROR),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_dfi_cmd_is_retry, EVENT_DFI_CMD_IS_RETRY),
+	/* Free run event counters */
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_ddr_reads, EVENT_DDR_READS),
+	CN10K_DDR_PMU_EVENT_ATTR(ddr_ddr_writes, EVENT_DDR_WRITES),
+	NULL
+};
+
+static struct attribute_group odyssey_ddr_perf_events_attr_group = {
+	.name = "events",
+	.attrs = odyssey_ddr_perf_events_attrs,
+};
+
 PMU_FORMAT_ATTR(event, "config:0-8");
 
 static struct attribute *cn10k_ddr_perf_format_attrs[] = {
@@ -255,6 +402,13 @@ static const struct attribute_group *cn10k_attr_groups[] = {
 	NULL,
 };
 
+static const struct attribute_group *odyssey_attr_groups[] = {
+	&odyssey_ddr_perf_events_attr_group,
+	&cn10k_ddr_perf_format_attr_group,
+	&cn10k_ddr_perf_cpumask_attr_group,
+	NULL
+};
+
 /* Default poll timeout is 100 sec, which is very sufficient for
  * 48 bit counter incremented max at 5.6 GT/s, which may take many
  * hours to overflow.
@@ -267,13 +421,23 @@ static ktime_t cn10k_ddr_pmu_timer_period(void)
 	return ms_to_ktime((u64)cn10k_ddr_pmu_poll_period_sec * USEC_PER_SEC);
 }
 
-static int ddr_perf_get_event_bitmap(int eventid, u64 *event_bitmap)
+static int ddr_perf_get_event_bitmap(int eventid, u64 *event_bitmap, struct cn10k_ddr_pmu *ddr_pmu)
 {
+	int ret = 0;
+
 	switch (eventid) {
 	case EVENT_HIF_RD_OR_WR ... EVENT_WAW_HAZARD:
 	case EVENT_OP_IS_REFRESH ... EVENT_OP_IS_ZQLATCH:
 		*event_bitmap = (1ULL << (eventid - 1));
 		break;
+	case EVENT_DFI_PARITY_POISON ...EVENT_DFI_CMD_IS_RETRY:
+		if (ddr_pmu->version == VERSION_V2) {
+			*event_bitmap = (1ULL << (eventid - 1));
+		} else {
+			pr_err("%s Invalid eventid %d\n", __func__, eventid);
+			ret = -EINVAL;
+		}
+		break;
 	case EVENT_OP_IS_ENTER_SELFREF:
 	case EVENT_OP_IS_ENTER_POWERDOWN:
 	case EVENT_OP_IS_ENTER_MPSM:
@@ -281,10 +445,10 @@ static int ddr_perf_get_event_bitmap(int eventid, u64 *event_bitmap)
 		break;
 	default:
 		pr_err("%s Invalid eventid %d\n", __func__, eventid);
-		return -EINVAL;
+		ret = -EINVAL;
 	}
 
-	return 0;
+	return ret;
 }
 
 static int cn10k_ddr_perf_alloc_counter(struct cn10k_ddr_pmu *pmu,
@@ -357,6 +521,7 @@ static void cn10k_ddr_perf_counter_enable(struct cn10k_ddr_pmu *pmu,
 {
 	u32 reg;
 	u64 val;
+	struct ddr_pmu_platform_data *p_data = pmu->p_data;
 
 	if (counter > DDRC_PERF_NUM_COUNTERS) {
 		pr_err("Error: unsupported counter %d\n", counter);
@@ -364,7 +529,7 @@ static void cn10k_ddr_perf_counter_enable(struct cn10k_ddr_pmu *pmu,
 	}
 
 	if (counter < DDRC_PERF_NUM_GEN_COUNTERS) {
-		reg = DDRC_PERF_CFG(counter);
+		reg = DDRC_PERF_CFG(p_data->ddrc_perf_cfg_base, counter);
 		val = readq_relaxed(pmu->base + reg);
 
 		if (enable)
@@ -374,7 +539,11 @@ static void cn10k_ddr_perf_counter_enable(struct cn10k_ddr_pmu *pmu,
 
 		writeq_relaxed(val, pmu->base + reg);
 	} else {
-		val = readq_relaxed(pmu->base + DDRC_PERF_CNT_FREERUN_EN);
+		if (p_data->ddrc_perf_cnt_freerun_en)
+			val = readq_relaxed(pmu->base + p_data->ddrc_perf_cnt_freerun_en);
+		else
+			val = readq_relaxed(pmu->base + p_data->ddrc_perf_cnt_freerun_ctrl);
+
 		if (enable) {
 			if (counter == DDRC_PERF_READ_COUNTER_IDX)
 				val |= DDRC_PERF_FREERUN_READ_EN;
@@ -386,7 +555,11 @@ static void cn10k_ddr_perf_counter_enable(struct cn10k_ddr_pmu *pmu,
 			else
 				val &= ~DDRC_PERF_FREERUN_WRITE_EN;
 		}
-		writeq_relaxed(val, pmu->base + DDRC_PERF_CNT_FREERUN_EN);
+
+		if (p_data->ddrc_perf_cnt_freerun_en)
+			writeq_relaxed(val, pmu->base + p_data->ddrc_perf_cnt_freerun_en);
+		else
+			writeq_relaxed(val, pmu->base + p_data->ddrc_perf_cnt_freerun_ctrl);
 	}
 }
 
@@ -394,13 +567,15 @@ static u64 cn10k_ddr_perf_read_counter(struct cn10k_ddr_pmu *pmu, int counter)
 {
 	u64 val;
 
+	struct ddr_pmu_platform_data *p_data = pmu->p_data;
+
 	if (counter == DDRC_PERF_READ_COUNTER_IDX)
-		return readq_relaxed(pmu->base + DDRC_PERF_CNT_VALUE_RD_OP);
+		return readq_relaxed(pmu->base + p_data->ddrc_perf_cnt_value_rd_op);
 
 	if (counter == DDRC_PERF_WRITE_COUNTER_IDX)
-		return readq_relaxed(pmu->base + DDRC_PERF_CNT_VALUE_WR_OP);
+		return readq_relaxed(pmu->base + p_data->ddrc_perf_cnt_value_wr_op);
 
-	val = readq_relaxed(pmu->base + DDRC_PERF_CNT_VALUE(counter));
+	val = readq_relaxed(pmu->base + DDRC_PERF_REG(p_data->ddrc_perf_cnt_base, counter));
 	return val;
 }
 
@@ -408,6 +583,7 @@ static void cn10k_ddr_perf_event_update(struct perf_event *event)
 {
 	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
 	struct hw_perf_event *hwc = &event->hw;
+	struct ddr_pmu_platform_data *p_data = pmu->p_data;
 	u64 prev_count, new_count, mask;
 
 	do {
@@ -415,11 +591,27 @@ static void cn10k_ddr_perf_event_update(struct perf_event *event)
 		new_count = cn10k_ddr_perf_read_counter(pmu, hwc->idx);
 	} while (local64_xchg(&hwc->prev_count, new_count) != prev_count);
 
-	mask = DDRC_PERF_CNT_MAX_VALUE;
+	mask = p_data->counter_max_val;
 
 	local64_add((new_count - prev_count) & mask, &event->count);
 }
 
+static void cn10k_ddr_perf_counter_start(struct cn10k_ddr_pmu *ddr_pmu, int counter)
+{
+	struct ddr_pmu_platform_data *p_data = ddr_pmu->p_data;
+
+	writeq_relaxed(START_OP_CTRL_VAL_START, ddr_pmu->base +
+		       DDRC_PERF_REG(p_data->ddrc_perf_cnt_start_op_ctrl, counter));
+}
+
+static void cn10k_ddr_perf_counter_stop(struct cn10k_ddr_pmu *ddr_pmu, int counter)
+{
+	struct ddr_pmu_platform_data *p_data = ddr_pmu->p_data;
+
+	writeq_relaxed(END_OP_CTRL_VAL_END, ddr_pmu->base +
+		       DDRC_PERF_REG(p_data->ddrc_perf_cnt_end_op_ctrl, counter));
+}
+
 static void cn10k_ddr_perf_event_start(struct perf_event *event, int flags)
 {
 	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
@@ -429,6 +621,14 @@ static void cn10k_ddr_perf_event_start(struct perf_event *event, int flags)
 	local64_set(&hwc->prev_count, 0);
 
 	cn10k_ddr_perf_counter_enable(pmu, counter, true);
+	if (pmu->version == VERSION_V2) {
+	/* Setup the PMU counter to work in manual mode */
+		writeq_relaxed(OP_MODE_CTRL_VAL_MANUAL, pmu->base +
+			DDRC_PERF_REG(pmu->p_data->ddrc_perf_cnt_op_mode_ctrl,
+				      counter));
+
+		cn10k_ddr_perf_counter_start(pmu, counter);
+	}
 
 	hwc->state = 0;
 }
@@ -436,6 +636,7 @@ static void cn10k_ddr_perf_event_start(struct perf_event *event, int flags)
 static int cn10k_ddr_perf_event_add(struct perf_event *event, int flags)
 {
 	struct cn10k_ddr_pmu *pmu = to_cn10k_ddr_pmu(event->pmu);
+	struct ddr_pmu_platform_data *p_data = pmu->p_data;
 	struct hw_perf_event *hwc = &event->hw;
 	u8 config = event->attr.config;
 	int counter, ret;
@@ -455,8 +656,8 @@ static int cn10k_ddr_perf_event_add(struct perf_event *event, int flags)
 
 	if (counter < DDRC_PERF_NUM_GEN_COUNTERS) {
 		/* Generic counters, configure event id */
-		reg_offset = DDRC_PERF_CFG(counter);
-		ret = ddr_perf_get_event_bitmap(config, &val);
+		reg_offset = DDRC_PERF_CFG(p_data->ddrc_perf_cfg_base, counter);
+		ret = ddr_perf_get_event_bitmap(config, &val, pmu);
 		if (ret)
 			return ret;
 
@@ -468,7 +669,10 @@ static int cn10k_ddr_perf_event_add(struct perf_event *event, int flags)
 		else
 			val = DDRC_FREERUN_WRITE_CNT_CLR;
 
-		writeq_relaxed(val, pmu->base + DDRC_PERF_CNT_FREERUN_CTRL);
+		if (p_data->ddrc_perf_cnt_freerun_clr)
+			writeq_relaxed(val, pmu->base + p_data->ddrc_perf_cnt_freerun_clr);
+		else
+			writeq_relaxed(val, pmu->base + p_data->ddrc_perf_cnt_freerun_ctrl);
 	}
 
 	hwc->state |= PERF_HES_STOPPED;
@@ -487,6 +691,9 @@ static void cn10k_ddr_perf_event_stop(struct perf_event *event, int flags)
 
 	cn10k_ddr_perf_counter_enable(pmu, counter, false);
 
+	if (pmu->version == VERSION_V2)
+		cn10k_ddr_perf_counter_stop(pmu, counter);
+
 	if (flags & PERF_EF_UPDATE)
 		cn10k_ddr_perf_event_update(event);
 
@@ -513,17 +720,19 @@ static void cn10k_ddr_perf_event_del(struct perf_event *event, int flags)
 static void cn10k_ddr_perf_pmu_enable(struct pmu *pmu)
 {
 	struct cn10k_ddr_pmu *ddr_pmu = to_cn10k_ddr_pmu(pmu);
+	struct ddr_pmu_platform_data *p_data = ddr_pmu->p_data;
 
 	writeq_relaxed(START_OP_CTRL_VAL_START, ddr_pmu->base +
-		       DDRC_PERF_CNT_START_OP_CTRL);
+		       p_data->ddrc_perf_cnt_start_op_ctrl);
 }
 
 static void cn10k_ddr_perf_pmu_disable(struct pmu *pmu)
 {
 	struct cn10k_ddr_pmu *ddr_pmu = to_cn10k_ddr_pmu(pmu);
+	struct ddr_pmu_platform_data *p_data = ddr_pmu->p_data;
 
 	writeq_relaxed(END_OP_CTRL_VAL_END, ddr_pmu->base +
-		       DDRC_PERF_CNT_END_OP_CTRL);
+		       p_data->ddrc_perf_cnt_end_op_ctrl);
 }
 
 static void cn10k_ddr_perf_event_update_all(struct cn10k_ddr_pmu *pmu)
@@ -550,6 +759,7 @@ static void cn10k_ddr_perf_event_update_all(struct cn10k_ddr_pmu *pmu)
 
 static irqreturn_t cn10k_ddr_pmu_overflow_handler(struct cn10k_ddr_pmu *pmu)
 {
+	struct ddr_pmu_platform_data *p_data = pmu->p_data;
 	struct perf_event *event;
 	struct hw_perf_event *hwc;
 	u64 prev_count, new_count;
@@ -587,11 +797,23 @@ static irqreturn_t cn10k_ddr_pmu_overflow_handler(struct cn10k_ddr_pmu *pmu)
 			continue;
 
 		value = cn10k_ddr_perf_read_counter(pmu, i);
-		if (value == DDRC_PERF_CNT_MAX_VALUE) {
+		if (value == p_data->counter_max_val) {
 			pr_info("Counter-(%d) reached max value\n", i);
-			cn10k_ddr_perf_event_update_all(pmu);
-			cn10k_ddr_perf_pmu_disable(&pmu->pmu);
-			cn10k_ddr_perf_pmu_enable(&pmu->pmu);
+			/*
+			 * As separate control register is added for each counter
+			 * in odyssey, no need to update all the events
+			 *
+			 */
+			if (pmu->version == VERSION_V2) {
+				cn10k_ddr_perf_event_update(pmu->events[i]);
+				cn10k_ddr_perf_counter_stop(pmu, i);
+				cn10k_ddr_perf_counter_start(pmu, i);
+
+			} else {
+				cn10k_ddr_perf_event_update_all(pmu);
+				cn10k_ddr_perf_pmu_disable(&pmu->pmu);
+				cn10k_ddr_perf_pmu_enable(&pmu->pmu);
+			}
 		}
 	}
 
@@ -632,7 +854,10 @@ static int cn10k_ddr_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
 
 static int cn10k_ddr_perf_probe(struct platform_device *pdev)
 {
+	struct ddr_pmu_platform_data *pltfm_data;
+	struct device *dev = &pdev->dev;
 	struct cn10k_ddr_pmu *ddr_pmu;
+	const char  *compatible;
 	struct resource *res;
 	void __iomem *base;
 	char *name;
@@ -643,6 +868,13 @@ static int cn10k_ddr_perf_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	ddr_pmu->dev = &pdev->dev;
+
+	pltfm_data = (struct ddr_pmu_platform_data *)device_get_match_data(&pdev->dev);
+	if (!pltfm_data) {
+		dev_err(&pdev->dev, "Error: No device match data found\n");
+		return -ENODEV;
+	}
+	ddr_pmu->p_data = pltfm_data;
 	platform_set_drvdata(pdev, ddr_pmu);
 
 	base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
@@ -651,25 +883,59 @@ static int cn10k_ddr_perf_probe(struct platform_device *pdev)
 
 	ddr_pmu->base = base;
 
-	/* Setup the PMU counter to work in manual mode */
-	writeq_relaxed(OP_MODE_CTRL_VAL_MANNUAL, ddr_pmu->base +
-		       DDRC_PERF_CNT_OP_MODE_CTRL);
-
-	ddr_pmu->pmu = (struct pmu) {
-		.module	      = THIS_MODULE,
-		.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
-		.task_ctx_nr = perf_invalid_context,
-		.attr_groups = cn10k_attr_groups,
-		.event_init  = cn10k_ddr_perf_event_init,
-		.add	     = cn10k_ddr_perf_event_add,
-		.del	     = cn10k_ddr_perf_event_del,
-		.start	     = cn10k_ddr_perf_event_start,
-		.stop	     = cn10k_ddr_perf_event_stop,
-		.read	     = cn10k_ddr_perf_event_update,
-		.pmu_enable  = cn10k_ddr_perf_pmu_enable,
-		.pmu_disable = cn10k_ddr_perf_pmu_disable,
-	};
+	ret = device_property_read_string(dev, "compatible", &compatible);
+	if (ret) {
+		pr_err("compatible property not found\n");
+		return ret;
+	}
 
+	if ((strncmp("marvell,cn10k-ddr-pmu", compatible,
+		     strlen(compatible)) == 0))
+		ddr_pmu->version = VERSION_V1;
+	else
+		ddr_pmu->version = VERSION_V2;
+
+	if (ddr_pmu->version == VERSION_V1) {
+		ddr_pmu->pmu = (struct pmu) {
+			.module	      = THIS_MODULE,
+			.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
+			.task_ctx_nr = perf_invalid_context,
+			.attr_groups = cn10k_attr_groups,
+			.event_init  = cn10k_ddr_perf_event_init,
+			.add	     = cn10k_ddr_perf_event_add,
+			.del	     = cn10k_ddr_perf_event_del,
+			.start	     = cn10k_ddr_perf_event_start,
+			.stop	     = cn10k_ddr_perf_event_stop,
+			.read	     = cn10k_ddr_perf_event_update,
+			.pmu_enable  = cn10k_ddr_perf_pmu_enable,
+			.pmu_disable = cn10k_ddr_perf_pmu_disable,
+		};
+
+	/*
+	 * As we have separate control registers for each counter in Odyssey,
+	 * setting up the mode will be done when we enable each counter
+	 *
+	 */
+
+	/* Setup the PMU counter to work in manual mode */
+		writeq(OP_MODE_CTRL_VAL_MANUAL, ddr_pmu->base +
+		      (ddr_pmu->p_data->ddrc_perf_cnt_op_mode_ctrl));
+	} else {
+		ddr_pmu->pmu = (struct pmu) {
+			.module       = THIS_MODULE,
+			.capabilities = PERF_PMU_CAP_NO_EXCLUDE,
+			.task_ctx_nr = perf_invalid_context,
+			.attr_groups = odyssey_attr_groups,
+			.event_init  = cn10k_ddr_perf_event_init,
+			.add         = cn10k_ddr_perf_event_add,
+			.del         = cn10k_ddr_perf_event_del,
+			.start       = cn10k_ddr_perf_event_start,
+			.stop        = cn10k_ddr_perf_event_stop,
+			.read        = cn10k_ddr_perf_event_update,
+			.pmu_enable  = NULL,
+			.pmu_disable = NULL,
+		};
+	}
 	/* Choose this cpu to collect perf data */
 	ddr_pmu->cpu = raw_smp_processor_id();
 
@@ -712,7 +978,8 @@ static int cn10k_ddr_perf_remove(struct platform_device *pdev)
 
 #ifdef CONFIG_OF
 static const struct of_device_id cn10k_ddr_pmu_of_match[] = {
-	{ .compatible = "marvell,cn10k-ddr-pmu", },
+	{ .compatible = "marvell,cn10k-ddr-pmu", .data = &cn10k_ddr_pmu_pdata },
+	{ .compatible = "marvell,odyssey-ddr-pmu", .data = &odyssey_ddr_pmu_pdata },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, cn10k_ddr_pmu_of_match);
@@ -720,7 +987,14 @@ MODULE_DEVICE_TABLE(of, cn10k_ddr_pmu_of_match);
 
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id cn10k_ddr_pmu_acpi_match[] = {
-	{"MRVL000A", 0},
+	{
+		.id = "MRVL000A",
+		.driver_data = (kernel_ulong_t)&cn10k_ddr_pmu_pdata,
+	},
+	{
+		.id = "MRVL000C",
+		.driver_data = (kernel_ulong_t)&odyssey_ddr_pmu_pdata,
+	},
 	{},
 };
 MODULE_DEVICE_TABLE(acpi, cn10k_ddr_pmu_acpi_match);
-- 
2.25.1

[PATCH 6/6] dt-bindings: Add YAML schemas for Marvell Odyssey DDR PMU

[Gowthami Thiagarajan]

Add device tree bindings for Marvell Odyssey DDR PMU.

Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com>
---
 .../devicetree/bindings/perf/marvell-cn10k-ddr.yaml      | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/perf/marvell-cn10k-ddr.yaml b/Documentation/devicetree/bindings/perf/marvell-cn10k-ddr.yaml
index a18dd0a8c43a..a435cbf4aea0 100644
--- a/Documentation/devicetree/bindings/perf/marvell-cn10k-ddr.yaml
+++ b/Documentation/devicetree/bindings/perf/marvell-cn10k-ddr.yaml
@@ -11,10 +11,15 @@ maintainers:
 
 properties:
   compatible:
-    items:
+    oneOf:
       - enum:
           - marvell,cn10k-ddr-pmu
-
+          - marvell,odyssey-ddr-pmu
+      - items:
+          - enum:
+              - marvell,cn10k-ddr-pmu
+              - marvell,odyssey-ddr-pmu
+          - const: marvell,cn10k-ddr-pmu
   reg:
     maxItems: 1
 
-- 
2.25.1

Re: [PATCH 2/6] dt-bindings: perf: marvell: Add YAML schemas for Marvell PEM pmu

[Krzysztof Kozlowski]

On 30/06/2023 14:03, Gowthami Thiagarajan wrote:
> Add device tree bindings for Marvell PEM performance monitor unit
> 
> Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com>
> Signed-off-by: Linu Cherian <lcherian@marvell.com>

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

You missed at least DT list (maybe more), so this won't be tested by our
tools. Performing review on untested code might be a waste of time, thus
I will skip this patch entirely till you follow the process allowing the
patch to be tested.

Please kindly resend and include all necessary To/Cc entries.


Best regards,
Krzysztof

Re: [PATCH 4/6] dt-bindings: perf: marvell: Add YAML schemas for Marvell Odyssey LLC-TAD pmu

[Krzysztof Kozlowski]

On 30/06/2023 14:03, Gowthami Thiagarajan wrote:
> Add device tree bindings for Marvell Odyssey LLC-TAD performance
> monitor unit
> 
> Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com>

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC.  It might happen, that command when run on an older
kernel, gives you outdated entries.  Therefore please be sure you base
your patches on recent Linux kernel.

You missed at least DT list (maybe more), so this won't be tested by our
tools. Performing review on untested code might be a waste of time, thus
I will skip this patch entirely till you follow the process allowing the
patch to be tested.

Please kindly resend and include all necessary To/Cc entries.

> ---
>  .../bindings/perf/marvell-odyssey-tad.yaml    | 63 +++++++++++++++++++
>  1 file changed, 63 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/perf/marvell-odyssey-tad.yaml
> 
> diff --git a/Documentation/devicetree/bindings/perf/marvell-odyssey-tad.yaml b/Documentation/devicetree/bindings/perf/marvell-odyssey-tad.yaml
> new file mode 100644
> index 000000000000..139567166f77
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/perf/marvell-odyssey-tad.yaml

Filename does not match compatible.



> +examples:
> +  - |
> +
> +    tad {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        tad_pmu@80000000 {

Node names should be generic. See also explanation and list of examples
in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +            compatible = "marvell,odyssey-tad-pmu";
> +            reg = <0x87E2 0x2B030000 0x0 0x1000>;

lowercase hex

Best regards,
Krzysztof

Re: [PATCH 6/6] dt-bindings: Add YAML schemas for Marvell Odyssey DDR PMU

[Krzysztof Kozlowski]

On 30/06/2023 14:03, Gowthami Thiagarajan wrote:
> Add device tree bindings for Marvell Odyssey DDR PMU.

For all patches:
1. A nit, subject: drop second/last, redundant "YAML schemas for". The
"dt-bindings" prefix is already stating that these are bindings.

2. Please use subject prefixes matching the subsystem. You can get them
for example with `git log --oneline -- DIRECTORY_OR_FILE` on the
directory your patch is touching.

3. Please use scripts/get_maintainers.pl to get a list of necessary
people and lists to CC.  It might happen, that command when run on an
older kernel, gives you outdated entries.  Therefore please be sure you
base your patches on recent Linux kernel.

You missed at least DT list (maybe more), so this won't be tested by our
tools. Performing review on untested code might be a waste of time, thus
I will skip this patch entirely till you follow the process allowing the
patch to be tested.

Please kindly resend and include all necessary To/Cc entries.


>  

Best regards,
Krzysztof

Re: [PATCH 1/6] perf/marvell: Marvell PEM performance monitor support

[Mark Rutland]

Hi,

On Fri, Jun 30, 2023 at 05:33:46PM +0530, Gowthami Thiagarajan wrote:
> PCI Express Interface PMU includes various performance counters to monitor
> the data that is transmitted over the PCIe link. The counters track various
> inbound and outbound transactions which includes separate counters for
> posted/non-posted/completion TLPs. Also, inbound and outbound memory read
> requests along with their latencies can also be monitored. Address
> Translation Services(ATS)events such as ATS Translation, ATS Page Request,
> ATS Invalidation along with their corresponding latencies are also
> supported.
> 
> The performance counters are 64 bits wide.
> 
> For instance,
> perf stat -e ib_tlp_pr <workload>
> tracks the inbound posted TLPs for the workload.
> 
> Signed-off-by: Linu Cherian <lcherian@marvell.com>
> Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com>

This generally looks fine; I have a few comments below.

[...]

> diff --git a/drivers/perf/marvell_pem_pmu.c b/drivers/perf/marvell_pem_pmu.c
> new file mode 100644
> index 000000000000..fb27112aa7d4
> --- /dev/null
> +++ b/drivers/perf/marvell_pem_pmu.c
> @@ -0,0 +1,433 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Marvell PEM(PCIe RC) Performance Monitor Driver
> + *
> + * Copyright (C) 2023 Marvell.
> + */

Nit: please follow the preferred coding style for comments. This should have a
newline immediately after the '/*', e.g.

	/*
	 * Marvell PEM(PCIe RC) Performance Monitor Driver
	 *
	 * Copyright (C) 2023 Marvell.
	 */

Likewise for all other multi-line comments.
	
> +#include <linux/acpi.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/perf_event.h>
> +
> +/* Each of these events maps to a free running 64 bit counter
> + * with no event control, but can be reset.
> + *
> + */
> +enum pem_events {
> +	IB_TLP_NPR,
> +	IB_TLP_PR,
> +	IB_TLP_CPL,

> +static u64 eventid_to_offset_table[] = {
> +	0x0,
> +	0x8,
> +	0x10,

I assume the event IDs are the values in the pem_events enum, so please use
array initalizers here to make that clear, e.g.

	static u64 eventid_to_offset_table[] = {
		[IB_TLP_NPR]	= 0x0,
		[IB_TLP_PR]	= 0x8,
		[IB_TLP_CPL]	 0x10,
		...
	};

[...]

> +static int pem_perf_event_init(struct perf_event *event)
> +{
> +	struct pem_pmu *pmu = to_pem_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +
> +	if (event->attr.type != event->pmu->type)
> +		return -ENOENT;
> +
> +	if (is_sampling_event(event)) {

Don't we also need to check for:

	event->attach_state & PERF_ATTACH_TASK

> +		dev_info(pmu->dev, "Sampling not supported!\n");
> +		return -EOPNOTSUPP;
> +	}

Please delete this dev_info().

> +
> +	if (event->cpu < 0) {
> +		dev_warn(pmu->dev, "Can't provide per-task data!\n");
> +		return -EOPNOTSUPP;
> +	}

Likewise, please delete this dev_warn().

> +
> +	/*  We must NOT create groups containing mixed PMUs */
> +	if (event->group_leader->pmu != event->pmu &&
> +	    !is_software_event(event->group_leader))
> +		return -EINVAL;
> +
> +	/* Set ownership of event to one CPU, same event can not be observed
> +	 * on multiple cpus at same time.
> +	 */

Please fix this comment style (or delete the comment).

> +	event->cpu = pmu->cpu;
> +	hwc->idx = -1;
> +	return 0;
> +}

Thanks,
Mark.

Re: [PATCH 2/6] dt-bindings: perf: marvell: Add YAML schemas for Marvell PEM pmu

[Mark Rutland]

On Fri, Jun 30, 2023 at 05:33:47PM +0530, Gowthami Thiagarajan wrote:
> Add device tree bindings for Marvell PEM performance monitor unit
> 
> Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com>
> Signed-off-by: Linu Cherian <lcherian@marvell.com>

As Krzysztof mentioned, the device tree list (and the DT bindings maintainers)
haven't been Cc'd, so this cannot be acked.

When resending, please CC the devicetree list, Rob, and Conor:

[mark@lakrids:~/src/linux]% ./scripts/get_maintainer.pl -f Documentation/devicetree/bindings/perf
Will Deacon <will@kernel.org> (maintainer:ARM PMU PROFILING AND DEBUGGING)
Mark Rutland <mark.rutland@arm.com> (maintainer:ARM PMU PROFILING AND DEBUGGING)
Rob Herring <robh+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
Conor Dooley <conor+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
linux-arm-kernel@lists.infradead.org (moderated list:ARM PMU PROFILING AND DEBUGGING)
devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS)
linux-kernel@vger.kernel.org (open list)

Thanks,
Mark.

> ---
>  .../bindings/perf/marvell-odyssey-pem.yaml    | 38 +++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/perf/marvell-odyssey-pem.yaml
> 
> diff --git a/Documentation/devicetree/bindings/perf/marvell-odyssey-pem.yaml b/Documentation/devicetree/bindings/perf/marvell-odyssey-pem.yaml
> new file mode 100644
> index 000000000000..6af201fbccd8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/perf/marvell-odyssey-pem.yaml
> @@ -0,0 +1,38 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/perf/marvell-odyssey-pem.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell Odyssey PCIe interface performance monitor
> +
> +maintainers:
> +  - Linu Cherian <lcherian@marvell.com>
> +  - Gowthami Thiagarajan <gthiagarajan@marvell.com>
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - marvell,pem-pmu
> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    bus {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        pmu@8e0000005000 {
> +            compatible = "marvell,pem-pmu";
> +            reg = <0x8E00 0x00005000 0x0 0x3000>;
> +        };
> +    };
> -- 
> 2.25.1
> 
> 

Re: [PATCH 3/6] perf/marvell : Odyssey LLC-TAD performance monitor support

[Mark Rutland]

On Fri, Jun 30, 2023 at 05:33:48PM +0530, Gowthami Thiagarajan wrote:
> Each TAD provides eight 64-bit counters for monitoring
> cache behavior.The driver always configures the same counter for
> all the TADs. The user would end up effectively reserving one of
> eight counters in every TAD to look across all TADs.
> The occurrences of events are aggregated and presented to the user
> at the end of running the workload. The driver does not provide a
> way for the user to partition TADs so that different TADs are used for
> different applications.
> 
> The performance events reflect various internal or interface activities.
> By combining the values from multiple performance counters, cache
> performance can be measured in terms such as: cache miss rate, cache
> allocations, interface retry rate, internal resource occupancy, etc.
> 
> Each supported counter's event and formatting information is exposed
> to sysfs at /sys/devices/tad/. Use perf tool stat command to measure
> the pmu events. For instance:
> 
> perf stat -e tad_hit_ltg,tad_hit_dtg <workload>
> 
> Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com>

This generally looks ok; I have a few comments below.

[...]

> +static void tad_pmu_event_counter_stop(struct perf_event *event, int flags)
> +{
> +	struct tad_pmu *tad_pmu = to_tad_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	u32 counter_idx = hwc->idx;
> +	int tad_region;
> +
> +	/* TAD()_PFC() stop counting on the write
> +	 * which sets TAD()_PRF()[CNTSEL] == 0
> +	 */

Please fix the comment style.

Likewise for all other instances within this file.

[...]

> +static int tad_pmu_event_counter_add(struct perf_event *event, int flags)
> +{
> +	struct tad_pmu *tad_pmu = to_tad_pmu(event->pmu);
> +	struct hw_perf_event *hwc = &event->hw;
> +	int idx;
> +
> +	/* Get a free counter for this event */
> +	idx = find_first_zero_bit(tad_pmu->counters_map, TAD_MAX_COUNTERS);
> +	if (idx == TAD_MAX_COUNTERS)
> +		return -EAGAIN;
> +
> +	set_bit(idx, tad_pmu->counters_map);
> +
> +	hwc->idx = idx;
> +	hwc->state = PERF_HES_STOPPED;
> +	tad_pmu->events[idx] = event;
> +
> +	if (flags & PERF_EF_START)
> +		tad_pmu_event_counter_start(event, flags);
> +
> +	return 0;
> +}
> +
> +static int tad_pmu_event_init(struct perf_event *event)
> +{
> +	struct tad_pmu *tad_pmu = to_tad_pmu(event->pmu);
> +
> +	if (event->attr.type != event->pmu->type)
> +		return -ENOENT;

Why is this not rejecting smapling events, as patch 1 does?

> +
> +	if (!event->attr.disabled)
> +		return -EINVAL;

Why?

> +
> +	if (event->state != PERF_EVENT_STATE_OFF)
> +		return -EINVAL;

Event groups need to be verified here too.

[...]

> +static int tad_pmu_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct tad_region *regions;
> +	struct tad_pmu *tad_pmu;
> +	struct resource *res;
> +	u32 tad_pmu_page_size;
> +	u32 tad_page_size;
> +	u32 tad_cnt;
> +	int i, ret;
> +	char *name;
> +
> +	tad_pmu = devm_kzalloc(&pdev->dev, sizeof(*tad_pmu), GFP_KERNEL);
> +	if (!tad_pmu)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, tad_pmu);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Mem resource not found\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = device_property_read_u32(dev, "marvell,tad-page-size", &tad_page_size);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't find tad-page-size property\n");
> +		return ret;
> +	}
> +
> +	ret = device_property_read_u32(dev, "marvell,tad-pmu-page-size",
> +				       &tad_pmu_page_size);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Can't find tad-pmu-page-size property\n");
> +		return ret;
> +	}

Why do you think these properties are necessary?

These should almost certainly be provided by IO resources, and shouldn't need a
custom property.

Thanks,
Mark.

RE: [EXT] Re: [PATCH 4/6] dt-bindings: perf: marvell: Add YAML schemas for Marvell Odyssey LLC-TAD pmu

[Gowthami Thiagarajan]

Hi,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Sunday, July 2, 2023 2:56 PM
> To: Gowthami Thiagarajan <gthiagarajan@marvell.com>; will@kernel.org; mark.rutland@arm.com; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Cc: Sunil Kovvuri Goutham <sgoutham@marvell.com>; Bharat Bhushan <bbhushan2@marvell.com>; George
> Cherian <gcherian@marvell.com>; Linu Cherian <lcherian@marvell.com>
> Subject: [EXT] Re: [PATCH 4/6] dt-bindings: perf: marvell: Add YAML schemas for Marvell Odyssey LLC-
> TAD pmu
> 
> External Email
> 
> ----------------------------------------------------------------------
> On 30/06/2023 14:03, Gowthami Thiagarajan wrote:
> > Add device tree bindings for Marvell Odyssey LLC-TAD performance
> > monitor unit
> >
> > Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com>
> 
> Please use scripts/get_maintainers.pl to get a list of necessary people
> and lists to CC.  It might happen, that command when run on an older
> kernel, gives you outdated entries.  Therefore please be sure you base
> your patches on recent Linux kernel.
> 
> You missed at least DT list (maybe more), so this won't be tested by our
> tools. Performing review on untested code might be a waste of time, thus
> I will skip this patch entirely till you follow the process allowing the
> patch to be tested.
> 
> Please kindly resend and include all necessary To/Cc entries.

Missed sending to DT list. Will add everyone while resending.

> 
> > ---
> >  .../bindings/perf/marvell-odyssey-tad.yaml    | 63 +++++++++++++++++++
> >  1 file changed, 63 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/perf/marvell-odyssey-tad.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/perf/marvell-odyssey-tad.yaml
> b/Documentation/devicetree/bindings/perf/marvell-odyssey-tad.yaml
> > new file mode 100644
> > index 000000000000..139567166f77
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/perf/marvell-odyssey-tad.yaml
> 
> Filename does not match compatible.

Ack.

> 
> 
> 
> > +examples:
> > +  - |
> > +
> > +    tad {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        tad_pmu@80000000 {
> 
> Node names should be generic. See also explanation and list of examples
> in DT specification:
> https://urldefense.proofpoint.com/v2/url?u=https-3A__devicetree-
> 2Dspecification.readthedocs.io_en_latest_chapter2-2Ddevicetree-2Dbasics.html-23generic-2Dnames-
> 2Drecommendation&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=jXyDe1pTHpmFH9abKZ66HpyuWJquipUlbI9yktcgBgU&m=GdU
> qi5f4jgPpB5WqFNmUoZBzc3eljzT7CbdkU7I2c2GJn6OrVE5DUNFL2qeL_k05&s=kjtFmv_ypTnmDrZqOm7nTET--
> ZCfPMWoyFBAQZVd7J8&e=

 Ack. 

> 
> > +            compatible = "marvell,odyssey-tad-pmu";
> > +            reg = <0x87E2 0x2B030000 0x0 0x1000>;
> 
> lowercase hex

Ack. Will make the changes in the next version.

-Thanks,
Gowthami
> 
> Best regards,
> Krzysztof

RE: [EXT] Re: [PATCH 1/6] perf/marvell: Marvell PEM performance monitor support

[Gowthami Thiagarajan]

Hi,

> -----Original Message-----
> From: Mark Rutland <mark.rutland@arm.com>
> Sent: Friday, July 28, 2023 8:32 PM
> To: Gowthami Thiagarajan <gthiagarajan@marvell.com>
> Cc: will@kernel.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Sunil Kovvuri
> Goutham <sgoutham@marvell.com>; Bharat Bhushan <bbhushan2@marvell.com>; George Cherian
> <gcherian@marvell.com>; Linu Cherian <lcherian@marvell.com>
> Subject: [EXT] Re: [PATCH 1/6] perf/marvell: Marvell PEM performance monitor support
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi,
> 
> On Fri, Jun 30, 2023 at 05:33:46PM +0530, Gowthami Thiagarajan wrote:
> > PCI Express Interface PMU includes various performance counters to monitor
> > the data that is transmitted over the PCIe link. The counters track various
> > inbound and outbound transactions which includes separate counters for
> > posted/non-posted/completion TLPs. Also, inbound and outbound memory read
> > requests along with their latencies can also be monitored. Address
> > Translation Services(ATS)events such as ATS Translation, ATS Page Request,
> > ATS Invalidation along with their corresponding latencies are also
> > supported.
> >
> > The performance counters are 64 bits wide.
> >
> > For instance,
> > perf stat -e ib_tlp_pr <workload>
> > tracks the inbound posted TLPs for the workload.
> >
> > Signed-off-by: Linu Cherian <lcherian@marvell.com>
> > Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com>
> 
> This generally looks fine; I have a few comments below.
> 
> [...]
> 
> > diff --git a/drivers/perf/marvell_pem_pmu.c b/drivers/perf/marvell_pem_pmu.c
> > new file mode 100644
> > index 000000000000..fb27112aa7d4
> > --- /dev/null
> > +++ b/drivers/perf/marvell_pem_pmu.c
> > @@ -0,0 +1,433 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Marvell PEM(PCIe RC) Performance Monitor Driver
> > + *
> > + * Copyright (C) 2023 Marvell.
> > + */
> 
> Nit: please follow the preferred coding style for comments. This should have a
> newline immediately after the '/*', e.g.
> 
> 	/*
> 	 * Marvell PEM(PCIe RC) Performance Monitor Driver
> 	 *
> 	 * Copyright (C) 2023 Marvell.
> 	 */
> 
> Likewise for all other multi-line comments.

Ack.
> 
> > +#include <linux/acpi.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_device.h>
> > +#include <linux/perf_event.h>
> > +
> > +/* Each of these events maps to a free running 64 bit counter
> > + * with no event control, but can be reset.
> > + *
> > + */
> > +enum pem_events {
> > +	IB_TLP_NPR,
> > +	IB_TLP_PR,
> > +	IB_TLP_CPL,
> 
> > +static u64 eventid_to_offset_table[] = {
> > +	0x0,
> > +	0x8,
> > +	0x10,
> 
> I assume the event IDs are the values in the pem_events enum, so please use
> array initalizers here to make that clear, e.g.
> 
> 	static u64 eventid_to_offset_table[] = {
> 		[IB_TLP_NPR]	= 0x0,
> 		[IB_TLP_PR]	= 0x8,
> 		[IB_TLP_CPL]	 0x10,
> 		...
> 	};
> 
> [...]

Ack. Yes. IDs refer to the pem_events. 

> 
> > +static int pem_perf_event_init(struct perf_event *event)
> > +{
> > +	struct pem_pmu *pmu = to_pem_pmu(event->pmu);
> > +	struct hw_perf_event *hwc = &event->hw;
> > +
> > +	if (event->attr.type != event->pmu->type)
> > +		return -ENOENT;
> > +
> > +	if (is_sampling_event(event)) {
> 
> Don't we also need to check for:
> 
> 	event->attach_state & PERF_ATTACH_TASK

Ack. Will add this check.

> 
> > +		dev_info(pmu->dev, "Sampling not supported!\n");
> > +		return -EOPNOTSUPP;
> > +	}
> 
> Please delete this dev_info().

Ack.
> 
> > +
> > +	if (event->cpu < 0) {
> > +		dev_warn(pmu->dev, "Can't provide per-task data!\n");
> > +		return -EOPNOTSUPP;
> > +	}
> 
> Likewise, please delete this dev_warn().
Ack.
> 
> > +
> > +	/*  We must NOT create groups containing mixed PMUs */
> > +	if (event->group_leader->pmu != event->pmu &&
> > +	    !is_software_event(event->group_leader))
> > +		return -EINVAL;
> > +
> > +	/* Set ownership of event to one CPU, same event can not be observed
> > +	 * on multiple cpus at same time.
> > +	 */
> 
> Please fix this comment style (or delete the comment).
Ack.

-Thanks,
Gowthami
> 
> > +	event->cpu = pmu->cpu;
> > +	hwc->idx = -1;
> > +	return 0;
> > +}
> 
> Thanks,
> Mark.

Re: [PATCH 3/6] perf/marvell : Odyssey LLC-TAD performance monitor support

[Gowthami Thiagarajan]

Hi,

> -----Original Message-----
> From: Mark Rutland <mark.rutland@arm.com>
> Sent: Friday, July 28, 2023 9:08 PM
> To: Gowthami Thiagarajan <gthiagarajan@marvell.com>
> Cc: will@kernel.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Sunil Kovvuri
> Goutham <sgoutham@marvell.com>; Bharat Bhushan <bbhushan2@marvell.com>; George Cherian
> <gcherian@marvell.com>; Linu Cherian <lcherian@marvell.com>
> Subject: [EXT] Re: [PATCH 3/6] perf/marvell : Odyssey LLC-TAD performance monitor support
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Fri, Jun 30, 2023 at 05:33:48PM +0530, Gowthami Thiagarajan wrote:
> > Each TAD provides eight 64-bit counters for monitoring
> > cache behavior.The driver always configures the same counter for
> > all the TADs. The user would end up effectively reserving one of
> > eight counters in every TAD to look across all TADs.
> > The occurrences of events are aggregated and presented to the user
> > at the end of running the workload. The driver does not provide a
> > way for the user to partition TADs so that different TADs are used for
> > different applications.
> >
> > The performance events reflect various internal or interface activities.
> > By combining the values from multiple performance counters, cache
> > performance can be measured in terms such as: cache miss rate, cache
> > allocations, interface retry rate, internal resource occupancy, etc.
> >
> > Each supported counter's event and formatting information is exposed
> > to sysfs at /sys/devices/tad/. Use perf tool stat command to measure
> > the pmu events. For instance:
> >
> > perf stat -e tad_hit_ltg,tad_hit_dtg <workload>
> >
> > Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com>
> 
> This generally looks ok; I have a few comments below.
> 
> [...]
> 
> > +static void tad_pmu_event_counter_stop(struct perf_event *event, int flags)
> > +{
> > +	struct tad_pmu *tad_pmu = to_tad_pmu(event->pmu);
> > +	struct hw_perf_event *hwc = &event->hw;
> > +	u32 counter_idx = hwc->idx;
> > +	int tad_region;
> > +
> > +	/* TAD()_PFC() stop counting on the write
> > +	 * which sets TAD()_PRF()[CNTSEL] == 0
> > +	 */
> 
> Please fix the comment style.
> 
> Likewise for all other instances within this file.
> 
> [...]
Ack.
> 
> > +static int tad_pmu_event_counter_add(struct perf_event *event, int flags)
> > +{
> > +	struct tad_pmu *tad_pmu = to_tad_pmu(event->pmu);
> > +	struct hw_perf_event *hwc = &event->hw;
> > +	int idx;
> > +
> > +	/* Get a free counter for this event */
> > +	idx = find_first_zero_bit(tad_pmu->counters_map, TAD_MAX_COUNTERS);
> > +	if (idx == TAD_MAX_COUNTERS)
> > +		return -EAGAIN;
> > +
> > +	set_bit(idx, tad_pmu->counters_map);
> > +
> > +	hwc->idx = idx;
> > +	hwc->state = PERF_HES_STOPPED;
> > +	tad_pmu->events[idx] = event;
> > +
> > +	if (flags & PERF_EF_START)
> > +		tad_pmu_event_counter_start(event, flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static int tad_pmu_event_init(struct perf_event *event)
> > +{
> > +	struct tad_pmu *tad_pmu = to_tad_pmu(event->pmu);
> > +
> > +	if (event->attr.type != event->pmu->type)
> > +		return -ENOENT;
> 
> Why is this not rejecting smapling events, as patch 1 does?
This is missed here. Will add the check for rejecting sampling events.
> 
> > +
> > +	if (!event->attr.disabled)
> > +		return -EINVAL;
> 
> Why?
Just checks the default disabled attribute.
> 
> > +
> > +	if (event->state != PERF_EVENT_STATE_OFF)
> > +		return -EINVAL;
> 
> Event groups need to be verified here too.
> 
> [...]
Ack. Will add the missed checks in the next version.
> 
> > +static int tad_pmu_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct tad_region *regions;
> > +	struct tad_pmu *tad_pmu;
> > +	struct resource *res;
> > +	u32 tad_pmu_page_size;
> > +	u32 tad_page_size;
> > +	u32 tad_cnt;
> > +	int i, ret;
> > +	char *name;
> > +
> > +	tad_pmu = devm_kzalloc(&pdev->dev, sizeof(*tad_pmu), GFP_KERNEL);
> > +	if (!tad_pmu)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, tad_pmu);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res) {
> > +		dev_err(&pdev->dev, "Mem resource not found\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	ret = device_property_read_u32(dev, "marvell,tad-page-size", &tad_page_size);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Can't find tad-page-size property\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = device_property_read_u32(dev, "marvell,tad-pmu-page-size",
> > +				       &tad_pmu_page_size);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Can't find tad-pmu-page-size property\n");
> > +		return ret;
> > +	}
> 
> Why do you think these properties are necessary?
> 
> These should almost certainly be provided by IO resources, and shouldn't need a
> custom property.

IO resources don't provide all the information in this case. Need to vary these values
at different boot times. So, kept these custom properties.

Thanks,
Gowthami
> 
> Thanks,
> Mark.

Re: [PATCH 3/6] perf/marvell : Odyssey LLC-TAD performance monitor support

[Will Deacon]

On Sat, Aug 12, 2023 at 01:51:00PM +0000, Gowthami Thiagarajan wrote:
> > > +static int tad_pmu_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct tad_region *regions;
> > > +	struct tad_pmu *tad_pmu;
> > > +	struct resource *res;
> > > +	u32 tad_pmu_page_size;
> > > +	u32 tad_page_size;
> > > +	u32 tad_cnt;
> > > +	int i, ret;
> > > +	char *name;
> > > +
> > > +	tad_pmu = devm_kzalloc(&pdev->dev, sizeof(*tad_pmu), GFP_KERNEL);
> > > +	if (!tad_pmu)
> > > +		return -ENOMEM;
> > > +
> > > +	platform_set_drvdata(pdev, tad_pmu);
> > > +
> > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +	if (!res) {
> > > +		dev_err(&pdev->dev, "Mem resource not found\n");
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	ret = device_property_read_u32(dev, "marvell,tad-page-size", &tad_page_size);
> > > +	if (ret) {
> > > +		dev_err(&pdev->dev, "Can't find tad-page-size property\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = device_property_read_u32(dev, "marvell,tad-pmu-page-size",
> > > +				       &tad_pmu_page_size);
> > > +	if (ret) {
> > > +		dev_err(&pdev->dev, "Can't find tad-pmu-page-size property\n");
> > > +		return ret;
> > > +	}
> > 
> > Why do you think these properties are necessary?
> > 
> > These should almost certainly be provided by IO resources, and shouldn't need a
> > custom property.
> 
> IO resources don't provide all the information in this case. Need to vary these values
> at different boot times. So, kept these custom properties.

Please can you be a bit more specific here? Which information is not
provided?

Will

Re: [PATCH 3/6] perf/marvell : Odyssey LLC-TAD performance monitor support

[Mark Rutland]

On Sat, Aug 12, 2023 at 01:51:00PM +0000, Gowthami Thiagarajan wrote:
> > -----Original Message-----
> > From: Mark Rutland <mark.rutland@arm.com>
> > Sent: Friday, July 28, 2023 9:08 PM
> > To: Gowthami Thiagarajan <gthiagarajan@marvell.com>
> > Cc: will@kernel.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Sunil Kovvuri
> > Goutham <sgoutham@marvell.com>; Bharat Bhushan <bbhushan2@marvell.com>; George Cherian
> > <gcherian@marvell.com>; Linu Cherian <lcherian@marvell.com>
> > Subject: [EXT] Re: [PATCH 3/6] perf/marvell : Odyssey LLC-TAD performance monitor support
> > 
> > External Email
> > 
> > ----------------------------------------------------------------------
> > On Fri, Jun 30, 2023 at 05:33:48PM +0530, Gowthami Thiagarajan wrote:

> > > +static int tad_pmu_event_counter_add(struct perf_event *event, int flags)
> > > +{

> > > +	if (!event->attr.disabled)
> > > +		return -EINVAL;
> > 
> > Why?
> Just checks the default disabled attribute.

Why does it matter?

What's the problem with opening an event which is *not* disabled?

[...]

> > > +static int tad_pmu_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	struct tad_region *regions;
> > > +	struct tad_pmu *tad_pmu;
> > > +	struct resource *res;
> > > +	u32 tad_pmu_page_size;
> > > +	u32 tad_page_size;
> > > +	u32 tad_cnt;
> > > +	int i, ret;
> > > +	char *name;
> > > +
> > > +	tad_pmu = devm_kzalloc(&pdev->dev, sizeof(*tad_pmu), GFP_KERNEL);
> > > +	if (!tad_pmu)
> > > +		return -ENOMEM;
> > > +
> > > +	platform_set_drvdata(pdev, tad_pmu);
> > > +
> > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > +	if (!res) {
> > > +		dev_err(&pdev->dev, "Mem resource not found\n");
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	ret = device_property_read_u32(dev, "marvell,tad-page-size", &tad_page_size);
> > > +	if (ret) {
> > > +		dev_err(&pdev->dev, "Can't find tad-page-size property\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = device_property_read_u32(dev, "marvell,tad-pmu-page-size",
> > > +				       &tad_pmu_page_size);
> > > +	if (ret) {
> > > +		dev_err(&pdev->dev, "Can't find tad-pmu-page-size property\n");
> > > +		return ret;
> > > +	}
> > 
> > Why do you think these properties are necessary?
> > 
> > These should almost certainly be provided by IO resources, and shouldn't need a
> > custom property.
> 
> IO resources don't provide all the information in this case. Need to vary these values
> at different boot times. So, kept these custom properties.

I think the only rason the information is missing is that your DT binding isn't quite right.

Later on you do:

+       for (i = 0; i < tad_cnt && res->start < res->end; i++) {
+               regions[i].base = devm_ioremap(&pdev->dev,
+                                              res->start,
+                                              tad_pmu_page_size);
+               if (IS_ERR(regions[i].base)) {
+                       dev_err(&pdev->dev, "TAD%d ioremap fail\n", i);
+                       return -ENOMEM;
+               }
+               res->start += tad_page_size;
+       }

... which means you're splitting one reg entry into multiple mappings, whereas
you could have multiple reg entries, one per TAD page.

Thanks,
Mark.

RE: [EXT] Re: [PATCH 3/6] perf/marvell : Odyssey LLC-TAD performance monitor support

[Gowthami Thiagarajan]

Hi Mark/Will,

> -----Original Message-----
> From: Mark Rutland <mark.rutland@arm.com>
> Sent: Tuesday, August 15, 2023 6:54 PM
> To: Gowthami Thiagarajan <gthiagarajan@marvell.com>
> Cc: will@kernel.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Sunil Kovvuri
> Goutham <sgoutham@marvell.com>; Bharat Bhushan <bbhushan2@marvell.com>; George Cherian
> <gcherian@marvell.com>; Linu Cherian <lcherian@marvell.com>
> Subject: [EXT] Re: [PATCH 3/6] perf/marvell : Odyssey LLC-TAD performance monitor support
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Sat, Aug 12, 2023 at 01:51:00PM +0000, Gowthami Thiagarajan wrote:
> > > -----Original Message-----
> > > From: Mark Rutland <mark.rutland@arm.com>
> > > Sent: Friday, July 28, 2023 9:08 PM
> > > To: Gowthami Thiagarajan <gthiagarajan@marvell.com>
> > > Cc: will@kernel.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Sunil
> Kovvuri
> > > Goutham <sgoutham@marvell.com>; Bharat Bhushan <bbhushan2@marvell.com>; George Cherian
> > > <gcherian@marvell.com>; Linu Cherian <lcherian@marvell.com>
> > > Subject: [EXT] Re: [PATCH 3/6] perf/marvell : Odyssey LLC-TAD performance monitor support
> > >
> > > External Email
> > >
> > > ----------------------------------------------------------------------
> > > On Fri, Jun 30, 2023 at 05:33:48PM +0530, Gowthami Thiagarajan wrote:
> 
> > > > +static int tad_pmu_event_counter_add(struct perf_event *event, int flags)
> > > > +{
> 
> > > > +	if (!event->attr.disabled)
> > > > +		return -EINVAL;
> > >
> > > Why?
> > Just checks the default disabled attribute.
> 
> Why does it matter?
> 
> What's the problem with opening an event which is *not* disabled?
> 
> [...]
Checked this. Don't find the significance of the validation. Can be removed.
> 
> > > > +static int tad_pmu_probe(struct platform_device *pdev)
> > > > +{
> > > > +	struct device *dev = &pdev->dev;
> > > > +	struct tad_region *regions;
> > > > +	struct tad_pmu *tad_pmu;
> > > > +	struct resource *res;
> > > > +	u32 tad_pmu_page_size;
> > > > +	u32 tad_page_size;
> > > > +	u32 tad_cnt;
> > > > +	int i, ret;
> > > > +	char *name;
> > > > +
> > > > +	tad_pmu = devm_kzalloc(&pdev->dev, sizeof(*tad_pmu), GFP_KERNEL);
> > > > +	if (!tad_pmu)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	platform_set_drvdata(pdev, tad_pmu);
> > > > +
> > > > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > +	if (!res) {
> > > > +		dev_err(&pdev->dev, "Mem resource not found\n");
> > > > +		return -ENODEV;
> > > > +	}
> > > > +
> > > > +	ret = device_property_read_u32(dev, "marvell,tad-page-size", &tad_page_size);
> > > > +	if (ret) {
> > > > +		dev_err(&pdev->dev, "Can't find tad-page-size property\n");
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	ret = device_property_read_u32(dev, "marvell,tad-pmu-page-size",
> > > > +				       &tad_pmu_page_size);
> > > > +	if (ret) {
> > > > +		dev_err(&pdev->dev, "Can't find tad-pmu-page-size property\n");
> > > > +		return ret;
> > > > +	}
> > >
> > > Why do you think these properties are necessary?
> > >
> > > These should almost certainly be provided by IO resources, and shouldn't need a
> > > custom property.
> >
> > IO resources don't provide all the information in this case. Need to vary these values
> > at different boot times. So, kept these custom properties.
> 
> I think the only rason the information is missing is that your DT binding isn't quite right.
> 
> Later on you do:
> 
> +       for (i = 0; i < tad_cnt && res->start < res->end; i++) {
> +               regions[i].base = devm_ioremap(&pdev->dev,
> +                                              res->start,
> +                                              tad_pmu_page_size);
> +               if (IS_ERR(regions[i].base)) {
> +                       dev_err(&pdev->dev, "TAD%d ioremap fail\n", i);
> +                       return -ENOMEM;
> +               }
> +               res->start += tad_page_size;
> +       }
> 
> ... which means you're splitting one reg entry into multiple mappings, whereas
> you could have multiple reg entries, one per TAD page.

These properties are updated by the firmware at runtime. In order to avoid the firmware updating multiple nodes, a single node for the entire system has been kept. So that the firmware can just update at a single place  .

Thanks,
Gowthami

> 
> Thanks,
> Mark.