From mboxrd@z Thu Jan 1 00:00:00 1970 From: Will Deacon Subject: Re: [PATCH V4 2/4] drivers/perf: imx_ddr: Add ddr performance counter support Date: Tue, 16 Apr 2019 14:51:16 +0100 Message-ID: <20190416135116.GC3313@fuggles.cambridge.arm.com> References: <1550253761-26841-1-git-send-email-Frank.Li@nxp.com> <1550253761-26841-2-git-send-email-Frank.Li@nxp.com> <20190404111714.GA26392@fuggles.cambridge.arm.com> <20190405143846.GB7313@fuggles.cambridge.arm.com> <20190405150319.GA7704@fuggles.cambridge.arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Zhi Li Cc: "mark.rutland@arm.com" , Aisheng Dong , "devicetree@vger.kernel.org" , "festevam@gmail.com" , "s.hauer@pengutronix.de" , Frank Li , "robh+dt@kernel.org" , dl-linux-imx , "kernel@pengutronix.de" , "shawnguo@kernel.org" , "linux-arm-kernel@lists.infradead.org" List-Id: devicetree@vger.kernel.org On Mon, Apr 08, 2019 at 02:10:28PM -0500, Zhi Li wrote: > On Fri, Apr 5, 2019 at 10:03 AM Will Deacon wrote: > > > > On Fri, Apr 05, 2019 at 09:58:38AM -0500, Zhi Li wrote: > > > On Fri, Apr 5, 2019 at 9:38 AM Will Deacon wrote: > > > > On Fri, Apr 05, 2019 at 09:34:38AM -0500, Zhi Li wrote: > > > > > On Thu, Apr 4, 2019 at 6:17 AM Will Deacon wrote: > > > > > > On Fri, Feb 15, 2019 at 06:03:11PM +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. > > > > > > > > > > > > > > for example: > > > > > > > > > > > > > > perf stat -a -e ddr0/read-access/,ddr0/write-access/,ddr0/precharge/ ls > > > > > > > perf stat -a -e ddr0/cycles/,ddr0/read-access/,ddr0/write-access/ ls > > > > > > > > > > > > Could you elaborate a bit on DDR_CAP_AXI_ID, please? Specifically, how > > > > > > Only imx845 have AXID filter capability now. > > > > > > > > > does the COUNTER_DPCR1 register work and what happens if I specify two > > > > > > simultaneous events with different values in config1? I'm a little wary > > > > > > There are difference match register for each event. > > > 1. Read event with config 1 A > > > 2. Read event with config 1 B > > > > > > 1 will show read count with filter A > > > 2 will show read count with filter B. > > > > Thanks, that makes sense, but I can't see how that corresponds to the code > > in the patch: > > > > +static void ddr_perf_event_start(struct perf_event *event, int flags) > > +{ > > + struct ddr_pmu *pmu = to_ddr_pmu(event->pmu); > > + struct hw_perf_event *hwc = &event->hw; > > + int counter = hwc->idx; > > + > > + if (pmu->flags & DDR_CAP_AXI_ID) { > > + if (event->attr.config == EVENT_AXI_READ || > > + event->attr.config == EVENT_AXI_WRITE) { > > + int val = event->attr.config1; > > + > > + writel(val, pmu->base + COUNTER_DPCR1); > > + } > > + } > > + > > + local64_set(&hwc->prev_count, 0); > > + > > + ddr_perf_event_enable(pmu, event->attr.config, counter, true); > > > > ddr_perf_event_enable() does what you'd expect, and uses hwc->idx to > > allocate a counter, but the code before it just always writes to DPCR1. > > > > What am I missing? > > I double check with our IC design team. My previous answer is wrong. > Only one DPCR1. It will impact all event EVENT_AXI_READ and EVENT_AXI_WRITE. > > Thank means just choose one config1 for event EVENT_AXI_READ or EVENT_AXI_WRITE I think you need either to enforce that in the driver, or drop this filtering capability for now. We shouldn't just tell userspace "oh yeah, don't do that." Given that your patch 3 (which updates a .dts file) doesn't appear to support the filtering, I suggest just dropping that support initially so that we can queue the rest of this driver for 5.2. Will 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 C0F22C10F13 for ; Tue, 16 Apr 2019 13:52:09 +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 8B5E2222B2 for ; Tue, 16 Apr 2019 13:52:09 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="L6jiNM+s" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8B5E2222B2 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=YcBWkPrZVXFhv08BLWHRbG2/rEDL4O7H7gCtFIAYSwE=; b=L6jiNM+sQTXlZA GR9I0FW22QCB393uMISMg4PxZ1SqaWQJy65WbCfE/7o7MQWy1hgLaNP1sbPXlq+Tl6d+C+gdfsZJC ewXuH/1plAAyg/n7yHraFf44qcUOgoLLDHbfFipXPIGJaDxW/uqhBY8vE8CM75LupQyQlM5Klbd21 S+dnbVKNv2mvBxQ+KibcYYzJ7xn4n0uPABxliYPzLVzuHHsADLWMTtQ5dBUwR7jlN5s1TF6LiemMr H2RMUoZwPYwFzXg86JT+fblwlIUx4dYrS+ZjMgHNw0GL2KqJzD3KS/a/+IjxHh8tykUUZ8pwbPoKc FHO3Xqmy3RfgwBfM5xlA==; 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 1hGOVI-0008MQ-QF; Tue, 16 Apr 2019 13:52:04 +0000 Received: from foss.arm.com ([217.140.101.70]) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1hGOUb-0007n8-GV for linux-arm-kernel@lists.infradead.org; Tue, 16 Apr 2019 13:51:23 +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 06652EBD; Tue, 16 Apr 2019 06:51:21 -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 E6D223F59C; Tue, 16 Apr 2019 06:51:18 -0700 (PDT) Date: Tue, 16 Apr 2019 14:51:16 +0100 From: Will Deacon To: Zhi Li Subject: Re: [PATCH V4 2/4] drivers/perf: imx_ddr: Add ddr performance counter support Message-ID: <20190416135116.GC3313@fuggles.cambridge.arm.com> References: <1550253761-26841-1-git-send-email-Frank.Li@nxp.com> <1550253761-26841-2-git-send-email-Frank.Li@nxp.com> <20190404111714.GA26392@fuggles.cambridge.arm.com> <20190405143846.GB7313@fuggles.cambridge.arm.com> <20190405150319.GA7704@fuggles.cambridge.arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.11.1+86 (6f28e57d73f2) () X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190416_065121_629654_C97DBE12 X-CRM114-Status: GOOD ( 23.26 ) 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" , "festevam@gmail.com" , "s.hauer@pengutronix.de" , Frank Li , "robh+dt@kernel.org" , dl-linux-imx , "kernel@pengutronix.de" , "shawnguo@kernel.org" , "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 On Mon, Apr 08, 2019 at 02:10:28PM -0500, Zhi Li wrote: > On Fri, Apr 5, 2019 at 10:03 AM Will Deacon wrote: > > > > On Fri, Apr 05, 2019 at 09:58:38AM -0500, Zhi Li wrote: > > > On Fri, Apr 5, 2019 at 9:38 AM Will Deacon wrote: > > > > On Fri, Apr 05, 2019 at 09:34:38AM -0500, Zhi Li wrote: > > > > > On Thu, Apr 4, 2019 at 6:17 AM Will Deacon wrote: > > > > > > On Fri, Feb 15, 2019 at 06:03:11PM +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. > > > > > > > > > > > > > > for example: > > > > > > > > > > > > > > perf stat -a -e ddr0/read-access/,ddr0/write-access/,ddr0/precharge/ ls > > > > > > > perf stat -a -e ddr0/cycles/,ddr0/read-access/,ddr0/write-access/ ls > > > > > > > > > > > > Could you elaborate a bit on DDR_CAP_AXI_ID, please? Specifically, how > > > > > > Only imx845 have AXID filter capability now. > > > > > > > > > does the COUNTER_DPCR1 register work and what happens if I specify two > > > > > > simultaneous events with different values in config1? I'm a little wary > > > > > > There are difference match register for each event. > > > 1. Read event with config 1 A > > > 2. Read event with config 1 B > > > > > > 1 will show read count with filter A > > > 2 will show read count with filter B. > > > > Thanks, that makes sense, but I can't see how that corresponds to the code > > in the patch: > > > > +static void ddr_perf_event_start(struct perf_event *event, int flags) > > +{ > > + struct ddr_pmu *pmu = to_ddr_pmu(event->pmu); > > + struct hw_perf_event *hwc = &event->hw; > > + int counter = hwc->idx; > > + > > + if (pmu->flags & DDR_CAP_AXI_ID) { > > + if (event->attr.config == EVENT_AXI_READ || > > + event->attr.config == EVENT_AXI_WRITE) { > > + int val = event->attr.config1; > > + > > + writel(val, pmu->base + COUNTER_DPCR1); > > + } > > + } > > + > > + local64_set(&hwc->prev_count, 0); > > + > > + ddr_perf_event_enable(pmu, event->attr.config, counter, true); > > > > ddr_perf_event_enable() does what you'd expect, and uses hwc->idx to > > allocate a counter, but the code before it just always writes to DPCR1. > > > > What am I missing? > > I double check with our IC design team. My previous answer is wrong. > Only one DPCR1. It will impact all event EVENT_AXI_READ and EVENT_AXI_WRITE. > > Thank means just choose one config1 for event EVENT_AXI_READ or EVENT_AXI_WRITE I think you need either to enforce that in the driver, or drop this filtering capability for now. We shouldn't just tell userspace "oh yeah, don't do that." Given that your patch 3 (which updates a .dts file) doesn't appear to support the filtering, I suggest just dropping that support initially so that we can queue the rest of this driver for 5.2. Will _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel