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

[Gowthami Thiagarajan]

Hi Andrew,

Please find the responses inline.

Thanks,
Gowthami

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Wednesday, March 27, 2024 6:41 PM
> To: Gowthami Thiagarajan <gthiagarajan@xxxxxxxxxxx>
> Cc: will@xxxxxxxxxx; mark.rutland@xxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Sunil Kovvuri Goutham <sgoutham@xxxxxxxxxxx>; George Cherian
> <gcherian@xxxxxxxxxxx>; Linu Cherian <lcherian@xxxxxxxxxxx>
> Subject: [EXTERNAL] Re: [RESEND PATCH v3] perf/marvell: Marvell PEM performance monitor support
> 
> 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@xxxxxxxxxxx>
> > ---
> > 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...

The ACPI bindings are present in the driver. Have the ACPI ID MRVL000E tied here
and the resources are fetched from the ACPI table.

static const struct acpi_device_id pem_pmu_acpi_match[] = {
	{"MRVL000E", 0},
	{},
};
MODULE_DEVICE_TABLE(acpi, pem_pmu_acpi_match);

base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);

> 
> > 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.
> 
 These header files are not needed. I will remove them in the next version.

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

Will get this message removed.
> 
>        Andrew