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

Nikunj Kela
2023-10-02T18:36:41Z

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.