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 1A1A2C2D0A3 for ; Wed, 4 Nov 2020 16:17:47 +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 909292064C for ; Wed, 4 Nov 2020 16:17:46 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="r3ImEdy3"; dkim=fail reason="signature verification failed" (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="IK9xHdd6" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 909292064C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org 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:References: To:Subject:From:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=gN9mMlq7CkccwQU5lbA7g34AR7Xq+z8bG4DanEvlbO8=; b=r3ImEdy3y4akRgetNrJOl8hLX nEpOHdwpIdZ81AeiVSAdO8BZmZNNFPK30n01CfQZ0XiBDr0uRgl4jPGAVRtM3EX3WW0ZoDvBwQW8Q GjVZ7+KDVIIIdNKy7uPJKgMn6jhp0Zz698tdbdxTONsQ06Ovs+qPjzguWeFpemnzRX9avSIrmQGy8 gOoKR+X4OBkKNIbVvq/CpFdDxm3S0DA6sszsLFhvWq8Igjg02W1bQtCoHcCiemQ9JBG/HSeHMUZUj A/dqlRgzwkWPUH+HBf4o+adjvbbF7X7K88bD+xb2iThPtpOX7td7/nwkSNEVwQYcJwuZnmg6qddSp 19xA70c7w==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1kaLSS-0001tU-OZ; Wed, 04 Nov 2020 16:16:24 +0000 Received: from mail-qk1-x742.google.com ([2607:f8b0:4864:20::742]) by merlin.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1kaLSP-0001sP-Cy for linux-arm-kernel@lists.infradead.org; Wed, 04 Nov 2020 16:16:22 +0000 Received: by mail-qk1-x742.google.com with SMTP id 11so196949qkd.5 for ; Wed, 04 Nov 2020 08:16:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:subject:to:cc:references:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=sMu+ZpDCHnLpFB+Y0yMO7W+S8qBAHVafomO84Zlx6mo=; b=IK9xHdd6O4nkuBtJTTZmy6o8WBo6b9FleeSn2vxgN12HMyDE1v8bh4HevoOFWoffUf gi0BoPVWVmIr+uJ5gKgG4dxlhVtBys3XDchQQ9abMJuNQz5hlLH7VFax85+EpttrnOl/ 3NYz9+leDlSrHCbCoqgCVD27QSDLXkVz5v0C74yh9P3Tt8xlwievCzKI5CqAlmMZcDBK DU533CP+ZGDW8SB6us7Dx/oz+b7un90YpPDInbCPthvQPeoWKiWPQ7w1O+kXUPs2ZXip kLvxSLtpped6OnMZ9vcnyJnPPHT7qErq2WaV7EpfeV4J8dF6UqUvtPBG9wQcS323alUY jifg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:subject:to:cc:references:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=sMu+ZpDCHnLpFB+Y0yMO7W+S8qBAHVafomO84Zlx6mo=; b=tWdZGMnpi4ZIw+kfJJDYmnO3N5E6H3QVVPpNXKt4C3DCRF4kWZ3YoU+R/vwyYZpRmW mfO4HbidLJ2BJus+R2J3PXTijmet8CIIQ7s0tbOSG5nVMUb6kqZyqTXpDpgqrsKYmBzn Ut/CwNcZpmFa9uVhMfFDAj9XYNc/U0N32RL/+tJhbA1b73splSpvYYJgMDKE57DsK1Oy k1K4rMC46/ExSdUZraOWngZSDLdhyoVF2zsSNY3R3qFTWDiplDs2lfC2kFJtKuZ4PYE0 fnYwB59c9MrL+GqceQJvpHJGltuvm9Y/vTbR3ZfT2CGmSLEpQj2bosUTx+UQQyWaG5/P X1CA== X-Gm-Message-State: AOAM532L8i5c3ObwyNpsF2VOT5GcTNx89QJrVrdpi2uf5xIT+21t6+yM 1gMeYc5cM4dyqkOSQSHJuXmhNw== X-Google-Smtp-Source: ABdhPJyuzbHEtB930xHAVmCShKKD/e839v53ULcRdvC8I2Wcp4YPrp4GA/MdLnzQfnbPHKQZTxhBGA== X-Received: by 2002:ae9:e41a:: with SMTP id q26mr17296299qkc.246.1604506579307; Wed, 04 Nov 2020 08:16:19 -0800 (PST) Received: from [192.168.1.93] (pool-71-163-245-5.washdc.fios.verizon.net. [71.163.245.5]) by smtp.gmail.com with ESMTPSA id 185sm2745672qke.16.2020.11.04.08.16.18 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 04 Nov 2020 08:16:18 -0800 (PST) From: Thara Gopinath Subject: Re: [PATCH v2 2/8] firmware: arm_scmi: introduce protocol handles To: Cristian Marussi , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org References: <20201028202914.43662-1-cristian.marussi@arm.com> <20201028202914.43662-3-cristian.marussi@arm.com> Message-ID: Date: Wed, 4 Nov 2020 11:16:18 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20201028202914.43662-3-cristian.marussi@arm.com> Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20201104_111621_473374_8DB5CA19 X-CRM114-Status: GOOD ( 36.04 ) 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: f.fainelli@gmail.com, vincent.guittot@linaro.org, sudeep.holla@arm.com, james.quinlan@broadcom.com, Jonathan.Cameron@Huawei.com, souvik.chakravarty@arm.com, etienne.carriere@linaro.org, lukasz.luba@arm.com 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 Cristian, On 10/28/20 4:29 PM, Cristian Marussi wrote: > Add basic protocol handles definitions and helpers support. > All protocols initialization code and SCMI drivers probing is still > performed using the handle based interface. > > Signed-off-by: Cristian Marussi > --- > drivers/firmware/arm_scmi/common.h | 61 ++++++++++++++++++++++++++++ > drivers/firmware/arm_scmi/driver.c | 64 ++++++++++++++++++++++++++++++ > 2 files changed, 125 insertions(+) > > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h > index b08a8ddbc22a..f0678be02a09 100644 > --- a/drivers/firmware/arm_scmi/common.h > +++ b/drivers/firmware/arm_scmi/common.h > @@ -151,6 +151,67 @@ int scmi_xfer_get_init(const struct scmi_handle *h, u8 msg_id, u8 prot_id, > size_t tx_size, size_t rx_size, struct scmi_xfer **p); > void scmi_reset_rx_to_maxsz(const struct scmi_handle *handle, > struct scmi_xfer *xfer); > + > +struct scmi_xfer_ops; > + > +/** > + * struct scmi_protocol_handle - Reference to an initialized protocol instance > + * > + * @dev: A reference to the associated SCMI instance device (handle->dev). > + * @xops: A reference to a struct holding refs to the core xfer operations that > + * can be used by the protocol implementation to generate SCMI messages. > + * @set_priv: A method to set protocol private data for this instance. > + * @get_priv: A method to get protocol private data previously set. > + * > + * This structure represents a protocol initialized against specific SCMI > + * instance and it will be used as follows: > + * - as a parameter fed from the core to the protocol initialization code so > + * that it can access the core xfer operations to build and generate SCMI > + * messages exclusively for the specific underlying protocol instance. > + * - as an opaque handle fed by an SCMI driver user when it tries to access > + * this protocol through its own protocol operations. > + * In this case this handle will be returned as an opaque object together > + * with the related protocol operations when the SCMI driver tries to access > + * the protocol. > + */ > +struct scmi_protocol_handle { > + struct device *dev; > + const struct scmi_xfer_ops *xops; > + int (*set_priv)(const struct scmi_protocol_handle *ph, void *priv); > + void *(*get_priv)(const struct scmi_protocol_handle *ph); > +}; So scmi_xfer_ops are the ops that actually talks with the scmi firmware on the other end , right ? IIUC, these ops are the same for all the protocols of a scmi instance. Imho, this struct is not the right place for these ops to reside.You are inadvertently exposing scmi internal details to the client drivers. There is no reason why this should be part of scmi_handle. The protocols can extract it from the handle during protocol_reigster, right? So, now to the second part, why do you need a scmi_protocol_handle? Again IIUC, if you have set_priv and get_priv hooks and get_ops and put_ops hooks, there is nothing that scmi_protocol_handle is providing extra, right? As mentioned in the comments for last patch any reason all of this cannot be rolled into scmi_protocol? As long as you are not supporting multiple scmi_protocol_instance and scmi_protocol_handle for a protocol, I don't think having separate structs make sense. And you need to do this only if you think there can be multiple versions/instances of a protocol in the same scmi instance. Or am I missing something here ? > + > +/** > + * struct scmi_xfer_ops - References to the core SCMI xfer operations. > + * @version_get: Get this version protocol. > + * @xfer_get_init: Initialize one struct xfer if any xfer slot is free. > + * @reset_rx_to_maxsz: Reset rx size to max transport size. > + * @do_xfer: Do the SCMI transfer. > + * @do_xfer_with_response: Do the SCMI transfer waiting for a response. > + * @xfer_put: Free the xfer slot. > + * > + * Note that all this operations expect a protocol handle as first parameter; > + * they then internally use it to infer the underlying protocol number: this > + * way is not possible for a protocol implementation to forge messages for > + * another protocol. > + */ > +struct scmi_xfer_ops { > + int (*version_get)(const struct scmi_protocol_handle *ph, u32 *version); > + int (*xfer_get_init)(const struct scmi_protocol_handle *ph, u8 msg_id, > + size_t tx_size, size_t rx_size, > + struct scmi_xfer **p); > + void (*reset_rx_to_maxsz)(const struct scmi_protocol_handle *ph, > + struct scmi_xfer *xfer); > + int (*do_xfer)(const struct scmi_protocol_handle *ph, > + struct scmi_xfer *xfer); > + int (*do_xfer_with_response)(const struct scmi_protocol_handle *ph, > + struct scmi_xfer *xfer); > + void (*xfer_put)(const struct scmi_protocol_handle *ph, > + struct scmi_xfer *xfer); > +}; > + > +struct scmi_revision_info * > +scmi_get_revision_area(const struct scmi_protocol_handle *ph); > int scmi_handle_put(const struct scmi_handle *handle); > struct scmi_handle *scmi_handle_get(struct device *dev); > void scmi_set_handle(struct scmi_device *scmi_dev); > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c > index beae8991422d..8ca04acb6abb 100644 > --- a/drivers/firmware/arm_scmi/driver.c > +++ b/drivers/firmware/arm_scmi/driver.c > @@ -72,19 +72,28 @@ struct scmi_xfers_info { > > /** > * struct scmi_protocol_instance - Describe an initialized protocol instance. > + * @handle: Reference to the SCMI handle associated to this protocol instance. > * @proto: A reference to the protocol descriptor. > * @gid: A reference for per-protocol devres management. > * @users: A refcount to track effective users of this protocol. > + * @priv: Reference for optional protocol private data. > + * @ph: An embedded protocol handle that will be passed down to protocol > + * initialization code to identify this instance. > * > * Each protocol is initialized independently once for each SCMI platform in > * which is defined by DT and implemented by the SCMI server fw. > */ > struct scmi_protocol_instance { > + const struct scmi_handle *handle; > const struct scmi_protocol *proto; > void *gid; > refcount_t users; > + void *priv; > + struct scmi_protocol_handle ph; > }; > > +#define ph_to_pi(h) container_of(h, struct scmi_protocol_instance, ph) > + > /** > * struct scmi_info - Structure representing a SCMI instance > * > @@ -543,6 +552,57 @@ int scmi_version_get(const struct scmi_handle *handle, u8 protocol, > return ret; > } > > +/** > + * scmi_set_protocol_priv - Set protocol specific data at init time > + * > + * @ph: A reference to the protocol handle. > + * @priv: The private data to set. > + * > + * Return: 0 on Success > + */ > +static int scmi_set_protocol_priv(const struct scmi_protocol_handle *ph, > + void *priv) > +{ > + struct scmi_protocol_instance *pi = ph_to_pi(ph); > + > + pi->priv = priv; > + > + return 0; > +} > + > +/** > + * scmi_get_protocol_priv - Set protocol specific data at init time > + * > + * @ph: A reference to the protocol handle. > + * > + * Return: Protocol private data if any was set. > + */ > +static void *scmi_get_protocol_priv(const struct scmi_protocol_handle *ph) > +{ > + const struct scmi_protocol_instance *pi = ph_to_pi(ph); > + > + return pi->priv; > +} > + > +/** > + * scmi_get_revision_area - Retrieve version memory area. > + * > + * @ph: A reference to the protocol handle. > + * > + * A helper to grab the version memory area reference during SCMI Base protocol > + * initialization. > + * > + * Return: A reference to the version memory area associated to the SCMI > + * instance underlying this protocol handle. > + */ > +struct scmi_revision_info * > +scmi_get_revision_area(const struct scmi_protocol_handle *ph) > +{ > + const struct scmi_protocol_instance *pi = ph_to_pi(ph); > + > + return pi->handle->version; > +} > + > /** > * scmi_get_protocol_instance - Protocol initialization helper. > * @handle: A reference to the SCMI platform instance. > @@ -588,6 +648,10 @@ scmi_get_protocol_instance(struct scmi_handle *handle, u8 protocol_id) > > pi->gid = gid; > pi->proto = proto; > + pi->handle = handle; > + pi->ph.dev = handle->dev; > + pi->ph.set_priv = scmi_set_protocol_priv; > + pi->ph.get_priv = scmi_get_protocol_priv; > refcount_set(&pi->users, 1); > /* proto->init is assured NON NULL by scmi_protocol_register */ > ret = pi->proto->init(handle); > -- Warm Regards Thara _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel