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

From: Nikunj Kela <quic_nkela@quicinc.com>
To: Brian Masney <bmasney@redhat.com>
Cc: <sudeep.holla@arm.com>, <cristian.marussi@arm.com>,
	<robh+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
	<conor+dt@kernel.org>, <andersson@kernel.org>,
	<konrad.dybcio@linaro.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-arm-msm@vger.kernel.org>
Subject: Re: [PATCH v4 1/4] firmware: arm_scmi: Add polling support for completion in smc
Date: Mon, 2 Oct 2023 11:36:01 -0700	[thread overview]
Message-ID: <8d5af4d7-23fc-a03e-bad8-760209b537a0@quicinc.com> (raw)
In-Reply-To: <ZRsJWdZVMc+F5ZgF@brian-x1>

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@quicinc.com>
>> ---
>>   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.