Messages in this thread | | | Date | Wed, 4 Oct 2023 10:48:58 -0700 | Subject | Re: [PATCH v4 4/4] firmware: arm_scmi: Add qcom hvc/shmem transport support | From | Nikunj Kela <> |
| |
On 10/4/2023 9:06 AM, Sudeep Holla wrote: > 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-descriptor. >>>> >>>> The hypervisor, in addition to the function-id, expects the capability-id >>>> to be passed in x1 register when HVC call is invoked. >>>> >>>> The function-id & capability-id are allocated by the hypervisor on bootup >>>> and are stored in the shmem region by the firmware before starting Linux. >>>> >>>> Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com> >>>> --- >>>> 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/arm_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[] = { >>>> #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC >>>> { .compatible = "arm,scmi-smc", .data = &scmi_smc_desc}, >>>> { .compatible = "arm,scmi-smc-param", .data = &scmi_smc_desc}, >>>> + { .compatible = "qcom,scmi-hvc-shmem", .data = &scmi_smc_desc}, >>>> #endif >>>> #ifdef CONFIG_ARM_SCMI_TRANSPORT_VIRTIO >>>> { .compatible = "arm,scmi-virtio", .data = &scmi_virtio_desc}, >>>> diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/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 virtual >>>> + * platforms >>>> + * @qcom_xport: Flag to indicate the transport on Qualcomm virtual platforms >>>> */ >>>> 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 64 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 u64. >> 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. Ok. >>>> + 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). Ok. >>>> int ret; >>>> if (!tx) >>>> @@ -158,9 +164,34 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, >>>> return -EADDRNOTAVAIL; >>>> } >>>> - ret = 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 = true; >>>> + >>>> + /* The func-id & capability-id are kept in last 16 bytes of shmem. >>>> + * +-------+ >>>> + * | | >>>> + * | shmem | >>>> + * | | >>>> + * | | >>>> + * +-------+ <-- (size - 16) >>>> + * | funcId| >>>> + * +-------+ <-- (size - 8) >>>> + * | capId | >>>> + * +-------+ <-- size >>>> + */ >>>> + >>>> + func_id = 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. Ok. >> 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. 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. Ok. >>>> +#ifdef CONFIG_ARM64 >>> I would rather make this arch agnostic using CONFIG_64BIT >> ok. >>>> + cap_id = readq((void __iomem *)(scmi_info->shmem) + size - 8); >>> Do you need __iomem typecast here ? Is scmi_info->shmem not already __iomem ? >>> Also scmi_info->shmem is ioremapped just few steps above and you are using >>> 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. Ok. >>>> +#else >>>> + /* capability-id is 32 bit wide on 32bit machines */ >>>> + cap_id = 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 = 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 = 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 = func_id; >>>> + scmi_info->cap_id = cap_id; >>>> scmi_info->cinfo = cinfo; >>>> smc_channel_lock_init(scmi_info); >>>> cinfo->transport_info = scmi_info; >>>> @@ -213,6 +245,7 @@ static int smc_send_message(struct scmi_chan_info *cinfo, >>>> struct arm_smccc_res res; >>>> unsigned long page = scmi_info->param_page; >>>> unsigned long offset = scmi_info->param_offset; >>>> + unsigned long cap_id = (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_info *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 flag. >>> 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. Ok. >
| |