| Message ID | 20231006164206.40710-3-quic_nkela@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add qcom smc/hvc transport support | expand |
On Fri, Oct 06, 2023 at 09:42:06AM -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 smc/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 SMC/HVC call is invoked. > > The capability-id is allocated by the hypervisor on bootup and is stored in > the shmem region by the firmware before starting Linux. > Since you are happy to move to signed value, I assume you are happy to loose upper half of the range values ? Anyways after Bjorn pointed out inconsistency, I am thinking of moving all the values to unsigned long to work with both 32bit and 64bit. Does the below delta on top of this patch works for you and makes sense? -- Regards, Sudeep -->8 diff --git c/drivers/firmware/arm_scmi/smc.c i/drivers/firmware/arm_scmi/smc.c index bf0b7769c7b2..e00c5e81c8d9 100644 --- c/drivers/firmware/arm_scmi/smc.c +++ i/drivers/firmware/arm_scmi/smc.c @@ -15,6 +15,7 @@ #include <linux/of.h> #include <linux/of_address.h> #include <linux/of_irq.h> +#include <linux/limits.h> #include <linux/processor.h> #include <linux/slab.h> @@ -65,7 +66,7 @@ struct scmi_smc { unsigned long func_id; unsigned long param_page; unsigned long param_offset; - s64 cap_id; + unsigned long cap_id; }; static irqreturn_t smc_msg_done_isr(int irq, void *data) @@ -127,11 +128,11 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, bool tx) { struct device *cdev = cinfo->dev; + unsigned long cap_id = ULONG_MAX; struct scmi_smc *scmi_info; resource_size_t size; struct resource res; struct device_node *np; - s64 cap_id = -EINVAL; u32 func_id; int ret; @@ -167,6 +168,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, return ret; if (of_device_is_compatible(dev->of_node, "qcom,scmi-smc")) { + void __iomem *ptr = (void __iomem *)scmi_info->shmem + size - 8; /* The capability-id is kept in last 8 bytes of shmem. * +-------+ * | | @@ -177,12 +179,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, * | capId | * +-------+ <-- size */ - -#ifdef CONFIG_64BIT - cap_id = ioread64((void *)scmi_info->shmem + size - 8); -#else - cap_id = ioread32((void *)scmi_info->shmem + size - 8); -#endif + memcpy_fromio(&cap_id, ptr, sizeof(cap_id)); } if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param")) { @@ -247,7 +244,7 @@ static int smc_send_message(struct scmi_chan_info *cinfo, shmem_tx_prepare(scmi_info->shmem, xfer, cinfo); - if (cap_id >= 0) + if (cap_id != ULONG_MAX) arm_smccc_1_1_invoke(scmi_info->func_id, cap_id, 0, 0, 0, 0, 0, 0, &res); else
On 10/9/2023 7:47 AM, Sudeep Holla wrote: > On Fri, Oct 06, 2023 at 09:42:06AM -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 smc/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 SMC/HVC call is invoked. >> >> The capability-id is allocated by the hypervisor on bootup and is stored in >> the shmem region by the firmware before starting Linux. >> > Since you are happy to move to signed value, I assume you are happy to loose > upper half of the range values ? > > Anyways after Bjorn pointed out inconsistency, I am thinking of moving > all the values to unsigned long to work with both 32bit and 64bit. > > Does the below delta on top of this patch works for you and makes sense? This looks good to me. Will do some testing and float v6 with the changes you suggested below. Thanks > > -- > Regards, > Sudeep > > -->8 > diff --git c/drivers/firmware/arm_scmi/smc.c i/drivers/firmware/arm_scmi/smc.c > index bf0b7769c7b2..e00c5e81c8d9 100644 > --- c/drivers/firmware/arm_scmi/smc.c > +++ i/drivers/firmware/arm_scmi/smc.c > @@ -15,6 +15,7 @@ > #include <linux/of.h> > #include <linux/of_address.h> > #include <linux/of_irq.h> > +#include <linux/limits.h> > #include <linux/processor.h> > #include <linux/slab.h> > > @@ -65,7 +66,7 @@ struct scmi_smc { > unsigned long func_id; > unsigned long param_page; > unsigned long param_offset; > - s64 cap_id; > + unsigned long cap_id; > }; > > static irqreturn_t smc_msg_done_isr(int irq, void *data) > @@ -127,11 +128,11 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, > bool tx) > { > struct device *cdev = cinfo->dev; > + unsigned long cap_id = ULONG_MAX; > struct scmi_smc *scmi_info; > resource_size_t size; > struct resource res; > struct device_node *np; > - s64 cap_id = -EINVAL; > u32 func_id; > int ret; > > @@ -167,6 +168,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, > return ret; > > if (of_device_is_compatible(dev->of_node, "qcom,scmi-smc")) { > + void __iomem *ptr = (void __iomem *)scmi_info->shmem + size - 8; > /* The capability-id is kept in last 8 bytes of shmem. > * +-------+ > * | | > @@ -177,12 +179,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, > * | capId | > * +-------+ <-- size > */ > - > -#ifdef CONFIG_64BIT > - cap_id = ioread64((void *)scmi_info->shmem + size - 8); > -#else > - cap_id = ioread32((void *)scmi_info->shmem + size - 8); > -#endif > + memcpy_fromio(&cap_id, ptr, sizeof(cap_id)); > } > > if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param")) { > @@ -247,7 +244,7 @@ static int smc_send_message(struct scmi_chan_info *cinfo, > > shmem_tx_prepare(scmi_info->shmem, xfer, cinfo); > > - if (cap_id >= 0) > + if (cap_id != ULONG_MAX) > arm_smccc_1_1_invoke(scmi_info->func_id, cap_id, 0, 0, 0, 0, 0, > 0, &res); > else >
On Mon, Oct 09, 2023 at 07:59:08AM -0700, Nikunj Kela wrote: > > On 10/9/2023 7:47 AM, Sudeep Holla wrote: > > On Fri, Oct 06, 2023 at 09:42:06AM -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 smc/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 SMC/HVC call is invoked. > > > > > > The capability-id is allocated by the hypervisor on bootup and is stored in > > > the shmem region by the firmware before starting Linux. > > > > > Since you are happy to move to signed value, I assume you are happy to loose > > upper half of the range values ? > > > > Anyways after Bjorn pointed out inconsistency, I am thinking of moving > > all the values to unsigned long to work with both 32bit and 64bit. > > > > Does the below delta on top of this patch works for you and makes sense? > > This looks good to me. Will do some testing and float v6 with the changes > you suggested below. Thanks > Please refer or use the patch from [1] when reposting. I rebased on my patch[2] that I posted few minutes back. I am trying to finalise the branch and send PR in next couple of days, so please test and post sooner. Sorry for the rush. -- Regards, Sudeep [1] https://git.kernel.org/sudeep.holla/h/for-next/scmi/updates [2] https://lore.kernel.org/r/20231009152049.1428872-1-sudeep.holla@arm.com
On 10/9/2023 8:29 AM, Sudeep Holla wrote: > On Mon, Oct 09, 2023 at 07:59:08AM -0700, Nikunj Kela wrote: >> On 10/9/2023 7:47 AM, Sudeep Holla wrote: >>> On Fri, Oct 06, 2023 at 09:42:06AM -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 smc/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 SMC/HVC call is invoked. >>>> >>>> The capability-id is allocated by the hypervisor on bootup and is stored in >>>> the shmem region by the firmware before starting Linux. >>>> >>> Since you are happy to move to signed value, I assume you are happy to loose >>> upper half of the range values ? >>> >>> Anyways after Bjorn pointed out inconsistency, I am thinking of moving >>> all the values to unsigned long to work with both 32bit and 64bit. >>> >>> Does the below delta on top of this patch works for you and makes sense? >> This looks good to me. Will do some testing and float v6 with the changes >> you suggested below. Thanks >> > Please refer or use the patch from [1] when reposting. I rebased on my > patch[2] that I posted few minutes back. I am trying to finalise the branch > and send PR in next couple of days, so please test and post sooner. Sorry > for the rush. Validated the patch from [1] below on Qualcomm ARM64 virtual platform using SMC64 convention. Thanks! > > -- > Regards, > Sudeep > [1] https://git.kernel.org/sudeep.holla/h/for-next/scmi/updates > [2] https://lore.kernel.org/r/20231009152049.1428872-1-sudeep.holla@arm.com
On Mon, Oct 09, 2023 at 10:49:44AM -0700, Nikunj Kela wrote: > > On 10/9/2023 8:29 AM, Sudeep Holla wrote: > > On Mon, Oct 09, 2023 at 07:59:08AM -0700, Nikunj Kela wrote: > > > On 10/9/2023 7:47 AM, Sudeep Holla wrote: > > > > On Fri, Oct 06, 2023 at 09:42:06AM -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 smc/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 SMC/HVC call is invoked. > > > > > > > > > > The capability-id is allocated by the hypervisor on bootup and is stored in > > > > > the shmem region by the firmware before starting Linux. > > > > > > > > > Since you are happy to move to signed value, I assume you are happy to loose > > > > upper half of the range values ? > > > > > > > > Anyways after Bjorn pointed out inconsistency, I am thinking of moving > > > > all the values to unsigned long to work with both 32bit and 64bit. > > > > > > > > Does the below delta on top of this patch works for you and makes sense? > > > This looks good to me. Will do some testing and float v6 with the changes > > > you suggested below. Thanks > > > > > Please refer or use the patch from [1] when reposting. I rebased on my > > patch[2] that I posted few minutes back. I am trying to finalise the branch > > and send PR in next couple of days, so please test and post sooner. Sorry > > for the rush. > > Validated the patch from [1] below on Qualcomm ARM64 virtual platform using > SMC64 convention. Thanks! > Thanks, since I have patched a bit, it is better if you post them so that we have a link for the exact patch on the list. Just pick up the patches from the branch[1] and post them as v6 with a change log so that all the details are captured for reference purposes.
On 10/9/2023 12:08 PM, Sudeep Holla wrote: > On Mon, Oct 09, 2023 at 10:49:44AM -0700, Nikunj Kela wrote: >> On 10/9/2023 8:29 AM, Sudeep Holla wrote: >>> On Mon, Oct 09, 2023 at 07:59:08AM -0700, Nikunj Kela wrote: >>>> On 10/9/2023 7:47 AM, Sudeep Holla wrote: >>>>> On Fri, Oct 06, 2023 at 09:42:06AM -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 smc/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 SMC/HVC call is invoked. >>>>>> >>>>>> The capability-id is allocated by the hypervisor on bootup and is stored in >>>>>> the shmem region by the firmware before starting Linux. >>>>>> >>>>> Since you are happy to move to signed value, I assume you are happy to loose >>>>> upper half of the range values ? >>>>> >>>>> Anyways after Bjorn pointed out inconsistency, I am thinking of moving >>>>> all the values to unsigned long to work with both 32bit and 64bit. >>>>> >>>>> Does the below delta on top of this patch works for you and makes sense? >>>> This looks good to me. Will do some testing and float v6 with the changes >>>> you suggested below. Thanks >>>> >>> Please refer or use the patch from [1] when reposting. I rebased on my >>> patch[2] that I posted few minutes back. I am trying to finalise the branch >>> and send PR in next couple of days, so please test and post sooner. Sorry >>> for the rush. >> Validated the patch from [1] below on Qualcomm ARM64 virtual platform using >> SMC64 convention. Thanks! >> > Thanks, since I have patched a bit, it is better if you post them so that > we have a link for the exact patch on the list. Just pick up the patches > from the branch[1] and post them as v6 with a change log so that all the > details are captured for reference purposes. v6 on its way, thanks! >
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index 87383c05424b..09371f40d61f 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-smc", .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 c193516a254d..3d594d65ab14 100644 --- a/drivers/firmware/arm_scmi/smc.c +++ b/drivers/firmware/arm_scmi/smc.c @@ -50,6 +50,8 @@ * @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: smc/hvc doorbell's capability id to be used on Qualcomm virtual + * platforms */ struct scmi_smc { @@ -63,6 +65,7 @@ struct scmi_smc { u32 func_id; u32 param_page; u32 param_offset; + s64 cap_id; }; static irqreturn_t smc_msg_done_isr(int irq, void *data) @@ -128,6 +131,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, resource_size_t size; struct resource res; struct device_node *np; + s64 cap_id = -EINVAL; u32 func_id; int ret; @@ -162,6 +166,25 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev, if (ret < 0) return ret; + if (of_device_is_compatible(dev->of_node, "qcom,scmi-smc")) { + /* The capability-id is kept in last 8 bytes of shmem. + * +-------+ + * | | + * | shmem | + * | | + * | | + * +-------+ <-- (size - 8) + * | capId | + * +-------+ <-- size + */ + +#ifdef CONFIG_64BIT + cap_id = ioread64((void *)scmi_info->shmem + size - 8); +#else + cap_id = ioread32((void *)scmi_info->shmem + size - 8); +#endif + } + if (of_device_is_compatible(dev->of_node, "arm,scmi-smc-param")) { scmi_info->param_page = SHMEM_PAGE(res.start); scmi_info->param_offset = SHMEM_OFFSET(res.start); @@ -184,6 +207,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 +237,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; + long cap_id = (long)scmi_info->cap_id; /* * Channel will be released only once response has been @@ -222,8 +247,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 (cap_id >= 0) + arm_smccc_1_1_invoke(scmi_info->func_id, cap_id, 0, 0, 0, 0, 0, + 0, &res); + else + arm_smccc_1_1_invoke(scmi_info->func_id, page, offset, 0, 0, 0, + 0, 0, &res); /* Only SMCCC_RET_NOT_SUPPORTED is valid error code */ if (res.a0) {
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 smc/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 SMC/HVC call is invoked. The capability-id is allocated by the hypervisor on bootup and is 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 | 33 ++++++++++++++++++++++++++++-- 2 files changed, 32 insertions(+), 2 deletions(-)