Re: [RESEND PATCH v3] perf/marvell: Marvell PEM performance monitor support

From: Andrew Lunn <andrew@lunn.ch>
To: Gowthami Thiagarajan <gthiagarajan@marvell.com>
Cc: will@kernel.org, mark.rutland@arm.com,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, sgoutham@marvell.com,
	gcherian@marvell.com, lcherian@marvell.com
Subject: Re: [RESEND PATCH v3] perf/marvell: Marvell PEM performance monitor support
Date: Wed, 27 Mar 2024 14:11:08 +0100	[thread overview]
Message-ID: <695199ba-c71d-43ec-8305-8f52488b8c5d@lunn.ch> (raw)
In-Reply-To: <20240327072117.1556653-1-gthiagarajan@marvell.com>

On Wed, Mar 27, 2024 at 12:51:17PM +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: Gowthami Thiagarajan <gthiagarajan@marvell.com>
> ---
> v2->v3 changes:
> - Dropped device tree support as the acpi table based probing is used.

So people using DT cannot use this driver? Can they use the PCIe
interface?

There does not appear to be any ACPI binding, it is not reading any
properties from ACPI tables etc. So the DT binding should be
trivial...

> index 000000000000..d4175483b982
> --- /dev/null
> +++ b/drivers/perf/marvell_pem_pmu.c
> @@ -0,0 +1,428 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Marvell PEM(PCIe RC) Performance Monitor Driver
> + *
> + * Copyright (C) 2024 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>

Why do you need these header files? I don't see any calls to of_
functions.

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

Please don't spam the kernel log like this.

       Andrew