Re: [PATCH v4 3/5] firmware: stratix10-svc: Add initial support for asynchronous communication with Stratix10 service channel

[Mahesh Rao <mahesh.rao@altera.com>]

Hi Mathew,

On 11-06-2025 09:40 pm, Matthew Gerlach wrote:
> 
> 
> On 6/10/25 8:37 AM, Mahesh Rao via B4 Relay wrote:
>> From: Mahesh Rao <mahesh.rao@altera.com>
>>
>> Introduce support for asynchronous communication
>> with the Stratix10 service channel. Define new
>> structures to enable asynchronous messaging with
>> the Secure Device Manager (SDM). Add and remove
>> asynchronous support for existing channels.
>> Implement initialization and cleanup routines for
>> the asynchronous framework. Enable sending and
>> polling of messages to the SDM asynchronously.
>>
>> The new public functions added are:
>> - stratix10_svc_add_async_client: Adds an client

>> +#define MAX_SDM_CLIENT_IDS 16
>> +/*Client ID for SIP Service Version 1.*/
>> +#define SIP_SVC_V1_CLIENT_ID 0x1
>> +/*Maximum number of SDM job IDs.*/
>> +#define MAX_SDM_JOB_IDS 16
>> +/*Number of bits used for asynchronous transaction hashing.*/
>> +#define ASYNC_TRX_HASH_BITS 3
>> +/* Total number of transaction IDs, which is a combination of
>> + * client ID and job ID.
>> + */
>> +#define TOTAL_TRANSACTION_IDS (MAX_SDM_CLIENT_IDS * MAX_SDM_JOB_IDS)
>> +
>> +/*Minimum major version of the ATF for Asynchronous transactions.*/
>> +#define ASYNC_ATF_MINIMUM_MAJOR_VERSION 0x3
>> +/*Minimum minor version of the ATF for Asynchronous transactions.*/
>> +#define ASYNC_ATF_MINIMUM_MINOR_VERSION 0x0
>> +
>> +/*Macro to extract the job ID from a transaction ID.*/
>> +#define STRATIX10_GET_JOBID(transaction_id) ((transaction_id) & 0xf)
>> +/*Macro to set a transaction ID using a client ID and a transaction 
>> ID.*/
>> +#define STRATIX10_SET_TRANSACTIONID(clientid, transaction_id) \
>> +    ((((clientid) & 0xf) << 4) | ((transaction_id) & 0xf))
> Consider using the macros GENMASK, FIELD_PREP, and FIELD_GET from linux/ 
> bitfield.h for the above.

Will make the change
> 
>> +
>> +/* Macro to set a transaction ID for SIP SMC using the lower 8 bits 
>> of the
>> + * transaction ID.
>> + */
>> +#define STRATIX10_SIP_SMC_SET_TRANSACTIONID_X1(transaction_id) \
>> +    ((transaction_id) & 0xff)
>> +
>> +/* Macro to get the SDM mailbox error status */
>> +#define STRATIX10_GET_SDM_STATUS_CODE(status) ((status) & 0x3ff)
>> +


>> +    if (!chan || !tx_handle || !data)
>> +        return -EINVAL;
>> +
>> +    ctrl = chan->ctrl;
>> +    actrl = &ctrl->actrl;
>> +    achan = chan->async_chan;
>> +
>> +    if (!achan) {
>> +        dev_err(ctrl->dev, "Async channel not allocated\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    struct stratix10_svc_async_handler *handle =
> It is better to define all local variables at the top of the function.

Will make the change
>> +        (struct stratix10_svc_async_handler *)tx_handle;
>> +    if (!hash_hashed(&handle->next)) {
>> +        dev_err(ctrl->dev, "Invalid transaction handler\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    args.a0 = INTEL_SIP_SMC_ASYNC_POLL;
>> +    args.a1 =
>> +        STRATIX10_SIP_SMC_SET_TRANSACTIONID_X1(handle->transaction_id);
>> +
>> +    actrl->invoke_fn(actrl, &args, &handle->res);
>> +
>> +    /*clear data for response*/
>> +    memset(data, 0, sizeof(*data));
>> +
>> +    if (handle->res.a0 == INTEL_SIP_SMC_STATUS_OK) {
>> +        return 0;
>> +    } else if (handle->res.a0 == INTEL_SIP_SMC_STATUS_BUSY) {
>> +        dev_dbg(ctrl->dev, "async message is still in progress\n");
>> +        return -EAGAIN;
>> +    }
>> +
>> +    dev_err(ctrl->dev,
>> +        "Failed to poll async message ,got status as %ld\n",
>> +        handle->res.a0);
>> +    return -EINVAL;
>> +}
>> +EXPORT_SYMBOL_GPL(stratix10_svc_async_poll);
>> +


>>       init_completion(&controller->complete_status);
>> +    ret = stratix10_svc_async_init(controller);
>> +    if (ret)
>> +        dev_dbg(dev, "Intel Service Layer Driver: Error on 
>> stratix10_svc_async_init %d\n",
>> +            ret);
> If the call to stratix10_svc_async_init fails, it seems like 
> stratix10_sv_drv_probe should fail too.

Will make the change

> 
>      if (ret)
>          return dev_err_probe(dev, ret, "Intel Service Layer Driver: 
> Error on stratix10_svc_async_init\n"); Matthew Gerlach
>> +
>>       fifo_size = sizeof(struct stratix10_svc_data) * 
>> SVC_NUM_DATA_IN_FIFO;
>>       ret = kfifo_alloc(&controller->svc_fifo, fifo_size, GFP_KERNEL);
>>       if (ret) {
>> @@ -1470,6 +2106,8 @@ static void stratix10_svc_drv_remove(struct 
>> platform_device *pdev)
>>       struct stratix10_svc *svc = dev_get_drvdata(&pdev->dev);
>>       struct stratix10_svc_controller *ctrl = platform_get_drvdata(pdev);
>> +    stratix10_svc_async_exit(ctrl);
>> +
>>       of_platform_depopulate(ctrl->dev);
>>       platform_device_unregister(svc->intel_svc_fcs);
>> diff --git a/include/linux/firmware/intel/stratix10-smc.h b/include/ 
>> linux/firmware/intel/stratix10-smc.h
>> index 

Regards
Mahesh Rao