On Mon, Sep 07, 2020 at 08:55:13AM +0100, Fuad Tabba wrote: > Hi Sudeep, > > I understand that this is an RFC, but I have a few suggestions about > how the FF-A interface code might be structured. See below. > > On Sat, Aug 29, 2020 at 6:09 PM Sudeep Holla <sudeep.holla@xxxxxxx> wrote: > > > > This just add a basic driver that sets up the transport(e.g. SMCCC), > > checks the FFA version implemented, get the partition ID for self and > > sets up the Tx/Rx buffers for communication. > > > > Signed-off-by: Sudeep Holla <sudeep.holla@xxxxxxx> > > --- > > drivers/firmware/arm_ffa/Makefile | 3 +- > > drivers/firmware/arm_ffa/common.h | 23 +++ > > drivers/firmware/arm_ffa/driver.c | 288 ++++++++++++++++++++++++++++++ > > 3 files changed, 313 insertions(+), 1 deletion(-) > > create mode 100644 drivers/firmware/arm_ffa/common.h > > create mode 100644 drivers/firmware/arm_ffa/driver.c > > [...] > > + > > +/** > > + * FF-A specification mentions explicitly about '4K pages'. This should > > + * not be confused with the kernel PAGE_SIZE, which is the translation > > + * granule kernel is configured and may be one among 4K, 16K and 64K. > > + */ > > +#define FFA_PAGE_SIZE SZ_4K > > +/* Keeping RX TX buffer size as 64K for now */ > > +#define RXTX_BUFFER_SIZE SZ_64K > > The code/definitions above will be reused in other parts that deal > will FF-A (e.g., support for FF-A in KVM itself), so it might be good > to have it in a common header. I was wondering if it might even be a > good idea to reuse the Hafnium headers here (assuming I understand > licensing right): > https://review.trustedfirmware.org/plugins/gitiles/hafnium/hafnium/+/refs/heads/master/inc/vmapi/hf/ffa.h > I know few DTS files have dual license, but I am not sure about the headers and other source. But I agree on a common header and forgot to mention that explicitly but I am aware of, that we not only need common header, but some of the functions may also be reused. I am keeping them in the driver for now. We can move once we the KVM part also starts shaping up(before or after one of then gets merged, doesn't matter much) > > + > > +static ffa_fn *invoke_ffa_fn; > > + > > +static const int ffa_linux_errmap[] = { > > + /* better than switch case as long as return value is continuous */ > > + 0, /* FFA_RET_SUCCESS */ > > + -EOPNOTSUPP, /* FFA_RET_NOT_SUPPORTED */ > > + -EINVAL, /* FFA_RET_INVALID_PARAMETERS */ > > + -ENOMEM, /* FFA_RET_NO_MEMORY */ > > + -EBUSY, /* FFA_RET_BUSY */ > > + -EINTR, /* FFA_RET_INTERRUPTED */ > > + -EACCES, /* FFA_RET_DENIED */ > > + -EAGAIN, /* FFA_RET_RETRY */ > > + -ECANCELED, /* FFA_RET_ABORTED */ > > +}; > > + > > +static inline int ffa_to_linux_errno(int errno) > > +{ > > + if (errno < FFA_RET_SUCCESS && errno >= FFA_RET_ABORTED) > > + return ffa_linux_errmap[-errno]; > > + return -EINVAL; > > +} > > Hardcoding the range check to be bound by FFA_RET_ABORTED could cause > some issues in the future if more error codes are added. It might be > safer to check against the number of elements in ffa_linux_errmap. > Makes sense, will see how I can fix that. -- Regards, Sudeep
