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=-15.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,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 26A40C433E0 for ; Mon, 8 Mar 2021 11:26:49 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (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 8D7F665105 for ; Mon, 8 Mar 2021 11:26:48 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8D7F665105 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=desiato.20200630; h=Sender:Content-Transfer-Encoding :Content-Type:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:Message-ID: Subject:Cc: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=wch3qbbECTIzHEEZxr8ITpVgwLZKAJW+vVhr0tuopK0=; b=o7JwchR0u/y1OL554LcQHqp78 9xTzEA41/wCAa1Yi+EeHD4BlWDqnlkUxpzSI0RK+UiLt+KH0fIi+BvFGtUHFe3Lm38+YgpPoE0ya1 vilfXYhqPmKe5wSHfqKqC4XXmkI04fSVBVXVxcJ6bsWnmukW7/VRXjhXHgtWgR1lC6XnWnrFQfdop srHV/cAbfi3qtClGZtf31yVSfhtK97E3qVMs4AKelplSbJW3WU+/xmlIrKkdM/IId4cQRYms4eKUE KVyxZwD5iPQXmXCycv2gK7aokSv8Qnwg7oGAXSWegoo1/9U9i8GolF49dh1FsJyK1z0U4Bj3xcWWG IitZAjK+w==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lJE0i-00Gk0D-81; Mon, 08 Mar 2021 11:25:16 +0000 Received: from casper.infradead.org ([2001:8b0:10b:1236::1]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lJE0e-00GjzR-Kb for linux-arm-kernel@desiato.infradead.org; Mon, 08 Mar 2021 11:25:13 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=ccgsqP2EntlfkNLFrTAXTvEdqU1ZIXqqHbwjxNkOvzE=; b=ZWg+6ZjNW+NOAu4272WIWkTtyk 1P9AZqKo6KCwOnYiBj2bo0N67yP0ksPkJWquu1dT6W27++Y1Z8+fJvXwoTjOe5TdnJLYL7em/+pEV raYa0SeYggAHE6qpL2L0I8uoXShdFIDG23mYg+PDxSs85ZFmp/qL9+NZ1EEQOAi4uAXQNpd9VyIUY 3uQW47L7Np3NoXAATpAC2gOyWN27+NBfbTZzzJGxSPqO6EPh8nVuVyJ0ZONv2tF/gXrIZn6Q+Ch9K PLGu0t39K+XSX+0HpP/wUTzscfn2EvowAXKNCrjxhp8fRIVi838ktm1Y+DgilEjkuB9YGG2ztNgj2 T8FO6yog==; Received: from foss.arm.com ([217.140.110.172]) by casper.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lJE0I-00FOf1-1A for linux-arm-kernel@lists.infradead.org; Mon, 08 Mar 2021 11:25:02 +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 753A3D6E; Mon, 8 Mar 2021 03:24:38 -0800 (PST) Received: from e120937-lin (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 8D7A93F73C; Mon, 8 Mar 2021 03:24:36 -0800 (PST) Date: Mon, 8 Mar 2021 11:24:33 +0000 From: Cristian Marussi To: Sudeep Holla Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, lukasz.luba@arm.com, james.quinlan@broadcom.com, Jonathan.Cameron@Huawei.com, f.fainelli@gmail.com, etienne.carriere@linaro.org, thara.gopinath@linaro.org, vincent.guittot@linaro.org, souvik.chakravarty@arm.com Subject: Re: [PATCH v6 01/37] firmware: arm_scmi: review protocol registration interface Message-ID: <20210308112433.GC30179@e120937-lin> References: <20210202221555.41167-1-cristian.marussi@arm.com> <20210202221555.41167-2-cristian.marussi@arm.com> <20210308043830.ln7nryxgnr3f4opj@bogus> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20210308043830.ln7nryxgnr3f4opj@bogus> User-Agent: Mutt/1.9.4 (2018-02-28) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210308_112453_412183_ACEEFFDA X-CRM114-Status: GOOD ( 31.58 ) 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 Hi Sudeep, thanks for reviewing this. On Mon, Mar 08, 2021 at 04:38:30AM +0000, Sudeep Holla wrote: > On Tue, Feb 02, 2021 at 10:15:19PM +0000, Cristian Marussi wrote: > > Extend common protocol registration routines and provide some new generic > > protocols get/put helpers that can track protocols usage and automatically > > perform the proper initialization and de-initialization on demand when > > required. > > > > Convert all standard protocols to use this new registration scheme while > > keeping them all still using the usual initialization logic bound to SCMI > > devices probing. > > > > Signed-off-by: Cristian Marussi > > --- > > v2 --> v3 > > - removed new Base protocol initialization, it will be re-introduced > > later with all other protocols > > --- > > drivers/firmware/arm_scmi/base.c | 8 ++ > > drivers/firmware/arm_scmi/bus.c | 61 ++++++++--- > > drivers/firmware/arm_scmi/clock.c | 10 +- > > drivers/firmware/arm_scmi/common.h | 30 +++++- > > drivers/firmware/arm_scmi/driver.c | 159 +++++++++++++++++++++++++++- > > drivers/firmware/arm_scmi/perf.c | 10 +- > > drivers/firmware/arm_scmi/power.c | 10 +- > > drivers/firmware/arm_scmi/reset.c | 10 +- > > drivers/firmware/arm_scmi/sensors.c | 8 +- > > drivers/firmware/arm_scmi/system.c | 8 +- > > drivers/firmware/arm_scmi/voltage.c | 8 +- > > include/linux/scmi_protocol.h | 6 +- > > 12 files changed, 296 insertions(+), 32 deletions(-) > > > > diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c > > index 1377ec76a45d..044aa9e3ebb0 100644 > > --- a/drivers/firmware/arm_scmi/bus.c > > +++ b/drivers/firmware/arm_scmi/bus.c > > @@ -16,7 +16,7 @@ > > #include "common.h" > > > > static DEFINE_IDA(scmi_bus_id); > > -static DEFINE_IDR(scmi_protocols); > > +static DEFINE_IDR(scmi_available_protocols); > > [nit] Any particular reason for this name change ? Does it hold refer to > something different from before ? IIRC, this is list of registered protocols ? > Available might refer to the ones advertised to be present by the platform > firmware ? > Indeed I was not completely sure to change the naming, the only difference here is the protocol data that is kept in the IDR, which is now a struct scmi_protocol describing a bunch of things and not just a protocol_init function. It contains protocols currently loaded (if module), so registered with the core and ready to be initialized (on get_protocol) if implemented by the platform AND once an SCMI driver user appears, so available here does NOT imply being implemented by platform. In fact the meaning is really as before, it's just that (I suppose I thought :D) I wanted to stress the fact that these are the currently loaded protocols modules which are available and differentiate a bit the naming from the struct scmi_protocol which they hold (which really maybe does not make sense now reviewing it) I think I can just revert to use scmi_protocols. > > static DEFINE_SPINLOCK(protocol_lock); > > > > static const struct scmi_device_id * > > @@ -51,13 +51,29 @@ static int scmi_dev_match(struct device *dev, struct device_driver *drv) > > return 0; > > } > > > > +const struct scmi_protocol *scmi_get_protocol(int protocol_id) > > +{ > > + const struct scmi_protocol *proto; > > + > > + proto = idr_find(&scmi_available_protocols, protocol_id); > > + if (!proto) { > > + pr_warn("SCMI Protocol 0x%x not found!\n", protocol_id); > > + return NULL; > > + } > > + > > + pr_debug("GOT SCMI Protocol 0x%x\n", protocol_id); > > + > > [nit] For sake of consistency, s/GOT/Found/ > > Sure. > [..] > > > @@ -194,26 +210,45 @@ void scmi_set_handle(struct scmi_device *scmi_dev) > > scmi_dev->handle = scmi_handle_get(&scmi_dev->dev); > > } > > > > -int scmi_protocol_register(int protocol_id, scmi_prot_init_fn_t fn) > > +int scmi_protocol_register(const struct scmi_protocol *proto) > > { > > int ret; > > > > + if (!proto) { > > + pr_err("invalid protocol\n"); > > + return -EINVAL; > > + } > > + > > + if (!proto->init && !proto->init_instance) { > > + pr_err("missing .init() for protocol 0x%x\n", proto->id); > > > s/.init()/init as it can be init_instance too ? > Right, in fact also because this is part of the 'transitional' code and later on the series ("firmware: arm_scmi: cleanup legacy protocol init code") I remove the .init method and keep only the init_instance, so much better if I leave it generic as 'init' as you suggested. > > diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c > > index 4645677d86f1..e8c84cff9922 100644 > > --- a/drivers/firmware/arm_scmi/clock.c > > +++ b/drivers/firmware/arm_scmi/clock.c > > @@ -2,7 +2,7 @@ > > /* > > * System Control and Management Interface (SCMI) Clock Protocol > > * > > - * Copyright (C) 2018 ARM Ltd. > > + * Copyright (C) 2018-2020 ARM Ltd. > > 2021 perhaps ? Few instances are not updated, I prefer to be consistent > across all modified scmi firmware driver files. > Ah, sorry I missed this, I'll review all the Copyrights for 2021. > > Other than these minor comments, the other changes looks good to me. > Thanks Cristian > -- > Regards, > Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel