[V5,2/4] drivers/perf: imx_ddr: Add ddr performance counter support

Add ddr performance monitor support for iMX8QXP

There are 4 counters for ddr perfomance events.
counter 0 is dedicated for cycles.
you choose any up to 3 no cycles events.

for example:

perf stat -a -e ddr0/read-cycles/,ddr0/write-cycles/,ddr0/precharge/ ls
perf stat -a -e ddr0/cycles/,ddr0/read-access/,ddr0/write-access/ ls

Support below events.

  ddr0/activate/                                     [Kernel PMU event]
  ddr0/cycles/                                       [Kernel PMU event]
  ddr0/hp-read-credit-cnt/                           [Kernel PMU event]
  ddr0/hp-read/                                      [Kernel PMU event]
  ddr0/hp-req-nodcredit/                             [Kernel PMU event]
  ddr0/hp-xact-credit/                               [Kernel PMU event]
  ddr0/load-mode/                                    [Kernel PMU event]
  ddr0/lp-read-credit-cnt/                           [Kernel PMU event]
  ddr0/lp-req-nocredit/                              [Kernel PMU event]
  ddr0/lp-xact-credit/                               [Kernel PMU event]
  ddr0/mwr/                                          [Kernel PMU event]
  ddr0/precharge/                                    [Kernel PMU event]
  ddr0/raw-hazard/                                   [Kernel PMU event]
  ddr0/read-access/                                  [Kernel PMU event]
  ddr0/read-activate/                                [Kernel PMU event]
  ddr0/read-command/                                 [Kernel PMU event]
  ddr0/read-cycles/                                  [Kernel PMU event]
  ddr0/read-modify-write-command/                    [Kernel PMU event]
  ddr0/read-queue-depth/                             [Kernel PMU event]
  ddr0/read-write-transition/                        [Kernel PMU event]
  ddr0/read/                                         [Kernel PMU event]
  ddr0/refresh/                                      [Kernel PMU event]
  ddr0/selfresh/                                     [Kernel PMU event]
  ddr0/wr-xact-credit/                               [Kernel PMU event]
  ddr0/write-access/                                 [Kernel PMU event]
  ddr0/write-command/                                [Kernel PMU event]
  ddr0/write-credit-cnt/                             [Kernel PMU event]
  ddr0/write-cycles/                                 [Kernel PMU event]
  ddr0/write-queue-depth/                            [Kernel PMU event]
  ddr0/write/

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Change from v4 to v5
 * Remove AXI ID filter function

Change from v3 to v4
 * Change FSL_IMX8_DDR_PERF to FSL_IMX8_DDR_PMU
 * sort include
 * remove struct fsl_ddr_devtype_data
 * Added comment need disable control first
 * Added comment about must enable cycle counter
 * Added macro for EVENT_AXI_READ, remove hardcode 0x41 and 0x42
 * Added comment about cycle counter is fastest one

Change from v2 to v3
 * remove kfree

Change from V1 to V2
 * update Kconfig by use i.MX8 instead of i.MX8 QXP
 * remove gpl statememnt since SPDX tag
 * use dev_kzalloc
 * use dev_err
 * commit message show axi_read 0x41\axi_write 0x42
 * commit message show cycles must be enabled
 * Irq only issue at cycles overflow
 * use NUM_COUNTER
 * use devm_request_irq
 * add hotplug callback to handle context migration

 drivers/perf/Kconfig             |   7 +
 drivers/perf/Makefile            |   1 +
 drivers/perf/fsl_imx8_ddr_perf.c | 553 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 561 insertions(+)
 create mode 100644 drivers/perf/fsl_imx8_ddr_perf.c

Hi Frank,

On Tue, Apr 16, 2019 at 08:39:33PM +0000, Frank Li wrote:
> Add ddr performance monitor support for iMX8QXP
> 
> There are 4 counters for ddr perfomance events.
> counter 0 is dedicated for cycles.
> you choose any up to 3 no cycles events.

I was about to pick this up, but I still have a few questions/comments
on the code:

