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 2BA84E7C4DB for ; Wed, 4 Oct 2023 16:07:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; 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=v0GxO8dQu8M1+Rq6wCr6If7hIzfHMLrKNJSMxPCHuqw=; b=SPxb9si4KfiAuu BD/X929/XeI5lbMqxFQYiiR5M3Zbvr/YnhZheMmn50oG9jAdcv9aF7761fHOjSG185wg0M9Z3qUqp iwB07iT/aqEV5tReCjVaBQieVLQJUy61d932rxW1w/Hi0Uo3Qh81aUQqzkwmEijEXOYzMQ0KR82oP M6+1r3i0Z1HVVbF6cGsHCXRqxDGWcl6RZqhxPCjvsMniqGN3Tfht9nF2KEpoow+2U84j5zxTN7Dwu sz3pSez91yhiGSVRD7oJal0JnOdbR6j4UdFUuLs80lbC+XuvVjKMnCtBTRd9lovgDZan7Do2Ip7ob gcHg72RIFTnbgf4eKEog==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qo4OV-000R4c-0D; Wed, 04 Oct 2023 16:06:39 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qo4OS-000R3S-1r for linux-arm-kernel@lists.infradead.org; Wed, 04 Oct 2023 16:06:38 +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 11DB2C15; Wed, 4 Oct 2023 09:07:13 -0700 (PDT) Received: from bogus (e103737-lin.cambridge.arm.com [10.1.197.49]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E57103F762; Wed, 4 Oct 2023 09:06:32 -0700 (PDT) Date: Wed, 4 Oct 2023 17:06:30 +0100 From: Sudeep Holla To: Nikunj Kela Cc: cristian.marussi@arm.com, robh+dt@kernel.org, Sudeep Holla , krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org, andersson@kernel.org, konrad.dybcio@linaro.org, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org Subject: Re: [PATCH v4 4/4] firmware: arm_scmi: Add qcom hvc/shmem transport support Message-ID: <20231004160630.pxspafszlt6o7oj6@bogus> References: <20230718160833.36397-1-quic_nkela@quicinc.com> <20230911194359.27547-1-quic_nkela@quicinc.com> <20230911194359.27547-5-quic_nkela@quicinc.com> <20231003111914.63z35sn3r3k7drtp@bogus> <6246714a-3b40-e1b6-640e-560ba55b6436@quicinc.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <6246714a-3b40-e1b6-640e-560ba55b6436@quicinc.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231004_090636_705933_C5E1E03B X-CRM114-Status: GOOD ( 52.81 ) 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="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Oct 03, 2023 at 09:16:27AM -0700, Nikunj Kela wrote: > = > On 10/3/2023 4:19 AM, Sudeep Holla wrote: > > On Mon, Sep 11, 2023 at 12:43:59PM -0700, Nikunj Kela wrote: > > > This change adds the support for SCMI message exchange on Qualcomm > > > virtual platforms. > > > = > > > The hypervisor associates an object-id also known as capability-id > > > with each hvc doorbell object. The capability-id is used to identify = the > > > doorbell from the VM's capability namespace, similar to a file-descri= ptor. > > > = > > > The hypervisor, in addition to the function-id, expects the capabilit= y-id > > > to be passed in x1 register when HVC call is invoked. > > > = > > > The function-id & capability-id are allocated by the hypervisor on bo= otup > > > and are stored in the shmem region by the firmware before starting Li= nux. > > > = > > > Signed-off-by: Nikunj Kela > > > --- > > > drivers/firmware/arm_scmi/driver.c | 1 + > > > drivers/firmware/arm_scmi/smc.c | 47 ++++++++++++++++++++++++++-= --- > > > 2 files changed, 43 insertions(+), 5 deletions(-) > > > = > > > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/ar= m_scmi/driver.c > > > index 87383c05424b..ea344bc6ae49 100644 > > > --- a/drivers/firmware/arm_scmi/driver.c > > > +++ b/drivers/firmware/arm_scmi/driver.c > > > @@ -2915,6 +2915,7 @@ static const struct of_device_id scmi_of_match[= ] =3D { > > > #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC > > > { .compatible =3D "arm,scmi-smc", .data =3D &scmi_smc_desc}, > > > { .compatible =3D "arm,scmi-smc-param", .data =3D &scmi_smc_desc}, > > > + { .compatible =3D "qcom,scmi-hvc-shmem", .data =3D &scmi_smc_desc}, > > > #endif > > > #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO > > > { .compatible =3D "arm,scmi-virtio", .data =3D &scmi_virtio_desc}, > > > diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_s= cmi/smc.c > > > index 0a0b7e401159..94ec07fdc14a 100644 > > > --- a/drivers/firmware/arm_scmi/smc.c > > > +++ b/drivers/firmware/arm_scmi/smc.c > > > @@ -50,6 +50,9 @@ > > > * @func_id: smc/hvc call function id > > > * @param_page: 4K page number of the shmem channel > > > * @param_offset: Offset within the 4K page of the shmem channel > > > + * @cap_id: hvc doorbell's capability id to be used on Qualcomm virt= ual > > > + * platforms > > > + * @qcom_xport: Flag to indicate the transport on Qualcomm virtual p= latforms > > > */ > > > struct scmi_smc { > > > @@ -63,6 +66,8 @@ struct scmi_smc { > > > u32 func_id; > > > u32 param_page; > > > u32 param_offset; > > > + u64 cap_id; > > Can it be unsigned long instead so that it just works for both 32 and 6= 4 bit. > = > My first version of this patch was ulong but Bjorn suggested to make this > structure size fixed i.e. architecture independent. Hence changed it to u= 64. > If you are ok with ulong, I can change it back to ulong. > SMCCC pre-v1.2 used the common structure in that way. I don't see any issue with that. I haven't followed Bjorn suggestions/comments though. > = > > = > > > + bool qcom_xport; > > Do we really need this ? > = > Not if we initialize it with a negative value since 0 is a valid value for > cap-id. > Fine with negative value(-EINVAL may be). > > > int ret; > > > if (!tx) > > > @@ -158,9 +164,34 @@ static int smc_chan_setup(struct scmi_chan_info = *cinfo, struct device *dev, > > > return -EADDRNOTAVAIL; > > > } > > > - ret =3D of_property_read_u32(dev->of_node, "arm,smc-id", &func_id); > > > - if (ret < 0) > > > - return ret; > > > + if (of_device_is_compatible(dev->of_node, "qcom,scmi-hvc-shmem")) { > > > + scmi_info->qcom_xport =3D true; > > > + > > > + /* The func-id & capability-id are kept in last 16 bytes of shmem. > > > + * +-------+ > > > + * | | > > > + * | shmem | > > > + * | | > > > + * | | > > > + * +-------+ <-- (size - 16) > > > + * | funcId| > > > + * +-------+ <-- (size - 8) > > > + * | capId | > > > + * +-------+ <-- size > > > + */ > > > + > > > + func_id =3D readl((void __iomem *)(scmi_info->shmem) + size - 16); > > So unlike 'arm,scmi-smc', you don't want 'arm,smc-id' in the DT ? Any > > particular reason ? Just to get both FID and cap ID from shmem ? > I am fine either way. If you use from DT(via arm,smc-id), then "qcom,scmi" can be just addition compatible that expects you to read cap-id from the shmem. May need adjustment in the binding as you allow both "qcom,scmi-smc", "arm,scmi-smc". I will leave the details to you. > I could use smc-id binding for func-id, it's just two parameters will come > from two different places so thought of keeping everything at one place to > maintain consistency.=A0 Since DT can't take cap-id, I decided to move > func-id. I am fine if you want me to use smc-id binding. > Up to you. If you want to make "qcom,scmi-smc" and "arm,scmi-smc" compatible in way in that way or you can keep it incompatible as you have proposed in this patch set. > = > > > +#ifdef CONFIG_ARM64 > > I would rather make this arch agnostic using CONFIG_64BIT > ok. > > = > > > + cap_id =3D readq((void __iomem *)(scmi_info->shmem) + size - 8); > > Do you need __iomem typecast here ? Is scmi_info->shmem not already __i= omem ? > > Also scmi_info->shmem is ioremapped just few steps above and you are us= ing > > read* here, is that safe ? > = > I saw some compilation warnings without __iomem. I will use ioread* API > instead of read*. > That was the clue that you were using __iomem with read* calls IMO. > = > > = > > > +#else > > > + /* capability-id is 32 bit wide on 32bit machines */ > > > + cap_id =3D rieadl((void __iomem *)(scmi_info->shmem) + size - 8); > > Other thought once you move for u64 to unsigned long you need not have > > #ifdeffery, just do copy of sizeof(unsigned long) > Right, my first version was like that only. OK > > = > > > +#endif > > > + } else { > > > + ret =3D of_property_read_u32(dev->of_node, "arm,smc-id", &func_id); > > > + if (ret < 0) > > > + return ret; > > > + } > > > if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param")) { > > > scmi_info->param_page =3D SHMEM_PAGE(res.start); > > > @@ -184,6 +215,7 @@ static int smc_chan_setup(struct scmi_chan_info *= cinfo, struct device *dev, > > > } > > > scmi_info->func_id =3D func_id; > > > + scmi_info->cap_id =3D cap_id; > > > scmi_info->cinfo =3D cinfo; > > > smc_channel_lock_init(scmi_info); > > > cinfo->transport_info =3D scmi_info; > > > @@ -213,6 +245,7 @@ static int smc_send_message(struct scmi_chan_info= *cinfo, > > > struct arm_smccc_res res; > > > unsigned long page =3D scmi_info->param_page; > > > unsigned long offset =3D scmi_info->param_offset; > > > + unsigned long cap_id =3D (unsigned long)scmi_info->cap_id; > > > /* > > > * Channel will be released only once response has been > > > @@ -222,8 +255,12 @@ static int smc_send_message(struct scmi_chan_inf= o *cinfo, > > > shmem_tx_prepare(scmi_info->shmem, xfer, cinfo); > > > - arm_smccc_1_1_invoke(scmi_info->func_id, page, offset, 0, 0, 0, 0, = 0, > > > - &res); > > > + if (scmi_info->qcom_xport) > > Just make sure cap_id is set only for qcom and just use that as your fl= ag. > > No point in setting always true scmi_info->qcom_xport and using it here. > ok, I can remove that. Though 0 is a valid value for cap-id so will have = to > init cap-id with a negative value. Yes as mentioned above. -- = Regards, Sudeep _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel