Re: [PATCH RESEND v2 4/4] firmware: stratix10-svc: Add for SDM mailbox doorbell interrupt

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

Hi Dinh,

On 01-09-2025 08:46 pm, Dinh Nguyen wrote:
> 
> 
> On 8/12/25 07:59, Mahesh Rao via B4 Relay wrote:
>> From: Mahesh Rao <mahesh.rao@altera.com>
>>
>> Add support for SDM (Secure Device Manager) mailbox
>> doorbell interrupt for async transactions. On interrupt,
>> a workqueue is triggered which polls the ATF for
>> pending responses and retrieves the bitmap of all
>> retrieved and unprocessed transaction ids of mailbox
>> responses from SDM. It then triggers the corresponding
>> registered callbacks.
> 
> You should configure your editor to use a full 80-char width. Why stop
> at ~50? When you're unsure, look at that other commit logs from other
> developers. If yours doesn't look similar, its probably a problem. For
> example:
> 
> "Add support for SDM (Secure Device Manager) mailbox doorbell interrupt
> for async transactions. On interrupt, a workqueue is triggered which
> polls the ATF for pending responses and retrieves the bitmap of all
> retrieved and unprocessed transaction ids of mailbox responses from SDM.
> It then triggers the corresponding registered callbacks."

ok, will do.

> 
>>
>> Signed-off-by: Mahesh Rao <mahesh.rao@altera.com>
>> Reviewed-by: Matthew Gerlach <matthew.gerlach@altera.com>
>> ---
>>   drivers/firmware/stratix10-svc.c             | 117 +++++++++++++++++ 
>> +++++++---
>>   include/linux/firmware/intel/stratix10-smc.h |  23 ++++++
>>   2 files changed, 130 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/ 
>> stratix10-svc.c
>> index 


>> +    struct work_struct async_work;
>>       DECLARE_HASHTABLE(trx_list, ASYNC_TRX_HASH_BITS);
>>   };
>> @@ -1709,14 +1716,81 @@ static inline void stratix10_smc_1_2(struct 
>> stratix10_async_ctrl *actrl,
>>       arm_smccc_1_2_smc(args, res);
>>   }
>> +static irqreturn_t stratix10_svc_async_irq_handler(int irq, void 
>> *dev_id)
>> +{
>> +    struct stratix10_svc_controller *ctrl = dev_id;
>> +    struct stratix10_async_ctrl *actrl = &ctrl->actrl;
>> +
>> +    queue_work(system_bh_wq, &actrl->async_work);
>> +    disable_irq_nosync(actrl->irq);
>> +    return IRQ_HANDLED;
>> +}
> 
> Add a newline here.
ok, will make this change.

> 
>> +/**
>> + * stratix10_async_workqueue_handler - Handler for the asynchronous
>> + * workqueue in Stratix10 service controller.
>> + * @work: Pointer to the work structure that contains the asynchronous
>> + *        workqueue handler.
>> + * This function is the handler for the asynchronous workqueue. It 
>> performs
>> + * the following tasks:
>> + * - Invokes the asynchronous polling on interrupt supervisory call.
>> + * - On success,it retrieves the bitmap of pending transactions from 
>> mailbox
>> + *   fifo in ATF.
>> + * - It processes each pending transaction by calling the corresponding
>> + *   callback function.
>> + *
>> + * The function ensures that the IRQ is enabled after processing the 
>> transactions
>> + * and logs the total time taken to handle the transactions along 
>> with the number
>> + * of transactions handled and the CPU on which the handler ran.


>> -    int ret;
>> +    int ret, irq;
>>       if (!controller)
>>           return -EINVAL;
>> @@ -1775,6 +1849,22 @@ static int stratix10_svc_async_init(struct 
>> stratix10_svc_controller *controller)
>>       hash_init(actrl->trx_list);
>>       atomic_set(&actrl->common_achan_refcount, 0);
>> +    irq = of_irq_get(dev_of_node(dev), 0);
>> +    if (irq < 0) {
> 
> 0 is a failing value as well.

will add this.

> 
>> +        dev_warn(dev, "Failed to get IRQ, falling back to polling 
>> mode\n");
>> +    } else {


>> - * the asynchronous ID pool and invoke function pointers to NULL.
>> + * This function performs the necessary cleanup for the asynchronous 
>> service
>> + * controller. It checks if the controller is valid and if it has been
>> + * initialized. Also If the controller has an IRQ assigned, it frees 
>> the IRQ
>> + * and flushes any pending asynchronous work. It then locks the 
>> transaction
>> + * list and safely removes and deallocates each handler in the list.
>> + * The function also removes any asynchronous clients associated with 
>> the
>> + * controller's channels and destroys the asynchronous ID pool. 
>> Finally, it
>> + * resets the asynchronous ID pool and invoke function pointers to NULL.
> 
> Did you mean to repeat the same paragraph twice?
> 
> Dinh