> +static irqreturn_t ddr_perf_irq_handler(int irq, void *p)
> +{
> +	int i;
> +	u8 reg;
> +	int val;
> +	int counter;
> +	struct ddr_pmu *pmu = (struct ddr_pmu *) p;
> +	struct perf_event *event;
> +
> +	/* Only cycles counter overflowed can issue irq. all counters will
> +	 * be stopped when cycles counter overflow. but other counter don't stop
> +	 * when overflow happen. Update all of the local counter values,
> +	 * then reset the cycles counter, so the others can continue
> +	 * counting. cycles counter is fastest counter in all events. at last
> +	 * 4 times than other counters.
> +	 */
> +	for (i = 0; i < NUM_COUNTER; i++) {
> +
> +		if (!pmu->active_events[i])
> +			continue;
> +
> +		event = pmu->active_events[i];
> +		counter = event->hw.idx;
> +		reg = counter * 4 + COUNTER_CNTL;
> +		val = readl(pmu->base + reg);

Does this read have a side-effect, or can it be removed given that you don't
use val for anything else?

> +		ddr_perf_event_update(event);
> +
> +		if (counter == EVENT_CYCLES_COUNTER) {
> +			ddr_perf_event_enable(pmu,
> +					      EVENT_CYCLES_ID,
> +					      EVENT_CYCLES_COUNTER,
> +					      true);
> +			ddr_perf_event_update(event);
> +		}
> +	}
> +
> +	return IRQ_HANDLED;
> +}

What stops the IRQ handler running concurrently with perf callbacks? I can't
see any locking here, or is the IRQ supposed to be affine to the same CPU
that's handling the perf context?

> +static int ddr_perf_offline_cpu(unsigned int cpu, struct hlist_node *node)
> +{
> +	struct ddr_pmu *pmu = hlist_entry_safe(node, struct ddr_pmu, node);
> +	int target;
> +
> +	if (!cpumask_test_and_clear_cpu(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);
> +	cpumask_set_cpu(target, &pmu->cpu);
> +
> +	return 0;
> +}
> +
> +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;
> +	u32 irq;
> +	const struct of_device_id *of_id =
> +		of_match_device(imx_ddr_pmu_dt_ids, &pdev->dev);
> +
> +	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);
> +
> +	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->flags = (uintptr_t) of_id->data;
> +
> +	cpumask_set_cpu(raw_smp_processor_id(), &pmu->cpu);
> +	ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, hpname, NULL,
> +					 ddr_perf_offline_cpu);
> +
> +	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);
> +	if (ret)
> +		goto ddr_perf_err;
> +
> +	/* Request irq */
> +	irq = of_irq_get(np, 0);
> +	if (irq < 0) {

irq is a u32 so this will never execute. You need to make it an int.

> +		dev_err(&pdev->dev, "Failed to get irq: %d", irq);
> +		ret = irq;
> +		goto ddr_perf_irq_err;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, irq,
> +					ddr_perf_irq_handler,
> +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,

Can you explain why you need to pass these specific IRQ flags, please? I'm
struggling to see what the ONESHOT is buying you.

Will

On 16/04/2019 21:39, Frank Li wrote:
[...]
> +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;
> +	u32 irq;
> +	const struct of_device_id *of_id =
> +		of_match_device(imx_ddr_pmu_dt_ids, &pdev->dev);
> +
> +	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);
> +
> +	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->flags = (uintptr_t) of_id->data;
> +
> +	cpumask_set_cpu(raw_smp_processor_id(), &pmu->cpu);
> +	ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, hpname, NULL,
> +					 ddr_perf_offline_cpu);
> +
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "cpuhp_setup_state_multi failed\n");
> +		goto ddr_perf_err;
> +	}

Since you don't seem to ever remove this state again, this looks like it 
would make subsequent loads of the module always fail after it's been 
loaded and unloaded once - have you tried that?

Robin.

