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 [off-list ref] wrote:

quoted

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 <redacted>
---
 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

[...]

quoted

+
+/**
+ * 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)

quoted

+
+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