Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional parameters

[Sudeep Holla <sudeep.holla@arm.com>]

On Mon, May 01, 2023 at 07:39:29AM -0700, Nikunj Kela wrote:
> Reminder: Please review this patch! Thanks
>

Since the current merge window is open, there is no rush and hence I had put
this on hold until merge window close. Anyways, it looks good in general.
Couple of minor nits below:

> On 4/18/2023 11:56 AM, Nikunj Kela wrote:
> > This patch add support for passing shmem channel address as parameters
> > in smc/hvc call. The address is split into 4KB-page and offset.
> > This patch is useful when multiple scmi instances are using same smc-id
> > and firmware needs to distinguish among the instances.
> >

Drop the term "patch". You can refer it as change. It is not match after
it is applied to the git.

> > Signed-off-by: Nikunj Kela <quic_nkela@quicinc.com>
> > ---
> >   drivers/firmware/arm_scmi/driver.c |  1 +
> >   drivers/firmware/arm_scmi/smc.c    | 14 +++++++++++++-
> >   2 files changed, 14 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index e7d97b59963b..b5957cc12fee 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -2914,6 +2914,7 @@ static const struct of_device_id scmi_of_match[] = {
> >   #endif
> >   #ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC
> >   	{ .compatible = "arm,scmi-smc", .data = &scmi_smc_desc},
> > +	{ .compatible = "arm,scmi-smc-param", .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 93272e4bbd12..71e080b70df5 100644
> > --- a/drivers/firmware/arm_scmi/smc.c
> > +++ b/drivers/firmware/arm_scmi/smc.c
> > @@ -20,6 +20,11 @@
> >   #include "common.h"
> > +#define SHMEM_SHIFT 12
> > +#define SHMEM_SIZE (_AC(1, UL) << SHMEM_SHIFT)
> > +#define SHMEM_PAGE(x) ((unsigned long)((x) >> SHMEM_SHIFT))

Since we are dealing with 4kB pages only, I prefer to do:
#define SHMEM_SIZE     (SZ_4K)
#define SHMEM_SHIFT    12
#define SHMEM_PAGE(x) (_UL((x) >> SHMEM_SHIFT))

> > +#define SHMEM_OFFSET(x) ((x) & (SHMEM_SIZE - 1))
> > +

Also it is definitely worth adding comment about supporting just 4kB pages
and limitations associated with it here(e.g. we can support up to 44 bit
address) and also parameters to the SMC call so that others get clear
idea on how to use it if they need that in the future.

> >   /**
> >    * struct scmi_smc - Structure representing a SCMI smc transport
> >    *
> > @@ -30,6 +35,7 @@
> >    * @inflight: Atomic flag to protect access to Tx/Rx shared memory area.
> >    *	      Used when operating in atomic mode.
> >    * @func_id: smc/hvc call function id
> > + * @param: physical address of the shmem channel
> >    */
> >   struct scmi_smc {
> > @@ -40,6 +46,7 @@ struct scmi_smc {
> >   #define INFLIGHT_NONE	MSG_TOKEN_MAX
> >   	atomic_t inflight;
> >   	u32 func_id;
> > +	phys_addr_t param;
> >   };
> >   static irqreturn_t smc_msg_done_isr(int irq, void *data)
> > @@ -137,6 +144,8 @@ 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, "arm,scmi-smc-param"))
> > +		scmi_info->param = res.start;
> >   	/*
> >   	 * If there is an interrupt named "a2p", then the service and
> >   	 * completion of a message is signaled by an interrupt rather than by
> > @@ -179,6 +188,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
> >   {
> >   	struct scmi_smc *scmi_info = cinfo->transport_info;
> >   	struct arm_smccc_res res;
> > +	unsigned long page = SHMEM_PAGE(scmi_info->param);
> > +	unsigned long offset = SHMEM_OFFSET(scmi_info->param);

While I see you initialise param in smc_chan_setup, I wonder why the page
and offset itself be initialised once and reused instead of computing the
same fixed value on every smc_send_message. You can probably have param_page
and param_offset stashed instead of just single param value ?

> >   	/*
> >   	 * Channel will be released only once response has been
> > @@ -188,7 +199,8 @@ 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, 0, 0, 0, 0, 0, 0, 0, &res);
> > +	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) {

-- 
Regards,
Sudeep