> +
> +	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);
> +	if (ret)
> +		goto ddr_perf_err;
> +
> +	/* Request irq */
> +	irq = of_irq_get(np, 0);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "Failed to get irq: %d", irq);
> +		ret = irq;
> +		goto ddr_perf_irq_err;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, irq,
> +					ddr_perf_irq_handler,
> +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> +					DDR_PERF_DEV_NAME,
> +					pmu);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Request irq failed: %d", ret);
> +		goto ddr_perf_irq_err;
> +	}
> +
> +	return 0;
> +
> +ddr_perf_irq_err:
> +	perf_pmu_unregister(&(pmu->pmu));
> +
> +ddr_perf_err:
> +	if (pmu->cpuhp_state)
> +		cpuhp_state_remove_instance_nocalls(pmu->cpuhp_state, &pmu->node);
> +
> +	dev_warn(&pdev->dev, "i.MX8 DDR Perf PMU failed (%d), disabled\n", ret);
> +	return ret;
> +}
> +
> +static int ddr_perf_remove(struct platform_device *pdev)
> +{
> +	struct ddr_pmu *pmu = platform_get_drvdata(pdev);
> +
> +	cpuhp_state_remove_instance_nocalls(pmu->cpuhp_state, &pmu->node);
> +	perf_pmu_unregister(&pmu->pmu);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver imx_ddr_pmu_driver = {
> +	.driver         = {
> +		.name   = "imx-ddr-pmu",
> +		.of_match_table = imx_ddr_pmu_dt_ids,
> +	},
> +	.probe          = ddr_perf_probe,
> +	.remove         = ddr_perf_remove,
> +};
> +
> +static int __init imx_ddr_pmu_init(void)
> +{
> +	return platform_driver_register(&imx_ddr_pmu_driver);
> +}
> +
> +module_init(imx_ddr_pmu_init);
> +
>

On Wed, Apr 24, 2019 at 5:58 AM Will Deacon <will.deacon@arm.com> wrote:
>
> Hi Frank,
>
> On Tue, Apr 16, 2019 at 08:39:33PM +0000, Frank Li wrote:
> > Add ddr performance monitor support for iMX8QXP
> >
> > There are 4 counters for ddr perfomance events.
> > counter 0 is dedicated for cycles.
> > you choose any up to 3 no cycles events.
>
> I was about to pick this up, but I still have a few questions/comments
> on the code:
>
> > +static irqreturn_t ddr_perf_irq_handler(int irq, void *p)
> > +{
> > +     int i;
> > +     u8 reg;
> > +     int val;
> > +     int counter;
> > +     struct ddr_pmu *pmu = (struct ddr_pmu *) p;
> > +     struct perf_event *event;
> > +
> > +     /* Only cycles counter overflowed can issue irq. all counters will
> > +      * be stopped when cycles counter overflow. but other counter don't stop
> > +      * when overflow happen. Update all of the local counter values,
> > +      * then reset the cycles counter, so the others can continue
> > +      * counting. cycles counter is fastest counter in all events. at last
> > +      * 4 times than other counters.
> > +      */
> > +     for (i = 0; i < NUM_COUNTER; i++) {
> > +
> > +             if (!pmu->active_events[i])
> > +                     continue;
> > +
> > +             event = pmu->active_events[i];
> > +             counter = event->hw.idx;
> > +             reg = counter * 4 + COUNTER_CNTL;
> > +             val = readl(pmu->base + reg);
>
> Does this read have a side-effect, or can it be removed given that you don't
> use val for anything else?

It can be removed.

>
> > +             ddr_perf_event_update(event);
> > +
> > +             if (counter == EVENT_CYCLES_COUNTER) {
> > +                     ddr_perf_event_enable(pmu,
> > +                                           EVENT_CYCLES_ID,
> > +                                           EVENT_CYCLES_COUNTER,
> > +                                           true);
> > +                     ddr_perf_event_update(event);
> > +             }
> > +     }
> > +
> > +     return IRQ_HANDLED;
> > +}
>
> What stops the IRQ handler running concurrently with perf callbacks? I can't
> see any locking here, or is the IRQ supposed to be affine to the same CPU
> that's handling the perf context?

I don't under your means here.
ddr_perf_event_update is lockless design.

I can't found problem if run ddr_perf_event_enable at both perf
context and IRQ concurrently.


>
> > +static int ddr_perf_offline_cpu(unsigned int cpu, struct hlist_node *node)
> > +{
> > +     struct ddr_pmu *pmu = hlist_entry_safe(node, struct ddr_pmu, node);
> > +     int target;
> > +
> > +     if (!cpumask_test_and_clear_cpu(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);
> > +     cpumask_set_cpu(target, &pmu->cpu);
> > +
> > +     return 0;
> > +}
> > +
> > +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;
> > +     u32 irq;
> > +     const struct of_device_id *of_id =
> > +             of_match_device(imx_ddr_pmu_dt_ids, &pdev->dev);
> > +
> > +     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);
> > +
> > +     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->flags = (uintptr_t) of_id->data;
> > +
> > +     cpumask_set_cpu(raw_smp_processor_id(), &pmu->cpu);
> > +     ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN, hpname, NULL,
> > +                                      ddr_perf_offline_cpu);
> > +
> > +     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);
> > +     if (ret)
> > +             goto ddr_perf_err;
> > +
> > +     /* Request irq */
> > +     irq = of_irq_get(np, 0);
> > +     if (irq < 0) {
>
> irq is a u32 so this will never execute. You need to make it an int.
>
> > +             dev_err(&pdev->dev, "Failed to get irq: %d", irq);
> > +             ret = irq;
> > +             goto ddr_perf_irq_err;
> > +     }
> > +
> > +     ret = devm_request_irq(&pdev->dev, irq,
> > +                                     ddr_perf_irq_handler,
> > +                                     IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>
> Can you explain why you need to pass these specific IRQ flags, please? I'm
> struggling to see what the ONESHOT is buying you.

I think default should be okay. I will update at next version
>
> Will

On Wed, Apr 24, 2019 at 11:58:22AM +0100, Will Deacon wrote:
> Hi Frank,
> 
> On Tue, Apr 16, 2019 at 08:39:33PM +0000, Frank Li wrote:
> > Add ddr performance monitor support for iMX8QXP
> > 
> > There are 4 counters for ddr perfomance events.
> > counter 0 is dedicated for cycles.
> > you choose any up to 3 no cycles events.
> 
> I was about to pick this up, but I still have a few questions/comments
> on the code:
> 
> > +static irqreturn_t ddr_perf_irq_handler(int irq, void *p)
> > +{
> > +	int i;
> > +	u8 reg;
> > +	int val;
> > +	int counter;
> > +	struct ddr_pmu *pmu = (struct ddr_pmu *) p;
> > +	struct perf_event *event;
> > +
> > +	/* Only cycles counter overflowed can issue irq. all counters will
> > +	 * be stopped when cycles counter overflow. but other counter don't stop
> > +	 * when overflow happen. Update all of the local counter values,
> > +	 * then reset the cycles counter, so the others can continue
> > +	 * counting. cycles counter is fastest counter in all events. at last
> > +	 * 4 times than other counters.
> > +	 */
> > +	for (i = 0; i < NUM_COUNTER; i++) {
> > +
> > +		if (!pmu->active_events[i])
> > +			continue;
> > +
> > +		event = pmu->active_events[i];
> > +		counter = event->hw.idx;
> > +		reg = counter * 4 + COUNTER_CNTL;
> > +		val = readl(pmu->base + reg);
> 
> Does this read have a side-effect, or can it be removed given that you don't
> use val for anything else?
> 
> > +		ddr_perf_event_update(event);
> > +
> > +		if (counter == EVENT_CYCLES_COUNTER) {
> > +			ddr_perf_event_enable(pmu,
> > +					      EVENT_CYCLES_ID,
> > +					      EVENT_CYCLES_COUNTER,
> > +					      true);
> > +			ddr_perf_event_update(event);
> > +		}
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> 
> What stops the IRQ handler running concurrently with perf callbacks? I can't
> see any locking here, or is the IRQ supposed to be affine to the same CPU
> that's handling the perf context?

To be correct, the IRQ affinity should be set to match the logical
affinity of the PMU, but it looks like that's not done at setup or
migrate time.

Frank, please take a look at what drivers/perf/arm-ccn.c does with teh
IRQ, including the use of IRQF_NOBALANCING | IRQF_NO_THREAD flags when
requesting the IRQ to begin with.

Thanks,
Mark.