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 C1A9EC3DA49 for ; Fri, 26 Jul 2024 15:00:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To: Content-Transfer-Encoding:Content-Type: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=unlZ+BVMXtVm/5mIrN5gEABFFEiEYsNd4ygBSWKgHnw=; b=U/IyCpYUuaNo33mZdU0Ds+KWH4 8taqYIZfMShbpvthB6YAtZqxpAgeWSn+TbSlS3NT3qWzja1+zEXi+vqZYSXUOk+4WBJI3nSfChbYE Y5+hRy0H62LXrrtjbz7/amHc5mV2jNM31iFSmQ4KIqzZpStUDqPr+vHpCCuLU6DeJJsYOeeEct/50 8obfxsK6sCMuSDWymUQ9oTtPOKF2DFw57nE79o5c8GYD0mOlQmcHDw++Z/nUrqB2J7TJz0FRHvzWd JpkotqQC82aj6j57562vmyOHNnSIY7DxLbt3/7e8LKsAu7+vRRWEkHI3YRCLnWeG+4VWBzz+QuE+L G2hljRKQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sXMQe-00000004DqQ-2RPh; Fri, 26 Jul 2024 15:00:20 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sXMQD-00000004Dhu-21GU for linux-arm-kernel@lists.infradead.org; Fri, 26 Jul 2024 14:59:54 +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 91CD41007; Fri, 26 Jul 2024 08:00:18 -0700 (PDT) Received: from pluto (usa-sjc-mx-foss1.foss.arm.com [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E02BE3F73F; Fri, 26 Jul 2024 07:59:50 -0700 (PDT) Date: Fri, 26 Jul 2024 15:59:44 +0100 From: Cristian Marussi To: Etienne CARRIERE Cc: Cristian Marussi , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "arm-scmi@vger.kernel.org" , "sudeep.holla@arm.com" , "james.quinlan@broadcom.com" , "f.fainelli@gmail.com" , "vincent.guittot@linaro.org" , "peng.fan@oss.nxp.com" , "michal.simek@amd.com" , "quic_sibis@quicinc.com" , "quic_nkela@quicinc.com" , "ptosi@google.com" , "dan.carpenter@linaro.org" , "souvik.chakravarty@arm.com" Subject: Re: [PATCH v2 3/8] firmware: arm_scmi: Add support for standalone transport drivers Message-ID: References: <20240710173153.4060457-1-cristian.marussi@arm.com> <20240710173153.4060457-4-cristian.marussi@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240726_075953_631551_704F7AB0 X-CRM114-Status: GOOD ( 28.93 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Jul 23, 2024 at 01:39:41PM +0000, Etienne CARRIERE wrote: > Hi Cristian, > > Few nitpicking comments. > > On Wednesday, July 10, 2024, Cristian Marussi wrote:   > > Extend the core SCMI stack with structures and methods to allow for > > transports to be split out as standalone drivers, while still supporting > > old style transports, defined as built into the SCMI core stack. > > > > No functional change. > > > > Signed-off-by: Cristian Marussi > > --- > > NOTE: old style transport support will be removed later in this series. > > > > v1 --> v2 > > - fixed comit message > > --- > >  drivers/firmware/arm_scmi/common.h | 84 ++++++++++++++++++++++++++++++ > >  drivers/firmware/arm_scmi/driver.c | 44 +++++++++++++++- > >  drivers/firmware/arm_scmi/msg.c    |  5 ++ > >  drivers/firmware/arm_scmi/shmem.c  |  5 ++ > >  4 files changed, 136 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h > > index 8e5751aaa600..4af06810eb39 100644 > > --- a/drivers/firmware/arm_scmi/common.h > > +++ b/drivers/firmware/arm_scmi/common.h > > @@ -349,6 +349,8 @@ struct scmi_shared_mem_operations { > >                                       bool tx, struct resource *res); > >  }; > >   > > +const struct scmi_shared_mem_operations *scmi_shared_mem_operations_get(void); > > + > >  /* declarations for message passing transports */ > >  struct scmi_msg_payld; > >   > > @@ -376,6 +378,88 @@ struct scmi_message_operations { > >                                     size_t max_len, struct scmi_xfer *xfer); > >  }; > >   > > +const struct scmi_message_operations *scmi_message_operations_get(void); > > + > > +/** > > + * struct scmi_transport_core_operations  - Transpoert core operations > > + * > > + * @bad_message_trace: An helper to report a malformed/unexpected message > > + * @rx_callback: Callback to report received messages > > + * @shmem: Datagram operations for shared memory based transports > > + * @msg: Datagram operations for message based transports > > + */ > > +struct scmi_transport_core_operations { > > +       void (*bad_message_trace)(struct scmi_chan_info *cinfo, > > +                                 u32 msg_hdr, enum scmi_bad_msg err); > > +       void (*rx_callback)(struct scmi_chan_info *cinfo, u32 msg_hdr, > > +                           void *priv); > > +       const struct scmi_shared_mem_operations *shmem; > > +       const struct scmi_message_operations *msg; > > +}; > > + > > +/** > > + * struct scmi_transport  - A structure representing a configured transport > > + * > > + * @supplier: Device representimng the transport and acting as a supplier for > > typo: s/representimng/representing/ > Fixed in V3. (...still to be posted) > > + *           the core SCMI stack > > + * @desc: Transport descriptor > > + * @core_ops: A pointer to a pointer used by the core SCMI stack to make the > > + *           core transport operations accessible to the transports. > > + */ > > +struct scmi_transport { > > +       struct device *supplier; > > +       const struct scmi_desc *desc; > > +       struct scmi_transport_core_operations **core_ops; > > +}; > > + > > +#define DEFINE_SCMI_TRANSPORT_DRIVER(__trans, __match_table, __core_ptr)\ > > +static int __trans##_probe(struct platform_device *pdev)               \ > > +{                                                                      \ > > +       struct scmi_transport *scmi_trans;                              \ > > +       struct platform_device *scmi_pdev;                              \ > > +       struct device *dev = &pdev->dev;                                \ > > +                                                                       \ > > +       scmi_trans = devm_kzalloc(dev, sizeof(*scmi_trans), GFP_KERNEL);\ > > +       if (!scmi_trans)                                                \ > > +               return -ENOMEM;                                         \ > > +                                                                       \ > > +       scmi_pdev = devm_kzalloc(dev, sizeof(*scmi_pdev), GFP_KERNEL);  \ > > +       if (!scmi_pdev)                                                 \ > > +               return -ENOMEM;                                         \ > > +                                                                       \ > > +       scmi_trans->supplier = dev;                                     \ > > +       scmi_trans->desc = &__trans##_desc;                             \ > > It's a bit weird the scmi_desc shall be specifically labeled __trans##_desc > in the transport driver source file while match table and transport core > operations instances references are passed as arguments. I think it's > worth having the scmi_desc label also passed as an argument to > DEFINE_SCMI_TRANSPORT_DRIVER() macro. Yes, I agree, I was unsure about this so I have reworked all of these in V3 to pass as explicit parameter the driver name and desc name. > > > +       scmi_trans->core_ops = __core_ptr;                              \ > > +                                                                       \ > > +       scmi_pdev->name = "arm-scmi";                                   \ > > +       scmi_pdev->id = PLATFORM_DEVID_AUTO;                            \ > > +       scmi_pdev->dev.platform_data = scmi_trans;                      \ > > +                                                                       \ > > +       device_set_of_node_from_dev(&scmi_pdev->dev, dev);              \ > > +                                                                       \ > > +       dev_set_drvdata(dev, scmi_pdev);                                \ > > +                                                                       \ > > +       return platform_device_register(scmi_pdev);                     \ > > +}                                                                      \ > > +                                                                       \ > > +static void __trans##_remove(struct platform_device *pdev)             \ > > +{                                                                      \ > > +       struct platform_device *scmi_pdev;                              \ > > +                                                                       \ > > +       scmi_pdev = dev_get_drvdata(&pdev->dev);                        \ > > +                                                                       \ > > +       platform_device_unregister(scmi_pdev);                          \ > > +}                                                                      \ > > +                                                                       \ > > +static struct platform_driver __trans##_driver = {                     \ > > Same here. I think __trans##_driver label should be also explicitly > passed as an argument to DEFINE_SCMI_TRANSPORT_DRIVER(). > Fixed in V3. Thanks, Cristian