On Tue, Apr 30, 2019 at 11:30:17AM -0500, Zhi Li wrote:
> On Tue, Apr 30, 2019 at 5:51 AM Will Deacon <will.deacon@xxxxxxx> wrote:
> >
> > On Mon, Apr 29, 2019 at 04:44:32PM +0000, Frank Li wrote:
> > > diff --git a/drivers/perf/fsl_imx8_ddr_perf.c b/drivers/perf/fsl_imx8_ddr_perf.c
> > > new file mode 100644
> > > index 0000000..087d75a
> > > --- /dev/null
> > > +++ b/drivers/perf/fsl_imx8_ddr_perf.c
> > > @@ -0,0 +1,589 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Copyright 2017 NXP
> > > + * Copyright 2016 Freescale Semiconductor, Inc.
> > > + */
> > > +
> > > +#include <linux/init.h>
> > > +#include <linux/interrupt.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/of_irq.h>
> > > +#include <linux/perf_event.h>
> > > +#include <linux/slab.h>
> > > +
> > > +#define COUNTER_CNTL 0x0
> > > +#define COUNTER_READ 0x20
> > > +
> > > +#define COUNTER_DPCR1 0x30
> > > +
> > > +#define CNTL_OVER 0x1
> > > +#define CNTL_CLEAR 0x2
> > > +#define CNTL_EN 0x4
> > > +#define CNTL_EN_MASK 0xFFFFFFFB
> > > +#define CNTL_CLEAR_MASK 0xFFFFFFFD
> > > +#define CNTL_OVER_MASK 0xFFFFFFFE
> > > +
> > > +#define CNTL_CSV_SHIFT 24
> > > +#define CNTL_CSV_MASK (0xFF << CNTL_CSV_SHIFT)
> > > +
> > > +#define EVENT_CYCLES_ID 0
> > > +#define EVENT_CYCLES_COUNTER 0
> > > +#define NUM_COUNTERS 4
> > > +
> > > +#define to_ddr_pmu(p) container_of(p, struct ddr_pmu, pmu)
> > > +
> > > +#define DDR_PERF_DEV_NAME "ddr_perf"
> >
> > This is far too generic. Please make it something like "imx8_ddr_perf_pmu".
> >
> > > +static int ddr_perf_probe(struct platform_device *pdev)
> > > +{
> > > + struct ddr_pmu *pmu;
> > > + struct device_node *np;
> > > + void __iomem *base;
> > > + struct resource *iomem;
> > > + char *name;
> > > + char *hpname;
> > > + int num;
> > > + int ret;
> > > + int irq;
> > > +
> > > + iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > + base = devm_ioremap_resource(&pdev->dev, iomem);
> > > + if (IS_ERR(base))
> > > + return PTR_ERR(base);
> > > +
> > > + np = pdev->dev.of_node;
> > > +
> > > + pmu = devm_kzalloc(&pdev->dev, sizeof(*pmu), GFP_KERNEL);
> > > + if (!pmu)
> > > + return -ENOMEM;
> > > +
> > > + num = ddr_perf_init(pmu, base, &pdev->dev);
> > > +
> > > + platform_set_drvdata(pdev, pmu);
> > > +
> > > + name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ddr%d", num);
> > > + if (!name)
> > > + return -ENOMEM;
> > > +
> > > + hpname = devm_kasprintf(&pdev->dev, GFP_KERNEL,
> > > + "perf/imx/ddr%d:online", num);
> > > + if (!hpname)
> > > + return -ENOMEM;
> > > +
> > > + pmu->cpu = raw_smp_processor_id();
> > > + ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, hpname, NULL,
> > > + ddr_perf_offline_cpu);
> >
> > This doesn't seem right to me. My understanding of the cpuhp mechanism
> > is that you register a single multi-instance state as part of driver
> > initialisation, and then add an instance for each device you probe.
> > That means you don't need to kasprintf the callback name as you are above --
> > you can just use DDR_PERF_DEV_NAME instead.
> >
> > Please see how other perf drivers manage this on my for-next/perf branch.
> >
> > > +
> > > + if (ret < 0) {
> > > + dev_err(&pdev->dev, "cpuhp_setup_state_multi failed\n");
> > > + goto ddr_perf_err;
> > > + }
> > > +
> > > + pmu->cpuhp_state = ret;
> > > +
> > > + /* Register the pmu instance for cpu hotplug */
> > > + cpuhp_state_add_instance_nocalls(pmu->cpuhp_state, &pmu->node);
> > > +
> > > + ret = perf_pmu_register(&pmu->pmu, name, -1);
> >
> > Again, the string you're passing in here is too generic. I suggest taking
> > DDR_PERF_DEV_NAME and adding "_%d" to the end to paste in your 'num'
> > identifier.
> >
> > Sorry if this feels like pedantry, but this gets exposed to userspace
> > via sysfs, so we need to be careful with the namespace.
>
> imx8_ddr_perf_pmu is too long to use. how about imx_ddr%d?
Too long for what? Drop the "perf" bit if you want, but I think we need
"pmu" in there.
Will
