From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.5 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS, URIBL_BLOCKED,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7FCCBC10F11 for ; Wed, 24 Apr 2019 10:58:42 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 4764D2175B for ; Wed, 24 Apr 2019 10:58:42 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="R6YrfPGC" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4764D2175B Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=S0+42FEmD2dYeMOE5kimzrpo8ipcSSHadnpucJVb6bc=; b=R6YrfPGCJ3b0Kw Soglwci22M8FsU852C20azZitrQr40nYIenzkY8cCynFKYfX7jCJcFmmrwqdhKOpwtPh0bChYGskv BdjiwvTG8HVMNubn1+r1cJsg2OFjEcZ73XcaB00m2AX4WoJfhBRBmKNfK5zEv/3bQ53sDBLvzIaii 5Lr9LIk8Z7CJ9INx8biRKbfMCKVSfMEhhRj7S48f7DVqLXOg0nZrw6gyPMfLQ1oIL6xrTdd6bvTE/ J/gSYGVxFIC2286495QFDjsRFxO5dU0bppXk03nWJoXBBjXKoYcIWJYgBjAHo95O00Igzu61l5tOJ 278h/aSWHz023AULwbBA==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1hJFbo-0004zX-2k; Wed, 24 Apr 2019 10:58:36 +0000 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70] helo=foss.arm.com) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1hJFbk-0004z5-0n for linux-arm-kernel@lists.infradead.org; Wed, 24 Apr 2019 10:58:33 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id BEC63A78; Wed, 24 Apr 2019 03:58:29 -0700 (PDT) Received: from fuggles.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 91E673F5AF; Wed, 24 Apr 2019 03:58:27 -0700 (PDT) Date: Wed, 24 Apr 2019 11:58:22 +0100 From: Will Deacon To: Frank Li Subject: Re: [PATCH V5 2/4] drivers/perf: imx_ddr: Add ddr performance counter support Message-ID: <20190424105822.GA14614@fuggles.cambridge.arm.com> References: <1555447156-28306-1-git-send-email-Frank.Li@nxp.com> <1555447156-28306-2-git-send-email-Frank.Li@nxp.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1555447156-28306-2-git-send-email-Frank.Li@nxp.com> User-Agent: Mutt/1.11.1+86 (6f28e57d73f2) () X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190424_035832_075123_2314BB66 X-CRM114-Status: GOOD ( 21.33 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "mark.rutland@arm.com" , Aisheng Dong , "devicetree@vger.kernel.org" , "shawnguo@kernel.org" , "s.hauer@pengutronix.de" , "robh+dt@kernel.org" , dl-linux-imx , "kernel@pengutronix.de" , "lznuaa@gmail.com" , "festevam@gmail.com" , "linux-arm-kernel@lists.infradead.org" Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel