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

[Mahesh Rao]

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@xxxxxxxxxx>
> >
> > 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