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=-11.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable 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 36EF9C388F7 for ; Thu, 5 Nov 2020 22:48:16 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (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 B4A84206CA for ; Thu, 5 Nov 2020 22:48:15 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="hDHKry9K" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B4A84206CA Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+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=merlin.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=8QQ5wpQg8NoroBbiGjbTJzs47YLhspLpxU+L1yoWNM8=; b=hDHKry9KrheKWE4F9Oegm2Dz3 LOW28KSQwJP+DcF36spyhvK2BhHJCUOYu2uNQPF5VZHfe7Vt272m6N7BTkgQStJAiiy36bXP0MqFH l3WZH+ubc4eMwIEFpWLD5vRsEq0+btwpsnw2icpCXRDFsPvtSSRRON4fecD3GixhuwkmfLST95DBB xaTyOqzoTX2Hgwn+ozVP9yY3hqxxATH4Gtds4p93eEVS6O3lkSwcSkOjtd4jc+13yhm9I0OCvEBsm hIbNwFrBI5zt1aSFTuH357KDsnZkjJEkAsZfnZUefM3DKPOFTagvN1E9B7JkoKffmkcY69Wi5B5/4 gwhdJ539w==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kao2m-0005EO-Es; Thu, 05 Nov 2020 22:47:48 +0000 Received: from foss.arm.com ([217.140.110.172]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kao2j-0005Dl-5j for linux-arm-kernel@lists.infradead.org; Thu, 05 Nov 2020 22:47:46 +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 208BD1474; Thu, 5 Nov 2020 14:47:43 -0800 (PST) Received: from [10.57.20.162] (unknown [10.57.20.162]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 11EA43F718; Thu, 5 Nov 2020 14:47:41 -0800 (PST) Subject: Re: [PATCH v3 14/26] coresight: etm4x: Add sysreg access helpers To: Mathieu Poirier References: <20201028220945.3826358-1-suzuki.poulose@arm.com> <20201028220945.3826358-16-suzuki.poulose@arm.com> <20201105205248.GA3047244@xps15> From: Suzuki K Poulose Message-ID: Date: Thu, 5 Nov 2020 22:47:41 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.4.0 MIME-Version: 1.0 In-Reply-To: <20201105205248.GA3047244@xps15> Content-Language: en-GB X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201105_174745_356537_6B44C966 X-CRM114-Status: GOOD ( 24.70 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: coresight@lists.linaro.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, mike.leach@linaro.org Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Mathieu, On 11/5/20 8:52 PM, Mathieu Poirier wrote: > On Wed, Oct 28, 2020 at 10:09:33PM +0000, Suzuki K Poulose wrote: >> ETMv4.4 architecture defines the system instructions for accessing >> ETM via register accesses. Add basic support for accessing a given >> register via system instructions. >> >> Cc: Mathieu Poirier >> Cc: Mike Leach >> Signed-off-by: Suzuki K Poulose >> --- >> .../coresight/coresight-etm4x-core.c | 39 ++ >> drivers/hwtracing/coresight/coresight-etm4x.h | 348 ++++++++++++++++-- >> 2 files changed, 365 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c >> index 4af7d45dfe63..90b80982c615 100644 >> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c >> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c >> @@ -56,6 +56,45 @@ static u64 etm4_get_access_type(struct etmv4_config *config); >> >> static enum cpuhp_state hp_online; >> >> +u64 etm4x_sysreg_read(struct csdev_access *csa, >> + u32 offset, >> + bool _relaxed, >> + bool _64bit) > > Please fix the stacking. > Sure. >> + >> +void etm4x_sysreg_write(struct csdev_access *csa, >> + u64 val, >> + u32 offset, >> + bool _relaxed, >> + bool _64bit) > > Here too. Sure. >> /* Writing 0 to TRCOSLAR unlocks the trace registers */ >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h >> index 510828c73db6..5cf71b30a652 100644 >> --- a/drivers/hwtracing/coresight/coresight-etm4x.h >> +++ b/drivers/hwtracing/coresight/coresight-etm4x. >> + >> +#define ETM4x_READ_CASES(res) CASE_LIST(READ, (res)) >> +#define ETM4x_WRITE_CASES(val) CASE_LIST(WRITE, (val)) >> + >> +#define read_etm4x_sysreg_offset(csa, offset, _64bit) \ >> + ({ \ >> + u64 __val; \ >> + \ >> + if (__builtin_constant_p((offset))) \ > > Neat trick - I wonder how you stumbled on that one. > :-). There are plenty of uses in the kernel and glibc. > >> + __val = read_etm4x_sysreg_const_offset((offset)); \ >> + else \ >> + __val = etm4x_sysreg_read((csa), (offset), \ >> + true, _64bit); \ >> + __val; \ >> + }) >> + >> +#define write_etm4x_sysreg_offset(csa, val, offset, _64bit) \ >> + do { \ >> + if (__builtin_constant_p((offset))) \ >> + write_etm4x_sysreg_const_offset((val), \ >> + (offset)); \ >> + else \ >> + etm4x_sysreg_write((csa), (val), (offset), \ >> + true, _64bit); \ >> + } while (0) >> + >> + >> +#define etm4x_relaxed_read32(csa, offset) \ >> + ((u32)((csa)->io_mem ? \ >> + readl_relaxed((csa)->base + (offset)) : \ >> + read_etm4x_sysreg_offset((csa), (offset), false))) > > Please add an extra new line - otherwise it is very hard to read. > Sure >> +#define etm4x_relaxed_read64(csa, offset) \ >> + ((u64)((csa)->io_mem ? \ >> + readq_relaxed((csa)->base + (offset)) : \ >> + read_etm4x_sysreg_offset((csa), (offset), true))) > > Here too. > sure >> +#define etm4x_read32(csa, offset) \ >> + ({ \ >> + u32 __val = etm4x_relaxed_read32((csa), (offset)); \ >> + __iormb(__val); \ >> + __val; \ >> + }) >> + >> +#define etm4x_read64(csa, offset) \ >> + ({ \ >> + u64 __val = etm4x_relaxed_read64((csa), (offset)); \ >> + __iormb(__val); \ >> + __val; \ >> + }) >> + >> +#define etm4x_relaxed_write32(csa, val, offset) \ >> + do { \ >> + if ((csa)->io_mem) \ >> + writel_relaxed((val), (csa)->base + (offset)); \ >> + else \ >> + write_etm4x_sysreg_offset((csa), (val), \ >> + (offset), false); \ > > Why using an if/else statement and above the '?' condition marker? I would > really like a uniform approach. Otherwise the reader keeps looking for > something subtle when there isn't. The write variants do not return a result, unlike the read. So, we cant use the '?' > >> + } while (0) >> + >> +#define etm4x_relaxed_write64(csa, val, offset) \ >> + do { \ >> + if ((csa)->io_mem) \ >> + writeq_relaxed((val), (csa)->base + (offset)); \ >> + else \ >> + write_etm4x_sysreg_offset((csa), (val), \ >> + (offset), true); \ >> + } while (0) >> + >> +#define etm4x_write32(csa, val, offset) \ >> + do { \ >> + __iowmb(); \ >> + etm4x_relaxed_write32((csa), (val), (offset)); \ >> + } while (0) >> + >> +#define etm4x_write64(csa, val, offset) \ >> + do { \ >> + __iowmb(); \ >> + etm4x_relaxed_write64((csa), (val), (offset)); \ >> + } while (0) >> >> -#define etm4x_write64(csa, val, offset) \ >> - writeq((val), (csa)->base + (offset)) >> >> /* ETMv4 resources */ >> #define ETM_MAX_NR_PE 8 >> @@ -512,4 +806,14 @@ enum etm_addr_ctxtype { >> >> extern const struct attribute_group *coresight_etmv4_groups[]; >> void etm4_config_trace_mode(struct etmv4_config *config); >> + >> +u64 etm4x_sysreg_read(struct csdev_access *csa, >> + u32 offset, >> + bool _relaxed, >> + bool _64bit); >> +void etm4x_sysreg_write(struct csdev_access *csa, >> + u64 val, >> + u32 offset, >> + bool _relaxed, >> + bool _64bit); > > With the above: > > Reviewed-by: Mathieu Poirier Thanks ! > > This patch holds together well. I commend you on rendering something that is > quite complex into a manageable implementation. That being said it will impact > Mike's complex configuration patchset (or Mike's complex configuration patchset > will have an impact on this). I understand. Will see when we get to it. Cheers Suzuki _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel