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

[Mahesh Rao]

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@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
> >          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@xxxxxxxxxx>
> > Reviewed-by: Matthew Gerlach <matthew.gerlach@xxxxxxxxxx>
> > Signed-off-by: Dinh Nguyen <dinguyen@xxxxxxxxxx>
> > ---
> >   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