Re: [PATCH v4 4/4] firmware: arm_scmi: Add qcom hvc/shmem transport support

[Nikunj Kela]

On 10/2/2023 11:34 AM, Brian Masney 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@xxxxxxxxxxx>
> > ---
> >   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;
> > +	bool qcom_xport;
> >   };
> [snip]
>
> >   static irqreturn_t smc_msg_done_isr(int irq, void *data)
> > @@ -129,6 +134,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
> >   	struct resource res;
> >   	struct device_node *np;
> >   	u32 func_id;
> > +	u64 cap_id;
> >   	int ret;
> [snip]
>
> > +		func_id = readl((void __iomem *)(scmi_info->shmem) + size - 16);
> > +#ifdef CONFIG_ARM64
> > +		cap_id = readq((void __iomem *)(scmi_info->shmem) + size - 8);
> > +#else
> > +		/* capability-id is 32 bit wide on 32bit machines */
> > +		cap_id = readl((void __iomem *)(scmi_info->shmem) + size - 8);
> > +#endif
> The 32 bit case is defined as a u64 in two places above.

That is done to make sure the size of the structure in memory is not 
architecture dependent. This was recommended in one of the previous 
version of this patch.


> > +
> > +		/* The func-id & capability-id are kept in last 16 bytes of shmem.
> > +		 *     +-------+
> > +		 *     |       |
> > +		 *     | shmem |
> > +		 *     |       |
> > +		 *     |       |
> > +		 *     +-------+ <-- (size - 16)
> > +		 *     | funcId|
> > +		 *     +-------+ <-- (size - 8)
> > +		 *     | capId |
> > +		 *     +-------+ <-- size
> > +		 */
> Personally I'd add one more space to the right side of the table after
> funcId.

I could do that but then in 32bit case, you would want one more space 
right after cap-id since it is 32 bit on 32 bit platform. If it helps, I 
can have two lay out one for 32bit and one for 64 bit.


> > -	arm_smccc_1_1_invoke(scmi_info->func_id, page, offset, 0, 0, 0, 0, 0,
> > -			     &res);
> > +	if (scmi_info->qcom_xport)
> > +		arm_smccc_1_1_hvc(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);
> Does it make sense to call this variable qcom_xport? Would hvc_xport be
> a more appropriate name?
>
> Brian

Cap-id is QCOM specific ABI parameter not HVC.

>