Re: [PATCH v4 1/4] firmware: arm_scmi: Add polling support for completion in smc

[Nikunj Kela <quic_nkela@xxxxxxxxxxx>]

On 10/2/2023 11:18 AM, Brian Masney wrote:
> On Mon, Sep 11, 2023 at 12:43:56PM -0700, Nikunj Kela wrote:
> > Currently, the return from the smc call assumes the completion of
> > the scmi request. However this may not be true in virtual platforms
> > that are using hvc doorbell.
> >
> > This change adds a Kconfig to enable the polling for the request
> > completion.
> >
> > Signed-off-by: Nikunj Kela <quic_nkela@xxxxxxxxxxx>
> > ---
> >   drivers/firmware/arm_scmi/Kconfig | 14 ++++++++++++++
> >   drivers/firmware/arm_scmi/smc.c   | 15 ++++++++++++++-
> >   2 files changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/Kconfig b/drivers/firmware/arm_scmi/Kconfig
> > index ea0f5083ac47..771d60f8319f 100644
> > --- a/drivers/firmware/arm_scmi/Kconfig
> > +++ b/drivers/firmware/arm_scmi/Kconfig
> > @@ -125,6 +125,20 @@ config ARM_SCMI_TRANSPORT_SMC_ATOMIC_ENABLE
> >   	  in atomic context too, at the price of using a number of busy-waiting
> >   	  primitives all over instead. If unsure say N.
> >   
> > +config ARM_SCMI_TRANSPORT_SMC_POLL_COMPLETION
> > +	bool "Enable polling support for SCMI SMC transport"
> > +	depends on ARM_SCMI_TRANSPORT_SMC
> > +	help
> > +	  Enable completion polling support for SCMI SMC based transport.
> > +
> > +	  If you want the SCMI SMC based transport to poll for the completion,
> > +	  answer Y.
> > +	  Enabling completion polling might be desired in the absence of the a2p
> > +	  irq when the return from smc/hvc call doesn't indicate the completion
> > +	  of the SCMI requests. This might be useful for instances used in
> > +	  virtual platforms.
> > +	  If unsure say N.
> > +
> >   config ARM_SCMI_TRANSPORT_VIRTIO
> >   	bool "SCMI transport based on VirtIO"
> >   	depends on VIRTIO=y || VIRTIO=ARM_SCMI_PROTOCOL
> > diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
> > index c193516a254d..0a0b7e401159 100644
> > --- a/drivers/firmware/arm_scmi/smc.c
> > +++ b/drivers/firmware/arm_scmi/smc.c
> > @@ -250,6 +250,16 @@ static void smc_mark_txdone(struct scmi_chan_info *cinfo, int ret,
> >   	smc_channel_lock_release(scmi_info);
> >   }
> >   
> > +#ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC_POLL_COMPLETION
> > +static bool
> > +smc_poll_done(struct scmi_chan_info *cinfo, struct scmi_xfer *xfer)
> > +{
> > +	struct scmi_smc *scmi_info = cinfo->transport_info;
> > +
> > +	return shmem_poll_done(scmi_info->shmem, xfer);
> > +}
> > +#endif
> > +
> >   static const struct scmi_transport_ops scmi_smc_ops = {
> >   	.chan_available = smc_chan_available,
> >   	.chan_setup = smc_chan_setup,
> > @@ -257,6 +267,9 @@ static const struct scmi_transport_ops scmi_smc_ops = {
> >   	.send_message = smc_send_message,
> >   	.mark_txdone = smc_mark_txdone,
> >   	.fetch_response = smc_fetch_response,
> > +#ifdef CONFIG_ARM_SCMI_TRANSPORT_SMC_POLL_COMPLETION
> > +	.poll_done = smc_poll_done,
> > +#endif
> >   };
> >   
> >   const struct scmi_desc scmi_smc_desc = {
> > @@ -272,6 +285,6 @@ const struct scmi_desc scmi_smc_desc = {
> >   	 * for the issued command will be immmediately ready to be fetched
> >   	 * from the shared memory area.
> >   	 */
> > -	.sync_cmds_completed_on_ret = true,
> > +	.sync_cmds_completed_on_ret = !IS_ENABLED(CONFIG_ARM_SCMI_TRANSPORT_SMC_POLL_COMPLETION),
> >   	.atomic_enabled = IS_ENABLED(CONFIG_ARM_SCMI_TRANSPORT_SMC_ATOMIC_ENABLE),
>  From a Linux distributor viewpoint, it would be nice if this was
> determined at runtime, rather than at compile time. We generate a single
> kernel binary that's used on systems from multiple hardware
> manufacturers. We'd run into an issue if one company required this, but
> another one didn't. We may potentially run into this same type of issue
> with the upstream arm64 defconfig.
>
> Brian
This is a transport dependent property. Either the transport supports 
"completion on return of the smc call" or not. For a given platform, 
this will be fixed for all channels. This is similar to

CONFIG_ARM_SCMI_TRANSPORT_SMC_ATOMIC_ENABLE which is also a Kconfig.