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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 54A6DC433FE for ; Fri, 30 Sep 2022 04:08:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To: Subject:MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=DmoY11Y/o3mFCuMWpXd+NNNfvLIyWTgjQjxF7QimxIg=; b=ryi7qNd1/XS9dO pG7Gk/TfaoC9jfO35BXCPrrrHdVrCeAaGGO184pmnzaFm9fp4ZO5bxfwdO4AyB6yk+Vjs+I1Re7DQ QgiUrwG4zUcOrI1UcEeZ+fzzefWEDl2Bu+EWzuWWEkywIb/VmB6bX2tgUveszuX4Btsyd2hrSMkff +wGMk7hKxuVycqQqk2ifbkEWMT7gleFADI+i9SesBlDDfl9URiS6BBnKHH80YxMREC6s/BRrpKZP9 qrTjlV3e8ryPfQiCaam9Uv+ubFfninE1vhi5xJ7k2vS8k0TNC2r//PZdSDzE37YlVsJrXFQnGKjhK Qq9U+BcvtLCGj1xODx4w==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oe7J8-0071MM-EX; Fri, 30 Sep 2022 04:07:26 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oe7J4-0071Jw-30 for linux-arm-kernel@lists.infradead.org; Fri, 30 Sep 2022 04:07:24 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id D5DA615A1; Thu, 29 Sep 2022 21:07:17 -0700 (PDT) Received: from [10.162.41.10] (unknown [10.162.41.10]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id D51FB3F73D; Thu, 29 Sep 2022 21:07:07 -0700 (PDT) Message-ID: <8751bc38-9020-ba5f-d2d6-dd7486e1709a@arm.com> Date: Fri, 30 Sep 2022 09:37:04 +0530 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH V3 1/7] arm64/perf: Add BRBE registers and fields Content-Language: en-US To: Mark Brown Cc: linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org, linux-arm-kernel@lists.infradead.org, peterz@infradead.org, acme@kernel.org, mark.rutland@arm.com, will@kernel.org, catalin.marinas@arm.com, Marc Zyngier References: <20220929075857.158358-1-anshuman.khandual@arm.com> <20220929075857.158358-2-anshuman.khandual@arm.com> From: Anshuman Khandual In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220929_210722_254470_AE399E12 X-CRM114-Status: GOOD ( 25.32 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 9/29/22 16:59, Mark Brown wrote: > On Thu, Sep 29, 2022 at 01:28:51PM +0530, Anshuman Khandual wrote: > > Thanks for doing this work - I did spot a few small issues though. > >> /* id_aa64dfr0 */ >> +#define ID_AA64DFR0_BRBE_SHIFT 52 >> #define ID_AA64DFR0_MTPMU_SHIFT 48 >> #define ID_AA64DFR0_TRBE_SHIFT 44 >> #define ID_AA64DFR0_TRACE_FILT_SHIFT 40 >> @@ -848,6 +952,9 @@ >> #define ID_AA64DFR0_PMSVER_8_2 0x1 >> #define ID_AA64DFR0_PMSVER_8_3 0x2 >> >> +#define ID_AA64DFR0_BRBE 0x1 >> +#define ID_AA64DFR0_BRBE_V1P1 0x2 >> + >> #define ID_DFR0_PERFMON_SHIFT 24 >> >> #define ID_DFR0_PERFMON_8_0 0x3 > > This is already done in -next as a result of ID_AA64DFR0_EL1 being > converted, the enumberation define comes out as > ID_AA64DFR0_EL1_BRBE_BRBE_V1P1. Right. I will rebase the series on upcoming 6.1-rc1 which should have all the current -next patches including ID_AA64DFR0_EL1 migration into tools/sysreg. This should just fall in place. > >> +# This is just a dummy register declaration to get all common field masks and >> +# shifts for accessing given BRBINF contents. >> +Sysreg BRBINF_EL1 2 1 8 0 0 >> +Res0 63:47 >> +Field 46 CCU >> +Field 45:32 CC >> +Res0 31:18 >> +Field 17 LASTFAILED >> +Field 16 TX > > According to DDI0487I.a bit 16 is called T not TX. I understand :) The intention here was to keep the field name associated with "transaction" some how. But I guess, it would be more important to keep it as is matching the ARM ARM than something for readability purpose. Will change it as 'T'. > >> +Res0 15:14 >> +Enum 13:8 TYPE > > It's probably worth noting in the comment the issue with Enums here > that's meaning you're not using a SysregFields - I'm not sure what Sure, will add a comment describing the problem of using enum elements inside SysregFields definition. > people will think of this but providing a definition using the ID for > the 0th register does seem expedient. I understand your concern but this turned out to be a better option - Original sysreg.h based definitions had all field masks on the right end - When reading (reg >> field_shift) & field_mask - When writing (val & field_mask) << field_shift - tools/sysreg format creates in-place field masks - When reading (reg & field_mask) >> field_shift - When writing (val << field_shift) & field_mask - After moving some BRBE registers into tools/sysreg, the driver code had to be changed to accommodate these new write/read methods - To avoid mix up in the BRBE driver, all BRBINF register fields need to be converted into in place masks, either in sysreg.h itself or moving into tools/sysreg Moving BRBE fields into tools/sysreg via a dummy BRBINF_EL1 register seems to achieve that objective. These common fields can be used to work on any BRBINF_EL1 register value. But I might just keep them in sysreg.h, if the proposed solution is not preferable or seems expedient. Later when enum support comes up in SysregFields and tools/sysreg supports formula based crm/op2 expansion entire BRBINF, BRBSRC, BRBTGT register set can be moved into tools/sysreg. > >> +Enum 7:6 EL >> + 0b00 EL0 >> + 0b01 EL1 >> + 0b10 EL2 >> +EndEnum > > According to DDI0487I.a 0b11 has the value EL3 (when FEAT_BRBEv1p1). Sure, will add it. > >> +Sysreg BRBCR_EL1 2 1 9 0 0 >> +Res0 63:24 >> +Field 23 EXCEPTION >> +Field 22 ERTN >> +Res0 21:9 >> +Field 8 FZP >> +Field 7 ZERO > > According to DDI0487I.a bit 7 is Res0. Sure, will change. > >> +Field 2 ZERO1 > > According to DDI0487I.a bit 2 is Res0. Sure, will change. > >> +Sysreg BRBFCR_EL1 2 1 9 0 1 > >> +Field 16 ENL > > Accoding to DDI0487I.a this is EnI (ie, an L not an I). ENL != Enl ? Do we need to match the case as well ? > >> +Sysreg BRBINFINJ_EL1 2 1 9 1 0 > >> +Field 16 TX > > According to DDI0487I.a this is T not TX. As mentioned, will change it as 'T'. > >> +Enum 7:6 EL >> + 0b00 EL0 >> + 0b01 EL1 >> + 0b10 EL2 >> +EndEnum > > According to DDI0487I.a 0b11 has the value EL3 (when FEAT_BRBEv1p1). Sure, will add. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel