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=-9.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,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 47707C433E0 for ; Thu, 30 Jul 2020 14:55:51 +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 10A562070B for ; Thu, 30 Jul 2020 14:55:51 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="hZ6qgE/J" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 10A562070B 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+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=0WYERMGEtkL0SCcxr6kel5x89WUUEtmpEFRvC6kX18o=; b=hZ6qgE/Jzp2Nb9UHPSkHkQSLr IIxE0nLtTsNf1Z8BiaUySV1WRd1b4Zpc/ksqQenp6xlha+S+6PvdKCl8YJZMx7hpWONew9F8rm4i0 pVmVTNSToiqic65mPTeMy+4ssu0mJVIB0bHtsn5tX5n0wObYSjOaOMpRRauiNaNV8unkckZYu/w5s y1JCOolXnn2SyXUcqSE1kSDKPC0mrLBOQLePXxumXwPYlqH0FLjny2W8kMJWEXBm61Hhv2YYd7Pwi OMLoL8+wyK2kdXylqd4IjjiwnZYrpwzUHBxO2kr8+DhZa5JZQcwjdzs6jAAejyItFKikbAK3BlzBY pblCdLL8A==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1k19wh-0007Gu-RO; Thu, 30 Jul 2020 14:54:11 +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 1k19we-0007Fe-6b for linux-arm-kernel@lists.infradead.org; Thu, 30 Jul 2020 14:54:09 +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 AF1841FB; Thu, 30 Jul 2020 07:54:04 -0700 (PDT) Received: from [10.37.12.83] (unknown [10.37.12.83]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 339BF3F66E; Thu, 30 Jul 2020 07:54:03 -0700 (PDT) Subject: Re: [RFC PATCH 02/14] coresight: Introduce device access abstraction To: mathieu.poirier@linaro.org References: <20200722172040.1299289-1-suzuki.poulose@arm.com> <20200722172040.1299289-3-suzuki.poulose@arm.com> <20200729195617.GB3073178@xps15> From: Suzuki K Poulose Message-ID: Date: Thu, 30 Jul 2020 15:58:48 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20200729195617.GB3073178@xps15> Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200730_105408_383644_39115348 X-CRM114-Status: GOOD ( 29.06 ) 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 On 07/29/2020 08:56 PM, Mathieu Poirier wrote: > On Wed, Jul 22, 2020 at 06:20:28PM +0100, Suzuki K Poulose wrote: >> We are about to introduce support for sysreg access to ETMv4.4+ >> component. Since there are generic routines that access the >> registers (e.g, CS_LOCK/UNLOCK , claim/disclaim operations, timeout) >> and in order to preserve the logic of these operations at a single place >> we introduce an abstraction layer for the accesses to a given device. >> This will also be helpful in consolidating the sysfs.attribute helpers, >> that we define per driver. > > Please drop the last sentence, it doesn't add to the current patch. > Sure. >> diff --git a/drivers/hwtracing/coresight/coresight.c b/drivers/hwtracing/coresight/coresight.c >> index e9c90f2de34a..38e9c03ab754 100644 >> --- a/drivers/hwtracing/coresight/coresight.c >> +++ b/drivers/hwtracing/coresight/coresight.c >> @@ -1387,6 +1387,54 @@ static int __init coresight_init(void) >> } ... >> * coresight_release_platform_data: Release references to the devices connected >> * to the output port of this device. >> @@ -1451,6 +1499,7 @@ struct coresight_device *coresight_register(struct coresight_desc *desc) >> csdev->type = desc->type; >> csdev->subtype = desc->subtype; >> csdev->ops = desc->ops; >> + csdev->access = desc->access; >> csdev->orphan = false; >> >> csdev->dev.type = &coresight_dev_type[desc->type]; >> diff --git a/include/linux/coresight.h b/include/linux/coresight.h >> index 58fffdecdbfd..81ac708689f8 100644 >> --- a/include/linux/coresight.h >> +++ b/include/linux/coresight.h >> @@ -7,6 +7,7 @@ >> #define _LINUX_CORESIGHT_H >> >> #include >> +#include >> #include >> #include >> >> @@ -114,6 +115,32 @@ struct coresight_platform_data { >> struct coresight_connection *conns; >> }; >> >> +/** >> + * struct csdev_access - Abstraction of a CoreSight device access. >> + * >> + * @no_iomem : True if the device doesn't have iomem access. >> + * @base : When no_iomem == false, base address of the component >> + * @read : Read from the given "offset" of the given instance. >> + * @write : Write "val" to the given "offset". >> + */ >> +struct csdev_access { >> + bool no_iomem; > > I find the no_iomen to be difficult to understand, especially when prefixed with > '!'. Using "has_iomem" would be a lot more intuitive and would avoid extra > mental gymnastics. I agree. That was a bit of laziness in part, to limit the changes to the existing drivers, where almost everyone, except the ETM would need to simply use the MMIO approach. So, in order to keep those changes to minimum, i.e, simply initialize the base, I used the inverted logic. I will fix it. >> +u32 coresight_relaxed_read32(struct coresight_device *csdev, u32 offset); >> +u32 coresight_read32(struct coresight_device *csdev, u32 offset); >> +void coresight_write32(struct coresight_device *csdev, u32 val, u32 offset); >> +void coresight_relaxed_write32(struct coresight_device *csdev, >> + u32 val, >> + u32 offset); >> + > > Why are the 64 bit version outside of the #ifdef and the 32 bit within? Mistake ;-). I will address all of the comments above in my next version. ... >> +static inline u64 coresight_read64(struct coresight_device *csdev, u32 offset) >> +{ >> + WARN_ON_ONCE(1); > > Not sure about the motivation behind using WARN_ON_ONCE(), and only in the read > functions. I would simply return 0 here. After all if CONFIG_CORESIGHT is not > defined they won't make it very far. > If someone is reading the values, they might do something with the value, i.e, make a decision when they shouldn't. This is just to prevent such cases. >> + return 0; >> +} >> + >> +static inline void coresight_relaxed_write64(struct coresight_device *csdev, >> + u64 val, >> + u32 offset) >> +{ >> +} >> + >> +static inline void coresight_write64(struct coresight_device *csdev, u64 val, u32 offset) >> +{ >> +} >> + >> #endif > > I will likely come back to this patch once I have reviewed the rest of the set. > Sure. I am looking for thoughts on the proposed API (not ABI) changes, as they are quite significant and invasive changes in the code, without much functionality changes. Thank you for the review ! Suzuki _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel