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

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

Hi Greg,

On 19-08-2025 04:38 pm, Greg KH wrote:
> On Tue, Jul 22, 2025 at 11:30:43AM -0500, Dinh Nguyen 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
>>          to the service channel.
>> - stratix10_svc_remove_async_client: Removes an
>>          asynchronous client from the service
>>          channel.
>> - stratix10_svc_async_send: Sends an asynchronous
>>          message to the SDM mailbox in EL3 secure
>>          firmware.
>> - stratix10_svc_async_poll: Polls the status of an
>>          asynchronous service request in EL3 secure
>>          firmware.
>> - stratix10_svc_async_done: Marks an asynchronous
>>          transaction as complete and free's up the
>>          resources.
>>
>> These changes enhance the functionality of the
>> Stratix10 service channel by allowing for more
>> efficient and flexible communication with the
>> firmware.
>>
>> Signed-off-by: Mahesh Rao <mahesh.rao@altera.com>
>> Reviewed-by: Matthew Gerlach <matthew.gerlach@altera.com>
>> Signed-off-by: Dinh Nguyen <dinguyen@kernel.org>
>> ---
>>   drivers/firmware/stratix10-svc.c              | 657 +++++++++++++++++-
>>   include/linux/firmware/intel/stratix10-smc.h  |  24 +
>>   .../firmware/intel/stratix10-svc-client.h     |  88 +++
>>   3 files changed, 765 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
>> index a8c57963da99..4480ba8b2e11 100644
>> --- a/drivers/firmware/stratix10-svc.c
>> +++ b/drivers/firmware/stratix10-svc.c
>> @@ -4,9 +4,11 @@
>>    * Copyright (C) 2025, Altera Corporation
>>    */
>>   
>> +#include <linux/atomic.h>
>>   #include <linux/completion.h>
>>   #include <linux/delay.h>
>>   #include <linux/genalloc.h>
>> +#include <linux/hashtable.h>
>>   #include <linux/io.h>
>>   #include <linux/kfifo.h>
>>   #include <linux/kthread.h>
>> @@ -44,6 +46,47 @@
>>   #define STRATIX10_RSU				"stratix10-rsu"
>>   #define INTEL_FCS				"intel-fcs"
>>   
>> +/*Maximum number of SDM client IDs.*/
>> +#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
> 
> Tabs for defines to make them more readable?
> 
> And use spaces in your comments please.
> 
> 
>> +/* Total number of transaction IDs, which is a combination of
>> + * client ID and job ID.
>> + */
> 
> Wrong comment style.
> 
> I stopped reading here :(

I will fix this in the next revision.
> 
> thanks,
> 
> greg k-h